| Message ID | 20200707201229.472834-5-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | dma-fence annotations, round 3 | expand |
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 >
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
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 > >
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
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
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; }
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(-)