All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] media: camss: Allocate power domain resources dynamically
@ 2022-05-12  8:23 Vladimir Zapolskiy
  2022-05-12  9:49 ` Robert Foss
       [not found] ` <CGME20220617084418eucas1p192dfca043bd0e0f7f335946f9e95e658@eucas1p1.samsung.com>
  0 siblings, 2 replies; 3+ messages in thread
From: Vladimir Zapolskiy @ 2022-05-12  8:23 UTC (permalink / raw)
  To: Robert Foss, Todor Tomov
  Cc: Andy Gross, Bjorn Andersson, Mauro Carvalho Chehab, linux-media,
	linux-arm-msm

To simplify the driver's maintenance it makes sense to escape from
hardcoded numbers of power domain resources per platform and statical
allocation of the resources. For instance on a QCOM SM8450 platform
the number of CAMSS power domains shall be bumped up to 6, also notably
CAMSS on MSM8916 has only one power domain, however it expects to get 2,
and thus it should result in a runtime error on driver probe.

The change fixes an issue mentioned above and gives more flexibility
to support more platforms in future.

Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
---
 drivers/media/platform/qcom/camss/camss.c | 38 +++++++++++++----------
 drivers/media/platform/qcom/camss/camss.h |  7 ++---
 2 files changed, 24 insertions(+), 21 deletions(-)

diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
index 79ad82e233cb..32d72b4f947b 100644
--- a/drivers/media/platform/qcom/camss/camss.c
+++ b/drivers/media/platform/qcom/camss/camss.c
@@ -1452,19 +1452,31 @@ static const struct media_device_ops camss_media_ops = {
 
 static int camss_configure_pd(struct camss *camss)
 {
-	int nbr_pm_domains = 0;
+	struct device *dev = camss->dev;
 	int last_pm_domain = 0;
 	int i;
 	int ret;
 
-	if (camss->version == CAMSS_8x96 ||
-	    camss->version == CAMSS_660)
-		nbr_pm_domains = PM_DOMAIN_GEN1_COUNT;
-	else if (camss->version == CAMSS_845 ||
-		 camss->version == CAMSS_8250)
-		nbr_pm_domains = PM_DOMAIN_GEN2_COUNT;
+	camss->genpd_num = of_count_phandle_with_args(dev->of_node,
+						      "power-domains",
+						      "#power-domain-cells");
+	if (camss->genpd_num < 0) {
+		dev_err(dev, "Power domains are not defined for camss\n");
+		return camss->genpd_num;
+	}
+
+	camss->genpd = devm_kmalloc_array(dev, camss->genpd_num,
+					  sizeof(*camss->genpd), GFP_KERNEL);
+	if (!camss->genpd)
+		return -ENOMEM;
 
-	for (i = 0; i < nbr_pm_domains; i++) {
+	camss->genpd_link = devm_kmalloc_array(dev, camss->genpd_num,
+					       sizeof(*camss->genpd_link),
+					       GFP_KERNEL);
+	if (!camss->genpd_link)
+		return -ENOMEM;
+
+	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]);
@@ -1689,7 +1701,6 @@ 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);
@@ -1698,14 +1709,7 @@ void camss_delete(struct camss *camss)
 
 	pm_runtime_disable(camss->dev);
 
-	if (camss->version == CAMSS_8x96 ||
-	    camss->version == CAMSS_660)
-		nbr_pm_domains = PM_DOMAIN_GEN1_COUNT;
-	else if (camss->version == CAMSS_845 ||
-		 camss->version == CAMSS_8250)
-		nbr_pm_domains = PM_DOMAIN_GEN2_COUNT;
-
-	for (i = 0; i < nbr_pm_domains; i++) {
+	for (i = 0; i < camss->genpd_num; i++) {
 		device_link_del(camss->genpd_link[i]);
 		dev_pm_domain_detach(camss->genpd[i], true);
 	}
diff --git a/drivers/media/platform/qcom/camss/camss.h b/drivers/media/platform/qcom/camss/camss.h
index c9b3e0df5be8..0db80cadbbaa 100644
--- a/drivers/media/platform/qcom/camss/camss.h
+++ b/drivers/media/platform/qcom/camss/camss.h
@@ -69,9 +69,7 @@ struct resources_icc {
 enum pm_domain {
 	PM_DOMAIN_VFE0 = 0,
 	PM_DOMAIN_VFE1 = 1,
-	PM_DOMAIN_GEN1_COUNT = 2,	/* CAMSS series of ISPs */
 	PM_DOMAIN_VFELITE = 2,		/* VFELITE / TOP GDSC */
-	PM_DOMAIN_GEN2_COUNT = 3,	/* Titan series of ISPs */
 };
 
 enum camss_version {
@@ -101,8 +99,9 @@ struct camss {
 	int vfe_num;
 	struct vfe_device *vfe;
 	atomic_t ref_count;
-	struct device *genpd[PM_DOMAIN_GEN2_COUNT];
-	struct device_link *genpd_link[PM_DOMAIN_GEN2_COUNT];
+	int genpd_num;
+	struct device **genpd;
+	struct device_link **genpd_link;
 	struct icc_path *icc_path[ICC_SM8250_COUNT];
 	struct icc_bw_tbl icc_bw_tbl[ICC_SM8250_COUNT];
 };
-- 
2.33.0


^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [PATCH] media: camss: Allocate power domain resources dynamically
  2022-05-12  8:23 [PATCH] media: camss: Allocate power domain resources dynamically Vladimir Zapolskiy
@ 2022-05-12  9:49 ` Robert Foss
       [not found] ` <CGME20220617084418eucas1p192dfca043bd0e0f7f335946f9e95e658@eucas1p1.samsung.com>
  1 sibling, 0 replies; 3+ messages in thread
From: Robert Foss @ 2022-05-12  9:49 UTC (permalink / raw)
  To: Vladimir Zapolskiy
  Cc: Todor Tomov, Andy Gross, Bjorn Andersson, Mauro Carvalho Chehab,
	linux-media, linux-arm-msm

On Thu, 12 May 2022 at 10:23, Vladimir Zapolskiy
<vladimir.zapolskiy@linaro.org> wrote:
>
> To simplify the driver's maintenance it makes sense to escape from
> hardcoded numbers of power domain resources per platform and statical
> allocation of the resources. For instance on a QCOM SM8450 platform
> the number of CAMSS power domains shall be bumped up to 6, also notably
> CAMSS on MSM8916 has only one power domain, however it expects to get 2,
> and thus it should result in a runtime error on driver probe.
>
> The change fixes an issue mentioned above and gives more flexibility
> to support more platforms in future.

This is a great idea, thanks for submitting this.

>
> Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
> ---
>  drivers/media/platform/qcom/camss/camss.c | 38 +++++++++++++----------
>  drivers/media/platform/qcom/camss/camss.h |  7 ++---
>  2 files changed, 24 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
> index 79ad82e233cb..32d72b4f947b 100644
> --- a/drivers/media/platform/qcom/camss/camss.c
> +++ b/drivers/media/platform/qcom/camss/camss.c
> @@ -1452,19 +1452,31 @@ static const struct media_device_ops camss_media_ops = {
>
>  static int camss_configure_pd(struct camss *camss)
>  {
> -       int nbr_pm_domains = 0;
> +       struct device *dev = camss->dev;
>         int last_pm_domain = 0;
>         int i;
>         int ret;
>
> -       if (camss->version == CAMSS_8x96 ||
> -           camss->version == CAMSS_660)
> -               nbr_pm_domains = PM_DOMAIN_GEN1_COUNT;
> -       else if (camss->version == CAMSS_845 ||
> -                camss->version == CAMSS_8250)
> -               nbr_pm_domains = PM_DOMAIN_GEN2_COUNT;
> +       camss->genpd_num = of_count_phandle_with_args(dev->of_node,
> +                                                     "power-domains",
> +                                                     "#power-domain-cells");
> +       if (camss->genpd_num < 0) {
> +               dev_err(dev, "Power domains are not defined for camss\n");
> +               return camss->genpd_num;
> +       }
> +
> +       camss->genpd = devm_kmalloc_array(dev, camss->genpd_num,
> +                                         sizeof(*camss->genpd), GFP_KERNEL);
> +       if (!camss->genpd)
> +               return -ENOMEM;
>
> -       for (i = 0; i < nbr_pm_domains; i++) {
> +       camss->genpd_link = devm_kmalloc_array(dev, camss->genpd_num,
> +                                              sizeof(*camss->genpd_link),
> +                                              GFP_KERNEL);
> +       if (!camss->genpd_link)
> +               return -ENOMEM;
> +
> +       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]);
> @@ -1689,7 +1701,6 @@ 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);
> @@ -1698,14 +1709,7 @@ void camss_delete(struct camss *camss)
>
>         pm_runtime_disable(camss->dev);
>
> -       if (camss->version == CAMSS_8x96 ||
> -           camss->version == CAMSS_660)
> -               nbr_pm_domains = PM_DOMAIN_GEN1_COUNT;
> -       else if (camss->version == CAMSS_845 ||
> -                camss->version == CAMSS_8250)
> -               nbr_pm_domains = PM_DOMAIN_GEN2_COUNT;
> -
> -       for (i = 0; i < nbr_pm_domains; i++) {
> +       for (i = 0; i < camss->genpd_num; i++) {
>                 device_link_del(camss->genpd_link[i]);
>                 dev_pm_domain_detach(camss->genpd[i], true);
>         }
> diff --git a/drivers/media/platform/qcom/camss/camss.h b/drivers/media/platform/qcom/camss/camss.h
> index c9b3e0df5be8..0db80cadbbaa 100644
> --- a/drivers/media/platform/qcom/camss/camss.h
> +++ b/drivers/media/platform/qcom/camss/camss.h
> @@ -69,9 +69,7 @@ struct resources_icc {
>  enum pm_domain {
>         PM_DOMAIN_VFE0 = 0,
>         PM_DOMAIN_VFE1 = 1,
> -       PM_DOMAIN_GEN1_COUNT = 2,       /* CAMSS series of ISPs */
>         PM_DOMAIN_VFELITE = 2,          /* VFELITE / TOP GDSC */
> -       PM_DOMAIN_GEN2_COUNT = 3,       /* Titan series of ISPs */
>  };
>
>  enum camss_version {
> @@ -101,8 +99,9 @@ struct camss {
>         int vfe_num;
>         struct vfe_device *vfe;
>         atomic_t ref_count;
> -       struct device *genpd[PM_DOMAIN_GEN2_COUNT];
> -       struct device_link *genpd_link[PM_DOMAIN_GEN2_COUNT];
> +       int genpd_num;
> +       struct device **genpd;
> +       struct device_link **genpd_link;
>         struct icc_path *icc_path[ICC_SM8250_COUNT];
>         struct icc_bw_tbl icc_bw_tbl[ICC_SM8250_COUNT];
>  };
> --
> 2.33.0
>

Reviewed-by: Robert Foss <robert.foss@linaro.org>

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] media: camss: Allocate power domain resources dynamically
       [not found] ` <CGME20220617084418eucas1p192dfca043bd0e0f7f335946f9e95e658@eucas1p1.samsung.com>
@ 2022-06-17  8:44   ` Marek Szyprowski
  0 siblings, 0 replies; 3+ messages in thread
From: Marek Szyprowski @ 2022-06-17  8:44 UTC (permalink / raw)
  To: Vladimir Zapolskiy, Robert Foss, Todor Tomov
  Cc: Andy Gross, Bjorn Andersson, Mauro Carvalho Chehab, linux-media,
	linux-arm-msm

On 12.05.2022 10:23, Vladimir Zapolskiy wrote:
> To simplify the driver's maintenance it makes sense to escape from
> hardcoded numbers of power domain resources per platform and statical
> allocation of the resources. For instance on a QCOM SM8450 platform
> the number of CAMSS power domains shall be bumped up to 6, also notably
> CAMSS on MSM8916 has only one power domain, however it expects to get 2,
> and thus it should result in a runtime error on driver probe.
>
> The change fixes an issue mentioned above and gives more flexibility
> to support more platforms in future.
>
> Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>

This patch landed recently in linux next-20220616 as commit f673698ceee5 
("media: camss: Allocate power domain resources dynamically"). 
Unfortunately it causes serious issues on DragonBoard 410c SBC 
(arch/arm64/boot/dts/qcom/apq8016-sbc.dts), like multiple 'Unable to 
handle kernel NULL pointer dereference at virtual address 0000000000000000'.

The problem originates in camss_configure_pd() function. Old version 
assigned 0 to camss->genpd_num on that board, while the new one sets it 
to 1 as a result of of_count_phandle_with_args(). When it is set to 1, 
it causes issues somewhere later in the code, the stack traces points to 
sysfs:

Unable to handle kernel NULL pointer dereference at virtual address 
0000000000000000
Mem abort info:
   ESR = 0x0000000096000006
   EC = 0x25: DABT (current EL), IL = 32 bits
   SET = 0, FnV = 0
   EA = 0, S1PTW = 0
   FSC = 0x06: level 2 translation fault
Data abort info:
   ISV = 0, ISS = 0x00000006
   CM = 0, WnR = 0
user pgtable: 4k pages, 48-bit VAs, pgdp=0000000080fba000
[0000000000000000] pgd=0800000080fd7003, p4d=0800000080fd7003, 
pud=0800000080fdb003, pmd=0000000000000000
Internal error: Oops: 96000006 [#2] PREEMPT SMP
Modules linked in: crct10dif_ce asix(+) qcom_wcnss_pil socinfo adv7511 
snd_soc_msm8916_digital snd_soc_lpass_apq8016 snd_soc_lpass_cpu 
snd_soc_lpass_platform snd_soc_apq8016_sbc snd_soc_qcom_common qrtr 
qcom_q6v5_mss qcom_pil_info qcom_q6v5 qcom_sysmon qcom_common 
qcom_glink_smem qmi_helpers snd_soc_msm8916_analog qcom_camss 
qnoc_msm8916 icc_smd_rpm videobuf2_dma_sg v4l2_fwnode 
qcom_spmi_temp_alarm v4l2_async rtc_pm8xxx videobuf2_memops venus_core 
qcom_pon v4l2_mem2mem videobuf2_v4l2 qcom_spmi_vadc qcom_vadc_common 
videobuf2_common qcom_rng qcom_stats msm videodev i2c_qcom_cci mc 
mdt_loader gpu_sched drm_dp_aux_bus display_connector rmtfs_mem
CPU: 0 PID: 255 Comm: systemd-udevd Tainted: G      D W 
5.19.0-rc2-next-20220616 #12197
Hardware name: Qualcomm Technologies, Inc. APQ 8016 SBC (DT)
pstate: 00000005 (nzcv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
pc : sysfs_kf_seq_show+0x34/0x140
lr : sysfs_kf_seq_show+0x30/0x140
ng swap..[   23.849418] sp : ffff80000c643bc0
...
Call trace:
  sysfs_kf_seq_show+0x34/0x140
  kernfs_seq_show+0x28/0x30
  seq_read_iter+0x118/0x440
  kernfs_fop_read_iter+0x11c/0x1b0
  new_sync_read+0xd0/0x188
  vfs_read+0x134/0x1d0
  ksys_read+0x64/0xf0
  __arm64_sys_read+0x14/0x20
  invoke_syscall+0x40/0xf8
  el0_svc_common.constprop.3+0x8c/0x120
  do_el0_svc_compat+0x18/0x48
  el0_svc_compat+0x48/0xd0
  el0t_32_sync_handler+0xec/0x140
  el0t_32_sync+0x160/0x164
Code: f9401821 f9404437 97fffe37 aa0003f5 (f9400000)
---[ end trace 0000000000000000 ]---


> ---
>   drivers/media/platform/qcom/camss/camss.c | 38 +++++++++++++----------
>   drivers/media/platform/qcom/camss/camss.h |  7 ++---
>   2 files changed, 24 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
> index 79ad82e233cb..32d72b4f947b 100644
> --- a/drivers/media/platform/qcom/camss/camss.c
> +++ b/drivers/media/platform/qcom/camss/camss.c
> @@ -1452,19 +1452,31 @@ static const struct media_device_ops camss_media_ops = {
>   
>   static int camss_configure_pd(struct camss *camss)
>   {
> -	int nbr_pm_domains = 0;
> +	struct device *dev = camss->dev;
>   	int last_pm_domain = 0;
>   	int i;
>   	int ret;
>   
> -	if (camss->version == CAMSS_8x96 ||
> -	    camss->version == CAMSS_660)
> -		nbr_pm_domains = PM_DOMAIN_GEN1_COUNT;
> -	else if (camss->version == CAMSS_845 ||
> -		 camss->version == CAMSS_8250)
> -		nbr_pm_domains = PM_DOMAIN_GEN2_COUNT;
> +	camss->genpd_num = of_count_phandle_with_args(dev->of_node,
> +						      "power-domains",
> +						      "#power-domain-cells");
> +	if (camss->genpd_num < 0) {
> +		dev_err(dev, "Power domains are not defined for camss\n");
> +		return camss->genpd_num;
> +	}
> +
> +	camss->genpd = devm_kmalloc_array(dev, camss->genpd_num,
> +					  sizeof(*camss->genpd), GFP_KERNEL);
> +	if (!camss->genpd)
> +		return -ENOMEM;
>   
> -	for (i = 0; i < nbr_pm_domains; i++) {
> +	camss->genpd_link = devm_kmalloc_array(dev, camss->genpd_num,
> +					       sizeof(*camss->genpd_link),
> +					       GFP_KERNEL);
> +	if (!camss->genpd_link)
> +		return -ENOMEM;
> +
> +	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]);
> @@ -1689,7 +1701,6 @@ 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);
> @@ -1698,14 +1709,7 @@ void camss_delete(struct camss *camss)
>   
>   	pm_runtime_disable(camss->dev);
>   
> -	if (camss->version == CAMSS_8x96 ||
> -	    camss->version == CAMSS_660)
> -		nbr_pm_domains = PM_DOMAIN_GEN1_COUNT;
> -	else if (camss->version == CAMSS_845 ||
> -		 camss->version == CAMSS_8250)
> -		nbr_pm_domains = PM_DOMAIN_GEN2_COUNT;
> -
> -	for (i = 0; i < nbr_pm_domains; i++) {
> +	for (i = 0; i < camss->genpd_num; i++) {
>   		device_link_del(camss->genpd_link[i]);
>   		dev_pm_domain_detach(camss->genpd[i], true);
>   	}
> diff --git a/drivers/media/platform/qcom/camss/camss.h b/drivers/media/platform/qcom/camss/camss.h
> index c9b3e0df5be8..0db80cadbbaa 100644
> --- a/drivers/media/platform/qcom/camss/camss.h
> +++ b/drivers/media/platform/qcom/camss/camss.h
> @@ -69,9 +69,7 @@ struct resources_icc {
>   enum pm_domain {
>   	PM_DOMAIN_VFE0 = 0,
>   	PM_DOMAIN_VFE1 = 1,
> -	PM_DOMAIN_GEN1_COUNT = 2,	/* CAMSS series of ISPs */
>   	PM_DOMAIN_VFELITE = 2,		/* VFELITE / TOP GDSC */
> -	PM_DOMAIN_GEN2_COUNT = 3,	/* Titan series of ISPs */
>   };
>   
>   enum camss_version {
> @@ -101,8 +99,9 @@ struct camss {
>   	int vfe_num;
>   	struct vfe_device *vfe;
>   	atomic_t ref_count;
> -	struct device *genpd[PM_DOMAIN_GEN2_COUNT];
> -	struct device_link *genpd_link[PM_DOMAIN_GEN2_COUNT];
> +	int genpd_num;
> +	struct device **genpd;
> +	struct device_link **genpd_link;
>   	struct icc_path *icc_path[ICC_SM8250_COUNT];
>   	struct icc_bw_tbl icc_bw_tbl[ICC_SM8250_COUNT];
>   };

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland


^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2022-06-17  8:44 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-12  8:23 [PATCH] media: camss: Allocate power domain resources dynamically Vladimir Zapolskiy
2022-05-12  9:49 ` Robert Foss
     [not found] ` <CGME20220617084418eucas1p192dfca043bd0e0f7f335946f9e95e658@eucas1p1.samsung.com>
2022-06-17  8:44   ` Marek Szyprowski

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.