[Intel-gfx] [PATCH v11 5/6] drm/i915: add query uAPI

[Lionel Landwerlin]

On 24/01/18 15:14, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2018-01-24 12:03:46)
>> On 23/01/2018 14:17, Lionel Landwerlin wrote:
>>> Hi all,
>>>
>>> I've been trying to expose some information to userspace about the fused
>>> parts of the GPU.
>>> This is the 4th attempt at getting this upstream, here are the previous
>>> ones :
>>>       https://patchwork.freedesktop.org/patch/185959/
>>>       https://patchwork.freedesktop.org/series/33436/
>>>       https://patchwork.freedesktop.org/series/33950/
>>>
>>> This last iteration was based upon some direction by Daniel :
>>> https://lists.freedesktop.org/archives/dri-devel/2017-December/160162.html
>>> There was, I think, a fair point about having this working in
>>> environments where sysfs (mechanism used in the 3rd iteration) is not
>>> available (containers).
>>>
>>> Following some discussion on IRC, it seems Joonas would like this
>>> rewritten in a such way that we essentially drop the generic mechanism
>>> introduced in this patch, and instead go for an additional ioctl() on
>>> the drm fd just for querying the state of a fused part of
>>> slice/subslice/eus.
>>> The proposal is to have a single struct like :
>>>
>>> struct drm_i915_topology {
>>>      /* All field are in/out */
>>>      int slice;
>>>      int subslice;
>>>      int eu;
>>>
>>>      int enabled;
>>> };
>>>
>>> You would let the slice field to -1 and then the kernel would fill it
>>> with the max slice value. Same for subslice (with a valid slice value)
>>> and eu.
>>> When querying with slice = 0, and all other fields to -1, the kernel
>>> would fill the enabled value with 0 or 1.
>>> Essentially that would mean that an application wanting to query the
>>> state of all of the EUs would have to go through them one by one (which
>>> would be about ~100 ioctl() on SKL GT4 for example).
>>>
>>> Apart from the fact that we'll probably end up adding another ioctl()
>>> for engine discovery, I don't have any problem with what Joonas is
>>> proposing.
>>> It's just a bit annoying this comes up on the 4th rewrite.
>>> I really wouldn't like to rewrite this one more time and get turned down
>>> because this isn't to the taste of one of the reviewer.
>>> So my question is : Is everybody happy with what Joonas is proposing?
>>> Anybody in favor of having a generic mechanism?
>> I am not very keen on this counter-proposal for two reasons.
>>
>> First is that I think is a bit inelegant to have to query so many times
>> just to get the full topology out. If this ends in some library, we may
>> end up running this on every trivial client startup.
>>
>> Secondly, it is kind of dispatcher in it's own right. Since the
>> operation mode will depend on the combination of field values. As such a
>> generic, or at least a more explicit, dispatcher, like the proposed
>> i915_query_ioctl sounds cleaner to me.
>>
>> I take the point we can't guess how many other users we will have for it
>> in the future. So there is a little bit of a risk of adding something
>> generic which won't be used a lot in the future.
>>
>> Because apart from the three queries Lionel needs, I would be adding an
>> engine info query and potentially, depending on userspace interest,
>> engine queue depths. So that would be a maximum of five queries I am
>> aware of would use the generic framework. Maybe too little, or maybe
>> good enough for a start?
> Another use case would be a single shot method to gather all GETPARAMs.
>
> There's a lot of too'ing and fro'ing at the start of mesa trying to
> determine all of the kernel's capabilities, which more or less come down
> to a long series of parsing GETPARAM results. Bundling all of those up
> into a single ioctl seems attractive to me (bonus for it being properly
> defined and not a compat nightmare).
> -Chris
>
Thanks all,

I don't really read much opposition to the current patch series. If 
anything we could actually want to do more it seems.
It would be good to have the green light and land that.
I've played quickly with a Chris' idea and will add a couple of RFC 
patches for discussion.

Cheers,

-
Lionel