Re: [PATCH v3 5/5] media: meson: vdec: add VP9 decoder support

From: Nicolas Dufresne <nicolas@ndufresne.ca>
To: Neil Armstrong <narmstrong@baylibre.com>,
	mchehab@kernel.org, hans.verkuil@cisco.com
Cc: Maxime Jourdan <mjourdan@baylibre.com>,
	linux-media@vger.kernel.org, linux-amlogic@lists.infradead.org,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 5/5] media: meson: vdec: add VP9 decoder support
Date: Sat, 08 Feb 2020 16:19:34 -0500	[thread overview]
Message-ID: <9aec0e87cb600ec1d89037f9527544b3c6098921.camel@ndufresne.ca> (raw)
In-Reply-To: <49e6168b-ffed-6011-3b1f-455224d3130b@baylibre.com>

Le jeudi 06 février 2020 à 09:08 +0100, Neil Armstrong a écrit :
> On 03/02/2020 04:11, Nicolas Dufresne wrote:
> > Hi Neil,
> > 
> > Le jeudi 16 janvier 2020 à 14:34 +0100, Neil Armstrong a écrit :
> > > From: Maxime Jourdan <mjourdan@baylibre.com>
> > > 
> > > This adds VP9 decoding for the Amlogic GXL, G12A & SM1 SoCs, using
> > > the commong "HEVC" HW decoder.
> > > 
> > > For G12A & SM1, it uses the IOMMU support from the firmware.
> > > 
> > > For 10bit decoding, the firmware can only decode in the proprietary
> > > Amlogic Framebuffer Compression format, but can output in 8bit NV12
> > > buffer while writing the decoded frame.
> > > 
> > > Signed-off-by: Maxime Jourdan <mjourdan@baylibre.com>
> > > Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> > > ---
> > >  drivers/staging/media/meson/vdec/Makefile     |    2 +-
> > >  drivers/staging/media/meson/vdec/codec_vp9.c  | 2139 +++++++++++++++++
> > >  drivers/staging/media/meson/vdec/codec_vp9.h  |   13 +
> > >  drivers/staging/media/meson/vdec/hevc_regs.h  |    7 +
> > >  drivers/staging/media/meson/vdec/vdec.c       |    5 +
> > >  .../staging/media/meson/vdec/vdec_helpers.c   |    4 +
> > >  .../staging/media/meson/vdec/vdec_platform.c  |   38 +
> > >  7 files changed, 2207 insertions(+), 1 deletion(-)
> > >  create mode 100644 drivers/staging/media/meson/vdec/codec_vp9.c
> > >  create mode 100644 drivers/staging/media/meson/vdec/codec_vp9.h
> > > 
> 
> [...]
> 
> > > diff --git a/drivers/staging/media/meson/vdec/vdec_platform.c b/drivers/staging/media/meson/vdec/vdec_platform.c
> > > index e9356a46828f..72a833b1cebd 100644
> > > --- a/drivers/staging/media/meson/vdec/vdec_platform.c
> > > +++ b/drivers/staging/media/meson/vdec/vdec_platform.c
> > > @@ -8,8 +8,10 @@
> > >  #include "vdec.h"
> > >  
> > >  #include "vdec_1.h"
> > > +#include "vdec_hevc.h"
> > >  #include "codec_mpeg12.h"
> > >  #include "codec_h264.h"
> > > +#include "codec_vp9.h"
> > >  
> > >  static const struct amvdec_format vdec_formats_gxbb[] = {
> > >  	{
> > > @@ -51,6 +53,18 @@ static const struct amvdec_format vdec_formats_gxbb[] = {
> > >  
> > >  static const struct amvdec_format vdec_formats_gxl[] = {
> > >  	{
> > > +		.pixfmt = V4L2_PIX_FMT_VP9,
> > > +		.min_buffers = 16,
> > > +		.max_buffers = 24,
> > > +		.max_width = 3840,
> > > +		.max_height = 2160,
> > > +		.vdec_ops = &vdec_hevc_ops,
> > > +		.codec_ops = &codec_vp9_ops,
> > > +		.firmware_path = "meson/vdec/gxl_vp9.bin",
> > 
> > Is there a pull request pending for this firmware ? I could not test as
> > this firmware was missing. Note that it could be nice to remove the
> > format from the enumeration in that case, as it's very confusing
> > initially.
> 
> It has been merged yesterday, sorry for the delay.
> 
> With maxime's patch, we tested it using ffmpeg master, mpv master with drm-prime
> rendering. We have a buildroot repo with the changed needed :
> 
> https://gitlab.com/baylibre/amlogic/atv/buildroot-yukawa
> 
> I will respin a v4 with the small fix from maxime.

Ok, I see it now. I've given a try, the output was corrupted with this
file from Android CTS + GStreamer 1.16.1 (Fedora 31 build).

  Profile_0_8bit/buf/crowd_run_1080X512_fr30_bd8_8buf_l3.webm

Looking forward Maxime's patches. Just like the H264 case, seeking with
kmssink lead to stalls, but this time, the kernel give only one
message:

  [498230.951737] meson-vdec c8820000.video-codec: No dst buffer available

And now I remember a comment, I guess it was Maxime about increasing
the some number of buffers from 2 to 3. So I'll try and find time to
get V4 and test again. Quite some improvement over my last series of
test with GStreamer. It has this particularity of running input/output
and capture QBUF on potentially 3 different threads.

> 
> Neil
> 
> 
> > > +		.pixfmts_cap = { V4L2_PIX_FMT_NV12M, 0 },
> > > +		.flags = V4L2_FMT_FLAG_COMPRESSED |
> > > +			 V4L2_FMT_FLAG_DYN_RESOLUTION,
> > > +	}, {
> > >  		.pixfmt = V4L2_PIX_FMT_H264,
> > >  		.min_buffers = 2,
> > >  		.max_buffers = 24,
> > > @@ -127,6 +141,18 @@ static const struct amvdec_format vdec_formats_gxm[] = {
> > >  
> > >  static const struct amvdec_format vdec_formats_g12a[] = {
> > >  	{
> > > +		.pixfmt = V4L2_PIX_FMT_VP9,
> > > +		.min_buffers = 16,
> > > +		.max_buffers = 24,
> > > +		.max_width = 3840,
> > > +		.max_height = 2160,
> > > +		.vdec_ops = &vdec_hevc_ops,
> > > +		.codec_ops = &codec_vp9_ops,
> > > +		.firmware_path = "meson/vdec/g12a_vp9.bin",
> > > +		.pixfmts_cap = { V4L2_PIX_FMT_NV12M, 0 },
> > > +		.flags = V4L2_FMT_FLAG_COMPRESSED |
> > > +			 V4L2_FMT_FLAG_DYN_RESOLUTION,
> > > +	}, {
> > >  		.pixfmt = V4L2_PIX_FMT_H264,
> > >  		.min_buffers = 2,
> > >  		.max_buffers = 24,
> > > @@ -165,6 +191,18 @@ static const struct amvdec_format vdec_formats_g12a[] = {
> > >  
> > >  static const struct amvdec_format vdec_formats_sm1[] = {
> > >  	{
> > > +		.pixfmt = V4L2_PIX_FMT_VP9,
> > > +		.min_buffers = 16,
> > > +		.max_buffers = 24,
> > > +		.max_width = 3840,
> > > +		.max_height = 2160,
> > > +		.vdec_ops = &vdec_hevc_ops,
> > > +		.codec_ops = &codec_vp9_ops,
> > > +		.firmware_path = "meson/vdec/g12a_vp9.bin",
> > > +		.pixfmts_cap = { V4L2_PIX_FMT_NV12M, 0 },
> > > +		.flags = V4L2_FMT_FLAG_COMPRESSED |
> > > +			 V4L2_FMT_FLAG_DYN_RESOLUTION,
> > > +	}, {
> > >  		.pixfmt = V4L2_PIX_FMT_H264,
> > >  		.min_buffers = 2,
> > >  		.max_buffers = 24,