[PATCH 3/3] present: Check for an invalid target CRTC before flipping

[Chris Wilson]

On Sun, Feb 08, 2015 at 04:28:31PM -0800, Kenneth Graunke wrote:
> On Sunday, February 08, 2015 12:00:37 PM Chris Wilson wrote:
> > On Sun, Feb 08, 2015 at 02:10:07AM -0800, Kenneth Graunke wrote:
> > > On Friday, February 06, 2015 08:25:44 AM Chris Wilson wrote:
> > > > Once we have chosen the target CRTC, check for an error when querying
> > > > the current MSC so we can report the error before attempting to record
> > > > the uninitialised MSC/UST, and subsequently feeding garbage values to
> > > > drmWaitVBlank.
> > > > 
> > > > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > > > ---
> > > >  present/present.c | 4 +++-
> > > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/present/present.c b/present/present.c
> > > > index 1ce587e..4a1cf0c 100644
> > > > --- a/present/present.c
> > > > +++ b/present/present.c
> > > > @@ -727,7 +727,9 @@ present_pixmap(WindowPtr window,
> > > >      else if (!target_crtc)
> > > >  	target_crtc = present_get_crtc(window);
> > > >  
> > > > -    present_get_ust_msc(screen, target_crtc, &ust, &crtc_msc);
> > > > +    ret = present_get_ust_msc(screen, target_crtc, &ust, &crtc_msc);
> > > > +    if (ret)
> > > > +	    return ret;
> > > >  
> > > >      target_msc = present_window_to_crtc_msc(window, target_crtc, window_msc, crtc_msc);
> > > 
> > > You've definitely found a bug here. Some drivers' get_ust_msc hooks
> > > (UXA, modesetting) return BadMatch and leave crtc_msc uninitialized.  
> > > At which point we'd happily continue and use rubbish values.
> > > 
> > > I discovered this today as well, when using valgrind to try and debug
> > > some crashes in xf86-video-modesetting using my 'pageflip' branch:
> > > [term1]# valgrind --vgdb-error=1 X
> > > [term2]# gdb
> > > [term2](gdb) target remote | vgdb
> > > [term2](gdb) c
> > > [term3]$ DISPLAY=:0 glxgears -fullscreen &
> > > [term3]$ DISPLAY=:0 xset force dpms off
> > > <hit a key to DPMS on>
> > > 
> > > valgrind will complain about the uninitialized values in the crtc_msc >=
> > > target_msc branch.
> > > 
> > > But...is an early return really the right thing to do?  I tested this
> > > with xf86-video-modesetting and my 'pageflip' branch, and it caused
> > > "glxgears -fullscreen" to die from a BadMatch error after a single DPMS
> > > cycle.  I tried changing it to 'return Success', and that caused it to
> > > stop drawing altogether.  I also tried it with UXA+DRI3, and it seemed
> > > to break there as well.  So it seems like we need to do something more.
> > 
> > The previous patch fixes that bug.
> 
> I was running with all three of your patches in this series.

In that case, the only way we end up with an invalid target_crtc is if
either the driver tells Present to use a CRTC it cannot use with
drmWaitVBlank, or if the user requests an invalid CRTC. If the user
requests an unusable CRTC, we definitely want to report that early.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre