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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <gl7qb2rqmqzd6tw7pei3ldbu6m4l3qiwld3ddhvn4pemdpgfjz@bvaf5prvmz3c>
Date: Fri, 22 Nov 2024 15:45:55 +0100
From: Jacopo Mondi <jacopo.mondi@...asonboard.com>
To: Naushir Patuck <naush@...pberrypi.com>
Cc: Raspberry Pi Kernel Maintenance <kernel-list@...pberrypi.com>, 
	Mauro Carvalho Chehab <mchehab@...nel.org>, Florian Fainelli <florian.fainelli@...adcom.com>, 
	Broadcom internal kernel review list <bcm-kernel-feedback-list@...adcom.com>, Ray Jui <rjui@...adcom.com>, 
	Scott Branden <sbranden@...adcom.com>, linux-media@...r.kernel.org, 
	linux-rpi-kernel@...ts.infradead.org, linux-arm-kernel@...ts.infradead.org, 
	linux-kernel@...r.kernel.org, jacopo.mondi@...asonboard.com, 
	Dave Stevenson <dave.stevenson@...pberrypi.com>
Subject: Re: [PATCH v1 1/5] drivers: media: bcm2835-unicam: Improve frame
 sequence count handling

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@...pberrypi.com>


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