[01/13] ttm: abstruct evictable bo

Submitted by Zhou, David(ChunMing) on May 9, 2018, 6:45 a.m.

Details

Message ID 20180509064543.15937-2-david1.zhou@amd.com
State New
Headers show
Series "*** per vm lru ***" ( rev: 1 ) in AMD X.Org drivers

Not browsing as part of any series.

Commit Message

Zhou, David(ChunMing) May 9, 2018, 6:45 a.m.
Change-Id: Ie81985282fab1e564fc2948109fae2173613b465
Signed-off-by: Chunming Zhou <david1.zhou@amd.com>
---
 drivers/gpu/drm/ttm/ttm_bo.c | 35 ++++++++++++++++++++++++-----------
 1 file changed, 24 insertions(+), 11 deletions(-)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index 98e06f8bf23b..15506682a0be 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -704,22 +704,20 @@  static bool ttm_bo_evict_swapout_allowable(struct ttm_buffer_object *bo,
 	return ret;
 }
 
-static int ttm_mem_evict_first(struct ttm_bo_device *bdev,
-			       uint32_t mem_type,
-			       const struct ttm_place *place,
-			       struct ttm_operation_ctx *ctx)
+static struct ttm_buffer_object *
+ttm_mem_get_evictable_bo(struct ttm_bo_device *bdev,
+			 uint32_t mem_type,
+			 const struct ttm_place *place,
+			 struct ttm_operation_ctx *ctx,
+			 bool *locked)
 {
-	struct ttm_bo_global *glob = bdev->glob;
-	struct ttm_mem_type_manager *man = &bdev->man[mem_type];
 	struct ttm_buffer_object *bo = NULL;
-	bool locked = false;
-	unsigned i;
-	int ret;
+	struct ttm_mem_type_manager *man = &bdev->man[mem_type];
+	int i;
 
-	spin_lock(&glob->lru_lock);
 	for (i = 0; i < TTM_MAX_BO_PRIORITY; ++i) {
 		list_for_each_entry(bo, &man->lru[i], lru) {
-			if (!ttm_bo_evict_swapout_allowable(bo, ctx, &locked))
+			if (!ttm_bo_evict_swapout_allowable(bo, ctx, locked))
 				continue;
 
 			if (place && !bdev->driver->eviction_valuable(bo,
@@ -738,6 +736,21 @@  static int ttm_mem_evict_first(struct ttm_bo_device *bdev,
 		bo = NULL;
 	}
 
+	return bo;
+}
+
+static int ttm_mem_evict_first(struct ttm_bo_device *bdev,
+			       uint32_t mem_type,
+			       const struct ttm_place *place,
+			       struct ttm_operation_ctx *ctx)
+{
+	struct ttm_bo_global *glob = bdev->glob;
+	struct ttm_buffer_object *bo = NULL;
+	bool locked = false;
+	int ret;
+
+	spin_lock(&glob->lru_lock);
+	bo = ttm_mem_get_evictable_bo(bdev, mem_type, place, ctx, &locked);
 	if (!bo) {
 		spin_unlock(&glob->lru_lock);
 		return -EBUSY;

Comments

All of those changes are including a Change-Id that has no bearing in
upstream patches and are missing a proper commit description explaining
why a specific change is done.

Regards,
Lucas

Am Mittwoch, den 09.05.2018, 14:45 +0800 schrieb Chunming Zhou:
> Change-Id: Ie81985282fab1e564fc2948109fae2173613b465
> > Signed-off-by: Chunming Zhou <david1.zhou@amd.com>
> ---
>  drivers/gpu/drm/ttm/ttm_bo.c | 35 ++++++++++++++++++++++++-----------
>  1 file changed, 24 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
> index 98e06f8bf23b..15506682a0be 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
> @@ -704,22 +704,20 @@ static bool ttm_bo_evict_swapout_allowable(struct ttm_buffer_object *bo,
> >  	return ret;
>  }
>  
> -static int ttm_mem_evict_first(struct ttm_bo_device *bdev,
> > -			       uint32_t mem_type,
> > -			       const struct ttm_place *place,
> > -			       struct ttm_operation_ctx *ctx)
> +static struct ttm_buffer_object *
> +ttm_mem_get_evictable_bo(struct ttm_bo_device *bdev,
> > +			 uint32_t mem_type,
> > +			 const struct ttm_place *place,
> > +			 struct ttm_operation_ctx *ctx,
> > +			 bool *locked)
>  {
> > -	struct ttm_bo_global *glob = bdev->glob;
> > -	struct ttm_mem_type_manager *man = &bdev->man[mem_type];
> >  	struct ttm_buffer_object *bo = NULL;
> > -	bool locked = false;
> > -	unsigned i;
> > -	int ret;
> > +	struct ttm_mem_type_manager *man = &bdev->man[mem_type];
> > +	int i;
>  
> > -	spin_lock(&glob->lru_lock);
> >  	for (i = 0; i < TTM_MAX_BO_PRIORITY; ++i) {
> >  		list_for_each_entry(bo, &man->lru[i], lru) {
> > -			if (!ttm_bo_evict_swapout_allowable(bo, ctx, &locked))
> > +			if (!ttm_bo_evict_swapout_allowable(bo, ctx, locked))
> >  				continue;
>  
> >  			if (place && !bdev->driver->eviction_valuable(bo,
> @@ -738,6 +736,21 @@ static int ttm_mem_evict_first(struct ttm_bo_device *bdev,
> >  		bo = NULL;
> >  	}
>  
> > +	return bo;
> +}
> +
> +static int ttm_mem_evict_first(struct ttm_bo_device *bdev,
> > +			       uint32_t mem_type,
> > +			       const struct ttm_place *place,
> > +			       struct ttm_operation_ctx *ctx)
> +{
> > +	struct ttm_bo_global *glob = bdev->glob;
> > +	struct ttm_buffer_object *bo = NULL;
> > +	bool locked = false;
> > +	int ret;
> +
> > +	spin_lock(&glob->lru_lock);
> > +	bo = ttm_mem_get_evictable_bo(bdev, mem_type, place, ctx, &locked);
> >  	if (!bo) {
> >  		spin_unlock(&glob->lru_lock);
> >  		return -EBUSY;
On Wed, May 09, 2018 at 10:34:51AM +0200, Lucas Stach wrote:
> All of those changes are including a Change-Id that has no bearing in
> upstream patches and are missing a proper commit description explaining
> why a specific change is done.

Imo the Change-Id: is ok if it makes people happy wrt internal tracking.
Linus might blow up, but there's lots of random nonsense that Linus blows
up on, so whatever.

Lack of real commit message that explains stuff is the real thing here I'd
say.
-Daniel

> 
> Regards,
> Lucas
> 
> Am Mittwoch, den 09.05.2018, 14:45 +0800 schrieb Chunming Zhou:
> > Change-Id: Ie81985282fab1e564fc2948109fae2173613b465
> > > Signed-off-by: Chunming Zhou <david1.zhou@amd.com>
> > ---
> >  drivers/gpu/drm/ttm/ttm_bo.c | 35 ++++++++++++++++++++++++-----------
> >  1 file changed, 24 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
> > index 98e06f8bf23b..15506682a0be 100644
> > --- a/drivers/gpu/drm/ttm/ttm_bo.c
> > +++ b/drivers/gpu/drm/ttm/ttm_bo.c
> > @@ -704,22 +704,20 @@ static bool ttm_bo_evict_swapout_allowable(struct ttm_buffer_object *bo,
> > >  	return ret;
> >  }
> >  
> > -static int ttm_mem_evict_first(struct ttm_bo_device *bdev,
> > > -			       uint32_t mem_type,
> > > -			       const struct ttm_place *place,
> > > -			       struct ttm_operation_ctx *ctx)
> > +static struct ttm_buffer_object *
> > +ttm_mem_get_evictable_bo(struct ttm_bo_device *bdev,
> > > +			 uint32_t mem_type,
> > > +			 const struct ttm_place *place,
> > > +			 struct ttm_operation_ctx *ctx,
> > > +			 bool *locked)
> >  {
> > > -	struct ttm_bo_global *glob = bdev->glob;
> > > -	struct ttm_mem_type_manager *man = &bdev->man[mem_type];
> > >  	struct ttm_buffer_object *bo = NULL;
> > > -	bool locked = false;
> > > -	unsigned i;
> > > -	int ret;
> > > +	struct ttm_mem_type_manager *man = &bdev->man[mem_type];
> > > +	int i;
> >  
> > > -	spin_lock(&glob->lru_lock);
> > >  	for (i = 0; i < TTM_MAX_BO_PRIORITY; ++i) {
> > >  		list_for_each_entry(bo, &man->lru[i], lru) {
> > > -			if (!ttm_bo_evict_swapout_allowable(bo, ctx, &locked))
> > > +			if (!ttm_bo_evict_swapout_allowable(bo, ctx, locked))
> > >  				continue;
> >  
> > >  			if (place && !bdev->driver->eviction_valuable(bo,
> > @@ -738,6 +736,21 @@ static int ttm_mem_evict_first(struct ttm_bo_device *bdev,
> > >  		bo = NULL;
> > >  	}
> >  
> > > +	return bo;
> > +}
> > +
> > +static int ttm_mem_evict_first(struct ttm_bo_device *bdev,
> > > +			       uint32_t mem_type,
> > > +			       const struct ttm_place *place,
> > > +			       struct ttm_operation_ctx *ctx)
> > +{
> > > +	struct ttm_bo_global *glob = bdev->glob;
> > > +	struct ttm_buffer_object *bo = NULL;
> > > +	bool locked = false;
> > > +	int ret;
> > +
> > > +	spin_lock(&glob->lru_lock);
> > > +	bo = ttm_mem_get_evictable_bo(bdev, mem_type, place, ctx, &locked);
> > >  	if (!bo) {
> > >  		spin_unlock(&glob->lru_lock);
> > >  		return -EBUSY;
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
On 2018年05月09日 17:50, Daniel Vetter wrote:
> On Wed, May 09, 2018 at 10:34:51AM +0200, Lucas Stach wrote:
>> All of those changes are including a Change-Id that has no bearing in
>> upstream patches and are missing a proper commit description explaining
>> why a specific change is done.
> Imo the Change-Id: is ok if it makes people happy wrt internal tracking.
> Linus might blow up, but there's lots of random nonsense that Linus blows
> up on, so whatever.
Yeah, Change-Id is just used internal, When upstreaming, it is removed. 
Alex, right? I'm not clear how you handle that when you upstream our 
internal patches.

>
> Lack of real commit message that explains stuff is the real thing here I'd
> say.
Agree, lacking commit message is really bad, that could be because this 
is a big feature, I was busy with implementing before.
If Christian agree with my this idea, I will update more commit for 
every patch when sending again.

Thanks,
David Zhou
> -Daniel
>
>> Regards,
>> Lucas
>>
>> Am Mittwoch, den 09.05.2018, 14:45 +0800 schrieb Chunming Zhou:
>>> Change-Id: Ie81985282fab1e564fc2948109fae2173613b465
>>>> Signed-off-by: Chunming Zhou <david1.zhou@amd.com>
>>> ---
>>>   drivers/gpu/drm/ttm/ttm_bo.c | 35 ++++++++++++++++++++++++-----------
>>>   1 file changed, 24 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
>>> index 98e06f8bf23b..15506682a0be 100644
>>> --- a/drivers/gpu/drm/ttm/ttm_bo.c
>>> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
>>> @@ -704,22 +704,20 @@ static bool ttm_bo_evict_swapout_allowable(struct ttm_buffer_object *bo,
>>>>   	return ret;
>>>   }
>>>   
>>> -static int ttm_mem_evict_first(struct ttm_bo_device *bdev,
>>>> -			       uint32_t mem_type,
>>>> -			       const struct ttm_place *place,
>>>> -			       struct ttm_operation_ctx *ctx)
>>> +static struct ttm_buffer_object *
>>> +ttm_mem_get_evictable_bo(struct ttm_bo_device *bdev,
>>>> +			 uint32_t mem_type,
>>>> +			 const struct ttm_place *place,
>>>> +			 struct ttm_operation_ctx *ctx,
>>>> +			 bool *locked)
>>>   {
>>>> -	struct ttm_bo_global *glob = bdev->glob;
>>>> -	struct ttm_mem_type_manager *man = &bdev->man[mem_type];
>>>>   	struct ttm_buffer_object *bo = NULL;
>>>> -	bool locked = false;
>>>> -	unsigned i;
>>>> -	int ret;
>>>> +	struct ttm_mem_type_manager *man = &bdev->man[mem_type];
>>>> +	int i;
>>>   
>>>> -	spin_lock(&glob->lru_lock);
>>>>   	for (i = 0; i < TTM_MAX_BO_PRIORITY; ++i) {
>>>>   		list_for_each_entry(bo, &man->lru[i], lru) {
>>>> -			if (!ttm_bo_evict_swapout_allowable(bo, ctx, &locked))
>>>> +			if (!ttm_bo_evict_swapout_allowable(bo, ctx, locked))
>>>>   				continue;
>>>   
>>>>   			if (place && !bdev->driver->eviction_valuable(bo,
>>> @@ -738,6 +736,21 @@ static int ttm_mem_evict_first(struct ttm_bo_device *bdev,
>>>>   		bo = NULL;
>>>>   	}
>>>   
>>>> +	return bo;
>>> +}
>>> +
>>> +static int ttm_mem_evict_first(struct ttm_bo_device *bdev,
>>>> +			       uint32_t mem_type,
>>>> +			       const struct ttm_place *place,
>>>> +			       struct ttm_operation_ctx *ctx)
>>> +{
>>>> +	struct ttm_bo_global *glob = bdev->glob;
>>>> +	struct ttm_buffer_object *bo = NULL;
>>>> +	bool locked = false;
>>>> +	int ret;
>>> +
>>>> +	spin_lock(&glob->lru_lock);
>>>> +	bo = ttm_mem_get_evictable_bo(bdev, mem_type, place, ctx, &locked);
>>>>   	if (!bo) {
>>>>   		spin_unlock(&glob->lru_lock);
>>>>   		return -EBUSY;
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Wed, May 9, 2018 at 6:06 AM, zhoucm1 <zhoucm1@amd.com> wrote:
>
>
> On 2018年05月09日 17:50, Daniel Vetter wrote:
>>
>> On Wed, May 09, 2018 at 10:34:51AM +0200, Lucas Stach wrote:
>>>
>>> All of those changes are including a Change-Id that has no bearing in
>>> upstream patches and are missing a proper commit description explaining
>>> why a specific change is done.
>>
>> Imo the Change-Id: is ok if it makes people happy wrt internal tracking.
>> Linus might blow up, but there's lots of random nonsense that Linus blows
>> up on, so whatever.
>
> Yeah, Change-Id is just used internal, When upstreaming, it is removed.
> Alex, right? I'm not clear how you handle that when you upstream our
> internal patches.

Yes, I strip them off when we upstream the patches, but we use them
internally.  For patch review just ignore them.

Alex

>
>>
>> Lack of real commit message that explains stuff is the real thing here I'd
>> say.
>
> Agree, lacking commit message is really bad, that could be because this is a
> big feature, I was busy with implementing before.
> If Christian agree with my this idea, I will update more commit for every
> patch when sending again.
>
> Thanks,
> David Zhou
>
>> -Daniel
>>
>>> Regards,
>>> Lucas
>>>
>>> Am Mittwoch, den 09.05.2018, 14:45 +0800 schrieb Chunming Zhou:
>>>>
>>>> Change-Id: Ie81985282fab1e564fc2948109fae2173613b465
>>>>>
>>>>> Signed-off-by: Chunming Zhou <david1.zhou@amd.com>
>>>>
>>>> ---
>>>>   drivers/gpu/drm/ttm/ttm_bo.c | 35 ++++++++++++++++++++++++-----------
>>>>   1 file changed, 24 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
>>>> index 98e06f8bf23b..15506682a0be 100644
>>>> --- a/drivers/gpu/drm/ttm/ttm_bo.c
>>>> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
>>>> @@ -704,22 +704,20 @@ static bool ttm_bo_evict_swapout_allowable(struct
>>>> ttm_buffer_object *bo,
>>>>>
>>>>>         return ret;
>>>>
>>>>   }
>>>>   -static int ttm_mem_evict_first(struct ttm_bo_device *bdev,
>>>>>
>>>>> -                              uint32_t mem_type,
>>>>> -                              const struct ttm_place *place,
>>>>> -                              struct ttm_operation_ctx *ctx)
>>>>
>>>> +static struct ttm_buffer_object *
>>>> +ttm_mem_get_evictable_bo(struct ttm_bo_device *bdev,
>>>>>
>>>>> +                        uint32_t mem_type,
>>>>> +                        const struct ttm_place *place,
>>>>> +                        struct ttm_operation_ctx *ctx,
>>>>> +                        bool *locked)
>>>>
>>>>   {
>>>>>
>>>>> -       struct ttm_bo_global *glob = bdev->glob;
>>>>> -       struct ttm_mem_type_manager *man = &bdev->man[mem_type];
>>>>>         struct ttm_buffer_object *bo = NULL;
>>>>> -       bool locked = false;
>>>>> -       unsigned i;
>>>>> -       int ret;
>>>>> +       struct ttm_mem_type_manager *man = &bdev->man[mem_type];
>>>>> +       int i;
>>>>
>>>>
>>>>>
>>>>> -       spin_lock(&glob->lru_lock);
>>>>>         for (i = 0; i < TTM_MAX_BO_PRIORITY; ++i) {
>>>>>                 list_for_each_entry(bo, &man->lru[i], lru) {
>>>>> -                       if (!ttm_bo_evict_swapout_allowable(bo, ctx,
>>>>> &locked))
>>>>> +                       if (!ttm_bo_evict_swapout_allowable(bo, ctx,
>>>>> locked))
>>>>>                                 continue;
>>>>
>>>>
>>>>>
>>>>>                         if (place &&
>>>>> !bdev->driver->eviction_valuable(bo,
>>>>
>>>> @@ -738,6 +736,21 @@ static int ttm_mem_evict_first(struct ttm_bo_device
>>>> *bdev,
>>>>>
>>>>>                 bo = NULL;
>>>>>         }
>>>>
>>>>
>>>>>
>>>>> +       return bo;
>>>>
>>>> +}
>>>> +
>>>> +static int ttm_mem_evict_first(struct ttm_bo_device *bdev,
>>>>>
>>>>> +                              uint32_t mem_type,
>>>>> +                              const struct ttm_place *place,
>>>>> +                              struct ttm_operation_ctx *ctx)
>>>>
>>>> +{
>>>>>
>>>>> +       struct ttm_bo_global *glob = bdev->glob;
>>>>> +       struct ttm_buffer_object *bo = NULL;
>>>>> +       bool locked = false;
>>>>> +       int ret;
>>>>
>>>> +
>>>>>
>>>>> +       spin_lock(&glob->lru_lock);
>>>>> +       bo = ttm_mem_get_evictable_bo(bdev, mem_type, place, ctx,
>>>>> &locked);
>>>>>         if (!bo) {
>>>>>                 spin_unlock(&glob->lru_lock);
>>>>>                 return -EBUSY;
>>>
>>> _______________________________________________
>>> dri-devel mailing list
>>> dri-devel@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx