[PATCH v4.1 0/3] CRTC background color

[PATCH v4.1 0/3] CRTC background color

[Matt Roper]

Previous patch series was here:
  https://lists.freedesktop.org/archives/dri-devel/2018-December/201949.html

I'm told the ChromeOS userspace code to make use of the background color
has been reviewed and is ready for use:
 * https://chromium-review.googlesource.com/c/chromium/src/+/1278858
 * https://chromium-review.googlesource.com/c/chromium/src/+/1278858

So I think ABI-wise we've met the userspace consumer requirements to
upstream this; we just need to get reviews on the two i915-specific
patches and a clean CI report.

v4.1 is identical to v4 aside from a rebase onto the latest drm-tip.


Matt Roper (3):
  drm/i915: Force background color to black for gen9+ (v2)
  drm: Add CRTC background color property (v4)
  drm/i915/gen9+: Add support for pipe background color (v4)

 drivers/gpu/drm/drm_atomic_uapi.c    |  4 ++++
 drivers/gpu/drm/drm_blend.c          | 27 +++++++++++++++++++---
 drivers/gpu/drm/drm_mode_config.c    |  6 +++++
 drivers/gpu/drm/i915/i915_debugfs.c  |  9 ++++++++
 drivers/gpu/drm/i915/i915_reg.h      |  6 +++++
 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, 138 insertions(+), 3 deletions(-)

-- 
2.14.5

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

[PATCH v4.1 1/3] drm/i915: Force background color to black for gen9+ (v2)

[Matt Roper]

We don't yet allow userspace to control the CRTC background color, but
we should manually program the color to black to ensure the BIOS didn't
leave us with some other color.  We should also set the pipe gamma and
pipe CSC bits so that the background color goes through the same color
management transformations that a plane with black pixels would.

v2: Rename register to SKL_BOTTOM_COLOR to more closely follow
    bspec naming.  (Ville)

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h      |  6 ++++++
 drivers/gpu/drm/i915/intel_display.c | 19 +++++++++++++++++++
 2 files changed, 25 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 03adcf3838de..a64deeb4e517 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -5710,6 +5710,12 @@ enum {
 #define   PIPEMISC_DITHER_TYPE_SP	(0 << 2)
 #define PIPEMISC(pipe)			_MMIO_PIPE2(pipe, _PIPE_MISC_A)
 
+/* Skylake+ pipe bottom (background) color */
+#define _SKL_BOTTOM_COLOR_A		0x70034
+#define   SKL_BOTTOM_COLOR_GAMMA_ENABLE	(1 << 31)
+#define   SKL_BOTTOM_COLOR_CSC_ENABLE	(1 << 30)
+#define SKL_BOTTOM_COLOR(pipe)		_MMIO_PIPE2(pipe, _SKL_BOTTOM_COLOR_A)
+
 #define VLV_DPFLIPSTAT				_MMIO(VLV_DISPLAY_BASE + 0x70028)
 #define   PIPEB_LINE_COMPARE_INT_EN		(1 << 29)
 #define   PIPEB_HLINE_INT_EN			(1 << 28)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 539d8915b55f..a025efb1d7c6 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3930,6 +3930,16 @@ static void intel_update_pipe_config(const struct intel_crtc_state *old_crtc_sta
 		else if (old_crtc_state->pch_pfit.enabled)
 			ironlake_pfit_disable(old_crtc_state);
 	}
+
+	/*
+	 * We don't (yet) allow userspace to control the pipe background color,
+	 * so force it to black, but apply pipe gamma and CSC so that its
+	 * handling will match how we program our planes.
+	 */
+	if (INTEL_GEN(dev_priv) >= 9)
+		I915_WRITE(SKL_BOTTOM_COLOR(crtc->pipe),
+			   SKL_BOTTOM_COLOR_GAMMA_ENABLE |
+			   SKL_BOTTOM_COLOR_CSC_ENABLE);
 }
 
 static void intel_fdi_normal_train(struct intel_crtc *crtc)
@@ -15488,6 +15498,15 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc,
 			    plane->base.type != DRM_PLANE_TYPE_PRIMARY)
 				intel_plane_disable_noatomic(crtc, plane);
 		}
+
+		/*
+		 * Disable any background color set by the BIOS, but enable the
+		 * gamma and CSC to match how we program our planes.
+		 */
+		if (INTEL_GEN(dev_priv) >= 9)
+			I915_WRITE(SKL_BOTTOM_COLOR(crtc->pipe),
+				   SKL_BOTTOM_COLOR_GAMMA_ENABLE |
+				   SKL_BOTTOM_COLOR_CSC_ENABLE);
 	}
 
 	/* Adjust the state of the output pipe according to whether we
-- 
2.14.5

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

[PATCH v4.1 2/3] drm: Add CRTC background color property (v4)

[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)

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       | 27 ++++++++++++++++++++++++---
 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, 80 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
index 9a1f41adfc67..d569e20e34e3 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..d451ac9e1d6d 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,11 @@ int drm_plane_create_blend_mode_property(struct drm_plane *plane,
 	return 0;
 }
 EXPORT_SYMBOL(drm_plane_create_blend_mode_property);
+
+void drm_crtc_add_bgcolor_property(struct drm_crtc *crtc)
+{
+	drm_object_attach_property(&crtc->base,
+				   crtc->dev->mode_config.bgcolor_property,
+				   drm_argb(16, 0xffff, 0, 0, 0));
+}
+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 1e6cb885994d..0463d3f4ae59 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

[PATCH v4.1 3/3] drm/i915/gen9+: Add support for pipe background color (v4)

[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.

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>
---
 drivers/gpu/drm/i915/i915_debugfs.c  |  9 +++++++
 drivers/gpu/drm/i915/intel_display.c | 46 +++++++++++++++++++++++++++---------
 2 files changed, 44 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index fa2c226fc779..8b07dd05c54e 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -3092,6 +3092,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) {
+			uint64_t 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_display.c b/drivers/gpu/drm/i915/intel_display.c
index a025efb1d7c6..bc78743e1411 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3896,6 +3896,28 @@ void intel_finish_reset(struct drm_i915_private *dev_priv)
 	clear_bit(I915_RESET_MODESET, &dev_priv->gpu_error.flags);
 }
 
+static void
+skl_update_background_color(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);
+	uint64_t propval = crtc_state->base.bgcolor;
+	uint32_t tmp;
+
+	/* Hardware is programmed with 10 bits of precision */
+	tmp = DRM_ARGB_RED(propval, 10) << 20
+	    | DRM_ARGB_GREEN(propval, 10) << 10
+	    | DRM_ARGB_BLUE(propval, 10);
+
+	/*
+	 * Set CSC and gamma for bottom color to ensure background pixels
+	 * receive the same color transformations as plane content.
+	 */
+	tmp |= SKL_BOTTOM_COLOR_CSC_ENABLE | SKL_BOTTOM_COLOR_GAMMA_ENABLE;
+
+	I915_WRITE(SKL_BOTTOM_COLOR(crtc->pipe), tmp);
+}
+
 static void intel_update_pipe_config(const struct intel_crtc_state *old_crtc_state,
 				     const struct intel_crtc_state *new_crtc_state)
 {
@@ -3931,15 +3953,8 @@ static void intel_update_pipe_config(const struct intel_crtc_state *old_crtc_sta
 			ironlake_pfit_disable(old_crtc_state);
 	}
 
-	/*
-	 * We don't (yet) allow userspace to control the pipe background color,
-	 * so force it to black, but apply pipe gamma and CSC so that its
-	 * handling will match how we program our planes.
-	 */
 	if (INTEL_GEN(dev_priv) >= 9)
-		I915_WRITE(SKL_BOTTOM_COLOR(crtc->pipe),
-			   SKL_BOTTOM_COLOR_GAMMA_ENABLE |
-			   SKL_BOTTOM_COLOR_CSC_ENABLE);
+		skl_update_background_color(new_crtc_state);
 }
 
 static void intel_fdi_normal_train(struct intel_crtc *crtc)
@@ -11042,6 +11057,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);
 
@@ -11069,6 +11086,9 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc,
 		crtc_state->planes_changed = true;
 	}
 
+	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);
@@ -14238,6 +14258,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:
@@ -15478,6 +15501,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);
@@ -15504,9 +15530,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc,
 		 * gamma and CSC to match how we program our planes.
 		 */
 		if (INTEL_GEN(dev_priv) >= 9)
