On 3/26/2024 5:34 PM, Lazar, Lijo wrote:
>
>
> On 3/26/2024 2:59 PM, Lazar, Lijo wrote:
>>
>>
>> On 3/25/2024 3:45 PM, Ma Jun wrote:
>>> Optimize the code to add support for BAMACO mode checking
>>>
>>> Signed-off-by: Ma Jun <Jun.Ma2@xxxxxxx>
>>> ---
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 4 +-
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 74 +++++++++++++++----------
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c | 4 +-
>>> 3 files changed, 50 insertions(+), 32 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>> index 80b9642f2bc4..e267ac032a1c 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>> @@ -2734,7 +2734,7 @@ static int amdgpu_pmops_runtime_suspend(struct device *dev)
>>> drm_dev->switch_power_state = DRM_SWITCH_POWER_DYNAMIC_OFF;
>>> } else if (adev->pm.rpm_mode == AMDGPU_RUNPM_BOCO) {
>>> /* nothing to do */
>>> - } else if (adev->pm.rpm_mode == AMDGPU_RUNPM_BACO) {
>>> + } else if (adev->pm.rpm_mode >= AMDGPU_RUNPM_BACO) {
>>
>> This kind of checking doesn't work well if we have to add new RPM modes.
>> Instead use || or a wrapper.
>>
>
> A few more comments below.
>
>> Thanks,
>> Lijo
>>
>>> amdgpu_device_baco_enter(drm_dev);
>>> }
>>>
>>> @@ -2774,7 +2774,7 @@ static int amdgpu_pmops_runtime_resume(struct device *dev)
>>> * PCI core handles it for _PR3.
>>> */
>>> pci_set_master(pdev);
>>> - } else if (adev->pm.rpm_mode == AMDGPU_RUNPM_BACO) {
>>> + } else if (adev->pm.rpm_mode >= AMDGPU_RUNPM_BACO) {
>>> amdgpu_device_baco_exit(drm_dev);
>>> }
>>> ret = amdgpu_device_resume(drm_dev, false);
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>>> index a66d47865e3b..81bb0a2c8227 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>>> @@ -133,6 +133,7 @@ void amdgpu_register_gpu_instance(struct amdgpu_device *adev)
>>> int amdgpu_driver_load_kms(struct amdgpu_device *adev, unsigned long flags)
>>> {
>>> struct drm_device *dev;
>>> + int bamaco_support = 0;
>>> int r, acpi_status;
>>>
>>> dev = adev_to_drm(adev);
>>> @@ -150,38 +151,55 @@ int amdgpu_driver_load_kms(struct amdgpu_device *adev, unsigned long flags)
>>> }
>>>
>>> adev->pm.rpm_mode = AMDGPU_RUNPM_NONE;
>>> - if (amdgpu_device_supports_px(dev) &&
>>> - (amdgpu_runtime_pm != 0)) { /* enable PX as runtime mode */
>>> - adev->pm.rpm_mode = AMDGPU_RUNPM_PX;
>>> - dev_info(adev->dev, "Using ATPX for runtime pm\n");
>>> - } else if (amdgpu_device_supports_boco(dev) &&
>>> - (amdgpu_runtime_pm != 0)) { /* enable boco as runtime mode */
>>> - adev->pm.rpm_mode = AMDGPU_RUNPM_BOCO;
>>> - dev_info(adev->dev, "Using BOCO for runtime pm\n");
>>> - } else if (amdgpu_device_supports_baco(dev) &&
>>> - (amdgpu_runtime_pm != 0)) {
>>> - switch (adev->asic_type) {
>>> - case CHIP_VEGA20:
>>> - case CHIP_ARCTURUS:
>>> - /* enable BACO as runpm mode if runpm=1 */
>>> - if (amdgpu_runtime_pm > 0)
>>> - adev->pm.rpm_mode = AMDGPU_RUNPM_BACO;
>>> - break;
>>> - case CHIP_VEGA10:
>>> - /* enable BACO as runpm mode if noretry=0 */
>>> - if (!adev->gmc.noretry)
>>> + if (amdgpu_runtime_pm == 2) {
>>> + adev->pm.rpm_mode = AMDGPU_RUNPM_BAMACO;
>>> + dev_info(adev->dev, "Forcing BAMACO for runtime pm\n");
>>> + } else if (amdgpu_runtime_pm == 1) {
>>> + adev->pm.rpm_mode = AMDGPU_RUNPM_BACO;
>>> + dev_info(adev->dev, "Forcing BACO for runtime pm\n");
>
> The above two don't work if SOC itself doesn't have the support. The
> option should be considered as what to choose for runtime pm when both
> BACO/BAMACO are supported rather than forcing irrespective of SOC
> feature support.
>
> If the forced mode is not supported, driver may decide to fallback to
> auto mode or drop runpm altogether.
>
>>> + } else if (amdgpu_runtime_pm != 0) {
>
> Since it's refactored, explicitly do a check here for AUTO MODE instead
> of any non-zero value.
>
Thanks. I will fix it.
Maybe I should have split this patch into two patches.
One is for BAMACO mode check, the other one is for this function refactor.
Regards,
Ma Jun
> Thanks,
> Lijo
>
>>> + if (amdgpu_device_supports_px(dev)) { /* enable PX as runtime mode */
>>> + adev->pm.rpm_mode = AMDGPU_RUNPM_PX;
>>> + dev_info(adev->dev, "Using ATPX for runtime pm\n");
>>> + } else if (amdgpu_device_supports_boco(dev)) { /* enable boco as runtime mode */
>>> + adev->pm.rpm_mode = AMDGPU_RUNPM_BOCO;
>>> + dev_info(adev->dev, "Using BOCO for runtime pm\n");
>>> + } else {
>>> + bamaco_support = amdgpu_device_supports_baco(dev);
>>> +
>>> + if (!bamaco_support)
>>> + goto no_runtime_pm;
>>> +
>>> + switch (adev->asic_type) {
>>> + case CHIP_VEGA20:
>>> + case CHIP_ARCTURUS:
>>> + /* vega20 and arcturus don't support runtime pm */
>>> + break;
>>> + case CHIP_VEGA10:
>>> + /* enable BACO as runpm mode if noretry=0 */
>>> + if (!adev->gmc.noretry)
>>> + adev->pm.rpm_mode = AMDGPU_RUNPM_BACO;
>>> + break;
>>> + default:
>>> + /* enable BACO as runpm mode on CI+ */
>>> adev->pm.rpm_mode = AMDGPU_RUNPM_BACO;
>>> - break;
>>> - default:
>>> - /* enable BACO as runpm mode on CI+ */
>>> - adev->pm.rpm_mode = AMDGPU_RUNPM_BACO;
>>> - break;
>>> + break;
>>> + }
>>> + if (adev->pm.rpm_mode == AMDGPU_RUNPM_BACO) {
>>> + if (bamaco_support & MACO_SUPPORT) {
>>> + adev->pm.rpm_mode = AMDGPU_RUNPM_BAMACO;
>>> + dev_info(adev->dev, "Using BAMACO for runtime pm\n");
>>> + } else {
>>> + dev_info(adev->dev, "Using BACO for runtime pm\n");
>>> + }
>>> + }
>>> }
>>> -
>>> - if (adev->pm.rpm_mode == AMDGPU_RUNPM_BACO)
>>> - dev_info(adev->dev, "Using BACO for runtime pm\n");
>>> }
>>>
>>> +no_runtime_pm:
>>> + if (adev->pm.rpm_mode == AMDGPU_RUNPM_NONE)
>>> + dev_info(adev->dev, "NO pm mode for runtime pm\n");
>>> +
>>> /* Call ACPI methods: require modeset init
>>> * but failure is not fatal
>>> */
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
>>> index 94b310fdb719..b4702a7961ec 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
>>> @@ -2617,7 +2617,7 @@ static int psp_load_p2s_table(struct psp_context *psp)
>>> struct amdgpu_firmware_info *ucode =
>>> &adev->firmware.ucode[AMDGPU_UCODE_ID_P2S_TABLE];
>>>
>>> - if (adev->in_runpm && (adev->pm.rpm_mode == AMDGPU_RUNPM_BACO))
>>> + if (adev->in_runpm && (adev->pm.rpm_mode >= AMDGPU_RUNPM_BACO))
>>> return 0;
>>>
>>> if (amdgpu_ip_version(adev, MP0_HWIP, 0) == IP_VERSION(13, 0, 6)) {
>>> @@ -2647,7 +2647,7 @@ static int psp_load_smu_fw(struct psp_context *psp)
>>> * Skip SMU FW reloading in case of using BACO for runpm only,
>>> * as SMU is always alive.
>>> */
>>> - if (adev->in_runpm && (adev->pm.rpm_mode == AMDGPU_RUNPM_BACO))
>>> + if (adev->in_runpm && (adev->pm.rpm_mode >= AMDGPU_RUNPM_BACO))
>>> return 0;
>>>
>>> if (!ucode->fw || amdgpu_sriov_vf(psp->adev))
