Message ID | 20190829232653.13214-5-niklas.soderlund@ragnatech.se |
---|---|
State | Superseded |
Headers | show |
Series |
|
Related | show |
Hi Niklas, Thank you for the patch. On Fri, Aug 30, 2019 at 01:26:43AM +0200, Niklas Söderlund wrote: > Buffers internal to a pipeline handler (buffers cycled between a CSI-2 > pipeline to a ISP pipeline, parameters and statistics buffers) needs to > be prepared before they can be properly used inside a pipeline handler. > > At this point the preparation consists of mapping the Buffer objects > memory and associating it with a request. Instead of adding this helper > on the Buffer object itself add it to the pipeline handler base class to > prevent it from being exposed to applications. > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > src/libcamera/include/pipeline_handler.h | 2 ++ > src/libcamera/pipeline_handler.cpp | 18 ++++++++++++++++++ > 2 files changed, 20 insertions(+) > > diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h > index ffc7adb802215313..91d40ef40a465c4e 100644 > --- a/src/libcamera/include/pipeline_handler.h > +++ b/src/libcamera/include/pipeline_handler.h > @@ -98,6 +98,8 @@ protected: > > CameraData *cameraData(const Camera *camera); > > + void prepareInternalBuffer(Buffer *buffer, Request *request, > + BufferMemory *mem); > CameraManager *manager_; > > private: > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp > index 558b4b254d111e31..846272485c7d2fc0 100644 > --- a/src/libcamera/pipeline_handler.cpp > +++ b/src/libcamera/pipeline_handler.cpp > @@ -485,6 +485,24 @@ void PipelineHandler::hotplugMediaDevice(MediaDevice *media) > media->disconnected.connect(this, &PipelineHandler::mediaDeviceDisconnected); > } > > +/** > + * \brief Prepare buffer for internal usage by a pipeline handler > + * \param[in,out] buffer The buffer to prepare > + * \param[in] request The request to associate the \a buffer with > + * \param[in] mem The memory to associate the \a buffer with > + * > + * Pipeline handlers creating internal buffers to facilitate data flow in the > + * pipeline need to prepare the buffers by setting up the buffer object state. > + * This function help pipeline handler implementations to perform this > + * preparation. > + */ I'm afraid I don't like this much. First of all, reading the documentation, I have no idea how/when this is supposed to be used, so we need to improve that. Then, looking at the rest of the series, I see this method only used for the rkisp1 stats and params buffers. We have other internal buffers, such as between the CIO2 and ImgU, or ImgU params or stats buffers. We should use this method consistently, and I would like to see pipeline handlers updated to use it as part of this patch, to show how it is supposed to be used (although if the documentation was clearer maybe I wouldn't feel this need). Finally, as mentioned in a review of v1, this belongs to the Buffer class. I understand you don't want to expose this to applications, but the method really doesn't belong to the PipelineHandler class. At the very least it should be a global helper method. I think this however calls for a rework of the buffer API. > +void PipelineHandler::prepareInternalBuffer(Buffer *buffer, Request *request, > + BufferMemory *mem) > +{ > + buffer->request_ = request; > + buffer->mem_ = mem; > +} > + > /** > * \brief Slot for the MediaDevice disconnected signal > */
diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h index ffc7adb802215313..91d40ef40a465c4e 100644 --- a/src/libcamera/include/pipeline_handler.h +++ b/src/libcamera/include/pipeline_handler.h @@ -98,6 +98,8 @@ protected: CameraData *cameraData(const Camera *camera); + void prepareInternalBuffer(Buffer *buffer, Request *request, + BufferMemory *mem); CameraManager *manager_; private: diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp index 558b4b254d111e31..846272485c7d2fc0 100644 --- a/src/libcamera/pipeline_handler.cpp +++ b/src/libcamera/pipeline_handler.cpp @@ -485,6 +485,24 @@ void PipelineHandler::hotplugMediaDevice(MediaDevice *media) media->disconnected.connect(this, &PipelineHandler::mediaDeviceDisconnected); } +/** + * \brief Prepare buffer for internal usage by a pipeline handler + * \param[in,out] buffer The buffer to prepare + * \param[in] request The request to associate the \a buffer with + * \param[in] mem The memory to associate the \a buffer with + * + * Pipeline handlers creating internal buffers to facilitate data flow in the + * pipeline need to prepare the buffers by setting up the buffer object state. + * This function help pipeline handler implementations to perform this + * preparation. + */ +void PipelineHandler::prepareInternalBuffer(Buffer *buffer, Request *request, + BufferMemory *mem) +{ + buffer->request_ = request; + buffer->mem_ = mem; +} + /** * \brief Slot for the MediaDevice disconnected signal */
Buffers internal to a pipeline handler (buffers cycled between a CSI-2 pipeline to a ISP pipeline, parameters and statistics buffers) needs to be prepared before they can be properly used inside a pipeline handler. At this point the preparation consists of mapping the Buffer objects memory and associating it with a request. Instead of adding this helper on the Buffer object itself add it to the pipeline handler base class to prevent it from being exposed to applications. Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> --- src/libcamera/include/pipeline_handler.h | 2 ++ src/libcamera/pipeline_handler.cpp | 18 ++++++++++++++++++ 2 files changed, 20 insertions(+)