[libcamera-devel] [PATCH 08/10] libcamera: pipelines: Align where to call base class queueRequest()

[Niklas Söderlund]

Hi Jacopo and Laurent,

Thanks for your feedback.

On 2019-11-18 13:39:12 +0200, Laurent Pinchart wrote:
> Hi Niklas,
> 
> Thank you for the patch.
> 
> On Mon, Oct 28, 2019 at 03:22:22AM +0100, Niklas Söderlund wrote:
> > The PipelineHandler base class queueRequest() where called at different
> 
> s/where called/is called/ ?
> 
> > points in different pipeline handlers. Align where they are called to
> > make it easier to read different pipeline implementations.
> 
> s/pipeline/pipeline handler/
> 
> > 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > ---
> >  src/libcamera/pipeline/ipu3/ipu3.cpp     | 5 +++--
> >  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 4 +---
> >  src/libcamera/pipeline/uvcvideo.cpp      | 4 +---
> >  src/libcamera/pipeline/vimc.cpp          | 4 +---
> >  4 files changed, 6 insertions(+), 11 deletions(-)
> > 
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > index 8d3ad568d16e9f8f..06ee6ccac641eb41 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -768,9 +768,10 @@ int PipelineHandlerIPU3::queueRequest(Camera *camera, Request *request)
> >  			error = ret;
> >  	}
> >  
> > -	PipelineHandler::queueRequest(camera, request);
> > +	if (error)
> > +		return error;
> 
> Should we instead return an error immediately in the above loop instead
> of continuing ?
> 
> >  
> > -	return error;
> > +	return PipelineHandler::queueRequest(camera, request);
> >  }
> >  
> >  bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator)
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > index 7a28b03b8d38c8b9..a99df4f8b846d8b7 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > @@ -814,8 +814,6 @@ int PipelineHandlerRkISP1::queueRequest(Camera *camera, Request *request)
> >  	RkISP1CameraData *data = cameraData(camera);
> >  	Stream *stream = &data->stream_;
> >  
> > -	PipelineHandler::queueRequest(camera, request);
> > -
> >  	RkISP1FrameInfo *info = data->frameInfo_.create(data->frame_, request,
> >  							stream);
> >  	if (!info)
> > @@ -833,7 +831,7 @@ int PipelineHandlerRkISP1::queueRequest(Camera *camera, Request *request)
> >  
> >  	data->frame_++;
> >  
> > -	return 0;
> > +	return PipelineHandler::queueRequest(camera, request);
> 
> What if the request completes before PipelineHandler::queueRequest() is
> called ? This isn't really a problem today, but may become one later
> when we'll use threads in pipeline handlers. The queueRequest() method
> could be called from a thread different than the one running the
> pipeline handler event loop. I suppose there will be more issues to fix
> at that point, so maybe this change is OK for now.

Good point, will fix in next version.

> 
> >  }
> >  
> >  /* -----------------------------------------------------------------------------
> > diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp
> > index fae0ffc4de30e1b7..dc17b456af6987e6 100644
> > --- a/src/libcamera/pipeline/uvcvideo.cpp
> > +++ b/src/libcamera/pipeline/uvcvideo.cpp
> > @@ -281,9 +281,7 @@ int PipelineHandlerUVC::queueRequest(Camera *camera, Request *request)
> >  	if (ret < 0)
> >  		return ret;
> >  
> > -	PipelineHandler::queueRequest(camera, request);
> > -
> > -	return 0;
> > +	return PipelineHandler::queueRequest(camera, request);
> 
> I'm slightly annoyed by this. It doesn't break anything, and forwards
> the error code from PipelineHandler::queueRequest(), which is good
> overall. However, PipelineHandler::queueRequest() never fails in its
> current implementation (which is why the code isn't currently broken),
> and if that were to change, we would need to handle the error here to
> undo the previous actions (which can't really be done, as a buffer
> queued to a V4L2 device can't be "unqueued"). The code is thus a bit
> fragile. I suppose this change doesn't make the situation worse, so we
> can address the issue later when it arises.

Good point will fix in v2.

> 
> I'm not opposed to this patch, if other developers think it should be
> merged, in its current form, let's do it.

The points above pointed out by both Jacopo and Laurent have made me 
reconsider this patch and take the issue in a new direction. I now 
believe the core of the problem to be specific pipeline handler 
implementations need to call it's base class equivalent in this case.

> 
> >  }
> >  
> >  bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator)
> > diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp
> > index c16ae4cb76b57e1c..5f83ae2b85997828 100644
> > --- a/src/libcamera/pipeline/vimc.cpp
> > +++ b/src/libcamera/pipeline/vimc.cpp
> > @@ -341,9 +341,7 @@ int PipelineHandlerVimc::queueRequest(Camera *camera, Request *request)
> >  	if (ret < 0)
> >  		return ret;
> >  
> > -	PipelineHandler::queueRequest(camera, request);
> > -
> > -	return 0;
> > +	return PipelineHandler::queueRequest(camera, request);
> >  }
> >  
> >  bool PipelineHandlerVimc::match(DeviceEnumerator *enumerator)
> 
> -- 
> Regards,
> 
> Laurent Pinchart

-- 
Regards,
Niklas Söderlund