[PATCHv3 0/4] Add /sys media_dev attr for V4L/DVB devices

[PATCHv3 0/4] 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.

The original idea for this v3 was to store the media device minor/major
at the time of the device node registration, however the media device
node is typically created last, so this information isn't known yet.

Instead a new media_device_devt() function was added that returns the
dev_t of the media device node safely.

In addition a patch was added for vim2m to ensure that the v4l2_dev.mdev
is set before video_register_device() to ensure that the media_dev
attribute is created.

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 v2: add new media_device_devt function.
        Add vim2m patch: v4l2_dev.mdev was set too late.

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

Hans Verkuil (4):
  vim2m: intialize the media device earlier
  media-device: add media_device_devt function
  v4l2-dev: add /sys media_dev attr for V4L2 devices
  dvbdev: add /sys media_dev attr for DVB devices

 drivers/media/dvb-core/dvbdev.c    | 45 +++++++++++++++++++++++++++
 drivers/media/mc/mc-device.c       | 16 ++++++++++
 drivers/media/test-drivers/vim2m.c | 14 +++++----
 drivers/media/v4l2-core/v4l2-dev.c | 49 +++++++++++++++++++++++++++++-
 include/media/media-device.h       | 10 ++++++
 5 files changed, 127 insertions(+), 7 deletions(-)

-- 
2.29.2

[PATCHv3 1/4] vim2m: intialize the media device earlier

[Hans Verkuil]

Before the video device node is registered, the v4l2_dev.mdev
pointer must be set in order to correctly associate the video
device with the media device. Move the initialization of the
media device up.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 drivers/media/test-drivers/vim2m.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/media/test-drivers/vim2m.c b/drivers/media/test-drivers/vim2m.c
index 331a9053a0ed..a24624353f9e 100644
--- a/drivers/media/test-drivers/vim2m.c
+++ b/drivers/media/test-drivers/vim2m.c
@@ -1339,12 +1339,6 @@ static int vim2m_probe(struct platform_device *pdev)
 		goto error_dev;
 	}
 
-	ret = video_register_device(vfd, VFL_TYPE_VIDEO, 0);
-	if (ret) {
-		v4l2_err(&dev->v4l2_dev, "Failed to register video device\n");
-		goto error_m2m;
-	}
-
 #ifdef CONFIG_MEDIA_CONTROLLER
 	dev->mdev.dev = &pdev->dev;
 	strscpy(dev->mdev.model, "vim2m", sizeof(dev->mdev.model));
@@ -1353,7 +1347,15 @@ static int vim2m_probe(struct platform_device *pdev)
 	media_device_init(&dev->mdev);
 	dev->mdev.ops = &m2m_media_ops;
 	dev->v4l2_dev.mdev = &dev->mdev;
+#endif
 
+	ret = video_register_device(vfd, VFL_TYPE_VIDEO, 0);
+	if (ret) {
+		v4l2_err(&dev->v4l2_dev, "Failed to register video device\n");
+		goto error_m2m;
+	}
+
+#ifdef CONFIG_MEDIA_CONTROLLER
 	ret = v4l2_m2m_register_media_controller(dev->m2m_dev, vfd,
 						 MEDIA_ENT_F_PROC_VIDEO_SCALER);
 	if (ret) {
-- 
2.29.2

[PATCHv3 2/4] media-device: add media_device_devt function

[Hans Verkuil]

Add a helper function for use by the DVB and V4L2 core frameworks
that safely returns the major and minor numbers of the media
device node, or 0 if the device node is not registered.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 drivers/media/mc/mc-device.c | 16 ++++++++++++++++
 include/media/media-device.h | 10 ++++++++++
 2 files changed, 26 insertions(+)

diff --git a/drivers/media/mc/mc-device.c b/drivers/media/mc/mc-device.c
index 9e56d2ad6b94..2207f1b01456 100644
--- a/drivers/media/mc/mc-device.c
+++ b/drivers/media/mc/mc-device.c
@@ -850,6 +850,22 @@ void media_device_unregister(struct media_device *mdev)
 }
 EXPORT_SYMBOL_GPL(media_device_unregister);
 
+dev_t media_device_devt(struct media_device *mdev)
+{
+	dev_t devt = 0;
+
+	if (!mdev)
+		return 0;
+
+	mutex_lock(&mdev->graph_mutex);
+	/* Check if mdev is registered */
+	if (media_devnode_is_registered(mdev->devnode))
+		devt = mdev->devnode->dev.devt;
+	mutex_unlock(&mdev->graph_mutex);
+	return devt;
+}
+EXPORT_SYMBOL_GPL(media_device_devt);
+
 #if IS_ENABLED(CONFIG_PCI)
 void media_device_pci_init(struct media_device *mdev,
 			   struct pci_dev *pci_dev,
diff --git a/include/media/media-device.h b/include/media/media-device.h
index 1345e6da688a..af089b1c55ef 100644
--- a/include/media/media-device.h
+++ b/include/media/media-device.h
@@ -300,6 +300,16 @@ int __must_check __media_device_register(struct media_device *mdev,
  */
 void media_device_unregister(struct media_device *mdev);
 
+/**
+ * media_device_devt() - return media device major/minor numbers
+ *
+ * @mdev:	pointer to struct &media_device
+ *
+ * Returns 0 if the media device node is not registered, otherwise
+ * it will return the major/minor number of the media device node.
+ */
+dev_t media_device_devt(struct media_device *mdev);
+
 /**
  * media_device_register_entity() - registers a media entity inside a
  *	previously registered media device.
-- 
2.29.2

[PATCHv3 3/4] v4l2-dev: add /sys media_dev attr for V4L2 devices

[Hans Verkuil]

For each V4L2 device node create a media_dev attribute in /sys
which contains the media device major and minor number.

It is not created if the CONFIG_MEDIA_CONTROLLER is not
defined or if there is no associated media device.

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

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

diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
index b6a72d297775..7a38176259ad 100644
--- a/drivers/media/v4l2-core/v4l2-dev.c
+++ b/drivers/media/v4l2-core/v4l2-dev.c
@@ -87,13 +87,60 @@ 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;
+	dev_t devt = 0;
+
+	buf[0] = '\0';
+	if (v4l2_dev)
+		devt = media_device_devt(v4l2_dev->mdev);
+	if (!devt)
+		return 0;
+	return sprintf(buf, "%u:%u\n", MAJOR(devt), MINOR(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

[PATCHv3 4/4] dvbdev: add /sys media_dev attr for DVB devices

[Hans Verkuil]

For each DVB device node create a media_dev attribute in /sys
which contains the media device major and minor number.

It is not created if the CONFIG_MEDIA_CONTROLLER_DVB is not
defined or if there is no associated media device.

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 | 45 +++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/drivers/media/dvb-core/dvbdev.c b/drivers/media/dvb-core/dvbdev.c
index 5ff7bedee247..db8bddcc5780 100644
--- a/drivers/media/dvb-core/dvbdev.c
+++ b/drivers/media/dvb-core/dvbdev.c
@@ -452,6 +452,48 @@ 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);
+	dev_t devt = media_device_devt(dvbdev->adapter->mdev);
+
+	buf[0] = '\0';
+	if (!devt)
+		return 0;
+	return sprintf(buf, "%u:%u\n", MAJOR(devt), MINOR(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 +1098,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: [PATCHv3 0/4] Add /sys media_dev attr for V4L/DVB devices

[Sebastian Fricke]

Hey Hans,

I have tested your patch with the same routine, that we've discussed in
the previous mail series. Here are the results:
https://paste.debian.net/1183755/

You probably still have to adjust the compliance tool, right?

Here is how the media_dev node looks like for me:
```
basti@nanopct4:~$ cat /sys/class/video4linux/v4l-subdev3/media_dev
240:0
# corresponds to:
basti@nanopct4:~$ ls -l /dev/
...
crw-rw---- 1 root video   240,   0 Feb  3 06:10 media0
...
# leads to:
basti@nanopct4:~$ cat /sys/dev/char/240:0/model
rkisp1
```

Greetings and thank you,
Sebastian

On 02.02.2021 15:49, Hans Verkuil wrote:
>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.
>
>The original idea for this v3 was to store the media device minor/major
>at the time of the device node registration, however the media device
>node is typically created last, so this information isn't known yet.
>
>Instead a new media_device_devt() function was added that returns the
>dev_t of the media device node safely.
>
>In addition a patch was added for vim2m to ensure that the v4l2_dev.mdev
>is set before video_register_device() to ensure that the media_dev
>attribute is created.
>
>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 v2: add new media_device_devt function.
>        Add vim2m patch: v4l2_dev.mdev was set too late.
>
>Changes since v1: use the is_visible callback to ensure the attribute
>        is only shown if mdev is non-NULL.
>
>Hans Verkuil (4):
>  vim2m: intialize the media device earlier
>  media-device: add media_device_devt function
>  v4l2-dev: add /sys media_dev attr for V4L2 devices
>  dvbdev: add /sys media_dev attr for DVB devices
>
> drivers/media/dvb-core/dvbdev.c    | 45 +++++++++++++++++++++++++++
> drivers/media/mc/mc-device.c       | 16 ++++++++++
> drivers/media/test-drivers/vim2m.c | 14 +++++----
> drivers/media/v4l2-core/v4l2-dev.c | 49 +++++++++++++++++++++++++++++-
> include/media/media-device.h       | 10 ++++++
> 5 files changed, 127 insertions(+), 7 deletions(-)
>
>-- 
>2.29.2
>

Re: [PATCHv3 0/4] Add /sys media_dev attr for V4L/DVB devices

[Hans Verkuil]

On 03/02/2021 06:25, Sebastian Fricke wrote:
> Hey Hans,
> 
> I have tested your patch with the same routine, that we've discussed in
> the previous mail series. Here are the results:
> https://paste.debian.net/1183755/
> 
> You probably still have to adjust the compliance tool, right?

That's correct. I'm waiting for this to get the OK before I work
on v4l-utils.

> 
> Here is how the media_dev node looks like for me:
> ```
> basti@nanopct4:~$ cat /sys/class/video4linux/v4l-subdev3/media_dev
> 240:0
> # corresponds to:
> basti@nanopct4:~$ ls -l /dev/
> ...
> crw-rw---- 1 root video   240,   0 Feb  3 06:10 media0
> ...
> # leads to:
> basti@nanopct4:~$ cat /sys/dev/char/240:0/model
> rkisp1

Nice.

Regards,

	Hans

> ```
> 
> Greetings and thank you,
> Sebastian
> 
> On 02.02.2021 15:49, Hans Verkuil wrote:
>> 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.
>>
>> The original idea for this v3 was to store the media device minor/major
>> at the time of the device node registration, however the media device
>> node is typically created last, so this information isn't known yet.
>>
>> Instead a new media_device_devt() function was added that returns the
>> dev_t of the media device node safely.
>>
>> In addition a patch was added for vim2m to ensure that the v4l2_dev.mdev
>> is set before video_register_device() to ensure that the media_dev
>> attribute is created.
>>
>> 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 v2: add new media_device_devt function.
>>        Add vim2m patch: v4l2_dev.mdev was set too late.
>>
>> Changes since v1: use the is_visible callback to ensure the attribute
>>        is only shown if mdev is non-NULL.
>>
>> Hans Verkuil (4):
>>  vim2m: intialize the media device earlier
>>  media-device: add media_device_devt function
>>  v4l2-dev: add /sys media_dev attr for V4L2 devices
>>  dvbdev: add /sys media_dev attr for DVB devices
>>
>> drivers/media/dvb-core/dvbdev.c    | 45 +++++++++++++++++++++++++++
>> drivers/media/mc/mc-device.c       | 16 ++++++++++
>> drivers/media/test-drivers/vim2m.c | 14 +++++----
>> drivers/media/v4l2-core/v4l2-dev.c | 49 +++++++++++++++++++++++++++++-
>> include/media/media-device.h       | 10 ++++++
>> 5 files changed, 127 insertions(+), 7 deletions(-)
>>
>> -- 
>> 2.29.2
>>