Re: powervr lockdep warnings
On 10/05/2024 09:43, Chen-Yu Tsai wrote: > Hi, > > I got the following lockdep warnings while trying to make the powervr > driver work on MT8173. This was observed while trying to run vkmark. > This was on the next-20240506 kernel running Debian Sid with the > Mesa 24.0.6 package rebuilt to include the powervr driver. Hi, Thanks for the report! I've got an elm chromebook set up with the same versions mentioned above, and spent yesterday trying to make it reproduce the issue without success. I don't really have time at the moment to keep working on this, but I've made a note to come back to it when I get a chance. From the traces below, it doesn't *seem* like this should be a platform specific issue, so I'm definitely interested in getting it fixed. Cheers, Matt > [73602.438144] [ cut here ] > [73602.450563] WARNING: CPU: 3 PID: 2244 at > drivers/gpu/drm/drm_gpuvm.c:1874 drm_gpuva_unlink+0xec/0x140 > [drm_gpuvm] > [73602.468778] Modules linked in: mtk_vcodec_dec mtk_vcodec_enc > v4l2_vp9 v4l2_h264 cdc_ether mtk_vcodec_dbgfs usbnet mtk_vcodec_common > mtk_jpeg uvcvideo mtk_scp powervr mtk_jpeg_enc_hw mtk_rpmsg mtk_mdp > rpmsg_core cros_ec_sensors mtk_jpeg_dec_hw mtk_scp_ipi > cros_ec_sensors_core videobuf2_vmalloc v4l2_mem2mem r8152 uvc > videobuf2_dma_contig videobuf2_v4l2 drm_gpuvm videobuf2_memops sha1_ce > videobuf2_common mii drm_exec cros_ec_sensorhub mtk_vpu joydev fuse > [73602.526113] CPU: 3 PID: 2244 Comm: vkmark Tainted: GW > 6.9.0-rc7-next-20240506-11489-g2585d27380e4-dirty #217 > b57080d80a375eadc1b59c661ce880f5be496816 > [73602.549750] Hardware name: Google Elm (DT) > [73602.562503] pstate: 6045 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--) > [73602.578257] pc : drm_gpuva_unlink+0xec/0x140 [drm_gpuvm] > [73602.592391] lr : drm_gpuva_unlink+0x12c/0x140 [drm_gpuvm] > [73602.606597] sp : ffc086aa7590 > [73602.618660] x29: ffc086aa7590 x28: 0080 x27: > ffc07ca59b98 > [73602.634725] x26: ffc086aa7900 x25: x24: > ff80c14c5980 > [73602.650776] x23: ff80054cee08 x22: ff80c14c5d00 x21: > ffc082f8a000 > [73602.666878] x20: ff80d1c3a000 x19: ff80054cee00 x18: > > [73602.682898] x17: x16: x15: > 007fc6e28c90 > [73602.698907] x14: 1ff810d54ea4 x13: 41b58ab3 x12: > ffb019c8f00c > [73602.714919] x11: 1ff019c8f00b x10: ffb019c8f00b x9 : > ffc07ca1dff8 > [73602.730964] x8 : 004fe6370ff5 x7 : ff80ce47805b x6 : > 0001 > [73602.746942] x5 : ff80ce478058 x4 : ff80ce478058 x3 : > > [73602.762920] x2 : x1 : ff80d1c3a1a0 x0 : > > [73602.778793] Call trace: > [73602.789746] drm_gpuva_unlink+0xec/0x140 [drm_gpuvm > bbf6d948c0b434a2936abb76cd7734fb954b4801] > [73602.807036] pvr_vm_gpuva_unmap+0x88/0xb0 [powervr > 3ad437ff1d69ca6bbe76c29aac5b59cf4d3e54e4] > [73602.824299] op_unmap_cb.isra.0+0xbc/0x108 [drm_gpuvm > bbf6d948c0b434a2936abb76cd7734fb954b4801] > [73602.841559] __drm_gpuvm_sm_unmap+0x288/0x2c0 [drm_gpuvm > bbf6d948c0b434a2936abb76cd7734fb954b4801] > [73602.858863] drm_gpuvm_sm_unmap+0x78/0xb8 [drm_gpuvm > bbf6d948c0b434a2936abb76cd7734fb954b4801] > [73602.875693] pvr_vm_bind_op_exec+0x6c/0x118 [powervr > 3ad437ff1d69ca6bbe76c29aac5b59cf4d3e54e4] > [73602.892483] pvr_vm_unmap+0x1f8/0x238 [powervr > 3ad437ff1d69ca6bbe76c29aac5b59cf4d3e54e4] > [73602.908746] pvr_ioctl_vm_unmap+0x80/0xb8 [powervr > 3ad437ff1d69ca6bbe76c29aac5b59cf4d3e54e4] > [73602.925386] drm_ioctl_kernel+0x140/0x1d0 > [73602.937503] drm_ioctl+0x3e8/0x7e0 > [73602.948949] __arm64_sys_ioctl+0xec/0x118 > [73602.960993] invoke_syscall+0x68/0x198 > [73602.972749] el0_svc_common.constprop.0+0x80/0x150 > [73602.985561] do_el0_svc+0x38/0x50 > [73602.996856] el0_svc+0x4c/0xc0 > [73603.007821] el0t_64_sync_handler+0x120/0x130 > [73603.020096] el0t_64_sync+0x1a8/0x1b0 > [73603.031606] irq event stamp: 153208 > [73603.042881] hardirqs last enabled at (153207): > [] _raw_spin_unlock_irqrestore+0x98/0xa8 > [73603.060595] hardirqs last disabled at (153208): > [] el1_dbg+0x28/0x90 > [73603.076534] softirqs last enabled at (153032): > [] fpsimd_restore_current_state+0x4c/0xf8 > [73603.094345] softirqs last disabled at (153030): > [] fpsimd_restore_current_state+0x1c/0xf8 > [73603.112041] ---[ end trace ]--- > [73603.125721] [ cut here ] > [73603.165947] WARNING: CPU: 0 PID: 2244 at > drivers/gpu/drm/drm_gpuvm.c:1514 drm_gpuvm_bo_put.part.0+0x1c0/0x4d0 > [drm_gpuvm] > [73603.213280] Modules linked in: mtk_vcodec_dec mtk_vcodec_enc > v4l2_vp9 v4l2_h264 cdc_ether mtk_vcodec_dbgfs usbnet mtk_vcodec_common > mtk_jpeg uvcvideo mtk_scp powervr mtk_jpeg_enc_hw mtk_rpmsg mtk_mdp > rpmsg_core cros_ec_sensors mtk_jpeg_dec_hw mtk_scp_ipi > cros_ec_sensors_core videobuf2_vmalloc v4l2_mem2mem r8152 uvc > videobuf2_dma_contig
[PATCH] drm/imagination: Ensure PVR_MIPS_PT_PAGE_COUNT is never zero
When the host page size was more than 4 times larger than the FW page
size, this macro evaluated to zero resulting in zero-sized arrays.
Use DIV_ROUND_UP() to ensure the correct behavior.
Reported-by: 20240228012313.5934-1-ya...@kylinos.cn
Closes: https://lore.kernel.org/dri-devel/20240228012313.5934-1-ya...@kylinos.cn
Fixes: 927f3e0253c1 ("drm/imagination: Implement MIPS firmware processor and
MMU support")
Signed-off-by: Matt Coster
---
drivers/gpu/drm/imagination/pvr_fw_mips.h | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/imagination/pvr_fw_mips.h
b/drivers/gpu/drm/imagination/pvr_fw_mips.h
index 408dbe63a90c..a0c5c41c8aa2 100644
--- a/drivers/gpu/drm/imagination/pvr_fw_mips.h
+++ b/drivers/gpu/drm/imagination/pvr_fw_mips.h
@@ -7,13 +7,14 @@
#include "pvr_rogue_mips.h"
#include
+#include
#include
/* Forward declaration from pvr_gem.h. */
struct pvr_gem_object;
-#define PVR_MIPS_PT_PAGE_COUNT ((ROGUE_MIPSFW_MAX_NUM_PAGETABLE_PAGES *
ROGUE_MIPSFW_PAGE_SIZE_4K) \
- >> PAGE_SHIFT)
+#define PVR_MIPS_PT_PAGE_COUNT
DIV_ROUND_UP(ROGUE_MIPSFW_MAX_NUM_PAGETABLE_PAGES * ROGUE_MIPSFW_PAGE_SIZE_4K,
PAGE_SIZE)
+
/**
* struct pvr_fw_mips_data - MIPS-specific data
*/
base-commit: e95752752eaf06c860811ac5ddf9badf6c1b43ca
--
2.44.0
OpenPGP_signature.asc
Description: OpenPGP digital signature
Re: [PATCH 1/2] drm/print: drop include debugfs.h and include where needed
On 22/04/2024 13:10, Jani Nikula wrote: > Surprisingly many places depend on debugfs.h to be included via > drm_print.h. Fix them. > > v3: Also fix armada, ite-it6505, imagination, msm, sti, vc4, and xe > > v2: Also fix ivpu and vmwgfx > > Reviewed-by: Andrzej Hajda > Acked-by: Maxime Ripard > Link: > https://patchwork.freedesktop.org/patch/msgid/20240410141434.157908-1-jani.nik...@intel.com > Signed-off-by: Jani Nikula > > --- > > Cc: Jacek Lawrynowicz > Cc: Stanislaw Gruszka > Cc: Oded Gabbay > Cc: Russell King > Cc: David Airlie > Cc: Daniel Vetter > Cc: Andrzej Hajda > Cc: Neil Armstrong > Cc: Robert Foss > Cc: Laurent Pinchart > Cc: Jonas Karlman > Cc: Jernej Skrabec > Cc: Maarten Lankhorst > Cc: Maxime Ripard > Cc: Thomas Zimmermann > Cc: Jani Nikula > Cc: Rodrigo Vivi > Cc: Joonas Lahtinen > Cc: Tvrtko Ursulin > Cc: Frank Binns > Cc: Matt Coster > Cc: Rob Clark > Cc: Abhinav Kumar > Cc: Dmitry Baryshkov > Cc: Sean Paul > Cc: Marijn Suijten > Cc: Karol Herbst > Cc: Lyude Paul > Cc: Danilo Krummrich > Cc: Alex Deucher > Cc: "Christian König" > Cc: "Pan, Xinhui" > Cc: Alain Volmat > Cc: Huang Rui > Cc: Zack Rusin > Cc: Broadcom internal kernel review list > > Cc: Lucas De Marchi > Cc: "Thomas Hellström" > Cc: dri-devel@lists.freedesktop.org > Cc: intel-...@lists.freedesktop.org > Cc: intel...@lists.freedesktop.org > Cc: linux-arm-...@vger.kernel.org > Cc: freedr...@lists.freedesktop.org > Cc: nouv...@lists.freedesktop.org > Cc: amd-...@lists.freedesktop.org > --- > drivers/accel/ivpu/ivpu_debugfs.c | 2 ++ > drivers/gpu/drm/armada/armada_debugfs.c | 1 + > drivers/gpu/drm/bridge/ite-it6505.c | 1 + > drivers/gpu/drm/bridge/panel.c | 2 ++ > drivers/gpu/drm/drm_print.c | 6 +++--- > drivers/gpu/drm/i915/display/intel_dmc.c| 1 + > drivers/gpu/drm/imagination/pvr_fw_trace.c | 1 + Acked-by: Matt Coster # drm/imagination Cheers, Matt > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c | 2 ++ > drivers/gpu/drm/nouveau/dispnv50/crc.c | 2 ++ > drivers/gpu/drm/radeon/r100.c | 1 + > drivers/gpu/drm/radeon/r300.c | 1 + > drivers/gpu/drm/radeon/r420.c | 1 + > drivers/gpu/drm/radeon/r600.c | 3 ++- > drivers/gpu/drm/radeon/radeon_fence.c | 1 + > drivers/gpu/drm/radeon/radeon_gem.c | 1 + > drivers/gpu/drm/radeon/radeon_ib.c | 2 ++ > drivers/gpu/drm/radeon/radeon_pm.c | 1 + > drivers/gpu/drm/radeon/radeon_ring.c| 2 ++ > drivers/gpu/drm/radeon/radeon_ttm.c | 1 + > drivers/gpu/drm/radeon/rs400.c | 1 + > drivers/gpu/drm/radeon/rv515.c | 1 + > drivers/gpu/drm/sti/sti_drv.c | 1 + > drivers/gpu/drm/ttm/ttm_device.c| 1 + > drivers/gpu/drm/ttm/ttm_resource.c | 3 ++- > drivers/gpu/drm/ttm/ttm_tt.c| 5 +++-- > drivers/gpu/drm/vc4/vc4_drv.h | 1 + > drivers/gpu/drm/vmwgfx/vmwgfx_gem.c | 2 ++ > drivers/gpu/drm/xe/xe_debugfs.c | 1 + > drivers/gpu/drm/xe/xe_gt_debugfs.c | 2 ++ > drivers/gpu/drm/xe/xe_uc_debugfs.c | 2 ++ > include/drm/drm_print.h | 2 +- > 31 files changed, 46 insertions(+), 8 deletions(-) > > diff --git a/drivers/accel/ivpu/ivpu_debugfs.c > b/drivers/accel/ivpu/ivpu_debugfs.c > index d09d29775b3f..e07e447d08d1 100644 > --- a/drivers/accel/ivpu/ivpu_debugfs.c > +++ b/drivers/accel/ivpu/ivpu_debugfs.c > @@ -3,6 +3,8 @@ > * Copyright (C) 2020-2023 Intel Corporation > */ > > +#include > + > #include > #include > #include > diff --git a/drivers/gpu/drm/armada/armada_debugfs.c > b/drivers/gpu/drm/armada/armada_debugfs.c > index 29f4b52e3c8d..a763349dd89f 100644 > --- a/drivers/gpu/drm/armada/armada_debugfs.c > +++ b/drivers/gpu/drm/armada/armada_debugfs.c > @@ -5,6 +5,7 @@ > */ > > #include > +#include > #include > #include > #include > diff --git a/drivers/gpu/drm/bridge/ite-it6505.c > b/drivers/gpu/drm/bridge/ite-it6505.c > index 27334173e911..3f68c82888c2 100644 > --- a/drivers/gpu/drm/bridge/ite-it6505.c > +++ b/drivers/gpu/drm/bridge/ite-it6505.c > @@ -3,6 +3,7 @@ > * Copyright (c) 2020, The Linux Foundation. All rights reserved. > */ > #include > +#include > #include > #include > #include > diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c > index 7f41525f7a6e..32506524d9a2 100644 > --- a/drivers/gpu/drm/bridge/panel.c >
Re: [PATCH 0/4] gpu: Convert to platform remove callback returning void
On 19/04/2024 08:20, Uwe Kleine-König wrote:
> Hello,
>
> On Tue, Apr 09, 2024 at 07:02:47PM +0200, Uwe Kleine-König wrote:
>> with some patches sent earlier[1], this series converts all platform
>> drivers below drivers/gpu to not use struct platform_device::remove()
>> any more.
>>
>> See commit 5c5a7680e67b ("platform: Provide a remove callback that
>> returns no value") for an extended explanation and the eventual goal.
>>
>> All conversations are trivial, because the driver's .remove() callbacks
>> returned zero unconditionally.
>>
>> There are no interdependencies between these patches. This is merge
>> window material.
>
> I wonder how this series will make it in. While I would prefer these
> patches to go in together (that I can consider this thread completed in
> one go), I think with how drm maintenace works, it's best if the patches
> are picked up by their individual maintainers. I guess that's:
>
> - Frank Binns + Matt Coster for imagination
I've acked the imagination patch - feel free to land it however you
like. We don't have a separate tree so we'd just land it in
drm-misc-next.
Cheers,
Matt
> - Chun-Kuang Hu + Philipp Zabel for mediatek
>
> - Thierry Reding + Mikko Perttunen for the host1x driver
>(Note there is another patch for this driver set at
> 20240409165043.105137-2-u.kleine-koe...@pengutronix.de that is
> relevant for the same quest.)
>
> - Philipp Zabel for ipu-v3
>
> I plan to send a patch changing struct platform_driver::remove after the
> end of the merge window leading to 6.10-rc1 for inclusion in next via
> Greg's driver core. So please either care the patches land in 6.10-rc1
> or ack that I include them in the submission to Greg.
>
> Thanks for your cooperation,
> Uwe
>
OpenPGP_signature.asc
Description: OpenPGP digital signature
Re: [PATCH 1/4] drm/imagination: Convert to platform remove callback returning void
On 09/04/2024 18:02, Uwe Kleine-König wrote:
> The .remove() callback for a platform driver returns an int which makes
> many driver authors wrongly assume it's possible to do error handling by
> returning an error code. However the value returned is ignored (apart
> from emitting a warning) and this typically results in resource leaks.
>
> To improve here there is a quest to make the remove callback return
> void. In the first step of this quest all drivers are converted to
> .remove_new(), which already returns void. Eventually after all drivers
> are converted, .remove_new() will be renamed to .remove().
>
> Trivially convert this driver from always returning zero in the remove
> callback to the void returning variant.
>
> Signed-off-by: Uwe Kleine-König
Acked-by: Matt Coster
> ---
> drivers/gpu/drm/imagination/pvr_drv.c | 7 ++-
> 1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/imagination/pvr_drv.c
> b/drivers/gpu/drm/imagination/pvr_drv.c
> index 5c3b2d58d766..1a0cb7aa9cea 100644
> --- a/drivers/gpu/drm/imagination/pvr_drv.c
> +++ b/drivers/gpu/drm/imagination/pvr_drv.c
> @@ -1451,8 +1451,7 @@ pvr_probe(struct platform_device *plat_dev)
> return err;
> }
>
> -static int
> -pvr_remove(struct platform_device *plat_dev)
> +static void pvr_remove(struct platform_device *plat_dev)
> {
> struct drm_device *drm_dev = platform_get_drvdata(plat_dev);
> struct pvr_device *pvr_dev = to_pvr_device(drm_dev);
> @@ -1469,8 +1468,6 @@ pvr_remove(struct platform_device *plat_dev)
> pvr_watchdog_fini(pvr_dev);
> pvr_queue_device_fini(pvr_dev);
> pvr_context_device_fini(pvr_dev);
> -
> - return 0;
> }
>
> static const struct of_device_id dt_match[] = {
> @@ -1485,7 +1482,7 @@ static const struct dev_pm_ops pvr_pm_ops = {
>
> static struct platform_driver pvr_driver = {
> .probe = pvr_probe,
> - .remove = pvr_remove,
> + .remove_new = pvr_remove,
> .driver = {
> .name = PVR_DRIVER_NAME,
> .pm = _pm_ops,
OpenPGP_0x747F0A9036F90DFA.asc
Description: OpenPGP public key
OpenPGP_signature.asc
Description: OpenPGP digital signature
Re: [PATCH 1/4] drm/imagination: Convert to platform remove callback returning void
On 09/04/2024 10:02, Uwe Kleine-König wrote:
> The .remove() callback for a platform driver returns an int which makes
> many driver authors wrongly assume it's possible to do error handling by
> returning an error code. However the value returned is ignored (apart
> from emitting a warning) and this typically results in resource leaks.
>
> To improve here there is a quest to make the remove callback return
> void. In the first step of this quest all drivers are converted to
> .remove_new(), which already returns void. Eventually after all drivers
> are converted, .remove_new() will be renamed to .remove().
>
> Trivially convert this driver from always returning zero in the remove
> callback to the void returning variant.
>
> Signed-off-by: Uwe Kleine-König
Reviewed-by: Matt Coster
> ---
> drivers/gpu/drm/imagination/pvr_drv.c | 7 ++-
> 1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/imagination/pvr_drv.c
> b/drivers/gpu/drm/imagination/pvr_drv.c
> index 5c3b2d58d766..1a0cb7aa9cea 100644
> --- a/drivers/gpu/drm/imagination/pvr_drv.c
> +++ b/drivers/gpu/drm/imagination/pvr_drv.c
> @@ -1451,8 +1451,7 @@ pvr_probe(struct platform_device *plat_dev)
> return err;
> }
>
> -static int
> -pvr_remove(struct platform_device *plat_dev)
> +static void pvr_remove(struct platform_device *plat_dev)
> {
> struct drm_device *drm_dev = platform_get_drvdata(plat_dev);
> struct pvr_device *pvr_dev = to_pvr_device(drm_dev);
> @@ -1469,8 +1468,6 @@ pvr_remove(struct platform_device *plat_dev)
> pvr_watchdog_fini(pvr_dev);
> pvr_queue_device_fini(pvr_dev);
> pvr_context_device_fini(pvr_dev);
> -
> - return 0;
> }
>
> static const struct of_device_id dt_match[] = {
> @@ -1485,7 +1482,7 @@ static const struct dev_pm_ops pvr_pm_ops = {
>
> static struct platform_driver pvr_driver = {
> .probe = pvr_probe,
> - .remove = pvr_remove,
> + .remove_new = pvr_remove,
> .driver = {
> .name = PVR_DRIVER_NAME,
> .pm = _pm_ops,
OpenPGP_signature.asc
Description: OpenPGP digital signature
Re: [PATCH] [RESEND] drm/imagination: avoid -Woverflow warning
On 22/03/2024 13:01, Arnd Bergmann wrote:
> From: Arnd Bergmann
>
> The array size calculation in pvr_vm_mips_fini() appears to be incorrect
> based on
> taking the size of the pointer rather than the size of the array, which
> manifests
> as a warning about signed integer overflow:
>
> In file included from include/linux/kernel.h:16,
> from drivers/gpu/drm/imagination/pvr_rogue_fwif.h:10,
> from drivers/gpu/drm/imagination/pvr_ccb.h:7,
> from drivers/gpu/drm/imagination/pvr_device.h:7,
> from drivers/gpu/drm/imagination/pvr_vm_mips.c:4:
> drivers/gpu/drm/imagination/pvr_vm_mips.c: In function 'pvr_vm_mips_fini':
> include/linux/array_size.h:11:25: error: overflow in conversion from 'long
> unsigned int' to 'int' changes value from '18446744073709551615' to '-1'
> [-Werror=overflow]
>11 | #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) +
> __must_be_array(arr))
> | ^
> drivers/gpu/drm/imagination/pvr_vm_mips.c:106:24: note: in expansion of macro
> 'ARRAY_SIZE'
> 106 | for (page_nr = ARRAY_SIZE(mips_data->pt_pages) - 1; page_nr
> >= 0; page_nr--) {
> |^~
>
> Just use the number of array elements directly here, and in the corresponding
> init function for consistency.
>
> Fixes: 927f3e0253c1 ("drm/imagination: Implement MIPS firmware processor and
> MMU support")
> Reviewed-by: Donald Robson
> Link:
> https://lore.kernel.org/lkml/9df9e4f87727399928c068dbbf614c9895ae15f9.ca...@imgtec.com/
> Signed-off-by: Arnd Bergmann
> ---
> I sent this one last year when the warning appeared, it looks like it
> got lost in the meantime, resending it unchanged.
Apologies for letting this slip through the cracks last year - applied
to drm-misc-next.
Thanks for resending!
Cheers,
Matt
> ---
> drivers/gpu/drm/imagination/pvr_vm_mips.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/imagination/pvr_vm_mips.c
> b/drivers/gpu/drm/imagination/pvr_vm_mips.c
> index b7fef3c797e6..4f99b4af871c 100644
> --- a/drivers/gpu/drm/imagination/pvr_vm_mips.c
> +++ b/drivers/gpu/drm/imagination/pvr_vm_mips.c
> @@ -46,7 +46,7 @@ pvr_vm_mips_init(struct pvr_device *pvr_dev)
> if (!mips_data)
> return -ENOMEM;
>
> - for (page_nr = 0; page_nr < ARRAY_SIZE(mips_data->pt_pages); page_nr++)
> {
> + for (page_nr = 0; page_nr < PVR_MIPS_PT_PAGE_COUNT; page_nr++) {
> mips_data->pt_pages[page_nr] = alloc_page(GFP_KERNEL |
> __GFP_ZERO);
> if (!mips_data->pt_pages[page_nr]) {
> err = -ENOMEM;
> @@ -102,7 +102,7 @@ pvr_vm_mips_fini(struct pvr_device *pvr_dev)
> int page_nr;
>
> vunmap(mips_data->pt);
> - for (page_nr = ARRAY_SIZE(mips_data->pt_pages) - 1; page_nr >= 0;
> page_nr--) {
> + for (page_nr = PVR_MIPS_PT_PAGE_COUNT - 1; page_nr >= 0; page_nr--) {
> dma_unmap_page(from_pvr_device(pvr_dev)->dev,
> mips_data->pt_dma_addr[page_nr], PAGE_SIZE,
> DMA_TO_DEVICE);
>
OpenPGP_signature.asc
Description: OpenPGP digital signature
Re: [PATCH] drm/imagination: Kconfig: add 'PAGE_SIZE=4K' dependency
Hi, thanks for the patch!
On 28/02/2024 01:23, Lu Yao wrote:
> When 'PAGE_SIZE=64K',the following compilation error occurs:
> "
> ../drivers/gpu/drm/imagination/pvr_fw_mips.c: In function
> ‘pvr_mips_fw_process’:
> ../drivers/gpu/drm/imagination/pvr_fw_mips.c:140:60: error: array
> subscript 0 is outside the bounds of an interior zero-length array
> ‘dma_addr_t[0]’ {aka ‘long long unsigned int[]’}
> [-Werror=zero-length-bounds]
> 140 | boot_data->pt_phys_addr[page_nr] =
> mips_data->pt_dma_addr[src_page_nr] +
> ~~^
> In file included from ../drivers/gpu/drm/imagination/pvr_fw_mips.c:6:
> ../drivers/gpu/drm/imagination/pvr_fw_mips.h:30:13: note: while
> referencing ‘pt_dma_addr’
>30 | dma_addr_t pt_dma_addr[PVR_MIPS_PT_PAGE_COUNT];
> "
>
> This is because 'PVR_MIPS_PT_PAGE_COUNT' is defined as
> '(ROGUE_MIPSFW_MAX_NUM_PAGETABLE_PAGES * ROGUE_MIPSFW_PAGE_SIZE_4K) \
>>> PAGE_SHIFT', and under the above conditions, 'PAGE_SHIFT' is '16',
> 'ROGUE_MIPSFW_MAX_NUM_PAGETABLE_PAGES' is '4','ROGUE_MIPSFW_PAGE_SIZE_4K'
> is '4096',so 'PVR_MIPS_PT_PAGE_COUNT' is '0' causing the member
> 'pt_dma_addr' to be incorrectly defined.
The whole MIPS page table system is supposed to be host page-size
agnostic. In practice, we don’t regularly test on non-4K platforms so
this may well be broken in subtle or not-so-subtle (as in this instance)
ways. There has been at least some testing with 64K host pages – the
configs from TI for the AM62-SK dev boards have that as the default (or
at least they did when we started working with them).
I’m loathed to accept this patch without at least investigating how deep
the problems go; the true fix here should be to fix the problems causing
this build error rather than just gating off non-4K hosts.
I’ll have a dig this afternoon to see what I can find. Did you try
anything to fix this yourself? It would be nice to not duplicate effort
on this if you have.
Cheers,
Matt
> Signed-off-by: Lu Yao
> ---
> drivers/gpu/drm/imagination/Kconfig | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/imagination/Kconfig
> b/drivers/gpu/drm/imagination/Kconfig
> index 3bfa2ac212dc..e585922f634d 100644
> --- a/drivers/gpu/drm/imagination/Kconfig
> +++ b/drivers/gpu/drm/imagination/Kconfig
> @@ -3,7 +3,7 @@
>
> config DRM_POWERVR
> tristate "Imagination Technologies PowerVR (Series 6 and later) & IMG
> Graphics"
> - depends on ARM64
> + depends on (ARM64 && ARM64_PAGE_SHIFT=12)
> depends on DRM
> depends on PM
> select DRM_EXEC
OpenPGP_signature.asc
Description: OpenPGP digital signature
Re: [PATCH 2/6] arm64: dts: renesas: r8a774a1: Enable GPU
Hi Adam,
Thanks for these patches! I'll just reply to this one patch, but my
comments apply to them all.
On 27/02/2024 03:45, Adam Ford wrote:
> The GPU on the RZ/G2M is a Rogue GX6250 which uses firmware
> rogue_4.45.2.58_v1.fw available from Imagination.
>
> When enumerated, it appears as:
> powervr fd00.gpu: [drm] loaded firmware powervr/rogue_4.45.2.58_v1.fw
> powervr fd00.gpu: [drm] FW version v1.0 (build 6513336 OS)
These messages are printed after verifying the firmware blob’s headers,
*before* attempting to upload it to the device. Just because they appear
in dmesg does *not* imply the device is functional beyond the handful of
register reads in pvr_load_gpu_id().
Since Mesa does not yet have support for this GPU, there’s not a lot
that can be done to actually test these bindings.
When we added upstream support for the first GPU (the AXE core in TI’s
AM62), we opted to wait until userspace was sufficiently progressed to
the point it could be used for testing. This thought process still
applies when adding new GPUs.
Our main concern is that adding bindings for GPUs implies a level of
support that cannot be tested. That in turn may make it challenging to
justify UAPI changes if/when they’re needed to actually make these GPUs
functional.
> Signed-off-by: Adam Ford
>
> diff --git a/arch/arm64/boot/dts/renesas/r8a774a1.dtsi
> b/arch/arm64/boot/dts/renesas/r8a774a1.dtsi
> index a8a44fe5e83b..8923d9624b39 100644
> --- a/arch/arm64/boot/dts/renesas/r8a774a1.dtsi
> +++ b/arch/arm64/boot/dts/renesas/r8a774a1.dtsi
> @@ -2352,6 +2352,16 @@ gic: interrupt-controller@f101 {
> resets = < 408>;
> };
>
> + gpu: gpu@fd00 {
> + compatible = "renesas,r8a774a1-gpu", "img,img-axe";
The GX6250 is *not* an AXE core - it shouldn’t be listed as compatible
with one. For prior art, see [1] where we added support for the MT8173
found in Elm Chromebooks R13 (also a Series6XT GPU).
> + reg = <0 0xfd00 0 0x2>;
> + clocks = < CPG_MOD 112>;
> + clock-names = "core";
Series6XT cores have three clocks (see [1] again). I don’t have a
Renesas TRM to hand – do you know if their docs go into detail on the
GPU integration?
> + interrupts = ;
> + power-domains = < R8A774A1_PD_3DG_B>;
> + resets = < 112>;
> + };
> +
> pciec0: pcie@fe00 {
> compatible = "renesas,pcie-r8a774a1",
>"renesas,pcie-rcar-gen3";
As you probably expect by this point, I have to nack this series for
now. I appreciate your effort here and I’ll be happy to help you land
these once Mesa gains some form of usable support to allow testing.
Cheers,
Matt
[1]:
https://gitlab.freedesktop.org/imagination/linux/-/blob/b3506b8bc45ed6d4005eb32a994df0e33d6613f1/arch/arm64/boot/dts/mediatek/mt8173.dtsi#L993-1006
OpenPGP_signature.asc
Description: OpenPGP digital signature
Re: [PATCH v2] drm/imagination: DRM_POWERVR should depend on ARCH_K3
Hi Adam, On 19/02/2024 20:38, Adam Ford wrote: > On Mon, Feb 19, 2024 at 3:00 AM Matt Coster wrote: >> >> Hi Adam, >> >> On 18/02/2024 23:26, Adam Ford wrote: >>> On Fri, Feb 16, 2024 at 8:14 AM Maxime Ripard wrote: >>>> >>>> On Fri, Feb 16, 2024 at 09:13:14AM +, Biju Das wrote: >>>>> Hi Maxime Ripard, >>>>> >>>>>> -Original Message- >>>>>> From: Maxime Ripard >>>>>> Sent: Friday, February 16, 2024 9:05 AM >>>>>> Subject: Re: RE: [PATCH v2] drm/imagination: DRM_POWERVR should depend on >>>>>> ARCH_K3 >>>>>> >>>>>> On Fri, Feb 16, 2024 at 08:47:46AM +, Biju Das wrote: >>>>>>> Hi Adam Ford, >>>>>>> >>>>>>>> -Original Message- >>>>>>>> From: Adam Ford >>>>>>>> Sent: Thursday, February 15, 2024 11:36 PM >>>>>>>> Subject: Re: [PATCH v2] drm/imagination: DRM_POWERVR should depend >>>>>>>> on >>>>>>>> ARCH_K3 >>>>>>>> >>>>>>>> On Thu, Feb 15, 2024 at 11:22 AM Adam Ford wrote: >>>>>>>>> >>>>>>>>> On Thu, Feb 15, 2024 at 11:10 AM Adam Ford >>>>>> wrote: >>>>>>>>>> >>>>>>>>>> On Thu, Feb 15, 2024 at 10:54 AM Geert Uytterhoeven >>>>>>>>>> wrote: >>>>>>>>>>> >>>>>>>>>>> Hi Maxime, >>>>>>>>>>> >>>>>>>>>>> On Thu, Feb 15, 2024 at 5:18 PM Maxime Ripard >>>>>>>>>>> >>>>>>>> wrote: >>>>>>>>>>>> On Thu, Feb 15, 2024 at 01:50:09PM +0100, Geert Uytterhoeven >>>>>>>> wrote: >>>>>>>>>>>>> Using the Imagination Technologies PowerVR Series 6 GPU >>>>>>>>>>>>> requires a proprietary firmware image, which is currently >>>>>>>>>>>>> only available for Texas Instruments K3 AM62x SoCs. Hence >>>>>>>>>>>>> add a dependency on ARCH_K3, to prevent asking the user >>>>>>>>>>>>> about this driver when configuring a kernel without Texas >>>>>>>>>>>>> Instruments K3 >>>>>>>> Multicore SoC support. >>>>>>>>>>>> >>>>>>>>>>>> This wasn't making sense the first time you sent it, and now >>>>>>>>>>>> that commit log is just plain wrong. We have firmwares for >>>>>>>>>>>> the G6110, GX6250, GX6650, BXE-4-32, and BXS-4-64 models, >>>>>>>>>>>> which can be found on (at least) Renesas, Mediatek, >>>>>>>>>>>> Rockchip, TI and StarFive, so across three >>>>>>>>>>> >>>>>>>>>>> I am so happy to be proven wrong! >>>>>>>>>>> Yeah, GX6650 is found on e.g. R-Car H3, and GX6250 on e.g. >>>>>>>>>>> R-Car M3- >>>>>>>> W. >>>>>>>>>>> >>>>>>>>>>>> architectures and 5 platforms. In two months. >>>>>>>>>>> >>>>>>>>>>> That sounds like great progress, thanks a lot! >>>>>>>>>>> >>>>>>>>>> Geert, >>>>>>>>>> >>>>>>>>>>> Where can I find these firmwares? Linux-firmware[1] seems to >>>>>>>>>>> lack all but the original K3 AM62x one. >>>>>>>>>> >>>>>>>>>> I think PowerVR has a repo [1], but the last time I checked it, >>>>>>>>>> the BVNC for the firmware didn't match what was necessary for >>>>>>>>>> the GX6250 on the RZ/G2M. I can't remember what the >>>>>>>>>> corresponding R-Car3 model is. I haven't tried recently because >>>>>>>>>> I was told more documentation for firmware porting would be >>>>>>>>>> delayed until everything was pushed into the kernel and Mesa. >&g
Re: [PATCH v2] drm/imagination: DRM_POWERVR should depend on ARCH_K3
Hi Adam, On 19/02/2024 16:38, Adam Ford wrote: > On Mon, Feb 19, 2024 at 1:45 AM Biju Das wrote: >> >> Hi Adam, >> >>> -Original Message- >>> From: Adam Ford >>> Sent: Sunday, February 18, 2024 11:26 PM >>> Subject: Re: RE: RE: [PATCH v2] drm/imagination: DRM_POWERVR should depend >>> on ARCH_K3 >>> >>> On Fri, Feb 16, 2024 at 8:14 AM Maxime Ripard wrote: On Fri, Feb 16, 2024 at 09:13:14AM +, Biju Das wrote: > Hi Maxime Ripard, > >> -Original Message- >> From: Maxime Ripard >> Sent: Friday, February 16, 2024 9:05 AM >> Subject: Re: RE: [PATCH v2] drm/imagination: DRM_POWERVR should >> depend on >> ARCH_K3 >> >> On Fri, Feb 16, 2024 at 08:47:46AM +, Biju Das wrote: >>> Hi Adam Ford, >>> -Original Message- From: Adam Ford Sent: Thursday, February 15, 2024 11:36 PM Subject: Re: [PATCH v2] drm/imagination: DRM_POWERVR should depend on ARCH_K3 On Thu, Feb 15, 2024 at 11:22 AM Adam Ford >>> wrote: > > On Thu, Feb 15, 2024 at 11:10 AM Adam Ford > >> wrote: >> >> On Thu, Feb 15, 2024 at 10:54 AM Geert Uytterhoeven >> wrote: >>> >>> Hi Maxime, >>> >>> On Thu, Feb 15, 2024 at 5:18 PM Maxime Ripard >>> wrote: On Thu, Feb 15, 2024 at 01:50:09PM +0100, Geert Uytterhoeven wrote: > Using the Imagination Technologies PowerVR Series 6 > GPU requires a proprietary firmware image, which is > currently only available for Texas Instruments K3 > AM62x SoCs. Hence add a dependency on ARCH_K3, to > prevent asking the user about this driver when > configuring a kernel without Texas Instruments K3 Multicore SoC support. This wasn't making sense the first time you sent it, and now that commit log is just plain wrong. We have firmwares for the G6110, GX6250, GX6650, BXE-4-32, and BXS-4-64 models, which can be found on (at least) Renesas, Mediatek, Rockchip, TI and StarFive, so across three >>> >>> I am so happy to be proven wrong! >>> Yeah, GX6650 is found on e.g. R-Car H3, and GX6250 on e.g. >>> R-Car M3- W. >>> architectures and 5 platforms. In two months. >>> >>> That sounds like great progress, thanks a lot! >>> >> Geert, >> >>> Where can I find these firmwares? Linux-firmware[1] >>> seems to lack all but the original K3 AM62x one. >> >> I think PowerVR has a repo [1], but the last time I >> checked it, the BVNC for the firmware didn't match what >> was necessary for the GX6250 on the RZ/G2M. I can't >> remember what the corresponding R-Car3 model is. I >> haven't tried recently because I was told more >> documentation for firmware porting would be delayed until >>> everything was pushed into the kernel and Mesa. >> Maybe there is a better repo and/or newer firmware somewhere >>> else. >> > I should have doubled checked the repo contents before I > sent my last e-mail , but it appears the firmware [2] for > the RZ/G2M, might be present now. I don't know if there are > driver updates necessary. I checked my e-mails, but I didn't > see any notification, or I would have tried it earlier. > Either way, thank you Frank for adding it. I'll try to test >>> when I have some time. > I don't have the proper version of Mesa setup yet, but for what it's worth, the firmware loads without error, and it >>> doesn't hang. >>> >>> Based on [1] and [2], >>> >>> kmscube should work on R-Car as it works on RZ/G2L with panfrost >>> as earlier version of RZ/G2L which uses drm based on RCar-Du, >>> later changed >> to rzg2l-du. >> >> IIRC, the mesa support isn't there yet for kmscube to start. > > What about glmark2? I tested glmark2 as well. It's not really a matter of kmscube itself, but the interaction with the compositor entirely. You can run a headless vulkan rendering, but an application that renders to a window won't work. >>> >>> I have made a little progress. I have Ubuntu running on an RZ/G2M (Rogue >>> GX6250) with a device tree configuring the GPU and the GPU loads with >>> firmware. >>> >>> powervr fd00.gpu: [drm] loaded firmware >>> powervr/rogue_4.45.2.58_v1.fw >>> powervr fd00.gpu: [drm] FW version v1.0 (build 6513336 OS) >>> [drm] Initialized powervr 1.0.0 20230904 for fd00.gpu on minor 0 >>> >>> drmdevice lists card0 and renderD128 >>> --- Checking the number of DRM device available --- >>> --- Devices
Re: [PATCH v2] drm/imagination: DRM_POWERVR should depend on ARCH_K3
Hi Adam, On 18/02/2024 23:26, Adam Ford wrote: > On Fri, Feb 16, 2024 at 8:14 AM Maxime Ripard wrote: >> >> On Fri, Feb 16, 2024 at 09:13:14AM +, Biju Das wrote: >>> Hi Maxime Ripard, >>> -Original Message- From: Maxime Ripard Sent: Friday, February 16, 2024 9:05 AM Subject: Re: RE: [PATCH v2] drm/imagination: DRM_POWERVR should depend on ARCH_K3 On Fri, Feb 16, 2024 at 08:47:46AM +, Biju Das wrote: > Hi Adam Ford, > >> -Original Message- >> From: Adam Ford >> Sent: Thursday, February 15, 2024 11:36 PM >> Subject: Re: [PATCH v2] drm/imagination: DRM_POWERVR should depend >> on >> ARCH_K3 >> >> On Thu, Feb 15, 2024 at 11:22 AM Adam Ford wrote: >>> >>> On Thu, Feb 15, 2024 at 11:10 AM Adam Ford wrote: On Thu, Feb 15, 2024 at 10:54 AM Geert Uytterhoeven wrote: > > Hi Maxime, > > On Thu, Feb 15, 2024 at 5:18 PM Maxime Ripard > >> wrote: >> On Thu, Feb 15, 2024 at 01:50:09PM +0100, Geert Uytterhoeven >> wrote: >>> Using the Imagination Technologies PowerVR Series 6 GPU >>> requires a proprietary firmware image, which is currently >>> only available for Texas Instruments K3 AM62x SoCs. Hence >>> add a dependency on ARCH_K3, to prevent asking the user >>> about this driver when configuring a kernel without Texas >>> Instruments K3 >> Multicore SoC support. >> >> This wasn't making sense the first time you sent it, and now >> that commit log is just plain wrong. We have firmwares for >> the G6110, GX6250, GX6650, BXE-4-32, and BXS-4-64 models, >> which can be found on (at least) Renesas, Mediatek, >> Rockchip, TI and StarFive, so across three > > I am so happy to be proven wrong! > Yeah, GX6650 is found on e.g. R-Car H3, and GX6250 on e.g. > R-Car M3- >> W. > >> architectures and 5 platforms. In two months. > > That sounds like great progress, thanks a lot! > Geert, > Where can I find these firmwares? Linux-firmware[1] seems to > lack all but the original K3 AM62x one. I think PowerVR has a repo [1], but the last time I checked it, the BVNC for the firmware didn't match what was necessary for the GX6250 on the RZ/G2M. I can't remember what the corresponding R-Car3 model is. I haven't tried recently because I was told more documentation for firmware porting would be delayed until everything was pushed into the kernel and Mesa. Maybe there is a better repo and/or newer firmware somewhere else. >>> I should have doubled checked the repo contents before I sent my >>> last e-mail , but it appears the firmware [2] for the RZ/G2M, >>> might be present now. I don't know if there are driver updates >>> necessary. I checked my e-mails, but I didn't see any >>> notification, or I would have tried it earlier. Either way, thank >>> you Frank for adding it. I'll try to test when I have some time. >>> >> >> I don't have the proper version of Mesa setup yet, but for what it's >> worth, the firmware loads without error, and it doesn't hang. > > Based on [1] and [2], > > kmscube should work on R-Car as it works on RZ/G2L with panfrost as > earlier version of RZ/G2L which uses drm based on RCar-Du, later changed to rzg2l-du. IIRC, the mesa support isn't there yet for kmscube to start. >>> >>> What about glmark2? I tested glmark2 as well. >> >> It's not really a matter of kmscube itself, but the interaction with the >> compositor entirely. You can run a headless vulkan rendering, but an >> application that renders to a window won't work. > > I have made a little progress. I have Ubuntu running on an RZ/G2M > (Rogue GX6250) with a device tree configuring the GPU and the GPU > loads with firmware. > > powervr fd00.gpu: [drm] loaded firmware powervr/rogue_4.45.2.58_v1.fw > powervr fd00.gpu: [drm] FW version v1.0 (build 6513336 OS) > [drm] Initialized powervr 1.0.0 20230904 for fd00.gpu on minor 0 > > drmdevice lists card0 and renderD128 > --- Checking the number of DRM device available --- > --- Devices reported 2 --- > --- Retrieving devices information (PCI device revision is ignored) --- > device[0] > +-> available_nodes 0x05 > +-> nodes > | +-> nodes[0] /dev/dri/card0 > | +-> nodes[2] /dev/dri/renderD128 > +-> bustype 0002 > | +-> platform > | +-> fullname /soc/gpu@fd00 > +-> deviceinfo > +-> platform > +-> compatible > renesas,r8a774a1-gpu > img,img-axe > > There is more to this dump, but it seems to repeat. I wanted to show > that it seems like it's trying to work. > > I
Re: [PATCH] drm/imagination: On device loss, handle unplug after critical section
On 25/01/2024 18:44, Daniel Vetter wrote:
> On Tue, Jan 23, 2024 at 01:04:24PM +0000, Matt Coster wrote:
>> From: Donald Robson
>>
>> When the kernel driver 'loses' the device, for instance if the firmware
>> stops communicating, the driver calls drm_dev_unplug(). This is
>> currently done inside the drm_dev_enter() critical section, which isn't
>> permitted. In addition, the bool that marks the device as lost is not
>> atomic or protected by a lock.
>>
>> This fix replaces the bool with an atomic that also acts as a mechanism
>> to ensure the device is unplugged after drm_dev_exit(), preventing a
>> concurrent call to drm_dev_enter() from succeeding in a race between it
>> and drm_dev_unplug().
>
> Uh ... atomic_t does not make locking.
>
> From a quick look this entire thing smells a bit like bad design overall,
> and my gut feeling is that you probably want to rip out pvr_dev->lost
> outright. Or alternatively, explain what exactly this does beyond
> drm_dev_enter/exit, and then probably add that functionality there instead
> of hand-roll lockless trickery in drivers.
>
> From a quick look keeping track of where you realize the device is lost
> and then calling drm_dev_unplug after the drm_dev_exit is probably the
> clean solution. That also means the drm_dev_unplug() is not delayed due to
> a drm_dev_enter/exit section on a different thread, which is probably a
> good thing.
>
> Cheers, Sima
Hi Sima,
Thanks for taking the time to look over this patch.
This was the last piece of work Donald did for us at Imagination but he
never got a chance to send it out himself.
I'll put it on my list to try a new, more minimal, approach before
sending a v2. Your suggestion sounds very promising – that's probably
the first thing I'll try.
Cheers,
Matt
>> Reported-by: Steven Price
>> Closes:
>> https://lore.kernel.org/dri-devel/1b957ca4-71cf-42fd-ac81-1920592b9...@arm.com/
>> Fixes: cc1aeedb98ad ("drm/imagination: Implement firmware infrastructure and
>> META FW support")
>> Signed-off-by: Donald Robson
>> Signed-off-by: Matt Coster
>> ---
>> drivers/gpu/drm/imagination/pvr_ccb.c | 2 +-
>> drivers/gpu/drm/imagination/pvr_device.c | 98 +-
>> drivers/gpu/drm/imagination/pvr_device.h | 72 +---
>> drivers/gpu/drm/imagination/pvr_drv.c | 87 ++-
>> drivers/gpu/drm/imagination/pvr_fw.c | 12 +--
>> drivers/gpu/drm/imagination/pvr_fw_trace.c | 4 +-
>> drivers/gpu/drm/imagination/pvr_mmu.c | 20 ++---
>> drivers/gpu/drm/imagination/pvr_power.c| 42 +++---
>> drivers/gpu/drm/imagination/pvr_power.h| 2 -
>> 9 files changed, 237 insertions(+), 102 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/imagination/pvr_ccb.c
>> b/drivers/gpu/drm/imagination/pvr_ccb.c
>> index 4deeac7ed40a..1fe64adc0c2c 100644
>> --- a/drivers/gpu/drm/imagination/pvr_ccb.c
>> +++ b/drivers/gpu/drm/imagination/pvr_ccb.c
>> @@ -247,7 +247,7 @@ pvr_kccb_send_cmd_reserved_powered(struct pvr_device
>> *pvr_dev,
>> u32 old_write_offset;
>> u32 new_write_offset;
>>
>> -WARN_ON(pvr_dev->lost);
>> +WARN_ON(pvr_device_is_lost(pvr_dev));
>>
>> mutex_lock(_ccb->lock);
>>
>> diff --git a/drivers/gpu/drm/imagination/pvr_device.c
>> b/drivers/gpu/drm/imagination/pvr_device.c
>> index 1704c0268589..397491375b7d 100644
>> --- a/drivers/gpu/drm/imagination/pvr_device.c
>> +++ b/drivers/gpu/drm/imagination/pvr_device.c
>> @@ -6,14 +6,15 @@
>>
>> #include "pvr_fw.h"
>> #include "pvr_params.h"
>> -#include "pvr_power.h"
>> #include "pvr_queue.h"
>> #include "pvr_rogue_cr_defs.h"
>> #include "pvr_stream.h"
>> #include "pvr_vm.h"
>>
>> +#include
>> #include
>>
>> +#include
>> #include
>> #include
>> #include
>> @@ -556,6 +557,101 @@ pvr_device_fini(struct pvr_device *pvr_dev)
>> pvr_device_gpu_fini(pvr_dev);
>> }
>>
>> +/**
>> + * pvr_device_enter() - Try to enter device critical section.
>> + * @pvr_dev: Target PowerVR device.
>> + * @idx: Pointer to index that will be passed to the matching
>> pvr_device_exit().
>> + *
>> + * Use this in place of drm_dev_enter() within this driver.
>> + *
>> + * Returns:
>> + * * %true if the critical section was entered, or
>> + * * %false otherwise.
>> + */
>> +bool pvr_device_enter(struct pvr_device *pvr_dev, int *idx)
>&
[PATCH] drm/imagination: On device loss, handle unplug after critical section
From: Donald Robson
When the kernel driver 'loses' the device, for instance if the firmware
stops communicating, the driver calls drm_dev_unplug(). This is
currently done inside the drm_dev_enter() critical section, which isn't
permitted. In addition, the bool that marks the device as lost is not
atomic or protected by a lock.
This fix replaces the bool with an atomic that also acts as a mechanism
to ensure the device is unplugged after drm_dev_exit(), preventing a
concurrent call to drm_dev_enter() from succeeding in a race between it
and drm_dev_unplug().
Reported-by: Steven Price
Closes:
https://lore.kernel.org/dri-devel/1b957ca4-71cf-42fd-ac81-1920592b9...@arm.com/
Fixes: cc1aeedb98ad ("drm/imagination: Implement firmware infrastructure and
META FW support")
Signed-off-by: Donald Robson
Signed-off-by: Matt Coster
---
drivers/gpu/drm/imagination/pvr_ccb.c | 2 +-
drivers/gpu/drm/imagination/pvr_device.c | 98 +-
drivers/gpu/drm/imagination/pvr_device.h | 72 +---
drivers/gpu/drm/imagination/pvr_drv.c | 87 ++-
drivers/gpu/drm/imagination/pvr_fw.c | 12 +--
drivers/gpu/drm/imagination/pvr_fw_trace.c | 4 +-
drivers/gpu/drm/imagination/pvr_mmu.c | 20 ++---
drivers/gpu/drm/imagination/pvr_power.c| 42 +++---
drivers/gpu/drm/imagination/pvr_power.h| 2 -
9 files changed, 237 insertions(+), 102 deletions(-)
diff --git a/drivers/gpu/drm/imagination/pvr_ccb.c
b/drivers/gpu/drm/imagination/pvr_ccb.c
index 4deeac7ed40a..1fe64adc0c2c 100644
--- a/drivers/gpu/drm/imagination/pvr_ccb.c
+++ b/drivers/gpu/drm/imagination/pvr_ccb.c
@@ -247,7 +247,7 @@ pvr_kccb_send_cmd_reserved_powered(struct pvr_device
*pvr_dev,
u32 old_write_offset;
u32 new_write_offset;
- WARN_ON(pvr_dev->lost);
+ WARN_ON(pvr_device_is_lost(pvr_dev));
mutex_lock(_ccb->lock);
diff --git a/drivers/gpu/drm/imagination/pvr_device.c
b/drivers/gpu/drm/imagination/pvr_device.c
index 1704c0268589..397491375b7d 100644
--- a/drivers/gpu/drm/imagination/pvr_device.c
+++ b/drivers/gpu/drm/imagination/pvr_device.c
@@ -6,14 +6,15 @@
#include "pvr_fw.h"
#include "pvr_params.h"
-#include "pvr_power.h"
#include "pvr_queue.h"
#include "pvr_rogue_cr_defs.h"
#include "pvr_stream.h"
#include "pvr_vm.h"
+#include
#include
+#include
#include
#include
#include
@@ -556,6 +557,101 @@ pvr_device_fini(struct pvr_device *pvr_dev)
pvr_device_gpu_fini(pvr_dev);
}
+/**
+ * pvr_device_enter() - Try to enter device critical section.
+ * @pvr_dev: Target PowerVR device.
+ * @idx: Pointer to index that will be passed to the matching
pvr_device_exit().
+ *
+ * Use this in place of drm_dev_enter() within this driver.
+ *
+ * Returns:
+ * * %true if the critical section was entered, or
+ * * %false otherwise.
+ */
+bool pvr_device_enter(struct pvr_device *pvr_dev, int *idx)
+{
+ const enum pvr_device_state old_state =
+ atomic_cmpxchg(_dev->state,
+ PVR_DEVICE_STATE_PRESENT,
+ PVR_DEVICE_STATE_ENTERED);
+
+ switch (old_state) {
+ case PVR_DEVICE_STATE_PRESENT:
+ case PVR_DEVICE_STATE_ENTERED:
+ return drm_dev_enter(from_pvr_device(pvr_dev), idx);
+
+ case PVR_DEVICE_STATE_LOST:
+ case PVR_DEVICE_STATE_LOST_UNPLUGGED:
+ WARN_ONCE(1, "Attempt to use GPU after becoming lost.");
+ break;
+ }
+
+ return false;
+}
+
+/**
+ * pvr_device_exit() - Exit a device critical section.
+ * @pvr_dev: Target PowerVR device.
+ * @idx: Index given by matching pvr_device_enter().
+ *
+ * Use this in place of drm_dev_exit() within this driver.
+ */
+void pvr_device_exit(struct pvr_device *pvr_dev, int idx)
+{
+ const enum pvr_device_state old_state =
+ atomic_cmpxchg(_dev->state,
+ PVR_DEVICE_STATE_ENTERED,
+ PVR_DEVICE_STATE_PRESENT);
+
+ switch (old_state) {
+ case PVR_DEVICE_STATE_PRESENT:
+ case PVR_DEVICE_STATE_ENTERED:
+ drm_dev_exit(idx);
+ return;
+
+ case PVR_DEVICE_STATE_LOST:
+ /* Unplug the device if it was lost during the critical
section. */
+ atomic_set(_dev->state, PVR_DEVICE_STATE_LOST_UNPLUGGED);
+
+ drm_dev_exit(idx);
+
+ WARN_ONCE(1, "GPU lost and now unplugged.");
+ drm_dev_unplug(from_pvr_device(pvr_dev));
+
+ return;
+
+ case PVR_DEVICE_STATE_LOST_UNPLUGGED:
+ WARN_ONCE(1, "GPU unplugged during critical section.");
+ drm_dev_exit(idx);
+ return;
+ }
+}
+
+/**
+ * pvr_device_set_lost() - Mark GPU device as lost.
+ * @pvr_dev: Target PowerVR device.
+ *
+ * Th
Re: [PATCH] drm/imagination: fix ARRAY_SIZE build error
On 10/01/2024 00:23, Randy Dunlap wrote:
Fix a build error when using GCC 13.2.0 from kernel.org crosstools
by changing ARRAY_SIZE() to the macro PVR_MIPS_PT_PAGE_COUNT:
I assume you're referring to the x86_64 => aarch64 toolchain here?
drivers/gpu/drm/imagination/pvr_vm_mips.c: In function 'pvr_vm_mips_fini':
../include/linux/array_size.h:11:25: warning: overflow in conversion from 'long
unsigned int' to 'int' changes value from '18446744073709551615' to '-1'
[-Woverflow]
11 | #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) +
__must_be_array(arr))
| ^
drivers/gpu/drm/imagination/pvr_vm_mips.c:105:24: note: in expansion of macro
'ARRAY_SIZE'
105 | for (page_nr = ARRAY_SIZE(mips_data->pt_pages) - 1; page_nr >=
0; page_nr--) {
|^~
I can't seem to reproduce this using the above toolchain (or any other),
even with -Woverflow explicitly specified.
The use of ARRAY_SIZE() in loop bounds is a pretty common construction –
even within the pvr driver. Do you see similar warnings anywhere else?
Regards,
Matt
--
Matt Coster
Imagination Technologies
Fixes: 927f3e0253c1 ("drm/imagination: Implement MIPS firmware processor and MMU
support")
Signed-off-by: Randy Dunlap
Cc: Donald Robson
Cc: Maxime Ripard
Cc: Frank Binns
Cc: Matt Coster
Cc: Maarten Lankhorst
Cc: Thomas Zimmermann
Cc: David Airlie
Cc: Daniel Vetter
Cc: dri-devel@lists.freedesktop.org
---
drivers/gpu/drm/imagination/pvr_vm_mips.c |4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff -- a/drivers/gpu/drm/imagination/pvr_vm_mips.c
b/drivers/gpu/drm/imagination/pvr_vm_mips.c
--- a/drivers/gpu/drm/imagination/pvr_vm_mips.c
+++ b/drivers/gpu/drm/imagination/pvr_vm_mips.c
@@ -46,7 +46,7 @@ pvr_vm_mips_init(struct pvr_device *pvr_
if (!mips_data)
return -ENOMEM;
- for (page_nr = 0; page_nr < ARRAY_SIZE(mips_data->pt_pages); page_nr++) {
+ for (page_nr = 0; page_nr < PVR_MIPS_PT_PAGE_COUNT; page_nr++) {
mips_data->pt_pages[page_nr] = alloc_page(GFP_KERNEL |
__GFP_ZERO);
if (!mips_data->pt_pages[page_nr]) {
err = -ENOMEM;
@@ -102,7 +102,7 @@ pvr_vm_mips_fini(struct pvr_device *pvr_
int page_nr;
vunmap(mips_data->pt);
- for (page_nr = ARRAY_SIZE(mips_data->pt_pages) - 1; page_nr >= 0;
page_nr--) {
+ for (page_nr = PVR_MIPS_PT_PAGE_COUNT - 1; page_nr >= 0; page_nr--) {
dma_unmap_page(from_pvr_device(pvr_dev)->dev,
mips_data->pt_dma_addr[page_nr], PAGE_SIZE,
DMA_TO_DEVICE);
OpenPGP_0x747F0A9036F90DFA.asc
Description: OpenPGP public key
OpenPGP_signature.asc
Description: OpenPGP digital signature
