[6/8] media: rkisp1: Propagate pre/post-config errors

  The support for the extensible parameters format introduces the
possibility of failures in handling the parameters buffer.

Errors in parsing the configuration parameters are not propagated
to the rkisp1_config_isp() and the rkisp1_isp_start() functions.

Propagate any possible errors to the callers to report it to userspace.

Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
---
 .../media/platform/rockchip/rkisp1/rkisp1-common.h | 10 +++++-----
 .../media/platform/rockchip/rkisp1/rkisp1-isp.c    | 14 +++++++++-----
 .../media/platform/rockchip/rkisp1/rkisp1-params.c | 14 +++++++++-----
 3 files changed, 23 insertions(+), 15 deletions(-)
  

Hi Jacopo - thanks for the patch. I think this probably should come before 5/8 in the series, and 
just hardcode return 0 in rkisp1_params_pre/post_configure() temporarily.

On 05/06/2024 17:54, Jacopo Mondi wrote:
> The support for the extensible parameters format introduces the
> possibility of failures in handling the parameters buffer.
>
> Errors in parsing the configuration parameters are not propagated
> to the rkisp1_config_isp() and the rkisp1_isp_start() functions.
>
> Propagate any possible errors to the callers to report it to userspace.
>
> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> ---
>   .../media/platform/rockchip/rkisp1/rkisp1-common.h | 10 +++++-----
>   .../media/platform/rockchip/rkisp1/rkisp1-isp.c    | 14 +++++++++-----
>   .../media/platform/rockchip/rkisp1/rkisp1-params.c | 14 +++++++++-----
>   3 files changed, 23 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> index 0bddae8dbdb1..f9df5ed96c98 100644
> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> @@ -591,10 +591,10 @@ const struct rkisp1_mbus_info *rkisp1_mbus_info_get_by_code(u32 mbus_code);
>    * It applies the initial ISP parameters from the first params buffer, but
>    * skips LSC as it needs to be configured after the ISP is started.
>    */
> -void rkisp1_params_pre_configure(struct rkisp1_params *params,
> -				 enum rkisp1_fmt_raw_pat_type bayer_pat,
> -				 enum v4l2_quantization quantization,
> -				 enum v4l2_ycbcr_encoding ycbcr_encoding);
> +int rkisp1_params_pre_configure(struct rkisp1_params *params,
> +				enum rkisp1_fmt_raw_pat_type bayer_pat,
> +				enum v4l2_quantization quantization,
> +				enum v4l2_ycbcr_encoding ycbcr_encoding);
>   
>   /*
>    * rkisp1_params_post_configure - Configure the params after stream start
> @@ -604,7 +604,7 @@ void rkisp1_params_pre_configure(struct rkisp1_params *params,
>    * This function is called by the ISP entity just after the ISP gets started.
>    * It applies the initial ISP LSC parameters from the first params buffer.
>    */
> -void rkisp1_params_post_configure(struct rkisp1_params *params);
> +int rkisp1_params_post_configure(struct rkisp1_params *params);
>   
>   /* rkisp1_params_disable - disable all parameters.
>    *			   This function is called by the isp entity upon stream start
> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
> index 91301d17d356..05227c6a16fe 100644
> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
> @@ -310,12 +310,16 @@ static int rkisp1_config_isp(struct rkisp1_isp *isp,
>   		rkisp1_params_disable(&rkisp1->params);
>   	} else {
>   		const struct v4l2_mbus_framefmt *src_frm;
> +		int ret;
>   
>   		src_frm = v4l2_subdev_state_get_format(sd_state,
>   						       RKISP1_ISP_PAD_SOURCE_VIDEO);
> -		rkisp1_params_pre_configure(&rkisp1->params, sink_fmt->bayer_pat,
> -					    src_frm->quantization,
> -					    src_frm->ycbcr_enc);
> +		ret = rkisp1_params_pre_configure(&rkisp1->params,
> +						  sink_fmt->bayer_pat,
> +						  src_frm->quantization,
> +						  src_frm->ycbcr_enc);
> +		if (ret)
> +			return ret;
>   	}
>   
>   	isp->sink_fmt = sink_fmt;
> @@ -458,9 +462,9 @@ static int rkisp1_isp_start(struct rkisp1_isp *isp,
>   	src_info = rkisp1_mbus_info_get_by_code(src_fmt->code);
>   
>   	if (src_info->pixel_enc != V4L2_PIXEL_ENC_BAYER)
> -		rkisp1_params_post_configure(&rkisp1->params);
> +		ret = rkisp1_params_post_configure(&rkisp1->params);
>   
> -	return 0;
> +	return ret;

I think ret could be returned uninitialised in some circumstances in this function now - if it's not 
the IMX8MP version and the pixel encoding is bayer...or am I missing something?

>   }
>   
>   /* ----------------------------------------------------------------------------
> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
> index 3d78e643d0b8..c081fd490b2b 100644
> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
> @@ -2123,10 +2123,10 @@ static const struct rkisp1_cif_isp_afc_config rkisp1_afc_params_default_config =
>   	14
>   };
>   
> -void rkisp1_params_pre_configure(struct rkisp1_params *params,
> -				 enum rkisp1_fmt_raw_pat_type bayer_pat,
> -				 enum v4l2_quantization quantization,
> -				 enum v4l2_ycbcr_encoding ycbcr_encoding)
> +int rkisp1_params_pre_configure(struct rkisp1_params *params,
> +				enum rkisp1_fmt_raw_pat_type bayer_pat,
> +				enum v4l2_quantization quantization,
> +				enum v4l2_ycbcr_encoding ycbcr_encoding)
>   {
>   	struct rkisp1_cif_isp_hst_config hst = rkisp1_hst_params_default_config;
>   	struct rkisp1_buffer *buf;
> @@ -2187,9 +2187,11 @@ void rkisp1_params_pre_configure(struct rkisp1_params *params,
>   
>   unlock:
>   	spin_unlock_irq(&params->config_lock);
> +
> +	return ret;
>   }
>   
> -void rkisp1_params_post_configure(struct rkisp1_params *params)
> +int rkisp1_params_post_configure(struct rkisp1_params *params)
>   {
>   	struct rkisp1_buffer *buf;
>   	int ret = 0;
> @@ -2227,6 +2229,8 @@ void rkisp1_params_post_configure(struct rkisp1_params *params)
>   
>   unlock:
>   	spin_unlock_irq(&params->config_lock);
> +
> +	return ret;
>   }
>   
>   /*
  

On Wed, Jun 12, 2024 at 02:35:39PM +0100, Daniel Scally wrote:
> Hi Jacopo - thanks for the patch. I think this probably should come
> before 5/8 in the series, and just hardcode return 0 in
> rkisp1_params_pre/post_configure() temporarily.

There should be no need to return errors from those two functions.
They're called at runtime to apply parameters. Validation of the
parameters should happen earlier, at qbuf time.

> On 05/06/2024 17:54, Jacopo Mondi wrote:
> > The support for the extensible parameters format introduces the
> > possibility of failures in handling the parameters buffer.
> >
> > Errors in parsing the configuration parameters are not propagated
> > to the rkisp1_config_isp() and the rkisp1_isp_start() functions.
> >
> > Propagate any possible errors to the callers to report it to userspace.
> >
> > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > ---
> >   .../media/platform/rockchip/rkisp1/rkisp1-common.h | 10 +++++-----
> >   .../media/platform/rockchip/rkisp1/rkisp1-isp.c    | 14 +++++++++-----
> >   .../media/platform/rockchip/rkisp1/rkisp1-params.c | 14 +++++++++-----
> >   3 files changed, 23 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> > index 0bddae8dbdb1..f9df5ed96c98 100644
> > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> > @@ -591,10 +591,10 @@ const struct rkisp1_mbus_info *rkisp1_mbus_info_get_by_code(u32 mbus_code);
> >    * It applies the initial ISP parameters from the first params buffer, but
> >    * skips LSC as it needs to be configured after the ISP is started.
> >    */
> > -void rkisp1_params_pre_configure(struct rkisp1_params *params,
> > -				 enum rkisp1_fmt_raw_pat_type bayer_pat,
> > -				 enum v4l2_quantization quantization,
> > -				 enum v4l2_ycbcr_encoding ycbcr_encoding);
> > +int rkisp1_params_pre_configure(struct rkisp1_params *params,
> > +				enum rkisp1_fmt_raw_pat_type bayer_pat,
> > +				enum v4l2_quantization quantization,
> > +				enum v4l2_ycbcr_encoding ycbcr_encoding);
> >   
> >   /*
> >    * rkisp1_params_post_configure - Configure the params after stream start
> > @@ -604,7 +604,7 @@ void rkisp1_params_pre_configure(struct rkisp1_params *params,
> >    * This function is called by the ISP entity just after the ISP gets started.
> >    * It applies the initial ISP LSC parameters from the first params buffer.
> >    */
> > -void rkisp1_params_post_configure(struct rkisp1_params *params);
> > +int rkisp1_params_post_configure(struct rkisp1_params *params);
> >   
> >   /* rkisp1_params_disable - disable all parameters.
> >    *			   This function is called by the isp entity upon stream start
> > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
> > index 91301d17d356..05227c6a16fe 100644
> > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
> > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
> > @@ -310,12 +310,16 @@ static int rkisp1_config_isp(struct rkisp1_isp *isp,
> >   		rkisp1_params_disable(&rkisp1->params);
> >   	} else {
> >   		const struct v4l2_mbus_framefmt *src_frm;
> > +		int ret;
> >   
> >   		src_frm = v4l2_subdev_state_get_format(sd_state,
> >   						       RKISP1_ISP_PAD_SOURCE_VIDEO);
> > -		rkisp1_params_pre_configure(&rkisp1->params, sink_fmt->bayer_pat,
> > -					    src_frm->quantization,
> > -					    src_frm->ycbcr_enc);
> > +		ret = rkisp1_params_pre_configure(&rkisp1->params,
> > +						  sink_fmt->bayer_pat,
> > +						  src_frm->quantization,
> > +						  src_frm->ycbcr_enc);
> > +		if (ret)
> > +			return ret;
> >   	}
> >   
> >   	isp->sink_fmt = sink_fmt;
> > @@ -458,9 +462,9 @@ static int rkisp1_isp_start(struct rkisp1_isp *isp,
> >   	src_info = rkisp1_mbus_info_get_by_code(src_fmt->code);
> >   
> >   	if (src_info->pixel_enc != V4L2_PIXEL_ENC_BAYER)
> > -		rkisp1_params_post_configure(&rkisp1->params);
> > +		ret = rkisp1_params_post_configure(&rkisp1->params);
> >   
> > -	return 0;
> > +	return ret;
> 
> I think ret could be returned uninitialised in some circumstances in this function now - if it's not 
> the IMX8MP version and the pixel encoding is bayer...or am I missing something?
> 
> >   }
> >   
> >   /* ----------------------------------------------------------------------------
> > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
> > index 3d78e643d0b8..c081fd490b2b 100644
> > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
> > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
> > @@ -2123,10 +2123,10 @@ static const struct rkisp1_cif_isp_afc_config rkisp1_afc_params_default_config =
> >   	14
> >   };
> >   
> > -void rkisp1_params_pre_configure(struct rkisp1_params *params,
> > -				 enum rkisp1_fmt_raw_pat_type bayer_pat,
> > -				 enum v4l2_quantization quantization,
> > -				 enum v4l2_ycbcr_encoding ycbcr_encoding)
> > +int rkisp1_params_pre_configure(struct rkisp1_params *params,
> > +				enum rkisp1_fmt_raw_pat_type bayer_pat,
> > +				enum v4l2_quantization quantization,
> > +				enum v4l2_ycbcr_encoding ycbcr_encoding)
> >   {
> >   	struct rkisp1_cif_isp_hst_config hst = rkisp1_hst_params_default_config;
> >   	struct rkisp1_buffer *buf;
> > @@ -2187,9 +2187,11 @@ void rkisp1_params_pre_configure(struct rkisp1_params *params,
> >   
> >   unlock:
> >   	spin_unlock_irq(&params->config_lock);
> > +
> > +	return ret;
> >   }
> >   
> > -void rkisp1_params_post_configure(struct rkisp1_params *params)
> > +int rkisp1_params_post_configure(struct rkisp1_params *params)
> >   {
> >   	struct rkisp1_buffer *buf;
> >   	int ret = 0;
> > @@ -2227,6 +2229,8 @@ void rkisp1_params_post_configure(struct rkisp1_params *params)
> >   
> >   unlock:
> >   	spin_unlock_irq(&params->config_lock);
> > +
> > +	return ret;
> >   }
> >   
> >   /*