[libcamera-devel,05/13] libcamera: pipeline: rkisp1: Create RkISP1Frames from camera data
diff mbox series

Message ID 20200813005246.3265807-6-niklas.soderlund@ragnatech.se
State Accepted
Headers show
Series
  • libcamera: pipeline: rkisp1: Extend to support two streams
Related show

Commit Message

Niklas Söderlund Aug. 13, 2020, 12:52 a.m. UTC 
Create RkISP1Frames from camera data instead of picking information out
form it. This is done to prepare for multi stream support where more
information from the camera data will be needed.

Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
---
 src/libcamera/pipeline/rkisp1/rkisp1.cpp | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

Comments

Jacopo Mondi Aug. 20, 2020, 8:42 a.m. UTC | #1 
Hi Niklas,

On Thu, Aug 13, 2020 at 02:52:38AM +0200, Niklas Söderlund wrote:
> Create RkISP1Frames from camera data instead of picking information out
> form it. This is done to prepare for multi stream support where more
> information from the camera data will be needed.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> ---
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index a1cda12d244f2d97..9d5a52d352aefabc 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -52,6 +52,7 @@ static const std::array<PixelFormat, 7> RKISP1_RSZ_MP_FORMATS{
>
>  class PipelineHandlerRkISP1;
>  class RkISP1ActionQueueBuffers;
> +class RkISP1CameraData;
>
>  enum RkISP1ActionType {
>  	SetSensor,
> @@ -77,7 +78,7 @@ class RkISP1Frames
>  public:
>  	RkISP1Frames(PipelineHandler *pipe);
>
> -	RkISP1FrameInfo *create(unsigned int frame, Request *request, Stream *stream);
> +	RkISP1FrameInfo *create(const RkISP1CameraData *data, Request *request);

Should data be a const & ?

This apart, the patch looks good
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
  j

>  	int destroy(unsigned int frame);
>  	void clear();
>
> @@ -242,8 +243,10 @@ RkISP1Frames::RkISP1Frames(PipelineHandler *pipe)
>  {
>  }
>
> -RkISP1FrameInfo *RkISP1Frames::create(unsigned int frame, Request *request, Stream *stream)
> +RkISP1FrameInfo *RkISP1Frames::create(const RkISP1CameraData *data, Request *request)
>  {
> +	unsigned int frame = data->frame_;
> +
>  	if (pipe_->availableParamBuffers_.empty()) {
>  		LOG(RkISP1, Error) << "Parameters buffer underrun";
>  		return nullptr;
> @@ -256,7 +259,7 @@ RkISP1FrameInfo *RkISP1Frames::create(unsigned int frame, Request *request, Stre
>  	}
>  	FrameBuffer *statBuffer = pipe_->availableStatBuffers_.front();
>
> -	FrameBuffer *videoBuffer = request->findBuffer(stream);
> +	FrameBuffer *videoBuffer = request->findBuffer(&data->stream_);
>  	if (!videoBuffer) {
>  		LOG(RkISP1, Error)
>  			<< "Attempt to queue request with invalid stream";
> @@ -880,10 +883,8 @@ void PipelineHandlerRkISP1::stop(Camera *camera)
>  int PipelineHandlerRkISP1::queueRequestDevice(Camera *camera, Request *request)
>  {
>  	RkISP1CameraData *data = cameraData(camera);
> -	Stream *stream = &data->stream_;
>
> -	RkISP1FrameInfo *info = data->frameInfo_.create(data->frame_, request,
> -							stream);
> +	RkISP1FrameInfo *info = data->frameInfo_.create(data, request);
>  	if (!info)
>  		return -ENOENT;
>
> --
> 2.28.0
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Laurent Pinchart Aug. 20, 2020, 3:02 p.m. UTC | #2 
Hi Niklas,

Thank you for the patch.

On Thu, Aug 20, 2020 at 10:42:27AM +0200, Jacopo Mondi wrote:
> On Thu, Aug 13, 2020 at 02:52:38AM +0200, Niklas Söderlund wrote:
> > Create RkISP1Frames from camera data instead of picking information out
> > form it. This is done to prepare for multi stream support where more

s/form/from/

> > information from the camera data will be needed.
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > ---
> >  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 13 +++++++------
> >  1 file changed, 7 insertions(+), 6 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > index a1cda12d244f2d97..9d5a52d352aefabc 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > @@ -52,6 +52,7 @@ static const std::array<PixelFormat, 7> RKISP1_RSZ_MP_FORMATS{
> >
> >  class PipelineHandlerRkISP1;
> >  class RkISP1ActionQueueBuffers;
> > +class RkISP1CameraData;
> >
> >  enum RkISP1ActionType {
> >  	SetSensor,
> > @@ -77,7 +78,7 @@ class RkISP1Frames
> >  public:
> >  	RkISP1Frames(PipelineHandler *pipe);
> >
> > -	RkISP1FrameInfo *create(unsigned int frame, Request *request, Stream *stream);
> > +	RkISP1FrameInfo *create(const RkISP1CameraData *data, Request *request);
> 
> Should data be a const & ?
> 
> This apart, the patch looks good
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
>
> >  	int destroy(unsigned int frame);
> >  	void clear();
> >
> > @@ -242,8 +243,10 @@ RkISP1Frames::RkISP1Frames(PipelineHandler *pipe)
> >  {
> >  }
> >
> > -RkISP1FrameInfo *RkISP1Frames::create(unsigned int frame, Request *request, Stream *stream)
> > +RkISP1FrameInfo *RkISP1Frames::create(const RkISP1CameraData *data, Request *request)
> >  {
> > +	unsigned int frame = data->frame_;
> > +

You could do away with the local variable by using data->frame_ directly
if desired.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> >  	if (pipe_->availableParamBuffers_.empty()) {
> >  		LOG(RkISP1, Error) << "Parameters buffer underrun";
> >  		return nullptr;
> > @@ -256,7 +259,7 @@ RkISP1FrameInfo *RkISP1Frames::create(unsigned int frame, Request *request, Stre
> >  	}
> >  	FrameBuffer *statBuffer = pipe_->availableStatBuffers_.front();
> >
> > -	FrameBuffer *videoBuffer = request->findBuffer(stream);
> > +	FrameBuffer *videoBuffer = request->findBuffer(&data->stream_);
> >  	if (!videoBuffer) {
> >  		LOG(RkISP1, Error)
> >  			<< "Attempt to queue request with invalid stream";
> > @@ -880,10 +883,8 @@ void PipelineHandlerRkISP1::stop(Camera *camera)
> >  int PipelineHandlerRkISP1::queueRequestDevice(Camera *camera, Request *request)
> >  {
> >  	RkISP1CameraData *data = cameraData(camera);
> > -	Stream *stream = &data->stream_;
> >
> > -	RkISP1FrameInfo *info = data->frameInfo_.create(data->frame_, request,
> > -							stream);
> > +	RkISP1FrameInfo *info = data->frameInfo_.create(data, request);
> >  	if (!info)
> >  		return -ENOENT;
> >
Niklas Söderlund Sept. 14, 2020, 9:31 a.m. UTC | #3 
Hi Jacopo,

On 2020-08-20 10:42:27 +0200, Jacopo Mondi wrote:
> Hi Niklas,
> 
> On Thu, Aug 13, 2020 at 02:52:38AM +0200, Niklas Söderlund wrote:
> > Create RkISP1Frames from camera data instead of picking information out
> > form it. This is done to prepare for multi stream support where more
> > information from the camera data will be needed.
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > ---
> >  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 13 +++++++------
> >  1 file changed, 7 insertions(+), 6 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > index a1cda12d244f2d97..9d5a52d352aefabc 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > @@ -52,6 +52,7 @@ static const std::array<PixelFormat, 7> RKISP1_RSZ_MP_FORMATS{
> >
> >  class PipelineHandlerRkISP1;
> >  class RkISP1ActionQueueBuffers;
> > +class RkISP1CameraData;
> >
> >  enum RkISP1ActionType {
> >  	SetSensor,
> > @@ -77,7 +78,7 @@ class RkISP1Frames
> >  public:
> >  	RkISP1Frames(PipelineHandler *pipe);
> >
> > -	RkISP1FrameInfo *create(unsigned int frame, Request *request, Stream *stream);
> > +	RkISP1FrameInfo *create(const RkISP1CameraData *data, Request *request);
> 
> Should data be a const & ?

It could but camera data is a pointer in all other places and I see no 
harm in keeping it the same here.

> 
> This apart, the patch looks good
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> 
> Thanks
>   j
> 
> >  	int destroy(unsigned int frame);
> >  	void clear();
> >
> > @@ -242,8 +243,10 @@ RkISP1Frames::RkISP1Frames(PipelineHandler *pipe)
> >  {
> >  }
> >
> > -RkISP1FrameInfo *RkISP1Frames::create(unsigned int frame, Request *request, Stream *stream)
> > +RkISP1FrameInfo *RkISP1Frames::create(const RkISP1CameraData *data, Request *request)
> >  {
> > +	unsigned int frame = data->frame_;
> > +
> >  	if (pipe_->availableParamBuffers_.empty()) {
> >  		LOG(RkISP1, Error) << "Parameters buffer underrun";
> >  		return nullptr;
> > @@ -256,7 +259,7 @@ RkISP1FrameInfo *RkISP1Frames::create(unsigned int frame, Request *request, Stre
> >  	}
> >  	FrameBuffer *statBuffer = pipe_->availableStatBuffers_.front();
> >
> > -	FrameBuffer *videoBuffer = request->findBuffer(stream);
> > +	FrameBuffer *videoBuffer = request->findBuffer(&data->stream_);
> >  	if (!videoBuffer) {
> >  		LOG(RkISP1, Error)
> >  			<< "Attempt to queue request with invalid stream";
> > @@ -880,10 +883,8 @@ void PipelineHandlerRkISP1::stop(Camera *camera)
> >  int PipelineHandlerRkISP1::queueRequestDevice(Camera *camera, Request *request)
> >  {
> >  	RkISP1CameraData *data = cameraData(camera);
> > -	Stream *stream = &data->stream_;
> >
> > -	RkISP1FrameInfo *info = data->frameInfo_.create(data->frame_, request,
> > -							stream);
> > +	RkISP1FrameInfo *info = data->frameInfo_.create(data, request);
> >  	if (!info)
> >  		return -ENOENT;
> >
> > --
> > 2.28.0
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel@lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel

Patch
diff mbox series 

diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
index a1cda12d244f2d97..9d5a52d352aefabc 100644
--- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
+++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
@@ -52,6 +52,7 @@  static const std::array<PixelFormat, 7> RKISP1_RSZ_MP_FORMATS{
 
 class PipelineHandlerRkISP1;
 class RkISP1ActionQueueBuffers;
+class RkISP1CameraData;
 
 enum RkISP1ActionType {
 	SetSensor,
@@ -77,7 +78,7 @@  class RkISP1Frames
 public:
 	RkISP1Frames(PipelineHandler *pipe);
 
-	RkISP1FrameInfo *create(unsigned int frame, Request *request, Stream *stream);
+	RkISP1FrameInfo *create(const RkISP1CameraData *data, Request *request);
 	int destroy(unsigned int frame);
 	void clear();
 
@@ -242,8 +243,10 @@  RkISP1Frames::RkISP1Frames(PipelineHandler *pipe)
 {
 }
 
-RkISP1FrameInfo *RkISP1Frames::create(unsigned int frame, Request *request, Stream *stream)
+RkISP1FrameInfo *RkISP1Frames::create(const RkISP1CameraData *data, Request *request)
 {
+	unsigned int frame = data->frame_;
+
 	if (pipe_->availableParamBuffers_.empty()) {
 		LOG(RkISP1, Error) << "Parameters buffer underrun";
 		return nullptr;
@@ -256,7 +259,7 @@  RkISP1FrameInfo *RkISP1Frames::create(unsigned int frame, Request *request, Stre
 	}
 	FrameBuffer *statBuffer = pipe_->availableStatBuffers_.front();
 
-	FrameBuffer *videoBuffer = request->findBuffer(stream);
+	FrameBuffer *videoBuffer = request->findBuffer(&data->stream_);
 	if (!videoBuffer) {
 		LOG(RkISP1, Error)
 			<< "Attempt to queue request with invalid stream";
@@ -880,10 +883,8 @@  void PipelineHandlerRkISP1::stop(Camera *camera)
 int PipelineHandlerRkISP1::queueRequestDevice(Camera *camera, Request *request)
 {
 	RkISP1CameraData *data = cameraData(camera);
-	Stream *stream = &data->stream_;
 
-	RkISP1FrameInfo *info = data->frameInfo_.create(data->frame_, request,
-							stream);
+	RkISP1FrameInfo *info = data->frameInfo_.create(data, request);
 	if (!info)
 		return -ENOENT;