[08/13] drm/i915: Populate gamma_mode for all platforms

Submitted by Ville Syrjälä on Jan. 11, 2019, 5:08 p.m.

Details

Message ID 20190111170823.4441-9-ville.syrjala@linux.intel.com
State New
Headers show
Series "Enable/disable gamma/csc dynamically and fix C8" ( rev: 1 ) in Intel GFX

Not browsing as part of any series.

Commit Message

Ville Syrjälä Jan. 11, 2019, 5:08 p.m.
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

On pre-HSW gamma mode is configured via PIPECONF. The bits are
the same except shifted up, so we can reuse just store them in
crtc_state->gamma_mode in the HSW+ way, allowing us to share
some code later.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h      | 10 ++++-
 drivers/gpu/drm/i915/intel_color.c   | 60 +++++++++++++++++++++-------
 drivers/gpu/drm/i915/intel_display.c | 14 ++++++-
 3 files changed, 66 insertions(+), 18 deletions(-)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 44958d994bfa..9d17ba199be4 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -5578,9 +5578,15 @@  enum {
 #define   PIPECONF_SINGLE_WIDE	0
 #define   PIPECONF_PIPE_UNLOCKED 0
 #define   PIPECONF_PIPE_LOCKED	(1 << 25)
-#define   PIPECONF_PALETTE	0
-#define   PIPECONF_GAMMA		(1 << 24)
 #define   PIPECONF_FORCE_BORDER	(1 << 25)
+#define   PIPECONF_GAMMA_MODE_MASK_I9XX	(1 << 24) /* gmch */
+#define   PIPECONF_GAMMA_MODE_MASK_ILK	(3 << 24) /* ilk-ivb */
+#define   PIPECONF_GAMMA_MODE_8BIT	(0 << 24) /* gmch,ilk-ivb */
+#define   PIPECONF_GAMMA_MODE_10BIT	(1 << 24) /* gmch,ilk-ivb */
+#define   PIPECONF_GAMMA_MODE_12BIT	(2 << 24) /* ilk-ivb */
+#define   PIPECONF_GAMMA_MODE_SPLIT	(3 << 24) /* ivb */
+#define   PIPECONF_GAMMA_MODE(x)	((x)<<24) /* pass in GAMMA_MODE_MODE_* */
+#define   PIPECONF_GAMMA_MODE_SHIFT	24
 #define   PIPECONF_INTERLACE_MASK	(7 << 21)
 #define   PIPECONF_INTERLACE_MASK_HSW	(3 << 21)
 /* Note that pre-gen3 does not support interlaced display directly. Panel
diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
index 0c0da7ed0fd7..6fdbfa8c4008 100644
--- a/drivers/gpu/drm/i915/intel_color.c
+++ b/drivers/gpu/drm/i915/intel_color.c
@@ -351,6 +351,32 @@  static void i9xx_load_luts(const struct intel_crtc_state *crtc_state)
 	i9xx_load_luts_internal(crtc_state, crtc_state->base.gamma_lut);
 }
 
+static void i9xx_color_commit(const struct intel_crtc_state *crtc_state)
+{
+	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
+	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
+	enum pipe pipe = crtc->pipe;
+	u32 val;
+
+	val = I915_READ(PIPECONF(pipe));
+	val &= ~PIPECONF_GAMMA_MODE_MASK_I9XX;
+	val |= PIPECONF_GAMMA_MODE(crtc_state->gamma_mode);
+	I915_WRITE(PIPECONF(pipe), val);
+}
+
+static void ilk_color_commit(const struct intel_crtc_state *crtc_state)
+{
+	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
+	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
+	enum pipe pipe = crtc->pipe;
+	u32 val;
+
+	val = I915_READ(PIPECONF(pipe));
+	val &= ~PIPECONF_GAMMA_MODE_MASK_ILK;
+	val |= PIPECONF_GAMMA_MODE(crtc_state->gamma_mode);
+	I915_WRITE(PIPECONF(pipe), val);
+}
+
 static void hsw_color_commit(const struct intel_crtc_state *crtc_state)
 {
 	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
@@ -585,8 +611,7 @@  void intel_color_commit(const struct intel_crtc_state *crtc_state)
 {
 	struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev);
 
-	if (dev_priv->display.color_commit)
-		dev_priv->display.color_commit(crtc_state);
+	dev_priv->display.color_commit(crtc_state);
 }
 
 int intel_color_check(struct intel_crtc_state *crtc_state)
@@ -634,20 +659,25 @@  void intel_color_init(struct intel_crtc *crtc)
 
 	drm_mode_crtc_set_gamma_size(&crtc->base, 256);
 
-	if (IS_CHERRYVIEW(dev_priv)) {
-		dev_priv->display.load_luts = cherryview_load_luts;
-	} else if (IS_HASWELL(dev_priv)) {
-		dev_priv->display.load_luts = i9xx_load_luts;
-		dev_priv->display.color_commit = hsw_color_commit;
-	} else if (IS_BROADWELL(dev_priv) || IS_GEN9_BC(dev_priv) ||
-		   IS_BROXTON(dev_priv)) {
-		dev_priv->display.load_luts = broadwell_load_luts;
-		dev_priv->display.color_commit = hsw_color_commit;
-	} else if (IS_GEMINILAKE(dev_priv) || IS_CANNONLAKE(dev_priv)) {
-		dev_priv->display.load_luts = glk_load_luts;
-		dev_priv->display.color_commit = hsw_color_commit;
+	if (HAS_GMCH_DISPLAY(dev_priv)) {
+		if (IS_CHERRYVIEW(dev_priv))
+			dev_priv->display.load_luts = cherryview_load_luts;
+		else
+			dev_priv->display.load_luts = i9xx_load_luts;
+
+		dev_priv->display.color_commit = i9xx_color_commit;
 	} else {
-		dev_priv->display.load_luts = i9xx_load_luts;
+		if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv))
+			dev_priv->display.load_luts = glk_load_luts;
+		else if (INTEL_GEN(dev_priv) >= 9 || IS_BROADWELL(dev_priv))
+			dev_priv->display.load_luts = broadwell_load_luts;
+		else
+			dev_priv->display.load_luts = i9xx_load_luts;
+
+		if (INTEL_GEN(dev_priv) >= 8 || IS_HASWELL(dev_priv))
+			dev_priv->display.color_commit = hsw_color_commit;
+		else
+			dev_priv->display.color_commit = ilk_color_commit;
 	}
 
 	/* Enable color management support when we have degamma & gamma LUTs. */
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 1caee4128974..90afcae91b30 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3415,7 +3415,7 @@  static void i9xx_disable_plane(struct intel_plane *plane,
 	 *
 	 * On pre-g4x there is no way to gamma correct the
 	 * pipe bottom color but we'll keep on doing this
-	 * anyway.
+	 * anyway so that the crtc state readout works correctly.
 	 */
 	dspcntr = i9xx_plane_ctl_crtc(crtc_state);
 
@@ -7627,6 +7627,8 @@  static void i9xx_set_pipeconf(const struct intel_crtc_state *crtc_state)
 	     crtc_state->limited_color_range)
 		pipeconf |= PIPECONF_COLOR_RANGE_SELECT;
 
+	pipeconf |= PIPECONF_GAMMA_MODE(crtc_state->gamma_mode);
+
 	I915_WRITE(PIPECONF(crtc->pipe), pipeconf);
 	POSTING_READ(PIPECONF(crtc->pipe));
 }
