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
>
