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