From mboxrd@z Thu Jan 1 00:00:00 1970 From: Lionel Landwerlin Subject: Re: [PATCH i-g-t 0/5] New pipe level color management tests V5 Date: Tue, 8 Mar 2016 11:50:17 +0000 Message-ID: <56DEBC79.3040802@gmail.com> References: <1456420573-4693-1-git-send-email-lionel.g.landwerlin@intel.com> <20160308024641.GB5450@intel.com> <56DEACAC.8080608@intel.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1335811103==" Return-path: Received: from mail-wm0-x22e.google.com (mail-wm0-x22e.google.com [IPv6:2a00:1450:400c:c09::22e]) by gabe.freedesktop.org (Postfix) with ESMTPS id 0A4CD6E667 for ; Tue, 8 Mar 2016 11:50:27 +0000 (UTC) Received: by mail-wm0-x22e.google.com with SMTP id n186so127576294wmn.1 for ; Tue, 08 Mar 2016 03:50:26 -0800 (PST) In-Reply-To: <56DEACAC.8080608@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Lionel Landwerlin , Matt Roper Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org This is a multi-part message in MIME format. --===============1335811103== Content-Type: multipart/alternative; boundary="------------070402010401080106030203" This is a multi-part message in MIME format. --------------070402010401080106030203 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit On 08/03/16 10:42, Lionel Landwerlin wrote: > 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. Actually, talking with Rob, it might be a bit tricky for ChromeOS. Most of the userspace modifications have been reviewed with the assumption that we didn't need atomic. Switching the userspace as atomic aware might have unintended consequences. > >> 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 > > > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx --------------070402010401080106030203 Content-Type: text/html; charset=utf-8 Content-Transfer-Encoding: quoted-printable
On 08/03/16 10:42, Lionel Landwerlin wrote:
On 08/03/16 02:46, Matt Roper wrote:= 
On Thu, Feb 25, 2016 at 05:16:08PM +0000, Lionel L=
andwerlin wrote:

Hi,

This series enables testing pipe level color management using kernel patc=
hes
from this serie :

https://patchwork.freedesktop.=
org/series/2720/

Most of the tests use pipe CRCs to check the results by comparing the out=
put
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://p= atchwork.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.<=
/pre>
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://p= atchwork.freedesktop.org/series/2720/

The usual flaky tests are : suspend-read-crc-pipe-* and basic-flip-vs-modeset.
The force-load-detect and=C2=A0 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.

Actually, talking with Rob, it might be a bit tricky for ChromeOS. Most of the userspace modifications have been reviewed with the assumption that we didn't need atomic.
Switching the userspace as atomic aware might have unintended consequences.

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.freede=
sktop.org/mailman/listinfo/intel-gfx




_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinf=
o/intel-gfx

--------------070402010401080106030203-- --===============1335811103== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KSW50ZWwtZ2Z4 IG1haWxpbmcgbGlzdApJbnRlbC1nZnhAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlz dHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vaW50ZWwtZ2Z4Cg== --===============1335811103==--