Re: [RFC PATCH 6/9] media: vb2: add support for requests in QBUF ioctl

[Hans Verkuil]

On 01/15/2018 09:24 AM, Alexandre Courbot wrote:
> On Fri, Jan 12, 2018 at 8:37 PM, Hans Verkuil <hverkuil@xxxxxxxxx> wrote:
>> On 12/15/17 08:56, Alexandre Courbot wrote:
>>> Support the request argument of the QBUF ioctl.
>>>
>>> Signed-off-by: Alexandre Courbot <acourbot@xxxxxxxxxxxx>
>>> ---
>>>  drivers/media/v4l2-core/v4l2-ioctl.c | 93 +++++++++++++++++++++++++++++++++++-
>>>  1 file changed, 92 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
>>> index 8d041247e97f..28f9c368563e 100644
>>> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
>>> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
>>> @@ -29,6 +29,7 @@
>>>  #include <media/v4l2-device.h>
>>>  #include <media/videobuf2-v4l2.h>
>>>  #include <media/v4l2-mc.h>
>>> +#include <media/media-request.h>
>>>
>>>  #include <trace/events/v4l2.h>
>>>
>>> @@ -965,6 +966,81 @@ static int check_fmt(struct file *file, enum v4l2_buf_type type)
>>>       return -EINVAL;
>>>  }
>>>
>>> +/*
>>> + * Validate that a given request can be used during an ioctl.
>>> + *
>>> + * When using the request API, request file descriptors must be matched against
>>> + * the actual request object. User-space can pass any file descriptor, so we
>>> + * need to make sure the call is valid before going further.
>>> + *
>>> + * This function looks up the request and associated data and performs the
>>> + * following sanity checks:
>>> + *
>>> + * * Make sure that the entity supports requests,
>>> + * * Make sure that the entity belongs to the media_device managing the passed
>>> + *   request,
>>> + * * Make sure that the entity data (if any) is associated to the current file
>>> + *   handler.
>>> + *
>>> + * This function returns a pointer to the valid request, or and error code in
>>> + * case of failure. When successful, a reference to the request is acquired and
>>> + * must be properly released.
>>> + */
>>> +#ifdef CONFIG_MEDIA_CONTROLLER
>>> +static struct media_request *
>>> +check_request(int request, struct file *file, void *fh)
>>> +{
>>> +     struct media_request *req = NULL;
>>> +     struct video_device *vfd = video_devdata(file);
>>> +     struct v4l2_fh *vfh =
>>> +             test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? fh : NULL;
>>> +     struct media_entity *entity = &vfd->entity;
>>> +     const struct media_entity *ent;
>>> +     struct media_request_entity_data *data;
>>> +     bool found = false;
>>> +
>>> +     if (!entity)
>>> +             return ERR_PTR(-EINVAL);
>>> +
>>> +     /* Check that the entity supports requests */
>>> +     if (!entity->req_ops)
>>> +             return ERR_PTR(-ENOTSUPP);
>>> +
>>> +     req = media_request_get_from_fd(request);
>>
>> You can get the media_device from vfd->v4l2_dev->mdev. So it is much easier
>> to just pass the media_device as an argument to media_request_get_from_fd()...
>>
>>> +     if (!req)
>>> +             return ERR_PTR(-EINVAL);
>>> +
>>> +     /* Validate that the entity belongs to the media_device managing
>>> +      * the request queue */
>>> +     media_device_for_each_entity(ent, req->queue->mdev) {
>>> +             if (entity == ent) {
>>> +                     found = true;
>>> +                     break;
>>> +             }
>>> +     }
>>> +     if (!found) {
>>> +             media_request_put(req);
>>> +             return ERR_PTR(-EINVAL);
>>> +     }
>>
>> ...and then you don't need to do this ^^^ extra validation check.
> 
> Ah right, all you need to do is check that req->queue->mdev ==
> vfd->v4l2_dev->mdev and you can get rid of this whole block.

Correct.

 I don't
> think we can do that in media_request_get_from_fd() though, since it
> is called from other places where (IIUC) we don't have access to the
> media_device.

Yes, sorry about that. Ignore my comment about media_request_get_from_fd().
I misunderstood what that function did.

> 
>>
>>> +
>>> +     /* Validate that the entity's data belongs to the correct fh */
>>> +     data = media_request_get_entity_data(req, entity, vfh);
>>> +     if (IS_ERR(data)) {
>>> +             media_request_put(req);
>>> +             return ERR_PTR(PTR_ERR(data));
>>> +     }
>>
>> This assumes that each filehandle has its own state. That's true for codecs,
>> but not for most (all?) other devices. There the state is per device instance.
>>
>> I'm not sure if we have a unique identifying mark for such drivers. The closest
>> is checking if fh->m2m_ctx is non-NULL, but I don't know if all drivers with
>> per-filehandle state use that field. An alternative might be to check if
>> fh->ctrl_handler is non-NULL. But again, I'm not sure if that's a 100% valid
>> check.
> 
> I think the current code already takes that case into account: if the
> device does not uses v4l2_fh, then the fh argument passed to
> media_request_get_entity_data() will be null, and so will be data->fh.
> 

Using v4l2_fh doesn't mean it has per-filehandle state. Most drivers only have global
state and they still use v4l2_fh.

This is why you should also look at vivid and vimc for the request API otherwise you
miss what non-codec drivers do. And those are the vast bulk of the drivers.

Regards,

	Hans