Messages in this thread | | | From | Oded Gabbay <> | Date | Mon, 28 Jan 2019 15:51:15 +0200 | Subject | Re: [PATCH 11/15] habanalabs: add command submission module |
| |
On Sun, Jan 27, 2019 at 5:11 PM Mike Rapoport <rppt@linux.ibm.com> wrote: > > On Wed, Jan 23, 2019 at 02:00:53AM +0200, Oded Gabbay wrote: > > This patch adds the main flow for the user to submit work to the device. > > > > Each work is described by a command submission object (CS). The CS contains > > 3 arrays of command buffers: One for execution, and two for context-switch > > (store and restore). > > > > For each CB, the user specifies on which queue to put that CB. In case of > > an internal queue, the entry doesn't contain a pointer to the CB but the > > address in the on-chip memory that the CB resides at. > > > > The driver parses some of the CBs to enforce security restrictions. > > > > The user receives a sequence number that represents the CS object. The user > > can then query the driver regarding the status of the CS, using that > > sequence number. > > > > In case the CS doesn't finish before the timeout expires, the driver will > > perform a soft-reset of the device. > > > > Signed-off-by: Oded Gabbay <oded.gabbay@gmail.com> > > --- > > drivers/misc/habanalabs/Makefile | 3 +- > > drivers/misc/habanalabs/command_submission.c | 787 +++++++++++++ > > drivers/misc/habanalabs/context.c | 52 +- > > drivers/misc/habanalabs/device.c | 16 + > > drivers/misc/habanalabs/goya/goya.c | 1082 ++++++++++++++++++ > > drivers/misc/habanalabs/habanalabs.h | 274 +++++ > > drivers/misc/habanalabs/habanalabs_drv.c | 23 + > > drivers/misc/habanalabs/habanalabs_ioctl.c | 4 +- > > drivers/misc/habanalabs/hw_queue.c | 250 ++++ > > drivers/misc/habanalabs/memory.c | 200 ++++ > > include/uapi/misc/habanalabs.h | 158 ++- > > 11 files changed, 2842 insertions(+), 7 deletions(-) > > create mode 100644 drivers/misc/habanalabs/command_submission.c > > create mode 100644 drivers/misc/habanalabs/memory.c > > > > diff --git a/drivers/misc/habanalabs/Makefile b/drivers/misc/habanalabs/Makefile > > index b5607233d216..d2fd0e18b1eb 100644 > > --- a/drivers/misc/habanalabs/Makefile > > +++ b/drivers/misc/habanalabs/Makefile > > @@ -5,7 +5,8 @@ > > obj-m := habanalabs.o > > > > habanalabs-y := habanalabs_drv.o device.o context.o asid.o habanalabs_ioctl.o \ > > - command_buffer.o hw_queue.o irq.o sysfs.o hwmon.o > > + command_buffer.o hw_queue.o irq.o sysfs.o hwmon.o memory.o \ > > + command_submission.o > > > > include $(src)/goya/Makefile > > habanalabs-y += $(HL_GOYA_FILES) > > diff --git a/drivers/misc/habanalabs/command_submission.c b/drivers/misc/habanalabs/command_submission.c > > new file mode 100644 > > index 000000000000..0116c2262f17 > > --- /dev/null > > +++ b/drivers/misc/habanalabs/command_submission.c > > @@ -0,0 +1,787 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > + > > +/* > > + * Copyright 2016-2018 HabanaLabs, Ltd. > > + * All Rights Reserved. > > + */ > > + > > +#include <uapi/misc/habanalabs.h> > > +#include "habanalabs.h" > > + > > +#include <linux/sched/mm.h> > > +#include <linux/sched/task.h> > > +#include <linux/sched/signal.h> > > +#include <linux/wait.h> > > +#include <linux/mm.h> > > +#include <linux/highmem.h> > > [ ... ] > > > +static void cs_do_release(struct kref *ref) > > +{ > > + struct hl_cs *cs = container_of(ref, struct hl_cs, > > + refcount); > > + struct hl_device *hdev = cs->ctx->hdev; > > + struct hl_cs_job *job, *tmp; > > + > > + cs->completed = true; > > + > > + /* > > + * Although if we reached here it means that all external jobs have > > + * finished, because each one of them took refcnt to CS, we still > > + * need to go over the internal jobs and free them. Otherwise, we > > + * will have leaked memory and what's worse, the CS object (and > > + * potentially the CTX object) could be released, while the JOB > > + * still holds a pointer to them (but no reference). > > + */ > > + list_for_each_entry_safe(job, tmp, &cs->job_list, cs_node) > > + free_job(hdev, job); > > + > > + /* We also need to update CI for internal queues */ > > + if (cs->submitted) { > > + hl_int_hw_queue_update_ci(cs); > > + > > + spin_lock(&hdev->hw_queues_mirror_lock); > > + /* remove CS from hw_queues mirror list */ > > + list_del_init(&cs->mirror_node); > > + spin_unlock(&hdev->hw_queues_mirror_lock); > > + > > + /* > > + * Don't cancel TDR in case this CS was timedout because we > > + * might be running from the TDR context > > + */ > > + if ((!cs->timedout) && > > + (hdev->timeout_jiffies != MAX_SCHEDULE_TIMEOUT)) { > > + struct hl_cs *next; > > + > > + if (cs->tdr_active) > > + cancel_delayed_work_sync(&cs->work_tdr); > > + > > + spin_lock(&hdev->hw_queues_mirror_lock); > > + /* queue TDR for next CS */ > > + next = list_first_entry_or_null( > > + &hdev->hw_queues_mirror_list, > > + struct hl_cs, mirror_node); > > + if ((next) && (!next->tdr_active)) { > > + next->tdr_active = true; > > + schedule_delayed_work(&next->work_tdr, > > + hdev->timeout_jiffies); > > + spin_unlock(&hdev->hw_queues_mirror_lock); > > + } else { > > + spin_unlock(&hdev->hw_queues_mirror_lock); > > + } > > 'else' can be dropped, just move spin_unlock() outside the 'if' > Fixed > > + } > > + } > > + > > + hl_ctx_put(cs->ctx); > > + > > + if (cs->timedout) > > + dma_fence_set_error(cs->fence, -ETIMEDOUT); > > + else if (cs->aborted) > > + dma_fence_set_error(cs->fence, -EIO); > > + > > + dma_fence_signal(cs->fence); > > + dma_fence_put(cs->fence); > > + > > + kfree(cs); > > +} > > [ ... ] > > > +static int allocate_cs(struct hl_device *hdev, struct hl_ctx *ctx, > > + struct hl_cs **cs_new) > > +{ > > + struct hl_dma_fence *fence; > > + struct dma_fence *other = NULL; > > + struct hl_cs *cs; > > + int rc; > > + > > + cs = kzalloc(sizeof(*cs), GFP_ATOMIC); > > + if (!cs) > > + return -ENOMEM; > > Does this ever run from a context that cannot use GFP_KERNEL? > This applies to other allocations below. > It *always* runs from a context that cannot use GFP_KERNEL, because we mustn't sleep during command submission due to low latency requirements.
> > + > > + cs->ctx = ctx; > > + cs->submitted = false; > > + cs->completed = false; > > + INIT_LIST_HEAD(&cs->job_list); > > + INIT_DELAYED_WORK(&cs->work_tdr, cs_timedout); > > + kref_init(&cs->refcount); > > + spin_lock_init(&cs->job_lock); > > + > > + fence = kmalloc(sizeof(*fence), GFP_ATOMIC); > > kzalloc? Can't waste time on memset
> > > + if (!fence) { > > + rc = -ENOMEM; > > + goto free_cs; > > + } > > + > > + fence->hdev = hdev; > > + spin_lock_init(&fence->lock); > > + cs->fence = &fence->base_fence; > > + > > + spin_lock(&ctx->cs_lock); > > + > > + fence->cs_seq = ctx->cs_sequence; > > + other = ctx->cs_pending[fence->cs_seq & (HL_MAX_PENDING_CS - 1)]; > > + if ((other) && (!dma_fence_is_signaled(other))) { > > + spin_unlock(&ctx->cs_lock); > > + rc = -EAGAIN; > > + goto free_fence; > > + } > > + > > + dma_fence_init(&fence->base_fence, &hl_fence_ops, &fence->lock, > > + ctx->asid, ctx->cs_sequence); > > + > > + cs->sequence = fence->cs_seq; > > + > > + ctx->cs_pending[fence->cs_seq & (HL_MAX_PENDING_CS - 1)] = > > + &fence->base_fence; > > + ctx->cs_sequence++; > > + > > + dma_fence_get(&fence->base_fence); > > + > > + dma_fence_put(other); > > + > > + spin_unlock(&ctx->cs_lock); > > + > > + *cs_new = cs; > > + > > + return 0; > > + > > +free_fence: > > + kfree(fence); > > +free_cs: > > + kfree(cs); > > + return rc; > > +} > > + > > [ ... ] > > > + > > +static int goya_validate_cb(struct hl_device *hdev, > > + struct hl_cs_parser *parser, bool is_mmu) > > +{ > > + u32 cb_parsed_length = 0; > > + int rc = 0; > > + > > + parser->patched_cb_size = 0; > > + > > + /* cb_user_size is more than 0 so loop will always be executed */ > > + while ((cb_parsed_length < parser->user_cb_size) && (!rc)) { > > + enum packet_id pkt_id; > > + u16 pkt_size; > > + void *user_pkt; > > + > > + user_pkt = (void *) (parser->user_cb->kernel_address + > > + cb_parsed_length); > > + > > + pkt_id = (enum packet_id) (((*(u64 *) user_pkt) & > > + PACKET_HEADER_PACKET_ID_MASK) >> > > + PACKET_HEADER_PACKET_ID_SHIFT); > > + > > + pkt_size = goya_packet_sizes[pkt_id]; > > + cb_parsed_length += pkt_size; > > + if (cb_parsed_length > parser->user_cb_size) { > > + dev_err(hdev->dev, > > + "packet 0x%x is out of CB boundary\n", pkt_id); > > + rc = -EINVAL; > > + continue; > > For me !rc in the while statement was blind. Please consider break here and > > if (!rc) > break; > > after the switch Fixed > > > + } > > + > > + switch (pkt_id) { > > + case PACKET_WREG_32: > > + /* > > + * Although it is validated after copy in patch_cb(), > > + * need to validate here as well because patch_cb() is > > + * not called in MMU path while this function is called > > + */ > > + rc = goya_validate_wreg32(hdev, parser, user_pkt); > > + break; > > + > > + case PACKET_WREG_BULK: > > + dev_err(hdev->dev, > > + "User not allowed to use WREG_BULK\n"); > > + rc = -EPERM; > > + break; > > + > > + case PACKET_MSG_PROT: > > + dev_err(hdev->dev, > > + "User not allowed to use MSG_PROT\n"); > > + rc = -EPERM; > > + break; > > + > > + case PACKET_CP_DMA: > > + dev_err(hdev->dev, "User not allowed to use CP_DMA\n"); > > + rc = -EPERM; > > + break; > > + > > + case PACKET_STOP: > > + dev_err(hdev->dev, "User not allowed to use STOP\n"); > > + rc = -EPERM; > > + break; > > + > > + case PACKET_LIN_DMA: > > + if (is_mmu) > > + rc = goya_validate_dma_pkt_mmu(hdev, parser, > > + user_pkt); > > + else > > + rc = goya_validate_dma_pkt_no_mmu(hdev, parser, > > + user_pkt); > > + break; > > + > > + case PACKET_MSG_LONG: > > + case PACKET_MSG_SHORT: > > + case PACKET_FENCE: > > + case PACKET_NOP: > > + parser->patched_cb_size += pkt_size; > > + break; > > + > > + default: > > + dev_err(hdev->dev, "Invalid packet header 0x%x\n", > > + pkt_id); > > + rc = -EINVAL; > > + break; > > + } > > + } > > + > > + /* > > + * The new CB should have space at the end for two MSG_PROT packets: > > + * 1. A packet that will act as a completion packet > > + * 2. A packet that will generate MSI-X interrupt > > + */ > > + parser->patched_cb_size += sizeof(struct packet_msg_prot) * 2; > > + > > + return rc; > > +} > > [ ... ] > > > +static int goya_patch_cb(struct hl_device *hdev, > > + struct hl_cs_parser *parser) > > +{ > > + u32 cb_parsed_length = 0; > > + u32 cb_patched_cur_length = 0; > > + int rc = 0; > > + > > + /* cb_user_size is more than 0 so loop will always be executed */ > > + while ((cb_parsed_length < parser->user_cb_size) && (!rc)) { > > + enum packet_id pkt_id; > > + u16 pkt_size; > > + u32 new_pkt_size = 0; > > + void *user_pkt, *kernel_pkt; > > + > > + user_pkt = (void *) (parser->user_cb->kernel_address + > > + cb_parsed_length); > > + kernel_pkt = (void *) (parser->patched_cb->kernel_address + > > + cb_patched_cur_length); > > + > > + pkt_id = (enum packet_id) (((*(u64 *) user_pkt) & > > + PACKET_HEADER_PACKET_ID_MASK) >> > > + PACKET_HEADER_PACKET_ID_SHIFT); > > + > > + pkt_size = goya_packet_sizes[pkt_id]; > > + cb_parsed_length += pkt_size; > > + if (cb_parsed_length > parser->user_cb_size) { > > + dev_err(hdev->dev, > > + "packet 0x%x is out of CB boundary\n", pkt_id); > > + rc = -EINVAL; > > + continue; > > Ditto Fixed > > > + } > > + > > + switch (pkt_id) { > > + case PACKET_LIN_DMA: > > + rc = goya_patch_dma_packet(hdev, parser, user_pkt, > > + kernel_pkt, &new_pkt_size); > > + cb_patched_cur_length += new_pkt_size; > > + break; > > + > > + case PACKET_WREG_32: > > + memcpy(kernel_pkt, user_pkt, pkt_size); > > + cb_patched_cur_length += pkt_size; > > + rc = goya_validate_wreg32(hdev, parser, kernel_pkt); > > + break; > > + > > + case PACKET_WREG_BULK: > > + dev_err(hdev->dev, > > + "User not allowed to use WREG_BULK\n"); > > + rc = -EPERM; > > + break; > > + > > + case PACKET_MSG_PROT: > > + dev_err(hdev->dev, > > + "User not allowed to use MSG_PROT\n"); > > + rc = -EPERM; > > + break; > > + > > + case PACKET_CP_DMA: > > + dev_err(hdev->dev, "User not allowed to use CP_DMA\n"); > > + rc = -EPERM; > > + break; > > + > > + case PACKET_STOP: > > + dev_err(hdev->dev, "User not allowed to use STOP\n"); > > + rc = -EPERM; > > + break; > > + > > + case PACKET_MSG_LONG: > > + case PACKET_MSG_SHORT: > > + case PACKET_FENCE: > > + case PACKET_NOP: > > + memcpy(kernel_pkt, user_pkt, pkt_size); > > + cb_patched_cur_length += pkt_size; > > + break; > > + > > + default: > > + dev_err(hdev->dev, "Invalid packet header 0x%x\n", > > + pkt_id); > > + rc = -EINVAL; > > + break; > > + } > > + } > > + > > + return rc; > > +} > > [ ... ] > > > static void goya_get_axi_name(struct hl_device *hdev, u32 agent_id, > > u16 event_type, char *axi_name, int len) > > { > > @@ -4645,6 +5677,48 @@ static void goya_disable_clock_gating(struct hl_device *hdev) > > > > } > > > > +static bool goya_is_device_idle(struct hl_device *hdev) > > +{ > > + u64 offset, dma_qm_reg, tpc_qm_reg, tpc_cmdq_reg, tpc_cfg_reg; > > + bool val = true; > > + int i; > > + > > + offset = mmDMA_QM_1_GLBL_STS0 - mmDMA_QM_0_GLBL_STS0; > > + > > + for (i = 0 ; i < DMA_MAX_NUM ; i++) { > > + dma_qm_reg = mmDMA_QM_0_GLBL_STS0 + i * offset; > > + > > + val = val && ((RREG32(dma_qm_reg) & DMA_QM_IDLE_MASK) == > > + DMA_QM_IDLE_MASK); > > + } > > + > > + offset = mmTPC1_QM_GLBL_STS0 - mmTPC0_QM_GLBL_STS0; > > + > > + for (i = 0 ; i < TPC_MAX_NUM ; i++) { > > + tpc_qm_reg = mmTPC0_QM_GLBL_STS0 + i * offset; > > + tpc_cmdq_reg = mmTPC0_CMDQ_GLBL_STS0 + i * offset; > > + tpc_cfg_reg = mmTPC0_CFG_STATUS + i * offset; > > + > > + val = val && ((RREG32(tpc_qm_reg) & TPC_QM_IDLE_MASK) == > > + TPC_QM_IDLE_MASK); > > + val = val && ((RREG32(tpc_cmdq_reg) & TPC_CMDQ_IDLE_MASK) == > > + TPC_CMDQ_IDLE_MASK); > > + val = val && ((RREG32(tpc_cfg_reg) & TPC_CFG_IDLE_MASK) == > > + TPC_CFG_IDLE_MASK); > > + } > > + > > + val = val && ((RREG32(mmMME_QM_GLBL_STS0) & MME_QM_IDLE_MASK) == > > + MME_QM_IDLE_MASK); > > + val = val && ((RREG32(mmMME_CMDQ_GLBL_STS0) & MME_CMDQ_IDLE_MASK) == > > + MME_CMDQ_IDLE_MASK); > > + val = val && ((RREG32(mmMME_ARCH_STATUS) & MME_ARCH_IDLE_MASK) == > > + MME_ARCH_IDLE_MASK); > > + val = val && ((RREG32(mmMME_SHADOW_0_STATUS) & MME_SHADOW_IDLE_MASK) == > > + 0); > > Huh, these are neat, but IMHO plain > > if ((RREG(reg) & mask) != mask) > return false; > > are more readable... > Fixed > > + > > + return val; > > +} > > + > > [ ... ] > > -- > Sincerely yours, > Mike. >
| |