Message-ID: <CAPY8ntCxH2C=YEJEcee0b2UuXU+xZ0Ntbuvc29MLAipr9DCmmw@mail.gmail.com>
Date: Tue, 7 Jan 2025 16:13:58 +0000
From: Dave Stevenson <dave.stevenson@...pberrypi.com>
To: Nicolas Dufresne <nicolas@...fresne.ca>
Cc: Sakari Ailus <sakari.ailus@...ux.intel.com>, 
	Laurent Pinchart <laurent.pinchart@...asonboard.com>, 
	Mauro Carvalho Chehab <mchehab@...nel.org>, Rob Herring <robh@...nel.org>, 
	Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, 
	Florian Fainelli <florian.fainelli@...adcom.com>, 
	Broadcom internal kernel review list <bcm-kernel-feedback-list@...adcom.com>, John Cox <john.cox@...pberrypi.com>, 
	Dom Cobley <dom@...pberrypi.com>, review list <kernel-list@...pberrypi.com>, 
	Ezequiel Garcia <ezequiel@...guardiasur.com.ar>, John Cox <jc@...esim.co.uk>, 
	linux-media@...r.kernel.org, linux-kernel@...r.kernel.org, 
	devicetree@...r.kernel.org, linux-rpi-kernel@...ts.infradead.org, 
	linux-arm-kernel@...ts.infradead.org, John Cox <john.cox@...pberypi.com>
Subject: Re: [PATCH 0/7] Raspberry Pi HEVC decoder driver

Hi Nicolas

On Mon, 6 Jan 2025 at 20:46, Nicolas Dufresne <nicolas@...fresne.ca> wrote:
>
> Hi Dave,
>
> Le vendredi 20 décembre 2024 à 16:21 +0000, Dave Stevenson a écrit :
> > Hi All
> >
> > This has been in the pipeline for a while, but I've finally cleaned
> > up our HEVC decoder driver to be in a shape to at least get a first
> > review.
> > John Cox has done almost all of the work under contract to Raspberry
> > Pi, and I'm largely just doing the process of patch curation and
> > sending.
> >
> > There are a couple of questions raised in frameworks.
> > The main one is that the codec has 2 independent phases to the decode,
> > CABAC and reconstruction. To keep the decoder operating optimally
> > means that two requests need to be in process at once, whilst the
> > current frameworks don't want to allow as there is an implicit
> > assumption of only a single job being active at once, and
> > completition returns both buffers and releases the media request.
> >
> > The OUTPUT queue buffer is finished with and can be returned at the
> > end of phase 1, but the media request is still required for phase 2.
> > The frameworks currently force the driver to be returning both
> > together via v4l2_m2m_buf_done_and_job_finish. v4l2_m2m_job_finish
> > would complete the job without returning the buffer as we need,
> > however if the driver has set VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF
> > then we have a WARN in v4l2_m2m_job_finish.
> > Dropping the WARN as this series is currently doing isn't going to be
> > the right answer, but it isn't obvious what the right answer is.
> > Discussion required.
>
> I think part of the manual request completion RFC will be to evaluate the impact
> on VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF feature. MTK does not support
> interleaved interlaced decoding (only alternate), so they didn't have to
> implement that feature.
>
> Overall, It would be nice to get your feedback on the new manual request
> proposal, which is I believe better then the pin/unpin API you have in this
> serie.

I wasn't aware of that series, but I / John will take a look.

> >
> > We also have a need to hold on to the media request for phase 2. John
> > had discussed this with Ezequiel (and others) a couple of years back,
> > and hence suggested a patch that adds media_request_{pin,unpin} to
> > grab references on the media request. Discussion required on that
> > or a better way of handling it.
> >
> > I will apologise in advance for sending this V1 just before I head off
> > on the Christmas break, but will respond to things as soon as possible.
>
> One thing missing in this summary is how this driver is being validated
> (specially that for this one requires a downstream fork of FFMPEG). To this
> report we ask for:
>
> - v4l2-compliance results
> - Fluster conformance tests results [1] and I believe you need [2]
>
> [1] https://github.com/fluendo/fluster
> [2] https://github.com/fluendo/fluster/pull/179

Sure, I'll sort that before doing a V2.

> GStreamer support is there in main now, but without the needed software video
> converter for you column tiling, we can't use it for that (i.e. only works
> through GL or Wayland).

Can you point me at the right place for the software converter?
It's a relatively trivial reformat required to get it back into NV12 /
I420 or 10bit equivalents, so happy to do that. I think John already
has NEON optimised code if desired.

  Dave

> regards,
> Nicolas
>
> >
> > Thanks
> >   Dave
> >
> > Signed-off-by: Dave Stevenson <dave.stevenson@...pberrypi.com>
> > ---
> > Dave Stevenson (4):
> >       docs: uapi: media: Document Raspberry Pi NV12 column format
> >       media: ioctl: Add pixel formats NV12MT_COL128 and NV12MT_10_COL128
> >       media: dt-bindings: media: Add binding for the Raspberry Pi HEVC decoder
> >       arm: dts: bcm2711-rpi: Add HEVC decoder node
> >
> > Ezequiel Garcia (1):
> >       RFC: media: Add media_request_{pin,unpin} API
> >
> > John Cox (2):
> >       media: platform: Add Raspberry Pi HEVC decoder driver
> >       RFC: v4l2-mem2mem: Remove warning from v4l2_m2m_job_finish
> >
> >  .../bindings/media/raspberrypi,hevc-dec.yaml       |   72 +
> >  .../userspace-api/media/v4l/pixfmt-yuv-planar.rst  |   42 +
> >  MAINTAINERS                                        |   10 +
> >  arch/arm/boot/dts/broadcom/bcm2711-rpi.dtsi        |    5 +
> >  arch/arm/boot/dts/broadcom/bcm2711.dtsi            |    9 +
> >  drivers/media/mc/mc-request.c                      |   35 +
> >  drivers/media/platform/raspberrypi/Kconfig         |    1 +
> >  drivers/media/platform/raspberrypi/Makefile        |    1 +
> >  .../media/platform/raspberrypi/hevc_dec/Kconfig    |   17 +
> >  .../media/platform/raspberrypi/hevc_dec/Makefile   |    5 +
> >  .../media/platform/raspberrypi/hevc_dec/hevc_d.c   |  443 ++++
> >  .../media/platform/raspberrypi/hevc_dec/hevc_d.h   |  190 ++
> >  .../platform/raspberrypi/hevc_dec/hevc_d_h265.c    | 2629 ++++++++++++++++++++
> >  .../platform/raspberrypi/hevc_dec/hevc_d_hw.c      |  376 +++
> >  .../platform/raspberrypi/hevc_dec/hevc_d_hw.h      |  303 +++
> >  .../platform/raspberrypi/hevc_dec/hevc_d_video.c   |  685 +++++
> >  .../platform/raspberrypi/hevc_dec/hevc_d_video.h   |   38 +
> >  drivers/media/v4l2-core/v4l2-ioctl.c               |    2 +
> >  drivers/media/v4l2-core/v4l2-mem2mem.c             |    7 -
> >  include/media/media-request.h                      |   12 +
> >  include/uapi/linux/videodev2.h                     |    5 +
> >  21 files changed, 4880 insertions(+), 7 deletions(-)
> > ---
> > base-commit: e90c9612ac3969cb8206029a26bcd2b6f5d4a942
> > change-id: 20241212-media-rpi-hevc-dec-3b5be739f3bd
> >
> > Best regards,
>

Powered by blists - more mailing lists