Hi Dafna,
On Thu, Aug 18, 2022 at 06:45:46AM +0300, Dafna Hirschfeld wrote:
> On 17.08.2022 05:18, Laurent Pinchart wrote:
> > The ISP8000Nano v18.02 (found in the i.MX8MP) requires the ISP to be
> > enabled (as indicated by the ISP_CTRL.ISP_ENABLE bit) to configure the
> > lens shading table in internal RAM. The driver currently configures all
> > ISP initial parameters before enabling the ISP, which causes the LSC RAM
> > to not be initialized properly.
> >
> > To fix this, split the rkisp1_params_configure() function into a
> > rkisp1_params_pre_configure() and a rkisp1_params_post_configure(). The
> > former configures all ISP parameters but LSC, while the latter
> > configures LSC. To implement this, the rkisp1_params_apply_params_cfg()
> > function is deconstructed, with two small helpers created to deal with
> > the parameters buffers, which are then used in rkisp1_params_isr(),
> > rkisp1_params_pre_configure() and rkisp1_params_post_configure().
> >
> > While this initialization ordering is only needed for the ISP8000Nano
> > v18.02, it doesn't affect other ISP versions negatively, and can thus be
> > followed unconditionally.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> > ---
> > .../platform/rockchip/rkisp1/rkisp1-common.h | 29 ++-
> > .../platform/rockchip/rkisp1/rkisp1-isp.c | 9 +-
> > .../platform/rockchip/rkisp1/rkisp1-params.c | 169 ++++++++++++------
> > 3 files changed, 143 insertions(+), 64 deletions(-)
> >
> > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> > index 1383c13e22b8..f612e1b42b91 100644
> > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> > @@ -589,19 +589,32 @@ void rkisp1_sd_adjust_crop(struct v4l2_rect *crop,
> > */
> > const struct rkisp1_mbus_info *rkisp1_mbus_info_get_by_code(u32 mbus_code);
> >
> > -/* rkisp1_params_configure - configure the params when stream starts.
> > - * This function is called by the isp entity upon stream starts.
> > - * The function applies the initial configuration of the parameters.
> > +/*
> > + * rkisp1_params_pre_configure - Configure the params before stream start
> > *
> > - * @params: pointer to rkisp1_params.
> > + * @params: pointer to rkisp1_params
> > * @bayer_pat: the bayer pattern on the isp video sink pad
> > * @quantization: the quantization configured on the isp's src pad
> > * @ycbcr_encoding: the ycbcr_encoding configured on the isp's src pad
> > + *
> > + * This function is called by the ISP entity just before the ISP gets started.
> > + * 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_configure(struct rkisp1_params *params,
> > - enum rkisp1_fmt_raw_pat_type bayer_pat,
> > - enum v4l2_quantization quantization,
> > - enum v4l2_ycbcr_encoding ycbcr_encoding);
> > +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);
> > +
> > +/*
> > + * rkisp1_params_post_configure - Configure the params after stream start
> > + *
> > + * @params: pointer to rkisp1_params
> > + *
> > + * This function is called by the ISP entity just after the ISP gets started.
> > + * It applies the initial ISP KSC parameters from the first params buffer.
>
> What is KSC ?
An incorrect spelling of LSC :-) I'll fix it.
> > + */
> > +void 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 c029d2e86717..4876bf890cb2 100644
> > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
> > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
> > @@ -343,9 +343,9 @@ static int rkisp1_config_isp(struct rkisp1_isp *isp,
> > src_frm = rkisp1_isp_get_pad_fmt(isp, NULL,
> > RKISP1_ISP_PAD_SOURCE_VIDEO,
> > V4L2_SUBDEV_FORMAT_ACTIVE);
> > - rkisp1_params_configure(&rkisp1->params, sink_fmt->bayer_pat,
> > - src_frm->quantization,
> > - src_frm->ycbcr_enc);
> > + rkisp1_params_pre_configure(&rkisp1->params, sink_fmt->bayer_pat,
> > + src_frm->quantization,
> > + src_frm->ycbcr_enc);
> > }
> >
> > return 0;
> > @@ -462,6 +462,9 @@ static int rkisp1_isp_start(struct rkisp1_isp *isp, struct media_pad *source)
> > RKISP1_CIF_ISP_CTRL_ISP_INFORM_ENABLE;
> > rkisp1_write(rkisp1, RKISP1_CIF_ISP_CTRL, val);
> >
> > + if (isp->src_fmt->pixel_enc == V4L2_PIXEL_ENC_BAYER)
> > + rkisp1_params_post_configure(&rkisp1->params);
>
> why is post config called only in case of bayer?
Because pre_configure is also called for Bayer input only. When the
input is YUV the ISP is bypassed, and the parameters are reset instead.
> also I see that the lsc config from the isr is called without this condition.
> In addition the post config also does 'complete buffer' so maybe you do want to
> call it without the bayer condition.
When the input is YUV there isn't supposed to be any ISP params buffers
queued. I don't know what will happen if userspace queues any, maybe
something bad, but this patch doesn't affect that behaviour as far as I
can tell. Further fixes, if required, would go on top.
> > +
> > return 0;
> > }
> >
> > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
> > index 421ade177b2d..a4664efdfea8 100644
> > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
> > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
> > @@ -1297,22 +1297,6 @@ rkisp1_isp_isr_other_config(struct rkisp1_params *params,
> > RKISP1_CIF_ISP_CTRL_ISP_GAMMA_IN_ENA);
> > }
> >
> > - /* update lsc config */
> > - if (module_cfg_update & RKISP1_CIF_ISP_MODULE_LSC)
> > - rkisp1_lsc_config(params,
> > - &new_params->others.lsc_config);
> > -
> > - if (module_en_update & RKISP1_CIF_ISP_MODULE_LSC) {
> > - if (module_ens & RKISP1_CIF_ISP_MODULE_LSC)
> > - rkisp1_param_set_bits(params,
> > - RKISP1_CIF_ISP_LSC_CTRL,
> > - RKISP1_CIF_ISP_LSC_CTRL_ENA);
> > - else
> > - rkisp1_param_clear_bits(params,
> > - RKISP1_CIF_ISP_LSC_CTRL,
> > - RKISP1_CIF_ISP_LSC_CTRL_ENA);
> > - }
> > -
> > /* update awb gains */
> > if (module_cfg_update & RKISP1_CIF_ISP_MODULE_AWB_GAIN)
> > params->ops->awb_gain_config(params, &new_params->others.awb_gain_config);
> > @@ -1429,6 +1413,33 @@ rkisp1_isp_isr_other_config(struct rkisp1_params *params,
> > }
> > }
> >
> > +static void
> > +rkisp1_isp_isr_lsc_config(struct rkisp1_params *params,
> > + const struct rkisp1_params_cfg *new_params)
> > +{
> > + unsigned int module_en_update, module_cfg_update, module_ens;
> > +
> > + module_en_update = new_params->module_en_update;
> > + module_cfg_update = new_params->module_cfg_update;
> > + module_ens = new_params->module_ens;
> > +
> > + /* update lsc config */
> > + if (module_cfg_update & RKISP1_CIF_ISP_MODULE_LSC)
> > + rkisp1_lsc_config(params,
> > + &new_params->others.lsc_config);
> > +
> > + if (module_en_update & RKISP1_CIF_ISP_MODULE_LSC) {
> > + if (module_ens & RKISP1_CIF_ISP_MODULE_LSC)
> > + rkisp1_param_set_bits(params,
> > + RKISP1_CIF_ISP_LSC_CTRL,
> > + RKISP1_CIF_ISP_LSC_CTRL_ENA);
> > + else
> > + rkisp1_param_clear_bits(params,
> > + RKISP1_CIF_ISP_LSC_CTRL,
> > + RKISP1_CIF_ISP_LSC_CTRL_ENA);
> > + }
> > +}
> > +
> > static void rkisp1_isp_isr_meas_config(struct rkisp1_params *params,
> > struct rkisp1_params_cfg *new_params)
> > {
> > @@ -1490,47 +1501,60 @@ static void rkisp1_isp_isr_meas_config(struct rkisp1_params *params,
> > }
> > }
> >
> > -static void rkisp1_params_apply_params_cfg(struct rkisp1_params *params,
> > - unsigned int frame_sequence)
> > +static bool rkisp1_params_get_buffer(struct rkisp1_params *params,
> > + struct rkisp1_buffer **buf,
> > + struct rkisp1_params_cfg **cfg)
> > {
> > - struct rkisp1_params_cfg *new_params;
> > - struct rkisp1_buffer *cur_buf = NULL;
> > -
> > if (list_empty(¶ms->params))
> > - return;
> > + return false;
> >
> > - cur_buf = list_first_entry(¶ms->params,
> > - struct rkisp1_buffer, queue);
> > + *buf = list_first_entry(¶ms->params, struct rkisp1_buffer, queue);
> > + *cfg = vb2_plane_vaddr(&(*buf)->vb.vb2_buf, 0);
> >
> > - new_params = (struct rkisp1_params_cfg *)vb2_plane_vaddr(&cur_buf->vb.vb2_buf, 0);
> > + return true;
> > +}
> >
> > - rkisp1_isp_isr_other_config(params, new_params);
> > - rkisp1_isp_isr_meas_config(params, new_params);
> > +static void rkisp1_params_complete_buffer(struct rkisp1_params *params,
> > + struct rkisp1_buffer *buf,
> > + unsigned int frame_sequence)
> > +{
> > + list_del(&buf->queue);
> >
> > - /* update shadow register immediately */
> > - rkisp1_param_set_bits(params, RKISP1_CIF_ISP_CTRL, RKISP1_CIF_ISP_CTRL_ISP_CFG_UPD);
> > -
> > - list_del(&cur_buf->queue);
> > -
> > - cur_buf->vb.sequence = frame_sequence;
> > - vb2_buffer_done(&cur_buf->vb.vb2_buf, VB2_BUF_STATE_DONE);
> > + buf->vb.sequence = frame_sequence;
> > + vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_DONE);
> > }
> >
> > void rkisp1_params_isr(struct rkisp1_device *rkisp1)
> > {
> > - /*
> > - * This isr is called when the ISR finishes processing a frame (RKISP1_CIF_ISP_FRAME).
> > - * Configurations performed here will be applied on the next frame.
> > - * Since frame_sequence is updated on the vertical sync signal, we should use
> > - * frame_sequence + 1 here to indicate to userspace on which frame these parameters
> > - * are being applied.
> > - */
> > - unsigned int frame_sequence = rkisp1->isp.frame_sequence + 1;
> > struct rkisp1_params *params = &rkisp1->params;
> > + struct rkisp1_params_cfg *new_params;
> > + struct rkisp1_buffer *cur_buf;
> >
> > spin_lock(¶ms->config_lock);
> > - rkisp1_params_apply_params_cfg(params, frame_sequence);
> >
> > + if (!rkisp1_params_get_buffer(params, &cur_buf, &new_params))
> > + goto unlock;
> > +
> > + rkisp1_isp_isr_other_config(params, new_params);
> > + rkisp1_isp_isr_lsc_config(params, new_params);
> > + rkisp1_isp_isr_meas_config(params, new_params);
> > +
> > + /* update shadow register immediately */
> > + rkisp1_param_set_bits(params, RKISP1_CIF_ISP_CTRL,
> > + RKISP1_CIF_ISP_CTRL_ISP_CFG_UPD);
> > +
> > + /*
> > + * This isr is called when the ISR finishes processing a frame
> > + * (RKISP1_CIF_ISP_FRAME). Configurations performed here will be
> > + * applied on the next frame. Since frame_sequence is updated on the
> > + * vertical sync signal, we should use frame_sequence + 1 here to
> > + * indicate to userspace on which frame these parameters are being
> > + * applied.
> > + */
> > + rkisp1_params_complete_buffer(params, cur_buf,
> > + rkisp1->isp.frame_sequence + 1);
> > +
> > +unlock:
> > spin_unlock(¶ms->config_lock);
> > }
> >
> > @@ -1573,9 +1597,18 @@ static const struct rkisp1_cif_isp_afc_config rkisp1_afc_params_default_config =
> > 14
> > };
> >
> > -static void rkisp1_params_config_parameter(struct rkisp1_params *params)
> > +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)
> > {
> > struct rkisp1_cif_isp_hst_config hst = rkisp1_hst_params_default_config;
> > + struct rkisp1_params_cfg *new_params;
> > + struct rkisp1_buffer *cur_buf;
> > +
> > + params->quantization = quantization;
> > + params->ycbcr_encoding = ycbcr_encoding;
> > + params->raw_type = bayer_pat;
> >
> > params->ops->awb_meas_config(params, &rkisp1_awb_params_default_config);
> > params->ops->awb_meas_enable(params, &rkisp1_awb_params_default_config,
> > @@ -1599,20 +1632,50 @@ static void rkisp1_params_config_parameter(struct rkisp1_params *params)
> > spin_lock_irq(¶ms->config_lock);
> >
> > /* apply the first buffer if there is one already */
> > - rkisp1_params_apply_params_cfg(params, 0);
> >
> > + if (!rkisp1_params_get_buffer(params, &cur_buf, &new_params))
> > + goto unlock;
> > +
> > + rkisp1_isp_isr_other_config(params, new_params);
> > + rkisp1_isp_isr_meas_config(params, new_params);
> > +
> > + /* update shadow register immediately */
> > + rkisp1_param_set_bits(params, RKISP1_CIF_ISP_CTRL,
> > + RKISP1_CIF_ISP_CTRL_ISP_CFG_UPD);
> > +
> > +unlock:
> > spin_unlock_irq(¶ms->config_lock);
> > }
> >
> > -void rkisp1_params_configure(struct rkisp1_params *params,
> > - enum rkisp1_fmt_raw_pat_type bayer_pat,
> > - enum v4l2_quantization quantization,
> > - enum v4l2_ycbcr_encoding ycbcr_encoding)
> > +void rkisp1_params_post_configure(struct rkisp1_params *params)
> > {
> > - params->quantization = quantization;
> > - params->ycbcr_encoding = ycbcr_encoding;
> > - params->raw_type = bayer_pat;
> > - rkisp1_params_config_parameter(params);
> > + struct rkisp1_params_cfg *new_params;
> > + struct rkisp1_buffer *cur_buf;
> > +
> > + spin_lock_irq(¶ms->config_lock);
> > +
> > + /*
> > + * Apply LSC parameters from the first buffer (if any is already
> > + * available. This must be done after the ISP gets started in the
> > + * ISP8000Nano v18.02 (found in the i.MX8MP) as access to the LSC RAM
> > + * is gated by the ISP_CTRL.ISP_ENABLE bit. As this initialization
> > + * ordering doesn't affect other ISP versions negatively, do so
> > + * unconditionally.
> > + */
> > +
> > + if (!rkisp1_params_get_buffer(params, &cur_buf, &new_params))
> > + goto unlock;
> > +
> > + rkisp1_isp_isr_lsc_config(params, new_params);
> > +
> > + /* update shadow register immediately */
> > + rkisp1_param_set_bits(params, RKISP1_CIF_ISP_CTRL,
> > + RKISP1_CIF_ISP_CTRL_ISP_CFG_UPD);
>
> updating the shadow regs is already done in the 'complete buffer'
I don't see that being done in rkisp1_params_complete_buffer().
> > +
> > + rkisp1_params_complete_buffer(params, cur_buf, 0);
> > +
> > +unlock:
> > + spin_unlock_irq(¶ms->config_lock);
> > }
> >
> > /*
--
Regards,
Laurent Pinchart
