[igt-dev] [PATCH] drm/doc: Make igts for cross-driver stuff mandatory

[Brian Starkey]

Hi Daniel,

On Thu, Jan 17, 2019 at 12:52:16PM +0100, Daniel Vetter wrote:
> On Thu, Jan 17, 2019 at 12:38 PM Liviu Dudau <liviu.dudau at arm.com> wrote:
> >
> > On Wed, Jan 16, 2019 at 05:39:03PM +0100, Daniel Vetter wrote:
> > > Compared to the RFC[1] no changes to the patch itself, but igt moved
> > > forward a lot:
> > >
> > > - gitlab CI builds with: reduced configs/libraries, arm cross build
> > >   and a sysroot build (should address all the build/cross platform
> > >   concerns raised in the RFC discussions).
> > >
> > > - tests reorganized into subdirectories so that the i915-gem tests
> > >   don't clog the main/shared tests directory anymore
> > >
> > > - quite a few more non-intel people contributing/reviewing/committing
> > >   igt tests patches.
> > >
> > > I think this addresses all the concerns raised in the RFC discussions,
> > > and assuming there's enough Acks and no new issues that pop up, we can
> > > go ahead with this.
> > >
> > > 1: https://patchwork.kernel.org/patch/10648851/
> > > Cc: Petri Latvala <petri.latvala at intel.com>
> > > Cc: Arkadiusz Hiler <arkadiusz.hiler at intel.com>
> > > Cc: Liviu Dudau <liviu.dudau at arm.com>
> > > Cc: Sean Paul <sean at poorly.run>
> > > Cc: Eric Anholt <eric at anholt.net>
> > > Cc: Alex Deucher <alexander.deucher at amd.com>
> > > Cc: Dave Airlie <airlied at redhat.com>
> > > Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>
> > > ---
> > >  Documentation/gpu/drm-uapi.rst | 7 +++++++
> > >  1 file changed, 7 insertions(+)
> > >
> > > diff --git a/Documentation/gpu/drm-uapi.rst b/Documentation/gpu/drm-uapi.rst
> > > index a752aa561ea4..413915d6b7d2 100644
> > > --- a/Documentation/gpu/drm-uapi.rst
> > > +++ b/Documentation/gpu/drm-uapi.rst
> > > @@ -238,6 +238,13 @@ DRM specific patterns. Note that ENOTTY has the slightly unintuitive meaning of
> > >  Testing and validation
> > >  ======================
> > >
> > > +Testing Requirements for userspace API
> > > +--------------------------------------
> > > +
> > > +New cross-driver userspace interface extensions, like new IOCTL, new KMS
> > > +properties, new files in sysfs or anything else that constitutes an API change
> > > +need to have driver-agnostic testcases in IGT for that feature.
> >
> > From an aspirational point of view I am fine with this and you can have
> > my Acked-by: Liviu Dudau <liviu.dudau at arm.com>.
> >
> > From a practical point of view I would like to see a matrix of KMS APIs
> > that are being validated and the drivers that have been tested. Otherwise,
> > the next person that comes and tries to add a new IOCTL, KMS property or new
> > file in sysfs is going to discover that he has subscribed to a much bigger
> > task of getting enough KMS drivers testable in the first place.
> 
> This is what the _new_ features is about, no expectation to write
> tests for all the existing stuff. Although I think there's not really
> any big gaps in igt anymore, we do have at least some (rather rough
> and coarse in some case) test coverage for everything I think. Should
> this be clarified further?
> -Daniel
> 

I share a similar view to Liviu here. I think this new requirement
raises the bar more than you intended.

By saying that all new features must be tested by igt, you're also
implying that a driver must run igt (at some basic level); before the
developers working on that driver can start trying to implement new
features. That puts an additional hurdle in the way of adding stuff
to KMS for people who aren't already using igt.

I'm all for testing, and UAPI being well proven before we merge it,
and even for a central KMS test suite. However, when we (Arm Mali-DP
people) have tried to implement things in igt it's been a battle,
because of various built-in assumptions which it made.

For example, most meaningful igt tests rely on CRC. Much of our HW
doesn't have CRC. CRC could be implemented in theory using writeback,
but that currently doesn't exist. That means you're effectively saying
that we (Arm) can't implement any new cross-device KMS features until
we've either:

 a) also implemented writeback-based CRC in igt OR
 b) implemented the new feature in someone else's driver which does
    support CRC.

That seems a bit out of order to me. It would be like me saying "all
KMS drivers must use Arm's test suite, which uses writeback and pixel
checking", and you'd be in a pickle because you don't have writeback.

In a similar vein, I remember having to fix igt on devices which
didn't have cursor planes, before I could even think about writing
tests.

If you can prove that issues like these are all resolved now in igt,
then great! Otherwise, I don't think igt is "ready" to be used as a
requirement for merging KMS kernel API.

Thanks,
-Brian

> >
> > Best regards,
> > Liviu
> >
> >
> > > +
> > >  Validating changes with IGT
> > >  ---------------------------
> > >
> > > --
> > > 2.20.1
> > >
> >
> > --
> > ====================
> > | I would like to |
> > | fix the world,  |
> > | but they're not |
> > | giving me the   |
> >  \ source code!  /
> >   ---------------
> >     ¯\_(ツ)_/¯
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel