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

[Hans Verkuil <hverkuil@xs4all.nl>]

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