Code review comment for lp:~cemil-azizoglu/mir/mir-on-x

Some high level first impressions:

~~~~~~~~~

The automatic module detection doesn't work correctly. In case of a priority tie, the first module found (in filesystem order) is selected. If that module is mesa-kms then mesa-x11 can never be loaded unless forced with the platform-graphics-lib option. We need to change mesa-kms and/or mesa-x11 to report different priorities depending on the environment.

~~~~~~~~~

I can't get any output on intel for EGL clients and only the first frame for software clients (I think these are known issues?).

~~~~~~~~~

196 + bool const use_dma_buf_extension)

It's better to use an enum for readability, since true/false values are very uninformative, e.g.:

return std::make_shared<mgm::BufferAllocator>(gbm.device, mgm::BypassOption::prohibited, false);

Compare the existing second (enum) to the new third (bool) option.

~~~~~~~~~

253 + return mir::Fd{IntOwnedFd{fd}};

We should dup() this fd to maintain the interface (i.e., that authenticated_fd() returns a new, independent fd). Ideally we would change the calling code to properly use mir::Fds, but that is beyond the scope of this MP.

~~~~~~~~~

333 + DRMHelper(bool const use_render_node)

Similarly, better to use an enum instead of a bool.

~~~~~~~~~

353 -mgm::IpcOperations::IpcOperations(std::shared_ptr<DRMAuthentication> const& drm_auth) :
355 +mgm::IpcOperations::IpcOperations(std::shared_ptr<DRMAuthentication> const& drm, bool const authenticate) :
...
364 + if (!authenticate)
365 + BOOST_THROW_EXCEPTION(
366 + std::runtime_error("Invalid platform operation"));

We should let the DRMAuthenticator succeed or fail here on its own, no need for changing the IpcOperations code at all. I think the proposed version of DRMHelper will fail in the right way (i.e., throw for auth_magic(), succeed for auth_fd()).

~~~~~~~~~

394 + if (!authenticate)
395 + {
396 + auto package = std::make_shared<mg::PlatformIPCPackage>();
397 + package->ipc_fds.push_back(dup(drm->authenticated_fd()));
398 + return package;
399 + }
400 + return std::make_shared<MesaPlatformIPCPackage>(drm->authenticated_fd());

The two paths are functionally equivalent except 1. the dup() in the first path and 2. the first path leaks fds. As suggested earlier, it's better to dup inside authenticated_fd(), and by doing so we wouldn't need to change the code here at all.