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

Modem, Bhanuprakash
2020-08-06T08:30:46Z

> -----Original Message-----
> From: Latvala, Petri <petri.latvala@intel.com>
> Sent: Thursday, August 6, 2020 1:47 PM
> To: Modem, Bhanuprakash <bhanuprakash.modem@intel.com>
> Cc: igt-dev@lists.freedesktop.org; Sharma, Swati2
> <swati2.sharma@intel.com>; B S, Karthik <karthik.b.s@intel.com>; B, Jeevan
> <jeevan.b@intel.com>; Arkadiusz Hiler <arek@hiler.eu>
> Subject: Re: [PATCH v2] tests/kms_pipe_crc_basic: Sanity check for CRC
> mismatches
>
> On Thu, Aug 06, 2020 at 06:14:50PM +0530, 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.
> >
> > This patch will create two framebuffers (fb0 & fb1) with the same
> > color info, flip fb0 with primary plane & collect CRC as reference.
> > Then flip fb1 with primary plane, collect the CRC and compare with
> > the reference CRC. There should be no CRC mismatch.
> >
> > v2:
> > * Run subtest to all pipes (Karthik & Swati)
> > * Fix commit message & few comments (Karthik)
> > * Use meaningful name for functions (Swati)
> > * Remove unwanted checks (Karthik)
> >
> > 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 | 73 ++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 73 insertions(+)
> >
> > diff --git a/tests/kms_pipe_crc_basic.c b/tests/kms_pipe_crc_basic.c
> > index cb93c1ad..599047cb 100644
> > --- a/tests/kms_pipe_crc_basic.c
> > +++ b/tests/kms_pipe_crc_basic.c
> > @@ -154,6 +154,75 @@ 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 with the Primary plane, collect the CRC & compare with
> > + * the ref CRC.
> > + *
> > + * No CRC mismatch should happen
> > + */
> > +static void test_compare_crc(data_t *data, enum pipe pipe)
> > +{
> > + igt_display_t *display = &data->display;
> > + igt_plane_t *primary;
> > + drmModeModeInfo *mode;
> > + igt_crc_t ref_crc, crc;
> > + igt_pipe_crc_t *pipe_crc = NULL;
> > + struct igt_fb fb0, fb1;
> > + igt_output_t *output = igt_get_single_output_for_pipe(display,
> pipe);
> > +
> > + igt_require_f(output, "No connector found for pipe %s\n",
> > + kmstest_pipe_name(pipe));
> > +
> > + igt_require_pipe(display, pipe);
> > + igt_display_reset(display);
> > + 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);
> > +
> > + /* 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);
> > +
> > + 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);
> > + 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);
> > +}
> > +
> > data_t data = {0, };
> >
> > igt_main
> > @@ -209,6 +278,10 @@ igt_main
> >
> > igt_disallow_hang(data.drm_fd, hang);
> > }
> > +
> > + igt_describe("Basic sanity check for CRC mismatches");
> > + igt_subtest_f("compare-crc-basic-%s", kmstest_pipe_name(pipe))
>
> Please change the name to "compare-crc-sanitycheck-pipe-%s". For one,
> calling it "basic" is redundant, this test binary already says
> "basic". Another change is the per-pipe naming convention used
> elsewhere, "pipe-%s".
[Bhanu] Thanks for the review, I'll make these changes in next revision.
>
> This test should go to BAT. Do you want to add a
> fast-feedback.testlist change separately or can it go to this patch?
[Bhanu] Having a separate commit seems a good option for me.
>
>
> --
> Petri Latvala