-			I915_WRITE(SKL_BOTTOM_COLOR(crtc->pipe),
-				   SKL_BOTTOM_COLOR_GAMMA_ENABLE |
-				   SKL_BOTTOM_COLOR_CSC_ENABLE);
+			skl_update_background_color(crtc_state);
 	}
 
 	/* Adjust the state of the output pipe according to whether we
-- 
2.14.5

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

Re: [PATCH v4.1 0/3] CRTC background color

[Matt Roper]

On Wed, Jan 30, 2019 at 10:51:19AM -0800, Matt Roper wrote:
> Previous patch series was here:
>   https://lists.freedesktop.org/archives/dri-devel/2018-December/201949.html
> 
> I'm told the ChromeOS userspace code to make use of the background color
> has been reviewed and is ready for use:
>  * https://chromium-review.googlesource.com/c/chromium/src/+/1278858
>  * https://chromium-review.googlesource.com/c/chromium/src/+/1278858

Woops, the second link here should have been to

  https://chromium-review.googlesource.com/c/chromiumos/platform/drm-tests/+/1241436

which I believe is some unit tests to go along with the main userspace
code.


Matt

> 
> So I think ABI-wise we've met the userspace consumer requirements to
> upstream this; we just need to get reviews on the two i915-specific
> patches and a clean CI report.
> 
> v4.1 is identical to v4 aside from a rebase onto the latest drm-tip.
> 
> 
> Matt Roper (3):
>   drm/i915: Force background color to black for gen9+ (v2)
>   drm: Add CRTC background color property (v4)
>   drm/i915/gen9+: Add support for pipe background color (v4)
> 
>  drivers/gpu/drm/drm_atomic_uapi.c    |  4 ++++
>  drivers/gpu/drm/drm_blend.c          | 27 +++++++++++++++++++---
>  drivers/gpu/drm/drm_mode_config.c    |  6 +++++
>  drivers/gpu/drm/i915/i915_debugfs.c  |  9 ++++++++
>  drivers/gpu/drm/i915/i915_reg.h      |  6 +++++
>  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, 138 insertions(+), 3 deletions(-)
> 
> -- 
> 2.14.5
> 

-- 
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

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

[Patchwork]

== Series Details ==

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

== Summary ==

$ dim checkpatch origin/drm-tip
a9fc702b09d5 drm/i915: Force background color to black for gen9+ (v2)
3f404502f7a1 drm: Add CRTC background color property (v4)
-:219: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'shift' - possible side-effects?
#219: 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, 132 lines checked
f96a4d9be129 drm/i915/gen9+: Add support for pipe background color (v4)
-:45: CHECK:PREFER_KERNEL_TYPES: Prefer kernel type 'u64' over 'uint64_t'
#45: FILE: drivers/gpu/drm/i915/i915_debugfs.c:3096:
+			uint64_t background = pipe_config->base.bgcolor;

-:69: CHECK:PREFER_KERNEL_TYPES: Prefer kernel type 'u64' over 'uint64_t'
#69: FILE: drivers/gpu/drm/i915/intel_display.c:3904:
+	uint64_t propval = crtc_state->base.bgcolor;

-:70: CHECK:PREFER_KERNEL_TYPES: Prefer kernel type 'u32' over 'uint32_t'
#70: FILE: drivers/gpu/drm/i915/intel_display.c:3905:
+	uint32_t tmp;

total: 0 errors, 0 warnings, 3 checks, 104 lines checked

_______________________________________________
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 (rev5)

[Patchwork]

== Series Details ==

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

== Summary ==

CI Bug Log - changes from CI_DRM_5512 -> Patchwork_12096
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

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

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

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

### IGT changes ###

#### Issues hit ####

  * igt@amdgpu/amd_basic@userptr:
    - fi-kbl-8809g:       PASS -> DMESG-WARN [fdo#108965]

  * igt@gem_exec_suspend@basic-s3:
    - fi-blb-e6850:       PASS -> INCOMPLETE [fdo#107718]

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

  * igt@kms_flip@basic-flip-vs-modeset:
    - fi-skl-6700hq:      PASS -> DMESG-WARN [fdo#105998]

  * igt@kms_frontbuffer_tracking@basic:
    - fi-hsw-peppy:       NOTRUN -> DMESG-FAIL [fdo#102614]

  
#### Possible fixes ####

  * igt@kms_frontbuffer_tracking@basic:
    - {fi-icl-u2}:        FAIL [fdo#103167] -> PASS

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#102614]: https://bugs.freedesktop.org/show_bug.cgi?id=102614
  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#103182]: https://bugs.freedesktop.org/show_bug.cgi?id=103182
  [fdo#105998]: https://bugs.freedesktop.org/show_bug.cgi?id=105998
  [fdo#107718]: https://bugs.freedesktop.org/show_bug.cgi?id=107718
  [fdo#108965]: https://bugs.freedesktop.org/show_bug.cgi?id=108965
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109278]: https://bugs.freedesktop.org/show_bug.cgi?id=109278


Participating hosts (42 -> 41)
------------------------------

  Additional (3): fi-hsw-peppy fi-gdg-551 fi-pnv-d510 
  Missing    (4): fi-kbl-soraka fi-ilk-m540 fi-byt-squawks fi-bsw-cyan 


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

    * Linux: CI_DRM_5512 -> Patchwork_12096

  CI_DRM_5512: bf1da2552de642ebce4ea3f91a3259ef819b13ce @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4801: 6f6bacf12759fb319ade3ba37861ae711f8a5cd9 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_12096: f96a4d9be129b2de9bab006d01adb14dbd80000f @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

f96a4d9be129 drm/i915/gen9+: Add support for pipe background color (v4)
3f404502f7a1 drm: Add CRTC background color property (v4)
a9fc702b09d5 drm/i915: Force background color to black for gen9+ (v2)

== Logs ==

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

Re: [PATCH v4.1 0/3] CRTC background color

[Daniel Vetter]

On Wed, Jan 30, 2019 at 10:56:26AM -0800, Matt Roper wrote:
> On Wed, Jan 30, 2019 at 10:51:19AM -0800, Matt Roper wrote:
> > Previous patch series was here:
> >   https://lists.freedesktop.org/archives/dri-devel/2018-December/201949.html
> > 
> > I'm told the ChromeOS userspace code to make use of the background color
> > has been reviewed and is ready for use:
> >  * https://chromium-review.googlesource.com/c/chromium/src/+/1278858
> >  * https://chromium-review.googlesource.com/c/chromium/src/+/1278858
> 
> Woops, the second link here should have been to
> 
>   https://chromium-review.googlesource.com/c/chromiumos/platform/drm-tests/+/1241436
> 
> which I believe is some unit tests to go along with the main userspace
> code.

Do we have this as igts too?
-Daniel

> 
> 
> Matt
> 
> > 
> > So I think ABI-wise we've met the userspace consumer requirements to
> > upstream this; we just need to get reviews on the two i915-specific
> > patches and a clean CI report.
> > 
> > v4.1 is identical to v4 aside from a rebase onto the latest drm-tip.
> > 
> > 
> > Matt Roper (3):
> >   drm/i915: Force background color to black for gen9+ (v2)
> >   drm: Add CRTC background color property (v4)
> >   drm/i915/gen9+: Add support for pipe background color (v4)
> > 
> >  drivers/gpu/drm/drm_atomic_uapi.c    |  4 ++++
> >  drivers/gpu/drm/drm_blend.c          | 27 +++++++++++++++++++---
> >  drivers/gpu/drm/drm_mode_config.c    |  6 +++++
> >  drivers/gpu/drm/i915/i915_debugfs.c  |  9 ++++++++
> >  drivers/gpu/drm/i915/i915_reg.h      |  6 +++++
> >  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, 138 insertions(+), 3 deletions(-)
> > 
> > -- 
> > 2.14.5
> > 
> 
> -- 
> 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

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

[Ville Syrjälä]

On Wed, Jan 30, 2019 at 10:51:21AM -0800, Matt Roper wrote:
> 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)
> 
> 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       | 27 ++++++++++++++++++++++++---
>  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, 80 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> index 9a1f41adfc67..d569e20e34e3 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..d451ac9e1d6d 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).

What would the alpha<1.0 do in that case? Blend the writeback output
with what's already there?

>   */
>  
>  /**
> @@ -593,3 +606,11 @@ int drm_plane_create_blend_mode_property(struct drm_plane *plane,
>  	return 0;
>  }
>  EXPORT_SYMBOL(drm_plane_create_blend_mode_property);
> +
> +void drm_crtc_add_bgcolor_property(struct drm_crtc *crtc)
> +{
> +	drm_object_attach_property(&crtc->base,
> +				   crtc->dev->mode_config.bgcolor_property,
> +				   drm_argb(16, 0xffff, 0, 0, 0));

if (crtc->state)
 crtc->state->bg = default value; ?

> +}
> +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 1e6cb885994d..0463d3f4ae59 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

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

Re: [PATCH v4.1 1/3] drm/i915: Force background color to black for gen9+ (v2)

[Ville Syrjälä]

On Wed, Jan 30, 2019 at 10:51:20AM -0800, Matt Roper wrote:
> We don't yet allow userspace to control the CRTC background color, but
> we should manually program the color to black to ensure the BIOS didn't
> leave us with some other color.  We should also set the pipe gamma and
> pipe CSC bits so that the background color goes through the same color
> management transformations that a plane with black pixels would.
> 
> v2: Rename register to SKL_BOTTOM_COLOR to more closely follow
>     bspec naming.  (Ville)
> 
> 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_reg.h      |  6 ++++++
>  drivers/gpu/drm/i915/intel_display.c | 19 +++++++++++++++++++
>  2 files changed, 25 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 03adcf3838de..a64deeb4e517 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -5710,6 +5710,12 @@ enum {
>  #define   PIPEMISC_DITHER_TYPE_SP	(0 << 2)
>  #define PIPEMISC(pipe)			_MMIO_PIPE2(pipe, _PIPE_MISC_A)
>  
> +/* Skylake+ pipe bottom (background) color */
> +#define _SKL_BOTTOM_COLOR_A		0x70034
> +#define   SKL_BOTTOM_COLOR_GAMMA_ENABLE	(1 << 31)
> +#define   SKL_BOTTOM_COLOR_CSC_ENABLE	(1 << 30)
> +#define SKL_BOTTOM_COLOR(pipe)		_MMIO_PIPE2(pipe, _SKL_BOTTOM_COLOR_A)
> +
>  #define VLV_DPFLIPSTAT				_MMIO(VLV_DISPLAY_BASE + 0x70028)
>  #define   PIPEB_LINE_COMPARE_INT_EN		(1 << 29)
>  #define   PIPEB_HLINE_INT_EN			(1 << 28)
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 539d8915b55f..a025efb1d7c6 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3930,6 +3930,16 @@ static void intel_update_pipe_config(const struct intel_crtc_state *old_crtc_sta
>  		else if (old_crtc_state->pch_pfit.enabled)
>  			ironlake_pfit_disable(old_crtc_state);
>  	}
> +
> +	/*
> +	 * We don't (yet) allow userspace to control the pipe background color,
> +	 * so force it to black, but apply pipe gamma and CSC so that its
> +	 * handling will match how we program our planes.
> +	 */
> +	if (INTEL_GEN(dev_priv) >= 9)
> +		I915_WRITE(SKL_BOTTOM_COLOR(crtc->pipe),
> +			   SKL_BOTTOM_COLOR_GAMMA_ENABLE |
> +			   SKL_BOTTOM_COLOR_CSC_ENABLE);
>  }
>  
>  static void intel_fdi_normal_train(struct intel_crtc *crtc)
> @@ -15488,6 +15498,15 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc,
>  			    plane->base.type != DRM_PLANE_TYPE_PRIMARY)
>  				intel_plane_disable_noatomic(crtc, plane);
>  		}
> +
> +		/*
> +		 * Disable any background color set by the BIOS, but enable the
> +		 * gamma and CSC to match how we program our planes.
> +		 */
> +		if (INTEL_GEN(dev_priv) >= 9)
> +			I915_WRITE(SKL_BOTTOM_COLOR(crtc->pipe),
> +				   SKL_BOTTOM_COLOR_GAMMA_ENABLE |
> +				   SKL_BOTTOM_COLOR_CSC_ENABLE);
>  	}
>  
>  	/* Adjust the state of the output pipe according to whether we
> -- 
> 2.14.5

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

Re: [PATCH v4.1 3/3] drm/i915/gen9+: Add support for pipe background color (v4)

[Ville Syrjälä]

On Wed, Jan 30, 2019 at 10:51:22AM -0800, Matt Roper wrote:
> 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.
> 
> 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>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c  |  9 +++++++
>  drivers/gpu/drm/i915/intel_display.c | 46 +++++++++++++++++++++++++++---------
>  2 files changed, 44 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index fa2c226fc779..8b07dd05c54e 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -3092,6 +3092,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) {
> +			uint64_t 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_display.c b/drivers/gpu/drm/i915/intel_display.c
> index a025efb1d7c6..bc78743e1411 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3896,6 +3896,28 @@ void intel_finish_reset(struct drm_i915_private *dev_priv)
>  	clear_bit(I915_RESET_MODESET, &dev_priv->gpu_error.flags);
>  }
>  
> +static void
> +skl_update_background_color(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);
> +	uint64_t propval = crtc_state->base.bgcolor;
> +	uint32_t tmp;
> +
> +	/* Hardware is programmed with 10 bits of precision */
> +	tmp = DRM_ARGB_RED(propval, 10) << 20
> +	    | DRM_ARGB_GREEN(propval, 10) << 10
> +	    | DRM_ARGB_BLUE(propval, 10);
> +
> +	/*
> +	 * Set CSC and gamma for bottom color to ensure background pixels
> +	 * receive the same color transformations as plane content.
> +	 */
> +	tmp |= SKL_BOTTOM_COLOR_CSC_ENABLE | SKL_BOTTOM_COLOR_GAMMA_ENABLE;
> +
> +	I915_WRITE(SKL_BOTTOM_COLOR(crtc->pipe), tmp);
> +}

Readout + state check would be nice too.

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

> +
>  static void intel_update_pipe_config(const struct intel_crtc_state *old_crtc_state,
>  				     const struct intel_crtc_state *new_crtc_state)
>  {
> @@ -3931,15 +3953,8 @@ static void intel_update_pipe_config(const struct intel_crtc_state *old_crtc_sta
>  			ironlake_pfit_disable(old_crtc_state);
>  	}
>  
> -	/*
> -	 * We don't (yet) allow userspace to control the pipe background color,
> -	 * so force it to black, but apply pipe gamma and CSC so that its
> -	 * handling will match how we program our planes.
> -	 */
>  	if (INTEL_GEN(dev_priv) >= 9)
> -		I915_WRITE(SKL_BOTTOM_COLOR(crtc->pipe),
> -			   SKL_BOTTOM_COLOR_GAMMA_ENABLE |
> -			   SKL_BOTTOM_COLOR_CSC_ENABLE);
> +		skl_update_background_color(new_crtc_state);
>  }
>  
>  static void intel_fdi_normal_train(struct intel_crtc *crtc)
> @@ -11042,6 +11057,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);
>  
> @@ -11069,6 +11086,9 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc,
>  		crtc_state->planes_changed = true;
>  	}
>  
> +	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);
> @@ -14238,6 +14258,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:
> @@ -15478,6 +15501,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);
> @@ -15504,9 +15530,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc,
>  		 * gamma and CSC to match how we program our planes.
>  		 */
>  		if (INTEL_GEN(dev_priv) >= 9)
> -			I915_WRITE(SKL_BOTTOM_COLOR(crtc->pipe),
> -				   SKL_BOTTOM_COLOR_GAMMA_ENABLE |
> -				   SKL_BOTTOM_COLOR_CSC_ENABLE);
> +			skl_update_background_color(crtc_state);
>  	}
>  
>  	/* Adjust the state of the output pipe according to whether we
> -- 
> 2.14.5

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

Re: [Intel-gfx] [PATCH v4.1 0/3] CRTC background color

[Matt Roper]

On Wed, Jan 30, 2019 at 09:57:10PM +0100, Daniel Vetter wrote:
> On Wed, Jan 30, 2019 at 10:56:26AM -0800, Matt Roper wrote:
> > On Wed, Jan 30, 2019 at 10:51:19AM -0800, Matt Roper wrote:
> > > Previous patch series was here:
> > >   https://lists.freedesktop.org/archives/dri-devel/2018-December/201949.html
> > > 
> > > I'm told the ChromeOS userspace code to make use of the background color
> > > has been reviewed and is ready for use:
> > >  * https://chromium-review.googlesource.com/c/chromium/src/+/1278858
> > >  * https://chromium-review.googlesource.com/c/chromium/src/+/1278858
> > 
> > Woops, the second link here should have been to
> > 
> >   https://chromium-review.googlesource.com/c/chromiumos/platform/drm-tests/+/1241436
> > 
> > which I believe is some unit tests to go along with the main userspace
> > code.
> 
> Do we have this as igts too?
> -Daniel

Yeah, I posted it along with some of the earlier revisions of the
series, but haven't reposted the latest copy lately.  I'll check and see
if it needs a rebase and then post it shortly.

Unfortunately the IGT isn't as useful as I'd like it to be since the
CRC's for a plane filled with a solid color never come up the same as a
pure background color (at least on the APL platform I'm using).  I can
run the IGT in interactive mode and the colors seem identical to visual
inspection, but the CRC values never agree, so I've disabled the CRC
comparison in the test for now.  I'm not sure if this is related to the
other blending quirks of gen9; it will be interesting to see if the
CRC's match on an Icelake or something.

FWIW, I've mmap'd the Cairo-generated plane framebuffer and verified
that it contains exactly the pixel values we'd expect (so it's not a
matter of bad roundoff in Cairo), and I've tried flipping both 8bpc and
10bpc pixel formats (to match the background color's 10 bits per
component), but nothing seems to make the CRC's match.  :-(


Matt

> 
> > 
> > 
> > Matt
> > 
> > > 
> > > So I think ABI-wise we've met the userspace consumer requirements to
> > > upstream this; we just need to get reviews on the two i915-specific
> > > patches and a clean CI report.
> > > 
> > > v4.1 is identical to v4 aside from a rebase onto the latest drm-tip.
> > > 
> > > 
> > > Matt Roper (3):
> > >   drm/i915: Force background color to black for gen9+ (v2)
> > >   drm: Add CRTC background color property (v4)
> > >   drm/i915/gen9+: Add support for pipe background color (v4)
> > > 
> > >  drivers/gpu/drm/drm_atomic_uapi.c    |  4 ++++
> > >  drivers/gpu/drm/drm_blend.c          | 27 +++++++++++++++++++---
> > >  drivers/gpu/drm/drm_mode_config.c    |  6 +++++
> > >  drivers/gpu/drm/i915/i915_debugfs.c  |  9 ++++++++
> > >  drivers/gpu/drm/i915/i915_reg.h      |  6 +++++
> > >  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, 138 insertions(+), 3 deletions(-)
> > > 
> > > -- 
> > > 2.14.5
> > > 
> > 
> > -- 
> > 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
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
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 i-g-t] tests/kms_crtc_background_color: overhaul to match upstream ABI (v4)

[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

Cc: igt-dev@lists.freedesktop.org
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 lib/igt_kms.c                     |   2 +-
 tests/kms_crtc_background_color.c | 262 ++++++++++++++++++--------------------
 2 files changed, 126 insertions(+), 138 deletions(-)

diff --git a/lib/igt_kms.c b/lib/igt_kms.c
index 71df98d2..c70cdba3 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..42458549 100644
--- a/tests/kms_crtc_background_color.c
+++ b/tests/kms_crtc_background_color.c
@@ -25,164 +25,152 @@
 #include "igt.h"
 #include <math.h>
 
-
 IGT_TEST_DESCRIPTION("Test crtc background color feature");
 
+/*
+ * The original idea was to paint a desired color into a full-screen primary
+ * plane and then compare that CRC with turning off all planes and setting the
+ * CRTC background to the same color.  Unforunately, the rounding and precision
+ * of color values as rendered by cairo vs created by the display controller
+ * are slightly different and give different CRC's, even though they're
+ * visually identical.
+ *
+ * Since we can't really use CRC's for testing, this test is mainly useful for
+ * visual inspection in interactive mode at the moment.
+ */
+
 typedef struct {
-	int gfx_fd;
 	igt_display_t display;
-	struct igt_fb fb;
-	igt_crc_t ref_crc;
+	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);
+
+	/* 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);
+
+	/*
+	 * In theory we should be able to compare the CRC's here and have
+	 * them come up equal.  However on Intel hardware this never seems
+	 * to be the case, even though the colors are visually identical.
+	 * Background color takes 10 bits of precision per component on Intel
+	 * hardware, but even with 10bpc plane content the CRC's don't match.
+	 * Disabling the CRC check for now; this test still provides some
+	 * value when run in interactive mode with visual inspection.
+	 */
+#if 0
+	igt_assert_crc_equal(&plane_crc, &bg_crc);
+#endif
 }
 
-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_INTEL);
+		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] tests/kms_crtc_background_color: overhaul to match upstream ABI (v4)

[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

Cc: igt-dev@lists.freedesktop.org
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 lib/igt_kms.c                     |   2 +-
 tests/kms_crtc_background_color.c | 262 ++++++++++++++++++--------------------
 2 files changed, 126 insertions(+), 138 deletions(-)

diff --git a/lib/igt_kms.c b/lib/igt_kms.c
index 71df98d2..c70cdba3 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..42458549 100644
--- a/tests/kms_crtc_background_color.c
+++ b/tests/kms_crtc_background_color.c
@@ -25,164 +25,152 @@
 #include "igt.h"
 #include <math.h>
 
-
 IGT_TEST_DESCRIPTION("Test crtc background color feature");
 
+/*
+ * The original idea was to paint a desired color into a full-screen primary
+ * plane and then compare that CRC with turning off all planes and setting the
+ * CRTC background to the same color.  Unforunately, the rounding and precision
+ * of color values as rendered by cairo vs created by the display controller
+ * are slightly different and give different CRC's, even though they're
+ * visually identical.
+ *
+ * Since we can't really use CRC's for testing, this test is mainly useful for
+ * visual inspection in interactive mode at the moment.
+ */
+
 typedef struct {
-	int gfx_fd;
 	igt_display_t display;
-	struct igt_fb fb;
-	igt_crc_t ref_crc;
+	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);
+
+	/* 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);
+
+	/*
+	 * In theory we should be able to compare the CRC's here and have
+	 * them come up equal.  However on Intel hardware this never seems
+	 * to be the case, even though the colors are visually identical.
+	 * Background color takes 10 bits of precision per component on Intel
+	 * hardware, but even with 10bpc plane content the CRC's don't match.
+	 * Disabling the CRC check for now; this test still provides some
+	 * value when run in interactive mode with visual inspection.
+	 */
+#if 0
+	igt_assert_crc_equal(&plane_crc, &bg_crc);
+#endif
 }
 
-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_INTEL);
+		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: [PATCH v4.1 1/3] drm/i915: Force background color to black for gen9+ (v2)

[Matt Roper]

On Wed, Jan 30, 2019 at 11:03:33PM +0200, Ville Syrjälä wrote:
> On Wed, Jan 30, 2019 at 10:51:20AM -0800, Matt Roper wrote:
> > We don't yet allow userspace to control the CRTC background color, but
> > we should manually program the color to black to ensure the BIOS didn't
> > leave us with some other color.  We should also set the pipe gamma and
> > pipe CSC bits so that the background color goes through the same color
> > management transformations that a plane with black pixels would.
> > 
> > v2: Rename register to SKL_BOTTOM_COLOR to more closely follow
> >     bspec naming.  (Ville)
> > 
> > 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>

Applied just this first patch to dinq since it's a bugfix and doesn't
impact the ABI that comes in the later patches.  Thanks for the review.


Matt

