| Message ID | 20241128-topic-sm8x50-gpu-bw-vote-v3-0-81d60c10fb73@linaro.org |
|---|---|
| Headers | show |
| Series | drm/msm: adreno: add support for DDR bandwidth scaling via GMU | expand |
On Thu, Nov 28, 2024 at 11:25:41AM +0100, Neil Armstrong wrote: > Even if the code uses ARRAY_SIZE() to fill those tables, > it's still a best practice to not use magic values for > tables in structs. > > Suggested-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> > --- > drivers/gpu/drm/msm/adreno/a6xx_gmu.h | 11 +++++++---- > 1 file changed, 7 insertions(+), 4 deletions(-) > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
On Thu, Nov 28, 2024 at 11:25:45AM +0100, Neil Armstrong wrote: > Now all the DDR bandwidth voting via the GPU Management Unit (GMU) > is in place, declare the Bus Control Modules (BCMs) and the > corresponding parameters in the GPU info struct and add the > GMU_BW_VOTE feature bit to enable it. > > Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> > --- > drivers/gpu/drm/msm/adreno/a6xx_catalog.c | 22 ++++++++++++++++++++++ > 1 file changed, 22 insertions(+) > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
On 28/11/2024 14:26, Dmitry Baryshkov wrote: > On Thu, Nov 28, 2024 at 11:25:46AM +0100, Neil Armstrong wrote: >> Each GPU OPP requires a specific peak DDR bandwidth, let's add >> those to each OPP and also the related interconnect path. >> >> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> >> --- >> arch/arm64/boot/dts/qcom/sm8550.dtsi | 11 +++++++++++ >> 1 file changed, 11 insertions(+) >> >> diff --git a/arch/arm64/boot/dts/qcom/sm8550.dtsi b/arch/arm64/boot/dts/qcom/sm8550.dtsi >> index e7774d32fb6d2288748ecec00bf525b2b3c40fbb..545eb52174c704bbefa69189fad9fbff053d8569 100644 >> --- a/arch/arm64/boot/dts/qcom/sm8550.dtsi >> +++ b/arch/arm64/boot/dts/qcom/sm8550.dtsi >> @@ -2114,6 +2114,9 @@ gpu: gpu@3d00000 { >> qcom,gmu = <&gmu>; >> #cooling-cells = <2>; >> >> + interconnects = <&gem_noc MASTER_GFX3D 0 &mc_virt SLAVE_EBI1 0>; > > QCOM_ICC_TAG_ALWAYS Damn, thanks forgot those one. Thanks, Neil > > LGTM otherwise. > >> + interconnect-names = "gfx-mem"; >> + >> status = "disabled"; >> >> zap-shader { >> @@ -2127,41 +2130,49 @@ gpu_opp_table: opp-table { >> opp-680000000 { >> opp-hz = /bits/ 64 <680000000>; >> opp-level = <RPMH_REGULATOR_LEVEL_SVS_L1>; >> + opp-peak-kBps = <16500000>; >> }; >> >> opp-615000000 { >> opp-hz = /bits/ 64 <615000000>; >> opp-level = <RPMH_REGULATOR_LEVEL_SVS_L0>; >> + opp-peak-kBps = <16500000>; >> }; >> >> opp-550000000 { >> opp-hz = /bits/ 64 <550000000>; >> opp-level = <RPMH_REGULATOR_LEVEL_SVS>; >> + opp-peak-kBps = <12449218>; >> }; >> >> opp-475000000 { >> opp-hz = /bits/ 64 <475000000>; >> opp-level = <RPMH_REGULATOR_LEVEL_LOW_SVS_L1>; >> + opp-peak-kBps = <8171875>; >> }; >> >> opp-401000000 { >> opp-hz = /bits/ 64 <401000000>; >> opp-level = <RPMH_REGULATOR_LEVEL_LOW_SVS>; >> + opp-peak-kBps = <6671875>; >> }; >> >> opp-348000000 { >> opp-hz = /bits/ 64 <348000000>; >> opp-level = <RPMH_REGULATOR_LEVEL_LOW_SVS_D0>; >> + opp-peak-kBps = <6074218>; >> }; >> >> opp-295000000 { >> opp-hz = /bits/ 64 <295000000>; >> opp-level = <RPMH_REGULATOR_LEVEL_LOW_SVS_D1>; >> + opp-peak-kBps = <6074218>; >> }; >> >> opp-220000000 { >> opp-hz = /bits/ 64 <220000000>; >> opp-level = <RPMH_REGULATOR_LEVEL_LOW_SVS_D2>; >> + opp-peak-kBps = <6074218>; >> }; >> }; >> }; >> >> -- >> 2.34.1 >> >
On 28.11.2024 11:25 AM, Neil Armstrong wrote: > The Adreno GPU Management Unit (GMU) can also scale DDR Bandwidth along > the Frequency and Power Domain level, but by default we leave the > OPP core scale the interconnect ddr path. > > While scaling via the interconnect path was sufficient, newer GPUs > like the A750 requires specific vote paremeters and bandwidth to > achieve full functionality. > > In order to calculate vote values used by the GPU Management > Unit (GMU), we need to parse all the possible OPP Bandwidths and > create a vote value to be sent to the appropriate Bus Control > Modules (BCMs) declared in the GPU info struct. > > This vote value is called IB, while on the othe side the GMU also > takes another vote called AB which is a 16bit quantized value > of the bandwidth against the maximum supported bandwidth. > > The vote array will then be used to dynamically generate the GMU > bw_table sent during the GMU power-up. > > Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> > --- > drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 174 ++++++++++++++++++++++++++++++++++ > drivers/gpu/drm/msm/adreno/a6xx_gmu.h | 14 +++ > drivers/gpu/drm/msm/adreno/a6xx_gpu.h | 1 + > drivers/gpu/drm/msm/adreno/a6xx_hfi.h | 5 + > 4 files changed, 194 insertions(+) > > diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c > index 14db7376c712d19446b38152e480bd5a1e0a5198..ee2010a01186721dd377f1655fcf05ddaff77131 100644 > --- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c > +++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c > @@ -9,6 +9,7 @@ > #include <linux/pm_domain.h> > #include <linux/pm_opp.h> > #include <soc/qcom/cmd-db.h> > +#include <soc/qcom/tcs.h> > #include <drm/drm_gem.h> > > #include "a6xx_gpu.h" > @@ -1287,6 +1288,131 @@ static int a6xx_gmu_memory_probe(struct a6xx_gmu *gmu) > return 0; > } > > +/** > + * struct bcm_db - Auxiliary data pertaining to each Bus Clock Manager (BCM) > + * @unit: divisor used to convert bytes/sec bw value to an RPMh msg > + * @width: multiplier used to convert bytes/sec bw value to an RPMh msg > + * @vcd: virtual clock domain that this bcm belongs to > + * @reserved: reserved field > + */ > +struct bcm_db { > + __le32 unit; > + __le16 width; > + u8 vcd; > + u8 reserved; > +}; > + > +static u64 bcm_div(u64 num, u32 base) > +{ > + /* Ensure that small votes aren't lost. */ > + if (num && num < base) > + return 1; > + > + do_div(num, base); > + > + return num; > +} This should live in include/soc/qcom/bcm.h, similarly to tcs.h in that directory > +static int a6xx_gmu_rpmh_bw_votes_init(const struct a6xx_info *info, > + struct a6xx_gmu *gmu) > +{ > + const struct bcm_db *bcm_data[GMU_MAX_BCMS] = { 0 }; > + unsigned int bcm_index, bw_index, bcm_count = 0; > + > + if (!info->bcms) > + return 0; > + > + /* Retrieve BCM data from cmd-db */ > + for (bcm_index = 0; bcm_index < GMU_MAX_BCMS; bcm_index++) { > + size_t count; > + > + /* Stop at first unconfigured bcm */ > + if (!info->bcms[bcm_index].name) > + break; > + > + bcm_data[bcm_index] = cmd_db_read_aux_data( > + info->bcms[bcm_index].name, > + &count); > + if (IS_ERR(bcm_data[bcm_index])) > + return PTR_ERR(bcm_data[bcm_index]); > + > + if (!count) > + return -EINVAL; > + > + ++bcm_count; > + } > + > + /* Generate BCM votes values for each bandwidth & BCM */ > + for (bw_index = 0; bw_index < gmu->nr_gpu_bws; bw_index++) { > + u32 *data = gmu->gpu_ib_votes[bw_index]; > + u32 bw = gmu->gpu_bw_table[bw_index]; > + > + /* Calculations loosely copied from bcm_aggregate() & tcs_cmd_gen() */ Ditto, perhaps this should be exported from icc [...] Konrad
On 28.11.2024 11:25 AM, Neil Armstrong wrote: > Now all the DDR bandwidth voting via the GPU Management Unit (GMU) > is in place, declare the Bus Control Modules (BCMs) and the > corresponding parameters in the GPU info struct and add the > GMU_BW_VOTE feature bit to enable it. > > Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> > --- > drivers/gpu/drm/msm/adreno/a6xx_catalog.c | 22 ++++++++++++++++++++++ > 1 file changed, 22 insertions(+) > > diff --git a/drivers/gpu/drm/msm/adreno/a6xx_catalog.c b/drivers/gpu/drm/msm/adreno/a6xx_catalog.c > index 0c560e84ad5a53bb4e8a49ba4e153ce9cf33f7ae..edffb7737a97b268bb2986d557969e651988a344 100644 > --- a/drivers/gpu/drm/msm/adreno/a6xx_catalog.c > +++ b/drivers/gpu/drm/msm/adreno/a6xx_catalog.c > @@ -1388,6 +1388,17 @@ static const struct adreno_info a7xx_gpus[] = { > .pwrup_reglist = &a7xx_pwrup_reglist, > .gmu_chipid = 0x7020100, > .gmu_cgc_mode = 0x00020202, > + .bcms = (const struct a6xx_bcm[]) { > + { .name = "SH0", .buswidth = 16 }, > + { .name = "MC0", .buswidth = 4 }, > + { > + .name = "ACV", > + .fixed = true, > + .perfmode = BIT(3), > + .perfmode_bw = 16500000, > + }, > + { /* sentinel */ }, > + }, This is not going to fly the second there's two SoCs implementing the same GPU with a difference in bus topology. I think we could add something like drvdata to ICC nodes and use it for BCMs on icc-rpmh. Then, we could retrieve it from the interconnect path we get from the dt node. It would also reduce duplication. Konrad
On 29/11/2024 16:21, Konrad Dybcio wrote: > On 28.11.2024 11:25 AM, Neil Armstrong wrote: >> The Adreno GPU Management Unit (GMU) can also scale DDR Bandwidth along >> the Frequency and Power Domain level, but by default we leave the >> OPP core scale the interconnect ddr path. >> >> While scaling via the interconnect path was sufficient, newer GPUs >> like the A750 requires specific vote paremeters and bandwidth to >> achieve full functionality. >> >> In order to calculate vote values used by the GPU Management >> Unit (GMU), we need to parse all the possible OPP Bandwidths and >> create a vote value to be sent to the appropriate Bus Control >> Modules (BCMs) declared in the GPU info struct. >> >> This vote value is called IB, while on the othe side the GMU also >> takes another vote called AB which is a 16bit quantized value >> of the bandwidth against the maximum supported bandwidth. >> >> The vote array will then be used to dynamically generate the GMU >> bw_table sent during the GMU power-up. >> >> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> >> --- >> drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 174 ++++++++++++++++++++++++++++++++++ >> drivers/gpu/drm/msm/adreno/a6xx_gmu.h | 14 +++ >> drivers/gpu/drm/msm/adreno/a6xx_gpu.h | 1 + >> drivers/gpu/drm/msm/adreno/a6xx_hfi.h | 5 + >> 4 files changed, 194 insertions(+) >> >> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c >> index 14db7376c712d19446b38152e480bd5a1e0a5198..ee2010a01186721dd377f1655fcf05ddaff77131 100644 >> --- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c >> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c >> @@ -9,6 +9,7 @@ >> #include <linux/pm_domain.h> >> #include <linux/pm_opp.h> >> #include <soc/qcom/cmd-db.h> >> +#include <soc/qcom/tcs.h> >> #include <drm/drm_gem.h> >> >> #include "a6xx_gpu.h" >> @@ -1287,6 +1288,131 @@ static int a6xx_gmu_memory_probe(struct a6xx_gmu *gmu) >> return 0; >> } >> >> +/** >> + * struct bcm_db - Auxiliary data pertaining to each Bus Clock Manager (BCM) >> + * @unit: divisor used to convert bytes/sec bw value to an RPMh msg >> + * @width: multiplier used to convert bytes/sec bw value to an RPMh msg >> + * @vcd: virtual clock domain that this bcm belongs to >> + * @reserved: reserved field >> + */ >> +struct bcm_db { >> + __le32 unit; >> + __le16 width; >> + u8 vcd; >> + u8 reserved; >> +}; >> + >> +static u64 bcm_div(u64 num, u32 base) >> +{ >> + /* Ensure that small votes aren't lost. */ >> + if (num && num < base) >> + return 1; >> + >> + do_div(num, base); >> + >> + return num; >> +} > > This should live in include/soc/qcom/bcm.h, similarly to tcs.h in > that directory Honestly, I don't think so, there's no bcm specific logic here, we simply avoid returning 0 after a division > >> +static int a6xx_gmu_rpmh_bw_votes_init(const struct a6xx_info *info, >> + struct a6xx_gmu *gmu) >> +{ >> + const struct bcm_db *bcm_data[GMU_MAX_BCMS] = { 0 }; >> + unsigned int bcm_index, bw_index, bcm_count = 0; >> + >> + if (!info->bcms) >> + return 0; >> + >> + /* Retrieve BCM data from cmd-db */ >> + for (bcm_index = 0; bcm_index < GMU_MAX_BCMS; bcm_index++) { >> + size_t count; >> + >> + /* Stop at first unconfigured bcm */ >> + if (!info->bcms[bcm_index].name) >> + break; >> + >> + bcm_data[bcm_index] = cmd_db_read_aux_data( >> + info->bcms[bcm_index].name, >> + &count); >> + if (IS_ERR(bcm_data[bcm_index])) >> + return PTR_ERR(bcm_data[bcm_index]); >> + >> + if (!count) >> + return -EINVAL; >> + >> + ++bcm_count; >> + } >> + >> + /* Generate BCM votes values for each bandwidth & BCM */ >> + for (bw_index = 0; bw_index < gmu->nr_gpu_bws; bw_index++) { >> + u32 *data = gmu->gpu_ib_votes[bw_index]; >> + u32 bw = gmu->gpu_bw_table[bw_index]; >> + >> + /* Calculations loosely copied from bcm_aggregate() & tcs_cmd_gen() */ > > Ditto, perhaps this should be exported from icc I think it's a bad idea to share code because the overall structures and purposes are completely different, and it will make the gpu maintenance a nightmare. > > [...] > > Konrad Thanks, Neil
On 29/11/2024 16:25, Konrad Dybcio wrote: > On 28.11.2024 11:25 AM, Neil Armstrong wrote: >> Now all the DDR bandwidth voting via the GPU Management Unit (GMU) >> is in place, declare the Bus Control Modules (BCMs) and the >> corresponding parameters in the GPU info struct and add the >> GMU_BW_VOTE feature bit to enable it. >> >> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> >> --- >> drivers/gpu/drm/msm/adreno/a6xx_catalog.c | 22 ++++++++++++++++++++++ >> 1 file changed, 22 insertions(+) >> >> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_catalog.c b/drivers/gpu/drm/msm/adreno/a6xx_catalog.c >> index 0c560e84ad5a53bb4e8a49ba4e153ce9cf33f7ae..edffb7737a97b268bb2986d557969e651988a344 100644 >> --- a/drivers/gpu/drm/msm/adreno/a6xx_catalog.c >> +++ b/drivers/gpu/drm/msm/adreno/a6xx_catalog.c >> @@ -1388,6 +1388,17 @@ static const struct adreno_info a7xx_gpus[] = { >> .pwrup_reglist = &a7xx_pwrup_reglist, >> .gmu_chipid = 0x7020100, >> .gmu_cgc_mode = 0x00020202, >> + .bcms = (const struct a6xx_bcm[]) { >> + { .name = "SH0", .buswidth = 16 }, >> + { .name = "MC0", .buswidth = 4 }, >> + { >> + .name = "ACV", >> + .fixed = true, >> + .perfmode = BIT(3), >> + .perfmode_bw = 16500000, >> + }, >> + { /* sentinel */ }, >> + }, > > This is not going to fly the second there's two SoCs implementing the > same GPU with a difference in bus topology. I think we could add > something like drvdata to ICC nodes and use it for BCMs on icc-rpmh. > Then, we could retrieve it from the interconnect path we get from the > dt node. It would also reduce duplication. I don't want to go into that, we can optimize this when adding topologies for other GPUs later, as-is this is a pointer so we can already share the same table between GPUs. > > Konrad
The Adreno GPU Management Unit (GMU) can also vote for DDR Bandwidth along the Frequency and Power Domain level, but by default we leave the OPP core scale the interconnect ddr path. While scaling the interconnect path was sufficient, newer GPUs like the A750 requires specific vote parameters and bandwidth to achieve full functionnality. In order to get the vote values to be used by the GPU Management Unit (GMU), we need to parse all the possible OPP Bandwidths and create a vote value to be send to the appropriate Bus Control Modules (BCMs) declared in the GPU info struct. The added dev_pm_opp_get_bw() is used in this case. The vote array will then be used to dynamically generate the GMU bw_table sent during the GMU power-up. Those entries will then be used by passing the appropriate bandwidth level when voting for a GPU frequency. This will make sure all resources are equally voted for a same OPP, whatever decision is done by the GMU, it will ensure all resources votes are synchronized. Depends on [1] to avoid crashing when getting OPP bandwidths. [1] https://lore.kernel.org/all/20241128-topic-opp-fix-assert-index-check-v1-0-cb8bd4c0370e@linaro.org/ Ran full vulkan-cts-1.3.7.3-0-gd71a36db16d98313c431829432a136dbda692a08 with mesa 25.0.0+git3ecf2a0518 on: - QRD8550 - QRD8650 - HDK8650 Patchset is based on current msm-next including preemption support. Any feedback is welcome. Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> --- Changes in v3: - I didn't take Dmitry's review tags since I significantly changed the patches - Dropped applied OPP change - Dropped QUIRK/FEATURE addition/rename in favor of checking the a6xx_info->bcms pointer - Switch a6xx_info->bcms to a pointer, so it can be easy to share the table - Generate AB votes in advance, the voting was wrong in v2 we need to quantitiwe each bandwidth value - Do not vote via GMU is there's only the OFF vote because DT doesn't have the right properties - Added defines for the a6xx_gmu freqs tables to not have magic 16 and 4 values - Renamed gpu_bw_votes to gpu_ib_votes to match the downstream naming - Changed the parameters of a6xx_hfi_set_freq() to u32 to match the data type we pass - Drop "request for maximum bus bandwidth usage" and merge it in previous changes - Link to v2: https://lore.kernel.org/r/20241119-topic-sm8x50-gpu-bw-vote-v2-0-4deb87be2498@linaro.org Changes in v2: - opp: rename to dev_pm_opp_get_bw, fix commit message and kerneldoc - remove quirks that are features and move them to a dedicated .features bitfield - get icc bcm kerneldoc, and simplify/cleanup a6xx_gmu_rpmh_bw_votes_init() - no more copies of data - take calculations from icc-rpmh/bcm-voter - move into a single cleaner function - fix a6xx_gmu_set_freq() but not calling dev_pm_opp_set_opp() if !bw_index - also vote for maximum bus bandwidth usage (AB) - overall fix typos in commit messages - Link to v1: https://lore.kernel.org/r/20241113-topic-sm8x50-gpu-bw-vote-v1-0-3b8d39737a9b@linaro.org --- Neil Armstrong (7): drm/msm: adreno: add defines for gpu & gmu frequency table sizes drm/msm: adreno: add plumbing to generate bandwidth vote table for GMU drm/msm: adreno: dynamically generate GMU bw table drm/msm: adreno: find bandwidth index of OPP and set it along freq index drm/msm: adreno: enable GMU bandwidth for A740 and A750 arm64: qcom: dts: sm8550: add interconnect and opp-peak-kBps for GPU arm64: qcom: dts: sm8650: add interconnect and opp-peak-kBps for GPU arch/arm64/boot/dts/qcom/sm8550.dtsi | 11 ++ arch/arm64/boot/dts/qcom/sm8650.dtsi | 14 +++ drivers/gpu/drm/msm/adreno/a6xx_catalog.c | 22 ++++ drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 197 +++++++++++++++++++++++++++++- drivers/gpu/drm/msm/adreno/a6xx_gmu.h | 27 +++- drivers/gpu/drm/msm/adreno/a6xx_gpu.h | 1 + drivers/gpu/drm/msm/adreno/a6xx_hfi.c | 45 ++++++- drivers/gpu/drm/msm/adreno/a6xx_hfi.h | 5 + 8 files changed, 309 insertions(+), 13 deletions(-) --- base-commit: 18ac96e1bd761af2b7c2fc99901e9a813a6f3bb3 change-id: 20241113-topic-sm8x50-gpu-bw-vote-f5e022fe7a47 Best regards,