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

[Quan, Evan]

[Public]



> -----Original Message-----
> From: Lazar, Lijo <Lijo.Lazar at amd.com>
> Sent: Monday, June 7, 2021 4:19 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
> 
> [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).
[Quan, Evan] Sure. 
Thanks,
Evan
> 
> > > +		 * 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))
> > >