[PATCH v1 0/5] media: bcm2835-unicam: Upstreaming various improvements

[PATCH v1 0/5] media: bcm2835-unicam: Upstreaming various improvements

[Naushir Patuck]

Hi,

This patch series contain backports of several fixes/improvements taken from the
donwstream Raspberry Pi kernel tree:

Patch 1/5 improves the handling of frame sequence numbering.
Patch 2/5 allows userland clients to return CSI-2 unpacked formats from Unicam.
Patch 3/5 fixes an intermitent HW bug where the first frame shows corruption.
Patch 4/5 fixes a possibly HW overflow when using the dummy buffer.
Patch 5/5 improves the robustness of the interrupt handling, particulary during
fast framerate operation.

All these patches have been present in the downstream RPi tree for some time now
and have been validated by our users.

Thanks,
Naush

Naushir Patuck (5):
  drivers: media: bcm2835-unicam: Improve frame sequence count handling
  drivers: media: bcm2835-unicam: Allow setting of unpacked formats
  drivers: media: bcm2835-unicam: Disable trigger mode operation
  drivers: media: bcm2835-unicam: Fix for possible dummy buffer overrun
  drivers: media: bcm2835-unicam: Correctly handle FS + FE ISR condition

 .../media/platform/broadcom/bcm2835-unicam.c  | 81 +++++++++++++++----
 1 file changed, 66 insertions(+), 15 deletions(-)

-- 
2.34.1

[PATCH v1 1/5] drivers: media: bcm2835-unicam: Improve frame sequence count handling

[Naushir Patuck]

Ensure that the frame sequence counter is incremented only if a previous
frame start interrupt has occurred, or a frame start + frame end has
occurred simultaneously.

This corresponds the sequence number with the actual number of frames
produced by the sensor, not the number of frame buffers dequeued back
to userland.

Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
---
 .../media/platform/broadcom/bcm2835-unicam.c  | 22 +++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/broadcom/bcm2835-unicam.c b/drivers/media/platform/broadcom/bcm2835-unicam.c
index 3aed0e493c81..36fb186a0421 100644
--- a/drivers/media/platform/broadcom/bcm2835-unicam.c
+++ b/drivers/media/platform/broadcom/bcm2835-unicam.c
@@ -199,6 +199,7 @@ struct unicam_device {
 	/* subdevice async notifier */
 	struct v4l2_async_notifier notifier;
 	unsigned int sequence;
+	bool frame_started;
 
 	/* Sensor node */
 	struct {
@@ -742,6 +743,8 @@ static irqreturn_t unicam_isr(int irq, void *dev)
 	 * buffer forever.
 	 */
 	if (fe) {
+		bool inc_seq = unicam->frame_started;
+
 		/*
 		 * Ensure we have swapped buffers already as we can't
 		 * stop the peripheral. If no buffer is available, use a
@@ -761,11 +764,24 @@ static irqreturn_t unicam_isr(int irq, void *dev)
 			 * + FS + LS). In this case, we cannot signal the buffer
 			 * as complete, as the HW will reuse that buffer.
 			 */
-			if (node->cur_frm && node->cur_frm != node->next_frm)
+			if (node->cur_frm && node->cur_frm != node->next_frm) {
 				unicam_process_buffer_complete(node, sequence);
+				inc_seq = true;
+			}
 			node->cur_frm = node->next_frm;
 		}
-		unicam->sequence++;
+
+		/*
+		 * Increment the sequence number conditionally on either a FS
+		 * having already occurred, or in the FE + FS condition as
+		 * caught in the FE handler above. This ensures the sequence
+		 * number corresponds to the frames generated by the sensor, not
+		 * the frames dequeued to userland.
+		 */
+		if (inc_seq) {
+			unicam->sequence++;
+			unicam->frame_started = false;
+		}
 	}
 
 	if (ista & UNICAM_FSI) {
@@ -795,6 +811,7 @@ static irqreturn_t unicam_isr(int irq, void *dev)
 		}
 
 		unicam_queue_event_sof(unicam);
+		unicam->frame_started = true;
 	}
 
 	/*
@@ -1413,6 +1430,7 @@ static int unicam_sd_enable_streams(struct v4l2_subdev *sd,
 		if (unicam->pipe.nodes & BIT(UNICAM_METADATA_NODE))
 			unicam_start_metadata(unicam);
 
+		unicam->frame_started = false;
 		unicam_start_rx(unicam, state);
 	}
 
-- 
2.34.1

[PATCH v1 2/5] drivers: media: bcm2835-unicam: Allow setting of unpacked formats

[Naushir Patuck]

When matching formats via try_fmt/set_fmt ioctls, test for the unpacked
formats as well as packed formats. This allows userland clients setup
unpacking to 16-bits from the 10/12/14-packed CSI2 formats.

Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
---
 drivers/media/platform/broadcom/bcm2835-unicam.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/broadcom/bcm2835-unicam.c b/drivers/media/platform/broadcom/bcm2835-unicam.c
index 36fb186a0421..d573d4d89881 100644
--- a/drivers/media/platform/broadcom/bcm2835-unicam.c
+++ b/drivers/media/platform/broadcom/bcm2835-unicam.c
@@ -547,7 +547,8 @@ unicam_find_format_by_fourcc(u32 fourcc, u32 pad)
 	}
 
 	for (i = 0; i < num_formats; ++i) {
-		if (formats[i].fourcc == fourcc)
+		if (formats[i].fourcc == fourcc ||
+		    formats[i].unpacked_fourcc == fourcc)
 			return &formats[i];
 	}
 
-- 
2.34.1

[PATCH v1 3/5] drivers: media: bcm2835-unicam: Disable trigger mode operation

[Naushir Patuck]

The imx219/imx708 sensors frequently generate a single corrupt frame
(image or embedded data) when the sensor first starts. This can either
be a missing line, or invalid samples within the line. This only occurrs
using the upstream Unicam kernel driver.

Disabling trigger mode elimiates this corruption. Since trigger mode is
a legacy feature copied from the firmware driver and not expected to be
needed, remove it. Tested on the Raspberry Pi cameras and shows no ill
effects.

Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
---
 drivers/media/platform/broadcom/bcm2835-unicam.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/media/platform/broadcom/bcm2835-unicam.c b/drivers/media/platform/broadcom/bcm2835-unicam.c
index d573d4d89881..550eb1b064f1 100644
--- a/drivers/media/platform/broadcom/bcm2835-unicam.c
+++ b/drivers/media/platform/broadcom/bcm2835-unicam.c
@@ -834,11 +834,6 @@ static irqreturn_t unicam_isr(int irq, void *dev)
 		}
 	}
 
-	if (unicam_reg_read(unicam, UNICAM_ICTL) & UNICAM_FCM) {
-		/* Switch out of trigger mode if selected */
-		unicam_reg_write_field(unicam, UNICAM_ICTL, 1, UNICAM_TFC);
-		unicam_reg_write_field(unicam, UNICAM_ICTL, 0, UNICAM_FCM);
-	}
 	return IRQ_HANDLED;
 }
 
@@ -1002,8 +997,7 @@ static void unicam_start_rx(struct unicam_device *unicam,
 
 	unicam_reg_write_field(unicam, UNICAM_ANA, 0, UNICAM_DDL);
 
-	/* Always start in trigger frame capture mode (UNICAM_FCM set) */
-	val = UNICAM_FSIE | UNICAM_FEIE | UNICAM_FCM | UNICAM_IBOB;
+	val = UNICAM_FSIE | UNICAM_FEIE | UNICAM_IBOB;
 	line_int_freq = max(fmt->height >> 2, 128);
 	unicam_set_field(&val, line_int_freq, UNICAM_LCIE_MASK);
 	unicam_reg_write(unicam, UNICAM_ICTL, val);
-- 
2.34.1

[PATCH v1 4/5] drivers: media: bcm2835-unicam: Fix for possible dummy buffer overrun

[Naushir Patuck]

The Unicam hardware has been observed to cause a buffer overrun when
using the dummy buffer as a circular buffer. The conditions that cause
the overrun are not fully known, but it seems to occur when the memory
bus is heavily loaded.

To avoid the overrun, program the hardware with a buffer size of 0 when
using the dummy buffer. This will cause overrun into the allocated dummy
buffer, but avoid out of bounds writes.

Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
---
 drivers/media/platform/broadcom/bcm2835-unicam.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/broadcom/bcm2835-unicam.c b/drivers/media/platform/broadcom/bcm2835-unicam.c
index 550eb1b064f1..f10064107d54 100644
--- a/drivers/media/platform/broadcom/bcm2835-unicam.c
+++ b/drivers/media/platform/broadcom/bcm2835-unicam.c
@@ -640,7 +640,14 @@ static inline void unicam_reg_write_field(struct unicam_device *unicam, u32 offs
 static void unicam_wr_dma_addr(struct unicam_node *node,
 			       struct unicam_buffer *buf)
 {
-	dma_addr_t endaddr = buf->dma_addr + buf->size;
+	/*
+	 * Due to a HW bug causing buffer overruns in circular buffer mode under
+	 * certain (not yet fully known) conditions, the dummy buffer allocation
+	 * is set to a a single page size, but the hardware gets programmed with
+	 * a buffer size of 0.
+	 */
+	dma_addr_t endaddr = buf->dma_addr +
+			     (buf != &node->dummy_buf ? buf->size : 0);
 
 	if (node->id == UNICAM_IMAGE_NODE) {
 		unicam_reg_write(node->dev, UNICAM_IBSA0, buf->dma_addr);
-- 
2.34.1

[PATCH v1 5/5] drivers: media: bcm2835-unicam: Correctly handle FS + FE ISR condition

[Naushir Patuck]

This change aligns the FS/FE interrupt handling with the Raspberry Pi
kernel downstream Unicam driver.

If we get a simultaneous FS + FE interrupt for the same frame, it cannot
be marked as completed and returned to userland as the framebuffer will
be refilled by Unicam on the next sensor frame. Additionally, the
timestamp will be set to 0 as the FS interrupt handling code will not
have run yet.

To avoid these problems, the frame is considered dropped in the FE
handler, and will be returned to userland on the subsequent sensor frame.

Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
---
 .../media/platform/broadcom/bcm2835-unicam.c  | 39 +++++++++++++++++--
 1 file changed, 35 insertions(+), 4 deletions(-)

diff --git a/drivers/media/platform/broadcom/bcm2835-unicam.c b/drivers/media/platform/broadcom/bcm2835-unicam.c
index f10064107d54..0d2aa25d483f 100644
--- a/drivers/media/platform/broadcom/bcm2835-unicam.c
+++ b/drivers/media/platform/broadcom/bcm2835-unicam.c
@@ -773,10 +773,26 @@ static irqreturn_t unicam_isr(int irq, void *dev)
 			 * as complete, as the HW will reuse that buffer.
 			 */
 			if (node->cur_frm && node->cur_frm != node->next_frm) {
+				/*
+				 * This condition checks if FE + FS for the same
+				 * frame has occurred. In such cases, we cannot
+				 * return out the frame, as no buffer handling
+				 * or timestamping has yet been done as part of
+				 * the FS handler.
+				 */
+				if (!node->cur_frm->vb.vb2_buf.timestamp) {
+					dev_dbg(unicam->v4l2_dev.dev,
+						"ISR: FE without FS, dropping frame\n");
+					continue;
+				}
+
 				unicam_process_buffer_complete(node, sequence);
+				node->cur_frm = node->next_frm;
+				node->next_frm = NULL;
 				inc_seq = true;
+			} else {
+				node->cur_frm = node->next_frm;
 			}
-			node->cur_frm = node->next_frm;
 		}
 
 		/*
@@ -812,10 +828,25 @@ static irqreturn_t unicam_isr(int irq, void *dev)
 					i);
 			/*
 			 * Set the next frame output to go to a dummy frame
-			 * if we have not managed to obtain another frame
-			 * from the queue.
+			 * if no buffer currently queued.
 			 */
-			unicam_schedule_dummy_buffer(node);
+			if (!node->next_frm ||
+			    node->next_frm == node->cur_frm) {
+				unicam_schedule_dummy_buffer(node);
+			} else if (unicam->node[i].cur_frm) {
+				/*
+				 * Repeated FS without FE. Hardware will have
+				 * swapped buffers, but the cur_frm doesn't
+				 * contain valid data. Return cur_frm to the
+				 * queue.
+				 */
+				spin_lock(&node->dma_queue_lock);
+				list_add_tail(&node->cur_frm->list,
+					      &node->dma_queue);
+				spin_unlock(&node->dma_queue_lock);
+				node->cur_frm = node->next_frm;
+				node->next_frm = NULL;
+			}
 		}
 
 		unicam_queue_event_sof(unicam);
-- 
2.34.1

Re: [PATCH v1 5/5] drivers: media: bcm2835-unicam: Correctly handle FS + FE ISR condition

[Jacopo Mondi]

Hi Naush

On Fri, Nov 22, 2024 at 08:41:52AM +0000, Naushir Patuck wrote:
> This change aligns the FS/FE interrupt handling with the Raspberry Pi
> kernel downstream Unicam driver.
>
> If we get a simultaneous FS + FE interrupt for the same frame, it cannot
> be marked as completed and returned to userland as the framebuffer will
> be refilled by Unicam on the next sensor frame. Additionally, the
> timestamp will be set to 0 as the FS interrupt handling code will not
> have run yet.
>
> To avoid these problems, the frame is considered dropped in the FE
> handler, and will be returned to userland on the subsequent sensor frame.
>
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> ---
>  .../media/platform/broadcom/bcm2835-unicam.c  | 39 +++++++++++++++++--
>  1 file changed, 35 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/media/platform/broadcom/bcm2835-unicam.c b/drivers/media/platform/broadcom/bcm2835-unicam.c
> index f10064107d54..0d2aa25d483f 100644
> --- a/drivers/media/platform/broadcom/bcm2835-unicam.c
> +++ b/drivers/media/platform/broadcom/bcm2835-unicam.c
> @@ -773,10 +773,26 @@ static irqreturn_t unicam_isr(int irq, void *dev)
>  			 * as complete, as the HW will reuse that buffer.
>  			 */
>  			if (node->cur_frm && node->cur_frm != node->next_frm) {
> +				/*
> +				 * This condition checks if FE + FS for the same
> +				 * frame has occurred. In such cases, we cannot
> +				 * return out the frame, as no buffer handling
> +				 * or timestamping has yet been done as part of
> +				 * the FS handler.
> +				 */
> +				if (!node->cur_frm->vb.vb2_buf.timestamp) {
> +					dev_dbg(unicam->v4l2_dev.dev,
> +						"ISR: FE without FS, dropping frame\n");
> +					continue;
> +				}
> +
>  				unicam_process_buffer_complete(node, sequence);
> +				node->cur_frm = node->next_frm;
> +				node->next_frm = NULL;
>  				inc_seq = true;
> +			} else {
> +				node->cur_frm = node->next_frm;
>  			}
> -			node->cur_frm = node->next_frm;
>  		}
>
>  		/*
> @@ -812,10 +828,25 @@ static irqreturn_t unicam_isr(int irq, void *dev)
>  					i);
>  			/*
>  			 * Set the next frame output to go to a dummy frame
> -			 * if we have not managed to obtain another frame
> -			 * from the queue.
> +			 * if no buffer currently queued.
>  			 */
> -			unicam_schedule_dummy_buffer(node);
> +			if (!node->next_frm ||
> +			    node->next_frm == node->cur_frm) {
> +				unicam_schedule_dummy_buffer(node);
> +			} else if (unicam->node[i].cur_frm) {
> +				/*
> +				 * Repeated FS without FE. Hardware will have
> +				 * swapped buffers, but the cur_frm doesn't
> +				 * contain valid data. Return cur_frm to the
> +				 * queue.

So the buffer gets silently recycled ? Or should it be returned with
errors to userspace ?

> +				 */
> +				spin_lock(&node->dma_queue_lock);
> +				list_add_tail(&node->cur_frm->list,
> +					      &node->dma_queue);
> +				spin_unlock(&node->dma_queue_lock);
> +				node->cur_frm = node->next_frm;
> +				node->next_frm = NULL;
> +			}
>  		}
>
>  		unicam_queue_event_sof(unicam);
> --
> 2.34.1
>
>

Re: [PATCH v1 2/5] drivers: media: bcm2835-unicam: Allow setting of unpacked formats

[Jacopo Mondi]

Hi Naush

On Fri, Nov 22, 2024 at 08:41:49AM +0000, Naushir Patuck wrote:
> When matching formats via try_fmt/set_fmt ioctls, test for the unpacked
> formats as well as packed formats. This allows userland clients setup
> unpacking to 16-bits from the 10/12/14-packed CSI2 formats.
>
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>

Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

Thanks
  j

> ---
>  drivers/media/platform/broadcom/bcm2835-unicam.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/platform/broadcom/bcm2835-unicam.c b/drivers/media/platform/broadcom/bcm2835-unicam.c
> index 36fb186a0421..d573d4d89881 100644
> --- a/drivers/media/platform/broadcom/bcm2835-unicam.c
> +++ b/drivers/media/platform/broadcom/bcm2835-unicam.c
> @@ -547,7 +547,8 @@ unicam_find_format_by_fourcc(u32 fourcc, u32 pad)
>  	}
>
>  	for (i = 0; i < num_formats; ++i) {
> -		if (formats[i].fourcc == fourcc)
> +		if (formats[i].fourcc == fourcc ||
> +		    formats[i].unpacked_fourcc == fourcc)
>  			return &formats[i];
>  	}
>
> --
> 2.34.1
>
>

Re: [PATCH v1 3/5] drivers: media: bcm2835-unicam: Disable trigger mode operation

[Jacopo Mondi]

Hi Naush

On Fri, Nov 22, 2024 at 08:41:50AM +0000, Naushir Patuck wrote:
> The imx219/imx708 sensors frequently generate a single corrupt frame
> (image or embedded data) when the sensor first starts. This can either
> be a missing line, or invalid samples within the line. This only occurrs
> using the upstream Unicam kernel driver.

I think the last statement can b dropped.

>
> Disabling trigger mode elimiates this corruption. Since trigger mode is
> a legacy feature copied from the firmware driver and not expected to be
> needed, remove it. Tested on the Raspberry Pi cameras and shows no ill
> effects.
>
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>

Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

Thanks
  j

