| Message ID | 20230919121728.126781-1-sakari.ailus@linux.intel.com |
|---|---|
| Headers | show |
| Series | Small fixes and cleanups (ov2740 and ccs) | expand |
Hi Laurent, On Tue, Sep 19, 2023 at 04:32:48PM +0300, Laurent Pinchart wrote: > Hi Sakari, > > Thank you for the patch. > > On Tue, Sep 19, 2023 at 03:17:27PM +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 | 32 ++++++++++++++++++++++++++- > > 1 file changed, 31 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c > > index 7b087be3ff4f..504ca625b2bd 100644 > > --- a/drivers/media/v4l2-core/v4l2-subdev.c > > +++ b/drivers/media/v4l2-core/v4l2-subdev.c > > @@ -15,6 +15,7 @@ > > #include <linux/module.h> > > #include <linux/overflow.h> > > #include <linux/slab.h> > > +#include <linux/string.h> > > #include <linux/types.h> > > #include <linux/version.h> > > #include <linux/videodev2.h> > > @@ -309,9 +310,38 @@ 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 on pad %u, type %s\n", pad, > > + fd->type == V4L2_MBUS_FRAME_DESC_TYPE_PARALLEL ? "parallel" : > > + fd->type == V4L2_MBUS_FRAME_DESC_TYPE_CSI2 ? "CSI-2" : > > + "unknown"); > > + > > + for (i = 0; i < fd->num_entries; i++) { > > + struct v4l2_mbus_frame_desc_entry *entry = &fd->entry[i]; > > + char buf[20] = ""; > > Should this be sized for the worst case ? The vc and dt should not be > large, but a buffer overflow on the stack in debug code if a subdev > returns an incorrect value would be bad. 17 should be enough but it's not useful to use a size not divisible by 4 in practice here. > > > + > > + if (fd->type == V4L2_MBUS_FRAME_DESC_TYPE_CSI2) > > + snprintf(buf, sizeof(buf), ", vc %u, dt 0x%2.2x", > > 0x%02x would be one character shorter ;-) Same below. It would be, but I prefer the above notation as it's more generic. > > > + entry->bus.csi2.vc, entry->bus.csi2.dt); > > + > > + dev_dbg(sd->dev, > > + "\tstream %u, code 0x%4.4x, lenght %u, flags%s%s%s\n", > > s/lenght/length/ Thanks, I'll fix this. > > > + entry->stream, entry->pixelcode, entry->length, > > + entry->flags & V4L2_MBUS_FRAME_DESC_FL_LEN_MAX ? > > + " LEN_MAX" : "", > > + entry->flags & V4L2_MBUS_FRAME_DESC_FL_BLOB ? > > + " BLOB" : "", buf); > > If no flags are set, you will get something like > > stream 0, code 0x1234, length ..., flags, vc 0, dt 0x2a > > Maybe printing the hex value for the flags would be simpler and clearer > ? How about adding the numerical value in addition to the strings? The use of these flags is rare though. I could just remove the strings, too.
On Tue, Sep 19, 2023 at 02:49:29PM +0000, Sakari Ailus wrote: > Hi Laurent, > > On Tue, Sep 19, 2023 at 04:32:48PM +0300, Laurent Pinchart wrote: > > Hi Sakari, > > > > Thank you for the patch. > > > > On Tue, Sep 19, 2023 at 03:17:27PM +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 | 32 ++++++++++++++++++++++++++- > > > 1 file changed, 31 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c > > > index 7b087be3ff4f..504ca625b2bd 100644 > > > --- a/drivers/media/v4l2-core/v4l2-subdev.c > > > +++ b/drivers/media/v4l2-core/v4l2-subdev.c > > > @@ -15,6 +15,7 @@ > > > #include <linux/module.h> > > > #include <linux/overflow.h> > > > #include <linux/slab.h> > > > +#include <linux/string.h> > > > #include <linux/types.h> > > > #include <linux/version.h> > > > #include <linux/videodev2.h> > > > @@ -309,9 +310,38 @@ 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 on pad %u, type %s\n", pad, > > > + fd->type == V4L2_MBUS_FRAME_DESC_TYPE_PARALLEL ? "parallel" : > > > + fd->type == V4L2_MBUS_FRAME_DESC_TYPE_CSI2 ? "CSI-2" : > > > + "unknown"); > > > + > > > + for (i = 0; i < fd->num_entries; i++) { > > > + struct v4l2_mbus_frame_desc_entry *entry = &fd->entry[i]; > > > + char buf[20] = ""; > > > > Should this be sized for the worst case ? The vc and dt should not be > > large, but a buffer overflow on the stack in debug code if a subdev > > returns an incorrect value would be bad. > > 17 should be enough but it's not useful to use a size not divisible by 4 in > practice here. 18 with the terminating 0. But indeed, it's large enough as vc and dt are u8. I'm just a bit worried we're opening the door to hard to debug problems if we later change the vc and dt types. > > > + > > > + if (fd->type == V4L2_MBUS_FRAME_DESC_TYPE_CSI2) > > > + snprintf(buf, sizeof(buf), ", vc %u, dt 0x%2.2x", > > > > 0x%02x would be one character shorter ;-) Same below. > > It would be, but I prefer the above notation as it's more generic. Out of curiosity, how so ? > > > + entry->bus.csi2.vc, entry->bus.csi2.dt); > > > + > > > + dev_dbg(sd->dev, > > > + "\tstream %u, code 0x%4.4x, lenght %u, flags%s%s%s\n", > > > > s/lenght/length/ > > Thanks, I'll fix this. > > > > + entry->stream, entry->pixelcode, entry->length, > > > + entry->flags & V4L2_MBUS_FRAME_DESC_FL_LEN_MAX ? > > > + " LEN_MAX" : "", > > > + entry->flags & V4L2_MBUS_FRAME_DESC_FL_BLOB ? > > > + " BLOB" : "", buf); > > > > If no flags are set, you will get something like > > > > stream 0, code 0x1234, length ..., flags, vc 0, dt 0x2a > > > > Maybe printing the hex value for the flags would be simpler and clearer > > ? > > How about adding the numerical value in addition to the strings? > > The use of these flags is rare though. I could just remove the strings, > too. Up to you.
On Tue, Sep 19, 2023 at 06:21:23PM +0300, Laurent Pinchart wrote: > On Tue, Sep 19, 2023 at 02:49:29PM +0000, Sakari Ailus wrote: > > Hi Laurent, > > > > On Tue, Sep 19, 2023 at 04:32:48PM +0300, Laurent Pinchart wrote: > > > Hi Sakari, > > > > > > Thank you for the patch. > > > > > > On Tue, Sep 19, 2023 at 03:17:27PM +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 | 32 ++++++++++++++++++++++++++- > > > > 1 file changed, 31 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c > > > > index 7b087be3ff4f..504ca625b2bd 100644 > > > > --- a/drivers/media/v4l2-core/v4l2-subdev.c > > > > +++ b/drivers/media/v4l2-core/v4l2-subdev.c > > > > @@ -15,6 +15,7 @@ > > > > #include <linux/module.h> > > > > #include <linux/overflow.h> > > > > #include <linux/slab.h> > > > > +#include <linux/string.h> > > > > #include <linux/types.h> > > > > #include <linux/version.h> > > > > #include <linux/videodev2.h> > > > > @@ -309,9 +310,38 @@ 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 on pad %u, type %s\n", pad, > > > > + fd->type == V4L2_MBUS_FRAME_DESC_TYPE_PARALLEL ? "parallel" : > > > > + fd->type == V4L2_MBUS_FRAME_DESC_TYPE_CSI2 ? "CSI-2" : > > > > + "unknown"); > > > > + > > > > + for (i = 0; i < fd->num_entries; i++) { > > > > + struct v4l2_mbus_frame_desc_entry *entry = &fd->entry[i]; > > > > + char buf[20] = ""; > > > > > > Should this be sized for the worst case ? The vc and dt should not be > > > large, but a buffer overflow on the stack in debug code if a subdev > > > returns an incorrect value would be bad. > > > > 17 should be enough but it's not useful to use a size not divisible by 4 in > > practice here. > > 18 with the terminating 0. But indeed, it's large enough as vc and dt I can count only 17 --- there's no newline. I guess it's most probably either of these then. X-) > are u8. I'm just a bit worried we're opening the door to hard to debug > problems if we later change the vc and dt types. I can add a WARN_ON() to cover this. > > > > > + > > > > + if (fd->type == V4L2_MBUS_FRAME_DESC_TYPE_CSI2) > > > > + snprintf(buf, sizeof(buf), ", vc %u, dt 0x%2.2x", > > > > > > 0x%02x would be one character shorter ;-) Same below. > > > > It would be, but I prefer the above notation as it's more generic. > > Out of curiosity, how so ? It works with data that would span more than 9 characters when printed. > > > > > + entry->bus.csi2.vc, entry->bus.csi2.dt); > > > > + > > > > + dev_dbg(sd->dev, > > > > + "\tstream %u, code 0x%4.4x, lenght %u, flags%s%s%s\n", > > > > > > s/lenght/length/ > > > > Thanks, I'll fix this. > > > > > > + entry->stream, entry->pixelcode, entry->length, > > > > + entry->flags & V4L2_MBUS_FRAME_DESC_FL_LEN_MAX ? > > > > + " LEN_MAX" : "", > > > > + entry->flags & V4L2_MBUS_FRAME_DESC_FL_BLOB ? > > > > + " BLOB" : "", buf); > > > > > > If no flags are set, you will get something like > > > > > > stream 0, code 0x1234, length ..., flags, vc 0, dt 0x2a > > > > > > Maybe printing the hex value for the flags would be simpler and clearer > > > ? > > > > How about adding the numerical value in addition to the strings? > > > > The use of these flags is rare though. I could just remove the strings, > > too. > > Up to you. On second thought, I'll make it an integer only. If we'll get more commonly used flags, we can rethink about strings.
On Fri, Sep 22, 2023 at 09:41:01AM +0000, Sakari Ailus wrote: > On Tue, Sep 19, 2023 at 06:21:23PM +0300, Laurent Pinchart wrote: > > On Tue, Sep 19, 2023 at 02:49:29PM +0000, Sakari Ailus wrote: > > > On Tue, Sep 19, 2023 at 04:32:48PM +0300, Laurent Pinchart wrote: > > > > On Tue, Sep 19, 2023 at 03:17:27PM +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 | 32 ++++++++++++++++++++++++++- > > > > > 1 file changed, 31 insertions(+), 1 deletion(-) > > > > > > > > > > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c > > > > > index 7b087be3ff4f..504ca625b2bd 100644 > > > > > --- a/drivers/media/v4l2-core/v4l2-subdev.c > > > > > +++ b/drivers/media/v4l2-core/v4l2-subdev.c > > > > > @@ -15,6 +15,7 @@ > > > > > #include <linux/module.h> > > > > > #include <linux/overflow.h> > > > > > #include <linux/slab.h> > > > > > +#include <linux/string.h> > > > > > #include <linux/types.h> > > > > > #include <linux/version.h> > > > > > #include <linux/videodev2.h> > > > > > @@ -309,9 +310,38 @@ 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 on pad %u, type %s\n", pad, > > > > > + fd->type == V4L2_MBUS_FRAME_DESC_TYPE_PARALLEL ? "parallel" : > > > > > + fd->type == V4L2_MBUS_FRAME_DESC_TYPE_CSI2 ? "CSI-2" : > > > > > + "unknown"); > > > > > + > > > > > + for (i = 0; i < fd->num_entries; i++) { > > > > > + struct v4l2_mbus_frame_desc_entry *entry = &fd->entry[i]; > > > > > + char buf[20] = ""; > > > > > > > > Should this be sized for the worst case ? The vc and dt should not be > > > > large, but a buffer overflow on the stack in debug code if a subdev > > > > returns an incorrect value would be bad. > > > > > > 17 should be enough but it's not useful to use a size not divisible by 4 in > > > practice here. > > > > 18 with the terminating 0. But indeed, it's large enough as vc and dt > > I can count only 17 --- there's no newline. > > I guess it's most probably either of these then. X-) > > > are u8. I'm just a bit worried we're opening the door to hard to debug > > problems if we later change the vc and dt types. > > I can add a WARN_ON() to cover this. > > > > > > + > > > > > + if (fd->type == V4L2_MBUS_FRAME_DESC_TYPE_CSI2) > > > > > + snprintf(buf, sizeof(buf), ", vc %u, dt 0x%2.2x", > > > > > > > > 0x%02x would be one character shorter ;-) Same below. > > > > > > It would be, but I prefer the above notation as it's more generic. > > > > Out of curiosity, how so ? > > It works with data that would span more than 9 characters when printed. And 0x%02x doesn't ? > > > > > + entry->bus.csi2.vc, entry->bus.csi2.dt); > > > > > + > > > > > + dev_dbg(sd->dev, > > > > > + "\tstream %u, code 0x%4.4x, lenght %u, flags%s%s%s\n", > > > > > > > > s/lenght/length/ > > > > > > Thanks, I'll fix this. > > > > > > > > + entry->stream, entry->pixelcode, entry->length, > > > > > + entry->flags & V4L2_MBUS_FRAME_DESC_FL_LEN_MAX ? > > > > > + " LEN_MAX" : "", > > > > > + entry->flags & V4L2_MBUS_FRAME_DESC_FL_BLOB ? > > > > > + " BLOB" : "", buf); > > > > > > > > If no flags are set, you will get something like > > > > > > > > stream 0, code 0x1234, length ..., flags, vc 0, dt 0x2a > > > > > > > > Maybe printing the hex value for the flags would be simpler and clearer > > > > ? > > > > > > How about adding the numerical value in addition to the strings? > > > > > > The use of these flags is rare though. I could just remove the strings, > > > too. > > > > Up to you. > > On second thought, I'll make it an integer only. If we'll get more commonly > used flags, we can rethink about strings.
On Fri, Sep 22, 2023 at 12:53:20PM +0300, Laurent Pinchart wrote: > On Fri, Sep 22, 2023 at 09:41:01AM +0000, Sakari Ailus wrote: > > On Tue, Sep 19, 2023 at 06:21:23PM +0300, Laurent Pinchart wrote: > > > On Tue, Sep 19, 2023 at 02:49:29PM +0000, Sakari Ailus wrote: > > > > On Tue, Sep 19, 2023 at 04:32:48PM +0300, Laurent Pinchart wrote: > > > > > On Tue, Sep 19, 2023 at 03:17:27PM +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 | 32 ++++++++++++++++++++++++++- > > > > > > 1 file changed, 31 insertions(+), 1 deletion(-) > > > > > > > > > > > > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c > > > > > > index 7b087be3ff4f..504ca625b2bd 100644 > > > > > > --- a/drivers/media/v4l2-core/v4l2-subdev.c > > > > > > +++ b/drivers/media/v4l2-core/v4l2-subdev.c > > > > > > @@ -15,6 +15,7 @@ > > > > > > #include <linux/module.h> > > > > > > #include <linux/overflow.h> > > > > > > #include <linux/slab.h> > > > > > > +#include <linux/string.h> > > > > > > #include <linux/types.h> > > > > > > #include <linux/version.h> > > > > > > #include <linux/videodev2.h> > > > > > > @@ -309,9 +310,38 @@ 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 on pad %u, type %s\n", pad, > > > > > > + fd->type == V4L2_MBUS_FRAME_DESC_TYPE_PARALLEL ? "parallel" : > > > > > > + fd->type == V4L2_MBUS_FRAME_DESC_TYPE_CSI2 ? "CSI-2" : > > > > > > + "unknown"); > > > > > > + > > > > > > + for (i = 0; i < fd->num_entries; i++) { > > > > > > + struct v4l2_mbus_frame_desc_entry *entry = &fd->entry[i]; > > > > > > + char buf[20] = ""; > > > > > > > > > > Should this be sized for the worst case ? The vc and dt should not be > > > > > large, but a buffer overflow on the stack in debug code if a subdev > > > > > returns an incorrect value would be bad. > > > > > > > > 17 should be enough but it's not useful to use a size not divisible by 4 in > > > > practice here. > > > > > > 18 with the terminating 0. But indeed, it's large enough as vc and dt > > > > I can count only 17 --- there's no newline. > > > > I guess it's most probably either of these then. X-) > > > > > are u8. I'm just a bit worried we're opening the door to hard to debug > > > problems if we later change the vc and dt types. > > > > I can add a WARN_ON() to cover this. > > > > > > > > + > > > > > > + if (fd->type == V4L2_MBUS_FRAME_DESC_TYPE_CSI2) > > > > > > + snprintf(buf, sizeof(buf), ", vc %u, dt 0x%2.2x", > > > > > > > > > > 0x%02x would be one character shorter ;-) Same below. > > > > > > > > It would be, but I prefer the above notation as it's more generic. > > > > > > Out of curiosity, how so ? > > > > It works with data that would span more than 9 characters when printed. > > And 0x%02x doesn't ? Ah, it should indeed work, 0 is actually a flag here and part of the field width or precision. I can use that in v4.
On 22/09/2023 13:09, Sakari Ailus wrote: > On Fri, Sep 22, 2023 at 12:53:20PM +0300, Laurent Pinchart wrote: >> On Fri, Sep 22, 2023 at 09:41:01AM +0000, Sakari Ailus wrote: >>> On Tue, Sep 19, 2023 at 06:21:23PM +0300, Laurent Pinchart wrote: >>>> On Tue, Sep 19, 2023 at 02:49:29PM +0000, Sakari Ailus wrote: >>>>> On Tue, Sep 19, 2023 at 04:32:48PM +0300, Laurent Pinchart wrote: >>>>>> On Tue, Sep 19, 2023 at 03:17:27PM +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 | 32 ++++++++++++++++++++++++++- >>>>>>> 1 file changed, 31 insertions(+), 1 deletion(-) >>>>>>> >>>>>>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c >>>>>>> index 7b087be3ff4f..504ca625b2bd 100644 >>>>>>> --- a/drivers/media/v4l2-core/v4l2-subdev.c >>>>>>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c >>>>>>> @@ -15,6 +15,7 @@ >>>>>>> #include <linux/module.h> >>>>>>> #include <linux/overflow.h> >>>>>>> #include <linux/slab.h> >>>>>>> +#include <linux/string.h> >>>>>>> #include <linux/types.h> >>>>>>> #include <linux/version.h> >>>>>>> #include <linux/videodev2.h> >>>>>>> @@ -309,9 +310,38 @@ 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 on pad %u, type %s\n", pad, >>>>>>> + fd->type == V4L2_MBUS_FRAME_DESC_TYPE_PARALLEL ? "parallel" : >>>>>>> + fd->type == V4L2_MBUS_FRAME_DESC_TYPE_CSI2 ? "CSI-2" : >>>>>>> + "unknown"); >>>>>>> + >>>>>>> + for (i = 0; i < fd->num_entries; i++) { >>>>>>> + struct v4l2_mbus_frame_desc_entry *entry = &fd->entry[i]; >>>>>>> + char buf[20] = ""; >>>>>> >>>>>> Should this be sized for the worst case ? The vc and dt should not be >>>>>> large, but a buffer overflow on the stack in debug code if a subdev >>>>>> returns an incorrect value would be bad. >>>>> >>>>> 17 should be enough but it's not useful to use a size not divisible by 4 in >>>>> practice here. >>>> >>>> 18 with the terminating 0. But indeed, it's large enough as vc and dt >>> >>> I can count only 17 --- there's no newline. >>> >>> I guess it's most probably either of these then. X-) >>> >>>> are u8. I'm just a bit worried we're opening the door to hard to debug >>>> problems if we later change the vc and dt types. >>> >>> I can add a WARN_ON() to cover this. >>> >>>>>>> + >>>>>>> + if (fd->type == V4L2_MBUS_FRAME_DESC_TYPE_CSI2) >>>>>>> + snprintf(buf, sizeof(buf), ", vc %u, dt 0x%2.2x", >>>>>> >>>>>> 0x%02x would be one character shorter ;-) Same below. >>>>> >>>>> It would be, but I prefer the above notation as it's more generic. >>>> >>>> Out of curiosity, how so ? >>> >>> It works with data that would span more than 9 characters when printed. >> >> And 0x%02x doesn't ? > > Ah, it should indeed work, 0 is actually a flag here and part of the field > width or precision. I can use that in v4. Or %#04x, even shorter! Tomi
On Fri, Sep 22, 2023 at 01:16:21PM +0300, Tomi Valkeinen wrote: > On 22/09/2023 13:09, Sakari Ailus wrote: > > On Fri, Sep 22, 2023 at 12:53:20PM +0300, Laurent Pinchart wrote: > >> On Fri, Sep 22, 2023 at 09:41:01AM +0000, Sakari Ailus wrote: > >>> On Tue, Sep 19, 2023 at 06:21:23PM +0300, Laurent Pinchart wrote: > >>>> On Tue, Sep 19, 2023 at 02:49:29PM +0000, Sakari Ailus wrote: > >>>>> On Tue, Sep 19, 2023 at 04:32:48PM +0300, Laurent Pinchart wrote: > >>>>>> On Tue, Sep 19, 2023 at 03:17:27PM +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 | 32 ++++++++++++++++++++++++++- > >>>>>>> 1 file changed, 31 insertions(+), 1 deletion(-) > >>>>>>> > >>>>>>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c > >>>>>>> index 7b087be3ff4f..504ca625b2bd 100644 > >>>>>>> --- a/drivers/media/v4l2-core/v4l2-subdev.c > >>>>>>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c > >>>>>>> @@ -15,6 +15,7 @@ > >>>>>>> #include <linux/module.h> > >>>>>>> #include <linux/overflow.h> > >>>>>>> #include <linux/slab.h> > >>>>>>> +#include <linux/string.h> > >>>>>>> #include <linux/types.h> > >>>>>>> #include <linux/version.h> > >>>>>>> #include <linux/videodev2.h> > >>>>>>> @@ -309,9 +310,38 @@ 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 on pad %u, type %s\n", pad, > >>>>>>> + fd->type == V4L2_MBUS_FRAME_DESC_TYPE_PARALLEL ? "parallel" : > >>>>>>> + fd->type == V4L2_MBUS_FRAME_DESC_TYPE_CSI2 ? "CSI-2" : > >>>>>>> + "unknown"); > >>>>>>> + > >>>>>>> + for (i = 0; i < fd->num_entries; i++) { > >>>>>>> + struct v4l2_mbus_frame_desc_entry *entry = &fd->entry[i]; > >>>>>>> + char buf[20] = ""; > >>>>>> > >>>>>> Should this be sized for the worst case ? The vc and dt should not be > >>>>>> large, but a buffer overflow on the stack in debug code if a subdev > >>>>>> returns an incorrect value would be bad. > >>>>> > >>>>> 17 should be enough but it's not useful to use a size not divisible by 4 in > >>>>> practice here. > >>>> > >>>> 18 with the terminating 0. But indeed, it's large enough as vc and dt > >>> > >>> I can count only 17 --- there's no newline. > >>> > >>> I guess it's most probably either of these then. X-) > >>> > >>>> are u8. I'm just a bit worried we're opening the door to hard to debug > >>>> problems if we later change the vc and dt types. > >>> > >>> I can add a WARN_ON() to cover this. > >>> > >>>>>>> + > >>>>>>> + if (fd->type == V4L2_MBUS_FRAME_DESC_TYPE_CSI2) > >>>>>>> + snprintf(buf, sizeof(buf), ", vc %u, dt 0x%2.2x", > >>>>>> > >>>>>> 0x%02x would be one character shorter ;-) Same below. > >>>>> > >>>>> It would be, but I prefer the above notation as it's more generic. > >>>> > >>>> Out of curiosity, how so ? > >>> > >>> It works with data that would span more than 9 characters when printed. > >> > >> And 0x%02x doesn't ? > > > > Ah, it should indeed work, 0 is actually a flag here and part of the field > > width or precision. I can use that in v4. > > Or %#04x, even shorter! That's different though. printf("0x%2.2x\n", 0); -> 0x00 printf("0x%2.2x\n", 1); -> 0x01 printf("0x%2.2x\n", 42); -> 0x2a printf("0x%02x\n", 0); -> 0x00 printf("0x%02x\n", 1); -> 0x01 printf("0x%02x\n", 42); -> 0x2a printf("%#2.2x\n", 0); -> 00 printf("%#2.2x\n", 1); -> 0x01 printf("%#2.2x\n", 42); -> 0x2a printf("%#02x\n", 0); -> 00 printf("%#02x\n", 1); -> 0x1 printf("%#02x\n", 42); -> 0x2a
On Fri, Sep 22, 2023 at 01:16:21PM +0300, Tomi Valkeinen wrote: > On 22/09/2023 13:09, Sakari Ailus wrote: > > On Fri, Sep 22, 2023 at 12:53:20PM +0300, Laurent Pinchart wrote: > > > On Fri, Sep 22, 2023 at 09:41:01AM +0000, Sakari Ailus wrote: > > > > On Tue, Sep 19, 2023 at 06:21:23PM +0300, Laurent Pinchart wrote: > > > > > On Tue, Sep 19, 2023 at 02:49:29PM +0000, Sakari Ailus wrote: > > > > > > On Tue, Sep 19, 2023 at 04:32:48PM +0300, Laurent Pinchart wrote: > > > > > > > On Tue, Sep 19, 2023 at 03:17:27PM +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 | 32 ++++++++++++++++++++++++++- > > > > > > > > 1 file changed, 31 insertions(+), 1 deletion(-) > > > > > > > > > > > > > > > > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c > > > > > > > > index 7b087be3ff4f..504ca625b2bd 100644 > > > > > > > > --- a/drivers/media/v4l2-core/v4l2-subdev.c > > > > > > > > +++ b/drivers/media/v4l2-core/v4l2-subdev.c > > > > > > > > @@ -15,6 +15,7 @@ > > > > > > > > #include <linux/module.h> > > > > > > > > #include <linux/overflow.h> > > > > > > > > #include <linux/slab.h> > > > > > > > > +#include <linux/string.h> > > > > > > > > #include <linux/types.h> > > > > > > > > #include <linux/version.h> > > > > > > > > #include <linux/videodev2.h> > > > > > > > > @@ -309,9 +310,38 @@ 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 on pad %u, type %s\n", pad, > > > > > > > > + fd->type == V4L2_MBUS_FRAME_DESC_TYPE_PARALLEL ? "parallel" : > > > > > > > > + fd->type == V4L2_MBUS_FRAME_DESC_TYPE_CSI2 ? "CSI-2" : > > > > > > > > + "unknown"); > > > > > > > > + > > > > > > > > + for (i = 0; i < fd->num_entries; i++) { > > > > > > > > + struct v4l2_mbus_frame_desc_entry *entry = &fd->entry[i]; > > > > > > > > + char buf[20] = ""; > > > > > > > > > > > > > > Should this be sized for the worst case ? The vc and dt should not be > > > > > > > large, but a buffer overflow on the stack in debug code if a subdev > > > > > > > returns an incorrect value would be bad. > > > > > > > > > > > > 17 should be enough but it's not useful to use a size not divisible by 4 in > > > > > > practice here. > > > > > > > > > > 18 with the terminating 0. But indeed, it's large enough as vc and dt > > > > > > > > I can count only 17 --- there's no newline. > > > > > > > > I guess it's most probably either of these then. X-) > > > > > > > > > are u8. I'm just a bit worried we're opening the door to hard to debug > > > > > problems if we later change the vc and dt types. > > > > > > > > I can add a WARN_ON() to cover this. > > > > > > > > > > > > + > > > > > > > > + if (fd->type == V4L2_MBUS_FRAME_DESC_TYPE_CSI2) > > > > > > > > + snprintf(buf, sizeof(buf), ", vc %u, dt 0x%2.2x", > > > > > > > > > > > > > > 0x%02x would be one character shorter ;-) Same below. > > > > > > > > > > > > It would be, but I prefer the above notation as it's more generic. > > > > > > > > > > Out of curiosity, how so ? > > > > > > > > It works with data that would span more than 9 characters when printed. > > > > > > And 0x%02x doesn't ? > > > > Ah, it should indeed work, 0 is actually a flag here and part of the field > > width or precision. I can use that in v4. > > Or %#04x, even shorter! No-one has used that in the media tree so far. I'll use 0x%02x, that appears to be common.
On 22/09/2023 13:22, Laurent Pinchart wrote: > On Fri, Sep 22, 2023 at 01:16:21PM +0300, Tomi Valkeinen wrote: >> On 22/09/2023 13:09, Sakari Ailus wrote: >>> On Fri, Sep 22, 2023 at 12:53:20PM +0300, Laurent Pinchart wrote: >>>> On Fri, Sep 22, 2023 at 09:41:01AM +0000, Sakari Ailus wrote: >>>>> On Tue, Sep 19, 2023 at 06:21:23PM +0300, Laurent Pinchart wrote: >>>>>> On Tue, Sep 19, 2023 at 02:49:29PM +0000, Sakari Ailus wrote: >>>>>>> On Tue, Sep 19, 2023 at 04:32:48PM +0300, Laurent Pinchart wrote: >>>>>>>> On Tue, Sep 19, 2023 at 03:17:27PM +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 | 32 ++++++++++++++++++++++++++- >>>>>>>>> 1 file changed, 31 insertions(+), 1 deletion(-) >>>>>>>>> >>>>>>>>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c >>>>>>>>> index 7b087be3ff4f..504ca625b2bd 100644 >>>>>>>>> --- a/drivers/media/v4l2-core/v4l2-subdev.c >>>>>>>>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c >>>>>>>>> @@ -15,6 +15,7 @@ >>>>>>>>> #include <linux/module.h> >>>>>>>>> #include <linux/overflow.h> >>>>>>>>> #include <linux/slab.h> >>>>>>>>> +#include <linux/string.h> >>>>>>>>> #include <linux/types.h> >>>>>>>>> #include <linux/version.h> >>>>>>>>> #include <linux/videodev2.h> >>>>>>>>> @@ -309,9 +310,38 @@ 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 on pad %u, type %s\n", pad, >>>>>>>>> + fd->type == V4L2_MBUS_FRAME_DESC_TYPE_PARALLEL ? "parallel" : >>>>>>>>> + fd->type == V4L2_MBUS_FRAME_DESC_TYPE_CSI2 ? "CSI-2" : >>>>>>>>> + "unknown"); >>>>>>>>> + >>>>>>>>> + for (i = 0; i < fd->num_entries; i++) { >>>>>>>>> + struct v4l2_mbus_frame_desc_entry *entry = &fd->entry[i]; >>>>>>>>> + char buf[20] = ""; >>>>>>>> >>>>>>>> Should this be sized for the worst case ? The vc and dt should not be >>>>>>>> large, but a buffer overflow on the stack in debug code if a subdev >>>>>>>> returns an incorrect value would be bad. >>>>>>> >>>>>>> 17 should be enough but it's not useful to use a size not divisible by 4 in >>>>>>> practice here. >>>>>> >>>>>> 18 with the terminating 0. But indeed, it's large enough as vc and dt >>>>> >>>>> I can count only 17 --- there's no newline. >>>>> >>>>> I guess it's most probably either of these then. X-) >>>>> >>>>>> are u8. I'm just a bit worried we're opening the door to hard to debug >>>>>> problems if we later change the vc and dt types. >>>>> >>>>> I can add a WARN_ON() to cover this. >>>>> >>>>>>>>> + >>>>>>>>> + if (fd->type == V4L2_MBUS_FRAME_DESC_TYPE_CSI2) >>>>>>>>> + snprintf(buf, sizeof(buf), ", vc %u, dt 0x%2.2x", >>>>>>>> >>>>>>>> 0x%02x would be one character shorter ;-) Same below. >>>>>>> >>>>>>> It would be, but I prefer the above notation as it's more generic. >>>>>> >>>>>> Out of curiosity, how so ? >>>>> >>>>> It works with data that would span more than 9 characters when printed. >>>> >>>> And 0x%02x doesn't ? >>> >>> Ah, it should indeed work, 0 is actually a flag here and part of the field >>> width or precision. I can use that in v4. >> >> Or %#04x, even shorter! > > That's different though. > > printf("0x%2.2x\n", 0); -> 0x00 > printf("0x%2.2x\n", 1); -> 0x01 > printf("0x%2.2x\n", 42); -> 0x2a > > printf("0x%02x\n", 0); -> 0x00 > printf("0x%02x\n", 1); -> 0x01 > printf("0x%02x\n", 42); -> 0x2a > > printf("%#2.2x\n", 0); -> 00 > printf("%#2.2x\n", 1); -> 0x01 > printf("%#2.2x\n", 42); -> 0x2a > > printf("%#02x\n", 0); -> 00 > printf("%#02x\n", 1); -> 0x1 > printf("%#02x\n", 42); -> 0x2a The length should be 4 there, not 2. But even after fixing that, the 0 case is printed wrong. Interesting, I haven't noticed that before. I wonder why it behaves that way... 0000 0x01 0x2a Tomi
On Fri, Sep 22, 2023 at 01:27:46PM +0300, Tomi Valkeinen wrote: > On 22/09/2023 13:22, Laurent Pinchart wrote: > > On Fri, Sep 22, 2023 at 01:16:21PM +0300, Tomi Valkeinen wrote: > >> On 22/09/2023 13:09, Sakari Ailus wrote: > >>> On Fri, Sep 22, 2023 at 12:53:20PM +0300, Laurent Pinchart wrote: > >>>> On Fri, Sep 22, 2023 at 09:41:01AM +0000, Sakari Ailus wrote: > >>>>> On Tue, Sep 19, 2023 at 06:21:23PM +0300, Laurent Pinchart wrote: > >>>>>> On Tue, Sep 19, 2023 at 02:49:29PM +0000, Sakari Ailus wrote: > >>>>>>> On Tue, Sep 19, 2023 at 04:32:48PM +0300, Laurent Pinchart wrote: > >>>>>>>> On Tue, Sep 19, 2023 at 03:17:27PM +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 | 32 ++++++++++++++++++++++++++- > >>>>>>>>> 1 file changed, 31 insertions(+), 1 deletion(-) > >>>>>>>>> > >>>>>>>>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c > >>>>>>>>> index 7b087be3ff4f..504ca625b2bd 100644 > >>>>>>>>> --- a/drivers/media/v4l2-core/v4l2-subdev.c > >>>>>>>>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c > >>>>>>>>> @@ -15,6 +15,7 @@ > >>>>>>>>> #include <linux/module.h> > >>>>>>>>> #include <linux/overflow.h> > >>>>>>>>> #include <linux/slab.h> > >>>>>>>>> +#include <linux/string.h> > >>>>>>>>> #include <linux/types.h> > >>>>>>>>> #include <linux/version.h> > >>>>>>>>> #include <linux/videodev2.h> > >>>>>>>>> @@ -309,9 +310,38 @@ 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 on pad %u, type %s\n", pad, > >>>>>>>>> + fd->type == V4L2_MBUS_FRAME_DESC_TYPE_PARALLEL ? "parallel" : > >>>>>>>>> + fd->type == V4L2_MBUS_FRAME_DESC_TYPE_CSI2 ? "CSI-2" : > >>>>>>>>> + "unknown"); > >>>>>>>>> + > >>>>>>>>> + for (i = 0; i < fd->num_entries; i++) { > >>>>>>>>> + struct v4l2_mbus_frame_desc_entry *entry = &fd->entry[i]; > >>>>>>>>> + char buf[20] = ""; > >>>>>>>> > >>>>>>>> Should this be sized for the worst case ? The vc and dt should not be > >>>>>>>> large, but a buffer overflow on the stack in debug code if a subdev > >>>>>>>> returns an incorrect value would be bad. > >>>>>>> > >>>>>>> 17 should be enough but it's not useful to use a size not divisible by 4 in > >>>>>>> practice here. > >>>>>> > >>>>>> 18 with the terminating 0. But indeed, it's large enough as vc and dt > >>>>> > >>>>> I can count only 17 --- there's no newline. > >>>>> > >>>>> I guess it's most probably either of these then. X-) > >>>>> > >>>>>> are u8. I'm just a bit worried we're opening the door to hard to debug > >>>>>> problems if we later change the vc and dt types. > >>>>> > >>>>> I can add a WARN_ON() to cover this. > >>>>> > >>>>>>>>> + > >>>>>>>>> + if (fd->type == V4L2_MBUS_FRAME_DESC_TYPE_CSI2) > >>>>>>>>> + snprintf(buf, sizeof(buf), ", vc %u, dt 0x%2.2x", > >>>>>>>> > >>>>>>>> 0x%02x would be one character shorter ;-) Same below. > >>>>>>> > >>>>>>> It would be, but I prefer the above notation as it's more generic. > >>>>>> > >>>>>> Out of curiosity, how so ? > >>>>> > >>>>> It works with data that would span more than 9 characters when printed. > >>>> > >>>> And 0x%02x doesn't ? > >>> > >>> Ah, it should indeed work, 0 is actually a flag here and part of the field > >>> width or precision. I can use that in v4. > >> > >> Or %#04x, even shorter! > > > > That's different though. > > > > printf("0x%2.2x\n", 0); -> 0x00 > > printf("0x%2.2x\n", 1); -> 0x01 > > printf("0x%2.2x\n", 42); -> 0x2a > > > > printf("0x%02x\n", 0); -> 0x00 > > printf("0x%02x\n", 1); -> 0x01 > > printf("0x%02x\n", 42); -> 0x2a > > > > printf("%#2.2x\n", 0); -> 00 > > printf("%#2.2x\n", 1); -> 0x01 > > printf("%#2.2x\n", 42); -> 0x2a > > > > printf("%#02x\n", 0); -> 00 > > printf("%#02x\n", 1); -> 0x1 > > printf("%#02x\n", 42); -> 0x2a > > The length should be 4 there, not 2. But even after fixing that, the 0 > case is printed wrong. Interesting, I haven't noticed that before. I > wonder why it behaves that way... It's documented as such: # The value should be converted to an "alternate form". [...] For x and X conversions, a nonzero result has the string "0x" (or "0X" for X conversions) prepended to it. [...] > 0000 > 0x01 > 0x2a