[v3,6/9] pipeline: rkisp1: Add error log when parameter queuing fails
diff mbox series

Message ID 20250707085520.39777-7-stefan.klug@ideasonboard.com
State New
Headers show
Series
  • Wdr preparations
Related show

Commit Message

Stefan Klug July 7, 2025, 8:55 a.m. UTC 
Add a error statement when quieing of the parameter buffer fails for
whatever reason.

Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>

---

Changes in v3:
- Collected tags
- Removed hint regarding unsupported parameter types as this will be
  handled using the now upstreamed RKISP1_CID_SUPPORTED_PARAMS_BLOCKS.

Changes in v2:
- Also print the error code in case of failure
---
 src/libcamera/pipeline/rkisp1/rkisp1.cpp | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

Comments

Laurent Pinchart July 7, 2025, 1:04 p.m. UTC | #1 
On Mon, Jul 07, 2025 at 10:55:09AM +0200, Stefan Klug wrote:
> Add a error statement when quieing of the parameter buffer fails for

s/quieing/

> whatever reason.

Commit messages need to explain first and foremost *why* a change is
needed.

> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
> 
> ---
> 
> Changes in v3:
> - Collected tags
> - Removed hint regarding unsupported parameter types as this will be
>   handled using the now upstreamed RKISP1_CID_SUPPORTED_PARAMS_BLOCKS.
> 
> Changes in v2:
> - Also print the error code in case of failure
> ---
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 675f0a7490a6..d5bb8aced3c8 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -411,7 +411,14 @@ void RkISP1CameraData::paramsComputed(unsigned int frame, unsigned int bytesused
>  		return;
>  
>  	info->paramBuffer->_d()->metadata().planes()[0].bytesused = bytesused;
> -	pipe->param_->queueBuffer(info->paramBuffer);
> +
> +	int ret = pipe->param_->queueBuffer(info->paramBuffer);
> +	if (ret < 0) {
> +		LOG(RkISP1, Error) << "Failed to queue parameter buffer: "
> +				   << strerror(-ret);
> +		return;

This seems very fatal, if we can't queue the params buffer, everything
will fall apart. I think we should in that case stop the camera and
return all requests as cancelled.

> +	}
> +
>  	pipe->stat_->queueBuffer(info->statBuffer);

In particular, I wonder why the check is needed for the param_ device
but not the stat_ device.

>  
>  	if (info->mainPathBuffer)
Stefan Klug July 7, 2025, 1:48 p.m. UTC | #2 
Hi Laurent,

Thanks for the comments.

Quoting Laurent Pinchart (2025-07-07 15:04:49)
> On Mon, Jul 07, 2025 at 10:55:09AM +0200, Stefan Klug wrote:
> > Add a error statement when quieing of the parameter buffer fails for
> 
> s/quieing/
> 
> > whatever reason.
> 
> Commit messages need to explain first and foremost *why* a change is
> needed.

Do we really need a reason to log an error when something fails? If we
only look at upstream and proper setups, this error condition will
possibly never occur again. I can't tell. But I wish I would have had
this error message when I hit the error in first place. My take on that
is that every call to the outside should be error checked and if
something unexpected happens we should log it. That maybe a bit on the
defensive side but in this case I was actually bitten.

> 
> > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
> > 
> > ---
> > 
> > Changes in v3:
> > - Collected tags
> > - Removed hint regarding unsupported parameter types as this will be
> >   handled using the now upstreamed RKISP1_CID_SUPPORTED_PARAMS_BLOCKS.
> > 
> > Changes in v2:
> > - Also print the error code in case of failure
> > ---
> >  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 9 ++++++++-
> >  1 file changed, 8 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > index 675f0a7490a6..d5bb8aced3c8 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > @@ -411,7 +411,14 @@ void RkISP1CameraData::paramsComputed(unsigned int frame, unsigned int bytesused
> >               return;
> >  
> >       info->paramBuffer->_d()->metadata().planes()[0].bytesused = bytesused;
> > -     pipe->param_->queueBuffer(info->paramBuffer);
> > +
> > +     int ret = pipe->param_->queueBuffer(info->paramBuffer);
> > +     if (ret < 0) {
> > +             LOG(RkISP1, Error) << "Failed to queue parameter buffer: "
> > +                                << strerror(-ret);
> > +             return;
> 
> This seems very fatal, if we can't queue the params buffer, everything
> will fall apart. I think we should in that case stop the camera and
> return all requests as cancelled.

Is there an easy way to do this? We could also replace Error by Fatal
and exit out as this is really an unexpected condition.

> 
> > +     }
> > +
> >       pipe->stat_->queueBuffer(info->statBuffer);
> 
> In particular, I wonder why the check is needed for the param_ device
> but not the stat_ device.

It should actually be added. It just didn't go wrong in the work I did
and therefore wasn't added.

Best regards,
Stefan

> 
> >  
> >       if (info->mainPathBuffer)
> 
> -- 
> Regards,
> 
> Laurent Pinchart
Laurent Pinchart July 23, 2025, 3:50 p.m. UTC | #3 
On Mon, Jul 07, 2025 at 03:48:40PM +0200, Stefan Klug wrote:
> Quoting Laurent Pinchart (2025-07-07 15:04:49)
> > On Mon, Jul 07, 2025 at 10:55:09AM +0200, Stefan Klug wrote:
> > > Add a error statement when quieing of the parameter buffer fails for
> > 
> > s/quieing/
> > 
> > > whatever reason.
> > 
> > Commit messages need to explain first and foremost *why* a change is
> > needed.
> 
> Do we really need a reason to log an error when something fails? If we
> only look at upstream and proper setups, this error condition will
> possibly never occur again. I can't tell. But I wish I would have had
> this error message when I hit the error in first place. My take on that
> is that every call to the outside should be error checked and if
> something unexpected happens we should log it. That maybe a bit on the
> defensive side but in this case I was actually bitten.

I'm not pushing back against this patch, the comment was about the fact
commit messages need to explain the rationale for a change. What may be
evident to you could not be so for someone else.

> > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
> > > 
> > > ---
> > > 
> > > Changes in v3:
> > > - Collected tags
> > > - Removed hint regarding unsupported parameter types as this will be
> > >   handled using the now upstreamed RKISP1_CID_SUPPORTED_PARAMS_BLOCKS.
> > > 
> > > Changes in v2:
> > > - Also print the error code in case of failure
> > > ---
> > >  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 9 ++++++++-
> > >  1 file changed, 8 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > index 675f0a7490a6..d5bb8aced3c8 100644
> > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > @@ -411,7 +411,14 @@ void RkISP1CameraData::paramsComputed(unsigned int frame, unsigned int bytesused
> > >               return;
> > >  
> > >       info->paramBuffer->_d()->metadata().planes()[0].bytesused = bytesused;
> > > -     pipe->param_->queueBuffer(info->paramBuffer);
> > > +
> > > +     int ret = pipe->param_->queueBuffer(info->paramBuffer);
> > > +     if (ret < 0) {
> > > +             LOG(RkISP1, Error) << "Failed to queue parameter buffer: "
> > > +                                << strerror(-ret);
> > > +             return;
> > 
> > This seems very fatal, if we can't queue the params buffer, everything
> > will fall apart. I think we should in that case stop the camera and
> > return all requests as cancelled.
> 
> Is there an easy way to do this? We could also replace Error by Fatal
> and exit out as this is really an unexpected condition.

It should be done on top, I don't know how easy or difficult it would
be. If it's difficult, we need helpers to make it easy :-)

> > > +     }
> > > +
> > >       pipe->stat_->queueBuffer(info->statBuffer);
> > 
> > In particular, I wonder why the check is needed for the param_ device
> > but not the stat_ device.
> 
> It should actually be added. It just didn't go wrong in the work I did
> and therefore wasn't added.
> 
> > >  
> > >       if (info->mainPathBuffer)

Patch
diff mbox series 

diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
index 675f0a7490a6..d5bb8aced3c8 100644
--- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
+++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
@@ -411,7 +411,14 @@  void RkISP1CameraData::paramsComputed(unsigned int frame, unsigned int bytesused
 		return;
 
 	info->paramBuffer->_d()->metadata().planes()[0].bytesused = bytesused;
-	pipe->param_->queueBuffer(info->paramBuffer);
+
+	int ret = pipe->param_->queueBuffer(info->paramBuffer);
+	if (ret < 0) {
+		LOG(RkISP1, Error) << "Failed to queue parameter buffer: "
+				   << strerror(-ret);
+		return;
+	}
+
 	pipe->stat_->queueBuffer(info->statBuffer);
 
 	if (info->mainPathBuffer)