[PATCH v2 1/2] drm/selftests/mm: Switch to bitmap_zalloc()

[PATCH v2 1/2] drm/selftests/mm: Switch to bitmap_zalloc()

[Andy Shevchenko]

Switch to bitmap_zalloc() to show clearly what we are allocating.
Besides that it returns pointer of bitmap type instead of opaque void *.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpu/drm/selftests/test-drm_mm.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/selftests/test-drm_mm.c b/drivers/gpu/drm/selftests/test-drm_mm.c
index fbed2c90fd51..286a0eeefcb6 100644
--- a/drivers/gpu/drm/selftests/test-drm_mm.c
+++ b/drivers/gpu/drm/selftests/test-drm_mm.c
@@ -1615,7 +1615,7 @@ static int igt_topdown(void *ignored)
 	DRM_RND_STATE(prng, random_seed);
 	const unsigned int count = 8192;
 	unsigned int size;
-	unsigned long *bitmap = NULL;
+	unsigned long *bitmap;
 	struct drm_mm mm;
 	struct drm_mm_node *nodes, *node, *next;
 	unsigned int *order, n, m, o = 0;
@@ -1631,8 +1631,7 @@ static int igt_topdown(void *ignored)
 	if (!nodes)
 		goto err;
 
-	bitmap = kcalloc(count / BITS_PER_LONG, sizeof(unsigned long),
-			 GFP_KERNEL);
+	bitmap = bitmap_zalloc(count, GFP_KERNEL);
 	if (!bitmap)
 		goto err_nodes;
 
@@ -1717,7 +1716,7 @@ static int igt_topdown(void *ignored)
 	drm_mm_takedown(&mm);
 	kfree(order);
 err_bitmap:
-	kfree(bitmap);
+	bitmap_free(bitmap);
 err_nodes:
 	vfree(nodes);
 err:
@@ -1745,8 +1744,7 @@ static int igt_bottomup(void *ignored)
 	if (!nodes)
 		goto err;
 
-	bitmap = kcalloc(count / BITS_PER_LONG, sizeof(unsigned long),
-			 GFP_KERNEL);
+	bitmap = bitmap_zalloc(count, GFP_KERNEL);
 	if (!bitmap)
 		goto err_nodes;
 
@@ -1818,7 +1816,7 @@ static int igt_bottomup(void *ignored)
 	drm_mm_takedown(&mm);
 	kfree(order);
 err_bitmap:
-	kfree(bitmap);
+	bitmap_free(bitmap);
 err_nodes:
 	vfree(nodes);
 err:
-- 
2.20.1

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

[PATCH v2 2/2] drm: i915: Switch to bitmap_zalloc()

[Andy Shevchenko]

Switch to bitmap_zalloc() to show clearly what we are allocating.
Besides that it returns pointer of bitmap type instead of opaque void *.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c               | 2 +-
 drivers/gpu/drm/i915/i915_gem_fence_reg.c     | 3 +--
 drivers/gpu/drm/i915/i915_gem_tiling.c        | 6 +++---
 drivers/gpu/drm/i915/selftests/intel_uncore.c | 5 ++---
 4 files changed, 7 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 6728ea5c71d4..0d96520cfdb3 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4410,7 +4410,7 @@ static void __i915_gem_free_objects(struct drm_i915_private *i915,
 		drm_gem_object_release(&obj->base);
 		i915_gem_info_remove_obj(i915, obj->base.size);
 
-		kfree(obj->bit_17);
+		bitmap_free(obj->bit_17);
 		i915_gem_object_free(obj);
 
 		GEM_BUG_ON(!atomic_read(&i915->mm.free_count));
diff --git a/drivers/gpu/drm/i915/i915_gem_fence_reg.c b/drivers/gpu/drm/i915/i915_gem_fence_reg.c
index e037e94792f3..1f880e5b79b0 100644
--- a/drivers/gpu/drm/i915/i915_gem_fence_reg.c
+++ b/drivers/gpu/drm/i915/i915_gem_fence_reg.c
@@ -790,8 +790,7 @@ i915_gem_object_save_bit_17_swizzle(struct drm_i915_gem_object *obj,
 	int i;
 
 	if (obj->bit_17 == NULL) {
-		obj->bit_17 = kcalloc(BITS_TO_LONGS(page_count),
-				      sizeof(long), GFP_KERNEL);
+		obj->bit_17 = bitmap_zalloc(page_count, GFP_KERNEL);
 		if (obj->bit_17 == NULL) {
 			DRM_ERROR("Failed to allocate memory for bit 17 "
 				  "record\n");
diff --git a/drivers/gpu/drm/i915/i915_gem_tiling.c b/drivers/gpu/drm/i915/i915_gem_tiling.c
index 16cc9ddbce34..a9b5329dae3b 100644
--- a/drivers/gpu/drm/i915/i915_gem_tiling.c
+++ b/drivers/gpu/drm/i915/i915_gem_tiling.c
@@ -301,11 +301,11 @@ i915_gem_object_set_tiling(struct drm_i915_gem_object *obj,
 	/* Try to preallocate memory required to save swizzling on put-pages */
 	if (i915_gem_object_needs_bit17_swizzle(obj)) {
 		if (!obj->bit_17) {
-			obj->bit_17 = kcalloc(BITS_TO_LONGS(obj->base.size >> PAGE_SHIFT),
-					      sizeof(long), GFP_KERNEL);
+			obj->bit_17 = bitmap_zalloc(obj->base.size >> PAGE_SHIFT,
+						    GFP_KERNEL);
 		}
 	} else {
-		kfree(obj->bit_17);
+		bitmap_free(obj->bit_17);
 		obj->bit_17 = NULL;
 	}
 
diff --git a/drivers/gpu/drm/i915/selftests/intel_uncore.c b/drivers/gpu/drm/i915/selftests/intel_uncore.c
index 81d9d31042a9..600167ebb303 100644
--- a/drivers/gpu/drm/i915/selftests/intel_uncore.c
+++ b/drivers/gpu/drm/i915/selftests/intel_uncore.c
@@ -137,8 +137,7 @@ static int intel_uncore_check_forcewake_domains(struct drm_i915_private *dev_pri
 	if (!IS_ENABLED(CONFIG_DRM_I915_SELFTEST_BROKEN))
 		return 0;
 
-	valid = kcalloc(BITS_TO_LONGS(FW_RANGE), sizeof(*valid),
-			GFP_KERNEL);
+	valid = bitmap_zalloc(FW_RANGE, GFP_KERNEL);
 	if (!valid)
 		return -ENOMEM;
 
@@ -173,7 +172,7 @@ static int intel_uncore_check_forcewake_domains(struct drm_i915_private *dev_pri
 		}
 	}
 
-	kfree(valid);
+	bitmap_free(valid);
 	return err;
 }
 
-- 
2.20.1

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

Re: [PATCH v2 2/2] drm: i915: Switch to bitmap_zalloc()

[Chris Wilson]

Quoting Andy Shevchenko (2019-03-04 09:29:08)
> Switch to bitmap_zalloc() to show clearly what we are allocating.
> Besides that it returns pointer of bitmap type instead of opaque void *.

Which is confusing; since we explicitly want unsigned longs, not some
amorphous bitmap type.

> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem.c               | 2 +-
>  drivers/gpu/drm/i915/i915_gem_fence_reg.c     | 3 +--
>  drivers/gpu/drm/i915/i915_gem_tiling.c        | 6 +++---
>  drivers/gpu/drm/i915/selftests/intel_uncore.c | 5 ++---
>  4 files changed, 7 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 6728ea5c71d4..0d96520cfdb3 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4410,7 +4410,7 @@ static void __i915_gem_free_objects(struct drm_i915_private *i915,
>                 drm_gem_object_release(&obj->base);
>                 i915_gem_info_remove_obj(i915, obj->base.size);
>  
> -               kfree(obj->bit_17);
> +               bitmap_free(obj->bit_17);
>                 i915_gem_object_free(obj);
>  
>                 GEM_BUG_ON(!atomic_read(&i915->mm.free_count));
> diff --git a/drivers/gpu/drm/i915/i915_gem_fence_reg.c b/drivers/gpu/drm/i915/i915_gem_fence_reg.c
> index e037e94792f3..1f880e5b79b0 100644
> --- a/drivers/gpu/drm/i915/i915_gem_fence_reg.c
> +++ b/drivers/gpu/drm/i915/i915_gem_fence_reg.c
> @@ -790,8 +790,7 @@ i915_gem_object_save_bit_17_swizzle(struct drm_i915_gem_object *obj,
>         int i;
>  
>         if (obj->bit_17 == NULL) {
> -               obj->bit_17 = kcalloc(BITS_TO_LONGS(page_count),
> -                                     sizeof(long), GFP_KERNEL);
> +               obj->bit_17 = bitmap_zalloc(page_count, GFP_KERNEL);

That feels a bit more of an overreach, as we just use bitops and never
actually use the bitmap iface.

Simply because it kills BITS_TO_LONGS(), even though I do not see why
the bitmap_[z]alloc and bitmap_free are not inlines...

And for this is not the overflow protection of kcalloc silly? We start
with a large value, factorise it, then check that the two factors do not
overflow? If it were to overflow, it would overflow in the
BITS_TO_LONGS() itself.

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 1/2] drm/selftests/mm: Switch to bitmap_zalloc()

[Chris Wilson]

Quoting Andy Shevchenko (2019-03-04 09:29:07)
> Switch to bitmap_zalloc() to show clearly what we are allocating.
> Besides that it returns pointer of bitmap type instead of opaque void *.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 2/2] drm: i915: Switch to bitmap_zalloc()

[Andy Shevchenko]

On Mon, Mar 04, 2019 at 09:41:34AM +0000, Chris Wilson wrote:
> Quoting Andy Shevchenko (2019-03-04 09:29:08)
> > Switch to bitmap_zalloc() to show clearly what we are allocating.
> > Besides that it returns pointer of bitmap type instead of opaque void *.
> 
> Which is confusing; since we explicitly want unsigned longs, not some
> amorphous bitmap type.

Why? You use it as a bitmap anyway since you are telling below you are using
bit ops like set/clear_bit.

> >         if (obj->bit_17 == NULL) {
> > -               obj->bit_17 = kcalloc(BITS_TO_LONGS(page_count),
> > -                                     sizeof(long), GFP_KERNEL);
> > +               obj->bit_17 = bitmap_zalloc(page_count, GFP_KERNEL);
> 
> That feels a bit more of an overreach, as we just use bitops and never
> actually use the bitmap iface.

bitops are _luckily_ part of bitmap iface. bitmap iface has been evolved
specifically the way the existing ops will work on it w/o any change.

> Simply because it kills BITS_TO_LONGS(), even though I do not see why
> the bitmap_[z]alloc and bitmap_free are not inlines...

Because of circular dependencies (hell) in the headers.

> And for this is not the overflow protection of kcalloc silly? We start
> with a large value, factorise it, then check that the two factors do not
> overflow? If it were to overflow, it would overflow in the
> BITS_TO_LONGS() itself.

This just a simple API change w/o functional changes.

> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

Thank you.


-- 
With Best Regards,
Andy Shevchenko


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

Re: [PATCH v2 2/2] drm: i915: Switch to bitmap_zalloc()

[Chris Wilson]

Quoting Andy Shevchenko (2019-03-04 09:54:46)
> On Mon, Mar 04, 2019 at 09:41:34AM +0000, Chris Wilson wrote:
> > Quoting Andy Shevchenko (2019-03-04 09:29:08)
> > > Switch to bitmap_zalloc() to show clearly what we are allocating.
> > > Besides that it returns pointer of bitmap type instead of opaque void *.
> > 
> > Which is confusing; since we explicitly want unsigned longs, not some
> > amorphous bitmap type.
> 
> Why? You use it as a bitmap anyway since you are telling below you are using
> bit ops like set/clear_bit.

I consider that bitmap sits on on top of the bitops iface and so we
should be using the types as defined by bitops. The allusion of "return
pointer of bitmap type" is that it may become an abstract type, ill
suited for the actual use. Just concerns over inferred language.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 1/2] drm/selftests/mm: Switch to bitmap_zalloc()

[Chris Wilson]

Quoting Chris Wilson (2019-03-04 09:43:43)
> Quoting Andy Shevchenko (2019-03-04 09:29:07)
> > Switch to bitmap_zalloc() to show clearly what we are allocating.
> > Besides that it returns pointer of bitmap type instead of opaque void *.
> > 
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

Applied to drm-misc-next.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx