在 2018/10/25 18:36, 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@xxxxxxx>
>> Cc: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
>> Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx
>> Cc: Christian König <easy2remember.chk@xxxxxxxxxxxxxx>
>> ---
>> 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.
No necessary, why change it?
>
> 3. Fix drm_syncobj_fence_get_or_add_callback to return an error.
why does it still need a return value? Chris and I are trying to avoid that.
Thanks,
David
>
> That way it should be nicely bisectable.
>
> ~Maarten
>
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
