From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?UTF-8?Q?Christian_K=c3=b6nig?= Subject: Re: [PATCH] drm/amdgpu: fix ring test failure issue during s3 in vce 3.0 Date: Tue, 28 May 2019 09:23:40 +0200 Message-ID: <3763ca45-b90e-dac9-f2a7-35aee4dc2548@gmail.com> References: <1558942936-16519-1-git-send-email-shirish.s@amd.com> <70c3ff3e-1f14-fcd6-e533-8e224fe0b976@amd.com> Reply-To: christian.koenig-5C7GfCeVMHo@public.gmane.org Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0781689964==" Return-path: In-Reply-To: Content-Language: en-US List-Id: Discussion list for AMD gfx List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: amd-gfx-bounces-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org Sender: "amd-gfx" To: "Li, Ching-shih (Louis)" , "Liu, Leo" , "S, Shirish" , "Grodzovsky, Andrey" , "Zhang, Jerry" , "Deng, Emily" , "Deucher, Alexander" Cc: "amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org" This is a multi-part message in MIME format. --===============0781689964== Content-Type: multipart/alternative; boundary="------------DE0E2CF00C82A2CFA063BC64" Content-Language: en-US This is a multi-part message in MIME format. --------------DE0E2CF00C82A2CFA063BC64 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Wow, really good catch! The underlying problem is most likely that VCE block is either power or clock gated and because of this the readptr read always returns zero. Now amdgpu_ring_alloc() informs the power management code that the block is about to be used and so the gating is turned off. Mhm, that is probably wrong at a hole bunch of other places, at least the UVD and VCN code comes to mind. I agree with Leo that you should remove the original read (so to not read twice) and it would be realy nice if you could double check the other code (UVD/VCN) for similar problems as well. Regards, Christian. Am 27.05.19 um 19:20 schrieb Li, Ching-shih (Louis): > > I don’t mean to read it twice. The solution is to make first read > later. I didn’t modify the original code to make code difference less > and simple. I guess it should work to remove the original read there. > > *From:*Liu, Leo > *Sent:* Tuesday, May 28, 2019 12:40 AM > *To:* Li, Ching-shih (Louis) ; S, Shirish > ; Grodzovsky, Andrey ; > Zhang, Jerry ; Deng, Emily ; > Deucher, Alexander > *Cc:* amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org > *Subject:* Re: [PATCH] drm/amdgpu: fix ring test failure issue during > s3 in vce 3.0 > > int amdgpu_vce_ring_test_ring(struct amdgpu_ring *ring) > { >     struct amdgpu_device *adev = ring->adev; > >     uint32_t rptr = amdgpu_ring_get_rptr(ring); > >     unsigned i; >     int r, timeout = adev->usec_timeout; > >     /* skip ring test for sriov*/ >     if (amdgpu_sriov_vf(adev)) >         return 0; > >     r = amdgpu_ring_alloc(ring, 16); >     if (r) >         return r; > >     amdgpu_ring_write(ring, VCE_CMD_END); >     amdgpu_ring_commit(ring); > > Above is original code, rptr is updated when called, and below is your > patch, my question is why do you need to get rptr twice? > > @@ -1084,6 +1084,8 @@ int amdgpu_vce_ring_test_ring(struct amdgpu_ring *ring) >        if (r) >               return r; > > +       rptr = amdgpu_ring_get_rptr(ring); > + >        amdgpu_ring_write(ring, VCE_CMD_END); >        amdgpu_ring_commit(ring); > > > On 5/27/19 12:22 PM, Li, Ching-shih (Louis) wrote: > > Hi Leo, > > Yes, I confirm it is the root cause *for the Chrome S3 issue*. > Whenever system is resumed, the original instruction always gets > zero. However, I have no idea why it fails, and didn’t verify this > problem on CRB or any other Linux platform yet. > > Although I think the ideal solution is an indicator, e.g. a > register, for driver to check if related firmware and hardware are > ready to work. So driver can make sure it is ok to read rptr. > Without any reference document, I can only try to solve the > problem by modifying driver. Debug traces reveal that only first > rptr read fails, but the read in check loop is ok. Therefore, a > solution comes to mind: to update rptr later for initial rptr > value. Tests prove it working in Chrome platforms. Fyi~ > > BR, > > Louis > > *From:*Liu, Leo > *Sent:* Monday, May 27, 2019 9:01 PM > *To:* S, Shirish ; > Grodzovsky, Andrey > ; Zhang, Jerry > ; Deng, Emily > ; Deucher, > Alexander > > *Cc:* amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org > ; Li, Ching-shih (Louis) > > *Subject:* Re: [PATCH] drm/amdgpu: fix ring test failure issue > during s3 in vce 3.0 > > On 5/27/19 3:42 AM, S, Shirish wrote: > > From: Louis Li > > > > [What] > > vce ring test fails consistently during resume in s3 cycle, due to > > mismatch read & write pointers. > > On debug/analysis its found that rptr to be compared is not being > > correctly updated/read, which leads to this failure. > > Below is the failure signature: > >   [drm:amdgpu_vce_ring_test_ring] *ERROR* amdgpu: ring 12 test failed > >   [drm:amdgpu_device_ip_resume_phase2] *ERROR* resume of IP block failed -110 > >   [drm:amdgpu_device_resume] *ERROR* amdgpu_device_ip_resume failed (-110). > > > > [How] > > fetch rptr appropriately, meaning move its read location further down > > in the code flow. > > With this patch applied the s3 failure is no more seen for >5k s3 cycles, > > which otherwise is pretty consistent. > > > > Signed-off-by: Louis Li > > --- > > drivers/gpu/drm/amd/amdgpu/amdgpu_vce.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vce.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vce.c > > index c021b11..92f9d46 100644 > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vce.c > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vce.c > > @@ -1084,6 +1084,8 @@ int amdgpu_vce_ring_test_ring(struct amdgpu_ring *ring) > >   if (r) > >         return r; > > > > + rptr = amdgpu_ring_get_rptr(ring); > > + > > The rptr update is there: > > |        uint32_t rptr = amdgpu_ring_get_rptr(ring);| > > || > > |Are you sure this is the root cause?| > > || > > |Regards,| > > |Leo| > > || > > > >   amdgpu_ring_write(ring, VCE_CMD_END); > >   amdgpu_ring_commit(ring); > > > > > _______________________________________________ > amd-gfx mailing list > amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org > https://lists.freedesktop.org/mailman/listinfo/amd-gfx --------------DE0E2CF00C82A2CFA063BC64 Content-Type: text/html; charset=utf-8 Content-Transfer-Encoding: 8bit
Wow, really good catch!

The underlying problem is most likely that VCE block is either power or clock gated and because of this the readptr read always returns zero.

Now amdgpu_ring_alloc() informs the power management code that the block is about to be used and so the gating is turned off.

Mhm, that is probably wrong at a hole bunch of other places, at least the UVD and VCN code comes to mind.

I agree with Leo that you should remove the original read (so to not read twice) and it would be realy nice if you could double check the other code (UVD/VCN) for similar problems as well.

Regards,
Christian.

Am 27.05.19 um 19:20 schrieb Li, Ching-shih (Louis):

I don’t mean to read it twice. The solution is to make first read later. I didn’t modify the original code to make code difference less and simple. I guess it should work to remove the original read there.

 

 

From: Liu, Leo <Leo.Liu-5C7GfCeVMHo@public.gmane.org>
Sent: Tuesday, May 28, 2019 12:40 AM
To: Li, Ching-shih (Louis) <Ching-shih.Li-5C7GfCeVMHo@public.gmane.org>; S, Shirish <Shirish.S-5C7GfCeVMHo@public.gmane.org>; Grodzovsky, Andrey <Andrey.Grodzovsky-5C7GfCeVMHo@public.gmane.org>; Zhang, Jerry <Jerry.Zhang-5C7GfCeVMHo@public.gmane.org>; Deng, Emily <Emily.Deng-5C7GfCeVMHo@public.gmane.org>; Deucher, Alexander <Alexander.Deucher-5C7GfCeVMHo@public.gmane.org>
Cc: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
Subject: Re: [PATCH] drm/amdgpu: fix ring test failure issue during s3 in vce 3.0

 

int amdgpu_vce_ring_test_ring(struct amdgpu_ring *ring)
{
    struct amdgpu_device *adev = ring->adev;

    uint32_t rptr = amdgpu_ring_get_rptr(ring);

    unsigned i;
    int r, timeout = adev->usec_timeout;

    /* skip ring test for sriov*/
    if (amdgpu_sriov_vf(adev))
        return 0;

    r = amdgpu_ring_alloc(ring, 16);
    if (r)
        return r;

    amdgpu_ring_write(ring, VCE_CMD_END);
    amdgpu_ring_commit(ring);

 

Above is original code, rptr is updated when called, and below is your patch, my question is why do you need to get rptr twice?

@@ -1084,6 +1084,8 @@ int amdgpu_vce_ring_test_ring(struct amdgpu_ring *ring)
        if (r)
               return r;
 
+       rptr = amdgpu_ring_get_rptr(ring);
+
        amdgpu_ring_write(ring, VCE_CMD_END);
        amdgpu_ring_commit(ring);

 

On 5/27/19 12:22 PM, Li, Ching-shih (Louis) wrote:

Hi Leo,

 

Yes, I confirm it is the root cause for the Chrome S3 issue. Whenever system is resumed, the original instruction always gets zero. However, I have no idea why it fails, and didn’t verify this problem on CRB or any other Linux platform yet.

Although I think the ideal solution is an indicator, e.g. a register, for driver to check if related firmware and hardware are ready to work. So driver can make sure it is ok to read rptr. Without any reference document, I can only try to solve the problem by modifying driver. Debug traces reveal that only first rptr read fails, but the read in check loop is ok. Therefore, a solution comes to mind: to update rptr later for initial rptr value. Tests prove it working in Chrome platforms. Fyi~

 

BR,

Louis

 

From: Liu, Leo <Leo.Liu-5C7GfCeVMHo@public.gmane.org>
Sent: Monday, May 27, 2019 9:01 PM
To: S, Shirish <Shirish.S-5C7GfCeVMHo@public.gmane.org>; Grodzovsky, Andrey <Andrey.Grodzovsky-5C7GfCeVMHo@public.gmane.org>; Zhang, Jerry <Jerry.Zhang-5C7GfCeVMHo@public.gmane.org>; Deng, Emily <Emily.Deng-5C7GfCeVMHo@public.gmane.org>; Deucher, Alexander <Alexander.Deucher-5C7GfCeVMHo@public.gmane.org>
Cc: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org; Li, Ching-shih (Louis) <Ching-shih.Li-5C7GfCeVMHo@public.gmane.org>
Subject: Re: [PATCH] drm/amdgpu: fix ring test failure issue during s3 in vce 3.0

 

 

On 5/27/19 3:42 AM, S, Shirish wrote:

From: Louis Li <Ching-shih.Li-5C7GfCeVMHo@public.gmane.org>
 
[What]
vce ring test fails consistently during resume in s3 cycle, due to
mismatch read & write pointers.
On debug/analysis its found that rptr to be compared is not being
correctly updated/read, which leads to this failure.
Below is the failure signature:
  [drm:amdgpu_vce_ring_test_ring] *ERROR* amdgpu: ring 12 test failed
  [drm:amdgpu_device_ip_resume_phase2] *ERROR* resume of IP block <vce_v3_0> failed -110
  [drm:amdgpu_device_resume] *ERROR* amdgpu_device_ip_resume failed (-110).
 
[How]
fetch rptr appropriately, meaning move its read location further down
in the code flow.
With this patch applied the s3 failure is no more seen for >5k s3 cycles,
which otherwise is pretty consistent.
 
Signed-off-by: Louis Li <Ching-shih.Li-5C7GfCeVMHo@public.gmane.org>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vce.c | 2 ++
 1 file changed, 2 insertions(+)
 
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vce.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vce.c
index c021b11..92f9d46 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vce.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vce.c
@@ -1084,6 +1084,8 @@ int amdgpu_vce_ring_test_ring(struct amdgpu_ring *ring)
  if (r)
         return r;
 
+ rptr = amdgpu_ring_get_rptr(ring);
+

The rptr update is there:

        uint32_t rptr = amdgpu_ring_get_rptr(ring);
 
Are you sure this is the root cause?
 
Regards,
Leo
 

 

 
  amdgpu_ring_write(ring, VCE_CMD_END);
  amdgpu_ring_commit(ring);
 

_______________________________________________
amd-gfx mailing list
amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

--------------DE0E2CF00C82A2CFA063BC64-- --===============0781689964== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KYW1kLWdmeCBt YWlsaW5nIGxpc3QKYW1kLWdmeEBsaXN0cy5mcmVlZGVza3RvcC5vcmcKaHR0cHM6Ly9saXN0cy5m cmVlZGVza3RvcC5vcmcvbWFpbG1hbi9saXN0aW5mby9hbWQtZ2Z4 --===============0781689964==--