Re: [PATCH] Revert "drm/imagination: Warn or error on unsupported hardware"

[Matt Coster <Matt.Coster@imgtec.com>]

[-- 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 --]