[PATCH 1/2] drm/vkms: Unset preferred_depth

[PATCH 1/2] drm/vkms: Unset preferred_depth

[Daniel Vetter]

There's a confusion between the preferred_depth uapi and the generic
fbdev helpers. Former wants depth, latter wants bpp, and for XRGB8888
they don't match. Which hit me with vkms, which wants that.

All other drivers setting this and using the generic fbdev helpers use
16, where both numbers match, for RGB565.

Since fixing this is a bit involved (I think for atomic drivers we
should just compute this all internally from the format list of the
first primary plane) paper over the issue in vkms by using defaults
everywhere. Then userspace will pick XRGB8888, and fbdev helpers will
do the same, and we have what we want.

Reported-by: Simon Ser <contact@emersion.fr>
Reviewed-by: Simon Ser <contact@emersion.fr>
Cc: Simon Ser <contact@emersion.fr>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
Cc: Melissa Wen <melissa.srw@gmail.com>
Cc: Haneen Mohammed <hamohammed.sa@gmail.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
---
 drivers/gpu/drm/vkms/vkms_drv.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
index d4d39227f2ed..aef29393b811 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.c
+++ b/drivers/gpu/drm/vkms/vkms_drv.c
@@ -113,7 +113,10 @@ static int vkms_modeset_init(struct vkms_device *vkmsdev)
 	dev->mode_config.max_height = YRES_MAX;
 	dev->mode_config.cursor_width = 512;
 	dev->mode_config.cursor_height = 512;
-	dev->mode_config.preferred_depth = 32;
+	/* FIXME: There's a confusion between bpp and depth between this and
+	 * fbdev helpers. We have to go with 0, meaning "pick the default",
+	 * which ix XRGB8888 in all cases. */
+	dev->mode_config.preferred_depth = 0;
 	dev->mode_config.helper_private = &vkms_mode_config_helpers;
 
 	return vkms_output_init(vkmsdev, 0);
-- 
2.29.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/2] drm/fb-helper: Add a FIXME that generic_setup is very confusing

[Daniel Vetter]

I tried to fix this for real, but it's very sprawling and lots of
drivers get this mildly wrong one way or the other.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: David Airlie <airlied@linux.ie>
Cc: Daniel Vetter <daniel@ffwll.ch>
---
 drivers/gpu/drm/drm_fb_helper.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 596255edf023..27deed4af015 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -2494,6 +2494,11 @@ void drm_fbdev_generic_setup(struct drm_device *dev,
 		return;
 	}
 
+	/*
+	 * 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.
+	 */
 	if (!preferred_bpp)
 		preferred_bpp = dev->mode_config.preferred_depth;
 	if (!preferred_bpp)
-- 
2.29.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm/vkms: Unset preferred_depth

[Ville Syrjälä]

On Fri, Dec 11, 2020 at 05:11:12PM +0100, Daniel Vetter wrote:
> There's a confusion between the preferred_depth uapi and the generic
> fbdev helpers. Former wants depth, latter wants bpp, and for XRGB8888
> they don't match. Which hit me with vkms, which wants that.
> 
> All other drivers setting this and using the generic fbdev helpers use
> 16, where both numbers match, for RGB565.
> 
> Since fixing this is a bit involved (I think for atomic drivers we
> should just compute this all internally from the format list of the
> first primary plane) paper over the issue in vkms by using defaults
> everywhere. Then userspace will pick XRGB8888, and fbdev helpers will
> do the same, and we have what we want.

I think I had a patch ages ago that tried to improve the fb_helper
pixel format stuff a bit. This one I think:
https://patchwork.freedesktop.org/patch/203189/

Haven't checked how much of that would still be relevant though.

> 
> Reported-by: Simon Ser <contact@emersion.fr>
> Reviewed-by: Simon Ser <contact@emersion.fr>
> Cc: Simon Ser <contact@emersion.fr>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> Cc: Melissa Wen <melissa.srw@gmail.com>
> Cc: Haneen Mohammed <hamohammed.sa@gmail.com>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> ---
>  drivers/gpu/drm/vkms/vkms_drv.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
> index d4d39227f2ed..aef29393b811 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.c
> +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> @@ -113,7 +113,10 @@ static int vkms_modeset_init(struct vkms_device *vkmsdev)
>  	dev->mode_config.max_height = YRES_MAX;
>  	dev->mode_config.cursor_width = 512;
>  	dev->mode_config.cursor_height = 512;
> -	dev->mode_config.preferred_depth = 32;
> +	/* FIXME: There's a confusion between bpp and depth between this and
> +	 * fbdev helpers. We have to go with 0, meaning "pick the default",
> +	 * which ix XRGB8888 in all cases. */
> +	dev->mode_config.preferred_depth = 0;
>  	dev->mode_config.helper_private = &vkms_mode_config_helpers;
>  
>  	return vkms_output_init(vkmsdev, 0);
> -- 
> 2.29.2
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

Re: [PATCH 1/2] drm/vkms: Unset preferred_depth

[Daniel Vetter]

On Fri, Dec 11, 2020 at 06:21:40PM +0200, Ville Syrjälä wrote:
> On Fri, Dec 11, 2020 at 05:11:12PM +0100, Daniel Vetter wrote:
> > There's a confusion between the preferred_depth uapi and the generic
> > fbdev helpers. Former wants depth, latter wants bpp, and for XRGB8888
> > they don't match. Which hit me with vkms, which wants that.
> > 
> > All other drivers setting this and using the generic fbdev helpers use
> > 16, where both numbers match, for RGB565.
> > 
> > Since fixing this is a bit involved (I think for atomic drivers we
> > should just compute this all internally from the format list of the
> > first primary plane) paper over the issue in vkms by using defaults
> > everywhere. Then userspace will pick XRGB8888, and fbdev helpers will
> > do the same, and we have what we want.
> 
> I think I had a patch ages ago that tried to improve the fb_helper
> pixel format stuff a bit. This one I think:
> https://patchwork.freedesktop.org/patch/203189/
> 
> Haven't checked how much of that would still be relevant though.

Uh this looks useful, my attempts have only tried to clean up the driver
side in what they're setting for preferred_depth and how they set up the
fbdev emulation. I haven't looked at the fbdev code itself, and especially
your mapping table should be useful.

Care to resubmit that one again?
-Daniel

> 
> > 
> > Reported-by: Simon Ser <contact@emersion.fr>
> > Reviewed-by: Simon Ser <contact@emersion.fr>
> > Cc: Simon Ser <contact@emersion.fr>
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > Cc: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> > Cc: Melissa Wen <melissa.srw@gmail.com>
> > Cc: Haneen Mohammed <hamohammed.sa@gmail.com>
> > Cc: Daniel Vetter <daniel@ffwll.ch>
> > ---
> >  drivers/gpu/drm/vkms/vkms_drv.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
> > index d4d39227f2ed..aef29393b811 100644
> > --- a/drivers/gpu/drm/vkms/vkms_drv.c
> > +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> > @@ -113,7 +113,10 @@ static int vkms_modeset_init(struct vkms_device *vkmsdev)
> >  	dev->mode_config.max_height = YRES_MAX;
> >  	dev->mode_config.cursor_width = 512;
> >  	dev->mode_config.cursor_height = 512;
> > -	dev->mode_config.preferred_depth = 32;
> > +	/* FIXME: There's a confusion between bpp and depth between this and
> > +	 * fbdev helpers. We have to go with 0, meaning "pick the default",
> > +	 * which ix XRGB8888 in all cases. */
> > +	dev->mode_config.preferred_depth = 0;
> >  	dev->mode_config.helper_private = &vkms_mode_config_helpers;
> >  
> >  	return vkms_output_init(vkmsdev, 0);
> > -- 
> > 2.29.2
> > 
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> -- 
> Ville Syrjälä
> Intel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] drm/fb-helper: Add a FIXME that generic_setup is very confusing

[Daniel Vetter]

On Fri, Dec 11, 2020 at 05:11:13PM +0100, Daniel Vetter wrote:
> I tried to fix this for real, but it's very sprawling and lots of
> drivers get this mildly wrong one way or the other.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: David Airlie <airlied@linux.ie>
> Cc: Daniel Vetter <daniel@ffwll.ch>

Anyone feeling like an ack on this one?
-Daniel

> ---
>  drivers/gpu/drm/drm_fb_helper.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 596255edf023..27deed4af015 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -2494,6 +2494,11 @@ void drm_fbdev_generic_setup(struct drm_device *dev,
>  		return;
>  	}
>  
> +	/*
> +	 * 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.
> +	 */
>  	if (!preferred_bpp)
>  		preferred_bpp = dev->mode_config.preferred_depth;
>  	if (!preferred_bpp)
> -- 
> 2.29.2
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] drm/fb-helper: Add a FIXME that generic_setup is very confusing

