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:
pgpO6ta0TcCYN.pgp
Description: OpenPGP digital signature