| Message ID | 20250122165353.1273739-4-niklas.soderlund+renesas@ragnatech.se (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | media: rcar-vin: Remove emulated SEQ_{TB,BT} | expand |
Hi, On 22/01/2025 18:53, Niklas Söderlund wrote: > The STARTING state is superfluous and can be replaced with a check of > the sequence counter. The design idea is that the first buffer returned > from the driver have to come from the first hardware buffer slot. > Failing this the first 3 buffers queued to the device can be returned > out-of-order. > > But it's much clearer to check the sequence counter to only return the > first buffer if it comes from hardware slot 0 then it is to carry around > an extra state just for this. Remove the unneeded state and replace it > with a simpler check. > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > --- > drivers/media/platform/renesas/rcar-vin/rcar-dma.c | 5 ++--- > drivers/media/platform/renesas/rcar-vin/rcar-vin.h | 2 -- > 2 files changed, 2 insertions(+), 5 deletions(-) Reviewed-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com> I don't understand the feature, though =). Why does the first buffer have to come from slot 0? Tomi > > diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-dma.c b/drivers/media/platform/renesas/rcar-vin/rcar-dma.c > index a16adc6fd4dc..ba55ccf648de 100644 > --- a/drivers/media/platform/renesas/rcar-vin/rcar-dma.c > +++ b/drivers/media/platform/renesas/rcar-vin/rcar-dma.c > @@ -1064,7 +1064,7 @@ static int rvin_capture_start(struct rvin_dev *vin) > /* Continuous Frame Capture Mode */ > rvin_write(vin, VNFC_C_FRAME, VNFC_REG); > > - vin->state = STARTING; > + vin->state = RUNNING; > > return 0; > } > @@ -1120,14 +1120,13 @@ static irqreturn_t rvin_irq(int irq, void *data) > * To hand buffers back in a known order to userspace start > * to capture first from slot 0. > */ > - if (vin->state == STARTING) { > + if (!vin->sequence) { > if (slot != 0) { > vin_dbg(vin, "Starting sync slot: %d\n", slot); > goto done; > } > > vin_dbg(vin, "Capture start synced!\n"); > - vin->state = RUNNING; > } > > /* Capture frame */ > diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-vin.h b/drivers/media/platform/renesas/rcar-vin/rcar-vin.h > index 4cb25d8bbf32..f13ef379d095 100644 > --- a/drivers/media/platform/renesas/rcar-vin/rcar-vin.h > +++ b/drivers/media/platform/renesas/rcar-vin/rcar-vin.h > @@ -64,13 +64,11 @@ enum rvin_isp_id { > /** > * enum rvin_dma_state - DMA states > * @STOPPED: No operation in progress > - * @STARTING: Capture starting up > * @RUNNING: Operation in progress have buffers > * @STOPPING: Stopping operation > */ > enum rvin_dma_state { > STOPPED = 0, > - STARTING, > RUNNING, > STOPPING, > };
Ho Tomi, On 2025-01-23 09:24:07 +0200, Tomi Valkeinen wrote: > Hi, > > On 22/01/2025 18:53, Niklas Söderlund wrote: > > The STARTING state is superfluous and can be replaced with a check of > > the sequence counter. The design idea is that the first buffer returned > > from the driver have to come from the first hardware buffer slot. > > Failing this the first 3 buffers queued to the device can be returned > > out-of-order. > > > > But it's much clearer to check the sequence counter to only return the > > first buffer if it comes from hardware slot 0 then it is to carry around > > an extra state just for this. Remove the unneeded state and replace it > > with a simpler check. > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > > --- > > drivers/media/platform/renesas/rcar-vin/rcar-dma.c | 5 ++--- > > drivers/media/platform/renesas/rcar-vin/rcar-vin.h | 2 -- > > 2 files changed, 2 insertions(+), 5 deletions(-) > > Reviewed-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com> > > I don't understand the feature, though =). Why does the first buffer have to > come from slot 0? The VIN have 3 slots it can use when streaming in continues mode. The usual operation is that it starts with slot 0 for the first capture, then moves to slot 1, slot 2, slot 0, etc. It was observed on that sometimes the first capture interrupt we get comes from a slot other then 0. In that case up to the 3 first frames returned from the device are out-of-order and that is not good. This check is to check for that seldom trigged condition and drop 1 or 2 frames when starting capture in order for it to sync so buffers are always returned in the order they where queued. IIRC this issue was only ever observed on Gen2 after the system had been suspended and then resumed. But I can't recall if the VIN had to be streaming at suspend time for this issue to have a chance to hit. > > Tomi > > > > > diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-dma.c b/drivers/media/platform/renesas/rcar-vin/rcar-dma.c > > index a16adc6fd4dc..ba55ccf648de 100644 > > --- a/drivers/media/platform/renesas/rcar-vin/rcar-dma.c > > +++ b/drivers/media/platform/renesas/rcar-vin/rcar-dma.c > > @@ -1064,7 +1064,7 @@ static int rvin_capture_start(struct rvin_dev *vin) > > /* Continuous Frame Capture Mode */ > > rvin_write(vin, VNFC_C_FRAME, VNFC_REG); > > - vin->state = STARTING; > > + vin->state = RUNNING; > > return 0; > > } > > @@ -1120,14 +1120,13 @@ static irqreturn_t rvin_irq(int irq, void *data) > > * To hand buffers back in a known order to userspace start > > * to capture first from slot 0. > > */ > > - if (vin->state == STARTING) { > > + if (!vin->sequence) { > > if (slot != 0) { > > vin_dbg(vin, "Starting sync slot: %d\n", slot); > > goto done; > > } > > vin_dbg(vin, "Capture start synced!\n"); > > - vin->state = RUNNING; > > } > > /* Capture frame */ > > diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-vin.h b/drivers/media/platform/renesas/rcar-vin/rcar-vin.h > > index 4cb25d8bbf32..f13ef379d095 100644 > > --- a/drivers/media/platform/renesas/rcar-vin/rcar-vin.h > > +++ b/drivers/media/platform/renesas/rcar-vin/rcar-vin.h > > @@ -64,13 +64,11 @@ enum rvin_isp_id { > > /** > > * enum rvin_dma_state - DMA states > > * @STOPPED: No operation in progress > > - * @STARTING: Capture starting up > > * @RUNNING: Operation in progress have buffers > > * @STOPPING: Stopping operation > > */ > > enum rvin_dma_state { > > STOPPED = 0, > > - STARTING, > > RUNNING, > > STOPPING, > > }; >
diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-dma.c b/drivers/media/platform/renesas/rcar-vin/rcar-dma.c index a16adc6fd4dc..ba55ccf648de 100644 --- a/drivers/media/platform/renesas/rcar-vin/rcar-dma.c +++ b/drivers/media/platform/renesas/rcar-vin/rcar-dma.c @@ -1064,7 +1064,7 @@ static int rvin_capture_start(struct rvin_dev *vin) /* Continuous Frame Capture Mode */ rvin_write(vin, VNFC_C_FRAME, VNFC_REG); - vin->state = STARTING; + vin->state = RUNNING; return 0; } @@ -1120,14 +1120,13 @@ static irqreturn_t rvin_irq(int irq, void *data) * To hand buffers back in a known order to userspace start * to capture first from slot 0. */ - if (vin->state == STARTING) { + if (!vin->sequence) { if (slot != 0) { vin_dbg(vin, "Starting sync slot: %d\n", slot); goto done; } vin_dbg(vin, "Capture start synced!\n"); - vin->state = RUNNING; } /* Capture frame */ diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-vin.h b/drivers/media/platform/renesas/rcar-vin/rcar-vin.h index 4cb25d8bbf32..f13ef379d095 100644 --- a/drivers/media/platform/renesas/rcar-vin/rcar-vin.h +++ b/drivers/media/platform/renesas/rcar-vin/rcar-vin.h @@ -64,13 +64,11 @@ enum rvin_isp_id { /** * enum rvin_dma_state - DMA states * @STOPPED: No operation in progress - * @STARTING: Capture starting up * @RUNNING: Operation in progress have buffers * @STOPPING: Stopping operation */ enum rvin_dma_state { STOPPED = 0, - STARTING, RUNNING, STOPPING, };
The STARTING state is superfluous and can be replaced with a check of the sequence counter. The design idea is that the first buffer returned from the driver have to come from the first hardware buffer slot. Failing this the first 3 buffers queued to the device can be returned out-of-order. But it's much clearer to check the sequence counter to only return the first buffer if it comes from hardware slot 0 then it is to carry around an extra state just for this. Remove the unneeded state and replace it with a simpler check. Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> --- drivers/media/platform/renesas/rcar-vin/rcar-dma.c | 5 ++--- drivers/media/platform/renesas/rcar-vin/rcar-vin.h | 2 -- 2 files changed, 2 insertions(+), 5 deletions(-)