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]);