Re: [PATCH 5/5] media: rkisp1: Configure LSC after enabling the ISP

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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(&params->params))
> > -		return;
> > +		return false;
> > 
> > -	cur_buf = list_first_entry(&params->params,
> > -				   struct rkisp1_buffer, queue);
> > +	*buf = list_first_entry(&params->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(&params->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(&params->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(&params->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(&params->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(&params->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(&params->config_lock);
> >  }
> > 
> >  /*

-- 
Regards,

Laurent Pinchart



[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux