[PATCH xf86-video-intel 2/2] Fix handling of target_msc, divisor, remainder in I830DRI2ScheduleSwap()

[Jesse Barnes]

On Sun, 7 Mar 2010 09:15:46 +0100
Mario Kleiner <mario.kleiner at tuebingen.mpg.de> wrote:

> On Mar 5, 2010, at 10:09 PM, Jesse Barnes wrote:
> 
> > I just fixed these up and pushed them into the tree along with another
> > fix for handling offscreen drawables better.  Tests indicate that OML
> > swap divisor/remainder stuff is working correctly now.
> 
> 
> Thanks for doing that. But stuff got seriously garbled in  
> I830ScheduleWaitMSC() inside src/i830_dri.c - this won't work  
> correctly for any glXWaitForMscOML(target_msc, divisor, remainder)  
> call, except for the special cases with divisor == 0!
> 
> This passage...
> 
>          /*
>           * If divisor is zero, or current_msc is smaller than  
> target_msc,
>           * we just need to make sure target_msc passes  before  
> waking up the
>           * client.
>           */
>          if (divisor == 0) {
> ...
> 
> should read as...
> 
>          if (divisor == 0 ||  current_msc < target_msc) {
> 
> This passage ...
> 
>          /*
>           * If the calculated  remainder and the condition isn't  
> satisified, it
>           * means we've passed the last point where seq % divisor ==  
> remainder,
>           * so we need to wait for the next time that will happen.
>           */
>          if ((current_msc % divisor) != remainder)
>                  vbl.request.sequence += divisor;
> 
> ... should be replaced by ...
> 
>      vbl.request.sequence = current_msc - (current_msc % divisor) +  
> remainder;
> 
>      /*
>       * If calculated remainder is larger than requested remainder,  
> it means
>       * we've passed the last point where seq % divisor == remainder,  
> so we need
>       * to wait for the next time that will happen.
>       */
>      if ((current_msc % divisor) > remainder)
>         vbl.request.sequence += divisor;
> 
> 
> Other than that, it's fine.
> 
> Oh and in I830DRI2ScheduleSwap() , this statement ...
> 
>                  if (pipe > 0)
>                          vbl.request.type |= DRM_VBLANK_SECONDARY;
> 
> ... accidentally got copied twice into the if () clause, which is not  
> harmful, but a bit redundant :-)

Arg, I did botch that patch.  And of course I only tested the swap
buffers behavior and not OML's WaitMSC so I didn't catch it.  I'll
improve the test and push the fix.

Thanks,
-- 
Jesse Barnes, Intel Open Source Technology Center