[PATCH v6 0/3] CRTC background color

[PATCH v6 0/3] CRTC background color

[Matt Roper]

This version is just a rebase of v5 onto the latest drm-tip, which was
posted here:
  https://lists.freedesktop.org/archives/intel-gfx/2019-January/188352.html

There were some minor conflicts with Ville's csc/gamma disable series,
so the background color write has now moved to the new color_commit
function, but that's about the only notable rebase change.

Reviewed userspace (chromeos) is here:
  https://chromium-review.googlesource.com/c/chromium/src/+/1278858
  https://chromium-review.googlesource.com/c/chromiumos/platform/drm-tests/+/1241436

I've made some minor changes to the i-g-t test, so I'll resend those
again shortly.

All the kernel patches are reviewed now, so I think this series is ready
to merge.  Since the i915 patches rely on other stuff that's landed
pretty recently in dinq, it's probably easiest to merge this via the
i915 tree; just need an Ack from Dave/Daniel.

Matt Roper (3):
  drm: Add CRTC background color property (v5)
  drm/i915/gen9+: Add support for pipe background color (v6)
  drm/i915: Add background color hardware readout and state check

 drivers/gpu/drm/drm_atomic_uapi.c    |  4 ++++
 drivers/gpu/drm/drm_blend.c          | 41 +++++++++++++++++++++++++++++++---
 drivers/gpu/drm/drm_mode_config.c    |  6 +++++
 drivers/gpu/drm/i915/i915_debugfs.c  |  9 ++++++++
 drivers/gpu/drm/i915/intel_color.c   | 11 ++++++---
 drivers/gpu/drm/i915/intel_display.c | 43 ++++++++++++++++++++++++++++++++++++
 include/drm/drm_blend.h              |  1 +
 include/drm/drm_crtc.h               | 12 ++++++++++
 include/drm/drm_mode_config.h        |  5 +++++
 include/uapi/drm/drm_mode.h          | 28 +++++++++++++++++++++++
 10 files changed, 154 insertions(+), 6 deletions(-)

-- 
2.14.5

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

[PATCH v6 1/3] drm: Add CRTC background color property (v5)

[Matt Roper]

Some display controllers can be programmed to present non-black colors
for pixels not covered by any plane (or pixels covered by the
transparent regions of higher planes).  Compositors that want a UI with
a solid color background can potentially save memory bandwidth by
setting the CRTC background property and using smaller planes to display
the rest of the content.

To avoid confusion between different ways of encoding RGB data, we
define a standard 64-bit format that should be used for this property's
value.  Helper functions and macros are provided to generate and dissect
values in this standard format with varying component precision values.

v2:
 - Swap internal representation's blue and red bits to make it easier
   to read if printed out.  (Ville)
 - Document bgcolor property in drm_blend.c.  (Sean Paul)
 - s/background_color/bgcolor/ for consistency between property name and
   value storage field.  (Sean Paul)
 - Add a convenience function to attach property to a given crtc.

v3:
 - Restructure ARGB component extraction macros to be easier to
   understand and enclose the parameters in () to avoid calculations
   if expressions are passed.  (Sean Paul)
 - s/rgba/argb/ in helper function/macro names.  Even though the idea is
   to not worry about the internal representation of the u64, it can
   still be confusing to look at code that uses 'rgba' terminology, but
   stores values with argb ordering.  (Ville)

v4:
 - Drop the bgcolor_changed flag.  (Ville, Brian Starkey)
 - Clarify in kerneldoc that background color is expected to undergo the
   same pipe-level degamma/csc/gamma transformations that planes do.
   (Brian Starkey)
 - Update kerneldoc to indicate non-opaque colors are allowed, but are
   generally only useful in special cases such as when writeback
   connectors are used (Brian Starkey / Eric Anholt)

v5:
 - Set crtc->state->bgcolor to solid black inside
   drm_crtc_add_bgcolor_property() in case drivers don't do this
   themselves.  (Ville)
 - Add kerneldoc to drm_crtc_add_bgcolor_property() function.

Cc: dri-devel@lists.freedesktop.org
Cc: wei.c.li@intel.com
Cc: harish.krupo.kps@intel.com
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Sean Paul <sean@poorly.run>
Cc: Brian Starkey <Brian.Starkey@arm.com>
Cc: Eric Anholt <eric@anholt.net>
Cc: Stéphane Marchesin <marcheu@chromium.org>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
Reviewed-by(v2): Sean Paul <sean@poorly.run>
Reviewed-by: Brian Starkey <brian.starkey@arm.com>
---
 drivers/gpu/drm/drm_atomic_uapi.c |  4 ++++
 drivers/gpu/drm/drm_blend.c       | 41 ++++++++++++++++++++++++++++++++++++---
 drivers/gpu/drm/drm_mode_config.c |  6 ++++++
 include/drm/drm_blend.h           |  1 +
 include/drm/drm_crtc.h            | 12 ++++++++++++
 include/drm/drm_mode_config.h     |  5 +++++
 include/uapi/drm/drm_mode.h       | 28 ++++++++++++++++++++++++++
 7 files changed, 94 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
index 0aabd401d3ca..5436201a6a0d 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -469,6 +469,8 @@ static int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
 			return -EFAULT;
 
 		set_out_fence_for_crtc(state->state, crtc, fence_ptr);
+	} else if (property == config->bgcolor_property) {
+		state->bgcolor = val;
 	} else if (crtc->funcs->atomic_set_property) {
 		return crtc->funcs->atomic_set_property(crtc, state, property, val);
 	} else {
@@ -503,6 +505,8 @@ drm_atomic_crtc_get_property(struct drm_crtc *crtc,
 		*val = (state->gamma_lut) ? state->gamma_lut->base.id : 0;
 	else if (property == config->prop_out_fence_ptr)
 		*val = 0;
+	else if (property == config->bgcolor_property)
+		*val = state->bgcolor;
 	else if (crtc->funcs->atomic_get_property)
 		return crtc->funcs->atomic_get_property(crtc, state, property, val);
 	else
diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c
index 0c78ca386cbe..2ff69fae385c 100644
--- a/drivers/gpu/drm/drm_blend.c
+++ b/drivers/gpu/drm/drm_blend.c
@@ -175,9 +175,22 @@
  *		 plane does not expose the "alpha" property, then this is
  *		 assumed to be 1.0
  *
- * Note that all the property extensions described here apply either to the
- * plane or the CRTC (e.g. for the background color, which currently is not
- * exposed and assumed to be black).
+ * The property extensions described above all apply to the plane.  Drivers
+ * may also expose the following crtc property extension:
+ *
+ * BACKGROUND_COLOR:
+ *	Background color is setup with drm_crtc_add_bgcolor_property().  It
+ *	controls the ARGB color of a full-screen layer that exists below all
+ *	planes.  This color will be used for pixels not covered by any plane
+ *	and may also be blended with plane contents as allowed by a plane's
+ *	alpha values.  The background color defaults to black, and is assumed
+ *	to be black for drivers that do not expose this property.  Although
+ *	background color isn't a plane, it is assumed that the color provided
+ *	here undergoes the same pipe-level degamma/CSC/gamma transformations
+ *	that planes undergo.  Note that the color value provided here includes
+ *	an alpha channel...non-opaque background color values are allowed,
+ *	but are generally only honored in special cases (e.g., when a memory
+ *	writeback connector is in use).
  */
 
 /**
@@ -593,3 +606,25 @@ int drm_plane_create_blend_mode_property(struct drm_plane *plane,
 	return 0;
 }
 EXPORT_SYMBOL(drm_plane_create_blend_mode_property);
+
+/**
+ * drm_crtc_add_bgcolor_property - add background color property
+ * @crtc: drm crtc
+ *
+ * Adds the background color property to @crtc.  The property defaults to
+ * solid black and will accept 64-bit ARGB values in the format generated by
+ * drm_argb().  @crtc's state will also be updated to hold a solid black
+ * background color when this function is called.
+ */
+void drm_crtc_add_bgcolor_property(struct drm_crtc *crtc)
+{
+	u64 initval = drm_argb(16, 0xffff, 0, 0, 0);
+
+	drm_object_attach_property(&crtc->base,
+				   crtc->dev->mode_config.bgcolor_property,
+				   initval);
+
+	if (crtc->state)
+		crtc->state->bgcolor = initval;
+}
+EXPORT_SYMBOL(drm_crtc_add_bgcolor_property);
diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
index 4a1c2023ccf0..8a7c346b3191 100644
--- a/drivers/gpu/drm/drm_mode_config.c
+++ b/drivers/gpu/drm/drm_mode_config.c
@@ -364,6 +364,12 @@ static int drm_mode_create_standard_properties(struct drm_device *dev)
 		return -ENOMEM;
 	dev->mode_config.modifiers_property = prop;
 
+	prop = drm_property_create_range(dev, 0, "BACKGROUND_COLOR",
+					 0, GENMASK_ULL(63, 0));
+	if (!prop)
+		return -ENOMEM;
+	dev->mode_config.bgcolor_property = prop;
+
 	return 0;
 }
 
diff --git a/include/drm/drm_blend.h b/include/drm/drm_blend.h
index 88bdfec3bd88..9e2538dd7b9a 100644
--- a/include/drm/drm_blend.h
+++ b/include/drm/drm_blend.h
@@ -58,4 +58,5 @@ int drm_atomic_normalize_zpos(struct drm_device *dev,
 			      struct drm_atomic_state *state);
 int drm_plane_create_blend_mode_property(struct drm_plane *plane,
 					 unsigned int supported_modes);
+void drm_crtc_add_bgcolor_property(struct drm_crtc *crtc);
 #endif
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 85abd3fe9e83..dbe0b45d5da6 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -274,6 +274,18 @@ struct drm_crtc_state {
 	 */
 	struct drm_property_blob *gamma_lut;
 
+	/**
+	 * @bgcolor:
+	 *
+	 * RGB value representing the pipe's background color.  The background
+	 * color (aka "canvas color") of a pipe is the color that will be used
+	 * for pixels not covered by a plane, or covered by transparent pixels
+	 * of a plane.  The value here should be built via drm_argb();
+	 * individual color components can be extracted with desired precision
+	 * via the DRM_ARGB_*() macros.
+	 */
+	u64 bgcolor;
+
 	/**
 	 * @target_vblank:
 	 *
diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
index 7f60e8eb269a..9a13d8ad92cc 100644
--- a/include/drm/drm_mode_config.h
+++ b/include/drm/drm_mode_config.h
@@ -836,6 +836,11 @@ struct drm_mode_config {
 	 */
 	struct drm_property *writeback_out_fence_ptr_property;
 
+	/**
+	 * @bgcolor_property: RGB background color for CRTC.
+	 */
+	struct drm_property *bgcolor_property;
+
 	/* dumb ioctl parameters */
 	uint32_t preferred_depth, prefer_shadow;
 
diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
index a439c2e67896..5f31e6a05bd9 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -907,6 +907,34 @@ struct drm_mode_rect {
 	__s32 y2;
 };
 
+/*
+ * Put ARGB values into a standard 64-bit representation that can be used
+ * for ioctl parameters, inter-driver commmunication, etc.  If the component
+ * values being provided contain less than 16 bits of precision, they'll
+ * be shifted into the most significant bits.
+ */
+static inline __u64
+drm_argb(__u8 bpc, __u16 alpha, __u16 red, __u16 green, __u16 blue)
+{
+	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;
+}
+
+/*
+ * Extract the specified number of bits of a specific color component from a
+ * standard 64-bit ARGB value.
+ */
+#define DRM_ARGB_COMP(c, shift, numbits) \
+	(__u16)(((c) & 0xFFFFull << (shift)) >> ((shift) + 16 - (numbits)))
+#define DRM_ARGB_BLUE(c, numbits)  DRM_ARGB_COMP(c, 0, numbits)
+#define DRM_ARGB_GREEN(c, numbits) DRM_ARGB_COMP(c, 16, numbits)
+#define DRM_ARGB_RED(c, numbits)   DRM_ARGB_COMP(c, 32, numbits)
+#define DRM_ARGB_ALPHA(c, numbits) DRM_ARGB_COMP(c, 48, numbits)
+
 #if defined(__cplusplus)
 }
 #endif
-- 
2.14.5

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

Re: [PATCH v6 1/3] drm: Add CRTC background color property (v5)

[Maarten Lankhorst]

Hey,

Op 21-02-2019 om 01:28 schreef Matt Roper:
> Some display controllers can be programmed to present non-black colors
> for pixels not covered by any plane (or pixels covered by the
> transparent regions of higher planes).  Compositors that want a UI with
> a solid color background can potentially save memory bandwidth by
> setting the CRTC background property and using smaller planes to display
> the rest of the content.
>
> To avoid confusion between different ways of encoding RGB data, we
> define a standard 64-bit format that should be used for this property's
> value.  Helper functions and macros are provided to generate and dissect
> values in this standard format with varying component precision values.
>
> v2:
>  - Swap internal representation's blue and red bits to make it easier
>    to read if printed out.  (Ville)
>  - Document bgcolor property in drm_blend.c.  (Sean Paul)
>  - s/background_color/bgcolor/ for consistency between property name and
>    value storage field.  (Sean Paul)
>  - Add a convenience function to attach property to a given crtc.
>
> v3:
>  - Restructure ARGB component extraction macros to be easier to
>    understand and enclose the parameters in () to avoid calculations
>    if expressions are passed.  (Sean Paul)
>  - s/rgba/argb/ in helper function/macro names.  Even though the idea is
>    to not worry about the internal representation of the u64, it can
>    still be confusing to look at code that uses 'rgba' terminology, but
>    stores values with argb ordering.  (Ville)
>
> v4:
>  - Drop the bgcolor_changed flag.  (Ville, Brian Starkey)
>  - Clarify in kerneldoc that background color is expected to undergo the
>    same pipe-level degamma/csc/gamma transformations that planes do.
>    (Brian Starkey)
>  - Update kerneldoc to indicate non-opaque colors are allowed, but are
>    generally only useful in special cases such as when writeback
>    connectors are used (Brian Starkey / Eric Anholt)
>
> v5:
>  - Set crtc->state->bgcolor to solid black inside
>    drm_crtc_add_bgcolor_property() in case drivers don't do this
>    themselves.  (Ville)
>  - Add kerneldoc to drm_crtc_add_bgcolor_property() function.
>
> Cc: dri-devel@lists.freedesktop.org
> Cc: wei.c.li@intel.com
> Cc: harish.krupo.kps@intel.com
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Sean Paul <sean@poorly.run>
> Cc: Brian Starkey <Brian.Starkey@arm.com>
> Cc: Eric Anholt <eric@anholt.net>
> Cc: Stéphane Marchesin <marcheu@chromium.org>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> Reviewed-by(v2): Sean Paul <sean@poorly.run>
> Reviewed-by: Brian Starkey <brian.starkey@arm.com>

I like how bgcolor is a u64 now, but there is an issue with setting crtc->state->bgcolor in attaching the property.

We should do it in atomic core init instead, like we already do for connector/plane properties..

See for example https://patchwork.freedesktop.org/series/52363/

This was specificallly for the background color proposal, but we probalby have to split out the non-trivial conversions of __drm_atomic_helper_crtc_reset.

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v6 1/3] drm: Add CRTC background color property (v5)

[Matt Roper]

On Tue, Feb 26, 2019 at 08:26:36AM +0100, Maarten Lankhorst wrote:
> Hey,
> 
> Op 21-02-2019 om 01:28 schreef Matt Roper:
> > Some display controllers can be programmed to present non-black colors
> > for pixels not covered by any plane (or pixels covered by the
> > transparent regions of higher planes).  Compositors that want a UI with
> > a solid color background can potentially save memory bandwidth by
> > setting the CRTC background property and using smaller planes to display
> > the rest of the content.
> >
> > To avoid confusion between different ways of encoding RGB data, we
> > define a standard 64-bit format that should be used for this property's
> > value.  Helper functions and macros are provided to generate and dissect
> > values in this standard format with varying component precision values.
> >
> > v2:
> >  - Swap internal representation's blue and red bits to make it easier
> >    to read if printed out.  (Ville)
> >  - Document bgcolor property in drm_blend.c.  (Sean Paul)
> >  - s/background_color/bgcolor/ for consistency between property name and
> >    value storage field.  (Sean Paul)
> >  - Add a convenience function to attach property to a given crtc.
> >
> > v3:
> >  - Restructure ARGB component extraction macros to be easier to
> >    understand and enclose the parameters in () to avoid calculations
> >    if expressions are passed.  (Sean Paul)
> >  - s/rgba/argb/ in helper function/macro names.  Even though the idea is
> >    to not worry about the internal representation of the u64, it can
> >    still be confusing to look at code that uses 'rgba' terminology, but
> >    stores values with argb ordering.  (Ville)
> >
> > v4:
> >  - Drop the bgcolor_changed flag.  (Ville, Brian Starkey)
> >  - Clarify in kerneldoc that background color is expected to undergo the
> >    same pipe-level degamma/csc/gamma transformations that planes do.
> >    (Brian Starkey)
> >  - Update kerneldoc to indicate non-opaque colors are allowed, but are
> >    generally only useful in special cases such as when writeback
> >    connectors are used (Brian Starkey / Eric Anholt)
> >
> > v5:
> >  - Set crtc->state->bgcolor to solid black inside
> >    drm_crtc_add_bgcolor_property() in case drivers don't do this
> >    themselves.  (Ville)
> >  - Add kerneldoc to drm_crtc_add_bgcolor_property() function.
> >
> > Cc: dri-devel@lists.freedesktop.org
> > Cc: wei.c.li@intel.com
> > Cc: harish.krupo.kps@intel.com
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Sean Paul <sean@poorly.run>
> > Cc: Brian Starkey <Brian.Starkey@arm.com>
> > Cc: Eric Anholt <eric@anholt.net>
> > Cc: Stéphane Marchesin <marcheu@chromium.org>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> > Reviewed-by(v2): Sean Paul <sean@poorly.run>
> > Reviewed-by: Brian Starkey <brian.starkey@arm.com>
> 
> I like how bgcolor is a u64 now, but there is an issue with setting
> crtc->state->bgcolor in attaching the property.
> 
> We should do it in atomic core init instead, like we already do for
> connector/plane properties..
> 
> See for example https://patchwork.freedesktop.org/series/52363/
> 
> This was specificallly for the background color proposal, but we
> probalby have to split out the non-trivial conversions of
> __drm_atomic_helper_crtc_reset.

Makes sense.  What's the status of that patch?  It looks like it has a
lot of a-b's/r-b's but hasn't landed yet.  Is there anything blocking
it?  If you're still driving it forward, I can wait for that to land and
then build on top of it.


Matt

-- 
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: [PATCH v6 1/3] drm: Add CRTC background color property (v5)

[Maarten Lankhorst]

Op 26-02-2019 om 17:17 schreef Matt Roper:
> On Tue, Feb 26, 2019 at 08:26:36AM +0100, Maarten Lankhorst wrote:
>> Hey,
>>
>> Op 21-02-2019 om 01:28 schreef Matt Roper:
>>> Some display controllers can be programmed to present non-black colors
>>> for pixels not covered by any plane (or pixels covered by the
>>> transparent regions of higher planes).  Compositors that want a UI with
>>> a solid color background can potentially save memory bandwidth by
>>> setting the CRTC background property and using smaller planes to display
>>> the rest of the content.
>>>
>>> To avoid confusion between different ways of encoding RGB data, we
>>> define a standard 64-bit format that should be used for this property's
>>> value.  Helper functions and macros are provided to generate and dissect
>>> values in this standard format with varying component precision values.
>>>
>>> v2:
>>>  - Swap internal representation's blue and red bits to make it easier
>>>    to read if printed out.  (Ville)
>>>  - Document bgcolor property in drm_blend.c.  (Sean Paul)
>>>  - s/background_color/bgcolor/ for consistency between property name and
>>>    value storage field.  (Sean Paul)
>>>  - Add a convenience function to attach property to a given crtc.
>>>
>>> v3:
>>>  - Restructure ARGB component extraction macros to be easier to
>>>    understand and enclose the parameters in () to avoid calculations
>>>    if expressions are passed.  (Sean Paul)
>>>  - s/rgba/argb/ in helper function/macro names.  Even though the idea is
>>>    to not worry about the internal representation of the u64, it can
>>>    still be confusing to look at code that uses 'rgba' terminology, but
>>>    stores values with argb ordering.  (Ville)
>>>
>>> v4:
>>>  - Drop the bgcolor_changed flag.  (Ville, Brian Starkey)
>>>  - Clarify in kerneldoc that background color is expected to undergo the
>>>    same pipe-level degamma/csc/gamma transformations that planes do.
>>>    (Brian Starkey)
>>>  - Update kerneldoc to indicate non-opaque colors are allowed, but are
>>>    generally only useful in special cases such as when writeback
>>>    connectors are used (Brian Starkey / Eric Anholt)
>>>
>>> v5:
>>>  - Set crtc->state->bgcolor to solid black inside
>>>    drm_crtc_add_bgcolor_property() in case drivers don't do this
>>>    themselves.  (Ville)
>>>  - Add kerneldoc to drm_crtc_add_bgcolor_property() function.
>>>
>>> Cc: dri-devel@lists.freedesktop.org
>>> Cc: wei.c.li@intel.com
>>> Cc: harish.krupo.kps@intel.com
>>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>> Cc: Sean Paul <sean@poorly.run>
>>> Cc: Brian Starkey <Brian.Starkey@arm.com>
>>> Cc: Eric Anholt <eric@anholt.net>
>>> Cc: Stéphane Marchesin <marcheu@chromium.org>
>>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>>> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
>>> Reviewed-by(v2): Sean Paul <sean@poorly.run>
>>> Reviewed-by: Brian Starkey <brian.starkey@arm.com>
>> I like how bgcolor is a u64 now, but there is an issue with setting
>> crtc->state->bgcolor in attaching the property.
>>
>> We should do it in atomic core init instead, like we already do for
>> connector/plane properties..
>>
>> See for example https://patchwork.freedesktop.org/series/52363/
>>
>> This was specificallly for the background color proposal, but we
>> probalby have to split out the non-trivial conversions of
>> __drm_atomic_helper_crtc_reset.
> Makes sense.  What's the status of that patch?  It looks like it has a
> lot of a-b's/r-b's but hasn't landed yet.  Is there anything blocking
> it?  If you're still driving it forward, I can wait for that to land and
> then build on top of it.

There was a discussion about it. I will resubmit it and split it up for the non-trivial cases, so those can be reverted if needed.

Will send a new version in a bit.

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

Re: [PATCH v6 1/3] drm: Add CRTC background color property (v5)

[Maarten Lankhorst]

