| Message ID | 20211126003118.42356-4-djrscally@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Daniel, Thank you for the patch. On Fri, Nov 26, 2021 at 12:31:16AM +0000, Daniel Scally wrote: > Rather than attempting to discover VCMs via model name matching we > should follow the ancillary links that define the relationship > between the two devices to locate any entities linked to the sensor. > Where we have linked entities with the function MEDIA_ENT_F_LENS we > can then create an instance of CameraLens to represent it. > > Signed-off-by: Daniel Scally <djrscally@gmail.com> Given that Han-lin's patch that add model-based matching hasn't been merged yet, I think this should be rebased on top of the master branch directly. There's no need to merge model-based matching to replace it immediately after. > --- > > Alternatively rather than replace the matching on model names we could try > the ancillary links first and then simply guard the existing model matching > with an if (!lens_) ... I think ancillary links are good enough, there's no need for model-based matching. > src/libcamera/pipeline/ipu3/cio2.cpp | 45 +++++++++++++++------------- > 1 file changed, 25 insertions(+), 20 deletions(-) > > diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp > index 59b2f586..169e7b54 100644 > --- a/src/libcamera/pipeline/ipu3/cio2.cpp > +++ b/src/libcamera/pipeline/ipu3/cio2.cpp > @@ -161,30 +161,35 @@ int CIO2Device::init(const MediaDevice *media, unsigned int index) > } > > /* > - * \todo Read the lens model from the sensor itself or from a device > - * database. For now use default values taken from ChromeOS database. > + * Sensors sometimes have ancillary devices such as a Lens or Flash > + * that could be linked to the MediaEntity - search for and handle > + * any such device (for now, we can only handle MEDIA_ENT_F_LENS) > + * > + * \todo Handle MEDIA_ENT_F_FLASH too. > */ > - static std::unordered_map<std::string, std::string> sensorLens = { > - { "ov13858", "dw9714" }, > - { "imx258", "dw9807" }, > - { "imx355", "ak7375" } > - }; > - > - auto it = sensorLens.find(sensor_->model()); > - if (it != sensorLens.end()) { > - const std::vector<MediaEntity *> &entities = media->entities(); > - for (auto ent : entities) { > - if (ent->function() == MEDIA_ENT_F_LENS) { > - lens_ = std::make_unique<CameraLens>(ent); > + const std::vector<MediaLink *> &ancillary_links = sensorEntity->ancillary_links(); > + if (!ancillary_links.empty()) { > + for (auto it : ancillary_links) { > + MediaEntity *ancillaryEntity = it->ancillary(); > + > + switch (ancillaryEntity->function()) { > + case MEDIA_ENT_F_LENS: > + lens_ = std::make_unique<CameraLens>(ancillaryEntity); > ret = lens_->init(); > - if (!ret && lens_->model() == it->second) { > - break; > + if (ret) { > + LOG(IPU3, Error) > + << "Error during CameraLens init"; > + return ret; > } > - lens_.reset(); > + break; > + case MEDIA_ENT_F_FLASH: > + LOG(IPU3, Warning) > + << "Flash not yet supported"; > + break; > + default: > + LOG(IPU3, Warning) > + << "Unsupported entity function"; > } Could we simplify the pipeline handlers a bit by moving at least the lookup of the VCM entity to the CameraSensor class ? > - if (!lens_) > - LOG(IPU3, Warning) << "Lens device " > - << it->second << " not found"; > } > } >
Hi Laurent On 28/11/2021 22:20, Laurent Pinchart wrote: > Hi Daniel, > > Thank you for the patch. > > On Fri, Nov 26, 2021 at 12:31:16AM +0000, Daniel Scally wrote: >> Rather than attempting to discover VCMs via model name matching we >> should follow the ancillary links that define the relationship >> between the two devices to locate any entities linked to the sensor. >> Where we have linked entities with the function MEDIA_ENT_F_LENS we >> can then create an instance of CameraLens to represent it. >> >> Signed-off-by: Daniel Scally <djrscally@gmail.com> > Given that Han-lin's patch that add model-based matching hasn't been > merged yet, I think this should be rebased on top of the master branch > directly. There's no need to merge model-based matching to replace it > immediately after. Did the patch introducing the CameraLens class get merged, then? I can rebase onto master in that case, no problem. >> --- >> >> Alternatively rather than replace the matching on model names we could try >> the ancillary links first and then simply guard the existing model matching >> with an if (!lens_) ... > I think ancillary links are good enough, there's no need for model-based > matching. Righto. > >> src/libcamera/pipeline/ipu3/cio2.cpp | 45 +++++++++++++++------------- >> 1 file changed, 25 insertions(+), 20 deletions(-) >> >> diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp >> index 59b2f586..169e7b54 100644 >> --- a/src/libcamera/pipeline/ipu3/cio2.cpp >> +++ b/src/libcamera/pipeline/ipu3/cio2.cpp >> @@ -161,30 +161,35 @@ int CIO2Device::init(const MediaDevice *media, unsigned int index) >> } >> >> /* >> - * \todo Read the lens model from the sensor itself or from a device >> - * database. For now use default values taken from ChromeOS database. >> + * Sensors sometimes have ancillary devices such as a Lens or Flash >> + * that could be linked to the MediaEntity - search for and handle >> + * any such device (for now, we can only handle MEDIA_ENT_F_LENS) >> + * >> + * \todo Handle MEDIA_ENT_F_FLASH too. >> */ >> - static std::unordered_map<std::string, std::string> sensorLens = { >> - { "ov13858", "dw9714" }, >> - { "imx258", "dw9807" }, >> - { "imx355", "ak7375" } >> - }; >> - >> - auto it = sensorLens.find(sensor_->model()); >> - if (it != sensorLens.end()) { >> - const std::vector<MediaEntity *> &entities = media->entities(); >> - for (auto ent : entities) { >> - if (ent->function() == MEDIA_ENT_F_LENS) { >> - lens_ = std::make_unique<CameraLens>(ent); >> + const std::vector<MediaLink *> &ancillary_links = sensorEntity->ancillary_links(); >> + if (!ancillary_links.empty()) { >> + for (auto it : ancillary_links) { >> + MediaEntity *ancillaryEntity = it->ancillary(); >> + >> + switch (ancillaryEntity->function()) { >> + case MEDIA_ENT_F_LENS: >> + lens_ = std::make_unique<CameraLens>(ancillaryEntity); >> ret = lens_->init(); >> - if (!ret && lens_->model() == it->second) { >> - break; >> + if (ret) { >> + LOG(IPU3, Error) >> + << "Error during CameraLens init"; >> + return ret; >> } >> - lens_.reset(); >> + break; >> + case MEDIA_ENT_F_FLASH: >> + LOG(IPU3, Warning) >> + << "Flash not yet supported"; >> + break; >> + default: >> + LOG(IPU3, Warning) >> + << "Unsupported entity function"; >> } > Could we simplify the pipeline handlers a bit by moving at least the > lookup of the VCM entity to the CameraSensor class ? Sure; I actually moved it to ::init() now, that sound ok? >> - if (!lens_) >> - LOG(IPU3, Warning) << "Lens device " >> - << it->second << " not found"; >> } >> } >>
Hi Daniel, On Sun, Nov 28, 2021 at 11:13:26PM +0000, Daniel Scally wrote: > On 28/11/2021 22:20, Laurent Pinchart wrote: > > On Fri, Nov 26, 2021 at 12:31:16AM +0000, Daniel Scally wrote: > >> Rather than attempting to discover VCMs via model name matching we > >> should follow the ancillary links that define the relationship > >> between the two devices to locate any entities linked to the sensor. > >> Where we have linked entities with the function MEDIA_ENT_F_LENS we > >> can then create an instance of CameraLens to represent it. > >> > >> Signed-off-by: Daniel Scally <djrscally@gmail.com> > > > > Given that Han-lin's patch that add model-based matching hasn't been > > merged yet, I think this should be rebased on top of the master branch > > directly. There's no need to merge model-based matching to replace it > > immediately after. > > Did the patch introducing the CameraLens class get merged, then? I can > rebase onto master in that case, no problem. Not yet. Feel free to include it in your series to develop its integration. > >> --- > >> > >> Alternatively rather than replace the matching on model names we could try > >> the ancillary links first and then simply guard the existing model matching > >> with an if (!lens_) ... > > > > I think ancillary links are good enough, there's no need for model-based > > matching. > > Righto. > > >> src/libcamera/pipeline/ipu3/cio2.cpp | 45 +++++++++++++++------------- > >> 1 file changed, 25 insertions(+), 20 deletions(-) > >> > >> diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp > >> index 59b2f586..169e7b54 100644 > >> --- a/src/libcamera/pipeline/ipu3/cio2.cpp > >> +++ b/src/libcamera/pipeline/ipu3/cio2.cpp > >> @@ -161,30 +161,35 @@ int CIO2Device::init(const MediaDevice *media, unsigned int index) > >> } > >> > >> /* > >> - * \todo Read the lens model from the sensor itself or from a device > >> - * database. For now use default values taken from ChromeOS database. > >> + * Sensors sometimes have ancillary devices such as a Lens or Flash > >> + * that could be linked to the MediaEntity - search for and handle > >> + * any such device (for now, we can only handle MEDIA_ENT_F_LENS) > >> + * > >> + * \todo Handle MEDIA_ENT_F_FLASH too. > >> */ > >> - static std::unordered_map<std::string, std::string> sensorLens = { > >> - { "ov13858", "dw9714" }, > >> - { "imx258", "dw9807" }, > >> - { "imx355", "ak7375" } > >> - }; > >> - > >> - auto it = sensorLens.find(sensor_->model()); > >> - if (it != sensorLens.end()) { > >> - const std::vector<MediaEntity *> &entities = media->entities(); > >> - for (auto ent : entities) { > >> - if (ent->function() == MEDIA_ENT_F_LENS) { > >> - lens_ = std::make_unique<CameraLens>(ent); > >> + const std::vector<MediaLink *> &ancillary_links = sensorEntity->ancillary_links(); > >> + if (!ancillary_links.empty()) { > >> + for (auto it : ancillary_links) { > >> + MediaEntity *ancillaryEntity = it->ancillary(); > >> + > >> + switch (ancillaryEntity->function()) { > >> + case MEDIA_ENT_F_LENS: > >> + lens_ = std::make_unique<CameraLens>(ancillaryEntity); > >> ret = lens_->init(); > >> - if (!ret && lens_->model() == it->second) { > >> - break; > >> + if (ret) { > >> + LOG(IPU3, Error) > >> + << "Error during CameraLens init"; > >> + return ret; > >> } > >> - lens_.reset(); > >> + break; > >> + case MEDIA_ENT_F_FLASH: > >> + LOG(IPU3, Warning) > >> + << "Flash not yet supported"; > >> + break; > >> + default: > >> + LOG(IPU3, Warning) > >> + << "Unsupported entity function"; > >> } > > > > Could we simplify the pipeline handlers a bit by moving at least the > > lookup of the VCM entity to the CameraSensor class ? > > Sure; I actually moved it to ::init() now, that sound ok? Do you mean CameraSensor::init() ? Yes, that sounds fine (although you may want to move it to a private function called by init(), there's lots being done at init time already). > >> - if (!lens_) > >> - LOG(IPU3, Warning) << "Lens device " > >> - << it->second << " not found"; > >> } > >> } > >>
diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp index 59b2f586..169e7b54 100644 --- a/src/libcamera/pipeline/ipu3/cio2.cpp +++ b/src/libcamera/pipeline/ipu3/cio2.cpp @@ -161,30 +161,35 @@ int CIO2Device::init(const MediaDevice *media, unsigned int index) } /* - * \todo Read the lens model from the sensor itself or from a device - * database. For now use default values taken from ChromeOS database. + * Sensors sometimes have ancillary devices such as a Lens or Flash + * that could be linked to the MediaEntity - search for and handle + * any such device (for now, we can only handle MEDIA_ENT_F_LENS) + * + * \todo Handle MEDIA_ENT_F_FLASH too. */ - static std::unordered_map<std::string, std::string> sensorLens = { - { "ov13858", "dw9714" }, - { "imx258", "dw9807" }, - { "imx355", "ak7375" } - }; - - auto it = sensorLens.find(sensor_->model()); - if (it != sensorLens.end()) { - const std::vector<MediaEntity *> &entities = media->entities(); - for (auto ent : entities) { - if (ent->function() == MEDIA_ENT_F_LENS) { - lens_ = std::make_unique<CameraLens>(ent); + const std::vector<MediaLink *> &ancillary_links = sensorEntity->ancillary_links(); + if (!ancillary_links.empty()) { + for (auto it : ancillary_links) { + MediaEntity *ancillaryEntity = it->ancillary(); + + switch (ancillaryEntity->function()) { + case MEDIA_ENT_F_LENS: + lens_ = std::make_unique<CameraLens>(ancillaryEntity); ret = lens_->init(); - if (!ret && lens_->model() == it->second) { - break; + if (ret) { + LOG(IPU3, Error) + << "Error during CameraLens init"; + return ret; } - lens_.reset(); + break; + case MEDIA_ENT_F_FLASH: + LOG(IPU3, Warning) + << "Flash not yet supported"; + break; + default: + LOG(IPU3, Warning) + << "Unsupported entity function"; } - if (!lens_) - LOG(IPU3, Warning) << "Lens device " - << it->second << " not found"; } }
Rather than attempting to discover VCMs via model name matching we should follow the ancillary links that define the relationship between the two devices to locate any entities linked to the sensor. Where we have linked entities with the function MEDIA_ENT_F_LENS we can then create an instance of CameraLens to represent it. Signed-off-by: Daniel Scally <djrscally@gmail.com> --- Alternatively rather than replace the matching on model names we could try the ancillary links first and then simply guard the existing model matching with an if (!lens_) ... src/libcamera/pipeline/ipu3/cio2.cpp | 45 +++++++++++++++------------- 1 file changed, 25 insertions(+), 20 deletions(-)