Message-ID: <fnhsnjlvifnma1rqlsm9kqk7ao3bc174ic@4ax.com>
Date: Wed, 08 Jan 2025 09:52:20 +0000
From: John Cox <jc@...esim.co.uk>
To: Nicolas Dufresne <nicolas@...fresne.ca>
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

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?

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 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.

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