[PATCH 2/2] drm/amdgpu: stop/resume fb access when gpu reset

Deucher, Alexander Alexander.Deucher at amd.com
Wed Jun 22 12:54:52 UTC 2016 

> -----Original Message-----
> From: amd-gfx [mailto:amd-gfx-bounces at lists.freedesktop.org] On Behalf
> Of zhoucm1
> Sent: Wednesday, June 22, 2016 2:02 AM
> To: Deucher, Alexander; Alex Deucher
> Cc: amd-gfx at lists.freedesktop.org
> Subject: Re: [PATCH 2/2] drm/amdgpu: stop/resume fb access when gpu
> reset
> 
> 
> 
> On 2016年06月22日 11:58, Deucher, Alexander wrote:
> >> -----Original Message-----
> >> From: amd-gfx [mailto:amd-gfx-bounces at lists.freedesktop.org] On
> Behalf
> >> Of zhoucm1
> >> Sent: Tuesday, June 21, 2016 9:51 PM
> >> To: Alex Deucher; Deucher, Alexander
> >> Cc: amd-gfx at lists.freedesktop.org
> >> Subject: Re: [PATCH 2/2] drm/amdgpu: stop/resume fb access when gpu
> >> reset
> >>
> >>
> >>
> >> On 2016年06月21日 23:25, Alex Deucher wrote:
> >>> On Tue, Jun 21, 2016 at 9:21 AM, Deucher, Alexander
> >>> <Alexander.Deucher at amd.com> wrote:
> >>>>> -----Original Message-----
> >>>>> From: amd-gfx [mailto:amd-gfx-bounces at lists.freedesktop.org] On
> >> Behalf
> >>>>> Of Chunming Zhou
> >>>>> Sent: Tuesday, June 21, 2016 4:12 AM
> >>>>> To: amd-gfx at lists.freedesktop.org
> >>>>> Cc: Zhou, David(ChunMing)
> >>>>> Subject: [PATCH 2/2] drm/amdgpu: stop/resume fb access when gpu
> >> reset
> >>>>> Change-Id: I86b5c2b8c78a17ef43ade2a97ef8a33853650be0
> >>>>> Signed-off-by: Chunming Zhou <David1.Zhou at amd.com>
> >>>>> ---
> >>>>>    drivers/gpu/drm/amd/amdgpu/cik.c | 9 +++++++++
> >>>>>    drivers/gpu/drm/amd/amdgpu/vi.c  | 8 +++++++-
> >>>>>    2 files changed, 16 insertions(+), 1 deletion(-)
> >>>>>
> >>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/cik.c
> >>>>> b/drivers/gpu/drm/amd/amdgpu/cik.c
> >>>>> index 40f4fda..716efa8 100644
> >>>>> --- a/drivers/gpu/drm/amd/amdgpu/cik.c
> >>>>> +++ b/drivers/gpu/drm/amd/amdgpu/cik.c
> >>>>> @@ -1213,10 +1213,19 @@ static void
> >>>>> cik_set_bios_scratch_engine_hung(struct amdgpu_device *adev,
> bool
> >> hu
> >>>>>    static int cik_asic_reset(struct amdgpu_device *adev)
> >>>>>    {
> >>>>>         int r;
> >>>>> +     struct amdgpu_mode_mc_save save;
> >>>>> +
> >>>>>         cik_set_bios_scratch_engine_hung(adev, true);
> >>>>> +     /* Disable fb access */
> >>>>> +     if (adev->mode_info.num_crtc)
> >>>>> +             amdgpu_display_stop_mc_access(adev, &save);
> >>>> You don't need to check if there are crtcs or not before stopping the
> mc.
> >> The stop_mc_access and resume_mc_access callbacks will handle that
> >> automatically.  Also on chips with no DCE hardware, you still want to stop
> the
> >> mc.  Same for vi.c.
> >>> Nevermind, I was thinking of the actual gmc stop/resume mc functions.
> >>> So the code here with the num_crtc check is correct with respect to
> >>> disabling mc access in the dce block.  However, why do we need this?
> >> This sequence is actually by referring other drivers, they're doing it,
> >> the comment is:
> >>       // Disable VGA memory access if it is enabled and display requests
> >> to avoid any issue when resetting MC.
> > Ah, ok.  In that case, we should probably just call stop_mc_access.  That
> way the displays will stop requesting memory before the reset and the mc
> will be properly re-configured in gmc mc_program() which is called from gmc
> hw_init via gmc resume.
> >
> >>> The mc_stop sort of makes sense, but resuming after the reset doesn't
> >>> since the gpu state is reset after the reset so what's the point?
> >>> Wouldn't this already be taken care of by the ip specific suspend and
> >>> resume functions (e.g., gmc resume)?
> >> the amdgpu_display_stop/resume_mc_access are called by
> >> gmc_v8_0_mc_stop/resume, which are called in gmc_v8_0_soft_reset
> not in
> >> gmc_xxx_suspend/resume.
> >> do you mean adding gmc_xxx_mc_stop/resume to
> >> gmc_xxx_suspend/resume is
> >> making more sence?
> > I think all we need to do is stop the displays from accessing the mc.  No
> need call resume_mc_access() after the reset since the entire GPU will be
> reset and the dce block won't be back in shape until various atom tables as
> called.  Atom asic_init and amdgpu_resume() code should take care of that.
> Yeah, I see your mean, but stop_mc_access must be use by matching
> resume_mc_access(), since there is a parameter "struct
> amdgpu_mode_mc_save *save" in them. gmc mc_program has one pair of
> them.
> So I feel we still need to add resume_mc_access().

We can just ignore the saved values.  We'll be programming all of the hw again anyway.  Also, I think all you have to worry about is the displays in this case.  There's no value in restoring the old save state to the hw after reset since the GPU will be in the reset state so reprogramming a handful of registers won't really be that useful.  I don't think you need to do a full mc stop unless the hw team recommends it.

Alex

> 
> in addition to that, I add the wait_for_mc_idle after stop_mc_access
> when I gone through code again.
> 
> I will send this patch again with V2.
> 
> Regards,
> David Zhou
> >
> > Alex
> >
> >> Regards,
> >> David Zhou
> >>> Alex
> >>>
> >>>> Alex
> >>>>
> >>>>>         r = cik_gpu_pci_config_reset(adev);
> >>>>>
> >>>>> +     /* resume fb access */
> >>>>> +     if (adev->mode_info.num_crtc)
> >>>>> +             amdgpu_display_resume_mc_access(adev, &save);
> >>>>> +
> >>>>>         cik_set_bios_scratch_engine_hung(adev, false);
> >>>>>
> >>>>>         return r;
> >>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/vi.c
> >>>>> b/drivers/gpu/drm/amd/amdgpu/vi.c
> >>>>> index 1ac0c91..497b817 100644
> >>>>> --- a/drivers/gpu/drm/amd/amdgpu/vi.c
> >>>>> +++ b/drivers/gpu/drm/amd/amdgpu/vi.c
> >>>>> @@ -633,10 +633,16 @@ static void
> >> vi_set_bios_scratch_engine_hung(struct
> >>>>> amdgpu_device *adev, bool hun
> >>>>>    static int vi_asic_reset(struct amdgpu_device *adev)
> >>>>>    {
> >>>>>         int r;
> >>>>> +     struct amdgpu_mode_mc_save save;
> >>>>>
> >>>>>         vi_set_bios_scratch_engine_hung(adev, true);
> >>>>> -
> >>>>> +     /* Disable fb access */
> >>>>> +     if (adev->mode_info.num_crtc)
> >>>>> +             amdgpu_display_stop_mc_access(adev, &save);
> >>>>>         r = vi_gpu_pci_config_reset(adev);
> >>>>> +     /* resume fb access */
> >>>>> +     if (adev->mode_info.num_crtc)
> >>>>> +             amdgpu_display_resume_mc_access(adev, &save);
> >>>>>
> >>>>>         vi_set_bios_scratch_engine_hung(adev, false);
> >>>>>
> >>>>> --
> >>>>> 1.9.1
> >>>>>
> >>>>> _______________________________________________
> >>>>> amd-gfx mailing list
> >>>>> amd-gfx at lists.freedesktop.org
> >>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
> >>>> _______________________________________________
> >>>> amd-gfx mailing list
> >>>> amd-gfx at lists.freedesktop.org
> >>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
> >> _______________________________________________
> >> amd-gfx mailing list
> >> amd-gfx at lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
> 
> _______________________________________________
> amd-gfx mailing list
> amd-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

More information about the amd-gfx mailing list