IOCTL feature detection (Was: Re: [Intel-gfx] [PATCH 8/8] drm/i915: Allow user to set cache at BO creation)

[Daniel Vetter]

On Tue, Apr 25, 2023 at 04:41:54PM +0300, Joonas Lahtinen wrote:
> (+ Faith and Daniel as they have been involved in previous discussions)
> 
> Quoting Jordan Justen (2023-04-24 20:13:00)
> > On 2023-04-24 02:08:43, Tvrtko Ursulin wrote:
> > > 
> > > Being able to "list" supported extensions sounds like a reasonable
> > > principle, albeit a departure from the design direction to date.
> > > Which means there are probably no quick solutions. Also, AFAIU, only
> > > PXP context create is the problematic one, right? Everything else is
> > > pretty much instant or delayed allocation so super cheap to probe by
> > > attempting to use.
> > > 
> > > If I got that right and given this series is about
> > > drm_i915_gem_create_ext I don't think this side discussion should be
> > > blocking it.
> > 
> > This still leaves the issue of no reasonable detection mechanism for
> > the extension.
> 
> I remember this exact discussion being repeated at least a few times in
> the past 5 years. Previously the conclusion was always that detection by
> trying to use the extension leads to least amount of uAPI surface. There
> is also no concern of having mismatch in backporting of the query and the
> functionality patches.
> 
> Why exactly do you think it is more reasonable to do the following?
> 
> check_if_extension_query_is_supported();
> <check retval>
> check_if_extension_xyz_is_supported();
> <check retval>
> 
> VS
> 
> create_[context,buffer,whatever]_with_extension();
> <check errno>
> destroy_[context,buffer,whatever]();
> 
> For contexts and buffers there's no overhead, and we're keeping the uAPI
> surface smaller (less bugs, less validation effort). Additionally we
> support checking combinations of extensions A, B and C without extra
> effort.
> 
> If we're not returning enough clear errnos, then that is something to
> make sure we do.

Joonas asked me to put my thoughts here:

- in general the "feature discovery by trying it out" approach is most
  robust and hence preferred, but it's also not something that's required
  when there's good reasons against it

- the more a feature spans drivers/modules, the more it should be
  discovered by trying it out, e.g. dma-buf fence import/export was a huge
  discussion, luckily mesa devs figured out how to transparantly fall back
  at runtime so we didn't end up merging the separate feature flag (I
  think at least, can't find it). pxp being split across i915/me/fw/who
  knows what else is kinda similar so I'd heavily lean towards discovery
  by creating a context

- pxp taking 8s to init a ctx sounds very broken, irrespective of anything
  else

- in practice there's not really a combinatorial explosion, for a lot of
  reasons:
  - kernel and userspace tend to assume/require implied features where it
    makes sense, e.g. in kms atomic implies planes and universal planes.
    mesa has been doing the same
  - you cold go all the way to what radeon/amdgpu have done for years with
    a single incrementing version. Probably not flexible enough for intel.
  - every pciid is it's own uapi, so a feature only needs to be tested on
    platforms where it didn't ship from the start. Also cuts down on
    runtime discovery a lot
  - mesa tends to only support down to current lts kernels and not older,
    which again cuts the combinations a lot

- I did look through upstream kernel docs for general (driver) uapi
  recommendations, but there isn't really anything about good taste stuff,
  just a lot about not screwing up compatibility across kernels/platforms.

tldr; prefer discovery, don't sweat it if not, definitely don't
overengineer this with some magic "give me all extensions" thing because
that results in guaranteed cross-component backporting pains sooner or
later. Or inconsistency, which defeats the point.

Cheers, Daniel
 
> Regards, Joonas
> 
> > If the discussion gets too complicated, then can we add
> > a GET_PARAM for the SET_PAT extension? I'm hoping we could either come
> > up with something better reasonably quickly, or i915/Xe can add a new
> > param for each new extensions until a better approach is available.
> > 
> > > Furthermore the PXP context create story is even more complicated,
> > > in a way that it is not just about querying whether the extension is
> > > supported, but the expensive check is something more complicated.
> > > 
> > > Going back to implementation details for this proposed new feature,
> > > one alternative to query could be something like:
> > > 
> > >    drm_i915_gem_create_ext.flags |= I915_GEM_CREATE_EXT_FLAG_PROBE_EXTENSIONS;
> > > 
> > > That would be somewhat more light weight to implement that the
> > > i915_query route. And it appears it would work for all ioctls which
> > > support extensions apart for i915_context_param_engines.
> > 
> > This seems little better than the "try it, and if it works then it's
> > supported".
> > 
> > I'm not suggesting that userspace should be able to check that
> > scenario x+y+z will work, but more a list of extensions that
> > conceivably could work. Normally this should just a matter of the
> > kernel unconditionally adding the newly implemented extension to the
> > list returned in the query call.
> > 
> > If a GET_PARAM can be made for the PXP case, then it seems like a
> > query item returning CONTEXT_CREATE extensions could conditionally
> > omit that extension just as easily as implementing the proposed new
> > GET_PARAM.
> > 
> > -Jordan

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch