Re: [PATCH] drm/udl: Make page_flip asynchronous

Chris Wilson
2017-07-25T11:46:29Z

Quoting Dawid Kurek (2017-07-07 06:48:49)
> In page_flip vblank is sent with no delay. Driver does not know when the
> actual update is present on the display and has no means for getting
> this information from a device. It is practically impossible to say
> exactly *when* as there is also i.e. a usb delay.
>
> When we are unable to determine when the vblank actually happens we may
> assume it will behave accordingly, i.e. it will present frames with
> proper timing. In the worst case scenario it should take up to duration
> of one frame (we may get new frame in the device just after presenting
> current one so we would need to wait for the whole frame).
>
> Because of the asynchronous nature of the delay we need to synchronize:
> * read/write vrefresh/page_flip data when changing mode and
> preparing/executing vblank
> * USB requests to prevent interleaved access to URBs for two different
> frame buffers
>
> All those changes are backports from ChromeOS:
> 1. https://chromium-review.googlesource.com/250622
> 2. https://chromium-review.googlesource.com/249450
> partially, only change in udl_modeset.c for 'udl_flip_queue'
> 3. https://chromium-review.googlesource.com/321378
> 4. https://chromium-review.googlesource.com/324119
> + fixes for checkpatch and latest drm changes
>
> Cc: hshi@chromium.org
> Cc: marcheu@chromium.org
> Cc: zachr@chromium.org
> Cc: dbehr@google.com
> Signed-off-by: Dawid Kurek <dawid.kurek@displaylink.com>

> +struct udl_flip_queue {
> + struct mutex lock;
> + struct workqueue_struct *wq;
> + struct delayed_work work;
> + struct drm_crtc *crtc;
> + struct drm_pending_vblank_event *event;
> + u64 flip_time; /*in jiffies */
> + u64 vblank_interval; /*in jiffies */

jiffies are unsigned long. That avoids the complication of the reader
having to consider whether all the divides are 32bit safe.

mallocing this struct seems a little overkill as opposed to embedding it
tino the udl_device, flip_queue.wq provides the safety check after
initialisation.

I couldn't spot anything drastically wrong, certainly it looks complete,
I would have structured it such that everything went through the same
worker with a hrtimer emulating the vblank. That model is then but a
stone's throw away from atomic.

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris