[Intel-gfx] [PATCH v2 2/4] drm/i915/icl: Add register definitions for Multi Segmented gamma

[Sharma, Shashank]

On 5/3/2019 8:35 PM, Ville Syrjälä wrote:
> On Tue, Apr 30, 2019 at 08:51:06PM +0530, Shashank Sharma wrote:
>> From: Uma Shankar <uma.shankar at intel.com>
>>
>> Add macros to define multi segmented gamma registers
>>
>> V2: Addressed Ville's comments:
>>      	Add gen-lable before bit definition
>>      Addressed Jani's comment
>> 	- Use REG_GENMASK() and REG_BIT()
>>
>> Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
>> Cc: Jani Nikula <jani.nikula at linux.intel.com>
>> Cc: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
>> Signed-off-by: Uma Shankar <uma.shankar at intel.com>
>> Signed-off-by: Shashank Sharma <shashank.sharma at intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_reg.h | 19 +++++++++++++++++++
>>   1 file changed, 19 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> index 6f0a0866c802..7d10b8d00d64 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -7198,7 +7198,10 @@ enum {
>>   #define  GAMMA_MODE_MODE_8BIT	(0 << 0)
>>   #define  GAMMA_MODE_MODE_10BIT	(1 << 0)
>>   #define  GAMMA_MODE_MODE_12BIT	(2 << 0)
>> +/* ivb-bdw */
>>   #define  GAMMA_MODE_MODE_SPLIT	(3 << 0)
>> +/* icl + */
>> +#define  GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED	(3 << 0)
> I would put the comments at the end of the line
> #define ... /* ivb-bdw */
>
>>   
>>   /* DMC/CSR */
>>   #define CSR_PROGRAM(i)		_MMIO(0x80000 + (i) * 4)
>> @@ -10145,6 +10148,22 @@ enum skl_power_gate {
>>   #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)
>>   
>> +/* Add registers for Gen11 Multi Segmented Gamma Mode */
> Weird comment. 's/Add registers for //' might make it somewhat useful.
> And no point in capitalizing every word. This isn't a movie title/etc.
>
> With those sorted this is
> Reviewed-by: Ville Syrjälä <ville.syrjala at linux.intel.com>

Thanks for the review, will do the changes as suggested.

- Shashank

>> +#define _PAL_PREC_MULTI_SEG_INDEX_A	0x4A408
>> +#define _PAL_PREC_MULTI_SEG_INDEX_B	0x4AC08
>> +#define  PAL_PREC_MULTI_SEGMENT_AUTO_INCREMENT		REG_BIT(15)
>> +#define  PAL_PREC_MULTI_SEGMENT_INDEX_VALUE_MASK	REG_GENMASK(4, 0)
>> +
>> +#define _PAL_PREC_MULTI_SEG_DATA_A	0x4A40C
>> +#define _PAL_PREC_MULTI_SEG_DATA_B	0x4AC0C
>> +
>> +#define PREC_PAL_MULTI_SEG_INDEX(pipe)	_MMIO_PIPE(pipe, \
>> +					_PAL_PREC_MULTI_SEG_INDEX_A, \
>> +					_PAL_PREC_MULTI_SEG_INDEX_B)
>> +#define PREC_PAL_MULTI_SEG_DATA(pipe)	_MMIO_PIPE(pipe, \
>> +					_PAL_PREC_MULTI_SEG_DATA_A, \
>> +					_PAL_PREC_MULTI_SEG_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)
>> -- 
>> 2.17.1