Hi, Jonathan
On 2019/10/14 下午6:32, Jonathan Cameron wrote:
On Mon, 14 Oct 2019 14:48:54 +0800
Zhangfei Gao <zhangfei.gao@xxxxxxxxxx> wrote:
From: Kenneth Lee <liguozhu@xxxxxxxxxxxxx>
Uacce (Unified/User-space-access-intended Accelerator Framework) targets to
provide Shared Virtual Addressing (SVA) between accelerators and processes.
So accelerator can access any data structure of the main cpu.
This differs from the data sharing between cpu and io device, which share
data content rather than address.
Since unified address, hardware and user space of process can share the
same virtual address in the communication.
Uacce create a chrdev for every registration, the queue is allocated to
the process when the chrdev is opened. Then the process can access the
hardware resource by interact with the queue file. By mmap the queue
file space to user space, the process can directly put requests to the
hardware without syscall to the kernel space.
Signed-off-by: Kenneth Lee <liguozhu@xxxxxxxxxxxxx>
Signed-off-by: Zaibo Xu <xuzaibo@xxxxxxxxxx>
Signed-off-by: Zhou Wang <wangzhou1@xxxxxxxxxxxxx>
Signed-off-by: Zhangfei Gao <zhangfei.gao@xxxxxxxxxx>
Hi,
Some superficial comments from me.
Thanks for the suggestion.
+/*
+ * While user space releases a queue, all the relatives on the queue
+ * should be released immediately by this putting.
This one needs rewording but I'm not quite sure what
relatives are in this case.
+ */
+static long uacce_put_queue(struct uacce_queue *q)
+{
+ struct uacce_device *uacce = q->uacce;
+
+ mutex_lock(&uacce_mutex);
+
+ if ((q->state == UACCE_Q_STARTED) && uacce->ops->stop_queue)
+ uacce->ops->stop_queue(q);
+
+ if ((q->state == UACCE_Q_INIT || q->state == UACCE_Q_STARTED) &&
+ uacce->ops->put_queue)
+ uacce->ops->put_queue(q);
+
+ q->state = UACCE_Q_ZOMBIE;
+ mutex_unlock(&uacce_mutex);
+
+ return 0;
+}
+
..
+
+static ssize_t qfrs_size_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct uacce_device *uacce = to_uacce_device(dev);
+ unsigned long size;
+ int i, ret;
+
+ for (i = 0, ret = 0; i < UACCE_QFRT_MAX; i++) {
+ size = uacce->qf_pg_size[i] << PAGE_SHIFT;
+ if (i == UACCE_QFRT_SS)
+ break;
+ ret += sprintf(buf + ret, "%lu\t", size);
+ }
+ ret += sprintf(buf + ret, "%lu\n", size);
+
+ return ret;
+}
This may break the sysfs rule of one thing per file. If you have
multiple regions, they should probably each have their own file
to give their size.
Is the rule must be applied?
Documentation/filesystems/sysfs.txt
Attributes should be ASCII text files, preferably with only one value
per file. It is noted that it may not be efficient to contain only one
value per file, so it is socially acceptable to express an array of
values of the same type.
We may have efficiency here.
For extensibility in future, UACCE_QFRT_MAX=16, do we export all of them?
In sva case only 2 region is used, and 4 at most if sva is not supported.
Do you think it is more efficient to put in one file?
+
+/**
+ * uacce_unregister - unregisters a uacce
+ * @uacce: the accelerator to unregister
+ *
+ * Unregister an accelerator that wat previously successully registered with
wat -> was
successully -> successfully
...
+/**
+ * struct uacce_queue
+ * @uacce: pointer to uacce
+ * @priv: private pointer
+ * @wait: wait queue head
+ * @pasid: pasid of the queue
+ * @handle: iommu_sva handle return from iommu_sva_bind_device
+ * @list: share list for qfr->qs
+ * @mm: current->mm
+ * @qfrs: pointer of qfr regions
Missing state. Make sure to run
./scripts/kernel-doc FILENAME > /dev/null and
fix any errors that show up.
Good tool, will use it to check.
Thanks.
