Re: [Intel-gfx] [RFC PATCH 55/97] drm/i915/guc: Update intel_gt_wait_for_idle to work with GuC
On Thu, May 27, 2021 at 10:02:55AM +0100, Tvrtko Ursulin wrote:
>
> On 26/05/2021 19:18, Matthew Brost wrote:
> > On Wed, May 26, 2021 at 10:21:05AM +0100, Tvrtko Ursulin wrote:
> > >
> > > On 25/05/2021 18:07, Matthew Brost wrote:
> > > > On Tue, May 25, 2021 at 11:06:00AM +0100, Tvrtko Ursulin wrote:
> > > > >
> > > > > On 06/05/2021 20:14, Matthew Brost wrote:
> > > > > > When running the GuC the GPU can't be considered idle if the GuC
> > > > > > still
> > > > > > has contexts pinned. As such, a call has been added in
> > > > > > intel_gt_wait_for_idle to idle the UC and in turn the GuC by
> > > > > > waiting for
> > > > > > the number of unpinned contexts to go to zero.
> > > > > >
> > > > > > Cc: John Harrison
> > > > > > Signed-off-by: Matthew Brost
> > > > > > ---
> > > > > > drivers/gpu/drm/i915/gem/i915_gem_mman.c | 3 +-
> > > > > > drivers/gpu/drm/i915/gt/intel_gt.c| 18
> > > > > > drivers/gpu/drm/i915/gt/intel_gt.h| 2 +
> > > > > > drivers/gpu/drm/i915/gt/intel_gt_requests.c | 22 ++---
> > > > > > drivers/gpu/drm/i915/gt/intel_gt_requests.h | 7 +-
> > > > > > drivers/gpu/drm/i915/gt/uc/intel_guc.h| 4 +
> > > > > > drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c | 1 +
> > > > > > drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h | 4 +
> > > > > > .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 91
> > > > > > ++-
> > > > > > drivers/gpu/drm/i915/gt/uc/intel_uc.h | 5 +
> > > > > > drivers/gpu/drm/i915/i915_debugfs.c | 1 +
> > > > > > drivers/gpu/drm/i915/i915_gem_evict.c | 1 +
> > > > > > .../gpu/drm/i915/selftests/igt_live_test.c| 2 +-
> > > > > > .../gpu/drm/i915/selftests/mock_gem_device.c | 3 +-
> > > > > > 14 files changed, 137 insertions(+), 27 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> > > > > > b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> > > > > > index 8598a1c78a4c..2f5295c9408d 100644
> > > > > > --- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> > > > > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> > > > > > @@ -634,7 +634,8 @@ mmap_offset_attach(struct drm_i915_gem_object
> > > > > > *obj,
> > > > > > goto insert;
> > > > > > /* Attempt to reap some mmap space from dead objects */
> > > > > > - err = intel_gt_retire_requests_timeout(>gt,
> > > > > > MAX_SCHEDULE_TIMEOUT);
> > > > > > + err = intel_gt_retire_requests_timeout(>gt,
> > > > > > MAX_SCHEDULE_TIMEOUT,
> > > > > > + NULL);
> > > > > > if (err)
> > > > > > goto err;
> > > > > > diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c
> > > > > > b/drivers/gpu/drm/i915/gt/intel_gt.c
> > > > > > index 8d77dcbad059..1742a8561f69 100644
> > > > > > --- a/drivers/gpu/drm/i915/gt/intel_gt.c
> > > > > > +++ b/drivers/gpu/drm/i915/gt/intel_gt.c
> > > > > > @@ -574,6 +574,24 @@ static void __intel_gt_disable(struct intel_gt
> > > > > > *gt)
> > > > > > GEM_BUG_ON(intel_gt_pm_is_awake(gt));
> > > > > > }
> > > > > > +int intel_gt_wait_for_idle(struct intel_gt *gt, long timeout)
> > > > > > +{
> > > > > > + long rtimeout;
> > > > > > +
> > > > > > + /* If the device is asleep, we have no requests outstanding */
> > > > > > + if (!intel_gt_pm_is_awake(gt))
> > > > > > + return 0;
> > > > > > +
> > > > > > + while ((timeout = intel_gt_retire_requests_timeout(gt, timeout,
> > > > > > + ))
> > > > > > > 0) {
> > > > > > + cond_resched();
> > > > > > + if (signal_pending(current))
> > > > > > + return -EINTR;
> > > > > > + }
> > > > > > +
> > > > > > + return timeout ? timeout : intel_uc_wait_for_idle(>uc,
> > > > > > rtimeout);
> > > > > > +}
> > > > > > +
> > > > > > int intel_gt_init(struct intel_gt *gt)
> > > > > > {
> > > > > > int err;
> > > > > > diff --git a/drivers/gpu/drm/i915/gt/intel_gt.h
> > > > > > b/drivers/gpu/drm/i915/gt/intel_gt.h
> > > > > > index 7ec395cace69..c775043334bf 100644
> > > > > > --- a/drivers/gpu/drm/i915/gt/intel_gt.h
> > > > > > +++ b/drivers/gpu/drm/i915/gt/intel_gt.h
> > > > > > @@ -48,6 +48,8 @@ void intel_gt_driver_release(struct intel_gt *gt);
> > > > > > void intel_gt_driver_late_release(struct intel_gt *gt);
> > > > > > +int intel_gt_wait_for_idle(struct intel_gt *gt, long timeout);
> > > > > > +
> > > > > > void intel_gt_check_and_clear_faults(struct intel_gt *gt);
> > > > > > void intel_gt_clear_error_registers(struct intel_gt *gt,
> > > > > > intel_engine_mask_t
> > > > > > engine_mask);
> > > > > > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_requests.c
> > > > > > b/drivers/gpu/drm/i915/gt/intel_gt_requests.c
> > > > > > index 647eca9d867a..c6c702f236fa 100644
> > > > >
Re: [Intel-gfx] [RFC PATCH 55/97] drm/i915/guc: Update intel_gt_wait_for_idle to work with GuC
On 26/05/2021 19:18, Matthew Brost wrote:
On Wed, May 26, 2021 at 10:21:05AM +0100, Tvrtko Ursulin wrote:
On 25/05/2021 18:07, Matthew Brost wrote:
On Tue, May 25, 2021 at 11:06:00AM +0100, Tvrtko Ursulin wrote:
On 06/05/2021 20:14, Matthew Brost wrote:
When running the GuC the GPU can't be considered idle if the GuC still
has contexts pinned. As such, a call has been added in
intel_gt_wait_for_idle to idle the UC and in turn the GuC by waiting for
the number of unpinned contexts to go to zero.
Cc: John Harrison
Signed-off-by: Matthew Brost
---
drivers/gpu/drm/i915/gem/i915_gem_mman.c | 3 +-
drivers/gpu/drm/i915/gt/intel_gt.c| 18
drivers/gpu/drm/i915/gt/intel_gt.h| 2 +
drivers/gpu/drm/i915/gt/intel_gt_requests.c | 22 ++---
drivers/gpu/drm/i915/gt/intel_gt_requests.h | 7 +-
drivers/gpu/drm/i915/gt/uc/intel_guc.h| 4 +
drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c | 1 +
drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h | 4 +
.../gpu/drm/i915/gt/uc/intel_guc_submission.c | 91 ++-
drivers/gpu/drm/i915/gt/uc/intel_uc.h | 5 +
drivers/gpu/drm/i915/i915_debugfs.c | 1 +
drivers/gpu/drm/i915/i915_gem_evict.c | 1 +
.../gpu/drm/i915/selftests/igt_live_test.c| 2 +-
.../gpu/drm/i915/selftests/mock_gem_device.c | 3 +-
14 files changed, 137 insertions(+), 27 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
index 8598a1c78a4c..2f5295c9408d 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
@@ -634,7 +634,8 @@ mmap_offset_attach(struct drm_i915_gem_object *obj,
goto insert;
/* Attempt to reap some mmap space from dead objects */
- err = intel_gt_retire_requests_timeout(>gt, MAX_SCHEDULE_TIMEOUT);
+ err = intel_gt_retire_requests_timeout(>gt, MAX_SCHEDULE_TIMEOUT,
+ NULL);
if (err)
goto err;
diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c
b/drivers/gpu/drm/i915/gt/intel_gt.c
index 8d77dcbad059..1742a8561f69 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt.c
+++ b/drivers/gpu/drm/i915/gt/intel_gt.c
@@ -574,6 +574,24 @@ static void __intel_gt_disable(struct intel_gt *gt)
GEM_BUG_ON(intel_gt_pm_is_awake(gt));
}
+int intel_gt_wait_for_idle(struct intel_gt *gt, long timeout)
+{
+ long rtimeout;
+
+ /* If the device is asleep, we have no requests outstanding */
+ if (!intel_gt_pm_is_awake(gt))
+ return 0;
+
+ while ((timeout = intel_gt_retire_requests_timeout(gt, timeout,
+ )) > 0) {
+ cond_resched();
+ if (signal_pending(current))
+ return -EINTR;
+ }
+
+ return timeout ? timeout : intel_uc_wait_for_idle(>uc, rtimeout);
+}
+
int intel_gt_init(struct intel_gt *gt)
{
int err;
diff --git a/drivers/gpu/drm/i915/gt/intel_gt.h
b/drivers/gpu/drm/i915/gt/intel_gt.h
index 7ec395cace69..c775043334bf 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt.h
+++ b/drivers/gpu/drm/i915/gt/intel_gt.h
@@ -48,6 +48,8 @@ void intel_gt_driver_release(struct intel_gt *gt);
void intel_gt_driver_late_release(struct intel_gt *gt);
+int intel_gt_wait_for_idle(struct intel_gt *gt, long timeout);
+
void intel_gt_check_and_clear_faults(struct intel_gt *gt);
void intel_gt_clear_error_registers(struct intel_gt *gt,
intel_engine_mask_t engine_mask);
diff --git a/drivers/gpu/drm/i915/gt/intel_gt_requests.c
b/drivers/gpu/drm/i915/gt/intel_gt_requests.c
index 647eca9d867a..c6c702f236fa 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt_requests.c
+++ b/drivers/gpu/drm/i915/gt/intel_gt_requests.c
@@ -13,6 +13,7 @@
#include "intel_gt_pm.h"
#include "intel_gt_requests.h"
#include "intel_timeline.h"
+#include "uc/intel_uc.h"
static bool retire_requests(struct intel_timeline *tl)
{
@@ -130,7 +131,8 @@ void intel_engine_fini_retire(struct intel_engine_cs
*engine)
GEM_BUG_ON(engine->retire);
}
-long intel_gt_retire_requests_timeout(struct intel_gt *gt, long timeout)
+long intel_gt_retire_requests_timeout(struct intel_gt *gt, long timeout,
+ long *rtimeout)
What is 'rtimeout', I know remaining, but it can be more self-descriptive to
start with.
'remaining_timeout' it is.
It feels a bit churny for what it is. How plausible would be alternatives to
either change existing timeout to in/out, or measure sleep internally in
this function, or just risk sleeping twice as long by passing the original
timeout to uc idle as well?
Originally had it just passing in the same value, got review feedback
saying I should pass in the adjusted value. Hard to make everyone happy.
Ok.
{
struct
Re: [Intel-gfx] [RFC PATCH 55/97] drm/i915/guc: Update intel_gt_wait_for_idle to work with GuC
On Wed, May 26, 2021 at 10:21:05AM +0100, Tvrtko Ursulin wrote:
>
> On 25/05/2021 18:07, Matthew Brost wrote:
> > On Tue, May 25, 2021 at 11:06:00AM +0100, Tvrtko Ursulin wrote:
> > >
> > > On 06/05/2021 20:14, Matthew Brost wrote:
> > > > When running the GuC the GPU can't be considered idle if the GuC still
> > > > has contexts pinned. As such, a call has been added in
> > > > intel_gt_wait_for_idle to idle the UC and in turn the GuC by waiting for
> > > > the number of unpinned contexts to go to zero.
> > > >
> > > > Cc: John Harrison
> > > > Signed-off-by: Matthew Brost
> > > > ---
> > > >drivers/gpu/drm/i915/gem/i915_gem_mman.c | 3 +-
> > > >drivers/gpu/drm/i915/gt/intel_gt.c| 18
> > > >drivers/gpu/drm/i915/gt/intel_gt.h| 2 +
> > > >drivers/gpu/drm/i915/gt/intel_gt_requests.c | 22 ++---
> > > >drivers/gpu/drm/i915/gt/intel_gt_requests.h | 7 +-
> > > >drivers/gpu/drm/i915/gt/uc/intel_guc.h| 4 +
> > > >drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c | 1 +
> > > >drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h | 4 +
> > > >.../gpu/drm/i915/gt/uc/intel_guc_submission.c | 91
> > > > ++-
> > > >drivers/gpu/drm/i915/gt/uc/intel_uc.h | 5 +
> > > >drivers/gpu/drm/i915/i915_debugfs.c | 1 +
> > > >drivers/gpu/drm/i915/i915_gem_evict.c | 1 +
> > > >.../gpu/drm/i915/selftests/igt_live_test.c| 2 +-
> > > >.../gpu/drm/i915/selftests/mock_gem_device.c | 3 +-
> > > >14 files changed, 137 insertions(+), 27 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> > > > b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> > > > index 8598a1c78a4c..2f5295c9408d 100644
> > > > --- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> > > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> > > > @@ -634,7 +634,8 @@ mmap_offset_attach(struct drm_i915_gem_object *obj,
> > > > goto insert;
> > > > /* Attempt to reap some mmap space from dead objects */
> > > > - err = intel_gt_retire_requests_timeout(>gt,
> > > > MAX_SCHEDULE_TIMEOUT);
> > > > + err = intel_gt_retire_requests_timeout(>gt,
> > > > MAX_SCHEDULE_TIMEOUT,
> > > > + NULL);
> > > > if (err)
> > > > goto err;
> > > > diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c
> > > > b/drivers/gpu/drm/i915/gt/intel_gt.c
> > > > index 8d77dcbad059..1742a8561f69 100644
> > > > --- a/drivers/gpu/drm/i915/gt/intel_gt.c
> > > > +++ b/drivers/gpu/drm/i915/gt/intel_gt.c
> > > > @@ -574,6 +574,24 @@ static void __intel_gt_disable(struct intel_gt *gt)
> > > > GEM_BUG_ON(intel_gt_pm_is_awake(gt));
> > > >}
> > > > +int intel_gt_wait_for_idle(struct intel_gt *gt, long timeout)
> > > > +{
> > > > + long rtimeout;
> > > > +
> > > > + /* If the device is asleep, we have no requests outstanding */
> > > > + if (!intel_gt_pm_is_awake(gt))
> > > > + return 0;
> > > > +
> > > > + while ((timeout = intel_gt_retire_requests_timeout(gt, timeout,
> > > > + ))
> > > > > 0) {
> > > > + cond_resched();
> > > > + if (signal_pending(current))
> > > > + return -EINTR;
> > > > + }
> > > > +
> > > > + return timeout ? timeout : intel_uc_wait_for_idle(>uc,
> > > > rtimeout);
> > > > +}
> > > > +
> > > >int intel_gt_init(struct intel_gt *gt)
> > > >{
> > > > int err;
> > > > diff --git a/drivers/gpu/drm/i915/gt/intel_gt.h
> > > > b/drivers/gpu/drm/i915/gt/intel_gt.h
> > > > index 7ec395cace69..c775043334bf 100644
> > > > --- a/drivers/gpu/drm/i915/gt/intel_gt.h
> > > > +++ b/drivers/gpu/drm/i915/gt/intel_gt.h
> > > > @@ -48,6 +48,8 @@ void intel_gt_driver_release(struct intel_gt *gt);
> > > >void intel_gt_driver_late_release(struct intel_gt *gt);
> > > > +int intel_gt_wait_for_idle(struct intel_gt *gt, long timeout);
> > > > +
> > > >void intel_gt_check_and_clear_faults(struct intel_gt *gt);
> > > >void intel_gt_clear_error_registers(struct intel_gt *gt,
> > > > intel_engine_mask_t engine_mask);
> > > > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_requests.c
> > > > b/drivers/gpu/drm/i915/gt/intel_gt_requests.c
> > > > index 647eca9d867a..c6c702f236fa 100644
> > > > --- a/drivers/gpu/drm/i915/gt/intel_gt_requests.c
> > > > +++ b/drivers/gpu/drm/i915/gt/intel_gt_requests.c
> > > > @@ -13,6 +13,7 @@
> > > >#include "intel_gt_pm.h"
> > > >#include "intel_gt_requests.h"
> > > >#include "intel_timeline.h"
> > > > +#include "uc/intel_uc.h"
> > > >static bool retire_requests(struct intel_timeline *tl)
> > > >{
> > > > @@ -130,7 +131,8 @@ void intel_engine_fini_retire(struct
> > > > intel_engine_cs *engine)
> > > > GEM_BUG_ON(engine->retire);
> > > >}
> > > > -long
Re: [Intel-gfx] [RFC PATCH 55/97] drm/i915/guc: Update intel_gt_wait_for_idle to work with GuC
On 25/05/2021 18:07, Matthew Brost wrote:
On Tue, May 25, 2021 at 11:06:00AM +0100, Tvrtko Ursulin wrote:
On 06/05/2021 20:14, Matthew Brost wrote:
When running the GuC the GPU can't be considered idle if the GuC still
has contexts pinned. As such, a call has been added in
intel_gt_wait_for_idle to idle the UC and in turn the GuC by waiting for
the number of unpinned contexts to go to zero.
Cc: John Harrison
Signed-off-by: Matthew Brost
---
drivers/gpu/drm/i915/gem/i915_gem_mman.c | 3 +-
drivers/gpu/drm/i915/gt/intel_gt.c| 18
drivers/gpu/drm/i915/gt/intel_gt.h| 2 +
drivers/gpu/drm/i915/gt/intel_gt_requests.c | 22 ++---
drivers/gpu/drm/i915/gt/intel_gt_requests.h | 7 +-
drivers/gpu/drm/i915/gt/uc/intel_guc.h| 4 +
drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c | 1 +
drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h | 4 +
.../gpu/drm/i915/gt/uc/intel_guc_submission.c | 91 ++-
drivers/gpu/drm/i915/gt/uc/intel_uc.h | 5 +
drivers/gpu/drm/i915/i915_debugfs.c | 1 +
drivers/gpu/drm/i915/i915_gem_evict.c | 1 +
.../gpu/drm/i915/selftests/igt_live_test.c| 2 +-
.../gpu/drm/i915/selftests/mock_gem_device.c | 3 +-
14 files changed, 137 insertions(+), 27 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
index 8598a1c78a4c..2f5295c9408d 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
@@ -634,7 +634,8 @@ mmap_offset_attach(struct drm_i915_gem_object *obj,
goto insert;
/* Attempt to reap some mmap space from dead objects */
- err = intel_gt_retire_requests_timeout(>gt, MAX_SCHEDULE_TIMEOUT);
+ err = intel_gt_retire_requests_timeout(>gt, MAX_SCHEDULE_TIMEOUT,
+ NULL);
if (err)
goto err;
diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c
b/drivers/gpu/drm/i915/gt/intel_gt.c
index 8d77dcbad059..1742a8561f69 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt.c
+++ b/drivers/gpu/drm/i915/gt/intel_gt.c
@@ -574,6 +574,24 @@ static void __intel_gt_disable(struct intel_gt *gt)
GEM_BUG_ON(intel_gt_pm_is_awake(gt));
}
+int intel_gt_wait_for_idle(struct intel_gt *gt, long timeout)
+{
+ long rtimeout;
+
+ /* If the device is asleep, we have no requests outstanding */
+ if (!intel_gt_pm_is_awake(gt))
+ return 0;
+
+ while ((timeout = intel_gt_retire_requests_timeout(gt, timeout,
+ )) > 0) {
+ cond_resched();
+ if (signal_pending(current))
+ return -EINTR;
+ }
+
+ return timeout ? timeout : intel_uc_wait_for_idle(>uc, rtimeout);
+}
+
int intel_gt_init(struct intel_gt *gt)
{
int err;
diff --git a/drivers/gpu/drm/i915/gt/intel_gt.h
b/drivers/gpu/drm/i915/gt/intel_gt.h
index 7ec395cace69..c775043334bf 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt.h
+++ b/drivers/gpu/drm/i915/gt/intel_gt.h
@@ -48,6 +48,8 @@ void intel_gt_driver_release(struct intel_gt *gt);
void intel_gt_driver_late_release(struct intel_gt *gt);
+int intel_gt_wait_for_idle(struct intel_gt *gt, long timeout);
+
void intel_gt_check_and_clear_faults(struct intel_gt *gt);
void intel_gt_clear_error_registers(struct intel_gt *gt,
intel_engine_mask_t engine_mask);
diff --git a/drivers/gpu/drm/i915/gt/intel_gt_requests.c
b/drivers/gpu/drm/i915/gt/intel_gt_requests.c
index 647eca9d867a..c6c702f236fa 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt_requests.c
+++ b/drivers/gpu/drm/i915/gt/intel_gt_requests.c
@@ -13,6 +13,7 @@
#include "intel_gt_pm.h"
#include "intel_gt_requests.h"
#include "intel_timeline.h"
+#include "uc/intel_uc.h"
static bool retire_requests(struct intel_timeline *tl)
{
@@ -130,7 +131,8 @@ void intel_engine_fini_retire(struct intel_engine_cs
*engine)
GEM_BUG_ON(engine->retire);
}
-long intel_gt_retire_requests_timeout(struct intel_gt *gt, long timeout)
+long intel_gt_retire_requests_timeout(struct intel_gt *gt, long timeout,
+ long *rtimeout)
What is 'rtimeout', I know remaining, but it can be more self-descriptive to
start with.
'remaining_timeout' it is.
It feels a bit churny for what it is. How plausible would be alternatives to
either change existing timeout to in/out, or measure sleep internally in
this function, or just risk sleeping twice as long by passing the original
timeout to uc idle as well?
Originally had it just passing in the same value, got review feedback
saying I should pass in the adjusted value. Hard to make everyone happy.
Ok.
{
struct intel_gt_timelines *timelines = >timelines;
struct intel_timeline *tl, *tn;
@@ -195,22 +197,10 @@ out_active:
Re: [Intel-gfx] [RFC PATCH 55/97] drm/i915/guc: Update intel_gt_wait_for_idle to work with GuC
On Tue, May 25, 2021 at 11:06:00AM +0100, Tvrtko Ursulin wrote:
>
> On 06/05/2021 20:14, Matthew Brost wrote:
> > When running the GuC the GPU can't be considered idle if the GuC still
> > has contexts pinned. As such, a call has been added in
> > intel_gt_wait_for_idle to idle the UC and in turn the GuC by waiting for
> > the number of unpinned contexts to go to zero.
> >
> > Cc: John Harrison
> > Signed-off-by: Matthew Brost
> > ---
> > drivers/gpu/drm/i915/gem/i915_gem_mman.c | 3 +-
> > drivers/gpu/drm/i915/gt/intel_gt.c| 18
> > drivers/gpu/drm/i915/gt/intel_gt.h| 2 +
> > drivers/gpu/drm/i915/gt/intel_gt_requests.c | 22 ++---
> > drivers/gpu/drm/i915/gt/intel_gt_requests.h | 7 +-
> > drivers/gpu/drm/i915/gt/uc/intel_guc.h| 4 +
> > drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c | 1 +
> > drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h | 4 +
> > .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 91 ++-
> > drivers/gpu/drm/i915/gt/uc/intel_uc.h | 5 +
> > drivers/gpu/drm/i915/i915_debugfs.c | 1 +
> > drivers/gpu/drm/i915/i915_gem_evict.c | 1 +
> > .../gpu/drm/i915/selftests/igt_live_test.c| 2 +-
> > .../gpu/drm/i915/selftests/mock_gem_device.c | 3 +-
> > 14 files changed, 137 insertions(+), 27 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> > b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> > index 8598a1c78a4c..2f5295c9408d 100644
> > --- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> > @@ -634,7 +634,8 @@ mmap_offset_attach(struct drm_i915_gem_object *obj,
> > goto insert;
> > /* Attempt to reap some mmap space from dead objects */
> > - err = intel_gt_retire_requests_timeout(>gt, MAX_SCHEDULE_TIMEOUT);
> > + err = intel_gt_retire_requests_timeout(>gt, MAX_SCHEDULE_TIMEOUT,
> > + NULL);
> > if (err)
> > goto err;
> > diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c
> > b/drivers/gpu/drm/i915/gt/intel_gt.c
> > index 8d77dcbad059..1742a8561f69 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_gt.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_gt.c
> > @@ -574,6 +574,24 @@ static void __intel_gt_disable(struct intel_gt *gt)
> > GEM_BUG_ON(intel_gt_pm_is_awake(gt));
> > }
> > +int intel_gt_wait_for_idle(struct intel_gt *gt, long timeout)
> > +{
> > + long rtimeout;
> > +
> > + /* If the device is asleep, we have no requests outstanding */
> > + if (!intel_gt_pm_is_awake(gt))
> > + return 0;
> > +
> > + while ((timeout = intel_gt_retire_requests_timeout(gt, timeout,
> > + )) > 0) {
> > + cond_resched();
> > + if (signal_pending(current))
> > + return -EINTR;
> > + }
> > +
> > + return timeout ? timeout : intel_uc_wait_for_idle(>uc, rtimeout);
> > +}
> > +
> > int intel_gt_init(struct intel_gt *gt)
> > {
> > int err;
> > diff --git a/drivers/gpu/drm/i915/gt/intel_gt.h
> > b/drivers/gpu/drm/i915/gt/intel_gt.h
> > index 7ec395cace69..c775043334bf 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_gt.h
> > +++ b/drivers/gpu/drm/i915/gt/intel_gt.h
> > @@ -48,6 +48,8 @@ void intel_gt_driver_release(struct intel_gt *gt);
> > void intel_gt_driver_late_release(struct intel_gt *gt);
> > +int intel_gt_wait_for_idle(struct intel_gt *gt, long timeout);
> > +
> > void intel_gt_check_and_clear_faults(struct intel_gt *gt);
> > void intel_gt_clear_error_registers(struct intel_gt *gt,
> > intel_engine_mask_t engine_mask);
> > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_requests.c
> > b/drivers/gpu/drm/i915/gt/intel_gt_requests.c
> > index 647eca9d867a..c6c702f236fa 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_gt_requests.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_gt_requests.c
> > @@ -13,6 +13,7 @@
> > #include "intel_gt_pm.h"
> > #include "intel_gt_requests.h"
> > #include "intel_timeline.h"
> > +#include "uc/intel_uc.h"
> > static bool retire_requests(struct intel_timeline *tl)
> > {
> > @@ -130,7 +131,8 @@ void intel_engine_fini_retire(struct intel_engine_cs
> > *engine)
> > GEM_BUG_ON(engine->retire);
> > }
> > -long intel_gt_retire_requests_timeout(struct intel_gt *gt, long timeout)
> > +long intel_gt_retire_requests_timeout(struct intel_gt *gt, long timeout,
> > + long *rtimeout)
>
> What is 'rtimeout', I know remaining, but it can be more self-descriptive to
> start with.
>
'remaining_timeout' it is.
> It feels a bit churny for what it is. How plausible would be alternatives to
> either change existing timeout to in/out, or measure sleep internally in
> this function, or just risk sleeping twice as long by passing the original
> timeout to uc idle as well?
>
Originally had it just passing in the same value, got review feedback
saying
Re: [Intel-gfx] [RFC PATCH 55/97] drm/i915/guc: Update intel_gt_wait_for_idle to work with GuC
On 06/05/2021 20:14, Matthew Brost wrote:
When running the GuC the GPU can't be considered idle if the GuC still
has contexts pinned. As such, a call has been added in
intel_gt_wait_for_idle to idle the UC and in turn the GuC by waiting for
the number of unpinned contexts to go to zero.
Cc: John Harrison
Signed-off-by: Matthew Brost
---
drivers/gpu/drm/i915/gem/i915_gem_mman.c | 3 +-
drivers/gpu/drm/i915/gt/intel_gt.c| 18
drivers/gpu/drm/i915/gt/intel_gt.h| 2 +
drivers/gpu/drm/i915/gt/intel_gt_requests.c | 22 ++---
drivers/gpu/drm/i915/gt/intel_gt_requests.h | 7 +-
drivers/gpu/drm/i915/gt/uc/intel_guc.h| 4 +
drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c | 1 +
drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h | 4 +
.../gpu/drm/i915/gt/uc/intel_guc_submission.c | 91 ++-
drivers/gpu/drm/i915/gt/uc/intel_uc.h | 5 +
drivers/gpu/drm/i915/i915_debugfs.c | 1 +
drivers/gpu/drm/i915/i915_gem_evict.c | 1 +
.../gpu/drm/i915/selftests/igt_live_test.c| 2 +-
.../gpu/drm/i915/selftests/mock_gem_device.c | 3 +-
14 files changed, 137 insertions(+), 27 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
index 8598a1c78a4c..2f5295c9408d 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
@@ -634,7 +634,8 @@ mmap_offset_attach(struct drm_i915_gem_object *obj,
goto insert;
/* Attempt to reap some mmap space from dead objects */
- err = intel_gt_retire_requests_timeout(>gt, MAX_SCHEDULE_TIMEOUT);
+ err = intel_gt_retire_requests_timeout(>gt, MAX_SCHEDULE_TIMEOUT,
+ NULL);
if (err)
goto err;
diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c b/drivers/gpu/drm/i915/gt/intel_gt.c
index 8d77dcbad059..1742a8561f69 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt.c
+++ b/drivers/gpu/drm/i915/gt/intel_gt.c
@@ -574,6 +574,24 @@ static void __intel_gt_disable(struct intel_gt *gt)
GEM_BUG_ON(intel_gt_pm_is_awake(gt));
}
+int intel_gt_wait_for_idle(struct intel_gt *gt, long timeout)
+{
+ long rtimeout;
+
+ /* If the device is asleep, we have no requests outstanding */
+ if (!intel_gt_pm_is_awake(gt))
+ return 0;
+
+ while ((timeout = intel_gt_retire_requests_timeout(gt, timeout,
+ )) > 0) {
+ cond_resched();
+ if (signal_pending(current))
+ return -EINTR;
+ }
+
+ return timeout ? timeout : intel_uc_wait_for_idle(>uc, rtimeout);
+}
+
int intel_gt_init(struct intel_gt *gt)
{
int err;
diff --git a/drivers/gpu/drm/i915/gt/intel_gt.h
b/drivers/gpu/drm/i915/gt/intel_gt.h
index 7ec395cace69..c775043334bf 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt.h
+++ b/drivers/gpu/drm/i915/gt/intel_gt.h
@@ -48,6 +48,8 @@ void intel_gt_driver_release(struct intel_gt *gt);
void intel_gt_driver_late_release(struct intel_gt *gt);
+int intel_gt_wait_for_idle(struct intel_gt *gt, long timeout);
+
void intel_gt_check_and_clear_faults(struct intel_gt *gt);
void intel_gt_clear_error_registers(struct intel_gt *gt,
intel_engine_mask_t engine_mask);
diff --git a/drivers/gpu/drm/i915/gt/intel_gt_requests.c
b/drivers/gpu/drm/i915/gt/intel_gt_requests.c
index 647eca9d867a..c6c702f236fa 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt_requests.c
+++ b/drivers/gpu/drm/i915/gt/intel_gt_requests.c
@@ -13,6 +13,7 @@
#include "intel_gt_pm.h"
#include "intel_gt_requests.h"
#include "intel_timeline.h"
+#include "uc/intel_uc.h"
static bool retire_requests(struct intel_timeline *tl)
{
@@ -130,7 +131,8 @@ void intel_engine_fini_retire(struct intel_engine_cs
*engine)
GEM_BUG_ON(engine->retire);
}
-long intel_gt_retire_requests_timeout(struct intel_gt *gt, long timeout)
+long intel_gt_retire_requests_timeout(struct intel_gt *gt, long timeout,
+ long *rtimeout)
What is 'rtimeout', I know remaining, but it can be more
self-descriptive to start with.
It feels a bit churny for what it is. How plausible would be
alternatives to either change existing timeout to in/out, or measure
sleep internally in this function, or just risk sleeping twice as long
by passing the original timeout to uc idle as well?
{
struct intel_gt_timelines *timelines = >timelines;
struct intel_timeline *tl, *tn;
@@ -195,22 +197,10 @@ out_active: spin_lock(>lock);
if (flush_submission(gt, timeout)) /* Wait, there's more! */
active_count++;
- return active_count ? timeout : 0;
-}
-
-int intel_gt_wait_for_idle(struct intel_gt *gt, long timeout)
-{
- /* If the device is asleep, we have no requests outstanding */
- if
[Intel-gfx] [RFC PATCH 55/97] drm/i915/guc: Update intel_gt_wait_for_idle to work with GuC
When running the GuC the GPU can't be considered idle if the GuC still
has contexts pinned. As such, a call has been added in
intel_gt_wait_for_idle to idle the UC and in turn the GuC by waiting for
the number of unpinned contexts to go to zero.
Cc: John Harrison
Signed-off-by: Matthew Brost
---
drivers/gpu/drm/i915/gem/i915_gem_mman.c | 3 +-
drivers/gpu/drm/i915/gt/intel_gt.c| 18
drivers/gpu/drm/i915/gt/intel_gt.h| 2 +
drivers/gpu/drm/i915/gt/intel_gt_requests.c | 22 ++---
drivers/gpu/drm/i915/gt/intel_gt_requests.h | 7 +-
drivers/gpu/drm/i915/gt/uc/intel_guc.h| 4 +
drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c | 1 +
drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h | 4 +
.../gpu/drm/i915/gt/uc/intel_guc_submission.c | 91 ++-
drivers/gpu/drm/i915/gt/uc/intel_uc.h | 5 +
drivers/gpu/drm/i915/i915_debugfs.c | 1 +
drivers/gpu/drm/i915/i915_gem_evict.c | 1 +
.../gpu/drm/i915/selftests/igt_live_test.c| 2 +-
.../gpu/drm/i915/selftests/mock_gem_device.c | 3 +-
14 files changed, 137 insertions(+), 27 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
index 8598a1c78a4c..2f5295c9408d 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
@@ -634,7 +634,8 @@ mmap_offset_attach(struct drm_i915_gem_object *obj,
goto insert;
/* Attempt to reap some mmap space from dead objects */
- err = intel_gt_retire_requests_timeout(>gt, MAX_SCHEDULE_TIMEOUT);
+ err = intel_gt_retire_requests_timeout(>gt, MAX_SCHEDULE_TIMEOUT,
+ NULL);
if (err)
goto err;
diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c
b/drivers/gpu/drm/i915/gt/intel_gt.c
index 8d77dcbad059..1742a8561f69 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt.c
+++ b/drivers/gpu/drm/i915/gt/intel_gt.c
@@ -574,6 +574,24 @@ static void __intel_gt_disable(struct intel_gt *gt)
GEM_BUG_ON(intel_gt_pm_is_awake(gt));
}
+int intel_gt_wait_for_idle(struct intel_gt *gt, long timeout)
+{
+ long rtimeout;
+
+ /* If the device is asleep, we have no requests outstanding */
+ if (!intel_gt_pm_is_awake(gt))
+ return 0;
+
+ while ((timeout = intel_gt_retire_requests_timeout(gt, timeout,
+ )) > 0) {
+ cond_resched();
+ if (signal_pending(current))
+ return -EINTR;
+ }
+
+ return timeout ? timeout : intel_uc_wait_for_idle(>uc, rtimeout);
+}
+
int intel_gt_init(struct intel_gt *gt)
{
int err;
diff --git a/drivers/gpu/drm/i915/gt/intel_gt.h
b/drivers/gpu/drm/i915/gt/intel_gt.h
index 7ec395cace69..c775043334bf 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt.h
+++ b/drivers/gpu/drm/i915/gt/intel_gt.h
@@ -48,6 +48,8 @@ void intel_gt_driver_release(struct intel_gt *gt);
void intel_gt_driver_late_release(struct intel_gt *gt);
+int intel_gt_wait_for_idle(struct intel_gt *gt, long timeout);
+
void intel_gt_check_and_clear_faults(struct intel_gt *gt);
void intel_gt_clear_error_registers(struct intel_gt *gt,
intel_engine_mask_t engine_mask);
diff --git a/drivers/gpu/drm/i915/gt/intel_gt_requests.c
b/drivers/gpu/drm/i915/gt/intel_gt_requests.c
index 647eca9d867a..c6c702f236fa 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt_requests.c
+++ b/drivers/gpu/drm/i915/gt/intel_gt_requests.c
@@ -13,6 +13,7 @@
#include "intel_gt_pm.h"
#include "intel_gt_requests.h"
#include "intel_timeline.h"
+#include "uc/intel_uc.h"
static bool retire_requests(struct intel_timeline *tl)
{
@@ -130,7 +131,8 @@ void intel_engine_fini_retire(struct intel_engine_cs
*engine)
GEM_BUG_ON(engine->retire);
}
-long intel_gt_retire_requests_timeout(struct intel_gt *gt, long timeout)
+long intel_gt_retire_requests_timeout(struct intel_gt *gt, long timeout,
+ long *rtimeout)
{
struct intel_gt_timelines *timelines = >timelines;
struct intel_timeline *tl, *tn;
@@ -195,22 +197,10 @@ out_active: spin_lock(>lock);
if (flush_submission(gt, timeout)) /* Wait, there's more! */
active_count++;
- return active_count ? timeout : 0;
-}
-
-int intel_gt_wait_for_idle(struct intel_gt *gt, long timeout)
-{
- /* If the device is asleep, we have no requests outstanding */
- if (!intel_gt_pm_is_awake(gt))
- return 0;
-
- while ((timeout = intel_gt_retire_requests_timeout(gt, timeout)) > 0) {
- cond_resched();
- if (signal_pending(current))
- return -EINTR;
- }
+ if (rtimeout)
+ *rtimeout = timeout;
- return timeout;
+ return active_count ? timeout : 0;
}
static void
