| Message ID | 1418914215.22813.18.camel@xylophone.i.decadent.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hello. On 12/18/2014 05:50 PM, Ben Hutchings wrote: > From: William Towle <william.towle@codethink.co.uk> > Move the buffer state test in the .buf_cleanup handler into > .stop_streaming so that a) the vb2_queue API is not subverted, and > b) tracking of active-state buffers via priv->queue_buf[] is handled > as early as is possible > Signed-off-by: William Towle <william.towle@codethink.co.uk> > --- > drivers/media/platform/soc_camera/rcar_vin.c | 36 ++++++++++---------------- > 1 file changed, 14 insertions(+), 22 deletions(-) > diff --git a/drivers/media/platform/soc_camera/rcar_vin.c b/drivers/media/platform/soc_camera/rcar_vin.c > index 20dbedf..bf60074 100644 > --- a/drivers/media/platform/soc_camera/rcar_vin.c > +++ b/drivers/media/platform/soc_camera/rcar_vin.c [...] > @@ -533,8 +513,20 @@ static void rcar_vin_stop_streaming(struct vb2_queue *vq) > rcar_vin_wait_stop_streaming(priv); > > for (i = 0; i < vq->num_buffers; ++i) > - if (vq->bufs[i]->state == VB2_BUF_STATE_ACTIVE) > + if (vq->bufs[i]->state == VB2_BUF_STATE_ACTIVE) { > + int j; > + > + /* Is this a buffer we have told the > + * hardware about? Update the associated > + * list, if so > + */ > + for (j = 0; j < MAX_BUFFER_NUM; j++) { > + if (priv->queue_buf[j] == vq->bufs[i]) { > + priv->queue_buf[j] = NULL; > + } Don't need {} here. > + } > vb2_buffer_done(vq->bufs[i], VB2_BUF_STATE_ERROR); > + } WBR, Sergei -- 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
On Thu, 18 Dec 2014, Ben Hutchings wrote: > From: William Towle <william.towle@codethink.co.uk> > > Move the buffer state test in the .buf_cleanup handler into > .stop_streaming so that a) the vb2_queue API is not subverted, and > b) tracking of active-state buffers via priv->queue_buf[] is handled > as early as is possible Huh... Sorry, patches 1, 2, 3, and 5 of this series look like a strange way to get from the present state to the destination. They all have to be merged IMHO. Further, looking at vb2, I don't think active buffers should ever stay active until .buf_cleanup() is called. I think rcar_vin's .stop_streaming() should stop the hardware, wait until it's stopped, then walk all queued buffers and return errors for them, while removing them from the list at the same time. Then I don't think a .buf_cleanup() method is needed there at all. Thanks Guennadi > > Signed-off-by: William Towle <william.towle@codethink.co.uk> > --- > drivers/media/platform/soc_camera/rcar_vin.c | 36 ++++++++++---------------- > 1 file changed, 14 insertions(+), 22 deletions(-) > > diff --git a/drivers/media/platform/soc_camera/rcar_vin.c b/drivers/media/platform/soc_camera/rcar_vin.c > index 20dbedf..bf60074 100644 > --- a/drivers/media/platform/soc_camera/rcar_vin.c > +++ b/drivers/media/platform/soc_camera/rcar_vin.c > @@ -486,28 +486,8 @@ static void rcar_vin_videobuf_release(struct vb2_buffer *vb) > struct soc_camera_device *icd = soc_camera_from_vb2q(vb->vb2_queue); > struct soc_camera_host *ici = to_soc_camera_host(icd->parent); > struct rcar_vin_priv *priv = ici->priv; > - unsigned int i; > - int buf_in_use = 0; > - spin_lock_irq(&priv->lock); > - > - /* Is the buffer in use by the VIN hardware? */ > - for (i = 0; i < MAX_BUFFER_NUM; i++) { > - if (priv->queue_buf[i] == vb) { > - buf_in_use = 1; > - break; > - } > - } > > - if (buf_in_use) { > - rcar_vin_wait_stop_streaming(priv); > - > - /* > - * Capturing has now stopped. The buffer we have been asked > - * to release could be any of the current buffers in use, so > - * release all buffers that are in use by HW > - */ > - priv->queue_buf[i] = NULL; > - } > + spin_lock_irq(&priv->lock); > > list_del_init(to_buf_list(vb)); > > @@ -533,8 +513,20 @@ static void rcar_vin_stop_streaming(struct vb2_queue *vq) > rcar_vin_wait_stop_streaming(priv); > > for (i = 0; i < vq->num_buffers; ++i) > - if (vq->bufs[i]->state == VB2_BUF_STATE_ACTIVE) > + if (vq->bufs[i]->state == VB2_BUF_STATE_ACTIVE) { > + int j; > + > + /* Is this a buffer we have told the > + * hardware about? Update the associated > + * list, if so > + */ > + for (j = 0; j < MAX_BUFFER_NUM; j++) { > + if (priv->queue_buf[j] == vq->bufs[i]) { > + priv->queue_buf[j] = NULL; > + } > + } > vb2_buffer_done(vq->bufs[i], VB2_BUF_STATE_ERROR); > + } > > list_for_each_safe(buf_head, tmp, &priv->capture) > list_del_init(buf_head); > -- > 1.7.10.4 > > > -- 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
On Sun, 2015-01-18 at 22:23 +0100, Guennadi Liakhovetski wrote: > On Thu, 18 Dec 2014, Ben Hutchings wrote: > > > From: William Towle <william.towle@codethink.co.uk> > > > > Move the buffer state test in the .buf_cleanup handler into > > .stop_streaming so that a) the vb2_queue API is not subverted, and > > b) tracking of active-state buffers via priv->queue_buf[] is handled > > as early as is possible > > Huh... Sorry, patches 1, 2, 3, and 5 of this series look like a strange > way to get from the present state to the destination. They all have to be > merged IMHO. [...] Well, I thought that too. Will's submission from last week has that change: http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/87009 Ben. -- 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
On Mon, 19 Jan 2015, Ben Hutchings wrote: > On Sun, 2015-01-18 at 22:23 +0100, Guennadi Liakhovetski wrote: > > On Thu, 18 Dec 2014, Ben Hutchings wrote: > > > > > From: William Towle <william.towle@codethink.co.uk> > > > > > > Move the buffer state test in the .buf_cleanup handler into > > > .stop_streaming so that a) the vb2_queue API is not subverted, and > > > b) tracking of active-state buffers via priv->queue_buf[] is handled > > > as early as is possible > > > > Huh... Sorry, patches 1, 2, 3, and 5 of this series look like a strange > > way to get from the present state to the destination. They all have to be > > merged IMHO. > [...] > > Well, I thought that too. Will's submission from last week has that > change: > http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/87009 Hm... interesting, why I didn't get those mails in my INBOX, although I do see myself in CC... Only got them from the list, and no, I don't have "avoid copies" enabled. Anyway, yes, that looks better! But I would still consider keeping buffers on the list in .buf_clean(), in which case you can remove it. And walk the list instead of the VB2 internal buffer array, as others have pointed out. Thanks Guennadi -- 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
On Mon, 19 Jan 2015, Guennadi Liakhovetski wrote: >>> On Thu, 18 Dec 2014, Ben Hutchings wrote: >> Well, I thought that too. Will's submission from last week has that >> change: >> http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/87009 > Anyway, yes, that looks better! But I would still consider keeping buffers > on the list in .buf_clean(), in which case you can remove it. And walk the > list instead of the VB2 internal buffer array, as others have pointed out. Hi Guennadi, Thanks for the clarification. Ian (when he was with us) did say "it was particularly difficult to understand WTH this driver was doing". Regarding your first point, if it's safe to skip the actions left in rcar_vin_videobuf_release() then I will do a further rework to remove it completely. Regarding your second, in the patchset Ben linked to above we think we have the appropriate loops: a for loop for queue_buf[], and list_for_each_safe() for anything left in priv->capture; this is consistent with rcar_vin_fill_hw_slot() setting up queue_buf[] with pointers unlinked from priv->capture. This in turn suggests that we are right not to call list_del_init() in both of rcar_vin_stop_streaming()'s loops ... as long as I've correctly interpreted the code and everyone's feedback thus far. Cheers, Wills. -- 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, On Mon, 19 Jan 2015, William Towle wrote: > > On Mon, 19 Jan 2015, Guennadi Liakhovetski wrote: > > > > > On Thu, 18 Dec 2014, Ben Hutchings wrote: > > > Well, I thought that too. Will's submission from last week has that > > > change: > > > http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/87009 > > > Anyway, yes, that looks better! But I would still consider keeping buffers > > on the list in .buf_clean(), in which case you can remove it. And walk the > > list instead of the VB2 internal buffer array, as others have pointed out. > > Hi Guennadi, > Thanks for the clarification. Ian (when he was with us) did say "it > was particularly difficult to understand WTH this driver was doing". > > Regarding your first point, if it's safe to skip the actions left > in rcar_vin_videobuf_release() then I will do a further rework to > remove it completely. > > Regarding your second, in the patchset Ben linked to above we think > we have the appropriate loops: a for loop for queue_buf[], and > list_for_each_safe() for anything left in priv->capture; this is > consistent with rcar_vin_fill_hw_slot() setting up queue_buf[] with > pointers unlinked from priv->capture. This in turn suggests that we > are right not to call list_del_init() in both of > rcar_vin_stop_streaming()'s loops ... as long as I've correctly > interpreted the code and everyone's feedback thus far. I'm referring to this comment by Hans Verkuil of 14 August last year: > 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. I think, that's the right way to implement that clean up loop. Thanks Guennadi -- 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
On 01/19/2015 03:11 PM, William Towle wrote: > > On Mon, 19 Jan 2015, Guennadi Liakhovetski wrote: > >>>> On Thu, 18 Dec 2014, Ben Hutchings wrote: >>> Well, I thought that too. Will's submission from last week has that >>> change: >>> http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/87009 > >> Anyway, yes, that looks better! But I would still consider keeping buffers >> on the list in .buf_clean(), in which case you can remove it. And walk the >> list instead of the VB2 internal buffer array, as others have pointed out. > > Hi Guennadi, > Thanks for the clarification. Ian (when he was with us) did say "it > was particularly difficult to understand WTH this driver was doing". > > Regarding your first point, if it's safe to skip the actions left > in rcar_vin_videobuf_release() then I will do a further rework to > remove it completely. Yes, that's safe. Just remove it altogether. The buf_init and buf_release ops are matching ops that are normally only used if you have to do per-buffer initialization and/or release. These are only called when the buffer memory changes. In most drivers including this one it's not needed at all. The same is true for rcar_vin_videobuf_init: it's pointless since the list initialization is done implicitly when you add the buffer to a list with list_add_tail(). Just drop the function. Regards, Hans > > Regarding your second, in the patchset Ben linked to above we think > we have the appropriate loops: a for loop for queue_buf[], and > list_for_each_safe() for anything left in priv->capture; this is > consistent with rcar_vin_fill_hw_slot() setting up queue_buf[] with > pointers unlinked from priv->capture. This in turn suggests that we > are right not to call list_del_init() in both of > rcar_vin_stop_streaming()'s loops ... as long as I've correctly > interpreted the code and everyone's feedback thus far. > > > Cheers, > Wills. > -- > 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 > -- 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
On Mon, 19 Jan 2015, Guennadi Liakhovetski wrote: > On Mon, 19 Jan 2015, William Towle wrote: >> in the patchset Ben linked to above we think >> we have the appropriate loops: a for loop for queue_buf[], and >> list_for_each_safe() for anything left in priv->capture; this is >> consistent with rcar_vin_fill_hw_slot() setting up queue_buf[] with >> pointers unlinked from priv->capture. This in turn suggests that we >> are right not to call list_del_init() in both of >> rcar_vin_stop_streaming()'s loops ... as long as I've correctly >> interpreted the code and everyone's feedback thus far. > > I'm referring to this comment by Hans Verkuil of 14 August last year: > >> 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. > > I think, that's the right way to implement that clean up loop. Hi, I was describing the code in my latest patch; we start with rcar_vin_stop_streaming() having a list_for_each_safe() loop like that above, and leave that loop in place but add statements to it. I add another loop to rcar_vin_stop_streaming() [which you will have seen -in the patch Ben published in my absence over Christmas- was particularly inelegant initially], but it can't be rewritten in the same way. The new version is undeniably neater, though. We believe the latest patches take Hans' comment above into account properly, and we are looking into his latest suggestion at the moment. Thanks again, Wills. -- 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/soc_camera/rcar_vin.c b/drivers/media/platform/soc_camera/rcar_vin.c index 20dbedf..bf60074 100644 --- a/drivers/media/platform/soc_camera/rcar_vin.c +++ b/drivers/media/platform/soc_camera/rcar_vin.c @@ -486,28 +486,8 @@ static void rcar_vin_videobuf_release(struct vb2_buffer *vb) struct soc_camera_device *icd = soc_camera_from_vb2q(vb->vb2_queue); struct soc_camera_host *ici = to_soc_camera_host(icd->parent); struct rcar_vin_priv *priv = ici->priv; - unsigned int i; - int buf_in_use = 0; - spin_lock_irq(&priv->lock); - - /* Is the buffer in use by the VIN hardware? */ - for (i = 0; i < MAX_BUFFER_NUM; i++) { - if (priv->queue_buf[i] == vb) { - buf_in_use = 1; - break; - } - } - if (buf_in_use) { - rcar_vin_wait_stop_streaming(priv); - - /* - * Capturing has now stopped. The buffer we have been asked - * to release could be any of the current buffers in use, so - * release all buffers that are in use by HW - */ - priv->queue_buf[i] = NULL; - } + spin_lock_irq(&priv->lock); list_del_init(to_buf_list(vb)); @@ -533,8 +513,20 @@ static void rcar_vin_stop_streaming(struct vb2_queue *vq) rcar_vin_wait_stop_streaming(priv); for (i = 0; i < vq->num_buffers; ++i) - if (vq->bufs[i]->state == VB2_BUF_STATE_ACTIVE) + if (vq->bufs[i]->state == VB2_BUF_STATE_ACTIVE) { + int j; + + /* Is this a buffer we have told the + * hardware about? Update the associated + * list, if so + */ + for (j = 0; j < MAX_BUFFER_NUM; j++) { + if (priv->queue_buf[j] == vq->bufs[i]) { + priv->queue_buf[j] = NULL; + } + } vb2_buffer_done(vq->bufs[i], VB2_BUF_STATE_ERROR); + } list_for_each_safe(buf_head, tmp, &priv->capture) list_del_init(buf_head);