From mboxrd@z Thu Jan 1 00:00:00 1970 From: zhoucm1 Subject: Re: [PATCH 2/2] drm/amdgpu: export test ring debugfs interface Date: Thu, 11 May 2017 10:50:17 +0800 Message-ID: <5913D169.6000304@amd.com> References: <1494469757-25784-1-git-send-email-ray.huang@amd.com> <1494469757-25784-2-git-send-email-ray.huang@amd.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1457810368==" Return-path: In-Reply-To: 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: "Deucher, Alexander" , "Huang, Ray" , "'amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org'" , "Koenig, Christian" Cc: "Huan, Alvin" , "Wang, Ken" --===============1457810368== Content-Type: multipart/alternative; boundary="------------080203060206020701010705" --------------080203060206020701010705 Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 8bit On 2017年05月11日 10:41, Deucher, Alexander wrote: > > -----Original Message----- > > From: Deucher, Alexander > > Sent: Wednesday, May 10, 2017 10:38 PM > > To: Huang, Ray; amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org; Koenig, Christian > > Cc: Zhou, David(ChunMing); Wang, Ken; Huan, Alvin; Huang, Ray > > Subject: RE: [PATCH 2/2] drm/amdgpu: export test ring debugfs interface > > > > > -----Original Message----- > > > From: Huang Rui [mailto:ray.huang-5C7GfCeVMHo@public.gmane.org] > > > Sent: Wednesday, May 10, 2017 10:29 PM > > > To: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org; Deucher, Alexander; Koenig, > Christian > > > Cc: Zhou, David(ChunMing); Wang, Ken; Huan, Alvin; Huang, Ray > > > Subject: [PATCH 2/2] drm/amdgpu: export test ring debugfs interface > > > > > > Signed-off-by: Huang Rui > > > > Reviewed-by: Alex Deucher > > Not sure if it's important for this since it's debugfs, but we don't > have any sort of ring locks anymore (since everything should go > through the scheduler once it's started) so we could theoretically get > collisions if there are active apps using the GPU and you run these tests. Yes, agree. So patch#1 for ib test should use scheduler, but which could be a big change and Christian doesn't like to use scheduler for ib test case, consider this is only for test case, the problem isn't big:). btw: I think patch#2 isn't need if patch#1 is landed, since when you use this sys interface, the amdgpu driver must be loaded successfully, ring test is meaningless. Regards, David Zhou > > Alex > > > > > > --- > > > drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 44 > > > ++++++++++++++++++++++++++++-- > > > 1 file changed, 42 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > > > index 7becf11..3637396 100644 > > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > > > @@ -3625,14 +3625,54 @@ static int amdgpu_debugfs_test_ib(struct > > > seq_file *m, void *data) > > > return 0; > > > } > > > > > > +static int amdgpu_ring_tests(struct amdgpu_device *adev) > > > +{ > > > + unsigned i; > > > + int r = 0; > > > + > > > + for (i = 0; i < AMDGPU_MAX_RINGS; ++i) { > > > + struct amdgpu_ring *ring = adev->rings[i]; > > > + > > > + if (!ring || !ring->ready) > > > + continue; > > > + > > > + r = amdgpu_ring_test_ring(ring); > > > + if (r) { > > > + ring->ready = false; > > > + DRM_ERROR("amdgpu: failed to test ring %d > > > (%d).\n", > > > + i, r); > > > + } > > > + } > > > + > > > + return r; > > > +} > > > + > > > +static int amdgpu_debugfs_test_ring(struct seq_file *m, void *data) > > > +{ > > > + struct drm_info_node *node = (struct drm_info_node *) m- > > > >private; > > > + struct drm_device *dev = node->minor->dev; > > > + struct amdgpu_device *adev = dev->dev_private; > > > + int r = 0; > > > + > > > + seq_printf(m, "run ring test:\n"); > > > + r = amdgpu_ring_tests(adev); > > > + if (r) > > > + seq_printf(m, "ring tests failed (%d).\n", r); > > > + else > > > + seq_printf(m, "ring tests passed.\n"); > > > + > > > + return 0; > > > +} > > > + > > > static const struct drm_info_list > amdgpu_debugfs_test_ib_ring_list[] = { > > > - {"amdgpu_test_ib", &amdgpu_debugfs_test_ib} > > > + {"amdgpu_test_ib", &amdgpu_debugfs_test_ib}, > > > + {"amdgpu_test_ring", &amdgpu_debugfs_test_ring} > > > }; > > > > > > static int amdgpu_debugfs_test_ib_ring_init(struct amdgpu_device > > *adev) > > > { > > > return amdgpu_debugfs_add_files(adev, > > > - amdgpu_debugfs_test_ib_ring_list, > > > 1); > > > + amdgpu_debugfs_test_ib_ring_list, > > > 2); > > > } > > > > > > int amdgpu_debugfs_init(struct drm_minor *minor) > > > -- > > > 2.7.4 > --------------080203060206020701010705 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: 8bit

On 2017年05月11日 10:41, Deucher, Alexander wrote:
> -----Original Message-----
> From: Deucher, Alexander
> Sent: Wednesday, May 10, 2017 10:38 PM
> To: Huang, Ray; amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org; Koenig, Christian
> Cc: Zhou, David(ChunMing); Wang, Ken; Huan, Alvin; Huang, Ray
> Subject: RE: [PATCH 2/2] drm/amdgpu: export test ring debugfs interface
>
> > -----Original Message-----
> > From: Huang Rui [mailto:ray.huang-5C7GfCeVMHo@public.gmane.org]
> > Sent: Wednesday, May 10, 2017 10:29 PM
> > To: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org; Deucher, Alexander; Koenig, Christian
> > Cc: Zhou, David(ChunMing); Wang, Ken; Huan, Alvin; Huang, Ray
> > Subject: [PATCH 2/2] drm/amdgpu: export test ring debugfs interface
> >
> > Signed-off-by: Huang Rui <ray.huang-5C7GfCeVMHo@public.gmane.org>
>
> Reviewed-by: Alex Deucher <alexander.deucher-5C7GfCeVMHo@public.gmane.org>

Not sure if it's important for this since it's debugfs, but we don't have any sort of ring locks anymore (since everything should go through the scheduler once it's started) so we could theoretically get collisions if there are active apps using the GPU and you run these tests.
Yes, agree. So patch#1 for ib test should use scheduler, but which could be a big change and Christian doesn't like to use scheduler for ib test case, consider this is only for test case, the problem isn't big:).

btw: I think patch#2 isn't need if patch#1 is landed, since when you use this sys interface, the amdgpu driver must be loaded successfully, ring test is meaningless.

Regards,
David Zhou

Alex

>
> > ---
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 44
> > ++++++++++++++++++++++++++++--
> >  1 file changed, 42 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > index 7becf11..3637396 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > @@ -3625,14 +3625,54 @@ static int amdgpu_debugfs_test_ib(struct
> > seq_file *m, void *data)
> >      return 0;
> >  }
> >
> > +static int amdgpu_ring_tests(struct amdgpu_device *adev)
> > +{
> > +   unsigned i;
> > +   int r = 0;
> > +
> > +   for (i = 0; i < AMDGPU_MAX_RINGS; ++i) {
> > +           struct amdgpu_ring *ring = adev->rings[i];
> > +
> > +           if (!ring || !ring->ready)
> > +                   continue;
> > +
> > +           r = amdgpu_ring_test_ring(ring);
> > +           if (r) {
> > +                   ring->ready = false;
> > +                   DRM_ERROR("amdgpu: failed to test ring %d
> > (%d).\n",
> > +                             i, r);
> > +           }
> > +   }
> > +
> > +   return r;
> > +}
> > +
> > +static int amdgpu_debugfs_test_ring(struct seq_file *m, void *data)
> > +{
> > +   struct drm_info_node *node = (struct drm_info_node *) m-
> > >private;
> > +   struct drm_device *dev = node->minor->dev;
> > +   struct amdgpu_device *adev = dev->dev_private;
> > +   int r = 0;
> > +
> > +   seq_printf(m, "run ring test:\n");
> > +   r = amdgpu_ring_tests(adev);
> > +   if (r)
> > +           seq_printf(m, "ring tests failed (%d).\n", r);
> > +   else
> > +           seq_printf(m, "ring tests passed.\n");
> > +
> > +   return 0;
> > +}
> > +
> >  static const struct drm_info_list amdgpu_debugfs_test_ib_ring_list[] = {
> > -   {"amdgpu_test_ib", &amdgpu_debugfs_test_ib}
> > +   {"amdgpu_test_ib", &amdgpu_debugfs_test_ib},
> > +   {"amdgpu_test_ring", &amdgpu_debugfs_test_ring}
> >  };
> >
> >  static int amdgpu_debugfs_test_ib_ring_init(struct amdgpu_device
> *adev)
> >  {
> >      return amdgpu_debugfs_add_files(adev,
> > -                                   amdgpu_debugfs_test_ib_ring_list,
> > 1);
> > +                                   amdgpu_debugfs_test_ib_ring_list,
> > 2);
> >  }
> >
> >  int amdgpu_debugfs_init(struct drm_minor *minor)
> > --
> > 2.7.4

--------------080203060206020701010705-- --===============1457810368== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KYW1kLWdmeCBt YWlsaW5nIGxpc3QKYW1kLWdmeEBsaXN0cy5mcmVlZGVza3RvcC5vcmcKaHR0cHM6Ly9saXN0cy5m cmVlZGVza3RvcC5vcmcvbWFpbG1hbi9saXN0aW5mby9hbWQtZ2Z4Cg== --===============1457810368==--