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]

 




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

>> +	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@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
_______________________________________________
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