diff mbox series

[07/10] drm/amdgpu/ttm: handle tt moves properly.

Message ID 20200923030454.362731-8-airlied@gmail.com (mailing list archive)
State New, archived
Headers show
Series ttm driver cleanups and invert move | expand

Commit Message

Dave Airlie Sept. 23, 2020, 3:04 a.m. UTC 
From: Dave Airlie <airlied@redhat.com>

The core move code currently handles use_tt moves, for amdgpu
this was being handled also in the driver, but not using the same
paths.

If moving between TT/SYSTEM (all the use_tt paths on amdgpu) use
the core move function.

Eventually the core will be flipped over to calling the driver.

Signed-off-by: Dave Airlie <airlied@redhat.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

Comments

Christian König Sept. 23, 2020, 2:45 p.m. UTC | #1 
Am 23.09.20 um 05:04 schrieb Dave Airlie:
> From: Dave Airlie <airlied@redhat.com>
>
> The core move code currently handles use_tt moves, for amdgpu
> this was being handled also in the driver, but not using the same
> paths.
>
> If moving between TT/SYSTEM (all the use_tt paths on amdgpu) use
> the core move function.
>
> Eventually the core will be flipped over to calling the driver.
>
> Signed-off-by: Dave Airlie <airlied@redhat.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 12 +++++++-----
>   1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> index db5f761f37ec..d3bd2fd448be 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> @@ -671,14 +671,16 @@ static int amdgpu_bo_move(struct ttm_buffer_object *bo, bool evict,
>   		ttm_bo_move_null(bo, new_mem);
>   		return 0;
>   	}
> -	if ((old_mem->mem_type == TTM_PL_TT &&
> -	     new_mem->mem_type == TTM_PL_SYSTEM) ||
> -	    (old_mem->mem_type == TTM_PL_SYSTEM &&
> -	     new_mem->mem_type == TTM_PL_TT)) {
> -		/* bind is enough */
> +	if (old_mem->mem_type == TTM_PL_SYSTEM &&
> +	    new_mem->mem_type == TTM_PL_TT) {
>   		ttm_bo_move_null(bo, new_mem);

I would feel better if we nuke ttm_bo_move_null() and always use 
ttm_bo_move_ttm().

Christian.

>   		return 0;
>   	}
> +
> +	if (old_mem->mem_type == TTM_PL_TT &&
> +	    new_mem->mem_type == TTM_PL_SYSTEM)
> +		return ttm_bo_move_ttm(bo, ctx, new_mem);
> +
>   	if (old_mem->mem_type == AMDGPU_PL_GDS ||
>   	    old_mem->mem_type == AMDGPU_PL_GWS ||
>   	    old_mem->mem_type == AMDGPU_PL_OA ||
Dave Airlie Sept. 24, 2020, 12:46 a.m. UTC | #2 
On Thu, 24 Sep 2020 at 00:46, Christian König <christian.koenig@amd.com> wrote:
>
> Am 23.09.20 um 05:04 schrieb Dave Airlie:
> > From: Dave Airlie <airlied@redhat.com>
> >
> > The core move code currently handles use_tt moves, for amdgpu
> > this was being handled also in the driver, but not using the same
> > paths.
> >
> > If moving between TT/SYSTEM (all the use_tt paths on amdgpu) use
> > the core move function.
> >
> > Eventually the core will be flipped over to calling the driver.
> >
> > Signed-off-by: Dave Airlie <airlied@redhat.com>
> > ---
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 12 +++++++-----
> >   1 file changed, 7 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> > index db5f761f37ec..d3bd2fd448be 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> > @@ -671,14 +671,16 @@ static int amdgpu_bo_move(struct ttm_buffer_object *bo, bool evict,
> >               ttm_bo_move_null(bo, new_mem);
> >               return 0;
> >       }
> > -     if ((old_mem->mem_type == TTM_PL_TT &&
> > -          new_mem->mem_type == TTM_PL_SYSTEM) ||
> > -         (old_mem->mem_type == TTM_PL_SYSTEM &&
> > -          new_mem->mem_type == TTM_PL_TT)) {
> > -             /* bind is enough */
> > +     if (old_mem->mem_type == TTM_PL_SYSTEM &&
> > +         new_mem->mem_type == TTM_PL_TT) {
> >               ttm_bo_move_null(bo, new_mem);
>
> I would feel better if we nuke ttm_bo_move_null() and always use
> ttm_bo_move_ttm().

Any reason? The code path in the current move code pretty much
is this.

The current move paths are

if (new_tt && old_tt)
  if old is system
     move null
  else
    move ttm
else
  call driver move.

So I wanted to maintain that order. calling the move ttm will just
make a pointless caching, populate, bind step.

But I'm going to think about it a bit more.

Dave.
Dave Airlie Sept. 24, 2020, 2:35 a.m. UTC | #3 
On Thu, 24 Sep 2020 at 10:46, Dave Airlie <airlied@gmail.com> wrote:
>
> On Thu, 24 Sep 2020 at 00:46, Christian König <christian.koenig@amd.com> wrote:
> >
> > Am 23.09.20 um 05:04 schrieb Dave Airlie:
> > > From: Dave Airlie <airlied@redhat.com>
> > >
> > > The core move code currently handles use_tt moves, for amdgpu
> > > this was being handled also in the driver, but not using the same
> > > paths.
> > >
> > > If moving between TT/SYSTEM (all the use_tt paths on amdgpu) use
> > > the core move function.
> > >
> > > Eventually the core will be flipped over to calling the driver.
> > >
> > > Signed-off-by: Dave Airlie <airlied@redhat.com>
> > > ---
> > >   drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 12 +++++++-----
> > >   1 file changed, 7 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> > > index db5f761f37ec..d3bd2fd448be 100644
> > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> > > @@ -671,14 +671,16 @@ static int amdgpu_bo_move(struct ttm_buffer_object *bo, bool evict,
> > >               ttm_bo_move_null(bo, new_mem);
> > >               return 0;
> > >       }
> > > -     if ((old_mem->mem_type == TTM_PL_TT &&
> > > -          new_mem->mem_type == TTM_PL_SYSTEM) ||
> > > -         (old_mem->mem_type == TTM_PL_SYSTEM &&
> > > -          new_mem->mem_type == TTM_PL_TT)) {
> > > -             /* bind is enough */
> > > +     if (old_mem->mem_type == TTM_PL_SYSTEM &&
> > > +         new_mem->mem_type == TTM_PL_TT) {
> > >               ttm_bo_move_null(bo, new_mem);
> >
> > I would feel better if we nuke ttm_bo_move_null() and always use
> > ttm_bo_move_ttm().
>
> Any reason? The code path in the current move code pretty much
> is this.
>
> The current move paths are
>
> if (new_tt && old_tt)
>   if old is system
>      move null
>   else
>     move ttm
> else
>   call driver move.
>
> So I wanted to maintain that order. calling the move ttm will just
> make a pointless caching, populate, bind step.
>
> But I'm going to think about it a bit more.

I've looked into moving the code over to move_ttm to see if I could
combine things better, but it doesn't really fall out that nicely.

I might try again on top of the refactoring once I'm gone further.

Dave.
Christian König Sept. 24, 2020, 9:48 a.m. UTC | #4 
Am 24.09.20 um 02:46 schrieb Dave Airlie:
> On Thu, 24 Sep 2020 at 00:46, Christian König <christian.koenig@amd.com> wrote:
>> Am 23.09.20 um 05:04 schrieb Dave Airlie:
>>> From: Dave Airlie <airlied@redhat.com>
>>>
>>> The core move code currently handles use_tt moves, for amdgpu
>>> this was being handled also in the driver, but not using the same
>>> paths.
>>>
>>> If moving between TT/SYSTEM (all the use_tt paths on amdgpu) use
>>> the core move function.
>>>
>>> Eventually the core will be flipped over to calling the driver.
>>>
>>> Signed-off-by: Dave Airlie <airlied@redhat.com>
>>> ---
>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 12 +++++++-----
>>>    1 file changed, 7 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>> index db5f761f37ec..d3bd2fd448be 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>> @@ -671,14 +671,16 @@ static int amdgpu_bo_move(struct ttm_buffer_object *bo, bool evict,
>>>                ttm_bo_move_null(bo, new_mem);
>>>                return 0;
>>>        }
>>> -     if ((old_mem->mem_type == TTM_PL_TT &&
>>> -          new_mem->mem_type == TTM_PL_SYSTEM) ||
>>> -         (old_mem->mem_type == TTM_PL_SYSTEM &&
>>> -          new_mem->mem_type == TTM_PL_TT)) {
>>> -             /* bind is enough */
>>> +     if (old_mem->mem_type == TTM_PL_SYSTEM &&
>>> +         new_mem->mem_type == TTM_PL_TT) {
>>>                ttm_bo_move_null(bo, new_mem);
>> I would feel better if we nuke ttm_bo_move_null() and always use
>> ttm_bo_move_ttm().
> Any reason? The code path in the current move code pretty much
> is this.
>
> The current move paths are
>
> if (new_tt && old_tt)
>    if old is system
>       move null
>    else
>      move ttm
> else
>    call driver move.
>
> So I wanted to maintain that order. calling the move ttm will just
> make a pointless caching, populate, bind step.

Well we want to get rid of the bind/unbind stuff in TTM.

And I'm seriously thinking about getting rid of all the caching stuff.

So all of this should just go away rather soon.

Christian.

>
> But I'm going to think about it a bit more.
>
> Dave.
diff mbox series

Patch

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
index db5f761f37ec..d3bd2fd448be 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
@@ -671,14 +671,16 @@  static int amdgpu_bo_move(struct ttm_buffer_object *bo, bool evict,
 		ttm_bo_move_null(bo, new_mem);
 		return 0;
 	}
-	if ((old_mem->mem_type == TTM_PL_TT &&
-	     new_mem->mem_type == TTM_PL_SYSTEM) ||
-	    (old_mem->mem_type == TTM_PL_SYSTEM &&
-	     new_mem->mem_type == TTM_PL_TT)) {
-		/* bind is enough */
+	if (old_mem->mem_type == TTM_PL_SYSTEM &&
+	    new_mem->mem_type == TTM_PL_TT) {
 		ttm_bo_move_null(bo, new_mem);
 		return 0;
 	}
+
+	if (old_mem->mem_type == TTM_PL_TT &&
+	    new_mem->mem_type == TTM_PL_SYSTEM)
+		return ttm_bo_move_ttm(bo, ctx, new_mem);
+
 	if (old_mem->mem_type == AMDGPU_PL_GDS ||
 	    old_mem->mem_type == AMDGPU_PL_GWS ||
 	    old_mem->mem_type == AMDGPU_PL_OA ||