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@xxxxxxx wrote:
> > > From: Leo Li <sunpeng.li@xxxxxxx>
> > >
> > > 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@xxxxxxx>
> > > ---
> > > 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;
> > >
_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
