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 Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 64BFBC433F5 for ; Tue, 15 Feb 2022 16:12:52 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:MIME-Version:References:In-Reply-To: Message-ID:Date:Subject:Cc:To:From:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=2hSbc0TkiEd00zIwK6vWFwsy9w1d1goH5nlsDbsHdks=; b=PsUpUT1LznPPnz IRMO+NcIF5dKbO2ECc3AgvvvGoxWxLwHZv0cpRFPxTf8xhwTip00cEyz4Tv5PAJ/aWkerXLgTZ034 TEtsu1HTntBfqMoiZJW/pW8AGn0CIUbj64Edsp2K2WMGIRpQEfcvqyUyIWG6e0iaonUhYUfzwexI3 eLC+AbqVXKkqeoygTXNKIKRg42IjYpD7l2/+gthUZ6GqNHBoKMKKk5YEU8TS/slYhSXhPZyHOrkMm M3sQmlRPXMo5HxK2nxSulHk+6S4f/rRlqWo7++bSjXlkUo+cSajI0ROl0/Owl2niJVCYzLgXSyfC+ AgEEZCyqj7Y9HMsRg87w==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1nK0QI-003b1t-2v; Tue, 15 Feb 2022 16:11:26 +0000 Received: from mail-wm1-x32d.google.com ([2a00:1450:4864:20::32d]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1nK0Q8-003azr-Iw for linux-arm-kernel@lists.infradead.org; Tue, 15 Feb 2022 16:11:18 +0000 Received: by mail-wm1-x32d.google.com with SMTP id l67-20020a1c2546000000b00353951c3f62so1720991wml.5 for ; Tue, 15 Feb 2022 08:11:15 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=from:to:cc:subject:date:message-id:in-reply-to:references :mime-version:content-transfer-encoding; bh=akgdp0nUv5uW3+1NF74dVrBjd5UKlSd+kdDxEGnURlI=; b=OW5eC5e0Hw7A8e+obDqk/mPLrNgsKu4gUvOxpCk1fQ31ZU+HqUQASIID/sK08luaon uTpX1ZKa3WoPtopByrinsY/9Vi8TcXr8l+tBThcHxLuOMOXQgvro9frSZncupt7ymjj1 xi+CfEY3VCyC0bVR4k3Qc9Z9vHlPiNbGfmt7DdqlO15Jp+ZtrgQnvn9KdjxjWzy9taIY Y8jziBkOyx36BUc5vEOCtkch9jc91y5uGmQHxHEHEXLhGdxdmre4DCEyEPoiICk4gsxN KkvS+0SX07teMVNjnAPsXb2BnpFxaAe81CZ4pR4Y0p1nvaKiJ2uvUi6oEa/4IhTocnLg ioDw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references:mime-version:content-transfer-encoding; bh=akgdp0nUv5uW3+1NF74dVrBjd5UKlSd+kdDxEGnURlI=; b=6W/iF1ktULXKOtCzKMJBeVsRymyvZrhsESbau9CUlSWZPdwVMGPQDWO7mhtHNSJm1R EOrWZy6/yK8mXFrWTDDOUpI7JWM7b+pSD63wBC669Pwb5DEqlr5co6I8XILVFfZfnsFg 3CBtSXXMF67QduNSM5egUeuZm5eALytEU36EJ7PNiHsaFfz3ePRDq8nyUhXACHwkSs/j HNlC4CVcceqJwNOYH1bINSjwYoFv3ELPiMOnrmi3Dwv2vsDDyGPJ1YxQGcf+nKhDVqjv 5kGDe/QwgVx+UIJU1w82WJv38hTNFo4vvGRpziIHcB8771iudNcw/8YyntIcXrPvliJ2 VDow== X-Gm-Message-State: AOAM530wFCk9+Z7ARkNW1zf4fH25a9jyvFrJt/fy39Kdjqsnov/53qhA uyOqca46FkGKbUUhK1NiUgc= X-Google-Smtp-Source: ABdhPJz+cr/FbFbp/YEWBzYvFGhBIyLbtURqpGlikSXHMAgdXnC/xWHzfx1gavbudlcdtP2aXAFQBQ== X-Received: by 2002:a05:600c:601a:: with SMTP id az26mr3769890wmb.2.1644941474494; Tue, 15 Feb 2022 08:11:14 -0800 (PST) Received: from kista.localnet (cpe-86-58-32-107.static.triera.net. [86.58.32.107]) by smtp.gmail.com with ESMTPSA id a9sm17741358wrg.53.2022.02.15.08.11.12 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 15 Feb 2022 08:11:13 -0800 (PST) From: Jernej =?utf-8?B?xaBrcmFiZWM=?= To: Nicolas Dufresne , John Cox Cc: Benjamin Gaignard , mchehab@kernel.org, ezequiel@vanguardiasur.com.ar, p.zabel@pengutronix.de, gregkh@linuxfoundation.org, mripard@kernel.org, paul.kocialkowski@bootlin.com, wens@csie.org, hverkuil-cisco@xs4all.nl, jonas@kwiboo.se, linux-media@vger.kernel.org, linux-kernel@vger.kernel.org, linux-staging@lists.linux.dev, linux-arm-kernel@lists.infradead.org, linux-sunxi@lists.linux.dev, kernel@collabora.com, knaerzche@gmail.com Subject: Re: Re: [RFC v2 6/8] media: uapi: Remove bit_size field from v4l2_ctrl_hevc_slice_params Date: Tue, 15 Feb 2022 17:11:12 +0100 Message-ID: <2071229.OBFZWjSADL@kista> In-Reply-To: <48in0h5qq4ea87gcnmtkpsfqdk1r4tipid@4ax.com> References: <20220215110103.241297-1-benjamin.gaignard@collabora.com> <9a78eb88f8690d43d34d8140420dae3f5f043637.camel@ndufresne.ca> <48in0h5qq4ea87gcnmtkpsfqdk1r4tipid@4ax.com> MIME-Version: 1.0 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220215_081116_672723_A6F48A4A X-CRM114-Status: GOOD ( 49.06 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Dne torek, 15. februar 2022 ob 17:00:33 CET je John Cox napisal(a): > On Tue, 15 Feb 2022 10:28:55 -0500, you wrote: > = > >Le mardi 15 f=E9vrier 2022 =E0 14:50 +0000, John Cox a =E9crit : > >> On Tue, 15 Feb 2022 15:35:12 +0100, you wrote: > >> = > >> > = > >> > Le 15/02/2022 =E0 15:17, John Cox a =E9crit : > >> > > Hi > >> > > = > >> > > > The bit size of the slice could be deduced from the buffer paylo= ad > >> > > > so remove bit_size field to avoid duplicated the information. > >> > > I think this is a bad idea. In the future we are (I hope) going to = want > >> > > to have an array (variable) of slice headers all referring to the = same > >> > > bit buffer. When we do that we will need this field. > >> > = > >> > I wonder if that could be considering like another decode mode and so > >> > use an other control ? > >> = > >> I, personally, would be in favour of making the slice header control a > >> variable array just as it is. If userland can't cope with multiple > >> entries then just send them one at a time and the code looks exactly > >> like it does at the moment and if the driver can't then set max array > >> entries to 1. > >> = > >> Having implemented this in rpi port of ffmpeg and the RPi V4L2 driver I > >> can say with experience that the code and effort overhead is very low. > >> = > >> Either way having a multiple slice header control in the UAPI is > >> important for efficiency. > > > >Just to clarify the idea, we would have a single slice controls, always = dynamic: > > > >1. For sliced based decoder > > > >The dynamic array slice control is implemented by the driver and its siz= e = must > >be 1. > = > Yes > = > >2. For frame based decoder that don't care for slices > > > >The dynamic array slice controls is not implement. Userland detects that= at > >runtime, similar to the VP9 compressed headers. > = > If the driver parses all the slice header then that seems plausible > = > >3. For frame based decoders that needs slices (or driver that supports = offset > >and can gain performance with such mode) > > > >The dynamic array slice controls is implemented, and should contain all = the > >slices found in the OUTPUT buffer. > > > >So the reason for this bit_size (not sure why its bits though, perhaps = someone > >can educate me ?) > = > RPi doesn't need bits and would be happy with bytes however > slice_segment data isn't byte aligned at the end so its possible that > there might be decoders out there that want an accurate length for that. There are two fields, please don't mix them up: __u32 bit_size; __u32 data_bit_offset; (changed to data_byte_offset in this series) data_bit_offset/data_byte_offset is useful, while bit_size is IMO not. If y= ou = have multiple slices in array, you only need to know start of the slice dat= a = and that offset is always offset from start of the buffer (absolute, it's n= ot = relative to previous slice data). Best regards, Jernej > = > > Would be to let the driver offset inside the the single > >OUTPUT/bitstream buffer in case this is not automatically found by the = driver > >(or that no start-code is needed). Is that last bit correct ? If so, sho= uld = we > >change it to an offset rather then a size ? Shall we allow using offeset= s = inside > >larger buffer (e.g. to avoid some memory copies) for the Sliced Base cas= es ? > = > I use (in the current structure) data_bit_offset to find the start of > each slice's slice_segment_data within the OUTPUT buffer and bit_size to > find the end. RPi doesn't / can't parse the slice_header and so wants > all of that. Decoders that do parse the header might plausably want > header offsets too and it would facilitate zero copy of the bit buffer. > = > = > >> Regards > >> = > >> John Cox > >> = > >> > > > Signed-off-by: Benjamin Gaignard > >> > > > --- > >> > > > .../userspace-api/media/v4l/ext-ctrls-codec.rst | 3 --- > >> > > > drivers/staging/media/sunxi/cedrus/cedrus_h265.c | 11 +++ +------- > >> > > > include/uapi/linux/v4l2-controls.h | 3 +-- > >> > > > 3 files changed, 5 insertions(+), 12 deletions(-) > >> > > > = > >> > > > diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls- codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst > >> > > > index 3296ac3b9fca..c3ae97657fa7 100644 > >> > > > --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst > >> > > > +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst > >> > > > @@ -2965,9 +2965,6 @@ enum v4l2_mpeg_video_hevc_size_of_length_f= ield = - > >> > > > :stub-columns: 0 > >> > > > :widths: 1 1 2 > >> > > > = > >> > > > - * - __u32 > >> > > > - - ``bit_size`` > >> > > > - - Size (in bits) of the current slice data. > >> > > > * - __u32 > >> > > > - ``data_bit_offset`` > >> > > > - Offset (in bits) to the video data in the current slice = data. > >> > > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c b/ drivers/staging/media/sunxi/cedrus/cedrus_h265.c > >> > > > index 8ab2d9c6f048..db8c7475eeb8 100644 > >> > > > --- a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c > >> > > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c > >> > > > @@ -312,8 +312,8 @@ static void cedrus_h265_setup(struct cedrus_= ctx = *ctx, > >> > > > const struct v4l2_hevc_pred_weight_table *pred_weight_table; > >> > > > unsigned int width_in_ctb_luma, ctb_size_luma; > >> > > > unsigned int log2_max_luma_coding_block_size; > >> > > > + size_t slice_bytes; > >> > > > dma_addr_t src_buf_addr; > >> > > > - dma_addr_t src_buf_end_addr; > >> > > > u32 chroma_log2_weight_denom; > >> > > > u32 output_pic_list_index; > >> > > > u32 pic_order_cnt[2]; > >> > > > @@ -370,8 +370,8 @@ static void cedrus_h265_setup(struct cedrus_= ctx = *ctx, > >> > > > = > >> > > > cedrus_write(dev, VE_DEC_H265_BITS_OFFSET, 0); > >> > > > = > >> > > > - reg =3D slice_params->bit_size; > >> > > > - cedrus_write(dev, VE_DEC_H265_BITS_LEN, reg); > >> > > > + slice_bytes =3D vb2_get_plane_payload(&run->src->vb2_buf, 0); > >> > > > + cedrus_write(dev, VE_DEC_H265_BITS_LEN, slice_bytes); > >> > > I think one of these must be wrong. bit_size is in bits, > >> > > vb2_get_plane_payload is in bytes? > >> > = > >> > You are right it should be vb2_get_plane_payload() * 8 to get the si= ze = in bits. > >> > = > >> > I will change that in v3. > >> > = > >> > > = > >> > > Regards > >> > > = > >> > > John Cox > >> > > = > >> > > > /* Source beginning and end addresses. */ > >> > > > = > >> > > > @@ -384,10 +384,7 @@ static void cedrus_h265_setup(struct = cedrus_ctx *ctx, > >> > > > = > >> > > > cedrus_write(dev, VE_DEC_H265_BITS_ADDR, reg); > >> > > > = > >> > > > - src_buf_end_addr =3D src_buf_addr + > >> > > > - DIV_ROUND_UP(slice_params->bit_size, = 8); > >> > > > - > >> > > > - reg =3D VE_DEC_H265_BITS_END_ADDR_BASE(src_buf_end_addr); > >> > > > + reg =3D VE_DEC_H265_BITS_END_ADDR_BASE(src_buf_addr + slice_by= tes); > >> > > > cedrus_write(dev, VE_DEC_H265_BITS_END_ADDR, reg); > >> > > > = > >> > > > /* Coding tree block address */ > >> > > > diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/ linux/v4l2-controls.h > >> > > > index b1a3dc05f02f..27f5d272dc43 100644 > >> > > > --- a/include/uapi/linux/v4l2-controls.h > >> > > > +++ b/include/uapi/linux/v4l2-controls.h > >> > > > @@ -2457,7 +2457,6 @@ struct v4l2_hevc_pred_weight_table { > >> > > > #define V4L2_HEVC_SLICE_PARAMS_FLAG_DEPENDENT_SLICE_SEGMENT = (1ULL << 9) > >> > > > = > >> > > > struct v4l2_ctrl_hevc_slice_params { > >> > > > - __u32 bit_size; > >> > > > __u32 data_bit_offset; > >> > > > = > >> > > > /* ISO/IEC 23008-2, ITU-T Rec. H.265: NAL unit header */ > >> > > > @@ -2484,7 +2483,7 @@ struct v4l2_ctrl_hevc_slice_params { > >> > > > /* ISO/IEC 23008-2, ITU-T Rec. H.265: Picture timing SEI messag= e = */ > >> > > > __u8 pic_struct; > >> > > > = > >> > > > - __u8 reserved; > >> > > > + __u8 reserved[5]; > >> > > > = > >> > > > /* ISO/IEC 23008-2, ITU-T Rec. H.265: General slice segment = header */ > >> > > > __u32 slice_segment_addr; > = _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel