* [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.