Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: 8BIT
From: Chris Wilson <chris@chris-wilson.co.uk>
User-Agent: alot/0.3.6
To: Dawid Kurek <dawid.kurek@displaylink.com>,
        "Dave Airlie" <airlied@redhat.com>, "David Airlie" <airlied@linux.ie>,
        dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org
References: <20170707054849.GA16959@displaylink.com>
In-Reply-To: <20170707054849.GA16959@displaylink.com>
Message-ID: <150098318060.11053.780601557384171088@mail.alporthouse.com>
Subject: Re: [PATCH] drm/udl: Make page_flip asynchronous
Date: Tue, 25 Jul 2017 12:46:20 +0100
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2376
Lines: 55

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