From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-0.9 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_PASS, T_DKIMWL_WL_HIGH,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id E6805C433F5 for ; Fri, 7 Sep 2018 08:05:17 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 97D452086B for ; Fri, 7 Sep 2018 08:05:17 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=chromium.org header.i=@chromium.org header.b="ldDlJHXU" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 97D452086B Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=chromium.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728060AbeIGMpA (ORCPT ); Fri, 7 Sep 2018 08:45:00 -0400 Received: from mail-yb1-f195.google.com ([209.85.219.195]:46032 "EHLO mail-yb1-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727826AbeIGMpA (ORCPT ); Fri, 7 Sep 2018 08:45:00 -0400 Received: by mail-yb1-f195.google.com with SMTP id h22-v6so5141407ybg.12 for ; Fri, 07 Sep 2018 01:05:14 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=kZPfF/RCYJRI3BVh+MBiYXmDSzF3fxRbuRGMSb1xQKg=; b=ldDlJHXU5Ndzt/oOEmXSuJRkPrS66JRDYFXEc24CmwssgChxY57OnMtumW1dPpMft/ rSkVqi1uGAjxCDolp2uxNkILwGumfr0hWN58AqASOPavrIr2GwXKEBsJ+4/6tTMiPSvp NLjPjQTmoNuRfR9iF0Gq67dZva7R8B7YcMz7Q= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=kZPfF/RCYJRI3BVh+MBiYXmDSzF3fxRbuRGMSb1xQKg=; b=kd9uEp717tATugeDtCxoIuqH4XdF+Z3UP8vgb8D1hr5/BJZ0zG7DK34ly5V1oUGazS PkjCztYJCJuC07I5qWT1bMqDjc5kTnBtwWoWPfcCpFS/mK+aj5ITnA6s1eJSo10Aanz5 KI8caaeVYiOXvYTfmw5iw+IMdQ5fPTSV/OLouIvqANrQb+OFVZoMV6ONB4xqvpp3u9d8 S+YmGdVK1sgRFEDNEQvjnl/ZbSBOlsJjPQk4CpjwrHRpwWwW+pgbzemM7V7H3/CM5uWD GszPNIw8e9+iiBS0NHOI6/rHUk7Rxdq0JBq2luR9ZpJfaty4JCAb68K6Q05e4KV1TXG0 9ZYQ== X-Gm-Message-State: APzg51BJ/j7jkUSq2LwLD9PeEN8XnL6uqsQT6clXt64E5zM9e8OZDM5u D0SveFh5ne/Y5VUGH8AyniAkDuGFnGwKIw== X-Google-Smtp-Source: ANB0Vdai4TVxwLD3uwbnPGT+utFUsJ5rDoFCXB7rffQ6yjp5ZnfFv+uXniVw/7TuUOa4CKLTaKJQRw== X-Received: by 2002:a25:a123:: with SMTP id z32-v6mr3483810ybh.132.1536307513801; Fri, 07 Sep 2018 01:05:13 -0700 (PDT) Received: from mail-yw1-f46.google.com (mail-yw1-f46.google.com. [209.85.161.46]) by smtp.gmail.com with ESMTPSA id v185-v6sm2483427ywc.94.2018.09.07.01.05.13 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 07 Sep 2018 01:05:13 -0700 (PDT) Received: by mail-yw1-f46.google.com with SMTP id n207-v6so5086007ywn.9 for ; Fri, 07 Sep 2018 01:05:13 -0700 (PDT) X-Received: by 2002:a0d:fdc4:: with SMTP id n187-v6mr3459805ywf.443.1536307513013; Fri, 07 Sep 2018 01:05:13 -0700 (PDT) MIME-Version: 1.0 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> <7a2167a3-5b61-0937-7f06-8b36fa12e7a0@xs4all.nl> In-Reply-To: <7a2167a3-5b61-0937-7f06-8b36fa12e7a0@xs4all.nl> From: Tomasz Figa Date: Fri, 7 Sep 2018 17:05:01 +0900 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH v8 4/8] media: platform: Add Cedrus VPU decoder driver 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 ," , "list@263.net:IOMMU DRIVERS , Joerg Roedel ," , "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 , Sakari Ailus , linux-sunxi@googlegroups.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Sep 6, 2018 at 4:39 PM Hans Verkuil wrote: > > On 09/06/2018 09:25 AM, Tomasz Figa wrote: > > 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=A9c= rit : > >>>> 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= _obj); > >>>>> + ctx =3D 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 ass= ociated > >>>> 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 docum= ent 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 th= e > >>> 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 bu= ffers > >> 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. Just FYI, we may want to move this discussion to Alex's RFC with documentation of stateless interface: https://patchwork.kernel.org/patch/10583233/ Best regards, Tomasz 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: Fri, 7 Sep 2018 17:05:01 +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> <7a2167a3-5b61-0937-7f06-8b36fa12e7a0@xs4all.nl> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: <7a2167a3-5b61-0937-7f06-8b36fa12e7a0@xs4all.nl> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: driverdev-devel-bounces@linuxdriverproject.org Sender: "devel" To: Hans Verkuil Cc: Mark Rutland , Maxime Ripard , contact@paulk.fr, linux-sunxi@googlegroups.com, Laurent Pinchart , thomas.petazzoni@bootlin.com, devel@driverdev.osuosl.org, "list@263.net:IOMMU DRIVERS , Joerg Roedel , " , Chen-Yu Tsai , Linux Media Mailing List , devicetree@vger.kernel.org, Philipp Zabel , ayaka , Rob Herring , Mauro Carvalho Chehab , Ezequiel Garcia , "list@263.net:IOMMU DRIVERS , Joerg Roedel , " , Alexandre Courbot , Greg KH Li List-Id: devicetree@vger.kernel.org T24gVGh1LCBTZXAgNiwgMjAxOCBhdCA0OjM5IFBNIEhhbnMgVmVya3VpbCA8aHZlcmt1aWxAeHM0 YWxsLm5sPiB3cm90ZToKPgo+IE9uIDA5LzA2LzIwMTggMDk6MjUgQU0sIFRvbWFzeiBGaWdhIHdy b3RlOgo+ID4gT24gVGh1LCBTZXAgNiwgMjAxOCBhdCA0OjAxIFBNIEhhbnMgVmVya3VpbCA8aHZl cmt1aWxAeHM0YWxsLm5sPiB3cm90ZToKPiA+Pgo+ID4+IE9uIDA5LzA1LzIwMTggMDY6MjkgUE0s IFBhdWwgS29jaWFsa293c2tpIHdyb3RlOgo+ID4+PiBIaSBhbmQgdGhhbmtzIGZvciB0aGUgcmV2 aWV3IQo+ID4+Pgo+ID4+PiBMZSBsdW5kaSAwMyBzZXB0ZW1icmUgMjAxOCDDoCAxMToxMSArMDIw MCwgSGFucyBWZXJrdWlsIGEgw6ljcml0IDoKPiA+Pj4+IE9uIDA4LzI4LzIwMTggMDk6MzQgQU0s IFBhdWwgS29jaWFsa293c2tpIHdyb3RlOgo+ID4+Pj4+ICtzdGF0aWMgaW50IGNlZHJ1c19yZXF1 ZXN0X3ZhbGlkYXRlKHN0cnVjdCBtZWRpYV9yZXF1ZXN0ICpyZXEpCj4gPj4+Pj4gK3sKPiA+Pj4+ PiArICAgc3RydWN0IG1lZGlhX3JlcXVlc3Rfb2JqZWN0ICpvYmosICpvYmpfc2FmZTsKPiA+Pj4+ PiArICAgc3RydWN0IHY0bDJfY3RybF9oYW5kbGVyICpwYXJlbnRfaGRsLCAqaGRsOwo+ID4+Pj4+ ICsgICBzdHJ1Y3QgY2VkcnVzX2N0eCAqY3R4ID0gTlVMTDsKPiA+Pj4+PiArICAgc3RydWN0IHY0 bDJfY3RybCAqY3RybF90ZXN0Owo+ID4+Pj4+ICsgICB1bnNpZ25lZCBpbnQgaTsKPiA+Pj4+PiAr Cj4gPj4+Pj4gKyAgIGxpc3RfZm9yX2VhY2hfZW50cnlfc2FmZShvYmosIG9ial9zYWZlLCAmcmVx LT5vYmplY3RzLCBsaXN0KSB7Cj4gPj4+Pgo+ID4+Pj4gWW91IGRvbid0IG5lZWQgdG8gdXNlIHRo ZSBfc2FmZSB2YXJpYW50IGR1cmluZyB2YWxpZGF0aW9uLgo+ID4+Pgo+ID4+PiBPa2F5LCBJJ2xs IHVzZSB0aGUgcmVndWxhciBvbmUgdGhlbi4KPiA+Pj4KPiA+Pj4+PiArICAgICAgICAgICBzdHJ1 Y3QgdmIyX2J1ZmZlciAqdmI7Cj4gPj4+Pj4gKwo+ID4+Pj4+ICsgICAgICAgICAgIGlmICh2YjJf cmVxdWVzdF9vYmplY3RfaXNfYnVmZmVyKG9iaikpIHsKPiA+Pj4+PiArICAgICAgICAgICAgICAg ICAgIHZiID0gY29udGFpbmVyX29mKG9iaiwgc3RydWN0IHZiMl9idWZmZXIsIHJlcV9vYmopOwo+ ID4+Pj4+ICsgICAgICAgICAgICAgICAgICAgY3R4ID0gdmIyX2dldF9kcnZfcHJpdih2Yi0+dmIy X3F1ZXVlKTsKPiA+Pj4+PiArCj4gPj4+Pj4gKyAgICAgICAgICAgICAgICAgICBicmVhazsKPiA+ Pj4+PiArICAgICAgICAgICB9Cj4gPj4+Pj4gKyAgIH0KPiA+Pj4+Cj4gPj4+PiBJbnRlcmVzdGlu ZyBxdWVzdGlvbjogd2hhdCBoYXBwZW5zIGlmIG1vcmUgdGhhbiBvbmUgYnVmZmVyIGlzIHF1ZXVl ZCBpbiB0aGUKPiA+Pj4+IHJlcXVlc3Q/IFRoaXMgaXMgYWxsb3dlZCBieSB0aGUgcmVxdWVzdCBB UEkgYW5kIGluIHRoYXQgY2FzZSB0aGUgYXNzb2NpYXRlZAo+ID4+Pj4gY29udHJvbHMgaW4gdGhl IHJlcXVlc3QgYXBwbHkgdG8gYWxsIHF1ZXVlZCBidWZmZXJzLgo+ID4+Pj4KPiA+Pj4+IFdvdWxk IHRoaXMgbWFrZSBzZW5zZSBhdCBhbGwgZm9yIHRoaXMgZHJpdmVyPyBJZiBub3QsIHRoZW4geW91 IG5lZWQgdG8KPiA+Pj4+IGNoZWNrIGhlcmUgaWYgdGhlcmUgaXMgbW9yZSB0aGFuIG9uZSBidWZm ZXIgaW4gdGhlIHJlcXVlc3QgYW5kIGRvY3VtZW50IGluCj4gPj4+PiB0aGUgc3BlYyB0aGF0IHRo aXMgaXMgbm90IGFsbG93ZWQuCj4gPj4+Cj4gPj4+IFdlbGwsIG91ciBkcml2ZXIgd2FzIHdyaXR0 ZW4gd2l0aCB0aGUgKHVuZm9ybWFsKSBhc3N1bXB0aW9uIHRoYXQgd2UKPiA+Pj4gb25seSBkZWFs IHdpdGggYSBwYWlyIG9mIG9uZSBvdXRwdXQgYW5kIG9uZSBjYXB0dXJlIGJ1ZmZlci4gU28gSSB3 aWxsCj4gPj4+IGFkZCBhIGNoZWNrIGZvciB0aGlzIGF0IHJlcXVlc3QgdmFsaWRhdGlvbiB0aW1l IGFuZCBkb2N1bWVudCBpdCBpbiB0aGUKPiA+Pj4gc3BlYy4gU2hvdWxkIHRoYXQgYmUgcGFydCBv ZiB0aGUgTVBFRy0yIFBJWEZNVCBkb2N1bWVudGF0aW9uIChhbmQKPiA+Pj4gZHVwbGljYXRlZCBm b3IgZnV0dXJlIGZvcm1hdHMgd2UgYWRkIHN1cHBvcnQgZm9yKT8KPiA+Pgo+ID4+IENhbiB5b3Ug bWFrZSBhIHBhdGNoIGZvciB2YjJfcmVxdWVzdF9oYXNfYnVmZmVycygpIGluIHZpZGVvYnVmMi1j b3JlLmMKPiA+PiByZW5hbWluZyBpdCB0byB2YjJfcmVxdWVzdF9idWZmZXJfY250KCkgYW5kIHJl dHVybmluZyB0aGUgbnVtYmVyIG9mIGJ1ZmZlcnMKPiA+PiBpbiB0aGUgcmVxdWVzdD8KPiA+Pgo+ ID4+IFRoZW4geW91IGNhbiBjYWxsIGl0IGhlcmUgdG8gY2hlY2sgdGhhdCB5b3UgaGF2ZSBvbmx5 IG9uZSBidWZmZXIuCj4gPj4KPiA+PiBBbmQgdGhpcyBoYXMgdG8gYmUgZG9jdW1lbnRlZCB3aXRo IHRoZSBQSVhGTVQuCj4gPj4KPiA+PiBNdWx0aXBsZSBidWZmZXJzIGFyZSBjZXJ0YWlubHkgcG9z c2libGUgaW4gbm9uLWNvZGVjIHNjZW5hcmlvcyAodmltMm0gYW5kCj4gPj4gdml2aWQgaGFwcGls eSBhY2NlcHQgdGhhdCksIHNvIHRoaXMgaXMgYW4gZXhjZXB0aW9uIHRoYXQgc2hvdWxkIGJlCj4g Pj4gZG9jdW1lbnRlZCBhbmQgY2hlY2tlZCBpbiB0aGUgY29kZWMgZHJpdmVyLgo+ID4KPiA+IEht bSwgaXNuJ3QgaXQgc3RpbGwgMSBidWZmZXIgcGVyIDEgcXVldWUgYW5kIGp1c3QgbXVsdGlwbGUg cXVldWVzCj4gPiBpbmNsdWRlZCBpbiB0aGUgcmVxdWVzdD8KPgo+IE5vLiBUaGUgcmVxdWVzdCBB UEkgYWxsb3dzIG11bHRpcGxlIGJ1ZmZlcnMgZm9yIHRoZSBzYW1lIHZiMl9xdWV1ZSB0byBiZQo+ IHF1ZXVlZCBmb3IgdGhlIHNhbWUgcmVxdWVzdCAob2J2aW91c2x5IHdoZW4gdGhlIHJlcXVlc3Qg aXMgY29tbWl0dGVkLCB0aGUKPiBidWZmZXJzIGFyZSBxdWV1ZWQgdG8gdGhlIGRyaXZlciBpbiB0 aGUgc2FtZSBvcmRlcikuCj4KPiA+Cj4gPiBJZiB3ZSBpbmRlZWQgYWxsb3cgbXVsdGlwbGUgYnVm ZmVycyBmb3IgdGhlIHNhbWUgcXVldWUgaW4gYSByZXF1ZXN0LAo+ID4gd2Ugc2hvdWxkbid0IHJl c3RyaWN0IHRoaXMgb24gYSBwZXItZHJpdmVyIGJhc2lzLiBJdCdzIGRlZmluaXRlbHkgbm90Cj4g PiBhIGhhcmR3YXJlIGxpbWl0YXRpb24sIHNpbmNlIHRoZSBkcml2ZXIgY291bGQganVzdCBkbyB0 aGUgc2FtZSBhcyBpZiAyCj4gPiByZXF1ZXN0cyB3aXRoIHRoZSBzYW1lIGNvbnRyb2xzIHdlcmUg Z2l2ZW4uCj4KPiBUaGF0J3MgaG93IGl0IG9wZXJhdGVzOiBmb3IgYWxsIGJ1ZmZlcnMgaW4gdGhl IHJlcXVlc3QgdGhlIHNhbWUgY29udHJvbHMKPiBhcHBseS4gQnV0IGRvZXMgdGhpcyBtYWtlIHNl bnNlIGZvciBjb2RlY3M/IElmIHRoZSBjb250cm9sKHMpIHdpdGggdGhlCj4gY29kZWMgbWV0YWRh dGEgYWx3YXlzIGNoYW5nZSBmb3IgZXZlcnkgYnVmZmVyLCB0aGVuIGhhdmluZyBtb3JlIHRoYW4g b25lCj4gYnVmZmVyIGluIHRoZSByZXF1ZXN0IGlzIHNlbnNlbGVzcyBhbmQgdGhlIGRyaXZlciBz aG91bGQgY2hlY2sgZm9yIHRoaXMKPiBpbiB0aGUgdmFsaWRhdGlvbiBzdGVwLgo+Cj4gSWYgaXQg KmRvZXMqIG1ha2Ugc2Vuc2UgaW4gc29tZSBjaXJjdW1zdGFuY2VzIHRvIGhhdmUgdGhlIHNhbWUg bWV0YWRhdGEKPiBmb3IgbXVsdGlwbGUgYnVmZmVycywgdGhlbiBpdCBzaG91bGQgYmUgY2hlY2tl ZCBpZiB0aGUgY2VkcnVzIGRyaXZlcgo+IGhhbmRsZXMgdGhpcyBjb3JyZWN0bHkuCgpKdXN0IEZZ SSwgd2UgbWF5IHdhbnQgdG8gbW92ZSB0aGlzIGRpc2N1c3Npb24gdG8gQWxleCdzIFJGQyB3aXRo CmRvY3VtZW50YXRpb24gb2Ygc3RhdGVsZXNzIGludGVyZmFjZToKaHR0cHM6Ly9wYXRjaHdvcmsu a2VybmVsLm9yZy9wYXRjaC8xMDU4MzIzMy8KCkJlc3QgcmVnYXJkcywKVG9tYXN6Cl9fX19fX19f X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fCmRldmVsIG1haWxpbmcgbGlz dApkZXZlbEBsaW51eGRyaXZlcnByb2plY3Qub3JnCmh0dHA6Ly9kcml2ZXJkZXYubGludXhkcml2 ZXJwcm9qZWN0Lm9yZy9tYWlsbWFuL2xpc3RpbmZvL2RyaXZlcmRldi1kZXZlbAo= From mboxrd@z Thu Jan 1 00:00:00 1970 From: tfiga@chromium.org (Tomasz Figa) Date: Fri, 7 Sep 2018 17:05:01 +0900 Subject: [PATCH v8 4/8] media: platform: Add Cedrus VPU decoder driver In-Reply-To: <7a2167a3-5b61-0937-7f06-8b36fa12e7a0@xs4all.nl> 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> <7a2167a3-5b61-0937-7f06-8b36fa12e7a0@xs4all.nl> Message-ID: To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Thu, Sep 6, 2018 at 4:39 PM Hans Verkuil wrote: > > On 09/06/2018 09:25 AM, Tomasz Figa wrote: > > 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 ? 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. Just FYI, we may want to move this discussion to Alex's RFC with documentation of stateless interface: https://patchwork.kernel.org/patch/10583233/ Best regards, Tomasz