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

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

[Adam Jackson]

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

Signed-off-by: Adam Jackson <ajax@redhat.com>
---
 drivers/gpu/drm/i915/i915_drv.h   |    1 +
 drivers/gpu/drm/i915/intel_bios.c |    1 +
 drivers/gpu/drm/i915/intel_sdvo.c |   41 ++++++++----------------------------
 3 files changed, 11 insertions(+), 32 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index a43a4f5..5d609a8 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -135,6 +135,7 @@ struct sdvo_device_mapping {
 	u8 slave_addr;
 	u8 dvo_wiring;
 	u8 initialized;
+	u8 ddc_pin;
 };
 
 struct drm_i915_error_state {
diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
index f9ba452..4c748d8 100644
--- a/drivers/gpu/drm/i915/intel_bios.c
+++ b/drivers/gpu/drm/i915/intel_bios.c
@@ -366,6 +366,7 @@ parse_sdvo_device_mapping(struct drm_i915_private *dev_priv,
 			p_mapping->dvo_port = p_child->dvo_port;
 			p_mapping->slave_addr = p_child->slave_addr;
 			p_mapping->dvo_wiring = p_child->dvo_wiring;
+			p_mapping->ddc_pin = p_child->ddc_pin;
 			p_mapping->initialized = 1;
 		} else {
 			DRM_DEBUG_KMS("Maybe one SDVO port is shared by "
diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
index df9f997..ad96360 100644
--- a/drivers/gpu/drm/i915/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/intel_sdvo.c
@@ -2053,40 +2053,17 @@ static const struct drm_encoder_funcs intel_sdvo_enc_funcs = {
  * outputs, then LVDS outputs.
  */
 static void
-intel_sdvo_select_ddc_bus(struct intel_sdvo_priv *dev_priv)
+intel_sdvo_select_ddc_bus(struct drm_i915_private *dev_priv,
+			  struct intel_sdvo_priv *sdvo, u32 reg)
 {
-	uint16_t mask = 0;
-	unsigned int num_bits;
+	struct sdvo_device_mapping *mapping;
 
-	/* Make a mask of outputs less than or equal to our own priority in the
-	 * list.
-	 */
-	switch (dev_priv->controlled_output) {
-	case SDVO_OUTPUT_LVDS1:
-		mask |= SDVO_OUTPUT_LVDS1;
-	case SDVO_OUTPUT_LVDS0:
-		mask |= SDVO_OUTPUT_LVDS0;
-	case SDVO_OUTPUT_TMDS1:
-		mask |= SDVO_OUTPUT_TMDS1;
-	case SDVO_OUTPUT_TMDS0:
-		mask |= SDVO_OUTPUT_TMDS0;
-	case SDVO_OUTPUT_RGB1:
-		mask |= SDVO_OUTPUT_RGB1;
-	case SDVO_OUTPUT_RGB0:
-		mask |= SDVO_OUTPUT_RGB0;
-		break;
-	}
-
-	/* Count bits to find what number we are in the priority list. */
-	mask &= dev_priv->caps.output_flags;
-	num_bits = hweight16(mask);
-	if (num_bits > 3) {
-		/* if more than 3 outputs, default to DDC bus 3 for now */
-		num_bits = 3;
-	}
+	if (IS_SDVOB(reg))
+		mapping = &(dev_priv->sdvo_mappings[0]);
+	else
+		mapping = &(dev_priv->sdvo_mappings[1]);
 
-	/* Corresponds to SDVO_CONTROL_BUS_DDCx */
-	dev_priv->ddc_bus = 1 << num_bits;
+	sdvo->ddc_bus = (mapping->ddc_pin & 0xf0) >> 4;
 }
 
 static bool
@@ -2863,7 +2840,7 @@ bool intel_sdvo_init(struct drm_device *dev, int sdvo_reg)
 		goto err_i2c;
 	}
 
-	intel_sdvo_select_ddc_bus(sdvo_priv);
+	intel_sdvo_select_ddc_bus(dev_priv, sdvo_priv, sdvo_reg);
 
 	/* Set the input timing to the screen. Assume always input 0. */
 	intel_sdvo_set_target_input(intel_encoder, true, false);
-- 
1.7.0.1

Re: [PATCH] drm/i915: Fix DDC bus selection for multifunction SDVO

[ykzhao]

On Sat, 2010-04-24 at 04:16 +0800, 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.

thanks for caring this issue.

It seems that now the incorrect DDC bus is selected for the this
multi-function SDVO card.
   >the DDC bus will be selected as 1. Then the SPD rom is obtained
instead of EDID.

> 
> 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 get the vbios.dump from the bugzilla. The the child device structure
in vbios.dump will report that the value of ddc_pin is 0x1D. And then we
still get the wrong argument for DDC bus switch command.


Now I have such one card in my hand. I can also work on this issue.

Thanks.
    Yakui

> Signed-off-by: Adam Jackson <ajax@redhat.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h   |    1 +
>  drivers/gpu/drm/i915/intel_bios.c |    1 +
>  drivers/gpu/drm/i915/intel_sdvo.c |   41 ++++++++----------------------------
>  3 files changed, 11 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index a43a4f5..5d609a8 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -135,6 +135,7 @@ struct sdvo_device_mapping {
>  	u8 slave_addr;
>  	u8 dvo_wiring;
>  	u8 initialized;
> +	u8 ddc_pin;
>  };
>  
>  struct drm_i915_error_state {
> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
> index f9ba452..4c748d8 100644
> --- a/drivers/gpu/drm/i915/intel_bios.c
> +++ b/drivers/gpu/drm/i915/intel_bios.c
> @@ -366,6 +366,7 @@ parse_sdvo_device_mapping(struct drm_i915_private *dev_priv,
>  			p_mapping->dvo_port = p_child->dvo_port;
>  			p_mapping->slave_addr = p_child->slave_addr;
>  			p_mapping->dvo_wiring = p_child->dvo_wiring;
> +			p_mapping->ddc_pin = p_child->ddc_pin;
>  			p_mapping->initialized = 1;
>  		} else {
>  			DRM_DEBUG_KMS("Maybe one SDVO port is shared by "
> diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
> index df9f997..ad96360 100644
> --- a/drivers/gpu/drm/i915/intel_sdvo.c
> +++ b/drivers/gpu/drm/i915/intel_sdvo.c
> @@ -2053,40 +2053,17 @@ static const struct drm_encoder_funcs intel_sdvo_enc_funcs = {
>   * outputs, then LVDS outputs.
>   */
>  static void
> -intel_sdvo_select_ddc_bus(struct intel_sdvo_priv *dev_priv)
> +intel_sdvo_select_ddc_bus(struct drm_i915_private *dev_priv,
> +			  struct intel_sdvo_priv *sdvo, u32 reg)
>  {
> -	uint16_t mask = 0;
> -	unsigned int num_bits;
> +	struct sdvo_device_mapping *mapping;
>  
> -	/* Make a mask of outputs less than or equal to our own priority in the
> -	 * list.
> -	 */
> -	switch (dev_priv->controlled_output) {
> -	case SDVO_OUTPUT_LVDS1:
> -		mask |= SDVO_OUTPUT_LVDS1;
> -	case SDVO_OUTPUT_LVDS0:
> -		mask |= SDVO_OUTPUT_LVDS0;
> -	case SDVO_OUTPUT_TMDS1:
> -		mask |= SDVO_OUTPUT_TMDS1;
> -	case SDVO_OUTPUT_TMDS0:
> -		mask |= SDVO_OUTPUT_TMDS0;
> -	case SDVO_OUTPUT_RGB1:
> -		mask |= SDVO_OUTPUT_RGB1;
> -	case SDVO_OUTPUT_RGB0:
> -		mask |= SDVO_OUTPUT_RGB0;
> -		break;
> -	}
> -
> -	/* Count bits to find what number we are in the priority list. */
> -	mask &= dev_priv->caps.output_flags;
> -	num_bits = hweight16(mask);
> -	if (num_bits > 3) {
> -		/* if more than 3 outputs, default to DDC bus 3 for now */
> -		num_bits = 3;
> -	}
> +	if (IS_SDVOB(reg))
> +		mapping = &(dev_priv->sdvo_mappings[0]);
> +	else
> +		mapping = &(dev_priv->sdvo_mappings[1]);
>  
> -	/* Corresponds to SDVO_CONTROL_BUS_DDCx */
> -	dev_priv->ddc_bus = 1 << num_bits;
> +	sdvo->ddc_bus = (mapping->ddc_pin & 0xf0) >> 4;
>  }
>  
>  static bool
> @@ -2863,7 +2840,7 @@ bool intel_sdvo_init(struct drm_device *dev, int sdvo_reg)
>  		goto err_i2c;
>  	}
>  
> -	intel_sdvo_select_ddc_bus(sdvo_priv);
> +	intel_sdvo_select_ddc_bus(dev_priv, sdvo_priv, sdvo_reg);
>  
>  	/* Set the input timing to the screen. Assume always input 0. */
>  	intel_sdvo_set_target_input(intel_encoder, true, false);

Re: [PATCH] drm/i915: Fix DDC bus selection for multifunction SDVO

[Adam Jackson]

[-- Attachment #1.1: Type: text/plain, Size: 521 bytes --]

On Fri, 2010-04-23 at 16:16 -0400, Adam Jackson wrote:

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

Except,

> -	/* Corresponds to SDVO_CONTROL_BUS_DDCx */
> -	dev_priv->ddc_bus = 1 << num_bits;
> +	sdvo->ddc_bus = (mapping->ddc_pin & 0xf0) >> 4;

This should probably be = 1 << ((mapping->ddc_pin & 0xf0) >> 4);

Still, do whatever the BIOS does here.

- ajax

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH] drm/i915: Fix DDC bus selection for multifunction SDVO

[Zhenyu Wang]

[-- Attachment #1.1: Type: text/plain, Size: 1262 bytes --]

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.

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.

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH] drm/i915: Fix DDC bus selection for multifunction SDVO

[Adam Jackson]

[-- Attachment #1.1: Type: text/plain, Size: 3230 bytes --]

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.

If they're _not_ following that recommendation, but there's still a CDT
that looks like it describes real SDVO devices, then either:

a) they're using the BIOS' DDC routines, and they put the DDC bus
they're really using in the CDT,
b) they're not using the BIOS' DDC routines.

Option b implies non-compatibility with off-the-shelf ADD2 cards, which
would defeat the whole purpose of ADD2.  Option a means the DDC bus
listed in the BIOS is correct, and we should use it.

If there isn't a CDT that looks like it describes real SDVO devices,
then we're already unable to drive them sensibly.

Seriously.  Read the ADD2 design documentation (it must exist, and Intel
must have written it, because it's your spec).  Find out what it says to
do and implement it.  It _has_ to work that way because that's the whole
point of ADD2, to have a bunch of vendors selling peripherals that
interoperate with Intel's GPUs, and have them work even in DOS.  If the
cards didn't do what the spec says to do, no one would buy them.  If the
hosts didn't do what the spec says to do, they would have only limited
market.  Either way it's reducing the amount of money someone can make,
so the Invisible Hand says that's just not going to happen.

- ajax

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH] drm/i915: Fix DDC bus selection for multifunction SDVO

[Adam Jackson]

[-- Attachment #1.1: Type: text/plain, Size: 1326 bytes --]

On Fri, 2010-04-23 at 16:16 -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.

Just to follow up on this, we're shipping the following two patches in
F13 gold kernel:

http://cvs.fedoraproject.org/viewvc/rpms/kernel/F-13/drm-intel-sdvo-fix.patch?revision=1.1&content-type=text%2Fplain&view=co
http://cvs.fedoraproject.org/viewvc/rpms/kernel/F-13/drm-intel-sdvo-fix-2.patch?revision=1.1&content-type=text%2Fplain&view=co

And, combined, they appear to fix DVI-I on at least one machine:

http://bugzilla.redhat.com/584229

I'm still skeptical of any approach that involves having to guess which
DDC bus belongs to which connector.  The only thing that seems remotely
sane to me is trusting the wiring table in the SDVO card itself.

- ajax

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [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@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@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


 

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

Re: [PATCH] drm/i915: Fix DDC bus selection for multifunction SDVO

[Adam Jackson]

[-- Attachment #1.1: Type: text/plain, Size: 1224 bytes --]

On Wed, 2010-05-05 at 15:38 +0800, ykzhao wrote:

> 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?

I don't think this is correct, though I haven't tried it.  RGB0 and
TMDS0 are both going to be the same DVI connector for DVI-I.  If they
use different DDC buses, one of them won't get DDC, ever, since DVI-I
only has one set of DDC pins.

- ajax

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH] drm/i915: Fix DDC bus selection for multifunction SDVO

[Eric Anholt]

[-- Attachment #1.1: Type: text/plain, Size: 2010 bytes --]

On Tue, 04 May 2010 14:17:28 -0400, Adam Jackson <ajax@redhat.com> wrote:
> On Fri, 2010-04-23 at 16:16 -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.
> 
> Just to follow up on this, we're shipping the following two patches in
> F13 gold kernel:
> 
> http://cvs.fedoraproject.org/viewvc/rpms/kernel/F-13/drm-intel-sdvo-fix.patch?revision=1.1&content-type=text%2Fplain&view=co
> http://cvs.fedoraproject.org/viewvc/rpms/kernel/F-13/drm-intel-sdvo-fix-2.patch?revision=1.1&content-type=text%2Fplain&view=co
> 
> And, combined, they appear to fix DVI-I on at least one machine:
> 
> http://bugzilla.redhat.com/584229
> 
> I'm still skeptical of any approach that involves having to guess which
> DDC bus belongs to which connector.  The only thing that seems remotely
> sane to me is trusting the wiring table in the SDVO card itself.

Applied those two.

The fact that we're exposing 2 connectors for this situation is bogus in
how the KMS architecture is supposed to work, right?  I mean, we've got
2 "outputs" in this encoder going to one physical connector.  The user
should see one connector, and the encoder+connector should sort out how
to program the encoder for that situation.

Would having the same DDC bus for the child devs be a good enough
indication that we should have one SDVO encoder/connector for them and
they get to light up one SDVO encoder to the connector at a time? 

[-- Attachment #1.2: Type: application/pgp-signature, Size: 197 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH] drm/i915: Fix DDC bus selection for multifunction SDVO

[Adam Jackson]

[-- Attachment #1.1: Type: text/plain, Size: 879 bytes --]

On Fri, 2010-05-07 at 14:41 -0700, Eric Anholt wrote:

> Applied those two.
> 
> The fact that we're exposing 2 connectors for this situation is bogus in
> how the KMS architecture is supposed to work, right?  I mean, we've got
> 2 "outputs" in this encoder going to one physical connector.  The user
> should see one connector, and the encoder+connector should sort out how
> to program the encoder for that situation.

That sounds more sane to me.  I don't know that we've formally taken a
stand on that in KMS, but we might as well start.

> Would having the same DDC bus for the child devs be a good enough
> indication that we should have one SDVO encoder/connector for them and
> they get to light up one SDVO encoder to the connector at a time? 

I can't think of a scenario where "shared DDC bus" wouldn't mean "shared
connector".  So, yeah.

- ajax

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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