Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20221219155616.848690-1-benjamin.gaignard@collabora.com>
 <CAAEAJfBP_D65kjHbwYP+LWfWKfzFtHtWo+3bDcbdO8tPtBurUA@mail.gmail.com>
 <20221219215431.GB26315@pengutronix.de> <939ce9a0f7f73c0b9f0cf5590f2d8b02e0825fa5.camel@ndufresne.ca>
 <20221221220127.GE26315@pengutronix.de>
In-Reply-To: <20221221220127.GE26315@pengutronix.de>
From:   Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
Date:   Thu, 22 Dec 2022 10:24:14 -0300
Message-ID: <CAAEAJfAMogQzU_etD2m8vOrkQJxd3jKuFtVU9mewzCsDP-GEQQ@mail.gmail.com>
Subject: Re: [PATCH v1 0/9] AV1 stateless decoder for RK3588
To:     Michael Grzeschik <mgr@pengutronix.de>
Cc:     Nicolas Dufresne <nicolas@ndufresne.ca>,
        Benjamin Gaignard <benjamin.gaignard@collabora.com>,
        Philipp Zabel <p.zabel@pengutronix.de>,
        Mauro Carvalho Chehab <mchehab@kernel.org>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Heiko Stuebner <heiko@sntech.de>,
        Daniel Almeida <daniel.almeida@collabora.com>,
        nicolas.dufresne@collabora.co.uk,
        linux-media <linux-media@vger.kernel.org>,
        "open list:ARM/Rockchip SoC..." <linux-rockchip@lists.infradead.org>,
        devicetree <devicetree@vger.kernel.org>,
        linux-arm-kernel <linux-arm-kernel@lists.infradead.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Collabora Kernel ML <kernel@collabora.com>,
        Andrzej Pietrasiewicz <andrzej.p@collabora.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hi Nicolas, Michael,

(+Andrzej)

El mi=C3=A9, 21 dic. 2022 19:01, Michael Grzeschik <mgr@pengutronix.de> esc=
ribi=C3=B3:
>
> On Tue, Dec 20, 2022 at 12:00:01PM -0500, Nicolas Dufresne wrote:
> >Le lundi 19 d=C3=A9cembre 2022 =C3=A0 22:54 +0100, Michael Grzeschik a =
=C3=A9crit :
> >> Hi Benjamin,
> >> Hi Ezequiel,
> >>
> >> On Mon, Dec 19, 2022 at 06:07:38PM -0300, Ezequiel Garcia wrote:
> >> > On Mon, Dec 19, 2022 at 12:56 PM Benjamin Gaignard
> >> > <benjamin.gaignard@collabora.com> wrote:
> >> > >
> >> > > This series implement AV1 stateless decoder for RK3588 SoC.
> >> > > The harware support 8 and 10 bits bitstreams up to 7680x4320.
> >> > > AV1 feature like film grain or scaling are done by the postprocess=
or.
> >> > > The driver can produce NV12_4L4 and NV12 pixel formats.
> >> > > A native 10bits NV12_4L4 format is possible but need more investig=
ation
> >> > > to be completly documented and enabled.
> >> > >
> >> > > It is based on Daniel's "[RFC,v3] media: Add AV1 uAPI" [1] patches=
 and
> >> > > Sebastian's device-tree patches for RK3588.
> >> > >
> >> >
> >> > I thought the AV1 decoder in RK3588 was really a separate hardware
> >> > from the Hantro G1/G2.
> >> >
> >> > Shouldn't this need a new driver for this new hardware?
> >>
> >> Just jumping into this discussion as I am currently working on the rkv=
enc driver.
> >>
> >> In my case I am extending the rkvdec driver to become more generic for
> >> other rockchip specific enc/decoders.
> >>
> >> My first change looks like this:
> >> ---
> >>  drivers/staging/media/rkvdec/Makefile              |   4 +-
> >>  drivers/staging/media/rkvdec/rkvdec-h264.c         | 100 ++++-----
> >>  drivers/staging/media/rkvdec/rkvdec-vp9.c          | 142 ++++++------=
-
> >>  drivers/staging/media/rkvdec/{rkvdec.c =3D> rkvpu.c} | 510 ++++++++++=
+++++++++++++-----------------------
> >>  drivers/staging/media/rkvdec/{rkvdec.h =3D> rkvpu.h} |  66 +++---
> >> ---
> >>
> >> While working on other parts of the encoder I found many places in the
> >> rkvdec driver (e.g. v4l2 and vb2 callbacks) that looked familiar to th=
e hantro
> >> functions but where limited to the decoder case.
> >>
> >> I think there are two options for the av1 codec.
> >>
> >> 1) If the vpu981 is a driver that has nothing to do with verisilicon b=
ut
> >> works with this driver framework, then we should integrate vepu981 int=
o it
> >> but consider rename the verisilicon unrelated parts to something gener=
ic.
> >
> >I've raised in my review the the naming is sub-optimal. This is an unmod=
ified
> >VC9000D AV1 decoder. No other codecs have been included in the package, =
even
> >though VC9000D cores can support more.
> >
> >Stating this driver have no place here seems a bit strange to me, but wi=
th
> >proper arguments, maybe we can make a case and start a VC9000D dedicated=
 driver
> >(that will be a lot of copy paste, VC9000D post processor notably is ide=
ntical
> >to VC8000 post processor, but one could argue we should make a VCX000 dr=
iver ?
> >
> >>
> >> 2) Move the vepu981 av1 driver into the rkvdec instead.
> >
> >That make no sense, its not a Rockchip HW design, and will likely start
> >appearing on non-RK SoC in the future.
>
> Sure. I did not know that it actually is an VC9000.
>
> >> If 1) is the way to go, we can even think of moving the staging code p=
arts from
> >> rkvdec to the verisilicon code. Likewise to the vepu981-av1.
> >
> >Again, I think using RK naming is unfortunate choice. This AV1 decoder i=
s just
> >like the G1/H1 combo you will find on RK3288. And that same combo is fou=
nd on
> >many older SoC (actually even newer SoC un the VC8000Nano brand).
> >
> >Like all generation of Hantro chips, there is an optional dependency tha=
t can
> >exist between encoder and decoders. The question is if this requires a s=
ingle
> >driver to maintain a valid state or not. So far, it seems devs have assu=
me that
> >is it needed.
> >
> >p.s. fun fact, on most HW, the decoder rate is cut in half with running
> >concurrently with the encoder
> >
> >>
> >> I could also keep on integrating the rkvenc on that base instead.
> >
> >Do you know if there is any interaction between the encoder and decoder =
? Shared
> >registers, shared internal cache ? That's basically what differentiate H=
antro
> >here. Also, be aware that some folks are considering starting on RKVDEC2=
 driver,
> >are you looking at RK32/33 series ? or more RK35 ?
>
> I don't know of any limitations or interactions between the encoder and
> decoder.

I believe we should explore separate drivers, and if there is any interacti=
on,
try to model the shared piece through a shared block in the device tree.

In most cases, the decoder and encoder are separate blocks.
Also, the V4L2 stateless decoder interface covers only decoding.

Supporting both in the same driver has been painful, especially
the V4L2 negotiation, is hard to support for both encoders and decoders,
and has led to many bugs (and even worse, regressions) in the drivers.

Thanks,
Ezequiel