Re: [PATCH v10] drm/i915/dg2: Add per-client control for Wa_22013059131

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] drm/i915: missing WA_22013059131 in ctx_setparam breaks uAPI for default context
--

commit 6bbdc7d4f28ec7ac3f465eca7548193f1bf2364e
Author: Jia Yao <jia.yao@intel.com>
Subject: drm/i915/dg2: Add per-client control for Wa_22013059131

This commit introduces a new context parameter I915_CONTEXT_PARAM_WA_22013059131
to allow userspace to handle the SLM contention workaround itself. It tracks the
opt-out per client and latches the value once before any context is submitted.

Link: https://github.com/intel/compute-runtime/pull/919

> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> index c58ffa5a8fa6f..2a9141e83936c 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
[ ... ]
> @@ -918,6 +941,15 @@ static int set_proto_ctx_param(struct drm_i915_file_private *fpriv,
>  			ret = -EINVAL;
>  		break;
>  
> +	case I915_CONTEXT_PARAM_WA_22013059131:
> +		if (args->size)
> +			ret = -EINVAL;
> +		else if (!IS_DG2_G11(i915))
> +			ret = -ENODEV;
> +		else
> +			ret = set_client_wa_22013059131(fpriv, args->value);
> +		break;
> +

[Severity: High]
Does this missing implementation in ctx_setparam() break the uAPI for the
default context?

The parameter is added here in set_proto_ctx_param(), but it appears to be
missing from ctx_setparam() and i915_gem_context_getparam_ioctl().

Because the default context (id 0) is fully finalized during
i915_gem_context_open(), it is not a proto-context. When userspace attempts
to set this parameter on ctx_id = 0, i915_gem_context_setparam_ioctl()
routes the call to ctx_setparam() instead:

i915_gem_context_setparam_ioctl()
   ctx = i915_gem_context_lookup(file_priv, args->ctx_id);
   if (ctx) {
       ret = ctx_setparam(file_priv, ctx, args);
       ...
   }

Since ctx_setparam() lacks this new case, it will fall through to the
-EINVAL default case, which prevents userspace from configuring this
workaround on the default context as documented.

Additionally, because it is missing from i915_gem_context_getparam_ioctl(),
the new UAPI parameter is strictly write-only and impossible to query.

[ ... ]
> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> index 535cb68fdb5c4..a9821e2cc3a26 100644
> --- a/include/uapi/drm/i915_drm.h
> +++ b/include/uapi/drm/i915_drm.h
[ ... ]
> + * any context of the client being submitted for the first time.  A later
> + * request for the opposite value fails with -EINVAL.  Userspace that wants
> + * to opt out must therefore do so before submitting any work on this fd,
> + * including via the default context (id 0) created on open().
> + *
> + * Note: LSC_CHICKEN_BIT_0_UDW MAXREQS_PER_BANK (bits 39:37) is the
> + * other part of Wa_22013059131 and remains unconditionally programmed
> + * by the kernel regardless of this setting.  DG2-G11 only.
> + */
> +#define I915_CONTEXT_PARAM_WA_22013059131	0x10

[Severity: Low]
Can this documentation be fulfilled if the parameter is rejected for the
default context?

The documentation states that userspace can opt out via the default context
(id 0) created on open, but because the parameter is only handled for
proto-contexts, that ioctl request will fail with -EINVAL.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260928031341.2958178-1-jia.yao@intel.com?part=1