On Fri, 2 Jul 2021 10:56:29 +0100
Steven Price <steven.price@xxxxxxx> wrote:
> On 01/07/2021 10:12, Boris Brezillon wrote:
> > Needed to keep VkQueues isolated from each other.
> >
> > Signed-off-by: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx>
>
> My Vulkan knowledge is limited so I'm not sure whether this is the right
> approach or not. In particular is it correct that an application can
> create a high priority queue which could affect other (normal priority)
> applications?
That's what msm does (with no extra CAPS check AFAICT), and the
freedreno driver can already create high priority queues if
PIPE_CONTEXT_HIGH_PRIORITY is passed. Not saying that's okay to allow
userspace to tweak the priority, but if that's a problem, other drivers
are in trouble too ;-).
>
> Also does it really make sense to allow user space to create an
> unlimited number of queues? It feels like an ideal way for an malicious
> application to waste kernel memory.
Same here, I see no limit on the number of queues the msm driver can
create. I can definitely pick an arbitrary limit of 2^16 (or 2^8 if
2^16 is too high) if you prefer, but I feel like there's plenty of ways
to force kernel allocations already, like allocating a gazillion of 4k
GEM buffers (cgroup can probably limit the total amount of memory
allocated, but you'd still have all gem-buf meta data in kernel memory).
>
> In terms of implementation it looks correct, but one comment below
>
> > ---
> > drivers/gpu/drm/panfrost/Makefile | 3 +-
> > drivers/gpu/drm/panfrost/panfrost_device.h | 2 +-
> > drivers/gpu/drm/panfrost/panfrost_drv.c | 69 ++++++++--
> > drivers/gpu/drm/panfrost/panfrost_job.c | 47 ++-----
> > drivers/gpu/drm/panfrost/panfrost_job.h | 9 +-
> > .../gpu/drm/panfrost/panfrost_submitqueue.c | 130 ++++++++++++++++++
> > .../gpu/drm/panfrost/panfrost_submitqueue.h | 27 ++++
> > include/uapi/drm/panfrost_drm.h | 17 +++
> > 8 files changed, 258 insertions(+), 46 deletions(-)
> > create mode 100644 drivers/gpu/drm/panfrost/panfrost_submitqueue.c
> > create mode 100644 drivers/gpu/drm/panfrost/panfrost_submitqueue.h
> >
> [...]
> > diff --git a/drivers/gpu/drm/panfrost/panfrost_submitqueue.c b/drivers/gpu/drm/panfrost/panfrost_submitqueue.c
> > new file mode 100644
> > index 000000000000..98050f7690df
> > --- /dev/null
> > +++ b/drivers/gpu/drm/panfrost/panfrost_submitqueue.c
> > @@ -0,0 +1,130 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/* Copyright 2021 Collabora ltd. */
> > +
> > +#include <linux/idr.h>
> > +
> > +#include "panfrost_device.h"
> > +#include "panfrost_job.h"
> > +#include "panfrost_submitqueue.h"
> > +
> > +static enum drm_sched_priority
> > +to_sched_prio(enum panfrost_submitqueue_priority priority)
> > +{
> > + switch (priority) {
> > + case PANFROST_SUBMITQUEUE_PRIORITY_LOW:
> > + return DRM_SCHED_PRIORITY_MIN;
> > + case PANFROST_SUBMITQUEUE_PRIORITY_MEDIUM:
> > + return DRM_SCHED_PRIORITY_NORMAL;
> > + case PANFROST_SUBMITQUEUE_PRIORITY_HIGH:
> > + return DRM_SCHED_PRIORITY_HIGH;
> > + default:
> > + break;
> > + }
> > +
> > + return DRM_SCHED_PRIORITY_UNSET;
> > +}
> > +
> > +static void
> > +panfrost_submitqueue_cleanup(struct kref *ref)
> > +{
> > + struct panfrost_submitqueue *queue;
> > + unsigned int i;
> > +
> > + queue = container_of(ref, struct panfrost_submitqueue, refcount);
> > +
> > + for (i = 0; i < NUM_JOB_SLOTS; i++)
> > + drm_sched_entity_destroy(&queue->sched_entity[i]);
> > +
> > + /* Kill in-flight jobs */
> > + panfrost_job_kill_queue(queue);
> > +
> > + kfree(queue);
> > +}
> > +
> > +void panfrost_submitqueue_put(struct panfrost_submitqueue *queue)
> > +{
> > + if (!IS_ERR_OR_NULL(queue))
> > + kref_put(&queue->refcount, panfrost_submitqueue_cleanup);
> > +}
> > +
> > +struct panfrost_submitqueue *
> > +panfrost_submitqueue_create(struct panfrost_file_priv *ctx,
> > + enum panfrost_submitqueue_priority priority,
> > + u32 flags)
>
> If this function returned an 'int' we could simplify some code. So
> instead of returning the struct panfrost_submitqueue just return the ID
> (or negative error). The only caller (panfrost_ioctl_create_submitqueue)
> doesn't actually want the object just the ID and we can ditch the 'id'
> field from panfrost_submitqueue.
Sure, I can do that.
