* [PATCH v2 1/7] habanalabs: add handle field to context structure
2019-07-30 9:47 [PATCH v2 0/7] habanalabs: support info queries by multiple processes Oded Gabbay
@ 2019-07-30 9:47 ` Oded Gabbay
2019-07-30 9:47 ` [PATCH v2 2/7] habanalabs: kill user process after CS rollback Oded Gabbay
` (5 subsequent siblings)
6 siblings, 0 replies; 10+ messages in thread
From: Oded Gabbay @ 2019-07-30 9:47 UTC (permalink / raw)
To: linux-kernel, oshpigelman, ttayar, gregkh
This patch adds a field to the context's structure that will hold a unique
handle for the context.
This will be needed when the user will create the context.
Signed-off-by: Oded Gabbay <oded.gabbay@gmail.com>
---
drivers/misc/habanalabs/context.c | 27 ++++++++++++++++-----------
drivers/misc/habanalabs/habanalabs.h | 2 ++
2 files changed, 18 insertions(+), 11 deletions(-)
diff --git a/drivers/misc/habanalabs/context.c b/drivers/misc/habanalabs/context.c
index 8682590e3f6e..1d8390418234 100644
--- a/drivers/misc/habanalabs/context.c
+++ b/drivers/misc/habanalabs/context.c
@@ -67,9 +67,20 @@ int hl_ctx_create(struct hl_device *hdev, struct hl_fpriv *hpriv)
goto out_err;
}
+ mutex_lock(&mgr->ctx_lock);
+ rc = idr_alloc(&mgr->ctx_handles, ctx, 1, 0, GFP_KERNEL);
+ mutex_unlock(&mgr->ctx_lock);
+
+ if (rc < 0) {
+ dev_err(hdev->dev, "Failed to allocate IDR for a new CTX\n");
+ goto free_ctx;
+ }
+
+ ctx->handle = rc;
+
rc = hl_ctx_init(hdev, ctx, false);
if (rc)
- goto free_ctx;
+ goto remove_from_idr;
hl_hpriv_get(hpriv);
ctx->hpriv = hpriv;
@@ -78,18 +89,12 @@ int hl_ctx_create(struct hl_device *hdev, struct hl_fpriv *hpriv)
hpriv->ctx = ctx;
hdev->user_ctx = ctx;
- mutex_lock(&mgr->ctx_lock);
- rc = idr_alloc(&mgr->ctx_handles, ctx, 1, 0, GFP_KERNEL);
- mutex_unlock(&mgr->ctx_lock);
-
- if (rc < 0) {
- dev_err(hdev->dev, "Failed to allocate IDR for a new CTX\n");
- hl_ctx_free(hdev, ctx);
- goto out_err;
- }
-
return 0;
+remove_from_idr:
+ mutex_lock(&mgr->ctx_lock);
+ idr_remove(&mgr->ctx_handles, ctx->handle);
+ mutex_unlock(&mgr->ctx_lock);
free_ctx:
kfree(ctx);
out_err:
diff --git a/drivers/misc/habanalabs/habanalabs.h b/drivers/misc/habanalabs/habanalabs.h
index 57183ae9b95d..e41800e68578 100644
--- a/drivers/misc/habanalabs/habanalabs.h
+++ b/drivers/misc/habanalabs/habanalabs.h
@@ -631,6 +631,7 @@ struct hl_va_range {
* execution phase before the context switch phase
* has finished.
* @asid: context's unique address space ID in the device's MMU.
+ * @handle: context's opaque handle for user
*/
struct hl_ctx {
DECLARE_HASHTABLE(mem_hash, MEM_HASH_TABLE_BITS);
@@ -652,6 +653,7 @@ struct hl_ctx {
atomic_t thread_ctx_switch_token;
u32 thread_ctx_switch_wait_token;
u32 asid;
+ u32 handle;
};
/**
--
2.17.1
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH v2 2/7] habanalabs: kill user process after CS rollback
2019-07-30 9:47 [PATCH v2 0/7] habanalabs: support info queries by multiple processes Oded Gabbay
2019-07-30 9:47 ` [PATCH v2 1/7] habanalabs: add handle field to context structure Oded Gabbay
@ 2019-07-30 9:47 ` Oded Gabbay
2019-07-30 9:47 ` [PATCH v2 3/7] habanalabs: show the process context dram usage Oded Gabbay
` (4 subsequent siblings)
6 siblings, 0 replies; 10+ messages in thread
From: Oded Gabbay @ 2019-07-30 9:47 UTC (permalink / raw)
To: linux-kernel, oshpigelman, ttayar, gregkh
This patch calls the kill user process function after we rollback the
in-flight CSs. This is because the user process can't be closed while
there are open CSs. Therefore, there is no point of sending it a SIGKILL
before we do the rollback CS part.
Signed-off-by: Oded Gabbay <oded.gabbay@gmail.com>
---
drivers/misc/habanalabs/device.c | 17 +++++++++++++----
1 file changed, 13 insertions(+), 4 deletions(-)
diff --git a/drivers/misc/habanalabs/device.c b/drivers/misc/habanalabs/device.c
index 0c4894dd9c02..d1bc8f4ed5bb 100644
--- a/drivers/misc/habanalabs/device.c
+++ b/drivers/misc/habanalabs/device.c
@@ -630,8 +630,6 @@ static void device_hard_reset_pending(struct work_struct *work)
container_of(work, struct hl_device_reset_work, reset_work);
struct hl_device *hdev = device_reset_work->hdev;
- device_kill_open_processes(hdev);
-
hl_device_reset(hdev, true, true);
kfree(device_reset_work);
@@ -736,6 +734,13 @@ int hl_device_reset(struct hl_device *hdev, bool hard_reset,
/* Go over all the queues, release all CS and their jobs */
hl_cs_rollback_all(hdev);
+ /* Kill processes here after CS rollback. This is because the process
+ * can't really exit until all its CSs are done, which is what we
+ * do in cs rollback
+ */
+ if (from_hard_reset_thread)
+ device_kill_open_processes(hdev);
+
/* Release kernel context */
if ((hard_reset) && (hl_ctx_put(hdev->kernel_ctx) == 1))
hdev->kernel_ctx = NULL;
@@ -1130,8 +1135,6 @@ void hl_device_fini(struct hl_device *hdev)
hdev->hard_reset_pending = true;
- device_kill_open_processes(hdev);
-
hl_hwmon_fini(hdev);
device_late_fini(hdev);
@@ -1150,6 +1153,12 @@ void hl_device_fini(struct hl_device *hdev)
/* Go over all the queues, release all CS and their jobs */
hl_cs_rollback_all(hdev);
+ /* Kill processes here after CS rollback. This is because the process
+ * can't really exit until all its CSs are done, which is what we
+ * do in cs rollback
+ */
+ device_kill_open_processes(hdev);
+
hl_cb_pool_fini(hdev);
/* Release kernel context */
--
2.17.1
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH v2 3/7] habanalabs: show the process context dram usage
2019-07-30 9:47 [PATCH v2 0/7] habanalabs: support info queries by multiple processes Oded Gabbay
2019-07-30 9:47 ` [PATCH v2 1/7] habanalabs: add handle field to context structure Oded Gabbay
2019-07-30 9:47 ` [PATCH v2 2/7] habanalabs: kill user process after CS rollback Oded Gabbay
@ 2019-07-30 9:47 ` Oded Gabbay
2019-07-30 9:47 ` [PATCH v2 4/7] habanalabs: rename user_ctx as compute_ctx Oded Gabbay
` (3 subsequent siblings)
6 siblings, 0 replies; 10+ messages in thread
From: Oded Gabbay @ 2019-07-30 9:47 UTC (permalink / raw)
To: linux-kernel, oshpigelman, ttayar, gregkh
When the user query the dram usage of a context, show it the dram usage of
its context, not the user context that is currently running on the device.
This has no effect right now as we only have a single process and a single
context, but this makes the code more ready for multiple process support.
Signed-off-by: Oded Gabbay <oded.gabbay@gmail.com>
---
drivers/misc/habanalabs/habanalabs_ioctl.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/drivers/misc/habanalabs/habanalabs_ioctl.c b/drivers/misc/habanalabs/habanalabs_ioctl.c
index 07127576b3e8..c9a4799eb251 100644
--- a/drivers/misc/habanalabs/habanalabs_ioctl.c
+++ b/drivers/misc/habanalabs/habanalabs_ioctl.c
@@ -89,8 +89,9 @@ static int hw_events_info(struct hl_device *hdev, struct hl_info_args *args)
return copy_to_user(out, arr, min(max_size, size)) ? -EFAULT : 0;
}
-static int dram_usage_info(struct hl_device *hdev, struct hl_info_args *args)
+static int dram_usage_info(struct hl_fpriv *hpriv, struct hl_info_args *args)
{
+ struct hl_device *hdev = hpriv->hdev;
struct hl_info_dram_usage dram_usage = {0};
u32 max_size = args->return_size;
void __user *out = (void __user *) (uintptr_t) args->return_pointer;
@@ -104,7 +105,9 @@ static int dram_usage_info(struct hl_device *hdev, struct hl_info_args *args)
prop->dram_base_address);
dram_usage.dram_free_mem = (prop->dram_size - dram_kmd_size) -
atomic64_read(&hdev->dram_used_mem);
- dram_usage.ctx_dram_mem = atomic64_read(&hdev->user_ctx->dram_phys_mem);
+ if (hpriv->ctx)
+ dram_usage.ctx_dram_mem =
+ atomic64_read(&hpriv->ctx->dram_phys_mem);
return copy_to_user(out, &dram_usage,
min((size_t) max_size, sizeof(dram_usage))) ? -EFAULT : 0;
@@ -218,7 +221,7 @@ static int hl_info_ioctl(struct hl_fpriv *hpriv, void *data)
break;
case HL_INFO_DRAM_USAGE:
- rc = dram_usage_info(hdev, args);
+ rc = dram_usage_info(hpriv, args);
break;
case HL_INFO_HW_IDLE:
@@ -321,7 +324,7 @@ long hl_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
cmd = ioctl->cmd;
} else {
dev_err(hdev->dev, "invalid ioctl: pid=%d, nr=0x%02x\n",
- task_pid_nr(current), nr);
+ task_pid_nr(current), nr);
return -ENOTTY;
}
--
2.17.1
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH v2 4/7] habanalabs: rename user_ctx as compute_ctx
2019-07-30 9:47 [PATCH v2 0/7] habanalabs: support info queries by multiple processes Oded Gabbay
` (2 preceding siblings ...)
2019-07-30 9:47 ` [PATCH v2 3/7] habanalabs: show the process context dram usage Oded Gabbay
@ 2019-07-30 9:47 ` Oded Gabbay
2019-07-30 9:47 ` [PATCH v2 5/7] habanalabs: maintain a list of file private data objects Oded Gabbay
` (2 subsequent siblings)
6 siblings, 0 replies; 10+ messages in thread
From: Oded Gabbay @ 2019-07-30 9:47 UTC (permalink / raw)
To: linux-kernel, oshpigelman, ttayar, gregkh
This patch renames the "user_ctx" field in the device structure to
"compute_ctx". This better reflects the meaning of this context.
In addition, we also check in the ctx_fini() that the debug mode should be
disabled only if the context being destroyed is the compute context. This
has no effect right now as we only have a single process and a single
context, but this makes the code more ready for multiple process support.
Signed-off-by: Oded Gabbay <oded.gabbay@gmail.com>
---
drivers/misc/habanalabs/context.c | 13 ++++++++-----
drivers/misc/habanalabs/debugfs.c | 4 ++--
drivers/misc/habanalabs/device.c | 14 +++++++-------
drivers/misc/habanalabs/habanalabs.h | 9 ++++-----
4 files changed, 21 insertions(+), 19 deletions(-)
diff --git a/drivers/misc/habanalabs/context.c b/drivers/misc/habanalabs/context.c
index 1d8390418234..bc0dec57a983 100644
--- a/drivers/misc/habanalabs/context.c
+++ b/drivers/misc/habanalabs/context.c
@@ -26,12 +26,13 @@ static void hl_ctx_fini(struct hl_ctx *ctx)
dma_fence_put(ctx->cs_pending[i]);
if (ctx->asid != HL_KERNEL_ASID_ID) {
- /*
- * The engines are stopped as there is no executing CS, but the
+ /* The engines are stopped as there is no executing CS, but the
* Coresight might be still working by accessing addresses
* related to the stopped engines. Hence stop it explicitly.
+ * Stop only if this is the compute context, as there can be
+ * only one compute context
*/
- if (hdev->in_debug)
+ if ((hdev->in_debug) && (hdev->compute_ctx == ctx))
hl_device_set_debug_mode(hdev, false);
hl_vm_ctx_fini(ctx);
@@ -85,9 +86,11 @@ int hl_ctx_create(struct hl_device *hdev, struct hl_fpriv *hpriv)
hl_hpriv_get(hpriv);
ctx->hpriv = hpriv;
- /* TODO: remove for multiple contexts */
+ /* TODO: remove for multiple contexts per process */
hpriv->ctx = ctx;
- hdev->user_ctx = ctx;
+
+ /* TODO: remove the following line for multiple process support */
+ hdev->compute_ctx = ctx;
return 0;
diff --git a/drivers/misc/habanalabs/debugfs.c b/drivers/misc/habanalabs/debugfs.c
index 18e499c900c7..2b9bc1c41d40 100644
--- a/drivers/misc/habanalabs/debugfs.c
+++ b/drivers/misc/habanalabs/debugfs.c
@@ -370,7 +370,7 @@ static int mmu_show(struct seq_file *s, void *data)
if (dev_entry->mmu_asid == HL_KERNEL_ASID_ID)
ctx = hdev->kernel_ctx;
else
- ctx = hdev->user_ctx;
+ ctx = hdev->compute_ctx;
if (!ctx) {
dev_err(hdev->dev, "no ctx available\n");
@@ -533,7 +533,7 @@ static bool hl_is_device_va(struct hl_device *hdev, u64 addr)
static int device_va_to_pa(struct hl_device *hdev, u64 virt_addr,
u64 *phys_addr)
{
- struct hl_ctx *ctx = hdev->user_ctx;
+ struct hl_ctx *ctx = hdev->compute_ctx;
u64 hop_addr, hop_pte_addr, hop_pte;
u64 offset_mask = HOP4_MASK | OFFSET_MASK;
int rc = 0;
diff --git a/drivers/misc/habanalabs/device.c b/drivers/misc/habanalabs/device.c
index d1bc8f4ed5bb..ca166abee7b5 100644
--- a/drivers/misc/habanalabs/device.c
+++ b/drivers/misc/habanalabs/device.c
@@ -59,7 +59,7 @@ static void hpriv_release(struct kref *ref)
atomic_dec(&hdev->fd_open_cnt);
/* This allows a new user context to open the device */
- hdev->user_ctx = NULL;
+ hdev->compute_ctx = NULL;
}
void hl_hpriv_get(struct hl_fpriv *hpriv)
@@ -590,7 +590,7 @@ static void device_kill_open_processes(struct hl_device *hdev)
}
if (atomic_read(&hdev->fd_open_cnt)) {
- task = get_pid_task(hdev->user_ctx->hpriv->taskpid,
+ task = get_pid_task(hdev->compute_ctx->hpriv->taskpid,
PIDTYPE_PID);
if (task) {
dev_info(hdev->dev, "Killing user processes\n");
@@ -760,9 +760,9 @@ int hl_device_reset(struct hl_device *hdev, bool hard_reset,
hl_cq_reset(hdev, &hdev->completion_queue[i]);
/* Make sure the context switch phase will run again */
- if (hdev->user_ctx) {
- atomic_set(&hdev->user_ctx->thread_ctx_switch_token, 1);
- hdev->user_ctx->thread_ctx_switch_wait_token = 0;
+ if (hdev->compute_ctx) {
+ atomic_set(&hdev->compute_ctx->thread_ctx_switch_token, 1);
+ hdev->compute_ctx->thread_ctx_switch_wait_token = 0;
}
/* Finished tear-down, starting to re-initialize */
@@ -793,7 +793,7 @@ int hl_device_reset(struct hl_device *hdev, bool hard_reset,
goto out_err;
}
- hdev->user_ctx = NULL;
+ hdev->compute_ctx = NULL;
rc = hl_ctx_init(hdev, hdev->kernel_ctx, true);
if (rc) {
@@ -970,7 +970,7 @@ int hl_device_init(struct hl_device *hdev, struct class *hclass)
goto mmu_fini;
}
- hdev->user_ctx = NULL;
+ hdev->compute_ctx = NULL;
rc = hl_ctx_init(hdev, hdev->kernel_ctx, true);
if (rc) {
diff --git a/drivers/misc/habanalabs/habanalabs.h b/drivers/misc/habanalabs/habanalabs.h
index e41800e68578..53f6b238e6f2 100644
--- a/drivers/misc/habanalabs/habanalabs.h
+++ b/drivers/misc/habanalabs/habanalabs.h
@@ -905,7 +905,7 @@ struct hl_debug_params {
* @hdev: habanalabs device structure.
* @filp: pointer to the given file structure.
* @taskpid: current process ID.
- * @ctx: current executing context.
+ * @ctx: current executing context. TODO: remove for multiple ctx per process
* @ctx_mgr: context manager to handle multiple context for this FD.
* @cb_mgr: command buffer manager to handle multiple buffers for this FD.
* @debugfs_list: list of relevant ASIC debugfs.
@@ -916,7 +916,7 @@ struct hl_fpriv {
struct hl_device *hdev;
struct file *filp;
struct pid *taskpid;
- struct hl_ctx *ctx; /* TODO: remove for multiple ctx */
+ struct hl_ctx *ctx;
struct hl_ctx_mgr ctx_mgr;
struct hl_cb_mgr cb_mgr;
struct list_head debugfs_list;
@@ -1182,7 +1182,7 @@ struct hl_device_reset_work {
* @hl_debugfs: device's debugfs manager.
* @cb_pool: list of preallocated CBs.
* @cb_pool_lock: protects the CB pool.
- * @user_ctx: current user context executing.
+ * @compute_ctx: current compute context executing.
* @dram_used_mem: current DRAM memory consumption.
* @timeout_jiffies: device CS timeout value.
* @max_power: the max power of the device, as configured by the sysadmin. This
@@ -1259,8 +1259,7 @@ struct hl_device {
struct list_head cb_pool;
spinlock_t cb_pool_lock;
- /* TODO: remove user_ctx for multiple process support */
- struct hl_ctx *user_ctx;
+ struct hl_ctx *compute_ctx;
atomic64_t dram_used_mem;
u64 timeout_jiffies;
--
2.17.1
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH v2 5/7] habanalabs: maintain a list of file private data objects
2019-07-30 9:47 [PATCH v2 0/7] habanalabs: support info queries by multiple processes Oded Gabbay
` (3 preceding siblings ...)
2019-07-30 9:47 ` [PATCH v2 4/7] habanalabs: rename user_ctx as compute_ctx Oded Gabbay
@ 2019-07-30 9:47 ` Oded Gabbay
2019-07-30 9:47 ` [PATCH v2 6/7] habanalabs: change device_setup_cdev() to be more generic Oded Gabbay
2019-07-30 9:47 ` [PATCH v2 7/7] habanalabs: create two char devices per ASIC Oded Gabbay
6 siblings, 0 replies; 10+ messages in thread
From: Oded Gabbay @ 2019-07-30 9:47 UTC (permalink / raw)
To: linux-kernel, oshpigelman, ttayar, gregkh
This patch adds a new list to the driver's device structure. The list will
keep the file private data structures that the driver creates when a user
process opens the device.
This change is needed because it is useless to try to count how many FD
are open. Instead, track our own private data structure per open file and
once it is released, remove it from the list. As long as the list is not
empty, it means we have a user that can do something with our device.
Signed-off-by: Oded Gabbay <oded.gabbay@gmail.com>
---
drivers/misc/habanalabs/device.c | 132 ++++++++++------------
drivers/misc/habanalabs/goya/goya_hwmgr.c | 11 +-
drivers/misc/habanalabs/habanalabs.h | 24 ++--
drivers/misc/habanalabs/habanalabs_drv.c | 78 +++++++------
4 files changed, 115 insertions(+), 130 deletions(-)
diff --git a/drivers/misc/habanalabs/device.c b/drivers/misc/habanalabs/device.c
index ca166abee7b5..926c85ff068f 100644
--- a/drivers/misc/habanalabs/device.c
+++ b/drivers/misc/habanalabs/device.c
@@ -42,10 +42,12 @@ static void hpriv_release(struct kref *ref)
{
struct hl_fpriv *hpriv;
struct hl_device *hdev;
+ struct hl_ctx *ctx;
hpriv = container_of(ref, struct hl_fpriv, refcount);
hdev = hpriv->hdev;
+ ctx = hpriv->ctx;
put_pid(hpriv->taskpid);
@@ -53,13 +55,12 @@ static void hpriv_release(struct kref *ref)
mutex_destroy(&hpriv->restore_phase_mutex);
- kfree(hpriv);
-
- /* Now the FD is really closed */
- atomic_dec(&hdev->fd_open_cnt);
-
- /* This allows a new user context to open the device */
+ mutex_lock(&hdev->fpriv_list_lock);
+ list_del(&hpriv->dev_node);
hdev->compute_ctx = NULL;
+ mutex_unlock(&hdev->fpriv_list_lock);
+
+ kfree(hpriv);
}
void hl_hpriv_get(struct hl_fpriv *hpriv)
@@ -229,14 +230,14 @@ static int device_early_init(struct hl_device *hdev)
hl_cb_mgr_init(&hdev->kernel_cb_mgr);
- mutex_init(&hdev->fd_open_cnt_lock);
mutex_init(&hdev->send_cpu_message_lock);
mutex_init(&hdev->debug_lock);
mutex_init(&hdev->mmu_cache_lock);
INIT_LIST_HEAD(&hdev->hw_queues_mirror_list);
spin_lock_init(&hdev->hw_queues_mirror_lock);
+ INIT_LIST_HEAD(&hdev->fpriv_list);
+ mutex_init(&hdev->fpriv_list_lock);
atomic_set(&hdev->in_reset, 0);
- atomic_set(&hdev->fd_open_cnt, 0);
atomic_set(&hdev->cs_active_cnt, 0);
return 0;
@@ -266,6 +267,8 @@ static void device_early_fini(struct hl_device *hdev)
mutex_destroy(&hdev->debug_lock);
mutex_destroy(&hdev->send_cpu_message_lock);
+ mutex_destroy(&hdev->fpriv_list_lock);
+
hl_cb_mgr_fini(hdev, &hdev->kernel_cb_mgr);
kfree(hdev->hl_chip_info);
@@ -277,8 +280,6 @@ static void device_early_fini(struct hl_device *hdev)
if (hdev->asic_funcs->early_fini)
hdev->asic_funcs->early_fini(hdev);
-
- mutex_destroy(&hdev->fd_open_cnt_lock);
}
static void set_freq_to_low_job(struct work_struct *work)
@@ -286,9 +287,13 @@ static void set_freq_to_low_job(struct work_struct *work)
struct hl_device *hdev = container_of(work, struct hl_device,
work_freq.work);
- if (atomic_read(&hdev->fd_open_cnt) == 0)
+ mutex_lock(&hdev->fpriv_list_lock);
+
+ if (!hdev->compute_ctx)
hl_device_set_frequency(hdev, PLL_LOW);
+ mutex_unlock(&hdev->fpriv_list_lock);
+
schedule_delayed_work(&hdev->work_freq,
usecs_to_jiffies(HL_PLL_LOW_JOB_FREQ_USEC));
}
@@ -338,7 +343,7 @@ static int device_late_init(struct hl_device *hdev)
hdev->high_pll = hdev->asic_prop.high_pll;
/* force setting to low frequency */
- atomic_set(&hdev->curr_pll_profile, PLL_LOW);
+ hdev->curr_pll_profile = PLL_LOW;
if (hdev->pm_mng_profile == PM_AUTO)
hdev->asic_funcs->set_pll_profile(hdev, PLL_LOW);
@@ -387,38 +392,26 @@ static void device_late_fini(struct hl_device *hdev)
* @hdev: pointer to habanalabs device structure
* @freq: the new frequency value
*
- * Change the frequency if needed.
- * We allose to set PLL to low only if there is no user process
- * Returns 0 if no change was done, otherwise returns 1;
+ * Change the frequency if needed. This function has no protection against
+ * concurrency, therefore it is assumed that the calling function has protected
+ * itself against the case of calling this function from multiple threads with
+ * different values
+ *
+ * Returns 0 if no change was done, otherwise returns 1
*/
int hl_device_set_frequency(struct hl_device *hdev, enum hl_pll_frequency freq)
{
- enum hl_pll_frequency old_freq =
- (freq == PLL_HIGH) ? PLL_LOW : PLL_HIGH;
- int ret;
-
- if (hdev->pm_mng_profile == PM_MANUAL)
+ if ((hdev->pm_mng_profile == PM_MANUAL) ||
+ (hdev->curr_pll_profile == freq))
return 0;
- ret = atomic_cmpxchg(&hdev->curr_pll_profile, old_freq, freq);
- if (ret == freq)
- return 0;
-
- /*
- * in case we want to lower frequency, check if device is not
- * opened. We must have a check here to workaround race condition with
- * hl_device_open
- */
- if ((freq == PLL_LOW) && (atomic_read(&hdev->fd_open_cnt) > 0)) {
- atomic_set(&hdev->curr_pll_profile, PLL_HIGH);
- return 0;
- }
-
dev_dbg(hdev->dev, "Changing device frequency to %s\n",
freq == PLL_HIGH ? "high" : "low");
hdev->asic_funcs->set_pll_profile(hdev, freq);
+ hdev->curr_pll_profile = freq;
+
return 1;
}
@@ -449,19 +442,8 @@ int hl_device_set_debug_mode(struct hl_device *hdev, bool enable)
goto out;
}
- mutex_lock(&hdev->fd_open_cnt_lock);
-
- if (atomic_read(&hdev->fd_open_cnt) > 1) {
- dev_err(hdev->dev,
- "Failed to enable debug mode. More then a single user is using the device\n");
- rc = -EPERM;
- goto unlock_fd_open_lock;
- }
-
hdev->in_debug = 1;
-unlock_fd_open_lock:
- mutex_unlock(&hdev->fd_open_cnt_lock);
out:
mutex_unlock(&hdev->debug_lock);
@@ -568,6 +550,7 @@ int hl_device_resume(struct hl_device *hdev)
static void device_kill_open_processes(struct hl_device *hdev)
{
u16 pending_total, pending_cnt;
+ struct hl_fpriv *hpriv;
struct task_struct *task = NULL;
if (hdev->pldm)
@@ -575,32 +558,30 @@ static void device_kill_open_processes(struct hl_device *hdev)
else
pending_total = HL_PENDING_RESET_PER_SEC;
- pending_cnt = pending_total;
-
- /* Flush all processes that are inside hl_open */
- mutex_lock(&hdev->fd_open_cnt_lock);
-
- while ((atomic_read(&hdev->fd_open_cnt)) && (pending_cnt)) {
-
- pending_cnt--;
-
- dev_info(hdev->dev,
- "Can't HARD reset, waiting for user to close FD\n");
+ /* Giving time for user to close FD, and for processes that are inside
+ * hl_device_open to finish
+ */
+ if (!list_empty(&hdev->fpriv_list))
ssleep(1);
- }
- if (atomic_read(&hdev->fd_open_cnt)) {
- task = get_pid_task(hdev->compute_ctx->hpriv->taskpid,
- PIDTYPE_PID);
+ mutex_lock(&hdev->fpriv_list_lock);
+
+ /* This section must be protected because we are dereferencing
+ * pointers that are freed if the process exits
+ */
+ list_for_each_entry(hpriv, &hdev->fpriv_list, dev_node) {
+ task = get_pid_task(hpriv->taskpid, PIDTYPE_PID);
if (task) {
- dev_info(hdev->dev, "Killing user processes\n");
+ dev_info(hdev->dev, "Killing user process\n");
send_sig(SIGKILL, task, 1);
- msleep(100);
+ usleep_range(1000, 10000);
put_task_struct(task);
}
}
+ mutex_unlock(&hdev->fpriv_list_lock);
+
/* We killed the open users, but because the driver cleans up after the
* user contexts are closed (e.g. mmu mappings), we need to wait again
* to make sure the cleaning phase is finished before continuing with
@@ -609,19 +590,18 @@ static void device_kill_open_processes(struct hl_device *hdev)
pending_cnt = pending_total;
- while ((atomic_read(&hdev->fd_open_cnt)) && (pending_cnt)) {
+ while ((!list_empty(&hdev->fpriv_list)) && (pending_cnt)) {
+ dev_info(hdev->dev,
+ "Waiting for all unmap operations to finish before hard reset\n");
pending_cnt--;
ssleep(1);
}
- if (atomic_read(&hdev->fd_open_cnt))
+ if (!list_empty(&hdev->fpriv_list))
dev_crit(hdev->dev,
"Going to hard reset with open user contexts\n");
-
- mutex_unlock(&hdev->fd_open_cnt_lock);
-
}
static void device_hard_reset_pending(struct work_struct *work)
@@ -677,13 +657,16 @@ int hl_device_reset(struct hl_device *hdev, bool hard_reset,
/* This also blocks future CS/VM/JOB completion operations */
hdev->disabled = true;
- /*
- * Flush anyone that is inside the critical section of enqueue
+ /* Flush anyone that is inside the critical section of enqueue
* jobs to the H/W
*/
hdev->asic_funcs->hw_queues_lock(hdev);
hdev->asic_funcs->hw_queues_unlock(hdev);
+ /* Flush anyone that is inside device open */
+ mutex_lock(&hdev->fpriv_list_lock);
+ mutex_unlock(&hdev->fpriv_list_lock);
+
dev_err(hdev->dev, "Going to RESET device!\n");
}
@@ -759,12 +742,16 @@ int hl_device_reset(struct hl_device *hdev, bool hard_reset,
for (i = 0 ; i < hdev->asic_prop.completion_queues_count ; i++)
hl_cq_reset(hdev, &hdev->completion_queue[i]);
+ mutex_lock(&hdev->fpriv_list_lock);
+
/* Make sure the context switch phase will run again */
if (hdev->compute_ctx) {
atomic_set(&hdev->compute_ctx->thread_ctx_switch_token, 1);
hdev->compute_ctx->thread_ctx_switch_wait_token = 0;
}
+ mutex_unlock(&hdev->fpriv_list_lock);
+
/* Finished tear-down, starting to re-initialize */
if (hard_reset) {
@@ -1126,13 +1113,16 @@ void hl_device_fini(struct hl_device *hdev)
/* Mark device as disabled */
hdev->disabled = true;
- /*
- * Flush anyone that is inside the critical section of enqueue
+ /* Flush anyone that is inside the critical section of enqueue
* jobs to the H/W
*/
hdev->asic_funcs->hw_queues_lock(hdev);
hdev->asic_funcs->hw_queues_unlock(hdev);
+ /* Flush anyone that is inside device open */
+ mutex_lock(&hdev->fpriv_list_lock);
+ mutex_unlock(&hdev->fpriv_list_lock);
+
hdev->hard_reset_pending = true;
hl_hwmon_fini(hdev);
diff --git a/drivers/misc/habanalabs/goya/goya_hwmgr.c b/drivers/misc/habanalabs/goya/goya_hwmgr.c
index a51d836542a1..3f123165cc25 100644
--- a/drivers/misc/habanalabs/goya/goya_hwmgr.c
+++ b/drivers/misc/habanalabs/goya/goya_hwmgr.c
@@ -254,11 +254,11 @@ static ssize_t pm_mng_profile_store(struct device *dev,
goto out;
}
- mutex_lock(&hdev->fd_open_cnt_lock);
+ mutex_lock(&hdev->fpriv_list_lock);
- if (atomic_read(&hdev->fd_open_cnt) > 0) {
+ if (hdev->compute_ctx) {
dev_err(hdev->dev,
- "Can't change PM profile while user process is opened on the device\n");
+ "Can't change PM profile while compute context is opened on the device\n");
count = -EPERM;
goto unlock_mutex;
}
@@ -266,7 +266,7 @@ static ssize_t pm_mng_profile_store(struct device *dev,
if (strncmp("auto", buf, strlen("auto")) == 0) {
/* Make sure we are in LOW PLL when changing modes */
if (hdev->pm_mng_profile == PM_MANUAL) {
- atomic_set(&hdev->curr_pll_profile, PLL_HIGH);
+ hdev->curr_pll_profile = PLL_HIGH;
hl_device_set_frequency(hdev, PLL_LOW);
hdev->pm_mng_profile = PM_AUTO;
}
@@ -279,11 +279,10 @@ static ssize_t pm_mng_profile_store(struct device *dev,
} else {
dev_err(hdev->dev, "value should be auto or manual\n");
count = -EINVAL;
- goto unlock_mutex;
}
unlock_mutex:
- mutex_unlock(&hdev->fd_open_cnt_lock);
+ mutex_unlock(&hdev->fpriv_list_lock);
out:
return count;
}
diff --git a/drivers/misc/habanalabs/habanalabs.h b/drivers/misc/habanalabs/habanalabs.h
index 53f6b238e6f2..f2ca06b2c155 100644
--- a/drivers/misc/habanalabs/habanalabs.h
+++ b/drivers/misc/habanalabs/habanalabs.h
@@ -909,6 +909,7 @@ struct hl_debug_params {
* @ctx_mgr: context manager to handle multiple context for this FD.
* @cb_mgr: command buffer manager to handle multiple buffers for this FD.
* @debugfs_list: list of relevant ASIC debugfs.
+ * @dev_node: node in the device list of file private data
* @refcount: number of related contexts.
* @restore_phase_mutex: lock for context switch and restore phase.
*/
@@ -920,6 +921,7 @@ struct hl_fpriv {
struct hl_ctx_mgr ctx_mgr;
struct hl_cb_mgr cb_mgr;
struct list_head debugfs_list;
+ struct list_head dev_node;
struct kref refcount;
struct mutex restore_phase_mutex;
};
@@ -1161,12 +1163,6 @@ struct hl_device_reset_work {
* @cpu_accessible_dma_pool: KMD <-> ArmCP shared memory pool.
* @asid_bitmap: holds used/available ASIDs.
* @asid_mutex: protects asid_bitmap.
- * @fd_open_cnt_lock: lock for updating fd_open_cnt in hl_device_open. Although
- * fd_open_cnt is atomic, we need this lock to serialize
- * the open function because the driver currently supports
- * only a single process at a time. In addition, we need a
- * lock here so we can flush user processes which are opening
- * the device while we are trying to hard reset it
* @send_cpu_message_lock: enforces only one message in KMD <-> ArmCP queue.
* @debug_lock: protects critical section of setting debug mode for device
* @asic_prop: ASIC specific immutable properties.
@@ -1182,6 +1178,9 @@ struct hl_device_reset_work {
* @hl_debugfs: device's debugfs manager.
* @cb_pool: list of preallocated CBs.
* @cb_pool_lock: protects the CB pool.
+ * @fpriv_list: list of file private data structures. Each structure is created
+ * when a user opens the device
+ * @fpriv_list_lock: protects the fpriv_list
* @compute_ctx: current compute context executing.
* @dram_used_mem: current DRAM memory consumption.
* @timeout_jiffies: device CS timeout value.
@@ -1189,10 +1188,9 @@ struct hl_device_reset_work {
* value is saved so in case of hard-reset, KMD will restore this
* value and update the F/W after the re-initialization
* @in_reset: is device in reset flow.
- * @curr_pll_profile: current PLL profile.
- * @fd_open_cnt: number of open user processes.
* @cs_active_cnt: number of active command submissions on this device (active
* means already in H/W queues)
+ * @curr_pll_profile: current PLL profile.
* @major: habanalabs KMD major.
* @high_pll: high PLL profile frequency.
* @soft_reset_cnt: number of soft reset since KMD loading.
@@ -1211,7 +1209,7 @@ struct hl_device_reset_work {
* @mmu_enable: is MMU enabled.
* @device_cpu_disabled: is the device CPU disabled (due to timeouts)
* @dma_mask: the dma mask that was set for this device
- * @in_debug: is device under debug. This, together with fd_open_cnt, enforces
+ * @in_debug: is device under debug. This, together with fpriv_list, enforces
* that only a single user is configuring the debug infrastructure.
*/
struct hl_device {
@@ -1239,8 +1237,6 @@ struct hl_device {
struct gen_pool *cpu_accessible_dma_pool;
unsigned long *asid_bitmap;
struct mutex asid_mutex;
- /* TODO: remove fd_open_cnt_lock for multiple process support */
- struct mutex fd_open_cnt_lock;
struct mutex send_cpu_message_lock;
struct mutex debug_lock;
struct asic_fixed_properties asic_prop;
@@ -1259,15 +1255,17 @@ struct hl_device {
struct list_head cb_pool;
spinlock_t cb_pool_lock;
+ struct list_head fpriv_list;
+ struct mutex fpriv_list_lock;
+
struct hl_ctx *compute_ctx;
atomic64_t dram_used_mem;
u64 timeout_jiffies;
u64 max_power;
atomic_t in_reset;
- atomic_t curr_pll_profile;
- atomic_t fd_open_cnt;
atomic_t cs_active_cnt;
+ enum hl_pll_frequency curr_pll_profile;
u32 major;
u32 high_pll;
u32 soft_reset_cnt;
diff --git a/drivers/misc/habanalabs/habanalabs_drv.c b/drivers/misc/habanalabs/habanalabs_drv.c
index 678f61646ca9..802c6ca7c604 100644
--- a/drivers/misc/habanalabs/habanalabs_drv.c
+++ b/drivers/misc/habanalabs/habanalabs_drv.c
@@ -95,80 +95,78 @@ int hl_device_open(struct inode *inode, struct file *filp)
return -ENXIO;
}
- mutex_lock(&hdev->fd_open_cnt_lock);
+ hpriv = kzalloc(sizeof(*hpriv), GFP_KERNEL);
+ if (!hpriv)
+ return -ENOMEM;
+
+ hpriv->hdev = hdev;
+ filp->private_data = hpriv;
+ hpriv->filp = filp;
+ mutex_init(&hpriv->restore_phase_mutex);
+ kref_init(&hpriv->refcount);
+ nonseekable_open(inode, filp);
+
+ hl_cb_mgr_init(&hpriv->cb_mgr);
+ hl_ctx_mgr_init(&hpriv->ctx_mgr);
+
+ hpriv->taskpid = find_get_pid(current->pid);
+
+ mutex_lock(&hdev->fpriv_list_lock);
if (hl_device_disabled_or_in_reset(hdev)) {
dev_err_ratelimited(hdev->dev,
"Can't open %s because it is disabled or in reset\n",
dev_name(hdev->dev));
- mutex_unlock(&hdev->fd_open_cnt_lock);
- return -EPERM;
+ rc = -EPERM;
+ goto out_err;
}
if (hdev->in_debug) {
dev_err_ratelimited(hdev->dev,
"Can't open %s because it is being debugged by another user\n",
dev_name(hdev->dev));
- mutex_unlock(&hdev->fd_open_cnt_lock);
- return -EPERM;
+ rc = -EPERM;
+ goto out_err;
}
- if (atomic_read(&hdev->fd_open_cnt)) {
+ if (hdev->compute_ctx) {
dev_info_ratelimited(hdev->dev,
"Can't open %s because another user is working on it\n",
dev_name(hdev->dev));
- mutex_unlock(&hdev->fd_open_cnt_lock);
- return -EBUSY;
- }
-
- atomic_inc(&hdev->fd_open_cnt);
-
- mutex_unlock(&hdev->fd_open_cnt_lock);
-
- hpriv = kzalloc(sizeof(*hpriv), GFP_KERNEL);
- if (!hpriv) {
- rc = -ENOMEM;
- goto close_device;
+ rc = -EBUSY;
+ goto out_err;
}
- hpriv->hdev = hdev;
- filp->private_data = hpriv;
- hpriv->filp = filp;
- mutex_init(&hpriv->restore_phase_mutex);
- kref_init(&hpriv->refcount);
- nonseekable_open(inode, filp);
-
- hl_cb_mgr_init(&hpriv->cb_mgr);
- hl_ctx_mgr_init(&hpriv->ctx_mgr);
-
rc = hl_ctx_create(hdev, hpriv);
if (rc) {
- dev_err(hdev->dev, "Failed to open FD (CTX fail)\n");
+ dev_err(hdev->dev, "Failed to create context %d\n", rc);
goto out_err;
}
- hpriv->taskpid = find_get_pid(current->pid);
-
- /*
- * Device is IDLE at this point so it is legal to change PLLs. There
- * is no need to check anything because if the PLL is already HIGH, the
- * set function will return without doing anything
+ /* Device is IDLE at this point so it is legal to change PLLs.
+ * There is no need to check anything because if the PLL is
+ * already HIGH, the set function will return without doing
+ * anything
*/
hl_device_set_frequency(hdev, PLL_HIGH);
+ list_add(&hpriv->dev_node, &hdev->fpriv_list);
+ mutex_unlock(&hdev->fpriv_list_lock);
+
hl_debugfs_add_file(hpriv);
return 0;
out_err:
- filp->private_data = NULL;
- hl_ctx_mgr_fini(hpriv->hdev, &hpriv->ctx_mgr);
+ mutex_unlock(&hdev->fpriv_list_lock);
+
hl_cb_mgr_fini(hpriv->hdev, &hpriv->cb_mgr);
+ hl_ctx_mgr_fini(hpriv->hdev, &hpriv->ctx_mgr);
+ filp->private_data = NULL;
mutex_destroy(&hpriv->restore_phase_mutex);
- kfree(hpriv);
+ put_pid(hpriv->taskpid);
-close_device:
- atomic_dec(&hdev->fd_open_cnt);
+ kfree(hpriv);
return rc;
}
--
2.17.1
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH v2 6/7] habanalabs: change device_setup_cdev() to be more generic
2019-07-30 9:47 [PATCH v2 0/7] habanalabs: support info queries by multiple processes Oded Gabbay
` (4 preceding siblings ...)
2019-07-30 9:47 ` [PATCH v2 5/7] habanalabs: maintain a list of file private data objects Oded Gabbay
@ 2019-07-30 9:47 ` Oded Gabbay
2019-07-30 9:47 ` [PATCH v2 7/7] habanalabs: create two char devices per ASIC Oded Gabbay
6 siblings, 0 replies; 10+ messages in thread
From: Oded Gabbay @ 2019-07-30 9:47 UTC (permalink / raw)
To: linux-kernel, oshpigelman, ttayar, gregkh
This patch re-factors the device_setup_cdev() function to make it more
generic. It doesn't manipulate members of the driver's internal device
structure but instead works only on the arguments that are sent to it.
This is in preparation for using this function to create an additional
char device per ASIC.
Signed-off-by: Oded Gabbay <oded.gabbay@gmail.com>
---
drivers/misc/habanalabs/device.c | 47 +++++++++++++++++---------------
1 file changed, 25 insertions(+), 22 deletions(-)
diff --git a/drivers/misc/habanalabs/device.c b/drivers/misc/habanalabs/device.c
index 926c85ff068f..60c779648f92 100644
--- a/drivers/misc/habanalabs/device.c
+++ b/drivers/misc/habanalabs/device.c
@@ -131,48 +131,43 @@ static const struct file_operations hl_ops = {
* @hdev: pointer to habanalabs device structure
* @hclass: pointer to the class object of the device
* @minor: minor number of the specific device
- * @fpos : file operations to install for this device
+ * @fpos: file operations to install for this device
+ * @name: name of the device as it will appear in the filesystem
+ * @cdev: pointer to the char device object that will be created
+ * @dev: pointer to the device object that will be created
*
* Create a cdev and a Linux device for habanalabs's device. Need to be
* called at the end of the habanalabs device initialization process,
* because this function exposes the device to the user
*/
static int device_setup_cdev(struct hl_device *hdev, struct class *hclass,
- int minor, const struct file_operations *fops)
+ int minor, const struct file_operations *fops,
+ char *name, struct cdev *cdev,
+ struct device **dev)
{
int err, devno = MKDEV(hdev->major, minor);
- struct cdev *hdev_cdev = &hdev->cdev;
- char *name;
-
- name = kasprintf(GFP_KERNEL, "hl%d", hdev->id);
- if (!name)
- return -ENOMEM;
- cdev_init(hdev_cdev, fops);
- hdev_cdev->owner = THIS_MODULE;
- err = cdev_add(hdev_cdev, devno, 1);
+ cdev_init(cdev, fops);
+ cdev->owner = THIS_MODULE;
+ err = cdev_add(cdev, devno, 1);
if (err) {
pr_err("Failed to add char device %s\n", name);
- goto err_cdev_add;
+ return err;
}
- hdev->dev = device_create(hclass, NULL, devno, NULL, "%s", name);
- if (IS_ERR(hdev->dev)) {
+ *dev = device_create(hclass, NULL, devno, NULL, "%s", name);
+ if (IS_ERR(*dev)) {
pr_err("Failed to create device %s\n", name);
- err = PTR_ERR(hdev->dev);
+ err = PTR_ERR(*dev);
goto err_device_create;
}
- dev_set_drvdata(hdev->dev, hdev);
-
- kfree(name);
+ dev_set_drvdata(*dev, hdev);
return 0;
err_device_create:
- cdev_del(hdev_cdev);
-err_cdev_add:
- kfree(name);
+ cdev_del(cdev);
return err;
}
@@ -875,9 +870,17 @@ int hl_device_reset(struct hl_device *hdev, bool hard_reset,
int hl_device_init(struct hl_device *hdev, struct class *hclass)
{
int i, rc, cq_ready_cnt;
+ char *name;
+
+ name = kasprintf(GFP_KERNEL, "hl%d", hdev->id);
+ if (!name)
+ return -ENOMEM;
/* Create device */
- rc = device_setup_cdev(hdev, hclass, hdev->id, &hl_ops);
+ rc = device_setup_cdev(hdev, hclass, hdev->id, &hl_ops, name,
+ &hdev->cdev, &hdev->dev);
+
+ kfree(name);
if (rc)
goto out_disabled;
--
2.17.1
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH v2 7/7] habanalabs: create two char devices per ASIC
2019-07-30 9:47 [PATCH v2 0/7] habanalabs: support info queries by multiple processes Oded Gabbay
` (5 preceding siblings ...)
2019-07-30 9:47 ` [PATCH v2 6/7] habanalabs: change device_setup_cdev() to be more generic Oded Gabbay
@ 2019-07-30 9:47 ` Oded Gabbay
2019-07-30 10:42 ` Greg KH
6 siblings, 1 reply; 10+ messages in thread
From: Oded Gabbay @ 2019-07-30 9:47 UTC (permalink / raw)
To: linux-kernel, oshpigelman, ttayar, gregkh
This patch changes the driver to create two char devices for each ASIC
it discovers. This is done to allow system/monitoring applications to
query the device for stats, information, idle state and more, while also
allowing the deep-learning application to send work to the ASIC.
One char device is the original device, hlX. IOCTL calls through this
device file can perform any task on the device (compute, memory, queries).
The open function for this device will fail if it was called before but
the file-descriptor it created was not completely released yet (the
release callback function is not called from the kernel until all
instances of that FD are closed). The driver needs to keep this behavior
to support backward compatibility with existing userspace, which count
that the open will fail if the device is "occupied".
The second char device is called "hl_controlDx", where x is the minor
number of the original char device + 64 (HL_CONTROL_MINOR). Applications
that open this device can only call the INFO IOCTL. There is no limitation
on the number of applications opening this device.
Signed-off-by: Oded Gabbay <oded.gabbay@gmail.com>
---
drivers/misc/habanalabs/device.c | 50 +++++++++++-
drivers/misc/habanalabs/habanalabs.h | 18 +++-
drivers/misc/habanalabs/habanalabs_drv.c | 49 +++++++++++
drivers/misc/habanalabs/habanalabs_ioctl.c | 95 +++++++++++++++-------
4 files changed, 180 insertions(+), 32 deletions(-)
diff --git a/drivers/misc/habanalabs/device.c b/drivers/misc/habanalabs/device.c
index 60c779648f92..6aef23a8d2ee 100644
--- a/drivers/misc/habanalabs/device.c
+++ b/drivers/misc/habanalabs/device.c
@@ -95,6 +95,24 @@ static int hl_device_release(struct inode *inode, struct file *filp)
return 0;
}
+static int hl_device_release_ctrl(struct inode *inode, struct file *filp)
+{
+ struct hl_fpriv *hpriv = filp->private_data;
+ struct hl_device *hdev;
+
+ filp->private_data = NULL;
+
+ hdev = hpriv->hdev;
+
+ mutex_lock(&hdev->fpriv_list_lock);
+ list_del(&hpriv->dev_node);
+ mutex_unlock(&hdev->fpriv_list_lock);
+
+ kfree(hpriv);
+
+ return 0;
+}
+
/*
* hl_mmap - mmap function for habanalabs device
*
@@ -125,6 +143,14 @@ static const struct file_operations hl_ops = {
.compat_ioctl = hl_ioctl
};
+static const struct file_operations hl_ctrl_ops = {
+ .owner = THIS_MODULE,
+ .open = hl_device_open_ctrl,
+ .release = hl_device_release_ctrl,
+ .unlocked_ioctl = hl_ioctl_control,
+ .compat_ioctl = hl_ioctl_control
+};
+
/*
* device_setup_cdev - setup cdev and device for habanalabs device
*
@@ -885,10 +911,27 @@ int hl_device_init(struct hl_device *hdev, struct class *hclass)
if (rc)
goto out_disabled;
+ hdev->id_control = hdev->id + HL_CONTROL_MINOR;
+
+ name = kasprintf(GFP_KERNEL, "hl_controlD%d", hdev->id_control);
+ if (!name) {
+ rc = -ENOMEM;
+ goto release_device;
+ }
+
+ /* Create control device */
+ rc = device_setup_cdev(hdev, hclass, hdev->id_control, &hl_ctrl_ops,
+ name, &hdev->cdev_ctrl, &hdev->dev_ctrl);
+
+ kfree(name);
+
+ if (rc)
+ goto release_device;
+
/* Initialize ASIC function pointers and perform early init */
rc = device_early_init(hdev);
if (rc)
- goto release_device;
+ goto release_control_device;
/*
* Start calling ASIC initialization. First S/W then H/W and finally
@@ -1064,6 +1107,9 @@ int hl_device_init(struct hl_device *hdev, struct class *hclass)
hdev->asic_funcs->sw_fini(hdev);
early_fini:
device_early_fini(hdev);
+release_control_device:
+ device_destroy(hclass, hdev->dev_ctrl->devt);
+ cdev_del(&hdev->cdev_ctrl);
release_device:
device_destroy(hclass, hdev->dev->devt);
cdev_del(&hdev->cdev);
@@ -1179,6 +1225,8 @@ void hl_device_fini(struct hl_device *hdev)
device_early_fini(hdev);
/* Hide device from user */
+ device_destroy(hdev->dev_ctrl->class, hdev->dev_ctrl->devt);
+ cdev_del(&hdev->cdev_ctrl);
device_destroy(hdev->dev->class, hdev->dev->devt);
cdev_del(&hdev->cdev);
diff --git a/drivers/misc/habanalabs/habanalabs.h b/drivers/misc/habanalabs/habanalabs.h
index f2ca06b2c155..c0dc4c8e1b7d 100644
--- a/drivers/misc/habanalabs/habanalabs.h
+++ b/drivers/misc/habanalabs/habanalabs.h
@@ -51,6 +51,8 @@
/* MMU */
#define MMU_HASH_TABLE_BITS 7 /* 1 << 7 buckets */
+#define HL_CONTROL_MINOR 64
+
/**
* struct pgt_info - MMU hop page info.
* @node: hash linked-list node for the pgts shadow hash of pgts.
@@ -912,6 +914,7 @@ struct hl_debug_params {
* @dev_node: node in the device list of file private data
* @refcount: number of related contexts.
* @restore_phase_mutex: lock for context switch and restore phase.
+ * @is_control: true for control device, false otherwise
*/
struct hl_fpriv {
struct hl_device *hdev;
@@ -924,6 +927,7 @@ struct hl_fpriv {
struct list_head dev_node;
struct kref refcount;
struct mutex restore_phase_mutex;
+ u8 is_control;
};
@@ -1010,9 +1014,9 @@ struct hl_dbg_device_entry {
*/
/* Theoretical limit only. A single host can only contain up to 4 or 8 PCIe
- * x16 cards. In extereme cases, there are hosts that can accommodate 16 cards
+ * x16 cards. In extreme cases, there are hosts that can accommodate 16 cards.
*/
-#define HL_MAX_MINORS 256
+#define HL_MAX_MINORS HL_CONTROL_MINOR
/*
* Registers read & write functions.
@@ -1143,7 +1147,9 @@ struct hl_device_reset_work {
* @pcie_bar: array of available PCIe bars.
* @rmmio: configuration area address on SRAM.
* @cdev: related char device.
- * @dev: realted kernel basic device structure.
+ * @cdev_ctrl: char device for control operations only (INFO IOCTL)
+ * @dev: related kernel basic device structure.
+ * @dev_ctrl: related kernel device structure for the control device
* @work_freq: delayed work to lower device frequency if possible.
* @work_heartbeat: delayed work for ArmCP is-alive check.
* @asic_name: ASIC specific nmae.
@@ -1196,6 +1202,7 @@ struct hl_device_reset_work {
* @soft_reset_cnt: number of soft reset since KMD loading.
* @hard_reset_cnt: number of hard reset since KMD loading.
* @id: device minor.
+ * @id_control: minor of the control device
* @disabled: is device disabled.
* @late_init_done: is late init stage was done during initialization.
* @hwmon_initialized: is H/W monitor sensors was initialized.
@@ -1217,7 +1224,9 @@ struct hl_device {
void __iomem *pcie_bar[6];
void __iomem *rmmio;
struct cdev cdev;
+ struct cdev cdev_ctrl;
struct device *dev;
+ struct device *dev_ctrl;
struct delayed_work work_freq;
struct delayed_work work_heartbeat;
char asic_name[16];
@@ -1271,6 +1280,7 @@ struct hl_device {
u32 soft_reset_cnt;
u32 hard_reset_cnt;
u16 id;
+ u16 id_control;
u8 disabled;
u8 late_init_done;
u8 hwmon_initialized;
@@ -1376,6 +1386,7 @@ static inline bool hl_mem_area_crosses_range(u64 address, u32 size,
}
int hl_device_open(struct inode *inode, struct file *filp);
+int hl_device_open_ctrl(struct inode *inode, struct file *filp);
bool hl_device_disabled_or_in_reset(struct hl_device *hdev);
enum hl_device_status hl_device_status(struct hl_device *hdev);
int hl_device_set_debug_mode(struct hl_device *hdev, bool enable);
@@ -1615,6 +1626,7 @@ static inline void hl_debugfs_remove_ctx_mem_hash(struct hl_device *hdev,
/* IOCTLs */
long hl_ioctl(struct file *filep, unsigned int cmd, unsigned long arg);
+long hl_ioctl_control(struct file *filep, unsigned int cmd, unsigned long arg);
int hl_cb_ioctl(struct hl_fpriv *hpriv, void *data);
int hl_cs_ioctl(struct hl_fpriv *hpriv, void *data);
int hl_cs_wait_ioctl(struct hl_fpriv *hpriv, void *data);
diff --git a/drivers/misc/habanalabs/habanalabs_drv.c b/drivers/misc/habanalabs/habanalabs_drv.c
index 802c6ca7c604..da3360ef6204 100644
--- a/drivers/misc/habanalabs/habanalabs_drv.c
+++ b/drivers/misc/habanalabs/habanalabs_drv.c
@@ -170,6 +170,55 @@ int hl_device_open(struct inode *inode, struct file *filp)
return rc;
}
+int hl_device_open_ctrl(struct inode *inode, struct file *filp)
+{
+ struct hl_device *hdev;
+ struct hl_fpriv *hpriv;
+ int rc;
+
+ mutex_lock(&hl_devs_idr_lock);
+ hdev = idr_find(&hl_devs_idr, iminor(inode) - HL_CONTROL_MINOR);
+ mutex_unlock(&hl_devs_idr_lock);
+
+ if (!hdev) {
+ pr_err("Couldn't find device %d:%d\n",
+ imajor(inode), iminor(inode));
+ return -ENXIO;
+ }
+
+ hpriv = kzalloc(sizeof(*hpriv), GFP_KERNEL);
+ if (!hpriv)
+ return -ENOMEM;
+
+ mutex_lock(&hdev->fpriv_list_lock);
+
+ if (hl_device_disabled_or_in_reset(hdev)) {
+ dev_err_ratelimited(hdev->dev_ctrl,
+ "Can't open %s because it is disabled or in reset\n",
+ dev_name(hdev->dev_ctrl));
+ rc = -EPERM;
+ goto out_err;
+ }
+
+ list_add(&hpriv->dev_node, &hdev->fpriv_list);
+ mutex_unlock(&hdev->fpriv_list_lock);
+
+ hpriv->hdev = hdev;
+ filp->private_data = hpriv;
+ hpriv->filp = filp;
+ hpriv->is_control = true;
+ nonseekable_open(inode, filp);
+
+ hpriv->taskpid = find_get_pid(current->pid);
+
+ return 0;
+
+out_err:
+ mutex_unlock(&hdev->fpriv_list_lock);
+ kfree(hpriv);
+ return rc;
+}
+
static void set_driver_behavior_per_device(struct hl_device *hdev)
{
hdev->mmu_enable = 1;
diff --git a/drivers/misc/habanalabs/habanalabs_ioctl.c b/drivers/misc/habanalabs/habanalabs_ioctl.c
index c9a4799eb251..ce0cd93a8421 100644
--- a/drivers/misc/habanalabs/habanalabs_ioctl.c
+++ b/drivers/misc/habanalabs/habanalabs_ioctl.c
@@ -194,7 +194,8 @@ static int debug_coresight(struct hl_device *hdev, struct hl_debug_args *args)
return rc;
}
-static int hl_info_ioctl(struct hl_fpriv *hpriv, void *data)
+static int _hl_info_ioctl(struct hl_fpriv *hpriv, void *data,
+ struct device *dev)
{
struct hl_info_args *args = data;
struct hl_device *hdev = hpriv->hdev;
@@ -205,7 +206,7 @@ static int hl_info_ioctl(struct hl_fpriv *hpriv, void *data)
return device_status_info(hdev, args);
if (hl_device_disabled_or_in_reset(hdev)) {
- dev_warn_ratelimited(hdev->dev,
+ dev_warn_ratelimited(dev,
"Device is %s. Can't execute INFO IOCTL\n",
atomic_read(&hdev->in_reset) ? "in_reset" : "disabled");
return -EBUSY;
@@ -229,7 +230,7 @@ static int hl_info_ioctl(struct hl_fpriv *hpriv, void *data)
break;
default:
- dev_err(hdev->dev, "Invalid request %d\n", args->op);
+ dev_err(dev, "Invalid request %d\n", args->op);
rc = -ENOTTY;
break;
}
@@ -237,6 +238,16 @@ static int hl_info_ioctl(struct hl_fpriv *hpriv, void *data)
return rc;
}
+static int hl_info_ioctl(struct hl_fpriv *hpriv, void *data)
+{
+ return _hl_info_ioctl(hpriv, data, hpriv->hdev->dev);
+}
+
+static int hl_info_ioctl_control(struct hl_fpriv *hpriv, void *data)
+{
+ return _hl_info_ioctl(hpriv, data, hpriv->hdev->dev_ctrl);
+}
+
static int hl_debug_ioctl(struct hl_fpriv *hpriv, void *data)
{
struct hl_debug_args *args = data;
@@ -291,52 +302,45 @@ static const struct hl_ioctl_desc hl_ioctls[] = {
HL_IOCTL_DEF(HL_IOCTL_DEBUG, hl_debug_ioctl)
};
-#define HL_CORE_IOCTL_COUNT ARRAY_SIZE(hl_ioctls)
+static const struct hl_ioctl_desc hl_ioctls_control[] = {
+ HL_IOCTL_DEF(HL_IOCTL_INFO, hl_info_ioctl_control)
+};
-long hl_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
+long _hl_ioctl(struct file *filep, unsigned int cmd, unsigned long arg,
+ const struct hl_ioctl_desc *ioctl, struct device *dev)
{
struct hl_fpriv *hpriv = filep->private_data;
struct hl_device *hdev = hpriv->hdev;
- hl_ioctl_t *func;
- const struct hl_ioctl_desc *ioctl = NULL;
unsigned int nr = _IOC_NR(cmd);
char stack_kdata[128] = {0};
char *kdata = NULL;
unsigned int usize, asize;
+ hl_ioctl_t *func;
+ u32 hl_size;
int retcode;
if (hdev->hard_reset_pending) {
- dev_crit_ratelimited(hdev->dev,
+ dev_crit_ratelimited(hdev->dev_ctrl,
"Device HARD reset pending! Please close FD\n");
return -ENODEV;
}
- if ((nr >= HL_COMMAND_START) && (nr < HL_COMMAND_END)) {
- u32 hl_size;
-
- ioctl = &hl_ioctls[nr];
-
- hl_size = _IOC_SIZE(ioctl->cmd);
- usize = asize = _IOC_SIZE(cmd);
- if (hl_size > asize)
- asize = hl_size;
-
- cmd = ioctl->cmd;
- } else {
- dev_err(hdev->dev, "invalid ioctl: pid=%d, nr=0x%02x\n",
- task_pid_nr(current), nr);
- return -ENOTTY;
- }
-
/* Do not trust userspace, use our own definition */
func = ioctl->func;
if (unlikely(!func)) {
- dev_dbg(hdev->dev, "no function\n");
+ dev_dbg(dev, "no function\n");
retcode = -ENOTTY;
goto out_err;
}
+ hl_size = _IOC_SIZE(ioctl->cmd);
+ usize = asize = _IOC_SIZE(cmd);
+ if (hl_size > asize)
+ asize = hl_size;
+
+ cmd = ioctl->cmd;
+
if (cmd & (IOC_IN | IOC_OUT)) {
if (asize <= sizeof(stack_kdata)) {
kdata = stack_kdata;
@@ -366,8 +370,7 @@ long hl_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
out_err:
if (retcode)
- dev_dbg(hdev->dev,
- "error in ioctl: pid=%d, cmd=0x%02x, nr=0x%02x\n",
+ dev_dbg(dev, "error in ioctl: pid=%d, cmd=0x%02x, nr=0x%02x\n",
task_pid_nr(current), cmd, nr);
if (kdata != stack_kdata)
@@ -375,3 +378,39 @@ long hl_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
return retcode;
}
+
+long hl_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
+{
+ struct hl_fpriv *hpriv = filep->private_data;
+ struct hl_device *hdev = hpriv->hdev;
+ const struct hl_ioctl_desc *ioctl = NULL;
+ unsigned int nr = _IOC_NR(cmd);
+
+ if ((nr >= HL_COMMAND_START) && (nr < HL_COMMAND_END)) {
+ ioctl = &hl_ioctls[nr];
+ } else {
+ dev_err(hdev->dev, "invalid ioctl: pid=%d, nr=0x%02x\n",
+ task_pid_nr(current), nr);
+ return -ENOTTY;
+ }
+
+ return _hl_ioctl(filep, cmd, arg, ioctl, hdev->dev);
+}
+
+long hl_ioctl_control(struct file *filep, unsigned int cmd, unsigned long arg)
+{
+ struct hl_fpriv *hpriv = filep->private_data;
+ struct hl_device *hdev = hpriv->hdev;
+ const struct hl_ioctl_desc *ioctl = NULL;
+ unsigned int nr = _IOC_NR(cmd);
+
+ if (nr == _IOC_NR(HL_IOCTL_INFO)) {
+ ioctl = &hl_ioctls_control[nr];
+ } else {
+ dev_err(hdev->dev_ctrl, "invalid ioctl: pid=%d, nr=0x%02x\n",
+ task_pid_nr(current), nr);
+ return -ENOTTY;
+ }
+
+ return _hl_ioctl(filep, cmd, arg, ioctl, hdev->dev_ctrl);
+}
--
2.17.1
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH v2 7/7] habanalabs: create two char devices per ASIC
2019-07-30 9:47 ` [PATCH v2 7/7] habanalabs: create two char devices per ASIC Oded Gabbay
@ 2019-07-30 10:42 ` Greg KH
2019-07-30 11:23 ` Oded Gabbay
0 siblings, 1 reply; 10+ messages in thread
From: Greg KH @ 2019-07-30 10:42 UTC (permalink / raw)
To: Oded Gabbay; +Cc: linux-kernel, oshpigelman, ttayar
On Tue, Jul 30, 2019 at 12:47:24PM +0300, Oded Gabbay wrote:
> This patch changes the driver to create two char devices for each ASIC
> it discovers. This is done to allow system/monitoring applications to
> query the device for stats, information, idle state and more, while also
> allowing the deep-learning application to send work to the ASIC.
>
> One char device is the original device, hlX. IOCTL calls through this
> device file can perform any task on the device (compute, memory, queries).
> The open function for this device will fail if it was called before but
> the file-descriptor it created was not completely released yet (the
> release callback function is not called from the kernel until all
> instances of that FD are closed). The driver needs to keep this behavior
> to support backward compatibility with existing userspace, which count
> that the open will fail if the device is "occupied".
>
> The second char device is called "hl_controlDx", where x is the minor
> number of the original char device + 64 (HL_CONTROL_MINOR). Applications
> that open this device can only call the INFO IOCTL. There is no limitation
> on the number of applications opening this device.
>
> Signed-off-by: Oded Gabbay <oded.gabbay@gmail.com>
This reminds me of the old tty "control" devices we finally got rid of
many years ago. The more things change... :)
Anyway, looks sane to me. If you are ok with this userspace api, no
objection from me.
Only one comment below:
> --- a/drivers/misc/habanalabs/habanalabs.h
> +++ b/drivers/misc/habanalabs/habanalabs.h
> @@ -51,6 +51,8 @@
> /* MMU */
> #define MMU_HASH_TABLE_BITS 7 /* 1 << 7 buckets */
>
> +#define HL_CONTROL_MINOR 64
Don't try to segment your "dev" and "control" device nodes by minor
number range, as you will run into problems once you have a system with
more than 64 of these in the box at once.
Just use the whole range, and do:
dev = minor N
control = minor N+1
and so on. That makes your control device node an "odd" minor and your
"real" device an "even".
Given that all existing systems should be using devtmpfs, you should not
have any problem with changing the minor number of them at the next
reboot, right?
thanks,
greg k-h
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 7/7] habanalabs: create two char devices per ASIC
2019-07-30 10:42 ` Greg KH
@ 2019-07-30 11:23 ` Oded Gabbay
0 siblings, 0 replies; 10+ messages in thread
From: Oded Gabbay @ 2019-07-30 11:23 UTC (permalink / raw)
To: Greg KH; +Cc: Linux-Kernel@Vger. Kernel. Org, Omer Shpigelman, Tomer Tayar
On Tue, Jul 30, 2019 at 1:42 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Tue, Jul 30, 2019 at 12:47:24PM +0300, Oded Gabbay wrote:
> > This patch changes the driver to create two char devices for each ASIC
> > it discovers. This is done to allow system/monitoring applications to
> > query the device for stats, information, idle state and more, while also
> > allowing the deep-learning application to send work to the ASIC.
> >
> > One char device is the original device, hlX. IOCTL calls through this
> > device file can perform any task on the device (compute, memory, queries).
> > The open function for this device will fail if it was called before but
> > the file-descriptor it created was not completely released yet (the
> > release callback function is not called from the kernel until all
> > instances of that FD are closed). The driver needs to keep this behavior
> > to support backward compatibility with existing userspace, which count
> > that the open will fail if the device is "occupied".
> >
> > The second char device is called "hl_controlDx", where x is the minor
> > number of the original char device + 64 (HL_CONTROL_MINOR). Applications
> > that open this device can only call the INFO IOCTL. There is no limitation
> > on the number of applications opening this device.
> >
> > Signed-off-by: Oded Gabbay <oded.gabbay@gmail.com>
>
> This reminds me of the old tty "control" devices we finally got rid of
> many years ago. The more things change... :)
>
> Anyway, looks sane to me. If you are ok with this userspace api, no
> objection from me.
>
> Only one comment below:
>
> > --- a/drivers/misc/habanalabs/habanalabs.h
> > +++ b/drivers/misc/habanalabs/habanalabs.h
> > @@ -51,6 +51,8 @@
> > /* MMU */
> > #define MMU_HASH_TABLE_BITS 7 /* 1 << 7 buckets */
> >
> > +#define HL_CONTROL_MINOR 64
>
> Don't try to segment your "dev" and "control" device nodes by minor
> number range, as you will run into problems once you have a system with
> more than 64 of these in the box at once.
>
> Just use the whole range, and do:
> dev = minor N
> control = minor N+1
> and so on. That makes your control device node an "odd" minor and your
> "real" device an "even".
>
> Given that all existing systems should be using devtmpfs, you should not
> have any problem with changing the minor number of them at the next
> reboot, right?
>
> thanks,
>
> greg k-h
I agree, will change that and re-send.
Oded
^ permalink raw reply [flat|nested] 10+ messages in thread