[PATCH] drm/simpledrm: Only advertise formats that are supported
Thomas Zimmermann
tzimmermann at suse.de
Thu Oct 27 12:51:28 UTC 2022
Hi
Am 27.10.22 um 14:22 schrieb Pekka Paalanen:
> On Thu, 27 Oct 2022 13:08:24 +0200
> Thomas Zimmermann <tzimmermann at 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 at collabora.com>
>>> Fixes: 6ea966fca084 ("drm/simpledrm: Add [AX]RGB2101010 formats")
>>> Fixes: 11e8f5fd223b ("drm: Add simpledrm driver")
>>> Cc: stable at vger.kernel.org
>>> Signed-off-by: Hector Martin <marcan at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 840 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20221027/6c8809c8/attachment-0001.sig>
More information about the dri-devel
mailing list