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

Tvrtko Ursulin
2023-09-21T10:41:33Z

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 <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 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;
> };