[09/13] drm/i915: Track pipe gamma enable/disable in crtc state

Submitted by Ville Syrjala on Jan. 11, 2019, 5:08 p.m.

Details

Message ID 20190111170823.4441-10-ville.syrjala@linux.intel.com
State New
Series "Enable/disable gamma/csc dynamically and fix C8"
Headers show

Commit Message

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

Track whether pipe gamma is enabled or disabled. For now we
stick to the current behaviour of always enabling gamma. But
we do get working state readout for this now. On SKL+ we use
the pipe bottom color as our hardware state. On pre-SKL we
read the state back from the primary plane control register.
That only really correct for g4x+, as older platforms never
gamma correct pipe bottom color. But doing the readout the
same way on all platforms is fine, and there is no other way
to do it really.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h      |  8 ++++
 drivers/gpu/drm/i915/intel_color.c   | 24 +++++++++++-
 drivers/gpu/drm/i915/intel_display.c | 56 ++++++++++++++++++++++------
 drivers/gpu/drm/i915/intel_drv.h     |  3 ++
 drivers/gpu/drm/i915/intel_sprite.c  | 17 +++++++--
 5 files changed, 92 insertions(+), 16 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 9d17ba199be4..7f0913bc1b47 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -5707,6 +5707,14 @@  enum {
 #define   PIPEMISC_DITHER_TYPE_SP	(0 << 2)
 #define PIPEMISC(pipe)			_MMIO_PIPE2(pipe, _PIPE_MISC_A)
 
+/* SKL+ pipe bottom color */
+#define _PIPE_BOTTOM_COLOR_A		0x70034
+#define _PIPE_BOTTOM_COLOR_B		0x71034
+#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)		_MMIO_PIPE2(pipe, _PIPE_BOTTOM_COLOR_A)
+
 #define VLV_DPFLIPSTAT				_MMIO(VLV_DISPLAY_BASE + 0x70028)
 #define   PIPEB_LINE_COMPARE_INT_EN		(1 << 29)
 #define   PIPEB_HLINE_INT_EN			(1 << 28)
diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
index 6fdbfa8c4008..313b281204fa 100644
--- a/drivers/gpu/drm/i915/intel_color.c
+++ b/drivers/gpu/drm/i915/intel_color.c
@@ -387,6 +387,24 @@  static void hsw_color_commit(const struct intel_crtc_state *crtc_state)
 	ilk_load_csc_matrix(crtc_state);
 }
 
+static void skl_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 = 0;
+	if (crtc_state->gamma_enable)
+		val |= PIPE_BOTTOM_GAMMA_ENABLE;
+	val |= PIPE_BOTTOM_CSC_ENABLE;
+	I915_WRITE(PIPE_BOTTOM_COLOR(pipe), val);
+
+	I915_WRITE(GAMMA_MODE(crtc->pipe), crtc_state->gamma_mode);
+
+	ilk_load_csc_matrix(crtc_state);
+}
+
 static void bdw_load_degamma_lut(const struct intel_crtc_state *crtc_state)
 {
 	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
@@ -624,6 +642,8 @@  int intel_color_check(struct intel_crtc_state *crtc_state)
 	degamma_length = INTEL_INFO(dev_priv)->color.degamma_lut_size;
 	gamma_length = INTEL_INFO(dev_priv)->color.gamma_lut_size;
 
+	crtc_state->gamma_enable = true;
+
 	/*
 	 * We also allow no degamma lut/ctm and a gamma lut at the legacy
 	 * size (256 entries).
@@ -674,7 +694,9 @@  void intel_color_init(struct intel_crtc *crtc)
 		else
 			dev_priv->display.load_luts = i9xx_load_luts;
 
-		if (INTEL_GEN(dev_priv) >= 8 || IS_HASWELL(dev_priv))
+		if (INTEL_GEN(dev_priv) >= 9)
+			dev_priv->display.color_commit = skl_color_commit;
+		else if (IS_BROADWELL(dev_priv) || IS_HASWELL(dev_priv))
 			dev_priv->display.color_commit = hsw_color_commit;
 		else
 			dev_priv->display.color_commit = ilk_color_commit;
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 90afcae91b30..896ce95790cb 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3186,7 +3186,8 @@  static u32 i9xx_plane_ctl_crtc(const struct intel_crtc_state *crtc_state)
 	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
 	u32 dspcntr = 0;
 
-	dspcntr |= DISPPLANE_GAMMA_ENABLE;
+	if (crtc_state->gamma_enable)
+		dspcntr |= DISPPLANE_GAMMA_ENABLE;
 
 	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
 		dspcntr |= DISPPLANE_PIPE_CSC_ENABLE;
@@ -3664,7 +3665,9 @@  u32 skl_plane_ctl_crtc(const struct intel_crtc_state *crtc_state)
 	if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))
 		return plane_ctl;
 
-	plane_ctl |= PLANE_CTL_PIPE_GAMMA_ENABLE;
+	if (crtc_state->gamma_enable)
+		plane_ctl |= PLANE_CTL_PIPE_GAMMA_ENABLE;
+
 	plane_ctl |= PLANE_CTL_PIPE_CSC_ENABLE;
 
 	return plane_ctl;
@@ -3717,7 +3720,9 @@  u32 glk_plane_color_ctl_crtc(const struct intel_crtc_state *crtc_state)
 	if (INTEL_GEN(dev_priv) >= 11)
 		return plane_color_ctl;
 
-	plane_color_ctl |= PLANE_COLOR_PIPE_GAMMA_ENABLE;
+	if (crtc_state->gamma_enable)
+		plane_color_ctl |= PLANE_COLOR_PIPE_GAMMA_ENABLE;
+
 	plane_color_ctl |= PLANE_COLOR_PIPE_CSC_ENABLE;
 
 	return plane_color_ctl;
@@ -3925,7 +3930,6 @@  static void intel_update_pipe_config(const struct intel_crtc_state *old_crtc_sta
 		   ((new_crtc_state->pipe_src_w - 1) << 16) |
 		   (new_crtc_state->pipe_src_h - 1));
 
-	/* on skylake this is done by detaching scalers */
 	if (INTEL_GEN(dev_priv) >= 9) {
 		skl_detach_scalers(new_crtc_state);
 
@@ -8036,6 +8040,20 @@  static void intel_get_crtc_ycbcr_config(struct intel_crtc *crtc,
 	pipe_config->output_format = output;
 }
 
+static void i9xx_get_pipe_color_config(struct intel_crtc_state *crtc_state)
+{
+	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
+	struct intel_plane *plane = to_intel_plane(crtc->base.primary);
+	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
+	enum i9xx_plane_id i9xx_plane = plane->i9xx_plane;
+	u32 tmp;
+
+	tmp = I915_READ(DSPCNTR(i9xx_plane));
+
+	if (tmp & DISPPLANE_GAMMA_ENABLE)
+		crtc_state->gamma_enable = true;
+}
+
 static bool i9xx_get_pipe_config(struct intel_crtc *crtc,
 				 struct intel_crtc_state *pipe_config)
 {
@@ -8082,6 +8100,8 @@  static bool i9xx_get_pipe_config(struct intel_crtc *crtc,
 	pipe_config->gamma_mode = (tmp & PIPECONF_GAMMA_MODE_MASK_I9XX) >>
 		PIPECONF_GAMMA_MODE_SHIFT;
 
+	i9xx_get_pipe_color_config(pipe_config);
+
 	if (INTEL_GEN(dev_priv) < 4)
 		pipe_config->double_wide = tmp & PIPECONF_DOUBLE_WIDE;
 
@@ -9157,6 +9177,8 @@  static bool ironlake_get_pipe_config(struct intel_crtc *crtc,
 	pipe_config->gamma_mode = (tmp & PIPECONF_GAMMA_MODE_MASK_ILK) >>
 		PIPECONF_GAMMA_MODE_SHIFT;
 
+	i9xx_get_pipe_color_config(pipe_config);
+
 	if (I915_READ(PCH_TRANSCONF(crtc->pipe)) & TRANS_ENABLE) {
 		struct intel_shared_dpll *pll;
 		enum intel_dpll_id pll_id;
@@ -9785,6 +9807,15 @@  static bool haswell_get_pipe_config(struct intel_crtc *crtc,
 	pipe_config->gamma_mode =
 		I915_READ(GAMMA_MODE(crtc->pipe)) & GAMMA_MODE_MODE_MASK;
 
+	if (INTEL_GEN(dev_priv) >= 9) {
+		u32 tmp = I915_READ(PIPE_BOTTOM_COLOR(crtc->pipe));
+
+		if (tmp & PIPE_BOTTOM_GAMMA_ENABLE)
+			pipe_config->gamma_enable = true;
+	} else {
+		i9xx_get_pipe_color_config(pipe_config);
+	}
+
 	power_domain = POWER_DOMAIN_PIPE_PANEL_FITTER(crtc->pipe);
 	if (intel_display_power_get_if_enabled(dev_priv, power_domain)) {
 		power_domain_mask |= BIT_ULL(power_domain);
@@ -9953,7 +9984,12 @@  i845_cursor_max_stride(struct intel_plane *plane,
 
 static u32 i845_cursor_ctl_crtc(const struct intel_crtc_state *crtc_state)
 {
-	return CURSOR_GAMMA_ENABLE;
+	u32 cntl = 0;
+
+	if (crtc_state->gamma_enable)
+		cntl |= CURSOR_GAMMA_ENABLE;
+
+	return cntl;
 }
 
 static u32 i845_cursor_ctl(const struct intel_crtc_state *crtc_state,
@@ -10105,7 +10141,8 @@  static u32 i9xx_cursor_ctl_crtc(const struct intel_crtc_state *crtc_state)
 	if (INTEL_GEN(dev_priv) >= 11)
 		return cntl;
 
-	cntl |= MCURSOR_GAMMA_ENABLE;
+	if (crtc_state->gamma_enable)
+		cntl = MCURSOR_GAMMA_ENABLE;
 
 	if (HAS_DDI(dev_priv))
 		cntl |= MCURSOR_PIPE_CSC_ENABLE;
@@ -11094,12 +11131,6 @@  static int intel_crtc_atomic_check(struct drm_crtc *crtc,
 		ret = intel_color_check(pipe_config);
 		if (ret)
 			return ret;
-
-		/*
-		 * Changing color management on Intel hardware is
-		 * handled as part of planes update.
-		 */
-		crtc_state->planes_changed = true;
 	}
 
 	ret = 0;
@@ -11988,6 +12019,7 @@  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_BOOL(gamma_enable);
 
 	PIPE_CONF_CHECK_P(shared_dpll);
 	PIPE_CONF_CHECK_X(dpll_hw_state.dpll);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 59f8d4270e82..eee734b48919 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -946,6 +946,9 @@  struct intel_crtc_state {
 	/* Output down scaling is done in LSPCON device */
 	bool lspcon_downsampling;
 
+	/* enable pipe gamma? */
+	bool gamma_enable;
+
 	/* Display Stream compression state */
 	struct {
 		bool compression_enable;
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index a45ef98b2f8d..034a355692db 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -739,7 +739,12 @@  vlv_update_clrc(const struct intel_plane_state *plane_state)
 
 static u32 vlv_sprite_ctl_crtc(const struct intel_crtc_state *crtc_state)
 {
-	return SP_GAMMA_ENABLE;
+	u32 sprctl = 0;
+
+	if (crtc_state->gamma_enable)
+		sprctl |= SP_GAMMA_ENABLE;
+
+	return sprctl;
 }
 
 static u32 vlv_sprite_ctl(const struct intel_crtc_state *crtc_state,
@@ -915,7 +920,8 @@  static u32 ivb_sprite_ctl_crtc(const struct intel_crtc_state *crtc_state)
 	struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev);
 	u32 sprctl = 0;
 
-	sprctl |= SPRITE_GAMMA_ENABLE;
+	if (crtc_state->gamma_enable)
+		sprctl |= SPRITE_GAMMA_ENABLE;
 
 	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
 		sprctl |= SPRITE_PIPE_CSC_ENABLE;
@@ -1101,7 +1107,12 @@  g4x_sprite_max_stride(struct intel_plane *plane,
 
 static u32 g4x_sprite_ctl_crtc(const struct intel_crtc_state *crtc_state)
 {
-	return DVS_GAMMA_ENABLE;
+	u32 dvscntr = 0;
+
+	if (crtc_state->gamma_enable)
+		dvscntr |= DVS_GAMMA_ENABLE;
+
+	return dvscntr;
 }
 
 static u32 g4x_sprite_ctl(const struct intel_crtc_state *crtc_state,

Comments

Matt Roper Jan. 16, 2019, 7:36 p.m.
On Fri, Jan 11, 2019 at 07:08:19PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Track whether pipe gamma is enabled or disabled. For now we
> stick to the current behaviour of always enabling gamma. But
> we do get working state readout for this now. On SKL+ we use
> the pipe bottom color as our hardware state. On pre-SKL we
> read the state back from the primary plane control register.
> That only really correct for g4x+, as older platforms never
> gamma correct pipe bottom color. But doing the readout the
> same way on all platforms is fine, and there is no other way
> to do it really.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Reviewed-by: Matt Roper <matthew.d.roper@intel.com>

> ---
>  drivers/gpu/drm/i915/i915_reg.h      |  8 ++++
>  drivers/gpu/drm/i915/intel_color.c   | 24 +++++++++++-
>  drivers/gpu/drm/i915/intel_display.c | 56 ++++++++++++++++++++++------
>  drivers/gpu/drm/i915/intel_drv.h     |  3 ++
>  drivers/gpu/drm/i915/intel_sprite.c  | 17 +++++++--
>  5 files changed, 92 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 9d17ba199be4..7f0913bc1b47 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -5707,6 +5707,14 @@ enum {
>  #define   PIPEMISC_DITHER_TYPE_SP	(0 << 2)
>  #define PIPEMISC(pipe)			_MMIO_PIPE2(pipe, _PIPE_MISC_A)
>  
> +/* SKL+ pipe bottom color */
> +#define _PIPE_BOTTOM_COLOR_A		0x70034
> +#define _PIPE_BOTTOM_COLOR_B		0x71034
> +#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)		_MMIO_PIPE2(pipe, _PIPE_BOTTOM_COLOR_A)
> +
>  #define VLV_DPFLIPSTAT				_MMIO(VLV_DISPLAY_BASE + 0x70028)
>  #define   PIPEB_LINE_COMPARE_INT_EN		(1 << 29)
>  #define   PIPEB_HLINE_INT_EN			(1 << 28)
> diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
> index 6fdbfa8c4008..313b281204fa 100644
> --- a/drivers/gpu/drm/i915/intel_color.c
> +++ b/drivers/gpu/drm/i915/intel_color.c
> @@ -387,6 +387,24 @@ static void hsw_color_commit(const struct intel_crtc_state *crtc_state)
>  	ilk_load_csc_matrix(crtc_state);
>  }
>  
> +static void skl_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 = 0;
> +	if (crtc_state->gamma_enable)
> +		val |= PIPE_BOTTOM_GAMMA_ENABLE;
> +	val |= PIPE_BOTTOM_CSC_ENABLE;
> +	I915_WRITE(PIPE_BOTTOM_COLOR(pipe), val);
> +
> +	I915_WRITE(GAMMA_MODE(crtc->pipe), crtc_state->gamma_mode);
> +
> +	ilk_load_csc_matrix(crtc_state);
> +}
> +
>  static void bdw_load_degamma_lut(const struct intel_crtc_state *crtc_state)
>  {
>  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> @@ -624,6 +642,8 @@ int intel_color_check(struct intel_crtc_state *crtc_state)
>  	degamma_length = INTEL_INFO(dev_priv)->color.degamma_lut_size;
>  	gamma_length = INTEL_INFO(dev_priv)->color.gamma_lut_size;
>  
> +	crtc_state->gamma_enable = true;
> +
>  	/*
>  	 * We also allow no degamma lut/ctm and a gamma lut at the legacy
>  	 * size (256 entries).
> @@ -674,7 +694,9 @@ void intel_color_init(struct intel_crtc *crtc)
>  		else
>  			dev_priv->display.load_luts = i9xx_load_luts;
>  
> -		if (INTEL_GEN(dev_priv) >= 8 || IS_HASWELL(dev_priv))
> +		if (INTEL_GEN(dev_priv) >= 9)
> +			dev_priv->display.color_commit = skl_color_commit;
> +		else if (IS_BROADWELL(dev_priv) || IS_HASWELL(dev_priv))
>  			dev_priv->display.color_commit = hsw_color_commit;
>  		else
>  			dev_priv->display.color_commit = ilk_color_commit;
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 90afcae91b30..896ce95790cb 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3186,7 +3186,8 @@ static u32 i9xx_plane_ctl_crtc(const struct intel_crtc_state *crtc_state)
>  	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>  	u32 dspcntr = 0;
>  
> -	dspcntr |= DISPPLANE_GAMMA_ENABLE;
> +	if (crtc_state->gamma_enable)
> +		dspcntr |= DISPPLANE_GAMMA_ENABLE;
>  
>  	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
>  		dspcntr |= DISPPLANE_PIPE_CSC_ENABLE;
> @@ -3664,7 +3665,9 @@ u32 skl_plane_ctl_crtc(const struct intel_crtc_state *crtc_state)
>  	if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))
>  		return plane_ctl;
>  
> -	plane_ctl |= PLANE_CTL_PIPE_GAMMA_ENABLE;
> +	if (crtc_state->gamma_enable)
> +		plane_ctl |= PLANE_CTL_PIPE_GAMMA_ENABLE;
> +
>  	plane_ctl |= PLANE_CTL_PIPE_CSC_ENABLE;
>  
>  	return plane_ctl;
> @@ -3717,7 +3720,9 @@ u32 glk_plane_color_ctl_crtc(const struct intel_crtc_state *crtc_state)
>  	if (INTEL_GEN(dev_priv) >= 11)
>  		return plane_color_ctl;
>  
> -	plane_color_ctl |= PLANE_COLOR_PIPE_GAMMA_ENABLE;
> +	if (crtc_state->gamma_enable)
> +		plane_color_ctl |= PLANE_COLOR_PIPE_GAMMA_ENABLE;
> +
>  	plane_color_ctl |= PLANE_COLOR_PIPE_CSC_ENABLE;
>  
>  	return plane_color_ctl;
> @@ -3925,7 +3930,6 @@ static void intel_update_pipe_config(const struct intel_crtc_state *old_crtc_sta
>  		   ((new_crtc_state->pipe_src_w - 1) << 16) |
>  		   (new_crtc_state->pipe_src_h - 1));
>  
> -	/* on skylake this is done by detaching scalers */
>  	if (INTEL_GEN(dev_priv) >= 9) {
>  		skl_detach_scalers(new_crtc_state);
>  
> @@ -8036,6 +8040,20 @@ static void intel_get_crtc_ycbcr_config(struct intel_crtc *crtc,
>  	pipe_config->output_format = output;
>  }
>  
> +static void i9xx_get_pipe_color_config(struct intel_crtc_state *crtc_state)
> +{
> +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> +	struct intel_plane *plane = to_intel_plane(crtc->base.primary);
> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> +	enum i9xx_plane_id i9xx_plane = plane->i9xx_plane;
> +	u32 tmp;
> +
> +	tmp = I915_READ(DSPCNTR(i9xx_plane));
> +
> +	if (tmp & DISPPLANE_GAMMA_ENABLE)
> +		crtc_state->gamma_enable = true;
> +}
> +
>  static bool i9xx_get_pipe_config(struct intel_crtc *crtc,
>  				 struct intel_crtc_state *pipe_config)
>  {
> @@ -8082,6 +8100,8 @@ static bool i9xx_get_pipe_config(struct intel_crtc *crtc,
>  	pipe_config->gamma_mode = (tmp & PIPECONF_GAMMA_MODE_MASK_I9XX) >>
>  		PIPECONF_GAMMA_MODE_SHIFT;
>  
> +	i9xx_get_pipe_color_config(pipe_config);
> +
>  	if (INTEL_GEN(dev_priv) < 4)
>  		pipe_config->double_wide = tmp & PIPECONF_DOUBLE_WIDE;
>  
> @@ -9157,6 +9177,8 @@ static bool ironlake_get_pipe_config(struct intel_crtc *crtc,
>  	pipe_config->gamma_mode = (tmp & PIPECONF_GAMMA_MODE_MASK_ILK) >>
>  		PIPECONF_GAMMA_MODE_SHIFT;
>  
> +	i9xx_get_pipe_color_config(pipe_config);
> +
>  	if (I915_READ(PCH_TRANSCONF(crtc->pipe)) & TRANS_ENABLE) {
>  		struct intel_shared_dpll *pll;
>  		enum intel_dpll_id pll_id;
> @@ -9785,6 +9807,15 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
>  	pipe_config->gamma_mode =
>  		I915_READ(GAMMA_MODE(crtc->pipe)) & GAMMA_MODE_MODE_MASK;
>  
> +	if (INTEL_GEN(dev_priv) >= 9) {
> +		u32 tmp = I915_READ(PIPE_BOTTOM_COLOR(crtc->pipe));
> +
> +		if (tmp & PIPE_BOTTOM_GAMMA_ENABLE)
> +			pipe_config->gamma_enable = true;
> +	} else {
> +		i9xx_get_pipe_color_config(pipe_config);
> +	}
> +
>  	power_domain = POWER_DOMAIN_PIPE_PANEL_FITTER(crtc->pipe);
>  	if (intel_display_power_get_if_enabled(dev_priv, power_domain)) {
>  		power_domain_mask |= BIT_ULL(power_domain);
> @@ -9953,7 +9984,12 @@ i845_cursor_max_stride(struct intel_plane *plane,
>  
>  static u32 i845_cursor_ctl_crtc(const struct intel_crtc_state *crtc_state)
>  {
> -	return CURSOR_GAMMA_ENABLE;
> +	u32 cntl = 0;
> +
> +	if (crtc_state->gamma_enable)
> +		cntl |= CURSOR_GAMMA_ENABLE;
> +
> +	return cntl;
>  }
>  
>  static u32 i845_cursor_ctl(const struct intel_crtc_state *crtc_state,
> @@ -10105,7 +10141,8 @@ static u32 i9xx_cursor_ctl_crtc(const struct intel_crtc_state *crtc_state)
>  	if (INTEL_GEN(dev_priv) >= 11)
>  		return cntl;
>  
> -	cntl |= MCURSOR_GAMMA_ENABLE;
> +	if (crtc_state->gamma_enable)
> +		cntl = MCURSOR_GAMMA_ENABLE;
>  
>  	if (HAS_DDI(dev_priv))
>  		cntl |= MCURSOR_PIPE_CSC_ENABLE;
> @@ -11094,12 +11131,6 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc,
>  		ret = intel_color_check(pipe_config);
>  		if (ret)
>  			return ret;
> -
> -		/*
> -		 * Changing color management on Intel hardware is
> -		 * handled as part of planes update.
> -		 */
> -		crtc_state->planes_changed = true;
>  	}
>  
>  	ret = 0;
> @@ -11988,6 +12019,7 @@ 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_BOOL(gamma_enable);
>  
>  	PIPE_CONF_CHECK_P(shared_dpll);
>  	PIPE_CONF_CHECK_X(dpll_hw_state.dpll);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 59f8d4270e82..eee734b48919 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -946,6 +946,9 @@ struct intel_crtc_state {
>  	/* Output down scaling is done in LSPCON device */
>  	bool lspcon_downsampling;
>  
> +	/* enable pipe gamma? */
> +	bool gamma_enable;
> +
>  	/* Display Stream compression state */
>  	struct {
>  		bool compression_enable;
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index a45ef98b2f8d..034a355692db 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -739,7 +739,12 @@ vlv_update_clrc(const struct intel_plane_state *plane_state)
>  
>  static u32 vlv_sprite_ctl_crtc(const struct intel_crtc_state *crtc_state)
>  {
> -	return SP_GAMMA_ENABLE;
> +	u32 sprctl = 0;
> +
> +	if (crtc_state->gamma_enable)
> +		sprctl |= SP_GAMMA_ENABLE;
> +
> +	return sprctl;
>  }
>  
>  static u32 vlv_sprite_ctl(const struct intel_crtc_state *crtc_state,
> @@ -915,7 +920,8 @@ static u32 ivb_sprite_ctl_crtc(const struct intel_crtc_state *crtc_state)
>  	struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev);
>  	u32 sprctl = 0;
>  
> -	sprctl |= SPRITE_GAMMA_ENABLE;
> +	if (crtc_state->gamma_enable)
> +		sprctl |= SPRITE_GAMMA_ENABLE;
>  
>  	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
>  		sprctl |= SPRITE_PIPE_CSC_ENABLE;
> @@ -1101,7 +1107,12 @@ g4x_sprite_max_stride(struct intel_plane *plane,
>  
>  static u32 g4x_sprite_ctl_crtc(const struct intel_crtc_state *crtc_state)
>  {
> -	return DVS_GAMMA_ENABLE;
> +	u32 dvscntr = 0;
> +
> +	if (crtc_state->gamma_enable)
> +		dvscntr |= DVS_GAMMA_ENABLE;
> +
> +	return dvscntr;
>  }
>  
>  static u32 g4x_sprite_ctl(const struct intel_crtc_state *crtc_state,
> -- 
> 2.19.2
>
Shankar, Uma Jan. 17, 2019, 5:14 a.m.
>-----Original Message-----
>From: Roper, Matthew D
>Sent: Thursday, January 17, 2019 1:07 AM
>To: Ville Syrjala <ville.syrjala@linux.intel.com>
>Cc: intel-gfx@lists.freedesktop.org; Shankar, Uma <uma.shankar@intel.com>
>Subject: Re: [PATCH 09/13] drm/i915: Track pipe gamma enable/disable in crtc
>state
>
>On Fri, Jan 11, 2019 at 07:08:19PM +0200, Ville Syrjala wrote:
>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>
>> Track whether pipe gamma is enabled or disabled. For now we stick to
>> the current behaviour of always enabling gamma. But we do get working
>> state readout for this now. On SKL+ we use the pipe bottom color as
>> our hardware state. On pre-SKL we read the state back from the primary
>> plane control register.
>> That only really correct for g4x+, as older platforms never gamma
>> correct pipe bottom color. But doing the readout the same way on all
>> platforms is fine, and there is no other way to do it really.
>>
>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
>Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
>
>> ---
>>  drivers/gpu/drm/i915/i915_reg.h      |  8 ++++
>>  drivers/gpu/drm/i915/intel_color.c   | 24 +++++++++++-
>>  drivers/gpu/drm/i915/intel_display.c | 56 ++++++++++++++++++++++------
>>  drivers/gpu/drm/i915/intel_drv.h     |  3 ++
>>  drivers/gpu/drm/i915/intel_sprite.c  | 17 +++++++--
>>  5 files changed, 92 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h
>> b/drivers/gpu/drm/i915/i915_reg.h index 9d17ba199be4..7f0913bc1b47
>> 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -5707,6 +5707,14 @@ enum {
>>  #define   PIPEMISC_DITHER_TYPE_SP	(0 << 2)
>>  #define PIPEMISC(pipe)			_MMIO_PIPE2(pipe,
>_PIPE_MISC_A)
>>
>> +/* SKL+ pipe bottom color */
>> +#define _PIPE_BOTTOM_COLOR_A		0x70034
>> +#define _PIPE_BOTTOM_COLOR_B		0x71034
>> +#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)		_MMIO_PIPE2(pipe,
>_PIPE_BOTTOM_COLOR_A)
>> +
>>  #define VLV_DPFLIPSTAT
>	_MMIO(VLV_DISPLAY_BASE + 0x70028)
>>  #define   PIPEB_LINE_COMPARE_INT_EN		(1 << 29)
>>  #define   PIPEB_HLINE_INT_EN			(1 << 28)
>> diff --git a/drivers/gpu/drm/i915/intel_color.c
>> b/drivers/gpu/drm/i915/intel_color.c
>> index 6fdbfa8c4008..313b281204fa 100644
>> --- a/drivers/gpu/drm/i915/intel_color.c
>> +++ b/drivers/gpu/drm/i915/intel_color.c
>> @@ -387,6 +387,24 @@ static void hsw_color_commit(const struct
>intel_crtc_state *crtc_state)
>>  	ilk_load_csc_matrix(crtc_state);
>>  }
>>
>> +static void skl_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 = 0;

We can initialize this above itself.

>> +	if (crtc_state->gamma_enable)
>> +		val |= PIPE_BOTTOM_GAMMA_ENABLE;
>> +	val |= PIPE_BOTTOM_CSC_ENABLE;
>> +	I915_WRITE(PIPE_BOTTOM_COLOR(pipe), val);
>> +
>> +	I915_WRITE(GAMMA_MODE(crtc->pipe), crtc_state->gamma_mode);
>> +
>> +	ilk_load_csc_matrix(crtc_state);
>> +}
>> +
>>  static void bdw_load_degamma_lut(const struct intel_crtc_state
>> *crtc_state)  {
>>  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
>> @@ -624,6 +642,8 @@ int intel_color_check(struct intel_crtc_state
>*crtc_state)
>>  	degamma_length = INTEL_INFO(dev_priv)->color.degamma_lut_size;
>>  	gamma_length = INTEL_INFO(dev_priv)->color.gamma_lut_size;
>>
>> +	crtc_state->gamma_enable = true;
>> +
>>  	/*
>>  	 * We also allow no degamma lut/ctm and a gamma lut at the legacy
>>  	 * size (256 entries).
>> @@ -674,7 +694,9 @@ void intel_color_init(struct intel_crtc *crtc)
>>  		else
>>  			dev_priv->display.load_luts = i9xx_load_luts;
>>
>> -		if (INTEL_GEN(dev_priv) >= 8 || IS_HASWELL(dev_priv))
>> +		if (INTEL_GEN(dev_priv) >= 9)
>> +			dev_priv->display.color_commit = skl_color_commit;
>> +		else if (IS_BROADWELL(dev_priv) || IS_HASWELL(dev_priv))
>>  			dev_priv->display.color_commit = hsw_color_commit;
>>  		else
>>  			dev_priv->display.color_commit = ilk_color_commit; diff
>--git
>> a/drivers/gpu/drm/i915/intel_display.c
>> b/drivers/gpu/drm/i915/intel_display.c
>> index 90afcae91b30..896ce95790cb 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -3186,7 +3186,8 @@ static u32 i9xx_plane_ctl_crtc(const struct
>intel_crtc_state *crtc_state)
>>  	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>>  	u32 dspcntr = 0;
>>
>> -	dspcntr |= DISPPLANE_GAMMA_ENABLE;
>> +	if (crtc_state->gamma_enable)
>> +		dspcntr |= DISPPLANE_GAMMA_ENABLE;
>>
>>  	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
>>  		dspcntr |= DISPPLANE_PIPE_CSC_ENABLE; @@ -3664,7 +3665,9
>@@ u32
>> skl_plane_ctl_crtc(const struct intel_crtc_state *crtc_state)
>>  	if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))
>>  		return plane_ctl;
>>
>> -	plane_ctl |= PLANE_CTL_PIPE_GAMMA_ENABLE;
>> +	if (crtc_state->gamma_enable)
>> +		plane_ctl |= PLANE_CTL_PIPE_GAMMA_ENABLE;
>> +
>>  	plane_ctl |= PLANE_CTL_PIPE_CSC_ENABLE;
>>
>>  	return plane_ctl;
>> @@ -3717,7 +3720,9 @@ u32 glk_plane_color_ctl_crtc(const struct
>intel_crtc_state *crtc_state)
>>  	if (INTEL_GEN(dev_priv) >= 11)
>>  		return plane_color_ctl;
>>
>> -	plane_color_ctl |= PLANE_COLOR_PIPE_GAMMA_ENABLE;
>> +	if (crtc_state->gamma_enable)
>> +		plane_color_ctl |= PLANE_COLOR_PIPE_GAMMA_ENABLE;
>> +
>>  	plane_color_ctl |= PLANE_COLOR_PIPE_CSC_ENABLE;
>>
>>  	return plane_color_ctl;
>> @@ -3925,7 +3930,6 @@ static void intel_update_pipe_config(const struct
>intel_crtc_state *old_crtc_sta
>>  		   ((new_crtc_state->pipe_src_w - 1) << 16) |
>>  		   (new_crtc_state->pipe_src_h - 1));
>>
>> -	/* on skylake this is done by detaching scalers */

Is this intentional ? Seems unrelated to the patch.

With the above minor nits fixed:
Reviewed-by: Uma Shankar <uma.shankar@intel.com>

>>  	if (INTEL_GEN(dev_priv) >= 9) {
>>  		skl_detach_scalers(new_crtc_state);
>>
>> @@ -8036,6 +8040,20 @@ static void intel_get_crtc_ycbcr_config(struct
>intel_crtc *crtc,
>>  	pipe_config->output_format = output;  }
>>
>> +static void i9xx_get_pipe_color_config(struct intel_crtc_state
>> +*crtc_state) {
>> +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
>> +	struct intel_plane *plane = to_intel_plane(crtc->base.primary);
>> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>> +	enum i9xx_plane_id i9xx_plane = plane->i9xx_plane;
>> +	u32 tmp;
>> +
>> +	tmp = I915_READ(DSPCNTR(i9xx_plane));
>> +
>> +	if (tmp & DISPPLANE_GAMMA_ENABLE)
>> +		crtc_state->gamma_enable = true;
>> +}
>> +
>>  static bool i9xx_get_pipe_config(struct intel_crtc *crtc,
>>  				 struct intel_crtc_state *pipe_config)  { @@ -
>8082,6 +8100,8 @@
>> static bool i9xx_get_pipe_config(struct intel_crtc *crtc,
>>  	pipe_config->gamma_mode = (tmp &
>PIPECONF_GAMMA_MODE_MASK_I9XX) >>
>>  		PIPECONF_GAMMA_MODE_SHIFT;
>>
>> +	i9xx_get_pipe_color_config(pipe_config);
>> +
>>  	if (INTEL_GEN(dev_priv) < 4)
>>  		pipe_config->double_wide = tmp & PIPECONF_DOUBLE_WIDE;
>>
>> @@ -9157,6 +9177,8 @@ static bool ironlake_get_pipe_config(struct intel_crtc
>*crtc,
>>  	pipe_config->gamma_mode = (tmp &
>PIPECONF_GAMMA_MODE_MASK_ILK) >>
>>  		PIPECONF_GAMMA_MODE_SHIFT;
>>
>> +	i9xx_get_pipe_color_config(pipe_config);
>> +
>>  	if (I915_READ(PCH_TRANSCONF(crtc->pipe)) & TRANS_ENABLE) {
>>  		struct intel_shared_dpll *pll;
>>  		enum intel_dpll_id pll_id;
>> @@ -9785,6 +9807,15 @@ static bool haswell_get_pipe_config(struct
>intel_crtc *crtc,
>>  	pipe_config->gamma_mode =
>>  		I915_READ(GAMMA_MODE(crtc->pipe)) &
>GAMMA_MODE_MODE_MASK;
>>
>> +	if (INTEL_GEN(dev_priv) >= 9) {
>> +		u32 tmp = I915_READ(PIPE_BOTTOM_COLOR(crtc->pipe));
>> +
>> +		if (tmp & PIPE_BOTTOM_GAMMA_ENABLE)
>> +			pipe_config->gamma_enable = true;
>> +	} else {
>> +		i9xx_get_pipe_color_config(pipe_config);
>> +	}
>> +
>>  	power_domain = POWER_DOMAIN_PIPE_PANEL_FITTER(crtc->pipe);
>>  	if (intel_display_power_get_if_enabled(dev_priv, power_domain)) {
>>  		power_domain_mask |= BIT_ULL(power_domain); @@ -9953,7
>+9984,12 @@
>> i845_cursor_max_stride(struct intel_plane *plane,
>>
>>  static u32 i845_cursor_ctl_crtc(const struct intel_crtc_state
>> *crtc_state)  {
>> -	return CURSOR_GAMMA_ENABLE;
>> +	u32 cntl = 0;
>> +
>> +	if (crtc_state->gamma_enable)
>> +		cntl |= CURSOR_GAMMA_ENABLE;
>> +
>> +	return cntl;
>>  }
>>
>>  static u32 i845_cursor_ctl(const struct intel_crtc_state *crtc_state,
>> @@ -10105,7 +10141,8 @@ static u32 i9xx_cursor_ctl_crtc(const struct
>intel_crtc_state *crtc_state)
>>  	if (INTEL_GEN(dev_priv) >= 11)
>>  		return cntl;
>>
>> -	cntl |= MCURSOR_GAMMA_ENABLE;
>> +	if (crtc_state->gamma_enable)
>> +		cntl = MCURSOR_GAMMA_ENABLE;
>>
>>  	if (HAS_DDI(dev_priv))
>>  		cntl |= MCURSOR_PIPE_CSC_ENABLE;
>> @@ -11094,12 +11131,6 @@ static int intel_crtc_atomic_check(struct
>drm_crtc *crtc,
>>  		ret = intel_color_check(pipe_config);
>>  		if (ret)
>>  			return ret;
>> -
>> -		/*
>> -		 * Changing color management on Intel hardware is
>> -		 * handled as part of planes update.
>> -		 */
>> -		crtc_state->planes_changed = true;
>>  	}
>>
>>  	ret = 0;
>> @@ -11988,6 +12019,7 @@ 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_BOOL(gamma_enable);
>>
>>  	PIPE_CONF_CHECK_P(shared_dpll);
>>  	PIPE_CONF_CHECK_X(dpll_hw_state.dpll);
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h
>> b/drivers/gpu/drm/i915/intel_drv.h
>> index 59f8d4270e82..eee734b48919 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -946,6 +946,9 @@ struct intel_crtc_state {
>>  	/* Output down scaling is done in LSPCON device */
>>  	bool lspcon_downsampling;
>>
>> +	/* enable pipe gamma? */
>> +	bool gamma_enable;
>> +
>>  	/* Display Stream compression state */
>>  	struct {
>>  		bool compression_enable;
>> diff --git a/drivers/gpu/drm/i915/intel_sprite.c
>> b/drivers/gpu/drm/i915/intel_sprite.c
>> index a45ef98b2f8d..034a355692db 100644
>> --- a/drivers/gpu/drm/i915/intel_sprite.c
>> +++ b/drivers/gpu/drm/i915/intel_sprite.c
>> @@ -739,7 +739,12 @@ vlv_update_clrc(const struct intel_plane_state
>> *plane_state)
>>
>>  static u32 vlv_sprite_ctl_crtc(const struct intel_crtc_state
>> *crtc_state)  {
>> -	return SP_GAMMA_ENABLE;
>> +	u32 sprctl = 0;
>> +
>> +	if (crtc_state->gamma_enable)
>> +		sprctl |= SP_GAMMA_ENABLE;
>> +
>> +	return sprctl;
>>  }
>>
>>  static u32 vlv_sprite_ctl(const struct intel_crtc_state *crtc_state,
>> @@ -915,7 +920,8 @@ static u32 ivb_sprite_ctl_crtc(const struct
>intel_crtc_state *crtc_state)
>>  	struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev);
>>  	u32 sprctl = 0;
>>
>> -	sprctl |= SPRITE_GAMMA_ENABLE;
>> +	if (crtc_state->gamma_enable)
>> +		sprctl |= SPRITE_GAMMA_ENABLE;
>>
>>  	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
>>  		sprctl |= SPRITE_PIPE_CSC_ENABLE;
>> @@ -1101,7 +1107,12 @@ g4x_sprite_max_stride(struct intel_plane
>> *plane,
>>
>>  static u32 g4x_sprite_ctl_crtc(const struct intel_crtc_state
>> *crtc_state)  {
>> -	return DVS_GAMMA_ENABLE;
>> +	u32 dvscntr = 0;
>> +
>> +	if (crtc_state->gamma_enable)
>> +		dvscntr |= DVS_GAMMA_ENABLE;
>> +
>> +	return dvscntr;
>>  }
>>
>>  static u32 g4x_sprite_ctl(const struct intel_crtc_state *crtc_state,
>> --
>> 2.19.2
>>
>
>--
>Matt Roper
>Graphics Software Engineer
>IoTG Platform Enabling & Development
>Intel Corporation
>(916) 356-2795
Ville Syrjala Jan. 17, 2019, 2:57 p.m.
On Thu, Jan 17, 2019 at 05:14:06AM +0000, Shankar, Uma wrote:
> 
> 
> >-----Original Message-----
> >From: Roper, Matthew D
> >Sent: Thursday, January 17, 2019 1:07 AM
> >To: Ville Syrjala <ville.syrjala@linux.intel.com>
> >Cc: intel-gfx@lists.freedesktop.org; Shankar, Uma <uma.shankar@intel.com>
> >Subject: Re: [PATCH 09/13] drm/i915: Track pipe gamma enable/disable in crtc
> >state
> >
> >On Fri, Jan 11, 2019 at 07:08:19PM +0200, Ville Syrjala wrote:
> >> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>
> >> Track whether pipe gamma is enabled or disabled. For now we stick to
> >> the current behaviour of always enabling gamma. But we do get working
> >> state readout for this now. On SKL+ we use the pipe bottom color as
> >> our hardware state. On pre-SKL we read the state back from the primary
> >> plane control register.
> >> That only really correct for g4x+, as older platforms never gamma
> >> correct pipe bottom color. But doing the readout the same way on all
> >> platforms is fine, and there is no other way to do it really.
> >>
> >> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> >Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
> >
> >> ---
> >>  drivers/gpu/drm/i915/i915_reg.h      |  8 ++++
> >>  drivers/gpu/drm/i915/intel_color.c   | 24 +++++++++++-
> >>  drivers/gpu/drm/i915/intel_display.c | 56 ++++++++++++++++++++++------
> >>  drivers/gpu/drm/i915/intel_drv.h     |  3 ++
> >>  drivers/gpu/drm/i915/intel_sprite.c  | 17 +++++++--
> >>  5 files changed, 92 insertions(+), 16 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/i915_reg.h
> >> b/drivers/gpu/drm/i915/i915_reg.h index 9d17ba199be4..7f0913bc1b47
> >> 100644
> >> --- a/drivers/gpu/drm/i915/i915_reg.h
> >> +++ b/drivers/gpu/drm/i915/i915_reg.h
> >> @@ -5707,6 +5707,14 @@ enum {
> >>  #define   PIPEMISC_DITHER_TYPE_SP	(0 << 2)
> >>  #define PIPEMISC(pipe)			_MMIO_PIPE2(pipe,
> >_PIPE_MISC_A)
> >>
> >> +/* SKL+ pipe bottom color */
> >> +#define _PIPE_BOTTOM_COLOR_A		0x70034
> >> +#define _PIPE_BOTTOM_COLOR_B		0x71034
> >> +#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)		_MMIO_PIPE2(pipe,
> >_PIPE_BOTTOM_COLOR_A)
> >> +
> >>  #define VLV_DPFLIPSTAT
> >	_MMIO(VLV_DISPLAY_BASE + 0x70028)
> >>  #define   PIPEB_LINE_COMPARE_INT_EN		(1 << 29)
> >>  #define   PIPEB_HLINE_INT_EN			(1 << 28)
> >> diff --git a/drivers/gpu/drm/i915/intel_color.c
> >> b/drivers/gpu/drm/i915/intel_color.c
> >> index 6fdbfa8c4008..313b281204fa 100644
> >> --- a/drivers/gpu/drm/i915/intel_color.c
> >> +++ b/drivers/gpu/drm/i915/intel_color.c
> >> @@ -387,6 +387,24 @@ static void hsw_color_commit(const struct
> >intel_crtc_state *crtc_state)
> >>  	ilk_load_csc_matrix(crtc_state);
> >>  }
> >>
> >> +static void skl_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 = 0;
> 
> We can initialize this above itself.

Ack.

> 
> >> +	if (crtc_state->gamma_enable)
> >> +		val |= PIPE_BOTTOM_GAMMA_ENABLE;
> >> +	val |= PIPE_BOTTOM_CSC_ENABLE;
> >> +	I915_WRITE(PIPE_BOTTOM_COLOR(pipe), val);
> >> +
> >> +	I915_WRITE(GAMMA_MODE(crtc->pipe), crtc_state->gamma_mode);
> >> +
> >> +	ilk_load_csc_matrix(crtc_state);
> >> +}
> >> +
> >>  static void bdw_load_degamma_lut(const struct intel_crtc_state
> >> *crtc_state)  {
> >>  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> >> @@ -624,6 +642,8 @@ int intel_color_check(struct intel_crtc_state
> >*crtc_state)
> >>  	degamma_length = INTEL_INFO(dev_priv)->color.degamma_lut_size;
> >>  	gamma_length = INTEL_INFO(dev_priv)->color.gamma_lut_size;
> >>
> >> +	crtc_state->gamma_enable = true;
> >> +
> >>  	/*
> >>  	 * We also allow no degamma lut/ctm and a gamma lut at the legacy
> >>  	 * size (256 entries).
> >> @@ -674,7 +694,9 @@ void intel_color_init(struct intel_crtc *crtc)
> >>  		else
> >>  			dev_priv->display.load_luts = i9xx_load_luts;
> >>
> >> -		if (INTEL_GEN(dev_priv) >= 8 || IS_HASWELL(dev_priv))
> >> +		if (INTEL_GEN(dev_priv) >= 9)
> >> +			dev_priv->display.color_commit = skl_color_commit;
> >> +		else if (IS_BROADWELL(dev_priv) || IS_HASWELL(dev_priv))
> >>  			dev_priv->display.color_commit = hsw_color_commit;
> >>  		else
> >>  			dev_priv->display.color_commit = ilk_color_commit; diff
> >--git
> >> a/drivers/gpu/drm/i915/intel_display.c
> >> b/drivers/gpu/drm/i915/intel_display.c
> >> index 90afcae91b30..896ce95790cb 100644
> >> --- a/drivers/gpu/drm/i915/intel_display.c
> >> +++ b/drivers/gpu/drm/i915/intel_display.c
> >> @@ -3186,7 +3186,8 @@ static u32 i9xx_plane_ctl_crtc(const struct
> >intel_crtc_state *crtc_state)
> >>  	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> >>  	u32 dspcntr = 0;
> >>
> >> -	dspcntr |= DISPPLANE_GAMMA_ENABLE;
> >> +	if (crtc_state->gamma_enable)
> >> +		dspcntr |= DISPPLANE_GAMMA_ENABLE;
> >>
> >>  	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
> >>  		dspcntr |= DISPPLANE_PIPE_CSC_ENABLE; @@ -3664,7 +3665,9
> >@@ u32
> >> skl_plane_ctl_crtc(const struct intel_crtc_state *crtc_state)
> >>  	if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))
> >>  		return plane_ctl;
> >>
> >> -	plane_ctl |= PLANE_CTL_PIPE_GAMMA_ENABLE;
> >> +	if (crtc_state->gamma_enable)
> >> +		plane_ctl |= PLANE_CTL_PIPE_GAMMA_ENABLE;
> >> +
> >>  	plane_ctl |= PLANE_CTL_PIPE_CSC_ENABLE;
> >>
> >>  	return plane_ctl;
> >> @@ -3717,7 +3720,9 @@ u32 glk_plane_color_ctl_crtc(const struct
> >intel_crtc_state *crtc_state)
> >>  	if (INTEL_GEN(dev_priv) >= 11)
> >>  		return plane_color_ctl;
> >>
> >> -	plane_color_ctl |= PLANE_COLOR_PIPE_GAMMA_ENABLE;
> >> +	if (crtc_state->gamma_enable)
> >> +		plane_color_ctl |= PLANE_COLOR_PIPE_GAMMA_ENABLE;
> >> +
> >>  	plane_color_ctl |= PLANE_COLOR_PIPE_CSC_ENABLE;
> >>
> >>  	return plane_color_ctl;
> >> @@ -3925,7 +3930,6 @@ static void intel_update_pipe_config(const struct
> >intel_crtc_state *old_crtc_sta
> >>  		   ((new_crtc_state->pipe_src_w - 1) << 16) |
> >>  		   (new_crtc_state->pipe_src_h - 1));
> >>
> >> -	/* on skylake this is done by detaching scalers */
> 
> Is this intentional ? Seems unrelated to the patch.

Some random rebase fail probably.

> 
> With the above minor nits fixed:
> Reviewed-by: Uma Shankar <uma.shankar@intel.com>
> 
> >>  	if (INTEL_GEN(dev_priv) >= 9) {
> >>  		skl_detach_scalers(new_crtc_state);
> >>
> >> @@ -8036,6 +8040,20 @@ static void intel_get_crtc_ycbcr_config(struct
> >intel_crtc *crtc,
> >>  	pipe_config->output_format = output;  }
> >>
> >> +static void i9xx_get_pipe_color_config(struct intel_crtc_state
> >> +*crtc_state) {
> >> +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> >> +	struct intel_plane *plane = to_intel_plane(crtc->base.primary);
> >> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> >> +	enum i9xx_plane_id i9xx_plane = plane->i9xx_plane;
> >> +	u32 tmp;
> >> +
> >> +	tmp = I915_READ(DSPCNTR(i9xx_plane));
> >> +
> >> +	if (tmp & DISPPLANE_GAMMA_ENABLE)
> >> +		crtc_state->gamma_enable = true;
> >> +}
> >> +
> >>  static bool i9xx_get_pipe_config(struct intel_crtc *crtc,
> >>  				 struct intel_crtc_state *pipe_config)  { @@ -
> >8082,6 +8100,8 @@
> >> static bool i9xx_get_pipe_config(struct intel_crtc *crtc,
> >>  	pipe_config->gamma_mode = (tmp &
> >PIPECONF_GAMMA_MODE_MASK_I9XX) >>
> >>  		PIPECONF_GAMMA_MODE_SHIFT;
> >>
> >> +	i9xx_get_pipe_color_config(pipe_config);
> >> +
> >>  	if (INTEL_GEN(dev_priv) < 4)
> >>  		pipe_config->double_wide = tmp & PIPECONF_DOUBLE_WIDE;
> >>
> >> @@ -9157,6 +9177,8 @@ static bool ironlake_get_pipe_config(struct intel_crtc
> >*crtc,
> >>  	pipe_config->gamma_mode = (tmp &
> >PIPECONF_GAMMA_MODE_MASK_ILK) >>
> >>  		PIPECONF_GAMMA_MODE_SHIFT;
> >>
> >> +	i9xx_get_pipe_color_config(pipe_config);
> >> +
> >>  	if (I915_READ(PCH_TRANSCONF(crtc->pipe)) & TRANS_ENABLE) {
> >>  		struct intel_shared_dpll *pll;
> >>  		enum intel_dpll_id pll_id;
> >> @@ -9785,6 +9807,15 @@ static bool haswell_get_pipe_config(struct
> >intel_crtc *crtc,
> >>  	pipe_config->gamma_mode =
> >>  		I915_READ(GAMMA_MODE(crtc->pipe)) &
> >GAMMA_MODE_MODE_MASK;
> >>
> >> +	if (INTEL_GEN(dev_priv) >= 9) {
> >> +		u32 tmp = I915_READ(PIPE_BOTTOM_COLOR(crtc->pipe));
> >> +
> >> +		if (tmp & PIPE_BOTTOM_GAMMA_ENABLE)
> >> +			pipe_config->gamma_enable = true;
> >> +	} else {
> >> +		i9xx_get_pipe_color_config(pipe_config);
> >> +	}
> >> +
> >>  	power_domain = POWER_DOMAIN_PIPE_PANEL_FITTER(crtc->pipe);
> >>  	if (intel_display_power_get_if_enabled(dev_priv, power_domain)) {
> >>  		power_domain_mask |= BIT_ULL(power_domain); @@ -9953,7
> >+9984,12 @@
> >> i845_cursor_max_stride(struct intel_plane *plane,
> >>
> >>  static u32 i845_cursor_ctl_crtc(const struct intel_crtc_state
> >> *crtc_state)  {
> >> -	return CURSOR_GAMMA_ENABLE;
> >> +	u32 cntl = 0;
> >> +
> >> +	if (crtc_state->gamma_enable)
> >> +		cntl |= CURSOR_GAMMA_ENABLE;
> >> +
> >> +	return cntl;
> >>  }
> >>
> >>  static u32 i845_cursor_ctl(const struct intel_crtc_state *crtc_state,
> >> @@ -10105,7 +10141,8 @@ static u32 i9xx_cursor_ctl_crtc(const struct
> >intel_crtc_state *crtc_state)
> >>  	if (INTEL_GEN(dev_priv) >= 11)
> >>  		return cntl;
> >>
> >> -	cntl |= MCURSOR_GAMMA_ENABLE;
> >> +	if (crtc_state->gamma_enable)
> >> +		cntl = MCURSOR_GAMMA_ENABLE;
> >>
> >>  	if (HAS_DDI(dev_priv))
> >>  		cntl |= MCURSOR_PIPE_CSC_ENABLE;
> >> @@ -11094,12 +11131,6 @@ static int intel_crtc_atomic_check(struct
> >drm_crtc *crtc,
> >>  		ret = intel_color_check(pipe_config);
> >>  		if (ret)
> >>  			return ret;
> >> -
> >> -		/*
> >> -		 * Changing color management on Intel hardware is
> >> -		 * handled as part of planes update.
> >> -		 */
> >> -		crtc_state->planes_changed = true;
> >>  	}
> >>
> >>  	ret = 0;
> >> @@ -11988,6 +12019,7 @@ 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_BOOL(gamma_enable);
> >>
> >>  	PIPE_CONF_CHECK_P(shared_dpll);
> >>  	PIPE_CONF_CHECK_X(dpll_hw_state.dpll);
> >> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> >> b/drivers/gpu/drm/i915/intel_drv.h
> >> index 59f8d4270e82..eee734b48919 100644
> >> --- a/drivers/gpu/drm/i915/intel_drv.h
> >> +++ b/drivers/gpu/drm/i915/intel_drv.h
> >> @@ -946,6 +946,9 @@ struct intel_crtc_state {
> >>  	/* Output down scaling is done in LSPCON device */
> >>  	bool lspcon_downsampling;
> >>
> >> +	/* enable pipe gamma? */
> >> +	bool gamma_enable;
> >> +
> >>  	/* Display Stream compression state */
> >>  	struct {
> >>  		bool compression_enable;
> >> diff --git a/drivers/gpu/drm/i915/intel_sprite.c
> >> b/drivers/gpu/drm/i915/intel_sprite.c
> >> index a45ef98b2f8d..034a355692db 100644
> >> --- a/drivers/gpu/drm/i915/intel_sprite.c
> >> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> >> @@ -739,7 +739,12 @@ vlv_update_clrc(const struct intel_plane_state
> >> *plane_state)
> >>
> >>  static u32 vlv_sprite_ctl_crtc(const struct intel_crtc_state
> >> *crtc_state)  {
> >> -	return SP_GAMMA_ENABLE;
> >> +	u32 sprctl = 0;
> >> +
> >> +	if (crtc_state->gamma_enable)
> >> +		sprctl |= SP_GAMMA_ENABLE;
> >> +
> >> +	return sprctl;
> >>  }
> >>
> >>  static u32 vlv_sprite_ctl(const struct intel_crtc_state *crtc_state,
> >> @@ -915,7 +920,8 @@ static u32 ivb_sprite_ctl_crtc(const struct
> >intel_crtc_state *crtc_state)
> >>  	struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev);
> >>  	u32 sprctl = 0;
> >>
> >> -	sprctl |= SPRITE_GAMMA_ENABLE;
> >> +	if (crtc_state->gamma_enable)
> >> +		sprctl |= SPRITE_GAMMA_ENABLE;
> >>
> >>  	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
> >>  		sprctl |= SPRITE_PIPE_CSC_ENABLE;
> >> @@ -1101,7 +1107,12 @@ g4x_sprite_max_stride(struct intel_plane
> >> *plane,
> >>
> >>  static u32 g4x_sprite_ctl_crtc(const struct intel_crtc_state
> >> *crtc_state)  {
> >> -	return DVS_GAMMA_ENABLE;
> >> +	u32 dvscntr = 0;
> >> +
> >> +	if (crtc_state->gamma_enable)
> >> +		dvscntr |= DVS_GAMMA_ENABLE;
> >> +
> >> +	return dvscntr;
> >>  }
> >>
> >>  static u32 g4x_sprite_ctl(const struct intel_crtc_state *crtc_state,
> >> --
> >> 2.19.2
> >>
> >
> >--
> >Matt Roper
> >Graphics Software Engineer
> >IoTG Platform Enabling & Development
> >Intel Corporation
> >(916) 356-2795