| Message ID | 20190326083902.26121-9-jacopo@jmondi.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, Thank you for the patch. On Tue, Mar 26, 2019 at 09:38:51AM +0100, Jacopo Mondi wrote: > When creating a camera, make sure a the image sensor provides images in > a format compatible with IPU3 CIO2 unit requirements and cache the > minimum and maximum camera sizes. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > src/libcamera/pipeline/ipu3/ipu3.cpp | 56 +++++++++++++++++++++++++++- > 1 file changed, 54 insertions(+), 2 deletions(-) > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index 55489c31df2d..d42c81273cc6 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -8,11 +8,14 @@ > #include <memory> > #include <vector> > > +#include <linux/media-bus-format.h> > + > #include <libcamera/camera.h> > #include <libcamera/request.h> > #include <libcamera/stream.h> > > #include "device_enumerator.h" > +#include "geometry.h" > #include "log.h" > #include "media_device.h" > #include "pipeline_handler.h" > @@ -24,6 +27,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 PipelineHandlerIPU3 : public PipelineHandler > { > public: > @@ -70,6 +89,9 @@ private: > V4L2Subdevice *sensor_; > > Stream stream_; > + > + /* Maximum sizes and the mbus code used to produce them. */ > + std::pair<unsigned int, SizeRange> maxSizes_; The use of std::pair here makes the code below use .first and .second, which are not very readable. I think it would be better to store the pixel format and max size in two separate fields, of unsigned int and SizeRange types respectively. Or, now that I think about it, as you only care about the maximum size, maybe adding a Size structure to geometry.h would be a good idea. > }; > > IPU3CameraData *cameraData(const Camera *camera) > @@ -404,18 +426,48 @@ void PipelineHandlerIPU3::registerCameras() > if (ret) > continue; > > - data->cio2_->bufferReady.connect(data.get(), &IPU3CameraData::bufferReady); > - > + /* > + * Make sure the sensor produces at least one image format > + * compatible with IPU3 CIO2 requirements and cache the camera > + * maximum sizes. > + */ > data->sensor_ = new V4L2Subdevice(sensor); > ret = data->sensor_->open(); > if (ret) > continue; > > + for (auto it : data->sensor_->formats(0)) { > + int mbusCode = mediaBusToCIO2Format(it.first); > + if (mbusCode < 0) > + continue; > + > + for (const SizeRange &size : it.second) { > + SizeRange &maxSize = data->maxSizes_.second; > + > + if (maxSize.maxWidth < size.maxWidth && > + maxSize.maxHeight < size.maxHeight) { > + maxSize.maxWidth = size.maxWidth; > + maxSize.maxHeight = size.maxHeight; > + data->maxSizes_.first = mbusCode; > + } > + } > + } One blank line ? > + if (data->maxSizes_.second.maxWidth == 0) { > + LOG(IPU3, Info) > + << "Sensor '" << data->sensor_->entityName() > + << "' detected, but no supported image format " > + << " found: skip camera creation"; > + continue; > + } > + > data->csi2_ = new V4L2Subdevice(csi2); > ret = data->csi2_->open(); > if (ret) > continue; > > + data->cio2_->bufferReady.connect(data.get(), > + &IPU3CameraData::bufferReady); > + This seems unrelated to this patch, is it needed ? > registerCamera(std::move(camera), std::move(data)); > > LOG(IPU3, Info)
Hi Laurent, On Tue, Apr 02, 2019 at 10:27:17AM +0300, Laurent Pinchart wrote: > Hi Jacopo, > > Thank you for the patch. > > On Tue, Mar 26, 2019 at 09:38:51AM +0100, Jacopo Mondi wrote: > > When creating a camera, make sure a the image sensor provides images in > > a format compatible with IPU3 CIO2 unit requirements and cache the > > minimum and maximum camera sizes. > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > src/libcamera/pipeline/ipu3/ipu3.cpp | 56 +++++++++++++++++++++++++++- > > 1 file changed, 54 insertions(+), 2 deletions(-) > > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > > index 55489c31df2d..d42c81273cc6 100644 > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > > @@ -8,11 +8,14 @@ > > #include <memory> > > #include <vector> > > > > +#include <linux/media-bus-format.h> > > + > > #include <libcamera/camera.h> > > #include <libcamera/request.h> > > #include <libcamera/stream.h> > > > > #include "device_enumerator.h" > > +#include "geometry.h" > > #include "log.h" > > #include "media_device.h" > > #include "pipeline_handler.h" > > @@ -24,6 +27,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 PipelineHandlerIPU3 : public PipelineHandler > > { > > public: > > @@ -70,6 +89,9 @@ private: > > V4L2Subdevice *sensor_; > > > > Stream stream_; > > + > > + /* Maximum sizes and the mbus code used to produce them. */ > > + std::pair<unsigned int, SizeRange> maxSizes_; > > The use of std::pair here makes the code below use .first and .second, > which are not very readable. I think it would be better to store the > pixel format and max size in two separate fields, of unsigned int and > SizeRange types respectively. Or, now that I think about it, as you only > care about the maximum size, maybe adding a Size structure to geometry.h > would be a good idea. > I see... I'll ponder about it. Adding new stuff it's always a burden because of documentation and such, but I get your point... > > }; > > > > IPU3CameraData *cameraData(const Camera *camera) > > @@ -404,18 +426,48 @@ void PipelineHandlerIPU3::registerCameras() > > if (ret) > > continue; > > > > - data->cio2_->bufferReady.connect(data.get(), &IPU3CameraData::bufferReady); > > - > > + /* > > + * Make sure the sensor produces at least one image format > > + * compatible with IPU3 CIO2 requirements and cache the camera > > + * maximum sizes. > > + */ > > data->sensor_ = new V4L2Subdevice(sensor); > > ret = data->sensor_->open(); > > if (ret) > > continue; > > > > + for (auto it : data->sensor_->formats(0)) { > > + int mbusCode = mediaBusToCIO2Format(it.first); > > + if (mbusCode < 0) > > + continue; > > + > > + for (const SizeRange &size : it.second) { > > + SizeRange &maxSize = data->maxSizes_.second; > > + > > + if (maxSize.maxWidth < size.maxWidth && > > + maxSize.maxHeight < size.maxHeight) { > > + maxSize.maxWidth = size.maxWidth; > > + maxSize.maxHeight = size.maxHeight; > > + data->maxSizes_.first = mbusCode; > > + } > > + } > > + } > > One blank line ? Not sure, as this is an error condition, I like it better without an empty line. > > > + if (data->maxSizes_.second.maxWidth == 0) { > > + LOG(IPU3, Info) > > + << "Sensor '" << data->sensor_->entityName() > > + << "' detected, but no supported image format " > > + << " found: skip camera creation"; > > + continue; > > + } > > + > > data->csi2_ = new V4L2Subdevice(csi2); > > ret = data->csi2_->open(); > > if (ret) > > continue; > > > > + data->cio2_->bufferReady.connect(data.get(), > > + &IPU3CameraData::bufferReady); > > + > > This seems unrelated to this patch, is it needed ? please see: > > - data->cio2_->bufferReady.connect(data.get(), &IPU3CameraData::bufferReady); A few lines above here. Thanks j > > > registerCamera(std::move(camera), std::move(data)); > > > > LOG(IPU3, Info) > > -- > Regards, > > Laurent Pinchart
Hi Jacopo, On Tue, Apr 02, 2019 at 10:26:07AM +0200, Jacopo Mondi wrote: > On Tue, Apr 02, 2019 at 10:27:17AM +0300, Laurent Pinchart wrote: > > On Tue, Mar 26, 2019 at 09:38:51AM +0100, Jacopo Mondi wrote: > >> When creating a camera, make sure a the image sensor provides images in > >> a format compatible with IPU3 CIO2 unit requirements and cache the > >> minimum and maximum camera sizes. > >> > >> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > >> --- > >> src/libcamera/pipeline/ipu3/ipu3.cpp | 56 +++++++++++++++++++++++++++- > >> 1 file changed, 54 insertions(+), 2 deletions(-) > >> > >> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > >> index 55489c31df2d..d42c81273cc6 100644 > >> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > >> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > >> @@ -8,11 +8,14 @@ > >> #include <memory> > >> #include <vector> > >> > >> +#include <linux/media-bus-format.h> > >> + > >> #include <libcamera/camera.h> > >> #include <libcamera/request.h> > >> #include <libcamera/stream.h> > >> > >> #include "device_enumerator.h" > >> +#include "geometry.h" > >> #include "log.h" > >> #include "media_device.h" > >> #include "pipeline_handler.h" > >> @@ -24,6 +27,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 PipelineHandlerIPU3 : public PipelineHandler > >> { > >> public: > >> @@ -70,6 +89,9 @@ private: > >> V4L2Subdevice *sensor_; > >> > >> Stream stream_; > >> + > >> + /* Maximum sizes and the mbus code used to produce them. */ > >> + std::pair<unsigned int, SizeRange> maxSizes_; > > > > The use of std::pair here makes the code below use .first and .second, > > which are not very readable. I think it would be better to store the > > pixel format and max size in two separate fields, of unsigned int and > > SizeRange types respectively. Or, now that I think about it, as you only > > care about the maximum size, maybe adding a Size structure to geometry.h > > would be a good idea. > > > > I see... I'll ponder about it. Adding new stuff it's always a burden > because of documentation and such, but I get your point... I know it can be a bit painful :-( A Size structure should hopefully be simple though. > >> }; > >> > >> IPU3CameraData *cameraData(const Camera *camera) > >> @@ -404,18 +426,48 @@ void PipelineHandlerIPU3::registerCameras() > >> if (ret) > >> continue; > >> > >> - data->cio2_->bufferReady.connect(data.get(), &IPU3CameraData::bufferReady); > >> - > >> + /* > >> + * Make sure the sensor produces at least one image format > >> + * compatible with IPU3 CIO2 requirements and cache the camera > >> + * maximum sizes. > >> + */ > >> data->sensor_ = new V4L2Subdevice(sensor); > >> ret = data->sensor_->open(); > >> if (ret) > >> continue; > >> > >> + for (auto it : data->sensor_->formats(0)) { > >> + int mbusCode = mediaBusToCIO2Format(it.first); > >> + if (mbusCode < 0) > >> + continue; > >> + > >> + for (const SizeRange &size : it.second) { > >> + SizeRange &maxSize = data->maxSizes_.second; > >> + > >> + if (maxSize.maxWidth < size.maxWidth && > >> + maxSize.maxHeight < size.maxHeight) { > >> + maxSize.maxWidth = size.maxWidth; > >> + maxSize.maxHeight = size.maxHeight; > >> + data->maxSizes_.first = mbusCode; > >> + } > >> + } > >> + } > > > > One blank line ? > > Not sure, as this is an error condition, I like it better without an > empty line. There are generally empty lines after a for loop of if block, but it's your code :-) > >> + if (data->maxSizes_.second.maxWidth == 0) { > >> + LOG(IPU3, Info) > >> + << "Sensor '" << data->sensor_->entityName() > >> + << "' detected, but no supported image format " > >> + << " found: skip camera creation"; > >> + continue; > >> + } > >> + > >> data->csi2_ = new V4L2Subdevice(csi2); > >> ret = data->csi2_->open(); > >> if (ret) > >> continue; > >> > >> + data->cio2_->bufferReady.connect(data.get(), > >> + &IPU3CameraData::bufferReady); > >> + > > > > This seems unrelated to this patch, is it needed ? > > please see: > >> - data->cio2_->bufferReady.connect(data.get(), &IPU3CameraData::bufferReady); > > A few lines above here. I know, what I meant is that moving the code seems unrelated, as you don't touch it (apart from wrapping the line). > >> registerCamera(std::move(camera), std::move(data)); > >> > >> LOG(IPU3, Info)
Hi Laurent, On Tue, Apr 02, 2019 at 12:11:59PM +0300, Laurent Pinchart wrote: > Hi Jacopo, > > On Tue, Apr 02, 2019 at 10:26:07AM +0200, Jacopo Mondi wrote: > > On Tue, Apr 02, 2019 at 10:27:17AM +0300, Laurent Pinchart wrote: > > > On Tue, Mar 26, 2019 at 09:38:51AM +0100, Jacopo Mondi wrote: > > >> When creating a camera, make sure a the image sensor provides images in > > >> a format compatible with IPU3 CIO2 unit requirements and cache the > > >> minimum and maximum camera sizes. > > >> > > >> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > >> --- > > >> src/libcamera/pipeline/ipu3/ipu3.cpp | 56 +++++++++++++++++++++++++++- > > >> 1 file changed, 54 insertions(+), 2 deletions(-) > > >> > > >> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > > >> index 55489c31df2d..d42c81273cc6 100644 > > >> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > > >> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > > >> @@ -8,11 +8,14 @@ > > >> #include <memory> > > >> #include <vector> > > >> > > >> +#include <linux/media-bus-format.h> > > >> + > > >> #include <libcamera/camera.h> > > >> #include <libcamera/request.h> > > >> #include <libcamera/stream.h> > > >> > > >> #include "device_enumerator.h" > > >> +#include "geometry.h" > > >> #include "log.h" > > >> #include "media_device.h" > > >> #include "pipeline_handler.h" > > >> @@ -24,6 +27,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 PipelineHandlerIPU3 : public PipelineHandler > > >> { > > >> public: > > >> @@ -70,6 +89,9 @@ private: > > >> V4L2Subdevice *sensor_; > > >> > > >> Stream stream_; > > >> + > > >> + /* Maximum sizes and the mbus code used to produce them. */ > > >> + std::pair<unsigned int, SizeRange> maxSizes_; > > > > > > The use of std::pair here makes the code below use .first and .second, > > > which are not very readable. I think it would be better to store the > > > pixel format and max size in two separate fields, of unsigned int and > > > SizeRange types respectively. Or, now that I think about it, as you only > > > care about the maximum size, maybe adding a Size structure to geometry.h > > > would be a good idea. > > > > > > > I see... I'll ponder about it. Adding new stuff it's always a burden > > because of documentation and such, but I get your point... > > I know it can be a bit painful :-( A Size structure should hopefully be > simple though. > > > >> }; > > >> > > >> IPU3CameraData *cameraData(const Camera *camera) > > >> @@ -404,18 +426,48 @@ void PipelineHandlerIPU3::registerCameras() > > >> if (ret) > > >> continue; > > >> > > >> - data->cio2_->bufferReady.connect(data.get(), &IPU3CameraData::bufferReady); > > >> - > > >> + /* > > >> + * Make sure the sensor produces at least one image format > > >> + * compatible with IPU3 CIO2 requirements and cache the camera > > >> + * maximum sizes. > > >> + */ > > >> data->sensor_ = new V4L2Subdevice(sensor); > > >> ret = data->sensor_->open(); > > >> if (ret) > > >> continue; > > >> > > >> + for (auto it : data->sensor_->formats(0)) { > > >> + int mbusCode = mediaBusToCIO2Format(it.first); > > >> + if (mbusCode < 0) > > >> + continue; > > >> + > > >> + for (const SizeRange &size : it.second) { > > >> + SizeRange &maxSize = data->maxSizes_.second; > > >> + > > >> + if (maxSize.maxWidth < size.maxWidth && > > >> + maxSize.maxHeight < size.maxHeight) { > > >> + maxSize.maxWidth = size.maxWidth; > > >> + maxSize.maxHeight = size.maxHeight; > > >> + data->maxSizes_.first = mbusCode; > > >> + } > > >> + } > > >> + } > > > > > > One blank line ? > > > > Not sure, as this is an error condition, I like it better without an > > empty line. > > There are generally empty lines after a for loop of if block, but it's > your code :-) > > > >> + if (data->maxSizes_.second.maxWidth == 0) { > > >> + LOG(IPU3, Info) > > >> + << "Sensor '" << data->sensor_->entityName() > > >> + << "' detected, but no supported image format " > > >> + << " found: skip camera creation"; > > >> + continue; > > >> + } > > >> + > > >> data->csi2_ = new V4L2Subdevice(csi2); > > >> ret = data->csi2_->open(); > > >> if (ret) > > >> continue; > > >> > > >> + data->cio2_->bufferReady.connect(data.get(), > > >> + &IPU3CameraData::bufferReady); > > >> + > > > > > > This seems unrelated to this patch, is it needed ? > > > > please see: > > >> - data->cio2_->bufferReady.connect(data.get(), &IPU3CameraData::bufferReady); > > > > A few lines above here. > > I know, what I meant is that moving the code seems unrelated, as you > don't touch it (apart from wrapping the line). > Ah, got it. I moved it here because I want to connect the signal only after we have successfully validated the sensor provided formats (introduced in this patch), so I don't think it's unrelated, isn't it? Thanks j > > >> registerCamera(std::move(camera), std::move(data)); > > >> > > >> LOG(IPU3, Info) > > -- > Regards, > > Laurent Pinchart
Hi Jacopo, On Tue, Apr 02, 2019 at 11:23:30AM +0200, Jacopo Mondi wrote: > On Tue, Apr 02, 2019 at 12:11:59PM +0300, Laurent Pinchart wrote: > > On Tue, Apr 02, 2019 at 10:26:07AM +0200, Jacopo Mondi wrote: > >> On Tue, Apr 02, 2019 at 10:27:17AM +0300, Laurent Pinchart wrote: > >>> On Tue, Mar 26, 2019 at 09:38:51AM +0100, Jacopo Mondi wrote: > >>>> When creating a camera, make sure a the image sensor provides images in > >>>> a format compatible with IPU3 CIO2 unit requirements and cache the > >>>> minimum and maximum camera sizes. > >>>> > >>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > >>>> --- > >>>> src/libcamera/pipeline/ipu3/ipu3.cpp | 56 +++++++++++++++++++++++++++- > >>>> 1 file changed, 54 insertions(+), 2 deletions(-) > >>>> > >>>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > >>>> index 55489c31df2d..d42c81273cc6 100644 > >>>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > >>>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > >>>> @@ -8,11 +8,14 @@ > >>>> #include <memory> > >>>> #include <vector> > >>>> > >>>> +#include <linux/media-bus-format.h> > >>>> + > >>>> #include <libcamera/camera.h> > >>>> #include <libcamera/request.h> > >>>> #include <libcamera/stream.h> > >>>> > >>>> #include "device_enumerator.h" > >>>> +#include "geometry.h" > >>>> #include "log.h" > >>>> #include "media_device.h" > >>>> #include "pipeline_handler.h" > >>>> @@ -24,6 +27,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 PipelineHandlerIPU3 : public PipelineHandler > >>>> { > >>>> public: > >>>> @@ -70,6 +89,9 @@ private: > >>>> V4L2Subdevice *sensor_; > >>>> > >>>> Stream stream_; > >>>> + > >>>> + /* Maximum sizes and the mbus code used to produce them. */ > >>>> + std::pair<unsigned int, SizeRange> maxSizes_; > >>> > >>> The use of std::pair here makes the code below use .first and .second, > >>> which are not very readable. I think it would be better to store the > >>> pixel format and max size in two separate fields, of unsigned int and > >>> SizeRange types respectively. Or, now that I think about it, as you only > >>> care about the maximum size, maybe adding a Size structure to geometry.h > >>> would be a good idea. > >>> > >> > >> I see... I'll ponder about it. Adding new stuff it's always a burden > >> because of documentation and such, but I get your point... > > > > I know it can be a bit painful :-( A Size structure should hopefully be > > simple though. > > > >>>> }; > >>>> > >>>> IPU3CameraData *cameraData(const Camera *camera) > >>>> @@ -404,18 +426,48 @@ void PipelineHandlerIPU3::registerCameras() > >>>> if (ret) > >>>> continue; > >>>> > >>>> - data->cio2_->bufferReady.connect(data.get(), &IPU3CameraData::bufferReady); > >>>> - > >>>> + /* > >>>> + * Make sure the sensor produces at least one image format > >>>> + * compatible with IPU3 CIO2 requirements and cache the camera > >>>> + * maximum sizes. > >>>> + */ > >>>> data->sensor_ = new V4L2Subdevice(sensor); > >>>> ret = data->sensor_->open(); > >>>> if (ret) > >>>> continue; > >>>> > >>>> + for (auto it : data->sensor_->formats(0)) { > >>>> + int mbusCode = mediaBusToCIO2Format(it.first); > >>>> + if (mbusCode < 0) > >>>> + continue; > >>>> + > >>>> + for (const SizeRange &size : it.second) { > >>>> + SizeRange &maxSize = data->maxSizes_.second; > >>>> + > >>>> + if (maxSize.maxWidth < size.maxWidth && > >>>> + maxSize.maxHeight < size.maxHeight) { > >>>> + maxSize.maxWidth = size.maxWidth; > >>>> + maxSize.maxHeight = size.maxHeight; > >>>> + data->maxSizes_.first = mbusCode; > >>>> + } > >>>> + } > >>>> + } > >>> > >>> One blank line ? > >> > >> Not sure, as this is an error condition, I like it better without an > >> empty line. > > > > There are generally empty lines after a for loop of if block, but it's > > your code :-) > > > >>>> + if (data->maxSizes_.second.maxWidth == 0) { > >>>> + LOG(IPU3, Info) > >>>> + << "Sensor '" << data->sensor_->entityName() > >>>> + << "' detected, but no supported image format " > >>>> + << " found: skip camera creation"; > >>>> + continue; > >>>> + } > >>>> + > >>>> data->csi2_ = new V4L2Subdevice(csi2); > >>>> ret = data->csi2_->open(); > >>>> if (ret) > >>>> continue; > >>>> > >>>> + data->cio2_->bufferReady.connect(data.get(), > >>>> + &IPU3CameraData::bufferReady); > >>>> + > >>> > >>> This seems unrelated to this patch, is it needed ? > >> > >> please see: > >> > >>>> - data->cio2_->bufferReady.connect(data.get(), &IPU3CameraData::bufferReady); > >> > >> A few lines above here. > > > > I know, what I meant is that moving the code seems unrelated, as you > > don't touch it (apart from wrapping the line). > > > > Ah, got it. I moved it here because I want to connect the signal only > after we have successfully validated the sensor provided formats > (introduced in this patch), so I don't think it's unrelated, isn't it? My point is that there's no strict need to connect the signal only after validating the sensor formats. It's no big deal though. > >>>> registerCamera(std::move(camera), std::move(data)); > >>>> > >>>> LOG(IPU3, Info)
Hi Jacopo, Thanks for your work. On 2019-03-26 09:38:51 +0100, Jacopo Mondi wrote: > When creating a camera, make sure a the image sensor provides images in > a format compatible with IPU3 CIO2 unit requirements and cache the > minimum and maximum camera sizes. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > src/libcamera/pipeline/ipu3/ipu3.cpp | 56 +++++++++++++++++++++++++++- > 1 file changed, 54 insertions(+), 2 deletions(-) > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index 55489c31df2d..d42c81273cc6 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -8,11 +8,14 @@ > #include <memory> > #include <vector> > > +#include <linux/media-bus-format.h> > + > #include <libcamera/camera.h> > #include <libcamera/request.h> > #include <libcamera/stream.h> > > #include "device_enumerator.h" > +#include "geometry.h" > #include "log.h" > #include "media_device.h" > #include "pipeline_handler.h" > @@ -24,6 +27,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 PipelineHandlerIPU3 : public PipelineHandler > { > public: > @@ -70,6 +89,9 @@ private: > V4L2Subdevice *sensor_; > > Stream stream_; > + > + /* Maximum sizes and the mbus code used to produce them. */ > + std::pair<unsigned int, SizeRange> maxSizes_; > }; > > IPU3CameraData *cameraData(const Camera *camera) > @@ -404,18 +426,48 @@ void PipelineHandlerIPU3::registerCameras() > if (ret) > continue; > > - data->cio2_->bufferReady.connect(data.get(), &IPU3CameraData::bufferReady); > - > + /* > + * Make sure the sensor produces at least one image format > + * compatible with IPU3 CIO2 requirements and cache the camera > + * maximum sizes. > + */ > data->sensor_ = new V4L2Subdevice(sensor); > ret = data->sensor_->open(); > if (ret) > continue; > > + for (auto it : data->sensor_->formats(0)) { > + int mbusCode = mediaBusToCIO2Format(it.first); > + if (mbusCode < 0) > + continue; > + > + for (const SizeRange &size : it.second) { > + SizeRange &maxSize = data->maxSizes_.second; > + > + if (maxSize.maxWidth < size.maxWidth && > + maxSize.maxHeight < size.maxHeight) { > + maxSize.maxWidth = size.maxWidth; > + maxSize.maxHeight = size.maxHeight; > + data->maxSizes_.first = mbusCode; > + } > + } > + } > + if (data->maxSizes_.second.maxWidth == 0) { > + LOG(IPU3, Info) > + << "Sensor '" << data->sensor_->entityName() > + << "' detected, but no supported image format " > + << " found: skip camera creation"; > + continue; > + } > + Nit-pick: I would break this out into a cacheSizes() function as registerCameras() is becoming rather large. Nothing I will press tho. > data->csi2_ = new V4L2Subdevice(csi2); > ret = data->csi2_->open(); > if (ret) > continue; > > + data->cio2_->bufferReady.connect(data.get(), > + &IPU3CameraData::bufferReady); > + > registerCamera(std::move(camera), std::move(data)); > > LOG(IPU3, Info) > -- > 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 55489c31df2d..d42c81273cc6 100644 --- a/src/libcamera/pipeline/ipu3/ipu3.cpp +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp @@ -8,11 +8,14 @@ #include <memory> #include <vector> +#include <linux/media-bus-format.h> + #include <libcamera/camera.h> #include <libcamera/request.h> #include <libcamera/stream.h> #include "device_enumerator.h" +#include "geometry.h" #include "log.h" #include "media_device.h" #include "pipeline_handler.h" @@ -24,6 +27,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 PipelineHandlerIPU3 : public PipelineHandler { public: @@ -70,6 +89,9 @@ private: V4L2Subdevice *sensor_; Stream stream_; + + /* Maximum sizes and the mbus code used to produce them. */ + std::pair<unsigned int, SizeRange> maxSizes_; }; IPU3CameraData *cameraData(const Camera *camera) @@ -404,18 +426,48 @@ void PipelineHandlerIPU3::registerCameras() if (ret) continue; - data->cio2_->bufferReady.connect(data.get(), &IPU3CameraData::bufferReady); - + /* + * Make sure the sensor produces at least one image format + * compatible with IPU3 CIO2 requirements and cache the camera + * maximum sizes. + */ data->sensor_ = new V4L2Subdevice(sensor); ret = data->sensor_->open(); if (ret) continue; + for (auto it : data->sensor_->formats(0)) { + int mbusCode = mediaBusToCIO2Format(it.first); + if (mbusCode < 0) + continue; + + for (const SizeRange &size : it.second) { + SizeRange &maxSize = data->maxSizes_.second; + + if (maxSize.maxWidth < size.maxWidth && + maxSize.maxHeight < size.maxHeight) { + maxSize.maxWidth = size.maxWidth; + maxSize.maxHeight = size.maxHeight; + data->maxSizes_.first = mbusCode; + } + } + } + if (data->maxSizes_.second.maxWidth == 0) { + LOG(IPU3, Info) + << "Sensor '" << data->sensor_->entityName() + << "' detected, but no supported image format " + << " found: skip camera creation"; + continue; + } + data->csi2_ = new V4L2Subdevice(csi2); ret = data->csi2_->open(); if (ret) continue; + data->cio2_->bufferReady.connect(data.get(), + &IPU3CameraData::bufferReady); + registerCamera(std::move(camera), std::move(data)); LOG(IPU3, Info)
When creating a camera, make sure a the image sensor provides images in a format compatible with IPU3 CIO2 unit requirements and cache the minimum and maximum camera sizes. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- src/libcamera/pipeline/ipu3/ipu3.cpp | 56 +++++++++++++++++++++++++++- 1 file changed, 54 insertions(+), 2 deletions(-)