[igt-dev] [PATCH i-g-t v2 2/3] lib/debugfs: Don't do CRC sanity checks on amdgpu

[Daniel Vetter]

On Wed, Mar 27, 2019 at 05:17:21PM +0000, Kazlauskas, Nicholas wrote:
> On 3/27/19 1:09 PM, Daniel Vetter wrote:
> > On Wed, Mar 27, 2019 at 4:11 PM Wentland, Harry <Harry.Wentland at amd.com> wrote:
> >>
> >>
> >>
> >> On 2019-03-15 11:04 a.m., Nicholas Kazlauskas wrote:
> >>> A black FB on amdgpu returns a CRC of (0, 0, 0), which IGT considers
> >>> suspicious. All our CRC values are also 16-bit so a value of 0xffffffff
> >>> can't be obtained.
> >>>
> >>> Drop the suspicious CRC checks on amdgpu by checking the device in
> >>> crc_sanity_checks. We need the drm_fd for this so pass in pipe_crc
> >>> to the function to get it. It makes more sense to me to do it this
> >>> way than to duplicate code and the explanation on both calls to
> >>> crc_sanity_checks.
> >>>
> >>> v2: rebase
> >>>
> >>> Cc: Leo Li <sunpeng.li at amd.com>
> >>> Cc: Harry Wentland <harry.wentland at amd.com>
> >>> Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas at amd.com>
> >>
> >> Reviewed-by: Harry Wentland <harry.wentland at amd.com>
> > 
> > Hm, I think simpler to just drop this test entirely. It's kinda bogus.
> > Or at least have the sanity check only for the kms_pipe_crc_basic
> > testcases. But I guess this works too.
> > -Daniel
> Isn't it still useful to keep this around for other tests on i915? Since 
> those special codes could happen anywhere from my understanding.
> 
> Not sure if these are really useful for any other drivers, but I figured 
> it was easier to just make an exception in this case for amdgpu since 
> the tests are explicitly bogus on it.

It essentially tests for "did we never record a crc and it's still all 0"
and "has the hw died and returns all 1s". But those values can happen too,
so I'm not sure it's all that useful really even on i915.

Except maybe in the basic "do our crcs produce useful data" testcase, and
there we could perhaps do an explicit if (intel) { crc sanity checks for
i915 ) logic.

Just a drive-by idea really.
-Daniel
> 
> Nicholas Kazlauskas
> 
> > 
> >>
> >> Harry
> >>
> >>> ---
> >>>   lib/igt_debugfs.c | 10 +++++++---
> >>>   1 file changed, 7 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/lib/igt_debugfs.c b/lib/igt_debugfs.c
> >>> index 7849faad..dd229c09 100644
> >>> --- a/lib/igt_debugfs.c
> >>> +++ b/lib/igt_debugfs.c
> >>> @@ -872,11 +872,15 @@ igt_pipe_crc_get_crcs(igt_pipe_crc_t *pipe_crc, int n_crcs,
> >>>        return n;
> >>>   }
> >>>
> >>> -static void crc_sanity_checks(igt_crc_t *crc)
> >>> +static void crc_sanity_checks(igt_pipe_crc_t *pipe_crc, igt_crc_t *crc)
> >>>   {
> >>>        int i;
> >>>        bool all_zero = true;
> >>>
> >>> +     /* Any CRC value can be considered valid on amdgpu hardware. */
> >>> +     if (is_amdgpu_device(pipe_crc->fd))
> >>> +             return;
> >>> +
> >>>        for (i = 0; i < crc->n_words; i++) {
> >>>                igt_warn_on_f(crc->crc[i] == 0xffffffff,
> >>>                              "Suspicious CRC: it looks like the CRC "
> >>> @@ -930,7 +934,7 @@ void igt_pipe_crc_get_single(igt_pipe_crc_t *pipe_crc, igt_crc_t *crc)
> >>>   {
> >>>        read_one_crc(pipe_crc, crc);
> >>>
> >>> -     crc_sanity_checks(crc);
> >>> +     crc_sanity_checks(pipe_crc, crc);
> >>>   }
> >>>
> >>>   /**
> >>> @@ -959,7 +963,7 @@ igt_pipe_crc_get_current(int drm_fd, igt_pipe_crc_t *pipe_crc, igt_crc_t *crc)
> >>>                }
> >>>        } while (igt_vblank_before_eq(crc->frame, vblank));
> >>>
> >>> -     crc_sanity_checks(crc);
> >>> +     crc_sanity_checks(pipe_crc, crc);
> >>>   }
> >>>
> >>>   /**
> >>>
> >> _______________________________________________
> >> igt-dev mailing list
> >> igt-dev at lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/igt-dev
> > 
> > 
> > 
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch