Hi Sakari,
Thank you for the patch.
On Tuesday 15 Sep 2015 10:51:10 Sakari Ailus wrote:
> Hans Verkuil wrote:
> > On 09/11/2015 01:50 PM, Sakari Ailus wrote:
> >> The cache synchronisation may be a time consuming operation and thus not
> >> best performed in an interrupt which is a typical context for
> >> vb2_buffer_done() calls. This may consume up to tens of ms on some
> >> machines, depending on the buffer size.
> >>
> >> Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> >> ---
> >>
> >> drivers/media/v4l2-core/videobuf2-core.c | 20 ++++++++++----------
> >> 1 file changed, 10 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/drivers/media/v4l2-core/videobuf2-core.c
> >> b/drivers/media/v4l2-core/videobuf2-core.c index 64fce4d..c5c0707a
> >> 100644
> >> --- a/drivers/media/v4l2-core/videobuf2-core.c
> >> +++ b/drivers/media/v4l2-core/videobuf2-core.c
> >> @@ -1177,7 +1177,6 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum
> >> vb2_buffer_state state)
> >> {
> >> struct vb2_queue *q = vb->vb2_queue;
> >> unsigned long flags;
> >> - unsigned int plane;
> >>
> >> if (WARN_ON(vb->state != VB2_BUF_STATE_ACTIVE))
> >> return;
> >> @@ -1197,10 +1196,6 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum
> >> vb2_buffer_state state)
> >> dprintk(4, "done processing on buffer %d, state: %d\n",
> >> vb->v4l2_buf.index, state);
> >>
> >> - /* sync buffers */
> >> - for (plane = 0; plane < vb->num_planes; ++plane)
> >> - call_void_memop(vb, finish, vb->planes[plane].mem_priv);
> >> -
> >
> > Ah, OK, so it is removed here,
>
> I can merge the two patches for the next version if you prefer that.
I think that would be a good idea. They're both pretty small, and they're
related. Merging them will make review easier.
> >> /* Add the buffer to the done buffers list */
> >> spin_lock_irqsave(&q->done_lock, flags);
> >> vb->state = state;
> >>
> >> @@ -2086,7 +2081,7 @@ EXPORT_SYMBOL_GPL(vb2_wait_for_all_buffers);
> >> static void __vb2_dqbuf(struct vb2_buffer *vb)
> >> {
> >> struct vb2_queue *q = vb->vb2_queue;
> >> - unsigned int i;
> >> + unsigned int plane;
> >>
> >> /* nothing to do if the buffer is already dequeued */
> >> if (vb->state == VB2_BUF_STATE_DEQUEUED)
> >> @@ -2094,13 +2089,18 @@ static void __vb2_dqbuf(struct vb2_buffer *vb)
> >>
> >> vb->state = VB2_BUF_STATE_DEQUEUED;
> >>
> >> + /* sync buffers */
> >> + for (plane = 0; plane < vb->num_planes; plane++)
> >> + call_void_memop(vb, finish, vb->planes[plane].mem_priv);
> >> +
> >
> > to here.
> >
> > I'm not sure if this is correct... So __vb2_dqbuf is called from
> > __vb2_queue_cancel(), but now the buf_finish() callback is called
> > *before* the memop finish() callback, where this was the other way around
> > in __vb2_queue_cancel(). I don't think that is right since buf_finish()
> > expects that the buffer is synced for the cpu.
>
> I don't mind reordering them.
The .buf_finish() driver callback could touch the contents of the buffer using
the CPU, so I agree with Hans that we need to reorder the calls.
> The vb->state will be different as __vb2_dqbuf() has already been called,
> there's at least one buffer state check in a driver. However, __vb2_dqbuf()
> unconditionally sets the buffer state to DEQUEUED, overriding e.g. ERROR
> which a driver would be interested in.
>
> I think the cache sync needs to be moved out of __vb2_dqbuf() to the
> same level where it's called so proper ordering can be maintained while
> still flushing cache before buf_finish() is called.
I think that would be the easiest option. It's a bit of a shame to duplicate
the call, but I don't see any other easy way. A comment that states that cache
sync needs to occur before .buf_finish() would probably be useful.
> > Was this tested with CONFIG_VIDEO_ADV_DEBUG set and with 'v4l2-compliance
> > -s'? Not that that would help if things are done in the wrong order...
>
> I'll do that the next time.
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
