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

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

Commit Message

Naushir Patuck July 21, 2023, 9:37 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 ----------------
 src/libcamera/pipeline/rpi/common/rpi_stream.cpp | 11 +----------
 src/libcamera/pipeline/rpi/common/rpi_stream.h   |  2 --
 3 files changed, 1 insertion(+), 28 deletions(-)

Comments

Jacopo Mondi July 24, 2023, 7:34 a.m. UTC | #1 
Hi Naush

On Fri, Jul 21, 2023 at 10:37:57AM +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>

Should you remove the function declaration too ?

src/libcamera/pipeline/rpi/common/pipeline_base.h:      void handleExternalBuffer(FrameBuffer *buffer, Stream *stream);

I'm surprised the compiler doesn't generate a linkage error..



> ---
>  .../pipeline/rpi/common/pipeline_base.cpp        | 16 ----------------
>  src/libcamera/pipeline/rpi/common/rpi_stream.cpp | 11 +----------
>  src/libcamera/pipeline/rpi/common/rpi_stream.h   |  2 --
>  3 files changed, 1 insertion(+), 28 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/rpi_stream.cpp b/src/libcamera/pipeline/rpi/common/rpi_stream.cpp
> index 1d05c5acc0d9..07b8de6875fe 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 July 24, 2023, 7:49 a.m. UTC | #2 
Hi Jacopo,

Thank you for your feedback.

On Mon, 24 Jul 2023 at 08:34, Jacopo Mondi
<jacopo.mondi@ideasonboard.com> wrote:
>
> Hi Naush
>
> On Fri, Jul 21, 2023 at 10:37:57AM +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>
>
> Should you remove the function declaration too ?
>
> src/libcamera/pipeline/rpi/common/pipeline_base.h:      void handleExternalBuffer(FrameBuffer *buffer, Stream *stream);
>
> I'm surprised the compiler doesn't generate a linkage error..

Yes it definitely should be removed.  I'm also a bit surprised the
compiler didn't shout!

Regards,
Naush

>
>
>
> > ---
> >  .../pipeline/rpi/common/pipeline_base.cpp        | 16 ----------------
> >  src/libcamera/pipeline/rpi/common/rpi_stream.cpp | 11 +----------
> >  src/libcamera/pipeline/rpi/common/rpi_stream.h   |  2 --
> >  3 files changed, 1 insertion(+), 28 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/rpi_stream.cpp b/src/libcamera/pipeline/rpi/common/rpi_stream.cpp
> > index 1d05c5acc0d9..07b8de6875fe 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/rpi_stream.cpp b/src/libcamera/pipeline/rpi/common/rpi_stream.cpp
index 1d05c5acc0d9..07b8de6875fe 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);