| Message ID | 20210309144457.32412-1-m.cichy@pengutronix.de |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hello Marian, On Tue, Mar 09, 2021 at 03:44:57PM +0100, Marian Cichy wrote: > V4L2Subdevice::formats() wants to enumerate all media bus codes and > frame sizes on a pad, however, the current implementation always > overwrites the SizeRange of the last mbus code, effectively returning > only this SizeRange. > > This can be even fatal, as camera_sensor is using this function to > enumerate all codes and sizes. If the last mbus code has no SizeRange > for whatever reason, the camera_sensor will error out, claiming that it > didn't found any image formats at all. What is the use case for a driver returning an mbus code without any image size associated ? Smells like a driver bug to me. > > Signed-off-by: Marian Cichy <m.cichy@pengutronix.de> > --- > src/libcamera/v4l2_subdevice.cpp | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp > index 9af1c0ab..37bafb9b 100644 > --- a/src/libcamera/v4l2_subdevice.cpp > +++ b/src/libcamera/v4l2_subdevice.cpp > @@ -354,10 +354,10 @@ V4L2Subdevice::Formats V4L2Subdevice::formats(unsigned int pad) > return {}; > } > > + std::vector<SizeRange> sizes = {}; > for (unsigned int code : enumPadCodes(pad)) { > - std::vector<SizeRange> sizes = enumPadSizes(pad, code); > - if (sizes.empty()) > - return {}; > + std::vector<SizeRange> codeSizeRange = enumPadSizes(pad, code); > + sizes.insert(sizes.end(), codeSizeRange.begin(), codeSizeRange.end()); Also, this builds 'sizes' incrementally, associating to the newly enumerated 'code' the sizes previously enumerated for other codes if I'm not mistaken. Is this intentional ? Thanks j > > const auto inserted = formats.insert({ code, sizes }); > if (!inserted.second) { > -- > 2.29.2 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
On 3/11/21 9:16 AM, Jacopo Mondi wrote: > Hello Marian, > > On Tue, Mar 09, 2021 at 03:44:57PM +0100, Marian Cichy wrote: >> V4L2Subdevice::formats() wants to enumerate all media bus codes and >> frame sizes on a pad, however, the current implementation always >> overwrites the SizeRange of the last mbus code, effectively returning >> only this SizeRange. >> >> This can be even fatal, as camera_sensor is using this function to >> enumerate all codes and sizes. If the last mbus code has no SizeRange >> for whatever reason, the camera_sensor will error out, claiming that it >> didn't found any image formats at all. > What is the use case for a driver returning an mbus code without any > image size associated ? Smells like a driver bug to me. The driver I am using has a code which is only used by the hardware-internal test pattern generator and it doesn't return a size for this code. It might still very well a driver bug though. >> Signed-off-by: Marian Cichy <m.cichy@pengutronix.de> >> --- >> src/libcamera/v4l2_subdevice.cpp | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp >> index 9af1c0ab..37bafb9b 100644 >> --- a/src/libcamera/v4l2_subdevice.cpp >> +++ b/src/libcamera/v4l2_subdevice.cpp >> @@ -354,10 +354,10 @@ V4L2Subdevice::Formats V4L2Subdevice::formats(unsigned int pad) >> return {}; >> } >> >> + std::vector<SizeRange> sizes = {}; >> for (unsigned int code : enumPadCodes(pad)) { >> - std::vector<SizeRange> sizes = enumPadSizes(pad, code); >> - if (sizes.empty()) >> - return {}; >> + std::vector<SizeRange> codeSizeRange = enumPadSizes(pad, code); >> + sizes.insert(sizes.end(), codeSizeRange.begin(), codeSizeRange.end()); > Also, this builds 'sizes' incrementally, associating to the newly > enumerated 'code' the sizes previously enumerated for other codes > if I'm not mistaken. > > Is this intentional ? Right, this merges all sizes and all codes together and is probably not wanted. The previous implementation is actually correct, expect that I fall into the empty sizes case. > > Thanks > j > >> const auto inserted = formats.insert({ code, sizes }); >> if (!inserted.second) { >> -- >> 2.29.2 >> >> _______________________________________________ >> libcamera-devel mailing list >> libcamera-devel@lists.libcamera.org >> https://lists.libcamera.org/listinfo/libcamera-devel
Hi Marian, On Thu, Mar 11, 2021 at 12:15:48PM +0100, Marian Cichy wrote: > > > On 3/11/21 9:16 AM, Jacopo Mondi wrote: > > Hello Marian, > > > > On Tue, Mar 09, 2021 at 03:44:57PM +0100, Marian Cichy wrote: > > > V4L2Subdevice::formats() wants to enumerate all media bus codes and > > > frame sizes on a pad, however, the current implementation always > > > overwrites the SizeRange of the last mbus code, effectively returning > > > only this SizeRange. > > > > > > This can be even fatal, as camera_sensor is using this function to > > > enumerate all codes and sizes. If the last mbus code has no SizeRange > > > for whatever reason, the camera_sensor will error out, claiming that it > > > didn't found any image formats at all. > > What is the use case for a driver returning an mbus code without any > > image size associated ? Smells like a driver bug to me. > > The driver I am using has a code which is only used by the hardware-internal > test pattern generator and it doesn't return a size for this code. It might > still very well a driver bug though. > I see. It might be a valid use case, even if test patterns have a size, doesn't they ? > > > Signed-off-by: Marian Cichy <m.cichy@pengutronix.de> > > > --- > > > src/libcamera/v4l2_subdevice.cpp | 6 +++--- > > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > > > diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp > > > index 9af1c0ab..37bafb9b 100644 > > > --- a/src/libcamera/v4l2_subdevice.cpp > > > +++ b/src/libcamera/v4l2_subdevice.cpp > > > @@ -354,10 +354,10 @@ V4L2Subdevice::Formats V4L2Subdevice::formats(unsigned int pad) > > > return {}; > > > } > > > > > > + std::vector<SizeRange> sizes = {}; > > > for (unsigned int code : enumPadCodes(pad)) { > > > - std::vector<SizeRange> sizes = enumPadSizes(pad, code); > > > - if (sizes.empty()) > > > - return {}; > > > + std::vector<SizeRange> codeSizeRange = enumPadSizes(pad, code); > > > + sizes.insert(sizes.end(), codeSizeRange.begin(), codeSizeRange.end()); > > Also, this builds 'sizes' incrementally, associating to the newly > > enumerated 'code' the sizes previously enumerated for other codes > > if I'm not mistaken. > > > > Is this intentional ? > > Right, this merges all sizes and all codes together and is probably not > wanted. The previous implementation is actually correct, expect that I fall > into the empty sizes case. One thing you could do is to simply 'continue' if no size is associated with a media bus code. I presume we bailed-out to be able to catch errors returned by enumPadSizes(), which in case you decide to 'continue' would be missed. Alternatively, the enumPad[Code|Size] could be changed to accept a vector of [unsigned int|SizeRange] respectively as output parameter and return an error code to be able to distinguish an error condition from an empty list of sizes. Anyway, I would first make sure if the driver behaviour is intended or not. Thanks j > > > > > Thanks > > j > > > > > const auto inserted = formats.insert({ code, sizes }); > > > if (!inserted.second) { > > > -- > > > 2.29.2 > > > > > > _______________________________________________ > > > libcamera-devel mailing list > > > libcamera-devel@lists.libcamera.org > > > https://lists.libcamera.org/listinfo/libcamera-devel >
Hello, On Thu, Mar 11, 2021 at 12:28:05PM +0100, Jacopo Mondi wrote: > On Thu, Mar 11, 2021 at 12:15:48PM +0100, Marian Cichy wrote: > > On 3/11/21 9:16 AM, Jacopo Mondi wrote: > > > On Tue, Mar 09, 2021 at 03:44:57PM +0100, Marian Cichy wrote: > > > > V4L2Subdevice::formats() wants to enumerate all media bus codes and > > > > frame sizes on a pad, however, the current implementation always > > > > overwrites the SizeRange of the last mbus code, effectively returning > > > > only this SizeRange. > > > > > > > > This can be even fatal, as camera_sensor is using this function to > > > > enumerate all codes and sizes. If the last mbus code has no SizeRange > > > > for whatever reason, the camera_sensor will error out, claiming that it > > > > didn't found any image formats at all. > > > > > > What is the use case for a driver returning an mbus code without any > > > image size associated ? Smells like a driver bug to me. > > > > The driver I am using has a code which is only used by the hardware-internal > > test pattern generator and it doesn't return a size for this code. It might > > still very well a driver bug though. > > I see. It might be a valid use case, even if test patterns have a size, > doesn't they ? A media bus code without sizes seems a bit weird. Marian, on which subdev/pad is this, and what's the code ? > > > > Signed-off-by: Marian Cichy <m.cichy@pengutronix.de> > > > > --- > > > > src/libcamera/v4l2_subdevice.cpp | 6 +++--- > > > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp > > > > index 9af1c0ab..37bafb9b 100644 > > > > --- a/src/libcamera/v4l2_subdevice.cpp > > > > +++ b/src/libcamera/v4l2_subdevice.cpp > > > > @@ -354,10 +354,10 @@ V4L2Subdevice::Formats V4L2Subdevice::formats(unsigned int pad) > > > > return {}; > > > > } > > > > > > > > + std::vector<SizeRange> sizes = {}; > > > > for (unsigned int code : enumPadCodes(pad)) { > > > > - std::vector<SizeRange> sizes = enumPadSizes(pad, code); > > > > - if (sizes.empty()) > > > > - return {}; > > > > + std::vector<SizeRange> codeSizeRange = enumPadSizes(pad, code); > > > > + sizes.insert(sizes.end(), codeSizeRange.begin(), codeSizeRange.end()); > > > > > > Also, this builds 'sizes' incrementally, associating to the newly > > > enumerated 'code' the sizes previously enumerated for other codes > > > if I'm not mistaken. > > > > > > Is this intentional ? > > > > Right, this merges all sizes and all codes together and is probably not > > wanted. The previous implementation is actually correct, expect that I fall > > into the empty sizes case. > > One thing you could do is to simply 'continue' if no size is > associated with a media bus code. I presume we bailed-out to be able > to catch errors returned by enumPadSizes(), which in case you decide > to 'continue' would be missed. > > Alternatively, the enumPad[Code|Size] could be changed to accept a > vector of [unsigned int|SizeRange] respectively as output parameter > and return an error code to be able to distinguish an error condition > from an empty list of sizes. > > Anyway, I would first make sure if the driver behaviour is intended or > not. > > > > > const auto inserted = formats.insert({ code, sizes }); > > > > if (!inserted.second) {
diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp index 9af1c0ab..37bafb9b 100644 --- a/src/libcamera/v4l2_subdevice.cpp +++ b/src/libcamera/v4l2_subdevice.cpp @@ -354,10 +354,10 @@ V4L2Subdevice::Formats V4L2Subdevice::formats(unsigned int pad) return {}; } + std::vector<SizeRange> sizes = {}; for (unsigned int code : enumPadCodes(pad)) { - std::vector<SizeRange> sizes = enumPadSizes(pad, code); - if (sizes.empty()) - return {}; + std::vector<SizeRange> codeSizeRange = enumPadSizes(pad, code); + sizes.insert(sizes.end(), codeSizeRange.begin(), codeSizeRange.end()); const auto inserted = formats.insert({ code, sizes }); if (!inserted.second) {
V4L2Subdevice::formats() wants to enumerate all media bus codes and frame sizes on a pad, however, the current implementation always overwrites the SizeRange of the last mbus code, effectively returning only this SizeRange. This can be even fatal, as camera_sensor is using this function to enumerate all codes and sizes. If the last mbus code has no SizeRange for whatever reason, the camera_sensor will error out, claiming that it didn't found any image formats at all. Signed-off-by: Marian Cichy <m.cichy@pengutronix.de> --- src/libcamera/v4l2_subdevice.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)