| Message ID | 1589453659-27581-6-git-send-email-smasetty@codeaurora.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add support for GPU DDR BW scaling | expand |
On Thu, May 14, 2020 at 04:24:18PM +0530, Sharat Masetty wrote: > This patches replaces the previously used static DDR vote and uses > dev_pm_opp_set_bw() to scale GPU->DDR bandwidth along with scaling > GPU frequency. > > Signed-off-by: Sharat Masetty <smasetty@codeaurora.org> > --- > drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 6 +----- > 1 file changed, 1 insertion(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c > index 2d8124b..79433d3 100644 > --- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c > +++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c > @@ -141,11 +141,7 @@ void a6xx_gmu_set_freq(struct msm_gpu *gpu, struct dev_pm_opp *opp) > > gmu->freq = gmu->gpu_freqs[perf_index]; > > - /* > - * Eventually we will want to scale the path vote with the frequency but > - * for now leave it at max so that the performance is nominal. > - */ > - icc_set_bw(gpu->icc_path, 0, MBps_to_icc(7216)); > + dev_pm_opp_set_bw(&gpu->pdev->dev, opp); Is there a particular reason to keep this one liner in a separate patch? I think it would make sense to squash it into "drm: msm: a6xx: send opp instead of a frequency" and change the subject of the combined patch to something like "drm: msm: a6xx: Scale the DDR bandwidth dynamically".
On Thu, May 14, 2020 at 04:24:18PM +0530, Sharat Masetty wrote: > This patches replaces the previously used static DDR vote and uses > dev_pm_opp_set_bw() to scale GPU->DDR bandwidth along with scaling > GPU frequency. > > Signed-off-by: Sharat Masetty <smasetty@codeaurora.org> > --- > drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 6 +----- > 1 file changed, 1 insertion(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c > index 2d8124b..79433d3 100644 > --- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c > +++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c > @@ -141,11 +141,7 @@ void a6xx_gmu_set_freq(struct msm_gpu *gpu, struct dev_pm_opp *opp) > > gmu->freq = gmu->gpu_freqs[perf_index]; > > - /* > - * Eventually we will want to scale the path vote with the frequency but > - * for now leave it at max so that the performance is nominal. > - */ > - icc_set_bw(gpu->icc_path, 0, MBps_to_icc(7216)); > + dev_pm_opp_set_bw(&gpu->pdev->dev, opp); > } This adds an implicit requirement that all targets need bandwidth settings defined in the OPP or they won't get a bus vote at all. I would prefer that there be an default escape valve but if not you'll need to add bandwidth values for the sdm845 OPP that target doesn't regress. Jordan > unsigned long a6xx_gmu_get_freq(struct msm_gpu *gpu) > -- > 2.7.4 >
On Mon, May 18, 2020 at 7:23 AM Jordan Crouse <jcrouse@codeaurora.org> wrote: > > On Thu, May 14, 2020 at 04:24:18PM +0530, Sharat Masetty wrote: > > This patches replaces the previously used static DDR vote and uses > > dev_pm_opp_set_bw() to scale GPU->DDR bandwidth along with scaling > > GPU frequency. > > > > Signed-off-by: Sharat Masetty <smasetty@codeaurora.org> > > --- > > drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 6 +----- > > 1 file changed, 1 insertion(+), 5 deletions(-) > > > > diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c > > index 2d8124b..79433d3 100644 > > --- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c > > +++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c > > @@ -141,11 +141,7 @@ void a6xx_gmu_set_freq(struct msm_gpu *gpu, struct dev_pm_opp *opp) > > > > gmu->freq = gmu->gpu_freqs[perf_index]; > > > > - /* > > - * Eventually we will want to scale the path vote with the frequency but > > - * for now leave it at max so that the performance is nominal. > > - */ > > - icc_set_bw(gpu->icc_path, 0, MBps_to_icc(7216)); > > + dev_pm_opp_set_bw(&gpu->pdev->dev, opp); > > } > > This adds an implicit requirement that all targets need bandwidth settings > defined in the OPP or they won't get a bus vote at all. I would prefer that > there be an default escape valve but if not you'll need to add > bandwidth values for the sdm845 OPP that target doesn't regress. > it looks like we could maybe do something like: ret = dev_pm_opp_set_bw(...); if (ret) { dev_warn_once(dev, "no bandwidth settings"); icc_set_bw(...); } ? BR, -R > Jordan > > > unsigned long a6xx_gmu_get_freq(struct msm_gpu *gpu) > > -- > > 2.7.4 > > > > -- > The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, > a Linux Foundation Collaborative Project > _______________________________________________ > Freedreno mailing list > Freedreno@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/freedreno
+ more folks On 5/18/2020 9:55 PM, Rob Clark wrote: > On Mon, May 18, 2020 at 7:23 AM Jordan Crouse <jcrouse@codeaurora.org> wrote: >> On Thu, May 14, 2020 at 04:24:18PM +0530, Sharat Masetty wrote: >>> This patches replaces the previously used static DDR vote and uses >>> dev_pm_opp_set_bw() to scale GPU->DDR bandwidth along with scaling >>> GPU frequency. >>> >>> Signed-off-by: Sharat Masetty <smasetty@codeaurora.org> >>> --- >>> drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 6 +----- >>> 1 file changed, 1 insertion(+), 5 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c >>> index 2d8124b..79433d3 100644 >>> --- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c >>> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c >>> @@ -141,11 +141,7 @@ void a6xx_gmu_set_freq(struct msm_gpu *gpu, struct dev_pm_opp *opp) >>> >>> gmu->freq = gmu->gpu_freqs[perf_index]; >>> >>> - /* >>> - * Eventually we will want to scale the path vote with the frequency but >>> - * for now leave it at max so that the performance is nominal. >>> - */ >>> - icc_set_bw(gpu->icc_path, 0, MBps_to_icc(7216)); >>> + dev_pm_opp_set_bw(&gpu->pdev->dev, opp); >>> } >> This adds an implicit requirement that all targets need bandwidth settings >> defined in the OPP or they won't get a bus vote at all. I would prefer that >> there be an default escape valve but if not you'll need to add >> bandwidth values for the sdm845 OPP that target doesn't regress. >> > it looks like we could maybe do something like: > > ret = dev_pm_opp_set_bw(...); > if (ret) { > dev_warn_once(dev, "no bandwidth settings"); > icc_set_bw(...); > } > > ? > > BR, > -R There is a bit of an issue here - Looks like its not possible to two icc handles to the same path. Its causing double enumeration of the paths in the icc core and messing up path votes. With [1] Since opp/core already gets a handle to the icc path as part of table add, drm/msm could do either a) Conditionally enumerate gpu->icc_path handle only when pm/opp core has not got the icc path handle. I could use something like [2] to determine if should initialize gpu->icc_path* b) Add peak-opp-configs in 845 dt and mandate all future versions to use this bindings. With this, I can remove gpu->icc_path from msm/drm completely and only rely on opp/core for bw voting. [1] - https://lore.kernel.org/patchwork/cover/1240687/ [2] - https://patchwork.kernel.org/patch/11527573/ Let me know your thoughts Sharat > >> Jordan >> >>> unsigned long a6xx_gmu_get_freq(struct msm_gpu *gpu) >>> -- >>> 2.7.4 >>> >> -- >> The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, >> a Linux Foundation Collaborative Project >> _______________________________________________ >> Freedreno mailing list >> Freedreno@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/freedreno
On Wed, May 27, 2020 at 1:47 AM Sharat Masetty <smasetty@codeaurora.org> wrote: > > + more folks > > On 5/18/2020 9:55 PM, Rob Clark wrote: > > On Mon, May 18, 2020 at 7:23 AM Jordan Crouse <jcrouse@codeaurora.org> wrote: > >> On Thu, May 14, 2020 at 04:24:18PM +0530, Sharat Masetty wrote: > >>> This patches replaces the previously used static DDR vote and uses > >>> dev_pm_opp_set_bw() to scale GPU->DDR bandwidth along with scaling > >>> GPU frequency. > >>> > >>> Signed-off-by: Sharat Masetty <smasetty@codeaurora.org> > >>> --- > >>> drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 6 +----- > >>> 1 file changed, 1 insertion(+), 5 deletions(-) > >>> > >>> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c > >>> index 2d8124b..79433d3 100644 > >>> --- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c > >>> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c > >>> @@ -141,11 +141,7 @@ void a6xx_gmu_set_freq(struct msm_gpu *gpu, struct dev_pm_opp *opp) > >>> > >>> gmu->freq = gmu->gpu_freqs[perf_index]; > >>> > >>> - /* > >>> - * Eventually we will want to scale the path vote with the frequency but > >>> - * for now leave it at max so that the performance is nominal. > >>> - */ > >>> - icc_set_bw(gpu->icc_path, 0, MBps_to_icc(7216)); > >>> + dev_pm_opp_set_bw(&gpu->pdev->dev, opp); > >>> } > >> This adds an implicit requirement that all targets need bandwidth settings > >> defined in the OPP or they won't get a bus vote at all. I would prefer that > >> there be an default escape valve but if not you'll need to add > >> bandwidth values for the sdm845 OPP that target doesn't regress. > >> > > it looks like we could maybe do something like: > > > > ret = dev_pm_opp_set_bw(...); > > if (ret) { > > dev_warn_once(dev, "no bandwidth settings"); > > icc_set_bw(...); > > } > > > > ? > > > > BR, > > -R > > There is a bit of an issue here - Looks like its not possible to two icc > handles to the same path. Its causing double enumeration of the paths > in the icc core and messing up path votes. With [1] Since opp/core > already gets a handle to the icc path as part of table add, drm/msm > could do either > > a) Conditionally enumerate gpu->icc_path handle only when pm/opp core > has not got the icc path handle. I could use something like [2] to > determine if should initialize gpu->icc_path* > > b) Add peak-opp-configs in 845 dt and mandate all future versions to use > this bindings. With this, I can remove gpu->icc_path from msm/drm > completely and only rely on opp/core for bw voting. The main thing is that we want to make sure newer dtb always works on an older kernel without regression.. but, hmm.. I guess the interconnects/interconnects-names properties haven't landed yet in sdm845.dtsi? Maybe that lets us go with the simpler approach (b). Looks like we haven't wired up interconnect for 8916 or 8996 either, so probably we can just mandate this for all of them? If we have landed the interconnect dts hookup for gpu somewhere that I'm overlooking, I guess we would have to go with (a) and keep the existing interconnects/interconnects-names properties. BR, -R > [1] - https://lore.kernel.org/patchwork/cover/1240687/ > > [2] - https://patchwork.kernel.org/patch/11527573/ > > Let me know your thoughts > > Sharat > > > > >> Jordan > >> > >>> unsigned long a6xx_gmu_get_freq(struct msm_gpu *gpu) > >>> -- > >>> 2.7.4 > >>> > >> -- > >> The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, > >> a Linux Foundation Collaborative Project > >> _______________________________________________ > >> Freedreno mailing list > >> Freedreno@lists.freedesktop.org > >> https://lists.freedesktop.org/mailman/listinfo/freedreno
On Wed, May 27, 2020 at 8:38 AM Rob Clark <robdclark@gmail.com> wrote: > > On Wed, May 27, 2020 at 1:47 AM Sharat Masetty <smasetty@codeaurora.org> wrote: > > > > + more folks > > > > On 5/18/2020 9:55 PM, Rob Clark wrote: > > > On Mon, May 18, 2020 at 7:23 AM Jordan Crouse <jcrouse@codeaurora.org> wrote: > > >> On Thu, May 14, 2020 at 04:24:18PM +0530, Sharat Masetty wrote: > > >>> This patches replaces the previously used static DDR vote and uses > > >>> dev_pm_opp_set_bw() to scale GPU->DDR bandwidth along with scaling > > >>> GPU frequency. > > >>> > > >>> Signed-off-by: Sharat Masetty <smasetty@codeaurora.org> > > >>> --- > > >>> drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 6 +----- > > >>> 1 file changed, 1 insertion(+), 5 deletions(-) > > >>> > > >>> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c > > >>> index 2d8124b..79433d3 100644 > > >>> --- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c > > >>> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c > > >>> @@ -141,11 +141,7 @@ void a6xx_gmu_set_freq(struct msm_gpu *gpu, struct dev_pm_opp *opp) > > >>> > > >>> gmu->freq = gmu->gpu_freqs[perf_index]; > > >>> > > >>> - /* > > >>> - * Eventually we will want to scale the path vote with the frequency but > > >>> - * for now leave it at max so that the performance is nominal. > > >>> - */ > > >>> - icc_set_bw(gpu->icc_path, 0, MBps_to_icc(7216)); > > >>> + dev_pm_opp_set_bw(&gpu->pdev->dev, opp); > > >>> } > > >> This adds an implicit requirement that all targets need bandwidth settings > > >> defined in the OPP or they won't get a bus vote at all. I would prefer that > > >> there be an default escape valve but if not you'll need to add > > >> bandwidth values for the sdm845 OPP that target doesn't regress. > > >> > > > it looks like we could maybe do something like: > > > > > > ret = dev_pm_opp_set_bw(...); > > > if (ret) { > > > dev_warn_once(dev, "no bandwidth settings"); > > > icc_set_bw(...); > > > } > > > > > > ? > > > > > > BR, > > > -R > > > > There is a bit of an issue here - Looks like its not possible to two icc > > handles to the same path. Its causing double enumeration of the paths > > in the icc core and messing up path votes. With [1] Since opp/core > > already gets a handle to the icc path as part of table add, drm/msm > > could do either Are you sure this is the real issue? I'd be surprised if this is a real limitation. And if it is, it either needs to be fixed in the ICC framework or OPP shouldn't be getting path handles by default (and maybe let the driver set the handles before using OPP APIs to change BW). I'd lean towards the former. > > a) Conditionally enumerate gpu->icc_path handle only when pm/opp core > > has not got the icc path handle. I could use something like [2] to > > determine if should initialize gpu->icc_path* This seems like a bandaid. Let's fix it correctly in ICC framework or OPP framework. > > b) Add peak-opp-configs in 845 dt and mandate all future versions to use > > this bindings. With this, I can remove gpu->icc_path from msm/drm > > completely and only rely on opp/core for bw voting. I don't know what you mean by "peak-opp-configs" but I guess you are referring to some kind of DT flag to say if you should vote for BW directly or use the OPP framework? If so, I'm pretty sure that won't fly. That's an OS implementation specific flag. -Saravana
On 2020-05-27 23:01, Saravana Kannan wrote: > On Wed, May 27, 2020 at 8:38 AM Rob Clark <robdclark@gmail.com> wrote: >> >> On Wed, May 27, 2020 at 1:47 AM Sharat Masetty >> <smasetty@codeaurora.org> wrote: >> > >> > + more folks >> > >> > On 5/18/2020 9:55 PM, Rob Clark wrote: >> > > On Mon, May 18, 2020 at 7:23 AM Jordan Crouse <jcrouse@codeaurora.org> wrote: >> > >> On Thu, May 14, 2020 at 04:24:18PM +0530, Sharat Masetty wrote: >> > >>> This patches replaces the previously used static DDR vote and uses >> > >>> dev_pm_opp_set_bw() to scale GPU->DDR bandwidth along with scaling >> > >>> GPU frequency. >> > >>> >> > >>> Signed-off-by: Sharat Masetty <smasetty@codeaurora.org> >> > >>> --- >> > >>> drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 6 +----- >> > >>> 1 file changed, 1 insertion(+), 5 deletions(-) >> > >>> >> > >>> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c >> > >>> index 2d8124b..79433d3 100644 >> > >>> --- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c >> > >>> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c >> > >>> @@ -141,11 +141,7 @@ void a6xx_gmu_set_freq(struct msm_gpu *gpu, struct dev_pm_opp *opp) >> > >>> >> > >>> gmu->freq = gmu->gpu_freqs[perf_index]; >> > >>> >> > >>> - /* >> > >>> - * Eventually we will want to scale the path vote with the frequency but >> > >>> - * for now leave it at max so that the performance is nominal. >> > >>> - */ >> > >>> - icc_set_bw(gpu->icc_path, 0, MBps_to_icc(7216)); >> > >>> + dev_pm_opp_set_bw(&gpu->pdev->dev, opp); >> > >>> } >> > >> This adds an implicit requirement that all targets need bandwidth settings >> > >> defined in the OPP or they won't get a bus vote at all. I would prefer that >> > >> there be an default escape valve but if not you'll need to add >> > >> bandwidth values for the sdm845 OPP that target doesn't regress. >> > >> >> > > it looks like we could maybe do something like: >> > > >> > > ret = dev_pm_opp_set_bw(...); >> > > if (ret) { >> > > dev_warn_once(dev, "no bandwidth settings"); >> > > icc_set_bw(...); >> > > } >> > > >> > > ? >> > > >> > > BR, >> > > -R >> > >> > There is a bit of an issue here - Looks like its not possible to two icc >> > handles to the same path. Its causing double enumeration of the paths >> > in the icc core and messing up path votes. With [1] Since opp/core >> > already gets a handle to the icc path as part of table add, drm/msm >> > could do either > > Are you sure this is the real issue? I'd be surprised if this is a > real limitation. And if it is, it either needs to be fixed in the ICC > framework or OPP shouldn't be getting path handles by default (and not really, this is already handled well in the icc framework. In this case the max peak vote would be considered among the two paths. > maybe let the driver set the handles before using OPP APIs to change > BW). I'd lean towards the former. https://patchwork.kernel.org/patch/11573827/ Yes the core shouldn't get paths by default unless the bw values are specified in the opps. > >> > a) Conditionally enumerate gpu->icc_path handle only when pm/opp core >> > has not got the icc path handle. I could use something like [2] to >> > determine if should initialize gpu->icc_path* > > This seems like a bandaid. Let's fix it correctly in ICC framework or > OPP framework. > >> > b) Add peak-opp-configs in 845 dt and mandate all future versions to use I can't understand ^^ proposal as well. We would ideally want to add scaling support for SDM845 as well while we are at it. >> > this bindings. With this, I can remove gpu->icc_path from msm/drm >> > completely and only rely on opp/core for bw voting. > > I don't know what you mean by "peak-opp-configs" but I guess you are > referring to some kind of DT flag to say if you should vote for BW > directly or use the OPP framework? If so, I'm pretty sure that won't > fly. That's an OS implementation specific flag. > > -Saravana
On Wed, May 27, 2020 at 08:38:47AM -0700, Rob Clark wrote: > On Wed, May 27, 2020 at 1:47 AM Sharat Masetty <smasetty@codeaurora.org> wrote: > > > > + more folks > > > > On 5/18/2020 9:55 PM, Rob Clark wrote: > > > On Mon, May 18, 2020 at 7:23 AM Jordan Crouse <jcrouse@codeaurora.org> wrote: > > >> On Thu, May 14, 2020 at 04:24:18PM +0530, Sharat Masetty wrote: > > >>> This patches replaces the previously used static DDR vote and uses > > >>> dev_pm_opp_set_bw() to scale GPU->DDR bandwidth along with scaling > > >>> GPU frequency. > > >>> > > >>> Signed-off-by: Sharat Masetty <smasetty@codeaurora.org> > > >>> --- > > >>> drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 6 +----- > > >>> 1 file changed, 1 insertion(+), 5 deletions(-) > > >>> > > >>> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c > > >>> index 2d8124b..79433d3 100644 > > >>> --- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c > > >>> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c > > >>> @@ -141,11 +141,7 @@ void a6xx_gmu_set_freq(struct msm_gpu *gpu, struct dev_pm_opp *opp) > > >>> > > >>> gmu->freq = gmu->gpu_freqs[perf_index]; > > >>> > > >>> - /* > > >>> - * Eventually we will want to scale the path vote with the frequency but > > >>> - * for now leave it at max so that the performance is nominal. > > >>> - */ > > >>> - icc_set_bw(gpu->icc_path, 0, MBps_to_icc(7216)); > > >>> + dev_pm_opp_set_bw(&gpu->pdev->dev, opp); > > >>> } > > >> This adds an implicit requirement that all targets need bandwidth settings > > >> defined in the OPP or they won't get a bus vote at all. I would prefer that > > >> there be an default escape valve but if not you'll need to add > > >> bandwidth values for the sdm845 OPP that target doesn't regress. > > >> > > > it looks like we could maybe do something like: > > > > > > ret = dev_pm_opp_set_bw(...); > > > if (ret) { > > > dev_warn_once(dev, "no bandwidth settings"); > > > icc_set_bw(...); > > > } > > > > > > ? > > > > > > BR, > > > -R > > > > There is a bit of an issue here - Looks like its not possible to two icc > > handles to the same path. Its causing double enumeration of the paths > > in the icc core and messing up path votes. With [1] Since opp/core > > already gets a handle to the icc path as part of table add, drm/msm > > could do either > > > > a) Conditionally enumerate gpu->icc_path handle only when pm/opp core > > has not got the icc path handle. I could use something like [2] to > > determine if should initialize gpu->icc_path* > > > > b) Add peak-opp-configs in 845 dt and mandate all future versions to use > > this bindings. With this, I can remove gpu->icc_path from msm/drm > > completely and only rely on opp/core for bw voting. > > The main thing is that we want to make sure newer dtb always works on > an older kernel without regression.. but, hmm.. I guess the > interconnects/interconnects-names properties haven't landed yet in > sdm845.dtsi? Maybe that lets us go with the simpler approach (b). > Looks like we haven't wired up interconnect for 8916 or 8996 either, > so probably we can just mandate this for all of them? > > If we have landed the interconnect dts hookup for gpu somewhere that > I'm overlooking, I guess we would have to go with (a) and keep the > existing interconnects/interconnects-names properties. The main problem is that (on sdm845 at least) the path comes up with a very slow default so even if we don't do scaling we had to set _something_. Perhaps if we solved that problem somewhere else (inerconnect, rpmh?) then we wouldn't need to worry about it in the leaf driver unless the full opp tables are described. Jordan > BR, > -R > > > [1] - https://lore.kernel.org/patchwork/cover/1240687/ > > > > [2] - https://patchwork.kernel.org/patch/11527573/ > > > > Let me know your thoughts > > > > Sharat > > > > > > > >> Jordan > > >> > > >>> unsigned long a6xx_gmu_get_freq(struct msm_gpu *gpu) > > >>> -- > > >>> 2.7.4 > > >>> > > >> -- > > >> The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, > > >> a Linux Foundation Collaborative Project > > >> _______________________________________________ > > >> Freedreno mailing list > > >> Freedreno@lists.freedesktop.org > > >> https://lists.freedesktop.org/mailman/listinfo/freedreno
On 5/27/2020 9:08 PM, Rob Clark wrote: > On Wed, May 27, 2020 at 1:47 AM Sharat Masetty <smasetty@codeaurora.org> wrote: >> + more folks >> >> On 5/18/2020 9:55 PM, Rob Clark wrote: >>> On Mon, May 18, 2020 at 7:23 AM Jordan Crouse <jcrouse@codeaurora.org> wrote: >>>> On Thu, May 14, 2020 at 04:24:18PM +0530, Sharat Masetty wrote: >>>>> This patches replaces the previously used static DDR vote and uses >>>>> dev_pm_opp_set_bw() to scale GPU->DDR bandwidth along with scaling >>>>> GPU frequency. >>>>> >>>>> Signed-off-by: Sharat Masetty <smasetty@codeaurora.org> >>>>> --- >>>>> drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 6 +----- >>>>> 1 file changed, 1 insertion(+), 5 deletions(-) >>>>> >>>>> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c >>>>> index 2d8124b..79433d3 100644 >>>>> --- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c >>>>> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c >>>>> @@ -141,11 +141,7 @@ void a6xx_gmu_set_freq(struct msm_gpu *gpu, struct dev_pm_opp *opp) >>>>> >>>>> gmu->freq = gmu->gpu_freqs[perf_index]; >>>>> >>>>> - /* >>>>> - * Eventually we will want to scale the path vote with the frequency but >>>>> - * for now leave it at max so that the performance is nominal. >>>>> - */ >>>>> - icc_set_bw(gpu->icc_path, 0, MBps_to_icc(7216)); >>>>> + dev_pm_opp_set_bw(&gpu->pdev->dev, opp); >>>>> } >>>> This adds an implicit requirement that all targets need bandwidth settings >>>> defined in the OPP or they won't get a bus vote at all. I would prefer that >>>> there be an default escape valve but if not you'll need to add >>>> bandwidth values for the sdm845 OPP that target doesn't regress. >>>> >>> it looks like we could maybe do something like: >>> >>> ret = dev_pm_opp_set_bw(...); >>> if (ret) { >>> dev_warn_once(dev, "no bandwidth settings"); >>> icc_set_bw(...); >>> } >>> >>> ? >>> >>> BR, >>> -R >> There is a bit of an issue here - Looks like its not possible to two icc >> handles to the same path. Its causing double enumeration of the paths >> in the icc core and messing up path votes. With [1] Since opp/core >> already gets a handle to the icc path as part of table add, drm/msm >> could do either >> >> a) Conditionally enumerate gpu->icc_path handle only when pm/opp core >> has not got the icc path handle. I could use something like [2] to >> determine if should initialize gpu->icc_path* >> >> b) Add peak-opp-configs in 845 dt and mandate all future versions to use >> this bindings. With this, I can remove gpu->icc_path from msm/drm >> completely and only rely on opp/core for bw voting. > The main thing is that we want to make sure newer dtb always works on > an older kernel without regression.. but, hmm.. I guess the > interconnects/interconnects-names properties haven't landed yet in > sdm845.dtsi? Maybe that lets us go with the simpler approach (b). > Looks like we haven't wired up interconnect for 8916 or 8996 either, > so probably we can just mandate this for all of them? I checked all three 845, 820 and 8916 and none of them have the interconnect configs for GPU. So, I think we are good here. I'll go with option (b) and re-spin v3. Adding interconnects and opp-peak-kBps configs for previous chips can be taken up as a separate activity. Sharat > If we have landed the interconnect dts hookup for gpu somewhere that > I'm overlooking, I guess we would have to go with (a) and keep the > existing interconnects/interconnects-names properties. > > BR, > -R > >> [1] - https://lore.kernel.org/patchwork/cover/1240687/ >> >> [2] - https://patchwork.kernel.org/patch/11527573/ >> >> Let me know your thoughts >> >> Sharat >> >>>> Jordan >>>> >>>>> unsigned long a6xx_gmu_get_freq(struct msm_gpu *gpu) >>>>> -- >>>>> 2.7.4 >>>>> >>>> -- >>>> The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, >>>> a Linux Foundation Collaborative Project >>>> _______________________________________________ >>>> Freedreno mailing list >>>> Freedreno@lists.freedesktop.org >>>> https://lists.freedesktop.org/mailman/listinfo/freedreno
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c index 2d8124b..79433d3 100644 --- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c +++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c @@ -141,11 +141,7 @@ void a6xx_gmu_set_freq(struct msm_gpu *gpu, struct dev_pm_opp *opp) gmu->freq = gmu->gpu_freqs[perf_index]; - /* - * Eventually we will want to scale the path vote with the frequency but - * for now leave it at max so that the performance is nominal. - */ - icc_set_bw(gpu->icc_path, 0, MBps_to_icc(7216)); + dev_pm_opp_set_bw(&gpu->pdev->dev, opp); } unsigned long a6xx_gmu_get_freq(struct msm_gpu *gpu)
This patches replaces the previously used static DDR vote and uses dev_pm_opp_set_bw() to scale GPU->DDR bandwidth along with scaling GPU frequency. Signed-off-by: Sharat Masetty <smasetty@codeaurora.org> --- drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) -- 2.7.4