[PATCH xserver 2/5] Remove always true GLAMOR_HAS_DRM_* guards

[Louis-Francis Ratté-Boulianne]

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 <emil.velikov at collabora.com>
> 
> 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 <keithp at keithp.com>
> Cc: Daniel Stone <daniels at collabora.com>
> Cc: Louis-Francis Ratté-Boulianne <lfrb at collabora.com>
> Signed-off-by: Emil Velikov <emil.velikov at collabora.com>
> ---
> 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