Re: [PATCH i-g-t 4/4] tests/kms_color_pipeline: collect reference CRC once per pipe

["Borah, Chaitanya Kumar" <chaitanya.kumar.borah@intel.com>]

On 2/17/2026 2:18 AM, Sharma, Swati2 wrote:
> Hi Chaitanya
> 
> On 02-02-2026 11:34 am, Chaitanya Kumar Borah wrote:
>> The reference (software-equivalent) CRC was being recomputed
> s/was/is
>> for every plane even though it is identical across planes for
>> a given test configuration. Refactor the test to collect the
>> reference CRC once per pipe and reuse it for all planes.
>>
>> Signed-off-by: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com>
>> ---
>>   tests/kms_color_pipeline.c | 60 ++++++++++++++++++++++++++++----------
>>   1 file changed, 44 insertions(+), 16 deletions(-)
>>
>> diff --git a/tests/kms_color_pipeline.c b/tests/kms_color_pipeline.c
>> index e15f4d79a..8cc055591 100644
>> --- a/tests/kms_color_pipeline.c
>> +++ b/tests/kms_color_pipeline.c
>> @@ -88,16 +88,52 @@ static bool ctm_colorop_only(kms_colorop_t 
>> *colorops[])
>>       return true;
>>   }
>> +static void
>> +test_grab_crc(data_t *data, igt_output_t *output,
>> +          const color_t *fb_color, igt_crc_t *crc /* out */)
> Function to be renamed to capture_ref_crc() ?
> 

Sure, I took some inspiration from igt@kms_plane but capture_ref_crc() 
sounds better.

> Also, shall we add some comment stating why we are capturing CRC of 
> primary plane as ref and not current plane?

Will adding it to commit message do?

> 
>> +{
>> +    struct igt_fb fb;
>> +    drmModeModeInfo *mode;
>> +    igt_plane_t *primary;
>> +    char *crc_str;
>> +    int ret;
>> +
>> +    primary = igt_output_get_plane(output, 0);
> Instead of 0, use DRM_PLANE_TYPE_PRIMARY
>> +
>> +    mode = igt_output_get_mode(output);
>> +    igt_assert_fd(igt_create_fb(data->drm_fd,
>> +                    mode->hdisplay, mode->vdisplay,
>> +                    DRM_FORMAT_XRGB8888,
>> +                    DRM_FORMAT_MOD_LINEAR,
>> +                    &fb));
>> +
>> +    paint_rectangles(data, mode, fb_color, &fb);
>> +    igt_plane_set_fb(primary, &fb);
>> +    ret = igt_display_try_commit_atomic(&data->display, 0, NULL);
>> +    igt_skip_on(ret != 0);
> Shouldn't we have assert here instead?

I was just following the existing test_grab_crc in kms_plane but we can 
use an assert too.

>> +
>> +    igt_pipe_crc_collect_crc(data->pipe_crc, crc);
> Initially before collecting CRC, we were waiting for vblank, shouldn't 
> we add same?
>> +
>> +    igt_plane_set_fb(primary, NULL);
>> +    igt_display_commit_atomic(&data->display, 0, NULL);
>> +
>> +    igt_remove_fb(data->drm_fd, &fb);
>> +
>> +    crc_str = igt_crc_to_string(crc);
>> +    igt_debug("CRC for reference fb: %s\n", crc_str);
>> +    free(crc_str);
>> +}
>> +
>>   static void _test_plane_colorops(data_t *data, enum pipe pipe,
>>                    igt_plane_t *plane,
>>                    const color_t *fb_colors,
>> -                 const color_t *exp_colors,
>> +                 igt_crc_t *crc_ref,
>>                    kms_colorop_t *colorops[])
>>   {
>>       igt_display_t *display = &data->display;
>>       drmModeModeInfo *mode = data->mode;
>>       igt_colorop_t *color_pipeline;
>> -    igt_crc_t crc_ref, crc_pipe;
>> +    igt_crc_t crc_pipe;
>>       struct igt_fb fb;
>>       color_pipeline = get_color_pipeline(display, plane, colorops);
>> @@ -110,18 +146,6 @@ static void _test_plane_colorops(data_t *data, 
>> enum pipe pipe,
>>                    DRM_FORMAT_XRGB8888,
>>                    DRM_FORMAT_MOD_LINEAR,
>>                    &fb));
>> -    igt_plane_set_fb(plane, &fb);
>> -
>> -    igt_display_commit_atomic(&data->display, 0, NULL);
>> -
>> -    /* Reference (software-equivalent) CRC */
>> -    set_color_pipeline_bypass(plane);
>> -    paint_rectangles(data, mode, exp_colors, &fb);
>> -
>> -    igt_plane_set_fb(plane, &fb);
>> -    igt_display_commit_atomic(&data->display, 0, NULL);
>> -    igt_wait_for_vblank(plane->crtc);
>> -    igt_pipe_crc_collect_crc(data->pipe_crc, &crc_ref);
>>       /* Hardware pipeline CRC */
>>       set_color_pipeline(display, plane, colorops, color_pipeline);
>> @@ -139,7 +163,7 @@ static void _test_plane_colorops(data_t *data, 
>> enum pipe pipe,
>>       igt_wait_for_vblank(plane->crtc);
>>       igt_pipe_crc_collect_crc(data->pipe_crc, &crc_pipe);
>> -    igt_assert_crc_equal(&crc_ref, &crc_pipe);
>> +    igt_assert_crc_equal(crc_ref, &crc_pipe);
>>       /* Cleanup per-test state */
>>       set_color_pipeline_bypass(plane);
>> @@ -158,6 +182,9 @@ static void test_plane_colorops(data_t *data, enum 
>> pipe pipe,
>>       int n_planes = igt_crtc_for_pipe(&data->display, pipe)->n_planes;
>>       igt_output_t *output = data->output;
>>       igt_plane_t *plane;
>> +    igt_crc_t reference_crc;
> Can we make this as ref_crc?

Sure.

==
Chaitanya

>> +
>> +    test_grab_crc(data, output, exp_colors, &reference_crc);
>>       for (int plane_id = 0; plane_id < n_planes; plane_id++) {
>>           plane = igt_output_get_plane(output, plane_id);
>> @@ -166,7 +193,8 @@ static void test_plane_colorops(data_t *data, enum 
>> pipe pipe,
>>               continue;
>>           igt_dynamic_f("pipe-%s-plane-%u", kmstest_pipe_name(pipe), 
>> plane_id)
>> -            _test_plane_colorops(data, pipe, plane, fb_colors, 
>> exp_colors, colorops);
>> +            _test_plane_colorops(data, pipe, plane, fb_colors,
>> +                         &reference_crc, colorops);
>>       }
>>   }