Re: [PATCH] drm/syncobj: remove boring message

Re: [PATCH] drm/syncobj: remove boring message

[Koenig, Christian]

[-- Attachment #1.1: Type: text/plain, Size: 3985 bytes --]



Am 02.08.2019 18:28 schrieb Jason Ekstrand <jason@jlekstrand.net>:
On Thu, Aug 1, 2019 at 9:05 AM Koenig, Christian <Christian.Koenig@amd.com<mailto:Christian.Koenig@amd.com>> wrote:
Am 01.08.19 um 15:45 schrieb Lionel Landwerlin:
> Just had a few exchanges with Chris about this.
> Chris suggests that if we're about to add a point to the timeline in
> an unordered fashion, actually better not add it at all.
>
> What's your take on this?

That is a clear NAK. See not adding a point at all means we lose some
synchronization and that is not something we can do here.

In other words syncing to much if userspace does something nasty is ok
and defensive programmed, but not syncing at all could have unforeseen
consequences.

So if process A signals 7, process B detects that and signals 3 and then process A tries to insert something which waits on 7 and signals 8, what happens?  My understanding is that it "breaks" the timeline and so, from the perspective of process A, its signal operation on 7 is gone and it's attempt to wait on 7 will either -EINVAL because the kernel can't find the time point or else just sit there.  Am I understanding this correctly?

Nope, what happens is that we note the largest signaled seqno and wait for everything without returning an error.

For example if we have signaled 7 and then 3 then any waiting for 7 we would wait for both 3 and 7.

  If so, it sounds more like an attack vector than defensive programming to me.

Yeah, completely agree. That's why I also rejected the idea to return an error on wait.


Yes, more syncornization is generally better than less.  However, if you're screwing up your syncronization from userspace and getting wrong rendering results, that's your fault.  If you're causing your compositor to suddenly start seeing -EINVAL which gets turned into VK_ERROR_DEVICE_LOST, that's a lot worse IMO.  I'm not saying that we shouldn't try to be robust in this case; I'm just concerned that the suggest solution isn't.

Completely agree as well.

The key point is we need to find a balance between keeping things working and signaling that something is wrong.

I mean the two options we have is to either ignore such errors and do the most defensive thing we can. And the current solution is already pretty good at that.

Or we can signal those errors but risk that it can be used for a deny of service.

Regards,
Christian.


--Jason


Another idea would be to add the fence, but also set an error flag and
deny any further signaling on that syncobj.

Regards,
Christian.

> I'm fine with this, but rather than add another variant of add_point()
> maybe we change change the existing.
>
> Thanks,
>
> -Lionel
>
> On 29/07/2019 11:20, Chunming Zhou wrote:
>> It is normal that binary syncobj replaces the underlying fence.
>>
>> Signed-off-by: Chunming Zhou <david1.zhou@amd.com<mailto:david1.zhou@amd.com>>
>> ---
>>   drivers/gpu/drm/drm_syncobj.c | 3 ---
>>   1 file changed, 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_syncobj.c
>> b/drivers/gpu/drm/drm_syncobj.c
>> index 929f7c64f9a2..bc7ec1679e4d 100644
>> --- a/drivers/gpu/drm/drm_syncobj.c
>> +++ b/drivers/gpu/drm/drm_syncobj.c
>> @@ -151,9 +151,6 @@ void drm_syncobj_add_point(struct drm_syncobj
>> *syncobj,
>>       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! */
>> -    if (prev && prev->seqno >= point)
>> -        DRM_ERROR("You are adding an unorder point to timeline!\n");
>>       dma_fence_chain_init(chain, prev, fence, point);
>>       rcu_assign_pointer(syncobj->fence, &chain->base);
>
>

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org<mailto:dri-devel@lists.freedesktop.org>
https://lists.freedesktop.org/mailman/listinfo/dri-devel


[-- Attachment #1.2: Type: text/html, Size: 7073 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/syncobj: remove boring message

[Jason Ekstrand]

[-- Attachment #1.1: Type: text/plain, Size: 4691 bytes --]

On Fri, Aug 2, 2019 at 12:33 PM Koenig, Christian <Christian.Koenig@amd.com>
wrote:

>
>
> Am 02.08.2019 18:28 schrieb Jason Ekstrand <jason@jlekstrand.net>:
>
> On Thu, Aug 1, 2019 at 9:05 AM Koenig, Christian <Christian.Koenig@amd.com>
> wrote:
>
> Am 01.08.19 um 15:45 schrieb Lionel Landwerlin:
> > Just had a few exchanges with Chris about this.
> > Chris suggests that if we're about to add a point to the timeline in
> > an unordered fashion, actually better not add it at all.
> >
> > What's your take on this?
>
> That is a clear NAK. See not adding a point at all means we lose some
> synchronization and that is not something we can do here.
>
> In other words syncing to much if userspace does something nasty is ok
> and defensive programmed, but not syncing at all could have unforeseen
> consequences.
>
>
> So if process A signals 7, process B detects that and signals 3 and then
> process A tries to insert something which waits on 7 and signals 8, what
> happens?  My understanding is that it "breaks" the timeline and so, from
> the perspective of process A, its signal operation on 7 is gone and it's
> attempt to wait on 7 will either -EINVAL because the kernel can't find the
> time point or else just sit there.  Am I understanding this correctly?
>
>
> Nope, what happens is that we note the largest signaled seqno and wait for
> everything without returning an error.
>
> For example if we have signaled 7 and then 3 then any waiting for 7 we
> would wait for both 3 and 7.
>

Ok, that's reasonable behavior.  I'm sorry, I'm a bit behind on the
implementation details right now and just had a scarry conversation on
IRC.  If what you are describing is accurate, we should be ok.  We should
also make sure that we have IGT tests which ensure this. :-)

FYI, at Lionel and Daniel's request, I just sent out a patch which adds
better design docs for the current sync file (assuming people think they're
actually better).  We should review and land that and then someone should
extend it for the timeline stuff and ensure that all these behaviors are
well-documented.

--Jason



>   If so, it sounds more like an attack vector than defensive programming
> to me.
>
>
> Yeah, completely agree. That's why I also rejected the idea to return an
> error on wait.
>
>
> Yes, more syncornization is generally better than less.  However, if
> you're screwing up your syncronization from userspace and getting wrong
> rendering results, that's your fault.  If you're causing your compositor to
> suddenly start seeing -EINVAL which gets turned into VK_ERROR_DEVICE_LOST,
> that's a lot worse IMO.  I'm not saying that we shouldn't try to be robust
> in this case; I'm just concerned that the suggest solution isn't.
>
>
> Completely agree as well.
>
> The key point is we need to find a balance between keeping things working
> and signaling that something is wrong.
>
> I mean the two options we have is to either ignore such errors and do the
> most defensive thing we can. And the current solution is already pretty
> good at that.
>
> Or we can signal those errors but risk that it can be used for a deny of
> service.
>
> Regards,
> Christian.
>
>
> --Jason
>
>
>
> Another idea would be to add the fence, but also set an error flag and
> deny any further signaling on that syncobj.
>
> Regards,
> Christian.
>
> > I'm fine with this, but rather than add another variant of add_point()
> > maybe we change change the existing.
> >
> > Thanks,
> >
> > -Lionel
> >
> > On 29/07/2019 11:20, Chunming Zhou wrote:
> >> It is normal that binary syncobj replaces the underlying fence.
> >>
> >> Signed-off-by: Chunming Zhou <david1.zhou@amd.com>
> >> ---
> >>   drivers/gpu/drm/drm_syncobj.c | 3 ---
> >>   1 file changed, 3 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/drm_syncobj.c
> >> b/drivers/gpu/drm/drm_syncobj.c
> >> index 929f7c64f9a2..bc7ec1679e4d 100644
> >> --- a/drivers/gpu/drm/drm_syncobj.c
> >> +++ b/drivers/gpu/drm/drm_syncobj.c
> >> @@ -151,9 +151,6 @@ void drm_syncobj_add_point(struct drm_syncobj
> >> *syncobj,
> >>       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! */
> >> -    if (prev && prev->seqno >= point)
> >> -        DRM_ERROR("You are adding an unorder point to timeline!\n");
> >>       dma_fence_chain_init(chain, prev, fence, point);
> >>       rcu_assign_pointer(syncobj->fence, &chain->base);
> >
> >
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
>
>

[-- Attachment #1.2: Type: text/html, Size: 8473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/syncobj: remove boring message

[Jason Ekstrand]

[-- Attachment #1.1: Type: text/plain, Size: 3120 bytes --]

On Thu, Aug 1, 2019 at 9:05 AM Koenig, Christian <Christian.Koenig@amd.com>
wrote:

> Am 01.08.19 um 15:45 schrieb Lionel Landwerlin:
> > Just had a few exchanges with Chris about this.
> > Chris suggests that if we're about to add a point to the timeline in
> > an unordered fashion, actually better not add it at all.
> >
> > What's your take on this?
>
> That is a clear NAK. See not adding a point at all means we lose some
> synchronization and that is not something we can do here.
>
> In other words syncing to much if userspace does something nasty is ok
> and defensive programmed, but not syncing at all could have unforeseen
> consequences.
>

So if process A signals 7, process B detects that and signals 3 and then
process A tries to insert something which waits on 7 and signals 8, what
happens?  My understanding is that it "breaks" the timeline and so, from
the perspective of process A, its signal operation on 7 is gone and it's
attempt to wait on 7 will either -EINVAL because the kernel can't find the
time point or else just sit there.  Am I understanding this correctly?  If
so, it sounds more like an attack vector than defensive programming to me.

Yes, more syncornization is generally better than less.  However, if you're
screwing up your syncronization from userspace and getting wrong rendering
results, that's your fault.  If you're causing your compositor to suddenly
start seeing -EINVAL which gets turned into VK_ERROR_DEVICE_LOST, that's a
lot worse IMO.  I'm not saying that we shouldn't try to be robust in this
case; I'm just concerned that the suggest solution isn't.

--Jason



> Another idea would be to add the fence, but also set an error flag and
> deny any further signaling on that syncobj.
>
> Regards,
> Christian.
>
> > I'm fine with this, but rather than add another variant of add_point()
> > maybe we change change the existing.
> >
> > Thanks,
> >
> > -Lionel
> >
> > On 29/07/2019 11:20, Chunming Zhou wrote:
> >> It is normal that binary syncobj replaces the underlying fence.
> >>
> >> Signed-off-by: Chunming Zhou <david1.zhou@amd.com>
> >> ---
> >>   drivers/gpu/drm/drm_syncobj.c | 3 ---
> >>   1 file changed, 3 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/drm_syncobj.c
> >> b/drivers/gpu/drm/drm_syncobj.c
> >> index 929f7c64f9a2..bc7ec1679e4d 100644
> >> --- a/drivers/gpu/drm/drm_syncobj.c
> >> +++ b/drivers/gpu/drm/drm_syncobj.c
> >> @@ -151,9 +151,6 @@ void drm_syncobj_add_point(struct drm_syncobj
> >> *syncobj,
> >>       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! */
> >> -    if (prev && prev->seqno >= point)
> >> -        DRM_ERROR("You are adding an unorder point to timeline!\n");
> >>       dma_fence_chain_init(chain, prev, fence, point);
> >>       rcu_assign_pointer(syncobj->fence, &chain->base);
> >
> >
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

[-- Attachment #1.2: Type: text/html, Size: 4365 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/syncobj: remove boring message

[Koenig, Christian]

Am 01.08.19 um 15:45 schrieb Lionel Landwerlin:
> Just had a few exchanges with Chris about this.
> Chris suggests that if we're about to add a point to the timeline in 
> an unordered fashion, actually better not add it at all.
>
> What's your take on this?

That is a clear NAK. See not adding a point at all means we lose some 
synchronization and that is not something we can do here.

In other words syncing to much if userspace does something nasty is ok 
and defensive programmed, but not syncing at all could have unforeseen 
consequences.

Another idea would be to add the fence, but also set an error flag and 
deny any further signaling on that syncobj.

Regards,
Christian.

> I'm fine with this, but rather than add another variant of add_point() 
> maybe we change change the existing.
>
> Thanks,
>
> -Lionel
>
> On 29/07/2019 11:20, Chunming Zhou wrote:
>> It is normal that binary syncobj replaces the underlying fence.
>>
>> Signed-off-by: Chunming Zhou <david1.zhou@amd.com>
>> ---
>>   drivers/gpu/drm/drm_syncobj.c | 3 ---
>>   1 file changed, 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_syncobj.c 
>> b/drivers/gpu/drm/drm_syncobj.c
>> index 929f7c64f9a2..bc7ec1679e4d 100644
>> --- a/drivers/gpu/drm/drm_syncobj.c
>> +++ b/drivers/gpu/drm/drm_syncobj.c
>> @@ -151,9 +151,6 @@ void drm_syncobj_add_point(struct drm_syncobj 
>> *syncobj,
>>       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! */
>> -    if (prev && prev->seqno >= point)
>> -        DRM_ERROR("You are adding an unorder point to timeline!\n");
>>       dma_fence_chain_init(chain, prev, fence, point);
>>       rcu_assign_pointer(syncobj->fence, &chain->base);
>
>

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/syncobj: remove boring message

[Lionel Landwerlin]

Just had a few exchanges with Chris about this.
Chris suggests that if we're about to add a point to the timeline in an 
unordered fashion, actually better not add it at all.

What's your take on this?
I'm fine with this, but rather than add another variant of add_point() 
maybe we change change the existing.

Thanks,

-Lionel

On 29/07/2019 11:20, Chunming Zhou wrote:
> It is normal that binary syncobj replaces the underlying fence.
>
> Signed-off-by: Chunming Zhou <david1.zhou@amd.com>
> ---
>   drivers/gpu/drm/drm_syncobj.c | 3 ---
>   1 file changed, 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
> index 929f7c64f9a2..bc7ec1679e4d 100644
> --- a/drivers/gpu/drm/drm_syncobj.c
> +++ b/drivers/gpu/drm/drm_syncobj.c
> @@ -151,9 +151,6 @@ void drm_syncobj_add_point(struct drm_syncobj *syncobj,
>   	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! */
> -	if (prev && prev->seqno >= point)
> -		DRM_ERROR("You are adding an unorder point to timeline!\n");
>   	dma_fence_chain_init(chain, prev, fence, point);
>   	rcu_assign_pointer(syncobj->fence, &chain->base);
>   


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/syncobj: remove boring message

[zhoucm1]

On 2019年07月30日 17:40, Lionel Landwerlin wrote:
> On 30/07/2019 12:36, Daniel Vetter wrote:
>> On Tue, Jul 30, 2019 at 05:31:26PM +0800, zhoucm1 wrote:
>>>
>>> On 2019年07月30日 17:27, Daniel Vetter wrote:
>>>> On Mon, Jul 29, 2019 at 04:20:39PM +0800, Chunming Zhou wrote:
>>>>> It is normal that binary syncobj replaces the underlying fence.
>>>>>
>>>>> Signed-off-by: Chunming Zhou <david1.zhou@amd.com>
>>>> Do we hit this with one of the syncobj igts?
>>> Unforturnately, No, It's only hit in AMDGPU path, which combines 
>>> timeline
>>> and binary to same path when timeline is enabled.
>
>
> We can totally build that case with sw_fences which is what one of the 
> IGT tests does.
OK, Thank you.

-David
>
>
> -Lionel
>
>
>> Looks like lionel has something, maybe help review that?
>> -Daniel
>>
>>> -David
>>>> -Daniel
>>>>
>>>>> ---
>>>>>    drivers/gpu/drm/drm_syncobj.c | 3 ---
>>>>>    1 file changed, 3 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/drm_syncobj.c 
>>>>> b/drivers/gpu/drm/drm_syncobj.c
>>>>> index 929f7c64f9a2..bc7ec1679e4d 100644
>>>>> --- a/drivers/gpu/drm/drm_syncobj.c
>>>>> +++ b/drivers/gpu/drm/drm_syncobj.c
>>>>> @@ -151,9 +151,6 @@ void drm_syncobj_add_point(struct drm_syncobj 
>>>>> *syncobj,
>>>>>        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! */
>>>>> -    if (prev && prev->seqno >= point)
>>>>> -        DRM_ERROR("You are adding an unorder point to timeline!\n");
>>>>>        dma_fence_chain_init(chain, prev, fence, point);
>>>>>        rcu_assign_pointer(syncobj->fence, &chain->base);
>>>>> -- 
>>>>> 2.17.1
>>>>>
>>>>> _______________________________________________
>>>>> dri-devel mailing list
>>>>> dri-devel@lists.freedesktop.org
>>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
>

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/syncobj: remove boring message

[Lionel Landwerlin]

On 30/07/2019 12:36, Daniel Vetter wrote:
> On Tue, Jul 30, 2019 at 05:31:26PM +0800, zhoucm1 wrote:
>>
>> On 2019年07月30日 17:27, Daniel Vetter wrote:
>>> On Mon, Jul 29, 2019 at 04:20:39PM +0800, Chunming Zhou wrote:
>>>> It is normal that binary syncobj replaces the underlying fence.
>>>>
>>>> Signed-off-by: Chunming Zhou <david1.zhou@amd.com>
>>> Do we hit this with one of the syncobj igts?
>> Unforturnately, No, It's only hit in AMDGPU path, which combines timeline
>> and binary to same path when timeline is enabled.


We can totally build that case with sw_fences which is what one of the 
IGT tests does.


-Lionel


> Looks like lionel has something, maybe help review that?
> -Daniel
>
>> -David
>>> -Daniel
>>>
>>>> ---
>>>>    drivers/gpu/drm/drm_syncobj.c | 3 ---
>>>>    1 file changed, 3 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
>>>> index 929f7c64f9a2..bc7ec1679e4d 100644
>>>> --- a/drivers/gpu/drm/drm_syncobj.c
>>>> +++ b/drivers/gpu/drm/drm_syncobj.c
>>>> @@ -151,9 +151,6 @@ void drm_syncobj_add_point(struct drm_syncobj *syncobj,
>>>>    	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! */
>>>> -	if (prev && prev->seqno >= point)
>>>> -		DRM_ERROR("You are adding an unorder point to timeline!\n");
>>>>    	dma_fence_chain_init(chain, prev, fence, point);
>>>>    	rcu_assign_pointer(syncobj->fence, &chain->base);
>>>> -- 
>>>> 2.17.1
>>>>
>>>> _______________________________________________
>>>> dri-devel mailing list
>>>> dri-devel@lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/syncobj: remove boring message

[Daniel Vetter]

On Tue, Jul 30, 2019 at 05:31:26PM +0800, zhoucm1 wrote:
> 
> 
> On 2019年07月30日 17:27, Daniel Vetter wrote:
> > On Mon, Jul 29, 2019 at 04:20:39PM +0800, Chunming Zhou wrote:
> > > It is normal that binary syncobj replaces the underlying fence.
> > > 
> > > Signed-off-by: Chunming Zhou <david1.zhou@amd.com>
> > Do we hit this with one of the syncobj igts?
> Unforturnately, No, It's only hit in AMDGPU path, which combines timeline
> and binary to same path when timeline is enabled.

Looks like lionel has something, maybe help review that?
-Daniel

> 
> -David
> > -Daniel
> > 
> > > ---
> > >   drivers/gpu/drm/drm_syncobj.c | 3 ---
> > >   1 file changed, 3 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
> > > index 929f7c64f9a2..bc7ec1679e4d 100644
> > > --- a/drivers/gpu/drm/drm_syncobj.c
> > > +++ b/drivers/gpu/drm/drm_syncobj.c
> > > @@ -151,9 +151,6 @@ void drm_syncobj_add_point(struct drm_syncobj *syncobj,
> > >   	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! */
> > > -	if (prev && prev->seqno >= point)
> > > -		DRM_ERROR("You are adding an unorder point to timeline!\n");
> > >   	dma_fence_chain_init(chain, prev, fence, point);
> > >   	rcu_assign_pointer(syncobj->fence, &chain->base);
> > > -- 
> > > 2.17.1
> > > 
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/syncobj: remove boring message

[zhoucm1]

On 2019年07月30日 17:27, Daniel Vetter wrote:
> On Mon, Jul 29, 2019 at 04:20:39PM +0800, Chunming Zhou wrote:
>> It is normal that binary syncobj replaces the underlying fence.
>>
>> Signed-off-by: Chunming Zhou <david1.zhou@amd.com>
> Do we hit this with one of the syncobj igts?
Unforturnately, No, It's only hit in AMDGPU path, which combines 
timeline and binary to same path when timeline is enabled.

-David
> -Daniel
>
>> ---
>>   drivers/gpu/drm/drm_syncobj.c | 3 ---
>>   1 file changed, 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
>> index 929f7c64f9a2..bc7ec1679e4d 100644
>> --- a/drivers/gpu/drm/drm_syncobj.c
>> +++ b/drivers/gpu/drm/drm_syncobj.c
>> @@ -151,9 +151,6 @@ void drm_syncobj_add_point(struct drm_syncobj *syncobj,
>>   	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! */
>> -	if (prev && prev->seqno >= point)
>> -		DRM_ERROR("You are adding an unorder point to timeline!\n");
>>   	dma_fence_chain_init(chain, prev, fence, point);
>>   	rcu_assign_pointer(syncobj->fence, &chain->base);
>>   
>> -- 
>> 2.17.1
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/syncobj: remove boring message

[Lionel Landwerlin]

On 30/07/2019 12:27, Daniel Vetter wrote:
> On Mon, Jul 29, 2019 at 04:20:39PM +0800, Chunming Zhou wrote:
>> It is normal that binary syncobj replaces the underlying fence.
>>
>> Signed-off-by: Chunming Zhou <david1.zhou@amd.com>
> Do we hit this with one of the syncobj igts?
> -Daniel


With one of the tests sitting on the mailing list waiting for review, yes.


-Lionel


>
>> ---
>>   drivers/gpu/drm/drm_syncobj.c | 3 ---
>>   1 file changed, 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
>> index 929f7c64f9a2..bc7ec1679e4d 100644
>> --- a/drivers/gpu/drm/drm_syncobj.c
>> +++ b/drivers/gpu/drm/drm_syncobj.c
>> @@ -151,9 +151,6 @@ void drm_syncobj_add_point(struct drm_syncobj *syncobj,
>>   	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! */
>> -	if (prev && prev->seqno >= point)
>> -		DRM_ERROR("You are adding an unorder point to timeline!\n");
>>   	dma_fence_chain_init(chain, prev, fence, point);
>>   	rcu_assign_pointer(syncobj->fence, &chain->base);
>>   
>> -- 
>> 2.17.1
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/syncobj: remove boring message

[Daniel Vetter]

On Mon, Jul 29, 2019 at 04:20:39PM +0800, Chunming Zhou wrote:
> It is normal that binary syncobj replaces the underlying fence.
> 
> Signed-off-by: Chunming Zhou <david1.zhou@amd.com>

Do we hit this with one of the syncobj igts?
-Daniel

> ---
>  drivers/gpu/drm/drm_syncobj.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
> index 929f7c64f9a2..bc7ec1679e4d 100644
> --- a/drivers/gpu/drm/drm_syncobj.c
> +++ b/drivers/gpu/drm/drm_syncobj.c
> @@ -151,9 +151,6 @@ void drm_syncobj_add_point(struct drm_syncobj *syncobj,
>  	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! */
> -	if (prev && prev->seqno >= point)
> -		DRM_ERROR("You are adding an unorder point to timeline!\n");
>  	dma_fence_chain_init(chain, prev, fence, point);
>  	rcu_assign_pointer(syncobj->fence, &chain->base);
>  
> -- 
> 2.17.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm/syncobj: remove boring message

[Chunming Zhou]

It is normal that binary syncobj replaces the underlying fence.

Signed-off-by: Chunming Zhou <david1.zhou@amd.com>
---
 drivers/gpu/drm/drm_syncobj.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
index 929f7c64f9a2..bc7ec1679e4d 100644
--- a/drivers/gpu/drm/drm_syncobj.c
+++ b/drivers/gpu/drm/drm_syncobj.c
@@ -151,9 +151,6 @@ void drm_syncobj_add_point(struct drm_syncobj *syncobj,
 	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! */
-	if (prev && prev->seqno >= point)
-		DRM_ERROR("You are adding an unorder point to timeline!\n");
 	dma_fence_chain_init(chain, prev, fence, point);
 	rcu_assign_pointer(syncobj->fence, &chain->base);
 
-- 
2.17.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel