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

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

Hi,

On 20/04/16 12:17, ankitprasad.r.sharma@intel.com wrote:
> From: Ankitprasad Sharma <ankitprasad.r.sharma@intel.com>

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 <ankitprasad.r.sharma@intel.com>
> ---
>   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


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx