Re: [PATCH] drm/amdgpu: Fix crash on device remove/driver unload

[Andrey Grodzovsky <andrey.grodzovsky@xxxxxxx>]

On 2021-09-16 11:51 a.m., Lazar, Lijo wrote:
> On 9/16/2021 9:15 PM, Andrey Grodzovsky wrote:
> > On 2021-09-16 4:20 a.m., Lazar, Lijo wrote:
> > > A minor comment below.
> > >
> > > On 9/16/2021 1:11 AM, Andrey Grodzovsky wrote:
> > > > Crash:
> > > > BUG: unable to handle page fault for address: 00000000000010e1
> > > > RIP: 0010:vega10_power_gate_vce+0x26/0x50 [amdgpu]
> > > > Call Trace:
> > > > pp_set_powergating_by_smu+0x16a/0x2b0 [amdgpu]
> > > > amdgpu_dpm_set_powergating_by_smu+0x92/0xf0 [amdgpu]
> > > > amdgpu_dpm_enable_vce+0x2e/0xc0 [amdgpu]
> > > > vce_v4_0_hw_fini+0x95/0xa0 [amdgpu]
> > > > amdgpu_device_fini_hw+0x232/0x30d [amdgpu]
> > > > amdgpu_driver_unload_kms+0x5c/0x80 [amdgpu]
> > > > amdgpu_pci_remove+0x27/0x40 [amdgpu]
> > > > pci_device_remove+0x3e/0xb0
> > > > device_release_driver_internal+0x103/0x1d0
> > > > device_release_driver+0x12/0x20
> > > > pci_stop_bus_device+0x79/0xa0
> > > > pci_stop_and_remove_bus_device_locked+0x1b/0x30
> > > > remove_store+0x7b/0x90
> > > > dev_attr_store+0x17/0x30
> > > > sysfs_kf_write+0x4b/0x60
> > > > kernfs_fop_write_iter+0x151/0x1e0
> > > >
> > > > Why:
> > > > VCE/UVD had dependency on SMC block for their suspend but
> > > > SMC block is the first to do HW fini due to some constraints
> > > >
> > > > How:
> > > > Since the original patch was dealing with suspend issues
> > > > move the SMC block dependency back into suspend hooks as
> > > > was done in V1 of the original patches.
> > > > Keep flushing idle work both in suspend and HW fini seuqnces
> > > > since it's essential in both cases.
> > > >
> > > > Fixes:
> > > > 2178d3c189b9 drm/amdgpu: add missing cleanups for more ASICs on 
> > > > UVD/VCE suspend
> > > > ee6679aaa61c drm/amdgpu: add missing cleanups for Polaris12 UVD/VCE 
> > > > on suspend
> > > > Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@xxxxxxx>
> > > > ---
> > > >   drivers/gpu/drm/amd/amdgpu/uvd_v3_1.c | 24 ++++++++-------
> > > >   drivers/gpu/drm/amd/amdgpu/uvd_v4_2.c | 24 ++++++++-------
> > > >   drivers/gpu/drm/amd/amdgpu/uvd_v5_0.c | 24 ++++++++-------
> > > >   drivers/gpu/drm/amd/amdgpu/uvd_v7_0.c | 32 ++++++++++---------
> > > >   drivers/gpu/drm/amd/amdgpu/vce_v2_0.c | 19 +++++++-----
> > > >   drivers/gpu/drm/amd/amdgpu/vce_v3_0.c | 28 +++++++++--------
> > > >   drivers/gpu/drm/amd/amdgpu/vce_v4_0.c | 44 
> > > > ++++++++++++++-------------
> > > >   7 files changed, 105 insertions(+), 90 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/amd/amdgpu/uvd_v3_1.c 
> > > > b/drivers/gpu/drm/amd/amdgpu/uvd_v3_1.c
> > > > index 7232241e3bfb..0fef925b6602 100644
> > > > --- a/drivers/gpu/drm/amd/amdgpu/uvd_v3_1.c
> > > > +++ b/drivers/gpu/drm/amd/amdgpu/uvd_v3_1.c
> > > > @@ -698,6 +698,19 @@ static int uvd_v3_1_hw_fini(void *handle)
> > > >   {
> > > >       struct amdgpu_device *adev = (struct amdgpu_device *)handle;
> > > >   + cancel_delayed_work_sync(&adev->uvd.idle_work);
> > > > +
> > > > +    if (RREG32(mmUVD_STATUS) != 0)
> > > > +        uvd_v3_1_stop(adev);
> > > > +
> > > > +    return 0;
> > > > +}
> > > > +
> > > > +static int uvd_v3_1_suspend(void *handle)
> > > > +{
> > > > +    int r;
> > > > +    struct amdgpu_device *adev = (struct amdgpu_device *)handle;
> > > > +
> > > >       /*
> > > >        * Proper cleanups before halting the HW engine:
> > > >        *   - cancel the delayed idle work
> > > > @@ -722,17 +735,6 @@ static int uvd_v3_1_hw_fini(void *handle)
> > > >                                  AMD_CG_STATE_GATE);
> > > >       }
> > > >   -    if (RREG32(mmUVD_STATUS) != 0)
> > > > -        uvd_v3_1_stop(adev);
> > > > -
> > > > -    return 0;
> > > > -}
> > > > -
> > > > -static int uvd_v3_1_suspend(void *handle)
> > > > -{
> > > > -    int r;
> > > > -    struct amdgpu_device *adev = (struct amdgpu_device *)handle;
> > > > -
> > > >       r = uvd_v3_1_hw_fini(adev);
> > >
> > > "cosmetic change" comment - hw_fini is supposed to be the final tear 
> > > down call. So instead of suspend calling hw_fini, perhaps it makes 
> > > sense to read the other way - hw_fini just suspends the hardware?
> > >
> > > Thanks,
> > > Lijo
> >
> >
> > Not sure what you mean ?
>
> Now it is - suspend()-> calls hw_fini()
>
> What I meant is hw_fini() -> calls suspend() and that is read as "to 
> do hw_fini() only suspend the hardware and nothing extra is needed".
>
> In short implementation stays in suspend() and hw_fini() calls suspend().


Sorry, still confused, what about amdgpu_vce_suspend being called from 
vce_v4_0_suspend for example - we don't want that to be called from hw_fini.
Can you maybe show draft change of what you mean for one specific UVD or 
VCE version ?

Andrey


> Thanks,
> Lijo
>
> > Andrey
> >
> >
> > > >       if (r)
> > > >           return r;
> > > > diff --git a/drivers/gpu/drm/amd/amdgpu/uvd_v4_2.c 
> > > > b/drivers/gpu/drm/amd/amdgpu/uvd_v4_2.c
> > > > index 52d6de969f46..c108b8381795 100644
> > > > --- a/drivers/gpu/drm/amd/amdgpu/uvd_v4_2.c
> > > > +++ b/drivers/gpu/drm/amd/amdgpu/uvd_v4_2.c
> > > > @@ -212,6 +212,19 @@ static int uvd_v4_2_hw_fini(void *handle)
> > > >   {
> > > >       struct amdgpu_device *adev = (struct amdgpu_device *)handle;
> > > >   + cancel_delayed_work_sync(&adev->uvd.idle_work);
> > > > +
> > > > +    if (RREG32(mmUVD_STATUS) != 0)
> > > > +        uvd_v4_2_stop(adev);
> > > > +
> > > > +    return 0;
> > > > +}
> > > > +
> > > > +static int uvd_v4_2_suspend(void *handle)
> > > > +{
> > > > +    int r;
> > > > +    struct amdgpu_device *adev = (struct amdgpu_device *)handle;
> > > > +
> > > >       /*
> > > >        * Proper cleanups before halting the HW engine:
> > > >        *   - cancel the delayed idle work
> > > > @@ -236,17 +249,6 @@ static int uvd_v4_2_hw_fini(void *handle)
> > > >                                  AMD_CG_STATE_GATE);
> > > >       }
> > > >   -    if (RREG32(mmUVD_STATUS) != 0)
> > > > -        uvd_v4_2_stop(adev);
> > > > -
> > > > -    return 0;
> > > > -}
> > > > -
> > > > -static int uvd_v4_2_suspend(void *handle)
> > > > -{
> > > > -    int r;
> > > > -    struct amdgpu_device *adev = (struct amdgpu_device *)handle;
> > > > -
> > > >       r = uvd_v4_2_hw_fini(adev);
> > > >       if (r)
> > > >           return r;
> > > > diff --git a/drivers/gpu/drm/amd/amdgpu/uvd_v5_0.c 
> > > > b/drivers/gpu/drm/amd/amdgpu/uvd_v5_0.c
> > > > index db6d06758e4d..563493d1f830 100644
> > > > --- a/drivers/gpu/drm/amd/amdgpu/uvd_v5_0.c
> > > > +++ b/drivers/gpu/drm/amd/amdgpu/uvd_v5_0.c
> > > > @@ -210,6 +210,19 @@ static int uvd_v5_0_hw_fini(void *handle)
> > > >   {
> > > >       struct amdgpu_device *adev = (struct amdgpu_device *)handle;
> > > >   + cancel_delayed_work_sync(&adev->uvd.idle_work);
> > > > +
> > > > +    if (RREG32(mmUVD_STATUS) != 0)
> > > > +        uvd_v5_0_stop(adev);
> > > > +
> > > > +    return 0;
> > > > +}
> > > > +
> > > > +static int uvd_v5_0_suspend(void *handle)
> > > > +{
> > > > +    int r;
> > > > +    struct amdgpu_device *adev = (struct amdgpu_device *)handle;
> > > > +
> > > >       /*
> > > >        * Proper cleanups before halting the HW engine:
> > > >        *   - cancel the delayed idle work
> > > > @@ -234,17 +247,6 @@ static int uvd_v5_0_hw_fini(void *handle)
> > > >                                  AMD_CG_STATE_GATE);
> > > >       }
> > > >   -    if (RREG32(mmUVD_STATUS) != 0)
> > > > -        uvd_v5_0_stop(adev);
> > > > -
> > > > -    return 0;
> > > > -}
> > > > -
> > > > -static int uvd_v5_0_suspend(void *handle)
> > > > -{
> > > > -    int r;
> > > > -    struct amdgpu_device *adev = (struct amdgpu_device *)handle;
> > > > -
> > > >       r = uvd_v5_0_hw_fini(adev);
> > > >       if (r)
> > > >           return r;
> > > > diff --git a/drivers/gpu/drm/amd/amdgpu/uvd_v7_0.c 
> > > > b/drivers/gpu/drm/amd/amdgpu/uvd_v7_0.c
> > > > index b6e82d75561f..1fd9ca21a091 100644
> > > > --- a/drivers/gpu/drm/amd/amdgpu/uvd_v7_0.c
> > > > +++ b/drivers/gpu/drm/amd/amdgpu/uvd_v7_0.c
> > > > @@ -606,6 +606,23 @@ static int uvd_v7_0_hw_fini(void *handle)
> > > >   {
> > > >       struct amdgpu_device *adev = (struct amdgpu_device *)handle;
> > > >   + cancel_delayed_work_sync(&adev->uvd.idle_work);
> > > > +
> > > > +    if (!amdgpu_sriov_vf(adev))
> > > > +        uvd_v7_0_stop(adev);
> > > > +    else {
> > > > +        /* full access mode, so don't touch any UVD register */
> > > > +        DRM_DEBUG("For SRIOV client, shouldn't do anything.\n");
> > > > +    }
> > > > +
> > > > +    return 0;
> > > > +}
> > > > +
> > > > +static int uvd_v7_0_suspend(void *handle)
> > > > +{
> > > > +    int r;
> > > > +    struct amdgpu_device *adev = (struct amdgpu_device *)handle;
> > > > +
> > > >       /*
> > > >        * Proper cleanups before halting the HW engine:
> > > >        *   - cancel the delayed idle work
> > > > @@ -630,21 +647,6 @@ static int uvd_v7_0_hw_fini(void *handle)
> > > >                                  AMD_CG_STATE_GATE);
> > > >       }
> > > >   -    if (!amdgpu_sriov_vf(adev))
> > > > -        uvd_v7_0_stop(adev);
> > > > -    else {
> > > > -        /* full access mode, so don't touch any UVD register */
> > > > -        DRM_DEBUG("For SRIOV client, shouldn't do anything.\n");
> > > > -    }
> > > > -
> > > > -    return 0;
> > > > -}
> > > > -
> > > > -static int uvd_v7_0_suspend(void *handle)
> > > > -{
> > > > -    int r;
> > > > -    struct amdgpu_device *adev = (struct amdgpu_device *)handle;
> > > > -
> > > >       r = uvd_v7_0_hw_fini(adev);
> > > >       if (r)
> > > >           return r;
> > > > diff --git a/drivers/gpu/drm/amd/amdgpu/vce_v2_0.c 
> > > > b/drivers/gpu/drm/amd/amdgpu/vce_v2_0.c
> > > > index 84e488f189f5..67eb01fef789 100644
> > > > --- a/drivers/gpu/drm/amd/amdgpu/vce_v2_0.c
> > > > +++ b/drivers/gpu/drm/amd/amdgpu/vce_v2_0.c
> > > > @@ -481,6 +481,17 @@ static int vce_v2_0_hw_fini(void *handle)
> > > >   {
> > > >       struct amdgpu_device *adev = (struct amdgpu_device *)handle;
> > > >   + cancel_delayed_work_sync(&adev->vce.idle_work);
> > > > +
> > > > +    return 0;
> > > > +}
> > > > +
> > > > +static int vce_v2_0_suspend(void *handle)
> > > > +{
> > > > +    int r;
> > > > +    struct amdgpu_device *adev = (struct amdgpu_device *)handle;
> > > > +
> > > > +
> > > >       /*
> > > >        * Proper cleanups before halting the HW engine:
> > > >        *   - cancel the delayed idle work
> > > > @@ -504,14 +515,6 @@ static int vce_v2_0_hw_fini(void *handle)
> > > >                                  AMD_CG_STATE_GATE);
> > > >       }
> > > >   -    return 0;
> > > > -}
> > > > -
> > > > -static int vce_v2_0_suspend(void *handle)
> > > > -{
> > > > -    int r;
> > > > -    struct amdgpu_device *adev = (struct amdgpu_device *)handle;
> > > > -
> > > >       r = vce_v2_0_hw_fini(adev);
> > > >       if (r)
> > > >           return r;
> > > > diff --git a/drivers/gpu/drm/amd/amdgpu/vce_v3_0.c 
> > > > b/drivers/gpu/drm/amd/amdgpu/vce_v3_0.c
> > > > index 2a18c1e089fd..142e291983b4 100644
> > > > --- a/drivers/gpu/drm/amd/amdgpu/vce_v3_0.c
> > > > +++ b/drivers/gpu/drm/amd/amdgpu/vce_v3_0.c
> > > > @@ -492,6 +492,21 @@ static int vce_v3_0_hw_fini(void *handle)
> > > >       int r;
> > > >       struct amdgpu_device *adev = (struct amdgpu_device *)handle;
> > > >   + cancel_delayed_work_sync(&adev->vce.idle_work);
> > > > +
> > > > +    r = vce_v3_0_wait_for_idle(handle);
> > > > +    if (r)
> > > > +        return r;
> > > > +
> > > > +    vce_v3_0_stop(adev);
> > > > +    return vce_v3_0_set_clockgating_state(adev, AMD_CG_STATE_GATE);
> > > > +}
> > > > +
> > > > +static int vce_v3_0_suspend(void *handle)
> > > > +{
> > > > +    int r;
> > > > +    struct amdgpu_device *adev = (struct amdgpu_device *)handle;
> > > > +
> > > >       /*
> > > >        * Proper cleanups before halting the HW engine:
> > > >        *   - cancel the delayed idle work
> > > > @@ -515,19 +530,6 @@ static int vce_v3_0_hw_fini(void *handle)
> > > >                                  AMD_CG_STATE_GATE);
> > > >       }
> > > >   -    r = vce_v3_0_wait_for_idle(handle);
> > > > -    if (r)
> > > > -        return r;
> > > > -
> > > > -    vce_v3_0_stop(adev);
> > > > -    return vce_v3_0_set_clockgating_state(adev, AMD_CG_STATE_GATE);
> > > > -}
> > > > -
> > > > -static int vce_v3_0_suspend(void *handle)
> > > > -{
> > > > -    int r;
> > > > -    struct amdgpu_device *adev = (struct amdgpu_device *)handle;
> > > > -
> > > >       r = vce_v3_0_hw_fini(adev);
> > > >       if (r)
> > > >           return r;
> > > > diff --git a/drivers/gpu/drm/amd/amdgpu/vce_v4_0.c 
> > > > b/drivers/gpu/drm/amd/amdgpu/vce_v4_0.c
> > > > index 044cf9d74b85..226b79254db8 100644
> > > > --- a/drivers/gpu/drm/amd/amdgpu/vce_v4_0.c
> > > > +++ b/drivers/gpu/drm/amd/amdgpu/vce_v4_0.c
> > > > @@ -544,29 +544,8 @@ static int vce_v4_0_hw_fini(void *handle)
> > > >   {
> > > >       struct amdgpu_device *adev = (struct amdgpu_device *)handle;
> > > >   -    /*
> > > > -     * Proper cleanups before halting the HW engine:
> > > > -     *   - cancel the delayed idle work
> > > > -     *   - enable powergating
> > > > -     *   - enable clockgating
> > > > -     *   - disable dpm
> > > > -     *
> > > > -     * TODO: to align with the VCN implementation, move the
> > > > -     * jobs for clockgating/powergating/dpm setting to
> > > > -     * ->set_powergating_state().
> > > > -     */
> > > >       cancel_delayed_work_sync(&adev->vce.idle_work);
> > > >   -    if (adev->pm.dpm_enabled) {
> > > > -        amdgpu_dpm_enable_vce(adev, false);
> > > > -    } else {
> > > > -        amdgpu_asic_set_vce_clocks(adev, 0, 0);
> > > > -        amdgpu_device_ip_set_powergating_state(adev, 
> > > > AMD_IP_BLOCK_TYPE_VCE,
> > > > -                               AMD_PG_STATE_GATE);
> > > > -        amdgpu_device_ip_set_clockgating_state(adev, 
> > > > AMD_IP_BLOCK_TYPE_VCE,
> > > > -                               AMD_CG_STATE_GATE);
> > > > -    }
> > > > -
> > > >       if (!amdgpu_sriov_vf(adev)) {
> > > >           /* vce_v4_0_wait_for_idle(handle); */
> > > >           vce_v4_0_stop(adev);
> > > > @@ -596,6 +575,29 @@ static int vce_v4_0_suspend(void *handle)
> > > >           drm_dev_exit(idx);
> > > >       }
> > > >   +    /*
> > > > +     * Proper cleanups before halting the HW engine:
> > > > +     *   - cancel the delayed idle work
> > > > +     *   - enable powergating
> > > > +     *   - enable clockgating
> > > > +     *   - disable dpm
> > > > +     *
> > > > +     * TODO: to align with the VCN implementation, move the
> > > > +     * jobs for clockgating/powergating/dpm setting to
> > > > +     * ->set_powergating_state().
> > > > +     */
> > > > +    cancel_delayed_work_sync(&adev->vce.idle_work);
> > > > +
> > > > +    if (adev->pm.dpm_enabled) {
> > > > +        amdgpu_dpm_enable_vce(adev, false);
> > > > +    } else {
> > > > +        amdgpu_asic_set_vce_clocks(adev, 0, 0);
> > > > +        amdgpu_device_ip_set_powergating_state(adev, 
> > > > AMD_IP_BLOCK_TYPE_VCE,
> > > > +                               AMD_PG_STATE_GATE);
> > > > +        amdgpu_device_ip_set_clockgating_state(adev, 
> > > > AMD_IP_BLOCK_TYPE_VCE,
> > > > +                               AMD_CG_STATE_GATE);
> > > > +    }
> > > > +
> > > >       r = vce_v4_0_hw_fini(adev);
> > > >       if (r)
> > > >           return r;