[Intel-gfx] [PATCH 2/3] drm/i915/icl: Add icl pipe degamma and gamma support

[Shankar, Uma]

>-----Original Message-----
>From: Maarten Lankhorst [mailto:maarten.lankhorst at linux.intel.com]
>Sent: Wednesday, October 24, 2018 7:04 PM
>To: Shankar, Uma <uma.shankar at intel.com>; intel-gfx at lists.freedesktop.org
>Cc: Syrjala, Ville <ville.syrjala at intel.com>; Lankhorst, Maarten
><maarten.lankhorst at intel.com>
>Subject: Re: [Intel-gfx] [PATCH 2/3] drm/i915/icl: Add icl pipe degamma and
>gamma support
>
>Op 23-10-18 om 22:40 schreef Uma Shankar:
>> Add support for icl pipe degamma and gamma.
>>
>> Signed-off-by: Uma Shankar <uma.shankar at intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_reg.h    |  3 ++
>>  drivers/gpu/drm/i915/intel_color.c | 75
>> ++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 78 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h
>> b/drivers/gpu/drm/i915/i915_reg.h index 8bd61f9..dd0514e 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 << 31)
>? This can't be right.

Ah, My Bad. I tested with directly putting (1 << 30) in driver and while
publishing to external tried to add a macro. Late in the night :(
Thanks for catching this (it was correct in our internal review)

>>  /* 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..7860244 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)) {
>> +		tmp = I915_READ(GAMMA_MODE(pipe));
>> +		I915_WRITE(GAMMA_MODE(pipe), tmp |
>POST_CSC_GAMMA_ENABLE);
>> +	}
>Could we just program the bits we care about, instead of doing this read?

Didn't want to disturb the existing "gamma mode" selected (degamma or gamma),
hence just appended to what was already there in the register. At a time, both
gamma and de-gamma can get called and selected in this registers.

>>  }
>>
>>  /* 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;
>> +	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.
>> +	 */
>> +	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.
>> +			 */
>> +			uint32_t word =
>> +				drm_color_lut_extract(lut[i].red, 16);
>> +			I915_WRITE(PRE_CSC_GAMC_DATA(pipe), word);
>> +		}
>> +	} 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);
>> +		}
>> +	}
>> +
>> +	tmp = I915_READ(GAMMA_MODE(pipe));
>> +	I915_WRITE(GAMMA_MODE(pipe), tmp | PRE_CSC_GAMMA_ENABLE);
>Same here.. we know what gamma mode should be.
>> +
>> +	/* 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,29 @@ 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;
>> +	}
>> +
>> +	bdw_load_gamma_lut(state, 0);
>> +	intel_state->gamma_mode = GAMMA_MODE_MODE_10BIT;
>> +
>> +	I915_WRITE(GAMMA_MODE(pipe), I915_READ(GAMMA_MODE(pipe)) |
>> +			intel_state->gamma_mode);
>> +	POSTING_READ(GAMMA_MODE(pipe));
>Posting read can be removed.

Ok Sure, will update that.

>> +}
>> +
>>  void intel_color_load_luts(struct drm_crtc_state *crtc_state)  {
>>  	struct drm_device *dev = crtc_state->crtc->dev; @@ -662,6 +735,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;
>>  	}
>
>Just curious, how was this tested if we wrongly defined the Post CSC gamma bit?

As mentioned above, this was just a macro fumble while pushing for external :(

Regards,
Uma Shankar

>~Maarten