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@xxxxxxxxxxxxxxx>
Thanks, looks good to me
Reviewed-by: Jacopo Mondi <jacopo.mondi@xxxxxxxxxxxxxxxx>
> ---
> .../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
>
>
