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

[Alexandre Courbot]

On Mon, Jan 15, 2018 at 6:19 PM, Hans Verkuil <hverkuil@xxxxxxxxx> wrote:
> 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.

Thanks for spotting this, I clearly misunderstood the purpose of
v4l2_fh here. I will take a look as you suggested.