[libcamera-devel] [PATCH v2 2/4] pipeline: rpi: Remove additional external dma buf handling logic
Naushir Patuck
naush at raspberrypi.com
Mon Sep 4 11:07:52 CEST 2023
Hi Kieran,
Thank you for the review.
On Mon, 4 Sept 2023 at 09:48, Kieran Bingham
<kieran.bingham at ideasonboard.com> wrote:
>
> Quoting Naushir Patuck via libcamera-devel (2023-07-25 09:55:38)
> > There is no need to distinguish between dma bufs allocated outside of
> > libcamera and internally allocated buffers. As such, remove all the
> > special case handling of such buffers.
>
> Hrm ... I now wonder why this was added in the first place?
>
>
> But indeed - a dmabuf is a dmabuf - there shouldn't be explicit handling
> for a buffer depending on where it was allocated - unless it was an
> 'internal only' buffer ... that isn't part of the stream...
>
> Was this part of handling streams that had to have an internally
> allocated 'scratch' buffer to support when the user didn't provide a
> (required) buffer for a stream?
>
> If you're sure it's safe to go then I'm happy...
All this external dmabuf handling code was mostly speculative code as
it had never been tested with a real application doing external buffer
allocation. Now that I've actually got an example of an application
doing this, I can weed out the unnecessary code (like this bit here).
Regards,
Naush
>
>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>
> > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > ---
> > .../pipeline/rpi/common/pipeline_base.cpp | 16 ----------------
> > .../pipeline/rpi/common/pipeline_base.h | 1 -
> > src/libcamera/pipeline/rpi/common/rpi_stream.cpp | 11 +----------
> > src/libcamera/pipeline/rpi/common/rpi_stream.h | 2 --
> > 4 files changed, 1 insertion(+), 29 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > index 179a5b81a516..f244edc68a85 100644
> > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > @@ -1391,11 +1391,6 @@ void CameraData::handleStreamBuffer(FrameBuffer *buffer, RPi::Stream *stream)
> > */
> > Request *request = requestQueue_.empty() ? nullptr : requestQueue_.front();
> > if (!dropFrameCount_ && request && request->findBuffer(stream) == buffer) {
> > - /*
> > - * Check if this is an externally provided buffer, and if
> > - * so, we must stop tracking it in the pipeline handler.
> > - */
> > - handleExternalBuffer(buffer, stream);
> > /*
> > * Tag the buffer as completed, returning it to the
> > * application.
> > @@ -1435,17 +1430,6 @@ void CameraData::handleState()
> > }
> > }
> >
> > -void CameraData::handleExternalBuffer(FrameBuffer *buffer, RPi::Stream *stream)
> > -{
> > - unsigned int id = stream->getBufferId(buffer);
> > -
> > - if (!(id & MaskExternalBuffer))
> > - return;
> > -
> > - /* Stop the Stream object from tracking the buffer. */
> > - stream->removeExternalBuffer(buffer);
> > -}
> > -
> > void CameraData::checkRequestCompleted()
> > {
> > bool requestCompleted = false;
> > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.h b/src/libcamera/pipeline/rpi/common/pipeline_base.h
> > index f648e81054bb..8ee20a1b6d9b 100644
> > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.h
> > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.h
> > @@ -193,7 +193,6 @@ protected:
> > unsigned int ispOutputTotal_;
> >
> > private:
> > - void handleExternalBuffer(FrameBuffer *buffer, Stream *stream);
> > void checkRequestCompleted();
> > };
> >
> > diff --git a/src/libcamera/pipeline/rpi/common/rpi_stream.cpp b/src/libcamera/pipeline/rpi/common/rpi_stream.cpp
> > index e9ad1e6f0848..74b5abf447c7 100644
> > --- a/src/libcamera/pipeline/rpi/common/rpi_stream.cpp
> > +++ b/src/libcamera/pipeline/rpi/common/rpi_stream.cpp
> > @@ -78,16 +78,7 @@ unsigned int Stream::getBufferId(FrameBuffer *buffer) const
> >
> > void Stream::setExternalBuffer(FrameBuffer *buffer)
> > {
> > - bufferMap_.emplace(BufferMask::MaskExternalBuffer | id_.get(), buffer);
> > -}
> > -
> > -void Stream::removeExternalBuffer(FrameBuffer *buffer)
> > -{
> > - unsigned int id = getBufferId(buffer);
> > -
> > - /* Ensure we have this buffer in the stream, and it is marked external. */
> > - ASSERT(id & BufferMask::MaskExternalBuffer);
> > - bufferMap_.erase(id);
> > + bufferMap_.emplace(id_.get(), buffer);
> > }
> >
> > int Stream::prepareBuffers(unsigned int count)
> > diff --git a/src/libcamera/pipeline/rpi/common/rpi_stream.h b/src/libcamera/pipeline/rpi/common/rpi_stream.h
> > index 6edd304bdfe2..ca591f99cc45 100644
> > --- a/src/libcamera/pipeline/rpi/common/rpi_stream.h
> > +++ b/src/libcamera/pipeline/rpi/common/rpi_stream.h
> > @@ -28,7 +28,6 @@ enum BufferMask {
> > MaskStats = 0x010000,
> > MaskEmbeddedData = 0x020000,
> > MaskBayerData = 0x040000,
> > - MaskExternalBuffer = 0x100000,
> > };
> >
> > /*
> > @@ -78,7 +77,6 @@ public:
> > unsigned int getBufferId(FrameBuffer *buffer) const;
> >
> > void setExternalBuffer(FrameBuffer *buffer);
> > - void removeExternalBuffer(FrameBuffer *buffer);
> >
> > int prepareBuffers(unsigned int count);
> > int queueBuffer(FrameBuffer *buffer);
> > --
> > 2.34.1
> >
More information about the libcamera-devel
mailing list