On Tue, 2018-04-24 at 10:35 +0100, Daniel Stone wrote: > Hi, > > On 20 April 2018 at 19:38, Adam Jackson <a...@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 <dani...@collabora.com> > > Cheers, > Daniel -- Cheers, Lyude Paul _______________________________________________ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel