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

[Kenneth Graunke]

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.

I can't see how this would affect SNA at all - SNA always returns
Success from sna_present_get_ust_msc(), so it shouldn't ever hit this
error path.  Instead, you seem to be using the last values.

Maybe present_get_usc_msc should never fail, and we should have drivers
return the last values, like SNA does?

Keith, Eric - thoughts?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.x.org/archives/xorg-devel/attachments/20150208/17a80545/attachment.sig>