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

[Tvrtko Ursulin]

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.

Regards,

Tvrtko