[Intel-gfx] [PATCH] drm/i915: Enable Plane Input CSC for YUV to RGB Conversion
Maarten Lankhorst
maarten.lankhorst at linux.intel.com
Wed Oct 24 13:42:34 UTC 2018
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..
More information about the Intel-gfx
mailing list