[PATCHv2 0/2] Add /sys media_dev attr for V4L/DVB devices

[PATCHv2 0/2] Add /sys media_dev attr for V4L/DVB devices

[Hans Verkuil]

A long standing issue is how to find the associated media device for
a V4L or DVB device node. This is primarily useful for applications
like v4l2-ctl and v4l2-compliance.

Until recently these applications relied on /sys and the fact that
the media device was accessible in /sys via:

/sys/class/video4linux/videoX/device/mediaY/

But commit ee494cf377e1 ("media: v4l2-device: Link subdevices to their
parent devices if available") broke that scheme for subdevices. That
scheme was rather a hack anyway.

Attempts to report the major/minor number of the media device via the
public API (i.e. by extending VIDIOC_QUERYCAP or VIDIOC_SUBDEV_QUERYCAP)
failed, so this patch series now just adds a media_dev attribute when a
V4L or DVB device node is created. This attribute contains the major:minor
of the media device. It is only created if the device node is actually
associated with a media controller.

This issue was reported by Sebastian Frick:

https://lore.kernel.org/linux-media/20210120183054.5ltnjdtrmumplevt@basti-TUXEDO-Book-XA1510/T/#t

Regards,

	Hans

Changes since v1: use the is_visible callback to ensure the attribute
is only shown if mdev is non-NULL.

Hans Verkuil (2):
  v4l2-dev: add /sys media_dev attr for video devices
  dvbdev: add /sys media_dev attr for dvb devices

 drivers/media/dvb-core/dvbdev.c    | 46 ++++++++++++++++++++++++++++
 drivers/media/v4l2-core/v4l2-dev.c | 48 +++++++++++++++++++++++++++++-
 2 files changed, 93 insertions(+), 1 deletion(-)

-- 
2.29.2

[PATCHv2 1/2] v4l2-dev: add /sys media_dev attr for video devices

[Hans Verkuil]

Create a media_dev attribute in /sys for each video device
which contains the media device major and minor number (or
is empty if there is no associated media device).

It is not created if the CONFIG_MEDIA_CONTROLLER is not
defined.

This makes it possible for applications like v4l2-compliance
to find the associated media controller of a video device.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 drivers/media/v4l2-core/v4l2-dev.c | 48 +++++++++++++++++++++++++++++-
 1 file changed, 47 insertions(+), 1 deletion(-)

diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
index b6a72d297775..85b94b25aba2 100644
--- a/drivers/media/v4l2-core/v4l2-dev.c
+++ b/drivers/media/v4l2-core/v4l2-dev.c
@@ -87,13 +87,59 @@ static ssize_t name_show(struct device *cd,
 }
 static DEVICE_ATTR_RO(name);
 
+#if defined(CONFIG_MEDIA_CONTROLLER)
+static ssize_t media_dev_show(struct device *cd,
+			 struct device_attribute *attr, char *buf)
+{
+	struct video_device *vdev = to_video_device(cd);
+	struct v4l2_device *v4l2_dev = vdev->v4l2_dev;
+
+	buf[0] = '\0';
+	if (!v4l2_dev->mdev)
+		return 0;
+	return sprintf(buf, "%u:%u\n",
+		       MAJOR(v4l2_dev->mdev->devnode->dev.devt),
+		       MINOR(v4l2_dev->mdev->devnode->dev.devt));
+}
+
+static DEVICE_ATTR_RO(media_dev);
+#endif
+
+static umode_t video_device_attr_is_visible(struct kobject *kobj,
+					    struct attribute *attr, int n)
+{
+	struct video_device *vdev = to_video_device(kobj_to_dev(kobj));
+
+#if defined(CONFIG_MEDIA_CONTROLLER)
+	if (attr == &dev_attr_media_dev.attr) {
+		struct v4l2_device *v4l2_dev = vdev->v4l2_dev;
+
+		if (!v4l2_dev->mdev)
+			return 0;
+	}
+#endif
+	return attr->mode;
+}
+
 static struct attribute *video_device_attrs[] = {
 	&dev_attr_name.attr,
 	&dev_attr_dev_debug.attr,
 	&dev_attr_index.attr,
+#if defined(CONFIG_MEDIA_CONTROLLER)
+	&dev_attr_media_dev.attr,
+#endif
 	NULL,
 };
-ATTRIBUTE_GROUPS(video_device);
+
+static const struct attribute_group video_device_group = {
+	.is_visible = video_device_attr_is_visible,
+	.attrs = video_device_attrs,
+};
+
+static const struct attribute_group *video_device_groups[] = {
+	&video_device_group,
+	NULL
+};
 
 /*
  *	Active devices
-- 
2.29.2

[PATCHv2 2/2] dvbdev: add /sys media_dev attr for dvb devices

[Hans Verkuil]

Create a media_dev attribute in /sys for each dvb device
which contains the media device major and minor number (or
is empty if there is no associated media device).

It is not created if the CONFIG_MEDIA_CONTROLLER_DVB is not
defined.

This makes it possible for applications like v4l2-compliance
to find the associated media controller of a dvb device.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 drivers/media/dvb-core/dvbdev.c | 46 +++++++++++++++++++++++++++++++++
 1 file changed, 46 insertions(+)

diff --git a/drivers/media/dvb-core/dvbdev.c b/drivers/media/dvb-core/dvbdev.c
index 5ff7bedee247..e63cee1fc331 100644
--- a/drivers/media/dvb-core/dvbdev.c
+++ b/drivers/media/dvb-core/dvbdev.c
@@ -452,6 +452,49 @@ static int dvb_register_media_device(struct dvb_device *dvbdev,
 	return 0;
 }
 
+#if defined(CONFIG_MEDIA_CONTROLLER_DVB)
+static ssize_t media_dev_show(struct device *cd,
+			 struct device_attribute *attr, char *buf)
+{
+	struct dvb_device *dvbdev = dev_get_drvdata(cd);
+
+	buf[0] = '\0';
+	if (!dvbdev->adapter->mdev)
+		return 0;
+	return sprintf(buf, "%u:%u\n",
+		       MAJOR(dvbdev->adapter->mdev->devnode->dev.devt),
+		       MINOR(dvbdev->adapter->mdev->devnode->dev.devt));
+}
+static DEVICE_ATTR_RO(media_dev);
+
+static umode_t dvb_device_attr_is_visible(struct kobject *kobj,
+					  struct attribute *attr, int n)
+{
+	struct dvb_device *dvbdev = dev_get_drvdata(kobj_to_dev(kobj));
+
+	if (attr == &dev_attr_media_dev.attr) {
+		if (!dvbdev->adapter->mdev)
+			return 0;
+	}
+	return attr->mode;
+}
+
+static struct attribute *dvb_device_attrs[] = {
+	&dev_attr_media_dev.attr,
+	NULL,
+};
+
+static const struct attribute_group dvb_device_group = {
+	.is_visible = dvb_device_attr_is_visible,
+	.attrs = dvb_device_attrs,
+};
+
+static const struct attribute_group *dvb_device_groups[] = {
+	&dvb_device_group,
+	NULL
+};
+#endif
+
 int dvb_register_device(struct dvb_adapter *adap, struct dvb_device **pdvbdev,
 			const struct dvb_device *template, void *priv,
 			enum dvb_device_type type, int demux_sink_pads)
@@ -1056,6 +1099,9 @@ static int __init init_dvbdev(void)
 	}
 	dvb_class->dev_uevent = dvb_uevent;
 	dvb_class->devnode = dvb_devnode;
+#if defined(CONFIG_MEDIA_CONTROLLER_DVB)
+	dvb_class->dev_groups = dvb_device_groups;
+#endif
 	return 0;
 
 error:
-- 
2.29.2

Re: [PATCHv2 1/2] v4l2-dev: add /sys media_dev attr for video devices

[Laurent Pinchart]

Hi Hans,

Thank you for the patch.

On Mon, Feb 01, 2021 at 10:36:58AM +0100, Hans Verkuil wrote:
> Create a media_dev attribute in /sys for each video device
> which contains the media device major and minor number (or
> is empty if there is no associated media device).
> 
> It is not created if the CONFIG_MEDIA_CONTROLLER is not
> defined.
> 
> This makes it possible for applications like v4l2-compliance
> to find the associated media controller of a video device.
> 
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> ---
>  drivers/media/v4l2-core/v4l2-dev.c | 48 +++++++++++++++++++++++++++++-
>  1 file changed, 47 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
> index b6a72d297775..85b94b25aba2 100644
> --- a/drivers/media/v4l2-core/v4l2-dev.c
> +++ b/drivers/media/v4l2-core/v4l2-dev.c
> @@ -87,13 +87,59 @@ static ssize_t name_show(struct device *cd,
>  }
>  static DEVICE_ATTR_RO(name);
>  
> +#if defined(CONFIG_MEDIA_CONTROLLER)
> +static ssize_t media_dev_show(struct device *cd,
> +			 struct device_attribute *attr, char *buf)
> +{
> +	struct video_device *vdev = to_video_device(cd);
> +	struct v4l2_device *v4l2_dev = vdev->v4l2_dev;
> +
> +	buf[0] = '\0';
> +	if (!v4l2_dev->mdev)
> +		return 0;
> +	return sprintf(buf, "%u:%u\n",
> +		       MAJOR(v4l2_dev->mdev->devnode->dev.devt),
> +		       MINOR(v4l2_dev->mdev->devnode->dev.devt));

Could v4l2-dev->mdev be set to null between time of check and time of
use, or are sysfs properties guaranteed to be removed first ?

I'm still not convinced that this is the right way to go from a
userspace point of view. I believe we should shift from the paradigm of
a video node belonging to a media device to a media device that contains
video nodes. This means that userspace should use the media device as
the entry point, and find video nodes from the media graph, instead of
going the other way around. That's the only sensible way to handle
complex devices, and is really a mindset change that should be pushed to
all userspace applications. It will obviously take time and effort, but
if we don't start by eating our own dogfood, we'll never succeed.

I'm thus not opposed to this patch series so much that I would like it
to not being merged, but I believe it's a step in the wrong direction.
With time I've learnt that I can't prevent every step I consider wrong
to be taken (and I also make mistakes, so who knows :-)).

> +}
> +
> +static DEVICE_ATTR_RO(media_dev);
> +#endif
> +
> +static umode_t video_device_attr_is_visible(struct kobject *kobj,
> +					    struct attribute *attr, int n)
> +{
> +	struct video_device *vdev = to_video_device(kobj_to_dev(kobj));
> +
> +#if defined(CONFIG_MEDIA_CONTROLLER)
> +	if (attr == &dev_attr_media_dev.attr) {
> +		struct v4l2_device *v4l2_dev = vdev->v4l2_dev;
> +
> +		if (!v4l2_dev->mdev)
> +			return 0;
> +	}
> +#endif
> +	return attr->mode;
> +}
> +
>  static struct attribute *video_device_attrs[] = {
>  	&dev_attr_name.attr,
>  	&dev_attr_dev_debug.attr,
>  	&dev_attr_index.attr,
> +#if defined(CONFIG_MEDIA_CONTROLLER)
> +	&dev_attr_media_dev.attr,
> +#endif
>  	NULL,
>  };
> -ATTRIBUTE_GROUPS(video_device);
> +
> +static const struct attribute_group video_device_group = {
> +	.is_visible = video_device_attr_is_visible,
> +	.attrs = video_device_attrs,
> +};
> +
> +static const struct attribute_group *video_device_groups[] = {
> +	&video_device_group,
> +	NULL
> +};
>  
>  /*
>   *	Active devices

-- 
Regards,

Laurent Pinchart

Re: [PATCHv2 1/2] v4l2-dev: add /sys media_dev attr for video devices

[Hans Verkuil]

On 01/02/2021 21:26, Laurent Pinchart wrote:
> Hi Hans,
> 
> Thank you for the patch.
> 
> On Mon, Feb 01, 2021 at 10:36:58AM +0100, Hans Verkuil wrote:
>> Create a media_dev attribute in /sys for each video device
>> which contains the media device major and minor number (or
>> is empty if there is no associated media device).
>>
>> It is not created if the CONFIG_MEDIA_CONTROLLER is not
>> defined.
>>
>> This makes it possible for applications like v4l2-compliance
>> to find the associated media controller of a video device.
>>
>> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>> ---
>>  drivers/media/v4l2-core/v4l2-dev.c | 48 +++++++++++++++++++++++++++++-
>>  1 file changed, 47 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
>> index b6a72d297775..85b94b25aba2 100644
>> --- a/drivers/media/v4l2-core/v4l2-dev.c
>> +++ b/drivers/media/v4l2-core/v4l2-dev.c
>> @@ -87,13 +87,59 @@ static ssize_t name_show(struct device *cd,
>>  }
>>  static DEVICE_ATTR_RO(name);
>>  
>> +#if defined(CONFIG_MEDIA_CONTROLLER)
>> +static ssize_t media_dev_show(struct device *cd,
>> +			 struct device_attribute *attr, char *buf)
>> +{
>> +	struct video_device *vdev = to_video_device(cd);
>> +	struct v4l2_device *v4l2_dev = vdev->v4l2_dev;
>> +
>> +	buf[0] = '\0';
>> +	if (!v4l2_dev->mdev)
>> +		return 0;
>> +	return sprintf(buf, "%u:%u\n",
>> +		       MAJOR(v4l2_dev->mdev->devnode->dev.devt),
>> +		       MINOR(v4l2_dev->mdev->devnode->dev.devt));
> 
> Could v4l2-dev->mdev be set to null between time of check and time of
> use, or are sysfs properties guaranteed to be removed first ?

After checking device_del() I see that these attributes are removed
before the device node itself is removed. However, I am not 100% certain
that all drivers will postpone unregistering the media device node until
all other device nodes are unregistered.

I think it would be safer to copy v4l2_dev->mdev->devnode->dev.devt into
struct video_device at registration time. It's more robust.

> 
> I'm still not convinced that this is the right way to go from a
> userspace point of view. I believe we should shift from the paradigm of
> a video node belonging to a media device to a media device that contains
> video nodes. This means that userspace should use the media device as
> the entry point, and find video nodes from the media graph, instead of
> going the other way around. That's the only sensible way to handle
> complex devices, and is really a mindset change that should be pushed to
> all userspace applications. It will obviously take time and effort, but
> if we don't start by eating our own dogfood, we'll never succeed.
> 
> I'm thus not opposed to this patch series so much that I would like it
> to not being merged, but I believe it's a step in the wrong direction.
> With time I've learnt that I can't prevent every step I consider wrong
> to be taken (and I also make mistakes, so who knows :-)).

I completely agree with you, but the reality is that many V4L2 drivers do
not use the media controller, and that is not something that will change.

I honestly do not see why having a reference to the actual associated media
device would be a bad thing: it will only ensure that v4l2-ctl/compliance
can tell the user that that device is part of a media controller.

I don't see how or why applications would want to abuse this.

I'll post a v3 of this series.

Regards,

	Hans

> 
>> +}
>> +
>> +static DEVICE_ATTR_RO(media_dev);
>> +#endif
>> +
>> +static umode_t video_device_attr_is_visible(struct kobject *kobj,
>> +					    struct attribute *attr, int n)
>> +{
>> +	struct video_device *vdev = to_video_device(kobj_to_dev(kobj));
>> +
>> +#if defined(CONFIG_MEDIA_CONTROLLER)
>> +	if (attr == &dev_attr_media_dev.attr) {
>> +		struct v4l2_device *v4l2_dev = vdev->v4l2_dev;
>> +
>> +		if (!v4l2_dev->mdev)
>> +			return 0;
>> +	}
>> +#endif
>> +	return attr->mode;
>> +}
>> +
>>  static struct attribute *video_device_attrs[] = {
>>  	&dev_attr_name.attr,
>>  	&dev_attr_dev_debug.attr,
>>  	&dev_attr_index.attr,
>> +#if defined(CONFIG_MEDIA_CONTROLLER)
>> +	&dev_attr_media_dev.attr,
>> +#endif
>>  	NULL,
>>  };
>> -ATTRIBUTE_GROUPS(video_device);
>> +
>> +static const struct attribute_group video_device_group = {
>> +	.is_visible = video_device_attr_is_visible,
>> +	.attrs = video_device_attrs,
>> +};
>> +
>> +static const struct attribute_group *video_device_groups[] = {
>> +	&video_device_group,
>> +	NULL
>> +};
>>  
>>  /*
>>   *	Active devices
> 

Re: [PATCHv2 1/2] v4l2-dev: add /sys media_dev attr for video devices

[Laurent Pinchart]

Hi Hans,

On Tue, Feb 02, 2021 at 10:48:15AM +0100, Hans Verkuil wrote:
> On 01/02/2021 21:26, Laurent Pinchart wrote:
> > On Mon, Feb 01, 2021 at 10:36:58AM +0100, Hans Verkuil wrote:
> >> Create a media_dev attribute in /sys for each video device
> >> which contains the media device major and minor number (or
> >> is empty if there is no associated media device).
> >>
> >> It is not created if the CONFIG_MEDIA_CONTROLLER is not
> >> defined.
> >>
> >> This makes it possible for applications like v4l2-compliance
> >> to find the associated media controller of a video device.
> >>
> >> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> >> ---
> >>  drivers/media/v4l2-core/v4l2-dev.c | 48 +++++++++++++++++++++++++++++-
> >>  1 file changed, 47 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
> >> index b6a72d297775..85b94b25aba2 100644
> >> --- a/drivers/media/v4l2-core/v4l2-dev.c
> >> +++ b/drivers/media/v4l2-core/v4l2-dev.c
> >> @@ -87,13 +87,59 @@ static ssize_t name_show(struct device *cd,
> >>  }
> >>  static DEVICE_ATTR_RO(name);
> >>  
> >> +#if defined(CONFIG_MEDIA_CONTROLLER)
> >> +static ssize_t media_dev_show(struct device *cd,
> >> +			 struct device_attribute *attr, char *buf)
> >> +{
> >> +	struct video_device *vdev = to_video_device(cd);
> >> +	struct v4l2_device *v4l2_dev = vdev->v4l2_dev;
> >> +
> >> +	buf[0] = '\0';
> >> +	if (!v4l2_dev->mdev)
> >> +		return 0;
> >> +	return sprintf(buf, "%u:%u\n",
> >> +		       MAJOR(v4l2_dev->mdev->devnode->dev.devt),
> >> +		       MINOR(v4l2_dev->mdev->devnode->dev.devt));
> > 
> > Could v4l2-dev->mdev be set to null between time of check and time of
> > use, or are sysfs properties guaranteed to be removed first ?
> 
> After checking device_del() I see that these attributes are removed
> before the device node itself is removed. However, I am not 100% certain
> that all drivers will postpone unregistering the media device node until
> all other device nodes are unregistered.
> 
> I think it would be safer to copy v4l2_dev->mdev->devnode->dev.devt into
> struct video_device at registration time. It's more robust.
> 
> > I'm still not convinced that this is the right way to go from a
> > userspace point of view. I believe we should shift from the paradigm of
> > a video node belonging to a media device to a media device that contains
> > video nodes. This means that userspace should use the media device as
> > the entry point, and find video nodes from the media graph, instead of
> > going the other way around. That's the only sensible way to handle
> > complex devices, and is really a mindset change that should be pushed to
> > all userspace applications. It will obviously take time and effort, but
> > if we don't start by eating our own dogfood, we'll never succeed.
> > 
> > I'm thus not opposed to this patch series so much that I would like it
> > to not being merged, but I believe it's a step in the wrong direction.
> > With time I've learnt that I can't prevent every step I consider wrong
> > to be taken (and I also make mistakes, so who knows :-)).
> 
> I completely agree with you, but the reality is that many V4L2 drivers do
> not use the media controller, and that is not something that will change.

But those also do not expose subdevs to userspace, and don't require
using the media controller API in v4l2-compliance for instance. I'm not
saying we need to drop the existing way of accessing video devices
overnight, but for applications that need to link video devices and
media controller devices, I think they should go from media controller
to video device.

> I honestly do not see why having a reference to the actual associated media
> device would be a bad thing: it will only ensure that v4l2-ctl/compliance
> can tell the user that that device is part of a media controller.
>
> I don't see how or why applications would want to abuse this.

My issue with it is that it will give applications a way to do things
incorrectly. There's no reason they should use this API, but we all know
there's a large difference between what applications should do and what
they end up doing :-)

When it comes to v4l2-compliance, I think it's important to lead by
example, and show how to go from media device to video device instead of
the other way around. Of course, until all drivers implement a media
device (which will not happen overnight, but it could also be fairly
easily automated if we wanted to, with very minimal changes to most
drivers), we will still need to support operation on video nodes
directly without a media device.

> I'll post a v3 of this series.
> 
> >> +}
> >> +
> >> +static DEVICE_ATTR_RO(media_dev);
> >> +#endif
> >> +
> >> +static umode_t video_device_attr_is_visible(struct kobject *kobj,
> >> +					    struct attribute *attr, int n)
> >> +{
> >> +	struct video_device *vdev = to_video_device(kobj_to_dev(kobj));
> >> +
> >> +#if defined(CONFIG_MEDIA_CONTROLLER)
> >> +	if (attr == &dev_attr_media_dev.attr) {
> >> +		struct v4l2_device *v4l2_dev = vdev->v4l2_dev;
> >> +
> >> +		if (!v4l2_dev->mdev)
> >> +			return 0;
> >> +	}
> >> +#endif
> >> +	return attr->mode;
> >> +}
> >> +
> >>  static struct attribute *video_device_attrs[] = {
> >>  	&dev_attr_name.attr,
> >>  	&dev_attr_dev_debug.attr,
> >>  	&dev_attr_index.attr,
> >> +#if defined(CONFIG_MEDIA_CONTROLLER)
> >> +	&dev_attr_media_dev.attr,
> >> +#endif
> >>  	NULL,
> >>  };
> >> -ATTRIBUTE_GROUPS(video_device);
> >> +
> >> +static const struct attribute_group video_device_group = {
> >> +	.is_visible = video_device_attr_is_visible,
> >> +	.attrs = video_device_attrs,
> >> +};
> >> +
> >> +static const struct attribute_group *video_device_groups[] = {
> >> +	&video_device_group,
> >> +	NULL
> >> +};
> >>  
> >>  /*
> >>   *	Active devices

-- 
Regards,

Laurent Pinchart