* [PATCH 0/5] drm/i915; Support for COLOR_ENCODING and COLOR_RANGE props
@ 2017-06-08 20:33 ville.syrjala
2017-06-08 20:33 ` [PATCH v2 1/5] drm/i915: Correctly handle limited range YCbCr data on VLV/CHV ville.syrjala
` (7 more replies)
0 siblings, 8 replies; 27+ messages in thread
From: ville.syrjala @ 2017-06-08 20:33 UTC (permalink / raw)
To: intel-gfx; +Cc: Jyri Sarha
From: Ville Syrjälä <ville.syrjala@linux.intel.com>
While reviewing the COLOR_ENCODING/COLOR_RANGE props I decided that I
might as well implement them in i915. And here is the result.
With this the user can now select between BT.601 vs. BT.709, and
limited vs. full range on every platform where we currently have
YCbCr framebuffer support. I had to fix up GLK a bit more since it
wasn't actually doing the YCbCr->RGB conversion at all.
In order to unblock this further I also hacked together some userspace
support in the form of the XV_COLORSPACE port attribute in the intel
ddx. For that I needed to hack off the atomic requirement for these
properties. I don't think there's much point in restricting these
to atomic anyway.
I've pushed the entire series (including Jyri's stuff with my hack) here:
git://github.com/vsyrjala/linux.git color_encoding_prop
And the intel ddx xv stuff is available here:
git://github.com/vsyrjala/xf86-video-intel.git xv_colorspace
Cc: Jyri Sarha <jsarha@ti.com>
Ville Syrjälä (5):
drm/i915: Correctly handle limited range YCbCr data on VLV/CHV
drm/i915: Fix plane YCbCr->RGB conversion for GLK
drm/i915: Add support for the YCbCr COLOR_ENCODING property
drm/i915: Change the COLOR_ENCODING prop default value to BT.709
drm/i915: Add support for the YCbCr COLOR_RANGE property
drivers/gpu/drm/i915/i915_reg.h | 22 ++++++
drivers/gpu/drm/i915/intel_display.c | 41 ++++++++++-
drivers/gpu/drm/i915/intel_drv.h | 2 +
drivers/gpu/drm/i915/intel_sprite.c | 130 +++++++++++++++++++++++++++--------
4 files changed, 164 insertions(+), 31 deletions(-)
--
2.13.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v2 1/5] drm/i915: Correctly handle limited range YCbCr data on VLV/CHV
2017-06-08 20:33 [PATCH 0/5] drm/i915; Support for COLOR_ENCODING and COLOR_RANGE props ville.syrjala
@ 2017-06-08 20:33 ` ville.syrjala
2017-06-15 12:38 ` Sharma, Shashank
2017-06-20 13:32 ` [PATCH v3 " ville.syrjala
2017-06-08 20:33 ` [PATCH 2/5] drm/i915: Fix plane YCbCr->RGB conversion for GLK ville.syrjala
` (6 subsequent siblings)
7 siblings, 2 replies; 27+ messages in thread
From: ville.syrjala @ 2017-06-08 20:33 UTC (permalink / raw)
To: intel-gfx; +Cc: Tang, Jun, stable, Jyri Sarha
From: Ville Syrjälä <ville.syrjala@linux.intel.com>
Turns out the VLV/CHV fixed function sprite CSC expects full range
data as input. We've been feeding it limited range data to it all
along. To expand the data out to full range we'll use the color
correction registers (brightness, contrast, and saturation).
On CHV pipe B we were actually doing the right thing already because we
progammed the custom CSC matrix to do expect limited range input. Now
that well pre-expand the data out with the color correction unit, we
need to change the CSC matrix to operate with full range input instead.
This should make the sprite output of the other pipes match the sprite
output of pipe B reasonably well. Looking at the resulting pipe CRCs,
there can be a slight difference in the output, but as I don't know
the formula used by the fixed function CSC of the other pipes, I don't
think it's worth the effort to try to match the output exactly. It
might not even be possible due to difference in internal precision etc.
One slight caveat here is that the color correction registers are single
bufferred, so we should really be updating them during vblank, but we
still don't have a mechanism for that, so just toss in another FIXME.
v2: Rebase
Cc: Jyri Sarha <jsarha@ti.com>
Cc: "Tang, Jun" <jun.tang@intel.com>
Reported-by: "Tang, Jun" <jun.tang@intel.com>
Cc: stable@vger.kernel.org
Fixes: 7f1f3851feb0 ("drm/i915: sprite support for ValleyView v4")
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
drivers/gpu/drm/i915/i915_reg.h | 10 +++++
drivers/gpu/drm/i915/intel_sprite.c | 74 ++++++++++++++++++++++++++++---------
2 files changed, 66 insertions(+), 18 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index b6d69e289974..ce7c0dc79cf7 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -5777,6 +5777,12 @@ enum {
#define _SPATILEOFF (VLV_DISPLAY_BASE + 0x721a4)
#define _SPACONSTALPHA (VLV_DISPLAY_BASE + 0x721a8)
#define SP_CONST_ALPHA_ENABLE (1<<31)
+#define _SPACLRC0 (VLV_DISPLAY_BASE + 0x721d0)
+#define SP_CONTRAST(x) ((x) << 18) /* u3.6 */
+#define SP_BRIGHTNESS(x) ((x) & 0xff) /* s8 */
+#define _SPACLRC1 (VLV_DISPLAY_BASE + 0x721d4)
+#define SP_SH_SIN(x) (((x) & 0x7ff) << 16) /* s4.7 */
+#define SP_SH_COS(x) (x) /* u3.7 */
#define _SPAGAMC (VLV_DISPLAY_BASE + 0x721f4)
#define _SPBCNTR (VLV_DISPLAY_BASE + 0x72280)
@@ -5790,6 +5796,8 @@ enum {
#define _SPBKEYMAXVAL (VLV_DISPLAY_BASE + 0x722a0)
#define _SPBTILEOFF (VLV_DISPLAY_BASE + 0x722a4)
#define _SPBCONSTALPHA (VLV_DISPLAY_BASE + 0x722a8)
+#define _SPBCLRC0 (VLV_DISPLAY_BASE + 0x722d0)
+#define _SPBCLRC1 (VLV_DISPLAY_BASE + 0x722d4)
#define _SPBGAMC (VLV_DISPLAY_BASE + 0x722f4)
#define _MMIO_VLV_SPR(pipe, plane_id, reg_a, reg_b) \
@@ -5806,6 +5814,8 @@ enum {
#define SPKEYMAXVAL(pipe, plane_id) _MMIO_VLV_SPR((pipe), (plane_id), _SPAKEYMAXVAL, _SPBKEYMAXVAL)
#define SPTILEOFF(pipe, plane_id) _MMIO_VLV_SPR((pipe), (plane_id), _SPATILEOFF, _SPBTILEOFF)
#define SPCONSTALPHA(pipe, plane_id) _MMIO_VLV_SPR((pipe), (plane_id), _SPACONSTALPHA, _SPBCONSTALPHA)
+#define SPCLRC0(pipe, plane_id) _MMIO_VLV_SPR((pipe), (plane_id), _SPACLRC0, _SPBCLRC0)
+#define SPCLRC1(pipe, plane_id) _MMIO_VLV_SPR((pipe), (plane_id), _SPACLRC1, _SPBCLRC1)
#define SPGAMC(pipe, plane_id) _MMIO_VLV_SPR((pipe), (plane_id), _SPAGAMC, _SPBGAMC)
/*
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 0c650c2cbca8..2ce3b3c6ffa8 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -325,44 +325,80 @@ skl_disable_plane(struct intel_plane *plane, struct intel_crtc *crtc)
}
static void
-chv_update_csc(struct intel_plane *plane, uint32_t format)
+chv_update_csc(const struct intel_plane_state *plane_state)
{
+ struct intel_plane *plane = to_intel_plane(plane_state->base.plane);
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;
/* Seems RGB data bypasses the CSC always */
- if (!format_is_yuv(format))
+ if (!format_is_yuv(fb->format->format))
return;
/*
- * BT.601 limited range YCbCr -> full range RGB
+ * BT.601 full range YCbCr -> full range RGB
*
- * |r| | 6537 4769 0| |cr |
- * |g| = |-3330 4769 -1605| x |y-64|
- * |b| | 0 4769 8263| |cb |
+ * |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, so no
- * need for any offset. For Y we need to remove the offset.
+ * 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(-64));
+ 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(4769) | SPCSC_C0(6537));
- I915_WRITE_FW(SPCSCC23(plane_id), SPCSC_C1(-3330) | SPCSC_C0(0));
- I915_WRITE_FW(SPCSCC45(plane_id), SPCSC_C1(-1605) | SPCSC_C0(4769));
- I915_WRITE_FW(SPCSCC67(plane_id), SPCSC_C1(4769) | SPCSC_C0(0));
- I915_WRITE_FW(SPCSCC8(plane_id), SPCSC_C0(8263));
+ 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(SPCSCYGICLAMP(plane_id), SPCSC_IMAX(940) | SPCSC_IMIN(64));
- I915_WRITE_FW(SPCSCCBICLAMP(plane_id), SPCSC_IMAX(448) | SPCSC_IMIN(-448));
- I915_WRITE_FW(SPCSCCRICLAMP(plane_id), SPCSC_IMAX(448) | SPCSC_IMIN(-448));
+ I915_WRITE_FW(SPCSCYGICLAMP(plane_id), SPCSC_IMAX(1023) | SPCSC_IMIN(0));
+ I915_WRITE_FW(SPCSCCBICLAMP(plane_id), SPCSC_IMAX(512) | SPCSC_IMIN(-512));
+ I915_WRITE_FW(SPCSCCRICLAMP(plane_id), SPCSC_IMAX(512) | SPCSC_IMIN(-512));
I915_WRITE_FW(SPCSCYGOCLAMP(plane_id), SPCSC_OMAX(1023) | SPCSC_OMIN(0));
I915_WRITE_FW(SPCSCCBOCLAMP(plane_id), SPCSC_OMAX(1023) | SPCSC_OMIN(0));
I915_WRITE_FW(SPCSCCROCLAMP(plane_id), SPCSC_OMAX(1023) | SPCSC_OMIN(0));
}
+static void
+vlv_update_clrc(const struct intel_plane_state *plane_state)
+{
+ struct intel_plane *plane = to_intel_plane(plane_state->base.plane);
+ struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
+ const struct drm_framebuffer *fb = plane_state->base.fb;
+ enum pipe pipe = plane->pipe;
+ enum plane_id plane_id = plane->id;
+ int con, bri, sh_sin, sh_cos;
+
+ if (format_is_yuv(fb->format->format)) {
+ /*
+ * expand limited range to full range.
+ * contrast is applied first, then brightness
+ */
+ con = ((255 << 7) / 219 + 1) >> 1;
+ bri = -((16 << 1) * 255 / 219 + 1) >> 1;
+ sh_sin = 0;
+ sh_cos = (((128 << 8) / 112) + 1) >> 1;
+ } else {
+ /* pass-through everything */
+ con = 1 << 6;
+ bri = 0;
+ sh_sin = 0;
+ sh_cos = 1 << 7;
+ }
+
+ /* FIXME these register are single buffered :( */
+ I915_WRITE_FW(SPCLRC0(pipe, plane_id),
+ SP_CONTRAST(con) | SP_BRIGHTNESS(bri));
+ I915_WRITE_FW(SPCLRC1(pipe, plane_id),
+ SP_SH_SIN(sh_sin) | SP_SH_COS(sh_cos));
+}
+
static u32 vlv_sprite_ctl(const struct intel_crtc_state *crtc_state,
const struct intel_plane_state *plane_state)
{
@@ -456,8 +492,10 @@ vlv_update_plane(struct intel_plane *plane,
spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
+ vlv_update_clrc(plane_state);
+
if (IS_CHERRYVIEW(dev_priv) && pipe == PIPE_B)
- chv_update_csc(plane, fb->format->format);
+ chv_update_csc(plane_state);
if (key->flags) {
I915_WRITE_FW(SPKEYMINVAL(pipe, plane_id), key->min_value);
--
2.13.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH 2/5] drm/i915: Fix plane YCbCr->RGB conversion for GLK
2017-06-08 20:33 [PATCH 0/5] drm/i915; Support for COLOR_ENCODING and COLOR_RANGE props ville.syrjala
2017-06-08 20:33 ` [PATCH v2 1/5] drm/i915: Correctly handle limited range YCbCr data on VLV/CHV ville.syrjala
@ 2017-06-08 20:33 ` ville.syrjala
2017-06-15 12:27 ` Sharma, Shashank
2017-06-08 20:33 ` [PATCH 3/5] drm/i915: Add support for the YCbCr COLOR_ENCODING property ville.syrjala
` (5 subsequent siblings)
7 siblings, 1 reply; 27+ messages in thread
From: ville.syrjala @ 2017-06-08 20:33 UTC (permalink / raw)
To: intel-gfx; +Cc: Ander Conselvan de Oliveira, Jyri Sarha
From: Ville Syrjälä <ville.syrjala@linux.intel.com>
On GLK the plane CSC controls moved into the COLOR_CTL register.
Update the code to progam the YCbCr->RGB CSC mode correctly when
faced with an YCbCr framebuffer.
The spec is rather confusing as it calls the mode "YUV601 to RGB709".
I'm going to assume that just means it's going to use the YCbCr->RGB
matrix as specified in BT.601 and doesn't actually change the gamut.
Cc: Jyri Sarha <jsarha@ti.com>
Cc: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
drivers/gpu/drm/i915/i915_reg.h | 5 +++++
drivers/gpu/drm/i915/intel_display.c | 19 ++++++++++++++++---
drivers/gpu/drm/i915/intel_drv.h | 2 ++
drivers/gpu/drm/i915/intel_sprite.c | 13 +++++--------
4 files changed, 28 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index ce7c0dc79cf7..f4f51e7a3e83 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -5924,6 +5924,11 @@ enum {
#define _PLANE_COLOR_CTL_3_A 0x703CC /* GLK+ */
#define PLANE_COLOR_PIPE_GAMMA_ENABLE (1 << 30)
#define PLANE_COLOR_PIPE_CSC_ENABLE (1 << 23)
+#define PLANE_COLOR_CSC_MODE_BYPASS (0 << 17)
+#define PLANE_COLOR_CSC_MODE_YUV601_TO_RGB709 (1 << 17)
+#define PLANE_COLOR_CSC_MODE_YUV709_TO_RGB709 (2 << 17)
+#define PLANE_COLOR_CSC_MODE_YUV2020_TO_RGB2020 (3 << 17)
+#define PLANE_COLOR_CSC_MODE_RGB709_TO_RGB2020 (4 << 17)
#define PLANE_COLOR_PLANE_GAMMA_DISABLE (1 << 13)
#define _PLANE_BUF_CFG_1_A 0x7027c
#define _PLANE_BUF_CFG_2_A 0x7037c
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 25390dd8e08e..e2aaaf60a969 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3335,6 +3335,21 @@ u32 skl_plane_ctl(const struct intel_crtc_state *crtc_state,
return plane_ctl;
}
+u32 glk_color_ctl(const struct intel_plane_state *plane_state)
+{
+ const struct drm_framebuffer *fb = plane_state->base.fb;
+ u32 color_ctl;
+
+ color_ctl = PLANE_COLOR_PIPE_GAMMA_ENABLE |
+ 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;
+
+ return color_ctl;
+}
+
static void skylake_update_primary_plane(struct intel_plane *plane,
const struct intel_crtc_state *crtc_state,
const struct intel_plane_state *plane_state)
@@ -3374,9 +3389,7 @@ static void skylake_update_primary_plane(struct intel_plane *plane,
if (IS_GEMINILAKE(dev_priv)) {
I915_WRITE_FW(PLANE_COLOR_CTL(pipe, plane_id),
- PLANE_COLOR_PIPE_GAMMA_ENABLE |
- PLANE_COLOR_PIPE_CSC_ENABLE |
- PLANE_COLOR_PLANE_GAMMA_DISABLE);
+ glk_color_ctl(plane_state));
}
I915_WRITE_FW(PLANE_CTL(pipe, plane_id), plane_ctl);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 83dd40905821..e13e714b1176 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1484,6 +1484,7 @@ static inline u32 intel_plane_ggtt_offset(const struct intel_plane_state *state)
u32 skl_plane_ctl(const struct intel_crtc_state *crtc_state,
const struct intel_plane_state *plane_state);
+u32 glk_color_ctl(const struct intel_plane_state *plane_state);
u32 skl_plane_stride(const struct drm_framebuffer *fb, int plane,
unsigned int rotation);
int skl_check_plane_surface(struct intel_plane_state *plane_state);
@@ -1888,6 +1889,7 @@ bool intel_sdvo_init(struct drm_i915_private *dev_priv,
/* intel_sprite.c */
+bool intel_format_is_yuv(u32 format);
int intel_usecs_to_scanlines(const struct drm_display_mode *adjusted_mode,
int usecs);
struct intel_plane *intel_sprite_plane_create(struct drm_i915_private *dev_priv,
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 2ce3b3c6ffa8..cc07157f2eb6 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -40,8 +40,7 @@
#include <drm/i915_drm.h>
#include "i915_drv.h"
-static bool
-format_is_yuv(uint32_t format)
+bool intel_format_is_yuv(u32 format)
{
switch (format) {
case DRM_FORMAT_YUYV:
@@ -264,9 +263,7 @@ skl_update_plane(struct intel_plane *plane,
if (IS_GEMINILAKE(dev_priv)) {
I915_WRITE_FW(PLANE_COLOR_CTL(pipe, plane_id),
- PLANE_COLOR_PIPE_GAMMA_ENABLE |
- PLANE_COLOR_PIPE_CSC_ENABLE |
- PLANE_COLOR_PLANE_GAMMA_DISABLE);
+ glk_color_ctl(plane_state));
}
if (key->flags) {
@@ -333,7 +330,7 @@ chv_update_csc(const struct intel_plane_state *plane_state)
enum plane_id plane_id = plane->id;
/* Seems RGB data bypasses the CSC always */
- if (!format_is_yuv(fb->format->format))
+ if (!intel_format_is_yuv(fb->format->format))
return;
/*
@@ -375,7 +372,7 @@ vlv_update_clrc(const struct intel_plane_state *plane_state)
enum plane_id plane_id = plane->id;
int con, bri, sh_sin, sh_cos;
- if (format_is_yuv(fb->format->format)) {
+ if (intel_format_is_yuv(fb->format->format)) {
/*
* expand limited range to full range.
* contrast is applied first, then brightness
@@ -933,7 +930,7 @@ intel_check_sprite_plane(struct intel_plane *plane,
src_y = src->y1 >> 16;
src_h = drm_rect_height(src) >> 16;
- if (format_is_yuv(fb->format->format)) {
+ if (intel_format_is_yuv(fb->format->format)) {
src_x &= ~1;
src_w &= ~1;
--
2.13.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH 3/5] drm/i915: Add support for the YCbCr COLOR_ENCODING property
2017-06-08 20:33 [PATCH 0/5] drm/i915; Support for COLOR_ENCODING and COLOR_RANGE props ville.syrjala
2017-06-08 20:33 ` [PATCH v2 1/5] drm/i915: Correctly handle limited range YCbCr data on VLV/CHV ville.syrjala
2017-06-08 20:33 ` [PATCH 2/5] drm/i915: Fix plane YCbCr->RGB conversion for GLK ville.syrjala
@ 2017-06-08 20:33 ` ville.syrjala
2017-06-15 13:22 ` Sharma, Shashank
2017-06-20 13:33 ` [PATCH v2 " ville.syrjala
2017-06-08 20:33 ` [PATCH 4/5] drm/i915: Change the COLOR_ENCODING prop default value to BT.709 ville.syrjala
` (4 subsequent siblings)
7 siblings, 2 replies; 27+ messages in thread
From: ville.syrjala @ 2017-06-08 20:33 UTC (permalink / raw)
To: intel-gfx; +Cc: Jyri Sarha
From: Ville Syrjälä <ville.syrjala@linux.intel.com>
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@ti.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
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)
#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)
+ 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;
--
2.13.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH 4/5] drm/i915: Change the COLOR_ENCODING prop default value to BT.709
2017-06-08 20:33 [PATCH 0/5] drm/i915; Support for COLOR_ENCODING and COLOR_RANGE props ville.syrjala
` (2 preceding siblings ...)
2017-06-08 20:33 ` [PATCH 3/5] drm/i915: Add support for the YCbCr COLOR_ENCODING property ville.syrjala
@ 2017-06-08 20:33 ` ville.syrjala
2017-06-15 13:31 ` Sharma, Shashank
2017-06-08 20:33 ` [PATCH 5/5] drm/i915: Add support for the YCbCr COLOR_RANGE property ville.syrjala
` (3 subsequent siblings)
7 siblings, 1 reply; 27+ messages in thread
From: ville.syrjala @ 2017-06-08 20:33 UTC (permalink / raw)
To: intel-gfx; +Cc: Jyri Sarha
From: Ville Syrjälä <ville.syrjala@linux.intel.com>
Bring us forward from the stone age and switch our default YCbCr->RGB
conversion matrix to BT.709 from BT.601. I would expect most matrial
to be BT.709 these days.
Cc: Jyri Sarha <jsarha@ti.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
drivers/gpu/drm/i915/intel_display.c | 2 +-
drivers/gpu/drm/i915/intel_sprite.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 810b53b0128c..ba3ad0e26043 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13831,7 +13831,7 @@ intel_primary_plane_create(struct drm_i915_private *dev_priv, enum pipe pipe)
BIT(DRM_COLOR_YCBCR_BT601) |
BIT(DRM_COLOR_YCBCR_BT709),
BIT(DRM_COLOR_YCBCR_LIMITED_RANGE),
- DRM_COLOR_YCBCR_BT601,
+ DRM_COLOR_YCBCR_BT709,
DRM_COLOR_YCBCR_LIMITED_RANGE);
drm_plane_helper_add(&primary->base, &intel_plane_helper_funcs);
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index c21f43d64b00..ba433bf40189 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -1249,7 +1249,7 @@ intel_sprite_plane_create(struct drm_i915_private *dev_priv,
BIT(DRM_COLOR_YCBCR_BT601) |
BIT(DRM_COLOR_YCBCR_BT709),
BIT(DRM_COLOR_YCBCR_LIMITED_RANGE),
- DRM_COLOR_YCBCR_BT601,
+ DRM_COLOR_YCBCR_BT709,
DRM_COLOR_YCBCR_LIMITED_RANGE);
drm_plane_helper_add(&intel_plane->base, &intel_plane_helper_funcs);
--
2.13.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH 5/5] drm/i915: Add support for the YCbCr COLOR_RANGE property
2017-06-08 20:33 [PATCH 0/5] drm/i915; Support for COLOR_ENCODING and COLOR_RANGE props ville.syrjala
` (3 preceding siblings ...)
2017-06-08 20:33 ` [PATCH 4/5] drm/i915: Change the COLOR_ENCODING prop default value to BT.709 ville.syrjala
@ 2017-06-08 20:33 ` ville.syrjala
2017-06-15 13:37 ` Sharma, Shashank
2017-06-20 13:35 ` [PATCH v2 " ville.syrjala
2017-06-08 20:33 ` [PATCH xf86-video-intel] sna: Add XV_COLORSPACE attribute support for sprite Xv adaptors ville.syrjala
` (2 subsequent siblings)
7 siblings, 2 replies; 27+ messages in thread
From: ville.syrjala @ 2017-06-08 20:33 UTC (permalink / raw)
To: intel-gfx; +Cc: Jyri Sarha
From: Ville Syrjälä <ville.syrjala@linux.intel.com>
Add support for the COLOR_RANGE property on planes. This property
selects whether the input YCbCr data is to treated as limited range
or full range.
On most platforms this is a matter of setting the "YUV range correction
disable" bit, and on VLV/CHV we'll just have to program the color
correction logic to pass the data through unmodified.
Cc: Jyri Sarha <jsarha@ti.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
drivers/gpu/drm/i915/i915_reg.h | 4 ++++
drivers/gpu/drm/i915/intel_display.c | 9 ++++++++-
drivers/gpu/drm/i915/intel_sprite.c | 12 ++++++++++--
3 files changed, 22 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index bac3ec378b6e..6b16b25ba96f 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -5604,6 +5604,7 @@ enum {
#define _DVSACNTR 0x72180
#define DVS_ENABLE (1<<31)
#define DVS_GAMMA_ENABLE (1<<30)
+#define DVS_YUV_RANGE_CORRECTION_DISABLE (1<<27)
#define DVS_PIXFORMAT_MASK (3<<25)
#define DVS_FORMAT_YUV422 (0<<25)
#define DVS_FORMAT_RGBX101010 (1<<25)
@@ -5672,6 +5673,7 @@ enum {
#define _SPRA_CTL 0x70280
#define SPRITE_ENABLE (1<<31)
#define SPRITE_GAMMA_ENABLE (1<<30)
+#define SPRITE_YUV_RANGE_CORRECTION_DISABLE (1<<28)
#define SPRITE_PIXFORMAT_MASK (7<<25)
#define SPRITE_FORMAT_YUV422 (0<<25)
#define SPRITE_FORMAT_RGBX101010 (1<<25)
@@ -5863,6 +5865,7 @@ enum {
#define _PLANE_CTL_3_A 0x70380
#define PLANE_CTL_ENABLE (1 << 31)
#define PLANE_CTL_PIPE_GAMMA_ENABLE (1 << 30)
+#define PLANE_CTL_YUV_RANGE_CORRECTION_DISABLE (1 << 28)
#define PLANE_CTL_FORMAT_MASK (0xf << 24)
#define PLANE_CTL_FORMAT_YUV422 ( 0 << 24)
#define PLANE_CTL_FORMAT_NV12 ( 1 << 24)
@@ -5926,6 +5929,7 @@ enum {
#define _PLANE_COLOR_CTL_2_A 0x702CC /* GLK+ */
#define _PLANE_COLOR_CTL_3_A 0x703CC /* GLK+ */
#define PLANE_COLOR_PIPE_GAMMA_ENABLE (1 << 30)
+#define PLANE_COLOR_YUV_RANGE_CORRECTION_DISABLE (1 << 28)
#define PLANE_COLOR_PIPE_CSC_ENABLE (1 << 23)
#define PLANE_COLOR_CSC_MODE_BYPASS (0 << 17)
#define PLANE_COLOR_CSC_MODE_YUV601_TO_RGB709 (1 << 17)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index ba3ad0e26043..6fe6e67eeefc 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3324,6 +3324,9 @@ u32 skl_plane_ctl(const struct intel_crtc_state *crtc_state,
if (plane_state->base.color_encoding == DRM_COLOR_YCBCR_BT709)
plane_ctl |= PLANE_CTL_YUV_CSC_FORMAT_BT709;
+
+ if (plane_state->base.color_range == DRM_COLOR_YCBCR_FULL_RANGE)
+ plane_ctl |= PLANE_CTL_YUV_RANGE_CORRECTION_DISABLE;
}
plane_ctl |= skl_plane_ctl_format(fb->format->format);
@@ -3352,6 +3355,9 @@ u32 glk_color_ctl(const struct intel_plane_state *plane_state)
color_ctl |= PLANE_COLOR_CSC_MODE_YUV709_TO_RGB709;
else
color_ctl |= PLANE_COLOR_CSC_MODE_YUV601_TO_RGB709;
+
+ if (plane_state->base.color_range == DRM_COLOR_YCBCR_FULL_RANGE)
+ color_ctl |= PLANE_COLOR_YUV_RANGE_CORRECTION_DISABLE;
}
return color_ctl;
@@ -13830,7 +13836,8 @@ intel_primary_plane_create(struct drm_i915_private *dev_priv, enum pipe pipe)
drm_plane_create_color_properties(&primary->base,
BIT(DRM_COLOR_YCBCR_BT601) |
BIT(DRM_COLOR_YCBCR_BT709),
- BIT(DRM_COLOR_YCBCR_LIMITED_RANGE),
+ BIT(DRM_COLOR_YCBCR_LIMITED_RANGE) |
+ BIT(DRM_COLOR_YCBCR_FULL_RANGE),
DRM_COLOR_YCBCR_BT709,
DRM_COLOR_YCBCR_LIMITED_RANGE);
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index ba433bf40189..6276322fd8ad 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -387,7 +387,8 @@ vlv_update_clrc(const struct intel_plane_state *plane_state)
enum plane_id plane_id = plane->id;
int con, bri, sh_sin, sh_cos;
- if (intel_format_is_yuv(fb->format->format)) {
+ if (intel_format_is_yuv(fb->format->format) &&
+ plane_state->base.color_range == DRM_COLOR_YCBCR_LIMITED_RANGE) {
/*
* expand limited range to full range.
* contrast is applied first, then brightness
@@ -599,6 +600,9 @@ static u32 ivb_sprite_ctl(const struct intel_crtc_state *crtc_state,
if (plane_state->base.color_encoding == DRM_COLOR_YCBCR_BT709)
sprctl |= SPRITE_YUV_CSC_FORMAT_BT709;
+ if (plane_state->base.color_range == DRM_COLOR_YCBCR_FULL_RANGE)
+ sprctl |= SPRITE_YUV_RANGE_CORRECTION_DISABLE;
+
if (fb->modifier == I915_FORMAT_MOD_X_TILED)
sprctl |= SPRITE_TILED;
@@ -739,6 +743,9 @@ static u32 g4x_sprite_ctl(const struct intel_crtc_state *crtc_state,
if (plane_state->base.color_encoding == DRM_COLOR_YCBCR_BT709)
dvscntr |= DVS_YUV_CSC_FORMAT_BT709;
+ if (plane_state->base.color_range == DRM_COLOR_YCBCR_FULL_RANGE)
+ dvscntr |= DVS_YUV_RANGE_CORRECTION_DISABLE;
+
if (fb->modifier == I915_FORMAT_MOD_X_TILED)
dvscntr |= DVS_TILED;
@@ -1248,7 +1255,8 @@ intel_sprite_plane_create(struct drm_i915_private *dev_priv,
drm_plane_create_color_properties(&intel_plane->base,
BIT(DRM_COLOR_YCBCR_BT601) |
BIT(DRM_COLOR_YCBCR_BT709),
- BIT(DRM_COLOR_YCBCR_LIMITED_RANGE),
+ BIT(DRM_COLOR_YCBCR_LIMITED_RANGE) |
+ BIT(DRM_COLOR_YCBCR_FULL_RANGE),
DRM_COLOR_YCBCR_BT709,
DRM_COLOR_YCBCR_LIMITED_RANGE);
--
2.13.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH xf86-video-intel] sna: Add XV_COLORSPACE attribute support for sprite Xv adaptors
2017-06-08 20:33 [PATCH 0/5] drm/i915; Support for COLOR_ENCODING and COLOR_RANGE props ville.syrjala
` (4 preceding siblings ...)
2017-06-08 20:33 ` [PATCH 5/5] drm/i915: Add support for the YCbCr COLOR_RANGE property ville.syrjala
@ 2017-06-08 20:33 ` ville.syrjala
2017-06-08 21:01 ` ✗ Fi.CI.BAT: failure for " Patchwork
2017-06-20 14:08 ` ✗ Fi.CI.BAT: failure for sna: Add XV_COLORSPACE attribute support for sprite Xv adaptors (rev4) Patchwork
7 siblings, 0 replies; 27+ messages in thread
From: ville.syrjala @ 2017-06-08 20:33 UTC (permalink / raw)
To: intel-gfx; +Cc: Jyri Sarha
From: Ville Syrjälä <ville.syrjala@linux.intel.com>
Use the new "COLOR_ENCODING" plane property to implement the
XV_COLORSPACE port attribute for sprite Xv adaptors.
Cc: Jyri Sarha <jsarha@ti.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
src/sna/sna.h | 1 +
src/sna/sna_display.c | 148 +++++++++++++++++++++++++++++++++++----------
src/sna/sna_video.h | 3 +
src/sna/sna_video_sprite.c | 22 ++++++-
4 files changed, 142 insertions(+), 32 deletions(-)
diff --git a/src/sna/sna.h b/src/sna/sna.h
index 7861110a99a9..189407c7cd9d 100644
--- a/src/sna/sna.h
+++ b/src/sna/sna.h
@@ -630,6 +630,7 @@ static inline void sna_present_cancel_flip(struct sna *sna) { }
extern unsigned sna_crtc_count_sprites(xf86CrtcPtr crtc);
extern bool sna_crtc_set_sprite_rotation(xf86CrtcPtr crtc, unsigned idx, uint32_t rotation);
+extern void sna_crtc_set_sprite_colorspace(xf86CrtcPtr crtc, unsigned idx, int colorspace);
extern uint32_t sna_crtc_to_sprite(xf86CrtcPtr crtc, unsigned idx);
extern bool sna_crtc_is_transformed(xf86CrtcPtr crtc);
diff --git a/src/sna/sna_display.c b/src/sna/sna_display.c
index d1f01218a233..78fdca3a1c3a 100644
--- a/src/sna/sna_display.c
+++ b/src/sna/sna_display.c
@@ -222,6 +222,10 @@ struct sna_crtc {
uint32_t supported;
uint32_t current;
} rotation;
+ struct {
+ uint32_t prop;
+ uint64_t values[2];
+ } color_encoding;
struct list link;
} primary;
struct list sprites;
@@ -3282,17 +3286,124 @@ static const xf86CrtcFuncsRec sna_crtc_funcs = {
#endif
};
-inline static bool prop_is_rotation(struct drm_mode_get_property *prop)
+inline static bool prop_has_type_and_name(const struct drm_mode_get_property *prop,
+ unsigned int type, const char *name)
{
- if ((prop->flags & (1 << 5)) == 0)
+ if ((prop->flags & (1 << type)) == 0)
return false;
- if (strcmp(prop->name, "rotation"))
+ if (strcmp(prop->name, name))
return false;
return true;
}
+inline static bool prop_is_rotation(const struct drm_mode_get_property *prop)
+{
+ return prop_has_type_and_name(prop, 5, "rotation");
+}
+
+static void parse_rotation_prop(struct sna *sna, struct plane *p,
+ struct drm_mode_get_property *prop,
+ uint64_t value)
+{
+ struct drm_mode_property_enum *enums;
+ int j;
+
+ p->rotation.prop = prop->prop_id;
+ p->rotation.current = value;
+
+ DBG(("%s: found rotation property .id=%d, value=%ld, num_enums=%d\n",
+ __FUNCTION__, prop->prop_id, value, prop->count_enum_blobs));
+
+ enums = malloc(prop->count_enum_blobs * sizeof(struct drm_mode_property_enum));
+ if (!enums)
+ return;
+
+ prop->count_values = 0;
+ prop->enum_blob_ptr = (uintptr_t)enums;
+
+ if (drmIoctl(sna->kgem.fd, DRM_IOCTL_MODE_GETPROPERTY, prop)) {
+ free(enums);
+ return;
+ }
+
+ /* XXX we assume that the mapping between kernel enum and
+ * RandR remains fixed for our lifetimes.
+ */
+ VG(VALGRIND_MAKE_MEM_DEFINED(enums, sizeof(*enums)*prop->count_enum_blobs));
+ for (j = 0; j < prop->count_enum_blobs; j++) {
+ DBG(("%s: rotation[%d] = %s [%lx]\n", __FUNCTION__,
+ j, enums[j].name, (long)enums[j].value));
+ p->rotation.supported |= 1 << enums[j].value;
+ }
+
+ free(enums);
+}
+
+inline static bool prop_is_color_encoding(const struct drm_mode_get_property *prop)
+{
+ return prop_has_type_and_name(prop, 3, "COLOR_ENCODING");
+}
+
+static void parse_color_encoding_prop(struct sna *sna, struct plane *p,
+ struct drm_mode_get_property *prop,
+ uint64_t value)
+{
+ struct drm_mode_property_enum *enums;
+ unsigned int supported = 0;
+ int j;
+
+ DBG(("%s: found color encoding property .id=%d, value=%ld, num_enums=%d\n",
+ __FUNCTION__, prop->prop_id, (long)value, prop->count_enum_blobs));
+
+ enums = malloc(prop->count_enum_blobs * sizeof(struct drm_mode_property_enum));
+ if (!enums)
+ return;
+
+ prop->count_values = 0;
+ prop->enum_blob_ptr = (uintptr_t)enums;
+
+ if (drmIoctl(sna->kgem.fd, DRM_IOCTL_MODE_GETPROPERTY, prop)) {
+ free(enums);
+ return;
+ }
+
+ VG(VALGRIND_MAKE_MEM_DEFINED(enums, sizeof(*enums)*prop->count_enum_blobs));
+ for (j = 0; j < prop->count_enum_blobs; j++) {
+ if (!strcmp(enums[j].name, "ITU-R BT.601 YCbCr")) {
+ p->color_encoding.values[0] = enums[j].value;
+ supported |= 1 << 0;
+ } else if (!strcmp(enums[j].name, "ITU-R BT.709 YCbCr")) {
+ p->color_encoding.values[1] = enums[j].value;
+ supported |= 1 << 1;
+ }
+ }
+
+ free(enums);
+
+ if (supported == 3)
+ p->color_encoding.prop = prop->prop_id;
+}
+
+void sna_crtc_set_sprite_colorspace(xf86CrtcPtr crtc,
+ unsigned idx, int colorspace)
+{
+ struct plane *p;
+
+ assert(to_sna_crtc(crtc));
+
+ p = lookup_sprite(to_sna_crtc(crtc), idx);
+
+ if (!p->color_encoding.prop)
+ return;
+
+ drmModeObjectSetProperty(to_sna(crtc->scrn)->kgem.fd,
+ p->id, DRM_MODE_OBJECT_PLANE,
+ p->color_encoding.prop,
+ p->color_encoding.values[colorspace]);
+}
+
static int plane_details(struct sna *sna, struct plane *p)
{
#define N_STACK_PROPS 32 /* must be a multiple of 2 */
@@ -3349,34 +3460,9 @@ static int plane_details(struct sna *sna, struct plane *p)
if (strcmp(prop.name, "type") == 0) {
type = values[i];
} else if (prop_is_rotation(&prop)) {
- struct drm_mode_property_enum *enums;
-
- p->rotation.prop = props[i];
- p->rotation.current = values[i];
-
- DBG(("%s: found rotation property .id=%d, value=%ld, num_enums=%d\n",
- __FUNCTION__, prop.prop_id, (long)values[i], prop.count_enum_blobs));
- enums = malloc(prop.count_enum_blobs * sizeof(struct drm_mode_property_enum));
- if (enums != NULL) {
- prop.count_values = 0;
- prop.enum_blob_ptr = (uintptr_t)enums;
-
- if (drmIoctl(sna->kgem.fd, DRM_IOCTL_MODE_GETPROPERTY, &prop) == 0) {
- int j;
-
- /* XXX we assume that the mapping between kernel enum and
- * RandR remains fixed for our lifetimes.
- */
- VG(VALGRIND_MAKE_MEM_DEFINED(enums, sizeof(*enums)*prop.count_enum_blobs));
- for (j = 0; j < prop.count_enum_blobs; j++) {
- DBG(("%s: rotation[%d] = %s [%lx]\n", __FUNCTION__,
- j, enums[j].name, (long)enums[j].value));
- p->rotation.supported |= 1 << enums[j].value;
- }
- }
-
- free(enums);
- }
+ parse_rotation_prop(sna, p, &prop, values[i]);
+ } else if (prop_is_color_encoding(&prop)) {
+ parse_color_encoding_prop(sna, p, &prop, values[i]);
}
}
diff --git a/src/sna/sna_video.h b/src/sna/sna_video.h
index 39cb725f0050..53ea540a62d3 100644
--- a/src/sna/sna_video.h
+++ b/src/sna/sna_video.h
@@ -90,6 +90,9 @@ struct sna_video {
unsigned color_key_changed;
bool has_color_key;
+ unsigned colorspace;
+ unsigned colorspace_changed;
+
/** YUV data buffers */
struct kgem_bo *old_buf[2];
struct kgem_bo *buf;
diff --git a/src/sna/sna_video_sprite.c b/src/sna/sna_video_sprite.c
index 69bfdfd21b29..86b787f76867 100644
--- a/src/sna/sna_video_sprite.c
+++ b/src/sna/sna_video_sprite.c
@@ -67,11 +67,12 @@ struct local_mode_set_plane {
#define MAKE_ATOM(a) MakeAtom(a, sizeof(a) - 1, true)
-static Atom xvColorKey, xvAlwaysOnTop, xvSyncToVblank;
+static Atom xvColorKey, xvAlwaysOnTop, xvSyncToVblank, xvColorspace;
static XvFormatRec formats[] = { {15}, {16}, {24} };
static const XvImageRec images[] = { XVIMAGE_YUY2, XVIMAGE_UYVY, XVMC_RGB888, XVMC_RGB565 };
static const XvAttributeRec attribs[] = {
+ { XvSettable | XvGettable, 0, 1, (char *)"XV_COLORSPACE" }, /* BT.601, BT.709 */
{ XvSettable | XvGettable, 0, 0xffffff, (char *)"XV_COLORKEY" },
{ XvSettable | XvGettable, 0, 1, (char *)"XV_ALWAYS_ON_TOP" },
};
@@ -117,6 +118,10 @@ static int sna_video_sprite_set_attr(ddSetPortAttribute_ARGS)
video->color_key = value;
RegionEmpty(&video->clip);
DBG(("COLORKEY = %ld\n", (long)value));
+ } else if (attribute == xvColorspace) {
+ video->colorspace_changed = ~0;
+ video->colorspace = value;
+ DBG(("COLORSPACE = %ld\n", (long)value));
} else if (attribute == xvSyncToVblank) {
DBG(("%s: SYNC_TO_VBLANK: %d -> %d\n", __FUNCTION__,
video->SyncToVblank, !!value));
@@ -138,6 +143,8 @@ static int sna_video_sprite_get_attr(ddGetPortAttribute_ARGS)
if (attribute == xvColorKey)
*value = video->color_key;
+ else if (attribute == xvColorspace)
+ *value = video->colorspace;
else if (attribute == xvAlwaysOnTop)
*value = video->AlwaysOnTop;
else if (attribute == xvSyncToVblank)
@@ -263,6 +270,16 @@ sna_video_sprite_show(struct sna *sna,
video->color_key_changed &= ~(1 << pipe);
}
+ if (video->colorspace_changed & (1 << pipe)) {
+ DBG(("%s: updating colorspace: %x\n",
+ __FUNCTION__, video->colorspace));
+
+ sna_crtc_set_sprite_colorspace(crtc, video->idx,
+ video->colorspace);
+
+ video->colorspace_changed &= ~(1 << pipe);
+ }
+
update_dst_box_to_crtc_coords(sna, crtc, dstBox);
if (frame->rotation & (RR_Rotate_90 | RR_Rotate_270)) {
int tmp = frame->width;
@@ -767,6 +784,8 @@ void sna_video_sprite_setup(struct sna *sna, ScreenPtr screen)
video->alignment = 64;
video->color_key = sna_video_sprite_color_key(sna);
video->color_key_changed = ~0;
+ video->colorspace = 1; /* BT.709 */
+ video->colorspace_changed = ~0;
video->has_color_key = true;
video->brightness = -19; /* (255/219) * -16 */
video->contrast = 75; /* 255/219 * 64 */
@@ -787,6 +806,7 @@ void sna_video_sprite_setup(struct sna *sna, ScreenPtr screen)
adaptor->base_id = adaptor->pPorts[0].id;
xvColorKey = MAKE_ATOM("XV_COLORKEY");
+ xvColorspace = MAKE_ATOM("XV_COLORSPACE");
xvAlwaysOnTop = MAKE_ATOM("XV_ALWAYS_ON_TOP");
xvSyncToVblank = MAKE_ATOM("XV_SYNC_TO_VBLANK");
--
2.13.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 27+ messages in thread
* ✗ Fi.CI.BAT: failure for sna: Add XV_COLORSPACE attribute support for sprite Xv adaptors
2017-06-08 20:33 [PATCH 0/5] drm/i915; Support for COLOR_ENCODING and COLOR_RANGE props ville.syrjala
` (5 preceding siblings ...)
2017-06-08 20:33 ` [PATCH xf86-video-intel] sna: Add XV_COLORSPACE attribute support for sprite Xv adaptors ville.syrjala
@ 2017-06-08 21:01 ` Patchwork
2017-06-20 14:08 ` ✗ Fi.CI.BAT: failure for sna: Add XV_COLORSPACE attribute support for sprite Xv adaptors (rev4) Patchwork
7 siblings, 0 replies; 27+ messages in thread
From: Patchwork @ 2017-06-08 21:01 UTC (permalink / raw)
To: ville.syrjala; +Cc: intel-gfx
== Series Details ==
Series: sna: Add XV_COLORSPACE attribute support for sprite Xv adaptors
URL : https://patchwork.freedesktop.org/series/25505/
State : failure
== Summary ==
CHK include/config/kernel.release
CHK include/generated/uapi/linux/version.h
CHK include/generated/utsrelease.h
CHK include/generated/bounds.h
CHK include/generated/timeconst.h
CHK include/generated/asm-offsets.h
CALL scripts/checksyscalls.sh
CHK scripts/mod/devicetable-offsets.h
CHK include/generated/compile.h
CHK kernel/config_data.h
CC [M] drivers/gpu/drm/i915/intel_display.o
drivers/gpu/drm/i915/intel_display.c: In function ‘skl_plane_ctl’:
drivers/gpu/drm/i915/intel_display.c:3325:24: error: ‘const struct drm_plane_state’ has no member named ‘color_encoding’
if (plane_state->base.color_encoding == DRM_COLOR_YCBCR_BT709)
^
drivers/gpu/drm/i915/intel_display.c:3325:43: error: ‘DRM_COLOR_YCBCR_BT709’ undeclared (first use in this function)
if (plane_state->base.color_encoding == DRM_COLOR_YCBCR_BT709)
^
drivers/gpu/drm/i915/intel_display.c:3325:43: note: each undeclared identifier is reported only once for each function it appears in
drivers/gpu/drm/i915/intel_display.c:3328:24: error: ‘const struct drm_plane_state’ has no member named ‘color_range’
if (plane_state->base.color_range == DRM_COLOR_YCBCR_FULL_RANGE)
^
drivers/gpu/drm/i915/intel_display.c:3328:40: error: ‘DRM_COLOR_YCBCR_FULL_RANGE’ undeclared (first use in this function)
if (plane_state->base.color_range == DRM_COLOR_YCBCR_FULL_RANGE)
^
drivers/gpu/drm/i915/intel_display.c: In function ‘glk_color_ctl’:
drivers/gpu/drm/i915/intel_display.c:3354:24: error: ‘const struct drm_plane_state’ has no member named ‘color_encoding’
if (plane_state->base.color_encoding == DRM_COLOR_YCBCR_BT709)
^
drivers/gpu/drm/i915/intel_display.c:3354:43: error: ‘DRM_COLOR_YCBCR_BT709’ undeclared (first use in this function)
if (plane_state->base.color_encoding == DRM_COLOR_YCBCR_BT709)
^
drivers/gpu/drm/i915/intel_display.c:3359:24: error: ‘const struct drm_plane_state’ has no member named ‘color_range’
if (plane_state->base.color_range == DRM_COLOR_YCBCR_FULL_RANGE)
^
drivers/gpu/drm/i915/intel_display.c:3359:40: error: ‘DRM_COLOR_YCBCR_FULL_RANGE’ undeclared (first use in this function)
if (plane_state->base.color_range == DRM_COLOR_YCBCR_FULL_RANGE)
^
drivers/gpu/drm/i915/intel_display.c: In function ‘intel_primary_plane_create’:
drivers/gpu/drm/i915/intel_display.c:13836:3: error: implicit declaration of function ‘drm_plane_create_color_properties’ [-Werror=implicit-function-declaration]
drm_plane_create_color_properties(&primary->base,
^
In file included from ./include/linux/kernel.h:10:0,
from ./include/linux/list.h:8,
from ./include/linux/dmi.h:4,
from drivers/gpu/drm/i915/intel_display.c:27:
drivers/gpu/drm/i915/intel_display.c:13837:13: error: ‘DRM_COLOR_YCBCR_BT601’ undeclared (first use in this function)
BIT(DRM_COLOR_YCBCR_BT601) |
^
./include/linux/bitops.h:6:28: note: in definition of macro ‘BIT’
#define BIT(nr) (1UL << (nr))
^
drivers/gpu/drm/i915/intel_display.c:13838:13: error: ‘DRM_COLOR_YCBCR_BT709’ undeclared (first use in this function)
BIT(DRM_COLOR_YCBCR_BT709),
^
./include/linux/bitops.h:6:28: note: in definition of macro ‘BIT’
#define BIT(nr) (1UL << (nr))
^
drivers/gpu/drm/i915/intel_display.c:13839:13: error: ‘DRM_COLOR_YCBCR_LIMITED_RANGE’ undeclared (first use in this function)
BIT(DRM_COLOR_YCBCR_LIMITED_RANGE) |
^
./include/linux/bitops.h:6:28: note: in definition of macro ‘BIT’
#define BIT(nr) (1UL << (nr))
^
drivers/gpu/drm/i915/intel_display.c:13840:13: error: ‘DRM_COLOR_YCBCR_FULL_RANGE’ undeclared (first use in this function)
BIT(DRM_COLOR_YCBCR_FULL_RANGE),
^
./include/linux/bitops.h:6:28: note: in definition of macro ‘BIT’
#define BIT(nr) (1UL << (nr))
^
cc1: all warnings being treated as errors
scripts/Makefile.build:302: recipe for target 'drivers/gpu/drm/i915/intel_display.o' failed
make[4]: *** [drivers/gpu/drm/i915/intel_display.o] Error 1
scripts/Makefile.build:561: recipe for target 'drivers/gpu/drm/i915' failed
make[3]: *** [drivers/gpu/drm/i915] Error 2
scripts/Makefile.build:561: recipe for target 'drivers/gpu/drm' failed
make[2]: *** [drivers/gpu/drm] Error 2
scripts/Makefile.build:561: recipe for target 'drivers/gpu' failed
make[1]: *** [drivers/gpu] Error 2
Makefile:1016: recipe for target 'drivers' failed
make: *** [drivers] Error 2
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 2/5] drm/i915: Fix plane YCbCr->RGB conversion for GLK
2017-06-08 20:33 ` [PATCH 2/5] drm/i915: Fix plane YCbCr->RGB conversion for GLK ville.syrjala
@ 2017-06-15 12:27 ` Sharma, Shashank
2017-06-15 12:42 ` Ville Syrjälä
0 siblings, 1 reply; 27+ messages in thread
From: Sharma, Shashank @ 2017-06-15 12:27 UTC (permalink / raw)
To: ville.syrjala, intel-gfx
Cc: Ander Conselvan de Oliveira, dhanya.p.r, Jyri Sarha
Regards
Shashank
On 6/9/2017 2:03 AM, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> On GLK the plane CSC controls moved into the COLOR_CTL register.
> Update the code to progam the YCbCr->RGB CSC mode correctly when
> faced with an YCbCr framebuffer.
>
> The spec is rather confusing as it calls the mode "YUV601 to RGB709".
> I'm going to assume that just means it's going to use the YCbCr->RGB
> matrix as specified in BT.601 and doesn't actually change the gamut.
>
> Cc: Jyri Sarha <jsarha@ti.com>
> Cc: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
> drivers/gpu/drm/i915/i915_reg.h | 5 +++++
> drivers/gpu/drm/i915/intel_display.c | 19 ++++++++++++++++---
> drivers/gpu/drm/i915/intel_drv.h | 2 ++
> drivers/gpu/drm/i915/intel_sprite.c | 13 +++++--------
> 4 files changed, 28 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index ce7c0dc79cf7..f4f51e7a3e83 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -5924,6 +5924,11 @@ enum {
> #define _PLANE_COLOR_CTL_3_A 0x703CC /* GLK+ */
> #define PLANE_COLOR_PIPE_GAMMA_ENABLE (1 << 30)
> #define PLANE_COLOR_PIPE_CSC_ENABLE (1 << 23)
> +#define PLANE_COLOR_CSC_MODE_BYPASS (0 << 17)
> +#define PLANE_COLOR_CSC_MODE_YUV601_TO_RGB709 (1 << 17)
> +#define PLANE_COLOR_CSC_MODE_YUV709_TO_RGB709 (2 << 17)
> +#define PLANE_COLOR_CSC_MODE_YUV2020_TO_RGB2020 (3 << 17)
> +#define PLANE_COLOR_CSC_MODE_RGB709_TO_RGB2020 (4 << 17)
> #define PLANE_COLOR_PLANE_GAMMA_DISABLE (1 << 13)
> #define _PLANE_BUF_CFG_1_A 0x7027c
> #define _PLANE_BUF_CFG_2_A 0x7037c
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 25390dd8e08e..e2aaaf60a969 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3335,6 +3335,21 @@ u32 skl_plane_ctl(const struct intel_crtc_state *crtc_state,
> return plane_ctl;
> }
>
> +u32 glk_color_ctl(const struct intel_plane_state *plane_state)
> +{
> + const struct drm_framebuffer *fb = plane_state->base.fb;
> + u32 color_ctl;
> +
> + color_ctl = PLANE_COLOR_PIPE_GAMMA_ENABLE |
> + 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;
why not YUV709_TO_RGB709 ?
> +
> + return color_ctl;
> +}
> +
You might want to have a look on this series from Dhanya too:
https://patchwork.freedesktop.org/series/17259/
- Shashank
> static void skylake_update_primary_plane(struct intel_plane *plane,
> const struct intel_crtc_state *crtc_state,
> const struct intel_plane_state *plane_state)
> @@ -3374,9 +3389,7 @@ static void skylake_update_primary_plane(struct intel_plane *plane,
>
> if (IS_GEMINILAKE(dev_priv)) {
> I915_WRITE_FW(PLANE_COLOR_CTL(pipe, plane_id),
> - PLANE_COLOR_PIPE_GAMMA_ENABLE |
> - PLANE_COLOR_PIPE_CSC_ENABLE |
> - PLANE_COLOR_PLANE_GAMMA_DISABLE);
> + glk_color_ctl(plane_state));
> }
>
> I915_WRITE_FW(PLANE_CTL(pipe, plane_id), plane_ctl);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 83dd40905821..e13e714b1176 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1484,6 +1484,7 @@ static inline u32 intel_plane_ggtt_offset(const struct intel_plane_state *state)
>
> u32 skl_plane_ctl(const struct intel_crtc_state *crtc_state,
> const struct intel_plane_state *plane_state);
> +u32 glk_color_ctl(const struct intel_plane_state *plane_state);
> u32 skl_plane_stride(const struct drm_framebuffer *fb, int plane,
> unsigned int rotation);
> int skl_check_plane_surface(struct intel_plane_state *plane_state);
> @@ -1888,6 +1889,7 @@ bool intel_sdvo_init(struct drm_i915_private *dev_priv,
>
>
> /* intel_sprite.c */
> +bool intel_format_is_yuv(u32 format);
> int intel_usecs_to_scanlines(const struct drm_display_mode *adjusted_mode,
> int usecs);
> struct intel_plane *intel_sprite_plane_create(struct drm_i915_private *dev_priv,
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 2ce3b3c6ffa8..cc07157f2eb6 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -40,8 +40,7 @@
> #include <drm/i915_drm.h>
> #include "i915_drv.h"
>
> -static bool
> -format_is_yuv(uint32_t format)
> +bool intel_format_is_yuv(u32 format)
> {
> switch (format) {
> case DRM_FORMAT_YUYV:
> @@ -264,9 +263,7 @@ skl_update_plane(struct intel_plane *plane,
>
> if (IS_GEMINILAKE(dev_priv)) {
> I915_WRITE_FW(PLANE_COLOR_CTL(pipe, plane_id),
> - PLANE_COLOR_PIPE_GAMMA_ENABLE |
> - PLANE_COLOR_PIPE_CSC_ENABLE |
> - PLANE_COLOR_PLANE_GAMMA_DISABLE);
> + glk_color_ctl(plane_state));
> }
>
> if (key->flags) {
> @@ -333,7 +330,7 @@ chv_update_csc(const struct intel_plane_state *plane_state)
> enum plane_id plane_id = plane->id;
>
> /* Seems RGB data bypasses the CSC always */
> - if (!format_is_yuv(fb->format->format))
> + if (!intel_format_is_yuv(fb->format->format))
> return;
>
> /*
> @@ -375,7 +372,7 @@ vlv_update_clrc(const struct intel_plane_state *plane_state)
> enum plane_id plane_id = plane->id;
> int con, bri, sh_sin, sh_cos;
>
> - if (format_is_yuv(fb->format->format)) {
> + if (intel_format_is_yuv(fb->format->format)) {
> /*
> * expand limited range to full range.
> * contrast is applied first, then brightness
> @@ -933,7 +930,7 @@ intel_check_sprite_plane(struct intel_plane *plane,
> src_y = src->y1 >> 16;
> src_h = drm_rect_height(src) >> 16;
>
> - if (format_is_yuv(fb->format->format)) {
> + if (intel_format_is_yuv(fb->format->format)) {
> src_x &= ~1;
> src_w &= ~1;
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 1/5] drm/i915: Correctly handle limited range YCbCr data on VLV/CHV
2017-06-08 20:33 ` [PATCH v2 1/5] drm/i915: Correctly handle limited range YCbCr data on VLV/CHV ville.syrjala
@ 2017-06-15 12:38 ` Sharma, Shashank
2017-06-15 12:46 ` Ville Syrjälä
2017-06-20 13:32 ` [PATCH v3 " ville.syrjala
1 sibling, 1 reply; 27+ messages in thread
From: Sharma, Shashank @ 2017-06-15 12:38 UTC (permalink / raw)
To: ville.syrjala, intel-gfx; +Cc: Jyri Sarha, Tang, Jun, stable
Regards
Shashank
On 6/9/2017 2:03 AM, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Turns out the VLV/CHV fixed function sprite CSC expects full range
> data as input. We've been feeding it limited range data to it all
> along. To expand the data out to full range we'll use the color
> correction registers (brightness, contrast, and saturation).
>
> On CHV pipe B we were actually doing the right thing already because we
> progammed the custom CSC matrix to do expect limited range input. Now
> that well pre-expand the data out with the color correction unit, we
> need to change the CSC matrix to operate with full range input instead.
>
> This should make the sprite output of the other pipes match the sprite
> output of pipe B reasonably well. Looking at the resulting pipe CRCs,
> there can be a slight difference in the output, but as I don't know
> the formula used by the fixed function CSC of the other pipes, I don't
> think it's worth the effort to try to match the output exactly. It
> might not even be possible due to difference in internal precision etc.
>
> One slight caveat here is that the color correction registers are single
> bufferred, so we should really be updating them during vblank, but we
> still don't have a mechanism for that, so just toss in another FIXME.
>
> v2: Rebase
>
> Cc: Jyri Sarha <jsarha@ti.com>
> Cc: "Tang, Jun" <jun.tang@intel.com>
> Reported-by: "Tang, Jun" <jun.tang@intel.com>
> Cc: stable@vger.kernel.org
> Fixes: 7f1f3851feb0 ("drm/i915: sprite support for ValleyView v4")
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
> drivers/gpu/drm/i915/i915_reg.h | 10 +++++
> drivers/gpu/drm/i915/intel_sprite.c | 74 ++++++++++++++++++++++++++++---------
> 2 files changed, 66 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index b6d69e289974..ce7c0dc79cf7 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -5777,6 +5777,12 @@ enum {
> #define _SPATILEOFF (VLV_DISPLAY_BASE + 0x721a4)
> #define _SPACONSTALPHA (VLV_DISPLAY_BASE + 0x721a8)
> #define SP_CONST_ALPHA_ENABLE (1<<31)
> +#define _SPACLRC0 (VLV_DISPLAY_BASE + 0x721d0)
> +#define SP_CONTRAST(x) ((x) << 18) /* u3.6 */
> +#define SP_BRIGHTNESS(x) ((x) & 0xff) /* s8 */
> +#define _SPACLRC1 (VLV_DISPLAY_BASE + 0x721d4)
> +#define SP_SH_SIN(x) (((x) & 0x7ff) << 16) /* s4.7 */
> +#define SP_SH_COS(x) (x) /* u3.7 */
> #define _SPAGAMC (VLV_DISPLAY_BASE + 0x721f4)
>
> #define _SPBCNTR (VLV_DISPLAY_BASE + 0x72280)
> @@ -5790,6 +5796,8 @@ enum {
> #define _SPBKEYMAXVAL (VLV_DISPLAY_BASE + 0x722a0)
> #define _SPBTILEOFF (VLV_DISPLAY_BASE + 0x722a4)
> #define _SPBCONSTALPHA (VLV_DISPLAY_BASE + 0x722a8)
> +#define _SPBCLRC0 (VLV_DISPLAY_BASE + 0x722d0)
> +#define _SPBCLRC1 (VLV_DISPLAY_BASE + 0x722d4)
> #define _SPBGAMC (VLV_DISPLAY_BASE + 0x722f4)
>
> #define _MMIO_VLV_SPR(pipe, plane_id, reg_a, reg_b) \
> @@ -5806,6 +5814,8 @@ enum {
> #define SPKEYMAXVAL(pipe, plane_id) _MMIO_VLV_SPR((pipe), (plane_id), _SPAKEYMAXVAL, _SPBKEYMAXVAL)
> #define SPTILEOFF(pipe, plane_id) _MMIO_VLV_SPR((pipe), (plane_id), _SPATILEOFF, _SPBTILEOFF)
> #define SPCONSTALPHA(pipe, plane_id) _MMIO_VLV_SPR((pipe), (plane_id), _SPACONSTALPHA, _SPBCONSTALPHA)
> +#define SPCLRC0(pipe, plane_id) _MMIO_VLV_SPR((pipe), (plane_id), _SPACLRC0, _SPBCLRC0)
> +#define SPCLRC1(pipe, plane_id) _MMIO_VLV_SPR((pipe), (plane_id), _SPACLRC1, _SPBCLRC1)
> #define SPGAMC(pipe, plane_id) _MMIO_VLV_SPR((pipe), (plane_id), _SPAGAMC, _SPBGAMC)
>
> /*
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 0c650c2cbca8..2ce3b3c6ffa8 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -325,44 +325,80 @@ skl_disable_plane(struct intel_plane *plane, struct intel_crtc *crtc)
> }
>
> static void
> -chv_update_csc(struct intel_plane *plane, uint32_t format)
> +chv_update_csc(const struct intel_plane_state *plane_state)
> {
> + struct intel_plane *plane = to_intel_plane(plane_state->base.plane);
> 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;
>
> /* Seems RGB data bypasses the CSC always */
> - if (!format_is_yuv(format))
> + if (!format_is_yuv(fb->format->format))
> return;
>
> /*
> - * BT.601 limited range YCbCr -> full range RGB
> + * BT.601 full range YCbCr -> full range RGB
> *
> - * |r| | 6537 4769 0| |cr |
> - * |g| = |-3330 4769 -1605| x |y-64|
> - * |b| | 0 4769 8263| |cb |
> + * |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, so no
> - * need for any offset. For Y we need to remove the offset.
> + * 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(-64));
> + 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(4769) | SPCSC_C0(6537));
> - I915_WRITE_FW(SPCSCC23(plane_id), SPCSC_C1(-3330) | SPCSC_C0(0));
> - I915_WRITE_FW(SPCSCC45(plane_id), SPCSC_C1(-1605) | SPCSC_C0(4769));
> - I915_WRITE_FW(SPCSCC67(plane_id), SPCSC_C1(4769) | SPCSC_C0(0));
> - I915_WRITE_FW(SPCSCC8(plane_id), SPCSC_C0(8263));
> + 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(SPCSCYGICLAMP(plane_id), SPCSC_IMAX(940) | SPCSC_IMIN(64));
> - I915_WRITE_FW(SPCSCCBICLAMP(plane_id), SPCSC_IMAX(448) | SPCSC_IMIN(-448));
> - I915_WRITE_FW(SPCSCCRICLAMP(plane_id), SPCSC_IMAX(448) | SPCSC_IMIN(-448));
> + I915_WRITE_FW(SPCSCYGICLAMP(plane_id), SPCSC_IMAX(1023) | SPCSC_IMIN(0));
> + I915_WRITE_FW(SPCSCCBICLAMP(plane_id), SPCSC_IMAX(512) | SPCSC_IMIN(-512));
> + I915_WRITE_FW(SPCSCCRICLAMP(plane_id), SPCSC_IMAX(512) | SPCSC_IMIN(-512));
>
> I915_WRITE_FW(SPCSCYGOCLAMP(plane_id), SPCSC_OMAX(1023) | SPCSC_OMIN(0));
> I915_WRITE_FW(SPCSCCBOCLAMP(plane_id), SPCSC_OMAX(1023) | SPCSC_OMIN(0));
> I915_WRITE_FW(SPCSCCROCLAMP(plane_id), SPCSC_OMAX(1023) | SPCSC_OMIN(0));
> }
>
> +static void
> +vlv_update_clrc(const struct intel_plane_state *plane_state)
> +{
> + struct intel_plane *plane = to_intel_plane(plane_state->base.plane);
> + struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> + const struct drm_framebuffer *fb = plane_state->base.fb;
> + enum pipe pipe = plane->pipe;
> + enum plane_id plane_id = plane->id;
> + int con, bri, sh_sin, sh_cos;
> +
con = contrast bri = brightness for better reading ?
> + if (format_is_yuv(fb->format->format)) {
> + /*
> + * expand limited range to full range.
> + * contrast is applied first, then brightness
> + */
> + con = ((255 << 7) / 219 + 1) >> 1;
> + bri = -((16 << 1) * 255 / 219 + 1) >> 1;
> + sh_sin = 0;
> + sh_cos = (((128 << 8) / 112) + 1) >> 1;
> + } else {
> + /* pass-through everything */
> + con = 1 << 6;
> + bri = 0;
> + sh_sin = 0;
> + sh_cos = 1 << 7;
> + }
contrast and brightness are mostly used for color / display correction.
Would it be better to create a
plane level property for the same, and attach into color management
framework ? In this way, userspace
can also play around.
- Shashank
> +
> + /* FIXME these register are single buffered :( */
> + I915_WRITE_FW(SPCLRC0(pipe, plane_id),
> + SP_CONTRAST(con) | SP_BRIGHTNESS(bri));
> + I915_WRITE_FW(SPCLRC1(pipe, plane_id),
> + SP_SH_SIN(sh_sin) | SP_SH_COS(sh_cos));
> +}
> +
> static u32 vlv_sprite_ctl(const struct intel_crtc_state *crtc_state,
> const struct intel_plane_state *plane_state)
> {
> @@ -456,8 +492,10 @@ vlv_update_plane(struct intel_plane *plane,
>
> spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
>
> + vlv_update_clrc(plane_state);
> +
> if (IS_CHERRYVIEW(dev_priv) && pipe == PIPE_B)
> - chv_update_csc(plane, fb->format->format);
> + chv_update_csc(plane_state);
>
> if (key->flags) {
> I915_WRITE_FW(SPKEYMINVAL(pipe, plane_id), key->min_value);
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 2/5] drm/i915: Fix plane YCbCr->RGB conversion for GLK
2017-06-15 12:27 ` Sharma, Shashank
@ 2017-06-15 12:42 ` Ville Syrjälä
2017-06-15 13:28 ` Sharma, Shashank
0 siblings, 1 reply; 27+ messages in thread
From: Ville Syrjälä @ 2017-06-15 12:42 UTC (permalink / raw)
To: Sharma, Shashank
Cc: Ander Conselvan de Oliveira, intel-gfx, dhanya.p.r, Jyri Sarha
On Thu, Jun 15, 2017 at 05:57:21PM +0530, Sharma, Shashank wrote:
>
> Regards
> Shashank
> On 6/9/2017 2:03 AM, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > On GLK the plane CSC controls moved into the COLOR_CTL register.
> > Update the code to progam the YCbCr->RGB CSC mode correctly when
> > faced with an YCbCr framebuffer.
> >
> > The spec is rather confusing as it calls the mode "YUV601 to RGB709".
> > I'm going to assume that just means it's going to use the YCbCr->RGB
> > matrix as specified in BT.601 and doesn't actually change the gamut.
> >
> > Cc: Jyri Sarha <jsarha@ti.com>
> > Cc: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> > drivers/gpu/drm/i915/i915_reg.h | 5 +++++
> > drivers/gpu/drm/i915/intel_display.c | 19 ++++++++++++++++---
> > drivers/gpu/drm/i915/intel_drv.h | 2 ++
> > drivers/gpu/drm/i915/intel_sprite.c | 13 +++++--------
> > 4 files changed, 28 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index ce7c0dc79cf7..f4f51e7a3e83 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -5924,6 +5924,11 @@ enum {
> > #define _PLANE_COLOR_CTL_3_A 0x703CC /* GLK+ */
> > #define PLANE_COLOR_PIPE_GAMMA_ENABLE (1 << 30)
> > #define PLANE_COLOR_PIPE_CSC_ENABLE (1 << 23)
> > +#define PLANE_COLOR_CSC_MODE_BYPASS (0 << 17)
> > +#define PLANE_COLOR_CSC_MODE_YUV601_TO_RGB709 (1 << 17)
> > +#define PLANE_COLOR_CSC_MODE_YUV709_TO_RGB709 (2 << 17)
> > +#define PLANE_COLOR_CSC_MODE_YUV2020_TO_RGB2020 (3 << 17)
> > +#define PLANE_COLOR_CSC_MODE_RGB709_TO_RGB2020 (4 << 17)
> > #define PLANE_COLOR_PLANE_GAMMA_DISABLE (1 << 13)
> > #define _PLANE_BUF_CFG_1_A 0x7027c
> > #define _PLANE_BUF_CFG_2_A 0x7037c
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 25390dd8e08e..e2aaaf60a969 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -3335,6 +3335,21 @@ u32 skl_plane_ctl(const struct intel_crtc_state *crtc_state,
> > return plane_ctl;
> > }
> >
> > +u32 glk_color_ctl(const struct intel_plane_state *plane_state)
> > +{
> > + const struct drm_framebuffer *fb = plane_state->base.fb;
> > + u32 color_ctl;
> > +
> > + color_ctl = PLANE_COLOR_PIPE_GAMMA_ENABLE |
> > + 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;
> why not YUV709_TO_RGB709 ?
To match the other platforms.
> > +
> > + return color_ctl;
> > +}
> > +
>
> You might want to have a look on this series from Dhanya too:
>
> https://patchwork.freedesktop.org/series/17259/
I think we have everything we need from there.
>
> - Shashank
>
> > static void skylake_update_primary_plane(struct intel_plane *plane,
> > const struct intel_crtc_state *crtc_state,
> > const struct intel_plane_state *plane_state)
> > @@ -3374,9 +3389,7 @@ static void skylake_update_primary_plane(struct intel_plane *plane,
> >
> > if (IS_GEMINILAKE(dev_priv)) {
> > I915_WRITE_FW(PLANE_COLOR_CTL(pipe, plane_id),
> > - PLANE_COLOR_PIPE_GAMMA_ENABLE |
> > - PLANE_COLOR_PIPE_CSC_ENABLE |
> > - PLANE_COLOR_PLANE_GAMMA_DISABLE);
> > + glk_color_ctl(plane_state));
> > }
> >
> > I915_WRITE_FW(PLANE_CTL(pipe, plane_id), plane_ctl);
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 83dd40905821..e13e714b1176 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1484,6 +1484,7 @@ static inline u32 intel_plane_ggtt_offset(const struct intel_plane_state *state)
> >
> > u32 skl_plane_ctl(const struct intel_crtc_state *crtc_state,
> > const struct intel_plane_state *plane_state);
> > +u32 glk_color_ctl(const struct intel_plane_state *plane_state);
> > u32 skl_plane_stride(const struct drm_framebuffer *fb, int plane,
> > unsigned int rotation);
> > int skl_check_plane_surface(struct intel_plane_state *plane_state);
> > @@ -1888,6 +1889,7 @@ bool intel_sdvo_init(struct drm_i915_private *dev_priv,
> >
> >
> > /* intel_sprite.c */
> > +bool intel_format_is_yuv(u32 format);
> > int intel_usecs_to_scanlines(const struct drm_display_mode *adjusted_mode,
> > int usecs);
> > struct intel_plane *intel_sprite_plane_create(struct drm_i915_private *dev_priv,
> > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> > index 2ce3b3c6ffa8..cc07157f2eb6 100644
> > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > @@ -40,8 +40,7 @@
> > #include <drm/i915_drm.h>
> > #include "i915_drv.h"
> >
> > -static bool
> > -format_is_yuv(uint32_t format)
> > +bool intel_format_is_yuv(u32 format)
> > {
> > switch (format) {
> > case DRM_FORMAT_YUYV:
> > @@ -264,9 +263,7 @@ skl_update_plane(struct intel_plane *plane,
> >
> > if (IS_GEMINILAKE(dev_priv)) {
> > I915_WRITE_FW(PLANE_COLOR_CTL(pipe, plane_id),
> > - PLANE_COLOR_PIPE_GAMMA_ENABLE |
> > - PLANE_COLOR_PIPE_CSC_ENABLE |
> > - PLANE_COLOR_PLANE_GAMMA_DISABLE);
> > + glk_color_ctl(plane_state));
> > }
> >
> > if (key->flags) {
> > @@ -333,7 +330,7 @@ chv_update_csc(const struct intel_plane_state *plane_state)
> > enum plane_id plane_id = plane->id;
> >
> > /* Seems RGB data bypasses the CSC always */
> > - if (!format_is_yuv(fb->format->format))
> > + if (!intel_format_is_yuv(fb->format->format))
> > return;
> >
> > /*
> > @@ -375,7 +372,7 @@ vlv_update_clrc(const struct intel_plane_state *plane_state)
> > enum plane_id plane_id = plane->id;
> > int con, bri, sh_sin, sh_cos;
> >
> > - if (format_is_yuv(fb->format->format)) {
> > + if (intel_format_is_yuv(fb->format->format)) {
> > /*
> > * expand limited range to full range.
> > * contrast is applied first, then brightness
> > @@ -933,7 +930,7 @@ intel_check_sprite_plane(struct intel_plane *plane,
> > src_y = src->y1 >> 16;
> > src_h = drm_rect_height(src) >> 16;
> >
> > - if (format_is_yuv(fb->format->format)) {
> > + if (intel_format_is_yuv(fb->format->format)) {
> > src_x &= ~1;
> > src_w &= ~1;
> >
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 1/5] drm/i915: Correctly handle limited range YCbCr data on VLV/CHV
2017-06-15 12:38 ` Sharma, Shashank
@ 2017-06-15 12:46 ` Ville Syrjälä
0 siblings, 0 replies; 27+ messages in thread
From: Ville Syrjälä @ 2017-06-15 12:46 UTC (permalink / raw)
To: Sharma, Shashank; +Cc: intel-gfx, Jyri Sarha, Tang, Jun, stable
On Thu, Jun 15, 2017 at 06:08:43PM +0530, Sharma, Shashank wrote:
> Regards
>
> Shashank
>
>
> On 6/9/2017 2:03 AM, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Turns out the VLV/CHV fixed function sprite CSC expects full range
> > data as input. We've been feeding it limited range data to it all
> > along. To expand the data out to full range we'll use the color
> > correction registers (brightness, contrast, and saturation).
> >
> > On CHV pipe B we were actually doing the right thing already because we
> > progammed the custom CSC matrix to do expect limited range input. Now
> > that well pre-expand the data out with the color correction unit, we
> > need to change the CSC matrix to operate with full range input instead.
> >
> > This should make the sprite output of the other pipes match the sprite
> > output of pipe B reasonably well. Looking at the resulting pipe CRCs,
> > there can be a slight difference in the output, but as I don't know
> > the formula used by the fixed function CSC of the other pipes, I don't
> > think it's worth the effort to try to match the output exactly. It
> > might not even be possible due to difference in internal precision etc.
> >
> > One slight caveat here is that the color correction registers are single
> > bufferred, so we should really be updating them during vblank, but we
> > still don't have a mechanism for that, so just toss in another FIXME.
> >
> > v2: Rebase
> >
> > Cc: Jyri Sarha <jsarha@ti.com>
> > Cc: "Tang, Jun" <jun.tang@intel.com>
> > Reported-by: "Tang, Jun" <jun.tang@intel.com>
> > Cc: stable@vger.kernel.org
> > Fixes: 7f1f3851feb0 ("drm/i915: sprite support for ValleyView v4")
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> > drivers/gpu/drm/i915/i915_reg.h | 10 +++++
> > drivers/gpu/drm/i915/intel_sprite.c | 74 ++++++++++++++++++++++++++++---------
> > 2 files changed, 66 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index b6d69e289974..ce7c0dc79cf7 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -5777,6 +5777,12 @@ enum {
> > #define _SPATILEOFF (VLV_DISPLAY_BASE + 0x721a4)
> > #define _SPACONSTALPHA (VLV_DISPLAY_BASE + 0x721a8)
> > #define SP_CONST_ALPHA_ENABLE (1<<31)
> > +#define _SPACLRC0 (VLV_DISPLAY_BASE + 0x721d0)
> > +#define SP_CONTRAST(x) ((x) << 18) /* u3.6 */
> > +#define SP_BRIGHTNESS(x) ((x) & 0xff) /* s8 */
> > +#define _SPACLRC1 (VLV_DISPLAY_BASE + 0x721d4)
> > +#define SP_SH_SIN(x) (((x) & 0x7ff) << 16) /* s4.7 */
> > +#define SP_SH_COS(x) (x) /* u3.7 */
> > #define _SPAGAMC (VLV_DISPLAY_BASE + 0x721f4)
> >
> > #define _SPBCNTR (VLV_DISPLAY_BASE + 0x72280)
> > @@ -5790,6 +5796,8 @@ enum {
> > #define _SPBKEYMAXVAL (VLV_DISPLAY_BASE + 0x722a0)
> > #define _SPBTILEOFF (VLV_DISPLAY_BASE + 0x722a4)
> > #define _SPBCONSTALPHA (VLV_DISPLAY_BASE + 0x722a8)
> > +#define _SPBCLRC0 (VLV_DISPLAY_BASE + 0x722d0)
> > +#define _SPBCLRC1 (VLV_DISPLAY_BASE + 0x722d4)
> > #define _SPBGAMC (VLV_DISPLAY_BASE + 0x722f4)
> >
> > #define _MMIO_VLV_SPR(pipe, plane_id, reg_a, reg_b) \
> > @@ -5806,6 +5814,8 @@ enum {
> > #define SPKEYMAXVAL(pipe, plane_id) _MMIO_VLV_SPR((pipe), (plane_id), _SPAKEYMAXVAL, _SPBKEYMAXVAL)
> > #define SPTILEOFF(pipe, plane_id) _MMIO_VLV_SPR((pipe), (plane_id), _SPATILEOFF, _SPBTILEOFF)
> > #define SPCONSTALPHA(pipe, plane_id) _MMIO_VLV_SPR((pipe), (plane_id), _SPACONSTALPHA, _SPBCONSTALPHA)
> > +#define SPCLRC0(pipe, plane_id) _MMIO_VLV_SPR((pipe), (plane_id), _SPACLRC0, _SPBCLRC0)
> > +#define SPCLRC1(pipe, plane_id) _MMIO_VLV_SPR((pipe), (plane_id), _SPACLRC1, _SPBCLRC1)
> > #define SPGAMC(pipe, plane_id) _MMIO_VLV_SPR((pipe), (plane_id), _SPAGAMC, _SPBGAMC)
> >
> > /*
> > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> > index 0c650c2cbca8..2ce3b3c6ffa8 100644
> > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > @@ -325,44 +325,80 @@ skl_disable_plane(struct intel_plane *plane, struct intel_crtc *crtc)
> > }
> >
> > static void
> > -chv_update_csc(struct intel_plane *plane, uint32_t format)
> > +chv_update_csc(const struct intel_plane_state *plane_state)
> > {
> > + struct intel_plane *plane = to_intel_plane(plane_state->base.plane);
> > 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;
> >
> > /* Seems RGB data bypasses the CSC always */
> > - if (!format_is_yuv(format))
> > + if (!format_is_yuv(fb->format->format))
> > return;
> >
> > /*
> > - * BT.601 limited range YCbCr -> full range RGB
> > + * BT.601 full range YCbCr -> full range RGB
> > *
> > - * |r| | 6537 4769 0| |cr |
> > - * |g| = |-3330 4769 -1605| x |y-64|
> > - * |b| | 0 4769 8263| |cb |
> > + * |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, so no
> > - * need for any offset. For Y we need to remove the offset.
> > + * 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(-64));
> > + 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(4769) | SPCSC_C0(6537));
> > - I915_WRITE_FW(SPCSCC23(plane_id), SPCSC_C1(-3330) | SPCSC_C0(0));
> > - I915_WRITE_FW(SPCSCC45(plane_id), SPCSC_C1(-1605) | SPCSC_C0(4769));
> > - I915_WRITE_FW(SPCSCC67(plane_id), SPCSC_C1(4769) | SPCSC_C0(0));
> > - I915_WRITE_FW(SPCSCC8(plane_id), SPCSC_C0(8263));
> > + 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(SPCSCYGICLAMP(plane_id), SPCSC_IMAX(940) | SPCSC_IMIN(64));
> > - I915_WRITE_FW(SPCSCCBICLAMP(plane_id), SPCSC_IMAX(448) | SPCSC_IMIN(-448));
> > - I915_WRITE_FW(SPCSCCRICLAMP(plane_id), SPCSC_IMAX(448) | SPCSC_IMIN(-448));
> > + I915_WRITE_FW(SPCSCYGICLAMP(plane_id), SPCSC_IMAX(1023) | SPCSC_IMIN(0));
> > + I915_WRITE_FW(SPCSCCBICLAMP(plane_id), SPCSC_IMAX(512) | SPCSC_IMIN(-512));
> > + I915_WRITE_FW(SPCSCCRICLAMP(plane_id), SPCSC_IMAX(512) | SPCSC_IMIN(-512));
> >
> > I915_WRITE_FW(SPCSCYGOCLAMP(plane_id), SPCSC_OMAX(1023) | SPCSC_OMIN(0));
> > I915_WRITE_FW(SPCSCCBOCLAMP(plane_id), SPCSC_OMAX(1023) | SPCSC_OMIN(0));
> > I915_WRITE_FW(SPCSCCROCLAMP(plane_id), SPCSC_OMAX(1023) | SPCSC_OMIN(0));
> > }
> >
> > +static void
> > +vlv_update_clrc(const struct intel_plane_state *plane_state)
> > +{
> > + struct intel_plane *plane = to_intel_plane(plane_state->base.plane);
> > + struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> > + const struct drm_framebuffer *fb = plane_state->base.fb;
> > + enum pipe pipe = plane->pipe;
> > + enum plane_id plane_id = plane->id;
> > + int con, bri, sh_sin, sh_cos;
> > +
> con = contrast bri = brightness for better reading ?
Sure, why not.
> > + if (format_is_yuv(fb->format->format)) {
> > + /*
> > + * expand limited range to full range.
> > + * contrast is applied first, then brightness
> > + */
> > + con = ((255 << 7) / 219 + 1) >> 1;
> > + bri = -((16 << 1) * 255 / 219 + 1) >> 1;
> > + sh_sin = 0;
> > + sh_cos = (((128 << 8) / 112) + 1) >> 1;
> > + } else {
> > + /* pass-through everything */
> > + con = 1 << 6;
> > + bri = 0;
> > + sh_sin = 0;
> > + sh_cos = 1 << 7;
> > + }
> contrast and brightness are mostly used for color / display correction.
> Would it be better to create a
> plane level property for the same, and attach into color management
> framework ? In this way, userspace
> can also play around.
I did have such plans long ago, but I'm not sure there's much point in
doing that since we would only be able to support these propeerties
on a very limited subset of planes on a very limited subset of platforms.
>
> - Shashank
> > +
> > + /* FIXME these register are single buffered :( */
> > + I915_WRITE_FW(SPCLRC0(pipe, plane_id),
> > + SP_CONTRAST(con) | SP_BRIGHTNESS(bri));
> > + I915_WRITE_FW(SPCLRC1(pipe, plane_id),
> > + SP_SH_SIN(sh_sin) | SP_SH_COS(sh_cos));
> > +}
> > +
> > static u32 vlv_sprite_ctl(const struct intel_crtc_state *crtc_state,
> > const struct intel_plane_state *plane_state)
> > {
> > @@ -456,8 +492,10 @@ vlv_update_plane(struct intel_plane *plane,
> >
> > spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
> >
> > + vlv_update_clrc(plane_state);
> > +
> > if (IS_CHERRYVIEW(dev_priv) && pipe == PIPE_B)
> > - chv_update_csc(plane, fb->format->format);
> > + chv_update_csc(plane_state);
> >
> > if (key->flags) {
> > I915_WRITE_FW(SPKEYMINVAL(pipe, plane_id), key->min_value);
--
Ville Syrjälä
Intel OTC
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 3/5] drm/i915: Add support for the YCbCr COLOR_ENCODING property
2017-06-08 20:33 ` [PATCH 3/5] drm/i915: Add support for the YCbCr COLOR_ENCODING property ville.syrjala
@ 2017-06-15 13:22 ` Sharma, Shashank
2017-06-16 13:02 ` Ville Syrjälä
2017-06-20 13:33 ` [PATCH v2 " ville.syrjala
1 sibling, 1 reply; 27+ messages in thread
From: Sharma, Shashank @ 2017-06-15 13:22 UTC (permalink / raw)
To: ville.syrjala, intel-gfx; +Cc: Jyri Sarha
Regards
Shashank
On 6/9/2017 2:03 AM, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> 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@ti.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
> 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.
> #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 ?
- 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;
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 2/5] drm/i915: Fix plane YCbCr->RGB conversion for GLK
2017-06-15 12:42 ` Ville Syrjälä
@ 2017-06-15 13:28 ` Sharma, Shashank
2017-06-16 13:03 ` Ville Syrjälä
0 siblings, 1 reply; 27+ messages in thread
From: Sharma, Shashank @ 2017-06-15 13:28 UTC (permalink / raw)
To: Ville Syrjälä
Cc: Ander Conselvan de Oliveira, intel-gfx, dhanya.p.r, Jyri Sarha
Regards
Shashank
On 6/15/2017 6:12 PM, Ville Syrjälä wrote:
> On Thu, Jun 15, 2017 at 05:57:21PM +0530, Sharma, Shashank wrote:
>> Regards
>> Shashank
>> On 6/9/2017 2:03 AM, ville.syrjala@linux.intel.com wrote:
>>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>
>>> On GLK the plane CSC controls moved into the COLOR_CTL register.
>>> Update the code to progam the YCbCr->RGB CSC mode correctly when
>>> faced with an YCbCr framebuffer.
>>>
>>> The spec is rather confusing as it calls the mode "YUV601 to RGB709".
>>> I'm going to assume that just means it's going to use the YCbCr->RGB
>>> matrix as specified in BT.601 and doesn't actually change the gamut.
>>>
>>> Cc: Jyri Sarha <jsarha@ti.com>
>>> Cc: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
>>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>> ---
>>> drivers/gpu/drm/i915/i915_reg.h | 5 +++++
>>> drivers/gpu/drm/i915/intel_display.c | 19 ++++++++++++++++---
>>> drivers/gpu/drm/i915/intel_drv.h | 2 ++
>>> drivers/gpu/drm/i915/intel_sprite.c | 13 +++++--------
>>> 4 files changed, 28 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>>> index ce7c0dc79cf7..f4f51e7a3e83 100644
>>> --- a/drivers/gpu/drm/i915/i915_reg.h
>>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>>> @@ -5924,6 +5924,11 @@ enum {
>>> #define _PLANE_COLOR_CTL_3_A 0x703CC /* GLK+ */
>>> #define PLANE_COLOR_PIPE_GAMMA_ENABLE (1 << 30)
>>> #define PLANE_COLOR_PIPE_CSC_ENABLE (1 << 23)
>>> +#define PLANE_COLOR_CSC_MODE_BYPASS (0 << 17)
>>> +#define PLANE_COLOR_CSC_MODE_YUV601_TO_RGB709 (1 << 17)
>>> +#define PLANE_COLOR_CSC_MODE_YUV709_TO_RGB709 (2 << 17)
>>> +#define PLANE_COLOR_CSC_MODE_YUV2020_TO_RGB2020 (3 << 17)
>>> +#define PLANE_COLOR_CSC_MODE_RGB709_TO_RGB2020 (4 << 17)
>>> #define PLANE_COLOR_PLANE_GAMMA_DISABLE (1 << 13)
>>> #define _PLANE_BUF_CFG_1_A 0x7027c
>>> #define _PLANE_BUF_CFG_2_A 0x7037c
>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>>> index 25390dd8e08e..e2aaaf60a969 100644
>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>> @@ -3335,6 +3335,21 @@ u32 skl_plane_ctl(const struct intel_crtc_state *crtc_state,
>>> return plane_ctl;
>>> }
>>>
>>> +u32 glk_color_ctl(const struct intel_plane_state *plane_state)
>>> +{
>>> + const struct drm_framebuffer *fb = plane_state->base.fb;
>>> + u32 color_ctl;
>>> +
>>> + color_ctl = PLANE_COLOR_PIPE_GAMMA_ENABLE |
>>> + 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;
>> why not YUV709_TO_RGB709 ?
> To match the other platforms.
I am slightly confused. Dint we just upgrade our CSC matrix from stone
age, to REC_709, in patch 4 ?
Shouldn't we be consistent here ? or am I missing something basic :-) ?
- Shashank
>>> +
>>> + return color_ctl;
>>> +}
>>> +
>> You might want to have a look on this series from Dhanya too:
>>
>> https://patchwork.freedesktop.org/series/17259/
> I think we have everything we need from there.
>
>> - Shashank
>>
>>> static void skylake_update_primary_plane(struct intel_plane *plane,
>>> const struct intel_crtc_state *crtc_state,
>>> const struct intel_plane_state *plane_state)
>>> @@ -3374,9 +3389,7 @@ static void skylake_update_primary_plane(struct intel_plane *plane,
>>>
>>> if (IS_GEMINILAKE(dev_priv)) {
>>> I915_WRITE_FW(PLANE_COLOR_CTL(pipe, plane_id),
>>> - PLANE_COLOR_PIPE_GAMMA_ENABLE |
>>> - PLANE_COLOR_PIPE_CSC_ENABLE |
>>> - PLANE_COLOR_PLANE_GAMMA_DISABLE);
>>> + glk_color_ctl(plane_state));
>>> }
>>>
>>> I915_WRITE_FW(PLANE_CTL(pipe, plane_id), plane_ctl);
>>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>>> index 83dd40905821..e13e714b1176 100644
>>> --- a/drivers/gpu/drm/i915/intel_drv.h
>>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>>> @@ -1484,6 +1484,7 @@ static inline u32 intel_plane_ggtt_offset(const struct intel_plane_state *state)
>>>
>>> u32 skl_plane_ctl(const struct intel_crtc_state *crtc_state,
>>> const struct intel_plane_state *plane_state);
>>> +u32 glk_color_ctl(const struct intel_plane_state *plane_state);
>>> u32 skl_plane_stride(const struct drm_framebuffer *fb, int plane,
>>> unsigned int rotation);
>>> int skl_check_plane_surface(struct intel_plane_state *plane_state);
>>> @@ -1888,6 +1889,7 @@ bool intel_sdvo_init(struct drm_i915_private *dev_priv,
>>>
>>>
>>> /* intel_sprite.c */
>>> +bool intel_format_is_yuv(u32 format);
>>> int intel_usecs_to_scanlines(const struct drm_display_mode *adjusted_mode,
>>> int usecs);
>>> struct intel_plane *intel_sprite_plane_create(struct drm_i915_private *dev_priv,
>>> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
>>> index 2ce3b3c6ffa8..cc07157f2eb6 100644
>>> --- a/drivers/gpu/drm/i915/intel_sprite.c
>>> +++ b/drivers/gpu/drm/i915/intel_sprite.c
>>> @@ -40,8 +40,7 @@
>>> #include <drm/i915_drm.h>
>>> #include "i915_drv.h"
>>>
>>> -static bool
>>> -format_is_yuv(uint32_t format)
>>> +bool intel_format_is_yuv(u32 format)
>>> {
>>> switch (format) {
>>> case DRM_FORMAT_YUYV:
>>> @@ -264,9 +263,7 @@ skl_update_plane(struct intel_plane *plane,
>>>
>>> if (IS_GEMINILAKE(dev_priv)) {
>>> I915_WRITE_FW(PLANE_COLOR_CTL(pipe, plane_id),
>>> - PLANE_COLOR_PIPE_GAMMA_ENABLE |
>>> - PLANE_COLOR_PIPE_CSC_ENABLE |
>>> - PLANE_COLOR_PLANE_GAMMA_DISABLE);
>>> + glk_color_ctl(plane_state));
>>> }
>>>
>>> if (key->flags) {
>>> @@ -333,7 +330,7 @@ chv_update_csc(const struct intel_plane_state *plane_state)
>>> enum plane_id plane_id = plane->id;
>>>
>>> /* Seems RGB data bypasses the CSC always */
>>> - if (!format_is_yuv(fb->format->format))
>>> + if (!intel_format_is_yuv(fb->format->format))
>>> return;
>>>
>>> /*
>>> @@ -375,7 +372,7 @@ vlv_update_clrc(const struct intel_plane_state *plane_state)
>>> enum plane_id plane_id = plane->id;
>>> int con, bri, sh_sin, sh_cos;
>>>
>>> - if (format_is_yuv(fb->format->format)) {
>>> + if (intel_format_is_yuv(fb->format->format)) {
>>> /*
>>> * expand limited range to full range.
>>> * contrast is applied first, then brightness
>>> @@ -933,7 +930,7 @@ intel_check_sprite_plane(struct intel_plane *plane,
>>> src_y = src->y1 >> 16;
>>> src_h = drm_rect_height(src) >> 16;
>>>
>>> - if (format_is_yuv(fb->format->format)) {
>>> + if (intel_format_is_yuv(fb->format->format)) {
>>> src_x &= ~1;
>>> src_w &= ~1;
>>>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 4/5] drm/i915: Change the COLOR_ENCODING prop default value to BT.709
2017-06-08 20:33 ` [PATCH 4/5] drm/i915: Change the COLOR_ENCODING prop default value to BT.709 ville.syrjala
@ 2017-06-15 13:31 ` Sharma, Shashank
0 siblings, 0 replies; 27+ messages in thread
From: Sharma, Shashank @ 2017-06-15 13:31 UTC (permalink / raw)
To: ville.syrjala, intel-gfx; +Cc: Jyri Sarha
ACK: Shashank Sharma <shashank.sharma@intel.com>
Regards
Shashank
On 6/9/2017 2:03 AM, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Bring us forward from the stone age and switch our default YCbCr->RGB
> conversion matrix to BT.709 from BT.601. I would expect most matrial
> to be BT.709 these days.
>
> Cc: Jyri Sarha <jsarha@ti.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
> drivers/gpu/drm/i915/intel_display.c | 2 +-
> drivers/gpu/drm/i915/intel_sprite.c | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 810b53b0128c..ba3ad0e26043 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13831,7 +13831,7 @@ intel_primary_plane_create(struct drm_i915_private *dev_priv, enum pipe pipe)
> BIT(DRM_COLOR_YCBCR_BT601) |
> BIT(DRM_COLOR_YCBCR_BT709),
> BIT(DRM_COLOR_YCBCR_LIMITED_RANGE),
> - DRM_COLOR_YCBCR_BT601,
> + DRM_COLOR_YCBCR_BT709,
> DRM_COLOR_YCBCR_LIMITED_RANGE);
>
> drm_plane_helper_add(&primary->base, &intel_plane_helper_funcs);
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index c21f43d64b00..ba433bf40189 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -1249,7 +1249,7 @@ intel_sprite_plane_create(struct drm_i915_private *dev_priv,
> BIT(DRM_COLOR_YCBCR_BT601) |
> BIT(DRM_COLOR_YCBCR_BT709),
> BIT(DRM_COLOR_YCBCR_LIMITED_RANGE),
> - DRM_COLOR_YCBCR_BT601,
> + DRM_COLOR_YCBCR_BT709,
> DRM_COLOR_YCBCR_LIMITED_RANGE);
>
> drm_plane_helper_add(&intel_plane->base, &intel_plane_helper_funcs);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 5/5] drm/i915: Add support for the YCbCr COLOR_RANGE property
2017-06-08 20:33 ` [PATCH 5/5] drm/i915: Add support for the YCbCr COLOR_RANGE property ville.syrjala
@ 2017-06-15 13:37 ` Sharma, Shashank
2017-06-20 13:35 ` [PATCH v2 " ville.syrjala
1 sibling, 0 replies; 27+ messages in thread
From: Sharma, Shashank @ 2017-06-15 13:37 UTC (permalink / raw)
To: ville.syrjala, intel-gfx; +Cc: Jyri Sarha
Reviewed-by: Shashank Sharma <shashank.sharma@intel.com>
Regards
Shashank
On 6/9/2017 2:03 AM, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Add support for the COLOR_RANGE property on planes. This property
> selects whether the input YCbCr data is to treated as limited range
> or full range.
>
> On most platforms this is a matter of setting the "YUV range correction
> disable" bit, and on VLV/CHV we'll just have to program the color
> correction logic to pass the data through unmodified.
>
> Cc: Jyri Sarha <jsarha@ti.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
> drivers/gpu/drm/i915/i915_reg.h | 4 ++++
> drivers/gpu/drm/i915/intel_display.c | 9 ++++++++-
> drivers/gpu/drm/i915/intel_sprite.c | 12 ++++++++++--
> 3 files changed, 22 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index bac3ec378b6e..6b16b25ba96f 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -5604,6 +5604,7 @@ enum {
> #define _DVSACNTR 0x72180
> #define DVS_ENABLE (1<<31)
> #define DVS_GAMMA_ENABLE (1<<30)
> +#define DVS_YUV_RANGE_CORRECTION_DISABLE (1<<27)
> #define DVS_PIXFORMAT_MASK (3<<25)
> #define DVS_FORMAT_YUV422 (0<<25)
> #define DVS_FORMAT_RGBX101010 (1<<25)
> @@ -5672,6 +5673,7 @@ enum {
> #define _SPRA_CTL 0x70280
> #define SPRITE_ENABLE (1<<31)
> #define SPRITE_GAMMA_ENABLE (1<<30)
> +#define SPRITE_YUV_RANGE_CORRECTION_DISABLE (1<<28)
> #define SPRITE_PIXFORMAT_MASK (7<<25)
> #define SPRITE_FORMAT_YUV422 (0<<25)
> #define SPRITE_FORMAT_RGBX101010 (1<<25)
> @@ -5863,6 +5865,7 @@ enum {
> #define _PLANE_CTL_3_A 0x70380
> #define PLANE_CTL_ENABLE (1 << 31)
> #define PLANE_CTL_PIPE_GAMMA_ENABLE (1 << 30)
> +#define PLANE_CTL_YUV_RANGE_CORRECTION_DISABLE (1 << 28)
> #define PLANE_CTL_FORMAT_MASK (0xf << 24)
> #define PLANE_CTL_FORMAT_YUV422 ( 0 << 24)
> #define PLANE_CTL_FORMAT_NV12 ( 1 << 24)
> @@ -5926,6 +5929,7 @@ enum {
> #define _PLANE_COLOR_CTL_2_A 0x702CC /* GLK+ */
> #define _PLANE_COLOR_CTL_3_A 0x703CC /* GLK+ */
> #define PLANE_COLOR_PIPE_GAMMA_ENABLE (1 << 30)
> +#define PLANE_COLOR_YUV_RANGE_CORRECTION_DISABLE (1 << 28)
> #define PLANE_COLOR_PIPE_CSC_ENABLE (1 << 23)
> #define PLANE_COLOR_CSC_MODE_BYPASS (0 << 17)
> #define PLANE_COLOR_CSC_MODE_YUV601_TO_RGB709 (1 << 17)
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index ba3ad0e26043..6fe6e67eeefc 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3324,6 +3324,9 @@ u32 skl_plane_ctl(const struct intel_crtc_state *crtc_state,
>
> if (plane_state->base.color_encoding == DRM_COLOR_YCBCR_BT709)
> plane_ctl |= PLANE_CTL_YUV_CSC_FORMAT_BT709;
> +
> + if (plane_state->base.color_range == DRM_COLOR_YCBCR_FULL_RANGE)
> + plane_ctl |= PLANE_CTL_YUV_RANGE_CORRECTION_DISABLE;
> }
>
> plane_ctl |= skl_plane_ctl_format(fb->format->format);
> @@ -3352,6 +3355,9 @@ u32 glk_color_ctl(const struct intel_plane_state *plane_state)
> color_ctl |= PLANE_COLOR_CSC_MODE_YUV709_TO_RGB709;
> else
> color_ctl |= PLANE_COLOR_CSC_MODE_YUV601_TO_RGB709;
> +
> + if (plane_state->base.color_range == DRM_COLOR_YCBCR_FULL_RANGE)
> + color_ctl |= PLANE_COLOR_YUV_RANGE_CORRECTION_DISABLE;
> }
>
> return color_ctl;
> @@ -13830,7 +13836,8 @@ intel_primary_plane_create(struct drm_i915_private *dev_priv, enum pipe pipe)
> drm_plane_create_color_properties(&primary->base,
> BIT(DRM_COLOR_YCBCR_BT601) |
> BIT(DRM_COLOR_YCBCR_BT709),
> - BIT(DRM_COLOR_YCBCR_LIMITED_RANGE),
> + BIT(DRM_COLOR_YCBCR_LIMITED_RANGE) |
> + BIT(DRM_COLOR_YCBCR_FULL_RANGE),
> DRM_COLOR_YCBCR_BT709,
> DRM_COLOR_YCBCR_LIMITED_RANGE);
>
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index ba433bf40189..6276322fd8ad 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -387,7 +387,8 @@ vlv_update_clrc(const struct intel_plane_state *plane_state)
> enum plane_id plane_id = plane->id;
> int con, bri, sh_sin, sh_cos;
>
> - if (intel_format_is_yuv(fb->format->format)) {
> + if (intel_format_is_yuv(fb->format->format) &&
> + plane_state->base.color_range == DRM_COLOR_YCBCR_LIMITED_RANGE) {
> /*
> * expand limited range to full range.
> * contrast is applied first, then brightness
> @@ -599,6 +600,9 @@ static u32 ivb_sprite_ctl(const struct intel_crtc_state *crtc_state,
> if (plane_state->base.color_encoding == DRM_COLOR_YCBCR_BT709)
> sprctl |= SPRITE_YUV_CSC_FORMAT_BT709;
>
> + if (plane_state->base.color_range == DRM_COLOR_YCBCR_FULL_RANGE)
> + sprctl |= SPRITE_YUV_RANGE_CORRECTION_DISABLE;
> +
> if (fb->modifier == I915_FORMAT_MOD_X_TILED)
> sprctl |= SPRITE_TILED;
>
> @@ -739,6 +743,9 @@ static u32 g4x_sprite_ctl(const struct intel_crtc_state *crtc_state,
> if (plane_state->base.color_encoding == DRM_COLOR_YCBCR_BT709)
> dvscntr |= DVS_YUV_CSC_FORMAT_BT709;
>
> + if (plane_state->base.color_range == DRM_COLOR_YCBCR_FULL_RANGE)
> + dvscntr |= DVS_YUV_RANGE_CORRECTION_DISABLE;
> +
> if (fb->modifier == I915_FORMAT_MOD_X_TILED)
> dvscntr |= DVS_TILED;
>
> @@ -1248,7 +1255,8 @@ intel_sprite_plane_create(struct drm_i915_private *dev_priv,
> drm_plane_create_color_properties(&intel_plane->base,
> BIT(DRM_COLOR_YCBCR_BT601) |
> BIT(DRM_COLOR_YCBCR_BT709),
> - BIT(DRM_COLOR_YCBCR_LIMITED_RANGE),
> + BIT(DRM_COLOR_YCBCR_LIMITED_RANGE) |
> + BIT(DRM_COLOR_YCBCR_FULL_RANGE),
> DRM_COLOR_YCBCR_BT709,
> DRM_COLOR_YCBCR_LIMITED_RANGE);
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 3/5] drm/i915: Add support for the YCbCr COLOR_ENCODING property
2017-06-15 13:22 ` Sharma, Shashank
@ 2017-06-16 13:02 ` Ville Syrjälä
0 siblings, 0 replies; 27+ messages in thread
From: Ville Syrjälä @ 2017-06-16 13:02 UTC (permalink / raw)
To: Sharma, Shashank; +Cc: intel-gfx, Jyri Sarha
On Thu, Jun 15, 2017 at 06:52:53PM +0530, Sharma, Shashank wrote:
> Regards
>
> Shashank
>
>
> On 6/9/2017 2:03 AM, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > 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@ti.com>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> > 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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 2/5] drm/i915: Fix plane YCbCr->RGB conversion for GLK
2017-06-15 13:28 ` Sharma, Shashank
@ 2017-06-16 13:03 ` Ville Syrjälä
0 siblings, 0 replies; 27+ messages in thread
From: Ville Syrjälä @ 2017-06-16 13:03 UTC (permalink / raw)
To: Sharma, Shashank
Cc: Ander Conselvan de Oliveira, intel-gfx, dhanya.p.r, Jyri Sarha
On Thu, Jun 15, 2017 at 06:58:45PM +0530, Sharma, Shashank wrote:
> Regards
>
> Shashank
>
>
> On 6/15/2017 6:12 PM, Ville Syrjälä wrote:
> > On Thu, Jun 15, 2017 at 05:57:21PM +0530, Sharma, Shashank wrote:
> >> Regards
> >> Shashank
> >> On 6/9/2017 2:03 AM, ville.syrjala@linux.intel.com wrote:
> >>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>>
> >>> On GLK the plane CSC controls moved into the COLOR_CTL register.
> >>> Update the code to progam the YCbCr->RGB CSC mode correctly when
> >>> faced with an YCbCr framebuffer.
> >>>
> >>> The spec is rather confusing as it calls the mode "YUV601 to RGB709".
> >>> I'm going to assume that just means it's going to use the YCbCr->RGB
> >>> matrix as specified in BT.601 and doesn't actually change the gamut.
> >>>
> >>> Cc: Jyri Sarha <jsarha@ti.com>
> >>> Cc: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
> >>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>> ---
> >>> drivers/gpu/drm/i915/i915_reg.h | 5 +++++
> >>> drivers/gpu/drm/i915/intel_display.c | 19 ++++++++++++++++---
> >>> drivers/gpu/drm/i915/intel_drv.h | 2 ++
> >>> drivers/gpu/drm/i915/intel_sprite.c | 13 +++++--------
> >>> 4 files changed, 28 insertions(+), 11 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> >>> index ce7c0dc79cf7..f4f51e7a3e83 100644
> >>> --- a/drivers/gpu/drm/i915/i915_reg.h
> >>> +++ b/drivers/gpu/drm/i915/i915_reg.h
> >>> @@ -5924,6 +5924,11 @@ enum {
> >>> #define _PLANE_COLOR_CTL_3_A 0x703CC /* GLK+ */
> >>> #define PLANE_COLOR_PIPE_GAMMA_ENABLE (1 << 30)
> >>> #define PLANE_COLOR_PIPE_CSC_ENABLE (1 << 23)
> >>> +#define PLANE_COLOR_CSC_MODE_BYPASS (0 << 17)
> >>> +#define PLANE_COLOR_CSC_MODE_YUV601_TO_RGB709 (1 << 17)
> >>> +#define PLANE_COLOR_CSC_MODE_YUV709_TO_RGB709 (2 << 17)
> >>> +#define PLANE_COLOR_CSC_MODE_YUV2020_TO_RGB2020 (3 << 17)
> >>> +#define PLANE_COLOR_CSC_MODE_RGB709_TO_RGB2020 (4 << 17)
> >>> #define PLANE_COLOR_PLANE_GAMMA_DISABLE (1 << 13)
> >>> #define _PLANE_BUF_CFG_1_A 0x7027c
> >>> #define _PLANE_BUF_CFG_2_A 0x7037c
> >>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >>> index 25390dd8e08e..e2aaaf60a969 100644
> >>> --- a/drivers/gpu/drm/i915/intel_display.c
> >>> +++ b/drivers/gpu/drm/i915/intel_display.c
> >>> @@ -3335,6 +3335,21 @@ u32 skl_plane_ctl(const struct intel_crtc_state *crtc_state,
> >>> return plane_ctl;
> >>> }
> >>>
> >>> +u32 glk_color_ctl(const struct intel_plane_state *plane_state)
> >>> +{
> >>> + const struct drm_framebuffer *fb = plane_state->base.fb;
> >>> + u32 color_ctl;
> >>> +
> >>> + color_ctl = PLANE_COLOR_PIPE_GAMMA_ENABLE |
> >>> + 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;
> >> why not YUV709_TO_RGB709 ?
> > To match the other platforms.
> I am slightly confused. Dint we just upgrade our CSC matrix from stone
> age, to REC_709, in patch 4 ?
> Shouldn't we be consistent here ? or am I missing something basic :-) ?
This is patch 2, which comes before patch 4. So it's consistent with
the state of the other platforms at the time. Patch 4 will then change
all platforms to BT.709 in one fell swoop.
>
> - Shashank
> >>> +
> >>> + return color_ctl;
> >>> +}
> >>> +
> >> You might want to have a look on this series from Dhanya too:
> >>
> >> https://patchwork.freedesktop.org/series/17259/
> > I think we have everything we need from there.
> >
> >> - Shashank
> >>
> >>> static void skylake_update_primary_plane(struct intel_plane *plane,
> >>> const struct intel_crtc_state *crtc_state,
> >>> const struct intel_plane_state *plane_state)
> >>> @@ -3374,9 +3389,7 @@ static void skylake_update_primary_plane(struct intel_plane *plane,
> >>>
> >>> if (IS_GEMINILAKE(dev_priv)) {
> >>> I915_WRITE_FW(PLANE_COLOR_CTL(pipe, plane_id),
> >>> - PLANE_COLOR_PIPE_GAMMA_ENABLE |
> >>> - PLANE_COLOR_PIPE_CSC_ENABLE |
> >>> - PLANE_COLOR_PLANE_GAMMA_DISABLE);
> >>> + glk_color_ctl(plane_state));
> >>> }
> >>>
> >>> I915_WRITE_FW(PLANE_CTL(pipe, plane_id), plane_ctl);
> >>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> >>> index 83dd40905821..e13e714b1176 100644
> >>> --- a/drivers/gpu/drm/i915/intel_drv.h
> >>> +++ b/drivers/gpu/drm/i915/intel_drv.h
> >>> @@ -1484,6 +1484,7 @@ static inline u32 intel_plane_ggtt_offset(const struct intel_plane_state *state)
> >>>
> >>> u32 skl_plane_ctl(const struct intel_crtc_state *crtc_state,
> >>> const struct intel_plane_state *plane_state);
> >>> +u32 glk_color_ctl(const struct intel_plane_state *plane_state);
> >>> u32 skl_plane_stride(const struct drm_framebuffer *fb, int plane,
> >>> unsigned int rotation);
> >>> int skl_check_plane_surface(struct intel_plane_state *plane_state);
> >>> @@ -1888,6 +1889,7 @@ bool intel_sdvo_init(struct drm_i915_private *dev_priv,
> >>>
> >>>
> >>> /* intel_sprite.c */
> >>> +bool intel_format_is_yuv(u32 format);
> >>> int intel_usecs_to_scanlines(const struct drm_display_mode *adjusted_mode,
> >>> int usecs);
> >>> struct intel_plane *intel_sprite_plane_create(struct drm_i915_private *dev_priv,
> >>> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> >>> index 2ce3b3c6ffa8..cc07157f2eb6 100644
> >>> --- a/drivers/gpu/drm/i915/intel_sprite.c
> >>> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> >>> @@ -40,8 +40,7 @@
> >>> #include <drm/i915_drm.h>
> >>> #include "i915_drv.h"
> >>>
> >>> -static bool
> >>> -format_is_yuv(uint32_t format)
> >>> +bool intel_format_is_yuv(u32 format)
> >>> {
> >>> switch (format) {
> >>> case DRM_FORMAT_YUYV:
> >>> @@ -264,9 +263,7 @@ skl_update_plane(struct intel_plane *plane,
> >>>
> >>> if (IS_GEMINILAKE(dev_priv)) {
> >>> I915_WRITE_FW(PLANE_COLOR_CTL(pipe, plane_id),
> >>> - PLANE_COLOR_PIPE_GAMMA_ENABLE |
> >>> - PLANE_COLOR_PIPE_CSC_ENABLE |
> >>> - PLANE_COLOR_PLANE_GAMMA_DISABLE);
> >>> + glk_color_ctl(plane_state));
> >>> }
> >>>
> >>> if (key->flags) {
> >>> @@ -333,7 +330,7 @@ chv_update_csc(const struct intel_plane_state *plane_state)
> >>> enum plane_id plane_id = plane->id;
> >>>
> >>> /* Seems RGB data bypasses the CSC always */
> >>> - if (!format_is_yuv(fb->format->format))
> >>> + if (!intel_format_is_yuv(fb->format->format))
> >>> return;
> >>>
> >>> /*
> >>> @@ -375,7 +372,7 @@ vlv_update_clrc(const struct intel_plane_state *plane_state)
> >>> enum plane_id plane_id = plane->id;
> >>> int con, bri, sh_sin, sh_cos;
> >>>
> >>> - if (format_is_yuv(fb->format->format)) {
> >>> + if (intel_format_is_yuv(fb->format->format)) {
> >>> /*
> >>> * expand limited range to full range.
> >>> * contrast is applied first, then brightness
> >>> @@ -933,7 +930,7 @@ intel_check_sprite_plane(struct intel_plane *plane,
> >>> src_y = src->y1 >> 16;
> >>> src_h = drm_rect_height(src) >> 16;
> >>>
> >>> - if (format_is_yuv(fb->format->format)) {
> >>> + if (intel_format_is_yuv(fb->format->format)) {
> >>> src_x &= ~1;
> >>> src_w &= ~1;
> >>>
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v3 1/5] drm/i915: Correctly handle limited range YCbCr data on VLV/CHV
2017-06-08 20:33 ` [PATCH v2 1/5] drm/i915: Correctly handle limited range YCbCr data on VLV/CHV ville.syrjala
2017-06-15 12:38 ` Sharma, Shashank
@ 2017-06-20 13:32 ` ville.syrjala
2017-06-20 13:57 ` Sharma, Shashank
1 sibling, 1 reply; 27+ messages in thread
From: ville.syrjala @ 2017-06-20 13:32 UTC (permalink / raw)
To: intel-gfx; +Cc: Tang, Jun, stable, Jyri Sarha
From: Ville Syrjälä <ville.syrjala@linux.intel.com>
Turns out the VLV/CHV fixed function sprite CSC expects full range
data as input. We've been feeding it limited range data to it all
along. To expand the data out to full range we'll use the color
correction registers (brightness, contrast, and saturation).
On CHV pipe B we were actually doing the right thing already because we
progammed the custom CSC matrix to do expect limited range input. Now
that well pre-expand the data out with the color correction unit, we
need to change the CSC matrix to operate with full range input instead.
This should make the sprite output of the other pipes match the sprite
output of pipe B reasonably well. Looking at the resulting pipe CRCs,
there can be a slight difference in the output, but as I don't know
the formula used by the fixed function CSC of the other pipes, I don't
think it's worth the effort to try to match the output exactly. It
might not even be possible due to difference in internal precision etc.
One slight caveat here is that the color correction registers are single
bufferred, so we should really be updating them during vblank, but we
still don't have a mechanism for that, so just toss in another FIXME.
v2: Rebase
v3: s/bri/brightness/ s/con/contrast/ (Shashank)
Cc: Shashank Sharma <shashank.sharma@intel.com>
Cc: Jyri Sarha <jsarha@ti.com>
Cc: "Tang, Jun" <jun.tang@intel.com>
Reported-by: "Tang, Jun" <jun.tang@intel.com>
Cc: stable@vger.kernel.org
Fixes: 7f1f3851feb0 ("drm/i915: sprite support for ValleyView v4")
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
drivers/gpu/drm/i915/i915_reg.h | 10 +++++
drivers/gpu/drm/i915/intel_sprite.c | 74 ++++++++++++++++++++++++++++---------
2 files changed, 66 insertions(+), 18 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index c8647cfa81ba..290322588f56 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -5974,6 +5974,12 @@ enum {
#define _SPATILEOFF (VLV_DISPLAY_BASE + 0x721a4)
#define _SPACONSTALPHA (VLV_DISPLAY_BASE + 0x721a8)
#define SP_CONST_ALPHA_ENABLE (1<<31)
+#define _SPACLRC0 (VLV_DISPLAY_BASE + 0x721d0)
+#define SP_CONTRAST(x) ((x) << 18) /* u3.6 */
+#define SP_BRIGHTNESS(x) ((x) & 0xff) /* s8 */
+#define _SPACLRC1 (VLV_DISPLAY_BASE + 0x721d4)
+#define SP_SH_SIN(x) (((x) & 0x7ff) << 16) /* s4.7 */
+#define SP_SH_COS(x) (x) /* u3.7 */
#define _SPAGAMC (VLV_DISPLAY_BASE + 0x721f4)
#define _SPBCNTR (VLV_DISPLAY_BASE + 0x72280)
@@ -5987,6 +5993,8 @@ enum {
#define _SPBKEYMAXVAL (VLV_DISPLAY_BASE + 0x722a0)
#define _SPBTILEOFF (VLV_DISPLAY_BASE + 0x722a4)
#define _SPBCONSTALPHA (VLV_DISPLAY_BASE + 0x722a8)
+#define _SPBCLRC0 (VLV_DISPLAY_BASE + 0x722d0)
+#define _SPBCLRC1 (VLV_DISPLAY_BASE + 0x722d4)
#define _SPBGAMC (VLV_DISPLAY_BASE + 0x722f4)
#define _MMIO_VLV_SPR(pipe, plane_id, reg_a, reg_b) \
@@ -6003,6 +6011,8 @@ enum {
#define SPKEYMAXVAL(pipe, plane_id) _MMIO_VLV_SPR((pipe), (plane_id), _SPAKEYMAXVAL, _SPBKEYMAXVAL)
#define SPTILEOFF(pipe, plane_id) _MMIO_VLV_SPR((pipe), (plane_id), _SPATILEOFF, _SPBTILEOFF)
#define SPCONSTALPHA(pipe, plane_id) _MMIO_VLV_SPR((pipe), (plane_id), _SPACONSTALPHA, _SPBCONSTALPHA)
+#define SPCLRC0(pipe, plane_id) _MMIO_VLV_SPR((pipe), (plane_id), _SPACLRC0, _SPBCLRC0)
+#define SPCLRC1(pipe, plane_id) _MMIO_VLV_SPR((pipe), (plane_id), _SPACLRC1, _SPBCLRC1)
#define SPGAMC(pipe, plane_id) _MMIO_VLV_SPR((pipe), (plane_id), _SPAGAMC, _SPBGAMC)
/*
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 0c650c2cbca8..4462408cc835 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -325,44 +325,80 @@ skl_disable_plane(struct intel_plane *plane, struct intel_crtc *crtc)
}
static void
-chv_update_csc(struct intel_plane *plane, uint32_t format)
+chv_update_csc(const struct intel_plane_state *plane_state)
{
+ struct intel_plane *plane = to_intel_plane(plane_state->base.plane);
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;
/* Seems RGB data bypasses the CSC always */
- if (!format_is_yuv(format))
+ if (!format_is_yuv(fb->format->format))
return;
/*
- * BT.601 limited range YCbCr -> full range RGB
+ * BT.601 full range YCbCr -> full range RGB
*
- * |r| | 6537 4769 0| |cr |
- * |g| = |-3330 4769 -1605| x |y-64|
- * |b| | 0 4769 8263| |cb |
+ * |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, so no
- * need for any offset. For Y we need to remove the offset.
+ * 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(-64));
+ 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(4769) | SPCSC_C0(6537));
- I915_WRITE_FW(SPCSCC23(plane_id), SPCSC_C1(-3330) | SPCSC_C0(0));
- I915_WRITE_FW(SPCSCC45(plane_id), SPCSC_C1(-1605) | SPCSC_C0(4769));
- I915_WRITE_FW(SPCSCC67(plane_id), SPCSC_C1(4769) | SPCSC_C0(0));
- I915_WRITE_FW(SPCSCC8(plane_id), SPCSC_C0(8263));
+ 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(SPCSCYGICLAMP(plane_id), SPCSC_IMAX(940) | SPCSC_IMIN(64));
- I915_WRITE_FW(SPCSCCBICLAMP(plane_id), SPCSC_IMAX(448) | SPCSC_IMIN(-448));
- I915_WRITE_FW(SPCSCCRICLAMP(plane_id), SPCSC_IMAX(448) | SPCSC_IMIN(-448));
+ I915_WRITE_FW(SPCSCYGICLAMP(plane_id), SPCSC_IMAX(1023) | SPCSC_IMIN(0));
+ I915_WRITE_FW(SPCSCCBICLAMP(plane_id), SPCSC_IMAX(512) | SPCSC_IMIN(-512));
+ I915_WRITE_FW(SPCSCCRICLAMP(plane_id), SPCSC_IMAX(512) | SPCSC_IMIN(-512));
I915_WRITE_FW(SPCSCYGOCLAMP(plane_id), SPCSC_OMAX(1023) | SPCSC_OMIN(0));
I915_WRITE_FW(SPCSCCBOCLAMP(plane_id), SPCSC_OMAX(1023) | SPCSC_OMIN(0));
I915_WRITE_FW(SPCSCCROCLAMP(plane_id), SPCSC_OMAX(1023) | SPCSC_OMIN(0));
}
+static void
+vlv_update_clrc(const struct intel_plane_state *plane_state)
+{
+ struct intel_plane *plane = to_intel_plane(plane_state->base.plane);
+ struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
+ const struct drm_framebuffer *fb = plane_state->base.fb;
+ enum pipe pipe = plane->pipe;
+ enum plane_id plane_id = plane->id;
+ int contrast, brightness, sh_sin, sh_cos;
+
+ if (format_is_yuv(fb->format->format)) {
+ /*
+ * expand limited range to full range.
+ * contrast is applied first, then brightness
+ */
+ contrast = ((255 << 7) / 219 + 1) >> 1;
+ brightness = -((16 << 1) * 255 / 219 + 1) >> 1;
+ sh_sin = 0;
+ sh_cos = (((128 << 8) / 112) + 1) >> 1;
+ } else {
+ /* pass-through everything */
+ contrast = 1 << 6;
+ brightness = 0;
+ sh_sin = 0;
+ sh_cos = 1 << 7;
+ }
+
+ /* FIXME these register are single buffered :( */
+ I915_WRITE_FW(SPCLRC0(pipe, plane_id),
+ SP_CONTRAST(contrast) | SP_BRIGHTNESS(brightness));
+ I915_WRITE_FW(SPCLRC1(pipe, plane_id),
+ SP_SH_SIN(sh_sin) | SP_SH_COS(sh_cos));
+}
+
static u32 vlv_sprite_ctl(const struct intel_crtc_state *crtc_state,
const struct intel_plane_state *plane_state)
{
@@ -456,8 +492,10 @@ vlv_update_plane(struct intel_plane *plane,
spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
+ vlv_update_clrc(plane_state);
+
if (IS_CHERRYVIEW(dev_priv) && pipe == PIPE_B)
- chv_update_csc(plane, fb->format->format);
+ chv_update_csc(plane_state);
if (key->flags) {
I915_WRITE_FW(SPKEYMINVAL(pipe, plane_id), key->min_value);
--
2.13.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v2 3/5] drm/i915: Add support for the YCbCr COLOR_ENCODING property
2017-06-08 20:33 ` [PATCH 3/5] drm/i915: Add support for the YCbCr COLOR_ENCODING property ville.syrjala
2017-06-15 13:22 ` Sharma, Shashank
@ 2017-06-20 13:33 ` ville.syrjala
2017-06-20 13:43 ` Sharma, Shashank
1 sibling, 1 reply; 27+ messages in thread
From: ville.syrjala @ 2017-06-20 13:33 UTC (permalink / raw)
To: intel-gfx; +Cc: Jyri Sarha
From: Ville Syrjälä <ville.syrjala@linux.intel.com>
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.
v2: Rename bit defines to match the spec more closely (Shashank)
Cc: Shashank Sharma <shashank.sharma@intel.com>
Cc: Jyri Sarha <jsarha@ti.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
drivers/gpu/drm/i915/i915_reg.h | 5 ++-
drivers/gpu/drm/i915/intel_display.c | 19 +++++++++--
drivers/gpu/drm/i915/intel_sprite.c | 61 +++++++++++++++++++++++++++---------
3 files changed, 67 insertions(+), 18 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 0d5d5747960a..3dccb1639f39 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -5809,6 +5809,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_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)
@@ -5879,7 +5880,7 @@ enum {
#define SPRITE_SOURCE_KEY (1<<22)
#define SPRITE_RGB_ORDER_RGBX (1<<20) /* only for 888 and 161616 */
#define SPRITE_YUV_TO_RGB_CSC_DISABLE (1<<19)
-#define SPRITE_YUV_CSC_FORMAT_BT709 (1<<18) /* 0 is BT601 */
+#define SPRITE_YUV_TO_RGB_CSC_FORMAT_BT709 (1<<18) /* 0 is BT601 */
#define SPRITE_YUV_BYTE_ORDER_MASK (3<<16)
#define SPRITE_YUV_ORDER_YUYV (0<<16)
#define SPRITE_YUV_ORDER_UYVY (1<<16)
@@ -5955,6 +5956,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_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)
@@ -6073,6 +6075,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_TO_RGB_CSC_FORMAT_BT709 (1 << 18)
#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 7c52d4b08f69..3688ff448f4f 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3316,6 +3316,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_TO_RGB_CSC_FORMAT_BT709;
}
plane_ctl |= skl_plane_ctl_format(fb->format->format);
@@ -3339,8 +3342,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;
}
@@ -13836,6 +13843,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)
+ 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 181be7c879fe..fd1629595cef 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_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_TO_RGB_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_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;
--
2.13.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v2 5/5] drm/i915: Add support for the YCbCr COLOR_RANGE property
2017-06-08 20:33 ` [PATCH 5/5] drm/i915: Add support for the YCbCr COLOR_RANGE property ville.syrjala
2017-06-15 13:37 ` Sharma, Shashank
@ 2017-06-20 13:35 ` ville.syrjala
1 sibling, 0 replies; 27+ messages in thread
From: ville.syrjala @ 2017-06-20 13:35 UTC (permalink / raw)
To: intel-gfx; +Cc: Jyri Sarha
From: Ville Syrjälä <ville.syrjala@linux.intel.com>
Add support for the COLOR_RANGE property on planes. This property
selects whether the input YCbCr data is to treated as limited range
or full range.
On most platforms this is a matter of setting the "YUV range correction
disable" bit, and on VLV/CHV we'll just have to program the color
correction logic to pass the data through unmodified.
v2: Rebase
Cc: Jyri Sarha <jsarha@ti.com>
Cc: Shashank Sharma <shashank.sharma@intel.com>
Reviewed-by: Shashank Sharma <shashank.sharma@intel.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
drivers/gpu/drm/i915/i915_reg.h | 4 ++++
drivers/gpu/drm/i915/intel_display.c | 9 ++++++++-
drivers/gpu/drm/i915/intel_sprite.c | 12 ++++++++++--
3 files changed, 22 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 3dccb1639f39..b83b7a41e168 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -5801,6 +5801,7 @@ enum {
#define _DVSACNTR 0x72180
#define DVS_ENABLE (1<<31)
#define DVS_GAMMA_ENABLE (1<<30)
+#define DVS_YUV_RANGE_CORRECTION_DISABLE (1<<27)
#define DVS_PIXFORMAT_MASK (3<<25)
#define DVS_FORMAT_YUV422 (0<<25)
#define DVS_FORMAT_RGBX101010 (1<<25)
@@ -5869,6 +5870,7 @@ enum {
#define _SPRA_CTL 0x70280
#define SPRITE_ENABLE (1<<31)
#define SPRITE_GAMMA_ENABLE (1<<30)
+#define SPRITE_YUV_RANGE_CORRECTION_DISABLE (1<<28)
#define SPRITE_PIXFORMAT_MASK (7<<25)
#define SPRITE_FORMAT_YUV422 (0<<25)
#define SPRITE_FORMAT_RGBX101010 (1<<25)
@@ -6060,6 +6062,7 @@ enum {
#define _PLANE_CTL_3_A 0x70380
#define PLANE_CTL_ENABLE (1 << 31)
#define PLANE_CTL_PIPE_GAMMA_ENABLE (1 << 30)
+#define PLANE_CTL_YUV_RANGE_CORRECTION_DISABLE (1 << 28)
#define PLANE_CTL_FORMAT_MASK (0xf << 24)
#define PLANE_CTL_FORMAT_YUV422 ( 0 << 24)
#define PLANE_CTL_FORMAT_NV12 ( 1 << 24)
@@ -6123,6 +6126,7 @@ enum {
#define _PLANE_COLOR_CTL_2_A 0x702CC /* GLK+ */
#define _PLANE_COLOR_CTL_3_A 0x703CC /* GLK+ */
#define PLANE_COLOR_PIPE_GAMMA_ENABLE (1 << 30)
+#define PLANE_COLOR_YUV_RANGE_CORRECTION_DISABLE (1 << 28)
#define PLANE_COLOR_PIPE_CSC_ENABLE (1 << 23)
#define PLANE_COLOR_CSC_MODE_BYPASS (0 << 17)
#define PLANE_COLOR_CSC_MODE_YUV601_TO_RGB709 (1 << 17)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 03efabee48c4..7143326f6f8d 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3319,6 +3319,9 @@ u32 skl_plane_ctl(const struct intel_crtc_state *crtc_state,
if (plane_state->base.color_encoding == DRM_COLOR_YCBCR_BT709)
plane_ctl |= PLANE_CTL_YUV_TO_RGB_CSC_FORMAT_BT709;
+
+ if (plane_state->base.color_range == DRM_COLOR_YCBCR_FULL_RANGE)
+ plane_ctl |= PLANE_CTL_YUV_RANGE_CORRECTION_DISABLE;
}
plane_ctl |= skl_plane_ctl_format(fb->format->format);
@@ -3347,6 +3350,9 @@ u32 glk_color_ctl(const struct intel_plane_state *plane_state)
color_ctl |= PLANE_COLOR_CSC_MODE_YUV709_TO_RGB709;
else
color_ctl |= PLANE_COLOR_CSC_MODE_YUV601_TO_RGB709;
+
+ if (plane_state->base.color_range == DRM_COLOR_YCBCR_FULL_RANGE)
+ color_ctl |= PLANE_COLOR_YUV_RANGE_CORRECTION_DISABLE;
}
return color_ctl;
@@ -13847,7 +13853,8 @@ intel_primary_plane_create(struct drm_i915_private *dev_priv, enum pipe pipe)
drm_plane_create_color_properties(&primary->base,
BIT(DRM_COLOR_YCBCR_BT601) |
BIT(DRM_COLOR_YCBCR_BT709),
- BIT(DRM_COLOR_YCBCR_LIMITED_RANGE),
+ BIT(DRM_COLOR_YCBCR_LIMITED_RANGE) |
+ BIT(DRM_COLOR_YCBCR_FULL_RANGE),
DRM_COLOR_YCBCR_BT709,
DRM_COLOR_YCBCR_LIMITED_RANGE);
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 0ba5299905fd..540b77eede51 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -387,7 +387,8 @@ vlv_update_clrc(const struct intel_plane_state *plane_state)
enum plane_id plane_id = plane->id;
int contrast, brightness, sh_sin, sh_cos;
- if (intel_format_is_yuv(fb->format->format)) {
+ if (intel_format_is_yuv(fb->format->format) &&
+ plane_state->base.color_range == DRM_COLOR_YCBCR_LIMITED_RANGE) {
/*
* expand limited range to full range.
* contrast is applied first, then brightness
@@ -599,6 +600,9 @@ static u32 ivb_sprite_ctl(const struct intel_crtc_state *crtc_state,
if (plane_state->base.color_encoding == DRM_COLOR_YCBCR_BT709)
sprctl |= SPRITE_YUV_TO_RGB_CSC_FORMAT_BT709;
+ if (plane_state->base.color_range == DRM_COLOR_YCBCR_FULL_RANGE)
+ sprctl |= SPRITE_YUV_RANGE_CORRECTION_DISABLE;
+
if (fb->modifier == I915_FORMAT_MOD_X_TILED)
sprctl |= SPRITE_TILED;
@@ -739,6 +743,9 @@ static u32 g4x_sprite_ctl(const struct intel_crtc_state *crtc_state,
if (plane_state->base.color_encoding == DRM_COLOR_YCBCR_BT709)
dvscntr |= DVS_YUV_FORMAT_BT709;
+ if (plane_state->base.color_range == DRM_COLOR_YCBCR_FULL_RANGE)
+ dvscntr |= DVS_YUV_RANGE_CORRECTION_DISABLE;
+
if (fb->modifier == I915_FORMAT_MOD_X_TILED)
dvscntr |= DVS_TILED;
@@ -1248,7 +1255,8 @@ intel_sprite_plane_create(struct drm_i915_private *dev_priv,
drm_plane_create_color_properties(&intel_plane->base,
BIT(DRM_COLOR_YCBCR_BT601) |
BIT(DRM_COLOR_YCBCR_BT709),
- BIT(DRM_COLOR_YCBCR_LIMITED_RANGE),
+ BIT(DRM_COLOR_YCBCR_LIMITED_RANGE) |
+ BIT(DRM_COLOR_YCBCR_FULL_RANGE),
DRM_COLOR_YCBCR_BT709,
DRM_COLOR_YCBCR_LIMITED_RANGE);
--
2.13.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH v2 3/5] drm/i915: Add support for the YCbCr COLOR_ENCODING property
2017-06-20 13:33 ` [PATCH v2 " ville.syrjala
@ 2017-06-20 13:43 ` Sharma, Shashank
0 siblings, 0 replies; 27+ messages in thread
From: Sharma, Shashank @ 2017-06-20 13:43 UTC (permalink / raw)
To: ville.syrjala, intel-gfx; +Cc: Jyri Sarha
[-- Attachment #1.1: Type: text/plain, Size: 8854 bytes --]
R-B: Shashank Sharma <shashank.sharma@intel.com
<mailto:shashank.sharma@intel.com>>
Regards
Shashank
On 6/20/2017 7:03 PM, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> 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.
>
> v2: Rename bit defines to match the spec more closely (Shashank)
>
> Cc: Shashank Sharma <shashank.sharma@intel.com>
> Cc: Jyri Sarha <jsarha@ti.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
> drivers/gpu/drm/i915/i915_reg.h | 5 ++-
> drivers/gpu/drm/i915/intel_display.c | 19 +++++++++--
> drivers/gpu/drm/i915/intel_sprite.c | 61 +++++++++++++++++++++++++++---------
> 3 files changed, 67 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 0d5d5747960a..3dccb1639f39 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -5809,6 +5809,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_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)
> @@ -5879,7 +5880,7 @@ enum {
> #define SPRITE_SOURCE_KEY (1<<22)
> #define SPRITE_RGB_ORDER_RGBX (1<<20) /* only for 888 and 161616 */
> #define SPRITE_YUV_TO_RGB_CSC_DISABLE (1<<19)
> -#define SPRITE_YUV_CSC_FORMAT_BT709 (1<<18) /* 0 is BT601 */
> +#define SPRITE_YUV_TO_RGB_CSC_FORMAT_BT709 (1<<18) /* 0 is BT601 */
> #define SPRITE_YUV_BYTE_ORDER_MASK (3<<16)
> #define SPRITE_YUV_ORDER_YUYV (0<<16)
> #define SPRITE_YUV_ORDER_UYVY (1<<16)
> @@ -5955,6 +5956,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_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)
> @@ -6073,6 +6075,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_TO_RGB_CSC_FORMAT_BT709 (1 << 18)
> #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 7c52d4b08f69..3688ff448f4f 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3316,6 +3316,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_TO_RGB_CSC_FORMAT_BT709;
> }
>
> plane_ctl |= skl_plane_ctl_format(fb->format->format);
> @@ -3339,8 +3342,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;
> }
> @@ -13836,6 +13843,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)
> + 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 181be7c879fe..fd1629595cef 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_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_TO_RGB_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_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;
[-- Attachment #1.2: Type: text/html, Size: 48143 bytes --]
[-- Attachment #2: Type: text/plain, Size: 160 bytes --]
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v3 1/5] drm/i915: Correctly handle limited range YCbCr data on VLV/CHV
2017-06-20 13:32 ` [PATCH v3 " ville.syrjala
@ 2017-06-20 13:57 ` Sharma, Shashank
2017-06-20 14:34 ` Ville Syrjälä
0 siblings, 1 reply; 27+ messages in thread
From: Sharma, Shashank @ 2017-06-20 13:57 UTC (permalink / raw)
To: ville.syrjala, intel-gfx; +Cc: Jyri Sarha, Tang, Jun, stable
Regards
Shashank
On 6/20/2017 7:02 PM, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Turns out the VLV/CHV fixed function sprite CSC expects full range
> data as input. We've been feeding it limited range data to it all
> along. To expand the data out to full range we'll use the color
> correction registers (brightness, contrast, and saturation).
>
> On CHV pipe B we were actually doing the right thing already because we
> progammed the custom CSC matrix to do expect limited range input. Now
> that well pre-expand the data out with the color correction unit, we
> need to change the CSC matrix to operate with full range input instead.
>
> This should make the sprite output of the other pipes match the sprite
> output of pipe B reasonably well. Looking at the resulting pipe CRCs,
> there can be a slight difference in the output, but as I don't know
> the formula used by the fixed function CSC of the other pipes, I don't
> think it's worth the effort to try to match the output exactly. It
> might not even be possible due to difference in internal precision etc.
>
> One slight caveat here is that the color correction registers are single
> bufferred, so we should really be updating them during vblank, but we
> still don't have a mechanism for that, so just toss in another FIXME.
>
> v2: Rebase
> v3: s/bri/brightness/ s/con/contrast/ (Shashank)
>
> Cc: Shashank Sharma <shashank.sharma@intel.com>
> Cc: Jyri Sarha <jsarha@ti.com>
> Cc: "Tang, Jun" <jun.tang@intel.com>
> Reported-by: "Tang, Jun" <jun.tang@intel.com>
> Cc: stable@vger.kernel.org
> Fixes: 7f1f3851feb0 ("drm/i915: sprite support for ValleyView v4")
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
> drivers/gpu/drm/i915/i915_reg.h | 10 +++++
> drivers/gpu/drm/i915/intel_sprite.c | 74 ++++++++++++++++++++++++++++---------
> 2 files changed, 66 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index c8647cfa81ba..290322588f56 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -5974,6 +5974,12 @@ enum {
> #define _SPATILEOFF (VLV_DISPLAY_BASE + 0x721a4)
> #define _SPACONSTALPHA (VLV_DISPLAY_BASE + 0x721a8)
> #define SP_CONST_ALPHA_ENABLE (1<<31)
> +#define _SPACLRC0 (VLV_DISPLAY_BASE + 0x721d0)
> +#define SP_CONTRAST(x) ((x) << 18) /* u3.6 */
protection for higher reserved bits ? From 27-31 ? something like ((x &
1FF) << 18) ?
> +#define SP_BRIGHTNESS(x) ((x) & 0xff) /* s8 */
> +#define _SPACLRC1 (VLV_DISPLAY_BASE + 0x721d4)
> +#define SP_SH_SIN(x) (((x) & 0x7ff) << 16) /* s4.7 */
> +#define SP_SH_COS(x) (x) /* u3.7 */
>
#define SP_SH_COS(x) (x & 3FF) /* u3.7 */ ?
> #define _SPAGAMC (VLV_DISPLAY_BASE + 0x721f4)
>
> #define _SPBCNTR (VLV_DISPLAY_BASE + 0x72280)
> @@ -5987,6 +5993,8 @@ enum {
> #define _SPBKEYMAXVAL (VLV_DISPLAY_BASE + 0x722a0)
> #define _SPBTILEOFF (VLV_DISPLAY_BASE + 0x722a4)
> #define _SPBCONSTALPHA (VLV_DISPLAY_BASE + 0x722a8)
> +#define _SPBCLRC0 (VLV_DISPLAY_BASE + 0x722d0)
> +#define _SPBCLRC1 (VLV_DISPLAY_BASE + 0x722d4)
> #define _SPBGAMC (VLV_DISPLAY_BASE + 0x722f4)
>
> #define _MMIO_VLV_SPR(pipe, plane_id, reg_a, reg_b) \
> @@ -6003,6 +6011,8 @@ enum {
> #define SPKEYMAXVAL(pipe, plane_id) _MMIO_VLV_SPR((pipe), (plane_id), _SPAKEYMAXVAL, _SPBKEYMAXVAL)
> #define SPTILEOFF(pipe, plane_id) _MMIO_VLV_SPR((pipe), (plane_id), _SPATILEOFF, _SPBTILEOFF)
> #define SPCONSTALPHA(pipe, plane_id) _MMIO_VLV_SPR((pipe), (plane_id), _SPACONSTALPHA, _SPBCONSTALPHA)
> +#define SPCLRC0(pipe, plane_id) _MMIO_VLV_SPR((pipe), (plane_id), _SPACLRC0, _SPBCLRC0)
> +#define SPCLRC1(pipe, plane_id) _MMIO_VLV_SPR((pipe), (plane_id), _SPACLRC1, _SPBCLRC1)
> #define SPGAMC(pipe, plane_id) _MMIO_VLV_SPR((pipe), (plane_id), _SPAGAMC, _SPBGAMC)
>
> /*
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 0c650c2cbca8..4462408cc835 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -325,44 +325,80 @@ skl_disable_plane(struct intel_plane *plane, struct intel_crtc *crtc)
> }
>
> static void
> -chv_update_csc(struct intel_plane *plane, uint32_t format)
> +chv_update_csc(const struct intel_plane_state *plane_state)
> {
> + struct intel_plane *plane = to_intel_plane(plane_state->base.plane);
> 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;
>
> /* Seems RGB data bypasses the CSC always */
> - if (!format_is_yuv(format))
> + if (!format_is_yuv(fb->format->format))
> return;
>
> /*
> - * BT.601 limited range YCbCr -> full range RGB
> + * BT.601 full range YCbCr -> full range RGB
> *
> - * |r| | 6537 4769 0| |cr |
> - * |g| = |-3330 4769 -1605| x |y-64|
> - * |b| | 0 4769 8263| |cb |
> + * |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, so no
> - * need for any offset. For Y we need to remove the offset.
> + * 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(-64));
> + 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(4769) | SPCSC_C0(6537));
> - I915_WRITE_FW(SPCSCC23(plane_id), SPCSC_C1(-3330) | SPCSC_C0(0));
> - I915_WRITE_FW(SPCSCC45(plane_id), SPCSC_C1(-1605) | SPCSC_C0(4769));
> - I915_WRITE_FW(SPCSCC67(plane_id), SPCSC_C1(4769) | SPCSC_C0(0));
> - I915_WRITE_FW(SPCSCC8(plane_id), SPCSC_C0(8263));
> + 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(SPCSCYGICLAMP(plane_id), SPCSC_IMAX(940) | SPCSC_IMIN(64));
> - I915_WRITE_FW(SPCSCCBICLAMP(plane_id), SPCSC_IMAX(448) | SPCSC_IMIN(-448));
> - I915_WRITE_FW(SPCSCCRICLAMP(plane_id), SPCSC_IMAX(448) | SPCSC_IMIN(-448));
> + I915_WRITE_FW(SPCSCYGICLAMP(plane_id), SPCSC_IMAX(1023) | SPCSC_IMIN(0));
> + I915_WRITE_FW(SPCSCCBICLAMP(plane_id), SPCSC_IMAX(512) | SPCSC_IMIN(-512));
> + I915_WRITE_FW(SPCSCCRICLAMP(plane_id), SPCSC_IMAX(512) | SPCSC_IMIN(-512));
>
> I915_WRITE_FW(SPCSCYGOCLAMP(plane_id), SPCSC_OMAX(1023) | SPCSC_OMIN(0));
> I915_WRITE_FW(SPCSCCBOCLAMP(plane_id), SPCSC_OMAX(1023) | SPCSC_OMIN(0));
> I915_WRITE_FW(SPCSCCROCLAMP(plane_id), SPCSC_OMAX(1023) | SPCSC_OMIN(0));
> }
>
> +static void
> +vlv_update_clrc(const struct intel_plane_state *plane_state)
> +{
> + struct intel_plane *plane = to_intel_plane(plane_state->base.plane);
> + struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> + const struct drm_framebuffer *fb = plane_state->base.fb;
> + enum pipe pipe = plane->pipe;
> + enum plane_id plane_id = plane->id;
> + int contrast, brightness, sh_sin, sh_cos;
> +
> + if (format_is_yuv(fb->format->format)) {
> + /*
> + * expand limited range to full range.
> + * contrast is applied first, then brightness
> + */
I would be happy to see some comment explaining the Brightness/Contrast
calculation magic nos, or may be a hint for other developers.
> + contrast = ((255 << 7) / 219 + 1) >> 1;
> + brightness = -((16 << 1) * 255 / 219 + 1) >> 1;
> + sh_sin = 0;
> + sh_cos = (((128 << 8) / 112) + 1) >> 1;
> + } else {
> + /* pass-through everything */
> + contrast = 1 << 6;
> + brightness = 0;
> + sh_sin = 0;
> + sh_cos = 1 << 7;
> + }
> +
> + /* FIXME these register are single buffered :( */
> + I915_WRITE_FW(SPCLRC0(pipe, plane_id),
> + SP_CONTRAST(contrast) | SP_BRIGHTNESS(brightness));
> + I915_WRITE_FW(SPCLRC1(pipe, plane_id),
> + SP_SH_SIN(sh_sin) | SP_SH_COS(sh_cos));
> +}
> +
> static u32 vlv_sprite_ctl(const struct intel_crtc_state *crtc_state,
> const struct intel_plane_state *plane_state)
> {
> @@ -456,8 +492,10 @@ vlv_update_plane(struct intel_plane *plane,
>
> spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
>
> + vlv_update_clrc(plane_state);
> +
> if (IS_CHERRYVIEW(dev_priv) && pipe == PIPE_B)
> - chv_update_csc(plane, fb->format->format);
> + chv_update_csc(plane_state);
>
> if (key->flags) {
> I915_WRITE_FW(SPKEYMINVAL(pipe, plane_id), key->min_value);
^ permalink raw reply [flat|nested] 27+ messages in thread
* ✗ Fi.CI.BAT: failure for sna: Add XV_COLORSPACE attribute support for sprite Xv adaptors (rev4)
2017-06-08 20:33 [PATCH 0/5] drm/i915; Support for COLOR_ENCODING and COLOR_RANGE props ville.syrjala
` (6 preceding siblings ...)
2017-06-08 21:01 ` ✗ Fi.CI.BAT: failure for " Patchwork
@ 2017-06-20 14:08 ` Patchwork
7 siblings, 0 replies; 27+ messages in thread
From: Patchwork @ 2017-06-20 14:08 UTC (permalink / raw)
To: ville.syrjala; +Cc: intel-gfx
== Series Details ==
Series: sna: Add XV_COLORSPACE attribute support for sprite Xv adaptors (rev4)
URL : https://patchwork.freedesktop.org/series/25505/
State : failure
== Summary ==
CHK include/config/kernel.release
CHK include/generated/uapi/linux/version.h
CHK include/generated/utsrelease.h
CHK include/generated/bounds.h
CHK include/generated/timeconst.h
CHK include/generated/asm-offsets.h
CALL scripts/checksyscalls.sh
CHK scripts/mod/devicetable-offsets.h
CHK include/generated/compile.h
CHK kernel/config_data.h
CC [M] drivers/gpu/drm/i915/intel_display.o
drivers/gpu/drm/i915/intel_display.c: In function ‘skl_plane_ctl’:
drivers/gpu/drm/i915/intel_display.c:3320:24: error: ‘const struct drm_plane_state’ has no member named ‘color_encoding’
if (plane_state->base.color_encoding == DRM_COLOR_YCBCR_BT709)
^
drivers/gpu/drm/i915/intel_display.c:3320:43: error: ‘DRM_COLOR_YCBCR_BT709’ undeclared (first use in this function)
if (plane_state->base.color_encoding == DRM_COLOR_YCBCR_BT709)
^
drivers/gpu/drm/i915/intel_display.c:3320:43: note: each undeclared identifier is reported only once for each function it appears in
drivers/gpu/drm/i915/intel_display.c:3323:24: error: ‘const struct drm_plane_state’ has no member named ‘color_range’
if (plane_state->base.color_range == DRM_COLOR_YCBCR_FULL_RANGE)
^
drivers/gpu/drm/i915/intel_display.c:3323:40: error: ‘DRM_COLOR_YCBCR_FULL_RANGE’ undeclared (first use in this function)
if (plane_state->base.color_range == DRM_COLOR_YCBCR_FULL_RANGE)
^
drivers/gpu/drm/i915/intel_display.c: In function ‘glk_color_ctl’:
drivers/gpu/drm/i915/intel_display.c:3349:24: error: ‘const struct drm_plane_state’ has no member named ‘color_encoding’
if (plane_state->base.color_encoding == DRM_COLOR_YCBCR_BT709)
^
drivers/gpu/drm/i915/intel_display.c:3349:43: error: ‘DRM_COLOR_YCBCR_BT709’ undeclared (first use in this function)
if (plane_state->base.color_encoding == DRM_COLOR_YCBCR_BT709)
^
drivers/gpu/drm/i915/intel_display.c:3354:24: error: ‘const struct drm_plane_state’ has no member named ‘color_range’
if (plane_state->base.color_range == DRM_COLOR_YCBCR_FULL_RANGE)
^
drivers/gpu/drm/i915/intel_display.c:3354:40: error: ‘DRM_COLOR_YCBCR_FULL_RANGE’ undeclared (first use in this function)
if (plane_state->base.color_range == DRM_COLOR_YCBCR_FULL_RANGE)
^
drivers/gpu/drm/i915/intel_display.c: In function ‘intel_primary_plane_create’:
drivers/gpu/drm/i915/intel_display.c:13853:3: error: implicit declaration of function ‘drm_plane_create_color_properties’ [-Werror=implicit-function-declaration]
drm_plane_create_color_properties(&primary->base,
^
In file included from ./include/linux/kernel.h:10:0,
from ./include/linux/list.h:8,
from ./include/linux/dmi.h:4,
from drivers/gpu/drm/i915/intel_display.c:27:
drivers/gpu/drm/i915/intel_display.c:13854:13: error: ‘DRM_COLOR_YCBCR_BT601’ undeclared (first use in this function)
BIT(DRM_COLOR_YCBCR_BT601) |
^
./include/linux/bitops.h:6:28: note: in definition of macro ‘BIT’
#define BIT(nr) (1UL << (nr))
^
drivers/gpu/drm/i915/intel_display.c:13855:13: error: ‘DRM_COLOR_YCBCR_BT709’ undeclared (first use in this function)
BIT(DRM_COLOR_YCBCR_BT709),
^
./include/linux/bitops.h:6:28: note: in definition of macro ‘BIT’
#define BIT(nr) (1UL << (nr))
^
drivers/gpu/drm/i915/intel_display.c:13856:13: error: ‘DRM_COLOR_YCBCR_LIMITED_RANGE’ undeclared (first use in this function)
BIT(DRM_COLOR_YCBCR_LIMITED_RANGE) |
^
./include/linux/bitops.h:6:28: note: in definition of macro ‘BIT’
#define BIT(nr) (1UL << (nr))
^
drivers/gpu/drm/i915/intel_display.c:13857:13: error: ‘DRM_COLOR_YCBCR_FULL_RANGE’ undeclared (first use in this function)
BIT(DRM_COLOR_YCBCR_FULL_RANGE),
^
./include/linux/bitops.h:6:28: note: in definition of macro ‘BIT’
#define BIT(nr) (1UL << (nr))
^
cc1: all warnings being treated as errors
scripts/Makefile.build:302: recipe for target 'drivers/gpu/drm/i915/intel_display.o' failed
make[4]: *** [drivers/gpu/drm/i915/intel_display.o] Error 1
scripts/Makefile.build:561: recipe for target 'drivers/gpu/drm/i915' failed
make[3]: *** [drivers/gpu/drm/i915] Error 2
scripts/Makefile.build:561: recipe for target 'drivers/gpu/drm' failed
make[2]: *** [drivers/gpu/drm] Error 2
scripts/Makefile.build:561: recipe for target 'drivers/gpu' failed
make[1]: *** [drivers/gpu] Error 2
Makefile:1016: recipe for target 'drivers' failed
make: *** [drivers] Error 2
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v3 1/5] drm/i915: Correctly handle limited range YCbCr data on VLV/CHV
2017-06-20 13:57 ` Sharma, Shashank
@ 2017-06-20 14:34 ` Ville Syrjälä
2017-06-20 14:43 ` Sharma, Shashank
0 siblings, 1 reply; 27+ messages in thread
From: Ville Syrjälä @ 2017-06-20 14:34 UTC (permalink / raw)
To: Sharma, Shashank; +Cc: intel-gfx, Tang, Jun, stable, Jyri Sarha
On Tue, Jun 20, 2017 at 07:27:54PM +0530, Sharma, Shashank wrote:
> Regards
> Shashank
>
> On 6/20/2017 7:02 PM, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Turns out the VLV/CHV fixed function sprite CSC expects full range
> > data as input. We've been feeding it limited range data to it all
> > along. To expand the data out to full range we'll use the color
> > correction registers (brightness, contrast, and saturation).
> >
> > On CHV pipe B we were actually doing the right thing already because we
> > progammed the custom CSC matrix to do expect limited range input. Now
> > that well pre-expand the data out with the color correction unit, we
> > need to change the CSC matrix to operate with full range input instead.
> >
> > This should make the sprite output of the other pipes match the sprite
> > output of pipe B reasonably well. Looking at the resulting pipe CRCs,
> > there can be a slight difference in the output, but as I don't know
> > the formula used by the fixed function CSC of the other pipes, I don't
> > think it's worth the effort to try to match the output exactly. It
> > might not even be possible due to difference in internal precision etc.
> >
> > One slight caveat here is that the color correction registers are single
> > bufferred, so we should really be updating them during vblank, but we
> > still don't have a mechanism for that, so just toss in another FIXME.
> >
> > v2: Rebase
> > v3: s/bri/brightness/ s/con/contrast/ (Shashank)
> >
> > Cc: Shashank Sharma <shashank.sharma@intel.com>
> > Cc: Jyri Sarha <jsarha@ti.com>
> > Cc: "Tang, Jun" <jun.tang@intel.com>
> > Reported-by: "Tang, Jun" <jun.tang@intel.com>
> > Cc: stable@vger.kernel.org
> > Fixes: 7f1f3851feb0 ("drm/i915: sprite support for ValleyView v4")
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> > drivers/gpu/drm/i915/i915_reg.h | 10 +++++
> > drivers/gpu/drm/i915/intel_sprite.c | 74 ++++++++++++++++++++++++++++---------
> > 2 files changed, 66 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index c8647cfa81ba..290322588f56 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -5974,6 +5974,12 @@ enum {
> > #define _SPATILEOFF (VLV_DISPLAY_BASE + 0x721a4)
> > #define _SPACONSTALPHA (VLV_DISPLAY_BASE + 0x721a8)
> > #define SP_CONST_ALPHA_ENABLE (1<<31)
> > +#define _SPACLRC0 (VLV_DISPLAY_BASE + 0x721d0)
> > +#define SP_CONTRAST(x) ((x) << 18) /* u3.6 */
> protection for higher reserved bits ? From 27-31 ? something like ((x &
> 1FF) << 18) ?
It's unsigned, so no need.
> > +#define SP_BRIGHTNESS(x) ((x) & 0xff) /* s8 */
> > +#define _SPACLRC1 (VLV_DISPLAY_BASE + 0x721d4)
> > +#define SP_SH_SIN(x) (((x) & 0x7ff) << 16) /* s4.7 */
> > +#define SP_SH_COS(x) (x) /* u3.7 */
> >
>
> #define SP_SH_COS(x) (x & 3FF) /* u3.7 */ ?
Also unsigned
>
> > #define _SPAGAMC (VLV_DISPLAY_BASE + 0x721f4)
> >
> > #define _SPBCNTR (VLV_DISPLAY_BASE + 0x72280)
> > @@ -5987,6 +5993,8 @@ enum {
> > #define _SPBKEYMAXVAL (VLV_DISPLAY_BASE + 0x722a0)
> > #define _SPBTILEOFF (VLV_DISPLAY_BASE + 0x722a4)
> > #define _SPBCONSTALPHA (VLV_DISPLAY_BASE + 0x722a8)
> > +#define _SPBCLRC0 (VLV_DISPLAY_BASE + 0x722d0)
> > +#define _SPBCLRC1 (VLV_DISPLAY_BASE + 0x722d4)
> > #define _SPBGAMC (VLV_DISPLAY_BASE + 0x722f4)
> >
> > #define _MMIO_VLV_SPR(pipe, plane_id, reg_a, reg_b) \
> > @@ -6003,6 +6011,8 @@ enum {
> > #define SPKEYMAXVAL(pipe, plane_id) _MMIO_VLV_SPR((pipe), (plane_id), _SPAKEYMAXVAL, _SPBKEYMAXVAL)
> > #define SPTILEOFF(pipe, plane_id) _MMIO_VLV_SPR((pipe), (plane_id), _SPATILEOFF, _SPBTILEOFF)
> > #define SPCONSTALPHA(pipe, plane_id) _MMIO_VLV_SPR((pipe), (plane_id), _SPACONSTALPHA, _SPBCONSTALPHA)
> > +#define SPCLRC0(pipe, plane_id) _MMIO_VLV_SPR((pipe), (plane_id), _SPACLRC0, _SPBCLRC0)
> > +#define SPCLRC1(pipe, plane_id) _MMIO_VLV_SPR((pipe), (plane_id), _SPACLRC1, _SPBCLRC1)
> > #define SPGAMC(pipe, plane_id) _MMIO_VLV_SPR((pipe), (plane_id), _SPAGAMC, _SPBGAMC)
> >
> > /*
> > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> > index 0c650c2cbca8..4462408cc835 100644
> > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > @@ -325,44 +325,80 @@ skl_disable_plane(struct intel_plane *plane, struct intel_crtc *crtc)
> > }
> >
> > static void
> > -chv_update_csc(struct intel_plane *plane, uint32_t format)
> > +chv_update_csc(const struct intel_plane_state *plane_state)
> > {
> > + struct intel_plane *plane = to_intel_plane(plane_state->base.plane);
> > 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;
> >
> > /* Seems RGB data bypasses the CSC always */
> > - if (!format_is_yuv(format))
> > + if (!format_is_yuv(fb->format->format))
> > return;
> >
> > /*
> > - * BT.601 limited range YCbCr -> full range RGB
> > + * BT.601 full range YCbCr -> full range RGB
> > *
> > - * |r| | 6537 4769 0| |cr |
> > - * |g| = |-3330 4769 -1605| x |y-64|
> > - * |b| | 0 4769 8263| |cb |
> > + * |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, so no
> > - * need for any offset. For Y we need to remove the offset.
> > + * 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(-64));
> > + 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(4769) | SPCSC_C0(6537));
> > - I915_WRITE_FW(SPCSCC23(plane_id), SPCSC_C1(-3330) | SPCSC_C0(0));
> > - I915_WRITE_FW(SPCSCC45(plane_id), SPCSC_C1(-1605) | SPCSC_C0(4769));
> > - I915_WRITE_FW(SPCSCC67(plane_id), SPCSC_C1(4769) | SPCSC_C0(0));
> > - I915_WRITE_FW(SPCSCC8(plane_id), SPCSC_C0(8263));
> > + 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(SPCSCYGICLAMP(plane_id), SPCSC_IMAX(940) | SPCSC_IMIN(64));
> > - I915_WRITE_FW(SPCSCCBICLAMP(plane_id), SPCSC_IMAX(448) | SPCSC_IMIN(-448));
> > - I915_WRITE_FW(SPCSCCRICLAMP(plane_id), SPCSC_IMAX(448) | SPCSC_IMIN(-448));
> > + I915_WRITE_FW(SPCSCYGICLAMP(plane_id), SPCSC_IMAX(1023) | SPCSC_IMIN(0));
> > + I915_WRITE_FW(SPCSCCBICLAMP(plane_id), SPCSC_IMAX(512) | SPCSC_IMIN(-512));
> > + I915_WRITE_FW(SPCSCCRICLAMP(plane_id), SPCSC_IMAX(512) | SPCSC_IMIN(-512));
> >
> > I915_WRITE_FW(SPCSCYGOCLAMP(plane_id), SPCSC_OMAX(1023) | SPCSC_OMIN(0));
> > I915_WRITE_FW(SPCSCCBOCLAMP(plane_id), SPCSC_OMAX(1023) | SPCSC_OMIN(0));
> > I915_WRITE_FW(SPCSCCROCLAMP(plane_id), SPCSC_OMAX(1023) | SPCSC_OMIN(0));
> > }
> >
> > +static void
> > +vlv_update_clrc(const struct intel_plane_state *plane_state)
> > +{
> > + struct intel_plane *plane = to_intel_plane(plane_state->base.plane);
> > + struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> > + const struct drm_framebuffer *fb = plane_state->base.fb;
> > + enum pipe pipe = plane->pipe;
> > + enum plane_id plane_id = plane->id;
> > + int contrast, brightness, sh_sin, sh_cos;
> > +
> > + if (format_is_yuv(fb->format->format)) {
> > + /*
> > + * expand limited range to full range.
> > + * contrast is applied first, then brightness
> > + */
> I would be happy to see some comment explaining the Brightness/Contrast
> calculation magic nos, or may be a hint for other developers.
Y: 16-235 * contrast + brightness -> ~0->255
CbCr: no hue adjustemnt -> 0 degree angle
scale to expand CbCr range from -112-112 to -128-128
sh_sin = sin(0) * 128/112 = 0
sh_cos = cos(0) * 128/112 = whatever
> > + contrast = ((255 << 7) / 219 + 1) >> 1;
> > + brightness = -((16 << 1) * 255 / 219 + 1) >> 1;
> > + sh_sin = 0;
> > + sh_cos = (((128 << 8) / 112) + 1) >> 1;
> > + } else {
> > + /* pass-through everything */
> > + contrast = 1 << 6;
> > + brightness = 0;
> > + sh_sin = 0;
> > + sh_cos = 1 << 7;
> > + }
> > +
> > + /* FIXME these register are single buffered :( */
> > + I915_WRITE_FW(SPCLRC0(pipe, plane_id),
> > + SP_CONTRAST(contrast) | SP_BRIGHTNESS(brightness));
> > + I915_WRITE_FW(SPCLRC1(pipe, plane_id),
> > + SP_SH_SIN(sh_sin) | SP_SH_COS(sh_cos));
> > +}
> > +
> > static u32 vlv_sprite_ctl(const struct intel_crtc_state *crtc_state,
> > const struct intel_plane_state *plane_state)
> > {
> > @@ -456,8 +492,10 @@ vlv_update_plane(struct intel_plane *plane,
> >
> > spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
> >
> > + vlv_update_clrc(plane_state);
> > +
> > if (IS_CHERRYVIEW(dev_priv) && pipe == PIPE_B)
> > - chv_update_csc(plane, fb->format->format);
> > + chv_update_csc(plane_state);
> >
> > if (key->flags) {
> > I915_WRITE_FW(SPKEYMINVAL(pipe, plane_id), key->min_value);
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v3 1/5] drm/i915: Correctly handle limited range YCbCr data on VLV/CHV
2017-06-20 14:34 ` Ville Syrjälä
@ 2017-06-20 14:43 ` Sharma, Shashank
2017-06-20 14:55 ` Ville Syrjälä
0 siblings, 1 reply; 27+ messages in thread
From: Sharma, Shashank @ 2017-06-20 14:43 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: intel-gfx, Tang, Jun, stable, Jyri Sarha
Regards
Shashank
On 6/20/2017 8:04 PM, Ville Syrjälä wrote:
> On Tue, Jun 20, 2017 at 07:27:54PM +0530, Sharma, Shashank wrote:
>> Regards
>> Shashank
>>
>> On 6/20/2017 7:02 PM, ville.syrjala@linux.intel.com wrote:
>>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>
>>> Turns out the VLV/CHV fixed function sprite CSC expects full range
>>> data as input. We've been feeding it limited range data to it all
>>> along. To expand the data out to full range we'll use the color
>>> correction registers (brightness, contrast, and saturation).
>>>
>>> On CHV pipe B we were actually doing the right thing already because we
>>> progammed the custom CSC matrix to do expect limited range input. Now
>>> that well pre-expand the data out with the color correction unit, we
>>> need to change the CSC matrix to operate with full range input instead.
>>>
>>> This should make the sprite output of the other pipes match the sprite
>>> output of pipe B reasonably well. Looking at the resulting pipe CRCs,
>>> there can be a slight difference in the output, but as I don't know
>>> the formula used by the fixed function CSC of the other pipes, I don't
>>> think it's worth the effort to try to match the output exactly. It
>>> might not even be possible due to difference in internal precision etc.
>>>
>>> One slight caveat here is that the color correction registers are single
>>> bufferred, so we should really be updating them during vblank, but we
>>> still don't have a mechanism for that, so just toss in another FIXME.
>>>
>>> v2: Rebase
>>> v3: s/bri/brightness/ s/con/contrast/ (Shashank)
>>>
>>> Cc: Shashank Sharma <shashank.sharma@intel.com>
>>> Cc: Jyri Sarha <jsarha@ti.com>
>>> Cc: "Tang, Jun" <jun.tang@intel.com>
>>> Reported-by: "Tang, Jun" <jun.tang@intel.com>
>>> Cc: stable@vger.kernel.org
>>> Fixes: 7f1f3851feb0 ("drm/i915: sprite support for ValleyView v4")
>>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>> ---
>>> drivers/gpu/drm/i915/i915_reg.h | 10 +++++
>>> drivers/gpu/drm/i915/intel_sprite.c | 74 ++++++++++++++++++++++++++++---------
>>> 2 files changed, 66 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>>> index c8647cfa81ba..290322588f56 100644
>>> --- a/drivers/gpu/drm/i915/i915_reg.h
>>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>>> @@ -5974,6 +5974,12 @@ enum {
>>> #define _SPATILEOFF (VLV_DISPLAY_BASE + 0x721a4)
>>> #define _SPACONSTALPHA (VLV_DISPLAY_BASE + 0x721a8)
>>> #define SP_CONST_ALPHA_ENABLE (1<<31)
>>> +#define _SPACLRC0 (VLV_DISPLAY_BASE + 0x721d0)
>>> +#define SP_CONTRAST(x) ((x) << 18) /* u3.6 */
>> protection for higher reserved bits ? From 27-31 ? something like ((x &
>> 1FF) << 18) ?
> It's unsigned, so no need.
Humm .. doesn't stops me to pass a bigger value, which will be shifted
towards reserved bits ?
>
>>> +#define SP_BRIGHTNESS(x) ((x) & 0xff) /* s8 */
>>> +#define _SPACLRC1 (VLV_DISPLAY_BASE + 0x721d4)
>>> +#define SP_SH_SIN(x) (((x) & 0x7ff) << 16) /* s4.7 */
>>> +#define SP_SH_COS(x) (x) /* u3.7 */
>>>
>> #define SP_SH_COS(x) (x & 3FF) /* u3.7 */ ?
> Also unsigned
>
>>> #define _SPAGAMC (VLV_DISPLAY_BASE + 0x721f4)
>>>
>>> #define _SPBCNTR (VLV_DISPLAY_BASE + 0x72280)
>>> @@ -5987,6 +5993,8 @@ enum {
>>> #define _SPBKEYMAXVAL (VLV_DISPLAY_BASE + 0x722a0)
>>> #define _SPBTILEOFF (VLV_DISPLAY_BASE + 0x722a4)
>>> #define _SPBCONSTALPHA (VLV_DISPLAY_BASE + 0x722a8)
>>> +#define _SPBCLRC0 (VLV_DISPLAY_BASE + 0x722d0)
>>> +#define _SPBCLRC1 (VLV_DISPLAY_BASE + 0x722d4)
>>> #define _SPBGAMC (VLV_DISPLAY_BASE + 0x722f4)
>>>
>>> #define _MMIO_VLV_SPR(pipe, plane_id, reg_a, reg_b) \
>>> @@ -6003,6 +6011,8 @@ enum {
>>> #define SPKEYMAXVAL(pipe, plane_id) _MMIO_VLV_SPR((pipe), (plane_id), _SPAKEYMAXVAL, _SPBKEYMAXVAL)
>>> #define SPTILEOFF(pipe, plane_id) _MMIO_VLV_SPR((pipe), (plane_id), _SPATILEOFF, _SPBTILEOFF)
>>> #define SPCONSTALPHA(pipe, plane_id) _MMIO_VLV_SPR((pipe), (plane_id), _SPACONSTALPHA, _SPBCONSTALPHA)
>>> +#define SPCLRC0(pipe, plane_id) _MMIO_VLV_SPR((pipe), (plane_id), _SPACLRC0, _SPBCLRC0)
>>> +#define SPCLRC1(pipe, plane_id) _MMIO_VLV_SPR((pipe), (plane_id), _SPACLRC1, _SPBCLRC1)
>>> #define SPGAMC(pipe, plane_id) _MMIO_VLV_SPR((pipe), (plane_id), _SPAGAMC, _SPBGAMC)
>>>
>>> /*
>>> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
>>> index 0c650c2cbca8..4462408cc835 100644
>>> --- a/drivers/gpu/drm/i915/intel_sprite.c
>>> +++ b/drivers/gpu/drm/i915/intel_sprite.c
>>> @@ -325,44 +325,80 @@ skl_disable_plane(struct intel_plane *plane, struct intel_crtc *crtc)
>>> }
>>>
>>> static void
>>> -chv_update_csc(struct intel_plane *plane, uint32_t format)
>>> +chv_update_csc(const struct intel_plane_state *plane_state)
>>> {
>>> + struct intel_plane *plane = to_intel_plane(plane_state->base.plane);
>>> 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;
>>>
>>> /* Seems RGB data bypasses the CSC always */
>>> - if (!format_is_yuv(format))
>>> + if (!format_is_yuv(fb->format->format))
>>> return;
>>>
>>> /*
>>> - * BT.601 limited range YCbCr -> full range RGB
>>> + * BT.601 full range YCbCr -> full range RGB
>>> *
>>> - * |r| | 6537 4769 0| |cr |
>>> - * |g| = |-3330 4769 -1605| x |y-64|
>>> - * |b| | 0 4769 8263| |cb |
>>> + * |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, so no
>>> - * need for any offset. For Y we need to remove the offset.
>>> + * 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(-64));
>>> + 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(4769) | SPCSC_C0(6537));
>>> - I915_WRITE_FW(SPCSCC23(plane_id), SPCSC_C1(-3330) | SPCSC_C0(0));
>>> - I915_WRITE_FW(SPCSCC45(plane_id), SPCSC_C1(-1605) | SPCSC_C0(4769));
>>> - I915_WRITE_FW(SPCSCC67(plane_id), SPCSC_C1(4769) | SPCSC_C0(0));
>>> - I915_WRITE_FW(SPCSCC8(plane_id), SPCSC_C0(8263));
>>> + 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(SPCSCYGICLAMP(plane_id), SPCSC_IMAX(940) | SPCSC_IMIN(64));
>>> - I915_WRITE_FW(SPCSCCBICLAMP(plane_id), SPCSC_IMAX(448) | SPCSC_IMIN(-448));
>>> - I915_WRITE_FW(SPCSCCRICLAMP(plane_id), SPCSC_IMAX(448) | SPCSC_IMIN(-448));
>>> + I915_WRITE_FW(SPCSCYGICLAMP(plane_id), SPCSC_IMAX(1023) | SPCSC_IMIN(0));
>>> + I915_WRITE_FW(SPCSCCBICLAMP(plane_id), SPCSC_IMAX(512) | SPCSC_IMIN(-512));
>>> + I915_WRITE_FW(SPCSCCRICLAMP(plane_id), SPCSC_IMAX(512) | SPCSC_IMIN(-512));
>>>
>>> I915_WRITE_FW(SPCSCYGOCLAMP(plane_id), SPCSC_OMAX(1023) | SPCSC_OMIN(0));
>>> I915_WRITE_FW(SPCSCCBOCLAMP(plane_id), SPCSC_OMAX(1023) | SPCSC_OMIN(0));
>>> I915_WRITE_FW(SPCSCCROCLAMP(plane_id), SPCSC_OMAX(1023) | SPCSC_OMIN(0));
>>> }
>>>
>>> +static void
>>> +vlv_update_clrc(const struct intel_plane_state *plane_state)
>>> +{
>>> + struct intel_plane *plane = to_intel_plane(plane_state->base.plane);
>>> + struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
>>> + const struct drm_framebuffer *fb = plane_state->base.fb;
>>> + enum pipe pipe = plane->pipe;
>>> + enum plane_id plane_id = plane->id;
>>> + int contrast, brightness, sh_sin, sh_cos;
>>> +
>>> + if (format_is_yuv(fb->format->format)) {
>>> + /*
>>> + * expand limited range to full range.
>>> + * contrast is applied first, then brightness
>>> + */
>> I would be happy to see some comment explaining the Brightness/Contrast
>> calculation magic nos, or may be a hint for other developers.
> Y: 16-235 * contrast + brightness -> ~0->255
> CbCr: no hue adjustemnt -> 0 degree angle
> scale to expand CbCr range from -112-112 to -128-128
> sh_sin = sin(0) * 128/112 = 0
> sh_cos = cos(0) * 128/112 = whatever
Yep, (fortunately I am aware of this formula :-)), this same in form of
a comment ?
With of without this comment, feel free to use:
R-B: Shashank Sharma <shashank.sharma@intel.com>
>>> + contrast = ((255 << 7) / 219 + 1) >> 1;
>>> + brightness = -((16 << 1) * 255 / 219 + 1) >> 1;
>>> + sh_sin = 0;
>>> + sh_cos = (((128 << 8) / 112) + 1) >> 1;
>>> + } else {
>>> + /* pass-through everything */
>>> + contrast = 1 << 6;
>>> + brightness = 0;
>>> + sh_sin = 0;
>>> + sh_cos = 1 << 7;
>>> + }
>>> +
>>> + /* FIXME these register are single buffered :( */
>>> + I915_WRITE_FW(SPCLRC0(pipe, plane_id),
>>> + SP_CONTRAST(contrast) | SP_BRIGHTNESS(brightness));
>>> + I915_WRITE_FW(SPCLRC1(pipe, plane_id),
>>> + SP_SH_SIN(sh_sin) | SP_SH_COS(sh_cos));
>>> +}
>>> +
>>> static u32 vlv_sprite_ctl(const struct intel_crtc_state *crtc_state,
>>> const struct intel_plane_state *plane_state)
>>> {
>>> @@ -456,8 +492,10 @@ vlv_update_plane(struct intel_plane *plane,
>>>
>>> spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
>>>
>>> + vlv_update_clrc(plane_state);
>>> +
>>> if (IS_CHERRYVIEW(dev_priv) && pipe == PIPE_B)
>>> - chv_update_csc(plane, fb->format->format);
>>> + chv_update_csc(plane_state);
>>>
>>> if (key->flags) {
>>> I915_WRITE_FW(SPKEYMINVAL(pipe, plane_id), key->min_value);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v3 1/5] drm/i915: Correctly handle limited range YCbCr data on VLV/CHV
2017-06-20 14:43 ` Sharma, Shashank
@ 2017-06-20 14:55 ` Ville Syrjälä
0 siblings, 0 replies; 27+ messages in thread
From: Ville Syrjälä @ 2017-06-20 14:55 UTC (permalink / raw)
To: Sharma, Shashank; +Cc: intel-gfx, Jyri Sarha, Tang, Jun, stable
On Tue, Jun 20, 2017 at 08:13:42PM +0530, Sharma, Shashank wrote:
> Regards
>
> Shashank
>
>
> On 6/20/2017 8:04 PM, Ville Syrjälä wrote:
> > On Tue, Jun 20, 2017 at 07:27:54PM +0530, Sharma, Shashank wrote:
> >> Regards
> >> Shashank
> >>
> >> On 6/20/2017 7:02 PM, ville.syrjala@linux.intel.com wrote:
> >>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>>
> >>> Turns out the VLV/CHV fixed function sprite CSC expects full range
> >>> data as input. We've been feeding it limited range data to it all
> >>> along. To expand the data out to full range we'll use the color
> >>> correction registers (brightness, contrast, and saturation).
> >>>
> >>> On CHV pipe B we were actually doing the right thing already because we
> >>> progammed the custom CSC matrix to do expect limited range input. Now
> >>> that well pre-expand the data out with the color correction unit, we
> >>> need to change the CSC matrix to operate with full range input instead.
> >>>
> >>> This should make the sprite output of the other pipes match the sprite
> >>> output of pipe B reasonably well. Looking at the resulting pipe CRCs,
> >>> there can be a slight difference in the output, but as I don't know
> >>> the formula used by the fixed function CSC of the other pipes, I don't
> >>> think it's worth the effort to try to match the output exactly. It
> >>> might not even be possible due to difference in internal precision etc.
> >>>
> >>> One slight caveat here is that the color correction registers are single
> >>> bufferred, so we should really be updating them during vblank, but we
> >>> still don't have a mechanism for that, so just toss in another FIXME.
> >>>
> >>> v2: Rebase
> >>> v3: s/bri/brightness/ s/con/contrast/ (Shashank)
> >>>
> >>> Cc: Shashank Sharma <shashank.sharma@intel.com>
> >>> Cc: Jyri Sarha <jsarha@ti.com>
> >>> Cc: "Tang, Jun" <jun.tang@intel.com>
> >>> Reported-by: "Tang, Jun" <jun.tang@intel.com>
> >>> Cc: stable@vger.kernel.org
> >>> Fixes: 7f1f3851feb0 ("drm/i915: sprite support for ValleyView v4")
> >>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>> ---
> >>> drivers/gpu/drm/i915/i915_reg.h | 10 +++++
> >>> drivers/gpu/drm/i915/intel_sprite.c | 74 ++++++++++++++++++++++++++++---------
> >>> 2 files changed, 66 insertions(+), 18 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> >>> index c8647cfa81ba..290322588f56 100644
> >>> --- a/drivers/gpu/drm/i915/i915_reg.h
> >>> +++ b/drivers/gpu/drm/i915/i915_reg.h
> >>> @@ -5974,6 +5974,12 @@ enum {
> >>> #define _SPATILEOFF (VLV_DISPLAY_BASE + 0x721a4)
> >>> #define _SPACONSTALPHA (VLV_DISPLAY_BASE + 0x721a8)
> >>> #define SP_CONST_ALPHA_ENABLE (1<<31)
> >>> +#define _SPACLRC0 (VLV_DISPLAY_BASE + 0x721d0)
> >>> +#define SP_CONTRAST(x) ((x) << 18) /* u3.6 */
> >> protection for higher reserved bits ? From 27-31 ? something like ((x &
> >> 1FF) << 18) ?
> > It's unsigned, so no need.
> Humm .. doesn't stops me to pass a bigger value, which will be shifted
> towards reserved bits ?
We don't hand hold developers quite that much. I did have some ideas in
the past that we might want to have some optional debug checks in these
things to trigger WARNs if a value overflows its bits. But I never got
as far as implementing anything like that.
> >
> >>> +#define SP_BRIGHTNESS(x) ((x) & 0xff) /* s8 */
> >>> +#define _SPACLRC1 (VLV_DISPLAY_BASE + 0x721d4)
> >>> +#define SP_SH_SIN(x) (((x) & 0x7ff) << 16) /* s4.7 */
> >>> +#define SP_SH_COS(x) (x) /* u3.7 */
> >>>
> >> #define SP_SH_COS(x) (x & 3FF) /* u3.7 */ ?
> > Also unsigned
> >
> >>> #define _SPAGAMC (VLV_DISPLAY_BASE + 0x721f4)
> >>>
> >>> #define _SPBCNTR (VLV_DISPLAY_BASE + 0x72280)
> >>> @@ -5987,6 +5993,8 @@ enum {
> >>> #define _SPBKEYMAXVAL (VLV_DISPLAY_BASE + 0x722a0)
> >>> #define _SPBTILEOFF (VLV_DISPLAY_BASE + 0x722a4)
> >>> #define _SPBCONSTALPHA (VLV_DISPLAY_BASE + 0x722a8)
> >>> +#define _SPBCLRC0 (VLV_DISPLAY_BASE + 0x722d0)
> >>> +#define _SPBCLRC1 (VLV_DISPLAY_BASE + 0x722d4)
> >>> #define _SPBGAMC (VLV_DISPLAY_BASE + 0x722f4)
> >>>
> >>> #define _MMIO_VLV_SPR(pipe, plane_id, reg_a, reg_b) \
> >>> @@ -6003,6 +6011,8 @@ enum {
> >>> #define SPKEYMAXVAL(pipe, plane_id) _MMIO_VLV_SPR((pipe), (plane_id), _SPAKEYMAXVAL, _SPBKEYMAXVAL)
> >>> #define SPTILEOFF(pipe, plane_id) _MMIO_VLV_SPR((pipe), (plane_id), _SPATILEOFF, _SPBTILEOFF)
> >>> #define SPCONSTALPHA(pipe, plane_id) _MMIO_VLV_SPR((pipe), (plane_id), _SPACONSTALPHA, _SPBCONSTALPHA)
> >>> +#define SPCLRC0(pipe, plane_id) _MMIO_VLV_SPR((pipe), (plane_id), _SPACLRC0, _SPBCLRC0)
> >>> +#define SPCLRC1(pipe, plane_id) _MMIO_VLV_SPR((pipe), (plane_id), _SPACLRC1, _SPBCLRC1)
> >>> #define SPGAMC(pipe, plane_id) _MMIO_VLV_SPR((pipe), (plane_id), _SPAGAMC, _SPBGAMC)
> >>>
> >>> /*
> >>> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> >>> index 0c650c2cbca8..4462408cc835 100644
> >>> --- a/drivers/gpu/drm/i915/intel_sprite.c
> >>> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> >>> @@ -325,44 +325,80 @@ skl_disable_plane(struct intel_plane *plane, struct intel_crtc *crtc)
> >>> }
> >>>
> >>> static void
> >>> -chv_update_csc(struct intel_plane *plane, uint32_t format)
> >>> +chv_update_csc(const struct intel_plane_state *plane_state)
> >>> {
> >>> + struct intel_plane *plane = to_intel_plane(plane_state->base.plane);
> >>> 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;
> >>>
> >>> /* Seems RGB data bypasses the CSC always */
> >>> - if (!format_is_yuv(format))
> >>> + if (!format_is_yuv(fb->format->format))
> >>> return;
> >>>
> >>> /*
> >>> - * BT.601 limited range YCbCr -> full range RGB
> >>> + * BT.601 full range YCbCr -> full range RGB
> >>> *
> >>> - * |r| | 6537 4769 0| |cr |
> >>> - * |g| = |-3330 4769 -1605| x |y-64|
> >>> - * |b| | 0 4769 8263| |cb |
> >>> + * |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, so no
> >>> - * need for any offset. For Y we need to remove the offset.
> >>> + * 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(-64));
> >>> + 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(4769) | SPCSC_C0(6537));
> >>> - I915_WRITE_FW(SPCSCC23(plane_id), SPCSC_C1(-3330) | SPCSC_C0(0));
> >>> - I915_WRITE_FW(SPCSCC45(plane_id), SPCSC_C1(-1605) | SPCSC_C0(4769));
> >>> - I915_WRITE_FW(SPCSCC67(plane_id), SPCSC_C1(4769) | SPCSC_C0(0));
> >>> - I915_WRITE_FW(SPCSCC8(plane_id), SPCSC_C0(8263));
> >>> + 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(SPCSCYGICLAMP(plane_id), SPCSC_IMAX(940) | SPCSC_IMIN(64));
> >>> - I915_WRITE_FW(SPCSCCBICLAMP(plane_id), SPCSC_IMAX(448) | SPCSC_IMIN(-448));
> >>> - I915_WRITE_FW(SPCSCCRICLAMP(plane_id), SPCSC_IMAX(448) | SPCSC_IMIN(-448));
> >>> + I915_WRITE_FW(SPCSCYGICLAMP(plane_id), SPCSC_IMAX(1023) | SPCSC_IMIN(0));
> >>> + I915_WRITE_FW(SPCSCCBICLAMP(plane_id), SPCSC_IMAX(512) | SPCSC_IMIN(-512));
> >>> + I915_WRITE_FW(SPCSCCRICLAMP(plane_id), SPCSC_IMAX(512) | SPCSC_IMIN(-512));
> >>>
> >>> I915_WRITE_FW(SPCSCYGOCLAMP(plane_id), SPCSC_OMAX(1023) | SPCSC_OMIN(0));
> >>> I915_WRITE_FW(SPCSCCBOCLAMP(plane_id), SPCSC_OMAX(1023) | SPCSC_OMIN(0));
> >>> I915_WRITE_FW(SPCSCCROCLAMP(plane_id), SPCSC_OMAX(1023) | SPCSC_OMIN(0));
> >>> }
> >>>
> >>> +static void
> >>> +vlv_update_clrc(const struct intel_plane_state *plane_state)
> >>> +{
> >>> + struct intel_plane *plane = to_intel_plane(plane_state->base.plane);
> >>> + struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> >>> + const struct drm_framebuffer *fb = plane_state->base.fb;
> >>> + enum pipe pipe = plane->pipe;
> >>> + enum plane_id plane_id = plane->id;
> >>> + int contrast, brightness, sh_sin, sh_cos;
> >>> +
> >>> + if (format_is_yuv(fb->format->format)) {
> >>> + /*
> >>> + * expand limited range to full range.
> >>> + * contrast is applied first, then brightness
> >>> + */
> >> I would be happy to see some comment explaining the Brightness/Contrast
> >> calculation magic nos, or may be a hint for other developers.
> > Y: 16-235 * contrast + brightness -> ~0->255
> > CbCr: no hue adjustemnt -> 0 degree angle
> > scale to expand CbCr range from -112-112 to -128-128
> > sh_sin = sin(0) * 128/112 = 0
> > sh_cos = cos(0) * 128/112 = whatever
> Yep, (fortunately I am aware of this formula :-)), this same in form of
> a comment ?
Perhaps. If I can write it in a way that doesn't just confuse people
more :P
> With of without this comment, feel free to use:
> R-B: Shashank Sharma <shashank.sharma@intel.com>
> >>> + contrast = ((255 << 7) / 219 + 1) >> 1;
> >>> + brightness = -((16 << 1) * 255 / 219 + 1) >> 1;
> >>> + sh_sin = 0;
> >>> + sh_cos = (((128 << 8) / 112) + 1) >> 1;
> >>> + } else {
> >>> + /* pass-through everything */
> >>> + contrast = 1 << 6;
> >>> + brightness = 0;
> >>> + sh_sin = 0;
> >>> + sh_cos = 1 << 7;
> >>> + }
> >>> +
> >>> + /* FIXME these register are single buffered :( */
> >>> + I915_WRITE_FW(SPCLRC0(pipe, plane_id),
> >>> + SP_CONTRAST(contrast) | SP_BRIGHTNESS(brightness));
> >>> + I915_WRITE_FW(SPCLRC1(pipe, plane_id),
> >>> + SP_SH_SIN(sh_sin) | SP_SH_COS(sh_cos));
> >>> +}
> >>> +
> >>> static u32 vlv_sprite_ctl(const struct intel_crtc_state *crtc_state,
> >>> const struct intel_plane_state *plane_state)
> >>> {
> >>> @@ -456,8 +492,10 @@ vlv_update_plane(struct intel_plane *plane,
> >>>
> >>> spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
> >>>
> >>> + vlv_update_clrc(plane_state);
> >>> +
> >>> if (IS_CHERRYVIEW(dev_priv) && pipe == PIPE_B)
> >>> - chv_update_csc(plane, fb->format->format);
> >>> + chv_update_csc(plane_state);
> >>>
> >>> if (key->flags) {
> >>> I915_WRITE_FW(SPKEYMINVAL(pipe, plane_id), key->min_value);
--
Ville Syrjälä
Intel OTC
^ permalink raw reply [flat|nested] 27+ messages in thread
end of thread, other threads:[~2017-06-20 14:55 UTC | newest]
Thread overview: 27+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-06-08 20:33 [PATCH 0/5] drm/i915; Support for COLOR_ENCODING and COLOR_RANGE props ville.syrjala
2017-06-08 20:33 ` [PATCH v2 1/5] drm/i915: Correctly handle limited range YCbCr data on VLV/CHV ville.syrjala
2017-06-15 12:38 ` Sharma, Shashank
2017-06-15 12:46 ` Ville Syrjälä
2017-06-20 13:32 ` [PATCH v3 " ville.syrjala
2017-06-20 13:57 ` Sharma, Shashank
2017-06-20 14:34 ` Ville Syrjälä
2017-06-20 14:43 ` Sharma, Shashank
2017-06-20 14:55 ` Ville Syrjälä
2017-06-08 20:33 ` [PATCH 2/5] drm/i915: Fix plane YCbCr->RGB conversion for GLK ville.syrjala
2017-06-15 12:27 ` Sharma, Shashank
2017-06-15 12:42 ` Ville Syrjälä
2017-06-15 13:28 ` Sharma, Shashank
2017-06-16 13:03 ` Ville Syrjälä
2017-06-08 20:33 ` [PATCH 3/5] drm/i915: Add support for the YCbCr COLOR_ENCODING property ville.syrjala
2017-06-15 13:22 ` Sharma, Shashank
2017-06-16 13:02 ` Ville Syrjälä
2017-06-20 13:33 ` [PATCH v2 " ville.syrjala
2017-06-20 13:43 ` Sharma, Shashank
2017-06-08 20:33 ` [PATCH 4/5] drm/i915: Change the COLOR_ENCODING prop default value to BT.709 ville.syrjala
2017-06-15 13:31 ` Sharma, Shashank
2017-06-08 20:33 ` [PATCH 5/5] drm/i915: Add support for the YCbCr COLOR_RANGE property ville.syrjala
2017-06-15 13:37 ` Sharma, Shashank
2017-06-20 13:35 ` [PATCH v2 " ville.syrjala
2017-06-08 20:33 ` [PATCH xf86-video-intel] sna: Add XV_COLORSPACE attribute support for sprite Xv adaptors ville.syrjala
2017-06-08 21:01 ` ✗ Fi.CI.BAT: failure for " Patchwork
2017-06-20 14:08 ` ✗ Fi.CI.BAT: failure for sna: Add XV_COLORSPACE attribute support for sprite Xv adaptors (rev4) Patchwork
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).