[PATCH v2 0/3] CRTC background color

[PATCH v2 0/3] CRTC background color

[Matt Roper]

This is a second revision of the series previously posted here:
   https://lists.freedesktop.org/archives/intel-gfx/2018-October/178202.html

As noted before, this functionality adds new ABI so we need a userspace
consumer ready before we merge the kernel work.  My understanding is
that some of the folks involved with ChromeOS are looking at this and
that there's a ChromeOS userspace review happening at
   https://chromium-review.googlesource.com/c/chromium/src/+/1278858

Since there are a few Intel-specific background color changes that we
want to make independently of the new ABI, I've separated those out into
a new patch #1; we may want to consider landing that patch before the
rest of the series since it fixes an inconsistency in how we currently
program our hardware.

On the i915-side of things, this series only deals with gen9+ at the
moment.  It looks like CHV may also have support for background color
functionality, but I couldn't find the register layout details for that
platform, so I haven't added support for it yet.

Cc: dri-devel@lists.freedesktop.org
Cc: Wei C Li <wei.c.li@intel.com>
Cc: Sean Paul <sean@poorly.run>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>

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

 drivers/gpu/drm/drm_atomic_state_helper.c |  1 +
 drivers/gpu/drm/drm_atomic_uapi.c         |  5 ++++
 drivers/gpu/drm/drm_blend.c               | 21 ++++++++++++++---
 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      | 39 +++++++++++++++++++++++++++++++
 include/drm/drm_blend.h                   |  1 +
 include/drm/drm_crtc.h                    | 17 ++++++++++++++
 include/drm/drm_mode_config.h             |  5 ++++
 include/uapi/drm/drm_mode.h               | 26 +++++++++++++++++++++
 11 files changed, 133 insertions(+), 3 deletions(-)

-- 
2.14.4

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

[PATCH v2 2/3] drm: Add CRTC background color property (v2)

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

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>
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/drm_atomic_state_helper.c |  1 +
 drivers/gpu/drm/drm_atomic_uapi.c         |  5 +++++
 drivers/gpu/drm/drm_blend.c               | 21 ++++++++++++++++++---
 drivers/gpu/drm/drm_mode_config.c         |  6 ++++++
 include/drm/drm_blend.h                   |  1 +
 include/drm/drm_crtc.h                    | 17 +++++++++++++++++
 include/drm/drm_mode_config.h             |  5 +++++
 include/uapi/drm/drm_mode.h               | 26 ++++++++++++++++++++++++++
 8 files changed, 79 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c
index 3ba996069d69..2f8c55668089 100644
--- a/drivers/gpu/drm/drm_atomic_state_helper.c
+++ b/drivers/gpu/drm/drm_atomic_state_helper.c
@@ -101,6 +101,7 @@ void __drm_atomic_helper_crtc_duplicate_state(struct drm_crtc *crtc,
 	state->planes_changed = false;
 	state->connectors_changed = false;
 	state->color_mgmt_changed = false;
+	state->bgcolor_changed = false;
 	state->zpos_changed = false;
 	state->commit = NULL;
 	state->event = NULL;
diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
index 86ac33922b09..b95a55a778e2 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -467,6 +467,9 @@ 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;
+		state->bgcolor_changed = true;
 	} else if (crtc->funcs->atomic_set_property) {
 		return crtc->funcs->atomic_set_property(crtc, state, property, val);
 	} else {
@@ -499,6 +502,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..7c73cb83874a 100644
--- a/drivers/gpu/drm/drm_blend.c
+++ b/drivers/gpu/drm/drm_blend.c
@@ -175,9 +175,16 @@
  *		 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:
+ *
+ * bgcolor:
+ *	Background color is setup with drm_crtc_add_bgcolor_property().  It
+ *	controls the RGB color of a full-screen, fully-opaque 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.
  */
 
 /**
@@ -593,3 +600,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_rgba(16, 0, 0, 0, 0xffff));
+}
+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 ee80788f2c40..75e376755176 100644
--- a/drivers/gpu/drm/drm_mode_config.c
+++ b/drivers/gpu/drm/drm_mode_config.c
@@ -352,6 +352,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 b21437bc95bf..d78f82f954e4 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -168,6 +168,11 @@ struct drm_crtc_state {
 	 * drivers to steer the atomic commit control flow.
 	 */
 	bool color_mgmt_changed : 1;
+	/**
+	 * @bgcolor_changed: Background color value has changed.  Used by
+	 * drivers to steer the atomic commit control flow.
+	 */
+	bool bgcolor_changed : 1;
 
 	/**
 	 * @no_vblank:
@@ -274,6 +279,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_rgba();
+	 * individual color components can be extracted with desired precision
+	 * via the DRM_RGBA_*() macros.
+	 */
+	u64 bgcolor;
+
 	/**
 	 * @target_vblank:
 	 *
diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
index 5dbeabdbaf91..8b686d228feb 100644
--- a/include/drm/drm_mode_config.h
+++ b/include/drm/drm_mode_config.h
@@ -813,6 +813,11 @@ struct drm_mode_config {
 	 */
 	struct drm_property *writeback_out_fence_ptr_property;
 
+	/**
+	 * @bgcolor_property: RGBA 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 d3e0fe31efc5..7c4f902aa290 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -888,6 +888,32 @@ struct drm_mode_revoke_lease {
 	__u32 lessee_id;
 };
 
+/*
+ * Put RGBA 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_rgba(__u8 bpc, __u16 red, __u16 green, __u16 blue, __u16 alpha)
+{
+	int msb_shift = 16 - bpc;
+
+	return (__u64)alpha << msb_shift << 48 |
+	       (__u64)red   << msb_shift << 32 |
+	       (__u64)green << msb_shift << 16 |
+	       (__u64)blue  << msb_shift;
+}
+
+/*
+ * Extract the specified number of bits of a specific color component from a
+ * standard 64-bit RGBA value.
+ */
+#define DRM_RGBA_BLUE(c, numbits)  (__u16)((c & 0xFFFFull)     >> (16-numbits))
+#define DRM_RGBA_GREEN(c, numbits) (__u16)((c & 0xFFFFull<<16) >> (32-numbits))
+#define DRM_RGBA_RED(c, numbits)   (__u16)((c & 0xFFFFull<<32) >> (48-numbits))
+#define DRM_RGBA_ALPHA(c, numbits) (__u16)((c & 0xFFFFull<<48) >> (64-numbits))
+
 #if defined(__cplusplus)
 }
 #endif
-- 
2.14.4

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

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

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

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 | 35 ++++++++++++++++++++++++++++-------
 2 files changed, 37 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 670db5073d70..1f2a19e6ec79 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -3254,6 +3254,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_RGBA_RED(background, 10),
+				   DRM_RGBA_GREEN(background, 10),
+				   DRM_RGBA_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 1d089d93d88b..e7a759e0c021 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3834,6 +3834,27 @@ 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 *cstate)
+{
+	struct intel_crtc *crtc = to_intel_crtc(cstate->base.crtc);
+	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
+	uint64_t propval = cstate->base.bgcolor;
+	uint32_t tmp;
+
+	/* Hardware is programmed with 10 bits of precision */
+	tmp = DRM_RGBA_RED(propval, 10) << 20
+	    | DRM_RGBA_GREEN(propval, 10) << 10
+	    | DRM_RGBA_BLUE(propval, 10);
+
+	/*
+	 * Set CSC and gamma for bottom color to ensure background pixels
+	 * receive the same color transformations as plane content.
+	 */
+	tmp |= SKL_CANVAS_CSC_ENABLE | SKL_CANVAS_GAMMA_ENABLE;
+
+	I915_WRITE_FW(SKL_CANVAS(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)
 {
@@ -3869,14 +3890,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_CANVAS(crtc->pipe),
-			   SKL_CANVAS_GAMMA_ENABLE | SKL_CANVAS_CSC_ENABLE);
+		skl_update_background_color(new_crtc_state);
 }
 
 static void intel_fdi_normal_train(struct intel_crtc *crtc)
@@ -10896,6 +10911,9 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc,
 		crtc_state->planes_changed = true;
 	}
 
