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=-2.2 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_2 autolearn=no 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 1440AC3F2D8 for ; Mon, 2 Mar 2020 14:30:46 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id E34ED2064A for ; Mon, 2 Mar 2020 14:30:45 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727305AbgCBOao convert rfc822-to-8bit (ORCPT ); Mon, 2 Mar 2020 09:30:44 -0500 Received: from bhuna.collabora.co.uk ([46.235.227.227]:46468 "EHLO bhuna.collabora.co.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727104AbgCBOao (ORCPT ); Mon, 2 Mar 2020 09:30:44 -0500 Received: from localhost (unknown [IPv6:2a01:e0a:2c:6930:5cf4:84a1:2763:fe0d]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) (Authenticated sender: bbrezillon) by bhuna.collabora.co.uk (Postfix) with ESMTPSA id C7561294B57; Mon, 2 Mar 2020 14:30:41 +0000 (GMT) Date: Mon, 2 Mar 2020 15:30:39 +0100 From: Boris Brezillon To: Mauro Carvalho Chehab Cc: Ezequiel Garcia , linux-media@vger.kernel.org, devicetree@vger.kernel.org, linux-rockchip@lists.infradead.org, linux-kernel@vger.kernel.org, Laurent Pinchart , Rob Herring , Tomasz Figa , Nicolas Dufresne , kernel@collabora.com, Paul Kocialkowski , Jonas Karlman , Heiko Stuebner , Sakari Ailus , Hans Verkuil Subject: Re: [PATCH v6 5/6] media: rkvdec: Add the rkvdec driver Message-ID: <20200302153039.0c4ff54f@collabora.com> In-Reply-To: <20200302145746.3e94c1d1@coco.lan> References: <20200220163016.21708-1-ezequiel@collabora.com> <20200220163016.21708-6-ezequiel@collabora.com> <20200302145746.3e94c1d1@coco.lan> Organization: Collabora X-Mailer: Claws Mail 3.17.4 (GTK+ 2.24.32; x86_64-redhat-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, 2 Mar 2020 14:57:46 +0100 Mauro Carvalho Chehab wrote: > > +#define M_N(ctxidx, idc0_m, idc0_n, idc1_m, idc1_n, \ > > + idc2_m, idc2_n, intra_m, intra_n) \ > > + [0][(ctxidx)] = {idc0_m, idc0_n}, \ > > + [1][(ctxidx)] = {idc1_m, idc1_n}, \ > > + [2][(ctxidx)] = {idc2_m, idc2_n}, \ > > + [3][(ctxidx)] = {intra_m, intra_n} > > Hmm... I can't even imagine what a macro named "M_N" would do. > Please use a better name for it. Well, the meaning of those fields is explained in the spec, and the name itself has been chosen so it's short enough to not have lines exceeding 80 chars while still keeping the number of lines used for the cabac_table[] definition acceptable. But, I'm open to any other suggestion. > > - > > With regards to the macro itself, at least for my eyes, it looked bad, > from long-term maintenance PoV, to have a first argument (ctxidx) whose > value is just a monotonic linearly-incremented counter. It's not, we have holes in the middle, hence the explicit indexing. I also tried to have something as close as possible to the spec, so people can easily see where it comes from. > > I mean, the way it is, it sounds risky, as one might miss a number > and one entire line of the array would be filled with zeros. That's exactly why I used explicit indexing: I want specific portions of the table to be 0-filled :-). > > > + > > +/* > > + * Constant CABAC table. > > + * Built from the tables described in section '9.3.1.1 Initialisation process > > + * for context variables' of the H264 spec. > > + */ > > +static const s8 rkvdec_h264_cabac_table[4][464][2] = { > > + /* Table 9-12 – Values of variables m and n for ctxIdx from 0 to 10 */ > > + M_N(0, 20, -15, 20, -15, 20, -15, 20, -15), > > So, (maybe except if the ctxidx value has some real meaning), > perhaps you could, instead, switch the array order at the tables, > and get rid of ctxidx parameter for good, so the above code would > be like: I can't switch the array order since the HW expects things to be organized this way (that table is directly copied to a memory region that's passed to the HW). > > #define INIT_MN_PAIRS(idc0_m, idc0_n, idc1_m, idc1_n, \ > idc2_m, idc2_n, intra_m, intra_n) \ > { \ > [0] = {idc0_m, idc0_n}, \ > [1] = {idc1_m, idc1_n}, \ > [2] = {idc2_m, idc2_n}, \ > [3] = {intra_m, intra_n} \ > }, > > static const s8 rkvdec_h264_cabac_table[464][4][2] = { > /* Table 9-12 – Values of variables m and n for ctxIdx from 0 to 10 */ > INIT_MN_PAIRS(20, -15, 20, -15, 20, -15, 20, -15), > ...