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

[Pekka Paalanen <pekka.paalanen@collabora.com>]

[-- Attachment #1: Type: text/plain, Size: 7730 bytes --]

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.


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  
> >   


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]