Re: [PATCH] drm/amdgpu: add parameter to allocate high priority contexts v8

["Nicolai Hähnle" <nhaehnle-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

On 24.04.2017 18:20, Andres Rodriguez wrote:
> Add a new context creation parameter to express a global context priority.
>
> The priority ranking in descending order is as follows:
>  * AMDGPU_CTX_PRIORITY_HIGH
>  * AMDGPU_CTX_PRIORITY_NORMAL
>  * AMDGPU_CTX_PRIORITY_LOW
>
> The driver will attempt to schedule work to the hardware according to
> the priorities. No latency or throughput guarantees are provided by
> this patch.
>
> This interface intends to service the EGL_IMG_context_priority
> extension, and vulkan equivalents.
>
> v2: Instead of using flags, repurpose __pad
> v3: Swap enum values of _NORMAL _HIGH for backwards compatibility
> v4: Validate usermode priority and store it
> v5: Move priority validation into amdgpu_ctx_ioctl(), headline reword
> v6: add UAPI note regarding priorities requiring CAP_SYS_ADMIN
> v7: remove ctx->priority
> v8: added AMDGPU_CTX_PRIORITY_LOW, s/CAP_SYS_ADMIN/CAP_SYS_NICE
 >
> Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com>
> Reviewed-by: Christian König <christian.koenig@amd.com>
> Signed-off-by: Andres Rodriguez <andresx7@gmail.com>

I didn't follow all the discussion, so feel free to shut me up if this 
has already been discussed, but...


[snip]
> +/* Context priority level */
> +#define AMDGPU_CTX_PRIORITY_NORMAL	0
> +#define AMDGPU_CTX_PRIORITY_LOW		1
> +/* Selecting a priority above NORMAL requires CAP_SYS_ADMIN */
> +#define AMDGPU_CTX_PRIORITY_HIGH	2
> +#define AMDGPU_CTX_PRIORITY_NUM		3

I get that normal priority needs to be 0 for backwards compatibility, 
but having LOW between NORMAL and HIGH is still odd. Have you considered 
using signed integers as a way to fix that?

(AMDGPU_CTX_PRIORITY_NUM doesn't seem to be used anywhere...)

Cheers,
Nicolai


> +
>  struct drm_amdgpu_ctx_in {
>  	/** AMDGPU_CTX_OP_* */
>  	__u32	op;
>  	/** For future use, no flags defined so far */
>  	__u32	flags;
>  	__u32	ctx_id;
> -	__u32	_pad;
> +	__u32	priority;
>  };
>
>  union drm_amdgpu_ctx_out {
>  		struct {
>  			__u32	ctx_id;
>  			__u32	_pad;
>  		} alloc;
>
>  		struct {
>  			/** For future use, no flags defined so far */
>  			__u64	flags;
>  			/** Number of resets caused by this context so far. */
>  			__u32	hangs;
>  			/** Reset status since the last call of the ioctl. */
>  			__u32	reset_status;
>  		} state;
>  };
>
>  union drm_amdgpu_ctx {
>  	struct drm_amdgpu_ctx_in in;
>


-- 
Lerne, wie die Welt wirklich ist,
Aber vergiss niemals, wie sie sein sollte.
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx