Re: [PATCH] drm/atomic-helper: Bump vblank timeout to 100 ms

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Apr 30, 2019 at 01:44:19PM +0300, Ville Syrjälä wrote:
> On Tue, Apr 30, 2019 at 11:37:46AM +0200, Linus Walleij wrote:
> > The 50 ms default timeout waiting for vblanks is too small
> > for the first vblank from the ST-Ericsson MCDE display
> > controller over DSI. Presumably this is because the DSI
> > display is command-mode only and the state machine will
> > take some time setting up its state for the first display
> > update, and we hit a timeout. 100 ms makes it pass without
> > problems.
> > 
> > Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> > ---
> > After a quite prolonged hunting for the cause of missed
> > vblanks in the MCDE driver I finally realized it timed
> > out because it was simply taking some time on the first
> > vblank. 50 ms makes sense on 60Hz monitors for sure,
> > but an embedded DSI state machine can be slow, as it
> > turns out.
> > 
> > Tell me if this should be a per-driver variable and I
> > will think of something.
> > ---
> >  drivers/gpu/drm/drm_atomic_helper.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > index 40ac19848034..f0aa7b195d79 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -1424,7 +1424,7 @@ drm_atomic_helper_wait_for_vblanks(struct drm_device *dev,
> >  		ret = wait_event_timeout(dev->vblank[i].queue,
> >  				old_state->crtcs[i].last_vblank_count !=
> >  					drm_crtc_vblank_count(crtc),
> > -				msecs_to_jiffies(50));
> > +				msecs_to_jiffies(100));
> 
> 50ms is pretty tight for 24Hz as well. I suppose we could make this
> depend on the expected frame/field duration, but it's generally
> indicative of a programming error of some sort, so as long as it's
> long enough I think we're good.
> 
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Yeah makes sense to bump this a bit. An uber-fancy version would look at
the programmed refresh rate and maybe give the driver extra slack for the
first frame after a modeset. But this is good enough.

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

> 
> >  
> >  		WARN(!ret, "[CRTC:%d:%s] vblank wait timed out\n",
> >  		     crtc->base.id, crtc->name);
> > -- 
> > 2.20.1
> 
> -- 
> Ville Syrjälä
> Intel

-- 
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