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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxxxxxxx>
> >Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Shankar, Uma <uma.shankar@xxxxxxxxx>
> >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@xxxxxxxxxxxxxxx>
> >>
> >> 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@xxxxxxxxxxxxxxx>
> >
> >Reviewed-by: Matt Roper <matthew.d.roper@xxxxxxxxx>
> >
> >> ---
> >>  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@xxxxxxxxx>
> 
> >>  	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 Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux