[03/10] drm/sysfs: Add mstpath attribute to connector devices
diff mbox series

Message ID 20190704190519.29525-4-sunpeng.li@amd.com
State New
Headers show
Series
  • Enable MST Aux devices (v2)
Related show

Commit Message

Leo July 4, 2019, 7:05 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

Signed-off-by: Leo Li <sunpeng.li@amd.com>
---
 drivers/gpu/drm/drm_sysfs.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

Comments

Ville Syrjälä July 4, 2019, 7:33 p.m. UTC | #1
On Thu, Jul 04, 2019 at 03:05:12PM -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
> 
> Signed-off-by: Leo Li <sunpeng.li@amd.com>
> ---
>  drivers/gpu/drm/drm_sysfs.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
> index ad10810bc972..53fd1f71e900 100644
> --- a/drivers/gpu/drm/drm_sysfs.c
> +++ b/drivers/gpu/drm/drm_sysfs.c
> @@ -236,16 +236,39 @@ 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;
> +
> +	mutex_lock(&connector->dev->mode_config.mutex);

The blob is populated when the connector is created so I don't think
this lock is actually doing anything for us.

One would also hope that device_unregister() protects us from racing
with the removal of the attribute. Eg. if you hold a file descriptor
open to the sysfs file does device_unregister() block until the fd is
closed?

> +	if (!connector->path_blob_ptr)
> +		goto unlock;
> +
> +	path = connector->path_blob_ptr->data;
> +	ret = snprintf(buf, PAGE_SIZE, "card%d-%s\n",
> +		       connector->dev->primary->index, path);
> +
> +unlock:
> +	mutex_unlock(&connector->dev->mode_config.mutex);
> +	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
Leo July 5, 2019, 2:03 p.m. UTC | #2
On 2019-07-04 3:33 p.m., Ville Syrjälä wrote:
> On Thu, Jul 04, 2019 at 03:05:12PM -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
>>
>> Signed-off-by: Leo Li <sunpeng.li@amd.com>
>> ---
>>  drivers/gpu/drm/drm_sysfs.c | 23 +++++++++++++++++++++++
>>  1 file changed, 23 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
>> index ad10810bc972..53fd1f71e900 100644
>> --- a/drivers/gpu/drm/drm_sysfs.c
>> +++ b/drivers/gpu/drm/drm_sysfs.c
>> @@ -236,16 +236,39 @@ 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;
>> +
>> +	mutex_lock(&connector->dev->mode_config.mutex);
> 
> The blob is populated when the connector is created so I don't think
> this lock is actually doing anything for us.

Right, will drop this.

> 
> One would also hope that device_unregister() protects us from racing
> with the removal of the attribute. Eg. if you hold a file descriptor
> open to the sysfs file does device_unregister() block until the fd is
> closed?

For dpcd transactions, as long as the aux device is unregistered through
drm_dp_aux_unregister_devnode(), we should be good. There's an atomic_t
use_count that syncs against reads and writes.

Although I'm not sure what would happen if the device was ripped out
from underneath us, say, if the parent connector device is removed
before calling aux_unregister_devnode(). If this does happen, I think
it's more of an order-of-operations issue from the driver. V2 of patch 2
(and 7-10) actually addresses a specific occurence of this during driver
unload.

Regarding sysfs attrs, the kernfs underneath does seem to guarantee that
any r/w is completed before removal. See kernfs_drain(), called as part
of kernfs_remove().

Leo

> 
>> +	if (!connector->path_blob_ptr)
>> +		goto unlock;
>> +
>> +	path = connector->path_blob_ptr->data;
>> +	ret = snprintf(buf, PAGE_SIZE, "card%d-%s\n",
>> +		       connector->dev->primary->index, path);
>> +
>> +unlock:
>> +	mutex_unlock(&connector->dev->mode_config.mutex);
>> +	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
>

Patch
diff mbox series

diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
index ad10810bc972..53fd1f71e900 100644
--- a/drivers/gpu/drm/drm_sysfs.c
+++ b/drivers/gpu/drm/drm_sysfs.c
@@ -236,16 +236,39 @@  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;
+
+	mutex_lock(&connector->dev->mode_config.mutex);
+	if (!connector->path_blob_ptr)
+		goto unlock;
+
+	path = connector->path_blob_ptr->data;
+	ret = snprintf(buf, PAGE_SIZE, "card%d-%s\n",
+		       connector->dev->primary->index, path);
+
+unlock:
+	mutex_unlock(&connector->dev->mode_config.mutex);
+	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
 };