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

Michel Thierry
2015-07-07T15:45:09Z

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

I'll change it to use drm_mm_node_allocated and return ENODEV in both
places.