| Message ID | 1555085131-8716-2-git-send-email-sunpeng.li@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add AUX device entries for DP MST devices | expand |
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; >
On 2019-04-16 6:16 p.m., Lyude Paul wrote: > Sorry for the slow response, I've been really busy ;_; No worries :) > > 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. That does sound neater - although FWICT, this will have to be done with udev rules? I took a brief look at how that's done for storage devices. It looks like they have rules defined in /lib/udev/rules.d/60-persistent-storage.rules that manages the "by-x" symlinks. To make this happen for aux devices, what we could do is attach sysfs attributes to the device. It can then be parsed by udev in a similar fashion. Currently, only 'name' attribute is attached, as seen in drm_dp_aux_dev.c (after name_show()). Thanks, Leo > >> >> 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; >>
On Wed, 2019-04-17 at 23:10 +0000, Li, Sun peng (Leo) wrote: > > > On 2019-04-16 6:16 p.m., Lyude Paul wrote: > > Sorry for the slow response, I've been really busy ;_; > > No worries :) > > > 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. > > That does sound neater - although FWICT, this will have to be done with > udev rules? > > I took a brief look at how that's done for storage devices. It looks > like they have rules defined in > /lib/udev/rules.d/60-persistent-storage.rules that manages the "by-x" > symlinks. > > To make this happen for aux devices, what we could do is attach sysfs > attributes to the device. It can then be parsed by udev in a similar > fashion. Currently, only 'name' attribute is attached, as seen in > drm_dp_aux_dev.c (after name_show()). Yeah-that sounds perfect to me! > > Thanks, > Leo > > > > 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; > > >
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;