Re: [igt-dev] [PATCH] tests/kms_pipe_crc_basic: Sanity check for CRC mismatches

["Sharma, Swati2" <swati2.sharma@intel.com>]

Hi,

PFI my review comments.
Agree with Karthik's comments except IMO dynamic subtest is not
required if we are testing one o/p per pipe its more than sufficient to
test CRC.

CI is not green for this patch. Please make sure there are no 
regressions. If these are false positive; report to Lakshmi.

On 05-Aug-20 5:13 PM, Karthik B S wrote:
> 
> 
> On 7/30/2020 3:59 AM, Bhanuprakash Modem wrote:
>> We’ve seen multiple CRC related issues on older platforms and
>> pre-silicon environment, most of the time we end up with debugging.
>>
>> This patch works as a crc-sanity test, which can verify that the
>> CRC mechanism is clean from the platform side before debugging the
>> actual CRC mismatches or other CRC related issues.
>>
> 
> Please add the test details as well in the commit message.
>> Cc: Swati Sharma <swati2.sharma@intel.com>
>> Cc: Karthik B S <karthik.b.s@intel.com>
>> Cc: Jeevan B <jeevan.b@intel.com>
>> Cc: Petri Latvala <petri.latvala@intel.com>
>> Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
>> Signed-off-by: Bhanuprakash Modem <bhanuprakash.modem@intel.com>
>> ---
>>   tests/kms_pipe_crc_basic.c | 77 ++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 77 insertions(+)
>>
>> diff --git a/tests/kms_pipe_crc_basic.c b/tests/kms_pipe_crc_basic.c
>> index cb93c1ad..a59c6bf1 100644
>> --- a/tests/kms_pipe_crc_basic.c
>> +++ b/tests/kms_pipe_crc_basic.c
>> @@ -154,6 +154,80 @@ static void test_read_crc(data_t *data, enum pipe 
>> pipe, unsigned flags)
>>       }
>>   }
>> +/*
>> + * CRC-sanity test, to make sure there would be no CRC mismatches
>> + *
>> + * - Create two framebuffers (FB0 & FB1) with same color info
>> + * - Flip FB0 with the Primary plane & collect the CRC as ref CRC.
>> + * - Flip FB1, collect CRC & compare with the ref CRC
> Nitpick, but its better if both sentences are consistent.
> In the first, primary plane is mentioned, but in the second
> nothing is mentioned. Little ambiguous for me.
>> + *
>> + *  No CRC mismatch should happen
>> + */
>> +static void test_check_crc(data_t *data)
>> +{
>> +    igt_display_t *display = &data->display;
>> +    igt_output_t *output;
>> +    enum pipe pipe;
>> +    drmModeModeInfo *mode;
>> +    struct igt_fb fb0, fb1;
>> +    igt_crc_t ref_crc, crc;
>> +    igt_pipe_crc_t *pipe_crc;
>> +    igt_plane_t *primary;
>> +
>> +    for_each_pipe_with_valid_output(display, pipe, output) {
> 
> Can we use 'igt_get_single_output_for_pipe' for the intended pipe 
> instead of using loop when we're only using it for one iteration always.
>> +        igt_output_set_pipe(output, pipe);
>> +        mode = igt_output_get_mode(output);
>> +
>> +        /* Create two framebuffers with the same color info. */
>> +        igt_create_color_fb(data->drm_fd,
>> +                mode->hdisplay, mode->vdisplay,
>> +                DRM_FORMAT_XRGB8888,
>> +                LOCAL_DRM_FORMAT_MOD_NONE,
>> +                1.0, 1.0, 1.0,
>> +                &fb0);
>> +        igt_create_color_fb(data->drm_fd,
>> +                mode->hdisplay, mode->vdisplay,
>> +                DRM_FORMAT_XRGB8888,
>> +                LOCAL_DRM_FORMAT_MOD_NONE,
>> +                1.0, 1.0, 1.0,
>> +                &fb1);

Alignment is off.
>> +
>> +        /* Flip FB0 with the Primary plane & collect the CRC as ref 
>> CRC. */
>> +        primary = igt_output_get_plane_type(output, 
>> DRM_PLANE_TYPE_PRIMARY);
>> +        igt_plane_set_fb(primary, &fb0);
>> +        igt_display_commit(display);
>> +
>> +        if (pipe_crc)
>> +            igt_pipe_crc_free(pipe_crc);
> 
> Is this check required?
>> +
>> +        pipe_crc = igt_pipe_crc_new(data->drm_fd, pipe,
>> +                        INTEL_PIPE_CRC_SOURCE_AUTO);
>> +        igt_pipe_crc_collect_crc(pipe_crc, &ref_crc);
>> +
>> +        /* Flip FB1 with the Primary plane & compare the CRC with ref 
>> CRC. */
>> +        igt_plane_set_fb(primary, &fb1);
>> +        igt_display_commit(display);
>> +
>> +        igt_pipe_crc_collect_crc(pipe_crc, &crc);
>> +        igt_assert_crc_equal(&crc, &ref_crc);
>> +
>> +        /* Clean-up */
>> +        igt_pipe_crc_free(pipe_crc);
>> +        pipe_crc = NULL;
>> +        igt_plane_set_fb(primary, NULL);
>> +        igt_output_set_pipe(output, PIPE_NONE);
>> +        igt_display_commit(display);
>> +
>> +        igt_remove_fb(data->drm_fd, &fb0);
>> +        igt_remove_fb(data->drm_fd, &fb1);
>> +
>> +        /* once is enough */
>> +        return;
>> +    }
>> +
>> +    igt_skip("no valid crtc/connector combinations found\n");
> 
> Based on the comments above, we can remove the return and igt_skip();
>> +}
>> +'
>>   data_t data = {0, };
>>   igt_main
>> @@ -211,6 +285,9 @@ igt_main
>>           }
>>       }
>> +    igt_subtest("compare-crc-basic")
>> +                test_check_crc(&data);

Can this be renamed to something more meaningful maybe test_compare_crc 
or test_crc_sanity?
Also igt_describe() is missing.
> 
> Should we have this at a per pipe level like the other subtests?
> May be dynamic-subtests, but not sure because other tests in this IGT 
> are not dynamic.
> 
> Thanks,
> Karthik.B.S
>> +
>>       igt_fixture {
>>           igt_display_fini(&data.display);
>>       }
>>

-- 
~Swati Sharma
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev