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

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

Hi,

On 07/08/2015 07:51 AM, 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)
>
> Signed-off-by: Ankitprasad Sharma <ankitprasad.r.sharma@intel.com>
> Reviewed-by: Chris Wilson <chris at chris-wilson.co.uk>
> ---
>   drivers/gpu/drm/i915/i915_debugfs.c    |  6 ++++++
>   drivers/gpu/drm/i915/i915_drv.h        |  3 +++
>   drivers/gpu/drm/i915/i915_gem.c        |  7 +++++++
>   drivers/gpu/drm/i915/i915_gem_stolen.c | 35 ++++++++++++++++++++++++++++++++++
>   include/uapi/drm/i915_drm.h            | 15 +++++++++++++++
>   5 files changed, 66 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 49ec438..d12ef0a 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -534,6 +534,12 @@ static int i915_gem_aperture_info(struct seq_file *m, void *data)
>   		   arg.fence_available_size);
>   	seq_printf(m, "Single largest fence available: %llu bytes\n",
>   		   arg.fence_largest_size);
> +	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",
> +		   arg.stolen_available_size);
> +	seq_printf(m, "Single largest area in the stolen region: %llu bytes\n",
> +		   arg.stolen_largest_size);
>
>   	return 0;
>   }
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index ea9caf2..7cd1b2e 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -3101,6 +3101,9 @@ i915_gem_object_create_stolen_for_preallocated(struct drm_device *dev,
>   					       u32 stolen_offset,
>   					       u32 gtt_offset,
>   					       u32 size);
> +void i915_gem_stolen_size_info(struct drm_mm *mm, uint64_t *stolen_total,
> +			       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 ccfc8d3..ec20c67 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -203,6 +203,7 @@ i915_gem_get_aperture_ioctl(struct drm_device *dev, void *data,
>   	struct list_head map_list;
>   	const u32 map_limit = dev_priv->gtt.mappable_end;
>   	size_t pinned, map_space, map_largest, fence_space, fence_largest;
> +	uint64_t stolen_total, stolen_available, stolen_largest;
>   	u32 last, size;
>
>   	INIT_LIST_HEAD(&map_list);
> @@ -260,6 +261,9 @@ skip_first:
>   			fence_largest = size;
>   		fence_space += size;
>   	}
> +
> +	i915_gem_stolen_size_info(&dev_priv->mm.stolen, &stolen_total,
> +				  &stolen_available, &stolen_largest);
>   	mutex_unlock(&dev->struct_mutex);
>
>   	args->aper_size = dev_priv->gtt.base.total;
> @@ -269,6 +273,9 @@ skip_first:
>   	args->map_total_size = dev_priv->gtt.mappable_end;
>   	args->fence_available_size = fence_space;
>   	args->fence_largest_size = fence_largest;
> +	args->stolen_total_size = stolen_total;
> +	args->stolen_available_size = stolen_available;
> +	args->stolen_largest_size = stolen_largest;
>
>   	return 0;
>   }
> diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
> index 348ed5a..08d983f 100644
> --- a/drivers/gpu/drm/i915/i915_gem_stolen.c
> +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
> @@ -551,3 +551,38 @@ err_out:
>   	drm_gem_object_unreference(&obj->base);
>   	return NULL;
>   }
> +
> +void i915_gem_stolen_size_info(struct drm_mm *mm, uint64_t *stolen_total,
> +			       uint64_t *stolen_free,
> +			       uint64_t *stolen_largest)
> +{
> +	struct drm_mm_node *entry;
> +	struct drm_mm_node *head_node = &mm->head_node;
> +	uint64_t hole_size, hole_start, hole_end, largest_hole = 0;
> +	uint64_t total_used = 0, total_free = 0;
> +
> +	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;

Guaranteed it is the largest hole by this point, but not worth the 
effort of respining to change it.

Maybe only consider getting rid of hole_start and hole_end since they 
are not used but once. It would compact this block and the one below. 
Unless it would be longer than 80 chars in which case is not worth it.

> +	}
> +
> +	drm_mm_for_each_node(entry, mm) {
> +		total_used += entry->size;
> +		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_total = total_free + total_used;

Pitty for such a round about way to get total amount of stolen memory, 
but it looks we don't store it elsewhere.

> +	*stolen_free = total_free;
> +	*stolen_largest = largest_hole;

Again I wonder the usefulness of this data?

But for the code review itself;

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

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