Hi, Sorry, I've initially missed this email and only saw your request for follow-up.
On Wed, 2018-03-07 at 18:45 +0000, Emil Velikov wrote: > From: Emil Velikov <[email protected]> > > With earlier commit the required version was bumped to 2.4.89, thus > the > guards always evaluate to true. > > Fixes: e4e3447603b ("Add RandR leases with modesetting driver support > [v6]") > Cc: Keith Packard <[email protected]> > Cc: Daniel Stone <[email protected]> > Cc: Louis-Francis Ratté-Boulianne <[email protected]> > Signed-off-by: Emil Velikov <[email protected]> > --- > Daniel, Louis, > Seems like the existing code has subtle bugs ... right? You are right, there seems to be a couple spots where runtime checks should be added. > @@ -1638,15 +1630,14 @@ drmmode_shadow_destroy(xf86CrtcPtr crtc, > PixmapPtr rotate_pixmap, void *data) > static void > drmmode_crtc_destroy(xf86CrtcPtr crtc) > { > -#ifdef GLAMOR_HAS_DRM_ATOMIC > drmmode_mode_ptr iterator, next; > drmmode_crtc_private_ptr drmmode_crtc = crtc->driver_private; > > + // XXX: if (!...->atomic_modeset) return; > drmmode_prop_info_free(drmmode_crtc->props_plane, > DRMMODE_PLANE__COUNT); > xorg_list_for_each_entry_safe(iterator, next, &drmmode_crtc- > >mode_list, entry) { > drm_mode_destroy(crtc, iterator); > } > -#endif > } Please add that check. It should do nothing if atomic_modeset is FALSE. > @@ -1884,12 +1872,11 @@ drmmode_crtc_create_planes(xf86CrtcPtr crtc, > int num) > drmmode_crtc->num_formats = best_kplane->count_formats; > drmmode_crtc->formats = calloc(sizeof(drmmode_format_rec), > best_kplane->count_formats); > -#ifdef GLAMOR_HAS_DRM_MODIFIERS > + // XXX: Runtime check modifiers_supported or it's implied by > the successfull blob creation? > if (blob_id) { > populate_format_modifiers(crtc, best_kplane, blob_id); > } > else > -#endif > { > for (i = 0; i < best_kplane->count_formats; i++) > drmmode_crtc->formats[i].format = best_kplane- > >formats[i]; I don't think we need an additional check here. If the blob exists, it's safe to retrieve the formats/modifiers from it even if not used afterwards (can be used by the modesetting driver or DRI3). j > @@ -1927,7 +1911,7 @@ drmmode_crtc_init(ScrnInfoPtr pScrn, > drmmode_ptr drmmode, drmModeResPtr mode_res > drmmode_crtc->vblank_pipe = drmmode_crtc_vblank_pipe(num); > xorg_list_init(&drmmode_crtc->mode_list); > > -#ifdef GLAMOR_HAS_DRM_ATOMIC > + // XXX: if (...->atomic_modeset) { > props = drmModeObjectGetProperties(drmmode->fd, mode_res- > >crtcs[num], > DRM_MODE_OBJECT_CRTC); > if (!props || !drmmode_prop_info_copy(drmmode_crtc->props, > crtc_props, > @@ -1940,7 +1924,7 @@ drmmode_crtc_init(ScrnInfoPtr pScrn, > drmmode_ptr drmmode, drmModeResPtr mode_res > DRMMODE_CRTC__COUNT, props); > drmModeFreeObjectProperties(props); > drmmode_crtc_create_planes(crtc, num); > -#endif > + // XXX: } > > /* Hide any cursors which may be active from previous users */ > drmModeSetCursor(drmmode->fd, drmmode_crtc->mode_crtc->crtc_id, > 0, 0, 0); Right. > @@ -2263,19 +2245,17 @@ drmmode_output_dpms(xf86OutputPtr output, int > mode) > drmmode_output_private_ptr drmmode_output = output- > >driver_private; > xf86CrtcPtr crtc = output->crtc; > drmModeConnectorPtr koutput = drmmode_output->mode_output; > -#ifndef GLAMOR_HAS_DRM_ATOMIC > drmmode_ptr drmmode = drmmode_output->drmmode; > -#endif > > if (!koutput) > return; > > -#ifdef GLAMOR_HAS_DRM_ATOMIC > + // XXX: if (...->atomic_modeset) { > drmmode_output->dpms = mode; > -#else > + // XXX: } else { > drmModeConnectorSetProperty(drmmode->fd, koutput->connector_id, > drmmode_output->dpms_enum_id, mode); > -#endif > + // XXX: } > > if (crtc) { > drmmode_crtc_private_ptr drmmode_crtc = crtc- > >driver_private; Right. > @@ -2718,7 +2696,7 @@ drmmode_output_init(ScrnInfoPtr pScrn, > drmmode_ptr drmmode, drmModeResPtr mode_r > /* work out the possible clones later */ > output->possible_clones = 0; > > -#ifdef GLAMOR_HAS_DRM_ATOMIC > + // XXX: if (...->atomic_modeset) { > if (!drmmode_prop_info_copy(drmmode_output->props_connector, > connector_props, > DRMMODE_CONNECTOR__COUNT, 0)) { > goto out_free_encoders; > @@ -2728,10 +2706,10 @@ drmmode_output_init(ScrnInfoPtr pScrn, > drmmode_ptr drmmode, drmModeResPtr mode_r > DRM_MODE_OBJECT_CONNECTOR); > drmmode_prop_info_update(drmmode, drmmode_output- > >props_connector, > DRMMODE_CONNECTOR__COUNT, props); > -#else > + // XXX: } else { > drmmode_output->dpms_enum_id = > koutput_get_prop_id(drmmode->fd, koutput, > DRM_MODE_PROP_ENUM, "DPMS"); > -#endif > + // XXX: } > > if (dynamic) > output->randr_output = > RROutputCreate(xf86ScrnToScreen(pScrn), output->name, strlen(output- > >name), output); Right. > @@ -256,7 +254,8 @@ ms_present_check_flip(RRCrtcPtr crtc, > pixmap->devKind != drmmode_bo_get_pitch(&ms- > >drmmode.front_bo)) > return FALSE; > > -#ifdef GLAMOR_HAS_DRM_MODIFIERS > + // XXX: Runtime check modifiers_supported? > +#ifdef GBM_BO_WITH_MODIFIERS > /* Check if buffer format/modifier is supported by all active > CRTCs */ > gbm = glamor_gbm_bo_from_pixmap(screen, pixmap); > if (gbm) { I don't a check is needed here. drmmode_is_format_supported() should return FALSE if modifiers aren't supported by the kernel. However, in that case (returning FALSE), we might add a check to see if there is ANY modifier available and avoid returning PRESENT_FLIP_REASON_BUFFER_FORMAT (and avoid the client issuing an extra XCB request). The rest of it looks simple enough. Thanks Emil, -- Louis-Francis _______________________________________________ [email protected]: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
