[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

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

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

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