>-----Original Message-----
>From: Roper, Matthew D
>Sent: Tuesday, October 30, 2018 4:59 AM
>To: Shankar, Uma <uma.shankar@xxxxxxxxx>
>Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Syrjala, Ville <ville.syrjala@xxxxxxxxx>;
>Lankhorst, Maarten <maarten.lankhorst@xxxxxxxxx>
>Subject: Re: [v5 2/2] drm/i915/icl: Enable Plane Input CSC for YUV to
>RGB Conversion
>
>On Fri, Oct 26, 2018 at 03:31:57PM +0530, Uma Shankar wrote:
>> 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 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.
>>
>> v2: Addressed Maarten's and Ville's review comments and added the
>> coefficients in a 2D array instead of independent Macros.
>>
>> v3: Added individual coefficient matrix (9 values) instead of 6
>> register values as per Maarten's comment. Also addresed a shift issue
>> with B channel coefficient.
>>
>> v4: Added support for Limited Range Color Handling
>>
>> v5: Fixed Matt and Maarten's review comments.
>>
>> Signed-off-by: Uma Shankar <uma.shankar@xxxxxxxxx>
>> ---
>> drivers/gpu/drm/i915/intel_color.c | 79
>++++++++++++++++++++++++++++++++++++
>> drivers/gpu/drm/i915/intel_display.c | 23 ++++++++---
>> drivers/gpu/drm/i915/intel_drv.h | 2 +
>> 3 files changed, 98 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_color.c
>> b/drivers/gpu/drm/i915/intel_color.c
>> index 5127da2..681cd13 100644
>> --- a/drivers/gpu/drm/i915/intel_color.c
>> +++ b/drivers/gpu/drm/i915/intel_color.c
>> @@ -57,6 +57,15 @@
>> #define CSC_RGB_TO_YUV_RV_GV 0xbce89ad8 #define
>CSC_RGB_TO_YUV_BV
>> 0x1e080000
>>
>> +#define ROFF(x) (((x) & 0xffff) << 16)
>> +#define GOFF(x) (((x) & 0xffff) << 0)
>> +#define BOFF(x) (((x) & 0xffff) << 16)
>> +
>> +/* Preoffset values for YUV to RGB Conversion */
>> +#define PREOFF_YUV_TO_RGB_HI 0x1800
>> +#define PREOFF_YUV_TO_RGB_ME 0x1F00
>> +#define PREOFF_YUV_TO_RGB_LO 0x1800
>> +
>> /*
>> * Extract the CSC coefficient from a CTM coefficient (in U32.32 fixed point
>> * format). This macro takes the coefficient we want transformed and
>> the @@ -643,6 +652,76 @@ int intel_color_check(struct drm_crtc *crtc,
>> return -EINVAL;
>> }
>>
>> +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;
>> +
>> + static const u16 input_csc_matrix[][9] = {
>
>Can you add comments to these indicating the human-readable values they
>translate to?
Sure Matt, will add that.
>> + /* BT.601 full range YCbCr -> full range RGB */
>> + [DRM_COLOR_YCBCR_BT601] = {
>> + 0x7AF8, 0x7800, 0x0,
>> + 0x8B28, 0x7800, 0x9AC0,
>> + 0x0, 0x7800, 0x7DD8,
>> + },
>> + /* BT.709 full range YCbCr -> full range RGB */
>> + [DRM_COLOR_YCBCR_BT709] = {
>> + 0x7C98, 0x7800, 0x0,
>> + 0x9EF8, 0x7800, 0xABF8,
>> + 0x0, 0x7800, 0x7ED8,
>> + },
>> + };
>> +
>> + /* Matrix for Limited Range to Full Range Conversion */
>> + static const u16 input_csc_matrix_lr[][9] = {
>> + /* BT.601 Limted range YCbCr -> full range RGB */
>> + [DRM_COLOR_YCBCR_BT601] = {
>> + 0x7CC8, 0x7950, 0x0,
>> + 0x8CB8, 0x7918, 0x9C40,
>> + 0x0, 0x7918, 0x7FC8,
>
>Are these obtained by scaling the first row (Y-based) by 256/219 and the other
>two rows (Cb and Cr) by 256/224? If so, it looks like you've always rounded
>down, whereas in some cases rounding up gives you a closer value (and matches
>how the bspec seems to have chosen the full range encodings for their example).
Yes, this is how it's done. But the only reason of delta is that you have taken 8 bit max
value as 256, but I am taking it as 255. I feel it should be (1 << bpc ie 8) - 1. If you put that,
the values which you got will match to what I have here. Please let me know if that's ok.
And thanks for detailed review and useful pointers.
Regards,
Uma Shankar
> [ 0x7CD0, 0x7958, 0x0 ]
> [ 0x8CC0, 0x7928, 0x9C48 ]
> [ 0x0, 0x7928, 0x7FD8 ]
>
>Our encodings of the 1.0 value on the second two rows seems to deviate slightly
>more for some reason; not sure why that is.
>
>For completeness, here's how I came up with 0x7928:
>
> 1 * 256/224 = 1.142857143
> Sign bit = 0
> Exponent bits = 0b111
> Mantissa bits = round(1.142857143 << 8)
> = round(292.571428571)
> = 293
> = 0b100100101
> Reserved bits = 0b000
>
> Result = 0111 1001 0010 1000
> = 0x7928
>
>If you did floor() instead of round() for the mantissa, you'd get 292, which would
>translate to 0x7920 instead.
>
>> + },
>> + /* BT.709 Limited range YCbCr -> full range RGB */
>> + [DRM_COLOR_YCBCR_BT709] = {
>> + 0x7EA8, 0x7950, 0x0,
>> + 0x8888, 0x7918, 0xADA8,
>> + 0x0, 0x7918, 0x6870,
>
>For these I get
>
> [ 0x7EB8, 0x7958, 0 ]
> [ 0x8890, 0x7928, 0xADB0 ]
> [ 0x0, 0x7928, 0x6878 ]
>
>So all the numbers are still pretty close to what you have.
>
>
>Matt
>
>> + },
>> + };
>> + const u16 *csc;
>> +
>> + if (plane_state->base.color_range ==
>DRM_COLOR_YCBCR_FULL_RANGE)
>> + csc = input_csc_matrix[plane_state->base.color_encoding];
>> + else
>> + csc = input_csc_matrix_lr[plane_state->base.color_encoding];
>> +
>> + I915_WRITE(PLANE_INPUT_CSC_COEFF(pipe, plane, 0), ROFF(csc[0]) |
>> + GOFF(csc[1]));
>> + I915_WRITE(PLANE_INPUT_CSC_COEFF(pipe, plane, 1), BOFF(csc[2]));
>> + I915_WRITE(PLANE_INPUT_CSC_COEFF(pipe, plane, 2), ROFF(csc[3]) |
>> + GOFF(csc[4]));
>> + I915_WRITE(PLANE_INPUT_CSC_COEFF(pipe, plane, 3), BOFF(csc[5]));
>> + I915_WRITE(PLANE_INPUT_CSC_COEFF(pipe, plane, 4), ROFF(csc[6]) |
>> + GOFF(csc[7]));
>> + I915_WRITE(PLANE_INPUT_CSC_COEFF(pipe, plane, 5), BOFF(csc[8]));
>> +
>> + I915_WRITE(PLANE_INPUT_CSC_PREOFF(pipe, plane, 0),
>> + PREOFF_YUV_TO_RGB_HI);
>> + I915_WRITE(PLANE_INPUT_CSC_PREOFF(pipe, plane, 1),
>> + PREOFF_YUV_TO_RGB_ME);
>> + I915_WRITE(PLANE_INPUT_CSC_PREOFF(pipe, plane, 2),
>> + PREOFF_YUV_TO_RGB_LO);
>> +
>> + I915_WRITE(PLANE_INPUT_CSC_POSTOFF(pipe, plane, 0), 0x0);
>> + I915_WRITE(PLANE_INPUT_CSC_POSTOFF(pipe, plane, 1), 0x0);
>> + I915_WRITE(PLANE_INPUT_CSC_POSTOFF(pipe, plane, 2), 0x0); }
>> +
>> void intel_color_init(struct drm_crtc *crtc) {
>> struct drm_i915_private *dev_priv = to_i915(crtc->dev); diff --git
>> a/drivers/gpu/drm/i915/intel_display.c
>> b/drivers/gpu/drm/i915/intel_display.c
>> index fe045ab..d16a064 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -3666,6 +3666,7 @@ u32 glk_plane_color_ctl(const struct intel_crtc_state
>*crtc_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);
>> u32 plane_color_ctl = 0;
>>
>> if (INTEL_GEN(dev_priv) < 11) {
>> @@ -3676,13 +3677,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 (!icl_is_hdr_plane(plane)) {
>> + 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;
>> + }
>> }
>>
>> return plane_color_ctl;
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h
>> b/drivers/gpu/drm/i915/intel_drv.h
>> index db24308..bd9e946 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -2285,6 +2285,8 @@ int intel_plane_atomic_check_with_state(const
>> struct intel_crtc_state *old_crtc_ int intel_color_check(struct
>> drm_crtc *crtc, struct drm_crtc_state *state); void
>> intel_color_set_csc(struct drm_crtc_state *crtc_state); void
>> intel_color_load_luts(struct drm_crtc_state *crtc_state);
>> +void icl_program_input_csc_coeff(const struct intel_crtc_state *crtc_state,
>> + const struct intel_plane_state *plane_state);
>>
>> /* intel_lspcon.c */
>> bool lspcon_init(struct intel_digital_port *intel_dig_port);
>> --
>> 1.9.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
>> 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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
