[PATCH 1/5] modesetting: Improve the ms_flush_drm_events() API.

[Michel Dänzer]

On 22.04.2015 09:58, Kenneth Graunke wrote:
> Previously, ms_flush_drm_events() returned a boolean value, and it was
> very easy to interpret the meaning incorrectly.  Now, we return an
> integer value.
> 
> The possible outcomes of this call are:
> - poll() raised an error (formerly TRUE, now -1 - poll's return value)
> - poll() said there are no events (formerly TRUE, now 0).
> - drmHandleEvent() raised an error (formerly FALSE, now the negative
>   value returned by drmHandleEvent).
> - An event was successfully handled (formerly TRUE, now 1).
> 
> The nice part is that this allows you to distinguish errors (< 0),
> nothing to do (= 0), and success (1).  We no longer conflate errors
> with success.
> 
> Signed-off-by: Kenneth Graunke <kenneth at whitecape.org>
> ---
>  hw/xfree86/drivers/modesetting/present.c | 23 +++++++++++++++++++----
>  1 file changed, 19 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/xfree86/drivers/modesetting/present.c b/hw/xfree86/drivers/modesetting/present.c
> index 359e113..3e8e72a 100644
> --- a/hw/xfree86/drivers/modesetting/present.c
> +++ b/hw/xfree86/drivers/modesetting/present.c
> @@ -72,8 +72,11 @@ ms_present_get_ust_msc(RRCrtcPtr crtc, CARD64 *ust, CARD64 *msc)
>  
>  /*
>   * Flush the DRM event queue when full; makes space for new events.
> + *
> + * Returns a negative value on error, 0 if there was nothing to process,
> + * or 1 if we handled any events.
>   */
> -static Bool
> +static int
>  ms_flush_drm_events(ScreenPtr screen)
>  {
>      ScrnInfoPtr scrn = xf86ScreenToScrn(screen);
> @@ -86,10 +89,19 @@ ms_flush_drm_events(ScreenPtr screen)
>              r = poll(&p, 1, 0);
>      } while (r == -1 && (errno == EINTR || errno == EAGAIN));
>  
> +    /* If there was an error, r will be < 0.  Return that.  If there was
> +     * nothing to process, r == 0.  Return that.
> +     */
>      if (r <= 0)
> -        return TRUE;
> +        return r;

AFAICT the only functional change of this patch is that this case of
poll() returning <= 0 is now treated as failure, whereas it was
previously treated as success. Not sure that warrants the API change,
but anyway I think it would be good to have that one-liner bugfix in a
separate change instead of buried in the API change.


> +    /* Try to handle the event.  If there was an error, return it. */
> +    r = drmHandleEvent(ms->fd, &ms->event_context);
> +    if (r < 0)
> +        return r;
>  
> -    return drmHandleEvent(ms->fd, &ms->event_context) >= 0;
> +    /* Otherwise return 1 to indicate that we handled an event. */
> +    return 1;
>  }
>  
>  /*
> @@ -159,7 +171,10 @@ ms_present_queue_vblank(RRCrtcPtr crtc,
>          ret = drmWaitVBlank(ms->fd, &vbl);
>          if (!ret)
>              break;
> -        if (errno != EBUSY || !ms_flush_drm_events(screen))
> +        /* If we hit EBUSY, then try to flush events.  If we can't, then
> +         * this is an error.
> +         */
> +        if (errno != EBUSY || ms_flush_drm_events(screen) <= 0)
>              return BadAlloc;
>      }
>      DebugPresent(("\t\tmq %lld seq %u msc %llu (hw msc %u)\n",
> 


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer