[PATCH] media: videobuf2: Print videobuf2 buffer state by name

[PATCH] media: videobuf2: Print videobuf2 buffer state by name

[Ezequiel Garcia]

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

Re: [PATCH] media: videobuf2: Print videobuf2 buffer state by name

[Hans Verkuil]

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;
>  
> 

[PATCH v2] media: videobuf2: Print videobuf2 buffer state by name

[Ezequiel Garcia]

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

[PATCH v3] media: videobuf2: Print videobuf2 buffer state by name

[Ezequiel Garcia]

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

Re: [PATCH v3] media: videobuf2: Print videobuf2 buffer state by name

[Hans Verkuil]

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;
>  
> 

Re: [PATCH v3] media: videobuf2: Print videobuf2 buffer state by name

[Ezequiel Garcia]

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;
> >  
> > 

Re: [PATCH v3] media: videobuf2: Print videobuf2 buffer state by name

[Hans Verkuil]

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;
>>>  
>>>
> 
> 

[PATCH v4] media: videobuf2: Print videobuf2 buffer state by name

[Ezequiel Garcia]

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