diff mbox series

[v5,12/22] media: camss: Remove per VFE power domain toggling

Message ID 20210217112122.424236-13-robert.foss@linaro.org (mailing list archive)
State New, archived
Headers show
Series Add support for the SDM845 Camera Subsystem | expand

Commit Message

Robert Foss Feb. 17, 2021, 11:21 a.m. UTC 
For Titan ISPs clocks fail to re-enable during vfe_get()
after any vfe has been halted and its corresponding power
domain power has been detached.

Since all of the clocks depend on all of the PDs, per
VFE PD detaching is no option for this generation of HW.

Signed-off-by: Robert Foss <robert.foss@linaro.org>
---
 .../media/platform/qcom/camss/camss-ispif.c   | 11 ---
 drivers/media/platform/qcom/camss/camss-vfe.c |  7 --
 drivers/media/platform/qcom/camss/camss.c     | 94 +++++++++++--------
 drivers/media/platform/qcom/camss/camss.h     | 12 +--
 4 files changed, 60 insertions(+), 64 deletions(-)

Comments

Andrey Konovalov Feb. 22, 2021, 11:44 a.m. UTC | #1 
Hi Robert,

Thank you for your patch!

On 17.02.2021 14:21, Robert Foss wrote:
> For Titan ISPs clocks fail to re-enable during vfe_get()
> after any vfe has been halted and its corresponding power
> domain power has been detached.

OK.

> Since all of the clocks depend on all of the PDs, per
> VFE PD detaching is no option for this generation of HW.

But this patch removes camss_pm_domain_on/off calls from
vfe_get/put() for all the SOCs, not only for sdm845.
And this looks like a regression (higher power consumption)
for all the generation1 devices.

Is it possible to handle gen1 and gen2 hardware differently,
so that gen1 continued to use camss_pm_domain_on/off as
before?

Thanks,
Andrey

> Signed-off-by: Robert Foss <robert.foss@linaro.org>
> ---
>   .../media/platform/qcom/camss/camss-ispif.c   | 11 ---
>   drivers/media/platform/qcom/camss/camss-vfe.c |  7 --
>   drivers/media/platform/qcom/camss/camss.c     | 94 +++++++++++--------
>   drivers/media/platform/qcom/camss/camss.h     | 12 +--
>   4 files changed, 60 insertions(+), 64 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/camss/camss-ispif.c b/drivers/media/platform/qcom/camss/camss-ispif.c
> index fc121da4ab0c..b88f4025010a 100644
> --- a/drivers/media/platform/qcom/camss/camss-ispif.c
> +++ b/drivers/media/platform/qcom/camss/camss-ispif.c
> @@ -323,14 +323,6 @@ static int ispif_reset(struct ispif_device *ispif, u8 vfe_id)
>   	struct camss *camss = ispif->camss;
>   	int ret;
>   
> -	ret = camss_pm_domain_on(camss, PM_DOMAIN_VFE0);
> -	if (ret < 0)
> -		return ret;
> -
> -	ret = camss_pm_domain_on(camss, PM_DOMAIN_VFE1);
> -	if (ret < 0)
> -		return ret;
> -
>   	ret = camss_enable_clocks(ispif->nclocks_for_reset,
>   				  ispif->clock_for_reset,
>   				  camss->dev);
> @@ -343,9 +335,6 @@ static int ispif_reset(struct ispif_device *ispif, u8 vfe_id)
>   
>   	camss_disable_clocks(ispif->nclocks_for_reset, ispif->clock_for_reset);
>   
> -	camss_pm_domain_off(camss, PM_DOMAIN_VFE0);
> -	camss_pm_domain_off(camss, PM_DOMAIN_VFE1);
> -
>   	return ret;
>   }
>   
> diff --git a/drivers/media/platform/qcom/camss/camss-vfe.c b/drivers/media/platform/qcom/camss/camss-vfe.c
> index 6fafeb8a5484..ed35f9ae9067 100644
> --- a/drivers/media/platform/qcom/camss/camss-vfe.c
> +++ b/drivers/media/platform/qcom/camss/camss-vfe.c
> @@ -580,10 +580,6 @@ static int vfe_get(struct vfe_device *vfe)
>   	mutex_lock(&vfe->power_lock);
>   
>   	if (vfe->power_count == 0) {
> -		ret = camss_pm_domain_on(vfe->camss, vfe->id);
> -		if (ret < 0)
> -			goto error_pm_domain;
> -
>   		ret = pm_runtime_get_sync(vfe->camss->dev);
>   		if (ret < 0)
>   			goto error_pm_runtime_get;
> @@ -620,9 +616,7 @@ static int vfe_get(struct vfe_device *vfe)
>   
>   error_pm_runtime_get:
>   	pm_runtime_put_sync(vfe->camss->dev);
> -	camss_pm_domain_off(vfe->camss, vfe->id);
>   
> -error_pm_domain:
>   	mutex_unlock(&vfe->power_lock);
>   
>   	return ret;
> @@ -646,7 +640,6 @@ static void vfe_put(struct vfe_device *vfe)
>   		}
>   		camss_disable_clocks(vfe->nclocks, vfe->clock);
>   		pm_runtime_put_sync(vfe->camss->dev);
> -		camss_pm_domain_off(vfe->camss, vfe->id);
>   	}
>   
>   	vfe->power_count--;
> diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
> index 5d0479b5589c..3c45537b2cfb 100644
> --- a/drivers/media/platform/qcom/camss/camss.c
> +++ b/drivers/media/platform/qcom/camss/camss.c
> @@ -774,28 +774,6 @@ int camss_get_pixel_clock(struct media_entity *entity, u32 *pixel_clock)
>   	return 0;
>   }
>   
> -int camss_pm_domain_on(struct camss *camss, int id)
> -{
> -	if (camss->version == CAMSS_8x96 ||
> -	    camss->version == CAMSS_660) {
> -		camss->genpd_link[id] = device_link_add(camss->dev,
> -				camss->genpd[id], DL_FLAG_STATELESS |
> -				DL_FLAG_PM_RUNTIME | DL_FLAG_RPM_ACTIVE);
> -
> -		if (!camss->genpd_link[id])
> -			return -EINVAL;
> -	}
> -
> -	return 0;
> -}
> -
> -void camss_pm_domain_off(struct camss *camss, int id)
> -{
> -	if (camss->version == CAMSS_8x96 ||
> -	    camss->version == CAMSS_660)
> -		device_link_del(camss->genpd_link[id]);
> -}
> -
>   /*
>    * camss_of_parse_endpoint_node - Parse port endpoint node
>    * @dev: Device
> @@ -1207,6 +1185,48 @@ static const struct media_device_ops camss_media_ops = {
>   	.link_notify = v4l2_pipeline_link_notify,
>   };
>   
> +
> +static int camss_configure_pd(struct camss *camss)
> +{
> +	int nbr_pm_domains = 0;
> +	int last_pm_domain = 0;
> +	int i;
> +	int ret;
> +
> +	if (camss->version == CAMSS_8x96 ||
> +	    camss->version == CAMSS_660)
> +		nbr_pm_domains = PM_DOMAIN_CAMSS_COUNT;
> +
> +	for (i = 0; i < nbr_pm_domains; i++) {
> +		camss->genpd[i] = dev_pm_domain_attach_by_id(camss->dev, i);
> +		if (IS_ERR(camss->genpd[i])) {
> +			ret = PTR_ERR(camss->genpd[i]);
> +			goto fail_pm;
> +		}
> +
> +		camss->genpd_link[i] = device_link_add(camss->dev, camss->genpd[i],
> +			DL_FLAG_STATELESS | DL_FLAG_PM_RUNTIME | DL_FLAG_RPM_ACTIVE);
> +
> +		if (!camss->genpd_link[i]) {
> +			dev_pm_domain_detach(camss->genpd[i], true);
> +			ret = -EINVAL;
> +			goto fail_pm;
> +		}
> +
> +		last_pm_domain = i;
> +	}
> +
> +	return 0;
> +
> +fail_pm:
> +	for (i = 0; i < last_pm_domain; i++) {
> +		device_link_del(camss->genpd_link[i]);
> +		dev_pm_domain_detach(camss->genpd[i], true);
> +	}
> +
> +	return ret;
> +}
> +
>   /*
>    * camss_probe - Probe CAMSS platform device
>    * @pdev: Pointer to CAMSS platform device
> @@ -1339,20 +1359,10 @@ static int camss_probe(struct platform_device *pdev)
>   		}
>   	}
>   
> -	if (camss->version == CAMSS_8x96 ||
> -	    camss->version == CAMSS_660) {
> -		camss->genpd[PM_DOMAIN_VFE0] = dev_pm_domain_attach_by_id(
> -						camss->dev, PM_DOMAIN_VFE0);
> -		if (IS_ERR(camss->genpd[PM_DOMAIN_VFE0]))
> -			return PTR_ERR(camss->genpd[PM_DOMAIN_VFE0]);
> -
> -		camss->genpd[PM_DOMAIN_VFE1] = dev_pm_domain_attach_by_id(
> -						camss->dev, PM_DOMAIN_VFE1);
> -		if (IS_ERR(camss->genpd[PM_DOMAIN_VFE1])) {
> -			dev_pm_domain_detach(camss->genpd[PM_DOMAIN_VFE0],
> -					     true);
> -			return PTR_ERR(camss->genpd[PM_DOMAIN_VFE1]);
> -		}
> +	ret = camss_configure_pd(camss);
> +	if (ret < 0) {
> +		dev_err(dev, "Failed to configure power domains: %d\n", ret);
> +		return ret;
>   	}
>   
>   	pm_runtime_enable(dev);
> @@ -1373,6 +1383,9 @@ static int camss_probe(struct platform_device *pdev)
>   
>   void camss_delete(struct camss *camss)
>   {
> +	int nbr_pm_domains = 0;
> +	int i;
> +
>   	v4l2_device_unregister(&camss->v4l2_dev);
>   	media_device_unregister(&camss->media_dev);
>   	media_device_cleanup(&camss->media_dev);
> @@ -1380,9 +1393,12 @@ void camss_delete(struct camss *camss)
>   	pm_runtime_disable(camss->dev);
>   
>   	if (camss->version == CAMSS_8x96 ||
> -	    camss->version == CAMSS_660) {
> -		dev_pm_domain_detach(camss->genpd[PM_DOMAIN_VFE0], true);
> -		dev_pm_domain_detach(camss->genpd[PM_DOMAIN_VFE1], true);
> +	    camss->version == CAMSS_660)
> +		nbr_pm_domains = PM_DOMAIN_CAMSS_COUNT;
> +
> +	for (i = 0; i < nbr_pm_domains; i++) {
> +		device_link_del(camss->genpd_link[i]);
> +		dev_pm_domain_detach(camss->genpd[i], true);
>   	}
>   
>   	kfree(camss);
> diff --git a/drivers/media/platform/qcom/camss/camss.h b/drivers/media/platform/qcom/camss/camss.h
> index b7ad8e9f68a8..7560d85b3352 100644
> --- a/drivers/media/platform/qcom/camss/camss.h
> +++ b/drivers/media/platform/qcom/camss/camss.h
> @@ -57,9 +57,9 @@ struct resources_ispif {
>   };
>   
>   enum pm_domain {
> -	PM_DOMAIN_VFE0,
> -	PM_DOMAIN_VFE1,
> -	PM_DOMAIN_COUNT
> +	PM_DOMAIN_VFE0 = 0,
> +	PM_DOMAIN_VFE1 = 1,
> +	PM_DOMAIN_CAMSS_COUNT = 2,	/* CAMSS series of ISPs */
>   };
>   
>   enum camss_version {
> @@ -83,8 +83,8 @@ struct camss {
>   	int vfe_num;
>   	struct vfe_device *vfe;
>   	atomic_t ref_count;
> -	struct device *genpd[PM_DOMAIN_COUNT];
> -	struct device_link *genpd_link[PM_DOMAIN_COUNT];
> +	struct device *genpd[PM_DOMAIN_CAMSS_COUNT];
> +	struct device_link *genpd_link[PM_DOMAIN_CAMSS_COUNT];
>   };
>   
>   struct camss_camera_interface {
> @@ -110,8 +110,6 @@ int camss_enable_clocks(int nclocks, struct camss_clock *clock,
>   void camss_disable_clocks(int nclocks, struct camss_clock *clock);
>   struct media_entity *camss_find_sensor(struct media_entity *entity);
>   int camss_get_pixel_clock(struct media_entity *entity, u32 *pixel_clock);
> -int camss_pm_domain_on(struct camss *camss, int id);
> -void camss_pm_domain_off(struct camss *camss, int id);
>   void camss_delete(struct camss *camss);
>   
>   #endif /* QC_MSM_CAMSS_H */
>
Robert Foss Feb. 24, 2021, 2:53 p.m. UTC | #2 
Hey Andrey,

>
> On 17.02.2021 14:21, Robert Foss wrote:
> > For Titan ISPs clocks fail to re-enable during vfe_get()
> > after any vfe has been halted and its corresponding power
> > domain power has been detached.
>
> OK.
>
> > Since all of the clocks depend on all of the PDs, per
> > VFE PD detaching is no option for this generation of HW.
>
> But this patch removes camss_pm_domain_on/off calls from
> vfe_get/put() for all the SOCs, not only for sdm845.
> And this looks like a regression (higher power consumption)
> for all the generation1 devices.

Yeah, that is a serious problem with the approach I picked here. The
power difference shouldn't be huge however, since the best case
scenario savings of the previous implementation was being able to
power down 1 VFE when the other one is working. If none of the VFEs is
working, nothing is powered up both in the previous implementation &
using this patch.

>
> Is it possible to handle gen1 and gen2 hardware differently,
> so that gen1 continued to use camss_pm_domain_on/off as
> before?

I hesitated going down this gen1/gen2 split here, due to how deep into
the common code some of this functionality is. Let me have another
look at this though, not having a power regression for gen1 devices
would definitely be preferable.
diff mbox series

Patch

diff --git a/drivers/media/platform/qcom/camss/camss-ispif.c b/drivers/media/platform/qcom/camss/camss-ispif.c
index fc121da4ab0c..b88f4025010a 100644
--- a/drivers/media/platform/qcom/camss/camss-ispif.c
+++ b/drivers/media/platform/qcom/camss/camss-ispif.c
@@ -323,14 +323,6 @@  static int ispif_reset(struct ispif_device *ispif, u8 vfe_id)
 	struct camss *camss = ispif->camss;
 	int ret;
 
-	ret = camss_pm_domain_on(camss, PM_DOMAIN_VFE0);
-	if (ret < 0)
-		return ret;
-
-	ret = camss_pm_domain_on(camss, PM_DOMAIN_VFE1);
-	if (ret < 0)
-		return ret;
-
 	ret = camss_enable_clocks(ispif->nclocks_for_reset,
 				  ispif->clock_for_reset,
 				  camss->dev);
@@ -343,9 +335,6 @@  static int ispif_reset(struct ispif_device *ispif, u8 vfe_id)
 
 	camss_disable_clocks(ispif->nclocks_for_reset, ispif->clock_for_reset);
 
-	camss_pm_domain_off(camss, PM_DOMAIN_VFE0);
-	camss_pm_domain_off(camss, PM_DOMAIN_VFE1);
-
 	return ret;
 }
 
diff --git a/drivers/media/platform/qcom/camss/camss-vfe.c b/drivers/media/platform/qcom/camss/camss-vfe.c
index 6fafeb8a5484..ed35f9ae9067 100644
--- a/drivers/media/platform/qcom/camss/camss-vfe.c
+++ b/drivers/media/platform/qcom/camss/camss-vfe.c
@@ -580,10 +580,6 @@  static int vfe_get(struct vfe_device *vfe)
 	mutex_lock(&vfe->power_lock);
 
 	if (vfe->power_count == 0) {
-		ret = camss_pm_domain_on(vfe->camss, vfe->id);
-		if (ret < 0)
-			goto error_pm_domain;
-
 		ret = pm_runtime_get_sync(vfe->camss->dev);
 		if (ret < 0)
 			goto error_pm_runtime_get;
@@ -620,9 +616,7 @@  static int vfe_get(struct vfe_device *vfe)
 
 error_pm_runtime_get:
 	pm_runtime_put_sync(vfe->camss->dev);
-	camss_pm_domain_off(vfe->camss, vfe->id);
 
-error_pm_domain:
 	mutex_unlock(&vfe->power_lock);
 
 	return ret;
@@ -646,7 +640,6 @@  static void vfe_put(struct vfe_device *vfe)
 		}
 		camss_disable_clocks(vfe->nclocks, vfe->clock);
 		pm_runtime_put_sync(vfe->camss->dev);
-		camss_pm_domain_off(vfe->camss, vfe->id);
 	}
 
 	vfe->power_count--;
diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
index 5d0479b5589c..3c45537b2cfb 100644
--- a/drivers/media/platform/qcom/camss/camss.c
+++ b/drivers/media/platform/qcom/camss/camss.c
@@ -774,28 +774,6 @@  int camss_get_pixel_clock(struct media_entity *entity, u32 *pixel_clock)
 	return 0;
 }
 
-int camss_pm_domain_on(struct camss *camss, int id)
-{
-	if (camss->version == CAMSS_8x96 ||
-	    camss->version == CAMSS_660) {
-		camss->genpd_link[id] = device_link_add(camss->dev,
-				camss->genpd[id], DL_FLAG_STATELESS |
-				DL_FLAG_PM_RUNTIME | DL_FLAG_RPM_ACTIVE);
-
-		if (!camss->genpd_link[id])
-			return -EINVAL;
-	}
-
-	return 0;
-}
-
-void camss_pm_domain_off(struct camss *camss, int id)
-{
-	if (camss->version == CAMSS_8x96 ||
-	    camss->version == CAMSS_660)
-		device_link_del(camss->genpd_link[id]);
-}
-
 /*
  * camss_of_parse_endpoint_node - Parse port endpoint node
  * @dev: Device
@@ -1207,6 +1185,48 @@  static const struct media_device_ops camss_media_ops = {
 	.link_notify = v4l2_pipeline_link_notify,
 };
 
+
+static int camss_configure_pd(struct camss *camss)
+{
+	int nbr_pm_domains = 0;
+	int last_pm_domain = 0;
+	int i;
+	int ret;
+
+	if (camss->version == CAMSS_8x96 ||
+	    camss->version == CAMSS_660)
+		nbr_pm_domains = PM_DOMAIN_CAMSS_COUNT;
+
+	for (i = 0; i < nbr_pm_domains; i++) {
+		camss->genpd[i] = dev_pm_domain_attach_by_id(camss->dev, i);
+		if (IS_ERR(camss->genpd[i])) {
+			ret = PTR_ERR(camss->genpd[i]);
+			goto fail_pm;
+		}
+
+		camss->genpd_link[i] = device_link_add(camss->dev, camss->genpd[i],
+			DL_FLAG_STATELESS | DL_FLAG_PM_RUNTIME | DL_FLAG_RPM_ACTIVE);
+
+		if (!camss->genpd_link[i]) {
+			dev_pm_domain_detach(camss->genpd[i], true);
+			ret = -EINVAL;
+			goto fail_pm;
+		}
+
+		last_pm_domain = i;
+	}
+
+	return 0;
+
+fail_pm:
+	for (i = 0; i < last_pm_domain; i++) {
+		device_link_del(camss->genpd_link[i]);
+		dev_pm_domain_detach(camss->genpd[i], true);
+	}
+
+	return ret;
+}
+
 /*
  * camss_probe - Probe CAMSS platform device
  * @pdev: Pointer to CAMSS platform device
@@ -1339,20 +1359,10 @@  static int camss_probe(struct platform_device *pdev)
 		}
 	}
 
-	if (camss->version == CAMSS_8x96 ||
-	    camss->version == CAMSS_660) {
-		camss->genpd[PM_DOMAIN_VFE0] = dev_pm_domain_attach_by_id(
-						camss->dev, PM_DOMAIN_VFE0);
-		if (IS_ERR(camss->genpd[PM_DOMAIN_VFE0]))
-			return PTR_ERR(camss->genpd[PM_DOMAIN_VFE0]);
-
-		camss->genpd[PM_DOMAIN_VFE1] = dev_pm_domain_attach_by_id(
-						camss->dev, PM_DOMAIN_VFE1);
-		if (IS_ERR(camss->genpd[PM_DOMAIN_VFE1])) {
-			dev_pm_domain_detach(camss->genpd[PM_DOMAIN_VFE0],
-					     true);
-			return PTR_ERR(camss->genpd[PM_DOMAIN_VFE1]);
-		}
+	ret = camss_configure_pd(camss);
+	if (ret < 0) {
+		dev_err(dev, "Failed to configure power domains: %d\n", ret);
+		return ret;
 	}
 
 	pm_runtime_enable(dev);
@@ -1373,6 +1383,9 @@  static int camss_probe(struct platform_device *pdev)
 
 void camss_delete(struct camss *camss)
 {
+	int nbr_pm_domains = 0;
+	int i;
+
 	v4l2_device_unregister(&camss->v4l2_dev);
 	media_device_unregister(&camss->media_dev);
 	media_device_cleanup(&camss->media_dev);
@@ -1380,9 +1393,12 @@  void camss_delete(struct camss *camss)
 	pm_runtime_disable(camss->dev);
 
 	if (camss->version == CAMSS_8x96 ||
-	    camss->version == CAMSS_660) {
-		dev_pm_domain_detach(camss->genpd[PM_DOMAIN_VFE0], true);
-		dev_pm_domain_detach(camss->genpd[PM_DOMAIN_VFE1], true);
+	    camss->version == CAMSS_660)
+		nbr_pm_domains = PM_DOMAIN_CAMSS_COUNT;
+
+	for (i = 0; i < nbr_pm_domains; i++) {
+		device_link_del(camss->genpd_link[i]);
+		dev_pm_domain_detach(camss->genpd[i], true);
 	}
 
 	kfree(camss);
diff --git a/drivers/media/platform/qcom/camss/camss.h b/drivers/media/platform/qcom/camss/camss.h
index b7ad8e9f68a8..7560d85b3352 100644
--- a/drivers/media/platform/qcom/camss/camss.h
+++ b/drivers/media/platform/qcom/camss/camss.h
@@ -57,9 +57,9 @@  struct resources_ispif {
 };
 
 enum pm_domain {
-	PM_DOMAIN_VFE0,
-	PM_DOMAIN_VFE1,
-	PM_DOMAIN_COUNT
+	PM_DOMAIN_VFE0 = 0,
+	PM_DOMAIN_VFE1 = 1,
+	PM_DOMAIN_CAMSS_COUNT = 2,	/* CAMSS series of ISPs */
 };
 
 enum camss_version {
@@ -83,8 +83,8 @@  struct camss {
 	int vfe_num;
 	struct vfe_device *vfe;
 	atomic_t ref_count;
-	struct device *genpd[PM_DOMAIN_COUNT];
-	struct device_link *genpd_link[PM_DOMAIN_COUNT];
+	struct device *genpd[PM_DOMAIN_CAMSS_COUNT];
+	struct device_link *genpd_link[PM_DOMAIN_CAMSS_COUNT];
 };
 
 struct camss_camera_interface {
@@ -110,8 +110,6 @@  int camss_enable_clocks(int nclocks, struct camss_clock *clock,
 void camss_disable_clocks(int nclocks, struct camss_clock *clock);
 struct media_entity *camss_find_sensor(struct media_entity *entity);
 int camss_get_pixel_clock(struct media_entity *entity, u32 *pixel_clock);
-int camss_pm_domain_on(struct camss *camss, int id);
-void camss_pm_domain_off(struct camss *camss, int id);
 void camss_delete(struct camss *camss);
 
 #endif /* QC_MSM_CAMSS_H */