Re: [PATCH i-g-t 0/5] New pipe level color management tests V5

Lionel Landwerlin
2016-03-08T10:42:55Z

[-- Attachment #1.1: Type: text/plain, Size: 4261 bytes --]

On 08/03/16 02:46, Matt Roper wrote:
> On Thu, Feb 25, 2016 at 05:16:08PM +0000, Lionel Landwerlin wrote:
>> Hi,
>>
>> This series enables testing pipe level color management using kernel patches
>> from this serie :
>>
>> https://patchwork.freedesktop.org/series/2720/
>>
>> Most of the tests use pipe CRCs to check the results by comparing the output
>> with the expected output drawn using cairo.
> These tests look good. The only thing that might be worth adding is a
> test that intermixes the legacy gamma ioctl with full color management
> use. I believe invoking the legacy ioctl should clear the degamma and
> CTM, right? It might be worth checking that that actually happens.
That's right. We have drm_atomic_helper_legacy_gamma_set helper() to do
this (https://patchwork.freedesktop.org/patch/75106/).
I'll add a test exercising that back and forth.

>
> Also, there was one misplaced patch hunk that I called out on patch 2/3.
Fixing.

>
> Aside from the final IGT tweaks, I think we're pretty close to merging.
> Here are the final opens I see:
>
> * I expect we'll just merge this whole thing through the Intel tree,
> even though patch #3 adds the properties, structures, and helpers to
> the DRM core, so we need to get an okay from airlied. Dave, do you
> have any concerns about this?
>
> * I know the ChromeOS guys have been reviewing this series
> independently on their end and putting together the userspace (open
> source) which will be using this. We should get someone to formally
> Ack this on their behalf; I think Rob Bradford has been working with
> them; adding him to the Cc list to see if he can provide the Ack.
>
> * I'm not sure if I ever saw a CI report for your final iteration. I
> may have just missed the email, or it may have never come out due to
> the temporary FDO downtime or the CI outage last week. Could you
> provide a pointer to the results (with false positives justified with
> bugzilla entries), or resubmit again for CI if necessary?

It's attached to the patchwork series :
https://patchwork.freedesktop.org/series/2720/

The usual flaky tests are : suspend-read-crc-pipe-* and
basic-flip-vs-modeset.
The force-load-detect and error-state-basic were failing for other
people at the time I posted the v10 series.

>
> * I'm still a little bit uncomfortable that we (the general DRM
> community) never really came to a consensus on the whole
> DRM_MODE_PROP_ATOMIC vs not question (both for this patchset and as a
> general guideline going forward with similar features). Emil had a
> good argument that if we make something atomic-only to start with,
> it's trivial to change in the future if we decide we actually want to
> make use of it with a non-atomic userspace; on the other hand, if we
> make it unrestricted on day one, that's a change we can't reverse
> down the line since it becomes part of the ABI. However nobody
> seemed to really care very strongly either way during the discussion
> on the color management series, so I'm just putting this here as a
> final warning in case anyone really wants to see it changed before
> merging.
I don't mind switching to atomic only.

>
> Thanks!
>
>
> Matt
>
>
>> Cheers,
>>
>> Lionel
>>
>> Lionel Landwerlin (5):
>> lib: kms: add crtc_id to igt_pipe_t
>> lib: kms: add helpers for color management properties on pipes
>> lib: fb: add igt_paint_color_gradient_range
>> lib: add crc comparison function without an assert
>> tests/kms_color: New test for pipe level color management
>>
>> lib/igt_debugfs.c | 17 +
>> lib/igt_debugfs.h | 1 +
>> lib/igt_fb.c | 34 ++
>> lib/igt_fb.h | 3 +
>> lib/igt_kms.c | 75 ++++
>> lib/igt_kms.h | 18 +-
>> tests/Makefile.sources | 1 +
>> tests/kms_pipe_color.c | 1045 ++++++++++++++++++++++++++++++++++++++++++++++++
>> 8 files changed, 1193 insertions(+), 1 deletion(-)
>> create mode 100644 tests/kms_pipe_color.c
>>
>> --
>> 2.7.0
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[-- Attachment #1.2: Type: text/html, Size: 5951 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]