[v2.1,3/3] drm/edid: Switch DDC when reading the EDID

Commit Message

Lukas Wunner Aug. 14, 2015, 4:28 p.m. UTC

Originally by Seth Forshee <[e-mail]>, 2012-10-04:
Some dual graphics machines support muxing the DDC separately from the
display, so make use of this functionality when reading the EDID on the
inactive GPU. Also serialize drm_get_edid() with a mutex to avoid races
on the DDC mux state.

Modified by Dave Airlie <[e-mail]>, 2012-12-22:
I can't figure out why I didn't like this, but I rewrote this [...] to
lock/unlock the ddc lines [...]. I think I'd prefer something like that
otherwise the interface got really ugly.

Modified by Lukas Wunner <[e-mail]>, 2015-03-27:
Unlock DDC lines if drm_probe_ddc() fails.

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=88861
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=61115
Tested-by: Pierre Moreau <[e-mail]>
    [MBP  5,3 2009  nvidia 9400M + 9600M GT   pre-retina]
Tested-by: Paul Hordiienko <[e-mail]>
    [MBP  6,2 2010  intel ILK + nvidia GT216  pre-retina]
Tested-by: William Brown <[e-mail]>
    [MBP  8,2 2011  intel SNB + amd turks     pre-retina]
Tested-by: Lukas Wunner <[e-mail]>
    [MBP  9,1 2012  intel IVB + nvidia GK107  pre-retina]
Tested-by: Bruno Bierbaumer <[e-mail]>
    [MBP 11,3 2013  intel HSW + nvidia GK107  retina -- work in progress]

Cc: Seth Forshee <[e-mail]>
Cc: Dave Airlie <[e-mail]>
Signed-off-by: Lukas Wunner <[e-mail]>
---
 drivers/gpu/drm/drm_edid.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

Comments

Thierry Reding Aug. 17, 2015, 10:41 a.m. UTC | #1

On Fri, Aug 14, 2015 at 06:28:35PM +0200, Lukas Wunner wrote:
> Originally by Seth Forshee <[e-mail]>, 2012-10-04:
> Some dual graphics machines support muxing the DDC separately from the
> display, so make use of this functionality when reading the EDID on the
> inactive GPU. Also serialize drm_get_edid() with a mutex to avoid races
> on the DDC mux state.
> 
> Modified by Dave Airlie <[e-mail]>, 2012-12-22:
> I can't figure out why I didn't like this, but I rewrote this [...] to
> lock/unlock the ddc lines [...]. I think I'd prefer something like that
> otherwise the interface got really ugly.
> 
> Modified by Lukas Wunner <[e-mail]>, 2015-03-27:
> Unlock DDC lines if drm_probe_ddc() fails.
> 
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=88861
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=61115
> Tested-by: Pierre Moreau <[e-mail]>
>     [MBP  5,3 2009  nvidia 9400M + 9600M GT   pre-retina]
> Tested-by: Paul Hordiienko <[e-mail]>
>     [MBP  6,2 2010  intel ILK + nvidia GT216  pre-retina]
> Tested-by: William Brown <[e-mail]>
>     [MBP  8,2 2011  intel SNB + amd turks     pre-retina]
> Tested-by: Lukas Wunner <[e-mail]>
>     [MBP  9,1 2012  intel IVB + nvidia GK107  pre-retina]
> Tested-by: Bruno Bierbaumer <[e-mail]>
>     [MBP 11,3 2013  intel HSW + nvidia GK107  retina -- work in progress]
> 
> Cc: Seth Forshee <[e-mail]>
> Cc: Dave Airlie <[e-mail]>
> Signed-off-by: Lukas Wunner <[e-mail]>
> ---
>  drivers/gpu/drm/drm_edid.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index e6e05bb..cdb2fa1 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -32,6 +32,7 @@
>  #include <linux/hdmi.h>
>  #include <linux/i2c.h>
>  #include <linux/module.h>
> +#include <linux/vga_switcheroo.h>
>  #include <drm/drmP.h>
>  #include <drm/drm_edid.h>
>  #include <drm/drm_displayid.h>
> @@ -1377,13 +1378,21 @@ struct edid *drm_get_edid(struct drm_connector *connector,
>  			  struct i2c_adapter *adapter)
>  {
>  	struct edid *edid;
> +	struct pci_dev *pdev = connector->dev->pdev;
>  
> -	if (!drm_probe_ddc(adapter))
> +	vga_switcheroo_lock_ddc(pdev);
> +
> +	if (!drm_probe_ddc(adapter)) {
> +		vga_switcheroo_unlock_ddc(pdev);
>  		return NULL;
> +	}
>  
>  	edid = drm_do_get_edid(connector, drm_do_probe_ddc_edid, adapter);
>  	if (edid)
>  		drm_get_displayid(connector, edid);
> +
> +	vga_switcheroo_unlock_ddc(pdev);
> +
>  	return edid;
>  }
>  EXPORT_SYMBOL(drm_get_edid);

I think this is backwards and it'd be more explicit (though I suspect
slightly more work for this patch) to add a separate helper that does
the VGA switcheroo wrapping rather than have this in drm_get_edid()
where essentially every driver will go through the motions even if it
doesn't remotely support switcheroo.

Thierry

Lukas Wunner Aug. 17, 2015, 8:24 p.m. UTC | #2

Hi Thierry,

On Mon, Aug 17, 2015 at 12:41:32PM +0200, Thierry Reding wrote:
> On Fri, Aug 14, 2015 at 06:28:35PM +0200, Lukas Wunner wrote:
> > @@ -1377,13 +1378,21 @@ struct edid *drm_get_edid(struct drm_connector *connector,
> >  			  struct i2c_adapter *adapter)
> >  {
> >  	struct edid *edid;
> > +	struct pci_dev *pdev = connector->dev->pdev;
> >  
> > -	if (!drm_probe_ddc(adapter))
> > +	vga_switcheroo_lock_ddc(pdev);
> > +
> > +	if (!drm_probe_ddc(adapter)) {
> > +		vga_switcheroo_unlock_ddc(pdev);
> >  		return NULL;
> > +	}
> >  
> >  	edid = drm_do_get_edid(connector, drm_do_probe_ddc_edid, adapter);
> >  	if (edid)
> >  		drm_get_displayid(connector, edid);
> > +
> > +	vga_switcheroo_unlock_ddc(pdev);
> > +
> >  	return edid;
> >  }
> >  EXPORT_SYMBOL(drm_get_edid);
> 
> I think this is backwards and it'd be more explicit (though I suspect
> slightly more work for this patch) to add a separate helper that does
> the VGA switcheroo wrapping rather than have this in drm_get_edid()
> where essentially every driver will go through the motions even if it
> doesn't remotely support switcheroo.

This is also something I carried over from Seth Forshee's and
Dave Airlie's patches and I think their intention was precisely
to do this in a centralized way in one of the generic functions,
rather than having to modify each driver.

