[PATCH v3 0/7] YCBCR 4:2:0/4:4:4 output support for LSPCON

[PATCH v3 0/7] YCBCR 4:2:0/4:4:4 output support for LSPCON

[Shashank Sharma]

This patch series adds YCBCR 4:2:0 output support for LSPCON displays.
In order to indicate the color format of output, to the LSPCON device,
a source has to set and send proper AVI infoframes to LSPCON. So this
patch series:
- first adds AVI infoframes support for LSPCON
- then adds YCBCR 4:2:0 output support for LSPCON

Previous versions of this series and its review can be found here:
https://patchwork.freedesktop.org/series/28536/
https://patchwork.freedesktop.org/series/33794/

In order to address review comment from V2, I have added 2 new patches
in this series, hence sending V3.

Shashank Sharma (7):
  drm/i915: Add CRTC output format YCBCR 4:2:0
  drm/i915: Add CRTC output format YCBCR 4:4:4
  drm/i915: Check LSPCON vendor OUI
  drm/i915: Add AVI infoframe support for LSPCON
  drm/i915: Write AVI infoframes for MCA LSPCON
  drm/i915: Write AVI infoframes for Parade LSPCON
  drm/i915: Add YCBCR 4:2:0/4:4:4 support for LSPCON

 drivers/gpu/drm/i915/i915_reg.h      |   2 +
 drivers/gpu/drm/i915/intel_color.c   |   3 +-
 drivers/gpu/drm/i915/intel_ddi.c     |  28 ++-
 drivers/gpu/drm/i915/intel_display.c |  73 +++++---
 drivers/gpu/drm/i915/intel_dp.c      |  10 +
 drivers/gpu/drm/i915/intel_drv.h     |  37 +++-
 drivers/gpu/drm/i915/intel_hdmi.c    |  23 ++-
 drivers/gpu/drm/i915/intel_lspcon.c  | 343 +++++++++++++++++++++++++++++++++--
 drivers/gpu/drm/i915/intel_panel.c   |   2 +-
 9 files changed, 469 insertions(+), 52 deletions(-)

-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v3 1/7] drm/i915: Add CRTC output format YCBCR 4:2:0

[Shashank Sharma]

Currently, we are using a bool in CRTC state (state->ycbcr420),
to indicate modeset, that the output format is YCBCR 4:2:0. Now in
order to support other YCBCR formats, we will need more such flags.

The idea behind this patch is to replace this bool with an enum,
and plug this in in the existing YCBCR 4:2:0 framework in such a
way that this can be re-used for YCBCR 4:4:4 and other output formats too.

This patch adds a new enum for CRTC output formats, and then plugs it
in the existing modeset framework.

V3: Added this patch in the series, to address review comments from
    second patchset.

Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
---
 drivers/gpu/drm/i915/intel_color.c   |  2 +-
 drivers/gpu/drm/i915/intel_ddi.c     |  2 +-
 drivers/gpu/drm/i915/intel_display.c | 66 ++++++++++++++++++++++++------------
 drivers/gpu/drm/i915/intel_drv.h     | 10 ++++--
 drivers/gpu/drm/i915/intel_hdmi.c    |  6 ++--
 drivers/gpu/drm/i915/intel_panel.c   |  2 +-
 6 files changed, 59 insertions(+), 29 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
index aa66e95..99e32cb 100644
--- a/drivers/gpu/drm/i915/intel_color.c
+++ b/drivers/gpu/drm/i915/intel_color.c
@@ -141,7 +141,7 @@ static void i9xx_load_csc_matrix(struct drm_crtc_state *crtc_state)
 	uint16_t coeffs[9] = { 0, };
 	struct intel_crtc_state *intel_crtc_state = to_intel_crtc_state(crtc_state);
 
-	if (intel_crtc_state->ycbcr420) {
+	if (intel_crtc_state->output_format == CRTC_OUTPUT_YCBCR420) {
 		i9xx_load_ycbcr_conversion_matrix(intel_crtc);
 		return;
 	} else if (crtc_state->ctm) {
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index f51645a..84327e7 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -1264,7 +1264,7 @@ static void ddi_dotclock_get(struct intel_crtc_state *pipe_config)
 	else
 		dotclock = pipe_config->port_clock;
 
-	if (pipe_config->ycbcr420)
+	if (pipe_config->output_format == CRTC_OUTPUT_YCBCR420)
 		dotclock *= 2;
 
 	if (pipe_config->pixel_multiplier)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 0cd3559..69b0aa3 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4644,7 +4644,8 @@ skl_update_scaler(struct intel_crtc_state *crtc_state, bool force_detach,
 	 */
 	need_scaling = src_w != dst_w || src_h != dst_h;
 
-	if (crtc_state->ycbcr420 && scaler_user == SKL_CRTC_INDEX)
+	if (crtc_state->output_format == CRTC_OUTPUT_YCBCR420 &&
+	    scaler_user == SKL_CRTC_INDEX)
 		need_scaling = true;
 
 	/*
@@ -6356,7 +6357,8 @@ static int intel_crtc_compute_config(struct intel_crtc *crtc,
 		return -EINVAL;
 	}
 
-	if (pipe_config->ycbcr420 && pipe_config->base.ctm) {
+	if (pipe_config->output_format == CRTC_OUTPUT_YCBCR420 &&
+	    pipe_config->base.ctm) {
 		/*
 		 * There is only one pipe CSC unit per pipe, and we need that
 		 * for output conversion from RGB->YCBCR. So if CTM is already
@@ -8177,10 +8179,10 @@ static void haswell_set_pipemisc(struct drm_crtc *crtc)
 		if (intel_crtc->config->dither)
 			val |= PIPEMISC_DITHER_ENABLE | PIPEMISC_DITHER_TYPE_SP;
 
-		if (config->ycbcr420) {
-			val |= PIPEMISC_OUTPUT_COLORSPACE_YUV |
-				PIPEMISC_YUV420_ENABLE |
-				PIPEMISC_YUV420_MODE_FULL_BLEND;
+		if (config->output_format == CRTC_OUTPUT_YCBCR420) {
+			val |= PIPEMISC_OUTPUT_COLORSPACE_YUV;
+			val |= PIPEMISC_YUV420_ENABLE |
+			       PIPEMISC_YUV420_MODE_FULL_BLEND;
 		}
 
 		I915_WRITE(PIPEMISC(intel_crtc->pipe), val);
@@ -9156,6 +9158,19 @@ static void haswell_get_ddi_port_state(struct intel_crtc *crtc,
 	}
 }
 
+static const char * const output_format_str[] = {
+	"Invalid",
+	"RGB",
+	"YCBCR4:2:0",
+};
+
+static const char *output_formats(enum crtc_output_format format)
+{
+	if (format < CRTC_OUTPUT_INVALID || format > CRTC_OUTPUT_YCBCR420)
+		format = CRTC_OUTPUT_INVALID;
+	return output_format_str[format + 1];
+}
+
 static bool haswell_get_pipe_config(struct intel_crtc *crtc,
 				    struct intel_crtc_state *pipe_config)
 {
@@ -9196,19 +9211,28 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
 
 	if (IS_BROADWELL(dev_priv) || INTEL_GEN(dev_priv) >= 9) {
 		u32 tmp = I915_READ(PIPEMISC(crtc->pipe));
-		bool clrspace_yuv = tmp & PIPEMISC_OUTPUT_COLORSPACE_YUV;
-
-		if (IS_GEMINILAKE(dev_priv) || INTEL_GEN(dev_priv) >= 10) {
-			bool blend_mode_420 = tmp &
-					      PIPEMISC_YUV420_MODE_FULL_BLEND;
+		enum crtc_output_format output_format = CRTC_OUTPUT_RGB;
+
+		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_format = CRTC_OUTPUT_INVALID;
+				else if (!(IS_GEMINILAKE(dev_priv) ||
+					   INTEL_GEN(dev_priv) >= 10))
+					output_format = CRTC_OUTPUT_INVALID;
+				else
+					output_format = CRTC_OUTPUT_YCBCR420;
+			}
 
-			pipe_config->ycbcr420 = tmp & PIPEMISC_YUV420_ENABLE;
-			if (pipe_config->ycbcr420 != clrspace_yuv ||
-			    pipe_config->ycbcr420 != blend_mode_420)
-				DRM_DEBUG_KMS("Bad 4:2:0 mode (%08x)\n", tmp);
-		} else if (clrspace_yuv) {
-			DRM_DEBUG_KMS("YCbCr 4:2:0 Unsupported\n");
 		}
+
+		pipe_config->output_format = output_format;
+		DRM_DEBUG_KMS("Output format %s\n",
+				output_formats(output_format));
 	}
 
 	power_domain = POWER_DOMAIN_PIPE_PANEL_FITTER(crtc->pipe);
@@ -10558,6 +10582,9 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc,
 	DRM_DEBUG_KMS("output_types: %s (0x%x)\n",
 		      buf, pipe_config->output_types);
 
+	DRM_DEBUG_KMS("output format: %s\n",
+		output_formats(pipe_config->output_format));
+
 	DRM_DEBUG_KMS("cpu_transcoder: %s, pipe bpp: %i, dithering: %i\n",
 		      transcoder_name(pipe_config->cpu_transcoder),
 		      pipe_config->pipe_bpp, pipe_config->dither);
@@ -10567,9 +10594,6 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc,
 				      pipe_config->fdi_lanes,
 				      &pipe_config->fdi_m_n);
 
-	if (pipe_config->ycbcr420)
-		DRM_DEBUG_KMS("YCbCr 4:2:0 output enabled\n");
-
 	if (intel_crtc_has_dp_encoder(pipe_config)) {
 		intel_dump_m_n_config(pipe_config, "dp m_n",
 				pipe_config->lane_count, &pipe_config->dp_m_n);
@@ -11143,6 +11167,7 @@ intel_pipe_config_compare(struct drm_i915_private *dev_priv,
 	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_vsync_end);
 
 	PIPE_CONF_CHECK_I(pixel_multiplier);
+	PIPE_CONF_CHECK_I(output_format);
 	PIPE_CONF_CHECK_BOOL(has_hdmi_sink);
 	if ((INTEL_GEN(dev_priv) < 8 && !IS_HASWELL(dev_priv)) ||
 	    IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
@@ -11151,7 +11176,6 @@ intel_pipe_config_compare(struct drm_i915_private *dev_priv,
 	PIPE_CONF_CHECK_BOOL(hdmi_scrambling);
 	PIPE_CONF_CHECK_BOOL(hdmi_high_tmds_clock_ratio);
 	PIPE_CONF_CHECK_BOOL_INCOMPLETE(has_infoframe);
-	PIPE_CONF_CHECK_BOOL(ycbcr420);
 
 	PIPE_CONF_CHECK_BOOL_INCOMPLETE(has_audio);
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 30f791f..79662650 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -609,6 +609,12 @@ struct intel_crtc_wm_state {
 	bool need_postvbl_update;
 };
 
+enum crtc_output_format {
+	CRTC_OUTPUT_INVALID = -1,
+	CRTC_OUTPUT_RGB,
+	CRTC_OUTPUT_YCBCR420,
+};
+
 struct intel_crtc_state {
 	struct drm_crtc_state base;
 
@@ -795,8 +801,8 @@ struct intel_crtc_state {
 	/* HDMI High TMDS char rate ratio */
 	bool hdmi_high_tmds_clock_ratio;
 
-	/* output format is YCBCR 4:2:0 */
-	bool ycbcr420;
+	/* Output format RGB/YCBCR etc */
+	enum crtc_output_format output_format;
 };
 
 struct intel_crtc {
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index bced7b9..b266a7f 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -478,7 +478,7 @@ static void intel_hdmi_set_avi_infoframe(struct drm_encoder *encoder,
 		return;
 	}
 
-	if (crtc_state->ycbcr420)
+	if (crtc_state->output_format == CRTC_OUTPUT_YCBCR420)
 		frame.avi.colorspace = HDMI_COLORSPACE_YUV420;
 	else
 		frame.avi.colorspace = HDMI_COLORSPACE_RGB;
@@ -1372,7 +1372,7 @@ static bool hdmi_12bpc_possible(const struct intel_crtc_state *crtc_state)
 		if (connector_state->crtc != crtc_state->base.crtc)
 			continue;
 
-		if (crtc_state->ycbcr420) {
+		if (crtc_state->output_format == CRTC_OUTPUT_YCBCR420) {
 			const struct drm_hdmi_info *hdmi = &info->hdmi;
 
 			if (!(hdmi->y420_dc_modes & DRM_EDID_YCBCR420_DC_36))
@@ -1407,7 +1407,7 @@ intel_hdmi_ycbcr420_config(struct drm_connector *connector,
 	config->port_clock /= 2;
 	*clock_12bpc /= 2;
 	*clock_8bpc /= 2;
-	config->ycbcr420 = true;
+	config->output_format = CRTC_OUTPUT_YCBCR420;
 
 	/* YCBCR 420 output conversion needs a scaler */
 	if (skl_update_scaler_crtc(config)) {
diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
index fa6831f..c57819f 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -111,7 +111,7 @@ intel_pch_panel_fitting(struct intel_crtc *intel_crtc,
 	/* Native modes don't need fitting */
 	if (adjusted_mode->crtc_hdisplay == pipe_config->pipe_src_w &&
 	    adjusted_mode->crtc_vdisplay == pipe_config->pipe_src_h &&
-	    !pipe_config->ycbcr420)
+	    pipe_config->output_format != CRTC_OUTPUT_YCBCR420)
 		goto done;
 
 	switch (fitting_mode) {
-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v3 2/7] drm/i915: Add CRTC output format YCBCR 4:4:4

[Shashank Sharma]

This patch adds support for YCBCR 4:4:4 CRTC output format.
To do this, this patch extends the existing YCBCR 4:2:0
framework by:
- Adding new parameter in for YCBCR 4:4:4 enum crtc_iutput_format.
- Adding case for YCBCR 4:4:4 in while setting AVI infoframes.
- Adding necessary checks in modeset sequence.

V3: Added this patch in the series

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
---
 drivers/gpu/drm/i915/intel_color.c   |  3 ++-
 drivers/gpu/drm/i915/intel_display.c | 13 +++++++++----
 drivers/gpu/drm/i915/intel_drv.h     |  1 +
 drivers/gpu/drm/i915/intel_hdmi.c    |  2 ++
 4 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
index 99e32cb..5b76de6 100644
--- a/drivers/gpu/drm/i915/intel_color.c
+++ b/drivers/gpu/drm/i915/intel_color.c
@@ -141,7 +141,8 @@ static void i9xx_load_csc_matrix(struct drm_crtc_state *crtc_state)
 	uint16_t coeffs[9] = { 0, };
 	struct intel_crtc_state *intel_crtc_state = to_intel_crtc_state(crtc_state);
 
-	if (intel_crtc_state->output_format == CRTC_OUTPUT_YCBCR420) {
+	if (intel_crtc_state->output_format == CRTC_OUTPUT_YCBCR420 ||
+	    intel_crtc_state->output_format == CRTC_OUTPUT_YCBCR444) {
 		i9xx_load_ycbcr_conversion_matrix(intel_crtc);
 		return;
 	} else if (crtc_state->ctm) {
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 69b0aa3..6ac5ca6 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6357,8 +6357,9 @@ static int intel_crtc_compute_config(struct intel_crtc *crtc,
 		return -EINVAL;
 	}
 
-	if (pipe_config->output_format == CRTC_OUTPUT_YCBCR420 &&
-	    pipe_config->base.ctm) {
+	if ((pipe_config->output_format == CRTC_OUTPUT_YCBCR420 ||
+	     pipe_config->output_format == CRTC_OUTPUT_YCBCR444) &&
+	     pipe_config->base.ctm) {
 		/*
 		 * There is only one pipe CSC unit per pipe, and we need that
 		 * for output conversion from RGB->YCBCR. So if CTM is already
@@ -8179,11 +8180,12 @@ static void haswell_set_pipemisc(struct drm_crtc *crtc)
 		if (intel_crtc->config->dither)
 			val |= PIPEMISC_DITHER_ENABLE | PIPEMISC_DITHER_TYPE_SP;
 
-		if (config->output_format == CRTC_OUTPUT_YCBCR420) {
+		if (config->output_format > CRTC_OUTPUT_RGB)
 			val |= PIPEMISC_OUTPUT_COLORSPACE_YUV;
+
+		if (config->output_format == CRTC_OUTPUT_YCBCR420)
 			val |= PIPEMISC_YUV420_ENABLE |
 			       PIPEMISC_YUV420_MODE_FULL_BLEND;
-		}
 
 		I915_WRITE(PIPEMISC(intel_crtc->pipe), val);
 	}
@@ -9161,6 +9163,7 @@ static void haswell_get_ddi_port_state(struct intel_crtc *crtc,
 static const char * const output_format_str[] = {
 	"Invalid",
 	"RGB",
+	"YCBCR4:4:4",
 	"YCBCR4:2:0",
 };
 
@@ -9226,6 +9229,8 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
 					output_format = CRTC_OUTPUT_INVALID;
 				else
 					output_format = CRTC_OUTPUT_YCBCR420;
+			} else {
+				output_format = CRTC_OUTPUT_YCBCR444;
 			}
 
 		}
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 79662650..a393342 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -612,6 +612,7 @@ struct intel_crtc_wm_state {
 enum crtc_output_format {
 	CRTC_OUTPUT_INVALID = -1,
 	CRTC_OUTPUT_RGB,
+	CRTC_OUTPUT_YCBCR444,
 	CRTC_OUTPUT_YCBCR420,
 };
 
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index b266a7f..258bb51 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -480,6 +480,8 @@ static void intel_hdmi_set_avi_infoframe(struct drm_encoder *encoder,
 
 	if (crtc_state->output_format == CRTC_OUTPUT_YCBCR420)
 		frame.avi.colorspace = HDMI_COLORSPACE_YUV420;
+	else if (crtc_state->output_format == CRTC_OUTPUT_YCBCR444)
+		frame.avi.colorspace = HDMI_COLORSPACE_YUV444;
 	else
 		frame.avi.colorspace = HDMI_COLORSPACE_RGB;
 
-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v3 3/7] drm/i915: Check LSPCON vendor OUI

[Shashank Sharma]

Intel LSPCON chip is provided by 2 vendors:
- Megachips America (MCA)
- Parade technologies (Parade tech)

Its important to know the vendor of this chip, as the address to
write AVI infoframes is different for those two.

This patch reads the vendor OUI signature, and marks into LSPCON
encoder structure for future usages.

This patch also does a small re-arrangement of the code, by moving
lspcon mode change into probe function.

V2: Use dp->desc for OUI detection, dont add a helper for this
    (Ville)
V3: Rebase, Added r-b from Maarten

Cc: Imre Deak <imre.deak@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Reviewed-by: Lankhorst <maarten.lankhorst@linux.intel.com>
Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
---
 drivers/gpu/drm/i915/intel_drv.h    |  6 ++++
 drivers/gpu/drm/i915/intel_lspcon.c | 69 +++++++++++++++++++++++++++++--------
 2 files changed, 61 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index a393342..ba6a599 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1055,9 +1055,15 @@ struct intel_dp {
 	struct intel_dp_compliance compliance;
 };
 
+enum lspcon_vendor {
+	LSPCON_VENDOR_MCA,
+	LSPCON_VENDOR_PARADE
+};
+
 struct intel_lspcon {
 	bool active;
 	enum drm_lspcon_mode mode;
+	enum lspcon_vendor vendor;
 };
 
 struct intel_digital_port {
diff --git a/drivers/gpu/drm/i915/intel_lspcon.c b/drivers/gpu/drm/i915/intel_lspcon.c
index dcbc786..946dfd0 100644
--- a/drivers/gpu/drm/i915/intel_lspcon.c
+++ b/drivers/gpu/drm/i915/intel_lspcon.c
@@ -27,6 +27,10 @@
 #include <drm/drm_dp_dual_mode_helper.h>
 #include "intel_drv.h"
 
+/* LSPCON OUI Vendor ID(signatures) */
+#define LSPCON_VENDOR_PARADE_OUI 0x001CF8
+#define LSPCON_VENDOR_MCA_OUI 0x0060AD
+
 static struct intel_dp *lspcon_to_intel_dp(struct intel_lspcon *lspcon)
 {
 	struct intel_digital_port *dig_port =
@@ -50,6 +54,40 @@ static const char *lspcon_mode_name(enum drm_lspcon_mode mode)
 	}
 }
 
+static bool lspcon_detect_vendor(struct intel_lspcon *lspcon)
+{
+	struct intel_dp *dp = lspcon_to_intel_dp(lspcon);
+	struct drm_dp_dpcd_ident *ident;
+	u32 vendor_oui;
+
+	if (drm_dp_read_desc(&dp->aux, &dp->desc, drm_dp_is_branch(dp->dpcd))) {
+		DRM_ERROR("Can't read description\n");
+		return false;
+	}
+
+	ident = &dp->desc.ident;
+	vendor_oui = (ident->oui[0] << 16) | (ident->oui[1] << 8) |
+		      ident->oui[2];
+
+	switch (vendor_oui) {
+	case LSPCON_VENDOR_MCA_OUI:
+		lspcon->vendor = LSPCON_VENDOR_MCA;
+		DRM_DEBUG_KMS("Vendor: Mega Chips\n");
+		break;
+
+	case LSPCON_VENDOR_PARADE_OUI:
+		lspcon->vendor = LSPCON_VENDOR_PARADE;
+		DRM_DEBUG_KMS("Vendor: Parade Tech\n");
+		break;
+
+	default:
+		DRM_ERROR("Invalid/Unknown vendor OUI\n");
+		return false;
+	}
+
+	return true;
+}
+
 static enum drm_lspcon_mode lspcon_get_current_mode(struct intel_lspcon *lspcon)
 {
 	enum drm_lspcon_mode current_mode;
@@ -159,7 +197,18 @@ static bool lspcon_probe(struct intel_lspcon *lspcon)
 	/* Yay ... got a LSPCON device */
 	DRM_DEBUG_KMS("LSPCON detected\n");
 	lspcon->mode = lspcon_wait_mode(lspcon, expected_mode);
-	lspcon->active = true;
+
+	/*
+	 * In the SW state machine, lets Put LSPCON in PCON mode only.
+	 * In this way, it will work with both HDMI 1.4 sinks as well as HDMI
+	 * 2.0 sinks.
+	 */
+	if (lspcon->active && lspcon->mode != DRM_LSPCON_MODE_PCON) {
+		if (lspcon_change_mode(lspcon, DRM_LSPCON_MODE_PCON) < 0) {
+			DRM_ERROR("LSPCON mode change to PCON failed\n");
+			return false;
+		}
+	}
 	return true;
 }
 
@@ -231,25 +280,17 @@ bool lspcon_init(struct intel_digital_port *intel_dig_port)
 		return false;
 	}
 
-	/*
-	* In the SW state machine, lets Put LSPCON in PCON mode only.
-	* In this way, it will work with both HDMI 1.4 sinks as well as HDMI
-	* 2.0 sinks.
-	*/
-	if (lspcon->active && lspcon->mode != DRM_LSPCON_MODE_PCON) {
-		if (lspcon_change_mode(lspcon, DRM_LSPCON_MODE_PCON) < 0) {
-			DRM_ERROR("LSPCON mode change to PCON failed\n");
-			return false;
-		}
-	}
-
 	if (!intel_dp_read_dpcd(dp)) {
 		DRM_ERROR("LSPCON DPCD read failed\n");
 		return false;
 	}
 
-	drm_dp_read_desc(&dp->aux, &dp->desc, drm_dp_is_branch(dp->dpcd));
+	if (!lspcon_detect_vendor(lspcon)) {
+		DRM_ERROR("LSPCON vendor detection failed\n");
+		return false;
+	}
 
+	lspcon->active = true;
 	DRM_DEBUG_KMS("Success: LSPCON init\n");
 	return true;
 }
-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v3 4/7] drm/i915: Add AVI infoframe support for LSPCON

[Shashank Sharma]

In order to pass AVI infoframes to LSPCON devices, a source has to
write them in a vendor recommended method and location.

This patch series:
- adds generic LSPCON infoframe setup functions.
- registers these functions into existing AVI infoframe framework.
- triggers these functions from modeset sequence.

Next patches in the series will add vendor specific code.

V2: Added new parameter to align with new definition of
    drm_hdmi_avi_infoframe_quant_range
V3: Added r-b from Maarten (for V2)
    Added new parameter output_format in struct lspcon to accommodate
    Ville's review comments on last patch of the series

Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
Cc: Imre Deak <imre.deak@linux.intel.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
---
 drivers/gpu/drm/i915/intel_ddi.c    | 19 +++++++++++---
 drivers/gpu/drm/i915/intel_drv.h    | 14 ++++++++++-
 drivers/gpu/drm/i915/intel_hdmi.c   | 13 +++++++---
 drivers/gpu/drm/i915/intel_lspcon.c | 49 +++++++++++++++++++++++++++++++++++++
 4 files changed, 87 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 84327e7..7b89f2a 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -2238,10 +2238,22 @@ static void intel_ddi_pre_enable(struct intel_encoder *encoder,
 
 	intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, true);
 
-	if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI))
+	if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI)) {
 		intel_ddi_pre_enable_hdmi(encoder, crtc_state, conn_state);
-	else
+	} else {
+		struct intel_lspcon *lspcon =
+				enc_to_intel_lspcon(&encoder->base);
+
 		intel_ddi_pre_enable_dp(encoder, crtc_state, conn_state);
+		if (lspcon->active) {
+			struct intel_digital_port *dig_port =
+					enc_to_dig_port(&encoder->base);
+
+			dig_port->set_infoframes(&encoder->base,
+						 crtc_state->has_infoframe,
+						 crtc_state, conn_state);
+		}
+	}
 }
 
 static void intel_disable_ddi_buf(struct intel_encoder *encoder)
@@ -2892,8 +2904,6 @@ void intel_ddi_init(struct drm_i915_private *dev_priv, enum port port)
 	intel_encoder->crtc_mask = (1 << 0) | (1 << 1) | (1 << 2);
 	intel_encoder->cloneable = 0;
 
-	intel_infoframe_init(intel_dig_port);
-
 	if (init_dp) {
 		if (!intel_ddi_init_dp_connector(intel_dig_port))
 			goto err;
@@ -2923,6 +2933,7 @@ void intel_ddi_init(struct drm_i915_private *dev_priv, enum port port)
 				port_name(port));
 	}
 
+	intel_infoframe_init(intel_dig_port);
 	return;
 
 err:
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index ba6a599..f2e8752 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1064,6 +1064,7 @@ struct intel_lspcon {
 	bool active;
 	enum drm_lspcon_mode mode;
 	enum lspcon_vendor vendor;
+	enum crtc_output_format output_format;
 };
 
 struct intel_digital_port {
@@ -1189,6 +1190,12 @@ static inline struct intel_dp *enc_to_intel_dp(struct drm_encoder *encoder)
 	return &enc_to_dig_port(encoder)->dp;
 }
 
+static inline struct intel_lspcon *
+enc_to_intel_lspcon(struct drm_encoder *encoder)
+{
+	return &enc_to_dig_port(encoder)->lspcon;
+}
+
 static inline struct intel_digital_port *
 dp_to_dig_port(struct intel_dp *intel_dp)
 {
@@ -1703,7 +1710,6 @@ void intel_hdmi_handle_sink_scrambling(struct intel_encoder *intel_encoder,
 void intel_dp_dual_mode_set_tmds_output(struct intel_hdmi *hdmi, bool enable);
 void intel_infoframe_init(struct intel_digital_port *intel_dig_port);
 
-
 /* intel_lvds.c */
 void intel_lvds_init(struct drm_i915_private *dev_priv);
 struct intel_encoder *intel_get_lvds_encoder(struct drm_device *dev);
@@ -2031,6 +2037,12 @@ void intel_color_load_luts(struct drm_crtc_state *crtc_state);
 bool lspcon_init(struct intel_digital_port *intel_dig_port);
 void lspcon_resume(struct intel_lspcon *lspcon);
 void lspcon_wait_pcon_mode(struct intel_lspcon *lspcon);
+void lspcon_set_infoframes(struct drm_encoder *encoder,
+			   bool enable,
+			   const struct intel_crtc_state *crtc_state,
+			   const struct drm_connector_state *conn_state);
+bool lspcon_infoframe_enabled(struct drm_encoder *encoder,
+			      const struct intel_crtc_state *pipe_config);
 
 /* intel_pipe_crc.c */
 int intel_pipe_crc_create(struct drm_minor *minor);
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 258bb51..0609f11 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -1999,9 +1999,16 @@ void intel_infoframe_init(struct intel_digital_port *intel_dig_port)
 		intel_dig_port->set_infoframes = g4x_set_infoframes;
 		intel_dig_port->infoframe_enabled = g4x_infoframe_enabled;
 	} else if (HAS_DDI(dev_priv)) {
-		intel_dig_port->write_infoframe = hsw_write_infoframe;
-		intel_dig_port->set_infoframes = hsw_set_infoframes;
-		intel_dig_port->infoframe_enabled = hsw_infoframe_enabled;
+		if (intel_dig_port->lspcon.active) {
+			intel_dig_port->set_infoframes = lspcon_set_infoframes;
+			intel_dig_port->infoframe_enabled =
+						lspcon_infoframe_enabled;
+		} else {
+			intel_dig_port->set_infoframes = hsw_set_infoframes;
+			intel_dig_port->infoframe_enabled =
+						hsw_infoframe_enabled;
+			intel_dig_port->write_infoframe = hsw_write_infoframe;
+		}
 	} else if (HAS_PCH_IBX(dev_priv)) {
 		intel_dig_port->write_infoframe = ibx_write_infoframe;
 		intel_dig_port->set_infoframes = ibx_set_infoframes;
diff --git a/drivers/gpu/drm/i915/intel_lspcon.c b/drivers/gpu/drm/i915/intel_lspcon.c
index 946dfd0..32d4198 100644
--- a/drivers/gpu/drm/i915/intel_lspcon.c
+++ b/drivers/gpu/drm/i915/intel_lspcon.c
@@ -235,6 +235,55 @@ static void lspcon_resume_in_pcon_wa(struct intel_lspcon *lspcon)
 	DRM_DEBUG_KMS("LSPCON DP descriptor mismatch after resume\n");
 }
 
+void lspcon_set_infoframes(struct drm_encoder *encoder,
+			   bool enable,
+			   const struct intel_crtc_state *crtc_state,
+			   const struct drm_connector_state *conn_state)
+{
+	ssize_t ret;
+	union hdmi_infoframe frame;
+	uint8_t buf[VIDEO_DIP_DATA_SIZE];
+	struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
+	struct intel_lspcon *lspcon = &dig_port->lspcon;
+	struct intel_dp *intel_dp = &dig_port->dp;
+	struct drm_connector *connector = &intel_dp->attached_connector->base;
+	const struct drm_display_mode *mode = &crtc_state->base.adjusted_mode;
+	bool is_hdmi2_sink = connector->display_info.hdmi.scdc.supported;
+
+	if (!lspcon->active) {
+		DRM_ERROR("Writing infoframes while LSPCON disabled ?\n");
+		return;
+	}
+
+	ret = drm_hdmi_avi_infoframe_from_display_mode(&frame.avi,
+						       mode, is_hdmi2_sink);
+	if (ret < 0) {
+		DRM_ERROR("couldn't fill AVI infoframe\n");
+		return;
+	}
+
+	drm_hdmi_avi_infoframe_quant_range(&frame.avi, mode,
+					   crtc_state->limited_color_range ?
+					   HDMI_QUANTIZATION_RANGE_LIMITED :
+					   HDMI_QUANTIZATION_RANGE_FULL,
+					   false, is_hdmi2_sink);
+
+	ret = hdmi_infoframe_pack(&frame, buf, sizeof(buf));
+	if (ret < 0) {
+		DRM_ERROR("Failed to pack AVI IF\n");
+		return;
+	}
+
+	dig_port->write_infoframe(encoder, crtc_state, HDMI_INFOFRAME_TYPE_AVI,
+				  buf, ret);
+}
+
+bool lspcon_infoframe_enabled(struct drm_encoder *encoder,
+			      const struct intel_crtc_state *pipe_config)
+{
+	return enc_to_intel_lspcon(encoder)->active;
+}
+
 void lspcon_resume(struct intel_lspcon *lspcon)
 {
 	enum drm_lspcon_mode expected_mode;
-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v3 5/7] drm/i915: Write AVI infoframes for MCA LSPCON

[Shashank Sharma]

As LSPCON is a DP branch device, LSPCON vendors define
specific methods to pass AVI infoframes to the the chip.
This patch adds:
- a generic wrapper function for writing AVI infoframes for
  all LSPCON devices.
- a vendor specific function to wrire AVI infoframes into
  MCA LSPCON devices.

V2: Rebase
V3: Added r-b from Maarten

Cc: Imre Deak <imre.deak@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
---
 drivers/gpu/drm/i915/intel_drv.h    |  4 ++
 drivers/gpu/drm/i915/intel_hdmi.c   |  2 +
 drivers/gpu/drm/i915/intel_lspcon.c | 80 +++++++++++++++++++++++++++++++++++++
 3 files changed, 86 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index f2e8752..2de6b41 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -2037,6 +2037,10 @@ void intel_color_load_luts(struct drm_crtc_state *crtc_state);
 bool lspcon_init(struct intel_digital_port *intel_dig_port);
 void lspcon_resume(struct intel_lspcon *lspcon);
 void lspcon_wait_pcon_mode(struct intel_lspcon *lspcon);
+void lspcon_write_infoframe(struct drm_encoder *encoder,
+			     const struct intel_crtc_state *crtc_state,
+			     enum hdmi_infoframe_type type,
+			     const void *buf, ssize_t len);
 void lspcon_set_infoframes(struct drm_encoder *encoder,
 			   bool enable,
 			   const struct intel_crtc_state *crtc_state,
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 0609f11..7b63889 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -2000,6 +2000,8 @@ void intel_infoframe_init(struct intel_digital_port *intel_dig_port)
 		intel_dig_port->infoframe_enabled = g4x_infoframe_enabled;
 	} else if (HAS_DDI(dev_priv)) {
 		if (intel_dig_port->lspcon.active) {
+			intel_dig_port->write_infoframe =
+					lspcon_write_infoframe;
 			intel_dig_port->set_infoframes = lspcon_set_infoframes;
 			intel_dig_port->infoframe_enabled =
 						lspcon_infoframe_enabled;
diff --git a/drivers/gpu/drm/i915/intel_lspcon.c b/drivers/gpu/drm/i915/intel_lspcon.c
index 32d4198..0571108 100644
--- a/drivers/gpu/drm/i915/intel_lspcon.c
+++ b/drivers/gpu/drm/i915/intel_lspcon.c
@@ -31,6 +31,12 @@
 #define LSPCON_VENDOR_PARADE_OUI 0x001CF8
 #define LSPCON_VENDOR_MCA_OUI 0x0060AD
 
+/* AUX addresses to write MCA AVI IF */
+#define LSPCON_MCA_AVI_IF_WRITE_OFFSET 0x5C0
+#define LSPCON_MCA_AVI_IF_CTRL 0x5DF
+#define  LSPCON_MCA_AVI_IF_KICKOFF (1 << 0)
+#define  LSPCON_MCA_AVI_IF_HANDLED (1 << 1)
+
 static struct intel_dp *lspcon_to_intel_dp(struct intel_lspcon *lspcon)
 {
 	struct intel_digital_port *dig_port =
@@ -235,6 +241,80 @@ static void lspcon_resume_in_pcon_wa(struct intel_lspcon *lspcon)
 	DRM_DEBUG_KMS("LSPCON DP descriptor mismatch after resume\n");
 }
 
+static bool _lspcon_write_avi_infoframe_mca(struct drm_dp_aux *aux,
+					     const uint8_t *buffer, ssize_t len)
+{
+	int ret;
+	uint32_t val = 0;
+	uint16_t reg;
+	const uint8_t *data = buffer;
+
+	reg = LSPCON_MCA_AVI_IF_WRITE_OFFSET;
+	while (val < len) {
+		ret = drm_dp_dpcd_write(aux, reg, (void *)data, 1);
+		if (ret < 0) {
+			DRM_ERROR("DPCD write failed, add:0x%x\n", reg);
+			return false;
+		}
+		val++; reg++; data++;
+	}
+
+	val = 0;
+	reg = LSPCON_MCA_AVI_IF_CTRL;
+	ret = drm_dp_dpcd_read(aux, reg, &val, 1);
+	if (ret < 0) {
+		DRM_ERROR("DPCD read failed, address 0x%x\n", reg);
+		return false;
+	}
+
+	/* Indicate LSPCON chip about infoframe, clear bit 1 and set bit 0 */
+	val &= ~LSPCON_MCA_AVI_IF_HANDLED;
+	val |= LSPCON_MCA_AVI_IF_KICKOFF;
+
+	ret = drm_dp_dpcd_write(aux, reg, &val, 1);
+	if (ret < 0) {
+		DRM_ERROR("DPCD read failed, address 0x%x\n", reg);
+		return false;
+	}
+
+	val = 0;
+	ret = drm_dp_dpcd_read(aux, reg, &val, 1);
+	if (ret < 0) {
+		DRM_ERROR("DPCD read failed, address 0x%x\n", reg);
+		return false;
+	}
+
+	if (val == LSPCON_MCA_AVI_IF_HANDLED)
+		DRM_DEBUG_KMS("AVI IF handled by FW\n");
+
+	return true;
+}
+
+void lspcon_write_infoframe(struct drm_encoder *encoder,
+			     const struct intel_crtc_state *crtc_state,
+			     enum hdmi_infoframe_type type,
+			     const void *frame, ssize_t len)
+{
+	bool ret = true;
+	struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
+	struct intel_lspcon *lspcon = enc_to_intel_lspcon(encoder);
+
+	/* LSPCON only needs AVI IF */
+	if (type != HDMI_INFOFRAME_TYPE_AVI)
+		return;
+
+	if (lspcon->vendor == LSPCON_VENDOR_MCA)
+		ret = _lspcon_write_avi_infoframe_mca(&intel_dp->aux,
+						      frame, len);
+	if (!ret) {
+		DRM_ERROR("Failed to write AVI infoframes\n");
+		return;
+	}
+
+	DRM_DEBUG_DRIVER("AVI infoframes updated successfully\n");
+}
+
+
 void lspcon_set_infoframes(struct drm_encoder *encoder,
 			   bool enable,
 			   const struct intel_crtc_state *crtc_state,
-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v3 6/7] drm/i915: Write AVI infoframes for Parade LSPCON

[Shashank Sharma]

Different LSPCON vendors specify their custom methods to pass
AVI infoframes to the LSPCON chip, so does Parade tech.

This patch adds functions to arrange and write AVI infoframes
into Parade LSPCON chips.

V2: rebase
V3: Added r-b from Maarten

Cc: Imre Deak <imre.deak@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
---
 drivers/gpu/drm/i915/intel_lspcon.c | 119 +++++++++++++++++++++++++++++++++++-
 1 file changed, 118 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_lspcon.c b/drivers/gpu/drm/i915/intel_lspcon.c
index 0571108..066ea91 100644
--- a/drivers/gpu/drm/i915/intel_lspcon.c
+++ b/drivers/gpu/drm/i915/intel_lspcon.c
@@ -37,6 +37,12 @@
 #define  LSPCON_MCA_AVI_IF_KICKOFF (1 << 0)
 #define  LSPCON_MCA_AVI_IF_HANDLED (1 << 1)
 
+/* AUX addresses to write Parade AVI IF */
+#define LSPCON_PARADE_AVI_IF_WRITE_OFFSET 0x516
+#define LSPCON_PARADE_AVI_IF_CTRL 0x51E
+#define  LSPCON_PARADE_AVI_IF_KICKOFF (1 << 7)
+#define LSPCON_PARADE_AVI_IF_DATA_SIZE 32
+
 static struct intel_dp *lspcon_to_intel_dp(struct intel_lspcon *lspcon)
 {
 	struct intel_digital_port *dig_port =
@@ -241,6 +247,113 @@ static void lspcon_resume_in_pcon_wa(struct intel_lspcon *lspcon)
 	DRM_DEBUG_KMS("LSPCON DP descriptor mismatch after resume\n");
 }
 
+static bool lspcon_parade_fw_ready(struct drm_dp_aux *aux)
+{
+	u8 avi_if_ctrl;
+	u8 retry;
+	ssize_t ret;
+
+	/* Check if LSPCON FW is ready for data */
+	for (retry = 0; retry < 5; retry++) {
+
+		if (retry)
+			usleep_range(200, 300);
+
+		ret = drm_dp_dpcd_read(aux, LSPCON_PARADE_AVI_IF_CTRL,
+				       &avi_if_ctrl, 1);
+		if (ret < 0) {
+			DRM_ERROR("Failed to read AVI IF control\n");
+			return false;
+		}
+
+		if ((avi_if_ctrl & LSPCON_PARADE_AVI_IF_KICKOFF) == 0)
+			return true;
+	}
+
+	DRM_ERROR("Parade FW not ready to accept AVI IF\n");
+	return false;
+}
+
+static bool _lspcon_parade_write_infoframe_blocks(struct drm_dp_aux *aux,
+					       uint8_t *avi_buf)
+{
+	u8 avi_if_ctrl;
+	u8 block_count = 0;
+	u8 *data;
+	uint16_t reg;
+	ssize_t ret;
+
+	while (block_count < 4) {
+
+		if (!lspcon_parade_fw_ready(aux)) {
+			DRM_DEBUG_KMS("LSPCON FW not ready, block %d\n",
+				       block_count);
+			return false;
+		}
+
+		reg = LSPCON_PARADE_AVI_IF_WRITE_OFFSET;
+		data = avi_buf + block_count * 8;
+		ret = drm_dp_dpcd_write(aux, reg, data, 8);
+		if (ret < 0) {
+			DRM_ERROR("Failed to write AVI IF block %d\n",
+				   block_count);
+			return false;
+		}
+
+		/*
+		 * Once a block of data is written, we have to inform the FW
+		 * about this by writing into avi infoframe control register:
+		 * - set the kickoff bit[7] to 1
+		 * - write the block no. to bits[1:0]
+		 */
+		reg = LSPCON_PARADE_AVI_IF_CTRL;
+		avi_if_ctrl = LSPCON_PARADE_AVI_IF_KICKOFF | block_count;
+		ret = drm_dp_dpcd_write(aux, reg, &avi_if_ctrl, 1);
+		if (ret < 0) {
+			DRM_ERROR("Failed to update (0x%x), block %d\n",
+					reg, block_count);
+			return false;
+		}
+
+		block_count++;
+	}
+
+	DRM_DEBUG_KMS("Wrote AVI IF blocks successfully\n");
+	return true;
+}
+
+static bool _lspcon_write_avi_infoframe_parade(struct drm_dp_aux *aux,
+					       const uint8_t *frame,
+					       ssize_t len)
+{
+	uint8_t avi_if[LSPCON_PARADE_AVI_IF_DATA_SIZE] = {1, };
+
+	/*
+	 * Parade's frames contains 32 bytes of data, divided
+	 * into 4 frames:
+	 *	Token byte (first byte of first frame, must be non-zero)
+	 *	HB0 to HB2	 from AVI IF (3 bytes header)
+	 *	PB0 to PB27 from AVI IF (28 bytes data)
+	 * So it should look like this
+	 *	first block: | <token> <HB0-HB2> <DB0-DB3> |
+	 *	next 3 blocks: |<DB4-DB11>|<DB12-DB19>|<DB20-DB28>|
+	 */
+
+	if (len > LSPCON_PARADE_AVI_IF_DATA_SIZE - 1) {
+		DRM_ERROR("Invalid length of infoframes\n");
+		return false;
+	}
+
+	memcpy(&avi_if[1], frame, len);
+
+	if (!_lspcon_parade_write_infoframe_blocks(aux, avi_if)) {
+		DRM_DEBUG_KMS("Failed to write infoframe blocks\n");
+		return false;
+	}
+
+	return true;
+}
+
 static bool _lspcon_write_avi_infoframe_mca(struct drm_dp_aux *aux,
 					     const uint8_t *buffer, ssize_t len)
 {
@@ -295,7 +408,7 @@ void lspcon_write_infoframe(struct drm_encoder *encoder,
 			     enum hdmi_infoframe_type type,
 			     const void *frame, ssize_t len)
 {
-	bool ret = true;
+	bool ret;
 	struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
 	struct intel_lspcon *lspcon = enc_to_intel_lspcon(encoder);
 
@@ -306,6 +419,10 @@ void lspcon_write_infoframe(struct drm_encoder *encoder,
 	if (lspcon->vendor == LSPCON_VENDOR_MCA)
 		ret = _lspcon_write_avi_infoframe_mca(&intel_dp->aux,
 						      frame, len);
+	else
+		ret = _lspcon_write_avi_infoframe_parade(&intel_dp->aux,
+							 frame, len);
+
 	if (!ret) {
 		DRM_ERROR("Failed to write AVI infoframes\n");
 		return;
-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v3 7/7] drm/i915: Add YCBCR 4:2:0/4:4:4 support for LSPCON

[Shashank Sharma]

LSPCON chips can generate YCBCR outputs, if asked nicely :).

In order to generate YCBCR 4:2:0 outputs, a source must:
- send YCBCR 4:4:4 signals to LSPCON
- program color space as 4:2:0 in AVI infoframes

Whereas for YCBCR 4:4:4 outputs, the source must:
- send YCBCR 4:4:4 signals to LSPCON
- program color space as 4:4:4 in AVI infoframes

So for both 4:2:0 as well as 4:4:4 outputs, we are driving the
pipe for YCBCR 4:4:4 output, but AVI infoframe's color space
information indicates LSPCON FW to start scaling down from YCBCR
4:4:4 and generate YCBCR 4:2:0 output. As the scaling is done by
LSPCON device, we need not to reserve a scaler for 4:2:0 outputs.

V2: rebase
V3: Addressed review comments from Ville
    - add enum crtc_output_format instead of bool ycbcr420
    - use crtc_output_format=4:4:4 for modeset of LSPCON 4:2:0 output
      cases in this way we will have YCBCR 4:4:4 framework ready (except
      the ABI part)

Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h     |  2 ++
 drivers/gpu/drm/i915/intel_ddi.c    |  7 +++++++
 drivers/gpu/drm/i915/intel_dp.c     | 10 ++++++++++
 drivers/gpu/drm/i915/intel_drv.h    |  2 ++
 drivers/gpu/drm/i915/intel_lspcon.c | 28 ++++++++++++++++++++++++++++
 5 files changed, 49 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 966e4df..45ee264 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -8547,6 +8547,8 @@ enum skl_power_gate {
 #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		(2<<3)
 #define  TRANS_MSA_6_BPC		(0<<5)
 #define  TRANS_MSA_8_BPC		(1<<5)
 #define  TRANS_MSA_10_BPC		(2<<5)
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 7b89f2a..7616f6f 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -1499,6 +1499,13 @@ void intel_ddi_set_pipe_settings(const struct intel_crtc_state *crtc_state)
 		break;
 	}
 
+	/*
+	 * As per DP 1.2 spec section 2.3.4.3 while sending
+	 * YCBCR 444 signals we should program MSA MISC1/0 fields with
+	 * colorspace information.
+	 */
+	if (crtc_state->output_format == CRTC_OUTPUT_YCBCR444)
+		temp |= TRANS_MSA_SAMPLING_444 | TRANS_MSA_CLRSP_YCBCR;
 	I915_WRITE(TRANS_MSA_MISC(cpu_transcoder), temp);
 }
 
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 35c5299..3bf82ea 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1613,6 +1613,7 @@ intel_dp_compute_config(struct intel_encoder *encoder,
 	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
 	struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode;
 	struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
+	struct intel_lspcon *lspcon = enc_to_intel_lspcon(&encoder->base);
 	enum port port = encoder->port;
 	struct intel_crtc *intel_crtc = to_intel_crtc(pipe_config->base.crtc);
 	struct intel_connector *intel_connector = intel_dp->attached_connector;
@@ -1642,6 +1643,15 @@ intel_dp_compute_config(struct intel_encoder *encoder,
 	if (HAS_PCH_SPLIT(dev_priv) && !HAS_DDI(dev_priv) && port != PORT_A)
 		pipe_config->has_pch_encoder = true;
 
+	if (lspcon->active) {
+		struct drm_connector *connector = &intel_connector->base;
+
+		if (lspcon_ycbcr420_config(connector, pipe_config)) {
+			pipe_config->output_format = CRTC_OUTPUT_YCBCR444;
+			lspcon->output_format = CRTC_OUTPUT_YCBCR420;
+		}
+	}
+
 	pipe_config->has_drrs = false;
 	if (IS_G4X(dev_priv) || port == PORT_A)
 		pipe_config->has_audio = false;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 2de6b41..5edba06 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -2047,6 +2047,8 @@ void lspcon_set_infoframes(struct drm_encoder *encoder,
 			   const struct drm_connector_state *conn_state);
 bool lspcon_infoframe_enabled(struct drm_encoder *encoder,
 			      const struct intel_crtc_state *pipe_config);
+bool lspcon_ycbcr420_config(struct drm_connector *connector,
+			     struct intel_crtc_state *config);
 
 /* intel_pipe_crc.c */
 int intel_pipe_crc_create(struct drm_minor *minor);
diff --git a/drivers/gpu/drm/i915/intel_lspcon.c b/drivers/gpu/drm/i915/intel_lspcon.c
index 066ea91..cb88138 100644
--- a/drivers/gpu/drm/i915/intel_lspcon.c
+++ b/drivers/gpu/drm/i915/intel_lspcon.c
@@ -180,6 +180,25 @@ static bool lspcon_wake_native_aux_ch(struct intel_lspcon *lspcon)
 	return true;
 }
 
+bool lspcon_ycbcr420_config(struct drm_connector *connector,
+			     struct intel_crtc_state *config)
+{
+	struct drm_display_info *info = &connector->display_info;
+	struct drm_display_mode *mode = &config->base.adjusted_mode;
+
+	if (drm_mode_is_420_only(info, mode)) {
+		if (!connector->ycbcr_420_allowed) {
+			DRM_ERROR("Platform doesn't support YCBCR420 output\n");
+			return false;
+		}
+
+		config->port_clock /= 2;
+		return true;
+	}
+
+	return false;
+}
+
 static bool lspcon_probe(struct intel_lspcon *lspcon)
 {
 	int retry;
@@ -459,6 +478,13 @@ void lspcon_set_infoframes(struct drm_encoder *encoder,
 		return;
 	}
 
+	if (lspcon->output_format == CRTC_OUTPUT_YCBCR420)
+		frame.avi.colorspace = HDMI_COLORSPACE_YUV420;
+	else if (lspcon->output_format == CRTC_OUTPUT_YCBCR444)
+		frame.avi.colorspace = HDMI_COLORSPACE_YUV444;
+	else
+		frame.avi.colorspace = HDMI_COLORSPACE_RGB;
+
 	drm_hdmi_avi_infoframe_quant_range(&frame.avi, mode,
 					   crtc_state->limited_color_range ?
 					   HDMI_QUANTIZATION_RANGE_LIMITED :
@@ -512,6 +538,7 @@ bool lspcon_init(struct intel_digital_port *intel_dig_port)
 	struct intel_lspcon *lspcon = &intel_dig_port->lspcon;
 	struct drm_device *dev = intel_dig_port->base.base.dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
+	struct drm_connector *connector = &dp->attached_connector->base;
 
 	if (!HAS_LSPCON(dev_priv)) {
 		DRM_ERROR("LSPCON is not supported on this platform\n");
@@ -536,6 +563,7 @@ bool lspcon_init(struct intel_digital_port *intel_dig_port)
 		return false;
 	}
 
+	connector->ycbcr_420_allowed = true;
 	lspcon->active = true;
 	DRM_DEBUG_KMS("Success: LSPCON init\n");
 	return true;
-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: warning for YCBCR 4:2:0/4:4:4 output support for LSPCON

[Patchwork]

== Series Details ==

Series: YCBCR 4:2:0/4:4:4 output support for LSPCON
URL   : https://patchwork.freedesktop.org/series/36068/
State : warning

== Summary ==

Series 36068v1 YCBCR 4:2:0/4:4:4 output support for LSPCON
https://patchwork.freedesktop.org/api/1.0/series/36068/revisions/1/mbox/

Test debugfs_test:
        Subgroup read_all_entries:
                dmesg-warn -> PASS       (fi-elk-e7500) fdo#103989 +2
Test gem_exec_suspend:
        Subgroup basic-s3:
                pass       -> DMESG-WARN (fi-kbl-7567u)
Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-a:
                pass       -> DMESG-WARN (fi-kbl-7567u)
                pass       -> DMESG-WARN (fi-kbl-r) fdo#104172 +1
        Subgroup suspend-read-crc-pipe-b:
                pass       -> DMESG-WARN (fi-kbl-7567u)
        Subgroup suspend-read-crc-pipe-c:
                pass       -> DMESG-WARN (fi-kbl-7567u)
Test kms_psr_sink_crc:
        Subgroup psr_basic:
                dmesg-warn -> PASS       (fi-skl-6700hq) fdo#104260

fdo#103989 https://bugs.freedesktop.org/show_bug.cgi?id=103989
fdo#104172 https://bugs.freedesktop.org/show_bug.cgi?id=104172
fdo#104260 https://bugs.freedesktop.org/show_bug.cgi?id=104260

fi-bdw-5557u     total:288  pass:267  dwarn:0   dfail:0   fail:0   skip:21  time:422s
fi-bdw-gvtdvm    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:426s
fi-blb-e6850     total:288  pass:223  dwarn:1   dfail:0   fail:0   skip:64  time:367s
fi-bsw-n3050     total:288  pass:242  dwarn:0   dfail:0   fail:0   skip:46  time:478s
fi-bwr-2160      total:288  pass:183  dwarn:0   dfail:0   fail:0   skip:105 time:275s
fi-bxt-dsi       total:288  pass:258  dwarn:0   dfail:0   fail:0   skip:30  time:477s
fi-bxt-j4205     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:478s
fi-byt-j1900     total:288  pass:253  dwarn:0   dfail:0   fail:0   skip:35  time:463s
fi-byt-n2820     total:288  pass:249  dwarn:0   dfail:0   fail:0   skip:39  time:453s
fi-elk-e7500     total:224  pass:168  dwarn:10  dfail:0   fail:0   skip:45 
fi-glk-1         total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:508s
fi-hsw-4770      total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:392s
fi-hsw-4770r     total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:405s
fi-ilk-650       total:288  pass:228  dwarn:0   dfail:0   fail:0   skip:60  time:411s
fi-ivb-3520m     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:449s
fi-ivb-3770      total:288  pass:255  dwarn:0   dfail:0   fail:0   skip:33  time:408s
fi-kbl-7500u     total:288  pass:263  dwarn:1   dfail:0   fail:0   skip:24  time:463s
fi-kbl-7560u     total:288  pass:268  dwarn:1   dfail:0   fail:0   skip:19  time:497s
fi-kbl-7567u     total:288  pass:264  dwarn:4   dfail:0   fail:0   skip:20  time:452s
fi-kbl-r         total:288  pass:260  dwarn:1   dfail:0   fail:0   skip:27  time:500s
fi-pnv-d510      total:288  pass:222  dwarn:1   dfail:0   fail:0   skip:65  time:572s
fi-skl-6260u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:429s
fi-skl-6600u     total:288  pass:260  dwarn:1   dfail:0   fail:0   skip:27  time:505s
fi-skl-6700hq    total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:522s
fi-skl-6700k2    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:496s
fi-skl-6770hq    total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:484s
fi-skl-gvtdvm    total:288  pass:265  dwarn:0   dfail:0   fail:0   skip:23  time:431s
fi-snb-2520m     total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:520s
fi-snb-2600      total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:394s
Blacklisted hosts:
fi-cfl-s2        total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:563s
fi-cnl-y         total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:595s
fi-glk-dsi       total:288  pass:258  dwarn:0   dfail:0   fail:0   skip:30  time:467s
fi-gdg-551 failed to collect. IGT log at Patchwork_7616/fi-gdg-551/igt.log

914d61a8fb5fc53f6b0366167210468147495b3f drm-tip: 2018y-01m-05d-09h-12m-18s UTC integration manifest
7e873eeda3c9 drm/i915: Add YCBCR 4:2:0/4:4:4 support for LSPCON
493d65ec6f3b drm/i915: Write AVI infoframes for Parade LSPCON
0681b480b514 drm/i915: Write AVI infoframes for MCA LSPCON
7dc583b6251a drm/i915: Add AVI infoframe support for LSPCON
3f657d86adf2 drm/i915: Check LSPCON vendor OUI
be479b386bb5 drm/i915: Add CRTC output format YCBCR 4:4:4
0344c5ac79d0 drm/i915: Add CRTC output format YCBCR 4:2:0

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7616/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 1/7] drm/i915: Add CRTC output format YCBCR 4:2:0

[Maarten Lankhorst]

Op 05-01-18 om 10:45 schreef Shashank Sharma:
> Currently, we are using a bool in CRTC state (state->ycbcr420),
> to indicate modeset, that the output format is YCBCR 4:2:0. Now in
> order to support other YCBCR formats, we will need more such flags.
>
> The idea behind this patch is to replace this bool with an enum,
> and plug this in in the existing YCBCR 4:2:0 framework in such a
> way that this can be re-used for YCBCR 4:4:4 and other output formats too.
>
> This patch adds a new enum for CRTC output formats, and then plugs it
> in the existing modeset framework.
>
> V3: Added this patch in the series, to address review comments from
>     second patchset.
>
> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
Series looks good to me now, so feel free to add my r-b to where its missing. But you still need an ack from Ville I think. :)

Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

~Maarten
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 1/7] drm/i915: Add CRTC output format YCBCR 4:2:0

[Ville Syrjälä]

On Fri, Jan 05, 2018 at 03:15:29PM +0530, Shashank Sharma wrote:
> Currently, we are using a bool in CRTC state (state->ycbcr420),
> to indicate modeset, that the output format is YCBCR 4:2:0. Now in
> order to support other YCBCR formats, we will need more such flags.
> 
> The idea behind this patch is to replace this bool with an enum,
> and plug this in in the existing YCBCR 4:2:0 framework in such a
> way that this can be re-used for YCBCR 4:4:4 and other output formats too.
> 
> This patch adds a new enum for CRTC output formats, and then plugs it
> in the existing modeset framework.
> 
> V3: Added this patch in the series, to address review comments from
>     second patchset.
> 
> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_color.c   |  2 +-
>  drivers/gpu/drm/i915/intel_ddi.c     |  2 +-
>  drivers/gpu/drm/i915/intel_display.c | 66 ++++++++++++++++++++++++------------
>  drivers/gpu/drm/i915/intel_drv.h     | 10 ++++--
>  drivers/gpu/drm/i915/intel_hdmi.c    |  6 ++--
>  drivers/gpu/drm/i915/intel_panel.c   |  2 +-
>  6 files changed, 59 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
> index aa66e95..99e32cb 100644
> --- a/drivers/gpu/drm/i915/intel_color.c
> +++ b/drivers/gpu/drm/i915/intel_color.c
> @@ -141,7 +141,7 @@ static void i9xx_load_csc_matrix(struct drm_crtc_state *crtc_state)
>  	uint16_t coeffs[9] = { 0, };
>  	struct intel_crtc_state *intel_crtc_state = to_intel_crtc_state(crtc_state);
>  
> -	if (intel_crtc_state->ycbcr420) {
> +	if (intel_crtc_state->output_format == CRTC_OUTPUT_YCBCR420) {
>  		i9xx_load_ycbcr_conversion_matrix(intel_crtc);
>  		return;
>  	} else if (crtc_state->ctm) {
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index f51645a..84327e7 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -1264,7 +1264,7 @@ static void ddi_dotclock_get(struct intel_crtc_state *pipe_config)
>  	else
>  		dotclock = pipe_config->port_clock;
>  
> -	if (pipe_config->ycbcr420)
> +	if (pipe_config->output_format == CRTC_OUTPUT_YCBCR420)
>  		dotclock *= 2;
>  
>  	if (pipe_config->pixel_multiplier)
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 0cd3559..69b0aa3 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4644,7 +4644,8 @@ skl_update_scaler(struct intel_crtc_state *crtc_state, bool force_detach,
>  	 */
>  	need_scaling = src_w != dst_w || src_h != dst_h;
>  
> -	if (crtc_state->ycbcr420 && scaler_user == SKL_CRTC_INDEX)
> +	if (crtc_state->output_format == CRTC_OUTPUT_YCBCR420 &&
> +	    scaler_user == SKL_CRTC_INDEX)
>  		need_scaling = true;
>  
>  	/*
> @@ -6356,7 +6357,8 @@ static int intel_crtc_compute_config(struct intel_crtc *crtc,
>  		return -EINVAL;
>  	}
>  
> -	if (pipe_config->ycbcr420 && pipe_config->base.ctm) {
> +	if (pipe_config->output_format == CRTC_OUTPUT_YCBCR420 &&
> +	    pipe_config->base.ctm) {
>  		/*
>  		 * There is only one pipe CSC unit per pipe, and we need that
>  		 * for output conversion from RGB->YCBCR. So if CTM is already
> @@ -8177,10 +8179,10 @@ static void haswell_set_pipemisc(struct drm_crtc *crtc)
>  		if (intel_crtc->config->dither)
>  			val |= PIPEMISC_DITHER_ENABLE | PIPEMISC_DITHER_TYPE_SP;
>  
> -		if (config->ycbcr420) {
> -			val |= PIPEMISC_OUTPUT_COLORSPACE_YUV |
> -				PIPEMISC_YUV420_ENABLE |
> -				PIPEMISC_YUV420_MODE_FULL_BLEND;
> +		if (config->output_format == CRTC_OUTPUT_YCBCR420) {
> +			val |= PIPEMISC_OUTPUT_COLORSPACE_YUV;
> +			val |= PIPEMISC_YUV420_ENABLE |
> +			       PIPEMISC_YUV420_MODE_FULL_BLEND;
>  		}
>  
>  		I915_WRITE(PIPEMISC(intel_crtc->pipe), val);
> @@ -9156,6 +9158,19 @@ static void haswell_get_ddi_port_state(struct intel_crtc *crtc,
>  	}
>  }
>  
> +static const char * const output_format_str[] = {
> +	"Invalid",
> +	"RGB",
> +	"YCBCR4:2:0",
> +};
> +
> +static const char *output_formats(enum crtc_output_format format)
> +{
> +	if (format < CRTC_OUTPUT_INVALID || format > CRTC_OUTPUT_YCBCR420)
> +		format = CRTC_OUTPUT_INVALID;
> +	return output_format_str[format + 1];
> +}
> +
>  static bool haswell_get_pipe_config(struct intel_crtc *crtc,
>  				    struct intel_crtc_state *pipe_config)
>  {
> @@ -9196,19 +9211,28 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
>  
>  	if (IS_BROADWELL(dev_priv) || INTEL_GEN(dev_priv) >= 9) {
>  		u32 tmp = I915_READ(PIPEMISC(crtc->pipe));
> -		bool clrspace_yuv = tmp & PIPEMISC_OUTPUT_COLORSPACE_YUV;
> -
> -		if (IS_GEMINILAKE(dev_priv) || INTEL_GEN(dev_priv) >= 10) {
> -			bool blend_mode_420 = tmp &
> -					      PIPEMISC_YUV420_MODE_FULL_BLEND;
> +		enum crtc_output_format output_format = CRTC_OUTPUT_RGB;
> +
> +		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_format = CRTC_OUTPUT_INVALID;

Not sure that INVALID thing really provides any real value.
Just feels like it's making the code more convoluted for something that
should never really happen.

> +				else if (!(IS_GEMINILAKE(dev_priv) ||
> +					   INTEL_GEN(dev_priv) >= 10))
> +					output_format = CRTC_OUTPUT_INVALID;
> +				else
> +					output_format = CRTC_OUTPUT_YCBCR420;
> +			}
>  
> -			pipe_config->ycbcr420 = tmp & PIPEMISC_YUV420_ENABLE;
> -			if (pipe_config->ycbcr420 != clrspace_yuv ||
> -			    pipe_config->ycbcr420 != blend_mode_420)
> -				DRM_DEBUG_KMS("Bad 4:2:0 mode (%08x)\n", tmp);
> -		} else if (clrspace_yuv) {
> -			DRM_DEBUG_KMS("YCbCr 4:2:0 Unsupported\n");
>  		}
> +
> +		pipe_config->output_format = output_format;
> +		DRM_DEBUG_KMS("Output format %s\n",
> +				output_formats(output_format));
>  	}
>  
>  	power_domain = POWER_DOMAIN_PIPE_PANEL_FITTER(crtc->pipe);
> @@ -10558,6 +10582,9 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc,
>  	DRM_DEBUG_KMS("output_types: %s (0x%x)\n",
>  		      buf, pipe_config->output_types);
>  
> +	DRM_DEBUG_KMS("output format: %s\n",
> +		output_formats(pipe_config->output_format));
> +
>  	DRM_DEBUG_KMS("cpu_transcoder: %s, pipe bpp: %i, dithering: %i\n",
>  		      transcoder_name(pipe_config->cpu_transcoder),
>  		      pipe_config->pipe_bpp, pipe_config->dither);
> @@ -10567,9 +10594,6 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc,
>  				      pipe_config->fdi_lanes,
>  				      &pipe_config->fdi_m_n);
>  
> -	if (pipe_config->ycbcr420)
> -		DRM_DEBUG_KMS("YCbCr 4:2:0 output enabled\n");
> -
>  	if (intel_crtc_has_dp_encoder(pipe_config)) {
>  		intel_dump_m_n_config(pipe_config, "dp m_n",
>  				pipe_config->lane_count, &pipe_config->dp_m_n);
> @@ -11143,6 +11167,7 @@ intel_pipe_config_compare(struct drm_i915_private *dev_priv,
>  	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_vsync_end);
>  
>  	PIPE_CONF_CHECK_I(pixel_multiplier);
> +	PIPE_CONF_CHECK_I(output_format);
>  	PIPE_CONF_CHECK_BOOL(has_hdmi_sink);
>  	if ((INTEL_GEN(dev_priv) < 8 && !IS_HASWELL(dev_priv)) ||
>  	    IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> @@ -11151,7 +11176,6 @@ intel_pipe_config_compare(struct drm_i915_private *dev_priv,
>  	PIPE_CONF_CHECK_BOOL(hdmi_scrambling);
>  	PIPE_CONF_CHECK_BOOL(hdmi_high_tmds_clock_ratio);
>  	PIPE_CONF_CHECK_BOOL_INCOMPLETE(has_infoframe);
> -	PIPE_CONF_CHECK_BOOL(ycbcr420);
>  
>  	PIPE_CONF_CHECK_BOOL_INCOMPLETE(has_audio);
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 30f791f..79662650 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -609,6 +609,12 @@ struct intel_crtc_wm_state {
>  	bool need_postvbl_update;
>  };
>  
> +enum crtc_output_format {

intel_output_format or something would fit the normal namespacing
better.

> +	CRTC_OUTPUT_INVALID = -1,

The -1 doesn't make much sense to me. If we want to keep this IMO
just let the enum start from 0. That will at least make it clear
if you entirely forgot to do readout.

> +	CRTC_OUTPUT_RGB,
> +	CRTC_OUTPUT_YCBCR420,
> +};
> +
>  struct intel_crtc_state {
>  	struct drm_crtc_state base;
>  
> @@ -795,8 +801,8 @@ struct intel_crtc_state {
>  	/* HDMI High TMDS char rate ratio */
>  	bool hdmi_high_tmds_clock_ratio;
>  
> -	/* output format is YCBCR 4:2:0 */
> -	bool ycbcr420;
> +	/* Output format RGB/YCBCR etc */
> +	enum crtc_output_format output_format;
>  };
>  
>  struct intel_crtc {
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index bced7b9..b266a7f 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -478,7 +478,7 @@ static void intel_hdmi_set_avi_infoframe(struct drm_encoder *encoder,
>  		return;
>  	}
>  
> -	if (crtc_state->ycbcr420)
> +	if (crtc_state->output_format == CRTC_OUTPUT_YCBCR420)
>  		frame.avi.colorspace = HDMI_COLORSPACE_YUV420;
>  	else
>  		frame.avi.colorspace = HDMI_COLORSPACE_RGB;
> @@ -1372,7 +1372,7 @@ static bool hdmi_12bpc_possible(const struct intel_crtc_state *crtc_state)
>  		if (connector_state->crtc != crtc_state->base.crtc)
>  			continue;
>  
> -		if (crtc_state->ycbcr420) {
> +		if (crtc_state->output_format == CRTC_OUTPUT_YCBCR420) {
>  			const struct drm_hdmi_info *hdmi = &info->hdmi;
>  
>  			if (!(hdmi->y420_dc_modes & DRM_EDID_YCBCR420_DC_36))
> @@ -1407,7 +1407,7 @@ intel_hdmi_ycbcr420_config(struct drm_connector *connector,
>  	config->port_clock /= 2;
>  	*clock_12bpc /= 2;
>  	*clock_8bpc /= 2;
> -	config->ycbcr420 = true;
> +	config->output_format = CRTC_OUTPUT_YCBCR420;
>  
>  	/* YCBCR 420 output conversion needs a scaler */
>  	if (skl_update_scaler_crtc(config)) {
> diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
> index fa6831f..c57819f 100644
> --- a/drivers/gpu/drm/i915/intel_panel.c
> +++ b/drivers/gpu/drm/i915/intel_panel.c
> @@ -111,7 +111,7 @@ intel_pch_panel_fitting(struct intel_crtc *intel_crtc,
>  	/* Native modes don't need fitting */
>  	if (adjusted_mode->crtc_hdisplay == pipe_config->pipe_src_w &&
>  	    adjusted_mode->crtc_vdisplay == pipe_config->pipe_src_h &&
> -	    !pipe_config->ycbcr420)
> +	    pipe_config->output_format != CRTC_OUTPUT_YCBCR420)
>  		goto done;
>  
>  	switch (fitting_mode) {
> -- 
> 2.7.4

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 2/7] drm/i915: Add CRTC output format YCBCR 4:4:4

[Ville Syrjälä]

On Fri, Jan 05, 2018 at 03:15:30PM +0530, Shashank Sharma wrote:
> This patch adds support for YCBCR 4:4:4 CRTC output format.
> To do this, this patch extends the existing YCBCR 4:2:0
> framework by:
> - Adding new parameter in for YCBCR 4:4:4 enum crtc_iutput_format.
> - Adding case for YCBCR 4:4:4 in while setting AVI infoframes.
> - Adding necessary checks in modeset sequence.
> 
> V3: Added this patch in the series
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_color.c   |  3 ++-
>  drivers/gpu/drm/i915/intel_display.c | 13 +++++++++----
>  drivers/gpu/drm/i915/intel_drv.h     |  1 +
>  drivers/gpu/drm/i915/intel_hdmi.c    |  2 ++
>  4 files changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
> index 99e32cb..5b76de6 100644
> --- a/drivers/gpu/drm/i915/intel_color.c
> +++ b/drivers/gpu/drm/i915/intel_color.c
> @@ -141,7 +141,8 @@ static void i9xx_load_csc_matrix(struct drm_crtc_state *crtc_state)
>  	uint16_t coeffs[9] = { 0, };
>  	struct intel_crtc_state *intel_crtc_state = to_intel_crtc_state(crtc_state);
>  
> -	if (intel_crtc_state->output_format == CRTC_OUTPUT_YCBCR420) {
> +	if (intel_crtc_state->output_format == CRTC_OUTPUT_YCBCR420 ||
> +	    intel_crtc_state->output_format == CRTC_OUTPUT_YCBCR444) {
>  		i9xx_load_ycbcr_conversion_matrix(intel_crtc);
>  		return;
>  	} else if (crtc_state->ctm) {
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 69b0aa3..6ac5ca6 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6357,8 +6357,9 @@ static int intel_crtc_compute_config(struct intel_crtc *crtc,
>  		return -EINVAL;
>  	}
>  
> -	if (pipe_config->output_format == CRTC_OUTPUT_YCBCR420 &&
> -	    pipe_config->base.ctm) {
> +	if ((pipe_config->output_format == CRTC_OUTPUT_YCBCR420 ||
> +	     pipe_config->output_format == CRTC_OUTPUT_YCBCR444) &&
> +	     pipe_config->base.ctm) {
>  		/*
>  		 * There is only one pipe CSC unit per pipe, and we need that
>  		 * for output conversion from RGB->YCBCR. So if CTM is already
> @@ -8179,11 +8180,12 @@ static void haswell_set_pipemisc(struct drm_crtc *crtc)
>  		if (intel_crtc->config->dither)
>  			val |= PIPEMISC_DITHER_ENABLE | PIPEMISC_DITHER_TYPE_SP;
>  
> -		if (config->output_format == CRTC_OUTPUT_YCBCR420) {
> +		if (config->output_format > CRTC_OUTPUT_RGB)

The '>' looks weird. For best clarity I would just use
'x == YCBCR444 || x == YCBCR420'

>  			val |= PIPEMISC_OUTPUT_COLORSPACE_YUV;
> +
> +		if (config->output_format == CRTC_OUTPUT_YCBCR420)
>  			val |= PIPEMISC_YUV420_ENABLE |
>  			       PIPEMISC_YUV420_MODE_FULL_BLEND;
> -		}
>  
>  		I915_WRITE(PIPEMISC(intel_crtc->pipe), val);
>  	}
> @@ -9161,6 +9163,7 @@ static void haswell_get_ddi_port_state(struct intel_crtc *crtc,
>  static const char * const output_format_str[] = {
>  	"Invalid",
>  	"RGB",
> +	"YCBCR4:4:4",
>  	"YCBCR4:2:0",
>  };
>  
> @@ -9226,6 +9229,8 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
>  					output_format = CRTC_OUTPUT_INVALID;
>  				else
>  					output_format = CRTC_OUTPUT_YCBCR420;
> +			} else {
> +				output_format = CRTC_OUTPUT_YCBCR444;
>  			}
>  
>  		}
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 79662650..a393342 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -612,6 +612,7 @@ struct intel_crtc_wm_state {
>  enum crtc_output_format {
>  	CRTC_OUTPUT_INVALID = -1,
>  	CRTC_OUTPUT_RGB,
> +	CRTC_OUTPUT_YCBCR444,
>  	CRTC_OUTPUT_YCBCR420,
>  };
>  
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index b266a7f..258bb51 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -480,6 +480,8 @@ static void intel_hdmi_set_avi_infoframe(struct drm_encoder *encoder,
>  
>  	if (crtc_state->output_format == CRTC_OUTPUT_YCBCR420)
>  		frame.avi.colorspace = HDMI_COLORSPACE_YUV420;
> +	else if (crtc_state->output_format == CRTC_OUTPUT_YCBCR444)
> +		frame.avi.colorspace = HDMI_COLORSPACE_YUV444;
>  	else
>  		frame.avi.colorspace = HDMI_COLORSPACE_RGB;
>  
> -- 
> 2.7.4

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 7/7] drm/i915: Add YCBCR 4:2:0/4:4:4 support for LSPCON

[Ville Syrjälä]

On Fri, Jan 05, 2018 at 03:15:35PM +0530, Shashank Sharma wrote:
> LSPCON chips can generate YCBCR outputs, if asked nicely :).
> 
> In order to generate YCBCR 4:2:0 outputs, a source must:
> - send YCBCR 4:4:4 signals to LSPCON
> - program color space as 4:2:0 in AVI infoframes
> 
> Whereas for YCBCR 4:4:4 outputs, the source must:
> - send YCBCR 4:4:4 signals to LSPCON
> - program color space as 4:4:4 in AVI infoframes
> 
> So for both 4:2:0 as well as 4:4:4 outputs, we are driving the
> pipe for YCBCR 4:4:4 output, but AVI infoframe's color space
> information indicates LSPCON FW to start scaling down from YCBCR
> 4:4:4 and generate YCBCR 4:2:0 output. As the scaling is done by
> LSPCON device, we need not to reserve a scaler for 4:2:0 outputs.
> 
> V2: rebase
> V3: Addressed review comments from Ville
>     - add enum crtc_output_format instead of bool ycbcr420
>     - use crtc_output_format=4:4:4 for modeset of LSPCON 4:2:0 output
>       cases in this way we will have YCBCR 4:4:4 framework ready (except
>       the ABI part)
> 
> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h     |  2 ++
>  drivers/gpu/drm/i915/intel_ddi.c    |  7 +++++++
>  drivers/gpu/drm/i915/intel_dp.c     | 10 ++++++++++
>  drivers/gpu/drm/i915/intel_drv.h    |  2 ++
>  drivers/gpu/drm/i915/intel_lspcon.c | 28 ++++++++++++++++++++++++++++
>  5 files changed, 49 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 966e4df..45ee264 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -8547,6 +8547,8 @@ enum skl_power_gate {
>  #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		(2<<3)
>  #define  TRANS_MSA_6_BPC		(0<<5)
>  #define  TRANS_MSA_8_BPC		(1<<5)
>  #define  TRANS_MSA_10_BPC		(2<<5)
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 7b89f2a..7616f6f 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -1499,6 +1499,13 @@ void intel_ddi_set_pipe_settings(const struct intel_crtc_state *crtc_state)
>  		break;
>  	}
>  
> +	/*
> +	 * As per DP 1.2 spec section 2.3.4.3 while sending
> +	 * YCBCR 444 signals we should program MSA MISC1/0 fields with
> +	 * colorspace information.
> +	 */
> +	if (crtc_state->output_format == CRTC_OUTPUT_YCBCR444)
> +		temp |= TRANS_MSA_SAMPLING_444 | TRANS_MSA_CLRSP_YCBCR;

This fails to state that we're indicating BT.601 encoding. I think we
should spell that out explicitly.

>  	I915_WRITE(TRANS_MSA_MISC(cpu_transcoder), temp);
>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 35c5299..3bf82ea 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1613,6 +1613,7 @@ intel_dp_compute_config(struct intel_encoder *encoder,
>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>  	struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode;
>  	struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
> +	struct intel_lspcon *lspcon = enc_to_intel_lspcon(&encoder->base);
>  	enum port port = encoder->port;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(pipe_config->base.crtc);
>  	struct intel_connector *intel_connector = intel_dp->attached_connector;
> @@ -1642,6 +1643,15 @@ intel_dp_compute_config(struct intel_encoder *encoder,
>  	if (HAS_PCH_SPLIT(dev_priv) && !HAS_DDI(dev_priv) && port != PORT_A)
>  		pipe_config->has_pch_encoder = true;
>  
> +	if (lspcon->active) {
> +		struct drm_connector *connector = &intel_connector->base;
> +
> +		if (lspcon_ycbcr420_config(connector, pipe_config)) {
> +			pipe_config->output_format = CRTC_OUTPUT_YCBCR444;

I think I'd like to see all compute_config hooks explicitly set the
outout_format (to RGB usually obviously).

> +			lspcon->output_format = CRTC_OUTPUT_YCBCR420;

You should not modify any non-atomic state like that in compute_config.

> +		}
> +	}
> +
>  	pipe_config->has_drrs = false;
>  	if (IS_G4X(dev_priv) || port == PORT_A)
>  		pipe_config->has_audio = false;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 2de6b41..5edba06 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -2047,6 +2047,8 @@ void lspcon_set_infoframes(struct drm_encoder *encoder,
>  			   const struct drm_connector_state *conn_state);
>  bool lspcon_infoframe_enabled(struct drm_encoder *encoder,
>  			      const struct intel_crtc_state *pipe_config);
> +bool lspcon_ycbcr420_config(struct drm_connector *connector,
> +			     struct intel_crtc_state *config);
>  
>  /* intel_pipe_crc.c */
>  int intel_pipe_crc_create(struct drm_minor *minor);
> diff --git a/drivers/gpu/drm/i915/intel_lspcon.c b/drivers/gpu/drm/i915/intel_lspcon.c
> index 066ea91..cb88138 100644
> --- a/drivers/gpu/drm/i915/intel_lspcon.c
> +++ b/drivers/gpu/drm/i915/intel_lspcon.c
> @@ -180,6 +180,25 @@ static bool lspcon_wake_native_aux_ch(struct intel_lspcon *lspcon)
>  	return true;
>  }
>  
> +bool lspcon_ycbcr420_config(struct drm_connector *connector,
> +			     struct intel_crtc_state *config)
> +{
> +	struct drm_display_info *info = &connector->display_info;
> +	struct drm_display_mode *mode = &config->base.adjusted_mode;
> +
> +	if (drm_mode_is_420_only(info, mode)) {
> +		if (!connector->ycbcr_420_allowed) {
> +			DRM_ERROR("Platform doesn't support YCBCR420 output\n");
> +			return false;
> +		}
> +
> +		config->port_clock /= 2;
> +		return true;
> +	}
> +
> +	return false;
> +}
> +
>  static bool lspcon_probe(struct intel_lspcon *lspcon)
>  {
>  	int retry;
> @@ -459,6 +478,13 @@ void lspcon_set_infoframes(struct drm_encoder *encoder,
>  		return;
>  	}
>  
> +	if (lspcon->output_format == CRTC_OUTPUT_YCBCR420)
> +		frame.avi.colorspace = HDMI_COLORSPACE_YUV420;
> +	else if (lspcon->output_format == CRTC_OUTPUT_YCBCR444)
> +		frame.avi.colorspace = HDMI_COLORSPACE_YUV444;
> +	else
> +		frame.avi.colorspace = HDMI_COLORSPACE_RGB;
> +
>  	drm_hdmi_avi_infoframe_quant_range(&frame.avi, mode,
>  					   crtc_state->limited_color_range ?
>  					   HDMI_QUANTIZATION_RANGE_LIMITED :
> @@ -512,6 +538,7 @@ bool lspcon_init(struct intel_digital_port *intel_dig_port)
>  	struct intel_lspcon *lspcon = &intel_dig_port->lspcon;
>  	struct drm_device *dev = intel_dig_port->base.base.dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
> +	struct drm_connector *connector = &dp->attached_connector->base;
>  
>  	if (!HAS_LSPCON(dev_priv)) {
>  		DRM_ERROR("LSPCON is not supported on this platform\n");
> @@ -536,6 +563,7 @@ bool lspcon_init(struct intel_digital_port *intel_dig_port)
>  		return false;
>  	}
>  
> +	connector->ycbcr_420_allowed = true;
>  	lspcon->active = true;
>  	DRM_DEBUG_KMS("Success: LSPCON init\n");
>  	return true;
> -- 
> 2.7.4

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 1/7] drm/i915: Add CRTC output format YCBCR 4:2:0

[Sharma, Shashank]

Regards

Shashank


On 1/17/2018 11:57 PM, Ville Syrjälä wrote:
> On Fri, Jan 05, 2018 at 03:15:29PM +0530, Shashank Sharma wrote:
>> Currently, we are using a bool in CRTC state (state->ycbcr420),
>> to indicate modeset, that the output format is YCBCR 4:2:0. Now in
>> order to support other YCBCR formats, we will need more such flags.
>>
>> The idea behind this patch is to replace this bool with an enum,
>> and plug this in in the existing YCBCR 4:2:0 framework in such a
>> way that this can be re-used for YCBCR 4:4:4 and other output formats too.
>>
>> This patch adds a new enum for CRTC output formats, and then plugs it
>> in the existing modeset framework.
>>
>> V3: Added this patch in the series, to address review comments from
>>      second patchset.
>>
>> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_color.c   |  2 +-
>>   drivers/gpu/drm/i915/intel_ddi.c     |  2 +-
>>   drivers/gpu/drm/i915/intel_display.c | 66 ++++++++++++++++++++++++------------
>>   drivers/gpu/drm/i915/intel_drv.h     | 10 ++++--
>>   drivers/gpu/drm/i915/intel_hdmi.c    |  6 ++--
>>   drivers/gpu/drm/i915/intel_panel.c   |  2 +-
>>   6 files changed, 59 insertions(+), 29 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
>> index aa66e95..99e32cb 100644
>> --- a/drivers/gpu/drm/i915/intel_color.c
>> +++ b/drivers/gpu/drm/i915/intel_color.c
>> @@ -141,7 +141,7 @@ static void i9xx_load_csc_matrix(struct drm_crtc_state *crtc_state)
>>   	uint16_t coeffs[9] = { 0, };
>>   	struct intel_crtc_state *intel_crtc_state = to_intel_crtc_state(crtc_state);
>>   
>> -	if (intel_crtc_state->ycbcr420) {
>> +	if (intel_crtc_state->output_format == CRTC_OUTPUT_YCBCR420) {
>>   		i9xx_load_ycbcr_conversion_matrix(intel_crtc);
>>   		return;
>>   	} else if (crtc_state->ctm) {
>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
>> index f51645a..84327e7 100644
>> --- a/drivers/gpu/drm/i915/intel_ddi.c
>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
>> @@ -1264,7 +1264,7 @@ static void ddi_dotclock_get(struct intel_crtc_state *pipe_config)
>>   	else
>>   		dotclock = pipe_config->port_clock;
>>   
>> -	if (pipe_config->ycbcr420)
>> +	if (pipe_config->output_format == CRTC_OUTPUT_YCBCR420)
>>   		dotclock *= 2;
>>   
>>   	if (pipe_config->pixel_multiplier)
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 0cd3559..69b0aa3 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -4644,7 +4644,8 @@ skl_update_scaler(struct intel_crtc_state *crtc_state, bool force_detach,
>>   	 */
>>   	need_scaling = src_w != dst_w || src_h != dst_h;
>>   
>> -	if (crtc_state->ycbcr420 && scaler_user == SKL_CRTC_INDEX)
>> +	if (crtc_state->output_format == CRTC_OUTPUT_YCBCR420 &&
>> +	    scaler_user == SKL_CRTC_INDEX)
>>   		need_scaling = true;
>>   
>>   	/*
>> @@ -6356,7 +6357,8 @@ static int intel_crtc_compute_config(struct intel_crtc *crtc,
>>   		return -EINVAL;
>>   	}
>>   
>> -	if (pipe_config->ycbcr420 && pipe_config->base.ctm) {
>> +	if (pipe_config->output_format == CRTC_OUTPUT_YCBCR420 &&
>> +	    pipe_config->base.ctm) {
>>   		/*
>>   		 * There is only one pipe CSC unit per pipe, and we need that
>>   		 * for output conversion from RGB->YCBCR. So if CTM is already
>> @@ -8177,10 +8179,10 @@ static void haswell_set_pipemisc(struct drm_crtc *crtc)
>>   		if (intel_crtc->config->dither)
>>   			val |= PIPEMISC_DITHER_ENABLE | PIPEMISC_DITHER_TYPE_SP;
>>   
>> -		if (config->ycbcr420) {
>> -			val |= PIPEMISC_OUTPUT_COLORSPACE_YUV |
>> -				PIPEMISC_YUV420_ENABLE |
>> -				PIPEMISC_YUV420_MODE_FULL_BLEND;
>> +		if (config->output_format == CRTC_OUTPUT_YCBCR420) {
>> +			val |= PIPEMISC_OUTPUT_COLORSPACE_YUV;
>> +			val |= PIPEMISC_YUV420_ENABLE |
>> +			       PIPEMISC_YUV420_MODE_FULL_BLEND;
>>   		}
>>   
>>   		I915_WRITE(PIPEMISC(intel_crtc->pipe), val);
>> @@ -9156,6 +9158,19 @@ static void haswell_get_ddi_port_state(struct intel_crtc *crtc,
>>   	}
>>   }
>>   
>> +static const char * const output_format_str[] = {
>> +	"Invalid",
>> +	"RGB",
>> +	"YCBCR4:2:0",
>> +};
>> +
>> +static const char *output_formats(enum crtc_output_format format)
>> +{
>> +	if (format < CRTC_OUTPUT_INVALID || format > CRTC_OUTPUT_YCBCR420)
>> +		format = CRTC_OUTPUT_INVALID;
>> +	return output_format_str[format + 1];
>> +}
>> +
>>   static bool haswell_get_pipe_config(struct intel_crtc *crtc,
>>   				    struct intel_crtc_state *pipe_config)
>>   {
>> @@ -9196,19 +9211,28 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
>>   
>>   	if (IS_BROADWELL(dev_priv) || INTEL_GEN(dev_priv) >= 9) {
>>   		u32 tmp = I915_READ(PIPEMISC(crtc->pipe));
>> -		bool clrspace_yuv = tmp & PIPEMISC_OUTPUT_COLORSPACE_YUV;
>> -
>> -		if (IS_GEMINILAKE(dev_priv) || INTEL_GEN(dev_priv) >= 10) {
>> -			bool blend_mode_420 = tmp &
>> -					      PIPEMISC_YUV420_MODE_FULL_BLEND;
>> +		enum crtc_output_format output_format = CRTC_OUTPUT_RGB;
>> +
>> +		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_format = CRTC_OUTPUT_INVALID;
> Not sure that INVALID thing really provides any real value.
> Just feels like it's making the code more convoluted for something that
> should never really happen.
The check is directly adapted from the previous version of the code, 
where we were still checking if
the registers are programmed properly, CRTC_OUTPUT_INVALID helps to 
print the error messages and
fits into the existing framework, so I thought there is no harm in 
keeping it.
>> +				else if (!(IS_GEMINILAKE(dev_priv) ||
>> +					   INTEL_GEN(dev_priv) >= 10))
>> +					output_format = CRTC_OUTPUT_INVALID;
>> +				else
>> +					output_format = CRTC_OUTPUT_YCBCR420;
>> +			}
>>   
>> -			pipe_config->ycbcr420 = tmp & PIPEMISC_YUV420_ENABLE;
>> -			if (pipe_config->ycbcr420 != clrspace_yuv ||
>> -			    pipe_config->ycbcr420 != blend_mode_420)
>> -				DRM_DEBUG_KMS("Bad 4:2:0 mode (%08x)\n", tmp);
>> -		} else if (clrspace_yuv) {
>> -			DRM_DEBUG_KMS("YCbCr 4:2:0 Unsupported\n");
>>   		}
>> +
>> +		pipe_config->output_format = output_format;
>> +		DRM_DEBUG_KMS("Output format %s\n",
>> +				output_formats(output_format));
>>   	}
>>   
>>   	power_domain = POWER_DOMAIN_PIPE_PANEL_FITTER(crtc->pipe);
>> @@ -10558,6 +10582,9 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc,
>>   	DRM_DEBUG_KMS("output_types: %s (0x%x)\n",
>>   		      buf, pipe_config->output_types);
>>   
>> +	DRM_DEBUG_KMS("output format: %s\n",
>> +		output_formats(pipe_config->output_format));
>> +
>>   	DRM_DEBUG_KMS("cpu_transcoder: %s, pipe bpp: %i, dithering: %i\n",
>>   		      transcoder_name(pipe_config->cpu_transcoder),
>>   		      pipe_config->pipe_bpp, pipe_config->dither);
>> @@ -10567,9 +10594,6 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc,
>>   				      pipe_config->fdi_lanes,
>>   				      &pipe_config->fdi_m_n);
>>   
>> -	if (pipe_config->ycbcr420)
>> -		DRM_DEBUG_KMS("YCbCr 4:2:0 output enabled\n");
>> -
>>   	if (intel_crtc_has_dp_encoder(pipe_config)) {
>>   		intel_dump_m_n_config(pipe_config, "dp m_n",
>>   				pipe_config->lane_count, &pipe_config->dp_m_n);
>> @@ -11143,6 +11167,7 @@ intel_pipe_config_compare(struct drm_i915_private *dev_priv,
>>   	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_vsync_end);
>>   
>>   	PIPE_CONF_CHECK_I(pixel_multiplier);
>> +	PIPE_CONF_CHECK_I(output_format);
>>   	PIPE_CONF_CHECK_BOOL(has_hdmi_sink);
>>   	if ((INTEL_GEN(dev_priv) < 8 && !IS_HASWELL(dev_priv)) ||
>>   	    IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
>> @@ -11151,7 +11176,6 @@ intel_pipe_config_compare(struct drm_i915_private *dev_priv,
>>   	PIPE_CONF_CHECK_BOOL(hdmi_scrambling);
>>   	PIPE_CONF_CHECK_BOOL(hdmi_high_tmds_clock_ratio);
>>   	PIPE_CONF_CHECK_BOOL_INCOMPLETE(has_infoframe);
>> -	PIPE_CONF_CHECK_BOOL(ycbcr420);
>>   
>>   	PIPE_CONF_CHECK_BOOL_INCOMPLETE(has_audio);
>>   
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index 30f791f..79662650 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -609,6 +609,12 @@ struct intel_crtc_wm_state {
>>   	bool need_postvbl_update;
>>   };
>>   
>> +enum crtc_output_format {
> intel_output_format or something would fit the normal namespacing
> better.
Agree, will change it accordingly.
>
>> +	CRTC_OUTPUT_INVALID = -1,
> The -1 doesn't make much sense to me. If we want to keep this IMO
> just let the enum start from 0. That will at least make it clear
> if you entirely forgot to do readout.
The reason why I kept INVALID as -1, is to make sure RGB starts at 0. So 
that we need not to
explicitly set the CRTC_OUTPUT_FORMAT = RGB (which is also one of your 
comment in the
series). This will allow least modifications in the existing code.

May be I can keep it in the end of the enum, but that would be moving as 
and when we add
new formats. What do you suggest ?

- Shashank
>> +	CRTC_OUTPUT_RGB,
>> +	CRTC_OUTPUT_YCBCR420,
>> +};
>> +
>>   struct intel_crtc_state {
>>   	struct drm_crtc_state base;
>>   
>> @@ -795,8 +801,8 @@ struct intel_crtc_state {
>>   	/* HDMI High TMDS char rate ratio */
>>   	bool hdmi_high_tmds_clock_ratio;
>>   
>> -	/* output format is YCBCR 4:2:0 */
>> -	bool ycbcr420;
>> +	/* Output format RGB/YCBCR etc */
>> +	enum crtc_output_format output_format;
>>   };
>>   
>>   struct intel_crtc {
>> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
>> index bced7b9..b266a7f 100644
>> --- a/drivers/gpu/drm/i915/intel_hdmi.c
>> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
>> @@ -478,7 +478,7 @@ static void intel_hdmi_set_avi_infoframe(struct drm_encoder *encoder,
>>   		return;
>>   	}
>>   
>> -	if (crtc_state->ycbcr420)
>> +	if (crtc_state->output_format == CRTC_OUTPUT_YCBCR420)
>>   		frame.avi.colorspace = HDMI_COLORSPACE_YUV420;
>>   	else
>>   		frame.avi.colorspace = HDMI_COLORSPACE_RGB;
>> @@ -1372,7 +1372,7 @@ static bool hdmi_12bpc_possible(const struct intel_crtc_state *crtc_state)
>>   		if (connector_state->crtc != crtc_state->base.crtc)
>>   			continue;
>>   
>> -		if (crtc_state->ycbcr420) {
>> +		if (crtc_state->output_format == CRTC_OUTPUT_YCBCR420) {
>>   			const struct drm_hdmi_info *hdmi = &info->hdmi;
>>   
>>   			if (!(hdmi->y420_dc_modes & DRM_EDID_YCBCR420_DC_36))
>> @@ -1407,7 +1407,7 @@ intel_hdmi_ycbcr420_config(struct drm_connector *connector,
>>   	config->port_clock /= 2;
>>   	*clock_12bpc /= 2;
>>   	*clock_8bpc /= 2;
>> -	config->ycbcr420 = true;
>> +	config->output_format = CRTC_OUTPUT_YCBCR420;
>>   
>>   	/* YCBCR 420 output conversion needs a scaler */
>>   	if (skl_update_scaler_crtc(config)) {
>> diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
>> index fa6831f..c57819f 100644
>> --- a/drivers/gpu/drm/i915/intel_panel.c
>> +++ b/drivers/gpu/drm/i915/intel_panel.c
>> @@ -111,7 +111,7 @@ intel_pch_panel_fitting(struct intel_crtc *intel_crtc,
>>   	/* Native modes don't need fitting */
>>   	if (adjusted_mode->crtc_hdisplay == pipe_config->pipe_src_w &&
>>   	    adjusted_mode->crtc_vdisplay == pipe_config->pipe_src_h &&
>> -	    !pipe_config->ycbcr420)
>> +	    pipe_config->output_format != CRTC_OUTPUT_YCBCR420)
>>   		goto done;
>>   
>>   	switch (fitting_mode) {
>> -- 
>> 2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 2/7] drm/i915: Add CRTC output format YCBCR 4:4:4

[Sharma, Shashank]

Regards

Shashank


On 1/17/2018 11:57 PM, Ville Syrjälä wrote:
> On Fri, Jan 05, 2018 at 03:15:30PM +0530, Shashank Sharma wrote:
>> This patch adds support for YCBCR 4:4:4 CRTC output format.
>> To do this, this patch extends the existing YCBCR 4:2:0
>> framework by:
>> - Adding new parameter in for YCBCR 4:4:4 enum crtc_iutput_format.
>> - Adding case for YCBCR 4:4:4 in while setting AVI infoframes.
>> - Adding necessary checks in modeset sequence.
>>
>> V3: Added this patch in the series
>>
>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_color.c   |  3 ++-
>>   drivers/gpu/drm/i915/intel_display.c | 13 +++++++++----
>>   drivers/gpu/drm/i915/intel_drv.h     |  1 +
>>   drivers/gpu/drm/i915/intel_hdmi.c    |  2 ++
>>   4 files changed, 14 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
>> index 99e32cb..5b76de6 100644
>> --- a/drivers/gpu/drm/i915/intel_color.c
>> +++ b/drivers/gpu/drm/i915/intel_color.c
>> @@ -141,7 +141,8 @@ static void i9xx_load_csc_matrix(struct drm_crtc_state *crtc_state)
>>   	uint16_t coeffs[9] = { 0, };
>>   	struct intel_crtc_state *intel_crtc_state = to_intel_crtc_state(crtc_state);
>>   
>> -	if (intel_crtc_state->output_format == CRTC_OUTPUT_YCBCR420) {
>> +	if (intel_crtc_state->output_format == CRTC_OUTPUT_YCBCR420 ||
>> +	    intel_crtc_state->output_format == CRTC_OUTPUT_YCBCR444) {
>>   		i9xx_load_ycbcr_conversion_matrix(intel_crtc);
>>   		return;
>>   	} else if (crtc_state->ctm) {
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 69b0aa3..6ac5ca6 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -6357,8 +6357,9 @@ static int intel_crtc_compute_config(struct intel_crtc *crtc,
>>   		return -EINVAL;
>>   	}
>>   
>> -	if (pipe_config->output_format == CRTC_OUTPUT_YCBCR420 &&
>> -	    pipe_config->base.ctm) {
>> +	if ((pipe_config->output_format == CRTC_OUTPUT_YCBCR420 ||
>> +	     pipe_config->output_format == CRTC_OUTPUT_YCBCR444) &&
>> +	     pipe_config->base.ctm) {
>>   		/*
>>   		 * There is only one pipe CSC unit per pipe, and we need that
>>   		 * for output conversion from RGB->YCBCR. So if CTM is already
>> @@ -8179,11 +8180,12 @@ static void haswell_set_pipemisc(struct drm_crtc *crtc)
>>   		if (intel_crtc->config->dither)
>>   			val |= PIPEMISC_DITHER_ENABLE | PIPEMISC_DITHER_TYPE_SP;
>>   
>> -		if (config->output_format == CRTC_OUTPUT_YCBCR420) {
>> +		if (config->output_format > CRTC_OUTPUT_RGB)
> The '>' looks weird. For best clarity I would just use
> 'x == YCBCR444 || x == YCBCR420'
I knew I would get this comment, still gave a try :-), was just trying 
to reduce one if condition, and dint want
to add (x == YCBCR444 || x == YCBCR420) to all the places. Will do the 
changes.

- Shashank
>>   			val |= PIPEMISC_OUTPUT_COLORSPACE_YUV;
>> +
>> +		if (config->output_format == CRTC_OUTPUT_YCBCR420)
>>   			val |= PIPEMISC_YUV420_ENABLE |
>>   			       PIPEMISC_YUV420_MODE_FULL_BLEND;
>> -		}
>>   
>>   		I915_WRITE(PIPEMISC(intel_crtc->pipe), val);
>>   	}
>> @@ -9161,6 +9163,7 @@ static void haswell_get_ddi_port_state(struct intel_crtc *crtc,
>>   static const char * const output_format_str[] = {
>>   	"Invalid",
>>   	"RGB",
>> +	"YCBCR4:4:4",
>>   	"YCBCR4:2:0",
>>   };
>>   
>> @@ -9226,6 +9229,8 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
>>   					output_format = CRTC_OUTPUT_INVALID;
>>   				else
>>   					output_format = CRTC_OUTPUT_YCBCR420;
>> +			} else {
>> +				output_format = CRTC_OUTPUT_YCBCR444;
>>   			}
>>   
>>   		}
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index 79662650..a393342 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -612,6 +612,7 @@ struct intel_crtc_wm_state {
>>   enum crtc_output_format {
>>   	CRTC_OUTPUT_INVALID = -1,
>>   	CRTC_OUTPUT_RGB,
>> +	CRTC_OUTPUT_YCBCR444,
>>   	CRTC_OUTPUT_YCBCR420,
>>   };
>>   
>> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
>> index b266a7f..258bb51 100644
>> --- a/drivers/gpu/drm/i915/intel_hdmi.c
>> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
>> @@ -480,6 +480,8 @@ static void intel_hdmi_set_avi_infoframe(struct drm_encoder *encoder,
>>   
>>   	if (crtc_state->output_format == CRTC_OUTPUT_YCBCR420)
>>   		frame.avi.colorspace = HDMI_COLORSPACE_YUV420;
>> +	else if (crtc_state->output_format == CRTC_OUTPUT_YCBCR444)
>> +		frame.avi.colorspace = HDMI_COLORSPACE_YUV444;
>>   	else
>>   		frame.avi.colorspace = HDMI_COLORSPACE_RGB;
>>   
>> -- 
>> 2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 7/7] drm/i915: Add YCBCR 4:2:0/4:4:4 support for LSPCON

[Sharma, Shashank]

Regards

Shashank


On 1/17/2018 11:57 PM, Ville Syrjälä wrote:
> On Fri, Jan 05, 2018 at 03:15:35PM +0530, Shashank Sharma wrote:
>> LSPCON chips can generate YCBCR outputs, if asked nicely :).
>>
>> In order to generate YCBCR 4:2:0 outputs, a source must:
>> - send YCBCR 4:4:4 signals to LSPCON
>> - program color space as 4:2:0 in AVI infoframes
>>
>> Whereas for YCBCR 4:4:4 outputs, the source must:
>> - send YCBCR 4:4:4 signals to LSPCON
>> - program color space as 4:4:4 in AVI infoframes
>>
>> So for both 4:2:0 as well as 4:4:4 outputs, we are driving the
>> pipe for YCBCR 4:4:4 output, but AVI infoframe's color space
>> information indicates LSPCON FW to start scaling down from YCBCR
>> 4:4:4 and generate YCBCR 4:2:0 output. As the scaling is done by
>> LSPCON device, we need not to reserve a scaler for 4:2:0 outputs.
>>
>> V2: rebase
>> V3: Addressed review comments from Ville
>>      - add enum crtc_output_format instead of bool ycbcr420
>>      - use crtc_output_format=4:4:4 for modeset of LSPCON 4:2:0 output
>>        cases in this way we will have YCBCR 4:4:4 framework ready (except
>>        the ABI part)
>>
>> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_reg.h     |  2 ++
>>   drivers/gpu/drm/i915/intel_ddi.c    |  7 +++++++
>>   drivers/gpu/drm/i915/intel_dp.c     | 10 ++++++++++
>>   drivers/gpu/drm/i915/intel_drv.h    |  2 ++
>>   drivers/gpu/drm/i915/intel_lspcon.c | 28 ++++++++++++++++++++++++++++
>>   5 files changed, 49 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> index 966e4df..45ee264 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -8547,6 +8547,8 @@ enum skl_power_gate {
>>   #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		(2<<3)
>>   #define  TRANS_MSA_6_BPC		(0<<5)
>>   #define  TRANS_MSA_8_BPC		(1<<5)
>>   #define  TRANS_MSA_10_BPC		(2<<5)
>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
>> index 7b89f2a..7616f6f 100644
>> --- a/drivers/gpu/drm/i915/intel_ddi.c
>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
>> @@ -1499,6 +1499,13 @@ void intel_ddi_set_pipe_settings(const struct intel_crtc_state *crtc_state)
>>   		break;
>>   	}
>>   
>> +	/*
>> +	 * As per DP 1.2 spec section 2.3.4.3 while sending
>> +	 * YCBCR 444 signals we should program MSA MISC1/0 fields with
>> +	 * colorspace information.
>> +	 */
>> +	if (crtc_state->output_format == CRTC_OUTPUT_YCBCR444)
>> +		temp |= TRANS_MSA_SAMPLING_444 | TRANS_MSA_CLRSP_YCBCR;
> This fails to state that we're indicating BT.601 encoding. I think we
> should spell that out explicitly.
Agree, I will add this in comments.
>>   	I915_WRITE(TRANS_MSA_MISC(cpu_transcoder), temp);
>>   }
>>   
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index 35c5299..3bf82ea 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -1613,6 +1613,7 @@ intel_dp_compute_config(struct intel_encoder *encoder,
>>   	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>>   	struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode;
>>   	struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
>> +	struct intel_lspcon *lspcon = enc_to_intel_lspcon(&encoder->base);
>>   	enum port port = encoder->port;
>>   	struct intel_crtc *intel_crtc = to_intel_crtc(pipe_config->base.crtc);
>>   	struct intel_connector *intel_connector = intel_dp->attached_connector;
>> @@ -1642,6 +1643,15 @@ intel_dp_compute_config(struct intel_encoder *encoder,
>>   	if (HAS_PCH_SPLIT(dev_priv) && !HAS_DDI(dev_priv) && port != PORT_A)
>>   		pipe_config->has_pch_encoder = true;
>>   
>> +	if (lspcon->active) {
>> +		struct drm_connector *connector = &intel_connector->base;
>> +
>> +		if (lspcon_ycbcr420_config(connector, pipe_config)) {
>> +			pipe_config->output_format = CRTC_OUTPUT_YCBCR444;
> I think I'd like to see all compute_config hooks explicitly set the
> outout_format (to RGB usually obviously).
That's the one I was talking about in previous patch. If we keep 
output_format_RGB = 0, we need
not to do this, as reset of the pipe_config will automatically make it RGB.
>> +			lspcon->output_format = CRTC_OUTPUT_YCBCR420;
> You should not modify any non-atomic state like that in compute_config.
Please help me to understand this better, Can you elaborate a bit more on:
- Why is this a non-atomic state ?
- What is the right place we should modify it ?

- Shashank
>> +		}
>> +	}
>> +
>>   	pipe_config->has_drrs = false;
>>   	if (IS_G4X(dev_priv) || port == PORT_A)
>>   		pipe_config->has_audio = false;
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index 2de6b41..5edba06 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -2047,6 +2047,8 @@ void lspcon_set_infoframes(struct drm_encoder *encoder,
>>   			   const struct drm_connector_state *conn_state);
>>   bool lspcon_infoframe_enabled(struct drm_encoder *encoder,
>>   			      const struct intel_crtc_state *pipe_config);
>> +bool lspcon_ycbcr420_config(struct drm_connector *connector,
>> +			     struct intel_crtc_state *config);
>>   
>>   /* intel_pipe_crc.c */
>>   int intel_pipe_crc_create(struct drm_minor *minor);
>> diff --git a/drivers/gpu/drm/i915/intel_lspcon.c b/drivers/gpu/drm/i915/intel_lspcon.c
>> index 066ea91..cb88138 100644
>> --- a/drivers/gpu/drm/i915/intel_lspcon.c
>> +++ b/drivers/gpu/drm/i915/intel_lspcon.c
>> @@ -180,6 +180,25 @@ static bool lspcon_wake_native_aux_ch(struct intel_lspcon *lspcon)
>>   	return true;
>>   }
>>   
>> +bool lspcon_ycbcr420_config(struct drm_connector *connector,
>> +			     struct intel_crtc_state *config)
>> +{
>> +	struct drm_display_info *info = &connector->display_info;
>> +	struct drm_display_mode *mode = &config->base.adjusted_mode;
>> +
>> +	if (drm_mode_is_420_only(info, mode)) {
>> +		if (!connector->ycbcr_420_allowed) {
>> +			DRM_ERROR("Platform doesn't support YCBCR420 output\n");
>> +			return false;
>> +		}
>> +
>> +		config->port_clock /= 2;
>> +		return true;
>> +	}
>> +
>> +	return false;
>> +}
>> +
>>   static bool lspcon_probe(struct intel_lspcon *lspcon)
>>   {
>>   	int retry;
>> @@ -459,6 +478,13 @@ void lspcon_set_infoframes(struct drm_encoder *encoder,
>>   		return;
>>   	}
>>   
>> +	if (lspcon->output_format == CRTC_OUTPUT_YCBCR420)
>> +		frame.avi.colorspace = HDMI_COLORSPACE_YUV420;
>> +	else if (lspcon->output_format == CRTC_OUTPUT_YCBCR444)
>> +		frame.avi.colorspace = HDMI_COLORSPACE_YUV444;
>> +	else
>> +		frame.avi.colorspace = HDMI_COLORSPACE_RGB;
>> +
>>   	drm_hdmi_avi_infoframe_quant_range(&frame.avi, mode,
>>   					   crtc_state->limited_color_range ?
>>   					   HDMI_QUANTIZATION_RANGE_LIMITED :
>> @@ -512,6 +538,7 @@ bool lspcon_init(struct intel_digital_port *intel_dig_port)
>>   	struct intel_lspcon *lspcon = &intel_dig_port->lspcon;
>>   	struct drm_device *dev = intel_dig_port->base.base.dev;
>>   	struct drm_i915_private *dev_priv = to_i915(dev);
>> +	struct drm_connector *connector = &dp->attached_connector->base;
>>   
>>   	if (!HAS_LSPCON(dev_priv)) {
>>   		DRM_ERROR("LSPCON is not supported on this platform\n");
>> @@ -536,6 +563,7 @@ bool lspcon_init(struct intel_digital_port *intel_dig_port)
>>   		return false;
>>   	}
>>   
>> +	connector->ycbcr_420_allowed = true;
>>   	lspcon->active = true;
>>   	DRM_DEBUG_KMS("Success: LSPCON init\n");
>>   	return true;
>> -- 
>> 2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 7/7] drm/i915: Add YCBCR 4:2:0/4:4:4 support for LSPCON

[Maarten Lankhorst]

Op 18-01-18 om 07:27 schreef Sharma, Shashank:
> Regards
>
> Shashank
>
>
> On 1/17/2018 11:57 PM, Ville Syrjälä wrote:
>> On Fri, Jan 05, 2018 at 03:15:35PM +0530, Shashank Sharma wrote:
>>> LSPCON chips can generate YCBCR outputs, if asked nicely :).
>>>
>>> In order to generate YCBCR 4:2:0 outputs, a source must:
>>> - send YCBCR 4:4:4 signals to LSPCON
>>> - program color space as 4:2:0 in AVI infoframes
>>>
>>> Whereas for YCBCR 4:4:4 outputs, the source must:
>>> - send YCBCR 4:4:4 signals to LSPCON
>>> - program color space as 4:4:4 in AVI infoframes
>>>
>>> So for both 4:2:0 as well as 4:4:4 outputs, we are driving the
>>> pipe for YCBCR 4:4:4 output, but AVI infoframe's color space
>>> information indicates LSPCON FW to start scaling down from YCBCR
>>> 4:4:4 and generate YCBCR 4:2:0 output. As the scaling is done by
>>> LSPCON device, we need not to reserve a scaler for 4:2:0 outputs.
>>>
>>> V2: rebase
>>> V3: Addressed review comments from Ville
>>>      - add enum crtc_output_format instead of bool ycbcr420
>>>      - use crtc_output_format=4:4:4 for modeset of LSPCON 4:2:0 output
>>>        cases in this way we will have YCBCR 4:4:4 framework ready (except
>>>        the ABI part)
>>>
>>> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
>>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/i915_reg.h     |  2 ++
>>>   drivers/gpu/drm/i915/intel_ddi.c    |  7 +++++++
>>>   drivers/gpu/drm/i915/intel_dp.c     | 10 ++++++++++
>>>   drivers/gpu/drm/i915/intel_drv.h    |  2 ++
>>>   drivers/gpu/drm/i915/intel_lspcon.c | 28 ++++++++++++++++++++++++++++
>>>   5 files changed, 49 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>>> index 966e4df..45ee264 100644
>>> --- a/drivers/gpu/drm/i915/i915_reg.h
>>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>>> @@ -8547,6 +8547,8 @@ enum skl_power_gate {
>>>   #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        (2<<3)
>>>   #define  TRANS_MSA_6_BPC        (0<<5)
>>>   #define  TRANS_MSA_8_BPC        (1<<5)
>>>   #define  TRANS_MSA_10_BPC        (2<<5)
>>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
>>> index 7b89f2a..7616f6f 100644
>>> --- a/drivers/gpu/drm/i915/intel_ddi.c
>>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
>>> @@ -1499,6 +1499,13 @@ void intel_ddi_set_pipe_settings(const struct intel_crtc_state *crtc_state)
>>>           break;
>>>       }
>>>   +    /*
>>> +     * As per DP 1.2 spec section 2.3.4.3 while sending
>>> +     * YCBCR 444 signals we should program MSA MISC1/0 fields with
>>> +     * colorspace information.
>>> +     */
>>> +    if (crtc_state->output_format == CRTC_OUTPUT_YCBCR444)
>>> +        temp |= TRANS_MSA_SAMPLING_444 | TRANS_MSA_CLRSP_YCBCR;
>> This fails to state that we're indicating BT.601 encoding. I think we
>> should spell that out explicitly.
> Agree, I will add this in comments.
>>>       I915_WRITE(TRANS_MSA_MISC(cpu_transcoder), temp);
>>>   }
>>>   diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>>> index 35c5299..3bf82ea 100644
>>> --- a/drivers/gpu/drm/i915/intel_dp.c
>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>>> @@ -1613,6 +1613,7 @@ intel_dp_compute_config(struct intel_encoder *encoder,
>>>       struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>>>       struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode;
>>>       struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
>>> +    struct intel_lspcon *lspcon = enc_to_intel_lspcon(&encoder->base);
>>>       enum port port = encoder->port;
>>>       struct intel_crtc *intel_crtc = to_intel_crtc(pipe_config->base.crtc);
>>>       struct intel_connector *intel_connector = intel_dp->attached_connector;
>>> @@ -1642,6 +1643,15 @@ intel_dp_compute_config(struct intel_encoder *encoder,
>>>       if (HAS_PCH_SPLIT(dev_priv) && !HAS_DDI(dev_priv) && port != PORT_A)
>>>           pipe_config->has_pch_encoder = true;
>>>   +    if (lspcon->active) {
>>> +        struct drm_connector *connector = &intel_connector->base;
>>> +
>>> +        if (lspcon_ycbcr420_config(connector, pipe_config)) {
>>> +            pipe_config->output_format = CRTC_OUTPUT_YCBCR444;
>> I think I'd like to see all compute_config hooks explicitly set the
>> outout_format (to RGB usually obviously).
> That's the one I was talking about in previous patch. If we keep output_format_RGB = 0, we need
> not to do this, as reset of the pipe_config will automatically make it RGB.
>>> +            lspcon->output_format = CRTC_OUTPUT_YCBCR420;
>> You should not modify any non-atomic state like that in compute_config.
> Please help me to understand this better, Can you elaborate a bit more on:
> - Why is this a non-atomic state ?
Because lspcon->output_format is modified even if a commit is TEST_ONLY..

This will break if you do a nonblocking modeset vs TEST_ONLY, it could commit the wrong lspcon->output_format. :)
> - What is the right place we should modify it ?
intel_digital_connector_state probably.

~Maarten
>
> - Shashank
>>> +        }
>>> +    }
>>> +
>>>       pipe_config->has_drrs = false;
>>>       if (IS_G4X(dev_priv) || port == PORT_A)
>>>           pipe_config->has_audio = false;
>>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>>> index 2de6b41..5edba06 100644
>>> --- a/drivers/gpu/drm/i915/intel_drv.h
>>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>>> @@ -2047,6 +2047,8 @@ void lspcon_set_infoframes(struct drm_encoder *encoder,
>>>                  const struct drm_connector_state *conn_state);
>>>   bool lspcon_infoframe_enabled(struct drm_encoder *encoder,
>>>                     const struct intel_crtc_state *pipe_config);
>>> +bool lspcon_ycbcr420_config(struct drm_connector *connector,
>>> +                 struct intel_crtc_state *config);
>>>     /* intel_pipe_crc.c */
>>>   int intel_pipe_crc_create(struct drm_minor *minor);
>>> diff --git a/drivers/gpu/drm/i915/intel_lspcon.c b/drivers/gpu/drm/i915/intel_lspcon.c
>>> index 066ea91..cb88138 100644
>>> --- a/drivers/gpu/drm/i915/intel_lspcon.c
>>> +++ b/drivers/gpu/drm/i915/intel_lspcon.c
>>> @@ -180,6 +180,25 @@ static bool lspcon_wake_native_aux_ch(struct intel_lspcon *lspcon)
>>>       return true;
>>>   }
>>>   +bool lspcon_ycbcr420_config(struct drm_connector *connector,
>>> +                 struct intel_crtc_state *config)
>>> +{
>>> +    struct drm_display_info *info = &connector->display_info;
>>> +    struct drm_display_mode *mode = &config->base.adjusted_mode;
>>> +
>>> +    if (drm_mode_is_420_only(info, mode)) {
>>> +        if (!connector->ycbcr_420_allowed) {
>>> +            DRM_ERROR("Platform doesn't support YCBCR420 output\n");
>>> +            return false;
>>> +        }
>>> +
>>> +        config->port_clock /= 2;
>>> +        return true;
>>> +    }
>>> +
>>> +    return false;
>>> +}
>>> +
>>>   static bool lspcon_probe(struct intel_lspcon *lspcon)
>>>   {
>>>       int retry;
>>> @@ -459,6 +478,13 @@ void lspcon_set_infoframes(struct drm_encoder *encoder,
>>>           return;
>>>       }
>>>   +    if (lspcon->output_format == CRTC_OUTPUT_YCBCR420)
>>> +        frame.avi.colorspace = HDMI_COLORSPACE_YUV420;
>>> +    else if (lspcon->output_format == CRTC_OUTPUT_YCBCR444)
>>> +        frame.avi.colorspace = HDMI_COLORSPACE_YUV444;
>>> +    else
>>> +        frame.avi.colorspace = HDMI_COLORSPACE_RGB;
>>> +
>>>       drm_hdmi_avi_infoframe_quant_range(&frame.avi, mode,
>>>                          crtc_state->limited_color_range ?
>>>                          HDMI_QUANTIZATION_RANGE_LIMITED :
>>> @@ -512,6 +538,7 @@ bool lspcon_init(struct intel_digital_port *intel_dig_port)
>>>       struct intel_lspcon *lspcon = &intel_dig_port->lspcon;
>>>       struct drm_device *dev = intel_dig_port->base.base.dev;
>>>       struct drm_i915_private *dev_priv = to_i915(dev);
>>> +    struct drm_connector *connector = &dp->attached_connector->base;
>>>         if (!HAS_LSPCON(dev_priv)) {
>>>           DRM_ERROR("LSPCON is not supported on this platform\n");
>>> @@ -536,6 +563,7 @@ bool lspcon_init(struct intel_digital_port *intel_dig_port)
>>>           return false;
>>>       }
>>>   +    connector->ycbcr_420_allowed = true;
>>>       lspcon->active = true;
>>>       DRM_DEBUG_KMS("Success: LSPCON init\n");
>>>       return true;
>>> -- 
>>> 2.7.4
>

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 7/7] drm/i915: Add YCBCR 4:2:0/4:4:4 support for LSPCON

[Ville Syrjälä]

On Thu, Jan 18, 2018 at 11:57:09AM +0530, Sharma, Shashank wrote:
> Regards
> 
> Shashank
> 
> 
> On 1/17/2018 11:57 PM, Ville Syrjälä wrote:
> > On Fri, Jan 05, 2018 at 03:15:35PM +0530, Shashank Sharma wrote:
> >> LSPCON chips can generate YCBCR outputs, if asked nicely :).
> >>
> >> In order to generate YCBCR 4:2:0 outputs, a source must:
> >> - send YCBCR 4:4:4 signals to LSPCON
> >> - program color space as 4:2:0 in AVI infoframes
> >>
> >> Whereas for YCBCR 4:4:4 outputs, the source must:
> >> - send YCBCR 4:4:4 signals to LSPCON
> >> - program color space as 4:4:4 in AVI infoframes
> >>
> >> So for both 4:2:0 as well as 4:4:4 outputs, we are driving the
> >> pipe for YCBCR 4:4:4 output, but AVI infoframe's color space
> >> information indicates LSPCON FW to start scaling down from YCBCR
> >> 4:4:4 and generate YCBCR 4:2:0 output. As the scaling is done by
> >> LSPCON device, we need not to reserve a scaler for 4:2:0 outputs.
> >>
> >> V2: rebase
> >> V3: Addressed review comments from Ville
> >>      - add enum crtc_output_format instead of bool ycbcr420
> >>      - use crtc_output_format=4:4:4 for modeset of LSPCON 4:2:0 output
> >>        cases in this way we will have YCBCR 4:4:4 framework ready (except
> >>        the ABI part)
> >>
> >> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> >> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> >> ---
> >>   drivers/gpu/drm/i915/i915_reg.h     |  2 ++
> >>   drivers/gpu/drm/i915/intel_ddi.c    |  7 +++++++
> >>   drivers/gpu/drm/i915/intel_dp.c     | 10 ++++++++++
> >>   drivers/gpu/drm/i915/intel_drv.h    |  2 ++
> >>   drivers/gpu/drm/i915/intel_lspcon.c | 28 ++++++++++++++++++++++++++++
> >>   5 files changed, 49 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> >> index 966e4df..45ee264 100644
> >> --- a/drivers/gpu/drm/i915/i915_reg.h
> >> +++ b/drivers/gpu/drm/i915/i915_reg.h
> >> @@ -8547,6 +8547,8 @@ enum skl_power_gate {
> >>   #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		(2<<3)
> >>   #define  TRANS_MSA_6_BPC		(0<<5)
> >>   #define  TRANS_MSA_8_BPC		(1<<5)
> >>   #define  TRANS_MSA_10_BPC		(2<<5)
> >> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> >> index 7b89f2a..7616f6f 100644
> >> --- a/drivers/gpu/drm/i915/intel_ddi.c
> >> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> >> @@ -1499,6 +1499,13 @@ void intel_ddi_set_pipe_settings(const struct intel_crtc_state *crtc_state)
> >>   		break;
> >>   	}
> >>   
> >> +	/*
> >> +	 * As per DP 1.2 spec section 2.3.4.3 while sending
> >> +	 * YCBCR 444 signals we should program MSA MISC1/0 fields with
> >> +	 * colorspace information.
> >> +	 */
> >> +	if (crtc_state->output_format == CRTC_OUTPUT_YCBCR444)
> >> +		temp |= TRANS_MSA_SAMPLING_444 | TRANS_MSA_CLRSP_YCBCR;
> > This fails to state that we're indicating BT.601 encoding. I think we
> > should spell that out explicitly.
> Agree, I will add this in comments.
> >>   	I915_WRITE(TRANS_MSA_MISC(cpu_transcoder), temp);
> >>   }
> >>   
> >> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> >> index 35c5299..3bf82ea 100644
> >> --- a/drivers/gpu/drm/i915/intel_dp.c
> >> +++ b/drivers/gpu/drm/i915/intel_dp.c
> >> @@ -1613,6 +1613,7 @@ intel_dp_compute_config(struct intel_encoder *encoder,
> >>   	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> >>   	struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode;
> >>   	struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
> >> +	struct intel_lspcon *lspcon = enc_to_intel_lspcon(&encoder->base);
> >>   	enum port port = encoder->port;
> >>   	struct intel_crtc *intel_crtc = to_intel_crtc(pipe_config->base.crtc);
> >>   	struct intel_connector *intel_connector = intel_dp->attached_connector;
> >> @@ -1642,6 +1643,15 @@ intel_dp_compute_config(struct intel_encoder *encoder,
> >>   	if (HAS_PCH_SPLIT(dev_priv) && !HAS_DDI(dev_priv) && port != PORT_A)
> >>   		pipe_config->has_pch_encoder = true;
> >>   
> >> +	if (lspcon->active) {
> >> +		struct drm_connector *connector = &intel_connector->base;
> >> +
> >> +		if (lspcon_ycbcr420_config(connector, pipe_config)) {
> >> +			pipe_config->output_format = CRTC_OUTPUT_YCBCR444;
> > I think I'd like to see all compute_config hooks explicitly set the
> > outout_format (to RGB usually obviously).
> That's the one I was talking about in previous patch. If we keep 
> output_format_RGB = 0, we need
> not to do this, as reset of the pipe_config will automatically make it RGB.

IMO either we have INVALID=0 so that we use it to catch readout
fails, or we have no INVALID. Other options make little sense to me.

> >> +			lspcon->output_format = CRTC_OUTPUT_YCBCR420;
> > You should not modify any non-atomic state like that in compute_config.
> Please help me to understand this better, Can you elaborate a bit more on:
> - Why is this a non-atomic state ?
> - What is the right place we should modify it ?

I don't think you need it at all. Just consult the crtc state when
needed.

> 
> - Shashank
> >> +		}
> >> +	}
> >> +
> >>   	pipe_config->has_drrs = false;
> >>   	if (IS_G4X(dev_priv) || port == PORT_A)
> >>   		pipe_config->has_audio = false;
> >> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> >> index 2de6b41..5edba06 100644
> >> --- a/drivers/gpu/drm/i915/intel_drv.h
> >> +++ b/drivers/gpu/drm/i915/intel_drv.h
> >> @@ -2047,6 +2047,8 @@ void lspcon_set_infoframes(struct drm_encoder *encoder,
> >>   			   const struct drm_connector_state *conn_state);
> >>   bool lspcon_infoframe_enabled(struct drm_encoder *encoder,
> >>   			      const struct intel_crtc_state *pipe_config);
> >> +bool lspcon_ycbcr420_config(struct drm_connector *connector,
> >> +			     struct intel_crtc_state *config);
> >>   
> >>   /* intel_pipe_crc.c */
> >>   int intel_pipe_crc_create(struct drm_minor *minor);
> >> diff --git a/drivers/gpu/drm/i915/intel_lspcon.c b/drivers/gpu/drm/i915/intel_lspcon.c
> >> index 066ea91..cb88138 100644
> >> --- a/drivers/gpu/drm/i915/intel_lspcon.c
> >> +++ b/drivers/gpu/drm/i915/intel_lspcon.c
> >> @@ -180,6 +180,25 @@ static bool lspcon_wake_native_aux_ch(struct intel_lspcon *lspcon)
> >>   	return true;
> >>   }
> >>   
> >> +bool lspcon_ycbcr420_config(struct drm_connector *connector,
> >> +			     struct intel_crtc_state *config)
> >> +{
> >> +	struct drm_display_info *info = &connector->display_info;
> >> +	struct drm_display_mode *mode = &config->base.adjusted_mode;
> >> +
> >> +	if (drm_mode_is_420_only(info, mode)) {
> >> +		if (!connector->ycbcr_420_allowed) {
> >> +			DRM_ERROR("Platform doesn't support YCBCR420 output\n");
> >> +			return false;
> >> +		}
> >> +
> >> +		config->port_clock /= 2;
> >> +		return true;
> >> +	}
> >> +
> >> +	return false;
> >> +}
> >> +
> >>   static bool lspcon_probe(struct intel_lspcon *lspcon)
> >>   {
> >>   	int retry;
> >> @@ -459,6 +478,13 @@ void lspcon_set_infoframes(struct drm_encoder *encoder,
> >>   		return;
> >>   	}
> >>   
> >> +	if (lspcon->output_format == CRTC_OUTPUT_YCBCR420)
> >> +		frame.avi.colorspace = HDMI_COLORSPACE_YUV420;
> >> +	else if (lspcon->output_format == CRTC_OUTPUT_YCBCR444)
> >> +		frame.avi.colorspace = HDMI_COLORSPACE_YUV444;
> >> +	else
> >> +		frame.avi.colorspace = HDMI_COLORSPACE_RGB;
> >> +
> >>   	drm_hdmi_avi_infoframe_quant_range(&frame.avi, mode,
> >>   					   crtc_state->limited_color_range ?
> >>   					   HDMI_QUANTIZATION_RANGE_LIMITED :
> >> @@ -512,6 +538,7 @@ bool lspcon_init(struct intel_digital_port *intel_dig_port)
> >>   	struct intel_lspcon *lspcon = &intel_dig_port->lspcon;
> >>   	struct drm_device *dev = intel_dig_port->base.base.dev;
> >>   	struct drm_i915_private *dev_priv = to_i915(dev);
> >> +	struct drm_connector *connector = &dp->attached_connector->base;
> >>   
> >>   	if (!HAS_LSPCON(dev_priv)) {
> >>   		DRM_ERROR("LSPCON is not supported on this platform\n");
> >> @@ -536,6 +563,7 @@ bool lspcon_init(struct intel_digital_port *intel_dig_port)
> >>   		return false;
> >>   	}
> >>   
> >> +	connector->ycbcr_420_allowed = true;
> >>   	lspcon->active = true;
> >>   	DRM_DEBUG_KMS("Success: LSPCON init\n");
> >>   	return true;
> >> -- 
> >> 2.7.4

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 7/7] drm/i915: Add YCBCR 4:2:0/4:4:4 support for LSPCON

[Sharma, Shashank]

Regards

Shashank


On 1/18/2018 7:33 PM, Ville Syrjälä wrote:
> On Thu, Jan 18, 2018 at 11:57:09AM +0530, Sharma, Shashank wrote:
>> Regards
>>
>> Shashank
>>
>>
>> On 1/17/2018 11:57 PM, Ville Syrjälä wrote:
>>> On Fri, Jan 05, 2018 at 03:15:35PM +0530, Shashank Sharma wrote:
>>>> LSPCON chips can generate YCBCR outputs, if asked nicely :).
>>>>
>>>> In order to generate YCBCR 4:2:0 outputs, a source must:
>>>> - send YCBCR 4:4:4 signals to LSPCON
>>>> - program color space as 4:2:0 in AVI infoframes
>>>>
>>>> Whereas for YCBCR 4:4:4 outputs, the source must:
>>>> - send YCBCR 4:4:4 signals to LSPCON
>>>> - program color space as 4:4:4 in AVI infoframes
>>>>
>>>> So for both 4:2:0 as well as 4:4:4 outputs, we are driving the
>>>> pipe for YCBCR 4:4:4 output, but AVI infoframe's color space
>>>> information indicates LSPCON FW to start scaling down from YCBCR
>>>> 4:4:4 and generate YCBCR 4:2:0 output. As the scaling is done by
>>>> LSPCON device, we need not to reserve a scaler for 4:2:0 outputs.
>>>>
>>>> V2: rebase
>>>> V3: Addressed review comments from Ville
>>>>       - add enum crtc_output_format instead of bool ycbcr420
>>>>       - use crtc_output_format=4:4:4 for modeset of LSPCON 4:2:0 output
>>>>         cases in this way we will have YCBCR 4:4:4 framework ready (except
>>>>         the ABI part)
>>>>
>>>> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
>>>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
>>>> ---
>>>>    drivers/gpu/drm/i915/i915_reg.h     |  2 ++
>>>>    drivers/gpu/drm/i915/intel_ddi.c    |  7 +++++++
>>>>    drivers/gpu/drm/i915/intel_dp.c     | 10 ++++++++++
>>>>    drivers/gpu/drm/i915/intel_drv.h    |  2 ++
>>>>    drivers/gpu/drm/i915/intel_lspcon.c | 28 ++++++++++++++++++++++++++++
>>>>    5 files changed, 49 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>>>> index 966e4df..45ee264 100644
>>>> --- a/drivers/gpu/drm/i915/i915_reg.h
>>>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>>>> @@ -8547,6 +8547,8 @@ enum skl_power_gate {
>>>>    #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		(2<<3)
>>>>    #define  TRANS_MSA_6_BPC		(0<<5)
>>>>    #define  TRANS_MSA_8_BPC		(1<<5)
>>>>    #define  TRANS_MSA_10_BPC		(2<<5)
>>>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
>>>> index 7b89f2a..7616f6f 100644
>>>> --- a/drivers/gpu/drm/i915/intel_ddi.c
>>>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
>>>> @@ -1499,6 +1499,13 @@ void intel_ddi_set_pipe_settings(const struct intel_crtc_state *crtc_state)
>>>>    		break;
>>>>    	}
>>>>    
>>>> +	/*
>>>> +	 * As per DP 1.2 spec section 2.3.4.3 while sending
>>>> +	 * YCBCR 444 signals we should program MSA MISC1/0 fields with
>>>> +	 * colorspace information.
>>>> +	 */
>>>> +	if (crtc_state->output_format == CRTC_OUTPUT_YCBCR444)
>>>> +		temp |= TRANS_MSA_SAMPLING_444 | TRANS_MSA_CLRSP_YCBCR;
>>> This fails to state that we're indicating BT.601 encoding. I think we
>>> should spell that out explicitly.
>> Agree, I will add this in comments.
>>>>    	I915_WRITE(TRANS_MSA_MISC(cpu_transcoder), temp);
>>>>    }
>>>>    
>>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>>>> index 35c5299..3bf82ea 100644
>>>> --- a/drivers/gpu/drm/i915/intel_dp.c
>>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>>>> @@ -1613,6 +1613,7 @@ intel_dp_compute_config(struct intel_encoder *encoder,
>>>>    	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>>>>    	struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode;
>>>>    	struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
>>>> +	struct intel_lspcon *lspcon = enc_to_intel_lspcon(&encoder->base);
>>>>    	enum port port = encoder->port;
>>>>    	struct intel_crtc *intel_crtc = to_intel_crtc(pipe_config->base.crtc);
>>>>    	struct intel_connector *intel_connector = intel_dp->attached_connector;
>>>> @@ -1642,6 +1643,15 @@ intel_dp_compute_config(struct intel_encoder *encoder,
>>>>    	if (HAS_PCH_SPLIT(dev_priv) && !HAS_DDI(dev_priv) && port != PORT_A)
>>>>    		pipe_config->has_pch_encoder = true;
>>>>    
>>>> +	if (lspcon->active) {
>>>> +		struct drm_connector *connector = &intel_connector->base;
>>>> +
>>>> +		if (lspcon_ycbcr420_config(connector, pipe_config)) {
>>>> +			pipe_config->output_format = CRTC_OUTPUT_YCBCR444;
>>> I think I'd like to see all compute_config hooks explicitly set the
>>> outout_format (to RGB usually obviously).
>> That's the one I was talking about in previous patch. If we keep
>> output_format_RGB = 0, we need
>> not to do this, as reset of the pipe_config will automatically make it RGB.
> IMO either we have INVALID=0 so that we use it to catch readout
> fails, or we have no INVALID. Other options make little sense to me.
Its a minor thing, and doesn't really matter. I can change this.
>>>> +			lspcon->output_format = CRTC_OUTPUT_YCBCR420;
>>> You should not modify any non-atomic state like that in compute_config.
>> Please help me to understand this better, Can you elaborate a bit more on:
>> - Why is this a non-atomic state ?
>> - What is the right place we should modify it ?
> I don't think you need it at all. Just consult the crtc state when
> needed.
I dont think CRTC state can cover all the cases, for example, what would 
be do when we need
YCBCR 4:4:4 output from LSPCON ? As we have already used crtc_state->444 
for LSPCON output
420, we can't handle this. This is equivalent to your previous 
suggestion of adding 'scaling_reqd',
but I added in LPSCON (instead of CRTC state) as this information is 
required only in case of LSPCON.
In all native cases, scaling would be always required for 4:2:0 outputs.

- Shashank
>> - Shashank
>>>> +		}
>>>> +	}
>>>> +
>>>>    	pipe_config->has_drrs = false;
>>>>    	if (IS_G4X(dev_priv) || port == PORT_A)
>>>>    		pipe_config->has_audio = false;
>>>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>>>> index 2de6b41..5edba06 100644
>>>> --- a/drivers/gpu/drm/i915/intel_drv.h
>>>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>>>> @@ -2047,6 +2047,8 @@ void lspcon_set_infoframes(struct drm_encoder *encoder,
>>>>    			   const struct drm_connector_state *conn_state);
>>>>    bool lspcon_infoframe_enabled(struct drm_encoder *encoder,
>>>>    			      const struct intel_crtc_state *pipe_config);
>>>> +bool lspcon_ycbcr420_config(struct drm_connector *connector,
>>>> +			     struct intel_crtc_state *config);
>>>>    
>>>>    /* intel_pipe_crc.c */
>>>>    int intel_pipe_crc_create(struct drm_minor *minor);
>>>> diff --git a/drivers/gpu/drm/i915/intel_lspcon.c b/drivers/gpu/drm/i915/intel_lspcon.c
>>>> index 066ea91..cb88138 100644
>>>> --- a/drivers/gpu/drm/i915/intel_lspcon.c
>>>> +++ b/drivers/gpu/drm/i915/intel_lspcon.c
>>>> @@ -180,6 +180,25 @@ static bool lspcon_wake_native_aux_ch(struct intel_lspcon *lspcon)
>>>>    	return true;
>>>>    }
>>>>    
>>>> +bool lspcon_ycbcr420_config(struct drm_connector *connector,
>>>> +			     struct intel_crtc_state *config)
>>>> +{
>>>> +	struct drm_display_info *info = &connector->display_info;
>>>> +	struct drm_display_mode *mode = &config->base.adjusted_mode;
>>>> +
>>>> +	if (drm_mode_is_420_only(info, mode)) {
>>>> +		if (!connector->ycbcr_420_allowed) {
>>>> +			DRM_ERROR("Platform doesn't support YCBCR420 output\n");
>>>> +			return false;
>>>> +		}
>>>> +
>>>> +		config->port_clock /= 2;
>>>> +		return true;
>>>> +	}
>>>> +
>>>> +	return false;
>>>> +}
>>>> +
>>>>    static bool lspcon_probe(struct intel_lspcon *lspcon)
>>>>    {
>>>>    	int retry;
>>>> @@ -459,6 +478,13 @@ void lspcon_set_infoframes(struct drm_encoder *encoder,
>>>>    		return;
>>>>    	}
>>>>    
>>>> +	if (lspcon->output_format == CRTC_OUTPUT_YCBCR420)
>>>> +		frame.avi.colorspace = HDMI_COLORSPACE_YUV420;
>>>> +	else if (lspcon->output_format == CRTC_OUTPUT_YCBCR444)
>>>> +		frame.avi.colorspace = HDMI_COLORSPACE_YUV444;
>>>> +	else
>>>> +		frame.avi.colorspace = HDMI_COLORSPACE_RGB;
>>>> +
>>>>    	drm_hdmi_avi_infoframe_quant_range(&frame.avi, mode,
>>>>    					   crtc_state->limited_color_range ?
>>>>    					   HDMI_QUANTIZATION_RANGE_LIMITED :
>>>> @@ -512,6 +538,7 @@ bool lspcon_init(struct intel_digital_port *intel_dig_port)
>>>>    	struct intel_lspcon *lspcon = &intel_dig_port->lspcon;
>>>>    	struct drm_device *dev = intel_dig_port->base.base.dev;
>>>>    	struct drm_i915_private *dev_priv = to_i915(dev);
>>>> +	struct drm_connector *connector = &dp->attached_connector->base;
>>>>    
>>>>    	if (!HAS_LSPCON(dev_priv)) {
>>>>    		DRM_ERROR("LSPCON is not supported on this platform\n");
>>>> @@ -536,6 +563,7 @@ bool lspcon_init(struct intel_digital_port *intel_dig_port)
>>>>    		return false;
>>>>    	}
>>>>    
>>>> +	connector->ycbcr_420_allowed = true;
>>>>    	lspcon->active = true;
>>>>    	DRM_DEBUG_KMS("Success: LSPCON init\n");
>>>>    	return true;
>>>> -- 
>>>> 2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 7/7] drm/i915: Add YCBCR 4:2:0/4:4:4 support for LSPCON

[Ville Syrjälä]

On Thu, Jan 18, 2018 at 09:00:50PM +0530, Sharma, Shashank wrote:
> Regards
> 
> Shashank
> 
> 
> On 1/18/2018 7:33 PM, Ville Syrjälä wrote:
> > On Thu, Jan 18, 2018 at 11:57:09AM +0530, Sharma, Shashank wrote:
> >> Regards
> >>
> >> Shashank
> >>
> >>
> >> On 1/17/2018 11:57 PM, Ville Syrjälä wrote:
> >>> On Fri, Jan 05, 2018 at 03:15:35PM +0530, Shashank Sharma wrote:
> >>>> LSPCON chips can generate YCBCR outputs, if asked nicely :).
> >>>>
> >>>> In order to generate YCBCR 4:2:0 outputs, a source must:
> >>>> - send YCBCR 4:4:4 signals to LSPCON
> >>>> - program color space as 4:2:0 in AVI infoframes
> >>>>
> >>>> Whereas for YCBCR 4:4:4 outputs, the source must:
> >>>> - send YCBCR 4:4:4 signals to LSPCON
> >>>> - program color space as 4:4:4 in AVI infoframes
> >>>>
> >>>> So for both 4:2:0 as well as 4:4:4 outputs, we are driving the
> >>>> pipe for YCBCR 4:4:4 output, but AVI infoframe's color space
> >>>> information indicates LSPCON FW to start scaling down from YCBCR
> >>>> 4:4:4 and generate YCBCR 4:2:0 output. As the scaling is done by
> >>>> LSPCON device, we need not to reserve a scaler for 4:2:0 outputs.
> >>>>
> >>>> V2: rebase
> >>>> V3: Addressed review comments from Ville
> >>>>       - add enum crtc_output_format instead of bool ycbcr420
> >>>>       - use crtc_output_format=4:4:4 for modeset of LSPCON 4:2:0 output
> >>>>         cases in this way we will have YCBCR 4:4:4 framework ready (except
> >>>>         the ABI part)
> >>>>
> >>>> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> >>>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >>>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> >>>> ---
> >>>>    drivers/gpu/drm/i915/i915_reg.h     |  2 ++
> >>>>    drivers/gpu/drm/i915/intel_ddi.c    |  7 +++++++
> >>>>    drivers/gpu/drm/i915/intel_dp.c     | 10 ++++++++++
> >>>>    drivers/gpu/drm/i915/intel_drv.h    |  2 ++
> >>>>    drivers/gpu/drm/i915/intel_lspcon.c | 28 ++++++++++++++++++++++++++++
> >>>>    5 files changed, 49 insertions(+)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> >>>> index 966e4df..45ee264 100644
> >>>> --- a/drivers/gpu/drm/i915/i915_reg.h
> >>>> +++ b/drivers/gpu/drm/i915/i915_reg.h
> >>>> @@ -8547,6 +8547,8 @@ enum skl_power_gate {
> >>>>    #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		(2<<3)
> >>>>    #define  TRANS_MSA_6_BPC		(0<<5)
> >>>>    #define  TRANS_MSA_8_BPC		(1<<5)
> >>>>    #define  TRANS_MSA_10_BPC		(2<<5)
> >>>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> >>>> index 7b89f2a..7616f6f 100644
> >>>> --- a/drivers/gpu/drm/i915/intel_ddi.c
> >>>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> >>>> @@ -1499,6 +1499,13 @@ void intel_ddi_set_pipe_settings(const struct intel_crtc_state *crtc_state)
> >>>>    		break;
> >>>>    	}
> >>>>    
> >>>> +	/*
> >>>> +	 * As per DP 1.2 spec section 2.3.4.3 while sending
> >>>> +	 * YCBCR 444 signals we should program MSA MISC1/0 fields with
> >>>> +	 * colorspace information.
> >>>> +	 */
> >>>> +	if (crtc_state->output_format == CRTC_OUTPUT_YCBCR444)
> >>>> +		temp |= TRANS_MSA_SAMPLING_444 | TRANS_MSA_CLRSP_YCBCR;
> >>> This fails to state that we're indicating BT.601 encoding. I think we
> >>> should spell that out explicitly.
> >> Agree, I will add this in comments.
> >>>>    	I915_WRITE(TRANS_MSA_MISC(cpu_transcoder), temp);
> >>>>    }
> >>>>    
> >>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> >>>> index 35c5299..3bf82ea 100644
> >>>> --- a/drivers/gpu/drm/i915/intel_dp.c
> >>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
> >>>> @@ -1613,6 +1613,7 @@ intel_dp_compute_config(struct intel_encoder *encoder,
> >>>>    	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> >>>>    	struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode;
> >>>>    	struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
> >>>> +	struct intel_lspcon *lspcon = enc_to_intel_lspcon(&encoder->base);
> >>>>    	enum port port = encoder->port;
> >>>>    	struct intel_crtc *intel_crtc = to_intel_crtc(pipe_config->base.crtc);
> >>>>    	struct intel_connector *intel_connector = intel_dp->attached_connector;
> >>>> @@ -1642,6 +1643,15 @@ intel_dp_compute_config(struct intel_encoder *encoder,
> >>>>    	if (HAS_PCH_SPLIT(dev_priv) && !HAS_DDI(dev_priv) && port != PORT_A)
> >>>>    		pipe_config->has_pch_encoder = true;
> >>>>    
> >>>> +	if (lspcon->active) {
> >>>> +		struct drm_connector *connector = &intel_connector->base;
> >>>> +
> >>>> +		if (lspcon_ycbcr420_config(connector, pipe_config)) {
> >>>> +			pipe_config->output_format = CRTC_OUTPUT_YCBCR444;
> >>> I think I'd like to see all compute_config hooks explicitly set the
> >>> outout_format (to RGB usually obviously).
> >> That's the one I was talking about in previous patch. If we keep
> >> output_format_RGB = 0, we need
> >> not to do this, as reset of the pipe_config will automatically make it RGB.
> > IMO either we have INVALID=0 so that we use it to catch readout
> > fails, or we have no INVALID. Other options make little sense to me.
> Its a minor thing, and doesn't really matter. I can change this.
> >>>> +			lspcon->output_format = CRTC_OUTPUT_YCBCR420;
> >>> You should not modify any non-atomic state like that in compute_config.
> >> Please help me to understand this better, Can you elaborate a bit more on:
> >> - Why is this a non-atomic state ?
> >> - What is the right place we should modify it ?
> > I don't think you need it at all. Just consult the crtc state when
> > needed.
> I dont think CRTC state can cover all the cases, for example, what would 
> be do when we need
> YCBCR 4:4:4 output from LSPCON ? As we have already used crtc_state->444 
> for LSPCON output
> 420, we can't handle this.

Like I said earlier, just add another thing to the state to indicate what
LSPCON should do with the 444 data. Either pass it through or downsample
it.

> This is equivalent to your previous 
> suggestion of adding 'scaling_reqd',
> but I added in LPSCON (instead of CRTC state) as this information is 
> required only in case of LSPCON.

But we have no atomic lspcon state. And probably not worth adding one
since there won't be much there. So just adding what's needed into the
crtc state seems like the best approach.

> In all native cases, scaling would be always required for 4:2:0 outputs.
> 
> - Shashank
> >> - Shashank
> >>>> +		}
> >>>> +	}
> >>>> +
> >>>>    	pipe_config->has_drrs = false;
> >>>>    	if (IS_G4X(dev_priv) || port == PORT_A)
> >>>>    		pipe_config->has_audio = false;
> >>>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> >>>> index 2de6b41..5edba06 100644
> >>>> --- a/drivers/gpu/drm/i915/intel_drv.h
> >>>> +++ b/drivers/gpu/drm/i915/intel_drv.h
> >>>> @@ -2047,6 +2047,8 @@ void lspcon_set_infoframes(struct drm_encoder *encoder,
> >>>>    			   const struct drm_connector_state *conn_state);
> >>>>    bool lspcon_infoframe_enabled(struct drm_encoder *encoder,
> >>>>    			      const struct intel_crtc_state *pipe_config);
> >>>> +bool lspcon_ycbcr420_config(struct drm_connector *connector,
> >>>> +			     struct intel_crtc_state *config);
> >>>>    
> >>>>    /* intel_pipe_crc.c */
> >>>>    int intel_pipe_crc_create(struct drm_minor *minor);
> >>>> diff --git a/drivers/gpu/drm/i915/intel_lspcon.c b/drivers/gpu/drm/i915/intel_lspcon.c
> >>>> index 066ea91..cb88138 100644
> >>>> --- a/drivers/gpu/drm/i915/intel_lspcon.c
> >>>> +++ b/drivers/gpu/drm/i915/intel_lspcon.c
> >>>> @@ -180,6 +180,25 @@ static bool lspcon_wake_native_aux_ch(struct intel_lspcon *lspcon)
> >>>>    	return true;
> >>>>    }
> >>>>    
> >>>> +bool lspcon_ycbcr420_config(struct drm_connector *connector,
> >>>> +			     struct intel_crtc_state *config)
> >>>> +{
> >>>> +	struct drm_display_info *info = &connector->display_info;
> >>>> +	struct drm_display_mode *mode = &config->base.adjusted_mode;
> >>>> +
> >>>> +	if (drm_mode_is_420_only(info, mode)) {
> >>>> +		if (!connector->ycbcr_420_allowed) {
> >>>> +			DRM_ERROR("Platform doesn't support YCBCR420 output\n");
> >>>> +			return false;
> >>>> +		}
> >>>> +
> >>>> +		config->port_clock /= 2;
> >>>> +		return true;
> >>>> +	}
> >>>> +
> >>>> +	return false;
> >>>> +}
> >>>> +
> >>>>    static bool lspcon_probe(struct intel_lspcon *lspcon)
> >>>>    {
> >>>>    	int retry;
> >>>> @@ -459,6 +478,13 @@ void lspcon_set_infoframes(struct drm_encoder *encoder,
> >>>>    		return;
> >>>>    	}
> >>>>    
> >>>> +	if (lspcon->output_format == CRTC_OUTPUT_YCBCR420)
> >>>> +		frame.avi.colorspace = HDMI_COLORSPACE_YUV420;
> >>>> +	else if (lspcon->output_format == CRTC_OUTPUT_YCBCR444)
> >>>> +		frame.avi.colorspace = HDMI_COLORSPACE_YUV444;
> >>>> +	else
> >>>> +		frame.avi.colorspace = HDMI_COLORSPACE_RGB;
> >>>> +
> >>>>    	drm_hdmi_avi_infoframe_quant_range(&frame.avi, mode,
> >>>>    					   crtc_state->limited_color_range ?
> >>>>    					   HDMI_QUANTIZATION_RANGE_LIMITED :
> >>>> @@ -512,6 +538,7 @@ bool lspcon_init(struct intel_digital_port *intel_dig_port)
> >>>>    	struct intel_lspcon *lspcon = &intel_dig_port->lspcon;
> >>>>    	struct drm_device *dev = intel_dig_port->base.base.dev;
> >>>>    	struct drm_i915_private *dev_priv = to_i915(dev);
> >>>> +	struct drm_connector *connector = &dp->attached_connector->base;
> >>>>    
> >>>>    	if (!HAS_LSPCON(dev_priv)) {
> >>>>    		DRM_ERROR("LSPCON is not supported on this platform\n");
> >>>> @@ -536,6 +563,7 @@ bool lspcon_init(struct intel_digital_port *intel_dig_port)
> >>>>    		return false;
> >>>>    	}
> >>>>    
> >>>> +	connector->ycbcr_420_allowed = true;
> >>>>    	lspcon->active = true;
> >>>>    	DRM_DEBUG_KMS("Success: LSPCON init\n");
> >>>>    	return true;
> >>>> -- 
> >>>> 2.7.4

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 7/7] drm/i915: Add YCBCR 4:2:0/4:4:4 support for LSPCON

[Sharma, Shashank]

Regards

Shashank


On 1/18/2018 9:05 PM, Ville Syrjälä wrote:
> On Thu, Jan 18, 2018 at 09:00:50PM +0530, Sharma, Shashank wrote:
>> Regards
>>
>> Shashank
>>
>>
>> On 1/18/2018 7:33 PM, Ville Syrjälä wrote:
>>> On Thu, Jan 18, 2018 at 11:57:09AM +0530, Sharma, Shashank wrote:
>>>> Regards
>>>>
>>>> Shashank
>>>>
>>>>
>>>> On 1/17/2018 11:57 PM, Ville Syrjälä wrote:
>>>>> On Fri, Jan 05, 2018 at 03:15:35PM +0530, Shashank Sharma wrote:
>>>>>> LSPCON chips can generate YCBCR outputs, if asked nicely :).
>>>>>>
>>>>>> In order to generate YCBCR 4:2:0 outputs, a source must:
>>>>>> - send YCBCR 4:4:4 signals to LSPCON
>>>>>> - program color space as 4:2:0 in AVI infoframes
>>>>>>
>>>>>> Whereas for YCBCR 4:4:4 outputs, the source must:
>>>>>> - send YCBCR 4:4:4 signals to LSPCON
>>>>>> - program color space as 4:4:4 in AVI infoframes
>>>>>>
>>>>>> So for both 4:2:0 as well as 4:4:4 outputs, we are driving the
>>>>>> pipe for YCBCR 4:4:4 output, but AVI infoframe's color space
>>>>>> information indicates LSPCON FW to start scaling down from YCBCR
>>>>>> 4:4:4 and generate YCBCR 4:2:0 output. As the scaling is done by
>>>>>> LSPCON device, we need not to reserve a scaler for 4:2:0 outputs.
>>>>>>
>>>>>> V2: rebase
>>>>>> V3: Addressed review comments from Ville
>>>>>>        - add enum crtc_output_format instead of bool ycbcr420
>>>>>>        - use crtc_output_format=4:4:4 for modeset of LSPCON 4:2:0 output
>>>>>>          cases in this way we will have YCBCR 4:4:4 framework ready (except
>>>>>>          the ABI part)
>>>>>>
>>>>>> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
>>>>>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>>>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
>>>>>> ---
>>>>>>     drivers/gpu/drm/i915/i915_reg.h     |  2 ++
>>>>>>     drivers/gpu/drm/i915/intel_ddi.c    |  7 +++++++
>>>>>>     drivers/gpu/drm/i915/intel_dp.c     | 10 ++++++++++
>>>>>>     drivers/gpu/drm/i915/intel_drv.h    |  2 ++
>>>>>>     drivers/gpu/drm/i915/intel_lspcon.c | 28 ++++++++++++++++++++++++++++
>>>>>>     5 files changed, 49 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>>>>>> index 966e4df..45ee264 100644
>>>>>> --- a/drivers/gpu/drm/i915/i915_reg.h
>>>>>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>>>>>> @@ -8547,6 +8547,8 @@ enum skl_power_gate {
>>>>>>     #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		(2<<3)
>>>>>>     #define  TRANS_MSA_6_BPC		(0<<5)
>>>>>>     #define  TRANS_MSA_8_BPC		(1<<5)
>>>>>>     #define  TRANS_MSA_10_BPC		(2<<5)
>>>>>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
>>>>>> index 7b89f2a..7616f6f 100644
>>>>>> --- a/drivers/gpu/drm/i915/intel_ddi.c
>>>>>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
>>>>>> @@ -1499,6 +1499,13 @@ void intel_ddi_set_pipe_settings(const struct intel_crtc_state *crtc_state)
>>>>>>     		break;
>>>>>>     	}
>>>>>>     
>>>>>> +	/*
>>>>>> +	 * As per DP 1.2 spec section 2.3.4.3 while sending
>>>>>> +	 * YCBCR 444 signals we should program MSA MISC1/0 fields with
>>>>>> +	 * colorspace information.
>>>>>> +	 */
>>>>>> +	if (crtc_state->output_format == CRTC_OUTPUT_YCBCR444)
>>>>>> +		temp |= TRANS_MSA_SAMPLING_444 | TRANS_MSA_CLRSP_YCBCR;
>>>>> This fails to state that we're indicating BT.601 encoding. I think we
>>>>> should spell that out explicitly.
>>>> Agree, I will add this in comments.
>>>>>>     	I915_WRITE(TRANS_MSA_MISC(cpu_transcoder), temp);
>>>>>>     }
>>>>>>     
>>>>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>>>>>> index 35c5299..3bf82ea 100644
>>>>>> --- a/drivers/gpu/drm/i915/intel_dp.c
>>>>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>>>>>> @@ -1613,6 +1613,7 @@ intel_dp_compute_config(struct intel_encoder *encoder,
>>>>>>     	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>>>>>>     	struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode;
>>>>>>     	struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
>>>>>> +	struct intel_lspcon *lspcon = enc_to_intel_lspcon(&encoder->base);
>>>>>>     	enum port port = encoder->port;
>>>>>>     	struct intel_crtc *intel_crtc = to_intel_crtc(pipe_config->base.crtc);
>>>>>>     	struct intel_connector *intel_connector = intel_dp->attached_connector;
>>>>>> @@ -1642,6 +1643,15 @@ intel_dp_compute_config(struct intel_encoder *encoder,
>>>>>>     	if (HAS_PCH_SPLIT(dev_priv) && !HAS_DDI(dev_priv) && port != PORT_A)
>>>>>>     		pipe_config->has_pch_encoder = true;
>>>>>>     
>>>>>> +	if (lspcon->active) {
>>>>>> +		struct drm_connector *connector = &intel_connector->base;
>>>>>> +
>>>>>> +		if (lspcon_ycbcr420_config(connector, pipe_config)) {
>>>>>> +			pipe_config->output_format = CRTC_OUTPUT_YCBCR444;
>>>>> I think I'd like to see all compute_config hooks explicitly set the
>>>>> outout_format (to RGB usually obviously).
>>>> That's the one I was talking about in previous patch. If we keep
>>>> output_format_RGB = 0, we need
>>>> not to do this, as reset of the pipe_config will automatically make it RGB.
>>> IMO either we have INVALID=0 so that we use it to catch readout
>>> fails, or we have no INVALID. Other options make little sense to me.
>> Its a minor thing, and doesn't really matter. I can change this.
>>>>>> +			lspcon->output_format = CRTC_OUTPUT_YCBCR420;
>>>>> You should not modify any non-atomic state like that in compute_config.
>>>> Please help me to understand this better, Can you elaborate a bit more on:
>>>> - Why is this a non-atomic state ?
>>>> - What is the right place we should modify it ?
>>> I don't think you need it at all. Just consult the crtc state when
>>> needed.
>> I dont think CRTC state can cover all the cases, for example, what would
>> be do when we need
>> YCBCR 4:4:4 output from LSPCON ? As we have already used crtc_state->444
>> for LSPCON output
>> 420, we can't handle this.
> Like I said earlier, just add another thing to the state to indicate what
> LSPCON should do with the 444 data. Either pass it through or downsample
> it.
>
>> This is equivalent to your previous
>> suggestion of adding 'scaling_reqd',
>> but I added in LPSCON (instead of CRTC state) as this information is
>> required only in case of LSPCON.
> But we have no atomic lspcon state. And probably not worth adding one
> since there won't be much there. So just adding what's needed into the
> crtc state seems like the best approach.
Ok, So I guess the suggestion to move this bool into CRTC state is to 
maintain the atomic flow.
Sure, in that case I will add a new bool in crtc_state called 
"output_scaling_reqd" or something
similar.

- Shashank
>> In all native cases, scaling would be always required for 4:2:0 outputs.
>>
>> - Shashank
>>>> - Shashank
>>>>>> +		}
>>>>>> +	}
>>>>>> +
>>>>>>     	pipe_config->has_drrs = false;
>>>>>>     	if (IS_G4X(dev_priv) || port == PORT_A)
>>>>>>     		pipe_config->has_audio = false;
>>>>>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>>>>>> index 2de6b41..5edba06 100644
>>>>>> --- a/drivers/gpu/drm/i915/intel_drv.h
>>>>>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>>>>>> @@ -2047,6 +2047,8 @@ void lspcon_set_infoframes(struct drm_encoder *encoder,
>>>>>>     			   const struct drm_connector_state *conn_state);
>>>>>>     bool lspcon_infoframe_enabled(struct drm_encoder *encoder,
>>>>>>     			      const struct intel_crtc_state *pipe_config);
>>>>>> +bool lspcon_ycbcr420_config(struct drm_connector *connector,
>>>>>> +			     struct intel_crtc_state *config);
>>>>>>     
>>>>>>     /* intel_pipe_crc.c */
>>>>>>     int intel_pipe_crc_create(struct drm_minor *minor);
>>>>>> diff --git a/drivers/gpu/drm/i915/intel_lspcon.c b/drivers/gpu/drm/i915/intel_lspcon.c
>>>>>> index 066ea91..cb88138 100644
>>>>>> --- a/drivers/gpu/drm/i915/intel_lspcon.c
>>>>>> +++ b/drivers/gpu/drm/i915/intel_lspcon.c
>>>>>> @@ -180,6 +180,25 @@ static bool lspcon_wake_native_aux_ch(struct intel_lspcon *lspcon)
>>>>>>     	return true;
>>>>>>     }
>>>>>>     
>>>>>> +bool lspcon_ycbcr420_config(struct drm_connector *connector,
>>>>>> +			     struct intel_crtc_state *config)
>>>>>> +{
>>>>>> +	struct drm_display_info *info = &connector->display_info;
>>>>>> +	struct drm_display_mode *mode = &config->base.adjusted_mode;
>>>>>> +
>>>>>> +	if (drm_mode_is_420_only(info, mode)) {
>>>>>> +		if (!connector->ycbcr_420_allowed) {
>>>>>> +			DRM_ERROR("Platform doesn't support YCBCR420 output\n");
>>>>>> +			return false;
>>>>>> +		}
>>>>>> +
>>>>>> +		config->port_clock /= 2;
>>>>>> +		return true;
>>>>>> +	}
>>>>>> +
>>>>>> +	return false;
>>>>>> +}
>>>>>> +
>>>>>>     static bool lspcon_probe(struct intel_lspcon *lspcon)
>>>>>>     {
>>>>>>     	int retry;
>>>>>> @@ -459,6 +478,13 @@ void lspcon_set_infoframes(struct drm_encoder *encoder,
>>>>>>     		return;
>>>>>>     	}
>>>>>>     
>>>>>> +	if (lspcon->output_format == CRTC_OUTPUT_YCBCR420)
>>>>>> +		frame.avi.colorspace = HDMI_COLORSPACE_YUV420;
>>>>>> +	else if (lspcon->output_format == CRTC_OUTPUT_YCBCR444)
>>>>>> +		frame.avi.colorspace = HDMI_COLORSPACE_YUV444;
>>>>>> +	else
>>>>>> +		frame.avi.colorspace = HDMI_COLORSPACE_RGB;
>>>>>> +
>>>>>>     	drm_hdmi_avi_infoframe_quant_range(&frame.avi, mode,
>>>>>>     					   crtc_state->limited_color_range ?
>>>>>>     					   HDMI_QUANTIZATION_RANGE_LIMITED :
>>>>>> @@ -512,6 +538,7 @@ bool lspcon_init(struct intel_digital_port *intel_dig_port)
>>>>>>     	struct intel_lspcon *lspcon = &intel_dig_port->lspcon;
>>>>>>     	struct drm_device *dev = intel_dig_port->base.base.dev;
>>>>>>     	struct drm_i915_private *dev_priv = to_i915(dev);
>>>>>> +	struct drm_connector *connector = &dp->attached_connector->base;
>>>>>>     
>>>>>>     	if (!HAS_LSPCON(dev_priv)) {
>>>>>>     		DRM_ERROR("LSPCON is not supported on this platform\n");
>>>>>> @@ -536,6 +563,7 @@ bool lspcon_init(struct intel_digital_port *intel_dig_port)
>>>>>>     		return false;
>>>>>>     	}
>>>>>>     
>>>>>> +	connector->ycbcr_420_allowed = true;
>>>>>>     	lspcon->active = true;
>>>>>>     	DRM_DEBUG_KMS("Success: LSPCON init\n");
>>>>>>     	return true;
>>>>>> -- 
>>>>>> 2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 7/7] drm/i915: Add YCBCR 4:2:0/4:4:4 support for LSPCON

[Sharma, Shashank]

Regards

Shashank


On 1/18/2018 3:00 PM, Maarten Lankhorst wrote:
> Op 18-01-18 om 07:27 schreef Sharma, Shashank:
>> Regards
>>
>> Shashank
>>
>>
>> On 1/17/2018 11:57 PM, Ville Syrjälä wrote:
>>> On Fri, Jan 05, 2018 at 03:15:35PM +0530, Shashank Sharma wrote:
>>>> LSPCON chips can generate YCBCR outputs, if asked nicely :).
>>>>
>>>> In order to generate YCBCR 4:2:0 outputs, a source must:
>>>> - send YCBCR 4:4:4 signals to LSPCON
>>>> - program color space as 4:2:0 in AVI infoframes
>>>>
>>>> Whereas for YCBCR 4:4:4 outputs, the source must:
>>>> - send YCBCR 4:4:4 signals to LSPCON
>>>> - program color space as 4:4:4 in AVI infoframes
>>>>
>>>> So for both 4:2:0 as well as 4:4:4 outputs, we are driving the
>>>> pipe for YCBCR 4:4:4 output, but AVI infoframe's color space
>>>> information indicates LSPCON FW to start scaling down from YCBCR
>>>> 4:4:4 and generate YCBCR 4:2:0 output. As the scaling is done by
>>>> LSPCON device, we need not to reserve a scaler for 4:2:0 outputs.
>>>>
>>>> V2: rebase
>>>> V3: Addressed review comments from Ville
>>>>       - add enum crtc_output_format instead of bool ycbcr420
>>>>       - use crtc_output_format=4:4:4 for modeset of LSPCON 4:2:0 output
>>>>         cases in this way we will have YCBCR 4:4:4 framework ready (except
>>>>         the ABI part)
>>>>
>>>> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
>>>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
>>>> ---
>>>>    drivers/gpu/drm/i915/i915_reg.h     |  2 ++
>>>>    drivers/gpu/drm/i915/intel_ddi.c    |  7 +++++++
>>>>    drivers/gpu/drm/i915/intel_dp.c     | 10 ++++++++++
>>>>    drivers/gpu/drm/i915/intel_drv.h    |  2 ++
>>>>    drivers/gpu/drm/i915/intel_lspcon.c | 28 ++++++++++++++++++++++++++++
>>>>    5 files changed, 49 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>>>> index 966e4df..45ee264 100644
>>>> --- a/drivers/gpu/drm/i915/i915_reg.h
>>>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>>>> @@ -8547,6 +8547,8 @@ enum skl_power_gate {
>>>>    #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        (2<<3)
>>>>    #define  TRANS_MSA_6_BPC        (0<<5)
>>>>    #define  TRANS_MSA_8_BPC        (1<<5)
>>>>    #define  TRANS_MSA_10_BPC        (2<<5)
>>>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
>>>> index 7b89f2a..7616f6f 100644
>>>> --- a/drivers/gpu/drm/i915/intel_ddi.c
>>>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
>>>> @@ -1499,6 +1499,13 @@ void intel_ddi_set_pipe_settings(const struct intel_crtc_state *crtc_state)
>>>>            break;
>>>>        }
>>>>    +    /*
>>>> +     * As per DP 1.2 spec section 2.3.4.3 while sending
>>>> +     * YCBCR 444 signals we should program MSA MISC1/0 fields with
>>>> +     * colorspace information.
>>>> +     */
>>>> +    if (crtc_state->output_format == CRTC_OUTPUT_YCBCR444)
>>>> +        temp |= TRANS_MSA_SAMPLING_444 | TRANS_MSA_CLRSP_YCBCR;
>>> This fails to state that we're indicating BT.601 encoding. I think we
>>> should spell that out explicitly.
>> Agree, I will add this in comments.
>>>>        I915_WRITE(TRANS_MSA_MISC(cpu_transcoder), temp);
>>>>    }
>>>>    diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>>>> index 35c5299..3bf82ea 100644
>>>> --- a/drivers/gpu/drm/i915/intel_dp.c
>>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>>>> @@ -1613,6 +1613,7 @@ intel_dp_compute_config(struct intel_encoder *encoder,
>>>>        struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>>>>        struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode;
>>>>        struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
>>>> +    struct intel_lspcon *lspcon = enc_to_intel_lspcon(&encoder->base);
>>>>        enum port port = encoder->port;
>>>>        struct intel_crtc *intel_crtc = to_intel_crtc(pipe_config->base.crtc);
>>>>        struct intel_connector *intel_connector = intel_dp->attached_connector;
>>>> @@ -1642,6 +1643,15 @@ intel_dp_compute_config(struct intel_encoder *encoder,
>>>>        if (HAS_PCH_SPLIT(dev_priv) && !HAS_DDI(dev_priv) && port != PORT_A)
>>>>            pipe_config->has_pch_encoder = true;
>>>>    +    if (lspcon->active) {
>>>> +        struct drm_connector *connector = &intel_connector->base;
>>>> +
>>>> +        if (lspcon_ycbcr420_config(connector, pipe_config)) {
>>>> +            pipe_config->output_format = CRTC_OUTPUT_YCBCR444;
>>> I think I'd like to see all compute_config hooks explicitly set the
>>> outout_format (to RGB usually obviously).
>> That's the one I was talking about in previous patch. If we keep output_format_RGB = 0, we need
>> not to do this, as reset of the pipe_config will automatically make it RGB.
>>>> +            lspcon->output_format = CRTC_OUTPUT_YCBCR420;
>>> You should not modify any non-atomic state like that in compute_config.
>> Please help me to understand this better, Can you elaborate a bit more on:
>> - Why is this a non-atomic state ?
> Because lspcon->output_format is modified even if a commit is TEST_ONLY..
>
> This will break if you do a nonblocking modeset vs TEST_ONLY, it could commit the wrong lspcon->output_format. :)
>> - What is the right place we should modify it ?
> intel_digital_connector_state probably.
>
> ~Maarten
Thanks for the explanation Maarten, this was very helpful :-).
- Shashank
>> - Shashank
>>>> +        }
>>>> +    }
>>>> +
>>>>        pipe_config->has_drrs = false;
>>>>        if (IS_G4X(dev_priv) || port == PORT_A)
>>>>            pipe_config->has_audio = false;
>>>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>>>> index 2de6b41..5edba06 100644
>>>> --- a/drivers/gpu/drm/i915/intel_drv.h
>>>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>>>> @@ -2047,6 +2047,8 @@ void lspcon_set_infoframes(struct drm_encoder *encoder,
>>>>                   const struct drm_connector_state *conn_state);
>>>>    bool lspcon_infoframe_enabled(struct drm_encoder *encoder,
>>>>                      const struct intel_crtc_state *pipe_config);
>>>> +bool lspcon_ycbcr420_config(struct drm_connector *connector,
>>>> +                 struct intel_crtc_state *config);
>>>>      /* intel_pipe_crc.c */
>>>>    int intel_pipe_crc_create(struct drm_minor *minor);
>>>> diff --git a/drivers/gpu/drm/i915/intel_lspcon.c b/drivers/gpu/drm/i915/intel_lspcon.c
>>>> index 066ea91..cb88138 100644
>>>> --- a/drivers/gpu/drm/i915/intel_lspcon.c
>>>> +++ b/drivers/gpu/drm/i915/intel_lspcon.c
>>>> @@ -180,6 +180,25 @@ static bool lspcon_wake_native_aux_ch(struct intel_lspcon *lspcon)
>>>>        return true;
>>>>    }
>>>>    +bool lspcon_ycbcr420_config(struct drm_connector *connector,
>>>> +                 struct intel_crtc_state *config)
>>>> +{
>>>> +    struct drm_display_info *info = &connector->display_info;
>>>> +    struct drm_display_mode *mode = &config->base.adjusted_mode;
>>>> +
>>>> +    if (drm_mode_is_420_only(info, mode)) {
>>>> +        if (!connector->ycbcr_420_allowed) {
>>>> +            DRM_ERROR("Platform doesn't support YCBCR420 output\n");
>>>> +            return false;
>>>> +        }
>>>> +
>>>> +        config->port_clock /= 2;
>>>> +        return true;
>>>> +    }
>>>> +
>>>> +    return false;
>>>> +}
>>>> +
>>>>    static bool lspcon_probe(struct intel_lspcon *lspcon)
>>>>    {
>>>>        int retry;
>>>> @@ -459,6 +478,13 @@ void lspcon_set_infoframes(struct drm_encoder *encoder,
>>>>            return;
>>>>        }
>>>>    +    if (lspcon->output_format == CRTC_OUTPUT_YCBCR420)
>>>> +        frame.avi.colorspace = HDMI_COLORSPACE_YUV420;
>>>> +    else if (lspcon->output_format == CRTC_OUTPUT_YCBCR444)
>>>> +        frame.avi.colorspace = HDMI_COLORSPACE_YUV444;
>>>> +    else
>>>> +        frame.avi.colorspace = HDMI_COLORSPACE_RGB;
>>>> +
>>>>        drm_hdmi_avi_infoframe_quant_range(&frame.avi, mode,
>>>>                           crtc_state->limited_color_range ?
>>>>                           HDMI_QUANTIZATION_RANGE_LIMITED :
>>>> @@ -512,6 +538,7 @@ bool lspcon_init(struct intel_digital_port *intel_dig_port)
>>>>        struct intel_lspcon *lspcon = &intel_dig_port->lspcon;
>>>>        struct drm_device *dev = intel_dig_port->base.base.dev;
>>>>        struct drm_i915_private *dev_priv = to_i915(dev);
>>>> +    struct drm_connector *connector = &dp->attached_connector->base;
>>>>          if (!HAS_LSPCON(dev_priv)) {
>>>>            DRM_ERROR("LSPCON is not supported on this platform\n");
>>>> @@ -536,6 +563,7 @@ bool lspcon_init(struct intel_digital_port *intel_dig_port)
>>>>            return false;
>>>>        }
>>>>    +    connector->ycbcr_420_allowed = true;
>>>>        lspcon->active = true;
>>>>        DRM_DEBUG_KMS("Success: LSPCON init\n");
>>>>        return true;
>>>> -- 
>>>> 2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx