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

From: Chris Wilson
Date: Tue Jul 25 2017 - 07:46:31 EST


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@xxxxxxxxxxxx
> Cc: marcheu@xxxxxxxxxxxx
> Cc: zachr@xxxxxxxxxxxx
> Cc: dbehr@xxxxxxxxxx
> Signed-off-by: Dawid Kurek <dawid.kurek@xxxxxxxxxxxxxxx>

> +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@xxxxxxxxxxxxxxxxxx>
-Chris