| Message ID | 20190320163055.22056-4-jacopo@jmondi.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, Thank you for the patch. On Wed, Mar 20, 2019 at 05:30:27PM +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. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > src/libcamera/pipeline/ipu3/ipu3.cpp | 43 ++++++++++++++++++++++++++-- > 1 file changed, 41 insertions(+), 2 deletions(-) > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index 55489c31df2d..2602f89617a3 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> > @@ -78,6 +80,8 @@ private: > PipelineHandler::cameraData(camera)); > } > > + int mediaBusToCIO2Format(unsigned int code); > + > void registerCameras(); > > std::shared_ptr<MediaDevice> cio2_; > @@ -327,6 +331,22 @@ bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator) > return true; > } > > +int PipelineHandlerIPU3::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; > + } > +} > + > /* > * Cameras are created associating an image sensor (represented by a > * media entity with function MEDIA_ENT_F_CAM_SENSOR) to one of the four > @@ -404,18 +424,37 @@ 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. > + */ > data->sensor_ = new V4L2Subdevice(sensor); > ret = data->sensor_->open(); > if (ret) > continue; > > + const FormatEnum formats = data->sensor_->formats(0); > + auto it = formats.begin(); > + for (; it != formats.end(); ++it) { How about for (auto it : formats) > + if (mediaBusToCIO2Format(it->first) != -EINVAL) > + break; > + } > + if (it == formats.end()) { > + LOG(IPU3, Info) > + << "Sensor '" << data->sensor_->deviceName() How about printing the entity name instead ? Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > + << "' 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)
Hi Laurent, On Thu, Mar 21, 2019 at 10:50:05AM +0200, Laurent Pinchart wrote: > Hi Jacopo, > > Thank you for the patch. > > On Wed, Mar 20, 2019 at 05:30:27PM +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. > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > src/libcamera/pipeline/ipu3/ipu3.cpp | 43 ++++++++++++++++++++++++++-- > > 1 file changed, 41 insertions(+), 2 deletions(-) > > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > > index 55489c31df2d..2602f89617a3 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> > > @@ -78,6 +80,8 @@ private: > > PipelineHandler::cameraData(camera)); > > } > > > > + int mediaBusToCIO2Format(unsigned int code); > > + > > void registerCameras(); > > > > std::shared_ptr<MediaDevice> cio2_; > > @@ -327,6 +331,22 @@ bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator) > > return true; > > } > > > > +int PipelineHandlerIPU3::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; > > + } > > +} > > + > > /* > > * Cameras are created associating an image sensor (represented by a > > * media entity with function MEDIA_ENT_F_CAM_SENSOR) to one of the four > > @@ -404,18 +424,37 @@ 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. > > + */ > > data->sensor_ = new V4L2Subdevice(sensor); > > ret = data->sensor_->open(); > > if (ret) > > continue; > > > > + const FormatEnum formats = data->sensor_->formats(0); > > + auto it = formats.begin(); > > + for (; it != formats.end(); ++it) { > > How about > > for (auto it : formats) > Here and in other patches in the series I need to check the value of it after the for loop, so I cannot use a variable with a scope local to the loop only. You suggested alternatives offline, but this cose seems quite parsable to me. > > + if (mediaBusToCIO2Format(it->first) != -EINVAL) > > + break; > > + } > > + if (it == formats.end()) { > > + LOG(IPU3, Info) > > + << "Sensor '" << data->sensor_->deviceName() > > How about printing the entity name instead ? For V4L2Subdevice deviceName() prints the entity name. Confusing? > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Thanks, I'll also consider caching the camera mix and min sizes when registering it, as you suggested in a comment to the next patch. Thanks j > > > + << "' 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) > > -- > Regards, > > Laurent Pinchart
Hi Jacopo, On Thu, Mar 21, 2019 at 03:50:16PM +0100, Jacopo Mondi wrote: > On Thu, Mar 21, 2019 at 10:50:05AM +0200, Laurent Pinchart wrote: > > On Wed, Mar 20, 2019 at 05:30:27PM +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. > >> > >> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > >> --- > >> src/libcamera/pipeline/ipu3/ipu3.cpp | 43 ++++++++++++++++++++++++++-- > >> 1 file changed, 41 insertions(+), 2 deletions(-) > >> > >> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > >> index 55489c31df2d..2602f89617a3 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> > >> @@ -78,6 +80,8 @@ private: > >> PipelineHandler::cameraData(camera)); > >> } > >> > >> + int mediaBusToCIO2Format(unsigned int code); > >> + > >> void registerCameras(); > >> > >> std::shared_ptr<MediaDevice> cio2_; > >> @@ -327,6 +331,22 @@ bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator) > >> return true; > >> } > >> > >> +int PipelineHandlerIPU3::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; > >> + } > >> +} > >> + > >> /* > >> * Cameras are created associating an image sensor (represented by a > >> * media entity with function MEDIA_ENT_F_CAM_SENSOR) to one of the four > >> @@ -404,18 +424,37 @@ 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. > >> + */ > >> data->sensor_ = new V4L2Subdevice(sensor); > >> ret = data->sensor_->open(); > >> if (ret) > >> continue; > >> > >> + const FormatEnum formats = data->sensor_->formats(0); > >> + auto it = formats.begin(); > >> + for (; it != formats.end(); ++it) { > > > > How about > > > > for (auto it : formats) > > > > Here and in other patches in the series I need to check the value of > it after the for loop, so I cannot use a variable with a scope local > to the loop only. > > You suggested alternatives offline, but this cose seems quite parsable > to me. > > >> + if (mediaBusToCIO2Format(it->first) != -EINVAL) > >> + break; > >> + } > >> + if (it == formats.end()) { > >> + LOG(IPU3, Info) > >> + << "Sensor '" << data->sensor_->deviceName() > > > > How about printing the entity name instead ? > > For V4L2Subdevice deviceName() prints the entity name. > Confusing? I suppose so, given that I got confused :-) > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Thanks, I'll also consider caching the camera mix and min sizes when > registering it, as you suggested in a comment to the next patch. > > >> + << "' 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)
Hi Laurent, On Sat, Mar 23, 2019 at 02:58:18PM +0200, Laurent Pinchart wrote: > Hi Jacopo, > > On Thu, Mar 21, 2019 at 03:50:16PM +0100, Jacopo Mondi wrote: > > On Thu, Mar 21, 2019 at 10:50:05AM +0200, Laurent Pinchart wrote: > > > On Wed, Mar 20, 2019 at 05:30:27PM +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. > > >> > > >> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > >> --- > > >> src/libcamera/pipeline/ipu3/ipu3.cpp | 43 ++++++++++++++++++++++++++-- > > >> 1 file changed, 41 insertions(+), 2 deletions(-) > > >> > > >> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > > >> index 55489c31df2d..2602f89617a3 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> > > >> @@ -78,6 +80,8 @@ private: > > >> PipelineHandler::cameraData(camera)); > > >> } > > >> > > >> + int mediaBusToCIO2Format(unsigned int code); > > >> + > > >> void registerCameras(); > > >> > > >> std::shared_ptr<MediaDevice> cio2_; > > >> @@ -327,6 +331,22 @@ bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator) > > >> return true; > > >> } > > >> > > >> +int PipelineHandlerIPU3::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; > > >> + } > > >> +} > > >> + > > >> /* > > >> * Cameras are created associating an image sensor (represented by a > > >> * media entity with function MEDIA_ENT_F_CAM_SENSOR) to one of the four > > >> @@ -404,18 +424,37 @@ 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. > > >> + */ > > >> data->sensor_ = new V4L2Subdevice(sensor); > > >> ret = data->sensor_->open(); > > >> if (ret) > > >> continue; > > >> > > >> + const FormatEnum formats = data->sensor_->formats(0); > > >> + auto it = formats.begin(); > > >> + for (; it != formats.end(); ++it) { > > > > > > How about > > > > > > for (auto it : formats) > > > > > > > Here and in other patches in the series I need to check the value of > > it after the for loop, so I cannot use a variable with a scope local > > to the loop only. > > > > You suggested alternatives offline, but this cose seems quite parsable > > to me. > > > > >> + if (mediaBusToCIO2Format(it->first) != -EINVAL) > > >> + break; > > >> + } > > >> + if (it == formats.end()) { > > >> + LOG(IPU3, Info) > > >> + << "Sensor '" << data->sensor_->deviceName() > > > > > > How about printing the entity name instead ? > > > > For V4L2Subdevice deviceName() prints the entity name. > > Confusing? > > I suppose so, given that I got confused :-) V5 will contain a patch which changes this (and makes the V4L2Subdevice return a const & to a string in its deviceNode() and deviceName() methods, which they embarassing enough copy back at the moment) Thanks j > > > > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > Thanks, I'll also consider caching the camera mix and min sizes when > > registering it, as you suggested in a comment to the next patch. > > > > >> + << "' 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) > > -- > Regards, > > Laurent Pinchart
diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp index 55489c31df2d..2602f89617a3 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> @@ -78,6 +80,8 @@ private: PipelineHandler::cameraData(camera)); } + int mediaBusToCIO2Format(unsigned int code); + void registerCameras(); std::shared_ptr<MediaDevice> cio2_; @@ -327,6 +331,22 @@ bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator) return true; } +int PipelineHandlerIPU3::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; + } +} + /* * Cameras are created associating an image sensor (represented by a * media entity with function MEDIA_ENT_F_CAM_SENSOR) to one of the four @@ -404,18 +424,37 @@ 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. + */ data->sensor_ = new V4L2Subdevice(sensor); ret = data->sensor_->open(); if (ret) continue; + const FormatEnum formats = data->sensor_->formats(0); + auto it = formats.begin(); + for (; it != formats.end(); ++it) { + if (mediaBusToCIO2Format(it->first) != -EINVAL) + break; + } + if (it == formats.end()) { + LOG(IPU3, Info) + << "Sensor '" << data->sensor_->deviceName() + << "' 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. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- src/libcamera/pipeline/ipu3/ipu3.cpp | 43 ++++++++++++++++++++++++++-- 1 file changed, 41 insertions(+), 2 deletions(-)