[04/25] drm/vkms: Annotate vblank timer
diff mbox series

Message ID 20200707201229.472834-5-daniel.vetter@ffwll.ch
State New
Headers show
Series
  • dma-fence annotations, round 3
Related show

Commit Message

Daniel Vetter July 7, 2020, 8:12 p.m. UTC
This is needed to signal the fences from page flips, annotate it
accordingly. We need to annotate entire timer callback since if we get
stuck anywhere in there, then the timer stops, and hence fences stop.
Just annotating the top part that does the vblank handling isn't
enough.

Cc: linux-media@vger.kernel.org
Cc: linaro-mm-sig@lists.linaro.org
Cc: linux-rdma@vger.kernel.org
Cc: amd-gfx@lists.freedesktop.org
Cc: intel-gfx@lists.freedesktop.org
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Christian König <christian.koenig@amd.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
Cc: Haneen Mohammed <hamohammed.sa@gmail.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
---
 drivers/gpu/drm/vkms/vkms_crtc.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

Comments

Rodrigo Siqueira July 12, 2020, 10:27 p.m. UTC | #1
Hi,

Everything looks fine to me, I just noticed that the amdgpu patches did
not apply smoothly, however it was trivial to fix the issues.

Reviewed-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>

Melissa,
Since you are using vkms regularly, could you test this patch and review
it? Remember to add your Tested-by when you finish.

Thanks

On 07/07, Daniel Vetter wrote:
> This is needed to signal the fences from page flips, annotate it
> accordingly. We need to annotate entire timer callback since if we get
> stuck anywhere in there, then the timer stops, and hence fences stop.
> Just annotating the top part that does the vblank handling isn't
> enough.
> 
> Cc: linux-media@vger.kernel.org
> Cc: linaro-mm-sig@lists.linaro.org
> Cc: linux-rdma@vger.kernel.org
> Cc: amd-gfx@lists.freedesktop.org
> Cc: intel-gfx@lists.freedesktop.org
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Christian König <christian.koenig@amd.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> Cc: Haneen Mohammed <hamohammed.sa@gmail.com>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> ---
>  drivers/gpu/drm/vkms/vkms_crtc.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
> index ac85e17428f8..a53a40848a72 100644
> --- a/drivers/gpu/drm/vkms/vkms_crtc.c
> +++ b/drivers/gpu/drm/vkms/vkms_crtc.c
> @@ -1,5 +1,7 @@
>  // SPDX-License-Identifier: GPL-2.0+
>  
> +#include <linux/dma-fence.h>
> +
>  #include <drm/drm_atomic.h>
>  #include <drm/drm_atomic_helper.h>
>  #include <drm/drm_probe_helper.h>
> @@ -14,7 +16,9 @@ static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
>  	struct drm_crtc *crtc = &output->crtc;
>  	struct vkms_crtc_state *state;
>  	u64 ret_overrun;
> -	bool ret;
> +	bool ret, fence_cookie;
> +
> +	fence_cookie = dma_fence_begin_signalling();
>  
>  	ret_overrun = hrtimer_forward_now(&output->vblank_hrtimer,
>  					  output->period_ns);
> @@ -49,6 +53,8 @@ static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
>  			DRM_DEBUG_DRIVER("Composer worker already queued\n");
>  	}
>  
> +	dma_fence_end_signalling(fence_cookie);
> +
>  	return HRTIMER_RESTART;
>  }
>  
> -- 
> 2.27.0
>
Melissa Wen July 14, 2020, 9:57 a.m. UTC | #2
On 07/12, Rodrigo Siqueira wrote:
> Hi,
> 
> Everything looks fine to me, I just noticed that the amdgpu patches did
> not apply smoothly, however it was trivial to fix the issues.
> 
> Reviewed-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> 
> Melissa,
> Since you are using vkms regularly, could you test this patch and review
> it? Remember to add your Tested-by when you finish.
>
Hi,

I've applied the patch series, ran some tests on vkms, and found no
issues. I mean, things have remained stable.

Tested-by: Melissa Wen <melissa.srw@gmail.com>

> Thanks
> 
> On 07/07, Daniel Vetter wrote:
> > This is needed to signal the fences from page flips, annotate it
> > accordingly. We need to annotate entire timer callback since if we get
> > stuck anywhere in there, then the timer stops, and hence fences stop.
> > Just annotating the top part that does the vblank handling isn't
> > enough.
> > 
> > Cc: linux-media@vger.kernel.org
> > Cc: linaro-mm-sig@lists.linaro.org
> > Cc: linux-rdma@vger.kernel.org
> > Cc: amd-gfx@lists.freedesktop.org
> > Cc: intel-gfx@lists.freedesktop.org
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: Christian König <christian.koenig@amd.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > Cc: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> > Cc: Haneen Mohammed <hamohammed.sa@gmail.com>
> > Cc: Daniel Vetter <daniel@ffwll.ch>
> > ---
> >  drivers/gpu/drm/vkms/vkms_crtc.c | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
> > index ac85e17428f8..a53a40848a72 100644
> > --- a/drivers/gpu/drm/vkms/vkms_crtc.c
> > +++ b/drivers/gpu/drm/vkms/vkms_crtc.c
> > @@ -1,5 +1,7 @@
> >  // SPDX-License-Identifier: GPL-2.0+
> >  
> > +#include <linux/dma-fence.h>
> > +
> >  #include <drm/drm_atomic.h>
> >  #include <drm/drm_atomic_helper.h>
> >  #include <drm/drm_probe_helper.h>
> > @@ -14,7 +16,9 @@ static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
> >  	struct drm_crtc *crtc = &output->crtc;
> >  	struct vkms_crtc_state *state;
> >  	u64 ret_overrun;
> > -	bool ret;
> > +	bool ret, fence_cookie;
> > +
> > +	fence_cookie = dma_fence_begin_signalling();
> >  
> >  	ret_overrun = hrtimer_forward_now(&output->vblank_hrtimer,
> >  					  output->period_ns);
> > @@ -49,6 +53,8 @@ static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
> >  			DRM_DEBUG_DRIVER("Composer worker already queued\n");
> >  	}
> >  
> > +	dma_fence_end_signalling(fence_cookie);
> > +
> >  	return HRTIMER_RESTART;
> >  }
> >  
> > -- 
> > 2.27.0
> > 
> 
> -- 
> Rodrigo Siqueira
> https://siqueira.tech
Daniel Vetter July 14, 2020, 9:59 a.m. UTC | #3
On Tue, Jul 14, 2020 at 11:57 AM Melissa Wen <melissa.srw@gmail.com> wrote:
>
> On 07/12, Rodrigo Siqueira wrote:
> > Hi,
> >
> > Everything looks fine to me, I just noticed that the amdgpu patches did
> > not apply smoothly, however it was trivial to fix the issues.
> >
> > Reviewed-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> >
> > Melissa,
> > Since you are using vkms regularly, could you test this patch and review
> > it? Remember to add your Tested-by when you finish.
> >
> Hi,
>
> I've applied the patch series, ran some tests on vkms, and found no
> issues. I mean, things have remained stable.
>
> Tested-by: Melissa Wen <melissa.srw@gmail.com>

Did you test with CONFIG_PROVE_LOCKING enabled in the kernel .config?
Without that enabled, there's not really any change here, but with
that enabled there might be some lockdep splats in dmesg indicating a
problem.

Thanks, Daniel
>
> > Thanks
> >
> > On 07/07, Daniel Vetter wrote:
> > > This is needed to signal the fences from page flips, annotate it
> > > accordingly. We need to annotate entire timer callback since if we get
> > > stuck anywhere in there, then the timer stops, and hence fences stop.
> > > Just annotating the top part that does the vblank handling isn't
> > > enough.
> > >
> > > Cc: linux-media@vger.kernel.org
> > > Cc: linaro-mm-sig@lists.linaro.org
> > > Cc: linux-rdma@vger.kernel.org
> > > Cc: amd-gfx@lists.freedesktop.org
> > > Cc: intel-gfx@lists.freedesktop.org
> > > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > Cc: Christian König <christian.koenig@amd.com>
> > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > > Cc: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> > > Cc: Haneen Mohammed <hamohammed.sa@gmail.com>
> > > Cc: Daniel Vetter <daniel@ffwll.ch>
> > > ---
> > >  drivers/gpu/drm/vkms/vkms_crtc.c | 8 +++++++-
> > >  1 file changed, 7 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
> > > index ac85e17428f8..a53a40848a72 100644
> > > --- a/drivers/gpu/drm/vkms/vkms_crtc.c
> > > +++ b/drivers/gpu/drm/vkms/vkms_crtc.c
> > > @@ -1,5 +1,7 @@
> > >  // SPDX-License-Identifier: GPL-2.0+
> > >
> > > +#include <linux/dma-fence.h>
> > > +
> > >  #include <drm/drm_atomic.h>
> > >  #include <drm/drm_atomic_helper.h>
> > >  #include <drm/drm_probe_helper.h>
> > > @@ -14,7 +16,9 @@ static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
> > >     struct drm_crtc *crtc = &output->crtc;
> > >     struct vkms_crtc_state *state;
> > >     u64 ret_overrun;
> > > -   bool ret;
> > > +   bool ret, fence_cookie;
> > > +
> > > +   fence_cookie = dma_fence_begin_signalling();
> > >
> > >     ret_overrun = hrtimer_forward_now(&output->vblank_hrtimer,
> > >                                       output->period_ns);
> > > @@ -49,6 +53,8 @@ static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
> > >                     DRM_DEBUG_DRIVER("Composer worker already queued\n");
> > >     }
> > >
> > > +   dma_fence_end_signalling(fence_cookie);
> > > +
> > >     return HRTIMER_RESTART;
> > >  }
> > >
> > > --
> > > 2.27.0
> > >
> >
> > --
> > Rodrigo Siqueira
> > https://siqueira.tech
>
>
Melissa Wen July 14, 2020, 2:55 p.m. UTC | #4
Hi,

On 07/14, Daniel Vetter wrote:
> On Tue, Jul 14, 2020 at 11:57 AM Melissa Wen <melissa.srw@gmail.com> wrote:
> >
> > On 07/12, Rodrigo Siqueira wrote:
> > > Hi,
> > >
> > > Everything looks fine to me, I just noticed that the amdgpu patches did
> > > not apply smoothly, however it was trivial to fix the issues.
> > >
> > > Reviewed-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> > >
> > > Melissa,
> > > Since you are using vkms regularly, could you test this patch and review
> > > it? Remember to add your Tested-by when you finish.
> > >
> > Hi,
> >
> > I've applied the patch series, ran some tests on vkms, and found no
> > issues. I mean, things have remained stable.
> >
> > Tested-by: Melissa Wen <melissa.srw@gmail.com>
> 
> Did you test with CONFIG_PROVE_LOCKING enabled in the kernel .config?
> Without that enabled, there's not really any change here, but with
> that enabled there might be some lockdep splats in dmesg indicating a
> problem.
>

Even with the lock debugging config enabled, no new issue arose in dmesg
during my tests using vkms.

Melissa

> Thanks, Daniel
> >
> > > Thanks
> > >
> > > On 07/07, Daniel Vetter wrote:
> > > > This is needed to signal the fences from page flips, annotate it
> > > > accordingly. We need to annotate entire timer callback since if we get
> > > > stuck anywhere in there, then the timer stops, and hence fences stop.
> > > > Just annotating the top part that does the vblank handling isn't
> > > > enough.
> > > >
> > > > Cc: linux-media@vger.kernel.org
> > > > Cc: linaro-mm-sig@lists.linaro.org
> > > > Cc: linux-rdma@vger.kernel.org
> > > > Cc: amd-gfx@lists.freedesktop.org
> > > > Cc: intel-gfx@lists.freedesktop.org
> > > > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > > Cc: Christian König <christian.koenig@amd.com>
> > > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > > > Cc: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> > > > Cc: Haneen Mohammed <hamohammed.sa@gmail.com>
> > > > Cc: Daniel Vetter <daniel@ffwll.ch>
> > > > ---
> > > >  drivers/gpu/drm/vkms/vkms_crtc.c | 8 +++++++-
> > > >  1 file changed, 7 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
> > > > index ac85e17428f8..a53a40848a72 100644
> > > > --- a/drivers/gpu/drm/vkms/vkms_crtc.c
> > > > +++ b/drivers/gpu/drm/vkms/vkms_crtc.c
> > > > @@ -1,5 +1,7 @@
> > > >  // SPDX-License-Identifier: GPL-2.0+
> > > >
> > > > +#include <linux/dma-fence.h>
> > > > +
> > > >  #include <drm/drm_atomic.h>
> > > >  #include <drm/drm_atomic_helper.h>
> > > >  #include <drm/drm_probe_helper.h>
> > > > @@ -14,7 +16,9 @@ static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
> > > >     struct drm_crtc *crtc = &output->crtc;
> > > >     struct vkms_crtc_state *state;
> > > >     u64 ret_overrun;
> > > > -   bool ret;
> > > > +   bool ret, fence_cookie;
> > > > +
> > > > +   fence_cookie = dma_fence_begin_signalling();
> > > >
> > > >     ret_overrun = hrtimer_forward_now(&output->vblank_hrtimer,
> > > >                                       output->period_ns);
> > > > @@ -49,6 +53,8 @@ static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
> > > >                     DRM_DEBUG_DRIVER("Composer worker already queued\n");
> > > >     }
> > > >
> > > > +   dma_fence_end_signalling(fence_cookie);
> > > > +
> > > >     return HRTIMER_RESTART;
> > > >  }
> > > >
> > > > --
> > > > 2.27.0
> > > >
> > >
> > > --
> > > Rodrigo Siqueira
> > > https://siqueira.tech
> >
> >
> 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
Daniel Vetter July 14, 2020, 3:23 p.m. UTC | #5
On Tue, Jul 14, 2020 at 4:56 PM Melissa Wen <melissa.srw@gmail.com> wrote:
>
> Hi,
>
> On 07/14, Daniel Vetter wrote:
> > On Tue, Jul 14, 2020 at 11:57 AM Melissa Wen <melissa.srw@gmail.com> wrote:
> > >
> > > On 07/12, Rodrigo Siqueira wrote:
> > > > Hi,
> > > >
> > > > Everything looks fine to me, I just noticed that the amdgpu patches did
> > > > not apply smoothly, however it was trivial to fix the issues.
> > > >
> > > > Reviewed-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> > > >
> > > > Melissa,
> > > > Since you are using vkms regularly, could you test this patch and review
> > > > it? Remember to add your Tested-by when you finish.
> > > >
> > > Hi,
> > >
> > > I've applied the patch series, ran some tests on vkms, and found no
> > > issues. I mean, things have remained stable.
> > >
> > > Tested-by: Melissa Wen <melissa.srw@gmail.com>
> >
> > Did you test with CONFIG_PROVE_LOCKING enabled in the kernel .config?
> > Without that enabled, there's not really any change here, but with
> > that enabled there might be some lockdep splats in dmesg indicating a
> > problem.
> >
>
> Even with the lock debugging config enabled, no new issue arose in dmesg
> during my tests using vkms.

Excellent, thanks a lot for confirming this.
-Daniel