+	if (crtc_state->bgcolor_changed)
+		pipe_config->update_pipe = true;
+
 	ret = 0;
 	if (dev_priv->display.compute_pipe_wm) {
 		ret = dev_priv->display.compute_pipe_wm(pipe_config);
@@ -14035,6 +14053,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:
-- 
2.14.4

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

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

[Sean Paul]

On Tue, Nov 13, 2018 at 03:21:48PM -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.
> 
> 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>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>  drivers/gpu/drm/drm_atomic_state_helper.c |  1 +
>  drivers/gpu/drm/drm_atomic_uapi.c         |  5 +++++
>  drivers/gpu/drm/drm_blend.c               | 21 ++++++++++++++++++---
>  drivers/gpu/drm/drm_mode_config.c         |  6 ++++++
>  include/drm/drm_blend.h                   |  1 +
>  include/drm/drm_crtc.h                    | 17 +++++++++++++++++
>  include/drm/drm_mode_config.h             |  5 +++++
>  include/uapi/drm/drm_mode.h               | 26 ++++++++++++++++++++++++++
>  8 files changed, 79 insertions(+), 3 deletions(-)

/snip

> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index d3e0fe31efc5..7c4f902aa290 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -888,6 +888,32 @@ struct drm_mode_revoke_lease {
>  	__u32 lessee_id;
>  };
>  
> +/*
> + * Put RGBA 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_rgba(__u8 bpc, __u16 red, __u16 green, __u16 blue, __u16 alpha)
> +{
> +	int msb_shift = 16 - bpc;
> +
> +	return (__u64)alpha << msb_shift << 48 |
> +	       (__u64)red   << msb_shift << 32 |
> +	       (__u64)green << msb_shift << 16 |
> +	       (__u64)blue  << msb_shift;
> +}
> +
> +/*
> + * Extract the specified number of bits of a specific color component from a
> + * standard 64-bit RGBA value.
> + */
> +#define DRM_RGBA_BLUE(c, numbits)  (__u16)((c & 0xFFFFull)     >> (16-numbits))
> +#define DRM_RGBA_GREEN(c, numbits) (__u16)((c & 0xFFFFull<<16) >> (32-numbits))
> +#define DRM_RGBA_RED(c, numbits)   (__u16)((c & 0xFFFFull<<32) >> (48-numbits))
> +#define DRM_RGBA_ALPHA(c, numbits) (__u16)((c & 0xFFFFull<<48) >> (64-numbits))


#define  DRM_RGBA_COMP(c, shift, numbits) \
        (__u16)(((c) & 0xFFFFull << (shift)) >> (32 - ((shift) + 16 - (numbits)))

#define DRM_RGBA_BLUE(c, numbits)  DRM_RGBA_COMP(c, 0, numbits)
#define DRM_RGBA_GREEN(c, numbits) DRM_RGBA_COMP(c, 16, numbits)
#define DRM_RGBA_RED(c, numbits)   DRM_RGBA_COMP(c, 32, numbits)
#define DRM_RGBA_ALPHA(c, numbits) DRM_RGBA_COMP(c, 48, numbits)


With that,

Reviewed-by: Sean Paul <sean@poorly.run>



> +
>  #if defined(__cplusplus)
>  }
>  #endif
> -- 
> 2.14.4
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Ville Syrjälä]

