diff mbox series

[v2] drm/sysfs: Add mstpath attribute to connector devices

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

Commit Message

Leo Li July 5, 2019, 2:32 p.m. UTC
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(+)

Comments

Ville Syrjala July 10, 2019, 10:07 a.m. UTC | #1
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
Lyude Paul July 10, 2019, 10:50 p.m. UTC | #2
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
>  };
>
Leo Li July 16, 2019, 6:28 p.m. UTC | #3
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
>>  };
>>  
>
Lyude Paul July 16, 2019, 8:28 p.m. UTC | #4
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 mbox series

Patch

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
 };