>
> Melissa
>
> > Thanks, Daniel
> > >
> > > > Thanks
> > > >
> > > > On 07/07, Daniel Vetter wrote:
> > > > > This is needed to signal the fences from page flips, annotate it
> > > > > accordingly. We need to annotate entire timer callback since if we get
> > > > > stuck anywhere in there, then the timer stops, and hence fences stop.
> > > > > Just annotating the top part that does the vblank handling isn't
> > > > > enough.
> > > > >
> > > > > Cc: linux-media@vger.kernel.org
> > > > > Cc: linaro-mm-sig@lists.linaro.org
> > > > > Cc: linux-rdma@vger.kernel.org
> > > > > Cc: amd-gfx@lists.freedesktop.org
> > > > > Cc: intel-gfx@lists.freedesktop.org
> > > > > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > > > Cc: Christian König <christian.koenig@amd.com>
> > > > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > > > > Cc: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> > > > > Cc: Haneen Mohammed <hamohammed.sa@gmail.com>
> > > > > Cc: Daniel Vetter <daniel@ffwll.ch>
> > > > > ---
> > > > >  drivers/gpu/drm/vkms/vkms_crtc.c | 8 +++++++-
> > > > >  1 file changed, 7 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
> > > > > index ac85e17428f8..a53a40848a72 100644
> > > > > --- a/drivers/gpu/drm/vkms/vkms_crtc.c
> > > > > +++ b/drivers/gpu/drm/vkms/vkms_crtc.c
> > > > > @@ -1,5 +1,7 @@
> > > > >  // SPDX-License-Identifier: GPL-2.0+
> > > > >
> > > > > +#include <linux/dma-fence.h>
> > > > > +
> > > > >  #include <drm/drm_atomic.h>
> > > > >  #include <drm/drm_atomic_helper.h>
> > > > >  #include <drm/drm_probe_helper.h>
> > > > > @@ -14,7 +16,9 @@ static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
> > > > >     struct drm_crtc *crtc = &output->crtc;
> > > > >     struct vkms_crtc_state *state;
> > > > >     u64 ret_overrun;
> > > > > -   bool ret;
> > > > > +   bool ret, fence_cookie;
> > > > > +
> > > > > +   fence_cookie = dma_fence_begin_signalling();
> > > > >
> > > > >     ret_overrun = hrtimer_forward_now(&output->vblank_hrtimer,
> > > > >                                       output->period_ns);
> > > > > @@ -49,6 +53,8 @@ static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
> > > > >                     DRM_DEBUG_DRIVER("Composer worker already queued\n");
> > > > >     }
> > > > >
> > > > > +   dma_fence_end_signalling(fence_cookie);
> > > > > +
> > > > >     return HRTIMER_RESTART;
> > > > >  }
> > > > >
> > > > > --
> > > > > 2.27.0
> > > > >
> > > >
> > > > --
> > > > Rodrigo Siqueira
> > > > https://siqueira.tech
> > >
> > >
> >
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch

Patch
diff mbox series

diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
index ac85e17428f8..a53a40848a72 100644
--- a/drivers/gpu/drm/vkms/vkms_crtc.c
+++ b/drivers/gpu/drm/vkms/vkms_crtc.c
@@ -1,5 +1,7 @@ 
 // SPDX-License-Identifier: GPL-2.0+
 
+#include <linux/dma-fence.h>
+
 #include <drm/drm_atomic.h>
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_probe_helper.h>
@@ -14,7 +16,9 @@  static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
 	struct drm_crtc *crtc = &output->crtc;
 	struct vkms_crtc_state *state;
 	u64 ret_overrun;
-	bool ret;
+	bool ret, fence_cookie;
+
+	fence_cookie = dma_fence_begin_signalling();
 
 	ret_overrun = hrtimer_forward_now(&output->vblank_hrtimer,
 					  output->period_ns);
@@ -49,6 +53,8 @@  static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
 			DRM_DEBUG_DRIVER("Composer worker already queued\n");
 	}
 
+	dma_fence_end_signalling(fence_cookie);
+
 	return HRTIMER_RESTART;
 }