Re: [PATCH v2 08/14] media: staging: rkisp1: params: set vb.sequence to be the isp's frame_sequence + 1

[Helen Koike <helen.koike@xxxxxxxxxxxxx>]

Hi Dafna,

On 8/15/20 7:37 AM, Dafna Hirschfeld wrote:
> The params isr is called when a frame is out of the isp. The parameters
> are applied immediately since the isr updates the shadow registers.
> Therefore the params are first applied on the next frame.
> We want the vb.sequence to be the frame that the params are applied to.
> So we set vb.sequence to be the isp's frame_sequence + 1
> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@xxxxxxxxxxxxx>
> ---
>  drivers/staging/media/rkisp1/rkisp1-params.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/media/rkisp1/rkisp1-params.c b/drivers/staging/media/rkisp1/rkisp1-params.c
> index 134b5c9a94c1..4b4391c0a2a0 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-params.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-params.c
> @@ -1220,7 +1220,14 @@ void rkisp1_params_apply_params_cfg(struct rkisp1_params *params, unsigned int f
>  
>  void rkisp1_params_isr(struct rkisp1_device *rkisp1)
>  {
> -	unsigned int frame_sequence = atomic_read(&rkisp1->isp.frame_sequence);
> +	/*
> +	 * The params isr is called when a frame is out of the isp. The parameters
> +	 * are applied immediately since the isr updates the shadow registers.
> +	 * Therefore the params are first applied on the next frame.
> +	 * We want the vb.sequence to be the frame that the params are applied to.
> +	 * So we set vb.sequence to be the isp's frame_sequence + 1
> +	 */

I would just re-phrase this a bit, how about:

	This isr is called when the ISP finishes processing a frame.
	To configure the parameters, we update the shadow registers, which means
	that the next frame will already take these new configuration into consideration.
	Since frame_sequence is only updated on the vertical sync signal, we should use
	frame_sequence + 1 here to indicate to userspace which frame this parameters
	are being applied to.


Or maybe smaller:

	This isr is called when the ISP finishes processing a frame.
	Configurations performed here will be applied to the next frame.	
	Since frame_sequence is only updated on the vertical sync signal, we should use
	frame_sequence + 1 here to indicate to userspace which frame this parameters
	are being applied to.

What do you think?

With an improvement in the text (and also commit message):

Acked-by: Helen Koike <helen.koike@xxxxxxxxxxxxx>

Regards,
Helen

> +	unsigned int frame_sequence = atomic_read(&rkisp1->isp.frame_sequence) + 1;
>  	struct rkisp1_params *params = &rkisp1->params;
>  
>  	spin_lock(&params->config_lock);
>