| Message ID | 20200813005246.3265807-4-niklas.soderlund@ragnatech.se |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Niklas, On Thu, Aug 13, 2020 at 02:52:36AM +0200, Niklas Söderlund wrote: > In preparation of supporting both the main and self path move all link all links > setup to be part of the configuration step. The main path is still the > only possible path. > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 79 +++++++++++++----------- > 1 file changed, 42 insertions(+), 37 deletions(-) > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > index a0e36a44d8d91260..38382a6894dac22a 100644 > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > @@ -211,7 +211,8 @@ private: > friend RkISP1CameraData; > friend RkISP1Frames; > > - int initLinks(); > + int initLinks(const Camera *camera, > + const RkISP1CameraConfiguration &config); > int createCamera(MediaEntity *sensor); > void tryCompleteRequest(Request *request); > void bufferReady(FrameBuffer *buffer); > @@ -601,28 +602,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, *config); > + if (ret) > + return ret; > > /* > * Configure the format on the sensor output and propagate it through > @@ -924,22 +906,51 @@ int PipelineHandlerRkISP1::queueRequestDevice(Camera *camera, Request *request) > * Match and Setup > */ > > -int PipelineHandlerRkISP1::initLinks() > +int PipelineHandlerRkISP1::initLinks(const Camera *camera, > + const RkISP1CameraConfiguration &config) > { > - MediaLink *link; > + const RkISP1CameraData *data = cameraData(camera); > + const CameraSensor *sensor = data->sensor_; You could actually pass sensor directly, don't you ? > 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) { > + std::string resizer; > + if (cfg.stream() == &data->stream_) > + resizer = "rkisp1_resizer_mainpath"; > + else > + return -EINVAL; > + > + MediaLink *link = media_->link("rkisp1_isp", 2, resizer, 0); > + if (!link) > + return -ENODEV; > + > + ret = link->setEnabled(true); > + if (ret < 0) > + return ret; > + } I also see a significant behaviour change, the code is not only moved. To be able to actually spot what has changed, could this be broken up in two patches ? One that moves the existing code to a separate function, the other that changes it on top ? > > return 0; > } > @@ -1021,12 +1032,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, Thank you for the patch. On Thu, Aug 13, 2020 at 02:52:36AM +0200, Niklas Söderlund wrote: > In preparation of supporting both the main and self path move all link > setup to be part of the configuration step. The main path is still the > only possible path. > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 79 +++++++++++++----------- > 1 file changed, 42 insertions(+), 37 deletions(-) > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > index a0e36a44d8d91260..38382a6894dac22a 100644 > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > @@ -211,7 +211,8 @@ private: > friend RkISP1CameraData; > friend RkISP1Frames; > > - int initLinks(); > + int initLinks(const Camera *camera, > + const RkISP1CameraConfiguration &config); > int createCamera(MediaEntity *sensor); > void tryCompleteRequest(Request *request); > void bufferReady(FrameBuffer *buffer); > @@ -601,28 +602,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, *config); > + if (ret) > + return ret; > > /* > * Configure the format on the sensor output and propagate it through > @@ -924,22 +906,51 @@ int PipelineHandlerRkISP1::queueRequestDevice(Camera *camera, Request *request) > * Match and Setup > */ > > -int PipelineHandlerRkISP1::initLinks() > +int PipelineHandlerRkISP1::initLinks(const Camera *camera, > + const RkISP1CameraConfiguration &config) > { > - MediaLink *link; > + const RkISP1CameraData *data = cameraData(camera); > + const CameraSensor *sensor = data->sensor_; > 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; > + } This will break switching between cameras, as you would have both links enabled. You not only need to enable the link to the sensor, but disable links to other sensors. The link disable step needs to happen first, as the driver won't (or at least shouldn't) allow two links to be enabled at the same time. > + > + for (const StreamConfiguration &cfg : config) { > + std::string resizer; > + if (cfg.stream() == &data->stream_) > + resizer = "rkisp1_resizer_mainpath"; > + else > + return -EINVAL; I'm not sure to get this change, it doesn't match the commit message. I suspect it belongs to a different patch. > + > + MediaLink *link = media_->link("rkisp1_isp", 2, resizer, 0); > + if (!link) > + return -ENODEV; > + > + ret = link->setEnabled(true); > + if (ret < 0) > + return ret; > + } > > return 0; > } > @@ -1021,12 +1032,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.
Hi Jacopo, On 2020-08-20 10:20:04 +0200, Jacopo Mondi wrote: > Hi Niklas, > > On Thu, Aug 13, 2020 at 02:52:36AM +0200, Niklas Söderlund wrote: > > In preparation of supporting both the main and self path move all link > all links > > > setup to be part of the configuration step. The main path is still the > > only possible path. > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > --- > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 79 +++++++++++++----------- > > 1 file changed, 42 insertions(+), 37 deletions(-) > > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > index a0e36a44d8d91260..38382a6894dac22a 100644 > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > @@ -211,7 +211,8 @@ private: > > friend RkISP1CameraData; > > friend RkISP1Frames; > > > > - int initLinks(); > > + int initLinks(const Camera *camera, > > + const RkISP1CameraConfiguration &config); > > int createCamera(MediaEntity *sensor); > > void tryCompleteRequest(Request *request); > > void bufferReady(FrameBuffer *buffer); > > @@ -601,28 +602,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, *config); > > + if (ret) > > + return ret; > > > > /* > > * Configure the format on the sensor output and propagate it through > > @@ -924,22 +906,51 @@ int PipelineHandlerRkISP1::queueRequestDevice(Camera *camera, Request *request) > > * Match and Setup > > */ > > > > -int PipelineHandlerRkISP1::initLinks() > > +int PipelineHandlerRkISP1::initLinks(const Camera *camera, > > + const RkISP1CameraConfiguration &config) > > { > > - MediaLink *link; > > + const RkISP1CameraData *data = cameraData(camera); > > + const CameraSensor *sensor = data->sensor_; > > You could actually pass sensor directly, don't you ? Grate idea. > > > 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) { > > + std::string resizer; > > + if (cfg.stream() == &data->stream_) > > + resizer = "rkisp1_resizer_mainpath"; > > + else > > + return -EINVAL; > > + > > + MediaLink *link = media_->link("rkisp1_isp", 2, resizer, 0); > > + if (!link) > > + return -ENODEV; > > + > > + ret = link->setEnabled(true); > > + if (ret < 0) > > + return ret; > > + } > > I also see a significant behaviour change, the code is not only moved. > To be able to actually spot what has changed, could this be broken up > in two patches ? One that moves the existing code to a separate > function, the other that changes it on top ? I really can't as the idea is to change the behavior. I will rewrite the commit message to better describe this is not simply a movement of code but change of behavior. Instead of setting up the link between DMA and ISP at match() and sensor and ISP at configure() all links are now setup at the configure() time. > > > > > return 0; > > } > > @@ -1021,12 +1032,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 Laurent, Thanks for your feedback. On 2020-08-20 17:55:59 +0300, Laurent Pinchart wrote: > Hi Niklas, > > Thank you for the patch. > > On Thu, Aug 13, 2020 at 02:52:36AM +0200, Niklas Söderlund wrote: > > In preparation of supporting both the main and self path move all link > > setup to be part of the configuration step. The main path is still the > > only possible path. > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > --- > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 79 +++++++++++++----------- > > 1 file changed, 42 insertions(+), 37 deletions(-) > > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > index a0e36a44d8d91260..38382a6894dac22a 100644 > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > @@ -211,7 +211,8 @@ private: > > friend RkISP1CameraData; > > friend RkISP1Frames; > > > > - int initLinks(); > > + int initLinks(const Camera *camera, > > + const RkISP1CameraConfiguration &config); > > int createCamera(MediaEntity *sensor); > > void tryCompleteRequest(Request *request); > > void bufferReady(FrameBuffer *buffer); > > @@ -601,28 +602,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, *config); > > + if (ret) > > + return ret; > > > > /* > > * Configure the format on the sensor output and propagate it through > > @@ -924,22 +906,51 @@ int PipelineHandlerRkISP1::queueRequestDevice(Camera *camera, Request *request) > > * Match and Setup > > */ > > > > -int PipelineHandlerRkISP1::initLinks() > > +int PipelineHandlerRkISP1::initLinks(const Camera *camera, > > + const RkISP1CameraConfiguration &config) > > { > > - MediaLink *link; > > + const RkISP1CameraData *data = cameraData(camera); > > + const CameraSensor *sensor = data->sensor_; > > 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; > > + } > > This will break switching between cameras, as you would have both links > enabled. You not only need to enable the link to the sensor, but disable > links to other sensors. The link disable step needs to happen first, as > the driver won't (or at least shouldn't) allow two links to be enabled > at the same time. It's not a change of behavior, but it's accomplished in a different way. With this change all links are configured in initLinks() so a first step is to disable all links with a call to 'media_->disableLinks()' all that is left after that is to enable the desired links. > > > + > > + for (const StreamConfiguration &cfg : config) { > > + std::string resizer; > > + if (cfg.stream() == &data->stream_) > > + resizer = "rkisp1_resizer_mainpath"; > > + else > > + return -EINVAL; > > I'm not sure to get this change, it doesn't match the commit message. I > suspect it belongs to a different patch. I will update the commit message as this is intended to be a part of this change. It may however be reworked to remove the intermediate resizer variable and save one LoC ;-) > > > + > > + MediaLink *link = media_->link("rkisp1_isp", 2, resizer, 0); > > + if (!link) > > + return -ENODEV; > > + > > + ret = link->setEnabled(true); > > + if (ret < 0) > > + return ret; > > + } > > > > return 0; > > } > > @@ -1021,12 +1032,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. > > -- > Regards, > > Laurent Pinchart
diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp index a0e36a44d8d91260..38382a6894dac22a 100644 --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp @@ -211,7 +211,8 @@ private: friend RkISP1CameraData; friend RkISP1Frames; - int initLinks(); + int initLinks(const Camera *camera, + const RkISP1CameraConfiguration &config); int createCamera(MediaEntity *sensor); void tryCompleteRequest(Request *request); void bufferReady(FrameBuffer *buffer); @@ -601,28 +602,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, *config); + if (ret) + return ret; /* * Configure the format on the sensor output and propagate it through @@ -924,22 +906,51 @@ int PipelineHandlerRkISP1::queueRequestDevice(Camera *camera, Request *request) * Match and Setup */ -int PipelineHandlerRkISP1::initLinks() +int PipelineHandlerRkISP1::initLinks(const Camera *camera, + const RkISP1CameraConfiguration &config) { - MediaLink *link; + const RkISP1CameraData *data = cameraData(camera); + const CameraSensor *sensor = data->sensor_; 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) { + std::string resizer; + if (cfg.stream() == &data->stream_) + resizer = "rkisp1_resizer_mainpath"; + else + return -EINVAL; + + MediaLink *link = media_->link("rkisp1_isp", 2, resizer, 0); + if (!link) + return -ENODEV; + + ret = link->setEnabled(true); + if (ret < 0) + return ret; + } return 0; } @@ -1021,12 +1032,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.
In preparation of supporting both the main and self path move all link setup to be part of the configuration step. The main path is still the only possible path. Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> --- src/libcamera/pipeline/rkisp1/rkisp1.cpp | 79 +++++++++++++----------- 1 file changed, 42 insertions(+), 37 deletions(-)