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

[Shankar, Uma]

>-----Original Message-----
>From: Roper, Matthew D
>Sent: Friday, December 21, 2018 6:54 AM
>To: Shankar, Uma <uma.shankar at intel.com>
>Cc: intel-gfx at lists.freedesktop.org; Lankhorst, Maarten
><maarten.lankhorst at intel.com>; Syrjala, Ville <ville.syrjala at intel.com>; Sharma,
>Shashank <shashank.sharma at intel.com>
>Subject: Re: [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);
>
>Since 1.0 = 0x100, should we scale this by (0x100 / 0xFF) until we have the newer
>segmented gamma ABI you're working on?

One clarification here, 1.0 is actually 0x10000 (since its 3.16 representation), so we should scale it
up by 0x10000/0xFFFF isn't it ? Not sure if I am missing something.

Also is scaling really a good idea. For values less than 1.0 16bit user input will be accurate and we can
program exactly what user wants to hardware. Problem is only for value of 1.0, making every other value
different may not be a good idea. What do you recommend ?
 
>> +		}
>> +	} 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)); }
>> +
>> +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);
>
>We might as well just stick these constants right in the I915_WRITE call; no need
>to create a separate local variable to hold them.

Sure, will change this. Thanks Matt.

Regards,
Uma Shankar
>
>Matt
>
>> +}
>> +
>>  /* 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
>>
>
>--
>Matt Roper
>Graphics Software Engineer
>IoTG Platform Enabling & Development
>Intel Corporation
>(916) 356-2795