[libcamera-devel,RFC,03/12] libcamera: pipeline: Add helper to find request from buffer

Message ID 20191028022525.796995-4-niklas.soderlund@ragnatech.se
State Superseded
Headers show
Series
  • libcamera: Rework buffer API
Related show

Commit Message

Niklas Söderlund Oct. 28, 2019, 2:25 a.m. UTC
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(+)

Comments

Laurent Pinchart Nov. 18, 2019, 7:14 p.m. UTC | #1
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
Niklas Söderlund Nov. 20, 2019, 4:33 p.m. UTC | #2
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
Laurent Pinchart Nov. 21, 2019, 3:11 a.m. UTC | #3
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
Niklas Söderlund Nov. 21, 2019, 9:04 p.m. UTC | #4
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

Patch

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