[PATCH 7/7] drm/fb-helper: Don't use the preferred depth for the BPP default

[Daniel Vetter]

On Wed, Nov 16, 2022 at 05:09:17PM +0100, Thomas Zimmermann wrote:
> If no preferred value for bits-per-pixel has been given, fall back
> to 32. Never use the preferred depth. The color depth is the number
> of color/alpha bits per pixel, while bpp is the overall number of
> bits in most cases.
> 
> Most noteworthy, XRGB8888 has a depth of 24 and a bpp value of 32.
> Using depth for bpp would make the value 24 as well and format
> selection in fbdev helpers fails. Unfortunately XRGB8888 is the most
> common format and the old heuristic therefore fails for most of
> the drivers (unless they implement the 24-bit RGB888 format).
> 
> Picking a bpp of 32 will lateron result in a default depth of 24
> and the format XRGB8888. As XRGB8888 is the default format for most
> of the current and legacy graphics stack, all drivers must support
> it. So it is the safe choice.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann at suse.de>
> ---
>  drivers/gpu/drm/drm_fbdev_generic.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_fbdev_generic.c b/drivers/gpu/drm/drm_fbdev_generic.c
> index ab86956692795..0a4c160e0e58a 100644
> --- a/drivers/gpu/drm/drm_fbdev_generic.c
> +++ b/drivers/gpu/drm/drm_fbdev_generic.c
> @@ -431,7 +431,6 @@ static const struct drm_client_funcs drm_fbdev_client_funcs = {
>   * drm_fbdev_generic_setup() - Setup generic fbdev emulation
>   * @dev: DRM device
>   * @preferred_bpp: Preferred bits per pixel for the device.
> - *                 @dev->mode_config.preferred_depth is used if this is zero.
>   *
>   * This function sets up generic fbdev emulation for drivers that supports
>   * dumb buffers with a virtual address and that can be mmap'ed.
> @@ -475,12 +474,16 @@ void drm_fbdev_generic_setup(struct drm_device *dev,
>  	}
>  
>  	/*
> -	 * FIXME: This mixes up depth with bpp, which results in a glorious
> -	 * mess, resulting in some drivers picking wrong fbdev defaults and
> -	 * others wrong preferred_depth defaults.
> +	 * Pick a preferred bpp of 32 if no value has been given. This
> +	 * will select XRGB8888 for the framebuffer formats. All drivers
> +	 * have to support XRGB8888 for backwards compatibility with legacy
> +	 * userspace, so it's the safe choice here.
> +	 *
> +	 * TODO: Replace struct drm_mode_config.preferred_depth and this
> +	 *       bpp value with a preferred format that is given as struct
> +	 *       drm_format_info. Then derive all other values from the
> +	 *       format.

I concur on this being the right design. What I've attempted years ago,
but never managed to finish, is sort the formats list on the primary plane
in preference order (since that seems useful for other reasons), and then
let everyone derive the preferred_whatever from the first format of the
first primary plane automatically.

But doing that is a pretty huge refactor, since you get to audit every
driver. So I kinda gave up on that. But I still thing something in that
direction would be a good design overall, since then userspace could also
use the same trick to infer format preferences ...

Anyway on the series, since it pushes in a direction I wanted to fix years
ago but gave up because too ambitious :-)

Acked-by: Daniel Vetter <daniel.vetter at ffwll.ch>

>  	 */
> -	if (!preferred_bpp)
> -		preferred_bpp = dev->mode_config.preferred_depth;
>  	if (!preferred_bpp)
>  		preferred_bpp = 32;
>  	fb_helper->preferred_bpp = preferred_bpp;
> -- 
> 2.38.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch