| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 25 Jun 2024 23:29:26 +0530
From: Akhil P Oommen <quic_akhilpo@...cinc.com>
To: Rob Clark <robdclark@...il.com>
CC: Kiarash Hajian <kiarash8112hajian@...il.com>,
Abhinav Kumar
<quic_abhinavk@...cinc.com>,
Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
Sean Paul <sean@...rly.run>,
Marijn Suijten <marijn.suijten@...ainline.org>,
David Airlie <airlied@...il.com>, Daniel Vetter <daniel@...ll.ch>,
<linux-arm-msm@...r.kernel.org>, <dri-devel@...ts.freedesktop.org>,
<freedreno@...ts.freedesktop.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] drm/msm/a6xx: request memory region
On Fri, Jun 21, 2024 at 02:09:58PM -0700, Rob Clark wrote:
> On Sat, Jun 8, 2024 at 8:44 AM Kiarash Hajian
> <kiarash8112hajian@...il.com> wrote:
> >
> > The driver's memory regions are currently just ioremap()ed, but not
> > reserved through a request. That's not a bug, but having the request is
> > a little more robust.
> >
> > Implement the region-request through the corresponding managed
> > devres-function.
> >
> > Signed-off-by: Kiarash Hajian <kiarash8112hajian@...il.com>
> > ---
> > Changes in v6:
> > -Fix compile error
> > -Link to v5: https://lore.kernel.org/all/20240607-memory-v1-1-8664f52fc2a1@gmail.com
> >
> > Changes in v5:
> > - Fix error hanlding problems.
> > - Link to v4: https://lore.kernel.org/r/20240512-msm-adreno-memory-region-v4-1-3881a64088e6@gmail.com
> >
> > Changes in v4:
> > - Combine v3 commits into a singel commit
> > - Link to v3: https://lore.kernel.org/r/20240512-msm-adreno-memory-region-v3-0-0a728ad45010@gmail.com
> >
> > Changes in v3:
> > - Remove redundant devm_iounmap calls, relying on devres for automatic resource cleanup.
> >
> > Changes in v2:
> > - update the subject prefix to "drm/msm/a6xx:", to match the majority of other changes to this file.
> > ---
> > drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 33 +++++++++++----------------------
> > 1 file changed, 11 insertions(+), 22 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> > index 8bea8ef26f77..d26cc6254ef9 100644
> > --- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> > +++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> > @@ -525,7 +525,7 @@ static void a6xx_gmu_rpmh_init(struct a6xx_gmu *gmu)
> > bool pdc_in_aop = false;
> >
> > if (IS_ERR(pdcptr))
> > - goto err;
> > + return;
> >
> > if (adreno_is_a650(adreno_gpu) ||
> > adreno_is_a660_family(adreno_gpu) ||
> > @@ -541,7 +541,7 @@ static void a6xx_gmu_rpmh_init(struct a6xx_gmu *gmu)
> > if (!pdc_in_aop) {
> > seqptr = a6xx_gmu_get_mmio(pdev, "gmu_pdc_seq");
> > if (IS_ERR(seqptr))
> > - goto err;
> > + return;
> > }
> >
> > /* Disable SDE clock gating */
> > @@ -633,12 +633,6 @@ static void a6xx_gmu_rpmh_init(struct a6xx_gmu *gmu)
> > wmb();
> >
> > a6xx_rpmh_stop(gmu);
> > -
> > -err:
> > - if (!IS_ERR_OR_NULL(pdcptr))
> > - iounmap(pdcptr);
> > - if (!IS_ERR_OR_NULL(seqptr))
> > - iounmap(seqptr);
> > }
> >
> > /*
> > @@ -1503,7 +1497,7 @@ static void __iomem *a6xx_gmu_get_mmio(struct platform_device *pdev,
> > return ERR_PTR(-EINVAL);
> > }
> >
> > - ret = ioremap(res->start, resource_size(res));
> > + ret = devm_ioremap_resource(&pdev->dev, res);
>
> So, this doesn't actually work, failing in __request_region_locked(),
> because the gmu region partially overlaps with the gpucc region (which
> is busy). I think this is intentional, since gmu is controlling the
> gpu clocks, etc. In particular REG_A6XX_GPU_CC_GX_GDSCR is in this
> overlapping region. Maybe Akhil knows more about GMU.
We don't really need to map gpucc region from driver on behalf of gmu.
Since we don't access any gpucc register from drm-msm driver, we can
update the range size to correct this. But due to backward compatibility
requirement with older dt, can we still enable region locking? I prefer
it if that is possible.
FYI, kgsl accesses gpucc registers to ensure gdsc has collapsed. So
gpucc region has to be mapped by kgsl and that is reflected in the kgsl
device tree.
-Akhil
>
> BR,
> -R
>
> > if (!ret) {
> > DRM_DEV_ERROR(&pdev->dev, "Unable to map the %s registers\n", name);
> > return ERR_PTR(-EINVAL);
> > @@ -1613,13 +1607,13 @@ int a6xx_gmu_wrapper_init(struct a6xx_gpu *a6xx_gpu, struct device_node *node)
> > gmu->mmio = a6xx_gmu_get_mmio(pdev, "gmu");
> > if (IS_ERR(gmu->mmio)) {
> > ret = PTR_ERR(gmu->mmio);
> > - goto err_mmio;
> > + goto err_cleanup;
> > }
> >
> > gmu->cxpd = dev_pm_domain_attach_by_name(gmu->dev, "cx");
> > if (IS_ERR(gmu->cxpd)) {
> > ret = PTR_ERR(gmu->cxpd);
> > - goto err_mmio;
> > + goto err_cleanup;
> > }
> >
> > if (!device_link_add(gmu->dev, gmu->cxpd, DL_FLAG_PM_RUNTIME)) {
> > @@ -1635,7 +1629,7 @@ int a6xx_gmu_wrapper_init(struct a6xx_gpu *a6xx_gpu, struct device_node *node)
> > gmu->gxpd = dev_pm_domain_attach_by_name(gmu->dev, "gx");
> > if (IS_ERR(gmu->gxpd)) {
> > ret = PTR_ERR(gmu->gxpd);
> > - goto err_mmio;
> > + goto err_cleanup;
> > }
> >
> > gmu->initialized = true;
> > @@ -1645,9 +1639,7 @@ int a6xx_gmu_wrapper_init(struct a6xx_gpu *a6xx_gpu, struct device_node *node)
> > detach_cxpd:
> > dev_pm_domain_detach(gmu->cxpd, false);
> >
> > -err_mmio:
> > - iounmap(gmu->mmio);
> > -
> > +err_cleanup:
> > /* Drop reference taken in of_find_device_by_node */
> > put_device(gmu->dev);
> >
> > @@ -1762,7 +1754,7 @@ int a6xx_gmu_init(struct a6xx_gpu *a6xx_gpu, struct device_node *node)
> > gmu->rscc = a6xx_gmu_get_mmio(pdev, "rscc");
> > if (IS_ERR(gmu->rscc)) {
> > ret = -ENODEV;
> > - goto err_mmio;
> > + goto err_cleanup;
> > }
> > } else {
> > gmu->rscc = gmu->mmio + 0x23000;
> > @@ -1774,13 +1766,13 @@ int a6xx_gmu_init(struct a6xx_gpu *a6xx_gpu, struct device_node *node)
> >
> > if (gmu->hfi_irq < 0 || gmu->gmu_irq < 0) {
> > ret = -ENODEV;
> > - goto err_mmio;
> > + goto err_cleanup;
> > }
> >
> > gmu->cxpd = dev_pm_domain_attach_by_name(gmu->dev, "cx");
> > if (IS_ERR(gmu->cxpd)) {
> > ret = PTR_ERR(gmu->cxpd);
> > - goto err_mmio;
> > + goto err_cleanup;
> > }
> >
> > link = device_link_add(gmu->dev, gmu->cxpd, DL_FLAG_PM_RUNTIME);
> > @@ -1824,10 +1816,7 @@ int a6xx_gmu_init(struct a6xx_gpu *a6xx_gpu, struct device_node *node)
> > detach_cxpd:
> > dev_pm_domain_detach(gmu->cxpd, false);
> >
> > -err_mmio:
> > - iounmap(gmu->mmio);
> > - if (platform_get_resource_byname(pdev, IORESOURCE_MEM, "rscc"))
> > - iounmap(gmu->rscc);
> > +err_cleanup:
> > free_irq(gmu->gmu_irq, gmu);
> > free_irq(gmu->hfi_irq, gmu);
> >
> >
> > ---
> > base-commit: 1b294a1f35616977caddaddf3e9d28e576a1adbc
> > change-id: 20240608-adreno-98c412bfdc03
> >
> > Best regards,
> > --
> > Kiarash Hajian <kiarash8112hajian@...il.com>
> >
Powered by blists - more mailing lists