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=-3.8 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS 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 0BC91C4320E for ; Fri, 27 Aug 2021 12:40:45 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id CC859604DC for ; Fri, 27 Aug 2021 12:40:44 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S245119AbhH0Mlc (ORCPT ); Fri, 27 Aug 2021 08:41:32 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:52190 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S245094AbhH0Mlb (ORCPT ); Fri, 27 Aug 2021 08:41:31 -0400 Received: from mail-ed1-x52a.google.com (mail-ed1-x52a.google.com [IPv6:2a00:1450:4864:20::52a]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id A574BC061757 for ; Fri, 27 Aug 2021 05:40:42 -0700 (PDT) Received: by mail-ed1-x52a.google.com with SMTP id q3so9680604edt.5 for ; Fri, 27 Aug 2021 05:40:42 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=vanguardiasur-com-ar.20150623.gappssmtp.com; s=20150623; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=2nPzWi/NZ8hWZLaCBAACfozkVkQ+SBGjdnmjr0f1fk0=; b=RwzREXGmXZLMaMprMGO8kB9mnWGLW8SQErl4tSQVsIK/KqoBVVxl1/Wq2h1t2Uo+9d QOrHKGNvA2U6wQxbmXnW4h71pi0WefyUBpIBfs/uQ+Qx0v6Vjsbg/nZ3x203R98otZ98 0FCT2PP/x2T+1iLNqVS0xbaXj1nuOE82BSLT1mtM6VHfj8yxvIUxFGvd8TNVwDKqE32N DBA/08T68yVXIc8aC0ZfEAAj+2y9Jbu2rrwLvRyvRVdIYHkN5wiOL+X2IX1USapxVuCQ 26wQi505C21Q/tVHAiZdrLbJh7csq6sr/jq89U6UvVGnrFJV+KXPvaccOIVx3qMAk3ST Op+A== 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=2nPzWi/NZ8hWZLaCBAACfozkVkQ+SBGjdnmjr0f1fk0=; b=GLxDHH3Vlo3X3a5BiV5tlPkQ3r+A38kTR5UI3zRc9OLfiSfuXnXWbLLSDZhsUh/s1A KaZ6oiaHmQ7UMbMnw+ZOwvJgjGTbG4/vgpXtBYxu64tJSBAfyYAahpxNLK6wttKoPp+e HFauhDSbCClhXcoimtDpTVf7sIN0lYqMPV+WQRGqEDyrfu543fdIPlAHCYUSAfCdDYR3 Ds6sxnSlBHGvbUr3uzZ4ufdU1DM82DHd0C/SsKYQ0nHBsVDuWVRVavBjHVOM0/6AeoYH crbLzcBESk61HRUQx24gXjSuSlP7YXkQb8JH+RGYMB7VTBNn1oYcEflfmGg40ZYhE0K2 Oeaw== X-Gm-Message-State: AOAM530bknqxdJuorxxDAdeJ7sOIFMphyYL/JbYEyxqDuNXS8dctJhgE WcuBCYrlyqrdOxWgNP8crmwFQtNoPRHH4hHLDQHEkw== X-Google-Smtp-Source: ABdhPJy6G5l8EbEo8HSfL/otUq0S7HfOLOVnL6opEuMcGSY2+ANC9ilYGkfZb35yEZdkaPtAXtv1jZ5IeYfEWWayyO4= X-Received: by 2002:a05:6402:11c9:: with SMTP id j9mr9584544edw.134.1630068041190; Fri, 27 Aug 2021 05:40:41 -0700 (PDT) MIME-Version: 1.0 References: <20210823082949.237716-1-benjamin.gaignard@collabora.com> <02r6ig176o0lqc52nm8rhta7cn5bfn04in@4ax.com> <9d6336fff6f122a9a4510a111387a000c65f797b.camel@ndufresne.ca> In-Reply-To: From: Ezequiel Garcia Date: Fri, 27 Aug 2021 09:40:29 -0300 Message-ID: Subject: Re: [PATCH] media: hevc: fix pictures lists type To: John Cox Cc: Benjamin Gaignard , Nicolas Dufresne , Mauro Carvalho Chehab , Hans Verkuil , linux-media , Linux Kernel Mailing List , Collabora Kernel ML Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, 27 Aug 2021 at 09:36, John Cox wrote: > > >Le 27/08/2021 =C3=A0 12:10, John Cox a =C3=A9crit : > >>> Le 26/08/2021 =C3=A0 18:09, Nicolas Dufresne a =C3=A9crit : > >>>> Le lundi 23 ao=C3=BBt 2021 =C3=A0 12:35 +0100, John Cox a =C3=A9crit= : > >>>>> Hi > >>>>> > >>>>>> Le 23/08/2021 =C3=A0 11:50, John Cox a =C3=A9crit : > >>>>>>>> The lists embedded Picture Order Count values which are s32 so t= heir type > >>>>>>>> most be s32 and not u8. > >>>>>>> I'm not convinced that you can't calculate all of those lists fro= m the > >>>>>>> info already contained in the DPB array so this is probably redun= dant > >>>>>>> info though I grant that having the list pre-calced might make yo= ur life > >>>>>>> easier, and the userland side will have calculated the lists to > >>>>>>> calculate other required things so it isn't much extra work for i= t. > >>>>>> Yes the userland have already compute these lists and the number o= f items > >>>>>> in each of them. > >>>>>> Build them in the kernel would means to also compute the values of= NumPocStCurrBefore, > >>>>>> NumPocStCurrAfter, NumPocLtCurr, NumPocStCurrAfter, NumPocStCurrBe= fore and NumPocLtCurr > >>>>>> and that requires information (NumNegativePics, NumPositivePics...= ) not provided to the kernel. > >>>>>> Since it have to be done in userland anyway, I'm reluctant to modi= fy the API to redo in the kernel. > >>>>> Well, fair enough, I'm not going to argue > >>>>> > >>>>>>> Even if you do need the lists wouldn't it be a better idea to hav= e them > >>>>>>> as indices into the DPB (you can't have a frame in any of those l= ists > >>>>>>> that isn't in the DPB) which already contains POCs then it will s= till > >>>>>>> fit into u8 and be smaller? > >>>>>> Hantro HW works with indexes but I think it is more simple to send= PoC rather than indexes. > >>>>> I'd disagree but as I don't use the info I'm not concerned. Though = I > >>>>> think I should point out that when Hantro converts the POCs to indi= cies > >>>>> it compares the now s32 POC in these lists with the u16 POC in the = DPB > >>>>> so you might need to fix that too; by std (8.3.1) no POC diff can b= e > >>>>> outside s16 so you can mask & compare or use u16 POCs in the lists = or > >>>>> s32 in the DPB. > >>>> Fun fact, my interpretation with the API when I drafted GStreamer su= pport was > >>>> that it was DPB indexes: > >>>> > >>>> https://gitlab.freedesktop.org/ndufresne/gst-plugins-bad/-/blob/hevc= _wip/sys/v4l2codecs/gstv4l2codech265dec.c#L850 > >>>> > >>>> It felt quite natural to be, since this is also how we pass referenc= es for l0/l1 > >>>> (unused by hantro I guess). > >>>> > >>>> Looking at old rkvdec code as a refresher: > >>>> > >>>> for (j =3D 0; j < run->num_slices; j++) { > >>>> sl_params =3D &run->slices_params[j]; > >>>> dpb =3D sl_params->dpb; > >>>> > >>>> hw_ps =3D &priv_tbl->rps[j]; > >>>> memset(hw_ps, 0, sizeof(*hw_ps)); > >>>> > >>>> for (i =3D 0; i <=3D sl_params->num_ref_idx_l0_act= ive_minus1; i++) { > >>>> WRITE_RPS(!!(dpb[sl_params->ref_idx_l0[i]]= .rps =3D=3D V4L2_HEVC_DPB_ENTRY_RPS_LT_CURR), > >>>> REF_PIC_LONG_TERM_L0(i)); > >>>> WRITE_RPS(sl_params->ref_idx_l0[i], REF_PI= C_IDX_L0(i)); > >>>> } > >>>> > >>>> for (i =3D 0; i <=3D sl_params->num_ref_idx_l1_act= ive_minus1; i++) { > >>>> WRITE_RPS(!!(dpb[sl_params->ref_idx_l1[i]]= .rps =3D=3D V4L2_HEVC_DPB_ENTRY_RPS_LT_CURR), > >>>> REF_PIC_LONG_TERM_L1(i)); > >>>> WRITE_RPS(sl_params->ref_idx_l1[i], REF_PI= C_IDX_L1(i)); > >>>> } > >>>> > >>>> > >>>> This is code is clearly unsafe, but now I remember that dpb_entry ha= s a flag > >>>> "rps". So we know from the DPB in which of the list the reference li= ves, if any. > >>>> In the case of RKVDEC the HW only cares to know if this is long term= or not. > >>>> > >>>> So without looking at the spec, is that dpb represention enough to r= econstruct > >>>> these array ? If we pass these array, shall we keep the rps flag ? I= think a > >>>> little step back and cleanup will be needed. I doubt there is a sing= le answer, > >>>> perhaps list what others do (VA, DXVA, NVDEC, Khronos, etc) and we c= an > >>>> collectively decide were we want V4L2 to sit ? > >>> I have done some tests with Hantro driver and look at the spec, the o= rder of the PoC > >>> in the reference lists matters. You can deducted the order for DPB rp= s flags. > >>> I would suggest to remove rps flags to avoid information duplication. > >> I want the DPB rps member for long term reference marking. I don't ca= re > >> about before / after, but LTR can't be deduced from PoC and if you are > >> going to keep the member you might as well keep before / after. > > > >Ok so keep like it is. > >In this case my patch is enough, right ? > The problem with the patch is that it breaks existing userspace. Currently, there's no upstreamed userspace so this is not a huge deal. However, it's definitely not a good practice. Even if these are staging controls, I think a proper action item is to start discussing what's missing on the HEVC interface as a whole, so it can be moved to stable. Otherwise, it almost feels like bikeshading. Thanks, Ezequiel