> -----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. 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
