Re: [PATCH v4 14/18] drm/i915: object size needs to be u64

[Michel Thierry <michel.thierry@intel.com>]

On 7/8/2015 4:22 PM, Daniel Vetter wrote:
> On Wed, Jul 08, 2015 at 12:22:58PM +0100, Michel Thierry wrote:
>> On 7/7/2015 9:08 PM, Chris Wilson wrote:
>>> On Tue, Jul 07, 2015 at 04:44:30PM +0100, Michel Thierry wrote:
>>>> On 7/7/2015 4:27 PM, Chris Wilson wrote:
>>>>> On Tue, Jul 07, 2015 at 04:14:59PM +0100, Michel Thierry wrote:
>>>>>> In a 48b world, users can try to allocate buffers bigger than 4GB; in
>>>>>> these cases it is important that size is a 64b variable.
>>>>>>
>>>>>> Also added a warning for illegal bind with size = 0.
>>>>>>
>>>>>> Signed-off-by: Michel Thierry <michel.thierry@intel.com>
>>>>>> ---
>>>>>>   drivers/gpu/drm/i915/i915_gem.c     | 5 +++--
>>>>>>   drivers/gpu/drm/i915/i915_gem_gtt.c | 3 +++
>>>>>>   2 files changed, 6 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
>>>>>> index a0bff41..ebfb789 100644
>>>>>> --- a/drivers/gpu/drm/i915/i915_gem.c
>>>>>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>>>>>> @@ -3718,7 +3718,8 @@ i915_gem_object_bind_to_vm(struct drm_i915_gem_object *obj,
>>>>>>   {
>>>>>>   	struct drm_device *dev = obj->base.dev;
>>>>>>   	struct drm_i915_private *dev_priv = dev->dev_private;
>>>>>> -	u32 size, fence_size, fence_alignment, unfenced_alignment;
>>>>>> +	u32 fence_alignment, unfenced_alignment;
>>>>>> +	u64 size, fence_size;
>>>>>>   	u64 start =
>>>>>>   		flags & PIN_OFFSET_BIAS ? flags & PIN_OFFSET_MASK : 0;
>>>>>>   	u64 end =
>>>>>> @@ -3777,7 +3778,7 @@ i915_gem_object_bind_to_vm(struct drm_i915_gem_object *obj,
>>>>>>   	 * attempt to find space.
>>>>>>   	 */
>>>>>>   	if (size > end) {
>>>>>> -		DRM_DEBUG("Attempting to bind an object (view type=%u) larger than the aperture: size=%u > %s aperture=%llu\n",
>>>>>> +		DRM_DEBUG("Attempting to bind an object (view type=%u) larger than the aperture: size=%llu > %s aperture=%llu\n",
>>>>>>   			  ggtt_view ? ggtt_view->type : 0,
>>>>>>   			  size,
>>>>>>   			  flags & PIN_MAPPABLE ? "mappable" : "total",
>>>>>> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
>>>>>> index 449a245..900bce6 100644
>>>>>> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
>>>>>> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
>>>>>> @@ -3312,6 +3312,9 @@ int i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level,
>>>>>>   	if (WARN_ON(flags == 0))
>>>>>>   		return -EINVAL;
>>>>>>
>>>>>> +	if (WARN_ON(vma->node.size == 0))
>>>>>> +		return -EINVAL;
>>>>>
>>>>> This is superfluous. We don't allow size=0 object creation, and the test
>>>>> is better (if at all) at vma_create, but what you mean here is
>>>>> WARN_ON(!drm_mm_node_allocated()) which seems sensisble. And both of
>>>>> these would be better as ENODEV so we don't confuse the user when they
>>>>> get propagated back to userspace.
>>>>> -Chris
>>>>>
>>>> My idea was to catch the node.size overflow if the variable is
>>>> inadvertently changed back to u32 (which has already happen in other
>>>> places).
>>>
>>> Ok, that didn't come across when I just read node.size == 0 (what are
>>> chances that node.size was exactly 2^32 and then truncated?)
>>>
>> Only a test explicitly looking for this kind of issues (I guess). In that
>> test, objects bigger than 2^32 were truncated, while objects exactly 2^32
>> were hitting a WARN in the driver; alloc_pages wouldn't do anything because
>> node.size == 0, and then insert would complain no pages existed.
>>
>>> vma->node should be fairly opaque, and if possible we want the checks in
>>> drm_mm.c - if we can think of good tests for that layer.
>>>
>>> Certainly drm_mm_reserve_node() probably wants a few sanity checks.
>>> Though most of those should fall out when it can't do the reservation
>>> the user requests.
>> Or change drm_mm_insert_node_in_range_generic() to warn when size==0?
>
> WARN_ON(vma->node.size != obj->base.size) ? Feel free to get the casting
> right - I suck at implicit C integer conversion rules ...
> -Daniel
>
Thanks, if there's no objections, I'll change it to:

     if (WARN_ON(vma->node.size != (u64)vma->obj->base.size))
         return -ENODEV;

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