[PATCH] venus: fix hw overload error log condition

[Mansur Alisha Shaik]

In current video driver, frequency is calculated for all the
running video instances and check aganist maximum supported frequency.
If both calculated frequency and maximum supported frequency are same,
even then HW overload error is printed.
Fix this by printing error log only when frequency is greater than
maximum supported frequency.

Signed-off-by: Mansur Alisha Shaik 
---
 drivers/media/platform/qcom/venus/pm_helpers.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c 
b/drivers/media/platform/qcom/venus/pm_helpers.c
index dfe3ee8..9714ca7 100644
--- a/drivers/media/platform/qcom/venus/pm_helpers.c
+++ b/drivers/media/platform/qcom/venus/pm_helpers.c
@@ -1083,7 +1083,7 @@ static int load_scale_v4(struct venus_inst *inst)
 
freq = max(freq_core1, freq_core2);
 
-   if (freq >= table[0].freq) {
+   if (freq > table[0].freq) {
freq = table[0].freq;
dev_warn(dev, "HW is overloaded, needed: %lu max: %lu\n",
 freq, table[0].freq);
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member 
of Code Aurora Forum, hosted by The Linux Foundation

[RESEND] venus: fix calculating mbps in calculate_inst_freq()

[Mansur Alisha Shaik]

Currently in calculate_inst_freq(), video driver is calculating
macro blocks per frame in stead of macro blocks per second(mpbs).
Which results frequency is always setting to lower frequency (150MB)
as per frequency table for sc7180. Hence the playback is not smooth.

Corrected this by correcting the mbps calculation in calculate_inst_freq().

Signed-off-by: Mansur Alisha Shaik 
---
 drivers/media/platform/qcom/venus/pm_helpers.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c 
b/drivers/media/platform/qcom/venus/pm_helpers.c
index 57877ea..001513f 100644
--- a/drivers/media/platform/qcom/venus/pm_helpers.c
+++ b/drivers/media/platform/qcom/venus/pm_helpers.c
@@ -928,7 +928,7 @@ static unsigned long calculate_inst_freq(struct venus_inst 
*inst,
u32 fps = (u32)inst->fps;
u32 mbs_per_sec;
 
-   mbs_per_sec = load_per_instance(inst) / fps;
+   mbs_per_sec = load_per_instance(inst);
 
vpp_freq = mbs_per_sec * inst->clk_data.codec_freq_data->vpp_freq;
/* 21 / 20 is overhead factor */
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member 
of Code Aurora Forum, hosted by The Linux Foundation

[PATCH] venus: fix calculating mbps in calculate_inst_freq()

[Mansur Alisha Shaik]

Currently in calculate_inst_freq() video driver is calculating
macro blocks per frame in steam of macro blocks per second(mpbs).
Which results frequency is always setting to lower frequency (150MB)
as per frequency table for sc7180. Hence the playback is not smooth.

Corrected this by correcting the mbps calculation.

Signed-off-by: Mansur Alisha Shaik 
---
 drivers/media/platform/qcom/venus/pm_helpers.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c 
b/drivers/media/platform/qcom/venus/pm_helpers.c
index 57877ea..001513f 100644
--- a/drivers/media/platform/qcom/venus/pm_helpers.c
+++ b/drivers/media/platform/qcom/venus/pm_helpers.c
@@ -928,7 +928,7 @@ static unsigned long calculate_inst_freq(struct venus_inst 
*inst,
u32 fps = (u32)inst->fps;
u32 mbs_per_sec;
 
-   mbs_per_sec = load_per_instance(inst) / fps;
+   mbs_per_sec = load_per_instance(inst);
 
vpp_freq = mbs_per_sec * inst->clk_data.codec_freq_data->vpp_freq;
/* 21 / 20 is overhead factor */
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member 
of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v4 1/4] venus: core: change clk enable and disable order in resume and suspend

[Mansur Alisha Shaik]

Currently video driver is voting after clk enable and un voting
before clk disable. This is incorrect, video driver should vote
before clk enable and unvote after clk disable.

Corrected this by changing the order of clk enable and clk disable.

Fixes: 07f8f22a33a9e ("media: venus: core: remove CNOC voting while device
suspend")
Signed-off-by: Mansur Alisha Shaik 
Reviewed-by: Stephen Boyd 
---
 drivers/media/platform/qcom/venus/core.c | 17 ++---
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/core.c 
b/drivers/media/platform/qcom/venus/core.c
index 6103aaf..52a3886 100644
--- a/drivers/media/platform/qcom/venus/core.c
+++ b/drivers/media/platform/qcom/venus/core.c
@@ -355,13 +355,16 @@ static __maybe_unused int venus_runtime_suspend(struct 
device *dev)
if (ret)
return ret;
 
+   if (pm_ops->core_power) {
+   ret = pm_ops->core_power(dev, POWER_OFF);
+   if (ret)
+   return ret;
+   }
+
ret = icc_set_bw(core->cpucfg_path, 0, 0);
if (ret)
return ret;
 
-   if (pm_ops->core_power)
-   ret = pm_ops->core_power(dev, POWER_OFF);
-
return ret;
 }
 
@@ -371,16 +374,16 @@ static __maybe_unused int venus_runtime_resume(struct 
device *dev)
const struct venus_pm_ops *pm_ops = core->pm_ops;
int ret;
 
+   ret = icc_set_bw(core->cpucfg_path, 0, kbps_to_icc(1000));
+   if (ret)
+   return ret;
+
if (pm_ops->core_power) {
ret = pm_ops->core_power(dev, POWER_ON);
if (ret)
return ret;
}
 
-   ret = icc_set_bw(core->cpucfg_path, 0, kbps_to_icc(1000));
-   if (ret)
-   return ret;
-
return hfi_core_resume(core, false);
 }
 
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member 
of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v4 2/4] venus: core: vote for video-mem path

[Mansur Alisha Shaik]

Currently video driver is voting for venus0-ebi path during buffer
processing with an average bandwidth of all the instances and
unvoting during session release.

While video streaming when we try to do XO-SD using the command
"echo mem > /sys/power/state command" , device is not entering
to suspend state and from interconnect summary seeing votes for venus0-ebi

Corrected this by voting for venus0-ebi path in venus_runtime_resume()
and unvote during venus_runtime_suspend().

Fixes: 07f8f22a33a9e ("media: venus: core: remove CNOC voting while device
suspend")
Signed-off-by: Mansur Alisha Shaik 
Reviewed-by: Stephen Boyd 
---
Changes in v4:
- As per Stanimir's comments, corrected fixes tag

 drivers/media/platform/qcom/venus/core.c | 17 -
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/qcom/venus/core.c 
b/drivers/media/platform/qcom/venus/core.c
index 52a3886..fa363b8 100644
--- a/drivers/media/platform/qcom/venus/core.c
+++ b/drivers/media/platform/qcom/venus/core.c
@@ -363,7 +363,18 @@ static __maybe_unused int venus_runtime_suspend(struct 
device *dev)
 
ret = icc_set_bw(core->cpucfg_path, 0, 0);
if (ret)
-   return ret;
+   goto err_cpucfg_path;
+
+   ret = icc_set_bw(core->video_path, 0, 0);
+   if (ret)
+   goto err_video_path;
+
+   return ret;
+
+err_video_path:
+   icc_set_bw(core->cpucfg_path, kbps_to_icc(1000), 0);
+err_cpucfg_path:
+   pm_ops->core_power(dev, POWER_ON);
 
return ret;
 }
@@ -374,6 +385,10 @@ static __maybe_unused int venus_runtime_resume(struct 
device *dev)
const struct venus_pm_ops *pm_ops = core->pm_ops;
int ret;
 
+   ret = icc_set_bw(core->video_path, 0, kbps_to_icc(1000));
+   if (ret)
+   return ret;
+
ret = icc_set_bw(core->cpucfg_path, 0, kbps_to_icc(1000));
if (ret)
return ret;
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member 
of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v4 3/4] venus: core: vote with average bandwidth and peak bandwidth as zero

[Mansur Alisha Shaik]

As per bandwidth table video driver is voting with average bandwidth
for "video-mem" and "cpu-cfg" paths as peak bandwidth is zero
in bandwidth table.

Fixes: 07f8f22a33a9e ("media: venus: core: remove CNOC voting while device
suspend")
Signed-off-by: Mansur Alisha Shaik 
Reviewed-by: Stephen Boyd 
---
Changes in v4:
- As per Stanimir's comments, corrected fixes tag

 drivers/media/platform/qcom/venus/core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/core.c 
b/drivers/media/platform/qcom/venus/core.c
index fa363b8..d5bfd6f 100644
--- a/drivers/media/platform/qcom/venus/core.c
+++ b/drivers/media/platform/qcom/venus/core.c
@@ -385,11 +385,11 @@ static __maybe_unused int venus_runtime_resume(struct 
device *dev)
const struct venus_pm_ops *pm_ops = core->pm_ops;
int ret;
 
-   ret = icc_set_bw(core->video_path, 0, kbps_to_icc(1000));
+   ret = icc_set_bw(core->video_path, kbps_to_icc(2), 0);
if (ret)
return ret;
 
-   ret = icc_set_bw(core->cpucfg_path, 0, kbps_to_icc(1000));
+   ret = icc_set_bw(core->cpucfg_path, kbps_to_icc(1000), 0);
if (ret)
return ret;
 
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member 
of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v4 0/4] Venus - change clk enable, disable order and change bw values

[Mansur Alisha Shaik]

The intention of this patchset is to correct clock enable and disable
order and vote for venus-ebi and cpucfg paths with average bandwidth
instad of peak bandwidth since with current implementation we are seeing
clock related warning during XO-SD and suspend device while video playback

---
Resending v4 series by correcting fixes tag for all patches in series`

Mansur Alisha Shaik (4):
  venus: core: change clk enable and disable order in resume and suspend
  venus: core: vote for video-mem path
  venus: core: vote with average bandwidth and peak bandwidth as zero
  venus: put dummy vote on video-mem path after last session release

 drivers/media/platform/qcom/venus/core.c   | 32 --
 drivers/media/platform/qcom/venus/pm_helpers.c | 10 
 2 files changed, 35 insertions(+), 7 deletions(-)

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member 
of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v4 4/4] venus: put dummy vote on video-mem path after last session release

[Mansur Alisha Shaik]

As per current implementation, video driver is unvoting "videom-mem" path
for last video session during vdec_session_release().
While video playback when we try to suspend device, we see video clock
warnings since votes are already removed during vdec_session_release().

corrected this by putting dummy vote on "video-mem" after last video
session release and unvoting it during suspend.

Fixes: 07f8f22a33a9e ("media: venus: core: remove CNOC voting while device
suspend")
Signed-off-by: Mansur Alisha Shaik 
Reviewed-by: Stephen Boyd 
---
Changes in v4:
- As per Stanimir's comments, corrected fixes tag

 drivers/media/platform/qcom/venus/pm_helpers.c | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c 
b/drivers/media/platform/qcom/venus/pm_helpers.c
index 57877ea..0ebba8e 100644
--- a/drivers/media/platform/qcom/venus/pm_helpers.c
+++ b/drivers/media/platform/qcom/venus/pm_helpers.c
@@ -212,6 +212,16 @@ static int load_scale_bw(struct venus_core *core)
}
mutex_unlock(>lock);
 
+   /*
+* keep minimum bandwidth vote for "video-mem" path,
+* so that clks can be disabled during vdec_session_release().
+* Actual bandwidth drop will be done during device supend
+* so that device can power down without any warnings.
+*/
+
+   if (!total_avg && !total_peak)
+   total_avg = kbps_to_icc(1000);
+
dev_dbg(core->dev, VDBGL "total: avg_bw: %u, peak_bw: %u\n",
total_avg, total_peak);
 
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member 
of Code Aurora Forum, hosted by The Linux Foundation

[RESEND v3 3/4] venus: core: vote with average bandwidth and peak bandwidth as zero

[Mansur Alisha Shaik]

As per bandwidth table video driver is voting with average bandwidth
for "video-mem" and "cpu-cfg" paths as peak bandwidth is zero
in bandwidth table.

Fixes: 7482a983d ("media: venus: redesign clocks and pm domains control")
Signed-off-by: Mansur Alisha Shaik 
Reviewed-by: Stephen Boyd 
---
 drivers/media/platform/qcom/venus/core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/core.c 
b/drivers/media/platform/qcom/venus/core.c
index fa363b8..d5bfd6f 100644
--- a/drivers/media/platform/qcom/venus/core.c
+++ b/drivers/media/platform/qcom/venus/core.c
@@ -385,11 +385,11 @@ static __maybe_unused int venus_runtime_resume(struct 
device *dev)
const struct venus_pm_ops *pm_ops = core->pm_ops;
int ret;
 
-   ret = icc_set_bw(core->video_path, 0, kbps_to_icc(1000));
+   ret = icc_set_bw(core->video_path, kbps_to_icc(2), 0);
if (ret)
return ret;
 
-   ret = icc_set_bw(core->cpucfg_path, 0, kbps_to_icc(1000));
+   ret = icc_set_bw(core->cpucfg_path, kbps_to_icc(1000), 0);
if (ret)
return ret;
 
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member 
of Code Aurora Forum, hosted by The Linux Foundation

[RESEND v3 0/4] Venus - change clk enable, disable order and change bw values

[Mansur Alisha Shaik]

The intention of this patchset is to correct clock enable and disable
order and vote for venus-ebi and cpucfg paths with average bandwidth
instad of peak bandwidth since with current implementation we are seeing
clock related warning during XO-SD and suspend device while video playback

---
Resending v3 patches by correcting fixes tag

Mansur Alisha Shaik (4):
  venus: core: change clk enable and disable order in resume and suspend
  venus: core: vote for video-mem path
  venus: core: vote with average bandwidth and peak bandwidth as zero
  venus: put dummy vote on video-mem path after last session release

 drivers/media/platform/qcom/venus/core.c   | 32 --
 drivers/media/platform/qcom/venus/pm_helpers.c | 10 
 2 files changed, 35 insertions(+), 7 deletions(-)

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member 
of Code Aurora Forum, hosted by The Linux Foundation

[RESEND v3 4/4] venus: put dummy vote on video-mem path after last session release

[Mansur Alisha Shaik]

As per current implementation, video driver is unvoting "videom-mem" path
for last video session during vdec_session_release().
While video playback when we try to suspend device, we see video clock
warnings since votes are already removed during vdec_session_release().

corrected this by putting dummy vote on "video-mem" after last video
session release and unvoting it during suspend.

Fixes: 7482a983d ("media: venus: redesign clocks and pm domains control")
Signed-off-by: Mansur Alisha Shaik 
Reviewed-by: Stephen Boyd 
---
 drivers/media/platform/qcom/venus/pm_helpers.c | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c 
b/drivers/media/platform/qcom/venus/pm_helpers.c
index 57877ea..0ebba8e 100644
--- a/drivers/media/platform/qcom/venus/pm_helpers.c
+++ b/drivers/media/platform/qcom/venus/pm_helpers.c
@@ -212,6 +212,16 @@ static int load_scale_bw(struct venus_core *core)
}
mutex_unlock(>lock);
 
+   /*
+* keep minimum bandwidth vote for "video-mem" path,
+* so that clks can be disabled during vdec_session_release().
+* Actual bandwidth drop will be done during device supend
+* so that device can power down without any warnings.
+*/
+
+   if (!total_avg && !total_peak)
+   total_avg = kbps_to_icc(1000);
+
dev_dbg(core->dev, VDBGL "total: avg_bw: %u, peak_bw: %u\n",
total_avg, total_peak);
 
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member 
of Code Aurora Forum, hosted by The Linux Foundation

[RESEND v3 1/4] venus: core: change clk enable and disable order in resume and suspend

[Mansur Alisha Shaik]

Currently video driver is voting after clk enable and un voting
before clk disable. This is incorrect, video driver should vote
before clk enable and unvote after clk disable.

Corrected this by changing the order of clk enable and clk disable.

Fixes: 07f8f22a33a9e ("media: venus: core: remove CNOC voting while device
suspend")
Signed-off-by: Mansur Alisha Shaik 
Reviewed-by: Stephen Boyd 
---
- Resending by corecting fixes tag

 drivers/media/platform/qcom/venus/core.c | 17 ++---
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/core.c 
b/drivers/media/platform/qcom/venus/core.c
index 6103aaf..52a3886 100644
--- a/drivers/media/platform/qcom/venus/core.c
+++ b/drivers/media/platform/qcom/venus/core.c
@@ -355,13 +355,16 @@ static __maybe_unused int venus_runtime_suspend(struct 
device *dev)
if (ret)
return ret;
 
+   if (pm_ops->core_power) {
+   ret = pm_ops->core_power(dev, POWER_OFF);
+   if (ret)
+   return ret;
+   }
+
ret = icc_set_bw(core->cpucfg_path, 0, 0);
if (ret)
return ret;
 
-   if (pm_ops->core_power)
-   ret = pm_ops->core_power(dev, POWER_OFF);
-
return ret;
 }
 
@@ -371,16 +374,16 @@ static __maybe_unused int venus_runtime_resume(struct 
device *dev)
const struct venus_pm_ops *pm_ops = core->pm_ops;
int ret;
 
+   ret = icc_set_bw(core->cpucfg_path, 0, kbps_to_icc(1000));
+   if (ret)
+   return ret;
+
if (pm_ops->core_power) {
ret = pm_ops->core_power(dev, POWER_ON);
if (ret)
return ret;
}
 
-   ret = icc_set_bw(core->cpucfg_path, 0, kbps_to_icc(1000));
-   if (ret)
-   return ret;
-
return hfi_core_resume(core, false);
 }
 
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member 
of Code Aurora Forum, hosted by The Linux Foundation

[RESEND v3 2/4] venus: core: vote for video-mem path

[Mansur Alisha Shaik]

Currently video driver is voting for venus0-ebi path during buffer
processing with an average bandwidth of all the instances and
unvoting during session release.

While video streaming when we try to do XO-SD using the command
"echo mem > /sys/power/state command" , device is not entering
to suspend state and from interconnect summary seeing votes for venus0-ebi

Corrected this by voting for venus0-ebi path in venus_runtime_resume()
and unvote during venus_runtime_suspend().

Fixes: 7482a983d ("media: venus: redesign clocks and pm domains control")
Signed-off-by: Mansur Alisha Shaik 
Reviewed-by: Stephen Boyd 
---
 drivers/media/platform/qcom/venus/core.c | 17 -
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/qcom/venus/core.c 
b/drivers/media/platform/qcom/venus/core.c
index 52a3886..fa363b8 100644
--- a/drivers/media/platform/qcom/venus/core.c
+++ b/drivers/media/platform/qcom/venus/core.c
@@ -363,7 +363,18 @@ static __maybe_unused int venus_runtime_suspend(struct 
device *dev)
 
ret = icc_set_bw(core->cpucfg_path, 0, 0);
if (ret)
-   return ret;
+   goto err_cpucfg_path;
+
+   ret = icc_set_bw(core->video_path, 0, 0);
+   if (ret)
+   goto err_video_path;
+
+   return ret;
+
+err_video_path:
+   icc_set_bw(core->cpucfg_path, kbps_to_icc(1000), 0);
+err_cpucfg_path:
+   pm_ops->core_power(dev, POWER_ON);
 
return ret;
 }
@@ -374,6 +385,10 @@ static __maybe_unused int venus_runtime_resume(struct 
device *dev)
const struct venus_pm_ops *pm_ops = core->pm_ops;
int ret;
 
+   ret = icc_set_bw(core->video_path, 0, kbps_to_icc(1000));
+   if (ret)
+   return ret;
+
ret = icc_set_bw(core->cpucfg_path, 0, kbps_to_icc(1000));
if (ret)
return ret;
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member 
of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v5 0/3] venus: core: add shutdown callback for venus

[Mansur Alisha Shaik]

Add shutdown callback for venus driver.
Handle race conditions in concurrency usecases like
multiple browser YouTube browser tabs(approx 50 tabs)
graphics_Stress, WiFi ON/OFF, Bluetooth ON/OF,
and reboot in parallel.

Mansur Alisha Shaik (3):
  venus: core: handle race condititon for core ops
  venus: handle use after free for iommu_map/iommu_unmap
  venus: core: add shutdown callback for venus

 drivers/media/platform/qcom/venus/core.c |  9 +
 drivers/media/platform/qcom/venus/firmware.c | 17 +
 drivers/media/platform/qcom/venus/hfi.c  | 12 
 3 files changed, 34 insertions(+), 4 deletions(-)

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member 
of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v5 1/3] venus: core: handle race condititon for core ops

[Mansur Alisha Shaik]

For core ops we are having only write protect but there
is no read protect, because of this in multithreading
and concurrency, one CPU core is reading without wait
which is causing the NULL pointer dereferece crash.

one such scenario is as show below, where in one CPU
core, core->ops becoming NULL and in another CPU core
calling core->ops->session_init().

CPU: core-7:
Call trace:
 hfi_session_init+0x180/0x1dc [venus_core]
 vdec_queue_setup+0x9c/0x364 [venus_dec]
 vb2_core_reqbufs+0x1e4/0x368 [videobuf2_common]
 vb2_reqbufs+0x4c/0x64 [videobuf2_v4l2]
 v4l2_m2m_reqbufs+0x50/0x84 [v4l2_mem2mem]
 v4l2_m2m_ioctl_reqbufs+0x2c/0x38 [v4l2_mem2mem]
 v4l_reqbufs+0x4c/0x5c
__video_do_ioctl+0x2b0/0x39c

CPU: core-0:
Call trace:
 venus_shutdown+0x98/0xfc [venus_core]
 venus_sys_error_handler+0x64/0x148 [venus_core]
 process_one_work+0x210/0x3d0
 worker_thread+0x248/0x3f4
 kthread+0x11c/0x12c

Signed-off-by: Mansur Alisha Shaik 
Acked-by: Stanimir Varbanov 
---
Changes in v5:
- Addressed review comments by stan in v4 version

 drivers/media/platform/qcom/venus/hfi.c | 12 
 1 file changed, 12 insertions(+)

diff --git a/drivers/media/platform/qcom/venus/hfi.c 
b/drivers/media/platform/qcom/venus/hfi.c
index a59022a..638ed5c 100644
--- a/drivers/media/platform/qcom/venus/hfi.c
+++ b/drivers/media/platform/qcom/venus/hfi.c
@@ -198,6 +198,18 @@ int hfi_session_init(struct venus_inst *inst, u32 pixfmt)
const struct hfi_ops *ops = core->ops;
int ret;
 
+   /*
+* If core shutdown is in progress or if we are in system
+* recovery, return an error as during system error recovery
+* session_init() can't pass successfully
+*/
+   mutex_lock(>lock);
+   if (!core->ops || core->sys_error) {
+   mutex_unlock(>lock);
+   return -EIO;
+   }
+   mutex_unlock(>lock);
+
if (inst->state != INST_UNINIT)
return -EINVAL;
 
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member 
of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v5 2/3] venus: handle use after free for iommu_map/iommu_unmap

[Mansur Alisha Shaik]

In concurrency usecase and reboot scenario we are seeing muliple
crashes related to iommu_map/iommu_unamp of core->fw.iommu_domain.

In one case we are seeing "Unable to handle kernel NULL pointer
dereference at virtual address 0008" crash, this is
because of core->fw.iommu_domain in venus_firmware_deinit() and
trying to map in venus_boot() during venus_sys_error_handler()

Call trace:
 __iommu_map+0x4c/0x348
 iommu_map+0x5c/0x70
 venus_boot+0x184/0x230 [venus_core]
 venus_sys_error_handler+0xa0/0x14c [venus_core]
 process_one_work+0x210/0x3d0
 worker_thread+0x248/0x3f4
 kthread+0x11c/0x12c
 ret_from_fork+0x10/0x18

In second case we are seeing "Unable to handle kernel paging request
at virtual address 006b6b6b6b6b6b9b" crash, this is because of
unmapping iommu domain which is already unmapped.

Call trace:
 venus_remove+0xf8/0x108 [venus_core]
 venus_core_shutdown+0x1c/0x34 [venus_core]
 platform_drv_shutdown+0x28/0x34
 device_shutdown+0x154/0x1fc
 kernel_restart_prepare+0x40/0x4c
 kernel_restart+0x1c/0x64
 __arm64_sys_reboot+0x190/0x238
 el0_svc_common+0xa4/0x154
 el0_svc_compat_handler+0x2c/0x38
 el0_svc_compat+0x8/0x10

Signed-off-by: Mansur Alisha Shaik 
---
 drivers/media/platform/qcom/venus/firmware.c | 17 +
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/firmware.c 
b/drivers/media/platform/qcom/venus/firmware.c
index 1db64a8..d03e2dd 100644
--- a/drivers/media/platform/qcom/venus/firmware.c
+++ b/drivers/media/platform/qcom/venus/firmware.c
@@ -171,9 +171,14 @@ static int venus_shutdown_no_tz(struct venus_core *core)
 
iommu = core->fw.iommu_domain;
 
-   unmapped = iommu_unmap(iommu, VENUS_FW_START_ADDR, mapped);
-   if (unmapped != mapped)
-   dev_err(dev, "failed to unmap firmware\n");
+   if (core->fw.mapped_mem_size && iommu) {
+   unmapped = iommu_unmap(iommu, VENUS_FW_START_ADDR, mapped);
+
+   if (unmapped != mapped)
+   dev_err(dev, "failed to unmap firmware\n");
+   else
+   core->fw.mapped_mem_size = 0;
+   }
 
return 0;
 }
@@ -305,7 +310,11 @@ void venus_firmware_deinit(struct venus_core *core)
iommu = core->fw.iommu_domain;
 
iommu_detach_device(iommu, core->fw.dev);
-   iommu_domain_free(iommu);
+
+   if (core->fw.iommu_domain) {
+   iommu_domain_free(iommu);
+   core->fw.iommu_domain = NULL;
+   }
 
platform_device_unregister(to_platform_device(core->fw.dev));
 }
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member 
of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v5 3/3] venus: core: add shutdown callback for venus

[Mansur Alisha Shaik]

After the SMMU translation is disabled in the
arm-smmu shutdown callback during reboot, if
any subsystem are still alive then IOVAs they
are using will become PAs on bus, which may
lead to crash.

So implemented shutdown callback, which detach iommu maps.

Signed-off-by: Mansur Alisha Shaik 
Acked-by: Stanimir Varbanov 
---
 drivers/media/platform/qcom/venus/core.c | 9 +
 1 file changed, 9 insertions(+)

diff --git a/drivers/media/platform/qcom/venus/core.c 
b/drivers/media/platform/qcom/venus/core.c
index 6103aaf..65b71ac 100644
--- a/drivers/media/platform/qcom/venus/core.c
+++ b/drivers/media/platform/qcom/venus/core.c
@@ -345,6 +345,14 @@ static int venus_remove(struct platform_device *pdev)
return ret;
 }
 
+static void venus_core_shutdown(struct platform_device *pdev)
+{
+   struct venus_core *core = platform_get_drvdata(pdev);
+
+   venus_shutdown(core);
+   venus_firmware_deinit(core);
+}
+
 static __maybe_unused int venus_runtime_suspend(struct device *dev)
 {
struct venus_core *core = dev_get_drvdata(dev);
@@ -602,6 +610,7 @@ static struct platform_driver qcom_venus_driver = {
.of_match_table = venus_dt_match,
.pm = _pm_ops,
},
+   .shutdown = venus_core_shutdown,
 };
 module_platform_driver(qcom_venus_driver);
 
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member 
of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v3 4/4] venus: put dummy vote on video-mem path after last session release

[Mansur Alisha Shaik]

As per current implementation, video driver is unvoting "videom-mem" path
for last video session during vdec_session_release().
While video playback when we try to suspend device, we see video clock
warnings since votes are already removed during vdec_session_release().

corrected this by putting dummy vote on "video-mem" after last video
session release and unvoting it during suspend.

Fixes: 7482a983d ("media: venus: redesign clocks and pm domains control")
Signed-off-by: Mansur Alisha Shaik 
---
Changes in v3:
- Added fixes tag

 drivers/media/platform/qcom/venus/pm_helpers.c | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c 
b/drivers/media/platform/qcom/venus/pm_helpers.c
index 57877ea..ca09ea8 100644
--- a/drivers/media/platform/qcom/venus/pm_helpers.c
+++ b/drivers/media/platform/qcom/venus/pm_helpers.c
@@ -212,6 +212,16 @@ static int load_scale_bw(struct venus_core *core)
}
mutex_unlock(>lock);
 
+   /*
+* keep minimum bandwidth vote for "video-mem" path,
+* so that clks can be disabled during vdec_session_release().
+* Actual bandwidth drop will be done during device supend
+* so that device can power down without any warnings.
+*/
+
+   if (!total_avg && !total_peak)
+   total_avg = kbps_to_icc(1000);
+
dev_dbg(core->dev, VDBGL "total: avg_bw: %u, peak_bw: %u\n",
total_avg, total_peak);
 
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member 
of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v3 2/4] venus: core: vote for video-mem path

[Mansur Alisha Shaik]

Currently video driver is voting for venus0-ebi path during buffer
processing with an average bandwidth of all the instances and
unvoting during session release.

While video streaming when we try to do XO-SD using the command
"echo mem > /sys/power/state command" , device is not entering
to suspend state and from interconnect summary seeing votes for venus0-ebi

Corrected this by voting for venus0-ebi path in venus_runtime_resume()
and unvote during venus_runtime_suspend().

Fixes: 7482a983d ("media: venus: redesign clocks and pm domains control")
Signed-off-by: Mansur Alisha Shaik 
---
Changes in v3:
- Addressed review comments by Stephen Boyd

 drivers/media/platform/qcom/venus/core.c | 17 -
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/qcom/venus/core.c 
b/drivers/media/platform/qcom/venus/core.c
index 52a3886..fa363b8 100644
--- a/drivers/media/platform/qcom/venus/core.c
+++ b/drivers/media/platform/qcom/venus/core.c
@@ -363,7 +363,18 @@ static __maybe_unused int venus_runtime_suspend(struct 
device *dev)
 
ret = icc_set_bw(core->cpucfg_path, 0, 0);
if (ret)
-   return ret;
+   goto err_cpucfg_path;
+
+   ret = icc_set_bw(core->video_path, 0, 0);
+   if (ret)
+   goto err_video_path;
+
+   return ret;
+
+err_video_path:
+   icc_set_bw(core->cpucfg_path, kbps_to_icc(1000), 0);
+err_cpucfg_path:
+   pm_ops->core_power(dev, POWER_ON);
 
return ret;
 }
@@ -374,6 +385,10 @@ static __maybe_unused int venus_runtime_resume(struct 
device *dev)
const struct venus_pm_ops *pm_ops = core->pm_ops;
int ret;
 
+   ret = icc_set_bw(core->video_path, 0, kbps_to_icc(1000));
+   if (ret)
+   return ret;
+
ret = icc_set_bw(core->cpucfg_path, 0, kbps_to_icc(1000));
if (ret)
return ret;
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member 
of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v3 3/4] venus: core: vote with average bandwidth and peak bandwidth as zero

[Mansur Alisha Shaik]

As per bandwidth table video driver is voting with average bandwidth
for "video-mem" and "cpu-cfg" paths as peak bandwidth is zero
in bandwidth table.

Fixes: 7482a983d ("media: venus: redesign clocks and pm domains control")
Signed-off-by: Mansur Alisha Shaik 
---
Changes in v3:
- Added fixes tag

 drivers/media/platform/qcom/venus/core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/core.c 
b/drivers/media/platform/qcom/venus/core.c
index fa363b8..d5bfd6f 100644
--- a/drivers/media/platform/qcom/venus/core.c
+++ b/drivers/media/platform/qcom/venus/core.c
@@ -385,11 +385,11 @@ static __maybe_unused int venus_runtime_resume(struct 
device *dev)
const struct venus_pm_ops *pm_ops = core->pm_ops;
int ret;
 
-   ret = icc_set_bw(core->video_path, 0, kbps_to_icc(1000));
+   ret = icc_set_bw(core->video_path, kbps_to_icc(2), 0);
if (ret)
return ret;
 
-   ret = icc_set_bw(core->cpucfg_path, 0, kbps_to_icc(1000));
+   ret = icc_set_bw(core->cpucfg_path, kbps_to_icc(1000), 0);
if (ret)
return ret;
 
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member 
of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v3 1/4] venus: core: change clk enable and disable order in resume and suspend

[Mansur Alisha Shaik]

Currently video driver is voting after clk enable and un voting
before clk disable. This is incorrect, video driver should vote
before clk enable and unvote after clk disable.

Corrected this by changing the order of clk enable and clk disable.

Fixes: 7482a983d ("media: venus: redesign clocks and pm domains control")
Signed-off-by: Mansur Alisha Shaik 
Reviewed-by: Stephen Boyd 
---
 drivers/media/platform/qcom/venus/core.c | 17 ++---
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/core.c 
b/drivers/media/platform/qcom/venus/core.c
index 6103aaf..52a3886 100644
--- a/drivers/media/platform/qcom/venus/core.c
+++ b/drivers/media/platform/qcom/venus/core.c
@@ -355,13 +355,16 @@ static __maybe_unused int venus_runtime_suspend(struct 
device *dev)
if (ret)
return ret;
 
+   if (pm_ops->core_power) {
+   ret = pm_ops->core_power(dev, POWER_OFF);
+   if (ret)
+   return ret;
+   }
+
ret = icc_set_bw(core->cpucfg_path, 0, 0);
if (ret)
return ret;
 
-   if (pm_ops->core_power)
-   ret = pm_ops->core_power(dev, POWER_OFF);
-
return ret;
 }
 
@@ -371,16 +374,16 @@ static __maybe_unused int venus_runtime_resume(struct 
device *dev)
const struct venus_pm_ops *pm_ops = core->pm_ops;
int ret;
 
+   ret = icc_set_bw(core->cpucfg_path, 0, kbps_to_icc(1000));
+   if (ret)
+   return ret;
+
if (pm_ops->core_power) {
ret = pm_ops->core_power(dev, POWER_ON);
if (ret)
return ret;
}
 
-   ret = icc_set_bw(core->cpucfg_path, 0, kbps_to_icc(1000));
-   if (ret)
-   return ret;
-
return hfi_core_resume(core, false);
 }
 
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member 
of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v3 0/4] Venus - change clk enable, disable order and change bw values

[Mansur Alisha Shaik]

The intention of this patchset is to correct clock enable and disable
order and vote for venus-ebi and cpucfg paths with average bandwidth
instad of peak bandwidth since with current implementation we are seeing
clock related warning during XO-SD and suspend device while video playback

Mansur Alisha Shaik (4):
  venus: core: change clk enable and disable order in resume and suspend
  venus: core: vote for video-mem path
  venus: core: vote with average bandwidth and peak bandwidth as zero
  venus: put dummy vote on video-mem path after last session release

 drivers/media/platform/qcom/venus/core.c   | 32 --
 drivers/media/platform/qcom/venus/pm_helpers.c | 10 
 2 files changed, 35 insertions(+), 7 deletions(-)

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member 
of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH v2 2/3] venus: core: cancel pending work items in workqueue

[mansur]

On 2020-09-11 15:52, Stanimir Varbanov wrote:

Hi,

On 9/10/20 3:44 PM, Mansur Alisha Shaik wrote:

In concurrency usecase and reboot scenario we are
observing race condition and seeing NULL pointer
dereference crash. In shutdown path and system
recovery path we are destroying the same mutex
hence seeing crash.

This case is handled by mutex protection and
cancel delayed work items in work queue.

Below is the call trace for the crash
Call trace:
 venus_remove+0xdc/0xec [venus_core]
 venus_core_shutdown+0x1c/0x34 [venus_core]
 platform_drv_shutdown+0x28/0x34
 device_shutdown+0x154/0x1fc
 kernel_restart_prepare+0x40/0x4c
 kernel_restart+0x1c/0x64

Call trace:
 mutex_lock+0x34/0x60
 venus_hfi_destroy+0x28/0x98 [venus_core]
 hfi_destroy+0x1c/0x28 [venus_core]


I queued up [1] and after it this cannot happen anymore because
hfi_destroy() is not called by venus_sys_error_handler().

So I guess this patch is not needed anymore.

[1] https://www.spinics.net/lists/linux-arm-msm/msg70092.html


Yes, this patch is not needed any more. rebased and posted new version
https://lore.kernel.org/patchwork/project/lkml/list/?series=463091


 venus_sys_error_handler+0x60/0x14c [venus_core]
 process_one_work+0x210/0x3d0
 worker_thread+0x248/0x3f4
 kthread+0x11c/0x12c
 ret_from_fork+0x10/0x18

Signed-off-by: Mansur Alisha Shaik 
---
 drivers/media/platform/qcom/venus/core.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/drivers/media/platform/qcom/venus/core.c 
b/drivers/media/platform/qcom/venus/core.c

index c5af428..69aa199 100644
--- a/drivers/media/platform/qcom/venus/core.c
+++ b/drivers/media/platform/qcom/venus/core.c
@@ -323,6 +323,8 @@ static int venus_remove(struct platform_device 
*pdev)

struct device *dev = core->dev;
int ret;

+   cancel_delayed_work_sync(>work);
+
ret = pm_runtime_get_sync(dev);
WARN_ON(ret < 0);

@@ -340,7 +342,9 @@ static int venus_remove(struct platform_device 
*pdev)

if (pm_ops->core_put)
pm_ops->core_put(dev);

+   mutex_lock(>lock);
hfi_destroy(core);
+   mutex_unlock(>lock);

icc_put(core->video_path);
icc_put(core->cpucfg_path);

Re: [PATCH v2 1/3] venus: core: handle race condititon for core ops

[mansur]

On 2020-09-11 15:40, Stanimir Varbanov wrote:

On 9/10/20 3:44 PM, Mansur Alisha Shaik wrote:

For core ops we are having only write protect but there
is no read protect, because of this in multthreading
and concurrency, one CPU core is reading without wait
which is causing the NULL pointer dereferece crash.

one such scenario is as show below, where in one CPU
core, core->ops becoming NULL and in another CPU core
calling core->ops->session_init().

CPU: core-7:
Call trace:
 hfi_session_init+0x180/0x1dc [venus_core]
 vdec_queue_setup+0x9c/0x364 [venus_dec]
 vb2_core_reqbufs+0x1e4/0x368 [videobuf2_common]
 vb2_reqbufs+0x4c/0x64 [videobuf2_v4l2]
 v4l2_m2m_reqbufs+0x50/0x84 [v4l2_mem2mem]
 v4l2_m2m_ioctl_reqbufs+0x2c/0x38 [v4l2_mem2mem]
 v4l_reqbufs+0x4c/0x5c
__video_do_ioctl+0x2b0/0x39c

CPU: core-0:
Call trace:
 venus_shutdown+0x98/0xfc [venus_core]
 venus_sys_error_handler+0x64/0x148 [venus_core]
 process_one_work+0x210/0x3d0
 worker_thread+0x248/0x3f4
 kthread+0x11c/0x12c

Signed-off-by: Mansur Alisha Shaik 
Acked-by: Stanimir Varbanov 
---
Changes in V2:
- Addressed review comments by stan by validating on top
- of 
https://lore.kernel.org/patchwork/project/lkml/list/?series=455962


 drivers/media/platform/qcom/venus/hfi.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/qcom/venus/hfi.c 
b/drivers/media/platform/qcom/venus/hfi.c

index a59022a..3137071 100644
--- a/drivers/media/platform/qcom/venus/hfi.c
+++ b/drivers/media/platform/qcom/venus/hfi.c
@@ -195,7 +195,7 @@ EXPORT_SYMBOL_GPL(hfi_session_create);
 int hfi_session_init(struct venus_inst *inst, u32 pixfmt)
 {
struct venus_core *core = inst->core;
-   const struct hfi_ops *ops = core->ops;
+   const struct hfi_ops *ops;
int ret;



If we are in system error recovery the session_init cannot pass
successfully, so we exit early in the function.

I'd suggest to make it:

	/* If core shutdown is in progress or we are in system error 
	recovery,

return an error */
mutex_lock(>lock);
if (!core->ops || core->sys_error) {
mutex_unclock(>lock);
return -EIO;
}
mutex_unclock(>lock);

Tried above suggestion and ran the failed scenario, I didn't see any 
issue.
Posted new version 
https://lore.kernel.org/patchwork/project/lkml/list/?series=463091

if (inst->state != INST_UNINIT)
@@ -204,10 +204,13 @@ int hfi_session_init(struct venus_inst *inst, 
u32 pixfmt)

inst->hfi_codec = to_codec_type(pixfmt);
reinit_completion(>done);

+   mutex_lock(>lock);
+   ops = core->ops;
ret = ops->session_init(inst, inst->session_type, inst->hfi_codec);
if (ret)
return ret;

+   mutex_unlock(>lock);
ret = wait_session_msg(inst);
if (ret)
return ret;

Re: [PATCH 2/2] venus: core: vote for video-mem icc path and change avg, peak bw

[mansur]

On 2020-09-09 00:08, Stephen Boyd wrote:

Quoting Mansur Alisha Shaik (2020-09-07 20:44:38)

Currently we are voting for venus0-ebi path during buffer processing
with an average bandwidth of all the instances and unvoting during
session release.

While video streaming when we try to do XO-SD using the command
"echo mem > /sys/power/state command" , device is not entering
to suspend state and from interconnect summary seeing votes for 
venus0-ebi


Corrected this by voting for venus0-ebi path in venus_runtime_resume


venus_runtime_resume() please so we know it's a function.

Changed to function like notation and RESEND v2
https://lore.kernel.org/patchwork/project/lkml/list/?series=463283



and unvote during venus_runtime_suspend.


venus_runtime_suspend().



Signed-off-by: Mansur Alisha Shaik 
---


Any Fixes: tag?

Added the Fixes tag



 drivers/media/platform/qcom/venus/core.c | 10 +-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/qcom/venus/core.c 
b/drivers/media/platform/qcom/venus/core.c

index 4857bbd..79d8600 100644
--- a/drivers/media/platform/qcom/venus/core.c
+++ b/drivers/media/platform/qcom/venus/core.c
@@ -373,6 +373,10 @@ static __maybe_unused int 
venus_runtime_suspend(struct device *dev)

if (ret)
return ret;

+   ret = icc_set_bw(core->video_path, 0, 0);
+   if (ret)
+   return ret;
+
return ret;
 }

@@ -382,7 +386,11 @@ static __maybe_unused int 
venus_runtime_resume(struct device *dev)

const struct venus_pm_ops *pm_ops = core->pm_ops;
int ret;

-   ret = icc_set_bw(core->cpucfg_path, 0, kbps_to_icc(1000));
+   ret = icc_set_bw(core->video_path, kbps_to_icc(2), 0);


This also shows that the arguments to icc_set_bw() we're backwards? 
Can

that be fixed in another patch instead of putting it in this one?

posted as another patch set.
https://lore.kernel.org/patchwork/project/lkml/list/?series=463283

[RESEND v2 3/4] venus: core: vote with average bandwidth and peak bandwidth as zero

[Mansur Alisha Shaik]

As per bandwidth table we are voting with average bandwidth
for "video-mem" and "cpu-cfg" paths as peak bandwidth is zero
in bandwidth table.

Signed-off-by: Mansur Alisha Shaik 
---
 drivers/media/platform/qcom/venus/core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/core.c 
b/drivers/media/platform/qcom/venus/core.c
index 064b6c8..c9669ad 100644
--- a/drivers/media/platform/qcom/venus/core.c
+++ b/drivers/media/platform/qcom/venus/core.c
@@ -382,11 +382,11 @@ static __maybe_unused int venus_runtime_resume(struct 
device *dev)
const struct venus_pm_ops *pm_ops = core->pm_ops;
int ret;
 
-   ret = icc_set_bw(core->video_path, 0, kbps_to_icc(1000));
+   ret = icc_set_bw(core->video_path, kbps_to_icc(2), 0);
if (ret)
return ret;
 
-   ret = icc_set_bw(core->cpucfg_path, 0, kbps_to_icc(1000));
+   ret = icc_set_bw(core->cpucfg_path, kbps_to_icc(1000), 0);
if (ret)
return ret;
 
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member 
of Code Aurora Forum, hosted by The Linux Foundation

[RESEND v2 0/4] Venus - change clk enable, disable order and change bw values

[Mansur Alisha Shaik]

The intention of this patchset is to correct clock enable and disable
order and vote for venus-ebi and cpucfg paths with average bandwidth
instad of peak bandwidth since with current implementation we are seeing
clock related warning during XO-SD and suspend device while video playback
---
Resending as all patches not updated properly because
of some mailing issues

Mansur Alisha Shaik (4):
  venus: core: change clk enable and disable order in resume and suspend
  venus: core: vote for video-mem path
  venus: core: vote with average bandwidth and peak bandwidth as zero
  venus: put dummy vote on video-mem path after last session release

 drivers/media/platform/qcom/venus/core.c   | 29 +++---
 drivers/media/platform/qcom/venus/pm_helpers.c |  3 +++
 2 files changed, 25 insertions(+), 7 deletions(-)

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member 
of Code Aurora Forum, hosted by The Linux Foundation

[RESEND v2 1/4] venus: core: change clk enable and disable order in resume and suspend

[Mansur Alisha Shaik]

Currently video driver is voting after clk enable and un voting
before clk disable. Basically we should vote before clk enable
and un vote after clk disable.

Corrected this by changing the order of clk enable and clk disable.

Fixes: 7482a983d ("media: venus: redesign clocks and pm domains control")
Signed-off-by: Mansur Alisha Shaik 
---
 drivers/media/platform/qcom/venus/core.c | 17 ++---
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/core.c 
b/drivers/media/platform/qcom/venus/core.c
index 6103aaf..52a3886 100644
--- a/drivers/media/platform/qcom/venus/core.c
+++ b/drivers/media/platform/qcom/venus/core.c
@@ -355,13 +355,16 @@ static __maybe_unused int venus_runtime_suspend(struct 
device *dev)
if (ret)
return ret;
 
+   if (pm_ops->core_power) {
+   ret = pm_ops->core_power(dev, POWER_OFF);
+   if (ret)
+   return ret;
+   }
+
ret = icc_set_bw(core->cpucfg_path, 0, 0);
if (ret)
return ret;
 
-   if (pm_ops->core_power)
-   ret = pm_ops->core_power(dev, POWER_OFF);
-
return ret;
 }
 
@@ -371,16 +374,16 @@ static __maybe_unused int venus_runtime_resume(struct 
device *dev)
const struct venus_pm_ops *pm_ops = core->pm_ops;
int ret;
 
+   ret = icc_set_bw(core->cpucfg_path, 0, kbps_to_icc(1000));
+   if (ret)
+   return ret;
+
if (pm_ops->core_power) {
ret = pm_ops->core_power(dev, POWER_ON);
if (ret)
return ret;
}
 
-   ret = icc_set_bw(core->cpucfg_path, 0, kbps_to_icc(1000));
-   if (ret)
-   return ret;
-
return hfi_core_resume(core, false);
 }
 
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member 
of Code Aurora Forum, hosted by The Linux Foundation

[RESEND v2 4/4] venus: put dummy vote on video-mem path after last session release

[Mansur Alisha Shaik]

As per current implementation, we are unvoting "videom-mem" path
for last video session during vdec_session_release().
While video playback when we try to suspend device, we see video clock
warnings since votes are already removed during vdec_session_release().

corrected this by putting dummy vote on "video-mem" after last video
session release and unvoting it during suspend.

Signed-off-by: Mansur Alisha Shaik 
---
 drivers/media/platform/qcom/venus/pm_helpers.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c 
b/drivers/media/platform/qcom/venus/pm_helpers.c
index 57877ea..c0a3524 100644
--- a/drivers/media/platform/qcom/venus/pm_helpers.c
+++ b/drivers/media/platform/qcom/venus/pm_helpers.c
@@ -212,6 +212,9 @@ static int load_scale_bw(struct venus_core *core)
}
mutex_unlock(>lock);
 
+   if (!total_avg && !total_peak)
+   total_avg = kbps_to_icc(1000);
+
dev_dbg(core->dev, VDBGL "total: avg_bw: %u, peak_bw: %u\n",
total_avg, total_peak);
 
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member 
of Code Aurora Forum, hosted by The Linux Foundation

[RESEND v2 2/4] venus: core: vote for video-mem path

[Mansur Alisha Shaik]

Currently we are voting for venus0-ebi path during buffer processing
with an average bandwidth of all the instances and unvoting during
session release.

While video streaming when we try to do XO-SD using the command
"echo mem > /sys/power/state command" , device is not entering
to suspend state and from interconnect summary seeing votes for venus0-ebi

Corrected this by voting for venus0-ebi path in venus_runtime_resume()
and unvote during venus_runtime_suspend().

Fixes: 7482a983d ("media: venus: redesign clocks and pm domains control")
Signed-off-by: Mansur Alisha Shaik 
---
Resending by adding () for functions to know it as function.

 drivers/media/platform/qcom/venus/core.c | 14 +-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/qcom/venus/core.c 
b/drivers/media/platform/qcom/venus/core.c
index 52a3886..064b6c8 100644
--- a/drivers/media/platform/qcom/venus/core.c
+++ b/drivers/media/platform/qcom/venus/core.c
@@ -363,8 +363,16 @@ static __maybe_unused int venus_runtime_suspend(struct 
device *dev)
 
ret = icc_set_bw(core->cpucfg_path, 0, 0);
if (ret)
-   return ret;
+   goto err_poweron_core;
+
+   ret = icc_set_bw(core->video_path, 0, 0);
+   if (ret)
+   goto err_poweron_core;
+
+   return ret;
 
+err_poweron_core:
+   pm_ops->core_power(dev, POWER_ON);
return ret;
 }
 
@@ -374,6 +382,10 @@ static __maybe_unused int venus_runtime_resume(struct 
device *dev)
const struct venus_pm_ops *pm_ops = core->pm_ops;
int ret;
 
+   ret = icc_set_bw(core->video_path, 0, kbps_to_icc(1000));
+   if (ret)
+   return ret;
+
ret = icc_set_bw(core->cpucfg_path, 0, kbps_to_icc(1000));
if (ret)
return ret;
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member 
of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH 1/2] venus: core: change clk enable and disable order in resume and suspend

[mansur]

On 2020-09-09 00:05, Stephen Boyd wrote:

Quoting Mansur Alisha Shaik (2020-09-07 20:44:05)

Currently video driver is voting after clk enable and un voting
before clk disable. Basically we should vote before clk enable
and un vote after clk disable.

Corrected this by changing the order of clk enable and clk disable.

Signed-off-by: Mansur Alisha Shaik 
---


Any Fixes: tag?

Added Fixes tag



 drivers/media/platform/qcom/venus/core.c | 17 ++---
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/core.c 
b/drivers/media/platform/qcom/venus/core.c

index c5af428..4857bbd 100644
--- a/drivers/media/platform/qcom/venus/core.c
+++ b/drivers/media/platform/qcom/venus/core.c
@@ -363,13 +363,16 @@ static __maybe_unused int 
venus_runtime_suspend(struct device *dev)

if (ret)
return ret;

+   if (pm_ops->core_power) {
+   ret = pm_ops->core_power(dev, POWER_OFF);
+   if (ret)
+   return ret;
+   }
+
ret = icc_set_bw(core->cpucfg_path, 0, 0);
if (ret)
return ret;


Shouldn't we power it back up if this fails during suspend?

On icc_set_bw() failure, we are just power it and return.
Addressed these comments and posted new version
https://lore.kernel.org/patchwork/project/lkml/list/?series=463224




-   if (pm_ops->core_power)
-   ret = pm_ops->core_power(dev, POWER_OFF);
-
return ret;
 }

[PATCH RESEND v2 0/4] Venus - change clk enable, disable order and change bw values

[Mansur Alisha Shaik]

The intention of this patchset is to correct clock enable and disable
order and vote for venus-ebi and cpucfg paths with average bandwidth
instad of peak bandwidth since with current implementation we are seeing
clock related warning during XO-SD and suspend device while video playback

---
Resending as all patches not updated properly because
of some mailing issues.

Mansur Alisha Shaik (4):
  venus: core: change clk enable and disable order in resume and suspend
  venus: core: vote for video-mem path
  venus: core: vote with average bandwidth and peak bandwidth as zero
  venus: put dummy vote on video-mem path after last session release

 drivers/media/platform/qcom/venus/core.c   | 29 +++---
 drivers/media/platform/qcom/venus/pm_helpers.c |  3 +++
 2 files changed, 25 insertions(+), 7 deletions(-)

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member 
of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v2 1/4] venus: core: change clk enable and disable order in resume and suspend

[Mansur Alisha Shaik]

Currently video driver is voting after clk enable and un voting
before clk disable. Basically we should vote before clk enable
and un vote after clk disable.

Corrected this by changing the order of clk enable and clk disable.

Fixes: 7482a983d ("media: venus: redesign clocks and pm domains control")
Signed-off-by: Mansur Alisha Shaik 
---
Changes in V2:
- Added fixes tag

 drivers/media/platform/qcom/venus/core.c | 17 ++---
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/core.c 
b/drivers/media/platform/qcom/venus/core.c
index 6103aaf..52a3886 100644
--- a/drivers/media/platform/qcom/venus/core.c
+++ b/drivers/media/platform/qcom/venus/core.c
@@ -355,13 +355,16 @@ static __maybe_unused int venus_runtime_suspend(struct 
device *dev)
if (ret)
return ret;
 
+   if (pm_ops->core_power) {
+   ret = pm_ops->core_power(dev, POWER_OFF);
+   if (ret)
+   return ret;
+   }
+
ret = icc_set_bw(core->cpucfg_path, 0, 0);
if (ret)
return ret;
 
-   if (pm_ops->core_power)
-   ret = pm_ops->core_power(dev, POWER_OFF);
-
return ret;
 }
 
@@ -371,16 +374,16 @@ static __maybe_unused int venus_runtime_resume(struct 
device *dev)
const struct venus_pm_ops *pm_ops = core->pm_ops;
int ret;
 
+   ret = icc_set_bw(core->cpucfg_path, 0, kbps_to_icc(1000));
+   if (ret)
+   return ret;
+
if (pm_ops->core_power) {
ret = pm_ops->core_power(dev, POWER_ON);
if (ret)
return ret;
}
 
-   ret = icc_set_bw(core->cpucfg_path, 0, kbps_to_icc(1000));
-   if (ret)
-   return ret;
-
return hfi_core_resume(core, false);
 }
 
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member 
of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v2 2/4] venus: core: vote for video-mem path

[Mansur Alisha Shaik]

Currently we are voting for venus0-ebi path during buffer processing
with an average bandwidth of all the instances and unvoting during
session release.

While video streaming when we try to do XO-SD using the command
"echo mem > /sys/power/state command" , device is not entering
to suspend state and from interconnect summary seeing votes for venus0-ebi

Corrected this by voting for venus0-ebi path in venus_runtime_resume
and unvote during venus_runtime_suspend.

Fixes: 7482a983d ("media: venus: redesign clocks and pm domains control")
Signed-off-by: Mansur Alisha Shaik 
---
Changes in V2:
- Added fixes tag
- Addressed review comments by Stephen Boyd
 drivers/media/platform/qcom/venus/core.c | 14 +-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/qcom/venus/core.c 
b/drivers/media/platform/qcom/venus/core.c
index 52a3886..064b6c8 100644
--- a/drivers/media/platform/qcom/venus/core.c
+++ b/drivers/media/platform/qcom/venus/core.c
@@ -363,8 +363,16 @@ static __maybe_unused int venus_runtime_suspend(struct 
device *dev)
 
ret = icc_set_bw(core->cpucfg_path, 0, 0);
if (ret)
-   return ret;
+   goto err_poweron_core;
+
+   ret = icc_set_bw(core->video_path, 0, 0);
+   if (ret)
+   goto err_poweron_core;
+
+   return ret;
 
+err_poweron_core:
+   pm_ops->core_power(dev, POWER_ON);
return ret;
 }
 
@@ -374,6 +382,10 @@ static __maybe_unused int venus_runtime_resume(struct 
device *dev)
const struct venus_pm_ops *pm_ops = core->pm_ops;
int ret;
 
+   ret = icc_set_bw(core->video_path, 0, kbps_to_icc(1000));
+   if (ret)
+   return ret;
+
ret = icc_set_bw(core->cpucfg_path, 0, kbps_to_icc(1000));
if (ret)
return ret;
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member 
of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v2 3/4] venus: core: vote with average bandwidth and peak bandwidth as zero

[Mansur Alisha Shaik]

As per bandwidth table we are voting with average bandwidth
for "video-mem" and "cpu-cfg" paths as peak bandwidth is zero
in bandwidth table.

Signed-off-by: Mansur Alisha Shaik 
---
 drivers/media/platform/qcom/venus/core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/core.c 
b/drivers/media/platform/qcom/venus/core.c
index 064b6c8..c9669ad 100644
--- a/drivers/media/platform/qcom/venus/core.c
+++ b/drivers/media/platform/qcom/venus/core.c
@@ -382,11 +382,11 @@ static __maybe_unused int venus_runtime_resume(struct 
device *dev)
const struct venus_pm_ops *pm_ops = core->pm_ops;
int ret;
 
-   ret = icc_set_bw(core->video_path, 0, kbps_to_icc(1000));
+   ret = icc_set_bw(core->video_path, kbps_to_icc(2), 0);
if (ret)
return ret;
 
-   ret = icc_set_bw(core->cpucfg_path, 0, kbps_to_icc(1000));
+   ret = icc_set_bw(core->cpucfg_path, kbps_to_icc(1000), 0);
if (ret)
return ret;
 
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member 
of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v2 4/4] venus: put dummy vote on video-mem path after last session release

[Mansur Alisha Shaik]

As per current implementation, we are unvoting "videom-mem" path
for last video session during vdec_session_release().
While video playback when we try to suspend device, we see video clock
warnings since votes are already removed during vdec_session_release().

corrected this by putting dummy vote on "video-mem" after last video
session release and unvoting it during suspend.

Signed-off-by: Mansur Alisha Shaik 
---
 drivers/media/platform/qcom/venus/pm_helpers.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c 
b/drivers/media/platform/qcom/venus/pm_helpers.c
index 57877ea..c0a3524 100644
--- a/drivers/media/platform/qcom/venus/pm_helpers.c
+++ b/drivers/media/platform/qcom/venus/pm_helpers.c
@@ -212,6 +212,9 @@ static int load_scale_bw(struct venus_core *core)
}
mutex_unlock(>lock);
 
+   if (!total_avg && !total_peak)
+   total_avg = kbps_to_icc(1000);
+
dev_dbg(core->dev, VDBGL "total: avg_bw: %u, peak_bw: %u\n",
total_avg, total_peak);
 
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member 
of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v2 2/4] venus: core: vote for video-mem path

[Mansur Alisha Shaik]

Currently we are voting for venus0-ebi path during buffer processing
with an average bandwidth of all the instances and unvoting during
session release.

While video streaming when we try to do XO-SD using the command
"echo mem > /sys/power/state command" , device is not entering
to suspend state and from interconnect summary seeing votes for venus0-ebi

Corrected this by voting for venus0-ebi path in venus_runtime_resume
and unvote during venus_runtime_suspend.

Fixes: 7482a983d ("media: venus: redesign clocks and pm domains control")
Signed-off-by: Mansur Alisha Shaik 
---
Changes in V2:
- Added fixes tag
- Addressed review comments by Stephen Boyd
 drivers/media/platform/qcom/venus/core.c | 14 +-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/qcom/venus/core.c 
b/drivers/media/platform/qcom/venus/core.c
index 52a3886..064b6c8 100644
--- a/drivers/media/platform/qcom/venus/core.c
+++ b/drivers/media/platform/qcom/venus/core.c
@@ -363,8 +363,16 @@ static __maybe_unused int venus_runtime_suspend(struct 
device *dev)
 
ret = icc_set_bw(core->cpucfg_path, 0, 0);
if (ret)
-   return ret;
+   goto err_poweron_core;
+
+   ret = icc_set_bw(core->video_path, 0, 0);
+   if (ret)
+   goto err_poweron_core;
+
+   return ret;
 
+err_poweron_core:
+   pm_ops->core_power(dev, POWER_ON);
return ret;
 }
 
@@ -374,6 +382,10 @@ static __maybe_unused int venus_runtime_resume(struct 
device *dev)
const struct venus_pm_ops *pm_ops = core->pm_ops;
int ret;
 
+   ret = icc_set_bw(core->video_path, 0, kbps_to_icc(1000));
+   if (ret)
+   return ret;
+
ret = icc_set_bw(core->cpucfg_path, 0, kbps_to_icc(1000));
if (ret)
return ret;
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member 
of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v2 3/4] venus: core: vote with average bandwidth and peak bandwidth as zero

[Mansur Alisha Shaik]

As per bandwidth table we are voting with average bandwidth
for "video-mem" and "cpu-cfg" paths as peak bandwidth is zero
in bandwidth table.

Signed-off-by: Mansur Alisha Shaik 
---
 drivers/media/platform/qcom/venus/core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/core.c 
b/drivers/media/platform/qcom/venus/core.c
index 064b6c8..c9669ad 100644
--- a/drivers/media/platform/qcom/venus/core.c
+++ b/drivers/media/platform/qcom/venus/core.c
@@ -382,11 +382,11 @@ static __maybe_unused int venus_runtime_resume(struct 
device *dev)
const struct venus_pm_ops *pm_ops = core->pm_ops;
int ret;
 
-   ret = icc_set_bw(core->video_path, 0, kbps_to_icc(1000));
+   ret = icc_set_bw(core->video_path, kbps_to_icc(2), 0);
if (ret)
return ret;
 
-   ret = icc_set_bw(core->cpucfg_path, 0, kbps_to_icc(1000));
+   ret = icc_set_bw(core->cpucfg_path, kbps_to_icc(1000), 0);
if (ret)
return ret;
 
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member 
of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v2 1/4] venus: core: change clk enable and disable order in resume and suspend

[Mansur Alisha Shaik]

Currently video driver is voting after clk enable and un voting
before clk disable. Basically we should vote before clk enable
and un vote after clk disable.

Corrected this by changing the order of clk enable and clk disable.

Fixes: 7482a983d ("media: venus: redesign clocks and pm domains control")
Signed-off-by: Mansur Alisha Shaik 
---
Changes in V2:
- Added fixes tag

 drivers/media/platform/qcom/venus/core.c | 17 ++---
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/core.c 
b/drivers/media/platform/qcom/venus/core.c
index 6103aaf..52a3886 100644
--- a/drivers/media/platform/qcom/venus/core.c
+++ b/drivers/media/platform/qcom/venus/core.c
@@ -355,13 +355,16 @@ static __maybe_unused int venus_runtime_suspend(struct 
device *dev)
if (ret)
return ret;
 
+   if (pm_ops->core_power) {
+   ret = pm_ops->core_power(dev, POWER_OFF);
+   if (ret)
+   return ret;
+   }
+
ret = icc_set_bw(core->cpucfg_path, 0, 0);
if (ret)
return ret;
 
-   if (pm_ops->core_power)
-   ret = pm_ops->core_power(dev, POWER_OFF);
-
return ret;
 }
 
@@ -371,16 +374,16 @@ static __maybe_unused int venus_runtime_resume(struct 
device *dev)
const struct venus_pm_ops *pm_ops = core->pm_ops;
int ret;
 
+   ret = icc_set_bw(core->cpucfg_path, 0, kbps_to_icc(1000));
+   if (ret)
+   return ret;
+
if (pm_ops->core_power) {
ret = pm_ops->core_power(dev, POWER_ON);
if (ret)
return ret;
}
 
-   ret = icc_set_bw(core->cpucfg_path, 0, kbps_to_icc(1000));
-   if (ret)
-   return ret;
-
return hfi_core_resume(core, false);
 }
 
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member 
of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v2 0/4] Venus - change clk enable, disable order and change bw values

[Mansur Alisha Shaik]

The intention of this patchset is to correct clock enable and disable
order and vote for venus-ebi and cpucfg paths with average bandwidth
instad of peak bandwidth since with current implementation we are seeing
clock related warning during XO-SD and suspend device while video playback

Mansur Alisha Shaik (4):
  venus: core: change clk enable and disable order in resume and suspend
  venus: core: vote for video-mem path
  venus: core: vote with average bandwidth and peak bandwidth as zero
  venus: put dummy vote on video-mem path after last session release

 drivers/media/platform/qcom/venus/core.c   | 29 +++---
 drivers/media/platform/qcom/venus/pm_helpers.c |  3 +++
 2 files changed, 25 insertions(+), 7 deletions(-)

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member 
of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v4 3/3] venus: core: add shutdown callback for venus

[Mansur Alisha Shaik]

After the SMMU translation is disabled in the
arm-smmu shutdown callback during reboot, if
any subsystem are still alive then IOVAs they
are using will become PAs on bus, which may
lead to crash.

So implemented shutdown callback, which detach iommu maps.

Signed-off-by: Mansur Alisha Shaik 
Acked-by: Stanimir Varbanov 
---
Changes in V4:
- In venus_core_shutdown(), instead of venurs_remove() calling 
  venus_shutdown() and venus_firmware_deinit().
- With venus_remove() call during shutdown we are facing few issue 
  like ui freez this is because in venus_remove, hfi_core_deinit()
  will wait until all core instances count become zero dudring 
  wait_var_event().

Changes in V3:
- Fix build errors

 drivers/media/platform/qcom/venus/core.c | 9 +
 1 file changed, 9 insertions(+)

diff --git a/drivers/media/platform/qcom/venus/core.c 
b/drivers/media/platform/qcom/venus/core.c
index 6103aaf..65b71ac 100644
--- a/drivers/media/platform/qcom/venus/core.c
+++ b/drivers/media/platform/qcom/venus/core.c
@@ -345,6 +345,14 @@ static int venus_remove(struct platform_device *pdev)
return ret;
 }
 
+static void venus_core_shutdown(struct platform_device *pdev)
+{
+   struct venus_core *core = platform_get_drvdata(pdev);
+
+   venus_shutdown(core);
+   venus_firmware_deinit(core);
+}
+
 static __maybe_unused int venus_runtime_suspend(struct device *dev)
 {
struct venus_core *core = dev_get_drvdata(dev);
@@ -602,6 +610,7 @@ static struct platform_driver qcom_venus_driver = {
.of_match_table = venus_dt_match,
.pm = _pm_ops,
},
+   .shutdown = venus_core_shutdown,
 };
 module_platform_driver(qcom_venus_driver);
 
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member 
of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v4 1/3] venus: core: handle race condititon for core ops

[Mansur Alisha Shaik]

For core ops we are having only write protect but there
is no read protect, because of this in multthreading
and concurrency, one CPU core is reading without wait
which is causing the NULL pointer dereferece crash.

one such scenario is as show below, where in one CPU
core, core->ops becoming NULL and in another CPU core
calling core->ops->session_init().

CPU: core-7:
Call trace:
 hfi_session_init+0x180/0x1dc [venus_core]
 vdec_queue_setup+0x9c/0x364 [venus_dec]
 vb2_core_reqbufs+0x1e4/0x368 [videobuf2_common]
 vb2_reqbufs+0x4c/0x64 [videobuf2_v4l2]
 v4l2_m2m_reqbufs+0x50/0x84 [v4l2_mem2mem]
 v4l2_m2m_ioctl_reqbufs+0x2c/0x38 [v4l2_mem2mem]
 v4l_reqbufs+0x4c/0x5c
__video_do_ioctl+0x2b0/0x39c

CPU: core-0:
Call trace:
 venus_shutdown+0x98/0xfc [venus_core]
 venus_sys_error_handler+0x64/0x148 [venus_core]
 process_one_work+0x210/0x3d0
 worker_thread+0x248/0x3f4
 kthread+0x11c/0x12c

Signed-off-by: Mansur Alisha Shaik 
Acked-by: Stanimir Varbanov 
---
Changes in V4:
- Addressed review comments by Stan in patch series 
  https://lore.kernel.org/patchwork/patch/1303678/
  and combining this change along with shutdown callback
  as we are facing race conditions with shutdown callback

 drivers/media/platform/qcom/venus/hfi.c | 17 -
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/qcom/venus/hfi.c 
b/drivers/media/platform/qcom/venus/hfi.c
index a59022a..58d4c06 100644
--- a/drivers/media/platform/qcom/venus/hfi.c
+++ b/drivers/media/platform/qcom/venus/hfi.c
@@ -195,19 +195,34 @@ EXPORT_SYMBOL_GPL(hfi_session_create);
 int hfi_session_init(struct venus_inst *inst, u32 pixfmt)
 {
struct venus_core *core = inst->core;
-   const struct hfi_ops *ops = core->ops;
+   const struct hfi_ops *ops;
int ret;
 
+   /*
+* If core shutdown is in progress or if we are in system
+* recovery, return an error as during system error recovery
+* session_init() can't pass successfully
+*/
+   mutex_lock(>lock);
+   if (!core->ops || core->sys_error) {
+   mutex_unlock(>lock);
+   return -EIO;
+   }
+   mutex_unlock(>lock);
+
if (inst->state != INST_UNINIT)
return -EINVAL;
 
inst->hfi_codec = to_codec_type(pixfmt);
reinit_completion(>done);
 
+   mutex_lock(>lock);
+   ops = core->ops;
ret = ops->session_init(inst, inst->session_type, inst->hfi_codec);
if (ret)
return ret;
 
+   mutex_unlock(>lock);
ret = wait_session_msg(inst);
if (ret)
return ret;
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member 
of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v4 2/3] venus: handle use after free for iommu_map/iommu_unmap

[Mansur Alisha Shaik]

In concurrency usecase and reboot scenario we are seeing muliple
crashes related to iommu_map/iommu_unamp of core->fw.iommu_domain.

In one case we are seeing "Unable to handle kernel NULL pointer
dereference at virtual address 0008" crash, this is
because of core->fw.iommu_domain in venus_firmware_deinit() and
trying to map in venus_boot() during venus_sys_error_handler()

Call trace:
 __iommu_map+0x4c/0x348
 iommu_map+0x5c/0x70
 venus_boot+0x184/0x230 [venus_core]
 venus_sys_error_handler+0xa0/0x14c [venus_core]
 process_one_work+0x210/0x3d0
 worker_thread+0x248/0x3f4
 kthread+0x11c/0x12c
 ret_from_fork+0x10/0x18

In second case we are seeing "Unable to handle kernel paging request
at virtual address 006b6b6b6b6b6b9b" crash, this is because of
unmappin iommu domain which is already unmapped.

Call trace:
 venus_remove+0xf8/0x108 [venus_core]
 venus_core_shutdown+0x1c/0x34 [venus_core]
 platform_drv_shutdown+0x28/0x34
 device_shutdown+0x154/0x1fc
 kernel_restart_prepare+0x40/0x4c
 kernel_restart+0x1c/0x64
 __arm64_sys_reboot+0x190/0x238
 el0_svc_common+0xa4/0x154
 el0_svc_compat_handler+0x2c/0x38
 el0_svc_compat+0x8/0x10

Signed-off-by: Mansur Alisha Shaik 
---
 drivers/media/platform/qcom/venus/firmware.c | 17 +
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/firmware.c 
b/drivers/media/platform/qcom/venus/firmware.c
index 1db64a8..d03e2dd 100644
--- a/drivers/media/platform/qcom/venus/firmware.c
+++ b/drivers/media/platform/qcom/venus/firmware.c
@@ -171,9 +171,14 @@ static int venus_shutdown_no_tz(struct venus_core *core)
 
iommu = core->fw.iommu_domain;
 
-   unmapped = iommu_unmap(iommu, VENUS_FW_START_ADDR, mapped);
-   if (unmapped != mapped)
-   dev_err(dev, "failed to unmap firmware\n");
+   if (core->fw.mapped_mem_size && iommu) {
+   unmapped = iommu_unmap(iommu, VENUS_FW_START_ADDR, mapped);
+
+   if (unmapped != mapped)
+   dev_err(dev, "failed to unmap firmware\n");
+   else
+   core->fw.mapped_mem_size = 0;
+   }
 
return 0;
 }
@@ -305,7 +310,11 @@ void venus_firmware_deinit(struct venus_core *core)
iommu = core->fw.iommu_domain;
 
iommu_detach_device(iommu, core->fw.dev);
-   iommu_domain_free(iommu);
+
+   if (core->fw.iommu_domain) {
+   iommu_domain_free(iommu);
+   core->fw.iommu_domain = NULL;
+   }
 
platform_device_unregister(to_platform_device(core->fw.dev));
 }
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member 
of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v4 0/3] venus: core: add shutdown callback for venus

[Mansur Alisha Shaik]

Add shutdown callback for venus driver.
Handle race conditions in concurrency usecases like
multiple browser YouTube browser tabs(approx 50 tabs)
graphics_Stress, WiFi ON/OFF, Bluetooth ON/OF,
and reboot in parallel.

Mansur Alisha Shaik (3):
  venus: core: handle race condititon for core ops
  venus: handle use after free for iommu_map/iommu_unmap
  venus: core: add shutdown callback for venus

 drivers/media/platform/qcom/venus/core.c |  9 +
 drivers/media/platform/qcom/venus/firmware.c | 17 +
 drivers/media/platform/qcom/venus/hfi.c  | 17 -
 3 files changed, 38 insertions(+), 5 deletions(-)

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member 
of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v2 0/3] Venus - Handle race conditions in concurrency

[Mansur Alisha Shaik]

The intention of this patchset is to handle race
conditions during concurrency usecases like
Multiple YouTube browser tabs(approx 50 plus tabs),
graphics_Stress, WiFi ON/OFF, Bluetooth ON/OF,
and reboot in parallel.

Mansur Alisha Shaik (3):
  venus: core: handle race condititon for core ops
  venus: core: cancel pending work items in workqueue
  venus: handle use after free for iommu_map/iommu_unmap

 drivers/media/platform/qcom/venus/core.c |  4 
 drivers/media/platform/qcom/venus/firmware.c | 17 +
 drivers/media/platform/qcom/venus/hfi.c  |  5 -
 3 files changed, 21 insertions(+), 5 deletions(-)

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member 
of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v2 3/3] venus: handle use after free for iommu_map/iommu_unmap

[Mansur Alisha Shaik]

In concurrency usecase and reboot scenario we are seeing muliple
crashes related to iommu_map/iommu_unamp of core->fw.iommu_domain.

In one case we are seeing "Unable to handle kernel NULL pointer
dereference at virtual address 0008" crash, this is 
because of core->fw.iommu_domain in venus_firmware_deinit() and 
trying to map in venus_boot() during venus_sys_error_handler()

Call trace:
 __iommu_map+0x4c/0x348
 iommu_map+0x5c/0x70
 venus_boot+0x184/0x230 [venus_core]
 venus_sys_error_handler+0xa0/0x14c [venus_core]
 process_one_work+0x210/0x3d0
 worker_thread+0x248/0x3f4
 kthread+0x11c/0x12c
 ret_from_fork+0x10/0x18

In second case we are seeing "Unable to handle kernel paging request
at virtual address 006b6b6b6b6b6b9b" crash, this is because of
unmappin iommu domain which is already unmapped.

Call trace:
 venus_remove+0xf8/0x108 [venus_core]
 venus_core_shutdown+0x1c/0x34 [venus_core]
 platform_drv_shutdown+0x28/0x34
 device_shutdown+0x154/0x1fc
 kernel_restart_prepare+0x40/0x4c
 kernel_restart+0x1c/0x64
 __arm64_sys_reboot+0x190/0x238
 el0_svc_common+0xa4/0x154
 el0_svc_compat_handler+0x2c/0x38
 el0_svc_compat+0x8/0x10


Signed-off-by: Mansur Alisha Shaik 
---
Changes in V2:
- Addressed review comments by stan
- Elaborated commit message

 drivers/media/platform/qcom/venus/firmware.c | 17 +
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/firmware.c 
b/drivers/media/platform/qcom/venus/firmware.c
index 8801a6a..c427e88 100644
--- a/drivers/media/platform/qcom/venus/firmware.c
+++ b/drivers/media/platform/qcom/venus/firmware.c
@@ -171,9 +171,14 @@ static int venus_shutdown_no_tz(struct venus_core *core)
 
iommu = core->fw.iommu_domain;
 
-   unmapped = iommu_unmap(iommu, VENUS_FW_START_ADDR, mapped);
-   if (unmapped != mapped)
-   dev_err(dev, "failed to unmap firmware\n");
+   if (core->fw.mapped_mem_size && iommu) {
+   unmapped = iommu_unmap(iommu, VENUS_FW_START_ADDR, mapped);
+
+   if (unmapped != mapped)
+   dev_err(dev, "failed to unmap firmware\n");
+   else
+   core->fw.mapped_mem_size = 0;
+   }
 
return 0;
 }
@@ -288,7 +293,11 @@ void venus_firmware_deinit(struct venus_core *core)
iommu = core->fw.iommu_domain;
 
iommu_detach_device(iommu, core->fw.dev);
-   iommu_domain_free(iommu);
+
+   if (core->fw.iommu_domain) {
+   iommu_domain_free(iommu);
+   core->fw.iommu_domain = NULL;
+   }
 
platform_device_unregister(to_platform_device(core->fw.dev));
 }
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member 
of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v2 2/3] venus: core: cancel pending work items in workqueue

[Mansur Alisha Shaik]

In concurrency usecase and reboot scenario we are
observing race condition and seeing NULL pointer
dereference crash. In shutdown path and system
recovery path we are destroying the same mutex
hence seeing crash.

This case is handled by mutex protection and
cancel delayed work items in work queue.

Below is the call trace for the crash
Call trace:
 venus_remove+0xdc/0xec [venus_core]
 venus_core_shutdown+0x1c/0x34 [venus_core]
 platform_drv_shutdown+0x28/0x34
 device_shutdown+0x154/0x1fc
 kernel_restart_prepare+0x40/0x4c
 kernel_restart+0x1c/0x64

Call trace:
 mutex_lock+0x34/0x60
 venus_hfi_destroy+0x28/0x98 [venus_core]
 hfi_destroy+0x1c/0x28 [venus_core]
 venus_sys_error_handler+0x60/0x14c [venus_core]
 process_one_work+0x210/0x3d0
 worker_thread+0x248/0x3f4
 kthread+0x11c/0x12c
 ret_from_fork+0x10/0x18

Signed-off-by: Mansur Alisha Shaik 
---
 drivers/media/platform/qcom/venus/core.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/drivers/media/platform/qcom/venus/core.c 
b/drivers/media/platform/qcom/venus/core.c
index c5af428..69aa199 100644
--- a/drivers/media/platform/qcom/venus/core.c
+++ b/drivers/media/platform/qcom/venus/core.c
@@ -323,6 +323,8 @@ static int venus_remove(struct platform_device *pdev)
struct device *dev = core->dev;
int ret;
 
+   cancel_delayed_work_sync(>work);
+
ret = pm_runtime_get_sync(dev);
WARN_ON(ret < 0);
 
@@ -340,7 +342,9 @@ static int venus_remove(struct platform_device *pdev)
if (pm_ops->core_put)
pm_ops->core_put(dev);
 
+   mutex_lock(>lock);
hfi_destroy(core);
+   mutex_unlock(>lock);
 
icc_put(core->video_path);
icc_put(core->cpucfg_path);
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member 
of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v2 1/3] venus: core: handle race condititon for core ops

[Mansur Alisha Shaik]

For core ops we are having only write protect but there
is no read protect, because of this in multthreading
and concurrency, one CPU core is reading without wait
which is causing the NULL pointer dereferece crash.

one such scenario is as show below, where in one CPU
core, core->ops becoming NULL and in another CPU core
calling core->ops->session_init().

CPU: core-7:
Call trace:
 hfi_session_init+0x180/0x1dc [venus_core]
 vdec_queue_setup+0x9c/0x364 [venus_dec]
 vb2_core_reqbufs+0x1e4/0x368 [videobuf2_common]
 vb2_reqbufs+0x4c/0x64 [videobuf2_v4l2]
 v4l2_m2m_reqbufs+0x50/0x84 [v4l2_mem2mem]
 v4l2_m2m_ioctl_reqbufs+0x2c/0x38 [v4l2_mem2mem]
 v4l_reqbufs+0x4c/0x5c
__video_do_ioctl+0x2b0/0x39c

CPU: core-0:
Call trace:
 venus_shutdown+0x98/0xfc [venus_core]
 venus_sys_error_handler+0x64/0x148 [venus_core]
 process_one_work+0x210/0x3d0
 worker_thread+0x248/0x3f4
 kthread+0x11c/0x12c

Signed-off-by: Mansur Alisha Shaik 
Acked-by: Stanimir Varbanov 
---
Changes in V2:
- Addressed review comments by stan by validating on top
- of https://lore.kernel.org/patchwork/project/lkml/list/?series=455962

 drivers/media/platform/qcom/venus/hfi.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/qcom/venus/hfi.c 
b/drivers/media/platform/qcom/venus/hfi.c
index a59022a..3137071 100644
--- a/drivers/media/platform/qcom/venus/hfi.c
+++ b/drivers/media/platform/qcom/venus/hfi.c
@@ -195,7 +195,7 @@ EXPORT_SYMBOL_GPL(hfi_session_create);
 int hfi_session_init(struct venus_inst *inst, u32 pixfmt)
 {
struct venus_core *core = inst->core;
-   const struct hfi_ops *ops = core->ops;
+   const struct hfi_ops *ops;
int ret;
 
if (inst->state != INST_UNINIT)
@@ -204,10 +204,13 @@ int hfi_session_init(struct venus_inst *inst, u32 pixfmt)
inst->hfi_codec = to_codec_type(pixfmt);
reinit_completion(>done);
 
+   mutex_lock(>lock);
+   ops = core->ops;
ret = ops->session_init(inst, inst->session_type, inst->hfi_codec);
if (ret)
return ret;
 
+   mutex_unlock(>lock);
ret = wait_session_msg(inst);
if (ret)
return ret;
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member 
of Code Aurora Forum, hosted by The Linux Foundation

[PATCH 2/2] venus: core: vote for video-mem icc path and change avg, peak bw

[Mansur Alisha Shaik]

Currently we are voting for venus0-ebi path during buffer processing
with an average bandwidth of all the instances and unvoting during
session release.

While video streaming when we try to do XO-SD using the command
"echo mem > /sys/power/state command" , device is not entering
to suspend state and from interconnect summary seeing votes for venus0-ebi

Corrected this by voting for venus0-ebi path in venus_runtime_resume
and unvote during venus_runtime_suspend.

Signed-off-by: Mansur Alisha Shaik 
---
 drivers/media/platform/qcom/venus/core.c | 10 +-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/qcom/venus/core.c 
b/drivers/media/platform/qcom/venus/core.c
index 4857bbd..79d8600 100644
--- a/drivers/media/platform/qcom/venus/core.c
+++ b/drivers/media/platform/qcom/venus/core.c
@@ -373,6 +373,10 @@ static __maybe_unused int venus_runtime_suspend(struct 
device *dev)
if (ret)
return ret;
 
+   ret = icc_set_bw(core->video_path, 0, 0);
+   if (ret)
+   return ret;
+
return ret;
 }
 
@@ -382,7 +386,11 @@ static __maybe_unused int venus_runtime_resume(struct 
device *dev)
const struct venus_pm_ops *pm_ops = core->pm_ops;
int ret;
 
-   ret = icc_set_bw(core->cpucfg_path, 0, kbps_to_icc(1000));
+   ret = icc_set_bw(core->video_path, kbps_to_icc(2), 0);
+   if (ret)
+   return ret;
+
+   ret = icc_set_bw(core->cpucfg_path, kbps_to_icc(1000), 0);
if (ret)
return ret;
 
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member 
of Code Aurora Forum, hosted by The Linux Foundation

[PATCH 1/2] venus: core: change clk enable and disable order in resume and suspend

[Mansur Alisha Shaik]

Currently video driver is voting after clk enable and un voting
before clk disable. Basically we should vote before clk enable
and un vote after clk disable.

Corrected this by changing the order of clk enable and clk disable.

Signed-off-by: Mansur Alisha Shaik 
---
 drivers/media/platform/qcom/venus/core.c | 17 ++---
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/core.c 
b/drivers/media/platform/qcom/venus/core.c
index c5af428..4857bbd 100644
--- a/drivers/media/platform/qcom/venus/core.c
+++ b/drivers/media/platform/qcom/venus/core.c
@@ -363,13 +363,16 @@ static __maybe_unused int venus_runtime_suspend(struct 
device *dev)
if (ret)
return ret;
 
+   if (pm_ops->core_power) {
+   ret = pm_ops->core_power(dev, POWER_OFF);
+   if (ret)
+   return ret;
+   }
+
ret = icc_set_bw(core->cpucfg_path, 0, 0);
if (ret)
return ret;
 
-   if (pm_ops->core_power)
-   ret = pm_ops->core_power(dev, POWER_OFF);
-
return ret;
 }
 
@@ -379,16 +382,16 @@ static __maybe_unused int venus_runtime_resume(struct 
device *dev)
const struct venus_pm_ops *pm_ops = core->pm_ops;
int ret;
 
+   ret = icc_set_bw(core->cpucfg_path, 0, kbps_to_icc(1000));
+   if (ret)
+   return ret;
+
if (pm_ops->core_power) {
ret = pm_ops->core_power(dev, POWER_ON);
if (ret)
return ret;
}
 
-   ret = icc_set_bw(core->cpucfg_path, 0, kbps_to_icc(1000));
-   if (ret)
-   return ret;
-
return hfi_core_resume(core, false);
 }
 
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member 
of Code Aurora Forum, hosted by The Linux Foundation

[PATCH 0/2] Venus - change clk enable, disable order and change bw values

[Mansur Alisha Shaik]

The intention of this patchset is to correct clock enable and disable
order and vote for venus-ebi and cpucfg paths with average bandwidht
instad of peakbandwidht since with current implementation we are seeing
"video_cc_venus_ctl_axi_clk status stuck at 'off' " warnings and XO-SD
failures while streaming.

Mansur Alisha Shaik (2):
  venus: core: change clk enable and disable order in resume and suspend
  venus: core: vote for video-mem icc path and change avg, peak bw

 drivers/media/platform/qcom/venus/core.c | 23 +--
 1 file changed, 17 insertions(+), 6 deletions(-)

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member 
of Code Aurora Forum, hosted by The Linux Foundation

Re: [RESEND 1/3] venus: core: handle race condititon for core ops

[mansur]

On 2020-08-21 16:29, Stanimir Varbanov wrote:

Hi Mansur,

On 8/10/20 12:50 PM, Stanimir Varbanov wrote:

Hi Mansur,

Thanks for the patches!

On 8/7/20 9:24 AM, Mansur Alisha Shaik wrote:

For core ops we are having only write protect but there
is no read protect, because of this in multthreading
and concurrency, one CPU core is reading without wait
which is causing the NULL pointer dereferece crash.

one such scenario is as show below, where in one
core core->ops becoming NULL and in another core
calling core->ops->session_init().

CPU: core-7:
Call trace:
 hfi_session_init+0x180/0x1dc [venus_core]
 vdec_queue_setup+0x9c/0x364 [venus_dec]
 vb2_core_reqbufs+0x1e4/0x368 [videobuf2_common]
 vb2_reqbufs+0x4c/0x64 [videobuf2_v4l2]
 v4l2_m2m_reqbufs+0x50/0x84 [v4l2_mem2mem]
 v4l2_m2m_ioctl_reqbufs+0x2c/0x38 [v4l2_mem2mem]
 v4l_reqbufs+0x4c/0x5c
__video_do_ioctl+0x2b0/0x39c

CPU: core-0:
Call trace:
 venus_shutdown+0x98/0xfc [venus_core]
 venus_sys_error_handler+0x64/0x148 [venus_core]
 process_one_work+0x210/0x3d0
 worker_thread+0x248/0x3f4
 kthread+0x11c/0x12c

Signed-off-by: Mansur Alisha Shaik 
---
 drivers/media/platform/qcom/venus/core.c | 2 +-
 drivers/media/platform/qcom/venus/hfi.c  | 5 -
 2 files changed, 5 insertions(+), 2 deletions(-)


See below comment, otherwise:

Acked-by: Stanimir Varbanov 



diff --git a/drivers/media/platform/qcom/venus/core.c 
b/drivers/media/platform/qcom/venus/core.c

index 203c653..fe99c83 100644
--- a/drivers/media/platform/qcom/venus/core.c
+++ b/drivers/media/platform/qcom/venus/core.c
@@ -64,8 +64,8 @@ static void venus_sys_error_handler(struct 
work_struct *work)

pm_runtime_get_sync(core->dev);

hfi_core_deinit(core, true);
-   hfi_destroy(core);
mutex_lock(>lock);
+   hfi_destroy(core);


As my recovery fixes [1] touches this part also, could you please 
apply

them on top of yours and re-test?


I'll drop above chunk from the patch because it is already taken into
account in my recovery fixes series and queue up the patch for v5.10.

yes, you can drop. I have validated these patches on top of your 
recovery patch
series. I will push V2 with dependency on "venus - recovery from 
frimware crash"
series 
(https://lore.kernel.org/patchwork/project/lkml/list/?series=455962)




Otherwise this patch looks good to me.

[1] https://www.spinics.net/lists/linux-arm-msm/msg70092.html


venus_shutdown(core);

pm_runtime_put_sync(core->dev);
diff --git a/drivers/media/platform/qcom/venus/hfi.c 
b/drivers/media/platform/qcom/venus/hfi.c

index a211eb9..2eeb31f 100644
--- a/drivers/media/platform/qcom/venus/hfi.c
+++ b/drivers/media/platform/qcom/venus/hfi.c
@@ -195,7 +195,7 @@ EXPORT_SYMBOL_GPL(hfi_session_create);
 int hfi_session_init(struct venus_inst *inst, u32 pixfmt)
 {
struct venus_core *core = inst->core;
-   const struct hfi_ops *ops = core->ops;
+   const struct hfi_ops *ops;
int ret;

if (inst->state != INST_UNINIT)
@@ -204,10 +204,13 @@ int hfi_session_init(struct venus_inst *inst, 
u32 pixfmt)

inst->hfi_codec = to_codec_type(pixfmt);
reinit_completion(>done);

+   mutex_lock(>lock);
+   ops = core->ops;
ret = ops->session_init(inst, inst->session_type, inst->hfi_codec);
if (ret)
return ret;

+   mutex_unlock(>lock);
ret = wait_session_msg(inst);
if (ret)
return ret;

[RESEND 0/3] Venus - Handle race conditions in concurrency

[Mansur Alisha Shaik]

The intention of this patchset is to handle race
conditions during concurrency usecases like
Multiple YouTube browser tabs(approx 50 plus tabs),
graphics_Stress, WiFi ON/OFF, Bluetooth ON/OF,
and reboot in parallel.

---
Resending the fixes by describing more about the issue
and correcting typo errors.

Mansur Alisha Shaik (3):
  venus: core: handle race condititon for core ops
  venus: core: cancel pending work items in workqueue
  venus: handle use after free for iommu_map/iommu_unmap

 drivers/media/platform/qcom/venus/core.c |  6 +-
 drivers/media/platform/qcom/venus/firmware.c | 17 +
 drivers/media/platform/qcom/venus/hfi.c  |  5 -
 3 files changed, 22 insertions(+), 6 deletions(-)

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member 
of Code Aurora Forum, hosted by The Linux Foundation

[RESEND 2/3] venus: core: cancel pending work items in workqueue

[Mansur Alisha Shaik]

In concurrency usecase and reboot scenario we are
observing race condition and seeing NULL pointer
dereference crash. In shutdown path and system
recovery path we are destroying the same mutex
hence seeing crash.

This case is handled by mutex protection and
cancel delayed work items in work queue.

Below is the call trace for the crash
Call trace:
 venus_remove+0xdc/0xec [venus_core]
 venus_core_shutdown+0x1c/0x34 [venus_core]
 platform_drv_shutdown+0x28/0x34
 device_shutdown+0x154/0x1fc
 kernel_restart_prepare+0x40/0x4c
 kernel_restart+0x1c/0x64

Call trace:
 mutex_lock+0x34/0x60
 venus_hfi_destroy+0x28/0x98 [venus_core]
 hfi_destroy+0x1c/0x28 [venus_core]
 venus_sys_error_handler+0x60/0x14c [venus_core]
 process_one_work+0x210/0x3d0
 worker_thread+0x248/0x3f4
 kthread+0x11c/0x12c
 ret_from_fork+0x10/0x18

Signed-off-by: Mansur Alisha Shaik 
---
 drivers/media/platform/qcom/venus/core.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/drivers/media/platform/qcom/venus/core.c 
b/drivers/media/platform/qcom/venus/core.c
index fe99c83..41a293e 100644
--- a/drivers/media/platform/qcom/venus/core.c
+++ b/drivers/media/platform/qcom/venus/core.c
@@ -312,6 +312,8 @@ static int venus_remove(struct platform_device *pdev)
struct device *dev = core->dev;
int ret;
 
+   cancel_delayed_work_sync(>work);
+
ret = pm_runtime_get_sync(dev);
WARN_ON(ret < 0);
 
@@ -329,7 +331,9 @@ static int venus_remove(struct platform_device *pdev)
if (pm_ops->core_put)
pm_ops->core_put(dev);
 
+   mutex_lock(>lock);
hfi_destroy(core);
+   mutex_unlock(>lock);
 
icc_put(core->video_path);
icc_put(core->cpucfg_path);
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member 
of Code Aurora Forum, hosted by The Linux Foundation

[RESEND 3/3] venus: handle use after free for iommu_map/iommu_unmap

[Mansur Alisha Shaik]

In concurrency usecase and reboot scenario we are trying
to map fw.iommu_domain which is already unmapped during
shutdown. This is causing NULL pointer dereference crash.

This case is handled by adding necessary checks.

Call trace:
 __iommu_map+0x4c/0x348
 iommu_map+0x5c/0x70
 venus_boot+0x184/0x230 [venus_core]
 venus_sys_error_handler+0xa0/0x14c [venus_core]
 process_one_work+0x210/0x3d0
 worker_thread+0x248/0x3f4
 kthread+0x11c/0x12c
 ret_from_fork+0x10/0x18

Signed-off-by: Mansur Alisha Shaik 
---
 drivers/media/platform/qcom/venus/firmware.c | 17 +
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/firmware.c 
b/drivers/media/platform/qcom/venus/firmware.c
index 8801a6a..c427e88 100644
--- a/drivers/media/platform/qcom/venus/firmware.c
+++ b/drivers/media/platform/qcom/venus/firmware.c
@@ -171,9 +171,14 @@ static int venus_shutdown_no_tz(struct venus_core *core)
 
iommu = core->fw.iommu_domain;
 
-   unmapped = iommu_unmap(iommu, VENUS_FW_START_ADDR, mapped);
-   if (unmapped != mapped)
-   dev_err(dev, "failed to unmap firmware\n");
+   if (core->fw.mapped_mem_size && iommu) {
+   unmapped = iommu_unmap(iommu, VENUS_FW_START_ADDR, mapped);
+
+   if (unmapped != mapped)
+   dev_err(dev, "failed to unmap firmware\n");
+   else
+   core->fw.mapped_mem_size = 0;
+   }
 
return 0;
 }
@@ -288,7 +293,11 @@ void venus_firmware_deinit(struct venus_core *core)
iommu = core->fw.iommu_domain;
 
iommu_detach_device(iommu, core->fw.dev);
-   iommu_domain_free(iommu);
+
+   if (core->fw.iommu_domain) {
+   iommu_domain_free(iommu);
+   core->fw.iommu_domain = NULL;
+   }
 
platform_device_unregister(to_platform_device(core->fw.dev));
 }
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member 
of Code Aurora Forum, hosted by The Linux Foundation

[RESEND 1/3] venus: core: handle race condititon for core ops

[Mansur Alisha Shaik]

For core ops we are having only write protect but there
is no read protect, because of this in multthreading
and concurrency, one CPU core is reading without wait
which is causing the NULL pointer dereferece crash.

one such scenario is as show below, where in one
core core->ops becoming NULL and in another core
calling core->ops->session_init().

CPU: core-7:
Call trace:
 hfi_session_init+0x180/0x1dc [venus_core]
 vdec_queue_setup+0x9c/0x364 [venus_dec]
 vb2_core_reqbufs+0x1e4/0x368 [videobuf2_common]
 vb2_reqbufs+0x4c/0x64 [videobuf2_v4l2]
 v4l2_m2m_reqbufs+0x50/0x84 [v4l2_mem2mem]
 v4l2_m2m_ioctl_reqbufs+0x2c/0x38 [v4l2_mem2mem]
 v4l_reqbufs+0x4c/0x5c
__video_do_ioctl+0x2b0/0x39c

CPU: core-0:
Call trace:
 venus_shutdown+0x98/0xfc [venus_core]
 venus_sys_error_handler+0x64/0x148 [venus_core]
 process_one_work+0x210/0x3d0
 worker_thread+0x248/0x3f4
 kthread+0x11c/0x12c

Signed-off-by: Mansur Alisha Shaik 
---
 drivers/media/platform/qcom/venus/core.c | 2 +-
 drivers/media/platform/qcom/venus/hfi.c  | 5 -
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/core.c 
b/drivers/media/platform/qcom/venus/core.c
index 203c653..fe99c83 100644
--- a/drivers/media/platform/qcom/venus/core.c
+++ b/drivers/media/platform/qcom/venus/core.c
@@ -64,8 +64,8 @@ static void venus_sys_error_handler(struct work_struct *work)
pm_runtime_get_sync(core->dev);
 
hfi_core_deinit(core, true);
-   hfi_destroy(core);
mutex_lock(>lock);
+   hfi_destroy(core);
venus_shutdown(core);
 
pm_runtime_put_sync(core->dev);
diff --git a/drivers/media/platform/qcom/venus/hfi.c 
b/drivers/media/platform/qcom/venus/hfi.c
index a211eb9..2eeb31f 100644
--- a/drivers/media/platform/qcom/venus/hfi.c
+++ b/drivers/media/platform/qcom/venus/hfi.c
@@ -195,7 +195,7 @@ EXPORT_SYMBOL_GPL(hfi_session_create);
 int hfi_session_init(struct venus_inst *inst, u32 pixfmt)
 {
struct venus_core *core = inst->core;
-   const struct hfi_ops *ops = core->ops;
+   const struct hfi_ops *ops;
int ret;
 
if (inst->state != INST_UNINIT)
@@ -204,10 +204,13 @@ int hfi_session_init(struct venus_inst *inst, u32 pixfmt)
inst->hfi_codec = to_codec_type(pixfmt);
reinit_completion(>done);
 
+   mutex_lock(>lock);
+   ops = core->ops;
ret = ops->session_init(inst, inst->session_type, inst->hfi_codec);
if (ret)
return ret;
 
+   mutex_unlock(>lock);
ret = wait_session_msg(inst);
if (ret)
return ret;
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member 
of Code Aurora Forum, hosted by The Linux Foundation

[PATCH 0/3] Venus - Handle race conditions in concurrency

[Mansur Alisha Shaik]

The intenstion of this patchset is to handle race
conditions during concurrency usecase.

Mansur Alisha Shaik (3):
  venus: core: handle race condititon for core ops
  venus: core: cancel pending work items in workqueue
  venus: handle use after free for iommu_map/iommu_unmap

 drivers/media/platform/qcom/venus/core.c |  6 +-
 drivers/media/platform/qcom/venus/firmware.c | 17 +
 drivers/media/platform/qcom/venus/hfi.c  |  5 -
 3 files changed, 22 insertions(+), 6 deletions(-)

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member 
of Code Aurora Forum, hosted by The Linux Foundation

[PATCH 2/3] venus: core: cancel pending work items in workqueue

[Mansur Alisha Shaik]

In concurrency usecase and reboot scenario we are
observing race condition and seeing NULL pointer
dereference crash.when workqueue is in progress
to handle system recovery and at same when device
happends, in bothe paths we are trying to destryo
the same mutex hence seeing the crash.

So this is hadled by protecting hfi_destroy in
remove by mutext variable and cancel a delayed
work items before reboot.

Below is the call trace for the crash
Call trace:
 venus_remove+0xdc/0xec [venus_core]
 venus_core_shutdown+0x1c/0x34 [venus_core]
 platform_drv_shutdown+0x28/0x34
 device_shutdown+0x154/0x1fc
 kernel_restart_prepare+0x40/0x4c
 kernel_restart+0x1c/0x64

Call trace:
 mutex_lock+0x34/0x60
 venus_hfi_destroy+0x28/0x98 [venus_core]
 hfi_destroy+0x1c/0x28 [venus_core]
 venus_sys_error_handler+0x60/0x14c [venus_core]
 process_one_work+0x210/0x3d0
 worker_thread+0x248/0x3f4
 kthread+0x11c/0x12c
 ret_from_fork+0x10/0x18

Signed-off-by: Mansur Alisha Shaik 
---
 drivers/media/platform/qcom/venus/core.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/drivers/media/platform/qcom/venus/core.c 
b/drivers/media/platform/qcom/venus/core.c
index fe99c83..41a293e 100644
--- a/drivers/media/platform/qcom/venus/core.c
+++ b/drivers/media/platform/qcom/venus/core.c
@@ -312,6 +312,8 @@ static int venus_remove(struct platform_device *pdev)
struct device *dev = core->dev;
int ret;
 
+   cancel_delayed_work_sync(>work);
+
ret = pm_runtime_get_sync(dev);
WARN_ON(ret < 0);
 
@@ -329,7 +331,9 @@ static int venus_remove(struct platform_device *pdev)
if (pm_ops->core_put)
pm_ops->core_put(dev);
 
+   mutex_lock(>lock);
hfi_destroy(core);
+   mutex_unlock(>lock);
 
icc_put(core->video_path);
icc_put(core->cpucfg_path);
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member 
of Code Aurora Forum, hosted by The Linux Foundation

[PATCH 3/3] venus: handle use after free for iommu_map/iommu_unmap

[Mansur Alisha Shaik]

In concurrency usecase and reboot scenario we are trying
to map fw.iommu_domain which is already unmapped during
shutdown. This is causing NULL pointer dereference crash.

This case is handled by necesassary check before unmappin.

Call trace:
 __iommu_map+0x4c/0x348
 iommu_map+0x5c/0x70
 venus_boot+0x184/0x230 [venus_core]
 venus_sys_error_handler+0xa0/0x14c [venus_core]
 process_one_work+0x210/0x3d0
 worker_thread+0x248/0x3f4
 kthread+0x11c/0x12c
 ret_from_fork+0x10/0x18

Signed-off-by: Mansur Alisha Shaik 
---
 drivers/media/platform/qcom/venus/firmware.c | 17 +
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/firmware.c 
b/drivers/media/platform/qcom/venus/firmware.c
index 8801a6a..d8cfa16 100644
--- a/drivers/media/platform/qcom/venus/firmware.c
+++ b/drivers/media/platform/qcom/venus/firmware.c
@@ -171,9 +171,14 @@ static int venus_shutdown_no_tz(struct venus_core *core)
 
iommu = core->fw.iommu_domain;
 
-   unmapped = iommu_unmap(iommu, VENUS_FW_START_ADDR, mapped);
-   if (unmapped != mapped)
-   dev_err(dev, "failed to unmap firmware\n");
+   if (core->fw.mapped_mem_size && iommu) {
+   unmapped = iommu_unmap(iommu, VENUS_FW_START_ADDR, mapped);
+
+   if (unmapped != mapped)
+   dev_err(dev, "failed to unmap firmware\n");
+   else
+   core->fw.mapped_mem_size = 0;
+   }
 
return 0;
 }
@@ -288,7 +293,11 @@ void venus_firmware_deinit(struct venus_core *core)
iommu = core->fw.iommu_domain;
 
iommu_detach_device(iommu, core->fw.dev);
-   iommu_domain_free(iommu);
+
+   if (core->fw.iommu_domain) {
+   iommu_domain_free(iommu);
+   core->fw.iommu_domain = NULL;
+   }
 
platform_device_unregister(to_platform_device(core->fw.dev));
 }
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member 
of Code Aurora Forum, hosted by The Linux Foundation

[PATCH 1/3] venus: core: handle race condititon for core ops

[Mansur Alisha Shaik]

For core ops we are having only write protect but
there is no read protect, because of this in mult
-threading and concurrency, one CPU core is readi
-ing without waiting which is causing the NULL
pointer dereferece crash.

one such scenario is as show below, where in one
core core->ops becoming NULL and in another core
calling core->ops->session_init().

CPU: 7(core):
Call trace:
 hfi_session_init+0x180/0x1dc [venus_core]
 vdec_queue_setup+0x9c/0x364 [venus_dec]
 vb2_core_reqbufs+0x1e4/0x368 [videobuf2_common]
 vb2_reqbufs+0x4c/0x64 [videobuf2_v4l2]
 v4l2_m2m_reqbufs+0x50/0x84 [v4l2_mem2mem]
 v4l2_m2m_ioctl_reqbufs+0x2c/0x38 [v4l2_mem2mem]
 v4l_reqbufs+0x4c/0x5c
__video_do_ioctl+0x2b0/0x39c

CPU: 0(core):
Call trace:
 venus_shutdown+0x98/0xfc [venus_core]
 venus_sys_error_handler+0x64/0x148 [venus_core]
 process_one_work+0x210/0x3d0
 worker_thread+0x248/0x3f4
 kthread+0x11c/0x12c

Signed-off-by: Mansur Alisha Shaik 
---
 drivers/media/platform/qcom/venus/core.c | 2 +-
 drivers/media/platform/qcom/venus/hfi.c  | 5 -
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/core.c 
b/drivers/media/platform/qcom/venus/core.c
index 203c653..fe99c83 100644
--- a/drivers/media/platform/qcom/venus/core.c
+++ b/drivers/media/platform/qcom/venus/core.c
@@ -64,8 +64,8 @@ static void venus_sys_error_handler(struct work_struct *work)
pm_runtime_get_sync(core->dev);
 
hfi_core_deinit(core, true);
-   hfi_destroy(core);
mutex_lock(>lock);
+   hfi_destroy(core);
venus_shutdown(core);
 
pm_runtime_put_sync(core->dev);
diff --git a/drivers/media/platform/qcom/venus/hfi.c 
b/drivers/media/platform/qcom/venus/hfi.c
index a211eb9..2eeb31f 100644
--- a/drivers/media/platform/qcom/venus/hfi.c
+++ b/drivers/media/platform/qcom/venus/hfi.c
@@ -195,7 +195,7 @@ EXPORT_SYMBOL_GPL(hfi_session_create);
 int hfi_session_init(struct venus_inst *inst, u32 pixfmt)
 {
struct venus_core *core = inst->core;
-   const struct hfi_ops *ops = core->ops;
+   const struct hfi_ops *ops;
int ret;
 
if (inst->state != INST_UNINIT)
@@ -204,10 +204,13 @@ int hfi_session_init(struct venus_inst *inst, u32 pixfmt)
inst->hfi_codec = to_codec_type(pixfmt);
reinit_completion(>done);
 
+   mutex_lock(>lock);
+   ops = core->ops;
ret = ops->session_init(inst, inst->session_type, inst->hfi_codec);
if (ret)
return ret;
 
+   mutex_unlock(>lock);
ret = wait_session_msg(inst);
if (ret)
return ret;
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member 
of Code Aurora Forum, hosted by The Linux Foundation

[PATCH V3] venus: core: add shutdown callback for venus

[Mansur Alisha Shaik]

After the SMMU translation is disabled in the
arm-smmu shutdown callback during reboot, if
any subsystem are still alive then IOVAs they
are using will become PAs on bus, which may
lead to crash.

Below are the consumers of smmu from venus
arm-smmu: consumer: aa0.video-codec supplier=1500.iommu
arm-smmu: consumer: video-firmware.0 supplier=1500.iommu

So implemented shutdown callback, which detach iommu maps.

Signed-off-by: Mansur Alisha Shaik 
---
Changes in V3:
- Fix build errors

 drivers/media/platform/qcom/venus/core.c | 11 +++
 1 file changed, 11 insertions(+)

diff --git a/drivers/media/platform/qcom/venus/core.c 
b/drivers/media/platform/qcom/venus/core.c
index 203c653..cfe211a 100644
--- a/drivers/media/platform/qcom/venus/core.c
+++ b/drivers/media/platform/qcom/venus/core.c
@@ -341,6 +341,16 @@ static int venus_remove(struct platform_device *pdev)
return ret;
 }
 
+static void venus_core_shutdown(struct platform_device *pdev)
+{
+   struct venus_core *core = platform_get_drvdata(pdev);
+   int ret;
+
+   ret = venus_remove(pdev);
+   if (ret)
+   dev_warn(core->dev, "shutdown failed %d\n", ret);
+}
+
 static __maybe_unused int venus_runtime_suspend(struct device *dev)
 {
struct venus_core *core = dev_get_drvdata(dev);
@@ -592,6 +602,7 @@ static struct platform_driver qcom_venus_driver = {
.of_match_table = venus_dt_match,
.pm = _pm_ops,
},
+   .shutdown = venus_core_shutdown,
 };
 module_platform_driver(qcom_venus_driver);
 
-- 
2.7.4

Re: [PATCH] venus: core: add shutdown callback for venus

[mansur]

Hi Sai,


On 2020-06-24 12:17, Sai Prakash Ranjan wrote:

Hi Mansur,

On 2020-06-13 16:03, Mansur Alisha Shaik wrote:

After the SMMU translation is disabled in the
arm-smmu shutdown callback during reboot, if
any subsystem are still alive then IOVAs they
are using will become PAs on bus, which may
lead to crash.

Below are the consumers of smmu from venus
arm-smmu: consumer: aa0.video-codec supplier=1500.iommu
arm-smmu: consumer: video-firmware.0 supplier=1500.iommu

So implemented shutdown callback, which detach iommu maps.

Change-Id: I0f0f331056e0b84b92f1d86f66618d4b1caaa24a
Signed-off-by: Mansur Alisha Shaik 
---
 drivers/media/platform/qcom/venus/core.c | 9 +
 1 file changed, 9 insertions(+)

diff --git a/drivers/media/platform/qcom/venus/core.c
b/drivers/media/platform/qcom/venus/core.c
index 30d4b9e..acf798c 100644
--- a/drivers/media/platform/qcom/venus/core.c
+++ b/drivers/media/platform/qcom/venus/core.c
@@ -371,6 +371,14 @@ static int venus_remove(struct platform_device 
*pdev)

return ret;
 }

+static void venus_core_shutdown(struct platform_device *pdev)
+{
+   int ret;
+
+   ret = venus_remove(pdev);
+   WARN_ON(ret < 0);


I don't think you should warn here, its shutdown path and you can't
do anything with this WARN unlike remove callback where you have
to be sure to cleanup properly so that you are able to reload module.
But if you still want a hint about this failure, then just add a 
dev_err()
to indicate the failure instead of a big stack trace spamming kernel 
log.




posted V2 version by adding dev_warn during shutdown failure instead of 
WARN_ON.

V2 version : https://lore.kernel.org/patchwork/patch/1284693/


Thanks,
Sai


---
Thanks,
Mansur

[PATCH V2] venus: core: add shutdown callback for venus

[Mansur Alisha Shaik]

After the SMMU translation is disabled in the
arm-smmu shutdown callback during reboot, if
any subsystem are still alive then IOVAs they
are using will become PAs on bus, which may
lead to crash.

Below are the consumers of smmu from venus
arm-smmu: consumer: aa0.video-codec supplier=1500.iommu
arm-smmu: consumer: video-firmware.0 supplier=1500.iommu

So implemented shutdown callback, which detach iommu maps.

Change-Id: I0f0f331056e0b84b92f1d86f66618d4b1caaa24a
Signed-off-by: Mansur Alisha Shaik 
---
 drivers/media/platform/qcom/venus/core.c | 11 +++
 1 file changed, 11 insertions(+)

diff --git a/drivers/media/platform/qcom/venus/core.c 
b/drivers/media/platform/qcom/venus/core.c
index 203c653..92aac06 100644
--- a/drivers/media/platform/qcom/venus/core.c
+++ b/drivers/media/platform/qcom/venus/core.c
@@ -341,6 +341,16 @@ static int venus_remove(struct platform_device *pdev)
return ret;
 }
 
+static void venus_core_shutdown(struct platform_device *pdev)
+{
+   struct venus_core *core = platform_get_drvdata(pdev);
+   int ret;
+
+   ret = venus_remove(pdev);
+   if(ret)
+   dev_warn(core->dev, "shutdown failed \n", ret);
+}
+
 static __maybe_unused int venus_runtime_suspend(struct device *dev)
 {
struct venus_core *core = dev_get_drvdata(dev);
@@ -592,6 +602,7 @@ static struct platform_driver qcom_venus_driver = {
.of_match_table = venus_dt_match,
.pm = _pm_ops,
},
+   .shutdown = venus_core_shutdown,
 };
 module_platform_driver(qcom_venus_driver);
 
-- 
2.7.4

Re: [PATCH] venus: core: add shutdown callback for venus

[mansur]

Hi Stan,


On 2020-06-13 17:43, Stanimir Varbanov wrote:

Hi Mansur,

Thanks for the patch!

How you test this? Is it enough to start playback and issue reboot 
(did

you test with reboot -f) ?
Yes, I have tested it with "reboot -f" and started video playback 
(YouTube browser and local video)

and issue reboot command.


On 6/13/20 1:33 PM, Mansur Alisha Shaik wrote:

After the SMMU translation is disabled in the
arm-smmu shutdown callback during reboot, if
any subsystem are still alive then IOVAs they
are using will become PAs on bus, which may
lead to crash.

Below are the consumers of smmu from venus
arm-smmu: consumer: aa0.video-codec supplier=1500.iommu
arm-smmu: consumer: video-firmware.0 supplier=1500.iommu

So implemented shutdown callback, which detach iommu maps.

Change-Id: I0f0f331056e0b84b92f1d86f66618d4b1caaa24a
Signed-off-by: Mansur Alisha Shaik 
---
 drivers/media/platform/qcom/venus/core.c | 9 +
 1 file changed, 9 insertions(+)

diff --git a/drivers/media/platform/qcom/venus/core.c 
b/drivers/media/platform/qcom/venus/core.c

index 30d4b9e..acf798c 100644
--- a/drivers/media/platform/qcom/venus/core.c
+++ b/drivers/media/platform/qcom/venus/core.c
@@ -371,6 +371,14 @@ static int venus_remove(struct platform_device 
*pdev)

return ret;
 }

+static void venus_core_shutdown(struct platform_device *pdev)
+{
+   int ret;
+
+   ret = venus_remove(pdev);
+   WARN_ON(ret < 0);
+}
+
 static __maybe_unused int venus_runtime_suspend(struct device *dev)
 {
struct venus_core *core = dev_get_drvdata(dev);
@@ -628,6 +636,7 @@ static struct platform_driver qcom_venus_driver = 
{

.of_match_table = venus_dt_match,
.pm = _pm_ops,
},
+   .shutdown = venus_core_shutdown,
 };
 module_platform_driver(qcom_venus_driver);



Thank,
Mansur

[PATCH] venus: core: add shutdown callback for venus

[Mansur Alisha Shaik]

After the SMMU translation is disabled in the
arm-smmu shutdown callback during reboot, if
any subsystem are still alive then IOVAs they
are using will become PAs on bus, which may
lead to crash.

Below are the consumers of smmu from venus
arm-smmu: consumer: aa0.video-codec supplier=1500.iommu
arm-smmu: consumer: video-firmware.0 supplier=1500.iommu

So implemented shutdown callback, which detach iommu maps.

Change-Id: I0f0f331056e0b84b92f1d86f66618d4b1caaa24a
Signed-off-by: Mansur Alisha Shaik 
---
 drivers/media/platform/qcom/venus/core.c | 9 +
 1 file changed, 9 insertions(+)

diff --git a/drivers/media/platform/qcom/venus/core.c 
b/drivers/media/platform/qcom/venus/core.c
index 30d4b9e..acf798c 100644
--- a/drivers/media/platform/qcom/venus/core.c
+++ b/drivers/media/platform/qcom/venus/core.c
@@ -371,6 +371,14 @@ static int venus_remove(struct platform_device *pdev)
return ret;
 }
 
+static void venus_core_shutdown(struct platform_device *pdev)
+{
+   int ret;
+
+   ret = venus_remove(pdev);
+   WARN_ON(ret < 0);
+}
+
 static __maybe_unused int venus_runtime_suspend(struct device *dev)
 {
struct venus_core *core = dev_get_drvdata(dev);
@@ -628,6 +636,7 @@ static struct platform_driver qcom_venus_driver = {
.of_match_table = venus_dt_match,
.pm = _pm_ops,
},
+   .shutdown = venus_core_shutdown,
 };
 module_platform_driver(qcom_venus_driver);
 
-- 
2.7.4

Re: [PATCH] venus: avoid extra locking in driver

[mansur]

On 2020-03-11 08:34, Alexandre Courbot wrote:
On Tue, Mar 10, 2020 at 7:07 AM Jeffrey Kardatzke 
 wrote:


On Thu, Mar 5, 2020 at 11:50 PM Alexandre Courbot 
 wrote:

>
> On Fri, Mar 6, 2020 at 2:34 PM Mansur Alisha Shaik
>  wrote:
> >
> > This change will avoid extra locking in driver.
>
> Could you elaborate a bit more on the problem that this patch solves?

For us it fixes a kernel null deref that happens when we run the
MultipleEncoders test (I've verified this to be true).

>
> >
> > Signed-off-by: Mansur Alisha Shaik 
> > ---
> >  drivers/media/platform/qcom/venus/core.c   |  2 +-
> >  drivers/media/platform/qcom/venus/core.h   |  2 +-
> >  drivers/media/platform/qcom/venus/helpers.c| 11 +--
> >  drivers/media/platform/qcom/venus/pm_helpers.c |  8 
> >  4 files changed, 15 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/media/platform/qcom/venus/core.c 
b/drivers/media/platform/qcom/venus/core.c
> > index 194b10b9..75d38b8 100644
> > --- a/drivers/media/platform/qcom/venus/core.c
> > +++ b/drivers/media/platform/qcom/venus/core.c
> > @@ -447,7 +447,7 @@ static const struct freq_tbl sdm845_freq_table[] = {
> > {  244800, 1 }, /* 1920x1080@30 */
> >  };
> >
> > -static struct codec_freq_data sdm845_codec_freq_data[] =  {
> > +static const struct codec_freq_data sdm845_codec_freq_data[] =  {
> > { V4L2_PIX_FMT_H264, VIDC_SESSION_TYPE_ENC, 675, 10 },
> > { V4L2_PIX_FMT_HEVC, VIDC_SESSION_TYPE_ENC, 675, 10 },
> > { V4L2_PIX_FMT_VP8, VIDC_SESSION_TYPE_ENC, 675, 10 },
> > diff --git a/drivers/media/platform/qcom/venus/core.h 
b/drivers/media/platform/qcom/venus/core.h
> > index ab7c360..8c8d0e9 100644
> > --- a/drivers/media/platform/qcom/venus/core.h
> > +++ b/drivers/media/platform/qcom/venus/core.h
> > @@ -245,7 +245,7 @@ struct venus_buffer {
> >  struct clock_data {
> > u32 core_id;
> > unsigned long freq;
> > -   const struct codec_freq_data *codec_freq_data;
> > +   struct codec_freq_data codec_freq_data;
> >  };
> >
> >  #define to_venus_buffer(ptr)   container_of(ptr, struct venus_buffer, vb)
> > diff --git a/drivers/media/platform/qcom/venus/helpers.c 
b/drivers/media/platform/qcom/venus/helpers.c
> > index bcc6038..550c4ff 100644
> > --- a/drivers/media/platform/qcom/venus/helpers.c
> > +++ b/drivers/media/platform/qcom/venus/helpers.c
> > @@ -807,6 +807,7 @@ int venus_helper_init_codec_freq_data(struct venus_inst 
*inst)
> > unsigned int i, data_size;
> > u32 pixfmt;
> > int ret = 0;
> > +   bool found = false;
> >
> > if (!IS_V4(inst->core))
> > return 0;
> > @@ -816,16 +817,22 @@ int venus_helper_init_codec_freq_data(struct 
venus_inst *inst)
> > pixfmt = inst->session_type == VIDC_SESSION_TYPE_DEC ?
> > inst->fmt_out->pixfmt : inst->fmt_cap->pixfmt;
> >
> > +   memset(>clk_data.codec_freq_data, 0,
> > +   sizeof(inst->clk_data.codec_freq_data));
> > +
> > for (i = 0; i < data_size; i++) {
> > if (data[i].pixfmt == pixfmt &&
> > data[i].session_type == inst->session_type) {
> > -   inst->clk_data.codec_freq_data = [i];
> > +   inst->clk_data.codec_freq_data = data[i];
>
> From the patch I'd infer that inst->clk_data.codec_freq_data needs to
> change at runtime. Is this what happens? Why? I'd expect that
> frequency tables remain constant, and thus that the global
> sdm845_codec_freq_data can remain constant while
> clock_data::codec_freq_data is a const reference to it. What prevents
> this from happening?
>
> > +   found = true;
> > break;
> > }
> > }
> >
> > -   if (!inst->clk_data.codec_freq_data)
> > +   if (!found) {
> > +   dev_err(inst->core->dev, "cannot find codec freq data\n");
> > ret = -EINVAL;
> > +   }
> >
> > return ret;
> >  }
> > diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c 
b/drivers/media/platform/qcom/venus/pm_helpers.c
> > index abf9315..240845e 100644
> > --- a/drivers/media/platform/qcom/venus/pm_helpers.c
> > +++ b/drivers/media/platform/qcom/venus/pm_helpers.c
> > @@ -496,7 +496,7 @@ min_loaded_core(struct venus_inst *inst, u32 
*min_coreid, u32 *min_load)
> > li

[PATCH V2] venus: fix multiple encoder crash

[Mansur Alisha Shaik]

Currently we are considering the instances which are available
in core->inst list for load calculation in min_loaded_core()
function, but this is incorrect because by the time we call
decide_core() for second instance, the third instance not
filled yet codec_freq_data pointer.

Solve this by considering the instances whose session has started.

Signed-off-by: Mansur Alisha Shaik 
---
Changes in V2:
- As per Alex and Jeffrey comments, elaborated problem
  and addressed review comments.

 drivers/media/platform/qcom/venus/pm_helpers.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c 
b/drivers/media/platform/qcom/venus/pm_helpers.c
index abf9315..531e7a4 100644
--- a/drivers/media/platform/qcom/venus/pm_helpers.c
+++ b/drivers/media/platform/qcom/venus/pm_helpers.c
@@ -496,6 +496,10 @@ min_loaded_core(struct venus_inst *inst, u32 *min_coreid, 
u32 *min_load)
list_for_each_entry(inst_pos, >instances, list) {
if (inst_pos == inst)
continue;
+
+   if (inst_pos->state != INST_START)
+   continue;
+
vpp_freq = inst_pos->clk_data.codec_freq_data->vpp_freq;
coreid = inst_pos->clk_data.core_id;
 
-- 
2.7.4

Re: [PATCH] powerpc: non-GPL export for eeh_dev_check_failure

[Vishal Mansur]

On 8/11/2014 8:46 AM, Benjamin Herrenschmidt wrote:
> On Tue, 2014-08-05 at 15:51 +0100, One Thousand Gnomes wrote:
>> On Tue, 05 Aug 2014 20:12:09 +0530
>> Vishal Mansur  wrote:
>>
>>> EEH kernel services are inconsistently exported by the 
>>> kernel. eeh_check_failure is exported for any use, but 
>>> eeh_dev_check_failure is exported only for GPL use. 
>>> While eeh_check_failure is implemented for a specific 
>>> purpose to be used by services such as readl, it is 
>>> not suited for a purpose where caller needs eeh status. 
>>> This functionality is provided by eeh_dev_check_failure.
>>>
>>> This patch relaxes the export for eeh_dev_check_failure
>>> to make it consistent with eeh_check_failure() and 
>>> usable by non-GPL modules.
>>
>> The GPL covers all derivative works. Tweaking this doesn't magically
>> allow you to use the feature in non GPL code. Your legal department can I
>> am sure explain in detail further.
> 
> This is an interesting case... I assume this has to do with a well known
> GPU manufacturer...
> 
> The PCI APIs are generally exported in such a way that a non-GPL driver
> can use them (regardless of whether one considers a non-GPL driver to be
> legal here or not, this is besides the point).
> 
> eeh_dev_check_failure() can be considered as powerpc specific extension
> of the PCI API for use by PCI drivers and as such, it *could* be
> construed that we should be consistent (and consistent with
> eeh_check_failure()) and expose it as an EXPORT_SYMBOL without the GPL
> suffix.
> 
> So I'm somewhat tempted to take this patch, but Vishal, the driver in
> question could, I suppose, as a workaround, use a readl to some scratch
> register of some description, no ?

Ben, calling readl to some scratch register will be same as calling 
eeh_check_failure, caller will not be able to get eeh status.

Hence for the purpose where caller wants to know eeh status during
a bad register read, it can not be worked around using readl.

Thanks,
Vishal

> 
> Cheers,
> Ben.
> 
>> Alan
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to majord...@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at  http://www.tux.org/lkml/
> 
> 


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] powerpc: non-GPL export for eeh_dev_check_failure

[Vishal Mansur]

On 8/11/2014 8:46 AM, Benjamin Herrenschmidt wrote:
 On Tue, 2014-08-05 at 15:51 +0100, One Thousand Gnomes wrote:
 On Tue, 05 Aug 2014 20:12:09 +0530
 Vishal Mansur vman...@linux.vnet.ibm.com wrote:

 EEH kernel services are inconsistently exported by the 
 kernel. eeh_check_failure is exported for any use, but 
 eeh_dev_check_failure is exported only for GPL use. 
 While eeh_check_failure is implemented for a specific 
 purpose to be used by services such as readl, it is 
 not suited for a purpose where caller needs eeh status. 
 This functionality is provided by eeh_dev_check_failure.

 This patch relaxes the export for eeh_dev_check_failure
 to make it consistent with eeh_check_failure() and 
 usable by non-GPL modules.

 The GPL covers all derivative works. Tweaking this doesn't magically
 allow you to use the feature in non GPL code. Your legal department can I
 am sure explain in detail further.
 
 This is an interesting case... I assume this has to do with a well known
 GPU manufacturer...
 
 The PCI APIs are generally exported in such a way that a non-GPL driver
 can use them (regardless of whether one considers a non-GPL driver to be
 legal here or not, this is besides the point).
 
 eeh_dev_check_failure() can be considered as powerpc specific extension
 of the PCI API for use by PCI drivers and as such, it *could* be
 construed that we should be consistent (and consistent with
 eeh_check_failure()) and expose it as an EXPORT_SYMBOL without the GPL
 suffix.
 
 So I'm somewhat tempted to take this patch, but Vishal, the driver in
 question could, I suppose, as a workaround, use a readl to some scratch
 register of some description, no ?

Ben, calling readl to some scratch register will be same as calling 
eeh_check_failure, caller will not be able to get eeh status.

Hence for the purpose where caller wants to know eeh status during
a bad register read, it can not be worked around using readl.

Thanks,
Vishal

 
 Cheers,
 Ben.
 
 Alan
 --
 To unsubscribe from this list: send the line unsubscribe linux-kernel in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
 Please read the FAQ at  http://www.tux.org/lkml/
 
 


--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] powerpc: non-GPL export for eeh_dev_check_failure

[Vishal Mansur]

EEH kernel services are inconsistently exported by the 
kernel. eeh_check_failure is exported for any use, but 
eeh_dev_check_failure is exported only for GPL use. 
While eeh_check_failure is implemented for a specific 
purpose to be used by services such as readl, it is 
not suited for a purpose where caller needs eeh status. 
This functionality is provided by eeh_dev_check_failure.

This patch relaxes the export for eeh_dev_check_failure
to make it consistent with eeh_check_failure() and 
usable by non-GPL modules.

Signed-off-by: Vishal Mansur 
---
 arch/powerpc/kernel/eeh.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c
index 86e2570..8689c30 100644
--- a/arch/powerpc/kernel/eeh.c
+++ b/arch/powerpc/kernel/eeh.c
@@ -496,8 +496,7 @@ dn_unlock:
eeh_serialize_unlock(flags);
return rc;
 }
-
-EXPORT_SYMBOL_GPL(eeh_dev_check_failure);
+EXPORT_SYMBOL(eeh_dev_check_failure);

 /**
  * eeh_check_failure - Check if all 1's data is due to EEH slot freeze
--
2.0.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] powerpc: non-GPL export for eeh_dev_check_failure

[Vishal Mansur]

EEH kernel services are inconsistently exported by the 
kernel. eeh_check_failure is exported for any use, but 
eeh_dev_check_failure is exported only for GPL use. 
While eeh_check_failure is implemented for a specific 
purpose to be used by services such as readl, it is 
not suited for a purpose where caller needs eeh status. 
This functionality is provided by eeh_dev_check_failure.

This patch relaxes the export for eeh_dev_check_failure
to make it consistent with eeh_check_failure() and 
usable by non-GPL modules.

Signed-off-by: Vishal Mansur 
---
 arch/powerpc/kernel/eeh.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c
index 86e2570..8689c30 100644
--- a/arch/powerpc/kernel/eeh.c
+++ b/arch/powerpc/kernel/eeh.c
@@ -496,8 +496,7 @@ dn_unlock:
eeh_serialize_unlock(flags);
return rc;
 }
-
-EXPORT_SYMBOL_GPL(eeh_dev_check_failure);
+EXPORT_SYMBOL(eeh_dev_check_failure);

 /**
  * eeh_check_failure - Check if all 1's data is due to EEH slot freeze
--
2.0.1


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] powerpc: non-GPL export for eeh_dev_check_failure

[Vishal Mansur]

EEH kernel services are inconsistently exported by the 
kernel. eeh_check_failure is exported for any use, but 
eeh_dev_check_failure is exported only for GPL use. 
While eeh_check_failure is implemented for a specific 
purpose to be used by services such as readl, it is 
not suited for a purpose where caller needs eeh status. 
This functionality is provided by eeh_dev_check_failure.

This patch relaxes the export for eeh_dev_check_failure
to make it consistent with eeh_check_failure() and 
usable by non-GPL modules.

Signed-off-by: Vishal Mansur vman...@linux.vnet.ibm.com
---
 arch/powerpc/kernel/eeh.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c
index 86e2570..8689c30 100644
--- a/arch/powerpc/kernel/eeh.c
+++ b/arch/powerpc/kernel/eeh.c
@@ -496,8 +496,7 @@ dn_unlock:
eeh_serialize_unlock(flags);
return rc;
 }
-
-EXPORT_SYMBOL_GPL(eeh_dev_check_failure);
+EXPORT_SYMBOL(eeh_dev_check_failure);

 /**
  * eeh_check_failure - Check if all 1's data is due to EEH slot freeze
--
2.0.1


--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] powerpc: non-GPL export for eeh_dev_check_failure

[Vishal Mansur]

EEH kernel services are inconsistently exported by the 
kernel. eeh_check_failure is exported for any use, but 
eeh_dev_check_failure is exported only for GPL use. 
While eeh_check_failure is implemented for a specific 
purpose to be used by services such as readl, it is 
not suited for a purpose where caller needs eeh status. 
This functionality is provided by eeh_dev_check_failure.

This patch relaxes the export for eeh_dev_check_failure
to make it consistent with eeh_check_failure() and 
usable by non-GPL modules.

Signed-off-by: Vishal Mansur vman...@linux.vnet.ibm.com
---
 arch/powerpc/kernel/eeh.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c
index 86e2570..8689c30 100644
--- a/arch/powerpc/kernel/eeh.c
+++ b/arch/powerpc/kernel/eeh.c
@@ -496,8 +496,7 @@ dn_unlock:
eeh_serialize_unlock(flags);
return rc;
 }
-
-EXPORT_SYMBOL_GPL(eeh_dev_check_failure);
+EXPORT_SYMBOL(eeh_dev_check_failure);

 /**
  * eeh_check_failure - Check if all 1's data is due to EEH slot freeze
--
2.0.1

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/