Re: [Intel-gfx] [PATCH 2/2] drm/amdgpu: Disable PCIE_DPM on Intel RKL Platform

[Jani Nikula <jani.nikula@linux.intel.com>]

On Wed, 25 Aug 2021, Koba Ko <koba.ko@canonical.com> wrote:
> On Wed, Aug 25, 2021 at 5:22 PM Jani Nikula <jani.nikula@linux.intel.com> wrote:
>>
>> On Wed, 25 Aug 2021, Koba Ko <koba.ko@canonical.com> wrote:
>> > AMD polaris GPUs have an issue about audio noise on RKL platform,
>> > they provide a commit to fix but for SMU7-based GPU still
>> > need another module parameter,
>> >
>> > For avoiding the module parameter, switch PCI_DPM by determining
>> > intel platform in amd drm driver.
>>
>> I'll just note that you could have a Tiger Lake PCH combined with a
>> number of platforms other than Rocket Lake, including not just the
>> obvious Tiger Lake but also Sky Lake, Kaby Lake, Coffee Lake, and Comet
>> Lake.
>>
>> Again, I don't know what the root cause or fix should be, the workaround
>> presented here impacts a much larger number of platforms than where
>> you're claiming the issue is.
>
> Hi Jani, thanks for your feedback.
> Is there any way to identify the RKL PCH?
> I trace the intel_pch.c and can't find the only pch id for RKL.
>
> INTEL_PCH_TGP_DEVICE_ID_TYPE is used by both TGL and RKL.
>
> so it seems that using IS_ROCKETLAKE() is the only way.

I don't think there is a Rocket Lake PCH. But is the problem related to
the PCH or not?

The GPU PCI IDs are in i915_pciids.h. See INTEL_RKL_IDS() for
RKL. There's a lot of indirection, but that's what IS_ROCKETLAKE() boils
down to. But again, I'm not sure if that's what you want or not.

BR,
Jani.


>
> Thanks
>>
>> BR,
>> Jani.
>>
>>
>> >
>> > Fixes: 1a31474cdb48 ("drm/amd/pm: workaround for audio noise issue")
>> > Ref: https://lists.freedesktop.org/archives/amd-gfx/2021-August/067413.html
>> > Signed-off-by: Koba Ko <koba.ko@canonical.com>
>> > ---
>> >  .../drm/amd/pm/powerplay/hwmgr/smu7_hwmgr.c   | 21 ++++++++++++++++++-
>> >  1 file changed, 20 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/smu7_hwmgr.c b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/smu7_hwmgr.c
>> > index 0541bfc81c1b..346110dd0f51 100644
>> > --- a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/smu7_hwmgr.c
>> > +++ b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/smu7_hwmgr.c
>> > @@ -1733,6 +1733,25 @@ static int smu7_disable_dpm_tasks(struct pp_hwmgr *hwmgr)
>> >       return result;
>> >  }
>> >
>> > +#include <drm/intel_pch.h>
>> > +
>> > +static bool intel_tgp_chk(void)
>> > +{
>> > +     struct pci_dev *pch = NULL;
>> > +     unsigned short id;
>> > +
>> > +     while ((pch = pci_get_class(PCI_CLASS_BRIDGE_ISA << 8, pch))) {
>> > +             if (pch->vendor != PCI_VENDOR_ID_INTEL)
>> > +                     continue;
>> > +
>> > +             id = pch->device & INTEL_PCH_DEVICE_ID_MASK;
>> > +             if (id == INTEL_PCH_TGP_DEVICE_ID_TYPE || INTEL_PCH_TGP2_DEVICE_ID_TYPE)
>>
>> PS. This is always true. ;)
>
> got, thanks
>
>>
>> > +                     return true;
>> > +     }
>> > +
>> > +     return false;
>> > +}
>> > +
>> >  static void smu7_init_dpm_defaults(struct pp_hwmgr *hwmgr)
>> >  {
>> >       struct smu7_hwmgr *data = (struct smu7_hwmgr *)(hwmgr->backend);
>> > @@ -1758,7 +1777,7 @@ static void smu7_init_dpm_defaults(struct pp_hwmgr *hwmgr)
>> >
>> >       data->mclk_dpm_key_disabled = hwmgr->feature_mask & PP_MCLK_DPM_MASK ? false : true;
>> >       data->sclk_dpm_key_disabled = hwmgr->feature_mask & PP_SCLK_DPM_MASK ? false : true;
>> > -     data->pcie_dpm_key_disabled = hwmgr->feature_mask & PP_PCIE_DPM_MASK ? false : true;
>> > +     data->pcie_dpm_key_disabled = intel_tgp_chk() || !(hwmgr->feature_mask & PP_PCIE_DPM_MASK);
>> >       /* need to set voltage control types before EVV patching */
>> >       data->voltage_control = SMU7_VOLTAGE_CONTROL_NONE;
>> >       data->vddci_control = SMU7_VOLTAGE_CONTROL_NONE;
>>
>> --
>> Jani Nikula, Intel Open Source Graphics Center

-- 
Jani Nikula, Intel Open Source Graphics Center