Re: [Intel-gfx] [PATCH i-g-t v2] tests/kms_crtc_background_color: overhaul for latest ABI proposal (v2)

2018-11-13 Thread Matt Roper
On Tue, Nov 13, 2018 at 11:43:12PM +, Lionel Landwerlin wrote:
> On 13/11/2018 23:22, Matt Roper wrote:
> > It's worth noting that we don't seem to be able to test this feature
> > with CRC's.  Originally we wanted to draw a color into a plane's FB
> > (with Cairo) and then compare the CRC to turning off all planes and just
> > setting the CRTC background to the same color.  However the precision
> > and rounding of the color components causes the CRC's to come out
> > differently, even though the end result is visually identical.  So at
> > the moment this test is mainly useful for visual inspection in
> > interactive mode.
> Hey Matt,
> Out of curiosity, are the CRCs different only when with 16bpc? Or with 8bpc
> too?

Both 8 and 16 give different CRC's.  I suspect it's because cairo
handles the color values (which get passed as double) slightly
differently internally, although I haven't ruled out a slight inaccuracy
on the hardware side.  Given the alpha problems on gen9/gen10, it
wouldn't surprise me if it turns out that our hardware is also treating
a 10bpc component of 0x3ff as 0.999 rather than 1.0.


Matt

> 
> Thanks,
> 
> -
> Lionel
> 
> 
> 

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx


Re: [Intel-gfx] [PATCH i-g-t v2] tests/kms_crtc_background_color: overhaul for latest ABI proposal (v2)

2018-11-13 Thread Lionel Landwerlin

On 13/11/2018 23:22, Matt Roper wrote:

It's worth noting that we don't seem to be able to test this feature
with CRC's.  Originally we wanted to draw a color into a plane's FB
(with Cairo) and then compare the CRC to turning off all planes and just
setting the CRTC background to the same color.  However the precision
and rounding of the color components causes the CRC's to come out
differently, even though the end result is visually identical.  So at
the moment this test is mainly useful for visual inspection in
interactive mode.

Hey Matt,
Out of curiosity, are the CRCs different only when with 16bpc? Or with 
8bpc too?


Thanks,

-
Lionel



___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx


[Intel-gfx] [PATCH i-g-t v2] tests/kms_crtc_background_color: overhaul for latest ABI proposal (v2)

2018-11-13 Thread Matt Roper
CRTC background color kernel patches were written about 2.5 years ago
and floated on the upstream mailing list, but since no opensource
userspace materialized, we never actually merged them.  However the
corresponding IGT test did get merged and has basically been dead code
ever since.

A couple years later we may finally be getting closer to landing the
kernel patches (there's some interest in this functionality now from
both the ChromeOS and Weston camps), so lets update the IGT test to
match the latest proposed ABI, and to remove some of the cruft from the
original test that wouldn't actually work.

It's worth noting that we don't seem to be able to test this feature
with CRC's.  Originally we wanted to draw a color into a plane's FB
(with Cairo) and then compare the CRC to turning off all planes and just
setting the CRTC background to the same color.  However the precision
and rounding of the color components causes the CRC's to come out
differently, even though the end result is visually identical.  So at
the moment this test is mainly useful for visual inspection in
interactive mode.

v2:
 - Swap red and blue ordering in property value to reflect change
   in v2 of kernel series.

Cc: igt-...@lists.freedesktop.org
Signed-off-by: Matt Roper 
---
 lib/igt_kms.c |   2 +-
 tests/kms_crtc_background_color.c | 221 --
 2 files changed, 120 insertions(+), 103 deletions(-)

