| Message ID | 20220721121310.1286862-3-kieran.bingham@ideasonboard.com |
|---|---|
| State | Superseded |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series |
|
| Related | show |
Hi On 7/21/22 17:43, Kieran Bingham via libcamera-devel wrote: > Report an error when failing to process controls, but still allow the > request to process and complete where possible. > > The Request ControlError flag is raised on the request. > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=135 > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > index 53b2f23ab029..1f282f26bec3 100644 > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > @@ -26,6 +26,7 @@ > #include "libcamera/internal/device_enumerator.h" > #include "libcamera/internal/media_device.h" > #include "libcamera/internal/pipeline_handler.h" > +#include "libcamera/internal/request.h" > #include "libcamera/internal/sysfs.h" > #include "libcamera/internal/v4l2_videodevice.h" > > @@ -373,8 +374,10 @@ int PipelineHandlerUVC::queueRequestDevice(Camera *camera, Request *request) > } > > int ret = processControls(data, request); I kept wondering why we are processing controls here (in PH) and not in IPA, to later realize UVC has no IPA ;-) > - if (ret < 0) > - return ret; > + if (ret < 0) { > + LOG(UVC, Error) << "Failed to process controls"; > + request->_d()->setErrorFlags(Request::ControlError); > + } Makes sense and as per design. Reviewed-by: Umang Jain <umang.jain@ideasonboard.com> > > ret = data->video_->queueBuffer(buffer); > if (ret < 0)
Hi Kieran On Thu, Jul 21, 2022 at 01:13:00PM +0100, Kieran Bingham via libcamera-devel wrote: > Report an error when failing to process controls, but still allow the > request to process and complete where possible. > > The Request ControlError flag is raised on the request. > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=135 > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > index 53b2f23ab029..1f282f26bec3 100644 > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > @@ -26,6 +26,7 @@ > #include "libcamera/internal/device_enumerator.h" > #include "libcamera/internal/media_device.h" > #include "libcamera/internal/pipeline_handler.h" > +#include "libcamera/internal/request.h" > #include "libcamera/internal/sysfs.h" > #include "libcamera/internal/v4l2_videodevice.h" > > @@ -373,8 +374,10 @@ int PipelineHandlerUVC::queueRequestDevice(Camera *camera, Request *request) > } > > int ret = processControls(data, request); > - if (ret < 0) > - return ret; > + if (ret < 0) { > + LOG(UVC, Error) << "Failed to process controls"; > + request->_d()->setErrorFlags(Request::ControlError); I wonder if Request::Private::setError() wouldn't be enough (that's probably a comment for patch #1). With the confirmation that this patch introduces a behavioural change, as before we returned early (which I presume from the commit message the change is intended). Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Thanks j > + } > > ret = data->video_->queueBuffer(buffer); > if (ret < 0) > -- > 2.34.1 >
Quoting Jacopo Mondi (2022-07-25 08:56:59) > Hi Kieran > > On Thu, Jul 21, 2022 at 01:13:00PM +0100, Kieran Bingham via libcamera-devel wrote: > > Report an error when failing to process controls, but still allow the > > request to process and complete where possible. > > > > The Request ControlError flag is raised on the request. > > > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=135 > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > --- > > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 7 +++++-- > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > > index 53b2f23ab029..1f282f26bec3 100644 > > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > > @@ -26,6 +26,7 @@ > > #include "libcamera/internal/device_enumerator.h" > > #include "libcamera/internal/media_device.h" > > #include "libcamera/internal/pipeline_handler.h" > > +#include "libcamera/internal/request.h" > > #include "libcamera/internal/sysfs.h" > > #include "libcamera/internal/v4l2_videodevice.h" > > > > @@ -373,8 +374,10 @@ int PipelineHandlerUVC::queueRequestDevice(Camera *camera, Request *request) > > } > > > > int ret = processControls(data, request); > > - if (ret < 0) > > - return ret; > > + if (ret < 0) { > > + LOG(UVC, Error) << "Failed to process controls"; > > + request->_d()->setErrorFlags(Request::ControlError); > > I wonder if Request::Private::setError() wouldn't be enough (that's > probably a comment for patch #1). Do you mean 'drop the word Flags' here too ? > > With the confirmation that this patch introduces a behavioural change, > as before we returned early (which I presume from the commit message > the change is intended). Yes, this is very much intended behavioural change. > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Thanks > > Thanks > j > > > + } > > > > ret = data->video_->queueBuffer(buffer); > > if (ret < 0) > > -- > > 2.34.1 > >
diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp index 53b2f23ab029..1f282f26bec3 100644 --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp @@ -26,6 +26,7 @@ #include "libcamera/internal/device_enumerator.h" #include "libcamera/internal/media_device.h" #include "libcamera/internal/pipeline_handler.h" +#include "libcamera/internal/request.h" #include "libcamera/internal/sysfs.h" #include "libcamera/internal/v4l2_videodevice.h" @@ -373,8 +374,10 @@ int PipelineHandlerUVC::queueRequestDevice(Camera *camera, Request *request) } int ret = processControls(data, request); - if (ret < 0) - return ret; + if (ret < 0) { + LOG(UVC, Error) << "Failed to process controls"; + request->_d()->setErrorFlags(Request::ControlError); + } ret = data->video_->queueBuffer(buffer); if (ret < 0)
Report an error when failing to process controls, but still allow the request to process and complete where possible. The Request ControlError flag is raised on the request. Bug: https://bugs.libcamera.org/show_bug.cgi?id=135 Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> --- src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)