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

23755 diff mbox series

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(-)

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)

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