[v1] media: rzg2l-cru: Rework slot programming for V2H/G3E

[PATCH 05/14] media: rzg2l-cru: Remove locking from start/stop routines

Posted by Jacopo Mondi 5 days, 22 hours ago

From: Jacopo Mondi <jacopo.mondi+renesas@ideasonboard.com>

The start/stop streaming routines do not need to lock the whole function
body against possible concurrent accesses to the CRU buffers or hardware
registers.

The stop function starts by disabling interrupts, and only this portion
needs to be protected not to race against a possible IRQ.

Once interrupts are disabled, nothing in the video device driver can race
and once the peripheral has been disabled we can release all pending
buffers.

Signed-off-by: Jacopo Mondi <jacopo.mondi+renesas@ideasonboard.com>
---
 drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c | 18 +++++-------------
 1 file changed, 5 insertions(+), 13 deletions(-)

diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
index b041c72837c6..43b1d35fb963 100644
--- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
+++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
@@ -341,23 +341,19 @@ bool rzg2l_fifo_empty(struct rzg2l_cru_dev *cru)
 void rzg2l_cru_stop_image_processing(struct rzg2l_cru_dev *cru)
 {
 	unsigned int retries = 0;
-	unsigned long flags;
 	u32 icnms;
 
-	spin_lock_irqsave(&cru->qlock, flags);
-
-	/* Disable and clear the interrupt */
-	cru->info->disable_interrupts(cru);
+	scoped_guard(spinlock_irq, &cru->qlock) {
+		/* Disable and clear the interrupt */
+		cru->info->disable_interrupts(cru);
+	}
 
 	/* Stop the operation of image conversion */
 	rzg2l_cru_write(cru, ICnEN, 0);
 
 	/* Wait for streaming to stop */
-	while ((rzg2l_cru_read(cru, ICnMS) & ICnMS_IA) && retries++ < RZG2L_RETRIES) {
-		spin_unlock_irqrestore(&cru->qlock, flags);
+	while ((rzg2l_cru_read(cru, ICnMS) & ICnMS_IA) && retries++ < RZG2L_RETRIES)
 		msleep(RZG2L_TIMEOUT_MS);
-		spin_lock_irqsave(&cru->qlock, flags);
-	}
 
 	icnms = rzg2l_cru_read(cru, ICnMS) & ICnMS_IA;
 	if (icnms)
@@ -401,8 +397,6 @@ void rzg2l_cru_stop_image_processing(struct rzg2l_cru_dev *cru)
 
 	/* Resets the image processing module */
 	rzg2l_cru_write(cru, CRUnRST, 0);
-
-	spin_unlock_irqrestore(&cru->qlock, flags);
 }
 
 static int rzg2l_cru_get_virtual_channel(struct rzg2l_cru_dev *cru)
@@ -470,8 +464,6 @@ int rzg2l_cru_start_image_processing(struct rzg2l_cru_dev *cru)
 	csi_vc = ret;
 	cru->svc_channel = csi_vc;
 
-	guard(spinlock_irqsave)(&cru->qlock);
-
 	/* Select a video input */
 	rzg2l_cru_write(cru, CRUnCTRL, CRUnCTRL_VINSEL(0));
 

-- 
2.53.0

Re: [PATCH 05/14] media: rzg2l-cru: Remove locking from start/stop routines

Posted by Lad, Prabhakar 2 days, 20 hours ago

On Fri, Mar 27, 2026 at 5:20 PM Jacopo Mondi
<jacopo.mondi@ideasonboard.com> wrote:
>
> From: Jacopo Mondi <jacopo.mondi+renesas@ideasonboard.com>
>
> The start/stop streaming routines do not need to lock the whole function
> body against possible concurrent accesses to the CRU buffers or hardware
> registers.
>
> The stop function starts by disabling interrupts, and only this portion
> needs to be protected not to race against a possible IRQ.
>
> Once interrupts are disabled, nothing in the video device driver can race
> and once the peripheral has been disabled we can release all pending
> buffers.
>
> Signed-off-by: Jacopo Mondi <jacopo.mondi+renesas@ideasonboard.com>
> ---
>  drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c | 18 +++++-------------
>  1 file changed, 5 insertions(+), 13 deletions(-)
>
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Cheers,
Prabhakar

> diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> index b041c72837c6..43b1d35fb963 100644
> --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> @@ -341,23 +341,19 @@ bool rzg2l_fifo_empty(struct rzg2l_cru_dev *cru)
>  void rzg2l_cru_stop_image_processing(struct rzg2l_cru_dev *cru)
>  {
>         unsigned int retries = 0;
> -       unsigned long flags;
>         u32 icnms;
>
> -       spin_lock_irqsave(&cru->qlock, flags);
> -
> -       /* Disable and clear the interrupt */
> -       cru->info->disable_interrupts(cru);
> +       scoped_guard(spinlock_irq, &cru->qlock) {
> +               /* Disable and clear the interrupt */
> +               cru->info->disable_interrupts(cru);
> +       }
>
>         /* Stop the operation of image conversion */
>         rzg2l_cru_write(cru, ICnEN, 0);
>
>         /* Wait for streaming to stop */
> -       while ((rzg2l_cru_read(cru, ICnMS) & ICnMS_IA) && retries++ < RZG2L_RETRIES) {
> -               spin_unlock_irqrestore(&cru->qlock, flags);
> +       while ((rzg2l_cru_read(cru, ICnMS) & ICnMS_IA) && retries++ < RZG2L_RETRIES)
>                 msleep(RZG2L_TIMEOUT_MS);
> -               spin_lock_irqsave(&cru->qlock, flags);
> -       }
>
>         icnms = rzg2l_cru_read(cru, ICnMS) & ICnMS_IA;
>         if (icnms)
> @@ -401,8 +397,6 @@ void rzg2l_cru_stop_image_processing(struct rzg2l_cru_dev *cru)
>
>         /* Resets the image processing module */
>         rzg2l_cru_write(cru, CRUnRST, 0);
> -
> -       spin_unlock_irqrestore(&cru->qlock, flags);
>  }
>
>  static int rzg2l_cru_get_virtual_channel(struct rzg2l_cru_dev *cru)
> @@ -470,8 +464,6 @@ int rzg2l_cru_start_image_processing(struct rzg2l_cru_dev *cru)
>         csi_vc = ret;
>         cru->svc_channel = csi_vc;
>
> -       guard(spinlock_irqsave)(&cru->qlock);
> -
>         /* Select a video input */
>         rzg2l_cru_write(cru, CRUnCTRL, CRUnCTRL_VINSEL(0));
>
>
> --
> 2.53.0
>
>

Re: [PATCH 05/14] media: rzg2l-cru: Remove locking from start/stop routines

Posted by Dan Scally 3 days, 1 hour ago

Hi Jacopo

On 27/03/2026 17:10, Jacopo Mondi wrote:
> From: Jacopo Mondi <jacopo.mondi+renesas@ideasonboard.com>
> 
> The start/stop streaming routines do not need to lock the whole function
> body against possible concurrent accesses to the CRU buffers or hardware
> registers.
> 
> The stop function starts by disabling interrupts, and only this portion
> needs to be protected not to race against a possible IRQ.
> 
> Once interrupts are disabled, nothing in the video device driver can race
> and once the peripheral has been disabled we can release all pending
> buffers.
> 
> Signed-off-by: Jacopo Mondi <jacopo.mondi+renesas@ideasonboard.com>
> ---

Looks ok to me:

Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>

>   drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c | 18 +++++-------------
>   1 file changed, 5 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> index b041c72837c6..43b1d35fb963 100644
> --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> @@ -341,23 +341,19 @@ bool rzg2l_fifo_empty(struct rzg2l_cru_dev *cru)
>   void rzg2l_cru_stop_image_processing(struct rzg2l_cru_dev *cru)
>   {
>   	unsigned int retries = 0;
> -	unsigned long flags;
>   	u32 icnms;
>   
> -	spin_lock_irqsave(&cru->qlock, flags);
> -
> -	/* Disable and clear the interrupt */
> -	cru->info->disable_interrupts(cru);
> +	scoped_guard(spinlock_irq, &cru->qlock) {
> +		/* Disable and clear the interrupt */
> +		cru->info->disable_interrupts(cru);
> +	}
>   
>   	/* Stop the operation of image conversion */
>   	rzg2l_cru_write(cru, ICnEN, 0);
>   
>   	/* Wait for streaming to stop */
> -	while ((rzg2l_cru_read(cru, ICnMS) & ICnMS_IA) && retries++ < RZG2L_RETRIES) {
> -		spin_unlock_irqrestore(&cru->qlock, flags);
> +	while ((rzg2l_cru_read(cru, ICnMS) & ICnMS_IA) && retries++ < RZG2L_RETRIES)
>   		msleep(RZG2L_TIMEOUT_MS);
> -		spin_lock_irqsave(&cru->qlock, flags);
> -	}
>   
>   	icnms = rzg2l_cru_read(cru, ICnMS) & ICnMS_IA;
>   	if (icnms)
> @@ -401,8 +397,6 @@ void rzg2l_cru_stop_image_processing(struct rzg2l_cru_dev *cru)
>   
>   	/* Resets the image processing module */
>   	rzg2l_cru_write(cru, CRUnRST, 0);
> -
> -	spin_unlock_irqrestore(&cru->qlock, flags);
>   }
>   
>   static int rzg2l_cru_get_virtual_channel(struct rzg2l_cru_dev *cru)
> @@ -470,8 +464,6 @@ int rzg2l_cru_start_image_processing(struct rzg2l_cru_dev *cru)
>   	csi_vc = ret;
>   	cru->svc_channel = csi_vc;
>   
> -	guard(spinlock_irqsave)(&cru->qlock);
> -
>   	/* Select a video input */
>   	rzg2l_cru_write(cru, CRUnCTRL, CRUnCTRL_VINSEL(0));
>   
>

Re: [PATCH 05/14] media: rzg2l-cru: Remove locking from start/stop routines

Posted by Tommaso Merciai 3 days, 2 hours ago

Hi Jacopo,
Thanks for your patch.

On Fri, Mar 27, 2026 at 06:10:10PM +0100, Jacopo Mondi wrote:
> From: Jacopo Mondi <jacopo.mondi+renesas@ideasonboard.com>
> 
> The start/stop streaming routines do not need to lock the whole function
> body against possible concurrent accesses to the CRU buffers or hardware
> registers.
> 
> The stop function starts by disabling interrupts, and only this portion
> needs to be protected not to race against a possible IRQ.
> 
> Once interrupts are disabled, nothing in the video device driver can race
> and once the peripheral has been disabled we can release all pending
> buffers.
> 

LGTM. Tested on RZ/G3E.

Tested-by: Tommaso Merciai <tommaso.merciai.xr@bp.renesas.com>
Reviewed-by: Tommaso Merciai <tommaso.merciai.xr@bp.renesas.com>


Kind Regards,
Tommaso

> Signed-off-by: Jacopo Mondi <jacopo.mondi+renesas@ideasonboard.com>
> ---
>  drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c | 18 +++++-------------
>  1 file changed, 5 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> index b041c72837c6..43b1d35fb963 100644
> --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> @@ -341,23 +341,19 @@ bool rzg2l_fifo_empty(struct rzg2l_cru_dev *cru)
>  void rzg2l_cru_stop_image_processing(struct rzg2l_cru_dev *cru)
>  {
>  	unsigned int retries = 0;
> -	unsigned long flags;
>  	u32 icnms;
>  
> -	spin_lock_irqsave(&cru->qlock, flags);
> -
> -	/* Disable and clear the interrupt */
> -	cru->info->disable_interrupts(cru);
> +	scoped_guard(spinlock_irq, &cru->qlock) {
> +		/* Disable and clear the interrupt */
> +		cru->info->disable_interrupts(cru);
> +	}
>  
>  	/* Stop the operation of image conversion */
>  	rzg2l_cru_write(cru, ICnEN, 0);
>  
>  	/* Wait for streaming to stop */
> -	while ((rzg2l_cru_read(cru, ICnMS) & ICnMS_IA) && retries++ < RZG2L_RETRIES) {
> -		spin_unlock_irqrestore(&cru->qlock, flags);
> +	while ((rzg2l_cru_read(cru, ICnMS) & ICnMS_IA) && retries++ < RZG2L_RETRIES)
>  		msleep(RZG2L_TIMEOUT_MS);
> -		spin_lock_irqsave(&cru->qlock, flags);
> -	}
>  
>  	icnms = rzg2l_cru_read(cru, ICnMS) & ICnMS_IA;
>  	if (icnms)
> @@ -401,8 +397,6 @@ void rzg2l_cru_stop_image_processing(struct rzg2l_cru_dev *cru)
>  
>  	/* Resets the image processing module */
>  	rzg2l_cru_write(cru, CRUnRST, 0);
> -
> -	spin_unlock_irqrestore(&cru->qlock, flags);
>  }
>  
>  static int rzg2l_cru_get_virtual_channel(struct rzg2l_cru_dev *cru)
> @@ -470,8 +464,6 @@ int rzg2l_cru_start_image_processing(struct rzg2l_cru_dev *cru)
>  	csi_vc = ret;
>  	cru->svc_channel = csi_vc;
>  
> -	guard(spinlock_irqsave)(&cru->qlock);
> -
>  	/* Select a video input */
>  	rzg2l_cru_write(cru, CRUnCTRL, CRUnCTRL_VINSEL(0));
>  
> 
> -- 
> 2.53.0
>