[PATCH] drm/i915/selftests: Avoid drm_gem_handle_create under struct_mutex

[PATCH] drm/i915/selftests: Avoid drm_gem_handle_create under struct_mutex

[Chris Wilson]

Despite us reloading the module around every selftest, the lockclasses
persist and the chains used in selftesting may then dictate how we are
allowed to nest locks during runtime testing. As such we have to be just
as careful, and in particular it turns out we are not allowed to nest
dev->object_name_lock (drm_gem_handle_create) inside dev->struct_mutex.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=103830
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Matthew Auld <matthew.auld@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/selftests/i915_gem_context.c | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/selftests/i915_gem_context.c b/drivers/gpu/drm/i915/selftests/i915_gem_context.c
index 09340b3c1156..ec1eff739e01 100644
--- a/drivers/gpu/drm/i915/selftests/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/selftests/i915_gem_context.c
@@ -264,6 +264,23 @@ static int cpu_check(struct drm_i915_gem_object *obj, unsigned int max)
 	return err;
 }
 
+static int file_add_object(struct drm_file *file,
+			    struct drm_i915_gem_object *obj)
+{
+	int err;
+
+	GEM_BUG_ON(obj->base.handle_count);
+
+	/* tie the object to the drm_file for easy reaping */
+	err = idr_alloc(&file->object_idr, &obj->base, 1, 0, GFP_KERNEL);
+	if (err < 0)
+		return  err;
+
+	i915_gem_object_get(obj);
+	obj->base.handle_count++;
+	return 0;
+}
+
 static struct drm_i915_gem_object *
 create_test_object(struct i915_gem_context *ctx,
 		   struct drm_file *file,
@@ -273,7 +290,6 @@ create_test_object(struct i915_gem_context *ctx,
 	struct i915_address_space *vm =
 		ctx->ppgtt ? &ctx->ppgtt->base : &ctx->i915->ggtt.base;
 	u64 size;
-	u32 handle;
 	int err;
 
 	size = min(vm->total / 2, 1024ull * DW_PER_PAGE * PAGE_SIZE);
@@ -283,8 +299,7 @@ create_test_object(struct i915_gem_context *ctx,
 	if (IS_ERR(obj))
 		return obj;
 
-	/* tie the handle to the drm_file for easy reaping */
-	err = drm_gem_handle_create(file, &obj->base, &handle);
+	err = file_add_object(file, obj);
 	i915_gem_object_put(obj);
 	if (err)
 		return ERR_PTR(err);
-- 
2.15.0

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

✓ Fi.CI.BAT: success for drm/i915/selftests: Avoid drm_gem_handle_create under struct_mutex

[Patchwork]

== Series Details ==

Series: drm/i915/selftests: Avoid drm_gem_handle_create under struct_mutex
URL   : https://patchwork.freedesktop.org/series/34153/
State : success

== Summary ==

Series 34153v1 drm/i915/selftests: Avoid drm_gem_handle_create under struct_mutex
https://patchwork.freedesktop.org/api/1.0/series/34153/revisions/1/mbox/

Test gem_exec_reloc:
        Subgroup basic-write-cpu-active:
                fail       -> PASS       (fi-gdg-551) fdo#102582
Test kms_cursor_legacy:
        Subgroup basic-flip-before-cursor-varying-size:
                skip       -> PASS       (fi-hsw-4770r)
Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-b:
                incomplete -> PASS       (fi-snb-2520m) fdo#103713

fdo#102582 https://bugs.freedesktop.org/show_bug.cgi?id=102582
fdo#103713 https://bugs.freedesktop.org/show_bug.cgi?id=103713

fi-bdw-5557u     total:289  pass:268  dwarn:0   dfail:0   fail:0   skip:21  time:447s
fi-bdw-gvtdvm    total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:461s
fi-blb-e6850     total:289  pass:223  dwarn:1   dfail:0   fail:0   skip:65  time:384s
fi-bsw-n3050     total:289  pass:243  dwarn:0   dfail:0   fail:0   skip:46  time:542s
fi-bwr-2160      total:289  pass:183  dwarn:0   dfail:0   fail:0   skip:106 time:276s
fi-bxt-dsi       total:289  pass:259  dwarn:0   dfail:0   fail:0   skip:30  time:506s
fi-bxt-j4205     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:506s
fi-byt-j1900     total:289  pass:254  dwarn:0   dfail:0   fail:0   skip:35  time:496s
fi-byt-n2820     total:289  pass:250  dwarn:0   dfail:0   fail:0   skip:39  time:494s
fi-cfl-s2        total:289  pass:263  dwarn:0   dfail:0   fail:0   skip:26  time:610s
fi-elk-e7500     total:289  pass:229  dwarn:0   dfail:0   fail:0   skip:60  time:436s
fi-gdg-551       total:289  pass:178  dwarn:1   dfail:0   fail:1   skip:109 time:265s
fi-glk-1         total:289  pass:261  dwarn:0   dfail:0   fail:0   skip:28  time:539s
fi-hsw-4770      total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:432s
fi-hsw-4770r     total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:440s
fi-ilk-650       total:289  pass:228  dwarn:0   dfail:0   fail:0   skip:61  time:432s
fi-ivb-3520m     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:484s
fi-ivb-3770      total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:479s
fi-kbl-7500u     total:289  pass:264  dwarn:1   dfail:0   fail:0   skip:24  time:488s
fi-kbl-7560u     total:289  pass:270  dwarn:0   dfail:0   fail:0   skip:19  time:540s
fi-kbl-7567u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:476s
fi-kbl-r         total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:535s
fi-pnv-d510      total:289  pass:222  dwarn:1   dfail:0   fail:0   skip:66  time:573s
fi-skl-6260u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:450s
fi-skl-6600u     total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:541s
fi-skl-6700hq    total:289  pass:263  dwarn:0   dfail:0   fail:0   skip:26  time:566s
fi-skl-6700k     total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:520s
fi-skl-6770hq    total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:499s
fi-skl-gvtdvm    total:289  pass:266  dwarn:0   dfail:0   fail:0   skip:23  time:457s
fi-snb-2520m     total:289  pass:250  dwarn:0   dfail:0   fail:0   skip:39  time:563s
fi-snb-2600      total:289  pass:249  dwarn:0   dfail:0   fail:0   skip:40  time:421s
Blacklisted hosts:
fi-glk-dsi       total:161  pass:73   dwarn:0   dfail:1   fail:0   skip:86 

ee3fc3c956f817479cfe2bac3cc2a72112dbdec1 drm-tip: 2017y-11m-21d-09h-02m-11s UTC integration manifest
b32bc6d76c6e drm/i915/selftests: Avoid drm_gem_handle_create under struct_mutex

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7217/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.IGT: success for drm/i915/selftests: Avoid drm_gem_handle_create under struct_mutex

[Patchwork]

== Series Details ==

Series: drm/i915/selftests: Avoid drm_gem_handle_create under struct_mutex
URL   : https://patchwork.freedesktop.org/series/34153/
State : success

== Summary ==

Warning: bzip CI_DRM_3367/shard-glkb3/results12.json.bz2 wasn't in correct JSON format
Test gem_busy:
        Subgroup close-race:
                pass       -> FAIL       (shard-snb) fdo#103829
Test kms_frontbuffer_tracking:
        Subgroup fbc-1p-offscren-pri-shrfb-draw-blt:
                fail       -> PASS       (shard-snb) fdo#101623
Test perf:
        Subgroup polling:
                pass       -> FAIL       (shard-hsw) fdo#102252
Test kms_flip:
        Subgroup flip-vs-expired-vblank:
                fail       -> PASS       (shard-hsw) fdo#102887

fdo#103829 https://bugs.freedesktop.org/show_bug.cgi?id=103829
fdo#101623 https://bugs.freedesktop.org/show_bug.cgi?id=101623
fdo#102252 https://bugs.freedesktop.org/show_bug.cgi?id=102252
fdo#102887 https://bugs.freedesktop.org/show_bug.cgi?id=102887

shard-hsw        total:2585 pass:1473 dwarn:2   dfail:1   fail:10  skip:1099 time:9491s
shard-snb        total:2585 pass:1257 dwarn:2   dfail:1   fail:12  skip:1313 time:8012s
Blacklisted hosts:
shard-apl        total:2563 pass:1602 dwarn:1   dfail:0   fail:23  skip:936 time:13116s
shard-kbl        total:2537 pass:1678 dwarn:3   dfail:1   fail:25  skip:828 time:10101s

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7217/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/selftests: Avoid drm_gem_handle_create under struct_mutex

[Joonas Lahtinen]

On Tue, 2017-11-21 at 11:06 +0000, Chris Wilson wrote:
> Despite us reloading the module around every selftest, the lockclasses
> persist and the chains used in selftesting may then dictate how we are
> allowed to nest locks during runtime testing. As such we have to be just
> as careful, and in particular it turns out we are not allowed to nest
> dev->object_name_lock (drm_gem_handle_create) inside dev->struct_mutex.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=103830
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Matthew Auld <matthew.auld@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

<SNIP>

> @@ -283,8 +299,7 @@ create_test_object(struct i915_gem_context *ctx,
>  	if (IS_ERR(obj))
>  		return obj;
>  
> -	/* tie the handle to the drm_file for easy reaping */
> -	err = drm_gem_handle_create(file, &obj->base, &handle);
> +	err = file_add_object(file, obj);

Should we have a more direct connection between the substituted mock
function names? So should we have mock_drm_gem_handle_create, and it'll
then mock that function only for the interesting parts, like here.

I know it might be equally confusing, too, when only a small portion of
the function is mocked. Especially if it's more of a side-effect like
here.

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] drm/i915/selftests: Avoid drm_gem_handle_create under struct_mutex

[Chris Wilson]

Quoting Joonas Lahtinen (2017-11-21 13:01:43)
> On Tue, 2017-11-21 at 11:06 +0000, Chris Wilson wrote:
> > Despite us reloading the module around every selftest, the lockclasses
> > persist and the chains used in selftesting may then dictate how we are
> > allowed to nest locks during runtime testing. As such we have to be just
> > as careful, and in particular it turns out we are not allowed to nest
> > dev->object_name_lock (drm_gem_handle_create) inside dev->struct_mutex.
> > 
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=103830
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Matthew Auld <matthew.auld@intel.com>
> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> <SNIP>
> 
> > @@ -283,8 +299,7 @@ create_test_object(struct i915_gem_context *ctx,
> >       if (IS_ERR(obj))
> >               return obj;
> >  
> > -     /* tie the handle to the drm_file for easy reaping */
> > -     err = drm_gem_handle_create(file, &obj->base, &handle);
> > +     err = file_add_object(file, obj);
> 
> Should we have a more direct connection between the substituted mock
> function names? So should we have mock_drm_gem_handle_create, and it'll
> then mock that function only for the interesting parts, like here.

I was thinking more along the lines of trying to split drm_gem.c, to
provide internal access. Along those lines we could start moving some of
the grubby bending-of-the-rules stuff we want to test those rules into
drm itself.

Hmm, that's an idea. Certainly we need more coverage of things like the
drm_vma_manager api, so the drm-st library will need to grow.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx