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

Fri Jan 18 11:19:12 UTC 2019

On Thu, Jan 17, 2019 at 04:59:49PM +0100, Daniel Vetter wrote:
> On Thu, Jan 17, 2019 at 3:54 PM Liviu Dudau <liviu.dudau at arm.com> wrote:
> >
> > On Thu, Jan 17, 2019 at 01:32:15PM +0100, Daniel Vetter wrote:
> > > On Thu, Jan 17, 2019 at 1:26 PM Liviu Dudau <liviu.dudau at arm.com> wrote:
> > > >
> > > > 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.
> > > >
> > > > Yeah, but if the "tests for all the existing stuff" doesn't exist, your
> > > > _new_ feature tests are not going to mean much, doesn't it?
> > >
> > > Why? There's still good test coverage for the new stuff at least. And
> > > eventually someone gets bored and fills in the gaps. This is how we've
> > > created all the current igt testcases, so it works - mandatory igt for
> > > new features is the rule for anything intel does since well over 5
> > > years.
> >
> > Because if you can't test basic stuff with igt how did you test your new
> > feature? What I'm saying is: if developer X is having to use igt for
> > a new feature then he is either: a) expecting that basic testing works for
> > his driver (or he has bootstrapping isssues) or that b) if the feature
> > doesn't get tested on driver A because that driver doesn't even have basic
> > tests passing for it then the feature is acceptable. And it will be helpful
> > to publish the list of igt tests that pass for each driver in KMS.
> 
> I think this is an entirely different discussion, something along the
> lines of "new drivers/new features implemented on existing drivers"
> must come with igt test results to prove it's working. Not what I'm
> proposing here at all. It would be a good discussion to have, but a
> different one I think.

New features implemented on existing drivers is what I'm expecting to apply most
of the time when we add the new test requirement. The new UAPI is likely to
come out as a result of finding better ways to handle existing code, but I might
be wrong here.

> 
> The proposal here is only for new uapi: Thus far the requirement is
> "driver + userspace", not it would be "driver + userspace + igt, if
> generic feature".
> 
> > Alternatively, you need to explain better what "driver-agnostic testcase" is
> > (my reading: a testcase that can be run on any driver). Or point to some igt
> > testcases that are considered driver-agnostic and known to run on different
> > (read: non-x86) platforms.
> 
> If this is just about clarifying "driver-agnostic testcase" then that
> boils down to drm_open(DRIVER_ANY). Plus reasonable engineering calls
> on what exactly agnostic means, because in practice I expect that
> "works on first driver, doesn't on 2nd" is fairly common, no matter
> which driver is the first one, and which is the second.

Driver-agnostic to me means "it uses KMS API without any driver-specific #defines
or IOCTLs". Now, I agree that you can come up with examples where this breaks down
really quickly, like posting a framebuffer via atomic IOCTL is driver
agnostic until you start talking about modifiers (and even finding a common format
other than RGB888 might be a stretch). I am (slowly, sorry about that) starting to
understand your point of view, that when you have done driver and userspace 
development, adding an igt testcase is not a big deal, but I have recollections of
my first encounter with igt and finding it daunting to decide which tests should
be run at the most basic level to even claim that igt supports my driver. Assuming
a seasoned KMS driver developer now being faced with IGT tests as a new requirement,
I was trying to preempt complaints that the test suite in itself is not very easy
to "port" to a new driver.

> 
> Now for a list of guaranteed-to-be-generic tests, I'm slowly pushing
> vkms forward to get there. But it's going to take a lot of internships
> to make vkms featureful enough to be useful in such a role.
> 
> If you're just looking for i915 test results, we publish them all. And
> afaik no one else does so, so not really possible to point at anything
> else.

Sorry for not being in constant touch with i915 development (and being lazy), can
you share the URL for the published test results? Should that URL be added in the
documentation (for igt or kernel)?

> 
> > > > > 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.
> > > >
> > > > I would like to see some proof of that in the form of ....
> > > >
> > > >
> > > > > Should this be clarified further?
> > > >
> > > > an URL that points to a page similar to Mesa's GL supported features
> > > > would be nice to add here, from my point of view.
> > >
> > > There isn't even a list of upstream drm features ...
> >
> > Yeah, I know that! :)
> >
> > > how exactly should that look like?
> >
> > What about a list of igt tests then? OR we could put in the igt TODO file
> > the kind of tests we would like to have at some moment?
> 
> I'm still not clear what your concern is and what you want to achieve
> here. There's lots of good ideas we can push for around igt, so
> arbitrarily filling a todo file isn't hard. But I also don't see the
> point (we do have some todo items btw on gitlab I think).
> -Daniel

Concerned that a new requirement added to the DRM framework can be met with pushbacks
if the tool is not usable or generates false negatives. Concerned that adding a generic
feature like writeback into igt is taking an excessive amount of time to be reviewed
and merged and that in the future getting through this process is going to be mandatory
for any new feature.

Best regards,
Liviu

> 
> >
> > Best regards,
> > Liviu
> >
> >
> > > Only thing I can tell you is that we had huge
> > > amounts of todo for missing igt tests, and afaik we've worked them all
> > > down. We = intel here. It's not perfect, but good enough that we've
> > > essentially stopped doing any validation except by running igt. Of
> > > course you can always write more tests, same way you can always
> > > bikeshed a patch a few more times. Eventually there's dimishing
> > > returns.
> > > -Daniel
> > >
> > > > Best regards,
> > > > Liviu
> > > >
> > > > > -Daniel
> > > > >
> > > > > >
> > > > > > Best regards,
> > > > > > Liviu
> > > > > >
> > > > > >
> > > > > > > +
> > > > > > >  Validating changes with IGT
> > > > > > >  ---------------------------
> > > > > > >
> > > > > > > --
> > > > > > > 2.20.1
> > > > > > >
> > > > > >
> > > > >
> > > > > --
> > > > > Daniel Vetter
> > > > > Software Engineer, Intel Corporation
> > > > > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> > > >
> > > > --
> > > > ====================
> > > > | I would like to |
> > > > | fix the world,  |
> > > > | but they're not |
> > > > | giving me the   |
> > > >  \ source code!  /
> > > >   ---------------
> > > >     ¯\_(ツ)_/¯
> > >
> > >
> > >
> > > --
> > > Daniel Vetter
> > > Software Engineer, Intel Corporation
> > > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> >
> > --
> > ====================
> > | I would like to |
> > | fix the world,  |
> > | but they're not |
> > | giving me the   |
> >  \ source code!  /
> >   ---------------
> >     ¯\_(ツ)_/¯
> 
> 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