<html xmlns:v="urn:schemas-microsoft-com:vml" xmlns:o="urn:schemas-microsoft-com:office:office" xmlns:w="urn:schemas-microsoft-com:office:word" xmlns:m="http://schemas.microsoft.com/office/2004/12/omml" xmlns="http://www.w3.org/TR/REC-html40"> <head> <meta http-equiv="Content-Type" content="text/html; charset=us-ascii"> <meta name="Generator" content="Microsoft Word 15 (filtered medium)"> <style></style> </head> <body lang="EN-US" link="#0563C1" vlink="#954F72" style="word-wrap:break-word"> [AMD Official Use Only - General] <div class="WordSection1"> Hi, Luben<o:p></o:p> <o:p> </o:p> Thanks for the review. Please see inline.<o:p></o:p> <o:p> </o:p> Best Regards,<o:p></o:p> Alan<o:p></o:p> <o:p> </o:p> -----Original Message----- From: Tuikov, Luben <Luben.Tuikov@amd.com> Sent: Tuesday, March 28, 2023 3:00 AM To: Liu, HaoPing (Alan) <HaoPing.Liu@amd.com>; amd-gfx@lists.freedesktop.org Cc: Lakha, Bhawanpreet <Bhawanpreet.Lakha@amd.com> Subject: Re: [PATCH] drm/amdgpu: Fix desktop freezed after gpu-reset<o:p></o:p> <o:p> </o:p> Hi,<o:p></o:p> <o:p> </o:p> That's a good fix. Some questions and comments below:<o:p></o:p> <o:p> </o:p> On 2023-03-27 11:20, Alan Liu wrote:<o:p></o:p> > [Why]<o:p></o:p> > After gpu-reset, sometimes the driver would fail to enable vblank irq, <o:p></o:p> > causing flip_done timed out and the desktop freezed.<o:p></o:p> > <o:p></o:p> > During gpu-reset, we will disable and enable vblank irq in <o:p></o:p> > dm_suspend() and dm_resume(). Later on in <o:p></o:p> > amdgpu_irq_gpu_reset_resume_helper(), we will check irqs' refcount and decide to enable or disable the irqs again.<o:p></o:p> > <o:p></o:p> > However, we have 2 sets of API for controling vblank irq, one is<o:p></o:p> > dm_vblank_get/put() and another is amdgpu_irq_get/put(). Each API has <o:p></o:p> > its own refcount and flag to store the state of vblank irq, and they <o:p></o:p> > are not synchronized.<o:p></o:p> <o:p> </o:p> Is it possible to reconcile controlling VBlank IRQ to a single refcount?<o:p></o:p> <o:p> </o:p> In struct drm_vblank_crtc, we have “enabled” and “refcount” to store vblank irq state, and in struct amdgpu_irq_src we have “enabled_types” as the refcount for each irq in dm layer.<o:p></o:p> To reconcile vblank irq to a single refcount, my idea is to remove enabled and refcount from struct drm_vblank_crtc, and add a callback function like vblank_irq_enabled() to drm_crtc_funcs.<o:p></o:p> Drm layer can use this function to check the state or refcount of vblank irq from dm layer. But it may be dangerous because it is a change to drm layer. Do you have any comments?<o:p></o:p> <o:p> </o:p> > <o:p></o:p> > In drm we use the first API to control vblank irq but in<o:p></o:p> > amdgpu_irq_gpu_reset_resume_helper() we use the second set of API.<o:p></o:p> > <o:p></o:p> > The failure happens when vblank irq was enabled by dm_vblank_get() <o:p></o:p> > before gpu-reset, we have vblank->enabled true. However, during <o:p></o:p> > gpu-reset, in amdgpu_irq_gpu_reset_resume_helper(), vblank irq's state <o:p></o:p> > checked from<o:p></o:p> > amdgpu_irq_update() is DISABLED. So finally it will disable vblank irq <o:p></o:p> > again. After gpu-reset, if there is a cursor plane commit, the driver <o:p></o:p> > will try to enable vblank irq by calling drm_vblank_enable(), but the<o:p></o:p> > vblank->enabled is still true, so it fails to turn on vblank irq and<o:p></o:p> > causes flip_done can't be completed in vblank irq handler and desktop <o:p></o:p> > become freezed.<o:p></o:p> > <o:p></o:p> > [How]<o:p></o:p> > Combining the 2 vblank control APIs by letting drm's API finally calls <o:p></o:p> > amdgpu_irq's API, so the irq's refcount and state of both APIs can be <o:p></o:p> > synchronized. Also add a check to prevent refcount from being less <o:p></o:p> > then<o:p></o:p> > 0 in amdgpu_irq_put().<o:p></o:p> > <o:p></o:p> > Signed-off-by: Alan Liu <<a href="mailto:HaoPing.Liu@amd.com">HaoPing.Liu@amd.com</a>><o:p></o:p> > ---<o:p></o:p> > drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c | 3 +++<o:p></o:p> > .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crtc.c | 14 <o:p></o:p> > ++++++++++----<o:p></o:p> > 2 files changed, 13 insertions(+), 4 deletions(-)<o:p></o:p> > <o:p></o:p> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c <o:p> </o:p> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c<o:p></o:p> > index a6aef488a822..1b66003657e2 100644<o:p></o:p> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c<o:p></o:p> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c<o:p></o:p> > @@ -597,6 +597,9 @@ int amdgpu_irq_put(struct amdgpu_device *adev, struct amdgpu_irq_src *src,<o:p></o:p> > if (!src->enabled_types || !src->funcs->set)<o:p></o:p> > return -EINVAL;<o:p></o:p> > <o:p></o:p> > + if (!amdgpu_irq_enabled(adev, src, type))<o:p></o:p> > + return 0;<o:p></o:p> > +<o:p></o:p> > if (atomic_dec_and_test(&src->enabled_types[type]))<o:p></o:p> > return amdgpu_irq_update(adev, src, type);<o:p></o:p> > <o:p></o:p> > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crtc.c <o:p></o:p> > b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crtc.c<o:p></o:p> > index dc4f37240beb..e04f846b0b19 100644<o:p></o:p> > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crtc.c<o:p></o:p> > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crtc.c<o:p></o:p> > @@ -146,7 +146,7 @@ static void vblank_control_worker(struct <o:p></o:p> > work_struct *work)<o:p></o:p> > <o:p></o:p> > static inline int dm_set_vblank(struct drm_crtc *crtc, bool enable) <o:p></o:p> > {<o:p></o:p> > - enum dc_irq_source irq_source;<o:p></o:p> > + int irq_type;<o:p></o:p> > struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);<o:p></o:p> > struct amdgpu_device *adev = drm_to_adev(crtc->dev);<o:p></o:p> > struct dm_crtc_state *acrtc_state = to_dm_crtc_state(crtc->state); <o:p></o:p> > @@ -169,10 +169,16 @@ static inline int dm_set_vblank(struct drm_crtc *crtc, bool enable)<o:p></o:p> > if (rc)<o:p></o:p> > return rc;<o:p></o:p> > <o:p></o:p> > - irq_source = IRQ_TYPE_VBLANK + acrtc->otg_inst;<o:p></o:p> > + irq_type = amdgpu_display_crtc_idx_to_irq_type(adev, <o:p></o:p> > +acrtc->crtc_id);<o:p></o:p> > +<o:p></o:p> > + if (enable)<o:p></o:p> > + rc = amdgpu_irq_get(adev, &adev->crtc_irq, irq_type);<o:p></o:p> > +<o:p></o:p> > + else<o:p></o:p> <o:p> </o:p> There's an unnecessary empty line before the "else". It's a good idea to run patches through scripts/checkpatch.pl.<o:p></o:p> <o:p> </o:p> Thanks, will use the tool next time.<o:p></o:p> <o:p> </o:p> > + rc = amdgpu_irq_put(adev, &adev->crtc_irq, irq_type);<o:p></o:p> > <o:p></o:p> > - if (!dc_interrupt_set(adev->dm.dc, irq_source, enable))<o:p></o:p> > - return -EBUSY;<o:p></o:p> > + if (rc)<o:p></o:p> > + return rc;<o:p></o:p> > <o:p></o:p> > skip:<o:p></o:p> > if (amdgpu_in_reset(adev))<o:p></o:p> <o:p> </o:p> --<o:p></o:p> Regards,<o:p></o:p> Luben<o:p></o:p> <o:p> </o:p> </div> </body> </html>