On Thu, Jun 15, 2017 at 06:52:53PM +0530, Sharma, Shashank wrote:
> Regards
>
> Shashank
>
>
> On 6/9/2017 2:03 AM, ville.syrjala@xxxxxxxxxxxxxxx wrote:
> > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> >
> > Add support for the COLOR_ENCODING plane property which selects
> > the matrix coefficients used for the YCbCr->RGB conversion. Our
> > hardware can generally handle BT.601 and BT.709.
> >
> > CHV pipe B sprites have a fully programmable matrix, so in theory
> > we could handle anything, but it doesn't seem all that useful to
> > expose anything beyond BT.601 and BT.709 at this time.
> >
> > GLK can supposedly do BT.2020, but let's leave enabling that for
> > the future as well.
> >
> > Cc: Jyri Sarha <jsarha@xxxxxx>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/i915_reg.h | 3 ++
> > drivers/gpu/drm/i915/intel_display.c | 19 +++++++++--
> > drivers/gpu/drm/i915/intel_sprite.c | 61 +++++++++++++++++++++++++++---------
> > 3 files changed, 66 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index f4f51e7a3e83..bac3ec378b6e 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -5612,6 +5612,7 @@ enum {
> > #define DVS_PIPE_CSC_ENABLE (1<<24)
> > #define DVS_SOURCE_KEY (1<<22)
> > #define DVS_RGB_ORDER_XBGR (1<<20)
> > +#define DVS_YUV_CSC_FORMAT_BT709 (1<<18)
> > #define DVS_YUV_BYTE_ORDER_MASK (3<<16)
> > #define DVS_YUV_ORDER_YUYV (0<<16)
> > #define DVS_YUV_ORDER_UYVY (1<<16)
> > @@ -5758,6 +5759,7 @@ enum {
> > #define SP_FORMAT_RGBA8888 (0xf<<26)
> > #define SP_ALPHA_PREMULTIPLY (1<<23) /* CHV pipe B */
> > #define SP_SOURCE_KEY (1<<22)
> > +#define SP_YUV_CSC_FORMAT_BT709 (1<<18)
> > #define SP_YUV_BYTE_ORDER_MASK (3<<16)
> > #define SP_YUV_ORDER_YUYV (0<<16)
> > #define SP_YUV_ORDER_UYVY (1<<16)
> > @@ -5876,6 +5878,7 @@ enum {
> > #define PLANE_CTL_KEY_ENABLE_DESTINATION ( 2 << 21)
> > #define PLANE_CTL_ORDER_BGRX (0 << 20)
> > #define PLANE_CTL_ORDER_RGBX (1 << 20)
> > +#define PLANE_CTL_YUV_CSC_FORMAT_BT709 (1 << 18)
> Should we call it PLANE_CTL_CSC_YUV_FORMAT_BT709 (or similar) so that it
> would be clear that source is BT709 format.
> Right now, it feels like CSC(destination) format is BT709.
I just copy pasted it from the IVB sprite plane definition.
I believe the spec calls it:
g4x+: "YUV Format"
ivb+: "YUV to RGB CSC Format"
I guess I could try to line up the definitions to match the spec more
closely.
> > #define PLANE_CTL_YUV422_ORDER_MASK (0x3 << 16)
> > #define PLANE_CTL_YUV422_YUYV ( 0 << 16)
> > #define PLANE_CTL_YUV422_UYVY ( 1 << 16)
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index e2aaaf60a969..810b53b0128c 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -3321,6 +3321,9 @@ u32 skl_plane_ctl(const struct intel_crtc_state *crtc_state,
> > PLANE_CTL_PIPE_GAMMA_ENABLE |
> > PLANE_CTL_PIPE_CSC_ENABLE |
> > PLANE_CTL_PLANE_GAMMA_DISABLE;
> > +
> > + if (plane_state->base.color_encoding == DRM_COLOR_YCBCR_BT709)
> > + plane_ctl |= PLANE_CTL_YUV_CSC_FORMAT_BT709;
> > }
> >
> > plane_ctl |= skl_plane_ctl_format(fb->format->format);
> > @@ -3344,8 +3347,12 @@ u32 glk_color_ctl(const struct intel_plane_state *plane_state)
> > PLANE_COLOR_PIPE_CSC_ENABLE |
> > PLANE_COLOR_PLANE_GAMMA_DISABLE;
> >
> > - if (intel_format_is_yuv(fb->format->format))
> > - color_ctl |= PLANE_COLOR_CSC_MODE_YUV601_TO_RGB709;
> > + if (intel_format_is_yuv(fb->format->format)) {
> > + if (plane_state->base.color_encoding == DRM_COLOR_YCBCR_BT709)
> > + color_ctl |= PLANE_COLOR_CSC_MODE_YUV709_TO_RGB709;
> > + else
> > + color_ctl |= PLANE_COLOR_CSC_MODE_YUV601_TO_RGB709;
> > + }
> >
> > return color_ctl;
> > }
> > @@ -13819,6 +13826,14 @@ intel_primary_plane_create(struct drm_i915_private *dev_priv, enum pipe pipe)
> > DRM_MODE_ROTATE_0,
> > supported_rotations);
> >
> > + if (INTEL_GEN(dev_priv) >= 9)
> I am seeing few if (INTEL_GEN(dev_priv) >= 9) checks above, would it
> make sense to reuse any of those, instead of adding
> a new one ?
I wanted to keep the steps clearly separate. First initialize the plane,
then add the rotation property, and finally add the color properties.
>
> - Shashank
> > + drm_plane_create_color_properties(&primary->base,
> > + BIT(DRM_COLOR_YCBCR_BT601) |
> > + BIT(DRM_COLOR_YCBCR_BT709),
> > + BIT(DRM_COLOR_YCBCR_LIMITED_RANGE),
> > + DRM_COLOR_YCBCR_BT601,
> > + DRM_COLOR_YCBCR_LIMITED_RANGE);
> > +
> > drm_plane_helper_add(&primary->base, &intel_plane_helper_funcs);
> >
> > return primary;
> > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> > index cc07157f2eb6..c21f43d64b00 100644
> > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > @@ -328,30 +328,45 @@ chv_update_csc(const struct intel_plane_state *plane_state)
> > struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> > const struct drm_framebuffer *fb = plane_state->base.fb;
> > enum plane_id plane_id = plane->id;
> > + /*
> > + * |r| | c0 c1 c2 | |cr|
> > + * |g| = | c3 c4 c5 | x |y |
> > + * |b| | c6 c7 c8 | |cb|
> > + *
> > + * Coefficients are s3.12.
> > + *
> > + * Cb and Cr apparently come in as signed already, and
> > + * we always get full range data in on account of CLRC0/1.
> > + */
> > + static const s16 csc_matrix[][9] = {
> > + /* BT.601 full range YCbCr -> full range RGB */
> > + [DRM_COLOR_YCBCR_BT601] = {
> > + 5743, 4096, 0,
> > + -2925, 4096, -1410,
> > + 0, 4096, 7258,
> > + },
> > + /* BT.709 full range YCbCr -> full range RGB */
> > + [DRM_COLOR_YCBCR_BT709] = {
> > + 6450, 4096, 0,
> > + -1917, 4096, -767,
> > + 0, 4096, 7601,
> > + },
> > + };
> > + const s16 *csc = csc_matrix[plane_state->base.color_encoding];
> >
> > /* Seems RGB data bypasses the CSC always */
> > if (!intel_format_is_yuv(fb->format->format))
> > return;
> >
> > - /*
> > - * BT.601 full range YCbCr -> full range RGB
> > - *
> > - * |r| | 5743 4096 0| |cr|
> > - * |g| = |-2925 4096 -1410| x |y |
> > - * |b| | 0 4096 7258| |cb|
> > - *
> > - * Cb and Cr apparently come in as signed already,
> > - * and we get full range data in on account of CLRC0/1
> > - */
> > I915_WRITE_FW(SPCSCYGOFF(plane_id), SPCSC_OOFF(0) | SPCSC_IOFF(0));
> > I915_WRITE_FW(SPCSCCBOFF(plane_id), SPCSC_OOFF(0) | SPCSC_IOFF(0));
> > I915_WRITE_FW(SPCSCCROFF(plane_id), SPCSC_OOFF(0) | SPCSC_IOFF(0));
> >
> > - I915_WRITE_FW(SPCSCC01(plane_id), SPCSC_C1(4096) | SPCSC_C0(5743));
> > - I915_WRITE_FW(SPCSCC23(plane_id), SPCSC_C1(-2925) | SPCSC_C0(0));
> > - I915_WRITE_FW(SPCSCC45(plane_id), SPCSC_C1(-1410) | SPCSC_C0(4096));
> > - I915_WRITE_FW(SPCSCC67(plane_id), SPCSC_C1(4096) | SPCSC_C0(0));
> > - I915_WRITE_FW(SPCSCC8(plane_id), SPCSC_C0(7258));
> > + I915_WRITE_FW(SPCSCC01(plane_id), SPCSC_C1(csc[1]) | SPCSC_C0(csc[0]));
> > + I915_WRITE_FW(SPCSCC23(plane_id), SPCSC_C1(csc[3]) | SPCSC_C0(csc[2]));
> > + I915_WRITE_FW(SPCSCC45(plane_id), SPCSC_C1(csc[5]) | SPCSC_C0(csc[4]));
> > + I915_WRITE_FW(SPCSCC67(plane_id), SPCSC_C1(csc[7]) | SPCSC_C0(csc[6]));
> > + I915_WRITE_FW(SPCSCC8(plane_id), SPCSC_C0(csc[8]));
> >
> > I915_WRITE_FW(SPCSCYGICLAMP(plane_id), SPCSC_IMAX(1023) | SPCSC_IMIN(0));
> > I915_WRITE_FW(SPCSCCBICLAMP(plane_id), SPCSC_IMAX(512) | SPCSC_IMIN(-512));
> > @@ -445,6 +460,9 @@ static u32 vlv_sprite_ctl(const struct intel_crtc_state *crtc_state,
> > return 0;
> > }
> >
> > + if (plane_state->base.color_encoding == DRM_COLOR_YCBCR_BT709)
> > + sprctl |= SP_YUV_CSC_FORMAT_BT709;
> > +
> > if (fb->modifier == I915_FORMAT_MOD_X_TILED)
> > sprctl |= SP_TILED;
> >
> > @@ -578,6 +596,9 @@ static u32 ivb_sprite_ctl(const struct intel_crtc_state *crtc_state,
> > return 0;
> > }
> >
> > + if (plane_state->base.color_encoding == DRM_COLOR_YCBCR_BT709)
> > + sprctl |= SPRITE_YUV_CSC_FORMAT_BT709;
> > +
> > if (fb->modifier == I915_FORMAT_MOD_X_TILED)
> > sprctl |= SPRITE_TILED;
> >
> > @@ -715,6 +736,9 @@ static u32 g4x_sprite_ctl(const struct intel_crtc_state *crtc_state,
> > return 0;
> > }
> >
> > + if (plane_state->base.color_encoding == DRM_COLOR_YCBCR_BT709)
> > + dvscntr |= DVS_YUV_CSC_FORMAT_BT709;
> > +
> > if (fb->modifier == I915_FORMAT_MOD_X_TILED)
> > dvscntr |= DVS_TILED;
> >
> > @@ -1221,6 +1245,13 @@ intel_sprite_plane_create(struct drm_i915_private *dev_priv,
> > DRM_MODE_ROTATE_0,
> > supported_rotations);
> >
> > + drm_plane_create_color_properties(&intel_plane->base,
> > + BIT(DRM_COLOR_YCBCR_BT601) |
> > + BIT(DRM_COLOR_YCBCR_BT709),
> > + BIT(DRM_COLOR_YCBCR_LIMITED_RANGE),
> > + DRM_COLOR_YCBCR_BT601,
> > + DRM_COLOR_YCBCR_LIMITED_RANGE);
> > +
> > drm_plane_helper_add(&intel_plane->base, &intel_plane_helper_funcs);
> >
> > return intel_plane;
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
