Re: [RFCv2 PATCH 01/10] vb2: add debugging code to check for unbalanced ops.
On 02/13/14 09:01, Pawel Osciak wrote:
> Hi Hans,
> Thanks for the patch. I'm really happy to see this, it's a great idea
> and it will be very useful.
>
> Two comments:
> - What do you think about moving the debug stuff to something like
> videobuf2-debug.{c,h} instead?
It's not quite worth it at the moment IMHO.
> - At this point vb2_buffer_done() shouldn't be required to be balanced
> with buf_queue(). I know later patches in this series will require it,
> but at this point it's not true. Perhaps we should move this to the
> 8th patch or after it? I don't feel too strong about this though.
I disagree with you here. It has always been required, except nobody noticed
it. Without the call to vb2_buffer_done the finish memop will never be called.
That's always been wrong.
Regards,
Hans
>
> One more nit inline.
>
> But in general:
>
> Acked-by: Pawel Osciak
>
>
> On Thu, Feb 6, 2014 at 8:02 PM, Hans Verkuil wrote:
>> From: Hans Verkuil
>>
>> When a vb2_queue is freed check if all the mem_ops and queue ops were
>> balanced.
>> So the number of calls to e.g. buf_finish has to match the number of calls to
>> buf_prepare, etc.
>>
>> This code is only enabled if CONFIG_VIDEO_ADV_DEBUG is set.
>>
>> Signed-off-by: Hans Verkuil
>> ---
>> drivers/media/v4l2-core/videobuf2-core.c | 233
>> ---
>> include/media/videobuf2-core.h | 43 ++
>> 2 files changed, 226 insertions(+), 50 deletions(-)
>>
>> diff --git a/drivers/media/v4l2-core/videobuf2-core.c
>> b/drivers/media/v4l2-core/videobuf2-core.c
>> index 5a5fb7f..07b58bd 100644
>> --- a/drivers/media/v4l2-core/videobuf2-core.c
>> +++ b/drivers/media/v4l2-core/videobuf2-core.c
>> @@ -33,12 +33,63 @@ module_param(debug, int, 0644);
>> printk(KERN_DEBUG "vb2: " fmt, ## arg); \
>> } while (0)
>>
>> -#define call_memop(q, op, args...) \
>> - (((q)->mem_ops->op) ? \
>> - ((q)->mem_ops->op(args)) : 0)
>> +#ifdef CONFIG_VIDEO_ADV_DEBUG
>> +
>> +/*
>> + * If advanced debugging is on, then count how often each op is called,
>> + * which can either be per-buffer or per-queue.
>> + *
>> + * If the op failed then the 'fail_' variant is called to decrease the
>> + * counter. That makes it easy to check that the 'init' and 'cleanup'
>> + * (and variations thereof) stay balanced.
>> + */
>> +
>> +#define call_memop(vb, op, args...)\
>> +({ \
>> + struct vb2_queue *_q = (vb)->vb2_queue; \
>> + dprintk(2, "call_memop(%p, %d, %s)%s\n",\
>> + _q, (vb)->v4l2_buf.index, #op, \
>> + _q->mem_ops->op ? "" : " (nop)"); \
>> + (vb)->cnt_mem_ ## op++; \
>> + _q->mem_ops->op ? _q->mem_ops->op(args) : 0;\
>> +})
>> +#define fail_memop(vb, op) ((vb)->cnt_mem_ ## op--)
>> +
>> +#define call_qop(q, op, args...) \
>> +({ \
>> + dprintk(2, "call_qop(%p, %s)%s\n", q, #op, \
>> + (q)->ops->op ? "" : " (nop)"); \
>> + (q)->cnt_ ## op++; \
>> + (q)->ops->op ? (q)->ops->op(args) : 0; \
>> +})
>> +#define fail_qop(q, op) ((q)->cnt_ ## op--)
>> +
>> +#define call_vb_qop(vb, op, args...) \
>> +({ \
>> + struct vb2_queue *_q = (vb)->vb2_queue; \
>> + dprintk(2, "call_vb_qop(%p, %d, %s)%s\n", \
>> + _q, (vb)->v4l2_buf.index, #op, \
>> + _q->ops->op ? "" : " (nop)"); \
>> + (vb)->cnt_ ## op++; \
>> + _q->ops->op ? _q->ops->op(args) : 0;\
>> +})
>> +#define fail_vb_qop(vb, op) ((vb)->cnt_ ## op--)
>> +
>> +#else
>> +
>> +#define call_memop(vb, op, args...)\
>> + ((vb)->vb2_queue->mem_ops->op ? (vb)->vb2_queue->mem_ops->op(args) :
>> 0)
>> +#define fail_memop(vb, op)
>>
>> #define call_qop(q, op, args...) \
>> - (((q)->ops->op) ? ((q)->ops->op(args)) : 0)
>> + ((q)->ops->op ? (q)->ops->op(args) : 0)
>> +#define fail_qop(q, op)
>> +
>> +#define call_vb_qop(vb, op, args...) \
>> + ((vb)->vb2_queue->ops->op ? (vb)->vb2_queue->ops->op(args) : 0)
>> +#define fail_vb_qop(vb, op)
>> +
>> +#endif
>>
>> #define V4L2
Re: [RFCv2 PATCH 01/10] vb2: add debugging code to check for unbalanced ops.
Hi Hans,
Thanks for the patch. I'm really happy to see this, it's a great idea
and it will be very useful.
Two comments:
- What do you think about moving the debug stuff to something like
videobuf2-debug.{c,h} instead?
- At this point vb2_buffer_done() shouldn't be required to be balanced
with buf_queue(). I know later patches in this series will require it,
but at this point it's not true. Perhaps we should move this to the
8th patch or after it? I don't feel too strong about this though.
One more nit inline.
But in general:
Acked-by: Pawel Osciak
On Thu, Feb 6, 2014 at 8:02 PM, Hans Verkuil wrote:
> From: Hans Verkuil
>
> When a vb2_queue is freed check if all the mem_ops and queue ops were
> balanced.
> So the number of calls to e.g. buf_finish has to match the number of calls to
> buf_prepare, etc.
>
> This code is only enabled if CONFIG_VIDEO_ADV_DEBUG is set.
>
> Signed-off-by: Hans Verkuil
> ---
> drivers/media/v4l2-core/videobuf2-core.c | 233
> ---
> include/media/videobuf2-core.h | 43 ++
> 2 files changed, 226 insertions(+), 50 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/videobuf2-core.c
> b/drivers/media/v4l2-core/videobuf2-core.c
> index 5a5fb7f..07b58bd 100644
> --- a/drivers/media/v4l2-core/videobuf2-core.c
> +++ b/drivers/media/v4l2-core/videobuf2-core.c
> @@ -33,12 +33,63 @@ module_param(debug, int, 0644);
> printk(KERN_DEBUG "vb2: " fmt, ## arg); \
> } while (0)
>
> -#define call_memop(q, op, args...) \
> - (((q)->mem_ops->op) ? \
> - ((q)->mem_ops->op(args)) : 0)
> +#ifdef CONFIG_VIDEO_ADV_DEBUG
> +
> +/*
> + * If advanced debugging is on, then count how often each op is called,
> + * which can either be per-buffer or per-queue.
> + *
> + * If the op failed then the 'fail_' variant is called to decrease the
> + * counter. That makes it easy to check that the 'init' and 'cleanup'
> + * (and variations thereof) stay balanced.
> + */
> +
> +#define call_memop(vb, op, args...)\
> +({ \
> + struct vb2_queue *_q = (vb)->vb2_queue; \
> + dprintk(2, "call_memop(%p, %d, %s)%s\n",\
> + _q, (vb)->v4l2_buf.index, #op, \
> + _q->mem_ops->op ? "" : " (nop)"); \
> + (vb)->cnt_mem_ ## op++; \
> + _q->mem_ops->op ? _q->mem_ops->op(args) : 0;\
> +})
> +#define fail_memop(vb, op) ((vb)->cnt_mem_ ## op--)
> +
> +#define call_qop(q, op, args...) \
> +({ \
> + dprintk(2, "call_qop(%p, %s)%s\n", q, #op, \
> + (q)->ops->op ? "" : " (nop)"); \
> + (q)->cnt_ ## op++; \
> + (q)->ops->op ? (q)->ops->op(args) : 0; \
> +})
> +#define fail_qop(q, op) ((q)->cnt_ ## op--)
> +
> +#define call_vb_qop(vb, op, args...) \
> +({ \
> + struct vb2_queue *_q = (vb)->vb2_queue; \
> + dprintk(2, "call_vb_qop(%p, %d, %s)%s\n", \
> + _q, (vb)->v4l2_buf.index, #op, \
> + _q->ops->op ? "" : " (nop)"); \
> + (vb)->cnt_ ## op++; \
> + _q->ops->op ? _q->ops->op(args) : 0;\
> +})
> +#define fail_vb_qop(vb, op) ((vb)->cnt_ ## op--)
> +
> +#else
> +
> +#define call_memop(vb, op, args...)\
> + ((vb)->vb2_queue->mem_ops->op ? (vb)->vb2_queue->mem_ops->op(args) :
> 0)
> +#define fail_memop(vb, op)
>
> #define call_qop(q, op, args...) \
> - (((q)->ops->op) ? ((q)->ops->op(args)) : 0)
> + ((q)->ops->op ? (q)->ops->op(args) : 0)
> +#define fail_qop(q, op)
> +
> +#define call_vb_qop(vb, op, args...) \
> + ((vb)->vb2_queue->ops->op ? (vb)->vb2_queue->ops->op(args) : 0)
> +#define fail_vb_qop(vb, op)
> +
> +#endif
>
> #define V4L2_BUFFER_MASK_FLAGS (V4L2_BUF_FLAG_MAPPED | V4L2_BUF_FLAG_QUEUED
> | \
> V4L2_BUF_FLAG_DONE | V4L2_BUF_FLAG_ERROR | \
> @@ -61,7 +112,7 @@ static int __vb2_buf_mem_alloc(struct vb2_buffer *vb)
> for (plane = 0; plane < vb->num_planes; ++plane) {
> unsigned long size = PAGE_ALIGN(q->plane_sizes[plane]);
>
> - mem_priv = call_memop(q, alloc, q->alloc_c
[RFCv2 PATCH 01/10] vb2: add debugging code to check for unbalanced ops.
From: Hans Verkuil
When a vb2_queue is freed check if all the mem_ops and queue ops were balanced.
So the number of calls to e.g. buf_finish has to match the number of calls to
buf_prepare, etc.
This code is only enabled if CONFIG_VIDEO_ADV_DEBUG is set.
Signed-off-by: Hans Verkuil
---
drivers/media/v4l2-core/videobuf2-core.c | 233 ---
include/media/videobuf2-core.h | 43 ++
2 files changed, 226 insertions(+), 50 deletions(-)
diff --git a/drivers/media/v4l2-core/videobuf2-core.c
b/drivers/media/v4l2-core/videobuf2-core.c
index 5a5fb7f..07b58bd 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -33,12 +33,63 @@ module_param(debug, int, 0644);
printk(KERN_DEBUG "vb2: " fmt, ## arg); \
} while (0)
-#define call_memop(q, op, args...) \
- (((q)->mem_ops->op) ? \
- ((q)->mem_ops->op(args)) : 0)
+#ifdef CONFIG_VIDEO_ADV_DEBUG
+
+/*
+ * If advanced debugging is on, then count how often each op is called,
+ * which can either be per-buffer or per-queue.
+ *
+ * If the op failed then the 'fail_' variant is called to decrease the
+ * counter. That makes it easy to check that the 'init' and 'cleanup'
+ * (and variations thereof) stay balanced.
+ */
+
+#define call_memop(vb, op, args...)\
+({ \
+ struct vb2_queue *_q = (vb)->vb2_queue; \
+ dprintk(2, "call_memop(%p, %d, %s)%s\n",\
+ _q, (vb)->v4l2_buf.index, #op, \
+ _q->mem_ops->op ? "" : " (nop)"); \
+ (vb)->cnt_mem_ ## op++; \
+ _q->mem_ops->op ? _q->mem_ops->op(args) : 0;\
+})
+#define fail_memop(vb, op) ((vb)->cnt_mem_ ## op--)
+
+#define call_qop(q, op, args...) \
+({ \
+ dprintk(2, "call_qop(%p, %s)%s\n", q, #op, \
+ (q)->ops->op ? "" : " (nop)"); \
+ (q)->cnt_ ## op++; \
+ (q)->ops->op ? (q)->ops->op(args) : 0; \
+})
+#define fail_qop(q, op) ((q)->cnt_ ## op--)
+
+#define call_vb_qop(vb, op, args...) \
+({ \
+ struct vb2_queue *_q = (vb)->vb2_queue; \
+ dprintk(2, "call_vb_qop(%p, %d, %s)%s\n", \
+ _q, (vb)->v4l2_buf.index, #op, \
+ _q->ops->op ? "" : " (nop)"); \
+ (vb)->cnt_ ## op++; \
+ _q->ops->op ? _q->ops->op(args) : 0;\
+})
+#define fail_vb_qop(vb, op) ((vb)->cnt_ ## op--)
+
+#else
+
+#define call_memop(vb, op, args...)\
+ ((vb)->vb2_queue->mem_ops->op ? (vb)->vb2_queue->mem_ops->op(args) : 0)
+#define fail_memop(vb, op)
#define call_qop(q, op, args...) \
- (((q)->ops->op) ? ((q)->ops->op(args)) : 0)
+ ((q)->ops->op ? (q)->ops->op(args) : 0)
+#define fail_qop(q, op)
+
+#define call_vb_qop(vb, op, args...) \
+ ((vb)->vb2_queue->ops->op ? (vb)->vb2_queue->ops->op(args) : 0)
+#define fail_vb_qop(vb, op)
+
+#endif
#define V4L2_BUFFER_MASK_FLAGS (V4L2_BUF_FLAG_MAPPED | V4L2_BUF_FLAG_QUEUED | \
V4L2_BUF_FLAG_DONE | V4L2_BUF_FLAG_ERROR | \
@@ -61,7 +112,7 @@ static int __vb2_buf_mem_alloc(struct vb2_buffer *vb)
for (plane = 0; plane < vb->num_planes; ++plane) {
unsigned long size = PAGE_ALIGN(q->plane_sizes[plane]);
- mem_priv = call_memop(q, alloc, q->alloc_ctx[plane],
+ mem_priv = call_memop(vb, alloc, q->alloc_ctx[plane],
size, q->gfp_flags);
if (IS_ERR_OR_NULL(mem_priv))
goto free;
@@ -73,9 +124,10 @@ static int __vb2_buf_mem_alloc(struct vb2_buffer *vb)
return 0;
free:
+ fail_memop(vb, alloc);
/* Free already allocated memory if one of the allocations failed */
for (; plane > 0; --plane) {
- call_memop(q, put, vb->planes[plane - 1].mem_priv);
+ call_memop(vb, put, vb->planes[plane - 1].mem_priv);
vb->planes[plane - 1].mem_priv = NULL;
}
@@ -87,11 +139,10 @@ free:
*/
static void __vb2_buf_mem_free(struct vb2_buffer *vb)
{
- struct vb2_queue *q = vb->vb2_
