From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 40A95CAC5BB for ; Wed, 1 Oct 2025 21:41:53 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:Content-Transfer-Encoding: Content-Type:In-Reply-To:From:References:Cc:To:Subject:MIME-Version:Date: Message-ID:Reply-To:Content-ID:Content-Description:Resent-Date:Resent-From: Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=9jfJUnPvTxcBMRlTiEiZPblzShDRedv1sQmdjn3No6s=; b=dfjXLZeV7ddlsUMzbEdyylI0Z7 rDTu5Osw9N4IOq66pIti5uWg12G5/Z5AP/eX4wyjjk1VkKn/vwObAkCyNDEiOkWAVr+9SjRoLpmy3 eLQP1k0D+4PPep21bwjFcDXp9PNDfgUM4WCDo4C7Nd7RUcZ3zlnKHuvjSWSxOpnql6kpCssEs7I8Y MBMe7WlVRK3Hdjg+Uv1MYzfOjc0QVvuww3NIt4kUZCidJOj6r3o3q0U+4qmbJnpXRjiHXuLxEASJ8 Qis0gXTj7cq3G3TEilvla/YAdtWN9tal3qpJkCgqXYN13FefQp9Iu/feyZQVvF5kG+pBQPAeYhCSl UMkdlLuw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.98.2 #2 (Red Hat Linux)) id 1v44a0-0000000953F-0XLn; Wed, 01 Oct 2025 21:41:44 +0000 Received: from bali.collaboradmins.com ([2a01:4f8:201:9162::2]) by bombadil.infradead.org with esmtps (Exim 4.98.2 #2 (Red Hat Linux)) id 1v44Zy-0000000952Z-0YZy; Wed, 01 Oct 2025 21:41:43 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=collabora.com; s=mail; t=1759354899; bh=teBzgg6W66TCegIEp7ZYSh7bHyXJUjH+ENcqJQ17cqs=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=Excnw8LIXnQ/LNhbk55aewH9+GfqyTaDKYKtMOVqBxvZYHyJUcxX8isJFBk+RUbGY VnEtzqf5we8wjMq+M70lpd3rMDHY7xiFrQcRqg9UVxlHzo7GfsHKJKpBLER7AHdRVW +/ywcqH6Nozs/jUdF9eibULEFlx7n71aoNPlbq8OSn0PDnQk2zWY30VVUd7TXuyTHe E7Wu/5FVm/uprZnT+5/4M3OQ70dXE1nNCyiIb1AQ5RBs+clm7Ck35PGhfbNCD6ftec 6hkFb3jKCKiKWyjvAGKMwkI/YLMcXi4y7IBWqmwPzM72kqf/jHLY19pkfSGW+qAyJk BYkh82tLhWAcg== Received: from [10.40.0.100] (185-67-175-126.lampert.tv [185.67.175.126]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) (Authenticated sender: mriesch) by bali.collaboradmins.com (Postfix) with ESMTPSA id C3CE917E12C0; Wed, 1 Oct 2025 23:41:38 +0200 (CEST) Message-ID: Date: Wed, 1 Oct 2025 23:41:38 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH 06/26] media: v4l2-ioctl: Introduce VIDIOC_BIND_CONTEXT To: Jacopo Mondi , Sakari Ailus , Laurent Pinchart , Tomi Valkeinen , Kieran Bingham , Nicolas Dufresne , Mauro Carvalho Chehab , Tomasz Figa , Marek Szyprowski , Raspberry Pi Kernel Maintenance , Florian Fainelli , Broadcom internal kernel review list , Hans Verkuil Cc: linux-kernel@vger.kernel.org, linux-media@vger.kernel.org, linux-rpi-kernel@lists.infradead.org, linux-arm-kernel@lists.infradead.org References: <20250717-multicontext-mainline-2025-v1-0-81ac18979c03@ideasonboard.com> <20250717-multicontext-mainline-2025-v1-6-81ac18979c03@ideasonboard.com> Content-Language: en-US From: Michael Riesch In-Reply-To: <20250717-multicontext-mainline-2025-v1-6-81ac18979c03@ideasonboard.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20251001_144142_372453_5BF5300E X-CRM114-Status: GOOD ( 38.99 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Hi Jacopo, On 7/17/25 12:45, Jacopo Mondi wrote: > Introduce a new ioctl in V4L2 to allocate a video device context and > associate it with a media device context. > > The ioctl is valid only if support for MEDIA_CONTROLLER is compiled in > as it calls into the entity ops to let driver allocate a new context and > binds the newly created context with the media context associated > with the file descriptor provided by userspace as the new ioctl > argument. I would have expected that the execution context of the video device already exists and is not allocated at ioctl call time. If I understand it correctly - after opening a video device, no context is allocated, but in v4l2_fh_release the reference counter of the context is decreased. This smells fishy. Note that the user may not call the ioctl. - after opening a video device there is no context. This could imply that two operating modes are required (with a context and without a context), which would seem unnecessarily complex. - What happens if the VIDIOC_BIND_CONTEXT ioctl is called more than once? (IIUC vfh->context gets overwritten but the old context is not released) (Just found that a later patch introduces default contexts. Should this address the comments above, consider rearranging the patches so that default contexts are introduced first.) > The newly allocated video context is then stored in the v4l2-fh that > represent the open file handle on which the ioctl has been called. Couldn't the same be achieved by - v4l2_fh_open allocates a new context - v4l2_fh_release releases it (already implemented) - ioctl takes the existing context and binds it to the media device context Then, - open/release are symmetric and not fishy - after open but before the ioctl call the user can safely operate on a context - calling ioctl twice will be idempotent (check already implemented in media_device_bind_context()) > > Signed-off-by: Jacopo Mondi > --- > drivers/media/v4l2-core/v4l2-dev.c | 10 ++++++ > drivers/media/v4l2-core/v4l2-fh.c | 1 + > drivers/media/v4l2-core/v4l2-ioctl.c | 64 ++++++++++++++++++++++++++++++++++++ > include/media/v4l2-ioctl.h | 7 ++++ > include/uapi/linux/videodev2.h | 11 +++++++ > 5 files changed, 93 insertions(+) > > diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c > index c83c37843c9e7beb899a4b2bd176273c3dec381b..bc6502b4ce21cc0ad53136e1637d1c926e31dd89 100644 > --- a/drivers/media/v4l2-core/v4l2-dev.c > +++ b/drivers/media/v4l2-core/v4l2-dev.c > @@ -606,6 +606,10 @@ static void determine_valid_ioctls(struct video_device *vdev) > __set_bit(_IOC_NR(VIDIOC_ENUM_FREQ_BANDS), valid_ioctls); > > if (is_vid) { > +#ifdef CONFIG_MEDIA_CONTROLLER > + __set_bit(_IOC_NR(VIDIOC_BIND_CONTEXT), valid_ioctls); > +#endif > + > /* video specific ioctls */ > if ((is_rx && (ops->vidioc_enum_fmt_vid_cap || > ops->vidioc_enum_fmt_vid_overlay)) || > @@ -661,12 +665,18 @@ static void determine_valid_ioctls(struct video_device *vdev) > SET_VALID_IOCTL(ops, VIDIOC_G_FMT, vidioc_g_fmt_meta_cap); > SET_VALID_IOCTL(ops, VIDIOC_S_FMT, vidioc_s_fmt_meta_cap); > SET_VALID_IOCTL(ops, VIDIOC_TRY_FMT, vidioc_try_fmt_meta_cap); > +#ifdef CONFIG_MEDIA_CONTROLLER > + __set_bit(_IOC_NR(VIDIOC_BIND_CONTEXT), valid_ioctls); > +#endif > } else if (is_meta && is_tx) { > /* metadata output specific ioctls */ > SET_VALID_IOCTL(ops, VIDIOC_ENUM_FMT, vidioc_enum_fmt_meta_out); > SET_VALID_IOCTL(ops, VIDIOC_G_FMT, vidioc_g_fmt_meta_out); > SET_VALID_IOCTL(ops, VIDIOC_S_FMT, vidioc_s_fmt_meta_out); > SET_VALID_IOCTL(ops, VIDIOC_TRY_FMT, vidioc_try_fmt_meta_out); > +#ifdef CONFIG_MEDIA_CONTROLLER > + __set_bit(_IOC_NR(VIDIOC_BIND_CONTEXT), valid_ioctls); > +#endif > } > if (is_vbi) { > /* vbi specific ioctls */ > diff --git a/drivers/media/v4l2-core/v4l2-fh.c b/drivers/media/v4l2-core/v4l2-fh.c > index 90eec79ee995a2d214590beeacc91b9f8f33236d..f7af444d2344541ccae1eae230b39d4cbc47f6bd 100644 > --- a/drivers/media/v4l2-core/v4l2-fh.c > +++ b/drivers/media/v4l2-core/v4l2-fh.c > @@ -93,6 +93,7 @@ int v4l2_fh_release(struct file *filp) > struct v4l2_fh *fh = filp->private_data; > > if (fh) { > + video_device_context_put(fh->context); > v4l2_fh_del(fh); > v4l2_fh_exit(fh); > kfree(fh); > diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c > index 46da373066f4ec786b87ef18b8372abee621332f..bade64cc62b66dd6237ccd5338aa6dd8ab00ef8c 100644 > --- a/drivers/media/v4l2-core/v4l2-ioctl.c > +++ b/drivers/media/v4l2-core/v4l2-ioctl.c > @@ -9,6 +9,7 @@ > */ > > #include > +#include > #include > #include > #include > @@ -350,6 +351,13 @@ static void v4l_print_format(const void *arg, bool write_only) > } > } > > +static void v4l_print_context(const void *arg, bool write_only) > +{ > + const struct v4l2_context *c = arg; > + > + pr_cont("context=%u\n", c->context_fd); > +} > + > static void v4l_print_framebuffer(const void *arg, bool write_only) > { > const struct v4l2_framebuffer *p = arg; > @@ -2151,6 +2159,61 @@ static int v4l_overlay(const struct v4l2_ioctl_ops *ops, > return ops->vidioc_overlay(file, fh, *(unsigned int *)arg); > } > > +static int v4l_bind_context(const struct v4l2_ioctl_ops *ops, > + struct file *file, void *fh, void *arg) > +{ > + struct video_device *vdev = video_devdata(file); > + struct media_device_context *mdev_context; > + struct v4l2_fh *vfh = > + test_bit(V4L2_FL_USES_V4L2_FH, &vdev->flags) ? fh : NULL; > + struct v4l2_context *c = arg; > + int ret; > + > + /* > + * TODO: do not __set_bit(_IOC_NR(VIDIOC_BIND_CONTEXT), valid_ioctls) > + * if V4L2_FL_USES_V4L2_FH isn't set or the driver does not implement > + * alloc_context and destroy_context. > + */ > + if (!vfh) > + return -ENOTTY; > + > + if (!vdev->entity.ops || !vdev->entity.ops->alloc_context || > + !vdev->entity.ops->destroy_context) > + return -ENOTTY; > + > + mdev_context = media_device_context_get_from_fd(c->context_fd); > + if (!mdev_context) > + return -EINVAL; > + > + /* Let the driver allocate the per-file handle context. */ > + ret = vdev->entity.ops->alloc_context(&vdev->entity, > + (struct media_entity_context **) > + &vfh->context); > + if (ret) > + goto err_put_mdev_context; > + > + /* > + * Bind the newly created video device context to the media device > + * context identified by the file descriptor. > + */ > + ret = media_device_bind_context(mdev_context, > + (struct media_entity_context *) > + vfh->context); > + if (ret) > + goto err_put_context; > + > + media_device_context_put(mdev_context); > + > + return 0; > + > +err_put_context: > + video_device_context_put(vfh->context); > +err_put_mdev_context: > + media_device_context_put(mdev_context); > + > + return ret; > +} > + > static int v4l_reqbufs(const struct v4l2_ioctl_ops *ops, > struct file *file, void *fh, void *arg) > { > @@ -2998,6 +3061,7 @@ static const struct v4l2_ioctl_info v4l2_ioctls[] = { > IOCTL_INFO(VIDIOC_DBG_G_CHIP_INFO, v4l_dbg_g_chip_info, v4l_print_dbg_chip_info, INFO_FL_CLEAR(v4l2_dbg_chip_info, match)), > IOCTL_INFO(VIDIOC_QUERY_EXT_CTRL, v4l_query_ext_ctrl, v4l_print_query_ext_ctrl, INFO_FL_CTRL | INFO_FL_CLEAR(v4l2_query_ext_ctrl, id)), > IOCTL_INFO(VIDIOC_REMOVE_BUFS, v4l_remove_bufs, v4l_print_remove_buffers, INFO_FL_PRIO | INFO_FL_QUEUE | INFO_FL_CLEAR(v4l2_remove_buffers, type)), > + IOCTL_INFO(VIDIOC_BIND_CONTEXT, v4l_bind_context, v4l_print_context, 0), > }; > #define V4L2_IOCTLS ARRAY_SIZE(v4l2_ioctls) > > diff --git a/include/media/v4l2-ioctl.h b/include/media/v4l2-ioctl.h > index 82695c3a300a73219f262fb556ed61a8f09d273e..6d9edfd9ca912972ad15acdc07014dee1ed36ab6 100644 > --- a/include/media/v4l2-ioctl.h > +++ b/include/media/v4l2-ioctl.h > @@ -18,6 +18,7 @@ > #include > > struct v4l2_fh; > +struct video_device_context; > > /** > * struct v4l2_ioctl_ops - describe operations for each V4L2 ioctl > @@ -149,6 +150,8 @@ struct v4l2_fh; > * :ref:`VIDIOC_TRY_FMT ` ioctl logic for metadata capture > * @vidioc_try_fmt_meta_out: pointer to the function that implements > * :ref:`VIDIOC_TRY_FMT ` ioctl logic for metadata output > + * @vidioc_bind_context: pointer to the function that implements > + * :ref:`VIDIOC_BIND_CONTEXT ` ioctl > * @vidioc_reqbufs: pointer to the function that implements > * :ref:`VIDIOC_REQBUFS ` ioctl > * @vidioc_querybuf: pointer to the function that implements > @@ -402,6 +405,10 @@ struct v4l2_ioctl_ops { > int (*vidioc_try_fmt_meta_out)(struct file *file, void *fh, > struct v4l2_format *f); > > + /* Context handlers */ > + int (*vidioc_bind_context)(struct file *file, void *fh, > + struct video_device_context *c); > + > /* Buffer handlers */ > int (*vidioc_reqbufs)(struct file *file, void *fh, > struct v4l2_requestbuffers *b); > diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h > index 3dd9fa45dde1066d52a68581625a39e7ec92c9b7..0b9aa89e2479620dbbaa54f1aadff7aaa7a3d0f7 100644 > --- a/include/uapi/linux/videodev2.h > +++ b/include/uapi/linux/videodev2.h > @@ -1057,6 +1057,14 @@ struct v4l2_jpegcompression { > * always use APP0 */ > }; > > +/* > + * V I D E O D E V I C E C O N T E X T > + */ > + > +struct v4l2_context { > + __u32 context_fd; Reserve some space for the future? > +}; > + > /* > * M E M O R Y - M A P P I N G B U F F E R S > */ > @@ -2818,6 +2826,9 @@ struct v4l2_remove_buffers { > #define VIDIOC_REMOVE_BUFS _IOWR('V', 104, struct v4l2_remove_buffers) > > > +/* Context handling */ > +#define VIDIOC_BIND_CONTEXT _IOW('V', 105, struct v4l2_context) > + > /* Reminder: when adding new ioctls please add support for them to > drivers/media/v4l2-core/v4l2-compat-ioctl32.c as well! */ > > Best regards, Michael