[Intel-gfx] [PATCH] drm: fix call_kern.cocci warnings v3

[Maarten Lankhorst]

Op 25-10-18 om 12:21 schreef Chunming Zhou:
> drivers/gpu/drm/drm_syncobj.c:202:4-14: ERROR: function drm_syncobj_find_signal_pt_for_point called on line 390 inside lock on line 389 but uses GFP_KERNEL
>
>   Find functions that refer to GFP_KERNEL but are called with locks held.
>
> Generated by: scripts/coccinelle/locks/call_kern.cocci
>
> v2:
> syncobj->timeline still needs protect.
>
> v3:
> use a global signaled fence instead of re-allocation.
>
> Signed-off-by: Chunming Zhou <david1.zhou at amd.com>
> Cc: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> Cc: intel-gfx at lists.freedesktop.org
> Cc: Christian König <easy2remember.chk at googlemail.com>
> ---
>  drivers/gpu/drm/drm_drv.c     |  2 ++
>  drivers/gpu/drm/drm_syncobj.c | 52 +++++++++++++++++++++--------------
>  include/drm/drm_syncobj.h     |  1 +
>  3 files changed, 34 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 36e8e9cbec52..0a6f1023d6c3 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -37,6 +37,7 @@
>  #include <drm/drm_client.h>
>  #include <drm/drm_drv.h>
>  #include <drm/drmP.h>
> +#include <drm/drm_syncobj.h>
>  
>  #include "drm_crtc_internal.h"
>  #include "drm_legacy.h"
> @@ -1003,6 +1004,7 @@ static int __init drm_core_init(void)
>  	if (ret < 0)
>  		goto error;
>  
> +	drm_syncobj_stub_fence_init();
>  	drm_core_init_complete = true;
>  
>  	DRM_DEBUG("Initialized\n");
> diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
> index b7eaa603f368..6b3f5a06e4d3 100644
> --- a/drivers/gpu/drm/drm_syncobj.c
> +++ b/drivers/gpu/drm/drm_syncobj.c
> @@ -80,6 +80,27 @@ struct drm_syncobj_signal_pt {
>  	struct list_head list;
>  };
>  
> +static struct drm_syncobj_stub_fence stub_signaled_fence;
> +static void global_stub_fence_release(struct dma_fence *fence)
> +{
> +	/* it is impossible to come here */
> +	BUG();
> +}
WARN_ON_ONCE(1)? No need to halt the machine.

> +static const struct dma_fence_ops global_stub_fence_ops = {
> +	.get_driver_name = drm_syncobj_stub_fence_get_name,
> +	.get_timeline_name = drm_syncobj_stub_fence_get_name,
> +	.release = global_stub_fence_release,
> +};
> +
> +void drm_syncobj_stub_fence_init(void)
> +{
> +	spin_lock_init(&stub_signaled_fence.lock);
> +	dma_fence_init(&stub_signaled_fence.base,
> +		       &global_stub_fence_ops,
> +		       &stub_signaled_fence.lock,
> +		       0, 0);
> +	dma_fence_signal(&stub_signaled_fence.base);
> +}
>  /**
>   * drm_syncobj_find - lookup and reference a sync object.
>   * @file_private: drm file private pointer
> @@ -111,24 +132,14 @@ static struct dma_fence
>  				      uint64_t point)
>  {
>  	struct drm_syncobj_signal_pt *signal_pt;
> +	struct dma_fence *f = NULL;
>  
> +	spin_lock(&syncobj->pt_lock);
>  	if ((syncobj->type == DRM_SYNCOBJ_TYPE_TIMELINE) &&
>  	    (point <= syncobj->timeline)) {
> -		struct drm_syncobj_stub_fence *fence =
> -			kzalloc(sizeof(struct drm_syncobj_stub_fence),
> -				GFP_KERNEL);
> -
> -		if (!fence)
> -			return NULL;
> -		spin_lock_init(&fence->lock);
> -		dma_fence_init(&fence->base,
> -			       &drm_syncobj_stub_fence_ops,
> -			       &fence->lock,
> -			       syncobj->timeline_context,
> -			       point);
> -
> -		dma_fence_signal(&fence->base);
> -		return &fence->base;
> +		dma_fence_get(&stub_signaled_fence.base);
> +		spin_unlock(&syncobj->pt_lock);
> +		return &stub_signaled_fence.base;
>  	}
>  
>  	list_for_each_entry(signal_pt, &syncobj->signal_pt_list, list) {
> @@ -137,9 +148,12 @@ static struct dma_fence
>  		if ((syncobj->type == DRM_SYNCOBJ_TYPE_BINARY) &&
>  		    (point != signal_pt->value))
>  			continue;
> -		return dma_fence_get(&signal_pt->fence_array->base);
> +		f = dma_fence_get(&signal_pt->fence_array->base);
> +		break;
>  	}
> -	return NULL;
> +	spin_unlock(&syncobj->pt_lock);
> +
> +	return f;
>  }
>  
>  static void drm_syncobj_add_callback_locked(struct drm_syncobj *syncobj,
> @@ -166,9 +180,7 @@ static void drm_syncobj_fence_get_or_add_callback(struct drm_syncobj *syncobj,
>  	}
>  
>  	mutex_lock(&syncobj->cb_mutex);
> -	spin_lock(&syncobj->pt_lock);
>  	*fence = drm_syncobj_find_signal_pt_for_point(syncobj, pt_value);
> -	spin_unlock(&syncobj->pt_lock);
>  	if (!*fence)
>  		drm_syncobj_add_callback_locked(syncobj, cb, func);
>  	mutex_unlock(&syncobj->cb_mutex);
> @@ -379,11 +391,9 @@ drm_syncobj_point_get(struct drm_syncobj *syncobj, u64 point, u64 flags,
>  		if (ret < 0)
>  			return ret;
>  	}
> -	spin_lock(&syncobj->pt_lock);
>  	*fence = drm_syncobj_find_signal_pt_for_point(syncobj, point);
>  	if (!*fence)
>  		ret = -EINVAL;
> -	spin_unlock(&syncobj->pt_lock);
>  	return ret;
>  }
>  
> diff --git a/include/drm/drm_syncobj.h b/include/drm/drm_syncobj.h
> index 29244cbcd05e..63cfd1540241 100644
> --- a/include/drm/drm_syncobj.h
> +++ b/include/drm/drm_syncobj.h
> @@ -148,4 +148,5 @@ int drm_syncobj_get_fd(struct drm_syncobj *syncobj, int *p_fd);
>  int drm_syncobj_search_fence(struct drm_syncobj *syncobj, u64 point, u64 flags,
>  			     struct dma_fence **fence);
>  
> +void drm_syncobj_stub_fence_init(void);
>  #endif

Move it to drm_internal.h ? This is not for driver consumption. :)

i would split up the changes at this point:

1. Replace memory allocation with a static fence, not necessarily changing any locking, add a fixes tag to this commit.

2. Move locking.

3. Fix drm_syncobj_fence_get_or_add_callback to return an error.

That way it should be nicely bisectable.

~Maarten