[PATCH v9] drm/i915/skl: Add support for SKL background color

[PATCH v9] drm/i915/skl: Add support for SKL background color

[Chandra Konduru]

This patch adds support for Skylake display pipe background color.

v2:
- added property documentation to drm DocBook (Daniel Vetter)
- moved property to drm_mode_config (Daniel Vetter)
- change to set property to NULL once it is freed. (me)
- change to make sure gamma/csc settings were retained during color change (me)
- change to make sure background color is restored after a power event (me)

v3:
- change to add gen check before writing background color (Matt Roper)

v4:
- change to increase color from 8bpc to 16bpc (Daniel Vetter)

v5:
- removed initialization of background color to 0 (Damien Lespiau)
- added change log to commit message (Damien Lespiau)

v6:
- optmization to use _PIPE() (Damien Lespiau)
- updated max limit value in DocBook (Damien Lespiau)
- simplied color conversion equation (Damien Lespiau)
- always enable gamma and csc to align with update plane (Damien Lespiau)
- move resume restoration code to haswell_crtc_enable (Damien Lespiau)

v7:
- remove superfluous comments (Damien)
- remove unused variables left over between v5 and v6 (Damien)

v8:
- remove the destruction of the property, DRM core does that for us (Damien)

v9:
- add ULL to a 64bits constant

Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>
Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 Documentation/DocBook/drm.tmpl       |   10 ++++++-
 drivers/gpu/drm/i915/i915_reg.h      |   10 +++++++
 drivers/gpu/drm/i915/intel_display.c |   54 ++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h     |    2 ++
 include/drm/drm_crtc.h               |    3 ++
 5 files changed, 78 insertions(+), 1 deletion(-)

diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
index 249f0c9..c83ffe6 100644
--- a/Documentation/DocBook/drm.tmpl
+++ b/Documentation/DocBook/drm.tmpl
@@ -2834,7 +2834,7 @@ void intel_crt_init(struct drm_device *dev)
 	<td valign="top" >TBD</td>
 	</tr>
 	<tr>
-	<td rowspan="21" valign="top" >i915</td>
+	<td rowspan="22" valign="top" >i915</td>
 	<td rowspan="2" valign="top" >Generic</td>
 	<td valign="top" >"Broadcast RGB"</td>
 	<td valign="top" >ENUM</td>
@@ -2858,6 +2858,14 @@ void intel_crt_init(struct drm_device *dev)
 	<td valign="top" >TBD</td>
 	</tr>
 	<tr>
+	<td rowspan="1" valign="top" >CRTC</td>
+	<td valign="top" >“background_color”</td>
+	<td valign="top" >Range</td>
+	<td valign="top" >Min=0, Max=0xFFFFFFFFFFFF</td>
+	<td valign="top" >CRTC</td>
+	<td valign="top" >Background color in 16bpc BGR (B-MSB, R-LSB)</td>
+	</tr>
+	<tr>
 	<td rowspan="17" valign="top" >SDVO-TV</td>
 	<td valign="top" >“mode”</td>
 	<td valign="top" >ENUM</td>
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 040cd83..2d7a9f5 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -6758,6 +6758,16 @@ enum skl_disp_power_wells {
 #define PIPE_CSC_POSTOFF_ME(pipe) _PIPE(pipe, _PIPE_A_CSC_POSTOFF_ME, _PIPE_B_CSC_POSTOFF_ME)
 #define PIPE_CSC_POSTOFF_LO(pipe) _PIPE(pipe, _PIPE_A_CSC_POSTOFF_LO, _PIPE_B_CSC_POSTOFF_LO)
 
+/* Skylake pipe bottom color */
+#define _PIPE_BOTTOM_COLOR_A        0x70034
+#define _PIPE_BOTTOM_COLOR_B        0x71034
+#define _PIPE_BOTTOM_COLOR_C        0x72034
+#define PIPE_BOTTOM_GAMMA_ENABLE   (1 << 31)
+#define PIPE_BOTTOM_CSC_ENABLE     (1 << 30)
+#define PIPE_BOTTOM_COLOR_MASK     0x3FFFFFFF
+#define PIPE_BOTTOM_COLOR(pipe) \
+	_PIPE(pipe, _PIPE_BOTTOM_COLOR_A, _PIPE_BOTTOM_COLOR_B)
+
 /* MIPI DSI registers */
 
 #define _MIPI_PORT(port, a, c)	_PORT3(port, a, 0, c)	/* ports A and C only */
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 3b0fe9f..4dc5cb2 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4413,6 +4413,20 @@ static void haswell_mode_set_planes_workaround(struct intel_crtc *crtc)
 	intel_wait_for_vblank(dev, other_active_crtc->pipe);
 }
 
+static void skl_crtc_set_background_color(struct intel_crtc *intel_crtc)
+{
+	struct drm_device *dev = intel_crtc->base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	if (INTEL_INFO(dev)->gen < 9)
+		return;
+
+	I915_WRITE(PIPE_BOTTOM_COLOR(intel_crtc->pipe),
+		intel_crtc->background_color |
+		PIPE_BOTTOM_GAMMA_ENABLE |
+		PIPE_BOTTOM_CSC_ENABLE);
+}
+
 static void haswell_crtc_enable(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
@@ -4474,6 +4488,8 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
 	 */
 	intel_crtc_load_lut(crtc);
 
+	skl_crtc_set_background_color(intel_crtc);
+
 	intel_ddi_set_pipe_settings(crtc);
 	intel_ddi_enable_transcoder_func(crtc);
 
@@ -9882,6 +9898,30 @@ out_hang:
 	return ret;
 }
 
+static int intel_crtc_set_property(struct drm_crtc *crtc,
+	struct drm_property *property, uint64_t val)
+{
+	struct drm_device *dev = crtc->dev;
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	int i;
+
+	if (INTEL_INFO(dev)->gen >= 9) {
+		if (property == dev->mode_config.background_color_property) {
+			uint64_t bottom = 0;
+
+			/* BGR 16bpc ==> RGB 10bpc */
+			for (i = 0; i < 3; i++)
+				bottom |= (((val >> (i * 16 + 6)) & 0x3FF) << ((2 - i) * 10));
+
+			intel_crtc->background_color = (uint32_t) bottom;
+
+			skl_crtc_set_background_color(intel_crtc);
+		}
+		return 0;
+	}
+	return -EINVAL;
+}
+
 static struct drm_crtc_helper_funcs intel_helper_funcs = {
 	.mode_set_base_atomic = intel_pipe_set_base_atomic,
 	.load_lut = intel_crtc_load_lut,
@@ -11660,6 +11700,7 @@ static const struct drm_crtc_funcs intel_crtc_funcs = {
 	.page_flip = intel_crtc_page_flip,
 	.atomic_duplicate_state = intel_crtc_duplicate_state,
 	.atomic_destroy_state = intel_crtc_destroy_state,
+	.set_property = intel_crtc_set_property,
 };
 
 static bool ibx_pch_dpll_get_hw_state(struct drm_i915_private *dev_priv,
@@ -12341,6 +12382,19 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
 	drm_crtc_helper_add(&intel_crtc->base, &intel_helper_funcs);
 
 	WARN_ON(drm_crtc_index(&intel_crtc->base) != intel_crtc->pipe);
+
+	/* background color = 48 bit quantity: MSB BGR 16bpc LSB */
+	if (INTEL_INFO(dev)->gen >= 9 &&
+		!dev->mode_config.background_color_property)
+		dev->mode_config.background_color_property =
+			drm_property_create_range(dev, 0, "background_color", 0,
+				0xFFFFFFFFFFFFULL);
+
+	if (dev->mode_config.background_color_property)
+		drm_object_attach_property(&intel_crtc->base.base,
+			dev->mode_config.background_color_property,
+			intel_crtc->background_color);
+
 	return;
 
 fail:
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 1de8e20..285f908 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -493,6 +493,8 @@ struct intel_crtc {
 	struct intel_mmio_flip mmio_flip;
 
 	struct intel_crtc_atomic_commit atomic;
+
+	uint32_t background_color;
 };
 
 struct intel_plane_wm_parameters {
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index b1465d6..5f8c80a 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -1122,6 +1122,9 @@ struct drm_mode_config {
 	struct drm_property *prop_crtc_id;
 	struct drm_property *prop_active;
 
+	/* crtc properties */
+	struct drm_property *background_color_property;
+
 	/* DVI-I properties */
 	struct drm_property *dvi_i_subconnector_property;
 	struct drm_property *dvi_i_select_subconnector_property;
-- 
1.7.9.5

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

Re: [PATCH v9] drm/i915/skl: Add support for SKL background color

[Daniel Vetter]

On Fri, Feb 20, 2015 at 04:11:33PM -0800, Chandra Konduru wrote:
> @@ -11660,6 +11700,7 @@ static const struct drm_crtc_funcs intel_crtc_funcs = {
>  	.page_flip = intel_crtc_page_flip,
>  	.atomic_duplicate_state = intel_crtc_duplicate_state,
>  	.atomic_destroy_state = intel_crtc_destroy_state,
> +	.set_property = intel_crtc_set_property,

So in my internals mail about whether atomic blocks this I've missed that
atm we can't do any atomic crtc properties. And I really don't want to
slow down the atomic conversion by piling up more legacy cruft first, so I
think properties on the crtc like this one will get stalled a bit despite
what I've claimed.

I guess we need to discuss this in our mtg tomorrow.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v9] drm/i915/skl: Add support for SKL background color

[Ville Syrjälä]

On Fri, Feb 20, 2015 at 04:11:33PM -0800, Chandra Konduru wrote:
> This patch adds support for Skylake display pipe background color.
> 
> v2:
> - added property documentation to drm DocBook (Daniel Vetter)
> - moved property to drm_mode_config (Daniel Vetter)
> - change to set property to NULL once it is freed. (me)
> - change to make sure gamma/csc settings were retained during color change (me)
> - change to make sure background color is restored after a power event (me)
> 
> v3:
> - change to add gen check before writing background color (Matt Roper)
> 
> v4:
> - change to increase color from 8bpc to 16bpc (Daniel Vetter)
> 
> v5:
> - removed initialization of background color to 0 (Damien Lespiau)
> - added change log to commit message (Damien Lespiau)
> 
> v6:
> - optmization to use _PIPE() (Damien Lespiau)
> - updated max limit value in DocBook (Damien Lespiau)
> - simplied color conversion equation (Damien Lespiau)
> - always enable gamma and csc to align with update plane (Damien Lespiau)
> - move resume restoration code to haswell_crtc_enable (Damien Lespiau)
> 
> v7:
> - remove superfluous comments (Damien)
> - remove unused variables left over between v5 and v6 (Damien)
> 
> v8:
> - remove the destruction of the property, DRM core does that for us (Damien)
> 
> v9:
> - add ULL to a 64bits constant
> 
> Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>
> Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> ---
>  Documentation/DocBook/drm.tmpl       |   10 ++++++-
>  drivers/gpu/drm/i915/i915_reg.h      |   10 +++++++
>  drivers/gpu/drm/i915/intel_display.c |   54 ++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h     |    2 ++
>  include/drm/drm_crtc.h               |    3 ++
>  5 files changed, 78 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
> index 249f0c9..c83ffe6 100644
> --- a/Documentation/DocBook/drm.tmpl
> +++ b/Documentation/DocBook/drm.tmpl
> @@ -2834,7 +2834,7 @@ void intel_crt_init(struct drm_device *dev)
>  	<td valign="top" >TBD</td>
>  	</tr>
>  	<tr>
> -	<td rowspan="21" valign="top" >i915</td>
> +	<td rowspan="22" valign="top" >i915</td>
>  	<td rowspan="2" valign="top" >Generic</td>
>  	<td valign="top" >"Broadcast RGB"</td>
>  	<td valign="top" >ENUM</td>
> @@ -2858,6 +2858,14 @@ void intel_crt_init(struct drm_device *dev)
>  	<td valign="top" >TBD</td>
>  	</tr>
>  	<tr>
> +	<td rowspan="1" valign="top" >CRTC</td>
> +	<td valign="top" >“background_color”</td>
> +	<td valign="top" >Range</td>
> +	<td valign="top" >Min=0, Max=0xFFFFFFFFFFFF</td>
> +	<td valign="top" >CRTC</td>
> +	<td valign="top" >Background color in 16bpc BGR (B-MSB, R-LSB)</td>

I'd prefer to settle on ARGB msb->lsb layout, unless there's already
precedent for something else. I think it's the most common component
order around, and so should result in least amount of code to stuff
the bits into the hardware registers.

> +	</tr>
> +	<tr>
>  	<td rowspan="17" valign="top" >SDVO-TV</td>
>  	<td valign="top" >“mode”</td>
>  	<td valign="top" >ENUM</td>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 040cd83..2d7a9f5 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -6758,6 +6758,16 @@ enum skl_disp_power_wells {
>  #define PIPE_CSC_POSTOFF_ME(pipe) _PIPE(pipe, _PIPE_A_CSC_POSTOFF_ME, _PIPE_B_CSC_POSTOFF_ME)
>  #define PIPE_CSC_POSTOFF_LO(pipe) _PIPE(pipe, _PIPE_A_CSC_POSTOFF_LO, _PIPE_B_CSC_POSTOFF_LO)
>  
> +/* Skylake pipe bottom color */
> +#define _PIPE_BOTTOM_COLOR_A        0x70034
> +#define _PIPE_BOTTOM_COLOR_B        0x71034
> +#define _PIPE_BOTTOM_COLOR_C        0x72034
> +#define PIPE_BOTTOM_GAMMA_ENABLE   (1 << 31)
> +#define PIPE_BOTTOM_CSC_ENABLE     (1 << 30)
> +#define PIPE_BOTTOM_COLOR_MASK     0x3FFFFFFF
> +#define PIPE_BOTTOM_COLOR(pipe) \
> +	_PIPE(pipe, _PIPE_BOTTOM_COLOR_A, _PIPE_BOTTOM_COLOR_B)
> +
>  /* MIPI DSI registers */
>  
>  #define _MIPI_PORT(port, a, c)	_PORT3(port, a, 0, c)	/* ports A and C only */
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 3b0fe9f..4dc5cb2 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4413,6 +4413,20 @@ static void haswell_mode_set_planes_workaround(struct intel_crtc *crtc)
>  	intel_wait_for_vblank(dev, other_active_crtc->pipe);
>  }
>  
> +static void skl_crtc_set_background_color(struct intel_crtc *intel_crtc)
> +{
> +	struct drm_device *dev = intel_crtc->base.dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +	if (INTEL_INFO(dev)->gen < 9)
> +		return;
> +
> +	I915_WRITE(PIPE_BOTTOM_COLOR(intel_crtc->pipe),
> +		intel_crtc->background_color |
> +		PIPE_BOTTOM_GAMMA_ENABLE |
> +		PIPE_BOTTOM_CSC_ENABLE);
> +}
> +
>  static void haswell_crtc_enable(struct drm_crtc *crtc)
>  {
>  	struct drm_device *dev = crtc->dev;
> @@ -4474,6 +4488,8 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
>  	 */
>  	intel_crtc_load_lut(crtc);
>  
> +	skl_crtc_set_background_color(intel_crtc);
> +
>  	intel_ddi_set_pipe_settings(crtc);
>  	intel_ddi_enable_transcoder_func(crtc);
>  
> @@ -9882,6 +9898,30 @@ out_hang:
>  	return ret;
>  }
>  
> +static int intel_crtc_set_property(struct drm_crtc *crtc,
> +	struct drm_property *property, uint64_t val)
> +{
> +	struct drm_device *dev = crtc->dev;
> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	int i;
> +
> +	if (INTEL_INFO(dev)->gen >= 9) {
> +		if (property == dev->mode_config.background_color_property) {
> +			uint64_t bottom = 0;
> +
> +			/* BGR 16bpc ==> RGB 10bpc */
> +			for (i = 0; i < 3; i++)
> +				bottom |= (((val >> (i * 16 + 6)) & 0x3FF) << ((2 - i) * 10));
> +
> +			intel_crtc->background_color = (uint32_t) bottom;
> +
> +			skl_crtc_set_background_color(intel_crtc);
> +		}
> +		return 0;
> +	}
> +	return -EINVAL;
> +}
> +
>  static struct drm_crtc_helper_funcs intel_helper_funcs = {
>  	.mode_set_base_atomic = intel_pipe_set_base_atomic,
>  	.load_lut = intel_crtc_load_lut,
> @@ -11660,6 +11700,7 @@ static const struct drm_crtc_funcs intel_crtc_funcs = {
>  	.page_flip = intel_crtc_page_flip,
>  	.atomic_duplicate_state = intel_crtc_duplicate_state,
>  	.atomic_destroy_state = intel_crtc_destroy_state,
> +	.set_property = intel_crtc_set_property,
>  };
>  
>  static bool ibx_pch_dpll_get_hw_state(struct drm_i915_private *dev_priv,
> @@ -12341,6 +12382,19 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
>  	drm_crtc_helper_add(&intel_crtc->base, &intel_helper_funcs);
>  
>  	WARN_ON(drm_crtc_index(&intel_crtc->base) != intel_crtc->pipe);
> +
> +	/* background color = 48 bit quantity: MSB BGR 16bpc LSB */
> +	if (INTEL_INFO(dev)->gen >= 9 &&
> +		!dev->mode_config.background_color_property)
> +		dev->mode_config.background_color_property =
> +			drm_property_create_range(dev, 0, "background_color", 0,
> +				0xFFFFFFFFFFFFULL);
> +
> +	if (dev->mode_config.background_color_property)
> +		drm_object_attach_property(&intel_crtc->base.base,
> +			dev->mode_config.background_color_property,
> +			intel_crtc->background_color);
> +
>  	return;
>  
>  fail:
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 1de8e20..285f908 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -493,6 +493,8 @@ struct intel_crtc {
>  	struct intel_mmio_flip mmio_flip;
>  
>  	struct intel_crtc_atomic_commit atomic;
> +
> +	uint32_t background_color;
>  };
>  
>  struct intel_plane_wm_parameters {
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index b1465d6..5f8c80a 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -1122,6 +1122,9 @@ struct drm_mode_config {
>  	struct drm_property *prop_crtc_id;
>  	struct drm_property *prop_active;
>  
> +	/* crtc properties */
> +	struct drm_property *background_color_property;
> +
>  	/* DVI-I properties */
>  	struct drm_property *dvi_i_subconnector_property;
>  	struct drm_property *dvi_i_select_subconnector_property;
> -- 
> 1.7.9.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH v9] drm/i915/skl: Add support for SKL background color

[Konduru, Chandra]

> -----Original Message-----
> From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> Sent: Monday, April 27, 2015 8:28 AM
> On Fri, Feb 20, 2015 at 04:11:33PM -0800, Chandra Konduru wrote:
> > This patch adds support for Skylake display pipe background color.
> > +	<td valign="top" >Background color in 16bpc BGR (B-MSB, R-LSB)</td>
> 
> I'd prefer to settle on ARGB msb->lsb layout, unless there's already precedent
> for something else. I think it's the most common component order around, and
> so should result in least amount of code to stuff the bits into the hardware
> registers.

To give some background on this, initially using ARGB msb->lsb layout to stuff
bit into hw regs, but changed to BGR msb->lsb per feedback. Before reversing/
respunning, would like to have an agreement. Daniel/Damien, are you ok with 
reversing the order to align with hw? Or just keep the way current patch doing?

> 
> > +static int intel_crtc_set_property(struct drm_crtc *crtc,
> > +	struct drm_property *property, uint64_t val) {
> > +	struct drm_device *dev = crtc->dev;
> > +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > +	int i;
> > +
> > +	if (INTEL_INFO(dev)->gen >= 9) {
> > +		if (property == dev->mode_config.background_color_property)
> {
> > +			uint64_t bottom = 0;
> > +
> > +			/* BGR 16bpc ==> RGB 10bpc */
> > +			for (i = 0; i < 3; i++)
> > +				bottom |= (((val >> (i * 16 + 6)) & 0x3FF) << ((2 -
> i) * 10));
> > +
> > +			intel_crtc->background_color = (uint32_t) bottom;
> > +
> > +			skl_crtc_set_background_color(intel_crtc);
> > +		}
> > +		return 0;
> > +	}
> > +	return -EINVAL;
> > +}

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

Re: [PATCH v9] drm/i915/skl: Add support for SKL background color

[Daniel Vetter]

On Mon, Apr 27, 2015 at 08:33:38PM +0000, Konduru, Chandra wrote:
> > -----Original Message-----
> > From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> > Sent: Monday, April 27, 2015 8:28 AM
> > On Fri, Feb 20, 2015 at 04:11:33PM -0800, Chandra Konduru wrote:
> > > This patch adds support for Skylake display pipe background color.
> > > +	<td valign="top" >Background color in 16bpc BGR (B-MSB, R-LSB)</td>
> > 
> > I'd prefer to settle on ARGB msb->lsb layout, unless there's already precedent
> > for something else. I think it's the most common component order around, and
> > so should result in least amount of code to stuff the bits into the hardware
> > registers.
> 
> To give some background on this, initially using ARGB msb->lsb layout to stuff
> bit into hw regs, but changed to BGR msb->lsb per feedback. Before reversing/
> respunning, would like to have an agreement. Daniel/Damien, are you ok with 
> reversing the order to align with hw? Or just keep the way current patch doing?

Imo if we want to really end up with a real standard we should have a
macro to encode ARGB values and 4 macros to extract the individual parts
in some uapi header and use that instead of open-coding shifts. That would
need to be a drm core header ofc. The exact layout is probably irrelevant
anyway.
-Daniel

> 
> > 
> > > +static int intel_crtc_set_property(struct drm_crtc *crtc,
> > > +	struct drm_property *property, uint64_t val) {
> > > +	struct drm_device *dev = crtc->dev;
> > > +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > > +	int i;
> > > +
> > > +	if (INTEL_INFO(dev)->gen >= 9) {
> > > +		if (property == dev->mode_config.background_color_property)
> > {
> > > +			uint64_t bottom = 0;
> > > +
> > > +			/* BGR 16bpc ==> RGB 10bpc */
> > > +			for (i = 0; i < 3; i++)
> > > +				bottom |= (((val >> (i * 16 + 6)) & 0x3FF) << ((2 -
> > i) * 10));
> > > +
> > > +			intel_crtc->background_color = (uint32_t) bottom;
> > > +
> > > +			skl_crtc_set_background_color(intel_crtc);
> > > +		}
> > > +		return 0;
> > > +	}
> > > +	return -EINVAL;
> > > +}
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://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
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v9] drm/i915/skl: Add support for SKL background color

[Ville Syrjälä]

On Mon, May 04, 2015 at 09:57:17AM +0200, Daniel Vetter wrote:
> On Mon, Apr 27, 2015 at 08:33:38PM +0000, Konduru, Chandra wrote:
> > > -----Original Message-----
> > > From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> > > Sent: Monday, April 27, 2015 8:28 AM
> > > On Fri, Feb 20, 2015 at 04:11:33PM -0800, Chandra Konduru wrote:
> > > > This patch adds support for Skylake display pipe background color.
> > > > +	<td valign="top" >Background color in 16bpc BGR (B-MSB, R-LSB)</td>
> > > 
> > > I'd prefer to settle on ARGB msb->lsb layout, unless there's already precedent
> > > for something else. I think it's the most common component order around, and
> > > so should result in least amount of code to stuff the bits into the hardware
> > > registers.
> > 
> > To give some background on this, initially using ARGB msb->lsb layout to stuff
> > bit into hw regs, but changed to BGR msb->lsb per feedback. Before reversing/
> > respunning, would like to have an agreement. Daniel/Damien, are you ok with 
> > reversing the order to align with hw? Or just keep the way current patch doing?
> 
> Imo if we want to really end up with a real standard we should have a
> macro to encode ARGB values and 4 macros to extract the individual parts
> in some uapi header and use that instead of open-coding shifts. That would
> need to be a drm core header ofc.

Sounds like a decent idea.

> The exact layout is probably irrelevant
> anyway.

I think going with (A)RGB makes it much easier to parse when you're
looking at the hex string. That's the order those things usually come
in.

Which brings another point to mind. Should we define a new property type
for color data? Would make it easier to pretty-print in userland.
Using decimal for color data isn't very useful after all.

> -Daniel
> 
> > 
> > > 
> > > > +static int intel_crtc_set_property(struct drm_crtc *crtc,
> > > > +	struct drm_property *property, uint64_t val) {
> > > > +	struct drm_device *dev = crtc->dev;
> > > > +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > > > +	int i;
> > > > +
> > > > +	if (INTEL_INFO(dev)->gen >= 9) {
> > > > +		if (property == dev->mode_config.background_color_property)
> > > {
> > > > +			uint64_t bottom = 0;
> > > > +
> > > > +			/* BGR 16bpc ==> RGB 10bpc */
> > > > +			for (i = 0; i < 3; i++)
> > > > +				bottom |= (((val >> (i * 16 + 6)) & 0x3FF) << ((2 -
> > > i) * 10));
> > > > +
> > > > +			intel_crtc->background_color = (uint32_t) bottom;
> > > > +
> > > > +			skl_crtc_set_background_color(intel_crtc);
> > > > +		}
> > > > +		return 0;
> > > > +	}
> > > > +	return -EINVAL;
> > > > +}
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

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

Re: [PATCH v9] drm/i915/skl: Add support for SKL background color

[Daniel Vetter]

On Tue, May 5, 2015 at 4:33 PM, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
> Which brings another point to mind. Should we define a new property type
> for color data? Would make it easier to pretty-print in userland.
> Using decimal for color data isn't very useful after all.

Maybe a hex type might be useful, but not even sure about that one.
I'd expect a proliferation of various types if we go as fine-grained
as color at least.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx