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

[Ander Conselvan De Oliveira <conselvan2@gmail.com>]

On Tue, 2017-01-24 at 20:05 +0200, Ville Syrjälä wrote:
> On Tue, Jan 24, 2017 at 03:35:54PM +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.
> > 
> > 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 | 105
> > +++++++++++++++++++++++++++++++++++++
> >  3 files changed, 120 insertions(+)
> > 
> > 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 7c240e4..625d8f7 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -8180,12 +8180,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 d81232b..09e0903 100644
> > --- a/drivers/gpu/drm/i915/intel_color.c
> > +++ b/drivers/gpu/drm/i915/intel_color.c
> > @@ -422,6 +422,108 @@ 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.
> > +	 */
> > +	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);
> > +
> Useless empty line.
> 
> I do still wonder how we ended up with the 0.16 scheme for LUTs. I'm
> pretty sure 8.24 or something like that was the plane. Oh well, it is
> what it is.
> 
> > 
> > +}
> > +
> > +static void glk_load_gamma_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 = INTEL_INFO(dev_priv)-
> > >color.gamma_lut_size;
> > +	uint32_t i;
> > +
> > +	I915_WRITE(PREC_PAL_INDEX(pipe), PAL_PREC_AUTO_INCREMENT);
> > +
> > +	if (state->gamma_lut) {
> > +		struct drm_color_lut *lut =
> > +			(struct drm_color_lut *) state->gamma_lut->data;
> const perhaps?
> 
> > 
> > +
> > +		for (i = 0; i < lut_size; i++) {
> > +			uint32_t word =
> > +			(drm_color_lut_extract(lut[i].red, 10) << 20) |
> > +			(drm_color_lut_extract(lut[i].green, 10) << 10) |
> > +			drm_color_lut_extract(lut[i].blue, 10);
> A bit strange calling it a "word", but that's what we apparently do
> elsewhere.
> 
> > 
> > +
> > +			I915_WRITE(PREC_PAL_DATA(pipe), word);
> > +		}
> > +
> > +		/* Program the max register to clamp values > 1.0. */
> > +		I915_WRITE(PREC_PAL_GC_MAX(pipe, 0),
> > +			   drm_color_lut_extract(lut[i].red, 16));
> > +		I915_WRITE(PREC_PAL_GC_MAX(pipe, 1),
> > +			   drm_color_lut_extract(lut[i].green, 16));
> > +		I915_WRITE(PREC_PAL_GC_MAX(pipe, 2),
> > +			   drm_color_lut_extract(lut[i].blue, 16));
> > +	} else {
> > +		for (i = 0; i < lut_size; i++) {
> > +			uint32_t v = (i * ((1 << 10) - 1)) / (lut_size -
> > 1);
> > +
> > +			I915_WRITE(PREC_PAL_DATA(pipe),
> > +				   (v << 20) | (v << 10) | v);
> > +		}
> > +
> > +		I915_WRITE(PREC_PAL_GC_MAX(pipe, 0), (1 << 16) - 1);
> > +		I915_WRITE(PREC_PAL_GC_MAX(pipe, 1), (1 << 16) - 1);
> > +		I915_WRITE(PREC_PAL_GC_MAX(pipe, 2), (1 << 16) - 1);
> > +	}
> > +}
> Hmm. Am I imagining it, or is that code pretty much identical to the
> "broadwell" code?

It is pretty much the same. The broadwell code uses split gamma, so the table
starts from the 512th entry and relies on the load of the degamma lut to leave
the index pointing there. I couldn't figure out how to merge the two without
obscuring this. I could call this function bdw_load_gamma_lut() and add a
parameter to tell where to start, but it just looks weird that the lenght of the
table comes from device info, but the start index would be hardcoded.

Probably still better than duplicated code, so just do that.

Ander

> 
> I think all this LUT code really needs to some heavy handed renaming
> to make things not confusing. But that's a separate issue.
> 
> > 
> > +
> > +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);
> > +	glk_load_gamma_lut(state);
> > +
> > +	intel_state->gamma_mode = GAMMA_MODE_MODE_10BIT;
> > +	I915_WRITE(GAMMA_MODE(pipe), GAMMA_MODE_MODE_10BIT);
> > +	POSTING_READ(GAMMA_MODE(pipe));
> > +
> > +	/*
> > +	 * Reset the index, otherwise it prevents the legacy palette to be
> > +	 * written properly.
> > +	 */
> > +	I915_WRITE(PREC_PAL_INDEX(pipe), 0);
> > +	I915_WRITE(PIPE_CSC_MODE(pipe), 0);
> > +}
> > +
> >  /* Loads the palette/gamma unit for the CRTC on CherryView. */
> >  static void cherryview_load_luts(struct drm_crtc_state *state)
> >  {
> > @@ -540,6 +642,9 @@ void intel_color_init(struct drm_crtc *crtc)
> >  		   IS_BROXTON(dev_priv) || IS_KABYLAKE(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-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx