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

Submitted by Michel Thierry on July 7, 2015, 3:14 p.m.

Details

Message ID 1436282103-5854-15-git-send-email-michel.thierry@intel.com
State New
Headers show

Not browsing as part of any series.

Commit Message

Michel Thierry July 7, 2015, 3:14 p.m.
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(-)

Patch hide | download patch | download mbox

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;
+
 	bind_flags = 0;
 	if (flags & PIN_GLOBAL)
 		bind_flags |= GLOBAL_BIND;

Comments

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

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.
-Chris
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?
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
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;
On Wed, Jul 08, 2015 at 05:42:17PM +0100, Michel Thierry wrote:
> >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;

Are we still talking about i915_vma_bind()? Then vma->node.size !=
vma->obj.base.size anyway.
-Chris
On 7/8/2015 6:03 PM, Chris Wilson wrote:
> On Wed, Jul 08, 2015 at 05:42:17PM +0100, Michel Thierry wrote:
>>> 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;
>
> Are we still talking about i915_vma_bind()? Then vma->node.size !=
> vma->obj.base.size anyway.
> -Chris
>
Right, it can be either obj->base.size, fence_size or view_size...