Re: [Intel-gfx] [PATCH 6/9] drm/i915: Disable waitboosting for fence_wait()
On pe, 2016-07-15 at 12:49 +0100, Chris Wilson wrote:
> On Fri, Jul 15, 2016 at 12:08:05PM +0100, Chris Wilson wrote:
> >
> > On Fri, Jul 15, 2016 at 01:47:34PM +0300, Joonas Lahtinen wrote:
> > >
> > > On pe, 2016-07-15 at 11:11 +0100, Chris Wilson wrote:
> > > >
> > > > +#define NO_WAITBOOST ERR_PTR(-1)
> > > This does not seem to be a very common idiom in kernel, just add flags
> > > maybe.
> > I find the special values for the client (including the existing NULL
> > client) much more informative.
> Is it more pleasant with...?
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_request.c
> b/drivers/gpu/drm/i915/i915_gem_request.c
> index bfec4a88707d..aedc15d46463 100644
> --- a/drivers/gpu/drm/i915/i915_gem_request.c
> +++ b/drivers/gpu/drm/i915/i915_gem_request.c
> @@ -713,7 +713,7 @@ complete:
> *timeout = 0;
> }
>
> - if (!IS_ERR_OR_NULL(rps) &&
> + if (IS_RPS_CLIENT(rps) &&
> req->fence.seqno == req->engine->last_submitted_seqno) {
> /* The GPU is now idle and this client has stalled.
> * Since no other client has submitted a request in the
> diff --git a/drivers/gpu/drm/i915/i915_gem_request.h
> b/drivers/gpu/drm/i915/i915_gem_request.h
> index f7f497a07893..b3fe80d4eb63 100644
> --- a/drivers/gpu/drm/i915/i915_gem_request.h
> +++ b/drivers/gpu/drm/i915/i915_gem_request.h
> @@ -207,6 +207,7 @@ void __i915_add_request(struct drm_i915_gem_request *req,
>
> struct intel_rps_client;
I almost suggested #define RPS_CLIENT(n) ERR_PTR(-n), but this is
somewhat better already.
> #define NO_WAITBOOST ERR_PTR(-1)
> +#define IS_RPS_CLIENT(p) (!IS_ERR_OR_NULL(p))
>
--
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 6/9] drm/i915: Disable waitboosting for fence_wait()
On Fri, Jul 15, 2016 at 12:08:05PM +0100, Chris Wilson wrote:
> On Fri, Jul 15, 2016 at 01:47:34PM +0300, Joonas Lahtinen wrote:
> > On pe, 2016-07-15 at 11:11 +0100, Chris Wilson wrote:
> > > +#define NO_WAITBOOST ERR_PTR(-1)
> >
> > This does not seem to be a very common idiom in kernel, just add flags
> > maybe.
>
> I find the special values for the client (including the existing NULL
> client) much more informative.
Is it more pleasant with...?
diff --git a/drivers/gpu/drm/i915/i915_gem_request.c
b/drivers/gpu/drm/i915/i915_gem_request.c
index bfec4a88707d..aedc15d46463 100644
--- a/drivers/gpu/drm/i915/i915_gem_request.c
+++ b/drivers/gpu/drm/i915/i915_gem_request.c
@@ -713,7 +713,7 @@ complete:
*timeout = 0;
}
- if (!IS_ERR_OR_NULL(rps) &&
+ if (IS_RPS_CLIENT(rps) &&
req->fence.seqno == req->engine->last_submitted_seqno) {
/* The GPU is now idle and this client has stalled.
* Since no other client has submitted a request in the
diff --git a/drivers/gpu/drm/i915/i915_gem_request.h
b/drivers/gpu/drm/i915/i915_gem_request.h
index f7f497a07893..b3fe80d4eb63 100644
--- a/drivers/gpu/drm/i915/i915_gem_request.h
+++ b/drivers/gpu/drm/i915/i915_gem_request.h
@@ -207,6 +207,7 @@ void __i915_add_request(struct drm_i915_gem_request *req,
struct intel_rps_client;
#define NO_WAITBOOST ERR_PTR(-1)
+#define IS_RPS_CLIENT(p) (!IS_ERR_OR_NULL(p))
--
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 6/9] drm/i915: Disable waitboosting for fence_wait()
On Fri, Jul 15, 2016 at 01:47:34PM +0300, Joonas Lahtinen wrote: > On pe, 2016-07-15 at 11:11 +0100, Chris Wilson wrote: > > +#define NO_WAITBOOST ERR_PTR(-1) > > This does not seem to be a very common idiom in kernel, just add flags > maybe. I find the special values for the client (including the existing NULL client) much more informative. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 6/9] drm/i915: Disable waitboosting for fence_wait()
On Fri, Jul 15, 2016 at 01:49:44PM +0300, Mika Kuoppala wrote: > Chris Wilson writes: > > > We want to restrict waitboosting to known process contexts, where we can > > track which clients are receiving waitboosts and prevent excessive power > > wasting. For fence_wait() we do not have any client tracking and so that > > leaves it open to abuse. > > > > Signed-off-by: Chris Wilson > > --- > > drivers/gpu/drm/i915/i915_gem_request.c | 7 --- > > drivers/gpu/drm/i915/i915_gem_request.h | 1 + > > 2 files changed, 5 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_gem_request.c > > b/drivers/gpu/drm/i915/i915_gem_request.c > > index 6528536878f2..bfec4a88707d 100644 > > --- a/drivers/gpu/drm/i915/i915_gem_request.c > > +++ b/drivers/gpu/drm/i915/i915_gem_request.c > > @@ -70,7 +70,7 @@ static signed long i915_fence_wait(struct fence *fence, > > > > ret = __i915_wait_request(to_request(fence), > > interruptible, timeout, > > - NULL); > > + NO_WAITBOOST); > > if (ret == -ETIME) > > return 0; > > > > @@ -642,7 +642,7 @@ int __i915_wait_request(struct drm_i915_gem_request > > *req, > > * forcing the clocks too high for the whole system, we only allow > > * each client to waitboost once in a busy period. > > */ > > - if (INTEL_GEN(req->i915) >= 6) > > + if (!IS_ERR(rps) && INTEL_GEN(req->i915) >= 6) > > gen6_rps_boost(req->i915, rps, req->emitted_jiffies); > > > > The intention here is to boost even if rps client is NULL? > Assuming so could you elaborate why. Yes, NULL is the kernel request for rps boosting, not associated with any client (and may be affecting multiple clients at any one time). -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 6/9] drm/i915: Disable waitboosting for fence_wait()
Chris Wilson writes:
> We want to restrict waitboosting to known process contexts, where we can
> track which clients are receiving waitboosts and prevent excessive power
> wasting. For fence_wait() we do not have any client tracking and so that
> leaves it open to abuse.
>
> Signed-off-by: Chris Wilson
> ---
> drivers/gpu/drm/i915/i915_gem_request.c | 7 ---
> drivers/gpu/drm/i915/i915_gem_request.h | 1 +
> 2 files changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_request.c
> b/drivers/gpu/drm/i915/i915_gem_request.c
> index 6528536878f2..bfec4a88707d 100644
> --- a/drivers/gpu/drm/i915/i915_gem_request.c
> +++ b/drivers/gpu/drm/i915/i915_gem_request.c
> @@ -70,7 +70,7 @@ static signed long i915_fence_wait(struct fence *fence,
>
> ret = __i915_wait_request(to_request(fence),
> interruptible, timeout,
> - NULL);
> + NO_WAITBOOST);
> if (ret == -ETIME)
> return 0;
>
> @@ -642,7 +642,7 @@ int __i915_wait_request(struct drm_i915_gem_request *req,
>* forcing the clocks too high for the whole system, we only allow
>* each client to waitboost once in a busy period.
>*/
> - if (INTEL_GEN(req->i915) >= 6)
> + if (!IS_ERR(rps) && INTEL_GEN(req->i915) >= 6)
> gen6_rps_boost(req->i915, rps, req->emitted_jiffies);
>
The intention here is to boost even if rps client is NULL?
Assuming so could you elaborate why.
Thanks,
-Mika
> /* Optimistic spin for the next ~jiffie before touching IRQs */
> @@ -713,7 +713,8 @@ complete:
> *timeout = 0;
> }
>
> - if (rps && req->fence.seqno == req->engine->last_submitted_seqno) {
> + if (!IS_ERR_OR_NULL(rps) &&
> + req->fence.seqno == req->engine->last_submitted_seqno) {
> /* The GPU is now idle and this client has stalled.
>* Since no other client has submitted a request in the
>* meantime, assume that this client is the only one
> diff --git a/drivers/gpu/drm/i915/i915_gem_request.h
> b/drivers/gpu/drm/i915/i915_gem_request.h
> index 6f2c820785f3..f7f497a07893 100644
> --- a/drivers/gpu/drm/i915/i915_gem_request.h
> +++ b/drivers/gpu/drm/i915/i915_gem_request.h
> @@ -206,6 +206,7 @@ void __i915_add_request(struct drm_i915_gem_request *req,
> __i915_add_request(req, NULL, false)
>
> struct intel_rps_client;
> +#define NO_WAITBOOST ERR_PTR(-1)
>
> int __i915_wait_request(struct drm_i915_gem_request *req,
> bool interruptible,
> --
> 2.8.1
>
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 6/9] drm/i915: Disable waitboosting for fence_wait()
On pe, 2016-07-15 at 11:11 +0100, Chris Wilson wrote: > +#define NO_WAITBOOST ERR_PTR(-1) This does not seem to be a very common idiom in kernel, just add flags maybe. Regards, Joonas -- Joonas Lahtinen Open Source Technology Center Intel Corporation ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 6/9] drm/i915: Disable waitboosting for fence_wait()
We want to restrict waitboosting to known process contexts, where we can
track which clients are receiving waitboosts and prevent excessive power
wasting. For fence_wait() we do not have any client tracking and so that
leaves it open to abuse.
Signed-off-by: Chris Wilson
---
drivers/gpu/drm/i915/i915_gem_request.c | 7 ---
drivers/gpu/drm/i915/i915_gem_request.h | 1 +
2 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem_request.c
b/drivers/gpu/drm/i915/i915_gem_request.c
index 6528536878f2..bfec4a88707d 100644
--- a/drivers/gpu/drm/i915/i915_gem_request.c
+++ b/drivers/gpu/drm/i915/i915_gem_request.c
@@ -70,7 +70,7 @@ static signed long i915_fence_wait(struct fence *fence,
ret = __i915_wait_request(to_request(fence),
interruptible, timeout,
- NULL);
+ NO_WAITBOOST);
if (ret == -ETIME)
return 0;
@@ -642,7 +642,7 @@ int __i915_wait_request(struct drm_i915_gem_request *req,
* forcing the clocks too high for the whole system, we only allow
* each client to waitboost once in a busy period.
*/
- if (INTEL_GEN(req->i915) >= 6)
+ if (!IS_ERR(rps) && INTEL_GEN(req->i915) >= 6)
gen6_rps_boost(req->i915, rps, req->emitted_jiffies);
/* Optimistic spin for the next ~jiffie before touching IRQs */
@@ -713,7 +713,8 @@ complete:
*timeout = 0;
}
- if (rps && req->fence.seqno == req->engine->last_submitted_seqno) {
+ if (!IS_ERR_OR_NULL(rps) &&
+ req->fence.seqno == req->engine->last_submitted_seqno) {
/* The GPU is now idle and this client has stalled.
* Since no other client has submitted a request in the
* meantime, assume that this client is the only one
diff --git a/drivers/gpu/drm/i915/i915_gem_request.h
b/drivers/gpu/drm/i915/i915_gem_request.h
index 6f2c820785f3..f7f497a07893 100644
--- a/drivers/gpu/drm/i915/i915_gem_request.h
+++ b/drivers/gpu/drm/i915/i915_gem_request.h
@@ -206,6 +206,7 @@ void __i915_add_request(struct drm_i915_gem_request *req,
__i915_add_request(req, NULL, false)
struct intel_rps_client;
+#define NO_WAITBOOST ERR_PTR(-1)
int __i915_wait_request(struct drm_i915_gem_request *req,
bool interruptible,
--
2.8.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
