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

[Chris Wilson <chris@chris-wilson.co.uk>]

On Thu, Apr 21, 2016 at 03:17:06PM +0100, Tvrtko Ursulin wrote:
> >+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?

Hmm, didn't I/somebody add drm_mm_for_each_hole() ?

> >+	*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?

As we are extending the structure:

1. Old userspace, old kernel: unaffacted

2. Old userspace, new kernel:
Kernel computes the new fields, but the struct is truncated in the copy
back to userspace. userspace only sees the fields it used to, no change.

3. New userspace, old kernel:
Userspace passes in a larger struct, kernel only copies back in the
fields it knows and zero fills the tail of the user's struct. userspace
sees a stolen_total_size of 0 and knows to avoid the new interface.

4. New userspace, new kernel:
Everything explodes^W just works.

In this case, the struct is only an out, so we don't need to do invalid
pad or flag rejection. We just have the ABI rule that anything the
kernel doesn't know about is ignored and zero-filled were applicable.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx