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

Message ID 20240605165434.432230-7-jacopo.mondi@ideasonboard.com (mailing list archive)
State New
Headers
Series media: rkisp1: Implement support for extensible parameters |

Commit Message

Jacopo Mondi June 5, 2024, 4:54 p.m. UTC
  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(-)
  

Comments

Dan Scally June 12, 2024, 1:35 p.m. UTC | #1
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;
>   }
>   
>   /*
  
Laurent Pinchart June 12, 2024, 3:46 p.m. UTC | #2
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;
> >   }
> >   
> >   /*
  

Patch

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;
 }
 
 /* ----------------------------------------------------------------------------
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;
 }
 
 /*