Re: [PATCH v2 4/5] drm/i915/glk: Program pipe gamma and degamma tables

["Conselvan De Oliveira, Ander" <ander.conselvan.de.oliveira@intel.com>]

On Thu, 2017-01-26 at 16:21 +0200, Ville Syrjälä wrote:
> On Thu, Jan 26, 2017 at 01:24:24PM +0200, Ander Conselvan de Oliveira wrote:
> > The gamma tables in Geminilake were changed. There is no split-gamma
> > mode. Instead, there is a dedicated degamma table that is enabled
> > whenever pipe CSC is enabled.
> > 
> > The dedicated gamma table has 16 bit precision but doesn't support
> > separate channels. Since that doesn't match the per-channel format of
> > the degamma LUT property, for now only a linear table is loaded and the
> > property ignored.
> > 
> > v2: Remove empty line. (Ville)
> >     Reuse broadwell code. (Ville)
> > 
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@inte
> > l.com>
> > ---
> >  drivers/gpu/drm/i915/i915_pci.c    |  1 +
> >  drivers/gpu/drm/i915/i915_reg.h    | 14 +++++++++
> >  drivers/gpu/drm/i915/intel_color.c | 60
> > +++++++++++++++++++++++++++++++++++++-
> >  3 files changed, 74 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_pci.c
> > b/drivers/gpu/drm/i915/i915_pci.c
> > index ecb487b..df2051b 100644
> > --- a/drivers/gpu/drm/i915/i915_pci.c
> > +++ b/drivers/gpu/drm/i915/i915_pci.c
> > @@ -403,6 +403,7 @@ static const struct intel_device_info
> > intel_geminilake_info = {
> >  	.platform = INTEL_GEMINILAKE,
> >  	.is_alpha_support = 1,
> >  	.ddb_size = 1024,
> > +	.color = { .degamma_lut_size = 0, .gamma_lut_size = 1024 }
> >  };
> >  
> >  static const struct intel_device_info intel_kabylake_info = {
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > b/drivers/gpu/drm/i915/i915_reg.h
> > index 06bbe55..e029691 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -8181,12 +8181,26 @@ enum {
> >  #define _PAL_PREC_EXT_GC_MAX_A	0x4A420
> >  #define _PAL_PREC_EXT_GC_MAX_B	0x4AC20
> >  #define _PAL_PREC_EXT_GC_MAX_C	0x4B420
> > +#define _PAL_PREC_EXT2_GC_MAX_A	0x4A430
> > +#define _PAL_PREC_EXT2_GC_MAX_B	0x4AC30
> > +#define _PAL_PREC_EXT2_GC_MAX_C	0x4B430
> >  
> >  #define PREC_PAL_INDEX(pipe)		_MMIO_PIPE(pipe,
> > _PAL_PREC_INDEX_A, _PAL_PREC_INDEX_B)
> >  #define PREC_PAL_DATA(pipe)		_MMIO_PIPE(pipe,
> > _PAL_PREC_DATA_A, _PAL_PREC_DATA_B)
> >  #define PREC_PAL_GC_MAX(pipe, i)	_MMIO(_PIPE(pipe,
> > _PAL_PREC_GC_MAX_A, _PAL_PREC_GC_MAX_B) + (i) * 4)
> >  #define PREC_PAL_EXT_GC_MAX(pipe, i)	_MMIO(_PIPE(pipe,
> > _PAL_PREC_EXT_GC_MAX_A, _PAL_PREC_EXT_GC_MAX_B) + (i) * 4)
> >  
> > +#define _PRE_CSC_GAMC_INDEX_A	0x4A484
> > +#define _PRE_CSC_GAMC_INDEX_B	0x4AC84
> > +#define _PRE_CSC_GAMC_INDEX_C	0x4B484
> > +#define   PRE_CSC_GAMC_AUTO_INCREMENT	(1 << 10)
> > +#define _PRE_CSC_GAMC_DATA_A	0x4A488
> > +#define _PRE_CSC_GAMC_DATA_B	0x4AC88
> > +#define _PRE_CSC_GAMC_DATA_C	0x4B488
> > +
> > +#define PRE_CSC_GAMC_INDEX(pipe)	_MMIO_PIPE(pipe,
> > _PRE_CSC_GAMC_INDEX_A, _PRE_CSC_GAMC_INDEX_B)
> > +#define PRE_CSC_GAMC_DATA(pipe)		_MMIO_PIPE(pipe,
> > _PRE_CSC_GAMC_DATA_A, _PRE_CSC_GAMC_DATA_B)
> > +
> >  /* pipe CSC & degamma/gamma LUTs on CHV */
> >  #define _CGM_PIPE_A_CSC_COEFF01	(VLV_DISPLAY_BASE + 0x67900)
> >  #define _CGM_PIPE_A_CSC_COEFF23	(VLV_DISPLAY_BASE + 0x67904)
> > diff --git a/drivers/gpu/drm/i915/intel_color.c
> > b/drivers/gpu/drm/i915/intel_color.c
> > index 82a3bc9..2125aa3 100644
> > --- a/drivers/gpu/drm/i915/intel_color.c
> > +++ b/drivers/gpu/drm/i915/intel_color.c
> > @@ -380,7 +380,9 @@ static void bdw_load_gamma_lut(struct drm_crtc_state
> > *state, u32 offset)
> >  	WARN_ON(offset & ~PAL_PREC_INDEX_VALUE_MASK);
> >  
> >  	I915_WRITE(PREC_PAL_INDEX(pipe),
> > -		   PAL_PREC_SPLIT_MODE | PAL_PREC_AUTO_INCREMENT | offset);
> > +		   (offset ? PAL_PREC_SPLIT_MODE : 0) |
> > +		   PAL_PREC_AUTO_INCREMENT |
> > +		   offset);
> 
> This confused me for a bit. I was thinking we're using this to write the
> deamma part for the split gamma case as well, which would end up
> disabling the split gamma mode when doing that. But that's not actually
> what's happening since you had another function to write the degamma
> half.
> 
> >  
> >  	if (state->gamma_lut) {
> >  		struct drm_color_lut *lut =
> > @@ -443,6 +445,59 @@ static void broadwell_load_luts(struct drm_crtc_state
> > *state)
> >  	I915_WRITE(PREC_PAL_INDEX(pipe), 0);
> >  }
> >  
> > +static void glk_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;
> > +	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.
> > +	 */
> 
> Interesting. Do we know if the same problem might be present in other
> gamma tables?

I haven't tested, but might be the case for the plane gamma tables, since they
are very similar.

> > +	I915_WRITE(PRE_CSC_GAMC_INDEX(pipe), 0);
> > +	I915_WRITE(PRE_CSC_GAMC_INDEX(pipe), PRE_CSC_GAMC_AUTO_INCREMENT);
> > +
> > +	/*
> > +	 *  FIXME: The pipe degamma table in geminilake doesn't support
> > +	 *  different values per channel, so this just loads a linear
> > table.
> > +	 */
> > +	for (i = 0; i < lut_size; i++) {
> > +		uint32_t v = (i * ((1 << 16) - 1)) / (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) - 1);
> > +}
> > +
> > +static void glk_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;
> > +
> > +	if (crtc_state_is_legacy(state)) {
> > +		haswell_load_luts(state);
> > +		return;
> > +	}
> > +
> > +	glk_load_degamma_lut(state);
> > +	bdw_load_gamma_lut(state, 0);
> > +
> > +	intel_state->gamma_mode = GAMMA_MODE_MODE_10BIT;
> > +	I915_WRITE(GAMMA_MODE(pipe), GAMMA_MODE_MODE_10BIT);
> > +	POSTING_READ(GAMMA_MODE(pipe));
> > +
> > +	I915_WRITE(PIPE_CSC_MODE(pipe), 0);
> 
> Why are we writing the CSC_MODE register here?

Because I can't read the spec. Somehow I got confused about the CSC position
relative to gamma in previous platforms and got to the conclusion it was
necessary to arm CSC after setting the gamma tables. Turns out that's not the
case, as the spec doesn't mention any such thing and as shown by testing.

I'll send a v3.

Ander

> 
> > +}
> > +
> >  /* Loads the palette/gamma unit for the CRTC on CherryView. */
> >  static void cherryview_load_luts(struct drm_crtc_state *state)
> >  {
> > @@ -561,6 +616,9 @@ void intel_color_init(struct drm_crtc *crtc)
> >  		   IS_BROXTON(dev_priv)) {
> >  		dev_priv->display.load_csc_matrix = i9xx_load_csc_matrix;
> >  		dev_priv->display.load_luts = broadwell_load_luts;
> > +	} else if (IS_GEMINILAKE(dev_priv)) {
> > +		dev_priv->display.load_csc_matrix = i9xx_load_csc_matrix;
> > +		dev_priv->display.load_luts = glk_load_luts;
> >  	} else {
> >  		dev_priv->display.load_luts = i9xx_load_luts;
> >  	}
> > -- 
> > 2.5.5
> 
> 
---------------------------------------------------------------------
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki 
Business Identity Code: 0357606 - 4 
Domiciled in Helsinki 

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx