| Message ID | 20201015231408.2399933-2-niklas.soderlund+renesas@ragnatech.se (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series | rcar-vin: Support suspend and resume | expand |
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@ragnatech.se> > --- > 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@jmondi.org> 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 >
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)) { 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. */ + retries = 0; + while (retries++ < RVIN_RETRIES) { + buffersFreed = true; + 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); + 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
In its early stages the VIN driver did not use an internal scratch buffer. This lead to a unnecessary complex start and stop behavior, 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 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 for future simplifications and enhancements. One direct improvement of this change is that TB/BT buffers returned to user-space wile stopping will always contains both fields, that was not guaranteed before. Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> --- drivers/media/platform/rcar-vin/rcar-dma.c | 30 +++++++++++++++------- 1 file changed, 21 insertions(+), 9 deletions(-)