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