Re: [v2 1/2] drm/i915/icl: Define Plane Input CSC Coefficient Registers

["Shankar, Uma" <uma.shankar@intel.com>]

>-----Original Message-----
>From: Roper, Matthew D
>Sent: Thursday, October 25, 2018 2:02 AM
>To: Shankar, Uma <uma.shankar@intel.com>
>Cc: intel-gfx@lists.freedesktop.org; Syrjala, Ville <ville.syrjala@intel.com>;
>Lankhorst, Maarten <maarten.lankhorst@intel.com>
>Subject: Re: [Intel-gfx] [v2 1/2] drm/i915/icl: Define Plane Input CSC Coefficient
>Registers
>
>On Wed, Oct 24, 2018 at 09:48:12PM +0530, Uma Shankar wrote:
>> Defined the plane input csc coefficient registers and macros.
>> 6 registers are used to program a total of 9 coefficients, added
>> macros to define each of them for all the planes supporting the
>> feature on pipes. On ICL, bottom 3 planes have this capability.
>>
>> v2: Segregated the register macro definition as separate patch as per
>> Maarten's suggestion.
>>
>> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_reg.h | 217
>> ++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 217 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h
>> b/drivers/gpu/drm/i915/i915_reg.h index 69eb573..6a363a32 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -6569,6 +6569,7 @@ enum {
>>  #define _PLANE_COLOR_CTL_3_A			0x703CC /* GLK+ */
>>  #define   PLANE_COLOR_PIPE_GAMMA_ENABLE		(1 << 30) /*
>Pre-ICL */
>>  #define   PLANE_COLOR_YUV_RANGE_CORRECTION_DISABLE	(1 << 28)
>> +#define   PLANE_COLOR_INPUT_CSC_ENABLE		(1 << 20) /* Pre-ICL */
>
>Is the "Pre-ICL" comment here a copy/paste error?  The other registers marked
>that way say "This field is deprecated" in the bspec, but that isn't the case for the
>input CSC.

Yes, this should be ICL+. I will update that. Thanks

>>  #define   PLANE_COLOR_PIPE_CSC_ENABLE		(1 << 23) /* Pre-ICL */
>>  #define   PLANE_COLOR_CSC_MODE_BYPASS			(0 << 17)
>>  #define   PLANE_COLOR_CSC_MODE_YUV601_TO_RGB709		(1 <<
>17)
>> @@ -6585,6 +6586,222 @@ enum {
>>  #define _PLANE_NV12_BUF_CFG_1_A		0x70278
>>  #define _PLANE_NV12_BUF_CFG_2_A		0x70378
>>
>> +/* Input CSC Register Definitions */
>> +#define _PLANE_INPUT_CSC_RY_GY_1_A	0x701E0
>> +#define _PLANE_INPUT_CSC_RY_GY_2_A	0x702E0
>> +#define _PLANE_INPUT_CSC_RY_GY_3_A	0x703E0
>
>I don't think we usually add the third address to i915_reg.h when planes/pipes are
>equally spaced since our macros can calculate all the subsequent offsets
>automatically.  Same also applied to other regs farther down.

Yes, there are some PLANE MACROS defined using 3rd plane, but it's really not
required, Will drop the same.

>> +
>> +#define _PLANE_INPUT_CSC_RY_GY_1_B	0x711E0
>> +#define _PLANE_INPUT_CSC_RY_GY_2_B	0x712E0
>> +#define _PLANE_INPUT_CSC_RY_GY_3_B	0x713E0
>> +
>> +#define _PLANE_INPUT_CSC_RY_GY_1(pipe)	\
>> +	_PIPE(pipe, _PLANE_INPUT_CSC_RY_GY_1_A, \
>> +	     _PLANE_INPUT_CSC_RY_GY_1_B)
>> +#define _PLANE_INPUT_CSC_RY_GY_2(pipe)	\
>> +	_PIPE(pipe, _PLANE_INPUT_CSC_RY_GY_2_A, \
>> +	     _PLANE_INPUT_CSC_RY_GY_2_B)
>> +#define PLANE_INPUT_CSC_RY_GY(pipe, plane)	\
>> +	_MMIO_PLANE(plane, _PLANE_INPUT_CSC_RY_GY_1(pipe), \
>> +		    _PLANE_INPUT_CSC_RY_GY_2(pipe))
>> +
>> +#define _PLANE_INPUT_CSC_BY_1_A		0x701E4
>> +#define _PLANE_INPUT_CSC_BY_2_A		0x702E4
>> +#define _PLANE_INPUT_CSC_BY_3_A		0x703E4
>> +
>> +#define _PLANE_INPUT_CSC_BY_1_B		0x711E4
>> +#define _PLANE_INPUT_CSC_BY_2_B		0x712E4
>> +#define _PLANE_INPUT_CSC_BY_3_B		0x713E4
>> +
>> +#define _PLANE_INPUT_CSC_BY_1(pipe)	\
>> +	_PIPE(pipe, _PLANE_INPUT_CSC_BY_1_A, \
>> +	     _PLANE_INPUT_CSC_BY_1_B)
>> +#define _PLANE_INPUT_CSC_BY_2(pipe)	\
>> +	_PIPE(pipe, _PLANE_INPUT_CSC_BY_2_A, \
>> +	     _PLANE_INPUT_CSC_BY_2_B)
>
>Since these are really just the next dword of the same logical register as above,
>maybe it would be simpler to just define these (and the other dwords of
>PLANE_INPUT_CSC_COEFF below) as _RY_GY + offset?

Yes, this can be done. Will modify these macros. 

Thanks Matt for the review comments. Will address and refloat the series.

Regards,
Uma Shankar

>
>Matt
>
>> +#define PLANE_INPUT_CSC_BY(pipe, plane)	\
>> +	_MMIO_PLANE(plane, _PLANE_INPUT_CSC_BY_1(pipe), \
>> +		    _PLANE_INPUT_CSC_BY_2(pipe))
>> +
>> +#define _PLANE_INPUT_CSC_RU_GU_1_A	0x701E8
>> +#define _PLANE_INPUT_CSC_RU_GU_2_A	0x702E8
>> +#define _PLANE_INPUT_CSC_RU_GU_3_A	0x703E8
>> +
>> +#define _PLANE_INPUT_CSC_RU_GU_1_B	0x711E8
>> +#define _PLANE_INPUT_CSC_RU_GU_2_B	0x712E8
>> +#define _PLANE_INPUT_CSC_RU_GU_3_B	0x713E8
>> +
>> +#define _PLANE_INPUT_CSC_RU_GU_1(pipe)	\
>> +	_PIPE(pipe, _PLANE_INPUT_CSC_RU_GU_1_A, \
>> +	     _PLANE_INPUT_CSC_RU_GU_1_B)
>> +#define _PLANE_INPUT_CSC_RU_GU_2(pipe)	\
>> +	_PIPE(pipe, _PLANE_INPUT_CSC_RU_GU_2_A, \
>> +	     _PLANE_INPUT_CSC_RU_GU_2_B)
>> +#define PLANE_INPUT_CSC_RU_GU(pipe, plane)	\
>> +	_MMIO_PLANE(plane, _PLANE_INPUT_CSC_RU_GU_1(pipe), \
>> +		    _PLANE_INPUT_CSC_RU_GU_2(pipe))
>> +
>> +#define _PLANE_INPUT_CSC_BU_1_A		0x701EC
>> +#define _PLANE_INPUT_CSC_BU_2_A		0x702EC
>> +#define _PLANE_INPUT_CSC_BU_3_A		0x703EC
>> +
>> +#define _PLANE_INPUT_CSC_BU_1_B		0x711EC
>> +#define _PLANE_INPUT_CSC_BU_2_B		0x712EC
>> +#define _PLANE_INPUT_CSC_BU_3_B		0x713EC
>> +
>> +#define _PLANE_INPUT_CSC_BU_1(pipe)	\
>> +	_PIPE(pipe, _PLANE_INPUT_CSC_BU_1_A, \
>> +	     _PLANE_INPUT_CSC_BU_1_B)
>> +#define _PLANE_INPUT_CSC_BU_2(pipe)	\
>> +	_PIPE(pipe, _PLANE_INPUT_CSC_BU_2_A, \
>> +	     _PLANE_INPUT_CSC_BU_2_B)
>> +#define PLANE_INPUT_CSC_BU(pipe, plane)	\
>> +	_MMIO_PLANE(plane, _PLANE_INPUT_CSC_BU_1(pipe), \
>> +		    _PLANE_INPUT_CSC_BU_2(pipe))
>> +
>> +#define _PLANE_INPUT_CSC_RV_GV_1_A	0x701F0
>> +#define _PLANE_INPUT_CSC_RV_GV_2_A	0x702F0
>> +#define _PLANE_INPUT_CSC_RV_GV_3_A	0x703F0
>> +
>> +#define _PLANE_INPUT_CSC_RV_GV_1_B	0x711F0
>> +#define _PLANE_INPUT_CSC_RV_GV_2_B	0x712F0
>> +#define _PLANE_INPUT_CSC_RV_GV_3_B	0x713F0
>> +
>> +#define _PLANE_INPUT_CSC_RV_GV_1(pipe)	\
>> +	_PIPE(pipe, _PLANE_INPUT_CSC_RV_GV_1_A, \
>> +	     _PLANE_INPUT_CSC_RV_GV_1_B)
>> +#define _PLANE_INPUT_CSC_RV_GV_2(pipe)	\
>> +	_PIPE(pipe, _PLANE_INPUT_CSC_RV_GV_2_A, \
>> +	     _PLANE_INPUT_CSC_RV_GV_2_B)
>> +#define PLANE_INPUT_CSC_RV_GV(pipe, plane)	\
>> +	_MMIO_PLANE(plane, _PLANE_INPUT_CSC_RV_GV_1(pipe), \
>> +		    _PLANE_INPUT_CSC_RV_GV_2(pipe))
>> +
>> +#define _PLANE_INPUT_CSC_BV_1_A		0x701F4
>> +#define _PLANE_INPUT_CSC_BV_2_A		0x702F4
>> +#define _PLANE_INPUT_CSC_BV_3_A		0x703F4
>> +
>> +#define _PLANE_INPUT_CSC_BV_1_B		0x711F4
>> +#define _PLANE_INPUT_CSC_BV_2_B		0x712F4
>> +#define _PLANE_INPUT_CSC_BV_3_B		0x713F4
>> +
>> +#define _PLANE_INPUT_CSC_BV_1(pipe)	\
>> +	_PIPE(pipe, _PLANE_INPUT_CSC_BV_1_A, \
>> +	     _PLANE_INPUT_CSC_BV_1_B)
>> +#define _PLANE_INPUT_CSC_BV_2(pipe)	\
>> +	_PIPE(pipe, _PLANE_INPUT_CSC_BV_2_A, \
>> +	     _PLANE_INPUT_CSC_BV_2_B)
>> +#define PLANE_INPUT_CSC_BV(pipe, plane)	\
>> +	_MMIO_PLANE(plane, _PLANE_INPUT_CSC_BV_1(pipe), \
>> +		    _PLANE_INPUT_CSC_BV_2(pipe))
>> +
>> +#define _PLANE_INPUT_CSC_PREOFF_HI_1_A		0x701F8
>> +#define _PLANE_INPUT_CSC_PREOFF_HI_2_A		0x702F8
>> +#define _PLANE_INPUT_CSC_PREOFF_HI_3_A		0x703F8
>> +
>> +#define _PLANE_INPUT_CSC_PREOFF_HI_1_B		0x711F8
>> +#define _PLANE_INPUT_CSC_PREOFF_HI_2_B		0x712F8
>> +#define _PLANE_INPUT_CSC_PREOFF_HI_3_B		0x713F8
>> +
>> +#define _PLANE_INPUT_CSC_PREOFF_HI_1(pipe)	\
>> +	_PIPE(pipe, _PLANE_INPUT_CSC_PREOFF_HI_1_A, \
>> +	     _PLANE_INPUT_CSC_PREOFF_HI_1_B)
>> +#define _PLANE_INPUT_CSC_PREOFF_HI_2(pipe)	\
>> +	_PIPE(pipe, _PLANE_INPUT_CSC_PREOFF_HI_2_A, \
>> +	     _PLANE_INPUT_CSC_PREOFF_HI_2_B)
>> +#define PLANE_INPUT_CSC_PREOFF_HI(pipe, plane)	\
>> +	_MMIO_PLANE(plane, _PLANE_INPUT_CSC_PREOFF_HI_1(pipe), \
>> +		    _PLANE_INPUT_CSC_PREOFF_HI_2(pipe))
>> +
>> +#define _PLANE_INPUT_CSC_PREOFF_ME_1_A		0x701FC
>> +#define _PLANE_INPUT_CSC_PREOFF_ME_2_A		0x702FC
>> +#define _PLANE_INPUT_CSC_PREOFF_ME_3_A		0x703FC
>> +
>> +#define _PLANE_INPUT_CSC_PREOFF_ME_1_B		0x711FC
>> +#define _PLANE_INPUT_CSC_PREOFF_ME_2_B		0x712FC
>> +#define _PLANE_INPUT_CSC_PREOFF_ME_3_B		0x713FC
>> +
>> +#define _PLANE_INPUT_CSC_PREOFF_ME_1(pipe)	\
>> +	_PIPE(pipe, _PLANE_INPUT_CSC_PREOFF_ME_1_A, \
>> +	     _PLANE_INPUT_CSC_PREOFF_ME_1_B)
>> +#define _PLANE_INPUT_CSC_PREOFF_ME_2(pipe)	\
>> +	_PIPE(pipe, _PLANE_INPUT_CSC_PREOFF_ME_2_A, \
>> +	     _PLANE_INPUT_CSC_PREOFF_ME_2_B)
>> +#define PLANE_INPUT_CSC_PREOFF_ME(pipe, plane)	\
>> +	_MMIO_PLANE(plane, _PLANE_INPUT_CSC_PREOFF_ME_1(pipe), \
>> +		    _PLANE_INPUT_CSC_PREOFF_ME_2(pipe))
>> +
>> +#define _PLANE_INPUT_CSC_PREOFF_LO_1_A		0x70200
>> +#define _PLANE_INPUT_CSC_PREOFF_LO_2_A		0x70300
>> +#define _PLANE_INPUT_CSC_PREOFF_LO_3_A		0x70400
>> +
>> +#define _PLANE_INPUT_CSC_PREOFF_LO_1_B		0x71200
>> +#define _PLANE_INPUT_CSC_PREOFF_LO_2_B		0x71300
>> +#define _PLANE_INPUT_CSC_PREOFF_LO_3_B		0x71400
>> +
>> +#define _PLANE_INPUT_CSC_PREOFF_LO_1(pipe)	\
>> +	_PIPE(pipe, _PLANE_INPUT_CSC_PREOFF_LO_1_A, \
>> +	     _PLANE_INPUT_CSC_PREOFF_LO_1_B)
>> +#define _PLANE_INPUT_CSC_PREOFF_LO_2(pipe)	\
>> +	_PIPE(pipe, _PLANE_INPUT_CSC_PREOFF_LO_2_A, \
>> +	     _PLANE_INPUT_CSC_PREOFF_LO_2_B)
>> +#define PLANE_INPUT_CSC_PREOFF_LO(pipe, plane)	\
>> +	_MMIO_PLANE(plane, _PLANE_INPUT_CSC_PREOFF_LO_1(pipe), \
>> +		    _PLANE_INPUT_CSC_PREOFF_LO_2(pipe))
>> +
>> +#define _PLANE_INPUT_CSC_POSTOFF_HI_1_A		0x70204
>> +#define _PLANE_INPUT_CSC_POSTOFF_HI_2_A		0x70304
>> +#define _PLANE_INPUT_CSC_POSTOFF_HI_3_A		0x70404
>> +
>> +#define _PLANE_INPUT_CSC_POSTOFF_HI_1_B		0x71204
>> +#define _PLANE_INPUT_CSC_POSTOFF_HI_2_B		0x71304
>> +#define _PLANE_INPUT_CSC_POSTOFF_HI_3_B		0x71404
>> +
>> +#define _PLANE_INPUT_CSC_POSTOFF_HI_1(pipe)	\
>> +	_PIPE(pipe, _PLANE_INPUT_CSC_POSTOFF_HI_1_A, \
>> +	     _PLANE_INPUT_CSC_POSTOFF_HI_1_B)
>> +#define _PLANE_INPUT_CSC_POSTOFF_HI_2(pipe)	\
>> +	_PIPE(pipe, _PLANE_INPUT_CSC_POSTOFF_HI_2_A, \
>> +	     _PLANE_INPUT_CSC_POSTOFF_HI_2_B)
>> +#define PLANE_INPUT_CSC_POSTOFF_HI(pipe, plane)	\
>> +	_MMIO_PLANE(plane, _PLANE_INPUT_CSC_POSTOFF_HI_1(pipe), \
>> +		    _PLANE_INPUT_CSC_POSTOFF_HI_2(pipe))
>> +
>> +#define _PLANE_INPUT_CSC_POSTOFF_ME_1_A		0x70208
>> +#define _PLANE_INPUT_CSC_POSTOFF_ME_2_A		0x70308
>> +#define _PLANE_INPUT_CSC_POSTOFF_ME_3_A		0x70408
>> +
>> +#define _PLANE_INPUT_CSC_POSTOFF_ME_1_B		0x71208
>> +#define _PLANE_INPUT_CSC_POSTOFF_ME_2_B		0x71308
>> +#define _PLANE_INPUT_CSC_POSTOFF_ME_3_B		0x71408
>> +
>> +#define _PLANE_INPUT_CSC_POSTOFF_ME_1(pipe)	\
>> +	_PIPE(pipe, _PLANE_INPUT_CSC_POSTOFF_ME_1_A, \
>> +	     _PLANE_INPUT_CSC_POSTOFF_ME_1_B)
>> +#define _PLANE_INPUT_CSC_POSTOFF_ME_2(pipe)	\
>> +	_PIPE(pipe, _PLANE_INPUT_CSC_POSTOFF_ME_2_A, \
>> +	     _PLANE_INPUT_CSC_POSTOFF_ME_2_B)
>> +#define PLANE_INPUT_CSC_POSTOFF_ME(pipe, plane)	\
>> +	_MMIO_PLANE(plane, _PLANE_INPUT_CSC_POSTOFF_ME_1(pipe), \
>> +		    _PLANE_INPUT_CSC_POSTOFF_ME_2(pipe))
>> +
>> +#define _PLANE_INPUT_CSC_POSTOFF_LO_1_A		0x7020C
>> +#define _PLANE_INPUT_CSC_POSTOFF_LO_2_A		0x7030C
>> +#define _PLANE_INPUT_CSC_POSTOFF_LO_3_A		0x7040C
>> +
>> +#define _PLANE_INPUT_CSC_POSTOFF_LO_1_B		0x7120C
>> +#define _PLANE_INPUT_CSC_POSTOFF_LO_2_B		0x7130C
>> +#define _PLANE_INPUT_CSC_POSTOFF_LO_3_B		0x7140C
>> +
>> +#define _PLANE_INPUT_CSC_POSTOFF_LO_1(pipe)	\
>> +	_PIPE(pipe, _PLANE_INPUT_CSC_POSTOFF_LO_1_A, \
>> +	     _PLANE_INPUT_CSC_POSTOFF_LO_1_B)
>> +#define _PLANE_INPUT_CSC_POSTOFF_LO_2(pipe)	\
>> +	_PIPE(pipe, _PLANE_INPUT_CSC_POSTOFF_LO_2_A, \
>> +	     _PLANE_INPUT_CSC_POSTOFF_LO_2_B)
>> +#define PLANE_INPUT_CSC_POSTOFF_LO(pipe, plane)	\
>> +	_MMIO_PLANE(plane, _PLANE_INPUT_CSC_POSTOFF_LO_1(pipe), \
>> +		    _PLANE_INPUT_CSC_POSTOFF_LO_2(pipe))
>>
>>  #define _PLANE_CTL_1_B				0x71180
>>  #define _PLANE_CTL_2_B				0x71280
>> --
>> 1.9.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>--
>Matt Roper
>Graphics Software Engineer
>IoTG Platform Enabling & Development
>Intel Corporation
>(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx