Re: [PATCH i-g-t 3/4] tests/kms_atomic: Add solid fill plane subtest

[Jessica Zhang <quic_jesszhan@quicinc.com>]

On 1/21/2024 11:56 PM, Pekka Paalanen wrote:
> On Fri, 19 Jan 2024 09:40:52 -0800
> Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> 
>> On 1/19/2024 1:00 AM, Pekka Paalanen wrote:
>>> On Thu, 18 Jan 2024 15:35:10 -0800
>>> Jessica Zhang <quic_jesszhan@quicinc.com> wrote:
>>>    
>>>> On 1/11/2024 1:20 AM, Pekka Paalanen wrote:
>>>>> On Thu, 21 Dec 2023 15:57:51 -0800
>>>>> Jessica Zhang <quic_jesszhan@quicinc.com> wrote:
>>>>>       
>>>>>> On 12/18/2023 2:12 AM, Pekka Paalanen wrote:
>>>>>>> On Fri, 15 Dec 2023 16:40:23 -0800
>>>>>>> Jessica Zhang <quic_jesszhan@quicinc.com> wrote:
>>>>>>>          
>>>>>>>> Add a basic test for solid fill planes.
>>>>>>>>
>>>>>>>> This test will first commit a single-color framebuffer plane then
>>>>>>>> a solid fill plane with the same contents. It then validates the solid
>>>>>>>> fill plane by comparing the resulting CRC with the CRC of the reference
>>>>>>>> framebuffer commit.
>>>>>>>>
>>>>>>>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>>>>>>>> ---
>>>>>>>>      tests/kms_atomic.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>>>>      1 file changed, 94 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/tests/kms_atomic.c b/tests/kms_atomic.c
>>>>>>>> old mode 100644
>>>>>>>> new mode 100755
>>>>>>>> index 2b6e9a8f0383..8f81e65ad84f
>>>>>>>> --- a/tests/kms_atomic.c
>>>>>>>> +++ b/tests/kms_atomic.c
>>>>>>>> @@ -128,6 +128,13 @@ enum kms_atomic_check_relax {
>>>>>>>>      	PLANE_RELAX_FB = (1 << 1)
>>>>>>>>      };
>>>>>>>>      
>>>>>>>> +struct solid_fill_blob {
>>>>>>>> +	uint32_t r;
>>>>>>>> +	uint32_t g;
>>>>>>>> +	uint32_t b;
>>>>>>>> +	uint32_t pad;
>>>>>>>> +};
>>>>>>>> +
>>>>>>>>      static inline int damage_rect_width(struct drm_mode_rect *r)
>>>>>>>>      {
>>>>>>>>      	return r->x2 - r->x1;
>>>>>>>> @@ -1322,6 +1329,79 @@ static void atomic_plane_damage(data_t *data)
>>>>>>>>      	igt_remove_fb(data->drm_fd, &fb_2);
>>>>>>>>      }
>>>>>>>>      
>>>>>>>> +static void test_solid_fill_plane(data_t *data, igt_output_t *output,  igt_plane_t *plane)
>>>>>>>> +{
>>>>>>>> +	struct drm_mode_create_blob c;
>>>>>>>> +	struct drm_mode_destroy_blob d;
>>>>>>>> +	drmModeModeInfo *mode = igt_output_get_mode(output);
>>>>>>>> +	struct drm_mode_rect rect = { 0 };
>>>>>>>> +	struct igt_fb ref_fb;
>>>>>>>> +	igt_pipe_crc_t *pipe_crc;
>>>>>>>> +	igt_crc_t ref_crc, new_crc;
>>>>>>>> +	enum pipe pipe = data->pipe->pipe;
>>>>>>>> +	int height, width;
>>>>>>>> +	int ret;
>>>>>>>> +
>>>>>>>> +	struct solid_fill_blob blob_data = {
>>>>>>>> +		.r = 0x00000000,
>>>>>>>> +		.g = 0x00000000,
>>>>>>>> +		.b = 0xff000000,
>>>>>>>> +		.pad = 0x0,
>>>>>>>> +	};
>>>>>>>
>>>>>>> Hi Jessica!
>>>>>>>
>>>>>>> This is the blob sent to KMS as the solid fill color...
>>>>>>>
>>>>>>> ...
>>>>>>>          
>>>>>>>> +	igt_create_color_fb(data->drm_fd, width, height,
>>>>>>>> +			    DRM_FORMAT_XRGB8888, DRM_FORMAT_MOD_LINEAR,
>>>>>>>> +			    0.0, 0.0, 1.0, &ref_fb);
>>>>>>>
>>>>>>> ..and this (0.0, 0.0, 1.0) is the corresponding color in normalized
>>>>>>> values, I presume.
>>>>>>>
>>>>>>> So you say that 0xff000000 = 1.0.
>>>>>>>
>>>>>>> However, the patch for the kernel UAPI header says this:
>>>>>>>
>>>>>>> +/**
>>>>>>> + * struct drm_mode_solid_fill - User info for solid fill planes
>>>>>>> + *
>>>>>>> + * This is the userspace API solid fill information structure.
>>>>>>> + *
>>>>>>> + * Userspace can enable solid fill planes by assigning the plane "solid_fill"
>>>>>>> + * property to a blob containing a single drm_mode_solid_fill struct populated with an RGB323232
>>>>>>> + * color and setting the pixel source to "SOLID_FILL".
>>>>>>> + *
>>>>>>> + * For information on the plane property, see drm_plane_create_solid_fill_property()
>>>>>>> + *
>>>>>>> + * @r: Red color value of single pixel
>>>>>>> + * @g: Green color value of single pixel
>>>>>>> + * @b: Blue color value of single pixel
>>>>>>> + * @pad: padding, must be zero
>>>>>>> + */
>>>>>>> +struct drm_mode_solid_fill {
>>>>>>> +	__u32 r;
>>>>>>> +	__u32 g;
>>>>>>> +	__u32 b;
>>>>>>> +	__u32 pad;
>>>>>>> +};
>>>>>>>
>>>>>>> I assume that RGB323232 means unsigned 32-bit UNORM (Vulkan term)
>>>>>>> format. That means 1.0 is 0xffffffff, not 0xff000000. This looks like a
>>>>>>> bug in the test.
>>>>>>
>>>>>> Hey Pekka,
>>>>>>
>>>>>> Ah, thanks for catching this -- I'll change the blob value to 0xffffffff
>>>>>> so it matches the 1.0.
>>>>>>
>>>>>> While we're talking about the UAPI struct, I'll also add the actual
>>>>>> drm_mode_solid_fill struct to the IGT drm-uapi instead of the current
>>>>>> workaround.
>>>>>>      
>>>>>>>
>>>>>>> It would be good to test more than one color:
>>>>>>> - 0.0, 0.0, 0.0
>>>>>>> - 1.0, 0.0, 0.0
>>>>>>> - 0.0, 1.0, 0.0
>>>>>>> - 0.0, 0.0, 1.0
>>>>>>> - 1.0, 1.0, 1.0
>>>>>>
>>>>>> Sounds good, will change the test to validate these combinations.
>>>>>>      
>>>>>>>
>>>>>>> for example. That would get at least the so often used black explicitly
>>>>>>> tested, and verify each channel gets mapped correctly rather than only
>>>>>>> blue.
>>>>>>>
>>>>>>> It would also be really good to test dim and mid grays, but I assume it
>>>>>>> might be difficult to get CRC to match over various hardware. You'd
>>>>>>> need to use writeback with an error tolerance. (For watching photos for
>>>>>>> example, the background is not usually black but dim gray I believe.)
>>>>>>
>>>>>> Got it, we can add this to the list of colors to test.
>>>>>>
>>>>>> FWIW, I think as long as we keep the test structure as grabbing a
>>>>>> reference CRC from an FB commit then comparing that to a CRC from a
>>>>>> solid fill commit, I'm not expecting a difference in CRC values.
>>>>>
>>>>> The worry I had here, is that different hardware may have different
>>>>> precision for the solid fill. Maybe that can be worked around by
>>>>> computing the solid fill blob values from the raw FB pixel values? Then
>>>>> even if something gets rounded/truncated somewhere in the hardware, the
>>>>> end result should be the same between FB and solid fill, right?
>>>>
>>>> Hi Pekka,
>>>>
>>>> Got it -- I see what you mean.
>>>>
>>>> In that case, can we stick to just testing the basic RGB + black/white
>>>> colors? I want to avoid adding writeback as a dependency for the solid
>>>> fill test since it's not a dependency for the solid fill feature itself.
>>>
>>> Up to you. My worries here are hypothetical, I don't know about
>>> hardware. It's also possible to adjust tests later if people find false
>>> positives or false negatives.
>>>
>>> Personally, I'd start with optimistically strict tests. False negatives
>>> will be found by driver developers easily, while false negatives would
>>> need an actual user reporting a bug.
>>>
>>>
>>> Thanks,
>>> pq
>>>    
>>
>> I agree with Jessica here. We can start with the basic RGB + black/white.
>>
>> We dont want to put an extra writeback dependency on this test as there
>> is no hardware dependency of solid fill planes with writeback connector.
> 
> Yes, but you can still test mid-gray pixel values with CRC. Craft the
> test values to carefully match the FB pixel values, and see if any
> driver has a problem with it. Maybe it just works.

Hi Pekka,

Sounds good. I'll add a few mid gray test cases with pixel values that 
work for us.

Thanks,

Jessica Zhang

> 
> 
> Thanks,
> pq
> 
>> If others would like to extend this later on based on the writeback
>> client cap to add another sub-test that would be a separate effort.
>>
>>>>>
>>>>> Unless, the hardware precision on solid fill values is less than FB
>>>>> pixel precision, and the CRC input precision is high enough to show
>>>>> that difference.
>>>>>
>>>>>
>>>>> Thanks,
>>>>> pq
>>>    
>