| Message ID | 20190402171309.6447-5-jacopo@jmondi.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, Thank you for the patch. On Tue, Apr 02, 2019 at 07:13:00PM +0200, Jacopo Mondi wrote: > Cache the sensor maximum size and the media bus code used to produce > it. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > src/libcamera/pipeline/ipu3/ipu3.cpp | 47 ++++++++++++++++++++++++++++ > 1 file changed, 47 insertions(+) > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index a870b325f4b3..1e315048738b 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -8,6 +8,8 @@ > #include <memory> > #include <vector> > > +#include <linux/media-bus-format.h> > + > #include <libcamera/camera.h> > #include <libcamera/request.h> > #include <libcamera/stream.h> > @@ -24,6 +26,22 @@ namespace libcamera { > > LOG_DEFINE_CATEGORY(IPU3) > > +static int mediaBusToCIO2Format(unsigned int code) I wonder why I hadn't thought about this before, would it make sense to move this function to the CIO2Device class, make it static, and rename it to mediaBusToPixelFormat (or just mediaBusToFormat) ? Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > +{ > + switch (code) { > + case MEDIA_BUS_FMT_SBGGR10_1X10: > + return V4L2_PIX_FMT_IPU3_SBGGR10; > + case MEDIA_BUS_FMT_SGBRG10_1X10: > + return V4L2_PIX_FMT_IPU3_SGBRG10; > + case MEDIA_BUS_FMT_SGRBG10_1X10: > + return V4L2_PIX_FMT_IPU3_SGRBG10; > + case MEDIA_BUS_FMT_SRGGB10_1X10: > + return V4L2_PIX_FMT_IPU3_SRGGB10; > + default: > + return -EINVAL; > + } > +} > + > class CIO2Device > { > public: > @@ -44,6 +62,10 @@ public: > V4L2Device *output_; > V4L2Subdevice *csi2_; > V4L2Subdevice *sensor_; > + > + /* Maximum sizes and the mbus code used to produce them. */ > + unsigned int mbusCode_; > + Size maxSize_; > }; > > class PipelineHandlerIPU3 : public PipelineHandler > @@ -442,6 +464,10 @@ int CIO2Device::init(const MediaDevice *media, unsigned int index) > return ret; > > /* > + * Now that we're sure a sensor subdevice is connected, make sure it > + * produces at least one image format compatible with CIO2 requirements > + * and cache the camera maximum size. > + * > * \todo Define when to open and close video device nodes, as they > * might impact on power consumption. > */ > @@ -450,6 +476,27 @@ int CIO2Device::init(const MediaDevice *media, unsigned int index) > if (ret) > return ret; > > + for (auto it : sensor_->formats(0)) { > + int mbusCode = mediaBusToCIO2Format(it.first); > + if (mbusCode < 0) > + continue; > + > + for (const SizeRange &size : it.second) { > + if (maxSize_.width < size.maxWidth && > + maxSize_.height < size.maxHeight) { > + maxSize_.width = size.maxWidth; > + maxSize_.height = size.maxHeight; > + mbusCode_ = mbusCode; > + } > + } > + } > + if (maxSize_.width == 0) { > + LOG(IPU3, Info) << "Sensor '" << sensor_->entityName() > + << "' detected, but no supported image format " > + << " found: skip camera creation"; > + return -ENODEV; > + } > + > csi2_ = new V4L2Subdevice(csi2Entity); > ret = csi2_->open(); > if (ret)
Hi Laurent, On Tue, Apr 02, 2019 at 08:22:35PM +0300, Laurent Pinchart wrote: > Hi Jacopo, > > Thank you for the patch. > > On Tue, Apr 02, 2019 at 07:13:00PM +0200, Jacopo Mondi wrote: > > Cache the sensor maximum size and the media bus code used to produce > > it. > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > src/libcamera/pipeline/ipu3/ipu3.cpp | 47 ++++++++++++++++++++++++++++ > > 1 file changed, 47 insertions(+) > > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > > index a870b325f4b3..1e315048738b 100644 > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > > @@ -8,6 +8,8 @@ > > #include <memory> > > #include <vector> > > > > +#include <linux/media-bus-format.h> > > + > > #include <libcamera/camera.h> > > #include <libcamera/request.h> > > #include <libcamera/stream.h> > > @@ -24,6 +26,22 @@ namespace libcamera { > > > > LOG_DEFINE_CATEGORY(IPU3) > > > > +static int mediaBusToCIO2Format(unsigned int code) > > I wonder why I hadn't thought about this before, would it make sense to > move this function to the CIO2Device class, make it static, and rename > it to mediaBusToPixelFormat (or just mediaBusToFormat) ? > It might make sense, I guess. > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > With or without the above change? > > +{ > > + switch (code) { > > + case MEDIA_BUS_FMT_SBGGR10_1X10: > > + return V4L2_PIX_FMT_IPU3_SBGGR10; > > + case MEDIA_BUS_FMT_SGBRG10_1X10: > > + return V4L2_PIX_FMT_IPU3_SGBRG10; > > + case MEDIA_BUS_FMT_SGRBG10_1X10: > > + return V4L2_PIX_FMT_IPU3_SGRBG10; > > + case MEDIA_BUS_FMT_SRGGB10_1X10: > > + return V4L2_PIX_FMT_IPU3_SRGGB10; > > + default: > > + return -EINVAL; > > + } > > +} > > + > > class CIO2Device > > { > > public: > > @@ -44,6 +62,10 @@ public: > > V4L2Device *output_; > > V4L2Subdevice *csi2_; > > V4L2Subdevice *sensor_; > > + > > + /* Maximum sizes and the mbus code used to produce them. */ > > + unsigned int mbusCode_; > > + Size maxSize_; > > }; > > > > class PipelineHandlerIPU3 : public PipelineHandler > > @@ -442,6 +464,10 @@ int CIO2Device::init(const MediaDevice *media, unsigned int index) > > return ret; > > > > /* > > + * Now that we're sure a sensor subdevice is connected, make sure it > > + * produces at least one image format compatible with CIO2 requirements > > + * and cache the camera maximum size. > > + * > > * \todo Define when to open and close video device nodes, as they > > * might impact on power consumption. > > */ > > @@ -450,6 +476,27 @@ int CIO2Device::init(const MediaDevice *media, unsigned int index) > > if (ret) > > return ret; > > > > + for (auto it : sensor_->formats(0)) { > > + int mbusCode = mediaBusToCIO2Format(it.first); > > + if (mbusCode < 0) > > + continue; > > + > > + for (const SizeRange &size : it.second) { > > + if (maxSize_.width < size.maxWidth && > > + maxSize_.height < size.maxHeight) { > > + maxSize_.width = size.maxWidth; > > + maxSize_.height = size.maxHeight; > > + mbusCode_ = mbusCode; > > + } > > + } > > + } > > + if (maxSize_.width == 0) { > > + LOG(IPU3, Info) << "Sensor '" << sensor_->entityName() > > + << "' detected, but no supported image format " > > + << " found: skip camera creation"; > > + return -ENODEV; > > + } > > + > > csi2_ = new V4L2Subdevice(csi2Entity); > > ret = csi2_->open(); > > if (ret) > > -- > Regards, > > Laurent Pinchart
Hi Jacopo, On Tue, Apr 02, 2019 at 07:29:29PM +0200, Jacopo Mondi wrote: > On Tue, Apr 02, 2019 at 08:22:35PM +0300, Laurent Pinchart wrote: > > On Tue, Apr 02, 2019 at 07:13:00PM +0200, Jacopo Mondi wrote: > >> Cache the sensor maximum size and the media bus code used to produce > >> it. > >> > >> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > >> --- > >> src/libcamera/pipeline/ipu3/ipu3.cpp | 47 ++++++++++++++++++++++++++++ > >> 1 file changed, 47 insertions(+) > >> > >> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > >> index a870b325f4b3..1e315048738b 100644 > >> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > >> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > >> @@ -8,6 +8,8 @@ > >> #include <memory> > >> #include <vector> > >> > >> +#include <linux/media-bus-format.h> > >> + > >> #include <libcamera/camera.h> > >> #include <libcamera/request.h> > >> #include <libcamera/stream.h> > >> @@ -24,6 +26,22 @@ namespace libcamera { > >> > >> LOG_DEFINE_CATEGORY(IPU3) > >> > >> +static int mediaBusToCIO2Format(unsigned int code) > > > > I wonder why I hadn't thought about this before, would it make sense to > > move this function to the CIO2Device class, make it static, and rename > > it to mediaBusToPixelFormat (or just mediaBusToFormat) ? > > It might make sense, I guess. > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > With or without the above change? Either, but if you can make the change it's even better :-) > >> +{ > >> + switch (code) { > >> + case MEDIA_BUS_FMT_SBGGR10_1X10: > >> + return V4L2_PIX_FMT_IPU3_SBGGR10; > >> + case MEDIA_BUS_FMT_SGBRG10_1X10: > >> + return V4L2_PIX_FMT_IPU3_SGBRG10; > >> + case MEDIA_BUS_FMT_SGRBG10_1X10: > >> + return V4L2_PIX_FMT_IPU3_SGRBG10; > >> + case MEDIA_BUS_FMT_SRGGB10_1X10: > >> + return V4L2_PIX_FMT_IPU3_SRGGB10; > >> + default: > >> + return -EINVAL; > >> + } > >> +} > >> + > >> class CIO2Device > >> { > >> public: > >> @@ -44,6 +62,10 @@ public: > >> V4L2Device *output_; > >> V4L2Subdevice *csi2_; > >> V4L2Subdevice *sensor_; > >> + > >> + /* Maximum sizes and the mbus code used to produce them. */ > >> + unsigned int mbusCode_; > >> + Size maxSize_; > >> }; > >> > >> class PipelineHandlerIPU3 : public PipelineHandler > >> @@ -442,6 +464,10 @@ int CIO2Device::init(const MediaDevice *media, unsigned int index) > >> return ret; > >> > >> /* > >> + * Now that we're sure a sensor subdevice is connected, make sure it > >> + * produces at least one image format compatible with CIO2 requirements > >> + * and cache the camera maximum size. > >> + * > >> * \todo Define when to open and close video device nodes, as they > >> * might impact on power consumption. > >> */ > >> @@ -450,6 +476,27 @@ int CIO2Device::init(const MediaDevice *media, unsigned int index) > >> if (ret) > >> return ret; > >> > >> + for (auto it : sensor_->formats(0)) { > >> + int mbusCode = mediaBusToCIO2Format(it.first); > >> + if (mbusCode < 0) > >> + continue; > >> + > >> + for (const SizeRange &size : it.second) { > >> + if (maxSize_.width < size.maxWidth && > >> + maxSize_.height < size.maxHeight) { > >> + maxSize_.width = size.maxWidth; > >> + maxSize_.height = size.maxHeight; > >> + mbusCode_ = mbusCode; > >> + } > >> + } > >> + } > >> + if (maxSize_.width == 0) { > >> + LOG(IPU3, Info) << "Sensor '" << sensor_->entityName() > >> + << "' detected, but no supported image format " > >> + << " found: skip camera creation"; > >> + return -ENODEV; > >> + } > >> + > >> csi2_ = new V4L2Subdevice(csi2Entity); > >> ret = csi2_->open(); > >> if (ret)
Hi Jacopo, Thanks for your patch. On 2019-04-02 19:13:00 +0200, Jacopo Mondi wrote: > Cache the sensor maximum size and the media bus code used to produce > it. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> With or without addressing Laurents comment about moving mediaBusToCIO2Format(), Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> I actually prefer it the way it is, a file local static helper function, but I don't feel strongly about it :-) > --- > src/libcamera/pipeline/ipu3/ipu3.cpp | 47 ++++++++++++++++++++++++++++ > 1 file changed, 47 insertions(+) > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index a870b325f4b3..1e315048738b 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -8,6 +8,8 @@ > #include <memory> > #include <vector> > > +#include <linux/media-bus-format.h> > + > #include <libcamera/camera.h> > #include <libcamera/request.h> > #include <libcamera/stream.h> > @@ -24,6 +26,22 @@ namespace libcamera { > > LOG_DEFINE_CATEGORY(IPU3) > > +static int mediaBusToCIO2Format(unsigned int code) > +{ > + switch (code) { > + case MEDIA_BUS_FMT_SBGGR10_1X10: > + return V4L2_PIX_FMT_IPU3_SBGGR10; > + case MEDIA_BUS_FMT_SGBRG10_1X10: > + return V4L2_PIX_FMT_IPU3_SGBRG10; > + case MEDIA_BUS_FMT_SGRBG10_1X10: > + return V4L2_PIX_FMT_IPU3_SGRBG10; > + case MEDIA_BUS_FMT_SRGGB10_1X10: > + return V4L2_PIX_FMT_IPU3_SRGGB10; > + default: > + return -EINVAL; > + } > +} > + > class CIO2Device > { > public: > @@ -44,6 +62,10 @@ public: > V4L2Device *output_; > V4L2Subdevice *csi2_; > V4L2Subdevice *sensor_; > + > + /* Maximum sizes and the mbus code used to produce them. */ > + unsigned int mbusCode_; > + Size maxSize_; > }; > > class PipelineHandlerIPU3 : public PipelineHandler > @@ -442,6 +464,10 @@ int CIO2Device::init(const MediaDevice *media, unsigned int index) > return ret; > > /* > + * Now that we're sure a sensor subdevice is connected, make sure it > + * produces at least one image format compatible with CIO2 requirements > + * and cache the camera maximum size. > + * > * \todo Define when to open and close video device nodes, as they > * might impact on power consumption. > */ > @@ -450,6 +476,27 @@ int CIO2Device::init(const MediaDevice *media, unsigned int index) > if (ret) > return ret; > > + for (auto it : sensor_->formats(0)) { > + int mbusCode = mediaBusToCIO2Format(it.first); > + if (mbusCode < 0) > + continue; > + > + for (const SizeRange &size : it.second) { > + if (maxSize_.width < size.maxWidth && > + maxSize_.height < size.maxHeight) { > + maxSize_.width = size.maxWidth; > + maxSize_.height = size.maxHeight; > + mbusCode_ = mbusCode; > + } > + } > + } > + if (maxSize_.width == 0) { > + LOG(IPU3, Info) << "Sensor '" << sensor_->entityName() > + << "' detected, but no supported image format " > + << " found: skip camera creation"; > + return -ENODEV; > + } > + > csi2_ = new V4L2Subdevice(csi2Entity); > ret = csi2_->open(); > if (ret) > -- > 2.21.0 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp index a870b325f4b3..1e315048738b 100644 --- a/src/libcamera/pipeline/ipu3/ipu3.cpp +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp @@ -8,6 +8,8 @@ #include <memory> #include <vector> +#include <linux/media-bus-format.h> + #include <libcamera/camera.h> #include <libcamera/request.h> #include <libcamera/stream.h> @@ -24,6 +26,22 @@ namespace libcamera { LOG_DEFINE_CATEGORY(IPU3) +static int mediaBusToCIO2Format(unsigned int code) +{ + switch (code) { + case MEDIA_BUS_FMT_SBGGR10_1X10: + return V4L2_PIX_FMT_IPU3_SBGGR10; + case MEDIA_BUS_FMT_SGBRG10_1X10: + return V4L2_PIX_FMT_IPU3_SGBRG10; + case MEDIA_BUS_FMT_SGRBG10_1X10: + return V4L2_PIX_FMT_IPU3_SGRBG10; + case MEDIA_BUS_FMT_SRGGB10_1X10: + return V4L2_PIX_FMT_IPU3_SRGGB10; + default: + return -EINVAL; + } +} + class CIO2Device { public: @@ -44,6 +62,10 @@ public: V4L2Device *output_; V4L2Subdevice *csi2_; V4L2Subdevice *sensor_; + + /* Maximum sizes and the mbus code used to produce them. */ + unsigned int mbusCode_; + Size maxSize_; }; class PipelineHandlerIPU3 : public PipelineHandler @@ -442,6 +464,10 @@ int CIO2Device::init(const MediaDevice *media, unsigned int index) return ret; /* + * Now that we're sure a sensor subdevice is connected, make sure it + * produces at least one image format compatible with CIO2 requirements + * and cache the camera maximum size. + * * \todo Define when to open and close video device nodes, as they * might impact on power consumption. */ @@ -450,6 +476,27 @@ int CIO2Device::init(const MediaDevice *media, unsigned int index) if (ret) return ret; + for (auto it : sensor_->formats(0)) { + int mbusCode = mediaBusToCIO2Format(it.first); + if (mbusCode < 0) + continue; + + for (const SizeRange &size : it.second) { + if (maxSize_.width < size.maxWidth && + maxSize_.height < size.maxHeight) { + maxSize_.width = size.maxWidth; + maxSize_.height = size.maxHeight; + mbusCode_ = mbusCode; + } + } + } + if (maxSize_.width == 0) { + LOG(IPU3, Info) << "Sensor '" << sensor_->entityName() + << "' detected, but no supported image format " + << " found: skip camera creation"; + return -ENODEV; + } + csi2_ = new V4L2Subdevice(csi2Entity); ret = csi2_->open(); if (ret)
Cache the sensor maximum size and the media bus code used to produce it. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- src/libcamera/pipeline/ipu3/ipu3.cpp | 47 ++++++++++++++++++++++++++++ 1 file changed, 47 insertions(+)