This change will avoid extra locking in driver.
Signed-off-by: Mansur Alisha Shaik <[email protected]>
---
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
On Fri, Mar 6, 2020 at 2:34 PM Mansur Alisha Shaik
<[email protected]> 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 <[email protected]>
> ---
> 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
>
On Thu, Mar 5, 2020 at 11:50 PM Alexandre Courbot <[email protected]> wrote:
>
> On Fri, Mar 6, 2020 at 2:34 PM Mansur Alisha Shaik
> <[email protected]> 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 <[email protected]>
> > ---
> > 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
[email protected]
Google, Inc.
Tested-by: Jeffrey Kardatzke <[email protected]>
On Thu, Mar 5, 2020 at 9:34 PM Mansur Alisha Shaik
<[email protected]> wrote:
>
> This change will avoid extra locking in driver.
>
> Signed-off-by: Mansur Alisha Shaik <[email protected]>
> ---
> 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
[email protected]
Google, Inc.
On Tue, Mar 10, 2020 at 7:07 AM Jeffrey Kardatzke <[email protected]> wrote:
>
> On Thu, Mar 5, 2020 at 11:50 PM Alexandre Courbot <[email protected]> wrote:
> >
> > On Fri, Mar 6, 2020 at 2:34 PM Mansur Alisha Shaik
> > <[email protected]> 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 <[email protected]>
> > > ---
> > > 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.
> On 2020-03-11 08:34, Alexandre Courbot wrote:
>> On Tue, Mar 10, 2020 at 7:07 AM Jeffrey Kardatzke
>> <[email protected]> wrote:
>>>
>>> On Thu, Mar 5, 2020 at 11:50 PM Alexandre Courbot
>>> <[email protected]> wrote:
>>> >
>>> > On Fri, Mar 6, 2020 at 2:34 PM Mansur Alisha Shaik
>>> > <[email protected]> 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 <[email protected]>
>>> > > ---
>>> > > 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.