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@xxxxxxxxx>
>> ---
>> 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
>
