On Thu, 2019-07-18 at 19:45 +0300, Ville Syrjala wrote:

From: Ville Syrjälä ville.syrjala@linux.intel.com

crtc_state->limited_color_range only applies to RGB output but we're currently setting it even for YCbCr output. That will lead to conflicting MSA and PIPECONF settings which can mess up the image. Let's make sure limited_color_range stays unset with YCbCr output.

Also WARN if we end up with such a bogus combination when programming the MSA MISC bits as it's impossible to even indicate quantization rangle for YCbCr via MSA MISC. YCbCr output is simply assumed to be limited range always. Note that VSC SDP does provide a mechanism for full range YCbCr, so in the future we may want to rethink how we compute/store this state.

And for good measure we add the same WARN to the HDMI path.

v2: s/==/!=/ in the HDMI WARN

Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com

drivers/gpu/drm/i915/display/intel_ddi.c | 10 +++++++--- drivers/gpu/drm/i915/display/intel_dp.c | 10 ++++++++++ drivers/gpu/drm/i915/display/intel_hdmi.c | 20 +++++++++++++++++--- 3 files changed, 34 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c index 157c5851a688..7dd54f573f35 100644 --- a/drivers/gpu/drm/i915/display/intel_ddi.c +++ b/drivers/gpu/drm/i915/display/intel_ddi.c @@ -1706,9 +1706,6 @@ void intel_ddi_set_pipe_settings(const struct intel_crtc_state *crtc_state)

temp = TRANS_MSA_SYNC_CLK;

• if (crtc_state->limited_color_range)

• temp |= TRANS_MSA_CEA_RANGE;
  

• switch (crtc_state->pipe_bpp) { case 18: temp |= TRANS_MSA_6_BPC;

@@ -1727,6 +1724,13 @@ void intel_ddi_set_pipe_settings(const struct intel_crtc_state *crtc_state) break; }

• /* nonsense combination */
• WARN_ON(crtc_state->limited_color_range &&

• crtc_state->output_format != INTEL_OUTPUT_FORMAT_RGB);
  

• if (crtc_state->limited_color_range)

• temp |= TRANS_MSA_CEA_RANGE;
  

• /*
  • As per DP 1.2 spec section 2.3.4.3 while sending
  • YCBCR 444 signals we should program MSA MISC1/0 fields with

diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c index 0eb5d66f87a7..84d2724f0854 100644 --- a/drivers/gpu/drm/i915/display/intel_dp.c +++ b/drivers/gpu/drm/i915/display/intel_dp.c @@ -2126,6 +2126,16 @@ bool intel_dp_limited_color_range(const struct intel_crtc_state *crtc_state, const struct drm_display_mode *adjusted_mode = &crtc_state->base.adjusted_mode;

• /*

• * Our YCbCr output is always limited range.
  

• * crtc_state->limited_color_range only applies to RGB,
  

• * and it must never be set for YCbCr or we risk setting
  

• * some conflicting bits in PIPECONF which will mess up
  

• * the colors on the monitor.
  

• */
  

• if (crtc_state->output_format != INTEL_OUTPUT_FORMAT_RGB)

• return false;
  

• if (intel_conn_state->broadcast_rgb ==

INTEL_BROADCAST_RGB_AUTO) { /* * See: diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c index ca377ba3a15e..325abd462a46 100644 --- a/drivers/gpu/drm/i915/display/intel_hdmi.c +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c @@ -724,6 +724,10 @@ intel_hdmi_compute_avi_infoframe(struct intel_encoder *encoder,

drm_hdmi_avi_infoframe_colorspace(frame, conn_state);

• /* nonsense combination */
• WARN_ON(crtc_state->limited_color_range &&

• crtc_state->output_format != INTEL_OUTPUT_FORMAT_RGB);
  

• if (crtc_state->output_format == INTEL_OUTPUT_FORMAT_RGB) { drm_hdmi_avi_infoframe_quant_range(frame, connector, adjusted_mode,

@@ -2305,6 +2309,16 @@ static bool intel_hdmi_limited_color_range(const struct intel_crtc_state *crtc_s const struct drm_display_mode *adjusted_mode = &crtc_state->base.adjusted_mode;

• /*

• * Our YCbCr output is always limited range.
  

• * crtc_state->limited_color_range only applies to RGB,
  

• * and it must never be set for YCbCr or we risk setting
  

• * some conflicting bits in PIPECONF which will mess up
  

• * the colors on the monitor.
  

• */
  

• if (crtc_state->output_format != INTEL_OUTPUT_FORMAT_RGB)

• return false;
  

• if (intel_conn_state->broadcast_rgb ==

INTEL_BROADCAST_RGB_AUTO) { /* See CEA-861-E - 5.1 Default Encoding Parameters */ return crtc_state->has_hdmi_sink && @@ -2341,9 +2355,6 @@ int intel_hdmi_compute_config(struct intel_encoder *encoder, if (pipe_config->has_hdmi_sink) pipe_config->has_infoframe = true;

• pipe_config->limited_color_range =

• intel_hdmi_limited_color_range(pipe_config,
  

conn_state);

• if (adjusted_mode->flags & DRM_MODE_FLAG_DBLCLK) { pipe_config->pixel_multiplier = 2; clock_8bpc *= 2;

@@ -2360,6 +2371,9 @@ int intel_hdmi_compute_config(struct intel_encoder *encoder, } }

• pipe_config->limited_color_range =

• intel_hdmi_limited_color_range(pipe_config,
  

conn_state);

• if (HAS_PCH_SPLIT(dev_priv) && !HAS_DDI(dev_priv)) pipe_config->has_pch_encoder = true;

The changes look good to me. Reviewed-by: Gwan-gyeong Mun gwan-gyeong.mun@intel.com

On Thu, 2019-07-18 at 17:50 +0300, Ville Syrjala wrote:

From: Ville Syrjälä ville.syrjala@linux.intel.com

Now that we have standard defines for the MSA MISC bits lets use them on HSW+ where we program these directly into the TRANS_MSA_MISC register.

Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com

drivers/gpu/drm/i915/display/intel_ddi.c | 18 +++++++++--------- drivers/gpu/drm/i915/i915_reg.h | 13 +------------ 2 files changed, 10 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c index 7dd54f573f35..0c0148c8c996 100644 --- a/drivers/gpu/drm/i915/display/intel_ddi.c +++ b/drivers/gpu/drm/i915/display/intel_ddi.c @@ -1704,20 +1704,20 @@ void intel_ddi_set_pipe_settings(const struct intel_crtc_state *crtc_state)

WARN_ON(transcoder_is_dsi(cpu_transcoder));

• temp = TRANS_MSA_SYNC_CLK;

• temp = DP_MSA_MISC_SYNC_CLOCK;

  switch (crtc_state->pipe_bpp) { case 18:

• temp |= TRANS_MSA_6_BPC;
  

• temp |= DP_MSA_MISC_6_BPC;
  

  break; case 24:

• temp |= TRANS_MSA_8_BPC;
  

• temp |= DP_MSA_MISC_8_BPC;
  

  break; case 30:

• temp |= TRANS_MSA_10_BPC;
  

• temp |= DP_MSA_MISC_10_BPC;
  

  break; case 36:

• temp |= TRANS_MSA_12_BPC;
  

• temp |= DP_MSA_MISC_12_BPC;
  

  break; default: MISSING_CASE(crtc_state->pipe_bpp);

@@ -1729,7 +1729,7 @@ void intel_ddi_set_pipe_settings(const struct intel_crtc_state *crtc_state) crtc_state->output_format != INTEL_OUTPUT_FORMAT_RGB);

if (crtc_state->limited_color_range)

• temp |= TRANS_MSA_CEA_RANGE;
  

• temp |= DP_MSA_MISC_COLOR_CEA_RGB;
  
  

  /*

  • As per DP 1.2 spec section 2.3.4.3 while sending

@@ -1737,8 +1737,7 @@ void intel_ddi_set_pipe_settings(const struct intel_crtc_state *crtc_state) * colorspace information. */ if (crtc_state->output_format == INTEL_OUTPUT_FORMAT_YCBCR444)

• temp |= TRANS_MSA_SAMPLING_444 | TRANS_MSA_CLRSP_YCBCR
  

|

• 	TRANS_MSA_YCBCR_BT709;
  

• temp |= DP_MSA_MISC_COLOR_YCBCR_444_BT709;
  
  

  /*

  • As per DP 1.4a spec section 2.2.4.3 [MSA Field for

Indication @@ -1747,7 +1746,8 @@ void intel_ddi_set_pipe_settings(const struct intel_crtc_state *crtc_state) * indicate VSC SDP for the Pixel Encoding/Colorimetry Format. */ if (crtc_state->output_format == INTEL_OUTPUT_FORMAT_YCBCR420)

• temp |= TRANS_MSA_USE_VSC_SDP;
  

• temp |= DP_MSA_MISC_COLOR_VSC_SDP;
  

• I915_WRITE(TRANS_MSA_MISC(cpu_transcoder), temp);

}

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 35133b2ef6c9..91bf714897e5 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -9602,18 +9602,7 @@ enum skl_power_gate { #define _TRANSC_MSA_MISC 0x62410 #define _TRANS_EDP_MSA_MISC 0x6f410 #define TRANS_MSA_MISC(tran) _MMIO_TRANS2(tran, _TRANSA_MSA_MISC)

-#define TRANS_MSA_SYNC_CLK (1 << 0) -#define TRANS_MSA_SAMPLING_444 (2 << 1) -#define TRANS_MSA_CLRSP_YCBCR (1 << 3) -#define TRANS_MSA_YCBCR_BT709 (1 << 4) -#define TRANS_MSA_6_BPC (0 << 5) -#define TRANS_MSA_8_BPC (1 << 5) -#define TRANS_MSA_10_BPC (2 << 5) -#define TRANS_MSA_12_BPC (3 << 5) -#define TRANS_MSA_16_BPC (4 << 5) -#define TRANS_MSA_CEA_RANGE (1 << 3) -#define TRANS_MSA_USE_VSC_SDP (1 << 14) +/* See DP_MSA_MISC_* for the bit definitions */

/* LCPLL Control */ #define LCPLL_CTL _MMIO(0x130040)

The changes look good to me. Reviewed-by: Gwan-gyeong Mun gwan-gyeong.mun@intel.com

On Thu, 2019-07-18 at 17:50 +0300, Ville Syrjala wrote:

From: Ville Syrjälä ville.syrjala@linux.intel.com

Since HSW the PIPECONF progressive vs. interlaced selection is done with just two bits instead of the earlier three. Let's not look at the extra bit on HSW+. Also gen2 doesn't support interlaced displays at all.

This is actually fine as is currently because the extra bit is mbz (as are all three bits on gen2). But just to avoid mishaps in the future if the bits get reused let's only look at what's properly defined.

Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com

drivers/gpu/drm/i915/display/intel_display.c | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c index e25b82d07d4f..ffdc350dc24a 100644 --- a/drivers/gpu/drm/i915/display/intel_display.c +++ b/drivers/gpu/drm/i915/display/intel_display.c @@ -8189,6 +8189,21 @@ static void intel_set_pipe_src_size(const struct intel_crtc_state *crtc_state) (crtc_state->pipe_src_h - 1)); }

+static bool intel_pipe_is_interlaced(struct intel_crtc_state *crtc_state) +{

• struct drm_i915_private *dev_priv = to_i915(crtc_state-

...

base.crtc->dev);

• enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
• if (IS_GEN(dev_priv, 2))

• return false;
  

• if (INTEL_GEN(dev_priv) >= 9 ||

•    IS_BROADWELL(dev_priv) || IS_HASWELL(dev_priv))
  

• return I915_READ(PIPECONF(cpu_transcoder)) &
  

PIPECONF_INTERLACE_MASK_HSW;

• else

• return I915_READ(PIPECONF(cpu_transcoder)) &
  

PIPECONF_INTERLACE_MASK; +}

static void intel_get_pipe_timings(struct intel_crtc *crtc, struct intel_crtc_state *pipe_config) { @@ -8227,7 +8242,7 @@ static void intel_get_pipe_timings(struct intel_crtc *crtc, pipe_config->base.adjusted_mode.crtc_vsync_start = (tmp & 0xffff) + 1; pipe_config->base.adjusted_mode.crtc_vsync_end = ((tmp >> 16) & 0xffff) + 1;

• if (I915_READ(PIPECONF(cpu_transcoder)) &

PIPECONF_INTERLACE_MASK) {

• if (intel_pipe_is_interlaced(pipe_config)) { pipe_config->base.adjusted_mode.flags |=

DRM_MODE_FLAG_INTERLACE; pipe_config->base.adjusted_mode.crtc_vtotal += 1; pipe_config->base.adjusted_mode.crtc_vblank_end += 1;

The changes look good to me. Reviewed-by: Gwan-gyeong Mun gwan-gyeong.mun@intel.com

On Thu, 2019-07-18 at 17:50 +0300, Ville Syrjala wrote:

From: Ville Syrjälä ville.syrjala@linux.intel.com

Make intel_get_crtc_ycbcr_config() simpler and rename it to bdw_get_pipemisc_output_format() to better reflect what it does.

Also toss in some comments to document that the 4:2:0 PIPECONF bits are glk+ only. They are mbz on earlier platforms so reading them unconditionally is safe however.

Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com

drivers/gpu/drm/i915/display/intel_display.c | 71 +++++++++---------

drivers/gpu/drm/i915/i915_reg.h | 4 +- 2 files changed, 34 insertions(+), 41 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c index ffdc350dc24a..1dd1aa29a649 100644 --- a/drivers/gpu/drm/i915/display/intel_display.c +++ b/drivers/gpu/drm/i915/display/intel_display.c @@ -8713,47 +8713,24 @@ static void chv_crtc_clock_get(struct intel_crtc *crtc, pipe_config->port_clock = chv_calc_dpll_params(refclk, &clock); }

-static void intel_get_crtc_ycbcr_config(struct intel_crtc *crtc,

• 			struct intel_crtc_state
  

*pipe_config) +static enum intel_output_format +bdw_get_pipemisc_output_format(struct intel_crtc *crtc) { struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);

• enum intel_output_format output = INTEL_OUTPUT_FORMAT_RGB;
• pipe_config->lspcon_downsampling = false;
• if (IS_BROADWELL(dev_priv) || INTEL_GEN(dev_priv) >= 9) {

• u32 tmp = I915_READ(PIPEMISC(crtc->pipe));
  

• if (tmp & PIPEMISC_OUTPUT_COLORSPACE_YUV) {
  

• 	bool ycbcr420_enabled = tmp &
  

PIPEMISC_YUV420_ENABLE;

• 	bool blend = tmp &
  

PIPEMISC_YUV420_MODE_FULL_BLEND;

• 	if (ycbcr420_enabled) {
  

• 		/* We support 4:2:0 in full blend mode
  

only */

• 		if (!blend)
  

• 			output =
  

INTEL_OUTPUT_FORMAT_INVALID;

• 		else if (!(IS_GEMINILAKE(dev_priv) ||
  

• 			   INTEL_GEN(dev_priv) >= 10))
  

• 			output =
  

INTEL_OUTPUT_FORMAT_INVALID;

• 		else
  

• 			output =
  

INTEL_OUTPUT_FORMAT_YCBCR420;

• 	} else {
  

• 		/*
  

• 		 * Currently there is no interface
  

defined to

• 		 * check user preference between
  

RGB/YCBCR444

• 		 * or YCBCR420. So the only possible
  

case for

• 		 * YCBCR444 usage is driving YCBCR420
  

output

• 		 * with LSPCON, when pipe is configured
  

for

• 		 * YCBCR444 output and LSPCON takes
  

care of

• 		 * downsampling it.
  

• 		 */
  

• 		pipe_config->lspcon_downsampling =
  

true;

• 		output = INTEL_OUTPUT_FORMAT_YCBCR444;
  

• 	}
  

• }
  

• }

• u32 tmp;
• tmp = I915_READ(PIPEMISC(crtc->pipe));

• pipe_config->output_format = output;

• if (tmp & PIPEMISC_YUV420_ENABLE) {

• /* We support 4:2:0 in full blend mode only */
  

• WARN_ON((tmp & PIPEMISC_YUV420_MODE_FULL_BLEND) == 0);
  

• return INTEL_OUTPUT_FORMAT_YCBCR420;
  

• } else if (tmp & PIPEMISC_OUTPUT_COLORSPACE_YUV) {

• return INTEL_OUTPUT_FORMAT_YCBCR444;
  

• } else {

• return INTEL_OUTPUT_FORMAT_RGB;
  

• }

}

static void i9xx_get_pipe_color_config(struct intel_crtc_state *crtc_state) @@ -10445,7 +10422,23 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc, }

intel_get_pipe_src_size(crtc, pipe_config);

• intel_get_crtc_ycbcr_config(crtc, pipe_config);

• if (INTEL_GEN(dev_priv) >= 9 || IS_BROADWELL(dev_priv)) {

• pipe_config->output_format =
  

• 	bdw_get_pipemisc_output_format(crtc);
  

• /*
  

•  * Currently there is no interface defined to
  

•  * check user preference between RGB/YCBCR444
  

•  * or YCBCR420. So the only possible case for
  

•  * YCBCR444 usage is driving YCBCR420 output
  

•  * with LSPCON, when pipe is configured for
  

•  * YCBCR444 output and LSPCON takes care of
  

•  * downsampling it.
  

•  */
  

• pipe_config->lspcon_downsampling =
  

• 	pipe_config->output_format ==
  

INTEL_OUTPUT_FORMAT_YCBCR444;

• }

  pipe_config->gamma_mode = I915_READ(GAMMA_MODE(crtc->pipe));

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 91bf714897e5..66f7f417231f 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -5803,8 +5803,8 @@ enum {

#define _PIPE_MISC_A 0x70030 #define _PIPE_MISC_B 0x71030 -#define PIPEMISC_YUV420_ENABLE (1 << 27) -#define PIPEMISC_YUV420_MODE_FULL_BLEND (1 << 26) +#define PIPEMISC_YUV420_ENABLE (1 << 27) /* glk+ */ +#define PIPEMISC_YUV420_MODE_FULL_BLEND (1 << 26) /* glk+ */ #define PIPEMISC_HDR_MODE_PRECISION (1 << 23) /* icl+ */ #define PIPEMISC_OUTPUT_COLORSPACE_YUV (1 << 11) #define PIPEMISC_DITHER_BPC_MASK (7 << 5)

The changes look good to me. Reviewed-by: Gwan-gyeong Mun gwan-gyeong.mun@intel.com

On Thu, 2019-07-18 at 17:50 +0300, Ville Syrjala wrote:

From: Ville Syrjälä ville.syrjala@linux.intel.com

On HSW the pipe colorspace is configured via PIPECONF (as opposed to PIPEMISC in BDW+). Let's configure+readout that stuff correctly.

Enablling YCbCr 4:4:4 output will now be a simple matter of

Typo: Enablling -> Enabling

setting crtc_state->output_format appropriately in the encoder .compute_config().

Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com

drivers/gpu/drm/i915/display/intel_display.c | 13 ++++++++++++- drivers/gpu/drm/i915/i915_reg.h | 1 + 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c index 1dd1aa29a649..bd3ff96c1618 100644 --- a/drivers/gpu/drm/i915/display/intel_display.c +++ b/drivers/gpu/drm/i915/display/intel_display.c @@ -9430,6 +9430,10 @@ static void haswell_set_pipeconf(const struct intel_crtc_state *crtc_state) else val |= PIPECONF_PROGRESSIVE;

• if (IS_HASWELL(dev_priv) &&

•    crtc_state->output_format != INTEL_OUTPUT_FORMAT_RGB)
  

• val |= PIPECONF_OUTPUT_COLORSPACE_YUV_HSW;
  

• I915_WRITE(PIPECONF(cpu_transcoder), val); POSTING_READ(PIPECONF(cpu_transcoder));

} @@ -10423,7 +10427,14 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,

intel_get_pipe_src_size(crtc, pipe_config);

• if (INTEL_GEN(dev_priv) >= 9 || IS_BROADWELL(dev_priv)) {

• if (IS_HASWELL(dev_priv)) {

• u32 tmp = I915_READ(PIPECONF(pipe_config-
  

...

cpu_transcoder));

• if (tmp & PIPECONF_OUTPUT_COLORSPACE_YUV_HSW)
  

• 	pipe_config->output_format =
  

INTEL_OUTPUT_FORMAT_YCBCR444;

• else
  

• 	pipe_config->output_format =
  

INTEL_OUTPUT_FORMAT_RGB;

• } else { pipe_config->output_format = bdw_get_pipemisc_output_format(crtc);

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 66f7f417231f..58471312b8b2 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -5712,6 +5712,7 @@ enum { #define PIPECONF_CXSR_DOWNCLOCK (1 << 16) #define PIPECONF_EDP_RR_MODE_SWITCH_VLV (1 << 14) #define PIPECONF_COLOR_RANGE_SELECT (1 << 13) +#define PIPECONF_OUTPUT_COLORSPACE_YUV_HSW (1 << 11) /* hsw only */ #define PIPECONF_BPC_MASK (0x7 << 5) #define PIPECONF_8BPC (0 << 5) #define PIPECONF_10BPC (1 << 5)

On Fri, Sep 20, 2019 at 02:24:32PM +0000, Mun, Gwan-gyeong wrote:

On Thu, 2019-07-18 at 17:50 +0300, Ville Syrjala wrote:

...

From: Ville Syrjälä ville.syrjala@linux.intel.com

Add comments to explain the ilk pipe csc operation a bit better.

Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com

drivers/gpu/drm/i915/display/intel_color.c | 26 +++++++++++++++++---

1 file changed, 21 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c index 23a84dd7989f..736c42720daf 100644 --- a/drivers/gpu/drm/i915/display/intel_color.c +++ b/drivers/gpu/drm/i915/display/intel_color.c @@ -42,6 +42,21 @@

#define LEGACY_LUT_LENGTH 256

+/*

• • ILK+ csc matrix:
• • |R/Cr| | c0 c1 c2 | ( |R/Cr| |preoff0| ) |postoff0|
• • |G/Y | = | c3 c4 c5 | x ( |G/Y | + |preoff1| ) + |postoff1|
• • |B/Cb| | c6 c7 c8 | ( |B/Cb| |preoff2| ) |postoff2|
• • ILK/SNB don't have explicit post offsets, and instead
• • CSC_MODE_YUV_TO_RGB and CSC_BLACK_SCREEN_OFFSET are used:
• • CSC_MODE_YUV_TO_RGB=0 + CSC_BLACK_SCREEN_OFFSET=0 -> 1/2, 0, 1/2
• • CSC_MODE_YUV_TO_RGB=0 + CSC_BLACK_SCREEN_OFFSET=1 -> 1/2, 1/16,

1/2

It seems that the calculated values are assumes ITU-R BT.709 spec, if you don't mind, can we add some comments which is based on ITU-R BT.709?

...

• • CSC_MODE_YUV_TO_RGB=1 + CSC_BLACK_SCREEN_OFFSET=0 -> 0, 0, 0
• • CSC_MODE_YUV_TO_RGB=1 + CSC_BLACK_SCREEN_OFFSET=1 -> 1/16, 1/16,

1/16

• */

/*

• Extract the CSC coefficient from a CTM coefficient (in U32.32

fixed point

• format). This macro takes the coefficient we want transformed and

the @@ -59,37 +74,38 @@

#define ILK_CSC_POSTOFF_LIMITED_RANGE (16 * (1 << 12) / 255)

+/* Nop pre/post offsets */ static const u16 ilk_csc_off_zero[3] = {};

+/* Identity matrix */ static const u16 ilk_csc_coeff_identity[9] = { ILK_CSC_COEFF_1_0, 0, 0, 0, ILK_CSC_COEFF_1_0, 0, 0, 0, ILK_CSC_COEFF_1_0, };

+/* Limited range RGB post offsets */ static const u16 ilk_csc_postoff_limited_range[3] = { ILK_CSC_POSTOFF_LIMITED_RANGE, ILK_CSC_POSTOFF_LIMITED_RANGE, ILK_CSC_POSTOFF_LIMITED_RANGE, };

+/* Full range RGB -> limited range RGB matrix */ static const u16 ilk_csc_coeff_limited_range[9] = { ILK_CSC_COEFF_LIMITED_RANGE, 0, 0, 0, ILK_CSC_COEFF_LIMITED_RANGE, 0, 0, 0, ILK_CSC_COEFF_LIMITED_RANGE, };

-/*

• • These values are direct register values specified in the Bspec,
• • for RGB->YUV conversion matrix (colorspace BT709)
• */

+/* BT.709 full range RGB -> limited range YCbCr matrix */

The comment is here ^

...

static const u16 ilk_csc_coeff_rgb_to_ycbcr[9] = { 0x1e08, 0x9cc0, 0xb528, 0x2ba8, 0x09d8, 0x37e8, 0xbce8, 0x9ad8, 0x1e08, };

-/* Post offset values for RGB->YCBCR conversion */ +/* Limited range YCbCr post offsets */ static const u16 ilk_csc_postoff_rgb_to_ycbcr[3] = { 0x0800, 0x0100, 0x0800, };

The changes look good to me. Reviewed-by: Gwan-gyeong Mun gwan-gyeong.mun@intel.com

On Thu, 2019-07-18 at 17:50 +0300, Ville Syrjala wrote:

From: Ville Syrjälä ville.syrjala@linux.intel.com

Prepare the pipe csc for YCbCr output on ilk/snb. The main difference to IVB+ is the lack of explicit post offsets, and instead we must configure the CSC info RGB->YUV mode (which takes care of offsetting Cb/Cr properly) and enable the "black screen offset" bit to add the required offset to Y.

And while at it throw some comments around the bit defines to document which platforms have which bits.

Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com

drivers/gpu/drm/i915/display/intel_color.c | 25 +++++++++++++++++---

drivers/gpu/drm/i915/i915_reg.h | 10 ++++----- 2 files changed, 25 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c index 736c42720daf..a902f7809840 100644 --- a/drivers/gpu/drm/i915/display/intel_color.c +++ b/drivers/gpu/drm/i915/display/intel_color.c @@ -1213,6 +1213,21 @@ static u32 ilk_gamma_mode(const struct intel_crtc_state *crtc_state) return GAMMA_MODE_MODE_10BIT; }

+static u32 ilk_csc_mode(const struct intel_crtc_state *crtc_state) +{

• /*

• * CSC comes after the LUT in RGB->YCbCr mode.
  

• * RGB->YCbCr needs the limited range offsets added to
  

• * the output. RGB limited range output is handled by
  

• * the hw automagically elsewhere.
  

• */
  

• if (crtc_state->output_format != INTEL_OUTPUT_FORMAT_RGB)

• return CSC_BLACK_SCREEN_OFFSET;
  

• return CSC_MODE_YUV_TO_RGB |

• CSC_POSITION_BEFORE_GAMMA;
  

+}

static int ilk_color_check(struct intel_crtc_state *crtc_state) { int ret; @@ -1226,15 +1241,15 @@ static int ilk_color_check(struct intel_crtc_state *crtc_state) !crtc_state->c8_planes;

/*

• * We don't expose the ctm on ilk/snb currently,
  

• * nor do we enable YCbCr output. Also RGB limited
  

• * range output is handled by the hw automagically.
  

• * We don't expose the ctm on ilk/snb currently, also RGB
  

• * limited range output is handled by the hw automagically.
  

  */

• crtc_state->csc_enable = false;

• crtc_state->csc_enable =

• crtc_state->output_format != INTEL_OUTPUT_FORMAT_RGB;
  
  

  crtc_state->gamma_mode = ilk_gamma_mode(crtc_state);

• crtc_state->csc_mode = 0;

• crtc_state->csc_mode = ilk_csc_mode(crtc_state);

  ret = intel_color_add_affected_planes(crtc_state); if (ret)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 58471312b8b2..33d535ae0944 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -10106,11 +10106,11 @@ enum skl_power_gate { #define _PIPE_A_CSC_COEFF_BV 0x49024

#define _PIPE_A_CSC_MODE 0x49028 -#define ICL_CSC_ENABLE (1 << 31) -#define ICL_OUTPUT_CSC_ENABLE (1 << 30) -#define CSC_BLACK_SCREEN_OFFSET (1 << 2) -#define CSC_POSITION_BEFORE_GAMMA (1 << 1) -#define CSC_MODE_YUV_TO_RGB (1 << 0) +#define ICL_CSC_ENABLE (1 << 31) /* icl+ */ +#define ICL_OUTPUT_CSC_ENABLE (1 << 30) /* icl+ */ +#define CSC_BLACK_SCREEN_OFFSET (1 << 2) /* ilk/snb */ +#define CSC_POSITION_BEFORE_GAMMA (1 << 1) /* pre-glk */ +#define CSC_MODE_YUV_TO_RGB (1 << 0) /* ilk/snb */

#define _PIPE_A_CSC_PREOFF_HI 0x49030 #define _PIPE_A_CSC_PREOFF_ME 0x49034

The changes look good to me. Reviewed-by: Gwan-gyeong Mun gwan-gyeong.mun@intel.com

On Thu, 2019-07-18 at 17:50 +0300, Ville Syrjala wrote:

From: Ville Syrjälä ville.syrjala@linux.intel.com

On ILK-IVB the pipe colorspace is configured via PIPECONF (as opposed to PIPEMISC in BDW+). Let's configure+readout that stuff correctly.

Enablling YCbCr 4:4:4 output will now be a simple matter of

Typo: Enablling -> Enabling

setting crtc_state->output_format appropriately in the encoder .compute_config(). However, when we do that we must be aware of the fact that YCbCr DP output doesn't seem to work on ILK (resulting image is totally garbled), but on SNB+ it works fine. However HDMI YCbCr output does work correctly even on ILK.

Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com

drivers/gpu/drm/i915/display/intel_display.c | 21 +++++++++++++++++++- drivers/gpu/drm/i915/i915_reg.h | 4 ++++ 2 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c index bd3ff96c1618..8e98715cd63b 100644 --- a/drivers/gpu/drm/i915/display/intel_display.c +++ b/drivers/gpu/drm/i915/display/intel_display.c @@ -9406,9 +9406,19 @@ static void ironlake_set_pipeconf(const struct intel_crtc_state *crtc_state) else val |= PIPECONF_PROGRESSIVE;

• /*

• * This would end up with an odd purple hue over
  

• * the entire display. Make sure we don't do it.
  

• */
  

• WARN_ON(crtc_state->limited_color_range &&

• crtc_state->output_format != INTEL_OUTPUT_FORMAT_RGB);
  

• if (crtc_state->limited_color_range) val |= PIPECONF_COLOR_RANGE_SELECT;

• if (crtc_state->output_format != INTEL_OUTPUT_FORMAT_RGB)

• val |= PIPECONF_OUTPUT_COLORSPACE_YUV709;
  

• val |= PIPECONF_GAMMA_MODE(crtc_state->gamma_mode);

  I915_WRITE(PIPECONF(pipe), val);

@@ -9945,7 +9955,6 @@ static bool ironlake_get_pipe_config(struct intel_crtc *crtc, if (!wakeref) return false;

• pipe_config->output_format = INTEL_OUTPUT_FORMAT_RGB; pipe_config->cpu_transcoder = (enum transcoder) crtc->pipe; pipe_config->shared_dpll = NULL;

@@ -9974,6 +9983,16 @@ static bool ironlake_get_pipe_config(struct intel_crtc *crtc, if (tmp & PIPECONF_COLOR_RANGE_SELECT) pipe_config->limited_color_range = true;

• switch (tmp & PIPECONF_OUTPUT_COLORSPACE_MASK) {
• case PIPECONF_OUTPUT_COLORSPACE_YUV601:
• case PIPECONF_OUTPUT_COLORSPACE_YUV709:

• pipe_config->output_format =
  

INTEL_OUTPUT_FORMAT_YCBCR444;

• break;
  

• default:

• pipe_config->output_format = INTEL_OUTPUT_FORMAT_RGB;
  

• break;
  

• }
• pipe_config->gamma_mode = (tmp & PIPECONF_GAMMA_MODE_MASK_ILK)

...

...

PIPECONF_GAMMA_MODE_SHIFT;

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 33d535ae0944..3d33a1e03a45 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -5712,6 +5712,10 @@ enum { #define PIPECONF_CXSR_DOWNCLOCK (1 << 16) #define PIPECONF_EDP_RR_MODE_SWITCH_VLV (1 << 14) #define PIPECONF_COLOR_RANGE_SELECT (1 << 13) +#define PIPECONF_OUTPUT_COLORSPACE_MASK (3 << 11) /* ilk-ivb */ +#define PIPECONF_OUTPUT_COLORSPACE_RGB (0 << 11) /* ilk-ivb */ +#define PIPECONF_OUTPUT_COLORSPACE_YUV601 (1 << 11) /* ilk-ivb */ +#define PIPECONF_OUTPUT_COLORSPACE_YUV709 (2 << 11) /* ilk-ivb */ #define PIPECONF_OUTPUT_COLORSPACE_YUV_HSW (1 << 11) /* hsw only */ #define PIPECONF_BPC_MASK (0x7 << 5) #define PIPECONF_8BPC (0 << 5)