Re: [PATCH 12/12] drm/i915: Extend GET_APERTURE ioctl to report size of the stolen region

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

On 20/04/16 12:17, ankitprasad.r.sharma@intel.com wrote:
> From: Ankitprasad Sharma <ankitprasad.r.sharma@intel.com>
>
> This patch extends the GET_APERTURE ioctl to add support
> for getting total size and available size of the stolen region
> as well as single largest block available in the stolen region.
> Also adds debugfs support to retieve the size information of the
> stolen area.
>
> v2: respinned over Rodrigo's patch which extends the GET_APERTURE
> too. Used drm_mm to get the size information of the stolen region
> (Chris)
> Added debugfs support for testing (Ankit)
>
> v3: Rebased to the latest drm-intel-nightly (Ankit)
>
> v4: Addressed comments, moved non-limits to debugfs.c (Chris/Tvrtko)
>
> Signed-off-by: Ankitprasad Sharma <ankitprasad.r.sharma@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_debugfs.c    | 12 +++++++++-
>   drivers/gpu/drm/i915/i915_drv.h        |  3 +++
>   drivers/gpu/drm/i915/i915_gem.c        |  2 ++
>   drivers/gpu/drm/i915/i915_gem_stolen.c | 41 ++++++++++++++++++++++++++++++++++
>   include/uapi/drm/i915_drm.h            |  6 +++++
>   5 files changed, 63 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index d8d7994..55d110e 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -580,7 +580,7 @@ static int i915_gem_aperture_info(struct seq_file *m, void *data)
>   	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;
> +	uint64_t last, size, stolen_free, stolen_largest;
>   	int ret;
>
>   	INIT_LIST_HEAD(&map_list);
> @@ -593,6 +593,10 @@ static int i915_gem_aperture_info(struct seq_file *m, void *data)
>   		return ret;
>
>   	mutex_lock(&dev->struct_mutex);
> +
> +	i915_gem_stolen_size_info(dev_priv, &stolen_free,
> +				  &stolen_largest);
> +
>   	list_for_each_entry(vma, &ggtt->base.active_list, vm_link)
>   		if (vma->pin_count &&
>   			(vma->node.start + vma->node.size) <= map_limit)
> @@ -652,6 +656,12 @@ skip_first:
>   		   fence_space);
>   	seq_printf(m, "Single largest fence available: %llu bytes\n",
>   		   fence_largest);
> +	seq_printf(m, "Total size of the stolen region: %llu bytes\n",
> +		   arg.stolen_total_size);
> +	seq_printf(m, "Available size of the stolen region: %llu bytes\n",
> +		   stolen_free);
> +	seq_printf(m, "Single largest area in the stolen region: %llu bytes\n",
> +		   stolen_largest);
>
>   	return 0;
>   }
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 37f9de8..d5c6b23 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -3424,6 +3424,9 @@ i915_gem_object_create_stolen_for_preallocated(struct drm_device *dev,
>   					       u32 gtt_offset,
>   					       u32 size);
>   int __must_check i915_gem_stolen_freeze(struct drm_i915_private *i915);
> +void i915_gem_stolen_size_info(struct drm_i915_private *dev_priv,
> +			       uint64_t *stolen_free,
> +			       uint64_t *stolen_largest);
>
>   /* i915_gem_shrinker.c */
>   unsigned long i915_gem_shrink(struct drm_i915_private *dev_priv,
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index a1be35f..e0f2259 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -166,6 +166,8 @@ 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;;
> +	args->stolen_total_size =
> +		dev_priv->mm.volatile_stolen ? 0 : ggtt->stolen_usable_size;
>
>   	return 0;
>   }
> diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
> index ef28af6..f2621c5 100644
> --- a/drivers/gpu/drm/i915/i915_gem_stolen.c
> +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
> @@ -912,3 +912,44 @@ int i915_gem_stolen_freeze(struct drm_i915_private *i915)
>
>   	return ret;
>   }
> +
> +
> +void i915_gem_stolen_size_info(struct drm_i915_private *dev_priv,
> +			       uint64_t *stolen_free,
> +			       uint64_t *stolen_largest)
> +{
> +	struct drm_mm *mm = &dev_priv->mm.stolen;
> +	struct drm_mm_node *head_node = &mm->head_node;
> +	struct drm_mm_node *entry;
> +	uint64_t hole_size, hole_start, hole_end, largest_hole = 0;
> +	uint64_t total_free = 0;
> +
> +	if (dev_priv->mm.volatile_stolen) {
> +		*stolen_free = 0;
> +		*stolen_largest = 0;
> +		return;
> +	}
> +
> +	if (head_node->hole_follows) {
> +		hole_start = drm_mm_hole_node_start(head_node);
> +		hole_end = drm_mm_hole_node_end(head_node);
> +		hole_size = hole_end - hole_start;
> +		total_free += hole_size;
> +		if (largest_hole < hole_size)
> +			largest_hole = hole_size;
> +	}
> +

Why does this block need to be separately handled and the loop below 
would not cover it? On first iteration entry will be the head node below 
as well, no?

> +	drm_mm_for_each_node(entry, mm) {
> +		if (entry->hole_follows) {
> +			hole_start = drm_mm_hole_node_start(entry);
> +			hole_end = drm_mm_hole_node_end(entry);
> +			hole_size = hole_end - hole_start;
> +			total_free += hole_size;
> +			if (largest_hole < hole_size)
> +				largest_hole = hole_size;
> +		}
> +	}
> +
> +	*stolen_free = total_free;
> +	*stolen_largest = largest_hole;
> +}
> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> index 8f38407..424e57e 100644
> --- a/include/uapi/drm/i915_drm.h
> +++ b/include/uapi/drm/i915_drm.h
> @@ -1012,6 +1012,12 @@ struct drm_i915_gem_get_aperture {
>   	 * Total space in the mappable region of the aperture, in bytes
>   	 */
>   	__u64 map_total_size;
> +
> +	/**
> +	 * Total space in the stolen region, in bytes
> +	 */
> +	__u64 stolen_total_size;
> +

How will the userspace detect existence of the new ioctl fields? Is it 
intended that they try to call it with a buffer of certain size and act 
on the failure when that fails? Is that good enough or we need something 
better like get_param or something?

Regards,

Tvrtko

>   };
>
>   struct drm_i915_get_pipe_from_crtc_id {
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx