| Message ID | 20190705143220.11109-1-sunpeng.li@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] drm/sysfs: Add mstpath attribute to connector devices | expand |
On Fri, Jul 05, 2019 at 10:32:20AM -0400, sunpeng.li@amd.com wrote: > From: Leo Li <sunpeng.li@amd.com> > > This can be used to create more descriptive symlinks for MST aux > devices. Consider the following udev rule: > > SUBSYSTEM=="drm_dp_aux_dev", SUBSYSTEMS=="drm", ATTRS{mstpath}=="?*", > SYMLINK+="drm_dp_aux/by-path/$attr{mstpath}" > > The following symlinks will be created (depending on your MST topology): > > $ ls /dev/drm_dp_aux/by-path/ > card0-mst:0-1 card0-mst:0-1-1 card0-mst:0-1-8 card0-mst:0-8 > > v2: remove unnecessary locking of mode_config.mutex > > Signed-off-by: Leo Li <sunpeng.li@amd.com> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/drm_sysfs.c | 20 ++++++++++++++++++++ > 1 file changed, 20 insertions(+) > > diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c > index ad10810bc972..7d483ab684a0 100644 > --- a/drivers/gpu/drm/drm_sysfs.c > +++ b/drivers/gpu/drm/drm_sysfs.c > @@ -236,16 +236,36 @@ static ssize_t modes_show(struct device *device, > return written; > } > > +static ssize_t mstpath_show(struct device *device, > + struct device_attribute *attr, > + char *buf) > +{ > + struct drm_connector *connector = to_drm_connector(device); > + ssize_t ret = 0; > + char *path; > + > + if (!connector->path_blob_ptr) > + return ret; > + > + path = connector->path_blob_ptr->data; > + ret = snprintf(buf, PAGE_SIZE, "card%d-%s\n", > + connector->dev->primary->index, path); > + > + return ret; > +} > + > static DEVICE_ATTR_RW(status); > static DEVICE_ATTR_RO(enabled); > static DEVICE_ATTR_RO(dpms); > static DEVICE_ATTR_RO(modes); > +static DEVICE_ATTR_RO(mstpath); > > static struct attribute *connector_dev_attrs[] = { > &dev_attr_status.attr, > &dev_attr_enabled.attr, > &dev_attr_dpms.attr, > &dev_attr_modes.attr, > + &dev_attr_mstpath.attr, > NULL > }; > > -- > 2.22.0
gah. So, I was originally going to ask "why didn't we add the connector name into this?", but then I realized we're doing the right thing already and just doing card%d-%s % (card_number, path_prop). Which means we probably really don't want to add a property called mstpath, since it's hardly different from path (whoops!). Additionally, after some thinking I realized I may have made a mistake as I'm not entirely sure if we would need to specify the DRM card in the path prop for udev, considering that's specified in the sysfs path all ready. Even if I'm wrong on that though, I think it might be better not to add an mstpath property and just go the route of just adding a new path_v2 property that we could use for both MST and non-MST connector paths. (I cc'd you on the email thread about this, so you can read more about this there. So, I would actually suggest we just drop this patch entirely for now. We should be fine without it, even though the dp_aux_dev paths will be kind of ugly for a little while. I'd rather the rest of this series get upstream first, and try to do the path prop stuff separately. On Fri, 2019-07-05 at 10:32 -0400, sunpeng.li@amd.com wrote: > From: Leo Li <sunpeng.li@amd.com> > > This can be used to create more descriptive symlinks for MST aux > devices. Consider the following udev rule: > > SUBSYSTEM=="drm_dp_aux_dev", SUBSYSTEMS=="drm", ATTRS{mstpath}=="?*", > SYMLINK+="drm_dp_aux/by-path/$attr{mstpath}" > > The following symlinks will be created (depending on your MST topology): > > $ ls /dev/drm_dp_aux/by-path/ > card0-mst:0-1 card0-mst:0-1-1 card0-mst:0-1-8 card0-mst:0-8 > > v2: remove unnecessary locking of mode_config.mutex > > Signed-off-by: Leo Li <sunpeng.li@amd.com> > --- > drivers/gpu/drm/drm_sysfs.c | 20 ++++++++++++++++++++ > 1 file changed, 20 insertions(+) > > diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c > index ad10810bc972..7d483ab684a0 100644 > --- a/drivers/gpu/drm/drm_sysfs.c > +++ b/drivers/gpu/drm/drm_sysfs.c > @@ -236,16 +236,36 @@ static ssize_t modes_show(struct device *device, > return written; > } > > +static ssize_t mstpath_show(struct device *device, > + struct device_attribute *attr, > + char *buf) > +{ > + struct drm_connector *connector = to_drm_connector(device); > + ssize_t ret = 0; > + char *path; > + > + if (!connector->path_blob_ptr) > + return ret; > + > + path = connector->path_blob_ptr->data; > + ret = snprintf(buf, PAGE_SIZE, "card%d-%s\n", > + connector->dev->primary->index, path); > + > + return ret; > +} > + > static DEVICE_ATTR_RW(status); > static DEVICE_ATTR_RO(enabled); > static DEVICE_ATTR_RO(dpms); > static DEVICE_ATTR_RO(modes); > +static DEVICE_ATTR_RO(mstpath); > > static struct attribute *connector_dev_attrs[] = { > &dev_attr_status.attr, > &dev_attr_enabled.attr, > &dev_attr_dpms.attr, > &dev_attr_modes.attr, > + &dev_attr_mstpath.attr, > NULL > }; >
On 2019-07-10 6:50 p.m., Lyude Paul wrote: > gah. So, I was originally going to ask "why didn't we add the connector name > into this?", but then I realized we're doing the right thing already and just > doing card%d-%s % (card_number, path_prop). Which means we probably really don't > want to add a property called mstpath, since it's hardly different from path > (whoops!). > > Additionally, after some thinking I realized I may have made a mistake as I'm > not entirely sure if we would need to specify the DRM card in the path prop for > udev, considering that's specified in the sysfs path all ready. Even if I'm > wrong on that though, I think it might be better not to add an mstpath property > and just go the route of just adding a new path_v2 property that we could use > for both MST and non-MST connector paths. (I cc'd you on the email thread about > this, so you can read more about this there. Funny enough, I was originally trying to make this work for SST devices. It didn't make sense to have by-name and by-path, but only have SST exist in the by-name symlinks. The question there was "what to use for sst paths?" Eventually I settled with keeping this purely for user friendliness. But since discussion is already underway for a better 'path', it makes sense to delay this. > > So, I would actually suggest we just drop this patch entirely for now. We should > be fine without it, even though the dp_aux_dev paths will be kind of ugly for a > little while. I'd rather the rest of this series get upstream first, and try to > do the path prop stuff separately.> Sounds fair, going to spin up v3. Thanks! Leo > > On Fri, 2019-07-05 at 10:32 -0400, sunpeng.li@amd.com wrote: >> From: Leo Li <sunpeng.li@amd.com> >> >> This can be used to create more descriptive symlinks for MST aux >> devices. Consider the following udev rule: >> >> SUBSYSTEM=="drm_dp_aux_dev", SUBSYSTEMS=="drm", ATTRS{mstpath}=="?*", >> SYMLINK+="drm_dp_aux/by-path/$attr{mstpath}" >> >> The following symlinks will be created (depending on your MST topology): >> >> $ ls /dev/drm_dp_aux/by-path/ >> card0-mst:0-1 card0-mst:0-1-1 card0-mst:0-1-8 card0-mst:0-8 >> >> v2: remove unnecessary locking of mode_config.mutex >> >> Signed-off-by: Leo Li <sunpeng.li@amd.com> >> --- >> drivers/gpu/drm/drm_sysfs.c | 20 ++++++++++++++++++++ >> 1 file changed, 20 insertions(+) >> >> diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c >> index ad10810bc972..7d483ab684a0 100644 >> --- a/drivers/gpu/drm/drm_sysfs.c >> +++ b/drivers/gpu/drm/drm_sysfs.c >> @@ -236,16 +236,36 @@ static ssize_t modes_show(struct device *device, >> return written; >> } >> >> +static ssize_t mstpath_show(struct device *device, >> + struct device_attribute *attr, >> + char *buf) >> +{ >> + struct drm_connector *connector = to_drm_connector(device); >> + ssize_t ret = 0; >> + char *path; >> + >> + if (!connector->path_blob_ptr) >> + return ret; >> + >> + path = connector->path_blob_ptr->data; >> + ret = snprintf(buf, PAGE_SIZE, "card%d-%s\n", >> + connector->dev->primary->index, path); >> + >> + return ret; >> +} >> + >> static DEVICE_ATTR_RW(status); >> static DEVICE_ATTR_RO(enabled); >> static DEVICE_ATTR_RO(dpms); >> static DEVICE_ATTR_RO(modes); >> +static DEVICE_ATTR_RO(mstpath); >> >> static struct attribute *connector_dev_attrs[] = { >> &dev_attr_status.attr, >> &dev_attr_enabled.attr, >> &dev_attr_dpms.attr, >> &dev_attr_modes.attr, >> + &dev_attr_mstpath.attr, >> NULL >> }; >> >
On Tue, 2019-07-16 at 18:28 +0000, Li, Sun peng (Leo) wrote: > > > On 2019-07-10 6:50 p.m., Lyude Paul wrote: > > gah. So, I was originally going to ask "why didn't we add the connector > > name > > into this?", but then I realized we're doing the right thing already and > > just > > doing card%d-%s % (card_number, path_prop). Which means we probably really > > don't > > want to add a property called mstpath, since it's hardly different from > > path > > (whoops!). > > > > Additionally, after some thinking I realized I may have made a mistake as > > I'm > > not entirely sure if we would need to specify the DRM card in the path > > prop for > > udev, considering that's specified in the sysfs path all ready. Even if > > I'm > > wrong on that though, I think it might be better not to add an mstpath > > property > > and just go the route of just adding a new path_v2 property that we could > > use > > for both MST and non-MST connector paths. (I cc'd you on the email thread > > about > > this, so you can read more about this there. > > Funny enough, I was originally trying to make this work for SST devices. > It didn't make sense to have by-name and by-path, but only have SST > exist in the by-name symlinks. The question there was "what to use for > sst paths?" Eventually I settled with keeping this purely for user > friendliness. But since discussion is already underway for a better > 'path', it makes sense to delay this. Yeah-I think Ville's suggestion of making object IDs global instead of per- device might be what we want to go with, but getting the aux dev stuff in the kernel first is priority imo > > > So, I would actually suggest we just drop this patch entirely for now. We > > should > > be fine without it, even though the dp_aux_dev paths will be kind of ugly > > for a > > little while. I'd rather the rest of this series get upstream first, and > > try to > > do the path prop stuff separately.> > > Sounds fair, going to spin up v3. > > Thanks! > Leo > > > On Fri, 2019-07-05 at 10:32 -0400, sunpeng.li@amd.com wrote: > > > From: Leo Li <sunpeng.li@amd.com> > > > > > > This can be used to create more descriptive symlinks for MST aux > > > devices. Consider the following udev rule: > > > > > > SUBSYSTEM=="drm_dp_aux_dev", SUBSYSTEMS=="drm", ATTRS{mstpath}=="?*", > > > SYMLINK+="drm_dp_aux/by-path/$attr{mstpath}" > > > > > > The following symlinks will be created (depending on your MST topology): > > > > > > $ ls /dev/drm_dp_aux/by-path/ > > > card0-mst:0-1 card0-mst:0-1-1 card0-mst:0-1-8 card0-mst:0-8 > > > > > > v2: remove unnecessary locking of mode_config.mutex > > > > > > Signed-off-by: Leo Li <sunpeng.li@amd.com> > > > --- > > > drivers/gpu/drm/drm_sysfs.c | 20 ++++++++++++++++++++ > > > 1 file changed, 20 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c > > > index ad10810bc972..7d483ab684a0 100644 > > > --- a/drivers/gpu/drm/drm_sysfs.c > > > +++ b/drivers/gpu/drm/drm_sysfs.c > > > @@ -236,16 +236,36 @@ static ssize_t modes_show(struct device *device, > > > return written; > > > } > > > > > > +static ssize_t mstpath_show(struct device *device, > > > + struct device_attribute *attr, > > > + char *buf) > > > +{ > > > + struct drm_connector *connector = to_drm_connector(device); > > > + ssize_t ret = 0; > > > + char *path; > > > + > > > + if (!connector->path_blob_ptr) > > > + return ret; > > > + > > > + path = connector->path_blob_ptr->data; > > > + ret = snprintf(buf, PAGE_SIZE, "card%d-%s\n", > > > + connector->dev->primary->index, path); > > > + > > > + return ret; > > > +} > > > + > > > static DEVICE_ATTR_RW(status); > > > static DEVICE_ATTR_RO(enabled); > > > static DEVICE_ATTR_RO(dpms); > > > static DEVICE_ATTR_RO(modes); > > > +static DEVICE_ATTR_RO(mstpath); > > > > > > static struct attribute *connector_dev_attrs[] = { > > > &dev_attr_status.attr, > > > &dev_attr_enabled.attr, > > > &dev_attr_dpms.attr, > > > &dev_attr_modes.attr, > > > + &dev_attr_mstpath.attr, > > > NULL > > > }; > > >
diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c index ad10810bc972..7d483ab684a0 100644 --- a/drivers/gpu/drm/drm_sysfs.c +++ b/drivers/gpu/drm/drm_sysfs.c @@ -236,16 +236,36 @@ static ssize_t modes_show(struct device *device, return written; } +static ssize_t mstpath_show(struct device *device, + struct device_attribute *attr, + char *buf) +{ + struct drm_connector *connector = to_drm_connector(device); + ssize_t ret = 0; + char *path; + + if (!connector->path_blob_ptr) + return ret; + + path = connector->path_blob_ptr->data; + ret = snprintf(buf, PAGE_SIZE, "card%d-%s\n", + connector->dev->primary->index, path); + + return ret; +} + static DEVICE_ATTR_RW(status); static DEVICE_ATTR_RO(enabled); static DEVICE_ATTR_RO(dpms); static DEVICE_ATTR_RO(modes); +static DEVICE_ATTR_RO(mstpath); static struct attribute *connector_dev_attrs[] = { &dev_attr_status.attr, &dev_attr_enabled.attr, &dev_attr_dpms.attr, &dev_attr_modes.attr, + &dev_attr_mstpath.attr, NULL };