mbox series

[v3,0/6] Add support for GPU DDR BW scaling

Message ID 1591417551-38051-1-git-send-email-smasetty@codeaurora.org
Headers show
Series Add support for GPU DDR BW scaling | expand

Message

Sharat Masetty June 6, 2020, 4:25 a.m. UTC 
This is a respin of [1]. Incorported review feedback and fixed issues observed
during testing. Picked up the Georgi's series from opp/linux-next [2], and this
series is also dependent on a helper function needed to set and clear ddr
bandwidth vote [3]. Patch number 4 in the series adds support for SDM845 as well
but its not tested yet(WIP), but the SC7180 patches are well tested now.

[1] https://patchwork.freedesktop.org/series/75291/
[2] https://kernel.googlesource.com/pub/scm/linux/kernel/git/vireshk/pm/+log/opp/linux-next/
[3] https://patchwork.kernel.org/patch/11590563/

Sharat Masetty (6):
  dt-bindings: drm/msm/gpu: Document gpu opp table
  drm: msm: a6xx: send opp instead of a frequency
  drm: msm: a6xx: use dev_pm_opp_set_bw to scale DDR
  arm64: dts: qcom: SDM845: Enable GPU DDR bw scaling
  arm64: dts: qcom: sc7180: Add interconnects property for GPU
  arm64: dts: qcom: sc7180: Add opp-peak-kBps to GPU opp

 .../devicetree/bindings/display/msm/gpu.txt        | 28 +++++++
 arch/arm64/boot/dts/qcom/sc7180.dtsi               |  9 +++
 arch/arm64/boot/dts/qcom/sdm845.dtsi               |  9 +++
 drivers/gpu/drm/msm/adreno/a6xx_gmu.c              | 85 +++++++++++++---------
 drivers/gpu/drm/msm/adreno/a6xx_gpu.h              |  2 +-
 drivers/gpu/drm/msm/adreno/adreno_gpu.c            |  8 --
 drivers/gpu/drm/msm/msm_gpu.c                      |  3 +-
 drivers/gpu/drm/msm/msm_gpu.h                      |  5 +-
 8 files changed, 100 insertions(+), 49 deletions(-)

--
2.7.4

Comments

Viresh Kumar June 15, 2020, 6:23 a.m. UTC | #1 
On 06-06-20, 09:55, Sharat Masetty wrote:
> This is a respin of [1]. Incorported review feedback and fixed issues observed
> during testing. Picked up the Georgi's series from opp/linux-next [2], and this
> series is also dependent on a helper function needed to set and clear ddr
> bandwidth vote [3]. Patch number 4 in the series adds support for SDM845 as well
> but its not tested yet(WIP), but the SC7180 patches are well tested now.
> 
> [1] https://patchwork.freedesktop.org/series/75291/
> [2] https://kernel.googlesource.com/pub/scm/linux/kernel/git/vireshk/pm/+log/opp/linux-next/
> [3] https://patchwork.kernel.org/patch/11590563/

Had a quick look of the series and looked mostly fine to me.
Rob Clark June 18, 2020, 5:52 p.m. UTC | #2 
On Fri, Jun 5, 2020 at 9:26 PM Sharat Masetty <smasetty@codeaurora.org> wrote:
>

> This patch changes the plumbing to send the devfreq recommended opp rather

> than the frequency. Also consolidate and rearrange the code in a6xx to set

> the GPU frequency and the icc vote in preparation for the upcoming

> changes for GPU->DDR scaling votes.

>

> Signed-off-by: Sharat Masetty <smasetty@codeaurora.org>

> ---

>  drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 62 +++++++++++++++++++----------------

>  drivers/gpu/drm/msm/adreno/a6xx_gpu.h |  2 +-

>  drivers/gpu/drm/msm/msm_gpu.c         |  3 +-

>  drivers/gpu/drm/msm/msm_gpu.h         |  3 +-

>  4 files changed, 38 insertions(+), 32 deletions(-)

>

> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c

> index 748cd37..2d8124b 100644

> --- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c

> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c

> @@ -100,17 +100,30 @@ bool a6xx_gmu_gx_is_on(struct a6xx_gmu *gmu)

>                 A6XX_GMU_SPTPRAC_PWR_CLK_STATUS_GX_HM_CLK_OFF));

>  }

>

> -static void __a6xx_gmu_set_freq(struct a6xx_gmu *gmu, int index)

> +void a6xx_gmu_set_freq(struct msm_gpu *gpu, struct dev_pm_opp *opp)

>  {

> -       struct a6xx_gpu *a6xx_gpu = container_of(gmu, struct a6xx_gpu, gmu);

> -       struct adreno_gpu *adreno_gpu = &a6xx_gpu->base;

> -       struct msm_gpu *gpu = &adreno_gpu->base;

> -       int ret;

> +       struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);

> +       struct a6xx_gpu *a6xx_gpu = to_a6xx_gpu(adreno_gpu);

> +       struct a6xx_gmu *gmu = &a6xx_gpu->gmu;

> +       u32 perf_index;

> +       unsigned long gpu_freq;

> +       int ret = 0;

> +

> +       gpu_freq = dev_pm_opp_get_freq(opp);

> +

> +       if (gpu_freq == gmu->freq)

> +               return;

> +

> +       for (perf_index = 0; perf_index < gmu->nr_gpu_freqs - 1; perf_index++)

> +               if (gpu_freq == gmu->gpu_freqs[perf_index])

> +                       break;

> +

> +       gmu->current_perf_index = perf_index;

>

>         gmu_write(gmu, REG_A6XX_GMU_DCVS_ACK_OPTION, 0);

>

>         gmu_write(gmu, REG_A6XX_GMU_DCVS_PERF_SETTING,

> -               ((3 & 0xf) << 28) | index);

> +                       ((3 & 0xf) << 28) | perf_index);

>

>         /*

>          * Send an invalid index as a vote for the bus bandwidth and let the

> @@ -126,7 +139,7 @@ static void __a6xx_gmu_set_freq(struct a6xx_gmu *gmu, int index)

>         if (ret)

>                 dev_err(gmu->dev, "GMU set GPU frequency error: %d\n", ret);

>

> -       gmu->freq = gmu->gpu_freqs[index];

> +       gmu->freq = gmu->gpu_freqs[perf_index];

>

>         /*

>          * Eventually we will want to scale the path vote with the frequency but

> @@ -135,25 +148,6 @@ static void __a6xx_gmu_set_freq(struct a6xx_gmu *gmu, int index)

>         icc_set_bw(gpu->icc_path, 0, MBps_to_icc(7216));

>  }

>

> -void a6xx_gmu_set_freq(struct msm_gpu *gpu, unsigned long freq)

> -{

> -       struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);

> -       struct a6xx_gpu *a6xx_gpu = to_a6xx_gpu(adreno_gpu);

> -       struct a6xx_gmu *gmu = &a6xx_gpu->gmu;

> -       u32 perf_index = 0;

> -

> -       if (freq == gmu->freq)

> -               return;

> -

> -       for (perf_index = 0; perf_index < gmu->nr_gpu_freqs - 1; perf_index++)

> -               if (freq == gmu->gpu_freqs[perf_index])

> -                       break;

> -

> -       gmu->current_perf_index = perf_index;

> -

> -       __a6xx_gmu_set_freq(gmu, perf_index);

> -}


this does end up conflicting a bit with some of the newer stuff that
landed this cycle, in particular "drm/msm/a6xx: HFI v2 for A640 and
A650"

Adding Jonathan on CC since I think he will want to test this on
a650/a640 as well..

BR,
-R

> -

>  unsigned long a6xx_gmu_get_freq(struct msm_gpu *gpu)

>  {

>         struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);

> @@ -708,6 +702,19 @@ static void a6xx_gmu_force_off(struct a6xx_gmu *gmu)

>         a6xx_gmu_rpmh_off(gmu);

>  }

>

> +static void a6xx_gmu_set_initial_freq(struct msm_gpu *gpu, struct a6xx_gmu *gmu)

> +{

> +       struct dev_pm_opp *gpu_opp;

> +       unsigned long gpu_freq = gmu->gpu_freqs[gmu->current_perf_index];

> +

> +       gpu_opp = dev_pm_opp_find_freq_exact(&gpu->pdev->dev, gpu_freq, true);

> +       if (IS_ERR_OR_NULL(gpu_opp))

> +               return;

> +

> +       a6xx_gmu_set_freq(gpu, gpu_opp);

> +       dev_pm_opp_put(gpu_opp);

> +}

> +

>  int a6xx_gmu_resume(struct a6xx_gpu *a6xx_gpu)

>  {

>         struct adreno_gpu *adreno_gpu = &a6xx_gpu->base;

> @@ -759,8 +766,7 @@ int a6xx_gmu_resume(struct a6xx_gpu *a6xx_gpu)

>         gmu_write(gmu, REG_A6XX_GMU_GMU2HOST_INTR_MASK, ~A6XX_HFI_IRQ_MASK);

>         enable_irq(gmu->hfi_irq);

>

> -       /* Set the GPU to the current freq */

> -       __a6xx_gmu_set_freq(gmu, gmu->current_perf_index);

> +       a6xx_gmu_set_initial_freq(gpu, gmu);

>

>         /*

>          * "enable" the GX power domain which won't actually do anything but it

> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.h b/drivers/gpu/drm/msm/adreno/a6xx_gpu.h

> index 7239b8b..03ba60d 100644

> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.h

> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.h

> @@ -63,7 +63,7 @@ void a6xx_gmu_clear_oob(struct a6xx_gmu *gmu, enum a6xx_gmu_oob_state state);

>  int a6xx_gmu_init(struct a6xx_gpu *a6xx_gpu, struct device_node *node);

>  void a6xx_gmu_remove(struct a6xx_gpu *a6xx_gpu);

>

> -void a6xx_gmu_set_freq(struct msm_gpu *gpu, unsigned long freq);

> +void a6xx_gmu_set_freq(struct msm_gpu *gpu, struct dev_pm_opp *opp);

>  unsigned long a6xx_gmu_get_freq(struct msm_gpu *gpu);

>

>  void a6xx_show(struct msm_gpu *gpu, struct msm_gpu_state *state,

> diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c

> index 615c5cd..82c3068 100644

> --- a/drivers/gpu/drm/msm/msm_gpu.c

> +++ b/drivers/gpu/drm/msm/msm_gpu.c

> @@ -13,7 +13,6 @@

>

>  #include <generated/utsrelease.h>

>  #include <linux/string_helpers.h>

> -#include <linux/pm_opp.h>

>  #include <linux/devfreq.h>

>  #include <linux/devcoredump.h>

>  #include <linux/sched/task.h>

> @@ -34,7 +33,7 @@ static int msm_devfreq_target(struct device *dev, unsigned long *freq,

>                 return PTR_ERR(opp);

>

>         if (gpu->funcs->gpu_set_freq)

> -               gpu->funcs->gpu_set_freq(gpu, (u64)*freq);

> +               gpu->funcs->gpu_set_freq(gpu, opp);

>         else

>                 clk_set_rate(gpu->core_clk, *freq);

>

> diff --git a/drivers/gpu/drm/msm/msm_gpu.h b/drivers/gpu/drm/msm/msm_gpu.h

> index ab8f0f9c..cf0dc6d 100644

> --- a/drivers/gpu/drm/msm/msm_gpu.h

> +++ b/drivers/gpu/drm/msm/msm_gpu.h

> @@ -9,6 +9,7 @@

>

>  #include <linux/clk.h>

>  #include <linux/interconnect.h>

> +#include <linux/pm_opp.h>

>  #include <linux/regulator/consumer.h>

>

>  #include "msm_drv.h"

> @@ -63,7 +64,7 @@ struct msm_gpu_funcs {

>         struct msm_gpu_state *(*gpu_state_get)(struct msm_gpu *gpu);

>         int (*gpu_state_put)(struct msm_gpu_state *state);

>         unsigned long (*gpu_get_freq)(struct msm_gpu *gpu);

> -       void (*gpu_set_freq)(struct msm_gpu *gpu, unsigned long freq);

> +       void (*gpu_set_freq)(struct msm_gpu *gpu, struct dev_pm_opp *opp);

>  };

>

>  struct msm_gpu {

> --

> 2.7.4

>
Matthias Kaehlcke June 24, 2020, 4:57 p.m. UTC | #3 
Hi,

On Thu, Jun 18, 2020 at 10:52:09AM -0700, Rob Clark wrote:
> On Fri, Jun 5, 2020 at 9:26 PM Sharat Masetty <smasetty@codeaurora.org> wrote:

> >

> > This patch changes the plumbing to send the devfreq recommended opp rather

> > than the frequency. Also consolidate and rearrange the code in a6xx to set

> > the GPU frequency and the icc vote in preparation for the upcoming

> > changes for GPU->DDR scaling votes.

> >

> > Signed-off-by: Sharat Masetty <smasetty@codeaurora.org>

> > ---

> >  drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 62 +++++++++++++++++++----------------

> >  drivers/gpu/drm/msm/adreno/a6xx_gpu.h |  2 +-

> >  drivers/gpu/drm/msm/msm_gpu.c         |  3 +-

> >  drivers/gpu/drm/msm/msm_gpu.h         |  3 +-

> >  4 files changed, 38 insertions(+), 32 deletions(-)

> >

> > diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c

> > index 748cd37..2d8124b 100644

> > --- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c

> > +++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c

> > @@ -100,17 +100,30 @@ bool a6xx_gmu_gx_is_on(struct a6xx_gmu *gmu)

> >                 A6XX_GMU_SPTPRAC_PWR_CLK_STATUS_GX_HM_CLK_OFF));

> >  }

> >

> > -static void __a6xx_gmu_set_freq(struct a6xx_gmu *gmu, int index)

> > +void a6xx_gmu_set_freq(struct msm_gpu *gpu, struct dev_pm_opp *opp)

> >  {

> > -       struct a6xx_gpu *a6xx_gpu = container_of(gmu, struct a6xx_gpu, gmu);

> > -       struct adreno_gpu *adreno_gpu = &a6xx_gpu->base;

> > -       struct msm_gpu *gpu = &adreno_gpu->base;

> > -       int ret;

> > +       struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);

> > +       struct a6xx_gpu *a6xx_gpu = to_a6xx_gpu(adreno_gpu);

> > +       struct a6xx_gmu *gmu = &a6xx_gpu->gmu;

> > +       u32 perf_index;

> > +       unsigned long gpu_freq;

> > +       int ret = 0;

> > +

> > +       gpu_freq = dev_pm_opp_get_freq(opp);

> > +

> > +       if (gpu_freq == gmu->freq)

> > +               return;

> > +

> > +       for (perf_index = 0; perf_index < gmu->nr_gpu_freqs - 1; perf_index++)

> > +               if (gpu_freq == gmu->gpu_freqs[perf_index])

> > +                       break;

> > +

> > +       gmu->current_perf_index = perf_index;

> >

> >         gmu_write(gmu, REG_A6XX_GMU_DCVS_ACK_OPTION, 0);

> >

> >         gmu_write(gmu, REG_A6XX_GMU_DCVS_PERF_SETTING,

> > -               ((3 & 0xf) << 28) | index);

> > +                       ((3 & 0xf) << 28) | perf_index);

> >

> >         /*

> >          * Send an invalid index as a vote for the bus bandwidth and let the

> > @@ -126,7 +139,7 @@ static void __a6xx_gmu_set_freq(struct a6xx_gmu *gmu, int index)

> >         if (ret)

> >                 dev_err(gmu->dev, "GMU set GPU frequency error: %d\n", ret);

> >

> > -       gmu->freq = gmu->gpu_freqs[index];

> > +       gmu->freq = gmu->gpu_freqs[perf_index];

> >

> >         /*

> >          * Eventually we will want to scale the path vote with the frequency but

> > @@ -135,25 +148,6 @@ static void __a6xx_gmu_set_freq(struct a6xx_gmu *gmu, int index)

> >         icc_set_bw(gpu->icc_path, 0, MBps_to_icc(7216));

> >  }

> >

> > -void a6xx_gmu_set_freq(struct msm_gpu *gpu, unsigned long freq)

> > -{

> > -       struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);

> > -       struct a6xx_gpu *a6xx_gpu = to_a6xx_gpu(adreno_gpu);

> > -       struct a6xx_gmu *gmu = &a6xx_gpu->gmu;

> > -       u32 perf_index = 0;

> > -

> > -       if (freq == gmu->freq)

> > -               return;

> > -

> > -       for (perf_index = 0; perf_index < gmu->nr_gpu_freqs - 1; perf_index++)

> > -               if (freq == gmu->gpu_freqs[perf_index])

> > -                       break;

> > -

> > -       gmu->current_perf_index = perf_index;

> > -

> > -       __a6xx_gmu_set_freq(gmu, perf_index);

> > -}

> 

> this does end up conflicting a bit with some of the newer stuff that

> landed this cycle, in particular "drm/msm/a6xx: HFI v2 for A640 and

> A650"

> 

> Adding Jonathan on CC since I think he will want to test this on

> a650/a640 as well..


Sharat, please send an updated version that is rebased on the latest drm-msm.

Thanks

Matthias