Skip to contentSkip to conversation

drm/v3d: Fix PM disable depth imbalance in v3d_platform_drm_probe

PATCH Superseded Miaoqian Lin 4 participants 7 messages Jan 5, 2022 – Jan 10, 2022

Subsystems (3)
DRM drivers DRM misc DRM drivers for V3D

v1 of 4 Superseded Latest is v4 →

lore
Share & tools
lore

Miaoqian Lin

author
#0 lore raw

PATCH 1/1 drm/v3d: Fix PM disable depth imbalance in v3d_platform_drm_probe

The pm_runtime_enable will increase power disable depth. If the probe fails, we should use pm_runtime_disable() to balance pm_runtime_enable().

Fixes 57692c9 ("drm/v3d: Introduce a new DRM driver for Broadcom V3D V3.x+")
Signed-off-by Miaoqian Lin <[email protected]>
---
 drivers/gpu/drm/v3d/v3d_drv.c | 2 ++
 1 file changed, 2 insertions(+)
drivers/gpu/drm/v3d/v3d_drv.c +2 -0
index bd46396a1ae0..4f293aa733b8 100644
@@ -300,6 +300,8 @@ static int v3d_platform_drm_probe(struct platform_device *pdev)
 	v3d_gem_destroy(drm);
 dma_free:
 	dma_free_wc(dev, 4096, v3d->mmu_scratch, v3d->mmu_scratch_paddr);
+pm_disable:
+	pm_runtime_disable(dev);
 	return ret;
 }
Signature · 1 line
2.17.1
build bot
#1 lore raw
Thank you for the patch! Perhaps something to improve:

Hi Miaoqian,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on drm/drm-next]
[also build test WARNING on linux/master linus/master v5.16-rc8 next-20220105] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Miaoqian-Lin/drm-v3d-Fix-PM-disable-depth-imbalance-in-v3d_platform_drm_probe/20220105-200602
base:   git://anongit.freedesktop.org/drm/drm drm-next
config: riscv-randconfig-r013-20220105 (https://download.01.org/0day-ci/archive/20220106/202201060655.NBwUPRqC-lkp(a)intel.com/config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project d5b6e30ed3acad794dd0aec400e617daffc6cc3d)

reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install riscv cross compiling tool for clang build
# apt-get install binutils-riscv64-linux-gnu
# https://github.com/0day-ci/linux/commit/9e89d2b210d16b00d43b6cf5a8e70495403da480
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Miaoqian-Lin/drm-v3d-Fix-PM-disable-depth-imbalance-in-v3d_platform_drm_probe/20220105-200602 git checkout 9e89d2b210d16b00d43b6cf5a8e70495403da480
# save the config file to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=riscv SHELL=/bin/bash drivers/gpu/drm/v3d/

If you fix the issue, kindly add following tag as appropriate

Reported-by kernel test robot <[email protected]>

All warnings (new ones prefixed by >>):

>> drivers/gpu/drm/v3d/v3d_drv.c:303:1: warning: unused label 'pm_disable' [-Wunused-label]

pm_disable:
^~~~~~~~~~~
1 warning generated.

vim +/pm_disable +303 drivers/gpu/drm/v3d/v3d_drv.c

213
214 static int v3d_platform_drm_probe(struct platform_device *pdev)
215 {
216 struct device *dev = &pdev->dev;
217 struct drm_device *drm;
218 struct v3d_dev *v3d;
219 int ret;
220 u32 mmu_debug;
221 u32 ident1;
222
223 v3d = devm_drm_dev_alloc(dev, &v3d_drm_driver, struct v3d_dev, drm);
224 if (IS_ERR(v3d))
225 return PTR_ERR(v3d);
226
227 drm = &v3d->drm;
228
229 platform_set_drvdata(pdev, drm);
230
231 ret = map_regs(v3d, &v3d->hub_regs, "hub");
232 if (ret)
233 return ret;
234
235 ret = map_regs(v3d, &v3d->core_regs[0], "core0");
236 if (ret)
237 return ret;
238
239 mmu_debug = V3D_READ(V3D_MMU_DEBUG_INFO);
240 dma_set_mask_and_coherent(dev,
241 DMA_BIT_MASK(30 + V3D_GET_FIELD(mmu_debug, V3D_MMU_PA_WIDTH)));
242 v3d->va_width = 30 + V3D_GET_FIELD(mmu_debug, V3D_MMU_VA_WIDTH);
243
244 ident1 = V3D_READ(V3D_HUB_IDENT1);
245 v3d->ver = (V3D_GET_FIELD(ident1, V3D_HUB_IDENT1_TVER) * 10 +
246 V3D_GET_FIELD(ident1, V3D_HUB_IDENT1_REV));
247 v3d->cores = V3D_GET_FIELD(ident1, V3D_HUB_IDENT1_NCORES);
248 WARN_ON(v3d->cores > 1); /* multicore not yet implemented */
249
250 v3d->reset = devm_reset_control_get_exclusive(dev, NULL);
251 if (IS_ERR(v3d->reset)) {
252 ret = PTR_ERR(v3d->reset);
253
254 if (ret == -EPROBE_DEFER)
255 return ret;
256
257 v3d->reset = NULL;
258 ret = map_regs(v3d, &v3d->bridge_regs, "bridge");
259 if (ret) {
260 dev_err(dev,
261 "Failed to get reset control or bridge regs\n");
262 return ret;
263 }
264 }
265
266 if (v3d->ver < 41) {
267 ret = map_regs(v3d, &v3d->gca_regs, "gca");
268 if (ret)
269 return ret;
270 }
271
272 v3d->mmu_scratch = dma_alloc_wc(dev, 4096, &v3d->mmu_scratch_paddr,
273 GFP_KERNEL | __GFP_NOWARN | __GFP_ZERO);
274 if (!v3d->mmu_scratch) {
275 dev_err(dev, "Failed to allocate MMU scratch page\n");
276 return -ENOMEM;
277 }
278
279 pm_runtime_use_autosuspend(dev);
280 pm_runtime_set_autosuspend_delay(dev, 50);
281 pm_runtime_enable(dev);
282
283 ret = v3d_gem_init(drm);
284 if (ret)
285 goto dma_free;
286
287 ret = v3d_irq_init(v3d);
288 if (ret)
289 goto gem_destroy;
290
291 ret = drm_dev_register(drm, 0);
292 if (ret)
293 goto irq_disable;
294
295 return 0;
296
297 irq_disable:
298 v3d_irq_disable(v3d);
299 gem_destroy:
300 v3d_gem_destroy(drm);
301 dma_free:
302 dma_free_wc(dev, 4096, v3d->mmu_scratch, v3d->mmu_scratch_paddr);

 > 303	pm_disable:

304 pm_runtime_disable(dev);
305 return ret;
306 }
307

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

#2 lore raw
In reply to Miaoqian Lin

Thanks for the patch.

Quoted 16 lines · Miaoqian Lin

The pm_runtime_enable will increase power disable depth. If the probe fails, we should use pm_runtime_disable() to balance pm_runtime_enable().

Fixes 57692c9 ("drm/v3d: Introduce a new DRM driver for Broadcom V3D V3.x+")
Signed-off-by Miaoqian Lin <[email protected]>
---
 drivers/gpu/drm/v3d/v3d_drv.c | 2 ++
 1 file changed, 2 insertions(+)
drivers/gpu/drm/v3d/v3d_drv.c +0 -0
index bd46396a1ae0..4f293aa733b8 100644

drivers/gpu/drm/v3d/v3d_drv.c static int v3d_platform_drm_probe(struct platform_device *pdev) · line 304

Quoted 5 lines
drivers/gpu/drm/v3d/v3d_drv.c +2 -0
@@ -300,6 +300,8 @@ static int v3d_platform_drm_probe(struct platform_device *pdev)
        v3d_gem_destroy(drm);
 dma_free:
        dma_free_wc(dev, 4096, v3d->mmu_scratch, v3d->mmu_scratch_paddr);
+pm_disable:
+       pm_runtime_disable(dev);

The dma_alloc_wc is done before the pm_runtime_enable, so the cleanup should be in the opposite order.
Functionally it makes minimal difference in this case as pm_runtime_enable can't fail, but could cause confusion/errors should any other initialisation step be added between the two.

The pm_disable label is also unused so not necessary, however if reversing the order then renaming dma_free to pm_disable would be sensible.

Dave

Quoted 7 lines
drivers/gpu/drm/v3d/v3d_drv.c +0 -1
        return ret;
 }
 
 
--

2.17.1

author
#3 lore raw

PATCH v2 1/1 drm/v3d: Fix PM disable depth imbalance in v3d_platform_drm_probe

The pm_runtime_enable will increase power disable depth. If the probe fails, we should use pm_runtime_disable() to balance pm_runtime_enable().

Fixes 57692c9 ("drm/v3d: Introduce a new DRM driver for Broadcom V3D V3.x+")
Signed-off-by Miaoqian Lin <[email protected]>

---
Changes in v2
- put pm_runtime_disable before dma_free_wc
- rename dma_free to pm_disable

---
 drivers/gpu/drm/v3d/v3d_drv.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
drivers/gpu/drm/v3d/v3d_drv.c +3 -2
index bd46396a1ae0..7d500dd5314e 100644
@@ -282,7 +282,7 @@ static int v3d_platform_drm_probe(struct platform_device *pdev)
 
 	ret = v3d_gem_init(drm);
 	if (ret)
-		goto dma_free;
+		goto pm_disable;
 
 	ret = v3d_irq_init(v3d);
 	if (ret)

@@ -298,7 +298,8 @@ static int v3d_platform_drm_probe(struct platform_device *pdev)
 	v3d_irq_disable(v3d);
 gem_destroy:
 	v3d_gem_destroy(drm);
-dma_free:
+pm_disable:
+	pm_runtime_disable(dev);
 	dma_free_wc(dev, 4096, v3d->mmu_scratch, v3d->mmu_scratch_paddr);
 	return ret;
 }
Signature · 1 line
2.17.1
Reviewed-by on v2 1/1
#4 lore raw
Quoted 7 lines · Miaoqian Lin

The pm_runtime_enable will increase power disable depth. If the probe fails, we should use pm_runtime_disable() to balance pm_runtime_enable().

Fixes 57692c9 ("drm/v3d: Introduce a new DRM driver for Broadcom V3D V3.x+")
Signed-off-by Miaoqian Lin <[email protected]>

Thanks for the update - that looks good to me.

Reviewed-by Dave Stevenson <[email protected]>
Quoted 34 lines

---
Changes in v2
- put pm_runtime_disable before dma_free_wc
- rename dma_free to pm_disable

---
 drivers/gpu/drm/v3d/v3d_drv.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
drivers/gpu/drm/v3d/v3d_drv.c +3 -3
index bd46396a1ae0..7d500dd5314e 100644
@@ -282,7 +282,7 @@ static int v3d_platform_drm_probe(struct platform_device *pdev)
 
        ret = v3d_gem_init(drm);
        if (ret)
-               goto dma_free;
+               goto pm_disable;
 
        ret = v3d_irq_init(v3d);
        if (ret)

@@ -298,7 +298,8 @@ static int v3d_platform_drm_probe(struct platform_device *pdev)
        v3d_irq_disable(v3d);
 gem_destroy:
        v3d_gem_destroy(drm);
-dma_free:
+pm_disable:
+       pm_runtime_disable(dev);
        dma_free_wc(dev, 4096, v3d->mmu_scratch, v3d->mmu_scratch_paddr);
        return ret;
 }
--

2.17.1

maintainer
#5 lore raw
In reply to Miaoqian Lin
Quoted 28 lines · Miaoqian Lin

The pm_runtime_enable will increase power disable depth. If the probe fails, we should use pm_runtime_disable() to balance pm_runtime_enable().

Fixes 57692c9 ("drm/v3d: Introduce a new DRM driver for Broadcom V3D V3.x+")
Signed-off-by Miaoqian Lin <[email protected]>

---
Changes in v2
- put pm_runtime_disable before dma_free_wc
- rename dma_free to pm_disable

---
 drivers/gpu/drm/v3d/v3d_drv.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
drivers/gpu/drm/v3d/v3d_drv.c +1 -1
index bd46396a1ae0..7d500dd5314e 100644
@@ -282,7 +282,7 @@ static int v3d_platform_drm_probe(struct platform_device *pdev)
 
 	ret = v3d_gem_init(drm);
 	if (ret)
-		goto dma_free;
+		goto pm_disable;
 
 	ret = v3d_irq_init(v3d);
 	if (ret)

drivers/gpu/drm/v3d/v3d_drv.c static int v3d_platform_drm_probe(struct platform_device *pdev) · line 302

Quoted 6 lines
drivers/gpu/drm/v3d/v3d_drv.c +2 -1
@@ -298,7 +298,8 @@ static int v3d_platform_drm_probe(struct platform_device *pdev)
 	v3d_irq_disable(v3d);
 gem_destroy:
 	v3d_gem_destroy(drm);
-dma_free:
+pm_disable:
+	pm_runtime_disable(dev);

Hi,

I see this pm_runtime_disable balancing is also missing for v3d_platform_drm_remove(), right?

BR,

Melissa

Quoted 6 lines
drivers/gpu/drm/v3d/v3d_drv.c +0 -0
 	dma_free_wc(dev, 4096, v3d->mmu_scratch, v3d->mmu_scratch_paddr);
 	return ret;
 }
Signature · 1 line
2.17.1
author
#6 lore raw

Hi Melissa,

Quoted 21 lines · Melissa Wen
Quoted 11 lines · Miaoqian Lin

The pm_runtime_enable will increase power disable depth. If the probe fails, we should use pm_runtime_disable() to balance pm_runtime_enable().

drivers/gpu/drm/v3d/v3d_drv.c +1 -1
 	if (ret)
-		goto dma_free;
+		goto pm_disable;
 
 	ret = v3d_irq_init(v3d);
 	if (ret)

drivers/gpu/drm/v3d/v3d_drv.c static int v3d_platform_drm_probe(struct platform_device *pdev) · line 302

Quoted 6 lines
drivers/gpu/drm/v3d/v3d_drv.c +2 -1
@@ -298,7 +298,8 @@ static int v3d_platform_drm_probe(struct platform_device *pdev)
 	v3d_irq_disable(v3d);
 gem_destroy:
 	v3d_gem_destroy(drm);
-dma_free:
+pm_disable:
+	pm_runtime_disable(dev);

Hi,

Quoted 3 lines

I see this pm_runtime_disable balancing is also missing for v3d_platform_drm_remove(), right?

I think, yes.

Quoted 6 lines
Quoted 6 lines
drivers/gpu/drm/v3d/v3d_drv.c +0 -0
 	dma_free_wc(dev, 4096, v3d->mmu_scratch, v3d->mmu_scratch_paddr);
 	return ret;
 }
Signature · 1 line
2.17.1