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

[PATCH 09/14] media: rzg2l-cru: Split hw locking from buffers

Posted by Jacopo Mondi 5 days, 22 hours ago

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

Split the locking between a spinlock dedicated to protect the hardware
slots programming (hw_lock) and one lock (qlock) to protect the queue of
buffers submitted by userspace.

Do not rework the locking strategy yet but start simply by splitting the
locking in two.

Signed-off-by: Jacopo Mondi <jacopo.mondi+renesas@ideasonboard.com>
---
 .../media/platform/renesas/rzg2l-cru/rzg2l-video.c | 32 +++++++++++++---------
 1 file changed, 19 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 a79b17e146bf..9406a089ec9f 100644
--- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
+++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
@@ -112,19 +112,21 @@ static void return_unused_buffers(struct rzg2l_cru_dev *cru,
 	struct rzg2l_cru_buffer *buf, *node;
 	unsigned int i;
 
-	guard(spinlock_irq)(&cru->qlock);
-
-	for (i = 0; i < cru->num_buf; i++) {
-		if (cru->queue_buf[i]) {
-			vb2_buffer_done(&cru->queue_buf[i]->vb2_buf,
-					state);
-			cru->queue_buf[i] = NULL;
+	scoped_guard(spinlock_irq, &cru->hw_lock) {
+		for (i = 0; i < cru->num_buf; i++) {
+			if (cru->queue_buf[i]) {
+				vb2_buffer_done(&cru->queue_buf[i]->vb2_buf,
+						state);
+				cru->queue_buf[i] = NULL;
+			}
 		}
 	}
 
-	list_for_each_entry_safe(buf, node, &cru->buf_list, list) {
-		vb2_buffer_done(&buf->vb.vb2_buf, state);
-		list_del(&buf->list);
+	scoped_guard(spinlock_irq, &cru->qlock) {
+		list_for_each_entry_safe(buf, node, &cru->buf_list, list) {
+			vb2_buffer_done(&buf->vb.vb2_buf, state);
+			list_del(&buf->list);
+		}
 	}
 }
 
@@ -198,12 +200,16 @@ static void rzg2l_cru_fill_hw_slot(struct rzg2l_cru_dev *cru, int slot)
 	struct rzg2l_cru_buffer *buf;
 	dma_addr_t phys_addr;
 
+	lockdep_assert_held(&cru->hw_lock);
+
 	/* A already populated slot shall never be overwritten. */
 	if (WARN_ON(cru->queue_buf[slot]))
 		return;
 
 	dev_dbg(cru->dev, "Filling HW slot: %d\n", slot);
 
+	guard(spinlock)(&cru->qlock);
+
 	if (list_empty(&cru->buf_list)) {
 		cru->queue_buf[slot] = NULL;
 		phys_addr = cru->scratch_phys;
@@ -342,7 +348,7 @@ void rzg2l_cru_stop_image_processing(struct rzg2l_cru_dev *cru)
 	unsigned int retries = 0;
 	u32 icnms;
 
-	scoped_guard(spinlock_irq, &cru->qlock) {
+	scoped_guard(spinlock_irq, &cru->hw_lock) {
 		/* Disable and clear the interrupt */
 		cru->info->disable_interrupts(cru);
 	}
@@ -560,7 +566,7 @@ irqreturn_t rzg2l_cru_irq(int irq, void *data)
 	u32 amnmbs;
 	int slot;
 
-	guard(spinlock_irqsave)(&cru->qlock);
+	guard(spinlock_irqsave)(&cru->hw_lock);
 
 	irq_status = rzg2l_cru_read(cru, CRUnINTS);
 	if (!irq_status)
@@ -662,7 +668,7 @@ irqreturn_t rzg3e_cru_irq(int irq, void *data)
 	u32 irq_status;
 	int slot;
 
-	guard(spinlock)(&cru->qlock);
+	guard(spinlock)(&cru->hw_lock);
 
 	irq_status = rzg2l_cru_read(cru, CRUnINTS2);
 	if (!irq_status)

-- 
2.53.0

Re: [PATCH 09/14] media: rzg2l-cru: Split hw locking from buffers

Posted by Dan Scally 3 days, 4 hours ago

Hi Jacopo

On 27/03/2026 17:10, Jacopo Mondi wrote:
> From: Jacopo Mondi <jacopo.mondi+renesas@ideasonboard.com>
> 
> Split the locking between a spinlock dedicated to protect the hardware
> slots programming (hw_lock) and one lock (qlock) to protect the queue of
> buffers submitted by userspace.
> 
> Do not rework the locking strategy yet but start simply by splitting the
> locking in two.
> 
> Signed-off-by: Jacopo Mondi <jacopo.mondi+renesas@ideasonboard.com>
> ---
>   .../media/platform/renesas/rzg2l-cru/rzg2l-video.c | 32 +++++++++++++---------
>   1 file changed, 19 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 a79b17e146bf..9406a089ec9f 100644
> --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> @@ -112,19 +112,21 @@ static void return_unused_buffers(struct rzg2l_cru_dev *cru,
>   	struct rzg2l_cru_buffer *buf, *node;
>   	unsigned int i;
>   
> -	guard(spinlock_irq)(&cru->qlock);
> -
> -	for (i = 0; i < cru->num_buf; i++) {
> -		if (cru->queue_buf[i]) {
> -			vb2_buffer_done(&cru->queue_buf[i]->vb2_buf,
> -					state);
> -			cru->queue_buf[i] = NULL;
> +	scoped_guard(spinlock_irq, &cru->hw_lock) {
> +		for (i = 0; i < cru->num_buf; i++) {
> +			if (cru->queue_buf[i]) {
> +				vb2_buffer_done(&cru->queue_buf[i]->vb2_buf,
> +						state);
> +				cru->queue_buf[i] = NULL;
> +			}
>   		}
>   	}
>   
> -	list_for_each_entry_safe(buf, node, &cru->buf_list, list) {
> -		vb2_buffer_done(&buf->vb.vb2_buf, state);
> -		list_del(&buf->list);
> +	scoped_guard(spinlock_irq, &cru->qlock) {
> +		list_for_each_entry_safe(buf, node, &cru->buf_list, list) {
> +			vb2_buffer_done(&buf->vb.vb2_buf, state);
> +			list_del(&buf->list);
> +		}
>   	}
>   }
>   
> @@ -198,12 +200,16 @@ static void rzg2l_cru_fill_hw_slot(struct rzg2l_cru_dev *cru, int slot)
>   	struct rzg2l_cru_buffer *buf;
>   	dma_addr_t phys_addr;
>   
> +	lockdep_assert_held(&cru->hw_lock);

I think this condition mightn't be true at the point of this commit; as far as I can see this 
function is called without hw_lock being held in rzg2l_cru_initialize_axi() until patch 12.

> +
>   	/* A already populated slot shall never be overwritten. */
>   	if (WARN_ON(cru->queue_buf[slot]))
>   		return;
>   
>   	dev_dbg(cru->dev, "Filling HW slot: %d\n", slot);
>   
> +	guard(spinlock)(&cru->qlock);
> +
>   	if (list_empty(&cru->buf_list)) {
>   		cru->queue_buf[slot] = NULL;
>   		phys_addr = cru->scratch_phys;
> @@ -342,7 +348,7 @@ void rzg2l_cru_stop_image_processing(struct rzg2l_cru_dev *cru)
>   	unsigned int retries = 0;
>   	u32 icnms;
>   
> -	scoped_guard(spinlock_irq, &cru->qlock) {
> +	scoped_guard(spinlock_irq, &cru->hw_lock) {
>   		/* Disable and clear the interrupt */
>   		cru->info->disable_interrupts(cru);
>   	}
> @@ -560,7 +566,7 @@ irqreturn_t rzg2l_cru_irq(int irq, void *data)
>   	u32 amnmbs;
>   	int slot;
>   
> -	guard(spinlock_irqsave)(&cru->qlock);
> +	guard(spinlock_irqsave)(&cru->hw_lock);
>   
>   	irq_status = rzg2l_cru_read(cru, CRUnINTS);
>   	if (!irq_status)
> @@ -662,7 +668,7 @@ irqreturn_t rzg3e_cru_irq(int irq, void *data)
>   	u32 irq_status;
>   	int slot;
>   
> -	guard(spinlock)(&cru->qlock);
> +	guard(spinlock)(&cru->hw_lock);
>   
>   	irq_status = rzg2l_cru_read(cru, CRUnINTS2);
>   	if (!irq_status)
>

Re: [PATCH 09/14] media: rzg2l-cru: Split hw locking from buffers

Posted by Tommaso Merciai 2 days, 23 hours ago

On Mon, Mar 30, 2026 at 12:19:00PM +0100, Dan Scally wrote:
> Hi Jacopo
> 
> On 27/03/2026 17:10, Jacopo Mondi wrote:
> > From: Jacopo Mondi <jacopo.mondi+renesas@ideasonboard.com>
> > 
> > Split the locking between a spinlock dedicated to protect the hardware
> > slots programming (hw_lock) and one lock (qlock) to protect the queue of
> > buffers submitted by userspace.
> > 
> > Do not rework the locking strategy yet but start simply by splitting the
> > locking in two.
> > 
> > Signed-off-by: Jacopo Mondi <jacopo.mondi+renesas@ideasonboard.com>
> > ---
> >   .../media/platform/renesas/rzg2l-cru/rzg2l-video.c | 32 +++++++++++++---------
> >   1 file changed, 19 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 a79b17e146bf..9406a089ec9f 100644
> > --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> > +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> > @@ -112,19 +112,21 @@ static void return_unused_buffers(struct rzg2l_cru_dev *cru,
> >   	struct rzg2l_cru_buffer *buf, *node;
> >   	unsigned int i;
> > -	guard(spinlock_irq)(&cru->qlock);
> > -
> > -	for (i = 0; i < cru->num_buf; i++) {
> > -		if (cru->queue_buf[i]) {
> > -			vb2_buffer_done(&cru->queue_buf[i]->vb2_buf,
> > -					state);
> > -			cru->queue_buf[i] = NULL;
> > +	scoped_guard(spinlock_irq, &cru->hw_lock) {
> > +		for (i = 0; i < cru->num_buf; i++) {
> > +			if (cru->queue_buf[i]) {
> > +				vb2_buffer_done(&cru->queue_buf[i]->vb2_buf,
> > +						state);
> > +				cru->queue_buf[i] = NULL;
> > +			}
> >   		}
> >   	}
> > -	list_for_each_entry_safe(buf, node, &cru->buf_list, list) {
> > -		vb2_buffer_done(&buf->vb.vb2_buf, state);
> > -		list_del(&buf->list);
> > +	scoped_guard(spinlock_irq, &cru->qlock) {
> > +		list_for_each_entry_safe(buf, node, &cru->buf_list, list) {
> > +			vb2_buffer_done(&buf->vb.vb2_buf, state);
> > +			list_del(&buf->list);
> > +		}
> >   	}
> >   }
> > @@ -198,12 +200,16 @@ static void rzg2l_cru_fill_hw_slot(struct rzg2l_cru_dev *cru, int slot)
> >   	struct rzg2l_cru_buffer *buf;
> >   	dma_addr_t phys_addr;
> > +	lockdep_assert_held(&cru->hw_lock);
> 
> I think this condition mightn't be true at the point of this commit; as far
> as I can see this function is called without hw_lock being held in
> rzg2l_cru_initialize_axi() until patch 12.

Agree.

Kind Regards,
Tommaso

> 
> > +
> >   	/* A already populated slot shall never be overwritten. */
> >   	if (WARN_ON(cru->queue_buf[slot]))
> >   		return;
> >   	dev_dbg(cru->dev, "Filling HW slot: %d\n", slot);
> > +	guard(spinlock)(&cru->qlock);
> > +
> >   	if (list_empty(&cru->buf_list)) {
> >   		cru->queue_buf[slot] = NULL;
> >   		phys_addr = cru->scratch_phys;
> > @@ -342,7 +348,7 @@ void rzg2l_cru_stop_image_processing(struct rzg2l_cru_dev *cru)
> >   	unsigned int retries = 0;
> >   	u32 icnms;
> > -	scoped_guard(spinlock_irq, &cru->qlock) {
> > +	scoped_guard(spinlock_irq, &cru->hw_lock) {
> >   		/* Disable and clear the interrupt */
> >   		cru->info->disable_interrupts(cru);
> >   	}
> > @@ -560,7 +566,7 @@ irqreturn_t rzg2l_cru_irq(int irq, void *data)
> >   	u32 amnmbs;
> >   	int slot;
> > -	guard(spinlock_irqsave)(&cru->qlock);
> > +	guard(spinlock_irqsave)(&cru->hw_lock);
> >   	irq_status = rzg2l_cru_read(cru, CRUnINTS);
> >   	if (!irq_status)
> > @@ -662,7 +668,7 @@ irqreturn_t rzg3e_cru_irq(int irq, void *data)
> >   	u32 irq_status;
> >   	int slot;
> > -	guard(spinlock)(&cru->qlock);
> > +	guard(spinlock)(&cru->hw_lock);
> >   	irq_status = rzg2l_cru_read(cru, CRUnINTS2);
> >   	if (!irq_status)
> > 
>