Re: [Intel-gfx] [PATCH] drm/i915/gem: Allow users to disable waitboost

2023-10-26 Thread kernel test robot 



Hello,

kernel test robot noticed "assertion_failure" on:

commit: 54fef7ea35dadd66193b98805b0bc42ef2b279db ("[PATCH] drm/i915/gem: Allow 
users to disable waitboost")
url: 
https://github.com/intel-lab-lkp/linux/commits/Vinay-Belgaumkar/drm-i915-gem-Allow-users-to-disable-waitboost/20230921-060357
base: git://anongit.freedesktop.org/drm-intel for-linux-next
patch link: 
https://lore.kernel.org/all/20230920215624.3482244-1-vinay.belgaum...@intel.com/
patch subject: [PATCH] drm/i915/gem: Allow users to disable waitboost

in testcase: igt
version: igt-x86_64-0f075441-1_20230520
with following parameters:

group: group-23



compiler: gcc-12
test machine: 20 threads 1 sockets (Commet Lake) with 16G memory

(please refer to attached dmesg/kmsg for entire log/backtrace)



If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot 
| Closes: 
https://lore.kernel.org/oe-lkp/202310271326.abb748d0-oliver.s...@intel.com



user  :notice: [   42.847071] Starting subtest: engines-cleanup

user  :notice: [   42.854776] Starting dynamic subtest: rcs0

user  :notice: [   42.863938] (gem_ctx_persistence:833) CRITICAL: Test 
assertion failure function test_nonpersistent_cleanup, file 
../tests/i915/gem_ctx_persistence.c:283:

user  :notice: [   42.882029] (gem_ctx_persistence:833) CRITICAL: Failed 
assertion: gem_wait(i915, spin->handle, ) == 0

user  :notice: [   42.895541] (gem_ctx_persistence:833) CRITICAL: error: -22 != 0



The kernel config and materials to reproduce are available at:
https://download.01.org/0day-ci/archive/20231027/202310271326.abb748d0-oliver.s...@intel.com



-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [Intel-gfx] [PATCH] drm/i915/gem: Allow users to disable waitboost

2023-10-16 Thread Rodrigo Vivi 
On Mon, Oct 16, 2023 at 09:02:38AM +0100, Tvrtko Ursulin wrote:
> 
> On 13/10/2023 21:51, Rodrigo Vivi wrote:
> > On Thu, Sep 28, 2023 at 01:48:34PM +0100, Tvrtko Ursulin wrote:
> > > 
> > > On 27/09/2023 20:34, Belgaumkar, Vinay wrote:
> > > > 
> > > > On 9/21/2023 3:41 AM, Tvrtko Ursulin wrote:
> > > > > 
> > > > > On 20/09/2023 22:56, Vinay Belgaumkar wrote:
> > > > > > Provide a bit to disable waitboost while waiting on a gem object.
> > > > > > Waitboost results in increased power consumption by requesting RP0
> > > > > > while waiting for the request to complete. Add a bit in the 
> > > > > > gem_wait()
> > > > > > IOCTL where this can be disabled.
> > > > > > 
> > > > > > This is related to the libva API change here -
> > > > > > Link: 
> > > > > > https://github.com/XinfengZhang/libva/commit/3d90d18c67609a73121bb71b20ee4776b54b61a7
> > > > > 
> > > > > This link does not appear to lead to userspace code using this uapi?
> > > > We have asked Carl (cc'd) to post a patch for the same.
> > > 
> > > Ack.
> > 
> > I'm glad to see that we will have the end-to-end flow of the high-level API.
> > 
> > > 
> > > > > > Cc: Rodrigo Vivi 
> > > > > > Signed-off-by: Vinay Belgaumkar 
> > > > > > ---
> > > > > >    drivers/gpu/drm/i915/gem/i915_gem_wait.c | 9 ++---
> > > > > >    drivers/gpu/drm/i915/i915_request.c  | 3 ++-
> > > > > >    drivers/gpu/drm/i915/i915_request.h  | 1 +
> > > > > >    include/uapi/drm/i915_drm.h  | 1 +
> > > > > >    4 files changed, 10 insertions(+), 4 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_wait.c
> > > > > > b/drivers/gpu/drm/i915/gem/i915_gem_wait.c
> > > > > > index d4b918fb11ce..955885ec859d 100644
> > > > > > --- a/drivers/gpu/drm/i915/gem/i915_gem_wait.c
> > > > > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_wait.c
> > > > > > @@ -72,7 +72,8 @@ i915_gem_object_wait_reservation(struct
> > > > > > dma_resv *resv,
> > > > > >    struct dma_fence *fence;
> > > > > >    long ret = timeout ?: 1;
> > > > > >    -    i915_gem_object_boost(resv, flags);
> > > > > > +    if (!(flags & I915_WAITBOOST_DISABLE))
> > > > > > +    i915_gem_object_boost(resv, flags);
> > > > > >      dma_resv_iter_begin(, resv,
> > > > > >    dma_resv_usage_rw(flags & I915_WAIT_ALL));
> > > > > > @@ -236,7 +237,7 @@ i915_gem_wait_ioctl(struct drm_device *dev,
> > > > > > void *data, struct drm_file *file)
> > > > > >    ktime_t start;
> > > > > >    long ret;
> > > > > >    -    if (args->flags != 0)
> > > > > > +    if (args->flags != 0 || args->flags != 
> > > > > > I915_GEM_WAITBOOST_DISABLE)
> > > > > >    return -EINVAL;
> > > > > >      obj = i915_gem_object_lookup(file, args->bo_handle);
> > > > > > @@ -248,7 +249,9 @@ i915_gem_wait_ioctl(struct drm_device *dev,
> > > > > > void *data, struct drm_file *file)
> > > > > >    ret = i915_gem_object_wait(obj,
> > > > > >   I915_WAIT_INTERRUPTIBLE |
> > > > > >   I915_WAIT_PRIORITY |
> > > > > > -   I915_WAIT_ALL,
> > > > > > +   I915_WAIT_ALL |
> > > > > > +   (args->flags & I915_GEM_WAITBOOST_DISABLE ?
> > > > > > +    I915_WAITBOOST_DISABLE : 0),
> > > > > >   to_wait_timeout(args->timeout_ns));
> > > > > >      if (args->timeout_ns > 0) {
> > > > > > diff --git a/drivers/gpu/drm/i915/i915_request.c
> > > > > > b/drivers/gpu/drm/i915/i915_request.c
> > > > > > index f59081066a19..2957409b4b2a 100644
> > > > > > --- a/drivers/gpu/drm/i915/i915_request.c
> > > > > > +++ b/drivers/gpu/drm/i915/i915_request.c
> > > > > > @@ -2044,7 +2044,8 @@ long i915_request_wait_timeout(struct
> > > > > > i915_request *rq,
> > > > > >     * but at a cost of spending more power processing the 
> > > > > > workload
> > > > > >     * (bad for battery).
> > > > > >     */
> > > > > > -    if (flags & I915_WAIT_PRIORITY && !i915_request_started(rq))
> > > > > > +    if (!(flags & I915_WAITBOOST_DISABLE) && (flags &
> > > > > > I915_WAIT_PRIORITY) &&
> > > > > > +    !i915_request_started(rq))
> > > > > >    intel_rps_boost(rq);
> > > > > >      wait.tsk = current;
> > > > > > diff --git a/drivers/gpu/drm/i915/i915_request.h
> > > > > > b/drivers/gpu/drm/i915/i915_request.h
> > > > > > index 0ac55b2e4223..3cc00e8254dc 100644
> > > > > > --- a/drivers/gpu/drm/i915/i915_request.h
> > > > > > +++ b/drivers/gpu/drm/i915/i915_request.h
> > > > > > @@ -445,6 +445,7 @@ long i915_request_wait(struct i915_request *rq,
> > > > > >    #define I915_WAIT_INTERRUPTIBLE    BIT(0)
> > > > > >    #define I915_WAIT_PRIORITY    BIT(1) /* small priority bump
> > > > > > for the request */
> > > > > >    #define I915_WAIT_ALL    BIT(2) /* used by
> > > > > > i915_gem_object_wait() */
> > > > > > +#define I915_WAITBOOST_DISABLE    BIT(3) /* used by
> > 
> > maybe name it I915_WAIT_NO_BOOST to align a bit better with the

Re: [Intel-gfx] [PATCH] drm/i915/gem: Allow users to disable waitboost

2023-10-16 Thread Tvrtko Ursulin 



On 13/10/2023 21:51, Rodrigo Vivi wrote:

On Thu, Sep 28, 2023 at 01:48:34PM +0100, Tvrtko Ursulin wrote:


On 27/09/2023 20:34, Belgaumkar, Vinay wrote:


On 9/21/2023 3:41 AM, Tvrtko Ursulin wrote:


On 20/09/2023 22:56, Vinay Belgaumkar wrote:

Provide a bit to disable waitboost while waiting on a gem object.
Waitboost results in increased power consumption by requesting RP0
while waiting for the request to complete. Add a bit in the gem_wait()
IOCTL where this can be disabled.

This is related to the libva API change here -
Link: 
https://github.com/XinfengZhang/libva/commit/3d90d18c67609a73121bb71b20ee4776b54b61a7


This link does not appear to lead to userspace code using this uapi?

We have asked Carl (cc'd) to post a patch for the same.


Ack.


I'm glad to see that we will have the end-to-end flow of the high-level API.




Cc: Rodrigo Vivi 
Signed-off-by: Vinay Belgaumkar 
---
   drivers/gpu/drm/i915/gem/i915_gem_wait.c | 9 ++---
   drivers/gpu/drm/i915/i915_request.c  | 3 ++-
   drivers/gpu/drm/i915/i915_request.h  | 1 +
   include/uapi/drm/i915_drm.h  | 1 +
   4 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_wait.c
b/drivers/gpu/drm/i915/gem/i915_gem_wait.c
index d4b918fb11ce..955885ec859d 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_wait.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_wait.c
@@ -72,7 +72,8 @@ i915_gem_object_wait_reservation(struct
dma_resv *resv,
   struct dma_fence *fence;
   long ret = timeout ?: 1;
   -    i915_gem_object_boost(resv, flags);
+    if (!(flags & I915_WAITBOOST_DISABLE))
+    i915_gem_object_boost(resv, flags);
     dma_resv_iter_begin(, resv,
   dma_resv_usage_rw(flags & I915_WAIT_ALL));
@@ -236,7 +237,7 @@ i915_gem_wait_ioctl(struct drm_device *dev,
void *data, struct drm_file *file)
   ktime_t start;
   long ret;
   -    if (args->flags != 0)
+    if (args->flags != 0 || args->flags != I915_GEM_WAITBOOST_DISABLE)
   return -EINVAL;
     obj = i915_gem_object_lookup(file, args->bo_handle);
@@ -248,7 +249,9 @@ i915_gem_wait_ioctl(struct drm_device *dev,
void *data, struct drm_file *file)
   ret = i915_gem_object_wait(obj,
  I915_WAIT_INTERRUPTIBLE |
  I915_WAIT_PRIORITY |
-   I915_WAIT_ALL,
+   I915_WAIT_ALL |
+   (args->flags & I915_GEM_WAITBOOST_DISABLE ?
+    I915_WAITBOOST_DISABLE : 0),
  to_wait_timeout(args->timeout_ns));
     if (args->timeout_ns > 0) {
diff --git a/drivers/gpu/drm/i915/i915_request.c
b/drivers/gpu/drm/i915/i915_request.c
index f59081066a19..2957409b4b2a 100644
--- a/drivers/gpu/drm/i915/i915_request.c
+++ b/drivers/gpu/drm/i915/i915_request.c
@@ -2044,7 +2044,8 @@ long i915_request_wait_timeout(struct
i915_request *rq,
    * but at a cost of spending more power processing the workload
    * (bad for battery).
    */
-    if (flags & I915_WAIT_PRIORITY && !i915_request_started(rq))
+    if (!(flags & I915_WAITBOOST_DISABLE) && (flags &
I915_WAIT_PRIORITY) &&
+    !i915_request_started(rq))
   intel_rps_boost(rq);
     wait.tsk = current;
diff --git a/drivers/gpu/drm/i915/i915_request.h
b/drivers/gpu/drm/i915/i915_request.h
index 0ac55b2e4223..3cc00e8254dc 100644
--- a/drivers/gpu/drm/i915/i915_request.h
+++ b/drivers/gpu/drm/i915/i915_request.h
@@ -445,6 +445,7 @@ long i915_request_wait(struct i915_request *rq,
   #define I915_WAIT_INTERRUPTIBLE    BIT(0)
   #define I915_WAIT_PRIORITY    BIT(1) /* small priority bump
for the request */
   #define I915_WAIT_ALL    BIT(2) /* used by
i915_gem_object_wait() */
+#define I915_WAITBOOST_DISABLE    BIT(3) /* used by


maybe name it I915_WAIT_NO_BOOST to align a bit better with the existent ones?


I thought it would be better to not mention wait boost in the uapi, but 
leave it as implementation detail.


My suggestion was along the lines of I915_GEM_WAIT_BACKGROUND/IDLE.

In essence saying allowing userspace to say this is not an important 
wait. Yes, it implies that other waits are (more) important, but I think 
this is still better than starting to mention wait boost in the uapi. 
Since that would kind of cement it exists, while we always just viewed 
it as an "go faster" driver internal heuristics and could freely decide 
not to employ it even for default waits.


Historically even we had a period when waits were getting elevated 
scheduling priority. We removed it, would have to dig through git and 
email history to remember exactly why, but probably along the lines that 
it is not always justified. Same as waitboost is not always justified 
and can be harmful.


So I think a generic name for the uapi leaves more freedom for the 
driver. Might be a wrong name that I am suggesting and should be 
something else, not sure.


[Comes back later.]

eec39e441c29 ("drm/i915: Remove wait priority boosting")

So

Re: [Intel-gfx] [PATCH] drm/i915/gem: Allow users to disable waitboost

2023-10-13 Thread Rodrigo Vivi 
On Thu, Sep 28, 2023 at 01:48:34PM +0100, Tvrtko Ursulin wrote:
> 
> On 27/09/2023 20:34, Belgaumkar, Vinay wrote:
> > 
> > On 9/21/2023 3:41 AM, Tvrtko Ursulin wrote:
> > > 
> > > On 20/09/2023 22:56, Vinay Belgaumkar wrote:
> > > > Provide a bit to disable waitboost while waiting on a gem object.
> > > > Waitboost results in increased power consumption by requesting RP0
> > > > while waiting for the request to complete. Add a bit in the gem_wait()
> > > > IOCTL where this can be disabled.
> > > > 
> > > > This is related to the libva API change here -
> > > > Link: 
> > > > https://github.com/XinfengZhang/libva/commit/3d90d18c67609a73121bb71b20ee4776b54b61a7
> > > 
> > > This link does not appear to lead to userspace code using this uapi?
> > We have asked Carl (cc'd) to post a patch for the same.
> 
> Ack.

I'm glad to see that we will have the end-to-end flow of the high-level API.

> 
> > > > Cc: Rodrigo Vivi 
> > > > Signed-off-by: Vinay Belgaumkar 
> > > > ---
> > > >   drivers/gpu/drm/i915/gem/i915_gem_wait.c | 9 ++---
> > > >   drivers/gpu/drm/i915/i915_request.c  | 3 ++-
> > > >   drivers/gpu/drm/i915/i915_request.h  | 1 +
> > > >   include/uapi/drm/i915_drm.h  | 1 +
> > > >   4 files changed, 10 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_wait.c
> > > > b/drivers/gpu/drm/i915/gem/i915_gem_wait.c
> > > > index d4b918fb11ce..955885ec859d 100644
> > > > --- a/drivers/gpu/drm/i915/gem/i915_gem_wait.c
> > > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_wait.c
> > > > @@ -72,7 +72,8 @@ i915_gem_object_wait_reservation(struct
> > > > dma_resv *resv,
> > > >   struct dma_fence *fence;
> > > >   long ret = timeout ?: 1;
> > > >   -    i915_gem_object_boost(resv, flags);
> > > > +    if (!(flags & I915_WAITBOOST_DISABLE))
> > > > +    i915_gem_object_boost(resv, flags);
> > > >     dma_resv_iter_begin(, resv,
> > > >   dma_resv_usage_rw(flags & I915_WAIT_ALL));
> > > > @@ -236,7 +237,7 @@ i915_gem_wait_ioctl(struct drm_device *dev,
> > > > void *data, struct drm_file *file)
> > > >   ktime_t start;
> > > >   long ret;
> > > >   -    if (args->flags != 0)
> > > > +    if (args->flags != 0 || args->flags != I915_GEM_WAITBOOST_DISABLE)
> > > >   return -EINVAL;
> > > >     obj = i915_gem_object_lookup(file, args->bo_handle);
> > > > @@ -248,7 +249,9 @@ i915_gem_wait_ioctl(struct drm_device *dev,
> > > > void *data, struct drm_file *file)
> > > >   ret = i915_gem_object_wait(obj,
> > > >  I915_WAIT_INTERRUPTIBLE |
> > > >  I915_WAIT_PRIORITY |
> > > > -   I915_WAIT_ALL,
> > > > +   I915_WAIT_ALL |
> > > > +   (args->flags & I915_GEM_WAITBOOST_DISABLE ?
> > > > +    I915_WAITBOOST_DISABLE : 0),
> > > >  to_wait_timeout(args->timeout_ns));
> > > >     if (args->timeout_ns > 0) {
> > > > diff --git a/drivers/gpu/drm/i915/i915_request.c
> > > > b/drivers/gpu/drm/i915/i915_request.c
> > > > index f59081066a19..2957409b4b2a 100644
> > > > --- a/drivers/gpu/drm/i915/i915_request.c
> > > > +++ b/drivers/gpu/drm/i915/i915_request.c
> > > > @@ -2044,7 +2044,8 @@ long i915_request_wait_timeout(struct
> > > > i915_request *rq,
> > > >    * but at a cost of spending more power processing the workload
> > > >    * (bad for battery).
> > > >    */
> > > > -    if (flags & I915_WAIT_PRIORITY && !i915_request_started(rq))
> > > > +    if (!(flags & I915_WAITBOOST_DISABLE) && (flags &
> > > > I915_WAIT_PRIORITY) &&
> > > > +    !i915_request_started(rq))
> > > >   intel_rps_boost(rq);
> > > >     wait.tsk = current;
> > > > diff --git a/drivers/gpu/drm/i915/i915_request.h
> > > > b/drivers/gpu/drm/i915/i915_request.h
> > > > index 0ac55b2e4223..3cc00e8254dc 100644
> > > > --- a/drivers/gpu/drm/i915/i915_request.h
> > > > +++ b/drivers/gpu/drm/i915/i915_request.h
> > > > @@ -445,6 +445,7 @@ long i915_request_wait(struct i915_request *rq,
> > > >   #define I915_WAIT_INTERRUPTIBLE    BIT(0)
> > > >   #define I915_WAIT_PRIORITY    BIT(1) /* small priority bump
> > > > for the request */
> > > >   #define I915_WAIT_ALL    BIT(2) /* used by
> > > > i915_gem_object_wait() */
> > > > +#define I915_WAITBOOST_DISABLE    BIT(3) /* used by

maybe name it I915_WAIT_NO_BOOST to align a bit better with the existent ones?

> > > > i915_gem_object_wait() */
> > > >     void i915_request_show(struct drm_printer *m,
> > > >  const struct i915_request *rq,
> > > > diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> > > > index 7000e5910a1d..4adee70e39cf 100644
> > > > --- a/include/uapi/drm/i915_drm.h
> > > > +++ b/include/uapi/drm/i915_drm.h
> > > > @@ -1928,6 +1928,7 @@ struct drm_i915_gem_wait {
> > > >   /** Handle of BO we shall wait on */
> > > >   __u32 bo_handle;
> > > >   __u32 flags;
> > > > +#define

Re: [Intel-gfx] [PATCH] drm/i915/gem: Allow users to disable waitboost

2023-09-28 Thread Tvrtko Ursulin 



On 27/09/2023 20:34, Belgaumkar, Vinay wrote:


On 9/21/2023 3:41 AM, Tvrtko Ursulin wrote:


On 20/09/2023 22:56, Vinay Belgaumkar wrote:

Provide a bit to disable waitboost while waiting on a gem object.
Waitboost results in increased power consumption by requesting RP0
while waiting for the request to complete. Add a bit in the gem_wait()
IOCTL where this can be disabled.

This is related to the libva API change here -
Link: 
https://github.com/XinfengZhang/libva/commit/3d90d18c67609a73121bb71b20ee4776b54b61a7


This link does not appear to lead to userspace code using this uapi?

We have asked Carl (cc'd) to post a patch for the same.


Ack.


Cc: Rodrigo Vivi 
Signed-off-by: Vinay Belgaumkar 
---
  drivers/gpu/drm/i915/gem/i915_gem_wait.c | 9 ++---
  drivers/gpu/drm/i915/i915_request.c  | 3 ++-
  drivers/gpu/drm/i915/i915_request.h  | 1 +
  include/uapi/drm/i915_drm.h  | 1 +
  4 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_wait.c 
b/drivers/gpu/drm/i915/gem/i915_gem_wait.c

index d4b918fb11ce..955885ec859d 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_wait.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_wait.c
@@ -72,7 +72,8 @@ i915_gem_object_wait_reservation(struct dma_resv 
*resv,

  struct dma_fence *fence;
  long ret = timeout ?: 1;
  -    i915_gem_object_boost(resv, flags);
+    if (!(flags & I915_WAITBOOST_DISABLE))
+    i915_gem_object_boost(resv, flags);
    dma_resv_iter_begin(, resv,
  dma_resv_usage_rw(flags & I915_WAIT_ALL));
@@ -236,7 +237,7 @@ i915_gem_wait_ioctl(struct drm_device *dev, void 
*data, struct drm_file *file)

  ktime_t start;
  long ret;
  -    if (args->flags != 0)
+    if (args->flags != 0 || args->flags != I915_GEM_WAITBOOST_DISABLE)
  return -EINVAL;
    obj = i915_gem_object_lookup(file, args->bo_handle);
@@ -248,7 +249,9 @@ i915_gem_wait_ioctl(struct drm_device *dev, void 
*data, struct drm_file *file)

  ret = i915_gem_object_wait(obj,
 I915_WAIT_INTERRUPTIBLE |
 I915_WAIT_PRIORITY |
-   I915_WAIT_ALL,
+   I915_WAIT_ALL |
+   (args->flags & I915_GEM_WAITBOOST_DISABLE ?
+    I915_WAITBOOST_DISABLE : 0),
 to_wait_timeout(args->timeout_ns));
    if (args->timeout_ns > 0) {
diff --git a/drivers/gpu/drm/i915/i915_request.c 
b/drivers/gpu/drm/i915/i915_request.c

index f59081066a19..2957409b4b2a 100644
--- a/drivers/gpu/drm/i915/i915_request.c
+++ b/drivers/gpu/drm/i915/i915_request.c
@@ -2044,7 +2044,8 @@ long i915_request_wait_timeout(struct 
i915_request *rq,

   * but at a cost of spending more power processing the workload
   * (bad for battery).
   */
-    if (flags & I915_WAIT_PRIORITY && !i915_request_started(rq))
+    if (!(flags & I915_WAITBOOST_DISABLE) && (flags & 
I915_WAIT_PRIORITY) &&

+    !i915_request_started(rq))
  intel_rps_boost(rq);
    wait.tsk = current;
diff --git a/drivers/gpu/drm/i915/i915_request.h 
b/drivers/gpu/drm/i915/i915_request.h

index 0ac55b2e4223..3cc00e8254dc 100644
--- a/drivers/gpu/drm/i915/i915_request.h
+++ b/drivers/gpu/drm/i915/i915_request.h
@@ -445,6 +445,7 @@ long i915_request_wait(struct i915_request *rq,
  #define I915_WAIT_INTERRUPTIBLE    BIT(0)
  #define I915_WAIT_PRIORITY    BIT(1) /* small priority bump for the 
request */
  #define I915_WAIT_ALL    BIT(2) /* used by 
i915_gem_object_wait() */
+#define I915_WAITBOOST_DISABLE    BIT(3) /* used by 
i915_gem_object_wait() */

    void i915_request_show(struct drm_printer *m,
 const struct i915_request *rq,
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index 7000e5910a1d..4adee70e39cf 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -1928,6 +1928,7 @@ struct drm_i915_gem_wait {
  /** Handle of BO we shall wait on */
  __u32 bo_handle;
  __u32 flags;
+#define I915_GEM_WAITBOOST_DISABLE  (1u<<0)


Probably would be good to avoid mentioning waitboost in the uapi since 
so far it wasn't an explicit feature/contract. Something like 
I915_GEM_WAIT_BACKGROUND_PRIORITY? Low priority?

sure.


I also wonder if there could be a possible angle to help Rob (+cc) 
upstream the syncobj/fence deadline code if our media driver might 
make use of that somehow.


Like if either we could wire up the deadline into GEM_WAIT (in a 
backward compatible manner), or if media could use sync fd wait 
instead. Assuming they have an out fence already, which may not be true.


Makes sense. We could add a SET_DEADLINE flag or something similar and 
pass in the deadline when appropriate.


Rob - do you have time and motivation to think about this angle at all 
currently? If not I guess we just proceed with the new flag for our 
GEM_WAIT.


Regards,

Tvrtko



Thanks,

Vinay.



Regards,

Tvrtko


  /** Number of nanoseconds to

Re: [Intel-gfx] [PATCH] drm/i915/gem: Allow users to disable waitboost

2023-09-27 Thread Belgaumkar, Vinay 



On 9/21/2023 3:41 AM, Tvrtko Ursulin wrote:


On 20/09/2023 22:56, Vinay Belgaumkar wrote:

Provide a bit to disable waitboost while waiting on a gem object.
Waitboost results in increased power consumption by requesting RP0
while waiting for the request to complete. Add a bit in the gem_wait()
IOCTL where this can be disabled.

This is related to the libva API change here -
Link: 
https://github.com/XinfengZhang/libva/commit/3d90d18c67609a73121bb71b20ee4776b54b61a7


This link does not appear to lead to userspace code using this uapi?

We have asked Carl (cc'd) to post a patch for the same.




Cc: Rodrigo Vivi 
Signed-off-by: Vinay Belgaumkar 
---
  drivers/gpu/drm/i915/gem/i915_gem_wait.c | 9 ++---
  drivers/gpu/drm/i915/i915_request.c  | 3 ++-
  drivers/gpu/drm/i915/i915_request.h  | 1 +
  include/uapi/drm/i915_drm.h  | 1 +
  4 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_wait.c 
b/drivers/gpu/drm/i915/gem/i915_gem_wait.c

index d4b918fb11ce..955885ec859d 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_wait.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_wait.c
@@ -72,7 +72,8 @@ i915_gem_object_wait_reservation(struct dma_resv 
*resv,

  struct dma_fence *fence;
  long ret = timeout ?: 1;
  -    i915_gem_object_boost(resv, flags);
+    if (!(flags & I915_WAITBOOST_DISABLE))
+    i915_gem_object_boost(resv, flags);
    dma_resv_iter_begin(, resv,
  dma_resv_usage_rw(flags & I915_WAIT_ALL));
@@ -236,7 +237,7 @@ i915_gem_wait_ioctl(struct drm_device *dev, void 
*data, struct drm_file *file)

  ktime_t start;
  long ret;
  -    if (args->flags != 0)
+    if (args->flags != 0 || args->flags != I915_GEM_WAITBOOST_DISABLE)
  return -EINVAL;
    obj = i915_gem_object_lookup(file, args->bo_handle);
@@ -248,7 +249,9 @@ i915_gem_wait_ioctl(struct drm_device *dev, void 
*data, struct drm_file *file)

  ret = i915_gem_object_wait(obj,
 I915_WAIT_INTERRUPTIBLE |
 I915_WAIT_PRIORITY |
-   I915_WAIT_ALL,
+   I915_WAIT_ALL |
+   (args->flags & I915_GEM_WAITBOOST_DISABLE ?
+    I915_WAITBOOST_DISABLE : 0),
 to_wait_timeout(args->timeout_ns));
    if (args->timeout_ns > 0) {
diff --git a/drivers/gpu/drm/i915/i915_request.c 
b/drivers/gpu/drm/i915/i915_request.c

index f59081066a19..2957409b4b2a 100644
--- a/drivers/gpu/drm/i915/i915_request.c
+++ b/drivers/gpu/drm/i915/i915_request.c
@@ -2044,7 +2044,8 @@ long i915_request_wait_timeout(struct 
i915_request *rq,

   * but at a cost of spending more power processing the workload
   * (bad for battery).
   */
-    if (flags & I915_WAIT_PRIORITY && !i915_request_started(rq))
+    if (!(flags & I915_WAITBOOST_DISABLE) && (flags & 
I915_WAIT_PRIORITY) &&

+    !i915_request_started(rq))
  intel_rps_boost(rq);
    wait.tsk = current;
diff --git a/drivers/gpu/drm/i915/i915_request.h 
b/drivers/gpu/drm/i915/i915_request.h

index 0ac55b2e4223..3cc00e8254dc 100644
--- a/drivers/gpu/drm/i915/i915_request.h
+++ b/drivers/gpu/drm/i915/i915_request.h
@@ -445,6 +445,7 @@ long i915_request_wait(struct i915_request *rq,
  #define I915_WAIT_INTERRUPTIBLE    BIT(0)
  #define I915_WAIT_PRIORITY    BIT(1) /* small priority bump for the 
request */
  #define I915_WAIT_ALL    BIT(2) /* used by 
i915_gem_object_wait() */
+#define I915_WAITBOOST_DISABLE    BIT(3) /* used by 
i915_gem_object_wait() */

    void i915_request_show(struct drm_printer *m,
 const struct i915_request *rq,
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index 7000e5910a1d..4adee70e39cf 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -1928,6 +1928,7 @@ struct drm_i915_gem_wait {
  /** Handle of BO we shall wait on */
  __u32 bo_handle;
  __u32 flags;
+#define I915_GEM_WAITBOOST_DISABLE  (1u<<0)


Probably would be good to avoid mentioning waitboost in the uapi since 
so far it wasn't an explicit feature/contract. Something like 
I915_GEM_WAIT_BACKGROUND_PRIORITY? Low priority?

sure.


I also wonder if there could be a possible angle to help Rob (+cc) 
upstream the syncobj/fence deadline code if our media driver might 
make use of that somehow.


Like if either we could wire up the deadline into GEM_WAIT (in a 
backward compatible manner), or if media could use sync fd wait 
instead. Assuming they have an out fence already, which may not be true.


Makes sense. We could add a SET_DEADLINE flag or something similar and 
pass in the deadline when appropriate.


Thanks,

Vinay.



Regards,

Tvrtko


  /** Number of nanoseconds to wait, Returns time remaining. */
  __s64 timeout_ns;
  };

Re: [Intel-gfx] [PATCH] drm/i915/gem: Allow users to disable waitboost

2023-09-25 Thread kernel test robot 



Hello,

kernel test robot noticed a -3.2% regression of 
phoronix-test-suite.paraview.WaveletContour.1024x768.mipolys___sec on:


commit: 54fef7ea35dadd66193b98805b0bc42ef2b279db ("[PATCH] drm/i915/gem: Allow 
users to disable waitboost")
url: 
https://github.com/intel-lab-lkp/linux/commits/Vinay-Belgaumkar/drm-i915-gem-Allow-users-to-disable-waitboost/20230921-060357
base: git://anongit.freedesktop.org/drm-intel for-linux-next
patch link: 
https://lore.kernel.org/all/20230920215624.3482244-1-vinay.belgaum...@intel.com/
patch subject: [PATCH] drm/i915/gem: Allow users to disable waitboost

testcase: phoronix-test-suite
test machine: 12 threads 1 sockets Intel(R) Core(TM) i7-8700 CPU @ 3.20GHz 
(Coffee Lake) with 32G memory
parameters:

need_x: true
test: paraview-1.0.2
option_a: Wavelet Contour
option_b: 1024 x 768
cpufreq_governor: performance


In addition to that, the commit also has significant impact on the following 
tests:

+--++
| testcase: change | phoronix-test-suite: 
phoronix-test-suite.x11perf.PutImageXY500x500Square.operations___second 12.8% 
improvement |
| test machine | 12 threads 1 sockets Intel(R) Core(TM) i7-8700 CPU @ 
3.20GHz (Coffee Lake) with 32G memory |
| test parameters  | cpufreq_governor=performance   
|
|  | need_x=true
|
|  | option_a=PutImage XY 500x500 Square
|
|  | test=x11perf-1.1.1 
|
+--++
| testcase: change | phoronix-test-suite: 
phoronix-test-suite.openarena.2560x1440.milliseconds -12.2% regression  
  |
| test machine | 12 threads 1 sockets Intel(R) Core(TM) i7-8700 CPU @ 
3.20GHz (Coffee Lake) with 32G memory |
| test parameters  | cpufreq_governor=performance   
|
|  | need_x=true
|
|  | option_a=2560 x 1440   
|
|  | test=openarena-1.5.5   
|
+--++


If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot 
| Closes: 
https://lore.kernel.org/oe-lkp/202309261055.b74df987-oliver.s...@intel.com


Details are as below:
-->


The kernel config and materials to reproduce are available at:
https://download.01.org/0day-ci/archive/20230926/202309261055.b74df987-oliver.s...@intel.com

=
compiler/cpufreq_governor/kconfig/need_x/option_a/option_b/rootfs/tbox_group/test/testcase:
  gcc-12/performance/x86_64-rhel-8.3/true/Wavelet Contour/1024 x 
768/debian-x86_64-phoronix/lkp-cfl-d2/paraview-1.0.2/phoronix-test-suite

commit: 
  16a9359401 ("drm/i915: Implement transcoder LRR for TGL+")
  54fef7ea35 ("drm/i915/gem: Allow users to disable waitboost")

16a9359401edcbc0 54fef7ea35dadd66193b98805b0 
 --- 
 %stddev %change %stddev
 \  |\  
  0.05 ±  4%  +0.00.06 ±  2%  mpstat.cpu.all.soft%
 66.17 ± 60%+145.6% 162.50 ± 33%  turbostat.C10
 28.61-3.3%  27.68
phoronix-test-suite.paraview.WaveletContour.1024x768.frames___sec
298.15-3.2% 288.49
phoronix-test-suite.paraview.WaveletContour.1024x768.mipolys___sec
535005+8.6% 580810
phoronix-test-suite.time.minor_page_faults
  6278+8.3%   6797
phoronix-test-suite.time.voluntary_context_switches
801166+5.6% 845675proc-vmstat.numa_hit
799382+5.1% 840353proc-vmstat.numa_local
 59648+2.6%  61211proc-vmstat.pgactivate
   1539307+2.7%1580759

Re: [Intel-gfx] [PATCH] drm/i915/gem: Allow users to disable waitboost

2023-09-21 Thread Tvrtko Ursulin 



On 20/09/2023 22:56, Vinay Belgaumkar wrote:

Provide a bit to disable waitboost while waiting on a gem object.
Waitboost results in increased power consumption by requesting RP0
while waiting for the request to complete. Add a bit in the gem_wait()
IOCTL where this can be disabled.

This is related to the libva API change here -
Link: 
https://github.com/XinfengZhang/libva/commit/3d90d18c67609a73121bb71b20ee4776b54b61a7


This link does not appear to lead to userspace code using this uapi?



Cc: Rodrigo Vivi 
Signed-off-by: Vinay Belgaumkar 
---
  drivers/gpu/drm/i915/gem/i915_gem_wait.c | 9 ++---
  drivers/gpu/drm/i915/i915_request.c  | 3 ++-
  drivers/gpu/drm/i915/i915_request.h  | 1 +
  include/uapi/drm/i915_drm.h  | 1 +
  4 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_wait.c 
b/drivers/gpu/drm/i915/gem/i915_gem_wait.c
index d4b918fb11ce..955885ec859d 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_wait.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_wait.c
@@ -72,7 +72,8 @@ i915_gem_object_wait_reservation(struct dma_resv *resv,
struct dma_fence *fence;
long ret = timeout ?: 1;
  
-	i915_gem_object_boost(resv, flags);

+   if (!(flags & I915_WAITBOOST_DISABLE))
+   i915_gem_object_boost(resv, flags);
  
  	dma_resv_iter_begin(, resv,

dma_resv_usage_rw(flags & I915_WAIT_ALL));
@@ -236,7 +237,7 @@ i915_gem_wait_ioctl(struct drm_device *dev, void *data, 
struct drm_file *file)
ktime_t start;
long ret;
  
-	if (args->flags != 0)

+   if (args->flags != 0 || args->flags != I915_GEM_WAITBOOST_DISABLE)
return -EINVAL;
  
  	obj = i915_gem_object_lookup(file, args->bo_handle);

@@ -248,7 +249,9 @@ i915_gem_wait_ioctl(struct drm_device *dev, void *data, 
struct drm_file *file)
ret = i915_gem_object_wait(obj,
   I915_WAIT_INTERRUPTIBLE |
   I915_WAIT_PRIORITY |
-  I915_WAIT_ALL,
+  I915_WAIT_ALL |
+  (args->flags & I915_GEM_WAITBOOST_DISABLE ?
+   I915_WAITBOOST_DISABLE : 0),
   to_wait_timeout(args->timeout_ns));
  
  	if (args->timeout_ns > 0) {

diff --git a/drivers/gpu/drm/i915/i915_request.c 
b/drivers/gpu/drm/i915/i915_request.c
index f59081066a19..2957409b4b2a 100644
--- a/drivers/gpu/drm/i915/i915_request.c
+++ b/drivers/gpu/drm/i915/i915_request.c
@@ -2044,7 +2044,8 @@ long i915_request_wait_timeout(struct i915_request *rq,
 * but at a cost of spending more power processing the workload
 * (bad for battery).
 */
-   if (flags & I915_WAIT_PRIORITY && !i915_request_started(rq))
+   if (!(flags & I915_WAITBOOST_DISABLE) && (flags & I915_WAIT_PRIORITY) &&
+   !i915_request_started(rq))
intel_rps_boost(rq);
  
  	wait.tsk = current;

diff --git a/drivers/gpu/drm/i915/i915_request.h 
b/drivers/gpu/drm/i915/i915_request.h
index 0ac55b2e4223..3cc00e8254dc 100644
--- a/drivers/gpu/drm/i915/i915_request.h
+++ b/drivers/gpu/drm/i915/i915_request.h
@@ -445,6 +445,7 @@ long i915_request_wait(struct i915_request *rq,
  #define I915_WAIT_INTERRUPTIBLE   BIT(0)
  #define I915_WAIT_PRIORITYBIT(1) /* small priority bump for the request */
  #define I915_WAIT_ALL BIT(2) /* used by i915_gem_object_wait() */
+#define I915_WAITBOOST_DISABLE BIT(3) /* used by i915_gem_object_wait() */
  
  void i915_request_show(struct drm_printer *m,

   const struct i915_request *rq,
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index 7000e5910a1d..4adee70e39cf 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -1928,6 +1928,7 @@ struct drm_i915_gem_wait {
/** Handle of BO we shall wait on */
__u32 bo_handle;
__u32 flags;
+#define I915_GEM_WAITBOOST_DISABLE  (1u<<0)


Probably would be good to avoid mentioning waitboost in the uapi since 
so far it wasn't an explicit feature/contract. Something like 
I915_GEM_WAIT_BACKGROUND_PRIORITY? Low priority?


I also wonder if there could be a possible angle to help Rob (+cc) 
upstream the syncobj/fence deadline code if our media driver might make 
use of that somehow.


Like if either we could wire up the deadline into GEM_WAIT (in a 
backward compatible manner), or if media could use sync fd wait instead. 
Assuming they have an out fence already, which may not be true.


Regards,

Tvrtko


/** Number of nanoseconds to wait, Returns time remaining. */
__s64 timeout_ns;
  };

[Intel-gfx] [PATCH] drm/i915/gem: Allow users to disable waitboost

2023-09-20 Thread Vinay Belgaumkar 
Provide a bit to disable waitboost while waiting on a gem object.
Waitboost results in increased power consumption by requesting RP0
while waiting for the request to complete. Add a bit in the gem_wait()
IOCTL where this can be disabled.

This is related to the libva API change here -
Link: 
https://github.com/XinfengZhang/libva/commit/3d90d18c67609a73121bb71b20ee4776b54b61a7

Cc: Rodrigo Vivi 
Signed-off-by: Vinay Belgaumkar 
---
 drivers/gpu/drm/i915/gem/i915_gem_wait.c | 9 ++---
 drivers/gpu/drm/i915/i915_request.c  | 3 ++-
 drivers/gpu/drm/i915/i915_request.h  | 1 +
 include/uapi/drm/i915_drm.h  | 1 +
 4 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_wait.c 
b/drivers/gpu/drm/i915/gem/i915_gem_wait.c
index d4b918fb11ce..955885ec859d 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_wait.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_wait.c
@@ -72,7 +72,8 @@ i915_gem_object_wait_reservation(struct dma_resv *resv,
struct dma_fence *fence;
long ret = timeout ?: 1;
 
-   i915_gem_object_boost(resv, flags);
+   if (!(flags & I915_WAITBOOST_DISABLE))
+   i915_gem_object_boost(resv, flags);
 
dma_resv_iter_begin(, resv,
dma_resv_usage_rw(flags & I915_WAIT_ALL));
@@ -236,7 +237,7 @@ i915_gem_wait_ioctl(struct drm_device *dev, void *data, 
struct drm_file *file)
ktime_t start;
long ret;
 
-   if (args->flags != 0)
+   if (args->flags != 0 || args->flags != I915_GEM_WAITBOOST_DISABLE)
return -EINVAL;
 
obj = i915_gem_object_lookup(file, args->bo_handle);
@@ -248,7 +249,9 @@ i915_gem_wait_ioctl(struct drm_device *dev, void *data, 
struct drm_file *file)
ret = i915_gem_object_wait(obj,
   I915_WAIT_INTERRUPTIBLE |
   I915_WAIT_PRIORITY |
-  I915_WAIT_ALL,
+  I915_WAIT_ALL |
+  (args->flags & I915_GEM_WAITBOOST_DISABLE ?
+   I915_WAITBOOST_DISABLE : 0),
   to_wait_timeout(args->timeout_ns));
 
if (args->timeout_ns > 0) {
diff --git a/drivers/gpu/drm/i915/i915_request.c 
b/drivers/gpu/drm/i915/i915_request.c
index f59081066a19..2957409b4b2a 100644
--- a/drivers/gpu/drm/i915/i915_request.c
+++ b/drivers/gpu/drm/i915/i915_request.c
@@ -2044,7 +2044,8 @@ long i915_request_wait_timeout(struct i915_request *rq,
 * but at a cost of spending more power processing the workload
 * (bad for battery).
 */
-   if (flags & I915_WAIT_PRIORITY && !i915_request_started(rq))
+   if (!(flags & I915_WAITBOOST_DISABLE) && (flags & I915_WAIT_PRIORITY) &&
+   !i915_request_started(rq))
intel_rps_boost(rq);
 
wait.tsk = current;
diff --git a/drivers/gpu/drm/i915/i915_request.h 
b/drivers/gpu/drm/i915/i915_request.h
index 0ac55b2e4223..3cc00e8254dc 100644
--- a/drivers/gpu/drm/i915/i915_request.h
+++ b/drivers/gpu/drm/i915/i915_request.h
@@ -445,6 +445,7 @@ long i915_request_wait(struct i915_request *rq,
 #define I915_WAIT_INTERRUPTIBLEBIT(0)
 #define I915_WAIT_PRIORITY BIT(1) /* small priority bump for the request */
 #define I915_WAIT_ALL  BIT(2) /* used by i915_gem_object_wait() */
+#define I915_WAITBOOST_DISABLE BIT(3) /* used by i915_gem_object_wait() */
 
 void i915_request_show(struct drm_printer *m,
   const struct i915_request *rq,
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index 7000e5910a1d..4adee70e39cf 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -1928,6 +1928,7 @@ struct drm_i915_gem_wait {
/** Handle of BO we shall wait on */
__u32 bo_handle;
__u32 flags;
+#define I915_GEM_WAITBOOST_DISABLE  (1u<<0)
/** Number of nanoseconds to wait, Returns time remaining. */
__s64 timeout_ns;
 };
-- 
2.38.1