Re: [PATCH] amdgpu: add context creation flags in CS IOCTL

["Christian König" <christian.koenig@amd.com>]

Am 02.08.22 um 15:55 schrieb Shashank Sharma:
> This patch adds:
> - A new input parameter "flags" in the amdgpu_ctx_create2 call.
> - Some new flags defining workload type hints.
> - Some change in the caller function of amdgpu_ctx_create2, to
>    accomodate this new parameter.
>
> The idea is to pass the workload hints while context creation,

Bad idea.

Please take AMDGPU_CTX_OP_SET_STABLE_PSTATE and 
AMDGPU_CTX_OP_GET_STABLE_PSTATE as blueprint for this and don't add 
extra flags to the context creation.

Regards,
Christian.


>   so
> that kernel GPU scheduler can pass this information to GPU FW, which in
> turn can adjust the GPU characterstics as per the workload type.
>
> Signed-off-by: Shashank Sharma <shashank.sharma@amd.com>
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Cc: Marek Olsak <marek.olsak@amd.com>
> Cc: Christian Koenig <christian.koenig@amd.com>
> Cc: Amarnath Somalapuram <amaranath.somalapuram@amd.com>
> ---
>   amdgpu/amdgpu.h          |  2 ++
>   amdgpu/amdgpu_cs.c       |  5 ++++-
>   include/drm/amdgpu_drm.h | 10 +++++++++-
>   3 files changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/amdgpu/amdgpu.h b/amdgpu/amdgpu.h
> index b118dd48..1ebb46e6 100644
> --- a/amdgpu/amdgpu.h
> +++ b/amdgpu/amdgpu.h
> @@ -874,6 +874,7 @@ int amdgpu_bo_list_update(amdgpu_bo_list_handle handle,
>    *
>    * \param   dev      - \c [in] Device handle. See #amdgpu_device_initialize()
>    * \param   priority - \c [in] Context creation flags. See AMDGPU_CTX_PRIORITY_*
> + * \param   flags    - \c [in] Context flags. See AMDGPU_CTX_FLAGS_*
>    * \param   context  - \c [out] GPU Context handle
>    *
>    * \return   0 on success\n
> @@ -884,6 +885,7 @@ int amdgpu_bo_list_update(amdgpu_bo_list_handle handle,
>   */
>   int amdgpu_cs_ctx_create2(amdgpu_device_handle dev,
>   			 uint32_t priority,
> +			 uint32_t flags,
>   			 amdgpu_context_handle *context);
>   /**
>    * Create GPU execution Context
> diff --git a/amdgpu/amdgpu_cs.c b/amdgpu/amdgpu_cs.c
> index fad484bf..d4723ea5 100644
> --- a/amdgpu/amdgpu_cs.c
> +++ b/amdgpu/amdgpu_cs.c
> @@ -44,12 +44,14 @@ static int amdgpu_cs_reset_sem(amdgpu_semaphore_handle sem);
>    *
>    * \param   dev      - \c [in] Device handle. See #amdgpu_device_initialize()
>    * \param   priority - \c [in] Context creation flags. See AMDGPU_CTX_PRIORITY_*
> + * \param   flags    - \c [in] Context flags. See AMDGPU_CTX_FLAGS_*
>    * \param   context  - \c [out] GPU Context handle
>    *
>    * \return  0 on success otherwise POSIX Error code
>   */
>   drm_public int amdgpu_cs_ctx_create2(amdgpu_device_handle dev,
>   				     uint32_t priority,
> +				     uint32_t flags,
>   				     amdgpu_context_handle *context)
>   {
>   	struct amdgpu_context *gpu_context;
> @@ -74,6 +76,7 @@ drm_public int amdgpu_cs_ctx_create2(amdgpu_device_handle dev,
>   	memset(&args, 0, sizeof(args));
>   	args.in.op = AMDGPU_CTX_OP_ALLOC_CTX;
>   	args.in.priority = priority;
> +	args.in.flags = flags;
>   
>   	r = drmCommandWriteRead(dev->fd, DRM_AMDGPU_CTX, &args, sizeof(args));
>   	if (r)
> @@ -97,7 +100,7 @@ error:
>   drm_public int amdgpu_cs_ctx_create(amdgpu_device_handle dev,
>   				    amdgpu_context_handle *context)
>   {
> -	return amdgpu_cs_ctx_create2(dev, AMDGPU_CTX_PRIORITY_NORMAL, context);
> +	return amdgpu_cs_ctx_create2(dev, AMDGPU_CTX_PRIORITY_NORMAL, 0, context);
>   }
>   
>   /**
> diff --git a/include/drm/amdgpu_drm.h b/include/drm/amdgpu_drm.h
> index 0cbd1540..d9fb1f20 100644
> --- a/include/drm/amdgpu_drm.h
> +++ b/include/drm/amdgpu_drm.h
> @@ -238,10 +238,18 @@ union drm_amdgpu_bo_list {
>   #define AMDGPU_CTX_PRIORITY_HIGH        512
>   #define AMDGPU_CTX_PRIORITY_VERY_HIGH   1023
>   
> +/* GPU context workload hint bitmask */
> +#define AMDGPU_CTX_FLAGS_WORKLOAD_HINT_MASK    0xFF
> +#define AMDGPU_CTX_FLAGS_WORKLOAD_HINT_NONE    0
> +#define AMDGPU_CTX_FLAGS_WORKLOAD_HINT_3D      (1 << 1)
> +#define AMDGPU_CTX_FLAGS_WORKLOAD_HINT_VIDEO   (1 << 2)
> +#define AMDGPU_CTX_FLAGS_WORKLOAD_HINT_VR      (1 << 3)
> +#define AMDGPU_CTX_FLAGS_WORKLOAD_HINT_COMPUTE (1 << 4)
> +
>   struct drm_amdgpu_ctx_in {
>   	/** AMDGPU_CTX_OP_* */
>   	__u32	op;
> -	/** For future use, no flags defined so far */
> +	/** AMDGPU_CTX_FLAGS_* */
>   	__u32	flags;
>   	__u32	ctx_id;
>   	/** AMDGPU_CTX_PRIORITY_* */