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

[Chris Wilson]

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.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre