[PATCH weston] compositor-drm: Don't page flip before a mode is set

Tue May 7 07:06:13 PDT 2013

On 05/07/2013 02:56 PM, Jonas Ådahl wrote:
> On Tue, May 7, 2013 at 1:16 PM, Ander Conselvan de Oliveira
> <ander.conselvan.de.oliveira at intel.com> wrote:
>> The function drm_output_start_repaint_loop() unconditionally issues a
>> page flip, even if the crtc for that output has not been enabled yet.
>> That causes the page flip to fail, and drm_output_repaint() is never
>> called.
>>
>> Solve this by bypassing the initial page flip if the output needs a
>> mode set.
>>
>> This has the caveat of affecting latency predictability for that first
>> frame and when a "driver" mode fullscreen surface causes a mode set.
>> However, on both cases the mode set would take an unpredictable amount
>> of time anyway.
>>
>> https://bugs.freedesktop.org/show_bug.cgi?id=63812
>> https://bugs.freedesktop.org/show_bug.cgi?id=64183
>> ---
>>   src/Makefile.am      |    2 +-
>>   src/compositor-drm.c |   32 +++++++++++++++++++++++++++-----
>>   2 files changed, 28 insertions(+), 6 deletions(-)
>>
>> diff --git a/src/Makefile.am b/src/Makefile.am
>> index d33ebc5..e391ecd 100644
>> --- a/src/Makefile.am
>> +++ b/src/Makefile.am
>> @@ -127,7 +127,7 @@ if ENABLE_DRM_COMPOSITOR
>>   drm_backend = drm-backend.la
>>   drm_backend_la_LDFLAGS = -module -avoid-version
>>   drm_backend_la_LIBADD = $(COMPOSITOR_LIBS) $(DRM_COMPOSITOR_LIBS) \
>> -       ../shared/libshared.la
>> +       ../shared/libshared.la -lrt
>>   drm_backend_la_CFLAGS =                                \
>>          $(COMPOSITOR_CFLAGS)                    \
>>          $(DRM_COMPOSITOR_CFLAGS)                \
>> diff --git a/src/compositor-drm.c b/src/compositor-drm.c
>> index f39096e..077e988 100644
>> --- a/src/compositor-drm.c
>> +++ b/src/compositor-drm.c
>> @@ -36,6 +36,7 @@
>>   #include <linux/input.h>
>>   #include <assert.h>
>>   #include <sys/mman.h>
>> +#include <time.h>
>>
>>   #include <xf86drm.h>
>>   #include <xf86drmMode.h>
>> @@ -51,6 +52,10 @@
>>   #include "udev-seat.h"
>>   #include "launcher-util.h"
>>
>> +#ifndef DRM_CAP_TIMESTAMP_MONOTONIC
>> +#define DRM_CAP_TIMESTAMP_MONOTONIC 0x6
>> +#endif
>> +
>>   static int option_current_mode = 0;
>>   static char *output_name;
>>   static char *output_mode;
>> @@ -113,6 +118,8 @@ struct drm_compositor {
>>          int use_pixman;
>>
>>          uint32_t prev_state;
>> +
>> +       clockid_t clock;
>>   };
>>
>>   struct drm_mode {
>> @@ -662,10 +669,19 @@ drm_output_start_repaint_loop(struct weston_output *output_base)
>>                  output_base->compositor;
>>          uint32_t fb_id;
>>
>> -       if (output->current)
>> -               fb_id = output->current->fb_id;
>> -       else
>> -               fb_id = output->original_crtc->buffer_id;
>> +       struct timespec ts;
>> +
>> +       if (!output->current) {
>> +               /* We can't page flip if there's no mode set */
>> +               uint32_t msec;
>> +
>> +               clock_gettime(compositor->clock, &ts);
>> +               msec = ts.tv_sec * 1000 + ts.tv_nsec / 1000000;
>> +               weston_output_finish_frame(output_base, msec);
>> +               return;
>> +       }
>> +
>> +       fb_id = output->current->fb_id;
>>
>>          if (drmModePageFlip(compositor->drm.fd, output->crtc_id, fb_id,
>>                              DRM_MODE_PAGE_FLIP_EVENT, output) < 0) {
>> @@ -1183,7 +1199,8 @@ static int
>>   init_drm(struct drm_compositor *ec, struct udev_device *device)
>>   {
>>          const char *filename, *sysnum;
>> -       int fd;
>> +       uint64_t cap;
>> +       int fd, ret;
>>
>>          sysnum = udev_device_get_sysnum(device);
>>          if (sysnum)
>> @@ -1206,6 +1223,11 @@ init_drm(struct drm_compositor *ec, struct udev_device *device)
>>
>>          ec->drm.fd = fd;
>>
>> +       ret = drmGetCap(fd, DRM_CAP_TIMESTAMP_MONOTONIC, &cap);
>> +       if (ret == 0 && cap == 1)
>> +               ec->clock = CLOCK_MONOTONIC;
>> +       else
>> +               ec->clock = CLOCK_REALTIME;
>
> Is it always guaranteed that the driver uses either of these time
> sources, and not some high precision internal time source? If the time
> stamps differ too much it will cause issues elsewhere.

Always is a really long time. :)  Per the current interface, the driver 
should use one of the two.  If it doesn't, that's a bug.  And kernel 
changes shouldn't break user space in the first place, so that change is 
safe in theory.

With that being said, I just looked over the kernel code and it seems 
several patches related to the monotonic clock change fell through the 
cracks, so pretty much every driver besides i915 is buggy. :/  The drm 
module will advertise the monotonic timestamp capability while the 
driver will use the real time clock.

Cheers,
Ander

> Jonas
>
>>
>>          return 0;
>>   }
>> --
>> 1.7.10.4
>>
>> _______________________________________________
>> wayland-devel mailing list
>> wayland-devel at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/wayland-devel

---------------------------------------------------------------------
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki 
Business Identity Code: 0357606 - 4 
Domiciled in Helsinki 

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.