Re: [igt-dev] [PATCH 3/3] tests/kms_writeback: support DRM_FORMAT_XRGB2101010 for writeback

[Harry Wentland <harry.wentland@amd.com>]

On 2023-08-22 10:14, Maxime Ripard wrote:
> On Thu, Aug 17, 2023 at 10:22:56AM -0600, Alex Hung wrote:
>>>>>> I think we should make a proper test for that format, instead of a
>>>>>> (silent) fallback?
>>>>>
>>>>> Not sure what's the proper test or the fallback you are referring to. The
>>>>> intention here is to test XRGB2101010 when possible without breaking the
>>>>> original behaviours for XRGB8888.
>>>>
>>>> Right, but you're not even testing that XRGB2101010 is actually there,
>>>> so if amdgpu format listing was somehow broken and you were to expose
>>>> XRGB8888 instead of XRGB2101010, you wouldn't notice. That's not great.
>>
>> Only XRGB2101010 is supported.
>>
>>>>> Changing kms_writeback to tests multiple formats requires significant
>>>>> modifications and tests which requires a driver capable of multiple formats
>>>>> in the first place.
>>>>
>>>> That's fine, amdgpu will be that driver.
>>
>> The purpose of this patch is to improve kms_writeback coverage:
>>
>> - check XRGB8888, and test if supported or skip otherwise
>> to
>> - no XRGB8888? also check XRGB2101010, and test if supported or skip
>> otherwise
>>
>> For amdgpu, no writeback -> support XRGB2101010 in writeback.
>>
>> XRGB2101010 will be used for testing. XRGB8888 may or may not be added later
>> depending on whether there will be consumers of the feature but that is out
>> of scope of this IGT patch.
> 
> My point is the fallback doesn't provide anything, and is harmful to
> some extent. So far, we have some hardware that support XRGB8888 only,
> and some that support XRGB2101010 only.
> 
> There's no point in falling back from one to the other, and if we ever
> have hardware that would support both it prevents from checking both.
> 
> And sure, we could address it then, or we could address it now. It's
> just adding an extra parameter to a couple of functions and a few new
> subtests, it's not like we need to rewrite the whole thing.
> 

I tend to agree. My preference would be to have a subtest for each
supported format, i.e. a -xrgb8888 and an -xrgb2101010 test.

I think there was some concern about not changing existing test
names. In that case we can keep the original test names for the
-xrgb8888 tests and append the format to the new tests.

Harry

> Maxime