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