[PATCH v2 1/3] drm/i915: Bump object bookkeeping to u64 from size_t

[PATCH v2 1/3] drm/i915: Bump object bookkeeping to u64 from size_t

[Chris Wilson]

Internally we allow for using more objects than a single process can
allocate, i.e. we allow for a 64bit GPU address space even on a 32bit
system. Using size_t may oveerflow.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 2 +-
 drivers/gpu/drm/i915/i915_drv.h     | 2 +-
 drivers/gpu/drm/i915/i915_gem.c     | 4 ++--
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 515c206ba653..dc057c770146 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -392,7 +392,7 @@ static int i915_gem_object_info(struct seq_file *m, void *data)
 	if (ret)
 		return ret;
 
-	seq_printf(m, "%u objects, %zu bytes\n",
+	seq_printf(m, "%u objects, %llu bytes\n",
 		   dev_priv->mm.object_count,
 		   dev_priv->mm.object_memory);
 
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 4d1133ffe093..092c5a0a44f0 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1389,7 +1389,7 @@ struct i915_gem_mm {
 
 	/* accounting, useful for userland debugging */
 	spinlock_t object_stat_lock;
-	size_t object_memory;
+	u64 object_memory;
 	u32 object_count;
 };
 
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index fe92e28ea0a8..838dc159a2d1 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -82,7 +82,7 @@ remove_mappable_node(struct drm_mm_node *node)
 
 /* some bookkeeping */
 static void i915_gem_info_add_obj(struct drm_i915_private *dev_priv,
-				  size_t size)
+				  u64 size)
 {
 	spin_lock(&dev_priv->mm.object_stat_lock);
 	dev_priv->mm.object_count++;
@@ -91,7 +91,7 @@ static void i915_gem_info_add_obj(struct drm_i915_private *dev_priv,
 }
 
 static void i915_gem_info_remove_obj(struct drm_i915_private *dev_priv,
-				     size_t size)
+				     u64 size)
 {
 	spin_lock(&dev_priv->mm.object_stat_lock);
 	dev_priv->mm.object_count--;
-- 
2.9.3

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

[PATCH v2 2/3] drm/i915: Document our internal limit on object size

[Chris Wilson]

In many places, we try to count pages using a 32 bit integer. That
implies if we are asked to create an object larger than 43bits, we will
subtly crash much later. Catch this on the boundary, and add a warning
to remind ourselves later on our exabyte systems.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_drv.h |  2 +-
 drivers/gpu/drm/i915/i915_gem.c | 17 +++++++++++++++--
 2 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 092c5a0a44f0..a2b5fc72fdd9 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3105,7 +3105,7 @@ void i915_gem_object_free(struct drm_i915_gem_object *obj);
 void i915_gem_object_init(struct drm_i915_gem_object *obj,
 			 const struct drm_i915_gem_object_ops *ops);
 struct drm_i915_gem_object *i915_gem_object_create(struct drm_device *dev,
-						  size_t size);
+						   u64 size);
 struct drm_i915_gem_object *i915_gem_object_create_from_data(
 		struct drm_device *dev, const void *data, size_t size);
 void i915_gem_close_object(struct drm_gem_object *gem, struct drm_file *file);
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 838dc159a2d1..181bda2db587 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4131,14 +4131,27 @@ static const struct drm_i915_gem_object_ops i915_gem_object_ops = {
 	.put_pages = i915_gem_object_put_pages_gtt,
 };
 
-struct drm_i915_gem_object *i915_gem_object_create(struct drm_device *dev,
-						  size_t size)
+#define overflows_type(x, T) \
+	(sizeof(x) > sizeof(T) && (x) >> (sizeof(T) * BITS_PER_BYTE))
+
+struct drm_i915_gem_object *
+i915_gem_object_create(struct drm_device *dev, u64 size)
 {
 	struct drm_i915_gem_object *obj;
 	struct address_space *mapping;
 	gfp_t mask;
 	int ret;
 
+	/* There is a prevalence of the assumption that we fit the object's
+	 * page count inside a 32bit variable. Let's document this and catch
+	 * if we ever need to fix it.
+	 */
+	if (WARN_ON(size >> PAGE_SHIFT > INT_MAX))
+		return ERR_PTR(-E2BIG);
+
+	if (overflows_type(size, obj->base.size))
+		return ERR_PTR(-E2BIG);
+
 	obj = i915_gem_object_alloc(dev);
 	if (obj == NULL)
 		return ERR_PTR(-ENOMEM);
-- 
2.9.3

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

[PATCH v2 3/3] drm/i915: Limit the scattergather coalescing to 32bits

[Chris Wilson]

The scattergather list uses a 32bit size counter, we should avoid
exceeding it.

v2: Also we should use unsigned int to match sg->length.

Fixes: 871dfbd67d4e ("drm/i915: Allow compaction upto SWIOTLB max segment size")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> # v1
---
 drivers/gpu/drm/i915/i915_gem.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 181bda2db587..a74ec10a5370 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2205,7 +2205,7 @@ i915_gem_object_put_pages(struct drm_i915_gem_object *obj)
 	return 0;
 }
 
-static unsigned long swiotlb_max_size(void)
+static unsigned int swiotlb_max_size(void)
 {
 #if IS_ENABLED(CONFIG_SWIOTLB)
 	return rounddown(swiotlb_nr_tbl() << IO_TLB_SHIFT, PAGE_SIZE);
@@ -2225,7 +2225,7 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
 	struct sgt_iter sgt_iter;
 	struct page *page;
 	unsigned long last_pfn = 0;	/* suppress gcc warning */
-	unsigned long max_segment;
+	unsigned int max_segment;
 	int ret;
 	gfp_t gfp;
 
@@ -2238,7 +2238,7 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
 
 	max_segment = swiotlb_max_size();
 	if (!max_segment)
-		max_segment = obj->base.size;
+		max_segment = rounddown(UINT_MAX, PAGE_SIZE);
 
 	st = kmalloc(sizeof(*st), GFP_KERNEL);
 	if (st == NULL)
-- 
2.9.3

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

✗ Fi.CI.BAT: warning for series starting with [v2,1/3] drm/i915: Bump object bookkeeping to u64 from size_t

[Patchwork]

== Series Details ==

Series: series starting with [v2,1/3] drm/i915: Bump object bookkeeping to u64 from size_t
URL   : https://patchwork.freedesktop.org/series/13853/
State : warning

== Summary ==

Series 13853v1 Series without cover letter
https://patchwork.freedesktop.org/api/1.0/series/13853/revisions/1/mbox/

Test vgem_basic:
        Subgroup unload:
                skip       -> PASS       (fi-skl-6770hq)
                pass       -> SKIP       (fi-hsw-4770)

fi-bdw-5557u     total:246  pass:231  dwarn:0   dfail:0   fail:0   skip:15 
fi-bsw-n3050     total:246  pass:204  dwarn:0   dfail:0   fail:0   skip:42 
fi-bxt-t5700     total:246  pass:216  dwarn:0   dfail:0   fail:0   skip:30 
fi-byt-j1900     total:246  pass:212  dwarn:2   dfail:0   fail:1   skip:31 
fi-byt-n2820     total:246  pass:210  dwarn:0   dfail:0   fail:1   skip:35 
fi-hsw-4770      total:246  pass:223  dwarn:0   dfail:0   fail:0   skip:23 
fi-hsw-4770r     total:246  pass:224  dwarn:0   dfail:0   fail:0   skip:22 
fi-ilk-650       total:246  pass:184  dwarn:0   dfail:0   fail:2   skip:60 
fi-ivb-3520m     total:246  pass:221  dwarn:0   dfail:0   fail:0   skip:25 
fi-ivb-3770      total:246  pass:221  dwarn:0   dfail:0   fail:0   skip:25 
fi-kbl-7200u     total:246  pass:222  dwarn:0   dfail:0   fail:0   skip:24 
fi-skl-6260u     total:246  pass:232  dwarn:0   dfail:0   fail:0   skip:14 
fi-skl-6700hq    total:246  pass:223  dwarn:0   dfail:0   fail:0   skip:23 
fi-skl-6700k     total:246  pass:221  dwarn:1   dfail:0   fail:0   skip:24 
fi-skl-6770hq    total:246  pass:230  dwarn:1   dfail:0   fail:1   skip:14 
fi-snb-2520m     total:246  pass:210  dwarn:0   dfail:0   fail:0   skip:36 
fi-snb-2600      total:246  pass:209  dwarn:0   dfail:0   fail:0   skip:37 

Results at /archive/results/CI_IGT_test/Patchwork_2733/

15dfed2b90e84e7c277f81842fc3f19355293061 drm-intel-nightly: 2016y-10m-16d-23h-15m-00s UTC integration manifest
f67f72a drm/i915: Limit the scattergather coalescing to 32bits
21a9dbe drm/i915: Document our internal limit on object size
f70eb45 drm/i915: Bump object bookkeeping to u64 from size_t

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

Re: [PATCH v2 1/3] drm/i915: Bump object bookkeeping to u64 from size_t

[Joonas Lahtinen]

On ma, 2016-10-17 at 09:00 +0100, Chris Wilson wrote:
> Internally we allow for using more objects than a single process can
> allocate, i.e. we allow for a 64bit GPU address space even on a 32bit
> system. Using size_t may oveerflow.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

How might we get more than size_t passed in? I only notice a single
instance which uses obj.base->size which is size_t.

So I'd not change the function parameters, only the sum. With that;

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 1/3] drm/i915: Bump object bookkeeping to u64 from size_t

[Chris Wilson]

On Mon, Oct 17, 2016 at 12:48:17PM +0300, Joonas Lahtinen wrote:
> On ma, 2016-10-17 at 09:00 +0100, Chris Wilson wrote:
> > Internally we allow for using more objects than a single process can
> > allocate, i.e. we allow for a 64bit GPU address space even on a 32bit
> > system. Using size_t may oveerflow.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> 
> How might we get more than size_t passed in? I only notice a single
> instance which uses obj.base->size which is size_t.
> 
> So I'd not change the function parameters, only the sum. With that;

Our ABI is u64. The values we pass around are u64. Only we chose to use
size_t for the object, go figure.
-Chris

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

Re: [PATCH v2 1/3] drm/i915: Bump object bookkeeping to u64 from size_t

[Tvrtko Ursulin]

On 17/10/2016 09:00, Chris Wilson wrote:
> Internally we allow for using more objects than a single process can
> allocate, i.e. we allow for a 64bit GPU address space even on a 32bit
> system. Using size_t may oveerflow.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>   drivers/gpu/drm/i915/i915_debugfs.c | 2 +-
>   drivers/gpu/drm/i915/i915_drv.h     | 2 +-
>   drivers/gpu/drm/i915/i915_gem.c     | 4 ++--
>   3 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 515c206ba653..dc057c770146 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -392,7 +392,7 @@ static int i915_gem_object_info(struct seq_file *m, void *data)
>   	if (ret)
>   		return ret;
>   
> -	seq_printf(m, "%u objects, %zu bytes\n",
> +	seq_printf(m, "%u objects, %llu bytes\n",
>   		   dev_priv->mm.object_count,
>   		   dev_priv->mm.object_memory);
>   
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 4d1133ffe093..092c5a0a44f0 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1389,7 +1389,7 @@ struct i915_gem_mm {
>   
>   	/* accounting, useful for userland debugging */
>   	spinlock_t object_stat_lock;
> -	size_t object_memory;
> +	u64 object_memory;
>   	u32 object_count;
>   };
>   
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index fe92e28ea0a8..838dc159a2d1 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -82,7 +82,7 @@ remove_mappable_node(struct drm_mm_node *node)
>   
>   /* some bookkeeping */
>   static void i915_gem_info_add_obj(struct drm_i915_private *dev_priv,
> -				  size_t size)
> +				  u64 size)
>   {
>   	spin_lock(&dev_priv->mm.object_stat_lock);
>   	dev_priv->mm.object_count++;
> @@ -91,7 +91,7 @@ static void i915_gem_info_add_obj(struct drm_i915_private *dev_priv,
>   }
>   
>   static void i915_gem_info_remove_obj(struct drm_i915_private *dev_priv,
> -				     size_t size)
> +				     u64 size)
>   {
>   	spin_lock(&dev_priv->mm.object_stat_lock);
>   	dev_priv->mm.object_count--;

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko

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

Re: [PATCH v2 2/3] drm/i915: Document our internal limit on object size

[Tvrtko Ursulin]

On 17/10/2016 09:00, Chris Wilson wrote:
> In many places, we try to count pages using a 32 bit integer. That
> implies if we are asked to create an object larger than 43bits, we will
> subtly crash much later. Catch this on the boundary, and add a warning
> to remind ourselves later on our exabyte systems.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>   drivers/gpu/drm/i915/i915_drv.h |  2 +-
>   drivers/gpu/drm/i915/i915_gem.c | 17 +++++++++++++++--
>   2 files changed, 16 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 092c5a0a44f0..a2b5fc72fdd9 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -3105,7 +3105,7 @@ void i915_gem_object_free(struct drm_i915_gem_object *obj);
>   void i915_gem_object_init(struct drm_i915_gem_object *obj,
>   			 const struct drm_i915_gem_object_ops *ops);
>   struct drm_i915_gem_object *i915_gem_object_create(struct drm_device *dev,
> -						  size_t size);
> +						   u64 size);
>   struct drm_i915_gem_object *i915_gem_object_create_from_data(
>   		struct drm_device *dev, const void *data, size_t size);
>   void i915_gem_close_object(struct drm_gem_object *gem, struct drm_file *file);
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 838dc159a2d1..181bda2db587 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4131,14 +4131,27 @@ static const struct drm_i915_gem_object_ops i915_gem_object_ops = {
>   	.put_pages = i915_gem_object_put_pages_gtt,
>   };
>   
> -struct drm_i915_gem_object *i915_gem_object_create(struct drm_device *dev,
> -						  size_t size)
> +#define overflows_type(x, T) \
> +	(sizeof(x) > sizeof(T) && (x) >> (sizeof(T) * BITS_PER_BYTE))
> +

Looks like it wouldn't detect storing unsigned int in a signed int but I 
guess we don't care that much as long as this is local use only. Just 
slightly relevant because of the int page_count situation we mention below.

> +struct drm_i915_gem_object *
> +i915_gem_object_create(struct drm_device *dev, u64 size)
>   {
>   	struct drm_i915_gem_object *obj;
>   	struct address_space *mapping;
>   	gfp_t mask;
>   	int ret;
>   
> +	/* There is a prevalence of the assumption that we fit the object's
> +	 * page count inside a 32bit variable. Let's document this and catch

_Signed_ 32-bit integer as you have explained to justify the INT_MAX below.

> +	 * if we ever need to fix it.
> +	 */
> +	if (WARN_ON(size >> PAGE_SHIFT > INT_MAX))
> +		return ERR_PTR(-E2BIG);
> +
> +	if (overflows_type(size, obj->base.size))
> +		return ERR_PTR(-E2BIG);
> +
>   	obj = i915_gem_object_alloc(dev);
>   	if (obj == NULL)
>   		return ERR_PTR(-ENOMEM);

With the comment clarification,

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko

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

Re: [PATCH v2 3/3] drm/i915: Limit the scattergather coalescing to 32bits

[Tvrtko Ursulin]

On 17/10/2016 09:00, Chris Wilson wrote:
> The scattergather list uses a 32bit size counter, we should avoid
> exceeding it.
>
> v2: Also we should use unsigned int to match sg->length.
>
> Fixes: 871dfbd67d4e ("drm/i915: Allow compaction upto SWIOTLB max segment size")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> # v1
> ---
>   drivers/gpu/drm/i915/i915_gem.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 181bda2db587..a74ec10a5370 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2205,7 +2205,7 @@ i915_gem_object_put_pages(struct drm_i915_gem_object *obj)
>   	return 0;
>   }
>   
> -static unsigned long swiotlb_max_size(void)
> +static unsigned int swiotlb_max_size(void)
>   {
>   #if IS_ENABLED(CONFIG_SWIOTLB)
>   	return rounddown(swiotlb_nr_tbl() << IO_TLB_SHIFT, PAGE_SIZE);
> @@ -2225,7 +2225,7 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
>   	struct sgt_iter sgt_iter;
>   	struct page *page;
>   	unsigned long last_pfn = 0;	/* suppress gcc warning */
> -	unsigned long max_segment;
> +	unsigned int max_segment;
>   	int ret;
>   	gfp_t gfp;
>   
> @@ -2238,7 +2238,7 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
>   
>   	max_segment = swiotlb_max_size();
>   	if (!max_segment)
> -		max_segment = obj->base.size;
> +		max_segment = rounddown(UINT_MAX, PAGE_SIZE);

rounddown looks like an overkill vs just UINT_MAX, no?

>   
>   	st = kmalloc(sizeof(*st), GFP_KERNEL);
>   	if (st == NULL)

Anyway,

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko

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

Re: [PATCH v2 2/3] drm/i915: Document our internal limit on object size

[Chris Wilson]

On Tue, Oct 18, 2016 at 10:27:58AM +0100, Tvrtko Ursulin wrote:
> 
> On 17/10/2016 09:00, Chris Wilson wrote:
> >In many places, we try to count pages using a 32 bit integer. That
> >implies if we are asked to create an object larger than 43bits, we will
> >subtly crash much later. Catch this on the boundary, and add a warning
> >to remind ourselves later on our exabyte systems.
> >
> >Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >---
> >  drivers/gpu/drm/i915/i915_drv.h |  2 +-
> >  drivers/gpu/drm/i915/i915_gem.c | 17 +++++++++++++++--
> >  2 files changed, 16 insertions(+), 3 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> >index 092c5a0a44f0..a2b5fc72fdd9 100644
> >--- a/drivers/gpu/drm/i915/i915_drv.h
> >+++ b/drivers/gpu/drm/i915/i915_drv.h
> >@@ -3105,7 +3105,7 @@ void i915_gem_object_free(struct drm_i915_gem_object *obj);
> >  void i915_gem_object_init(struct drm_i915_gem_object *obj,
> >  			 const struct drm_i915_gem_object_ops *ops);
> >  struct drm_i915_gem_object *i915_gem_object_create(struct drm_device *dev,
> >-						  size_t size);
> >+						   u64 size);
> >  struct drm_i915_gem_object *i915_gem_object_create_from_data(
> >  		struct drm_device *dev, const void *data, size_t size);
> >  void i915_gem_close_object(struct drm_gem_object *gem, struct drm_file *file);
> >diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> >index 838dc159a2d1..181bda2db587 100644
> >--- a/drivers/gpu/drm/i915/i915_gem.c
> >+++ b/drivers/gpu/drm/i915/i915_gem.c
> >@@ -4131,14 +4131,27 @@ static const struct drm_i915_gem_object_ops i915_gem_object_ops = {
> >  	.put_pages = i915_gem_object_put_pages_gtt,
> >  };
> >-struct drm_i915_gem_object *i915_gem_object_create(struct drm_device *dev,
> >-						  size_t size)
> >+#define overflows_type(x, T) \
> >+	(sizeof(x) > sizeof(T) && (x) >> (sizeof(T) * BITS_PER_BYTE))
> >+
> 
> Looks like it wouldn't detect storing unsigned int in a signed int
> but I guess we don't care that much as long as this is local use
> only. Just slightly relevant because of the int page_count situation
> we mention below.

Hmm. Yeah, definitely worth improving. Quick googling shows that you are
the first to notice! :-p

I was thinking of trying gcc's __builtin_add_overflowp(x, 0, T) or
something like that.

But I also wonder if we can use signed T *var vs unsigned T *var in any
way to generalise the number of positive bits in a type.
-Chris

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

Re: [PATCH v2 3/3] drm/i915: Limit the scattergather coalescing to 32bits

[Chris Wilson]

On Tue, Oct 18, 2016 at 10:33:11AM +0100, Tvrtko Ursulin wrote:
> 
> On 17/10/2016 09:00, Chris Wilson wrote:
> >The scattergather list uses a 32bit size counter, we should avoid
> >exceeding it.
> >
> >v2: Also we should use unsigned int to match sg->length.
> >
> >Fixes: 871dfbd67d4e ("drm/i915: Allow compaction upto SWIOTLB max segment size")
> >Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> # v1
> >---
> >  drivers/gpu/drm/i915/i915_gem.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> >index 181bda2db587..a74ec10a5370 100644
> >--- a/drivers/gpu/drm/i915/i915_gem.c
> >+++ b/drivers/gpu/drm/i915/i915_gem.c
> >@@ -2205,7 +2205,7 @@ i915_gem_object_put_pages(struct drm_i915_gem_object *obj)
> >  	return 0;
> >  }
> >-static unsigned long swiotlb_max_size(void)
> >+static unsigned int swiotlb_max_size(void)
> >  {
> >  #if IS_ENABLED(CONFIG_SWIOTLB)
> >  	return rounddown(swiotlb_nr_tbl() << IO_TLB_SHIFT, PAGE_SIZE);
> >@@ -2225,7 +2225,7 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
> >  	struct sgt_iter sgt_iter;
> >  	struct page *page;
> >  	unsigned long last_pfn = 0;	/* suppress gcc warning */
> >-	unsigned long max_segment;
> >+	unsigned int max_segment;
> >  	int ret;
> >  	gfp_t gfp;
> >@@ -2238,7 +2238,7 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
> >  	max_segment = swiotlb_max_size();
> >  	if (!max_segment)
> >-		max_segment = obj->base.size;
> >+		max_segment = rounddown(UINT_MAX, PAGE_SIZE);
> 
> rounddown looks like an overkill vs just UINT_MAX, no?

I think there is a subtle difference becuase we are couting in pages and
compare sg->length >= max_segment. If we left it at UINT_MAX, sg->length
would wrap to zero rather than start a new sg.
-Chris

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