* [PATCH] drm/i915: Add support for CHV pipe B sprite CSC
@ 2014-10-20 16:47 ville.syrjala
2014-10-29 20:47 ` Rodrigo Vivi
0 siblings, 1 reply; 5+ messages in thread
From: ville.syrjala @ 2014-10-20 16:47 UTC (permalink / raw)
To: intel-gfx
From: Ville Syrjälä <ville.syrjala@linux.intel.com>
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@linux.intel.com>
---
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 |
+ *
+ * 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));
+
+ 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));
+}
+
+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@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] drm/i915: Add support for CHV pipe B sprite CSC
2014-10-20 16:47 [PATCH] drm/i915: Add support for CHV pipe B sprite CSC ville.syrjala
@ 2014-10-29 20:47 ` Rodrigo Vivi
2014-10-30 8:30 ` Ville Syrjälä
0 siblings, 1 reply; 5+ messages in thread
From: Rodrigo Vivi @ 2014-10-29 20:47 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: intel-gfx
First of all thanks for the spec
On Mon, Oct 20, 2014 at 9:47 AM, <ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> 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@linux.intel.com>
> ---
> 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...
> + *
> + * 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?
> +
> + 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?
> +}
> +
> +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@lists.freedesktop.org
> 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@intel.com>
--
Rodrigo Vivi
Blog: http://blog.vivi.eng.br
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] drm/i915: Add support for CHV pipe B sprite CSC
2014-10-29 20:47 ` Rodrigo Vivi
@ 2014-10-30 8:30 ` Ville Syrjälä
2014-10-30 19:59 ` Rodrigo Vivi
0 siblings, 1 reply; 5+ messages in thread
From: Ville Syrjälä @ 2014-10-30 8:30 UTC (permalink / raw)
To: Rodrigo Vivi; +Cc: intel-gfx
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@linux.intel.com> wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > 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@linux.intel.com>
> > ---
> > 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. 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.
>
> > + *
> > + * 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.
>
> > +
> > + 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.
>
> > +}
> > +
> > +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@lists.freedesktop.org
> > 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@intel.com>
>
>
> --
> Rodrigo Vivi
> Blog: http://blog.vivi.eng.br
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] drm/i915: Add support for CHV pipe B sprite CSC
2014-10-30 8:30 ` Ville Syrjälä
@ 2014-10-30 19:59 ` Rodrigo Vivi
2014-11-03 11:17 ` Daniel Vetter
0 siblings, 1 reply; 5+ messages in thread
From: Rodrigo Vivi @ 2014-10-30 19:59 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: intel-gfx
On Thu, Oct 30, 2014 at 1:30 AM, Ville Syrjälä
<ville.syrjala@linux.intel.com> 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@linux.intel.com> wrote:
>> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> >
>> > 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@linux.intel.com>
>> > ---
>> > 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@intel.com>
>
>>
>> > +}
>> > +
>> > +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@lists.freedesktop.org
>> > 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@intel.com>
>>
>>
>> --
>> 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@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2014-11-03 11:17 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-10-20 16:47 [PATCH] drm/i915: Add support for CHV pipe B sprite CSC ville.syrjala
2014-10-29 20:47 ` Rodrigo Vivi
2014-10-30 8:30 ` Ville Syrjälä
2014-10-30 19:59 ` Rodrigo Vivi
2014-11-03 11:17 ` Daniel Vetter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox