| Message ID | 20190228200410.3022-5-jacopo@jmondi.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, Thank you for the patch. On Thu, Feb 28, 2019 at 09:04:04PM +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 | 257 +++++++++++++++++++++++---- > 1 file changed, 224 insertions(+), 33 deletions(-) > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index 9fa59c1bc97e..1e89e57f628b 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" > @@ -106,6 +109,29 @@ private: > PipelineHandler::cameraData(camera)); > } > > + void printDevFormat(const V4L2Device *dev, const V4L2DeviceFormat &fmt) > + { > + LOG(IPU3, Info) I think this is way too verbose in general. > + << dev->deviceNode() << ": " << fmt.width << "x" > + << fmt.height << "- 0x" << std::hex << fmt.fourcc > + << " planes: " << fmt.planesCount; > + } > + > + void printSubdevFormat(const V4L2Subdevice *dev, unsigned int pad, > + const V4L2SubdeviceFormat &fmt) > + { > + LOG(IPU3, Info) > + << "'" << dev->deviceName() << "':" << pad << " = " > + << fmt.width << "x" << fmt.height << " - 0x" > + << std::hex << fmt.mbus_code; > + } How about adding a std::string toString() const method to the V4L2DeviceFormat and V4L2SubdeviceFormat classes ? print methods in the PipelineHandlerIPU3 class aren't very nice :-/ (especially with the hardcoded log level) > + > + int setImguFormat(V4L2Subdevice *imgu, > + const StreamConfiguration &config, > + Rectangle *rect); > + int setSensorFormat(V4L2Subdevice *sensor, > + const StreamConfiguration &config, > + V4L2SubdeviceFormat *format); > int linkImgu(ImguDevice *imgu); > > V4L2Device *openDevice(MediaDevice *media, std::string &name); > @@ -186,14 +212,29 @@ int PipelineHandlerIPU3::configureStreams(Camera *camera, > std::map<Stream *, StreamConfiguration> &config) > { > IPU3CameraData *data = cameraData(camera); > - StreamConfiguration *cfg = &config[&data->stream_]; > - V4L2Subdevice *sensor = data->cio2.sensor; > + const StreamConfiguration &cfg = config[&data->stream_]; > V4L2Subdevice *csi2 = data->cio2.csi2; > V4L2Device *cio2 = data->cio2.output; > - V4L2SubdeviceFormat subdevFormat = {}; > - V4L2DeviceFormat devFormat = {}; > int ret; > > + /* > + * Verify that the requested size respects the IPU3 alignement > + * requirements: image size shall be 8-aligned in width and 4-aligned > + * in height. Maybe "the image width shall be a multiple of 8 pixels and its height a multiple of 4 pixels" to clarify we're talking about pixels, not bytes ? > + * > + * 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; > + } > + > + LOG(IPU3, Info) > + << "Camera :'" << camera->name() << " - Stream format: " > + << cfg.width << "x" << cfg.height << " - " > + << std::hex << cfg.pixelFormat; Logging is important, but this is all way too verbose. Please consider for every log message if it's needed, and if it is, what the appropriate log level should be. The messages should also be more informative. This message, for instance, should explicitly state that it prints the requested format, and also mention which stream it applies to. You should also move it before the alignment check. > + > /* > * TODO: dynamically assign ImgU devices; as of now, with a single > * stream supported, always use 'imgu0'. > @@ -203,52 +244,73 @@ int PipelineHandlerIPU3::configureStreams(Camera *camera, > if (ret) > return ret; > > + V4L2Device *viewfinder = data->imgu->viewfinder; > + V4L2Subdevice *sensor = data->cio2.sensor; > + V4L2Device *output = data->imgu->output; > + V4L2Subdevice *imgu = data->imgu->imgu; > + V4L2Device *input = data->imgu->input; > + > /* > - * 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. > + * 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. > */ If the sensor driver returns the closest match, you may get a size smaller than requested, which would require up-scaling. I'm not sure if the IPU3 supports that, but in any case it may not be a very good idea. I think you should instead iterate over the sizes supported by the sensor and pick the best one yourself. We may also want to use cropping at some point, but that can be left for later. And now that I've written this, I realized that your setSensorFormat() method has such an iteration. Could you update the above comment accordingly, and possibly rename setSensorFormat() to a more descriptive name ? > + V4L2SubdeviceFormat sensorFormat = {}; > + ret = setSensorFormat(sensor, cfg, &sensorFormat); > + if (ret) { > + LOG(IPU3, Error) << "Stream format not supported: "; Looks like you're missing part of the message. I would also move it to the setSensorFormat() function in order to print the exact reason of the error. > + return ret; > + } > + printSubdevFormat(sensor, 0, sensorFormat); > > - ret = sensor->getFormat(0, &subdevFormat); > + ret = csi2->setFormat(0, &sensorFormat); > if (ret) > return ret; > - > - subdevFormat.width = cfg->width; > - subdevFormat.height = cfg->height; > - ret = sensor->setFormat(0, &subdevFormat); > + printSubdevFormat(csi2, 0, sensorFormat); > + > + /* Apply the CIO2 image format to the CIO2 output and ImgU input. */ > + V4L2DeviceFormat cio2Format = {}; > + cio2Format.width = sensorFormat.width; > + cio2Format.height = sensorFormat.height; > + cio2Format.fourcc = V4L2_PIX_FMT_IPU3_SGRBG10; Let's not hardcode a particular bayer pattern, please pick the appropriate one based on what the sensor provides. > + cio2Format.planesCount = 1; > + ret = cio2->setFormat(&cio2Format); > if (ret) > return ret; > + printDevFormat(cio2, cio2Format); > > - /* 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; > - } > - > - ret = csi2->setFormat(0, &subdevFormat); > + ret = input->setFormat(&cio2Format); > if (ret) > return ret; > - > - ret = cio2->getFormat(&devFormat); > + printDevFormat(input, cio2Format); > + > + /* Apply pad formats and crop/compose rectangle to the ImgU. */ > + Rectangle rect = { > + .x = 0, > + .y = 0, > + .w = cio2Format.width, > + .h = cio2Format.height, > + }; > + ret = setImguFormat(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. */ As commented on the previous patch, with a single stream supported, do you really need both outputs ? > + 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; > + printDevFormat(output, outputFormat); > > - LOG(IPU3, Info) << cio2->driverName() << ": " > - << devFormat.width << "x" << devFormat.height > - << "- 0x" << std::hex << devFormat.fourcc << " planes: " > - << devFormat.planes; > + ret = viewfinder->setFormat(&outputFormat); > + if (ret) > + return ret; > + printDevFormat(viewfinder, outputFormat); > > return 0; > } > @@ -409,6 +471,135 @@ error_release_mdev: > return false; > } > > +int PipelineHandlerIPU3::setImguFormat(V4L2Subdevice *imgu, > + const StreamConfiguration &config, > + Rectangle *rect) > +{ > + 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(IMGU_PAD_INPUT, rect); > + if (ret) > + return ret; > + > + LOG(IPU3, Info) > + << "'" << imgu->deviceName() << "':" << IMGU_PAD_INPUT > + << " = crop: (0,0)/" << rect->w << "x" << rect->h; > + > + ret = imgu->setCompose(IMGU_PAD_INPUT, rect); > + if (ret) > + return ret; > + > + LOG(IPU3, Info) > + << "'" << imgu->deviceName() << "':" << IMGU_PAD_INPUT > + << " = compose: (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(IMGU_PAD_INPUT, &imguFormat); > + if (ret) > + return ret; > + printSubdevFormat(imgu, IMGU_PAD_INPUT, imguFormat); > + > + ret = imgu->setFormat(IMGU_PAD_OUTPUT, &imguFormat); > + if (ret) > + return ret; > + printSubdevFormat(imgu, IMGU_PAD_OUTPUT, imguFormat); > + > + ret = imgu->setFormat(IMGU_PAD_VF, &imguFormat); > + if (ret) > + return ret; > + printSubdevFormat(imgu, IMGU_PAD_VF, imguFormat); > + > + ret = imgu->setFormat(IMGU_PAD_STAT, &imguFormat); > + if (ret) > + return ret; > + printSubdevFormat(imgu, IMGU_PAD_STAT, imguFormat); > + > + return 0; > +} > + > +int PipelineHandlerIPU3::setSensorFormat(V4L2Subdevice *sensor, > + const StreamConfiguration &config, > + V4L2SubdeviceFormat *format) > +{ > + std::map<unsigned int, std::vector<SizeRange>> formats; > + unsigned int best = ~0; Please use -1 instead of ~0. > + bool found = false; > + int ret; > + > + formats = sensor->formats(0); > + if (formats.empty()) { > + /* > + * If the format list is empty, try with the currently > + * applied one. > + */ This shouldn't happen. > + ret = sensor->getFormat(0, format); > + if (ret) > + return ret; > + > + if (format->width < config.width || > + format->height < config.height) > + return -EINVAL; > + > + return 0; > + } > + > + /* Search for the best approximation the sensor can provide. */ > + auto it = formats.begin(); > + while (it != formats.end()) { for loop. > + for (SizeRange &size : it->second) { > + if (size.maxWidth < config.width || > + size.maxHeight < config.height) > + continue; > + > + unsigned int diff = > + (abs(size.maxWidth - config.width) + > + abs(size.maxHeight - config.height)); No need for abs() as you check that maxWidth >= config.width above. Should you use the difference of area instead of the difference of lengths ? > + if (diff >= best) > + continue; > + > + best = diff; > + found = true; No need for the found variable, you can just check best != -1. > + > + format->width = size.maxWidth; > + format->height = size.maxHeight; > + format->mbus_code = it->first; > + } > + > + ++it; > + } > + if (!found) > + return -EINVAL; > + > + ret = sensor->setFormat(0, format); > + if (ret) > + return ret; > + > + /* > + * Make sure everything is all right and the format did not get > + * adjusted. > + */ > + if (format->width < config.width || > + format->height < config.height) > + return -EINVAL; > + > + return 0; > +} > + > /* Link entities in the ImgU unit to prepare for capture operations. */ > int PipelineHandlerIPU3::linkImgu(ImguDevice *imguDevice) > {
Hi Laurent, On Sun, Mar 03, 2019 at 12:44:23AM +0200, Laurent Pinchart wrote: > Hi Jacopo, > > Thank you for the patch. > > On Thu, Feb 28, 2019 at 09:04:04PM +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 | 257 +++++++++++++++++++++++---- > > 1 file changed, 224 insertions(+), 33 deletions(-) > > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > > index 9fa59c1bc97e..1e89e57f628b 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" > > @@ -106,6 +109,29 @@ private: > > PipelineHandler::cameraData(camera)); > > } > > > > + void printDevFormat(const V4L2Device *dev, const V4L2DeviceFormat &fmt) > > + { > > + LOG(IPU3, Info) > > I think this is way too verbose in general. > > > + << dev->deviceNode() << ": " << fmt.width << "x" > > + << fmt.height << "- 0x" << std::hex << fmt.fourcc > > + << " planes: " << fmt.planesCount; > > + } > > + > > + void printSubdevFormat(const V4L2Subdevice *dev, unsigned int pad, > > + const V4L2SubdeviceFormat &fmt) > > + { > > + LOG(IPU3, Info) > > + << "'" << dev->deviceName() << "':" << pad << " = " > > + << fmt.width << "x" << fmt.height << " - 0x" > > + << std::hex << fmt.mbus_code; > > + } > > How about adding a std::string toString() const method to the > V4L2DeviceFormat and V4L2SubdeviceFormat classes ? print methods in the > PipelineHandlerIPU3 class aren't very nice :-/ (especially with the > hardcoded log level) > More code in the framework, less in pipeline handlers: it's a good idea! > > + > > + int setImguFormat(V4L2Subdevice *imgu, > > + const StreamConfiguration &config, > > + Rectangle *rect); > > + int setSensorFormat(V4L2Subdevice *sensor, > > + const StreamConfiguration &config, > > + V4L2SubdeviceFormat *format); > > int linkImgu(ImguDevice *imgu); > > > > V4L2Device *openDevice(MediaDevice *media, std::string &name); > > @@ -186,14 +212,29 @@ int PipelineHandlerIPU3::configureStreams(Camera *camera, > > std::map<Stream *, StreamConfiguration> &config) > > { > > IPU3CameraData *data = cameraData(camera); > > - StreamConfiguration *cfg = &config[&data->stream_]; > > - V4L2Subdevice *sensor = data->cio2.sensor; > > + const StreamConfiguration &cfg = config[&data->stream_]; > > V4L2Subdevice *csi2 = data->cio2.csi2; > > V4L2Device *cio2 = data->cio2.output; > > - V4L2SubdeviceFormat subdevFormat = {}; > > - V4L2DeviceFormat devFormat = {}; > > int ret; > > > > + /* > > + * Verify that the requested size respects the IPU3 alignement > > + * requirements: image size shall be 8-aligned in width and 4-aligned > > + * in height. > > Maybe "the image width shall be a multiple of 8 pixels and its height a > multiple of 4 pixels" to clarify we're talking about pixels, not bytes ? > Ack > > + * > > + * 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; > > + } > > + > > + LOG(IPU3, Info) > > + << "Camera :'" << camera->name() << " - Stream format: " > > + << cfg.width << "x" << cfg.height << " - " > > + << std::hex << cfg.pixelFormat; > > Logging is important, but this is all way too verbose. Please consider > for every log message if it's needed, and if it is, what the appropriate > log level should be. The messages should also be more informative. This > message, for instance, should explicitly state that it prints the > requested format, and also mention which stream it applies to. You > should also move it before the alignment check. As we have clarified offline I'll make this Debug and will make this and the others more user-consumable. > > > + > > /* > > * TODO: dynamically assign ImgU devices; as of now, with a single > > * stream supported, always use 'imgu0'. > > @@ -203,52 +244,73 @@ int PipelineHandlerIPU3::configureStreams(Camera *camera, > > if (ret) > > return ret; > > > > + V4L2Device *viewfinder = data->imgu->viewfinder; > > + V4L2Subdevice *sensor = data->cio2.sensor; > > + V4L2Device *output = data->imgu->output; > > + V4L2Subdevice *imgu = data->imgu->imgu; > > + V4L2Device *input = data->imgu->input; > > + > > /* > > - * 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. > > + * 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. > > */ > > If the sensor driver returns the closest match, you may get a size > smaller than requested, which would require up-scaling. I'm not sure if > the IPU3 supports that, but in any case it may not be a very good idea. > I think you should instead iterate over the sizes supported by the > sensor and pick the best one yourself. We may also want to use cropping > at some point, but that can be left for later. > > And now that I've written this, I realized that your setSensorFormat() > method has such an iteration. Could you update the above comment > accordingly, and possibly rename setSensorFormat() to a more descriptive > name ? Re-reading the comment, I got why you got mis-lead... I'll change this. Suggestions welcome for the method name. > > > + V4L2SubdeviceFormat sensorFormat = {}; > > + ret = setSensorFormat(sensor, cfg, &sensorFormat); > > + if (ret) { > > + LOG(IPU3, Error) << "Stream format not supported: "; > > Looks like you're missing part of the message. I would also move it to > the setSensorFormat() function in order to print the exact reason of the > error. > > > + return ret; > > + } > > + printSubdevFormat(sensor, 0, sensorFormat); > > > > - ret = sensor->getFormat(0, &subdevFormat); > > + ret = csi2->setFormat(0, &sensorFormat); > > if (ret) > > return ret; > > - > > - subdevFormat.width = cfg->width; > > - subdevFormat.height = cfg->height; > > - ret = sensor->setFormat(0, &subdevFormat); > > + printSubdevFormat(csi2, 0, sensorFormat); > > + > > + /* Apply the CIO2 image format to the CIO2 output and ImgU input. */ > > + V4L2DeviceFormat cio2Format = {}; > > + cio2Format.width = sensorFormat.width; > > + cio2Format.height = sensorFormat.height; > > + cio2Format.fourcc = V4L2_PIX_FMT_IPU3_SGRBG10; > > Let's not hardcode a particular bayer pattern, please pick the > appropriate one based on what the sensor provides. Ah! Isn't the output of the CIO2 device IPU3 specific and fixed? That's not what the sensor provides, but the CIO2 produced one (for which there are a few versions, which are different permutations of the RGB bayer components, but I don't think that' too relevant) > > > + cio2Format.planesCount = 1; > > + ret = cio2->setFormat(&cio2Format); > > if (ret) > > return ret; > > + printDevFormat(cio2, cio2Format); > > > > - /* 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; > > - } > > - > > - ret = csi2->setFormat(0, &subdevFormat); > > + ret = input->setFormat(&cio2Format); > > if (ret) > > return ret; > > - > > - ret = cio2->getFormat(&devFormat); > > + printDevFormat(input, cio2Format); > > + > > + /* Apply pad formats and crop/compose rectangle to the ImgU. */ > > + Rectangle rect = { > > + .x = 0, > > + .y = 0, > > + .w = cio2Format.width, > > + .h = cio2Format.height, > > + }; > > + ret = setImguFormat(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. */ > > As commented on the previous patch, with a single stream supported, do > you really need both outputs ? > See my comments on the previous patch. > > + 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; > > + printDevFormat(output, outputFormat); > > > > - LOG(IPU3, Info) << cio2->driverName() << ": " > > - << devFormat.width << "x" << devFormat.height > > - << "- 0x" << std::hex << devFormat.fourcc << " planes: " > > - << devFormat.planes; > > + ret = viewfinder->setFormat(&outputFormat); > > + if (ret) > > + return ret; > > + printDevFormat(viewfinder, outputFormat); > > > > return 0; > > } > > @@ -409,6 +471,135 @@ error_release_mdev: > > return false; > > } > > > > +int PipelineHandlerIPU3::setImguFormat(V4L2Subdevice *imgu, > > + const StreamConfiguration &config, > > + Rectangle *rect) > > +{ > > + 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(IMGU_PAD_INPUT, rect); > > + if (ret) > > + return ret; > > + > > + LOG(IPU3, Info) > > + << "'" << imgu->deviceName() << "':" << IMGU_PAD_INPUT > > + << " = crop: (0,0)/" << rect->w << "x" << rect->h; > > + > > + ret = imgu->setCompose(IMGU_PAD_INPUT, rect); > > + if (ret) > > + return ret; > > + > > + LOG(IPU3, Info) > > + << "'" << imgu->deviceName() << "':" << IMGU_PAD_INPUT > > + << " = compose: (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(IMGU_PAD_INPUT, &imguFormat); > > + if (ret) > > + return ret; > > + printSubdevFormat(imgu, IMGU_PAD_INPUT, imguFormat); > > + > > + ret = imgu->setFormat(IMGU_PAD_OUTPUT, &imguFormat); > > + if (ret) > > + return ret; > > + printSubdevFormat(imgu, IMGU_PAD_OUTPUT, imguFormat); > > + > > + ret = imgu->setFormat(IMGU_PAD_VF, &imguFormat); > > + if (ret) > > + return ret; > > + printSubdevFormat(imgu, IMGU_PAD_VF, imguFormat); > > + > > + ret = imgu->setFormat(IMGU_PAD_STAT, &imguFormat); > > + if (ret) > > + return ret; > > + printSubdevFormat(imgu, IMGU_PAD_STAT, imguFormat); > > + > > + return 0; > > +} > > + > > +int PipelineHandlerIPU3::setSensorFormat(V4L2Subdevice *sensor, > > + const StreamConfiguration &config, > > + V4L2SubdeviceFormat *format) > > +{ > > + std::map<unsigned int, std::vector<SizeRange>> formats; > > + unsigned int best = ~0; > > Please use -1 instead of ~0. > Ack. Personal tastes or some reason I'm missing? > > + bool found = false; > > + int ret; > > + > > + formats = sensor->formats(0); > > + if (formats.empty()) { > > + /* > > + * If the format list is empty, try with the currently > > + * applied one. > > + */ > > This shouldn't happen. > Agreed. Let's always assume sensor drivers are well written (creepy laughs in background..) > > + ret = sensor->getFormat(0, format); > > + if (ret) > > + return ret; > > + > > + if (format->width < config.width || > > + format->height < config.height) > > + return -EINVAL; > > + > > + return 0; > > + } > > + > > + /* Search for the best approximation the sensor can provide. */ > > + auto it = formats.begin(); > > + while (it != formats.end()) { > > for loop. > Ack > > + for (SizeRange &size : it->second) { > > + if (size.maxWidth < config.width || > > + size.maxHeight < config.height) > > + continue; > > + > > + unsigned int diff = > > + (abs(size.maxWidth - config.width) + > > + abs(size.maxHeight - config.height)); > > No need for abs() as you check that maxWidth >= config.width above. > > Should you use the difference of area instead of the difference of > lengths ? > I'll try to move most of this to geometry > > + if (diff >= best) > > + continue; > > + > > + best = diff; > > + found = true; > > No need for the found variable, you can just check best != -1. > Correct, this probably answers my above question. Thanks j > > + > > + format->width = size.maxWidth; > > + format->height = size.maxHeight; > > + format->mbus_code = it->first; > > + } > > + > > + ++it; > > + } > > + if (!found) > > + return -EINVAL; > > + > > + ret = sensor->setFormat(0, format); > > + if (ret) > > + return ret; > > + > > + /* > > + * Make sure everything is all right and the format did not get > > + * adjusted. > > + */ > > + if (format->width < config.width || > > + format->height < config.height) > > + return -EINVAL; > > + > > + return 0; > > +} > > + > > /* Link entities in the ImgU unit to prepare for capture operations. */ > > int PipelineHandlerIPU3::linkImgu(ImguDevice *imguDevice) > > { > > -- > Regards, > > Laurent Pinchart
Hi Jacopo, On Sat, Mar 09, 2019 at 09:58:16PM +0100, Jacopo Mondi wrote: > On Sun, Mar 03, 2019 at 12:44:23AM +0200, Laurent Pinchart wrote: > > On Thu, Feb 28, 2019 at 09:04:04PM +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 | 257 +++++++++++++++++++++++---- > >> 1 file changed, 224 insertions(+), 33 deletions(-) > >> > >> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > >> index 9fa59c1bc97e..1e89e57f628b 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" > >> @@ -106,6 +109,29 @@ private: > >> PipelineHandler::cameraData(camera)); > >> } > >> > >> + void printDevFormat(const V4L2Device *dev, const V4L2DeviceFormat &fmt) > >> + { > >> + LOG(IPU3, Info) > > > > I think this is way too verbose in general. > > > >> + << dev->deviceNode() << ": " << fmt.width << "x" > >> + << fmt.height << "- 0x" << std::hex << fmt.fourcc > >> + << " planes: " << fmt.planesCount; > >> + } > >> + > >> + void printSubdevFormat(const V4L2Subdevice *dev, unsigned int pad, > >> + const V4L2SubdeviceFormat &fmt) > >> + { > >> + LOG(IPU3, Info) > >> + << "'" << dev->deviceName() << "':" << pad << " = " > >> + << fmt.width << "x" << fmt.height << " - 0x" > >> + << std::hex << fmt.mbus_code; > >> + } > > > > How about adding a std::string toString() const method to the > > V4L2DeviceFormat and V4L2SubdeviceFormat classes ? print methods in the > > PipelineHandlerIPU3 class aren't very nice :-/ (especially with the > > hardcoded log level) > > More code in the framework, less in pipeline handlers: it's a good idea! > > >> + > >> + int setImguFormat(V4L2Subdevice *imgu, > >> + const StreamConfiguration &config, > >> + Rectangle *rect); > >> + int setSensorFormat(V4L2Subdevice *sensor, > >> + const StreamConfiguration &config, > >> + V4L2SubdeviceFormat *format); > >> int linkImgu(ImguDevice *imgu); > >> > >> V4L2Device *openDevice(MediaDevice *media, std::string &name); > >> @@ -186,14 +212,29 @@ int PipelineHandlerIPU3::configureStreams(Camera *camera, > >> std::map<Stream *, StreamConfiguration> &config) > >> { > >> IPU3CameraData *data = cameraData(camera); > >> - StreamConfiguration *cfg = &config[&data->stream_]; > >> - V4L2Subdevice *sensor = data->cio2.sensor; > >> + const StreamConfiguration &cfg = config[&data->stream_]; > >> V4L2Subdevice *csi2 = data->cio2.csi2; > >> V4L2Device *cio2 = data->cio2.output; > >> - V4L2SubdeviceFormat subdevFormat = {}; > >> - V4L2DeviceFormat devFormat = {}; > >> int ret; > >> > >> + /* > >> + * Verify that the requested size respects the IPU3 alignement > >> + * requirements: image size shall be 8-aligned in width and 4-aligned > >> + * in height. > > > > Maybe "the image width shall be a multiple of 8 pixels and its height a > > multiple of 4 pixels" to clarify we're talking about pixels, not bytes ? > > Ack > > >> + * > >> + * 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; > >> + } > >> + > >> + LOG(IPU3, Info) > >> + << "Camera :'" << camera->name() << " - Stream format: " > >> + << cfg.width << "x" << cfg.height << " - " > >> + << std::hex << cfg.pixelFormat; > > > > Logging is important, but this is all way too verbose. Please consider > > for every log message if it's needed, and if it is, what the appropriate > > log level should be. The messages should also be more informative. This > > message, for instance, should explicitly state that it prints the > > requested format, and also mention which stream it applies to. You > > should also move it before the alignment check. > > As we have clarified offline I'll make this Debug and will make this > and the others more user-consumable. > > >> + > >> /* > >> * TODO: dynamically assign ImgU devices; as of now, with a single > >> * stream supported, always use 'imgu0'. > >> @@ -203,52 +244,73 @@ int PipelineHandlerIPU3::configureStreams(Camera *camera, > >> if (ret) > >> return ret; > >> > >> + V4L2Device *viewfinder = data->imgu->viewfinder; > >> + V4L2Subdevice *sensor = data->cio2.sensor; > >> + V4L2Device *output = data->imgu->output; > >> + V4L2Subdevice *imgu = data->imgu->imgu; > >> + V4L2Device *input = data->imgu->input; > >> + > >> /* > >> - * 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. > >> + * 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. > >> */ > > > > If the sensor driver returns the closest match, you may get a size > > smaller than requested, which would require up-scaling. I'm not sure if > > the IPU3 supports that, but in any case it may not be a very good idea. > > I think you should instead iterate over the sizes supported by the > > sensor and pick the best one yourself. We may also want to use cropping > > at some point, but that can be left for later. > > > > And now that I've written this, I realized that your setSensorFormat() > > method has such an iteration. Could you update the above comment > > accordingly, and possibly rename setSensorFormat() to a more descriptive > > name ? > > Re-reading the comment, I got why you got mis-lead... I'll change > this. Suggestions welcome for the method name. > > >> + V4L2SubdeviceFormat sensorFormat = {}; > >> + ret = setSensorFormat(sensor, cfg, &sensorFormat); > >> + if (ret) { > >> + LOG(IPU3, Error) << "Stream format not supported: "; > > > > Looks like you're missing part of the message. I would also move it to > > the setSensorFormat() function in order to print the exact reason of the > > error. > > > >> + return ret; > >> + } > >> + printSubdevFormat(sensor, 0, sensorFormat); > >> > >> - ret = sensor->getFormat(0, &subdevFormat); > >> + ret = csi2->setFormat(0, &sensorFormat); > >> if (ret) > >> return ret; > >> - > >> - subdevFormat.width = cfg->width; > >> - subdevFormat.height = cfg->height; > >> - ret = sensor->setFormat(0, &subdevFormat); > >> + printSubdevFormat(csi2, 0, sensorFormat); > >> + > >> + /* Apply the CIO2 image format to the CIO2 output and ImgU input. */ > >> + V4L2DeviceFormat cio2Format = {}; > >> + cio2Format.width = sensorFormat.width; > >> + cio2Format.height = sensorFormat.height; > >> + cio2Format.fourcc = V4L2_PIX_FMT_IPU3_SGRBG10; > > > > Let's not hardcode a particular bayer pattern, please pick the > > appropriate one based on what the sensor provides. > > Ah! Isn't the output of the CIO2 device IPU3 specific and fixed? > That's not what the sensor provides, but the CIO2 produced one > (for which there are a few versions, which are different permutations > of the RGB bayer components, but I don't think that' too relevant) The CIO2 writes a custom packing of 10-bit Bayer data to memory (25 pixels stored in 32 bytes if I remember correctly). As far as I know it doesn't reorder Bayer components, so you'll have to pick the V4L2_PIX_FMT_IPU3_S[RGB]*10 variant that corresponds to the order produced by the sensor. > >> + cio2Format.planesCount = 1; > >> + ret = cio2->setFormat(&cio2Format); > >> if (ret) > >> return ret; > >> + printDevFormat(cio2, cio2Format); > >> > >> - /* 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; > >> - } > >> - > >> - ret = csi2->setFormat(0, &subdevFormat); > >> + ret = input->setFormat(&cio2Format); > >> if (ret) > >> return ret; > >> - > >> - ret = cio2->getFormat(&devFormat); > >> + printDevFormat(input, cio2Format); > >> + > >> + /* Apply pad formats and crop/compose rectangle to the ImgU. */ > >> + Rectangle rect = { > >> + .x = 0, > >> + .y = 0, > >> + .w = cio2Format.width, > >> + .h = cio2Format.height, > >> + }; > >> + ret = setImguFormat(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. */ > > > > As commented on the previous patch, with a single stream supported, do > > you really need both outputs ? > > See my comments on the previous patch. > > >> + 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; > >> + printDevFormat(output, outputFormat); > >> > >> - LOG(IPU3, Info) << cio2->driverName() << ": " > >> - << devFormat.width << "x" << devFormat.height > >> - << "- 0x" << std::hex << devFormat.fourcc << " planes: " > >> - << devFormat.planes; > >> + ret = viewfinder->setFormat(&outputFormat); > >> + if (ret) > >> + return ret; > >> + printDevFormat(viewfinder, outputFormat); > >> > >> return 0; > >> } > >> @@ -409,6 +471,135 @@ error_release_mdev: > >> return false; > >> } > >> > >> +int PipelineHandlerIPU3::setImguFormat(V4L2Subdevice *imgu, > >> + const StreamConfiguration &config, > >> + Rectangle *rect) > >> +{ > >> + 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(IMGU_PAD_INPUT, rect); > >> + if (ret) > >> + return ret; > >> + > >> + LOG(IPU3, Info) > >> + << "'" << imgu->deviceName() << "':" << IMGU_PAD_INPUT > >> + << " = crop: (0,0)/" << rect->w << "x" << rect->h; > >> + > >> + ret = imgu->setCompose(IMGU_PAD_INPUT, rect); > >> + if (ret) > >> + return ret; > >> + > >> + LOG(IPU3, Info) > >> + << "'" << imgu->deviceName() << "':" << IMGU_PAD_INPUT > >> + << " = compose: (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(IMGU_PAD_INPUT, &imguFormat); > >> + if (ret) > >> + return ret; > >> + printSubdevFormat(imgu, IMGU_PAD_INPUT, imguFormat); > >> + > >> + ret = imgu->setFormat(IMGU_PAD_OUTPUT, &imguFormat); > >> + if (ret) > >> + return ret; > >> + printSubdevFormat(imgu, IMGU_PAD_OUTPUT, imguFormat); > >> + > >> + ret = imgu->setFormat(IMGU_PAD_VF, &imguFormat); > >> + if (ret) > >> + return ret; > >> + printSubdevFormat(imgu, IMGU_PAD_VF, imguFormat); > >> + > >> + ret = imgu->setFormat(IMGU_PAD_STAT, &imguFormat); > >> + if (ret) > >> + return ret; > >> + printSubdevFormat(imgu, IMGU_PAD_STAT, imguFormat); > >> + > >> + return 0; > >> +} > >> + > >> +int PipelineHandlerIPU3::setSensorFormat(V4L2Subdevice *sensor, > >> + const StreamConfiguration &config, > >> + V4L2SubdeviceFormat *format) > >> +{ > >> + std::map<unsigned int, std::vector<SizeRange>> formats; > >> + unsigned int best = ~0; > > > > Please use -1 instead of ~0. > > Ack. Personal tastes or some reason I'm missing? > > >> + bool found = false; > >> + int ret; > >> + > >> + formats = sensor->formats(0); > >> + if (formats.empty()) { > >> + /* > >> + * If the format list is empty, try with the currently > >> + * applied one. > >> + */ > > > > This shouldn't happen. > > Agreed. Let's always assume sensor drivers are well written (creepy > laughs in background..) :-) Well, sanity checks are nice, but I'd move them to match() time to fail early instead of creating a camera that won't be able to work. Some checks could even be moved to the V4L2 device and subdevice classes. > >> + ret = sensor->getFormat(0, format); > >> + if (ret) > >> + return ret; > >> + > >> + if (format->width < config.width || > >> + format->height < config.height) > >> + return -EINVAL; > >> + > >> + return 0; > >> + } > >> + > >> + /* Search for the best approximation the sensor can provide. */ > >> + auto it = formats.begin(); > >> + while (it != formats.end()) { > > > > for loop. > > Ack > > >> + for (SizeRange &size : it->second) { > >> + if (size.maxWidth < config.width || > >> + size.maxHeight < config.height) > >> + continue; > >> + > >> + unsigned int diff = > >> + (abs(size.maxWidth - config.width) + > >> + abs(size.maxHeight - config.height)); > > > > No need for abs() as you check that maxWidth >= config.width above. > > > > Should you use the difference of area instead of the difference of > > lengths ? > > I'll try to move most of this to geometry > > >> + if (diff >= best) > >> + continue; > >> + > >> + best = diff; > >> + found = true; > > > > No need for the found variable, you can just check best != -1. > > Correct, this probably answers my above question. > > >> + > >> + format->width = size.maxWidth; > >> + format->height = size.maxHeight; > >> + format->mbus_code = it->first; > >> + } > >> + > >> + ++it; > >> + } > >> + if (!found) > >> + return -EINVAL; > >> + > >> + ret = sensor->setFormat(0, format); > >> + if (ret) > >> + return ret; > >> + > >> + /* > >> + * Make sure everything is all right and the format did not get > >> + * adjusted. > >> + */ > >> + if (format->width < config.width || > >> + format->height < config.height) > >> + return -EINVAL; > >> + > >> + return 0; > >> +} > >> + > >> /* Link entities in the ImgU unit to prepare for capture operations. */ > >> int PipelineHandlerIPU3::linkImgu(ImguDevice *imguDevice) > >> {
Hi Laurent, two updates so that v2 comes with no surprises On Sun, Mar 03, 2019 at 12:44:23AM +0200, Laurent Pinchart wrote: > Hi Jacopo, > > Thank you for the patch. > [snip] > > + for (SizeRange &size : it->second) { > > + if (size.maxWidth < config.width || > > + size.maxHeight < config.height) > > + continue; > > + > > + unsigned int diff = > > + (abs(size.maxWidth - config.width) + > > + abs(size.maxHeight - config.height)); > > No need for abs() as you check that maxWidth >= config.width above. > > Should you use the difference of area instead of the difference of > lengths ? > I suggested moving this to geometry, but as in this pipeline implementation we often compare SizeRanges with other structure, it's actually nicer to calculate areas here directly. It's a basic multiplication after all... > > + if (diff >= best) > > + continue; > > + > > + best = diff; > > + found = true; > > No need for the found variable, you can just check best != -1. > I went down a path of casts between signed and unsigned int this way. I'm keeping the extra variable. Thanks j > > + > > + format->width = size.maxWidth; > > + format->height = size.maxHeight; > > + format->mbus_code = it->first; > > + } > > + > > + ++it; > > + } > > + if (!found) > > + return -EINVAL; > > + > > + ret = sensor->setFormat(0, format); > > + if (ret) > > + return ret; > > + > > + /* > > + * Make sure everything is all right and the format did not get > > + * adjusted. > > + */ > > + if (format->width < config.width || > > + format->height < config.height) > > + return -EINVAL; > > + > > + return 0; > > +} > > + > > /* Link entities in the ImgU unit to prepare for capture operations. */ > > int PipelineHandlerIPU3::linkImgu(ImguDevice *imguDevice) > > { > > -- > Regards, > > Laurent Pinchart
diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp index 9fa59c1bc97e..1e89e57f628b 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" @@ -106,6 +109,29 @@ private: PipelineHandler::cameraData(camera)); } + void printDevFormat(const V4L2Device *dev, const V4L2DeviceFormat &fmt) + { + LOG(IPU3, Info) + << dev->deviceNode() << ": " << fmt.width << "x" + << fmt.height << "- 0x" << std::hex << fmt.fourcc + << " planes: " << fmt.planesCount; + } + + void printSubdevFormat(const V4L2Subdevice *dev, unsigned int pad, + const V4L2SubdeviceFormat &fmt) + { + LOG(IPU3, Info) + << "'" << dev->deviceName() << "':" << pad << " = " + << fmt.width << "x" << fmt.height << " - 0x" + << std::hex << fmt.mbus_code; + } + + int setImguFormat(V4L2Subdevice *imgu, + const StreamConfiguration &config, + Rectangle *rect); + int setSensorFormat(V4L2Subdevice *sensor, + const StreamConfiguration &config, + V4L2SubdeviceFormat *format); int linkImgu(ImguDevice *imgu); V4L2Device *openDevice(MediaDevice *media, std::string &name); @@ -186,14 +212,29 @@ int PipelineHandlerIPU3::configureStreams(Camera *camera, std::map<Stream *, StreamConfiguration> &config) { IPU3CameraData *data = cameraData(camera); - StreamConfiguration *cfg = &config[&data->stream_]; - V4L2Subdevice *sensor = data->cio2.sensor; + const StreamConfiguration &cfg = config[&data->stream_]; V4L2Subdevice *csi2 = data->cio2.csi2; V4L2Device *cio2 = data->cio2.output; - V4L2SubdeviceFormat subdevFormat = {}; - V4L2DeviceFormat devFormat = {}; int ret; + /* + * Verify that the requested size respects the IPU3 alignement + * requirements: image size shall be 8-aligned in width and 4-aligned + * in height. + * + * 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; + } + + LOG(IPU3, Info) + << "Camera :'" << camera->name() << " - Stream format: " + << cfg.width << "x" << cfg.height << " - " + << std::hex << cfg.pixelFormat; + /* * TODO: dynamically assign ImgU devices; as of now, with a single * stream supported, always use 'imgu0'. @@ -203,52 +244,73 @@ int PipelineHandlerIPU3::configureStreams(Camera *camera, if (ret) return ret; + V4L2Device *viewfinder = data->imgu->viewfinder; + V4L2Subdevice *sensor = data->cio2.sensor; + V4L2Device *output = data->imgu->output; + V4L2Subdevice *imgu = data->imgu->imgu; + V4L2Device *input = data->imgu->input; + /* - * 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. + * 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 = setSensorFormat(sensor, cfg, &sensorFormat); + if (ret) { + LOG(IPU3, Error) << "Stream format not supported: "; + return ret; + } + printSubdevFormat(sensor, 0, sensorFormat); - ret = sensor->getFormat(0, &subdevFormat); + ret = csi2->setFormat(0, &sensorFormat); if (ret) return ret; - - subdevFormat.width = cfg->width; - subdevFormat.height = cfg->height; - ret = sensor->setFormat(0, &subdevFormat); + printSubdevFormat(csi2, 0, sensorFormat); + + /* Apply the CIO2 image format to the CIO2 output and ImgU input. */ + V4L2DeviceFormat cio2Format = {}; + cio2Format.width = sensorFormat.width; + cio2Format.height = sensorFormat.height; + cio2Format.fourcc = V4L2_PIX_FMT_IPU3_SGRBG10; + cio2Format.planesCount = 1; + ret = cio2->setFormat(&cio2Format); if (ret) return ret; + printDevFormat(cio2, cio2Format); - /* 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; - } - - ret = csi2->setFormat(0, &subdevFormat); + ret = input->setFormat(&cio2Format); if (ret) return ret; - - ret = cio2->getFormat(&devFormat); + printDevFormat(input, cio2Format); + + /* Apply pad formats and crop/compose rectangle to the ImgU. */ + Rectangle rect = { + .x = 0, + .y = 0, + .w = cio2Format.width, + .h = cio2Format.height, + }; + ret = setImguFormat(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; + printDevFormat(output, outputFormat); - LOG(IPU3, Info) << cio2->driverName() << ": " - << devFormat.width << "x" << devFormat.height - << "- 0x" << std::hex << devFormat.fourcc << " planes: " - << devFormat.planes; + ret = viewfinder->setFormat(&outputFormat); + if (ret) + return ret; + printDevFormat(viewfinder, outputFormat); return 0; } @@ -409,6 +471,135 @@ error_release_mdev: return false; } +int PipelineHandlerIPU3::setImguFormat(V4L2Subdevice *imgu, + const StreamConfiguration &config, + Rectangle *rect) +{ + 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(IMGU_PAD_INPUT, rect); + if (ret) + return ret; + + LOG(IPU3, Info) + << "'" << imgu->deviceName() << "':" << IMGU_PAD_INPUT + << " = crop: (0,0)/" << rect->w << "x" << rect->h; + + ret = imgu->setCompose(IMGU_PAD_INPUT, rect); + if (ret) + return ret; + + LOG(IPU3, Info) + << "'" << imgu->deviceName() << "':" << IMGU_PAD_INPUT + << " = compose: (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(IMGU_PAD_INPUT, &imguFormat); + if (ret) + return ret; + printSubdevFormat(imgu, IMGU_PAD_INPUT, imguFormat); + + ret = imgu->setFormat(IMGU_PAD_OUTPUT, &imguFormat); + if (ret) + return ret; + printSubdevFormat(imgu, IMGU_PAD_OUTPUT, imguFormat); + + ret = imgu->setFormat(IMGU_PAD_VF, &imguFormat); + if (ret) + return ret; + printSubdevFormat(imgu, IMGU_PAD_VF, imguFormat); + + ret = imgu->setFormat(IMGU_PAD_STAT, &imguFormat); + if (ret) + return ret; + printSubdevFormat(imgu, IMGU_PAD_STAT, imguFormat); + + return 0; +} + +int PipelineHandlerIPU3::setSensorFormat(V4L2Subdevice *sensor, + const StreamConfiguration &config, + V4L2SubdeviceFormat *format) +{ + std::map<unsigned int, std::vector<SizeRange>> formats; + unsigned int best = ~0; + bool found = false; + int ret; + + formats = sensor->formats(0); + if (formats.empty()) { + /* + * If the format list is empty, try with the currently + * applied one. + */ + ret = sensor->getFormat(0, format); + if (ret) + return ret; + + if (format->width < config.width || + format->height < config.height) + return -EINVAL; + + return 0; + } + + /* Search for the best approximation the sensor can provide. */ + auto it = formats.begin(); + while (it != formats.end()) { + for (SizeRange &size : it->second) { + if (size.maxWidth < config.width || + size.maxHeight < config.height) + continue; + + unsigned int diff = + (abs(size.maxWidth - config.width) + + abs(size.maxHeight - config.height)); + if (diff >= best) + continue; + + best = diff; + found = true; + + format->width = size.maxWidth; + format->height = size.maxHeight; + format->mbus_code = it->first; + } + + ++it; + } + if (!found) + return -EINVAL; + + ret = sensor->setFormat(0, format); + if (ret) + return ret; + + /* + * Make sure everything is all right and the format did not get + * adjusted. + */ + if (format->width < config.width || + format->height < config.height) + return -EINVAL; + + return 0; +} + /* Link entities in the ImgU unit to prepare for capture operations. */ int PipelineHandlerIPU3::linkImgu(ImguDevice *imguDevice) {
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 | 257 +++++++++++++++++++++++---- 1 file changed, 224 insertions(+), 33 deletions(-)