Re: [PATCH] drm/nouveau: prefer XBGR2101010 for addfb ioctl

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Sat, Feb 03, 2018 at 02:11:23PM -0500, Ilia Mirkin wrote:
> Nouveau only exposes support for XBGR2101010. Prior to the atomic
> conversion, drm would pass in the wrong format in the framebuffer, but
> it was always ignored -- both userspace (xf86-video-nouveau) and the
> kernel driver agreed on the layout, so the fact that the format was
> wrong didn't matter.
> 
> With the atomic conversion, nouveau all of a sudden started caring about
> the exact format, and so the previously-working code in
> xf86-video-nouveau no longer functioned since the (internally-assigned)
> format from the addfb ioctl was wrong.
> 
> This change adds infrastructure to allow a drm driver to specify that it
> prefers the XBGR format variant for the addfb ioctl, and makes nouveau's
> nv50 display driver set it. (Prior gens had no support for 30bpp at all.)
> 
> Signed-off-by: Ilia Mirkin <imirkin@alum.mit.edu>
> Cc: stable@vger.kernel.org # v4.10+
> ---
> 
> Wasn't sure if the nouveau line needs to be split out into a separate
> change or not.
> 
>  drivers/gpu/drm/drm_framebuffer.c      | 4 ++++
>  drivers/gpu/drm/nouveau/nv50_display.c | 1 +
>  include/drm/drm_drv.h                  | 1 +
>  3 files changed, 6 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
> index 5a13ff29f4f0..c0530a1af5e3 100644
> --- a/drivers/gpu/drm/drm_framebuffer.c
> +++ b/drivers/gpu/drm/drm_framebuffer.c
> @@ -121,6 +121,10 @@ int drm_mode_addfb(struct drm_device *dev,
>  	r.pixel_format = drm_mode_legacy_fb_format(or->bpp, or->depth);
>  	r.handles[0] = or->handle;
>  
> +	if (r.pixel_format == DRM_FORMAT_XRGB2101010 &&
> +	    dev->driver->driver_features & DRIVER_PREFER_XBGR_30BPP)
> +		r.pixel_format = DRM_FORMAT_XBGR2101010;

I think I'd much prefer if the driver just passed some kind of a
bpp/depth->format mapping table to the core, or maybe an optional
vfunc to allow the driver to override drm_mode_legacy_fb_format()
behaviour.

drm_mode_legacy_fb_format() is called from the fbdev setup as well
so handling this only in addfb doesn't look sufficient.

> +
>  	ret = drm_mode_addfb2(dev, &r, file_priv);
>  	if (ret)
>  		return ret;
> diff --git a/drivers/gpu/drm/nouveau/nv50_display.c b/drivers/gpu/drm/nouveau/nv50_display.c
> index dd8d4352ed99..caddce88d2d8 100644
> --- a/drivers/gpu/drm/nouveau/nv50_display.c
> +++ b/drivers/gpu/drm/nouveau/nv50_display.c
> @@ -4477,6 +4477,7 @@ nv50_display_create(struct drm_device *dev)
>  	nouveau_display(dev)->fini = nv50_display_fini;
>  	disp->disp = &nouveau_display(dev)->disp;
>  	dev->mode_config.funcs = &nv50_disp_func;
> +	dev->driver->driver_features |= DRIVER_PREFER_XBGR_30BPP;
>  	if (nouveau_atomic)
>  		dev->driver->driver_features |= DRIVER_ATOMIC;
>  
> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
> index d32b688eb346..d23dcdd1bd95 100644
> --- a/include/drm/drm_drv.h
> +++ b/include/drm/drm_drv.h
> @@ -56,6 +56,7 @@ struct drm_printer;
>  #define DRIVER_ATOMIC			0x10000
>  #define DRIVER_KMS_LEGACY_CONTEXT	0x20000
>  #define DRIVER_SYNCOBJ                  0x40000
> +#define DRIVER_PREFER_XBGR_30BPP        0x80000
>  
>  /**
>   * struct drm_driver - DRM driver structure
> -- 
> 2.13.6
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel