On Thu, Oct 30, 2014 at 1:30 AM, Ville Syrjälä
<ville.syrjala@xxxxxxxxxxxxxxx> wrote:
> On Wed, Oct 29, 2014 at 01:47:35PM -0700, Rodrigo Vivi wrote:
>> First of all thanks for the spec
>>
>> On Mon, Oct 20, 2014 at 9:47 AM, <ville.syrjala@xxxxxxxxxxxxxxx> wrote:
>> > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>> >
>> > CHV has a programmable CSC unit on the pipe B sprites. Program the unit
>> > appropriately for BT.601 limited range YCbCr to full range RGB color
>> > conversion. This matches the programming we currently do for sprites
>> > on the other pipes and on other platforms.
>> >
>> > It seems the CSC only works when the input data is YCbCr. For RGB
>> > pixel formats it doesn't matter what we program into the CSC registers.
>> > Doesn't make much sense to me especially since the register names give
>> > the impression that RGB input data would also work. But that's how
>> > it behaves here.
>> >
>> > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>> > ---
>> > drivers/gpu/drm/i915/i915_reg.h | 33 +++++++++++++++++
>> > drivers/gpu/drm/i915/intel_sprite.c | 70 +++++++++++++++++++++++++++++--------
>> > 2 files changed, 89 insertions(+), 14 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> > index 9dee063..05e7fd3 100644
>> > --- a/drivers/gpu/drm/i915/i915_reg.h
>> > +++ b/drivers/gpu/drm/i915/i915_reg.h
>> > @@ -4584,6 +4584,39 @@ enum punit_power_well {
>> > #define SPCONSTALPHA(pipe, plane) _PIPE(pipe * 2 + plane, _SPACONSTALPHA, _SPBCONSTALPHA)
>> > #define SPGAMC(pipe, plane) _PIPE(pipe * 2 + plane, _SPAGAMC, _SPBGAMC)
>> >
>> > +/*
>> > + * CHV pipe B sprite CSC
>> > + *
>> > + * |cr| |c0 c1 c2| |cr + cr_ioff| |cr_ooff|
>> > + * |yg| = |c3 c4 c5| x |yg + yg_ioff| + |yg_ooff|
>> > + * |cb| |c6 c7 c8| |cb + cr_ioff| |cb_ooff|
>> > + */
>> > +#define SPCSCYGOFF(sprite) (VLV_DISPLAY_BASE + 0x6d900 + (sprite) * 0x1000)
>> > +#define SPCSCCBOFF(sprite) (VLV_DISPLAY_BASE + 0x6d904 + (sprite) * 0x1000)
>> > +#define SPCSCCROFF(sprite) (VLV_DISPLAY_BASE + 0x6d908 + (sprite) * 0x1000)
>> > +#define SPCSC_OOFF(x) (((x) & 0x7ff) << 16) /* s11 */
>> > +#define SPCSC_IOFF(x) (((x) & 0x7ff) << 0) /* s11 */
>> > +
>> > +#define SPCSCC01(sprite) (VLV_DISPLAY_BASE + 0x6d90c + (sprite) * 0x1000)
>> > +#define SPCSCC23(sprite) (VLV_DISPLAY_BASE + 0x6d910 + (sprite) * 0x1000)
>> > +#define SPCSCC45(sprite) (VLV_DISPLAY_BASE + 0x6d914 + (sprite) * 0x1000)
>> > +#define SPCSCC67(sprite) (VLV_DISPLAY_BASE + 0x6d918 + (sprite) * 0x1000)
>> > +#define SPCSCC8(sprite) (VLV_DISPLAY_BASE + 0x6d91c + (sprite) * 0x1000)
>> > +#define SPCSC_C1(x) (((x) & 0x7fff) << 16) /* s3.12 */
>> > +#define SPCSC_C0(x) (((x) & 0x7fff) << 0) /* s3.12 */
>> > +
>> > +#define SPCSCYGICLAMP(sprite) (VLV_DISPLAY_BASE + 0x6d920 + (sprite) * 0x1000)
>> > +#define SPCSCCBICLAMP(sprite) (VLV_DISPLAY_BASE + 0x6d924 + (sprite) * 0x1000)
>> > +#define SPCSCCRICLAMP(sprite) (VLV_DISPLAY_BASE + 0x6d928 + (sprite) * 0x1000)
>> > +#define SPCSC_IMAX(x) (((x) & 0x7ff) << 16) /* s11 */
>> > +#define SPCSC_IMIN(x) (((x) & 0x7ff) << 0) /* s11 */
>> > +
>> > +#define SPCSCYGOCLAMP(sprite) (VLV_DISPLAY_BASE + 0x6d92c + (sprite) * 0x1000)
>> > +#define SPCSCCBOCLAMP(sprite) (VLV_DISPLAY_BASE + 0x6d930 + (sprite) * 0x1000)
>> > +#define SPCSCCROCLAMP(sprite) (VLV_DISPLAY_BASE + 0x6d934 + (sprite) * 0x1000)
>> > +#define SPCSC_OMAX(x) ((x) << 16) /* u10 */
>> > +#define SPCSC_OMIN(x) ((x) << 0) /* u10 */
>> > +
>> > /* Skylake plane registers */
>> >
>> > #define _PLANE_CTL_1_A 0x70180
>> > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
>> > index a452819..fb6ae5f 100644
>> > --- a/drivers/gpu/drm/i915/intel_sprite.c
>> > +++ b/drivers/gpu/drm/i915/intel_sprite.c
>> > @@ -37,6 +37,20 @@
>> > #include <drm/i915_drm.h>
>> > #include "i915_drv.h"
>> >
>> > +static bool
>> > +format_is_yuv(uint32_t format)
>> > +{
>> > + switch (format) {
>> > + case DRM_FORMAT_YUYV:
>> > + case DRM_FORMAT_UYVY:
>> > + case DRM_FORMAT_VYUY:
>> > + case DRM_FORMAT_YVYU:
>> > + return true;
>> > + default:
>> > + return false;
>> > + }
>> > +}
>> > +
>> > static int usecs_to_scanlines(const struct drm_display_mode *mode, int usecs)
>> > {
>> > /* paranoia */
>> > @@ -320,6 +334,45 @@ skl_get_colorkey(struct drm_plane *drm_plane,
>> > }
>> >
>> > static void
>> > +chv_update_csc(struct intel_plane *intel_plane, uint32_t format)
>> > +{
>> > + struct drm_i915_private *dev_priv = intel_plane->base.dev->dev_private;
>> > + int plane = intel_plane->plane;
>> > +
>> > + /* Seems RGB data bypasses the CSC always */
>> > + if (!format_is_yuv(format))
>> > + return;
>> > +
>> > + /*
>> > + * BT.601 limited range YCbCr -> full range RGB
>> > + *
>> > + * |r| | 6537 4769 0| |cr |
>> > + * |g| = |-3330 4769 -1605| x |y-64|
>> > + * |b| | 0 4769 8263| |cb |
>>
>> Where did you get this values? the ones at wikipedia didn't match...
>
> I had the RGB->YCbCr matrix, inverted it and the values came out. But they
> should match the wikipedia article. Also keep in mind that the coefficients
> are in .12 in fixed point format, hence we need a 1<<12 factor.
Oh!
> So let's
> try it:
>
> Kb=.114
> Kr=.299
> (1<<12) * 255/219 ~= 4769
> -(1<<12) * 255/112*(1-Kb)*Kb/(1-Kb-Kr) ~= -1605
> -(1<<12) * 255/112*(1-Kr)*Kr/(1-Kb-Kr) ~= -3330
> (1<<12) * 255/112*(1-Kr) ~= 6537
> (1<<12) * 255/112*(1-Kb) ~= 8263
>
> Looks like the same values to me.
Indeed.
>
>>
>> > + *
>> > + * Cb and Cr apparently come in as signed already, so no
>> > + * need for any offset. For Y we need to remove the offset.
>> > + */
>> > + I915_WRITE(SPCSCYGOFF(plane), SPCSC_OOFF(0) | SPCSC_IOFF(-64));
>> > + I915_WRITE(SPCSCCBOFF(plane), SPCSC_OOFF(0) | SPCSC_IOFF(0));
>> > + I915_WRITE(SPCSCCROFF(plane), SPCSC_OOFF(0) | SPCSC_IOFF(0));
>> > +
>> > + I915_WRITE(SPCSCC01(plane), SPCSC_C1(4769) | SPCSC_C0(6537));
>> > + I915_WRITE(SPCSCC23(plane), SPCSC_C1(-3330) | SPCSC_C0(0));
>> > + I915_WRITE(SPCSCC45(plane), SPCSC_C1(-1605) | SPCSC_C0(4769));
>> > + I915_WRITE(SPCSCC67(plane), SPCSC_C1(4769) | SPCSC_C0(0));
>> > + I915_WRITE(SPCSCC8(plane), SPCSC_C0(8263));
>> > +
>> > + I915_WRITE(SPCSCYGICLAMP(plane), SPCSC_IMAX(940) | SPCSC_IMIN(64));
>> > + I915_WRITE(SPCSCCBICLAMP(plane), SPCSC_IMAX(448) | SPCSC_IMIN(-448));
>> > + I915_WRITE(SPCSCCRICLAMP(plane), SPCSC_IMAX(448) | SPCSC_IMIN(-448));
>>
>> where did you get these min/max?
>
> The hardware apparently deals in 10bit values, so we need to multiply everything
> by 4 when we start with the 8bit min/max values.
>
> Y = [16:235] * 4 = [64:940]
> CbCr = ([16:240] - 128) * 4 = [-112:112] * 4 = [-448:448]
>
> The -128 being the -0.5 bias that the hardware already applied before
> the data entered the CSC unit.
Ah! cool, thanks!
>
>>
>> > +
>> > + I915_WRITE(SPCSCYGOCLAMP(plane), SPCSC_OMAX(1023) | SPCSC_OMIN(0));
>> > + I915_WRITE(SPCSCCBOCLAMP(plane), SPCSC_OMAX(1023) | SPCSC_OMIN(0));
>> > + I915_WRITE(SPCSCCROCLAMP(plane), SPCSC_OMAX(1023) | SPCSC_OMIN(0));
>>
>> and these ones?
>
> Just 10bpc RGB data going out.
ok!
Thanks for the explanation.
Patch looks good.
Reviewed-by Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
>
>>
>> > +}
>> > +
>> > +static void
>> > vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
>> > struct drm_framebuffer *fb,
>> > struct drm_i915_gem_object *obj, int crtc_x, int crtc_y,
>> > @@ -430,6 +483,9 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
>> >
>> > intel_update_primary_plane(intel_crtc);
>> >
>> > + if (IS_CHERRYVIEW(dev) && pipe == PIPE_B)
>> > + chv_update_csc(intel_plane, fb->pixel_format);
>> > +
>> > I915_WRITE(SPSTRIDE(pipe, plane), fb->pitches[0]);
>> > I915_WRITE(SPPOS(pipe, plane), (crtc_y << 16) | crtc_x);
>> >
>> > @@ -1004,20 +1060,6 @@ ilk_get_colorkey(struct drm_plane *plane, struct drm_intel_sprite_colorkey *key)
>> > key->flags = I915_SET_COLORKEY_NONE;
>> > }
>> >
>> > -static bool
>> > -format_is_yuv(uint32_t format)
>> > -{
>> > - switch (format) {
>> > - case DRM_FORMAT_YUYV:
>> > - case DRM_FORMAT_UYVY:
>> > - case DRM_FORMAT_VYUY:
>> > - case DRM_FORMAT_YVYU:
>> > - return true;
>> > - default:
>> > - return false;
>> > - }
>> > -}
>> > -
>> > static bool colorkey_enabled(struct intel_plane *intel_plane)
>> > {
>> > struct drm_intel_sprite_colorkey key;
>> > --
>> > 2.0.4
>> >
>> > _______________________________________________
>> > Intel-gfx mailing list
>> > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
>> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>
>> With numbers explained the rest is ok and you can use Reviewed-by:
>> Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
>>
>>
>> --
>> Rodrigo Vivi
>> Blog: http://blog.vivi.eng.br
>
> --
> Ville Syrjälä
> Intel OTC
--
Rodrigo Vivi
Blog: http://blog.vivi.eng.br
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
