[RFC weston 00/14] Atomic modesetting support

[Pekka Paalanen]

On Thu, 21 May 2015 11:55:31 +0300
Pekka Paalanen <ppaalanen at gmail.com> wrote:

> On Thu, 21 May 2015 08:34:56 +0100
> Daniel Stone <daniel at fooishbar.org> wrote:
> 
> > Hi,
> > 
> > On 21 May 2015 at 08:28, Daniel Stone <daniels at collabora.com> wrote:
> > > This patchset is an experimental/RFC implementation of atomic modesetting
> > > for Weston, as a proof-of-concept for the overall kernel API. It still
> > > definitely has some rough edges, and safe to say it isn't 1.8 material,
> > > but might be useful to look at regardless, especially for those of you
> > > interested in compositor-drm internals.
> > 
> > This is also available at:
> > git://git.collabora.com/git/user/daniels/weston wip/atomic-modeset

Hi,

here's my read-through review.

Patch 1: R-b, trivially mergable immediately if we want to (before
	1.8-RC2).

Patches 2-4: authored by me, still looking good.

Patch 5: needs fixing, comments sent.

Patch 6: R-b.

Patch 7: Mostly ok, fixing double call to weston_plane_init() would be
good. Patch 11 eventually removes the extra init call.

Patch 8: could use a litte fixing, comments sent, but looking good.
	Good job on the docs!

Patch 9: This should probably also rename sprite_list to plane_list?
	Ok otherwise.

Patch 10: R-b.

Patch 11: Hmm, maybe could squash patch 10 here? You change the type of
	'cursor_plane' so you need to touch at least all the places
	patch 10 does.

	The "Gross open-coding" could be a separate function
	drm_plane_create_legacy_cursor() or something?
	The legacy cursor is not added to 'sprite_list', intentional?

	I think it is leaking the legacy version of 'cursor_plane' on
	exit.

Patch 12: Comments sent. It's a bit odd, but I think it also works.

Patch 13: struct drm_output::{current, next} are now unused and should
	be removed.

	I think this is leaking the legacy version of 'primary_plane'
	as it is never added to 'sprite_list'.

	Otherwise ok.

Patch 14: What libdrm branch are we supposed to use again?

	Only minor comments sent, it looks very good.
	IMHO certainly enough to excercise kernel interfaces.


We should have probably reverted

commit 6858383d51b12632481370fdc7d886a1e6bb4ebd
Author: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
Date:   Tue May 19 09:53:16 2015 +0300

    compositor-drm: disable hardware cursors

as the first thing in your branch, so that until the atomic patch, we
could still test legacy cursors. Debug options for disabling atomic or
universal planes would be nice for regression testing.

If people are interested in doing regression testing, patches 1-13
should not affect Weston's apparent operation on DRM. These patches
also do not require libdrm updates (if you have 2.4.47 or later) nor
kernel updates.

Patch 14 needs a version of libdrm that supports atomic. "Old" kernels
*should* keep on working as well as before.

To really test the atomic code paths, you need the kernel branch
mentioned in an earlier email.

Looking good to me!


Thanks,
pq