[PATCH 1/3] drm/i915: Assert the context is not closed on object-close

[PATCH 1/3] drm/i915: Assert the context is not closed on object-close

[Chris Wilson]

During the context-close, we should be decoupling all the vma from the
object so that upon object-closing we shouldn't see any vma from the
already closed contexts. So include a check upon closing the object that
the context is still open.

v2: Eek, the fpriv check is required for shared objects. Double eek, BAT
passed?

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index b9e8e0d6e97b..3ed9fb0921e2 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3253,11 +3253,11 @@ void i915_gem_close_object(struct drm_gem_object *gem, struct drm_file *file)
 		struct i915_gem_context *ctx = lut->ctx;
 		struct i915_vma *vma;
 
+		GEM_BUG_ON(ctx->file_priv == ERR_PTR(-EBADF));
 		if (ctx->file_priv != fpriv)
 			continue;
 
 		vma = radix_tree_delete(&ctx->handles_vma, lut->handle);
-
 		if (!i915_vma_is_ggtt(vma))
 			i915_vma_close(vma);
 
-- 
2.14.1

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

[PATCH 2/3] drm/i915: Assert that the handle->vma lut is empty on object close

[Chris Wilson]

Make sure that we are not leaking an entry in the ctx->handles_lut by
asserting that the object was removed prior to being freed. This should
be enforced by all such handles being removed by i915_gem_close_object.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 3ed9fb0921e2..5dc396c20c06 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4416,6 +4416,7 @@ static void __i915_gem_free_objects(struct drm_i915_private *i915,
 	llist_for_each_entry_safe(obj, on, freed, freed) {
 		GEM_BUG_ON(obj->bind_count);
 		GEM_BUG_ON(atomic_read(&obj->frontbuffer_bits));
+		GEM_BUG_ON(!list_empty(&obj->lut_list));
 
 		if (obj->ops->release)
 			obj->ops->release(obj);
-- 
2.14.1

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

[PATCH 3/3] drm/i915: Ignore duplicate VMA stored within the per-object handle LUT

[Chris Wilson]

By using drm_gem_flink/drm_gem_open on an object using the same fd, it
is possible for a client to create multiple handles pointing to the same
object (tied to the same contexts and VMA), as exemplified by
igt::gem_handle_to_libdrm_bo(). Since this duplication has been possible
since forever, we cannot assume that the handle:(fpriv, object) is
unique and so must handle the multiple users of a single VMA.

Testcase: igt/gem_close
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=102355
Fixes: d1b48c1e7184 ("drm/i915: Replace execbuf vma ht with an idr")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c            | 8 +++++++-
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 1 +
 drivers/gpu/drm/i915/i915_vma.h            | 1 +
 3 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 5dc396c20c06..ac02785fdaff 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3258,7 +3258,13 @@ void i915_gem_close_object(struct drm_gem_object *gem, struct drm_file *file)
 			continue;
 
 		vma = radix_tree_delete(&ctx->handles_vma, lut->handle);
-		if (!i915_vma_is_ggtt(vma))
+		GEM_BUG_ON(vma->obj != obj);
+
+		/* We allow the process to have multiple handles to the same
+		 * vma, in the same fd namespace, by virtue of flink/open.
+		 */
+		GEM_BUG_ON(!vma->open_count);
+		if (!--vma->open_count && !i915_vma_is_ggtt(vma))
 			i915_vma_close(vma);
 
 		list_del(&lut->obj_link);
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 3d74f3a27c13..1c4fac032329 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -720,6 +720,7 @@ static int eb_lookup_vmas(struct i915_execbuffer *eb)
 			goto err_obj;
 		}
 
+		vma->open_count++;
 		list_add(&lut->obj_link, &obj->lut_list);
 		list_add(&lut->ctx_link, &eb->ctx->handles_list);
 		lut->ctx = eb->ctx;
diff --git a/drivers/gpu/drm/i915/i915_vma.h b/drivers/gpu/drm/i915/i915_vma.h
index 1fd61e88cfd0..893467a28801 100644
--- a/drivers/gpu/drm/i915/i915_vma.h
+++ b/drivers/gpu/drm/i915/i915_vma.h
@@ -59,6 +59,7 @@ struct i915_vma {
 	u32 fence_size;
 	u32 fence_alignment;
 
+	unsigned int open_count;
 	unsigned int flags;
 	/**
 	 * How many users have pinned this object in GTT space. The following
-- 
2.14.1

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

Re: [PATCH 1/3] drm/i915: Assert the context is not closed on object-close

[Lofstedt, Marta]

Thanks Chris,
With this series the test pin-pointed in the bug now pass.

Tested-by: Marta Lofstedt <marta.lofstedt@intel.com>

> -----Original Message-----
> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf
> Of Chris Wilson
> Sent: Tuesday, August 22, 2017 2:05 PM
> To: intel-gfx@lists.freedesktop.org
> Subject: [Intel-gfx] [PATCH 1/3] drm/i915: Assert the context is not closed on
> object-close
> 
> During the context-close, we should be decoupling all the vma from the
> object so that upon object-closing we shouldn't see any vma from the
> already closed contexts. So include a check upon closing the object that the
> context is still open.
> 
> v2: Eek, the fpriv check is required for shared objects. Double eek, BAT
> passed?

Well, the KBL-shards results actually exposed the regression: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_5429/shards-all.html
So, could you remove that comment.

> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_gem.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c
> b/drivers/gpu/drm/i915/i915_gem.c index b9e8e0d6e97b..3ed9fb0921e2
> 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3253,11 +3253,11 @@ void i915_gem_close_object(struct
> drm_gem_object *gem, struct drm_file *file)
>  		struct i915_gem_context *ctx = lut->ctx;
>  		struct i915_vma *vma;
> 
> +		GEM_BUG_ON(ctx->file_priv == ERR_PTR(-
> EBADF));
>  		if (ctx->file_priv != fpriv)
>  			continue;
> 
>  		vma = radix_tree_delete(&ctx->handles_vma,
> lut->handle);
> -
>  		if (!i915_vma_is_ggtt(vma))
>  			i915_vma_close(vma);
> 
> --
> 2.14.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for series starting with [1/3] drm/i915: Assert the context is not closed on object-close

[Patchwork]

== Series Details ==

Series: series starting with [1/3] drm/i915: Assert the context is not closed on object-close
URL   : https://patchwork.freedesktop.org/series/29137/
State : success

== Summary ==

Series 29137v1 series starting with [1/3] drm/i915: Assert the context is not closed on object-close
https://patchwork.freedesktop.org/api/1.0/series/29137/revisions/1/mbox/

Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-b:
                dmesg-warn -> PASS       (fi-byt-n2820) fdo#101705

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

fi-bdw-5557u     total:279  pass:268  dwarn:0   dfail:0   fail:0   skip:11  time:452s
fi-bdw-gvtdvm    total:279  pass:265  dwarn:0   dfail:0   fail:0   skip:14  time:438s
fi-blb-e6850     total:279  pass:224  dwarn:1   dfail:0   fail:0   skip:54  time:361s
fi-bsw-n3050     total:279  pass:243  dwarn:0   dfail:0   fail:0   skip:36  time:542s
fi-bwr-2160      total:279  pass:184  dwarn:0   dfail:0   fail:0   skip:95  time:252s
fi-bxt-j4205     total:279  pass:260  dwarn:0   dfail:0   fail:0   skip:19  time:523s
fi-byt-j1900     total:279  pass:254  dwarn:1   dfail:0   fail:0   skip:24  time:525s
fi-byt-n2820     total:279  pass:251  dwarn:0   dfail:0   fail:0   skip:28  time:520s
fi-elk-e7500     total:279  pass:230  dwarn:0   dfail:0   fail:0   skip:49  time:433s
fi-glk-2a        total:279  pass:260  dwarn:0   dfail:0   fail:0   skip:19  time:616s
fi-hsw-4770      total:279  pass:263  dwarn:0   dfail:0   fail:0   skip:16  time:445s
fi-hsw-4770r     total:279  pass:263  dwarn:0   dfail:0   fail:0   skip:16  time:421s
fi-ilk-650       total:279  pass:229  dwarn:0   dfail:0   fail:0   skip:50  time:428s
fi-ivb-3520m     total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:511s
fi-ivb-3770      total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:477s
fi-kbl-7260u     total:279  pass:268  dwarn:1   dfail:0   fail:0   skip:10  time:495s
fi-kbl-7500u     total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:476s
fi-kbl-7560u     total:279  pass:269  dwarn:0   dfail:0   fail:0   skip:10  time:600s
fi-kbl-r         total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:596s
fi-pnv-d510      total:279  pass:223  dwarn:1   dfail:0   fail:0   skip:55  time:527s
fi-skl-6260u     total:279  pass:269  dwarn:0   dfail:0   fail:0   skip:10  time:470s
fi-skl-6700k     total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:480s
fi-skl-6770hq    total:279  pass:269  dwarn:0   dfail:0   fail:0   skip:10  time:488s
fi-skl-gvtdvm    total:279  pass:266  dwarn:0   dfail:0   fail:0   skip:13  time:445s
fi-skl-x1585l    total:279  pass:268  dwarn:0   dfail:0   fail:0   skip:11  time:481s
fi-snb-2520m     total:279  pass:251  dwarn:0   dfail:0   fail:0   skip:28  time:549s
fi-snb-2600      total:279  pass:250  dwarn:0   dfail:0   fail:0   skip:29  time:403s

93365f59a9908f8aa7858bca57338967a20a75b7 drm-tip: 2017y-08m-22d-11h-29m-39s UTC integration manifest
4ef2c3816638 drm/i915: Ignore duplicate VMA stored within the per-object handle LUT
e43e7ce92969 drm/i915: Assert that the handle->vma lut is empty on object close
a29adf5cbaec drm/i915: Assert the context is not closed on object-close

== Logs ==

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

Re: [PATCH 3/3] drm/i915: Ignore duplicate VMA stored within the per-object handle LUT

[Joonas Lahtinen]

On Tue, 2017-08-22 at 12:05 +0100, Chris Wilson wrote:
> By using drm_gem_flink/drm_gem_open on an object using the same fd, it
> is possible for a client to create multiple handles pointing to the same
> object (tied to the same contexts and VMA), as exemplified by
> igt::gem_handle_to_libdrm_bo(). Since this duplication has been possible
> since forever, we cannot assume that the handle:(fpriv, object) is
> unique and so must handle the multiple users of a single VMA.
> 
> Testcase: igt/gem_close
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=102355
> Fixes: d1b48c1e7184 ("drm/i915: Replace execbuf vma ht with an idr")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

<SNIP>

> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -720,6 +720,7 @@ static int eb_lookup_vmas(struct i915_execbuffer *eb)
>  			goto err_obj;
>  		}
>  
> +		vma->open_count++;
>  		list_add(&lut->obj_link, &obj->lut_list);

This code maybe should be in i915_gem.c as "i915_gem_object_add_lut" or
something.

> +++ b/drivers/gpu/drm/i915/i915_vma.h
> @@ -59,6 +59,7 @@ struct i915_vma {
>  	u32 fence_size;
>  	u32 fence_alignment;
>  
> +	unsigned int open_count;
>  	unsigned int flags;

Kerneldocs.

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 3/3] drm/i915: Ignore duplicate VMA stored within the per-object handle LUT

[Chris Wilson]

Quoting Joonas Lahtinen (2017-08-23 11:05:18)
> On Tue, 2017-08-22 at 12:05 +0100, Chris Wilson wrote:
> > By using drm_gem_flink/drm_gem_open on an object using the same fd, it
> > is possible for a client to create multiple handles pointing to the same
> > object (tied to the same contexts and VMA), as exemplified by
> > igt::gem_handle_to_libdrm_bo(). Since this duplication has been possible
> > since forever, we cannot assume that the handle:(fpriv, object) is
> > unique and so must handle the multiple users of a single VMA.
> > 
> > Testcase: igt/gem_close
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=102355
> > Fixes: d1b48c1e7184 ("drm/i915: Replace execbuf vma ht with an idr")
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> 
> <SNIP>
> 
> > +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > @@ -720,6 +720,7 @@ static int eb_lookup_vmas(struct i915_execbuffer *eb)
> >                       goto err_obj;
> >               }
> >  
> > +             vma->open_count++;
> >               list_add(&lut->obj_link, &obj->lut_list);
> 
> This code maybe should be in i915_gem.c as "i915_gem_object_add_lut" or
> something.

I disagree. It's very much tied to being an execbuf only interaction,
that obj/ctx/handle.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915: Assert the context is not closed on object-close

[Chris Wilson]

Quoting Lofstedt, Marta (2017-08-22 12:59:59)
> Thanks Chris,
> With this series the test pin-pointed in the bug now pass.
> 
> Tested-by: Marta Lofstedt <marta.lofstedt@intel.com>
> 
> > -----Original Message-----
> > From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf
> > Of Chris Wilson
> > Sent: Tuesday, August 22, 2017 2:05 PM
> > To: intel-gfx@lists.freedesktop.org
> > Subject: [Intel-gfx] [PATCH 1/3] drm/i915: Assert the context is not closed on
> > object-close
> > 
> > During the context-close, we should be decoupling all the vma from the
> > object so that upon object-closing we shouldn't see any vma from the
> > already closed contexts. So include a check upon closing the object that the
> > context is still open.
> > 
> > v2: Eek, the fpriv check is required for shared objects. Double eek, BAT
> > passed?
> 
> Well, the KBL-shards results actually exposed the regression: 
> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_5429/shards-all.html
> So, could you remove that comment.

The comment is referrering to v1 of this patch which did manage to pass
BAT despite it being common practice on ggtt/aliasing_ppgtt setups.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: ✓ Fi.CI.BAT: success for series starting with [1/3] drm/i915: Assert the context is not closed on object-close

[Chris Wilson]

Quoting Patchwork (2017-08-22 13:06:37)
> == Series Details ==
> 
> Series: series starting with [1/3] drm/i915: Assert the context is not closed on object-close
> URL   : https://patchwork.freedesktop.org/series/29137/
> State : success
> 
> == Summary ==
> 
> Series 29137v1 series starting with [1/3] drm/i915: Assert the context is not closed on object-close
> https://patchwork.freedesktop.org/api/1.0/series/29137/revisions/1/mbox/
> 
> Test kms_pipe_crc_basic:
>         Subgroup suspend-read-crc-pipe-b:
>                 dmesg-warn -> PASS       (fi-byt-n2820) fdo#101705
> 
> fdo#101705 https://bugs.freedesktop.org/show_bug.cgi?id=101705

Pushed with Michał's review from the other thread and with added
commentary for vma->open_count.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/3] drm/i915: Ignore duplicate VMA stored within the per-object handle LUT

[Joonas Lahtinen]

On Wed, 2017-08-23 at 11:20 +0100, Chris Wilson wrote:
> Quoting Joonas Lahtinen (2017-08-23 11:05:18)
> > On Tue, 2017-08-22 at 12:05 +0100, Chris Wilson wrote:
> > > By using drm_gem_flink/drm_gem_open on an object using the same fd, it
> > > is possible for a client to create multiple handles pointing to the same
> > > object (tied to the same contexts and VMA), as exemplified by
> > > igt::gem_handle_to_libdrm_bo(). Since this duplication has been possible
> > > since forever, we cannot assume that the handle:(fpriv, object) is
> > > unique and so must handle the multiple users of a single VMA.
> > > 
> > > Testcase: igt/gem_close
> > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=102355
> > > Fixes: d1b48c1e7184 ("drm/i915: Replace execbuf vma ht with an idr")
> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > 
> > <SNIP>
> > 
> > > +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > > @@ -720,6 +720,7 @@ static int eb_lookup_vmas(struct i915_execbuffer *eb)
> > >                       goto err_obj;
> > >               }
> > >  
> > > +             vma->open_count++;
> > >               list_add(&lut->obj_link, &obj->lut_list);
> > 
> > This code maybe should be in i915_gem.c as "i915_gem_object_add_lut" or
> > something.
> 
> I disagree. It's very much tied to being an execbuf only interaction,
> that obj/ctx/handle.

So how are we going to proceed here? The current proposed solution is
very unintuitive, one counter spread over multiple files.

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 3/3] drm/i915: Ignore duplicate VMA stored within the per-object handle LUT

[Chris Wilson]

Quoting Joonas Lahtinen (2017-08-30 12:07:47)
> On Wed, 2017-08-23 at 11:20 +0100, Chris Wilson wrote:
> > Quoting Joonas Lahtinen (2017-08-23 11:05:18)
> > > On Tue, 2017-08-22 at 12:05 +0100, Chris Wilson wrote:
> > > > By using drm_gem_flink/drm_gem_open on an object using the same fd, it
> > > > is possible for a client to create multiple handles pointing to the same
> > > > object (tied to the same contexts and VMA), as exemplified by
> > > > igt::gem_handle_to_libdrm_bo(). Since this duplication has been possible
> > > > since forever, we cannot assume that the handle:(fpriv, object) is
> > > > unique and so must handle the multiple users of a single VMA.
> > > > 
> > > > Testcase: igt/gem_close
> > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=102355
> > > > Fixes: d1b48c1e7184 ("drm/i915: Replace execbuf vma ht with an idr")
> > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > > > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > > 
> > > <SNIP>
> > > 
> > > > +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > > > @@ -720,6 +720,7 @@ static int eb_lookup_vmas(struct i915_execbuffer *eb)
> > > >                       goto err_obj;
> > > >               }
> > > >  
> > > > +             vma->open_count++;
> > > >               list_add(&lut->obj_link, &obj->lut_list);
> > > 
> > > This code maybe should be in i915_gem.c as "i915_gem_object_add_lut" or
> > > something.
> > 
> > I disagree. It's very much tied to being an execbuf only interaction,
> > that obj/ctx/handle.
> 
> So how are we going to proceed here? The current proposed solution is
> very unintuitive, one counter spread over multiple files.

The table is very much for the entertainment of execbuf (and if you
squint hard, ok not hard at all, so is the rest of GEM), if you were to
push hard that's where I suggest to shove it.

But I'm not yet seeing the issue with one side being clear where the
user opens the vma and the other where it is closed by the user.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/3] drm/i915: Ignore duplicate VMA stored within the per-object handle LUT

[Joonas Lahtinen]

On Wed, 2017-08-30 at 12:56 +0100, Chris Wilson wrote:
> Quoting Joonas Lahtinen (2017-08-30 12:07:47)
> > On Wed, 2017-08-23 at 11:20 +0100, Chris Wilson wrote:
> > > Quoting Joonas Lahtinen (2017-08-23 11:05:18)
> > > > On Tue, 2017-08-22 at 12:05 +0100, Chris Wilson wrote:
> > > > > By using drm_gem_flink/drm_gem_open on an object using the same fd, it
> > > > > is possible for a client to create multiple handles pointing to the same
> > > > > object (tied to the same contexts and VMA), as exemplified by
> > > > > igt::gem_handle_to_libdrm_bo(). Since this duplication has been possible
> > > > > since forever, we cannot assume that the handle:(fpriv, object) is
> > > > > unique and so must handle the multiple users of a single VMA.
> > > > > 
> > > > > Testcase: igt/gem_close
> > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=102355
> > > > > Fixes: d1b48c1e7184 ("drm/i915: Replace execbuf vma ht with an idr")
> > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > > > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > > > > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > > > 
> > > > <SNIP>
> > > > 
> > > > > +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > > > > @@ -720,6 +720,7 @@ static int eb_lookup_vmas(struct i915_execbuffer *eb)
> > > > >                       goto err_obj;
> > > > >               }
> > > > >  
> > > > > +             vma->open_count++;
> > > > >               list_add(&lut->obj_link, &obj->lut_list);
> > > > 
> > > > This code maybe should be in i915_gem.c as "i915_gem_object_add_lut" or
> > > > something.
> > > 
> > > I disagree. It's very much tied to being an execbuf only interaction,
> > > that obj/ctx/handle.
> > 
> > So how are we going to proceed here? The current proposed solution is
> > very unintuitive, one counter spread over multiple files.
> 
> The table is very much for the entertainment of execbuf (and if you
> squint hard, ok not hard at all, so is the rest of GEM), if you were to
> push hard that's where I suggest to shove it.
> 
> But I'm not yet seeing the issue with one side being clear where the
> user opens the vma and the other where it is closed by the user.

As long as it's in one file, all good, so execbuf is fine.

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