Re: [PATCH v8 4/8] media: platform: Add Cedrus VPU decoder driver

[Hans Verkuil]

On 09/06/2018 09:25 AM, Tomasz Figa wrote:
> On Thu, Sep 6, 2018 at 4:01 PM Hans Verkuil <hverkuil@xxxxxxxxx> wrote:
>>
>> On 09/05/2018 06:29 PM, Paul Kocialkowski wrote:
>>> Hi and thanks for the review!
>>>
>>> Le lundi 03 septembre 2018 Ã 11:11 +0200, Hans Verkuil a Ãcrit :
>>>> On 08/28/2018 09:34 AM, Paul Kocialkowski wrote:
>>>>> +static int cedrus_request_validate(struct media_request *req)
>>>>> +{
>>>>> +   struct media_request_object *obj, *obj_safe;
>>>>> +   struct v4l2_ctrl_handler *parent_hdl, *hdl;
>>>>> +   struct cedrus_ctx *ctx = NULL;
>>>>> +   struct v4l2_ctrl *ctrl_test;
>>>>> +   unsigned int i;
>>>>> +
>>>>> +   list_for_each_entry_safe(obj, obj_safe, &req->objects, list) {
>>>>
>>>> You don't need to use the _safe variant during validation.
>>>
>>> Okay, I'll use the regular one then.
>>>
>>>>> +           struct vb2_buffer *vb;
>>>>> +
>>>>> +           if (vb2_request_object_is_buffer(obj)) {
>>>>> +                   vb = container_of(obj, struct vb2_buffer, req_obj);
>>>>> +                   ctx = vb2_get_drv_priv(vb->vb2_queue);
>>>>> +
>>>>> +                   break;
>>>>> +           }
>>>>> +   }
>>>>
>>>> Interesting question: what happens if more than one buffer is queued in the
>>>> request? This is allowed by the request API and in that case the associated
>>>> controls in the request apply to all queued buffers.
>>>>
>>>> Would this make sense at all for this driver? If not, then you need to
>>>> check here if there is more than one buffer in the request and document in
>>>> the spec that this is not allowed.
>>>
>>> Well, our driver was written with the (unformal) assumption that we
>>> only deal with a pair of one output and one capture buffer. So I will
>>> add a check for this at request validation time and document it in the
>>> spec. Should that be part of the MPEG-2 PIXFMT documentation (and
>>> duplicated for future formats we add support for)?
>>
>> Can you make a patch for vb2_request_has_buffers() in videobuf2-core.c
>> renaming it to vb2_request_buffer_cnt() and returning the number of buffers
>> in the request?
>>
>> Then you can call it here to check that you have only one buffer.
>>
>> And this has to be documented with the PIXFMT.
>>
>> Multiple buffers are certainly possible in non-codec scenarios (vim2m and
>> vivid happily accept that), so this is an exception that should be
>> documented and checked in the codec driver.
> 
> Hmm, isn't it still 1 buffer per 1 queue and just multiple queues
> included in the request?

No. The request API allows multiple buffers for the same vb2_queue to be
queued for the same request (obviously when the request is committed, the
buffers are queued to the driver in the same order).

> 
> If we indeed allow multiple buffers for the same queue in a request,
> we shouldn't restrict this on a per-driver basis. It's definitely not
> a hardware limitation, since the driver could just do the same as if 2
> requests with the same controls were given.

That's how it operates: for all buffers in the request the same controls
apply. But does this make sense for codecs? If the control(s) with the
codec metadata always change for every buffer, then having more than one
buffer in the request is senseless and the driver should check for this
in the validation step.

If it *does* make sense in some circumstances to have the same metadata
for multiple buffers, then it should be checked if the cedrus driver
handles this correctly.

Regards,

	Hans