diff mbox

[xf96-video-intel] DRI2GetMSC: Do not send a bogus ust when no drawable is not displayed

Message ID 1364536477-7844-1-git-send-email-djkurtz@chromium.org (mailing list archive)
State New, archived
Headers show

Commit Message

Daniel Kurtz March 29, 2013, 5:54 a.m. UTC
According to the opengl glx_sync_control spec, the Unadjusted System Time
(or UST) is a 64-bit monotonically increasing counter that is available
throughout the system:
http://www.opengl.org/registry/specs/OML/glx_sync_control.txt

Therefore, sending 0, even in this corner case, is out of spec.
Instead, just return FALSE indicating that the operation could not be
completed.

Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
---
 src/intel_dri.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

Comments

Chris Wilson March 29, 2013, 9:07 a.m. UTC | #1
On Fri, Mar 29, 2013 at 01:54:37PM +0800, Daniel Kurtz wrote:
> According to the opengl glx_sync_control spec, the Unadjusted System Time
> (or UST) is a 64-bit monotonically increasing counter that is available
> throughout the system:
> http://www.opengl.org/registry/specs/OML/glx_sync_control.txt
> 
> Therefore, sending 0, even in this corner case, is out of spec.
> Instead, just return FALSE indicating that the operation could not be
> completed.

The problem is that ends up sending BadDrawable back to the client,
which ends up killing many clients and a compositor or two due to the
unexpected error.

Would you prefer querying pipe 0 in this case for the UST?
-Chris
Stéphane Marchesin March 29, 2013, 10:13 a.m. UTC | #2
On Fri, Mar 29, 2013 at 2:07 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Fri, Mar 29, 2013 at 01:54:37PM +0800, Daniel Kurtz wrote:
>> According to the opengl glx_sync_control spec, the Unadjusted System Time
>> (or UST) is a 64-bit monotonically increasing counter that is available
>> throughout the system:
>> http://www.opengl.org/registry/specs/OML/glx_sync_control.txt
>>
>> Therefore, sending 0, even in this corner case, is out of spec.
>> Instead, just return FALSE indicating that the operation could not be
>> completed.
>
> The problem is that ends up sending BadDrawable back to the client,
> which ends up killing many clients and a compositor or two due to the
> unexpected error.
>
> Would you prefer querying pipe 0 in this case for the UST?

For us it comes down to the fact that we'd like something monotonous.
So yes querying pipe 0 could work if it's enabled. But I wonder what
do you do when you have no enabled pipe though? Return the last UST?

Stéphane
Chris Wilson March 29, 2013, 12:54 p.m. UTC | #3
On Fri, Mar 29, 2013 at 03:13:35AM -0700, Stéphane Marchesin wrote:
> On Fri, Mar 29, 2013 at 2:07 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > On Fri, Mar 29, 2013 at 01:54:37PM +0800, Daniel Kurtz wrote:
> >> According to the opengl glx_sync_control spec, the Unadjusted System Time
> >> (or UST) is a 64-bit monotonically increasing counter that is available
> >> throughout the system:
> >> http://www.opengl.org/registry/specs/OML/glx_sync_control.txt
> >>
> >> Therefore, sending 0, even in this corner case, is out of spec.
> >> Instead, just return FALSE indicating that the operation could not be
> >> completed.
> >
> > The problem is that ends up sending BadDrawable back to the client,
> > which ends up killing many clients and a compositor or two due to the
> > unexpected error.
> >
> > Would you prefer querying pipe 0 in this case for the UST?
> 
> For us it comes down to the fact that we'd like something monotonous.
> So yes querying pipe 0 could work if it's enabled. But I wonder what
> do you do when you have no enabled pipe though? Return the last UST?

That's a problem as we would need a running pipe, which is not
guaranteed. I wonder if CLOCK_MONOTONIC would suffice?
-Chris
Stéphane Marchesin March 29, 2013, 4:54 p.m. UTC | #4
On Fri, Mar 29, 2013 at 5:54 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Fri, Mar 29, 2013 at 03:13:35AM -0700, Stéphane Marchesin wrote:
>> On Fri, Mar 29, 2013 at 2:07 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>> > On Fri, Mar 29, 2013 at 01:54:37PM +0800, Daniel Kurtz wrote:
>> >> According to the opengl glx_sync_control spec, the Unadjusted System Time
>> >> (or UST) is a 64-bit monotonically increasing counter that is available
>> >> throughout the system:
>> >> http://www.opengl.org/registry/specs/OML/glx_sync_control.txt
>> >>
>> >> Therefore, sending 0, even in this corner case, is out of spec.
>> >> Instead, just return FALSE indicating that the operation could not be
>> >> completed.
>> >
>> > The problem is that ends up sending BadDrawable back to the client,
>> > which ends up killing many clients and a compositor or two due to the
>> > unexpected error.
>> >
>> > Would you prefer querying pipe 0 in this case for the UST?
>>
>> For us it comes down to the fact that we'd like something monotonous.
>> So yes querying pipe 0 could work if it's enabled. But I wonder what
>> do you do when you have no enabled pipe though? Return the last UST?
>
> That's a problem as we would need a running pipe, which is not
> guaranteed. I wonder if CLOCK_MONOTONIC would suffice?

Yeah that's what I had in mind. Would that work for you?

Stéphane
Chris Wilson April 2, 2013, 10:05 a.m. UTC | #5
On Fri, Mar 29, 2013 at 09:54:50AM -0700, Stéphane Marchesin wrote:
> On Fri, Mar 29, 2013 at 5:54 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > That's a problem as we would need a running pipe, which is not
> > guaranteed. I wonder if CLOCK_MONOTONIC would suffice?
> 
> Yeah that's what I had in mind. Would that work for you?

I think so, the value looks like it should be consistent with the
kernel. I've applied a patch to return the CLOCK_MONOTONIC usec, thanks.
-Chris
diff mbox

Patch

diff --git a/src/intel_dri.c b/src/intel_dri.c
index f351203..179bfb7 100644
--- a/src/intel_dri.c
+++ b/src/intel_dri.c
@@ -1339,12 +1339,9 @@  I830DRI2GetMSC(DrawablePtr draw, CARD64 *ust, CARD64 *msc)
 	drmVBlank vbl;
 	int ret, pipe = I830DRI2DrawablePipe(draw);
 
-	/* Drawable not displayed, make up a value */
-	if (pipe == -1) {
-		*ust = 0;
-		*msc = 0;
-		return TRUE;
-	}
+	/* Drawable not displayed, return FALSE */
+	if (pipe == -1)
+		return FALSE;
 
 	vbl.request.type = DRM_VBLANK_RELATIVE | pipe_select(pipe);
 	vbl.request.sequence = 0;