Op 26-02-2019 om 17:17 schreef Matt Roper:
> On Tue, Feb 26, 2019 at 08:26:36AM +0100, Maarten Lankhorst wrote:
>> Hey,
>>
>> Op 21-02-2019 om 01:28 schreef Matt Roper:
>>> Some display controllers can be programmed to present non-black colors
>>> for pixels not covered by any plane (or pixels covered by the
>>> transparent regions of higher planes).  Compositors that want a UI with
>>> a solid color background can potentially save memory bandwidth by
>>> setting the CRTC background property and using smaller planes to display
>>> the rest of the content.
>>>
>>> To avoid confusion between different ways of encoding RGB data, we
>>> define a standard 64-bit format that should be used for this property's
>>> value.  Helper functions and macros are provided to generate and dissect
>>> values in this standard format with varying component precision values.
>>>
>>> v2:
>>>  - Swap internal representation's blue and red bits to make it easier
>>>    to read if printed out.  (Ville)
>>>  - Document bgcolor property in drm_blend.c.  (Sean Paul)
>>>  - s/background_color/bgcolor/ for consistency between property name and
>>>    value storage field.  (Sean Paul)
>>>  - Add a convenience function to attach property to a given crtc.
>>>
>>> v3:
>>>  - Restructure ARGB component extraction macros to be easier to
>>>    understand and enclose the parameters in () to avoid calculations
>>>    if expressions are passed.  (Sean Paul)
>>>  - s/rgba/argb/ in helper function/macro names.  Even though the idea is
>>>    to not worry about the internal representation of the u64, it can
>>>    still be confusing to look at code that uses 'rgba' terminology, but
>>>    stores values with argb ordering.  (Ville)
>>>
>>> v4:
>>>  - Drop the bgcolor_changed flag.  (Ville, Brian Starkey)
>>>  - Clarify in kerneldoc that background color is expected to undergo the
>>>    same pipe-level degamma/csc/gamma transformations that planes do.
>>>    (Brian Starkey)
>>>  - Update kerneldoc to indicate non-opaque colors are allowed, but are
>>>    generally only useful in special cases such as when writeback
>>>    connectors are used (Brian Starkey / Eric Anholt)
>>>
>>> v5:
>>>  - Set crtc->state->bgcolor to solid black inside
>>>    drm_crtc_add_bgcolor_property() in case drivers don't do this
>>>    themselves.  (Ville)
>>>  - Add kerneldoc to drm_crtc_add_bgcolor_property() function.
>>>
>>> Cc: dri-devel@lists.freedesktop.org
>>> Cc: wei.c.li@intel.com
>>> Cc: harish.krupo.kps@intel.com
>>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>> Cc: Sean Paul <sean@poorly.run>
>>> Cc: Brian Starkey <Brian.Starkey@arm.com>
>>> Cc: Eric Anholt <eric@anholt.net>
>>> Cc: Stéphane Marchesin <marcheu@chromium.org>
>>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>>> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
>>> Reviewed-by(v2): Sean Paul <sean@poorly.run>
>>> Reviewed-by: Brian Starkey <brian.starkey@arm.com>
>> I like how bgcolor is a u64 now, but there is an issue with setting
>> crtc->state->bgcolor in attaching the property.
>>
>> We should do it in atomic core init instead, like we already do for
>> connector/plane properties..
>>
>> See for example https://patchwork.freedesktop.org/series/52363/
>>
>> This was specificallly for the background color proposal, but we
>> probalby have to split out the non-trivial conversions of
>> __drm_atomic_helper_crtc_reset.
> Makes sense.  What's the status of that patch?  It looks like it has a
> lot of a-b's/r-b's but hasn't landed yet.  Is there anything blocking
> it?  If you're still driving it forward, I can wait for that to land and
> then build on top of it.
>
>
> Matt
>
I've split out the patch and landed some of the preparation patches, it should be good enough to unblock this patch series at least now. :)

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

Re: [PATCH v6 1/3] drm: Add CRTC background color property (v5)

[Matt Roper]

On Thu, Apr 25, 2019 at 12:45:33PM +0200, Maarten Lankhorst wrote:
> Op 26-02-2019 om 17:17 schreef Matt Roper:
> > On Tue, Feb 26, 2019 at 08:26:36AM +0100, Maarten Lankhorst wrote:
> >> Hey,
> >>
> >> Op 21-02-2019 om 01:28 schreef Matt Roper:
> >>> Some display controllers can be programmed to present non-black colors
> >>> for pixels not covered by any plane (or pixels covered by the
> >>> transparent regions of higher planes).  Compositors that want a UI with
> >>> a solid color background can potentially save memory bandwidth by
> >>> setting the CRTC background property and using smaller planes to display
> >>> the rest of the content.
> >>>
> >>> To avoid confusion between different ways of encoding RGB data, we
> >>> define a standard 64-bit format that should be used for this property's
> >>> value.  Helper functions and macros are provided to generate and dissect
> >>> values in this standard format with varying component precision values.
> >>>
> >>> v2:
> >>>  - Swap internal representation's blue and red bits to make it easier
> >>>    to read if printed out.  (Ville)
> >>>  - Document bgcolor property in drm_blend.c.  (Sean Paul)
> >>>  - s/background_color/bgcolor/ for consistency between property name and
> >>>    value storage field.  (Sean Paul)
> >>>  - Add a convenience function to attach property to a given crtc.
> >>>
> >>> v3:
> >>>  - Restructure ARGB component extraction macros to be easier to
> >>>    understand and enclose the parameters in () to avoid calculations
> >>>    if expressions are passed.  (Sean Paul)
> >>>  - s/rgba/argb/ in helper function/macro names.  Even though the idea is
> >>>    to not worry about the internal representation of the u64, it can
> >>>    still be confusing to look at code that uses 'rgba' terminology, but
> >>>    stores values with argb ordering.  (Ville)
> >>>
> >>> v4:
> >>>  - Drop the bgcolor_changed flag.  (Ville, Brian Starkey)
> >>>  - Clarify in kerneldoc that background color is expected to undergo the
> >>>    same pipe-level degamma/csc/gamma transformations that planes do.
> >>>    (Brian Starkey)
> >>>  - Update kerneldoc to indicate non-opaque colors are allowed, but are
> >>>    generally only useful in special cases such as when writeback
> >>>    connectors are used (Brian Starkey / Eric Anholt)
> >>>
> >>> v5:
> >>>  - Set crtc->state->bgcolor to solid black inside
> >>>    drm_crtc_add_bgcolor_property() in case drivers don't do this
> >>>    themselves.  (Ville)
> >>>  - Add kerneldoc to drm_crtc_add_bgcolor_property() function.
> >>>
> >>> Cc: dri-devel@lists.freedesktop.org
> >>> Cc: wei.c.li@intel.com
> >>> Cc: harish.krupo.kps@intel.com
> >>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>> Cc: Sean Paul <sean@poorly.run>
> >>> Cc: Brian Starkey <Brian.Starkey@arm.com>
> >>> Cc: Eric Anholt <eric@anholt.net>
> >>> Cc: Stéphane Marchesin <marcheu@chromium.org>
> >>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> >>> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> >>> Reviewed-by(v2): Sean Paul <sean@poorly.run>
> >>> Reviewed-by: Brian Starkey <brian.starkey@arm.com>
> >> I like how bgcolor is a u64 now, but there is an issue with setting
> >> crtc->state->bgcolor in attaching the property.
> >>
> >> We should do it in atomic core init instead, like we already do for
> >> connector/plane properties..
> >>
> >> See for example https://patchwork.freedesktop.org/series/52363/
> >>
> >> This was specificallly for the background color proposal, but we
> >> probalby have to split out the non-trivial conversions of
> >> __drm_atomic_helper_crtc_reset.
> > Makes sense.  What's the status of that patch?  It looks like it has a
> > lot of a-b's/r-b's but hasn't landed yet.  Is there anything blocking
> > it?  If you're still driving it forward, I can wait for that to land and
> > then build on top of it.
> >
> >
> > Matt
> >
> I've split out the patch and landed some of the preparation patches, it should be good enough to unblock this patch series at least now. :)
> 

Great, thanks!  I've been traveling a bunch the last month and haven't
had time to revisit this yet, but I'll come back and rebase / repost the
series in the next week or so once I'm caught up again.


Matt

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

[PATCH v6 2/3] drm/i915/gen9+: Add support for pipe background color (v6)

[Matt Roper]

Gen9+ platforms allow CRTC's to be programmed with a background/canvas
color below the programmable planes.  Let's expose this for use by
compositors.

v2:
 - Split out bgcolor sanitization and programming of csc/gamma bits to a
   separate patch that we can land before the ABI changes are ready to
   go in.  (Ville)
 - Change a temporary variable name to be more consistent with
   other similar functions.  (Ville)
 - Change register name to SKL_CANVAS for consistency with the
   CHV_CANVAS register.

v3:
 - Switch register name back to SKL_BOTTOM_COLOR.  (Ville)
 - Use non-_FW register write.  (Ville)
 - Minor parameter rename for consistency.  (Ville)

v4:
 - Removed use of bgcolor_changed flag.

v5:
 - s/uint64_t/u64/

v6:
 - Rebase onto latest drm-tip (bgcolor writing now moves to the new
   color_commit function added by Ville's series)

Cc: dri-devel@lists.freedesktop.org
Cc: wei.c.li@intel.com
Cc: harish.krupo.kps@intel.com
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c  |  9 +++++++++
 drivers/gpu/drm/i915/intel_color.c   | 11 ++++++++---
 drivers/gpu/drm/i915/intel_display.c | 11 +++++++++++
 3 files changed, 28 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 2aeea977283f..0d9d951512af 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -3060,6 +3060,15 @@ static int i915_display_info(struct seq_file *m, void *unused)
 			intel_plane_info(m, crtc);
 		}
 
+		if (INTEL_GEN(dev_priv) >= 9 && pipe_config->base.active) {
+			u64 background = pipe_config->base.bgcolor;
+
+			seq_printf(m, "\tbackground color (10bpc): r=%x g=%x b=%x\n",
+				   DRM_ARGB_RED(background, 10),
+				   DRM_ARGB_GREEN(background, 10),
+				   DRM_ARGB_BLUE(background, 10));
+		}
+
 		seq_printf(m, "\tunderrun reporting: cpu=%s pch=%s \n",
 			   yesno(!crtc->cpu_fifo_underrun_disabled),
 			   yesno(!crtc->pch_fifo_underrun_disabled));
diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
index da7a07d5ccea..1e329a3ee6bd 100644
--- a/drivers/gpu/drm/i915/intel_color.c
+++ b/drivers/gpu/drm/i915/intel_color.c
@@ -424,12 +424,17 @@ static void skl_color_commit(const struct intel_crtc_state *crtc_state)
 	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
 	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
 	enum pipe pipe = crtc->pipe;
+	u64 propval = crtc_state->base.bgcolor;
 	u32 val = 0;
 
+	/* Hardware is programmed with 10 bits of precision */
+	val = DRM_ARGB_RED(propval, 10) << 20
+	    | DRM_ARGB_GREEN(propval, 10) << 10
+	    | DRM_ARGB_BLUE(propval, 10);
+
 	/*
-	 * We don't (yet) allow userspace to control the pipe background color,
-	 * so force it to black, but apply pipe gamma and CSC appropriately
-	 * so that its handling will match how we program our planes.
+	 * Apply pipe gamma and CSC appropriately so that its handling will
+	 * match how we program our planes.
 	 */
 	if (crtc_state->gamma_enable)
 		val |= SKL_BOTTOM_COLOR_GAMMA_ENABLE;
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index afa21daaae51..49e99d73ef89 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11220,6 +11220,8 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc,
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	struct intel_crtc_state *pipe_config =
 		to_intel_crtc_state(crtc_state);
+	struct drm_crtc_state *old_crtc_state =
+		drm_atomic_get_old_crtc_state(crtc_state->state, crtc);
 	int ret;
 	bool mode_changed = needs_modeset(crtc_state);
 
@@ -11242,6 +11244,9 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc,
 			return ret;
 	}
 
+	if (crtc_state->bgcolor != old_crtc_state->bgcolor)
+		pipe_config->update_pipe = true;
+
 	ret = 0;
 	if (dev_priv->display.compute_pipe_wm) {
 		ret = dev_priv->display.compute_pipe_wm(pipe_config);
@@ -14423,6 +14428,9 @@ static int intel_crtc_init(struct drm_i915_private *dev_priv, enum pipe pipe)
 
 	WARN_ON(drm_crtc_index(&intel_crtc->base) != intel_crtc->pipe);
 
+	if (INTEL_GEN(dev_priv) >= 9)
+		drm_crtc_add_bgcolor_property(&intel_crtc->base);
+
 	return 0;
 
 fail:
@@ -15665,6 +15673,9 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc,
 	struct intel_crtc_state *crtc_state = to_intel_crtc_state(crtc->base.state);
 	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
 
+	/* Always force bgcolor to solid black */
+	crtc_state->base.bgcolor = drm_argb(16, 0xFFFF, 0, 0, 0);
+
 	/* Clear any frame start delays used for debugging left by the BIOS */
 	if (crtc->active && !transcoder_is_dsi(cpu_transcoder)) {
 		i915_reg_t reg = PIPECONF(cpu_transcoder);
-- 
2.14.5

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

[PATCH v6 3/3] drm/i915: Add background color hardware readout and state check

[Matt Roper]

We should support readout and verification of crtc background color as
we do with other pipe state.  Note that our hardware holds less bits of
precision than the CRTC state allows, so we need to take care to only
verify the most significant bits of the color after performing readout.

At boot time the pipe color is already sanitized to full black as
required by ABI, so the new readout here won't break that requirement.

Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 49e99d73ef89..2eba05e2fda6 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -9869,6 +9869,7 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
 	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
 	enum intel_display_power_domain power_domain;
 	u64 power_domain_mask;
+	u32 bgcolor;
 	bool active;
 
 	intel_crtc_init_scalers(crtc, pipe_config);
@@ -9947,6 +9948,15 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
 		pipe_config->pixel_multiplier = 1;
 	}
 
+	if (INTEL_GEN(dev_priv) >= 9) {
+		bgcolor = I915_READ(SKL_BOTTOM_COLOR(crtc->pipe));
+		pipe_config->base.bgcolor =
+			drm_argb(10, 0xFFFF,
+				 bgcolor >> 20 & 0x3FF,
+				 bgcolor >> 10 & 0x3FF,
+				 bgcolor       & 0x3FF);
+	}
+
 out:
 	for_each_power_domain(power_domain, power_domain_mask)
 		intel_display_power_put_unchecked(dev_priv, power_domain);
@@ -11580,6 +11590,10 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc,
 				      drm_rect_width(&state->base.dst),
 				      drm_rect_height(&state->base.dst));
 	}
+
+	if (INTEL_GEN(dev_priv) >= 9)
+		DRM_DEBUG_KMS("background color: %llx\n",
+			      pipe_config->base.bgcolor);
 }
 
 static bool check_digital_port_conflicts(struct drm_atomic_state *state)
@@ -11946,6 +11960,16 @@ intel_pipe_config_compare(struct drm_i915_private *dev_priv,
 	} \
 } while (0)
 