On Tue, Nov 13, 2018 at 03:21:48PM -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.
> 
> 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>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>  drivers/gpu/drm/drm_atomic_state_helper.c |  1 +
>  drivers/gpu/drm/drm_atomic_uapi.c         |  5 +++++
>  drivers/gpu/drm/drm_blend.c               | 21 ++++++++++++++++++---
>  drivers/gpu/drm/drm_mode_config.c         |  6 ++++++
>  include/drm/drm_blend.h                   |  1 +
>  include/drm/drm_crtc.h                    | 17 +++++++++++++++++
>  include/drm/drm_mode_config.h             |  5 +++++
>  include/uapi/drm/drm_mode.h               | 26 ++++++++++++++++++++++++++
>  8 files changed, 79 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c
> index 3ba996069d69..2f8c55668089 100644
> --- a/drivers/gpu/drm/drm_atomic_state_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_state_helper.c
> @@ -101,6 +101,7 @@ void __drm_atomic_helper_crtc_duplicate_state(struct drm_crtc *crtc,
>  	state->planes_changed = false;
>  	state->connectors_changed = false;
>  	state->color_mgmt_changed = false;
> +	state->bgcolor_changed = false;

Wondering a bit about the granulairty of these flags. Is bgcolor
important enough to warrant its own flag, or should it just be part of
color_mgmt_changed for example?

color_mgmt_changed is rather heavy though as it would force LUT
reprogramming, so I've been thinking that it should perhaps be 
split into separate
gamma_lut_changed/degamma_lut_changed/other_color_stuff_changed
flags.

Anyways, just thinking out loud here. We can combine some
of these flags later if we start to accumulate too many.


>  	state->zpos_changed = false;
>  	state->commit = NULL;
>  	state->event = NULL;
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> index 86ac33922b09..b95a55a778e2 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -467,6 +467,9 @@ 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;
> +		state->bgcolor_changed = true;
>  	} else if (crtc->funcs->atomic_set_property) {
>  		return crtc->funcs->atomic_set_property(crtc, state, property, val);
>  	} else {
> @@ -499,6 +502,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..7c73cb83874a 100644
> --- a/drivers/gpu/drm/drm_blend.c
> +++ b/drivers/gpu/drm/drm_blend.c
> @@ -175,9 +175,16 @@
>   *		 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:
> + *
> + * bgcolor:
> + *	Background color is setup with drm_crtc_add_bgcolor_property().  It
> + *	controls the RGB color of a full-screen, fully-opaque 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.
>   */
>  
>  /**
> @@ -593,3 +600,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_rgba(16, 0, 0, 0, 0xffff));
> +}
> +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 ee80788f2c40..75e376755176 100644
> --- a/drivers/gpu/drm/drm_mode_config.c
> +++ b/drivers/gpu/drm/drm_mode_config.c
> @@ -352,6 +352,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 b21437bc95bf..d78f82f954e4 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -168,6 +168,11 @@ struct drm_crtc_state {
>  	 * drivers to steer the atomic commit control flow.
>  	 */
>  	bool color_mgmt_changed : 1;
> +	/**
> +	 * @bgcolor_changed: Background color value has changed.  Used by
> +	 * drivers to steer the atomic commit control flow.
> +	 */
> +	bool bgcolor_changed : 1;
>  
>  	/**
>  	 * @no_vblank:
> @@ -274,6 +279,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_rgba();
> +	 * individual color components can be extracted with desired precision
> +	 * via the DRM_RGBA_*() macros.
> +	 */
> +	u64 bgcolor;
> +
>  	/**
>  	 * @target_vblank:
>  	 *
> diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
> index 5dbeabdbaf91..8b686d228feb 100644
> --- a/include/drm/drm_mode_config.h
> +++ b/include/drm/drm_mode_config.h
> @@ -813,6 +813,11 @@ struct drm_mode_config {
>  	 */
>  	struct drm_property *writeback_out_fence_ptr_property;
>  
> +	/**
> +	 * @bgcolor_property: RGBA 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 d3e0fe31efc5..7c4f902aa290 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -888,6 +888,32 @@ struct drm_mode_revoke_lease {
>  	__u32 lessee_id;
>  };
>  
> +/*
> + * Put RGBA 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_rgba(__u8 bpc, __u16 red, __u16 green, __u16 blue, __u16 alpha)
> +{
> +	int msb_shift = 16 - bpc;
> +
> +	return (__u64)alpha << msb_shift << 48 |
> +	       (__u64)red   << msb_shift << 32 |
> +	       (__u64)green << msb_shift << 16 |
> +	       (__u64)blue  << msb_shift;

So the value is now ARGB but everything still says RGBA?

> +}
> +
> +/*
> + * Extract the specified number of bits of a specific color component from a
> + * standard 64-bit RGBA value.
> + */
> +#define DRM_RGBA_BLUE(c, numbits)  (__u16)((c & 0xFFFFull)     >> (16-numbits))
> +#define DRM_RGBA_GREEN(c, numbits) (__u16)((c & 0xFFFFull<<16) >> (32-numbits))
> +#define DRM_RGBA_RED(c, numbits)   (__u16)((c & 0xFFFFull<<32) >> (48-numbits))
> +#define DRM_RGBA_ALPHA(c, numbits) (__u16)((c & 0xFFFFull<<48) >> (64-numbits))
> +
>  #if defined(__cplusplus)
>  }
>  #endif
> -- 
> 2.14.4

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

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

