| Message ID | 20191028022224.795355-9-niklas.soderlund@ragnatech.se |
|---|---|
| State | Superseded |
| Delegated to: | Niklas Söderlund |
| Headers | show |
| Series |
|
| Related | show |
Hi Niklas,
thanks 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
> points in different pipeline handlers. Align where they are called to
> make it easier to read different pipeline implementations.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund@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;
I wonder if, since we are now queueing the request if no buffer
queuing fails, isn't it better to return immediately as soon as
int ret = stream->device_->dev->queueBuffer(buffer);
fails, in the for() loop ?
>
> - return error;
> + return PipelineHandler::queueRequest(camera, request);
Here and in all below parts: PipelineHandler::queueRequest() returns 0
unconditionally. As of now this patch does not change anything, but
maybe in future we'll made queueRequest() return a real value, so it
might be good to have it like this already ?
Thanks
j
> }
>
> 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);
> }
>
> /* -----------------------------------------------------------------------------
> 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);
> }
>
> 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)
> --
> 2.23.0
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
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@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. > } > > /* ----------------------------------------------------------------------------- > 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. I'm not opposed to this patch, if other developers think it should be merged, in its current form, let's do it. > } > > 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)
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@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
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; - 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); } /* ----------------------------------------------------------------------------- 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); } 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)
The PipelineHandler base class queueRequest() where called at different points in different pipeline handlers. Align where they are called to make it easier to read different pipeline implementations. Signed-off-by: Niklas Söderlund <niklas.soderlund@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(-)