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.) > > Thanks for pointing out that I missed properly delayering this. > -Daniel > > > > > > > Best regards, > > > > Lukas > > > > > > > > > > Signed-off-by: Rafael Antognolli <rafael.antognolli@xxxxxxxxx> > > > --- > > > drivers/gpu/drm/i915/intel_dp.c | 1 + > > > include/drm/drm_dp_helper.h | 1 + > > > 2 files changed, 2 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > > > index 77f7330..f90439d 100644 > > > --- a/drivers/gpu/drm/i915/intel_dp.c > > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > > @@ -1079,6 +1079,7 @@ intel_dp_aux_init(struct intel_dp *intel_dp, struct intel_connector *connector) > > > > > > intel_dp->aux.name = name; > > > intel_dp->aux.dev = dev->dev; > > > + intel_dp->aux.connector = connector->base.kdev; > > > intel_dp->aux.transfer = intel_dp_aux_transfer; > > > > > > DRM_DEBUG_KMS("registering %s bus for %s\n", name, > > > diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h > > > index 9ec4716..e009b5d 100644 > > > --- a/include/drm/drm_dp_helper.h > > > +++ b/include/drm/drm_dp_helper.h > > > @@ -702,6 +702,7 @@ struct drm_dp_aux { > > > const char *name; > > > struct i2c_adapter ddc; > > > struct device *dev; > > > + struct device *connector; > > > struct mutex hw_mutex; > > > ssize_t (*transfer)(struct drm_dp_aux *aux, > > > struct drm_dp_aux_msg *msg); > > > -- > > > 2.4.3 > > > > > > _______________________________________________ > > > dri-devel mailing list > > > dri-devel@xxxxxxxxxxxxxxxxxxxxx > > > http://lists.freedesktop.org/mailman/listinfo/dri-devel > > -- > 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
