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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Regards

Shashank


On 1/17/2018 11:57 PM, Ville Syrjälä wrote:
On Fri, Jan 05, 2018 at 03:15:29PM +0530, Shashank Sharma wrote:
Currently, we are using a bool in CRTC state (state->ycbcr420),
to indicate modeset, that the output format is YCBCR 4:2:0. Now in
order to support other YCBCR formats, we will need more such flags.

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

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

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

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

diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
index aa66e95..99e32cb 100644
--- a/drivers/gpu/drm/i915/intel_color.c
+++ b/drivers/gpu/drm/i915/intel_color.c
@@ -141,7 +141,7 @@ static void i9xx_load_csc_matrix(struct drm_crtc_state *crtc_state)
  	uint16_t coeffs[9] = { 0, };
  	struct intel_crtc_state *intel_crtc_state = to_intel_crtc_state(crtc_state);
- if (intel_crtc_state->ycbcr420) {
+	if (intel_crtc_state->output_format == CRTC_OUTPUT_YCBCR420) {
  		i9xx_load_ycbcr_conversion_matrix(intel_crtc);
  		return;
  	} else if (crtc_state->ctm) {
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index f51645a..84327e7 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -1264,7 +1264,7 @@ static void ddi_dotclock_get(struct intel_crtc_state *pipe_config)
  	else
  		dotclock = pipe_config->port_clock;
- if (pipe_config->ycbcr420)
+	if (pipe_config->output_format == CRTC_OUTPUT_YCBCR420)
  		dotclock *= 2;
if (pipe_config->pixel_multiplier)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 0cd3559..69b0aa3 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4644,7 +4644,8 @@ skl_update_scaler(struct intel_crtc_state *crtc_state, bool force_detach,
  	 */
  	need_scaling = src_w != dst_w || src_h != dst_h;
- if (crtc_state->ycbcr420 && scaler_user == SKL_CRTC_INDEX)
+	if (crtc_state->output_format == CRTC_OUTPUT_YCBCR420 &&
+	    scaler_user == SKL_CRTC_INDEX)
  		need_scaling = true;
/*
@@ -6356,7 +6357,8 @@ static int intel_crtc_compute_config(struct intel_crtc *crtc,
  		return -EINVAL;
  	}
- if (pipe_config->ycbcr420 && pipe_config->base.ctm) {
+	if (pipe_config->output_format == CRTC_OUTPUT_YCBCR420 &&
+	    pipe_config->base.ctm) {
  		/*
  		 * There is only one pipe CSC unit per pipe, and we need that
  		 * for output conversion from RGB->YCBCR. So if CTM is already
@@ -8177,10 +8179,10 @@ static void haswell_set_pipemisc(struct drm_crtc *crtc)
  		if (intel_crtc->config->dither)
  			val |= PIPEMISC_DITHER_ENABLE | PIPEMISC_DITHER_TYPE_SP;
- if (config->ycbcr420) {
-			val |= PIPEMISC_OUTPUT_COLORSPACE_YUV |
-				PIPEMISC_YUV420_ENABLE |
-				PIPEMISC_YUV420_MODE_FULL_BLEND;
+		if (config->output_format == CRTC_OUTPUT_YCBCR420) {
+			val |= PIPEMISC_OUTPUT_COLORSPACE_YUV;
+			val |= PIPEMISC_YUV420_ENABLE |
+			       PIPEMISC_YUV420_MODE_FULL_BLEND;
  		}
I915_WRITE(PIPEMISC(intel_crtc->pipe), val);
@@ -9156,6 +9158,19 @@ static void haswell_get_ddi_port_state(struct intel_crtc *crtc,
  	}
  }
+static const char * const output_format_str[] = {
+	"Invalid",
+	"RGB",
+	"YCBCR4:2:0",
+};
+
+static const char *output_formats(enum crtc_output_format format)
+{
+	if (format < CRTC_OUTPUT_INVALID || format > CRTC_OUTPUT_YCBCR420)
+		format = CRTC_OUTPUT_INVALID;
+	return output_format_str[format + 1];
+}
+
  static bool haswell_get_pipe_config(struct intel_crtc *crtc,
  				    struct intel_crtc_state *pipe_config)
  {
@@ -9196,19 +9211,28 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
if (IS_BROADWELL(dev_priv) || INTEL_GEN(dev_priv) >= 9) {
  		u32 tmp = I915_READ(PIPEMISC(crtc->pipe));
-		bool clrspace_yuv = tmp & PIPEMISC_OUTPUT_COLORSPACE_YUV;
-
-		if (IS_GEMINILAKE(dev_priv) || INTEL_GEN(dev_priv) >= 10) {
-			bool blend_mode_420 = tmp &
-					      PIPEMISC_YUV420_MODE_FULL_BLEND;
+		enum crtc_output_format output_format = CRTC_OUTPUT_RGB;
+
+		if (tmp & PIPEMISC_OUTPUT_COLORSPACE_YUV) {
+			bool ycbcr420_enabled = tmp & PIPEMISC_YUV420_ENABLE;
+			bool blend = tmp & PIPEMISC_YUV420_MODE_FULL_BLEND;
+
+			if (ycbcr420_enabled) {
+				/* We support 4:2:0 in full blend mode only */
+				if (!blend)
+					output_format = CRTC_OUTPUT_INVALID;
Not sure that INVALID thing really provides any real value.
Just feels like it's making the code more convoluted for something that
should never really happen.
The check is directly adapted from the previous version of the code, where we were still checking if the registers are programmed properly, CRTC_OUTPUT_INVALID helps to print the error messages and fits into the existing framework, so I thought there is no harm in keeping it.
+				else if (!(IS_GEMINILAKE(dev_priv) ||
+					   INTEL_GEN(dev_priv) >= 10))
+					output_format = CRTC_OUTPUT_INVALID;
+				else
+					output_format = CRTC_OUTPUT_YCBCR420;
+			}
- pipe_config->ycbcr420 = tmp & PIPEMISC_YUV420_ENABLE;
-			if (pipe_config->ycbcr420 != clrspace_yuv ||
-			    pipe_config->ycbcr420 != blend_mode_420)
-				DRM_DEBUG_KMS("Bad 4:2:0 mode (%08x)\n", tmp);
-		} else if (clrspace_yuv) {
-			DRM_DEBUG_KMS("YCbCr 4:2:0 Unsupported\n");
  		}
+
+		pipe_config->output_format = output_format;
+		DRM_DEBUG_KMS("Output format %s\n",
+				output_formats(output_format));
  	}
power_domain = POWER_DOMAIN_PIPE_PANEL_FITTER(crtc->pipe);
@@ -10558,6 +10582,9 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc,
  	DRM_DEBUG_KMS("output_types: %s (0x%x)\n",
  		      buf, pipe_config->output_types);
+ DRM_DEBUG_KMS("output format: %s\n",
+		output_formats(pipe_config->output_format));
+
  	DRM_DEBUG_KMS("cpu_transcoder: %s, pipe bpp: %i, dithering: %i\n",
  		      transcoder_name(pipe_config->cpu_transcoder),
  		      pipe_config->pipe_bpp, pipe_config->dither);
@@ -10567,9 +10594,6 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc,
  				      pipe_config->fdi_lanes,
  				      &pipe_config->fdi_m_n);
- if (pipe_config->ycbcr420)
-		DRM_DEBUG_KMS("YCbCr 4:2:0 output enabled\n");
-
  	if (intel_crtc_has_dp_encoder(pipe_config)) {
  		intel_dump_m_n_config(pipe_config, "dp m_n",
  				pipe_config->lane_count, &pipe_config->dp_m_n);
@@ -11143,6 +11167,7 @@ intel_pipe_config_compare(struct drm_i915_private *dev_priv,
  	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_vsync_end);
PIPE_CONF_CHECK_I(pixel_multiplier);
+	PIPE_CONF_CHECK_I(output_format);
  	PIPE_CONF_CHECK_BOOL(has_hdmi_sink);
  	if ((INTEL_GEN(dev_priv) < 8 && !IS_HASWELL(dev_priv)) ||
  	    IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
@@ -11151,7 +11176,6 @@ intel_pipe_config_compare(struct drm_i915_private *dev_priv,
  	PIPE_CONF_CHECK_BOOL(hdmi_scrambling);
  	PIPE_CONF_CHECK_BOOL(hdmi_high_tmds_clock_ratio);
  	PIPE_CONF_CHECK_BOOL_INCOMPLETE(has_infoframe);
-	PIPE_CONF_CHECK_BOOL(ycbcr420);
PIPE_CONF_CHECK_BOOL_INCOMPLETE(has_audio); diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 30f791f..79662650 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -609,6 +609,12 @@ struct intel_crtc_wm_state {
  	bool need_postvbl_update;
  };
+enum crtc_output_format {
intel_output_format or something would fit the normal namespacing
better.
Agree, will change it accordingly.

+	CRTC_OUTPUT_INVALID = -1,
The -1 doesn't make much sense to me. If we want to keep this IMO
just let the enum start from 0. That will at least make it clear
if you entirely forgot to do readout.
The reason why I kept INVALID as -1, is to make sure RGB starts at 0. So that we need not to explicitly set the CRTC_OUTPUT_FORMAT = RGB (which is also one of your comment in the
series). This will allow least modifications in the existing code.

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

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

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




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux