Re: [PATCH 1/5] rcar-vin: Use scratch buffer when not in running state

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Niklas,

On Fri, Oct 16, 2020 at 01:14:04AM +0200, Niklas Söderlund wrote:
> In its early stages the VIN driver did not use an internal scratch
> buffer. This lead to a unnecessary complex start and stop behavior,

This leads

> specially for TB/BT. The driver now always allocates a scratch buffer to
> deal with buffer under-runs, use the scratch buffer to also simplify
> starting and stopping.
>
> When capture is starting use the scratch buffer instead of a user-space
> buffers while syncing the driver with the hardware state. This allows
> the driver to know that no user-space buffers is given to the hardware

s/buffers/buffer

> before the running state is reached.
>
> When capture is stopping use the scratch buffer instead of leaving the
> user-space buffers in place and add a check that all user-space buffers
> are processed by the hardware before transitioning from the stopping to
> stopped state. This allows the driver to know all user-space buffers
> given to the hardware are fully processed.
>
> This change in itself does not improve the driver much but it paves way

paves the way ?

> for future simplifications and enhancements. One direct improvement of
> this change is that TB/BT buffers returned to user-space wile stopping

s/wile/while

> will always contains both fields, that was not guaranteed before.

contain

>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@xxxxxxxxxxxx>
> ---
>  drivers/media/platform/rcar-vin/rcar-dma.c | 30 +++++++++++++++-------
>  1 file changed, 21 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
> index 692dea300b0de8b5..ca90bde8d29f77d1 100644
> --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> @@ -905,7 +905,7 @@ static void rvin_fill_hw_slot(struct rvin_dev *vin, int slot)
>  				vin->format.sizeimage / 2;
>  			break;
>  		}
> -	} else if (list_empty(&vin->buf_list)) {
> +	} else if (vin->state != RUNNING || list_empty(&vin->buf_list)) {

Does this make the

	for (slot = 0; slot < HW_BUFFER_NUM; slot++)
		rvin_fill_hw_slot(vin, slot);

cycle in rvin_capture_start() a no-op, as it already sets

	for (slot = 0; slot < HW_BUFFER_NUM; slot++) {
		vin->buf_hw[slot].buffer = NULL;
		vin->buf_hw[slot].type = FULL;
	}

just before that entering the loop ?

>  		vin->buf_hw[slot].buffer = NULL;
>  		vin->buf_hw[slot].type = FULL;
>  		phys_addr = vin->scratch_phys;
> @@ -998,12 +998,6 @@ static irqreturn_t rvin_irq(int irq, void *data)
>  		goto done;
>  	}
>
> -	/* Nothing to do if capture status is 'STOPPING' */
> -	if (vin->state == STOPPING) {
> -		vin_dbg(vin, "IRQ while state stopping\n");
> -		goto done;
> -	}
> -
>  	/* Prepare for capture and update state */
>  	vnms = rvin_read(vin, VNMS_REG);
>  	slot = (vnms & VNMS_FBS_MASK) >> VNMS_FBS_SHIFT;
> @@ -1313,14 +1307,32 @@ static int rvin_start_streaming(struct vb2_queue *vq, unsigned int count)
>  static void rvin_stop_streaming(struct vb2_queue *vq)
>  {
>  	struct rvin_dev *vin = vb2_get_drv_priv(vq);
> +	unsigned int i, retries;
>  	unsigned long flags;
> -	int retries = 0;
> +	bool buffersFreed;
>
>  	spin_lock_irqsave(&vin->qlock, flags);
>
>  	vin->state = STOPPING;
>
> +	/* Wait until only scratch buffer is used, max 3 interrupts. */

nit: I see RVIN_RETRIES being 10. The number of buffers is 3. The number
of interrutps is less relevant than the fact we might end up waiting
1 second (10 * 100ms) before stopping ?

> +	retries = 0;
> +	while (retries++ < RVIN_RETRIES) {

> +		for (i = 0; i < HW_BUFFER_NUM; i++)
> +			if (vin->buf_hw[i].buffer)
> +				buffersFreed = false;
> +
> +		if (buffersFreed)
> +			break;
> +
> +		spin_unlock_irqrestore(&vin->qlock, flags);
> +		msleep(RVIN_TIMEOUT_MS);

As you're waiting for an interrupt, msleep_interruptible() might be a
better choice ?

Mostly just nits, so:
Reviewed-by: Jacopo Mondi <jacopo+renesas@xxxxxxxxxx>

Thanks
  j

> +		spin_lock_irqsave(&vin->qlock, flags);
> +	}
> +
>  	/* Wait for streaming to stop */
> +	retries = 0;
>  	while (retries++ < RVIN_RETRIES) {
>
>  		rvin_capture_stop(vin);
> @@ -1336,7 +1348,7 @@ static void rvin_stop_streaming(struct vb2_queue *vq)
>  		spin_lock_irqsave(&vin->qlock, flags);
>  	}
>
> -	if (vin->state != STOPPED) {
> +	if (!buffersFreed || vin->state != STOPPED) {
>  		/*
>  		 * If this happens something have gone horribly wrong.
>  		 * Set state to stopped to prevent the interrupt handler
> --
> 2.28.0
>



[Index of Archives]     [Linux Samsung SOC]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]

  Powered by Linux