Hi Tomi, Laurent,
Thanks for the review.
On Tue, Sep 19, 2023 at 11:19:36AM +0300, Tomi Valkeinen wrote:
> 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.
I guess that's reasonable if I switch to the format Tomi proposed. You'd
only need to count lines.
> >
> > > +
> > > + 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)
It's indeed much more condensed but still contains all the relevant
information. I'll switch to this (or something alike).
--
Regards,
Sakari Ailus
