[PATCH xserver] modesetting: Only use modifiers on kms drivers which do support them.

[Louis-Francis Ratté-Boulianne]

Hi,

It seems to do the right thing, so:

On Fri, 2018-04-20 at 19:59 +0200, Mario Kleiner wrote:
> Use the DRM_CAP_ADDFB2_MODIFIERS query to make sure the kms
> driver supports modifiers in the addfb2 ioctl, and fall back
> to addfb ioctl without modifiers if modifiers are unsupported.
> 
> E.g., as of Linux 4.17, nouveau-kms so far does not suppport
> modifiers and gets angry if drmModeAddFB2WithModifiers() is
> called (-> failure to set a video mode -> blank screen), but
> Mesa's nvc0+ gallium driver causes gbm_bo_get_modifier() to
> return a valid modifier by translating the default tiling of
> bo's created via gbm_bo_create() into a modifier other than
> DRM_FORMAT_MOD_INVALID (see Mesa's nvc0_miptree_get_modifier()).
> 
> Testing for != DRM_FORMAT_MOD_INVALID is apparently not
> sufficient for safe use of drmModeAddFB2WithModifiers.
> 
> Bonus: Handle potential failure of populate_format_modifiers().
> 
> The required DRM_CAP is defined since libdrm v2.4.65, and we
> require v2.4.89+ for the server, so we can use it unconditionally.
> 
> Tested on intel-kms, radeon-kms, nouveau-kms. Fixes failure on
> NVidia Pascal.
> 
> Fixes: 2f807c2324b4 ("modesetting: Add support for multi-plane
> pixmaps when page-flipping")
> Signed-off-by: Mario Kleiner <mario.kleiner.de at gmail.com>
> Cc: Daniel Stone <daniels at collabora.com>

Reviewed-by: Louis-Francis Ratté-Boulianne <lfrb at collabora.com>


> ---
>  hw/xfree86/drivers/modesetting/driver.c          |  5 +++++
>  hw/xfree86/drivers/modesetting/driver.h          |  2 ++
>  hw/xfree86/drivers/modesetting/drmmode_display.c | 13 ++++++-------
>  3 files changed, 13 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/xfree86/drivers/modesetting/driver.c
> b/hw/xfree86/drivers/modesetting/driver.c
> index 7682b24..5d8906d 100644
> --- a/hw/xfree86/drivers/modesetting/driver.c
> +++ b/hw/xfree86/drivers/modesetting/driver.c
> @@ -1018,6 +1018,11 @@ PreInit(ScrnInfoPtr pScrn, int flags)
>      ret |= drmSetClientCap(ms->fd, DRM_CLIENT_CAP_ATOMIC, 1);
>      ms->atomic_modeset = (ret == 0);
>  
> +    ms->kms_has_modifiers = FALSE;
> +    ret = drmGetCap(ms->fd, DRM_CAP_ADDFB2_MODIFIERS, &value);
> +    if (ret == 0 && value != 0)
> +        ms->kms_has_modifiers = TRUE;
> +
>      if (drmmode_pre_init(pScrn, &ms->drmmode, pScrn->bitsPerPixel /
> 8) == FALSE) {
>          xf86DrvMsg(pScrn->scrnIndex, X_ERROR, "KMS setup failed\n");
>          goto fail;
> diff --git a/hw/xfree86/drivers/modesetting/driver.h
> b/hw/xfree86/drivers/modesetting/driver.h
> index cb8cfe6..b2a4e52 100644
> --- a/hw/xfree86/drivers/modesetting/driver.h
> +++ b/hw/xfree86/drivers/modesetting/driver.h
> @@ -117,6 +117,8 @@ typedef struct _modesettingRec {
>      Bool has_queue_sequence;
>      Bool tried_queue_sequence;
>  
> +    Bool kms_has_modifiers;
> +
>  } modesettingRec, *modesettingPtr;
>  
>  #define modesettingPTR(p) ((modesettingPtr)((p)->driverPrivate))
> diff --git a/hw/xfree86/drivers/modesetting/drmmode_display.c
> b/hw/xfree86/drivers/modesetting/drmmode_display.c
> index 79e91f0..859a21a 100644
> --- a/hw/xfree86/drivers/modesetting/drmmode_display.c
> +++ b/hw/xfree86/drivers/modesetting/drmmode_display.c
> @@ -954,7 +954,8 @@ drmmode_bo_import(drmmode_ptr drmmode, drmmode_bo
> *bo,
>                    uint32_t *fb_id)
>  {
>  #ifdef GBM_BO_WITH_MODIFIERS
> -    if (bo->gbm &&
> +    modesettingPtr ms = modesettingPTR(drmmode->scrn);
> +    if (bo->gbm && ms->kms_has_modifiers &&
>          gbm_bo_get_modifier(bo->gbm) != DRM_FORMAT_MOD_INVALID) {
>          int num_fds;
>  
> @@ -1974,6 +1975,9 @@ populate_format_modifiers(xf86CrtcPtr crtc,
> const drmModePlane *kplane,
>      uint32_t *blob_formats;
>      struct drm_format_modifier *blob_modifiers;
>  
> +    if (!blob_id)
> +        return FALSE;
> +
>      blob = drmModeGetPropertyBlob(drmmode->fd, blob_id);
>      if (!blob)
>          return FALSE;
> @@ -1988,7 +1992,6 @@ populate_format_modifiers(xf86CrtcPtr crtc,
> const drmModePlane *kplane,
>          uint32_t num_modifiers = 0;
>          uint64_t *modifiers = NULL;
>          uint64_t *tmp;
> -
>          for (j = 0; j < fmt_mod_blob->count_modifiers; j++) {
>              struct drm_format_modifier *mod = &blob_modifiers[j];
>  
> @@ -2147,11 +2150,7 @@ 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);
> -        if (blob_id) {
> -            populate_format_modifiers(crtc, best_kplane, blob_id);
> -        }
> -        else
> -        {
> +        if (!populate_format_modifiers(crtc, best_kplane, blob_id))
> {
>              for (i = 0; i < best_kplane->count_formats; i++)
>                  drmmode_crtc->formats[i].format = best_kplane-
> >formats[i];
>          }