Re: [PATCH] intel: Adding locks for drm objects synchronization.

From: Daniel Vetter <daniel@ffwll.ch>
To: Jacek Danecki <jacek.danecki@intel.com>
Cc: Daniel Vetter <daniel.vetter@intel.com>,
	intel-gfx@lists.freedesktop.org, "Sapala,
	Rafal A" <rafal.a.sapala@intel.com>
Subject: Re: [PATCH] intel: Adding locks for drm objects synchronization.
Date: Fri, 19 Sep 2014 17:36:19 +0200	[thread overview]
Message-ID: <20140919153619.GE15734@phenom.ffwll.local> (raw)
In-Reply-To: <541C3377.6090203@intel.com>

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