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

From: Lukas Wunner <lukas@wunner.de>
To: Thierry Reding <thierry.reding@gmail.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>,
	Seth Forshee <seth.forshee@canonical.com>,
	dri-devel@lists.freedesktop.org, Dave Airlie <airlied@redhat.com>
Subject: Re: [PATCH v2.1 3/3] drm/edid: Switch DDC when reading the EDID
Date: Mon, 17 Aug 2015 22:24:32 +0200	[thread overview]
Message-ID: <20150817202432.GA6813@wunner.de> (raw)
In-Reply-To: <20150817104130.GC8453@ulmo.nvidia.com>

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel