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