| Message ID | 20200925014207.1455796-5-niklas.soderlund@ragnatech.se |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Niklas, On Fri, Sep 25, 2020 at 03:41:49AM +0200, Niklas Söderlund wrote: > In preparation of supporting both the main and self path configure all > the media graph links as a part of the configuration step. Before this > change the link between ISP and DMA engine was setup at match time as > the only supported path was the main path and only the link between > sensor and ISP was updated at part of the configuration step. > > The main path is still the only path between ISP and DMA engine that is > possible to enable. > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > * Changes since v2 > - Simplify link selection per Laurent's suggestion. This is reworked > once more in a later patch but makes thins one looks nicer for now. > > * Changes since v1 > - Pass sensor directly to initLinks(). > - Remove string variable form link selection. > - Rewrite commit message. > --- > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 77 ++++++++++++------------ > 1 file changed, 40 insertions(+), 37 deletions(-) > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > index a5b6bb07e4f8dee2..8bb4582eeb688a4c 100644 > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > @@ -213,7 +213,8 @@ private: > friend RkISP1CameraData; > friend RkISP1Frames; > > - int initLinks(); > + int initLinks(const Camera *camera, const CameraSensor *sensor, > + const RkISP1CameraConfiguration &config); > int createCamera(MediaEntity *sensor); > void tryCompleteRequest(Request *request); > void bufferReady(FrameBuffer *buffer); > @@ -598,28 +599,9 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c) > CameraSensor *sensor = data->sensor_; > int ret; > > - /* > - * Configure the sensor links: enable the link corresponding to this > - * camera and disable all the other sensor links. > - */ > - const MediaPad *pad = isp_->entity()->getPadByIndex(0); > - > - for (MediaLink *link : pad->links()) { > - bool enable = link->source()->entity() == sensor->entity(); > - > - if (!!(link->flags() & MEDIA_LNK_FL_ENABLED) == enable) > - continue; > - > - LOG(RkISP1, Debug) > - << (enable ? "Enabling" : "Disabling") > - << " link from sensor '" > - << link->source()->entity()->name() > - << "' to ISP"; > - > - ret = link->setEnabled(enable); > - if (ret < 0) > - return ret; > - } > + ret = initLinks(camera, sensor, *config); > + if (ret) > + return ret; > > /* > * Configure the format on the sensor output and propagate it through > @@ -922,22 +904,49 @@ int PipelineHandlerRkISP1::queueRequestDevice(Camera *camera, Request *request) > * Match and Setup > */ > > -int PipelineHandlerRkISP1::initLinks() > +int PipelineHandlerRkISP1::initLinks(const Camera *camera, > + const CameraSensor *sensor, > + const RkISP1CameraConfiguration &config) > { > - MediaLink *link; > + RkISP1CameraData *data = cameraData(camera); > int ret; > > ret = media_->disableLinks(); > if (ret < 0) > return ret; > > - link = media_->link("rkisp1_isp", 2, "rkisp1_resizer_mainpath", 0); > - if (!link) > - return -ENODEV; > + /* > + * Configure the sensor links: enable the link corresponding to this > + * camera. > + */ > + const MediaPad *pad = isp_->entity()->getPadByIndex(0); > + for (MediaLink *link : pad->links()) { > + if (link->source()->entity() != sensor->entity()) > + continue; > > - ret = link->setEnabled(true); > - if (ret < 0) > - return ret; > + LOG(RkISP1, Debug) > + << "Enabling link from sensor '" > + << link->source()->entity()->name() > + << "' to ISP"; > + > + ret = link->setEnabled(true); The previous version disables links that are not used, while this one only enables them. Is this intended ? Thanks j > + if (ret < 0) > + return ret; > + } > + > + for (const StreamConfiguration &cfg : config) { > + if (cfg.stream() != &data->stream_) > + return -EINVAL; > + > + MediaLink *link = media_->link("rkisp1_isp", 2, > + "rkisp1_resizer_mainpath", 0); > + if (!link) > + return -ENODEV; > + > + ret = link->setEnabled(true); > + if (ret < 0) > + return ret; > + } > > return 0; > } > @@ -1019,12 +1028,6 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator) > stat_->bufferReady.connect(this, &PipelineHandlerRkISP1::statReady); > param_->bufferReady.connect(this, &PipelineHandlerRkISP1::paramReady); > > - /* Configure default links. */ > - if (initLinks() < 0) { > - LOG(RkISP1, Error) << "Failed to setup links"; > - return false; > - } > - > /* > * Enumerate all sensors connected to the ISP and create one > * camera instance for each of them. > -- > 2.28.0 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Jacopo, Thanks for your feedback. On 2020-09-25 16:14:53 +0200, Jacopo Mondi wrote: > Hi Niklas, > > On Fri, Sep 25, 2020 at 03:41:49AM +0200, Niklas Söderlund wrote: > > In preparation of supporting both the main and self path configure all > > the media graph links as a part of the configuration step. Before this > > change the link between ISP and DMA engine was setup at match time as > > the only supported path was the main path and only the link between > > sensor and ISP was updated at part of the configuration step. > > > > The main path is still the only path between ISP and DMA engine that is > > possible to enable. > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > * Changes since v2 > > - Simplify link selection per Laurent's suggestion. This is reworked > > once more in a later patch but makes thins one looks nicer for now. > > > > * Changes since v1 > > - Pass sensor directly to initLinks(). > > - Remove string variable form link selection. > > - Rewrite commit message. > > --- > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 77 ++++++++++++------------ > > 1 file changed, 40 insertions(+), 37 deletions(-) > > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > index a5b6bb07e4f8dee2..8bb4582eeb688a4c 100644 > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > @@ -213,7 +213,8 @@ private: > > friend RkISP1CameraData; > > friend RkISP1Frames; > > > > - int initLinks(); > > + int initLinks(const Camera *camera, const CameraSensor *sensor, > > + const RkISP1CameraConfiguration &config); > > int createCamera(MediaEntity *sensor); > > void tryCompleteRequest(Request *request); > > void bufferReady(FrameBuffer *buffer); > > @@ -598,28 +599,9 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c) > > CameraSensor *sensor = data->sensor_; > > int ret; > > > > - /* > > - * Configure the sensor links: enable the link corresponding to this > > - * camera and disable all the other sensor links. > > - */ > > - const MediaPad *pad = isp_->entity()->getPadByIndex(0); > > - > > - for (MediaLink *link : pad->links()) { > > - bool enable = link->source()->entity() == sensor->entity(); > > - > > - if (!!(link->flags() & MEDIA_LNK_FL_ENABLED) == enable) > > - continue; > > - > > - LOG(RkISP1, Debug) > > - << (enable ? "Enabling" : "Disabling") > > - << " link from sensor '" > > - << link->source()->entity()->name() > > - << "' to ISP"; > > - > > - ret = link->setEnabled(enable); > > - if (ret < 0) > > - return ret; > > - } > > + ret = initLinks(camera, sensor, *config); > > + if (ret) > > + return ret; > > > > /* > > * Configure the format on the sensor output and propagate it through > > @@ -922,22 +904,49 @@ int PipelineHandlerRkISP1::queueRequestDevice(Camera *camera, Request *request) > > * Match and Setup > > */ > > > > -int PipelineHandlerRkISP1::initLinks() > > +int PipelineHandlerRkISP1::initLinks(const Camera *camera, > > + const CameraSensor *sensor, > > + const RkISP1CameraConfiguration &config) > > { > > - MediaLink *link; > > + RkISP1CameraData *data = cameraData(camera); > > int ret; > > > > ret = media_->disableLinks(); > > if (ret < 0) > > return ret; > > > > - link = media_->link("rkisp1_isp", 2, "rkisp1_resizer_mainpath", 0); > > - if (!link) > > - return -ENODEV; > > + /* > > + * Configure the sensor links: enable the link corresponding to this > > + * camera. > > + */ > > + const MediaPad *pad = isp_->entity()->getPadByIndex(0); > > + for (MediaLink *link : pad->links()) { > > + if (link->source()->entity() != sensor->entity()) > > + continue; > > > > - ret = link->setEnabled(true); > > - if (ret < 0) > > - return ret; > > + LOG(RkISP1, Debug) > > + << "Enabling link from sensor '" > > + << link->source()->entity()->name() > > + << "' to ISP"; > > + > > + ret = link->setEnabled(true); > > The previous version disables links that are not used, while this one > only enables them. Is this intended ? Yes, the call to disableLinks() above disables all links so there is no point in disabling an already disabled link. > > Thanks > j > > > + if (ret < 0) > > + return ret; > > + } > > + > > + for (const StreamConfiguration &cfg : config) { > > + if (cfg.stream() != &data->stream_) > > + return -EINVAL; > > + > > + MediaLink *link = media_->link("rkisp1_isp", 2, > > + "rkisp1_resizer_mainpath", 0); > > + if (!link) > > + return -ENODEV; > > + > > + ret = link->setEnabled(true); > > + if (ret < 0) > > + return ret; > > + } > > > > return 0; > > } > > @@ -1019,12 +1028,6 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator) > > stat_->bufferReady.connect(this, &PipelineHandlerRkISP1::statReady); > > param_->bufferReady.connect(this, &PipelineHandlerRkISP1::paramReady); > > > > - /* Configure default links. */ > > - if (initLinks() < 0) { > > - LOG(RkISP1, Error) << "Failed to setup links"; > > - return false; > > - } > > - > > /* > > * Enumerate all sensors connected to the ISP and create one > > * camera instance for each of them. > > -- > > 2.28.0 > > > > _______________________________________________ > > libcamera-devel mailing list > > libcamera-devel@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Niklas, On Fri, Sep 25, 2020 at 06:16:07PM +0200, Niklas Söderlund wrote: > Hi Jacopo, > > Thanks for your feedback. > > On 2020-09-25 16:14:53 +0200, Jacopo Mondi wrote: > > Hi Niklas, > > > > On Fri, Sep 25, 2020 at 03:41:49AM +0200, Niklas Söderlund wrote: > > > In preparation of supporting both the main and self path configure all > > > the media graph links as a part of the configuration step. Before this > > > change the link between ISP and DMA engine was setup at match time as > > > the only supported path was the main path and only the link between > > > sensor and ISP was updated at part of the configuration step. > > > > > > The main path is still the only path between ISP and DMA engine that is > > > possible to enable. > > > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > --- > > > * Changes since v2 > > > - Simplify link selection per Laurent's suggestion. This is reworked > > > once more in a later patch but makes thins one looks nicer for now. > > > > > > * Changes since v1 > > > - Pass sensor directly to initLinks(). > > > - Remove string variable form link selection. > > > - Rewrite commit message. > > > --- > > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 77 ++++++++++++------------ > > > 1 file changed, 40 insertions(+), 37 deletions(-) > > > > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > > index a5b6bb07e4f8dee2..8bb4582eeb688a4c 100644 > > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > > @@ -213,7 +213,8 @@ private: > > > friend RkISP1CameraData; > > > friend RkISP1Frames; > > > > > > - int initLinks(); > > > + int initLinks(const Camera *camera, const CameraSensor *sensor, > > > + const RkISP1CameraConfiguration &config); > > > int createCamera(MediaEntity *sensor); > > > void tryCompleteRequest(Request *request); > > > void bufferReady(FrameBuffer *buffer); > > > @@ -598,28 +599,9 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c) > > > CameraSensor *sensor = data->sensor_; > > > int ret; > > > > > > - /* > > > - * Configure the sensor links: enable the link corresponding to this > > > - * camera and disable all the other sensor links. > > > - */ > > > - const MediaPad *pad = isp_->entity()->getPadByIndex(0); > > > - > > > - for (MediaLink *link : pad->links()) { > > > - bool enable = link->source()->entity() == sensor->entity(); > > > - > > > - if (!!(link->flags() & MEDIA_LNK_FL_ENABLED) == enable) > > > - continue; > > > - > > > - LOG(RkISP1, Debug) > > > - << (enable ? "Enabling" : "Disabling") > > > - << " link from sensor '" > > > - << link->source()->entity()->name() > > > - << "' to ISP"; > > > - > > > - ret = link->setEnabled(enable); > > > - if (ret < 0) > > > - return ret; > > > - } > > > + ret = initLinks(camera, sensor, *config); > > > + if (ret) > > > + return ret; > > > > > > /* > > > * Configure the format on the sensor output and propagate it through > > > @@ -922,22 +904,49 @@ int PipelineHandlerRkISP1::queueRequestDevice(Camera *camera, Request *request) > > > * Match and Setup > > > */ > > > > > > -int PipelineHandlerRkISP1::initLinks() > > > +int PipelineHandlerRkISP1::initLinks(const Camera *camera, > > > + const CameraSensor *sensor, > > > + const RkISP1CameraConfiguration &config) > > > { > > > - MediaLink *link; > > > + RkISP1CameraData *data = cameraData(camera); > > > int ret; > > > > > > ret = media_->disableLinks(); > > > if (ret < 0) > > > return ret; > > > > > > - link = media_->link("rkisp1_isp", 2, "rkisp1_resizer_mainpath", 0); > > > - if (!link) > > > - return -ENODEV; > > > + /* > > > + * Configure the sensor links: enable the link corresponding to this > > > + * camera. > > > + */ > > > + const MediaPad *pad = isp_->entity()->getPadByIndex(0); > > > + for (MediaLink *link : pad->links()) { > > > + if (link->source()->entity() != sensor->entity()) > > > + continue; > > > > > > - ret = link->setEnabled(true); > > > - if (ret < 0) > > > - return ret; > > > + LOG(RkISP1, Debug) > > > + << "Enabling link from sensor '" > > > + << link->source()->entity()->name() > > > + << "' to ISP"; > > > + > > > + ret = link->setEnabled(true); > > > > The previous version disables links that are not used, while this one > > only enables them. Is this intended ? > > Yes, the call to disableLinks() above disables all links so there is no > point in disabling an already disabled link. > Ah right, sorry, missed that! > > > > Thanks > > j > > > > > + if (ret < 0) > > > + return ret; > > > + } > > > + > > > + for (const StreamConfiguration &cfg : config) { > > > + if (cfg.stream() != &data->stream_) > > > + return -EINVAL; > > > + > > > + MediaLink *link = media_->link("rkisp1_isp", 2, > > > + "rkisp1_resizer_mainpath", 0); > > > + if (!link) > > > + return -ENODEV; > > > + > > > + ret = link->setEnabled(true); > > > + if (ret < 0) > > > + return ret; > > > + } > > > > > > return 0; > > > } > > > @@ -1019,12 +1028,6 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator) > > > stat_->bufferReady.connect(this, &PipelineHandlerRkISP1::statReady); > > > param_->bufferReady.connect(this, &PipelineHandlerRkISP1::paramReady); > > > > > > - /* Configure default links. */ > > > - if (initLinks() < 0) { > > > - LOG(RkISP1, Error) << "Failed to setup links"; > > > - return false; > > > - } > > > - > > > /* > > > * Enumerate all sensors connected to the ISP and create one > > > * camera instance for each of them. > > > -- > > > 2.28.0 > > > > > > _______________________________________________ > > > libcamera-devel mailing list > > > libcamera-devel@lists.libcamera.org > > > https://lists.libcamera.org/listinfo/libcamera-devel > > -- > Regards, > Niklas Söderlund
diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp index a5b6bb07e4f8dee2..8bb4582eeb688a4c 100644 --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp @@ -213,7 +213,8 @@ private: friend RkISP1CameraData; friend RkISP1Frames; - int initLinks(); + int initLinks(const Camera *camera, const CameraSensor *sensor, + const RkISP1CameraConfiguration &config); int createCamera(MediaEntity *sensor); void tryCompleteRequest(Request *request); void bufferReady(FrameBuffer *buffer); @@ -598,28 +599,9 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c) CameraSensor *sensor = data->sensor_; int ret; - /* - * Configure the sensor links: enable the link corresponding to this - * camera and disable all the other sensor links. - */ - const MediaPad *pad = isp_->entity()->getPadByIndex(0); - - for (MediaLink *link : pad->links()) { - bool enable = link->source()->entity() == sensor->entity(); - - if (!!(link->flags() & MEDIA_LNK_FL_ENABLED) == enable) - continue; - - LOG(RkISP1, Debug) - << (enable ? "Enabling" : "Disabling") - << " link from sensor '" - << link->source()->entity()->name() - << "' to ISP"; - - ret = link->setEnabled(enable); - if (ret < 0) - return ret; - } + ret = initLinks(camera, sensor, *config); + if (ret) + return ret; /* * Configure the format on the sensor output and propagate it through @@ -922,22 +904,49 @@ int PipelineHandlerRkISP1::queueRequestDevice(Camera *camera, Request *request) * Match and Setup */ -int PipelineHandlerRkISP1::initLinks() +int PipelineHandlerRkISP1::initLinks(const Camera *camera, + const CameraSensor *sensor, + const RkISP1CameraConfiguration &config) { - MediaLink *link; + RkISP1CameraData *data = cameraData(camera); int ret; ret = media_->disableLinks(); if (ret < 0) return ret; - link = media_->link("rkisp1_isp", 2, "rkisp1_resizer_mainpath", 0); - if (!link) - return -ENODEV; + /* + * Configure the sensor links: enable the link corresponding to this + * camera. + */ + const MediaPad *pad = isp_->entity()->getPadByIndex(0); + for (MediaLink *link : pad->links()) { + if (link->source()->entity() != sensor->entity()) + continue; - ret = link->setEnabled(true); - if (ret < 0) - return ret; + LOG(RkISP1, Debug) + << "Enabling link from sensor '" + << link->source()->entity()->name() + << "' to ISP"; + + ret = link->setEnabled(true); + if (ret < 0) + return ret; + } + + for (const StreamConfiguration &cfg : config) { + if (cfg.stream() != &data->stream_) + return -EINVAL; + + MediaLink *link = media_->link("rkisp1_isp", 2, + "rkisp1_resizer_mainpath", 0); + if (!link) + return -ENODEV; + + ret = link->setEnabled(true); + if (ret < 0) + return ret; + } return 0; } @@ -1019,12 +1028,6 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator) stat_->bufferReady.connect(this, &PipelineHandlerRkISP1::statReady); param_->bufferReady.connect(this, &PipelineHandlerRkISP1::paramReady); - /* Configure default links. */ - if (initLinks() < 0) { - LOG(RkISP1, Error) << "Failed to setup links"; - return false; - } - /* * Enumerate all sensors connected to the ISP and create one * camera instance for each of them.