+#define PIPE_CONF_CHECK_LLX_MASKED(name, mask) do { \
+	if ((current_config->name & mask) != (pipe_config->name & mask)) { \
+		pipe_config_err(adjust, __stringify(name), \
+			  "(expected 0x%016llx, found 0x%016llx)\n", \
+			  current_config->name & mask, \
+			  pipe_config->name & mask); \
+		ret = false; \
+	} \
+} while (0)
+
 #define PIPE_CONF_CHECK_I(name) do { \
 	if (current_config->name != pipe_config->name) { \
 		pipe_config_err(adjust, __stringify(name), \
@@ -12201,6 +12225,14 @@ intel_pipe_config_compare(struct drm_i915_private *dev_priv,
 
 	PIPE_CONF_CHECK_I(min_voltage_level);
 
+	/*
+	 * Hardware only holds top 10 bits of each color component; ignore
+	 * bottom six bits (and all of alpha) when comparing against readout.
+	 */
+	if (INTEL_GEN(dev_priv) >= 9)
+		PIPE_CONF_CHECK_LLX_MASKED(base.bgcolor, 0x0000FFC0FFC0FFC0);
+
+#undef PIPE_CONF_CHECK_LLX_MASKED
 #undef PIPE_CONF_CHECK_X
 #undef PIPE_CONF_CHECK_I
 #undef PIPE_CONF_CHECK_BOOL
-- 
2.14.5

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

[PATCH i-g-t 1/2] lib: Add --skip-crc-compare option

[Matt Roper]

When using --interactive-debug, it's sometimes desirable to ignore CRC
mismatches and let the test proceed as if they passed so that the
on-screen outcome can be inspected.  Let's add a debug option to allow
this.

Cc: igt-dev@lists.freedesktop.org
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 lib/igt_core.c    | 7 +++++++
 lib/igt_core.h    | 1 +
 lib/igt_debugfs.c | 8 +++++++-
 3 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/lib/igt_core.c b/lib/igt_core.c
index 71b05d3b..5523950f 100644
--- a/lib/igt_core.c
+++ b/lib/igt_core.c
@@ -256,6 +256,7 @@
 
 static unsigned int exit_handler_count;
 const char *igt_interactive_debug;
+bool igt_skip_crc_compare;
 
 /* subtests helpers */
 static bool list_subtests = false;
@@ -285,6 +286,7 @@ enum {
  OPT_DESCRIPTION,
  OPT_DEBUG,
  OPT_INTERACTIVE_DEBUG,
+ OPT_SKIP_CRC,
  OPT_HELP = 'h'
 };
 
@@ -552,6 +554,7 @@ static void print_usage(const char *help_str, bool output_on_stderr)
 		   "  --run-subtest <pattern>\n"
 		   "  --debug[=log-domain]\n"
 		   "  --interactive-debug[=domain]\n"
+		   "  --skip-crc-compare\n"
 		   "  --help-description\n"
 		   "  --help\n");
 	if (help_str)
@@ -668,6 +671,7 @@ static int common_init(int *argc, char **argv,
 		{"help-description", 0, 0, OPT_DESCRIPTION},
 		{"debug", optional_argument, 0, OPT_DEBUG},
 		{"interactive-debug", optional_argument, 0, OPT_INTERACTIVE_DEBUG},
+		{"skip-crc-compare", 0, 0, OPT_SKIP_CRC},
 		{"help", 0, 0, OPT_HELP},
 		{0, 0, 0, 0}
 	};
@@ -768,6 +772,9 @@ static int common_init(int *argc, char **argv,
 			print_test_description();
 			ret = -1;
 			goto out;
+		case OPT_SKIP_CRC:
+			igt_skip_crc_compare = true;
+			goto out;
 		case OPT_HELP:
 			print_usage(help_str, false);
 			ret = -1;
diff --git a/lib/igt_core.h b/lib/igt_core.h
index 47ffd9e7..f12fc5cb 100644
--- a/lib/igt_core.h
+++ b/lib/igt_core.h
@@ -843,6 +843,7 @@ bool igt_run_in_simulation(void);
 void igt_skip_on_simulation(void);
 
 extern const char *igt_interactive_debug;
+extern bool igt_skip_crc_compare;
 
 /**
  * igt_log_level:
diff --git a/lib/igt_debugfs.c b/lib/igt_debugfs.c
index 640c78e9..04d1648b 100644
--- a/lib/igt_debugfs.c
+++ b/lib/igt_debugfs.c
@@ -405,6 +405,12 @@ static bool igt_find_crc_mismatch(const igt_crc_t *a, const igt_crc_t *b,
  * assert that CRCs match, never that they are different. Otherwise there might
  * be random testcase failures when different screen contents end up with the
  * same CRC by chance.
+ *
+ * Passing --skip-crc-compare on the command line will force this function
+ * to always pass, which can be useful in interactive debugging where you
+ * might know the test will fail, but still want the test to keep going as if
+ * it had succeeded so that you can see the on-screen behavior.
+ *
  */
 void igt_assert_crc_equal(const igt_crc_t *a, const igt_crc_t *b)
 {
@@ -416,7 +422,7 @@ void igt_assert_crc_equal(const igt_crc_t *a, const igt_crc_t *b)
 		igt_debug("CRC mismatch at index %d: 0x%x != 0x%x\n", index,
 			  a->crc[index], b->crc[index]);
 
-	igt_assert(!mismatch);
+	igt_assert(!mismatch || igt_skip_crc_compare);
 }
 
 /**
-- 
2.14.5

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

[igt-dev] [PATCH i-g-t 1/2] lib: Add --skip-crc-compare option

[Matt Roper]

When using --interactive-debug, it's sometimes desirable to ignore CRC
mismatches and let the test proceed as if they passed so that the
on-screen outcome can be inspected.  Let's add a debug option to allow
this.

Cc: igt-dev@lists.freedesktop.org
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 lib/igt_core.c    | 7 +++++++
 lib/igt_core.h    | 1 +
 lib/igt_debugfs.c | 8 +++++++-
 3 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/lib/igt_core.c b/lib/igt_core.c
index 71b05d3b..5523950f 100644
--- a/lib/igt_core.c
+++ b/lib/igt_core.c
@@ -256,6 +256,7 @@
 
 static unsigned int exit_handler_count;
 const char *igt_interactive_debug;
+bool igt_skip_crc_compare;
 
 /* subtests helpers */
 static bool list_subtests = false;
@@ -285,6 +286,7 @@ enum {
  OPT_DESCRIPTION,
  OPT_DEBUG,
  OPT_INTERACTIVE_DEBUG,
+ OPT_SKIP_CRC,
  OPT_HELP = 'h'
 };
 
@@ -552,6 +554,7 @@ static void print_usage(const char *help_str, bool output_on_stderr)
 		   "  --run-subtest <pattern>\n"
 		   "  --debug[=log-domain]\n"
 		   "  --interactive-debug[=domain]\n"
+		   "  --skip-crc-compare\n"
 		   "  --help-description\n"
 		   "  --help\n");
 	if (help_str)
@@ -668,6 +671,7 @@ static int common_init(int *argc, char **argv,
 		{"help-description", 0, 0, OPT_DESCRIPTION},
 		{"debug", optional_argument, 0, OPT_DEBUG},
 		{"interactive-debug", optional_argument, 0, OPT_INTERACTIVE_DEBUG},
+		{"skip-crc-compare", 0, 0, OPT_SKIP_CRC},
 		{"help", 0, 0, OPT_HELP},
 		{0, 0, 0, 0}
 	};
@@ -768,6 +772,9 @@ static int common_init(int *argc, char **argv,
 			print_test_description();
 			ret = -1;
 			goto out;
+		case OPT_SKIP_CRC:
+			igt_skip_crc_compare = true;
+			goto out;
 		case OPT_HELP:
 			print_usage(help_str, false);
 			ret = -1;
diff --git a/lib/igt_core.h b/lib/igt_core.h
index 47ffd9e7..f12fc5cb 100644
--- a/lib/igt_core.h
+++ b/lib/igt_core.h
@@ -843,6 +843,7 @@ bool igt_run_in_simulation(void);
 void igt_skip_on_simulation(void);
 
 extern const char *igt_interactive_debug;
+extern bool igt_skip_crc_compare;
 
 /**
  * igt_log_level:
diff --git a/lib/igt_debugfs.c b/lib/igt_debugfs.c
index 640c78e9..04d1648b 100644
--- a/lib/igt_debugfs.c
+++ b/lib/igt_debugfs.c
@@ -405,6 +405,12 @@ static bool igt_find_crc_mismatch(const igt_crc_t *a, const igt_crc_t *b,
  * assert that CRCs match, never that they are different. Otherwise there might
  * be random testcase failures when different screen contents end up with the
  * same CRC by chance.
+ *
+ * Passing --skip-crc-compare on the command line will force this function
+ * to always pass, which can be useful in interactive debugging where you
+ * might know the test will fail, but still want the test to keep going as if
+ * it had succeeded so that you can see the on-screen behavior.
+ *
  */
 void igt_assert_crc_equal(const igt_crc_t *a, const igt_crc_t *b)
 {
@@ -416,7 +422,7 @@ void igt_assert_crc_equal(const igt_crc_t *a, const igt_crc_t *b)
 		igt_debug("CRC mismatch at index %d: 0x%x != 0x%x\n", index,
 			  a->crc[index], b->crc[index]);
 
-	igt_assert(!mismatch);
+	igt_assert(!mismatch || igt_skip_crc_compare);
 }
 
 /**
-- 
2.14.5

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[PATCH i-g-t 2/2] tests/kms_crtc_background_color: overhaul to match upstream ABI (v5)

[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 finally have an open source userspace
(ChromeOS), so lets update the IGT test to match the ABI that's actually
going upstream 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, at least on Intel gen9.  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.

v3:
 - Minor updates to proposed uapi helpers (s/rgba/argb/).

v4:
 - General restructuring into pipe/color subtests.
 - Use RGB2101010 framebuffers for comparison so that we match the bits
   of precision that Intel hardware background color accepts

v5:
 - Re-enable CRC comparisons; just because these don't work on Intel
   doesn't mean we shouldn't use them on other vendors' platforms
   (Daniel).
 - Use DRIVER_ANY rather than DRIVER_INTEL. (Heiko Stuebner)

Cc: igt-dev@lists.freedesktop.org
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Heiko Stuebner <heiko@sntech.de>
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 lib/igt_kms.c                     |   2 +-
 tests/kms_crtc_background_color.c | 263 ++++++++++++++++++--------------------
 2 files changed, 127 insertions(+), 138 deletions(-)

diff --git a/lib/igt_kms.c b/lib/igt_kms.c
index 080f90ae..c52ec5e6 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..58cdd7a9 100644
--- a/tests/kms_crtc_background_color.c
+++ b/tests/kms_crtc_background_color.c
@@ -25,164 +25,153 @@
 #include "igt.h"
 #include <math.h>
 
-
 IGT_TEST_DESCRIPTION("Test crtc background color feature");
 
+/*
+ * Paints a desired color into a full-screen primary plane and then compares
+ * that CRC with turning off all planes and setting the CRTC background to the
+ * same color.
+ *
+ * At least on current Intel platforms, the CRC tests appear to always fail,
+ * even though the resulting pipe output seems to be the same.  The bspec tells
+ * us that we must have at least one plane turned on when taking a pipe-level
+ * CRC, so the fact that we're violating that may explain the failures.  If
+ * your platform gives failures for all tests, you may want to run with
+ * --interactive-debug=bgcolor --skip-crc-compare to visually inspect that the
+ * background color matches the equivalent solid plane.
+ */
+
 typedef struct {
-	int gfx_fd;
 	igt_display_t display;
-	struct igt_fb fb;
-	igt_crc_t ref_crc;
+	int gfx_fd;
+	igt_output_t *output;
 	igt_pipe_crc_t *pipe_crc;
+	drmModeModeInfo *mode;
 } data_t;
 
-#define BLACK      0x000000           /* BGR 8bpc */
-#define CYAN       0xFFFF00           /* BGR 8bpc */
-#define PURPLE     0xFF00FF           /* BGR 8bpc */
-#define WHITE      0xFFFFFF           /* BGR 8bpc */
-
-#define BLACK64    0x000000000000     /* BGR 16bpc */
-#define CYAN64     0xFFFFFFFF0000     /* BGR 16bpc */
-#define PURPLE64   0xFFFF0000FFFF     /* BGR 16bpc */
-#define YELLOW64   0x0000FFFFFFFF     /* BGR 16bpc */
-#define WHITE64    0xFFFFFFFFFFFF     /* BGR 16bpc */
-#define RED64      0x00000000FFFF     /* BGR 16bpc */
-#define GREEN64    0x0000FFFF0000     /* BGR 16bpc */
-#define BLUE64     0xFFFF00000000     /* BGR 16bpc */
-
-static void
-paint_background(data_t *data, struct igt_fb *fb, drmModeModeInfo *mode,
-		uint32_t background, double alpha)
+/*
+ * Local copy of kernel uapi
+ */
+static inline __u64
+local_argb(__u8 bpc, __u16 alpha, __u16 red, __u16 green, __u16 blue)
 {
-	cairo_t *cr;
-	int w, h;
-	double r, g, b;
-
-	w = mode->hdisplay;
-	h = mode->vdisplay;
-
-	cr = igt_get_cairo_ctx(data->gfx_fd, &data->fb);
+       int msb_shift = 16 - bpc;
 
-	/* Paint with background color */
-	r = (double) (background & 0xFF) / 255.0;
-	g = (double) ((background & 0xFF00) >> 8) / 255.0;
-	b = (double) ((background & 0xFF0000) >> 16) / 255.0;
-	igt_paint_color_alpha(cr, 0, 0, w, h, r, g, b, alpha);
-
-	igt_put_cairo_ctx(data->gfx_fd, &data->fb, cr);
+       return (__u64)alpha << msb_shift << 48 |
+              (__u64)red   << msb_shift << 32 |
+              (__u64)green << msb_shift << 16 |
+              (__u64)blue  << msb_shift;
 }
 
-static void prepare_crtc(data_t *data, igt_output_t *output, enum pipe pipe,
-			igt_plane_t *plane, int opaque_buffer, int plane_color,
-			uint64_t pipe_background_color)
+static void test_background(data_t *data, enum pipe pipe, int w, int h,
+			    __u64 r, __u64 g, __u64 b)
 {
-	drmModeModeInfo *mode;
-	igt_display_t *display = &data->display;
-	int fb_id;
-	double alpha;
-
-	igt_output_set_pipe(output, pipe);
-
-	/* create the pipe_crc object for this pipe */
-	igt_pipe_crc_free(data->pipe_crc);
-	data->pipe_crc = igt_pipe_crc_new(data->gfx_fd, pipe, INTEL_PIPE_CRC_SOURCE_AUTO);
-
-	mode = igt_output_get_mode(output);
-
-	fb_id = igt_create_fb(data->gfx_fd,
-			mode->hdisplay, mode->vdisplay,
-			DRM_FORMAT_XRGB8888,
-			LOCAL_DRM_FORMAT_MOD_NONE, /* tiled */
-			&data->fb);
-	igt_assert(fb_id);
-
-	/* To make FB pixel win with background color, set alpha as full opaque */
-	igt_pipe_set_prop_value(display, pipe, IGT_CRTC_BACKGROUND, pipe_background_color);
-	if (opaque_buffer)
-		alpha = 1.0;    /* alpha 1 is fully opque */
-	else
-		alpha = 0.0;    /* alpha 0 is fully transparent */
-	paint_background(data, &data->fb, mode, plane_color, alpha);
-
-	igt_plane_set_fb(plane, &data->fb);
-	igt_display_commit2(display, COMMIT_UNIVERSAL);
-}
-
-static void cleanup_crtc(data_t *data, igt_output_t *output, igt_plane_t *plane)
-{
-	igt_display_t *display = &data->display;
-
-	igt_pipe_crc_free(data->pipe_crc);
-	data->pipe_crc = NULL;
-
-	igt_remove_fb(data->gfx_fd, &data->fb);
-
-	igt_pipe_obj_set_prop_value(plane->pipe, IGT_CRTC_BACKGROUND, BLACK64);
+	igt_crc_t plane_crc, bg_crc;
+	struct igt_fb colorfb;
+	igt_plane_t *plane = igt_output_get_plane_type(data->output,
+						       DRM_PLANE_TYPE_PRIMARY);
+
+
+	/* Fill the primary plane and set the background to the same color */
+	igt_create_color_fb(data->gfx_fd, w, h, DRM_FORMAT_XRGB2101010,
+			    LOCAL_DRM_FORMAT_MOD_NONE,
+			    (double)r / 0xFFFF,
+			    (double)g / 0xFFFF,
+			    (double)b / 0xFFFF,
+			    &colorfb);
+	igt_plane_set_fb(plane, &colorfb);
+	igt_pipe_set_prop_value(&data->display, pipe, IGT_CRTC_BACKGROUND,
+				local_argb(16, 0xffff, r, g, b));
+	igt_display_commit2(&data->display, COMMIT_ATOMIC);
+	igt_pipe_crc_collect_crc(data->pipe_crc, &plane_crc);
+	igt_debug_wait_for_keypress("bgcolor");
+
+	/* Turn off the primary plane so only bg shows */
 	igt_plane_set_fb(plane, NULL);
-	igt_output_set_pipe(output, PIPE_ANY);
-
-	igt_display_commit2(display, COMMIT_UNIVERSAL);
+	igt_display_commit2(&data->display, COMMIT_ATOMIC);
+	igt_pipe_crc_collect_crc(data->pipe_crc, &bg_crc);
+	igt_debug_wait_for_keypress("bgcolor");
+
+	/*
+	 * The following test relies on hardware that generates valid CRCs
+	 * even when no planes are on.  Sadly, this doesn't appear to be the
+	 * case for current Intel platforms; pipe CRC's never match bgcolor
+	 * CRC's, likely because we're violating the bspec's guidance that there
+	 * must always be at least one real plane turned on when taking the
+	 * pipe-level ("dmux") CRC.
+	 */
+	igt_assert_crc_equal(&plane_crc, &bg_crc);
 }
 
-static void test_crtc_background(data_t *data)
+igt_main
 {
-	igt_display_t *display = &data->display;
+	data_t data = {};
 	igt_output_t *output;
+	drmModeModeInfo *mode;
+	int w, h;
 	enum pipe pipe;
-	int valid_tests = 0;
-
-	for_each_pipe_with_valid_output(display, pipe, output) {
-		igt_plane_t *plane;
-
-		igt_output_set_pipe(output, pipe);
-
-		plane = igt_output_get_plane_type(output, DRM_PLANE_TYPE_PRIMARY);
-		igt_require(igt_pipe_has_prop(display, pipe, IGT_CRTC_BACKGROUND));
-
-		prepare_crtc(data, output, pipe, plane, 1, PURPLE, BLACK64);
-
-		/* Now set background without using a plane, i.e.,
-		 * Disable the plane to let hw background color win blend. */
-		igt_plane_set_fb(plane, NULL);
-		igt_pipe_set_prop_value(display, pipe, IGT_CRTC_BACKGROUND, PURPLE64);
-		igt_display_commit2(display, COMMIT_UNIVERSAL);
-
-		/* Try few other background colors */
-		igt_pipe_set_prop_value(display, pipe, IGT_CRTC_BACKGROUND, CYAN64);
-		igt_display_commit2(display, COMMIT_UNIVERSAL);
-
-		igt_pipe_set_prop_value(display, pipe, IGT_CRTC_BACKGROUND, YELLOW64);
-		igt_display_commit2(display, COMMIT_UNIVERSAL);
 
-		igt_pipe_set_prop_value(display, pipe, IGT_CRTC_BACKGROUND, RED64);
-		igt_display_commit2(display, COMMIT_UNIVERSAL);
+	igt_fixture {
+		data.gfx_fd = drm_open_driver_master(DRIVER_ANY);
+		kmstest_set_vt_graphics_mode();
 
-		igt_pipe_set_prop_value(display, pipe, IGT_CRTC_BACKGROUND, GREEN64);
-		igt_display_commit2(display, COMMIT_UNIVERSAL);
-
-		igt_pipe_set_prop_value(display, pipe, IGT_CRTC_BACKGROUND, BLUE64);
-		igt_display_commit2(display, COMMIT_UNIVERSAL);
-
-		igt_pipe_set_prop_value(display, pipe, IGT_CRTC_BACKGROUND, WHITE64);
-		igt_display_commit2(display, COMMIT_UNIVERSAL);
-
-		valid_tests++;
-		cleanup_crtc(data, output, plane);
+		igt_require_pipe_crc(data.gfx_fd);
+		igt_display_require(&data.display, data.gfx_fd);
 	}
-	igt_require_f(valid_tests, "no valid crtc/connector combinations found\n");
-}
 
-igt_simple_main
-{
-	data_t data = {};
-
-	igt_skip_on_simulation();
-
-	data.gfx_fd = drm_open_driver(DRIVER_INTEL);
-	igt_require_pipe_crc(data.gfx_fd);
-	igt_display_require(&data.display, data.gfx_fd);
-
-	test_crtc_background(&data);
+	for_each_pipe_static(pipe) igt_subtest_group {
+		igt_fixture {
+			igt_display_require_output_on_pipe(&data.display, pipe);
+			igt_require(igt_pipe_has_prop(&data.display, pipe,
+						      IGT_CRTC_BACKGROUND));
+
+			output = igt_get_single_output_for_pipe(&data.display,
+								pipe);
+			igt_output_set_pipe(output, pipe);
+			data.output = output;
+
+			mode = igt_output_get_mode(output);
+			w = mode->hdisplay;
+			h = mode->vdisplay;
+
+			data.pipe_crc = igt_pipe_crc_new(data.gfx_fd, pipe,
+							  INTEL_PIPE_CRC_SOURCE_AUTO);
+		}
+
+		igt_subtest_f("background-color-pipe-%s-black",
+			      kmstest_pipe_name(pipe))
+			test_background(&data, pipe, w, h,
+					0, 0, 0);
+
+		igt_subtest_f("background-color-pipe-%s-white",
+			      kmstest_pipe_name(pipe))
+			test_background(&data, pipe, w, h,
+					0xFFFF, 0xFFFF, 0xFFFF);
+
+		igt_subtest_f("background-color-pipe-%s-red",
+			      kmstest_pipe_name(pipe))
+			test_background(&data, pipe, w, h,
+					0xFFFF, 0, 0);
+
+		igt_subtest_f("background-color-pipe-%s-green",
+			      kmstest_pipe_name(pipe))
+
+			test_background(&data, pipe, w, h,
+					0, 0xFFFF, 0);
+
+		igt_subtest_f("background-color-pipe-%s-blue",
+			      kmstest_pipe_name(pipe))
+			test_background(&data, pipe, w, h,
+					0, 0, 0xFFFF);
+
+		igt_fixture {
+			igt_display_reset(&data.display);
+			igt_pipe_crc_free(data.pipe_crc);
+			data.pipe_crc = NULL;
+		}
+	}
 
-	igt_display_fini(&data.display);
+	igt_fixture {
+		igt_display_fini(&data.display);
+	}
 }
-- 
2.14.5

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

[igt-dev] [PATCH i-g-t 2/2] tests/kms_crtc_background_color: overhaul to match upstream ABI (v5)

[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 finally have an open source userspace
(ChromeOS), so lets update the IGT test to match the ABI that's actually
going upstream 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, at least on Intel gen9.  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.

v3:
 - Minor updates to proposed uapi helpers (s/rgba/argb/).

v4:
 - General restructuring into pipe/color subtests.
 - Use RGB2101010 framebuffers for comparison so that we match the bits
   of precision that Intel hardware background color accepts

v5:
 - Re-enable CRC comparisons; just because these don't work on Intel
   doesn't mean we shouldn't use them on other vendors' platforms
   (Daniel).
 - Use DRIVER_ANY rather than DRIVER_INTEL. (Heiko Stuebner)

Cc: igt-dev@lists.freedesktop.org
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Heiko Stuebner <heiko@sntech.de>
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 lib/igt_kms.c                     |   2 +-
 tests/kms_crtc_background_color.c | 263 ++++++++++++++++++--------------------
 2 files changed, 127 insertions(+), 138 deletions(-)

diff --git a/lib/igt_kms.c b/lib/igt_kms.c
index 080f90ae..c52ec5e6 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..58cdd7a9 100644
--- a/tests/kms_crtc_background_color.c
+++ b/tests/kms_crtc_background_color.c
@@ -25,164 +25,153 @@
 #include "igt.h"
 #include <math.h>
 
-
 IGT_TEST_DESCRIPTION("Test crtc background color feature");
 
+/*
+ * Paints a desired color into a full-screen primary plane and then compares
+ * that CRC with turning off all planes and setting the CRTC background to the
+ * same color.
+ *
+ * At least on current Intel platforms, the CRC tests appear to always fail,
+ * even though the resulting pipe output seems to be the same.  The bspec tells
+ * us that we must have at least one plane turned on when taking a pipe-level
+ * CRC, so the fact that we're violating that may explain the failures.  If
+ * your platform gives failures for all tests, you may want to run with
+ * --interactive-debug=bgcolor --skip-crc-compare to visually inspect that the
+ * background color matches the equivalent solid plane.
+ */
+
 typedef struct {
-	int gfx_fd;
 	igt_display_t display;
-	struct igt_fb fb;
-	igt_crc_t ref_crc;
+	int gfx_fd;
+	igt_output_t *output;
 	igt_pipe_crc_t *pipe_crc;
+	drmModeModeInfo *mode;
 } data_t;
 
-#define BLACK      0x000000           /* BGR 8bpc */
-#define CYAN       0xFFFF00           /* BGR 8bpc */
-#define PURPLE     0xFF00FF           /* BGR 8bpc */
-#define WHITE      0xFFFFFF           /* BGR 8bpc */
-
-#define BLACK64    0x000000000000     /* BGR 16bpc */
-#define CYAN64     0xFFFFFFFF0000     /* BGR 16bpc */
-#define PURPLE64   0xFFFF0000FFFF     /* BGR 16bpc */
-#define YELLOW64   0x0000FFFFFFFF     /* BGR 16bpc */
-#define WHITE64    0xFFFFFFFFFFFF     /* BGR 16bpc */
-#define RED64      0x00000000FFFF     /* BGR 16bpc */
-#define GREEN64    0x0000FFFF0000     /* BGR 16bpc */
-#define BLUE64     0xFFFF00000000     /* BGR 16bpc */
-
-static void
-paint_background(data_t *data, struct igt_fb *fb, drmModeModeInfo *mode,
-		uint32_t background, double alpha)
+/*
+ * Local copy of kernel uapi
+ */
+static inline __u64
+local_argb(__u8 bpc, __u16 alpha, __u16 red, __u16 green, __u16 blue)
 {
-	cairo_t *cr;
-	int w, h;
-	double r, g, b;
-
-	w = mode->hdisplay;
-	h = mode->vdisplay;
-
-	cr = igt_get_cairo_ctx(data->gfx_fd, &data->fb);
+       int msb_shift = 16 - bpc;
 
-	/* Paint with background color */
-	r = (double) (background & 0xFF) / 255.0;
-	g = (double) ((background & 0xFF00) >> 8) / 255.0;
-	b = (double) ((background & 0xFF0000) >> 16) / 255.0;
-	igt_paint_color_alpha(cr, 0, 0, w, h, r, g, b, alpha);
-
-	igt_put_cairo_ctx(data->gfx_fd, &data->fb, cr);
+       return (__u64)alpha << msb_shift << 48 |
+              (__u64)red   << msb_shift << 32 |
+              (__u64)green << msb_shift << 16 |
+              (__u64)blue  << msb_shift;
 }
 
-static void prepare_crtc(data_t *data, igt_output_t *output, enum pipe pipe,
-			igt_plane_t *plane, int opaque_buffer, int plane_color,
-			uint64_t pipe_background_color)
+static void test_background(data_t *data, enum pipe pipe, int w, int h,
+			    __u64 r, __u64 g, __u64 b)
 {
-	drmModeModeInfo *mode;
-	igt_display_t *display = &data->display;
-	int fb_id;
-	double alpha;
-
-	igt_output_set_pipe(output, pipe);
-
-	/* create the pipe_crc object for this pipe */
-	igt_pipe_crc_free(data->pipe_crc);
-	data->pipe_crc = igt_pipe_crc_new(data->gfx_fd, pipe, INTEL_PIPE_CRC_SOURCE_AUTO);
-
-	mode = igt_output_get_mode(output);
-
-	fb_id = igt_create_fb(data->gfx_fd,
-			mode->hdisplay, mode->vdisplay,
-			DRM_FORMAT_XRGB8888,
-			LOCAL_DRM_FORMAT_MOD_NONE, /* tiled */
-			&data->fb);
-	igt_assert(fb_id);
-
-	/* To make FB pixel win with background color, set alpha as full opaque */
-	igt_pipe_set_prop_value(display, pipe, IGT_CRTC_BACKGROUND, pipe_background_color);
-	if (opaque_buffer)
-		alpha = 1.0;    /* alpha 1 is fully opque */
-	else
-		alpha = 0.0;    /* alpha 0 is fully transparent */
-	paint_background(data, &data->fb, mode, plane_color, alpha);
-
-	igt_plane_set_fb(plane, &data->fb);
-	igt_display_commit2(display, COMMIT_UNIVERSAL);
-}
-
-static void cleanup_crtc(data_t *data, igt_output_t *output, igt_plane_t *plane)
-{
-	igt_display_t *display = &data->display;
-
-	igt_pipe_crc_free(data->pipe_crc);
-	data->pipe_crc = NULL;
-
-	igt_remove_fb(data->gfx_fd, &data->fb);
-
-	igt_pipe_obj_set_prop_value(plane->pipe, IGT_CRTC_BACKGROUND, BLACK64);
+	igt_crc_t plane_crc, bg_crc;
+	struct igt_fb colorfb;
+	igt_plane_t *plane = igt_output_get_plane_type(data->output,
+						       DRM_PLANE_TYPE_PRIMARY);
+
+
+	/* Fill the primary plane and set the background to the same color */
+	igt_create_color_fb(data->gfx_fd, w, h, DRM_FORMAT_XRGB2101010,
+			    LOCAL_DRM_FORMAT_MOD_NONE,
+			    (double)r / 0xFFFF,
+			    (double)g / 0xFFFF,
+			    (double)b / 0xFFFF,
+			    &colorfb);
+	igt_plane_set_fb(plane, &colorfb);
+	igt_pipe_set_prop_value(&data->display, pipe, IGT_CRTC_BACKGROUND,
+				local_argb(16, 0xffff, r, g, b));
+	igt_display_commit2(&data->display, COMMIT_ATOMIC);
+	igt_pipe_crc_collect_crc(data->pipe_crc, &plane_crc);
+	igt_debug_wait_for_keypress("bgcolor");
+
+	/* Turn off the primary plane so only bg shows */
 	igt_plane_set_fb(plane, NULL);
-	igt_output_set_pipe(output, PIPE_ANY);
-
-	igt_display_commit2(display, COMMIT_UNIVERSAL);
+	igt_display_commit2(&data->display, COMMIT_ATOMIC);
+	igt_pipe_crc_collect_crc(data->pipe_crc, &bg_crc);
+	igt_debug_wait_for_keypress("bgcolor");
+
+	/*
+	 * The following test relies on hardware that generates valid CRCs
+	 * even when no planes are on.  Sadly, this doesn't appear to be the
+	 * case for current Intel platforms; pipe CRC's never match bgcolor
+	 * CRC's, likely because we're violating the bspec's guidance that there
+	 * must always be at least one real plane turned on when taking the
+	 * pipe-level ("dmux") CRC.
+	 */
+	igt_assert_crc_equal(&plane_crc, &bg_crc);
 }
 
-static void test_crtc_background(data_t *data)
+igt_main
 {
-	igt_display_t *display = &data->display;
+	data_t data = {};
 	igt_output_t *output;
+	drmModeModeInfo *mode;
+	int w, h;
 	enum pipe pipe;
-	int valid_tests = 0;
-
-	for_each_pipe_with_valid_output(display, pipe, output) {
-		igt_plane_t *plane;
-
-		igt_output_set_pipe(output, pipe);
-
-		plane = igt_output_get_plane_type(output, DRM_PLANE_TYPE_PRIMARY);
-		igt_require(igt_pipe_has_prop(display, pipe, IGT_CRTC_BACKGROUND));
-
-		prepare_crtc(data, output, pipe, plane, 1, PURPLE, BLACK64);
-
-		/* Now set background without using a plane, i.e.,
-		 * Disable the plane to let hw background color win blend. */
-		igt_plane_set_fb(plane, NULL);
-		igt_pipe_set_prop_value(display, pipe, IGT_CRTC_BACKGROUND, PURPLE64);
-		igt_display_commit2(display, COMMIT_UNIVERSAL);
-
-		/* Try few other background colors */
-		igt_pipe_set_prop_value(display, pipe, IGT_CRTC_BACKGROUND, CYAN64);
-		igt_display_commit2(display, COMMIT_UNIVERSAL);
-
-		igt_pipe_set_prop_value(display, pipe, IGT_CRTC_BACKGROUND, YELLOW64);
-		igt_display_commit2(display, COMMIT_UNIVERSAL);
 
-		igt_pipe_set_prop_value(display, pipe, IGT_CRTC_BACKGROUND, RED64);
-		igt_display_commit2(display, COMMIT_UNIVERSAL);
+	igt_fixture {
+		data.gfx_fd = drm_open_driver_master(DRIVER_ANY);
+		kmstest_set_vt_graphics_mode();
 
-		igt_pipe_set_prop_value(display, pipe, IGT_CRTC_BACKGROUND, GREEN64);
-		igt_display_commit2(display, COMMIT_UNIVERSAL);
-
-		igt_pipe_set_prop_value(display, pipe, IGT_CRTC_BACKGROUND, BLUE64);
-		igt_display_commit2(display, COMMIT_UNIVERSAL);
-
-		igt_pipe_set_prop_value(display, pipe, IGT_CRTC_BACKGROUND, WHITE64);
-		igt_display_commit2(display, COMMIT_UNIVERSAL);
-
-		valid_tests++;
-		cleanup_crtc(data, output, plane);
+		igt_require_pipe_crc(data.gfx_fd);
+		igt_display_require(&data.display, data.gfx_fd);
 	}
-	igt_require_f(valid_tests, "no valid crtc/connector combinations found\n");
-}
 
-igt_simple_main
-{
-	data_t data = {};
-
-	igt_skip_on_simulation();
-
-	data.gfx_fd = drm_open_driver(DRIVER_INTEL);
-	igt_require_pipe_crc(data.gfx_fd);
-	igt_display_require(&data.display, data.gfx_fd);
-
-	test_crtc_background(&data);
+	for_each_pipe_static(pipe) igt_subtest_group {
+		igt_fixture {
+			igt_display_require_output_on_pipe(&data.display, pipe);
+			igt_require(igt_pipe_has_prop(&data.display, pipe,
+						      IGT_CRTC_BACKGROUND));
+
+			output = igt_get_single_output_for_pipe(&data.display,
+								pipe);
+			igt_output_set_pipe(output, pipe);
+			data.output = output;
+
+			mode = igt_output_get_mode(output);
+			w = mode->hdisplay;
+			h = mode->vdisplay;
+
+			data.pipe_crc = igt_pipe_crc_new(data.gfx_fd, pipe,
+							  INTEL_PIPE_CRC_SOURCE_AUTO);
+		}
+
+		igt_subtest_f("background-color-pipe-%s-black",
+			      kmstest_pipe_name(pipe))
+			test_background(&data, pipe, w, h,
+					0, 0, 0);
+
+		igt_subtest_f("background-color-pipe-%s-white",
+			      kmstest_pipe_name(pipe))
+			test_background(&data, pipe, w, h,
+					0xFFFF, 0xFFFF, 0xFFFF);
+
+		igt_subtest_f("background-color-pipe-%s-red",
+			      kmstest_pipe_name(pipe))
+			test_background(&data, pipe, w, h,
+					0xFFFF, 0, 0);
+
+		igt_subtest_f("background-color-pipe-%s-green",
+			      kmstest_pipe_name(pipe))
+
+			test_background(&data, pipe, w, h,
+					0, 0xFFFF, 0);
+
+		igt_subtest_f("background-color-pipe-%s-blue",
+			      kmstest_pipe_name(pipe))
+			test_background(&data, pipe, w, h,
+					0, 0, 0xFFFF);
+
+		igt_fixture {
+			igt_display_reset(&data.display);
+			igt_pipe_crc_free(data.pipe_crc);
+			data.pipe_crc = NULL;
+		}
+	}
 
-	igt_display_fini(&data.display);
+	igt_fixture {
+		igt_display_fini(&data.display);
+	}
 }
-- 
2.14.5

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[PATCH i-g-t 2/2] tests/kms_crtc_background_color: overhaul to match upstream ABI (v5.1)

[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 finally have an open source userspace
(ChromeOS), so lets update the IGT test to match the ABI that's actually
going upstream and to remove some of the cruft from the original test
that wouldn't actually work.

It's worth noting that CRC's don't seem to work properly on Intel gen9.
The bspec does tell us that we must have one plane enabled before taking
a pipe-level ("dmux") CRC, so this test is violating that by disabling
all planes; it's quite possible that this is the source of the CRC
mismatch.  If running on an Intel platform, you may want to run in
interactive mode ("--interactive-debug=bgcolor --skip-crc-compare") to
ensure that the colors being generated actually do match visually since
the CRC's can't be trusted.

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

v3:
 - Minor updates to proposed uapi helpers (s/rgba/argb/).

v4:
 - General restructuring into pipe/color subtests.
 - Use RGB2101010 framebuffers for comparison so that we match the bits
   of precision that Intel hardware background color accepts

v5:
 - Re-enable CRC comparisons; just because these don't work on Intel
   doesn't mean we shouldn't use them on other vendors' platforms
   (Daniel).
 - Use DRIVER_ANY rather than DRIVER_INTEL. (Heiko Stuebner)

v5.1:
 - Update commit message body discussion of CRC issues.

Cc: igt-dev@lists.freedesktop.org
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Heiko Stuebner <heiko@sntech.de>
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 lib/igt_kms.c                     |   2 +-
 tests/kms_crtc_background_color.c | 263 ++++++++++++++++++--------------------
 2 files changed, 127 insertions(+), 138 deletions(-)

diff --git a/lib/igt_kms.c b/lib/igt_kms.c
index 080f90ae..c52ec5e6 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..58cdd7a9 100644
--- a/tests/kms_crtc_background_color.c
+++ b/tests/kms_crtc_background_color.c
@@ -25,164 +25,153 @@
 #include "igt.h"
 #include <math.h>
 
-
 IGT_TEST_DESCRIPTION("Test crtc background color feature");
 
+/*
+ * Paints a desired color into a full-screen primary plane and then compares
+ * that CRC with turning off all planes and setting the CRTC background to the
+ * same color.
+ *
+ * At least on current Intel platforms, the CRC tests appear to always fail,
+ * even though the resulting pipe output seems to be the same.  The bspec tells
+ * us that we must have at least one plane turned on when taking a pipe-level
+ * CRC, so the fact that we're violating that may explain the failures.  If
+ * your platform gives failures for all tests, you may want to run with
+ * --interactive-debug=bgcolor --skip-crc-compare to visually inspect that the
+ * background color matches the equivalent solid plane.
+ */
+
 typedef struct {
-	int gfx_fd;
 	igt_display_t display;
-	struct igt_fb fb;
-	igt_crc_t ref_crc;
+	int gfx_fd;
+	igt_output_t *output;
 	igt_pipe_crc_t *pipe_crc;
+	drmModeModeInfo *mode;
 } data_t;
 
-#define BLACK      0x000000           /* BGR 8bpc */
-#define CYAN       0xFFFF00           /* BGR 8bpc */
-#define PURPLE     0xFF00FF           /* BGR 8bpc */
-#define WHITE      0xFFFFFF           /* BGR 8bpc */
-
-#define BLACK64    0x000000000000     /* BGR 16bpc */
-#define CYAN64     0xFFFFFFFF0000     /* BGR 16bpc */
-#define PURPLE64   0xFFFF0000FFFF     /* BGR 16bpc */
-#define YELLOW64   0x0000FFFFFFFF     /* BGR 16bpc */
-#define WHITE64    0xFFFFFFFFFFFF     /* BGR 16bpc */
-#define RED64      0x00000000FFFF     /* BGR 16bpc */
-#define GREEN64    0x0000FFFF0000     /* BGR 16bpc */
-#define BLUE64     0xFFFF00000000     /* BGR 16bpc */
-
-static void
-paint_background(data_t *data, struct igt_fb *fb, drmModeModeInfo *mode,
-		uint32_t background, double alpha)
+/*
+ * Local copy of kernel uapi
+ */
+static inline __u64
+local_argb(__u8 bpc, __u16 alpha, __u16 red, __u16 green, __u16 blue)
 {
-	cairo_t *cr;
-	int w, h;
-	double r, g, b;
-
-	w = mode->hdisplay;
-	h = mode->vdisplay;
-
-	cr = igt_get_cairo_ctx(data->gfx_fd, &data->fb);
+       int msb_shift = 16 - bpc;
 
-	/* Paint with background color */
-	r = (double) (background & 0xFF) / 255.0;
-	g = (double) ((background & 0xFF00) >> 8) / 255.0;
-	b = (double) ((background & 0xFF0000) >> 16) / 255.0;
-	igt_paint_color_alpha(cr, 0, 0, w, h, r, g, b, alpha);
-
-	igt_put_cairo_ctx(data->gfx_fd, &data->fb, cr);
+       return (__u64)alpha << msb_shift << 48 |
+              (__u64)red   << msb_shift << 32 |
+              (__u64)green << msb_shift << 16 |
+              (__u64)blue  << msb_shift;
 }
 
-static void prepare_crtc(data_t *data, igt_output_t *output, enum pipe pipe,
-			igt_plane_t *plane, int opaque_buffer, int plane_color,
-			uint64_t pipe_background_color)
+static void test_background(data_t *data, enum pipe pipe, int w, int h,
+			    __u64 r, __u64 g, __u64 b)
 {
-	drmModeModeInfo *mode;
-	igt_display_t *display = &data->display;
-	int fb_id;
-	double alpha;
-
-	igt_output_set_pipe(output, pipe);
-
-	/* create the pipe_crc object for this pipe */
-	igt_pipe_crc_free(data->pipe_crc);
-	data->pipe_crc = igt_pipe_crc_new(data->gfx_fd, pipe, INTEL_PIPE_CRC_SOURCE_AUTO);
-
-	mode = igt_output_get_mode(output);
-
-	fb_id = igt_create_fb(data->gfx_fd,
-			mode->hdisplay, mode->vdisplay,
-			DRM_FORMAT_XRGB8888,
-			LOCAL_DRM_FORMAT_MOD_NONE, /* tiled */
-			&data->fb);
-	igt_assert(fb_id);
-
-	/* To make FB pixel win with background color, set alpha as full opaque */
-	igt_pipe_set_prop_value(display, pipe, IGT_CRTC_BACKGROUND, pipe_background_color);
-	if (opaque_buffer)
-		alpha = 1.0;    /* alpha 1 is fully opque */
-	else
-		alpha = 0.0;    /* alpha 0 is fully transparent */
-	paint_background(data, &data->fb, mode, plane_color, alpha);
-
-	igt_plane_set_fb(plane, &data->fb);
-	igt_display_commit2(display, COMMIT_UNIVERSAL);
-}
-
-static void cleanup_crtc(data_t *data, igt_output_t *output, igt_plane_t *plane)
-{
-	igt_display_t *display = &data->display;
-
-	igt_pipe_crc_free(data->pipe_crc);
-	data->pipe_crc = NULL;
-
-	igt_remove_fb(data->gfx_fd, &data->fb);
-
-	igt_pipe_obj_set_prop_value(plane->pipe, IGT_CRTC_BACKGROUND, BLACK64);
+	igt_crc_t plane_crc, bg_crc;
+	struct igt_fb colorfb;
+	igt_plane_t *plane = igt_output_get_plane_type(data->output,
+						       DRM_PLANE_TYPE_PRIMARY);
+
+
+	/* Fill the primary plane and set the background to the same color */
+	igt_create_color_fb(data->gfx_fd, w, h, DRM_FORMAT_XRGB2101010,
+			    LOCAL_DRM_FORMAT_MOD_NONE,
+			    (double)r / 0xFFFF,
+			    (double)g / 0xFFFF,
+			    (double)b / 0xFFFF,
+			    &colorfb);
+	igt_plane_set_fb(plane, &colorfb);
+	igt_pipe_set_prop_value(&data->display, pipe, IGT_CRTC_BACKGROUND,
+				local_argb(16, 0xffff, r, g, b));
+	igt_display_commit2(&data->display, COMMIT_ATOMIC);
+	igt_pipe_crc_collect_crc(data->pipe_crc, &plane_crc);
+	igt_debug_wait_for_keypress("bgcolor");
+
+	/* Turn off the primary plane so only bg shows */
 	igt_plane_set_fb(plane, NULL);
-	igt_output_set_pipe(output, PIPE_ANY);
-
-	igt_display_commit2(display, COMMIT_UNIVERSAL);
+	igt_display_commit2(&data->display, COMMIT_ATOMIC);
+	igt_pipe_crc_collect_crc(data->pipe_crc, &bg_crc);
+	igt_debug_wait_for_keypress("bgcolor");
+
+	/*
+	 * The following test relies on hardware that generates valid CRCs
+	 * even when no planes are on.  Sadly, this doesn't appear to be the
+	 * case for current Intel platforms; pipe CRC's never match bgcolor
+	 * CRC's, likely because we're violating the bspec's guidance that there
+	 * must always be at least one real plane turned on when taking the
+	 * pipe-level ("dmux") CRC.
+	 */
+	igt_assert_crc_equal(&plane_crc, &bg_crc);
 }
 
-static void test_crtc_background(data_t *data)
+igt_main
 {
-	igt_display_t *display = &data->display;
+	data_t data = {};
 	igt_output_t *output;
+	drmModeModeInfo *mode;
+	int w, h;
 	enum pipe pipe;
-	int valid_tests = 0;
-
-	for_each_pipe_with_valid_output(display, pipe, output) {
-		igt_plane_t *plane;
-
-		igt_output_set_pipe(output, pipe);
-
-		plane = igt_output_get_plane_type(output, DRM_PLANE_TYPE_PRIMARY);
-		igt_require(igt_pipe_has_prop(display, pipe, IGT_CRTC_BACKGROUND));
-
-		prepare_crtc(data, output, pipe, plane, 1, PURPLE, BLACK64);
-
-		/* Now set background without using a plane, i.e.,
-		 * Disable the plane to let hw background color win blend. */
-		igt_plane_set_fb(plane, NULL);
-		igt_pipe_set_prop_value(display, pipe, IGT_CRTC_BACKGROUND, PURPLE64);
-		igt_display_commit2(display, COMMIT_UNIVERSAL);
-
-		/* Try few other background colors */
-		igt_pipe_set_prop_value(display, pipe, IGT_CRTC_BACKGROUND, CYAN64);
-		igt_display_commit2(display, COMMIT_UNIVERSAL);
-
-		igt_pipe_set_prop_value(display, pipe, IGT_CRTC_BACKGROUND, YELLOW64);
-		igt_display_commit2(display, COMMIT_UNIVERSAL);
 
-		igt_pipe_set_prop_value(display, pipe, IGT_CRTC_BACKGROUND, RED64);
-		igt_display_commit2(display, COMMIT_UNIVERSAL);
+	igt_fixture {
+		data.gfx_fd = drm_open_driver_master(DRIVER_ANY);
+		kmstest_set_vt_graphics_mode();
 
-		igt_pipe_set_prop_value(display, pipe, IGT_CRTC_BACKGROUND, GREEN64);
-		igt_display_commit2(display, COMMIT_UNIVERSAL);
-
-		igt_pipe_set_prop_value(display, pipe, IGT_CRTC_BACKGROUND, BLUE64);
-		igt_display_commit2(display, COMMIT_UNIVERSAL);
-
-		igt_pipe_set_prop_value(display, pipe, IGT_CRTC_BACKGROUND, WHITE64);
-		igt_display_commit2(display, COMMIT_UNIVERSAL);
-
-		valid_tests++;
-		cleanup_crtc(data, output, plane);
+		igt_require_pipe_crc(data.gfx_fd);
+		igt_display_require(&data.display, data.gfx_fd);
 	}
-	igt_require_f(valid_tests, "no valid crtc/connector combinations found\n");
-}
 
-igt_simple_main
-{
-	data_t data = {};
-
-	igt_skip_on_simulation();
-
-	data.gfx_fd = drm_open_driver(DRIVER_INTEL);
-	igt_require_pipe_crc(data.gfx_fd);
-	igt_display_require(&data.display, data.gfx_fd);
-
-	test_crtc_background(&data);
+	for_each_pipe_static(pipe) igt_subtest_group {
+		igt_fixture {
+			igt_display_require_output_on_pipe(&data.display, pipe);
+			igt_require(igt_pipe_has_prop(&data.display, pipe,
+						      IGT_CRTC_BACKGROUND));
+
+			output = igt_get_single_output_for_pipe(&data.display,
+								pipe);
+			igt_output_set_pipe(output, pipe);
+			data.output = output;
+
+			mode = igt_output_get_mode(output);
+			w = mode->hdisplay;
+			h = mode->vdisplay;
+
+			data.pipe_crc = igt_pipe_crc_new(data.gfx_fd, pipe,
+							  INTEL_PIPE_CRC_SOURCE_AUTO);
+		}
+
+		igt_subtest_f("background-color-pipe-%s-black",
+			      kmstest_pipe_name(pipe))
+			test_background(&data, pipe, w, h,
+					0, 0, 0);
+
+		igt_subtest_f("background-color-pipe-%s-white",
+			      kmstest_pipe_name(pipe))
+			test_background(&data, pipe, w, h,
+					0xFFFF, 0xFFFF, 0xFFFF);
+
+		igt_subtest_f("background-color-pipe-%s-red",
+			      kmstest_pipe_name(pipe))
+			test_background(&data, pipe, w, h,
+					0xFFFF, 0, 0);
+
+		igt_subtest_f("background-color-pipe-%s-green",
+			      kmstest_pipe_name(pipe))
+
+			test_background(&data, pipe, w, h,
+					0, 0xFFFF, 0);
+
+		igt_subtest_f("background-color-pipe-%s-blue",
+			      kmstest_pipe_name(pipe))
+			test_background(&data, pipe, w, h,
+					0, 0, 0xFFFF);
+
+		igt_fixture {
+			igt_display_reset(&data.display);
+			igt_pipe_crc_free(data.pipe_crc);
+			data.pipe_crc = NULL;
+		}
+	}
 
-	igt_display_fini(&data.display);
+	igt_fixture {
+		igt_display_fini(&data.display);
+	}
 }
-- 
2.14.5

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

[igt-dev] [PATCH i-g-t 2/2] tests/kms_crtc_background_color: overhaul to match upstream ABI (v5.1)

[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 finally have an open source userspace
(ChromeOS), so lets update the IGT test to match the ABI that's actually
going upstream and to remove some of the cruft from the original test
that wouldn't actually work.

It's worth noting that CRC's don't seem to work properly on Intel gen9.
The bspec does tell us that we must have one plane enabled before taking
a pipe-level ("dmux") CRC, so this test is violating that by disabling
all planes; it's quite possible that this is the source of the CRC
mismatch.  If running on an Intel platform, you may want to run in
interactive mode ("--interactive-debug=bgcolor --skip-crc-compare") to
ensure that the colors being generated actually do match visually since
the CRC's can't be trusted.

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

v3:
 - Minor updates to proposed uapi helpers (s/rgba/argb/).

v4:
 - General restructuring into pipe/color subtests.
 - Use RGB2101010 framebuffers for comparison so that we match the bits
   of precision that Intel hardware background color accepts

v5:
 - Re-enable CRC comparisons; just because these don't work on Intel
   doesn't mean we shouldn't use them on other vendors' platforms
   (Daniel).
 - Use DRIVER_ANY rather than DRIVER_INTEL. (Heiko Stuebner)

v5.1:
 - Update commit message body discussion of CRC issues.

Cc: igt-dev@lists.freedesktop.org
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Heiko Stuebner <heiko@sntech.de>
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 lib/igt_kms.c                     |   2 +-
 tests/kms_crtc_background_color.c | 263 ++++++++++++++++++--------------------
 2 files changed, 127 insertions(+), 138 deletions(-)

diff --git a/lib/igt_kms.c b/lib/igt_kms.c
index 080f90ae..c52ec5e6 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..58cdd7a9 100644
--- a/tests/kms_crtc_background_color.c
+++ b/tests/kms_crtc_background_color.c
@@ -25,164 +25,153 @@
 #include "igt.h"
 #include <math.h>
 
-
 IGT_TEST_DESCRIPTION("Test crtc background color feature");
 
+/*
+ * Paints a desired color into a full-screen primary plane and then compares
+ * that CRC with turning off all planes and setting the CRTC background to the
+ * same color.
+ *
+ * At least on current Intel platforms, the CRC tests appear to always fail,
+ * even though the resulting pipe output seems to be the same.  The bspec tells
+ * us that we must have at least one plane turned on when taking a pipe-level
+ * CRC, so the fact that we're violating that may explain the failures.  If
+ * your platform gives failures for all tests, you may want to run with
+ * --interactive-debug=bgcolor --skip-crc-compare to visually inspect that the
+ * background color matches the equivalent solid plane.
+ */
+
 typedef struct {
-	int gfx_fd;
 	igt_display_t display;
-	struct igt_fb fb;
-	igt_crc_t ref_crc;
+	int gfx_fd;
+	igt_output_t *output;
 	igt_pipe_crc_t *pipe_crc;
+	drmModeModeInfo *mode;
 } data_t;
 
-#define BLACK      0x000000           /* BGR 8bpc */
-#define CYAN       0xFFFF00           /* BGR 8bpc */
-#define PURPLE     0xFF00FF           /* BGR 8bpc */
-#define WHITE      0xFFFFFF           /* BGR 8bpc */
-
-#define BLACK64    0x000000000000     /* BGR 16bpc */
-#define CYAN64     0xFFFFFFFF0000     /* BGR 16bpc */
-#define PURPLE64   0xFFFF0000FFFF     /* BGR 16bpc */
-#define YELLOW64   0x0000FFFFFFFF     /* BGR 16bpc */
-#define WHITE64    0xFFFFFFFFFFFF     /* BGR 16bpc */
-#define RED64      0x00000000FFFF     /* BGR 16bpc */
-#define GREEN64    0x0000FFFF0000     /* BGR 16bpc */
-#define BLUE64     0xFFFF00000000     /* BGR 16bpc */
-
-static void
-paint_background(data_t *data, struct igt_fb *fb, drmModeModeInfo *mode,
-		uint32_t background, double alpha)
+/*
+ * Local copy of kernel uapi
+ */
+static inline __u64
+local_argb(__u8 bpc, __u16 alpha, __u16 red, __u16 green, __u16 blue)
 {
-	cairo_t *cr;
-	int w, h;
-	double r, g, b;
-
-	w = mode->hdisplay;
-	h = mode->vdisplay;
-
-	cr = igt_get_cairo_ctx(data->gfx_fd, &data->fb);
+       int msb_shift = 16 - bpc;
 
-	/* Paint with background color */
-	r = (double) (background & 0xFF) / 255.0;
-	g = (double) ((background & 0xFF00) >> 8) / 255.0;
-	b = (double) ((background & 0xFF0000) >> 16) / 255.0;
-	igt_paint_color_alpha(cr, 0, 0, w, h, r, g, b, alpha);
-
-	igt_put_cairo_ctx(data->gfx_fd, &data->fb, cr);
+       return (__u64)alpha << msb_shift << 48 |
+              (__u64)red   << msb_shift << 32 |
+              (__u64)green << msb_shift << 16 |
+              (__u64)blue  << msb_shift;
 }
 
-static void prepare_crtc(data_t *data, igt_output_t *output, enum pipe pipe,
-			igt_plane_t *plane, int opaque_buffer, int plane_color,
-			uint64_t pipe_background_color)
+static void test_background(data_t *data, enum pipe pipe, int w, int h,
+			    __u64 r, __u64 g, __u64 b)
 {
-	drmModeModeInfo *mode;
-	igt_display_t *display = &data->display;
-	int fb_id;
-	double alpha;
-
-	igt_output_set_pipe(output, pipe);
-
-	/* create the pipe_crc object for this pipe */
-	igt_pipe_crc_free(data->pipe_crc);
-	data->pipe_crc = igt_pipe_crc_new(data->gfx_fd, pipe, INTEL_PIPE_CRC_SOURCE_AUTO);
-
-	mode = igt_output_get_mode(output);
-
-	fb_id = igt_create_fb(data->gfx_fd,
-			mode->hdisplay, mode->vdisplay,
-			DRM_FORMAT_XRGB8888,
-			LOCAL_DRM_FORMAT_MOD_NONE, /* tiled */
-			&data->fb);
-	igt_assert(fb_id);
-
-	/* To make FB pixel win with background color, set alpha as full opaque */
-	igt_pipe_set_prop_value(display, pipe, IGT_CRTC_BACKGROUND, pipe_background_color);
-	if (opaque_buffer)
-		alpha = 1.0;    /* alpha 1 is fully opque */
-	else
-		alpha = 0.0;    /* alpha 0 is fully transparent */
-	paint_background(data, &data->fb, mode, plane_color, alpha);
-
-	igt_plane_set_fb(plane, &data->fb);
-	igt_display_commit2(display, COMMIT_UNIVERSAL);
-}
-
-static void cleanup_crtc(data_t *data, igt_output_t *output, igt_plane_t *plane)
-{
-	igt_display_t *display = &data->display;
-
-	igt_pipe_crc_free(data->pipe_crc);
-	data->pipe_crc = NULL;
-
-	igt_remove_fb(data->gfx_fd, &data->fb);
-
-	igt_pipe_obj_set_prop_value(plane->pipe, IGT_CRTC_BACKGROUND, BLACK64);
+	igt_crc_t plane_crc, bg_crc;
+	struct igt_fb colorfb;
+	igt_plane_t *plane = igt_output_get_plane_type(data->output,
+						       DRM_PLANE_TYPE_PRIMARY);
+
+
+	/* Fill the primary plane and set the background to the same color */
+	igt_create_color_fb(data->gfx_fd, w, h, DRM_FORMAT_XRGB2101010,
+			    LOCAL_DRM_FORMAT_MOD_NONE,
+			    (double)r / 0xFFFF,
+			    (double)g / 0xFFFF,
+			    (double)b / 0xFFFF,
+			    &colorfb);
+	igt_plane_set_fb(plane, &colorfb);
+	igt_pipe_set_prop_value(&data->display, pipe, IGT_CRTC_BACKGROUND,
+				local_argb(16, 0xffff, r, g, b));
+	igt_display_commit2(&data->display, COMMIT_ATOMIC);
+	igt_pipe_crc_collect_crc(data->pipe_crc, &plane_crc);
+	igt_debug_wait_for_keypress("bgcolor");
+
+	/* Turn off the primary plane so only bg shows */
 	igt_plane_set_fb(plane, NULL);
-	igt_output_set_pipe(output, PIPE_ANY);
-
-	igt_display_commit2(display, COMMIT_UNIVERSAL);
+	igt_display_commit2(&data->display, COMMIT_ATOMIC);
+	igt_pipe_crc_collect_crc(data->pipe_crc, &bg_crc);
+	igt_debug_wait_for_keypress("bgcolor");
+
+	/*
+	 * The following test relies on hardware that generates valid CRCs
+	 * even when no planes are on.  Sadly, this doesn't appear to be the
+	 * case for current Intel platforms; pipe CRC's never match bgcolor
+	 * CRC's, likely because we're violating the bspec's guidance that there
+	 * must always be at least one real plane turned on when taking the
+	 * pipe-level ("dmux") CRC.
+	 */
+	igt_assert_crc_equal(&plane_crc, &bg_crc);
 }
 
-static void test_crtc_background(data_t *data)
+igt_main
 {
-	igt_display_t *display = &data->display;
+	data_t data = {};
 	igt_output_t *output;
+	drmModeModeInfo *mode;
+	int w, h;
 	enum pipe pipe;
-	int valid_tests = 0;
-
-	for_each_pipe_with_valid_output(display, pipe, output) {
-		igt_plane_t *plane;
-
-		igt_output_set_pipe(output, pipe);
-
-		plane = igt_output_get_plane_type(output, DRM_PLANE_TYPE_PRIMARY);
-		igt_require(igt_pipe_has_prop(display, pipe, IGT_CRTC_BACKGROUND));
-
-		prepare_crtc(data, output, pipe, plane, 1, PURPLE, BLACK64);
-
-		/* Now set background without using a plane, i.e.,
-		 * Disable the plane to let hw background color win blend. */
-		igt_plane_set_fb(plane, NULL);
-		igt_pipe_set_prop_value(display, pipe, IGT_CRTC_BACKGROUND, PURPLE64);
-		igt_display_commit2(display, COMMIT_UNIVERSAL);
-
-		/* Try few other background colors */
-		igt_pipe_set_prop_value(display, pipe, IGT_CRTC_BACKGROUND, CYAN64);
-		igt_display_commit2(display, COMMIT_UNIVERSAL);
-
-		igt_pipe_set_prop_value(display, pipe, IGT_CRTC_BACKGROUND, YELLOW64);
-		igt_display_commit2(display, COMMIT_UNIVERSAL);
 
-		igt_pipe_set_prop_value(display, pipe, IGT_CRTC_BACKGROUND, RED64);
-		igt_display_commit2(display, COMMIT_UNIVERSAL);
+	igt_fixture {
+		data.gfx_fd = drm_open_driver_master(DRIVER_ANY);
+		kmstest_set_vt_graphics_mode();
 
-		igt_pipe_set_prop_value(display, pipe, IGT_CRTC_BACKGROUND, GREEN64);
-		igt_display_commit2(display, COMMIT_UNIVERSAL);
-
-		igt_pipe_set_prop_value(display, pipe, IGT_CRTC_BACKGROUND, BLUE64);
-		igt_display_commit2(display, COMMIT_UNIVERSAL);
-
-		igt_pipe_set_prop_value(display, pipe, IGT_CRTC_BACKGROUND, WHITE64);
-		igt_display_commit2(display, COMMIT_UNIVERSAL);
-
-		valid_tests++;
-		cleanup_crtc(data, output, plane);
+		igt_require_pipe_crc(data.gfx_fd);
+		igt_display_require(&data.display, data.gfx_fd);
 	}
-	igt_require_f(valid_tests, "no valid crtc/connector combinations found\n");
-}
 
-igt_simple_main
-{
-	data_t data = {};
-
-	igt_skip_on_simulation();
-
-	data.gfx_fd = drm_open_driver(DRIVER_INTEL);
-	igt_require_pipe_crc(data.gfx_fd);
-	igt_display_require(&data.display, data.gfx_fd);
-
-	test_crtc_background(&data);
+	for_each_pipe_static(pipe) igt_subtest_group {
+		igt_fixture {
+			igt_display_require_output_on_pipe(&data.display, pipe);
+			igt_require(igt_pipe_has_prop(&data.display, pipe,
+						      IGT_CRTC_BACKGROUND));
+
+			output = igt_get_single_output_for_pipe(&data.display,
+								pipe);
+			igt_output_set_pipe(output, pipe);
+			data.output = output;
+
+			mode = igt_output_get_mode(output);
+			w = mode->hdisplay;
+			h = mode->vdisplay;
+
+			data.pipe_crc = igt_pipe_crc_new(data.gfx_fd, pipe,
+							  INTEL_PIPE_CRC_SOURCE_AUTO);
+		}
+
+		igt_subtest_f("background-color-pipe-%s-black",
+			      kmstest_pipe_name(pipe))
+			test_background(&data, pipe, w, h,
+					0, 0, 0);
+
+		igt_subtest_f("background-color-pipe-%s-white",
+			      kmstest_pipe_name(pipe))
+			test_background(&data, pipe, w, h,
+					0xFFFF, 0xFFFF, 0xFFFF);
+
+		igt_subtest_f("background-color-pipe-%s-red",
+			      kmstest_pipe_name(pipe))
+			test_background(&data, pipe, w, h,
+					0xFFFF, 0, 0);
+
+		igt_subtest_f("background-color-pipe-%s-green",
+			      kmstest_pipe_name(pipe))
+
+			test_background(&data, pipe, w, h,
+					0, 0xFFFF, 0);
+
+		igt_subtest_f("background-color-pipe-%s-blue",
+			      kmstest_pipe_name(pipe))
+			test_background(&data, pipe, w, h,
+					0, 0, 0xFFFF);
+
+		igt_fixture {
+			igt_display_reset(&data.display);
+			igt_pipe_crc_free(data.pipe_crc);
+			data.pipe_crc = NULL;
+		}
+	}
 
-	igt_display_fini(&data.display);
+	igt_fixture {
+		igt_display_fini(&data.display);
+	}
 }
-- 
2.14.5

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t 1/2] lib: Add --skip-crc-compare option

[Daniel Vetter]

On Wed, Feb 20, 2019 at 04:34:21PM -0800, Matt Roper wrote:
> When using --interactive-debug, it's sometimes desirable to ignore CRC
> mismatches and let the test proceed as if they passed so that the
> on-screen outcome can be inspected.  Let's add a debug option to allow
> this.
> 
> Cc: igt-dev@lists.freedesktop.org
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>

I'm wondering whether we shouldn't tie this into an --interactive-debug
mode, since you'll need both for the manual run to be actually useful.
E.g. anytime any --interactive-debug is set we skip all crc checks.
-Daniel

> ---
>  lib/igt_core.c    | 7 +++++++
>  lib/igt_core.h    | 1 +
>  lib/igt_debugfs.c | 8 +++++++-
>  3 files changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/igt_core.c b/lib/igt_core.c
> index 71b05d3b..5523950f 100644
> --- a/lib/igt_core.c
> +++ b/lib/igt_core.c
> @@ -256,6 +256,7 @@
>  
>  static unsigned int exit_handler_count;
>  const char *igt_interactive_debug;
> +bool igt_skip_crc_compare;
>  
>  /* subtests helpers */
>  static bool list_subtests = false;
> @@ -285,6 +286,7 @@ enum {
>   OPT_DESCRIPTION,
>   OPT_DEBUG,
>   OPT_INTERACTIVE_DEBUG,
> + OPT_SKIP_CRC,
>   OPT_HELP = 'h'
>  };
>  
> @@ -552,6 +554,7 @@ static void print_usage(const char *help_str, bool output_on_stderr)
>  		   "  --run-subtest <pattern>\n"
>  		   "  --debug[=log-domain]\n"
>  		   "  --interactive-debug[=domain]\n"
> +		   "  --skip-crc-compare\n"
>  		   "  --help-description\n"
>  		   "  --help\n");
>  	if (help_str)
> @@ -668,6 +671,7 @@ static int common_init(int *argc, char **argv,
>  		{"help-description", 0, 0, OPT_DESCRIPTION},
>  		{"debug", optional_argument, 0, OPT_DEBUG},
>  		{"interactive-debug", optional_argument, 0, OPT_INTERACTIVE_DEBUG},
> +		{"skip-crc-compare", 0, 0, OPT_SKIP_CRC},
>  		{"help", 0, 0, OPT_HELP},
>  		{0, 0, 0, 0}
>  	};
> @@ -768,6 +772,9 @@ static int common_init(int *argc, char **argv,
>  			print_test_description();
>  			ret = -1;
>  			goto out;
> +		case OPT_SKIP_CRC:
> +			igt_skip_crc_compare = true;
> +			goto out;
>  		case OPT_HELP:
>  			print_usage(help_str, false);
>  			ret = -1;
> diff --git a/lib/igt_core.h b/lib/igt_core.h
> index 47ffd9e7..f12fc5cb 100644
> --- a/lib/igt_core.h
> +++ b/lib/igt_core.h
> @@ -843,6 +843,7 @@ bool igt_run_in_simulation(void);
>  void igt_skip_on_simulation(void);
>  
>  extern const char *igt_interactive_debug;
> +extern bool igt_skip_crc_compare;
>  
>  /**
>   * igt_log_level:
> diff --git a/lib/igt_debugfs.c b/lib/igt_debugfs.c
> index 640c78e9..04d1648b 100644
> --- a/lib/igt_debugfs.c
> +++ b/lib/igt_debugfs.c
> @@ -405,6 +405,12 @@ static bool igt_find_crc_mismatch(const igt_crc_t *a, const igt_crc_t *b,
>   * assert that CRCs match, never that they are different. Otherwise there might
>   * be random testcase failures when different screen contents end up with the
>   * same CRC by chance.
> + *
> + * Passing --skip-crc-compare on the command line will force this function
> + * to always pass, which can be useful in interactive debugging where you
> + * might know the test will fail, but still want the test to keep going as if
> + * it had succeeded so that you can see the on-screen behavior.
> + *
>   */
>  void igt_assert_crc_equal(const igt_crc_t *a, const igt_crc_t *b)
>  {
> @@ -416,7 +422,7 @@ void igt_assert_crc_equal(const igt_crc_t *a, const igt_crc_t *b)
>  		igt_debug("CRC mismatch at index %d: 0x%x != 0x%x\n", index,
>  			  a->crc[index], b->crc[index]);
>  
> -	igt_assert(!mismatch);
> +	igt_assert(!mismatch || igt_skip_crc_compare);
>  }
>  
>  /**
> -- 
> 2.14.5
> 
> _______________________________________________
> igt-dev mailing list
> igt-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/igt-dev

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [igt-dev] [PATCH i-g-t 1/2] lib: Add --skip-crc-compare option

[Daniel Vetter]

On Wed, Feb 20, 2019 at 04:34:21PM -0800, Matt Roper wrote:
> When using --interactive-debug, it's sometimes desirable to ignore CRC
> mismatches and let the test proceed as if they passed so that the
> on-screen outcome can be inspected.  Let's add a debug option to allow
> this.
> 
> Cc: igt-dev@lists.freedesktop.org
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>

I'm wondering whether we shouldn't tie this into an --interactive-debug
mode, since you'll need both for the manual run to be actually useful.
E.g. anytime any --interactive-debug is set we skip all crc checks.
-Daniel

> ---
>  lib/igt_core.c    | 7 +++++++
>  lib/igt_core.h    | 1 +
>  lib/igt_debugfs.c | 8 +++++++-
>  3 files changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/igt_core.c b/lib/igt_core.c
> index 71b05d3b..5523950f 100644
> --- a/lib/igt_core.c
> +++ b/lib/igt_core.c
> @@ -256,6 +256,7 @@
>  
>  static unsigned int exit_handler_count;
>  const char *igt_interactive_debug;
> +bool igt_skip_crc_compare;
>  
>  /* subtests helpers */
>  static bool list_subtests = false;
> @@ -285,6 +286,7 @@ enum {
>   OPT_DESCRIPTION,
>   OPT_DEBUG,
>   OPT_INTERACTIVE_DEBUG,
> + OPT_SKIP_CRC,
>   OPT_HELP = 'h'
>  };
>  
> @@ -552,6 +554,7 @@ static void print_usage(const char *help_str, bool output_on_stderr)
>  		   "  --run-subtest <pattern>\n"
>  		   "  --debug[=log-domain]\n"
>  		   "  --interactive-debug[=domain]\n"
> +		   "  --skip-crc-compare\n"
>  		   "  --help-description\n"
>  		   "  --help\n");
>  	if (help_str)
> @@ -668,6 +671,7 @@ static int common_init(int *argc, char **argv,
>  		{"help-description", 0, 0, OPT_DESCRIPTION},
>  		{"debug", optional_argument, 0, OPT_DEBUG},
>  		{"interactive-debug", optional_argument, 0, OPT_INTERACTIVE_DEBUG},
> +		{"skip-crc-compare", 0, 0, OPT_SKIP_CRC},
>  		{"help", 0, 0, OPT_HELP},
>  		{0, 0, 0, 0}
>  	};
> @@ -768,6 +772,9 @@ static int common_init(int *argc, char **argv,
>  			print_test_description();
>  			ret = -1;
>  			goto out;
> +		case OPT_SKIP_CRC:
> +			igt_skip_crc_compare = true;
> +			goto out;
>  		case OPT_HELP:
>  			print_usage(help_str, false);
>  			ret = -1;
> diff --git a/lib/igt_core.h b/lib/igt_core.h
> index 47ffd9e7..f12fc5cb 100644
> --- a/lib/igt_core.h
> +++ b/lib/igt_core.h
> @@ -843,6 +843,7 @@ bool igt_run_in_simulation(void);
>  void igt_skip_on_simulation(void);
>  
>  extern const char *igt_interactive_debug;
> +extern bool igt_skip_crc_compare;
>  
>  /**
>   * igt_log_level:
> diff --git a/lib/igt_debugfs.c b/lib/igt_debugfs.c
> index 640c78e9..04d1648b 100644
> --- a/lib/igt_debugfs.c
> +++ b/lib/igt_debugfs.c
> @@ -405,6 +405,12 @@ static bool igt_find_crc_mismatch(const igt_crc_t *a, const igt_crc_t *b,
>   * assert that CRCs match, never that they are different. Otherwise there might
>   * be random testcase failures when different screen contents end up with the
>   * same CRC by chance.
> + *
> + * Passing --skip-crc-compare on the command line will force this function
> + * to always pass, which can be useful in interactive debugging where you
> + * might know the test will fail, but still want the test to keep going as if
> + * it had succeeded so that you can see the on-screen behavior.
> + *
>   */
>  void igt_assert_crc_equal(const igt_crc_t *a, const igt_crc_t *b)
>  {
> @@ -416,7 +422,7 @@ void igt_assert_crc_equal(const igt_crc_t *a, const igt_crc_t *b)
>  		igt_debug("CRC mismatch at index %d: 0x%x != 0x%x\n", index,
>  			  a->crc[index], b->crc[index]);
>  
> -	igt_assert(!mismatch);
> +	igt_assert(!mismatch || igt_skip_crc_compare);
>  }
>  
>  /**
> -- 
> 2.14.5
> 
> _______________________________________________
> igt-dev mailing list
> igt-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/igt-dev

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t 1/2] lib: Add --skip-crc-compare option

[Ville Syrjälä]

On Thu, Feb 21, 2019 at 10:33:25AM +0100, Daniel Vetter wrote:
> On Wed, Feb 20, 2019 at 04:34:21PM -0800, Matt Roper wrote:
> > When using --interactive-debug, it's sometimes desirable to ignore CRC
> > mismatches and let the test proceed as if they passed so that the
> > on-screen outcome can be inspected.  Let's add a debug option to allow
> > this.
> > 
> > Cc: igt-dev@lists.freedesktop.org
> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> 
> I'm wondering whether we shouldn't tie this into an --interactive-debug
> mode, since you'll need both for the manual run to be actually useful.
> E.g. anytime any --interactive-debug is set we skip all crc checks.

That might make sense, but I'd like to still have the option to disable
the crc checks without interactive debug as well. Good for observing
tearing and whatnot. When looking at failures I seem to end up patching
the crc checks out most of the time and relying on the tracepoints
to get me the data I need. This knob could save me a step.

So
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> -Daniel
> 
> > ---
> >  lib/igt_core.c    | 7 +++++++
> >  lib/igt_core.h    | 1 +
> >  lib/igt_debugfs.c | 8 +++++++-
> >  3 files changed, 15 insertions(+), 1 deletion(-)
> > 
> > diff --git a/lib/igt_core.c b/lib/igt_core.c
> > index 71b05d3b..5523950f 100644
> > --- a/lib/igt_core.c
> > +++ b/lib/igt_core.c
> > @@ -256,6 +256,7 @@
> >  
> >  static unsigned int exit_handler_count;
> >  const char *igt_interactive_debug;
> > +bool igt_skip_crc_compare;
> >  
> >  /* subtests helpers */
> >  static bool list_subtests = false;
> > @@ -285,6 +286,7 @@ enum {
> >   OPT_DESCRIPTION,
> >   OPT_DEBUG,
> >   OPT_INTERACTIVE_DEBUG,
> > + OPT_SKIP_CRC,
> >   OPT_HELP = 'h'
> >  };
> >  
> > @@ -552,6 +554,7 @@ static void print_usage(const char *help_str, bool output_on_stderr)
> >  		   "  --run-subtest <pattern>\n"
> >  		   "  --debug[=log-domain]\n"
> >  		   "  --interactive-debug[=domain]\n"
> > +		   "  --skip-crc-compare\n"
> >  		   "  --help-description\n"
> >  		   "  --help\n");
> >  	if (help_str)
> > @@ -668,6 +671,7 @@ static int common_init(int *argc, char **argv,
> >  		{"help-description", 0, 0, OPT_DESCRIPTION},
> >  		{"debug", optional_argument, 0, OPT_DEBUG},
> >  		{"interactive-debug", optional_argument, 0, OPT_INTERACTIVE_DEBUG},
> > +		{"skip-crc-compare", 0, 0, OPT_SKIP_CRC},
> >  		{"help", 0, 0, OPT_HELP},
> >  		{0, 0, 0, 0}
> >  	};
> > @@ -768,6 +772,9 @@ static int common_init(int *argc, char **argv,
> >  			print_test_description();
> >  			ret = -1;
> >  			goto out;
> > +		case OPT_SKIP_CRC:
> > +			igt_skip_crc_compare = true;
> > +			goto out;
> >  		case OPT_HELP:
> >  			print_usage(help_str, false);
> >  			ret = -1;
> > diff --git a/lib/igt_core.h b/lib/igt_core.h
> > index 47ffd9e7..f12fc5cb 100644
> > --- a/lib/igt_core.h
> > +++ b/lib/igt_core.h
> > @@ -843,6 +843,7 @@ bool igt_run_in_simulation(void);
> >  void igt_skip_on_simulation(void);
> >  
> >  extern const char *igt_interactive_debug;
> > +extern bool igt_skip_crc_compare;
> >  
> >  /**
> >   * igt_log_level:
> > diff --git a/lib/igt_debugfs.c b/lib/igt_debugfs.c
> > index 640c78e9..04d1648b 100644
> > --- a/lib/igt_debugfs.c
> > +++ b/lib/igt_debugfs.c
> > @@ -405,6 +405,12 @@ static bool igt_find_crc_mismatch(const igt_crc_t *a, const igt_crc_t *b,
> >   * assert that CRCs match, never that they are different. Otherwise there might
> >   * be random testcase failures when different screen contents end up with the
> >   * same CRC by chance.
> > + *
> > + * Passing --skip-crc-compare on the command line will force this function
> > + * to always pass, which can be useful in interactive debugging where you
> > + * might know the test will fail, but still want the test to keep going as if
> > + * it had succeeded so that you can see the on-screen behavior.
> > + *
> >   */
> >  void igt_assert_crc_equal(const igt_crc_t *a, const igt_crc_t *b)
> >  {
> > @@ -416,7 +422,7 @@ void igt_assert_crc_equal(const igt_crc_t *a, const igt_crc_t *b)
> >  		igt_debug("CRC mismatch at index %d: 0x%x != 0x%x\n", index,
> >  			  a->crc[index], b->crc[index]);
> >  
> > -	igt_assert(!mismatch);
> > +	igt_assert(!mismatch || igt_skip_crc_compare);
> >  }
> >  
> >  /**
> > -- 
> > 2.14.5
> > 
> > _______________________________________________
> > igt-dev mailing list
> > igt-dev@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/igt-dev
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
> _______________________________________________
> igt-dev mailing list
> igt-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/igt-dev

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [igt-dev] [PATCH i-g-t 1/2] lib: Add --skip-crc-compare option

[Ville Syrjälä]

On Thu, Feb 21, 2019 at 10:33:25AM +0100, Daniel Vetter wrote:
> On Wed, Feb 20, 2019 at 04:34:21PM -0800, Matt Roper wrote:
> > When using --interactive-debug, it's sometimes desirable to ignore CRC
> > mismatches and let the test proceed as if they passed so that the
> > on-screen outcome can be inspected.  Let's add a debug option to allow
> > this.
> > 
> > Cc: igt-dev@lists.freedesktop.org
> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> 
> I'm wondering whether we shouldn't tie this into an --interactive-debug
> mode, since you'll need both for the manual run to be actually useful.
> E.g. anytime any --interactive-debug is set we skip all crc checks.

That might make sense, but I'd like to still have the option to disable
the crc checks without interactive debug as well. Good for observing
tearing and whatnot. When looking at failures I seem to end up patching
the crc checks out most of the time and relying on the tracepoints
to get me the data I need. This knob could save me a step.

So
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> -Daniel
> 
> > ---
> >  lib/igt_core.c    | 7 +++++++
> >  lib/igt_core.h    | 1 +
> >  lib/igt_debugfs.c | 8 +++++++-
> >  3 files changed, 15 insertions(+), 1 deletion(-)
> > 
> > diff --git a/lib/igt_core.c b/lib/igt_core.c
> > index 71b05d3b..5523950f 100644
> > --- a/lib/igt_core.c
> > +++ b/lib/igt_core.c
> > @@ -256,6 +256,7 @@
> >  
> >  static unsigned int exit_handler_count;
> >  const char *igt_interactive_debug;
> > +bool igt_skip_crc_compare;
> >  
> >  /* subtests helpers */
> >  static bool list_subtests = false;
> > @@ -285,6 +286,7 @@ enum {
> >   OPT_DESCRIPTION,
> >   OPT_DEBUG,
> >   OPT_INTERACTIVE_DEBUG,
> > + OPT_SKIP_CRC,
> >   OPT_HELP = 'h'
> >  };
> >  
> > @@ -552,6 +554,7 @@ static void print_usage(const char *help_str, bool output_on_stderr)
> >  		   "  --run-subtest <pattern>\n"
> >  		   "  --debug[=log-domain]\n"
> >  		   "  --interactive-debug[=domain]\n"
> > +		   "  --skip-crc-compare\n"
> >  		   "  --help-description\n"
> >  		   "  --help\n");
> >  	if (help_str)
> > @@ -668,6 +671,7 @@ static int common_init(int *argc, char **argv,
> >  		{"help-description", 0, 0, OPT_DESCRIPTION},
> >  		{"debug", optional_argument, 0, OPT_DEBUG},
> >  		{"interactive-debug", optional_argument, 0, OPT_INTERACTIVE_DEBUG},
> > +		{"skip-crc-compare", 0, 0, OPT_SKIP_CRC},
> >  		{"help", 0, 0, OPT_HELP},
> >  		{0, 0, 0, 0}
> >  	};
> > @@ -768,6 +772,9 @@ static int common_init(int *argc, char **argv,
> >  			print_test_description();
> >  			ret = -1;
> >  			goto out;
> > +		case OPT_SKIP_CRC:
> > +			igt_skip_crc_compare = true;
> > +			goto out;
> >  		case OPT_HELP:
> >  			print_usage(help_str, false);
> >  			ret = -1;
> > diff --git a/lib/igt_core.h b/lib/igt_core.h
> > index 47ffd9e7..f12fc5cb 100644
> > --- a/lib/igt_core.h
> > +++ b/lib/igt_core.h
> > @@ -843,6 +843,7 @@ bool igt_run_in_simulation(void);
> >  void igt_skip_on_simulation(void);
> >  
> >  extern const char *igt_interactive_debug;
> > +extern bool igt_skip_crc_compare;
> >  
> >  /**
> >   * igt_log_level:
> > diff --git a/lib/igt_debugfs.c b/lib/igt_debugfs.c
> > index 640c78e9..04d1648b 100644
> > --- a/lib/igt_debugfs.c
> > +++ b/lib/igt_debugfs.c
> > @@ -405,6 +405,12 @@ static bool igt_find_crc_mismatch(const igt_crc_t *a, const igt_crc_t *b,
> >   * assert that CRCs match, never that they are different. Otherwise there might
> >   * be random testcase failures when different screen contents end up with the
> >   * same CRC by chance.
> > + *
> > + * Passing --skip-crc-compare on the command line will force this function
> > + * to always pass, which can be useful in interactive debugging where you
> > + * might know the test will fail, but still want the test to keep going as if
> > + * it had succeeded so that you can see the on-screen behavior.
> > + *
> >   */
> >  void igt_assert_crc_equal(const igt_crc_t *a, const igt_crc_t *b)
> >  {
> > @@ -416,7 +422,7 @@ void igt_assert_crc_equal(const igt_crc_t *a, const igt_crc_t *b)
> >  		igt_debug("CRC mismatch at index %d: 0x%x != 0x%x\n", index,
> >  			  a->crc[index], b->crc[index]);
> >  
> > -	igt_assert(!mismatch);
> > +	igt_assert(!mismatch || igt_skip_crc_compare);
> >  }
> >  
> >  /**
> > -- 
> > 2.14.5
> > 
> > _______________________________________________
> > igt-dev mailing list
> > igt-dev@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/igt-dev
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
> _______________________________________________
> igt-dev mailing list
> igt-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/igt-dev

-- 
Ville Syrjälä
Intel
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t 1/2] lib: Add --skip-crc-compare option

[Daniel Vetter]

On Wed, Feb 20, 2019 at 04:34:21PM -0800, Matt Roper wrote:
> When using --interactive-debug, it's sometimes desirable to ignore CRC
> mismatches and let the test proceed as if they passed so that the
> on-screen outcome can be inspected.  Let's add a debug option to allow
> this.
> 
> Cc: igt-dev@lists.freedesktop.org
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>  lib/igt_core.c    | 7 +++++++
>  lib/igt_core.h    | 1 +
>  lib/igt_debugfs.c | 8 +++++++-
>  3 files changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/igt_core.c b/lib/igt_core.c
> index 71b05d3b..5523950f 100644
> --- a/lib/igt_core.c
> +++ b/lib/igt_core.c
> @@ -256,6 +256,7 @@
>  
>  static unsigned int exit_handler_count;
>  const char *igt_interactive_debug;
> +bool igt_skip_crc_compare;
>  
>  /* subtests helpers */
>  static bool list_subtests = false;
> @@ -285,6 +286,7 @@ enum {
>   OPT_DESCRIPTION,
>   OPT_DEBUG,
>   OPT_INTERACTIVE_DEBUG,
> + OPT_SKIP_CRC,
>   OPT_HELP = 'h'
>  };
>  
> @@ -552,6 +554,7 @@ static void print_usage(const char *help_str, bool output_on_stderr)
>  		   "  --run-subtest <pattern>\n"
>  		   "  --debug[=log-domain]\n"
>  		   "  --interactive-debug[=domain]\n"
> +		   "  --skip-crc-compare\n"
>  		   "  --help-description\n"
>  		   "  --help\n");
>  	if (help_str)
> @@ -668,6 +671,7 @@ static int common_init(int *argc, char **argv,
>  		{"help-description", 0, 0, OPT_DESCRIPTION},
>  		{"debug", optional_argument, 0, OPT_DEBUG},
>  		{"interactive-debug", optional_argument, 0, OPT_INTERACTIVE_DEBUG},
> +		{"skip-crc-compare", 0, 0, OPT_SKIP_CRC},
>  		{"help", 0, 0, OPT_HELP},
>  		{0, 0, 0, 0}
>  	};
> @@ -768,6 +772,9 @@ static int common_init(int *argc, char **argv,
>  			print_test_description();
>  			ret = -1;
>  			goto out;
> +		case OPT_SKIP_CRC:
> +			igt_skip_crc_compare = true;
> +			goto out;
>  		case OPT_HELP:
>  			print_usage(help_str, false);
>  			ret = -1;
> diff --git a/lib/igt_core.h b/lib/igt_core.h
> index 47ffd9e7..f12fc5cb 100644
> --- a/lib/igt_core.h
> +++ b/lib/igt_core.h
> @@ -843,6 +843,7 @@ bool igt_run_in_simulation(void);
>  void igt_skip_on_simulation(void);
>  
>  extern const char *igt_interactive_debug;
> +extern bool igt_skip_crc_compare;
>  
>  /**
>   * igt_log_level:
> diff --git a/lib/igt_debugfs.c b/lib/igt_debugfs.c
> index 640c78e9..04d1648b 100644
> --- a/lib/igt_debugfs.c
> +++ b/lib/igt_debugfs.c
> @@ -405,6 +405,12 @@ static bool igt_find_crc_mismatch(const igt_crc_t *a, const igt_crc_t *b,
>   * assert that CRCs match, never that they are different. Otherwise there might
>   * be random testcase failures when different screen contents end up with the
>   * same CRC by chance.
> + *
> + * Passing --skip-crc-compare on the command line will force this function
> + * to always pass, which can be useful in interactive debugging where you
> + * might know the test will fail, but still want the test to keep going as if
> + * it had succeeded so that you can see the on-screen behavior.
> + *
>   */
>  void igt_assert_crc_equal(const igt_crc_t *a, const igt_crc_t *b)
>  {
> @@ -416,7 +422,7 @@ void igt_assert_crc_equal(const igt_crc_t *a, const igt_crc_t *b)
>  		igt_debug("CRC mismatch at index %d: 0x%x != 0x%x\n", index,
>  			  a->crc[index], b->crc[index]);
>  
> -	igt_assert(!mismatch);
> +	igt_assert(!mismatch || igt_skip_crc_compare);

I think an igt_debug line here could be useful when we skip the assert.
With that

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

aka Ville convinced me.
-Daniel

>  }
>  
>  /**
> -- 
> 2.14.5
> 
> _______________________________________________
> igt-dev mailing list
> igt-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/igt-dev

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [igt-dev] [PATCH i-g-t 1/2] lib: Add --skip-crc-compare option

[Daniel Vetter]

On Wed, Feb 20, 2019 at 04:34:21PM -0800, Matt Roper wrote:
> When using --interactive-debug, it's sometimes desirable to ignore CRC
> mismatches and let the test proceed as if they passed so that the
> on-screen outcome can be inspected.  Let's add a debug option to allow
> this.
> 
> Cc: igt-dev@lists.freedesktop.org
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>  lib/igt_core.c    | 7 +++++++
>  lib/igt_core.h    | 1 +
>  lib/igt_debugfs.c | 8 +++++++-
>  3 files changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/igt_core.c b/lib/igt_core.c
> index 71b05d3b..5523950f 100644
> --- a/lib/igt_core.c
> +++ b/lib/igt_core.c
> @@ -256,6 +256,7 @@
>  
>  static unsigned int exit_handler_count;
>  const char *igt_interactive_debug;
> +bool igt_skip_crc_compare;
>  
>  /* subtests helpers */
>  static bool list_subtests = false;
> @@ -285,6 +286,7 @@ enum {
>   OPT_DESCRIPTION,
>   OPT_DEBUG,
>   OPT_INTERACTIVE_DEBUG,
> + OPT_SKIP_CRC,
>   OPT_HELP = 'h'
>  };
>  
> @@ -552,6 +554,7 @@ static void print_usage(const char *help_str, bool output_on_stderr)
>  		   "  --run-subtest <pattern>\n"
>  		   "  --debug[=log-domain]\n"
>  		   "  --interactive-debug[=domain]\n"
> +		   "  --skip-crc-compare\n"
>  		   "  --help-description\n"
>  		   "  --help\n");
>  	if (help_str)
> @@ -668,6 +671,7 @@ static int common_init(int *argc, char **argv,
>  		{"help-description", 0, 0, OPT_DESCRIPTION},
>  		{"debug", optional_argument, 0, OPT_DEBUG},
>  		{"interactive-debug", optional_argument, 0, OPT_INTERACTIVE_DEBUG},
> +		{"skip-crc-compare", 0, 0, OPT_SKIP_CRC},
>  		{"help", 0, 0, OPT_HELP},
>  		{0, 0, 0, 0}
>  	};
> @@ -768,6 +772,9 @@ static int common_init(int *argc, char **argv,
>  			print_test_description();
>  			ret = -1;
>  			goto out;
> +		case OPT_SKIP_CRC:
> +			igt_skip_crc_compare = true;
> +			goto out;
>  		case OPT_HELP:
>  			print_usage(help_str, false);
>  			ret = -1;
> diff --git a/lib/igt_core.h b/lib/igt_core.h
> index 47ffd9e7..f12fc5cb 100644
> --- a/lib/igt_core.h
> +++ b/lib/igt_core.h
> @@ -843,6 +843,7 @@ bool igt_run_in_simulation(void);
>  void igt_skip_on_simulation(void);
>  
>  extern const char *igt_interactive_debug;
> +extern bool igt_skip_crc_compare;
>  
>  /**
>   * igt_log_level:
> diff --git a/lib/igt_debugfs.c b/lib/igt_debugfs.c
> index 640c78e9..04d1648b 100644
> --- a/lib/igt_debugfs.c
> +++ b/lib/igt_debugfs.c
> @@ -405,6 +405,12 @@ static bool igt_find_crc_mismatch(const igt_crc_t *a, const igt_crc_t *b,
>   * assert that CRCs match, never that they are different. Otherwise there might
>   * be random testcase failures when different screen contents end up with the
>   * same CRC by chance.
> + *
> + * Passing --skip-crc-compare on the command line will force this function
> + * to always pass, which can be useful in interactive debugging where you
> + * might know the test will fail, but still want the test to keep going as if
> + * it had succeeded so that you can see the on-screen behavior.
> + *
>   */
>  void igt_assert_crc_equal(const igt_crc_t *a, const igt_crc_t *b)
>  {
> @@ -416,7 +422,7 @@ void igt_assert_crc_equal(const igt_crc_t *a, const igt_crc_t *b)
>  		igt_debug("CRC mismatch at index %d: 0x%x != 0x%x\n", index,
>  			  a->crc[index], b->crc[index]);
>  
> -	igt_assert(!mismatch);
> +	igt_assert(!mismatch || igt_skip_crc_compare);

I think an igt_debug line here could be useful when we skip the assert.
With that

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

aka Ville convinced me.
-Daniel

>  }
>  
>  /**
> -- 
> 2.14.5
> 
> _______________________________________________
> igt-dev mailing list
> igt-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/igt-dev

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [igt-dev] [PATCH i-g-t 1/2] lib: Add --skip-crc-compare option

[Ville Syrjälä]

On Fri, Feb 22, 2019 at 03:32:50PM +0100, Daniel Vetter wrote:
> On Wed, Feb 20, 2019 at 04:34:21PM -0800, Matt Roper wrote:
> > When using --interactive-debug, it's sometimes desirable to ignore CRC
> > mismatches and let the test proceed as if they passed so that the
> > on-screen outcome can be inspected.  Let's add a debug option to allow
> > this.
> > 
> > Cc: igt-dev@lists.freedesktop.org
> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> > ---
> >  lib/igt_core.c    | 7 +++++++
> >  lib/igt_core.h    | 1 +
> >  lib/igt_debugfs.c | 8 +++++++-
> >  3 files changed, 15 insertions(+), 1 deletion(-)
> > 
> > diff --git a/lib/igt_core.c b/lib/igt_core.c
> > index 71b05d3b..5523950f 100644
> > --- a/lib/igt_core.c
> > +++ b/lib/igt_core.c
> > @@ -256,6 +256,7 @@
> >  
> >  static unsigned int exit_handler_count;
> >  const char *igt_interactive_debug;
> > +bool igt_skip_crc_compare;
> >  
> >  /* subtests helpers */
> >  static bool list_subtests = false;
> > @@ -285,6 +286,7 @@ enum {
> >   OPT_DESCRIPTION,
> >   OPT_DEBUG,
> >   OPT_INTERACTIVE_DEBUG,
> > + OPT_SKIP_CRC,
> >   OPT_HELP = 'h'
> >  };
> >  
> > @@ -552,6 +554,7 @@ static void print_usage(const char *help_str, bool output_on_stderr)
> >  		   "  --run-subtest <pattern>\n"
> >  		   "  --debug[=log-domain]\n"
> >  		   "  --interactive-debug[=domain]\n"
> > +		   "  --skip-crc-compare\n"
> >  		   "  --help-description\n"
> >  		   "  --help\n");
> >  	if (help_str)
> > @@ -668,6 +671,7 @@ static int common_init(int *argc, char **argv,
> >  		{"help-description", 0, 0, OPT_DESCRIPTION},
> >  		{"debug", optional_argument, 0, OPT_DEBUG},
> >  		{"interactive-debug", optional_argument, 0, OPT_INTERACTIVE_DEBUG},
> > +		{"skip-crc-compare", 0, 0, OPT_SKIP_CRC},
> >  		{"help", 0, 0, OPT_HELP},
> >  		{0, 0, 0, 0}
> >  	};
> > @@ -768,6 +772,9 @@ static int common_init(int *argc, char **argv,
> >  			print_test_description();
> >  			ret = -1;
> >  			goto out;
> > +		case OPT_SKIP_CRC:
> > +			igt_skip_crc_compare = true;
> > +			goto out;
> >  		case OPT_HELP:
> >  			print_usage(help_str, false);
> >  			ret = -1;
> > diff --git a/lib/igt_core.h b/lib/igt_core.h
> > index 47ffd9e7..f12fc5cb 100644
> > --- a/lib/igt_core.h
> > +++ b/lib/igt_core.h
> > @@ -843,6 +843,7 @@ bool igt_run_in_simulation(void);
> >  void igt_skip_on_simulation(void);
> >  
> >  extern const char *igt_interactive_debug;
> > +extern bool igt_skip_crc_compare;
> >  
> >  /**
> >   * igt_log_level:
> > diff --git a/lib/igt_debugfs.c b/lib/igt_debugfs.c
> > index 640c78e9..04d1648b 100644
> > --- a/lib/igt_debugfs.c
> > +++ b/lib/igt_debugfs.c
> > @@ -405,6 +405,12 @@ static bool igt_find_crc_mismatch(const igt_crc_t *a, const igt_crc_t *b,
> >   * assert that CRCs match, never that they are different. Otherwise there might
> >   * be random testcase failures when different screen contents end up with the
> >   * same CRC by chance.
> > + *
> > + * Passing --skip-crc-compare on the command line will force this function
> > + * to always pass, which can be useful in interactive debugging where you
> > + * might know the test will fail, but still want the test to keep going as if
> > + * it had succeeded so that you can see the on-screen behavior.
> > + *
> >   */
> >  void igt_assert_crc_equal(const igt_crc_t *a, const igt_crc_t *b)
> >  {
> > @@ -416,7 +422,7 @@ void igt_assert_crc_equal(const igt_crc_t *a, const igt_crc_t *b)
> >  		igt_debug("CRC mismatch at index %d: 0x%x != 0x%x\n", index,
> >  			  a->crc[index], b->crc[index]);
> >  
> > -	igt_assert(!mismatch);
> > +	igt_assert(!mismatch || igt_skip_crc_compare);
> 
> I think an igt_debug line here could be useful when we skip the assert.
> With that

Daniel, is the debug print just above not sufficient? Or would you like
a more explicit indication that we skipped the assert even though there
was mismatch?

I guess we could do something like:

if (mismatch)
 igt_debug("CRC mismatch%s at index %d: 0x%x != 0x%x\n",
           skip_crc_compare ? " (ignored)" : "",
	   index, a->crc[index], b->crc[index]);

to avoid spamming the log with two lines per mismatch?

I was just looking for this knob until I realized we never applied this
patch :/

> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> aka Ville convinced me.
> -Daniel
> 
> >  }
> >  
> >  /**
> > -- 
> > 2.14.5
> > 
> > _______________________________________________
> > igt-dev mailing list
> > igt-dev@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/igt-dev
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [igt-dev] [Intel-gfx] [PATCH i-g-t 1/2] lib: Add --skip-crc-compare option

[Ville Syrjälä]

On Fri, Feb 22, 2019 at 03:32:50PM +0100, Daniel Vetter wrote:
> On Wed, Feb 20, 2019 at 04:34:21PM -0800, Matt Roper wrote:
> > When using --interactive-debug, it's sometimes desirable to ignore CRC
> > mismatches and let the test proceed as if they passed so that the
> > on-screen outcome can be inspected.  Let's add a debug option to allow
> > this.
> > 
> > Cc: igt-dev@lists.freedesktop.org
> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> > ---
> >  lib/igt_core.c    | 7 +++++++
> >  lib/igt_core.h    | 1 +
> >  lib/igt_debugfs.c | 8 +++++++-
> >  3 files changed, 15 insertions(+), 1 deletion(-)
> > 
> > diff --git a/lib/igt_core.c b/lib/igt_core.c
> > index 71b05d3b..5523950f 100644
> > --- a/lib/igt_core.c
> > +++ b/lib/igt_core.c
> > @@ -256,6 +256,7 @@
> >  
> >  static unsigned int exit_handler_count;
> >  const char *igt_interactive_debug;
> > +bool igt_skip_crc_compare;
> >  
> >  /* subtests helpers */
> >  static bool list_subtests = false;
> > @@ -285,6 +286,7 @@ enum {
> >   OPT_DESCRIPTION,
> >   OPT_DEBUG,
> >   OPT_INTERACTIVE_DEBUG,
> > + OPT_SKIP_CRC,
> >   OPT_HELP = 'h'
> >  };
> >  
> > @@ -552,6 +554,7 @@ static void print_usage(const char *help_str, bool output_on_stderr)
> >  		   "  --run-subtest <pattern>\n"
> >  		   "  --debug[=log-domain]\n"
> >  		   "  --interactive-debug[=domain]\n"
> > +		   "  --skip-crc-compare\n"
> >  		   "  --help-description\n"
> >  		   "  --help\n");
> >  	if (help_str)
> > @@ -668,6 +671,7 @@ static int common_init(int *argc, char **argv,
> >  		{"help-description", 0, 0, OPT_DESCRIPTION},
> >  		{"debug", optional_argument, 0, OPT_DEBUG},
> >  		{"interactive-debug", optional_argument, 0, OPT_INTERACTIVE_DEBUG},
> > +		{"skip-crc-compare", 0, 0, OPT_SKIP_CRC},
> >  		{"help", 0, 0, OPT_HELP},
> >  		{0, 0, 0, 0}
> >  	};
> > @@ -768,6 +772,9 @@ static int common_init(int *argc, char **argv,
> >  			print_test_description();
> >  			ret = -1;
> >  			goto out;
> > +		case OPT_SKIP_CRC:
> > +			igt_skip_crc_compare = true;
> > +			goto out;
> >  		case OPT_HELP:
> >  			print_usage(help_str, false);
> >  			ret = -1;
> > diff --git a/lib/igt_core.h b/lib/igt_core.h
> > index 47ffd9e7..f12fc5cb 100644
> > --- a/lib/igt_core.h
> > +++ b/lib/igt_core.h
> > @@ -843,6 +843,7 @@ bool igt_run_in_simulation(void);
> >  void igt_skip_on_simulation(void);
> >  
> >  extern const char *igt_interactive_debug;
> > +extern bool igt_skip_crc_compare;
> >  
> >  /**
> >   * igt_log_level:
> > diff --git a/lib/igt_debugfs.c b/lib/igt_debugfs.c
> > index 640c78e9..04d1648b 100644
> > --- a/lib/igt_debugfs.c
> > +++ b/lib/igt_debugfs.c
> > @@ -405,6 +405,12 @@ static bool igt_find_crc_mismatch(const igt_crc_t *a, const igt_crc_t *b,
> >   * assert that CRCs match, never that they are different. Otherwise there might
> >   * be random testcase failures when different screen contents end up with the
> >   * same CRC by chance.
> > + *
> > + * Passing --skip-crc-compare on the command line will force this function
> > + * to always pass, which can be useful in interactive debugging where you
> > + * might know the test will fail, but still want the test to keep going as if
> > + * it had succeeded so that you can see the on-screen behavior.
> > + *
> >   */
> >  void igt_assert_crc_equal(const igt_crc_t *a, const igt_crc_t *b)
> >  {
> > @@ -416,7 +422,7 @@ void igt_assert_crc_equal(const igt_crc_t *a, const igt_crc_t *b)
> >  		igt_debug("CRC mismatch at index %d: 0x%x != 0x%x\n", index,
> >  			  a->crc[index], b->crc[index]);
> >  
> > -	igt_assert(!mismatch);
> > +	igt_assert(!mismatch || igt_skip_crc_compare);
> 
> I think an igt_debug line here could be useful when we skip the assert.
> With that

Daniel, is the debug print just above not sufficient? Or would you like
a more explicit indication that we skipped the assert even though there
was mismatch?

I guess we could do something like:

if (mismatch)
 igt_debug("CRC mismatch%s at index %d: 0x%x != 0x%x\n",
           skip_crc_compare ? " (ignored)" : "",
	   index, a->crc[index], b->crc[index]);

to avoid spamming the log with two lines per mismatch?

I was just looking for this knob until I realized we never applied this
patch :/

> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> aka Ville convinced me.
> -Daniel
> 
> >  }
> >  
> >  /**
> > -- 
> > 2.14.5
> > 
> > _______________________________________________
> > igt-dev mailing list
> > igt-dev@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/igt-dev
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t 1/2] lib: Add --skip-crc-compare option

[Daniel Vetter]

On Thu, Jun 27, 2019 at 07:05:17PM +0300, Ville Syrjälä wrote:
> On Fri, Feb 22, 2019 at 03:32:50PM +0100, Daniel Vetter wrote:
> > On Wed, Feb 20, 2019 at 04:34:21PM -0800, Matt Roper wrote:
> > > When using --interactive-debug, it's sometimes desirable to ignore CRC
> > > mismatches and let the test proceed as if they passed so that the
> > > on-screen outcome can be inspected.  Let's add a debug option to allow
> > > this.
> > > 
> > > Cc: igt-dev@lists.freedesktop.org
> > > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> > > ---
> > >  lib/igt_core.c    | 7 +++++++
> > >  lib/igt_core.h    | 1 +
> > >  lib/igt_debugfs.c | 8 +++++++-
> > >  3 files changed, 15 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/lib/igt_core.c b/lib/igt_core.c
> > > index 71b05d3b..5523950f 100644
> > > --- a/lib/igt_core.c
> > > +++ b/lib/igt_core.c
> > > @@ -256,6 +256,7 @@
> > >  
> > >  static unsigned int exit_handler_count;
> > >  const char *igt_interactive_debug;
> > > +bool igt_skip_crc_compare;
> > >  
> > >  /* subtests helpers */
> > >  static bool list_subtests = false;
> > > @@ -285,6 +286,7 @@ enum {
> > >   OPT_DESCRIPTION,
> > >   OPT_DEBUG,
> > >   OPT_INTERACTIVE_DEBUG,
> > > + OPT_SKIP_CRC,
> > >   OPT_HELP = 'h'
> > >  };
> > >  
> > > @@ -552,6 +554,7 @@ static void print_usage(const char *help_str, bool output_on_stderr)
> > >  		   "  --run-subtest <pattern>\n"
> > >  		   "  --debug[=log-domain]\n"
> > >  		   "  --interactive-debug[=domain]\n"
> > > +		   "  --skip-crc-compare\n"
> > >  		   "  --help-description\n"
> > >  		   "  --help\n");
> > >  	if (help_str)
> > > @@ -668,6 +671,7 @@ static int common_init(int *argc, char **argv,
> > >  		{"help-description", 0, 0, OPT_DESCRIPTION},
> > >  		{"debug", optional_argument, 0, OPT_DEBUG},
> > >  		{"interactive-debug", optional_argument, 0, OPT_INTERACTIVE_DEBUG},
> > > +		{"skip-crc-compare", 0, 0, OPT_SKIP_CRC},
> > >  		{"help", 0, 0, OPT_HELP},
> > >  		{0, 0, 0, 0}
> > >  	};
> > > @@ -768,6 +772,9 @@ static int common_init(int *argc, char **argv,
> > >  			print_test_description();
> > >  			ret = -1;
> > >  			goto out;
> > > +		case OPT_SKIP_CRC:
> > > +			igt_skip_crc_compare = true;
> > > +			goto out;
> > >  		case OPT_HELP:
> > >  			print_usage(help_str, false);
> > >  			ret = -1;
> > > diff --git a/lib/igt_core.h b/lib/igt_core.h
> > > index 47ffd9e7..f12fc5cb 100644
> > > --- a/lib/igt_core.h
> > > +++ b/lib/igt_core.h
> > > @@ -843,6 +843,7 @@ bool igt_run_in_simulation(void);
> > >  void igt_skip_on_simulation(void);
> > >  
> > >  extern const char *igt_interactive_debug;
> > > +extern bool igt_skip_crc_compare;
> > >  
> > >  /**
> > >   * igt_log_level:
> > > diff --git a/lib/igt_debugfs.c b/lib/igt_debugfs.c
> > > index 640c78e9..04d1648b 100644
> > > --- a/lib/igt_debugfs.c
> > > +++ b/lib/igt_debugfs.c
> > > @@ -405,6 +405,12 @@ static bool igt_find_crc_mismatch(const igt_crc_t *a, const igt_crc_t *b,
> > >   * assert that CRCs match, never that they are different. Otherwise there might
> > >   * be random testcase failures when different screen contents end up with the
> > >   * same CRC by chance.
> > > + *
> > > + * Passing --skip-crc-compare on the command line will force this function
> > > + * to always pass, which can be useful in interactive debugging where you
> > > + * might know the test will fail, but still want the test to keep going as if
> > > + * it had succeeded so that you can see the on-screen behavior.
> > > + *
> > >   */
> > >  void igt_assert_crc_equal(const igt_crc_t *a, const igt_crc_t *b)
> > >  {
> > > @@ -416,7 +422,7 @@ void igt_assert_crc_equal(const igt_crc_t *a, const igt_crc_t *b)
> > >  		igt_debug("CRC mismatch at index %d: 0x%x != 0x%x\n", index,
> > >  			  a->crc[index], b->crc[index]);
> > >  
> > > -	igt_assert(!mismatch);
> > > +	igt_assert(!mismatch || igt_skip_crc_compare);
> > 
> > I think an igt_debug line here could be useful when we skip the assert.
> > With that
> 
> Daniel, is the debug print just above not sufficient? Or would you like
> a more explicit indication that we skipped the assert even though there
> was mismatch?
> 
> I guess we could do something like:
> 
> if (mismatch)
>  igt_debug("CRC mismatch%s at index %d: 0x%x != 0x%x\n",
>            skip_crc_compare ? " (ignored)" : "",
> 	   index, a->crc[index], b->crc[index]);
> 
> to avoid spamming the log with two lines per mismatch?
> 
> I was just looking for this knob until I realized we never applied this
> patch :/

Hm yeah I missed that debug line, I think that's good enough.
-Daniel

> 
> > 
> > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > 
> > aka Ville convinced me.
> > -Daniel
> > 
> > >  }
> > >  
> > >  /**
> > > -- 
> > > 2.14.5
> > > 
> > > _______________________________________________
> > > igt-dev mailing list
> > > igt-dev@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/igt-dev
> > 
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [igt-dev] [Intel-gfx] [PATCH i-g-t 1/2] lib: Add --skip-crc-compare option

[Daniel Vetter]

On Thu, Jun 27, 2019 at 07:05:17PM +0300, Ville Syrjälä wrote:
> On Fri, Feb 22, 2019 at 03:32:50PM +0100, Daniel Vetter wrote:
> > On Wed, Feb 20, 2019 at 04:34:21PM -0800, Matt Roper wrote:
> > > When using --interactive-debug, it's sometimes desirable to ignore CRC
> > > mismatches and let the test proceed as if they passed so that the
> > > on-screen outcome can be inspected.  Let's add a debug option to allow
> > > this.
> > > 
> > > Cc: igt-dev@lists.freedesktop.org
> > > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> > > ---
> > >  lib/igt_core.c    | 7 +++++++
> > >  lib/igt_core.h    | 1 +
> > >  lib/igt_debugfs.c | 8 +++++++-
> > >  3 files changed, 15 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/lib/igt_core.c b/lib/igt_core.c
> > > index 71b05d3b..5523950f 100644
> > > --- a/lib/igt_core.c
> > > +++ b/lib/igt_core.c
> > > @@ -256,6 +256,7 @@
> > >  
> > >  static unsigned int exit_handler_count;
> > >  const char *igt_interactive_debug;
> > > +bool igt_skip_crc_compare;
> > >  
> > >  /* subtests helpers */
> > >  static bool list_subtests = false;
> > > @@ -285,6 +286,7 @@ enum {
> > >   OPT_DESCRIPTION,
> > >   OPT_DEBUG,
> > >   OPT_INTERACTIVE_DEBUG,
> > > + OPT_SKIP_CRC,
> > >   OPT_HELP = 'h'
> > >  };
> > >  
> > > @@ -552,6 +554,7 @@ static void print_usage(const char *help_str, bool output_on_stderr)
> > >  		   "  --run-subtest <pattern>\n"
> > >  		   "  --debug[=log-domain]\n"
> > >  		   "  --interactive-debug[=domain]\n"
> > > +		   "  --skip-crc-compare\n"
> > >  		   "  --help-description\n"
> > >  		   "  --help\n");
> > >  	if (help_str)
> > > @@ -668,6 +671,7 @@ static int common_init(int *argc, char **argv,
> > >  		{"help-description", 0, 0, OPT_DESCRIPTION},
> > >  		{"debug", optional_argument, 0, OPT_DEBUG},
> > >  		{"interactive-debug", optional_argument, 0, OPT_INTERACTIVE_DEBUG},
> > > +		{"skip-crc-compare", 0, 0, OPT_SKIP_CRC},
> > >  		{"help", 0, 0, OPT_HELP},
> > >  		{0, 0, 0, 0}
> > >  	};
> > > @@ -768,6 +772,9 @@ static int common_init(int *argc, char **argv,
> > >  			print_test_description();
> > >  			ret = -1;
> > >  			goto out;
> > > +		case OPT_SKIP_CRC:
> > > +			igt_skip_crc_compare = true;
> > > +			goto out;
> > >  		case OPT_HELP:
> > >  			print_usage(help_str, false);
> > >  			ret = -1;
> > > diff --git a/lib/igt_core.h b/lib/igt_core.h
> > > index 47ffd9e7..f12fc5cb 100644
> > > --- a/lib/igt_core.h
> > > +++ b/lib/igt_core.h
> > > @@ -843,6 +843,7 @@ bool igt_run_in_simulation(void);
> > >  void igt_skip_on_simulation(void);
> > >  
> > >  extern const char *igt_interactive_debug;
> > > +extern bool igt_skip_crc_compare;
> > >  
> > >  /**
> > >   * igt_log_level:
> > > diff --git a/lib/igt_debugfs.c b/lib/igt_debugfs.c
> > > index 640c78e9..04d1648b 100644
> > > --- a/lib/igt_debugfs.c
> > > +++ b/lib/igt_debugfs.c
> > > @@ -405,6 +405,12 @@ static bool igt_find_crc_mismatch(const igt_crc_t *a, const igt_crc_t *b,
> > >   * assert that CRCs match, never that they are different. Otherwise there might
> > >   * be random testcase failures when different screen contents end up with the
> > >   * same CRC by chance.
> > > + *
> > > + * Passing --skip-crc-compare on the command line will force this function
> > > + * to always pass, which can be useful in interactive debugging where you
> > > + * might know the test will fail, but still want the test to keep going as if
> > > + * it had succeeded so that you can see the on-screen behavior.
> > > + *
> > >   */
> > >  void igt_assert_crc_equal(const igt_crc_t *a, const igt_crc_t *b)
> > >  {
> > > @@ -416,7 +422,7 @@ void igt_assert_crc_equal(const igt_crc_t *a, const igt_crc_t *b)
> > >  		igt_debug("CRC mismatch at index %d: 0x%x != 0x%x\n", index,
> > >  			  a->crc[index], b->crc[index]);
> > >  
> > > -	igt_assert(!mismatch);
> > > +	igt_assert(!mismatch || igt_skip_crc_compare);
> > 
> > I think an igt_debug line here could be useful when we skip the assert.
> > With that
> 
> Daniel, is the debug print just above not sufficient? Or would you like
> a more explicit indication that we skipped the assert even though there
> was mismatch?
> 
> I guess we could do something like:
> 
> if (mismatch)
>  igt_debug("CRC mismatch%s at index %d: 0x%x != 0x%x\n",
>            skip_crc_compare ? " (ignored)" : "",
> 	   index, a->crc[index], b->crc[index]);
> 
> to avoid spamming the log with two lines per mismatch?
> 
> I was just looking for this knob until I realized we never applied this
> patch :/

Hm yeah I missed that debug line, I think that's good enough.
-Daniel

> 
> > 
> > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > 
> > aka Ville convinced me.
> > -Daniel
> > 
> > >  }
> > >  
> > >  /**
> > > -- 
> > > 2.14.5
> > > 
> > > _______________________________________________
> > > igt-dev mailing list
> > > igt-dev@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/igt-dev
> > 
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t 1/2] lib: Add --skip-crc-compare option

[Ville Syrjälä]

On Wed, Jul 03, 2019 at 12:36:27PM +0200, Daniel Vetter wrote:
> On Thu, Jun 27, 2019 at 07:05:17PM +0300, Ville Syrjälä wrote:
> > On Fri, Feb 22, 2019 at 03:32:50PM +0100, Daniel Vetter wrote:
> > > On Wed, Feb 20, 2019 at 04:34:21PM -0800, Matt Roper wrote:
> > > > When using --interactive-debug, it's sometimes desirable to ignore CRC
> > > > mismatches and let the test proceed as if they passed so that the
> > > > on-screen outcome can be inspected.  Let's add a debug option to allow
> > > > this.
> > > > 
> > > > Cc: igt-dev@lists.freedesktop.org
> > > > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> > > > ---
> > > >  lib/igt_core.c    | 7 +++++++
> > > >  lib/igt_core.h    | 1 +
> > > >  lib/igt_debugfs.c | 8 +++++++-
> > > >  3 files changed, 15 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/lib/igt_core.c b/lib/igt_core.c
> > > > index 71b05d3b..5523950f 100644
> > > > --- a/lib/igt_core.c
> > > > +++ b/lib/igt_core.c
> > > > @@ -256,6 +256,7 @@
> > > >  
> > > >  static unsigned int exit_handler_count;
> > > >  const char *igt_interactive_debug;
> > > > +bool igt_skip_crc_compare;
> > > >  
> > > >  /* subtests helpers */
> > > >  static bool list_subtests = false;
> > > > @@ -285,6 +286,7 @@ enum {
> > > >   OPT_DESCRIPTION,
> > > >   OPT_DEBUG,
> > > >   OPT_INTERACTIVE_DEBUG,
> > > > + OPT_SKIP_CRC,
> > > >   OPT_HELP = 'h'
> > > >  };
> > > >  
> > > > @@ -552,6 +554,7 @@ static void print_usage(const char *help_str, bool output_on_stderr)
> > > >  		   "  --run-subtest <pattern>\n"
> > > >  		   "  --debug[=log-domain]\n"
> > > >  		   "  --interactive-debug[=domain]\n"
> > > > +		   "  --skip-crc-compare\n"
> > > >  		   "  --help-description\n"
> > > >  		   "  --help\n");
> > > >  	if (help_str)
> > > > @@ -668,6 +671,7 @@ static int common_init(int *argc, char **argv,
> > > >  		{"help-description", 0, 0, OPT_DESCRIPTION},
> > > >  		{"debug", optional_argument, 0, OPT_DEBUG},
> > > >  		{"interactive-debug", optional_argument, 0, OPT_INTERACTIVE_DEBUG},
> > > > +		{"skip-crc-compare", 0, 0, OPT_SKIP_CRC},
> > > >  		{"help", 0, 0, OPT_HELP},
> > > >  		{0, 0, 0, 0}
> > > >  	};
> > > > @@ -768,6 +772,9 @@ static int common_init(int *argc, char **argv,
> > > >  			print_test_description();
> > > >  			ret = -1;
> > > >  			goto out;
> > > > +		case OPT_SKIP_CRC:
> > > > +			igt_skip_crc_compare = true;
> > > > +			goto out;
> > > >  		case OPT_HELP:
> > > >  			print_usage(help_str, false);
> > > >  			ret = -1;
> > > > diff --git a/lib/igt_core.h b/lib/igt_core.h
> > > > index 47ffd9e7..f12fc5cb 100644
> > > > --- a/lib/igt_core.h
> > > > +++ b/lib/igt_core.h
> > > > @@ -843,6 +843,7 @@ bool igt_run_in_simulation(void);
> > > >  void igt_skip_on_simulation(void);
> > > >  
> > > >  extern const char *igt_interactive_debug;
> > > > +extern bool igt_skip_crc_compare;
> > > >  
> > > >  /**
> > > >   * igt_log_level:
> > > > diff --git a/lib/igt_debugfs.c b/lib/igt_debugfs.c
> > > > index 640c78e9..04d1648b 100644
> > > > --- a/lib/igt_debugfs.c
> > > > +++ b/lib/igt_debugfs.c
> > > > @@ -405,6 +405,12 @@ static bool igt_find_crc_mismatch(const igt_crc_t *a, const igt_crc_t *b,
> > > >   * assert that CRCs match, never that they are different. Otherwise there might
> > > >   * be random testcase failures when different screen contents end up with the
> > > >   * same CRC by chance.
> > > > + *
> > > > + * Passing --skip-crc-compare on the command line will force this function
> > > > + * to always pass, which can be useful in interactive debugging where you
> > > > + * might know the test will fail, but still want the test to keep going as if
> > > > + * it had succeeded so that you can see the on-screen behavior.
> > > > + *
> > > >   */
> > > >  void igt_assert_crc_equal(const igt_crc_t *a, const igt_crc_t *b)
> > > >  {
> > > > @@ -416,7 +422,7 @@ void igt_assert_crc_equal(const igt_crc_t *a, const igt_crc_t *b)
> > > >  		igt_debug("CRC mismatch at index %d: 0x%x != 0x%x\n", index,
> > > >  			  a->crc[index], b->crc[index]);
> > > >  
> > > > -	igt_assert(!mismatch);
> > > > +	igt_assert(!mismatch || igt_skip_crc_compare);
> > > 
> > > I think an igt_debug line here could be useful when we skip the assert.
> > > With that
> > 
> > Daniel, is the debug print just above not sufficient? Or would you like
> > a more explicit indication that we skipped the assert even though there
> > was mismatch?
> > 
> > I guess we could do something like:
> > 
> > if (mismatch)
> >  igt_debug("CRC mismatch%s at index %d: 0x%x != 0x%x\n",
> >            skip_crc_compare ? " (ignored)" : "",
> > 	   index, a->crc[index], b->crc[index]);
> > 
> > to avoid spamming the log with two lines per mismatch?
> > 
> > I was just looking for this knob until I realized we never applied this
> > patch :/
> 
> Hm yeah I missed that debug line, I think that's good enough.

Cool. Pimped the debug message as above and pushed.

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [igt-dev] [Intel-gfx] [PATCH i-g-t 1/2] lib: Add --skip-crc-compare option

[Ville Syrjälä]

On Wed, Jul 03, 2019 at 12:36:27PM +0200, Daniel Vetter wrote:
> On Thu, Jun 27, 2019 at 07:05:17PM +0300, Ville Syrjälä wrote:
> > On Fri, Feb 22, 2019 at 03:32:50PM +0100, Daniel Vetter wrote:
> > > On Wed, Feb 20, 2019 at 04:34:21PM -0800, Matt Roper wrote:
> > > > When using --interactive-debug, it's sometimes desirable to ignore CRC
> > > > mismatches and let the test proceed as if they passed so that the
> > > > on-screen outcome can be inspected.  Let's add a debug option to allow
> > > > this.
> > > > 
> > > > Cc: igt-dev@lists.freedesktop.org
> > > > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> > > > ---
> > > >  lib/igt_core.c    | 7 +++++++
> > > >  lib/igt_core.h    | 1 +
> > > >  lib/igt_debugfs.c | 8 +++++++-
> > > >  3 files changed, 15 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/lib/igt_core.c b/lib/igt_core.c
> > > > index 71b05d3b..5523950f 100644
> > > > --- a/lib/igt_core.c
> > > > +++ b/lib/igt_core.c
> > > > @@ -256,6 +256,7 @@
> > > >  
> > > >  static unsigned int exit_handler_count;
> > > >  const char *igt_interactive_debug;
> > > > +bool igt_skip_crc_compare;
> > > >  
> > > >  /* subtests helpers */
> > > >  static bool list_subtests = false;
> > > > @@ -285,6 +286,7 @@ enum {
> > > >   OPT_DESCRIPTION,
> > > >   OPT_DEBUG,
> > > >   OPT_INTERACTIVE_DEBUG,
> > > > + OPT_SKIP_CRC,
> > > >   OPT_HELP = 'h'
> > > >  };
> > > >  
> > > > @@ -552,6 +554,7 @@ static void print_usage(const char *help_str, bool output_on_stderr)
> > > >  		   "  --run-subtest <pattern>\n"
> > > >  		   "  --debug[=log-domain]\n"
> > > >  		   "  --interactive-debug[=domain]\n"
> > > > +		   "  --skip-crc-compare\n"
> > > >  		   "  --help-description\n"
> > > >  		   "  --help\n");
> > > >  	if (help_str)
> > > > @@ -668,6 +671,7 @@ static int common_init(int *argc, char **argv,
> > > >  		{"help-description", 0, 0, OPT_DESCRIPTION},
> > > >  		{"debug", optional_argument, 0, OPT_DEBUG},
> > > >  		{"interactive-debug", optional_argument, 0, OPT_INTERACTIVE_DEBUG},
> > > > +		{"skip-crc-compare", 0, 0, OPT_SKIP_CRC},
> > > >  		{"help", 0, 0, OPT_HELP},
> > > >  		{0, 0, 0, 0}
> > > >  	};
> > > > @@ -768,6 +772,9 @@ static int common_init(int *argc, char **argv,
> > > >  			print_test_description();
> > > >  			ret = -1;
> > > >  			goto out;
> > > > +		case OPT_SKIP_CRC:
> > > > +			igt_skip_crc_compare = true;
> > > > +			goto out;
> > > >  		case OPT_HELP:
> > > >  			print_usage(help_str, false);
> > > >  			ret = -1;
> > > > diff --git a/lib/igt_core.h b/lib/igt_core.h
> > > > index 47ffd9e7..f12fc5cb 100644
> > > > --- a/lib/igt_core.h
> > > > +++ b/lib/igt_core.h
> > > > @@ -843,6 +843,7 @@ bool igt_run_in_simulation(void);
> > > >  void igt_skip_on_simulation(void);
> > > >  
> > > >  extern const char *igt_interactive_debug;
> > > > +extern bool igt_skip_crc_compare;
> > > >  
> > > >  /**
> > > >   * igt_log_level:
> > > > diff --git a/lib/igt_debugfs.c b/lib/igt_debugfs.c
> > > > index 640c78e9..04d1648b 100644
> > > > --- a/lib/igt_debugfs.c
> > > > +++ b/lib/igt_debugfs.c
> > > > @@ -405,6 +405,12 @@ static bool igt_find_crc_mismatch(const igt_crc_t *a, const igt_crc_t *b,
> > > >   * assert that CRCs match, never that they are different. Otherwise there might
> > > >   * be random testcase failures when different screen contents end up with the
> > > >   * same CRC by chance.
> > > > + *
> > > > + * Passing --skip-crc-compare on the command line will force this function
> > > > + * to always pass, which can be useful in interactive debugging where you
> > > > + * might know the test will fail, but still want the test to keep going as if
> > > > + * it had succeeded so that you can see the on-screen behavior.
> > > > + *
> > > >   */
> > > >  void igt_assert_crc_equal(const igt_crc_t *a, const igt_crc_t *b)
> > > >  {
> > > > @@ -416,7 +422,7 @@ void igt_assert_crc_equal(const igt_crc_t *a, const igt_crc_t *b)
> > > >  		igt_debug("CRC mismatch at index %d: 0x%x != 0x%x\n", index,
> > > >  			  a->crc[index], b->crc[index]);
> > > >  
> > > > -	igt_assert(!mismatch);
> > > > +	igt_assert(!mismatch || igt_skip_crc_compare);
> > > 
> > > I think an igt_debug line here could be useful when we skip the assert.
> > > With that
> > 
> > Daniel, is the debug print just above not sufficient? Or would you like
> > a more explicit indication that we skipped the assert even though there
> > was mismatch?
> > 
> > I guess we could do something like:
> > 
> > if (mismatch)
> >  igt_debug("CRC mismatch%s at index %d: 0x%x != 0x%x\n",
> >            skip_crc_compare ? " (ignored)" : "",
> > 	   index, a->crc[index], b->crc[index]);
> > 
> > to avoid spamming the log with two lines per mismatch?
> > 
> > I was just looking for this knob until I realized we never applied this
> > patch :/
> 
> Hm yeah I missed that debug line, I think that's good enough.

Cool. Pimped the debug message as above and pushed.

-- 
Ville Syrjälä
Intel
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

✗ Fi.CI.CHECKPATCH: warning for CRTC background color (rev7)

[Patchwork]

== Series Details ==

Series: CRTC background color (rev7)
URL   : https://patchwork.freedesktop.org/series/50834/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
db7e455099da drm: Add CRTC background color property (v5)
-:239: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'shift' - possible side-effects?
#239: FILE: include/uapi/drm/drm_mode.h:931:
+#define DRM_ARGB_COMP(c, shift, numbits) \
+	(__u16)(((c) & 0xFFFFull << (shift)) >> ((shift) + 16 - (numbits)))

total: 0 errors, 0 warnings, 1 checks, 146 lines checked
cc27a4184d81 drm/i915/gen9+: Add support for pipe background color (v6)
7ba199d8432b drm/i915: Add background color hardware readout and state check
-:66: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'name' - possible side-effects?
#66: FILE: drivers/gpu/drm/i915/intel_display.c:11960:
+#define PIPE_CONF_CHECK_LLX_MASKED(name, mask) do { \
+	if ((current_config->name & mask) != (pipe_config->name & mask)) { \
+		pipe_config_err(adjust, __stringify(name), \
+			  "(expected 0x%016llx, found 0x%016llx)\n", \
+			  current_config->name & mask, \
+			  pipe_config->name & mask); \
+		ret = false; \
+	} \
+} while (0)

-:66: CHECK:MACRO_ARG_PRECEDENCE: Macro argument 'name' may be better as '(name)' to avoid precedence issues
#66: FILE: drivers/gpu/drm/i915/intel_display.c:11960:
+#define PIPE_CONF_CHECK_LLX_MASKED(name, mask) do { \
+	if ((current_config->name & mask) != (pipe_config->name & mask)) { \
+		pipe_config_err(adjust, __stringify(name), \
+			  "(expected 0x%016llx, found 0x%016llx)\n", \
+			  current_config->name & mask, \
+			  pipe_config->name & mask); \
+		ret = false; \
+	} \
+} while (0)

-:66: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'mask' - possible side-effects?
#66: FILE: drivers/gpu/drm/i915/intel_display.c:11960:
+#define PIPE_CONF_CHECK_LLX_MASKED(name, mask) do { \
+	if ((current_config->name & mask) != (pipe_config->name & mask)) { \
+		pipe_config_err(adjust, __stringify(name), \
+			  "(expected 0x%016llx, found 0x%016llx)\n", \
+			  current_config->name & mask, \
+			  pipe_config->name & mask); \
+		ret = false; \
+	} \
+} while (0)

-:66: CHECK:MACRO_ARG_PRECEDENCE: Macro argument 'mask' may be better as '(mask)' to avoid precedence issues
#66: FILE: drivers/gpu/drm/i915/intel_display.c:11960:
+#define PIPE_CONF_CHECK_LLX_MASKED(name, mask) do { \
+	if ((current_config->name & mask) != (pipe_config->name & mask)) { \
+		pipe_config_err(adjust, __stringify(name), \
+			  "(expected 0x%016llx, found 0x%016llx)\n", \
+			  current_config->name & mask, \
+			  pipe_config->name & mask); \
+		ret = false; \
+	} \
+} while (0)

total: 0 errors, 0 warnings, 4 checks, 62 lines checked

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

✗ Fi.CI.SPARSE: warning for CRTC background color (rev7)

[Patchwork]

== Series Details ==

Series: CRTC background color (rev7)
URL   : https://patchwork.freedesktop.org/series/50834/
State : warning

== Summary ==

$ dim sparse origin/drm-tip
Sparse version: v0.5.2
Commit: drm: Add CRTC background color property (v5)
Okay!

Commit: drm/i915/gen9+: Add support for pipe background color (v6)
Okay!

Commit: drm/i915: Add background color hardware readout and state check
+drivers/gpu/drm/i915/intel_display.c:12230:17: warning: constant 0x0000FFC0FFC0FFC0 is so big it is long
+drivers/gpu/drm/i915/intel_display.c:12230:17: warning: constant 0x0000FFC0FFC0FFC0 is so big it is long
+drivers/gpu/drm/i915/intel_display.c:12230:17: warning: constant 0x0000FFC0FFC0FFC0 is so big it is long
+drivers/gpu/drm/i915/intel_display.c:12230:17: warning: constant 0x0000FFC0FFC0FFC0 is so big it is long

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

✓ Fi.CI.BAT: success for CRTC background color (rev7)

[Patchwork]

== Series Details ==

Series: CRTC background color (rev7)
URL   : https://patchwork.freedesktop.org/series/50834/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_5645 -> Patchwork_12268
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/50834/revisions/7/mbox/

Known issues
------------

  Here are the changes found in Patchwork_12268 that come from known issues:

### IGT changes ###

#### Possible fixes ####

  * igt@kms_busy@basic-flip-b:
    - fi-gdg-551:         FAIL [fdo#103182] -> PASS

  * igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a:
    - fi-byt-clapper:     FAIL [fdo#103191] / [fdo#107362] -> PASS +1

  
#### Warnings ####

  * igt@i915_selftest@live_contexts:
    - fi-icl-u2:          DMESG-FAIL [fdo#108569] -> INCOMPLETE [fdo#108569]

  
  [fdo#103182]: https://bugs.freedesktop.org/show_bug.cgi?id=103182
  [fdo#103191]: https://bugs.freedesktop.org/show_bug.cgi?id=103191
  [fdo#107362]: https://bugs.freedesktop.org/show_bug.cgi?id=107362
  [fdo#108569]: https://bugs.freedesktop.org/show_bug.cgi?id=108569


Participating hosts (46 -> 40)
------------------------------

  Missing    (6): fi-kbl-soraka fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-blb-e6850 


Build changes
-------------

    * Linux: CI_DRM_5645 -> Patchwork_12268

  CI_DRM_5645: c67ed483692270fa9c8402d172d54b6c1335b7f7 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4846: 5aa3651ce2f5f562dad74f3e9d1ba47844e7a998 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_12268: 7ba199d8432b289f9fbfcf65fc6c6510d51908b2 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

7ba199d8432b drm/i915: Add background color hardware readout and state check
cc27a4184d81 drm/i915/gen9+: Add support for pipe background color (v6)
db7e455099da drm: Add CRTC background color property (v5)

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12268/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.IGT: success for CRTC background color (rev7)

[Patchwork]

== Series Details ==

Series: CRTC background color (rev7)
URL   : https://patchwork.freedesktop.org/series/50834/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_5645_full -> Patchwork_12268_full
====================================================

Summary
-------

  **WARNING**

  Minor unknown changes coming with Patchwork_12268_full need to be verified
  manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_12268_full, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_12268_full:

### IGT changes ###

#### Warnings ####

  * igt@runner@aborted:
    - shard-iclb:         ( 14 FAIL ) [fdo#109624] -> ( 17 FAIL ) [fdo#109624]

  
Known issues
------------

  Here are the changes found in Patchwork_12268_full that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_exec_schedule@preempt-bsd1:
    - shard-iclb:         NOTRUN -> SKIP [fdo#109276] +1

  * igt@i915_pm_lpsp@edp-native:
    - shard-iclb:         NOTRUN -> SKIP [fdo#109301]

  * igt@i915_pm_rpm@gem-execbuf:
    - shard-iclb:         PASS -> DMESG-WARN [fdo#107724] +1

  * igt@i915_pm_rps@min-max-config-loaded:
    - shard-iclb:         NOTRUN -> FAIL [fdo#102250]

  * igt@kms_busy@extended-modeset-hang-newfb-render-a:
    - shard-snb:          NOTRUN -> DMESG-WARN [fdo#107956]

  * igt@kms_busy@extended-modeset-hang-newfb-render-f:
    - shard-kbl:          NOTRUN -> SKIP [fdo#109271] / [fdo#109278] +2

  * igt@kms_busy@extended-modeset-hang-newfb-with-reset-render-a:
    - shard-kbl:          NOTRUN -> DMESG-WARN [fdo#107956]

  * igt@kms_chv_cursor_fail@pipe-c-64x64-top-edge:
    - shard-snb:          NOTRUN -> SKIP [fdo#109271] / [fdo#109278] +3

  * igt@kms_color@pipe-b-legacy-gamma:
    - shard-apl:          PASS -> FAIL [fdo#104782]

  * igt@kms_color@pipe-c-ctm-red-to-blue:
    - shard-iclb:         NOTRUN -> DMESG-WARN [fdo#109624] +1

  * igt@kms_flip@2x-busy-flip-interruptible:
    - shard-apl:          NOTRUN -> SKIP [fdo#109271] +2

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-cur-indfb-draw-pwrite:
    - shard-apl:          PASS -> FAIL [fdo#103167] +2

  * igt@kms_frontbuffer_tracking@fbc-2p-primscrn-spr-indfb-draw-blt:
    - shard-glk:          PASS -> FAIL [fdo#103167] +1

  * igt@kms_frontbuffer_tracking@psr-1p-offscren-pri-indfb-draw-mmap-wc:
    - shard-kbl:          NOTRUN -> SKIP [fdo#109271] +46

  * igt@kms_frontbuffer_tracking@psr-1p-primscrn-spr-indfb-fullscreen:
    - shard-iclb:         PASS -> FAIL [fdo#103167] +1

  * igt@kms_frontbuffer_tracking@psr-2p-scndscrn-indfb-msflip-blt:
    - shard-snb:          NOTRUN -> SKIP [fdo#109271] +77

  * igt@kms_frontbuffer_tracking@psr-2p-scndscrn-indfb-plflip-blt:
    - shard-iclb:         NOTRUN -> SKIP [fdo#109280]

  * igt@kms_plane_multiple@atomic-pipe-c-tiling-none:
    - shard-glk:          PASS -> FAIL [fdo#103166]
    - shard-apl:          PASS -> FAIL [fdo#103166] +1

  * igt@kms_rotation_crc@multiplane-rotation-cropping-bottom:
    - shard-kbl:          PASS -> DMESG-FAIL [fdo#105763]

  * igt@kms_setmode@basic:
    - shard-kbl:          PASS -> FAIL [fdo#99912]

  * igt@kms_vblank@pipe-a-ts-continuation-dpms-rpm:
    - shard-apl:          PASS -> FAIL [fdo#104894]

  * igt@kms_vblank@pipe-c-ts-continuation-dpms-suspend:
    - shard-iclb:         PASS -> INCOMPLETE [fdo#107713]

  
#### Possible fixes ####

  * igt@i915_pm_rpm@gem-evict-pwrite:
    - shard-iclb:         INCOMPLETE [fdo#108840] -> PASS

  * igt@i915_pm_rpm@modeset-lpsp-stress:
    - shard-iclb:         DMESG-WARN [fdo#107724] -> PASS

  * igt@kms_atomic_transition@1x-modeset-transitions-nonblocking-fencing:
    - shard-apl:          FAIL [fdo#109660] -> PASS

  * igt@kms_cursor_crc@cursor-64x64-dpms:
    - shard-apl:          FAIL [fdo#103232] -> PASS +1

  * igt@kms_cursor_crc@cursor-64x64-suspend:
    - shard-apl:          FAIL [fdo#103191] / [fdo#103232] -> PASS

  * igt@kms_flip@2x-modeset-vs-vblank-race:
    - shard-glk:          FAIL [fdo#103060] -> PASS

  * igt@kms_flip@flip-vs-suspend:
    - shard-iclb:         INCOMPLETE [fdo#109507] -> PASS

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-draw-render:
    - shard-apl:          FAIL [fdo#103167] -> PASS

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-move:
    - shard-glk:          FAIL [fdo#103167] -> PASS +5

  * igt@kms_frontbuffer_tracking@psr-1p-primscrn-spr-indfb-move:
    - shard-iclb:         FAIL [fdo#103167] -> PASS

  * igt@kms_plane@plane-position-covered-pipe-b-planes:
    - shard-iclb:         FAIL [fdo#103166] -> PASS

  * igt@kms_rotation_crc@multiplane-rotation-cropping-top:
    - shard-kbl:          FAIL [fdo#109016] -> PASS

  * igt@kms_setmode@basic:
    - shard-apl:          FAIL [fdo#99912] -> PASS
    - shard-hsw:          FAIL [fdo#99912] -> PASS

  * igt@kms_vblank@pipe-c-ts-continuation-modeset-hang:
    - shard-apl:          FAIL [fdo#104894] -> PASS

  
#### Warnings ####

  * igt@kms_cursor_crc@cursor-128x128-suspend:
    - shard-iclb:         FAIL [fdo#103232] -> INCOMPLETE [fdo#107713]

  * igt@runner@aborted:
    - shard-glk:          FAIL [fdo#109373] / [k.org#202321] -> ( 2 FAIL ) [fdo#109373] / [k.org#202321]

  * igt@syncobj_wait@multi-wait-all-for-submit-submitted-signaled:
    - shard-snb:          DMESG-WARN [fdo#107469] -> INCOMPLETE [fdo#105411]

  
  [fdo#102250]: https://bugs.freedesktop.org/show_bug.cgi?id=102250
  [fdo#103060]: https://bugs.freedesktop.org/show_bug.cgi?id=103060
  [fdo#103166]: https://bugs.freedesktop.org/show_bug.cgi?id=103166
  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#103191]: https://bugs.freedesktop.org/show_bug.cgi?id=103191
  [fdo#103232]: https://bugs.freedesktop.org/show_bug.cgi?id=103232
  [fdo#104782]: https://bugs.freedesktop.org/show_bug.cgi?id=104782
  [fdo#104894]: https://bugs.freedesktop.org/show_bug.cgi?id=104894
  [fdo#105411]: https://bugs.freedesktop.org/show_bug.cgi?id=105411
  [fdo#105763]: https://bugs.freedesktop.org/show_bug.cgi?id=105763
  [fdo#107469]: https://bugs.freedesktop.org/show_bug.cgi?id=107469
  [fdo#107713]: https://bugs.freedesktop.org/show_bug.cgi?id=107713
  [fdo#107724]: https://bugs.freedesktop.org/show_bug.cgi?id=107724
  [fdo#107956]: https://bugs.freedesktop.org/show_bug.cgi?id=107956
  [fdo#108840]: https://bugs.freedesktop.org/show_bug.cgi?id=108840
  [fdo#109016]: https://bugs.freedesktop.org/show_bug.cgi?id=109016
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109276]: https://bugs.freedesktop.org/show_bug.cgi?id=109276
  [fdo#109278]: https://bugs.freedesktop.org/show_bug.cgi?id=109278
  [fdo#109280]: https://bugs.freedesktop.org/show_bug.cgi?id=109280
  [fdo#109301]: https://bugs.freedesktop.org/show_bug.cgi?id=109301
  [fdo#109373]: https://bugs.freedesktop.org/show_bug.cgi?id=109373
  [fdo#109507]: https://bugs.freedesktop.org/show_bug.cgi?id=109507
  [fdo#109624]: https://bugs.freedesktop.org/show_bug.cgi?id=109624
  [fdo#109660]: https://bugs.freedesktop.org/show_bug.cgi?id=109660
  [fdo#99912]: https://bugs.freedesktop.org/show_bug.cgi?id=99912
  [k.org#202321]: https://bugzilla.kernel.org/show_bug.cgi?id=202321


Participating hosts (7 -> 6)
------------------------------

  Missing    (1): shard-skl 


Build changes
-------------

    * Linux: CI_DRM_5645 -> Patchwork_12268

  CI_DRM_5645: c67ed483692270fa9c8402d172d54b6c1335b7f7 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4846: 5aa3651ce2f5f562dad74f3e9d1ba47844e7a998 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_12268: 7ba199d8432b289f9fbfcf65fc6c6510d51908b2 @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12268/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx