| Message ID | 20191028022525.796995-4-niklas.soderlund@ragnatech.se |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Niklas, Thank you for the patch. On Mon, Oct 28, 2019 at 03:25:16AM +0100, Niklas Söderlund wrote: > The pipeline knows which buffer coming from the application belongs to > which request. Add a helper to allow pipeline implementations to access > this knowledge. > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> Why is this needed, given that Buffer stores a pointer to Request ? I assume I'll find out later in the series, but it seems more efficient to rely on the pointer. > --- > src/libcamera/include/pipeline_handler.h | 2 ++ > src/libcamera/pipeline_handler.cpp | 10 ++++++++++ > 2 files changed, 12 insertions(+) > > diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h > index 320740746bc6e998..6024357e266c2e2b 100644 > --- a/src/libcamera/include/pipeline_handler.h > +++ b/src/libcamera/include/pipeline_handler.h > @@ -39,6 +39,8 @@ public: > } > virtual ~CameraData() {} > > + Request *requestFromBuffer(Buffer *buffer); > + > Camera *camera_; > PipelineHandler *pipe_; > std::list<Request *> queuedRequests_; > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp > index f9ae767d529d44d9..d70e286661aded8e 100644 > --- a/src/libcamera/pipeline_handler.cpp > +++ b/src/libcamera/pipeline_handler.cpp > @@ -58,6 +58,16 @@ LOG_DEFINE_CATEGORY(Pipeline) > * exists. > */ > > +Request *CameraData::requestFromBuffer(Buffer *buffer) > +{ > + for (Request *request : queuedRequests_) > + for (const auto &it : request->buffers()) > + if (it.second == buffer) > + return request; > + > + return nullptr; > +} > + > /** > * \var CameraData::camera_ > * \brief The camera related to this CameraData instance
Hi Laurent, Thanks for your feedback. On 2019-11-18 21:14:42 +0200, Laurent Pinchart wrote: > Hi Niklas, > > Thank you for the patch. > > On Mon, Oct 28, 2019 at 03:25:16AM +0100, Niklas Söderlund wrote: > > The pipeline knows which buffer coming from the application belongs to > > which request. Add a helper to allow pipeline implementations to access > > this knowledge. > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > Why is this needed, given that Buffer stores a pointer to Request ? I > assume I'll find out later in the series, but it seems more efficient to > rely on the pointer. This is needed as the Buffer will no longer keep a reference to a Request. > > > --- > > src/libcamera/include/pipeline_handler.h | 2 ++ > > src/libcamera/pipeline_handler.cpp | 10 ++++++++++ > > 2 files changed, 12 insertions(+) > > > > diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h > > index 320740746bc6e998..6024357e266c2e2b 100644 > > --- a/src/libcamera/include/pipeline_handler.h > > +++ b/src/libcamera/include/pipeline_handler.h > > @@ -39,6 +39,8 @@ public: > > } > > virtual ~CameraData() {} > > > > + Request *requestFromBuffer(Buffer *buffer); > > + > > Camera *camera_; > > PipelineHandler *pipe_; > > std::list<Request *> queuedRequests_; > > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp > > index f9ae767d529d44d9..d70e286661aded8e 100644 > > --- a/src/libcamera/pipeline_handler.cpp > > +++ b/src/libcamera/pipeline_handler.cpp > > @@ -58,6 +58,16 @@ LOG_DEFINE_CATEGORY(Pipeline) > > * exists. > > */ > > > > +Request *CameraData::requestFromBuffer(Buffer *buffer) > > +{ > > + for (Request *request : queuedRequests_) > > + for (const auto &it : request->buffers()) > > + if (it.second == buffer) > > + return request; > > + > > + return nullptr; > > +} > > + > > /** > > * \var CameraData::camera_ > > * \brief The camera related to this CameraData instance > > -- > Regards, > > Laurent Pinchart
Hi Niklas, On Wed, Nov 20, 2019 at 05:33:56PM +0100, Niklas Söderlund wrote: > On 2019-11-18 21:14:42 +0200, Laurent Pinchart wrote: > > On Mon, Oct 28, 2019 at 03:25:16AM +0100, Niklas Söderlund wrote: > > > The pipeline knows which buffer coming from the application belongs to > > > which request. Add a helper to allow pipeline implementations to access > > > this knowledge. > > > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > > > Why is this needed, given that Buffer stores a pointer to Request ? I > > assume I'll find out later in the series, but it seems more efficient to > > rely on the pointer. > > This is needed as the Buffer will no longer keep a reference to a > Request. But why can't it keep a reference to a request ? I understand the Buffer object won't be transient anymore, but can't we still have a request pointer, that will only be valid from the time the buffer is added to the request to the time the request completes ? As long as we document the lifetime of that pointer, I think it should be fine. > > > --- > > > src/libcamera/include/pipeline_handler.h | 2 ++ > > > src/libcamera/pipeline_handler.cpp | 10 ++++++++++ > > > 2 files changed, 12 insertions(+) > > > > > > diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h > > > index 320740746bc6e998..6024357e266c2e2b 100644 > > > --- a/src/libcamera/include/pipeline_handler.h > > > +++ b/src/libcamera/include/pipeline_handler.h > > > @@ -39,6 +39,8 @@ public: > > > } > > > virtual ~CameraData() {} > > > > > > + Request *requestFromBuffer(Buffer *buffer); > > > + > > > Camera *camera_; > > > PipelineHandler *pipe_; > > > std::list<Request *> queuedRequests_; > > > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp > > > index f9ae767d529d44d9..d70e286661aded8e 100644 > > > --- a/src/libcamera/pipeline_handler.cpp > > > +++ b/src/libcamera/pipeline_handler.cpp > > > @@ -58,6 +58,16 @@ LOG_DEFINE_CATEGORY(Pipeline) > > > * exists. > > > */ > > > > > > +Request *CameraData::requestFromBuffer(Buffer *buffer) > > > +{ > > > + for (Request *request : queuedRequests_) > > > + for (const auto &it : request->buffers()) > > > + if (it.second == buffer) > > > + return request; > > > + > > > + return nullptr; > > > +} > > > + > > > /** > > > * \var CameraData::camera_ > > > * \brief The camera related to this CameraData instance
Hi Laurent, On 2019-11-21 05:11:53 +0200, Laurent Pinchart wrote: > Hi Niklas, > > On Wed, Nov 20, 2019 at 05:33:56PM +0100, Niklas Söderlund wrote: > > On 2019-11-18 21:14:42 +0200, Laurent Pinchart wrote: > > > On Mon, Oct 28, 2019 at 03:25:16AM +0100, Niklas Söderlund wrote: > > > > The pipeline knows which buffer coming from the application belongs to > > > > which request. Add a helper to allow pipeline implementations to access > > > > this knowledge. > > > > > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > > > > > Why is this needed, given that Buffer stores a pointer to Request ? I > > > assume I'll find out later in the series, but it seems more efficient to > > > rely on the pointer. > > > > This is needed as the Buffer will no longer keep a reference to a > > Request. > > But why can't it keep a reference to a request ? I understand the Buffer > object won't be transient anymore, but can't we still have a request > pointer, that will only be valid from the time the buffer is added to > the request to the time the request completes ? As long as we document > the lifetime of that pointer, I think it should be fine. You convinced me, I have restored the request reference in the next version. > > > > > --- > > > > src/libcamera/include/pipeline_handler.h | 2 ++ > > > > src/libcamera/pipeline_handler.cpp | 10 ++++++++++ > > > > 2 files changed, 12 insertions(+) > > > > > > > > diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h > > > > index 320740746bc6e998..6024357e266c2e2b 100644 > > > > --- a/src/libcamera/include/pipeline_handler.h > > > > +++ b/src/libcamera/include/pipeline_handler.h > > > > @@ -39,6 +39,8 @@ public: > > > > } > > > > virtual ~CameraData() {} > > > > > > > > + Request *requestFromBuffer(Buffer *buffer); > > > > + > > > > Camera *camera_; > > > > PipelineHandler *pipe_; > > > > std::list<Request *> queuedRequests_; > > > > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp > > > > index f9ae767d529d44d9..d70e286661aded8e 100644 > > > > --- a/src/libcamera/pipeline_handler.cpp > > > > +++ b/src/libcamera/pipeline_handler.cpp > > > > @@ -58,6 +58,16 @@ LOG_DEFINE_CATEGORY(Pipeline) > > > > * exists. > > > > */ > > > > > > > > +Request *CameraData::requestFromBuffer(Buffer *buffer) > > > > +{ > > > > + for (Request *request : queuedRequests_) > > > > + for (const auto &it : request->buffers()) > > > > + if (it.second == buffer) > > > > + return request; > > > > + > > > > + return nullptr; > > > > +} > > > > + > > > > /** > > > > * \var CameraData::camera_ > > > > * \brief The camera related to this CameraData instance > > -- > Regards, > > Laurent Pinchart
diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h index 320740746bc6e998..6024357e266c2e2b 100644 --- a/src/libcamera/include/pipeline_handler.h +++ b/src/libcamera/include/pipeline_handler.h @@ -39,6 +39,8 @@ public: } virtual ~CameraData() {} + Request *requestFromBuffer(Buffer *buffer); + Camera *camera_; PipelineHandler *pipe_; std::list<Request *> queuedRequests_; diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp index f9ae767d529d44d9..d70e286661aded8e 100644 --- a/src/libcamera/pipeline_handler.cpp +++ b/src/libcamera/pipeline_handler.cpp @@ -58,6 +58,16 @@ LOG_DEFINE_CATEGORY(Pipeline) * exists. */ +Request *CameraData::requestFromBuffer(Buffer *buffer) +{ + for (Request *request : queuedRequests_) + for (const auto &it : request->buffers()) + if (it.second == buffer) + return request; + + return nullptr; +} + /** * \var CameraData::camera_ * \brief The camera related to this CameraData instance
The pipeline knows which buffer coming from the application belongs to which request. Add a helper to allow pipeline implementations to access this knowledge. Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> --- src/libcamera/include/pipeline_handler.h | 2 ++ src/libcamera/pipeline_handler.cpp | 10 ++++++++++ 2 files changed, 12 insertions(+)