[PATCH V2 3/5] drm/amdgpu: correct the audio function initial Dstate

[Lazar, Lijo]

[Public]

Thanks, that explains.  

You may modify the comment to something like " amdgpu_get_audio_func() makes a PMFW-aware D-state transition to update audio dev's D-state in PMFW" (now it gives the impression that function makes audio dev to stay in D0 state).

> > +		 * Via amdgpu_get_audio_func() below, the audio function
> > +		 * is guarded to be in D0 correctly.

Thanks,
Lijo

-----Original Message-----
From: Quan, Evan <Evan.Quan at amd.com> 
Sent: Monday, June 7, 2021 12:40 PM
To: Lazar, Lijo <Lijo.Lazar at amd.com>; amd-gfx at lists.freedesktop.org
Cc: Deucher, Alexander <Alexander.Deucher at amd.com>
Subject: RE: [PATCH V2 3/5] drm/amdgpu: correct the audio function initial Dstate

[AMD Official Use Only]



> -----Original Message-----
> From: Lazar, Lijo <Lijo.Lazar at amd.com>
> Sent: Friday, June 4, 2021 8:24 PM
> To: Quan, Evan <Evan.Quan at amd.com>; amd-gfx at lists.freedesktop.org
> Cc: Deucher, Alexander <Alexander.Deucher at amd.com>
> Subject: Re: [PATCH V2 3/5] drm/amdgpu: correct the audio function 
> initial Dstate
> 
> 
> 
> On 6/4/2021 3:28 PM, Evan Quan wrote:
> > On driver loading, the ASIC is in D0 state. The bundled audio 
> > function should be in the same state also.
> >
> > Change-Id: I136e196be7633e95883a7f6c33963f7583e9bad1
> > Signed-off-by: Evan Quan <evan.quan at amd.com>
> > ---
> > V1->V2:
> >    - Lijo: include the code in a seperate API for better readability
> >    - limit the change for Navi10 and later dGPUs
> >    - comments more about the background
> > ---
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 40
> +++++++++++++++++++++++++
> >   1 file changed, 40 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> > index c354ffa62483..e9f2161738d1 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> > @@ -123,6 +123,22 @@ void amdgpu_register_gpu_instance(struct
> amdgpu_device *adev)
> >   	mutex_unlock(&mgpu_info.mutex);
> >   }
> >
> > +static void amdgpu_get_audio_func(struct amdgpu_device *adev) {
> > +	struct pci_dev *p = NULL;
> > +
> > +	p = pci_get_domain_bus_and_slot(pci_domain_nr(adev->pdev-
> >bus),
> > +			adev->pdev->bus->number, 1);
> > +	if (p) {
> > +		pm_runtime_get_sync(&p->dev);
> > +
> > +		pm_runtime_mark_last_busy(&p->dev);
> > +		pm_runtime_put_autosuspend(&p->dev);
> > +
> > +		pci_dev_put(p);
> > +	}
> > +}
> > +
> >   /**
> >    * amdgpu_driver_load_kms - Main load function for KMS.
> >    *
> > @@ -212,9 +228,33 @@ int amdgpu_driver_load_kms(struct
> amdgpu_device *adev, unsigned long flags)
> >
> 	DPM_FLAG_MAY_SKIP_RESUME);
> >   		pm_runtime_use_autosuspend(dev->dev);
> >   		pm_runtime_set_autosuspend_delay(dev->dev, 5000);
> > +
> >   		pm_runtime_allow(dev->dev);
> > +
> >   		pm_runtime_mark_last_busy(dev->dev);
> >   		pm_runtime_put_autosuspend(dev->dev);
> > +
> > +		/*
> > +		 * For runpm implemented via BACO, PMFW will handle the
> > +		 * timing for BACO in and out:
> > +		 *   - put ASIC into BACO state only when both video and
> > +		 *     audio functions are in D3 state.
> > +		 *   - pull ASIC out of BACO state when either video or
> > +		 *     audio function is in D0 state.
> > +		 * Also, at startup, PMFW assumes both functions are in
> > +		 * D0 state.
> > +		 *
> > +		 * So if snd driver was loaded prior to amdgpu driver
> > +		 * and audio function was put into D3 state behind PMFW's
> > +		 * back, the BACO may not correctly kicks in and out.
> > +		 *
> > +		 * Via amdgpu_get_audio_func() below, the audio function
> > +		 * is guarded to be in D0 correctly.
> > +		 */
> 
> On a second look at the comments - should runpm _get on audio dev be 
> done before doing device_init (so that it's in D0 while FW is getting
> loaded) and put done here?
[Quan, Evan] It does not matter as long as all those are performed before  .runtime_suspend() kicks.
The issue around here is : if the audio dev is already in D3 state before PMFW alive, during .runtime_suspend there will be no Dstate transfer(D0->D3). Thus no interrupt will go to PMFW and PMFW will be not aware of the audio dev D3 state. With the proper audio dev D3 state, the baco state will never kick in.
Via the amdgpu_get_audio_func(), the audio dev is put into D0 state. Then on the succeeding .runtime_suspend(), there will be Dstate transfer(D0->D3) and thus interrupt for PMFW.

BR
Evan
> 
> Thanks,
> Lijo
> 
> > +		if (amdgpu_device_supports_baco(dev) &&
> > +		    !(adev->flags & AMD_IS_APU) &&
> > +		    (adev->asic_type >= CHIP_NAVI10))
> > +			amdgpu_get_audio_func(adev);
> >   	}
> >
> >   	if (amdgpu_acpi_smart_shift_update(dev,
> AMDGPU_SS_DRV_LOAD))
> >