From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tomasz Figa Subject: Re: [PATCH v8 4/8] media: platform: Add Cedrus VPU decoder driver Date: Thu, 6 Sep 2018 16:25:34 +0900 Message-ID: References: <20180828073424.30247-1-paul.kocialkowski@bootlin.com> <20180828073424.30247-5-paul.kocialkowski@bootlin.com> <5faf5eed-eb2c-f804-93e3-5a42f6204d99@xs4all.nl> <461c6a0d-a346-b9da-b75e-4aab907054df@xs4all.nl> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Return-path: In-Reply-To: <461c6a0d-a346-b9da-b75e-4aab907054df@xs4all.nl> Sender: linux-kernel-owner@vger.kernel.org To: Hans Verkuil Cc: contact@paulk.fr, Linux Media Mailing List , devicetree@vger.kernel.org, Linux Kernel Mailing List , "list@263.net:IOMMU DRIVERS , Joerg Roedel ," , devel@driverdev.osuosl.org, Mauro Carvalho Chehab , Rob Herring , Mark Rutland , Maxime Ripard , Chen-Yu Tsai , Greg KH , thomas.petazzoni@bootlin.com, ayaka , Ezequiel Garcia , Alexandre Courbot , Philipp Zabel , Laurent Pinchart List-Id: devicetree@vger.kernel.org On Thu, Sep 6, 2018 at 4:01 PM Hans Verkuil wrote: > > On 09/05/2018 06:29 PM, Paul Kocialkowski wrote: > > Hi and thanks for the review! > > > > Le lundi 03 septembre 2018 =C3=A0 11:11 +0200, Hans Verkuil a =C3=A9cri= t : > >> 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 =3D 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 =3D container_of(obj, struct vb2_buffer, req_o= bj); > >>> + ctx =3D vb2_get_drv_priv(vb->vb2_queue); > >>> + > >>> + break; > >>> + } > >>> + } > >> > >> Interesting question: what happens if more than one buffer is queued i= n the > >> request? This is allowed by the request API and in that case the assoc= iated > >> 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 documen= t 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 buffe= rs > 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