> -----Original Message----- > From: Zhou, David(ChunMing) > Sent: Friday, July 15, 2016 2:37 AM > To: Deucher, Alexander; 'Christian König'; amd-gfx at lists.freedesktop.org > Subject: Re: [PATCH 01/10] drm/amdgpu: is_dislay_hung will be called by soft > reset > > > > On 2016å¹´07æ??14æ?¥ 00:15, Deucher, Alexander wrote: > >> -----Original Message----- > >> From: amd-gfx [mailto:amd-gfx-bounces at lists.freedesktop.org] On > Behalf > >> Of Christian König > >> Sent: Wednesday, July 13, 2016 9:01 AM > >> To: Zhou, David(ChunMing); amd-gfx at lists.freedesktop.org > >> Subject: Re: [PATCH 01/10] drm/amdgpu: is_dislay_hung will be called by > soft > >> reset > >> > >> The set looks good in general, but you use the same old approach we had > >> for radeon. Alex, what do you think about this? Should we stick with > >> that approach or rather start over? > >> > >> I think it would be better if we can get rid of the AMDGPU_RESET_ enum > >> and use a per IP block based function to figure out if a block is hung > >> or not. > >> > >> Then we can have a per block flag which tels us if a hung of this block > >> can be cleared by a soft reset or needs a hard reset. > >> > >> Then last but not least we reset only the hung blocks if possible. > > This should be implemented at the IP level, not as a new asic callback. We > just never flushed out the design since it wasn't working at the time. I think > we should add any new callbacks to the IP functions and then we should use > either the IP idle checks or add a new check_soft_reset callback to set the > flags. Then in amdgpu_device.c we could have a core soft_reset that just > walks the IP list and calls the callbacks. Something like: > > > > +static int amdgpu_soft_reset(struct amdgpu_device *adev) > > +{ > > + int i, r, failied = 0; > > + > > + for (i = 0; i < adev->num_ip_blocks; i++) { > > + if (!adev->ip_block_status[i].valid) > > + continue; > > + if (adev->ip_blocks[i].funcs->check_soft_reset((void > *)adev)) { > > + r = adev->ip_blocks[i].funcs->soft_reset((void > *)adev); > > + if (r) { > > + DRM_ERROR("soft_reset %d failed %d\n", i, > r); > > + failed = r; > > + } > > + } > > + } > > + > > + return failed; > > +} > > > > No need for pre/post callbacks, that call all be handled in the soft_reset > callback directly. > > I see your means, this way will have a problem, take gfx hang for example: > gfx block soft reset will pre--->reset--->post, then gfx will start to > run, but this will result in other blocks busy, at this time, the loop > could walk to other block soft reset, then trigger other block reset, > then gpu will dead off. > > So we still need to post engine after all soft reset at a time. Ok, got it. That makes sense. Alex > > Regards, > David Zhou > > > > Alex > > > >> Regards, > >> Christian. > >> > >> Am 13.07.2016 um 12:32 schrieb Chunming Zhou: > >>> Change-Id: Ie81f6b1124aeb4304f59f92282fe8b18fb06a693 > >>> Signed-off-by: Chunming Zhou <David1.Zhou at amd.com> > >>> --- > >>> drivers/gpu/drm/amd/amdgpu/dce_v10_0.c | 2 +- > >>> drivers/gpu/drm/amd/amdgpu/dce_v10_0.h | 2 +- > >>> drivers/gpu/drm/amd/amdgpu/dce_v11_0.c | 2 +- > >>> drivers/gpu/drm/amd/amdgpu/dce_v11_0.h | 2 +- > >>> drivers/gpu/drm/amd/amdgpu/dce_v8_0.c | 2 +- > >>> drivers/gpu/drm/amd/amdgpu/dce_v8_0.h | 2 +- > >>> drivers/gpu/drm/amd/dal/amdgpu_dm/amdgpu_dm.c | 6 +++--- > >>> 7 files changed, 9 insertions(+), 9 deletions(-) > >>> > >>> diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c > >> b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c > >>> index 7554dd7..a592029 100644 > >>> --- a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c > >>> +++ b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c > >>> @@ -532,7 +532,7 @@ static u32 dce_v10_0_hpd_get_gpio_reg(struct > >> amdgpu_device *adev) > >>> return mmDC_GPIO_HPD_A; > >>> } > >>> > >>> -static bool dce_v10_0_is_display_hung(struct amdgpu_device *adev) > >>> +bool dce_v10_0_is_display_hung(struct amdgpu_device *adev) > >>> { > >>> u32 crtc_hung = 0; > >>> u32 crtc_status[6]; > >>> diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.h > >> b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.h > >>> index 3947956..77a732c 100644 > >>> --- a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.h > >>> +++ b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.h > >>> @@ -32,5 +32,5 @@ void dce_v10_0_stop_mc_access(struct > >> amdgpu_device *adev, > >>> struct amdgpu_mode_mc_save *save); > >>> void dce_v10_0_resume_mc_access(struct amdgpu_device *adev, > >>> struct amdgpu_mode_mc_save *save); > >>> - > >>> +bool dce_v10_0_is_display_hung(struct amdgpu_device *adev); > >>> #endif > >>> diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c > >> b/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c > >>> index d9c9b88..e18bd20 100644 > >>> --- a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c > >>> +++ b/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c > >>> @@ -549,7 +549,7 @@ static u32 dce_v11_0_hpd_get_gpio_reg(struct > >> amdgpu_device *adev) > >>> return mmDC_GPIO_HPD_A; > >>> } > >>> > >>> -static bool dce_v11_0_is_display_hung(struct amdgpu_device *adev) > >>> +bool dce_v11_0_is_display_hung(struct amdgpu_device *adev) > >>> { > >>> u32 crtc_hung = 0; > >>> u32 crtc_status[6]; > >>> diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.h > >> b/drivers/gpu/drm/amd/amdgpu/dce_v11_0.h > >>> index dc6ff04..fa1884c 100644 > >>> --- a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.h > >>> +++ b/drivers/gpu/drm/amd/amdgpu/dce_v11_0.h > >>> @@ -32,5 +32,5 @@ void dce_v11_0_stop_mc_access(struct > >> amdgpu_device *adev, > >>> struct amdgpu_mode_mc_save *save); > >>> void dce_v11_0_resume_mc_access(struct amdgpu_device *adev, > >>> struct amdgpu_mode_mc_save *save); > >>> - > >>> +bool dce_v11_0_is_display_hung(struct amdgpu_device *adev); > >>> #endif > >>> diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c > >> b/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c > >>> index 14ac085..4863504 100644 > >>> --- a/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c > >>> +++ b/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c > >>> @@ -478,7 +478,7 @@ static u32 dce_v8_0_hpd_get_gpio_reg(struct > >> amdgpu_device *adev) > >>> return mmDC_GPIO_HPD_A; > >>> } > >>> > >>> -static bool dce_v8_0_is_display_hung(struct amdgpu_device *adev) > >>> +bool dce_v8_0_is_display_hung(struct amdgpu_device *adev) > >>> { > >>> u32 crtc_hung = 0; > >>> u32 crtc_status[6]; > >>> diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v8_0.h > >> b/drivers/gpu/drm/amd/amdgpu/dce_v8_0.h > >>> index 4bb72ab..53f643c 100644 > >>> --- a/drivers/gpu/drm/amd/amdgpu/dce_v8_0.h > >>> +++ b/drivers/gpu/drm/amd/amdgpu/dce_v8_0.h > >>> @@ -32,5 +32,5 @@ void dce_v8_0_stop_mc_access(struct > >> amdgpu_device *adev, > >>> struct amdgpu_mode_mc_save *save); > >>> void dce_v8_0_resume_mc_access(struct amdgpu_device *adev, > >>> struct amdgpu_mode_mc_save *save); > >>> - > >>> +bool dce_v8_0_is_display_hung(struct amdgpu_device *adev); > >>> #endif > >>> diff --git a/drivers/gpu/drm/amd/dal/amdgpu_dm/amdgpu_dm.c > >> b/drivers/gpu/drm/amd/dal/amdgpu_dm/amdgpu_dm.c > >>> index ca64158..0ea0956 100644 > >>> --- a/drivers/gpu/drm/amd/dal/amdgpu_dm/amdgpu_dm.c > >>> +++ b/drivers/gpu/drm/amd/dal/amdgpu_dm/amdgpu_dm.c > >>> @@ -1301,7 +1301,7 @@ static const struct amdgpu_display_funcs > >> dm_dce_v8_0_display_funcs = { > >>> .bandwidth_update = dm_bandwidth_update, /* called > >> unconditionally */ > >>> .vblank_get_counter = dm_vblank_get_counter,/* called > >> unconditionally */ > >>> .vblank_wait = NULL, > >>> - .is_display_hung = NULL, /* not called anywhere */ > >>> + .is_display_hung = dce_v8_0_is_display_hung, > >>> .backlight_set_level = > >>> dm_set_backlight_level,/* called unconditionally */ > >>> .backlight_get_level = > >>> @@ -1325,7 +1325,7 @@ static const struct amdgpu_display_funcs > >> dm_dce_v10_0_display_funcs = { > >>> .bandwidth_update = dm_bandwidth_update, /* called > >> unconditionally */ > >>> .vblank_get_counter = dm_vblank_get_counter,/* called > >> unconditionally */ > >>> .vblank_wait = NULL, > >>> - .is_display_hung = NULL, /* not called anywhere */ > >>> + .is_display_hung = dce_v10_0_is_display_hung, > >>> .backlight_set_level = > >>> dm_set_backlight_level,/* called unconditionally */ > >>> .backlight_get_level = > >>> @@ -1349,7 +1349,7 @@ static const struct amdgpu_display_funcs > >> dm_dce_v11_0_display_funcs = { > >>> .bandwidth_update = dm_bandwidth_update, /* called > >> unconditionally */ > >>> .vblank_get_counter = dm_vblank_get_counter,/* called > >> unconditionally */ > >>> .vblank_wait = NULL, > >>> - .is_display_hung = NULL, /* not called anywhere */ > >>> + .is_display_hung = dce_v11_0_is_display_hung, > >>> .backlight_set_level = > >>> dm_set_backlight_level,/* called unconditionally */ > >>> .backlight_get_level = > >> _______________________________________________ > >> amd-gfx mailing list > >> amd-gfx at lists.freedesktop.org > >> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
