diff mbox series

[v2,07/10] drm/msm/gem: Rework vma lookup and pin

Message ID 20220330204804.660819-8-robdclark@gmail.com (mailing list archive)
State New, archived
Headers show
Series drm/msm: Userspace allocated GPU addresses | expand

Commit Message

Rob Clark March 30, 2022, 8:47 p.m. UTC 
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(-)

Comments

Dmitry Osipenko March 31, 2022, 6:27 p.m. UTC | #1 
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


>  			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.
Dmitry Osipenko March 31, 2022, 6:28 p.m. UTC | #2 
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.
Rob Clark March 31, 2022, 6:58 p.m. UTC | #3 
On Thu, Mar 31, 2022 at 11:27 AM Dmitry Osipenko
<dmitry.osipenko@collabora.com> 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
>
>
> >                       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.
Dmitry Osipenko March 31, 2022, 8:49 p.m. UTC | #4 
On 3/31/22 21:58, Rob Clark wrote:
> On Thu, Mar 31, 2022 at 11:27 AM Dmitry Osipenko
> <dmitry.osipenko@collabora.com> 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
>>
>>
>>>                       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 <dmitry.osipenko@collabora.com>
diff mbox series

Patch

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, &local,
-		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;