[PATCH xserver 1/3] xwayland: Decouple GBM from glamor

[Lyude Paul]

On Tue, 2018-04-24 at 10:35 +0100, Daniel Stone wrote:
> Hi,
> 
> On 20 April 2018 at 19:38, Adam Jackson <ajax at redhat.com> wrote:
> > This takes all of the gbm related code in wayland-glamor.c and moves it
> > into it's own EGL backend for Xwayland, xwayland-glamor-gbm.c.
> > Additionally, we add the egl_backend struct into xwl_screen in order to
> > provide hooks for alternative EGL backends such as nvidia's EGLStreams.
> 
> I do think the end result of this commit is better; there are a lot of
> good cleanups in here. It would be much easier to review next time
> though, if this was broken into a few separate commits. There is mass
> code motion, resequencing of functions, reordering of struct members,
> minor changes of struct declarations (e.g. void * -> EGLImage in
> xwl_pixmap), a lot of formatting changes, and other cleanups like
> moving variable declarations into child blocks. It's taken until now
> to review it because I've got four panes open with new and old code
> side by side, with quite a lot of back and forth.
> 
> > +_X_EXPORT Bool
> > +glamor_get_formats(ScreenPtr screen,
> > +                   CARD32 *num_formats, CARD32 **formats)
> > +{
> > +    struct xwl_screen *xwl_screen = xwl_screen_get(screen);
> > +    struct xwl_gbm_private *xwl_gbm = xwl_gbm_get(xwl_screen);
> > +    int i;
> > +
> > +    if (!xwl_gbm->dmabuf_capable || !xwl_gbm->dmabuf)
> > +        return FALSE;
> > +
> > +    if (xwl_screen->num_formats == 0) {
> > +       *num_formats = 0;
> > +       return TRUE;
> > +    }
> 
> Changes from ac48724639e0a6a9e421b3b4e545d8506fd6bf5dost in rebase.
> 
> > +_X_EXPORT Bool
> > +glamor_get_modifiers(ScreenPtr screen, CARD32 format,
> > +                     CARD32 *num_modifiers, uint64_t **modifiers)
> > +{
> > +    struct xwl_screen *xwl_screen = xwl_screen_get(screen);
> > +    struct xwl_gbm_private *xwl_gbm = xwl_gbm_get(xwl_screen);
> > +    struct xwl_format *xwl_format = NULL;
> > +    int i;
> > +
> > +    if (!xwl_gbm->dmabuf_capable || !xwl_gbm->dmabuf)
> > +        return FALSE;
> > +
> > +    /* Explicitly zero the count as the caller may ignore the return
> > value */
> > +    *num_modifiers = 0;
> 
> Changes from b36a14c0b0e7e38406622eb5ff0666a8b8bc50f4 misplaced in rebase.
> 
> > +static void
> > +xwl_drm_handle_device(void *data, struct wl_drm *drm, const char *device)
> > +{
> > +   struct xwl_screen *xwl_screen = data;
> > +   struct xwl_gbm_private *xwl_gbm = xwl_gbm_get(xwl_screen);
> > +   drm_magic_t magic;
> > +
> > +   xwl_gbm->device_name = strdup(device);
> > +   if (!xwl_gbm->device_name) {
> > +       xwl_glamor_gbm_cleanup(xwl_screen);
> > +       return;
> > +   }
> > +
> > +   xwl_gbm->drm_fd = open(xwl_gbm->device_name, O_RDWR | O_CLOEXEC);
> > +   if (xwl_gbm->drm_fd == -1) {
> > +       ErrorF("wayland-egl: could not open %s (%s)\n",
> > +              xwl_gbm->device_name, strerror(errno));
> > +       xwl_glamor_gbm_cleanup(xwl_screen);
> > +       return;
> > +   }
> > +
> > +   if (is_fd_render_node(xwl_gbm->drm_fd)) {
> > +       xwl_gbm->fd_render_node = 1;
> > +       xwl_screen->expecting_event--;
> > +   } else {
> > +       drmGetMagic(xwl_gbm->drm_fd, &magic);
> > +       wl_drm_authenticate(xwl_gbm->drm, magic);
> > +   }
> > +}
> 
> e.g. here, the change to expecting_event is unnecessary; the previous
> code explicitly decremented and re-incremented to make it clear which
> event was which, and the change meant I had to double back and read
> through the whole init flow again. The current code is still correct,
> and I don't care enough to ask for it to be made back the way it was,
> but in future please try to keep these kinds of subtle changes
> separate from mass code motion.

Will keep that in mind for the future, thanks!
> 
> The rest is:
> Reviewed-by: Daniel Stone <daniels at collabora.com>
> 
> Cheers,
> Daniel
-- 
Cheers,
	Lyude Paul