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

[Wentland, Harry]

On 2019-01-22 2:19 p.m., Daniel Vetter wrote:
> On Tue, Jan 22, 2019 at 8:00 PM Wentland, Harry <Harry.Wentland at amd.com> wrote:
>> On 2019-01-16 11:39 a.m., 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>
>>
>> I'm all for anything resembling TDD and standardizing on one test framework. IGT works quite well for us for testing display stuff. We still have a way to go but have been trying to adopt this requirement lately anyways for the DC driver. Can't really comment on anything beyond display, though, for AMD.
>>
>> No matter how much I want this to be mandatory, seeing the discussions with ARM and the comment about lack of CRC on Nouveau makes me think that we might not be quite ready to go there. Implementing DWB is non-trivial. VKMS knows how to compute a CRC from a framebuffer, but that's the trivial part. Setting up the HW and SW to do DWB is the hard part.
> 
> We also discussed a bit writeback implementations on irc, and it looks
> like you can't really use writeback to accurately test that your
> compositing engine is programmed correctly, since on at least vc4,
> malidp and msm (not yet merged upstream) the writeback engine can't be
> shared with any other outputs, often it even needs a
> dedicated/special-purpose CRTC (at least vc4 from what I can tell).
> That means if you botch your programming and e.g. cause an underrun
> scanning out continous-update outputs, then the writeback won't show
> that to you, since it's composited separately. I guess we could teach
> igt to run these tests on the special crtc->writeback pipeline only,
> but essentially that's a new testcase, and not really testing the
> actual display: It tests writeback, not hdmi/dp/panels/whatever real
> outputs you have.
> 
> I'd say we'll shrug these cases off as "can't be reasonable tested,
> won't have an igt". First driver team with hw that can be validated
> gets to fill the gaps :-) In practice still going to be a lot better
> than no tests at all, just exercising the feature will be useful, and
> will make it a lot easier for the next team to add the crc based tests
> on top.
> 

I think that's reasonable. After all, we want to start somewhere.

Would it make sense to append something like ", if such a test can be reasonably made using IGT for the target HW." to make it clear to contributors that in cases like the one discussed this is at the reviewers discretion?

With that change (or anything else that clarifies your intentions as described above) I'd be happy to give my AB.

Harry

> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
>