xen-troops / displ_be Goto Github PK

I use your displ_be for graphical output from PV and PVH domains. I use it to run some domU with full desktop environment. Both in Wayland mode and with DRM. Great software, thanks!

I have not yet succeeded in displaying the virtual console. And the output of the boot messages even less.
Is that even possible?

Dear Oleksandr,

Be so kind to end up with PRs to the related repositories to get this functionality integrated.

here is common improvement - use the switch , because req.operation may have the invalid value
(this->*sCmdTable.at(req.operation))(req, rsp);
class DisplayCommandHandler::

class Registry has the method void* bind(const wl_interface interface)
Some cases to use are below ;
SharedMemory::init
mWlSharedMemory = static_cast<wl_shm>(bind(&wl_shm_interface));

WaylandKms::init()
mWlKms = static_cast<wl_kms*>(bind(&wl_kms_interface));

...

There are 2 prposals

1. add the verification for the nullptr in bind method
2. change the method like following to avoid 'void*" as type of the result
   template
   T* bind(const wl_interface interface)
   {
   T res= nullptr;
   if(interface) {
   res= static_cast<T*>(wl_registry_bind(mWlRegistry, mId, interface, mVersion));
   }
   return res;
   }

benefits - it allows to encapsulate the deduction of the returned type

of course.some changes are required in other classes to adopt the behavior like

1. check the result for a nullptr (really not required, because there is a reference at the object)
2. the call of function should look like
   WaylandKms::init()
   mWlKms = bind<wl_kms>(&wl_kms_interface));

src/displayBackend/drm/Display.hpp has number of "explicit" key word usages, which are not needed.
This needs to be reviewed and fixed.
Reported by @iusyk

If segmentation fault occurs backend doesn't go to closed state. On next restart it sees that frontend is in connected state and tries to create same ring buffer. The solution could be: update current frontend state on backend restart and do not call frontend state changed callback.

Reported by @rshym

Because backend overrides default segmentation fault handler, crash dumb is not generated.
To keep both handlers following solution could be applied:

http://www.alexonlinux.com/how-to-handle-sigsegv-but-also-generate-core-dump

Currently in case unsupported pixel format, wayland returns error on polling fd which treated as general wayland error. Backend is closed in this case. For proper handling, backend shell store supported pixel format and perform check on buffer creating.

Description

Event handler OnDevice is registered in wl_drm_listener
The code of the handler make the instance(in heap) of type DisplayWayland.
The class DisplayWayland inherits the Drm::Display, the ctor of which can throw the exception (type Exception)
In general - event handler must not throw the exception because there are not any guarantees that C code can handle it
see code

void WaylandZCopy::onDevice(const string& name)
{
lock_guard lock(mMutex);

LOG(mLog, DEBUG) << "onDevice name: " << name;

mDrmDevice.reset(new Drm::DisplayWayland(name));

authenticate();

}

Display::Display ()
{
...
if (mDrmFd < 0)
{
throw Exception("Cannot open DRM device: " + mName, errno);
}
...
if (drmGetCap(mDrmFd, DRM_CAP_DUMB_BUFFER, &hasDumb) < 0 || !hasDumb)
{
throw Exception("Drm device does not support dumb buffers", errno);
}
}

Solution

Wrap code inside OnDevice in block try{}catch(const Exception&){}

There is the following code in function 'int main(int argc, char argv[])'
for(size_t i = 0; i < db1->getSize() / 4; i++)
{
(reinterpret_cast<uint32_t>(db1->getBuffer()))[i] = 0xFFAACC;
}
No sense to use the reinterpret_cast here, the following code allows to avoid of reinterpret_cast usage.
uint32_t *uiBegin= static_cast<uint32_t * >(db1->getBuffer());
uint32_t *uiEnd= uiBegin + size/sizeof(uint32_t);
std::for_each(uiBegin, uiEnd, [](uint32_t &v){ v= 0xFFAACC;});

COVESA/wayland-ivi-extension@64d5b40

Description

The dtor has the call mConnector->getName(), where mConnector is the pointer that is not checked for the nullptr (see the issue in DisplayCommandHandler::DisplayCommandHandler
DisplayCommandHandler::~DisplayCommandHandler()
{
LOG(mLog, DEBUG) << "Delete command handler, connector name: "
<< mConnector->getName();

mConnector.reset();

}

Solution

wrap dtor code in if(mConnector){}

DisplayBackend::DisplayBackend(DisplayPtr display,
const string& deviceName) :
BackendBase("DisplBackend", deviceName),
mDisplay(display)
{
// mDisplay maybe not initilized
// reasonable to add
if(mDisplay) {
mDisplay->start();
}
else {
// inform about the error (throw an exception) or change the object state at invalid
}
}

Memcpy impact on PV DRM performance should be measured following this discussion:
https://lists.xenproject.org/archives/html/xen-devel/2016-11/msg01298.html

Currently DRM device path is hardcoded.
It should be put to command line or config file.

The class ConnectorBase has some private methods that do not use the class members or functions. All these methods should be declared as static or moved in .cpp file, because they are not a part of the class interface

We've agreed that for this special case the scenario should be as following:

• to identify this case BE will receive create dbuf req with w = h = 0;
• BE doesn't create dbuf but stores refs (or buffer?) in map associated with dbuf cookie;
• on fbuf create req BE checks dbuf cookie is it already created or not. If it is not created BE creates dbuf with previously stored refs (or buffer) and w, h, same as for fbuf but bpp should be calculated from pixel format.
• on dbuf delete req: BE should delete the dbuf.

For multi touch device onUp event is only reported for the very first id in series, e.g. for 2 fingers:
onUp id=0 and no id=1.
Workaround:

diff --git a/src/displayBackend/wayland/SeatTouch.cpp b/src/displayBackend/wayland/SeatTouch.cpp
index 01965d2be68a..c47580440867 100644
--- a/src/displayBackend/wayland/SeatTouch.cpp
+++ b/src/displayBackend/wayland/SeatTouch.cpp
@@ -117,7 +117,7 @@ void SeatTouch::onUp(uint32_t serial, uint32_t time, int32_t id)
}
}

•   mCurrentCallback = mCallbacks.end();
  

+// mCurrentCallback = mCallbacks.end();
}

Extend exception with errcode field. And return it if req failed.

according to the code of base class , FrontendHandlerBase does not expect any exceptions, but, potentially, DisplayFrontendHandler::onBind may throw the exception if

1. ctor of BuffersStorage throws an exception (now it may be just a bad_alloc, but who knows how code will be changed)
2. DisplayFrontendHandler::createConnector can be source of invalid_argument, because there is a call of getXenStore().readInt inside of the function. readInt uses stoi and stoi can throws the exception if string is invalid.
3. DisplayFrontendHandler::createConnector - the following call , potentially, my be the source of the exception CtrlRingBufferPtr ctrlRingBuffer(
   new CtrlRingBuffer(mDisplay,
   connector,
   bufferStorage,
   eventRingBuffer,
   getDomId(), port, ref));

Description

The ctor DisplayCommandHandler::DisplayCommandHandler takes the followinf input parameters
-DisplayPtr display,
-ConnectorPtr connector,
-BuffersStoragePtr buffersStorage,
-EventRingBufferPtr eventBuffer
all of the parameters are the pointers (rather shared_ptr)
The problems

1. parameters must be checked for a nullptr
2. There is the following code
   LOG(mLog, DEBUG) << "Create command handler, connector name: "
   << mConnector->getName();
   Of course in the case of null pointer - this is crash

Solution

1. check all parameters for a nullptr
2. wrap the logging in block if(mConnector){}

It is sometimes useful not to redirect logs, but request logs to be collected such as DEBUG level was set

src/displayBackend/wayland/Display.cpp:void Display::registryHandler(wl_registry *registry, uint32_t id, const std::string& interface, uint32_t version)
When registry is being parsed multiple instances of classes are created with new which may fail. These need to be reviewed if proper error handling is implemented.
Reported by @iusyk

displ_be -c ../displ_be.cfg
ivi_controller not available
[Warning] The ilm_control_context is already destroyed
01.01.70 00:03:27.549 | Display | WRN - Can't initialize ILM: failed. ILM capability will be disabled.
01.01.70 00:03:27.557 | DisplBackend | INF - Create new frontend: Dom(1/0)
01.01.70 00:03:27.558 | DisplFrontend | INF - Dom(1/0) Set backend state to: [Initializing]
01.01.70 00:03:27.560 | DisplFrontend | INF - Dom(1/0) Frontend state changed to: [Initializing]
01.01.70 00:03:27.560 | DisplFrontend | INF - Dom(1/0) Set backend state to: [InitWait]
01.01.70 00:03:27.576 | DisplFrontend | INF - Dom(1/0) Frontend state changed to: [Initialized]
01.01.70 00:03:27.577 | DisplFrontend | INF - Dom(1/0) Add ring buffer, ref: 2476, port: 20
01.01.70 00:03:27.579 | DisplFrontend | INF - Dom(1/0) Add ring buffer, ref: 2475, port: 19
01.01.70 00:03:27.580 | DisplFrontend | INF - Dom(1/0) Set backend state to: [Connected]
01.01.70 00:03:27.599 | DisplFrontend | INF - Dom(1/0) Frontend state changed to: [Connected]
^C01.01.70 00:11:15.003 | DisplFrontend | INF - Dom(1/0) Set backend state to: [Closing]
01.01.70 00:11:15.079 | Main | ERR - Segmentation fault!
displ_be(_Z19segmentationHandleri+0x74)[0x4298b4]
linux-vdso.so.1(__kernel_rt_sigreturn+0x0)[0xffffb73866c0]
displ_be(_ZN21DisplayCommandHandler13sendFlipEventEm+0x224)[0x436ce4]
displ_be(_ZN7Wayland7Surface12frameHandlerEv+0x13c)[0x45ea8c]
/usr/lib/libffi.so.6(ffi_call_SYSV+0x64)[0xffffb6d8de14]
/usr/lib/libffi.so.6(ffi_call+0x9c)[0xffffb6d8e73c]
/usr/lib/libwayland-client.so.0(+0x8c08)[0xffffb7282c08]

Method getEDID does not change the state of the instance, so, it is reasonable to make it const.

Scenario:

1. Start BE
2. Start FE
3. Stop BE
4. Start BE
   29.06.17 09:57:38.223 | RingBuffer | DBG - Send event, port: 13, prod: 0, cons: 2139, num events: 51
   prod: 0, cons: 2139
   In this scenario care must be taken of the state machine of xen_kbdfront which initializes ring buffer once at the first start, e.g. its state machine doesn't see that BE has recycled.
   This makes it stuck on the very first event from BE, because cons > prod and it tries to read all the events until its cons wraps and matches prod. This renders guest machine unusable

Fix could be for the BE to read cons at start and set prod = cons

wl_shell is considered as deprecated across the entire Wayland community, with new clients being designed to use the new xdg_shell protocol.

It still possible to work with ws_shell using EXTRA_OEMESON:append = " -Ddeprecated-wl-shell=true" but quite possible that this workaround will be not available in further versions of weston

Problem: compositor has to be verified for the nullptr, otherwise, in the case of nullptr the behavior is unexpected

Description
We have the following sequence

Surface::Surface (wl_compositor* compositor)
->wl_compositor_create_surface(compositor)
->wl_proxy_marshal_constructor((struct wl_proxy *) wl_compositor,...)
->wl_proxy_marshal_array_constructor()
->wl_proxy_marshal_array_constructor(struct wl_proxy *proxy,...)
->wl_proxy_marshal_array_constructor_versioned(struct wl_proxy *proxy,...)

The wl_proxy_marshal_array_constructor_versioned starts with the following code
"
pthread_mutex_lock(&proxy->display->mutex);
message = &proxy->object.interface->methods[opcode];
....
"

proxy is the composer in Surface ctor, so, in the case of NULL - behavior undefined

It is clear, the instance of the class can be created just via the factory (because of private ctor)
but, at least assert has to be implemented, to be sure that value is correct

Proposed improvement

1. add assert (compositor != nullptr) or if (compositor == nullptr) {throw std::invalid_parameter("compositor can not be nullptr.");}

2. add explicit to the declaration of Surface ctor.

Method DevInputBase::init()
There is the following code:
if (ioctl(mFd, EVIOCGRAB, reinterpret_cast<void*>(1)))
{
throw XenBackend::Exception("Grabbed by another process", EBUSY);
}

ioctl(mFd, EVIOCGRAB, reinterpret_cast<void*>(0));

It is proposed the following to avoid reinterpret_cast

template T& as_lvalue( T&& t ) { return t; }

ioctl(mFd, EVIOCGRAB,, &as_lvalue(int{1})
or
ioctl(mFd, EVIOCGRAB,, &as_lvalue(int{0})

Introduction

Base class DisplayItf::FrameBuffer has the method getHandle, which is used in some ways

1. return the frame id
2. return the address of the pointer
3. return dummy values

This approach brings the following problems
1.Using reinterpret_cast to get the desirable value
2.Deduction of the type from uintptr_t
3.Not safe code with unpredictable behavior (in the code it is possoble to deduct any type from uintptr_t)

Description

For the purpose of data buffers handling, the real type of FrameBuffer depends on the DisplayMode (DRM or Wayland)
The following class hierachy has been developed to work with the both modes:

Base class DisplayItf::FrameBuffer has the following declaration of the method getHandle

DisplayItf::FrameBuffer
...
/**

• Gets handle
  */
  virtual uintptr_t getHandle() const = 0;
  ...

Children

a)Drm::FrameBuffer

uintptr_t getHandle() - method is used to return the frame id(integer), so no issue here, because this value is used to save and return frame ID

b)Wayland::WlBuffer (and his children)

/**

• Gets handle
  */
  uintptr_t getHandle() const override
  {
  return reinterpret_cast<uintptr_t>(mWlBuffer);
  }

where mWlBuffer is the pointer at wl_buffer*

The sample of the usage

wl_surface_attach(mWlSurface, reinterpret_cast<wl_buffer*>(frameBuffer->getHandle()), 0, 0);

Method is used just to cast pointer to the int to be casted back to the original (wl_buffer*) value !!!

The main problem is the method getHandle, this method should be removed from the base class, because it does not have any reasonable sense - it maybe ID, pointer, dummy value.

Solution

Remove getHandle from DisplayItf::FrameBuffer.

Instead of getHandle method:

1)Add the virtual uint32_t FrameBuffer::getID()const {...} to the DRM FrameBuffer
2)Add wl_buffer* Wayland::WlBuffer::getWLBuffer()const {...} to WlBuffer
3)To support the new types, use dynamic_cast to deduct the correct type
4) Fix the following function (Surface::draw), because code code is not safe (see comments marked IU)

mBuffer = dynamic_cast<WlBuffer*>(frameBuffer.get());

if (nullptr == mBuffer)
{
    return ; // or inform about the error
}
    mBuffer->setSurface(this);
wl_surface_damage(mWlSurface, 0, 0,  frameBuffer->getWidth(), frameBuffer->toWLBuffer());
	wl_surface_attach(mWlSurface, frameBuffer->getWLBuffer(),  0, 0);

5)Drm::Connector::init

change the code
auto fbId = frameBuffer->getHandle();

at

auto fBuffer= dynamic_castDRM::FrameBuffer*frameBuffer.get())
if(nullptr == fBuffer) {
throw Exception("Buffer type has to be DRM::FrameBuffer*")
}
auto fbId = fBuffer->getID();

6)DRM::Connector::pageFlip
auto fbId = frameBuffer->getHandle();

at
auto fBuffer= dynamic_castDRM::FrameBuffer*frameBuffer.get())
if(nullptr == fBuffer) {
throw Exception("Buffer type has to be DRM::FrameBuffer*")
}
auto fbId = fBuffer->getID();

src/displayBackend/wayland/WaylandZCopy.hpp:
Classes derived from WaylandZCopy have destructors which may result in undefined behavior:
Deleting a derived class object using a pointer to a base class that has a non-virtual destructor results in undefined behavior. To correct this situation, the base class should be defined with a virtual destructor.
Reported by @lorc

Investigate following input cases:

• 2 displays, 2 touch screens;
• input device hot plug;

After running Xorg and xclock, the screen does not output the clock, it's keeps graying out.
Thanks ！

The Xorg runing log：
/# Xorg :0

X.Org X Server 1.20.11
X Protocol Version 11, Revision 0
Build Operating System: linux Ubuntu
Current Operating System: Linux vm1-desktop 5.11.0-34-generic #3620.04.1-Ubuntu SMP Fri Aug 27 08:06:32 UTC 2021 x86_64
Kernel command line: root=/dev/xvda5
Build Date: 06 July 2021 10:17:51AM
xorg-server 2:1.20.11-1ubuntu120.04.2 (For technical support please see http://www.ubuntu.com/support)
Current version of pixman: 0.38.4
Before reporting problems, check http://wiki.x.org
to make sure that you have the latest version.
Markers: (--) probed, (**) from config file, (==) default setting,
(++) from command line, (!!) notice, (II) informational,
(WW) warning, (EE) error, (NI) not implemented, (??) unknown.
(==) Log file: "/var/log/Xorg.0.log", Time: Sun Sep 26 13:41:45 2021
(==) Using system config directory "/usr/share/X11/xorg.conf.d"
MESA-LOADER: failed to retrieve device information
MESA-LOADER: failed to open xendrm-du: /usr/lib/dri/xendrm-du_dri.so: cannot open shared object file: No such file or directory (search paths /usr/lib/x86_64-linux-gnu/dri:$${ORIGIN}/dri:/usr/lib/dri)
failed to load driver: xendrm-du
(II) modeset(0): Initializing kms color map for depth 24, 8 bpc.

Description

There are 2 issues with the ctor of class CtrlRingBuffer

1. The input parameters are the pointers, but they are not checked for a nullptr, what brings the risk of unexpected crash
2. Parameter 'connector' must be removed
   The reason of this is because connector can be used just when it is initialized, but the ctor takes always not initialized input parameter, see the code
   auto width = stoi(resolution.substr(0, pos));
   auto height = stoi(resolution.substr(++pos));
   auto connector = mDisplay->createConnector(getDomId(), id, width, height);
   CtrlRingBufferPtr ctrlRingBuffer(
   new CtrlRingBuffer(mDisplay,
   connector,
   bufferStorage,
   eventRingBuffer,
   getDomId(), port, ref));
   There is not any sense to create the not initialized connector from mDisplay, and put both parameters in the ctor of CtrlRingBuffer. This behavior just increase the risk of the exceptions (what can be avoided) and crashes(pointers are not checked for the nullptr), besides - all code to work with connector must be wrapped in try{}catch{} block or it is necessary to , because connector is not initialized to check the connector state.

Solution

1. Remove input parameter 'connector'
2. Add the pointers validation in ctor of CtrlRingBuffer
3. Create the conenctor just when it is necessary, in our case this is DisplayCommandHandler::setConfig(const xendispl_req& req,
   xendispl_resp& rsp)
   Because, CtrlRingBuffer has the member mCommandHandler,
4. Remove input parameter 'connector' from the ctor declaration of DisplayCommandHandler
5. Instead of if([connector->]isInitialilised) add if(connector) where required. (and totaly remove from connector implementaiton)

xl destroy 1
[ 87.949763] xenbr0: port 2(vif1.0) entered disabled state
[ 88.015029] xenbr0: port 2(vif1.0) entered disabled state
[ 88.022057] device vif1.0 left promiscuous mode
[ 88.026714] audit: type=1700 audit(87.575:4): dev=vif1.0 prom=0 old_prom=256 auid=4294967295 uid=0 gid=0 ses=4294967295
[ 88.037574] xenbr0: port 2(vif1.0) entered disabled state
root@m3ulcb-xen-dom0:/xen-scripts#
root@m3ulcb-xen-dom0:/xen-scripts#
root@m3ulcb-xen-dom0:/xen-scripts# xl list
Name ID Mem VCPUs State Time(s)
Domain-0 0 512 4 r----- 18.8
(null) 1 0 4 --p--d 5.8
root@m3ulcb-xen-dom0:/xen-scripts#
root@m3ulcb-xen-dom0:~/xen-scripts# xenstore-ls -f
/tool = ""
/tool/xenstored = ""
/local = ""
/local/domain = ""
/local/domain/0 = ""
/local/domain/0/control = ""
/local/domain/0/control/feature-poweroff = "1"
/local/domain/0/control/feature-reboot = "1"
/local/domain/0/domid = "0"
/local/domain/0/name = "Domain-0"
/local/domain/0/backend = ""
/local/domain/0/backend/vdispl = ""
/local/domain/0/backend/vdispl/1 = ""
/local/domain/0/backend/vdispl/1/0 = ""
/local/domain/0/backend/vdispl/1/0/versions = "1"
/local/domain/0/backend/vdispl/1/0/frontend-id = "1"
/local/domain/0/backend/vdispl/1/0/frontend = "/local/domain/1/device/vdispl/0"
/local/domain/0/backend/vdispl/1/0/state = "4"
/vm = ""
/libxl = ""

The CPU usage of the kmscube process running in DomU exceeds 50%, but its CPU usage in Dom0 does not exceed 2%.

Remark:
Back-end command: displ_be -m DRM -z -v *:Debug
Displ_be disables zero copy (under zero copy, the front-end error: [drm:xen_drm_drv_dumb_create [drm_xen_front]] ERROR Failed to cre ate dumb buffer: -25. Back-end error: gnttab: error: ioctl DMABUF_EXP_FROM_REFS_V2 ioctlapproate failed: Inappro device )

Thanks !

Description

There are some methods that:

1. are not necessary
2. bring the implementation of additional code to support them
3. require the additional try{}catch{} blocks to handle the class behavior properly

The following methods should be removed or modified

1. release() - the sense of this method is to release the instance state, because of this, instance of class is not usable until user calls method 'init'.
   If it is necessary to change the state of the class instance in one transaction, it is better to use
   a) pattern state - implement the State class and use the method setState
   b) replace the method release with reset one, where reset should have the list of the parameters to activate a new class state
   This method MUST be replaced.

