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

[Daniel Vetter]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch