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@xxxxxxxxxxxxxxx>
> ---
> 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
