| Message ID | 20220512082318.189398-1-vladimir.zapolskiy@linaro.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | media: camss: Allocate power domain resources dynamically | expand |
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>
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
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]; };
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(-)