[igt-dev] [PATCH 1/2 i-g-t][V2] tests/kms_universal_plane: Skip off screen universal calls on AMD devices

[Mark Yacoub]

@Petri and @Sean, what do you think of this case?
Is this something the driver should allow to happen or not necessarily?

On Mon, May 9, 2022 at 12:58 PM Hung, Alex <Alex.Hung at amd.com> wrote:
>
> [AMD Official Use Only - General]
>
>
> I think the idea is to avoid wasting of HW resources when it's not visible to end-users. If the check is removed in driver there will be other errors since the driver is not designed to handle this.
what errors do you see?
>
> If it is not visible to end-users, current AMD driver behaviours are more optimal in IMHO.
Hmm, I'm not sure about this. Not cause I disagree with you, but that
the behavior is not check against in DRM.
I'm not a fan at all if hardware specific code, so if this is a case
that DRM should allow then it should pass on AMD. if it's useless then
the test is pointless.
Deferring to Petri and Sean's judgment.
> ________________________________
> From: Mark Yacoub <markyacoub at chromium.org>
> Sent: 09 May 2022 10:21
> To: Hung, Alex <Alex.Hung at amd.com>
> Cc: igt-dev at lists.freedesktop.org <igt-dev at lists.freedesktop.org>; markyacoub at google.com <markyacoub at google.com>
> Subject: Re: [igt-dev] [PATCH 1/2 i-g-t][V2] tests/kms_universal_plane: Skip off screen universal calls on AMD devices
>
> On Mon, May 9, 2022 at 11:51 AM Alex Hung <alex.hung at amd.com> wrote:
> >
> > amdgpu_dm intentionally blocks planes outside the screen in the function
> > "dm_plane_helper_check_state" and returns -EINVAL. As a results, this
> do you know why it intentionally does that? What happens if you remove
> this check? Maybe AMD shouldn't check for it? I'm thinking that
> ideally we don't want drivers to have special handling when it's not
> needed and for KMS tests to become a standard for userspace.
> > piece of code should be skipped on AMD devices.
> >
> > Signed-off-by: Alex Hung <alex.hung at amd.com>
> > ---
> >  tests/kms_universal_plane.c | 22 ++++++++++++----------
> >  1 file changed, 12 insertions(+), 10 deletions(-)
> >
> > diff --git a/tests/kms_universal_plane.c b/tests/kms_universal_plane.c
> > index 3cb6d704..3a3667e6 100644
> > --- a/tests/kms_universal_plane.c
> > +++ b/tests/kms_universal_plane.c
> > @@ -242,16 +242,18 @@ functional_test_pipe(data_t *data, enum pipe pipe, igt_output_t *output)
> >         igt_display_commit2(display, COMMIT_LEGACY);
> >
> >         /* Step 14: Universal API, set primary completely offscreen (CRC 9) */
> > -       igt_assert(drmModeSetPlane(data->drm_fd, primary->drm_plane->plane_id,
> > -                                  output->config.crtc->crtc_id,
> > -                                  test.blue_fb.fb_id, 0,
> > -                                  9000, 9000,
> > -                                  test.mode->hdisplay,
> > -                                  test.mode->vdisplay,
> > -                                  IGT_FIXED(0,0), IGT_FIXED(0,0),
> > -                                  IGT_FIXED(test.mode->hdisplay,0),
> > -                                  IGT_FIXED(test.mode->vdisplay,0)) == 0);
> > -       igt_pipe_crc_collect_crc(test.pipe_crc, &test.crc_9);
> > +       if (!is_amdgpu_device(data->drm_fd)) {
> > +               igt_assert(drmModeSetPlane(data->drm_fd, primary->drm_plane->plane_id,
> > +                                          output->config.crtc->crtc_id,
> > +                                          test.blue_fb.fb_id, 0,
> > +                                          9000, 9000,
> > +                                          test.mode->hdisplay,
> > +                                          test.mode->vdisplay,
> > +                                          IGT_FIXED(0,0), IGT_FIXED(0,0),
> > +                                          IGT_FIXED(test.mode->hdisplay,0),
> > +                                          IGT_FIXED(test.mode->vdisplay,0)) == 0);
> > +               igt_pipe_crc_collect_crc(test.pipe_crc, &test.crc_9);
> > +       }
> >
> >         /*
> >          * Step 15: Explicitly disable primary after it's already been
> > --
> > 2.25.1
> >