[Simon Ser]

On Tuesday, December 22nd, 2020 at 9:55 AM, Daniel Vetter <daniel@ffwll.ch> wrote:

> On Fri, Dec 11, 2020 at 05:11:13PM +0100, Daniel Vetter wrote:
> > I tried to fix this for real, but it's very sprawling and lots of
> > drivers get this mildly wrong one way or the other.
> >
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: Maxime Ripard <mripard@kernel.org>
> > Cc: Thomas Zimmermann <tzimmermann@suse.de>
> > Cc: David Airlie <airlied@linux.ie>
> > Cc: Daniel Vetter <daniel@ffwll.ch>
>
> Anyone feeling like an ack on this one?

From my limited understanding of fbdev, this FIXME sounds correct to me.

Acked-by: Simon Ser <contact@emersion.fr>

> > ---
> >  drivers/gpu/drm/drm_fb_helper.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> > index 596255edf023..27deed4af015 100644
> > --- a/drivers/gpu/drm/drm_fb_helper.c
> > +++ b/drivers/gpu/drm/drm_fb_helper.c
> > @@ -2494,6 +2494,11 @@ void drm_fbdev_generic_setup(struct drm_device *dev,
> >  		return;
> >  	}
> >
> > +	/*
> > +	 * 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.
> > +	 */
> >  	if (!preferred_bpp)
> >  		preferred_bpp = dev->mode_config.preferred_depth;
> >  	if (!preferred_bpp)
> > --
> > 2.29.2
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] drm/fb-helper: Add a FIXME that generic_setup is very confusing

[Daniel Vetter]

On Tue, Dec 22, 2020 at 09:00:43AM +0000, Simon Ser wrote:
> On Tuesday, December 22nd, 2020 at 9:55 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> 
> > On Fri, Dec 11, 2020 at 05:11:13PM +0100, Daniel Vetter wrote:
> > > I tried to fix this for real, but it's very sprawling and lots of
> > > drivers get this mildly wrong one way or the other.
> > >
> > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > Cc: Maxime Ripard <mripard@kernel.org>
> > > Cc: Thomas Zimmermann <tzimmermann@suse.de>
> > > Cc: David Airlie <airlied@linux.ie>
> > > Cc: Daniel Vetter <daniel@ffwll.ch>
> >
> > Anyone feeling like an ack on this one?
> 
> From my limited understanding of fbdev, this FIXME sounds correct to me.
> 
> Acked-by: Simon Ser <contact@emersion.fr>

Thanks for taking a look, patch pushed to drm-misc-next.
-Daniel

> 
> > > ---
> > >  drivers/gpu/drm/drm_fb_helper.c | 5 +++++
> > >  1 file changed, 5 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> > > index 596255edf023..27deed4af015 100644
> > > --- a/drivers/gpu/drm/drm_fb_helper.c
> > > +++ b/drivers/gpu/drm/drm_fb_helper.c
> > > @@ -2494,6 +2494,11 @@ void drm_fbdev_generic_setup(struct drm_device *dev,
> > >  		return;
> > >  	}
> > >
> > > +	/*
> > > +	 * 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.
> > > +	 */
> > >  	if (!preferred_bpp)
> > >  		preferred_bpp = dev->mode_config.preferred_depth;
> > >  	if (!preferred_bpp)
> > > --
> > > 2.29.2

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] drm/fb-helper: Add a FIXME that generic_setup is very confusing

[Maxime Ripard]

On Tue, Dec 22, 2020 at 09:55:28AM +0100, Daniel Vetter wrote:
> On Fri, Dec 11, 2020 at 05:11:13PM +0100, Daniel Vetter wrote:
> > I tried to fix this for real, but it's very sprawling and lots of
> > drivers get this mildly wrong one way or the other.
> >
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: Maxime Ripard <mripard@kernel.org>
> > Cc: Thomas Zimmermann <tzimmermann@suse.de>
> > Cc: David Airlie <airlied@linux.ie>
> > Cc: Daniel Vetter <daniel@ffwll.ch>
>
> Anyone feeling like an ack on this one?

Acked-by: Maxime Ripard <mripard@kernel.org>

Maxime
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel