Re: [PATCH 19/20] drm/i915: Move AVI infoframe function to DDI layer

"Sharma, Shashank" <shashank.sharma@xxxxxxxxx> · Thu, 13 Jul 2017 10:28:00 +0530

Thanks for the review, Ville.

My comments, inline.

Regards
Shashank
On 7/12/2017 10:45 PM, Ville Syrjälä wrote:
On Mon, Jul 10, 2017 at 04:48:47PM +0530, Shashank Sharma wrote:
We have an existing function to prepare AVI infoframes for HDMI,
this patch moves that function from HDMI layer, to DDI layer, so
that we can reuse the function for DP(LSPCON) displays too.

This patch:
- Moves the intel_hdmi_set_avi_infoframes function in ddi layer.
- Adds code to accommodate LSPCON in the same function.
- Links this function as AVI infoframe setup function for LSPCON.

Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
Cc: Imre Deak <imre.deak@xxxxxxxxx>
Signed-off-by: Shashank Sharma <shashank.sharma@xxxxxxxxx>
---
  drivers/gpu/drm/i915/intel_ddi.c    | 89 ++++++++++++++++++++++++++++++++++---
  drivers/gpu/drm/i915/intel_drv.h    |  9 ++++
  drivers/gpu/drm/i915/intel_hdmi.c   | 52 +++-------------------
  drivers/gpu/drm/i915/intel_lspcon.c |  1 +
  4 files changed, 99 insertions(+), 52 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 80e96f1..f691710 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -2003,8 +2003,76 @@ static void intel_ddi_clk_select(struct intel_encoder *encoder,
  	}
  }
  
+void intel_ddi_set_avi_infoframe(struct drm_encoder *encoder,
+				  const struct intel_crtc_state *crtc_state)
NAK. Please look at my 'move infoframe stuff into intel_dig_port' series
if you need infoframes with DP. IIRC you already reviewed part of that.
Oh, looks like we were thinking in the same direction :).
Sure, I will look for the whole patch series, and will re-use that code 
in LSPCON/DP AVI IF series.
Also, do let me know if you need review for those !

- Shashank
+{
+	int ret;
+	union hdmi_infoframe frame;
+	struct drm_connector *connector;
+	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder);
+	struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
+	const struct drm_display_mode *adjusted_mode =
+					&crtc_state->base.adjusted_mode;
+	enum intel_output_type type = to_intel_encoder(encoder)->type;
+	bool is_hdmi2_sink = false;
+	bool rgb_qrange_selectable = false;
+	enum hdmi_colorspace colorspace = crtc_state->ycbcr420 ?
+					  HDMI_COLORSPACE_YUV420 :
+					  HDMI_COLORSPACE_RGB;
+	bool rgb_qrange_limited =  crtc_state->limited_color_range ?
+					HDMI_QUANTIZATION_RANGE_LIMITED :
+					HDMI_QUANTIZATION_RANGE_FULL;
+
+	switch (type) {
+	case INTEL_OUTPUT_HDMI:
+		connector = &intel_hdmi->attached_connector->base;
+		is_hdmi2_sink = connector->display_info.hdmi.scdc.supported;
+		rgb_qrange_selectable = intel_hdmi->rgb_quant_range_selectable;
+		break;
+
+	case INTEL_OUTPUT_DP:
+		/* We are here means its a LSPCON device, still be paranoid */
+		if (!crtc_state->lspcon_active) {
+			DRM_ERROR("No LSPCON, why am I here ?\n");
+			return;
+		}
+
+		connector = &intel_dp->attached_connector->base;
+		is_hdmi2_sink = connector->display_info.hdmi.scdc.supported;
+		if (crtc_state->ycbcr420)
+			rgb_qrange_limited = true;
+		break;
+
+	default:
+		DRM_ERROR("No other encoder allowed\n");
+		return;
+	}
+
+	ret = drm_hdmi_avi_infoframe_from_display_mode(&frame.avi,
+						       adjusted_mode,
+						       is_hdmi2_sink);
+	if (ret < 0) {
+		DRM_ERROR("couldn't fill AVI infoframe\n");
+		return;
+	}
+
+	ret = drm_hdmi_avi_infoframe_set_colorspace(&frame.avi,
+						    adjusted_mode,
+						    colorspace);
+	if (ret < 0) {
+		DRM_ERROR("couldn't fill AVI colorspace\n");
+		return;
+	}
+
+	drm_hdmi_avi_infoframe_quant_range(&frame.avi, adjusted_mode,
+					   rgb_qrange_limited,
+					   rgb_qrange_selectable);
+
+	intel_write_infoframe(encoder, crtc_state, &frame);
+}
+
  static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder,
