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

Thu Dec 27 13:02:46 UTC 2018

>-----Original Message-----
>From: Ville Syrjälä [mailto:ville.syrjala at linux.intel.com]
>Sent: Friday, December 21, 2018 11:09 PM
>To: Shankar, Uma <uma.shankar at intel.com>
>Cc: intel-gfx at lists.freedesktop.org; Syrjala, Ville <ville.syrjala at intel.com>;
>Lankhorst, Maarten <maarten.lankhorst at intel.com>
>Subject: Re: [Intel-gfx] [v4 2/4] drm/i915/icl: Add icl pipe degamma and gamma
>support
>
>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 at 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.

Sure, will change it.

>> +		}
>> +	} 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.

I feel there are lot of differences with GLK and Gen11+, hence keeping them separate
looks better. Multi-segmented gamma is another addition along with output csc on GEN11.

For GLK, if we have done linear blending we may not need de-gamma and use CSC for any
range correction etc and then apply gamma to make it non-linear as per the desired colorspace.
In case of non-linear input to pipe (this will mostly happen if we have single non-linear
frame directly flipped to plane), degamma will be required to make it linear in order to really do
accurate CSC operations. As of now, degamma for GLK is just a pass through limiting this usecase.

AFAIK, the biggest usecase of Output CSC on Gen11 will be to work on nonlinear RGB data (blended
and after gamma applied for colorspace transfer function) and convert it to YUV formats.

Please let me know your recommendation. Will change the series accordingly.

Thanks & Regards,
Uma Shankar

>> +}
>> +
>> +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 at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>--
>Ville Syrjälä
>Intel