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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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

-- 
Regards,

Laurent Pinchart