Re: [PATCH v2 11/12] media: v4l: subdev: Print debug information on frame descriptor

[Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>]

On 18/09/2023 16:39, Laurent Pinchart wrote:
> Hi Sakari,
> 
> Thank you for the patch.
> 
> On Mon, Sep 18, 2023 at 03:51:37PM +0300, Sakari Ailus wrote:
>> Print debug level information on returned frame descriptors.
>>
>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
>> ---
>>   drivers/media/v4l2-core/v4l2-subdev.c | 35 ++++++++++++++++++++++++++-
>>   1 file changed, 34 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
>> index 7b087be3ff4f..abd9275febdb 100644
>> --- a/drivers/media/v4l2-core/v4l2-subdev.c
>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
>> @@ -309,9 +309,42 @@ static int call_set_selection(struct v4l2_subdev *sd,
>>   static int call_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
>>   			       struct v4l2_mbus_frame_desc *fd)
>>   {
>> +	unsigned int i;
>> +	int ret;
>> +
>>   	memset(fd, 0, sizeof(*fd));
>>   
>> -	return sd->ops->pad->get_frame_desc(sd, pad, fd);
>> +	ret = sd->ops->pad->get_frame_desc(sd, pad, fd);
>> +	if (ret)
>> +		return ret;
>> +
>> +	dev_dbg(sd->dev, "Frame descriptor\n");
>> +	dev_dbg(sd->dev, "\ttype %s\n",
>> +		fd->type == V4L2_MBUS_FRAME_DESC_TYPE_PARALLEL ? "parallel" :
>> +		fd->type == V4L2_MBUS_FRAME_DESC_TYPE_CSI2 ? "CSI-2" :
>> +		"unknown");
>> +	dev_dbg(sd->dev, "\tentries %u\n", fd->num_entries);
> 
> You could skip this line, it's implied by the entries that you print
> below.
> 
>> +
>> +	for (i = 0; i < fd->num_entries; i++) {
>> +		struct v4l2_mbus_frame_desc_entry *entry = &fd->entry[i];
>> +
>> +		dev_dbg(sd->dev, "\tentry %u\n", i);
>> +		dev_dbg(sd->dev, "\tflags%s%s\n",
>> +			entry->flags & V4L2_MBUS_FRAME_DESC_FL_LEN_MAX ?
>> +			" LEN_MAX" : "",
>> +			entry->flags & V4L2_MBUS_FRAME_DESC_FL_BLOB ?
>> +			" BLOB" : "");
>> +		dev_dbg(sd->dev, "\t\tstream %u\n", entry->stream);
>> +		dev_dbg(sd->dev, "\t\tpixelcode 0x%4.4x\n", entry->pixelcode);
>> +		dev_dbg(sd->dev, "\t\tlength %u\n", entry->length);
>> +
>> +		if (fd->type == V4L2_MBUS_FRAME_DESC_TYPE_CSI2) {
>> +			dev_dbg(sd->dev, "\t\tvc %u\n", entry->bus.csi2.vc);
>> +			dev_dbg(sd->dev, "\t\tdt 0x%2.2x\n", entry->bus.csi2.dt);
> 
> I'd merge those two in a single line.
> 
>> +		}
>> +	}
> 
> All this is a bit verbose. If it was in a hot path I would be annoyed,
> but in this case I suppose it can be useful for debugging and won't
> affect runtime too much.
> 
> It would be nice if we could have a single check and return early. That
> should be possible by using DEFINE_DYNAMIC_DEBUG_METADATA() and
> DYNAMIC_DEBUG_BRANCH(), like done in alloc_contig_dump_pages() for
> instance. It has the additional upside of being able to control all the
> messages with a single flag. I'm not sure it's worth it though, I'll let
> you decide.

Yes, this is very spammy. Maybe instead of:

Frame descriptor
	type CSI-2
	entries 2
	entry 0
	flags LEN_MAX
		stream 0
		pixelcode 0x3012
		length 3194400
		vc 0
		dt 0x2c
	entry 1
	flags LEN_MAX
		stream 1
		pixelcode 0x8003
		length 2904
		vc 0
		dt 0x12


Frame descriptor type CSI-2:
   pad 0 stream 0 code 0x3012 len 3194400 vc 0 dt 0x2c LEN_MAX
   pad 0 stream 1 code 0x8003 len 2904 vc 0 dt 0x12 LEN_MAX

(note: pad is missing in your patch)

  Tomi