2. isInitialized - MUST be removed because the sense if this method, to be sure, that method init has been called after release
   This method is the source of the following problems
   a) isInitialized MUST be called in all methods of the class, besides, the exception MUST be thrown if isInitialized is false
   b) all calls of then class methods MUST be inside of block try{}catch{}
   c) because of 'a' and 'b', the source code grows and class is more complicated

Solution

Redesign the class to use the State pattern or method reset.

Description

The function stop() is called from dtor of DevInputBase
DevInputBase::~DevInputBase()
{
stop();
....
}
Where stop delegates the call to PollFd::stop
mPollFd->stop();

But function PollFd::stop may throw the exception in the case of unsuccess execution of the write operation
see the code:

void PollFd::stop()
{
uint8_t data = 0xFF;
if (write(mPipeFds[PipeType::WRITE], &data, sizeof(data)) < 0)
{
throw Exception("Error writing pipe", errno);
}
}

The behavior may be the source of application crash

Solution

Wrap the call of stop() in try{}catch(const Exception&){} block

Implement Display Manager which will handle surfaces from display backend and other applications.

• display backend and applications create surfaces with unique surface id;
• display manager arrange all surfaces based on surface id and global display policies;
• check GENIVI/wayland-ivi-extension LayerManagerControl as main solution.