Hi Andres,
There's a couple of nitpicks below, but feel free to address those as
follow-up. Considering they're correct of course ;-)
On 28 February 2017 at 22:14, Andres Rodriguez <andresx7 at gmail.com> wrote:
> Add a new context creation parameter to express a global context priority.
>
> Contexts allocated with AMDGPU_CTX_PRIORITY_HIGH will receive higher
> priority to scheduler their work than AMDGPU_CTX_PRIORITY_NORMAL
> (default) contexts.
>
> v2: Instead of using flags, repurpose __pad
> v3: Swap enum values of _NORMAL _HIGH for backwards compatibility
> v4: Validate usermode priority and store it
>
> Signed-off-by: Andres Rodriguez <andresx7 at gmail.com>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu.h | 1 +
> drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c | 41 +++++++++++++++++++++++----
> drivers/gpu/drm/amd/scheduler/gpu_scheduler.h | 1 +
> include/uapi/drm/amdgpu_drm.h | 7 ++++-
> 4 files changed, 44 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index e30c47e..366f6d3 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> @@ -664,20 +664,21 @@ struct amdgpu_ctx_ring {
> struct amd_sched_entity entity;
> };
>
> struct amdgpu_ctx {
> struct kref refcount;
> struct amdgpu_device *adev;
> unsigned reset_counter;
> spinlock_t ring_lock;
> struct dma_fence **fences;
> struct amdgpu_ctx_ring rings[AMDGPU_MAX_RINGS];
> + int priority;
> bool preamble_presented;
> };
>
> struct amdgpu_ctx_mgr {
> struct amdgpu_device *adev;
> struct mutex lock;
> /* protected by lock */
> struct idr ctx_handles;
> };
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
> index 400c66b..22a15d6 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
> @@ -18,47 +18,75 @@
> * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> * OTHER DEALINGS IN THE SOFTWARE.
> *
> * Authors: monk liu <monk.liu at amd.com>
> */
>
> #include <drm/drmP.h>
> #include "amdgpu.h"
>
> -static int amdgpu_ctx_init(struct amdgpu_device *adev, struct amdgpu_ctx *ctx)
> +static enum amd_sched_priority amdgpu_to_sched_priority(int amdgpu_priority)
> +{
> + switch (amdgpu_priority) {
> + case AMDGPU_CTX_PRIORITY_HIGH:
> + return AMD_SCHED_PRIORITY_HIGH;
> + case AMDGPU_CTX_PRIORITY_NORMAL:
> + return AMD_SCHED_PRIORITY_NORMAL;
> + default:
> + WARN(1, "Invalid context priority %d\n", amdgpu_priority);
> + return AMD_SCHED_PRIORITY_NORMAL;
> + }
> +}
> +
> +static int amdgpu_ctx_init(struct amdgpu_device *adev,
> + uint32_t priority,
> + struct amdgpu_ctx *ctx)
> {
> unsigned i, j;
> int r;
> + enum amd_sched_priority sched_priority;
> +
> + sched_priority = amdgpu_to_sched_priority(priority);
> +
This will trigger dmesg spam on normal user input. I'd keep the WARN
in amdgpu_to_sched_priority, but move the function call after the
validation of priority.
Thinking about it the input validation really belongs in the ioctl -
amdgpu_ctx_ioctl().
> + if (priority >= AMDGPU_CTX_PRIORITY_NUM)
> + return -EINVAL;
> +
> + if (sched_priority < 0 || sched_priority >= AMD_SCHED_MAX_PRIORITY)
> + return -EINVAL;
> +
> + if (sched_priority == AMD_SCHED_PRIORITY_HIGH && !capable(CAP_SYS_ADMIN))
This is not obvious neither in the commit message nor the UAPI. I'd
suggest adding a comment in the latter.
If memory is not failing - high prio will _not_ work with render nodes
so you really want to cover and/or explain why.
> + return -EACCES;
>
> memset(ctx, 0, sizeof(*ctx));
> ctx->adev = adev;
> + ctx->priority = priority;
> kref_init(&ctx->refcount);
> spin_lock_init(&ctx->ring_lock);
> ctx->fences = kcalloc(amdgpu_sched_jobs * AMDGPU_MAX_RINGS,
> sizeof(struct dma_fence*), GFP_KERNEL);
> if (!ctx->fences)
> return -ENOMEM;
>
> for (i = 0; i < AMDGPU_MAX_RINGS; ++i) {
> ctx->rings[i].sequence = 1;
> ctx->rings[i].fences = &ctx->fences[amdgpu_sched_jobs * i];
> }
>
> ctx->reset_counter = atomic_read(&adev->gpu_reset_counter);
>
> /* create context entity for each ring */
> for (i = 0; i < adev->num_rings; i++) {
> struct amdgpu_ring *ring = adev->rings[i];
> struct amd_sched_rq *rq;
>
> - rq = &ring->sched.sched_rq[AMD_SCHED_PRIORITY_NORMAL];
> + rq = &ring->sched.sched_rq[sched_priority];
> r = amd_sched_entity_init(&ring->sched, &ctx->rings[i].entity,
> rq, amdgpu_sched_jobs);
> if (r)
> goto failed;
> }
>
> return 0;
>
> failed:
> for (j = 0; j < i; j++)
> @@ -83,39 +111,41 @@ static void amdgpu_ctx_fini(struct amdgpu_ctx *ctx)
> kfree(ctx->fences);
> ctx->fences = NULL;
>
> for (i = 0; i < adev->num_rings; i++)
> amd_sched_entity_fini(&adev->rings[i]->sched,
> &ctx->rings[i].entity);
> }
>
> static int amdgpu_ctx_alloc(struct amdgpu_device *adev,
> struct amdgpu_fpriv *fpriv,
> + uint32_t priority,
> uint32_t *id)
> {
> struct amdgpu_ctx_mgr *mgr = &fpriv->ctx_mgr;
> struct amdgpu_ctx *ctx;
> int r;
>
> ctx = kmalloc(sizeof(*ctx), GFP_KERNEL);
> if (!ctx)
> return -ENOMEM;
>
> mutex_lock(&mgr->lock);
> r = idr_alloc(&mgr->ctx_handles, ctx, 1, 0, GFP_KERNEL);
> if (r < 0) {
> mutex_unlock(&mgr->lock);
> kfree(ctx);
> return r;
> }
> +
> *id = (uint32_t)r;
> - r = amdgpu_ctx_init(adev, ctx);
> + r = amdgpu_ctx_init(adev, priority, ctx);
> if (r) {
> idr_remove(&mgr->ctx_handles, *id);
> *id = 0;
> kfree(ctx);
> }
> mutex_unlock(&mgr->lock);
> return r;
> }
>
> static void amdgpu_ctx_do_release(struct kref *ref)
> @@ -179,32 +209,33 @@ static int amdgpu_ctx_query(struct amdgpu_device *adev,
> ctx->reset_counter = reset_counter;
>
> mutex_unlock(&mgr->lock);
> return 0;
> }
>
> int amdgpu_ctx_ioctl(struct drm_device *dev, void *data,
> struct drm_file *filp)
> {
> int r;
> - uint32_t id;
> + uint32_t id, priority;
>
> union drm_amdgpu_ctx *args = data;
> struct amdgpu_device *adev = dev->dev_private;
> struct amdgpu_fpriv *fpriv = filp->driver_priv;
>
> r = 0;
> id = args->in.ctx_id;
> + priority = args->in.priority;
>
Hmm we don't seem to be doing any in.flags validation - not cool.
Someone seriously wants to add that and check the remaining ioctls.
At the same time - I think you want to add a flag bit "HAS_PRIORITY"
[or similar] and honour in.priority only when that is set.
Even if the USM drivers are safe, this will break on a poor soul that
is learning how to program their GPU. "My program was running before -
I updated the kernel and it no longer does :-("
Either way, the patch is:
Reviewed-by: Emil Velikov <emil.l.velikov at gmail.com>
-Emil
