* [PATCH] media: videobuf2: Print videobuf2 buffer state by name @ 2020-07-09 17:43 Ezequiel Garcia 2020-07-10 10:10 ` Hans Verkuil 2020-07-10 12:51 ` [PATCH v2] " Ezequiel Garcia 0 siblings, 2 replies; 8+ messages in thread From: Ezequiel Garcia @ 2020-07-09 17:43 UTC (permalink / raw) To: linux-media; +Cc: Hans Verkuil, Ezequiel Garcia, kernel, Nicolas Dufresne For debugging purposes, seeing the state integer representation is really inconvenient. Improve this and be developer-friendly by printing the state name instead. Suggested-by: Nicolas Dufresne <nicolas.dufresne@collabora.com> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> --- .../media/common/videobuf2/videobuf2-core.c | 32 ++++++++++++++----- 1 file changed, 24 insertions(+), 8 deletions(-) diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c index abaf28e057eb..8480772d58c6 100644 --- a/drivers/media/common/videobuf2/videobuf2-core.c +++ b/drivers/media/common/videobuf2/videobuf2-core.c @@ -191,6 +191,20 @@ module_param(debug, int, 0644); static void __vb2_queue_cancel(struct vb2_queue *q); static void __enqueue_in_driver(struct vb2_buffer *vb); +static const char * const vb2_state_names[] = { + [VB2_BUF_STATE_DEQUEUED] = "dequeued", + [VB2_BUF_STATE_IN_REQUEST] = "in request", + [VB2_BUF_STATE_PREPARING] = "preparing", + [VB2_BUF_STATE_QUEUED] = "queued", + [VB2_BUF_STATE_ACTIVE] = "active", + [VB2_BUF_STATE_DONE] = "done", + [VB2_BUF_STATE_ERROR] = "error", +}; + +#define vb2_state_name(s) \ + (((unsigned int)(s)) < ARRAY_SIZE(vb2_state_names) ? \ + vb2_state_names[s] : "unknown") + /* * __vb2_buf_mem_alloc() - allocate video memory for the given buffer */ @@ -1015,8 +1029,8 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum vb2_buffer_state state) */ vb->cnt_buf_done++; #endif - dprintk(q, 4, "done processing on buffer %d, state: %d\n", - vb->index, state); + dprintk(q, 4, "done processing on buffer %d, state: %s\n", + vb->index, vb2_state_name(state)); if (state != VB2_BUF_STATE_QUEUED) __vb2_buf_mem_finish(vb); @@ -1490,8 +1504,8 @@ int vb2_core_prepare_buf(struct vb2_queue *q, unsigned int index, void *pb) vb = q->bufs[index]; if (vb->state != VB2_BUF_STATE_DEQUEUED) { - dprintk(q, 1, "invalid buffer state %d\n", - vb->state); + dprintk(q, 1, "invalid buffer state %s\n", + vb2_state_name(vb->state)); return -EINVAL; } if (vb->prepared) { @@ -1670,7 +1684,8 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb, dprintk(q, 1, "buffer still being prepared\n"); return -EINVAL; default: - dprintk(q, 1, "invalid buffer state %d\n", vb->state); + dprintk(q, 1, "invalid buffer state %s\n", + vb2_state_name(vb->state)); return -EINVAL; } @@ -1884,7 +1899,8 @@ int vb2_core_dqbuf(struct vb2_queue *q, unsigned int *pindex, void *pb, dprintk(q, 3, "returning done buffer with errors\n"); break; default: - dprintk(q, 1, "invalid buffer state\n"); + dprintk(q, 1, "invalid buffer state %s\n", + vb2_state_name(vb->state)); return -EINVAL; } @@ -1915,8 +1931,8 @@ int vb2_core_dqbuf(struct vb2_queue *q, unsigned int *pindex, void *pb, media_request_put(vb->request); vb->request = NULL; - dprintk(q, 2, "dqbuf of buffer %d, with state %d\n", - vb->index, vb->state); + dprintk(q, 2, "dqbuf of buffer %d, state: %s\n", + vb->index, vb2_state_name(vb->state)); return 0; -- 2.27.0 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] media: videobuf2: Print videobuf2 buffer state by name 2020-07-09 17:43 [PATCH] media: videobuf2: Print videobuf2 buffer state by name Ezequiel Garcia @ 2020-07-10 10:10 ` Hans Verkuil 2020-07-10 12:51 ` [PATCH v2] " Ezequiel Garcia 1 sibling, 0 replies; 8+ messages in thread From: Hans Verkuil @ 2020-07-10 10:10 UTC (permalink / raw) To: Ezequiel Garcia, linux-media; +Cc: kernel, Nicolas Dufresne On 09/07/2020 19:43, Ezequiel Garcia wrote: > For debugging purposes, seeing the state integer > representation is really inconvenient. > > Improve this and be developer-friendly by printing > the state name instead. > > Suggested-by: Nicolas Dufresne <nicolas.dufresne@collabora.com> > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> > --- > .../media/common/videobuf2/videobuf2-core.c | 32 ++++++++++++++----- > 1 file changed, 24 insertions(+), 8 deletions(-) > > diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c > index abaf28e057eb..8480772d58c6 100644 > --- a/drivers/media/common/videobuf2/videobuf2-core.c > +++ b/drivers/media/common/videobuf2/videobuf2-core.c > @@ -191,6 +191,20 @@ module_param(debug, int, 0644); > static void __vb2_queue_cancel(struct vb2_queue *q); > static void __enqueue_in_driver(struct vb2_buffer *vb); > > +static const char * const vb2_state_names[] = { > + [VB2_BUF_STATE_DEQUEUED] = "dequeued", > + [VB2_BUF_STATE_IN_REQUEST] = "in request", > + [VB2_BUF_STATE_PREPARING] = "preparing", > + [VB2_BUF_STATE_QUEUED] = "queued", > + [VB2_BUF_STATE_ACTIVE] = "active", > + [VB2_BUF_STATE_DONE] = "done", > + [VB2_BUF_STATE_ERROR] = "error", > +}; > + > +#define vb2_state_name(s) \ > + (((unsigned int)(s)) < ARRAY_SIZE(vb2_state_names) ? \ > + vb2_state_names[s] : "unknown") Can you turn this into a function? That avoids this checkpatch warning: CHECK: Macro argument reuse 's' - possible side-effects? #37: FILE: drivers/media/common/videobuf2/videobuf2-core.c:204: +#define vb2_state_name(s) \ + (((unsigned int)(s)) < ARRAY_SIZE(vb2_state_names) ? \ + vb2_state_names[s] : "unknown") And I think a function is cleaner as well. This looks good otherwise. Regards, Hans > + > /* > * __vb2_buf_mem_alloc() - allocate video memory for the given buffer > */ > @@ -1015,8 +1029,8 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum vb2_buffer_state state) > */ > vb->cnt_buf_done++; > #endif > - dprintk(q, 4, "done processing on buffer %d, state: %d\n", > - vb->index, state); > + dprintk(q, 4, "done processing on buffer %d, state: %s\n", > + vb->index, vb2_state_name(state)); > > if (state != VB2_BUF_STATE_QUEUED) > __vb2_buf_mem_finish(vb); > @@ -1490,8 +1504,8 @@ int vb2_core_prepare_buf(struct vb2_queue *q, unsigned int index, void *pb) > > vb = q->bufs[index]; > if (vb->state != VB2_BUF_STATE_DEQUEUED) { > - dprintk(q, 1, "invalid buffer state %d\n", > - vb->state); > + dprintk(q, 1, "invalid buffer state %s\n", > + vb2_state_name(vb->state)); > return -EINVAL; > } > if (vb->prepared) { > @@ -1670,7 +1684,8 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb, > dprintk(q, 1, "buffer still being prepared\n"); > return -EINVAL; > default: > - dprintk(q, 1, "invalid buffer state %d\n", vb->state); > + dprintk(q, 1, "invalid buffer state %s\n", > + vb2_state_name(vb->state)); > return -EINVAL; > } > > @@ -1884,7 +1899,8 @@ int vb2_core_dqbuf(struct vb2_queue *q, unsigned int *pindex, void *pb, > dprintk(q, 3, "returning done buffer with errors\n"); > break; > default: > - dprintk(q, 1, "invalid buffer state\n"); > + dprintk(q, 1, "invalid buffer state %s\n", > + vb2_state_name(vb->state)); > return -EINVAL; > } > > @@ -1915,8 +1931,8 @@ int vb2_core_dqbuf(struct vb2_queue *q, unsigned int *pindex, void *pb, > media_request_put(vb->request); > vb->request = NULL; > > - dprintk(q, 2, "dqbuf of buffer %d, with state %d\n", > - vb->index, vb->state); > + dprintk(q, 2, "dqbuf of buffer %d, state: %s\n", > + vb->index, vb2_state_name(vb->state)); > > return 0; > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2] media: videobuf2: Print videobuf2 buffer state by name 2020-07-09 17:43 [PATCH] media: videobuf2: Print videobuf2 buffer state by name Ezequiel Garcia 2020-07-10 10:10 ` Hans Verkuil @ 2020-07-10 12:51 ` Ezequiel Garcia 2020-07-10 13:00 ` [PATCH v3] " Ezequiel Garcia 1 sibling, 1 reply; 8+ messages in thread From: Ezequiel Garcia @ 2020-07-10 12:51 UTC (permalink / raw) To: linux-media; +Cc: Hans Verkuil, Ezequiel Garcia, kernel, Nicolas Dufresne For debugging purposes, seeing the state integer representation is really inconvenient. Improve this and be developer-friendly by printing the state name instead. Suggested-by: Nicolas Dufresne <nicolas.dufresne@collabora.com> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> --- v2: * Use a proper function instead of a C macro. .../media/common/videobuf2/videobuf2-core.c | 35 ++++++++++++++----- 1 file changed, 27 insertions(+), 8 deletions(-) diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c index abaf28e057eb..a1520c1833d0 100644 --- a/drivers/media/common/videobuf2/videobuf2-core.c +++ b/drivers/media/common/videobuf2/videobuf2-core.c @@ -191,6 +191,23 @@ module_param(debug, int, 0644); static void __vb2_queue_cancel(struct vb2_queue *q); static void __enqueue_in_driver(struct vb2_buffer *vb); +static inline const char *vb2_state_name(enum vb2_buffer_state s) +{ + static const char * const state_names[] = { + [VB2_BUF_STATE_DEQUEUED] = "dequeued", + [VB2_BUF_STATE_IN_REQUEST] = "in request", + [VB2_BUF_STATE_PREPARING] = "preparing", + [VB2_BUF_STATE_QUEUED] = "queued", + [VB2_BUF_STATE_ACTIVE] = "active", + [VB2_BUF_STATE_DONE] = "done", + [VB2_BUF_STATE_ERROR] = "error", + }; + + if ((unsigned int)(s) < ARRAY_SIZE(state_names)) + return state_names[s]; + return "unknown"; +} + /* * __vb2_buf_mem_alloc() - allocate video memory for the given buffer */ @@ -1015,8 +1032,8 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum vb2_buffer_state state) */ vb->cnt_buf_done++; #endif - dprintk(q, 4, "done processing on buffer %d, state: %d\n", - vb->index, state); + dprintk(q, 4, "done processing on buffer %d, state: %s\n", + vb->index, vb2_state_name(state)); if (state != VB2_BUF_STATE_QUEUED) __vb2_buf_mem_finish(vb); @@ -1490,8 +1507,8 @@ int vb2_core_prepare_buf(struct vb2_queue *q, unsigned int index, void *pb) vb = q->bufs[index]; if (vb->state != VB2_BUF_STATE_DEQUEUED) { - dprintk(q, 1, "invalid buffer state %d\n", - vb->state); + dprintk(q, 1, "invalid buffer state %s\n", + vb2_state_name(vb->state)); return -EINVAL; } if (vb->prepared) { @@ -1670,7 +1687,8 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb, dprintk(q, 1, "buffer still being prepared\n"); return -EINVAL; default: - dprintk(q, 1, "invalid buffer state %d\n", vb->state); + dprintk(q, 1, "invalid buffer state %s\n", + vb2_state_name(vb->state)); return -EINVAL; } @@ -1884,7 +1902,8 @@ int vb2_core_dqbuf(struct vb2_queue *q, unsigned int *pindex, void *pb, dprintk(q, 3, "returning done buffer with errors\n"); break; default: - dprintk(q, 1, "invalid buffer state\n"); + dprintk(q, 1, "invalid buffer state %s\n", + vb2_state_name(vb->state)); return -EINVAL; } @@ -1915,8 +1934,8 @@ int vb2_core_dqbuf(struct vb2_queue *q, unsigned int *pindex, void *pb, media_request_put(vb->request); vb->request = NULL; - dprintk(q, 2, "dqbuf of buffer %d, with state %d\n", - vb->index, vb->state); + dprintk(q, 2, "dqbuf of buffer %d, state: %s\n", + vb->index, vb2_state_name(vb->state)); return 0; -- 2.27.0 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH v3] media: videobuf2: Print videobuf2 buffer state by name 2020-07-10 12:51 ` [PATCH v2] " Ezequiel Garcia @ 2020-07-10 13:00 ` Ezequiel Garcia 2020-07-13 9:31 ` Hans Verkuil 2020-07-17 3:39 ` [PATCH v4] " Ezequiel Garcia 0 siblings, 2 replies; 8+ messages in thread From: Ezequiel Garcia @ 2020-07-10 13:00 UTC (permalink / raw) To: linux-media; +Cc: Hans Verkuil, Ezequiel Garcia, kernel, Nicolas Dufresne For debugging purposes, seeing the state integer representation is really inconvenient. Improve this and be developer-friendly by printing the state name instead. Suggested-by: Nicolas Dufresne <nicolas.dufresne@collabora.com> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> --- v3: * Drop static modifier from the now local name array. v2: * Use a proper function instead of a C macro. .../media/common/videobuf2/videobuf2-core.c | 35 ++++++++++++++----- 1 file changed, 27 insertions(+), 8 deletions(-) diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c index abaf28e057eb..d5942cd455bf 100644 --- a/drivers/media/common/videobuf2/videobuf2-core.c +++ b/drivers/media/common/videobuf2/videobuf2-core.c @@ -191,6 +191,23 @@ module_param(debug, int, 0644); static void __vb2_queue_cancel(struct vb2_queue *q); static void __enqueue_in_driver(struct vb2_buffer *vb); +static inline const char *vb2_state_name(enum vb2_buffer_state s) +{ + const char * const state_names[] = { + [VB2_BUF_STATE_DEQUEUED] = "dequeued", + [VB2_BUF_STATE_IN_REQUEST] = "in request", + [VB2_BUF_STATE_PREPARING] = "preparing", + [VB2_BUF_STATE_QUEUED] = "queued", + [VB2_BUF_STATE_ACTIVE] = "active", + [VB2_BUF_STATE_DONE] = "done", + [VB2_BUF_STATE_ERROR] = "error", + }; + + if ((unsigned int)(s) < ARRAY_SIZE(state_names)) + return state_names[s]; + return "unknown"; +} + /* * __vb2_buf_mem_alloc() - allocate video memory for the given buffer */ @@ -1015,8 +1032,8 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum vb2_buffer_state state) */ vb->cnt_buf_done++; #endif - dprintk(q, 4, "done processing on buffer %d, state: %d\n", - vb->index, state); + dprintk(q, 4, "done processing on buffer %d, state: %s\n", + vb->index, vb2_state_name(state)); if (state != VB2_BUF_STATE_QUEUED) __vb2_buf_mem_finish(vb); @@ -1490,8 +1507,8 @@ int vb2_core_prepare_buf(struct vb2_queue *q, unsigned int index, void *pb) vb = q->bufs[index]; if (vb->state != VB2_BUF_STATE_DEQUEUED) { - dprintk(q, 1, "invalid buffer state %d\n", - vb->state); + dprintk(q, 1, "invalid buffer state %s\n", + vb2_state_name(vb->state)); return -EINVAL; } if (vb->prepared) { @@ -1670,7 +1687,8 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb, dprintk(q, 1, "buffer still being prepared\n"); return -EINVAL; default: - dprintk(q, 1, "invalid buffer state %d\n", vb->state); + dprintk(q, 1, "invalid buffer state %s\n", + vb2_state_name(vb->state)); return -EINVAL; } @@ -1884,7 +1902,8 @@ int vb2_core_dqbuf(struct vb2_queue *q, unsigned int *pindex, void *pb, dprintk(q, 3, "returning done buffer with errors\n"); break; default: - dprintk(q, 1, "invalid buffer state\n"); + dprintk(q, 1, "invalid buffer state %s\n", + vb2_state_name(vb->state)); return -EINVAL; } @@ -1915,8 +1934,8 @@ int vb2_core_dqbuf(struct vb2_queue *q, unsigned int *pindex, void *pb, media_request_put(vb->request); vb->request = NULL; - dprintk(q, 2, "dqbuf of buffer %d, with state %d\n", - vb->index, vb->state); + dprintk(q, 2, "dqbuf of buffer %d, state: %s\n", + vb->index, vb2_state_name(vb->state)); return 0; -- 2.27.0 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v3] media: videobuf2: Print videobuf2 buffer state by name 2020-07-10 13:00 ` [PATCH v3] " Ezequiel Garcia @ 2020-07-13 9:31 ` Hans Verkuil 2020-07-13 13:10 ` Ezequiel Garcia 2020-07-17 3:39 ` [PATCH v4] " Ezequiel Garcia 1 sibling, 1 reply; 8+ messages in thread From: Hans Verkuil @ 2020-07-13 9:31 UTC (permalink / raw) To: Ezequiel Garcia, linux-media; +Cc: kernel, Nicolas Dufresne On 10/07/2020 15:00, Ezequiel Garcia wrote: > For debugging purposes, seeing the state integer > representation is really inconvenient. > > Improve this and be developer-friendly by printing > the state name instead. > > Suggested-by: Nicolas Dufresne <nicolas.dufresne@collabora.com> > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> > --- > v3: > * Drop static modifier from the now local name array. > v2: > * Use a proper function instead of a C macro. > > .../media/common/videobuf2/videobuf2-core.c | 35 ++++++++++++++----- > 1 file changed, 27 insertions(+), 8 deletions(-) > > diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c > index abaf28e057eb..d5942cd455bf 100644 > --- a/drivers/media/common/videobuf2/videobuf2-core.c > +++ b/drivers/media/common/videobuf2/videobuf2-core.c > @@ -191,6 +191,23 @@ module_param(debug, int, 0644); > static void __vb2_queue_cancel(struct vb2_queue *q); > static void __enqueue_in_driver(struct vb2_buffer *vb); > > +static inline const char *vb2_state_name(enum vb2_buffer_state s) Why inline? That adds nothing of value. It's too big for an inline anyway and it's only used when debug is on. > +{ > + const char * const state_names[] = { This really should be static, local or not. I'm not sure why you dropped that. Possibly because of the inline? Regards, Hans > + [VB2_BUF_STATE_DEQUEUED] = "dequeued", > + [VB2_BUF_STATE_IN_REQUEST] = "in request", > + [VB2_BUF_STATE_PREPARING] = "preparing", > + [VB2_BUF_STATE_QUEUED] = "queued", > + [VB2_BUF_STATE_ACTIVE] = "active", > + [VB2_BUF_STATE_DONE] = "done", > + [VB2_BUF_STATE_ERROR] = "error", > + }; > + > + if ((unsigned int)(s) < ARRAY_SIZE(state_names)) > + return state_names[s]; > + return "unknown"; > +} > + > /* > * __vb2_buf_mem_alloc() - allocate video memory for the given buffer > */ > @@ -1015,8 +1032,8 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum vb2_buffer_state state) > */ > vb->cnt_buf_done++; > #endif > - dprintk(q, 4, "done processing on buffer %d, state: %d\n", > - vb->index, state); > + dprintk(q, 4, "done processing on buffer %d, state: %s\n", > + vb->index, vb2_state_name(state)); > > if (state != VB2_BUF_STATE_QUEUED) > __vb2_buf_mem_finish(vb); > @@ -1490,8 +1507,8 @@ int vb2_core_prepare_buf(struct vb2_queue *q, unsigned int index, void *pb) > > vb = q->bufs[index]; > if (vb->state != VB2_BUF_STATE_DEQUEUED) { > - dprintk(q, 1, "invalid buffer state %d\n", > - vb->state); > + dprintk(q, 1, "invalid buffer state %s\n", > + vb2_state_name(vb->state)); > return -EINVAL; > } > if (vb->prepared) { > @@ -1670,7 +1687,8 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb, > dprintk(q, 1, "buffer still being prepared\n"); > return -EINVAL; > default: > - dprintk(q, 1, "invalid buffer state %d\n", vb->state); > + dprintk(q, 1, "invalid buffer state %s\n", > + vb2_state_name(vb->state)); > return -EINVAL; > } > > @@ -1884,7 +1902,8 @@ int vb2_core_dqbuf(struct vb2_queue *q, unsigned int *pindex, void *pb, > dprintk(q, 3, "returning done buffer with errors\n"); > break; > default: > - dprintk(q, 1, "invalid buffer state\n"); > + dprintk(q, 1, "invalid buffer state %s\n", > + vb2_state_name(vb->state)); > return -EINVAL; > } > > @@ -1915,8 +1934,8 @@ int vb2_core_dqbuf(struct vb2_queue *q, unsigned int *pindex, void *pb, > media_request_put(vb->request); > vb->request = NULL; > > - dprintk(q, 2, "dqbuf of buffer %d, with state %d\n", > - vb->index, vb->state); > + dprintk(q, 2, "dqbuf of buffer %d, state: %s\n", > + vb->index, vb2_state_name(vb->state)); > > return 0; > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3] media: videobuf2: Print videobuf2 buffer state by name 2020-07-13 9:31 ` Hans Verkuil @ 2020-07-13 13:10 ` Ezequiel Garcia 2020-07-13 13:45 ` Hans Verkuil 0 siblings, 1 reply; 8+ messages in thread From: Ezequiel Garcia @ 2020-07-13 13:10 UTC (permalink / raw) To: Hans Verkuil, linux-media; +Cc: kernel, Nicolas Dufresne On Mon, 2020-07-13 at 11:31 +0200, Hans Verkuil wrote: > On 10/07/2020 15:00, Ezequiel Garcia wrote: > > For debugging purposes, seeing the state integer > > representation is really inconvenient. > > > > Improve this and be developer-friendly by printing > > the state name instead. > > > > Suggested-by: Nicolas Dufresne <nicolas.dufresne@collabora.com> > > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> > > --- > > v3: > > * Drop static modifier from the now local name array. > > v2: > > * Use a proper function instead of a C macro. > > > > .../media/common/videobuf2/videobuf2-core.c | 35 ++++++++++++++----- > > 1 file changed, 27 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c > > index abaf28e057eb..d5942cd455bf 100644 > > --- a/drivers/media/common/videobuf2/videobuf2-core.c > > +++ b/drivers/media/common/videobuf2/videobuf2-core.c > > @@ -191,6 +191,23 @@ module_param(debug, int, 0644); > > static void __vb2_queue_cancel(struct vb2_queue *q); > > static void __enqueue_in_driver(struct vb2_buffer *vb); > > > > +static inline const char *vb2_state_name(enum vb2_buffer_state s) > > Why inline? That adds nothing of value. It's too big for an inline > anyway and it's only used when debug is on. > Yes, you're right. > > +{ > > + const char * const state_names[] = { > > This really should be static, local or not. I'm not sure why you dropped > that. Possibly because of the inline? > I was hesitant about this, as you can see. After some thought, it seemed a waste of space to have this array allocated statically. Could you clarify this for me? Why do you believe it should be static? Thanks, Ezequiel > Regards, > > Hans > > > + [VB2_BUF_STATE_DEQUEUED] = "dequeued", > > + [VB2_BUF_STATE_IN_REQUEST] = "in request", > > + [VB2_BUF_STATE_PREPARING] = "preparing", > > + [VB2_BUF_STATE_QUEUED] = "queued", > > + [VB2_BUF_STATE_ACTIVE] = "active", > > + [VB2_BUF_STATE_DONE] = "done", > > + [VB2_BUF_STATE_ERROR] = "error", > > + }; > > + > > + if ((unsigned int)(s) < ARRAY_SIZE(state_names)) > > + return state_names[s]; > > + return "unknown"; > > +} > > + > > /* > > * __vb2_buf_mem_alloc() - allocate video memory for the given buffer > > */ > > @@ -1015,8 +1032,8 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum vb2_buffer_state state) > > */ > > vb->cnt_buf_done++; > > #endif > > - dprintk(q, 4, "done processing on buffer %d, state: %d\n", > > - vb->index, state); > > + dprintk(q, 4, "done processing on buffer %d, state: %s\n", > > + vb->index, vb2_state_name(state)); > > > > if (state != VB2_BUF_STATE_QUEUED) > > __vb2_buf_mem_finish(vb); > > @@ -1490,8 +1507,8 @@ int vb2_core_prepare_buf(struct vb2_queue *q, unsigned int index, void *pb) > > > > vb = q->bufs[index]; > > if (vb->state != VB2_BUF_STATE_DEQUEUED) { > > - dprintk(q, 1, "invalid buffer state %d\n", > > - vb->state); > > + dprintk(q, 1, "invalid buffer state %s\n", > > + vb2_state_name(vb->state)); > > return -EINVAL; > > } > > if (vb->prepared) { > > @@ -1670,7 +1687,8 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb, > > dprintk(q, 1, "buffer still being prepared\n"); > > return -EINVAL; > > default: > > - dprintk(q, 1, "invalid buffer state %d\n", vb->state); > > + dprintk(q, 1, "invalid buffer state %s\n", > > + vb2_state_name(vb->state)); > > return -EINVAL; > > } > > > > @@ -1884,7 +1902,8 @@ int vb2_core_dqbuf(struct vb2_queue *q, unsigned int *pindex, void *pb, > > dprintk(q, 3, "returning done buffer with errors\n"); > > break; > > default: > > - dprintk(q, 1, "invalid buffer state\n"); > > + dprintk(q, 1, "invalid buffer state %s\n", > > + vb2_state_name(vb->state)); > > return -EINVAL; > > } > > > > @@ -1915,8 +1934,8 @@ int vb2_core_dqbuf(struct vb2_queue *q, unsigned int *pindex, void *pb, > > media_request_put(vb->request); > > vb->request = NULL; > > > > - dprintk(q, 2, "dqbuf of buffer %d, with state %d\n", > > - vb->index, vb->state); > > + dprintk(q, 2, "dqbuf of buffer %d, state: %s\n", > > + vb->index, vb2_state_name(vb->state)); > > > > return 0; > > > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3] media: videobuf2: Print videobuf2 buffer state by name 2020-07-13 13:10 ` Ezequiel Garcia @ 2020-07-13 13:45 ` Hans Verkuil 0 siblings, 0 replies; 8+ messages in thread From: Hans Verkuil @ 2020-07-13 13:45 UTC (permalink / raw) To: Ezequiel Garcia, linux-media; +Cc: kernel, Nicolas Dufresne On 13/07/2020 15:10, Ezequiel Garcia wrote: > On Mon, 2020-07-13 at 11:31 +0200, Hans Verkuil wrote: >> On 10/07/2020 15:00, Ezequiel Garcia wrote: >>> For debugging purposes, seeing the state integer >>> representation is really inconvenient. >>> >>> Improve this and be developer-friendly by printing >>> the state name instead. >>> >>> Suggested-by: Nicolas Dufresne <nicolas.dufresne@collabora.com> >>> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> >>> --- >>> v3: >>> * Drop static modifier from the now local name array. >>> v2: >>> * Use a proper function instead of a C macro. >>> >>> .../media/common/videobuf2/videobuf2-core.c | 35 ++++++++++++++----- >>> 1 file changed, 27 insertions(+), 8 deletions(-) >>> >>> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c >>> index abaf28e057eb..d5942cd455bf 100644 >>> --- a/drivers/media/common/videobuf2/videobuf2-core.c >>> +++ b/drivers/media/common/videobuf2/videobuf2-core.c >>> @@ -191,6 +191,23 @@ module_param(debug, int, 0644); >>> static void __vb2_queue_cancel(struct vb2_queue *q); >>> static void __enqueue_in_driver(struct vb2_buffer *vb); >>> >>> +static inline const char *vb2_state_name(enum vb2_buffer_state s) >> >> Why inline? That adds nothing of value. It's too big for an inline >> anyway and it's only used when debug is on. >> > > Yes, you're right. > >>> +{ >>> + const char * const state_names[] = { >> >> This really should be static, local or not. I'm not sure why you dropped >> that. Possibly because of the inline? >> > > I was hesitant about this, as you can see. After some thought, > it seemed a waste of space to have this array allocated statically. > > Could you clarify this for me? Why do you believe it should be static? If it is not static, then the compiler could create this array every time you enter this function. It probably is smart enough to avoid that, but there is no guarantee. This array is constant, will never change, so why not make it static? I'm not really should what you waste here. Regards, Hans > > Thanks, > Ezequiel > >> Regards, >> >> Hans >> >>> + [VB2_BUF_STATE_DEQUEUED] = "dequeued", >>> + [VB2_BUF_STATE_IN_REQUEST] = "in request", >>> + [VB2_BUF_STATE_PREPARING] = "preparing", >>> + [VB2_BUF_STATE_QUEUED] = "queued", >>> + [VB2_BUF_STATE_ACTIVE] = "active", >>> + [VB2_BUF_STATE_DONE] = "done", >>> + [VB2_BUF_STATE_ERROR] = "error", >>> + }; >>> + >>> + if ((unsigned int)(s) < ARRAY_SIZE(state_names)) >>> + return state_names[s]; >>> + return "unknown"; >>> +} >>> + >>> /* >>> * __vb2_buf_mem_alloc() - allocate video memory for the given buffer >>> */ >>> @@ -1015,8 +1032,8 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum vb2_buffer_state state) >>> */ >>> vb->cnt_buf_done++; >>> #endif >>> - dprintk(q, 4, "done processing on buffer %d, state: %d\n", >>> - vb->index, state); >>> + dprintk(q, 4, "done processing on buffer %d, state: %s\n", >>> + vb->index, vb2_state_name(state)); >>> >>> if (state != VB2_BUF_STATE_QUEUED) >>> __vb2_buf_mem_finish(vb); >>> @@ -1490,8 +1507,8 @@ int vb2_core_prepare_buf(struct vb2_queue *q, unsigned int index, void *pb) >>> >>> vb = q->bufs[index]; >>> if (vb->state != VB2_BUF_STATE_DEQUEUED) { >>> - dprintk(q, 1, "invalid buffer state %d\n", >>> - vb->state); >>> + dprintk(q, 1, "invalid buffer state %s\n", >>> + vb2_state_name(vb->state)); >>> return -EINVAL; >>> } >>> if (vb->prepared) { >>> @@ -1670,7 +1687,8 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb, >>> dprintk(q, 1, "buffer still being prepared\n"); >>> return -EINVAL; >>> default: >>> - dprintk(q, 1, "invalid buffer state %d\n", vb->state); >>> + dprintk(q, 1, "invalid buffer state %s\n", >>> + vb2_state_name(vb->state)); >>> return -EINVAL; >>> } >>> >>> @@ -1884,7 +1902,8 @@ int vb2_core_dqbuf(struct vb2_queue *q, unsigned int *pindex, void *pb, >>> dprintk(q, 3, "returning done buffer with errors\n"); >>> break; >>> default: >>> - dprintk(q, 1, "invalid buffer state\n"); >>> + dprintk(q, 1, "invalid buffer state %s\n", >>> + vb2_state_name(vb->state)); >>> return -EINVAL; >>> } >>> >>> @@ -1915,8 +1934,8 @@ int vb2_core_dqbuf(struct vb2_queue *q, unsigned int *pindex, void *pb, >>> media_request_put(vb->request); >>> vb->request = NULL; >>> >>> - dprintk(q, 2, "dqbuf of buffer %d, with state %d\n", >>> - vb->index, vb->state); >>> + dprintk(q, 2, "dqbuf of buffer %d, state: %s\n", >>> + vb->index, vb2_state_name(vb->state)); >>> >>> return 0; >>> >>> > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v4] media: videobuf2: Print videobuf2 buffer state by name 2020-07-10 13:00 ` [PATCH v3] " Ezequiel Garcia 2020-07-13 9:31 ` Hans Verkuil @ 2020-07-17 3:39 ` Ezequiel Garcia 1 sibling, 0 replies; 8+ messages in thread From: Ezequiel Garcia @ 2020-07-17 3:39 UTC (permalink / raw) To: linux-media; +Cc: Hans Verkuil, Ezequiel Garcia, kernel, Nicolas Dufresne For debugging purposes, seeing the state integer representation is really inconvenient. Improve this and be developer-friendly by printing the state name instead. Suggested-by: Nicolas Dufresne <nicolas.dufresne@collabora.com> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> --- v4: * Drop inline modifier. * Add back static modifier from the now local name array. v3: * Drop static modifier from the now local name array. v2: * Use a proper function instead of a C macro. .../media/common/videobuf2/videobuf2-core.c | 35 ++++++++++++++----- 1 file changed, 27 insertions(+), 8 deletions(-) diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c index abaf28e057eb..f544d3393e9d 100644 --- a/drivers/media/common/videobuf2/videobuf2-core.c +++ b/drivers/media/common/videobuf2/videobuf2-core.c @@ -191,6 +191,23 @@ module_param(debug, int, 0644); static void __vb2_queue_cancel(struct vb2_queue *q); static void __enqueue_in_driver(struct vb2_buffer *vb); +static const char *vb2_state_name(enum vb2_buffer_state s) +{ + static const char * const state_names[] = { + [VB2_BUF_STATE_DEQUEUED] = "dequeued", + [VB2_BUF_STATE_IN_REQUEST] = "in request", + [VB2_BUF_STATE_PREPARING] = "preparing", + [VB2_BUF_STATE_QUEUED] = "queued", + [VB2_BUF_STATE_ACTIVE] = "active", + [VB2_BUF_STATE_DONE] = "done", + [VB2_BUF_STATE_ERROR] = "error", + }; + + if ((unsigned int)(s) < ARRAY_SIZE(state_names)) + return state_names[s]; + return "unknown"; +} + /* * __vb2_buf_mem_alloc() - allocate video memory for the given buffer */ @@ -1015,8 +1032,8 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum vb2_buffer_state state) */ vb->cnt_buf_done++; #endif - dprintk(q, 4, "done processing on buffer %d, state: %d\n", - vb->index, state); + dprintk(q, 4, "done processing on buffer %d, state: %s\n", + vb->index, vb2_state_name(state)); if (state != VB2_BUF_STATE_QUEUED) __vb2_buf_mem_finish(vb); @@ -1490,8 +1507,8 @@ int vb2_core_prepare_buf(struct vb2_queue *q, unsigned int index, void *pb) vb = q->bufs[index]; if (vb->state != VB2_BUF_STATE_DEQUEUED) { - dprintk(q, 1, "invalid buffer state %d\n", - vb->state); + dprintk(q, 1, "invalid buffer state %s\n", + vb2_state_name(vb->state)); return -EINVAL; } if (vb->prepared) { @@ -1670,7 +1687,8 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb, dprintk(q, 1, "buffer still being prepared\n"); return -EINVAL; default: - dprintk(q, 1, "invalid buffer state %d\n", vb->state); + dprintk(q, 1, "invalid buffer state %s\n", + vb2_state_name(vb->state)); return -EINVAL; } @@ -1884,7 +1902,8 @@ int vb2_core_dqbuf(struct vb2_queue *q, unsigned int *pindex, void *pb, dprintk(q, 3, "returning done buffer with errors\n"); break; default: - dprintk(q, 1, "invalid buffer state\n"); + dprintk(q, 1, "invalid buffer state %s\n", + vb2_state_name(vb->state)); return -EINVAL; } @@ -1915,8 +1934,8 @@ int vb2_core_dqbuf(struct vb2_queue *q, unsigned int *pindex, void *pb, media_request_put(vb->request); vb->request = NULL; - dprintk(q, 2, "dqbuf of buffer %d, with state %d\n", - vb->index, vb->state); + dprintk(q, 2, "dqbuf of buffer %d, state: %s\n", + vb->index, vb2_state_name(vb->state)); return 0; -- 2.27.0 ^ permalink raw reply related [flat|nested] 8+ messages in thread
end of thread, other threads:[~2020-07-17 3:43 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-07-09 17:43 [PATCH] media: videobuf2: Print videobuf2 buffer state by name Ezequiel Garcia 2020-07-10 10:10 ` Hans Verkuil 2020-07-10 12:51 ` [PATCH v2] " Ezequiel Garcia 2020-07-10 13:00 ` [PATCH v3] " Ezequiel Garcia 2020-07-13 9:31 ` Hans Verkuil 2020-07-13 13:10 ` Ezequiel Garcia 2020-07-13 13:45 ` Hans Verkuil 2020-07-17 3:39 ` [PATCH v4] " Ezequiel Garcia
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.