[libcamera-devel,v2,2/4] pipeline: rpi: Remove additional external dma buf handling logic
diff mbox series

Message ID 20230725085540.24863-3-naush@raspberrypi.com
State Accepted
Headers show
Series
  • Raspberry Pi: External buffer handling
Related show

Commit Message

Naushir Patuck July 25, 2023, 8:55 a.m. UTC 
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.

Signed-off-by: Naushir Patuck <naush@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(-)

Comments

Jacopo Mondi July 27, 2023, 9:33 a.m. UTC | #1 
Hi Naush

On Tue, Jul 25, 2023 at 09:55:38AM +0100, Naushir Patuck via libcamera-devel wrote:
> 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.
>
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>

Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

Thanks
  j

> ---
>  .../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
>
Kieran Bingham Sept. 4, 2023, 8:48 a.m. UTC | #2 
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...

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> Signed-off-by: Naushir Patuck <naush@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
>
Naushir Patuck Sept. 4, 2023, 9:07 a.m. UTC | #3 
Hi Kieran,

Thank you for the review.

On Mon, 4 Sept 2023 at 09:48, Kieran Bingham
<kieran.bingham@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@ideasonboard.com>
>
> > Signed-off-by: Naushir Patuck <naush@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
> >

Patch
diff mbox series 

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);