[PATCH 1/2] Always call the flush callback chain when we might send out damage events

[Kristian Høgsberg]

2010/8/2 Keith Packard <keithp at keithp.com>:
> On Mon, 2 Aug 2010 08:24:04 -0400, Kristian Høgsberg <krh at bitplanet.net> wrote:
>> On Sun, Aug 1, 2010 at 3:20 PM, Keith Packard <keithp at keithp.com> wrote:
>> > On Sun, 1 Aug 2010 14:39:46 -0400, Kristian Høgsberg <krh at bitplanet.net> wrote:
>> >> Before this gets drowned out in janitorial patches... Keith, do you
>> >> want a pull request for this and the damageext patch?  Did you have
>> >> have a look at the damage change?
>> >
>> > I read through the change and reviewed the potential impacts on the
>> > server. I'm a little concerned about having the semantics of the damage
>> > extension accidentally change as the code paths for the post-activity
>> > damage region processing.
>>
>> Half a sentence missing here?
>
> Yeah, missed the 'are completely different than the pre-activity code path'
>
>> The patch is not changing the
>> semantics, just making sure that the semantics that we effectively
>> provide and clients expect is what they get (post rendering
>> notification).
>
> Sure, my concern is that within the damage code itself, the code to
> perform post-activity notification is quite convoluted and may easily
> introduce subtle changes in the events delivered to applications. I just
> don't know; I haven't really reviewed that in depth.

OK, I see.   Well, it was almost working before, except in the case
where the client io buffer overflows and we send out an event before
the driver has seen the rendering request.  Another way to fix that
would be to change the odd semantics of WriteToClient() of also
writing the reply that caused the io buffer to overflow to just
putting that in the next output buffer instead.  It looks to me like
that's just an silly little optimization - "we're going to do a writev
syscall, might as well add the reply that overflowed in a separate
iovec."  And nothing should rely on that behaviour, there's no way the
caller can know whether the reply is going to be placed in the buffer
or cause a flush.  And removing the __extraBuf argument from
FlushClient would make that code a little simpler too.

Kristian

>> Yup, it is the kind of change that requires a bit of review.
>
> I'll merge your change and then probably spend some time looking at the
> damage code and seeing if I can't clean it up a bit; right now it's
> messy with lots of different options about when and how the callbacks
> are made.
>
> Just for fun, I briefly considered having the damage extension use the
> pre-activity mode and simply pend all output to the client until the
> FlushAllOutput call from the main loop. That would have been a
> smaller change in the code paths than your patch, but would expose the X
> server to potentially buffering a fairly hefty set of events, which
> didn't seem like a great plan -- in case of malloc failure in that code
> path, the client is disconnected from the X server...
>
> --
> keith.packard at intel.com
>