Re: [PATCH] drm/i915: Enable Plane Input CSC for YUV to RGB Conversion

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




>-----Original Message-----
>From: Maarten Lankhorst [mailto:maarten.lankhorst@xxxxxxxxxxxxxxx]
>Sent: Wednesday, October 24, 2018 4:18 PM
>To: Shankar, Uma <uma.shankar@xxxxxxxxx>; intel-gfx@xxxxxxxxxxxxxxxxxxxxx
>Cc: Syrjala, Ville <ville.syrjala@xxxxxxxxx>; Lankhorst, Maarten
><maarten.lankhorst@xxxxxxxxx>
>Subject: Re:  [PATCH] drm/i915: Enable Plane Input CSC for YUV to RGB
>Conversion
>
>Op 23-10-18 om 22:11 schreef Uma Shankar:
>> Plane input CSC needs to be enabled to convert frambuffers from YUV to
>> RGB. This is needed for bottom 3 planes on ICL, rest of the planes
>> have hardcoded conversion and taken care by the legacy code.
>>
>> This patch defines the plane input csc registers and co-efficient
>> values for YUV to RGB conversion in BT709 and BT601 formats. It
>> programs the coefficients and enables the plane input csc unit in
>> hardware.
>>
>> Note: This is currently untested and floated to get an early feedback
>> on the design and implementation for this feature. In parallel, I will
>> test this on actual ICL hardware and confirm with planar formats.
>>
>> Signed-off-by: Uma Shankar <uma.shankar@xxxxxxxxx>
>> ---
>>  drivers/gpu/drm/i915/i915_reg.h      | 243
>+++++++++++++++++++++++++++++++++++
>>  drivers/gpu/drm/i915/intel_display.c |  76 ++++++++++-
>>  2 files changed, 313 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h
>> b/drivers/gpu/drm/i915/i915_reg.h index 8bd61f9..f0f6f7c 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -6553,6 +6553,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 */
>>  #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)
>> @@ -6569,6 +6570,248 @@ 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
>> +
>> +#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)
>> +#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))
>Would probably be best to keep those separate.

You mean in a separate patch ? If yes, I can do that and segregate the register macro
definitions in a separate patch.

>> +/*
>> + * These values are direct register values specified in the Bspec,
>> + * for YUV->RGB Full Range conversion matrix (colorspace BT709)  */
>> +#define CSC_BT709_YUV_TO_RGB_RY_GY	0x7C987800
>> +#define CSC_BT709_YUV_TO_RGB_BY		0x0
>> +#define CSC_BT709_YUV_TO_RGB_RU_GU	0x9EF87800
>> +#define CSC_BT709_YUV_TO_RGB_BU		0xABF8
>> +#define CSC_BT709_YUV_TO_RGB_RV_GV	0x7800
>> +#define CSC_BT709_YUV_TO_RGB_BV		0x7ED8
>> +
>> +/*
>> + * These values are direct register values specified in the Bspec,
>> + * for YUV->RGB Full Range conversion matrix (colorspace BT601)  */
>> +#define CSC_BT601_YUV_TO_RGB_RY_GY	0x7AF87800
>> +#define CSC_BT601_YUV_TO_RGB_BY		0x0
>> +#define CSC_BT601_YUV_TO_RGB_RU_GU	0x8B287800
>> +#define CSC_BT601_YUV_TO_RGB_BU		0x9AC0
>> +#define CSC_BT601_YUV_TO_RGB_RV_GV	0x7800
>> +#define CSC_BT601_YUV_TO_RGB_BV		0x7DD8
>> +
>> +/* Preoffset values for YUV to RGB Conversion */
>> +#define PREOFF_YUV_TO_RGB_HI		0x800
>> +#define PREOFF_YUV_TO_RGB_ME		0xF00
>> +#define PREOFF_YUV_TO_RGB_LO		0x800
>>
>>  #define _PLANE_CTL_1_B				0x71180
>>  #define _PLANE_CTL_2_B				0x71280
>Probably move those coefficients to intel_color.c ?

Sure, I will do that.

>> diff --git a/drivers/gpu/drm/i915/intel_display.c
>> b/drivers/gpu/drm/i915/intel_display.c
>> index fc7e3b0..38b41ed 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -3689,12 +3689,66 @@ u32 skl_plane_ctl(const struct intel_crtc_state
>*crtc_state,
>>  	return plane_ctl;
>>  }
>>
>> +void icl_program_input_csc_coeff(const struct intel_crtc_state *crtc_state,
>> +				const struct intel_plane_state *plane_state) {
>> +	struct drm_i915_private *dev_priv =
>> +		to_i915(plane_state->base.plane->dev);
>> +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
>> +	enum pipe pipe = crtc->pipe;
>> +	struct intel_plane *intel_plane =
>> +			to_intel_plane(plane_state->base.plane);
>> +	enum plane_id plane = intel_plane->id;
>> +
>> +	if (plane_state->base.color_encoding == DRM_COLOR_YCBCR_BT709) {
>> +		I915_WRITE(PLANE_INPUT_CSC_RY_GY(pipe, plane),
>> +			   CSC_BT709_YUV_TO_RGB_RY_GY);
>> +		I915_WRITE(PLANE_INPUT_CSC_BY(pipe, plane),
>> +			   CSC_BT709_YUV_TO_RGB_BY);
>> +		I915_WRITE(PLANE_INPUT_CSC_RU_GU(pipe, plane),
>> +			   CSC_BT709_YUV_TO_RGB_RU_GU);
>> +		I915_WRITE(PLANE_INPUT_CSC_BU(pipe, plane),
>> +			   CSC_BT709_YUV_TO_RGB_BU);
>> +		I915_WRITE(PLANE_INPUT_CSC_RV_GV(pipe, plane),
>> +			   CSC_BT709_YUV_TO_RGB_RV_GV);
>> +		I915_WRITE(PLANE_INPUT_CSC_BV(pipe, plane),
>> +			   CSC_BT709_YUV_TO_RGB_BV);
>> +	} else {
>> +		I915_WRITE(PLANE_INPUT_CSC_RY_GY(pipe, plane),
>> +			   CSC_BT601_YUV_TO_RGB_RY_GY);
>> +		I915_WRITE(PLANE_INPUT_CSC_BY(pipe, plane),
>> +			   CSC_BT601_YUV_TO_RGB_BY);
>> +		I915_WRITE(PLANE_INPUT_CSC_RU_GU(pipe, plane),
>> +			   CSC_BT601_YUV_TO_RGB_RU_GU);
>> +		I915_WRITE(PLANE_INPUT_CSC_BU(pipe, plane),
>> +			   CSC_BT601_YUV_TO_RGB_BU);
>> +		I915_WRITE(PLANE_INPUT_CSC_RV_GV(pipe, plane),
>> +			   CSC_BT601_YUV_TO_RGB_RV_GV);
>> +		I915_WRITE(PLANE_INPUT_CSC_BV(pipe, plane),
>> +			   CSC_BT601_YUV_TO_RGB_BV);
>> +	}
>Considering there's going to be BT.601, BT.709 and perhaps newer colorspaces
>with limited vs full range, would it make more sense to generate the values?

These are all floating point coefficient values and driver has restrictions on the
floating math, so would be tough to generate them. Currently only BT709
and BT601 was supported. Later BT2020 co-eficients can be added. Other way can be
to expose this as a property and get these values from userspace (I would not want to
go that path as it will be an extra ABI burden and will require a userspace).

Personally I feel, defining 6 macro value for a new colorspace would be an easier option.

>> +
>> +	I915_WRITE(PLANE_INPUT_CSC_PREOFF_HI(pipe, plane),
>> +		   PREOFF_YUV_TO_RGB_HI);
>> +	I915_WRITE(PLANE_INPUT_CSC_PREOFF_ME(pipe, plane),
>> +		   PREOFF_YUV_TO_RGB_ME);
>> +	I915_WRITE(PLANE_INPUT_CSC_PREOFF_LO(pipe, plane),
>> +		   PREOFF_YUV_TO_RGB_LO);
>> +
>> +	I915_WRITE(PLANE_INPUT_CSC_POSTOFF_HI(pipe, plane), 0x0);
>> +	I915_WRITE(PLANE_INPUT_CSC_POSTOFF_ME(pipe, plane), 0x0);
>> +	I915_WRITE(PLANE_INPUT_CSC_POSTOFF_LO(pipe, plane), 0x0); }
>We already have some support code for CSC matrices in intel_color.c, so I think it
>makes sense to program this from there..

Since for the non HDR planes CSC bits are programmed here, it would be good if
all planes are handled at one place. I will move the function to intel_color.c to keep
all the color related functions in one file.

>>  u32 glk_plane_color_ctl(const struct intel_crtc_state *crtc_state,
>>  			const struct intel_plane_state *plane_state)  {
>>  	struct drm_i915_private *dev_priv =
>>  		to_i915(plane_state->base.plane->dev);
>>  	const struct drm_framebuffer *fb = plane_state->base.fb;
>> +	struct intel_plane *plane = to_intel_plane(plane_state->base.plane);
>> +	enum plane_id plane_id = plane->id;
>> +
>>  	u32 plane_color_ctl = 0;
>>
>>  	if (INTEL_GEN(dev_priv) < 11) {
>> @@ -3705,13 +3759,23 @@ u32 glk_plane_color_ctl(const struct
>intel_crtc_state *crtc_state,
>>  	plane_color_ctl |= glk_plane_color_ctl_alpha(plane_state);
>>
>>  	if (fb->format->is_yuv) {
>> -		if (plane_state->base.color_encoding ==
>DRM_COLOR_YCBCR_BT709)
>> -			plane_color_ctl |=
>PLANE_COLOR_CSC_MODE_YUV709_TO_RGB709;
>> -		else
>> -			plane_color_ctl |=
>PLANE_COLOR_CSC_MODE_YUV601_TO_RGB709;
>> +		if (INTEL_GEN(dev_priv) < 11 || plane_id > 2) {
>We now have icl_is_hdr_plane(), it just landed :)
>> +			if (plane_state->base.color_encoding ==
>> +					DRM_COLOR_YCBCR_BT709)
>> +				plane_color_ctl |=
>> +
>	PLANE_COLOR_CSC_MODE_YUV709_TO_RGB709;
>> +			else
>> +				plane_color_ctl |=
>> +
>	PLANE_COLOR_CSC_MODE_YUV601_TO_RGB709;
>>
>> -		if (plane_state->base.color_range ==
>DRM_COLOR_YCBCR_FULL_RANGE)
>> -			plane_color_ctl |=
>PLANE_COLOR_YUV_RANGE_CORRECTION_DISABLE;
>> +			if (plane_state->base.color_range ==
>> +					DRM_COLOR_YCBCR_FULL_RANGE)
>> +				plane_color_ctl |=
>> +
>	PLANE_COLOR_YUV_RANGE_CORRECTION_DISABLE;
>> +		} else {
>> +			icl_program_input_csc_coeff(crtc_state, plane_state);
>> +			plane_color_ctl |= PLANE_COLOR_INPUT_CSC_ENABLE;
>The coefficients are likely different for full vs limited range, and I don't see that
>handled at the moment. :(

I have supported the FULL Range YUV to RGB conversion in this patch. Will add
limited range along with BT2020 with a later patch. This should unblock all the
current YUV Full Range planar format implementation. Hope this is ok ?

Thanks for the review Maarten. Will send out the updated patch based on your
recommendations.

Regards,
Uma Shankar

>> +		}
>>  	}
>>
>>  	return plane_color_ctl;
>

_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux