* [PATCH] intel: Adding locks for drm objects synchronization.
@ 2014-08-05 18:51 Rafal Sapala
2014-09-18 12:43 ` Daniel Vetter
0 siblings, 1 reply; 5+ messages in thread
From: Rafal Sapala @ 2014-08-05 18:51 UTC (permalink / raw)
To: intel-gfx; +Cc: Rafal Sapala
The changes make sure that members of the bufmgr_gem and bo_gem
name lists are sychronized between threads
when using the create from prime and create from name methods.
Signed-off-by: Rafal Sapala <rafal.a.sapala@intel.com>
---
intel/intel_bufmgr_gem.c | 28 ++++++++++++++++++++++++----
1 files changed, 24 insertions(+), 4 deletions(-)
diff --git a/intel/intel_bufmgr_gem.c b/intel/intel_bufmgr_gem.c
index 007a6d8..243f563 100644
--- a/intel/intel_bufmgr_gem.c
+++ b/intel/intel_bufmgr_gem.c
@@ -871,12 +871,14 @@ drm_intel_bo_gem_create_from_name(drm_intel_bufmgr *bufmgr,
* alternating names for the front/back buffer a linear search
* provides a sufficiently fast match.
*/
+ pthread_mutex_lock(&bufmgr_gem->lock);
for (list = bufmgr_gem->named.next;
list != &bufmgr_gem->named;
list = list->next) {
bo_gem = DRMLISTENTRY(drm_intel_bo_gem, list, name_list);
if (bo_gem->global_name == handle) {
drm_intel_gem_bo_reference(&bo_gem->bo);
+ pthread_mutex_unlock(&bufmgr_gem->lock);
return &bo_gem->bo;
}
}
@@ -889,6 +891,7 @@ drm_intel_bo_gem_create_from_name(drm_intel_bufmgr *bufmgr,
if (ret != 0) {
DBG("Couldn't reference %s handle 0x%08x: %s\n",
name, handle, strerror(errno));
+ pthread_mutex_unlock(&bufmgr_gem->lock);
return NULL;
}
/* Now see if someone has used a prime handle to get this
@@ -901,13 +904,16 @@ drm_intel_bo_gem_create_from_name(drm_intel_bufmgr *bufmgr,
bo_gem = DRMLISTENTRY(drm_intel_bo_gem, list, name_list);
if (bo_gem->gem_handle == open_arg.handle) {
drm_intel_gem_bo_reference(&bo_gem->bo);
+ pthread_mutex_unlock(&bufmgr_gem->lock);
return &bo_gem->bo;
}
}
bo_gem = calloc(1, sizeof(*bo_gem));
- if (!bo_gem)
+ if (!bo_gem) {
+ pthread_mutex_unlock(&bufmgr_gem->lock);
return NULL;
+ }
bo_gem->bo.size = open_arg.size;
bo_gem->bo.offset = 0;
@@ -929,6 +935,7 @@ drm_intel_bo_gem_create_from_name(drm_intel_bufmgr *bufmgr,
&get_tiling);
if (ret != 0) {
drm_intel_gem_bo_unreference(&bo_gem->bo);
+ pthread_mutex_unlock(&bufmgr_gem->lock);
return NULL;
}
bo_gem->tiling_mode = get_tiling.tiling_mode;
@@ -938,6 +945,7 @@ drm_intel_bo_gem_create_from_name(drm_intel_bufmgr *bufmgr,
DRMINITLISTHEAD(&bo_gem->vma_list);
DRMLISTADDTAIL(&bo_gem->name_list, &bufmgr_gem->named);
+ pthread_mutex_unlock(&bufmgr_gem->lock);
DBG("bo_create_from_handle: %d (%s)\n", handle, bo_gem->name);
return &bo_gem->bo;
@@ -2502,25 +2510,29 @@ drm_intel_bo_gem_create_from_prime(drm_intel_bufmgr *bufmgr, int prime_fd, int s
* for named buffers, we must not create two bo's pointing at the same
* kernel object
*/
+ pthread_mutex_lock(&bufmgr_gem->lock);
for (list = bufmgr_gem->named.next;
list != &bufmgr_gem->named;
list = list->next) {
bo_gem = DRMLISTENTRY(drm_intel_bo_gem, list, name_list);
if (bo_gem->gem_handle == handle) {
drm_intel_gem_bo_reference(&bo_gem->bo);
+ pthread_mutex_unlock(&bufmgr_gem->lock);
return &bo_gem->bo;
}
}
if (ret) {
fprintf(stderr,"ret is %d %d\n", ret, errno);
+ pthread_mutex_unlock(&bufmgr_gem->lock);
return NULL;
}
bo_gem = calloc(1, sizeof(*bo_gem));
- if (!bo_gem)
+ if (!bo_gem) {
+ pthread_mutex_unlock(&bufmgr_gem->lock);
return NULL;
-
+ }
/* Determine size of bo. The fd-to-handle ioctl really should
* return the size, but it doesn't. If we have kernel 3.12 or
* later, we can lseek on the prime fd to get the size. Older
@@ -2548,6 +2560,7 @@ drm_intel_bo_gem_create_from_prime(drm_intel_bufmgr *bufmgr, int prime_fd, int s
DRMINITLISTHEAD(&bo_gem->vma_list);
DRMLISTADDTAIL(&bo_gem->name_list, &bufmgr_gem->named);
+ pthread_mutex_unlock(&bufmgr_gem->lock);
VG_CLEAR(get_tiling);
get_tiling.handle = bo_gem->gem_handle;
@@ -2572,8 +2585,10 @@ drm_intel_bo_gem_export_to_prime(drm_intel_bo *bo, int *prime_fd)
drm_intel_bufmgr_gem *bufmgr_gem = (drm_intel_bufmgr_gem *) bo->bufmgr;
drm_intel_bo_gem *bo_gem = (drm_intel_bo_gem *) bo;
+ pthread_mutex_lock(&bufmgr_gem->lock);
if (DRMLISTEMPTY(&bo_gem->name_list))
DRMLISTADDTAIL(&bo_gem->name_list, &bufmgr_gem->named);
+ pthread_mutex_unlock(&bufmgr_gem->lock);
if (drmPrimeHandleToFD(bufmgr_gem->fd, bo_gem->gem_handle,
DRM_CLOEXEC, prime_fd) != 0)
@@ -2597,15 +2612,20 @@ drm_intel_gem_bo_flink(drm_intel_bo *bo, uint32_t * name)
VG_CLEAR(flink);
flink.handle = bo_gem->gem_handle;
+ pthread_mutex_lock(&bufmgr_gem->lock);
+
ret = drmIoctl(bufmgr_gem->fd, DRM_IOCTL_GEM_FLINK, &flink);
- if (ret != 0)
+ if (ret != 0) {
+ pthread_mutex_unlock(&bufmgr_gem->lock);
return -errno;
+ }
bo_gem->global_name = flink.name;
bo_gem->reusable = false;
if (DRMLISTEMPTY(&bo_gem->name_list))
DRMLISTADDTAIL(&bo_gem->name_list, &bufmgr_gem->named);
+ pthread_mutex_unlock(&bufmgr_gem->lock);
}
*name = bo_gem->global_name;
--
1.7.1
--------------------------------------------------------------------
Intel Technology Poland sp. z o.o.
ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wydzial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-316 | Kapital zakladowy 200.000 PLN.
Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata i moze zawierac informacje poufne. W razie przypadkowego otrzymania tej wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; jakiekolwiek
przegladanie lub rozpowszechnianie jest zabronione.
This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by
others is strictly prohibited.
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] intel: Adding locks for drm objects synchronization.
2014-08-05 18:51 [PATCH] intel: Adding locks for drm objects synchronization Rafal Sapala
@ 2014-09-18 12:43 ` Daniel Vetter
2014-09-19 13:45 ` Jacek Danecki
0 siblings, 1 reply; 5+ messages in thread
From: Daniel Vetter @ 2014-09-18 12:43 UTC (permalink / raw)
To: Rafal Sapala; +Cc: intel-gfx
On Tue, Aug 05, 2014 at 02:51:38PM -0400, Rafal Sapala wrote:
> The changes make sure that members of the bufmgr_gem and bo_gem
> name lists are sychronized between threads
> when using the create from prime and create from name methods.
>
> Signed-off-by: Rafal Sapala <rafal.a.sapala@intel.com>
> ---
> intel/intel_bufmgr_gem.c | 28 ++++++++++++++++++++++++----
> 1 files changed, 24 insertions(+), 4 deletions(-)
>
> diff --git a/intel/intel_bufmgr_gem.c b/intel/intel_bufmgr_gem.c
> index 007a6d8..243f563 100644
> --- a/intel/intel_bufmgr_gem.c
> +++ b/intel/intel_bufmgr_gem.c
> @@ -871,12 +871,14 @@ drm_intel_bo_gem_create_from_name(drm_intel_bufmgr *bufmgr,
> * alternating names for the front/back buffer a linear search
> * provides a sufficiently fast match.
> */
> + pthread_mutex_lock(&bufmgr_gem->lock);
> for (list = bufmgr_gem->named.next;
> list != &bufmgr_gem->named;
> list = list->next) {
> bo_gem = DRMLISTENTRY(drm_intel_bo_gem, list, name_list);
> if (bo_gem->global_name == handle) {
> drm_intel_gem_bo_reference(&bo_gem->bo);
> + pthread_mutex_unlock(&bufmgr_gem->lock);
> return &bo_gem->bo;
> }
> }
> @@ -889,6 +891,7 @@ drm_intel_bo_gem_create_from_name(drm_intel_bufmgr *bufmgr,
> if (ret != 0) {
> DBG("Couldn't reference %s handle 0x%08x: %s\n",
> name, handle, strerror(errno));
> + pthread_mutex_unlock(&bufmgr_gem->lock);
> return NULL;
> }
> /* Now see if someone has used a prime handle to get this
> @@ -901,13 +904,16 @@ drm_intel_bo_gem_create_from_name(drm_intel_bufmgr *bufmgr,
> bo_gem = DRMLISTENTRY(drm_intel_bo_gem, list, name_list);
> if (bo_gem->gem_handle == open_arg.handle) {
> drm_intel_gem_bo_reference(&bo_gem->bo);
> + pthread_mutex_unlock(&bufmgr_gem->lock);
> return &bo_gem->bo;
> }
> }
>
> bo_gem = calloc(1, sizeof(*bo_gem));
> - if (!bo_gem)
> + if (!bo_gem) {
> + pthread_mutex_unlock(&bufmgr_gem->lock);
> return NULL;
> + }
>
> bo_gem->bo.size = open_arg.size;
> bo_gem->bo.offset = 0;
> @@ -929,6 +935,7 @@ drm_intel_bo_gem_create_from_name(drm_intel_bufmgr *bufmgr,
> &get_tiling);
> if (ret != 0) {
> drm_intel_gem_bo_unreference(&bo_gem->bo);
> + pthread_mutex_unlock(&bufmgr_gem->lock);
> return NULL;
> }
> bo_gem->tiling_mode = get_tiling.tiling_mode;
> @@ -938,6 +945,7 @@ drm_intel_bo_gem_create_from_name(drm_intel_bufmgr *bufmgr,
>
> DRMINITLISTHEAD(&bo_gem->vma_list);
> DRMLISTADDTAIL(&bo_gem->name_list, &bufmgr_gem->named);
> + pthread_mutex_unlock(&bufmgr_gem->lock);
> DBG("bo_create_from_handle: %d (%s)\n", handle, bo_gem->name);
>
> return &bo_gem->bo;
> @@ -2502,25 +2510,29 @@ drm_intel_bo_gem_create_from_prime(drm_intel_bufmgr *bufmgr, int prime_fd, int s
> * for named buffers, we must not create two bo's pointing at the same
> * kernel object
> */
> + pthread_mutex_lock(&bufmgr_gem->lock);
> for (list = bufmgr_gem->named.next;
> list != &bufmgr_gem->named;
> list = list->next) {
> bo_gem = DRMLISTENTRY(drm_intel_bo_gem, list, name_list);
> if (bo_gem->gem_handle == handle) {
> drm_intel_gem_bo_reference(&bo_gem->bo);
> + pthread_mutex_unlock(&bufmgr_gem->lock);
> return &bo_gem->bo;
> }
> }
>
> if (ret) {
> fprintf(stderr,"ret is %d %d\n", ret, errno);
> + pthread_mutex_unlock(&bufmgr_gem->lock);
> return NULL;
> }
>
> bo_gem = calloc(1, sizeof(*bo_gem));
> - if (!bo_gem)
> + if (!bo_gem) {
> + pthread_mutex_unlock(&bufmgr_gem->lock);
> return NULL;
> -
> + }
> /* Determine size of bo. The fd-to-handle ioctl really should
> * return the size, but it doesn't. If we have kernel 3.12 or
> * later, we can lseek on the prime fd to get the size. Older
> @@ -2548,6 +2560,7 @@ drm_intel_bo_gem_create_from_prime(drm_intel_bufmgr *bufmgr, int prime_fd, int s
>
> DRMINITLISTHEAD(&bo_gem->vma_list);
> DRMLISTADDTAIL(&bo_gem->name_list, &bufmgr_gem->named);
> + pthread_mutex_unlock(&bufmgr_gem->lock);
>
> VG_CLEAR(get_tiling);
> get_tiling.handle = bo_gem->gem_handle;
> @@ -2572,8 +2585,10 @@ drm_intel_bo_gem_export_to_prime(drm_intel_bo *bo, int *prime_fd)
> drm_intel_bufmgr_gem *bufmgr_gem = (drm_intel_bufmgr_gem *) bo->bufmgr;
> drm_intel_bo_gem *bo_gem = (drm_intel_bo_gem *) bo;
>
> + pthread_mutex_lock(&bufmgr_gem->lock);
> if (DRMLISTEMPTY(&bo_gem->name_list))
> DRMLISTADDTAIL(&bo_gem->name_list, &bufmgr_gem->named);
> + pthread_mutex_unlock(&bufmgr_gem->lock);
>
> if (drmPrimeHandleToFD(bufmgr_gem->fd, bo_gem->gem_handle,
> DRM_CLOEXEC, prime_fd) != 0)
> @@ -2597,15 +2612,20 @@ drm_intel_gem_bo_flink(drm_intel_bo *bo, uint32_t * name)
> VG_CLEAR(flink);
> flink.handle = bo_gem->gem_handle;
>
> + pthread_mutex_lock(&bufmgr_gem->lock);
> +
> ret = drmIoctl(bufmgr_gem->fd, DRM_IOCTL_GEM_FLINK, &flink);
> - if (ret != 0)
> + if (ret != 0) {
> + pthread_mutex_unlock(&bufmgr_gem->lock);
> return -errno;
> + }
>
> bo_gem->global_name = flink.name;
> bo_gem->reusable = false;
>
> if (DRMLISTEMPTY(&bo_gem->name_list))
> DRMLISTADDTAIL(&bo_gem->name_list, &bufmgr_gem->named);
> + pthread_mutex_unlock(&bufmgr_gem->lock);
> }
>
> *name = bo_gem->global_name;
> --
> 1.7.1
>
> --------------------------------------------------------------------
>
> Intel Technology Poland sp. z o.o.
> ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wydzial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-316 | Kapital zakladowy 200.000 PLN.
>
> Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata i moze zawierac informacje poufne. W razie przypadkowego otrzymania tej wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; jakiekolwiek
> przegladanie lub rozpowszechnianie jest zabronione.
> This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by
> others is strictly prohibited.
I can't merge patches with this disclaimer ...
Patch looks good otherwise. Is there a testcase for it? As long as it's a
stand-alone C testcase I can easily do the integration with igt myself
quickly.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] intel: Adding locks for drm objects synchronization.
2014-09-18 12:43 ` Daniel Vetter
@ 2014-09-19 13:45 ` Jacek Danecki
2014-09-19 15:36 ` Daniel Vetter
0 siblings, 1 reply; 5+ messages in thread
From: Jacek Danecki @ 2014-09-19 13:45 UTC (permalink / raw)
To: Daniel Vetter, Sapala, Rafal A; +Cc: intel-gfx
On 09/18/14 14:43, Daniel Vetter wrote:
> I can't merge patches with this disclaimer ...
We're working on this, sorry... We'll send it again.
Btw, in another tests with prime we have also found new problem with synchronization, which below patch fixed.
From: Rafal Sapala <rafal.a.sapala@intel.com>
Date: Thu, 18 Sep 2014 18:01:02 +0200
Subject: [PATCH] Prime sharing mechanism mutex patch for multithread usage
Signed-off-by: Rafal Sapala <rafal.a.sapala@intel.com>
---
intel/intel_bufmgr_gem.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/intel/intel_bufmgr_gem.c b/intel/intel_bufmgr_gem.c
index d512343..e05920a 100755
--- a/intel/intel_bufmgr_gem.c
+++ b/intel/intel_bufmgr_gem.c
@@ -2604,6 +2604,7 @@ drm_intel_bo_gem_create_from_prime(drm_intel_bufmgr *bufmgr, int prime_fd, int s
struct drm_i915_gem_get_tiling get_tiling;
drmMMListHead *list;
+ pthread_mutex_lock(&bufmgr_gem->lock);
ret = drmPrimeFDToHandle(bufmgr_gem->fd, prime_fd, &handle);
/*
@@ -2611,7 +2612,6 @@ drm_intel_bo_gem_create_from_prime(drm_intel_bufmgr *bufmgr, int prime_fd, int s
* for named buffers, we must not create two bo's pointing at the same
* kernel object
*/
- pthread_mutex_lock(&bufmgr_gem->lock);
for (list = bufmgr_gem->named.next;
list != &bufmgr_gem->named;
list = list->next) {
--
1.7.12.4
--
jacek
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] intel: Adding locks for drm objects synchronization.
2014-09-19 13:45 ` Jacek Danecki
@ 2014-09-19 15:36 ` Daniel Vetter
2014-09-19 15:52 ` Jacek Danecki
0 siblings, 1 reply; 5+ messages in thread
From: Daniel Vetter @ 2014-09-19 15:36 UTC (permalink / raw)
To: Jacek Danecki; +Cc: Daniel Vetter, intel-gfx, Sapala, Rafal A
On Fri, Sep 19, 2014 at 03:45:27PM +0200, Jacek Danecki wrote:
> On 09/18/14 14:43, Daniel Vetter wrote:
> > I can't merge patches with this disclaimer ...
>
> We're working on this, sorry... We'll send it again.
Yeah just dropped it ;-)
> Btw, in another tests with prime we have also found new problem with synchronization, which below patch fixed.
>
> From: Rafal Sapala <rafal.a.sapala@intel.com>
> Date: Thu, 18 Sep 2014 18:01:02 +0200
> Subject: [PATCH] Prime sharing mechanism mutex patch for multithread usage
>
> Signed-off-by: Rafal Sapala <rafal.a.sapala@intel.com>
Hm, I don't see what this fixes, except maybe a race in the kernel?
Testcase plus some analysis in the commit message about what blows up
exactly and how this fixes it is required here.
Rule of thumb is that the tricker the implications of your change the
longer the commit message should be. No commit message for a locking
change is definitely too little.
-Daniel
> ---
> intel/intel_bufmgr_gem.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/intel/intel_bufmgr_gem.c b/intel/intel_bufmgr_gem.c
> index d512343..e05920a 100755
> --- a/intel/intel_bufmgr_gem.c
> +++ b/intel/intel_bufmgr_gem.c
> @@ -2604,6 +2604,7 @@ drm_intel_bo_gem_create_from_prime(drm_intel_bufmgr *bufmgr, int prime_fd, int s
> struct drm_i915_gem_get_tiling get_tiling;
> drmMMListHead *list;
>
> + pthread_mutex_lock(&bufmgr_gem->lock);
> ret = drmPrimeFDToHandle(bufmgr_gem->fd, prime_fd, &handle);
>
> /*
> @@ -2611,7 +2612,6 @@ drm_intel_bo_gem_create_from_prime(drm_intel_bufmgr *bufmgr, int prime_fd, int s
> * for named buffers, we must not create two bo's pointing at the same
> * kernel object
> */
> - pthread_mutex_lock(&bufmgr_gem->lock);
> for (list = bufmgr_gem->named.next;
> list != &bufmgr_gem->named;
> list = list->next) {
> --
> 1.7.12.4
>
> --
> jacek
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] intel: Adding locks for drm objects synchronization.
2014-09-19 15:36 ` Daniel Vetter
@ 2014-09-19 15:52 ` Jacek Danecki
0 siblings, 0 replies; 5+ messages in thread
From: Jacek Danecki @ 2014-09-19 15:52 UTC (permalink / raw)
To: Daniel Vetter; +Cc: Daniel Vetter, intel-gfx, Sapala, Rafal A
On 09/19/14 17:36, Daniel Vetter wrote:
> Hm, I don't see what this fixes, except maybe a race in the kernel?
> Testcase plus some analysis in the commit message about what blows up
> exactly and how this fixes it is required here.
Currently we can reproduce this issue using our "internal" projects in multi-threaded configurations.
I'll try to extend test [1] to reproduce issue. Without this patch DRM_IOCTL_I915_GEM_GET_TILING ioctl
returned -ENOENT from i915_gem_get_tiling function (drivers/gpu/drm/i915/i915_gem_tiling.c):
/**
* Returns the current tiling mode and required bit 6 swizzling for the object.
*/
int
i915_gem_get_tiling(struct drm_device *dev, void *data,
struct drm_file *file)
{
struct drm_i915_gem_get_tiling *args = data;
struct drm_i915_private *dev_priv = dev->dev_private;
struct drm_i915_gem_object *obj;
obj = to_intel_bo(drm_gem_object_lookup(dev, file, args->handle));
if (&obj->base == NULL)
return -ENOENT;
This ioctl is being called in libdrm in function drm_intel_bo_gem_create_from_prime:
get_tiling.handle = bo_gem->gem_handle;
ret = drmIoctl(bufmgr_gem->fd,
DRM_IOCTL_I915_GEM_GET_TILING,
&get_tiling);
if (ret != 0) {
drm_intel_gem_bo_unreference(&bo_gem->bo);
return NULL;
}
[1] http://lists.freedesktop.org/archives/intel-gfx/2014-September/052586.html
--
jacek
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2014-09-19 15:53 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-08-05 18:51 [PATCH] intel: Adding locks for drm objects synchronization Rafal Sapala
2014-09-18 12:43 ` Daniel Vetter
2014-09-19 13:45 ` Jacek Danecki
2014-09-19 15:36 ` Daniel Vetter
2014-09-19 15:52 ` Jacek Danecki
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox