diff mbox series

[v4,4/7] drm: msm: a6xx: use dev_pm_opp_set_bw to scale DDR

Message ID 1594324828-9571-5-git-send-email-akhilpo@codeaurora.org (mailing list archive)
State Superseded
Headers show
Series Add support for GPU DDR BW scaling | expand

Commit Message

Akhil P Oommen July 9, 2020, 8 p.m. UTC 
From: Sharat Masetty <smasetty@codeaurora.org>

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. Also since the icc path voting is handled completely
in the opp driver, remove the icc_path handle and its usage in the
drm driver.

Signed-off-by: Sharat Masetty <smasetty@codeaurora.org>
Signed-off-by: Akhil P Oommen <akhilpo@codeaurora.org>
---
 drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 25 +++++++++++++++++--------
 1 file changed, 17 insertions(+), 8 deletions(-)

Comments

Rob Clark July 10, 2020, 7:41 p.m. UTC | #1 
On Thu, Jul 9, 2020 at 1:01 PM Akhil P Oommen <akhilpo@codeaurora.org> wrote:
>
> From: Sharat Masetty <smasetty@codeaurora.org>
>
> 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. Also since the icc path voting is handled completely
> in the opp driver, remove the icc_path handle and its usage in the
> drm driver.
>
> Signed-off-by: Sharat Masetty <smasetty@codeaurora.org>
> Signed-off-by: Akhil P Oommen <akhilpo@codeaurora.org>
> ---
>  drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 25 +++++++++++++++++--------
>  1 file changed, 17 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> index b547339..6fbfd7d 100644
> --- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> @@ -123,7 +123,7 @@ void a6xx_gmu_set_freq(struct msm_gpu *gpu, struct dev_pm_opp *opp)
>
>         if (!gmu->legacy) {
>                 a6xx_hfi_set_freq(gmu, gmu->current_perf_index);
> -               icc_set_bw(gpu->icc_path, 0, MBps_to_icc(7216));
> +               dev_pm_opp_set_bw(&gpu->pdev->dev, opp);

What is the status of the patch to add dev_pm_opp_set_bw()?  If it is
ready to go, and I get an ack-by from the OPP maintainer, I suppose I
could merge it via drm/msm.

Otherwise should we consider pulling in a private copy of it into
drm/msm (and then drop it to use the helper in, hopefully, the next
cycle)?

I'm pulling the patches preceding this one into msm-next-staging to do
some testing.  And the dt patches following this one would normally
get merged via Bjorn.  At the moment, I'm not sure what to do with
this one.

BR,
-R

>                 return;
>         }
>
> @@ -149,11 +149,7 @@ void a6xx_gmu_set_freq(struct msm_gpu *gpu, struct dev_pm_opp *opp)
>         if (ret)
>                 dev_err(gmu->dev, "GMU set GPU frequency error: %d\n", ret);
>
> -       /*
> -        * 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)
> @@ -840,6 +836,19 @@ static void a6xx_gmu_set_initial_freq(struct msm_gpu *gpu, struct a6xx_gmu *gmu)
>         dev_pm_opp_put(gpu_opp);
>  }
>
> +static void a6xx_gmu_set_initial_bw(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;
> +
> +       dev_pm_opp_set_bw(&gpu->pdev->dev, 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;
> @@ -864,7 +873,7 @@ int a6xx_gmu_resume(struct a6xx_gpu *a6xx_gpu)
>         }
>
>         /* Set the bus quota to a reasonable value for boot */
> -       icc_set_bw(gpu->icc_path, 0, MBps_to_icc(3072));
> +       a6xx_gmu_set_initial_bw(gpu, gmu);
>
>         /* Enable the GMU interrupt */
>         gmu_write(gmu, REG_A6XX_GMU_AO_HOST_INTERRUPT_CLR, ~0);
> @@ -1040,7 +1049,7 @@ int a6xx_gmu_stop(struct a6xx_gpu *a6xx_gpu)
>                 a6xx_gmu_shutdown(gmu);
>
>         /* Remove the bus vote */
> -       icc_set_bw(gpu->icc_path, 0, 0);
> +       dev_pm_opp_set_bw(&gpu->pdev->dev, NULL);
>
>         /*
>          * Make sure the GX domain is off before turning off the GMU (CX)
> --
> 2.7.4
>
> _______________________________________________
> Freedreno mailing list
> Freedreno@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/freedreno
Akhil P Oommen July 10, 2020, 9:03 p.m. UTC | #2 
On 7/11/2020 1:11 AM, Rob Clark wrote:
> On Thu, Jul 9, 2020 at 1:01 PM Akhil P Oommen <akhilpo@codeaurora.org> wrote:
>> From: Sharat Masetty <smasetty@codeaurora.org>
>>
>> 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. Also since the icc path voting is handled completely
>> in the opp driver, remove the icc_path handle and its usage in the
>> drm driver.
>>
>> Signed-off-by: Sharat Masetty <smasetty@codeaurora.org>
>> Signed-off-by: Akhil P Oommen <akhilpo@codeaurora.org>
>> ---
>>   drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 25 +++++++++++++++++--------
>>   1 file changed, 17 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
>> index b547339..6fbfd7d 100644
>> --- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
>> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
>> @@ -123,7 +123,7 @@ void a6xx_gmu_set_freq(struct msm_gpu *gpu, struct dev_pm_opp *opp)
>>
>>          if (!gmu->legacy) {
>>                  a6xx_hfi_set_freq(gmu, gmu->current_perf_index);
>> -               icc_set_bw(gpu->icc_path, 0, MBps_to_icc(7216));
>> +               dev_pm_opp_set_bw(&gpu->pdev->dev, opp);
> What is the status of the patch to add dev_pm_opp_set_bw()?  If it is
> ready to go, and I get an ack-by from the OPP maintainer, I suppose I
> could merge it via drm/msm.
>
> Otherwise should we consider pulling in a private copy of it into
> drm/msm (and then drop it to use the helper in, hopefully, the next
> cycle)?
>
> I'm pulling the patches preceding this one into msm-next-staging to do
> some testing.  And the dt patches following this one would normally
> get merged via Bjorn.  At the moment, I'm not sure what to do with
> this one.
>
> BR,
> -R
I see Sibi's patch is already picked in opp/linux-next branch.
https://kernel.googlesource.com/pub/scm/linux/kernel/git/vireshk/pm/+/b466542f331e221a3628c1cfe5ccff307d7d787f 


Thanks,
-Akhil

>>                  return;
>>          }
>>
>> @@ -149,11 +149,7 @@ void a6xx_gmu_set_freq(struct msm_gpu *gpu, struct dev_pm_opp *opp)
>>          if (ret)
>>                  dev_err(gmu->dev, "GMU set GPU frequency error: %d\n", ret);
>>
>> -       /*
>> -        * 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)
>> @@ -840,6 +836,19 @@ static void a6xx_gmu_set_initial_freq(struct msm_gpu *gpu, struct a6xx_gmu *gmu)
>>          dev_pm_opp_put(gpu_opp);
>>   }
>>
>> +static void a6xx_gmu_set_initial_bw(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;
>> +
>> +       dev_pm_opp_set_bw(&gpu->pdev->dev, 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;
>> @@ -864,7 +873,7 @@ int a6xx_gmu_resume(struct a6xx_gpu *a6xx_gpu)
>>          }
>>
>>          /* Set the bus quota to a reasonable value for boot */
>> -       icc_set_bw(gpu->icc_path, 0, MBps_to_icc(3072));
>> +       a6xx_gmu_set_initial_bw(gpu, gmu);
>>
>>          /* Enable the GMU interrupt */
>>          gmu_write(gmu, REG_A6XX_GMU_AO_HOST_INTERRUPT_CLR, ~0);
>> @@ -1040,7 +1049,7 @@ int a6xx_gmu_stop(struct a6xx_gpu *a6xx_gpu)
>>                  a6xx_gmu_shutdown(gmu);
>>
>>          /* Remove the bus vote */
>> -       icc_set_bw(gpu->icc_path, 0, 0);
>> +       dev_pm_opp_set_bw(&gpu->pdev->dev, NULL);
>>
>>          /*
>>           * Make sure the GX domain is off before turning off the GMU (CX)
>> --
>> 2.7.4
>>
>> _______________________________________________
>> Freedreno mailing list
>> Freedreno@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/freedreno
Rob Clark July 10, 2020, 10:32 p.m. UTC | #3 
On Fri, Jul 10, 2020 at 2:03 PM Akhil P Oommen <akhilpo@codeaurora.org> wrote:
>
>
> On 7/11/2020 1:11 AM, Rob Clark wrote:
> > On Thu, Jul 9, 2020 at 1:01 PM Akhil P Oommen <akhilpo@codeaurora.org> wrote:
> >> From: Sharat Masetty <smasetty@codeaurora.org>
> >>
> >> 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. Also since the icc path voting is handled completely
> >> in the opp driver, remove the icc_path handle and its usage in the
> >> drm driver.
> >>
> >> Signed-off-by: Sharat Masetty <smasetty@codeaurora.org>
> >> Signed-off-by: Akhil P Oommen <akhilpo@codeaurora.org>
> >> ---
> >>   drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 25 +++++++++++++++++--------
> >>   1 file changed, 17 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> >> index b547339..6fbfd7d 100644
> >> --- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> >> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> >> @@ -123,7 +123,7 @@ void a6xx_gmu_set_freq(struct msm_gpu *gpu, struct dev_pm_opp *opp)
> >>
> >>          if (!gmu->legacy) {
> >>                  a6xx_hfi_set_freq(gmu, gmu->current_perf_index);
> >> -               icc_set_bw(gpu->icc_path, 0, MBps_to_icc(7216));
> >> +               dev_pm_opp_set_bw(&gpu->pdev->dev, opp);
> > What is the status of the patch to add dev_pm_opp_set_bw()?  If it is
> > ready to go, and I get an ack-by from the OPP maintainer, I suppose I
> > could merge it via drm/msm.
> >
> > Otherwise should we consider pulling in a private copy of it into
> > drm/msm (and then drop it to use the helper in, hopefully, the next
> > cycle)?
> >
> > I'm pulling the patches preceding this one into msm-next-staging to do
> > some testing.  And the dt patches following this one would normally
> > get merged via Bjorn.  At the moment, I'm not sure what to do with
> > this one.
> >
> > BR,
> > -R
> I see Sibi's patch is already picked in opp/linux-next branch.
> https://kernel.googlesource.com/pub/scm/linux/kernel/git/vireshk/pm/+/b466542f331e221a3628c1cfe5ccff307d7d787f
>

ok, I guess we can try and do a 2nd late pull-req for msm-next, after
the opp pull-req lands..

BR,
-R

>
> Thanks,
> -Akhil
>
> >>                  return;
> >>          }
> >>
> >> @@ -149,11 +149,7 @@ void a6xx_gmu_set_freq(struct msm_gpu *gpu, struct dev_pm_opp *opp)
> >>          if (ret)
> >>                  dev_err(gmu->dev, "GMU set GPU frequency error: %d\n", ret);
> >>
> >> -       /*
> >> -        * 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)
> >> @@ -840,6 +836,19 @@ static void a6xx_gmu_set_initial_freq(struct msm_gpu *gpu, struct a6xx_gmu *gmu)
> >>          dev_pm_opp_put(gpu_opp);
> >>   }
> >>
> >> +static void a6xx_gmu_set_initial_bw(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;
> >> +
> >> +       dev_pm_opp_set_bw(&gpu->pdev->dev, 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;
> >> @@ -864,7 +873,7 @@ int a6xx_gmu_resume(struct a6xx_gpu *a6xx_gpu)
> >>          }
> >>
> >>          /* Set the bus quota to a reasonable value for boot */
> >> -       icc_set_bw(gpu->icc_path, 0, MBps_to_icc(3072));
> >> +       a6xx_gmu_set_initial_bw(gpu, gmu);
> >>
> >>          /* Enable the GMU interrupt */
> >>          gmu_write(gmu, REG_A6XX_GMU_AO_HOST_INTERRUPT_CLR, ~0);
> >> @@ -1040,7 +1049,7 @@ int a6xx_gmu_stop(struct a6xx_gpu *a6xx_gpu)
> >>                  a6xx_gmu_shutdown(gmu);
> >>
> >>          /* Remove the bus vote */
> >> -       icc_set_bw(gpu->icc_path, 0, 0);
> >> +       dev_pm_opp_set_bw(&gpu->pdev->dev, NULL);
> >>
> >>          /*
> >>           * Make sure the GX domain is off before turning off the GMU (CX)
> >> --
> >> 2.7.4
> >>
> >> _______________________________________________
> >> Freedreno mailing list
> >> Freedreno@lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/freedreno
diff mbox series

Patch

diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
index b547339..6fbfd7d 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
@@ -123,7 +123,7 @@  void a6xx_gmu_set_freq(struct msm_gpu *gpu, struct dev_pm_opp *opp)
 
 	if (!gmu->legacy) {
 		a6xx_hfi_set_freq(gmu, gmu->current_perf_index);
-		icc_set_bw(gpu->icc_path, 0, MBps_to_icc(7216));
+		dev_pm_opp_set_bw(&gpu->pdev->dev, opp);
 		return;
 	}
 
@@ -149,11 +149,7 @@  void a6xx_gmu_set_freq(struct msm_gpu *gpu, struct dev_pm_opp *opp)
 	if (ret)
 		dev_err(gmu->dev, "GMU set GPU frequency error: %d\n", ret);
 
-	/*
-	 * 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)
@@ -840,6 +836,19 @@  static void a6xx_gmu_set_initial_freq(struct msm_gpu *gpu, struct a6xx_gmu *gmu)
 	dev_pm_opp_put(gpu_opp);
 }
 
+static void a6xx_gmu_set_initial_bw(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;
+
+	dev_pm_opp_set_bw(&gpu->pdev->dev, 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;
@@ -864,7 +873,7 @@  int a6xx_gmu_resume(struct a6xx_gpu *a6xx_gpu)
 	}
 
 	/* Set the bus quota to a reasonable value for boot */
-	icc_set_bw(gpu->icc_path, 0, MBps_to_icc(3072));
+	a6xx_gmu_set_initial_bw(gpu, gmu);
 
 	/* Enable the GMU interrupt */
 	gmu_write(gmu, REG_A6XX_GMU_AO_HOST_INTERRUPT_CLR, ~0);
@@ -1040,7 +1049,7 @@  int a6xx_gmu_stop(struct a6xx_gpu *a6xx_gpu)
 		a6xx_gmu_shutdown(gmu);
 
 	/* Remove the bus vote */
-	icc_set_bw(gpu->icc_path, 0, 0);
+	dev_pm_opp_set_bw(&gpu->pdev->dev, NULL);
 
 	/*
 	 * Make sure the GX domain is off before turning off the GMU (CX)