* [PATCH] venus: avoid extra locking in driver
@ 2020-03-06 5:32 Mansur Alisha Shaik
2020-03-06 7:50 ` Alexandre Courbot
2020-03-10 19:15 ` Jeffrey Kardatzke
0 siblings, 2 replies; 6+ messages in thread
From: Mansur Alisha Shaik @ 2020-03-06 5:32 UTC (permalink / raw)
To: linux-media, stanimir.varbanov
Cc: linux-kernel, linux-arm-msm, vgarodia, mansur
This change will avoid extra locking in driver.
Signed-off-by: Mansur Alisha Shaik <mansur@codeaurora.org>
---
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, 100000000 }, /* 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(&inst->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 = &data[i];
+ inst->clk_data.codec_freq_data = data[i];
+ 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)
list_for_each_entry(inst_pos, &core->instances, list) {
if (inst_pos == inst)
continue;
- vpp_freq = inst_pos->clk_data.codec_freq_data->vpp_freq;
+ vpp_freq = inst_pos->clk_data.codec_freq_data.vpp_freq;
coreid = inst_pos->clk_data.core_id;
mbs_per_sec = load_per_instance(inst_pos);
@@ -545,7 +545,7 @@ static int decide_core(struct venus_inst *inst)
return 0;
inst_load = load_per_instance(inst);
- inst_load *= inst->clk_data.codec_freq_data->vpp_freq;
+ inst_load *= inst->clk_data.codec_freq_data.vpp_freq;
max_freq = core->res->freq_tbl[0].freq;
min_loaded_core(inst, &min_coreid, &min_load);
@@ -848,10 +848,10 @@ static unsigned long calculate_inst_freq(struct venus_inst *inst,
mbs_per_sec = load_per_instance(inst) / fps;
- vpp_freq = mbs_per_sec * inst->clk_data.codec_freq_data->vpp_freq;
+ vpp_freq = mbs_per_sec * inst->clk_data.codec_freq_data.vpp_freq;
/* 21 / 20 is overhead factor */
vpp_freq += vpp_freq / 20;
- vsp_freq = mbs_per_sec * inst->clk_data.codec_freq_data->vsp_freq;
+ vsp_freq = mbs_per_sec * inst->clk_data.codec_freq_data.vsp_freq;
/* 10 / 7 is overhead factor */
if (inst->session_type == VIDC_SESSION_TYPE_ENC)
--
2.7.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] venus: avoid extra locking in driver
2020-03-06 5:32 [PATCH] venus: avoid extra locking in driver Mansur Alisha Shaik
@ 2020-03-06 7:50 ` Alexandre Courbot
2020-03-09 22:07 ` Jeffrey Kardatzke
2020-03-10 19:15 ` Jeffrey Kardatzke
1 sibling, 1 reply; 6+ messages in thread
From: Alexandre Courbot @ 2020-03-06 7:50 UTC (permalink / raw)
To: Mansur Alisha Shaik
Cc: Linux Media Mailing List, Stanimir Varbanov, LKML, linux-arm-msm,
Vikash Garodia
On Fri, Mar 6, 2020 at 2:34 PM Mansur Alisha Shaik
<mansur@codeaurora.org> wrote:
>
> This change will avoid extra locking in driver.
Could you elaborate a bit more on the problem that this patch solves?
>
> Signed-off-by: Mansur Alisha Shaik <mansur@codeaurora.org>
> ---
> 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, 100000000 }, /* 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(&inst->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 = &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)
> list_for_each_entry(inst_pos, &core->instances, list) {
> if (inst_pos == inst)
> continue;
> - vpp_freq = inst_pos->clk_data.codec_freq_data->vpp_freq;
> + vpp_freq = inst_pos->clk_data.codec_freq_data.vpp_freq;
> coreid = inst_pos->clk_data.core_id;
>
> mbs_per_sec = load_per_instance(inst_pos);
> @@ -545,7 +545,7 @@ static int decide_core(struct venus_inst *inst)
> return 0;
>
> inst_load = load_per_instance(inst);
> - inst_load *= inst->clk_data.codec_freq_data->vpp_freq;
> + inst_load *= inst->clk_data.codec_freq_data.vpp_freq;
> max_freq = core->res->freq_tbl[0].freq;
>
> min_loaded_core(inst, &min_coreid, &min_load);
> @@ -848,10 +848,10 @@ static unsigned long calculate_inst_freq(struct venus_inst *inst,
>
> mbs_per_sec = load_per_instance(inst) / fps;
>
> - vpp_freq = mbs_per_sec * inst->clk_data.codec_freq_data->vpp_freq;
> + vpp_freq = mbs_per_sec * inst->clk_data.codec_freq_data.vpp_freq;
> /* 21 / 20 is overhead factor */
> vpp_freq += vpp_freq / 20;
> - vsp_freq = mbs_per_sec * inst->clk_data.codec_freq_data->vsp_freq;
> + vsp_freq = mbs_per_sec * inst->clk_data.codec_freq_data.vsp_freq;
>
> /* 10 / 7 is overhead factor */
> if (inst->session_type == VIDC_SESSION_TYPE_ENC)
> --
> 2.7.4
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] venus: avoid extra locking in driver
2020-03-06 7:50 ` Alexandre Courbot
@ 2020-03-09 22:07 ` Jeffrey Kardatzke
2020-03-11 3:04 ` Alexandre Courbot
0 siblings, 1 reply; 6+ messages in thread
From: Jeffrey Kardatzke @ 2020-03-09 22:07 UTC (permalink / raw)
To: Alexandre Courbot
Cc: Mansur Alisha Shaik, Linux Media Mailing List, Stanimir Varbanov,
LKML, linux-arm-msm, Vikash Garodia
On Thu, Mar 5, 2020 at 11:50 PM Alexandre Courbot <acourbot@chromium.org> wrote:
>
> On Fri, Mar 6, 2020 at 2:34 PM Mansur Alisha Shaik
> <mansur@codeaurora.org> 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 <mansur@codeaurora.org>
> > ---
> > 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, 100000000 }, /* 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(&inst->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 = &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)
> > list_for_each_entry(inst_pos, &core->instances, list) {
> > if (inst_pos == inst)
> > continue;
> > - vpp_freq = inst_pos->clk_data.codec_freq_data->vpp_freq;
> > + vpp_freq = inst_pos->clk_data.codec_freq_data.vpp_freq;
This is the main thing it fixes (this is where the null deref occurs).
If there's multiple instances in use and the other instance hasn't
populated the codec_freq_data pointer then we'll hit a null deref
here.
> > coreid = inst_pos->clk_data.core_id;
> >
> > mbs_per_sec = load_per_instance(inst_pos);
> > @@ -545,7 +545,7 @@ static int decide_core(struct venus_inst *inst)
> > return 0;
> >
> > inst_load = load_per_instance(inst);
> > - inst_load *= inst->clk_data.codec_freq_data->vpp_freq;
> > + inst_load *= inst->clk_data.codec_freq_data.vpp_freq;
> > max_freq = core->res->freq_tbl[0].freq;
> >
> > min_loaded_core(inst, &min_coreid, &min_load);
> > @@ -848,10 +848,10 @@ static unsigned long calculate_inst_freq(struct venus_inst *inst,
> >
> > mbs_per_sec = load_per_instance(inst) / fps;
> >
> > - vpp_freq = mbs_per_sec * inst->clk_data.codec_freq_data->vpp_freq;
> > + vpp_freq = mbs_per_sec * inst->clk_data.codec_freq_data.vpp_freq;
> > /* 21 / 20 is overhead factor */
> > vpp_freq += vpp_freq / 20;
> > - vsp_freq = mbs_per_sec * inst->clk_data.codec_freq_data->vsp_freq;
> > + vsp_freq = mbs_per_sec * inst->clk_data.codec_freq_data.vsp_freq;
> >
> > /* 10 / 7 is overhead factor */
> > if (inst->session_type == VIDC_SESSION_TYPE_ENC)
> > --
> > 2.7.4
> >
--
Jeffrey Kardatzke
jkardatzke@google.com
Google, Inc.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] venus: avoid extra locking in driver
2020-03-06 5:32 [PATCH] venus: avoid extra locking in driver Mansur Alisha Shaik
2020-03-06 7:50 ` Alexandre Courbot
@ 2020-03-10 19:15 ` Jeffrey Kardatzke
1 sibling, 0 replies; 6+ messages in thread
From: Jeffrey Kardatzke @ 2020-03-10 19:15 UTC (permalink / raw)
To: Mansur Alisha Shaik
Cc: Linux Media Mailing List, Stanimir Varbanov, LKML, linux-arm-msm,
Vikash Garodia
Tested-by: Jeffrey Kardatzke <jkardatzke@google.com>
On Thu, Mar 5, 2020 at 9:34 PM Mansur Alisha Shaik
<mansur@codeaurora.org> wrote:
>
> This change will avoid extra locking in driver.
>
> Signed-off-by: Mansur Alisha Shaik <mansur@codeaurora.org>
> ---
> 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, 100000000 }, /* 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(&inst->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 = &data[i];
> + inst->clk_data.codec_freq_data = data[i];
> + 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)
> list_for_each_entry(inst_pos, &core->instances, list) {
> if (inst_pos == inst)
> continue;
> - vpp_freq = inst_pos->clk_data.codec_freq_data->vpp_freq;
> + vpp_freq = inst_pos->clk_data.codec_freq_data.vpp_freq;
> coreid = inst_pos->clk_data.core_id;
>
> mbs_per_sec = load_per_instance(inst_pos);
> @@ -545,7 +545,7 @@ static int decide_core(struct venus_inst *inst)
> return 0;
>
> inst_load = load_per_instance(inst);
> - inst_load *= inst->clk_data.codec_freq_data->vpp_freq;
> + inst_load *= inst->clk_data.codec_freq_data.vpp_freq;
> max_freq = core->res->freq_tbl[0].freq;
>
> min_loaded_core(inst, &min_coreid, &min_load);
> @@ -848,10 +848,10 @@ static unsigned long calculate_inst_freq(struct venus_inst *inst,
>
> mbs_per_sec = load_per_instance(inst) / fps;
>
> - vpp_freq = mbs_per_sec * inst->clk_data.codec_freq_data->vpp_freq;
> + vpp_freq = mbs_per_sec * inst->clk_data.codec_freq_data.vpp_freq;
> /* 21 / 20 is overhead factor */
> vpp_freq += vpp_freq / 20;
> - vsp_freq = mbs_per_sec * inst->clk_data.codec_freq_data->vsp_freq;
> + vsp_freq = mbs_per_sec * inst->clk_data.codec_freq_data.vsp_freq;
>
> /* 10 / 7 is overhead factor */
> if (inst->session_type == VIDC_SESSION_TYPE_ENC)
> --
> 2.7.4
>
--
Jeffrey Kardatzke
jkardatzke@google.com
Google, Inc.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] venus: avoid extra locking in driver
2020-03-09 22:07 ` Jeffrey Kardatzke
@ 2020-03-11 3:04 ` Alexandre Courbot
2020-05-01 7:09 ` mansur
0 siblings, 1 reply; 6+ messages in thread
From: Alexandre Courbot @ 2020-03-11 3:04 UTC (permalink / raw)
To: Jeffrey Kardatzke
Cc: Mansur Alisha Shaik, Linux Media Mailing List, Stanimir Varbanov,
LKML, linux-arm-msm, Vikash Garodia
On Tue, Mar 10, 2020 at 7:07 AM Jeffrey Kardatzke <jkardatzke@google.com> wrote:
>
> On Thu, Mar 5, 2020 at 11:50 PM Alexandre Courbot <acourbot@chromium.org> wrote:
> >
> > On Fri, Mar 6, 2020 at 2:34 PM Mansur Alisha Shaik
> > <mansur@codeaurora.org> 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 <mansur@codeaurora.org>
> > > ---
> > > 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, 100000000 }, /* 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(&inst->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 = &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)
> > > list_for_each_entry(inst_pos, &core->instances, list) {
> > > if (inst_pos == inst)
> > > continue;
> > > - vpp_freq = inst_pos->clk_data.codec_freq_data->vpp_freq;
> > > + vpp_freq = inst_pos->clk_data.codec_freq_data.vpp_freq;
>
> This is the main thing it fixes (this is where the null deref occurs).
> If there's multiple instances in use and the other instance hasn't
> populated the codec_freq_data pointer then we'll hit a null deref
> here.
Couldn't this be fixed by checking the pointer for NULL here or
(probably better) populating codec_freq_data earlier so that it is
always valid?
This fix looks like it is replacing a NULL pointer dereference with
access to data initialized to fallback values (which may or may not be
meaningful), and I don't see the need to copy what is effectively
constant data into each instance.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] venus: avoid extra locking in driver
2020-03-11 3:04 ` Alexandre Courbot
@ 2020-05-01 7:09 ` mansur
0 siblings, 0 replies; 6+ messages in thread
From: mansur @ 2020-05-01 7:09 UTC (permalink / raw)
To: Alexandre Courbot
Cc: Jeffrey Kardatzke, Linux Media Mailing List, Stanimir Varbanov,
LKML, linux-arm-msm, Vikash Garodia
> On 2020-03-11 08:34, Alexandre Courbot wrote:
>> On Tue, Mar 10, 2020 at 7:07 AM Jeffrey Kardatzke
>> <jkardatzke@google.com> wrote:
>>>
>>> On Thu, Mar 5, 2020 at 11:50 PM Alexandre Courbot
>>> <acourbot@chromium.org> wrote:
>>> >
>>> > On Fri, Mar 6, 2020 at 2:34 PM Mansur Alisha Shaik
>>> > <mansur@codeaurora.org> 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 <mansur@codeaurora.org>
>>> > > ---
>>> > > 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, 100000000 }, /* 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(&inst->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 = &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)
>>> > > list_for_each_entry(inst_pos, &core->instances, list) {
>>> > > if (inst_pos == inst)
>>> > > continue;
>>> > > - vpp_freq = inst_pos->clk_data.codec_freq_data->vpp_freq;
>>> > > + vpp_freq = inst_pos->clk_data.codec_freq_data.vpp_freq;
>>>
>>> This is the main thing it fixes (this is where the null deref
>>> occurs).
>>> If there's multiple instances in use and the other instance hasn't
>>> populated the codec_freq_data pointer then we'll hit a null deref
>>> here.
>>
>> Couldn't this be fixed by checking the pointer for NULL here or
>> (probably better) populating codec_freq_data earlier so that it is
>> always valid?
This can be fix by checking for NULL pointer as well, this looks like
masking the actual issue.
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.
So we consider the instances for load calculation for those session has
started.
and uploaded V2 version https://lore.kernel.org/patchwork/patch/1234136/
for the same.
>> This fix looks like it is replacing a NULL pointer dereference with
>> access to data initialized to fallback values (which may or may not be
>> meaningful), and I don't see the need to copy what is effectively
>> constant data into each instance.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-05-01 7:09 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-06 5:32 [PATCH] venus: avoid extra locking in driver Mansur Alisha Shaik
2020-03-06 7:50 ` Alexandre Courbot
2020-03-09 22:07 ` Jeffrey Kardatzke
2020-03-11 3:04 ` Alexandre Courbot
2020-05-01 7:09 ` mansur
2020-03-10 19:15 ` Jeffrey Kardatzke
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.