[PATCH v4 1/2] drm/dp: Store the drm_connector device pointer on the helper.
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
[PATCH v4 1/2] drm/dp: Store the drm_connector device pointer on the helper.
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.
--
Rafael
> >
> > >
> > > Thanks for pointing out that I missed properly delayering this.
> > > -Daniel
> > >
> > > >
> > > >
> > > > Best regards,
> > > >
> > > > Lukas
> > > >
> > > >
> > > > >
> > > > > Signed-off-by: Rafael Antognolli
> > > > > ---
> > > > > 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
> > > > >
> > > > >
[PATCH v4 1/2] drm/dp: Store the drm_connector device pointer on the helper.
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).
-Daniel
>
> >
> > Thanks for pointing out that I missed properly delayering this.
> > -Daniel
> >
> > >
> > >
> > > Best regards,
> > >
> > > Lukas
> > >
> > >
> > > >
> > > > Signed-off-by: Rafael Antognolli
> > > > ---
> > > > 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 at lists.freedesktop.org
> > > > http://lists.freedesktop.org/mailman/listinfo/dri-devel
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
[PATCH v4 1/2] drm/dp: Store the drm_connector device pointer on the helper.
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
> > > ---
> > > 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 at lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
[PATCH v4 1/2] drm/dp: Store the drm_connector device pointer on the helper.
On Tue, Sep 29, 2015 at 02:49:20PM +0200, Lukas Wunner wrote:
> Hi Rafael,
>
> 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.
Thanks for pointing out that I missed properly delayering this.
-Daniel
>
>
> Best regards,
>
> Lukas
>
>
> >
> > Signed-off-by: Rafael Antognolli
> > ---
> > 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 at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
[PATCH v4 1/2] drm/dp: Store the drm_connector device pointer on the helper.
Hi Rafael,
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
Best regards,
Lukas
>
> Signed-off-by: Rafael Antognolli
> ---
> 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 at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
[Intel-gfx] [PATCH v4 1/2] drm/dp: Store the drm_connector device pointer on the helper.
Hi Rafael, [auto build test results on v4.3-rc3 -- if it's inappropriate base, please ignore] reproduce: make htmldocs All warnings (new ones prefixed by >>): drivers/gpu/drm/i915/i915_irq.c:491: warning: No description found for parameter 'dev' drivers/gpu/drm/i915/i915_irq.c:2217: warning: No description found for parameter 'dev' drivers/gpu/drm/i915/i915_irq.c:2397: warning: No description found for parameter 'wedged' drivers/gpu/drm/i915/i915_irq.c:2397: warning: No description found for parameter 'fmt' include/drm/drm_crtc.h:310: warning: No description found for parameter 'mode_blob' include/drm/drm_crtc.h:748: warning: No description found for parameter 'tile_blob_ptr' include/drm/drm_crtc.h:787: warning: No description found for parameter 'rotation' include/drm/drm_crtc.h:883: warning: No description found for parameter 'mutex' include/drm/drm_crtc.h:883: warning: No description found for parameter 'helper_private' include/drm/drm_crtc.h:931: warning: Excess struct/union/enum/typedef member 'base' description in 'drm_bridge' include/drm/drm_crtc.h:1169: warning: No description found for parameter 'tile_idr' include/drm/drm_crtc.h:1169: warning: No description found for parameter 'delayed_event' include/drm/drm_crtc.h:1169: warning: No description found for parameter 'edid_property' include/drm/drm_crtc.h:1169: warning: No description found for parameter 'dpms_property' include/drm/drm_crtc.h:1169: warning: No description found for parameter 'path_property' include/drm/drm_crtc.h:1169: warning: No description found for parameter 'tile_property' include/drm/drm_crtc.h:1169: warning: No description found for parameter 'plane_type_property' include/drm/drm_crtc.h:1169: warning: No description found for parameter 'rotation_property' include/drm/drm_crtc.h:1169: warning: No description found for parameter 'prop_src_x' include/drm/drm_crtc.h:1169: warning: No description found for parameter 'prop_src_y' include/drm/drm_crtc.h:1169: warning: No description found for parameter 'prop_src_w' include/drm/drm_crtc.h:1169: warning: No description found for parameter 'prop_src_h' include/drm/drm_crtc.h:1169: warning: No description found for parameter 'prop_crtc_x' include/drm/drm_crtc.h:1169: warning: No description found for parameter 'prop_crtc_y' include/drm/drm_crtc.h:1169: warning: No description found for parameter 'prop_crtc_w' include/drm/drm_crtc.h:1169: warning: No description found for parameter 'prop_crtc_h' include/drm/drm_crtc.h:1169: warning: No description found for parameter 'prop_fb_id' include/drm/drm_crtc.h:1169: warning: No description found for parameter 'prop_crtc_id' include/drm/drm_crtc.h:1169: warning: No description found for parameter 'prop_active' include/drm/drm_crtc.h:1169: warning: No description found for parameter 'prop_mode_id' include/drm/drm_crtc.h:1169: warning: No description found for parameter 'dvi_i_subconnector_property' include/drm/drm_crtc.h:1169: warning: No description found for parameter 'dvi_i_select_subconnector_property' include/drm/drm_crtc.h:1169: warning: No description found for parameter 'tv_subconnector_property' include/drm/drm_crtc.h:1169: warning: No description found for parameter 'tv_select_subconnector_property' include/drm/drm_crtc.h:1169: warning: No description found for parameter 'tv_mode_property' include/drm/drm_crtc.h:1169: warning: No description found for parameter 'tv_left_margin_property' include/drm/drm_crtc.h:1169: warning: No description found for parameter 'tv_right_margin_property' include/drm/drm_crtc.h:1169: warning: No description found for parameter 'tv_top_margin_property' include/drm/drm_crtc.h:1169: warning: No description found for parameter 'tv_bottom_margin_property' include/drm/drm_crtc.h:1169: warning: No description found for parameter 'tv_brightness_property' include/drm/drm_crtc.h:1169: warning: No description found for parameter 'tv_contrast_property' include/drm/drm_crtc.h:1169: warning: No description found for parameter 'tv_flicker_reduction_property' include/drm/drm_crtc.h:1169: warning: No description found for parameter 'tv_overscan_property' include/drm/drm_crtc.h:1169: warning: No description found for parameter 'tv_saturation_property' include/drm/drm_crtc.h:1169: warning: No description found for parameter 'tv_hue_property' include/drm/drm_crtc.h:1169: warning: No description found for parameter 'scaling_mode_property' include/drm/drm_crtc.h:1169: warning: No description found for parameter 'aspect_ratio_property' include/drm/drm_crtc.h:1169: warning: No description found for parameter 'dirty_info_property' include/drm/drm_crtc.h:1169: warning: No description found for parameter 'suggested_x_property' include/drm/drm_crtc.h:1169: warning: No description found for parameter 'suggested_y_property'
[PATCH v4 1/2] drm/dp: Store the drm_connector device pointer on the helper.
On Tue, Sep 29, 2015 at 02:49:20PM +0200, Lukas Wunner wrote:
> Hi Rafael,
>
> 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!
I'm sorry, I don't follow this list as closely yet, so I didn't see that
patch there, otherwise I would have talked to Daniel about it in the
first place.
Thanks,
Rafael
> Source:
> http://lists.freedesktop.org/archives/dri-devel/2015-August/089108.html
>
>
> Best regards,
>
> Lukas
>
>
> >
> > Signed-off-by: Rafael Antognolli
> > ---
> > 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 at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v4 1/2] drm/dp: Store the drm_connector device pointer on the helper.
This is useful to determine which connector owns this AUX channel.
Signed-off-by: Rafael Antognolli
---
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
