[{"id":1175,"web_url":"https://patchwork.libcamera.org/comment/1175/","msgid":"<20190402090758.GE4805@pendragon.ideasonboard.com>","date":"2019-04-02T09:07:58","subject":"Re: [libcamera-devel] [PATCH v5 10/19] libcamera: ipu3: Create\n\tCIO2Device class","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the patch.\n\nOn Tue, Mar 26, 2019 at 09:38:53AM +0100, Jacopo Mondi wrote:\n> Group CIO2 components (cio2, csi2 and image sensor) in a class\n> associated with the CameraData, to ease management and initialization of\n> CIO2 unit at camera registration time. A CIO2 unit will always be associated\n> with a single Camera only.\n\nBy the way, aren't commit messages supposed to be wrapped at 72 columns\n?\n\n> \n> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>\n> ---\n>  src/libcamera/pipeline/ipu3/ipu3.cpp | 320 +++++++++++++++------------\n>  1 file changed, 175 insertions(+), 145 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 04cd02653711..21205b39afee 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -44,6 +44,32 @@ static int mediaBusToCIO2Format(unsigned int code)\n>  \t}\n>  }\n>  \n> +class CIO2Device\n> +{\n> +public:\n> +\tCIO2Device()\n> +\t\t: output_(nullptr), csi2_(nullptr), sensor_(nullptr)\n> +\t{\n> +\t}\n> +\n> +\t~CIO2Device()\n> +\t{\n> +\t\tdelete output_;\n> +\t\tdelete csi2_;\n> +\t\tdelete sensor_;\n> +\t}\n> +\n> +\tconst std::string &name() const;\n> +\tint init(const MediaDevice *media, unsigned int index);\n\nI would put init() first, to declare methods in the same order as\nthey're supposed to be called.\n\n> +\n> +\tV4L2Device *output_;\n> +\tV4L2Subdevice *csi2_;\n> +\tV4L2Subdevice *sensor_;\n> +\n> +\t/* Maximum sizes and the mbus code used to produce them. */\n> +\tstd::pair<unsigned int, SizeRange> maxSizes_;\n> +};\n> +\n>  class PipelineHandlerIPU3 : public PipelineHandler\n>  {\n>  public:\n> @@ -67,36 +93,23 @@ public:\n>  \tbool match(DeviceEnumerator *enumerator);\n>  \n>  private:\n> +\tstatic constexpr unsigned int IPU3_BUFFER_COUNT = 4;\n> +\n>  \tclass IPU3CameraData : public CameraData\n>  \t{\n>  \tpublic:\n>  \t\tIPU3CameraData(PipelineHandler *pipe)\n> -\t\t\t: CameraData(pipe), cio2_(nullptr), csi2_(nullptr),\n> -\t\t\t  sensor_(nullptr)\n> -\t\t{\n> -\t\t}\n> -\n> -\t\t~IPU3CameraData()\n> +\t\t\t: CameraData(pipe)\n>  \t\t{\n> -\t\t\tdelete cio2_;\n> -\t\t\tdelete csi2_;\n> -\t\t\tdelete sensor_;\n>  \t\t}\n>  \n>  \t\tvoid bufferReady(Buffer *buffer);\n>  \n> -\t\tV4L2Device *cio2_;\n> -\t\tV4L2Subdevice *csi2_;\n> -\t\tV4L2Subdevice *sensor_;\n> +\t\tCIO2Device cio2_;\n>  \n>  \t\tStream stream_;\n> -\n> -\t\t/* Maximum sizes and the mbus code used to produce them. */\n> -\t\tstd::pair<unsigned int, SizeRange> maxSizes_;\n>  \t};\n>  \n> -\tstatic constexpr unsigned int IPU3_BUFFER_COUNT = 4;\n> -\n>  \tIPU3CameraData *cameraData(const Camera *camera)\n>  \t{\n>  \t\treturn static_cast<IPU3CameraData *>(\n> @@ -105,22 +118,22 @@ private:\n>  \n>  \tvoid registerCameras();\n>  \n> -\tstd::shared_ptr<MediaDevice> cio2_;\n> -\tstd::shared_ptr<MediaDevice> imgu_;\n> +\tstd::shared_ptr<MediaDevice> cio2MediaDev_;\n> +\tstd::shared_ptr<MediaDevice> imguMediaDev_;\n\nI still dislike those longer new names, but I can live with our\ndisagreement on this topic, so please keep them :-)\n\n>  };\n>  \n>  PipelineHandlerIPU3::PipelineHandlerIPU3(CameraManager *manager)\n> -\t: PipelineHandler(manager), cio2_(nullptr), imgu_(nullptr)\n> +\t: PipelineHandler(manager), cio2MediaDev_(nullptr), imguMediaDev_(nullptr)\n>  {\n>  }\n>  \n>  PipelineHandlerIPU3::~PipelineHandlerIPU3()\n>  {\n> -\tif (cio2_)\n> -\t\tcio2_->release();\n> +\tif (cio2MediaDev_)\n> +\t\tcio2MediaDev_->release();\n>  \n> -\tif (imgu_)\n> -\t\timgu_->release();\n> +\tif (imguMediaDev_)\n> +\t\timguMediaDev_->release();\n>  }\n>  \n>  std::map<Stream *, StreamConfiguration>\n> @@ -130,11 +143,12 @@ PipelineHandlerIPU3::streamConfiguration(Camera *camera,\n>  \tstd::map<Stream *, StreamConfiguration> configs;\n>  \tIPU3CameraData *data = cameraData(camera);\n>  \tStreamConfiguration *config = &configs[&data->stream_];\n> -\tSizeRange &maxRange = data->maxSizes_.second;\n> +\tCIO2Device *cio2 = &data->cio2_;\n\nYou could make this a const reference instead of a pointer (const isn't\nstrictly required, but as this function isn't supposed to modify\ndata->cio2_, it would help catching bugs).\n\n> +\tSizeRange &maxRange = cio2->maxSizes_.second;\n\nSame here, this could be const. If you prefer pointers that's OK too,\nbut I would then make both cio2 and maxRange pointers for consistency.\n\n>  \tconfig->width = maxRange.maxWidth;\n>  \tconfig->height = maxRange.maxHeight;\n> -\tconfig->pixelFormat = data->maxSizes_.first;\n> +\tconfig->pixelFormat = cio2->maxSizes_.first;\n>  \tconfig->bufferCount = IPU3_BUFFER_COUNT;\n>  \n>  \tLOG(IPU3, Debug)\n> @@ -150,9 +164,9 @@ int PipelineHandlerIPU3::configureStreams(Camera *camera,\n>  {\n>  \tIPU3CameraData *data = cameraData(camera);\n>  \tStreamConfiguration *cfg = &config[&data->stream_];\n> -\tV4L2Subdevice *sensor = data->sensor_;\n> -\tV4L2Subdevice *csi2 = data->csi2_;\n> -\tV4L2Device *cio2 = data->cio2_;\n> +\tV4L2Subdevice *sensor = data->cio2_.sensor_;\n> +\tV4L2Subdevice *csi2 = data->cio2_.csi2_;\n> +\tV4L2Device *cio2 = data->cio2_.output_;\n>  \tV4L2SubdeviceFormat subdevFormat = {};\n>  \tV4L2DeviceFormat devFormat = {};\n>  \tint ret;\n> @@ -209,13 +223,14 @@ int PipelineHandlerIPU3::configureStreams(Camera *camera,\n>  \n>  int PipelineHandlerIPU3::allocateBuffers(Camera *camera, Stream *stream)\n>  {\n> -\tIPU3CameraData *data = cameraData(camera);\n>  \tconst StreamConfiguration &cfg = stream->configuration();\n> +\tIPU3CameraData *data = cameraData(camera);\n\nIs there a need for the reordering ?\n\n> +\tV4L2Device *cio2 = data->cio2_.output_;\n>  \n>  \tif (!cfg.bufferCount)\n>  \t\treturn -EINVAL;\n>  \n> -\tint ret = data->cio2_->exportBuffers(&stream->bufferPool());\n> +\tint ret = cio2->exportBuffers(&stream->bufferPool());\n>  \tif (ret) {\n>  \t\tLOG(IPU3, Error) << \"Failed to request memory\";\n>  \t\treturn ret;\n> @@ -227,8 +242,9 @@ int PipelineHandlerIPU3::allocateBuffers(Camera *camera, Stream *stream)\n>  int PipelineHandlerIPU3::freeBuffers(Camera *camera, Stream *stream)\n>  {\n>  \tIPU3CameraData *data = cameraData(camera);\n> +\tV4L2Device *cio2 = data->cio2_.output_;\n>  \n> -\tint ret = data->cio2_->releaseBuffers();\n> +\tint ret = cio2->releaseBuffers();\n>  \tif (ret) {\n>  \t\tLOG(IPU3, Error) << \"Failed to release memory\";\n>  \t\treturn ret;\n> @@ -240,9 +256,10 @@ int PipelineHandlerIPU3::freeBuffers(Camera *camera, Stream *stream)\n>  int PipelineHandlerIPU3::start(Camera *camera)\n>  {\n>  \tIPU3CameraData *data = cameraData(camera);\n> +\tV4L2Device *cio2 = data->cio2_.output_;\n>  \tint ret;\n>  \n> -\tret = data->cio2_->streamOn();\n> +\tret = cio2->streamOn();\n>  \tif (ret) {\n>  \t\tLOG(IPU3, Info) << \"Failed to start camera \" << camera->name();\n>  \t\treturn ret;\n> @@ -254,8 +271,9 @@ int PipelineHandlerIPU3::start(Camera *camera)\n>  void PipelineHandlerIPU3::stop(Camera *camera)\n>  {\n>  \tIPU3CameraData *data = cameraData(camera);\n> +\tV4L2Device *cio2 = data->cio2_.output_;\n>  \n> -\tif (data->cio2_->streamOff())\n> +\tif (cio2->streamOff())\n>  \t\tLOG(IPU3, Info) << \"Failed to stop camera \" << camera->name();\n>  \n>  \tPipelineHandler::stop(camera);\n> @@ -264,6 +282,7 @@ void PipelineHandlerIPU3::stop(Camera *camera)\n>  int PipelineHandlerIPU3::queueRequest(Camera *camera, Request *request)\n>  {\n>  \tIPU3CameraData *data = cameraData(camera);\n> +\tV4L2Device *cio2 = data->cio2_.output_;\n>  \tStream *stream = &data->stream_;\n>  \n>  \tBuffer *buffer = request->findBuffer(stream);\n> @@ -273,7 +292,7 @@ int PipelineHandlerIPU3::queueRequest(Camera *camera, Request *request)\n>  \t\treturn -ENOENT;\n>  \t}\n>  \n> -\tint ret = data->cio2_->queueBuffer(buffer);\n> +\tint ret = cio2->queueBuffer(buffer);\n>  \tif (ret < 0)\n>  \t\treturn ret;\n>  \n> @@ -312,17 +331,17 @@ bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator)\n>  \t * It is safe to acquire both media devices at this point as\n>  \t * DeviceEnumerator::search() skips the busy ones for us.\n>  \t */\n> -\tcio2_ = enumerator->search(cio2_dm);\n> -\tif (!cio2_)\n> +\tcio2MediaDev_ = enumerator->search(cio2_dm);\n> +\tif (!cio2MediaDev_)\n>  \t\treturn false;\n>  \n> -\tcio2_->acquire();\n> +\tcio2MediaDev_->acquire();\n>  \n> -\timgu_ = enumerator->search(imgu_dm);\n> -\tif (!imgu_)\n> +\timguMediaDev_ = enumerator->search(imgu_dm);\n> +\tif (!imguMediaDev_)\n>  \t\treturn false;\n>  \n> -\timgu_->acquire();\n> +\timguMediaDev_->acquire();\n>  \n>  \t/*\n>  \t * Disable all links that are enabled by default on CIO2, as camera\n> @@ -331,17 +350,17 @@ bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator)\n>  \t * Close the CIO2 media device after, as links are enabled and should\n>  \t * not need to be changed after.\n>  \t */\n> -\tif (cio2_->open())\n> +\tif (cio2MediaDev_->open())\n>  \t\treturn false;\n>  \n> -\tif (cio2_->disableLinks()) {\n> -\t\tcio2_->close();\n> +\tif (cio2MediaDev_->disableLinks()) {\n> +\t\tcio2MediaDev_->close();\n>  \t\treturn false;\n>  \t}\n>  \n>  \tregisterCameras();\n>  \n> -\tcio2_->close();\n> +\tcio2MediaDev_->close();\n>  \n>  \treturn true;\n>  }\n> @@ -355,115 +374,28 @@ void PipelineHandlerIPU3::registerCameras()\n>  {\n>  \t/*\n>  \t * For each CSI-2 receiver on the IPU3, create a Camera if an\n> -\t * image sensor is connected to it.\n> +\t * image sensor is connected to it and it can produce images in\n\ns/and it/and the sensor/\n\n> +\t * a compatible format.\n>  \t */\n>  \tunsigned int numCameras = 0;\n>  \tfor (unsigned int id = 0; id < 4; ++id) {\n> -\t\tstd::string csi2Name = \"ipu3-csi2 \" + std::to_string(id);\n> -\t\tMediaEntity *csi2 = cio2_->getEntityByName(csi2Name);\n> -\t\tint ret;\n> -\n> -\t\t/*\n> -\t\t * This shall not happen, as the device enumerator matched\n> -\t\t * all entities described in the cio2_dm DeviceMatch.\n> -\t\t *\n> -\t\t * As this check is basically free, better stay safe than sorry.\n> -\t\t */\n> -\t\tif (!csi2)\n> -\t\t\tcontinue;\n> -\n> -\t\tconst std::vector<MediaPad *> &pads = csi2->pads();\n> -\t\tif (pads.empty())\n> -\t\t\tcontinue;\n> -\n> -\t\t/* IPU3 CSI-2 receivers have a single sink pad at index 0. */\n> -\t\tMediaPad *sink = pads[0];\n> -\t\tconst std::vector<MediaLink *> &links = sink->links();\n> -\t\tif (links.empty())\n> -\t\t\tcontinue;\n> -\n> -\t\t/*\n> -\t\t * Verify that the receiver is connected to a sensor, enable\n> -\t\t * the media link between the two, and create a Camera with\n> -\t\t * a unique name.\n> -\t\t */\n> -\t\tMediaLink *link = links[0];\n> -\t\tMediaEntity *sensor = link->source()->entity();\n> -\t\tif (sensor->function() != MEDIA_ENT_F_CAM_SENSOR)\n> -\t\t\tcontinue;\n> -\n> -\t\tif (link->setEnabled(true))\n> -\t\t\tcontinue;\n> -\n> -\t\tstd::unique_ptr<IPU3CameraData> data = utils::make_unique<IPU3CameraData>(this);\n> -\n> -\t\tstd::string cameraName = sensor->name() + \" \" + std::to_string(id);\n> +\t\tstd::unique_ptr<IPU3CameraData> data =\n> +\t\t\tutils::make_unique<IPU3CameraData>(this);\n>  \t\tstd::set<Stream *> streams{ &data->stream_ };\n> -\t\tstd::shared_ptr<Camera> camera = Camera::create(this, cameraName, streams);\n> -\n> -\t\t/*\n> -\t\t * Create and open video devices and subdevices associated with\n> -\t\t * the camera.\n> -\t\t *\n> -\t\t * If any of these operations fails, the Camera instance won't\n> -\t\t * be registered. The 'camera' shared pointer and the 'data'\n> -\t\t * unique pointers go out of scope and delete the objects they\n> -\t\t * manage.\n> -\t\t */\n> -\t\tstd::string cio2Name = \"ipu3-cio2 \" + std::to_string(id);\n> -\t\tMediaEntity *cio2 = cio2_->getEntityByName(cio2Name);\n> -\t\tif (!cio2) {\n> -\t\t\tLOG(IPU3, Error)\n> -\t\t\t\t<< \"Failed to get entity '\" << cio2Name << \"'\";\n> -\t\t\tcontinue;\n> -\t\t}\n> -\n> -\t\tdata->cio2_ = new V4L2Device(cio2);\n> -\t\tret = data->cio2_->open();\n> -\t\tif (ret)\n> -\t\t\tcontinue;\n> +\t\tCIO2Device *cio2 = &data->cio2_;\n>  \n> -\t\t/*\n> -\t\t * Make sure the sensor produces at least one image format\n> -\t\t * compatible with IPU3 CIO2 requirements and cache the camera\n> -\t\t * maximum sizes.\n> -\t\t */\n> -\t\tdata->sensor_ = new V4L2Subdevice(sensor);\n> -\t\tret = data->sensor_->open();\n> +\t\tint ret = cio2->init(cio2MediaDev_.get(), id);\n>  \t\tif (ret)\n>  \t\t\tcontinue;\n>  \n> -\t\tfor (auto it : data->sensor_->formats(0)) {\n> -\t\t\tint mbusCode = mediaBusToCIO2Format(it.first);\n> -\t\t\tif (mbusCode < 0)\n> -\t\t\t\tcontinue;\n> -\n> -\t\t\tfor (const SizeRange &size : it.second) {\n> -\t\t\t\tSizeRange &maxSize = data->maxSizes_.second;\n> -\n> -\t\t\t\tif (maxSize.maxWidth < size.maxWidth &&\n> -\t\t\t\t    maxSize.maxHeight < size.maxHeight) {\n> -\t\t\t\t\tmaxSize.maxWidth = size.maxWidth;\n> -\t\t\t\t\tmaxSize.maxHeight = size.maxHeight;\n> -\t\t\t\t\tdata->maxSizes_.first = mbusCode;\n> -\t\t\t\t}\n> -\t\t\t}\n> -\t\t}\n> -\t\tif (data->maxSizes_.second.maxWidth == 0) {\n> -\t\t\tLOG(IPU3, Info)\n> -\t\t\t\t<< \"Sensor '\" << data->sensor_->entityName()\n> -\t\t\t\t<< \"' detected, but no supported image format \"\n> -\t\t\t\t<< \" found: skip camera creation\";\n> -\t\t\tcontinue;\n> -\t\t}\n> -\n> -\t\tdata->csi2_ = new V4L2Subdevice(csi2);\n> -\t\tret = data->csi2_->open();\n> -\t\tif (ret)\n> -\t\t\tcontinue;\n> +\t\tstd::string cameraName = cio2->name() + \" \"\n> +\t\t\t\t       + std::to_string(id);\n> +\t\tstd::shared_ptr<Camera> camera = Camera::create(this,\n> +\t\t\t\t\t\t\t\tcameraName,\n> +\t\t\t\t\t\t\t\tstreams);\n>  \n> -\t\tdata->cio2_->bufferReady.connect(data.get(),\n> -\t\t\t\t\t\t &IPU3CameraData::bufferReady);\n> +\t\tcio2->output_->bufferReady.connect(data.get(),\n> +\t\t\t\t\t\t   &IPU3CameraData::bufferReady);\n>  \n>  \t\tregisterCamera(std::move(camera), std::move(data));\n>  \n> @@ -484,6 +416,104 @@ void PipelineHandlerIPU3::IPU3CameraData::bufferReady(Buffer *buffer)\n>  \tpipe_->completeRequest(camera_, request);\n>  }\n>  \n> +/*------------------------------------------------------------------------------\n> + * CIO2 Device\n> + */\n> +\n> +const std::string &CIO2Device::name() const\n> +{\n> +\treturn sensor_->entityName();\n\nThis is dangerous, as you will crash if name() is called before init().\nAs this method is only called once, in\nPipelineHandlerIPU3::registerCameras, I think you can inline\ncio2->sensor_->entityName() there.\n\n> +}\n> +\n> +/**\n> + * \\brief Initialize components of the CIO2 device with \\a index\n> + * \\param[in] media The CIO2 media device\n> + * \\param[in] index The CIO2 device index\n> + *\n> + * Create and open the device and subdevices in the CIO2 instance at \\a index,\n\ns/the device/the video device/\n\n> + * if an image sensor is connected to the CSI-2 receiver of this CIO2 instance.\n> + * Enable the media links connecting the CIO2 components to prepare for capture\n> + * operations and cache the sensor maximum sizes.\n\n\ns/sizes/size/\n\n> + *\n> + * \\return 0 on success or a negative error code otherwise\n> + * \\retval -ENODEV No image sensor is connected to this CIO2 instance\n\ns/No image sensor/No supported image sensor/\n\n> + */\n> +int CIO2Device::init(const MediaDevice *media, unsigned int index)\n> +{\n> +\tint ret;\n> +\n> +\t/* Verify that a sensor subdevice is connected to this CIO2 instance. */\n\n\"... and enable the link between the two.\" ?\n\n> +\tstd::string csi2Name = \"ipu3-csi2 \" + std::to_string(index);\n> +\tMediaEntity *csi2Entity = media->getEntityByName(csi2Name);\n> +\tconst std::vector<MediaPad *> &pads = csi2Entity->pads();\n> +\tif (pads.empty())\n> +\t\treturn -ENODEV;\n> +\n> +\t/* IPU3 CSI-2 receivers have a single sink pad at index 0. */\n> +\tMediaPad *sink = pads[0];\n> +\tconst std::vector<MediaLink *> &links = sink->links();\n> +\tif (links.empty())\n> +\t\treturn -ENODEV;\n> +\n> +\tMediaLink *link = links[0];\n> +\tMediaEntity *sensorEntity = link->source()->entity();\n> +\tif (sensorEntity->function() != MEDIA_ENT_F_CAM_SENSOR)\n> +\t\treturn -ENODEV;\n> +\n> +\tret = link->setEnabled(true);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\t/*\n> +\t * Now that we're sure a sensor subdevice is connected, make sure it\n> +\t * produces at least one image format compatible with CIO2 requirements\n> +\t * and cache the camera maximum sizes.\n\ns/sizes/size/\n\n> +\t *\n> +\t * \\todo Define when to open and close video device nodes, as they\n> +\t * might impact on power consumption.\n> +\t */\n> +\tsensor_ = new V4L2Subdevice(sensorEntity);\n> +\tret = sensor_->open();\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\tfor (auto it : sensor_->formats(0)) {\n> +\t\tint mbusCode = mediaBusToCIO2Format(it.first);\n> +\t\tif (mbusCode < 0)\n> +\t\t\tcontinue;\n> +\n> +\t\tfor (const SizeRange &size : it.second) {\n> +\t\t\tSizeRange &maxRange = maxSizes_.second;\n> +\n> +\t\t\tif (maxRange.maxWidth < size.maxWidth &&\n> +\t\t\t    maxRange.maxHeight < size.maxHeight) {\n> +\t\t\t\tmaxRange.maxWidth = size.maxWidth;\n> +\t\t\t\tmaxRange.maxHeight = size.maxHeight;\n> +\t\t\t\tmaxSizes_.first = mbusCode;\n> +\t\t\t}\n> +\t\t}\n> +\t}\n> +\tif (maxSizes_.second.maxWidth == 0) {\n> +\t\tLOG(IPU3, Info) << \"Sensor '\" << sensor_->entityName()\n> +\t\t\t\t<< \"' detected, but no supported image format \"\n> +\t\t\t\t<< \" found: skip camera creation\";\n> +\t\treturn -ENODEV;\n> +\t}\n\nAs this code is moved from PipelineHandlerIPU3::registerCameras(), don't\nforget to update it after taking the comments on the previous patches\ninto account.\n\n> +\n> +\tcsi2_ = new V4L2Subdevice(csi2Entity);\n> +\tret = csi2_->open();\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\tstd::string cio2Name = \"ipu3-cio2 \" + std::to_string(index);\n> +\toutput_ = V4L2Device::fromEntityName(media, cio2Name);\n> +\tret = output_->open();\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\treturn 0;\n> +}\n> +\n>  REGISTER_PIPELINE_HANDLER(PipelineHandlerIPU3);\n>  \n>  } /* namespace libcamera */","headers":{"Return-Path":"<laurent.pinchart@ideasonboard.com>","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id F2AB3600FD\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue,  2 Apr 2019 11:08:09 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 3E6AB2F9;\n\tTue,  2 Apr 2019 11:08:09 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1554196089;\n\tbh=ZfYCmv2RXMcJqXMjIJN8tkPjOM3Ug0nYgCrBoeSqK2I=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=KCbACkU8RXWTHDz/PXWmUbnXuZZRrCfHVawT3w98meVgtaqzaZYFq9I+882COsBIf\n\tCwKEBgYD3+kK4ZmaHY8LyTLqqigiewXjkE/cA1yA8zjma807uYDKDdoaOIiV55B9wB\n\tTDNra3BE1A/UvPfg7VucK7/w1+RMMk6WzCjMRVFE=","Date":"Tue, 2 Apr 2019 12:07:58 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Jacopo Mondi <jacopo@jmondi.org>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190402090758.GE4805@pendragon.ideasonboard.com>","References":"<20190326083902.26121-1-jacopo@jmondi.org>\n\t<20190326083902.26121-11-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190326083902.26121-11-jacopo@jmondi.org>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v5 10/19] libcamera: ipu3: Create\n\tCIO2Device class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Tue, 02 Apr 2019 09:08:10 -0000"}},{"id":1193,"web_url":"https://patchwork.libcamera.org/comment/1193/","msgid":"<20190402112222.GM23466@bigcity.dyn.berto.se>","date":"2019-04-02T11:22:22","subject":"Re: [libcamera-devel] [PATCH v5 10/19] libcamera: ipu3: Create\n\tCIO2Device class","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Jacopo,\n\nThanks for your work.\n\nOn 2019-03-26 09:38:53 +0100, Jacopo Mondi wrote:\n> Group CIO2 components (cio2, csi2 and image sensor) in a class\n> associated with the CameraData, to ease management and initialization of\n> CIO2 unit at camera registration time. A CIO2 unit will always be associated\n> with a single Camera only.\n\nI like the thing this patch does as it makes things more clean in the \nend.\n\n> \n> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>\n> ---\n>  src/libcamera/pipeline/ipu3/ipu3.cpp | 320 +++++++++++++++------------\n>  1 file changed, 175 insertions(+), 145 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 04cd02653711..21205b39afee 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -44,6 +44,32 @@ static int mediaBusToCIO2Format(unsigned int code)\n>  \t}\n>  }\n>  \n> +class CIO2Device\n> +{\n> +public:\n> +\tCIO2Device()\n> +\t\t: output_(nullptr), csi2_(nullptr), sensor_(nullptr)\n> +\t{\n> +\t}\n> +\n> +\t~CIO2Device()\n> +\t{\n> +\t\tdelete output_;\n> +\t\tdelete csi2_;\n> +\t\tdelete sensor_;\n> +\t}\n> +\n> +\tconst std::string &name() const;\n> +\tint init(const MediaDevice *media, unsigned int index);\n> +\n> +\tV4L2Device *output_;\n> +\tV4L2Subdevice *csi2_;\n> +\tV4L2Subdevice *sensor_;\n> +\n> +\t/* Maximum sizes and the mbus code used to produce them. */\n> +\tstd::pair<unsigned int, SizeRange> maxSizes_;\n> +};\n> +\n>  class PipelineHandlerIPU3 : public PipelineHandler\n>  {\n>  public:\n> @@ -67,36 +93,23 @@ public:\n>  \tbool match(DeviceEnumerator *enumerator);\n>  \n>  private:\n> +\tstatic constexpr unsigned int IPU3_BUFFER_COUNT = 4;\n> +\n>  \tclass IPU3CameraData : public CameraData\n>  \t{\n>  \tpublic:\n>  \t\tIPU3CameraData(PipelineHandler *pipe)\n> -\t\t\t: CameraData(pipe), cio2_(nullptr), csi2_(nullptr),\n> -\t\t\t  sensor_(nullptr)\n> -\t\t{\n> -\t\t}\n> -\n> -\t\t~IPU3CameraData()\n> +\t\t\t: CameraData(pipe)\n>  \t\t{\n> -\t\t\tdelete cio2_;\n> -\t\t\tdelete csi2_;\n> -\t\t\tdelete sensor_;\n>  \t\t}\n>  \n>  \t\tvoid bufferReady(Buffer *buffer);\n>  \n> -\t\tV4L2Device *cio2_;\n> -\t\tV4L2Subdevice *csi2_;\n> -\t\tV4L2Subdevice *sensor_;\n> +\t\tCIO2Device cio2_;\n>  \n>  \t\tStream stream_;\n> -\n> -\t\t/* Maximum sizes and the mbus code used to produce them. */\n> -\t\tstd::pair<unsigned int, SizeRange> maxSizes_;\n>  \t};\n>  \n> -\tstatic constexpr unsigned int IPU3_BUFFER_COUNT = 4;\n> -\n\nThis was introduced in the previous patch count you not move this change \nto that patch and not move again here? Would it make sens to swap this \nand the previous patch in this series? First move and break things out \nand then add new features on top?\n\n>  \tIPU3CameraData *cameraData(const Camera *camera)\n>  \t{\n>  \t\treturn static_cast<IPU3CameraData *>(\n> @@ -105,22 +118,22 @@ private:\n>  \n>  \tvoid registerCameras();\n>  \n> -\tstd::shared_ptr<MediaDevice> cio2_;\n> -\tstd::shared_ptr<MediaDevice> imgu_;\n> +\tstd::shared_ptr<MediaDevice> cio2MediaDev_;\n> +\tstd::shared_ptr<MediaDevice> imguMediaDev_;\n\nI would move this rename to its own commit to make this patch easier to \nreview. You could tuck some of the reordering of declarations you do \ninto such a clean up patch also if you fancy.\n\n>  };\n>  \n>  PipelineHandlerIPU3::PipelineHandlerIPU3(CameraManager *manager)\n> -\t: PipelineHandler(manager), cio2_(nullptr), imgu_(nullptr)\n> +\t: PipelineHandler(manager), cio2MediaDev_(nullptr), imguMediaDev_(nullptr)\n>  {\n>  }\n>  \n>  PipelineHandlerIPU3::~PipelineHandlerIPU3()\n>  {\n> -\tif (cio2_)\n> -\t\tcio2_->release();\n> +\tif (cio2MediaDev_)\n> +\t\tcio2MediaDev_->release();\n>  \n> -\tif (imgu_)\n> -\t\timgu_->release();\n> +\tif (imguMediaDev_)\n> +\t\timguMediaDev_->release();\n>  }\n>  \n>  std::map<Stream *, StreamConfiguration>\n> @@ -130,11 +143,12 @@ PipelineHandlerIPU3::streamConfiguration(Camera *camera,\n>  \tstd::map<Stream *, StreamConfiguration> configs;\n>  \tIPU3CameraData *data = cameraData(camera);\n>  \tStreamConfiguration *config = &configs[&data->stream_];\n> -\tSizeRange &maxRange = data->maxSizes_.second;\n> +\tCIO2Device *cio2 = &data->cio2_;\n> +\tSizeRange &maxRange = cio2->maxSizes_.second;\n>  \n>  \tconfig->width = maxRange.maxWidth;\n>  \tconfig->height = maxRange.maxHeight;\n> -\tconfig->pixelFormat = data->maxSizes_.first;\n> +\tconfig->pixelFormat = cio2->maxSizes_.first;\n>  \tconfig->bufferCount = IPU3_BUFFER_COUNT;\n>  \n>  \tLOG(IPU3, Debug)\n> @@ -150,9 +164,9 @@ int PipelineHandlerIPU3::configureStreams(Camera *camera,\n>  {\n>  \tIPU3CameraData *data = cameraData(camera);\n>  \tStreamConfiguration *cfg = &config[&data->stream_];\n> -\tV4L2Subdevice *sensor = data->sensor_;\n> -\tV4L2Subdevice *csi2 = data->csi2_;\n> -\tV4L2Device *cio2 = data->cio2_;\n> +\tV4L2Subdevice *sensor = data->cio2_.sensor_;\n> +\tV4L2Subdevice *csi2 = data->cio2_.csi2_;\n> +\tV4L2Device *cio2 = data->cio2_.output_;\n>  \tV4L2SubdeviceFormat subdevFormat = {};\n>  \tV4L2DeviceFormat devFormat = {};\n>  \tint ret;\n> @@ -209,13 +223,14 @@ int PipelineHandlerIPU3::configureStreams(Camera *camera,\n>  \n>  int PipelineHandlerIPU3::allocateBuffers(Camera *camera, Stream *stream)\n>  {\n> -\tIPU3CameraData *data = cameraData(camera);\n>  \tconst StreamConfiguration &cfg = stream->configuration();\n> +\tIPU3CameraData *data = cameraData(camera);\n> +\tV4L2Device *cio2 = data->cio2_.output_;\n>  \n>  \tif (!cfg.bufferCount)\n>  \t\treturn -EINVAL;\n>  \n> -\tint ret = data->cio2_->exportBuffers(&stream->bufferPool());\n> +\tint ret = cio2->exportBuffers(&stream->bufferPool());\n>  \tif (ret) {\n>  \t\tLOG(IPU3, Error) << \"Failed to request memory\";\n>  \t\treturn ret;\n> @@ -227,8 +242,9 @@ int PipelineHandlerIPU3::allocateBuffers(Camera *camera, Stream *stream)\n>  int PipelineHandlerIPU3::freeBuffers(Camera *camera, Stream *stream)\n>  {\n>  \tIPU3CameraData *data = cameraData(camera);\n> +\tV4L2Device *cio2 = data->cio2_.output_;\n>  \n> -\tint ret = data->cio2_->releaseBuffers();\n> +\tint ret = cio2->releaseBuffers();\n>  \tif (ret) {\n>  \t\tLOG(IPU3, Error) << \"Failed to release memory\";\n>  \t\treturn ret;\n> @@ -240,9 +256,10 @@ int PipelineHandlerIPU3::freeBuffers(Camera *camera, Stream *stream)\n>  int PipelineHandlerIPU3::start(Camera *camera)\n>  {\n>  \tIPU3CameraData *data = cameraData(camera);\n> +\tV4L2Device *cio2 = data->cio2_.output_;\n>  \tint ret;\n>  \n> -\tret = data->cio2_->streamOn();\n> +\tret = cio2->streamOn();\n>  \tif (ret) {\n>  \t\tLOG(IPU3, Info) << \"Failed to start camera \" << camera->name();\n>  \t\treturn ret;\n> @@ -254,8 +271,9 @@ int PipelineHandlerIPU3::start(Camera *camera)\n>  void PipelineHandlerIPU3::stop(Camera *camera)\n>  {\n>  \tIPU3CameraData *data = cameraData(camera);\n> +\tV4L2Device *cio2 = data->cio2_.output_;\n>  \n> -\tif (data->cio2_->streamOff())\n> +\tif (cio2->streamOff())\n>  \t\tLOG(IPU3, Info) << \"Failed to stop camera \" << camera->name();\n>  \n>  \tPipelineHandler::stop(camera);\n> @@ -264,6 +282,7 @@ void PipelineHandlerIPU3::stop(Camera *camera)\n>  int PipelineHandlerIPU3::queueRequest(Camera *camera, Request *request)\n>  {\n>  \tIPU3CameraData *data = cameraData(camera);\n> +\tV4L2Device *cio2 = data->cio2_.output_;\n>  \tStream *stream = &data->stream_;\n>  \n>  \tBuffer *buffer = request->findBuffer(stream);\n> @@ -273,7 +292,7 @@ int PipelineHandlerIPU3::queueRequest(Camera *camera, Request *request)\n>  \t\treturn -ENOENT;\n>  \t}\n>  \n> -\tint ret = data->cio2_->queueBuffer(buffer);\n> +\tint ret = cio2->queueBuffer(buffer);\n>  \tif (ret < 0)\n>  \t\treturn ret;\n>  \n> @@ -312,17 +331,17 @@ bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator)\n>  \t * It is safe to acquire both media devices at this point as\n>  \t * DeviceEnumerator::search() skips the busy ones for us.\n>  \t */\n> -\tcio2_ = enumerator->search(cio2_dm);\n> -\tif (!cio2_)\n> +\tcio2MediaDev_ = enumerator->search(cio2_dm);\n> +\tif (!cio2MediaDev_)\n>  \t\treturn false;\n>  \n> -\tcio2_->acquire();\n> +\tcio2MediaDev_->acquire();\n>  \n> -\timgu_ = enumerator->search(imgu_dm);\n> -\tif (!imgu_)\n> +\timguMediaDev_ = enumerator->search(imgu_dm);\n> +\tif (!imguMediaDev_)\n>  \t\treturn false;\n>  \n> -\timgu_->acquire();\n> +\timguMediaDev_->acquire();\n>  \n>  \t/*\n>  \t * Disable all links that are enabled by default on CIO2, as camera\n> @@ -331,17 +350,17 @@ bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator)\n>  \t * Close the CIO2 media device after, as links are enabled and should\n>  \t * not need to be changed after.\n>  \t */\n> -\tif (cio2_->open())\n> +\tif (cio2MediaDev_->open())\n>  \t\treturn false;\n>  \n> -\tif (cio2_->disableLinks()) {\n> -\t\tcio2_->close();\n> +\tif (cio2MediaDev_->disableLinks()) {\n> +\t\tcio2MediaDev_->close();\n>  \t\treturn false;\n>  \t}\n>  \n>  \tregisterCameras();\n>  \n> -\tcio2_->close();\n> +\tcio2MediaDev_->close();\n>  \n>  \treturn true;\n>  }\n> @@ -355,115 +374,28 @@ void PipelineHandlerIPU3::registerCameras()\n>  {\n>  \t/*\n>  \t * For each CSI-2 receiver on the IPU3, create a Camera if an\n> -\t * image sensor is connected to it.\n> +\t * image sensor is connected to it and it can produce images in\n> +\t * a compatible format.\n>  \t */\n>  \tunsigned int numCameras = 0;\n>  \tfor (unsigned int id = 0; id < 4; ++id) {\n> -\t\tstd::string csi2Name = \"ipu3-csi2 \" + std::to_string(id);\n> -\t\tMediaEntity *csi2 = cio2_->getEntityByName(csi2Name);\n> -\t\tint ret;\n> -\n> -\t\t/*\n> -\t\t * This shall not happen, as the device enumerator matched\n> -\t\t * all entities described in the cio2_dm DeviceMatch.\n> -\t\t *\n> -\t\t * As this check is basically free, better stay safe than sorry.\n> -\t\t */\n> -\t\tif (!csi2)\n> -\t\t\tcontinue;\n> -\n> -\t\tconst std::vector<MediaPad *> &pads = csi2->pads();\n> -\t\tif (pads.empty())\n> -\t\t\tcontinue;\n> -\n> -\t\t/* IPU3 CSI-2 receivers have a single sink pad at index 0. */\n> -\t\tMediaPad *sink = pads[0];\n> -\t\tconst std::vector<MediaLink *> &links = sink->links();\n> -\t\tif (links.empty())\n> -\t\t\tcontinue;\n> -\n> -\t\t/*\n> -\t\t * Verify that the receiver is connected to a sensor, enable\n> -\t\t * the media link between the two, and create a Camera with\n> -\t\t * a unique name.\n> -\t\t */\n> -\t\tMediaLink *link = links[0];\n> -\t\tMediaEntity *sensor = link->source()->entity();\n> -\t\tif (sensor->function() != MEDIA_ENT_F_CAM_SENSOR)\n> -\t\t\tcontinue;\n> -\n> -\t\tif (link->setEnabled(true))\n> -\t\t\tcontinue;\n> -\n> -\t\tstd::unique_ptr<IPU3CameraData> data = utils::make_unique<IPU3CameraData>(this);\n> -\n> -\t\tstd::string cameraName = sensor->name() + \" \" + std::to_string(id);\n> +\t\tstd::unique_ptr<IPU3CameraData> data =\n> +\t\t\tutils::make_unique<IPU3CameraData>(this);\n>  \t\tstd::set<Stream *> streams{ &data->stream_ };\n> -\t\tstd::shared_ptr<Camera> camera = Camera::create(this, cameraName, streams);\n> -\n> -\t\t/*\n> -\t\t * Create and open video devices and subdevices associated with\n> -\t\t * the camera.\n> -\t\t *\n> -\t\t * If any of these operations fails, the Camera instance won't\n> -\t\t * be registered. The 'camera' shared pointer and the 'data'\n> -\t\t * unique pointers go out of scope and delete the objects they\n> -\t\t * manage.\n> -\t\t */\n> -\t\tstd::string cio2Name = \"ipu3-cio2 \" + std::to_string(id);\n> -\t\tMediaEntity *cio2 = cio2_->getEntityByName(cio2Name);\n> -\t\tif (!cio2) {\n> -\t\t\tLOG(IPU3, Error)\n> -\t\t\t\t<< \"Failed to get entity '\" << cio2Name << \"'\";\n> -\t\t\tcontinue;\n> -\t\t}\n> -\n> -\t\tdata->cio2_ = new V4L2Device(cio2);\n> -\t\tret = data->cio2_->open();\n> -\t\tif (ret)\n> -\t\t\tcontinue;\n> +\t\tCIO2Device *cio2 = &data->cio2_;\n>  \n> -\t\t/*\n> -\t\t * Make sure the sensor produces at least one image format\n> -\t\t * compatible with IPU3 CIO2 requirements and cache the camera\n> -\t\t * maximum sizes.\n> -\t\t */\n> -\t\tdata->sensor_ = new V4L2Subdevice(sensor);\n> -\t\tret = data->sensor_->open();\n> +\t\tint ret = cio2->init(cio2MediaDev_.get(), id);\n>  \t\tif (ret)\n>  \t\t\tcontinue;\n>  \n> -\t\tfor (auto it : data->sensor_->formats(0)) {\n> -\t\t\tint mbusCode = mediaBusToCIO2Format(it.first);\n> -\t\t\tif (mbusCode < 0)\n> -\t\t\t\tcontinue;\n> -\n> -\t\t\tfor (const SizeRange &size : it.second) {\n> -\t\t\t\tSizeRange &maxSize = data->maxSizes_.second;\n> -\n> -\t\t\t\tif (maxSize.maxWidth < size.maxWidth &&\n> -\t\t\t\t    maxSize.maxHeight < size.maxHeight) {\n> -\t\t\t\t\tmaxSize.maxWidth = size.maxWidth;\n> -\t\t\t\t\tmaxSize.maxHeight = size.maxHeight;\n> -\t\t\t\t\tdata->maxSizes_.first = mbusCode;\n> -\t\t\t\t}\n> -\t\t\t}\n> -\t\t}\n> -\t\tif (data->maxSizes_.second.maxWidth == 0) {\n> -\t\t\tLOG(IPU3, Info)\n> -\t\t\t\t<< \"Sensor '\" << data->sensor_->entityName()\n> -\t\t\t\t<< \"' detected, but no supported image format \"\n> -\t\t\t\t<< \" found: skip camera creation\";\n> -\t\t\tcontinue;\n> -\t\t}\n> -\n> -\t\tdata->csi2_ = new V4L2Subdevice(csi2);\n> -\t\tret = data->csi2_->open();\n> -\t\tif (ret)\n> -\t\t\tcontinue;\n> +\t\tstd::string cameraName = cio2->name() + \" \"\n> +\t\t\t\t       + std::to_string(id);\n> +\t\tstd::shared_ptr<Camera> camera = Camera::create(this,\n> +\t\t\t\t\t\t\t\tcameraName,\n> +\t\t\t\t\t\t\t\tstreams);\n>  \n> -\t\tdata->cio2_->bufferReady.connect(data.get(),\n> -\t\t\t\t\t\t &IPU3CameraData::bufferReady);\n> +\t\tcio2->output_->bufferReady.connect(data.get(),\n> +\t\t\t\t\t\t   &IPU3CameraData::bufferReady);\n>  \n>  \t\tregisterCamera(std::move(camera), std::move(data));\n>  \n> @@ -484,6 +416,104 @@ void PipelineHandlerIPU3::IPU3CameraData::bufferReady(Buffer *buffer)\n>  \tpipe_->completeRequest(camera_, request);\n>  }\n>  \n> +/*------------------------------------------------------------------------------\n> + * CIO2 Device\n> + */\n> +\n> +const std::string &CIO2Device::name() const\n> +{\n> +\treturn sensor_->entityName();\n> +}\n> +\n> +/**\n> + * \\brief Initialize components of the CIO2 device with \\a index\n> + * \\param[in] media The CIO2 media device\n> + * \\param[in] index The CIO2 device index\n> + *\n> + * Create and open the device and subdevices in the CIO2 instance at \\a index,\n> + * if an image sensor is connected to the CSI-2 receiver of this CIO2 instance.\n> + * Enable the media links connecting the CIO2 components to prepare for capture\n> + * operations and cache the sensor maximum sizes.\n> + *\n> + * \\return 0 on success or a negative error code otherwise\n> + * \\retval -ENODEV No image sensor is connected to this CIO2 instance\n> + */\n> +int CIO2Device::init(const MediaDevice *media, unsigned int index)\n> +{\n> +\tint ret;\n> +\n> +\t/* Verify that a sensor subdevice is connected to this CIO2 instance. */\n> +\tstd::string csi2Name = \"ipu3-csi2 \" + std::to_string(index);\n> +\tMediaEntity *csi2Entity = media->getEntityByName(csi2Name);\n> +\tconst std::vector<MediaPad *> &pads = csi2Entity->pads();\n> +\tif (pads.empty())\n> +\t\treturn -ENODEV;\n> +\n> +\t/* IPU3 CSI-2 receivers have a single sink pad at index 0. */\n> +\tMediaPad *sink = pads[0];\n> +\tconst std::vector<MediaLink *> &links = sink->links();\n> +\tif (links.empty())\n> +\t\treturn -ENODEV;\n> +\n> +\tMediaLink *link = links[0];\n> +\tMediaEntity *sensorEntity = link->source()->entity();\n> +\tif (sensorEntity->function() != MEDIA_ENT_F_CAM_SENSOR)\n> +\t\treturn -ENODEV;\n> +\n> +\tret = link->setEnabled(true);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\t/*\n> +\t * Now that we're sure a sensor subdevice is connected, make sure it\n> +\t * produces at least one image format compatible with CIO2 requirements\n> +\t * and cache the camera maximum sizes.\n> +\t *\n> +\t * \\todo Define when to open and close video device nodes, as they\n> +\t * might impact on power consumption.\n> +\t */\n> +\tsensor_ = new V4L2Subdevice(sensorEntity);\n> +\tret = sensor_->open();\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\tfor (auto it : sensor_->formats(0)) {\n> +\t\tint mbusCode = mediaBusToCIO2Format(it.first);\n> +\t\tif (mbusCode < 0)\n> +\t\t\tcontinue;\n> +\n> +\t\tfor (const SizeRange &size : it.second) {\n> +\t\t\tSizeRange &maxRange = maxSizes_.second;\n> +\n> +\t\t\tif (maxRange.maxWidth < size.maxWidth &&\n> +\t\t\t    maxRange.maxHeight < size.maxHeight) {\n> +\t\t\t\tmaxRange.maxWidth = size.maxWidth;\n> +\t\t\t\tmaxRange.maxHeight = size.maxHeight;\n> +\t\t\t\tmaxSizes_.first = mbusCode;\n> +\t\t\t}\n> +\t\t}\n> +\t}\n> +\tif (maxSizes_.second.maxWidth == 0) {\n> +\t\tLOG(IPU3, Info) << \"Sensor '\" << sensor_->entityName()\n> +\t\t\t\t<< \"' detected, but no supported image format \"\n> +\t\t\t\t<< \" found: skip camera creation\";\n> +\t\treturn -ENODEV;\n> +\t}\n> +\n> +\tcsi2_ = new V4L2Subdevice(csi2Entity);\n> +\tret = csi2_->open();\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\tstd::string cio2Name = \"ipu3-cio2 \" + std::to_string(index);\n> +\toutput_ = V4L2Device::fromEntityName(media, cio2Name);\n> +\tret = output_->open();\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\treturn 0;\n> +}\n> +\n>  REGISTER_PIPELINE_HANDLER(PipelineHandlerIPU3);\n>  \n>  } /* namespace libcamera */\n> -- \n> 2.21.0\n> \n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"<niklas.soderlund@ragnatech.se>","Received":["from mail-lf1-x133.google.com (mail-lf1-x133.google.com\n\t[IPv6:2a00:1450:4864:20::133])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 10B0560DB3\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue,  2 Apr 2019 13:22:26 +0200 (CEST)","by mail-lf1-x133.google.com with SMTP id a28so8692323lfo.7\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 02 Apr 2019 04:22:25 -0700 (PDT)","from localhost (89-233-230-99.cust.bredband2.com. [89.233.230.99])\n\tby smtp.gmail.com with ESMTPSA id\n\te17sm2704902ljj.20.2019.04.02.04.22.22\n\t(version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256);\n\tTue, 02 Apr 2019 04:22:23 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to\n\t:user-agent; bh=M32XDNbNzt1WVEe8VqIndP7BALU/EG9cahZ1GPDcbow=;\n\tb=krkKmljNW2ggcCTSASDknhTGF4Tv2P8q96JvD9xa3Wn3lnlTN7Pcvr7FgfPHPH1slK\n\tIkSnoOcRWyiVCVM/ZyF5TqIX1TrijDKVQs9//v190m14WiPrkMNt9IxYn0+Owt0BRBks\n\tfZDJf3MLENgMomkA3wSH60RYkD00omSWDWMQOSa0Hs3QEIK8JSLywZTkVRHFpMlgF3QZ\n\tgbh9835HtTXXJkQmKPGXBfZ7Jca/95ttIUaqQDEWUttcZNizbbs1duDdgTJtSnprg1gH\n\te/UXfJb9PAL6Mcy4Bg+UViciXB/frWjebMC7vawwJkx6ZAwmi2HoLpTq9PErpV5Phsxz\n\td/nw==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to:user-agent;\n\tbh=M32XDNbNzt1WVEe8VqIndP7BALU/EG9cahZ1GPDcbow=;\n\tb=c1HFG9dKHs2hYtyjhtfUa1WqIcWaqnq5GtHKpXQ4H5N0dGyqbUb6REEslrEuF6zunJ\n\tH2ma+HFYkDgVpR4LQXtRvvefp7Yw8WYoV/oGUWlr7weF23cePbPGnO8EMBV0Tp5EEzzm\n\tN875tTekXtwi8pZ2Yki9dJxcPLBJAkVBtKYq45NWR179+ksoqnOk/Ojk/MeXrXypORzU\n\tyvxYV5nXNVWHIrXq8GbBlHDSfZeXutzPNdn/7OgA8j1M14IUonbjKVFMVQ+I/5zKdocG\n\txqoeDP7NeKmOBNVzpHG1ZngaZPjvObQYBpccCz+Ng6ZP7+fcwltznQCTrbirdLJA4a30\n\tGGrQ==","X-Gm-Message-State":"APjAAAUzt1e8G1clJf1b9XMrvLZv8oUejub7yvQz9/nNguoEEdCCtW6G\n\tXw3XE/KV6vprDQzjXCg5Zssvh87zXh0=","X-Google-Smtp-Source":"APXvYqxmIHV4y7mw9GJ6dQhIm9pNsmWzrUrfle+4J7XfB71PEiQ+1jTyJywXoxOx76fb8BsCmO3s3Q==","X-Received":"by 2002:ac2:51a1:: with SMTP id f1mr37504539lfk.25.1554204143906;\n\tTue, 02 Apr 2019 04:22:23 -0700 (PDT)","Date":"Tue, 2 Apr 2019 13:22:22 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= <niklas.soderlund@ragnatech.se>","To":"Jacopo Mondi <jacopo@jmondi.org>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190402112222.GM23466@bigcity.dyn.berto.se>","References":"<20190326083902.26121-1-jacopo@jmondi.org>\n\t<20190326083902.26121-11-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190326083902.26121-11-jacopo@jmondi.org>","User-Agent":"Mutt/1.11.3 (2019-02-01)","Subject":"Re: [libcamera-devel] [PATCH v5 10/19] libcamera: ipu3: Create\n\tCIO2Device class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Tue, 02 Apr 2019 11:22:26 -0000"}},{"id":1204,"web_url":"https://patchwork.libcamera.org/comment/1204/","msgid":"<20190402122822.kwqe7wdqnwoz5zzz@uno.localdomain>","date":"2019-04-02T12:28:22","subject":"Re: [libcamera-devel] [PATCH v5 10/19] libcamera: ipu3: Create\n\tCIO2Device class","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Niklas,\n\nOn Tue, Apr 02, 2019 at 01:22:22PM +0200, Niklas Söderlund wrote:\n> Hi Jacopo,\n>\n> Thanks for your work.\n>\n> On 2019-03-26 09:38:53 +0100, Jacopo Mondi wrote:\n> > Group CIO2 components (cio2, csi2 and image sensor) in a class\n> > associated with the CameraData, to ease management and initialization of\n> > CIO2 unit at camera registration time. A CIO2 unit will always be associated\n> > with a single Camera only.\n>\n> I like the thing this patch does as it makes things more clean in the\n> end.\n>\n> >\n> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>\n> > ---\n> >  src/libcamera/pipeline/ipu3/ipu3.cpp | 320 +++++++++++++++------------\n> >  1 file changed, 175 insertions(+), 145 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > index 04cd02653711..21205b39afee 100644\n> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > @@ -44,6 +44,32 @@ static int mediaBusToCIO2Format(unsigned int code)\n> >  \t}\n> >  }\n> >\n> > +class CIO2Device\n> > +{\n> > +public:\n> > +\tCIO2Device()\n> > +\t\t: output_(nullptr), csi2_(nullptr), sensor_(nullptr)\n> > +\t{\n> > +\t}\n> > +\n> > +\t~CIO2Device()\n> > +\t{\n> > +\t\tdelete output_;\n> > +\t\tdelete csi2_;\n> > +\t\tdelete sensor_;\n> > +\t}\n> > +\n> > +\tconst std::string &name() const;\n> > +\tint init(const MediaDevice *media, unsigned int index);\n> > +\n> > +\tV4L2Device *output_;\n> > +\tV4L2Subdevice *csi2_;\n> > +\tV4L2Subdevice *sensor_;\n> > +\n> > +\t/* Maximum sizes and the mbus code used to produce them. */\n> > +\tstd::pair<unsigned int, SizeRange> maxSizes_;\n> > +};\n> > +\n> >  class PipelineHandlerIPU3 : public PipelineHandler\n> >  {\n> >  public:\n> > @@ -67,36 +93,23 @@ public:\n> >  \tbool match(DeviceEnumerator *enumerator);\n> >\n> >  private:\n> > +\tstatic constexpr unsigned int IPU3_BUFFER_COUNT = 4;\n> > +\n> >  \tclass IPU3CameraData : public CameraData\n> >  \t{\n> >  \tpublic:\n> >  \t\tIPU3CameraData(PipelineHandler *pipe)\n> > -\t\t\t: CameraData(pipe), cio2_(nullptr), csi2_(nullptr),\n> > -\t\t\t  sensor_(nullptr)\n> > -\t\t{\n> > -\t\t}\n> > -\n> > -\t\t~IPU3CameraData()\n> > +\t\t\t: CameraData(pipe)\n> >  \t\t{\n> > -\t\t\tdelete cio2_;\n> > -\t\t\tdelete csi2_;\n> > -\t\t\tdelete sensor_;\n> >  \t\t}\n> >\n> >  \t\tvoid bufferReady(Buffer *buffer);\n> >\n> > -\t\tV4L2Device *cio2_;\n> > -\t\tV4L2Subdevice *csi2_;\n> > -\t\tV4L2Subdevice *sensor_;\n> > +\t\tCIO2Device cio2_;\n> >\n> >  \t\tStream stream_;\n> > -\n> > -\t\t/* Maximum sizes and the mbus code used to produce them. */\n> > -\t\tstd::pair<unsigned int, SizeRange> maxSizes_;\n> >  \t};\n> >\n> > -\tstatic constexpr unsigned int IPU3_BUFFER_COUNT = 4;\n> > -\n>\n> This was introduced in the previous patch count you not move this change\n> to that patch and not move again here? Would it make sens to swap this\n> and the previous patch in this series? First move and break things out\n> and then add new features on top?\n>\n\nAll the fuss around returning a default config, then moving it to the\nCIO2 device, then changing it again to use the format provided by the\nImgU, then default to a safe value for Soraka could be avoided if I\nchange streamConfiguration in a final last patch. Although that breaks\ncapture operations in the intermediate patches, which is in my opinion\nacceptable as the series will be pushed all in one go, but has been\npointed out in a previous review round as an error.\n\nIf that's fine with everyone, I'll change streamConfiguration() once\nat the end of the series.\n\n> >  \tIPU3CameraData *cameraData(const Camera *camera)\n> >  \t{\n> >  \t\treturn static_cast<IPU3CameraData *>(\n> > @@ -105,22 +118,22 @@ private:\n> >\n> >  \tvoid registerCameras();\n> >\n> > -\tstd::shared_ptr<MediaDevice> cio2_;\n> > -\tstd::shared_ptr<MediaDevice> imgu_;\n> > +\tstd::shared_ptr<MediaDevice> cio2MediaDev_;\n> > +\tstd::shared_ptr<MediaDevice> imguMediaDev_;\n>\n> I would move this rename to its own commit to make this patch easier to\n> review. You could tuck some of the reordering of declarations you do\n> into such a clean up patch also if you fancy.\n>\n\nI changed this as I have here introduced data->cio2_, while this here\nabove would still be PipelineHandlerIPU3::cio2_ and in a few places I\nwould have things like:\n        data->cio2_.init(cio2_, ..)\nwhich is pretty confusing.\n\nLaurent does not like the rename either, but I would like to not\nconfuse the CIO2Device with the CIO2 MediaDevice.\n\nThanks\n  j\n\n> >  };\n> >\n> >  PipelineHandlerIPU3::PipelineHandlerIPU3(CameraManager *manager)\n> > -\t: PipelineHandler(manager), cio2_(nullptr), imgu_(nullptr)\n> > +\t: PipelineHandler(manager), cio2MediaDev_(nullptr), imguMediaDev_(nullptr)\n> >  {\n> >  }\n> >\n> >  PipelineHandlerIPU3::~PipelineHandlerIPU3()\n> >  {\n> > -\tif (cio2_)\n> > -\t\tcio2_->release();\n> > +\tif (cio2MediaDev_)\n> > +\t\tcio2MediaDev_->release();\n> >\n> > -\tif (imgu_)\n> > -\t\timgu_->release();\n> > +\tif (imguMediaDev_)\n> > +\t\timguMediaDev_->release();\n> >  }\n> >\n> >  std::map<Stream *, StreamConfiguration>\n> > @@ -130,11 +143,12 @@ PipelineHandlerIPU3::streamConfiguration(Camera *camera,\n> >  \tstd::map<Stream *, StreamConfiguration> configs;\n> >  \tIPU3CameraData *data = cameraData(camera);\n> >  \tStreamConfiguration *config = &configs[&data->stream_];\n> > -\tSizeRange &maxRange = data->maxSizes_.second;\n> > +\tCIO2Device *cio2 = &data->cio2_;\n> > +\tSizeRange &maxRange = cio2->maxSizes_.second;\n> >\n> >  \tconfig->width = maxRange.maxWidth;\n> >  \tconfig->height = maxRange.maxHeight;\n> > -\tconfig->pixelFormat = data->maxSizes_.first;\n> > +\tconfig->pixelFormat = cio2->maxSizes_.first;\n> >  \tconfig->bufferCount = IPU3_BUFFER_COUNT;\n> >\n> >  \tLOG(IPU3, Debug)\n> > @@ -150,9 +164,9 @@ int PipelineHandlerIPU3::configureStreams(Camera *camera,\n> >  {\n> >  \tIPU3CameraData *data = cameraData(camera);\n> >  \tStreamConfiguration *cfg = &config[&data->stream_];\n> > -\tV4L2Subdevice *sensor = data->sensor_;\n> > -\tV4L2Subdevice *csi2 = data->csi2_;\n> > -\tV4L2Device *cio2 = data->cio2_;\n> > +\tV4L2Subdevice *sensor = data->cio2_.sensor_;\n> > +\tV4L2Subdevice *csi2 = data->cio2_.csi2_;\n> > +\tV4L2Device *cio2 = data->cio2_.output_;\n> >  \tV4L2SubdeviceFormat subdevFormat = {};\n> >  \tV4L2DeviceFormat devFormat = {};\n> >  \tint ret;\n> > @@ -209,13 +223,14 @@ int PipelineHandlerIPU3::configureStreams(Camera *camera,\n> >\n> >  int PipelineHandlerIPU3::allocateBuffers(Camera *camera, Stream *stream)\n> >  {\n> > -\tIPU3CameraData *data = cameraData(camera);\n> >  \tconst StreamConfiguration &cfg = stream->configuration();\n> > +\tIPU3CameraData *data = cameraData(camera);\n> > +\tV4L2Device *cio2 = data->cio2_.output_;\n> >\n> >  \tif (!cfg.bufferCount)\n> >  \t\treturn -EINVAL;\n> >\n> > -\tint ret = data->cio2_->exportBuffers(&stream->bufferPool());\n> > +\tint ret = cio2->exportBuffers(&stream->bufferPool());\n> >  \tif (ret) {\n> >  \t\tLOG(IPU3, Error) << \"Failed to request memory\";\n> >  \t\treturn ret;\n> > @@ -227,8 +242,9 @@ int PipelineHandlerIPU3::allocateBuffers(Camera *camera, Stream *stream)\n> >  int PipelineHandlerIPU3::freeBuffers(Camera *camera, Stream *stream)\n> >  {\n> >  \tIPU3CameraData *data = cameraData(camera);\n> > +\tV4L2Device *cio2 = data->cio2_.output_;\n> >\n> > -\tint ret = data->cio2_->releaseBuffers();\n> > +\tint ret = cio2->releaseBuffers();\n> >  \tif (ret) {\n> >  \t\tLOG(IPU3, Error) << \"Failed to release memory\";\n> >  \t\treturn ret;\n> > @@ -240,9 +256,10 @@ int PipelineHandlerIPU3::freeBuffers(Camera *camera, Stream *stream)\n> >  int PipelineHandlerIPU3::start(Camera *camera)\n> >  {\n> >  \tIPU3CameraData *data = cameraData(camera);\n> > +\tV4L2Device *cio2 = data->cio2_.output_;\n> >  \tint ret;\n> >\n> > -\tret = data->cio2_->streamOn();\n> > +\tret = cio2->streamOn();\n> >  \tif (ret) {\n> >  \t\tLOG(IPU3, Info) << \"Failed to start camera \" << camera->name();\n> >  \t\treturn ret;\n> > @@ -254,8 +271,9 @@ int PipelineHandlerIPU3::start(Camera *camera)\n> >  void PipelineHandlerIPU3::stop(Camera *camera)\n> >  {\n> >  \tIPU3CameraData *data = cameraData(camera);\n> > +\tV4L2Device *cio2 = data->cio2_.output_;\n> >\n> > -\tif (data->cio2_->streamOff())\n> > +\tif (cio2->streamOff())\n> >  \t\tLOG(IPU3, Info) << \"Failed to stop camera \" << camera->name();\n> >\n> >  \tPipelineHandler::stop(camera);\n> > @@ -264,6 +282,7 @@ void PipelineHandlerIPU3::stop(Camera *camera)\n> >  int PipelineHandlerIPU3::queueRequest(Camera *camera, Request *request)\n> >  {\n> >  \tIPU3CameraData *data = cameraData(camera);\n> > +\tV4L2Device *cio2 = data->cio2_.output_;\n> >  \tStream *stream = &data->stream_;\n> >\n> >  \tBuffer *buffer = request->findBuffer(stream);\n> > @@ -273,7 +292,7 @@ int PipelineHandlerIPU3::queueRequest(Camera *camera, Request *request)\n> >  \t\treturn -ENOENT;\n> >  \t}\n> >\n> > -\tint ret = data->cio2_->queueBuffer(buffer);\n> > +\tint ret = cio2->queueBuffer(buffer);\n> >  \tif (ret < 0)\n> >  \t\treturn ret;\n> >\n> > @@ -312,17 +331,17 @@ bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator)\n> >  \t * It is safe to acquire both media devices at this point as\n> >  \t * DeviceEnumerator::search() skips the busy ones for us.\n> >  \t */\n> > -\tcio2_ = enumerator->search(cio2_dm);\n> > -\tif (!cio2_)\n> > +\tcio2MediaDev_ = enumerator->search(cio2_dm);\n> > +\tif (!cio2MediaDev_)\n> >  \t\treturn false;\n> >\n> > -\tcio2_->acquire();\n> > +\tcio2MediaDev_->acquire();\n> >\n> > -\timgu_ = enumerator->search(imgu_dm);\n> > -\tif (!imgu_)\n> > +\timguMediaDev_ = enumerator->search(imgu_dm);\n> > +\tif (!imguMediaDev_)\n> >  \t\treturn false;\n> >\n> > -\timgu_->acquire();\n> > +\timguMediaDev_->acquire();\n> >\n> >  \t/*\n> >  \t * Disable all links that are enabled by default on CIO2, as camera\n> > @@ -331,17 +350,17 @@ bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator)\n> >  \t * Close the CIO2 media device after, as links are enabled and should\n> >  \t * not need to be changed after.\n> >  \t */\n> > -\tif (cio2_->open())\n> > +\tif (cio2MediaDev_->open())\n> >  \t\treturn false;\n> >\n> > -\tif (cio2_->disableLinks()) {\n> > -\t\tcio2_->close();\n> > +\tif (cio2MediaDev_->disableLinks()) {\n> > +\t\tcio2MediaDev_->close();\n> >  \t\treturn false;\n> >  \t}\n> >\n> >  \tregisterCameras();\n> >\n> > -\tcio2_->close();\n> > +\tcio2MediaDev_->close();\n> >\n> >  \treturn true;\n> >  }\n> > @@ -355,115 +374,28 @@ void PipelineHandlerIPU3::registerCameras()\n> >  {\n> >  \t/*\n> >  \t * For each CSI-2 receiver on the IPU3, create a Camera if an\n> > -\t * image sensor is connected to it.\n> > +\t * image sensor is connected to it and it can produce images in\n> > +\t * a compatible format.\n> >  \t */\n> >  \tunsigned int numCameras = 0;\n> >  \tfor (unsigned int id = 0; id < 4; ++id) {\n> > -\t\tstd::string csi2Name = \"ipu3-csi2 \" + std::to_string(id);\n> > -\t\tMediaEntity *csi2 = cio2_->getEntityByName(csi2Name);\n> > -\t\tint ret;\n> > -\n> > -\t\t/*\n> > -\t\t * This shall not happen, as the device enumerator matched\n> > -\t\t * all entities described in the cio2_dm DeviceMatch.\n> > -\t\t *\n> > -\t\t * As this check is basically free, better stay safe than sorry.\n> > -\t\t */\n> > -\t\tif (!csi2)\n> > -\t\t\tcontinue;\n> > -\n> > -\t\tconst std::vector<MediaPad *> &pads = csi2->pads();\n> > -\t\tif (pads.empty())\n> > -\t\t\tcontinue;\n> > -\n> > -\t\t/* IPU3 CSI-2 receivers have a single sink pad at index 0. */\n> > -\t\tMediaPad *sink = pads[0];\n> > -\t\tconst std::vector<MediaLink *> &links = sink->links();\n> > -\t\tif (links.empty())\n> > -\t\t\tcontinue;\n> > -\n> > -\t\t/*\n> > -\t\t * Verify that the receiver is connected to a sensor, enable\n> > -\t\t * the media link between the two, and create a Camera with\n> > -\t\t * a unique name.\n> > -\t\t */\n> > -\t\tMediaLink *link = links[0];\n> > -\t\tMediaEntity *sensor = link->source()->entity();\n> > -\t\tif (sensor->function() != MEDIA_ENT_F_CAM_SENSOR)\n> > -\t\t\tcontinue;\n> > -\n> > -\t\tif (link->setEnabled(true))\n> > -\t\t\tcontinue;\n> > -\n> > -\t\tstd::unique_ptr<IPU3CameraData> data = utils::make_unique<IPU3CameraData>(this);\n> > -\n> > -\t\tstd::string cameraName = sensor->name() + \" \" + std::to_string(id);\n> > +\t\tstd::unique_ptr<IPU3CameraData> data =\n> > +\t\t\tutils::make_unique<IPU3CameraData>(this);\n> >  \t\tstd::set<Stream *> streams{ &data->stream_ };\n> > -\t\tstd::shared_ptr<Camera> camera = Camera::create(this, cameraName, streams);\n> > -\n> > -\t\t/*\n> > -\t\t * Create and open video devices and subdevices associated with\n> > -\t\t * the camera.\n> > -\t\t *\n> > -\t\t * If any of these operations fails, the Camera instance won't\n> > -\t\t * be registered. The 'camera' shared pointer and the 'data'\n> > -\t\t * unique pointers go out of scope and delete the objects they\n> > -\t\t * manage.\n> > -\t\t */\n> > -\t\tstd::string cio2Name = \"ipu3-cio2 \" + std::to_string(id);\n> > -\t\tMediaEntity *cio2 = cio2_->getEntityByName(cio2Name);\n> > -\t\tif (!cio2) {\n> > -\t\t\tLOG(IPU3, Error)\n> > -\t\t\t\t<< \"Failed to get entity '\" << cio2Name << \"'\";\n> > -\t\t\tcontinue;\n> > -\t\t}\n> > -\n> > -\t\tdata->cio2_ = new V4L2Device(cio2);\n> > -\t\tret = data->cio2_->open();\n> > -\t\tif (ret)\n> > -\t\t\tcontinue;\n> > +\t\tCIO2Device *cio2 = &data->cio2_;\n> >\n> > -\t\t/*\n> > -\t\t * Make sure the sensor produces at least one image format\n> > -\t\t * compatible with IPU3 CIO2 requirements and cache the camera\n> > -\t\t * maximum sizes.\n> > -\t\t */\n> > -\t\tdata->sensor_ = new V4L2Subdevice(sensor);\n> > -\t\tret = data->sensor_->open();\n> > +\t\tint ret = cio2->init(cio2MediaDev_.get(), id);\n> >  \t\tif (ret)\n> >  \t\t\tcontinue;\n> >\n> > -\t\tfor (auto it : data->sensor_->formats(0)) {\n> > -\t\t\tint mbusCode = mediaBusToCIO2Format(it.first);\n> > -\t\t\tif (mbusCode < 0)\n> > -\t\t\t\tcontinue;\n> > -\n> > -\t\t\tfor (const SizeRange &size : it.second) {\n> > -\t\t\t\tSizeRange &maxSize = data->maxSizes_.second;\n> > -\n> > -\t\t\t\tif (maxSize.maxWidth < size.maxWidth &&\n> > -\t\t\t\t    maxSize.maxHeight < size.maxHeight) {\n> > -\t\t\t\t\tmaxSize.maxWidth = size.maxWidth;\n> > -\t\t\t\t\tmaxSize.maxHeight = size.maxHeight;\n> > -\t\t\t\t\tdata->maxSizes_.first = mbusCode;\n> > -\t\t\t\t}\n> > -\t\t\t}\n> > -\t\t}\n> > -\t\tif (data->maxSizes_.second.maxWidth == 0) {\n> > -\t\t\tLOG(IPU3, Info)\n> > -\t\t\t\t<< \"Sensor '\" << data->sensor_->entityName()\n> > -\t\t\t\t<< \"' detected, but no supported image format \"\n> > -\t\t\t\t<< \" found: skip camera creation\";\n> > -\t\t\tcontinue;\n> > -\t\t}\n> > -\n> > -\t\tdata->csi2_ = new V4L2Subdevice(csi2);\n> > -\t\tret = data->csi2_->open();\n> > -\t\tif (ret)\n> > -\t\t\tcontinue;\n> > +\t\tstd::string cameraName = cio2->name() + \" \"\n> > +\t\t\t\t       + std::to_string(id);\n> > +\t\tstd::shared_ptr<Camera> camera = Camera::create(this,\n> > +\t\t\t\t\t\t\t\tcameraName,\n> > +\t\t\t\t\t\t\t\tstreams);\n> >\n> > -\t\tdata->cio2_->bufferReady.connect(data.get(),\n> > -\t\t\t\t\t\t &IPU3CameraData::bufferReady);\n> > +\t\tcio2->output_->bufferReady.connect(data.get(),\n> > +\t\t\t\t\t\t   &IPU3CameraData::bufferReady);\n> >\n> >  \t\tregisterCamera(std::move(camera), std::move(data));\n> >\n> > @@ -484,6 +416,104 @@ void PipelineHandlerIPU3::IPU3CameraData::bufferReady(Buffer *buffer)\n> >  \tpipe_->completeRequest(camera_, request);\n> >  }\n> >\n> > +/*------------------------------------------------------------------------------\n> > + * CIO2 Device\n> > + */\n> > +\n> > +const std::string &CIO2Device::name() const\n> > +{\n> > +\treturn sensor_->entityName();\n> > +}\n> > +\n> > +/**\n> > + * \\brief Initialize components of the CIO2 device with \\a index\n> > + * \\param[in] media The CIO2 media device\n> > + * \\param[in] index The CIO2 device index\n> > + *\n> > + * Create and open the device and subdevices in the CIO2 instance at \\a index,\n> > + * if an image sensor is connected to the CSI-2 receiver of this CIO2 instance.\n> > + * Enable the media links connecting the CIO2 components to prepare for capture\n> > + * operations and cache the sensor maximum sizes.\n> > + *\n> > + * \\return 0 on success or a negative error code otherwise\n> > + * \\retval -ENODEV No image sensor is connected to this CIO2 instance\n> > + */\n> > +int CIO2Device::init(const MediaDevice *media, unsigned int index)\n> > +{\n> > +\tint ret;\n> > +\n> > +\t/* Verify that a sensor subdevice is connected to this CIO2 instance. */\n> > +\tstd::string csi2Name = \"ipu3-csi2 \" + std::to_string(index);\n> > +\tMediaEntity *csi2Entity = media->getEntityByName(csi2Name);\n> > +\tconst std::vector<MediaPad *> &pads = csi2Entity->pads();\n> > +\tif (pads.empty())\n> > +\t\treturn -ENODEV;\n> > +\n> > +\t/* IPU3 CSI-2 receivers have a single sink pad at index 0. */\n> > +\tMediaPad *sink = pads[0];\n> > +\tconst std::vector<MediaLink *> &links = sink->links();\n> > +\tif (links.empty())\n> > +\t\treturn -ENODEV;\n> > +\n> > +\tMediaLink *link = links[0];\n> > +\tMediaEntity *sensorEntity = link->source()->entity();\n> > +\tif (sensorEntity->function() != MEDIA_ENT_F_CAM_SENSOR)\n> > +\t\treturn -ENODEV;\n> > +\n> > +\tret = link->setEnabled(true);\n> > +\tif (ret)\n> > +\t\treturn ret;\n> > +\n> > +\t/*\n> > +\t * Now that we're sure a sensor subdevice is connected, make sure it\n> > +\t * produces at least one image format compatible with CIO2 requirements\n> > +\t * and cache the camera maximum sizes.\n> > +\t *\n> > +\t * \\todo Define when to open and close video device nodes, as they\n> > +\t * might impact on power consumption.\n> > +\t */\n> > +\tsensor_ = new V4L2Subdevice(sensorEntity);\n> > +\tret = sensor_->open();\n> > +\tif (ret)\n> > +\t\treturn ret;\n> > +\n> > +\tfor (auto it : sensor_->formats(0)) {\n> > +\t\tint mbusCode = mediaBusToCIO2Format(it.first);\n> > +\t\tif (mbusCode < 0)\n> > +\t\t\tcontinue;\n> > +\n> > +\t\tfor (const SizeRange &size : it.second) {\n> > +\t\t\tSizeRange &maxRange = maxSizes_.second;\n> > +\n> > +\t\t\tif (maxRange.maxWidth < size.maxWidth &&\n> > +\t\t\t    maxRange.maxHeight < size.maxHeight) {\n> > +\t\t\t\tmaxRange.maxWidth = size.maxWidth;\n> > +\t\t\t\tmaxRange.maxHeight = size.maxHeight;\n> > +\t\t\t\tmaxSizes_.first = mbusCode;\n> > +\t\t\t}\n> > +\t\t}\n> > +\t}\n> > +\tif (maxSizes_.second.maxWidth == 0) {\n> > +\t\tLOG(IPU3, Info) << \"Sensor '\" << sensor_->entityName()\n> > +\t\t\t\t<< \"' detected, but no supported image format \"\n> > +\t\t\t\t<< \" found: skip camera creation\";\n> > +\t\treturn -ENODEV;\n> > +\t}\n> > +\n> > +\tcsi2_ = new V4L2Subdevice(csi2Entity);\n> > +\tret = csi2_->open();\n> > +\tif (ret)\n> > +\t\treturn ret;\n> > +\n> > +\tstd::string cio2Name = \"ipu3-cio2 \" + std::to_string(index);\n> > +\toutput_ = V4L2Device::fromEntityName(media, cio2Name);\n> > +\tret = output_->open();\n> > +\tif (ret)\n> > +\t\treturn ret;\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> >  REGISTER_PIPELINE_HANDLER(PipelineHandlerIPU3);\n> >\n> >  } /* namespace libcamera */\n> > --\n> > 2.21.0\n> >\n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel\n>\n> --\n> Regards,\n> Niklas Söderlund","headers":{"Return-Path":"<jacopo@jmondi.org>","Received":["from relay4-d.mail.gandi.net (relay4-d.mail.gandi.net\n\t[217.70.183.196])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D757660DB3\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue,  2 Apr 2019 14:27:42 +0200 (CEST)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay4-d.mail.gandi.net (Postfix) with ESMTPSA id 399B6E0008;\n\tTue,  2 Apr 2019 12:27:41 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Tue, 2 Apr 2019 14:28:22 +0200","From":"Jacopo Mondi <jacopo@jmondi.org>","To":"Niklas =?utf-8?q?S=C3=B6derlund?= <niklas.soderlund@ragnatech.se>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190402122822.kwqe7wdqnwoz5zzz@uno.localdomain>","References":"<20190326083902.26121-1-jacopo@jmondi.org>\n\t<20190326083902.26121-11-jacopo@jmondi.org>\n\t<20190402112222.GM23466@bigcity.dyn.berto.se>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"bxuwpv7fozbtjqll\"","Content-Disposition":"inline","In-Reply-To":"<20190402112222.GM23466@bigcity.dyn.berto.se>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH v5 10/19] libcamera: ipu3: Create\n\tCIO2Device class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Tue, 02 Apr 2019 12:27:43 -0000"}}]