[PATCH 4/6] drm/amd/powerplay: add sclk OD support on Fiji

[Alex Deucher]

On Mon, May 16, 2016 at 6:04 PM, Emil Velikov <emil.l.velikov at gmail.com> wrote:
> On 16 May 2016 at 15:42, Alex Deucher <alexdeucher at gmail.com> wrote:
>> On Sat, May 14, 2016 at 10:03 AM, Emil Velikov <emil.l.velikov at gmail.com> wrote:
>>> Hi all,
>>>
>>> On 13 May 2016 at 19:48, Alex Deucher <alexdeucher at gmail.com> wrote:
>>>> From: Eric Huang <JinHuiEric.Huang at amd.com>
>>>>
>>>> This implements sclk overdrive(OD) overclocking support for Fiji,
>>>> and the maximum overdrive percentage is 20.
>>>>
>>>> Reviewed-by: Alex Deucher <alexander.deucher at amd.com>
>>>> Signed-off-by: Eric Huang <JinHuiEric.Huang at amd.com>
>>>> Signed-off-by: Alex Deucher <alexander.deucher at amd.com>
>>>> ---
>>>>  drivers/gpu/drm/amd/powerplay/hwmgr/fiji_hwmgr.c | 43 ++++++++++++++++++++++++
>>>>  1 file changed, 43 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/fiji_hwmgr.c b/drivers/gpu/drm/amd/powerplay/hwmgr/fiji_hwmgr.c
>>>> index 6f1bad4..bf7bf5f 100644
>>>> --- a/drivers/gpu/drm/amd/powerplay/hwmgr/fiji_hwmgr.c
>>>> +++ b/drivers/gpu/drm/amd/powerplay/hwmgr/fiji_hwmgr.c
>>>> @@ -5274,6 +5274,47 @@ bool fiji_check_smc_update_required_for_display_configuration(struct pp_hwmgr *h
>>>>         return is_update_required;
>>>>  }
>>>>
>>>> +static int fiji_get_sclk_od(struct pp_hwmgr *hwmgr)
>>>> +{
>>>> +       struct fiji_hwmgr *data = (struct fiji_hwmgr *)(hwmgr->backend);
>>>> +       struct fiji_single_dpm_table *sclk_table = &(data->dpm_table.sclk_table);
>>>> +       struct fiji_single_dpm_table *golden_sclk_table =
>>>> +                       &(data->golden_dpm_table.sclk_table);
>>>> +       int value;
>>>> +
>>>> +       value = (sclk_table->dpm_levels[sclk_table->count - 1].value -
>>>> +                       golden_sclk_table->dpm_levels[golden_sclk_table->count - 1].value) *
>>>> +                       100 /
>>>> +                       golden_sclk_table->dpm_levels[golden_sclk_table->count - 1].value;
>>>> +
>>>> +       return value;
>>>> +}
>>>> +
>>>> +static int fiji_set_sclk_od(struct pp_hwmgr *hwmgr, uint32_t value)
>>>> +{
>>>> +       struct fiji_hwmgr *data = (struct fiji_hwmgr *)(hwmgr->backend);
>>>> +       struct fiji_single_dpm_table *golden_sclk_table =
>>>> +                       &(data->golden_dpm_table.sclk_table);
>>>> +       struct pp_power_state  *ps;
>>>> +       struct fiji_power_state  *fiji_ps;
>>>> +
>>>> +       if (value > 20)
>>>> +               value = 20;
>>>> +
>>>> +       ps = hwmgr->request_ps;
>>>> +
>>>> +       if (ps == NULL)
>>>> +               return -EINVAL;
>>>> +
>>>> +       fiji_ps = cast_phw_fiji_power_state(&ps->hardware);
>>>> +
>>>> +       fiji_ps->performance_levels[fiji_ps->performance_level_count - 1].engine_clock =
>>>> +                       golden_sclk_table->dpm_levels[golden_sclk_table->count - 1].value *
>>>> +                       value / 100 +
>>>> +                       golden_sclk_table->dpm_levels[golden_sclk_table->count - 1].value;
>>>> +
>>>> +       return 0;
>>>> +}
>>>>
>>> Is it me or this patch 5/6 and 6/6 are identical ? Apart from the
>>> structs of course which... seems to be identical as well despite that
>>> they are copied across different headers and given different names.
>>> Imho one should try to hold them alongside this (and other related)
>>> code as opposed to duplicating even more code ?
>>>
>>
>> If someone was motived.
> I guess the people working/maintaining it should be motivated. Less
> code, easier to maintain, debug, smaller binary and a bunch other
> arguments that you know far better than me.

It cuts both ways.  Sharing a lot of code also hides bugs and makes
things harder to follow and debug.  You end up with lot of chips
specific if/else cases and you end up adding lot of function pointers
and forms of abstraction to split things up that make the code harder
to follow.  It's also really annoying when a handful of registers
change offsets so the code looks right but you aren't actually writing
to the registers you think you are.  It's a fine line.  We try and
share as much as makes sense, but still try and make the code easy to
follow and relatively compartmentalized.

>
>> The hw is similar, but not quite the same.
>> Generally new asics start with a copy of the old structs and then we
>> add/remove stuff to handle the difference in feature sets.
>> Unfortunately, power management is a bit less well defined compared to
>> other IP blocks since it has fingers into all of the other blocks.
>> With amdgpu, we've generally tried to keep IP blocks versions distinct
>> even if they are pretty similar and results in slightly more code to
>> avoid regressions on different asics due to differences in hw behavior
>> and changes in register offsets, etc.
>>
> So that's the root here - one copies the code and then makes changes if any.
>
> If people are using the "let's copy it all just in case for the
> future" approach this sounds like over-engineering (according to some
> people). Or perhaps it serves as a nice way to boost the stats -
> kernel is not XX lines of code :-P
>
> Fwiw I would strongly encourage that you and/or members of the team
> take a look at nouveau's structure [1]. It has handled duplication
> across generations, abstraction layers, using the driver as an user
> space application, etc., in a very elegant manner, imho. I believe
> others have mentioned/given it as an example, as well ;-)

I haven't looked too closely at the nouveau code, but I think our
structure is actually pretty similar.  Each asic is a collection of
versioned hw blocks.  We have a separate component for each hw block
(e.g., uvd5.x, uvd6.x, gfx7.x, gfx8.x, etc.) and each asic consists of
a table of hw blocks that correspond to the different block version
that make up the asic.  The hw block version specific code provides
callbacks to the asic independent core to provide the functionality
fullfilled by that hw block.

Alex


>
>
> Thanks
> Emil
>
> [1] https://cgit.freedesktop.org/~darktama/nouveau/