Re: [PATCH 1/6] drm/ttm: add on_alloc_stage and reservation into ttm_operation_ctx

[Daniel Vetter]

On Tue, Dec 12, 2017 at 10:34 AM Roger He  wrote:
>
> on_alloc_stage: is this operation on allocation stage
> resv: reservation bo used of this operation
>
> Change-Id: I01ea482e8c7470014196eb218e2ff8913306eef0
> Signed-off-by: Roger He 

Real commit message (the later patches using this are even sparser)
and/or proper kerneldoc would be massively appreciated in common code.
You guys have done a ton of stuff and special cases just for amdgpu,
and if someone doesn't at least know what you aimed to do it's pretty
much impossible to understand. I don't care much if this is the
standard in drivers, but common code really should bother to explain
what problem it's trying to solve.

(yes I think I figured out what it's doing, but I'm pretty sure most
dri-devel folks hacking on gem/cs/render stuff won't be so lucky)
-Daniel

> ---
>  include/drm/ttm/ttm_bo_api.h | 4 
>  1 file changed, 4 insertions(+)
>
> diff --git a/include/drm/ttm/ttm_bo_api.h b/include/drm/ttm/ttm_bo_api.h
> index 368eb02..25de597 100644
> --- a/include/drm/ttm/ttm_bo_api.h
> +++ b/include/drm/ttm/ttm_bo_api.h
> @@ -263,6 +263,8 @@ struct ttm_bo_kmap_obj {
>   *
>   * @interruptible: Sleep interruptible if sleeping.
>   * @no_wait_gpu: Return immediately if the GPU is busy.
> + * @on_alloc_stage: is this operation on allocation stage
> + * @resv: resvation bo used
>   *
>   * Context for TTM operations like changing buffer placement or general 
> memory
>   * allocation.
> @@ -270,6 +272,8 @@ struct ttm_bo_kmap_obj {
>  struct ttm_operation_ctx {
> bool interruptible;
> bool no_wait_gpu;
> +   bool on_alloc_stage;
> +   struct reservation_object *resv;
> uint64_t bytes_moved;
>  };
>
> --
> 2.7.4
>
> ___
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

RE: [PATCH 1/6] drm/ttm: add on_alloc_stage and reservation into ttm_operation_ctx

[He, Roger]

Hi Thomas:

Really sorry for that and will keep that in mind.
If necessary, next time I will send cover letter to provide more background and 
details.


Thanks
Roger(Hongbo.He)

-Original Message-
From: Thomas Hellstrom [mailto:tho...@shipmail.org] 
Sent: Thursday, December 14, 2017 3:21 AM
To: He, Roger <hongbo...@amd.com>; amd-...@lists.freedesktop.org; 
dri-devel@lists.freedesktop.org
Subject: Re: [PATCH 1/6] drm/ttm: add on_alloc_stage and reservation into 
ttm_operation_ctx

Hi,

I think this series is quite poorly documented. We should have a log message 
explaining the purpose of the commit.
Also since it's not obvious what the series is attempting to achieve, please 
add a 0/X series header message..

/Thomas


On 12/12/2017 10:33 AM, Roger He wrote:
> on_alloc_stage: is this operation on allocation stage
> resv: reservation bo used of this operation
>
> Change-Id: I01ea482e8c7470014196eb218e2ff8913306eef0
> Signed-off-by: Roger He <hongbo...@amd.com>
> ---
>   include/drm/ttm/ttm_bo_api.h | 4 
>   1 file changed, 4 insertions(+)
>
> diff --git a/include/drm/ttm/ttm_bo_api.h 
> b/include/drm/ttm/ttm_bo_api.h index 368eb02..25de597 100644
> --- a/include/drm/ttm/ttm_bo_api.h
> +++ b/include/drm/ttm/ttm_bo_api.h
> @@ -263,6 +263,8 @@ struct ttm_bo_kmap_obj {
>*
>* @interruptible: Sleep interruptible if sleeping.
>* @no_wait_gpu: Return immediately if the GPU is busy.
> + * @on_alloc_stage: is this operation on allocation stage
> + * @resv: resvation bo used
>*
>* Context for TTM operations like changing buffer placement or general 
> memory
>* allocation.
> @@ -270,6 +272,8 @@ struct ttm_bo_kmap_obj {
>   struct ttm_operation_ctx {
>   bool interruptible;
>   bool no_wait_gpu;
> + bool on_alloc_stage;
> + struct reservation_object *resv;
>   uint64_t bytes_moved;
>   };
>   


___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/6] drm/ttm: add on_alloc_stage and reservation into ttm_operation_ctx

[Christian König]

Am 14.12.2017 um 09:55 schrieb Thomas Hellstrom:

Hi, Christian,

On 12/14/2017 09:40 AM, Christian König wrote:

Hi Thomas,

sorry for that. Noted on the rest of that series as well that we need 
to improve the commit messages. But this one somehow slipped through 
because I discussed this change previously internally with Roger.


That made the change completely logical for me, but without this 
context everybody else just thinks "Hui what?". Going to keep that in 
mind the next time.


But back to topic: This series allows BOs which share the same 
reservation object as the BO currently allocated/validated to be 
evicted even when they are reserved.


This is useful because amdgpu wants to use a single reservation 
object for almost all BOs of a process.


Yes, that indeed makes the whole thing more clear, and makes sense.

Out of interest, is the shared reservation object usage a speed 
optimization (avoiding the ww_mutex_locks at reservation time?)

or something else?


Avoiding taking many ww_mutex_locks is one reason. The other major 
reason comes with GPU-VM page tables.


Just the same as CPU page tables multi level GPU page tables are 
allocated individually when needed. But during command submission we 
must make sure that all GPU page tables are validated and fences added 
to all of them.


Because of this we are using the reservation object of the root page 
directory (because that one is always allocated) as reservation object 
for all other page tables.


The effect is that you have to add the resulting fence of a command 
submission to only one reservation object and not a couple of hundreds 
or even thousands.


I guess that even if LRU lists might get crowded with unevictable BOs, 
iterating through those lists isn't really part of the fast path.


Yes, exactly. When we start to massively evict things performance goes 
down so much anyway that this extra cycling over the LRU doesn't hurt us 
much.


Christian.


/Thomas



Regards,
Christian. 




___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/6] drm/ttm: add on_alloc_stage and reservation into ttm_operation_ctx

[Thomas Hellstrom]

Hi, Christian,

On 12/14/2017 09:40 AM, Christian König wrote:

Hi Thomas,

sorry for that. Noted on the rest of that series as well that we need 
to improve the commit messages. But this one somehow slipped through 
because I discussed this change previously internally with Roger.


That made the change completely logical for me, but without this 
context everybody else just thinks "Hui what?". Going to keep that in 
mind the next time.


But back to topic: This series allows BOs which share the same 
reservation object as the BO currently allocated/validated to be 
evicted even when they are reserved.


This is useful because amdgpu wants to use a single reservation object 
for almost all BOs of a process.


Yes, that indeed makes the whole thing more clear, and makes sense.

Out of interest, is the shared reservation object usage a speed 
optimization (avoiding the ww_mutex_locks at reservation time?)

or something else?

I guess that even if LRU lists might get crowded with unevictable BOs, 
iterating through those lists isn't really part of the fast path.

/Thomas



Regards,
Christian. 


___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/6] drm/ttm: add on_alloc_stage and reservation into ttm_operation_ctx

[Christian König]

Hi Thomas,

sorry for that. Noted on the rest of that series as well that we need to 
improve the commit messages. But this one somehow slipped through 
because I discussed this change previously internally with Roger.


That made the change completely logical for me, but without this context 
everybody else just thinks "Hui what?". Going to keep that in mind the 
next time.


But back to topic: This series allows BOs which share the same 
reservation object as the BO currently allocated/validated to be evicted 
even when they are reserved.


This is useful because amdgpu wants to use a single reservation object 
for almost all BOs of a process.


Regards,
Christian.

Am 13.12.2017 um 20:21 schrieb Thomas Hellstrom:

Hi,

I think this series is quite poorly documented. We should have a log 
message explaining the purpose of the commit.
Also since it's not obvious what the series is attempting to achieve, 
please add a 0/X series header message..


/Thomas


On 12/12/2017 10:33 AM, Roger He wrote:

on_alloc_stage: is this operation on allocation stage
resv: reservation bo used of this operation

Change-Id: I01ea482e8c7470014196eb218e2ff8913306eef0
Signed-off-by: Roger He 
---
  include/drm/ttm/ttm_bo_api.h | 4 
  1 file changed, 4 insertions(+)

diff --git a/include/drm/ttm/ttm_bo_api.h b/include/drm/ttm/ttm_bo_api.h
index 368eb02..25de597 100644
--- a/include/drm/ttm/ttm_bo_api.h
+++ b/include/drm/ttm/ttm_bo_api.h
@@ -263,6 +263,8 @@ struct ttm_bo_kmap_obj {
   *
   * @interruptible: Sleep interruptible if sleeping.
   * @no_wait_gpu: Return immediately if the GPU is busy.
+ * @on_alloc_stage: is this operation on allocation stage
+ * @resv: resvation bo used
   *
   * Context for TTM operations like changing buffer placement or 
general memory

   * allocation.
@@ -270,6 +272,8 @@ struct ttm_bo_kmap_obj {
  struct ttm_operation_ctx {
  bool interruptible;
  bool no_wait_gpu;
+    bool on_alloc_stage;
+    struct reservation_object *resv;
  uint64_t bytes_moved;
  };



___
amd-gfx mailing list
amd-...@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx


___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/6] drm/ttm: add on_alloc_stage and reservation into ttm_operation_ctx

[Thomas Hellstrom]

Hi,

I think this series is quite poorly documented. We should have a log 
message explaining the purpose of the commit.
Also since it's not obvious what the series is attempting to achieve, 
please add a 0/X series header message..


/Thomas


On 12/12/2017 10:33 AM, Roger He wrote:

on_alloc_stage: is this operation on allocation stage
resv: reservation bo used of this operation

Change-Id: I01ea482e8c7470014196eb218e2ff8913306eef0
Signed-off-by: Roger He 
---
  include/drm/ttm/ttm_bo_api.h | 4 
  1 file changed, 4 insertions(+)

diff --git a/include/drm/ttm/ttm_bo_api.h b/include/drm/ttm/ttm_bo_api.h
index 368eb02..25de597 100644
--- a/include/drm/ttm/ttm_bo_api.h
+++ b/include/drm/ttm/ttm_bo_api.h
@@ -263,6 +263,8 @@ struct ttm_bo_kmap_obj {
   *
   * @interruptible: Sleep interruptible if sleeping.
   * @no_wait_gpu: Return immediately if the GPU is busy.
+ * @on_alloc_stage: is this operation on allocation stage
+ * @resv: resvation bo used
   *
   * Context for TTM operations like changing buffer placement or general memory
   * allocation.
@@ -270,6 +272,8 @@ struct ttm_bo_kmap_obj {
  struct ttm_operation_ctx {
bool interruptible;
bool no_wait_gpu;
+   bool on_alloc_stage;
+   struct reservation_object *resv;
uint64_t bytes_moved;
  };
  



___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/6] drm/ttm: add on_alloc_stage and reservation into ttm_operation_ctx

[Christian König]

Am 12.12.2017 um 10:33 schrieb Roger He:

on_alloc_stage: is this operation on allocation stage
resv: reservation bo used of this operation

Change-Id: I01ea482e8c7470014196eb218e2ff8913306eef0
Signed-off-by: Roger He 
---
  include/drm/ttm/ttm_bo_api.h | 4 
  1 file changed, 4 insertions(+)

diff --git a/include/drm/ttm/ttm_bo_api.h b/include/drm/ttm/ttm_bo_api.h
index 368eb02..25de597 100644
--- a/include/drm/ttm/ttm_bo_api.h
+++ b/include/drm/ttm/ttm_bo_api.h
@@ -263,6 +263,8 @@ struct ttm_bo_kmap_obj {
   *
   * @interruptible: Sleep interruptible if sleeping.
   * @no_wait_gpu: Return immediately if the GPU is busy.
+ * @on_alloc_stage: is this operation on allocation stage
+ * @resv: resvation bo used
   *
   * Context for TTM operations like changing buffer placement or general memory
   * allocation.
@@ -270,6 +272,8 @@ struct ttm_bo_kmap_obj {
  struct ttm_operation_ctx {
bool interruptible;
bool no_wait_gpu;
+   bool on_alloc_stage;


The name describes how amdgpu want to use the flag and not what effect 
it has.


How about "allow_reserved_eviction"?

And also please update the documentation with something like:
* @allow_reserved_eviction: Allow eviction of reserved BOs.
* @resv: Reservation object to allow reserved evictions with.

Alternative we could put the ww_mutex context in here as Daniel 
suggested, but I think we should stick with what we have for now and 
make that change when we find another use case for this.


Christian.


+   struct reservation_object *resv;
uint64_t bytes_moved;
  };
  


___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel