Moving the location of the hooks to VFE power domains has several
advantages.
1. Separation of concerns and functional decomposition.
vfe.c should be responsible for and know best how manage
power-domains for a VFE, excising from camss.c follows this
principle.
2. Embedding a pointer to genpd in struct camss_vfe{} meas that we can
dispense with a bunch of kmalloc array inside of camss.c.
3. Splitting up titan top gdsc from vfe/ife gdsc provides a base for
breaking up magic indexes in dtsi.
Suggested-by: Matti Lehtimäki <[email protected]>
Tested-by: Matti Lehtimäki <[email protected]>
Signed-off-by: Bryan O'Donoghue <[email protected]>
---
drivers/media/platform/qcom/camss/camss-vfe.c | 24 +++++++++-
drivers/media/platform/qcom/camss/camss-vfe.h | 2 +
drivers/media/platform/qcom/camss/camss.c | 68 +++++++++++++++------------
drivers/media/platform/qcom/camss/camss.h | 4 +-
4 files changed, 63 insertions(+), 35 deletions(-)
diff --git a/drivers/media/platform/qcom/camss/camss-vfe.c b/drivers/media/platform/qcom/camss/camss-vfe.c
index 5172eb5612a1c..defff24f07ce3 100644
--- a/drivers/media/platform/qcom/camss/camss-vfe.c
+++ b/drivers/media/platform/qcom/camss/camss-vfe.c
@@ -14,6 +14,7 @@
#include <linux/mutex.h>
#include <linux/of.h>
#include <linux/platform_device.h>
+#include <linux/pm_domain.h>
#include <linux/pm_runtime.h>
#include <linux/spinlock_types.h>
#include <linux/spinlock.h>
@@ -1381,8 +1382,13 @@ int msm_vfe_subdev_init(struct camss *camss, struct vfe_device *vfe,
if (!res->line_num)
return -EINVAL;
- if (res->has_pd)
- vfe->genpd = camss->genpd[id];
+ if (res->has_pd) {
+ vfe->genpd = dev_pm_domain_attach_by_id(camss->dev, id);
+ if (IS_ERR(vfe->genpd)) {
+ ret = PTR_ERR(vfe->genpd);
+ return ret;
+ }
+ }
vfe->line_num = res->line_num;
vfe->ops->subdev_init(dev, vfe);
@@ -1506,6 +1512,20 @@ int msm_vfe_subdev_init(struct camss *camss, struct vfe_device *vfe,
return 0;
}
+/*
+ * msm_vfe_genpd_cleanup - Cleanup VFE genpd linkages
+ * @vfe: VFE device
+ *
+ */
+void msm_vfe_genpd_cleanup(struct vfe_device *vfe)
+{
+ if (vfe->genpd_link)
+ device_link_del(vfe->genpd_link);
+
+ if (vfe->genpd)
+ dev_pm_domain_detach(vfe->genpd, true);
+}
+
/*
* vfe_link_setup - Setup VFE connections
* @entity: Pointer to media entity structure
diff --git a/drivers/media/platform/qcom/camss/camss-vfe.h b/drivers/media/platform/qcom/camss/camss-vfe.h
index 992a2103ec44c..cdbe59d8d437e 100644
--- a/drivers/media/platform/qcom/camss/camss-vfe.h
+++ b/drivers/media/platform/qcom/camss/camss-vfe.h
@@ -159,6 +159,8 @@ struct camss_subdev_resources;
int msm_vfe_subdev_init(struct camss *camss, struct vfe_device *vfe,
const struct camss_subdev_resources *res, u8 id);
+void msm_vfe_genpd_cleanup(struct vfe_device *vfe);
+
int msm_vfe_register_entities(struct vfe_device *vfe,
struct v4l2_device *v4l2_dev);
diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
index ed01a3ac7a38e..f10a47f1aee7d 100644
--- a/drivers/media/platform/qcom/camss/camss.c
+++ b/drivers/media/platform/qcom/camss/camss.c
@@ -1487,7 +1487,9 @@ static const struct media_device_ops camss_media_ops = {
static int camss_configure_pd(struct camss *camss)
{
struct device *dev = camss->dev;
+ const struct camss_resources *res = camss->res;
int i;
+ int vfepd_num;
int ret;
camss->genpd_num = of_count_phandle_with_args(dev->of_node,
@@ -1506,45 +1508,42 @@ static int camss_configure_pd(struct camss *camss)
if (camss->genpd_num == 1)
return 0;
- camss->genpd = devm_kmalloc_array(dev, camss->genpd_num,
- sizeof(*camss->genpd), GFP_KERNEL);
- if (!camss->genpd)
- return -ENOMEM;
+ /* count the # of VFEs which have flagged power-domain */
+ for (vfepd_num = i = 0; i < camss->vfe_total_num; i++) {
+ if (res->vfe_res[i].has_pd)
+ vfepd_num++;
+ }
- camss->genpd_link = devm_kmalloc_array(dev, camss->genpd_num,
- sizeof(*camss->genpd_link),
- GFP_KERNEL);
- if (!camss->genpd_link)
- return -ENOMEM;
+ /*
+ * If the number of power-domains is greater than the number of VFEs
+ * then the additional power-domain is for the entire CAMSS block the
+ * 'top' power-domain.
+ */
+ if (camss->genpd_num <= vfepd_num)
+ return 0;
/*
* VFE power domains are in the beginning of the list, and while all
* power domains should be attached, only if TITAN_TOP power domain is
* found in the list, it should be linked over here.
*/
- for (i = 0; i < camss->genpd_num; 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 = dev_pm_domain_attach_by_id(camss->dev, camss->genpd_num - 1);
+ if (IS_ERR(camss->genpd)) {
+ ret = PTR_ERR(camss->genpd);
+ goto fail_pm;
}
-
- if (i > camss->res->vfe_num) {
- camss->genpd_link[i - 1] = device_link_add(camss->dev, camss->genpd[i - 1],
- DL_FLAG_STATELESS | DL_FLAG_PM_RUNTIME |
- DL_FLAG_RPM_ACTIVE);
- if (!camss->genpd_link[i - 1]) {
- ret = -EINVAL;
- goto fail_pm;
- }
+ camss->genpd_link = device_link_add(camss->dev, camss->genpd,
+ DL_FLAG_STATELESS | DL_FLAG_PM_RUNTIME |
+ DL_FLAG_RPM_ACTIVE);
+ if (!camss->genpd_link) {
+ ret = -EINVAL;
+ goto fail_pm;
}
return 0;
fail_pm:
- for (--i ; i >= 0; i--)
- dev_pm_domain_detach(camss->genpd[i], true);
+ dev_pm_domain_detach(camss->genpd, true);
return ret;
}
@@ -1566,18 +1565,25 @@ static int camss_icc_get(struct camss *camss)
return 0;
}
-static void camss_genpd_cleanup(struct camss *camss)
+static void camss_genpd_subdevice_cleanup(struct camss *camss)
{
int i;
+ for (i = 0; i < camss->vfe_total_num; i++)
+ msm_vfe_genpd_cleanup(&camss->vfe[i]);
+}
+
+static void camss_genpd_cleanup(struct camss *camss)
+{
if (camss->genpd_num == 1)
return;
- if (camss->genpd_num > camss->res->vfe_num)
- device_link_del(camss->genpd_link[camss->genpd_num - 1]);
+ if (camss->genpd_link)
+ device_link_del(camss->genpd_link);
+
+ dev_pm_domain_detach(camss->genpd, true);
- for (i = 0; i < camss->genpd_num; i++)
- dev_pm_domain_detach(camss->genpd[i], true);
+ camss_genpd_subdevice_cleanup(camss);
}
/*
diff --git a/drivers/media/platform/qcom/camss/camss.h b/drivers/media/platform/qcom/camss/camss.h
index b854cff1774d4..1ba824a2cb76c 100644
--- a/drivers/media/platform/qcom/camss/camss.h
+++ b/drivers/media/platform/qcom/camss/camss.h
@@ -107,8 +107,8 @@ struct camss {
struct vfe_device *vfe;
atomic_t ref_count;
int genpd_num;
- struct device **genpd;
- struct device_link **genpd_link;
+ struct device *genpd;
+ struct device_link *genpd_link;
struct icc_path *icc_path[ICC_SM8250_COUNT];
const struct camss_resources *res;
unsigned int vfe_total_num;
--
2.42.0
On 3.11.2023 13:29, Bryan O'Donoghue wrote:
> Moving the location of the hooks to VFE power domains has several
> advantages.
>
> 1. Separation of concerns and functional decomposition.
> vfe.c should be responsible for and know best how manage
> power-domains for a VFE, excising from camss.c follows this
> principle.
>
> 2. Embedding a pointer to genpd in struct camss_vfe{} meas that we can
> dispense with a bunch of kmalloc array inside of camss.c.
>
> 3. Splitting up titan top gdsc from vfe/ife gdsc provides a base for
> breaking up magic indexes in dtsi.
>
> Suggested-by: Matti Lehtimäki <[email protected]>
> Tested-by: Matti Lehtimäki <[email protected]>
> Signed-off-by: Bryan O'Donoghue <[email protected]>
> ---
[...]
> +/*
> + * msm_vfe_genpd_cleanup - Cleanup VFE genpd linkages
> + * @vfe: VFE device
You can even give this an upgrade to kerneldoc! :)
[...]
> + /* count the # of VFEs which have flagged power-domain */
[...]
Personal peeve, but this comment seems a bit excessive
> + for (vfepd_num = i = 0; i < camss->vfe_total_num; i++) {
> + if (res->vfe_res[i].has_pd)
> + vfepd_num++;
> + }
>
> - camss->genpd_link = devm_kmalloc_array(dev, camss->genpd_num,
> - sizeof(*camss->genpd_link),
> - GFP_KERNEL);
> - if (!camss->genpd_link)
> - return -ENOMEM;
> + /*
> + * If the number of power-domains is greater than the number of VFEs
> + * then the additional power-domain is for the entire CAMSS block the
> + * 'top' power-domain.
the last 3 words seem out of place
> + */
> + if (camss->genpd_num <= vfepd_num)
> + return 0;
if (!(camss->genpd_num > vfepd_num))
would probably be easier to follow given your comment above
Konrad
On 18/11/2023 00:32, Konrad Dybcio wrote:
>> + /* count the # of VFEs which have flagged power-domain */
> [...]
>
> Personal peeve, but this comment seems a bit excessive
Well, to me this code "does stuff" that isn't terribly obvious. It took
a while to understand the Gordian knot so its worthwhile documenting it
inline until we can use named pds to make it all go away forever.
>
>> + for (vfepd_num = i = 0; i < camss->vfe_total_num; i++) {
>> + if (res->vfe_res[i].has_pd)
>> + vfepd_num++;
>> + }
>>
>> - camss->genpd_link = devm_kmalloc_array(dev, camss->genpd_num,
>> - sizeof(*camss->genpd_link),
>> - GFP_KERNEL);
>> - if (!camss->genpd_link)
>> - return -ENOMEM;
>> + /*
>> + * If the number of power-domains is greater than the number of VFEs
>> + * then the additional power-domain is for the entire CAMSS block the
>> + * 'top' power-domain.
> the last 3 words seem out of place
>
>> + */
>> + if (camss->genpd_num <= vfepd_num)
>> + return 0;
> if (!(camss->genpd_num > vfepd_num))
>
> would probably be easier to follow given your comment above
Sure, if its easier to read/understand for you, then it is probably so
for others.
I will V5 this.
---
bod