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 21/04/16 15:41, Chris Wilson wrote:
> 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() ?

I see it in the header, yes.

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

I suppose it doesn't make any practical difference to the driver between 
"there is no stolen memory" and "driver does not support stolen memory 
query / create".

For mappable region it is a bit weirder because it wouldn't be able to 
tell if there is no mappable or no query support so it would potentially 
incorrectly avoid mmap_gtt etc.

> 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
>

Regards,

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