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

[Lukas Wunner]

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 at 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 at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel