| Message ID | 20241113-topic-sm8x50-gpu-bw-vote-v1-6-3b8d39737a9b@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | drm/msm: adreno: add support for DDR bandwidth scaling via GMU | expand |
On Wed, Nov 13, 2024 at 04:48:32PM +0100, Neil Armstrong wrote: > Now all the DDR bandwidth voting via the GPU Management Unit (GMU) > is in place, let's declare the Bus Control Modules (BCMs) and s/let's //g > it's parameters in the GPU info struct and add the GMU_BW_VOTE > quirk to enable it. Can we define a function that checks for info.bcm[0].name isntead of adding a quirk? > > Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> > --- > drivers/gpu/drm/msm/adreno/a6xx_catalog.c | 26 ++++++++++++++++++++++++-- > 1 file changed, 24 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/msm/adreno/a6xx_catalog.c b/drivers/gpu/drm/msm/adreno/a6xx_catalog.c > index 0c560e84ad5a53bb4e8a49ba4e153ce9cf33f7ae..014a24256b832d8e03fe06a6516b5348a5c0474a 100644 > --- a/drivers/gpu/drm/msm/adreno/a6xx_catalog.c > +++ b/drivers/gpu/drm/msm/adreno/a6xx_catalog.c > @@ -1379,7 +1379,8 @@ static const struct adreno_info a7xx_gpus[] = { > .inactive_period = DRM_MSM_INACTIVE_PERIOD, > .quirks = ADRENO_QUIRK_HAS_CACHED_COHERENT | > ADRENO_QUIRK_HAS_HW_APRIV | > - ADRENO_QUIRK_PREEMPTION, > + ADRENO_QUIRK_PREEMPTION | > + ADRENO_QUIRK_GMU_BW_VOTE, > .init = a6xx_gpu_init, > .zapfw = "a740_zap.mdt", > .a6xx = &(const struct a6xx_info) { > @@ -1388,6 +1389,16 @@ static const struct adreno_info a7xx_gpus[] = { > .pwrup_reglist = &a7xx_pwrup_reglist, > .gmu_chipid = 0x7020100, > .gmu_cgc_mode = 0x00020202, > + .bcm = { > + [0] = { .name = "SH0", .buswidth = 16 }, > + [1] = { .name = "MC0", .buswidth = 4 }, > + [2] = { > + .name = "ACV", > + .fixed = true, > + .perfmode = BIT(3), > + .perfmode_bw = 16500000, Is it a platform property or GPU / GMU property? Can expect that there might be several SoCs having the same GPU, but different perfmode_bw entry? > + }, > + }, > }, > .address_space_size = SZ_16G, > .preempt_record_size = 4192 * SZ_1K, > @@ -1424,7 +1435,8 @@ static const struct adreno_info a7xx_gpus[] = { > .inactive_period = DRM_MSM_INACTIVE_PERIOD, > .quirks = ADRENO_QUIRK_HAS_CACHED_COHERENT | > ADRENO_QUIRK_HAS_HW_APRIV | > - ADRENO_QUIRK_PREEMPTION, > + ADRENO_QUIRK_PREEMPTION | > + ADRENO_QUIRK_GMU_BW_VOTE, > .init = a6xx_gpu_init, > .zapfw = "gen70900_zap.mbn", > .a6xx = &(const struct a6xx_info) { > @@ -1432,6 +1444,16 @@ static const struct adreno_info a7xx_gpus[] = { > .pwrup_reglist = &a7xx_pwrup_reglist, > .gmu_chipid = 0x7090100, > .gmu_cgc_mode = 0x00020202, > + .bcm = { > + [0] = { .name = "SH0", .buswidth = 16 }, > + [1] = { .name = "MC0", .buswidth = 4 }, > + [2] = { > + .name = "ACV", > + .fixed = true, > + .perfmode = BIT(2), > + .perfmode_bw = 10687500, > + }, > + }, > }, > .address_space_size = SZ_16G, > .preempt_record_size = 3572 * SZ_1K, > > -- > 2.34.1 >
On 15/11/2024 08:33, Dmitry Baryshkov wrote: > On Wed, Nov 13, 2024 at 04:48:32PM +0100, Neil Armstrong wrote: >> Now all the DDR bandwidth voting via the GPU Management Unit (GMU) >> is in place, let's declare the Bus Control Modules (BCMs) and > > s/let's //g > >> it's parameters in the GPU info struct and add the GMU_BW_VOTE >> quirk to enable it. > > Can we define a function that checks for info.bcm[0].name isntead of > adding a quirk? Probably, I'll need ideas to how design this better, perhaps a simple capability bitfield in a6xx_info ? There's other feature that are lacking, like ACD or BCL which are not supported on all a6xx/a7xx gpus. > >> >> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> >> --- >> drivers/gpu/drm/msm/adreno/a6xx_catalog.c | 26 ++++++++++++++++++++++++-- >> 1 file changed, 24 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_catalog.c b/drivers/gpu/drm/msm/adreno/a6xx_catalog.c >> index 0c560e84ad5a53bb4e8a49ba4e153ce9cf33f7ae..014a24256b832d8e03fe06a6516b5348a5c0474a 100644 >> --- a/drivers/gpu/drm/msm/adreno/a6xx_catalog.c >> +++ b/drivers/gpu/drm/msm/adreno/a6xx_catalog.c >> @@ -1379,7 +1379,8 @@ static const struct adreno_info a7xx_gpus[] = { >> .inactive_period = DRM_MSM_INACTIVE_PERIOD, >> .quirks = ADRENO_QUIRK_HAS_CACHED_COHERENT | >> ADRENO_QUIRK_HAS_HW_APRIV | >> - ADRENO_QUIRK_PREEMPTION, >> + ADRENO_QUIRK_PREEMPTION | >> + ADRENO_QUIRK_GMU_BW_VOTE, >> .init = a6xx_gpu_init, >> .zapfw = "a740_zap.mdt", >> .a6xx = &(const struct a6xx_info) { >> @@ -1388,6 +1389,16 @@ static const struct adreno_info a7xx_gpus[] = { >> .pwrup_reglist = &a7xx_pwrup_reglist, >> .gmu_chipid = 0x7020100, >> .gmu_cgc_mode = 0x00020202, >> + .bcm = { >> + [0] = { .name = "SH0", .buswidth = 16 }, >> + [1] = { .name = "MC0", .buswidth = 4 }, >> + [2] = { >> + .name = "ACV", >> + .fixed = true, >> + .perfmode = BIT(3), >> + .perfmode_bw = 16500000, > > Is it a platform property or GPU / GMU property? Can expect that there > might be several SoCs having the same GPU, but different perfmode_bw > entry? I presume this is SoC specific ? But today the XXX_build_bw_table() are already SoC specific, so where should this go ? Downstream specifies this in the adreno-gpulist.h, which is the equivalent here. Neil > >> + }, >> + }, >> }, >> .address_space_size = SZ_16G, >> .preempt_record_size = 4192 * SZ_1K, >> @@ -1424,7 +1435,8 @@ static const struct adreno_info a7xx_gpus[] = { >> .inactive_period = DRM_MSM_INACTIVE_PERIOD, >> .quirks = ADRENO_QUIRK_HAS_CACHED_COHERENT | >> ADRENO_QUIRK_HAS_HW_APRIV | >> - ADRENO_QUIRK_PREEMPTION, >> + ADRENO_QUIRK_PREEMPTION | >> + ADRENO_QUIRK_GMU_BW_VOTE, >> .init = a6xx_gpu_init, >> .zapfw = "gen70900_zap.mbn", >> .a6xx = &(const struct a6xx_info) { >> @@ -1432,6 +1444,16 @@ static const struct adreno_info a7xx_gpus[] = { >> .pwrup_reglist = &a7xx_pwrup_reglist, >> .gmu_chipid = 0x7090100, >> .gmu_cgc_mode = 0x00020202, >> + .bcm = { >> + [0] = { .name = "SH0", .buswidth = 16 }, >> + [1] = { .name = "MC0", .buswidth = 4 }, >> + [2] = { >> + .name = "ACV", >> + .fixed = true, >> + .perfmode = BIT(2), >> + .perfmode_bw = 10687500, >> + }, >> + }, >> }, >> .address_space_size = SZ_16G, >> .preempt_record_size = 3572 * SZ_1K, >> >> -- >> 2.34.1 >> >
On Fri, Nov 15, 2024 at 10:20:01AM +0100, Neil Armstrong wrote: > On 15/11/2024 08:33, Dmitry Baryshkov wrote: > > On Wed, Nov 13, 2024 at 04:48:32PM +0100, Neil Armstrong wrote: > > > Now all the DDR bandwidth voting via the GPU Management Unit (GMU) > > > is in place, let's declare the Bus Control Modules (BCMs) and > > > > s/let's //g > > > > > it's parameters in the GPU info struct and add the GMU_BW_VOTE > > > quirk to enable it. > > > > Can we define a function that checks for info.bcm[0].name isntead of > > adding a quirk? > > Probably, I'll need ideas to how design this better, perhaps a simple > capability bitfield in a6xx_info ? I'm not sure if I follow the question. I think it's better to check for the presens of the data rather than having a separate 'cap' bit in addition to that data. > There's other feature that are lacking, like ACD or BCL which are not supported > on all a6xx/a7xx gpus. Akhil is currently working on ACD, as you have seen from the patches. > > > > > > > > > Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> > > > --- > > > drivers/gpu/drm/msm/adreno/a6xx_catalog.c | 26 ++++++++++++++++++++++++-- > > > 1 file changed, 24 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/msm/adreno/a6xx_catalog.c b/drivers/gpu/drm/msm/adreno/a6xx_catalog.c > > > index 0c560e84ad5a53bb4e8a49ba4e153ce9cf33f7ae..014a24256b832d8e03fe06a6516b5348a5c0474a 100644 > > > --- a/drivers/gpu/drm/msm/adreno/a6xx_catalog.c > > > +++ b/drivers/gpu/drm/msm/adreno/a6xx_catalog.c > > > @@ -1379,7 +1379,8 @@ static const struct adreno_info a7xx_gpus[] = { > > > .inactive_period = DRM_MSM_INACTIVE_PERIOD, > > > .quirks = ADRENO_QUIRK_HAS_CACHED_COHERENT | > > > ADRENO_QUIRK_HAS_HW_APRIV | > > > - ADRENO_QUIRK_PREEMPTION, > > > + ADRENO_QUIRK_PREEMPTION | > > > + ADRENO_QUIRK_GMU_BW_VOTE, > > > .init = a6xx_gpu_init, > > > .zapfw = "a740_zap.mdt", > > > .a6xx = &(const struct a6xx_info) { > > > @@ -1388,6 +1389,16 @@ static const struct adreno_info a7xx_gpus[] = { > > > .pwrup_reglist = &a7xx_pwrup_reglist, > > > .gmu_chipid = 0x7020100, > > > .gmu_cgc_mode = 0x00020202, > > > + .bcm = { > > > + [0] = { .name = "SH0", .buswidth = 16 }, > > > + [1] = { .name = "MC0", .buswidth = 4 }, > > > + [2] = { > > > + .name = "ACV", > > > + .fixed = true, > > > + .perfmode = BIT(3), > > > + .perfmode_bw = 16500000, > > > > Is it a platform property or GPU / GMU property? Can expect that there > > might be several SoCs having the same GPU, but different perfmode_bw > > entry? > > I presume this is SoC specific ? But today the XXX_build_bw_table() are > already SoC specific, so where should this go ? XXX_build_bw_table() are GPU-specific. There are cases of several SoCs sharing the same GPU on them. > Downstream specifies this in the adreno-gpulist.h, which is the equivalent > here.
On 15/11/2024 15:39, Dmitry Baryshkov wrote: > On Fri, Nov 15, 2024 at 10:20:01AM +0100, Neil Armstrong wrote: >> On 15/11/2024 08:33, Dmitry Baryshkov wrote: >>> On Wed, Nov 13, 2024 at 04:48:32PM +0100, Neil Armstrong wrote: >>>> Now all the DDR bandwidth voting via the GPU Management Unit (GMU) >>>> is in place, let's declare the Bus Control Modules (BCMs) and >>> >>> s/let's //g >>> >>>> it's parameters in the GPU info struct and add the GMU_BW_VOTE >>>> quirk to enable it. >>> >>> Can we define a function that checks for info.bcm[0].name isntead of >>> adding a quirk? >> >> Probably, I'll need ideas to how design this better, perhaps a simple >> capability bitfield in a6xx_info ? > > I'm not sure if I follow the question. I think it's better to check for > the presens of the data rather than having a separate 'cap' bit in > addition to that data. I don't fully agree here, I just follow the other features (CACHED_COHERENT/APRIV/...) nothing fancy. I'll introduce a features bitfield, so we don't mix them with quirks > >> There's other feature that are lacking, like ACD or BCL which are not supported >> on all a6xx/a7xx gpus. > > Akhil is currently working on ACD, as you have seen from the patches. Yep I've tested and reviewed the patches > >> >>> >>>> >>>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> >>>> --- >>>> drivers/gpu/drm/msm/adreno/a6xx_catalog.c | 26 ++++++++++++++++++++++++-- >>>> 1 file changed, 24 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_catalog.c b/drivers/gpu/drm/msm/adreno/a6xx_catalog.c >>>> index 0c560e84ad5a53bb4e8a49ba4e153ce9cf33f7ae..014a24256b832d8e03fe06a6516b5348a5c0474a 100644 >>>> --- a/drivers/gpu/drm/msm/adreno/a6xx_catalog.c >>>> +++ b/drivers/gpu/drm/msm/adreno/a6xx_catalog.c >>>> @@ -1379,7 +1379,8 @@ static const struct adreno_info a7xx_gpus[] = { >>>> .inactive_period = DRM_MSM_INACTIVE_PERIOD, >>>> .quirks = ADRENO_QUIRK_HAS_CACHED_COHERENT | >>>> ADRENO_QUIRK_HAS_HW_APRIV | >>>> - ADRENO_QUIRK_PREEMPTION, >>>> + ADRENO_QUIRK_PREEMPTION | >>>> + ADRENO_QUIRK_GMU_BW_VOTE, >>>> .init = a6xx_gpu_init, >>>> .zapfw = "a740_zap.mdt", >>>> .a6xx = &(const struct a6xx_info) { >>>> @@ -1388,6 +1389,16 @@ static const struct adreno_info a7xx_gpus[] = { >>>> .pwrup_reglist = &a7xx_pwrup_reglist, >>>> .gmu_chipid = 0x7020100, >>>> .gmu_cgc_mode = 0x00020202, >>>> + .bcm = { >>>> + [0] = { .name = "SH0", .buswidth = 16 }, >>>> + [1] = { .name = "MC0", .buswidth = 4 }, >>>> + [2] = { >>>> + .name = "ACV", >>>> + .fixed = true, >>>> + .perfmode = BIT(3), >>>> + .perfmode_bw = 16500000, >>> >>> Is it a platform property or GPU / GMU property? Can expect that there >>> might be several SoCs having the same GPU, but different perfmode_bw >>> entry? >> >> I presume this is SoC specific ? But today the XXX_build_bw_table() are >> already SoC specific, so where should this go ? > > XXX_build_bw_table() are GPU-specific. There are cases of several SoCs > sharing the same GPU on them. So it's gpu-specific > >> Downstream specifies this in the adreno-gpulist.h, which is the equivalent >> here. >
On Mon, 18 Nov 2024 at 15:43, Neil Armstrong <neil.armstrong@linaro.org> wrote: > > On 15/11/2024 15:39, Dmitry Baryshkov wrote: > > On Fri, Nov 15, 2024 at 10:20:01AM +0100, Neil Armstrong wrote: > >> On 15/11/2024 08:33, Dmitry Baryshkov wrote: > >>> On Wed, Nov 13, 2024 at 04:48:32PM +0100, Neil Armstrong wrote: > >>>> Now all the DDR bandwidth voting via the GPU Management Unit (GMU) > >>>> is in place, let's declare the Bus Control Modules (BCMs) and > >>> > >>> s/let's //g > >>> > >>>> it's parameters in the GPU info struct and add the GMU_BW_VOTE > >>>> quirk to enable it. > >>> > >>> Can we define a function that checks for info.bcm[0].name isntead of > >>> adding a quirk? > >> > >> Probably, I'll need ideas to how design this better, perhaps a simple > >> capability bitfield in a6xx_info ? > > > > I'm not sure if I follow the question. I think it's better to check for > > the presens of the data rather than having a separate 'cap' bit in > > addition to that data. > > I don't fully agree here, I just follow the other features (CACHED_COHERENT/APRIV/...) > nothing fancy. > I'll introduce a features bitfield, so we don't mix them with quirks SGTM > > > > >> There's other feature that are lacking, like ACD or BCL which are not supported > >> on all a6xx/a7xx gpus. > > > > Akhil is currently working on ACD, as you have seen from the patches. > > Yep I've tested and reviewed the patches > > > > >> > >>> > >>>> > >>>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> > >>>> --- > >>>> drivers/gpu/drm/msm/adreno/a6xx_catalog.c | 26 ++++++++++++++++++++++++-- > >>>> 1 file changed, 24 insertions(+), 2 deletions(-) > >>>> > >>>> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_catalog.c b/drivers/gpu/drm/msm/adreno/a6xx_catalog.c > >>>> index 0c560e84ad5a53bb4e8a49ba4e153ce9cf33f7ae..014a24256b832d8e03fe06a6516b5348a5c0474a 100644 > >>>> --- a/drivers/gpu/drm/msm/adreno/a6xx_catalog.c > >>>> +++ b/drivers/gpu/drm/msm/adreno/a6xx_catalog.c > >>>> @@ -1379,7 +1379,8 @@ static const struct adreno_info a7xx_gpus[] = { > >>>> .inactive_period = DRM_MSM_INACTIVE_PERIOD, > >>>> .quirks = ADRENO_QUIRK_HAS_CACHED_COHERENT | > >>>> ADRENO_QUIRK_HAS_HW_APRIV | > >>>> - ADRENO_QUIRK_PREEMPTION, > >>>> + ADRENO_QUIRK_PREEMPTION | > >>>> + ADRENO_QUIRK_GMU_BW_VOTE, > >>>> .init = a6xx_gpu_init, > >>>> .zapfw = "a740_zap.mdt", > >>>> .a6xx = &(const struct a6xx_info) { > >>>> @@ -1388,6 +1389,16 @@ static const struct adreno_info a7xx_gpus[] = { > >>>> .pwrup_reglist = &a7xx_pwrup_reglist, > >>>> .gmu_chipid = 0x7020100, > >>>> .gmu_cgc_mode = 0x00020202, > >>>> + .bcm = { > >>>> + [0] = { .name = "SH0", .buswidth = 16 }, > >>>> + [1] = { .name = "MC0", .buswidth = 4 }, > >>>> + [2] = { > >>>> + .name = "ACV", > >>>> + .fixed = true, > >>>> + .perfmode = BIT(3), > >>>> + .perfmode_bw = 16500000, > >>> > >>> Is it a platform property or GPU / GMU property? Can expect that there > >>> might be several SoCs having the same GPU, but different perfmode_bw > >>> entry? > >> > >> I presume this is SoC specific ? But today the XXX_build_bw_table() are > >> already SoC specific, so where should this go ? > > > > XXX_build_bw_table() are GPU-specific. There are cases of several SoCs > > sharing the same GPU on them. > > So it's gpu-specific > > > > >> Downstream specifies this in the adreno-gpulist.h, which is the equivalent > >> here. > > >
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_catalog.c b/drivers/gpu/drm/msm/adreno/a6xx_catalog.c index 0c560e84ad5a53bb4e8a49ba4e153ce9cf33f7ae..014a24256b832d8e03fe06a6516b5348a5c0474a 100644 --- a/drivers/gpu/drm/msm/adreno/a6xx_catalog.c +++ b/drivers/gpu/drm/msm/adreno/a6xx_catalog.c @@ -1379,7 +1379,8 @@ static const struct adreno_info a7xx_gpus[] = { .inactive_period = DRM_MSM_INACTIVE_PERIOD, .quirks = ADRENO_QUIRK_HAS_CACHED_COHERENT | ADRENO_QUIRK_HAS_HW_APRIV | - ADRENO_QUIRK_PREEMPTION, + ADRENO_QUIRK_PREEMPTION | + ADRENO_QUIRK_GMU_BW_VOTE, .init = a6xx_gpu_init, .zapfw = "a740_zap.mdt", .a6xx = &(const struct a6xx_info) { @@ -1388,6 +1389,16 @@ static const struct adreno_info a7xx_gpus[] = { .pwrup_reglist = &a7xx_pwrup_reglist, .gmu_chipid = 0x7020100, .gmu_cgc_mode = 0x00020202, + .bcm = { + [0] = { .name = "SH0", .buswidth = 16 }, + [1] = { .name = "MC0", .buswidth = 4 }, + [2] = { + .name = "ACV", + .fixed = true, + .perfmode = BIT(3), + .perfmode_bw = 16500000, + }, + }, }, .address_space_size = SZ_16G, .preempt_record_size = 4192 * SZ_1K, @@ -1424,7 +1435,8 @@ static const struct adreno_info a7xx_gpus[] = { .inactive_period = DRM_MSM_INACTIVE_PERIOD, .quirks = ADRENO_QUIRK_HAS_CACHED_COHERENT | ADRENO_QUIRK_HAS_HW_APRIV | - ADRENO_QUIRK_PREEMPTION, + ADRENO_QUIRK_PREEMPTION | + ADRENO_QUIRK_GMU_BW_VOTE, .init = a6xx_gpu_init, .zapfw = "gen70900_zap.mbn", .a6xx = &(const struct a6xx_info) { @@ -1432,6 +1444,16 @@ static const struct adreno_info a7xx_gpus[] = { .pwrup_reglist = &a7xx_pwrup_reglist, .gmu_chipid = 0x7090100, .gmu_cgc_mode = 0x00020202, + .bcm = { + [0] = { .name = "SH0", .buswidth = 16 }, + [1] = { .name = "MC0", .buswidth = 4 }, + [2] = { + .name = "ACV", + .fixed = true, + .perfmode = BIT(2), + .perfmode_bw = 10687500, + }, + }, }, .address_space_size = SZ_16G, .preempt_record_size = 3572 * SZ_1K,
Now all the DDR bandwidth voting via the GPU Management Unit (GMU) is in place, let's declare the Bus Control Modules (BCMs) and it's parameters in the GPU info struct and add the GMU_BW_VOTE quirk to enable it. Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> --- drivers/gpu/drm/msm/adreno/a6xx_catalog.c | 26 ++++++++++++++++++++++++-- 1 file changed, 24 insertions(+), 2 deletions(-)