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

[Daniel Stone]

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.

The rest is:
Reviewed-by: Daniel Stone <daniels at collabora.com>

Cheers,
Daniel