On Wed, Nov 29, 2023 at 6:17 AM Lazar, Lijo <lijo.lazar@xxxxxxx> wrote:
>
>
>
> On 11/29/2023 12:22 AM, Mario Limonciello wrote:
> > As the module parameter can be used to control behavior, all parts
> > of the driver should obey what has been programmed by user or
> > detected by auto mode rather than what "can" be supported.
> >
>
> This is also not correct. You can very well disable runpm mode and the
> rest of the logic will still apply. A substitution like this doesn't
> work when runpm mode is disabled. Only in cases where the support check
> is used for runpm related logic, this replacement can be applied.
Right. There are cases where we use BACO for things besides runtime
pm. E.g., GPU reset.
Alex
>
> Thanks,
> Lijo
>
> > Drop calls to all functions that check for BACO/BOCO/PX runpm modes
> > and instead use the variable that is programmed when device is probed.
> >
> > Signed-off-by: Mario Limonciello <mario.limonciello@xxxxxxx>
> > ---
> > drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 7 +++--
> > drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 34 ++++++++++++----------
> > drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c | 3 +-
> > 3 files changed, 24 insertions(+), 20 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > index 1181fe4baf0f..8f7377b37f2f 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > @@ -1822,9 +1822,10 @@ static void amdgpu_switcheroo_set_state(struct pci_dev *pdev,
> > enum vga_switcheroo_state state)
> > {
> > struct drm_device *dev = pci_get_drvdata(pdev);
> > + struct amdgpu_device *adev = drm_to_adev(dev);
> > int r;
> >
> > - if (amdgpu_device_supports_px(dev) && state == VGA_SWITCHEROO_OFF)
> > + if (adev->pm.rpm_mode == AMDGPU_RUNPM_PX && state == VGA_SWITCHEROO_OFF)
> > return;
> >
> > if (state == VGA_SWITCHEROO_ON) {
> > @@ -4244,7 +4245,7 @@ int amdgpu_device_init(struct amdgpu_device *adev,
> > if ((adev->pdev->class >> 8) == PCI_CLASS_DISPLAY_VGA)
> > vga_client_register(adev->pdev, amdgpu_device_vga_set_decode);
> >
> > - px = amdgpu_device_supports_px(ddev);
> > + px = (adev->pm.rpm_mode == AMDGPU_RUNPM_PX);
> >
> > if (px || (!dev_is_removable(&adev->pdev->dev) &&
> > apple_gmux_detect(NULL, NULL)))
> > @@ -4392,7 +4393,7 @@ void amdgpu_device_fini_sw(struct amdgpu_device *adev)
> > kfree(adev->fru_info);
> > adev->fru_info = NULL;
> >
> > - px = amdgpu_device_supports_px(adev_to_drm(adev));
> > + px = (adev->pm.rpm_mode == AMDGPU_RUNPM_PX);
> >
> > if (px || (!dev_is_removable(&adev->pdev->dev) &&
> > apple_gmux_detect(NULL, NULL)))
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> > index e39f3a334c9d..12fb8398fb45 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> > @@ -2248,10 +2248,10 @@ static int amdgpu_pci_probe(struct pci_dev *pdev,
> >
> > if (adev->pm.rpm_mode != AMDGPU_RUNPM_NONE) {
> > /* only need to skip on ATPX */
> > - if (amdgpu_device_supports_px(ddev))
> > + if (adev->pm.rpm_mode == AMDGPU_RUNPM_PX)
> > dev_pm_set_driver_flags(ddev->dev, DPM_FLAG_NO_DIRECT_COMPLETE);
> > /* we want direct complete for BOCO */
> > - if (amdgpu_device_supports_boco(ddev))
> > + if (adev->pm.rpm_mode == AMDGPU_RUNPM_BOCO)
> > dev_pm_set_driver_flags(ddev->dev, DPM_FLAG_SMART_PREPARE |
> > DPM_FLAG_SMART_SUSPEND |
> > DPM_FLAG_MAY_SKIP_RESUME);
> > @@ -2284,7 +2284,8 @@ static int amdgpu_pci_probe(struct pci_dev *pdev,
> > * into D0 state. Then there will be a PMFW-aware D-state
> > * transition(D0->D3) on runpm suspend.
> > */
> > - if (amdgpu_device_supports_baco(ddev) &&
> > + if ((adev->pm.rpm_mode == AMDGPU_RUNPM_BACO ||
> > + adev->pm.rpm_mode == AMDGPU_RUNPM_BAMACO) &&
> > !(adev->flags & AMD_IS_APU) &&
> > (adev->asic_type >= CHIP_NAVI10))
> > amdgpu_get_secondary_funcs(adev);
> > @@ -2466,7 +2467,7 @@ static int amdgpu_pmops_prepare(struct device *dev)
> > /* Return a positive number here so
> > * DPM_FLAG_SMART_SUSPEND works properly
> > */
> > - if (amdgpu_device_supports_boco(drm_dev) &&
> > + if (adev->pm.rpm_mode == AMDGPU_RUNPM_BOCO &&
> > pm_runtime_suspended(dev))
> > return 1;
> >
> > @@ -2664,7 +2665,7 @@ static int amdgpu_pmops_runtime_suspend(struct device *dev)
> > }
> >
> > adev->in_runpm = true;
> > - if (amdgpu_device_supports_px(drm_dev))
> > + if (adev->pm.rpm_mode == AMDGPU_RUNPM_PX)
> > drm_dev->switch_power_state = DRM_SWITCH_POWER_CHANGING;
> >
> > /*
> > @@ -2674,7 +2675,7 @@ static int amdgpu_pmops_runtime_suspend(struct device *dev)
> > * platforms.
> > * TODO: this may be also needed for PX capable platform.
> > */
> > - if (amdgpu_device_supports_boco(drm_dev))
> > + if (adev->pm.rpm_mode == AMDGPU_RUNPM_BOCO)
> > adev->mp1_state = PP_MP1_STATE_UNLOAD;
> >
> > ret = amdgpu_device_prepare(drm_dev);
> > @@ -2683,15 +2684,15 @@ static int amdgpu_pmops_runtime_suspend(struct device *dev)
> > ret = amdgpu_device_suspend(drm_dev, false);
> > if (ret) {
> > adev->in_runpm = false;
> > - if (amdgpu_device_supports_boco(drm_dev))
> > + if (adev->pm.rpm_mode == AMDGPU_RUNPM_BOCO)
> > adev->mp1_state = PP_MP1_STATE_NONE;
> > return ret;
> > }
> >
> > - if (amdgpu_device_supports_boco(drm_dev))
> > + if (adev->pm.rpm_mode == AMDGPU_RUNPM_BOCO)
> > adev->mp1_state = PP_MP1_STATE_NONE;
> >
> > - if (amdgpu_device_supports_px(drm_dev)) {
> > + if (adev->pm.rpm_mode == AMDGPU_RUNPM_PX) {
> > /* Only need to handle PCI state in the driver for ATPX
> > * PCI core handles it for _PR3.
> > */
> > @@ -2700,9 +2701,10 @@ static int amdgpu_pmops_runtime_suspend(struct device *dev)
> > pci_ignore_hotplug(pdev);
> > pci_set_power_state(pdev, PCI_D3cold);
> > drm_dev->switch_power_state = DRM_SWITCH_POWER_DYNAMIC_OFF;
> > - } else if (amdgpu_device_supports_boco(drm_dev)) {
> > + } else if (adev->pm.rpm_mode == AMDGPU_RUNPM_BOCO) {
> > /* nothing to do */
> > - } else if (amdgpu_device_supports_baco(drm_dev)) {
> > + } else if (adev->pm.rpm_mode == AMDGPU_RUNPM_BACO ||
> > + adev->pm.rpm_mode == AMDGPU_RUNPM_BAMACO) {
> > amdgpu_device_baco_enter(drm_dev);
> > }
> >
> > @@ -2725,7 +2727,7 @@ static int amdgpu_pmops_runtime_resume(struct device *dev)
> > if (!pci_device_is_present(adev->pdev))
> > adev->no_hw_access = true;
> >
> > - if (amdgpu_device_supports_px(drm_dev)) {
> > + if (adev->pm.rpm_mode == AMDGPU_RUNPM_PX) {
> > drm_dev->switch_power_state = DRM_SWITCH_POWER_CHANGING;
> >
> > /* Only need to handle PCI state in the driver for ATPX
> > @@ -2737,22 +2739,22 @@ static int amdgpu_pmops_runtime_resume(struct device *dev)
> > if (ret)
> > return ret;
> > pci_set_master(pdev);
> > - } else if (amdgpu_device_supports_boco(drm_dev)) {
> > + } else if (adev->pm.rpm_mode == AMDGPU_RUNPM_BOCO) {
> > /* Only need to handle PCI state in the driver for ATPX
> > * PCI core handles it for _PR3.
> > */
> > pci_set_master(pdev);
> > - } else if (amdgpu_device_supports_baco(drm_dev)) {
> > + } else if (adev->pm.rpm_mode == AMDGPU_RUNPM_BACO) {
> > amdgpu_device_baco_exit(drm_dev);
> > }
> > ret = amdgpu_device_resume(drm_dev, false);
> > if (ret) {
> > - if (amdgpu_device_supports_px(drm_dev))
> > + if (adev->pm.rpm_mode == AMDGPU_RUNPM_PX)
> > pci_disable_device(pdev);
> > return ret;
> > }
> >
> > - if (amdgpu_device_supports_px(drm_dev))
> > + if (adev->pm.rpm_mode == AMDGPU_RUNPM_PX)
> > drm_dev->switch_power_state = DRM_SWITCH_POWER_ON;
> > adev->in_runpm = false;
> > return 0;
> > diff --git a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> > index f464610a959f..d7977185f4e2 100644
> > --- a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> > +++ b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> > @@ -1618,7 +1618,8 @@ static int smu_disable_dpms(struct smu_context *smu)
> > bool use_baco = !smu->is_apu &&
> > ((amdgpu_in_reset(adev) &&
> > (amdgpu_asic_reset_method(adev) == AMD_RESET_METHOD_BACO)) ||
> > - ((adev->in_runpm || adev->in_s4) && amdgpu_asic_supports_baco(adev)));
> > + ((adev->in_runpm || adev->in_s4) &&
> > + (adev->pm.rpm_mode == AMDGPU_RUNPM_BACO || adev->pm.rpm_mode == AMDGPU_RUNPM_BAMACO)));
> >
> > /*
> > * For SMU 13.0.0 and 13.0.7, PMFW will handle the DPM features(disablement or others)
