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

On Thu, Sep 6, 2018 at 4:01 PM Hans Verkuil [off-list ref] wrote:

On 09/05/2018 06:29 PM, Paul Kocialkowski wrote:

quoted

Hi and thanks for the review!

Le lundi 03 septembre 2018 ? 11:11 +0200, Hans Verkuil a ?crit :

quoted

On 08/28/2018 09:34 AM, Paul Kocialkowski wrote:

quoted

+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.

quoted

quoted

+           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?

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.

Best regards,
Tomasz