<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">2013/8/1 Deucher, Alexander <span dir="ltr"><<a href="mailto:Alexander.Deucher@amd.com" target="_blank">Alexander.Deucher@amd.com</a>></span><br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">> -----Original Message-----<br>
> From: Anthoine Bourgeois [mailto:<a href="mailto:anthoine.bourgeois@gmail.com">anthoine.bourgeois@gmail.com</a>]<br>
> Sent: Wednesday, July 31, 2013 6:34 PM<br>
> To: Deucher, Alexander; Koenig, Christian; Jerome Glisse; Anthoine<br>
> Bourgeois<br>
> Cc: <a href="mailto:dri-devel@lists.freedesktop.org">dri-devel@lists.freedesktop.org</a><br>
> Subject: [PATCH] drm/radeon/dpm: implement force performance levels for<br>
> rs780<br>
><br>
> Allows you to limit the selected power levels via sysfs.<br>
><br>
> Force the feedback divider to select a power level.<br>
<br>
</div>Nice work.  A few comments below.<br>
<br>
Alex<br>
<div><div class="h5"><br>
><br>
> Signed-off-by: Anthoine Bourgeois <<a href="mailto:anthoine.bourgeois@gmail.com">anthoine.bourgeois@gmail.com</a>><br>
> ---<br>
>  drivers/gpu/drm/radeon/radeon_asic.c |  1 +<br>
>  drivers/gpu/drm/radeon/radeon_asic.h |  2 ++<br>
>  drivers/gpu/drm/radeon/rs780_dpm.c   | 67<br>
> ++++++++++++++++++++++++++++++------<br>
>  3 files changed, 59 insertions(+), 11 deletions(-)<br>
><br>
> diff --git a/drivers/gpu/drm/radeon/radeon_asic.c<br>
> b/drivers/gpu/drm/radeon/radeon_asic.c<br>
> index f8f8b31..437d357 100644<br>
> --- a/drivers/gpu/drm/radeon/radeon_asic.c<br>
> +++ b/drivers/gpu/drm/radeon/radeon_asic.c<br>
> @@ -1272,6 +1272,7 @@ static struct radeon_asic rs780_asic = {<br>
>               .get_mclk = &rs780_dpm_get_mclk,<br>
>               .print_power_state = &rs780_dpm_print_power_state,<br>
>               .debugfs_print_current_performance_level =<br>
> &rs780_dpm_debugfs_print_current_performance_level,<br>
> +             .force_performance_level =<br>
> &rs780_dpm_force_performance_level,<br>
>       },<br>
>       .pflip = {<br>
>               .pre_page_flip = &rs600_pre_page_flip,<br>
> diff --git a/drivers/gpu/drm/radeon/radeon_asic.h<br>
> b/drivers/gpu/drm/radeon/radeon_asic.h<br>
> index 902479f..09841fc 100644<br>
> --- a/drivers/gpu/drm/radeon/radeon_asic.h<br>
> +++ b/drivers/gpu/drm/radeon/radeon_asic.h<br>
> @@ -437,6 +437,8 @@ void rs780_dpm_print_power_state(struct<br>
> radeon_device *rdev,<br>
>                                struct radeon_ps *ps);<br>
>  void rs780_dpm_debugfs_print_current_performance_level(struct<br>
> radeon_device *rdev,<br>
>                                                      struct seq_file *m);<br>
> +int rs780_dpm_force_performance_level(struct radeon_device *rdev,<br>
> +                                   enum radeon_dpm_forced_level level);<br>
><br>
>  /* uvd */<br>
>  int r600_uvd_init(struct radeon_device *rdev);<br>
> diff --git a/drivers/gpu/drm/radeon/rs780_dpm.c<br>
> b/drivers/gpu/drm/radeon/rs780_dpm.c<br>
> index d1a1ce7..df58e34 100644<br>
> --- a/drivers/gpu/drm/radeon/rs780_dpm.c<br>
> +++ b/drivers/gpu/drm/radeon/rs780_dpm.c<br>
> @@ -404,6 +404,27 @@ static void rs780_force_voltage_to_high(struct<br>
> radeon_device *rdev)<br>
>       WREG32_P(GFX_MACRO_BYPASS_CNTL, 0, ~SPLL_BYPASS_CNTL);<br>
>  }<br>
><br>
> +static void rs780_force_fbdiv(struct radeon_device *rdev, u32 fb_div)<br>
> +{<br>
> +     struct igp_ps *current_state = rs780_get_ps(rdev-<br>
> >pm.dpm.current_ps);<br>
> +<br>
> +     if ((current_state->sclk_low == fb_div) &&<br>
> +         (current_state->sclk_high == fb_div))<br>
> +             return;<br>
<br>
</div></div>I'm not quite sure what you are checking here.<br></blockquote><div><br></div><div>Oh yes, my mistake, fb_div shouldn't be in that condition. In my memory, I just wanted to check that sclk_low is equal to sclk_high. In that case, no need to force the fbdiv because there are the same.<br>
</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><div class="h5"><br>
> +<br>
> +     WREG32_P(GFX_MACRO_BYPASS_CNTL, SPLL_BYPASS_CNTL,<br>
> ~SPLL_BYPASS_CNTL);<br>
> +<br>
> +     WREG32_P(FVTHROT_FBDIV_REG2,<br>
> FORCED_FEEDBACK_DIV(fb_div),<br>
> +              ~FORCED_FEEDBACK_DIV_MASK);<br>
> +     WREG32_P(FVTHROT_FBDIV_REG1,<br>
> STARTING_FEEDBACK_DIV(fb_div),<br>
> +              ~STARTING_FEEDBACK_DIV_MASK);<br>
> +     WREG32_P(FVTHROT_FBDIV_REG1, FORCE_FEEDBACK_DIV,<br>
> ~FORCE_FEEDBACK_DIV);<br>
> +<br>
> +     udelay(100);<br>
> +<br>
> +     WREG32_P(GFX_MACRO_BYPASS_CNTL, 0, ~SPLL_BYPASS_CNTL);<br>
> +}<br>
> +<br>
>  static int rs780_set_engine_clock_scaling(struct radeon_device *rdev,<br>
>                                         struct radeon_ps *new_ps,<br>
>                                         struct radeon_ps *old_ps)<br>
> @@ -432,17 +453,7 @@ static int rs780_set_engine_clock_scaling(struct<br>
> radeon_device *rdev,<br>
>       if (ret)<br>
>               return ret;<br>
><br>
> -     WREG32_P(GFX_MACRO_BYPASS_CNTL, SPLL_BYPASS_CNTL,<br>
> ~SPLL_BYPASS_CNTL);<br>
> -<br>
> -     WREG32_P(FVTHROT_FBDIV_REG2,<br>
> FORCED_FEEDBACK_DIV(max_dividers.fb_div),<br>
> -              ~FORCED_FEEDBACK_DIV_MASK);<br>
> -     WREG32_P(FVTHROT_FBDIV_REG1,<br>
> STARTING_FEEDBACK_DIV(max_dividers.fb_div),<br>
> -              ~STARTING_FEEDBACK_DIV_MASK);<br>
> -     WREG32_P(FVTHROT_FBDIV_REG1, FORCE_FEEDBACK_DIV,<br>
> ~FORCE_FEEDBACK_DIV);<br>
> -<br>
> -     udelay(100);<br>
> -<br>
> -     WREG32_P(GFX_MACRO_BYPASS_CNTL, 0, ~SPLL_BYPASS_CNTL);<br>
> +     rs780_force_fbdiv(rdev, max_dividers.fb_div);<br>
><br>
>       if (max_dividers.fb_div > min_dividers.fb_div) {<br>
>               WREG32_P(FVTHROT_FBDIV_REG0,<br>
> @@ -986,3 +997,37 @@ void<br>
> rs780_dpm_debugfs_print_current_performance_level(struct<br>
> radeon_device *rde<br>
>               seq_printf(m, "power level 1    sclk: %u vddc_index: %d\n",<br>
>                          ps->sclk_high, ps->max_voltage);<br>
>  }<br>
> +<br>
> +int rs780_dpm_force_performance_level(struct radeon_device *rdev,<br>
> +                                   enum radeon_dpm_forced_level level)<br>
> +{<br>
> +     struct radeon_ps *rps = rdev->pm.dpm.current_ps;<br>
> +     struct igp_ps *ps = rs780_get_ps(rps);<br>
> +     struct atom_clock_dividers min_dividers, max_dividers;<br>
<br>
</div></div>You can drop the separate min and max and just use a single dividers struct.</blockquote><div><br></div><div>OK.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
  Also,<br>
You are only forcing the sclk.  It would probably also be a good idea to force the voltage levels.<br></blockquote><div><br></div><div>OK, I'm working on it but I can't test it, my board (RS880) doesn't seem to support the voltage scaling. I'll do my best.<br>
<br></div><div>Anthoine<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="HOEnZb"><div class="h5"><br>
> +     int ret;<br>
> +<br>
> +     rs780_clk_scaling_enable(rdev, false);<br>
> +<br>
> +     if (level == RADEON_DPM_FORCED_LEVEL_HIGH) {<br>
> +             ret = radeon_atom_get_clock_dividers(rdev,<br>
> COMPUTE_ENGINE_PLL_PARAM,<br>
> +                                                  ps->sclk_high, false,<br>
> &max_dividers);<br>
> +             if (ret)<br>
> +                     return ret;<br>
> +<br>
> +             rs780_force_fbdiv(rdev, max_dividers.fb_div);<br>
> +     } else if (level == RADEON_DPM_FORCED_LEVEL_LOW) {<br>
> +             ret = radeon_atom_get_clock_dividers(rdev,<br>
> COMPUTE_ENGINE_PLL_PARAM,<br>
> +                                                  ps->sclk_low, false,<br>
> &min_dividers);<br>
> +             if (ret)<br>
> +                     return ret;<br>
> +<br>
> +             rs780_force_fbdiv(rdev, min_dividers.fb_div);<br>
> +     } else {<br>
> +             WREG32_P(FVTHROT_FBDIV_REG1, 0,<br>
> ~FORCE_FEEDBACK_DIV);<br>
> +             rs780_clk_scaling_enable(rdev, true);<br>
> +     }<br>
> +<br>
> +     rdev->pm.dpm.forced_level = level;<br>
> +<br>
> +     return 0;<br>
> +}<br>
> --<br>
> 1.8.1.5<br>
><br>
<br>
<br>
</div></div></blockquote></div><br></div></div>