From: Dmitry Osipenko <dmitry.osipenko@collabora.com> To: Rob Clark <robdclark@gmail.com>, dri-devel@lists.freedesktop.org Cc: freedreno@lists.freedesktop.org, linux-arm-msm@vger.kernel.org, Dmitry Baryshkov <dmitry.baryshkov@linaro.org>, Rob Clark <robdclark@chromium.org>, Sean Paul <sean@poorly.run>, Abhinav Kumar <quic_abhinavk@quicinc.com>, David Airlie <airlied@linux.ie>, Daniel Vetter <daniel@ffwll.ch>, open list <linux-kernel@vger.kernel.org> Subject: Re: [PATCH v2 07/10] drm/msm/gem: Rework vma lookup and pin Date: Thu, 31 Mar 2022 21:28:36 +0300 [thread overview] Message-ID: <9de85a5a-0b61-0b20-d8b2-d412fc081647@collabora.com> (raw) In-Reply-To: <83979c7b-8a8a-5006-6af3-f3ca8b0d8ced@collabora.com> On 3/31/22 21:27, Dmitry Osipenko wrote: > On 3/30/22 23:47, Rob Clark wrote: >> From: Rob Clark <robdclark@chromium.org> >> >> Combines duplicate vma lookup in the get_and_pin path. >> >> Signed-off-by: Rob Clark <robdclark@chromium.org> >> --- >> drivers/gpu/drm/msm/msm_gem.c | 50 ++++++++++++++++++----------------- >> 1 file changed, 26 insertions(+), 24 deletions(-) >> >> diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c >> index deafae6feaa8..218744a490a4 100644 >> --- a/drivers/gpu/drm/msm/msm_gem.c >> +++ b/drivers/gpu/drm/msm/msm_gem.c >> @@ -376,39 +376,40 @@ put_iova_vmas(struct drm_gem_object *obj) >> } >> } >> >> -static int get_iova_locked(struct drm_gem_object *obj, >> - struct msm_gem_address_space *aspace, uint64_t *iova, >> +static struct msm_gem_vma *get_vma_locked(struct drm_gem_object *obj, >> + struct msm_gem_address_space *aspace, >> u64 range_start, u64 range_end) >> { >> struct msm_gem_vma *vma; >> - int ret = 0; >> >> GEM_WARN_ON(!msm_gem_is_locked(obj)); >> >> vma = lookup_vma(obj, aspace); >> >> if (!vma) { >> + int ret; >> + >> vma = add_vma(obj, aspace); >> if (IS_ERR(vma)) >> - return PTR_ERR(vma); >> + return vma; >> >> ret = msm_gem_init_vma(aspace, vma, obj->size, >> range_start, range_end); >> if (ret) { > You're allocation range_start -> range_end *allocating > >> del_vma(vma); >> - return ret; >> + return ERR_PTR(ret); >> } >> + } else { >> + GEM_WARN_ON(vma->iova < range_start); >> + GEM_WARN_ON((vma->iova + obj->size) > range_end); > > and then comparing range_start -> range_start + obj->size, hence you're > assuming that range_end always equals to obj->size during the allocation. > > I'm not sure what is the idea here.. this looks inconsistent. I think > you wanted to write: > > GEM_WARN_ON(vma->iova < range_start); > GEM_WARN_ON(vma->iova + (vma->node.size << PAGE_SHIFT) > range_end); > > But is it really useful to check whether the new range is inside of the > old range? Shouldn't it be always a error to change the IOVA range > without reallocating vma? > > I'd expect to see: > > GEM_WARN_ON(vma->iova != range_start); > GEM_WARN_ON(vma->iova + (vma->node.size << PAGE_SHIFT) != range_end); > > and then error out if range mismatches.
WARNING: multiple messages have this Message-ID (diff)
From: Dmitry Osipenko <dmitry.osipenko@collabora.com> To: Rob Clark <robdclark@gmail.com>, dri-devel@lists.freedesktop.org Cc: Rob Clark <robdclark@chromium.org>, freedreno@lists.freedesktop.org, David Airlie <airlied@linux.ie>, linux-arm-msm@vger.kernel.org, Abhinav Kumar <quic_abhinavk@quicinc.com>, open list <linux-kernel@vger.kernel.org>, Dmitry Baryshkov <dmitry.baryshkov@linaro.org>, Sean Paul <sean@poorly.run> Subject: Re: [PATCH v2 07/10] drm/msm/gem: Rework vma lookup and pin Date: Thu, 31 Mar 2022 21:28:36 +0300 [thread overview] Message-ID: <9de85a5a-0b61-0b20-d8b2-d412fc081647@collabora.com> (raw) In-Reply-To: <83979c7b-8a8a-5006-6af3-f3ca8b0d8ced@collabora.com> On 3/31/22 21:27, Dmitry Osipenko wrote: > On 3/30/22 23:47, Rob Clark wrote: >> From: Rob Clark <robdclark@chromium.org> >> >> Combines duplicate vma lookup in the get_and_pin path. >> >> Signed-off-by: Rob Clark <robdclark@chromium.org> >> --- >> drivers/gpu/drm/msm/msm_gem.c | 50 ++++++++++++++++++----------------- >> 1 file changed, 26 insertions(+), 24 deletions(-) >> >> diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c >> index deafae6feaa8..218744a490a4 100644 >> --- a/drivers/gpu/drm/msm/msm_gem.c >> +++ b/drivers/gpu/drm/msm/msm_gem.c >> @@ -376,39 +376,40 @@ put_iova_vmas(struct drm_gem_object *obj) >> } >> } >> >> -static int get_iova_locked(struct drm_gem_object *obj, >> - struct msm_gem_address_space *aspace, uint64_t *iova, >> +static struct msm_gem_vma *get_vma_locked(struct drm_gem_object *obj, >> + struct msm_gem_address_space *aspace, >> u64 range_start, u64 range_end) >> { >> struct msm_gem_vma *vma; >> - int ret = 0; >> >> GEM_WARN_ON(!msm_gem_is_locked(obj)); >> >> vma = lookup_vma(obj, aspace); >> >> if (!vma) { >> + int ret; >> + >> vma = add_vma(obj, aspace); >> if (IS_ERR(vma)) >> - return PTR_ERR(vma); >> + return vma; >> >> ret = msm_gem_init_vma(aspace, vma, obj->size, >> range_start, range_end); >> if (ret) { > You're allocation range_start -> range_end *allocating > >> del_vma(vma); >> - return ret; >> + return ERR_PTR(ret); >> } >> + } else { >> + GEM_WARN_ON(vma->iova < range_start); >> + GEM_WARN_ON((vma->iova + obj->size) > range_end); > > and then comparing range_start -> range_start + obj->size, hence you're > assuming that range_end always equals to obj->size during the allocation. > > I'm not sure what is the idea here.. this looks inconsistent. I think > you wanted to write: > > GEM_WARN_ON(vma->iova < range_start); > GEM_WARN_ON(vma->iova + (vma->node.size << PAGE_SHIFT) > range_end); > > But is it really useful to check whether the new range is inside of the > old range? Shouldn't it be always a error to change the IOVA range > without reallocating vma? > > I'd expect to see: > > GEM_WARN_ON(vma->iova != range_start); > GEM_WARN_ON(vma->iova + (vma->node.size << PAGE_SHIFT) != range_end); > > and then error out if range mismatches.
next prev parent reply other threads:[~2022-03-31 18:28 UTC|newest] Thread overview: 40+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-03-30 20:47 [PATCH v2 00/10] drm/msm: Userspace allocated GPU addresses Rob Clark 2022-03-30 20:47 ` Rob Clark 2022-03-30 20:47 ` [PATCH v2 01/10] drm/msm/gem: Move prototypes Rob Clark 2022-03-30 20:47 ` Rob Clark 2022-03-30 20:47 ` [PATCH v2 02/10] drm/msm/gpu: Drop duplicate fence counter Rob Clark 2022-03-30 20:47 ` Rob Clark 2022-03-30 20:47 ` [PATCH v2 03/10] drm/msm/gem: Convert some missed GEM_WARN_ON()s Rob Clark 2022-03-30 20:47 ` Rob Clark 2022-03-30 20:47 ` [PATCH v2 04/10] drm/msm/gem: Split out inuse helper Rob Clark 2022-03-30 20:47 ` Rob Clark 2022-03-30 20:47 ` [PATCH v2 05/10] drm/msm/gem: Drop PAGE_SHIFT for address space mm Rob Clark 2022-03-30 20:47 ` Rob Clark 2022-03-30 20:47 ` [PATCH v2 06/10] drm/msm: Drop msm_gem_iova() Rob Clark 2022-03-30 20:47 ` Rob Clark 2022-03-30 20:47 ` [PATCH v2 07/10] drm/msm/gem: Rework vma lookup and pin Rob Clark 2022-03-30 20:47 ` Rob Clark 2022-03-31 18:27 ` Dmitry Osipenko 2022-03-31 18:27 ` Dmitry Osipenko 2022-03-31 18:28 ` Dmitry Osipenko [this message] 2022-03-31 18:28 ` Dmitry Osipenko 2022-03-31 18:58 ` Rob Clark 2022-03-31 18:58 ` Rob Clark 2022-03-31 20:49 ` Dmitry Osipenko 2022-03-31 20:49 ` Dmitry Osipenko 2022-03-30 20:47 ` [PATCH v2 08/10] drm/msm/gem: Split " Rob Clark 2022-03-30 20:47 ` Rob Clark 2022-03-30 20:47 ` [PATCH v2 09/10] drm/msm/gem: Add fenced vma unpin Rob Clark 2022-03-30 20:47 ` Rob Clark 2022-03-30 20:47 ` [PATCH v2 10/10] drm/msm: Add a way for userspace to allocate GPU iova Rob Clark 2022-03-30 20:47 ` Rob Clark 2022-03-31 18:52 ` Dmitry Osipenko 2022-03-31 18:52 ` Dmitry Osipenko 2022-03-31 18:53 ` Dmitry Osipenko 2022-03-31 18:53 ` Dmitry Osipenko 2022-03-31 19:02 ` Rob Clark 2022-03-31 19:02 ` Rob Clark 2022-03-31 19:41 ` Dmitry Osipenko 2022-03-31 19:41 ` Dmitry Osipenko 2022-03-31 19:54 ` Rob Clark 2022-03-31 19:54 ` Rob Clark
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=9de85a5a-0b61-0b20-d8b2-d412fc081647@collabora.com \ --to=dmitry.osipenko@collabora.com \ --cc=airlied@linux.ie \ --cc=daniel@ffwll.ch \ --cc=dmitry.baryshkov@linaro.org \ --cc=dri-devel@lists.freedesktop.org \ --cc=freedreno@lists.freedesktop.org \ --cc=linux-arm-msm@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=quic_abhinavk@quicinc.com \ --cc=robdclark@chromium.org \ --cc=robdclark@gmail.com \ --cc=sean@poorly.run \ /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.