[Matt Roper]

On Wed, Nov 14, 2018 at 07:20:36PM +0200, Ville Syrjälä wrote:
> On Tue, Nov 13, 2018 at 03:21:48PM -0800, Matt Roper wrote:
...
> >  
> > +/*
> > + * Put RGBA 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_rgba(__u8 bpc, __u16 red, __u16 green, __u16 blue, __u16 alpha)
> > +{
> > +	int msb_shift = 16 - bpc;
> > +
> > +	return (__u64)alpha << msb_shift << 48 |
> > +	       (__u64)red   << msb_shift << 32 |
> > +	       (__u64)green << msb_shift << 16 |
> > +	       (__u64)blue  << msb_shift;
> 
> So the value is now ARGB but everything still says RGBA?

Well, the general idea was that you're providing four color components
and you're not supposed to be looking behind the curtain at the internal
representation (which may not match any given platform's hardware
ordering).  If everyone uses the helpers here to pack/unpack the
components, then that avoids accidental differences in interpretation
between different platforms.

But I'm fine with changing the names; I'll do that when I incorporate
Sean's suggestion.



Matt

> 
> > +}
> > +
> > +/*
> > + * Extract the specified number of bits of a specific color component from a
> > + * standard 64-bit RGBA value.
> > + */
> > +#define DRM_RGBA_BLUE(c, numbits)  (__u16)((c & 0xFFFFull)     >> (16-numbits))
> > +#define DRM_RGBA_GREEN(c, numbits) (__u16)((c & 0xFFFFull<<16) >> (32-numbits))
> > +#define DRM_RGBA_RED(c, numbits)   (__u16)((c & 0xFFFFull<<32) >> (48-numbits))
> > +#define DRM_RGBA_ALPHA(c, numbits) (__u16)((c & 0xFFFFull<<48) >> (64-numbits))
> > +
> >  #if defined(__cplusplus)
> >  }
> >  #endif
> > -- 
> > 2.14.4
> 
> -- 
> 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 v2 3/3] drm/i915/gen9+: Add support for pipe background color (v2)

[Ville Syrjälä]

On Tue, Nov 13, 2018 at 03:21:49PM -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.
> 
> 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 | 35 ++++++++++++++++++++++++++++-------
>  2 files changed, 37 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 670db5073d70..1f2a19e6ec79 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -3254,6 +3254,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_RGBA_RED(background, 10),
> +				   DRM_RGBA_GREEN(background, 10),
> +				   DRM_RGBA_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 1d089d93d88b..e7a759e0c021 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3834,6 +3834,27 @@ 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 *cstate)

s/cstate/crtc_state/ please

> +{
> +	struct intel_crtc *crtc = to_intel_crtc(cstate->base.crtc);
> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> +	uint64_t propval = cstate->base.bgcolor;
> +	uint32_t tmp;
> +
> +	/* Hardware is programmed with 10 bits of precision */
> +	tmp = DRM_RGBA_RED(propval, 10) << 20
> +	    | DRM_RGBA_GREEN(propval, 10) << 10
> +	    | DRM_RGBA_BLUE(propval, 10);
> +
> +	/*
> +	 * Set CSC and gamma for bottom color to ensure background pixels
> +	 * receive the same color transformations as plane content.
> +	 */
> +	tmp |= SKL_CANVAS_CSC_ENABLE | SKL_CANVAS_GAMMA_ENABLE;
> +
> +	I915_WRITE_FW(SKL_CANVAS(crtc->pipe), tmp);

Why _FW?

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

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

[Matt Roper]

On Wed, Nov 14, 2018 at 11:17:25AM -0500, Sean Paul wrote:
> On Tue, Nov 13, 2018 at 03:21:48PM -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.
> > 
> > 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>
> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> > ---
> >  drivers/gpu/drm/drm_atomic_state_helper.c |  1 +
> >  drivers/gpu/drm/drm_atomic_uapi.c         |  5 +++++
> >  drivers/gpu/drm/drm_blend.c               | 21 ++++++++++++++++++---
> >  drivers/gpu/drm/drm_mode_config.c         |  6 ++++++
> >  include/drm/drm_blend.h                   |  1 +
> >  include/drm/drm_crtc.h                    | 17 +++++++++++++++++
> >  include/drm/drm_mode_config.h             |  5 +++++
> >  include/uapi/drm/drm_mode.h               | 26 ++++++++++++++++++++++++++
> >  8 files changed, 79 insertions(+), 3 deletions(-)
> 
> /snip
> 
> > diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> > index d3e0fe31efc5..7c4f902aa290 100644
> > --- a/include/uapi/drm/drm_mode.h
> > +++ b/include/uapi/drm/drm_mode.h
> > @@ -888,6 +888,32 @@ struct drm_mode_revoke_lease {
> >  	__u32 lessee_id;
> >  };
> >  
> > +/*
> > + * Put RGBA 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_rgba(__u8 bpc, __u16 red, __u16 green, __u16 blue, __u16 alpha)
> > +{
> > +	int msb_shift = 16 - bpc;
> > +
> > +	return (__u64)alpha << msb_shift << 48 |
> > +	       (__u64)red   << msb_shift << 32 |
> > +	       (__u64)green << msb_shift << 16 |
> > +	       (__u64)blue  << msb_shift;
> > +}
> > +
> > +/*
> > + * Extract the specified number of bits of a specific color component from a
> > + * standard 64-bit RGBA value.
> > + */
> > +#define DRM_RGBA_BLUE(c, numbits)  (__u16)((c & 0xFFFFull)     >> (16-numbits))
> > +#define DRM_RGBA_GREEN(c, numbits) (__u16)((c & 0xFFFFull<<16) >> (32-numbits))
> > +#define DRM_RGBA_RED(c, numbits)   (__u16)((c & 0xFFFFull<<32) >> (48-numbits))
> > +#define DRM_RGBA_ALPHA(c, numbits) (__u16)((c & 0xFFFFull<<48) >> (64-numbits))
> 
> 
> #define  DRM_RGBA_COMP(c, shift, numbits) \
>         (__u16)(((c) & 0xFFFFull << (shift)) >> (32 - ((shift) + 16 - (numbits)))

I think this should just be

    #define  DRM_RGBA_COMP(c, shift, numbits) \
          (__u16)(((c) & 0xFFFFull << (shift)) >> ((shift) + 16 - (numbits)))

right?  I.e., right shift the same amount as the left shift, plus an
additional (16-numbits) to account for the lower precision requested.


Matt

> 
> #define DRM_RGBA_BLUE(c, numbits)  DRM_RGBA_COMP(c, 0, numbits)
> #define DRM_RGBA_GREEN(c, numbits) DRM_RGBA_COMP(c, 16, numbits)
> #define DRM_RGBA_RED(c, numbits)   DRM_RGBA_COMP(c, 32, numbits)
> #define DRM_RGBA_ALPHA(c, numbits) DRM_RGBA_COMP(c, 48, numbits)
> 
> 
> With that,
> 
> Reviewed-by: Sean Paul <sean@poorly.run>
> 
> 
> 
> > +
> >  #if defined(__cplusplus)
> >  }
> >  #endif
> > -- 
> > 2.14.4
> > 
> 
> -- 
> Sean Paul, Software Engineer, Google / Chromium OS

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