Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <3fd504bf-0415-3466-78c5-918a295dcf93@linux.intel.com>
Date:   Tue, 28 Mar 2023 15:24:42 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.8.0
Subject: Re: [PATCH v10 09/15] drm/syncobj: Add deadline support for syncobj
 waits
Content-Language: en-US
To:     Rob Clark <robdclark@gmail.com>, dri-devel@lists.freedesktop.org
Cc:     Rob Clark <robdclark@chromium.org>,
        =?UTF-8?Q?Christian_K=c3=b6nig?= <ckoenig.leichtzumerken@gmail.com>,
        intel-gfx@lists.freedesktop.org,
        open list <linux-kernel@vger.kernel.org>,
        Luben Tuikov <luben.tuikov@amd.com>,
        Thomas Zimmermann <tzimmermann@suse.de>,
        Rodrigo Vivi <rodrigo.vivi@intel.com>,
        Matt Turner <mattst88@gmail.com>,
        freedreno@lists.freedesktop.org
References: <20230308155322.344664-1-robdclark@gmail.com>
 <20230308155322.344664-10-robdclark@gmail.com>
From:   Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
Organization: Intel Corporation UK Plc
In-Reply-To: <20230308155322.344664-10-robdclark@gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 08/03/2023 15:53, Rob Clark wrote:
> From: Rob Clark <robdclark@chromium.org>
> 
> Add a new flag to let userspace provide a deadline as a hint for syncobj
> and timeline waits.  This gives a hint to the driver signaling the
> backing fences about how soon userspace needs it to compete work, so it
> can addjust GPU frequency accordingly.  An immediate deadline can be

adjust

> given to provide something equivalent to i915 "wait boost".
> 
> v2: Use absolute u64 ns value for deadline hint, drop cap and driver
>      feature flag in favor of allowing count_handles==0 as a way for
>      userspace to probe kernel for support of new flag
> v3: More verbose comments about UAPI
> 
> Signed-off-by: Rob Clark <robdclark@chromium.org>
> ---
>   drivers/gpu/drm/drm_syncobj.c | 64 ++++++++++++++++++++++++++++-------
>   include/uapi/drm/drm.h        | 17 ++++++++++
>   2 files changed, 68 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
> index 0c2be8360525..a85e9464f07b 100644
> --- a/drivers/gpu/drm/drm_syncobj.c
> +++ b/drivers/gpu/drm/drm_syncobj.c
> @@ -126,6 +126,11 @@
>    * synchronize between the two.
>    * This requirement is inherited from the Vulkan fence API.
>    *
> + * If &DRM_SYNCOBJ_WAIT_FLAGS_WAIT_DEADLINE is set, the ioctl will also set
> + * a fence deadline hint on the backing fences before waiting, to provide the
> + * fence signaler with an appropriate sense of urgency.  The deadline is
> + * specified as an absolute &CLOCK_MONOTONIC value in units of ns.
> + *
>    * Similarly, &DRM_IOCTL_SYNCOBJ_TIMELINE_WAIT takes an array of syncobj
>    * handles as well as an array of u64 points and does a host-side wait on all
>    * of syncobj fences at the given points simultaneously.
> @@ -973,7 +978,8 @@ static signed long drm_syncobj_array_wait_timeout(struct drm_syncobj **syncobjs,
>   						  uint32_t count,
>   						  uint32_t flags,
>   						  signed long timeout,
> -						  uint32_t *idx)
> +						  uint32_t *idx,
> +						  ktime_t *deadline)
>   {
>   	struct syncobj_wait_entry *entries;
>   	struct dma_fence *fence;
> @@ -1053,6 +1059,15 @@ static signed long drm_syncobj_array_wait_timeout(struct drm_syncobj **syncobjs,
>   			drm_syncobj_fence_add_wait(syncobjs[i], &entries[i]);
>   	}
>   
> +	if (deadline) {
> +		for (i = 0; i < count; ++i) {
> +			fence = entries[i].fence;
> +			if (!fence)
> +				continue;
> +			dma_fence_set_deadline(fence, *deadline);
> +		}
> +	}
> +
>   	do {
>   		set_current_state(TASK_INTERRUPTIBLE);
>   
> @@ -1151,7 +1166,8 @@ static int drm_syncobj_array_wait(struct drm_device *dev,
>   				  struct drm_file *file_private,
>   				  struct drm_syncobj_wait *wait,
>   				  struct drm_syncobj_timeline_wait *timeline_wait,
> -				  struct drm_syncobj **syncobjs, bool timeline)
> +				  struct drm_syncobj **syncobjs, bool timeline,
> +				  ktime_t *deadline)
>   {
>   	signed long timeout = 0;
>   	uint32_t first = ~0;
> @@ -1162,7 +1178,8 @@ static int drm_syncobj_array_wait(struct drm_device *dev,
>   							 NULL,
>   							 wait->count_handles,
>   							 wait->flags,
> -							 timeout, &first);
> +							 timeout, &first,
> +							 deadline);
>   		if (timeout < 0)
>   			return timeout;
>   		wait->first_signaled = first;
> @@ -1172,7 +1189,8 @@ static int drm_syncobj_array_wait(struct drm_device *dev,
>   							 u64_to_user_ptr(timeline_wait->points),
>   							 timeline_wait->count_handles,
>   							 timeline_wait->flags,
> -							 timeout, &first);
> +							 timeout, &first,
> +							 deadline);
>   		if (timeout < 0)
>   			return timeout;
>   		timeline_wait->first_signaled = first;
> @@ -1243,17 +1261,22 @@ drm_syncobj_wait_ioctl(struct drm_device *dev, void *data,
>   {
>   	struct drm_syncobj_wait *args = data;
>   	struct drm_syncobj **syncobjs;
> +	unsigned possible_flags;
> +	ktime_t t, *tp = NULL;
>   	int ret = 0;
>   
>   	if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ))
>   		return -EOPNOTSUPP;
>   
> -	if (args->flags & ~(DRM_SYNCOBJ_WAIT_FLAGS_WAIT_ALL |
> -			    DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT))
> +	possible_flags = DRM_SYNCOBJ_WAIT_FLAGS_WAIT_ALL |
> +			 DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT |
> +			 DRM_SYNCOBJ_WAIT_FLAGS_WAIT_DEADLINE;
> +
> +	if (args->flags & ~possible_flags)
>   		return -EINVAL;
>   
>   	if (args->count_handles == 0)
> -		return -EINVAL;
> +		return 0;
>   
>   	ret = drm_syncobj_array_find(file_private,
>   				     u64_to_user_ptr(args->handles),
> @@ -1262,8 +1285,13 @@ drm_syncobj_wait_ioctl(struct drm_device *dev, void *data,
>   	if (ret < 0)
>   		return ret;
>   
> +	if (args->flags & DRM_SYNCOBJ_WAIT_FLAGS_WAIT_DEADLINE) {
> +		t = ns_to_ktime(args->deadline_ns);
> +		tp = &t;
> +	}
> +
>   	ret = drm_syncobj_array_wait(dev, file_private,
> -				     args, NULL, syncobjs, false);
> +				     args, NULL, syncobjs, false, tp);
>   
>   	drm_syncobj_array_free(syncobjs, args->count_handles);
>   
> @@ -1276,18 +1304,23 @@ drm_syncobj_timeline_wait_ioctl(struct drm_device *dev, void *data,
>   {
>   	struct drm_syncobj_timeline_wait *args = data;
>   	struct drm_syncobj **syncobjs;
> +	unsigned possible_flags;
> +	ktime_t t, *tp = NULL;
>   	int ret = 0;
>   
>   	if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ_TIMELINE))
>   		return -EOPNOTSUPP;
>   
> -	if (args->flags & ~(DRM_SYNCOBJ_WAIT_FLAGS_WAIT_ALL |
> -			    DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT |
> -			    DRM_SYNCOBJ_WAIT_FLAGS_WAIT_AVAILABLE))
> +	possible_flags = DRM_SYNCOBJ_WAIT_FLAGS_WAIT_ALL |
> +			 DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT |
> +			 DRM_SYNCOBJ_WAIT_FLAGS_WAIT_AVAILABLE |
> +			 DRM_SYNCOBJ_WAIT_FLAGS_WAIT_DEADLINE;
> +
> +	if (args->flags & ~possible_flags)
>   		return -EINVAL;
>   
>   	if (args->count_handles == 0)
> -		return -EINVAL;
> +		return -0;
>   
>   	ret = drm_syncobj_array_find(file_private,
>   				     u64_to_user_ptr(args->handles),
> @@ -1296,8 +1329,13 @@ drm_syncobj_timeline_wait_ioctl(struct drm_device *dev, void *data,
>   	if (ret < 0)
>   		return ret;
>   
> +	if (args->flags & DRM_SYNCOBJ_WAIT_FLAGS_WAIT_DEADLINE) {
> +		t = ns_to_ktime(args->deadline_ns);
> +		tp = &t;
> +	}
> +
>   	ret = drm_syncobj_array_wait(dev, file_private,
> -				     NULL, args, syncobjs, true);
> +				     NULL, args, syncobjs, true, tp);
>   
>   	drm_syncobj_array_free(syncobjs, args->count_handles);
>   
> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> index 642808520d92..bff0509ac8b6 100644
> --- a/include/uapi/drm/drm.h
> +++ b/include/uapi/drm/drm.h
> @@ -887,6 +887,7 @@ struct drm_syncobj_transfer {
>   #define DRM_SYNCOBJ_WAIT_FLAGS_WAIT_ALL (1 << 0)
>   #define DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT (1 << 1)
>   #define DRM_SYNCOBJ_WAIT_FLAGS_WAIT_AVAILABLE (1 << 2) /* wait for time point to become available */
> +#define DRM_SYNCOBJ_WAIT_FLAGS_WAIT_DEADLINE (1 << 3) /* set fence deadline based to deadline_ns */

s/based// ?

>   struct drm_syncobj_wait {
>   	__u64 handles;
>   	/* absolute timeout */
> @@ -895,6 +896,14 @@ struct drm_syncobj_wait {
>   	__u32 flags;
>   	__u32 first_signaled; /* only valid when not waiting all */
>   	__u32 pad;
> +	/**
> +	 * @deadline_ns - fence deadline hint
> +	 *
> +	 * Deadline hint, in absolute CLOCK_MONOTONIC, to set on backing
> +	 * fence(s) if the DRM_SYNCOBJ_WAIT_FLAGS_WAIT_DEADLINE flag is
> +	 * set.
> +	 */
> +	__u64 deadline_ns;
>   };
>   
>   struct drm_syncobj_timeline_wait {
> @@ -907,6 +916,14 @@ struct drm_syncobj_timeline_wait {
>   	__u32 flags;
>   	__u32 first_signaled; /* only valid when not waiting all */
>   	__u32 pad;
> +	/**
> +	 * @deadline_ns - fence deadline hint
> +	 *
> +	 * Deadline hint, in absolute CLOCK_MONOTONIC, to set on backing
> +	 * fence(s) if the DRM_SYNCOBJ_WAIT_FLAGS_WAIT_DEADLINE flag is
> +	 * set.
> +	 */
> +	__u64 deadline_ns;
>   };
>   
>   

FWIW,

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko