[v1,0/6] rkisp1: Allow usage of more than 4 buffers
mbox series

Message ID 20250630081126.2384387-1-stefan.klug@ideasonboard.com
Headers show
Series
  • rkisp1: Allow usage of more than 4 buffers
Related show

Message

Stefan Klug June 30, 2025, 8:11 a.m. UTC
Hi all,

In the rkisp1 pipeline the value of StreamConfiuration.bufferCount is
reset to 4 on validate(). This effectively limits the number of buffers
that are in the loop to 4. As soon as the consumers
(gstream/pipewire...) of the buffers need more than 1 buffer at a time,
we start to see frame drops. Sven resurrected an old series [1] that
solves the issue at the expense of an additional control and breaking
changes in the API.

In the IoB code camp two weeks ago there were some discussions regarding
improvements in the rkisp1 regulation and per-frame-control (PFC). PFC
has been an ongoing topic for quite some time now and it seems to slowly
converge. Still we were hesitant in breaking the API or introducing new
controls before the overall concepts have stabilized enough.

To mitigate the pain mentioned above we came up with a minimal changeset
that allows to request more than 4 buffers. Only 4 (or the pipeline
depth reported by the pipeline) buffers are queued into the device.
This has the same benefits as [1] but doesn't require an API break.

Updates in Version 1 of the series (after the RFC):
- Included the feedback from reviews of the RFC (For etails see the chaneglog in
  the individual patches)
- Included two patches with fixes from Sven an Kieran
- I modified patch (6/6) from Kieran a bit after the review comment from
  Laurent. @Kieran could you have a look at that as I don't have a easy way to
  replicate the issue? Ideally I would like to merge that patch into patch 2/6
  as it fixes an issue introduced there. For now I left it separate for easier
  review.

Best regards,
Stefan

Kieran Bingham (1):
  libcamera: pipeline_handler: cancel waiting requests first

NĂ­colas F. R. A. Prado (1):
  libcamera: pipeline: rkisp1: Don't rely on bufferCount

Stefan Klug (4):
  libcamera: pipeline_handler: Move waitingRequests_ into camera class
  libcamera: pipeline_handler: Allow to limit the number of queued
    requests
  pipeline: rkisp1: Limit the maximum number of buffers queued in
  pipeline: rkisp1: Properly handle the bufferCount set in the stream
    configuration

 include/libcamera/internal/camera.h           |  2 +
 include/libcamera/internal/pipeline_handler.h |  9 ++--
 src/libcamera/pipeline/rkisp1/rkisp1.cpp      | 28 ++++++----
 src/libcamera/pipeline/rkisp1/rkisp1_path.cpp |  7 +--
 src/libcamera/pipeline/rkisp1/rkisp1_path.h   |  4 +-
 src/libcamera/pipeline_handler.cpp            | 54 +++++++++++++------
 6 files changed, 65 insertions(+), 39 deletions(-)

Comments

Kieran Bingham June 30, 2025, 10:45 a.m. UTC | #1
Quoting Stefan Klug (2025-06-30 09:11:15)
> Hi all,
> 
> In the rkisp1 pipeline the value of StreamConfiuration.bufferCount is
> reset to 4 on validate(). This effectively limits the number of buffers
> that are in the loop to 4. As soon as the consumers
> (gstream/pipewire...) of the buffers need more than 1 buffer at a time,
> we start to see frame drops. Sven resurrected an old series [1] that
> solves the issue at the expense of an additional control and breaking
> changes in the API.
> 
> In the IoB code camp two weeks ago there were some discussions regarding
> improvements in the rkisp1 regulation and per-frame-control (PFC). PFC
> has been an ongoing topic for quite some time now and it seems to slowly
> converge. Still we were hesitant in breaking the API or introducing new
> controls before the overall concepts have stabilized enough.
> 
> To mitigate the pain mentioned above we came up with a minimal changeset
> that allows to request more than 4 buffers. Only 4 (or the pipeline
> depth reported by the pipeline) buffers are queued into the device.
> This has the same benefits as [1] but doesn't require an API break.
> 
> Updates in Version 1 of the series (after the RFC):
> - Included the feedback from reviews of the RFC (For etails see the chaneglog in
>   the individual patches)
> - Included two patches with fixes from Sven an Kieran
> - I modified patch (6/6) from Kieran a bit after the review comment from
>   Laurent. @Kieran could you have a look at that as I don't have a easy way to
>   replicate the issue? Ideally I would like to merge that patch into patch 2/6
>   as it fixes an issue introduced there. For now I left it separate for easier
>   review.

I haven't understood yet how they are tied for 6/6 and 2/6 - so I do'nt
mind them being merged or kept separate. If the request queue was
already there before this series then a separate patch is fine I think.
It seems like 2/6 is just moving the queue into the camera data ?

Whichever way you prefer.

However there are some intereseting CI failures on this series:

https://gitlab.freedesktop.org/camera/libcamera/-/pipelines/1460735


> 
> Best regards,
> Stefan
> 
> Kieran Bingham (1):
>   libcamera: pipeline_handler: cancel waiting requests first
> 
> NĂ­colas F. R. A. Prado (1):
>   libcamera: pipeline: rkisp1: Don't rely on bufferCount
> 
> Stefan Klug (4):
>   libcamera: pipeline_handler: Move waitingRequests_ into camera class
>   libcamera: pipeline_handler: Allow to limit the number of queued
>     requests
>   pipeline: rkisp1: Limit the maximum number of buffers queued in
>   pipeline: rkisp1: Properly handle the bufferCount set in the stream
>     configuration
> 
>  include/libcamera/internal/camera.h           |  2 +
>  include/libcamera/internal/pipeline_handler.h |  9 ++--
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp      | 28 ++++++----
>  src/libcamera/pipeline/rkisp1/rkisp1_path.cpp |  7 +--
>  src/libcamera/pipeline/rkisp1/rkisp1_path.h   |  4 +-
>  src/libcamera/pipeline_handler.cpp            | 54 +++++++++++++------
>  6 files changed, 65 insertions(+), 39 deletions(-)
> 
> -- 
> 2.48.1
>