linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] drm/panfrost:modify 'break' to 'continue' to traverse the circulation
@ 2021-06-17  8:04 ChunyouTang
  2021-06-18 12:43 ` Steven Price
  0 siblings, 1 reply; 5+ messages in thread
From: ChunyouTang @ 2021-06-17  8:04 UTC (permalink / raw)
  To: robh, tomeu.vizoso, steven.price, alyssa.rosenzweig, airlied, daniel
  Cc: dri-devel, linux-kernel, ChunyouTang

From: ChunyouTang <tangchunyou@icubecorp.cn>

The 'break' can cause 'Memory manager not clean during takedown'

It cannot use break to finish the circulation,it should use

continue to traverse the circulation.it should put every mapping

which is not NULL.

Signed-off-by: ChunyouTang <tangchunyou@icubecorp.cn>
---
 drivers/gpu/drm/panfrost/panfrost_job.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
index 6003cfeb1322..52bccc1d2d42 100644
--- a/drivers/gpu/drm/panfrost/panfrost_job.c
+++ b/drivers/gpu/drm/panfrost/panfrost_job.c
@@ -281,7 +281,7 @@ static void panfrost_job_cleanup(struct kref *ref)
 	if (job->mappings) {
 		for (i = 0; i < job->bo_count; i++) {
 			if (!job->mappings[i])
-				break;
+				continue;
 
 			atomic_dec(&job->mappings[i]->obj->gpu_usecount);
 			panfrost_gem_mapping_put(job->mappings[i]);
-- 
2.25.1



^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] drm/panfrost:modify 'break' to 'continue' to traverse the circulation
  2021-06-17  8:04 [PATCH] drm/panfrost:modify 'break' to 'continue' to traverse the circulation ChunyouTang
@ 2021-06-18 12:43 ` Steven Price
  2021-06-19  3:09   ` Chunyou Tang
  0 siblings, 1 reply; 5+ messages in thread
From: Steven Price @ 2021-06-18 12:43 UTC (permalink / raw)
  To: ChunyouTang, robh, tomeu.vizoso, alyssa.rosenzweig, airlied, daniel
  Cc: ChunyouTang, linux-kernel, dri-devel

On 17/06/2021 09:04, ChunyouTang wrote:
> From: ChunyouTang <tangchunyou@icubecorp.cn>
> 
> The 'break' can cause 'Memory manager not clean during takedown'
> 
> It cannot use break to finish the circulation,it should use
> 
> continue to traverse the circulation.it should put every mapping
> 
> which is not NULL.

You don't appear to have answered my question about whether you've
actually seen this happen (and ideally what circumstances). In my
previous email[1] I explained why I don't think this is needed. You need
to convince me that I've overlooked something.

Thanks,

Steve

[1] https://lore.kernel.org/r/31644881-134a-2d6e-dddf-e658a3a8176b%40arm.com

> Signed-off-by: ChunyouTang <tangchunyou@icubecorp.cn>
> ---
>  drivers/gpu/drm/panfrost/panfrost_job.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
> index 6003cfeb1322..52bccc1d2d42 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_job.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_job.c
> @@ -281,7 +281,7 @@ static void panfrost_job_cleanup(struct kref *ref)
>  	if (job->mappings) {
>  		for (i = 0; i < job->bo_count; i++) {
>  			if (!job->mappings[i])
> -				break;
> +				continue;
>  
>  			atomic_dec(&job->mappings[i]->obj->gpu_usecount);
>  			panfrost_gem_mapping_put(job->mappings[i]);
> 


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] drm/panfrost:modify 'break' to 'continue' to traverse the circulation
  2021-06-18 12:43 ` Steven Price
@ 2021-06-19  3:09   ` Chunyou Tang
  2021-06-21 10:45     ` Steven Price
  0 siblings, 1 reply; 5+ messages in thread
From: Chunyou Tang @ 2021-06-19  3:09 UTC (permalink / raw)
  To: Steven Price
  Cc: robh, tomeu.vizoso, alyssa.rosenzweig, airlied, daniel,
	ChunyouTang, linux-kernel, dri-devel

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=GB18030, Size: 3368 bytes --]

Hi Steve,
	1,
from
https://lore.kernel.org/lkml/31644881-134a-2d6e-dddf-e658a3a8176b@arm.com/
I can see what your sent,I used a wrong email address,Now it correct.
	2,
> >Unless I'm mistaken the situation where some mappings may be NULL is
> >caused by the loop in panfrost_lookup_bos() not completing
> >successfully
> >(panfrost_gem_mapping_get() returning NULL). In this case if
> >mappings[i]
> >is NULL then all following mappings must also be NULL. So 'break'
> >allows
> >us to skip the later ones. Admittedly the performance here isn't
> >important so I'm not sure it's worth the optimisation, but AIUI this
> >code isn't actually wrong.

from panfrost_lookup_bos(),you can see:
        for (i = 0; i < job->bo_count; i++) {
                struct panfrost_gem_mapping *mapping;

                bo = to_panfrost_bo(job->bos[i]);
                ICUBE_DEBUG_PRINTK("panfrost bo gem handle=0x%x
                is_dumb=%d\n", bo->gem_handle, bo->is_dumb);
                if (!bo->is_dumb) {
                       mapping = panfrost_gem_mapping_get(bo, priv);
                       if (!mapping) {
                                ret = -EINVAL;
                                break;
                       }

                        atomic_inc(&bo->gpu_usecount);
                        job->mappings[i] = mapping;
                } else {
                        atomic_inc(&bo->gpu_usecount);
                        job->mappings[i] = NULL;
                }
        }
if bo->is_dumb is TRUE,the job->mappings[i] will set to NULL,and the
while will be continue,so if job->mappings[i] is NULL,the following
can not be NULL.

	3,
I've had this problem in our project,the value of is_dumb like these:
0
0
0
1
0
0
0
so,when job->mappings[i] is NULL,we can not break the while in 
panfrost_job_cleanup().

thanks
Chunyou

ÓÚ Fri, 18 Jun 2021 13:43:25 +0100
Steven Price <steven.price@arm.com> дµÀ:

> On 17/06/2021 09:04, ChunyouTang wrote:
> > From: ChunyouTang <tangchunyou@icubecorp.cn>
> > 
> > The 'break' can cause 'Memory manager not clean during takedown'
> > 
> > It cannot use break to finish the circulation,it should use
> > 
> > continue to traverse the circulation.it should put every mapping
> > 
> > which is not NULL.
> 
> You don't appear to have answered my question about whether you've
> actually seen this happen (and ideally what circumstances). In my
> previous email[1] I explained why I don't think this is needed. You
> need to convince me that I've overlooked something.
> 
> Thanks,
> 
> Steve
> 
> [1]
> https://lore.kernel.org/r/31644881-134a-2d6e-dddf-e658a3a8176b%40arm.com
> 
> > Signed-off-by: ChunyouTang <tangchunyou@icubecorp.cn>
> > ---
> >  drivers/gpu/drm/panfrost/panfrost_job.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c
> > b/drivers/gpu/drm/panfrost/panfrost_job.c index
> > 6003cfeb1322..52bccc1d2d42 100644 ---
> > a/drivers/gpu/drm/panfrost/panfrost_job.c +++
> > b/drivers/gpu/drm/panfrost/panfrost_job.c @@ -281,7 +281,7 @@
> > static void panfrost_job_cleanup(struct kref *ref) if
> > (job->mappings) { for (i = 0; i < job->bo_count; i++) {
> >  			if (!job->mappings[i])
> > -				break;
> > +				continue;
> >  
> >  			atomic_dec(&job->mappings[i]->obj->gpu_usecount);
> >  			panfrost_gem_mapping_put(job->mappings[i]);
> > 



^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] drm/panfrost:modify 'break' to 'continue' to traverse the circulation
  2021-06-19  3:09   ` Chunyou Tang
@ 2021-06-21 10:45     ` Steven Price
  2021-06-22  1:31       ` Chunyou Tang
  0 siblings, 1 reply; 5+ messages in thread
From: Steven Price @ 2021-06-21 10:45 UTC (permalink / raw)
  To: Chunyou Tang
  Cc: tomeu.vizoso, airlied, linux-kernel, dri-devel,
	alyssa.rosenzweig, ChunyouTang

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=gb18030, Size: 3886 bytes --]

On 19/06/2021 04:09, Chunyou Tang wrote:
> Hi Steve,
> 	1,
> from
> https://lore.kernel.org/lkml/31644881-134a-2d6e-dddf-e658a3a8176b@arm.com/
> I can see what your sent,I used a wrong email address,Now it correct.
> 	2,
>>> Unless I'm mistaken the situation where some mappings may be NULL is
>>> caused by the loop in panfrost_lookup_bos() not completing
>>> successfully
>>> (panfrost_gem_mapping_get() returning NULL). In this case if
>>> mappings[i]
>>> is NULL then all following mappings must also be NULL. So 'break'
>>> allows
>>> us to skip the later ones. Admittedly the performance here isn't
>>> important so I'm not sure it's worth the optimisation, but AIUI this
>>> code isn't actually wrong.
> 
> from panfrost_lookup_bos(),you can see:
>         for (i = 0; i < job->bo_count; i++) {
>                 struct panfrost_gem_mapping *mapping;
> 
>                 bo = to_panfrost_bo(job->bos[i]);
>                 ICUBE_DEBUG_PRINTK("panfrost bo gem handle=0x%x
>                 is_dumb=%d\n", bo->gem_handle, bo->is_dumb);
>                 if (!bo->is_dumb) {
>                        mapping = panfrost_gem_mapping_get(bo, priv);
>                        if (!mapping) {
>                                 ret = -EINVAL;
>                                 break;
>                        }
> 
>                         atomic_inc(&bo->gpu_usecount);
>                         job->mappings[i] = mapping;
>                 } else {
>                         atomic_inc(&bo->gpu_usecount);
>                         job->mappings[i] = NULL;
>                 }
>         }

This code isn't upstream - in drm-misc/drm-misc-next (and all mainline
kernels from what I can tell) this doesn't have any "is_dumb" test.
Which branch are you using?

> if bo->is_dumb is TRUE,the job->mappings[i] will set to NULL,and the
> while will be continue,so if job->mappings[i] is NULL,the following
> can not be NULL.

I agree that with the above code the panfrost_job_cleanup() would need
changing. But we don't (currently) have this code upstream, so this
change doesn't make sense upstream.

Thanks,

Steve

> 	3,
> I've had this problem in our project,the value of is_dumb like these:
> 0
> 0
> 0
> 1
> 0
> 0
> 0
> so,when job->mappings[i] is NULL,we can not break the while in 
> panfrost_job_cleanup().
> 
> thanks
> Chunyou
> 
> ÓÚ Fri, 18 Jun 2021 13:43:25 +0100
> Steven Price <steven.price@arm.com> дµÀ:
> 
>> On 17/06/2021 09:04, ChunyouTang wrote:
>>> From: ChunyouTang <tangchunyou@icubecorp.cn>
>>>
>>> The 'break' can cause 'Memory manager not clean during takedown'
>>>
>>> It cannot use break to finish the circulation,it should use
>>>
>>> continue to traverse the circulation.it should put every mapping
>>>
>>> which is not NULL.
>>
>> You don't appear to have answered my question about whether you've
>> actually seen this happen (and ideally what circumstances). In my
>> previous email[1] I explained why I don't think this is needed. You
>> need to convince me that I've overlooked something.
>>
>> Thanks,
>>
>> Steve
>>
>> [1]
>> https://lore.kernel.org/r/31644881-134a-2d6e-dddf-e658a3a8176b%40arm.com
>>
>>> Signed-off-by: ChunyouTang <tangchunyou@icubecorp.cn>
>>> ---
>>>  drivers/gpu/drm/panfrost/panfrost_job.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c
>>> b/drivers/gpu/drm/panfrost/panfrost_job.c index
>>> 6003cfeb1322..52bccc1d2d42 100644 ---
>>> a/drivers/gpu/drm/panfrost/panfrost_job.c +++
>>> b/drivers/gpu/drm/panfrost/panfrost_job.c @@ -281,7 +281,7 @@
>>> static void panfrost_job_cleanup(struct kref *ref) if
>>> (job->mappings) { for (i = 0; i < job->bo_count; i++) {
>>>  			if (!job->mappings[i])
>>> -				break;
>>> +				continue;
>>>  
>>>  			atomic_dec(&job->mappings[i]->obj->gpu_usecount);
>>>  			panfrost_gem_mapping_put(job->mappings[i]);
>>>
> 
> 


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] drm/panfrost:modify 'break' to 'continue' to traverse the circulation
  2021-06-21 10:45     ` Steven Price
@ 2021-06-22  1:31       ` Chunyou Tang
  0 siblings, 0 replies; 5+ messages in thread
From: Chunyou Tang @ 2021-06-22  1:31 UTC (permalink / raw)
  To: Steven Price
  Cc: tomeu.vizoso, airlied, linux-kernel, dri-devel,
	alyssa.rosenzweig, ChunyouTang

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=GB18030, Size: 4305 bytes --]

Hi Steve,
	I make a mistake about the code branch,I will test it later,
thinks for your reply.

Chunyou

ÓÚ Mon, 21 Jun 2021 11:45:18 +0100
Steven Price <steven.price@arm.com> дµÀ:

> On 19/06/2021 04:09, Chunyou Tang wrote:
> > Hi Steve,
> > 	1,
> > from
> > https://lore.kernel.org/lkml/31644881-134a-2d6e-dddf-e658a3a8176b@arm.com/
> > I can see what your sent,I used a wrong email address,Now it
> > correct. 2,
> >>> Unless I'm mistaken the situation where some mappings may be NULL
> >>> is caused by the loop in panfrost_lookup_bos() not completing
> >>> successfully
> >>> (panfrost_gem_mapping_get() returning NULL). In this case if
> >>> mappings[i]
> >>> is NULL then all following mappings must also be NULL. So 'break'
> >>> allows
> >>> us to skip the later ones. Admittedly the performance here isn't
> >>> important so I'm not sure it's worth the optimisation, but AIUI
> >>> this code isn't actually wrong.
> > 
> > from panfrost_lookup_bos(),you can see:
> >         for (i = 0; i < job->bo_count; i++) {
> >                 struct panfrost_gem_mapping *mapping;
> > 
> >                 bo = to_panfrost_bo(job->bos[i]);
> >                 ICUBE_DEBUG_PRINTK("panfrost bo gem handle=0x%x
> >                 is_dumb=%d\n", bo->gem_handle, bo->is_dumb);
> >                 if (!bo->is_dumb) {
> >                        mapping = panfrost_gem_mapping_get(bo, priv);
> >                        if (!mapping) {
> >                                 ret = -EINVAL;
> >                                 break;
> >                        }
> > 
> >                         atomic_inc(&bo->gpu_usecount);
> >                         job->mappings[i] = mapping;
> >                 } else {
> >                         atomic_inc(&bo->gpu_usecount);
> >                         job->mappings[i] = NULL;
> >                 }
> >         }
> 
> This code isn't upstream - in drm-misc/drm-misc-next (and all mainline
> kernels from what I can tell) this doesn't have any "is_dumb" test.
> Which branch are you using?
> 
> > if bo->is_dumb is TRUE,the job->mappings[i] will set to NULL,and the
> > while will be continue,so if job->mappings[i] is NULL,the following
> > can not be NULL.
> 
> I agree that with the above code the panfrost_job_cleanup() would need
> changing. But we don't (currently) have this code upstream, so this
> change doesn't make sense upstream.
> 
> Thanks,
> 
> Steve
> 
> > 	3,
> > I've had this problem in our project,the value of is_dumb like
> > these: 0
> > 0
> > 0
> > 1
> > 0
> > 0
> > 0
> > so,when job->mappings[i] is NULL,we can not break the while in 
> > panfrost_job_cleanup().
> > 
> > thanks
> > Chunyou
> > 
> > ÓÚ Fri, 18 Jun 2021 13:43:25 +0100
> > Steven Price <steven.price@arm.com> дµÀ:
> > 
> >> On 17/06/2021 09:04, ChunyouTang wrote:
> >>> From: ChunyouTang <tangchunyou@icubecorp.cn>
> >>>
> >>> The 'break' can cause 'Memory manager not clean during takedown'
> >>>
> >>> It cannot use break to finish the circulation,it should use
> >>>
> >>> continue to traverse the circulation.it should put every mapping
> >>>
> >>> which is not NULL.
> >>
> >> You don't appear to have answered my question about whether you've
> >> actually seen this happen (and ideally what circumstances). In my
> >> previous email[1] I explained why I don't think this is needed. You
> >> need to convince me that I've overlooked something.
> >>
> >> Thanks,
> >>
> >> Steve
> >>
> >> [1]
> >> https://lore.kernel.org/r/31644881-134a-2d6e-dddf-e658a3a8176b%40arm.com
> >>
> >>> Signed-off-by: ChunyouTang <tangchunyou@icubecorp.cn>
> >>> ---
> >>>  drivers/gpu/drm/panfrost/panfrost_job.c | 2 +-
> >>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c
> >>> b/drivers/gpu/drm/panfrost/panfrost_job.c index
> >>> 6003cfeb1322..52bccc1d2d42 100644 ---
> >>> a/drivers/gpu/drm/panfrost/panfrost_job.c +++
> >>> b/drivers/gpu/drm/panfrost/panfrost_job.c @@ -281,7 +281,7 @@
> >>> static void panfrost_job_cleanup(struct kref *ref) if
> >>> (job->mappings) { for (i = 0; i < job->bo_count; i++) {
> >>>  			if (!job->mappings[i])
> >>> -				break;
> >>> +				continue;
> >>>  
> >>>  			atomic_dec(&job->mappings[i]->obj->gpu_usecount);
> >>>  			panfrost_gem_mapping_put(job->mappings[i]);
> >>>
> > 
> > 



^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2021-06-22  1:47 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-17  8:04 [PATCH] drm/panfrost:modify 'break' to 'continue' to traverse the circulation ChunyouTang
2021-06-18 12:43 ` Steven Price
2021-06-19  3:09   ` Chunyou Tang
2021-06-21 10:45     ` Steven Price
2021-06-22  1:31       ` Chunyou Tang

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).