Hi Am 27.10.22 um 14:22 schrieb Pekka Paalanen: > On Thu, 27 Oct 2022 13:08:24 +0200 > Thomas Zimmermann 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 >>> Fixes: 6ea966fca084 ("drm/simpledrm: Add [AX]RGB2101010 formats") >>> Fixes: 11e8f5fd223b ("drm: Add simpledrm driver") >>> Cc: stable@vger.kernel.org >>> Signed-off-by: Hector Martin >>> --- >>> 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 ->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