@@ -8077,6 +8079,9 @@  static bool i9xx_get_pipe_config(struct intel_crtc *crtc,
 	    (tmp & PIPECONF_COLOR_RANGE_SELECT))
 		pipe_config->limited_color_range = true;
 
+	pipe_config->gamma_mode = (tmp & PIPECONF_GAMMA_MODE_MASK_I9XX) >>
+		PIPECONF_GAMMA_MODE_SHIFT;
+
 	if (INTEL_GEN(dev_priv) < 4)
 		pipe_config->double_wide = tmp & PIPECONF_DOUBLE_WIDE;
 
@@ -8616,6 +8621,8 @@  static void ironlake_set_pipeconf(const struct intel_crtc_state *crtc_state)
 	if (crtc_state->limited_color_range)
 		val |= PIPECONF_COLOR_RANGE_SELECT;
 
+	val |= PIPECONF_GAMMA_MODE(crtc_state->gamma_mode);
+
 	I915_WRITE(PIPECONF(pipe), val);
 	POSTING_READ(PIPECONF(pipe));
 }
@@ -9147,6 +9154,9 @@  static bool ironlake_get_pipe_config(struct intel_crtc *crtc,
 	if (tmp & PIPECONF_COLOR_RANGE_SELECT)
 		pipe_config->limited_color_range = true;
 
+	pipe_config->gamma_mode = (tmp & PIPECONF_GAMMA_MODE_MASK_ILK) >>
+		PIPECONF_GAMMA_MODE_SHIFT;
+
 	if (I915_READ(PCH_TRANSCONF(crtc->pipe)) & TRANS_ENABLE) {
 		struct intel_shared_dpll *pll;
 		enum intel_dpll_id pll_id;
@@ -11977,6 +11987,8 @@  intel_pipe_config_compare(struct drm_i915_private *dev_priv,
 
 	PIPE_CONF_CHECK_BOOL(double_wide);
 
+	PIPE_CONF_CHECK_X(gamma_mode);
+
 	PIPE_CONF_CHECK_P(shared_dpll);
 	PIPE_CONF_CHECK_X(dpll_hw_state.dpll);
 	PIPE_CONF_CHECK_X(dpll_hw_state.dpll_md);

Comments

On Fri, Jan 11, 2019 at 07:08:18PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> On pre-HSW gamma mode is configured via PIPECONF. The bits are
> the same except shifted up, so we can reuse just store them in
> crtc_state->gamma_mode in the HSW+ way, allowing us to share
> some code later.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h      | 10 ++++-
>  drivers/gpu/drm/i915/intel_color.c   | 60 +++++++++++++++++++++-------
>  drivers/gpu/drm/i915/intel_display.c | 14 ++++++-
>  3 files changed, 66 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 44958d994bfa..9d17ba199be4 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -5578,9 +5578,15 @@ enum {
>  #define   PIPECONF_SINGLE_WIDE	0
>  #define   PIPECONF_PIPE_UNLOCKED 0
>  #define   PIPECONF_PIPE_LOCKED	(1 << 25)
> -#define   PIPECONF_PALETTE	0
> -#define   PIPECONF_GAMMA		(1 << 24)
>  #define   PIPECONF_FORCE_BORDER	(1 << 25)
> +#define   PIPECONF_GAMMA_MODE_MASK_I9XX	(1 << 24) /* gmch */
> +#define   PIPECONF_GAMMA_MODE_MASK_ILK	(3 << 24) /* ilk-ivb */
> +#define   PIPECONF_GAMMA_MODE_8BIT	(0 << 24) /* gmch,ilk-ivb */
> +#define   PIPECONF_GAMMA_MODE_10BIT	(1 << 24) /* gmch,ilk-ivb */
> +#define   PIPECONF_GAMMA_MODE_12BIT	(2 << 24) /* ilk-ivb */
> +#define   PIPECONF_GAMMA_MODE_SPLIT	(3 << 24) /* ivb */
> +#define   PIPECONF_GAMMA_MODE(x)	((x)<<24) /* pass in GAMMA_MODE_MODE_* */
> +#define   PIPECONF_GAMMA_MODE_SHIFT	24
>  #define   PIPECONF_INTERLACE_MASK	(7 << 21)
>  #define   PIPECONF_INTERLACE_MASK_HSW	(3 << 21)
>  /* Note that pre-gen3 does not support interlaced display directly. Panel
> diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
> index 0c0da7ed0fd7..6fdbfa8c4008 100644
> --- a/drivers/gpu/drm/i915/intel_color.c
> +++ b/drivers/gpu/drm/i915/intel_color.c
> @@ -351,6 +351,32 @@ static void i9xx_load_luts(const struct intel_crtc_state *crtc_state)
>  	i9xx_load_luts_internal(crtc_state, crtc_state->base.gamma_lut);
>  }
>  
> +static void i9xx_color_commit(const struct intel_crtc_state *crtc_state)
> +{
> +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> +	enum pipe pipe = crtc->pipe;
> +	u32 val;
> +
> +	val = I915_READ(PIPECONF(pipe));
> +	val &= ~PIPECONF_GAMMA_MODE_MASK_I9XX;
> +	val |= PIPECONF_GAMMA_MODE(crtc_state->gamma_mode);
> +	I915_WRITE(PIPECONF(pipe), val);
> +}
> +
> +static void ilk_color_commit(const struct intel_crtc_state *crtc_state)
> +{
> +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> +	enum pipe pipe = crtc->pipe;
> +	u32 val;
> +
> +	val = I915_READ(PIPECONF(pipe));
> +	val &= ~PIPECONF_GAMMA_MODE_MASK_ILK;
> +	val |= PIPECONF_GAMMA_MODE(crtc_state->gamma_mode);
> +	I915_WRITE(PIPECONF(pipe), val);
> +}

Could we just set color_commit to i9xx_set_pipeconf and
ironlake_set_pipeconf to handle these without the r-m-w?


Matt

> +
>  static void hsw_color_commit(const struct intel_crtc_state *crtc_state)
>  {
>  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> @@ -585,8 +611,7 @@ void intel_color_commit(const struct intel_crtc_state *crtc_state)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev);
>  
> -	if (dev_priv->display.color_commit)
> -		dev_priv->display.color_commit(crtc_state);
> +	dev_priv->display.color_commit(crtc_state);
>  }
>  
>  int intel_color_check(struct intel_crtc_state *crtc_state)
> @@ -634,20 +659,25 @@ void intel_color_init(struct intel_crtc *crtc)
>  
>  	drm_mode_crtc_set_gamma_size(&crtc->base, 256);
>  
> -	if (IS_CHERRYVIEW(dev_priv)) {
> -		dev_priv->display.load_luts = cherryview_load_luts;
> -	} else if (IS_HASWELL(dev_priv)) {
> -		dev_priv->display.load_luts = i9xx_load_luts;
> -		dev_priv->display.color_commit = hsw_color_commit;
> -	} else if (IS_BROADWELL(dev_priv) || IS_GEN9_BC(dev_priv) ||
> -		   IS_BROXTON(dev_priv)) {
> -		dev_priv->display.load_luts = broadwell_load_luts;
> -		dev_priv->display.color_commit = hsw_color_commit;
> -	} else if (IS_GEMINILAKE(dev_priv) || IS_CANNONLAKE(dev_priv)) {
> -		dev_priv->display.load_luts = glk_load_luts;
> -		dev_priv->display.color_commit = hsw_color_commit;
> +	if (HAS_GMCH_DISPLAY(dev_priv)) {
> +		if (IS_CHERRYVIEW(dev_priv))
> +			dev_priv->display.load_luts = cherryview_load_luts;
> +		else
> +			dev_priv->display.load_luts = i9xx_load_luts;
> +
> +		dev_priv->display.color_commit = i9xx_color_commit;
>  	} else {
> -		dev_priv->display.load_luts = i9xx_load_luts;
> +		if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv))
> +			dev_priv->display.load_luts = glk_load_luts;
> +		else if (INTEL_GEN(dev_priv) >= 9 || IS_BROADWELL(dev_priv))
> +			dev_priv->display.load_luts = broadwell_load_luts;
> +		else
> +			dev_priv->display.load_luts = i9xx_load_luts;
> +
> +		if (INTEL_GEN(dev_priv) >= 8 || IS_HASWELL(dev_priv))
> +			dev_priv->display.color_commit = hsw_color_commit;
> +		else
> +			dev_priv->display.color_commit = ilk_color_commit;
>  	}
>  
>  	/* Enable color management support when we have degamma & gamma LUTs. */
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 1caee4128974..90afcae91b30 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3415,7 +3415,7 @@ static void i9xx_disable_plane(struct intel_plane *plane,
>  	 *
>  	 * On pre-g4x there is no way to gamma correct the
>  	 * pipe bottom color but we'll keep on doing this
> -	 * anyway.
> +	 * anyway so that the crtc state readout works correctly.
>  	 */
>  	dspcntr = i9xx_plane_ctl_crtc(crtc_state);
>  
> @@ -7627,6 +7627,8 @@ static void i9xx_set_pipeconf(const struct intel_crtc_state *crtc_state)
>  	     crtc_state->limited_color_range)
>  		pipeconf |= PIPECONF_COLOR_RANGE_SELECT;
>  
> +	pipeconf |= PIPECONF_GAMMA_MODE(crtc_state->gamma_mode);
> +
>  	I915_WRITE(PIPECONF(crtc->pipe), pipeconf);
>  	POSTING_READ(PIPECONF(crtc->pipe));
>  }
> @@ -8077,6 +8079,9 @@ static bool i9xx_get_pipe_config(struct intel_crtc *crtc,
>  	    (tmp & PIPECONF_COLOR_RANGE_SELECT))
>  		pipe_config->limited_color_range = true;
>  
> +	pipe_config->gamma_mode = (tmp & PIPECONF_GAMMA_MODE_MASK_I9XX) >>
> +		PIPECONF_GAMMA_MODE_SHIFT;
> +
>  	if (INTEL_GEN(dev_priv) < 4)
>  		pipe_config->double_wide = tmp & PIPECONF_DOUBLE_WIDE;
>  
> @@ -8616,6 +8621,8 @@ static void ironlake_set_pipeconf(const struct intel_crtc_state *crtc_state)
>  	if (crtc_state->limited_color_range)
>  		val |= PIPECONF_COLOR_RANGE_SELECT;
>  
> +	val |= PIPECONF_GAMMA_MODE(crtc_state->gamma_mode);
> +
>  	I915_WRITE(PIPECONF(pipe), val);
>  	POSTING_READ(PIPECONF(pipe));
>  }
> @@ -9147,6 +9154,9 @@ static bool ironlake_get_pipe_config(struct intel_crtc *crtc,
>  	if (tmp & PIPECONF_COLOR_RANGE_SELECT)
>  		pipe_config->limited_color_range = true;
>  
> +	pipe_config->gamma_mode = (tmp & PIPECONF_GAMMA_MODE_MASK_ILK) >>
> +		PIPECONF_GAMMA_MODE_SHIFT;
> +
>  	if (I915_READ(PCH_TRANSCONF(crtc->pipe)) & TRANS_ENABLE) {
>  		struct intel_shared_dpll *pll;
>  		enum intel_dpll_id pll_id;
> @@ -11977,6 +11987,8 @@ intel_pipe_config_compare(struct drm_i915_private *dev_priv,
>  
>  	PIPE_CONF_CHECK_BOOL(double_wide);
>  
> +	PIPE_CONF_CHECK_X(gamma_mode);
> +
>  	PIPE_CONF_CHECK_P(shared_dpll);
>  	PIPE_CONF_CHECK_X(dpll_hw_state.dpll);
>  	PIPE_CONF_CHECK_X(dpll_hw_state.dpll_md);
> -- 
> 2.19.2
>
On Wed, Jan 16, 2019 at 10:31:56AM -0800, Matt Roper wrote:
> On Fri, Jan 11, 2019 at 07:08:18PM +0200, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > On pre-HSW gamma mode is configured via PIPECONF. The bits are
> > the same except shifted up, so we can reuse just store them in
> > crtc_state->gamma_mode in the HSW+ way, allowing us to share
> > some code later.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_reg.h      | 10 ++++-
> >  drivers/gpu/drm/i915/intel_color.c   | 60 +++++++++++++++++++++-------
> >  drivers/gpu/drm/i915/intel_display.c | 14 ++++++-
> >  3 files changed, 66 insertions(+), 18 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index 44958d994bfa..9d17ba199be4 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -5578,9 +5578,15 @@ enum {
> >  #define   PIPECONF_SINGLE_WIDE	0
> >  #define   PIPECONF_PIPE_UNLOCKED 0
> >  #define   PIPECONF_PIPE_LOCKED	(1 << 25)
> > -#define   PIPECONF_PALETTE	0
> > -#define   PIPECONF_GAMMA		(1 << 24)
> >  #define   PIPECONF_FORCE_BORDER	(1 << 25)
> > +#define   PIPECONF_GAMMA_MODE_MASK_I9XX	(1 << 24) /* gmch */
> > +#define   PIPECONF_GAMMA_MODE_MASK_ILK	(3 << 24) /* ilk-ivb */
> > +#define   PIPECONF_GAMMA_MODE_8BIT	(0 << 24) /* gmch,ilk-ivb */
> > +#define   PIPECONF_GAMMA_MODE_10BIT	(1 << 24) /* gmch,ilk-ivb */
> > +#define   PIPECONF_GAMMA_MODE_12BIT	(2 << 24) /* ilk-ivb */
> > +#define   PIPECONF_GAMMA_MODE_SPLIT	(3 << 24) /* ivb */
> > +#define   PIPECONF_GAMMA_MODE(x)	((x)<<24) /* pass in GAMMA_MODE_MODE_* */
> > +#define   PIPECONF_GAMMA_MODE_SHIFT	24
> >  #define   PIPECONF_INTERLACE_MASK	(7 << 21)
> >  #define   PIPECONF_INTERLACE_MASK_HSW	(3 << 21)
> >  /* Note that pre-gen3 does not support interlaced display directly. Panel
> > diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
> > index 0c0da7ed0fd7..6fdbfa8c4008 100644
> > --- a/drivers/gpu/drm/i915/intel_color.c
> > +++ b/drivers/gpu/drm/i915/intel_color.c
> > @@ -351,6 +351,32 @@ static void i9xx_load_luts(const struct intel_crtc_state *crtc_state)
> >  	i9xx_load_luts_internal(crtc_state, crtc_state->base.gamma_lut);
> >  }
> >  
> > +static void i9xx_color_commit(const struct intel_crtc_state *crtc_state)
> > +{
> > +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> > +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> > +	enum pipe pipe = crtc->pipe;
> > +	u32 val;
> > +
> > +	val = I915_READ(PIPECONF(pipe));
> > +	val &= ~PIPECONF_GAMMA_MODE_MASK_I9XX;
> > +	val |= PIPECONF_GAMMA_MODE(crtc_state->gamma_mode);
> > +	I915_WRITE(PIPECONF(pipe), val);
> > +}
> > +
> > +static void ilk_color_commit(const struct intel_crtc_state *crtc_state)
> > +{
> > +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> > +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> > +	enum pipe pipe = crtc->pipe;
> > +	u32 val;
> > +
> > +	val = I915_READ(PIPECONF(pipe));
> > +	val &= ~PIPECONF_GAMMA_MODE_MASK_ILK;
> > +	val |= PIPECONF_GAMMA_MODE(crtc_state->gamma_mode);
> > +	I915_WRITE(PIPECONF(pipe), val);
> > +}
> 
> Could we just set color_commit to i9xx_set_pipeconf and
> ironlake_set_pipeconf to handle these without the r-m-w?

Perhaps. But not quite sure if we have any magic restrictions
in the crtc enable sequence that would prevent that.

> 
> 
> Matt
> 
> > +
> >  static void hsw_color_commit(const struct intel_crtc_state *crtc_state)
> >  {
> >  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> > @@ -585,8 +611,7 @@ void intel_color_commit(const struct intel_crtc_state *crtc_state)
> >  {
> >  	struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev);
> >  
> > -	if (dev_priv->display.color_commit)
> > -		dev_priv->display.color_commit(crtc_state);
> > +	dev_priv->display.color_commit(crtc_state);
> >  }
> >  
> >  int intel_color_check(struct intel_crtc_state *crtc_state)
> > @@ -634,20 +659,25 @@ void intel_color_init(struct intel_crtc *crtc)
> >  
> >  	drm_mode_crtc_set_gamma_size(&crtc->base, 256);
> >  
> > -	if (IS_CHERRYVIEW(dev_priv)) {
> > -		dev_priv->display.load_luts = cherryview_load_luts;
> > -	} else if (IS_HASWELL(dev_priv)) {
> > -		dev_priv->display.load_luts = i9xx_load_luts;
> > -		dev_priv->display.color_commit = hsw_color_commit;
> > -	} else if (IS_BROADWELL(dev_priv) || IS_GEN9_BC(dev_priv) ||
> > -		   IS_BROXTON(dev_priv)) {
> > -		dev_priv->display.load_luts = broadwell_load_luts;
> > -		dev_priv->display.color_commit = hsw_color_commit;
> > -	} else if (IS_GEMINILAKE(dev_priv) || IS_CANNONLAKE(dev_priv)) {
> > -		dev_priv->display.load_luts = glk_load_luts;
> > -		dev_priv->display.color_commit = hsw_color_commit;
> > +	if (HAS_GMCH_DISPLAY(dev_priv)) {
> > +		if (IS_CHERRYVIEW(dev_priv))
> > +			dev_priv->display.load_luts = cherryview_load_luts;
> > +		else
> > +			dev_priv->display.load_luts = i9xx_load_luts;
> > +
> > +		dev_priv->display.color_commit = i9xx_color_commit;
> >  	} else {
> > -		dev_priv->display.load_luts = i9xx_load_luts;
> > +		if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv))
> > +			dev_priv->display.load_luts = glk_load_luts;
> > +		else if (INTEL_GEN(dev_priv) >= 9 || IS_BROADWELL(dev_priv))
> > +			dev_priv->display.load_luts = broadwell_load_luts;
> > +		else
> > +			dev_priv->display.load_luts = i9xx_load_luts;
> > +
> > +		if (INTEL_GEN(dev_priv) >= 8 || IS_HASWELL(dev_priv))
> > +			dev_priv->display.color_commit = hsw_color_commit;
> > +		else
> > +			dev_priv->display.color_commit = ilk_color_commit;
> >  	}
> >  
> >  	/* Enable color management support when we have degamma & gamma LUTs. */
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 1caee4128974..90afcae91b30 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -3415,7 +3415,7 @@ static void i9xx_disable_plane(struct intel_plane *plane,
> >  	 *
> >  	 * On pre-g4x there is no way to gamma correct the
> >  	 * pipe bottom color but we'll keep on doing this
> > -	 * anyway.
> > +	 * anyway so that the crtc state readout works correctly.
> >  	 */
> >  	dspcntr = i9xx_plane_ctl_crtc(crtc_state);
> >  
> > @@ -7627,6 +7627,8 @@ static void i9xx_set_pipeconf(const struct intel_crtc_state *crtc_state)
> >  	     crtc_state->limited_color_range)
> >  		pipeconf |= PIPECONF_COLOR_RANGE_SELECT;
> >  
> > +	pipeconf |= PIPECONF_GAMMA_MODE(crtc_state->gamma_mode);
> > +
> >  	I915_WRITE(PIPECONF(crtc->pipe), pipeconf);
> >  	POSTING_READ(PIPECONF(crtc->pipe));
> >  }
> > @@ -8077,6 +8079,9 @@ static bool i9xx_get_pipe_config(struct intel_crtc *crtc,
> >  	    (tmp & PIPECONF_COLOR_RANGE_SELECT))
> >  		pipe_config->limited_color_range = true;
> >  
> > +	pipe_config->gamma_mode = (tmp & PIPECONF_GAMMA_MODE_MASK_I9XX) >>
> > +		PIPECONF_GAMMA_MODE_SHIFT;
> > +
> >  	if (INTEL_GEN(dev_priv) < 4)
> >  		pipe_config->double_wide = tmp & PIPECONF_DOUBLE_WIDE;
> >  
> > @@ -8616,6 +8621,8 @@ static void ironlake_set_pipeconf(const struct intel_crtc_state *crtc_state)
> >  	if (crtc_state->limited_color_range)
> >  		val |= PIPECONF_COLOR_RANGE_SELECT;
> >  
> > +	val |= PIPECONF_GAMMA_MODE(crtc_state->gamma_mode);
> > +
> >  	I915_WRITE(PIPECONF(pipe), val);
> >  	POSTING_READ(PIPECONF(pipe));
> >  }
> > @@ -9147,6 +9154,9 @@ static bool ironlake_get_pipe_config(struct intel_crtc *crtc,
> >  	if (tmp & PIPECONF_COLOR_RANGE_SELECT)
> >  		pipe_config->limited_color_range = true;
> >  
> > +	pipe_config->gamma_mode = (tmp & PIPECONF_GAMMA_MODE_MASK_ILK) >>
> > +		PIPECONF_GAMMA_MODE_SHIFT;
> > +
> >  	if (I915_READ(PCH_TRANSCONF(crtc->pipe)) & TRANS_ENABLE) {
> >  		struct intel_shared_dpll *pll;
> >  		enum intel_dpll_id pll_id;
> > @@ -11977,6 +11987,8 @@ intel_pipe_config_compare(struct drm_i915_private *dev_priv,
> >  
> >  	PIPE_CONF_CHECK_BOOL(double_wide);
> >  
> > +	PIPE_CONF_CHECK_X(gamma_mode);
> > +
> >  	PIPE_CONF_CHECK_P(shared_dpll);
> >  	PIPE_CONF_CHECK_X(dpll_hw_state.dpll);
> >  	PIPE_CONF_CHECK_X(dpll_hw_state.dpll_md);
> > -- 
> > 2.19.2
> > 
> 
> -- 
> Matt Roper
> Graphics Software Engineer
> IoTG Platform Enabling & Development
> Intel Corporation
> (916) 356-2795
On Wed, Jan 16, 2019 at 08:58:04PM +0200, Ville Syrjälä wrote:
> On Wed, Jan 16, 2019 at 10:31:56AM -0800, Matt Roper wrote:
> > On Fri, Jan 11, 2019 at 07:08:18PM +0200, Ville Syrjala wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > On pre-HSW gamma mode is configured via PIPECONF. The bits are
> > > the same except shifted up, so we can reuse just store them in
> > > crtc_state->gamma_mode in the HSW+ way, allowing us to share
> > > some code later.
> > > 
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_reg.h      | 10 ++++-
> > >  drivers/gpu/drm/i915/intel_color.c   | 60 +++++++++++++++++++++-------
> > >  drivers/gpu/drm/i915/intel_display.c | 14 ++++++-
> > >  3 files changed, 66 insertions(+), 18 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > > index 44958d994bfa..9d17ba199be4 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -5578,9 +5578,15 @@ enum {
> > >  #define   PIPECONF_SINGLE_WIDE	0
> > >  #define   PIPECONF_PIPE_UNLOCKED 0
> > >  #define   PIPECONF_PIPE_LOCKED	(1 << 25)
> > > -#define   PIPECONF_PALETTE	0
> > > -#define   PIPECONF_GAMMA		(1 << 24)
> > >  #define   PIPECONF_FORCE_BORDER	(1 << 25)
> > > +#define   PIPECONF_GAMMA_MODE_MASK_I9XX	(1 << 24) /* gmch */
> > > +#define   PIPECONF_GAMMA_MODE_MASK_ILK	(3 << 24) /* ilk-ivb */
> > > +#define   PIPECONF_GAMMA_MODE_8BIT	(0 << 24) /* gmch,ilk-ivb */
> > > +#define   PIPECONF_GAMMA_MODE_10BIT	(1 << 24) /* gmch,ilk-ivb */
> > > +#define   PIPECONF_GAMMA_MODE_12BIT	(2 << 24) /* ilk-ivb */
> > > +#define   PIPECONF_GAMMA_MODE_SPLIT	(3 << 24) /* ivb */
> > > +#define   PIPECONF_GAMMA_MODE(x)	((x)<<24) /* pass in GAMMA_MODE_MODE_* */
> > > +#define   PIPECONF_GAMMA_MODE_SHIFT	24
> > >  #define   PIPECONF_INTERLACE_MASK	(7 << 21)
> > >  #define   PIPECONF_INTERLACE_MASK_HSW	(3 << 21)
> > >  /* Note that pre-gen3 does not support interlaced display directly. Panel
> > > diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
> > > index 0c0da7ed0fd7..6fdbfa8c4008 100644
> > > --- a/drivers/gpu/drm/i915/intel_color.c
> > > +++ b/drivers/gpu/drm/i915/intel_color.c
> > > @@ -351,6 +351,32 @@ static void i9xx_load_luts(const struct intel_crtc_state *crtc_state)
> > >  	i9xx_load_luts_internal(crtc_state, crtc_state->base.gamma_lut);
> > >  }
> > >  
> > > +static void i9xx_color_commit(const struct intel_crtc_state *crtc_state)
> > > +{
> > > +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> > > +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> > > +	enum pipe pipe = crtc->pipe;
> > > +	u32 val;
> > > +
> > > +	val = I915_READ(PIPECONF(pipe));
> > > +	val &= ~PIPECONF_GAMMA_MODE_MASK_I9XX;
> > > +	val |= PIPECONF_GAMMA_MODE(crtc_state->gamma_mode);
> > > +	I915_WRITE(PIPECONF(pipe), val);
> > > +}
> > > +
> > > +static void ilk_color_commit(const struct intel_crtc_state *crtc_state)
> > > +{
> > > +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> > > +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> > > +	enum pipe pipe = crtc->pipe;
> > > +	u32 val;
> > > +
> > > +	val = I915_READ(PIPECONF(pipe));
> > > +	val &= ~PIPECONF_GAMMA_MODE_MASK_ILK;
> > > +	val |= PIPECONF_GAMMA_MODE(crtc_state->gamma_mode);
> > > +	I915_WRITE(PIPECONF(pipe), val);
> > > +}
> > 
> > Could we just set color_commit to i9xx_set_pipeconf and
> > ironlake_set_pipeconf to handle these without the r-m-w?
> 
> Perhaps. But not quite sure if we have any magic restrictions
> in the crtc enable sequence that would prevent that.

I guess we could always keep the double set_pipeconf() in the
crtc enable path so that we won't have to think whether to move the
color_commit() earlier or the set_pipeconf() later.

> 
> > 
> > 
> > Matt
> > 
> > > +
> > >  static void hsw_color_commit(const struct intel_crtc_state *crtc_state)
> > >  {
> > >  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> > > @@ -585,8 +611,7 @@ void intel_color_commit(const struct intel_crtc_state *crtc_state)
> > >  {
> > >  	struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev);
> > >  
> > > -	if (dev_priv->display.color_commit)
> > > -		dev_priv->display.color_commit(crtc_state);
> > > +	dev_priv->display.color_commit(crtc_state);
> > >  }
> > >  
> > >  int intel_color_check(struct intel_crtc_state *crtc_state)
> > > @@ -634,20 +659,25 @@ void intel_color_init(struct intel_crtc *crtc)
> > >  
> > >  	drm_mode_crtc_set_gamma_size(&crtc->base, 256);
> > >  
> > > -	if (IS_CHERRYVIEW(dev_priv)) {
> > > -		dev_priv->display.load_luts = cherryview_load_luts;
> > > -	} else if (IS_HASWELL(dev_priv)) {
> > > -		dev_priv->display.load_luts = i9xx_load_luts;
> > > -		dev_priv->display.color_commit = hsw_color_commit;
> > > -	} else if (IS_BROADWELL(dev_priv) || IS_GEN9_BC(dev_priv) ||
> > > -		   IS_BROXTON(dev_priv)) {
> > > -		dev_priv->display.load_luts = broadwell_load_luts;
> > > -		dev_priv->display.color_commit = hsw_color_commit;
> > > -	} else if (IS_GEMINILAKE(dev_priv) || IS_CANNONLAKE(dev_priv)) {
> > > -		dev_priv->display.load_luts = glk_load_luts;
> > > -		dev_priv->display.color_commit = hsw_color_commit;
> > > +	if (HAS_GMCH_DISPLAY(dev_priv)) {
> > > +		if (IS_CHERRYVIEW(dev_priv))
> > > +			dev_priv->display.load_luts = cherryview_load_luts;
> > > +		else
> > > +			dev_priv->display.load_luts = i9xx_load_luts;
> > > +
> > > +		dev_priv->display.color_commit = i9xx_color_commit;
> > >  	} else {
> > > -		dev_priv->display.load_luts = i9xx_load_luts;
> > > +		if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv))
> > > +			dev_priv->display.load_luts = glk_load_luts;
> > > +		else if (INTEL_GEN(dev_priv) >= 9 || IS_BROADWELL(dev_priv))
> > > +			dev_priv->display.load_luts = broadwell_load_luts;
> > > +		else
> > > +			dev_priv->display.load_luts = i9xx_load_luts;
> > > +
> > > +		if (INTEL_GEN(dev_priv) >= 8 || IS_HASWELL(dev_priv))
> > > +			dev_priv->display.color_commit = hsw_color_commit;
> > > +		else
> > > +			dev_priv->display.color_commit = ilk_color_commit;
> > >  	}
> > >  
> > >  	/* Enable color management support when we have degamma & gamma LUTs. */
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index 1caee4128974..90afcae91b30 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -3415,7 +3415,7 @@ static void i9xx_disable_plane(struct intel_plane *plane,
> > >  	 *
> > >  	 * On pre-g4x there is no way to gamma correct the
> > >  	 * pipe bottom color but we'll keep on doing this
> > > -	 * anyway.
> > > +	 * anyway so that the crtc state readout works correctly.
> > >  	 */
> > >  	dspcntr = i9xx_plane_ctl_crtc(crtc_state);
> > >  
> > > @@ -7627,6 +7627,8 @@ static void i9xx_set_pipeconf(const struct intel_crtc_state *crtc_state)
> > >  	     crtc_state->limited_color_range)
> > >  		pipeconf |= PIPECONF_COLOR_RANGE_SELECT;
> > >  
> > > +	pipeconf |= PIPECONF_GAMMA_MODE(crtc_state->gamma_mode);
> > > +
> > >  	I915_WRITE(PIPECONF(crtc->pipe), pipeconf);
> > >  	POSTING_READ(PIPECONF(crtc->pipe));
> > >  }
> > > @@ -8077,6 +8079,9 @@ static bool i9xx_get_pipe_config(struct intel_crtc *crtc,
> > >  	    (tmp & PIPECONF_COLOR_RANGE_SELECT))
> > >  		pipe_config->limited_color_range = true;
> > >  
> > > +	pipe_config->gamma_mode = (tmp & PIPECONF_GAMMA_MODE_MASK_I9XX) >>
> > > +		PIPECONF_GAMMA_MODE_SHIFT;
> > > +
> > >  	if (INTEL_GEN(dev_priv) < 4)
> > >  		pipe_config->double_wide = tmp & PIPECONF_DOUBLE_WIDE;
> > >  
> > > @@ -8616,6 +8621,8 @@ static void ironlake_set_pipeconf(const struct intel_crtc_state *crtc_state)
> > >  	if (crtc_state->limited_color_range)
> > >  		val |= PIPECONF_COLOR_RANGE_SELECT;
> > >  
> > > +	val |= PIPECONF_GAMMA_MODE(crtc_state->gamma_mode);
> > > +
> > >  	I915_WRITE(PIPECONF(pipe), val);
> > >  	POSTING_READ(PIPECONF(pipe));
> > >  }
> > > @@ -9147,6 +9154,9 @@ static bool ironlake_get_pipe_config(struct intel_crtc *crtc,
> > >  	if (tmp & PIPECONF_COLOR_RANGE_SELECT)
> > >  		pipe_config->limited_color_range = true;
> > >  
> > > +	pipe_config->gamma_mode = (tmp & PIPECONF_GAMMA_MODE_MASK_ILK) >>
> > > +		PIPECONF_GAMMA_MODE_SHIFT;
> > > +
> > >  	if (I915_READ(PCH_TRANSCONF(crtc->pipe)) & TRANS_ENABLE) {
> > >  		struct intel_shared_dpll *pll;
> > >  		enum intel_dpll_id pll_id;
> > > @@ -11977,6 +11987,8 @@ intel_pipe_config_compare(struct drm_i915_private *dev_priv,
> > >  
> > >  	PIPE_CONF_CHECK_BOOL(double_wide);
> > >  
> > > +	PIPE_CONF_CHECK_X(gamma_mode);
> > > +
> > >  	PIPE_CONF_CHECK_P(shared_dpll);
> > >  	PIPE_CONF_CHECK_X(dpll_hw_state.dpll);
> > >  	PIPE_CONF_CHECK_X(dpll_hw_state.dpll_md);
> > > -- 
> > > 2.19.2
> > > 
> > 
> > -- 
> > Matt Roper
> > Graphics Software Engineer
> > IoTG Platform Enabling & Development
> > Intel Corporation
> > (916) 356-2795
> 
> -- 
> Ville Syrjälä
> Intel
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, Jan 16, 2019 at 09:51:01PM +0200, Ville Syrjälä wrote:
> On Wed, Jan 16, 2019 at 08:58:04PM +0200, Ville Syrjälä wrote:
> > On Wed, Jan 16, 2019 at 10:31:56AM -0800, Matt Roper wrote:
> > > On Fri, Jan 11, 2019 at 07:08:18PM +0200, Ville Syrjala wrote:
> > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > 
> > > > On pre-HSW gamma mode is configured via PIPECONF. The bits are
> > > > the same except shifted up, so we can reuse just store them in
> > > > crtc_state->gamma_mode in the HSW+ way, allowing us to share
> > > > some code later.
> > > > 
> > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/i915_reg.h      | 10 ++++-
> > > >  drivers/gpu/drm/i915/intel_color.c   | 60 +++++++++++++++++++++-------
> > > >  drivers/gpu/drm/i915/intel_display.c | 14 ++++++-
> > > >  3 files changed, 66 insertions(+), 18 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > > > index 44958d994bfa..9d17ba199be4 100644
> > > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > > @@ -5578,9 +5578,15 @@ enum {
> > > >  #define   PIPECONF_SINGLE_WIDE	0
> > > >  #define   PIPECONF_PIPE_UNLOCKED 0
> > > >  #define   PIPECONF_PIPE_LOCKED	(1 << 25)
> > > > -#define   PIPECONF_PALETTE	0
> > > > -#define   PIPECONF_GAMMA		(1 << 24)
> > > >  #define   PIPECONF_FORCE_BORDER	(1 << 25)
> > > > +#define   PIPECONF_GAMMA_MODE_MASK_I9XX	(1 << 24) /* gmch */
> > > > +#define   PIPECONF_GAMMA_MODE_MASK_ILK	(3 << 24) /* ilk-ivb */
> > > > +#define   PIPECONF_GAMMA_MODE_8BIT	(0 << 24) /* gmch,ilk-ivb */
> > > > +#define   PIPECONF_GAMMA_MODE_10BIT	(1 << 24) /* gmch,ilk-ivb */
> > > > +#define   PIPECONF_GAMMA_MODE_12BIT	(2 << 24) /* ilk-ivb */
> > > > +#define   PIPECONF_GAMMA_MODE_SPLIT	(3 << 24) /* ivb */
> > > > +#define   PIPECONF_GAMMA_MODE(x)	((x)<<24) /* pass in GAMMA_MODE_MODE_* */
> > > > +#define   PIPECONF_GAMMA_MODE_SHIFT	24
> > > >  #define   PIPECONF_INTERLACE_MASK	(7 << 21)
> > > >  #define   PIPECONF_INTERLACE_MASK_HSW	(3 << 21)
> > > >  /* Note that pre-gen3 does not support interlaced display directly. Panel
> > > > diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
> > > > index 0c0da7ed0fd7..6fdbfa8c4008 100644
> > > > --- a/drivers/gpu/drm/i915/intel_color.c
> > > > +++ b/drivers/gpu/drm/i915/intel_color.c
> > > > @@ -351,6 +351,32 @@ static void i9xx_load_luts(const struct intel_crtc_state *crtc_state)
> > > >  	i9xx_load_luts_internal(crtc_state, crtc_state->base.gamma_lut);
> > > >  }
> > > >  
> > > > +static void i9xx_color_commit(const struct intel_crtc_state *crtc_state)
> > > > +{
> > > > +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> > > > +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> > > > +	enum pipe pipe = crtc->pipe;
> > > > +	u32 val;
> > > > +
> > > > +	val = I915_READ(PIPECONF(pipe));
> > > > +	val &= ~PIPECONF_GAMMA_MODE_MASK_I9XX;
> > > > +	val |= PIPECONF_GAMMA_MODE(crtc_state->gamma_mode);
> > > > +	I915_WRITE(PIPECONF(pipe), val);
> > > > +}
> > > > +
> > > > +static void ilk_color_commit(const struct intel_crtc_state *crtc_state)
> > > > +{
> > > > +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> > > > +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> > > > +	enum pipe pipe = crtc->pipe;
> > > > +	u32 val;
> > > > +
> > > > +	val = I915_READ(PIPECONF(pipe));
> > > > +	val &= ~PIPECONF_GAMMA_MODE_MASK_ILK;
> > > > +	val |= PIPECONF_GAMMA_MODE(crtc_state->gamma_mode);
> > > > +	I915_WRITE(PIPECONF(pipe), val);
> > > > +}
> > > 
> > > Could we just set color_commit to i9xx_set_pipeconf and
> > > ironlake_set_pipeconf to handle these without the r-m-w?
> > 
> > Perhaps. But not quite sure if we have any magic restrictions
> > in the crtc enable sequence that would prevent that.
> 
> I guess we could always keep the double set_pipeconf() in the
> crtc enable path so that we won't have to think whether to move the
> color_commit() earlier or the set_pipeconf() later.

Actually on second thought we can't avoid the RMW without more
restructuring because ironlake_set_pipeconf() & co. get called before
we enable the pipe and thus won't set the enable bit. So I think I'm
going to stick with the original patch for now.

> 
> > 
> > > 
> > > 
> > > Matt
> > > 
> > > > +
> > > >  static void hsw_color_commit(const struct intel_crtc_state *crtc_state)
> > > >  {
> > > >  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> > > > @@ -585,8 +611,7 @@ void intel_color_commit(const struct intel_crtc_state *crtc_state)
> > > >  {
> > > >  	struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev);
> > > >  
> > > > -	if (dev_priv->display.color_commit)
> > > > -		dev_priv->display.color_commit(crtc_state);
> > > > +	dev_priv->display.color_commit(crtc_state);
> > > >  }
> > > >  
> > > >  int intel_color_check(struct intel_crtc_state *crtc_state)
> > > > @@ -634,20 +659,25 @@ void intel_color_init(struct intel_crtc *crtc)
> > > >  
> > > >  	drm_mode_crtc_set_gamma_size(&crtc->base, 256);
> > > >  
> > > > -	if (IS_CHERRYVIEW(dev_priv)) {
> > > > -		dev_priv->display.load_luts = cherryview_load_luts;
> > > > -	} else if (IS_HASWELL(dev_priv)) {
> > > > -		dev_priv->display.load_luts = i9xx_load_luts;
> > > > -		dev_priv->display.color_commit = hsw_color_commit;
> > > > -	} else if (IS_BROADWELL(dev_priv) || IS_GEN9_BC(dev_priv) ||
> > > > -		   IS_BROXTON(dev_priv)) {
> > > > -		dev_priv->display.load_luts = broadwell_load_luts;
> > > > -		dev_priv->display.color_commit = hsw_color_commit;
> > > > -	} else if (IS_GEMINILAKE(dev_priv) || IS_CANNONLAKE(dev_priv)) {
> > > > -		dev_priv->display.load_luts = glk_load_luts;
> > > > -		dev_priv->display.color_commit = hsw_color_commit;
> > > > +	if (HAS_GMCH_DISPLAY(dev_priv)) {
> > > > +		if (IS_CHERRYVIEW(dev_priv))
> > > > +			dev_priv->display.load_luts = cherryview_load_luts;
> > > > +		else
> > > > +			dev_priv->display.load_luts = i9xx_load_luts;
> > > > +
> > > > +		dev_priv->display.color_commit = i9xx_color_commit;
> > > >  	} else {
> > > > -		dev_priv->display.load_luts = i9xx_load_luts;
> > > > +		if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv))
> > > > +			dev_priv->display.load_luts = glk_load_luts;
> > > > +		else if (INTEL_GEN(dev_priv) >= 9 || IS_BROADWELL(dev_priv))
> > > > +			dev_priv->display.load_luts = broadwell_load_luts;
> > > > +		else
> > > > +			dev_priv->display.load_luts = i9xx_load_luts;
> > > > +
> > > > +		if (INTEL_GEN(dev_priv) >= 8 || IS_HASWELL(dev_priv))
> > > > +			dev_priv->display.color_commit = hsw_color_commit;
> > > > +		else
> > > > +			dev_priv->display.color_commit = ilk_color_commit;
> > > >  	}
> > > >  
> > > >  	/* Enable color management support when we have degamma & gamma LUTs. */
> > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > > index 1caee4128974..90afcae91b30 100644
> > > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > > @@ -3415,7 +3415,7 @@ static void i9xx_disable_plane(struct intel_plane *plane,
> > > >  	 *
> > > >  	 * On pre-g4x there is no way to gamma correct the
> > > >  	 * pipe bottom color but we'll keep on doing this
> > > > -	 * anyway.
> > > > +	 * anyway so that the crtc state readout works correctly.
> > > >  	 */
> > > >  	dspcntr = i9xx_plane_ctl_crtc(crtc_state);
> > > >  
> > > > @@ -7627,6 +7627,8 @@ static void i9xx_set_pipeconf(const struct intel_crtc_state *crtc_state)
> > > >  	     crtc_state->limited_color_range)
> > > >  		pipeconf |= PIPECONF_COLOR_RANGE_SELECT;
> > > >  
> > > > +	pipeconf |= PIPECONF_GAMMA_MODE(crtc_state->gamma_mode);
> > > > +
> > > >  	I915_WRITE(PIPECONF(crtc->pipe), pipeconf);
> > > >  	POSTING_READ(PIPECONF(crtc->pipe));
> > > >  }
> > > > @@ -8077,6 +8079,9 @@ static bool i9xx_get_pipe_config(struct intel_crtc *crtc,
> > > >  	    (tmp & PIPECONF_COLOR_RANGE_SELECT))
> > > >  		pipe_config->limited_color_range = true;
> > > >  
> > > > +	pipe_config->gamma_mode = (tmp & PIPECONF_GAMMA_MODE_MASK_I9XX) >>
> > > > +		PIPECONF_GAMMA_MODE_SHIFT;
> > > > +
> > > >  	if (INTEL_GEN(dev_priv) < 4)
> > > >  		pipe_config->double_wide = tmp & PIPECONF_DOUBLE_WIDE;
> > > >  
> > > > @@ -8616,6 +8621,8 @@ static void ironlake_set_pipeconf(const struct intel_crtc_state *crtc_state)
> > > >  	if (crtc_state->limited_color_range)
> > > >  		val |= PIPECONF_COLOR_RANGE_SELECT;
> > > >  
> > > > +	val |= PIPECONF_GAMMA_MODE(crtc_state->gamma_mode);
> > > > +
> > > >  	I915_WRITE(PIPECONF(pipe), val);
> > > >  	POSTING_READ(PIPECONF(pipe));
> > > >  }
> > > > @@ -9147,6 +9154,9 @@ static bool ironlake_get_pipe_config(struct intel_crtc *crtc,
> > > >  	if (tmp & PIPECONF_COLOR_RANGE_SELECT)
> > > >  		pipe_config->limited_color_range = true;
> > > >  
> > > > +	pipe_config->gamma_mode = (tmp & PIPECONF_GAMMA_MODE_MASK_ILK) >>
> > > > +		PIPECONF_GAMMA_MODE_SHIFT;
> > > > +
> > > >  	if (I915_READ(PCH_TRANSCONF(crtc->pipe)) & TRANS_ENABLE) {
> > > >  		struct intel_shared_dpll *pll;
> > > >  		enum intel_dpll_id pll_id;
> > > > @@ -11977,6 +11987,8 @@ intel_pipe_config_compare(struct drm_i915_private *dev_priv,
> > > >  
> > > >  	PIPE_CONF_CHECK_BOOL(double_wide);
> > > >  
> > > > +	PIPE_CONF_CHECK_X(gamma_mode);
> > > > +
> > > >  	PIPE_CONF_CHECK_P(shared_dpll);
> > > >  	PIPE_CONF_CHECK_X(dpll_hw_state.dpll);
> > > >  	PIPE_CONF_CHECK_X(dpll_hw_state.dpll_md);
> > > > -- 
> > > > 2.19.2
> > > > 
> > > 
> > > -- 
> > > Matt Roper
> > > Graphics Software Engineer
> > > IoTG Platform Enabling & Development
> > > Intel Corporation
> > > (916) 356-2795
> > 
> > -- 
> > Ville Syrjälä
> > Intel
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel