Re: [PATCH 1/2] drm/dp_aux: Use non-cyclic idr, add suffix option for aux device

[Lyude Paul <lyude-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>]

Sorry for the slow response, I've been really busy ;_;

On Fri, 2019-04-12 at 12:05 -0400, sunpeng.li@amd.com wrote:
> From: Leo Li <sunpeng.li@amd.com>
> 
> In preparation for adding aux devices for DP MST:
> 
> 1. A non-cyclic idr is used for the device minor version. That way,
>    hotplug cycling MST devices won't needlessly increment the minor
>    version index.

I really like this btw, cyclic idrs are really annoying for drm_dp_aux_dev,
even outside of MST (try reloading a drm driver a couple of times and you'll
understand ;). I think we should split this into another commit though

> 2. A suffix option is added to the aux device file name. It can be used
>    to identify the corresponding MST device.

I like this idea, but I think there may be a better way that we can do this.
Using /dev/nvme* as an example, we have the standard dev paths (/dev/nvme0,
/dev/nvme0n1, etc.). But we can also access them through /dev/disk/by-
{id,label,partlabel,partuuid,path,uuid}.

So what if we added something similar for aux devices? We start off with the
standard /dev/drm_dp_aux*, then provide more descriptive symlinks and
directories:

(feel free to come up with better naming then this if you can)

/dev/drm_dp_aux/card0-DP-1 -> /dev/drm_dp_aux1
/dev/drm_dp_aux/card0-DP-2 -> /dev/drm_dp_aux2
etc.

> 
> Signed-off-by: Leo Li <sunpeng.li@amd.com>
> ---
>  drivers/gpu/drm/drm_crtc_helper_internal.h | 5 +++--
>  drivers/gpu/drm/drm_dp_aux_dev.c           | 8 ++++----
>  drivers/gpu/drm/drm_dp_helper.c            | 2 +-
>  3 files changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc_helper_internal.h
> b/drivers/gpu/drm/drm_crtc_helper_internal.h
> index b5ac158..9f0907b 100644
> --- a/drivers/gpu/drm/drm_crtc_helper_internal.h
> +++ b/drivers/gpu/drm/drm_crtc_helper_internal.h
> @@ -46,7 +46,7 @@ static inline int drm_fb_helper_modinit(void)
>  #ifdef CONFIG_DRM_DP_AUX_CHARDEV
>  int drm_dp_aux_dev_init(void);
>  void drm_dp_aux_dev_exit(void);
> -int drm_dp_aux_register_devnode(struct drm_dp_aux *aux);
> +int drm_dp_aux_register_devnode(struct drm_dp_aux *aux, const char
> *suffix);
>  void drm_dp_aux_unregister_devnode(struct drm_dp_aux *aux);
>  #else
>  static inline int drm_dp_aux_dev_init(void)
> @@ -58,7 +58,8 @@ static inline void drm_dp_aux_dev_exit(void)
>  {
>  }
>  
> -static inline int drm_dp_aux_register_devnode(struct drm_dp_aux *aux)
> +static inline int drm_dp_aux_register_devnode(struct drm_dp_aux *aux,
> +					      const char *suffix)
>  {
>  	return 0;
>  }
> diff --git a/drivers/gpu/drm/drm_dp_aux_dev.c
> b/drivers/gpu/drm/drm_dp_aux_dev.c
> index 0e4f25d..2310a67 100644
> --- a/drivers/gpu/drm/drm_dp_aux_dev.c
> +++ b/drivers/gpu/drm/drm_dp_aux_dev.c
> @@ -80,8 +80,7 @@ static struct drm_dp_aux_dev *alloc_drm_dp_aux_dev(struct
> drm_dp_aux *aux)
>  	kref_init(&aux_dev->refcount);
>  
>  	mutex_lock(&aux_idr_mutex);
> -	index = idr_alloc_cyclic(&aux_idr, aux_dev, 0, DRM_AUX_MINORS,
> -				 GFP_KERNEL);
> +	index = idr_alloc(&aux_idr, aux_dev, 0, DRM_AUX_MINORS, GFP_KERNEL);
>  	mutex_unlock(&aux_idr_mutex);
>  	if (index < 0) {
>  		kfree(aux_dev);
> @@ -290,7 +289,7 @@ void drm_dp_aux_unregister_devnode(struct drm_dp_aux
> *aux)
>  	kref_put(&aux_dev->refcount, release_drm_dp_aux_dev);
>  }
>  
> -int drm_dp_aux_register_devnode(struct drm_dp_aux *aux)
> +int drm_dp_aux_register_devnode(struct drm_dp_aux *aux, const char *suffix)
>  {
>  	struct drm_dp_aux_dev *aux_dev;
>  	int res;
> @@ -301,7 +300,8 @@ int drm_dp_aux_register_devnode(struct drm_dp_aux *aux)
>  
>  	aux_dev->dev = device_create(drm_dp_aux_dev_class, aux->dev,
>  				     MKDEV(drm_dev_major, aux_dev->index),
> NULL,
> -				     "drm_dp_aux%d", aux_dev->index);
> +				     "drm_dp_aux%d%s", aux_dev->index,
> +				     suffix ? suffix : "");
>  	if (IS_ERR(aux_dev->dev)) {
>  		res = PTR_ERR(aux_dev->dev);
>  		aux_dev->dev = NULL;
> diff --git a/drivers/gpu/drm/drm_dp_helper.c
> b/drivers/gpu/drm/drm_dp_helper.c
> index e2266ac..13f1005 100644
> --- a/drivers/gpu/drm/drm_dp_helper.c
> +++ b/drivers/gpu/drm/drm_dp_helper.c
> @@ -1143,7 +1143,7 @@ int drm_dp_aux_register(struct drm_dp_aux *aux)
>  	strlcpy(aux->ddc.name, aux->name ? aux->name : dev_name(aux->dev),
>  		sizeof(aux->ddc.name));
>  
> -	ret = drm_dp_aux_register_devnode(aux);
> +	ret = drm_dp_aux_register_devnode(aux, NULL);
>  	if (ret)
>  		return ret;
>  
-- 
Cheers,
	Lyude Paul

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx