From: Chunyou Tang <tangchunyou@163.com> To: Steven Price <steven.price@arm.com> Cc: robh@kernel.org, tomeu.vizoso@collabora.com, alyssa.rosenzweig@collabora.com, airlied@linux.ie, daniel@ffwll.ch, ChunyouTang <tangchunyou@icubecorp.cn>, linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org Subject: Re: [PATCH] drm/panfrost:modify 'break' to 'continue' to traverse the circulation Date: Sat, 19 Jun 2021 11:09:23 +0800 [thread overview] Message-ID: <20210619110923.00001c64@163.com> (raw) In-Reply-To: <4d289eed-59f2-161a-40d1-2a434a1955c2@arm.com> [-- 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]); > >
WARNING: multiple messages have this Message-ID (diff)
From: Chunyou Tang <tangchunyou@163.com> To: Steven Price <steven.price@arm.com> Cc: tomeu.vizoso@collabora.com, airlied@linux.ie, linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, alyssa.rosenzweig@collabora.com, ChunyouTang <tangchunyou@icubecorp.cn> Subject: Re: [PATCH] drm/panfrost:modify 'break' to 'continue' to traverse the circulation Date: Sat, 19 Jun 2021 11:09:23 +0800 [thread overview] Message-ID: <20210619110923.00001c64@163.com> (raw) In-Reply-To: <4d289eed-59f2-161a-40d1-2a434a1955c2@arm.com> [-- 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]); > >
next prev parent reply other threads:[~2021-06-19 3:13 UTC|newest] Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-06-17 8:04 [PATCH] drm/panfrost:modify 'break' to 'continue' to traverse the circulation ChunyouTang 2021-06-17 8:04 ` ChunyouTang 2021-06-18 12:43 ` Steven Price 2021-06-19 3:09 ` Chunyou Tang [this message] 2021-06-19 3:09 ` Chunyou Tang 2021-06-21 10:45 ` Steven Price 2021-06-22 1:31 ` Chunyou Tang
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20210619110923.00001c64@163.com \ --to=tangchunyou@163.com \ --cc=airlied@linux.ie \ --cc=alyssa.rosenzweig@collabora.com \ --cc=daniel@ffwll.ch \ --cc=dri-devel@lists.freedesktop.org \ --cc=linux-kernel@vger.kernel.org \ --cc=robh@kernel.org \ --cc=steven.price@arm.com \ --cc=tangchunyou@icubecorp.cn \ --cc=tomeu.vizoso@collabora.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.