-				    int link_rate, uint32_t lane_count,
+				    const struct intel_crtc_state *pipe_config,
  				    struct intel_shared_dpll *pll,
  				    bool link_mst)
  {
@@ -2012,6 +2080,8 @@ static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder,
  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
  	enum port port = intel_ddi_get_encoder_port(encoder);
  	struct intel_digital_port *dig_port = enc_to_dig_port(&encoder->base);
+	int link_rate = pipe_config->port_clock;
+	uint32_t lane_count = pipe_config->lane_count;
  
  	WARN_ON(link_mst && (port == PORT_A || port == PORT_E));
  
@@ -2030,6 +2100,14 @@ static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder,
  	intel_dp_start_link_train(intel_dp);
  	if (port != PORT_A || INTEL_GEN(dev_priv) >= 9)
  		intel_dp_stop_link_train(intel_dp);
+
+	if (pipe_config->lspcon_active) {
+		struct drm_encoder *drm_encoder = &encoder->base;
+		struct intel_lspcon *lspcon = enc_to_intel_lspcon(drm_encoder);
+
+		if (lspcon->set_infoframes)
+			lspcon->set_infoframes(&encoder->base, pipe_config);
+	}
  }
  
  static void intel_ddi_pre_enable_hdmi(struct intel_encoder *encoder,
@@ -2072,8 +2150,7 @@ static void intel_ddi_pre_enable(struct intel_encoder *encoder,
  
  	if (type == INTEL_OUTPUT_DP || type == INTEL_OUTPUT_EDP) {
  		intel_ddi_pre_enable_dp(encoder,
-					pipe_config->port_clock,
-					pipe_config->lane_count,
+					pipe_config,
  					pipe_config->shared_dpll,
  					intel_crtc_has_type(pipe_config,
  							    INTEL_OUTPUT_DP_MST));
@@ -2628,17 +2705,17 @@ void intel_ddi_init(struct drm_i915_private *dev_priv, enum port port)
  	}
  
  	if (init_lspcon) {
-		if (lspcon_init(intel_dig_port))
-			/* TODO: handle hdmi info frame part */
+		if (lspcon_init(intel_dig_port)) {
  			DRM_DEBUG_KMS("LSPCON init success on port %c\n",
  				port_name(port));
-		else
+		} else {
  			/*
  			 * LSPCON init faied, but DP init was success, so
  			 * lets try to drive as DP++ port.
  			 */
  			DRM_ERROR("LSPCON init failed on port %c\n",
  				port_name(port));
+		}
  	}
  
  	return;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 40d56f2..fad9a53 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1075,6 +1075,10 @@ struct intel_lspcon {
  	bool active;
  	enum drm_lspcon_mode mode;
  	enum lspcon_vendor vendor;
+
+	/* AVI IF setup function for LSPCON */
+	void (*set_infoframes)(struct drm_encoder *encoder,
+				const struct intel_crtc_state *crtc_state);
  };
  
  struct intel_digital_port {
@@ -1320,6 +1324,8 @@ u8 intel_ddi_dp_voltage_max(struct intel_encoder *encoder);
  
  unsigned int intel_fb_align_height(const struct drm_framebuffer *fb,
  				   int plane, unsigned int height);
+void intel_ddi_set_avi_infoframe(struct drm_encoder *encoder,
+				 const struct intel_crtc_state *crtc_state);
  
  /* intel_audio.c */
  void intel_init_audio_hooks(struct drm_i915_private *dev_priv);
@@ -1690,6 +1696,9 @@ void intel_hdmi_handle_sink_scrambling(struct intel_encoder *intel_encoder,
  				       struct drm_connector *connector,
  				       bool high_tmds_clock_ratio,
  				       bool scrambling);
+void intel_write_infoframe(struct drm_encoder *encoder,
+			    const struct intel_crtc_state *crtc_state,
+			    union hdmi_infoframe *frame);
  void intel_dp_dual_mode_set_tmds_output(struct intel_hdmi *hdmi, bool enable);
  bool intel_hdmi_ycbcr420_config(struct drm_connector *connector,
  				struct intel_crtc_state *config,
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index a08ab99..7cab86a 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -430,7 +430,7 @@ static bool hsw_infoframe_enabled(struct drm_encoder *encoder,
   * trick them by giving an offset into the buffer and moving back the header
   * bytes by one.
   */
-static void intel_write_infoframe(struct drm_encoder *encoder,
+void intel_write_infoframe(struct drm_encoder *encoder,
  				  const struct intel_crtc_state *crtc_state,
  				  union hdmi_infoframe *frame)
  {
@@ -453,46 +453,6 @@ static void intel_write_infoframe(struct drm_encoder *encoder,
  	intel_hdmi->write_infoframe(encoder, crtc_state, frame->any.type, buffer, len);
  }
  
-static void intel_hdmi_set_avi_infoframe(struct drm_encoder *encoder,
-					 const struct intel_crtc_state *crtc_state)
-{
-	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder);
-	const struct drm_display_mode *adjusted_mode =
-		&crtc_state->base.adjusted_mode;
-	struct drm_connector *connector = &intel_hdmi->attached_connector->base;
-	bool is_hdmi2_sink = connector->display_info.hdmi.scdc.supported;
-	enum hdmi_colorspace colorspace = HDMI_COLORSPACE_RGB;
-	union hdmi_infoframe frame;
-	int ret;
-
-	ret = drm_hdmi_avi_infoframe_from_display_mode(&frame.avi,
-						       adjusted_mode,
-						       is_hdmi2_sink);
-	if (ret < 0) {
-		DRM_ERROR("couldn't fill AVI infoframe\n");
-		return;
-	}
-
-	if (crtc_state->ycbcr420)
-		colorspace = HDMI_COLORSPACE_YUV420;
-
-	ret = drm_hdmi_avi_infoframe_set_colorspace(&frame.avi,
-						    adjusted_mode,
-						    colorspace);
-	if (ret < 0) {
-		DRM_ERROR("couldn't fill AVI colorspace\n");
-		return;
-	}
-
-	drm_hdmi_avi_infoframe_quant_range(&frame.avi, adjusted_mode,
-					   crtc_state->limited_color_range ?
-					   HDMI_QUANTIZATION_RANGE_LIMITED :
-					   HDMI_QUANTIZATION_RANGE_FULL,
-					   intel_hdmi->rgb_quant_range_selectable);
-
-	intel_write_infoframe(encoder, crtc_state, &frame);
-}
-
  static void intel_hdmi_set_spd_infoframe(struct drm_encoder *encoder,
  					 const struct intel_crtc_state *crtc_state)
  {
@@ -582,7 +542,7 @@ static void g4x_set_infoframes(struct drm_encoder *encoder,
  	I915_WRITE(reg, val);
  	POSTING_READ(reg);
  
-	intel_hdmi_set_avi_infoframe(encoder, crtc_state);
+	intel_ddi_set_avi_infoframe(encoder, crtc_state);
  	intel_hdmi_set_spd_infoframe(encoder, crtc_state);
  	intel_hdmi_set_hdmi_infoframe(encoder, crtc_state);
  }
@@ -723,7 +683,7 @@ static void ibx_set_infoframes(struct drm_encoder *encoder,
  	I915_WRITE(reg, val);
  	POSTING_READ(reg);
  
-	intel_hdmi_set_avi_infoframe(encoder, crtc_state);
+	intel_ddi_set_avi_infoframe(encoder, crtc_state);
  	intel_hdmi_set_spd_infoframe(encoder, crtc_state);
  	intel_hdmi_set_hdmi_infoframe(encoder, crtc_state);
  }
@@ -766,7 +726,7 @@ static void cpt_set_infoframes(struct drm_encoder *encoder,
  	I915_WRITE(reg, val);
  	POSTING_READ(reg);
  
-	intel_hdmi_set_avi_infoframe(encoder, crtc_state);
+	intel_ddi_set_avi_infoframe(encoder, crtc_state);
  	intel_hdmi_set_spd_infoframe(encoder, crtc_state);
  	intel_hdmi_set_hdmi_infoframe(encoder, crtc_state);
  }
@@ -819,7 +779,7 @@ static void vlv_set_infoframes(struct drm_encoder *encoder,
  	I915_WRITE(reg, val);
  	POSTING_READ(reg);
  
-	intel_hdmi_set_avi_infoframe(encoder, crtc_state);
+	intel_ddi_set_avi_infoframe(encoder, crtc_state);
  	intel_hdmi_set_spd_infoframe(encoder, crtc_state);
  	intel_hdmi_set_hdmi_infoframe(encoder, crtc_state);
  }
@@ -852,7 +812,7 @@ static void hsw_set_infoframes(struct drm_encoder *encoder,
  	I915_WRITE(reg, val);
  	POSTING_READ(reg);
  
-	intel_hdmi_set_avi_infoframe(encoder, crtc_state);
+	intel_ddi_set_avi_infoframe(encoder, crtc_state);
  	intel_hdmi_set_spd_infoframe(encoder, crtc_state);
  	intel_hdmi_set_hdmi_infoframe(encoder, crtc_state);
  }
diff --git a/drivers/gpu/drm/i915/intel_lspcon.c b/drivers/gpu/drm/i915/intel_lspcon.c
index f611b6d..53ddd39 100644
--- a/drivers/gpu/drm/i915/intel_lspcon.c
+++ b/drivers/gpu/drm/i915/intel_lspcon.c
@@ -281,6 +281,7 @@ bool lspcon_init(struct intel_digital_port *intel_dig_port)
  	}
  
  	connector->ycbcr_420_allowed = true;
+	lspcon->set_infoframes = intel_ddi_set_avi_infoframe;
  	drm_dp_read_desc(&dp->aux, &dp->desc, drm_dp_is_branch(dp->dpcd));
  
  	DRM_DEBUG_KMS("Success: LSPCON init\n");
--
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel







