linux-kernel - Re: [PATCH] drm/udl: Make page

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <150098318060.11053.780601557384171088@mail.alporthouse.com>
Date:   Tue, 25 Jul 2017 12:46:20 +0100
From:   Chris Wilson <chris@...is-wilson.co.uk>
To:     Dawid Kurek <dawid.kurek@...playlink.com>,
        "Dave Airlie" <airlied@...hat.com>,
        "David Airlie" <airlied@...ux.ie>, dri-devel@...ts.freedesktop.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] drm/udl: Make page_flip asynchronous

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@...omium.org
> Cc: marcheu@...omium.org
> Cc: zachr@...omium.org
> Cc: dbehr@...gle.com
> Signed-off-by: Dawid Kurek <dawid.kurek@...playlink.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@...is-wilson.co.uk>
-Chris