| Message ID | 1449183242-2608-4-git-send-email-rafael.antognolli@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Rafael, On Thu, Dec 03, 2015 at 02:54:02PM -0800, Rafael Antognolli wrote: > So far, the i915 driver and some other drivers set it to the drm_device, > which doesn't allow one to know which DP a given aux channel is related > to. Changing this to be the drm_connector provides proper nesting, still > allowing one to get the drm_device from it. Some drivers already set it > to the drm_connector. > > This also removes the need to add a sysfs link for the i2c device under > the connector, as it will already be there. > > v2: Two minor nits, I think this should be "v9" instead of "v2" because this was newly added since v8, and the subject should be prefixed "drm/i915" (or "drm/i915/dp" or whatever) instead of "drm/dp" because this only touches i915 and not drm core. > - As a side effect, drm_dp_aux_unregister() must be called before > intel_connector_unregister(), as both the aux.dev and the i2c adapter > dev are children of the drm_connector device now. Calling > drm_dp_aux_unregister() before prevents them from being destroyed > twice. I haven't verified what Ville wrote (that there are WARNs because of this ordering issue), but if this is true then we need to make sure all other drivers get the order right, otherwise they'd spew WARNs as well once this gets merged. I've checked that for every driver and the only one affected is radeon. A fix is now in my GitHub repo, feel free to include the commit in your series if you want to. I haven't been able to actually test it though as I don't have a radeon card: https://github.com/l1k/linux/commit/485e197a7cac88e24348150526988149428feeaa > > Signed-off-by: Rafael Antognolli <rafael.antognolli@intel.com> > --- > drivers/gpu/drm/i915/intel_dp.c | 22 ++++------------------ > 1 file changed, 4 insertions(+), 18 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index f2bfca0..515893c 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -1162,14 +1162,12 @@ static void intel_aux_reg_init(struct intel_dp *intel_dp) > static void > intel_dp_aux_fini(struct intel_dp *intel_dp) > { > - drm_dp_aux_unregister(&intel_dp->aux); > kfree(intel_dp->aux.name); > } Hm, instead of moving the single call of drm_dp_aux_unregister() to intel_dp_connector_unregister() I think it would be more clear to call intel_dp_aux_fini() from intel_dp_connector_unregister() and remove its invocation from intel_dp_encoder_destroy(). Ville introduced intel_dp_aux_fini() very recently with a121f4e5fae5, he should probably have a say on how to solve this. Kind regards, Lukas > > static int > intel_dp_aux_init(struct intel_dp *intel_dp, struct intel_connector *connector) > { > - struct drm_device *dev = intel_dp_to_dev(intel_dp); > struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); > enum port port = intel_dig_port->port; > int ret; > @@ -1180,7 +1178,7 @@ intel_dp_aux_init(struct intel_dp *intel_dp, struct intel_connector *connector) > if (!intel_dp->aux.name) > return -ENOMEM; > > - intel_dp->aux.dev = dev->dev; > + intel_dp->aux.dev = connector->base.kdev; > intel_dp->aux.transfer = intel_dp_aux_transfer; > > DRM_DEBUG_KMS("registering %s bus for %s\n", > @@ -1195,27 +1193,15 @@ intel_dp_aux_init(struct intel_dp *intel_dp, struct intel_connector *connector) > return ret; > } > > - ret = sysfs_create_link(&connector->base.kdev->kobj, > - &intel_dp->aux.ddc.dev.kobj, > - intel_dp->aux.ddc.dev.kobj.name); > - if (ret < 0) { > - DRM_ERROR("sysfs_create_link() for %s failed (%d)\n", > - intel_dp->aux.name, ret); > - intel_dp_aux_fini(intel_dp); > - return ret; > - } > - > return 0; > } > > static void > intel_dp_connector_unregister(struct intel_connector *intel_connector) > { > - struct intel_dp *intel_dp = intel_attached_dp(&intel_connector->base); > - > - if (!intel_connector->mst_port) > - sysfs_remove_link(&intel_connector->base.kdev->kobj, > - intel_dp->aux.ddc.dev.kobj.name); > + struct intel_dp *intel_dp = > + enc_to_intel_dp(&intel_connector->encoder->base); > + drm_dp_aux_unregister(&intel_dp->aux); > intel_connector_unregister(intel_connector); > } > > -- > 2.4.3 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi Lukas, Sorry for the late answer, I went on vacation and then was busy with something else. Anyway, I tried to address all comments (yours and Daniel's) on a new version. See some comments below. On Sun, Dec 06, 2015 at 05:08:49PM +0100, Lukas Wunner wrote: > Hi Rafael, > > On Thu, Dec 03, 2015 at 02:54:02PM -0800, Rafael Antognolli wrote: > > So far, the i915 driver and some other drivers set it to the drm_device, > > which doesn't allow one to know which DP a given aux channel is related > > to. Changing this to be the drm_connector provides proper nesting, still > > allowing one to get the drm_device from it. Some drivers already set it > > to the drm_connector. > > > > This also removes the need to add a sysfs link for the i2c device under > > the connector, as it will already be there. > > > > v2: > > Two minor nits, I think this should be "v9" instead of "v2" because > this was newly added since v8, and the subject should be prefixed > "drm/i915" (or "drm/i915/dp" or whatever) instead of "drm/dp" because > this only touches i915 and not drm core. > > > > - As a side effect, drm_dp_aux_unregister() must be called before > > intel_connector_unregister(), as both the aux.dev and the i2c adapter > > dev are children of the drm_connector device now. Calling > > drm_dp_aux_unregister() before prevents them from being destroyed > > twice. > > I haven't verified what Ville wrote (that there are WARNs because of > this ordering issue), but if this is true then we need to make sure > all other drivers get the order right, otherwise they'd spew WARNs > as well once this gets merged. > > I've checked that for every driver and the only one affected is radeon. > A fix is now in my GitHub repo, feel free to include the commit in your > series if you want to. I haven't been able to actually test it though > as I don't have a radeon card: > https://github.com/l1k/linux/commit/485e197a7cac88e24348150526988149428feeaa > Nice, I added it to the series, though I also don't have a radeon to test it. > > > > Signed-off-by: Rafael Antognolli <rafael.antognolli@intel.com> > > --- > > drivers/gpu/drm/i915/intel_dp.c | 22 ++++------------------ > > 1 file changed, 4 insertions(+), 18 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > > index f2bfca0..515893c 100644 > > --- a/drivers/gpu/drm/i915/intel_dp.c > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > @@ -1162,14 +1162,12 @@ static void intel_aux_reg_init(struct intel_dp *intel_dp) > > static void > > intel_dp_aux_fini(struct intel_dp *intel_dp) > > { > > - drm_dp_aux_unregister(&intel_dp->aux); > > kfree(intel_dp->aux.name); > > } > > Hm, instead of moving the single call of drm_dp_aux_unregister() > to intel_dp_connector_unregister() I think it would be more clear > to call intel_dp_aux_fini() from intel_dp_connector_unregister() > and remove its invocation from intel_dp_encoder_destroy(). > > Ville introduced intel_dp_aux_fini() very recently with a121f4e5fae5, > he should probably have a say on how to solve this. I makes sense to me, so I did what you suggest. Thanks for the review, Rafael > > > > static int > > intel_dp_aux_init(struct intel_dp *intel_dp, struct intel_connector *connector) > > { > > - struct drm_device *dev = intel_dp_to_dev(intel_dp); > > struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); > > enum port port = intel_dig_port->port; > > int ret; > > @@ -1180,7 +1178,7 @@ intel_dp_aux_init(struct intel_dp *intel_dp, struct intel_connector *connector) > > if (!intel_dp->aux.name) > > return -ENOMEM; > > > > - intel_dp->aux.dev = dev->dev; > > + intel_dp->aux.dev = connector->base.kdev; > > intel_dp->aux.transfer = intel_dp_aux_transfer; > > > > DRM_DEBUG_KMS("registering %s bus for %s\n", > > @@ -1195,27 +1193,15 @@ intel_dp_aux_init(struct intel_dp *intel_dp, struct intel_connector *connector) > > return ret; > > } > > > > - ret = sysfs_create_link(&connector->base.kdev->kobj, > > - &intel_dp->aux.ddc.dev.kobj, > > - intel_dp->aux.ddc.dev.kobj.name); > > - if (ret < 0) { > > - DRM_ERROR("sysfs_create_link() for %s failed (%d)\n", > > - intel_dp->aux.name, ret); > > - intel_dp_aux_fini(intel_dp); > > - return ret; > > - } > > - > > return 0; > > } > > > > static void > > intel_dp_connector_unregister(struct intel_connector *intel_connector) > > { > > - struct intel_dp *intel_dp = intel_attached_dp(&intel_connector->base); > > - > > - if (!intel_connector->mst_port) > > - sysfs_remove_link(&intel_connector->base.kdev->kobj, > > - intel_dp->aux.ddc.dev.kobj.name); > > + struct intel_dp *intel_dp = > > + enc_to_intel_dp(&intel_connector->encoder->base); > > + drm_dp_aux_unregister(&intel_dp->aux); > > intel_connector_unregister(intel_connector); > > } > > > > -- > > 2.4.3 > > > > _______________________________________________ > > dri-devel mailing list > > dri-devel@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/dri-devel
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index f2bfca0..515893c 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -1162,14 +1162,12 @@ static void intel_aux_reg_init(struct intel_dp *intel_dp) static void intel_dp_aux_fini(struct intel_dp *intel_dp) { - drm_dp_aux_unregister(&intel_dp->aux); kfree(intel_dp->aux.name); } static int intel_dp_aux_init(struct intel_dp *intel_dp, struct intel_connector *connector) { - struct drm_device *dev = intel_dp_to_dev(intel_dp); struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); enum port port = intel_dig_port->port; int ret; @@ -1180,7 +1178,7 @@ intel_dp_aux_init(struct intel_dp *intel_dp, struct intel_connector *connector) if (!intel_dp->aux.name) return -ENOMEM; - intel_dp->aux.dev = dev->dev; + intel_dp->aux.dev = connector->base.kdev; intel_dp->aux.transfer = intel_dp_aux_transfer; DRM_DEBUG_KMS("registering %s bus for %s\n", @@ -1195,27 +1193,15 @@ intel_dp_aux_init(struct intel_dp *intel_dp, struct intel_connector *connector) return ret; } - ret = sysfs_create_link(&connector->base.kdev->kobj, - &intel_dp->aux.ddc.dev.kobj, - intel_dp->aux.ddc.dev.kobj.name); - if (ret < 0) { - DRM_ERROR("sysfs_create_link() for %s failed (%d)\n", - intel_dp->aux.name, ret); - intel_dp_aux_fini(intel_dp); - return ret; - } - return 0; } static void intel_dp_connector_unregister(struct intel_connector *intel_connector) { - struct intel_dp *intel_dp = intel_attached_dp(&intel_connector->base); - - if (!intel_connector->mst_port) - sysfs_remove_link(&intel_connector->base.kdev->kobj, - intel_dp->aux.ddc.dev.kobj.name); + struct intel_dp *intel_dp = + enc_to_intel_dp(&intel_connector->encoder->base); + drm_dp_aux_unregister(&intel_dp->aux); intel_connector_unregister(intel_connector); }
So far, the i915 driver and some other drivers set it to the drm_device, which doesn't allow one to know which DP a given aux channel is related to. Changing this to be the drm_connector provides proper nesting, still allowing one to get the drm_device from it. Some drivers already set it to the drm_connector. This also removes the need to add a sysfs link for the i2c device under the connector, as it will already be there. v2: - As a side effect, drm_dp_aux_unregister() must be called before intel_connector_unregister(), as both the aux.dev and the i2c adapter dev are children of the drm_connector device now. Calling drm_dp_aux_unregister() before prevents them from being destroyed twice. Signed-off-by: Rafael Antognolli <rafael.antognolli@intel.com> --- drivers/gpu/drm/i915/intel_dp.c | 22 ++++------------------ 1 file changed, 4 insertions(+), 18 deletions(-)