| Message ID | 20241213-add-venus-for-qcs615-v4-2-7e2c9a72d309@quicinc.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | media: venus: enable venus on qcs615 | expand |
On Fri, Dec 13, 2024 at 03:26:47PM +0530, Renjiang Han wrote: > Get frequency value from the opp-table of devicetree for the v4 core. > For compatibility, if getting data from the opp table fails, the data > in the frequency table will be used. > > The order of variable definitions is adjusted only to keep the reverse > Christmas tree order coding style. > 1) Do the best you can to add your variables while trying to maintain the order, but if it's not possible better leave it than making it hard to parse logical change from shuffling of code. 2) This comment is useful during review, but not necessarily so in the git history, so I'd suggest to keep it below the '---' line. > Signed-off-by: Renjiang Han <quic_renjiang@quicinc.com> > --- > drivers/media/platform/qcom/venus/pm_helpers.c | 67 ++++++++++++++++++-------- > 1 file changed, 46 insertions(+), 21 deletions(-) > > diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c > index 33a5a659c0ada0ca97eebb5522c5f349f95c49c7..a5c3f9ad2088d8c80247b52d5c1b8e053f499bfe 100644 > --- a/drivers/media/platform/qcom/venus/pm_helpers.c > +++ b/drivers/media/platform/qcom/venus/pm_helpers.c > @@ -40,17 +40,23 @@ static int core_clks_get(struct venus_core *core) > > static int core_clks_enable(struct venus_core *core) > { > - const struct venus_resources *res = core->res; > const struct freq_tbl *freq_tbl = core->res->freq_tbl; > unsigned int freq_tbl_size = core->res->freq_tbl_size; > - unsigned long freq; > + const struct venus_resources *res = core->res; > + struct device *dev = core->dev; > + unsigned long freq = 0; Is it really necessary to initialize this? I'd expect that dev_pm_opp_find_freq_ceil() would either initialize freq or return a failure, in which case you assign freq. Perhaps the compiler isn't clever enough to see this? > + struct dev_pm_opp *opp; > unsigned int i; > int ret; > > - if (!freq_tbl) > - return -EINVAL; > - > - freq = freq_tbl[freq_tbl_size - 1].freq; > + opp = dev_pm_opp_find_freq_ceil(dev, &freq); > + if (IS_ERR(opp)) { > + if (!freq_tbl) > + return -EINVAL; > + freq = freq_tbl[freq_tbl_size - 1].freq; > + } else { > + dev_pm_opp_put(opp); > + } > > for (i = 0; i < res->clks_num; i++) { > if (IS_V6(core)) { > @@ -627,12 +633,15 @@ min_loaded_core(struct venus_inst *inst, u32 *min_coreid, u32 *min_load, bool lo > > static int decide_core(struct venus_inst *inst) > { > + const struct freq_tbl *freq_tbl = inst->core->res->freq_tbl; > const u32 ptype = HFI_PROPERTY_CONFIG_VIDEOCORES_USAGE; > - struct venus_core *core = inst->core; > - u32 min_coreid, min_load, cur_inst_load; > u32 min_lp_coreid, min_lp_load, cur_inst_lp_load; > + u32 min_coreid, min_load, cur_inst_load; > + struct venus_core *core = inst->core; > struct hfi_videocores_usage_type cu; > - unsigned long max_freq; > + unsigned long max_freq = ULONG_MAX; > + struct device *dev = core->dev; > + struct dev_pm_opp *opp; Here the line shuffling makes it hard to determine what is part of the logical change and what is just style changes... Regards, Bjorn > int ret = 0; > > if (legacy_binding) { > @@ -655,7 +664,11 @@ static int decide_core(struct venus_inst *inst) > cur_inst_lp_load *= inst->clk_data.low_power_freq; > /*TODO : divide this inst->load by work_route */ > > - max_freq = core->res->freq_tbl[0].freq; > + opp = dev_pm_opp_find_freq_floor(dev, &max_freq); > + if (IS_ERR(opp)) > + max_freq = freq_tbl[0].freq; > + else > + dev_pm_opp_put(opp); > > min_loaded_core(inst, &min_coreid, &min_load, false); > min_loaded_core(inst, &min_lp_coreid, &min_lp_load, true); > @@ -1073,12 +1086,14 @@ static unsigned long calculate_inst_freq(struct venus_inst *inst, > > static int load_scale_v4(struct venus_inst *inst) > { > + const struct freq_tbl *table = inst->core->res->freq_tbl; > + unsigned int num_rows = inst->core->res->freq_tbl_size; > + unsigned long freq = 0, freq_core1 = 0, freq_core2 = 0; > struct venus_core *core = inst->core; > - const struct freq_tbl *table = core->res->freq_tbl; > - unsigned int num_rows = core->res->freq_tbl_size; > + unsigned long max_freq = ULONG_MAX; > struct device *dev = core->dev; > - unsigned long freq = 0, freq_core1 = 0, freq_core2 = 0; > unsigned long filled_len = 0; > + struct dev_pm_opp *opp; > int i, ret = 0; > > for (i = 0; i < inst->num_input_bufs; i++) > @@ -1104,19 +1119,29 @@ static int load_scale_v4(struct venus_inst *inst) > > freq = max(freq_core1, freq_core2); > > - if (freq > table[0].freq) { > - dev_dbg(dev, VDBGL "requested clock rate: %lu scaling clock rate : %lu\n", > - freq, table[0].freq); > + opp = dev_pm_opp_find_freq_floor(dev, &max_freq); > + if (IS_ERR(opp)) > + max_freq = table[0].freq; > + else > + dev_pm_opp_put(opp); > > - freq = table[0].freq; > + if (freq > max_freq) { > + dev_dbg(dev, VDBGL "requested clock rate: %lu scaling clock rate : %lu\n", > + freq, max_freq); > + freq = max_freq; > goto set_freq; > } > > - for (i = num_rows - 1 ; i >= 0; i--) { > - if (freq <= table[i].freq) { > - freq = table[i].freq; > - break; > + opp = dev_pm_opp_find_freq_ceil(dev, &freq); > + if (IS_ERR(opp)) { > + for (i = num_rows - 1 ; i >= 0; i--) { > + if (freq <= table[i].freq) { > + freq = table[i].freq; > + break; > + } > } > + } else { > + dev_pm_opp_put(opp); > } > > set_freq: > > -- > 2.34.1 >
On 12/17/2024 2:21 AM, Bjorn Andersson wrote: > On Fri, Dec 13, 2024 at 03:26:47PM +0530, Renjiang Han wrote: >> Get frequency value from the opp-table of devicetree for the v4 core. >> For compatibility, if getting data from the opp table fails, the data >> in the frequency table will be used. >> >> The order of variable definitions is adjusted only to keep the reverse >> Christmas tree order coding style. >> > 1) Do the best you can to add your variables while trying to maintain > the order, but if it's not possible better leave it than making it hard > to parse logical change from shuffling of code. OK, thanks for pointing it out. > > 2) This comment is useful during review, but not necessarily so in the > git history, so I'd suggest to keep it below the '---' line. Do you mean that the message about my code style changes only needs to be added to the cover letter? > >> Signed-off-by: Renjiang Han <quic_renjiang@quicinc.com> >> --- >> drivers/media/platform/qcom/venus/pm_helpers.c | 67 ++++++++++++++++++-------- >> 1 file changed, 46 insertions(+), 21 deletions(-) >> >> diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c >> index 33a5a659c0ada0ca97eebb5522c5f349f95c49c7..a5c3f9ad2088d8c80247b52d5c1b8e053f499bfe 100644 >> --- a/drivers/media/platform/qcom/venus/pm_helpers.c >> +++ b/drivers/media/platform/qcom/venus/pm_helpers.c >> @@ -40,17 +40,23 @@ static int core_clks_get(struct venus_core *core) >> >> static int core_clks_enable(struct venus_core *core) >> { >> - const struct venus_resources *res = core->res; >> const struct freq_tbl *freq_tbl = core->res->freq_tbl; >> unsigned int freq_tbl_size = core->res->freq_tbl_size; >> - unsigned long freq; >> + const struct venus_resources *res = core->res; >> + struct device *dev = core->dev; >> + unsigned long freq = 0; > Is it really necessary to initialize this? I'd expect that > dev_pm_opp_find_freq_ceil() would either initialize freq or return a > failure, in which case you assign freq. Thanks for your review. There is really no need to initialize freq. The default value of freq is 0 here. dev_pm_opp_find_freq_ceil() will query the matching value from the opp table based on this default value. > > Perhaps the compiler isn't clever enough to see this? > >> + struct dev_pm_opp *opp; >> unsigned int i; >> int ret; >> >> - if (!freq_tbl) >> - return -EINVAL; >> - >> - freq = freq_tbl[freq_tbl_size - 1].freq; >> + opp = dev_pm_opp_find_freq_ceil(dev, &freq); >> + if (IS_ERR(opp)) { >> + if (!freq_tbl) >> + return -EINVAL; >> + freq = freq_tbl[freq_tbl_size - 1].freq; >> + } else { >> + dev_pm_opp_put(opp); >> + } >> >> for (i = 0; i < res->clks_num; i++) { >> if (IS_V6(core)) { >> @@ -627,12 +633,15 @@ min_loaded_core(struct venus_inst *inst, u32 *min_coreid, u32 *min_load, bool lo >> >> static int decide_core(struct venus_inst *inst) >> { >> + const struct freq_tbl *freq_tbl = inst->core->res->freq_tbl; >> const u32 ptype = HFI_PROPERTY_CONFIG_VIDEOCORES_USAGE; >> - struct venus_core *core = inst->core; >> - u32 min_coreid, min_load, cur_inst_load; >> u32 min_lp_coreid, min_lp_load, cur_inst_lp_load; >> + u32 min_coreid, min_load, cur_inst_load; >> + struct venus_core *core = inst->core; >> struct hfi_videocores_usage_type cu; >> - unsigned long max_freq; >> + unsigned long max_freq = ULONG_MAX; >> + struct device *dev = core->dev; >> + struct dev_pm_opp *opp; > Here the line shuffling makes it hard to determine what is part of the > logical change and what is just style changes... > > Regards, > Bjorn You mean I only need to add variable definitions basing on the original order of variable definitions, and I don't need to modify the order of variable definitions specifically for the reverse Christmas tree code style. Is that right? > >> int ret = 0; >> >> if (legacy_binding) { >> @@ -655,7 +664,11 @@ static int decide_core(struct venus_inst *inst) >> cur_inst_lp_load *= inst->clk_data.low_power_freq; >> /*TODO : divide this inst->load by work_route */ >> >> - max_freq = core->res->freq_tbl[0].freq; >> + opp = dev_pm_opp_find_freq_floor(dev, &max_freq); >> + if (IS_ERR(opp)) >> + max_freq = freq_tbl[0].freq; >> + else >> + dev_pm_opp_put(opp); >> >> min_loaded_core(inst, &min_coreid, &min_load, false); >> min_loaded_core(inst, &min_lp_coreid, &min_lp_load, true); >> @@ -1073,12 +1086,14 @@ static unsigned long calculate_inst_freq(struct venus_inst *inst, >> >> static int load_scale_v4(struct venus_inst *inst) >> { >> + const struct freq_tbl *table = inst->core->res->freq_tbl; >> + unsigned int num_rows = inst->core->res->freq_tbl_size; >> + unsigned long freq = 0, freq_core1 = 0, freq_core2 = 0; >> struct venus_core *core = inst->core; >> - const struct freq_tbl *table = core->res->freq_tbl; >> - unsigned int num_rows = core->res->freq_tbl_size; >> + unsigned long max_freq = ULONG_MAX; >> struct device *dev = core->dev; >> - unsigned long freq = 0, freq_core1 = 0, freq_core2 = 0; >> unsigned long filled_len = 0; >> + struct dev_pm_opp *opp; >> int i, ret = 0; >> >> for (i = 0; i < inst->num_input_bufs; i++) >> @@ -1104,19 +1119,29 @@ static int load_scale_v4(struct venus_inst *inst) >> >> freq = max(freq_core1, freq_core2); >> >> - if (freq > table[0].freq) { >> - dev_dbg(dev, VDBGL "requested clock rate: %lu scaling clock rate : %lu\n", >> - freq, table[0].freq); >> + opp = dev_pm_opp_find_freq_floor(dev, &max_freq); >> + if (IS_ERR(opp)) >> + max_freq = table[0].freq; >> + else >> + dev_pm_opp_put(opp); >> >> - freq = table[0].freq; >> + if (freq > max_freq) { >> + dev_dbg(dev, VDBGL "requested clock rate: %lu scaling clock rate : %lu\n", >> + freq, max_freq); >> + freq = max_freq; >> goto set_freq; >> } >> >> - for (i = num_rows - 1 ; i >= 0; i--) { >> - if (freq <= table[i].freq) { >> - freq = table[i].freq; >> - break; >> + opp = dev_pm_opp_find_freq_ceil(dev, &freq); >> + if (IS_ERR(opp)) { >> + for (i = num_rows - 1 ; i >= 0; i--) { >> + if (freq <= table[i].freq) { >> + freq = table[i].freq; >> + break; >> + } >> } >> + } else { >> + dev_pm_opp_put(opp); >> } >> >> set_freq: >> >> -- >> 2.34.1 >>
diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c index 33a5a659c0ada0ca97eebb5522c5f349f95c49c7..a5c3f9ad2088d8c80247b52d5c1b8e053f499bfe 100644 --- a/drivers/media/platform/qcom/venus/pm_helpers.c +++ b/drivers/media/platform/qcom/venus/pm_helpers.c @@ -40,17 +40,23 @@ static int core_clks_get(struct venus_core *core) static int core_clks_enable(struct venus_core *core) { - const struct venus_resources *res = core->res; const struct freq_tbl *freq_tbl = core->res->freq_tbl; unsigned int freq_tbl_size = core->res->freq_tbl_size; - unsigned long freq; + const struct venus_resources *res = core->res; + struct device *dev = core->dev; + unsigned long freq = 0; + struct dev_pm_opp *opp; unsigned int i; int ret; - if (!freq_tbl) - return -EINVAL; - - freq = freq_tbl[freq_tbl_size - 1].freq; + opp = dev_pm_opp_find_freq_ceil(dev, &freq); + if (IS_ERR(opp)) { + if (!freq_tbl) + return -EINVAL; + freq = freq_tbl[freq_tbl_size - 1].freq; + } else { + dev_pm_opp_put(opp); + } for (i = 0; i < res->clks_num; i++) { if (IS_V6(core)) { @@ -627,12 +633,15 @@ min_loaded_core(struct venus_inst *inst, u32 *min_coreid, u32 *min_load, bool lo static int decide_core(struct venus_inst *inst) { + const struct freq_tbl *freq_tbl = inst->core->res->freq_tbl; const u32 ptype = HFI_PROPERTY_CONFIG_VIDEOCORES_USAGE; - struct venus_core *core = inst->core; - u32 min_coreid, min_load, cur_inst_load; u32 min_lp_coreid, min_lp_load, cur_inst_lp_load; + u32 min_coreid, min_load, cur_inst_load; + struct venus_core *core = inst->core; struct hfi_videocores_usage_type cu; - unsigned long max_freq; + unsigned long max_freq = ULONG_MAX; + struct device *dev = core->dev; + struct dev_pm_opp *opp; int ret = 0; if (legacy_binding) { @@ -655,7 +664,11 @@ static int decide_core(struct venus_inst *inst) cur_inst_lp_load *= inst->clk_data.low_power_freq; /*TODO : divide this inst->load by work_route */ - max_freq = core->res->freq_tbl[0].freq; + opp = dev_pm_opp_find_freq_floor(dev, &max_freq); + if (IS_ERR(opp)) + max_freq = freq_tbl[0].freq; + else + dev_pm_opp_put(opp); min_loaded_core(inst, &min_coreid, &min_load, false); min_loaded_core(inst, &min_lp_coreid, &min_lp_load, true); @@ -1073,12 +1086,14 @@ static unsigned long calculate_inst_freq(struct venus_inst *inst, static int load_scale_v4(struct venus_inst *inst) { + const struct freq_tbl *table = inst->core->res->freq_tbl; + unsigned int num_rows = inst->core->res->freq_tbl_size; + unsigned long freq = 0, freq_core1 = 0, freq_core2 = 0; struct venus_core *core = inst->core; - const struct freq_tbl *table = core->res->freq_tbl; - unsigned int num_rows = core->res->freq_tbl_size; + unsigned long max_freq = ULONG_MAX; struct device *dev = core->dev; - unsigned long freq = 0, freq_core1 = 0, freq_core2 = 0; unsigned long filled_len = 0; + struct dev_pm_opp *opp; int i, ret = 0; for (i = 0; i < inst->num_input_bufs; i++) @@ -1104,19 +1119,29 @@ static int load_scale_v4(struct venus_inst *inst) freq = max(freq_core1, freq_core2); - if (freq > table[0].freq) { - dev_dbg(dev, VDBGL "requested clock rate: %lu scaling clock rate : %lu\n", - freq, table[0].freq); + opp = dev_pm_opp_find_freq_floor(dev, &max_freq); + if (IS_ERR(opp)) + max_freq = table[0].freq; + else + dev_pm_opp_put(opp); - freq = table[0].freq; + if (freq > max_freq) { + dev_dbg(dev, VDBGL "requested clock rate: %lu scaling clock rate : %lu\n", + freq, max_freq); + freq = max_freq; goto set_freq; } - for (i = num_rows - 1 ; i >= 0; i--) { - if (freq <= table[i].freq) { - freq = table[i].freq; - break; + opp = dev_pm_opp_find_freq_ceil(dev, &freq); + if (IS_ERR(opp)) { + for (i = num_rows - 1 ; i >= 0; i--) { + if (freq <= table[i].freq) { + freq = table[i].freq; + break; + } } + } else { + dev_pm_opp_put(opp); } set_freq:
Get frequency value from the opp-table of devicetree for the v4 core. For compatibility, if getting data from the opp table fails, the data in the frequency table will be used. The order of variable definitions is adjusted only to keep the reverse Christmas tree order coding style. Signed-off-by: Renjiang Han <quic_renjiang@quicinc.com> --- drivers/media/platform/qcom/venus/pm_helpers.c | 67 ++++++++++++++++++-------- 1 file changed, 46 insertions(+), 21 deletions(-)