> 
> > ---
> >  drivers/gpu/drm/i915/i915_reg.h      |  6 ++++++
> >  drivers/gpu/drm/i915/intel_display.c | 19 +++++++++++++++++++
> >  2 files changed, 25 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index 03adcf3838de..a64deeb4e517 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -5710,6 +5710,12 @@ enum {
> >  #define   PIPEMISC_DITHER_TYPE_SP	(0 << 2)
> >  #define PIPEMISC(pipe)			_MMIO_PIPE2(pipe, _PIPE_MISC_A)
> >  
> > +/* Skylake+ pipe bottom (background) color */
> > +#define _SKL_BOTTOM_COLOR_A		0x70034
> > +#define   SKL_BOTTOM_COLOR_GAMMA_ENABLE	(1 << 31)
> > +#define   SKL_BOTTOM_COLOR_CSC_ENABLE	(1 << 30)
> > +#define SKL_BOTTOM_COLOR(pipe)		_MMIO_PIPE2(pipe, _SKL_BOTTOM_COLOR_A)
> > +
> >  #define VLV_DPFLIPSTAT				_MMIO(VLV_DISPLAY_BASE + 0x70028)
> >  #define   PIPEB_LINE_COMPARE_INT_EN		(1 << 29)
> >  #define   PIPEB_HLINE_INT_EN			(1 << 28)
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 539d8915b55f..a025efb1d7c6 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -3930,6 +3930,16 @@ static void intel_update_pipe_config(const struct intel_crtc_state *old_crtc_sta
> >  		else if (old_crtc_state->pch_pfit.enabled)
> >  			ironlake_pfit_disable(old_crtc_state);
> >  	}
> > +
> > +	/*
> > +	 * We don't (yet) allow userspace to control the pipe background color,
> > +	 * so force it to black, but apply pipe gamma and CSC so that its
> > +	 * handling will match how we program our planes.
> > +	 */
> > +	if (INTEL_GEN(dev_priv) >= 9)
> > +		I915_WRITE(SKL_BOTTOM_COLOR(crtc->pipe),
> > +			   SKL_BOTTOM_COLOR_GAMMA_ENABLE |
> > +			   SKL_BOTTOM_COLOR_CSC_ENABLE);
> >  }
> >  
> >  static void intel_fdi_normal_train(struct intel_crtc *crtc)
> > @@ -15488,6 +15498,15 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc,
> >  			    plane->base.type != DRM_PLANE_TYPE_PRIMARY)
> >  				intel_plane_disable_noatomic(crtc, plane);
> >  		}
> > +
> > +		/*
> > +		 * Disable any background color set by the BIOS, but enable the
> > +		 * gamma and CSC to match how we program our planes.
> > +		 */
> > +		if (INTEL_GEN(dev_priv) >= 9)
> > +			I915_WRITE(SKL_BOTTOM_COLOR(crtc->pipe),
> > +				   SKL_BOTTOM_COLOR_GAMMA_ENABLE |
> > +				   SKL_BOTTOM_COLOR_CSC_ENABLE);
> >  	}
> >  
> >  	/* Adjust the state of the output pipe according to whether we
> > -- 
> > 2.14.5
> 
> -- 
> Ville Syrjälä
> Intel

-- 
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

[igt-dev] ✓ Fi.CI.BAT: success for tests/kms_crtc_background_color: overhaul to match upstream ABI (v4)

[Patchwork]

== Series Details ==

Series: tests/kms_crtc_background_color: overhaul to match upstream ABI (v4)
URL   : https://patchwork.freedesktop.org/series/56012/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_5514 -> IGTPW_2324
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/56012/revisions/1/mbox/

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

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

### IGT changes ###

#### Issues hit ####

  * igt@kms_pipe_crc_basic@nonblocking-crc-pipe-b:
    - fi-byt-clapper:     PASS -> FAIL [fdo#107362]

  
#### Possible fixes ####

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

  * igt@kms_flip@basic-flip-vs-modeset:
    - fi-skl-6700hq:      DMESG-WARN [fdo#105998] -> PASS

  * igt@kms_frontbuffer_tracking@basic:
    - fi-byt-clapper:     FAIL [fdo#103167] -> PASS

  * igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a-frame-sequence:
    - fi-byt-clapper:     FAIL [fdo#103191] / [fdo#107362] -> PASS

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


Participating hosts (44 -> 41)
------------------------------

  Missing    (3): fi-ilk-m540 fi-byt-squawks fi-bsw-cyan 


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

    * IGT: IGT_4801 -> IGTPW_2324

  CI_DRM_5514: 8a7e6109652e58be9c43e2a7a4d318a7c5b34fef @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_2324: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2324/
  IGT_4801: 6f6bacf12759fb319ade3ba37861ae711f8a5cd9 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools



== Testlist changes ==

+igt@kms_crtc_background_color@background-color-pipe-a-black
+igt@kms_crtc_background_color@background-color-pipe-a-blue
+igt@kms_crtc_background_color@background-color-pipe-a-green
+igt@kms_crtc_background_color@background-color-pipe-a-red
+igt@kms_crtc_background_color@background-color-pipe-a-white
+igt@kms_crtc_background_color@background-color-pipe-b-black
+igt@kms_crtc_background_color@background-color-pipe-b-blue
+igt@kms_crtc_background_color@background-color-pipe-b-green
+igt@kms_crtc_background_color@background-color-pipe-b-red
+igt@kms_crtc_background_color@background-color-pipe-b-white
+igt@kms_crtc_background_color@background-color-pipe-c-black
+igt@kms_crtc_background_color@background-color-pipe-c-blue
+igt@kms_crtc_background_color@background-color-pipe-c-green
+igt@kms_crtc_background_color@background-color-pipe-c-red
+igt@kms_crtc_background_color@background-color-pipe-c-white
+igt@kms_crtc_background_color@background-color-pipe-d-black
+igt@kms_crtc_background_color@background-color-pipe-d-blue
+igt@kms_crtc_background_color@background-color-pipe-d-green
+igt@kms_crtc_background_color@background-color-pipe-d-red
+igt@kms_crtc_background_color@background-color-pipe-d-white
+igt@kms_crtc_background_color@background-color-pipe-e-black
+igt@kms_crtc_background_color@background-color-pipe-e-blue
+igt@kms_crtc_background_color@background-color-pipe-e-green
+igt@kms_crtc_background_color@background-color-pipe-e-red
+igt@kms_crtc_background_color@background-color-pipe-e-white
+igt@kms_crtc_background_color@background-color-pipe-f-black
+igt@kms_crtc_background_color@background-color-pipe-f-blue
+igt@kms_crtc_background_color@background-color-pipe-f-green
+igt@kms_crtc_background_color@background-color-pipe-f-red
+igt@kms_crtc_background_color@background-color-pipe-f-white
-igt@kms_crtc_background_color

== Logs ==

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

Re: [PATCH v4.1 2/3] drm: Add CRTC background color property (v4)

[Matt Roper]

On Wed, Jan 30, 2019 at 11:01:25PM +0200, Ville Syrjälä wrote:
> On Wed, Jan 30, 2019 at 10:51:21AM -0800, Matt Roper wrote:
> > 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)
> > 
> > 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       | 27 ++++++++++++++++++++++++---
> >  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, 80 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> > index 9a1f41adfc67..d569e20e34e3 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..d451ac9e1d6d 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).
> 
> What would the alpha<1.0 do in that case? Blend the writeback output
> with what's already there?

I think it's more for cases where your hardware's pipe writes back
actual ARGB data to memory, including an alpha component, rather than
just RGB or XRGB.  You could then take that ARGB writeback buffer and
pass it through some other form of useful compositing if the bgcolor
pixels have a non-opaque alpha.


Matt

> 
> >   */
> >  
> >  /**
> > @@ -593,3 +606,11 @@ int drm_plane_create_blend_mode_property(struct drm_plane *plane,
> >  	return 0;
> >  }
> >  EXPORT_SYMBOL(drm_plane_create_blend_mode_property);
> > +
> > +void drm_crtc_add_bgcolor_property(struct drm_crtc *crtc)
> > +{
> > +	drm_object_attach_property(&crtc->base,
> > +				   crtc->dev->mode_config.bgcolor_property,
> > +				   drm_argb(16, 0xffff, 0, 0, 0));
> 
> if (crtc->state)
>  crtc->state->bg = default value; ?
> 
> > +}
> > +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 1e6cb885994d..0463d3f4ae59 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
> 
> -- 
> Ville Syrjälä
> Intel

-- 
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

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

[Patchwork]

== Series Details ==

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

== Summary ==

CI Bug Log - changes from CI_DRM_5512_full -> Patchwork_12096_full
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  

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

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

### IGT changes ###

#### Suppressed ####

  The following results come from untrusted machines, tests, or statuses.
  They do not affect the overall result.

  * {igt@runner@aborted}:
    - shard-snb:          NOTRUN -> FAIL

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

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

### IGT changes ###

#### Issues hit ####

  * igt@kms_cursor_crc@cursor-256x85-onscreen:
    - shard-apl:          PASS -> FAIL [fdo#103232] +1

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

  * igt@kms_cursor_legacy@2x-long-nonblocking-modeset-vs-cursor-atomic:
    - shard-glk:          PASS -> FAIL [fdo#105454] / [fdo#106509]

  * igt@kms_flip@flip-vs-blocking-wf-vblank:
    - shard-snb:          PASS -> DMESG-WARN [fdo#107469]

  
#### Possible fixes ####

  * igt@kms_cursor_crc@cursor-128x128-sliding:
    - shard-glk:          FAIL [fdo#103232] -> PASS

  * igt@kms_flip@2x-flip-vs-modeset:
    - shard-hsw:          DMESG-WARN [fdo#102614] -> PASS

  * igt@kms_flip@flip-vs-expired-vblank:
    - shard-kbl:          FAIL [fdo#102887] / [fdo#105363] -> PASS

  * igt@kms_plane@pixel-format-pipe-a-planes:
    - shard-apl:          FAIL [fdo#103166] -> PASS +1

  * igt@pm_rc6_residency@rc6-accuracy:
    - shard-kbl:          {SKIP} [fdo#109271] -> PASS
    - shard-snb:          {SKIP} [fdo#109271] -> PASS

  
#### Warnings ####

  * igt@i915_suspend@shrink:
    - shard-kbl:          DMESG-WARN [fdo#109244] -> INCOMPLETE [fdo#103665] / [fdo#106886]

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#102614]: https://bugs.freedesktop.org/show_bug.cgi?id=102614
  [fdo#102887]: https://bugs.freedesktop.org/show_bug.cgi?id=102887
  [fdo#103166]: https://bugs.freedesktop.org/show_bug.cgi?id=103166
  [fdo#103232]: https://bugs.freedesktop.org/show_bug.cgi?id=103232
  [fdo#103665]: https://bugs.freedesktop.org/show_bug.cgi?id=103665
  [fdo#105363]: https://bugs.freedesktop.org/show_bug.cgi?id=105363
  [fdo#105454]: https://bugs.freedesktop.org/show_bug.cgi?id=105454
  [fdo#106509]: https://bugs.freedesktop.org/show_bug.cgi?id=106509
  [fdo#106886]: https://bugs.freedesktop.org/show_bug.cgi?id=106886
  [fdo#107469]: https://bugs.freedesktop.org/show_bug.cgi?id=107469
  [fdo#109244]: https://bugs.freedesktop.org/show_bug.cgi?id=109244
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109278]: https://bugs.freedesktop.org/show_bug.cgi?id=109278


Participating hosts (7 -> 5)
------------------------------

  Missing    (2): shard-skl shard-iclb 


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

    * Linux: CI_DRM_5512 -> Patchwork_12096

  CI_DRM_5512: bf1da2552de642ebce4ea3f91a3259ef819b13ce @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4801: 6f6bacf12759fb319ade3ba37861ae711f8a5cd9 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_12096: f96a4d9be129b2de9bab006d01adb14dbd80000f @ 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_12096/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[igt-dev] ✗ Fi.CI.IGT: failure for tests/kms_crtc_background_color: overhaul to match upstream ABI (v4)

[Patchwork]

== Series Details ==

Series: tests/kms_crtc_background_color: overhaul to match upstream ABI (v4)
URL   : https://patchwork.freedesktop.org/series/56012/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_5514_full -> IGTPW_2324_full
====================================================

Summary
-------

  **FAILURE**

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

  External URL: https://patchwork.freedesktop.org/api/1.0/series/56012/revisions/1/mbox/

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

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

### IGT changes ###

#### Possible regressions ####

  * igt@kms_draw_crc@draw-method-xrgb8888-render-xtiled:
    - shard-hsw:          PASS -> FAIL

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

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

### IGT changes ###

#### Issues hit ####

  * igt@kms_ccs@pipe-b-crc-sprite-planes-basic:
    - shard-glk:          PASS -> FAIL [fdo#108145]

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

  * igt@kms_cursor_crc@cursor-64x64-random:
    - shard-kbl:          PASS -> FAIL [fdo#103232]
    - shard-glk:          PASS -> FAIL [fdo#103232]

  * igt@kms_draw_crc@draw-method-xrgb8888-mmap-cpu-xtiled:
    - shard-glk:          PASS -> FAIL [fdo#107791]

  * igt@kms_plane@plane-position-covered-pipe-c-planes:
    - shard-apl:          PASS -> FAIL [fdo#103166] +1

  * igt@kms_plane@plane-position-hole-pipe-a-planes:
    - shard-hsw:          PASS -> DMESG-WARN [fdo#102614]

  * igt@kms_plane_multiple@atomic-pipe-a-tiling-x:
    - shard-glk:          PASS -> FAIL [fdo#103166]

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

  * igt@kms_vblank@pipe-c-ts-continuation-suspend:
    - shard-kbl:          PASS -> INCOMPLETE [fdo#103665]

  
#### Possible fixes ####

  * igt@gem_workarounds@suspend-resume:
    - shard-kbl:          INCOMPLETE [fdo#103665] -> PASS

  * igt@kms_available_modes_crc@available_mode_test_crc:
    - shard-apl:          FAIL [fdo#106641] -> PASS
    - shard-glk:          FAIL [fdo#106641] -> PASS
    - shard-kbl:          FAIL [fdo#106641] -> PASS

  * igt@kms_ccs@pipe-a-crc-sprite-planes-basic:
    - shard-apl:          FAIL [fdo#106510] / [fdo#108145] -> PASS

  * igt@kms_cursor_crc@cursor-128x128-random:
    - shard-apl:          FAIL [fdo#103232] -> PASS +1

  * igt@kms_cursor_crc@cursor-256x85-random:
    - shard-glk:          FAIL [fdo#103232] -> PASS +1

  * igt@kms_cursor_crc@cursor-64x64-sliding:
    - shard-kbl:          FAIL [fdo#103232] -> PASS

  * igt@kms_flip@flip-vs-expired-vblank:
    - shard-glk:          FAIL [fdo#105363] -> PASS

  * igt@kms_plane_alpha_blend@pipe-c-alpha-opaque-fb:
    - shard-glk:          FAIL [fdo#108145] -> PASS

  * igt@kms_plane_multiple@atomic-pipe-a-tiling-y:
    - shard-glk:          FAIL [fdo#103166] -> PASS +3

  * igt@kms_plane_multiple@atomic-pipe-c-tiling-yf:
    - shard-apl:          FAIL [fdo#103166] -> PASS +6
    - shard-kbl:          FAIL [fdo#103166] -> PASS +2

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

  
#### Warnings ####

  * igt@gem_exec_schedule@promotion-bsd2:
    - shard-apl:          {SKIP} [fdo#109271] -> INCOMPLETE [fdo#103927]

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#102614]: https://bugs.freedesktop.org/show_bug.cgi?id=102614
  [fdo#103166]: https://bugs.freedesktop.org/show_bug.cgi?id=103166
  [fdo#103232]: https://bugs.freedesktop.org/show_bug.cgi?id=103232
  [fdo#103665]: https://bugs.freedesktop.org/show_bug.cgi?id=103665
  [fdo#103927]: https://bugs.freedesktop.org/show_bug.cgi?id=103927
  [fdo#105363]: https://bugs.freedesktop.org/show_bug.cgi?id=105363
  [fdo#106510]: https://bugs.freedesktop.org/show_bug.cgi?id=106510
  [fdo#106641]: https://bugs.freedesktop.org/show_bug.cgi?id=106641
  [fdo#107791]: https://bugs.freedesktop.org/show_bug.cgi?id=107791
  [fdo#108145]: https://bugs.freedesktop.org/show_bug.cgi?id=108145
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109278]: https://bugs.freedesktop.org/show_bug.cgi?id=109278
  [fdo#99912]: https://bugs.freedesktop.org/show_bug.cgi?id=99912


Participating hosts (7 -> 4)
------------------------------

  Missing    (3): shard-snb shard-skl shard-iclb 


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

    * IGT: IGT_4801 -> IGTPW_2324
    * Piglit: piglit_4509 -> None

  CI_DRM_5514: 8a7e6109652e58be9c43e2a7a4d318a7c5b34fef @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_2324: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2324/
  IGT_4801: 6f6bacf12759fb319ade3ba37861ae711f8a5cd9 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

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

Re: [PATCH v4.1 2/3] drm: Add CRTC background color property (v4)

[Ville Syrjälä]

On Wed, Jan 30, 2019 at 06:11:16PM -0800, Matt Roper wrote:
> On Wed, Jan 30, 2019 at 11:01:25PM +0200, Ville Syrjälä wrote:
> > On Wed, Jan 30, 2019 at 10:51:21AM -0800, Matt Roper wrote:
> > > 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)
> > > 
> > > 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       | 27 ++++++++++++++++++++++++---
> > >  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, 80 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> > > index 9a1f41adfc67..d569e20e34e3 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..d451ac9e1d6d 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).
> > 
> > What would the alpha<1.0 do in that case? Blend the writeback output
> > with what's already there?
> 
> I think it's more for cases where your hardware's pipe writes back
> actual ARGB data to memory, including an alpha component, rather than
> just RGB or XRGB.  You could then take that ARGB writeback buffer and
> pass it through some other form of useful compositing if the bgcolor
> pixels have a non-opaque alpha.

I guess someone should specify the blend equation for the alpha
channel as well. I presume it should just be 'da = sa + (1-sa)*da'
regardless of the plane pre-multiply setting?

> 
> 
> Matt
> 
> > 
> > >   */
> > >  
> > >  /**
> > > @@ -593,3 +606,11 @@ int drm_plane_create_blend_mode_property(struct drm_plane *plane,
> > >  	return 0;
> > >  }
> > >  EXPORT_SYMBOL(drm_plane_create_blend_mode_property);
> > > +
> > > +void drm_crtc_add_bgcolor_property(struct drm_crtc *crtc)
> > > +{
> > > +	drm_object_attach_property(&crtc->base,
> > > +				   crtc->dev->mode_config.bgcolor_property,
> > > +				   drm_argb(16, 0xffff, 0, 0, 0));
> > 
> > if (crtc->state)
> >  crtc->state->bg = default value; ?
> > 
> > > +}
> > > +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 1e6cb885994d..0463d3f4ae59 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
> > 
> > -- 
> > Ville Syrjälä
> > Intel
> 
> -- 
> Matt Roper
> Graphics Software Engineer
> IoTG Platform Enabling & Development
> Intel Corporation
> (916) 356-2795

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

Re: [PATCH v4.1 0/3] CRTC background color

[Daniel Vetter]

On Wed, Jan 30, 2019 at 03:48:50PM -0800, Matt Roper wrote:
> On Wed, Jan 30, 2019 at 09:57:10PM +0100, Daniel Vetter wrote:
> > On Wed, Jan 30, 2019 at 10:56:26AM -0800, Matt Roper wrote:
> > > On Wed, Jan 30, 2019 at 10:51:19AM -0800, Matt Roper wrote:
> > > > Previous patch series was here:
> > > >   https://lists.freedesktop.org/archives/dri-devel/2018-December/201949.html
> > > > 
> > > > I'm told the ChromeOS userspace code to make use of the background color
> > > > has been reviewed and is ready for use:
> > > >  * https://chromium-review.googlesource.com/c/chromium/src/+/1278858
> > > >  * https://chromium-review.googlesource.com/c/chromium/src/+/1278858
> > > 
> > > Woops, the second link here should have been to
> > > 
> > >   https://chromium-review.googlesource.com/c/chromiumos/platform/drm-tests/+/1241436
> > > 
> > > which I believe is some unit tests to go along with the main userspace
> > > code.
> > 
> > Do we have this as igts too?
> > -Daniel
> 
> Yeah, I posted it along with some of the earlier revisions of the
> series, but haven't reposted the latest copy lately.  I'll check and see
> if it needs a rebase and then post it shortly.
> 
> Unfortunately the IGT isn't as useful as I'd like it to be since the
> CRC's for a plane filled with a solid color never come up the same as a
> pure background color (at least on the APL platform I'm using).  I can
> run the IGT in interactive mode and the colors seem identical to visual
> inspection, but the CRC values never agree, so I've disabled the CRC
> comparison in the test for now.  I'm not sure if this is related to the
> other blending quirks of gen9; it will be interesting to see if the
> CRC's match on an Icelake or something.

Please don't do that, we need to know when stuff doesn't work. Also, igt
(at least for more generic stuff like this) shouldn't be bent to exactly
match intel hw bugs.

And yes if the blending is generally broken on gen9 then I'd be surprised
if they managed to not screw it up for the background color. Usually
backgroun color works as if it's a separate additional plane that you
blend the others with.
-Daniel

> FWIW, I've mmap'd the Cairo-generated plane framebuffer and verified
> that it contains exactly the pixel values we'd expect (so it's not a
> matter of bad roundoff in Cairo), and I've tried flipping both 8bpc and
> 10bpc pixel formats (to match the background color's 10 bits per
> component), but nothing seems to make the CRC's match.  :-(
> 
> 
> Matt
> 
> > 
> > > 
> > > 
> > > Matt
> > > 
> > > > 
> > > > So I think ABI-wise we've met the userspace consumer requirements to
> > > > upstream this; we just need to get reviews on the two i915-specific
> > > > patches and a clean CI report.
> > > > 
> > > > v4.1 is identical to v4 aside from a rebase onto the latest drm-tip.
> > > > 
> > > > 
> > > > Matt Roper (3):
> > > >   drm/i915: Force background color to black for gen9+ (v2)
> > > >   drm: Add CRTC background color property (v4)
> > > >   drm/i915/gen9+: Add support for pipe background color (v4)
> > > > 
> > > >  drivers/gpu/drm/drm_atomic_uapi.c    |  4 ++++
> > > >  drivers/gpu/drm/drm_blend.c          | 27 +++++++++++++++++++---
> > > >  drivers/gpu/drm/drm_mode_config.c    |  6 +++++
> > > >  drivers/gpu/drm/i915/i915_debugfs.c  |  9 ++++++++
> > > >  drivers/gpu/drm/i915/i915_reg.h      |  6 +++++
> > > >  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, 138 insertions(+), 3 deletions(-)
> > > > 
> > > > -- 
> > > > 2.14.5
> > > > 
> > > 
> > > -- 
> > > 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
> > 
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
> 
> -- 
> Matt Roper
> Graphics Software Engineer
> IoTG Platform Enabling & Development
> Intel Corporation
> (916) 356-2795

-- 
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: [PATCH v4.1 0/3] CRTC background color

[Matt Roper]

On Fri, Feb 01, 2019 at 06:13:48PM +0100, Daniel Vetter wrote:
> On Wed, Jan 30, 2019 at 03:48:50PM -0800, Matt Roper wrote:
> > On Wed, Jan 30, 2019 at 09:57:10PM +0100, Daniel Vetter wrote:
> > > On Wed, Jan 30, 2019 at 10:56:26AM -0800, Matt Roper wrote:
> > > > On Wed, Jan 30, 2019 at 10:51:19AM -0800, Matt Roper wrote:
> > > > > Previous patch series was here:
> > > > >   https://lists.freedesktop.org/archives/dri-devel/2018-December/201949.html
> > > > > 
> > > > > I'm told the ChromeOS userspace code to make use of the background color
> > > > > has been reviewed and is ready for use:
> > > > >  * https://chromium-review.googlesource.com/c/chromium/src/+/1278858
> > > > >  * https://chromium-review.googlesource.com/c/chromium/src/+/1278858
> > > > 
> > > > Woops, the second link here should have been to
> > > > 
> > > >   https://chromium-review.googlesource.com/c/chromiumos/platform/drm-tests/+/1241436
> > > > 
> > > > which I believe is some unit tests to go along with the main userspace
> > > > code.
> > > 
> > > Do we have this as igts too?
> > > -Daniel
> > 
> > Yeah, I posted it along with some of the earlier revisions of the
> > series, but haven't reposted the latest copy lately.  I'll check and see
> > if it needs a rebase and then post it shortly.
> > 
> > Unfortunately the IGT isn't as useful as I'd like it to be since the
> > CRC's for a plane filled with a solid color never come up the same as a
> > pure background color (at least on the APL platform I'm using).  I can
> > run the IGT in interactive mode and the colors seem identical to visual
> > inspection, but the CRC values never agree, so I've disabled the CRC
> > comparison in the test for now.  I'm not sure if this is related to the
> > other blending quirks of gen9; it will be interesting to see if the
> > CRC's match on an Icelake or something.
> 
> Please don't do that, we need to know when stuff doesn't work. Also, igt
> (at least for more generic stuff like this) shouldn't be bent to exactly
> match intel hw bugs.
> 
> And yes if the blending is generally broken on gen9 then I'd be surprised
> if they managed to not screw it up for the background color. Usually
> backgroun color works as if it's a separate additional plane that you
> blend the others with.
> -Daniel

+igt-dev list

So looking at the bspec closer, it sounds like it might not actually be
valid for us to take a DMUX (pipe) CRC of just the background color.
The bspec for gen9+ states

    "The DMUX CRC should only be enabled when the pipe, and one or more
    planes in the pipe are enabled."

which implies that just using the background color (which is always on)
isn't sufficient.

I'll uncomment the CRC check when we actually push the test so that it
can still be used on non-Intel platforms that don't have this
restriction.  Should we just make the test skip on the relevant i915
platforms, or should we do something else to mark the test as an
expected failure due to hardware?


Matt

> 
> > FWIW, I've mmap'd the Cairo-generated plane framebuffer and verified
> > that it contains exactly the pixel values we'd expect (so it's not a
> > matter of bad roundoff in Cairo), and I've tried flipping both 8bpc and
> > 10bpc pixel formats (to match the background color's 10 bits per
> > component), but nothing seems to make the CRC's match.  :-(
> > 
> > 
> > Matt
> > 
> > > 
> > > > 
> > > > 
> > > > Matt
> > > > 
> > > > > 
> > > > > So I think ABI-wise we've met the userspace consumer requirements to
> > > > > upstream this; we just need to get reviews on the two i915-specific
> > > > > patches and a clean CI report.
> > > > > 
> > > > > v4.1 is identical to v4 aside from a rebase onto the latest drm-tip.
> > > > > 
> > > > > 
> > > > > Matt Roper (3):
> > > > >   drm/i915: Force background color to black for gen9+ (v2)
> > > > >   drm: Add CRTC background color property (v4)
> > > > >   drm/i915/gen9+: Add support for pipe background color (v4)
> > > > > 
> > > > >  drivers/gpu/drm/drm_atomic_uapi.c    |  4 ++++
> > > > >  drivers/gpu/drm/drm_blend.c          | 27 +++++++++++++++++++---
> > > > >  drivers/gpu/drm/drm_mode_config.c    |  6 +++++
> > > > >  drivers/gpu/drm/i915/i915_debugfs.c  |  9 ++++++++
> > > > >  drivers/gpu/drm/i915/i915_reg.h      |  6 +++++
> > > > >  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, 138 insertions(+), 3 deletions(-)
> > > > > 
> > > > > -- 
> > > > > 2.14.5
> > > > > 
> > > > 
> > > > -- 
> > > > 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
> > > 
> > > -- 
> > > Daniel Vetter
> > > Software Engineer, Intel Corporation
> > > http://blog.ffwll.ch
> > 
> > -- 
> > Matt Roper
> > Graphics Software Engineer
> > IoTG Platform Enabling & Development
> > Intel Corporation
> > (916) 356-2795
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

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

Re: [Intel-gfx] [PATCH v4.1 0/3] CRTC background color

[Matt Roper]

On Fri, Feb 01, 2019 at 06:13:48PM +0100, Daniel Vetter wrote:
> On Wed, Jan 30, 2019 at 03:48:50PM -0800, Matt Roper wrote:
> > On Wed, Jan 30, 2019 at 09:57:10PM +0100, Daniel Vetter wrote:
> > > On Wed, Jan 30, 2019 at 10:56:26AM -0800, Matt Roper wrote:
> > > > On Wed, Jan 30, 2019 at 10:51:19AM -0800, Matt Roper wrote:
> > > > > Previous patch series was here:
> > > > >   https://lists.freedesktop.org/archives/dri-devel/2018-December/201949.html
> > > > > 
> > > > > I'm told the ChromeOS userspace code to make use of the background color
> > > > > has been reviewed and is ready for use:
> > > > >  * https://chromium-review.googlesource.com/c/chromium/src/+/1278858
> > > > >  * https://chromium-review.googlesource.com/c/chromium/src/+/1278858
> > > > 
> > > > Woops, the second link here should have been to
> > > > 
> > > >   https://chromium-review.googlesource.com/c/chromiumos/platform/drm-tests/+/1241436
> > > > 
> > > > which I believe is some unit tests to go along with the main userspace
> > > > code.
> > > 
> > > Do we have this as igts too?
> > > -Daniel
> > 
> > Yeah, I posted it along with some of the earlier revisions of the
> > series, but haven't reposted the latest copy lately.  I'll check and see
> > if it needs a rebase and then post it shortly.
> > 
> > Unfortunately the IGT isn't as useful as I'd like it to be since the
> > CRC's for a plane filled with a solid color never come up the same as a
> > pure background color (at least on the APL platform I'm using).  I can
> > run the IGT in interactive mode and the colors seem identical to visual
> > inspection, but the CRC values never agree, so I've disabled the CRC
> > comparison in the test for now.  I'm not sure if this is related to the
> > other blending quirks of gen9; it will be interesting to see if the
> > CRC's match on an Icelake or something.
> 
> Please don't do that, we need to know when stuff doesn't work. Also, igt
> (at least for more generic stuff like this) shouldn't be bent to exactly
> match intel hw bugs.
> 
> And yes if the blending is generally broken on gen9 then I'd be surprised
> if they managed to not screw it up for the background color. Usually
> backgroun color works as if it's a separate additional plane that you
> blend the others with.
> -Daniel

+igt-dev list

So looking at the bspec closer, it sounds like it might not actually be
valid for us to take a DMUX (pipe) CRC of just the background color.
The bspec for gen9+ states

    "The DMUX CRC should only be enabled when the pipe, and one or more
    planes in the pipe are enabled."

which implies that just using the background color (which is always on)
isn't sufficient.

I'll uncomment the CRC check when we actually push the test so that it
can still be used on non-Intel platforms that don't have this
restriction.  Should we just make the test skip on the relevant i915
platforms, or should we do something else to mark the test as an
expected failure due to hardware?


Matt

> 
> > FWIW, I've mmap'd the Cairo-generated plane framebuffer and verified
> > that it contains exactly the pixel values we'd expect (so it's not a
> > matter of bad roundoff in Cairo), and I've tried flipping both 8bpc and
> > 10bpc pixel formats (to match the background color's 10 bits per
> > component), but nothing seems to make the CRC's match.  :-(
> > 
> > 
> > Matt
> > 
> > > 
> > > > 
> > > > 
> > > > Matt
> > > > 
> > > > > 
> > > > > So I think ABI-wise we've met the userspace consumer requirements to
> > > > > upstream this; we just need to get reviews on the two i915-specific
> > > > > patches and a clean CI report.
> > > > > 
> > > > > v4.1 is identical to v4 aside from a rebase onto the latest drm-tip.
> > > > > 
> > > > > 
> > > > > Matt Roper (3):
> > > > >   drm/i915: Force background color to black for gen9+ (v2)
> > > > >   drm: Add CRTC background color property (v4)
> > > > >   drm/i915/gen9+: Add support for pipe background color (v4)
> > > > > 
> > > > >  drivers/gpu/drm/drm_atomic_uapi.c    |  4 ++++
> > > > >  drivers/gpu/drm/drm_blend.c          | 27 +++++++++++++++++++---
> > > > >  drivers/gpu/drm/drm_mode_config.c    |  6 +++++
> > > > >  drivers/gpu/drm/i915/i915_debugfs.c  |  9 ++++++++
> > > > >  drivers/gpu/drm/i915/i915_reg.h      |  6 +++++
> > > > >  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, 138 insertions(+), 3 deletions(-)
> > > > > 
> > > > > -- 
> > > > > 2.14.5
> > > > > 
> > > > 
> > > > -- 
> > > > 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
> > > 
> > > -- 
> > > Daniel Vetter
> > > Software Engineer, Intel Corporation
> > > http://blog.ffwll.ch
> > 
> > -- 
> > Matt Roper
> > Graphics Software Engineer
> > IoTG Platform Enabling & Development
> > Intel Corporation
> > (916) 356-2795
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
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: [i-g-t] tests/kms_crtc_background_color: overhaul to match upstream ABI (v4)

[Heiko Stuebner]

Am Donnerstag, 31. Januar 2019, 01:00:34 CET schrieb 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
> 
> Cc: igt-dev@lists.freedesktop.org
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>

[...]

> +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_INTEL);

DRIVER_ANY perhaps like in other tests?

I'm currently looking into implementing your new background-property
in the Rockchip kms driver and I guess this test shouldn't contain any
intel-specifics?

Heiko


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

Re: [Intel-gfx] [i-g-t] tests/kms_crtc_background_color: overhaul to match upstream ABI (v4)

[Heiko Stuebner]

Am Donnerstag, 31. Januar 2019, 01:00:34 CET schrieb 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
> 
> Cc: igt-dev@lists.freedesktop.org
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>

[...]

> +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_INTEL);

DRIVER_ANY perhaps like in other tests?

I'm currently looking into implementing your new background-property
in the Rockchip kms driver and I guess this test shouldn't contain any
intel-specifics?

Heiko


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