lkml.org 
[lkml]   [2019]   [Jul]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
From
Subject[PATCH 4/9] habanalabs: don't change frequency if user context is valid
Date
Instead of using the FD open counter to check if there is a user opened on
the device, check if there is a valid user context.

Use the new lazy context creation mutex to protect against multiple calls
to change the frequency.

This is in preparation for removing the FD open counter.

Signed-off-by: Oded Gabbay <oded.gabbay@gmail.com>
---
drivers/misc/habanalabs/context.c | 9 +++++
drivers/misc/habanalabs/device.c | 40 +++++++++--------------
drivers/misc/habanalabs/goya/goya_hwmgr.c | 11 +++----
drivers/misc/habanalabs/habanalabs.h | 4 +--
drivers/misc/habanalabs/habanalabs_drv.c | 34 ++++++++-----------
5 files changed, 45 insertions(+), 53 deletions(-)

diff --git a/drivers/misc/habanalabs/context.c b/drivers/misc/habanalabs/context.c
index 3c1f7c29e908..a4cd47ced94d 100644
--- a/drivers/misc/habanalabs/context.c
+++ b/drivers/misc/habanalabs/context.c
@@ -220,9 +220,18 @@ bool hl_ctx_is_valid(struct hl_fpriv *hpriv)
if (rc) {
dev_err(hdev->dev, "Failed to create context %d\n", rc);
valid = false;
+ goto unlock_mutex;
}
+
+ /* 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);
}

+unlock_mutex:
mutex_unlock(&hdev->lazy_ctx_creation_lock);

return valid;
diff --git a/drivers/misc/habanalabs/device.c b/drivers/misc/habanalabs/device.c
index b63beb73ec76..a791a1b58215 100644
--- a/drivers/misc/habanalabs/device.c
+++ b/drivers/misc/habanalabs/device.c
@@ -289,9 +289,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->lazy_ctx_creation_lock);
+
+ if (!hdev->user_ctx)
hl_device_set_frequency(hdev, PLL_LOW);

+ mutex_unlock(&hdev->lazy_ctx_creation_lock);
+
schedule_delayed_work(&hdev->work_freq,
usecs_to_jiffies(HL_PLL_LOW_JOB_FREQ_USEC));
}
@@ -341,7 +345,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);
@@ -390,38 +394,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)
- return 0;
-
- ret = atomic_cmpxchg(&hdev->curr_pll_profile, old_freq, freq);
- if (ret == freq)
+ if ((hdev->pm_mng_profile == PM_MANUAL) ||
+ (hdev->curr_pll_profile == 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;
}

diff --git a/drivers/misc/habanalabs/goya/goya_hwmgr.c b/drivers/misc/habanalabs/goya/goya_hwmgr.c
index a51d836542a1..8522c6e0a25e 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->lazy_ctx_creation_lock);

- if (atomic_read(&hdev->fd_open_cnt) > 0) {
+ if (hdev->user_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 user 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->lazy_ctx_creation_lock);
out:
return count;
}
diff --git a/drivers/misc/habanalabs/habanalabs.h b/drivers/misc/habanalabs/habanalabs.h
index d0e55839d673..6354fc88ef8a 100644
--- a/drivers/misc/habanalabs/habanalabs.h
+++ b/drivers/misc/habanalabs/habanalabs.h
@@ -1190,10 +1190,10 @@ 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.
@@ -1268,9 +1268,9 @@ struct hl_device {
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 b2c52e46e130..a781aa936160 100644
--- a/drivers/misc/habanalabs/habanalabs_drv.c
+++ b/drivers/misc/habanalabs/habanalabs_drv.c
@@ -95,42 +95,40 @@ int hl_device_open(struct inode *inode, struct file *filp)
return -ENXIO;
}

+ hpriv = kzalloc(sizeof(*hpriv), GFP_KERNEL);
+ if (!hpriv)
+ return -ENOMEM;
+
mutex_lock(&hdev->fd_open_cnt_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)) {
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;
+ rc = -EBUSY;
+ goto out_err;
}

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;
- }
-
hpriv->hdev = hdev;
filp->private_data = hpriv;
hpriv->filp = filp;
@@ -143,19 +141,13 @@ int hl_device_open(struct inode *inode, struct file *filp)

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
- */
- hl_device_set_frequency(hdev, PLL_HIGH);
-
hl_debugfs_add_file(hpriv);

return 0;

-close_device:
- atomic_dec(&hdev->fd_open_cnt);
+out_err:
+ mutex_unlock(&hdev->fd_open_cnt_lock);
+ kfree(hpriv);
return rc;
}

--
2.17.1
\
 
 \ /
  Last update: 2019-07-28 13:29    [W:0.066 / U:4.204 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site