Re: [PATCH v9 3/3] drm/dp: Set aux.dev to the drm_connector device, instead of drm_device.

[Rafael Antognolli <rafael.antognolli@intel.com>]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx