Date:   Thu, 27 Oct 2022 14:51:28 +0200
From:   Thomas Zimmermann <tzimmermann@...e.de>
To:     Pekka Paalanen <ppaalanen@...il.com>
Cc:     Hector Martin <marcan@...can.st>,
        Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
        Maxime Ripard <mripard@...nel.org>,
        David Airlie <airlied@...il.com>,
        Daniel Vetter <daniel@...ll.ch>,
        Javier Martinez Canillas <javierm@...hat.com>,
        dri-devel@...ts.freedesktop.org, asahi@...ts.linux.dev,
        linux-kernel@...r.kernel.org, stable@...r.kernel.org
Subject: Re: [PATCH] drm/simpledrm: Only advertise formats that are supported

Hi

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

Download attachment "OpenPGP_signature" of type "application/pgp-signature" (841 bytes)

Powered by blists - more mailing lists