diff --git a/lib/igt_kms.c b/lib/igt_kms.c
index d806ccc1..33d6a6fb 100644
--- a/lib/igt_kms.c
+++ b/lib/igt_kms.c
@@ -180,7 +180,7 @@ const char * const 
igt_plane_prop_names[IGT_NUM_PLANE_PROPS] = {
 };
 
 const char * const igt_crtc_prop_names[IGT_NUM_CRTC_PROPS] = {
-   [IGT_CRTC_BACKGROUND] = "background_color",
+   [IGT_CRTC_BACKGROUND] = "BACKGROUND_COLOR",
[IGT_CRTC_CTM] = "CTM",
[IGT_CRTC_GAMMA_LUT] = "GAMMA_LUT",
[IGT_CRTC_GAMMA_LUT_SIZE] = "GAMMA_LUT_SIZE",
diff --git a/tests/kms_crtc_background_color.c 
b/tests/kms_crtc_background_color.c
index 3df3401f..e990ecfa 100644
--- a/tests/kms_crtc_background_color.c
+++ b/tests/kms_crtc_background_color.c
@@ -25,164 +25,181 @@
 #include "igt.h"
 #include 
 
-
 IGT_TEST_DESCRIPTION("Test crtc background color feature");
 
+/*
+ * The original idea was to paint a desired color into a full-screen primary
+ * plane and then compare that CRC with turning off all planes and setting the
+ * CRTC background to the same color.  Unforunately, the rounding and precision
+ * of color values as rendered by cairo vs created by the display controller
+ * are slightly different and give different CRC's, even though they're
+ * visually identical.
+ *
+ * Since we can't really use CRC's for testing, this test is mainly useful for
+ * visual inspection in interactive mode at the moment.
+ */
+
 typedef struct {
int gfx_fd;
-   igt_display_t display;
-   struct igt_fb fb;
-   igt_crc_t ref_crc;
-   igt_pipe_crc_t *pipe_crc;
+   igt_output_t *output;
+   drmModeModeInfo *mode;
 } data_t;
 
-#define BLACK  0x00   /* BGR 8bpc */
-#define CYAN   0x00   /* BGR 8bpc */
-#define PURPLE 0xFF00FF   /* BGR 8bpc */
-#define WHITE  0xFF   /* BGR 8bpc */
-
-#define BLACK640x /* BGR 16bpc */
-#define CYAN64 0x /* BGR 16bpc */
-#define PURPLE64   0x /* BGR 16bpc */
-#define YELLOW64   0x /* BGR 16bpc */
-#define WHITE640x /* BGR 16bpc */
-#define RED64  0x /* BGR 16bpc */
-#define GREEN640x /* BGR 16bpc */
-#define BLUE64 0x /* BGR 16bpc */
+/*
+ * Local copy of proposed kernel uapi
+ */
+static inline __u64
+local_rgba(__u8 bpc, __u16 red, __u16 green, __u16 blue, __u16 alpha)
+{
+   int msb_shift = 16 - bpc;
 
+   return (__u64)alpha << msb_shift << 48 |
+  (__u64)red   << msb_shift << 32 |
+  (__u64)green << msb_shift << 16 |
+  (__u64)blue  << msb_shift;
+}
+#define LOCAL_RGBA_BLUE(c, numbits)  (__u16)((c & 0xull) >> 
(16-numbits))
+#define LOCAL_RGBA_GREEN(c, numbits) (__u16)((c & 0xull<<16) >> 
(32-numbits))
+#define LOCAL_RGBA_RED(c, numbits)   (__u16)((c & 0xull<<32) >> 
(48-numbits))
+#define LOCAL_RGBA_ALPHA(c, numbits) (__u16)((c & 0xull<<48) >> 
(64-numbits))
+
+
+/* 8bpc values */
+#define BLACK  local_rgba(8,0,0,0, 0xff)
+#define REDlocal_rgba(8, 0xff,0,0, 0xff)
+#define GREEN  local_rgba(8,0, 0xff,0, 0xff)
+#define BLUE   local_rgba(8,0,0, 0xff, 0xff)
+#define YELLOW local_rgba(8, 0xff, 0xff,0, 0xff)
+#define WHITE  local_rgba(8, 0xff, 0xff, 0xff, 0xff)
+
+/* 16bpc values */
+#define BLACK64local_rgba(16,  0,  0,  0, 0x)
+#define RED64  local_rgba(16, 0x,  0,  0, 0x)
+#define GREEN64local_rgba(16,