Re: [PATCH v4 1/2] drm/dp: Store the drm_connector device pointer on the helper.

[Daniel Vetter <daniel@ffwll.ch>]

On Fri, Oct 09, 2015 at 02:50:10PM -0700, Rafael Antognolli wrote:
> On Tue, Sep 29, 2015 at 06:25:44PM +0200, Daniel Vetter wrote:
> > On Tue, Sep 29, 2015 at 05:27:33PM +0200, Lukas Wunner wrote:
> > > Hi Daniel,
> > > 
> > > On Tue, Sep 29, 2015 at 05:04:03PM +0200, Daniel Vetter wrote:
> > > > On Tue, Sep 29, 2015 at 02:49:20PM +0200, Lukas Wunner wrote:
> > > > > On Mon, Sep 28, 2015 at 04:45:35PM -0700, Rafael Antognolli wrote:
> > > > > > This is useful to determine which connector owns this AUX channel.
> > > > > 
> > > > > WTF? I posted a patch in August which does exactly that:
> > > > > http://lists.freedesktop.org/archives/dri-devel/2015-August/088172.html
> > > > > 
> > > > > Can also be pulled in from this git repo:
> > > > > https://github.com/l1k/linux/commit/b78b38d53fc0fc4fa0f6acf699b0fcad56ec1fe6
> > > > > 
> > > > > My patch has the advantage that it updates all the drivers which use
> > > > > drm_dp_aux to fill that attribute. Yours only updates i915.
> > > > > 
> > > > > Daniel Vetter criticized storing a drm_connector pointer in drm_dp_aux,
> > > > > quote:
> > > > > 
> > > > > "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."
> > > > > 
> > > > > So now Intel itself does precisely what Daniel criticized? Confusing!
> > > > > 
> > > > > Source:
> > > > > http://lists.freedesktop.org/archives/dri-devel/2015-August/089108.html
> > > > 
> > > > Critism is still valid, and thinking about this again a cleaner solution
> > > > would be to just have a correct parent/child relationship in the device
> > > > hirarchy. I.e. add a struct device *parent to the aux channel structure
> > > > which should point to the right connector.
> > > 
> > > We already have that:
> > > 
> > > struct drm_dp_aux {
> > > 	const char *name;
> > > 	struct i2c_adapter ddc;
> > > 	struct device *dev;				<-----------
> > > 	struct mutex hw_mutex;
> > > 	ssize_t (*transfer)(struct drm_dp_aux *aux,
> > > 			    struct drm_dp_aux_msg *msg);
> > > 	unsigned i2c_nack_count, i2c_defer_count;
> > > };
> > > 
> > > What Rafael is struggling with is that you cannot unambiguously
> > > get from drm_dp_aux->dev to the drm_connector. (The drm_device
> > > may have multiple drm_connectors with type
> > > DRM_MODE_CONNECTOR_DisplayPort.)
> > 
> > What I meant to say is that we don't need that, if instead of filling in
> > the overall dev in dp_aux->dev we fill in the connector sysfs device
> > thing. The we have proper nesting, like with i2c buses. And then there's
> > no need for a connector property in sysfs to show that link (which should
> > be done with a proper sysfs link anyway).
> 
> OK, I sent a new version, which does not add a new *connector pointer,
> and uses the dev pointer on the struct to store the drm_connector
> device, instead of the drm_device device. Is that what you meant? In
> any case, as I mention on the patch, it is already how some drivers do,
> while others store the drm_device.
> 
> This leaves the aux device, for instance in my case, at:
> 
> /sys/class/drm/card0/card0-eDP-1/drm_dp_aux0
> 
> If this is what you wanted, I can send other patches to the proper
> mailing lists, trying to update other drivers.

Yeah that's kinda what I had in mind, makes the nesting more obvious.
Especially for mst hub dp aux channels.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx