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 <[email protected]> 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/[email protected]?part=1
