Re: [PATCH] drm/simpledrm: Only advertise formats that are supported

From: Pekka Paalanen
Date: Thu Oct 27 2022 - 08:22:44 EST

On Thu, 27 Oct 2022 13:08:24 +0200
Thomas Zimmermann <tzimmermann@xxxxxxx> 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@xxxxxxxxxxxxx>
> > Fixes: 6ea966fca084 ("drm/simpledrm: Add [AX]RGB2101010 formats")
> > Fixes: 11e8f5fd223b ("drm: Add simpledrm driver")
> > Cc: stable@xxxxxxxxxxxxxxx
> > Signed-off-by: Hector Martin <marcan@xxxxxxxxx>
> > ---
> > 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: pgpPmqK1j4bVk.pgp
Description: OpenPGP digital signature