* Re: [PATCH] drm/simpledrm: Only advertise formats that are supported
@ 2022-10-27 11:08 ` Thomas Zimmermann
0 siblings, 0 replies; 18+ messages in thread
From: Thomas Zimmermann @ 2022-10-27 11:08 UTC (permalink / raw)
To: Hector Martin, Maarten Lankhorst, Maxime Ripard, David Airlie,
Daniel Vetter, Javier Martinez Canillas
Cc: stable, Pekka Paalanen, asahi, dri-devel, linux-kernel
[-- Attachment #1.1: Type: text/plain, Size: 8353 bytes --]
Hi
Am 27.10.22 um 12:13 schrieb Hector Martin:
> Until now, simpledrm unconditionally advertised all formats that can be
> supported natively as conversions. However, we don't actually have a
> full conversion matrix of helpers. Although the list is arguably
> provided to userspace in precedence order, userspace can pick something
> out-of-order (and thus break when it shouldn't), or simply only support
> a format that is unsupported (and thus think it can work, which results
> in the appearance of a hang as FB blits fail later on, instead of the
> initialization error you'd expect in this case).
>
> Split up the format table into separate ones for each required subset,
> and then pick one based on the native format. Also remove the
> native<->conversion overlap check from the helper (which doesn't make
> sense any more, since the native format is advertised anyway and this
> way RGB565/RGB888 can share a format table), and instead print the same
> message in simpledrm when the native format is not one for which we have
> conversions at all.
>
> This fixes a real user regression where the ?RGB2101010 support commit
> started advertising it unconditionally where not supported, and KWin
> decided to start to use it over the native format, but also the fixes
> the spurious RGB565/RGB888 formats which have been wrongly
> unconditionally advertised since the dawn of simpledrm.
>
> Note: this patch is merged because splitting it into two patches, one
> for the helper and one for simpledrm, would regress at the midpoint
> regardless of the order. If simpledrm is changed first, that would break
> working conversions to RGB565/RGB888 (since those share a table that
> does not include the native formats). If the helper is changed first, it
> would start spuriously advertising all conversion formats when the
> native format doesn't have any supported conversions at all.
>
> Acked-by: Pekka Paalanen <pekka.paalanen@collabora.com>
> Fixes: 6ea966fca084 ("drm/simpledrm: Add [AX]RGB2101010 formats")
> Fixes: 11e8f5fd223b ("drm: Add simpledrm driver")
> Cc: stable@vger.kernel.org
> Signed-off-by: Hector Martin <marcan@marcan.st>
> ---
> drivers/gpu/drm/drm_format_helper.c | 15 -------
> drivers/gpu/drm/tiny/simpledrm.c | 62 +++++++++++++++++++++++++----
We currently have two DRM drivers that call drm_fb_build_fourcc_list():
simpledrm and ofdrm. I've been very careful to keep the format selection
in sync between them. (That's the reason why the helper exists at all.)
If the drivers start to use different logic, it will only become more
chaotic.
The format array of ofdrm is at [1]. At a minimum, ofdrm should get the
same fix as simpledrm.
[1]
https://cgit.freedesktop.org/drm/drm-tip/tree/drivers/gpu/drm/tiny/ofdrm.c#n760
> 2 files changed, 55 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_format_helper.c b/drivers/gpu/drm/drm_format_helper.c
> index e2f76621453c..c60c13f3a872 100644
> --- a/drivers/gpu/drm/drm_format_helper.c
> +++ b/drivers/gpu/drm/drm_format_helper.c
> @@ -864,20 +864,6 @@ size_t drm_fb_build_fourcc_list(struct drm_device *dev,
> ++fourccs;
> }
>
> - /*
> - * The plane's atomic_update helper converts the framebuffer's color format
> - * to a native format when copying to device memory.
> - *
> - * If there is not a single format supported by both, device and
> - * driver, the native formats are likely not supported by the conversion
> - * helpers. Therefore *only* support the native formats and add a
> - * conversion helper ASAP.
> - */
> - if (!found_native) {
> - drm_warn(dev, "Format conversion helpers required to add extra formats.\n");
> - goto out;
> - }
> -
> /*
> * The extra formats, emulated by the driver, go second.
> */
> @@ -898,7 +884,6 @@ size_t drm_fb_build_fourcc_list(struct drm_device *dev,
> ++fourccs;
> }
>
> -out:
> return fourccs - fourccs_out;
> }
> EXPORT_SYMBOL(drm_fb_build_fourcc_list);
> diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
> index 18489779fb8a..1257411f3d44 100644
> --- a/drivers/gpu/drm/tiny/simpledrm.c
> +++ b/drivers/gpu/drm/tiny/simpledrm.c
> @@ -446,22 +446,48 @@ static int simpledrm_device_init_regulators(struct simpledrm_device *sdev)
> */
>
> /*
> - * Support all formats of simplefb and maybe more; in order
> - * of preference. The display's update function will do any
> + * Support the subset of formats that we have conversion helpers for,
> + * in order of preference. The display's update function will do any
> * conversion necessary.
> *
> * TODO: Add blit helpers for remaining formats and uncomment
> * constants.
> */
> -static const uint32_t simpledrm_primary_plane_formats[] = {
> +
> +/*
> + * Supported conversions to RGB565 and RGB888:
> + * from [AX]RGB8888
> + */
> +static const uint32_t simpledrm_primary_plane_formats_base[] = {
> + DRM_FORMAT_XRGB8888,
> + DRM_FORMAT_ARGB8888,
> +};
> +
> +/*
> + * Supported conversions to [AX]RGB8888:
> + * A/X variants (no-op)
> + * from RGB565
> + * from RGB888
> + */
> +static const uint32_t simpledrm_primary_plane_formats_xrgb8888[] = {
> DRM_FORMAT_XRGB8888,
> DRM_FORMAT_ARGB8888,
> + DRM_FORMAT_RGB888,
> DRM_FORMAT_RGB565,
> //DRM_FORMAT_XRGB1555,
> //DRM_FORMAT_ARGB1555,
> - DRM_FORMAT_RGB888,
> +};
> +
> +/*
> + * Supported conversions to [AX]RGB2101010:
> + * A/X variants (no-op)
> + * from [AX]RGB8888
> + */
> +static const uint32_t simpledrm_primary_plane_formats_xrgb2101010[] = {
> DRM_FORMAT_XRGB2101010,
> DRM_FORMAT_ARGB2101010,
> + DRM_FORMAT_XRGB8888,
> + DRM_FORMAT_ARGB8888,
> };
>
> static const uint64_t simpledrm_primary_plane_format_modifiers[] = {
> @@ -642,7 +668,8 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
> struct drm_encoder *encoder;
> struct drm_connector *connector;
> unsigned long max_width, max_height;
> - size_t nformats;
> + const uint32_t *conv_formats;
> + size_t conv_nformats, nformats;
> int ret;
>
> sdev = devm_drm_dev_alloc(&pdev->dev, drv, struct simpledrm_device, dev);
> @@ -755,10 +782,31 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
> dev->mode_config.funcs = &simpledrm_mode_config_funcs;
>
> /* Primary plane */
> + switch (format->format) {
I trust you when you say that <native>->XRGB8888 is not enough. But
although I've read your replies, I still don't understand why this
switch is necessary.
Why don't we call drm_fb_build_fourcc_list() with the native
format/formats and let it append a number of formats, such as adding
XRGB888, adding ARGB8888 if necessary, adding ARGB2101010 if necessary.
Each with a elaborate comment why and which userspace needs the format. (?)
Best regards
Thomas
> + case DRM_FORMAT_RGB565:
> + case DRM_FORMAT_RGB888:
> + conv_formats = simpledrm_primary_plane_formats_base;
> + conv_nformats = ARRAY_SIZE(simpledrm_primary_plane_formats_base);
> + break;
> + case DRM_FORMAT_XRGB8888:
> + case DRM_FORMAT_ARGB8888:
> + conv_formats = simpledrm_primary_plane_formats_xrgb8888;
> + conv_nformats = ARRAY_SIZE(simpledrm_primary_plane_formats_xrgb8888);
> + break;
> + case DRM_FORMAT_XRGB2101010:
> + case DRM_FORMAT_ARGB2101010:
> + conv_formats = simpledrm_primary_plane_formats_xrgb2101010;
> + conv_nformats = ARRAY_SIZE(simpledrm_primary_plane_formats_xrgb2101010);
> + break;
> + default:
> + conv_formats = NULL;
> + conv_nformats = 0;
> + drm_warn(dev, "Format conversion helpers required to add extra formats.\n");
> + break;
> + }
>
> nformats = drm_fb_build_fourcc_list(dev, &format->format, 1,
> - simpledrm_primary_plane_formats,
> - ARRAY_SIZE(simpledrm_primary_plane_formats),
> + conv_formats, conv_nformats,
> sdev->formats, ARRAY_SIZE(sdev->formats));
>
> primary_plane = &sdev->primary_plane;
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH] drm/simpledrm: Only advertise formats that are supported
2022-10-27 11:08 ` Thomas Zimmermann
@ 2022-10-27 12:22 ` Pekka Paalanen
-1 siblings, 0 replies; 18+ messages in thread
From: Pekka Paalanen @ 2022-10-27 12:22 UTC (permalink / raw)
To: Thomas Zimmermann
Cc: Hector Martin, Maarten Lankhorst, Maxime Ripard, David Airlie,
Daniel Vetter, Javier Martinez Canillas, dri-devel, asahi,
linux-kernel, stable
[-- Attachment #1: Type: text/plain, Size: 9513 bytes --]
On Thu, 27 Oct 2022 13:08:24 +0200
Thomas Zimmermann <tzimmermann@suse.de> wrote:
> Hi
>
> Am 27.10.22 um 12:13 schrieb Hector Martin:
> > Until now, simpledrm unconditionally advertised all formats that can be
> > supported natively as conversions. However, we don't actually have a
> > full conversion matrix of helpers. Although the list is arguably
> > provided to userspace in precedence order, userspace can pick something
> > out-of-order (and thus break when it shouldn't), or simply only support
> > a format that is unsupported (and thus think it can work, which results
> > in the appearance of a hang as FB blits fail later on, instead of the
> > initialization error you'd expect in this case).
> >
> > Split up the format table into separate ones for each required subset,
> > and then pick one based on the native format. Also remove the
> > native<->conversion overlap check from the helper (which doesn't make
> > sense any more, since the native format is advertised anyway and this
> > way RGB565/RGB888 can share a format table), and instead print the same
> > message in simpledrm when the native format is not one for which we have
> > conversions at all.
> >
> > This fixes a real user regression where the ?RGB2101010 support commit
> > started advertising it unconditionally where not supported, and KWin
> > decided to start to use it over the native format, but also the fixes
> > the spurious RGB565/RGB888 formats which have been wrongly
> > unconditionally advertised since the dawn of simpledrm.
> >
> > Note: this patch is merged because splitting it into two patches, one
> > for the helper and one for simpledrm, would regress at the midpoint
> > regardless of the order. If simpledrm is changed first, that would break
> > working conversions to RGB565/RGB888 (since those share a table that
> > does not include the native formats). If the helper is changed first, it
> > would start spuriously advertising all conversion formats when the
> > native format doesn't have any supported conversions at all.
> >
> > Acked-by: Pekka Paalanen <pekka.paalanen@collabora.com>
> > Fixes: 6ea966fca084 ("drm/simpledrm: Add [AX]RGB2101010 formats")
> > Fixes: 11e8f5fd223b ("drm: Add simpledrm driver")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Hector Martin <marcan@marcan.st>
> > ---
> > drivers/gpu/drm/drm_format_helper.c | 15 -------
> > drivers/gpu/drm/tiny/simpledrm.c | 62 +++++++++++++++++++++++++----
>
> We currently have two DRM drivers that call drm_fb_build_fourcc_list():
> simpledrm and ofdrm. I've been very careful to keep the format selection
> in sync between them. (That's the reason why the helper exists at all.)
> If the drivers start to use different logic, it will only become more
> chaotic.
>
> The format array of ofdrm is at [1]. At a minimum, ofdrm should get the
> same fix as simpledrm.
>
> [1]
> https://cgit.freedesktop.org/drm/drm-tip/tree/drivers/gpu/drm/tiny/ofdrm.c#n760
Hi Thomas,
yes, the principle applies to all drivers except VKMS: do not emulate
anything in software unless it must be done to prevent kernel
regressions on specific hardware.
ofdrm should indeed do the same.
> > 2 files changed, 55 insertions(+), 22 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_format_helper.c b/drivers/gpu/drm/drm_format_helper.c
> > index e2f76621453c..c60c13f3a872 100644
> > --- a/drivers/gpu/drm/drm_format_helper.c
> > +++ b/drivers/gpu/drm/drm_format_helper.c
> > @@ -864,20 +864,6 @@ size_t drm_fb_build_fourcc_list(struct drm_device *dev,
> > ++fourccs;
> > }
> >
> > - /*
> > - * The plane's atomic_update helper converts the framebuffer's color format
> > - * to a native format when copying to device memory.
> > - *
> > - * If there is not a single format supported by both, device and
> > - * driver, the native formats are likely not supported by the conversion
> > - * helpers. Therefore *only* support the native formats and add a
> > - * conversion helper ASAP.
> > - */
> > - if (!found_native) {
> > - drm_warn(dev, "Format conversion helpers required to add extra formats.\n");
> > - goto out;
> > - }
> > -
> > /*
> > * The extra formats, emulated by the driver, go second.
> > */
> > @@ -898,7 +884,6 @@ size_t drm_fb_build_fourcc_list(struct drm_device *dev,
> > ++fourccs;
> > }
> >
> > -out:
> > return fourccs - fourccs_out;
> > }
> > EXPORT_SYMBOL(drm_fb_build_fourcc_list);
> > diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
> > index 18489779fb8a..1257411f3d44 100644
> > --- a/drivers/gpu/drm/tiny/simpledrm.c
> > +++ b/drivers/gpu/drm/tiny/simpledrm.c
> > @@ -446,22 +446,48 @@ static int simpledrm_device_init_regulators(struct simpledrm_device *sdev)
> > */
> >
> > /*
> > - * Support all formats of simplefb and maybe more; in order
> > - * of preference. The display's update function will do any
> > + * Support the subset of formats that we have conversion helpers for,
> > + * in order of preference. The display's update function will do any
> > * conversion necessary.
> > *
> > * TODO: Add blit helpers for remaining formats and uncomment
> > * constants.
> > */
> > -static const uint32_t simpledrm_primary_plane_formats[] = {
> > +
> > +/*
> > + * Supported conversions to RGB565 and RGB888:
> > + * from [AX]RGB8888
> > + */
> > +static const uint32_t simpledrm_primary_plane_formats_base[] = {
> > + DRM_FORMAT_XRGB8888,
> > + DRM_FORMAT_ARGB8888,
> > +};
> > +
> > +/*
> > + * Supported conversions to [AX]RGB8888:
> > + * A/X variants (no-op)
> > + * from RGB565
> > + * from RGB888
> > + */
> > +static const uint32_t simpledrm_primary_plane_formats_xrgb8888[] = {
> > DRM_FORMAT_XRGB8888,
> > DRM_FORMAT_ARGB8888,
> > + DRM_FORMAT_RGB888,
> > DRM_FORMAT_RGB565,
> > //DRM_FORMAT_XRGB1555,
> > //DRM_FORMAT_ARGB1555,
> > - DRM_FORMAT_RGB888,
> > +};
> > +
> > +/*
> > + * Supported conversions to [AX]RGB2101010:
> > + * A/X variants (no-op)
> > + * from [AX]RGB8888
> > + */
> > +static const uint32_t simpledrm_primary_plane_formats_xrgb2101010[] = {
> > DRM_FORMAT_XRGB2101010,
> > DRM_FORMAT_ARGB2101010,
> > + DRM_FORMAT_XRGB8888,
> > + DRM_FORMAT_ARGB8888,
> > };
> >
> > static const uint64_t simpledrm_primary_plane_format_modifiers[] = {
> > @@ -642,7 +668,8 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
> > struct drm_encoder *encoder;
> > struct drm_connector *connector;
> > unsigned long max_width, max_height;
> > - size_t nformats;
> > + const uint32_t *conv_formats;
> > + size_t conv_nformats, nformats;
> > int ret;
> >
> > sdev = devm_drm_dev_alloc(&pdev->dev, drv, struct simpledrm_device, dev);
> > @@ -755,10 +782,31 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
> > dev->mode_config.funcs = &simpledrm_mode_config_funcs;
> >
> > /* Primary plane */
> > + switch (format->format) {
>
> I trust you when you say that <native>->XRGB8888 is not enough. But
> although I've read your replies, I still don't understand why this
> switch is necessary.
>
> Why don't we call drm_fb_build_fourcc_list() with the native
> format/formats and let it append a number of formats, such as adding
> XRGB888, adding ARGB8888 if necessary, adding ARGB2101010 if necessary.
> Each with a elaborate comment why and which userspace needs the format. (?)
Something like
uint32_t conv_formats[] = {
DRM_FORMAT_XRGB8888, /* expected by old userspace */
DRM_FORMAT_ARGB8888, /* historically exposed and working */
0,
0,
0,
};
size_t conv_nformats = 2;
if (native_format == DRM_FORMAT_XRGB2101010)
conv_formats[conv_nformats++] = DRM_FORMAT_ARGB2101010; /* historically exposed and working */
if (native_format == DRM_FORMAT_XRGB8888) {
conv_formats[conv_nformats++] = DRM_FORMAT_RGB565; /* historically exposed and working */
conv_formats[conv_nformats++] = DRM_FORMAT_RGB888; /* historically exposed and working */
}
maybe?
Thanks,
pq
>
> Best regards
> Thomas
>
> > + case DRM_FORMAT_RGB565:
> > + case DRM_FORMAT_RGB888:
> > + conv_formats = simpledrm_primary_plane_formats_base;
> > + conv_nformats = ARRAY_SIZE(simpledrm_primary_plane_formats_base);
> > + break;
> > + case DRM_FORMAT_XRGB8888:
> > + case DRM_FORMAT_ARGB8888:
> > + conv_formats = simpledrm_primary_plane_formats_xrgb8888;
> > + conv_nformats = ARRAY_SIZE(simpledrm_primary_plane_formats_xrgb8888);
> > + break;
> > + case DRM_FORMAT_XRGB2101010:
> > + case DRM_FORMAT_ARGB2101010:
> > + conv_formats = simpledrm_primary_plane_formats_xrgb2101010;
> > + conv_nformats = ARRAY_SIZE(simpledrm_primary_plane_formats_xrgb2101010);
> > + break;
> > + default:
> > + conv_formats = NULL;
> > + conv_nformats = 0;
> > + drm_warn(dev, "Format conversion helpers required to add extra formats.\n");
> > + break;
> > + }
> >
> > nformats = drm_fb_build_fourcc_list(dev, &format->format, 1,
> > - simpledrm_primary_plane_formats,
> > - ARRAY_SIZE(simpledrm_primary_plane_formats),
> > + conv_formats, conv_nformats,
> > sdev->formats, ARRAY_SIZE(sdev->formats));
> >
> > primary_plane = &sdev->primary_plane;
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH] drm/simpledrm: Only advertise formats that are supported
@ 2022-10-27 12:22 ` Pekka Paalanen
0 siblings, 0 replies; 18+ messages in thread
From: Pekka Paalanen @ 2022-10-27 12:22 UTC (permalink / raw)
To: Thomas Zimmermann
Cc: Hector Martin, Javier Martinez Canillas, linux-kernel, dri-devel,
stable, asahi
[-- Attachment #1: Type: text/plain, Size: 9513 bytes --]
On Thu, 27 Oct 2022 13:08:24 +0200
Thomas Zimmermann <tzimmermann@suse.de> wrote:
> Hi
>
> Am 27.10.22 um 12:13 schrieb Hector Martin:
> > Until now, simpledrm unconditionally advertised all formats that can be
> > supported natively as conversions. However, we don't actually have a
> > full conversion matrix of helpers. Although the list is arguably
> > provided to userspace in precedence order, userspace can pick something
> > out-of-order (and thus break when it shouldn't), or simply only support
> > a format that is unsupported (and thus think it can work, which results
> > in the appearance of a hang as FB blits fail later on, instead of the
> > initialization error you'd expect in this case).
> >
> > Split up the format table into separate ones for each required subset,
> > and then pick one based on the native format. Also remove the
> > native<->conversion overlap check from the helper (which doesn't make
> > sense any more, since the native format is advertised anyway and this
> > way RGB565/RGB888 can share a format table), and instead print the same
> > message in simpledrm when the native format is not one for which we have
> > conversions at all.
> >
> > This fixes a real user regression where the ?RGB2101010 support commit
> > started advertising it unconditionally where not supported, and KWin
> > decided to start to use it over the native format, but also the fixes
> > the spurious RGB565/RGB888 formats which have been wrongly
> > unconditionally advertised since the dawn of simpledrm.
> >
> > Note: this patch is merged because splitting it into two patches, one
> > for the helper and one for simpledrm, would regress at the midpoint
> > regardless of the order. If simpledrm is changed first, that would break
> > working conversions to RGB565/RGB888 (since those share a table that
> > does not include the native formats). If the helper is changed first, it
> > would start spuriously advertising all conversion formats when the
> > native format doesn't have any supported conversions at all.
> >
> > Acked-by: Pekka Paalanen <pekka.paalanen@collabora.com>
> > Fixes: 6ea966fca084 ("drm/simpledrm: Add [AX]RGB2101010 formats")
> > Fixes: 11e8f5fd223b ("drm: Add simpledrm driver")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Hector Martin <marcan@marcan.st>
> > ---
> > drivers/gpu/drm/drm_format_helper.c | 15 -------
> > drivers/gpu/drm/tiny/simpledrm.c | 62 +++++++++++++++++++++++++----
>
> We currently have two DRM drivers that call drm_fb_build_fourcc_list():
> simpledrm and ofdrm. I've been very careful to keep the format selection
> in sync between them. (That's the reason why the helper exists at all.)
> If the drivers start to use different logic, it will only become more
> chaotic.
>
> The format array of ofdrm is at [1]. At a minimum, ofdrm should get the
> same fix as simpledrm.
>
> [1]
> https://cgit.freedesktop.org/drm/drm-tip/tree/drivers/gpu/drm/tiny/ofdrm.c#n760
Hi Thomas,
yes, the principle applies to all drivers except VKMS: do not emulate
anything in software unless it must be done to prevent kernel
regressions on specific hardware.
ofdrm should indeed do the same.
> > 2 files changed, 55 insertions(+), 22 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_format_helper.c b/drivers/gpu/drm/drm_format_helper.c
> > index e2f76621453c..c60c13f3a872 100644
> > --- a/drivers/gpu/drm/drm_format_helper.c
> > +++ b/drivers/gpu/drm/drm_format_helper.c
> > @@ -864,20 +864,6 @@ size_t drm_fb_build_fourcc_list(struct drm_device *dev,
> > ++fourccs;
> > }
> >
> > - /*
> > - * The plane's atomic_update helper converts the framebuffer's color format
> > - * to a native format when copying to device memory.
> > - *
> > - * If there is not a single format supported by both, device and
> > - * driver, the native formats are likely not supported by the conversion
> > - * helpers. Therefore *only* support the native formats and add a
> > - * conversion helper ASAP.
> > - */
> > - if (!found_native) {
> > - drm_warn(dev, "Format conversion helpers required to add extra formats.\n");
> > - goto out;
> > - }
> > -
> > /*
> > * The extra formats, emulated by the driver, go second.
> > */
> > @@ -898,7 +884,6 @@ size_t drm_fb_build_fourcc_list(struct drm_device *dev,
> > ++fourccs;
> > }
> >
> > -out:
> > return fourccs - fourccs_out;
> > }
> > EXPORT_SYMBOL(drm_fb_build_fourcc_list);
> > diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
> > index 18489779fb8a..1257411f3d44 100644
> > --- a/drivers/gpu/drm/tiny/simpledrm.c
> > +++ b/drivers/gpu/drm/tiny/simpledrm.c
> > @@ -446,22 +446,48 @@ static int simpledrm_device_init_regulators(struct simpledrm_device *sdev)
> > */
> >
> > /*
> > - * Support all formats of simplefb and maybe more; in order
> > - * of preference. The display's update function will do any
> > + * Support the subset of formats that we have conversion helpers for,
> > + * in order of preference. The display's update function will do any
> > * conversion necessary.
> > *
> > * TODO: Add blit helpers for remaining formats and uncomment
> > * constants.
> > */
> > -static const uint32_t simpledrm_primary_plane_formats[] = {
> > +
> > +/*
> > + * Supported conversions to RGB565 and RGB888:
> > + * from [AX]RGB8888
> > + */
> > +static const uint32_t simpledrm_primary_plane_formats_base[] = {
> > + DRM_FORMAT_XRGB8888,
> > + DRM_FORMAT_ARGB8888,
> > +};
> > +
> > +/*
> > + * Supported conversions to [AX]RGB8888:
> > + * A/X variants (no-op)
> > + * from RGB565
> > + * from RGB888
> > + */
> > +static const uint32_t simpledrm_primary_plane_formats_xrgb8888[] = {
> > DRM_FORMAT_XRGB8888,
> > DRM_FORMAT_ARGB8888,
> > + DRM_FORMAT_RGB888,
> > DRM_FORMAT_RGB565,
> > //DRM_FORMAT_XRGB1555,
> > //DRM_FORMAT_ARGB1555,
> > - DRM_FORMAT_RGB888,
> > +};
> > +
> > +/*
> > + * Supported conversions to [AX]RGB2101010:
> > + * A/X variants (no-op)
> > + * from [AX]RGB8888
> > + */
> > +static const uint32_t simpledrm_primary_plane_formats_xrgb2101010[] = {
> > DRM_FORMAT_XRGB2101010,
> > DRM_FORMAT_ARGB2101010,
> > + DRM_FORMAT_XRGB8888,
> > + DRM_FORMAT_ARGB8888,
> > };
> >
> > static const uint64_t simpledrm_primary_plane_format_modifiers[] = {
> > @@ -642,7 +668,8 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
> > struct drm_encoder *encoder;
> > struct drm_connector *connector;
> > unsigned long max_width, max_height;
> > - size_t nformats;
> > + const uint32_t *conv_formats;
> > + size_t conv_nformats, nformats;
> > int ret;
> >
> > sdev = devm_drm_dev_alloc(&pdev->dev, drv, struct simpledrm_device, dev);
> > @@ -755,10 +782,31 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
> > dev->mode_config.funcs = &simpledrm_mode_config_funcs;
> >
> > /* Primary plane */
> > + switch (format->format) {
>
> I trust you when you say that <native>->XRGB8888 is not enough. But
> although I've read your replies, I still don't understand why this
> switch is necessary.
>
> Why don't we call drm_fb_build_fourcc_list() with the native
> format/formats and let it append a number of formats, such as adding
> XRGB888, adding ARGB8888 if necessary, adding ARGB2101010 if necessary.
> Each with a elaborate comment why and which userspace needs the format. (?)
Something like
uint32_t conv_formats[] = {
DRM_FORMAT_XRGB8888, /* expected by old userspace */
DRM_FORMAT_ARGB8888, /* historically exposed and working */
0,
0,
0,
};
size_t conv_nformats = 2;
if (native_format == DRM_FORMAT_XRGB2101010)
conv_formats[conv_nformats++] = DRM_FORMAT_ARGB2101010; /* historically exposed and working */
if (native_format == DRM_FORMAT_XRGB8888) {
conv_formats[conv_nformats++] = DRM_FORMAT_RGB565; /* historically exposed and working */
conv_formats[conv_nformats++] = DRM_FORMAT_RGB888; /* historically exposed and working */
}
maybe?
Thanks,
pq
>
> Best regards
> Thomas
>
> > + case DRM_FORMAT_RGB565:
> > + case DRM_FORMAT_RGB888:
> > + conv_formats = simpledrm_primary_plane_formats_base;
> > + conv_nformats = ARRAY_SIZE(simpledrm_primary_plane_formats_base);
> > + break;
> > + case DRM_FORMAT_XRGB8888:
> > + case DRM_FORMAT_ARGB8888:
> > + conv_formats = simpledrm_primary_plane_formats_xrgb8888;
> > + conv_nformats = ARRAY_SIZE(simpledrm_primary_plane_formats_xrgb8888);
> > + break;
> > + case DRM_FORMAT_XRGB2101010:
> > + case DRM_FORMAT_ARGB2101010:
> > + conv_formats = simpledrm_primary_plane_formats_xrgb2101010;
> > + conv_nformats = ARRAY_SIZE(simpledrm_primary_plane_formats_xrgb2101010);
> > + break;
> > + default:
> > + conv_formats = NULL;
> > + conv_nformats = 0;
> > + drm_warn(dev, "Format conversion helpers required to add extra formats.\n");
> > + break;
> > + }
> >
> > nformats = drm_fb_build_fourcc_list(dev, &format->format, 1,
> > - simpledrm_primary_plane_formats,
> > - ARRAY_SIZE(simpledrm_primary_plane_formats),
> > + conv_formats, conv_nformats,
> > sdev->formats, ARRAY_SIZE(sdev->formats));
> >
> > primary_plane = &sdev->primary_plane;
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH] drm/simpledrm: Only advertise formats that are supported
2022-10-27 12:22 ` Pekka Paalanen
@ 2022-10-27 12:51 ` Thomas Zimmermann
-1 siblings, 0 replies; 18+ messages in thread
From: Thomas Zimmermann @ 2022-10-27 12:51 UTC (permalink / raw)
To: Pekka Paalanen
Cc: Hector Martin, Maarten Lankhorst, Maxime Ripard, David Airlie,
Daniel Vetter, Javier Martinez Canillas, dri-devel, asahi,
linux-kernel, stable
[-- Attachment #1.1: Type: text/plain, Size: 10388 bytes --]
Hi
Am 27.10.22 um 14:22 schrieb Pekka Paalanen:
> On Thu, 27 Oct 2022 13:08:24 +0200
> Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
>> Hi
>>
>> Am 27.10.22 um 12:13 schrieb Hector Martin:
>>> Until now, simpledrm unconditionally advertised all formats that can be
>>> supported natively as conversions. However, we don't actually have a
>>> full conversion matrix of helpers. Although the list is arguably
>>> provided to userspace in precedence order, userspace can pick something
>>> out-of-order (and thus break when it shouldn't), or simply only support
>>> a format that is unsupported (and thus think it can work, which results
>>> in the appearance of a hang as FB blits fail later on, instead of the
>>> initialization error you'd expect in this case).
>>>
>>> Split up the format table into separate ones for each required subset,
>>> and then pick one based on the native format. Also remove the
>>> native<->conversion overlap check from the helper (which doesn't make
>>> sense any more, since the native format is advertised anyway and this
>>> way RGB565/RGB888 can share a format table), and instead print the same
>>> message in simpledrm when the native format is not one for which we have
>>> conversions at all.
>>>
>>> This fixes a real user regression where the ?RGB2101010 support commit
>>> started advertising it unconditionally where not supported, and KWin
>>> decided to start to use it over the native format, but also the fixes
>>> the spurious RGB565/RGB888 formats which have been wrongly
>>> unconditionally advertised since the dawn of simpledrm.
>>>
>>> Note: this patch is merged because splitting it into two patches, one
>>> for the helper and one for simpledrm, would regress at the midpoint
>>> regardless of the order. If simpledrm is changed first, that would break
>>> working conversions to RGB565/RGB888 (since those share a table that
>>> does not include the native formats). If the helper is changed first, it
>>> would start spuriously advertising all conversion formats when the
>>> native format doesn't have any supported conversions at all.
>>>
>>> Acked-by: Pekka Paalanen <pekka.paalanen@collabora.com>
>>> Fixes: 6ea966fca084 ("drm/simpledrm: Add [AX]RGB2101010 formats")
>>> Fixes: 11e8f5fd223b ("drm: Add simpledrm driver")
>>> Cc: stable@vger.kernel.org
>>> Signed-off-by: Hector Martin <marcan@marcan.st>
>>> ---
>>> drivers/gpu/drm/drm_format_helper.c | 15 -------
>>> drivers/gpu/drm/tiny/simpledrm.c | 62 +++++++++++++++++++++++++----
>>
>> We currently have two DRM drivers that call drm_fb_build_fourcc_list():
>> simpledrm and ofdrm. I've been very careful to keep the format selection
>> in sync between them. (That's the reason why the helper exists at all.)
>> If the drivers start to use different logic, it will only become more
>> chaotic.
>>
>> The format array of ofdrm is at [1]. At a minimum, ofdrm should get the
>> same fix as simpledrm.
>>
>> [1]
>> https://cgit.freedesktop.org/drm/drm-tip/tree/drivers/gpu/drm/tiny/ofdrm.c#n760
>
> Hi Thomas,
>
> yes, the principle applies to all drivers except VKMS: do not emulate
> anything in software unless it must be done to prevent kernel
> regressions on specific hardware.
>
> ofdrm should indeed do the same.
>
>>> 2 files changed, 55 insertions(+), 22 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_format_helper.c b/drivers/gpu/drm/drm_format_helper.c
>>> index e2f76621453c..c60c13f3a872 100644
>>> --- a/drivers/gpu/drm/drm_format_helper.c
>>> +++ b/drivers/gpu/drm/drm_format_helper.c
>>> @@ -864,20 +864,6 @@ size_t drm_fb_build_fourcc_list(struct drm_device *dev,
>>> ++fourccs;
>>> }
>>>
>>> - /*
>>> - * The plane's atomic_update helper converts the framebuffer's color format
>>> - * to a native format when copying to device memory.
>>> - *
>>> - * If there is not a single format supported by both, device and
>>> - * driver, the native formats are likely not supported by the conversion
>>> - * helpers. Therefore *only* support the native formats and add a
>>> - * conversion helper ASAP.
>>> - */
>>> - if (!found_native) {
>>> - drm_warn(dev, "Format conversion helpers required to add extra formats.\n");
>>> - goto out;
>>> - }
>>> -
>>> /*
>>> * The extra formats, emulated by the driver, go second.
>>> */
>>> @@ -898,7 +884,6 @@ size_t drm_fb_build_fourcc_list(struct drm_device *dev,
>>> ++fourccs;
>>> }
>>>
>>> -out:
>>> return fourccs - fourccs_out;
>>> }
>>> EXPORT_SYMBOL(drm_fb_build_fourcc_list);
>>> diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
>>> index 18489779fb8a..1257411f3d44 100644
>>> --- a/drivers/gpu/drm/tiny/simpledrm.c
>>> +++ b/drivers/gpu/drm/tiny/simpledrm.c
>>> @@ -446,22 +446,48 @@ static int simpledrm_device_init_regulators(struct simpledrm_device *sdev)
>>> */
>>>
>>> /*
>>> - * Support all formats of simplefb and maybe more; in order
>>> - * of preference. The display's update function will do any
>>> + * Support the subset of formats that we have conversion helpers for,
>>> + * in order of preference. The display's update function will do any
>>> * conversion necessary.
>>> *
>>> * TODO: Add blit helpers for remaining formats and uncomment
>>> * constants.
>>> */
>>> -static const uint32_t simpledrm_primary_plane_formats[] = {
>>> +
>>> +/*
>>> + * Supported conversions to RGB565 and RGB888:
>>> + * from [AX]RGB8888
>>> + */
>>> +static const uint32_t simpledrm_primary_plane_formats_base[] = {
>>> + DRM_FORMAT_XRGB8888,
>>> + DRM_FORMAT_ARGB8888,
>>> +};
>>> +
>>> +/*
>>> + * Supported conversions to [AX]RGB8888:
>>> + * A/X variants (no-op)
>>> + * from RGB565
>>> + * from RGB888
>>> + */
>>> +static const uint32_t simpledrm_primary_plane_formats_xrgb8888[] = {
>>> DRM_FORMAT_XRGB8888,
>>> DRM_FORMAT_ARGB8888,
>>> + DRM_FORMAT_RGB888,
>>> DRM_FORMAT_RGB565,
>>> //DRM_FORMAT_XRGB1555,
>>> //DRM_FORMAT_ARGB1555,
>>> - DRM_FORMAT_RGB888,
>>> +};
>>> +
>>> +/*
>>> + * Supported conversions to [AX]RGB2101010:
>>> + * A/X variants (no-op)
>>> + * from [AX]RGB8888
>>> + */
>>> +static const uint32_t simpledrm_primary_plane_formats_xrgb2101010[] = {
>>> DRM_FORMAT_XRGB2101010,
>>> DRM_FORMAT_ARGB2101010,
>>> + DRM_FORMAT_XRGB8888,
>>> + DRM_FORMAT_ARGB8888,
>>> };
>>>
>>> static const uint64_t simpledrm_primary_plane_format_modifiers[] = {
>>> @@ -642,7 +668,8 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
>>> struct drm_encoder *encoder;
>>> struct drm_connector *connector;
>>> unsigned long max_width, max_height;
>>> - size_t nformats;
>>> + const uint32_t *conv_formats;
>>> + size_t conv_nformats, nformats;
>>> int ret;
>>>
>>> sdev = devm_drm_dev_alloc(&pdev->dev, drv, struct simpledrm_device, dev);
>>> @@ -755,10 +782,31 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
>>> dev->mode_config.funcs = &simpledrm_mode_config_funcs;
>>>
>>> /* Primary plane */
>>> + switch (format->format) {
>>
>> I trust you when you say that <native>->XRGB8888 is not enough. But
>> although I've read your replies, I still don't understand why this
>> switch is necessary.
>>
>> Why don't we call drm_fb_build_fourcc_list() with the native
>> format/formats and let it append a number of formats, such as adding
>> XRGB888, adding ARGB8888 if necessary, adding ARGB2101010 if necessary.
>> Each with a elaborate comment why and which userspace needs the format. (?)
>
> Something like
>
> uint32_t conv_formats[] = {
> DRM_FORMAT_XRGB8888, /* expected by old userspace */
> DRM_FORMAT_ARGB8888, /* historically exposed and working */
> 0,
> 0,
> 0,
> };
> size_t conv_nformats = 2;
>
> if (native_format == DRM_FORMAT_XRGB2101010)
> conv_formats[conv_nformats++] = DRM_FORMAT_ARGB2101010; /* historically exposed and working */
>
> if (native_format == DRM_FORMAT_XRGB8888) {
> conv_formats[conv_nformats++] = DRM_FORMAT_RGB565; /* historically exposed and working */
> conv_formats[conv_nformats++] = DRM_FORMAT_RGB888; /* historically exposed and working */
> }
>
> maybe?
Yes, that's what I have in mind. We give a list of native formats to
drm_fb_build_fourcc_list(), the helper appends whatever is needed and
returns the result for use by the driver.
Currently, drm_fb_build_fourcc_list() gets the native formats plus a
list of all driver-exported formats. And if I'm not mistake, we could
remove the driver list entirely.
Maybe we could condense the native-format list to a single entry. This
would simplify things significantly.
Best regards
Thomas
>
>
>
> Thanks,
> pq
>
>
>>
>> Best regards
>> Thomas
>>
>>> + case DRM_FORMAT_RGB565:
>>> + case DRM_FORMAT_RGB888:
>>> + conv_formats = simpledrm_primary_plane_formats_base;
>>> + conv_nformats = ARRAY_SIZE(simpledrm_primary_plane_formats_base);
>>> + break;
>>> + case DRM_FORMAT_XRGB8888:
>>> + case DRM_FORMAT_ARGB8888:
>>> + conv_formats = simpledrm_primary_plane_formats_xrgb8888;
>>> + conv_nformats = ARRAY_SIZE(simpledrm_primary_plane_formats_xrgb8888);
>>> + break;
>>> + case DRM_FORMAT_XRGB2101010:
>>> + case DRM_FORMAT_ARGB2101010:
>>> + conv_formats = simpledrm_primary_plane_formats_xrgb2101010;
>>> + conv_nformats = ARRAY_SIZE(simpledrm_primary_plane_formats_xrgb2101010);
>>> + break;
>>> + default:
>>> + conv_formats = NULL;
>>> + conv_nformats = 0;
>>> + drm_warn(dev, "Format conversion helpers required to add extra formats.\n");
>>> + break;
>>> + }
>>>
>>> nformats = drm_fb_build_fourcc_list(dev, &format->format, 1,
>>> - simpledrm_primary_plane_formats,
>>> - ARRAY_SIZE(simpledrm_primary_plane_formats),
>>> + conv_formats, conv_nformats,
>>> sdev->formats, ARRAY_SIZE(sdev->formats));
>>>
>>> primary_plane = &sdev->primary_plane;
>>
>
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH] drm/simpledrm: Only advertise formats that are supported
@ 2022-10-27 12:51 ` Thomas Zimmermann
0 siblings, 0 replies; 18+ messages in thread
From: Thomas Zimmermann @ 2022-10-27 12:51 UTC (permalink / raw)
To: Pekka Paalanen
Cc: Hector Martin, Javier Martinez Canillas, linux-kernel, dri-devel,
stable, asahi
[-- Attachment #1.1: Type: text/plain, Size: 10388 bytes --]
Hi
Am 27.10.22 um 14:22 schrieb Pekka Paalanen:
> On Thu, 27 Oct 2022 13:08:24 +0200
> Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
>> Hi
>>
>> Am 27.10.22 um 12:13 schrieb Hector Martin:
>>> Until now, simpledrm unconditionally advertised all formats that can be
>>> supported natively as conversions. However, we don't actually have a
>>> full conversion matrix of helpers. Although the list is arguably
>>> provided to userspace in precedence order, userspace can pick something
>>> out-of-order (and thus break when it shouldn't), or simply only support
>>> a format that is unsupported (and thus think it can work, which results
>>> in the appearance of a hang as FB blits fail later on, instead of the
>>> initialization error you'd expect in this case).
>>>
>>> Split up the format table into separate ones for each required subset,
>>> and then pick one based on the native format. Also remove the
>>> native<->conversion overlap check from the helper (which doesn't make
>>> sense any more, since the native format is advertised anyway and this
>>> way RGB565/RGB888 can share a format table), and instead print the same
>>> message in simpledrm when the native format is not one for which we have
>>> conversions at all.
>>>
>>> This fixes a real user regression where the ?RGB2101010 support commit
>>> started advertising it unconditionally where not supported, and KWin
>>> decided to start to use it over the native format, but also the fixes
>>> the spurious RGB565/RGB888 formats which have been wrongly
>>> unconditionally advertised since the dawn of simpledrm.
>>>
>>> Note: this patch is merged because splitting it into two patches, one
>>> for the helper and one for simpledrm, would regress at the midpoint
>>> regardless of the order. If simpledrm is changed first, that would break
>>> working conversions to RGB565/RGB888 (since those share a table that
>>> does not include the native formats). If the helper is changed first, it
>>> would start spuriously advertising all conversion formats when the
>>> native format doesn't have any supported conversions at all.
>>>
>>> Acked-by: Pekka Paalanen <pekka.paalanen@collabora.com>
>>> Fixes: 6ea966fca084 ("drm/simpledrm: Add [AX]RGB2101010 formats")
>>> Fixes: 11e8f5fd223b ("drm: Add simpledrm driver")
>>> Cc: stable@vger.kernel.org
>>> Signed-off-by: Hector Martin <marcan@marcan.st>
>>> ---
>>> drivers/gpu/drm/drm_format_helper.c | 15 -------
>>> drivers/gpu/drm/tiny/simpledrm.c | 62 +++++++++++++++++++++++++----
>>
>> We currently have two DRM drivers that call drm_fb_build_fourcc_list():
>> simpledrm and ofdrm. I've been very careful to keep the format selection
>> in sync between them. (That's the reason why the helper exists at all.)
>> If the drivers start to use different logic, it will only become more
>> chaotic.
>>
>> The format array of ofdrm is at [1]. At a minimum, ofdrm should get the
>> same fix as simpledrm.
>>
>> [1]
>> https://cgit.freedesktop.org/drm/drm-tip/tree/drivers/gpu/drm/tiny/ofdrm.c#n760
>
> Hi Thomas,
>
> yes, the principle applies to all drivers except VKMS: do not emulate
> anything in software unless it must be done to prevent kernel
> regressions on specific hardware.
>
> ofdrm should indeed do the same.
>
>>> 2 files changed, 55 insertions(+), 22 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_format_helper.c b/drivers/gpu/drm/drm_format_helper.c
>>> index e2f76621453c..c60c13f3a872 100644
>>> --- a/drivers/gpu/drm/drm_format_helper.c
>>> +++ b/drivers/gpu/drm/drm_format_helper.c
>>> @@ -864,20 +864,6 @@ size_t drm_fb_build_fourcc_list(struct drm_device *dev,
>>> ++fourccs;
>>> }
>>>
>>> - /*
>>> - * The plane's atomic_update helper converts the framebuffer's color format
>>> - * to a native format when copying to device memory.
>>> - *
>>> - * If there is not a single format supported by both, device and
>>> - * driver, the native formats are likely not supported by the conversion
>>> - * helpers. Therefore *only* support the native formats and add a
>>> - * conversion helper ASAP.
>>> - */
>>> - if (!found_native) {
>>> - drm_warn(dev, "Format conversion helpers required to add extra formats.\n");
>>> - goto out;
>>> - }
>>> -
>>> /*
>>> * The extra formats, emulated by the driver, go second.
>>> */
>>> @@ -898,7 +884,6 @@ size_t drm_fb_build_fourcc_list(struct drm_device *dev,
>>> ++fourccs;
>>> }
>>>
>>> -out:
>>> return fourccs - fourccs_out;
>>> }
>>> EXPORT_SYMBOL(drm_fb_build_fourcc_list);
>>> diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
>>> index 18489779fb8a..1257411f3d44 100644
>>> --- a/drivers/gpu/drm/tiny/simpledrm.c
>>> +++ b/drivers/gpu/drm/tiny/simpledrm.c
>>> @@ -446,22 +446,48 @@ static int simpledrm_device_init_regulators(struct simpledrm_device *sdev)
>>> */
>>>
>>> /*
>>> - * Support all formats of simplefb and maybe more; in order
>>> - * of preference. The display's update function will do any
>>> + * Support the subset of formats that we have conversion helpers for,
>>> + * in order of preference. The display's update function will do any
>>> * conversion necessary.
>>> *
>>> * TODO: Add blit helpers for remaining formats and uncomment
>>> * constants.
>>> */
>>> -static const uint32_t simpledrm_primary_plane_formats[] = {
>>> +
>>> +/*
>>> + * Supported conversions to RGB565 and RGB888:
>>> + * from [AX]RGB8888
>>> + */
>>> +static const uint32_t simpledrm_primary_plane_formats_base[] = {
>>> + DRM_FORMAT_XRGB8888,
>>> + DRM_FORMAT_ARGB8888,
>>> +};
>>> +
>>> +/*
>>> + * Supported conversions to [AX]RGB8888:
>>> + * A/X variants (no-op)
>>> + * from RGB565
>>> + * from RGB888
>>> + */
>>> +static const uint32_t simpledrm_primary_plane_formats_xrgb8888[] = {
>>> DRM_FORMAT_XRGB8888,
>>> DRM_FORMAT_ARGB8888,
>>> + DRM_FORMAT_RGB888,
>>> DRM_FORMAT_RGB565,
>>> //DRM_FORMAT_XRGB1555,
>>> //DRM_FORMAT_ARGB1555,
>>> - DRM_FORMAT_RGB888,
>>> +};
>>> +
>>> +/*
>>> + * Supported conversions to [AX]RGB2101010:
>>> + * A/X variants (no-op)
>>> + * from [AX]RGB8888
>>> + */
>>> +static const uint32_t simpledrm_primary_plane_formats_xrgb2101010[] = {
>>> DRM_FORMAT_XRGB2101010,
>>> DRM_FORMAT_ARGB2101010,
>>> + DRM_FORMAT_XRGB8888,
>>> + DRM_FORMAT_ARGB8888,
>>> };
>>>
>>> static const uint64_t simpledrm_primary_plane_format_modifiers[] = {
>>> @@ -642,7 +668,8 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
>>> struct drm_encoder *encoder;
>>> struct drm_connector *connector;
>>> unsigned long max_width, max_height;
>>> - size_t nformats;
>>> + const uint32_t *conv_formats;
>>> + size_t conv_nformats, nformats;
>>> int ret;
>>>
>>> sdev = devm_drm_dev_alloc(&pdev->dev, drv, struct simpledrm_device, dev);
>>> @@ -755,10 +782,31 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
>>> dev->mode_config.funcs = &simpledrm_mode_config_funcs;
>>>
>>> /* Primary plane */
>>> + switch (format->format) {
>>
>> I trust you when you say that <native>->XRGB8888 is not enough. But
>> although I've read your replies, I still don't understand why this
>> switch is necessary.
>>
>> Why don't we call drm_fb_build_fourcc_list() with the native
>> format/formats and let it append a number of formats, such as adding
>> XRGB888, adding ARGB8888 if necessary, adding ARGB2101010 if necessary.
>> Each with a elaborate comment why and which userspace needs the format. (?)
>
> Something like
>
> uint32_t conv_formats[] = {
> DRM_FORMAT_XRGB8888, /* expected by old userspace */
> DRM_FORMAT_ARGB8888, /* historically exposed and working */
> 0,
> 0,
> 0,
> };
> size_t conv_nformats = 2;
>
> if (native_format == DRM_FORMAT_XRGB2101010)
> conv_formats[conv_nformats++] = DRM_FORMAT_ARGB2101010; /* historically exposed and working */
>
> if (native_format == DRM_FORMAT_XRGB8888) {
> conv_formats[conv_nformats++] = DRM_FORMAT_RGB565; /* historically exposed and working */
> conv_formats[conv_nformats++] = DRM_FORMAT_RGB888; /* historically exposed and working */
> }
>
> maybe?
Yes, that's what I have in mind. We give a list of native formats to
drm_fb_build_fourcc_list(), the helper appends whatever is needed and
returns the result for use by the driver.
Currently, drm_fb_build_fourcc_list() gets the native formats plus a
list of all driver-exported formats. And if I'm not mistake, we could
remove the driver list entirely.
Maybe we could condense the native-format list to a single entry. This
would simplify things significantly.
Best regards
Thomas
>
>
>
> Thanks,
> pq
>
>
>>
>> Best regards
>> Thomas
>>
>>> + case DRM_FORMAT_RGB565:
>>> + case DRM_FORMAT_RGB888:
>>> + conv_formats = simpledrm_primary_plane_formats_base;
>>> + conv_nformats = ARRAY_SIZE(simpledrm_primary_plane_formats_base);
>>> + break;
>>> + case DRM_FORMAT_XRGB8888:
>>> + case DRM_FORMAT_ARGB8888:
>>> + conv_formats = simpledrm_primary_plane_formats_xrgb8888;
>>> + conv_nformats = ARRAY_SIZE(simpledrm_primary_plane_formats_xrgb8888);
>>> + break;
>>> + case DRM_FORMAT_XRGB2101010:
>>> + case DRM_FORMAT_ARGB2101010:
>>> + conv_formats = simpledrm_primary_plane_formats_xrgb2101010;
>>> + conv_nformats = ARRAY_SIZE(simpledrm_primary_plane_formats_xrgb2101010);
>>> + break;
>>> + default:
>>> + conv_formats = NULL;
>>> + conv_nformats = 0;
>>> + drm_warn(dev, "Format conversion helpers required to add extra formats.\n");
>>> + break;
>>> + }
>>>
>>> nformats = drm_fb_build_fourcc_list(dev, &format->format, 1,
>>> - simpledrm_primary_plane_formats,
>>> - ARRAY_SIZE(simpledrm_primary_plane_formats),
>>> + conv_formats, conv_nformats,
>>> sdev->formats, ARRAY_SIZE(sdev->formats));
>>>
>>> primary_plane = &sdev->primary_plane;
>>
>
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] drm/simpledrm: Only advertise formats that are supported
2022-10-27 11:08 ` Thomas Zimmermann
@ 2022-10-27 12:31 ` Hector Martin
-1 siblings, 0 replies; 18+ messages in thread
From: Hector Martin @ 2022-10-27 12:31 UTC (permalink / raw)
To: Thomas Zimmermann, Maarten Lankhorst, Maxime Ripard, David Airlie,
Daniel Vetter, Javier Martinez Canillas
Cc: Pekka Paalanen, dri-devel, asahi, linux-kernel, stable
On 27/10/2022 20.08, Thomas Zimmermann wrote:
> We currently have two DRM drivers that call drm_fb_build_fourcc_list():
> simpledrm and ofdrm. I've been very careful to keep the format selection
> in sync between them. (That's the reason why the helper exists at all.)
> If the drivers start to use different logic, it will only become more
> chaotic.
>
> The format array of ofdrm is at [1]. At a minimum, ofdrm should get the
> same fix as simpledrm.
Looks like this was merged recently, so I didn't see it on my tree (I
was basing off of 6.1-rc2).
Since this patch is a regression fix, it should be applied to drm-fixes
(and automatically picked up by stable folks) soon to be fixed in 6.1,
and then we can fix whatever is needed in ofdrm separately in drm-tip.
As long as ofdrm is ready for the new behavior prior to the merge of
drm-tip with 6.1, there will be no breakage.
In this case, the change required to ofdrm is probably just to replace
that array with just DRM_FORMAT_XRGB8888 (which should be the only
supported fallback format for new drivers) and then to add a test to
only expose it for formats for which we actually have conversion
helpers, similar to what the the switch() enumerates here. That logic
could later be moved into the helper as a refactor.
>> /* Primary plane */
>> + switch (format->format) {
>
> I trust you when you say that <native>->XRGB8888 is not enough. But
> although I've read your replies, I still don't understand why this
> switch is necessary.
>
> Why don't we call drm_fb_build_fourcc_list() with the native
> format/formats and let it append a number of formats, such as adding
> XRGB888, adding ARGB8888 if necessary, adding ARGB2101010 if necessary.
> Each with a elaborate comment why and which userspace needs the format. (?)
That would be fine to do, it would just be moving the logic to the
helper. That kind of refactoring is better suited for subsequent
patches. This is a regression fix, it attempts to minimize the amount of
refactoring, which means keeping the logic in simpledrm, to make it
easier to review for correctness.
Also, that would change the API of that function, which would likely
make the merge with the new ofdrm painful. The way things are now, a
small fix to ofdrm will make it compatible with both the state before
and after this patch, which means the merge will go through painlessly,
and then we can just refactor everything once everything is in the same
tree.
- Hector
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH] drm/simpledrm: Only advertise formats that are supported
@ 2022-10-27 12:31 ` Hector Martin
0 siblings, 0 replies; 18+ messages in thread
From: Hector Martin @ 2022-10-27 12:31 UTC (permalink / raw)
To: Thomas Zimmermann, Maarten Lankhorst, Maxime Ripard, David Airlie,
Daniel Vetter, Javier Martinez Canillas
Cc: stable, Pekka Paalanen, asahi, dri-devel, linux-kernel
On 27/10/2022 20.08, Thomas Zimmermann wrote:
> We currently have two DRM drivers that call drm_fb_build_fourcc_list():
> simpledrm and ofdrm. I've been very careful to keep the format selection
> in sync between them. (That's the reason why the helper exists at all.)
> If the drivers start to use different logic, it will only become more
> chaotic.
>
> The format array of ofdrm is at [1]. At a minimum, ofdrm should get the
> same fix as simpledrm.
Looks like this was merged recently, so I didn't see it on my tree (I
was basing off of 6.1-rc2).
Since this patch is a regression fix, it should be applied to drm-fixes
(and automatically picked up by stable folks) soon to be fixed in 6.1,
and then we can fix whatever is needed in ofdrm separately in drm-tip.
As long as ofdrm is ready for the new behavior prior to the merge of
drm-tip with 6.1, there will be no breakage.
In this case, the change required to ofdrm is probably just to replace
that array with just DRM_FORMAT_XRGB8888 (which should be the only
supported fallback format for new drivers) and then to add a test to
only expose it for formats for which we actually have conversion
helpers, similar to what the the switch() enumerates here. That logic
could later be moved into the helper as a refactor.
>> /* Primary plane */
>> + switch (format->format) {
>
> I trust you when you say that <native>->XRGB8888 is not enough. But
> although I've read your replies, I still don't understand why this
> switch is necessary.
>
> Why don't we call drm_fb_build_fourcc_list() with the native
> format/formats and let it append a number of formats, such as adding
> XRGB888, adding ARGB8888 if necessary, adding ARGB2101010 if necessary.
> Each with a elaborate comment why and which userspace needs the format. (?)
That would be fine to do, it would just be moving the logic to the
helper. That kind of refactoring is better suited for subsequent
patches. This is a regression fix, it attempts to minimize the amount of
refactoring, which means keeping the logic in simpledrm, to make it
easier to review for correctness.
Also, that would change the API of that function, which would likely
make the merge with the new ofdrm painful. The way things are now, a
small fix to ofdrm will make it compatible with both the state before
and after this patch, which means the merge will go through painlessly,
and then we can just refactor everything once everything is in the same
tree.
- Hector
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] drm/simpledrm: Only advertise formats that are supported
2022-10-27 11:08 ` Thomas Zimmermann
@ 2022-10-28 10:04 ` Ville Syrjälä
-1 siblings, 0 replies; 18+ messages in thread
From: Ville Syrjälä @ 2022-10-28 10:04 UTC (permalink / raw)
To: Thomas Zimmermann
Cc: Hector Martin, Maarten Lankhorst, Maxime Ripard, David Airlie,
Daniel Vetter, Javier Martinez Canillas, stable, Pekka Paalanen,
asahi, dri-devel, linux-kernel
On Thu, Oct 27, 2022 at 01:08:24PM +0200, Thomas Zimmermann wrote:
> I trust you when you say that <native>->XRGB8888 is not enough. But
> although I've read your replies, I still don't understand why this
> switch is necessary.
>
> Why don't we call drm_fb_build_fourcc_list() with the native
> format/formats and let it append a number of formats, such as adding
> XRGB888, adding ARGB8888 if necessary, adding ARGB2101010 if necessary.
> Each with a elaborate comment why and which userspace needs the format. (?)
Are you saying there is some real userspace that breaks without
the alpha formats? That would already be broken on many devices.
--
Ville Syrjälä
Intel
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] drm/simpledrm: Only advertise formats that are supported
@ 2022-10-28 10:04 ` Ville Syrjälä
0 siblings, 0 replies; 18+ messages in thread
From: Ville Syrjälä @ 2022-10-28 10:04 UTC (permalink / raw)
To: Thomas Zimmermann
Cc: Pekka Paalanen, dri-devel, Hector Martin,
Javier Martinez Canillas, linux-kernel, stable, asahi
On Thu, Oct 27, 2022 at 01:08:24PM +0200, Thomas Zimmermann wrote:
> I trust you when you say that <native>->XRGB8888 is not enough. But
> although I've read your replies, I still don't understand why this
> switch is necessary.
>
> Why don't we call drm_fb_build_fourcc_list() with the native
> format/formats and let it append a number of formats, such as adding
> XRGB888, adding ARGB8888 if necessary, adding ARGB2101010 if necessary.
> Each with a elaborate comment why and which userspace needs the format. (?)
Are you saying there is some real userspace that breaks without
the alpha formats? That would already be broken on many devices.
--
Ville Syrjälä
Intel
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] drm/simpledrm: Only advertise formats that are supported
2022-10-28 10:04 ` Ville Syrjälä
@ 2022-10-28 11:11 ` Thomas Zimmermann
-1 siblings, 0 replies; 18+ messages in thread
From: Thomas Zimmermann @ 2022-10-28 11:11 UTC (permalink / raw)
To: Ville Syrjälä
Cc: Pekka Paalanen, dri-devel, Hector Martin,
Javier Martinez Canillas, linux-kernel, stable, asahi
[-- Attachment #1.1: Type: text/plain, Size: 1078 bytes --]
Hi
Am 28.10.22 um 12:04 schrieb Ville Syrjälä:
> On Thu, Oct 27, 2022 at 01:08:24PM +0200, Thomas Zimmermann wrote:
>> I trust you when you say that <native>->XRGB8888 is not enough. But
>> although I've read your replies, I still don't understand why this
>> switch is necessary.
>>
>> Why don't we call drm_fb_build_fourcc_list() with the native
>> format/formats and let it append a number of formats, such as adding
>> XRGB888, adding ARGB8888 if necessary, adding ARGB2101010 if necessary.
>> Each with a elaborate comment why and which userspace needs the format. (?)
>
> Are you saying there is some real userspace that breaks without
> the alpha formats? That would already be broken on many devices.
We should scrap them all IMHO, unless the hardware really supports them.
See the other discussion on this patch's v2 as well.
Best regards
Thomas
>
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] drm/simpledrm: Only advertise formats that are supported
@ 2022-10-28 11:11 ` Thomas Zimmermann
0 siblings, 0 replies; 18+ messages in thread
From: Thomas Zimmermann @ 2022-10-28 11:11 UTC (permalink / raw)
To: Ville Syrjälä
Cc: Pekka Paalanen, Hector Martin, Javier Martinez Canillas, stable,
linux-kernel, dri-devel, asahi
[-- Attachment #1.1: Type: text/plain, Size: 1078 bytes --]
Hi
Am 28.10.22 um 12:04 schrieb Ville Syrjälä:
> On Thu, Oct 27, 2022 at 01:08:24PM +0200, Thomas Zimmermann wrote:
>> I trust you when you say that <native>->XRGB8888 is not enough. But
>> although I've read your replies, I still don't understand why this
>> switch is necessary.
>>
>> Why don't we call drm_fb_build_fourcc_list() with the native
>> format/formats and let it append a number of formats, such as adding
>> XRGB888, adding ARGB8888 if necessary, adding ARGB2101010 if necessary.
>> Each with a elaborate comment why and which userspace needs the format. (?)
>
> Are you saying there is some real userspace that breaks without
> the alpha formats? That would already be broken on many devices.
We should scrap them all IMHO, unless the hardware really supports them.
See the other discussion on this patch's v2 as well.
Best regards
Thomas
>
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]
^ permalink raw reply [flat|nested] 18+ messages in thread