> ---
>  drivers/media/platform/broadcom/bcm2835-unicam.c | 8 +-------
>  1 file changed, 1 insertion(+), 7 deletions(-)
>
> diff --git a/drivers/media/platform/broadcom/bcm2835-unicam.c b/drivers/media/platform/broadcom/bcm2835-unicam.c
> index d573d4d89881..550eb1b064f1 100644
> --- a/drivers/media/platform/broadcom/bcm2835-unicam.c
> +++ b/drivers/media/platform/broadcom/bcm2835-unicam.c
> @@ -834,11 +834,6 @@ static irqreturn_t unicam_isr(int irq, void *dev)
>  		}
>  	}
>
> -	if (unicam_reg_read(unicam, UNICAM_ICTL) & UNICAM_FCM) {
> -		/* Switch out of trigger mode if selected */
> -		unicam_reg_write_field(unicam, UNICAM_ICTL, 1, UNICAM_TFC);
> -		unicam_reg_write_field(unicam, UNICAM_ICTL, 0, UNICAM_FCM);
> -	}
>  	return IRQ_HANDLED;
>  }
>
> @@ -1002,8 +997,7 @@ static void unicam_start_rx(struct unicam_device *unicam,
>
>  	unicam_reg_write_field(unicam, UNICAM_ANA, 0, UNICAM_DDL);
>
> -	/* Always start in trigger frame capture mode (UNICAM_FCM set) */
> -	val = UNICAM_FSIE | UNICAM_FEIE | UNICAM_FCM | UNICAM_IBOB;
> +	val = UNICAM_FSIE | UNICAM_FEIE | UNICAM_IBOB;
>  	line_int_freq = max(fmt->height >> 2, 128);
>  	unicam_set_field(&val, line_int_freq, UNICAM_LCIE_MASK);
>  	unicam_reg_write(unicam, UNICAM_ICTL, val);
> --
> 2.34.1
>
>

Re: [PATCH v1 4/5] drivers: media: bcm2835-unicam: Fix for possible dummy buffer overrun

[Jacopo Mondi]

Hi Naush

On Fri, Nov 22, 2024 at 08:41:51AM +0000, Naushir Patuck wrote:
> The Unicam hardware has been observed to cause a buffer overrun when
> using the dummy buffer as a circular buffer. The conditions that cause
> the overrun are not fully known, but it seems to occur when the memory
> bus is heavily loaded.
>
> To avoid the overrun, program the hardware with a buffer size of 0 when
> using the dummy buffer. This will cause overrun into the allocated dummy
> buffer, but avoid out of bounds writes.
>
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> ---
>  drivers/media/platform/broadcom/bcm2835-unicam.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/platform/broadcom/bcm2835-unicam.c b/drivers/media/platform/broadcom/bcm2835-unicam.c
> index 550eb1b064f1..f10064107d54 100644
> --- a/drivers/media/platform/broadcom/bcm2835-unicam.c
> +++ b/drivers/media/platform/broadcom/bcm2835-unicam.c
> @@ -640,7 +640,14 @@ static inline void unicam_reg_write_field(struct unicam_device *unicam, u32 offs
>  static void unicam_wr_dma_addr(struct unicam_node *node,
>  			       struct unicam_buffer *buf)
>  {
> -	dma_addr_t endaddr = buf->dma_addr + buf->size;
> +	/*
> +	 * Due to a HW bug causing buffer overruns in circular buffer mode under
> +	 * certain (not yet fully known) conditions, the dummy buffer allocation
> +	 * is set to a a single page size, but the hardware gets programmed with
> +	 * a buffer size of 0.
> +	 */
> +	dma_addr_t endaddr = buf->dma_addr +
> +			     (buf != &node->dummy_buf ? buf->size : 0);

So the DMA engine doesn't actually write any data to dummy_buf
anymore ?

Does it still need to be allocated at all ? Or can we simply set the
dma transfer size to 0 ?

>
>  	if (node->id == UNICAM_IMAGE_NODE) {
>  		unicam_reg_write(node->dev, UNICAM_IBSA0, buf->dma_addr);
> --
> 2.34.1
>
>

Re: [PATCH v1 4/5] drivers: media: bcm2835-unicam: Fix for possible dummy buffer overrun

[Naushir Patuck]

Hi Jacopo,

On Fri, 22 Nov 2024 at 11:20, Jacopo Mondi
<jacopo.mondi@ideasonboard.com> wrote:
>
> Hi Naush
>
> On Fri, Nov 22, 2024 at 08:41:51AM +0000, Naushir Patuck wrote:
> > The Unicam hardware has been observed to cause a buffer overrun when
> > using the dummy buffer as a circular buffer. The conditions that cause
> > the overrun are not fully known, but it seems to occur when the memory
> > bus is heavily loaded.
> >
> > To avoid the overrun, program the hardware with a buffer size of 0 when
> > using the dummy buffer. This will cause overrun into the allocated dummy
> > buffer, but avoid out of bounds writes.
> >
> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > ---
> >  drivers/media/platform/broadcom/bcm2835-unicam.c | 9 ++++++++-
> >  1 file changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/platform/broadcom/bcm2835-unicam.c b/drivers/media/platform/broadcom/bcm2835-unicam.c
> > index 550eb1b064f1..f10064107d54 100644
> > --- a/drivers/media/platform/broadcom/bcm2835-unicam.c
> > +++ b/drivers/media/platform/broadcom/bcm2835-unicam.c
> > @@ -640,7 +640,14 @@ static inline void unicam_reg_write_field(struct unicam_device *unicam, u32 offs
> >  static void unicam_wr_dma_addr(struct unicam_node *node,
> >                              struct unicam_buffer *buf)
> >  {
> > -     dma_addr_t endaddr = buf->dma_addr + buf->size;
> > +     /*
> > +      * Due to a HW bug causing buffer overruns in circular buffer mode under
> > +      * certain (not yet fully known) conditions, the dummy buffer allocation
> > +      * is set to a a single page size, but the hardware gets programmed with
> > +      * a buffer size of 0.
> > +      */
> > +     dma_addr_t endaddr = buf->dma_addr +
> > +                          (buf != &node->dummy_buf ? buf->size : 0);
>
> So the DMA engine doesn't actually write any data to dummy_buf
> anymore ?
>
>
> Does it still need to be allocated at all ? Or can we simply set the
> dma transfer size to 0 ?

The DMA engine does still write to the buffer, so the allocation needs
to occur. The zero size programmed into the register is a quirk of the
HW itself, and is used to ensure the write wrap correctly in the
buffer.

Naush

>
> >
> >       if (node->id == UNICAM_IMAGE_NODE) {
> >               unicam_reg_write(node->dev, UNICAM_IBSA0, buf->dma_addr);
> > --
> > 2.34.1
> >
> >

Re: [PATCH v1 5/5] drivers: media: bcm2835-unicam: Correctly handle FS + FE ISR condition

[Naushir Patuck]

Hi Jacopo,

On Fri, 22 Nov 2024 at 11:16, Jacopo Mondi
<jacopo.mondi@ideasonboard.com> wrote:
>
> Hi Naush
>
> On Fri, Nov 22, 2024 at 08:41:52AM +0000, Naushir Patuck wrote:
> > This change aligns the FS/FE interrupt handling with the Raspberry Pi
> > kernel downstream Unicam driver.
> >
> > If we get a simultaneous FS + FE interrupt for the same frame, it cannot
> > be marked as completed and returned to userland as the framebuffer will
> > be refilled by Unicam on the next sensor frame. Additionally, the
> > timestamp will be set to 0 as the FS interrupt handling code will not
> > have run yet.
> >
> > To avoid these problems, the frame is considered dropped in the FE
> > handler, and will be returned to userland on the subsequent sensor frame.
> >
> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > ---
> >  .../media/platform/broadcom/bcm2835-unicam.c  | 39 +++++++++++++++++--
> >  1 file changed, 35 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/media/platform/broadcom/bcm2835-unicam.c b/drivers/media/platform/broadcom/bcm2835-unicam.c
> > index f10064107d54..0d2aa25d483f 100644
> > --- a/drivers/media/platform/broadcom/bcm2835-unicam.c
> > +++ b/drivers/media/platform/broadcom/bcm2835-unicam.c
> > @@ -773,10 +773,26 @@ static irqreturn_t unicam_isr(int irq, void *dev)
> >                        * as complete, as the HW will reuse that buffer.
> >                        */
> >                       if (node->cur_frm && node->cur_frm != node->next_frm) {
> > +                             /*
> > +                              * This condition checks if FE + FS for the same
> > +                              * frame has occurred. In such cases, we cannot
> > +                              * return out the frame, as no buffer handling
> > +                              * or timestamping has yet been done as part of
> > +                              * the FS handler.
> > +                              */
> > +                             if (!node->cur_frm->vb.vb2_buf.timestamp) {
> > +                                     dev_dbg(unicam->v4l2_dev.dev,
> > +                                             "ISR: FE without FS, dropping frame\n");
> > +                                     continue;
> > +                             }
> > +
> >                               unicam_process_buffer_complete(node, sequence);
> > +                             node->cur_frm = node->next_frm;
> > +                             node->next_frm = NULL;
> >                               inc_seq = true;
> > +                     } else {
> > +                             node->cur_frm = node->next_frm;
> >                       }
> > -                     node->cur_frm = node->next_frm;
> >               }
> >
> >               /*
> > @@ -812,10 +828,25 @@ static irqreturn_t unicam_isr(int irq, void *dev)
> >                                       i);
> >                       /*
> >                        * Set the next frame output to go to a dummy frame
> > -                      * if we have not managed to obtain another frame
> > -                      * from the queue.
> > +                      * if no buffer currently queued.
> >                        */
> > -                     unicam_schedule_dummy_buffer(node);
> > +                     if (!node->next_frm ||
> > +                         node->next_frm == node->cur_frm) {
> > +                             unicam_schedule_dummy_buffer(node);
> > +                     } else if (unicam->node[i].cur_frm) {
> > +                             /*
> > +                              * Repeated FS without FE. Hardware will have
> > +                              * swapped buffers, but the cur_frm doesn't
> > +                              * contain valid data. Return cur_frm to the
> > +                              * queue.
>
> So the buffer gets silently recycled ? Or should it be returned with
> errors to userspace ?

The buffer silently gets recycled and we dequeue when we are sure it
is valid and will not get overwritten.  If we were to return to
userspace with an error, there is still a race condition on the name
frame/buffer which will also have to return as error.

Regards,
Naush


>
> > +                              */
> > +                             spin_lock(&node->dma_queue_lock);
> > +                             list_add_tail(&node->cur_frm->list,
> > +                                           &node->dma_queue);
> > +                             spin_unlock(&node->dma_queue_lock);
> > +                             node->cur_frm = node->next_frm;
> > +                             node->next_frm = NULL;
> > +                     }
> >               }
> >
> >               unicam_queue_event_sof(unicam);
> > --
> > 2.34.1
> >
> >

Re: [PATCH v1 5/5] drivers: media: bcm2835-unicam: Correctly handle FS + FE ISR condition

[Jacopo Mondi]

Hi Naush

On Fri, Nov 22, 2024 at 11:40:26AM +0000, Naushir Patuck wrote:
> Hi Jacopo,
>
> On Fri, 22 Nov 2024 at 11:16, Jacopo Mondi
> <jacopo.mondi@ideasonboard.com> wrote:
> >
> > Hi Naush
> >
> > On Fri, Nov 22, 2024 at 08:41:52AM +0000, Naushir Patuck wrote:
> > > This change aligns the FS/FE interrupt handling with the Raspberry Pi
> > > kernel downstream Unicam driver.
> > >
> > > If we get a simultaneous FS + FE interrupt for the same frame, it cannot
> > > be marked as completed and returned to userland as the framebuffer will
> > > be refilled by Unicam on the next sensor frame. Additionally, the
> > > timestamp will be set to 0 as the FS interrupt handling code will not
> > > have run yet.
> > >
> > > To avoid these problems, the frame is considered dropped in the FE
> > > handler, and will be returned to userland on the subsequent sensor frame.
> > >
> > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > > ---
> > >  .../media/platform/broadcom/bcm2835-unicam.c  | 39 +++++++++++++++++--
> > >  1 file changed, 35 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/media/platform/broadcom/bcm2835-unicam.c b/drivers/media/platform/broadcom/bcm2835-unicam.c
> > > index f10064107d54..0d2aa25d483f 100644
> > > --- a/drivers/media/platform/broadcom/bcm2835-unicam.c
> > > +++ b/drivers/media/platform/broadcom/bcm2835-unicam.c
> > > @@ -773,10 +773,26 @@ static irqreturn_t unicam_isr(int irq, void *dev)
> > >                        * as complete, as the HW will reuse that buffer.
> > >                        */
> > >                       if (node->cur_frm && node->cur_frm != node->next_frm) {
> > > +                             /*
> > > +                              * This condition checks if FE + FS for the same
> > > +                              * frame has occurred. In such cases, we cannot
> > > +                              * return out the frame, as no buffer handling
> > > +                              * or timestamping has yet been done as part of
> > > +                              * the FS handler.
> > > +                              */
> > > +                             if (!node->cur_frm->vb.vb2_buf.timestamp) {
> > > +                                     dev_dbg(unicam->v4l2_dev.dev,
> > > +                                             "ISR: FE without FS, dropping frame\n");
> > > +                                     continue;
> > > +                             }
> > > +
> > >                               unicam_process_buffer_complete(node, sequence);
> > > +                             node->cur_frm = node->next_frm;
> > > +                             node->next_frm = NULL;
> > >                               inc_seq = true;
> > > +                     } else {
> > > +                             node->cur_frm = node->next_frm;
> > >                       }
> > > -                     node->cur_frm = node->next_frm;
> > >               }
> > >
> > >               /*
> > > @@ -812,10 +828,25 @@ static irqreturn_t unicam_isr(int irq, void *dev)
> > >                                       i);
> > >                       /*
> > >                        * Set the next frame output to go to a dummy frame
> > > -                      * if we have not managed to obtain another frame
> > > -                      * from the queue.
> > > +                      * if no buffer currently queued.
> > >                        */
> > > -                     unicam_schedule_dummy_buffer(node);
> > > +                     if (!node->next_frm ||
> > > +                         node->next_frm == node->cur_frm) {
> > > +                             unicam_schedule_dummy_buffer(node);
> > > +                     } else if (unicam->node[i].cur_frm) {
> > > +                             /*
> > > +                              * Repeated FS without FE. Hardware will have
> > > +                              * swapped buffers, but the cur_frm doesn't
> > > +                              * contain valid data. Return cur_frm to the
> > > +                              * queue.
> >
> > So the buffer gets silently recycled ? Or should it be returned with
> > errors to userspace ?
>
> The buffer silently gets recycled and we dequeue when we are sure it
> is valid and will not get overwritten.  If we were to return to

I haven't find in the v4l2 specs any reference to what the behaviour
should be.

If I can summarize it: When a frame capture is aborted after the DMA
transfer has already started, should the corresponding capture buffer
be returned to the user in error state or the frame drop can go
silently ignored ?

Cc-ing Hans Sakari and Laurent for opinions.

> userspace with an error, there is still a race condition on the name
> frame/buffer which will also have to return as error.
>

I'm sorry I didn't get this part :)

> Regards,
> Naush
>
>
> >
> > > +                              */
> > > +                             spin_lock(&node->dma_queue_lock);
> > > +                             list_add_tail(&node->cur_frm->list,
> > > +                                           &node->dma_queue);
> > > +                             spin_unlock(&node->dma_queue_lock);
> > > +                             node->cur_frm = node->next_frm;
> > > +                             node->next_frm = NULL;
> > > +                     }
> > >               }
> > >
> > >               unicam_queue_event_sof(unicam);
> > > --
> > > 2.34.1
> > >
> > >
>

Re: [PATCH v1 4/5] drivers: media: bcm2835-unicam: Fix for possible dummy buffer overrun

[Jacopo Mondi]

Hi Naush

On Fri, Nov 22, 2024 at 11:35:59AM +0000, Naushir Patuck wrote:
> Hi Jacopo,

Thanks for the explanation

>
> On Fri, 22 Nov 2024 at 11:20, Jacopo Mondi
> <jacopo.mondi@ideasonboard.com> wrote:
> >
> > Hi Naush
> >
> > On Fri, Nov 22, 2024 at 08:41:51AM +0000, Naushir Patuck wrote:
> > > The Unicam hardware has been observed to cause a buffer overrun when
> > > using the dummy buffer as a circular buffer. The conditions that cause
> > > the overrun are not fully known, but it seems to occur when the memory
> > > bus is heavily loaded.
> > >
> > > To avoid the overrun, program the hardware with a buffer size of 0 when
> > > using the dummy buffer. This will cause overrun into the allocated dummy
> > > buffer, but avoid out of bounds writes.
> > >
> > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>

Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

Thanks
  j

> > > ---
> > >  drivers/media/platform/broadcom/bcm2835-unicam.c | 9 ++++++++-
> > >  1 file changed, 8 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/media/platform/broadcom/bcm2835-unicam.c b/drivers/media/platform/broadcom/bcm2835-unicam.c
> > > index 550eb1b064f1..f10064107d54 100644
> > > --- a/drivers/media/platform/broadcom/bcm2835-unicam.c
> > > +++ b/drivers/media/platform/broadcom/bcm2835-unicam.c
> > > @@ -640,7 +640,14 @@ static inline void unicam_reg_write_field(struct unicam_device *unicam, u32 offs
> > >  static void unicam_wr_dma_addr(struct unicam_node *node,
> > >                              struct unicam_buffer *buf)
> > >  {
> > > -     dma_addr_t endaddr = buf->dma_addr + buf->size;
> > > +     /*
> > > +      * Due to a HW bug causing buffer overruns in circular buffer mode under
> > > +      * certain (not yet fully known) conditions, the dummy buffer allocation
> > > +      * is set to a a single page size, but the hardware gets programmed with
> > > +      * a buffer size of 0.
> > > +      */
> > > +     dma_addr_t endaddr = buf->dma_addr +
> > > +                          (buf != &node->dummy_buf ? buf->size : 0);
> >
> > So the DMA engine doesn't actually write any data to dummy_buf
> > anymore ?
> >
> >
> > Does it still need to be allocated at all ? Or can we simply set the
> > dma transfer size to 0 ?
>
> The DMA engine does still write to the buffer, so the allocation needs
> to occur. The zero size programmed into the register is a quirk of the
> HW itself, and is used to ensure the write wrap correctly in the
> buffer.
>
> Naush
>
> >
> > >
> > >       if (node->id == UNICAM_IMAGE_NODE) {
> > >               unicam_reg_write(node->dev, UNICAM_IBSA0, buf->dma_addr);
> > > --
> > > 2.34.1
> > >
> > >
>

Re: [PATCH v1 1/5] drivers: media: bcm2835-unicam: Improve frame sequence count handling

[Jacopo Mondi]

Hi Naush

On Fri, Nov 22, 2024 at 08:41:48AM +0000, Naushir Patuck wrote:
> Ensure that the frame sequence counter is incremented only if a previous
> frame start interrupt has occurred, or a frame start + frame end has
> occurred simultaneously.
>
> This corresponds the sequence number with the actual number of frames
> produced by the sensor, not the number of frame buffers dequeued back
> to userland.
>
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>


Thanks, looks good to me
Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

> ---
>  .../media/platform/broadcom/bcm2835-unicam.c  | 22 +++++++++++++++++--
>  1 file changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/platform/broadcom/bcm2835-unicam.c b/drivers/media/platform/broadcom/bcm2835-unicam.c
> index 3aed0e493c81..36fb186a0421 100644
> --- a/drivers/media/platform/broadcom/bcm2835-unicam.c
> +++ b/drivers/media/platform/broadcom/bcm2835-unicam.c
> @@ -199,6 +199,7 @@ struct unicam_device {
>  	/* subdevice async notifier */
>  	struct v4l2_async_notifier notifier;
>  	unsigned int sequence;
> +	bool frame_started;
>
>  	/* Sensor node */
>  	struct {
> @@ -742,6 +743,8 @@ static irqreturn_t unicam_isr(int irq, void *dev)
>  	 * buffer forever.
>  	 */
>  	if (fe) {
> +		bool inc_seq = unicam->frame_started;
> +
>  		/*
>  		 * Ensure we have swapped buffers already as we can't
>  		 * stop the peripheral. If no buffer is available, use a
> @@ -761,11 +764,24 @@ static irqreturn_t unicam_isr(int irq, void *dev)
>  			 * + FS + LS). In this case, we cannot signal the buffer
>  			 * as complete, as the HW will reuse that buffer.
>  			 */
> -			if (node->cur_frm && node->cur_frm != node->next_frm)
> +			if (node->cur_frm && node->cur_frm != node->next_frm) {
>  				unicam_process_buffer_complete(node, sequence);
> +				inc_seq = true;
> +			}
>  			node->cur_frm = node->next_frm;
>  		}
> -		unicam->sequence++;
> +
> +		/*
> +		 * Increment the sequence number conditionally on either a FS
> +		 * having already occurred, or in the FE + FS condition as
> +		 * caught in the FE handler above. This ensures the sequence
> +		 * number corresponds to the frames generated by the sensor, not
> +		 * the frames dequeued to userland.
> +		 */
> +		if (inc_seq) {
> +			unicam->sequence++;
> +			unicam->frame_started = false;
> +		}
>  	}
>
>  	if (ista & UNICAM_FSI) {
> @@ -795,6 +811,7 @@ static irqreturn_t unicam_isr(int irq, void *dev)
>  		}
>
>  		unicam_queue_event_sof(unicam);
> +		unicam->frame_started = true;
>  	}
>
>  	/*
> @@ -1413,6 +1430,7 @@ static int unicam_sd_enable_streams(struct v4l2_subdev *sd,
>  		if (unicam->pipe.nodes & BIT(UNICAM_METADATA_NODE))
>  			unicam_start_metadata(unicam);
>
> +		unicam->frame_started = false;
>  		unicam_start_rx(unicam, state);
>  	}
>
> --
> 2.34.1
>
>

Re: [PATCH v1 5/5] drivers: media: bcm2835-unicam: Correctly handle FS + FE ISR condition

[Jacopo Mondi]

With Hans Sakari and Laurent in cc for real now

On Fri, Nov 22, 2024 at 03:41:31PM +0100, Jacopo Mondi wrote:
> Hi Naush
>
> On Fri, Nov 22, 2024 at 11:40:26AM +0000, Naushir Patuck wrote:
> > Hi Jacopo,
> >
> > On Fri, 22 Nov 2024 at 11:16, Jacopo Mondi
> > <jacopo.mondi@ideasonboard.com> wrote:
> > >
> > > Hi Naush
> > >
> > > On Fri, Nov 22, 2024 at 08:41:52AM +0000, Naushir Patuck wrote:
> > > > This change aligns the FS/FE interrupt handling with the Raspberry Pi
> > > > kernel downstream Unicam driver.
> > > >
> > > > If we get a simultaneous FS + FE interrupt for the same frame, it cannot
> > > > be marked as completed and returned to userland as the framebuffer will
> > > > be refilled by Unicam on the next sensor frame. Additionally, the
> > > > timestamp will be set to 0 as the FS interrupt handling code will not
> > > > have run yet.
> > > >
> > > > To avoid these problems, the frame is considered dropped in the FE
> > > > handler, and will be returned to userland on the subsequent sensor frame.
> > > >
> > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > > > ---
> > > >  .../media/platform/broadcom/bcm2835-unicam.c  | 39 +++++++++++++++++--
> > > >  1 file changed, 35 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/drivers/media/platform/broadcom/bcm2835-unicam.c b/drivers/media/platform/broadcom/bcm2835-unicam.c
> > > > index f10064107d54..0d2aa25d483f 100644
> > > > --- a/drivers/media/platform/broadcom/bcm2835-unicam.c
> > > > +++ b/drivers/media/platform/broadcom/bcm2835-unicam.c
> > > > @@ -773,10 +773,26 @@ static irqreturn_t unicam_isr(int irq, void *dev)
> > > >                        * as complete, as the HW will reuse that buffer.
> > > >                        */
> > > >                       if (node->cur_frm && node->cur_frm != node->next_frm) {
> > > > +                             /*
> > > > +                              * This condition checks if FE + FS for the same
> > > > +                              * frame has occurred. In such cases, we cannot
> > > > +                              * return out the frame, as no buffer handling
> > > > +                              * or timestamping has yet been done as part of
> > > > +                              * the FS handler.
> > > > +                              */
> > > > +                             if (!node->cur_frm->vb.vb2_buf.timestamp) {
> > > > +                                     dev_dbg(unicam->v4l2_dev.dev,
> > > > +                                             "ISR: FE without FS, dropping frame\n");
> > > > +                                     continue;
> > > > +                             }
> > > > +
> > > >                               unicam_process_buffer_complete(node, sequence);
> > > > +                             node->cur_frm = node->next_frm;
> > > > +                             node->next_frm = NULL;
> > > >                               inc_seq = true;
> > > > +                     } else {
> > > > +                             node->cur_frm = node->next_frm;
> > > >                       }
> > > > -                     node->cur_frm = node->next_frm;
> > > >               }
> > > >
> > > >               /*
> > > > @@ -812,10 +828,25 @@ static irqreturn_t unicam_isr(int irq, void *dev)
> > > >                                       i);
> > > >                       /*
> > > >                        * Set the next frame output to go to a dummy frame
> > > > -                      * if we have not managed to obtain another frame
> > > > -                      * from the queue.
> > > > +                      * if no buffer currently queued.
> > > >                        */
> > > > -                     unicam_schedule_dummy_buffer(node);
> > > > +                     if (!node->next_frm ||
> > > > +                         node->next_frm == node->cur_frm) {
> > > > +                             unicam_schedule_dummy_buffer(node);
> > > > +                     } else if (unicam->node[i].cur_frm) {
> > > > +                             /*
> > > > +                              * Repeated FS without FE. Hardware will have
> > > > +                              * swapped buffers, but the cur_frm doesn't
> > > > +                              * contain valid data. Return cur_frm to the
> > > > +                              * queue.
> > >
> > > So the buffer gets silently recycled ? Or should it be returned with
> > > errors to userspace ?
> >
> > The buffer silently gets recycled and we dequeue when we are sure it
> > is valid and will not get overwritten.  If we were to return to
>
> I haven't find in the v4l2 specs any reference to what the behaviour
> should be.
>
> If I can summarize it: When a frame capture is aborted after the DMA
> transfer has already started, should the corresponding capture buffer
> be returned to the user in error state or the frame drop can go
> silently ignored ?
>
> Cc-ing Hans Sakari and Laurent for opinions.
>
> > userspace with an error, there is still a race condition on the name
> > frame/buffer which will also have to return as error.
> >
>
> I'm sorry I didn't get this part :)
>
> > Regards,
> > Naush
> >
> >
> > >
> > > > +                              */
> > > > +                             spin_lock(&node->dma_queue_lock);
> > > > +                             list_add_tail(&node->cur_frm->list,
> > > > +                                           &node->dma_queue);
> > > > +                             spin_unlock(&node->dma_queue_lock);
> > > > +                             node->cur_frm = node->next_frm;
> > > > +                             node->next_frm = NULL;
> > > > +                     }
> > > >               }
> > > >
> > > >               unicam_queue_event_sof(unicam);
> > > > --
> > > > 2.34.1
> > > >
> > > >
> >

Re: [PATCH v1 5/5] drivers: media: bcm2835-unicam: Correctly handle FS + FE ISR condition

[Laurent Pinchart]

On Fri, Nov 22, 2024 at 03:48:11PM +0100, Jacopo Mondi wrote:
> 
> With Hans Sakari and Laurent in cc for real now
> 
> On Fri, Nov 22, 2024 at 03:41:31PM +0100, Jacopo Mondi wrote:
> > On Fri, Nov 22, 2024 at 11:40:26AM +0000, Naushir Patuck wrote:
> > > On Fri, 22 Nov 2024 at 11:16, Jacopo Mondi wrote:
> > > > On Fri, Nov 22, 2024 at 08:41:52AM +0000, Naushir Patuck wrote:
> > > > > This change aligns the FS/FE interrupt handling with the Raspberry Pi
> > > > > kernel downstream Unicam driver.
> > > > >
> > > > > If we get a simultaneous FS + FE interrupt for the same frame, it cannot
> > > > > be marked as completed and returned to userland as the framebuffer will
> > > > > be refilled by Unicam on the next sensor frame. Additionally, the
> > > > > timestamp will be set to 0 as the FS interrupt handling code will not
> > > > > have run yet.
> > > > >
> > > > > To avoid these problems, the frame is considered dropped in the FE
> > > > > handler, and will be returned to userland on the subsequent sensor frame.
> > > > >
> > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > > > > ---
> > > > >  .../media/platform/broadcom/bcm2835-unicam.c  | 39 +++++++++++++++++--
> > > > >  1 file changed, 35 insertions(+), 4 deletions(-)
> > > > >
> > > > > diff --git a/drivers/media/platform/broadcom/bcm2835-unicam.c b/drivers/media/platform/broadcom/bcm2835-unicam.c
> > > > > index f10064107d54..0d2aa25d483f 100644
> > > > > --- a/drivers/media/platform/broadcom/bcm2835-unicam.c
> > > > > +++ b/drivers/media/platform/broadcom/bcm2835-unicam.c
> > > > > @@ -773,10 +773,26 @@ static irqreturn_t unicam_isr(int irq, void *dev)
> > > > >                        * as complete, as the HW will reuse that buffer.
> > > > >                        */
> > > > >                       if (node->cur_frm && node->cur_frm != node->next_frm) {
> > > > > +                             /*
> > > > > +                              * This condition checks if FE + FS for the same
> > > > > +                              * frame has occurred. In such cases, we cannot
> > > > > +                              * return out the frame, as no buffer handling
> > > > > +                              * or timestamping has yet been done as part of
> > > > > +                              * the FS handler.
> > > > > +                              */
> > > > > +                             if (!node->cur_frm->vb.vb2_buf.timestamp) {
> > > > > +                                     dev_dbg(unicam->v4l2_dev.dev,
> > > > > +                                             "ISR: FE without FS, dropping frame\n");
> > > > > +                                     continue;
> > > > > +                             }
> > > > > +
> > > > >                               unicam_process_buffer_complete(node, sequence);
> > > > > +                             node->cur_frm = node->next_frm;
> > > > > +                             node->next_frm = NULL;
> > > > >                               inc_seq = true;
> > > > > +                     } else {
> > > > > +                             node->cur_frm = node->next_frm;
> > > > >                       }
> > > > > -                     node->cur_frm = node->next_frm;
> > > > >               }
> > > > >
> > > > >               /*
> > > > > @@ -812,10 +828,25 @@ static irqreturn_t unicam_isr(int irq, void *dev)
> > > > >                                       i);
> > > > >                       /*
> > > > >                        * Set the next frame output to go to a dummy frame
> > > > > -                      * if we have not managed to obtain another frame
> > > > > -                      * from the queue.
> > > > > +                      * if no buffer currently queued.
> > > > >                        */
> > > > > -                     unicam_schedule_dummy_buffer(node);
> > > > > +                     if (!node->next_frm ||
> > > > > +                         node->next_frm == node->cur_frm) {
> > > > > +                             unicam_schedule_dummy_buffer(node);
> > > > > +                     } else if (unicam->node[i].cur_frm) {
> > > > > +                             /*
> > > > > +                              * Repeated FS without FE. Hardware will have
> > > > > +                              * swapped buffers, but the cur_frm doesn't
> > > > > +                              * contain valid data. Return cur_frm to the
> > > > > +                              * queue.
> > > >
> > > > So the buffer gets silently recycled ? Or should it be returned with
> > > > errors to userspace ?
> > >
> > > The buffer silently gets recycled and we dequeue when we are sure it
> > > is valid and will not get overwritten.  If we were to return to
> >
> > I haven't find in the v4l2 specs any reference to what the behaviour
> > should be.
> >
> > If I can summarize it: When a frame capture is aborted after the DMA
> > transfer has already started, should the corresponding capture buffer
> > be returned to the user in error state or the frame drop can go
> > silently ignored ?

If the DMA tranfer is aborted, I would return the buffer to userspace.
This will indicate a frame loss better than deducing it from a gap in
the sequence numbers.

Is the DMA really aborted here though ?

> > Cc-ing Hans Sakari and Laurent for opinions.
> >
> > > userspace with an error, there is still a race condition on the name
> > > frame/buffer which will also have to return as error.
> >
> > I'm sorry I didn't get this part :)
> >
> > > > > +                              */
> > > > > +                             spin_lock(&node->dma_queue_lock);
> > > > > +                             list_add_tail(&node->cur_frm->list,
> > > > > +                                           &node->dma_queue);
> > > > > +                             spin_unlock(&node->dma_queue_lock);
> > > > > +                             node->cur_frm = node->next_frm;
> > > > > +                             node->next_frm = NULL;
> > > > > +                     }
> > > > >               }
> > > > >
> > > > >               unicam_queue_event_sof(unicam);

-- 
Regards,

Laurent Pinchart

Re: [PATCH v1 5/5] drivers: media: bcm2835-unicam: Correctly handle FS + FE ISR condition

[Naushir Patuck]

On Sun, 24 Nov 2024 at 07:04, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> On Fri, Nov 22, 2024 at 03:48:11PM +0100, Jacopo Mondi wrote:
> >
> > With Hans Sakari and Laurent in cc for real now
> >
> > On Fri, Nov 22, 2024 at 03:41:31PM +0100, Jacopo Mondi wrote:
> > > On Fri, Nov 22, 2024 at 11:40:26AM +0000, Naushir Patuck wrote:
> > > > On Fri, 22 Nov 2024 at 11:16, Jacopo Mondi wrote:
> > > > > On Fri, Nov 22, 2024 at 08:41:52AM +0000, Naushir Patuck wrote:
> > > > > > This change aligns the FS/FE interrupt handling with the Raspberry Pi
> > > > > > kernel downstream Unicam driver.
> > > > > >
> > > > > > If we get a simultaneous FS + FE interrupt for the same frame, it cannot
> > > > > > be marked as completed and returned to userland as the framebuffer will
> > > > > > be refilled by Unicam on the next sensor frame. Additionally, the
> > > > > > timestamp will be set to 0 as the FS interrupt handling code will not
> > > > > > have run yet.
> > > > > >
> > > > > > To avoid these problems, the frame is considered dropped in the FE
> > > > > > handler, and will be returned to userland on the subsequent sensor frame.
> > > > > >
> > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > > > > > ---
> > > > > >  .../media/platform/broadcom/bcm2835-unicam.c  | 39 +++++++++++++++++--
> > > > > >  1 file changed, 35 insertions(+), 4 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/media/platform/broadcom/bcm2835-unicam.c b/drivers/media/platform/broadcom/bcm2835-unicam.c
> > > > > > index f10064107d54..0d2aa25d483f 100644
> > > > > > --- a/drivers/media/platform/broadcom/bcm2835-unicam.c
> > > > > > +++ b/drivers/media/platform/broadcom/bcm2835-unicam.c
> > > > > > @@ -773,10 +773,26 @@ static irqreturn_t unicam_isr(int irq, void *dev)
> > > > > >                        * as complete, as the HW will reuse that buffer.
> > > > > >                        */
> > > > > >                       if (node->cur_frm && node->cur_frm != node->next_frm) {
> > > > > > +                             /*
> > > > > > +                              * This condition checks if FE + FS for the same
> > > > > > +                              * frame has occurred. In such cases, we cannot
> > > > > > +                              * return out the frame, as no buffer handling
> > > > > > +                              * or timestamping has yet been done as part of
> > > > > > +                              * the FS handler.
> > > > > > +                              */
> > > > > > +                             if (!node->cur_frm->vb.vb2_buf.timestamp) {
> > > > > > +                                     dev_dbg(unicam->v4l2_dev.dev,
> > > > > > +                                             "ISR: FE without FS, dropping frame\n");
> > > > > > +                                     continue;
> > > > > > +                             }
> > > > > > +
> > > > > >                               unicam_process_buffer_complete(node, sequence);
> > > > > > +                             node->cur_frm = node->next_frm;
> > > > > > +                             node->next_frm = NULL;
> > > > > >                               inc_seq = true;
> > > > > > +                     } else {
> > > > > > +                             node->cur_frm = node->next_frm;
> > > > > >                       }
> > > > > > -                     node->cur_frm = node->next_frm;
> > > > > >               }
> > > > > >
> > > > > >               /*
> > > > > > @@ -812,10 +828,25 @@ static irqreturn_t unicam_isr(int irq, void *dev)
> > > > > >                                       i);
> > > > > >                       /*
> > > > > >                        * Set the next frame output to go to a dummy frame
> > > > > > -                      * if we have not managed to obtain another frame
> > > > > > -                      * from the queue.
> > > > > > +                      * if no buffer currently queued.
> > > > > >                        */
> > > > > > -                     unicam_schedule_dummy_buffer(node);
> > > > > > +                     if (!node->next_frm ||
> > > > > > +                         node->next_frm == node->cur_frm) {
> > > > > > +                             unicam_schedule_dummy_buffer(node);
> > > > > > +                     } else if (unicam->node[i].cur_frm) {
> > > > > > +                             /*
> > > > > > +                              * Repeated FS without FE. Hardware will have
> > > > > > +                              * swapped buffers, but the cur_frm doesn't
> > > > > > +                              * contain valid data. Return cur_frm to the
> > > > > > +                              * queue.
> > > > >
> > > > > So the buffer gets silently recycled ? Or should it be returned with
> > > > > errors to userspace ?
> > > >
> > > > The buffer silently gets recycled and we dequeue when we are sure it
> > > > is valid and will not get overwritten.  If we were to return to
> > >
> > > I haven't find in the v4l2 specs any reference to what the behaviour
> > > should be.
> > >
> > > If I can summarize it: When a frame capture is aborted after the DMA
> > > transfer has already started, should the corresponding capture buffer
> > > be returned to the user in error state or the frame drop can go
> > > silently ignored ?
>
> If the DMA tranfer is aborted, I would return the buffer to userspace.
> This will indicate a frame loss better than deducing it from a gap in
> the sequence numbers.
>
> Is the DMA really aborted here though ?

No, the DMA continues, causing possilbe overwrite/tearing in the
framebuffer.  Hence we defer returning it until we can ensure we don't
overwrite into the buffer on the next frame.

Naush


>
> > > Cc-ing Hans Sakari and Laurent for opinions.
> > >
> > > > userspace with an error, there is still a race condition on the name
> > > > frame/buffer which will also have to return as error.
> > >
> > > I'm sorry I didn't get this part :)
> > >
> > > > > > +                              */
> > > > > > +                             spin_lock(&node->dma_queue_lock);
> > > > > > +                             list_add_tail(&node->cur_frm->list,
> > > > > > +                                           &node->dma_queue);
> > > > > > +                             spin_unlock(&node->dma_queue_lock);
> > > > > > +                             node->cur_frm = node->next_frm;
> > > > > > +                             node->next_frm = NULL;
> > > > > > +                     }
> > > > > >               }
> > > > > >
> > > > > >               unicam_queue_event_sof(unicam);
>
> --
> Regards,
>
> Laurent Pinchart

Re: [PATCH v1 5/5] drivers: media: bcm2835-unicam: Correctly handle FS + FE ISR condition

[Laurent Pinchart]

Hi Naush,

On Mon, Nov 25, 2024 at 08:37:22AM +0000, Naushir Patuck wrote:
> On Sun, 24 Nov 2024 at 07:04, Laurent Pinchart wrote:
> > On Fri, Nov 22, 2024 at 03:48:11PM +0100, Jacopo Mondi wrote:
> > >
> > > With Hans Sakari and Laurent in cc for real now
> > >
> > > On Fri, Nov 22, 2024 at 03:41:31PM +0100, Jacopo Mondi wrote:
> > > > On Fri, Nov 22, 2024 at 11:40:26AM +0000, Naushir Patuck wrote:
> > > > > On Fri, 22 Nov 2024 at 11:16, Jacopo Mondi wrote:
> > > > > > On Fri, Nov 22, 2024 at 08:41:52AM +0000, Naushir Patuck wrote:
> > > > > > > This change aligns the FS/FE interrupt handling with the Raspberry Pi
> > > > > > > kernel downstream Unicam driver.
> > > > > > >
> > > > > > > If we get a simultaneous FS + FE interrupt for the same frame, it cannot
> > > > > > > be marked as completed and returned to userland as the framebuffer will
> > > > > > > be refilled by Unicam on the next sensor frame. Additionally, the
> > > > > > > timestamp will be set to 0 as the FS interrupt handling code will not
> > > > > > > have run yet.
> > > > > > >
> > > > > > > To avoid these problems, the frame is considered dropped in the FE
> > > > > > > handler, and will be returned to userland on the subsequent sensor frame.
> > > > > > >
> > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > > > > > > ---
> > > > > > >  .../media/platform/broadcom/bcm2835-unicam.c  | 39 +++++++++++++++++--
> > > > > > >  1 file changed, 35 insertions(+), 4 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/media/platform/broadcom/bcm2835-unicam.c b/drivers/media/platform/broadcom/bcm2835-unicam.c
> > > > > > > index f10064107d54..0d2aa25d483f 100644
> > > > > > > --- a/drivers/media/platform/broadcom/bcm2835-unicam.c
> > > > > > > +++ b/drivers/media/platform/broadcom/bcm2835-unicam.c
> > > > > > > @@ -773,10 +773,26 @@ static irqreturn_t unicam_isr(int irq, void *dev)
> > > > > > >                        * as complete, as the HW will reuse that buffer.
> > > > > > >                        */
> > > > > > >                       if (node->cur_frm && node->cur_frm != node->next_frm) {
> > > > > > > +                             /*
> > > > > > > +                              * This condition checks if FE + FS for the same
> > > > > > > +                              * frame has occurred. In such cases, we cannot
> > > > > > > +                              * return out the frame, as no buffer handling
> > > > > > > +                              * or timestamping has yet been done as part of
> > > > > > > +                              * the FS handler.
> > > > > > > +                              */
> > > > > > > +                             if (!node->cur_frm->vb.vb2_buf.timestamp) {
> > > > > > > +                                     dev_dbg(unicam->v4l2_dev.dev,
> > > > > > > +                                             "ISR: FE without FS, dropping frame\n");
> > > > > > > +                                     continue;
> > > > > > > +                             }
> > > > > > > +
> > > > > > >                               unicam_process_buffer_complete(node, sequence);
> > > > > > > +                             node->cur_frm = node->next_frm;
> > > > > > > +                             node->next_frm = NULL;
> > > > > > >                               inc_seq = true;
> > > > > > > +                     } else {
> > > > > > > +                             node->cur_frm = node->next_frm;
> > > > > > >                       }
> > > > > > > -                     node->cur_frm = node->next_frm;
> > > > > > >               }
> > > > > > >
> > > > > > >               /*
> > > > > > > @@ -812,10 +828,25 @@ static irqreturn_t unicam_isr(int irq, void *dev)
> > > > > > >                                       i);
> > > > > > >                       /*
> > > > > > >                        * Set the next frame output to go to a dummy frame
> > > > > > > -                      * if we have not managed to obtain another frame
> > > > > > > -                      * from the queue.
> > > > > > > +                      * if no buffer currently queued.
> > > > > > >                        */
> > > > > > > -                     unicam_schedule_dummy_buffer(node);
> > > > > > > +                     if (!node->next_frm ||
> > > > > > > +                         node->next_frm == node->cur_frm) {
> > > > > > > +                             unicam_schedule_dummy_buffer(node);
> > > > > > > +                     } else if (unicam->node[i].cur_frm) {
> > > > > > > +                             /*
> > > > > > > +                              * Repeated FS without FE. Hardware will have
> > > > > > > +                              * swapped buffers, but the cur_frm doesn't
> > > > > > > +                              * contain valid data. Return cur_frm to the
> > > > > > > +                              * queue.
> > > > > >
> > > > > > So the buffer gets silently recycled ? Or should it be returned with
> > > > > > errors to userspace ?
> > > > >
> > > > > The buffer silently gets recycled and we dequeue when we are sure it
> > > > > is valid and will not get overwritten.  If we were to return to
> > > >
> > > > I haven't find in the v4l2 specs any reference to what the behaviour
> > > > should be.
> > > >
> > > > If I can summarize it: When a frame capture is aborted after the DMA
> > > > transfer has already started, should the corresponding capture buffer
> > > > be returned to the user in error state or the frame drop can go
> > > > silently ignored ?
> >
> > If the DMA tranfer is aborted, I would return the buffer to userspace.
> > This will indicate a frame loss better than deducing it from a gap in
> > the sequence numbers.
> >
> > Is the DMA really aborted here though ?
> 
> No, the DMA continues, causing possilbe overwrite/tearing in the
> framebuffer.  Hence we defer returning it until we can ensure we don't
> overwrite into the buffer on the next frame.

If the DMA continues then we certainly can't return the buffer to
userspace. Is it the next frame being DMA'ed to the same buffer, or does
the hardware put it the buffer at the back of its queue ?

> > > > Cc-ing Hans Sakari and Laurent for opinions.
> > > >
> > > > > userspace with an error, there is still a race condition on the name
> > > > > frame/buffer which will also have to return as error.
> > > >
> > > > I'm sorry I didn't get this part :)
> > > >
> > > > > > > +                              */
> > > > > > > +                             spin_lock(&node->dma_queue_lock);
> > > > > > > +                             list_add_tail(&node->cur_frm->list,
> > > > > > > +                                           &node->dma_queue);
> > > > > > > +                             spin_unlock(&node->dma_queue_lock);
> > > > > > > +                             node->cur_frm = node->next_frm;
> > > > > > > +                             node->next_frm = NULL;
> > > > > > > +                     }
> > > > > > >               }
> > > > > > >
> > > > > > >               unicam_queue_event_sof(unicam);

-- 
Regards,

Laurent Pinchart

Re: [PATCH v1 5/5] drivers: media: bcm2835-unicam: Correctly handle FS + FE ISR condition

[Naushir Patuck]

Hi Laurent,

On Mon, 25 Nov 2024 at 09:23, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Naush,
>
> On Mon, Nov 25, 2024 at 08:37:22AM +0000, Naushir Patuck wrote:
> > On Sun, 24 Nov 2024 at 07:04, Laurent Pinchart wrote:
> > > On Fri, Nov 22, 2024 at 03:48:11PM +0100, Jacopo Mondi wrote:
> > > >
> > > > With Hans Sakari and Laurent in cc for real now
> > > >
> > > > On Fri, Nov 22, 2024 at 03:41:31PM +0100, Jacopo Mondi wrote:
> > > > > On Fri, Nov 22, 2024 at 11:40:26AM +0000, Naushir Patuck wrote:
> > > > > > On Fri, 22 Nov 2024 at 11:16, Jacopo Mondi wrote:
> > > > > > > On Fri, Nov 22, 2024 at 08:41:52AM +0000, Naushir Patuck wrote:
> > > > > > > > This change aligns the FS/FE interrupt handling with the Raspberry Pi
> > > > > > > > kernel downstream Unicam driver.
> > > > > > > >
> > > > > > > > If we get a simultaneous FS + FE interrupt for the same frame, it cannot
> > > > > > > > be marked as completed and returned to userland as the framebuffer will
> > > > > > > > be refilled by Unicam on the next sensor frame. Additionally, the
> > > > > > > > timestamp will be set to 0 as the FS interrupt handling code will not
> > > > > > > > have run yet.
> > > > > > > >
> > > > > > > > To avoid these problems, the frame is considered dropped in the FE
> > > > > > > > handler, and will be returned to userland on the subsequent sensor frame.
> > > > > > > >
> > > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > > > > > > > ---
> > > > > > > >  .../media/platform/broadcom/bcm2835-unicam.c  | 39 +++++++++++++++++--
> > > > > > > >  1 file changed, 35 insertions(+), 4 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/drivers/media/platform/broadcom/bcm2835-unicam.c b/drivers/media/platform/broadcom/bcm2835-unicam.c
> > > > > > > > index f10064107d54..0d2aa25d483f 100644
> > > > > > > > --- a/drivers/media/platform/broadcom/bcm2835-unicam.c
> > > > > > > > +++ b/drivers/media/platform/broadcom/bcm2835-unicam.c
> > > > > > > > @@ -773,10 +773,26 @@ static irqreturn_t unicam_isr(int irq, void *dev)
> > > > > > > >                        * as complete, as the HW will reuse that buffer.
> > > > > > > >                        */
> > > > > > > >                       if (node->cur_frm && node->cur_frm != node->next_frm) {
> > > > > > > > +                             /*
> > > > > > > > +                              * This condition checks if FE + FS for the same
> > > > > > > > +                              * frame has occurred. In such cases, we cannot
> > > > > > > > +                              * return out the frame, as no buffer handling
> > > > > > > > +                              * or timestamping has yet been done as part of
> > > > > > > > +                              * the FS handler.
> > > > > > > > +                              */
> > > > > > > > +                             if (!node->cur_frm->vb.vb2_buf.timestamp) {
> > > > > > > > +                                     dev_dbg(unicam->v4l2_dev.dev,
> > > > > > > > +                                             "ISR: FE without FS, dropping frame\n");
> > > > > > > > +                                     continue;
> > > > > > > > +                             }
> > > > > > > > +
> > > > > > > >                               unicam_process_buffer_complete(node, sequence);
> > > > > > > > +                             node->cur_frm = node->next_frm;
> > > > > > > > +                             node->next_frm = NULL;
> > > > > > > >                               inc_seq = true;
> > > > > > > > +                     } else {
> > > > > > > > +                             node->cur_frm = node->next_frm;
> > > > > > > >                       }
> > > > > > > > -                     node->cur_frm = node->next_frm;
> > > > > > > >               }
> > > > > > > >
> > > > > > > >               /*
> > > > > > > > @@ -812,10 +828,25 @@ static irqreturn_t unicam_isr(int irq, void *dev)
> > > > > > > >                                       i);
> > > > > > > >                       /*
> > > > > > > >                        * Set the next frame output to go to a dummy frame
> > > > > > > > -                      * if we have not managed to obtain another frame
> > > > > > > > -                      * from the queue.
> > > > > > > > +                      * if no buffer currently queued.
> > > > > > > >                        */
> > > > > > > > -                     unicam_schedule_dummy_buffer(node);
> > > > > > > > +                     if (!node->next_frm ||
> > > > > > > > +                         node->next_frm == node->cur_frm) {
> > > > > > > > +                             unicam_schedule_dummy_buffer(node);
> > > > > > > > +                     } else if (unicam->node[i].cur_frm) {
> > > > > > > > +                             /*
> > > > > > > > +                              * Repeated FS without FE. Hardware will have
> > > > > > > > +                              * swapped buffers, but the cur_frm doesn't
> > > > > > > > +                              * contain valid data. Return cur_frm to the
> > > > > > > > +                              * queue.
> > > > > > >
> > > > > > > So the buffer gets silently recycled ? Or should it be returned with
> > > > > > > errors to userspace ?
> > > > > >
> > > > > > The buffer silently gets recycled and we dequeue when we are sure it
> > > > > > is valid and will not get overwritten.  If we were to return to
> > > > >
> > > > > I haven't find in the v4l2 specs any reference to what the behaviour
> > > > > should be.
> > > > >
> > > > > If I can summarize it: When a frame capture is aborted after the DMA
> > > > > transfer has already started, should the corresponding capture buffer
> > > > > be returned to the user in error state or the frame drop can go
> > > > > silently ignored ?
> > >
> > > If the DMA tranfer is aborted, I would return the buffer to userspace.
> > > This will indicate a frame loss better than deducing it from a gap in
> > > the sequence numbers.
> > >
> > > Is the DMA really aborted here though ?
> >
> > No, the DMA continues, causing possilbe overwrite/tearing in the
> > framebuffer.  Hence we defer returning it until we can ensure we don't
> > overwrite into the buffer on the next frame.
>
> If the DMA continues then we certainly can't return the buffer to
> userspace. Is it the next frame being DMA'ed to the same buffer, or does
> the hardware put it the buffer at the back of its queue ?

The next frame will be DMA'ed into the same buffer in this error
condition. The hardware really only has a 2-deep buffer queue (current
+ next frame), and no reliable way of telling if next has been swapped
to been swapped.

Regards,
Naush


>
> > > > > Cc-ing Hans Sakari and Laurent for opinions.
> > > > >
> > > > > > userspace with an error, there is still a race condition on the name
> > > > > > frame/buffer which will also have to return as error.
> > > > >
> > > > > I'm sorry I didn't get this part :)
> > > > >
> > > > > > > > +                              */
> > > > > > > > +                             spin_lock(&node->dma_queue_lock);
> > > > > > > > +                             list_add_tail(&node->cur_frm->list,
> > > > > > > > +                                           &node->dma_queue);
> > > > > > > > +                             spin_unlock(&node->dma_queue_lock);
> > > > > > > > +                             node->cur_frm = node->next_frm;
> > > > > > > > +                             node->next_frm = NULL;
> > > > > > > > +                     }
> > > > > > > >               }
> > > > > > > >
> > > > > > > >               unicam_queue_event_sof(unicam);
>
> --
> Regards,
>
> Laurent Pinchart

Re: [PATCH v1 5/5] drivers: media: bcm2835-unicam: Correctly handle FS + FE ISR condition

[Laurent Pinchart]

Hi Naush,

On Mon, Nov 25, 2024 at 09:46:26AM +0000, Naushir Patuck wrote:
> On Mon, 25 Nov 2024 at 09:23, Laurent Pinchart wrote:
> > On Mon, Nov 25, 2024 at 08:37:22AM +0000, Naushir Patuck wrote:
> > > On Sun, 24 Nov 2024 at 07:04, Laurent Pinchart wrote:
> > > > On Fri, Nov 22, 2024 at 03:48:11PM +0100, Jacopo Mondi wrote:
> > > > >
> > > > > With Hans Sakari and Laurent in cc for real now
> > > > >
> > > > > On Fri, Nov 22, 2024 at 03:41:31PM +0100, Jacopo Mondi wrote:
> > > > > > On Fri, Nov 22, 2024 at 11:40:26AM +0000, Naushir Patuck wrote:
> > > > > > > On Fri, 22 Nov 2024 at 11:16, Jacopo Mondi wrote:
> > > > > > > > On Fri, Nov 22, 2024 at 08:41:52AM +0000, Naushir Patuck wrote:
> > > > > > > > > This change aligns the FS/FE interrupt handling with the Raspberry Pi
> > > > > > > > > kernel downstream Unicam driver.
> > > > > > > > >
> > > > > > > > > If we get a simultaneous FS + FE interrupt for the same frame, it cannot
> > > > > > > > > be marked as completed and returned to userland as the framebuffer will
> > > > > > > > > be refilled by Unicam on the next sensor frame. Additionally, the
> > > > > > > > > timestamp will be set to 0 as the FS interrupt handling code will not
> > > > > > > > > have run yet.
> > > > > > > > >
> > > > > > > > > To avoid these problems, the frame is considered dropped in the FE
> > > > > > > > > handler, and will be returned to userland on the subsequent sensor frame.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > > > > > > > > ---
> > > > > > > > >  .../media/platform/broadcom/bcm2835-unicam.c  | 39 +++++++++++++++++--
> > > > > > > > >  1 file changed, 35 insertions(+), 4 deletions(-)
> > > > > > > > >
> > > > > > > > > diff --git a/drivers/media/platform/broadcom/bcm2835-unicam.c b/drivers/media/platform/broadcom/bcm2835-unicam.c
> > > > > > > > > index f10064107d54..0d2aa25d483f 100644
> > > > > > > > > --- a/drivers/media/platform/broadcom/bcm2835-unicam.c
> > > > > > > > > +++ b/drivers/media/platform/broadcom/bcm2835-unicam.c
> > > > > > > > > @@ -773,10 +773,26 @@ static irqreturn_t unicam_isr(int irq, void *dev)
> > > > > > > > >                        * as complete, as the HW will reuse that buffer.
> > > > > > > > >                        */
> > > > > > > > >                       if (node->cur_frm && node->cur_frm != node->next_frm) {
> > > > > > > > > +                             /*
> > > > > > > > > +                              * This condition checks if FE + FS for the same
> > > > > > > > > +                              * frame has occurred. In such cases, we cannot
> > > > > > > > > +                              * return out the frame, as no buffer handling
> > > > > > > > > +                              * or timestamping has yet been done as part of
> > > > > > > > > +                              * the FS handler.
> > > > > > > > > +                              */
> > > > > > > > > +                             if (!node->cur_frm->vb.vb2_buf.timestamp) {
> > > > > > > > > +                                     dev_dbg(unicam->v4l2_dev.dev,
> > > > > > > > > +                                             "ISR: FE without FS, dropping frame\n");
> > > > > > > > > +                                     continue;
> > > > > > > > > +                             }
> > > > > > > > > +
> > > > > > > > >                               unicam_process_buffer_complete(node, sequence);
> > > > > > > > > +                             node->cur_frm = node->next_frm;
> > > > > > > > > +                             node->next_frm = NULL;
> > > > > > > > >                               inc_seq = true;
> > > > > > > > > +                     } else {
> > > > > > > > > +                             node->cur_frm = node->next_frm;
> > > > > > > > >                       }
> > > > > > > > > -                     node->cur_frm = node->next_frm;
> > > > > > > > >               }
> > > > > > > > >
> > > > > > > > >               /*
> > > > > > > > > @@ -812,10 +828,25 @@ static irqreturn_t unicam_isr(int irq, void *dev)
> > > > > > > > >                                       i);
> > > > > > > > >                       /*
> > > > > > > > >                        * Set the next frame output to go to a dummy frame
> > > > > > > > > -                      * if we have not managed to obtain another frame
> > > > > > > > > -                      * from the queue.
> > > > > > > > > +                      * if no buffer currently queued.
> > > > > > > > >                        */
> > > > > > > > > -                     unicam_schedule_dummy_buffer(node);
> > > > > > > > > +                     if (!node->next_frm ||
> > > > > > > > > +                         node->next_frm == node->cur_frm) {
> > > > > > > > > +                             unicam_schedule_dummy_buffer(node);
> > > > > > > > > +                     } else if (unicam->node[i].cur_frm) {
> > > > > > > > > +                             /*
> > > > > > > > > +                              * Repeated FS without FE. Hardware will have
> > > > > > > > > +                              * swapped buffers, but the cur_frm doesn't
> > > > > > > > > +                              * contain valid data. Return cur_frm to the
> > > > > > > > > +                              * queue.
> > > > > > > >
> > > > > > > > So the buffer gets silently recycled ? Or should it be returned with
> > > > > > > > errors to userspace ?
> > > > > > >
> > > > > > > The buffer silently gets recycled and we dequeue when we are sure it
> > > > > > > is valid and will not get overwritten.  If we were to return to
> > > > > >
> > > > > > I haven't find in the v4l2 specs any reference to what the behaviour
> > > > > > should be.
> > > > > >
> > > > > > If I can summarize it: When a frame capture is aborted after the DMA
> > > > > > transfer has already started, should the corresponding capture buffer
> > > > > > be returned to the user in error state or the frame drop can go
> > > > > > silently ignored ?
> > > >
> > > > If the DMA tranfer is aborted, I would return the buffer to userspace.
> > > > This will indicate a frame loss better than deducing it from a gap in
> > > > the sequence numbers.
> > > >
> > > > Is the DMA really aborted here though ?
> > >
> > > No, the DMA continues, causing possilbe overwrite/tearing in the
> > > framebuffer.  Hence we defer returning it until we can ensure we don't
> > > overwrite into the buffer on the next frame.
> >
> > If the DMA continues then we certainly can't return the buffer to
> > userspace. Is it the next frame being DMA'ed to the same buffer, or does
> > the hardware put it the buffer at the back of its queue ?
> 
> The next frame will be DMA'ed into the same buffer in this error
> condition. The hardware really only has a 2-deep buffer queue (current
> + next frame), and no reliable way of telling if next has been swapped
> to been swapped.

OK, that makes sense.

In that case, is putting the buffer back to the back of the dma_queue
the right option ? Shouldn't it be kept current and "just" be completed
one frame later ? Or did I misunderstand the patch ?

> > > > > > Cc-ing Hans Sakari and Laurent for opinions.
> > > > > >
> > > > > > > userspace with an error, there is still a race condition on the name
> > > > > > > frame/buffer which will also have to return as error.
> > > > > >
> > > > > > I'm sorry I didn't get this part :)
> > > > > >
> > > > > > > > > +                              */
> > > > > > > > > +                             spin_lock(&node->dma_queue_lock);
> > > > > > > > > +                             list_add_tail(&node->cur_frm->list,
> > > > > > > > > +                                           &node->dma_queue);
> > > > > > > > > +                             spin_unlock(&node->dma_queue_lock);
> > > > > > > > > +                             node->cur_frm = node->next_frm;
> > > > > > > > > +                             node->next_frm = NULL;
> > > > > > > > > +                     }
> > > > > > > > >               }
> > > > > > > > >
> > > > > > > > >               unicam_queue_event_sof(unicam);

-- 
Regards,

Laurent Pinchart

Re: [PATCH v1 5/5] drivers: media: bcm2835-unicam: Correctly handle FS + FE ISR condition

[Naushir Patuck]

Hi Laurent,


On Mon, 25 Nov 2024 at 10:27, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Naush,
>
> On Mon, Nov 25, 2024 at 09:46:26AM +0000, Naushir Patuck wrote:
> > On Mon, 25 Nov 2024 at 09:23, Laurent Pinchart wrote:
> > > On Mon, Nov 25, 2024 at 08:37:22AM +0000, Naushir Patuck wrote:
> > > > On Sun, 24 Nov 2024 at 07:04, Laurent Pinchart wrote:
> > > > > On Fri, Nov 22, 2024 at 03:48:11PM +0100, Jacopo Mondi wrote:
> > > > > >
> > > > > > With Hans Sakari and Laurent in cc for real now
> > > > > >
> > > > > > On Fri, Nov 22, 2024 at 03:41:31PM +0100, Jacopo Mondi wrote:
> > > > > > > On Fri, Nov 22, 2024 at 11:40:26AM +0000, Naushir Patuck wrote:
> > > > > > > > On Fri, 22 Nov 2024 at 11:16, Jacopo Mondi wrote:
> > > > > > > > > On Fri, Nov 22, 2024 at 08:41:52AM +0000, Naushir Patuck wrote:
> > > > > > > > > > This change aligns the FS/FE interrupt handling with the Raspberry Pi
> > > > > > > > > > kernel downstream Unicam driver.
> > > > > > > > > >
> > > > > > > > > > If we get a simultaneous FS + FE interrupt for the same frame, it cannot
> > > > > > > > > > be marked as completed and returned to userland as the framebuffer will
> > > > > > > > > > be refilled by Unicam on the next sensor frame. Additionally, the
> > > > > > > > > > timestamp will be set to 0 as the FS interrupt handling code will not
> > > > > > > > > > have run yet.
> > > > > > > > > >
> > > > > > > > > > To avoid these problems, the frame is considered dropped in the FE
> > > > > > > > > > handler, and will be returned to userland on the subsequent sensor frame.
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > > > > > > > > > ---
> > > > > > > > > >  .../media/platform/broadcom/bcm2835-unicam.c  | 39 +++++++++++++++++--
> > > > > > > > > >  1 file changed, 35 insertions(+), 4 deletions(-)
> > > > > > > > > >
> > > > > > > > > > diff --git a/drivers/media/platform/broadcom/bcm2835-unicam.c b/drivers/media/platform/broadcom/bcm2835-unicam.c
> > > > > > > > > > index f10064107d54..0d2aa25d483f 100644
> > > > > > > > > > --- a/drivers/media/platform/broadcom/bcm2835-unicam.c
> > > > > > > > > > +++ b/drivers/media/platform/broadcom/bcm2835-unicam.c
> > > > > > > > > > @@ -773,10 +773,26 @@ static irqreturn_t unicam_isr(int irq, void *dev)
> > > > > > > > > >                        * as complete, as the HW will reuse that buffer.
> > > > > > > > > >                        */
> > > > > > > > > >                       if (node->cur_frm && node->cur_frm != node->next_frm) {
> > > > > > > > > > +                             /*
> > > > > > > > > > +                              * This condition checks if FE + FS for the same
> > > > > > > > > > +                              * frame has occurred. In such cases, we cannot
> > > > > > > > > > +                              * return out the frame, as no buffer handling
> > > > > > > > > > +                              * or timestamping has yet been done as part of
> > > > > > > > > > +                              * the FS handler.
> > > > > > > > > > +                              */
> > > > > > > > > > +                             if (!node->cur_frm->vb.vb2_buf.timestamp) {
> > > > > > > > > > +                                     dev_dbg(unicam->v4l2_dev.dev,
> > > > > > > > > > +                                             "ISR: FE without FS, dropping frame\n");
> > > > > > > > > > +                                     continue;
> > > > > > > > > > +                             }
> > > > > > > > > > +
> > > > > > > > > >                               unicam_process_buffer_complete(node, sequence);
> > > > > > > > > > +                             node->cur_frm = node->next_frm;
> > > > > > > > > > +                             node->next_frm = NULL;
> > > > > > > > > >                               inc_seq = true;
> > > > > > > > > > +                     } else {
> > > > > > > > > > +                             node->cur_frm = node->next_frm;
> > > > > > > > > >                       }
> > > > > > > > > > -                     node->cur_frm = node->next_frm;
> > > > > > > > > >               }
> > > > > > > > > >
> > > > > > > > > >               /*
> > > > > > > > > > @@ -812,10 +828,25 @@ static irqreturn_t unicam_isr(int irq, void *dev)
> > > > > > > > > >                                       i);
> > > > > > > > > >                       /*
> > > > > > > > > >                        * Set the next frame output to go to a dummy frame
> > > > > > > > > > -                      * if we have not managed to obtain another frame
> > > > > > > > > > -                      * from the queue.
> > > > > > > > > > +                      * if no buffer currently queued.
> > > > > > > > > >                        */
> > > > > > > > > > -                     unicam_schedule_dummy_buffer(node);
> > > > > > > > > > +                     if (!node->next_frm ||
> > > > > > > > > > +                         node->next_frm == node->cur_frm) {
> > > > > > > > > > +                             unicam_schedule_dummy_buffer(node);
> > > > > > > > > > +                     } else if (unicam->node[i].cur_frm) {
> > > > > > > > > > +                             /*
> > > > > > > > > > +                              * Repeated FS without FE. Hardware will have
> > > > > > > > > > +                              * swapped buffers, but the cur_frm doesn't
> > > > > > > > > > +                              * contain valid data. Return cur_frm to the
> > > > > > > > > > +                              * queue.
> > > > > > > > >
> > > > > > > > > So the buffer gets silently recycled ? Or should it be returned with
> > > > > > > > > errors to userspace ?
> > > > > > > >
> > > > > > > > The buffer silently gets recycled and we dequeue when we are sure it
> > > > > > > > is valid and will not get overwritten.  If we were to return to
> > > > > > >
> > > > > > > I haven't find in the v4l2 specs any reference to what the behaviour
> > > > > > > should be.
> > > > > > >
> > > > > > > If I can summarize it: When a frame capture is aborted after the DMA
> > > > > > > transfer has already started, should the corresponding capture buffer
> > > > > > > be returned to the user in error state or the frame drop can go
> > > > > > > silently ignored ?
> > > > >
> > > > > If the DMA tranfer is aborted, I would return the buffer to userspace.
> > > > > This will indicate a frame loss better than deducing it from a gap in
> > > > > the sequence numbers.
> > > > >
> > > > > Is the DMA really aborted here though ?
> > > >
> > > > No, the DMA continues, causing possilbe overwrite/tearing in the
> > > > framebuffer.  Hence we defer returning it until we can ensure we don't
> > > > overwrite into the buffer on the next frame.
> > >
> > > If the DMA continues then we certainly can't return the buffer to
> > > userspace. Is it the next frame being DMA'ed to the same buffer, or does
> > > the hardware put it the buffer at the back of its queue ?
> >
> > The next frame will be DMA'ed into the same buffer in this error
> > condition. The hardware really only has a 2-deep buffer queue (current
> > + next frame), and no reliable way of telling if next has been swapped
> > to been swapped.
>
> OK, that makes sense.
>
> In that case, is putting the buffer back to the back of the dma_queue
> the right option ? Shouldn't it be kept current and "just" be completed
> one frame later ? Or did I misunderstand the patch ?

Yes, I agree that the buffer handling logic below does seem
contradictory.  I'm going to need time to look into this in more
detail, it's been quite some time since I looked into this.  I would
suggest we remove this particular patch from the series until I get a
better understanding of the change.

Regards,
Naush


>
> > > > > > > Cc-ing Hans Sakari and Laurent for opinions.
> > > > > > >
> > > > > > > > userspace with an error, there is still a race condition on the name
> > > > > > > > frame/buffer which will also have to return as error.
> > > > > > >
> > > > > > > I'm sorry I didn't get this part :)
> > > > > > >
> > > > > > > > > > +                              */
> > > > > > > > > > +                             spin_lock(&node->dma_queue_lock);
> > > > > > > > > > +                             list_add_tail(&node->cur_frm->list,
> > > > > > > > > > +                                           &node->dma_queue);
> > > > > > > > > > +                             spin_unlock(&node->dma_queue_lock);
> > > > > > > > > > +                             node->cur_frm = node->next_frm;
> > > > > > > > > > +                             node->next_frm = NULL;
> > > > > > > > > > +                     }
> > > > > > > > > >               }
> > > > > > > > > >
> > > > > > > > > >               unicam_queue_event_sof(unicam);
>
> --
> Regards,
>
> Laurent Pinchart

Re: [PATCH v1 5/5] drivers: media: bcm2835-unicam: Correctly handle FS + FE ISR condition

[Laurent Pinchart]

Hi Naush,

On Mon, Nov 25, 2024 at 10:46:01AM +0000, Naushir Patuck wrote:
> On Mon, 25 Nov 2024 at 10:27, Laurent Pinchart wrote:
> > On Mon, Nov 25, 2024 at 09:46:26AM +0000, Naushir Patuck wrote:
> > > On Mon, 25 Nov 2024 at 09:23, Laurent Pinchart wrote:
> > > > On Mon, Nov 25, 2024 at 08:37:22AM +0000, Naushir Patuck wrote:
> > > > > On Sun, 24 Nov 2024 at 07:04, Laurent Pinchart wrote:
> > > > > > On Fri, Nov 22, 2024 at 03:48:11PM +0100, Jacopo Mondi wrote:
> > > > > > >
> > > > > > > With Hans Sakari and Laurent in cc for real now
> > > > > > >
> > > > > > > On Fri, Nov 22, 2024 at 03:41:31PM +0100, Jacopo Mondi wrote:
> > > > > > > > On Fri, Nov 22, 2024 at 11:40:26AM +0000, Naushir Patuck wrote:
> > > > > > > > > On Fri, 22 Nov 2024 at 11:16, Jacopo Mondi wrote:
> > > > > > > > > > On Fri, Nov 22, 2024 at 08:41:52AM +0000, Naushir Patuck wrote:
> > > > > > > > > > > This change aligns the FS/FE interrupt handling with the Raspberry Pi
> > > > > > > > > > > kernel downstream Unicam driver.
> > > > > > > > > > >
> > > > > > > > > > > If we get a simultaneous FS + FE interrupt for the same frame, it cannot
> > > > > > > > > > > be marked as completed and returned to userland as the framebuffer will
> > > > > > > > > > > be refilled by Unicam on the next sensor frame. Additionally, the
> > > > > > > > > > > timestamp will be set to 0 as the FS interrupt handling code will not
> > > > > > > > > > > have run yet.
> > > > > > > > > > >
> > > > > > > > > > > To avoid these problems, the frame is considered dropped in the FE
> > > > > > > > > > > handler, and will be returned to userland on the subsequent sensor frame.
> > > > > > > > > > >
> > > > > > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > > > > > > > > > > ---
> > > > > > > > > > >  .../media/platform/broadcom/bcm2835-unicam.c  | 39 +++++++++++++++++--
> > > > > > > > > > >  1 file changed, 35 insertions(+), 4 deletions(-)
> > > > > > > > > > >
> > > > > > > > > > > diff --git a/drivers/media/platform/broadcom/bcm2835-unicam.c b/drivers/media/platform/broadcom/bcm2835-unicam.c
> > > > > > > > > > > index f10064107d54..0d2aa25d483f 100644
> > > > > > > > > > > --- a/drivers/media/platform/broadcom/bcm2835-unicam.c
> > > > > > > > > > > +++ b/drivers/media/platform/broadcom/bcm2835-unicam.c
> > > > > > > > > > > @@ -773,10 +773,26 @@ static irqreturn_t unicam_isr(int irq, void *dev)
> > > > > > > > > > >                        * as complete, as the HW will reuse that buffer.
> > > > > > > > > > >                        */
> > > > > > > > > > >                       if (node->cur_frm && node->cur_frm != node->next_frm) {
> > > > > > > > > > > +                             /*
> > > > > > > > > > > +                              * This condition checks if FE + FS for the same
> > > > > > > > > > > +                              * frame has occurred. In such cases, we cannot
> > > > > > > > > > > +                              * return out the frame, as no buffer handling
> > > > > > > > > > > +                              * or timestamping has yet been done as part of
> > > > > > > > > > > +                              * the FS handler.
> > > > > > > > > > > +                              */
> > > > > > > > > > > +                             if (!node->cur_frm->vb.vb2_buf.timestamp) {
> > > > > > > > > > > +                                     dev_dbg(unicam->v4l2_dev.dev,
> > > > > > > > > > > +                                             "ISR: FE without FS, dropping frame\n");
> > > > > > > > > > > +                                     continue;
> > > > > > > > > > > +                             }
> > > > > > > > > > > +
> > > > > > > > > > >                               unicam_process_buffer_complete(node, sequence);
> > > > > > > > > > > +                             node->cur_frm = node->next_frm;
> > > > > > > > > > > +                             node->next_frm = NULL;
> > > > > > > > > > >                               inc_seq = true;
> > > > > > > > > > > +                     } else {
> > > > > > > > > > > +                             node->cur_frm = node->next_frm;
> > > > > > > > > > >                       }
> > > > > > > > > > > -                     node->cur_frm = node->next_frm;
> > > > > > > > > > >               }
> > > > > > > > > > >
> > > > > > > > > > >               /*
> > > > > > > > > > > @@ -812,10 +828,25 @@ static irqreturn_t unicam_isr(int irq, void *dev)
> > > > > > > > > > >                                       i);
> > > > > > > > > > >                       /*
> > > > > > > > > > >                        * Set the next frame output to go to a dummy frame
> > > > > > > > > > > -                      * if we have not managed to obtain another frame
> > > > > > > > > > > -                      * from the queue.
> > > > > > > > > > > +                      * if no buffer currently queued.
> > > > > > > > > > >                        */
> > > > > > > > > > > -                     unicam_schedule_dummy_buffer(node);
> > > > > > > > > > > +                     if (!node->next_frm ||
> > > > > > > > > > > +                         node->next_frm == node->cur_frm) {
> > > > > > > > > > > +                             unicam_schedule_dummy_buffer(node);
> > > > > > > > > > > +                     } else if (unicam->node[i].cur_frm) {
> > > > > > > > > > > +                             /*
> > > > > > > > > > > +                              * Repeated FS without FE. Hardware will have
> > > > > > > > > > > +                              * swapped buffers, but the cur_frm doesn't
> > > > > > > > > > > +                              * contain valid data. Return cur_frm to the
> > > > > > > > > > > +                              * queue.
> > > > > > > > > >
> > > > > > > > > > So the buffer gets silently recycled ? Or should it be returned with
> > > > > > > > > > errors to userspace ?
> > > > > > > > >
> > > > > > > > > The buffer silently gets recycled and we dequeue when we are sure it
> > > > > > > > > is valid and will not get overwritten.  If we were to return to
> > > > > > > >
> > > > > > > > I haven't find in the v4l2 specs any reference to what the behaviour
> > > > > > > > should be.
> > > > > > > >
> > > > > > > > If I can summarize it: When a frame capture is aborted after the DMA
> > > > > > > > transfer has already started, should the corresponding capture buffer
> > > > > > > > be returned to the user in error state or the frame drop can go
> > > > > > > > silently ignored ?
> > > > > >
> > > > > > If the DMA tranfer is aborted, I would return the buffer to userspace.
> > > > > > This will indicate a frame loss better than deducing it from a gap in
> > > > > > the sequence numbers.
> > > > > >
> > > > > > Is the DMA really aborted here though ?
> > > > >
> > > > > No, the DMA continues, causing possilbe overwrite/tearing in the
> > > > > framebuffer.  Hence we defer returning it until we can ensure we don't
> > > > > overwrite into the buffer on the next frame.
> > > >
> > > > If the DMA continues then we certainly can't return the buffer to
> > > > userspace. Is it the next frame being DMA'ed to the same buffer, or does
> > > > the hardware put it the buffer at the back of its queue ?
> > >
> > > The next frame will be DMA'ed into the same buffer in this error
> > > condition. The hardware really only has a 2-deep buffer queue (current
> > > + next frame), and no reliable way of telling if next has been swapped
> > > to been swapped.
> >
> > OK, that makes sense.
> >
> > In that case, is putting the buffer back to the back of the dma_queue
> > the right option ? Shouldn't it be kept current and "just" be completed
> > one frame later ? Or did I misunderstand the patch ?
> 
> Yes, I agree that the buffer handling logic below does seem
> contradictory.  I'm going to need time to look into this in more
> detail, it's been quite some time since I looked into this.  I would
> suggest we remove this particular patch from the series until I get a
> better understanding of the change.

Fine with me.

> > > > > > > > Cc-ing Hans Sakari and Laurent for opinions.
> > > > > > > >
> > > > > > > > > userspace with an error, there is still a race condition on the name
> > > > > > > > > frame/buffer which will also have to return as error.
> > > > > > > >
> > > > > > > > I'm sorry I didn't get this part :)
> > > > > > > >
> > > > > > > > > > > +                              */
> > > > > > > > > > > +                             spin_lock(&node->dma_queue_lock);
> > > > > > > > > > > +                             list_add_tail(&node->cur_frm->list,
> > > > > > > > > > > +                                           &node->dma_queue);
> > > > > > > > > > > +                             spin_unlock(&node->dma_queue_lock);
> > > > > > > > > > > +                             node->cur_frm = node->next_frm;
> > > > > > > > > > > +                             node->next_frm = NULL;
> > > > > > > > > > > +                     }
> > > > > > > > > > >               }
> > > > > > > > > > >
> > > > > > > > > > >               unicam_queue_event_sof(unicam);

-- 
Regards,

Laurent Pinchart