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

[Chris Wilson]

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