Message-ID: <fd5b45f02e5a7dcc828a8e9ae9e6fb2948532dff.camel@ndufresne.ca>
Date: Wed, 08 Jan 2025 14:15:20 -0500
From: Nicolas Dufresne <nicolas@...fresne.ca>
To: John Cox <jc@...esim.co.uk>
Cc: Dave Stevenson <dave.stevenson@...pberrypi.com>, 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>, 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 John,

Le mercredi 08 janvier 2025 à 09:52 +0000, John Cox a écrit :
> On Mon, 06 Jan 2025 15:46:51 -0500, you 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.
> 
> Is the effect of the manual complete API different from the pin API? Pin
> incs the ref count on the media object to prevent competion,
> manaul_complete sets a flag to do the same thing. Or have I missed the
> point?

The request contains "objects", usually controls and a src buffer. The state of
the request goes to complete implicitly when the last object is removed. The
manual completion avoids adding ref-count on top of this, and simply disable the
implicit signalling of the request. With that we can signal everything in the
most logical order:

1. Program the register from controls -> mark controls complete
2. Entropy decode the stream -> mark src buffer done
3. Reconstruct the image -> mark dst buffer done
4. Signal the request completion

Before that, you always had to hold on the src buffer, and only mark it done
after the reconstruction was complete and the dst buffer was marked done. That
didn't matter for single function HW of course.

Unlike the pin/unpin, manual completion mode removes the implicit completion
bound to the fact the last "object" is removed from the request, and leave it to
the driver to decide when to actually signal the request. Internally, the give a
reference to the driver of course (similar to pin).

> 
> I don't mind what call is used as long as the effect is to be able to
> delay media object completion. (In my first veraion of the code I
> created a dummy object and attached it to the media object, when I
> wanted to unpin I deleted the dummy object - pin was just a cleaner
> API.)

The manual completion patchset is very similar really in you step back.

> 
> The pin API is counted and needs no new structure elements (which is
> nice), but manual_complete does give a flag that allows other functions
> to know that holding on to the media object whilst releasing OUTPUT is
> intentional so can be made to work cleanly with things like
> v4l2_m2m_job_finish so is probably the better solution (assuming my
> understand of how it works is correct). 
> 
> I'll try to build a version of the code using manual_complete in the
> next few days.

Thanks! your feedback will be very useful.

Nicolas

> 
> Regards
> 
> JC
> 
> > > 
> > > 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
> > 
> > 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).
> > 
> > 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