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

[Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx>]

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 <rodrigo.vivi@xxxxxxxxx>
> > > Signed-off-by: Vinay Belgaumkar <vinay.belgaumkar@xxxxxxxxx>
> > > ---
> > >   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(&cursor, 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 wait, Returns time remaining. */
> > >       __s64 timeout_ns;
> > >   };