Proposal of the z-order patch

Proposal of the z-order patch

[Yu Dai]

All,

Thanks for the review of z-order patch. Based on that, we had some 
internal discussion within our Display driver team. Here is the summary.

1. Re: define plane z-order combinations as an enum property

User mode needs extra work to understand the definition. The number of 
combinations expands dramatically with the increasing of planes. Says 
for 4 planes, the total combo is 24.

2. Re: per-plane z-order property

We prefer the atomic setup of z-order for all planes. So driver can 
validate the property value in the SetProperty call. Otherwise, if for 
each plane the separated SetProperty call is made to update the z-order, 
it could be difficult to handle and validate. For example, when set a 
plane to zorder 2, there might be another plane with zorder 2 already. 
In the end, we still need another check point to validate this.

3. We also discussed the possibility of using blob property

Some generic blob property calls are needed to handle a property with a 
list of plane id and zorder etc. But currently it is lack of support in 
libdrm to pass down user data via blob property. The current 
drm_mode_obj_set_property really doesn't handle blob. It might be useful 
to implement a property that can handle complicated data structure from 
user. But this is long term goal.

The new proposal we have is still based on previous solution (64bits 
property) but it tries to make the code platform agnostic and eliminate 
the magic number like PASASBCA.

Define the supported plane name tag in i915_drm.h. For i915, it might be
#define DRM_I915_PLANE_PRIMARY    0
#define DRM_I915_PLANE_SPRITE_A   1
#define DRM_I915_PLANE_SPRITE_B   2
#define DRM_I915_PLANE_CURSOR     3

Limits the above setting within 0~15. So we need 4 bits to save it into 
z-order property. The 64bits of z-order will indicate the order of 
planes from bottom (lower bits) to top (higher bits) by every 4 bits. 
Bit 3:0 is the plane at bottom; Bit 7:4 is the one right on top of it 
and so on. In this way, we can support up to 16 planes total. And it is 
very straightforward for user mode to use. The SetProperty call will 
reject any invalid setting. The HW specific code will be wrapped up in 
kernel SetProperty callback. Be note on Intel platform, the 
PLANE_SPRITE_A/B will be mapped to HW Sprite C/D for pipe 2. And this is 
crtc property - we don't have to pass around pipe id.

Please let us know your opinion on this.

Thanks,

Alex
GCM Core uArch, Intel VGP

Re: Proposal of the z-order patch

[Matt Roper]

On Thu, Mar 20, 2014 at 11:53:07AM -0700, Yu Dai wrote:
> All,
> 
> Thanks for the review of z-order patch. Based on that, we had some
> internal discussion within our Display driver team. Here is the
> summary.
> 
> 1. Re: define plane z-order combinations as an enum property
> 
> User mode needs extra work to understand the definition. The number
> of combinations expands dramatically with the increasing of planes.
> Says for 4 planes, the total combo is 24.
> 
> 2. Re: per-plane z-order property
> 
> We prefer the atomic setup of z-order for all planes. So driver can
> validate the property value in the SetProperty call. Otherwise, if
> for each plane the separated SetProperty call is made to update the
> z-order, it could be difficult to handle and validate. For example,
> when set a plane to zorder 2, there might be another plane with
> zorder 2 already. In the end, we still need another check point to
> validate this.
> 
> 3. We also discussed the possibility of using blob property
> 
> Some generic blob property calls are needed to handle a property
> with a list of plane id and zorder etc. But currently it is lack of
> support in libdrm to pass down user data via blob property. The
> current drm_mode_obj_set_property really doesn't handle blob. It
> might be useful to implement a property that can handle complicated
> data structure from user. But this is long term goal.
> 
> The new proposal we have is still based on previous solution (64bits
> property) but it tries to make the code platform agnostic and
> eliminate the magic number like PASASBCA.
> 
> Define the supported plane name tag in i915_drm.h. For i915, it might be
> #define DRM_I915_PLANE_PRIMARY    0
> #define DRM_I915_PLANE_SPRITE_A   1
> #define DRM_I915_PLANE_SPRITE_B   2
> #define DRM_I915_PLANE_CURSOR     3
> 
> Limits the above setting within 0~15. So we need 4 bits to save it
> into z-order property. The 64bits of z-order will indicate the order
> of planes from bottom (lower bits) to top (higher bits) by every 4
> bits. Bit 3:0 is the plane at bottom; Bit 7:4 is the one right on
> top of it and so on. In this way, we can support up to 16 planes
> total. And it is very straightforward for user mode to use. The
> SetProperty call will reject any invalid setting. The HW specific
> code will be wrapped up in kernel SetProperty callback. Be note on
> Intel platform, the PLANE_SPRITE_A/B will be mapped to HW Sprite C/D
> for pipe 2. And this is crtc property - we don't have to pass around
> pipe id.
> 
> Please let us know your opinion on this.
> 
> Thanks,
> 
> Alex
> GCM Core uArch, Intel VGP

Hi Alex.

I think the first question here is whether you're proposing a solution
for a pre-atomic or post-atomic driver stack?  Based on your description
of option #2, it sounds like you're focusing on a pre-atomic environment
since you're using SetProperty and drm_mode_obj_set_property.  With the
atomic framework, you can use drmModePropertySetAdd() to add a bunch of
desired property values to a single userspace property set object, then
dispatch that whole set to the kernel with drmModePropertySetCommit()
which calls the DRM_IOCTL_MODE_ATOMIC ioctl.  The kernel can sanity
check the whole batch of property values and then either accept or
reject the whole set together in a transactional manner (so you don't
have to worry about winding up in some strange limbo where some of your
changes were good and took effect but others weren't).

Not to say your proposed solution directly conflicts with the atomic
programming approach in any way (programming a single CRTC property
works the same way as programming a bunch of separate plane properties
in that case); I just want to make sure you understand the direction
that we're expecting the API's to go in the near (hopefully!) future so
that we can make sure we're settling on the best possible approach for
z-order handling for the long term.

For the specific approach you propose above, can you clarify what the
logic looks like from the point of view of a generic (non hw-specific)
userspace compositor (e.g., Weston)?  The compositor, which starts out
knowing nothing at all about the underlying hardware, would call
drmModeGetPlaneResources() and get a list of all the planes that are
present.  How does it know which of the HW-specific plane ID's you
#define above correspond to which planes in the plane list it received?
Are these ID's going to show up as immutable plane properties or
something that userspace can read?  Is it going to assume that those
values correspond to the order that planes show up in the plane list?
One of the advantages of using a single value to program z-order as
you're doing here is that presumably there can be a mechanism to
advertise what the valid plane orderings are so that the compositor
doesn't have to spend some time at startup testing the various
combinations itself...do you have plans on how you'd enumerate the valid
configs for userspace?


Matt

-- 
Matt Roper
Graphics Software Engineer
ISG Platform Enabling & Development
Intel Corporation
(916) 356-2795

[PATCH] drm/i915: add support for Z-order of planes

[yu.dai]

From: "Yu(Alex) Dai" <yu.dai@intel.com>

Add "zorder" property to crtc to control Z-order of sprite and
primary planes. The plane tag is packed into the 64 bits value
in the order of from bottom (lower bits) to top (higher bits).
Four bits are used for each plane tag. It can support up to 16
planes.

The alpha channel of the plane is set during Z-order change. If
a plane is put to the bottom, its alpha channel is off. Otherwise,
its alpha channel is on.

Signed-off-by: Yu(Alex) Dai <yu.dai@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h      |   1 +
 drivers/gpu/drm/i915/i915_reg.h      |   3 +
 drivers/gpu/drm/i915/intel_display.c |  58 +++++++++++++++--
 drivers/gpu/drm/i915/intel_drv.h     |   6 ++
 drivers/gpu/drm/i915/intel_sprite.c  | 119 ++++++++++++++++++++++++++++++++++-
 include/uapi/drm/i915_drm.h          |   6 ++
 6 files changed, 186 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 9b8c1e0..8de2f45 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2656,6 +2656,7 @@ int i915_reg_read_ioctl(struct drm_device *dev, void *data,
 			struct drm_file *file);
 int i915_get_reset_stats_ioctl(struct drm_device *dev, void *data,
 			       struct drm_file *file);
+extern int i915_set_plane_zorder(struct drm_crtc *crtc, uint64_t zorder);
 
 /* overlay */
 extern struct intel_overlay_error_state *intel_overlay_capture_error_state(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 74f7d85..a0173b1 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -3778,6 +3778,9 @@ enum punit_power_well {
 #define   SPRITE_INT_GAMMA_ENABLE	(1<<13)
 #define   SPRITE_TILED			(1<<10)
 #define   SPRITE_DEST_KEY		(1<<2)
+#define   SPRITE_FORCE_BOTTOM		(1<<2)
+#define   SPRITE_ZORDER_ENABLE		(1<<0)
+
 #define _SPRA_LINOFF		0x70284
 #define _SPRA_STRIDE		0x70288
 #define _SPRA_POS		0x7028c
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 7e4ea8d..0846ce5 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2188,18 +2188,27 @@ static int i9xx_update_primary_plane(struct drm_crtc *crtc,
 		break;
 	case DRM_FORMAT_XRGB1555:
 	case DRM_FORMAT_ARGB1555:
-		dspcntr |= DISPPLANE_BGRX555;
+		if (intel_crtc->primary_alpha)
+			dspcntr |= DISPPLANE_BGRA555;
+		else
+			dspcntr |= DISPPLANE_BGRX555;
 		break;
 	case DRM_FORMAT_RGB565:
 		dspcntr |= DISPPLANE_BGRX565;
 		break;
 	case DRM_FORMAT_XRGB8888:
 	case DRM_FORMAT_ARGB8888:
-		dspcntr |= DISPPLANE_BGRX888;
+		if (intel_crtc->primary_alpha)
+			dspcntr |= DISPPLANE_BGRA888;
+		else
+			dspcntr |= DISPPLANE_BGRX888;
 		break;
 	case DRM_FORMAT_XBGR8888:
 	case DRM_FORMAT_ABGR8888:
-		dspcntr |= DISPPLANE_RGBX888;
+		if (intel_crtc->primary_alpha)
+			dspcntr |= DISPPLANE_RGBA888;
+		else
+			dspcntr |= DISPPLANE_RGBX888;
 		break;
 	case DRM_FORMAT_XRGB2101010:
 	case DRM_FORMAT_ARGB2101010:
@@ -2207,7 +2216,10 @@ static int i9xx_update_primary_plane(struct drm_crtc *crtc,
 		break;
 	case DRM_FORMAT_XBGR2101010:
 	case DRM_FORMAT_ABGR2101010:
-		dspcntr |= DISPPLANE_RGBX101010;
+		if (intel_crtc->primary_alpha)
+			dspcntr |= DISPPLANE_RGBA101010;
+		else
+			dspcntr |= DISPPLANE_RGBX101010;
 		break;
 	default:
 		BUG();
@@ -8492,6 +8504,9 @@ static void intel_crtc_destroy(struct drm_crtc *crtc)
 
 	intel_crtc_cursor_set(crtc, NULL, 0, 0, 0);
 
+	if (intel_crtc->zorder_property)
+		drm_property_destroy(dev, intel_crtc->zorder_property);
+
 	drm_crtc_cleanup(crtc);
 
 	kfree(intel_crtc);
@@ -10428,6 +10443,27 @@ out_config:
 	return ret;
 }
 
+static int intel_crtc_set_property(struct drm_crtc *crtc,
+				   struct drm_property *prop,
+				   uint64_t val)
+{
+	struct intel_crtc *icrtc = to_intel_crtc(crtc);
+	int ret = -ENOENT;
+
+	if (prop && prop == icrtc->zorder_property) {
+		/* Simply return if no change in zorder */
+		if (icrtc->zorder == val)
+			return 0;
+
+		ret = i915_set_plane_zorder(crtc, val);
+
+		if (!ret)
+			icrtc->zorder = val;
+	}
+
+	return ret;
+}
+
 static const struct drm_crtc_funcs intel_crtc_funcs = {
 	.cursor_set = intel_crtc_cursor_set,
 	.cursor_move = intel_crtc_cursor_move,
@@ -10435,6 +10471,7 @@ static const struct drm_crtc_funcs intel_crtc_funcs = {
 	.set_config = intel_crtc_set_config,
 	.destroy = intel_crtc_destroy,
 	.page_flip = intel_crtc_page_flip,
+	.set_property = intel_crtc_set_property,
 };
 
 static void intel_cpu_pll_init(struct drm_device *dev)
@@ -10542,6 +10579,7 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
 {
 	drm_i915_private_t *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc;
+	struct drm_property *prop = 0;
 	int i;
 
 	intel_crtc = kzalloc(sizeof(*intel_crtc), GFP_KERNEL);
@@ -10584,6 +10622,18 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
 	dev_priv->pipe_to_crtc_mapping[intel_crtc->pipe] = &intel_crtc->base;
 
 	drm_crtc_helper_add(&intel_crtc->base, &intel_helper_funcs);
+
+	intel_crtc->primary_alpha = false;
+	intel_crtc->sprite0_alpha = true;
+	intel_crtc->sprite1_alpha = true;
+
+	if (IS_VALLEYVIEW(dev))
+		prop = drm_property_create(dev, 0, "zorder", 1);
+
+	if (prop)
+		drm_object_attach_property(&intel_crtc->base.base, prop, 0);
+
+	intel_crtc->zorder_property = prop;
 }
 
 enum pipe intel_get_pipe_from_connector(struct intel_connector *connector)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index fa99104..6d90629 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -382,6 +382,9 @@ struct intel_crtc {
 	bool new_enabled;
 
 	uint32_t ddi_pll_sel;
+	bool primary_alpha;
+	bool sprite0_alpha;
+	bool sprite1_alpha;
 
 	/* reset counter value when the last flip was submitted */
 	unsigned int reset_counter;
@@ -395,6 +398,9 @@ struct intel_crtc {
 		/* watermarks currently being used  */
 		struct intel_pipe_wm active;
 	} wm;
+
+	struct drm_property *zorder_property;
+	uint64_t zorder;
 };
 
 struct intel_plane_wm_parameters {
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 336ae6c..fb495d9 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -37,6 +37,101 @@
 #include <drm/i915_drm.h>
 #include "i915_drv.h"
 
+static int vlv_set_plane_zorder(struct drm_crtc *crtc, uint64_t order)
+{
+	struct drm_i915_private *dev_priv = crtc->dev->dev_private;
+	u32 val;
+	int s1_zorder = 0, s1_bottom = 0, s2_zorder = 0, s2_bottom = 0;
+	unsigned int i, z_order[I915_PLANE_MAX_NUM];
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	int pipe = intel_crtc->pipe;
+
+	/* Get plane order from bottom (lower bits) to top (higher bits) */
+	for (i = 0; i < I915_PLANE_MAX_NUM; i++)
+		z_order[i] = (order >> (i*4)) & 0xF;
+
+	/* The top layer must be cursor */
+	if (z_order[i-1] != I915_PLANE_CURSOR)
+		return -EINVAL;
+
+	/* No duplicated setting */
+	if (z_order[0] == z_order[1] ||
+		z_order[0] == z_order[2] ||
+		z_order[1] == z_order[2])
+		return -EINVAL;
+
+	switch (z_order[0]) {
+	case I915_PLANE_PRIMARY:
+		if (z_order[1] == I915_PLANE_SPRITE_B)
+			s1_zorder = 1;
+		break;
+	case I915_PLANE_SPRITE_A:
+		s1_bottom = 1;
+		if (z_order[1] == I915_PLANE_SPRITE_B)
+			s2_zorder = 1;
+		break;
+	case I915_PLANE_SPRITE_B:
+		s2_bottom = 1;
+		if (z_order[1] == I915_PLANE_SPRITE_A)
+			s1_zorder = 1;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	/* Clear the older Z-order */
+	val = I915_READ(SPCNTR(pipe, 0));
+	val &= ~(SPRITE_FORCE_BOTTOM | SPRITE_ZORDER_ENABLE);
+	I915_WRITE(SPCNTR(pipe, 0), val);
+
+	val = I915_READ(SPCNTR(pipe, 1));
+	val &= ~(SPRITE_FORCE_BOTTOM | SPRITE_ZORDER_ENABLE);
+	I915_WRITE(SPCNTR(pipe, 1), val);
+
+	/* Program new Z-order */
+	val = I915_READ(SPCNTR(pipe, 0));
+	if (s1_zorder)
+		val |= SPRITE_ZORDER_ENABLE;
+	if (s1_bottom)
+		val |= SPRITE_FORCE_BOTTOM;
+	I915_WRITE(SPCNTR(pipe, 0), val);
+
+	val = I915_READ(SPCNTR(pipe, 1));
+	if (s2_zorder)
+		val |= SPRITE_ZORDER_ENABLE;
+	if (s2_bottom)
+		val |= SPRITE_FORCE_BOTTOM;
+	I915_WRITE(SPCNTR(pipe, 1), val);
+
+	/* If a plane is not at bottom, the alpha channel should be on. */
+	if (z_order[0] != I915_PLANE_PRIMARY)
+		intel_crtc->primary_alpha = true;
+	else
+		intel_crtc->primary_alpha = false;
+
+	if (z_order[0] != I915_PLANE_SPRITE_A)
+		intel_crtc->sprite0_alpha = true;
+	else
+		intel_crtc->sprite0_alpha = false;
+
+	if (z_order[0] != I915_PLANE_SPRITE_B)
+		intel_crtc->sprite1_alpha = true;
+	else
+		intel_crtc->sprite1_alpha = false;
+
+	return 0;
+}
+
+int i915_set_plane_zorder(struct drm_crtc *crtc, uint64_t val)
+{
+	int ret = -ENOENT;
+
+	if (IS_VALLEYVIEW(crtc->dev))
+		ret = vlv_set_plane_zorder(crtc, val);
+
+	return ret;
+}
+
 static void
 vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
 		 struct drm_framebuffer *fb,
@@ -50,10 +145,19 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
 	struct intel_plane *intel_plane = to_intel_plane(dplane);
 	int pipe = intel_plane->pipe;
 	int plane = intel_plane->plane;
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	u32 sprctl;
+	bool alpha = true;
 	unsigned long sprsurf_offset, linear_offset;
 	int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
 
+	if (plane && intel_crtc->sprite1_alpha)
+		alpha = true;
+	else if (!plane && intel_crtc->sprite0_alpha)
+		alpha = true;
+	else
+		alpha = false;
+
 	sprctl = I915_READ(SPCNTR(pipe, plane));
 
 	/* Mask out pixel format bits in case we change it */
@@ -81,19 +185,28 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
 		sprctl |= SP_FORMAT_BGRX8888;
 		break;
 	case DRM_FORMAT_ARGB8888:
-		sprctl |= SP_FORMAT_BGRA8888;
+		if (alpha)
+			sprctl |= SP_FORMAT_BGRA8888;
+		else
+			sprctl |= SP_FORMAT_BGRX8888;
 		break;
 	case DRM_FORMAT_XBGR2101010:
 		sprctl |= SP_FORMAT_RGBX1010102;
 		break;
 	case DRM_FORMAT_ABGR2101010:
-		sprctl |= SP_FORMAT_RGBA1010102;
+		if (alpha)
+			sprctl |= SP_FORMAT_RGBA1010102;
+		else
+			sprctl |= SP_FORMAT_RGBX1010102;
 		break;
 	case DRM_FORMAT_XBGR8888:
 		sprctl |= SP_FORMAT_RGBX8888;
 		break;
 	case DRM_FORMAT_ABGR8888:
-		sprctl |= SP_FORMAT_RGBA8888;
+		if (alpha)
+			sprctl |= SP_FORMAT_RGBA8888;
+		else
+			sprctl |= SP_FORMAT_RGBX8888;
 		break;
 	default:
 		/*
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index 126bfaa..df12b5d 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -1049,4 +1049,10 @@ struct drm_i915_reset_stats {
 	__u32 pad;
 };
 
+#define I915_PLANE_PRIMARY		0
+#define I915_PLANE_SPRITE_A		1
+#define I915_PLANE_SPRITE_B		2
+#define I915_PLANE_CURSOR		3
+#define I915_PLANE_MAX_NUM		(I915_PLANE_CURSOR + 1)
+
 #endif /* _UAPI_I915_DRM_H_ */
-- 
1.8.3.2

Re: Proposal of the z-order patch

[Yu Dai]

On 03/21/2014 04:03 PM, Matt Roper wrote:
> On Thu, Mar 20, 2014 at 11:53:07AM -0700, Yu Dai wrote:
>> All,
>>
>> Thanks for the review of z-order patch. Based on that, we had some
>> internal discussion within our Display driver team. Here is the
>> summary.
>>
>> 1. Re: define plane z-order combinations as an enum property
>>
>> User mode needs extra work to understand the definition. The number
>> of combinations expands dramatically with the increasing of planes.
>> Says for 4 planes, the total combo is 24.
>>
>> 2. Re: per-plane z-order property
>>
>> We prefer the atomic setup of z-order for all planes. So driver can
>> validate the property value in the SetProperty call. Otherwise, if
>> for each plane the separated SetProperty call is made to update the
>> z-order, it could be difficult to handle and validate. For example,
>> when set a plane to zorder 2, there might be another plane with
>> zorder 2 already. In the end, we still need another check point to
>> validate this.
>>
>> 3. We also discussed the possibility of using blob property
>>
>> Some generic blob property calls are needed to handle a property
>> with a list of plane id and zorder etc. But currently it is lack of
>> support in libdrm to pass down user data via blob property. The
>> current drm_mode_obj_set_property really doesn't handle blob. It
>> might be useful to implement a property that can handle complicated
>> data structure from user. But this is long term goal.
>>
>> The new proposal we have is still based on previous solution (64bits
>> property) but it tries to make the code platform agnostic and
>> eliminate the magic number like PASASBCA.
>>
>> Define the supported plane name tag in i915_drm.h. For i915, it might be
>> #define DRM_I915_PLANE_PRIMARY    0
>> #define DRM_I915_PLANE_SPRITE_A   1
>> #define DRM_I915_PLANE_SPRITE_B   2
>> #define DRM_I915_PLANE_CURSOR     3
>>
>> Limits the above setting within 0~15. So we need 4 bits to save it
>> into z-order property. The 64bits of z-order will indicate the order
>> of planes from bottom (lower bits) to top (higher bits) by every 4
>> bits. Bit 3:0 is the plane at bottom; Bit 7:4 is the one right on
>> top of it and so on. In this way, we can support up to 16 planes
>> total. And it is very straightforward for user mode to use. The
>> SetProperty call will reject any invalid setting. The HW specific
>> code will be wrapped up in kernel SetProperty callback. Be note on
>> Intel platform, the PLANE_SPRITE_A/B will be mapped to HW Sprite C/D
>> for pipe 2. And this is crtc property - we don't have to pass around
>> pipe id.
>>
>> Please let us know your opinion on this.
>>
>> Thanks,
>>
>> Alex
>> GCM Core uArch, Intel VGP
> Hi Alex.
>
> I think the first question here is whether you're proposing a solution
> for a pre-atomic or post-atomic driver stack?  Based on your description
> of option #2, it sounds like you're focusing on a pre-atomic environment
> since you're using SetProperty and drm_mode_obj_set_property.  With the
> atomic framework, you can use drmModePropertySetAdd() to add a bunch of
> desired property values to a single userspace property set object, then
> dispatch that whole set to the kernel with drmModePropertySetCommit()
> which calls the DRM_IOCTL_MODE_ATOMIC ioctl.  The kernel can sanity
> check the whole batch of property values and then either accept or
> reject the whole set together in a transactional manner (so you don't
> have to worry about winding up in some strange limbo where some of your
> changes were good and took effect but others weren't).
>
> Not to say your proposed solution directly conflicts with the atomic
> programming approach in any way (programming a single CRTC property
> works the same way as programming a bunch of separate plane properties
> in that case); I just want to make sure you understand the direction
> that we're expecting the API's to go in the near (hopefully!) future so
> that we can make sure we're settling on the best possible approach for
> z-order handling for the long term.
Agree.
> For the specific approach you propose above, can you clarify what the
> logic looks like from the point of view of a generic (non hw-specific)
> userspace compositor (e.g., Weston)?  The compositor, which starts out
> knowing nothing at all about the underlying hardware, would call
> drmModeGetPlaneResources() and get a list of all the planes that are
> present.  How does it know which of the HW-specific plane ID's you
> #define above correspond to which planes in the plane list it received?
> Are these ID's going to show up as immutable plane properties or
> something that userspace can read?  Is it going to assume that those
> values correspond to the order that planes show up in the plane list?
> One of the advantages of using a single value to program z-order as
> you're doing here is that presumably there can be a mechanism to
> advertise what the valid plane orderings are so that the compositor
> doesn't have to spend some time at startup testing the various
> combinations itself...do you have plans on how you'd enumerate the valid
> configs for userspace?
>
I submitted the new patch for review. It is still based on the 
assumption that user knows how those surfaces are mapped to HW planes. 
Such as, which one is primary and which one is sprite A etc. If not such 
case, it won't work.

Alex