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

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

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 <rodrigo.vivi@intel.com>
> > > > Signed-off-by: Vinay Belgaumkar <vinay.belgaumkar@intel.com>
> > > > ---
> > > >   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

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 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.

Well, this could be the first user for that uapi that Rob was proposing
indeed.

The downside is probably because we should implement the deadline in i915
and consider all the deadline as 0 (urgent) and boost, unless in this
case where before the gem_wait the UMD would use the set_deadline to
something higher (max?).

Well, if we have a clarity on how to proceed with the deadline we should
probably go there. But for simplicity I would be in favor of this proposed
gem_wait flag as is, because this already solves many real important cases.

> 
> Regards,
> 
> Tvrtko
> 
> > 
> > Thanks,
> > 
> > Vinay.
> > 
> > > 
> > > Regards,
> > > 
> > > Tvrtko
> > > 
> > > >       /** Number of nanoseconds to wait, Returns time remaining. */
> > > >       __s64 timeout_ns;
> > > >   };