Message-ID: <1ba4a5b3-abe8-4580-abf5-1e0fe19f9fb5@collabora.com>
Date: Fri, 3 Oct 2025 09:40:39 +0200
From: Michael Riesch <michael.riesch@...labora.com>
To: Jacopo Mondi <jacopo.mondi@...asonboard.com>
Cc: Sakari Ailus <sakari.ailus@...ux.intel.com>,
 Laurent Pinchart <laurent.pinchart@...asonboard.com>,
 Tomi Valkeinen <tomi.valkeinen@...asonboard.com>,
 Kieran Bingham <kieran.bingham@...asonboard.com>,
 Nicolas Dufresne <nicolas.dufresne@...labora.com>,
 Mauro Carvalho Chehab <mchehab@...nel.org>, Tomasz Figa
 <tfiga@...omium.org>, Marek Szyprowski <m.szyprowski@...sung.com>,
 Raspberry Pi Kernel Maintenance <kernel-list@...pberrypi.com>,
 Florian Fainelli <florian.fainelli@...adcom.com>,
 Broadcom internal kernel review list
 <bcm-kernel-feedback-list@...adcom.com>, Hans Verkuil <hverkuil@...nel.org>,
 linux-kernel@...r.kernel.org, linux-media@...r.kernel.org,
 linux-rpi-kernel@...ts.infradead.org, linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH 06/26] media: v4l2-ioctl: Introduce VIDIOC_BIND_CONTEXT

Hi Jacopo,

On 10/2/25 10:48, Jacopo Mondi wrote:
> Hi Michael
> 
> On Wed, Oct 01, 2025 at 11:41:38PM +0200, Michael Riesch wrote:
>> 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
> 
> If userspace doesn't use contexts, why pre-allocate it ?

This is more along the lines "if it keeps things simple, why not". But I
am still diving into this and may have not gotten the complete picture yet.

> See also below on the implications of using a context regardless of
> userspace actions
> 
>> 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.
> 
> As far as I can see v4l2_fh_release() calls:
> 
> void video_device_context_put(struct video_device_context *ctx)
> {
>        if (!ctx)
>                return;
> 
>        media_entity_context_put(&ctx->base);
> }
> 
> which is safe is !ctx

Ack.

> 
>>  - 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.
> 
> You'll find out later on that I have introduced a default context for
> this purpose

Right, I'll check this out.

> 
>>  - 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)
> 
> Do you mean:
> 
> - Multiple file handles representing the same video device are bound
>   multiple times to the same media device context ?
> 
>   media_device_bind_context() called from v4l_bind_context() returns an error
> 
> - An already bound video device fh is bound to (different) media
>   device contexts ?
> 
>   I should probably
> 
>   if (vfh->context)
>         return -EINVAL;
> 
>   in v4l_bind_context()
> 
>   As an already bound context cannot be re-bound. There currently is
>   not un-binding mechanism, it is required to close a file handle to
>   unbind.

The latter. The check you proposed should do the trick.

>> (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.)
> 
> To be honest I don't see much difference. I'll see if it's practical
> to do so or not.
> 
>>
>>> 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
> 
> Why do you think video_device_context_put() is fishy ?

Not the video_device_context_put itself, but I expected "open()
allocates something, close() release that something". But apparently
there is a good reason to deviate from that...

> 
>>  - after open but before the ioctl call the user can safely operate on a
>>    context
> 
> If we always operate on a per-file-handle context even before binding
> it, all the operations performed by an application, even it doesn't
> use contexts, will be isolated from the rest of the world.
> 
> This might seem desirable, but changes the semantic of all the v4l2
> operations and an application that doesn't use context that runs on a
> driver ported to use context will suddenly find all its configuration
> to be transient and tied to the lifetime of an open file handle
> instead of being device-persistent.
> 
> Using a default, device-wise, default context allows instead existing
> applications that do not use contexts to operate as they are used to,
> with all the setting/configurations being stored in a persistent
> place.

... and there it is. Did not have that in mind, thanks for pointing it out.

> 
> [...]
>>>
>>> +/*
>>> + *     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?
>>
> 
> Might be a good idea. I can't tell how much space we should reserve
> though :)

Prediction is difficult, especially about the future. But having zero
reserve sounds like something we could regret at some point down the
road. __u32 reserved[3]; ?

Best regards,
Michael

> 
>>> +};
>>> +
>>>  /*
>>>   *	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
>>

Powered by blists - more mailing lists