[Intel-gfx] [PATCH 11/12] drm/i915: Extend GET_APERTURE ioctl to report available map space

[Tvrtko Ursulin]

On 28/04/16 11:24, Chris Wilson wrote:
> On Thu, Apr 28, 2016 at 10:30:32AM +0100, Tvrtko Ursulin wrote:
>>
>> On 26/04/16 10:44, Chris Wilson wrote:
>>> On Mon, Apr 25, 2016 at 03:51:09PM +0100, Tvrtko Ursulin wrote:
>>>>
>>>> On 25/04/16 11:35, Ankitprasad Sharma wrote:
>>>>> On Thu, 2016-04-21 at 15:59 +0100, Tvrtko Ursulin wrote:
>>>>>> On 21/04/16 15:46, Chris Wilson wrote:
>>>>>>> On Thu, Apr 21, 2016 at 03:04:52PM +0100, Tvrtko Ursulin wrote:
>>>>>>>>
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> On 20/04/16 12:17, ankitprasad.r.sharma at intel.com wrote:
>>>>>>>>> +	mutex_unlock(&dev->struct_mutex);
>>>>>>>>> +
>>>>>>>>> +	seq_printf(m, "Total size of the GTT: %llu bytes\n",
>>>>>>>>> +		   arg.aper_size);
>>>>>>>>> +	seq_printf(m, "Available space in the GTT: %llu bytes\n",
>>>>>>>>> +		   arg.aper_available_size);
>>>>>>>>> +	seq_printf(m, "Total space in the mappable aperture: %llu bytes\n",
>>>>>>>>> +		   arg.map_total_size);
>>>>>>>>> +	seq_printf(m, "Available space in the mappable aperture: %llu bytes\n",
>>>>>>>>> +		   map_space);
>>>>>>>>> +	seq_printf(m, "Single largest space in the mappable aperture: %llu bytes\n",
>>>>>>>>> +		   map_largest);
>>>>>>>>> +	seq_printf(m, "Available space for fences: %llu bytes\n",
>>>>>>>>> +		   fence_space);
>>>>>>>>> +	seq_printf(m, "Single largest fence available: %llu bytes\n",
>>>>>>>>> +		   fence_largest);
>>>>>>>>> +
>>>>>>>>> +	return 0;
>>>>>>>>> +}
>>>>>>>>> +
>>>>>>>>
>>>>>>>> In general I find this a lot of code for a feature of questionable
>>>>>>>> utility. As such I would prefer someone really stated the need for
>>>>>>>> this and explained how it really is useful - even though whetever
>>>>>>>> number they get from this may be completely irrelevant by the time
>>>>>>>> it is acted upon.
>>>>>>>
>>>>>>> Yes, with the exception of the size of the mappable aperture, this is
>>>>>>> really is debug info. It will get automatically dumped by userspace
>>>>>>> when it sees an ENOSPC, and that may prove enough to solve the riddle of
>>>>>>> why it failed. However, this information is terrible outdated and now
>>>>>>> longer of such relevance.
>>>>>>>
>>>>>>> As for the mappable aperture size, there has been a request many years
>>>>>>> ago! could we provide it without resorting to a privilege operation. I
>>>>>>> guess by know that request has died out - but there is still the issue
>>>>>>> with libpciassess that make it unsuitable for use inside a library where
>>>>>>> one may want to avoid it and use a simple ioctl on the device you
>>>>>>> already have open.
>>>>>>>
>>>>>>> Yes, it is meh.
>>>>>>
>>>>>> Aperture size in the ioctl is fine I think, just that detection caveat
>>>>>> what I asked in the other reply.
>>>>>>
>>>>>> Here I wanted to suggest dropping all the non-trivial debugfs stuff and
>>>>>> just leave the info queried via i915_gem_get_aperture ioctl. So
>>>>>> effectively dropping the list traversal and vma sorting bits.
>>>>>>
>>>>> I think, debug info regarding the mappable space is good to have for
>>>>> debugging purpose as Chris mentioned.
>>>>> Also, the list traversal and the vma sorting stuff will only be called
>>>>> for debugging purposes, not slowing anything down or so.
>>>>
>>>> I am pretty indifferent on the topic of debugfs edition.
>>>>
>>>> But for the ioctl extension, how about adding a version field as the
>>>> first one in the extended area?
>>>
>>> A version number only makes sense when you are changing the meaning of
>>> an existing field. Adding one implies that we are planning to do so, are
>>> we?
>>>
>>> In the scenarios, I've run through I haven't found one where a caller
>>> would behave any differently faced with "0 - ioctl version not
>>> supported" and "0 - no available space (mappable/stolen)". Adding a
>>> version doesn't help using the new fields afaict. The argument is the
>>> same as whether a flags field is forward thinking or unthinkingly
>>> forward.
>>
>> I was thinking that if 0 = no aperture or ioctl not supported
>> userspace has to try one mmap_gtt to find out which is true, will it
>> be ENODEV or ENOSPC (assuming, haven't checked). If we put a version
>> in there then it can avoid doing that. Sounds like a better
>> interface to me and I don't see any downsides to it.
>
> I was thinking either userspace already cares and has a method for
> finding the size of the PCI memory region or it doesn't. If it doesn't
> and the ioctl reports 0 and its older second method fails with EPERM/EACCESS
> then it is no worse off than before.

I don't get it - if we are adding something why not add it in a way that 
makes it clear and self-contained - what is the downside of what I 
propose to meet such resistance?

Regards,

Tvrtko