| Message ID | 20190415165700.22970-11-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, Thanks for your work. On 2019-04-15 19:56:59 +0300, Laurent Pinchart wrote: > Replace the manual handling of sensor formats with usage of the new > CameraSensor class. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > src/libcamera/pipeline/ipu3/ipu3.cpp | 78 +++++----------------------- > 1 file changed, 14 insertions(+), 64 deletions(-) I just love to see code melt away from pipeline handler implementations to helpers. > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index e3c79f93963e..1326a1be105f 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -15,6 +15,7 @@ > #include <libcamera/request.h> > #include <libcamera/stream.h> > > +#include "camera_sensor.h" > #include "device_enumerator.h" > #include "log.h" > #include "media_device.h" > @@ -124,11 +125,7 @@ public: > > V4L2Device *output_; > V4L2Subdevice *csi2_; > - V4L2Subdevice *sensor_; > - > - /* Maximum sizes and the mbus code used to produce them. */ > - unsigned int mbusCode_; > - Size maxSize_; > + CameraSensor *sensor_; > > BufferPool pool_; > }; > @@ -255,8 +252,9 @@ int PipelineHandlerIPU3::configureStreams(Camera *camera, > return -EINVAL; > } > > - if (cfg.width > cio2->maxSize_.width || > - cfg.height > cio2->maxSize_.height) { > + const Size &resolution = cio2->sensor_->resolution(); > + if (cfg.width > resolution.width || > + cfg.height > resolution.height) { > LOG(IPU3, Error) > << "Invalid stream size: larger than sensor resolution"; > return -EINVAL; > @@ -1036,31 +1034,11 @@ int CIO2Device::init(const MediaDevice *media, unsigned int index) > * \todo Define when to open and close video device nodes, as they > * might impact on power consumption. > */ > - sensor_ = new V4L2Subdevice(sensorEntity); > - ret = sensor_->open(); > + sensor_ = new CameraSensor(sensorEntity); > + ret = sensor_->init(); > if (ret) > return ret; > > - for (auto it : sensor_->formats(0)) { > - int mbusCode = mediaBusToFormat(it.first); > - if (mbusCode < 0) > - continue; > - > - for (const SizeRange &size : it.second) { > - if (maxSize_.width < size.max.width && > - maxSize_.height < size.max.height) { > - maxSize_ = size.max; > - mbusCode_ = mbusCode; > - } > - } > - } > - if (maxSize_.width == 0) { > - LOG(IPU3, Info) << "Sensor '" << sensor_->entity()->name() > - << "' detected, but no supported image format " > - << " found: skip camera creation"; > - return -ENODEV; > - } > - > csi2_ = new V4L2Subdevice(csi2Entity); > ret = csi2_->open(); > if (ret) > @@ -1085,47 +1063,19 @@ int CIO2Device::init(const MediaDevice *media, unsigned int index) > int CIO2Device::configure(const StreamConfiguration &config, > V4L2DeviceFormat *outputFormat) > { > - unsigned int imageSize = config.width * config.height; > - V4L2SubdeviceFormat sensorFormat = {}; > - unsigned int best = ~0; > + V4L2SubdeviceFormat sensorFormat; > int ret; > > - for (auto it : sensor_->formats(0)) { > - /* Only consider formats consumable by the CIO2 unit. */ > - if (mediaBusToFormat(it.first) < 0) > - continue; > - > - for (const SizeRange &size : it.second) { > - /* > - * Only select formats bigger than the requested sizes > - * as the IPU3 cannot up-scale. > - * > - * \todo: Unconditionally scale on the sensor as much > - * as possible. This will need to be revisited when > - * implementing the scaling policy. > - */ > - if (size.max.width < config.width || > - size.max.height < config.height) > - continue; > - > - unsigned int diff = size.max.width * size.max.height > - - imageSize; > - if (diff >= best) > - continue; > - > - best = diff; > - > - sensorFormat.width = size.max.width; > - sensorFormat.height = size.max.height; > - sensorFormat.mbus_code = it.first; > - } > - } > - > /* > * Apply the selected format to the sensor, the CSI-2 receiver and > * the CIO2 output device. > */ > - ret = sensor_->setFormat(0, &sensorFormat); > + sensorFormat = sensor_->getFormat({ MEDIA_BUS_FMT_SBGGR10_1X10, > + MEDIA_BUS_FMT_SGBRG10_1X10, > + MEDIA_BUS_FMT_SGRBG10_1X10, > + MEDIA_BUS_FMT_SRGGB10_1X10 }, > + Size(config.width, config.height)); > + ret = sensor_->setFormat(&sensorFormat); > if (ret) > return ret; > > -- > Regards, > > Laurent Pinchart > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Laurent, On Mon, Apr 15, 2019 at 07:56:59PM +0300, Laurent Pinchart wrote: > Replace the manual handling of sensor formats with usage of the new > CameraSensor class. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > src/libcamera/pipeline/ipu3/ipu3.cpp | 78 +++++----------------------- > 1 file changed, 14 insertions(+), 64 deletions(-) > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index e3c79f93963e..1326a1be105f 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -15,6 +15,7 @@ > #include <libcamera/request.h> > #include <libcamera/stream.h> > > +#include "camera_sensor.h" > #include "device_enumerator.h" > #include "log.h" > #include "media_device.h" > @@ -124,11 +125,7 @@ public: > > V4L2Device *output_; > V4L2Subdevice *csi2_; > - V4L2Subdevice *sensor_; > - > - /* Maximum sizes and the mbus code used to produce them. */ > - unsigned int mbusCode_; > - Size maxSize_; > + CameraSensor *sensor_; > > BufferPool pool_; > }; > @@ -255,8 +252,9 @@ int PipelineHandlerIPU3::configureStreams(Camera *camera, > return -EINVAL; > } > > - if (cfg.width > cio2->maxSize_.width || > - cfg.height > cio2->maxSize_.height) { > + const Size &resolution = cio2->sensor_->resolution(); > + if (cfg.width > resolution.width || > + cfg.height > resolution.height) { > LOG(IPU3, Error) > << "Invalid stream size: larger than sensor resolution"; > return -EINVAL; > @@ -1036,31 +1034,11 @@ int CIO2Device::init(const MediaDevice *media, unsigned int index) > * \todo Define when to open and close video device nodes, as they > * might impact on power consumption. > */ > - sensor_ = new V4L2Subdevice(sensorEntity); > - ret = sensor_->open(); > + sensor_ = new CameraSensor(sensorEntity); > + ret = sensor_->init(); > if (ret) > return ret; > > - for (auto it : sensor_->formats(0)) { > - int mbusCode = mediaBusToFormat(it.first); > - if (mbusCode < 0) > - continue; > - > - for (const SizeRange &size : it.second) { > - if (maxSize_.width < size.max.width && > - maxSize_.height < size.max.height) { > - maxSize_ = size.max; > - mbusCode_ = mbusCode; > - } > - } > - } > - if (maxSize_.width == 0) { > - LOG(IPU3, Info) << "Sensor '" << sensor_->entity()->name() > - << "' detected, but no supported image format " > - << " found: skip camera creation"; > - return -ENODEV; > - } > - > csi2_ = new V4L2Subdevice(csi2Entity); > ret = csi2_->open(); > if (ret) > @@ -1085,47 +1063,19 @@ int CIO2Device::init(const MediaDevice *media, unsigned int index) > int CIO2Device::configure(const StreamConfiguration &config, > V4L2DeviceFormat *outputFormat) > { > - unsigned int imageSize = config.width * config.height; > - V4L2SubdeviceFormat sensorFormat = {}; > - unsigned int best = ~0; > + V4L2SubdeviceFormat sensorFormat; > int ret; > > - for (auto it : sensor_->formats(0)) { > - /* Only consider formats consumable by the CIO2 unit. */ > - if (mediaBusToFormat(it.first) < 0) > - continue; > - > - for (const SizeRange &size : it.second) { > - /* > - * Only select formats bigger than the requested sizes > - * as the IPU3 cannot up-scale. > - * > - * \todo: Unconditionally scale on the sensor as much > - * as possible. This will need to be revisited when > - * implementing the scaling policy. > - */ > - if (size.max.width < config.width || > - size.max.height < config.height) > - continue; > - > - unsigned int diff = size.max.width * size.max.height > - - imageSize; > - if (diff >= best) > - continue; > - > - best = diff; > - > - sensorFormat.width = size.max.width; > - sensorFormat.height = size.max.height; > - sensorFormat.mbus_code = it.first; > - } > - } > - > /* > * Apply the selected format to the sensor, the CSI-2 receiver and > * the CIO2 output device. > */ > - ret = sensor_->setFormat(0, &sensorFormat); > + sensorFormat = sensor_->getFormat({ MEDIA_BUS_FMT_SBGGR10_1X10, > + MEDIA_BUS_FMT_SGBRG10_1X10, > + MEDIA_BUS_FMT_SGRBG10_1X10, > + MEDIA_BUS_FMT_SRGGB10_1X10 }, > + Size(config.width, config.height)); over-paranoid me wonders if this has to be checked... Very nice removing code from pipeline handlers Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Thanks j > + ret = sensor_->setFormat(&sensorFormat); > if (ret) > return ret; > > -- > Regards, > > Laurent Pinchart > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Jacopo, On Tue, Apr 16, 2019 at 06:40:21PM +0200, Jacopo Mondi wrote: > On Mon, Apr 15, 2019 at 07:56:59PM +0300, Laurent Pinchart wrote: > > Replace the manual handling of sensor formats with usage of the new > > CameraSensor class. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > src/libcamera/pipeline/ipu3/ipu3.cpp | 78 +++++----------------------- > > 1 file changed, 14 insertions(+), 64 deletions(-) > > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > > index e3c79f93963e..1326a1be105f 100644 > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > > @@ -15,6 +15,7 @@ > > #include <libcamera/request.h> > > #include <libcamera/stream.h> > > > > +#include "camera_sensor.h" > > #include "device_enumerator.h" > > #include "log.h" > > #include "media_device.h" > > @@ -124,11 +125,7 @@ public: > > > > V4L2Device *output_; > > V4L2Subdevice *csi2_; > > - V4L2Subdevice *sensor_; > > - > > - /* Maximum sizes and the mbus code used to produce them. */ > > - unsigned int mbusCode_; > > - Size maxSize_; > > + CameraSensor *sensor_; > > > > BufferPool pool_; > > }; > > @@ -255,8 +252,9 @@ int PipelineHandlerIPU3::configureStreams(Camera *camera, > > return -EINVAL; > > } > > > > - if (cfg.width > cio2->maxSize_.width || > > - cfg.height > cio2->maxSize_.height) { > > + const Size &resolution = cio2->sensor_->resolution(); > > + if (cfg.width > resolution.width || > > + cfg.height > resolution.height) { > > LOG(IPU3, Error) > > << "Invalid stream size: larger than sensor resolution"; > > return -EINVAL; > > @@ -1036,31 +1034,11 @@ int CIO2Device::init(const MediaDevice *media, unsigned int index) > > * \todo Define when to open and close video device nodes, as they > > * might impact on power consumption. > > */ > > - sensor_ = new V4L2Subdevice(sensorEntity); > > - ret = sensor_->open(); > > + sensor_ = new CameraSensor(sensorEntity); > > + ret = sensor_->init(); > > if (ret) > > return ret; > > > > - for (auto it : sensor_->formats(0)) { > > - int mbusCode = mediaBusToFormat(it.first); > > - if (mbusCode < 0) > > - continue; > > - > > - for (const SizeRange &size : it.second) { > > - if (maxSize_.width < size.max.width && > > - maxSize_.height < size.max.height) { > > - maxSize_ = size.max; > > - mbusCode_ = mbusCode; > > - } > > - } > > - } > > - if (maxSize_.width == 0) { > > - LOG(IPU3, Info) << "Sensor '" << sensor_->entity()->name() > > - << "' detected, but no supported image format " > > - << " found: skip camera creation"; > > - return -ENODEV; > > - } > > - > > csi2_ = new V4L2Subdevice(csi2Entity); > > ret = csi2_->open(); > > if (ret) > > @@ -1085,47 +1063,19 @@ int CIO2Device::init(const MediaDevice *media, unsigned int index) > > int CIO2Device::configure(const StreamConfiguration &config, > > V4L2DeviceFormat *outputFormat) > > { > > - unsigned int imageSize = config.width * config.height; > > - V4L2SubdeviceFormat sensorFormat = {}; > > - unsigned int best = ~0; > > + V4L2SubdeviceFormat sensorFormat; > > int ret; > > > > - for (auto it : sensor_->formats(0)) { > > - /* Only consider formats consumable by the CIO2 unit. */ > > - if (mediaBusToFormat(it.first) < 0) > > - continue; > > - > > - for (const SizeRange &size : it.second) { > > - /* > > - * Only select formats bigger than the requested sizes > > - * as the IPU3 cannot up-scale. > > - * > > - * \todo: Unconditionally scale on the sensor as much > > - * as possible. This will need to be revisited when > > - * implementing the scaling policy. > > - */ > > - if (size.max.width < config.width || > > - size.max.height < config.height) > > - continue; > > - > > - unsigned int diff = size.max.width * size.max.height > > - - imageSize; > > - if (diff >= best) > > - continue; > > - > > - best = diff; > > - > > - sensorFormat.width = size.max.width; > > - sensorFormat.height = size.max.height; > > - sensorFormat.mbus_code = it.first; > > - } > > - } > > - > > /* > > * Apply the selected format to the sensor, the CSI-2 receiver and > > * the CIO2 output device. > > */ > > - ret = sensor_->setFormat(0, &sensorFormat); > > + sensorFormat = sensor_->getFormat({ MEDIA_BUS_FMT_SBGGR10_1X10, > > + MEDIA_BUS_FMT_SGBRG10_1X10, > > + MEDIA_BUS_FMT_SGRBG10_1X10, > > + MEDIA_BUS_FMT_SRGGB10_1X10 }, > > + Size(config.width, config.height)); > > over-paranoid me wonders if this has to be checked... In practice it would be very weird to find a system with a sensor connected to the IPU3 where the two would have no common format. I however think it would make sense to check this, not here, but after calling CameraSensor::init(). I'll do so. > Very nice removing code from pipeline handlers > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > > + ret = sensor_->setFormat(&sensorFormat); > > if (ret) > > return ret; > >
diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp index e3c79f93963e..1326a1be105f 100644 --- a/src/libcamera/pipeline/ipu3/ipu3.cpp +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp @@ -15,6 +15,7 @@ #include <libcamera/request.h> #include <libcamera/stream.h> +#include "camera_sensor.h" #include "device_enumerator.h" #include "log.h" #include "media_device.h" @@ -124,11 +125,7 @@ public: V4L2Device *output_; V4L2Subdevice *csi2_; - V4L2Subdevice *sensor_; - - /* Maximum sizes and the mbus code used to produce them. */ - unsigned int mbusCode_; - Size maxSize_; + CameraSensor *sensor_; BufferPool pool_; }; @@ -255,8 +252,9 @@ int PipelineHandlerIPU3::configureStreams(Camera *camera, return -EINVAL; } - if (cfg.width > cio2->maxSize_.width || - cfg.height > cio2->maxSize_.height) { + const Size &resolution = cio2->sensor_->resolution(); + if (cfg.width > resolution.width || + cfg.height > resolution.height) { LOG(IPU3, Error) << "Invalid stream size: larger than sensor resolution"; return -EINVAL; @@ -1036,31 +1034,11 @@ int CIO2Device::init(const MediaDevice *media, unsigned int index) * \todo Define when to open and close video device nodes, as they * might impact on power consumption. */ - sensor_ = new V4L2Subdevice(sensorEntity); - ret = sensor_->open(); + sensor_ = new CameraSensor(sensorEntity); + ret = sensor_->init(); if (ret) return ret; - for (auto it : sensor_->formats(0)) { - int mbusCode = mediaBusToFormat(it.first); - if (mbusCode < 0) - continue; - - for (const SizeRange &size : it.second) { - if (maxSize_.width < size.max.width && - maxSize_.height < size.max.height) { - maxSize_ = size.max; - mbusCode_ = mbusCode; - } - } - } - if (maxSize_.width == 0) { - LOG(IPU3, Info) << "Sensor '" << sensor_->entity()->name() - << "' detected, but no supported image format " - << " found: skip camera creation"; - return -ENODEV; - } - csi2_ = new V4L2Subdevice(csi2Entity); ret = csi2_->open(); if (ret) @@ -1085,47 +1063,19 @@ int CIO2Device::init(const MediaDevice *media, unsigned int index) int CIO2Device::configure(const StreamConfiguration &config, V4L2DeviceFormat *outputFormat) { - unsigned int imageSize = config.width * config.height; - V4L2SubdeviceFormat sensorFormat = {}; - unsigned int best = ~0; + V4L2SubdeviceFormat sensorFormat; int ret; - for (auto it : sensor_->formats(0)) { - /* Only consider formats consumable by the CIO2 unit. */ - if (mediaBusToFormat(it.first) < 0) - continue; - - for (const SizeRange &size : it.second) { - /* - * Only select formats bigger than the requested sizes - * as the IPU3 cannot up-scale. - * - * \todo: Unconditionally scale on the sensor as much - * as possible. This will need to be revisited when - * implementing the scaling policy. - */ - if (size.max.width < config.width || - size.max.height < config.height) - continue; - - unsigned int diff = size.max.width * size.max.height - - imageSize; - if (diff >= best) - continue; - - best = diff; - - sensorFormat.width = size.max.width; - sensorFormat.height = size.max.height; - sensorFormat.mbus_code = it.first; - } - } - /* * Apply the selected format to the sensor, the CSI-2 receiver and * the CIO2 output device. */ - ret = sensor_->setFormat(0, &sensorFormat); + sensorFormat = sensor_->getFormat({ MEDIA_BUS_FMT_SBGGR10_1X10, + MEDIA_BUS_FMT_SGBRG10_1X10, + MEDIA_BUS_FMT_SGRBG10_1X10, + MEDIA_BUS_FMT_SRGGB10_1X10 }, + Size(config.width, config.height)); + ret = sensor_->setFormat(&sensorFormat); if (ret) return ret;
Replace the manual handling of sensor formats with usage of the new CameraSensor class. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- src/libcamera/pipeline/ipu3/ipu3.cpp | 78 +++++----------------------- 1 file changed, 14 insertions(+), 64 deletions(-)