| Message ID | 20210907115752.6181-8-noralf@tronnes.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/gud: Add some more pixel formats | expand |
Hi Am 07.09.21 um 13:57 schrieb Noralf Trønnes: > For devices that don't support XRGB8888 give the user the ability to > choose what's most important: Color depth or frames per second. > > Add an 'xrgb8888' module parameter to override the emulation format. > > Assume the user wants full control if xrgb8888 is set and don't set > DRM_CAP_DUMB_PREFERRED_DEPTH if RGB565 is supported (AFAIK only X.org > supports this). More of a general statement: wouldn't it make more sense to auto-detect this entirely? The GUD protocol could order the list of supported formats by preference (maybe it does already). Or you could take the type of USB connection into account. Additionally, xrgb8888 is really a fall-back for lazy userspace programs, but userspace should do better IMHO. Best regards Thomas > > Signed-off-by: Noralf Trønnes <noralf@tronnes.org> > --- > drivers/gpu/drm/gud/gud_drv.c | 13 ++++++++++--- > 1 file changed, 10 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/gud/gud_drv.c b/drivers/gpu/drm/gud/gud_drv.c > index 3f9d4b9a1e3d..60d27ee5ddbd 100644 > --- a/drivers/gpu/drm/gud/gud_drv.c > +++ b/drivers/gpu/drm/gud/gud_drv.c > @@ -30,6 +30,10 @@ > > #include "gud_internal.h" > > +static int gud_xrgb8888; > +module_param_named(xrgb8888, gud_xrgb8888, int, 0644); > +MODULE_PARM_DESC(xrgb8888, "XRGB8888 emulation format: GUD_PIXEL_FORMAT_* value, 0=auto, -1=disable [default=auto]"); > + > /* Only used internally */ > static const struct drm_format_info gud_drm_format_r1 = { > .format = GUD_DRM_FORMAT_R1, > @@ -530,12 +534,12 @@ static int gud_probe(struct usb_interface *intf, const struct usb_device_id *id) > case DRM_FORMAT_RGB332: > fallthrough; > case DRM_FORMAT_RGB888: > - if (!xrgb8888_emulation_format) > + if (!gud_xrgb8888 && !xrgb8888_emulation_format) > xrgb8888_emulation_format = info; > break; > case DRM_FORMAT_RGB565: > rgb565_supported = true; > - if (!xrgb8888_emulation_format) > + if (!gud_xrgb8888 && !xrgb8888_emulation_format) > xrgb8888_emulation_format = info; > break; > case DRM_FORMAT_XRGB8888: > @@ -543,6 +547,9 @@ static int gud_probe(struct usb_interface *intf, const struct usb_device_id *id) > break; > } > > + if (gud_xrgb8888 == formats_dev[i]) > + xrgb8888_emulation_format = info; > + > fmt_buf_size = drm_format_info_min_pitch(info, 0, drm->mode_config.max_width) * > drm->mode_config.max_height; > max_buffer_size = max(max_buffer_size, fmt_buf_size); > @@ -559,7 +566,7 @@ static int gud_probe(struct usb_interface *intf, const struct usb_device_id *id) > } > > /* Prefer speed over color depth */ > - if (rgb565_supported) > + if (!gud_xrgb8888 && rgb565_supported) > drm->mode_config.preferred_depth = 16; > > if (!xrgb8888_supported && xrgb8888_emulation_format) { >
Den 08.09.2021 20.10, skrev Thomas Zimmermann: > Hi > > Am 07.09.21 um 13:57 schrieb Noralf Trønnes: >> For devices that don't support XRGB8888 give the user the ability to >> choose what's most important: Color depth or frames per second. >> >> Add an 'xrgb8888' module parameter to override the emulation format. >> >> Assume the user wants full control if xrgb8888 is set and don't set >> DRM_CAP_DUMB_PREFERRED_DEPTH if RGB565 is supported (AFAIK only X.org >> supports this). > > More of a general statement: wouldn't it make more sense to auto-detect > this entirely? The GUD protocol could order the list of supported > formats by preference (maybe it does already). Or you could take the > type of USB connection into account. > There is preference in that the driver will use xrgb8888 emulation on the first format returned by the device. Yeah USB speed could be used as a metric for preferred depth, but I don't know of anyone else than X.org using it so I'm unsure about the value. Daniel mentioned that this parameter targets all devices, so it's not a very good solution. Maybe I should drop this patch and gather more user experience with the depth/speed challenge before trying to come up with a solution. > Additionally, xrgb8888 is really a fall-back for lazy userspace > programs, but userspace should do better IMHO. > I don't think even the most agile userspace supports monochrome, greyscale or even RGB332 ;) Noralf. > Best regards > Thomas > >> >> Signed-off-by: Noralf Trønnes <noralf@tronnes.org> >> --- >> drivers/gpu/drm/gud/gud_drv.c | 13 ++++++++++--- >> 1 file changed, 10 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/gpu/drm/gud/gud_drv.c >> b/drivers/gpu/drm/gud/gud_drv.c >> index 3f9d4b9a1e3d..60d27ee5ddbd 100644 >> --- a/drivers/gpu/drm/gud/gud_drv.c >> +++ b/drivers/gpu/drm/gud/gud_drv.c >> @@ -30,6 +30,10 @@ >> #include "gud_internal.h" >> +static int gud_xrgb8888; >> +module_param_named(xrgb8888, gud_xrgb8888, int, 0644); >> +MODULE_PARM_DESC(xrgb8888, "XRGB8888 emulation format: >> GUD_PIXEL_FORMAT_* value, 0=auto, -1=disable [default=auto]"); >> + >> /* Only used internally */ >> static const struct drm_format_info gud_drm_format_r1 = { >> .format = GUD_DRM_FORMAT_R1, >> @@ -530,12 +534,12 @@ static int gud_probe(struct usb_interface *intf, >> const struct usb_device_id *id) >> case DRM_FORMAT_RGB332: >> fallthrough; >> case DRM_FORMAT_RGB888: >> - if (!xrgb8888_emulation_format) >> + if (!gud_xrgb8888 && !xrgb8888_emulation_format) >> xrgb8888_emulation_format = info; >> break; >> case DRM_FORMAT_RGB565: >> rgb565_supported = true; >> - if (!xrgb8888_emulation_format) >> + if (!gud_xrgb8888 && !xrgb8888_emulation_format) >> xrgb8888_emulation_format = info; >> break; >> case DRM_FORMAT_XRGB8888: >> @@ -543,6 +547,9 @@ static int gud_probe(struct usb_interface *intf, >> const struct usb_device_id *id) >> break; >> } >> + if (gud_xrgb8888 == formats_dev[i]) >> + xrgb8888_emulation_format = info; >> + >> fmt_buf_size = drm_format_info_min_pitch(info, 0, >> drm->mode_config.max_width) * >> drm->mode_config.max_height; >> max_buffer_size = max(max_buffer_size, fmt_buf_size); >> @@ -559,7 +566,7 @@ static int gud_probe(struct usb_interface *intf, >> const struct usb_device_id *id) >> } >> /* Prefer speed over color depth */ >> - if (rgb565_supported) >> + if (!gud_xrgb8888 && rgb565_supported) >> drm->mode_config.preferred_depth = 16; >> if (!xrgb8888_supported && xrgb8888_emulation_format) { >> >
diff --git a/drivers/gpu/drm/gud/gud_drv.c b/drivers/gpu/drm/gud/gud_drv.c index 3f9d4b9a1e3d..60d27ee5ddbd 100644 --- a/drivers/gpu/drm/gud/gud_drv.c +++ b/drivers/gpu/drm/gud/gud_drv.c @@ -30,6 +30,10 @@ #include "gud_internal.h" +static int gud_xrgb8888; +module_param_named(xrgb8888, gud_xrgb8888, int, 0644); +MODULE_PARM_DESC(xrgb8888, "XRGB8888 emulation format: GUD_PIXEL_FORMAT_* value, 0=auto, -1=disable [default=auto]"); + /* Only used internally */ static const struct drm_format_info gud_drm_format_r1 = { .format = GUD_DRM_FORMAT_R1, @@ -530,12 +534,12 @@ static int gud_probe(struct usb_interface *intf, const struct usb_device_id *id) case DRM_FORMAT_RGB332: fallthrough; case DRM_FORMAT_RGB888: - if (!xrgb8888_emulation_format) + if (!gud_xrgb8888 && !xrgb8888_emulation_format) xrgb8888_emulation_format = info; break; case DRM_FORMAT_RGB565: rgb565_supported = true; - if (!xrgb8888_emulation_format) + if (!gud_xrgb8888 && !xrgb8888_emulation_format) xrgb8888_emulation_format = info; break; case DRM_FORMAT_XRGB8888: @@ -543,6 +547,9 @@ static int gud_probe(struct usb_interface *intf, const struct usb_device_id *id) break; } + if (gud_xrgb8888 == formats_dev[i]) + xrgb8888_emulation_format = info; + fmt_buf_size = drm_format_info_min_pitch(info, 0, drm->mode_config.max_width) * drm->mode_config.max_height; max_buffer_size = max(max_buffer_size, fmt_buf_size); @@ -559,7 +566,7 @@ static int gud_probe(struct usb_interface *intf, const struct usb_device_id *id) } /* Prefer speed over color depth */ - if (rgb565_supported) + if (!gud_xrgb8888 && rgb565_supported) drm->mode_config.preferred_depth = 16; if (!xrgb8888_supported && xrgb8888_emulation_format) {
For devices that don't support XRGB8888 give the user the ability to choose what's most important: Color depth or frames per second. Add an 'xrgb8888' module parameter to override the emulation format. Assume the user wants full control if xrgb8888 is set and don't set DRM_CAP_DUMB_PREFERRED_DEPTH if RGB565 is supported (AFAIK only X.org supports this). Signed-off-by: Noralf Trønnes <noralf@tronnes.org> --- drivers/gpu/drm/gud/gud_drv.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-)