| Message ID | 1513071228-29551-1-git-send-email-Hongbo.He@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <Hongbo.He@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; 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; > }; >
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.He@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; > }; >
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 <Hongbo.He@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; >> }; > > > _______________________________________________ > amd-gfx mailing list > amd-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/amd-gfx
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.
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. >
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:thomas@shipmail.org] Sent: Thursday, December 14, 2017 3:21 AM To: He, Roger <Hongbo.He@amd.com>; amd-gfx@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.He@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; > }; >
On Tue, Dec 12, 2017 at 10:34 AM Roger He <Hongbo.He@amd.com> 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.He@amd.com> 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
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; };
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.He@amd.com> --- include/drm/ttm/ttm_bo_api.h | 4 ++++ 1 file changed, 4 insertions(+)