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@xxxxxxxxxxxxxxxx>
> > ---
> > .../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(¶ms->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(¶ms->config_lock);
> > +
> > + return ret;
> > }
> >
> > /*
--
Regards,
Laurent Pinchart
