Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 26 Dec 2022 11:52:23 +0200
From:   Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To:     Kieran Bingham <kieran.bingham@ideasonboard.com>
Cc:     linux-media@vger.kernel.org,
        Mauro Carvalho Chehab <mchehab@kernel.org>,
        Sakari Ailus <sakari.ailus@linux.intel.com>,
        Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>,
        Hans Verkuil <hverkuil-cisco@xs4all.nl>,
        Christophe JAILLET <christophe.jaillet@wanadoo.fr>,
        open list <linux-kernel@vger.kernel.org>
Subject: Re: [RFC PATCH] media: v4l2-dev: sysfs: Support streaming attribute
Message-ID: <Y6lu14VsuH1LbqFH@pendragon.ideasonboard.com>
References: <20221223231736.2111774-1-kieran.bingham@ideasonboard.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20221223231736.2111774-1-kieran.bingham@ideasonboard.com>
Precedence: bulk

Hi Kieran,

Thank you for the patch.

On Fri, Dec 23, 2022 at 11:17:35PM +0000, Kieran Bingham wrote:
> Provide a streaming attribute to allow userspace to interogate if a device
> is actively streaming or not.
> 
> This will allow desktop notifications to report if a camera or device
> is active on the system, rather than just 'open' which can occur when
> configuring the device.
> 
> Bug: https://gitlab.freedesktop.org/pipewire/pipewire/-/issues/2669
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
> 
> This is a quick POC to see if such a facility makes sense.
> I'm weary that not all video devices may have the queues registered on
> the struct video_device, but this seems like an effective way to be able
> to determine if a device is actively streaming on a system.

I can imagine multiple problems, from race conditions to permissions and
privacy. In order to comment on the fitness of this solution to address
the problem you're trying to solve, could you describe the actual
problem ?

>  Documentation/ABI/stable/sysfs-class-video |  9 +++++++++
>  MAINTAINERS                                |  1 +
>  drivers/media/v4l2-core/v4l2-dev.c         | 13 +++++++++++++
>  3 files changed, 23 insertions(+)
>  create mode 100644 Documentation/ABI/stable/sysfs-class-video
> 
> diff --git a/Documentation/ABI/stable/sysfs-class-video b/Documentation/ABI/stable/sysfs-class-video
> new file mode 100644
> index 000000000000..99dd27475a92
> --- /dev/null
> +++ b/Documentation/ABI/stable/sysfs-class-video
> @@ -0,0 +1,9 @@
> +What:		/sys/class/video4linux/video<n>/streaming
> +Date:		January 2023
> +KernelVersion:	6.3
> +Contact:	Kieran Bingham <kieran.bingham@ideasonboard.com>
> +Description:
> +		Indicates if the video device has an actively streaming queue.
> +		This may indicate that the device is capturing or outputing
> +		video data.
> +Users:
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 11987154eeee..8449f5a6c0da 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -12867,6 +12867,7 @@ S:	Maintained
>  W:	https://linuxtv.org
>  Q:	http://patchwork.kernel.org/project/linux-media/list/
>  T:	git git://linuxtv.org/media_tree.git
> +F:	Documentation/ABI/stable/sysfs-class-video
>  F:	Documentation/admin-guide/media/
>  F:	Documentation/devicetree/bindings/media/
>  F:	Documentation/driver-api/media/
> diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
> index 397d553177fa..7d800309d076 100644
> --- a/drivers/media/v4l2-core/v4l2-dev.c
> +++ b/drivers/media/v4l2-core/v4l2-dev.c
> @@ -30,6 +30,7 @@
>  #include <media/v4l2-device.h>
>  #include <media/v4l2-ioctl.h>
>  #include <media/v4l2-event.h>
> +#include <media/videobuf2-core.h>
>  
>  #define VIDEO_NUM_DEVICES	256
>  #define VIDEO_NAME              "video4linux"
> @@ -85,7 +86,19 @@ static ssize_t name_show(struct device *cd,
>  }
>  static DEVICE_ATTR_RO(name);
>  
> +static ssize_t streaming_show(struct device *cd,
> +			      struct device_attribute *attr, char *buf)
> +{
> +	struct video_device *vdev = to_video_device(cd);
> +
> +	int active = vdev->queue ? vb2_is_streaming(vdev->queue) : false;
> +
> +	return sprintf(buf, "%s\n", active ? "active" : "inactive");
> +}
> +static DEVICE_ATTR_RO(streaming);
> +
>  static struct attribute *video_device_attrs[] = {
> +	&dev_attr_streaming.attr,
>  	&dev_attr_name.attr,
>  	&dev_attr_dev_debug.attr,
>  	&dev_attr_index.attr,

-- 
Regards,

Laurent Pinchart