[PATCH 1/2] drm/scheduler: fix drm_sched_job_add_implicit_dependencies

[PATCH 1/2] drm/scheduler: fix drm_sched_job_add_implicit_dependencies

[Christian König]

Trivial fix since we now need to grab a reference to the fence we have
added. Previously the dma_resv function where doing that for us.

Signed-off-by: Christian König <christian.koenig@amd.com>
Fixes: 9c2ba265352a ("drm/scheduler: use new iterator in drm_sched_job_add_implicit_dependencies v2")
Link: https://patchwork.freedesktop.org/patch/msgid/20211019112706.27769-1-christian.koenig@amd.com
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Reported-by: Nicolas Frattaroli <frattaroli.nicolas@gmail.com>
References: https://lore.kernel.org/dri-devel/2023306.UmlnhvANQh@archbook/
Tested-by: Nicolas Frattaroli <frattaroli.nicolas@gmail.com>
Tested-by: Yassine Oudjana <y.oudjana@protonmail.com>
---
 drivers/gpu/drm/scheduler/sched_main.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
index 5bc5f775abe1..94fe51b3caa2 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -707,6 +707,9 @@ int drm_sched_job_add_implicit_dependencies(struct drm_sched_job *job,
 		ret = drm_sched_job_add_dependency(job, fence);
 		if (ret)
 			return ret;
+
+		/* Make sure to grab an additional ref on the added fence */
+		dma_fence_get(fence);
 	}
 	return 0;
 }
-- 
2.25.1

[PATCH 2/2] drm/sched: fix dropping the last fence ref

[Christian König]

We need to grab another ref before trying to add the fence to the sched
job and not after.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/scheduler/sched_main.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
index 94fe51b3caa2..400d201c3c28 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -704,12 +704,14 @@ int drm_sched_job_add_implicit_dependencies(struct drm_sched_job *job,
 	int ret;
 
 	dma_resv_for_each_fence(&cursor, obj->resv, write, fence) {
-		ret = drm_sched_job_add_dependency(job, fence);
-		if (ret)
-			return ret;
-
 		/* Make sure to grab an additional ref on the added fence */
 		dma_fence_get(fence);
+
+		ret = drm_sched_job_add_dependency(job, fence);
+		if (ret) {
+			dma_fence_put(fence);
+			return ret;
+		}
 	}
 	return 0;
 }
-- 
2.25.1

Re: [PATCH 2/2] drm/sched: fix dropping the last fence ref

[Daniel Vetter]

On Tue, Nov 16, 2021 at 10:25:19AM +0100, Christian König wrote:
> We need to grab another ref before trying to add the fence to the sched
> job and not after.
> 
> Signed-off-by: Christian König <christian.koenig@amd.com>

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

I wondered first why this goes boom, but then I realized that in some
cases add_dependency() drops the reference of the passed-in fence.

Please also add the Fixes: line like in the previous patch.

Cheers, Daniel

> ---
>  drivers/gpu/drm/scheduler/sched_main.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
> index 94fe51b3caa2..400d201c3c28 100644
> --- a/drivers/gpu/drm/scheduler/sched_main.c
> +++ b/drivers/gpu/drm/scheduler/sched_main.c
> @@ -704,12 +704,14 @@ int drm_sched_job_add_implicit_dependencies(struct drm_sched_job *job,
>  	int ret;
>  
>  	dma_resv_for_each_fence(&cursor, obj->resv, write, fence) {
> -		ret = drm_sched_job_add_dependency(job, fence);
> -		if (ret)
> -			return ret;
> -
>  		/* Make sure to grab an additional ref on the added fence */
>  		dma_fence_get(fence);
> +
> +		ret = drm_sched_job_add_dependency(job, fence);
> +		if (ret) {
> +			dma_fence_put(fence);
> +			return ret;
> +		}
>  	}
>  	return 0;
>  }
> -- 
> 2.25.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 2/2] drm/sched: fix dropping the last fence ref

[Rob Clark]

On Tue, Nov 16, 2021 at 8:37 AM Daniel Vetter <daniel@ffwll.ch> wrote:
>
> On Tue, Nov 16, 2021 at 10:25:19AM +0100, Christian König wrote:
> > We need to grab another ref before trying to add the fence to the sched
> > job and not after.
> >
> > Signed-off-by: Christian König <christian.koenig@amd.com>
>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>
> I wondered first why this goes boom, but then I realized that in some
> cases add_dependency() drops the reference of the passed-in fence.
>
> Please also add the Fixes: line like in the previous patch.

oh, I sent https://patchwork.freedesktop.org/patch/463329/ before I
saw this.. it already has the fixes tag, and IMO a better description,
so I think you can just pick that one instead

BR,
-R

> Cheers, Daniel
>
> > ---
> >  drivers/gpu/drm/scheduler/sched_main.c | 10 ++++++----
> >  1 file changed, 6 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
> > index 94fe51b3caa2..400d201c3c28 100644
> > --- a/drivers/gpu/drm/scheduler/sched_main.c
> > +++ b/drivers/gpu/drm/scheduler/sched_main.c
> > @@ -704,12 +704,14 @@ int drm_sched_job_add_implicit_dependencies(struct drm_sched_job *job,
> >       int ret;
> >
> >       dma_resv_for_each_fence(&cursor, obj->resv, write, fence) {
> > -             ret = drm_sched_job_add_dependency(job, fence);
> > -             if (ret)
> > -                     return ret;
> > -
> >               /* Make sure to grab an additional ref on the added fence */
> >               dma_fence_get(fence);
> > +
> > +             ret = drm_sched_job_add_dependency(job, fence);
> > +             if (ret) {
> > +                     dma_fence_put(fence);
> > +                     return ret;
> > +             }
> >       }
> >       return 0;
> >  }
> > --
> > 2.25.1
> >
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

Re: [PATCH 2/2] drm/sched: fix dropping the last fence ref

[Christian König]

Am 16.11.21 um 19:07 schrieb Rob Clark:
> On Tue, Nov 16, 2021 at 8:37 AM Daniel Vetter <daniel@ffwll.ch> wrote:
>> On Tue, Nov 16, 2021 at 10:25:19AM +0100, Christian König wrote:
>>> We need to grab another ref before trying to add the fence to the sched
>>> job and not after.
>>>
>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>
>> I wondered first why this goes boom, but then I realized that in some
>> cases add_dependency() drops the reference of the passed-in fence.
>>
>> Please also add the Fixes: line like in the previous patch.
> oh, I sent https://patchwork.freedesktop.org/patch/463329/ before I
> saw this.. it already has the fixes tag, and IMO a better description,
> so I think you can just pick that one instead

Yeah, agree. You also have the missing Fixes line already.

Going to add Daniels rb to your patch as well since it is technically 
the same.

Thanks,
Christian.

>
> BR,
> -R
>
>> Cheers, Daniel
>>
>>> ---
>>>   drivers/gpu/drm/scheduler/sched_main.c | 10 ++++++----
>>>   1 file changed, 6 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
>>> index 94fe51b3caa2..400d201c3c28 100644
>>> --- a/drivers/gpu/drm/scheduler/sched_main.c
>>> +++ b/drivers/gpu/drm/scheduler/sched_main.c
>>> @@ -704,12 +704,14 @@ int drm_sched_job_add_implicit_dependencies(struct drm_sched_job *job,
>>>        int ret;
>>>
>>>        dma_resv_for_each_fence(&cursor, obj->resv, write, fence) {
>>> -             ret = drm_sched_job_add_dependency(job, fence);
>>> -             if (ret)
>>> -                     return ret;
>>> -
>>>                /* Make sure to grab an additional ref on the added fence */
>>>                dma_fence_get(fence);
>>> +
>>> +             ret = drm_sched_job_add_dependency(job, fence);
>>> +             if (ret) {
>>> +                     dma_fence_put(fence);
>>> +                     return ret;
>>> +             }
>>>        }
>>>        return 0;
>>>   }
>>> --
>>> 2.25.1
>>>
>> --
>> Daniel Vetter
>> Software Engineer, Intel Corporation
>> http://blog.ffwll.ch