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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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.

> +
> +	return 0;
>  }
>  
>  static inline int check_edid(struct v4l2_subdev *sd,

-- 
Regards,

Laurent Pinchart