| Message ID | 20201020083515.941832-1-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/vkms: Unset preferred_depth | expand |
On Tuesday, October 20, 2020 10:35 AM, Daniel Vetter <daniel.vetter@ffwll.ch> 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. Not quite: grepping reveals a majority of 24 (as shown in drmdb). A few drivers set 16 (udl, cirrus), and hisilicon sets 32. > 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 > 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 With the commit message clarified: Reviewed-by: Simon Ser <contact@emersion.fr>
Hi On 20.10.20 10:35, 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. > > Reported-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 25faba5aac08..98ba844ae77d 100644 > --- a/drivers/gpu/drm/vkms/vkms_drv.c > +++ b/drivers/gpu/drm/vkms/vkms_drv.c > @@ -114,7 +114,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; It's already 0 at this point. I suggest to rather document this FIXME as part of the documentation of drm_fbdev_generic_setup() and preferred_depth. Other drivers are also affected. Best regards Thomas > dev->mode_config.helper_private = &vkms_mode_config_helpers; > > return vkms_output_init(vkmsdev, 0); >
On Tue, Oct 20, 2020 at 10:42 AM Simon Ser <contact@emersion.fr> wrote: > > On Tuesday, October 20, 2020 10:35 AM, Daniel Vetter <daniel.vetter@ffwll.ch> 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. > > Not quite: grepping reveals a majority of 24 (as shown in drmdb). A few > drivers set 16 (udl, cirrus), and hisilicon sets 32. If I haven't screwed up my grepping, those drivers that set 24 don't use the generic fbdev emulation. Which is the broken combination. hisilicon setting 32 is just plain wrong, and it's indeed using fbdev emulation :-/ I also noticed that mga200 can end up setting 32bit. So I guess I've not been terribly well awake when doing my grepping ... > > 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 > > 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 > > With the commit message clarified: > > Reviewed-by: Simon Ser <contact@emersion.fr> >
diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c index 25faba5aac08..98ba844ae77d 100644 --- a/drivers/gpu/drm/vkms/vkms_drv.c +++ b/drivers/gpu/drm/vkms/vkms_drv.c @@ -114,7 +114,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);
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> 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(-)