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