[PATCH] drm/vblank: Fix flip event vblank count

[PATCH] drm/vblank: Fix flip event vblank count

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

On machines where the vblank interrupt fires some time after the start
of vblank (or we just manage to race with the vblank interrupt handler)
we will currently stuff a stale vblank counter value into the flip event,
and thus we'll prematurely complete the flip.

Switch over to drm_crtc_accurate_vblank_count() to make sure we have an
up to date counter value, crucially also remember to add the +1 so that
the delayed vblank interrupt won't complete the flip prematurely.

Cc: stable@vger.kernel.org
Cc: Daniel Vetter <daniel@ffwll.ch>
Suggested-by: Daniel Vetter <daniel@ffwll.ch>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_vblank.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
index 70f2b9593edc..f14e6c92e332 100644
--- a/drivers/gpu/drm/drm_vblank.c
+++ b/drivers/gpu/drm/drm_vblank.c
@@ -869,7 +869,7 @@ void drm_crtc_arm_vblank_event(struct drm_crtc *crtc,
 	assert_spin_locked(&dev->event_lock);
 
 	e->pipe = pipe;
-	e->event.sequence = drm_vblank_count(dev, pipe);
+	e->event.sequence = drm_crtc_accurate_vblank_count(crtc) + 1;
 	e->event.crtc_id = crtc->base.id;
 	list_add_tail(&e->base.link, &dev->vblank_event_list);
 }
-- 
2.13.6

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/vblank: Fix flip event vblank count

[Ville Syrjälä]

On Tue, Oct 10, 2017 at 04:33:22PM +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> On machines where the vblank interrupt fires some time after the start
> of vblank (or we just manage to race with the vblank interrupt handler)
> we will currently stuff a stale vblank counter value into the flip event,
> and thus we'll prematurely complete the flip.
> 
> Switch over to drm_crtc_accurate_vblank_count() to make sure we have an
> up to date counter value, crucially also remember to add the +1 so that
> the delayed vblank interrupt won't complete the flip prematurely.
> 
> Cc: stable@vger.kernel.org
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Suggested-by: Daniel Vetter <daniel@ffwll.ch>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Pushed to drm-misc-next with Daniel's irc r-b:
16:48 < danvet> vsyrjala, r-b, I think
...
17:31 < vsyrjala> danvet: your irc r-b for vblank_accurate+1 still
holds?
17:32 < danvet> vsyrjala, yeah

> ---
>  drivers/gpu/drm/drm_vblank.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> index 70f2b9593edc..f14e6c92e332 100644
> --- a/drivers/gpu/drm/drm_vblank.c
> +++ b/drivers/gpu/drm/drm_vblank.c
> @@ -869,7 +869,7 @@ void drm_crtc_arm_vblank_event(struct drm_crtc *crtc,
>  	assert_spin_locked(&dev->event_lock);
>  
>  	e->pipe = pipe;
> -	e->event.sequence = drm_vblank_count(dev, pipe);
> +	e->event.sequence = drm_crtc_accurate_vblank_count(crtc) + 1;
>  	e->event.crtc_id = crtc->base.id;
>  	list_add_tail(&e->base.link, &dev->vblank_event_list);
>  }
> -- 
> 2.13.6

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH] drm/vblank: Fix flip event vblank count

[Andrzej Hajda]

Hi Ville,

On 10.10.2017 15:33, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> On machines where the vblank interrupt fires some time after the start
> of vblank (or we just manage to race with the vblank interrupt handler)
> we will currently stuff a stale vblank counter value into the flip event,
> and thus we'll prematurely complete the flip.
>
> Switch over to drm_crtc_accurate_vblank_count() to make sure we have an
> up to date counter value, crucially also remember to add the +1 so that
> the delayed vblank interrupt won't complete the flip prematurely.
>
> Cc: stable@vger.kernel.org
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Suggested-by: Daniel Vetter <daniel@ffwll.ch>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_vblank.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> index 70f2b9593edc..f14e6c92e332 100644
> --- a/drivers/gpu/drm/drm_vblank.c
> +++ b/drivers/gpu/drm/drm_vblank.c
> @@ -869,7 +869,7 @@ void drm_crtc_arm_vblank_event(struct drm_crtc *crtc,
>  	assert_spin_locked(&dev->event_lock);
>  
>  	e->pipe = pipe;
> -	e->event.sequence = drm_vblank_count(dev, pipe);
> +	e->event.sequence = drm_crtc_accurate_vblank_count(crtc) + 1;

With this patch drm_crtc_arm_vblank_event calls
drm_crtc_accurate_vblank_count, which requires get_vblank_timestamp
callback, otherwise it issue WARN every time it is called.
On the other side most of the users of drm_crtc_arm_vblank_event do not
implement this callback. As a result one can observe multiple WARNs.
It was observed on Exynos platform but grep shows it can affect many
other platforms:

$ git grep -l drm_crtc_arm_vblank_event drivers/gpu/drm/
drivers/gpu/drm/arm/hdlcd_crtc.c
drivers/gpu/drm/arm/malidp_drv.c
drivers/gpu/drm/drm_vblank.c
drivers/gpu/drm/exynos/exynos_drm_crtc.c
drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c
drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c
drivers/gpu/drm/i915/intel_sprite.c
drivers/gpu/drm/imx/ipuv3-crtc.c
drivers/gpu/drm/mxsfb/mxsfb_crtc.c
drivers/gpu/drm/nouveau/nouveau_display.c
drivers/gpu/drm/pl111/pl111_display.c
drivers/gpu/drm/sti/sti_crtc.c
drivers/gpu/drm/stm/ltdc.c
drivers/gpu/drm/sun4i/sun4i_crtc.c
drivers/gpu/drm/tve200/tve200_display.c
drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
drivers/gpu/drm/zte/zx_vou.c

$ git grep -l get_vblank_timestamp drivers/gpu/drm/
drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
drivers/gpu/drm/drm_vblank.c
drivers/gpu/drm/i915/i915_irq.c
drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c
drivers/gpu/drm/nouveau/nouveau_drm.c
drivers/gpu/drm/radeon/radeon_drv.c
drivers/gpu/drm/vc4/vc4_drv.c

Regards
Andrzej

>  	e->event.crtc_id = crtc->base.id;
>  	list_add_tail(&e->base.link, &dev->vblank_event_list);
>  }


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/vblank: Fix flip event vblank count

[Ville Syrjälä]

On Wed, Oct 18, 2017 at 09:07:47AM +0200, Andrzej Hajda wrote:
> Hi Ville,
> 
> On 10.10.2017 15:33, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > On machines where the vblank interrupt fires some time after the start
> > of vblank (or we just manage to race with the vblank interrupt handler)
> > we will currently stuff a stale vblank counter value into the flip event,
> > and thus we'll prematurely complete the flip.
> >
> > Switch over to drm_crtc_accurate_vblank_count() to make sure we have an
> > up to date counter value, crucially also remember to add the +1 so that
> > the delayed vblank interrupt won't complete the flip prematurely.
> >
> > Cc: stable@vger.kernel.org
> > Cc: Daniel Vetter <daniel@ffwll.ch>
> > Suggested-by: Daniel Vetter <daniel@ffwll.ch>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/drm_vblank.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> > index 70f2b9593edc..f14e6c92e332 100644
> > --- a/drivers/gpu/drm/drm_vblank.c
> > +++ b/drivers/gpu/drm/drm_vblank.c
> > @@ -869,7 +869,7 @@ void drm_crtc_arm_vblank_event(struct drm_crtc *crtc,
> >  	assert_spin_locked(&dev->event_lock);
> >  
> >  	e->pipe = pipe;
> > -	e->event.sequence = drm_vblank_count(dev, pipe);
> > +	e->event.sequence = drm_crtc_accurate_vblank_count(crtc) + 1;
> 
> With this patch drm_crtc_arm_vblank_event calls
> drm_crtc_accurate_vblank_count, which requires get_vblank_timestamp
> callback, otherwise it issue WARN every time it is called.

Argh! Maybe just remove the WARN then?

> On the other side most of the users of drm_crtc_arm_vblank_event do not
> implement this callback. As a result one can observe multiple WARNs.
> It was observed on Exynos platform but grep shows it can affect many
> other platforms:
> 
> $ git grep -l drm_crtc_arm_vblank_event drivers/gpu/drm/
> drivers/gpu/drm/arm/hdlcd_crtc.c
> drivers/gpu/drm/arm/malidp_drv.c
> drivers/gpu/drm/drm_vblank.c
> drivers/gpu/drm/exynos/exynos_drm_crtc.c
> drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c
> drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c
> drivers/gpu/drm/i915/intel_sprite.c
> drivers/gpu/drm/imx/ipuv3-crtc.c
> drivers/gpu/drm/mxsfb/mxsfb_crtc.c
> drivers/gpu/drm/nouveau/nouveau_display.c
> drivers/gpu/drm/pl111/pl111_display.c
> drivers/gpu/drm/sti/sti_crtc.c
> drivers/gpu/drm/stm/ltdc.c
> drivers/gpu/drm/sun4i/sun4i_crtc.c
> drivers/gpu/drm/tve200/tve200_display.c
> drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> drivers/gpu/drm/zte/zx_vou.c
> 
> $ git grep -l get_vblank_timestamp drivers/gpu/drm/
> drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> drivers/gpu/drm/drm_vblank.c
> drivers/gpu/drm/i915/i915_irq.c
> drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c
> drivers/gpu/drm/nouveau/nouveau_drm.c
> drivers/gpu/drm/radeon/radeon_drv.c
> drivers/gpu/drm/vc4/vc4_drv.c
> 
> Regards
> Andrzej
> 
> >  	e->event.crtc_id = crtc->base.id;
> >  	list_add_tail(&e->base.link, &dev->vblank_event_list);
> >  }
> 

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH] drm/vblank: Fix flip event vblank count

[Daniel Vetter]

On Wed, Oct 18, 2017 at 04:50:19PM +0300, Ville Syrjälä wrote:
> On Wed, Oct 18, 2017 at 09:07:47AM +0200, Andrzej Hajda wrote:
> > Hi Ville,
> > 
> > On 10.10.2017 15:33, Ville Syrjala wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > >
> > > On machines where the vblank interrupt fires some time after the start
> > > of vblank (or we just manage to race with the vblank interrupt handler)
> > > we will currently stuff a stale vblank counter value into the flip event,
> > > and thus we'll prematurely complete the flip.
> > >
> > > Switch over to drm_crtc_accurate_vblank_count() to make sure we have an
> > > up to date counter value, crucially also remember to add the +1 so that
> > > the delayed vblank interrupt won't complete the flip prematurely.
> > >
> > > Cc: stable@vger.kernel.org
> > > Cc: Daniel Vetter <daniel@ffwll.ch>
> > > Suggested-by: Daniel Vetter <daniel@ffwll.ch>
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/drm_vblank.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> > > index 70f2b9593edc..f14e6c92e332 100644
> > > --- a/drivers/gpu/drm/drm_vblank.c
> > > +++ b/drivers/gpu/drm/drm_vblank.c
> > > @@ -869,7 +869,7 @@ void drm_crtc_arm_vblank_event(struct drm_crtc *crtc,
> > >  	assert_spin_locked(&dev->event_lock);
> > >  
> > >  	e->pipe = pipe;
> > > -	e->event.sequence = drm_vblank_count(dev, pipe);
> > > +	e->event.sequence = drm_crtc_accurate_vblank_count(crtc) + 1;
> > 
> > With this patch drm_crtc_arm_vblank_event calls
> > drm_crtc_accurate_vblank_count, which requires get_vblank_timestamp
> > callback, otherwise it issue WARN every time it is called.
> 
> Argh! Maybe just remove the WARN then?

Yeah tune it down to DRM_DEBUG_KMS or something similar I'd say.
-Daniel

> 
> > On the other side most of the users of drm_crtc_arm_vblank_event do not
> > implement this callback. As a result one can observe multiple WARNs.
> > It was observed on Exynos platform but grep shows it can affect many
> > other platforms:
> > 
> > $ git grep -l drm_crtc_arm_vblank_event drivers/gpu/drm/
> > drivers/gpu/drm/arm/hdlcd_crtc.c
> > drivers/gpu/drm/arm/malidp_drv.c
> > drivers/gpu/drm/drm_vblank.c
> > drivers/gpu/drm/exynos/exynos_drm_crtc.c
> > drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c
> > drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c
> > drivers/gpu/drm/i915/intel_sprite.c
> > drivers/gpu/drm/imx/ipuv3-crtc.c
> > drivers/gpu/drm/mxsfb/mxsfb_crtc.c
> > drivers/gpu/drm/nouveau/nouveau_display.c
> > drivers/gpu/drm/pl111/pl111_display.c
> > drivers/gpu/drm/sti/sti_crtc.c
> > drivers/gpu/drm/stm/ltdc.c
> > drivers/gpu/drm/sun4i/sun4i_crtc.c
> > drivers/gpu/drm/tve200/tve200_display.c
> > drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> > drivers/gpu/drm/zte/zx_vou.c
> > 
> > $ git grep -l get_vblank_timestamp drivers/gpu/drm/
> > drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> > drivers/gpu/drm/drm_vblank.c
> > drivers/gpu/drm/i915/i915_irq.c
> > drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c
> > drivers/gpu/drm/nouveau/nouveau_drm.c
> > drivers/gpu/drm/radeon/radeon_drv.c
> > drivers/gpu/drm/vc4/vc4_drv.c
> > 
> > Regards
> > Andrzej
> > 
> > >  	e->event.crtc_id = crtc->base.id;
> > >  	list_add_tail(&e->base.link, &dev->vblank_event_list);
> > >  }
> > 
> 
> -- 
> Ville Syrjälä
> Intel OTC

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[PATCH] drm/vblank: Tune drm_crtc_accurate_vblank_count() WARN down to a debug

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Since commit 632c6e4edef1 ("drm/vblank: Fix flip event vblank count")
even drivers that don't implement accurate vblank timestamps will end
up using drm_crtc_accurate_vblank_count(). That leads to a WARN every
time drm_crtc_arm_vblank_event() gets called. The could be as often
as every frame for each active crtc.

Considering drm_crtc_accurate_vblank_count() is never any worse than
the drm_vblank_count() we used previously, let's just skip the WARN
unless DRM_UT_VBL is enabled. That way people won't be bothered by
this unless they're debugging vblank code. And let's also change it
to WARN_ONCE() so that even when you're debugging vblank code you
won't get drowned by constant WARNs.

Cc: stable@vger.kernel.org
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: "Szyprowski, Marek" <m.szyprowski@samsung.com>
Cc: Andrzej Hajda <a.hajda@samsung.com>
Reported-by: Andrzej Hajda <a.hajda@samsung.com>
Fixes: 632c6e4edef1 ("drm/vblank: Fix flip event vblank count")
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_vblank.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
index 13722c373a6a..c81c297995c6 100644
--- a/drivers/gpu/drm/drm_vblank.c
+++ b/drivers/gpu/drm/drm_vblank.c
@@ -299,8 +299,8 @@ u32 drm_crtc_accurate_vblank_count(struct drm_crtc *crtc)
 	u32 vblank;
 	unsigned long flags;
 
-	WARN(!dev->driver->get_vblank_timestamp,
-	     "This function requires support for accurate vblank timestamps.");
+	WARN_ONCE(drm_debug & DRM_UT_VBL && !dev->driver->get_vblank_timestamp,
+		  "This function requires support for accurate vblank timestamps.");
 
 	spin_lock_irqsave(&dev->vblank_time_lock, flags);
 
-- 
2.13.6

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/vblank: Tune drm_crtc_accurate_vblank_count() WARN down to a debug

[Benjamin Gaignard]

2017-10-23 17:25 GMT+02:00 Ville Syrjala <ville.syrjala@linux.intel.com>:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Since commit 632c6e4edef1 ("drm/vblank: Fix flip event vblank count")
> even drivers that don't implement accurate vblank timestamps will end
> up using drm_crtc_accurate_vblank_count(). That leads to a WARN every
> time drm_crtc_arm_vblank_event() gets called. The could be as often
> as every frame for each active crtc.
>
> Considering drm_crtc_accurate_vblank_count() is never any worse than
> the drm_vblank_count() we used previously, let's just skip the WARN
> unless DRM_UT_VBL is enabled. That way people won't be bothered by
> this unless they're debugging vblank code. And let's also change it
> to WARN_ONCE() so that even when you're debugging vblank code you
> won't get drowned by constant WARNs.
>
> Cc: stable@vger.kernel.org
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: "Szyprowski, Marek" <m.szyprowski@samsung.com>
> Cc: Andrzej Hajda <a.hajda@samsung.com>
> Reported-by: Andrzej Hajda <a.hajda@samsung.com>
> Fixes: 632c6e4edef1 ("drm/vblank: Fix flip event vblank count")
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

I have tested it on sti and stm driver, it fix the problem, thanks.

Acked-by: Benjamin Gaignard <benjamin.gaignard@linaro.org>

> ---
>  drivers/gpu/drm/drm_vblank.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> index 13722c373a6a..c81c297995c6 100644
> --- a/drivers/gpu/drm/drm_vblank.c
> +++ b/drivers/gpu/drm/drm_vblank.c
> @@ -299,8 +299,8 @@ u32 drm_crtc_accurate_vblank_count(struct drm_crtc *crtc)
>         u32 vblank;
>         unsigned long flags;
>
> -       WARN(!dev->driver->get_vblank_timestamp,
> -            "This function requires support for accurate vblank timestamps.");
> +       WARN_ONCE(drm_debug & DRM_UT_VBL && !dev->driver->get_vblank_timestamp,
> +                 "This function requires support for accurate vblank timestamps.");
>
>         spin_lock_irqsave(&dev->vblank_time_lock, flags);
>
> --
> 2.13.6
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/vblank: Tune drm_crtc_accurate_vblank_count() WARN down to a debug

[Daniel Vetter]

On Tue, Oct 24, 2017 at 11:01:32AM +0200, Benjamin Gaignard wrote:
> 2017-10-23 17:25 GMT+02:00 Ville Syrjala <ville.syrjala@linux.intel.com>:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Since commit 632c6e4edef1 ("drm/vblank: Fix flip event vblank count")
> > even drivers that don't implement accurate vblank timestamps will end
> > up using drm_crtc_accurate_vblank_count(). That leads to a WARN every
> > time drm_crtc_arm_vblank_event() gets called. The could be as often
> > as every frame for each active crtc.
> >
> > Considering drm_crtc_accurate_vblank_count() is never any worse than
> > the drm_vblank_count() we used previously, let's just skip the WARN
> > unless DRM_UT_VBL is enabled. That way people won't be bothered by
> > this unless they're debugging vblank code. And let's also change it
> > to WARN_ONCE() so that even when you're debugging vblank code you
> > won't get drowned by constant WARNs.
> >
> > Cc: stable@vger.kernel.org
> > Cc: Daniel Vetter <daniel@ffwll.ch>
> > Cc: "Szyprowski, Marek" <m.szyprowski@samsung.com>
> > Cc: Andrzej Hajda <a.hajda@samsung.com>
> > Reported-by: Andrzej Hajda <a.hajda@samsung.com>
> > Fixes: 632c6e4edef1 ("drm/vblank: Fix flip event vblank count")
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> I have tested it on sti and stm driver, it fix the problem, thanks.
> 
> Acked-by: Benjamin Gaignard <benjamin.gaignard@linaro.org>
> 
> > ---
> >  drivers/gpu/drm/drm_vblank.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> > index 13722c373a6a..c81c297995c6 100644
> > --- a/drivers/gpu/drm/drm_vblank.c
> > +++ b/drivers/gpu/drm/drm_vblank.c
> > @@ -299,8 +299,8 @@ u32 drm_crtc_accurate_vblank_count(struct drm_crtc *crtc)
> >         u32 vblank;
> >         unsigned long flags;
> >
> > -       WARN(!dev->driver->get_vblank_timestamp,
> > -            "This function requires support for accurate vblank timestamps.");
> > +       WARN_ONCE(drm_debug & DRM_UT_VBL && !dev->driver->get_vblank_timestamp,
> > +                 "This function requires support for accurate vblank timestamps.");

Somewhat a bikeshed, but if we e.g. enable debugging in a CI or piglit
run, then this could change the results. I'd go with a if ()
DRM_DEBUG_VBLANK. Either way:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> >
> >         spin_lock_irqsave(&dev->vblank_time_lock, flags);
> >
> > --
> > 2.13.6
> >
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/vblank: Tune drm_crtc_accurate_vblank_count() WARN down to a debug

[Benjamin Gaignard]

2017-10-30 10:19 GMT+01:00 Daniel Vetter <daniel@ffwll.ch>:
> On Tue, Oct 24, 2017 at 11:01:32AM +0200, Benjamin Gaignard wrote:
>> 2017-10-23 17:25 GMT+02:00 Ville Syrjala <ville.syrjala@linux.intel.com>:
>> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> >
>> > Since commit 632c6e4edef1 ("drm/vblank: Fix flip event vblank count")
>> > even drivers that don't implement accurate vblank timestamps will end
>> > up using drm_crtc_accurate_vblank_count(). That leads to a WARN every
>> > time drm_crtc_arm_vblank_event() gets called. The could be as often
>> > as every frame for each active crtc.
>> >
>> > Considering drm_crtc_accurate_vblank_count() is never any worse than
>> > the drm_vblank_count() we used previously, let's just skip the WARN
>> > unless DRM_UT_VBL is enabled. That way people won't be bothered by
>> > this unless they're debugging vblank code. And let's also change it
>> > to WARN_ONCE() so that even when you're debugging vblank code you
>> > won't get drowned by constant WARNs.
>> >
>> > Cc: stable@vger.kernel.org
>> > Cc: Daniel Vetter <daniel@ffwll.ch>
>> > Cc: "Szyprowski, Marek" <m.szyprowski@samsung.com>
>> > Cc: Andrzej Hajda <a.hajda@samsung.com>
>> > Reported-by: Andrzej Hajda <a.hajda@samsung.com>
>> > Fixes: 632c6e4edef1 ("drm/vblank: Fix flip event vblank count")
>> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>
>> I have tested it on sti and stm driver, it fix the problem, thanks.
>>
>> Acked-by: Benjamin Gaignard <benjamin.gaignard@linaro.org>
>>
>> > ---
>> >  drivers/gpu/drm/drm_vblank.c | 4 ++--
>> >  1 file changed, 2 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
>> > index 13722c373a6a..c81c297995c6 100644
>> > --- a/drivers/gpu/drm/drm_vblank.c
>> > +++ b/drivers/gpu/drm/drm_vblank.c
>> > @@ -299,8 +299,8 @@ u32 drm_crtc_accurate_vblank_count(struct drm_crtc *crtc)
>> >         u32 vblank;
>> >         unsigned long flags;
>> >
>> > -       WARN(!dev->driver->get_vblank_timestamp,
>> > -            "This function requires support for accurate vblank timestamps.");
>> > +       WARN_ONCE(drm_debug & DRM_UT_VBL && !dev->driver->get_vblank_timestamp,
>> > +                 "This function requires support for accurate vblank timestamps.");
>
> Somewhat a bikeshed, but if we e.g. enable debugging in a CI or piglit
> run, then this could change the results. I'd go with a if ()
> DRM_DEBUG_VBLANK. Either way:
>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>

Does something is missing to merge this fix in drm-misc ?
Without it I got warning at each frame.

Benjamin

>> >
>> >         spin_lock_irqsave(&dev->vblank_time_lock, flags);
>> >
>> > --
>> > 2.13.6
>> >
>> > _______________________________________________
>> > dri-devel mailing list
>> > dri-devel@lists.freedesktop.org
>> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

Re: [PATCH] drm/vblank: Tune drm_crtc_accurate_vblank_count() WARN down to a debug

[Ville Syrjälä]

On Thu, Nov 16, 2017 at 03:19:35PM +0100, Benjamin Gaignard wrote:
> 2017-10-30 10:19 GMT+01:00 Daniel Vetter <daniel@ffwll.ch>:
> > On Tue, Oct 24, 2017 at 11:01:32AM +0200, Benjamin Gaignard wrote:
> >> 2017-10-23 17:25 GMT+02:00 Ville Syrjala <ville.syrjala@linux.intel.com>:
> >> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> >
> >> > Since commit 632c6e4edef1 ("drm/vblank: Fix flip event vblank count")
> >> > even drivers that don't implement accurate vblank timestamps will end
> >> > up using drm_crtc_accurate_vblank_count(). That leads to a WARN every
> >> > time drm_crtc_arm_vblank_event() gets called. The could be as often
> >> > as every frame for each active crtc.
> >> >
> >> > Considering drm_crtc_accurate_vblank_count() is never any worse than
> >> > the drm_vblank_count() we used previously, let's just skip the WARN
> >> > unless DRM_UT_VBL is enabled. That way people won't be bothered by
> >> > this unless they're debugging vblank code. And let's also change it
> >> > to WARN_ONCE() so that even when you're debugging vblank code you
> >> > won't get drowned by constant WARNs.
> >> >
> >> > Cc: stable@vger.kernel.org
> >> > Cc: Daniel Vetter <daniel@ffwll.ch>
> >> > Cc: "Szyprowski, Marek" <m.szyprowski@samsung.com>
> >> > Cc: Andrzej Hajda <a.hajda@samsung.com>
> >> > Reported-by: Andrzej Hajda <a.hajda@samsung.com>
> >> > Fixes: 632c6e4edef1 ("drm/vblank: Fix flip event vblank count")
> >> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>
> >> I have tested it on sti and stm driver, it fix the problem, thanks.
> >>
> >> Acked-by: Benjamin Gaignard <benjamin.gaignard@linaro.org>
> >>
> >> > ---
> >> >  drivers/gpu/drm/drm_vblank.c | 4 ++--
> >> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >> >
> >> > diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> >> > index 13722c373a6a..c81c297995c6 100644
> >> > --- a/drivers/gpu/drm/drm_vblank.c
> >> > +++ b/drivers/gpu/drm/drm_vblank.c
> >> > @@ -299,8 +299,8 @@ u32 drm_crtc_accurate_vblank_count(struct drm_crtc *crtc)
> >> >         u32 vblank;
> >> >         unsigned long flags;
> >> >
> >> > -       WARN(!dev->driver->get_vblank_timestamp,
> >> > -            "This function requires support for accurate vblank timestamps.");
> >> > +       WARN_ONCE(drm_debug & DRM_UT_VBL && !dev->driver->get_vblank_timestamp,
> >> > +                 "This function requires support for accurate vblank timestamps.");
> >
> > Somewhat a bikeshed, but if we e.g. enable debugging in a CI or piglit
> > run, then this could change the results. I'd go with a if ()
> > DRM_DEBUG_VBLANK. Either way:
> >
> > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> >
> 
> Does something is missing to merge this fix in drm-misc ?
> Without it I got warning at each frame.

Pushed... 9 days ago it seems.

-- 
Ville Syrjälä
Intel OTC