Re: [PATCH v2 07/13] drm/i915: Populate gamma_mode for all platforms

[Maarten Lankhorst <maarten.lankhorst@linux.intel.com>]

Op 07-02-2019 om 17:27 schreef Ville Syrjälä:
> On Thu, Feb 07, 2019 at 04:49:47PM +0100, Maarten Lankhorst wrote:
>> Op 05-02-2019 om 17:08 schreef Ville Syrjala:
>>> 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 ede54fdc1676..41111a17e519 100644
>>> --- a/drivers/gpu/drm/i915/i915_reg.h
>>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>>> @@ -5590,9 +5590,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 2a371eed8061..e7dd07490c68 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);
>>>  }
>>>  
>>>  static int check_lut_size(const struct drm_property_blob *lut, int expected)
>>> @@ -649,20 +674,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 ad5d39d81d6e..8e93459f312f 100644
>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>> @@ -3450,7 +3450,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);
>>>  
>>> @@ -7674,6 +7674,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));
>>>  }
>>> @@ -8126,6 +8128,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;
>>>  
>>> @@ -8665,6 +8670,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));
>>>  }
>>> @@ -9199,6 +9206,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;
>>> @@ -12069,6 +12079,8 @@ intel_pipe_config_compare(struct drm_i915_private *dev_priv,
>>>  
>>>  	PIPE_CONF_CHECK_BOOL(double_wide);
>>>  
>>> +	PIPE_CONF_CHECK_X(gamma_mode);
>> Hmm...  We should only check this if adjust is unset, or we will start skipping fastset on a lot of platforms.
> There's no sane macros for this sort of stuff AFAICS. Maybe someone
> should add those first? Heck, I don't even know what 'adjust'
> really means in this context.
>
> Also on a related note, this fastboot thing has made the logs rather
> confusing. It's not at all obvious that all the state mismatch prints
> are harmless (especially when the function is called something_err()).
> Some people (myself included) were already lead astray by these
> apparent "errors".

Could we change the messages then for fastset?

We already pass adjust to pipe_config_err, so we could add something like fastset cannot be done to the message.

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