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

[Hector Martin <marcan@marcan.st>]

On 27/10/2022 20.08, Thomas Zimmermann wrote:
> 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.

Looks like this was merged recently, so I didn't see it on my tree (I
was basing off of 6.1-rc2).

Since this patch is a regression fix, it should be applied to drm-fixes
(and automatically picked up by stable folks) soon to be fixed in 6.1,
and then we can fix whatever is needed in ofdrm separately in drm-tip.
As long as ofdrm is ready for the new behavior prior to the merge of
drm-tip with 6.1, there will be no breakage.

In this case, the change required to ofdrm is probably just to replace
that array with just DRM_FORMAT_XRGB8888 (which should be the only
supported fallback format for new drivers) and then to add a test to
only expose it for formats for which we actually have conversion
helpers, similar to what the the switch() enumerates here. That logic
could later be moved into the helper as a refactor.

>>   	/* 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. (?)

That would be fine to do, it would just be moving the logic to the
helper. That kind of refactoring is better suited for subsequent
patches. This is a regression fix, it attempts to minimize the amount of
refactoring, which means keeping the logic in simpledrm, to make it
easier to review for correctness.

Also, that would change the API of that function, which would likely
make the merge with the new ofdrm painful. The way things are now, a
small fix to ofdrm will make it compatible with both the state before
and after this patch, which means the merge will go through painlessly,
and then we can just refactor everything once everything is in the same
tree.

- Hector