| Message ID | 1404812474-7627-3-git-send-email-ian.molton@codethink.co.uk (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
On 07/08/2014 01:41 PM, Ian Molton wrote: > Videobuf2 complains about buffers that are still marked ACTIVE (in use by the driver) following a call to stop_streaming(). > This patch returns all active buffers to state ERROR prior to stopping. > Note: this introduces a (non fatal) race condition as the stream is not guaranteed to be stopped at this point. > Signed-off-by: Ian Molton <ian.molton@codethink.co.uk> > Signed-off-by: William Towle <william.towle@codethink.co.uk> Fixed kernel WARNINGs for me! \o/ Ian, perhaps it makes sense for me to take these patches into my hands? WBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/08/2014 11:41 AM, Ian Molton wrote: > Videobuf2 complains about buffers that are still marked ACTIVE (in use by the driver) following a call to stop_streaming(). > > This patch returns all active buffers to state ERROR prior to stopping. > > Note: this introduces a (non fatal) race condition as the stream is not guaranteed to be stopped at this point. > > Signed-off-by: Ian Molton <ian.molton@codethink.co.uk> > Signed-off-by: William Towle <william.towle@codethink.co.uk> > --- > drivers/media/platform/soc_camera/rcar_vin.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/media/platform/soc_camera/rcar_vin.c b/drivers/media/platform/soc_camera/rcar_vin.c > index 7154500..06ce705 100644 > --- a/drivers/media/platform/soc_camera/rcar_vin.c > +++ b/drivers/media/platform/soc_camera/rcar_vin.c > @@ -513,8 +513,14 @@ static void rcar_vin_stop_streaming(struct vb2_queue *vq) > struct soc_camera_host *ici = to_soc_camera_host(icd->parent); > struct rcar_vin_priv *priv = ici->priv; > struct list_head *buf_head, *tmp; > + int i; > > spin_lock_irq(&priv->lock); > + > + for (i = 0; i < vq->num_buffers; ++i) > + if (vq->bufs[i]->state == VB2_BUF_STATE_ACTIVE) > + vb2_buffer_done(vq->bufs[i], VB2_BUF_STATE_ERROR); > + > list_for_each_safe(buf_head, tmp, &priv->capture) > list_del_init(buf_head); I'm assuming all buffers that are queued to the driver via buf_queue() are linked into priv->capture. So you would typically call vb2_buffer_done when you are walking that list: list_for_each_safe(buf_head, tmp, &priv->capture) { // usually you go from buf_head to the real buffer struct // containing a vb2_buffer struct vb2_buffer_done(&buf->vb, VB2_BUF_STATE_ERROR); list_del_init(buf_head); } Please use this rather than looking into internal vb2_queue datastructures. Regards, Hans > spin_unlock_irq(&priv->lock); > -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> Fixed kernel WARNINGs for me! \o/ > Ian, perhaps it makes sense for me to take these patches into my hands? I'm planning to respin these tomorrow - is that OK? I have test hardware with two different frontends here. -Ian -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello. On 08/13/2014 09:30 PM, Ian Molton wrote: >> Fixed kernel WARNINGs for me! \o/ >> Ian, perhaps it makes sense for me to take these patches into my hands? > I'm planning to respin these tomorrow - is that OK? Yes. > I have test hardware with two different frontends here. I'm sorry, what do you mean by frontends? > -Ian WBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Ian, On Wed, 13 Aug 2014, Ian Molton wrote: > > > Fixed kernel WARNINGs for me! \o/ > > Ian, perhaps it makes sense for me to take these patches into my hands? > > I'm planning to respin these tomorrow - is that OK? I have test hardware with two different frontends here. Any progress on this? Thanks Guennadi > -Ian -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Ian, Apparently, I wasn't quite right in my reply to patch 1 from this series. It seems to be related to this one and to Hans' comment below. Taking it into account, it seems you shouldn't remove cancelled active buffers from the list without calling vb2_buffer_done(&buf->vb, VB2_BUF_STATE_ERROR) for them. So, I think you should modify your patch 1 to either leave those buffers on the list and report them "done" here, or call "done" for them when removing from the list. I think I'd rather go with the latter option to not keep failed buffers on the list and rely, that the vb2 core will stop streaming soon. Hans, do you agree? Thanks Guennadi On Mon, 4 Aug 2014, Hans Verkuil wrote: > On 07/08/2014 11:41 AM, Ian Molton wrote: > > Videobuf2 complains about buffers that are still marked ACTIVE (in use by the driver) following a call to stop_streaming(). > > > > This patch returns all active buffers to state ERROR prior to stopping. > > > > Note: this introduces a (non fatal) race condition as the stream is not guaranteed to be stopped at this point. > > > > Signed-off-by: Ian Molton <ian.molton@codethink.co.uk> > > Signed-off-by: William Towle <william.towle@codethink.co.uk> > > --- > > drivers/media/platform/soc_camera/rcar_vin.c | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/drivers/media/platform/soc_camera/rcar_vin.c b/drivers/media/platform/soc_camera/rcar_vin.c > > index 7154500..06ce705 100644 > > --- a/drivers/media/platform/soc_camera/rcar_vin.c > > +++ b/drivers/media/platform/soc_camera/rcar_vin.c > > @@ -513,8 +513,14 @@ static void rcar_vin_stop_streaming(struct vb2_queue *vq) > > struct soc_camera_host *ici = to_soc_camera_host(icd->parent); > > struct rcar_vin_priv *priv = ici->priv; > > struct list_head *buf_head, *tmp; > > + int i; > > > > spin_lock_irq(&priv->lock); > > + > > + for (i = 0; i < vq->num_buffers; ++i) > > + if (vq->bufs[i]->state == VB2_BUF_STATE_ACTIVE) > > + vb2_buffer_done(vq->bufs[i], VB2_BUF_STATE_ERROR); > > + > > list_for_each_safe(buf_head, tmp, &priv->capture) > > list_del_init(buf_head); > > I'm assuming all buffers that are queued to the driver via buf_queue() are > linked into priv->capture. So you would typically call vb2_buffer_done > when you are walking that list: > > list_for_each_safe(buf_head, tmp, &priv->capture) { > // usually you go from buf_head to the real buffer struct > // containing a vb2_buffer struct > vb2_buffer_done(&buf->vb, VB2_BUF_STATE_ERROR); > list_del_init(buf_head); > } > > Please use this rather than looking into internal vb2_queue datastructures. > > Regards, > > Hans > > > spin_unlock_irq(&priv->lock); > > > -- To unsubscribe from this list: send the line "unsubscribe linux-sh" 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/soc_camera/rcar_vin.c b/drivers/media/platform/soc_camera/rcar_vin.c index 7154500..06ce705 100644 --- a/drivers/media/platform/soc_camera/rcar_vin.c +++ b/drivers/media/platform/soc_camera/rcar_vin.c @@ -513,8 +513,14 @@ static void rcar_vin_stop_streaming(struct vb2_queue *vq) struct soc_camera_host *ici = to_soc_camera_host(icd->parent); struct rcar_vin_priv *priv = ici->priv; struct list_head *buf_head, *tmp; + int i; spin_lock_irq(&priv->lock); + + for (i = 0; i < vq->num_buffers; ++i) + if (vq->bufs[i]->state == VB2_BUF_STATE_ACTIVE) + vb2_buffer_done(vq->bufs[i], VB2_BUF_STATE_ERROR); + list_for_each_safe(buf_head, tmp, &priv->capture) list_del_init(buf_head); spin_unlock_irq(&priv->lock);