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=-4.0 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_PASS,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 94A95C43441 for ; Wed, 28 Nov 2018 08:41:55 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 587B520832 for ; Wed, 28 Nov 2018 08:41:55 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 587B520832 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=xs4all.nl 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 S1727970AbeK1Tmn (ORCPT ); Wed, 28 Nov 2018 14:42:43 -0500 Received: from lb2-smtp-cloud8.xs4all.net ([194.109.24.25]:46539 "EHLO lb2-smtp-cloud8.xs4all.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727668AbeK1Tmn (ORCPT ); Wed, 28 Nov 2018 14:42:43 -0500 Received: from [192.168.2.10] ([212.251.195.8]) by smtp-cloud8.xs4all.net with ESMTPA id RvPlgzK43EPjORvPpgfrsS; Wed, 28 Nov 2018 09:41:51 +0100 Subject: Re: [PATCH v3] media: venus: amend buffer size for bitstream plane To: Alexandre Courbot Cc: Tomasz Figa , Stanimir Varbanov , mgottam@codeaurora.org, Mauro Carvalho Chehab , Linux Media Mailing List , LKML , linux-arm-msm@vger.kernel.org, vgarodia@codeaurora.org References: <1543227173-2160-1-git-send-email-mgottam@codeaurora.org> <57b28a7f-8c5c-22d2-2f89-e6d6ebdcb8a2@xs4all.nl> <2a8bbdf7-cec6-4bdf-5833-93d5014ddf89@xs4all.nl> From: Hans Verkuil Message-ID: Date: Wed, 28 Nov 2018 09:41:45 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-CMAE-Envelope: MS4wfF+OGAF3zB4JRasM35sQB57geK7DdEZGnt/XqLkkFpgYjo//imKdwxL956SAMn2HPh3DsamwQMrSxT6jmhsn8Vh6+d+kLY4HHt1aNh0K/t0F0npb1rDy jTf1ydxXmqa0cWrawyr7ngbK/GMlc1qW6LKL3EzUqHZTovztAHVAXA0K2HMvx9aMja/5ml6T90BrJ1SAh0UbQOqMmJdRRjJpxQLHY8B+pghx1SAx/lwdJbVL y0N2SWp7KFR95XV/AGJRXHmPV+L7I9y/Z7+kakTSRaAIs2+0gjlcyWzZjUIVg+1XxLqXbrvtRoJfK9+6rhCsT8YxvHBkxo/qcsa0GIr8oL5q5tu/4OWd+maU lxDo0khOTMm9092TZkCXgTBMstv+nJvsiGJu1aNotARDeSsV8GiOiU95zBzXda3A2ZQbp5L68toGFa3W6ya6eBIwU3iQDtIiZYQK03oZG4yHazLdb4s= Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 11/27/2018 09:16 AM, Alexandre Courbot wrote: > Hi Hans, > > On Tue, Nov 27, 2018 at 1:41 AM Hans Verkuil wrote: >> >> On 11/26/2018 05:07 PM, Tomasz Figa wrote: >>> On Tue, Nov 27, 2018 at 1:00 AM Hans Verkuil wrote: >>>> >>>> On 11/26/2018 04:44 PM, Tomasz Figa wrote: >>>>> Hi Hans, >>>>> >>>>> On Tue, Nov 27, 2018 at 12:24 AM Hans Verkuil wrote: >>>>>> >>>>>> On 11/26/2018 03:57 PM, Stanimir Varbanov wrote: >>>>>>> Hi Hans, >>>>>>> >>>>>>> On 11/26/18 3:37 PM, Hans Verkuil wrote: >>>>>>>> On 11/26/2018 11:12 AM, Malathi Gottam wrote: >>>>>>>>> Accept the buffer size requested by client and compare it >>>>>>>>> against driver calculated size and set the maximum to >>>>>>>>> bitstream plane. >>>>>>>>> >>>>>>>>> Signed-off-by: Malathi Gottam >>>>>>>> >>>>>>>> Sorry, this isn't allowed. It is the driver that sets the sizeimage value, >>>>>>>> never the other way around. >>>>>>> >>>>>>> I think for decoders (OUTPUT queue) and encoders (CAPTURE queue) we >>>>>>> allowed userspace to set sizeimage for buffers. See [1] Initialization >>>>>>> paragraph point 2: >>>>>>> >>>>>>> ``sizeimage`` >>>>>>> desired size of ``CAPTURE`` buffers; the encoder may adjust it to >>>>>>> match hardware requirements >>>>>>> >>>>>>> Similar patch we be needed for decoder as well. >>>>>> >>>>>> I may have missed that change since it wasn't present in v1 of the stateful >>>>>> encoder spec. >>>>> >>>>> It's been there from the very beginning, even before I started working >>>>> on it. Actually, even the early slides from Kamil mention the >>>>> application setting the buffer size for compressed streams: >>>>> https://events.static.linuxfound.org/images/stories/pdf/lceu2012_debski.pdf >>>>> >>>>>> >>>>>> Tomasz, what was the reason for this change? I vaguely remember some thread >>>>>> about this, but I forgot the details. Since this would be a departure of >>>>>> the current API this should be explained in more detail. >>>>> >>>>> The kernel is not the place to encode assumptions about optimal >>>>> bitstream chunk sizes. It depends on the use case and the application >>>>> should be able decide. It may for example want to use smaller buffers, >>>>> optimizing for the well compressible video streams and just reallocate >>>>> if bigger chunks are needed. >>>>> >>>>>> >>>>>> I don't really see the point of requiring userspace to fill this in. For >>>>>> stateful codecs it can just return some reasonable size. Possibly calculated >>>>>> using the provided width/height values or (if those are 0) some default value. >>>>> >>>>> How do we decide what's "reasonable"? Would it be reasonable for all >>>>> applications? >>>> >>>> In theory it should be the minimum size that the hardware supports. But it is >>>> silly to i.e. provide the size of one PAGE as the minimum. In practice you >>>> probably want to set sizeimage to something larger than that. Depending on >>>> the typical compression ratio perhaps 5 or 10% of what a raw YUV 4:2:0 frame >>>> would be. >>>> >>>>> >>>>>> >>>>>> Ditto for decoders. >>>>>> >>>>>> Stanimir, I certainly cannot merge this until this has been fully nailed down >>>>>> as it would be a departure from the current API. >>>>> >>>>> It would not be a departure, because I can see existing stateful >>>>> drivers behaving like that: >>>>> https://elixir.bootlin.com/linux/v4.20-rc4/source/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c#L1444 >>>>> https://elixir.bootlin.com/linux/v4.20-rc4/source/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c#L469 >>>> >>>> Yes, and that's out of spec. Clearly v4l2-compliance doesn't test for this. >>>> It should have been caught at least for the mtk driver. >>>> >>> >>> Perhaps we should make it a part of the spec then? >>> >>> Actually I'm not really sure if we can say that this is out of spec >>> There has been no clear spec for the stateful codecs for many years, >>> with drivers doing wildly whatever they like and applications ending >>> up relying on those quirks. >> >> The VIDIOC_S_FMT spec for v4l2_pix_format is quite clear that it is the >> driver that sets this value. The spec for v4l2_plane_pix_format is >> unfortunately not so clear. >> >>> My spec actually attempts to incorporate what was decided on the >>> earlier summits, including what's in Kamil's slides, the drivers are >>> already doing and existing applications rely on. The sizeimage >>> handling is just a part of it. >>> >>>>> >>>>> Also, Chromium has been setting the size on its own for long time >>>>> using its own heuristics. >>>>> >>>>>> >>>>>> And looking at the venus patch I do not see how it helps userspace. >>>>>> >>>>>> Regards, >>>>>> >>>>>> Hans >>>>>> >>>>>>> >>>>>>>> >>>>>>>> If you need to allocate larger buffers, then use VIDIOC_CREATE_BUFS instead >>>>>>>> of VIDIOC_REQBUFS. >>>>> >>>>> CREATE_BUFS wouldn't work, because one needs to use TRY_FMT to obtain >>>>> a format for it and the format returned by it would have the sizeimage >>>>> as hardcoded in the driver... >>>> >>>> ??? >>>> >>>> Userspace can change the sizeimage to whatever it wants before calling >>>> CREATE_BUFS as long as it is >= the sizeimage of the current format. >>>> >>>> If we want to allow smaller sizes, then I think that would not be >>>> unreasonable for stateful codecs. I actually think that drivers can >>>> already do this in queue_setup(), but the spec would have to be updated >>>> a bit. >>> >>> Existing applications rely on REQBUFS honoring the size they set in >>> sizeimage, though... >> >> REQBUFS, yes. But not CREATE_BUFS. Which is why that ioctl was added in the >> first place. >> >> However (and now I remember the real problem with CREATE_BUFS) the spec for >> CREATE_BUFS says that it will not change the provided sizeimage value. So >> any adjustments required due to specific alignment requirements won't be >> applied, all CREATE_BUFS can do is to reject it. >> >> So what this boils down to is a change to the spec: >> >> For compressed formats (and only those!) userspace can set sizeimage to a >> proposed value. The driver may either ignore it and just set its own value, >> or modify it to satisfy HW requirements. The returned value will be used >> by REQBUFS when it allocates buffers. >> >> I think this is reasonable, provided the spec is updated accordingly. >> >> As far as I can tell this shouldn't cause any backwards compatibility >> problems, and it should be easy to test in v4l2-compliance. > > Do you mean that this patch is acceptable provided the stateful codec > specification is updated accordingly? Yes. Although I would update the spec in a separate patch. It is not really part of the codec spec as such, more a prerequisite. Regards, Hans > For our (Chromium) needs this seems to do the job, so: > > Tested-by: Alexandre Courbot > > Although I would like to also have the equivalent for the decoder's > OUTPUT queue, either as a v4 or a follow-up patch. >