Re: [PATCH 19/22] drm/i915: BDW: Pipe level Gamma correction

["Sharma, Shashank" <shashank.sharma@intel.com>]

Regards
Shashank

On 10/12/2015 11:39 PM, Rob Bradford wrote:
> On Sat, 2015-10-10 at 00:59 +0530, Shashank Sharma wrote:
>> BDW/SKL/BXT platforms support various Gamma correction modes
>> which are:
>> 1. Legacy 8-bit mode
>> 2. 10-bit Split Gamma mode
>> 3. 12-bit mode
>>
>> This patch does the following:
>> 1. Adds the core function to program Gamma correction values
>>     for BDW/SKL/BXT platforms
>> 2. Adds Gamma correction macros/defines
>>
>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
>> Signed-off-by: Kausal Malladi <kausalmalladi@gmail.com>
>> ---
>>   drivers/gpu/drm/i915/i915_reg.h            |  25 ++-
>>   drivers/gpu/drm/i915/intel_color_manager.c | 248
>> +++++++++++++++++++++++++++++
>>   drivers/gpu/drm/i915/intel_color_manager.h |   6 +
>>   3 files changed, 277 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h
>> b/drivers/gpu/drm/i915/i915_reg.h
>> index 5825ab2..ed50f75 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -5647,11 +5647,15 @@ enum skl_disp_power_wells {
>>   /* legacy palette */
>>   #define _LGC_PALETTE_A           0x4a000
>>   #define _LGC_PALETTE_B           0x4a800
>> -#define LGC_PALETTE(pipe, i) (_PIPE(pipe, _LGC_PALETTE_A,
>> _LGC_PALETTE_B) + (i) * 4)
>> +#define _LGC_PALETTE_C           0x4b000
>> +#define LGC_PALETTE(pipe, i) (_PIPE3(pipe, _LGC_PALETTE_A,
>> _LGC_PALETTE_B, \
>> +			 _LGC_PALETTE_C) + (i) * 4)
>>
>>   #define _GAMMA_MODE_A		0x4a480
>>   #define _GAMMA_MODE_B		0x4ac80
>> -#define GAMMA_MODE(pipe) _PIPE(pipe, _GAMMA_MODE_A, _GAMMA_MODE_B)
>> +#define _GAMMA_MODE_C		0x4b480
>> +#define GAMMA_MODE(pipe) \
>> +	_PIPE3(pipe, _GAMMA_MODE_A, _GAMMA_MODE_B, _GAMMA_MODE_C)
>>   #define GAMMA_MODE_MODE_MASK	(3 << 0)
>>   #define GAMMA_MODE_MODE_8BIT	(0 << 0)
>>   #define GAMMA_MODE_MODE_10BIT	(1 << 0)
>> @@ -8062,6 +8066,23 @@ enum skl_disp_power_wells {
>>   #define _PIPE_CSC_BASE(pipe) \
>>   	(_PIPE3(pipe, PIPEA_CGM_CSC, PIPEB_CGM_CSC, PIPEC_CGM_CSC))
>>
>> +/* BDW gamma correction */
>> +#define PAL_PREC_INDEX_A                       0x4A400
>> +#define PAL_PREC_INDEX_B                       0x4AC00
>> +#define PAL_PREC_INDEX_C                       0x4B400
>> +#define PAL_PREC_DATA_A                        0x4A404
>> +#define PAL_PREC_DATA_B                        0x4AC04
>> +#define PAL_PREC_DATA_C                        0x4B404
>> +#define PAL_PREC_GCMAX_A			0x4A410
>> +#define PAL_PREC_GCMAX_B			0x4AC10
>> +#define PAL_PREC_GCMAX_C			0x4B410
>> +
>> +#define _PREC_PAL_INDEX(pipe) \
>> +	(_PIPE3(pipe, PAL_PREC_INDEX_A, PAL_PREC_INDEX_B,
>> PAL_PREC_INDEX_C))
>> +#define _PREC_PAL_DATA(pipe) \
>> +	(_PIPE3(pipe, PAL_PREC_DATA_A, PAL_PREC_DATA_B,
>> PAL_PREC_DATA_C))
>> +#define _PREC_PAL_GCMAX(pipe) \
>> +	(_PIPE3(pipe, PAL_PREC_GCMAX_A, PAL_PREC_GCMAX_B,
>> PAL_PREC_GCMAX_C))
>>
>>
>>   #endif /* _I915_REG_H_ */
>> diff --git a/drivers/gpu/drm/i915/intel_color_manager.c
>> b/drivers/gpu/drm/i915/intel_color_manager.c
>> index d5315b2..74f8fc3 100644
>> --- a/drivers/gpu/drm/i915/intel_color_manager.c
>> +++ b/drivers/gpu/drm/i915/intel_color_manager.c
>> @@ -26,6 +26,252 @@
>>    */
>>
>>   #include "intel_color_manager.h"
>> +static void bdw_write_10bit_gamma_precision(struct drm_device *dev,
>> +	struct drm_r32g32b32 *correction_values, u32 pal_prec_data,
>> +			u32 no_of_coeff)
>> +{
>> +	u16 blue_fract, green_fract, red_fract;
>> +	u32 word = 0;
>> +	u32 count = 0;
>> +	u32 blue, green, red;
>> +	struct drm_i915_private *dev_priv = dev->dev_private;
>> +
>> +	while (count < no_of_coeff) {
>> +
>> +		blue = correction_values[count].b32;
>> +		green = correction_values[count].g32;
>> +		red = correction_values[count].r32;
>> +
>> +		/*
>> +		* Maximum possible gamma correction value supported
>> +		* for BDW is 0xFFFFFFFF, so clip the values
>> accordingly
>> +		*/
>
> I think you mean clamp not clip.
Yes, will fix this.
>
>> +		if (blue >= BDW_MAX_GAMMA)
>> +			blue = BDW_MAX_GAMMA;
>> +		if (green >= BDW_MAX_GAMMA)
>> +			green = BDW_MAX_GAMMA;
>> +		if (red >= BDW_MAX_GAMMA)
>> +			red = BDW_MAX_GAMMA;
>
> So this handles the issue that was raised before that 1.0 in 8.24
> should map to 1023.
Yes.
>
>> +
>> +		/*
>> +		* Gamma correction values are sent in 8.24 format
>> +		* with 8 int and 24 fraction bits. BDW 10 bit gamma
>> +		* unit expects correction registers to be programmed
>> in
>> +		* 0.10 format, with 0 int and 16 fraction bits. So
>> take
>> +		* MSB 10 bit values(bits 23-14) from the fraction
>> part and
>> +		* prepare the correction registers.
>> +		*/
>> +		blue_fract = GET_BITS(blue, 14, 10);
>> +		green_fract = GET_BITS(green, 14, 10);
>> +		red_fract = GET_BITS(red, 14, 10);
>> +
>
> I think this should round to the nearest rather than floor.
>
Why ? we are getting the exact values already.
>> +		/* Arrange: Red (29:20) Green (19:10) and Blue (9:0)
>> */
>> +		SET_BITS(word, red_fract, 20, 10);
>> +		SET_BITS(word, red_fract, 10, 10);
>> +		SET_BITS(word, red_fract, 0, 10);
>
> Red is my favourite colour too, is that why you programmed all the
> channels to the red bits? :-)
Guilty :). While testing, all the channels have the same values, so it 
dint matter. I will fix this.
>
>> +		I915_WRITE(pal_prec_data, word);
>> +		count++;
>> +	}
>> +	DRM_DEBUG_DRIVER("Gamma correction programmed\n");
>> +}
>> +
>> +void bdw_write_12bit_gamma_precision(struct drm_device *dev,
>> +	struct drm_r32g32b32 *correction_values, u32 pal_prec_data,
>> +		enum pipe pipe)
>> +{
>> +	uint16_t blue_fract, green_fract, red_fract;
>> +	uint32_t gcmax;
>> +	uint32_t word = 0;
>> +	uint32_t count = 0;
>> +	uint32_t gcmax_reg;
>> +	struct drm_i915_private *dev_priv = dev->dev_private;
>> +
>> +	/* Program first 512 values in precision palette */
>> +	while (count < BDW_12BIT_GAMMA_MAX_VALS - 1) {
>> +		/*
>> +		* Framework's general gamma format is 8.24 (8 int 16
>> fraction)
>> +		* BDW Platform's supported gamma format is 16 bit
>> correction
>> +		* values in 0.16 format. So extract higher 16
>> fraction bits
>> +		* from 8.24 gamma correction values.
>> +		*/
>> +		red_fract = GET_BITS(correction_values[count].r32,
>> 8, 16);
>> +		green_fract = GET_BITS(correction_values[count].g32,
>> 8, 16);
>> +		blue_fract = GET_BITS(correction_values[count].b32,
>> 8, 16);
>
> Again, 1.0 in 8.24 will result in you programming zeros rather than
> max.
Will add the max check and conversion here also.
>
>> +		/*
>> +		* From the bspec:
>> +		* For 12 bit gamma correction, program precision
>> palette
>> +		* with 16 bits per color in a 0.16 format with 0
>> integer and
>> +		* 16 fractional bits (upper 10 bits in odd indexes,
>> lower 6
>> +		* bits in even indexes)
>> +		*/
>> +
>> +		/* Even index: Lower 6 bits from correction should
>> go as MSB */
>> +		SET_BITS(word, GET_BITS(red_fract, 0, 6), 24, 6);
>> +		SET_BITS(word, GET_BITS(green_fract, 0, 6), 14, 6);
>> +		SET_BITS(word, GET_BITS(blue_fract, 0, 6), 4, 6);
>> +		I915_WRITE(pal_prec_data, word);
>> +
>> +		word = 0x0;
>> +		/* Odd index: Upper 10 bits of correction should go
>> as MSB */
>> +		SET_BITS(word, GET_BITS(red_fract, 6, 10), 20, 10);
>> +		SET_BITS(word, GET_BITS(green_fract, 6, 10), 10,
>> 10);
>> +		SET_BITS(word, GET_BITS(blue_fract, 6, 10), 0, 10);
>> +
>> +		I915_WRITE(pal_prec_data, word);
>> +		count++;
>> +	}
>> +
>> +	/* Now program the 513th value in GCMAX regs */
>> +	word = 0;
>> +	gcmax_reg = _PREC_PAL_GCMAX(pipe);
>> +	gcmax = min_t(u32, GET_BITS(correction_values[count].r32, 8,
>> 17),
>> +				BDW_MAX_GAMMA);
>> +	SET_BITS(word, gcmax, 0, 17);
>> +	I915_WRITE(gcmax_reg, word);
>> +	gcmax_reg += 4;
>> +
>> +	word = 0;
>> +	gcmax = min_t(u32, GET_BITS(correction_values[count].g32, 8,
>> 17),
>> +				BDW_MAX_GAMMA);
>> +	SET_BITS(word, gcmax, 0, 17);
>> +	I915_WRITE(gcmax_reg, word);
>> +	gcmax_reg += 4;
>> +
>> +	word = 0;
>> +	gcmax = min_t(u32, GET_BITS(correction_values[count].b32, 8,
>> 17),
>> +				BDW_MAX_GAMMA);
>> +	SET_BITS(word, gcmax, 0, 17);
>> +	I915_WRITE(gcmax_reg, word);
>> +}
>> +
>> +/* Apply unity gamma for gamma reset */
>> +static void bdw_reset_gamma(struct drm_i915_private *dev_priv,
>> +			enum pipe pipe)
>> +{
>> +	u16 count = 0;
>> +	u32 val;
>> +	u32 pal_prec_data = LGC_PALETTE(pipe, 0);
>> +
>> +	DRM_DEBUG_DRIVER("\n");
>> +
>> +	/* Reset the palette for unit gamma */
>> +	while (count < BDW_8BIT_GAMMA_MAX_VALS) {
>> +		/* Red (23:16) Green (15:8) and Blue (7:0) */
>> +		val = (count << 16) | (count << 8) | count;
>> +		I915_WRITE(pal_prec_data, val);
>> +		pal_prec_data += 4;
>> +		count++;
>> +	}
>> +}
>> +
>> +static int bdw_set_gamma(struct drm_device *dev, struct
>> drm_property_blob *blob,
>> +			struct drm_crtc *crtc)
>> +{
>> +	u16 blue_fract, green_fract, red_fract;
>> +	enum pipe pipe;
>> +	int count, num_samples;
>> +	u32 blue, green, red;
>> +	u32 mode, pal_prec_index, pal_prec_data;
>> +	u32 index;
>> +	u32 word = 0;
>> +	struct drm_palette *gamma_data;
>> +	struct drm_crtc_state *state = crtc->state;
>> +	struct drm_i915_private *dev_priv = dev->dev_private;
>> +	struct drm_r32g32b32 *correction_values = NULL;
>> +
>> +	if (WARN_ON(!blob))
>> +		return -EINVAL;
>> +
>> +	gamma_data = (struct drm_palette *)blob->data;
>> +	pipe = to_intel_crtc(crtc)->pipe;
>> +	num_samples = gamma_data->num_samples;
>> +
>> +	pal_prec_index = _PREC_PAL_INDEX(pipe);
>> +	pal_prec_data = _PREC_PAL_DATA(pipe);
>> +
>> +	correction_values = (struct drm_r32g32b32 *)&gamma_data
>> ->lut;
>> +	index = I915_READ(pal_prec_index);
>> +
>> +	switch (num_samples) {
>> +	case GAMMA_DISABLE_VALS:
>> +
>> +		/* Disable Gamma functionality on Pipe */
>> +		DRM_DEBUG_DRIVER("Disabling gamma on Pipe %c\n",
>> +		pipe_name(pipe));
>> +		mode = I915_READ(GAMMA_MODE(pipe));
>> +		if ((mode & GAMMA_MODE_MODE_MASK) ==
>> GAMMA_MODE_MODE_12BIT)
>> +			bdw_reset_gamma(dev_priv, pipe);
>
> Why only call bdw_reset_gamma() if we were in 12-bit mode? What about
> the other modes? What about if somebody has programmed the value
> through the legacy ioctl?
We tested this part, its only required for 12 bit gamma. The legacy 
IOCTL only programs 8bit gamma, which doesnt need this anyways, coz we 
are overwriting legacy palette to gamma = 1, while disabling gamma. If 
we dont do this for 12 bit gamma, screen goes all black.
>
>> +		state->palette_after_ctm_blob = NULL;
>> +		word = GAMMA_MODE_MODE_8BIT;
>> +		break;
>> +
>> +	case BDW_8BIT_GAMMA_MAX_VALS:
>> +
>> +		/* Legacy palette */
>> +		pal_prec_data = LGC_PALETTE(pipe, 0);
>> +		count = 0;
>> +		while (count < BDW_8BIT_GAMMA_MAX_VALS) {
>> +			blue = correction_values[count].b32;
>> +			green = correction_values[count].g32;
>> +			red = correction_values[count].r32;
>> +
>> +			blue_fract = GET_BITS(blue, 16, 8);
>> +			green_fract = GET_BITS(green, 16, 8);
>> +			red_fract = GET_BITS(red, 16, 8);
>
> Shouldn't these round? Rather than always floor.
Why ? This is exact value.
 > Also 1.0 in 8.24
> format will result in you programming zero into the palette. You need
> to do the max clamping again.
Will fix this part.
>
>> +
>> +			/* Blue (7:0) Green (15:8) and Red (23:16)
>> */
>> +			SET_BITS(word, blue_fract, 0, 8);
>> +			SET_BITS(word, green_fract, 8, 8);
>> +			SET_BITS(word, blue_fract, 16, 8);
>> +			I915_WRITE(pal_prec_data, word);
>> +			pal_prec_data += 4;
>> +			count++;
>
> This code also needs to be made to work nicely with other users of
> LGC_PALETTE.
This is anyways working with LGC IOCTL, but will clean it up further.
>
>> +		}
>> +		word = GAMMA_MODE_MODE_8BIT;
>> +		break;
>> +
>> +	case BDW_SPLITGAMMA_MAX_VALS:
>> +
>> +		index |= BDW_INDEX_AUTO_INCREMENT |
>> BDW_INDEX_SPLIT_MODE;
>> +		I915_WRITE(pal_prec_index, index);
>> +		bdw_write_10bit_gamma_precision(dev,
>> correction_values,
>> +			pal_prec_data, BDW_SPLITGAMMA_MAX_VALS);
>> +		word = GAMMA_MODE_MODE_SPLIT;
>> +		break;
>> +
>> +	case BDW_12BIT_GAMMA_MAX_VALS:
>> +
>> +		index |= BDW_INDEX_AUTO_INCREMENT;
>> +		index &= ~BDW_INDEX_SPLIT_MODE;
>> +		I915_WRITE(pal_prec_index, index);
>> +		bdw_write_12bit_gamma_precision(dev,
>> correction_values,
>> +			pal_prec_data, pipe);
>> +		word = GAMMA_MODE_MODE_12BIT;
>> +		break;
>> +
>> +	case BDW_10BIT_GAMMA_MAX_VALS:
>> +		index |= BDW_INDEX_AUTO_INCREMENT;
>> +		index &= ~BDW_INDEX_SPLIT_MODE;
>> +		I915_WRITE(pal_prec_index, index);
>> +		bdw_write_10bit_gamma_precision(dev,
>> correction_values,
>> +			pal_prec_data, BDW_10BIT_GAMMA_MAX_VALS);
>> +		word = GAMMA_MODE_MODE_10BIT;
>> +		break;
>> +
>> +	default:
>> +		DRM_ERROR("Invalid number of samples\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	/* Set gamma mode on pipe control reg */
>> +	mode = I915_READ(GAMMA_MODE(pipe));
>> +	mode &= ~GAMMA_MODE_MODE_MASK;
>> +	I915_WRITE(GAMMA_MODE(pipe), mode | word);
>> +	DRM_DEBUG_DRIVER("Gamma applied on pipe %c\n",
>> +	pipe_name(pipe));
>> +	return 0;
>> +}
>>
>>   static s16 chv_prepare_csc_coeff(s64 csc_value)
>>   {
>> @@ -319,6 +565,8 @@ void intel_color_manager_crtc_commit(struct
>> drm_device *dev,
>>   		/* Gamma correction is platform specific */
>>   		if (IS_CHERRYVIEW(dev))
>>   			ret = chv_set_gamma(dev, blob, crtc);
>> +		else if (IS_BROADWELL(dev) || IS_GEN9(dev))
>> +			ret = bdw_set_gamma(dev, blob, crtc);
>>
>>   		if (ret)
>>   			DRM_ERROR("set Gamma correction failed\n");
>> diff --git a/drivers/gpu/drm/i915/intel_color_manager.h
>> b/drivers/gpu/drm/i915/intel_color_manager.h
>> index 271246a..6c7cb08 100644
>> --- a/drivers/gpu/drm/i915/intel_color_manager.h
>> +++ b/drivers/gpu/drm/i915/intel_color_manager.h
>> @@ -92,3 +92,9 @@
>>
>>   /* Gamma on BDW */
>>   #define BDW_SPLITGAMMA_MAX_VALS                512
>> +#define BDW_8BIT_GAMMA_MAX_VALS		256
>> +#define BDW_10BIT_GAMMA_MAX_VALS		1024
>> +#define BDW_12BIT_GAMMA_MAX_VALS		513
>> +#define BDW_MAX_GAMMA                         ((1 << 24) - 1)
>> +#define BDW_INDEX_AUTO_INCREMENT               (1 << 15)
>> +#define BDW_INDEX_SPLIT_MODE                   (1 << 31)
>
> Rob
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx