Extension detection by enumeration vs attempt to use extension (Was: Re: [Intel-gfx] [PATCH v10 2/2] drm/i915: Allow user to set cache at BO creation)

[Joonas Lahtinen]

Quoting Jordan Justen (2023-05-21 07:30:52)
> On 2023-05-18 22:11:03,  wrote:
> > From: Fei Yang <fei.yang at intel.com>
> > 
> > To comply with the design that buffer objects shall have immutable
> > cache setting through out their life cycle, {set, get}_caching ioctl's
> > are no longer supported from MTL onward. With that change caching
> > policy can only be set at object creation time. The current code
> > applies a default (platform dependent) cache setting for all objects.
> > However this is not optimal for performance tuning. The patch extends
> > the existing gem_create uAPI to let user set PAT index for the object
> > at creation time.
> > The new extension is platform independent, so UMD's can switch to using
> > this extension for older platforms as well, while {set, get}_caching are
> > still supported on these legacy paltforms for compatibility reason.
> > 
> > Test igt at gem_create@create_ext_set_pat posted at
> > https://patchwork.freedesktop.org/series/117695/
> > 
> > Tested with https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/22878
> > 
> > Signed-off-by: Fei Yang <fei.yang at intel.com>
> > Cc: Chris Wilson <chris.p.wilson at linux.intel.com>
> > Cc: Matt Roper <matthew.d.roper at intel.com>
> > Cc: Andi Shyti <andi.shyti at linux.intel.com>
> > Reviewed-by: Andi Shyti <andi.shyti at linux.intel.com>
> > Acked-by: Jordan Justen <jordan.l.justen at intel.com>
> 
> Nevertheless, I'm still disappointed my suggestion was so quickly shot
> down.

Sorry to hear that you feel that your suggestion was shot down quickly.
The intent was not to be rude. Attempt was just to make sure we're not
blocking an important patch due to repeat of an orthogonal discussion
which has been discussed to exhaustion in past.

There are pros and cons to both solutions, some of which were recapped
in the thread quickly. We can surely continue the discussion on the
details now that the patch is not blocked.

> I tried to look over our usage Mesa of i915 extensions, and found
> this:
> 
> I915_GEM_CREATE_EXT_MEMORY_REGIONS:
> 
>  * If DRM_I915_QUERY_MEMORY_REGIONS is found
> 
> I915_GEM_CREATE_EXT_PROTECTED_CONTENT:
> 
>  * Probed via the current "robust" method. Resulted in 8s driver
>    startup delay in some bad scenarios.

I think this is an another orthogonal topic. Just listing the existence
of that extension in the kernel codebase would not really help.

The fact that an uAPI is known at compile time by kernel doesn't mean it
would not be defunctional / disabled / fused out on the specific system.

>  * Will be guarded by I915_PARAM_PXP_STATUS when available in future
> 
> I915_CONTEXT_CREATE_EXT_SETPARAM (I915_CONTEXT_PARAM_ENGINES):
> 
>  * If DRM_I915_QUERY_ENGINE_INFO is found
> 
> I915_GEM_CREATE_EXT_SET_PAT:
> 
>  * When platform is mtl or newer
> 
> I think we will continue to try to find workarounds that imply the
> extension's existence,

You're not supposed to just probe for existence of an extension in the
kernel codebase, but also check that the extension works on that system.

So probing for the extension array is just half the work. If the KMD
started to block out extensions from the array dynamically, then we
would be doing that based on adding heuristics that are better added in
the userspace. Ultimately you need to have the error handling for the
initialization added anyway to userspace, so there should not be that
much new code that needs adding.

Regards, Joonas

> but it could be nice to have a generic way to
> find out what extensions the kernel knows about.
> 
> -Jordan