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

Tue Jan 22 13:27:57 UTC 2019

Hi,

On Tue, Jan 22, 2019 at 09:53:20AM +0100, Daniel Vetter wrote:
> On Mon, Jan 21, 2019 at 6:21 PM Daniel Vetter <daniel.vetter at ffwll.ch> wrote:
> >
> > On Mon, Jan 21, 2019 at 12:54 PM Brian Starkey <Brian.Starkey at arm.com> wrote:
> > >
> > > 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.
> >
> > We didn't just pick crcs for lols (or because that's all intel
> > supports), we picked it because it will work for both hw with crc and
> > hw with writeback. I checked with a pile of driver writers way back
> > (over irc), and the interface we picked is something pretty much all
> > display blocks (except the _very_ simple ones) should be able to
> > support. Same discussion also happened again when made the crc
> > interfaces in debugfs more generic.

That doesn't really address my issue, but no matter. I guess I'm
having a hard time separating the existing igts from _new_ igts for
new features; so sorry about that.

Please appreciate that there's honestly a whole lot of work needed to
make existing igts work for CRC using writeback. There's no "nonblock"
or continuous CRC collection using writeback. You also need to request
your CRC before you make your commit, so that the FB can be attached
to the writeback connector. Fixing this up isn't small or trivial,
it's not just a case of "ramp-up", it's pervasive throughout the igt
tree.

That shouldn't need to impact your proposal for new tests, if they are
written with that in mind.

> >
> > > 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.
> 
> With a night's worth of sleep, this here is what annoys me - you
> demand I "prove" that igt works everywhere. That's not realistic.
> Intel is not going to pay the bill to get a CI farm for every drm
> driver existing up&running, including fixing all the bugs that will
> uncover (both in igt and even more so in drivers). Especially not if
> mere mortals can't even buy the hardware. Nor is anyone else going to
> do that. If there are some fundamental overall issues with igt then
> I'm ofc happy to get them addressed (like the build issues raised a
> few months ago).

I'm really not asking you to. I'm sorry that I've annoyed you, and I'm
not being deliberately awkward.

I genuinely don't know what the current state of "not-i915" testing is
in igt. Do you really not think it's a good idea to have that known
and documented before you make igt a mandatory part of the DRM tree?

Sure, there's commits from non-intel folks - heck there's even a few
from me. But I can tell you right away that doesn't mean that igt
works in any meaningful way on my platform. Does it work well enough
for me to add new test cases? Honestly I don't know.

Maybe you can suggest some suites which are expected to already be
fully generic and should run anywhere which we can use as
confirmation? To me, having some reasonable subset of the active
driver devs building and running that on their platforms and reporting
back before you merge this patch wouldn't be unreasonable or
outlandish.

> 
> There's also the options to somehow find funding for an igt clone, but
> if you just find a bit of fund then realistically it's more effective
> to throw it at igt. That's how google funded making igt work on
> non-i915 drivers at least. Or you manage to open source one of the
> existing internal/proprietary test suites, but I think that's going to
> be worse than a fresh clone since on top of finding money you also
> have to wait a few years for legal review.
> -Danie
> 
> 
> > I don't think this is realistic (or Liviu's take on this): Expecting
> > that igt just works on all drivers, and that driver writers don't have
> > to do some ramp-up on a testsuite code base is just not going to
> > happen. If we go with a testsuite then there
> > - will be some ramp up required for people who've never used it
> > - it won't work on every driver under the sun

That's fair, and I'll grant you that it's hard to push people to do
that ramp-up before it's mandatory.

> >
> > The later could perhaps be fixed if we go khronos-style on uapi
> > design, but pls note that the requirements for ratifying new khr
> > extensions are _much_ stricter than what we have right now, or what's
> > proposed here. Plus khr essentially got the testsuite for old gl
> > versions donated by google to them (they still don't have one for
> > legacy gl, only for modern gl and all versions of gles).

meh. There's both advantages and disadvantages to that. I do think
there's a lot of scope for more concrete API design docs.

> >
> > Also, the fundamental difference between igt and all these other kms
> > test suites is that they're all not open source. We're not going to
> > use a closed source to validate i915, nor are we going to pour
> > engineering resources into that.
> >

Yes, obviously we can't use a closed codebase as the test suite for
DRM. I don't think anyone has/would/will suggest that. I was making an
analogy, that if I came to you with an open test-suite, which was
written by Arm and which didn't run on your hardware, and said it was
about to become mandatory, you might have some complaints.

> > So from a theoretical pov I think your argument has some merit, in
> > practice it boils down to "let's not have a test suite, mkay". Is that
> > your stance here?

I think you're twisting my words a bit far there. That's clearly not
what I'm saying. I was quite explicit about that point:

	> I'm all for [...] a central KMS test suite.

If I could magically wish for anything I like, it would be for a
test-suite which was built from the ground up to be *the* generic KMS
test-suite.

We don't live in a magical world, so igt makes a good starting point.
I'm not disputing that. I'm just asking for a sanity check of your
assertions that igt is "ready" today.

Thanks,
-Brian

> > -Daniel
> >
> >