* [PATCH] Revert "drm/imagination: Warn or error on unsupported hardware"
@ 2026-05-11 13:28 Geert Uytterhoeven
2026-05-11 14:05 ` Matt Coster
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Geert Uytterhoeven @ 2026-05-11 13:28 UTC (permalink / raw)
To: Frank Binns, Matt Coster, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Simona Vetter,
Greg Kroah-Hartman, Marek Vasut, Adam Ford
Cc: dri-devel, linux-renesas-soc, linux-kernel, Geert Uytterhoeven
Revert commit 1c21f240fbc1e47b94e68abfa2da2c01ed29a74d, as it stopped
the driver from working on various Renesas R-Car SoCs.
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
DT binding documentation updates were reviewed by the drm/imagination
maintainers[1][2][3], DTS additions were reviewed and/or acked by the
drm/imagination maintainers[4][5][6], and firmware is available[7].
Note that the GPU nodes were not enabled in board DTS files before, as
not having suitable firmware installed under /lib/firmware could trigger
a crash, not directly related to drm/imagination driver support. This
was fixed only recently in v7.1-rc3[8], so board enablement[9] is now
unblocked.
[1] commit 18ff1dc462ef6dac ("dt-bindings: gpu: img,powervr-rogue:
Document GX6250 GPU in Renesas R-Car M3-W/M3-W+")
[2] commit 6126a7f27f002408 ("dt-bindings: gpu: img,powervr-rogue:
Document GE7800 GPU in Renesas R-Car M3-N")
[3] commit 67549b73f10b8517 ("dt-bindings: gpu: img,powervr-rogue:
Document GE7800 GPU in Renesas R-Car V3U")
[4] commit 73100fa8e4ce21cc ("arm64: dts: renesas: r8a77960: Add GX6250
GPU node")
[5] commit 6e20a9d94a459b4e ("arm64: dts: renesas: r8a77961: Add GX6250
GPU node")
[6] commit 303a5185e024ee62 ("arm64: dts: renesas: r8a77965: Add GE7800
GPU node")
[7] https://gitlab.freedesktop.org/imagination/linux-firmware/-/tree/powervr/powervr
[8] commit 26735dfdd8930d9e ("pmdomain: core: Fix detach procedure for
virtual devices in genpd")
[9] https://lore.kernel.org/all/20251027211249.95826-1-marek.vasut+renesas@mailbox.org/
---
drivers/gpu/drm/imagination/pvr_device.c | 73 +-----------------------
1 file changed, 1 insertion(+), 72 deletions(-)
diff --git a/drivers/gpu/drm/imagination/pvr_device.c b/drivers/gpu/drm/imagination/pvr_device.c
index dbb6f5a8ded12a42..b7984563627de753 100644
--- a/drivers/gpu/drm/imagination/pvr_device.c
+++ b/drivers/gpu/drm/imagination/pvr_device.c
@@ -508,77 +508,6 @@ pvr_gpuid_decode_string(const struct pvr_device *pvr_dev,
}
EXPORT_SYMBOL_IF_KUNIT(pvr_gpuid_decode_string);
-static bool pvr_exp_hw_support;
-module_param_named(exp_hw_support, pvr_exp_hw_support, bool, 0600);
-MODULE_PARM_DESC(exp_hw_support, "Bypass runtime checks for fully supported GPU cores. WARNING: enabling this option may result in a buggy, insecure, or otherwise unusable driver.");
-
-/**
- * enum pvr_gpu_support_level - The level of support for a gpu_id in the current
- * version of the driver.
- *
- * @PVR_GPU_UNKNOWN: Cores that are unknown to the driver. These may not even exist.
- * @PVR_GPU_EXPERIMENTAL: Cores that have experimental support.
- * @PVR_GPU_SUPPORTED: Cores that are supported and maintained.
- */
-enum pvr_gpu_support_level {
- PVR_GPU_UNKNOWN,
- PVR_GPU_EXPERIMENTAL,
- PVR_GPU_SUPPORTED,
-};
-
-static enum pvr_gpu_support_level
-pvr_gpu_support_level(const struct pvr_gpu_id *gpu_id)
-{
- switch (pvr_gpu_id_to_packed_bvnc(gpu_id)) {
- case PVR_PACKED_BVNC(33, 15, 11, 3):
- case PVR_PACKED_BVNC(36, 53, 104, 796):
- return PVR_GPU_SUPPORTED;
-
- case PVR_PACKED_BVNC(36, 52, 104, 182):
- return PVR_GPU_EXPERIMENTAL;
-
- default:
- return PVR_GPU_UNKNOWN;
- }
-}
-
-static int
-pvr_check_gpu_supported(struct pvr_device *pvr_dev,
- const struct pvr_gpu_id *gpu_id)
-{
- struct drm_device *drm_dev = from_pvr_device(pvr_dev);
-
- switch (pvr_gpu_support_level(gpu_id)) {
- case PVR_GPU_SUPPORTED:
- if (pvr_exp_hw_support)
- drm_info(drm_dev, "Module parameter 'exp_hw_support' was set, but this hardware is fully supported by the current driver.");
-
- break;
-
- case PVR_GPU_EXPERIMENTAL:
- if (!pvr_exp_hw_support) {
- drm_err(drm_dev, "Unsupported GPU! Set 'exp_hw_support' to bypass this check.");
- return -ENODEV;
- }
-
- drm_warn(drm_dev, "Running on unsupported hardware; you may encounter bugs!");
- break;
-
- /* NOTE: This code path may indicate misbehaving hardware. */
- case PVR_GPU_UNKNOWN:
- default:
- if (!pvr_exp_hw_support) {
- drm_err(drm_dev, "Unknown GPU! Set 'exp_hw_support' to bypass this check.");
- return -ENODEV;
- }
-
- drm_warn(drm_dev, "Running on unknown hardware; expect issues.");
- break;
- }
-
- return 0;
-}
-
static char *pvr_gpuid_override;
module_param_named(gpuid, pvr_gpuid_override, charp, 0400);
MODULE_PARM_DESC(gpuid, "GPU ID (BVNC) to be used instead of the value read from hardware.");
@@ -609,7 +538,7 @@ pvr_load_gpu_id(struct pvr_device *pvr_dev)
return err;
}
- return pvr_check_gpu_supported(pvr_dev, gpu_id);
+ return 0;
}
/**
--
2.43.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] Revert "drm/imagination: Warn or error on unsupported hardware"
2026-05-11 13:28 [PATCH] Revert "drm/imagination: Warn or error on unsupported hardware" Geert Uytterhoeven
@ 2026-05-11 14:05 ` Matt Coster
2026-05-11 14:43 ` Geert Uytterhoeven
2026-05-16 5:12 ` Claude review: " Claude Code Review Bot
2026-05-16 5:12 ` Claude Code Review Bot
2 siblings, 1 reply; 6+ messages in thread
From: Matt Coster @ 2026-05-11 14:05 UTC (permalink / raw)
To: Geert Uytterhoeven, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Simona Vetter,
Greg Kroah-Hartman, Marek Vasut, Adam Ford
Cc: Frank Binns, Alessio Belle, Brajesh Gupta, Alexandru Dadu,
Luigi Santivetti, dri-devel@lists.freedesktop.org,
linux-renesas-soc@vger.kernel.org, linux-kernel@vger.kernel.org
[-- Attachment #1.1: Type: text/plain, Size: 7122 bytes --]
Hi Geert,
On 11/05/2026 14:28, Geert Uytterhoeven wrote:
> Revert commit 1c21f240fbc1e47b94e68abfa2da2c01ed29a74d, as it stopped
> the driver from working on various Renesas R-Car SoCs.
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> DT binding documentation updates were reviewed by the drm/imagination
> maintainers[1][2][3], DTS additions were reviewed and/or acked by the
> drm/imagination maintainers[4][5][6], and firmware is available[7].
> Note that the GPU nodes were not enabled in board DTS files before, as
> not having suitable firmware installed under /lib/firmware could trigger
> a crash, not directly related to drm/imagination driver support. This
> was fixed only recently in v7.1-rc3[8], so board enablement[9] is now
> unblocked.
We will freely acknowledge that the sequencing was not ideal here. This
patch should probably have been sent before we started accepting DTS
changes for those Renesas platforms. However, the purpose of this patch
still stands.
We're not saying we never want to list all these platforms as
"supported", but we don't want to mislead anyone into thinking the GPU
on these platforms will function in any meaningful way just because they
now have DTS nodes. We were originally convinved to allow these DTS
nodes to be added since it would facilitate active development on these
platforms, but this does not mean that we as a team have the bandwidth
to do that work ourselves at this time.
Our main concern is around the UAPI: we don't know for sure that support
for these platforms (which are significantly older than anything we
currently support) can be correctly implemented without UAPI changes. To
that end, we don't want to back ourselves into a corner where the UAPI
cannot be updated at a later date.
There's a similar mechanism in place in userspace: the user must set an
environment variable (PVR_I_WANT_A_BROKEN_VULKAN_DRIVER) to use
platforms for which we don't promise API conformance, but just like in
the kernel, this is not a compile time option and any user and/or
developer can enable it if they know what they're doing.
As for "it stopped the driver from working", no it didn't. The driver
never really worked on those platforms, at least not in any useful way,
and certainly not sufficiently for any non-developer user to benefit in
any way from it. The only change is that the user must now acknowledge
that this is the case to clarify that they shouldn't expect much (if
anything) to work. Just to be explicit, "firmware boots" is a loooooong
way from "ooh pretty triangles".
Would you prefer a different approach to providing this information to
users, perhaps a purely docs-based solution? I'm not convinced that
would be as effective at preserving our ability to mutate the UAPI for
these as-yet-unsupported platforms.
Cheers,
Matt
>
> [1] commit 18ff1dc462ef6dac ("dt-bindings: gpu: img,powervr-rogue:
> Document GX6250 GPU in Renesas R-Car M3-W/M3-W+")
> [2] commit 6126a7f27f002408 ("dt-bindings: gpu: img,powervr-rogue:
> Document GE7800 GPU in Renesas R-Car M3-N")
> [3] commit 67549b73f10b8517 ("dt-bindings: gpu: img,powervr-rogue:
> Document GE7800 GPU in Renesas R-Car V3U")
> [4] commit 73100fa8e4ce21cc ("arm64: dts: renesas: r8a77960: Add GX6250
> GPU node")
> [5] commit 6e20a9d94a459b4e ("arm64: dts: renesas: r8a77961: Add GX6250
> GPU node")
> [6] commit 303a5185e024ee62 ("arm64: dts: renesas: r8a77965: Add GE7800
> GPU node")
> [7] https://gitlab.freedesktop.org/imagination/linux-firmware/-/tree/powervr/powervr
> [8] commit 26735dfdd8930d9e ("pmdomain: core: Fix detach procedure for
> virtual devices in genpd")
> [9] https://lore.kernel.org/all/20251027211249.95826-1-marek.vasut+renesas@mailbox.org/
> ---
> drivers/gpu/drm/imagination/pvr_device.c | 73 +-----------------------
> 1 file changed, 1 insertion(+), 72 deletions(-)
>
> diff --git a/drivers/gpu/drm/imagination/pvr_device.c b/drivers/gpu/drm/imagination/pvr_device.c
> index dbb6f5a8ded12a42..b7984563627de753 100644
> --- a/drivers/gpu/drm/imagination/pvr_device.c
> +++ b/drivers/gpu/drm/imagination/pvr_device.c
> @@ -508,77 +508,6 @@ pvr_gpuid_decode_string(const struct pvr_device *pvr_dev,
> }
> EXPORT_SYMBOL_IF_KUNIT(pvr_gpuid_decode_string);
>
> -static bool pvr_exp_hw_support;
> -module_param_named(exp_hw_support, pvr_exp_hw_support, bool, 0600);
> -MODULE_PARM_DESC(exp_hw_support, "Bypass runtime checks for fully supported GPU cores. WARNING: enabling this option may result in a buggy, insecure, or otherwise unusable driver.");
> -
> -/**
> - * enum pvr_gpu_support_level - The level of support for a gpu_id in the current
> - * version of the driver.
> - *
> - * @PVR_GPU_UNKNOWN: Cores that are unknown to the driver. These may not even exist.
> - * @PVR_GPU_EXPERIMENTAL: Cores that have experimental support.
> - * @PVR_GPU_SUPPORTED: Cores that are supported and maintained.
> - */
> -enum pvr_gpu_support_level {
> - PVR_GPU_UNKNOWN,
> - PVR_GPU_EXPERIMENTAL,
> - PVR_GPU_SUPPORTED,
> -};
> -
> -static enum pvr_gpu_support_level
> -pvr_gpu_support_level(const struct pvr_gpu_id *gpu_id)
> -{
> - switch (pvr_gpu_id_to_packed_bvnc(gpu_id)) {
> - case PVR_PACKED_BVNC(33, 15, 11, 3):
> - case PVR_PACKED_BVNC(36, 53, 104, 796):
> - return PVR_GPU_SUPPORTED;
> -
> - case PVR_PACKED_BVNC(36, 52, 104, 182):
> - return PVR_GPU_EXPERIMENTAL;
> -
> - default:
> - return PVR_GPU_UNKNOWN;
> - }
> -}
> -
> -static int
> -pvr_check_gpu_supported(struct pvr_device *pvr_dev,
> - const struct pvr_gpu_id *gpu_id)
> -{
> - struct drm_device *drm_dev = from_pvr_device(pvr_dev);
> -
> - switch (pvr_gpu_support_level(gpu_id)) {
> - case PVR_GPU_SUPPORTED:
> - if (pvr_exp_hw_support)
> - drm_info(drm_dev, "Module parameter 'exp_hw_support' was set, but this hardware is fully supported by the current driver.");
> -
> - break;
> -
> - case PVR_GPU_EXPERIMENTAL:
> - if (!pvr_exp_hw_support) {
> - drm_err(drm_dev, "Unsupported GPU! Set 'exp_hw_support' to bypass this check.");
> - return -ENODEV;
> - }
> -
> - drm_warn(drm_dev, "Running on unsupported hardware; you may encounter bugs!");
> - break;
> -
> - /* NOTE: This code path may indicate misbehaving hardware. */
> - case PVR_GPU_UNKNOWN:
> - default:
> - if (!pvr_exp_hw_support) {
> - drm_err(drm_dev, "Unknown GPU! Set 'exp_hw_support' to bypass this check.");
> - return -ENODEV;
> - }
> -
> - drm_warn(drm_dev, "Running on unknown hardware; expect issues.");
> - break;
> - }
> -
> - return 0;
> -}
> -
> static char *pvr_gpuid_override;
> module_param_named(gpuid, pvr_gpuid_override, charp, 0400);
> MODULE_PARM_DESC(gpuid, "GPU ID (BVNC) to be used instead of the value read from hardware.");
> @@ -609,7 +538,7 @@ pvr_load_gpu_id(struct pvr_device *pvr_dev)
> return err;
> }
>
> - return pvr_check_gpu_supported(pvr_dev, gpu_id);
> + return 0;
> }
>
> /**
--
Matt Coster
E: matt.coster@imgtec.com
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Revert "drm/imagination: Warn or error on unsupported hardware"
2026-05-11 14:05 ` Matt Coster
@ 2026-05-11 14:43 ` Geert Uytterhoeven
2026-05-12 10:00 ` Matt Coster
0 siblings, 1 reply; 6+ messages in thread
From: Geert Uytterhoeven @ 2026-05-11 14:43 UTC (permalink / raw)
To: Matt Coster
Cc: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie,
Simona Vetter, Greg Kroah-Hartman, Marek Vasut, Adam Ford,
Frank Binns, Alessio Belle, Brajesh Gupta, Alexandru Dadu,
Luigi Santivetti, dri-devel@lists.freedesktop.org,
linux-renesas-soc@vger.kernel.org, linux-kernel@vger.kernel.org
Hi Matt,
On Mon, 11 May 2026 at 16:06, Matt Coster <Matt.Coster@imgtec.com> wrote:
> On 11/05/2026 14:28, Geert Uytterhoeven wrote:
> > Revert commit 1c21f240fbc1e47b94e68abfa2da2c01ed29a74d, as it stopped
> > the driver from working on various Renesas R-Car SoCs.
> >
> > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > ---
> > DT binding documentation updates were reviewed by the drm/imagination
> > maintainers[1][2][3], DTS additions were reviewed and/or acked by the
> > drm/imagination maintainers[4][5][6], and firmware is available[7].
> > Note that the GPU nodes were not enabled in board DTS files before, as
> > not having suitable firmware installed under /lib/firmware could trigger
> > a crash, not directly related to drm/imagination driver support. This
> > was fixed only recently in v7.1-rc3[8], so board enablement[9] is now
> > unblocked.
>
> We will freely acknowledge that the sequencing was not ideal here. This
> patch should probably have been sent before we started accepting DTS
> changes for those Renesas platforms. However, the purpose of this patch
> still stands.
>
> We're not saying we never want to list all these platforms as
> "supported", but we don't want to mislead anyone into thinking the GPU
> on these platforms will function in any meaningful way just because they
> now have DTS nodes. We were originally convinved to allow these DTS
> nodes to be added since it would facilitate active development on these
> platforms, but this does not mean that we as a team have the bandwidth
> to do that work ourselves at this time.
>
> Our main concern is around the UAPI: we don't know for sure that support
> for these platforms (which are significantly older than anything we
> currently support) can be correctly implemented without UAPI changes. To
> that end, we don't want to back ourselves into a corner where the UAPI
> cannot be updated at a later date.
Automotive life cycles are long...
> There's a similar mechanism in place in userspace: the user must set an
> environment variable (PVR_I_WANT_A_BROKEN_VULKAN_DRIVER) to use
> platforms for which we don't promise API conformance, but just like in
> the kernel, this is not a compile time option and any user and/or
> developer can enable it if they know what they're doing.
>
> As for "it stopped the driver from working", no it didn't. The driver
> never really worked on those platforms, at least not in any useful way,
> and certainly not sufficiently for any non-developer user to benefit in
> any way from it. The only change is that the user must now acknowledge
> that this is the case to clarify that they shouldn't expect much (if
> anything) to work. Just to be explicit, "firmware boots" is a loooooong
> way from "ooh pretty triangles".
AFAIK, it's working better than just "pretty triangles", e.g. glxgears.
And people are working on support for more SoCs (both newer and older),
for which patches (both Linux kernel and MESA) have been posted...
https://gitlab.freedesktop.org/imagination/linux-firmware/-/work_items/13
> Would you prefer a different approach to providing this information to
> users, perhaps a purely docs-based solution? I'm not convinced that
> would be as effective at preserving our ability to mutate the UAPI for
> these as-yet-unsupported platforms.
One can wonder if it's the kernel's job to block the use of this
hardware by default?
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Revert "drm/imagination: Warn or error on unsupported hardware"
2026-05-11 14:43 ` Geert Uytterhoeven
@ 2026-05-12 10:00 ` Matt Coster
0 siblings, 0 replies; 6+ messages in thread
From: Matt Coster @ 2026-05-12 10:00 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie,
Simona Vetter, Greg Kroah-Hartman, Marek Vasut, Adam Ford,
Frank Binns, Alessio Belle, Brajesh Gupta, Alexandru Dadu,
Luigi Santivetti, dri-devel@lists.freedesktop.org,
linux-renesas-soc@vger.kernel.org, linux-kernel@vger.kernel.org
[-- Attachment #1.1: Type: text/plain, Size: 5306 bytes --]
Hi Geert,
On 11/05/2026 15:43, Geert Uytterhoeven wrote:
> On Mon, 11 May 2026 at 16:06, Matt Coster <Matt.Coster@imgtec.com> wrote:
>> On 11/05/2026 14:28, Geert Uytterhoeven wrote:
>>> Revert commit 1c21f240fbc1e47b94e68abfa2da2c01ed29a74d, as it stopped
>>> the driver from working on various Renesas R-Car SoCs.
>>>
>>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>>> ---
>>> DT binding documentation updates were reviewed by the drm/imagination
>>> maintainers[1][2][3], DTS additions were reviewed and/or acked by the
>>> drm/imagination maintainers[4][5][6], and firmware is available[7].
>>> Note that the GPU nodes were not enabled in board DTS files before, as
>>> not having suitable firmware installed under /lib/firmware could trigger
>>> a crash, not directly related to drm/imagination driver support. This
>>> was fixed only recently in v7.1-rc3[8], so board enablement[9] is now
>>> unblocked.
>>
>> We will freely acknowledge that the sequencing was not ideal here. This
>> patch should probably have been sent before we started accepting DTS
>> changes for those Renesas platforms. However, the purpose of this patch
>> still stands.
>>
>> We're not saying we never want to list all these platforms as
>> "supported", but we don't want to mislead anyone into thinking the GPU
>> on these platforms will function in any meaningful way just because they
>> now have DTS nodes. We were originally convinved to allow these DTS
>> nodes to be added since it would facilitate active development on these
>> platforms, but this does not mean that we as a team have the bandwidth
>> to do that work ourselves at this time.
>>
>> Our main concern is around the UAPI: we don't know for sure that support
>> for these platforms (which are significantly older than anything we
>> currently support) can be correctly implemented without UAPI changes. To
>> that end, we don't want to back ourselves into a corner where the UAPI
>> cannot be updated at a later date.
>
> Automotive life cycles are long...
Which is why those cores (that are substantially older than anything
we've been targetting so far) are still relevant. But due to that age,
they're going to have quirks (and often just different hardware blocks)
that we haven't considered yet.
>
>> There's a similar mechanism in place in userspace: the user must set an
>> environment variable (PVR_I_WANT_A_BROKEN_VULKAN_DRIVER) to use
>> platforms for which we don't promise API conformance, but just like in
>> the kernel, this is not a compile time option and any user and/or
>> developer can enable it if they know what they're doing.
>>
>> As for "it stopped the driver from working", no it didn't. The driver
>> never really worked on those platforms, at least not in any useful way,
>> and certainly not sufficiently for any non-developer user to benefit in
>> any way from it. The only change is that the user must now acknowledge
>> that this is the case to clarify that they shouldn't expect much (if
>> anything) to work. Just to be explicit, "firmware boots" is a loooooong
>> way from "ooh pretty triangles".
>
> AFAIK, it's working better than just "pretty triangles", e.g. glxgears.
> And people are working on support for more SoCs (both newer and older),
> for which patches (both Linux kernel and MESA) have been posted...
>
> https://gitlab.freedesktop.org/imagination/linux-firmware/-/work_items/13
This is great! But the patches that have been posted are almost
exclusively just surface level enablement (adding feature tables,
providing firmware blobs, allow-listing compatible strings, etc.), much
like the DTS changes made on the kernel side.
>
>> Would you prefer a different approach to providing this information to
>> users, perhaps a purely docs-based solution? I'm not convinced that
>> would be as effective at preserving our ability to mutate the UAPI for
>> these as-yet-unsupported platforms.
>
> One can wonder if it's the kernel's job to block the use of this
> hardware by default?
Our logic was this:
1. The kernel requires absolute stability in the UAPI
2. These cores have yet to be tested sufficiently to be confident that
the current UAPI can accomodate them
3. Don't allow these cores to be used by default so the UAPI remains
somewhat malleable on platforms without this confidence.
When we were first looking to send the powervr driver upstream, we were
given a watershed of ~70% Vulkan 1.0 conformance passing to ensure the
UAPI was _probably_ solid enough to be frozen as-is. These cores are
currently nowhere near that mark.
If we can remove these restrictions now and come back later to update
the UAPI _in a way that "breaks" these cores_ (but not really because
they don't work fully yet), then I could go for that; my understanding
is that this is not an option.
We absolutely agree that it's in everyone's interest to have
experimental support in the upstream driver rather than rotting in
downstream forks, but we need to ensure that this is done in a way that
does not lock down the UAPI in a shape that may not actually be useful
for the cores with only experimental support.
Cheers,
Matt
--
Matt Coster
E: matt.coster@imgtec.com
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Claude review: Revert "drm/imagination: Warn or error on unsupported hardware"
2026-05-11 13:28 [PATCH] Revert "drm/imagination: Warn or error on unsupported hardware" Geert Uytterhoeven
2026-05-11 14:05 ` Matt Coster
@ 2026-05-16 5:12 ` Claude Code Review Bot
2026-05-16 5:12 ` Claude Code Review Bot
2 siblings, 0 replies; 6+ messages in thread
From: Claude Code Review Bot @ 2026-05-16 5:12 UTC (permalink / raw)
To: dri-devel-reviews
Overall Series Review
Subject: Revert "drm/imagination: Warn or error on unsupported hardware"
Author: Geert Uytterhoeven <geert+renesas@glider.be>
Patches: 4
Reviewed: 2026-05-16T15:12:53.518717
---
This is a single patch that reverts commit `1c21f240fbc1e47b` ("drm/imagination: Warn or error on unsupported hardware"). That commit introduced a GPU support allowlist that blocks driver probing on GPU cores not explicitly listed as `PVR_GPU_SUPPORTED` or `PVR_GPU_EXPERIMENTAL`, unless the user sets the `exp_hw_support` module parameter.
The revert is motivated by the fact that Renesas R-Car SoCs (which have PowerVR Rogue GPUs like GX6250 and GE7800) are blocked by this allowlist — their BVNC values are not in the switch statement, so they fall into `PVR_GPU_UNKNOWN` and the driver refuses to probe with `-ENODEV`.
**Assessment:** The patch is technically correct as a revert — it cleanly removes the support-level checking infrastructure and replaces the call with `return 0`. The commit message is well-documented with extensive references to DT bindings, DTS additions, firmware availability, and the power domain fix that unblocked board enablement.
However, the better fix is arguably **not** a full revert but rather adding the Renesas GPU BVNC values to the allowlist. The support-level mechanism was intentionally introduced as a safety gate for the early-stage driver. Reverting it entirely removes protection for truly unknown/untested hardware configurations. That said, the commit message makes a valid argument that the allowlist approach creates ongoing friction for new platform enablement — every new SoC with a PowerVR GPU must also have its BVNC added to the driver's allowlist, which is maintenance burden that doesn't exist in other DRM drivers.
**Recommendation:** This is a policy/design decision for the drm/imagination maintainers. The patch is correct in what it does, but the maintainers should weigh whether to accept the full revert vs. expanding the allowlist. The author has clearly thought about this and chosen the revert deliberately.
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 6+ messages in thread
* Claude review: Revert "drm/imagination: Warn or error on unsupported hardware"
2026-05-11 13:28 [PATCH] Revert "drm/imagination: Warn or error on unsupported hardware" Geert Uytterhoeven
2026-05-11 14:05 ` Matt Coster
2026-05-16 5:12 ` Claude review: " Claude Code Review Bot
@ 2026-05-16 5:12 ` Claude Code Review Bot
2 siblings, 0 replies; 6+ messages in thread
From: Claude Code Review Bot @ 2026-05-16 5:12 UTC (permalink / raw)
To: dri-devel-reviews
Patch Review
**Correctness:** The revert is clean. It removes:
1. The `pvr_exp_hw_support` module parameter and its `module_param_named`/`MODULE_PARM_DESC` declarations
2. The `enum pvr_gpu_support_level` type and its three values
3. The `pvr_gpu_support_level()` function with the BVNC-to-support-level mapping
4. The `pvr_check_gpu_supported()` function with the support-level gating logic
5. Replaces `return pvr_check_gpu_supported(pvr_dev, gpu_id);` with `return 0;`
All removed code is dead after the change — no other callers or references remain.
**Observations:**
1. **No `Fixes:` tag:** The patch doesn't include a `Fixes:` tag pointing to the original commit `1c21f240fbc1e47b`. While this is a revert (not strictly a "fix"), if the intent is for the revert to be backported to stable kernels (since the original commit broke Renesas SoC support), a `Fixes:` tag would be appropriate to signal that. Without it, stable maintainers won't pick it up automatically.
2. **Well-documented rationale:** The below-`---` section is thorough, with 9 references justifying the revert. This is excellent for reviewer context but won't appear in the final git log (which is the right place for it, since it's background justification rather than core commit message content).
3. **Alternative approach not discussed in commit message body:** The commit message body (above the `---`) is very terse — just "Revert commit X, as it stopped the driver from working on various Renesas R-Car SoCs." It doesn't explain why a revert was chosen over adding the BVNC values. A brief note on that design choice would strengthen the commit message for future readers.
4. **Module parameter removal is an ABI change:** Removing the `exp_hw_support` module parameter is technically a user-visible ABI change. Any existing users who set this parameter in their module configuration (e.g., in `/etc/modprobe.d/`) would see a warning about an unknown parameter on the next boot after this patch lands. This is minor but worth noting.
5. **Diff context matches tree:** The removed code at lines 511–580 of the current tree matches the patch's diff context exactly, confirming this is a clean revert against the expected base.
**Verdict:** The patch is technically sound. The main question is whether the drm/imagination maintainers prefer a full revert or would rather have the Renesas BVNCs added to the allowlist. Both approaches are valid; the full revert is simpler and avoids future churn as new SoCs are enabled, but loses the safety net for truly unknown hardware.
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2026-05-16 5:12 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-05-11 13:28 [PATCH] Revert "drm/imagination: Warn or error on unsupported hardware" Geert Uytterhoeven
2026-05-11 14:05 ` Matt Coster
2026-05-11 14:43 ` Geert Uytterhoeven
2026-05-12 10:00 ` Matt Coster
2026-05-16 5:12 ` Claude review: " Claude Code Review Bot
2026-05-16 5:12 ` Claude Code Review Bot
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox