[Intel-gfx] [PATCH] drm/i915: Fix DDC bus selection for multifunction SDVO

[ykzhao]

On Wed, 2010-04-28 at 23:58 +0800, Adam Jackson wrote:
> On Wed, 2010-04-28 at 10:15 +0800, Zhenyu Wang wrote:
> > On 2010.04.23 16:16:12 -0400, Adam Jackson wrote:
> > > Multifunction SDVO cards stopped working after 14571b4, and would report
> > > something that looked remarkably like an ADD2 SPD ROM instead of EDID.
> > > This appears to be because DDC bus selection was utterly horked by that
> > > commit; controlled_output was no longer always a single bit, so
> > > intel_sdvo_select_ddc_bus would pick bus 0, which is (unsurprisingly)
> > > the SPD ROM bus, not a DDC bus.
> > > 
> > > So, instead of that, let's just use the DDC bus the child device table
> > > tells us to use.  I'm guessing at the bitmask and shifting from VBIOS
> > > dumps, but it can't possibly be worse.
> > > 
> > > cf. https://bugzilla.redhat.com/584229
> > 
> > I'm worried about anything depending on BIOS table info for everytime.
> > Or if we have a fallback to spec method way to validate if BIOS info
> > really makes sense? As intel_sdvo_select_ddc_bus follows spec to select
> > ddc bus switch, which in most case should be followed by SDVO chip too.
> 
> Well, if the BIOS uses this data table, and if output detection of SDVO
> devices works at BIOS time, then we can probably use it safely at
> runtime too.  Read the BIOS and find out.
> 
> > We should fix the DDC bus selection issue by check attached_output now
> > and after detection for getting back the real connected output type, instead
> > of fixed in init.
> 
> The "priority order" thing in the current implementation of
> intel_sdvo_select_ddc_bus is presented without justification.  I assume
> it's derived from a design document given by Intel to BIOS vendors
> and/or ADD2 device manufacturers.  If they're following _that_
> recommendation, then they would probably also be following the
> recommendations to put the DDC bus they choose in the child device
> table.  (Otherwise, why would that field in the CDT even exist.)  So the
> DDC bus listed in the CDT is correct, and we should use it.

Hi, Ajax
   
    Using the BIOS-defined value in VBT can fix the bug you mentioned.
but the problem is that it is static and would still have problem if
user does outputs swap on a multiple function SDVO device at run
time(E.g. The external monitor is changed from SDVO-VGA to SDVO-DVI). 
    Indeed, there is a SDVO specification that we use to write the SDVO
code(Unfortunately, it's not white-washed yet to be released publicly).
There is a predefined priority scheme described in the specification
that driver and HW vendor need to follow, in order to pick up the
correct DDC bus on the SDVO device that supports multiple outputs. For
example, the RGB0 will have higher priority than TMDS0 if both
RGB0/TMDS0 exist. RGB0 should use DDC1 and TMDS0 use the DDC2.)

What do you think the following patch? Would it work for the bug
reported on redhat bugzilla584229?

>From ea7e5963ef5d69f6ccf78022027ee1294513fe32 Mon Sep 17 00:00:00 2001
From: Zhao Yakui <yakui.zhao at intel.com>
Date: Wed, 5 May 2010 15:19:10 +0800
Subject: [PATCH] drm/i915: dynamically select the ddc bus according to detected SDVO output

Signed-off-by: Zhao Yakui <yakui.zhao at intel.com>
---
According to the SDVO spec the multiple DDC buses need to be mapped
accordingly to each expected outputs for the SDVO device with multiple
outputs. And they are selected by using predefined priority scheme.
(E.g. The RGB0 will have higher priority than TMDS0 if it reports the 
capability of RGB0/TMDS0. RGB0 should use DDC1 and TMDS0 use the DDC2)

 drivers/gpu/drm/i915/intel_sdvo.c |   23 ++++++++++++++++-------
 1 files changed, 16 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
index df9f997..4431ab6 100644
--- a/drivers/gpu/drm/i915/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/intel_sdvo.c
@@ -172,6 +172,10 @@ struct intel_sdvo_connector {
 	u32	cur_hue,	max_hue;
 };
 
+static void
+intel_sdvo_select_ddc_bus(struct intel_sdvo_priv *dev_priv,
+				struct intel_connector *intel_connector);
+
 static bool
 intel_sdvo_output_setup(struct intel_encoder *intel_encoder,
 			uint16_t flags);
@@ -1599,8 +1603,11 @@ static enum drm_connector_status intel_sdvo_detect(struct drm_connector *connect
 	sdvo_priv->attached_output = response;
 
 	if ((sdvo_connector->output_flag & response) == 0)
-		ret = connector_status_disconnected;
-	else if (response & (SDVO_OUTPUT_TMDS0 | SDVO_OUTPUT_TMDS1))
+		return connector_status_disconnected;
+
+	intel_sdvo_select_ddc_bus(sdvo_priv, intel_connector);
+
+	if (response & (SDVO_OUTPUT_TMDS0 | SDVO_OUTPUT_TMDS1))
 		ret = intel_sdvo_hdmi_sink_detect(connector, response);
 	else
 		ret = connector_status_connected;
@@ -2047,21 +2054,25 @@ static const struct drm_encoder_funcs intel_sdvo_enc_funcs = {
 
 /**
  * Choose the appropriate DDC bus for control bus switch command for this
- * SDVO output based on the controlled output.
+ * SDVO output based on the connector's output.
  *
  * DDC bus number assignment is in a priority order of RGB outputs, then TMDS
  * outputs, then LVDS outputs.
  */
 static void
-intel_sdvo_select_ddc_bus(struct intel_sdvo_priv *dev_priv)
+intel_sdvo_select_ddc_bus(struct intel_sdvo_priv *dev_priv,
+				struct intel_connector *intel_connector)
 {
 	uint16_t mask = 0;
 	unsigned int num_bits;
+	struct intel_sdvo_connector *sdvo_connector;
+
+	sdvo_connector = intel_connector->dev_priv;
 
 	/* Make a mask of outputs less than or equal to our own priority in the
 	 * list.
 	 */
-	switch (dev_priv->controlled_output) {
+	switch (sdvo_connector->output_flag) {
 	case SDVO_OUTPUT_LVDS1:
 		mask |= SDVO_OUTPUT_LVDS1;
 	case SDVO_OUTPUT_LVDS0:
@@ -2863,8 +2874,6 @@ bool intel_sdvo_init(struct drm_device *dev, int sdvo_reg)
 		goto err_i2c;
 	}
 
-	intel_sdvo_select_ddc_bus(sdvo_priv);
-
 	/* Set the input timing to the screen. Assume always input 0. */
 	intel_sdvo_set_target_input(intel_encoder, true, false);
 
-- 
1.5.4.5