[2/9] drm/syncobj: add new drm_syncobj_add_point interface v4

Commit Message

Chunming Zhou March 25, 2019, 8:32 a.m. UTC

From: Christian König <[e-mail]>

Use the dma_fence_chain object to create a timeline of fence objects
instead of just replacing the existing fence.

v2: rebase and cleanup
v3: fix garbage collection parameters
v4: add unorder point check, print a warn calltrace

Signed-off-by: Christian König <[e-mail]>
Cc: Lionel Landwerlin <[e-mail]>
---
 drivers/gpu/drm/drm_syncobj.c | 39 +++++++++++++++++++++++++++++++++++
 include/drm/drm_syncobj.h     |  5 +++++
 2 files changed, 44 insertions(+)

Comments

Lionel Landwerlin March 28, 2019, 1:50 p.m. UTC | #1

On 25/03/2019 08:32, Chunming Zhou wrote:
> From: Christian König <[e-mail]>
>
> Use the dma_fence_chain object to create a timeline of fence objects
> instead of just replacing the existing fence.
>
> v2: rebase and cleanup
> v3: fix garbage collection parameters
> v4: add unorder point check, print a warn calltrace
>
> Signed-off-by: Christian König <[e-mail]>
> Cc: Lionel Landwerlin <[e-mail]>
> ---
>   drivers/gpu/drm/drm_syncobj.c | 39 +++++++++++++++++++++++++++++++++++
>   include/drm/drm_syncobj.h     |  5 +++++
>   2 files changed, 44 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
> index 5329e66598c6..19a9ce638119 100644
> --- a/drivers/gpu/drm/drm_syncobj.c
> +++ b/drivers/gpu/drm/drm_syncobj.c
> @@ -122,6 +122,45 @@ static void drm_syncobj_remove_wait(struct drm_syncobj *syncobj,
>   	spin_unlock(&syncobj->lock);
>   }
>   
> +/**
> + * drm_syncobj_add_point - add new timeline point to the syncobj
> + * @syncobj: sync object to add timeline point do
> + * @chain: chain node to use to add the point
> + * @fence: fence to encapsulate in the chain node
> + * @point: sequence number to use for the point
> + *
> + * Add the chain node as new timeline point to the syncobj.
> + */
> +void drm_syncobj_add_point(struct drm_syncobj *syncobj,
> +			   struct dma_fence_chain *chain,
> +			   struct dma_fence *fence,
> +			   uint64_t point)
> +{
> +	struct syncobj_wait_entry *cur, *tmp;
> +	struct dma_fence *prev;
> +
> +	dma_fence_get(fence);
> +
> +	spin_lock(&syncobj->lock);
> +
> +	prev = drm_syncobj_fence_get(syncobj);
> +	/* You are adding an unorder point to timeline, which could cause payload returned from query_ioctl is 0! */
> +	WARN_ON_ONCE(prev && prev->seqno >= point);


I think the WARN/BUG macros should only fire when there is an issue with 
programming from within the kernel.

But this particular warning can be triggered by an application.


Probably best to just remove it?


-Lionel


> +	dma_fence_chain_init(chain, prev, fence, point);
> +	rcu_assign_pointer(syncobj->fence, &chain->base);
> +
> +	list_for_each_entry_safe(cur, tmp, &syncobj->cb_list, node) {
> +		list_del_init(&cur->node);
> +		syncobj_wait_syncobj_func(syncobj, cur);
> +	}
> +	spin_unlock(&syncobj->lock);
> +
> +	/* Walk the chain once to trigger garbage collection */
> +	dma_fence_chain_for_each(fence, prev);
> +	dma_fence_put(prev);
> +}
> +EXPORT_SYMBOL(drm_syncobj_add_point);
> +
>   /**
>    * drm_syncobj_replace_fence - replace fence in a sync object.
>    * @syncobj: Sync object to replace fence in
> diff --git a/include/drm/drm_syncobj.h b/include/drm/drm_syncobj.h
> index 0311c9fdbd2f..6cf7243a1dc5 100644
> --- a/include/drm/drm_syncobj.h
> +++ b/include/drm/drm_syncobj.h
> @@ -27,6 +27,7 @@
>   #define __DRM_SYNCOBJ_H__
>   
>   #include <linux/dma-fence.h>
> +#include <linux/dma-fence-chain.h>
>   
>   struct drm_file;
>   
> @@ -112,6 +113,10 @@ drm_syncobj_fence_get(struct drm_syncobj *syncobj)
>   
>   struct drm_syncobj *drm_syncobj_find(struct drm_file *file_private,
>   				     u32 handle);
> +void drm_syncobj_add_point(struct drm_syncobj *syncobj,
> +			   struct dma_fence_chain *chain,
> +			   struct dma_fence *fence,
> +			   uint64_t point);
>   void drm_syncobj_replace_fence(struct drm_syncobj *syncobj,
>   			       struct dma_fence *fence);
>   int drm_syncobj_find_fence(struct drm_file *file_private,

Christian König March 28, 2019, 3:18 p.m. UTC | #2

Am 28.03.19 um 14:50 schrieb Lionel Landwerlin:
> On 25/03/2019 08:32, Chunming Zhou wrote:
>> From: Christian König <[e-mail]>
>>
>> Use the dma_fence_chain object to create a timeline of fence objects
>> instead of just replacing the existing fence.
>>
>> v2: rebase and cleanup
>> v3: fix garbage collection parameters
>> v4: add unorder point check, print a warn calltrace
>>
>> Signed-off-by: Christian König <[e-mail]>
>> Cc: Lionel Landwerlin <[e-mail]>
>> ---
>>   drivers/gpu/drm/drm_syncobj.c | 39 +++++++++++++++++++++++++++++++++++
>>   include/drm/drm_syncobj.h     |  5 +++++
>>   2 files changed, 44 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_syncobj.c 
>> b/drivers/gpu/drm/drm_syncobj.c
>> index 5329e66598c6..19a9ce638119 100644
>> --- a/drivers/gpu/drm/drm_syncobj.c
>> +++ b/drivers/gpu/drm/drm_syncobj.c
>> @@ -122,6 +122,45 @@ static void drm_syncobj_remove_wait(struct 
>> drm_syncobj *syncobj,
>>       spin_unlock(&syncobj->lock);
>>   }
>>   +/**
>> + * drm_syncobj_add_point - add new timeline point to the syncobj
>> + * @syncobj: sync object to add timeline point do
>> + * @chain: chain node to use to add the point
>> + * @fence: fence to encapsulate in the chain node
>> + * @point: sequence number to use for the point
>> + *
>> + * Add the chain node as new timeline point to the syncobj.
>> + */
>> +void drm_syncobj_add_point(struct drm_syncobj *syncobj,
>> +               struct dma_fence_chain *chain,
>> +               struct dma_fence *fence,
>> +               uint64_t point)
>> +{
>> +    struct syncobj_wait_entry *cur, *tmp;
>> +    struct dma_fence *prev;
>> +
>> +    dma_fence_get(fence);
>> +
>> +    spin_lock(&syncobj->lock);
>> +
>> +    prev = drm_syncobj_fence_get(syncobj);
>> +    /* You are adding an unorder point to timeline, which could 
>> cause payload returned from query_ioctl is 0! */
>> +    WARN_ON_ONCE(prev && prev->seqno >= point);
>
>
> I think the WARN/BUG macros should only fire when there is an issue 
> with programming from within the kernel.
>
> But this particular warning can be triggered by an application.
>
>
> Probably best to just remove it?

Yeah, that was also my argument against it.

Key point here is that we still want to note somehow that userspace did 
something wrong and returning an error is not an option.

Maybe just use DRM_ERROR with a static variable to print the message 
only once.

Christian.

>
>
> -Lionel
>
>
>> +    dma_fence_chain_init(chain, prev, fence, point);
>> +    rcu_assign_pointer(syncobj->fence, &chain->base);
>> +
>> +    list_for_each_entry_safe(cur, tmp, &syncobj->cb_list, node) {
>> +        list_del_init(&cur->node);
>> +        syncobj_wait_syncobj_func(syncobj, cur);
>> +    }
>> +    spin_unlock(&syncobj->lock);
>> +
>> +    /* Walk the chain once to trigger garbage collection */
>> +    dma_fence_chain_for_each(fence, prev);
>> +    dma_fence_put(prev);
>> +}
>> +EXPORT_SYMBOL(drm_syncobj_add_point);
>> +
>>   /**
>>    * drm_syncobj_replace_fence - replace fence in a sync object.
>>    * @syncobj: Sync object to replace fence in
>> diff --git a/include/drm/drm_syncobj.h b/include/drm/drm_syncobj.h
>> index 0311c9fdbd2f..6cf7243a1dc5 100644
>> --- a/include/drm/drm_syncobj.h
>> +++ b/include/drm/drm_syncobj.h
>> @@ -27,6 +27,7 @@
>>   #define __DRM_SYNCOBJ_H__
>>     #include <linux/dma-fence.h>
>> +#include <linux/dma-fence-chain.h>
>>     struct drm_file;
>>   @@ -112,6 +113,10 @@ drm_syncobj_fence_get(struct drm_syncobj 
>> *syncobj)
>>     struct drm_syncobj *drm_syncobj_find(struct drm_file *file_private,
>>                        u32 handle);
>> +void drm_syncobj_add_point(struct drm_syncobj *syncobj,
>> +               struct dma_fence_chain *chain,
>> +               struct dma_fence *fence,
>> +               uint64_t point);
>>   void drm_syncobj_replace_fence(struct drm_syncobj *syncobj,
>>                      struct dma_fence *fence);
>>   int drm_syncobj_find_fence(struct drm_file *file_private,
>
>

Michel Dänzer March 28, 2019, 3:34 p.m. UTC | #3

On 2019-03-28 4:18 p.m., Christian König wrote:
> Am 28.03.19 um 14:50 schrieb Lionel Landwerlin:
>> On 25/03/2019 08:32, Chunming Zhou wrote:
>>> From: Christian König <[e-mail]>
>>>
>>> Use the dma_fence_chain object to create a timeline of fence objects
>>> instead of just replacing the existing fence.
>>>
>>> v2: rebase and cleanup
>>> v3: fix garbage collection parameters
>>> v4: add unorder point check, print a warn calltrace
>>>
>>> Signed-off-by: Christian König <[e-mail]>
>>> Cc: Lionel Landwerlin <[e-mail]>
>>> ---
>>>   drivers/gpu/drm/drm_syncobj.c | 39 +++++++++++++++++++++++++++++++++++
>>>   include/drm/drm_syncobj.h     |  5 +++++
>>>   2 files changed, 44 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/drm_syncobj.c
>>> b/drivers/gpu/drm/drm_syncobj.c
>>> index 5329e66598c6..19a9ce638119 100644
>>> --- a/drivers/gpu/drm/drm_syncobj.c
>>> +++ b/drivers/gpu/drm/drm_syncobj.c
>>> @@ -122,6 +122,45 @@ static void drm_syncobj_remove_wait(struct
>>> drm_syncobj *syncobj,
>>>       spin_unlock(&syncobj->lock);
>>>   }
>>>   +/**
>>> + * drm_syncobj_add_point - add new timeline point to the syncobj
>>> + * @syncobj: sync object to add timeline point do
>>> + * @chain: chain node to use to add the point
>>> + * @fence: fence to encapsulate in the chain node
>>> + * @point: sequence number to use for the point
>>> + *
>>> + * Add the chain node as new timeline point to the syncobj.
>>> + */
>>> +void drm_syncobj_add_point(struct drm_syncobj *syncobj,
>>> +               struct dma_fence_chain *chain,
>>> +               struct dma_fence *fence,
>>> +               uint64_t point)
>>> +{
>>> +    struct syncobj_wait_entry *cur, *tmp;
>>> +    struct dma_fence *prev;
>>> +
>>> +    dma_fence_get(fence);
>>> +
>>> +    spin_lock(&syncobj->lock);
>>> +
>>> +    prev = drm_syncobj_fence_get(syncobj);
>>> +    /* You are adding an unorder point to timeline, which could
>>> cause payload returned from query_ioctl is 0! */
>>> +    WARN_ON_ONCE(prev && prev->seqno >= point);
>>
>>
>> I think the WARN/BUG macros should only fire when there is an issue
>> with programming from within the kernel.
>>
>> But this particular warning can be triggered by an application.
>>
>>
>> Probably best to just remove it?
> 
> Yeah, that was also my argument against it.
> 
> Key point here is that we still want to note somehow that userspace did
> something wrong and returning an error is not an option.
> 
> Maybe just use DRM_ERROR with a static variable to print the message
> only once.

How about DRM_WARN_ONCE ?

Lionel Landwerlin March 30, 2019, 2:09 p.m. UTC | #4

On 28/03/2019 15:18, Christian König wrote:
> Am 28.03.19 um 14:50 schrieb Lionel Landwerlin:
>> On 25/03/2019 08:32, Chunming Zhou wrote:
>>> From: Christian König <[e-mail]>
>>>
>>> Use the dma_fence_chain object to create a timeline of fence objects
>>> instead of just replacing the existing fence.
>>>
>>> v2: rebase and cleanup
>>> v3: fix garbage collection parameters
>>> v4: add unorder point check, print a warn calltrace
>>>
>>> Signed-off-by: Christian König <[e-mail]>
>>> Cc: Lionel Landwerlin <[e-mail]>
>>> ---
>>>   drivers/gpu/drm/drm_syncobj.c | 39 
>>> +++++++++++++++++++++++++++++++++++
>>>   include/drm/drm_syncobj.h     |  5 +++++
>>>   2 files changed, 44 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/drm_syncobj.c 
>>> b/drivers/gpu/drm/drm_syncobj.c
>>> index 5329e66598c6..19a9ce638119 100644
>>> --- a/drivers/gpu/drm/drm_syncobj.c
>>> +++ b/drivers/gpu/drm/drm_syncobj.c
>>> @@ -122,6 +122,45 @@ static void drm_syncobj_remove_wait(struct 
>>> drm_syncobj *syncobj,
>>>       spin_unlock(&syncobj->lock);
>>>   }
>>>   +/**
>>> + * drm_syncobj_add_point - add new timeline point to the syncobj
>>> + * @syncobj: sync object to add timeline point do
>>> + * @chain: chain node to use to add the point
>>> + * @fence: fence to encapsulate in the chain node
>>> + * @point: sequence number to use for the point
>>> + *
>>> + * Add the chain node as new timeline point to the syncobj.
>>> + */
>>> +void drm_syncobj_add_point(struct drm_syncobj *syncobj,
>>> +               struct dma_fence_chain *chain,
>>> +               struct dma_fence *fence,
>>> +               uint64_t point)
>>> +{
>>> +    struct syncobj_wait_entry *cur, *tmp;
>>> +    struct dma_fence *prev;
>>> +
>>> +    dma_fence_get(fence);
>>> +
>>> +    spin_lock(&syncobj->lock);
>>> +
>>> +    prev = drm_syncobj_fence_get(syncobj);
>>> +    /* You are adding an unorder point to timeline, which could 
>>> cause payload returned from query_ioctl is 0! */
>>> +    WARN_ON_ONCE(prev && prev->seqno >= point);
>>
>>
>> I think the WARN/BUG macros should only fire when there is an issue 
>> with programming from within the kernel.
>>
>> But this particular warning can be triggered by an application.
>>
>>
>> Probably best to just remove it?
>
> Yeah, that was also my argument against it.
>
> Key point here is that we still want to note somehow that userspace 
> did something wrong and returning an error is not an option.
>
> Maybe just use DRM_ERROR with a static variable to print the message 
> only once.
>
> Christian.

I don't really see any point in printing an error once. If you run your 
application twice you end up thinking there was an issue just on the 
first run but it's actually always wrong.


Unless we're willing to take the syncobj lock for longer periods of time 
when adding points, I guess we'll have to defer validation to validation 
layers.


-Lionel

>
>>
>>
>> -Lionel
>>
>>
>>> +    dma_fence_chain_init(chain, prev, fence, point);
>>> +    rcu_assign_pointer(syncobj->fence, &chain->base);
>>> +
>>> +    list_for_each_entry_safe(cur, tmp, &syncobj->cb_list, node) {
>>> +        list_del_init(&cur->node);
>>> +        syncobj_wait_syncobj_func(syncobj, cur);
>>> +    }
>>> +    spin_unlock(&syncobj->lock);
>>> +
>>> +    /* Walk the chain once to trigger garbage collection */
>>> +    dma_fence_chain_for_each(fence, prev);
>>> +    dma_fence_put(prev);
>>> +}
>>> +EXPORT_SYMBOL(drm_syncobj_add_point);
>>> +
>>>   /**
>>>    * drm_syncobj_replace_fence - replace fence in a sync object.
>>>    * @syncobj: Sync object to replace fence in
>>> diff --git a/include/drm/drm_syncobj.h b/include/drm/drm_syncobj.h
>>> index 0311c9fdbd2f..6cf7243a1dc5 100644
>>> --- a/include/drm/drm_syncobj.h
>>> +++ b/include/drm/drm_syncobj.h
>>> @@ -27,6 +27,7 @@
>>>   #define __DRM_SYNCOBJ_H__
>>>     #include <linux/dma-fence.h>
>>> +#include <linux/dma-fence-chain.h>
>>>     struct drm_file;
>>>   @@ -112,6 +113,10 @@ drm_syncobj_fence_get(struct drm_syncobj 
>>> *syncobj)
>>>     struct drm_syncobj *drm_syncobj_find(struct drm_file *file_private,
>>>                        u32 handle);
>>> +void drm_syncobj_add_point(struct drm_syncobj *syncobj,
>>> +               struct dma_fence_chain *chain,
>>> +               struct dma_fence *fence,
>>> +               uint64_t point);
>>>   void drm_syncobj_replace_fence(struct drm_syncobj *syncobj,
>>>                      struct dma_fence *fence);
>>>   int drm_syncobj_find_fence(struct drm_file *file_private,
>>
>>
>
>

Chunming Zhou April 1, 2019, 3:54 a.m. UTC | #5

> -----Original Message-----
> From: Lionel Landwerlin <[e-mail]>
> Sent: Saturday, March 30, 2019 10:09 PM
> To: Koenig, Christian <[e-mail]>; Zhou, David(ChunMing)
> <[e-mail]>; [e-mail]; amd-
> [e-mail]; [e-mail]; Hector, Tobias
> <[e-mail]>
> Subject: Re: [PATCH 2/9] drm/syncobj: add new drm_syncobj_add_point
> interface v4
> 
> On 28/03/2019 15:18, Christian König wrote:
> > Am 28.03.19 um 14:50 schrieb Lionel Landwerlin:
> >> On 25/03/2019 08:32, Chunming Zhou wrote:
> >>> From: Christian König <[e-mail]>
> >>>
> >>> Use the dma_fence_chain object to create a timeline of fence objects
> >>> instead of just replacing the existing fence.
> >>>
> >>> v2: rebase and cleanup
> >>> v3: fix garbage collection parameters
> >>> v4: add unorder point check, print a warn calltrace
> >>>
> >>> Signed-off-by: Christian König <[e-mail]>
> >>> Cc: Lionel Landwerlin <[e-mail]>
> >>> ---
> >>>   drivers/gpu/drm/drm_syncobj.c | 39
> >>> +++++++++++++++++++++++++++++++++++
> >>>   include/drm/drm_syncobj.h     |  5 +++++
> >>>   2 files changed, 44 insertions(+)
> >>>
> >>> diff --git a/drivers/gpu/drm/drm_syncobj.c
> >>> b/drivers/gpu/drm/drm_syncobj.c index 5329e66598c6..19a9ce638119
> >>> 100644
> >>> --- a/drivers/gpu/drm/drm_syncobj.c
> >>> +++ b/drivers/gpu/drm/drm_syncobj.c
> >>> @@ -122,6 +122,45 @@ static void drm_syncobj_remove_wait(struct
> >>> drm_syncobj *syncobj,
> >>>       spin_unlock(&syncobj->lock);
> >>>   }
> >>>   +/**
> >>> + * drm_syncobj_add_point - add new timeline point to the syncobj
> >>> + * @syncobj: sync object to add timeline point do
> >>> + * @chain: chain node to use to add the point
> >>> + * @fence: fence to encapsulate in the chain node
> >>> + * @point: sequence number to use for the point
> >>> + *
> >>> + * Add the chain node as new timeline point to the syncobj.
> >>> + */
> >>> +void drm_syncobj_add_point(struct drm_syncobj *syncobj,
> >>> +               struct dma_fence_chain *chain,
> >>> +               struct dma_fence *fence,
> >>> +               uint64_t point)
> >>> +{
> >>> +    struct syncobj_wait_entry *cur, *tmp;
> >>> +    struct dma_fence *prev;
> >>> +
> >>> +    dma_fence_get(fence);
> >>> +
> >>> +    spin_lock(&syncobj->lock);
> >>> +
> >>> +    prev = drm_syncobj_fence_get(syncobj);
> >>> +    /* You are adding an unorder point to timeline, which could
> >>> cause payload returned from query_ioctl is 0! */
> >>> +    WARN_ON_ONCE(prev && prev->seqno >= point);
> >>
> >>
> >> I think the WARN/BUG macros should only fire when there is an issue
> >> with programming from within the kernel.
> >>
> >> But this particular warning can be triggered by an application.
> >>
> >>
> >> Probably best to just remove it?
> >
> > Yeah, that was also my argument against it.
> >
> > Key point here is that we still want to note somehow that userspace
> > did something wrong and returning an error is not an option.
> >
> > Maybe just use DRM_ERROR with a static variable to print the message
> > only once.
> >
> > Christian.
> 
> I don't really see any point in printing an error once. If you run your
> application twice you end up thinking there was an issue just on the first run
> but it's actually always wrong.
> 

Except this nitpick, is there any other concern to push whole patch set? Is that time to push whole patch set?

-David

> 
> Unless we're willing to take the syncobj lock for longer periods of time when
> adding points, I guess we'll have to defer validation to validation layers.
> 
> 
> -Lionel
> 
> >
> >>
> >>
> >> -Lionel
> >>
> >>
> >>> +    dma_fence_chain_init(chain, prev, fence, point);
> >>> +    rcu_assign_pointer(syncobj->fence, &chain->base);
> >>> +
> >>> +    list_for_each_entry_safe(cur, tmp, &syncobj->cb_list, node) {
> >>> +        list_del_init(&cur->node);
> >>> +        syncobj_wait_syncobj_func(syncobj, cur);
> >>> +    }
> >>> +    spin_unlock(&syncobj->lock);
> >>> +
> >>> +    /* Walk the chain once to trigger garbage collection */
> >>> +    dma_fence_chain_for_each(fence, prev);
> >>> +    dma_fence_put(prev);
> >>> +}
> >>> +EXPORT_SYMBOL(drm_syncobj_add_point);
> >>> +
> >>>   /**
> >>>    * drm_syncobj_replace_fence - replace fence in a sync object.
> >>>    * @syncobj: Sync object to replace fence in diff --git
> >>> a/include/drm/drm_syncobj.h b/include/drm/drm_syncobj.h index
> >>> 0311c9fdbd2f..6cf7243a1dc5 100644
> >>> --- a/include/drm/drm_syncobj.h
> >>> +++ b/include/drm/drm_syncobj.h
> >>> @@ -27,6 +27,7 @@
> >>>   #define __DRM_SYNCOBJ_H__
> >>>     #include <linux/dma-fence.h>
> >>> +#include <linux/dma-fence-chain.h>
> >>>     struct drm_file;
> >>>   @@ -112,6 +113,10 @@ drm_syncobj_fence_get(struct drm_syncobj
> >>> *syncobj)
> >>>     struct drm_syncobj *drm_syncobj_find(struct drm_file
> >>> *file_private,
> >>>                        u32 handle);
> >>> +void drm_syncobj_add_point(struct drm_syncobj *syncobj,
> >>> +               struct dma_fence_chain *chain,
> >>> +               struct dma_fence *fence,
> >>> +               uint64_t point);
> >>>   void drm_syncobj_replace_fence(struct drm_syncobj *syncobj,
> >>>                      struct dma_fence *fence);
> >>>   int drm_syncobj_find_fence(struct drm_file *file_private,
> >>
> >>
> >
> >

Lionel Landwerlin April 1, 2019, 8:19 a.m. UTC | #6

On 01/04/2019 06:54, Zhou, David(ChunMing) wrote:
>
>> -----Original Message-----
>> From: Lionel Landwerlin <[e-mail]>
>> Sent: Saturday, March 30, 2019 10:09 PM
>> To: Koenig, Christian <[e-mail]>; Zhou, David(ChunMing)
>> <[e-mail]>; [e-mail]; amd-
>> [e-mail]; [e-mail]; Hector, Tobias
>> <[e-mail]>
>> Subject: Re: [PATCH 2/9] drm/syncobj: add new drm_syncobj_add_point
>> interface v4
>>
>> On 28/03/2019 15:18, Christian König wrote:
>>> Am 28.03.19 um 14:50 schrieb Lionel Landwerlin:
>>>> On 25/03/2019 08:32, Chunming Zhou wrote:
>>>>> From: Christian König <[e-mail]>
>>>>>
>>>>> Use the dma_fence_chain object to create a timeline of fence objects
>>>>> instead of just replacing the existing fence.
>>>>>
>>>>> v2: rebase and cleanup
>>>>> v3: fix garbage collection parameters
>>>>> v4: add unorder point check, print a warn calltrace
>>>>>
>>>>> Signed-off-by: Christian König <[e-mail]>
>>>>> Cc: Lionel Landwerlin <[e-mail]>
>>>>> ---
>>>>>    drivers/gpu/drm/drm_syncobj.c | 39
>>>>> +++++++++++++++++++++++++++++++++++
>>>>>    include/drm/drm_syncobj.h     |  5 +++++
>>>>>    2 files changed, 44 insertions(+)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/drm_syncobj.c
>>>>> b/drivers/gpu/drm/drm_syncobj.c index 5329e66598c6..19a9ce638119
>>>>> 100644
>>>>> --- a/drivers/gpu/drm/drm_syncobj.c
>>>>> +++ b/drivers/gpu/drm/drm_syncobj.c
>>>>> @@ -122,6 +122,45 @@ static void drm_syncobj_remove_wait(struct
>>>>> drm_syncobj *syncobj,
>>>>>        spin_unlock(&syncobj->lock);
>>>>>    }
>>>>>    +/**
>>>>> + * drm_syncobj_add_point - add new timeline point to the syncobj
>>>>> + * @syncobj: sync object to add timeline point do
>>>>> + * @chain: chain node to use to add the point
>>>>> + * @fence: fence to encapsulate in the chain node
>>>>> + * @point: sequence number to use for the point
>>>>> + *
>>>>> + * Add the chain node as new timeline point to the syncobj.
>>>>> + */
>>>>> +void drm_syncobj_add_point(struct drm_syncobj *syncobj,
>>>>> +               struct dma_fence_chain *chain,
>>>>> +               struct dma_fence *fence,
>>>>> +               uint64_t point)
>>>>> +{
>>>>> +    struct syncobj_wait_entry *cur, *tmp;
>>>>> +    struct dma_fence *prev;
>>>>> +
>>>>> +    dma_fence_get(fence);
>>>>> +
>>>>> +    spin_lock(&syncobj->lock);
>>>>> +
>>>>> +    prev = drm_syncobj_fence_get(syncobj);
>>>>> +    /* You are adding an unorder point to timeline, which could
>>>>> cause payload returned from query_ioctl is 0! */
>>>>> +    WARN_ON_ONCE(prev && prev->seqno >= point);
>>>>
>>>> I think the WARN/BUG macros should only fire when there is an issue
>>>> with programming from within the kernel.
>>>>
>>>> But this particular warning can be triggered by an application.
>>>>
>>>>
>>>> Probably best to just remove it?
>>> Yeah, that was also my argument against it.
>>>
>>> Key point here is that we still want to note somehow that userspace
>>> did something wrong and returning an error is not an option.
>>>
>>> Maybe just use DRM_ERROR with a static variable to print the message
>>> only once.
>>>
>>> Christian.
>> I don't really see any point in printing an error once. If you run your
>> application twice you end up thinking there was an issue just on the first run
>> but it's actually always wrong.
>>
> Except this nitpick, is there any other concern to push whole patch set? Is that time to push whole patch set?
>
> -David


Looks good to me.

I have an additional change to make drm_syncobj_find_fence() also return 
the drm_syncobj : 
https://github.com/djdeath/linux/commit/0b7732b267b931339d71fe6f493ea6fa4eab453e

This is needed in i915 to avoid looking up the drm_syncobj handle twice.

Our driver allows to wait on the syncobj's dma_fence that we're then 
going to replace so we need to get bot the fence & syncobj at the same time.

I guess it can go in a follow up series.


-Lionel


>
>> Unless we're willing to take the syncobj lock for longer periods of time when
>> adding points, I guess we'll have to defer validation to validation layers.
>>
>>
>> -Lionel
>>
>>>>
>>>> -Lionel
>>>>
>>>>
>>>>> +    dma_fence_chain_init(chain, prev, fence, point);
>>>>> +    rcu_assign_pointer(syncobj->fence, &chain->base);
>>>>> +
>>>>> +    list_for_each_entry_safe(cur, tmp, &syncobj->cb_list, node) {
>>>>> +        list_del_init(&cur->node);
>>>>> +        syncobj_wait_syncobj_func(syncobj, cur);
>>>>> +    }
>>>>> +    spin_unlock(&syncobj->lock);
>>>>> +
>>>>> +    /* Walk the chain once to trigger garbage collection */
>>>>> +    dma_fence_chain_for_each(fence, prev);
>>>>> +    dma_fence_put(prev);
>>>>> +}
>>>>> +EXPORT_SYMBOL(drm_syncobj_add_point);
>>>>> +
>>>>>    /**
>>>>>     * drm_syncobj_replace_fence - replace fence in a sync object.
>>>>>     * @syncobj: Sync object to replace fence in diff --git
>>>>> a/include/drm/drm_syncobj.h b/include/drm/drm_syncobj.h index
>>>>> 0311c9fdbd2f..6cf7243a1dc5 100644
>>>>> --- a/include/drm/drm_syncobj.h
>>>>> +++ b/include/drm/drm_syncobj.h
>>>>> @@ -27,6 +27,7 @@
>>>>>    #define __DRM_SYNCOBJ_H__
>>>>>      #include <linux/dma-fence.h>
>>>>> +#include <linux/dma-fence-chain.h>
>>>>>      struct drm_file;
>>>>>    @@ -112,6 +113,10 @@ drm_syncobj_fence_get(struct drm_syncobj
>>>>> *syncobj)
>>>>>      struct drm_syncobj *drm_syncobj_find(struct drm_file
>>>>> *file_private,
>>>>>                         u32 handle);
>>>>> +void drm_syncobj_add_point(struct drm_syncobj *syncobj,
>>>>> +               struct dma_fence_chain *chain,
>>>>> +               struct dma_fence *fence,
>>>>> +               uint64_t point);
>>>>>    void drm_syncobj_replace_fence(struct drm_syncobj *syncobj,
>>>>>                       struct dma_fence *fence);
>>>>>    int drm_syncobj_find_fence(struct drm_file *file_private,
>>>>
>>>

Chunming Zhou April 1, 2019, 8:50 a.m. UTC | #7

On 2019年04月01日 16:19, Lionel Landwerlin wrote:
> On 01/04/2019 06:54, Zhou, David(ChunMing) wrote:
>>
>>> -----Original Message-----
>>> From: Lionel Landwerlin <[e-mail]>
>>> Sent: Saturday, March 30, 2019 10:09 PM
>>> To: Koenig, Christian <[e-mail]>; Zhou, David(ChunMing)
>>> <[e-mail]>; [e-mail]; amd-
>>> [e-mail]; [e-mail]; Hector, Tobias
>>> <[e-mail]>
>>> Subject: Re: [PATCH 2/9] drm/syncobj: add new drm_syncobj_add_point
>>> interface v4
>>>
>>> On 28/03/2019 15:18, Christian König wrote:
>>>> Am 28.03.19 um 14:50 schrieb Lionel Landwerlin:
>>>>> On 25/03/2019 08:32, Chunming Zhou wrote:
>>>>>> From: Christian König <[e-mail]>
>>>>>>
>>>>>> Use the dma_fence_chain object to create a timeline of fence objects
>>>>>> instead of just replacing the existing fence.
>>>>>>
>>>>>> v2: rebase and cleanup
>>>>>> v3: fix garbage collection parameters
>>>>>> v4: add unorder point check, print a warn calltrace
>>>>>>
>>>>>> Signed-off-by: Christian König <[e-mail]>
>>>>>> Cc: Lionel Landwerlin <[e-mail]>
>>>>>> ---
>>>>>>    drivers/gpu/drm/drm_syncobj.c | 39
>>>>>> +++++++++++++++++++++++++++++++++++
>>>>>>    include/drm/drm_syncobj.h     |  5 +++++
>>>>>>    2 files changed, 44 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/drm_syncobj.c
>>>>>> b/drivers/gpu/drm/drm_syncobj.c index 5329e66598c6..19a9ce638119
>>>>>> 100644
>>>>>> --- a/drivers/gpu/drm/drm_syncobj.c
>>>>>> +++ b/drivers/gpu/drm/drm_syncobj.c
>>>>>> @@ -122,6 +122,45 @@ static void drm_syncobj_remove_wait(struct
>>>>>> drm_syncobj *syncobj,
>>>>>>        spin_unlock(&syncobj->lock);
>>>>>>    }
>>>>>>    +/**
>>>>>> + * drm_syncobj_add_point - add new timeline point to the syncobj
>>>>>> + * @syncobj: sync object to add timeline point do
>>>>>> + * @chain: chain node to use to add the point
>>>>>> + * @fence: fence to encapsulate in the chain node
>>>>>> + * @point: sequence number to use for the point
>>>>>> + *
>>>>>> + * Add the chain node as new timeline point to the syncobj.
>>>>>> + */
>>>>>> +void drm_syncobj_add_point(struct drm_syncobj *syncobj,
>>>>>> +               struct dma_fence_chain *chain,
>>>>>> +               struct dma_fence *fence,
>>>>>> +               uint64_t point)
>>>>>> +{
>>>>>> +    struct syncobj_wait_entry *cur, *tmp;
>>>>>> +    struct dma_fence *prev;
>>>>>> +
>>>>>> +    dma_fence_get(fence);
>>>>>> +
>>>>>> +    spin_lock(&syncobj->lock);
>>>>>> +
>>>>>> +    prev = drm_syncobj_fence_get(syncobj);
>>>>>> +    /* You are adding an unorder point to timeline, which could
>>>>>> cause payload returned from query_ioctl is 0! */
>>>>>> +    WARN_ON_ONCE(prev && prev->seqno >= point);
>>>>>
>>>>> I think the WARN/BUG macros should only fire when there is an issue
>>>>> with programming from within the kernel.
>>>>>
>>>>> But this particular warning can be triggered by an application.
>>>>>
>>>>>
>>>>> Probably best to just remove it?
>>>> Yeah, that was also my argument against it.
>>>>
>>>> Key point here is that we still want to note somehow that userspace
>>>> did something wrong and returning an error is not an option.
>>>>
>>>> Maybe just use DRM_ERROR with a static variable to print the message
>>>> only once.
>>>>
>>>> Christian.
>>> I don't really see any point in printing an error once. If you run your
>>> application twice you end up thinking there was an issue just on the 
>>> first run
>>> but it's actually always wrong.
>>>
>> Except this nitpick, is there any other concern to push whole patch 
>> set? Is that time to push whole patch set?
>>
>> -David
>
>
> Looks good to me.
Does that mean we can add your RB on patch set so that we can submit the 
patch set to drm-misc-next branch?

>
> I have an additional change to make drm_syncobj_find_fence() also 
> return the drm_syncobj : 
> https://github.com/djdeath/linux/commit/0b7732b267b931339d71fe6f493ea6fa4eab453e
>
> This is needed in i915 to avoid looking up the drm_syncobj handle twice.
>
> Our driver allows to wait on the syncobj's dma_fence that we're then 
> going to replace so we need to get bot the fence & syncobj at the same 
> time.
>
> I guess it can go in a follow up series.
Yes, agree.

Thanks for your effort as well,
-David
>
>
> -Lionel
>
>
>>
>>> Unless we're willing to take the syncobj lock for longer periods of 
>>> time when
>>> adding points, I guess we'll have to defer validation to validation 
>>> layers.
>>>
>>>
>>> -Lionel
>>>
>>>>>
>>>>> -Lionel
>>>>>
>>>>>
>>>>>> +    dma_fence_chain_init(chain, prev, fence, point);
>>>>>> +    rcu_assign_pointer(syncobj->fence, &chain->base);
>>>>>> +
>>>>>> +    list_for_each_entry_safe(cur, tmp, &syncobj->cb_list, node) {
>>>>>> +        list_del_init(&cur->node);
>>>>>> +        syncobj_wait_syncobj_func(syncobj, cur);
>>>>>> +    }
>>>>>> +    spin_unlock(&syncobj->lock);
>>>>>> +
>>>>>> +    /* Walk the chain once to trigger garbage collection */
>>>>>> +    dma_fence_chain_for_each(fence, prev);
>>>>>> +    dma_fence_put(prev);
>>>>>> +}
>>>>>> +EXPORT_SYMBOL(drm_syncobj_add_point);
>>>>>> +
>>>>>>    /**
>>>>>>     * drm_syncobj_replace_fence - replace fence in a sync object.
>>>>>>     * @syncobj: Sync object to replace fence in diff --git
>>>>>> a/include/drm/drm_syncobj.h b/include/drm/drm_syncobj.h index
>>>>>> 0311c9fdbd2f..6cf7243a1dc5 100644
>>>>>> --- a/include/drm/drm_syncobj.h
>>>>>> +++ b/include/drm/drm_syncobj.h
>>>>>> @@ -27,6 +27,7 @@
>>>>>>    #define __DRM_SYNCOBJ_H__
>>>>>>      #include <linux/dma-fence.h>
>>>>>> +#include <linux/dma-fence-chain.h>
>>>>>>      struct drm_file;
>>>>>>    @@ -112,6 +113,10 @@ drm_syncobj_fence_get(struct drm_syncobj
>>>>>> *syncobj)
>>>>>>      struct drm_syncobj *drm_syncobj_find(struct drm_file
>>>>>> *file_private,
>>>>>>                         u32 handle);
>>>>>> +void drm_syncobj_add_point(struct drm_syncobj *syncobj,
>>>>>> +               struct dma_fence_chain *chain,
>>>>>> +               struct dma_fence *fence,
>>>>>> +               uint64_t point);
>>>>>>    void drm_syncobj_replace_fence(struct drm_syncobj *syncobj,
>>>>>>                       struct dma_fence *fence);
>>>>>>    int drm_syncobj_find_fence(struct drm_file *file_private,
>>>>>
>>>>
>

Lionel Landwerlin April 1, 2019, 9:05 a.m. UTC | #8

On 01/04/2019 11:50, zhoucm1 wrote:
>
>
> On 2019年04月01日 16:19, Lionel Landwerlin wrote:
>> On 01/04/2019 06:54, Zhou, David(ChunMing) wrote:
>>>
>>>> -----Original Message-----
>>>> From: Lionel Landwerlin <[e-mail]>
>>>> Sent: Saturday, March 30, 2019 10:09 PM
>>>> To: Koenig, Christian <[e-mail]>; Zhou, 
>>>> David(ChunMing)
>>>> <[e-mail]>; [e-mail]; amd-
>>>> [e-mail]; [e-mail]; Hector, Tobias
>>>> <[e-mail]>
>>>> Subject: Re: [PATCH 2/9] drm/syncobj: add new drm_syncobj_add_point
>>>> interface v4
>>>>
>>>> On 28/03/2019 15:18, Christian König wrote:
>>>>> Am 28.03.19 um 14:50 schrieb Lionel Landwerlin:
>>>>>> On 25/03/2019 08:32, Chunming Zhou wrote:
>>>>>>> From: Christian König <[e-mail]>
>>>>>>>
>>>>>>> Use the dma_fence_chain object to create a timeline of fence 
>>>>>>> objects
>>>>>>> instead of just replacing the existing fence.
>>>>>>>
>>>>>>> v2: rebase and cleanup
>>>>>>> v3: fix garbage collection parameters
>>>>>>> v4: add unorder point check, print a warn calltrace
>>>>>>>
>>>>>>> Signed-off-by: Christian König <[e-mail]>
>>>>>>> Cc: Lionel Landwerlin <[e-mail]>
>>>>>>> ---
>>>>>>>    drivers/gpu/drm/drm_syncobj.c | 39
>>>>>>> +++++++++++++++++++++++++++++++++++
>>>>>>>    include/drm/drm_syncobj.h     |  5 +++++
>>>>>>>    2 files changed, 44 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/drm_syncobj.c
>>>>>>> b/drivers/gpu/drm/drm_syncobj.c index 5329e66598c6..19a9ce638119
>>>>>>> 100644
>>>>>>> --- a/drivers/gpu/drm/drm_syncobj.c
>>>>>>> +++ b/drivers/gpu/drm/drm_syncobj.c
>>>>>>> @@ -122,6 +122,45 @@ static void drm_syncobj_remove_wait(struct
>>>>>>> drm_syncobj *syncobj,
>>>>>>>        spin_unlock(&syncobj->lock);
>>>>>>>    }
>>>>>>>    +/**
>>>>>>> + * drm_syncobj_add_point - add new timeline point to the syncobj
>>>>>>> + * @syncobj: sync object to add timeline point do
>>>>>>> + * @chain: chain node to use to add the point
>>>>>>> + * @fence: fence to encapsulate in the chain node
>>>>>>> + * @point: sequence number to use for the point
>>>>>>> + *
>>>>>>> + * Add the chain node as new timeline point to the syncobj.
>>>>>>> + */
>>>>>>> +void drm_syncobj_add_point(struct drm_syncobj *syncobj,
>>>>>>> +               struct dma_fence_chain *chain,
>>>>>>> +               struct dma_fence *fence,
>>>>>>> +               uint64_t point)
>>>>>>> +{
>>>>>>> +    struct syncobj_wait_entry *cur, *tmp;
>>>>>>> +    struct dma_fence *prev;
>>>>>>> +
>>>>>>> +    dma_fence_get(fence);
>>>>>>> +
>>>>>>> +    spin_lock(&syncobj->lock);
>>>>>>> +
>>>>>>> +    prev = drm_syncobj_fence_get(syncobj);
>>>>>>> +    /* You are adding an unorder point to timeline, which could
>>>>>>> cause payload returned from query_ioctl is 0! */
>>>>>>> +    WARN_ON_ONCE(prev && prev->seqno >= point);
>>>>>>
>>>>>> I think the WARN/BUG macros should only fire when there is an issue
>>>>>> with programming from within the kernel.
>>>>>>
>>>>>> But this particular warning can be triggered by an application.
>>>>>>
>>>>>>
>>>>>> Probably best to just remove it?
>>>>> Yeah, that was also my argument against it.
>>>>>
>>>>> Key point here is that we still want to note somehow that userspace
>>>>> did something wrong and returning an error is not an option.
>>>>>
>>>>> Maybe just use DRM_ERROR with a static variable to print the message
>>>>> only once.
>>>>>
>>>>> Christian.
>>>> I don't really see any point in printing an error once. If you run 
>>>> your
>>>> application twice you end up thinking there was an issue just on 
>>>> the first run
>>>> but it's actually always wrong.
>>>>
>>> Except this nitpick, is there any other concern to push whole patch 
>>> set? Is that time to push whole patch set?
>>>
>>> -David
>>
>>
>> Looks good to me.
> Does that mean we can add your RB on patch set so that we can submit 
> the patch set to drm-misc-next branch?


Yes :

Reviewed-by: Lionel Landwerlin <[e-mail]>


Not too sure about the process for drm-misc-next. Sounds like Sumit 
Semwal <[e-mail]> is the go to person according to 
MAINTAINERS.


>
>>
>> I have an additional change to make drm_syncobj_find_fence() also 
>> return the drm_syncobj : 
>> https://github.com/djdeath/linux/commit/0b7732b267b931339d71fe6f493ea6fa4eab453e
>>
>> This is needed in i915 to avoid looking up the drm_syncobj handle twice.
>>
>> Our driver allows to wait on the syncobj's dma_fence that we're then 
>> going to replace so we need to get bot the fence & syncobj at the 
>> same time.
>>
>> I guess it can go in a follow up series.
> Yes, agree.
>
> Thanks for your effort as well,
> -David


Thanks to you!


-Lionel


>>
>>
>> -Lionel
>>
>>
>>>
>>>> Unless we're willing to take the syncobj lock for longer periods of 
>>>> time when
>>>> adding points, I guess we'll have to defer validation to validation 
>>>> layers.
>>>>
>>>>
>>>> -Lionel
>>>>
>>>>>>
>>>>>> -Lionel
>>>>>>
>>>>>>
>>>>>>> +    dma_fence_chain_init(chain, prev, fence, point);
>>>>>>> +    rcu_assign_pointer(syncobj->fence, &chain->base);
>>>>>>> +
>>>>>>> +    list_for_each_entry_safe(cur, tmp, &syncobj->cb_list, node) {
>>>>>>> +        list_del_init(&cur->node);
>>>>>>> +        syncobj_wait_syncobj_func(syncobj, cur);
>>>>>>> +    }
>>>>>>> +    spin_unlock(&syncobj->lock);
>>>>>>> +
>>>>>>> +    /* Walk the chain once to trigger garbage collection */
>>>>>>> +    dma_fence_chain_for_each(fence, prev);
>>>>>>> +    dma_fence_put(prev);
>>>>>>> +}
>>>>>>> +EXPORT_SYMBOL(drm_syncobj_add_point);
>>>>>>> +
>>>>>>>    /**
>>>>>>>     * drm_syncobj_replace_fence - replace fence in a sync object.
>>>>>>>     * @syncobj: Sync object to replace fence in diff --git
>>>>>>> a/include/drm/drm_syncobj.h b/include/drm/drm_syncobj.h index
>>>>>>> 0311c9fdbd2f..6cf7243a1dc5 100644
>>>>>>> --- a/include/drm/drm_syncobj.h
>>>>>>> +++ b/include/drm/drm_syncobj.h
>>>>>>> @@ -27,6 +27,7 @@
>>>>>>>    #define __DRM_SYNCOBJ_H__
>>>>>>>      #include <linux/dma-fence.h>
>>>>>>> +#include <linux/dma-fence-chain.h>
>>>>>>>      struct drm_file;
>>>>>>>    @@ -112,6 +113,10 @@ drm_syncobj_fence_get(struct drm_syncobj
>>>>>>> *syncobj)
>>>>>>>      struct drm_syncobj *drm_syncobj_find(struct drm_file
>>>>>>> *file_private,
>>>>>>>                         u32 handle);
>>>>>>> +void drm_syncobj_add_point(struct drm_syncobj *syncobj,
>>>>>>> +               struct dma_fence_chain *chain,
>>>>>>> +               struct dma_fence *fence,
>>>>>>> +               uint64_t point);
>>>>>>>    void drm_syncobj_replace_fence(struct drm_syncobj *syncobj,
>>>>>>>                       struct dma_fence *fence);
>>>>>>>    int drm_syncobj_find_fence(struct drm_file *file_private,
>>>>>>
>>>>>
>>
>
>

Christian König April 1, 2019, 9:10 a.m. UTC | #9

Am 01.04.19 um 11:05 schrieb Lionel Landwerlin:
> On 01/04/2019 11:50, zhoucm1 wrote:
>>
>>
>> On 2019年04月01日 16:19, Lionel Landwerlin wrote:
>>> On 01/04/2019 06:54, Zhou, David(ChunMing) wrote:
>>>>
>>>>> -----Original Message-----
>>>>> From: Lionel Landwerlin <[e-mail]>
>>>>> Sent: Saturday, March 30, 2019 10:09 PM
>>>>> To: Koenig, Christian <[e-mail]>; Zhou, 
>>>>> David(ChunMing)
>>>>> <[e-mail]>; [e-mail]; amd-
>>>>> [e-mail]; [e-mail]; Hector, Tobias
>>>>> <[e-mail]>
>>>>> Subject: Re: [PATCH 2/9] drm/syncobj: add new drm_syncobj_add_point
>>>>> interface v4
>>>>>
>>>>> On 28/03/2019 15:18, Christian König wrote:
>>>>>> Am 28.03.19 um 14:50 schrieb Lionel Landwerlin:
>>>>>>> On 25/03/2019 08:32, Chunming Zhou wrote:
>>>>>>>> From: Christian König <[e-mail]>
>>>>>>>>
>>>>>>>> Use the dma_fence_chain object to create a timeline of fence 
>>>>>>>> objects
>>>>>>>> instead of just replacing the existing fence.
>>>>>>>>
>>>>>>>> v2: rebase and cleanup
>>>>>>>> v3: fix garbage collection parameters
>>>>>>>> v4: add unorder point check, print a warn calltrace
>>>>>>>>
>>>>>>>> Signed-off-by: Christian König <[e-mail]>
>>>>>>>> Cc: Lionel Landwerlin <[e-mail]>
>>>>>>>> ---
>>>>>>>>    drivers/gpu/drm/drm_syncobj.c | 39
>>>>>>>> +++++++++++++++++++++++++++++++++++
>>>>>>>>    include/drm/drm_syncobj.h     |  5 +++++
>>>>>>>>    2 files changed, 44 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/drivers/gpu/drm/drm_syncobj.c
>>>>>>>> b/drivers/gpu/drm/drm_syncobj.c index 5329e66598c6..19a9ce638119
>>>>>>>> 100644
>>>>>>>> --- a/drivers/gpu/drm/drm_syncobj.c
>>>>>>>> +++ b/drivers/gpu/drm/drm_syncobj.c
>>>>>>>> @@ -122,6 +122,45 @@ static void drm_syncobj_remove_wait(struct
>>>>>>>> drm_syncobj *syncobj,
>>>>>>>>        spin_unlock(&syncobj->lock);
>>>>>>>>    }
>>>>>>>>    +/**
>>>>>>>> + * drm_syncobj_add_point - add new timeline point to the syncobj
>>>>>>>> + * @syncobj: sync object to add timeline point do
>>>>>>>> + * @chain: chain node to use to add the point
>>>>>>>> + * @fence: fence to encapsulate in the chain node
>>>>>>>> + * @point: sequence number to use for the point
>>>>>>>> + *
>>>>>>>> + * Add the chain node as new timeline point to the syncobj.
>>>>>>>> + */
>>>>>>>> +void drm_syncobj_add_point(struct drm_syncobj *syncobj,
>>>>>>>> +               struct dma_fence_chain *chain,
>>>>>>>> +               struct dma_fence *fence,
>>>>>>>> +               uint64_t point)
>>>>>>>> +{
>>>>>>>> +    struct syncobj_wait_entry *cur, *tmp;
>>>>>>>> +    struct dma_fence *prev;
>>>>>>>> +
>>>>>>>> +    dma_fence_get(fence);
>>>>>>>> +
>>>>>>>> +    spin_lock(&syncobj->lock);
>>>>>>>> +
>>>>>>>> +    prev = drm_syncobj_fence_get(syncobj);
>>>>>>>> +    /* You are adding an unorder point to timeline, which could
>>>>>>>> cause payload returned from query_ioctl is 0! */
>>>>>>>> +    WARN_ON_ONCE(prev && prev->seqno >= point);
>>>>>>>
>>>>>>> I think the WARN/BUG macros should only fire when there is an issue
>>>>>>> with programming from within the kernel.
>>>>>>>
>>>>>>> But this particular warning can be triggered by an application.
>>>>>>>
>>>>>>>
>>>>>>> Probably best to just remove it?
>>>>>> Yeah, that was also my argument against it.
>>>>>>
>>>>>> Key point here is that we still want to note somehow that userspace
>>>>>> did something wrong and returning an error is not an option.
>>>>>>
>>>>>> Maybe just use DRM_ERROR with a static variable to print the message
>>>>>> only once.
>>>>>>
>>>>>> Christian.
>>>>> I don't really see any point in printing an error once. If you run 
>>>>> your
>>>>> application twice you end up thinking there was an issue just on 
>>>>> the first run
>>>>> but it's actually always wrong.
>>>>>
>>>> Except this nitpick, is there any other concern to push whole patch 
>>>> set? Is that time to push whole patch set?
>>>>
>>>> -David
>>>
>>>
>>> Looks good to me.
>> Does that mean we can add your RB on patch set so that we can submit 
>> the patch set to drm-misc-next branch?
>
>
> Yes :
>
> Reviewed-by: Lionel Landwerlin <[e-mail]>
>
>
> Not too sure about the process for drm-misc-next. Sounds like Sumit 
> Semwal <[e-mail]> is the go to person according to 
> MAINTAINERS.

Well, I've got commit rights as well.

Going to remove the warning, add your rb and push everything if nobody 
objects in the next hour or so.

Christian.

>
>
>>
>>>
>>> I have an additional change to make drm_syncobj_find_fence() also 
>>> return the drm_syncobj : 
>>> https://github.com/djdeath/linux/commit/0b7732b267b931339d71fe6f493ea6fa4eab453e
>>>
>>> This is needed in i915 to avoid looking up the drm_syncobj handle 
>>> twice.
>>>
>>> Our driver allows to wait on the syncobj's dma_fence that we're then 
>>> going to replace so we need to get bot the fence & syncobj at the 
>>> same time.
>>>
>>> I guess it can go in a follow up series.
>> Yes, agree.
>>
>> Thanks for your effort as well,
>> -David
>
>
> Thanks to you!
>
>
> -Lionel
>
>
>>>
>>>
>>> -Lionel
>>>
>>>
>>>>
>>>>> Unless we're willing to take the syncobj lock for longer periods 
>>>>> of time when
>>>>> adding points, I guess we'll have to defer validation to 
>>>>> validation layers.
>>>>>
>>>>>
>>>>> -Lionel
>>>>>
>>>>>>>
>>>>>>> -Lionel
>>>>>>>
>>>>>>>
>>>>>>>> + dma_fence_chain_init(chain, prev, fence, point);
>>>>>>>> +    rcu_assign_pointer(syncobj->fence, &chain->base);
>>>>>>>> +
>>>>>>>> +    list_for_each_entry_safe(cur, tmp, &syncobj->cb_list, node) {
>>>>>>>> +        list_del_init(&cur->node);
>>>>>>>> +        syncobj_wait_syncobj_func(syncobj, cur);
>>>>>>>> +    }
>>>>>>>> +    spin_unlock(&syncobj->lock);
>>>>>>>> +
>>>>>>>> +    /* Walk the chain once to trigger garbage collection */
>>>>>>>> +    dma_fence_chain_for_each(fence, prev);
>>>>>>>> +    dma_fence_put(prev);
>>>>>>>> +}
>>>>>>>> +EXPORT_SYMBOL(drm_syncobj_add_point);
>>>>>>>> +
>>>>>>>>    /**
>>>>>>>>     * drm_syncobj_replace_fence - replace fence in a sync object.
>>>>>>>>     * @syncobj: Sync object to replace fence in diff --git
>>>>>>>> a/include/drm/drm_syncobj.h b/include/drm/drm_syncobj.h index
>>>>>>>> 0311c9fdbd2f..6cf7243a1dc5 100644
>>>>>>>> --- a/include/drm/drm_syncobj.h
>>>>>>>> +++ b/include/drm/drm_syncobj.h
>>>>>>>> @@ -27,6 +27,7 @@
>>>>>>>>    #define __DRM_SYNCOBJ_H__
>>>>>>>>      #include <linux/dma-fence.h>
>>>>>>>> +#include <linux/dma-fence-chain.h>
>>>>>>>>      struct drm_file;
>>>>>>>>    @@ -112,6 +113,10 @@ drm_syncobj_fence_get(struct drm_syncobj
>>>>>>>> *syncobj)
>>>>>>>>      struct drm_syncobj *drm_syncobj_find(struct drm_file
>>>>>>>> *file_private,
>>>>>>>>                         u32 handle);
>>>>>>>> +void drm_syncobj_add_point(struct drm_syncobj *syncobj,
>>>>>>>> +               struct dma_fence_chain *chain,
>>>>>>>> +               struct dma_fence *fence,
>>>>>>>> +               uint64_t point);
>>>>>>>>    void drm_syncobj_replace_fence(struct drm_syncobj *syncobj,
>>>>>>>>                       struct dma_fence *fence);
>>>>>>>>    int drm_syncobj_find_fence(struct drm_file *file_private,
>>>>>>>
>>>>>>
>>>
>>
>>
>

Dave Airlie April 15, 2019, 6:54 p.m. UTC | #10

> Well, I've got commit rights as well.
>
> Going to remove the warning, add your rb and push everything if nobody
> objects in the next hour or so.

So this got committed and is in my -next tree, but where is the
userspace and igt tests?

There needs to be a functional mesa userspace and a set of IGTs for
this, maybe I've overlooked them if I haven't we can't ship this.

Dave.

Christian König April 15, 2019, 6:56 p.m. UTC | #11

Am 15.04.19 um 20:54 schrieb Dave Airlie:
>> Well, I've got commit rights as well.
>>
>> Going to remove the warning, add your rb and push everything if nobody
>> objects in the next hour or so.
> So this got committed and is in my -next tree, but where is the
> userspace and igt tests?

I was waiting for this to land in -next before pushing the libdrm 
changes with amdgpu test cases.

As soon as those are committed Lionel and David wanted to commit the igt 
tests.

Christian.

>
> There needs to be a functional mesa userspace and a set of IGTs for
> this, maybe I've overlooked them if I haven't we can't ship this.
>
> Dave.

Daniel Vetter April 15, 2019, 7:06 p.m. UTC | #12

On Mon, Apr 15, 2019 at 8:56 PM Koenig, Christian
<[e-mail]> wrote:
>
> Am 15.04.19 um 20:54 schrieb Dave Airlie:
> >> Well, I've got commit rights as well.
> >>
> >> Going to remove the warning, add your rb and push everything if nobody
> >> objects in the next hour or so.
> > So this got committed and is in my -next tree, but where is the
> > userspace and igt tests?
>
> I was waiting for this to land in -next before pushing the libdrm
> changes with amdgpu test cases.
>
> As soon as those are committed Lionel and David wanted to commit the igt
> tests.

Testcases are ofc great, but that's not the userspace we're looking
for :-) All still khr embargo? Even if that's all available at under
khr nda, and reviewed by relevant folks I'm not sure we're setting a
great precedence here - if khr still wants to be able to change the
interface spec, casting the kernel uapi into stone is a bit premature.
Aside from that this excludes everyone without khr nda, which isn't a
nice move really. Note: Picky because this is winsys stuff, so
relevant beyond just a single driver - for driver/hw specific stuff it
makes sense to allow plenty of leeway to work around various embargos.
-Daniel

Patch

diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
index 5329e66598c6..19a9ce638119 100644
--- a/drivers/gpu/drm/drm_syncobj.c
+++ b/drivers/gpu/drm/drm_syncobj.c
@@ -122,6 +122,45 @@  static void drm_syncobj_remove_wait(struct drm_syncobj *syncobj,
 	spin_unlock(&syncobj->lock);
 }
 
+/**
+ * drm_syncobj_add_point - add new timeline point to the syncobj
+ * @syncobj: sync object to add timeline point do
+ * @chain: chain node to use to add the point
+ * @fence: fence to encapsulate in the chain node
+ * @point: sequence number to use for the point
+ *
+ * Add the chain node as new timeline point to the syncobj.
+ */
+void drm_syncobj_add_point(struct drm_syncobj *syncobj,
+			   struct dma_fence_chain *chain,
+			   struct dma_fence *fence,
+			   uint64_t point)
+{
+	struct syncobj_wait_entry *cur, *tmp;
+	struct dma_fence *prev;
+
+	dma_fence_get(fence);
+
+	spin_lock(&syncobj->lock);
+
+	prev = drm_syncobj_fence_get(syncobj);
+	/* You are adding an unorder point to timeline, which could cause payload returned from query_ioctl is 0! */
+	WARN_ON_ONCE(prev && prev->seqno >= point);
+	dma_fence_chain_init(chain, prev, fence, point);
+	rcu_assign_pointer(syncobj->fence, &chain->base);
+
+	list_for_each_entry_safe(cur, tmp, &syncobj->cb_list, node) {
+		list_del_init(&cur->node);
+		syncobj_wait_syncobj_func(syncobj, cur);
+	}
+	spin_unlock(&syncobj->lock);
+
+	/* Walk the chain once to trigger garbage collection */
+	dma_fence_chain_for_each(fence, prev);
+	dma_fence_put(prev);
+}
+EXPORT_SYMBOL(drm_syncobj_add_point);
+
 /**
  * drm_syncobj_replace_fence - replace fence in a sync object.
  * @syncobj: Sync object to replace fence in
diff --git a/include/drm/drm_syncobj.h b/include/drm/drm_syncobj.h
index 0311c9fdbd2f..6cf7243a1dc5 100644
--- a/include/drm/drm_syncobj.h
+++ b/include/drm/drm_syncobj.h
@@ -27,6 +27,7 @@ 
 #define __DRM_SYNCOBJ_H__
 
 #include <linux/dma-fence.h>
+#include <linux/dma-fence-chain.h>
 
 struct drm_file;
 
@@ -112,6 +113,10 @@  drm_syncobj_fence_get(struct drm_syncobj *syncobj)
 
 struct drm_syncobj *drm_syncobj_find(struct drm_file *file_private,
 				     u32 handle);
+void drm_syncobj_add_point(struct drm_syncobj *syncobj,
+			   struct dma_fence_chain *chain,
+			   struct dma_fence *fence,
+			   uint64_t point);
 void drm_syncobj_replace_fence(struct drm_syncobj *syncobj,
 			       struct dma_fence *fence);
 int drm_syncobj_find_fence(struct drm_file *file_private,