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

Commit Message

[e-mail] April 20, 2016, 11:17 a.m. UTC

From: Ankitprasad Sharma <[e-mail]>

When constructing a batchbuffer, it is sometimes crucial to know the
largest hole into which we can fit a fenceable buffer (for example when
handling very large objects on gen2 and gen3). This depends on the
fragmentation of pinned buffers inside the aperture, a question only the
kernel can easily answer.

This patch extends the current DRM_I915_GEM_GET_APERTURE ioctl to
include a couple of new fields in its reply to userspace - the total
amount of space available in the mappable region of the aperture and
also the single largest block available.

This is not quite what userspace wants to answer the question of whether
this batch will fit as fences are also required to meet severe alignment
constraints within the batch. For this purpose, a third conservative
estimate of largest fence available is also provided. For when userspace
needs more than one batch, we also provide the culmulative space
available for fences such that it has some additional guidance to how
much space it could allocate to fences. Conservatism still wins.

The patch also adds a debugfs file for convenient testing and reporting.

v2: The first object cannot end at offset 0, so we can use last==0 to
detect the empty list.

v3: Expand all values to 64bit, just in case.
    Report total mappable aperture size for userspace that cannot easily
    determine it by inspecting the PCI device.

v4: (Rodrigo) Fixed rebase conflicts.

v5: Keeping limits to get_aperture ioctl, and moved changing numbers to
debugfs, Addressed comments (Chris/Tvrtko)

Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi at intel.com>
Signed-off-by: Ankitprasad Sharma <[e-mail]>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 133 ++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_gem.c     |   1 +
 include/uapi/drm/i915_drm.h         |   5 ++
 3 files changed, 139 insertions(+)

Comments

kernel test robot April 20, 2016, 1:02 p.m. UTC | #1

Hi,

[auto build test WARNING on drm-intel/for-linux-next]
[also build test WARNING on next-20160420]
[cannot apply to v4.6-rc4]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/ankitprasad-r-sharma-intel-com/Support-for-creating-using-Stolen-memory-backed-objects/20160420-194608
base:   git://anongit.freedesktop.org/drm-intel for-linux-next


coccinelle warnings: (new ones prefixed by >>)

>> drivers/gpu/drm/i915/i915_gem.c:168:43-44: Unneeded semicolon

Please review and possibly fold the followup patch.

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

Tvrtko Ursulin April 21, 2016, 2:04 p.m. UTC | #2

Hi,

On 20/04/16 12:17, [e-mail] wrote:
> From: Ankitprasad Sharma <[e-mail]>

Patch is mostly Rodrigo's, right? So I assumed he approved the 
authorship transfer.

> When constructing a batchbuffer, it is sometimes crucial to know the
> largest hole into which we can fit a fenceable buffer (for example when
> handling very large objects on gen2 and gen3). This depends on the
> fragmentation of pinned buffers inside the aperture, a question only the
> kernel can easily answer.
>
> This patch extends the current DRM_I915_GEM_GET_APERTURE ioctl to
> include a couple of new fields in its reply to userspace - the total
> amount of space available in the mappable region of the aperture and
> also the single largest block available.
>
> This is not quite what userspace wants to answer the question of whether
> this batch will fit as fences are also required to meet severe alignment
> constraints within the batch. For this purpose, a third conservative
> estimate of largest fence available is also provided. For when userspace
> needs more than one batch, we also provide the culmulative space
> available for fences such that it has some additional guidance to how
> much space it could allocate to fences. Conservatism still wins.
>
> The patch also adds a debugfs file for convenient testing and reporting.
>
> v2: The first object cannot end at offset 0, so we can use last==0 to
> detect the empty list.
>
> v3: Expand all values to 64bit, just in case.
>      Report total mappable aperture size for userspace that cannot easily
>      determine it by inspecting the PCI device.
>
> v4: (Rodrigo) Fixed rebase conflicts.
>
> v5: Keeping limits to get_aperture ioctl, and moved changing numbers to
> debugfs, Addressed comments (Chris/Tvrtko)

It is confusing that you already posted a different v5 on 1st of July 2015.

> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi at intel.com>
> Signed-off-by: Ankitprasad Sharma <[e-mail]>
> ---
>   drivers/gpu/drm/i915/i915_debugfs.c | 133 ++++++++++++++++++++++++++++++++++++
>   drivers/gpu/drm/i915/i915_gem.c     |   1 +
>   include/uapi/drm/i915_drm.h         |   5 ++
>   3 files changed, 139 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 7f94c6a..d8d7994 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -524,6 +524,138 @@ static int i915_gem_object_info(struct seq_file *m, void* data)
>   	return 0;
>   }
>
> +static int vma_rank_by_ggtt(void *priv,
> +			    struct list_head *A,
> +			    struct list_head *B)
> +{
> +	struct i915_vma *a = list_entry(A, typeof(*a), exec_list);
> +	struct i915_vma *b = list_entry(B, typeof(*b), exec_list);
> +
> +	return a->node.start - b->node.start;
> +}
> +
> +static u32 __fence_size(struct drm_i915_private *dev_priv, u32 start, u32 end)
> +{
> +	u32 size = end - start;
> +	u32 fence_size;
> +
> +	if (INTEL_INFO(dev_priv)->gen < 4) {
> +		u32 fence_max;
> +		u32 fence_next;
> +
> +		if (IS_GEN3(dev_priv)) {
> +			fence_max = I830_FENCE_MAX_SIZE_VAL << 20;
> +			fence_next = 1024*1024;
> +		} else {
> +			fence_max = I830_FENCE_MAX_SIZE_VAL << 19;
> +			fence_next = 512*1024;
> +		}
> +
> +		fence_max = min(fence_max, size);
> +		fence_size = 0;
> +		/* Find fence_size less than fence_max and power of 2 */
> +		while (fence_next <= fence_max) {
> +			u32 base = ALIGN(start, fence_next);
> +			if (base + fence_next > end)
> +				break;
> +
> +			fence_size = fence_next;
> +			fence_next <<= 1;
> +		}
> +	} else {
> +		fence_size = size;
> +	}
> +
> +	return fence_size;
> +}
> +
> +static int i915_gem_aperture_info(struct seq_file *m, void *data)
> +{
> +	struct drm_info_node *node = m->private;
> +	struct drm_device *dev = node->minor->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct i915_ggtt *ggtt = &dev_priv->ggtt;
> +	struct drm_i915_gem_get_aperture arg;
> +	struct i915_vma *vma;
> +	struct list_head map_list;
> +	const uint64_t map_limit = ggtt->mappable_end;
> +	uint64_t map_space, map_largest, fence_space, fence_largest;
> +	uint64_t last, size;
> +	int ret;
> +
> +	INIT_LIST_HEAD(&map_list);
> +
> +	map_space = map_largest = 0;
> +	fence_space = fence_largest = 0;
> +
> +	ret = i915_gem_get_aperture_ioctl(node->minor->dev, &arg, NULL);
> +	if (ret)
> +		return ret;
> +
> +	mutex_lock(&dev->struct_mutex);
> +	list_for_each_entry(vma, &ggtt->base.active_list, vm_link)
> +		if (vma->pin_count &&
> +			(vma->node.start + vma->node.size) <= map_limit)
> +			list_add(&vma->exec_list, &map_list);
> +	list_for_each_entry(vma, &ggtt->base.inactive_list, vm_link)
> +		if (vma->pin_count &&
> +			(vma->node.start + vma->node.size) <= map_limit)
> +			list_add(&vma->exec_list, &map_list);

Could squash the two with an outer loop containing pointers to active 
and inactive list, like a pattern from the shrinker or something. 
Wouldn't save many lines of code though so not sure.

> +
> +	last = 0;
> +	list_sort(NULL, &map_list, vma_rank_by_ggtt);
> +	while (!list_empty(&map_list)) {
> +		vma = list_first_entry(&map_list, typeof(*vma), exec_list);
> +		list_del_init(&vma->exec_list);
> +
> +		if (last == 0)
> +			goto skip_first;
> +
> +		size = vma->node.start - last;

hole_size for readability? (took me some time to figure it out)

> +		if (size > map_largest)
> +			map_largest = size;
> +		map_space += size;
> +
> +		size = __fence_size(dev_priv, last, vma->node.start);
> +		if (size > fence_largest)
> +			fence_largest = size;
> +		fence_space += size;
> +
> +skip_first:
> +		last = vma->node.start + vma->node.size;
> +	}
> +	if (last < map_limit) {
> +		size = map_limit - last;
> +		if (size > map_largest)
> +			map_largest = size;
> +		map_space += size;
> +
> +		size = __fence_size(dev_priv, last, map_limit);
> +		if (size > fence_largest)
> +			fence_largest = size;
> +		fence_space += size;
> +	}
> +
> +	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.

>   static int i915_gem_gtt_info(struct seq_file *m, void *data)
>   {
>   	struct drm_info_node *node = m->private;
> @@ -5354,6 +5486,7 @@ static int i915_debugfs_create(struct dentry *root,
>   static const struct drm_info_list i915_debugfs_list[] = {
>   	{"i915_capabilities", i915_capabilities, 0},
>   	{"i915_gem_objects", i915_gem_object_info, 0},
> +	{"i915_gem_aperture", i915_gem_aperture_info, 0},
>   	{"i915_gem_gtt", i915_gem_gtt_info, 0},
>   	{"i915_gem_pinned", i915_gem_gtt_info, 0, (void *) PINNED_LIST},
>   	{"i915_gem_active", i915_gem_object_list_info, 0, (void *) ACTIVE_LIST},
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 45fd049..a1be35f 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -165,6 +165,7 @@ i915_gem_get_aperture_ioctl(struct drm_device *dev, void *data,
>
>   	args->aper_size = ggtt->base.total;
>   	args->aper_available_size = args->aper_size - pinned;
> +	args->map_total_size = ggtt->mappable_end;;
>
>   	return 0;
>   }
> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> index dae02d8..8f38407 100644
> --- a/include/uapi/drm/i915_drm.h
> +++ b/include/uapi/drm/i915_drm.h
> @@ -1007,6 +1007,11 @@ struct drm_i915_gem_get_aperture {
>   	 * bytes
>   	 */
>   	__u64 aper_available_size;
> +
> +	/**
> +	 * Total space in the mappable region of the aperture, in bytes
> +	 */
> +	__u64 map_total_size;
>   };
>
>   struct drm_i915_get_pipe_from_crtc_id {
>

And I guess it is up to Daniel to approve any ABI extensions? (CCed)

Open source user for it exists?

Regards,

Tvrtko

Chris Wilson April 21, 2016, 2:46 p.m. UTC | #3

On Thu, Apr 21, 2016 at 03:04:52PM +0100, Tvrtko Ursulin wrote:
> 
> Hi,
> 
> On 20/04/16 12:17, [e-mail] 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.
-Chris

Tvrtko Ursulin April 21, 2016, 2:59 p.m. UTC | #4

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, [e-mail] 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.

Regards,

Tvrtko

[e-mail] April 25, 2016, 10:35 a.m. UTC | #5

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, [e-mail] 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.

Thanks,
Ankit

Tvrtko Ursulin April 25, 2016, 2:51 p.m. UTC | #6

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, [e-mail] 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?

Regards,

Tvrtko

Chris Wilson April 26, 2016, 9:44 a.m. UTC | #7

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, [e-mail] 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.
-Chris

Tvrtko Ursulin April 28, 2016, 9:30 a.m. UTC | #8

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, [e-mail] 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

Chris Wilson April 28, 2016, 10:24 a.m. UTC | #9

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, [e-mail] 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

Tvrtko Ursulin April 29, 2016, 10:06 a.m. UTC | #10

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, [e-mail] 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

Chris Wilson April 29, 2016, 10:18 a.m. UTC | #11

On Fri, Apr 29, 2016 at 11:06:49AM +0100, Tvrtko Ursulin wrote:
> 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?

You're suggesting to add a field I'm not going to use. Is any one?
Until there is an actual use (which would afaict be if one of the
existing values changed meaning, which itself would be an abi break) I'm
not convinced we should be designing for the unknowable. If we did need
to version would we not be better off with a new ioctl? 
-Chris

Tvrtko Ursulin April 29, 2016, 10:26 a.m. UTC | #12

On 29/04/16 11:18, Chris Wilson wrote:
> On Fri, Apr 29, 2016 at 11:06:49AM +0100, Tvrtko Ursulin wrote:
>> 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?
>
> You're suggesting to add a field I'm not going to use. Is any one?
> Until there is an actual use (which would afaict be if one of the
> existing values changed meaning, which itself would be an abi break) I'm
> not convinced we should be designing for the unknowable. If we did need
> to version would we not be better off with a new ioctl?

I am suggesting if we are adding aper_total_size, or whatever it is 
called, to make it usable in an self-contained matter.

My impression was you want aper_total_size so I was simply objecting to 
adding it without being able to directly tell if kernel actually 
supports that extension.

Regards,

Tvrtko

Chris Wilson April 29, 2016, 10:39 a.m. UTC | #13

On Fri, Apr 29, 2016 at 11:26:45AM +0100, Tvrtko Ursulin wrote:
> 
> On 29/04/16 11:18, Chris Wilson wrote:
> >On Fri, Apr 29, 2016 at 11:06:49AM +0100, Tvrtko Ursulin wrote:
> >>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?
> >
> >You're suggesting to add a field I'm not going to use. Is any one?
> >Until there is an actual use (which would afaict be if one of the
> >existing values changed meaning, which itself would be an abi break) I'm
> >not convinced we should be designing for the unknowable. If we did need
> >to version would we not be better off with a new ioctl?
> 
> I am suggesting if we are adding aper_total_size, or whatever it is
> called, to make it usable in an self-contained matter.
> 
> My impression was you want aper_total_size so I was simply objecting
> to adding it without being able to directly tell if kernel actually
> supports that extension.

The two additions that I thought we have reduced it to:

	u64 mappable_aperture_size
	u64 stolen_size;

Of which I agree that the first has some ambiguity over 0. But I don't
think it makes a difference in practice. I for one would not bother with
checking a version. I already have a cascade here to deal with not being
able to use various probes, with an eventual fallback to a safe value.

We know the mappable_aperture_size cannot be zero with the exception
that the device is disabled - but we have opened the device already.
-Chris

Tvrtko Ursulin April 29, 2016, 10:56 a.m. UTC | #14

On 29/04/16 11:39, Chris Wilson wrote:
> On Fri, Apr 29, 2016 at 11:26:45AM +0100, Tvrtko Ursulin wrote:
>>
>> On 29/04/16 11:18, Chris Wilson wrote:
>>> On Fri, Apr 29, 2016 at 11:06:49AM +0100, Tvrtko Ursulin wrote:
>>>> 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?
>>>
>>> You're suggesting to add a field I'm not going to use. Is any one?
>>> Until there is an actual use (which would afaict be if one of the
>>> existing values changed meaning, which itself would be an abi break) I'm
>>> not convinced we should be designing for the unknowable. If we did need
>>> to version would we not be better off with a new ioctl?
>>
>> I am suggesting if we are adding aper_total_size, or whatever it is
>> called, to make it usable in an self-contained matter.
>>
>> My impression was you want aper_total_size so I was simply objecting
>> to adding it without being able to directly tell if kernel actually
>> supports that extension.
>
> The two additions that I thought we have reduced it to:
>
> 	u64 mappable_aperture_size
> 	u64 stolen_size;
>
> Of which I agree that the first has some ambiguity over 0. But I don't
> think it makes a difference in practice. I for one would not bother with
> checking a version. I already have a cascade here to deal with not being
> able to use various probes, with an eventual fallback to a safe value.
>
> We know the mappable_aperture_size cannot be zero with the exception
> that the device is disabled - but we have opened the device already.

Since you agree there is ambiguity lets just add a version. You don't 
have to use it, but someone will.

	u32 version; // == 1
  	u64 mappable_aperture_size;

And then it is clear and self-contained.

Regards,

Tvrtko

Chris Wilson April 29, 2016, 11:03 a.m. UTC | #15

On Fri, Apr 29, 2016 at 11:56:28AM +0100, Tvrtko Ursulin wrote:
> 
> On 29/04/16 11:39, Chris Wilson wrote:
> >On Fri, Apr 29, 2016 at 11:26:45AM +0100, Tvrtko Ursulin wrote:
> >>
> >>On 29/04/16 11:18, Chris Wilson wrote:
> >>>On Fri, Apr 29, 2016 at 11:06:49AM +0100, Tvrtko Ursulin wrote:
> >>>>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?
> >>>
> >>>You're suggesting to add a field I'm not going to use. Is any one?
> >>>Until there is an actual use (which would afaict be if one of the
> >>>existing values changed meaning, which itself would be an abi break) I'm
> >>>not convinced we should be designing for the unknowable. If we did need
> >>>to version would we not be better off with a new ioctl?
> >>
> >>I am suggesting if we are adding aper_total_size, or whatever it is
> >>called, to make it usable in an self-contained matter.
> >>
> >>My impression was you want aper_total_size so I was simply objecting
> >>to adding it without being able to directly tell if kernel actually
> >>supports that extension.
> >
> >The two additions that I thought we have reduced it to:
> >
> >	u64 mappable_aperture_size
> >	u64 stolen_size;
> >
> >Of which I agree that the first has some ambiguity over 0. But I don't
> >think it makes a difference in practice. I for one would not bother with
> >checking a version. I already have a cascade here to deal with not being
> >able to use various probes, with an eventual fallback to a safe value.
> >
> >We know the mappable_aperture_size cannot be zero with the exception
> >that the device is disabled - but we have opened the device already.
> 
> Since you agree there is ambiguity lets just add a version. You
> don't have to use it, but someone will.
> 
> 	u32 version; // == 1
>  	u64 mappable_aperture_size;
> 
> And then it is clear and self-contained.

	u64 version;

or

	u32 version;
	u32 flags;

uABI fields need to be naturally aligned.

Hah, you didn't mention the trump card you pulled on IRC. In light of
that issue ever being a problem (that we may be faced with 0 aperture),
yes.
-Chris

Patch

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 7f94c6a..d8d7994 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -524,6 +524,138 @@  static int i915_gem_object_info(struct seq_file *m, void* data)
 	return 0;
 }
 
+static int vma_rank_by_ggtt(void *priv,
+			    struct list_head *A,
+			    struct list_head *B)
+{
+	struct i915_vma *a = list_entry(A, typeof(*a), exec_list);
+	struct i915_vma *b = list_entry(B, typeof(*b), exec_list);
+
+	return a->node.start - b->node.start;
+}
+
+static u32 __fence_size(struct drm_i915_private *dev_priv, u32 start, u32 end)
+{
+	u32 size = end - start;
+	u32 fence_size;
+
+	if (INTEL_INFO(dev_priv)->gen < 4) {
+		u32 fence_max;
+		u32 fence_next;
+
+		if (IS_GEN3(dev_priv)) {
+			fence_max = I830_FENCE_MAX_SIZE_VAL << 20;
+			fence_next = 1024*1024;
+		} else {
+			fence_max = I830_FENCE_MAX_SIZE_VAL << 19;
+			fence_next = 512*1024;
+		}
+
+		fence_max = min(fence_max, size);
+		fence_size = 0;
+		/* Find fence_size less than fence_max and power of 2 */
+		while (fence_next <= fence_max) {
+			u32 base = ALIGN(start, fence_next);
+			if (base + fence_next > end)
+				break;
+
+			fence_size = fence_next;
+			fence_next <<= 1;
+		}
+	} else {
+		fence_size = size;
+	}
+
+	return fence_size;
+}
+
+static int i915_gem_aperture_info(struct seq_file *m, void *data)
+{
+	struct drm_info_node *node = m->private;
+	struct drm_device *dev = node->minor->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct i915_ggtt *ggtt = &dev_priv->ggtt;
+	struct drm_i915_gem_get_aperture arg;
+	struct i915_vma *vma;
+	struct list_head map_list;
+	const uint64_t map_limit = ggtt->mappable_end;
+	uint64_t map_space, map_largest, fence_space, fence_largest;
+	uint64_t last, size;
+	int ret;
+
+	INIT_LIST_HEAD(&map_list);
+
+	map_space = map_largest = 0;
+	fence_space = fence_largest = 0;
+
+	ret = i915_gem_get_aperture_ioctl(node->minor->dev, &arg, NULL);
+	if (ret)
+		return ret;
+
+	mutex_lock(&dev->struct_mutex);
+	list_for_each_entry(vma, &ggtt->base.active_list, vm_link)
+		if (vma->pin_count &&
+			(vma->node.start + vma->node.size) <= map_limit)
+			list_add(&vma->exec_list, &map_list);
+	list_for_each_entry(vma, &ggtt->base.inactive_list, vm_link)
+		if (vma->pin_count &&
+			(vma->node.start + vma->node.size) <= map_limit)
+			list_add(&vma->exec_list, &map_list);
+
+	last = 0;
+	list_sort(NULL, &map_list, vma_rank_by_ggtt);
+	while (!list_empty(&map_list)) {
+		vma = list_first_entry(&map_list, typeof(*vma), exec_list);
+		list_del_init(&vma->exec_list);
+
+		if (last == 0)
+			goto skip_first;
+
+		size = vma->node.start - last;
+		if (size > map_largest)
+			map_largest = size;
+		map_space += size;
+
+		size = __fence_size(dev_priv, last, vma->node.start);
+		if (size > fence_largest)
+			fence_largest = size;
+		fence_space += size;
+
+skip_first:
+		last = vma->node.start + vma->node.size;
+	}
+	if (last < map_limit) {
+		size = map_limit - last;
+		if (size > map_largest)
+			map_largest = size;
+		map_space += size;
+
+		size = __fence_size(dev_priv, last, map_limit);
+		if (size > fence_largest)
+			fence_largest = size;
+		fence_space += size;
+	}
+
+	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;
+}
+
 static int i915_gem_gtt_info(struct seq_file *m, void *data)
 {
 	struct drm_info_node *node = m->private;
@@ -5354,6 +5486,7 @@  static int i915_debugfs_create(struct dentry *root,
 static const struct drm_info_list i915_debugfs_list[] = {
 	{"i915_capabilities", i915_capabilities, 0},
 	{"i915_gem_objects", i915_gem_object_info, 0},
+	{"i915_gem_aperture", i915_gem_aperture_info, 0},
 	{"i915_gem_gtt", i915_gem_gtt_info, 0},
 	{"i915_gem_pinned", i915_gem_gtt_info, 0, (void *) PINNED_LIST},
 	{"i915_gem_active", i915_gem_object_list_info, 0, (void *) ACTIVE_LIST},
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 45fd049..a1be35f 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -165,6 +165,7 @@  i915_gem_get_aperture_ioctl(struct drm_device *dev, void *data,
 
 	args->aper_size = ggtt->base.total;
 	args->aper_available_size = args->aper_size - pinned;
+	args->map_total_size = ggtt->mappable_end;;
 
 	return 0;
 }
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index dae02d8..8f38407 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -1007,6 +1007,11 @@  struct drm_i915_gem_get_aperture {
 	 * bytes
 	 */
 	__u64 aper_available_size;
+
+	/**
+	 * Total space in the mappable region of the aperture, in bytes
+	 */
+	__u64 map_total_size;
 };
 
 struct drm_i915_get_pipe_from_crtc_id {