[PATCH v2 4/7] drm/panfrost: Add the ability to create submit queues

[Steven Price]

On 02/07/2021 11:43, Boris Brezillon wrote:
> On Fri, 2 Jul 2021 10:56:29 +0100
> Steven Price <steven.price at arm.com> 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 at collabora.com>  
>>
>> 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 ;-).

Oh well I guess if others are doing the same ;) I have to admit kbase
has always struggled with how to identify a "privileged" process - it's
something that makes a bit of sense on Android but for other userspaces
there really doesn't seem to be a good way of identifying what should or
should not be allowed to create high priority queues.

>>
>> 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).

I guess the real problem is picking a sensible limit ;) My main concern
here is that there doesn't appear to be any memory accounted against the
process. For GEM buffers at least there is some cost to the application
- so an unbounded allocation isn't possible, even if the bounds are
likely to be very high.

With kbase we found that syzcaller was good at finding ways of using up
all the memory on the platform - and if it wasn't accounted to the right
process that meant the OOM-killer knocked out the wrong process and
produced a bug report to investigate. Perhaps I'm just scarred by that
history ;)

Steve

>>
>> 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.
>