| Message ID | 1346400670-16002-8-git-send-email-p.zabel@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Philipp, On 31 August 2012 10:11, Philipp Zabel <p.zabel@pengutronix.de> wrote: > Add a 1 second timeout for each PIC_RUN command to the CODA. In > case it locks up, stop all queues and dequeue remaining buffers. > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> > --- > Changes since v2: > - Call cancel_delayed_work in coda_stop_streaming instead of coda_irq_handler. > --- > drivers/media/platform/coda.c | 21 +++++++++++++++++++++ > 1 file changed, 21 insertions(+) > > diff --git a/drivers/media/platform/coda.c b/drivers/media/platform/coda.c > index 7bc2d87..6e3f026 100644 > --- a/drivers/media/platform/coda.c > +++ b/drivers/media/platform/coda.c > @@ -137,6 +137,7 @@ struct coda_dev { > struct vb2_alloc_ctx *alloc_ctx; > struct list_head instances; > unsigned long instance_mask; > + struct delayed_work timeout; > }; > > struct coda_params { > @@ -723,6 +724,9 @@ static void coda_device_run(void *m2m_priv) > CODA7_REG_BIT_AXI_SRAM_USE); > } > > + /* 1 second timeout in case CODA locks up */ > + schedule_delayed_work(&dev->timeout, HZ); > + > coda_command_async(ctx, CODA_COMMAND_PIC_RUN); > } > > @@ -1221,6 +1225,8 @@ static int coda_stop_streaming(struct vb2_queue *q) > } > > if (!ctx->rawstreamon && !ctx->compstreamon) { > + cancel_delayed_work(&dev->timeout); > + This breaks compilation. There is no such variable 'dev' in this function at this time. I see you add it later in patch 9 but I think we should avoid breaking bisect as long as possible. CC drivers/media/video/coda.o drivers/media/video/coda.c: In function 'coda_stop_streaming': drivers/media/video/coda.c:1227: error: 'dev' undeclared (first use in this function) drivers/media/video/coda.c:1227: error: (Each undeclared identifier is reported only once drivers/media/video/coda.c:1227: error: for each function it appears in.) Could you please add it in this patch instead? > v4l2_dbg(1, coda_debug, &ctx->dev->v4l2_dev, > "%s: sent command 'SEQ_END' to coda\n", __func__); > if (coda_command_sync(ctx, CODA_COMMAND_SEQ_END)) { > @@ -1565,6 +1571,20 @@ static irqreturn_t coda_irq_handler(int irq, void *data) > return IRQ_HANDLED; > } > > +static void coda_timeout(struct work_struct *work) > +{ > + struct coda_ctx *ctx; > + struct coda_dev *dev = container_of(to_delayed_work(work), > + struct coda_dev, timeout); > + > + v4l2_err(&dev->v4l2_dev, "CODA PIC_RUN timeout, stopping all streams\n"); > + > + list_for_each_entry(ctx, &dev->instances, list) { > + v4l2_m2m_streamoff(NULL, ctx->m2m_ctx, V4L2_BUF_TYPE_VIDEO_OUTPUT); > + v4l2_m2m_streamoff(NULL, ctx->m2m_ctx, V4L2_BUF_TYPE_VIDEO_CAPTURE); > + } > +} > + > static u32 coda_supported_firmwares[] = { > CODA_FIRMWARE_VERNUM(CODA_DX6, 2, 2, 5), > CODA_FIRMWARE_VERNUM(CODA_7541, 13, 4, 29), > @@ -1836,6 +1856,7 @@ static int __devinit coda_probe(struct platform_device *pdev) > > spin_lock_init(&dev->irqlock); > INIT_LIST_HEAD(&dev->instances); > + INIT_DELAYED_WORK(&dev->timeout, coda_timeout); > > dev->plat_dev = pdev; > dev->clk_per = devm_clk_get(&pdev->dev, "per"); > -- > 1.7.10.4 >
On 31 August 2012 10:11, Philipp Zabel <p.zabel@pengutronix.de> wrote: > Add a 1 second timeout for each PIC_RUN command to the CODA. In > case it locks up, stop all queues and dequeue remaining buffers. > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> > --- > Changes since v2: > - Call cancel_delayed_work in coda_stop_streaming instead of coda_irq_handler. > --- > drivers/media/platform/coda.c | 21 +++++++++++++++++++++ > 1 file changed, 21 insertions(+) > > diff --git a/drivers/media/platform/coda.c b/drivers/media/platform/coda.c > index 7bc2d87..6e3f026 100644 > --- a/drivers/media/platform/coda.c > +++ b/drivers/media/platform/coda.c > @@ -137,6 +137,7 @@ struct coda_dev { > struct vb2_alloc_ctx *alloc_ctx; > struct list_head instances; > unsigned long instance_mask; > + struct delayed_work timeout; > }; > > struct coda_params { > @@ -723,6 +724,9 @@ static void coda_device_run(void *m2m_priv) > CODA7_REG_BIT_AXI_SRAM_USE); > } > > + /* 1 second timeout in case CODA locks up */ > + schedule_delayed_work(&dev->timeout, HZ); > + > coda_command_async(ctx, CODA_COMMAND_PIC_RUN); > } > > @@ -1221,6 +1225,8 @@ static int coda_stop_streaming(struct vb2_queue *q) > } > > if (!ctx->rawstreamon && !ctx->compstreamon) { > + cancel_delayed_work(&dev->timeout); > + Since 'schedule_delayed_work()' is called for each frame and 'cancel_delayed_work()' is called only when stopping the streaming, the timeout will always trigger after 1 second. I can confirm this due some tests where I always get this message after one second: coda coda-imx27.0: CODA PIC_RUN timeout, stopping all streams Please find some of my doubts below: Do we really need this patch? Couldn't you just use 'coda_command_sync()' for CODA_COMMAND_PIC_RUN? Why did you changed 'cancel_delayed_work()' from the IRQ to stop streaming? I think the former was correct, wasn't it? > v4l2_dbg(1, coda_debug, &ctx->dev->v4l2_dev, > "%s: sent command 'SEQ_END' to coda\n", __func__); > if (coda_command_sync(ctx, CODA_COMMAND_SEQ_END)) { > @@ -1565,6 +1571,20 @@ static irqreturn_t coda_irq_handler(int irq, void *data) > return IRQ_HANDLED; > } > > +static void coda_timeout(struct work_struct *work) > +{ > + struct coda_ctx *ctx; > + struct coda_dev *dev = container_of(to_delayed_work(work), > + struct coda_dev, timeout); > + > + v4l2_err(&dev->v4l2_dev, "CODA PIC_RUN timeout, stopping all streams\n"); > + > + list_for_each_entry(ctx, &dev->instances, list) { > + v4l2_m2m_streamoff(NULL, ctx->m2m_ctx, V4L2_BUF_TYPE_VIDEO_OUTPUT); > + v4l2_m2m_streamoff(NULL, ctx->m2m_ctx, V4L2_BUF_TYPE_VIDEO_CAPTURE); > + } > +} > + > static u32 coda_supported_firmwares[] = { > CODA_FIRMWARE_VERNUM(CODA_DX6, 2, 2, 5), > CODA_FIRMWARE_VERNUM(CODA_7541, 13, 4, 29), > @@ -1836,6 +1856,7 @@ static int __devinit coda_probe(struct platform_device *pdev) > > spin_lock_init(&dev->irqlock); > INIT_LIST_HEAD(&dev->instances); > + INIT_DELAYED_WORK(&dev->timeout, coda_timeout); > > dev->plat_dev = pdev; > dev->clk_per = devm_clk_get(&pdev->dev, "per"); > -- > 1.7.10.4 >
Hi Javier, Am Montag, den 03.09.2012, 14:18 +0200 schrieb javier Martin: > On 31 August 2012 10:11, Philipp Zabel <p.zabel@pengutronix.de> wrote: > > Add a 1 second timeout for each PIC_RUN command to the CODA. In > > case it locks up, stop all queues and dequeue remaining buffers. > > > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> > > --- > > Changes since v2: > > - Call cancel_delayed_work in coda_stop_streaming instead of coda_irq_handler. > > --- > > drivers/media/platform/coda.c | 21 +++++++++++++++++++++ > > 1 file changed, 21 insertions(+) > > > > diff --git a/drivers/media/platform/coda.c b/drivers/media/platform/coda.c > > index 7bc2d87..6e3f026 100644 > > --- a/drivers/media/platform/coda.c > > +++ b/drivers/media/platform/coda.c > > @@ -137,6 +137,7 @@ struct coda_dev { > > struct vb2_alloc_ctx *alloc_ctx; > > struct list_head instances; > > unsigned long instance_mask; > > + struct delayed_work timeout; > > }; > > > > struct coda_params { > > @@ -723,6 +724,9 @@ static void coda_device_run(void *m2m_priv) > > CODA7_REG_BIT_AXI_SRAM_USE); > > } > > > > + /* 1 second timeout in case CODA locks up */ > > + schedule_delayed_work(&dev->timeout, HZ); > > + > > coda_command_async(ctx, CODA_COMMAND_PIC_RUN); > > } > > > > @@ -1221,6 +1225,8 @@ static int coda_stop_streaming(struct vb2_queue *q) > > } > > > > if (!ctx->rawstreamon && !ctx->compstreamon) { > > + cancel_delayed_work(&dev->timeout); > > + > > Since 'schedule_delayed_work()' is called for each frame and > 'cancel_delayed_work()' is called only when stopping the streaming, > the timeout will always trigger after 1 second. > I can confirm this due some tests where I always get this message > after one second: coda coda-imx27.0: CODA PIC_RUN timeout, stopping > all streams Yes, removing the __cancel_delayed_work in coda_irq_handler was a mistake. > Please find some of my doubts below: > > Do we really need this patch? Couldn't you just use > 'coda_command_sync()' for CODA_COMMAND_PIC_RUN? Why did you changed > 'cancel_delayed_work()' from the IRQ to stop streaming? I think the > former was correct, wasn't it? Ideally, the CODA shouldn't hang. I've seen it happen, though - whether because of firmware or driver bugs I couldn't say. Using coda_command_sync would mean spinning in coda_wait_timeout all the time. I guess we could put device_run to sleep with wait_for_completion_interruptible_timeout and just complete the completion in the irq handler. regards Philipp -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Javier, Am Montag, den 03.09.2012, 14:01 +0200 schrieb javier Martin: > Hi Philipp, > > On 31 August 2012 10:11, Philipp Zabel <p.zabel@pengutronix.de> wrote: > > Add a 1 second timeout for each PIC_RUN command to the CODA. In > > case it locks up, stop all queues and dequeue remaining buffers. > > > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> > > --- > > Changes since v2: > > - Call cancel_delayed_work in coda_stop_streaming instead of coda_irq_handler. > > --- > > drivers/media/platform/coda.c | 21 +++++++++++++++++++++ > > 1 file changed, 21 insertions(+) > > > > diff --git a/drivers/media/platform/coda.c b/drivers/media/platform/coda.c > > index 7bc2d87..6e3f026 100644 > > --- a/drivers/media/platform/coda.c > > +++ b/drivers/media/platform/coda.c > > @@ -137,6 +137,7 @@ struct coda_dev { > > struct vb2_alloc_ctx *alloc_ctx; > > struct list_head instances; > > unsigned long instance_mask; > > + struct delayed_work timeout; > > }; > > > > struct coda_params { > > @@ -723,6 +724,9 @@ static void coda_device_run(void *m2m_priv) > > CODA7_REG_BIT_AXI_SRAM_USE); > > } > > > > + /* 1 second timeout in case CODA locks up */ > > + schedule_delayed_work(&dev->timeout, HZ); > > + > > coda_command_async(ctx, CODA_COMMAND_PIC_RUN); > > } > > > > @@ -1221,6 +1225,8 @@ static int coda_stop_streaming(struct vb2_queue *q) > > } > > > > if (!ctx->rawstreamon && !ctx->compstreamon) { > > + cancel_delayed_work(&dev->timeout); > > + > > This breaks compilation. There is no such variable 'dev' in this > function at this time. > I see you add it later in patch 9 but I think we should avoid breaking > bisect as long as possible. > > CC drivers/media/video/coda.o > drivers/media/video/coda.c: In function 'coda_stop_streaming': > drivers/media/video/coda.c:1227: error: 'dev' undeclared (first use in > this function) > drivers/media/video/coda.c:1227: error: (Each undeclared identifier is > reported only once > drivers/media/video/coda.c:1227: error: for each function it appears in.) > > Could you please add it in this patch instead? Thank you, I'll fix this next round. regards Philipp -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/media/platform/coda.c b/drivers/media/platform/coda.c index 7bc2d87..6e3f026 100644 --- a/drivers/media/platform/coda.c +++ b/drivers/media/platform/coda.c @@ -137,6 +137,7 @@ struct coda_dev { struct vb2_alloc_ctx *alloc_ctx; struct list_head instances; unsigned long instance_mask; + struct delayed_work timeout; }; struct coda_params { @@ -723,6 +724,9 @@ static void coda_device_run(void *m2m_priv) CODA7_REG_BIT_AXI_SRAM_USE); } + /* 1 second timeout in case CODA locks up */ + schedule_delayed_work(&dev->timeout, HZ); + coda_command_async(ctx, CODA_COMMAND_PIC_RUN); } @@ -1221,6 +1225,8 @@ static int coda_stop_streaming(struct vb2_queue *q) } if (!ctx->rawstreamon && !ctx->compstreamon) { + cancel_delayed_work(&dev->timeout); + v4l2_dbg(1, coda_debug, &ctx->dev->v4l2_dev, "%s: sent command 'SEQ_END' to coda\n", __func__); if (coda_command_sync(ctx, CODA_COMMAND_SEQ_END)) { @@ -1565,6 +1571,20 @@ static irqreturn_t coda_irq_handler(int irq, void *data) return IRQ_HANDLED; } +static void coda_timeout(struct work_struct *work) +{ + struct coda_ctx *ctx; + struct coda_dev *dev = container_of(to_delayed_work(work), + struct coda_dev, timeout); + + v4l2_err(&dev->v4l2_dev, "CODA PIC_RUN timeout, stopping all streams\n"); + + list_for_each_entry(ctx, &dev->instances, list) { + v4l2_m2m_streamoff(NULL, ctx->m2m_ctx, V4L2_BUF_TYPE_VIDEO_OUTPUT); + v4l2_m2m_streamoff(NULL, ctx->m2m_ctx, V4L2_BUF_TYPE_VIDEO_CAPTURE); + } +} + static u32 coda_supported_firmwares[] = { CODA_FIRMWARE_VERNUM(CODA_DX6, 2, 2, 5), CODA_FIRMWARE_VERNUM(CODA_7541, 13, 4, 29), @@ -1836,6 +1856,7 @@ static int __devinit coda_probe(struct platform_device *pdev) spin_lock_init(&dev->irqlock); INIT_LIST_HEAD(&dev->instances); + INIT_DELAYED_WORK(&dev->timeout, coda_timeout); dev->plat_dev = pdev; dev->clk_per = devm_clk_get(&pdev->dev, "per");
Add a 1 second timeout for each PIC_RUN command to the CODA. In case it locks up, stop all queues and dequeue remaining buffers. Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> --- Changes since v2: - Call cancel_delayed_work in coda_stop_streaming instead of coda_irq_handler. --- drivers/media/platform/coda.c | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+)