| Message ID | 20190320163055.22056-8-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:31PM +0100, Jacopo Mondi wrote: > Apply the requested image format to the sensor device, and apply the > adjusted one to the CIO2 device, the ImgU subdevice and its input and > output video devices. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > src/libcamera/pipeline/ipu3/ipu3.cpp | 220 +++++++++++++++++++++++---- > 1 file changed, 189 insertions(+), 31 deletions(-) > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index 26fc56a76eb1..2975c218f6c9 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -16,6 +16,7 @@ > #include <libcamera/stream.h> > > #include "device_enumerator.h" > +#include "geometry.h" > #include "log.h" > #include "media_device.h" > #include "pipeline_handler.h" > @@ -30,6 +31,11 @@ LOG_DEFINE_CATEGORY(IPU3) > class ImgUDevice > { > public: > + static constexpr unsigned int PAD_INPUT = 0; > + static constexpr unsigned int PAD_OUTPUT = 2; > + static constexpr unsigned int PAD_VF = 3; > + static constexpr unsigned int PAD_STAT = 4; > + > ImgUDevice() > : imgu(nullptr), input(nullptr), output(nullptr), > viewfinder(nullptr), stat(nullptr) > @@ -135,6 +141,13 @@ private: > > int initImgU(ImgUDevice *imgu); > > + int setImguFormat(ImgUDevice *imguDevice, > + const StreamConfiguration &config, > + Rectangle *rect); This should be moved to the ImgU class. > + int setCIO2Format(CIO2Device *cio2Device, > + const StreamConfiguration &config, > + V4L2SubdeviceFormat *format); > + > void registerCameras(); > > ImgUDevice imgus_[IPU3_IMGU_COUNT]; > @@ -202,60 +215,86 @@ int PipelineHandlerIPU3::configureStreams(Camera *camera, > std::map<Stream *, StreamConfiguration> &config) > { > IPU3CameraData *data = cameraData(camera); > - StreamConfiguration *cfg = &config[&data->stream_]; > - V4L2Subdevice *sensor = data->cio2.sensor; > - V4L2Subdevice *csi2 = data->cio2.csi2; > + const StreamConfiguration &cfg = config[&data->stream_]; > + V4L2Device *viewfinder = data->imgu->viewfinder; > + V4L2Device *output = data->imgu->output; > + V4L2Device *input = data->imgu->input; > V4L2Device *cio2 = data->cio2.output; > - V4L2SubdeviceFormat subdevFormat = {}; > - V4L2DeviceFormat devFormat = {}; > int ret; > > + LOG(IPU3, Info) > + << "Requested image format: " << cfg.width << "x" > + << cfg.height << " - " << std::hex << std::setw(8) > + << cfg.pixelFormat << " on camera:'" << camera->name() << "'"; > + > /* > - * FIXME: as of now, the format gets applied to the sensor and is > - * propagated along the pipeline. It should instead be applied on the > - * capture device and the sensor format calculated accordingly. > + * Verify that the requested size respects the IPU3 alignement > + * requirements: the image width shall be a multiple of 8 pixels and > + * its height a multiple of 4 pixels. > + * > + * \todo: consider the BDS scaling factor requirements: > + * "the downscaling factor must be an integer value multiple of 1/32" > */ > + if (cfg.width % 8 || cfg.height % 4) { > + LOG(IPU3, Error) << "Stream format not support: bad alignement"; > + return -EINVAL; > + } > > - ret = sensor->getFormat(0, &subdevFormat); > + /* > + * Pass the requested output image size to the sensor and get back the > + * adjusted one to be propagated to the CIO2 device and to the ImgU > + * input. > + */ > + V4L2SubdeviceFormat sensorFormat = {}; > + ret = setCIO2Format(&data->cio2, cfg, &sensorFormat); > if (ret) > return ret; > > - subdevFormat.width = cfg->width; > - subdevFormat.height = cfg->height; > - ret = sensor->setFormat(0, &subdevFormat); > + /* Apply the CIO2 image format to the CIO2 output and ImgU input. */ > + V4L2DeviceFormat cio2Format = {}; > + cio2Format.width = sensorFormat.width; > + cio2Format.height = sensorFormat.height; > + cio2Format.fourcc = mediaBusToCIO2Format(sensorFormat.mbus_code); > + cio2Format.planesCount = 1; > + ret = cio2->setFormat(&cio2Format); Shouldn't this be part of setCIO2Format() ? > if (ret) > return ret; > > - /* Return error if the requested format cannot be applied to sensor. */ > - if (subdevFormat.width != cfg->width || > - subdevFormat.height != cfg->height) { > - LOG(IPU3, Error) > - << "Failed to apply image format " > - << subdevFormat.width << "x" << subdevFormat.height > - << " - got: " << cfg->width << "x" << cfg->height; > - return -EINVAL; > - } > + LOG(IPU3, Debug) > + << "CIO2 output format = " << cio2Format.toString(); > > - ret = csi2->setFormat(0, &subdevFormat); > + ret = input->setFormat(&cio2Format); > if (ret) > return ret; > > - ret = cio2->getFormat(&devFormat); > + /* Apply pad formats and crop/compose rectangle to the ImgU. */ > + Rectangle rect = { > + .x = 0, > + .y = 0, > + .w = cio2Format.width, > + .h = cio2Format.height, > + }; > + ret = setImguFormat(data->imgu, cfg, &rect); > if (ret) > return ret; > > - devFormat.width = subdevFormat.width; > - devFormat.height = subdevFormat.height; > - devFormat.fourcc = cfg->pixelFormat; > + /* Apply the format to the ImgU output and viewfinder devices. */ > + V4L2DeviceFormat outputFormat = {}; > + outputFormat.width = cfg.width; > + outputFormat.height = cfg.height; > + outputFormat.fourcc = V4L2_PIX_FMT_NV12; > + outputFormat.planesCount = 2; > > - ret = cio2->setFormat(&devFormat); > + ret = output->setFormat(&outputFormat); > if (ret) > return ret; > > - LOG(IPU3, Info) << cio2->driverName() << ": " > - << devFormat.width << "x" << devFormat.height > - << "- 0x" << std::hex << devFormat.fourcc << " planes: " > - << devFormat.planes; > + LOG(IPU3, Debug) > + << "ImgU output format = " << outputFormat.toString(); > + > + ret = viewfinder->setFormat(&outputFormat); > + if (ret) > + return ret; > > return 0; > } > @@ -666,6 +705,125 @@ void PipelineHandlerIPU3::deleteCIO2(CIO2Device *cio2) > delete cio2->sensor; > } > > +int PipelineHandlerIPU3::setImguFormat(ImgUDevice *imguDevice, > + const StreamConfiguration &config, > + Rectangle *rect) > +{ > + V4L2Subdevice *imgu = imguDevice->imgu; > + int ret; > + > + /* > + * Configure the 'imgu' subdevice with the requested sizes. s/'imgu'/ImgU/ ? > + * > + * FIXME: the IPU3 driver implementation shall be changed to use the > + * actual input sizes as 'imgu input' subdevice sizes, and use the > + * desired output sizes to configure the crop/compose rectangles. The > + * current implementation uses output sizes as 'imgu input' sizes, and > + * uses the input dimension to configure the crop/compose rectangles, > + * which contradicts the V4L2 specifications. s/specifications/specification/ > + */ > + ret = imgu->setCrop(ImgUDevice::PAD_INPUT, rect); > + if (ret) > + return ret; > + > + ret = imgu->setCompose(ImgUDevice::PAD_INPUT, rect); > + if (ret) > + return ret; > + > + LOG(IPU3, Debug) > + << "ImgU input feeder and BDS rectangle = (0,0)/" > + << rect->w << "x" << rect->h; toString() for our Rectangle class ? > + > + V4L2SubdeviceFormat imguFormat = {}; > + imguFormat.width = config.width; > + imguFormat.height = config.height; > + imguFormat.mbus_code = MEDIA_BUS_FMT_FIXED; > + > + ret = imgu->setFormat(ImgUDevice::PAD_INPUT, &imguFormat); > + if (ret) > + return ret; > + > + ret = imgu->setFormat(ImgUDevice::PAD_OUTPUT, &imguFormat); > + if (ret) > + return ret; > + > + LOG(IPU3, Debug) > + << "ImgU GDC format = " << imguFormat.toString(); > + > + ret = imgu->setFormat(ImgUDevice::PAD_VF, &imguFormat); > + if (ret) > + return ret; > + > + ret = imgu->setFormat(ImgUDevice::PAD_STAT, &imguFormat); > + if (ret) > + return ret; > + > + return 0; > +} > + > +int PipelineHandlerIPU3::setCIO2Format(CIO2Device *cio2Device, > + const StreamConfiguration &config, > + V4L2SubdeviceFormat *format) > +{ We're starting to get quite some code associated with the CIO2Device. I know I requested for CIO2Device to be a struct without methods, but I wonder if that was a mistake :-( I would also rename the function, maybe to configure(). setFormat() hints that it sets the format passed as an argument, which is not the case here, the format parameter is an output. > + unsigned int imageSize = config.width * config.height; > + V4L2Subdevice *sensor = cio2Device->sensor; > + V4L2Subdevice *csi2 = cio2Device->csi2; > + unsigned int best = ~0; > + bool found = false; > + int ret; > + > + const FormatEnum formats = sensor->formats(0); > + for (auto it = formats.begin(); it != formats.end(); ++it) { > + /* Only consider formats consumable by the CIO2 unit. */ > + int cio2Code = mediaBusToCIO2Format(it->first); > + if (cio2Code == -EINVAL) You could write if (mediaBusToCIO2Format(it->first) == -EINVAL) (and I would check for < 0 instead of == -EINVAL to avoid surprises if we later change that method) > + continue; > + > + for (const SizeRange &size : it->second) { > + /* > + * Only select formats bigger than the requested sizes > + * as the IPU3 cannot up-scale. > + */ > + if (size.maxWidth < config.width || > + size.maxHeight < config.height) > + continue; > + > + unsigned int diff = size.maxWidth * size.maxHeight > + - imageSize; > + if (diff >= best) > + continue; > + > + best = diff; > + found = true; > + > + format->width = size.maxWidth; > + format->height = size.maxHeight; > + format->mbus_code = it->first; > + } > + } Open question, should we do this, or always pick the largest size from the sensor and scale in the ImgU ? > + if (!found) { > + LOG(IPU3, Error) > + << "Unable to find image format suitable to produce: " > + << config.width << "x" << config.height > + << "- 0x" << std::hex << std::setfill('0') Spaces before and after dash, or none at all. > + << std::setw(8) << config.pixelFormat; Again a shame we can't use toString(). Do you use that helper at all ? :-) > + return -EINVAL; > + } > + > + /* Apply the selected format to the sensor and the CSI-2 receiver. */ > + ret = sensor->setFormat(0, format); > + if (ret) > + return ret; > + > + ret = csi2->setFormat(0, format); > + if (ret) > + return ret; > + > + LOG(IPU3, Debug) << "CIO2 Image format: " << format->toString(); Ah yes you use it here. > + > + return 0; > +} > + > /* > * Cameras are created associating an image sensor (represented by a > * media entity with function MEDIA_ENT_F_CAM_SENSOR) to one of the four
Hi Laurent, On Thu, Mar 21, 2019 at 11:54:18AM +0200, Laurent Pinchart wrote: > Hi Jacopo, > > Thank you for the patch. > > On Wed, Mar 20, 2019 at 05:30:31PM +0100, Jacopo Mondi wrote: > > Apply the requested image format to the sensor device, and apply the > > adjusted one to the CIO2 device, the ImgU subdevice and its input and > > output video devices. > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > src/libcamera/pipeline/ipu3/ipu3.cpp | 220 +++++++++++++++++++++++---- > > 1 file changed, 189 insertions(+), 31 deletions(-) > > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > > index 26fc56a76eb1..2975c218f6c9 100644 > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > > @@ -16,6 +16,7 @@ > > #include <libcamera/stream.h> > > > > #include "device_enumerator.h" > > +#include "geometry.h" > > #include "log.h" > > #include "media_device.h" > > #include "pipeline_handler.h" > > @@ -30,6 +31,11 @@ LOG_DEFINE_CATEGORY(IPU3) > > class ImgUDevice > > { > > public: > > + static constexpr unsigned int PAD_INPUT = 0; > > + static constexpr unsigned int PAD_OUTPUT = 2; > > + static constexpr unsigned int PAD_VF = 3; > > + static constexpr unsigned int PAD_STAT = 4; > > + > > ImgUDevice() > > : imgu(nullptr), input(nullptr), output(nullptr), > > viewfinder(nullptr), stat(nullptr) > > @@ -135,6 +141,13 @@ private: > > > > int initImgU(ImgUDevice *imgu); > > > > + int setImguFormat(ImgUDevice *imguDevice, > > + const StreamConfiguration &config, > > + Rectangle *rect); > > This should be moved to the ImgU class. > > > + int setCIO2Format(CIO2Device *cio2Device, > > + const StreamConfiguration &config, > > + V4L2SubdeviceFormat *format); and this to the CIO2 class too? > > + > > void registerCameras(); > > > > ImgUDevice imgus_[IPU3_IMGU_COUNT]; > > @@ -202,60 +215,86 @@ int PipelineHandlerIPU3::configureStreams(Camera *camera, > > std::map<Stream *, StreamConfiguration> &config) > > { > > IPU3CameraData *data = cameraData(camera); > > - StreamConfiguration *cfg = &config[&data->stream_]; > > - V4L2Subdevice *sensor = data->cio2.sensor; > > - V4L2Subdevice *csi2 = data->cio2.csi2; > > + const StreamConfiguration &cfg = config[&data->stream_]; > > + V4L2Device *viewfinder = data->imgu->viewfinder; > > + V4L2Device *output = data->imgu->output; > > + V4L2Device *input = data->imgu->input; > > V4L2Device *cio2 = data->cio2.output; > > - V4L2SubdeviceFormat subdevFormat = {}; > > - V4L2DeviceFormat devFormat = {}; > > int ret; > > > > + LOG(IPU3, Info) > > + << "Requested image format: " << cfg.width << "x" > > + << cfg.height << " - " << std::hex << std::setw(8) > > + << cfg.pixelFormat << " on camera:'" << camera->name() << "'"; > > + > > /* > > - * FIXME: as of now, the format gets applied to the sensor and is > > - * propagated along the pipeline. It should instead be applied on the > > - * capture device and the sensor format calculated accordingly. > > + * Verify that the requested size respects the IPU3 alignement > > + * requirements: the image width shall be a multiple of 8 pixels and > > + * its height a multiple of 4 pixels. > > + * > > + * \todo: consider the BDS scaling factor requirements: > > + * "the downscaling factor must be an integer value multiple of 1/32" > > */ > > + if (cfg.width % 8 || cfg.height % 4) { > > + LOG(IPU3, Error) << "Stream format not support: bad alignement"; > > + return -EINVAL; > > + } > > > > - ret = sensor->getFormat(0, &subdevFormat); > > + /* > > + * Pass the requested output image size to the sensor and get back the > > + * adjusted one to be propagated to the CIO2 device and to the ImgU > > + * input. > > + */ > > + V4L2SubdeviceFormat sensorFormat = {}; > > + ret = setCIO2Format(&data->cio2, cfg, &sensorFormat); > > if (ret) > > return ret; > > > > - subdevFormat.width = cfg->width; > > - subdevFormat.height = cfg->height; > > - ret = sensor->setFormat(0, &subdevFormat); > > + /* Apply the CIO2 image format to the CIO2 output and ImgU input. */ > > + V4L2DeviceFormat cio2Format = {}; > > + cio2Format.width = sensorFormat.width; > > + cio2Format.height = sensorFormat.height; > > + cio2Format.fourcc = mediaBusToCIO2Format(sensorFormat.mbus_code); > > + cio2Format.planesCount = 1; > > + ret = cio2->setFormat(&cio2Format); > > Shouldn't this be part of setCIO2Format() ? Moved there. I wanted to expose the fact the CIO2 output and the ImgU input are configured with the same format. > > > if (ret) > > return ret; > > > > - /* Return error if the requested format cannot be applied to sensor. */ > > - if (subdevFormat.width != cfg->width || > > - subdevFormat.height != cfg->height) { > > - LOG(IPU3, Error) > > - << "Failed to apply image format " > > - << subdevFormat.width << "x" << subdevFormat.height > > - << " - got: " << cfg->width << "x" << cfg->height; > > - return -EINVAL; > > - } > > + LOG(IPU3, Debug) > > + << "CIO2 output format = " << cio2Format.toString(); > > > > - ret = csi2->setFormat(0, &subdevFormat); > > + ret = input->setFormat(&cio2Format); > > if (ret) > > return ret; > > > > - ret = cio2->getFormat(&devFormat); > > + /* Apply pad formats and crop/compose rectangle to the ImgU. */ > > + Rectangle rect = { > > + .x = 0, > > + .y = 0, > > + .w = cio2Format.width, > > + .h = cio2Format.height, > > + }; > > + ret = setImguFormat(data->imgu, cfg, &rect); > > if (ret) > > return ret; > > > > - devFormat.width = subdevFormat.width; > > - devFormat.height = subdevFormat.height; > > - devFormat.fourcc = cfg->pixelFormat; > > + /* Apply the format to the ImgU output and viewfinder devices. */ > > + V4L2DeviceFormat outputFormat = {}; > > + outputFormat.width = cfg.width; > > + outputFormat.height = cfg.height; > > + outputFormat.fourcc = V4L2_PIX_FMT_NV12; > > + outputFormat.planesCount = 2; > > > > - ret = cio2->setFormat(&devFormat); > > + ret = output->setFormat(&outputFormat); > > if (ret) > > return ret; > > > > - LOG(IPU3, Info) << cio2->driverName() << ": " > > - << devFormat.width << "x" << devFormat.height > > - << "- 0x" << std::hex << devFormat.fourcc << " planes: " > > - << devFormat.planes; > > + LOG(IPU3, Debug) > > + << "ImgU output format = " << outputFormat.toString(); > > + > > + ret = viewfinder->setFormat(&outputFormat); > > + if (ret) > > + return ret; > > > > return 0; > > } > > @@ -666,6 +705,125 @@ void PipelineHandlerIPU3::deleteCIO2(CIO2Device *cio2) > > delete cio2->sensor; > > } > > > > +int PipelineHandlerIPU3::setImguFormat(ImgUDevice *imguDevice, > > + const StreamConfiguration &config, > > + Rectangle *rect) > > +{ > > + V4L2Subdevice *imgu = imguDevice->imgu; > > + int ret; > > + > > + /* > > + * Configure the 'imgu' subdevice with the requested sizes. > > s/'imgu'/ImgU/ ? > > > + * > > + * FIXME: the IPU3 driver implementation shall be changed to use the > > + * actual input sizes as 'imgu input' subdevice sizes, and use the > > + * desired output sizes to configure the crop/compose rectangles. The > > + * current implementation uses output sizes as 'imgu input' sizes, and > > + * uses the input dimension to configure the crop/compose rectangles, > > + * which contradicts the V4L2 specifications. > > s/specifications/specification/ > > > + */ > > + ret = imgu->setCrop(ImgUDevice::PAD_INPUT, rect); > > + if (ret) > > + return ret; > > + > > + ret = imgu->setCompose(ImgUDevice::PAD_INPUT, rect); > > + if (ret) > > + return ret; > > + > > + LOG(IPU3, Debug) > > + << "ImgU input feeder and BDS rectangle = (0,0)/" > > + << rect->w << "x" << rect->h; > > toString() for our Rectangle class ? > I'll add a patch > > + > > + V4L2SubdeviceFormat imguFormat = {}; > > + imguFormat.width = config.width; > > + imguFormat.height = config.height; > > + imguFormat.mbus_code = MEDIA_BUS_FMT_FIXED; > > + > > + ret = imgu->setFormat(ImgUDevice::PAD_INPUT, &imguFormat); > > + if (ret) > > + return ret; > > + > > + ret = imgu->setFormat(ImgUDevice::PAD_OUTPUT, &imguFormat); > > + if (ret) > > + return ret; > > + > > + LOG(IPU3, Debug) > > + << "ImgU GDC format = " << imguFormat.toString(); > > + > > + ret = imgu->setFormat(ImgUDevice::PAD_VF, &imguFormat); > > + if (ret) > > + return ret; > > + > > + ret = imgu->setFormat(ImgUDevice::PAD_STAT, &imguFormat); > > + if (ret) > > + return ret; > > + > > + return 0; > > +} > > + > > +int PipelineHandlerIPU3::setCIO2Format(CIO2Device *cio2Device, > > + const StreamConfiguration &config, > > + V4L2SubdeviceFormat *format) > > +{ > > We're starting to get quite some code associated with the CIO2Device. I > know I requested for CIO2Device to be a struct without methods, but I > wonder if that was a mistake :-( > > I would also rename the function, maybe to configure(). setFormat() > hints that it sets the format passed as an argument, which is not the > case here, the format parameter is an output. > > > + unsigned int imageSize = config.width * config.height; > > + V4L2Subdevice *sensor = cio2Device->sensor; > > + V4L2Subdevice *csi2 = cio2Device->csi2; > > + unsigned int best = ~0; > > + bool found = false; > > + int ret; > > + > > + const FormatEnum formats = sensor->formats(0); > > + for (auto it = formats.begin(); it != formats.end(); ++it) { > > + /* Only consider formats consumable by the CIO2 unit. */ > > + int cio2Code = mediaBusToCIO2Format(it->first); > > + if (cio2Code == -EINVAL) > > You could write > > if (mediaBusToCIO2Format(it->first) == -EINVAL) > > (and I would check for < 0 instead of == -EINVAL to avoid surprises if > we later change that method) > > > + continue; > > + > > + for (const SizeRange &size : it->second) { > > + /* > > + * Only select formats bigger than the requested sizes > > + * as the IPU3 cannot up-scale. > > + */ > > + if (size.maxWidth < config.width || > > + size.maxHeight < config.height) > > + continue; > > + > > + unsigned int diff = size.maxWidth * size.maxHeight > > + - imageSize; > > + if (diff >= best) > > + continue; > > + > > + best = diff; > > + found = true; > > + > > + format->width = size.maxWidth; > > + format->height = size.maxHeight; > > + format->mbus_code = it->first; > > + } > > + } > > Open question, should we do this, or always pick the largest size from > the sensor and scale in the ImgU ? > Good question... I cannot tell the pros and cons actually... why would you think it is better? Thanks j > > + if (!found) { > > + LOG(IPU3, Error) > > + << "Unable to find image format suitable to produce: " > > + << config.width << "x" << config.height > > + << "- 0x" << std::hex << std::setfill('0') > > Spaces before and after dash, or none at all. > > > + << std::setw(8) << config.pixelFormat; > > Again a shame we can't use toString(). Do you use that helper at all ? > :-) > > > + return -EINVAL; > > + } > > + > > + /* Apply the selected format to the sensor and the CSI-2 receiver. */ > > + ret = sensor->setFormat(0, format); > > + if (ret) > > + return ret; > > + > > + ret = csi2->setFormat(0, format); > > + if (ret) > > + return ret; > > + > > + LOG(IPU3, Debug) << "CIO2 Image format: " << format->toString(); > > Ah yes you use it here. > > > + > > + return 0; > > +} > > + > > /* > > * Cameras are created associating an image sensor (represented by a > > * media entity with function MEDIA_ENT_F_CAM_SENSOR) to one of the four > > -- > Regards, > > Laurent Pinchart
On Thu, Mar 21, 2019 at 08:31:12PM +0100, Jacopo Mondi wrote: > Hi Laurent, > > On Thu, Mar 21, 2019 at 11:54:18AM +0200, Laurent Pinchart wrote: > > Hi Jacopo, > > > > Thank you for the patch. > > > > On Wed, Mar 20, 2019 at 05:30:31PM +0100, Jacopo Mondi wrote: > > > Apply the requested image format to the sensor device, and apply the > > > adjusted one to the CIO2 device, the ImgU subdevice and its input and > > > output video devices. > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > --- > > > src/libcamera/pipeline/ipu3/ipu3.cpp | 220 +++++++++++++++++++++++---- > > > 1 file changed, 189 insertions(+), 31 deletions(-) > > > > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > > > index 26fc56a76eb1..2975c218f6c9 100644 > > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > > > @@ -16,6 +16,7 @@ > > > #include <libcamera/stream.h> > > > > > > #include "device_enumerator.h" > > > +#include "geometry.h" > > > #include "log.h" > > > #include "media_device.h" > > > #include "pipeline_handler.h" > > > @@ -30,6 +31,11 @@ LOG_DEFINE_CATEGORY(IPU3) > > > class ImgUDevice > > > { > > > public: > > > + static constexpr unsigned int PAD_INPUT = 0; > > > + static constexpr unsigned int PAD_OUTPUT = 2; > > > + static constexpr unsigned int PAD_VF = 3; > > > + static constexpr unsigned int PAD_STAT = 4; > > > + > > > ImgUDevice() > > > : imgu(nullptr), input(nullptr), output(nullptr), > > > viewfinder(nullptr), stat(nullptr) > > > @@ -135,6 +141,13 @@ private: > > > > > > int initImgU(ImgUDevice *imgu); > > > > > > + int setImguFormat(ImgUDevice *imguDevice, > > > + const StreamConfiguration &config, > > > + Rectangle *rect); > > > > This should be moved to the ImgU class. > > > > > + int setCIO2Format(CIO2Device *cio2Device, > > > + const StreamConfiguration &config, > > > + V4L2SubdeviceFormat *format); > > and this to the CIO2 class too? > > > > > + > > > void registerCameras(); > > > > > > ImgUDevice imgus_[IPU3_IMGU_COUNT]; > > > @@ -202,60 +215,86 @@ int PipelineHandlerIPU3::configureStreams(Camera *camera, > > > std::map<Stream *, StreamConfiguration> &config) > > > { > > > IPU3CameraData *data = cameraData(camera); > > > - StreamConfiguration *cfg = &config[&data->stream_]; > > > - V4L2Subdevice *sensor = data->cio2.sensor; > > > - V4L2Subdevice *csi2 = data->cio2.csi2; > > > + const StreamConfiguration &cfg = config[&data->stream_]; > > > + V4L2Device *viewfinder = data->imgu->viewfinder; > > > + V4L2Device *output = data->imgu->output; > > > + V4L2Device *input = data->imgu->input; > > > V4L2Device *cio2 = data->cio2.output; > > > - V4L2SubdeviceFormat subdevFormat = {}; > > > - V4L2DeviceFormat devFormat = {}; > > > int ret; > > > > > > + LOG(IPU3, Info) > > > + << "Requested image format: " << cfg.width << "x" > > > + << cfg.height << " - " << std::hex << std::setw(8) > > > + << cfg.pixelFormat << " on camera:'" << camera->name() << "'"; > > > + > > > /* > > > - * FIXME: as of now, the format gets applied to the sensor and is > > > - * propagated along the pipeline. It should instead be applied on the > > > - * capture device and the sensor format calculated accordingly. > > > + * Verify that the requested size respects the IPU3 alignement > > > + * requirements: the image width shall be a multiple of 8 pixels and > > > + * its height a multiple of 4 pixels. > > > + * > > > + * \todo: consider the BDS scaling factor requirements: > > > + * "the downscaling factor must be an integer value multiple of 1/32" > > > */ > > > + if (cfg.width % 8 || cfg.height % 4) { > > > + LOG(IPU3, Error) << "Stream format not support: bad alignement"; > > > + return -EINVAL; > > > + } > > > > > > - ret = sensor->getFormat(0, &subdevFormat); > > > + /* > > > + * Pass the requested output image size to the sensor and get back the > > > + * adjusted one to be propagated to the CIO2 device and to the ImgU > > > + * input. > > > + */ > > > + V4L2SubdeviceFormat sensorFormat = {}; > > > + ret = setCIO2Format(&data->cio2, cfg, &sensorFormat); > > > if (ret) > > > return ret; > > > > > > - subdevFormat.width = cfg->width; > > > - subdevFormat.height = cfg->height; > > > - ret = sensor->setFormat(0, &subdevFormat); > > > + /* Apply the CIO2 image format to the CIO2 output and ImgU input. */ > > > + V4L2DeviceFormat cio2Format = {}; > > > + cio2Format.width = sensorFormat.width; > > > + cio2Format.height = sensorFormat.height; > > > + cio2Format.fourcc = mediaBusToCIO2Format(sensorFormat.mbus_code); > > > + cio2Format.planesCount = 1; > > > + ret = cio2->setFormat(&cio2Format); > > > > Shouldn't this be part of setCIO2Format() ? > > Moved there. I wanted to expose the fact the CIO2 output and the ImgU > input are configured with the same format. > > > > > if (ret) > > > return ret; > > > > > > - /* Return error if the requested format cannot be applied to sensor. */ > > > - if (subdevFormat.width != cfg->width || > > > - subdevFormat.height != cfg->height) { > > > - LOG(IPU3, Error) > > > - << "Failed to apply image format " > > > - << subdevFormat.width << "x" << subdevFormat.height > > > - << " - got: " << cfg->width << "x" << cfg->height; > > > - return -EINVAL; > > > - } > > > + LOG(IPU3, Debug) > > > + << "CIO2 output format = " << cio2Format.toString(); > > > > > > - ret = csi2->setFormat(0, &subdevFormat); > > > + ret = input->setFormat(&cio2Format); > > > if (ret) > > > return ret; > > > > > > - ret = cio2->getFormat(&devFormat); > > > + /* Apply pad formats and crop/compose rectangle to the ImgU. */ > > > + Rectangle rect = { > > > + .x = 0, > > > + .y = 0, > > > + .w = cio2Format.width, > > > + .h = cio2Format.height, > > > + }; > > > + ret = setImguFormat(data->imgu, cfg, &rect); > > > if (ret) > > > return ret; > > > > > > - devFormat.width = subdevFormat.width; > > > - devFormat.height = subdevFormat.height; > > > - devFormat.fourcc = cfg->pixelFormat; > > > + /* Apply the format to the ImgU output and viewfinder devices. */ > > > + V4L2DeviceFormat outputFormat = {}; > > > + outputFormat.width = cfg.width; > > > + outputFormat.height = cfg.height; > > > + outputFormat.fourcc = V4L2_PIX_FMT_NV12; > > > + outputFormat.planesCount = 2; > > > > > > - ret = cio2->setFormat(&devFormat); > > > + ret = output->setFormat(&outputFormat); > > > if (ret) > > > return ret; > > > > > > - LOG(IPU3, Info) << cio2->driverName() << ": " > > > - << devFormat.width << "x" << devFormat.height > > > - << "- 0x" << std::hex << devFormat.fourcc << " planes: " > > > - << devFormat.planes; > > > + LOG(IPU3, Debug) > > > + << "ImgU output format = " << outputFormat.toString(); > > > + > > > + ret = viewfinder->setFormat(&outputFormat); > > > + if (ret) > > > + return ret; > > > > > > return 0; > > > } > > > @@ -666,6 +705,125 @@ void PipelineHandlerIPU3::deleteCIO2(CIO2Device *cio2) > > > delete cio2->sensor; > > > } > > > > > > +int PipelineHandlerIPU3::setImguFormat(ImgUDevice *imguDevice, > > > + const StreamConfiguration &config, > > > + Rectangle *rect) > > > +{ > > > + V4L2Subdevice *imgu = imguDevice->imgu; > > > + int ret; > > > + > > > + /* > > > + * Configure the 'imgu' subdevice with the requested sizes. > > > > s/'imgu'/ImgU/ ? > > > > > + * > > > + * FIXME: the IPU3 driver implementation shall be changed to use the > > > + * actual input sizes as 'imgu input' subdevice sizes, and use the > > > + * desired output sizes to configure the crop/compose rectangles. The > > > + * current implementation uses output sizes as 'imgu input' sizes, and > > > + * uses the input dimension to configure the crop/compose rectangles, > > > + * which contradicts the V4L2 specifications. > > > > s/specifications/specification/ > > > > > + */ > > > + ret = imgu->setCrop(ImgUDevice::PAD_INPUT, rect); > > > + if (ret) > > > + return ret; > > > + > > > + ret = imgu->setCompose(ImgUDevice::PAD_INPUT, rect); > > > + if (ret) > > > + return ret; > > > + > > > + LOG(IPU3, Debug) > > > + << "ImgU input feeder and BDS rectangle = (0,0)/" > > > + << rect->w << "x" << rect->h; > > > > toString() for our Rectangle class ? > > > > I'll add a patch > > > > + > > > + V4L2SubdeviceFormat imguFormat = {}; > > > + imguFormat.width = config.width; > > > + imguFormat.height = config.height; > > > + imguFormat.mbus_code = MEDIA_BUS_FMT_FIXED; > > > + > > > + ret = imgu->setFormat(ImgUDevice::PAD_INPUT, &imguFormat); > > > + if (ret) > > > + return ret; > > > + > > > + ret = imgu->setFormat(ImgUDevice::PAD_OUTPUT, &imguFormat); > > > + if (ret) > > > + return ret; > > > + > > > + LOG(IPU3, Debug) > > > + << "ImgU GDC format = " << imguFormat.toString(); > > > + > > > + ret = imgu->setFormat(ImgUDevice::PAD_VF, &imguFormat); > > > + if (ret) > > > + return ret; > > > + > > > + ret = imgu->setFormat(ImgUDevice::PAD_STAT, &imguFormat); > > > + if (ret) > > > + return ret; > > > + > > > + return 0; > > > +} > > > + > > > +int PipelineHandlerIPU3::setCIO2Format(CIO2Device *cio2Device, > > > + const StreamConfiguration &config, > > > + V4L2SubdeviceFormat *format) > > > +{ > > > > We're starting to get quite some code associated with the CIO2Device. I > > know I requested for CIO2Device to be a struct without methods, but I > > wonder if that was a mistake :-( > > > > I would also rename the function, maybe to configure(). setFormat() > > hints that it sets the format passed as an argument, which is not the > > case here, the format parameter is an output. > > > > > + unsigned int imageSize = config.width * config.height; > > > + V4L2Subdevice *sensor = cio2Device->sensor; > > > + V4L2Subdevice *csi2 = cio2Device->csi2; > > > + unsigned int best = ~0; > > > + bool found = false; > > > + int ret; > > > + > > > + const FormatEnum formats = sensor->formats(0); > > > + for (auto it = formats.begin(); it != formats.end(); ++it) { > > > + /* Only consider formats consumable by the CIO2 unit. */ > > > + int cio2Code = mediaBusToCIO2Format(it->first); > > > + if (cio2Code == -EINVAL) > > > > You could write > > > > if (mediaBusToCIO2Format(it->first) == -EINVAL) > > > > (and I would check for < 0 instead of == -EINVAL to avoid surprises if > > we later change that method) > > > > > + continue; > > > + > > > + for (const SizeRange &size : it->second) { > > > + /* > > > + * Only select formats bigger than the requested sizes > > > + * as the IPU3 cannot up-scale. > > > + */ > > > + if (size.maxWidth < config.width || > > > + size.maxHeight < config.height) > > > + continue; > > > + > > > + unsigned int diff = size.maxWidth * size.maxHeight > > > + - imageSize; > > > + if (diff >= best) > > > + continue; > > > + > > > + best = diff; > > > + found = true; > > > + > > > + format->width = size.maxWidth; > > > + format->height = size.maxHeight; > > > + format->mbus_code = it->first; > > > + } > > > + } > > > > Open question, should we do this, or always pick the largest size from > > the sensor and scale in the ImgU ? > > > > Good question... I cannot tell the pros and cons actually... why would > you think it is better? > To add a few elements, I looked into the xml files used to calculate the input, BDS and GDC sizes at: https://chromium.googlesource.com/chromiumos/overlays/board-overlays/+/master/baseboard-poppy/media-libs/cros-camera-hal-configs-poppy/files/gcss/ and it seems the sensor provided sizes are indeed selected matching the output and viewfinder sizes. How to calculate the GDB and BDS rectangle it's not clear at all at the moment to me, I'll get back to the linux-media list to require for more clarifications. Thanks j > Thanks > j > > > > + if (!found) { > > > + LOG(IPU3, Error) > > > + << "Unable to find image format suitable to produce: " > > > + << config.width << "x" << config.height > > > + << "- 0x" << std::hex << std::setfill('0') > > > > Spaces before and after dash, or none at all. > > > > > + << std::setw(8) << config.pixelFormat; > > > > Again a shame we can't use toString(). Do you use that helper at all ? > > :-) > > > > > + return -EINVAL; > > > + } > > > + > > > + /* Apply the selected format to the sensor and the CSI-2 receiver. */ > > > + ret = sensor->setFormat(0, format); > > > + if (ret) > > > + return ret; > > > + > > > + ret = csi2->setFormat(0, format); > > > + if (ret) > > > + return ret; > > > + > > > + LOG(IPU3, Debug) << "CIO2 Image format: " << format->toString(); > > > > Ah yes you use it here. > > > > > + > > > + return 0; > > > +} > > > + > > > /* > > > * Cameras are created associating an image sensor (represented by a > > > * media entity with function MEDIA_ENT_F_CAM_SENSOR) to one of the four > > > > -- > > Regards, > > > > Laurent Pinchart > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Jacopo, On Thu, Mar 21, 2019 at 08:31:12PM +0100, Jacopo Mondi wrote: > On Thu, Mar 21, 2019 at 11:54:18AM +0200, Laurent Pinchart wrote: > > On Wed, Mar 20, 2019 at 05:30:31PM +0100, Jacopo Mondi wrote: > >> Apply the requested image format to the sensor device, and apply the > >> adjusted one to the CIO2 device, the ImgU subdevice and its input and > >> output video devices. > >> > >> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > >> --- > >> src/libcamera/pipeline/ipu3/ipu3.cpp | 220 +++++++++++++++++++++++---- > >> 1 file changed, 189 insertions(+), 31 deletions(-) > >> > >> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > >> index 26fc56a76eb1..2975c218f6c9 100644 > >> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > >> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > >> @@ -16,6 +16,7 @@ > >> #include <libcamera/stream.h> > >> > >> #include "device_enumerator.h" > >> +#include "geometry.h" > >> #include "log.h" > >> #include "media_device.h" > >> #include "pipeline_handler.h" > >> @@ -30,6 +31,11 @@ LOG_DEFINE_CATEGORY(IPU3) > >> class ImgUDevice > >> { > >> public: > >> + static constexpr unsigned int PAD_INPUT = 0; > >> + static constexpr unsigned int PAD_OUTPUT = 2; > >> + static constexpr unsigned int PAD_VF = 3; > >> + static constexpr unsigned int PAD_STAT = 4; > >> + > >> ImgUDevice() > >> : imgu(nullptr), input(nullptr), output(nullptr), > >> viewfinder(nullptr), stat(nullptr) > >> @@ -135,6 +141,13 @@ private: > >> > >> int initImgU(ImgUDevice *imgu); > >> > >> + int setImguFormat(ImgUDevice *imguDevice, > >> + const StreamConfiguration &config, > >> + Rectangle *rect); > > > > This should be moved to the ImgU class. > > > >> + int setCIO2Format(CIO2Device *cio2Device, > >> + const StreamConfiguration &config, > >> + V4L2SubdeviceFormat *format); > > and this to the CIO2 class too? > > >> + > >> void registerCameras(); > >> > >> ImgUDevice imgus_[IPU3_IMGU_COUNT]; > >> @@ -202,60 +215,86 @@ int PipelineHandlerIPU3::configureStreams(Camera *camera, > >> std::map<Stream *, StreamConfiguration> &config) > >> { > >> IPU3CameraData *data = cameraData(camera); > >> - StreamConfiguration *cfg = &config[&data->stream_]; > >> - V4L2Subdevice *sensor = data->cio2.sensor; > >> - V4L2Subdevice *csi2 = data->cio2.csi2; > >> + const StreamConfiguration &cfg = config[&data->stream_]; > >> + V4L2Device *viewfinder = data->imgu->viewfinder; > >> + V4L2Device *output = data->imgu->output; > >> + V4L2Device *input = data->imgu->input; > >> V4L2Device *cio2 = data->cio2.output; > >> - V4L2SubdeviceFormat subdevFormat = {}; > >> - V4L2DeviceFormat devFormat = {}; > >> int ret; > >> > >> + LOG(IPU3, Info) > >> + << "Requested image format: " << cfg.width << "x" > >> + << cfg.height << " - " << std::hex << std::setw(8) > >> + << cfg.pixelFormat << " on camera:'" << camera->name() << "'"; > >> + > >> /* > >> - * FIXME: as of now, the format gets applied to the sensor and is > >> - * propagated along the pipeline. It should instead be applied on the > >> - * capture device and the sensor format calculated accordingly. > >> + * Verify that the requested size respects the IPU3 alignement > >> + * requirements: the image width shall be a multiple of 8 pixels and > >> + * its height a multiple of 4 pixels. > >> + * > >> + * \todo: consider the BDS scaling factor requirements: > >> + * "the downscaling factor must be an integer value multiple of 1/32" > >> */ > >> + if (cfg.width % 8 || cfg.height % 4) { > >> + LOG(IPU3, Error) << "Stream format not support: bad alignement"; > >> + return -EINVAL; > >> + } > >> > >> - ret = sensor->getFormat(0, &subdevFormat); > >> + /* > >> + * Pass the requested output image size to the sensor and get back the > >> + * adjusted one to be propagated to the CIO2 device and to the ImgU > >> + * input. > >> + */ > >> + V4L2SubdeviceFormat sensorFormat = {}; > >> + ret = setCIO2Format(&data->cio2, cfg, &sensorFormat); > >> if (ret) > >> return ret; > >> > >> - subdevFormat.width = cfg->width; > >> - subdevFormat.height = cfg->height; > >> - ret = sensor->setFormat(0, &subdevFormat); > >> + /* Apply the CIO2 image format to the CIO2 output and ImgU input. */ > >> + V4L2DeviceFormat cio2Format = {}; > >> + cio2Format.width = sensorFormat.width; > >> + cio2Format.height = sensorFormat.height; > >> + cio2Format.fourcc = mediaBusToCIO2Format(sensorFormat.mbus_code); > >> + cio2Format.planesCount = 1; > >> + ret = cio2->setFormat(&cio2Format); > > > > Shouldn't this be part of setCIO2Format() ? > > Moved there. I wanted to expose the fact the CIO2 output and the ImgU > input are configured with the same format. > > > >> if (ret) > >> return ret; > >> > >> - /* Return error if the requested format cannot be applied to sensor. */ > >> - if (subdevFormat.width != cfg->width || > >> - subdevFormat.height != cfg->height) { > >> - LOG(IPU3, Error) > >> - << "Failed to apply image format " > >> - << subdevFormat.width << "x" << subdevFormat.height > >> - << " - got: " << cfg->width << "x" << cfg->height; > >> - return -EINVAL; > >> - } > >> + LOG(IPU3, Debug) > >> + << "CIO2 output format = " << cio2Format.toString(); > >> > >> - ret = csi2->setFormat(0, &subdevFormat); > >> + ret = input->setFormat(&cio2Format); > >> if (ret) > >> return ret; > >> > >> - ret = cio2->getFormat(&devFormat); > >> + /* Apply pad formats and crop/compose rectangle to the ImgU. */ > >> + Rectangle rect = { > >> + .x = 0, > >> + .y = 0, > >> + .w = cio2Format.width, > >> + .h = cio2Format.height, > >> + }; > >> + ret = setImguFormat(data->imgu, cfg, &rect); > >> if (ret) > >> return ret; > >> > >> - devFormat.width = subdevFormat.width; > >> - devFormat.height = subdevFormat.height; > >> - devFormat.fourcc = cfg->pixelFormat; > >> + /* Apply the format to the ImgU output and viewfinder devices. */ > >> + V4L2DeviceFormat outputFormat = {}; > >> + outputFormat.width = cfg.width; > >> + outputFormat.height = cfg.height; > >> + outputFormat.fourcc = V4L2_PIX_FMT_NV12; > >> + outputFormat.planesCount = 2; > >> > >> - ret = cio2->setFormat(&devFormat); > >> + ret = output->setFormat(&outputFormat); > >> if (ret) > >> return ret; > >> > >> - LOG(IPU3, Info) << cio2->driverName() << ": " > >> - << devFormat.width << "x" << devFormat.height > >> - << "- 0x" << std::hex << devFormat.fourcc << " planes: " > >> - << devFormat.planes; > >> + LOG(IPU3, Debug) > >> + << "ImgU output format = " << outputFormat.toString(); > >> + > >> + ret = viewfinder->setFormat(&outputFormat); > >> + if (ret) > >> + return ret; > >> > >> return 0; > >> } > >> @@ -666,6 +705,125 @@ void PipelineHandlerIPU3::deleteCIO2(CIO2Device *cio2) > >> delete cio2->sensor; > >> } > >> > >> +int PipelineHandlerIPU3::setImguFormat(ImgUDevice *imguDevice, > >> + const StreamConfiguration &config, > >> + Rectangle *rect) > >> +{ > >> + V4L2Subdevice *imgu = imguDevice->imgu; > >> + int ret; > >> + > >> + /* > >> + * Configure the 'imgu' subdevice with the requested sizes. > > > > s/'imgu'/ImgU/ ? > > > >> + * > >> + * FIXME: the IPU3 driver implementation shall be changed to use the > >> + * actual input sizes as 'imgu input' subdevice sizes, and use the > >> + * desired output sizes to configure the crop/compose rectangles. The > >> + * current implementation uses output sizes as 'imgu input' sizes, and > >> + * uses the input dimension to configure the crop/compose rectangles, > >> + * which contradicts the V4L2 specifications. > > > > s/specifications/specification/ > > > >> + */ > >> + ret = imgu->setCrop(ImgUDevice::PAD_INPUT, rect); > >> + if (ret) > >> + return ret; > >> + > >> + ret = imgu->setCompose(ImgUDevice::PAD_INPUT, rect); > >> + if (ret) > >> + return ret; > >> + > >> + LOG(IPU3, Debug) > >> + << "ImgU input feeder and BDS rectangle = (0,0)/" > >> + << rect->w << "x" << rect->h; > > > > toString() for our Rectangle class ? > > I'll add a patch > > >> + > >> + V4L2SubdeviceFormat imguFormat = {}; > >> + imguFormat.width = config.width; > >> + imguFormat.height = config.height; > >> + imguFormat.mbus_code = MEDIA_BUS_FMT_FIXED; > >> + > >> + ret = imgu->setFormat(ImgUDevice::PAD_INPUT, &imguFormat); > >> + if (ret) > >> + return ret; > >> + > >> + ret = imgu->setFormat(ImgUDevice::PAD_OUTPUT, &imguFormat); > >> + if (ret) > >> + return ret; > >> + > >> + LOG(IPU3, Debug) > >> + << "ImgU GDC format = " << imguFormat.toString(); > >> + > >> + ret = imgu->setFormat(ImgUDevice::PAD_VF, &imguFormat); > >> + if (ret) > >> + return ret; > >> + > >> + ret = imgu->setFormat(ImgUDevice::PAD_STAT, &imguFormat); > >> + if (ret) > >> + return ret; > >> + > >> + return 0; > >> +} > >> + > >> +int PipelineHandlerIPU3::setCIO2Format(CIO2Device *cio2Device, > >> + const StreamConfiguration &config, > >> + V4L2SubdeviceFormat *format) > >> +{ > > > > We're starting to get quite some code associated with the CIO2Device. I > > know I requested for CIO2Device to be a struct without methods, but I > > wonder if that was a mistake :-( > > > > I would also rename the function, maybe to configure(). setFormat() > > hints that it sets the format passed as an argument, which is not the > > case here, the format parameter is an output. > > > >> + unsigned int imageSize = config.width * config.height; > >> + V4L2Subdevice *sensor = cio2Device->sensor; > >> + V4L2Subdevice *csi2 = cio2Device->csi2; > >> + unsigned int best = ~0; > >> + bool found = false; > >> + int ret; > >> + > >> + const FormatEnum formats = sensor->formats(0); > >> + for (auto it = formats.begin(); it != formats.end(); ++it) { > >> + /* Only consider formats consumable by the CIO2 unit. */ > >> + int cio2Code = mediaBusToCIO2Format(it->first); > >> + if (cio2Code == -EINVAL) > > > > You could write > > > > if (mediaBusToCIO2Format(it->first) == -EINVAL) > > > > (and I would check for < 0 instead of == -EINVAL to avoid surprises if > > we later change that method) > > > >> + continue; > >> + > >> + for (const SizeRange &size : it->second) { > >> + /* > >> + * Only select formats bigger than the requested sizes > >> + * as the IPU3 cannot up-scale. > >> + */ > >> + if (size.maxWidth < config.width || > >> + size.maxHeight < config.height) > >> + continue; > >> + > >> + unsigned int diff = size.maxWidth * size.maxHeight > >> + - imageSize; > >> + if (diff >= best) > >> + continue; > >> + > >> + best = diff; > >> + found = true; > >> + > >> + format->width = size.maxWidth; > >> + format->height = size.maxHeight; > >> + format->mbus_code = it->first; > >> + } > >> + } > > > > Open question, should we do this, or always pick the largest size from > > the sensor and scale in the ImgU ? > > > > Good question... I cannot tell the pros and cons actually... why would > you think it is better? The ImgU scaler should be of better quality than the sensor scaler, but scaling down on the sensor lowers the required bandwidth, and potentially increases the maximum frame rate. > >> + if (!found) { > >> + LOG(IPU3, Error) > >> + << "Unable to find image format suitable to produce: " > >> + << config.width << "x" << config.height > >> + << "- 0x" << std::hex << std::setfill('0') > > > > Spaces before and after dash, or none at all. > > > >> + << std::setw(8) << config.pixelFormat; > > > > Again a shame we can't use toString(). Do you use that helper at all ? > > :-) > > > >> + return -EINVAL; > >> + } > >> + > >> + /* Apply the selected format to the sensor and the CSI-2 receiver. */ > >> + ret = sensor->setFormat(0, format); > >> + if (ret) > >> + return ret; > >> + > >> + ret = csi2->setFormat(0, format); > >> + if (ret) > >> + return ret; > >> + > >> + LOG(IPU3, Debug) << "CIO2 Image format: " << format->toString(); > > > > Ah yes you use it here. > > > >> + > >> + return 0; > >> +} > >> + > >> /* > >> * Cameras are created associating an image sensor (represented by a > >> * media entity with function MEDIA_ENT_F_CAM_SENSOR) to one of the four
diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp index 26fc56a76eb1..2975c218f6c9 100644 --- a/src/libcamera/pipeline/ipu3/ipu3.cpp +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp @@ -16,6 +16,7 @@ #include <libcamera/stream.h> #include "device_enumerator.h" +#include "geometry.h" #include "log.h" #include "media_device.h" #include "pipeline_handler.h" @@ -30,6 +31,11 @@ LOG_DEFINE_CATEGORY(IPU3) class ImgUDevice { public: + static constexpr unsigned int PAD_INPUT = 0; + static constexpr unsigned int PAD_OUTPUT = 2; + static constexpr unsigned int PAD_VF = 3; + static constexpr unsigned int PAD_STAT = 4; + ImgUDevice() : imgu(nullptr), input(nullptr), output(nullptr), viewfinder(nullptr), stat(nullptr) @@ -135,6 +141,13 @@ private: int initImgU(ImgUDevice *imgu); + int setImguFormat(ImgUDevice *imguDevice, + const StreamConfiguration &config, + Rectangle *rect); + int setCIO2Format(CIO2Device *cio2Device, + const StreamConfiguration &config, + V4L2SubdeviceFormat *format); + void registerCameras(); ImgUDevice imgus_[IPU3_IMGU_COUNT]; @@ -202,60 +215,86 @@ int PipelineHandlerIPU3::configureStreams(Camera *camera, std::map<Stream *, StreamConfiguration> &config) { IPU3CameraData *data = cameraData(camera); - StreamConfiguration *cfg = &config[&data->stream_]; - V4L2Subdevice *sensor = data->cio2.sensor; - V4L2Subdevice *csi2 = data->cio2.csi2; + const StreamConfiguration &cfg = config[&data->stream_]; + V4L2Device *viewfinder = data->imgu->viewfinder; + V4L2Device *output = data->imgu->output; + V4L2Device *input = data->imgu->input; V4L2Device *cio2 = data->cio2.output; - V4L2SubdeviceFormat subdevFormat = {}; - V4L2DeviceFormat devFormat = {}; int ret; + LOG(IPU3, Info) + << "Requested image format: " << cfg.width << "x" + << cfg.height << " - " << std::hex << std::setw(8) + << cfg.pixelFormat << " on camera:'" << camera->name() << "'"; + /* - * FIXME: as of now, the format gets applied to the sensor and is - * propagated along the pipeline. It should instead be applied on the - * capture device and the sensor format calculated accordingly. + * Verify that the requested size respects the IPU3 alignement + * requirements: the image width shall be a multiple of 8 pixels and + * its height a multiple of 4 pixels. + * + * \todo: consider the BDS scaling factor requirements: + * "the downscaling factor must be an integer value multiple of 1/32" */ + if (cfg.width % 8 || cfg.height % 4) { + LOG(IPU3, Error) << "Stream format not support: bad alignement"; + return -EINVAL; + } - ret = sensor->getFormat(0, &subdevFormat); + /* + * Pass the requested output image size to the sensor and get back the + * adjusted one to be propagated to the CIO2 device and to the ImgU + * input. + */ + V4L2SubdeviceFormat sensorFormat = {}; + ret = setCIO2Format(&data->cio2, cfg, &sensorFormat); if (ret) return ret; - subdevFormat.width = cfg->width; - subdevFormat.height = cfg->height; - ret = sensor->setFormat(0, &subdevFormat); + /* Apply the CIO2 image format to the CIO2 output and ImgU input. */ + V4L2DeviceFormat cio2Format = {}; + cio2Format.width = sensorFormat.width; + cio2Format.height = sensorFormat.height; + cio2Format.fourcc = mediaBusToCIO2Format(sensorFormat.mbus_code); + cio2Format.planesCount = 1; + ret = cio2->setFormat(&cio2Format); if (ret) return ret; - /* Return error if the requested format cannot be applied to sensor. */ - if (subdevFormat.width != cfg->width || - subdevFormat.height != cfg->height) { - LOG(IPU3, Error) - << "Failed to apply image format " - << subdevFormat.width << "x" << subdevFormat.height - << " - got: " << cfg->width << "x" << cfg->height; - return -EINVAL; - } + LOG(IPU3, Debug) + << "CIO2 output format = " << cio2Format.toString(); - ret = csi2->setFormat(0, &subdevFormat); + ret = input->setFormat(&cio2Format); if (ret) return ret; - ret = cio2->getFormat(&devFormat); + /* Apply pad formats and crop/compose rectangle to the ImgU. */ + Rectangle rect = { + .x = 0, + .y = 0, + .w = cio2Format.width, + .h = cio2Format.height, + }; + ret = setImguFormat(data->imgu, cfg, &rect); if (ret) return ret; - devFormat.width = subdevFormat.width; - devFormat.height = subdevFormat.height; - devFormat.fourcc = cfg->pixelFormat; + /* Apply the format to the ImgU output and viewfinder devices. */ + V4L2DeviceFormat outputFormat = {}; + outputFormat.width = cfg.width; + outputFormat.height = cfg.height; + outputFormat.fourcc = V4L2_PIX_FMT_NV12; + outputFormat.planesCount = 2; - ret = cio2->setFormat(&devFormat); + ret = output->setFormat(&outputFormat); if (ret) return ret; - LOG(IPU3, Info) << cio2->driverName() << ": " - << devFormat.width << "x" << devFormat.height - << "- 0x" << std::hex << devFormat.fourcc << " planes: " - << devFormat.planes; + LOG(IPU3, Debug) + << "ImgU output format = " << outputFormat.toString(); + + ret = viewfinder->setFormat(&outputFormat); + if (ret) + return ret; return 0; } @@ -666,6 +705,125 @@ void PipelineHandlerIPU3::deleteCIO2(CIO2Device *cio2) delete cio2->sensor; } +int PipelineHandlerIPU3::setImguFormat(ImgUDevice *imguDevice, + const StreamConfiguration &config, + Rectangle *rect) +{ + V4L2Subdevice *imgu = imguDevice->imgu; + int ret; + + /* + * Configure the 'imgu' subdevice with the requested sizes. + * + * FIXME: the IPU3 driver implementation shall be changed to use the + * actual input sizes as 'imgu input' subdevice sizes, and use the + * desired output sizes to configure the crop/compose rectangles. The + * current implementation uses output sizes as 'imgu input' sizes, and + * uses the input dimension to configure the crop/compose rectangles, + * which contradicts the V4L2 specifications. + */ + ret = imgu->setCrop(ImgUDevice::PAD_INPUT, rect); + if (ret) + return ret; + + ret = imgu->setCompose(ImgUDevice::PAD_INPUT, rect); + if (ret) + return ret; + + LOG(IPU3, Debug) + << "ImgU input feeder and BDS rectangle = (0,0)/" + << rect->w << "x" << rect->h; + + V4L2SubdeviceFormat imguFormat = {}; + imguFormat.width = config.width; + imguFormat.height = config.height; + imguFormat.mbus_code = MEDIA_BUS_FMT_FIXED; + + ret = imgu->setFormat(ImgUDevice::PAD_INPUT, &imguFormat); + if (ret) + return ret; + + ret = imgu->setFormat(ImgUDevice::PAD_OUTPUT, &imguFormat); + if (ret) + return ret; + + LOG(IPU3, Debug) + << "ImgU GDC format = " << imguFormat.toString(); + + ret = imgu->setFormat(ImgUDevice::PAD_VF, &imguFormat); + if (ret) + return ret; + + ret = imgu->setFormat(ImgUDevice::PAD_STAT, &imguFormat); + if (ret) + return ret; + + return 0; +} + +int PipelineHandlerIPU3::setCIO2Format(CIO2Device *cio2Device, + const StreamConfiguration &config, + V4L2SubdeviceFormat *format) +{ + unsigned int imageSize = config.width * config.height; + V4L2Subdevice *sensor = cio2Device->sensor; + V4L2Subdevice *csi2 = cio2Device->csi2; + unsigned int best = ~0; + bool found = false; + int ret; + + const FormatEnum formats = sensor->formats(0); + for (auto it = formats.begin(); it != formats.end(); ++it) { + /* Only consider formats consumable by the CIO2 unit. */ + int cio2Code = mediaBusToCIO2Format(it->first); + if (cio2Code == -EINVAL) + continue; + + for (const SizeRange &size : it->second) { + /* + * Only select formats bigger than the requested sizes + * as the IPU3 cannot up-scale. + */ + if (size.maxWidth < config.width || + size.maxHeight < config.height) + continue; + + unsigned int diff = size.maxWidth * size.maxHeight + - imageSize; + if (diff >= best) + continue; + + best = diff; + found = true; + + format->width = size.maxWidth; + format->height = size.maxHeight; + format->mbus_code = it->first; + } + } + if (!found) { + LOG(IPU3, Error) + << "Unable to find image format suitable to produce: " + << config.width << "x" << config.height + << "- 0x" << std::hex << std::setfill('0') + << std::setw(8) << config.pixelFormat; + return -EINVAL; + } + + /* Apply the selected format to the sensor and the CSI-2 receiver. */ + ret = sensor->setFormat(0, format); + if (ret) + return ret; + + ret = csi2->setFormat(0, format); + if (ret) + return ret; + + LOG(IPU3, Debug) << "CIO2 Image format: " << format->toString(); + + return 0; +} + /* * Cameras are created associating an image sensor (represented by a * media entity with function MEDIA_ENT_F_CAM_SENSOR) to one of the four
Apply the requested image format to the sensor device, and apply the adjusted one to the CIO2 device, the ImgU subdevice and its input and output video devices. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- src/libcamera/pipeline/ipu3/ipu3.cpp | 220 +++++++++++++++++++++++---- 1 file changed, 189 insertions(+), 31 deletions(-)