| Message ID | 20230725085540.24863-4-naush@raspberrypi.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Quoting Naushir Patuck via libcamera-devel (2023-07-25 09:55:39) > Since we don't distinguish between externally and internally allocated > dma bufs, rename this function to setExportedBuffer() to clearer on its > function. > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > --- > src/libcamera/pipeline/rpi/common/pipeline_base.cpp | 2 +- > src/libcamera/pipeline/rpi/common/rpi_stream.cpp | 2 +- > src/libcamera/pipeline/rpi/common/rpi_stream.h | 2 +- > 3 files changed, 3 insertions(+), 3 deletions(-) > > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp > index f244edc68a85..fc0c0ec3c53c 100644 > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp > @@ -764,7 +764,7 @@ int PipelineHandlerBase::queueRequestDevice(Camera *camera, Request *request) > * outside the v4l2 device. Store it in the stream buffer list > * so we can track it. > */ > - stream->setExternalBuffer(buffer); > + stream->setExportedBuffer(buffer); I'm a bit confused. Aren't these (externally allocated buffers) 'imported' buffers ? Does this conflict with the 'RPi::Stream::setExportedBuffers() call? That said - both setExportedBuffer and setExportedBuffers perform the same action (on each buffer) - so it seems this is expected behaviour... Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> There's a comment in Stream::prepareBuffers() /* Add these exported buffers to the internal/external buffer list. */ Is that incorrect/redundant now? Is this list really tracking internal/external buffers? Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > } > > /* > diff --git a/src/libcamera/pipeline/rpi/common/rpi_stream.cpp b/src/libcamera/pipeline/rpi/common/rpi_stream.cpp > index 74b5abf447c7..e1858c731f57 100644 > --- a/src/libcamera/pipeline/rpi/common/rpi_stream.cpp > +++ b/src/libcamera/pipeline/rpi/common/rpi_stream.cpp > @@ -76,7 +76,7 @@ unsigned int Stream::getBufferId(FrameBuffer *buffer) const > return it->first; > } > > -void Stream::setExternalBuffer(FrameBuffer *buffer) > +void Stream::setExportedBuffer(FrameBuffer *buffer) > { > bufferMap_.emplace(id_.get(), buffer); > } > diff --git a/src/libcamera/pipeline/rpi/common/rpi_stream.h b/src/libcamera/pipeline/rpi/common/rpi_stream.h > index ca591f99cc45..d1289c4679b9 100644 > --- a/src/libcamera/pipeline/rpi/common/rpi_stream.h > +++ b/src/libcamera/pipeline/rpi/common/rpi_stream.h > @@ -76,7 +76,7 @@ public: > const BufferMap &getBuffers() const; > unsigned int getBufferId(FrameBuffer *buffer) const; > > - void setExternalBuffer(FrameBuffer *buffer); > + void setExportedBuffer(FrameBuffer *buffer); > > int prepareBuffers(unsigned int count); > int queueBuffer(FrameBuffer *buffer); > -- > 2.34.1 >
Hi Kieran, Thank you for the review. On Mon, 4 Sept 2023 at 10:01, Kieran Bingham <kieran.bingham@ideasonboard.com> wrote: > > Quoting Naushir Patuck via libcamera-devel (2023-07-25 09:55:39) > > Since we don't distinguish between externally and internally allocated > > dma bufs, rename this function to setExportedBuffer() to clearer on its > > function. > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > --- > > src/libcamera/pipeline/rpi/common/pipeline_base.cpp | 2 +- > > src/libcamera/pipeline/rpi/common/rpi_stream.cpp | 2 +- > > src/libcamera/pipeline/rpi/common/rpi_stream.h | 2 +- > > 3 files changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp > > index f244edc68a85..fc0c0ec3c53c 100644 > > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp > > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp > > @@ -764,7 +764,7 @@ int PipelineHandlerBase::queueRequestDevice(Camera *camera, Request *request) > > * outside the v4l2 device. Store it in the stream buffer list > > * so we can track it. > > */ > > - stream->setExternalBuffer(buffer); > > + stream->setExportedBuffer(buffer); > > I'm a bit confused. > > Aren't these (externally allocated buffers) 'imported' buffers ? I use "exported" as they are exported from the application. I guess I was trying to follow the convention from PipelineHandler::exportFrameBuffer that gets called through the FrameBufferAllocator route. I do kind of agree that for both sound like "import" might be the better term, but I can live it with :-) > > Does this conflict with the 'RPi::Stream::setExportedBuffers() call? Different prototype, but does exactly the same function - it adds one or more buffers to the availableBuffers_ vector. > > That said - both setExportedBuffer and setExportedBuffers perform the > same action (on each buffer) - so it seems this is expected behaviour... > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > There's a comment in Stream::prepareBuffers() > /* Add these exported buffers to the internal/external buffer list. */ > > Is that incorrect/redundant now? Is this list really tracking > internal/external buffers? No, I think that comment is still valid as availableBuffers_ holds all the buffers (internal or externally allocated) that the stream may handle during its lifetime. If any of that doesn't make sense, please shout! Cheers, Naush > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > } > > > > /* > > diff --git a/src/libcamera/pipeline/rpi/common/rpi_stream.cpp b/src/libcamera/pipeline/rpi/common/rpi_stream.cpp > > index 74b5abf447c7..e1858c731f57 100644 > > --- a/src/libcamera/pipeline/rpi/common/rpi_stream.cpp > > +++ b/src/libcamera/pipeline/rpi/common/rpi_stream.cpp > > @@ -76,7 +76,7 @@ unsigned int Stream::getBufferId(FrameBuffer *buffer) const > > return it->first; > > } > > > > -void Stream::setExternalBuffer(FrameBuffer *buffer) > > +void Stream::setExportedBuffer(FrameBuffer *buffer) > > { > > bufferMap_.emplace(id_.get(), buffer); > > } > > diff --git a/src/libcamera/pipeline/rpi/common/rpi_stream.h b/src/libcamera/pipeline/rpi/common/rpi_stream.h > > index ca591f99cc45..d1289c4679b9 100644 > > --- a/src/libcamera/pipeline/rpi/common/rpi_stream.h > > +++ b/src/libcamera/pipeline/rpi/common/rpi_stream.h > > @@ -76,7 +76,7 @@ public: > > const BufferMap &getBuffers() const; > > unsigned int getBufferId(FrameBuffer *buffer) const; > > > > - void setExternalBuffer(FrameBuffer *buffer); > > + void setExportedBuffer(FrameBuffer *buffer); > > > > int prepareBuffers(unsigned int count); > > int queueBuffer(FrameBuffer *buffer); > > -- > > 2.34.1 > >
Quoting Naushir Patuck (2023-09-04 10:35:06) > Hi Kieran, > > Thank you for the review. > > On Mon, 4 Sept 2023 at 10:01, Kieran Bingham > <kieran.bingham@ideasonboard.com> wrote: > > > > Quoting Naushir Patuck via libcamera-devel (2023-07-25 09:55:39) > > > Since we don't distinguish between externally and internally allocated > > > dma bufs, rename this function to setExportedBuffer() to clearer on its > > > function. > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > > --- > > > src/libcamera/pipeline/rpi/common/pipeline_base.cpp | 2 +- > > > src/libcamera/pipeline/rpi/common/rpi_stream.cpp | 2 +- > > > src/libcamera/pipeline/rpi/common/rpi_stream.h | 2 +- > > > 3 files changed, 3 insertions(+), 3 deletions(-) > > > > > > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp > > > index f244edc68a85..fc0c0ec3c53c 100644 > > > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp > > > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp > > > @@ -764,7 +764,7 @@ int PipelineHandlerBase::queueRequestDevice(Camera *camera, Request *request) > > > * outside the v4l2 device. Store it in the stream buffer list > > > * so we can track it. > > > */ > > > - stream->setExternalBuffer(buffer); > > > + stream->setExportedBuffer(buffer); > > > > I'm a bit confused. > > > > Aren't these (externally allocated buffers) 'imported' buffers ? > > I use "exported" as they are exported from the application. I guess I > was trying to follow the convention from > PipelineHandler::exportFrameBuffer that gets called through the > FrameBufferAllocator route. I do kind of agree that for both sound > like "import" might be the better term, but I can live it with :-) > > > > > Does this conflict with the 'RPi::Stream::setExportedBuffers() call? > > Different prototype, but does exactly the same function - it adds one > or more buffers to the availableBuffers_ vector. > > > > > That said - both setExportedBuffer and setExportedBuffers perform the > > same action (on each buffer) - so it seems this is expected behaviour... > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > There's a comment in Stream::prepareBuffers() > > /* Add these exported buffers to the internal/external buffer list. */ > > > > Is that incorrect/redundant now? Is this list really tracking > > internal/external buffers? > > No, I think that comment is still valid as availableBuffers_ holds all > the buffers (internal or externally allocated) that the stream may > handle during its lifetime. > If any of that doesn't make sense, please shout! I think it's clearer now. The list doesn't distinguish between internally/externally allocated buffers - it's a list of /all/ buffers used by the stream. -- Kieran > > Cheers, > Naush > > > > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > > > > > } > > > > > > /* > > > diff --git a/src/libcamera/pipeline/rpi/common/rpi_stream.cpp b/src/libcamera/pipeline/rpi/common/rpi_stream.cpp > > > index 74b5abf447c7..e1858c731f57 100644 > > > --- a/src/libcamera/pipeline/rpi/common/rpi_stream.cpp > > > +++ b/src/libcamera/pipeline/rpi/common/rpi_stream.cpp > > > @@ -76,7 +76,7 @@ unsigned int Stream::getBufferId(FrameBuffer *buffer) const > > > return it->first; > > > } > > > > > > -void Stream::setExternalBuffer(FrameBuffer *buffer) > > > +void Stream::setExportedBuffer(FrameBuffer *buffer) > > > { > > > bufferMap_.emplace(id_.get(), buffer); > > > } > > > diff --git a/src/libcamera/pipeline/rpi/common/rpi_stream.h b/src/libcamera/pipeline/rpi/common/rpi_stream.h > > > index ca591f99cc45..d1289c4679b9 100644 > > > --- a/src/libcamera/pipeline/rpi/common/rpi_stream.h > > > +++ b/src/libcamera/pipeline/rpi/common/rpi_stream.h > > > @@ -76,7 +76,7 @@ public: > > > const BufferMap &getBuffers() const; > > > unsigned int getBufferId(FrameBuffer *buffer) const; > > > > > > - void setExternalBuffer(FrameBuffer *buffer); > > > + void setExportedBuffer(FrameBuffer *buffer); > > > > > > int prepareBuffers(unsigned int count); > > > int queueBuffer(FrameBuffer *buffer); > > > -- > > > 2.34.1 > > >
diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp index f244edc68a85..fc0c0ec3c53c 100644 --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp @@ -764,7 +764,7 @@ int PipelineHandlerBase::queueRequestDevice(Camera *camera, Request *request) * outside the v4l2 device. Store it in the stream buffer list * so we can track it. */ - stream->setExternalBuffer(buffer); + stream->setExportedBuffer(buffer); } /* diff --git a/src/libcamera/pipeline/rpi/common/rpi_stream.cpp b/src/libcamera/pipeline/rpi/common/rpi_stream.cpp index 74b5abf447c7..e1858c731f57 100644 --- a/src/libcamera/pipeline/rpi/common/rpi_stream.cpp +++ b/src/libcamera/pipeline/rpi/common/rpi_stream.cpp @@ -76,7 +76,7 @@ unsigned int Stream::getBufferId(FrameBuffer *buffer) const return it->first; } -void Stream::setExternalBuffer(FrameBuffer *buffer) +void Stream::setExportedBuffer(FrameBuffer *buffer) { bufferMap_.emplace(id_.get(), buffer); } diff --git a/src/libcamera/pipeline/rpi/common/rpi_stream.h b/src/libcamera/pipeline/rpi/common/rpi_stream.h index ca591f99cc45..d1289c4679b9 100644 --- a/src/libcamera/pipeline/rpi/common/rpi_stream.h +++ b/src/libcamera/pipeline/rpi/common/rpi_stream.h @@ -76,7 +76,7 @@ public: const BufferMap &getBuffers() const; unsigned int getBufferId(FrameBuffer *buffer) const; - void setExternalBuffer(FrameBuffer *buffer); + void setExportedBuffer(FrameBuffer *buffer); int prepareBuffers(unsigned int count); int queueBuffer(FrameBuffer *buffer);