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]

 



Op 24-10-18 om 13:19 schreef Shankar, Uma:
>
>> -----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.
This would work for NV12, but what about P01X or the Y21X formats? Do they need slightly different coefficients for each?

~Maarten
_______________________________________________
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