For drivers without vga_switcheroo support, the additional cost
amounts to one mutex_lock/unlock (because there's no handler
registered and vga_switcheroo_lock_ddc bails out immediately
if there's none).

As an example why I think this approach was taken: Let's say that
Tegra or other mobile SoCs have dual GPUs in the future, kind of
like ARM big.LITTLE for graphics. We would potentially support
those devices out of the box without having to modify the driver
to call drm_get_edid_switcheroo() rather than drm_get_edid().
That was kind of the idea I guess.

On the other hand, a case could be made for your suggested approach
as well: The proxying stuff will clutter drm_get_edid() and
drm_dp_dpcd_read() with even more vga_switcheroo calls,
as can be seen in experimental patch 20:
http://lists.freedesktop.org/archives/dri-devel/2015-August/088154.html

Also, to constrain proxying to eDP, I had to amend drm_dp_aux with
a connector attribute so that the connector type can be checked in
drm_dp_dpcd_read() (patch 19):
http://lists.freedesktop.org/archives/dri-devel/2015-August/088172.html

With your approach, I think this will be unnecessary because the
functions in the drivers which would call drm_get_edid_switcheroo()
would already know the connector type.

So once again, a case can be made for either approach, I feel like
I'm not in a position to properly judge this, and I kindly ask that
you or another maintainer make that decision based on the additional
information I've provided above. I'll then gladly adjust the patch.

Thanks,

Lukas

Jani Nikula Aug. 18, 2015, 7:02 a.m. UTC | #3

On Fri, 14 Aug 2015, Lukas Wunner <[e-mail]> wrote:
> Originally by Seth Forshee <[e-mail]>, 2012-10-04:
> Some dual graphics machines support muxing the DDC separately from the
> display, so make use of this functionality when reading the EDID on the
> inactive GPU. Also serialize drm_get_edid() with a mutex to avoid races
> on the DDC mux state.

At a glance, all the referenced bugs have LVDS displays. Is this
supposed to work for eDP as well? How about muxing and locking of DPCD
access?

BR,
Jani.





>
> Modified by Dave Airlie <[e-mail]>, 2012-12-22:
> I can't figure out why I didn't like this, but I rewrote this [...] to
> lock/unlock the ddc lines [...]. I think I'd prefer something like that
> otherwise the interface got really ugly.
>
> Modified by Lukas Wunner <[e-mail]>, 2015-03-27:
> Unlock DDC lines if drm_probe_ddc() fails.
>
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=88861
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=61115
> Tested-by: Pierre Moreau <[e-mail]>
>     [MBP  5,3 2009  nvidia 9400M + 9600M GT   pre-retina]
> Tested-by: Paul Hordiienko <[e-mail]>
>     [MBP  6,2 2010  intel ILK + nvidia GT216  pre-retina]
> Tested-by: William Brown <[e-mail]>
>     [MBP  8,2 2011  intel SNB + amd turks     pre-retina]
> Tested-by: Lukas Wunner <[e-mail]>
>     [MBP  9,1 2012  intel IVB + nvidia GK107  pre-retina]
> Tested-by: Bruno Bierbaumer <[e-mail]>
>     [MBP 11,3 2013  intel HSW + nvidia GK107  retina -- work in progress]
>
> Cc: Seth Forshee <[e-mail]>
> Cc: Dave Airlie <[e-mail]>
> Signed-off-by: Lukas Wunner <[e-mail]>
> ---
>  drivers/gpu/drm/drm_edid.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index e6e05bb..cdb2fa1 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -32,6 +32,7 @@
>  #include <linux/hdmi.h>
>  #include <linux/i2c.h>
>  #include <linux/module.h>
> +#include <linux/vga_switcheroo.h>
>  #include <drm/drmP.h>
>  #include <drm/drm_edid.h>
>  #include <drm/drm_displayid.h>
> @@ -1377,13 +1378,21 @@ struct edid *drm_get_edid(struct drm_connector *connector,
>  			  struct i2c_adapter *adapter)
>  {
>  	struct edid *edid;
> +	struct pci_dev *pdev = connector->dev->pdev;
>  
> -	if (!drm_probe_ddc(adapter))
> +	vga_switcheroo_lock_ddc(pdev);
> +
> +	if (!drm_probe_ddc(adapter)) {
> +		vga_switcheroo_unlock_ddc(pdev);
>  		return NULL;
> +	}
>  
>  	edid = drm_do_get_edid(connector, drm_do_probe_ddc_edid, adapter);
>  	if (edid)
>  		drm_get_displayid(connector, edid);
> +
> +	vga_switcheroo_unlock_ddc(pdev);
> +
>  	return edid;
>  }
>  EXPORT_SYMBOL(drm_get_edid);
> -- 
> 2.1.0
>
> _______________________________________________
> dri-devel mailing list
> [e-mail]
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

Lukas Wunner Aug. 18, 2015, 2:16 p.m. UTC | #4

Hi Jani,

On Tue, Aug 18, 2015 at 10:02:43AM +0300, Jani Nikula wrote:
> On Fri, 14 Aug 2015, Lukas Wunner <[e-mail]> wrote:
> > Originally by Seth Forshee <[e-mail]>, 2012-10-04:
> > Some dual graphics machines support muxing the DDC separately from the
> > display, so make use of this functionality when reading the EDID on the
> > inactive GPU. Also serialize drm_get_edid() with a mutex to avoid races
> > on the DDC mux state.
> 
> At a glance, all the referenced bugs have LVDS displays.

For the original reporters of the bugs that's true, but another user
jumped on the bandwagon who has a retina and that one uses eDP:
https://bugzilla.kernel.org/show_bug.cgi?id=88861#c7

The pre-retinas with dual GPUs (produced 2008-2013) used LVDS.
The retinas (2012-present) use eDP. (Because the dotclock required
for 2880x1800 (337750 kHz) is not supported by LVDS even with dual
channels, max is 224 MHz.)


> Is this supposed to work for eDP as well?

The 3 patches I've posted with v2.1 enable GPU switching only on LVDS,
and only under certain circumstances. (apple-gmux must register before
the inactive GPU's driver and the inactive GPU may not be an Nvidia.)

The 22 patches I've posted with v2 enable GPU switching on LVDS under
any circumstances and also contain experimental commits that preview
how we're tackling eDP support.


> How about muxing and locking of DPCD access?

Muxing the AUX channel separately from the main link is apparently
unsupported by the gmux controller. See the cover letter of v2 for
details (scroll down to "The long journey towards retina support"):
http://lists.freedesktop.org/archives/dri-devel/2015-August/088156.html

I'm trying to solve this by emulating muxing in software: Read access
to the DPCD is proxied via the active GPU. However the drivers also
try to write to the DPCD and abort link training if the write fails,
so in those situations I check if the contents of DPCD match with
what the driver wants to write, and if so, signal success without
writing anything. E.g. nouveau tries to configure 4 lanes at 270000
and i915 configures the same, so no point in writing to DPCD.
Daniel expressed concerns that the inactive GPU might actually write
to DPCD but there's no code let it do that (deliberately so as it
would be unsafe):
http://lists.freedesktop.org/archives/dri-devel/2015-August/088154.html
http://lists.freedesktop.org/archives/dri-devel/2015-July/088173.html


Best regards,

Lukas

Daniel Vetter Aug. 25, 2015, 8 a.m. UTC | #5

On Mon, Aug 17, 2015 at 10:24:32PM +0200, Lukas Wunner wrote:
> Hi Thierry,
> 
> On Mon, Aug 17, 2015 at 12:41:32PM +0200, Thierry Reding wrote:
> > On Fri, Aug 14, 2015 at 06:28:35PM +0200, Lukas Wunner wrote:
> > > @@ -1377,13 +1378,21 @@ struct edid *drm_get_edid(struct drm_connector *connector,
> > >  			  struct i2c_adapter *adapter)
> > >  {
> > >  	struct edid *edid;
> > > +	struct pci_dev *pdev = connector->dev->pdev;
> > >  
> > > -	if (!drm_probe_ddc(adapter))
> > > +	vga_switcheroo_lock_ddc(pdev);
> > > +
> > > +	if (!drm_probe_ddc(adapter)) {
> > > +		vga_switcheroo_unlock_ddc(pdev);
> > >  		return NULL;
> > > +	}
> > >  
> > >  	edid = drm_do_get_edid(connector, drm_do_probe_ddc_edid, adapter);
> > >  	if (edid)
> > >  		drm_get_displayid(connector, edid);
> > > +
> > > +	vga_switcheroo_unlock_ddc(pdev);
> > > +
> > >  	return edid;
> > >  }
> > >  EXPORT_SYMBOL(drm_get_edid);
> > 
> > I think this is backwards and it'd be more explicit (though I suspect
> > slightly more work for this patch) to add a separate helper that does
> > the VGA switcheroo wrapping rather than have this in drm_get_edid()
> > where essentially every driver will go through the motions even if it
> > doesn't remotely support switcheroo.
> 
> This is also something I carried over from Seth Forshee's and
> Dave Airlie's patches and I think their intention was precisely
> to do this in a centralized way in one of the generic functions,
> rather than having to modify each driver.
> 
> For drivers without vga_switcheroo support, the additional cost
> amounts to one mutex_lock/unlock (because there's no handler
> registered and vga_switcheroo_lock_ddc bails out immediately
> if there's none).
> 
> As an example why I think this approach was taken: Let's say that
> Tegra or other mobile SoCs have dual GPUs in the future, kind of
> like ARM big.LITTLE for graphics. We would potentially support
> those devices out of the box without having to modify the driver
> to call drm_get_edid_switcheroo() rather than drm_get_edid().
> That was kind of the idea I guess.
> 
> On the other hand, a case could be made for your suggested approach
> as well: The proxying stuff will clutter drm_get_edid() and
> drm_dp_dpcd_read() with even more vga_switcheroo calls,
> as can be seen in experimental patch 20:
> http://lists.freedesktop.org/archives/dri-devel/2015-August/088154.html
> 
> Also, to constrain proxying to eDP, I had to amend drm_dp_aux with
> a connector attribute so that the connector type can be checked in
> drm_dp_dpcd_read() (patch 19):
> http://lists.freedesktop.org/archives/dri-devel/2015-August/088172.html
> 
> With your approach, I think this will be unnecessary because the
> functions in the drivers which would call drm_get_edid_switcheroo()
> would already know the connector type.
> 
> So once again, a case can be made for either approach, I feel like
> I'm not in a position to properly judge this, and I kindly ask that
> you or another maintainer make that decision based on the additional
> information I've provided above. I'll then gladly adjust the patch.

Generally we make helpers opt-in and not opt-out since there's always the
oddball one out there which needs some additional code. The addition of
drm_do_get_edid is imo clear precedence that there's lots of fun things
going on when fetching edid. Hence I'm also voting for a separate helper
to do ddc switching.

That will also clear up the confusion with drm_dp_aux, adding a
drm_connector there feels wrong since not every dp_aux line has a
connector (e.g. for dp mst). If we can lift this relation out into drivers
(where this is known) that seems cleaner.
-Daniel

Patch

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index e6e05bb..cdb2fa1 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -32,6 +32,7 @@ 
 #include <linux/hdmi.h>
 #include <linux/i2c.h>
 #include <linux/module.h>
+#include <linux/vga_switcheroo.h>
 #include <drm/drmP.h>
 #include <drm/drm_edid.h>
 #include <drm/drm_displayid.h>
@@ -1377,13 +1378,21 @@  struct edid *drm_get_edid(struct drm_connector *connector,
 			  struct i2c_adapter *adapter)
 {
 	struct edid *edid;
+	struct pci_dev *pdev = connector->dev->pdev;
 
-	if (!drm_probe_ddc(adapter))
+	vga_switcheroo_lock_ddc(pdev);
+
+	if (!drm_probe_ddc(adapter)) {
+		vga_switcheroo_unlock_ddc(pdev);
 		return NULL;
+	}
 
 	edid = drm_do_get_edid(connector, drm_do_probe_ddc_edid, adapter);
 	if (edid)
 		drm_get_displayid(connector, edid);
+
+	vga_switcheroo_unlock_ddc(pdev);
+
 	return edid;
 }
 EXPORT_SYMBOL(drm_get_edid);