Re: [v4 2/4] drm/i915/icl: Add icl pipe degamma and gamma support

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Fri, Dec 21, 2018 at 01:29:39AM +0530, Uma Shankar wrote:
> Add support for icl pipe degamma and gamma.
> 
> v2: Removed a POSTING_READ and corrected the Bit
> Definition as per Maarten's comments.
> 
> v3: Addressed Matt's review comments. Removed rmw patterns
> as suggested by Matt.
> 
> v4: Fixed Matt's review comments.
> 
> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h    |  3 ++
>  drivers/gpu/drm/i915/intel_color.c | 67 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 70 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 02af9b5..1852c33 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -7092,6 +7092,9 @@ enum {
>  #define GAMMA_MODE_MODE_12BIT	(2 << 0)
>  #define GAMMA_MODE_MODE_SPLIT	(3 << 0)
>  
> +#define	PRE_CSC_GAMMA_ENABLE	(1 << 31)
> +#define	POST_CSC_GAMMA_ENABLE	(1 << 30)
> +
>  /* DMC/CSR */
>  #define CSR_PROGRAM(i)		_MMIO(0x80000 + (i) * 4)
>  #define CSR_SSP_BASE_ADDR_GEN9	0x00002FC0
> diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
> index f32e4a7..e72d8d6 100644
> --- a/drivers/gpu/drm/i915/intel_color.c
> +++ b/drivers/gpu/drm/i915/intel_color.c
> @@ -534,6 +534,71 @@ static void glk_load_luts(struct intel_crtc_state *crtc_state)
>  	POSTING_READ(GAMMA_MODE(pipe));
>  }
>  
> +static void icl_load_degamma_lut(struct intel_crtc_state *crtc_state)
> +{
> +	struct drm_device *dev = crtc_state->base.crtc->dev;
> +	struct drm_i915_private *dev_priv = to_i915(dev);
> +	enum pipe pipe = to_intel_crtc(crtc_state->base.crtc)->pipe;
> +	const uint32_t lut_size = INTEL_INFO(dev_priv)->color.degamma_lut_size;
> +	uint32_t i;
> +
> +	/*
> +	 * When setting the auto-increment bit, the hardware seems to
> +	 * ignore the index bits, so we need to reset it to index 0
> +	 * separately.
> +	 */
> +	I915_WRITE(PRE_CSC_GAMC_INDEX(pipe), 0);
> +	I915_WRITE(PRE_CSC_GAMC_INDEX(pipe), PRE_CSC_GAMC_AUTO_INCREMENT);
> +
> +	if (crtc_state->base.degamma_lut) {
> +		struct drm_color_lut *lut = crtc_state->base.degamma_lut->data;
> +
> +		for (i = 0; i < lut_size; i++) {
> +			/*
> +			 * First 33 entries represent range from 0 to 1.0
> +			 * 34th and 35th entry will represent extended range
> +			 * inputs 3.0 and 7.0 respectively, currently clamped
> +			 * at 1.0. Since the precision is 16bit, the user value
> +			 * can be directly filled to register.
> +			 * ToDo: Extend to max 7.0.
> +			 */
> +			I915_WRITE(PRE_CSC_GAMC_DATA(pipe), lut[i].red);

red? I would have gone for green.

> +		}
> +	} else {
> +		/* load a linear table. */
> +		for (i = 0; i < lut_size; i++) {
> +			uint32_t v = (i * (1 << 16)) / (lut_size - 1);
> +
> +			I915_WRITE(PRE_CSC_GAMC_DATA(pipe), v);
> +		}
> +	}
> +
> +	/* Clamp values > 1.0. */
> +	while (i++ < 35)
> +		I915_WRITE(PRE_CSC_GAMC_DATA(pipe), (1 << 16));

This is the glk+ degamma lut is it not? Why is this code 
pretending to be icl+ only?

IMO step 1 should be to just reuse the current glk code for icl.
Which I think boils down to just setting the correct bit(s) in
GAMMA_MODE. After that we can think of extending it to support
user provided degamma. 

There is a bit of a problem with that on glk/cnl though
since we don't have the output csc. Hence when the pipe csc
is used we need to either reject gamma (which doesn't sound 
very prone to work with legacy userspace) or we program the
degamma LUT with the data meant for the gamma LUT which has
its own problems.

> +}
> +
> +static void icl_load_luts(struct intel_crtc_state *crtc_state)
> +{
> +	struct drm_crtc *crtc = crtc_state->base.crtc;
> +	struct drm_device *dev = crtc_state->base.crtc->dev;
> +	struct drm_i915_private *dev_priv = to_i915(dev);
> +	enum pipe pipe = to_intel_crtc(crtc)->pipe;
> +	u32 gamma_mode = 0;
> +
> +	if (crtc_state_is_legacy_gamma(crtc_state)) {
> +		haswell_load_luts(crtc_state);
> +		return;
> +	}
> +
> +	icl_load_degamma_lut(crtc_state);
> +	bdw_load_gamma_lut(crtc_state, 0);
> +
> +	gamma_mode = GAMMA_MODE_MODE_10BIT | PRE_CSC_GAMMA_ENABLE
> +			| POST_CSC_GAMMA_ENABLE;
> +	I915_WRITE(GAMMA_MODE(pipe), gamma_mode);
> +}
> +
>  /* Loads the palette/gamma unit for the CRTC on CherryView. */
>  static void cherryview_load_luts(struct intel_crtc_state *crtc_state)
>  {
> @@ -649,6 +714,8 @@ void intel_color_init(struct intel_crtc *crtc)
>  	} else if (IS_GEMINILAKE(dev_priv) || IS_CANNONLAKE(dev_priv)) {
>  		dev_priv->display.load_csc_matrix = ilk_load_csc_matrix;
>  		dev_priv->display.load_luts = glk_load_luts;
> +	} else if (IS_ICELAKE(dev_priv)) {
> +		dev_priv->display.load_luts = icl_load_luts;
>  	} else {
>  		dev_priv->display.load_luts = i9xx_load_luts;
>  	}
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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