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

Shankar, Uma uma.shankar at intel.com
Wed Oct 24 15:55:42 UTC 2018



>-----Original Message-----
>From: Ville Syrjälä [mailto:ville.syrjala at linux.intel.com]
>Sent: Wednesday, October 24, 2018 8:10 PM
>To: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
>Cc: Shankar, Uma <uma.shankar at intel.com>; intel-gfx at lists.freedesktop.org;
>Syrjala, Ville <ville.syrjala at intel.com>; Lankhorst, Maarten
><maarten.lankhorst at intel.com>
>Subject: Re: [Intel-gfx] [PATCH] drm/i915: Enable Plane Input CSC for YUV to RGB
>Conversion
>
>On Wed, Oct 24, 2018 at 03:42:34PM +0200, Maarten Lankhorst wrote:
>> Op 24-10-18 om 15:12 schreef Shankar, Uma:
>> >
>> >> -----Original Message-----
>> >> From: Maarten Lankhorst [mailto:maarten.lankhorst at linux.intel.com]
>> >> Sent: Wednesday, October 24, 2018 5:37 PM
>> >> To: Shankar, Uma <uma.shankar at intel.com>;
>> >> intel-gfx at lists.freedesktop.org
>> >> Cc: Syrjala, Ville <ville.syrjala at intel.com>; Lankhorst, Maarten
>> >> <maarten.lankhorst at intel.com>
>> >> Subject: Re: [Intel-gfx] [PATCH] drm/i915: Enable Plane Input CSC
>> >> for YUV to RGB Conversion
>> >>
>> >> Op 24-10-18 om 13:19 schreef Shankar, Uma:
>> >>>> -----Original Message-----
>> >>>> From: Maarten Lankhorst
>> >>>> [mailto:maarten.lankhorst at linux.intel.com]
>> >>>> Sent: Wednesday, October 24, 2018 4:18 PM
>> >>>> To: Shankar, Uma <uma.shankar at intel.com>;
>> >>>> intel-gfx at lists.freedesktop.org
>> >>>> Cc: Syrjala, Ville <ville.syrjala at intel.com>; Lankhorst, Maarten
>> >>>> <maarten.lankhorst at intel.com>
>> >>>> Subject: Re: [Intel-gfx] [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 at intel.com>
>> >>>>> ---
>> >>>>>  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?
>> > All the planar formats will be up sampled before conversion. So
>> > standard YUV to RGB conversion matrix should work even for P01X and Y21X
>formats.
>> Not opposed to hardcoding then, but would be nice to have those values in nice
>static const u16[3][3] array instead of a bunch of defines..
>
>Just like chv_update_csc() already does it.

Sure, got it. Will update the patch accordingly.

>I'd actually like to see a tool in igt that would be capable of calculating the
>coefficients for each hardware generation with a different matrix. We have
>igt_color_encoding so it should be mostly a matter of converting the resulting
>matrix to the correct fixed point (or whatever) precision.

I will try to create something on these lines. Atleast a mechanism to get
the values which matches to bspec register expectations.

Thanks & Regards,
Uma Shankar

>--
>Ville Syrjälä
>Intel


More information about the Intel-gfx mailing list