Re: [PATCH v2 07/10] drm/msm/gem: Rework vma lookup and pin
On 3/31/22 21:58, Rob Clark wrote:
> On Thu, Mar 31, 2022 at 11:27 AM Dmitry Osipenko
> wrote:
>>
>> On 3/30/22 23:47, Rob Clark wrote:
>>> From: Rob Clark
>>>
>>> Combines duplicate vma lookup in the get_and_pin path.
>>>
>>> Signed-off-by: Rob Clark
>>> ---
>>> 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
>>
>>
>>> 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?
>
> There are a few cases (for allocations for GMU) where the range is
> larger than the bo.. see a6xx_gmu_memory_alloc()
Ahh, I didn't read the code properly and see now why you're using the
obj->size. It's the range where you want to put the BO. Looks good then.
Reviewed-by: Dmitry Osipenko
Re: [PATCH v2 07/10] drm/msm/gem: Rework vma lookup and pin
On Thu, Mar 31, 2022 at 11:27 AM Dmitry Osipenko
wrote:
>
> On 3/30/22 23:47, Rob Clark wrote:
> > From: Rob Clark
> >
> > Combines duplicate vma lookup in the get_and_pin path.
> >
> > Signed-off-by: Rob Clark
> > ---
> > 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
>
>
> > 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?
There are a few cases (for allocations for GMU) where the range is
larger than the bo.. see a6xx_gmu_memory_alloc()
BR,
-R
>
> 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.
Re: [PATCH v2 07/10] drm/msm/gem: Rework vma lookup and pin
On 3/31/22 21:27, Dmitry Osipenko wrote:
> On 3/30/22 23:47, Rob Clark wrote:
>> From: Rob Clark
>>
>> Combines duplicate vma lookup in the get_and_pin path.
>>
>> Signed-off-by: Rob Clark
>> ---
>> 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.
Re: [PATCH v2 07/10] drm/msm/gem: Rework vma lookup and pin
On 3/30/22 23:47, Rob Clark wrote:
> From: Rob Clark
>
> Combines duplicate vma lookup in the get_and_pin path.
>
> Signed-off-by: Rob Clark
> ---
> 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
> 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.
[PATCH v2 07/10] drm/msm/gem: Rework vma lookup and pin
From: Rob Clark
Combines duplicate vma lookup in the get_and_pin path.
Signed-off-by: Rob Clark
---
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) {
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);
}
- *iova = vma->iova;
- return 0;
+ return vma;
}
-static int msm_gem_pin_iova(struct drm_gem_object *obj,
- struct msm_gem_address_space *aspace)
+static int msm_gem_pin_iova(struct drm_gem_object *obj, struct msm_gem_vma
*vma)
{
struct msm_gem_object *msm_obj = to_msm_bo(obj);
- struct msm_gem_vma *vma;
struct page **pages;
int ret, prot = IOMMU_READ;
@@ -426,15 +427,11 @@ static int msm_gem_pin_iova(struct drm_gem_object *obj,
if (GEM_WARN_ON(msm_obj->madv != MSM_MADV_WILLNEED))
return -EBUSY;
- vma = lookup_vma(obj, aspace);
- if (GEM_WARN_ON(!vma))
- return -EINVAL;
-
pages = get_pages(obj);
if (IS_ERR(pages))
return PTR_ERR(pages);
- ret = msm_gem_map_vma(aspace, vma, prot, msm_obj->sgt, obj->size);
+ ret = msm_gem_map_vma(vma->aspace, vma, prot, msm_obj->sgt, obj->size);
if (!ret)
msm_obj->pin_count++;
@@ -446,19 +443,18 @@ static int get_and_pin_iova_range_locked(struct
drm_gem_object *obj,
struct msm_gem_address_space *aspace, uint64_t *iova,
u64 range_start, u64 range_end)
{
- u64 local;
+ struct msm_gem_vma *vma;
int ret;
GEM_WARN_ON(!msm_gem_is_locked(obj));
- ret = get_iova_locked(obj, aspace, ,
- range_start, range_end);
-
- if (!ret)
- ret = msm_gem_pin_iova(obj, aspace);
+ vma = get_vma_locked(obj, aspace, range_start, range_end);
+ if (IS_ERR(vma))
+ return PTR_ERR(vma);
+ ret = msm_gem_pin_iova(obj, vma);
if (!ret)
- *iova = local;
+ *iova = vma->iova;
return ret;
}
@@ -500,10 +496,16 @@ int msm_gem_get_and_pin_iova(struct drm_gem_object *obj,
int msm_gem_get_iova(struct drm_gem_object *obj,
struct msm_gem_address_space *aspace, uint64_t *iova)
{
- int ret;
+ struct msm_gem_vma *vma;
+ int ret = 0;
msm_gem_lock(obj);
- ret = get_iova_locked(obj, aspace, iova, 0, U64_MAX);
+ vma = get_vma_locked(obj, aspace, 0, U64_MAX);
+ if (IS_ERR(vma)) {
+ ret = PTR_ERR(vma);
+ } else {
+ *iova = vma->iova;
+ }
msm_gem_unlock(obj);
return ret;
--
2.35.1
