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

[PATCH v4 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 sent V3, and now V4 to address additional comments.

Sharma, Shashank (6):
  drm/i915: Add CRTC output format YCBCR 4:2:0
  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

Shashank Sharma (1):
  drm/i915: Add CRTC output format YCBCR 4:4:4

 drivers/gpu/drm/i915/i915_reg.h      |   2 +
 drivers/gpu/drm/i915/intel_color.c   |   3 +-
 drivers/gpu/drm/i915/intel_ddi.c     |  30 ++-
 drivers/gpu/drm/i915/intel_display.c |  83 ++++++---
 drivers/gpu/drm/i915/intel_dp.c      |  10 +
 drivers/gpu/drm/i915/intel_drv.h     |  39 +++-
 drivers/gpu/drm/i915/intel_hdmi.c    |  23 ++-
 drivers/gpu/drm/i915/intel_lspcon.c  | 345 +++++++++++++++++++++++++++++++++--
 drivers/gpu/drm/i915/intel_panel.c   |   2 +-
 9 files changed, 485 insertions(+), 52 deletions(-)

-- 
2.7.4

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

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

[Shashank Sharma]

From: "Sharma, Shashank" <shashank.sharma@intel.com>

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.
V4: Added r-b from Maarten (on v3)
    Addressed review comments from Ville:
	- Change the enum name to intel_output_format from crtc_output_format.
	- Start the enum value (INVALID) from 0 instaed of 1.
        - Set the crtc's output_format to RGB in encoder's compute_config.

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>
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_color.c   |  2 +-
 drivers/gpu/drm/i915/intel_ddi.c     |  4 ++-
 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, 61 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 e51559b..d565e28 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)
@@ -2759,6 +2759,8 @@ static bool intel_ddi_compute_config(struct intel_encoder *encoder,
 	if (port == PORT_A)
 		pipe_config->cpu_transcoder = TRANSCODER_EDP;
 
+	pipe_config->output_format = CRTC_OUTPUT_RGB;
+
 	if (intel_crtc_has_type(pipe_config, INTEL_OUTPUT_HDMI))
 		ret = intel_hdmi_compute_config(encoder, pipe_config, conn_state);
 	else
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 83de43c..877336d 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4650,7 +4650,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;
 
 	/*
@@ -6362,7 +6363,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
@@ -8192,10 +8194,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);
@@ -9171,6 +9173,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 intel_output_format format)
+{
+	if (format < CRTC_OUTPUT_INVALID || format > CRTC_OUTPUT_YCBCR420)
+		format = CRTC_OUTPUT_INVALID;
+	return output_format_str[format];
+}
+
 static bool haswell_get_pipe_config(struct intel_crtc *crtc,
 				    struct intel_crtc_state *pipe_config)
 {
@@ -9211,19 +9226,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 intel_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);
@@ -10578,6 +10602,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);
@@ -10587,9 +10614,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);
@@ -11163,6 +11187,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))
@@ -11171,7 +11196,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 3cee54b..35be78e 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -687,6 +687,12 @@ struct intel_crtc_wm_state {
 	bool need_postvbl_update;
 };
 
+enum intel_output_format {
+	CRTC_OUTPUT_INVALID,
+	CRTC_OUTPUT_RGB,
+	CRTC_OUTPUT_YCBCR420,
+};
+
 struct intel_crtc_state {
 	struct drm_crtc_state base;
 
@@ -873,8 +879,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 intel_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 978f22b..6700ed6 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -479,7 +479,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;
@@ -1615,7 +1615,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))
@@ -1650,7 +1650,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 e702a64..17025bc 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 v4 1/7] drm/i915: Add CRTC output format YCBCR 4:2:0

[Jani Nikula]

On Tue, 30 Jan 2018, Shashank Sharma <shashank.sharma@intel.com> wrote:
> From: "Sharma, Shashank" <shashank.sharma@intel.com>
>
> 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.
> V4: Added r-b from Maarten (on v3)
>     Addressed review comments from Ville:
> 	- Change the enum name to intel_output_format from crtc_output_format.
> 	- Start the enum value (INVALID) from 0 instaed of 1.
>         - Set the crtc's output_format to RGB in encoder's compute_config.
>
> 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>
> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_color.c   |  2 +-
>  drivers/gpu/drm/i915/intel_ddi.c     |  4 ++-
>  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, 61 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 e51559b..d565e28 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)
> @@ -2759,6 +2759,8 @@ static bool intel_ddi_compute_config(struct intel_encoder *encoder,
>  	if (port == PORT_A)
>  		pipe_config->cpu_transcoder = TRANSCODER_EDP;
>  
> +	pipe_config->output_format = CRTC_OUTPUT_RGB;
> +
>  	if (intel_crtc_has_type(pipe_config, INTEL_OUTPUT_HDMI))
>  		ret = intel_hdmi_compute_config(encoder, pipe_config, conn_state);
>  	else
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 83de43c..877336d 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4650,7 +4650,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;
>  
>  	/*
> @@ -6362,7 +6363,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
> @@ -8192,10 +8194,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);
> @@ -9171,6 +9173,19 @@ static void haswell_get_ddi_port_state(struct intel_crtc *crtc,
>  	}
>  }
>  
> +static const char * const output_format_str[] = {
> +	"Invalid",
> +	"RGB",
> +	"YCBCR4:2:0",

This should really be:

	[CRTC_OUTPUT_INVALID] = "Invalid",
	[CRTC_OUTPUT_RGB] = "RGB",
	[CRTC_OUTPUT_YCBCR420] = "YCBCR4:2:0",

BR,
Jani.

> +};
> +
> +static const char *output_formats(enum intel_output_format format)
> +{
> +	if (format < CRTC_OUTPUT_INVALID || format > CRTC_OUTPUT_YCBCR420)
> +		format = CRTC_OUTPUT_INVALID;
> +	return output_format_str[format];
> +}
> +
>  static bool haswell_get_pipe_config(struct intel_crtc *crtc,
>  				    struct intel_crtc_state *pipe_config)
>  {
> @@ -9211,19 +9226,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 intel_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);
> @@ -10578,6 +10602,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);
> @@ -10587,9 +10614,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);
> @@ -11163,6 +11187,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))
> @@ -11171,7 +11196,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 3cee54b..35be78e 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -687,6 +687,12 @@ struct intel_crtc_wm_state {
>  	bool need_postvbl_update;
>  };
>  
> +enum intel_output_format {
> +	CRTC_OUTPUT_INVALID,
> +	CRTC_OUTPUT_RGB,
> +	CRTC_OUTPUT_YCBCR420,
> +};
> +
>  struct intel_crtc_state {
>  	struct drm_crtc_state base;
>  
> @@ -873,8 +879,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 intel_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 978f22b..6700ed6 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -479,7 +479,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;
> @@ -1615,7 +1615,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))
> @@ -1650,7 +1650,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 e702a64..17025bc 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) {

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Sharma, Shashank]

Regards

Shashank


On 1/30/2018 3:53 PM, Jani Nikula wrote:
> On Tue, 30 Jan 2018, Shashank Sharma <shashank.sharma@intel.com> wrote:
>> From: "Sharma, Shashank" <shashank.sharma@intel.com>
>>
>> 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.
>> V4: Added r-b from Maarten (on v3)
>>      Addressed review comments from Ville:
>> 	- Change the enum name to intel_output_format from crtc_output_format.
>> 	- Start the enum value (INVALID) from 0 instaed of 1.
>>          - Set the crtc's output_format to RGB in encoder's compute_config.
>>
>> 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>
>> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_color.c   |  2 +-
>>   drivers/gpu/drm/i915/intel_ddi.c     |  4 ++-
>>   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, 61 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 e51559b..d565e28 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)
>> @@ -2759,6 +2759,8 @@ static bool intel_ddi_compute_config(struct intel_encoder *encoder,
>>   	if (port == PORT_A)
>>   		pipe_config->cpu_transcoder = TRANSCODER_EDP;
>>   
>> +	pipe_config->output_format = CRTC_OUTPUT_RGB;
>> +
>>   	if (intel_crtc_has_type(pipe_config, INTEL_OUTPUT_HDMI))
>>   		ret = intel_hdmi_compute_config(encoder, pipe_config, conn_state);
>>   	else
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 83de43c..877336d 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -4650,7 +4650,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;
>>   
>>   	/*
>> @@ -6362,7 +6363,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
>> @@ -8192,10 +8194,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);
>> @@ -9171,6 +9173,19 @@ static void haswell_get_ddi_port_state(struct intel_crtc *crtc,
>>   	}
>>   }
>>   
>> +static const char * const output_format_str[] = {
>> +	"Invalid",
>> +	"RGB",
>> +	"YCBCR4:2:0",
> This should really be:
>
> 	[CRTC_OUTPUT_INVALID] = "Invalid",
> 	[CRTC_OUTPUT_RGB] = "RGB",
> 	[CRTC_OUTPUT_YCBCR420] = "YCBCR4:2:0",
Hey sure, thanks. Just after the comment I realized that now when have 
started the enum from 0 (instead of -1) I can do this.
- Shashank
>
> BR,
> Jani.
>
>> +};
>> +
>> +static const char *output_formats(enum intel_output_format format)
>> +{
>> +	if (format < CRTC_OUTPUT_INVALID || format > CRTC_OUTPUT_YCBCR420)
>> +		format = CRTC_OUTPUT_INVALID;
>> +	return output_format_str[format];
>> +}
>> +
>>   static bool haswell_get_pipe_config(struct intel_crtc *crtc,
>>   				    struct intel_crtc_state *pipe_config)
>>   {
>> @@ -9211,19 +9226,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 intel_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);
>> @@ -10578,6 +10602,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);
>> @@ -10587,9 +10614,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);
>> @@ -11163,6 +11187,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))
>> @@ -11171,7 +11196,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 3cee54b..35be78e 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -687,6 +687,12 @@ struct intel_crtc_wm_state {
>>   	bool need_postvbl_update;
>>   };
>>   
>> +enum intel_output_format {
>> +	CRTC_OUTPUT_INVALID,
>> +	CRTC_OUTPUT_RGB,
>> +	CRTC_OUTPUT_YCBCR420,
>> +};
>> +
>>   struct intel_crtc_state {
>>   	struct drm_crtc_state base;
>>   
>> @@ -873,8 +879,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 intel_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 978f22b..6700ed6 100644
>> --- a/drivers/gpu/drm/i915/intel_hdmi.c
>> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
>> @@ -479,7 +479,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;
>> @@ -1615,7 +1615,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))
>> @@ -1650,7 +1650,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 e702a64..17025bc 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) {

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

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

[Ville Syrjälä]

On Tue, Jan 30, 2018 at 03:05:57PM +0530, Shashank Sharma wrote:
> From: "Sharma, Shashank" <shashank.sharma@intel.com>
> 
> 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.
> V4: Added r-b from Maarten (on v3)
>     Addressed review comments from Ville:
> 	- Change the enum name to intel_output_format from crtc_output_format.
> 	- Start the enum value (INVALID) from 0 instaed of 1.
>         - Set the crtc's output_format to RGB in encoder's compute_config.
> 
> 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>
> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_color.c   |  2 +-
>  drivers/gpu/drm/i915/intel_ddi.c     |  4 ++-
>  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, 61 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 e51559b..d565e28 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)
> @@ -2759,6 +2759,8 @@ static bool intel_ddi_compute_config(struct intel_encoder *encoder,
>  	if (port == PORT_A)
>  		pipe_config->cpu_transcoder = TRANSCODER_EDP;
>  
> +	pipe_config->output_format = CRTC_OUTPUT_RGB;

We seem to be missing this for most platforms/encoder types.

> +
>  	if (intel_crtc_has_type(pipe_config, INTEL_OUTPUT_HDMI))
>  		ret = intel_hdmi_compute_config(encoder, pipe_config, conn_state);
>  	else
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 83de43c..877336d 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4650,7 +4650,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;
>  
>  	/*
> @@ -6362,7 +6363,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
> @@ -8192,10 +8194,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;

Why two |= ?

>  		}
>  
>  		I915_WRITE(PIPEMISC(intel_crtc->pipe), val);
> @@ -9171,6 +9173,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 intel_output_format format)
> +{
> +	if (format < CRTC_OUTPUT_INVALID || format > CRTC_OUTPUT_YCBCR420)
> +		format = CRTC_OUTPUT_INVALID;
> +	return output_format_str[format];
> +}
> +
>  static bool haswell_get_pipe_config(struct intel_crtc *crtc,
>  				    struct intel_crtc_state *pipe_config)
>  {
> @@ -9211,19 +9226,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 intel_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));

Useless debug print.

>  	}
>  
>  	power_domain = POWER_DOMAIN_PIPE_PANEL_FITTER(crtc->pipe);
> @@ -10578,6 +10602,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);
> @@ -10587,9 +10614,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);
> @@ -11163,6 +11187,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))
> @@ -11171,7 +11196,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 3cee54b..35be78e 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -687,6 +687,12 @@ struct intel_crtc_wm_state {
>  	bool need_postvbl_update;
>  };
>  
> +enum intel_output_format {
> +	CRTC_OUTPUT_INVALID,
> +	CRTC_OUTPUT_RGB,
> +	CRTC_OUTPUT_YCBCR420,
> +};
> +
>  struct intel_crtc_state {
>  	struct drm_crtc_state base;
>  
> @@ -873,8 +879,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 intel_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 978f22b..6700ed6 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -479,7 +479,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;
> @@ -1615,7 +1615,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))
> @@ -1650,7 +1650,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 e702a64..17025bc 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 v4 1/7] drm/i915: Add CRTC output format YCBCR 4:2:0

[Sharma, Shashank]

Thanks for the comments, mine inline.

Regards
Shashank
On 2/2/2018 12:39 AM, Ville Syrjälä wrote:
> On Tue, Jan 30, 2018 at 03:05:57PM +0530, Shashank Sharma wrote:
>> From: "Sharma, Shashank" <shashank.sharma@intel.com>
>>
>> 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.
>> V4: Added r-b from Maarten (on v3)
>>      Addressed review comments from Ville:
>> 	- Change the enum name to intel_output_format from crtc_output_format.
>> 	- Start the enum value (INVALID) from 0 instaed of 1.
>>          - Set the crtc's output_format to RGB in encoder's compute_config.
>>
>> 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>
>> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_color.c   |  2 +-
>>   drivers/gpu/drm/i915/intel_ddi.c     |  4 ++-
>>   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, 61 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 e51559b..d565e28 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)
>> @@ -2759,6 +2759,8 @@ static bool intel_ddi_compute_config(struct intel_encoder *encoder,
>>   	if (port == PORT_A)
>>   		pipe_config->cpu_transcoder = TRANSCODER_EDP;
>>   
>> +	pipe_config->output_format = CRTC_OUTPUT_RGB;
> We seem to be missing this for most platforms/encoder types.
I thought this would be required only for DDI interfaces, do you suggest 
we should set this per encoder basis ?
>
>> +
>>   	if (intel_crtc_has_type(pipe_config, INTEL_OUTPUT_HDMI))
>>   		ret = intel_hdmi_compute_config(encoder, pipe_config, conn_state);
>>   	else
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 83de43c..877336d 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -4650,7 +4650,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;
>>   
>>   	/*
>> @@ -6362,7 +6363,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
>> @@ -8192,10 +8194,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;
> Why two |= ?
Wanna make it clear that first set is for all YCBCR outputs(420 and 444) 
whereas other two are explicitly for 420.
>>   		}
>>   
>>   		I915_WRITE(PIPEMISC(intel_crtc->pipe), val);
>> @@ -9171,6 +9173,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 intel_output_format format)
>> +{
>> +	if (format < CRTC_OUTPUT_INVALID || format > CRTC_OUTPUT_YCBCR420)
>> +		format = CRTC_OUTPUT_INVALID;
>> +	return output_format_str[format];
>> +}
>> +
>>   static bool haswell_get_pipe_config(struct intel_crtc *crtc,
>>   				    struct intel_crtc_state *pipe_config)
>>   {
>> @@ -9211,19 +9226,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 intel_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));
> Useless debug print.
Its a _kms only, but can be removed if you think so.
- Shashank
>>   	}
>>   
>>   	power_domain = POWER_DOMAIN_PIPE_PANEL_FITTER(crtc->pipe);
>> @@ -10578,6 +10602,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);
>> @@ -10587,9 +10614,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);
>> @@ -11163,6 +11187,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))
>> @@ -11171,7 +11196,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 3cee54b..35be78e 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -687,6 +687,12 @@ struct intel_crtc_wm_state {
>>   	bool need_postvbl_update;
>>   };
>>   
>> +enum intel_output_format {
>> +	CRTC_OUTPUT_INVALID,
>> +	CRTC_OUTPUT_RGB,
>> +	CRTC_OUTPUT_YCBCR420,
>> +};
>> +
>>   struct intel_crtc_state {
>>   	struct drm_crtc_state base;
>>   
>> @@ -873,8 +879,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 intel_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 978f22b..6700ed6 100644
>> --- a/drivers/gpu/drm/i915/intel_hdmi.c
>> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
>> @@ -479,7 +479,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;
>> @@ -1615,7 +1615,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))
>> @@ -1650,7 +1650,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 e702a64..17025bc 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 v4 1/7] drm/i915: Add CRTC output format YCBCR 4:2:0

[Ville Syrjälä]

On Fri, Feb 02, 2018 at 11:38:07AM +0530, Sharma, Shashank wrote:
> Thanks for the comments, mine inline.
> 
> Regards
> Shashank
> On 2/2/2018 12:39 AM, Ville Syrjälä wrote:
> > On Tue, Jan 30, 2018 at 03:05:57PM +0530, Shashank Sharma wrote:
> >> From: "Sharma, Shashank" <shashank.sharma@intel.com>
> >>
> >> 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.
> >> V4: Added r-b from Maarten (on v3)
> >>      Addressed review comments from Ville:
> >> 	- Change the enum name to intel_output_format from crtc_output_format.
> >> 	- Start the enum value (INVALID) from 0 instaed of 1.
> >>          - Set the crtc's output_format to RGB in encoder's compute_config.
> >>
> >> 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>
> >> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >> ---
> >>   drivers/gpu/drm/i915/intel_color.c   |  2 +-
> >>   drivers/gpu/drm/i915/intel_ddi.c     |  4 ++-
> >>   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, 61 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 e51559b..d565e28 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)
> >> @@ -2759,6 +2759,8 @@ static bool intel_ddi_compute_config(struct intel_encoder *encoder,
> >>   	if (port == PORT_A)
> >>   		pipe_config->cpu_transcoder = TRANSCODER_EDP;
> >>   
> >> +	pipe_config->output_format = CRTC_OUTPUT_RGB;
> > We seem to be missing this for most platforms/encoder types.
> I thought this would be required only for DDI interfaces, do you suggest 
> we should set this per encoder basis ?

Yes, I'd like to see every encoder set it. Otherwise our state dumps
will contain invalid information.

> >
> >> +
> >>   	if (intel_crtc_has_type(pipe_config, INTEL_OUTPUT_HDMI))
> >>   		ret = intel_hdmi_compute_config(encoder, pipe_config, conn_state);
> >>   	else
> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >> index 83de43c..877336d 100644
> >> --- a/drivers/gpu/drm/i915/intel_display.c
> >> +++ b/drivers/gpu/drm/i915/intel_display.c
> >> @@ -4650,7 +4650,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;
> >>   
> >>   	/*
> >> @@ -6362,7 +6363,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
> >> @@ -8192,10 +8194,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;
> > Why two |= ?
> Wanna make it clear that first set is for all YCBCR outputs(420 and 444) 
> whereas other two are explicitly for 420.

Fair enough.

> >>   		}
> >>   
> >>   		I915_WRITE(PIPEMISC(intel_crtc->pipe), val);
> >> @@ -9171,6 +9173,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 intel_output_format format)
> >> +{
> >> +	if (format < CRTC_OUTPUT_INVALID || format > CRTC_OUTPUT_YCBCR420)
> >> +		format = CRTC_OUTPUT_INVALID;

BTW >= ARRAY_SIZE(format_str)

would seem like a better check here.

> >> +	return output_format_str[format];
> >> +}
> >> +
> >>   static bool haswell_get_pipe_config(struct intel_crtc *crtc,
> >>   				    struct intel_crtc_state *pipe_config)
> >>   {
> >> @@ -9211,19 +9226,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 intel_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));
> > Useless debug print.
> Its a _kms only, but can be removed if you think so.

Yes, we can't have everyone pollute the dmesg with their favorite detail
of the day. That'll lead to the whole thing being mostly noise.

I think the important stuff we want to print is mostly to do with 
error conditions and the modeset sequencing. Detais about the
crtc/plane/etc. states should be covered by the state dumps.

> - Shashank
> >>   	}
> >>   
> >>   	power_domain = POWER_DOMAIN_PIPE_PANEL_FITTER(crtc->pipe);
> >> @@ -10578,6 +10602,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);
> >> @@ -10587,9 +10614,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);
> >> @@ -11163,6 +11187,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))
> >> @@ -11171,7 +11196,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 3cee54b..35be78e 100644
> >> --- a/drivers/gpu/drm/i915/intel_drv.h
> >> +++ b/drivers/gpu/drm/i915/intel_drv.h
> >> @@ -687,6 +687,12 @@ struct intel_crtc_wm_state {
> >>   	bool need_postvbl_update;
> >>   };
> >>   
> >> +enum intel_output_format {
> >> +	CRTC_OUTPUT_INVALID,
> >> +	CRTC_OUTPUT_RGB,
> >> +	CRTC_OUTPUT_YCBCR420,
> >> +};
> >> +
> >>   struct intel_crtc_state {
> >>   	struct drm_crtc_state base;
> >>   
> >> @@ -873,8 +879,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 intel_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 978f22b..6700ed6 100644
> >> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> >> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> >> @@ -479,7 +479,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;
> >> @@ -1615,7 +1615,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))
> >> @@ -1650,7 +1650,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 e702a64..17025bc 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 v4 1/7] drm/i915: Add CRTC output format YCBCR 4:2:0

[Sharma, Shashank]

Regards

Shashank


On 2/2/2018 7:30 PM, Ville Syrjälä wrote:
> On Fri, Feb 02, 2018 at 11:38:07AM +0530, Sharma, Shashank wrote:
>> Thanks for the comments, mine inline.
>>
>> Regards
>> Shashank
>> On 2/2/2018 12:39 AM, Ville Syrjälä wrote:
>>> On Tue, Jan 30, 2018 at 03:05:57PM +0530, Shashank Sharma wrote:
>>>> From: "Sharma, Shashank" <shashank.sharma@intel.com>
>>>>
>>>> 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.
>>>> V4: Added r-b from Maarten (on v3)
>>>>       Addressed review comments from Ville:
>>>> 	- Change the enum name to intel_output_format from crtc_output_format.
>>>> 	- Start the enum value (INVALID) from 0 instaed of 1.
>>>>           - Set the crtc's output_format to RGB in encoder's compute_config.
>>>>
>>>> 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>
>>>> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>> ---
>>>>    drivers/gpu/drm/i915/intel_color.c   |  2 +-
>>>>    drivers/gpu/drm/i915/intel_ddi.c     |  4 ++-
>>>>    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, 61 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 e51559b..d565e28 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)
>>>> @@ -2759,6 +2759,8 @@ static bool intel_ddi_compute_config(struct intel_encoder *encoder,
>>>>    	if (port == PORT_A)
>>>>    		pipe_config->cpu_transcoder = TRANSCODER_EDP;
>>>>    
>>>> +	pipe_config->output_format = CRTC_OUTPUT_RGB;
>>> We seem to be missing this for most platforms/encoder types.
>> I thought this would be required only for DDI interfaces, do you suggest
>> we should set this per encoder basis ?
> Yes, I'd like to see every encoder set it. Otherwise our state dumps
> will contain invalid information.
Got it.
>>>> +
>>>>    	if (intel_crtc_has_type(pipe_config, INTEL_OUTPUT_HDMI))
>>>>    		ret = intel_hdmi_compute_config(encoder, pipe_config, conn_state);
>>>>    	else
>>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>>>> index 83de43c..877336d 100644
>>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>>> @@ -4650,7 +4650,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;
>>>>    
>>>>    	/*
>>>> @@ -6362,7 +6363,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
>>>> @@ -8192,10 +8194,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;
>>> Why two |= ?
>> Wanna make it clear that first set is for all YCBCR outputs(420 and 444)
>> whereas other two are explicitly for 420.
> Fair enough.
>
>>>>    		}
>>>>    
>>>>    		I915_WRITE(PIPEMISC(intel_crtc->pipe), val);
>>>> @@ -9171,6 +9173,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 intel_output_format format)
>>>> +{
>>>> +	if (format < CRTC_OUTPUT_INVALID || format > CRTC_OUTPUT_YCBCR420)
>>>> +		format = CRTC_OUTPUT_INVALID;
> BTW >= ARRAY_SIZE(format_str)
>
> would seem like a better check here.
Yep, agree.
>>>> +	return output_format_str[format];
>>>> +}
>>>> +
>>>>    static bool haswell_get_pipe_config(struct intel_crtc *crtc,
>>>>    				    struct intel_crtc_state *pipe_config)
>>>>    {
>>>> @@ -9211,19 +9226,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 intel_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));
>>> Useless debug print.
>> Its a _kms only, but can be removed if you think so.
> Yes, we can't have everyone pollute the dmesg with their favorite detail
> of the day. That'll lead to the whole thing being mostly noise.
>
> I think the important stuff we want to print is mostly to do with
> error conditions and the modeset sequencing. Detais about the
> crtc/plane/etc. states should be covered by the state dumps.
Ok, it will be gone.
- Shashank
>> - Shashank
>>>>    	}
>>>>    
>>>>    	power_domain = POWER_DOMAIN_PIPE_PANEL_FITTER(crtc->pipe);
>>>> @@ -10578,6 +10602,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);
>>>> @@ -10587,9 +10614,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);
>>>> @@ -11163,6 +11187,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))
>>>> @@ -11171,7 +11196,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 3cee54b..35be78e 100644
>>>> --- a/drivers/gpu/drm/i915/intel_drv.h
>>>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>>>> @@ -687,6 +687,12 @@ struct intel_crtc_wm_state {
>>>>    	bool need_postvbl_update;
>>>>    };
>>>>    
>>>> +enum intel_output_format {
>>>> +	CRTC_OUTPUT_INVALID,
>>>> +	CRTC_OUTPUT_RGB,
>>>> +	CRTC_OUTPUT_YCBCR420,
>>>> +};
>>>> +
>>>>    struct intel_crtc_state {
>>>>    	struct drm_crtc_state base;
>>>>    
>>>> @@ -873,8 +879,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 intel_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 978f22b..6700ed6 100644
>>>> --- a/drivers/gpu/drm/i915/intel_hdmi.c
>>>> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
>>>> @@ -479,7 +479,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;
>>>> @@ -1615,7 +1615,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))
>>>> @@ -1650,7 +1650,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 e702a64..17025bc 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 v4 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
V4: Added r-b from Maarten (for v3)
    Addressed review comment from Ville:
    Do not use (config->output_format > CRTC_OUTPUT_RGB)

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 | 14 ++++++++++----
 drivers/gpu/drm/i915/intel_drv.h     |  1 +
 drivers/gpu/drm/i915/intel_hdmi.c    |  2 ++
 4 files changed, 15 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 877336d..4a40de9 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6363,8 +6363,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
@@ -8194,11 +8195,13 @@ 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_YCBCR420 ||
+		    config->output_format == CRTC_OUTPUT_YCBCR444)
 			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);
 	}
@@ -9176,6 +9179,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",
 };
 
@@ -9241,6 +9245,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 35be78e..04b38c9 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -690,6 +690,7 @@ struct intel_crtc_wm_state {
 enum intel_output_format {
 	CRTC_OUTPUT_INVALID,
 	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 6700ed6..02c5954 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -481,6 +481,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 v4 3/7] drm/i915: Check LSPCON vendor OUI

[Shashank Sharma]

From: "Sharma, Shashank" <shashank.sharma@intel.com>

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
V3: Rebase

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 04b38c9..f1c23b9 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1133,9 +1133,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 v4 4/7] drm/i915: Add AVI infoframe support for LSPCON

[Shashank Sharma]

From: "Sharma, Shashank" <shashank.sharma@intel.com>

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
V4: Addressed Ville's review comment
    - Do not add output_format in LSPCON state, as its non-atomic. Add
      this into CRTC state (added in a later patch).

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    | 13 +++++++++-
 drivers/gpu/drm/i915/intel_hdmi.c   | 13 +++++++---
 drivers/gpu/drm/i915/intel_lspcon.c | 49 +++++++++++++++++++++++++++++++++++++
 4 files changed, 86 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index d565e28..510bb77 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -2267,10 +2267,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)
@@ -2972,8 +2984,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;
@@ -3003,6 +3013,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 f1c23b9..9182a3f 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1267,6 +1267,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)
 {
@@ -1783,7 +1789,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);
@@ -2124,6 +2129,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 02c5954..f0c5194 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -2286,9 +2286,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 v4 5/7] drm/i915: Write AVI infoframes for MCA LSPCON

[Shashank Sharma]

From: "Sharma, Shashank" <shashank.sharma@intel.com>

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
V4: Rebase

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 9182a3f..efaa331 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -2129,6 +2129,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 f0c5194..a281821 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -2287,6 +2287,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 v4 6/7] drm/i915: Write AVI infoframes for Parade LSPCON

[Shashank Sharma]

From: "Sharma, Shashank" <shashank.sharma@intel.com>

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 v4 7/7] drm/i915: Add YCBCR 4:2:0/4:4:4 support for LSPCON

[Shashank Sharma]

From: "Sharma, Shashank" <shashank.sharma@intel.com>

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)
V4: Added r-b from Maarten (for v3)
    Addressed review comments from Ville:
    - Do not add a non-atomic state variable to determine lspcon output.
      Instead add bool in CRTC state to indicate lspcon based scaling.

Cc: Ville Syrjala <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/i915_reg.h      |  2 ++
 drivers/gpu/drm/i915/intel_ddi.c     |  7 +++++++
 drivers/gpu/drm/i915/intel_display.c | 11 ++++++++++-
 drivers/gpu/drm/i915/intel_dp.c      | 10 ++++++++++
 drivers/gpu/drm/i915/intel_drv.h     |  5 +++++
 drivers/gpu/drm/i915/intel_lspcon.c  | 30 ++++++++++++++++++++++++++++++
 6 files changed, 64 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 64933fd..e383b05 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -8721,6 +8721,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 510bb77..db7d940 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. The output colorspace encoding is BT601.
+	 */
+	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_display.c b/drivers/gpu/drm/i915/intel_display.c
index 4a40de9..b26f4a9 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -9228,6 +9228,7 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
 	pipe_config->gamma_mode =
 		I915_READ(GAMMA_MODE(crtc->pipe)) & GAMMA_MODE_MODE_MASK;
 
+	pipe_config->external_scaling = false;
 	if (IS_BROADWELL(dev_priv) || INTEL_GEN(dev_priv) >= 9) {
 		u32 tmp = I915_READ(PIPEMISC(crtc->pipe));
 		enum intel_output_format output_format = CRTC_OUTPUT_RGB;
@@ -9247,8 +9248,16 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
 					output_format = CRTC_OUTPUT_YCBCR420;
 			} else {
 				output_format = CRTC_OUTPUT_YCBCR444;
-			}
 
+				/*
+				 * As there is no interface defined (yet)
+				 * to get the user's preference for output
+				 * format, YCBCR444 output format is only
+				 * possible with LSPCON YCBCR420 output,
+				 * which uses LSPCON's (external )scaler.
+				 */
+				pipe_config->external_scaling = true;
+			}
 		}
 
 		pipe_config->output_format = output_format;
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index a2e8879..be19579 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1640,6 +1640,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;
@@ -1669,6 +1670,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;
+			pipe_config->external_scaling = true;
+		}
+	}
+
 	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 efaa331..f23129b 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -882,6 +882,9 @@ struct intel_crtc_state {
 
 	/* Output format RGB/YCBCR etc */
 	enum intel_output_format output_format;
+
+	/* Output up/down scaling is done in external device */
+	bool external_scaling;
 };
 
 struct intel_crtc {
@@ -2139,6 +2142,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..8d6a9b2 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,15 @@ void lspcon_set_infoframes(struct drm_encoder *encoder,
 		return;
 	}
 
+	if (crtc_state->output_format == CRTC_OUTPUT_YCBCR444) {
+		if (crtc_state->external_scaling)
+			frame.avi.colorspace = HDMI_COLORSPACE_YUV420;
+		else
+			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 +540,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 +565,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 v4 7/7] drm/i915: Add YCBCR 4:2:0/4:4:4 support for LSPCON

[Ville Syrjälä]

On Tue, Jan 30, 2018 at 03:06:03PM +0530, Shashank Sharma wrote:
> From: "Sharma, Shashank" <shashank.sharma@intel.com>
> 
> 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)
> V4: Added r-b from Maarten (for v3)
>     Addressed review comments from Ville:
>     - Do not add a non-atomic state variable to determine lspcon output.
>       Instead add bool in CRTC state to indicate lspcon based scaling.
> 
> Cc: Ville Syrjala <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/i915_reg.h      |  2 ++
>  drivers/gpu/drm/i915/intel_ddi.c     |  7 +++++++
>  drivers/gpu/drm/i915/intel_display.c | 11 ++++++++++-
>  drivers/gpu/drm/i915/intel_dp.c      | 10 ++++++++++
>  drivers/gpu/drm/i915/intel_drv.h     |  5 +++++
>  drivers/gpu/drm/i915/intel_lspcon.c  | 30 ++++++++++++++++++++++++++++++
>  6 files changed, 64 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 64933fd..e383b05 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -8721,6 +8721,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 510bb77..db7d940 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. The output colorspace encoding is BT601.
> +	 */
> +	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_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 4a40de9..b26f4a9 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -9228,6 +9228,7 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
>  	pipe_config->gamma_mode =
>  		I915_READ(GAMMA_MODE(crtc->pipe)) & GAMMA_MODE_MODE_MASK;
>  
> +	pipe_config->external_scaling = false;

I really don't like the name of that. Makes me think we're actually
scaling the entire output or something. 'external_ycbcr_420_downsample'
or something like that would be better.

>  	if (IS_BROADWELL(dev_priv) || INTEL_GEN(dev_priv) >= 9) {
>  		u32 tmp = I915_READ(PIPEMISC(crtc->pipe));
>  		enum intel_output_format output_format = CRTC_OUTPUT_RGB;
> @@ -9247,8 +9248,16 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
>  					output_format = CRTC_OUTPUT_YCBCR420;
>  			} else {
>  				output_format = CRTC_OUTPUT_YCBCR444;
> -			}
>  
> +				/*
> +				 * As there is no interface defined (yet)
> +				 * to get the user's preference for output
> +				 * format, YCBCR444 output format is only
> +				 * possible with LSPCON YCBCR420 output,
> +				 * which uses LSPCON's (external )scaler.
> +				 */
> +				pipe_config->external_scaling = true;
> +			}
>  		}
>  
>  		pipe_config->output_format = output_format;
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index a2e8879..be19579 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1640,6 +1640,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;
> @@ -1669,6 +1670,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;
> +			pipe_config->external_scaling = true;

Why split the work between two functions like this? I think it would be
cleaner to either inline all the code here, or move it all into
lspcon_ycbcr420_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 efaa331..f23129b 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -882,6 +882,9 @@ struct intel_crtc_state {
>  
>  	/* Output format RGB/YCBCR etc */
>  	enum intel_output_format output_format;
> +
> +	/* Output up/down scaling is done in external device */
> +	bool external_scaling;
>  };
>  
>  struct intel_crtc {
> @@ -2139,6 +2142,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..8d6a9b2 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)

crtc_state

> +{
> +	struct drm_display_info *info = &connector->display_info;

const

> +	struct drm_display_mode *mode = &config->base.adjusted_mode;

const ... 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;

How can we even get here?

> +		}
> +
> +		config->port_clock /= 2;
> +		return true;
> +	}
> +
> +	return false;
> +}
> +
>  static bool lspcon_probe(struct intel_lspcon *lspcon)
>  {
>  	int retry;
> @@ -459,6 +478,15 @@ void lspcon_set_infoframes(struct drm_encoder *encoder,
>  		return;
>  	}
>  
> +	if (crtc_state->output_format == CRTC_OUTPUT_YCBCR444) {
> +		if (crtc_state->external_scaling)
> +			frame.avi.colorspace = HDMI_COLORSPACE_YUV420;
> +		else
> +			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 +540,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 +565,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 v4 7/7] drm/i915: Add YCBCR 4:2:0/4:4:4 support for LSPCON

[Sharma, Shashank]

Regards

Shashank


On 2/2/2018 12:39 AM, Ville Syrjälä wrote:
> On Tue, Jan 30, 2018 at 03:06:03PM +0530, Shashank Sharma wrote:
>> From: "Sharma, Shashank" <shashank.sharma@intel.com>
>>
>> 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)
>> V4: Added r-b from Maarten (for v3)
>>      Addressed review comments from Ville:
>>      - Do not add a non-atomic state variable to determine lspcon output.
>>        Instead add bool in CRTC state to indicate lspcon based scaling.
>>
>> Cc: Ville Syrjala <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/i915_reg.h      |  2 ++
>>   drivers/gpu/drm/i915/intel_ddi.c     |  7 +++++++
>>   drivers/gpu/drm/i915/intel_display.c | 11 ++++++++++-
>>   drivers/gpu/drm/i915/intel_dp.c      | 10 ++++++++++
>>   drivers/gpu/drm/i915/intel_drv.h     |  5 +++++
>>   drivers/gpu/drm/i915/intel_lspcon.c  | 30 ++++++++++++++++++++++++++++++
>>   6 files changed, 64 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> index 64933fd..e383b05 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -8721,6 +8721,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 510bb77..db7d940 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. The output colorspace encoding is BT601.
>> +	 */
>> +	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_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 4a40de9..b26f4a9 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -9228,6 +9228,7 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
>>   	pipe_config->gamma_mode =
>>   		I915_READ(GAMMA_MODE(crtc->pipe)) & GAMMA_MODE_MODE_MASK;
>>   
>> +	pipe_config->external_scaling = false;
> I really don't like the name of that. Makes me think we're actually
> scaling the entire output or something. 'external_ycbcr_420_downsample'
> or something like that would be better.
ok, let me rename this to something in those lines.
>>   	if (IS_BROADWELL(dev_priv) || INTEL_GEN(dev_priv) >= 9) {
>>   		u32 tmp = I915_READ(PIPEMISC(crtc->pipe));
>>   		enum intel_output_format output_format = CRTC_OUTPUT_RGB;
>> @@ -9247,8 +9248,16 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
>>   					output_format = CRTC_OUTPUT_YCBCR420;
>>   			} else {
>>   				output_format = CRTC_OUTPUT_YCBCR444;
>> -			}
>>   
>> +				/*
>> +				 * As there is no interface defined (yet)
>> +				 * to get the user's preference for output
>> +				 * format, YCBCR444 output format is only
>> +				 * possible with LSPCON YCBCR420 output,
>> +				 * which uses LSPCON's (external )scaler.
>> +				 */
>> +				pipe_config->external_scaling = true;
>> +			}
>>   		}
>>   
>>   		pipe_config->output_format = output_format;
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index a2e8879..be19579 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -1640,6 +1640,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;
>> @@ -1669,6 +1670,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;
>> +			pipe_config->external_scaling = true;
> Why split the work between two functions like this? I think it would be
> cleaner to either inline all the code here, or move it all into
> lspcon_ycbcr420_config().
Sure, makes sense.
>> +		}
>> +	}
>> +
>>   	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 efaa331..f23129b 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -882,6 +882,9 @@ struct intel_crtc_state {
>>   
>>   	/* Output format RGB/YCBCR etc */
>>   	enum intel_output_format output_format;
>> +
>> +	/* Output up/down scaling is done in external device */
>> +	bool external_scaling;
>>   };
>>   
>>   struct intel_crtc {
>> @@ -2139,6 +2142,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..8d6a9b2 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)
> crtc_state
ok
>
>> +{
>> +	struct drm_display_info *info = &connector->display_info;
> const
ok
>> +	struct drm_display_mode *mode = &config->base.adjusted_mode;
> const ... adjusted_mode;
ok
>> +
>> +	if (drm_mode_is_420_only(info, mode)) {
>> +		if (!connector->ycbcr_420_allowed) {
>> +			DRM_ERROR("Platform doesn't support YCBCR420 output\n");
>> +			return false;
> How can we even get here?
This code flow is coming from CRTC/encoder side, whereas 420 capability 
is set in connector_init, this is the first time we are checking 
connector's capabilities. So yes, its possible to reach here.
- Shashank
>> +		}
>> +
>> +		config->port_clock /= 2;
>> +		return true;
>> +	}
>> +
>> +	return false;
>> +}
>> +
>>   static bool lspcon_probe(struct intel_lspcon *lspcon)
>>   {
>>   	int retry;
>> @@ -459,6 +478,15 @@ void lspcon_set_infoframes(struct drm_encoder *encoder,
>>   		return;
>>   	}
>>   
>> +	if (crtc_state->output_format == CRTC_OUTPUT_YCBCR444) {
>> +		if (crtc_state->external_scaling)
>> +			frame.avi.colorspace = HDMI_COLORSPACE_YUV420;
>> +		else
>> +			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 +540,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 +565,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 v4 7/7] drm/i915: Add YCBCR 4:2:0/4:4:4 support for LSPCON

[Ville Syrjälä]

On Fri, Feb 02, 2018 at 11:44:01AM +0530, Sharma, Shashank wrote:
> Regards
> 
> Shashank
> 
> 
> On 2/2/2018 12:39 AM, Ville Syrjälä wrote:
> > On Tue, Jan 30, 2018 at 03:06:03PM +0530, Shashank Sharma wrote:
> >> From: "Sharma, Shashank" <shashank.sharma@intel.com>
> >>
> >> 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)
> >> V4: Added r-b from Maarten (for v3)
> >>      Addressed review comments from Ville:
> >>      - Do not add a non-atomic state variable to determine lspcon output.
> >>        Instead add bool in CRTC state to indicate lspcon based scaling.
> >>
> >> Cc: Ville Syrjala <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/i915_reg.h      |  2 ++
> >>   drivers/gpu/drm/i915/intel_ddi.c     |  7 +++++++
> >>   drivers/gpu/drm/i915/intel_display.c | 11 ++++++++++-
> >>   drivers/gpu/drm/i915/intel_dp.c      | 10 ++++++++++
> >>   drivers/gpu/drm/i915/intel_drv.h     |  5 +++++
> >>   drivers/gpu/drm/i915/intel_lspcon.c  | 30 ++++++++++++++++++++++++++++++
> >>   6 files changed, 64 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> >> index 64933fd..e383b05 100644
> >> --- a/drivers/gpu/drm/i915/i915_reg.h
> >> +++ b/drivers/gpu/drm/i915/i915_reg.h
> >> @@ -8721,6 +8721,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 510bb77..db7d940 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. The output colorspace encoding is BT601.
> >> +	 */
> >> +	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_display.c b/drivers/gpu/drm/i915/intel_display.c
> >> index 4a40de9..b26f4a9 100644
> >> --- a/drivers/gpu/drm/i915/intel_display.c
> >> +++ b/drivers/gpu/drm/i915/intel_display.c
> >> @@ -9228,6 +9228,7 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
> >>   	pipe_config->gamma_mode =
> >>   		I915_READ(GAMMA_MODE(crtc->pipe)) & GAMMA_MODE_MODE_MASK;
> >>   
> >> +	pipe_config->external_scaling = false;
> > I really don't like the name of that. Makes me think we're actually
> > scaling the entire output or something. 'external_ycbcr_420_downsample'
> > or something like that would be better.
> ok, let me rename this to something in those lines.
> >>   	if (IS_BROADWELL(dev_priv) || INTEL_GEN(dev_priv) >= 9) {
> >>   		u32 tmp = I915_READ(PIPEMISC(crtc->pipe));
> >>   		enum intel_output_format output_format = CRTC_OUTPUT_RGB;
> >> @@ -9247,8 +9248,16 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
> >>   					output_format = CRTC_OUTPUT_YCBCR420;
> >>   			} else {
> >>   				output_format = CRTC_OUTPUT_YCBCR444;
> >> -			}
> >>   
> >> +				/*
> >> +				 * As there is no interface defined (yet)
> >> +				 * to get the user's preference for output
> >> +				 * format, YCBCR444 output format is only
> >> +				 * possible with LSPCON YCBCR420 output,
> >> +				 * which uses LSPCON's (external )scaler.
> >> +				 */
> >> +				pipe_config->external_scaling = true;
> >> +			}
> >>   		}
> >>   
> >>   		pipe_config->output_format = output_format;
> >> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> >> index a2e8879..be19579 100644
> >> --- a/drivers/gpu/drm/i915/intel_dp.c
> >> +++ b/drivers/gpu/drm/i915/intel_dp.c
> >> @@ -1640,6 +1640,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;
> >> @@ -1669,6 +1670,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;
> >> +			pipe_config->external_scaling = true;
> > Why split the work between two functions like this? I think it would be
> > cleaner to either inline all the code here, or move it all into
> > lspcon_ycbcr420_config().
> Sure, makes sense.
> >> +		}
> >> +	}
> >> +
> >>   	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 efaa331..f23129b 100644
> >> --- a/drivers/gpu/drm/i915/intel_drv.h
> >> +++ b/drivers/gpu/drm/i915/intel_drv.h
> >> @@ -882,6 +882,9 @@ struct intel_crtc_state {
> >>   
> >>   	/* Output format RGB/YCBCR etc */
> >>   	enum intel_output_format output_format;
> >> +
> >> +	/* Output up/down scaling is done in external device */
> >> +	bool external_scaling;
> >>   };
> >>   
> >>   struct intel_crtc {
> >> @@ -2139,6 +2142,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..8d6a9b2 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)
> > crtc_state
> ok
> >
> >> +{
> >> +	struct drm_display_info *info = &connector->display_info;
> > const
> ok
> >> +	struct drm_display_mode *mode = &config->base.adjusted_mode;
> > const ... adjusted_mode;
> ok
> >> +
> >> +	if (drm_mode_is_420_only(info, mode)) {
> >> +		if (!connector->ycbcr_420_allowed) {
> >> +			DRM_ERROR("Platform doesn't support YCBCR420 output\n");
> >> +			return false;
> > How can we even get here?
> This code flow is coming from CRTC/encoder side, whereas 420 capability 
> is set in connector_init, this is the first time we are checking 
> connector's capabilities. So yes, its possible to reach here.

Oh right, we don't yet use the .mode_valid() stuff for modesets.

One thing that bothers me is that the code here is structured slightly
differently than the native HDMI case. Would be nice if we could make
the two codepaths at least follow the same general flow.

Oh and the DRM_ERROR() has to go since it's possibly user triggerable.
Looks like one snuck into the native HDMI code as well. That too should
be killed off.

> - Shashank
> >> +		}
> >> +
> >> +		config->port_clock /= 2;
> >> +		return true;
> >> +	}
> >> +
> >> +	return false;
> >> +}
> >> +
> >>   static bool lspcon_probe(struct intel_lspcon *lspcon)
> >>   {
> >>   	int retry;
> >> @@ -459,6 +478,15 @@ void lspcon_set_infoframes(struct drm_encoder *encoder,
> >>   		return;
> >>   	}
> >>   
> >> +	if (crtc_state->output_format == CRTC_OUTPUT_YCBCR444) {
> >> +		if (crtc_state->external_scaling)
> >> +			frame.avi.colorspace = HDMI_COLORSPACE_YUV420;
> >> +		else
> >> +			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 +540,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 +565,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 v4 7/7] drm/i915: Add YCBCR 4:2:0/4:4:4 support for LSPCON

[Sharma, Shashank]

Regards

Shashank


On 2/2/2018 7:22 PM, Ville Syrjälä wrote:
> On Fri, Feb 02, 2018 at 11:44:01AM +0530, Sharma, Shashank wrote:
>> Regards
>>
>> Shashank
>>
>>
>> On 2/2/2018 12:39 AM, Ville Syrjälä wrote:
>>> On Tue, Jan 30, 2018 at 03:06:03PM +0530, Shashank Sharma wrote:
>>>> From: "Sharma, Shashank" <shashank.sharma@intel.com>
>>>>
>>>> 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)
>>>> V4: Added r-b from Maarten (for v3)
>>>>       Addressed review comments from Ville:
>>>>       - Do not add a non-atomic state variable to determine lspcon output.
>>>>         Instead add bool in CRTC state to indicate lspcon based scaling.
>>>>
>>>> Cc: Ville Syrjala <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/i915_reg.h      |  2 ++
>>>>    drivers/gpu/drm/i915/intel_ddi.c     |  7 +++++++
>>>>    drivers/gpu/drm/i915/intel_display.c | 11 ++++++++++-
>>>>    drivers/gpu/drm/i915/intel_dp.c      | 10 ++++++++++
>>>>    drivers/gpu/drm/i915/intel_drv.h     |  5 +++++
>>>>    drivers/gpu/drm/i915/intel_lspcon.c  | 30 ++++++++++++++++++++++++++++++
>>>>    6 files changed, 64 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>>>> index 64933fd..e383b05 100644
>>>> --- a/drivers/gpu/drm/i915/i915_reg.h
>>>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>>>> @@ -8721,6 +8721,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 510bb77..db7d940 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. The output colorspace encoding is BT601.
>>>> +	 */
>>>> +	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_display.c b/drivers/gpu/drm/i915/intel_display.c
>>>> index 4a40de9..b26f4a9 100644
>>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>>> @@ -9228,6 +9228,7 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
>>>>    	pipe_config->gamma_mode =
>>>>    		I915_READ(GAMMA_MODE(crtc->pipe)) & GAMMA_MODE_MODE_MASK;
>>>>    
>>>> +	pipe_config->external_scaling = false;
>>> I really don't like the name of that. Makes me think we're actually
>>> scaling the entire output or something. 'external_ycbcr_420_downsample'
>>> or something like that would be better.
>> ok, let me rename this to something in those lines.
>>>>    	if (IS_BROADWELL(dev_priv) || INTEL_GEN(dev_priv) >= 9) {
>>>>    		u32 tmp = I915_READ(PIPEMISC(crtc->pipe));
>>>>    		enum intel_output_format output_format = CRTC_OUTPUT_RGB;
>>>> @@ -9247,8 +9248,16 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
>>>>    					output_format = CRTC_OUTPUT_YCBCR420;
>>>>    			} else {
>>>>    				output_format = CRTC_OUTPUT_YCBCR444;
>>>> -			}
>>>>    
>>>> +				/*
>>>> +				 * As there is no interface defined (yet)
>>>> +				 * to get the user's preference for output
>>>> +				 * format, YCBCR444 output format is only
>>>> +				 * possible with LSPCON YCBCR420 output,
>>>> +				 * which uses LSPCON's (external )scaler.
>>>> +				 */
>>>> +				pipe_config->external_scaling = true;
>>>> +			}
>>>>    		}
>>>>    
>>>>    		pipe_config->output_format = output_format;
>>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>>>> index a2e8879..be19579 100644
>>>> --- a/drivers/gpu/drm/i915/intel_dp.c
>>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>>>> @@ -1640,6 +1640,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;
>>>> @@ -1669,6 +1670,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;
>>>> +			pipe_config->external_scaling = true;
>>> Why split the work between two functions like this? I think it would be
>>> cleaner to either inline all the code here, or move it all into
>>> lspcon_ycbcr420_config().
>> Sure, makes sense.
>>>> +		}
>>>> +	}
>>>> +
>>>>    	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 efaa331..f23129b 100644
>>>> --- a/drivers/gpu/drm/i915/intel_drv.h
>>>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>>>> @@ -882,6 +882,9 @@ struct intel_crtc_state {
>>>>    
>>>>    	/* Output format RGB/YCBCR etc */
>>>>    	enum intel_output_format output_format;
>>>> +
>>>> +	/* Output up/down scaling is done in external device */
>>>> +	bool external_scaling;
>>>>    };
>>>>    
>>>>    struct intel_crtc {
>>>> @@ -2139,6 +2142,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..8d6a9b2 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)
>>> crtc_state
>> ok
>>>> +{
>>>> +	struct drm_display_info *info = &connector->display_info;
>>> const
>> ok
>>>> +	struct drm_display_mode *mode = &config->base.adjusted_mode;
>>> const ... adjusted_mode;
>> ok
>>>> +
>>>> +	if (drm_mode_is_420_only(info, mode)) {
>>>> +		if (!connector->ycbcr_420_allowed) {
>>>> +			DRM_ERROR("Platform doesn't support YCBCR420 output\n");
>>>> +			return false;
>>> How can we even get here?
>> This code flow is coming from CRTC/encoder side, whereas 420 capability
>> is set in connector_init, this is the first time we are checking
>> connector's capabilities. So yes, its possible to reach here.
> Oh right, we don't yet use the .mode_valid() stuff for modesets.
>
> One thing that bothers me is that the code here is structured slightly
> differently than the native HDMI case. Would be nice if we could make
> the two codepaths at least follow the same general flow.
Very valid point, but the difference mostly is due to LSPCON still 
running like a DP encoder, whereas native HDMI case is in the HDMI side.
>
> Oh and the DRM_ERROR() has to go since it's possibly user triggerable.
> Looks like one snuck into the native HDMI code as well. That too should
> be killed off.
Agree, will prune it !
- Shashank
>
>> - Shashank
>>>> +		}
>>>> +
>>>> +		config->port_clock /= 2;
>>>> +		return true;
>>>> +	}
>>>> +
>>>> +	return false;
>>>> +}
>>>> +
>>>>    static bool lspcon_probe(struct intel_lspcon *lspcon)
>>>>    {
>>>>    	int retry;
>>>> @@ -459,6 +478,15 @@ void lspcon_set_infoframes(struct drm_encoder *encoder,
>>>>    		return;
>>>>    	}
>>>>    
>>>> +	if (crtc_state->output_format == CRTC_OUTPUT_YCBCR444) {
>>>> +		if (crtc_state->external_scaling)
>>>> +			frame.avi.colorspace = HDMI_COLORSPACE_YUV420;
>>>> +		else
>>>> +			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 +540,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 +565,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 (rev2)

[Patchwork]

== Series Details ==

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

== Summary ==

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

Test debugfs_test:
        Subgroup read_all_entries:
                pass       -> DMESG-WARN (fi-hsw-4770r)
Test gem_exec_suspend:
        Subgroup basic-s3:
                pass       -> DMESG-WARN (fi-hsw-4770r) fdo#103375 +3
                pass       -> DMESG-WARN (fi-kbl-7567u)
        Subgroup basic-s4-devices:
                pass       -> DMESG-WARN (fi-hsw-4770r)
Test kms_busy:
        Subgroup basic-flip-a:
                pass       -> DMESG-WARN (fi-hsw-4770r)
        Subgroup basic-flip-b:
                pass       -> DMESG-WARN (fi-hsw-4770r)
        Subgroup basic-flip-c:
                pass       -> DMESG-WARN (fi-hsw-4770r)
Test kms_flip:
        Subgroup basic-flip-vs-dpms:
                pass       -> DMESG-WARN (fi-hsw-4770r)
        Subgroup basic-flip-vs-modeset:
                pass       -> DMESG-WARN (fi-hsw-4770r)
        Subgroup basic-flip-vs-wf_vblank:
                pass       -> DMESG-WARN (fi-hsw-4770r) fdo#100368
        Subgroup basic-plain-flip:
                pass       -> DMESG-WARN (fi-hsw-4770r)
Test kms_frontbuffer_tracking:
        Subgroup basic:
                pass       -> DMESG-WARN (fi-hsw-4770r)
Test kms_pipe_crc_basic:
        Subgroup hang-read-crc-pipe-b:
                pass       -> DMESG-WARN (fi-hsw-4770r)
        Subgroup hang-read-crc-pipe-c:
                pass       -> DMESG-WARN (fi-hsw-4770r)
        Subgroup nonblocking-crc-pipe-b:
                pass       -> DMESG-WARN (fi-hsw-4770r)
        Subgroup nonblocking-crc-pipe-b-frame-sequence:
                pass       -> DMESG-WARN (fi-hsw-4770r) fdo#103481 +3
        Subgroup nonblocking-crc-pipe-c:
                pass       -> DMESG-WARN (fi-hsw-4770r)
        Subgroup read-crc-pipe-b:
                pass       -> DMESG-WARN (fi-hsw-4770r)
        Subgroup read-crc-pipe-c:
                pass       -> DMESG-WARN (fi-hsw-4770r)
        Subgroup suspend-read-crc-pipe-a:
                fail       -> PASS       (fi-skl-guc) fdo#103191
                pass       -> DMESG-WARN (fi-kbl-7567u)
        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 pm_rpm:
        Subgroup basic-pci-d3-state:
                pass       -> DMESG-WARN (fi-hsw-4770r)
        Subgroup basic-rte:
                pass       -> DMESG-WARN (fi-hsw-4770r)
Test drv_module_reload:
        Subgroup basic-reload:
                pass       -> DMESG-WARN (fi-hsw-4770r)
        Subgroup basic-no-display:
                pass       -> DMESG-WARN (fi-hsw-4770r)
        Subgroup basic-reload-inject:
                pass       -> DMESG-WARN (fi-hsw-4770r)

fdo#103375 https://bugs.freedesktop.org/show_bug.cgi?id=103375
fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
fdo#103481 https://bugs.freedesktop.org/show_bug.cgi?id=103481
fdo#103191 https://bugs.freedesktop.org/show_bug.cgi?id=103191

fi-bdw-5557u     total:288  pass:267  dwarn:0   dfail:0   fail:0   skip:21  time:420s
fi-bdw-gvtdvm    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:424s
fi-blb-e6850     total:288  pass:223  dwarn:1   dfail:0   fail:0   skip:64  time:372s
fi-bwr-2160      total:288  pass:183  dwarn:0   dfail:0   fail:0   skip:105 time:279s
fi-bxt-j4205     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:486s
fi-byt-j1900     total:288  pass:253  dwarn:0   dfail:0   fail:0   skip:35  time:466s
fi-cfl-s2        total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:565s
fi-glk-1         total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:512s
fi-hsw-4770r     total:288  pass:232  dwarn:29  dfail:0   fail:0   skip:27  time:404s
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:457s
fi-ivb-3770      total:288  pass:255  dwarn:0   dfail:0   fail:0   skip:33  time:414s
fi-kbl-7500u     total:288  pass:263  dwarn:1   dfail:0   fail:0   skip:24  time:459s
fi-kbl-7560u     total:288  pass:269  dwarn:0   dfail:0   fail:0   skip:19  time:494s
fi-kbl-7567u     total:288  pass:264  dwarn:4   dfail:0   fail:0   skip:20  time:455s
fi-kbl-r         total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:504s
fi-pnv-d510      total:288  pass:222  dwarn:1   dfail:0   fail:0   skip:65  time:576s
fi-skl-6260u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:431s
fi-skl-6600u     total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:511s
fi-skl-6700hq    total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:527s
fi-skl-6700k2    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:484s
fi-skl-6770hq    total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:497s
fi-skl-guc       total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:415s
fi-skl-gvtdvm    total:288  pass:265  dwarn:0   dfail:0   fail:0   skip:23  time:430s
fi-snb-2600      total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:395s
Blacklisted hosts:
fi-glk-dsi       total:288  pass:230  dwarn:28  dfail:0   fail:0   skip:30  time:474s

d0eb027422f612e32f5b78983bd25fcbc80c70fc drm-tip: 2018y-01m-30d-10h-47m-29s UTC integration manifest
9c55b1b37c96 drm/i915: Add YCBCR 4:2:0/4:4:4 support for LSPCON
8a7f2930d115 drm/i915: Write AVI infoframes for Parade LSPCON
f31a035a0764 drm/i915: Write AVI infoframes for MCA LSPCON
bdbed7a3b87a drm/i915: Add AVI infoframe support for LSPCON
41f1b9cb0c08 drm/i915: Check LSPCON vendor OUI
af927d9d878c drm/i915: Add CRTC output format YCBCR 4:4:4
99d109942b63 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_7815/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: failure for YCBCR 4:2:0/4:4:4 output support for LSPCON (rev2)

[Patchwork]

== Series Details ==

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

== Summary ==

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

Test debugfs_test:
        Subgroup read_all_entries:
                pass       -> DMESG-WARN (fi-hsw-4770r)
Test gem_ctx_switch:
        Subgroup basic-default:
                pass       -> INCOMPLETE (fi-hsw-4770)
Test gem_exec_suspend:
        Subgroup basic-s3:
                pass       -> DMESG-WARN (fi-hsw-4770r) fdo#103375 +3
                pass       -> DMESG-WARN (fi-bxt-dsi)
                pass       -> DMESG-WARN (fi-kbl-7567u)
        Subgroup basic-s4-devices:
                pass       -> DMESG-WARN (fi-hsw-4770r)
                pass       -> DMESG-WARN (fi-bxt-dsi)
Test kms_busy:
        Subgroup basic-flip-a:
                pass       -> DMESG-WARN (fi-hsw-4770r)
                pass       -> DMESG-WARN (fi-bxt-dsi)
        Subgroup basic-flip-b:
                pass       -> DMESG-WARN (fi-hsw-4770r)
                pass       -> DMESG-WARN (fi-bxt-dsi)
        Subgroup basic-flip-c:
                pass       -> DMESG-WARN (fi-hsw-4770r)
                pass       -> DMESG-WARN (fi-bxt-dsi)
Test kms_flip:
        Subgroup basic-flip-vs-dpms:
                pass       -> DMESG-WARN (fi-hsw-4770r)
                pass       -> DMESG-WARN (fi-bxt-dsi)
        Subgroup basic-flip-vs-modeset:
                pass       -> DMESG-WARN (fi-hsw-4770r)
                pass       -> DMESG-WARN (fi-bxt-dsi)
        Subgroup basic-flip-vs-wf_vblank:
                pass       -> DMESG-WARN (fi-hsw-4770r) fdo#100368
                pass       -> DMESG-WARN (fi-bxt-dsi)
        Subgroup basic-plain-flip:
                pass       -> DMESG-WARN (fi-hsw-4770r)
                pass       -> DMESG-WARN (fi-bxt-dsi)
Test kms_frontbuffer_tracking:
        Subgroup basic:
                pass       -> DMESG-WARN (fi-hsw-4770r)
                pass       -> DMESG-WARN (fi-bxt-dsi)
Test kms_pipe_crc_basic:
        Subgroup hang-read-crc-pipe-b:
                pass       -> DMESG-WARN (fi-hsw-4770r)
                pass       -> DMESG-WARN (fi-bxt-dsi)
        Subgroup hang-read-crc-pipe-c:
                pass       -> DMESG-WARN (fi-hsw-4770r)
                pass       -> DMESG-WARN (fi-bxt-dsi)
        Subgroup nonblocking-crc-pipe-b:
                pass       -> DMESG-WARN (fi-hsw-4770r)
                pass       -> DMESG-WARN (fi-bxt-dsi)
        Subgroup nonblocking-crc-pipe-b-frame-sequence:
                pass       -> DMESG-WARN (fi-hsw-4770r) fdo#103481 +3
                pass       -> DMESG-WARN (fi-bxt-dsi)
        Subgroup nonblocking-crc-pipe-c:
                pass       -> DMESG-WARN (fi-hsw-4770r)
                pass       -> DMESG-WARN (fi-bxt-dsi)
        Subgroup nonblocking-crc-pipe-c-frame-sequence:
                pass       -> DMESG-WARN (fi-bxt-dsi)
        Subgroup read-crc-pipe-b:
                pass       -> DMESG-WARN (fi-hsw-4770r)
                pass       -> DMESG-WARN (fi-bxt-dsi)
        Subgroup read-crc-pipe-b-frame-sequence:
                pass       -> DMESG-WARN (fi-bxt-dsi)
        Subgroup read-crc-pipe-c:
                pass       -> DMESG-WARN (fi-hsw-4770r)
                pass       -> DMESG-WARN (fi-bxt-dsi)
        Subgroup read-crc-pipe-c-frame-sequence:
                pass       -> DMESG-WARN (fi-bxt-dsi)
        Subgroup suspend-read-crc-pipe-a:
                fail       -> PASS       (fi-skl-guc) fdo#103191
                pass       -> DMESG-WARN (fi-bxt-dsi)
                pass       -> DMESG-WARN (fi-kbl-7567u)
        Subgroup suspend-read-crc-pipe-b:
                pass       -> DMESG-WARN (fi-bxt-dsi)
                pass       -> DMESG-WARN (fi-kbl-7567u)
        Subgroup suspend-read-crc-pipe-c:
                pass       -> DMESG-WARN (fi-bxt-dsi)
                pass       -> DMESG-WARN (fi-kbl-7567u)
Test pm_rpm:
        Subgroup basic-pci-d3-state:
                pass       -> DMESG-WARN (fi-hsw-4770r)
                pass       -> DMESG-WARN (fi-bxt-dsi)
        Subgroup basic-rte:
                pass       -> DMESG-WARN (fi-hsw-4770r)
                pass       -> DMESG-WARN (fi-bxt-dsi)
Test drv_module_reload:
        Subgroup basic-reload:
                pass       -> DMESG-WARN (fi-hsw-4770r)
                pass       -> DMESG-WARN (fi-bxt-dsi)
        Subgroup basic-no-display:
                pass       -> DMESG-WARN (fi-hsw-4770r)
                pass       -> DMESG-WARN (fi-bxt-dsi)
        Subgroup basic-reload-inject:
                pass       -> DMESG-WARN (fi-hsw-4770r)
                pass       -> DMESG-WARN (fi-bxt-dsi)

WARNING: Long output truncated

d0eb027422f612e32f5b78983bd25fcbc80c70fc drm-tip: 2018y-01m-30d-10h-47m-29s UTC integration manifest
a4baf73a4144 drm/i915: Add YCBCR 4:2:0/4:4:4 support for LSPCON
3266ec23edd0 drm/i915: Write AVI infoframes for Parade LSPCON
ff4e9f870e1c drm/i915: Write AVI infoframes for MCA LSPCON
385997bec72c drm/i915: Add AVI infoframe support for LSPCON
99697fbbfe5c drm/i915: Check LSPCON vendor OUI
21e8d35227ee drm/i915: Add CRTC output format YCBCR 4:4:4
64d1e8a99e61 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_7818/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx