Re: [PATCH 1/3] drm/i915/icl: Add icl pipe degamma and gamma support

[Matt Roper <matthew.d.roper@intel.com>]

On Wed, Oct 24, 2018 at 08:30:11PM +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.
> 
> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h    |  3 ++
>  drivers/gpu/drm/i915/intel_color.c | 74 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 77 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 8bd61f9..3adf689 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -6965,6 +6965,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 5127da2..12c659f 100644
> --- a/drivers/gpu/drm/i915/intel_color.c
> +++ b/drivers/gpu/drm/i915/intel_color.c
> @@ -424,6 +424,7 @@ static void bdw_load_gamma_lut(struct drm_crtc_state *state, u32 offset)
>  	struct drm_i915_private *dev_priv = to_i915(state->crtc->dev);
>  	enum pipe pipe = to_intel_crtc(state->crtc)->pipe;
>  	uint32_t i, lut_size = INTEL_INFO(dev_priv)->color.gamma_lut_size;
> +	uint32_t tmp;
>  
>  	WARN_ON(offset & ~PAL_PREC_INDEX_VALUE_MASK);
>  
> @@ -464,6 +465,11 @@ static void bdw_load_gamma_lut(struct drm_crtc_state *state, u32 offset)
>  		I915_WRITE(PREC_PAL_GC_MAX(pipe, 1), (1 << 16) - 1);
>  		I915_WRITE(PREC_PAL_GC_MAX(pipe, 2), (1 << 16) - 1);
>  	}
> +
> +	if (IS_ICELAKE(dev_priv)) {

We probably want to do "INTEL_GEN(dev_priv) >= 11" here to future-proof
this against future gen's (or other platforms that might match gen11
design without being classified as icelake).

> +		tmp = I915_READ(GAMMA_MODE(pipe));
> +		I915_WRITE(GAMMA_MODE(pipe), tmp | POST_CSC_GAMMA_ENABLE);
> +	}

We generally try to avoid read-modify-write approaches to updating
registers; it's better to build the full register value from scratch to
make sure we know exactly what's in it.

There are a few places in this patch you do a r-m-w cycle.  I'd suggest
we add an intel_crtc_state->gamma_mode field and calculate all the
appropriate bits during the atomic check phase.  Then we can just do a

    if (INTEL_GEN(dev_priv) >= 11)
        I915_WRITE(GAMMA_MODE(pipe), intel_crtc_state->gamma_mode);

once somewhere more central any time crtc_state->color_mgmt_changed is
true.

>  }
>  
>  /* Loads the palette/gamma unit for the CRTC on Broadwell+. */
> @@ -523,6 +529,50 @@ static void glk_load_degamma_lut(struct drm_crtc_state *state)
>  		I915_WRITE(PRE_CSC_GAMC_DATA(pipe), (1 << 16));
>  }
>  
> +static void icl_load_degamma_lut(struct drm_crtc_state *state)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(state->crtc->dev);
> +	enum pipe pipe = to_intel_crtc(state->crtc)->pipe;
> +	const uint32_t lut_size = 33;

Isn't this the value from INTEL_INFO(dev)->color.degamma_lut_size?
Actually, upon closer inspection it looks like LUT size, ranges, etc. is
sort of complicated; more comments farther down.

> +	uint32_t tmp, 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.
> +	 */

The final sentence of the bspec description says (emphasis mine):

        "While in auto increment mode, after performing reads or writes
        to only part of the range, the auto increment bit must be
        cleared *before* resetting the index value."

so I suspect it's technically the first write (which disables the
auto-increment) that ignores the index bits.  Then your second write
both sets the index bits (to 0) and turns auto-increment back on.

> +	I915_WRITE(PRE_CSC_GAMC_INDEX(pipe), 0);
> +	I915_WRITE(PRE_CSC_GAMC_INDEX(pipe), PRE_CSC_GAMC_AUTO_INCREMENT);
> +
> +	if (state->degamma_lut) {
> +		struct drm_color_lut *lut =
> +			(struct drm_color_lut *) state->degamma_lut->data;
> +		for (i = 0; i < lut_size; i++) {
> +			/*
> +			 * Currently Clamp input to 1.0.
> +			 * ToDo: Extend to max 7.0.
> +			 */

This comment is a little confusing.  I think what you mean is that this
for() loop covers the first 33 entries, which represent the range of
inputs from 0 to 1.0.  The remaining two entries are for extended range
of inputs (representing gamma values at 3.0 and 7.0) and are not evenly
spaced.

> +			uint32_t word =
> +				drm_color_lut_extract(lut[i].red, 16);
> +			I915_WRITE(PRE_CSC_GAMC_DATA(pipe), word);
> +		}

Following the loop is where we'd need to deal with the two extended
range entries, right?  So if we leave dealing with extended range
properly as a TODO for now, then I think we want to fill them in with
linear values.  E.g.,

     I915_WRITE(PRE_CSC_GAMC_DATA(pipe), 0x30000);
     I915_WRITE(PRE_CSC_GAMC_DATA(pipe), 0x70000);

Also, I believe the 1.0 input can have a value >= 1.0, so we need to
extract more bits of precision for that.

I'm not really an expert on how this color management stuff gets used by
userspace in practice, but I suspect our current ABI probably needs to
be augmented to deal more complicated gamma LUT's than we support today.
E.g., different ranges with different input values, precision, etc.  Do
we have a clear understanding of if/how userspace wants to deal with
extended range?

> +	} 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);
> +		}

As above, I think we should still fill in the last two entries with
linear values for now.

> +	}
> +
> +	tmp = I915_READ(GAMMA_MODE(pipe));
> +	I915_WRITE(GAMMA_MODE(pipe), tmp | PRE_CSC_GAMMA_ENABLE);
> +
> +	/* Clamp values > 1.0. */
> +	while (i++ < 35)
> +		I915_WRITE(PRE_CSC_GAMC_DATA(pipe), (1 << 16));
> +}
> +
>  static void glk_load_luts(struct drm_crtc_state *state)
>  {
>  	struct drm_crtc *crtc = state->crtc;
> @@ -606,6 +656,28 @@ static void cherryview_load_luts(struct drm_crtc_state *state)
>  	i9xx_load_luts_internal(crtc, NULL, to_intel_crtc_state(state));
>  }
>  
> +static void icl_load_luts(struct drm_crtc_state *state)
> +{
> +	struct drm_crtc *crtc = state->crtc;
> +	struct drm_device *dev = crtc->dev;
> +	struct drm_i915_private *dev_priv = to_i915(dev);
> +	struct intel_crtc_state *intel_state = to_intel_crtc_state(state);
> +	enum pipe pipe = to_intel_crtc(crtc)->pipe;
> +
> +	icl_load_degamma_lut(state);
> +
> +	if (crtc_state_is_legacy_gamma(state)) {
> +		haswell_load_luts(state);
> +		return;
> +	}

This block will only run if there's no degamma LUT, so it might be
slightly easier to read the code if we move this above the
icl_load_degamma_lut() call above?

> +
> +	bdw_load_gamma_lut(state, 0);
> +	intel_state->gamma_mode = GAMMA_MODE_MODE_10BIT;

I think we also need to update PAL_EXT_GC_MAX with a FP3.16 for the 3.0
input value and the PAL_EXT2_GC_MAX for the 7.0 input.  Assuming we want
to leave the ABI details of this until later, I assume we'd handle this
with linear values like I suggested for the degamma extended range
entries?


Matt

> +
> +	I915_WRITE(GAMMA_MODE(pipe), I915_READ(GAMMA_MODE(pipe)) |
> +			intel_state->gamma_mode);
> +}
> +
>  void intel_color_load_luts(struct drm_crtc_state *crtc_state)
>  {
>  	struct drm_device *dev = crtc_state->crtc->dev;
> @@ -662,6 +734,8 @@ void intel_color_init(struct drm_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

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx