[Intel-gfx] [PATCH] drm/i915/gem: Allow users to disable waitboost
Rodrigo Vivi
rodrigo.vivi at intel.com
Mon Oct 16 17:58:33 UTC 2023
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 <rodrigo.vivi at intel.com>
> > > > > > Signed-off-by: Vinay Belgaumkar <vinay.belgaumkar at 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?
>
> 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.
good idea!
>
> 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 it seems we only removed it because corner cases with the current
> scheduler were hard. Unfortunately improved deadline based scheduler never
> got in despite being ready so we can not restore this now.
>
> > > > > > 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.
>
> Yes I don't think we had consensus on the semantics of when no deadline is
> set, so it does sound better to proceed with i915 specific solution for now.
> The two wouldn't be incompatible if deadlines were later added.
>
> Regards,
>
> Tvrtko
>
> >
> > >
> > > Regards,
> > >
> > > Tvrtko
> > >
> > > >
> > > > Thanks,
> > > >
> > > > Vinay.
> > > >
> > > > >
> > > > > Regards,
> > > > >
> > > > > Tvrtko
> > > > >
> > > > > > /** Number of nanoseconds to wait, Returns time remaining. */
> > > > > > __s64 timeout_ns;
> > > > > > };
More information about the dri-devel
mailing list