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

From: Chris Wilson <chris@chris-wilson.co.uk>
To: Michel Thierry <michel.thierry@intel.com>
Cc: intel-gfx@lists.freedesktop.org, akash.goel@intel.com
Subject: Re: [PATCH v4 14/18] drm/i915: object size needs to be u64
Date: Tue, 7 Jul 2015 21:08:21 +0100	[thread overview]
Message-ID: <20150707200821.GF17203@nuc-i3427.alporthouse.com> (raw)
In-Reply-To: <559BF3DE.4090908@intel.com>

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

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