[v3] drm/i915: add immutable zpos plane properties

Submitted by Simon Ser on April 13, 2019, 11:13 a.m.

Details

Message ID YSH9PasoADJJdNJCSdI4m55ankIBsCaoSgkw-NQ5dlruCAxc8J-SQwVl5n3ddSAMDLTdbdyQvkONmtbjkUU-TQk5VIu1p-aZRO1OjjuSxjY=@emersion.fr
State New
Headers show
Series "drm/i915: add immutable zpos plane properties" ( rev: 3 ) in Intel GFX

Browsing this patch as part of:
"drm/i915: add immutable zpos plane properties" rev 3 in Intel GFX
<< prev patch [1/1] next patch >>

Commit Message

Simon Ser April 13, 2019, 11:13 a.m.
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

This adds basic immutable support for the zpos property. The zpos increases
from bottom to top: primary, sprites, cursor.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
[contact@emersion.fr: adapted for latest drm-tip]
Signed-off-by: Simon Ser <contact@emersion.fr>
---

Maarten, could your review this patch?

Changes from v2 to v3: add the zpos property in skl_universal_plane_create too.

 drivers/gpu/drm/i915/intel_display.c | 10 ++++++++--
 drivers/gpu/drm/i915/intel_sprite.c  |  7 ++++++-
 2 files changed, 14 insertions(+), 3 deletions(-)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 8576a7f799..f0a85a75bd 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -14323,7 +14323,7 @@  intel_primary_plane_create(struct drm_i915_private *dev_priv, enum pipe pipe)
 	const u64 *modifiers;
 	const u32 *formats;
 	int num_formats;
-	int ret;
+	int ret, zpos;
 
 	if (INTEL_GEN(dev_priv) >= 9)
 		return skl_universal_plane_create(dev_priv, pipe,
@@ -14412,6 +14412,9 @@  intel_primary_plane_create(struct drm_i915_private *dev_priv, enum pipe pipe)
 						   DRM_MODE_ROTATE_0,
 						   supported_rotations);
 
+	zpos = 0;
+	drm_plane_create_zpos_immutable_property(&plane->base, zpos);
+
 	drm_plane_helper_add(&plane->base, &intel_plane_helper_funcs);
 
 	return plane;
@@ -14428,7 +14431,7 @@  intel_cursor_plane_create(struct drm_i915_private *dev_priv,
 {
 	unsigned int possible_crtcs;
 	struct intel_plane *cursor;
-	int ret;
+	int ret, zpos;
 
 	cursor = intel_plane_alloc();
 	if (IS_ERR(cursor))
@@ -14477,6 +14480,9 @@  intel_cursor_plane_create(struct drm_i915_private *dev_priv,
 						   DRM_MODE_ROTATE_0 |
 						   DRM_MODE_ROTATE_180);
 
+	zpos = RUNTIME_INFO(dev_priv)->num_sprites[pipe] + 1;
+	drm_plane_create_zpos_immutable_property(&cursor->base, zpos);
+
 	drm_plane_helper_add(&cursor->base, &intel_plane_helper_funcs);
 
 	return cursor;
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 65de7387bf..40b7bcd720 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -2333,6 +2333,8 @@  skl_universal_plane_create(struct drm_i915_private *dev_priv,
 					     BIT(DRM_MODE_BLEND_PREMULTI) |
 					     BIT(DRM_MODE_BLEND_COVERAGE));
 
+	drm_plane_create_zpos_immutable_property(&plane->base, plane_id);
+
 	drm_plane_helper_add(&plane->base, &intel_plane_helper_funcs);
 
 	return plane;
@@ -2354,7 +2356,7 @@  intel_sprite_plane_create(struct drm_i915_private *dev_priv,
 	const u64 *modifiers;
 	const u32 *formats;
 	int num_formats;
-	int ret;
+	int ret, zpos;
 
 	if (INTEL_GEN(dev_priv) >= 9)
 		return skl_universal_plane_create(dev_priv, pipe,
@@ -2444,6 +2446,9 @@  intel_sprite_plane_create(struct drm_i915_private *dev_priv,
 					  DRM_COLOR_YCBCR_BT709,
 					  DRM_COLOR_YCBCR_LIMITED_RANGE);
 
+	zpos = sprite + 1;
+	drm_plane_create_zpos_immutable_property(&plane->base, zpos);
+
 	drm_plane_helper_add(&plane->base, &intel_plane_helper_funcs);
 
 	return plane;

Comments

Op 13-04-2019 om 13:13 schreef Simon Ser:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> This adds basic immutable support for the zpos property. The zpos increases
> from bottom to top: primary, sprites, cursor.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> [contact@emersion.fr: adapted for latest drm-tip]
> Signed-off-by: Simon Ser <contact@emersion.fr>
> ---
>
> Maarten, could your review this patch?
>
> Changes from v2 to v3: add the zpos property in skl_universal_plane_create too.


Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
On Sat, Apr 13, 2019 at 11:13:27AM +0000, Simon Ser wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> This adds basic immutable support for the zpos property. The zpos increases
> from bottom to top: primary, sprites, cursor.

I was thinking a bit about how we might go about testing this.

We probably want a basic test that just checks that if any
plane has a zpos prop then all planes should have it.

A functional test would stack the planes up in some way and
compare against a software rendered reference. IIRC there was 
a zpos test case floating around but that depended on alpha
blending which we don't necessarily have.

> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> [contact@emersion.fr: adapted for latest drm-tip]
> Signed-off-by: Simon Ser <contact@emersion.fr>
> ---
> 
> Maarten, could your review this patch?
> 
> Changes from v2 to v3: add the zpos property in skl_universal_plane_create too.
> 
>  drivers/gpu/drm/i915/intel_display.c | 10 ++++++++--
>  drivers/gpu/drm/i915/intel_sprite.c  |  7 ++++++-
>  2 files changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 8576a7f799..f0a85a75bd 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -14323,7 +14323,7 @@ intel_primary_plane_create(struct drm_i915_private *dev_priv, enum pipe pipe)
>  	const u64 *modifiers;
>  	const u32 *formats;
>  	int num_formats;
> -	int ret;
> +	int ret, zpos;
>  
>  	if (INTEL_GEN(dev_priv) >= 9)
>  		return skl_universal_plane_create(dev_priv, pipe,
> @@ -14412,6 +14412,9 @@ intel_primary_plane_create(struct drm_i915_private *dev_priv, enum pipe pipe)
>  						   DRM_MODE_ROTATE_0,
>  						   supported_rotations);
>  
> +	zpos = 0;
> +	drm_plane_create_zpos_immutable_property(&plane->base, zpos);
> +
>  	drm_plane_helper_add(&plane->base, &intel_plane_helper_funcs);
>  
>  	return plane;
> @@ -14428,7 +14431,7 @@ intel_cursor_plane_create(struct drm_i915_private *dev_priv,
>  {
>  	unsigned int possible_crtcs;
>  	struct intel_plane *cursor;
> -	int ret;
> +	int ret, zpos;
>  
>  	cursor = intel_plane_alloc();
>  	if (IS_ERR(cursor))
> @@ -14477,6 +14480,9 @@ intel_cursor_plane_create(struct drm_i915_private *dev_priv,
>  						   DRM_MODE_ROTATE_0 |
>  						   DRM_MODE_ROTATE_180);
>  
> +	zpos = RUNTIME_INFO(dev_priv)->num_sprites[pipe] + 1;
> +	drm_plane_create_zpos_immutable_property(&cursor->base, zpos);
> +
>  	drm_plane_helper_add(&cursor->base, &intel_plane_helper_funcs);
>  
>  	return cursor;
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 65de7387bf..40b7bcd720 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -2333,6 +2333,8 @@ skl_universal_plane_create(struct drm_i915_private *dev_priv,
>  					     BIT(DRM_MODE_BLEND_PREMULTI) |
>  					     BIT(DRM_MODE_BLEND_COVERAGE));
>  
> +	drm_plane_create_zpos_immutable_property(&plane->base, plane_id);
> +
>  	drm_plane_helper_add(&plane->base, &intel_plane_helper_funcs);
>  
>  	return plane;
> @@ -2354,7 +2356,7 @@ intel_sprite_plane_create(struct drm_i915_private *dev_priv,
>  	const u64 *modifiers;
>  	const u32 *formats;
>  	int num_formats;
> -	int ret;
> +	int ret, zpos;
>  
>  	if (INTEL_GEN(dev_priv) >= 9)
>  		return skl_universal_plane_create(dev_priv, pipe,
> @@ -2444,6 +2446,9 @@ intel_sprite_plane_create(struct drm_i915_private *dev_priv,
>  					  DRM_COLOR_YCBCR_BT709,
>  					  DRM_COLOR_YCBCR_LIMITED_RANGE);
>  
> +	zpos = sprite + 1;
> +	drm_plane_create_zpos_immutable_property(&plane->base, zpos);
> +
>  	drm_plane_helper_add(&plane->base, &intel_plane_helper_funcs);
>  
>  	return plane;
> -- 
> 2.21.0
>
Op 16-04-2019 om 15:20 schreef Ville Syrjälä:
> On Sat, Apr 13, 2019 at 11:13:27AM +0000, Simon Ser wrote:
>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>
>> This adds basic immutable support for the zpos property. The zpos increases
>> from bottom to top: primary, sprites, cursor.
> I was thinking a bit about how we might go about testing this.
>
> We probably want a basic test that just checks that if any
> plane has a zpos prop then all planes should have it.
This would be a good test for BAT.
> A functional test would stack the planes up in some way and
> compare against a software rendered reference. IIRC there was 
> a zpos test case floating around but that depended on alpha
> blending which we don't necessarily have.

But with semi-overlapping planes you would accomplish the same, without alpha dependency.

Something like this?

[BG]   [Sprite 1]    [Cursor]
  [Primary]   [Sprite 2]

Perhaps primary fullscreen to prevent issues with hw that doesn't support partial planes?

~Maarten
On Tue, Apr 16, 2019 at 03:28:15PM +0200, Maarten Lankhorst wrote:
> Op 16-04-2019 om 15:20 schreef Ville Syrjälä:
> > On Sat, Apr 13, 2019 at 11:13:27AM +0000, Simon Ser wrote:
> >> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>
> >> This adds basic immutable support for the zpos property. The zpos increases
> >> from bottom to top: primary, sprites, cursor.
> > I was thinking a bit about how we might go about testing this.
> >
> > We probably want a basic test that just checks that if any
> > plane has a zpos prop then all planes should have it.
> This would be a good test for BAT.
> > A functional test would stack the planes up in some way and
> > compare against a software rendered reference. IIRC there was 
> > a zpos test case floating around but that depended on alpha
> > blending which we don't necessarily have.
> 
> But with semi-overlapping planes you would accomplish the same, without alpha dependency.
> 
> Something like this?
> 
> [BG]   [Sprite 1]    [Cursor]
>   [Primary]   [Sprite 2]

Should probably be good enough. Though I was pondering is there a
way to position an arbitraty number of planes such that the
resulting picture has a visible region for every possible
combination of planes?

> 
> Perhaps primary fullscreen to prevent issues with hw that doesn't support partial planes?

I guess. And maybe a second test that disables the primary
so that we can also get the bg color into the picture?
Op 16-04-2019 om 15:42 schreef Ville Syrjälä:
> On Tue, Apr 16, 2019 at 03:28:15PM +0200, Maarten Lankhorst wrote:
>> Op 16-04-2019 om 15:20 schreef Ville Syrjälä:
>>> On Sat, Apr 13, 2019 at 11:13:27AM +0000, Simon Ser wrote:
>>>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>>
>>>> This adds basic immutable support for the zpos property. The zpos increases
>>>> from bottom to top: primary, sprites, cursor.
>>> I was thinking a bit about how we might go about testing this.
>>>
>>> We probably want a basic test that just checks that if any
>>> plane has a zpos prop then all planes should have it.
>> This would be a good test for BAT.
>>> A functional test would stack the planes up in some way and
>>> compare against a software rendered reference. IIRC there was 
>>> a zpos test case floating around but that depended on alpha
>>> blending which we don't necessarily have.
>> But with semi-overlapping planes you would accomplish the same, without alpha dependency.
>>
>> Something like this?
>>
>> [BG]   [Sprite 1]    [Cursor]
>>   [Primary]   [Sprite 2]
> Should probably be good enough. Though I was pondering is there a
> way to position an arbitraty number of planes such that the
> resulting picture has a visible region for every possible
> combination of planes?

n planes, width = width / (n + 1)

position = n * 3/4 * plane_width ? or something

If each plane has its own color, then it would work..

>> Perhaps primary fullscreen to prevent issues with hw that doesn't support partial planes?
> I guess. And maybe a second test that disables the primary
> so that we can also get the bg color into the picture?

I don't think we finalized the bg color api yet, else it would be good to have..
On Tue, Apr 16, 2019 at 08:13:12PM +0200, Maarten Lankhorst wrote:
> Op 16-04-2019 om 15:42 schreef Ville Syrjälä:
> > On Tue, Apr 16, 2019 at 03:28:15PM +0200, Maarten Lankhorst wrote:
> >> Op 16-04-2019 om 15:20 schreef Ville Syrjälä:
> >>> On Sat, Apr 13, 2019 at 11:13:27AM +0000, Simon Ser wrote:
> >>>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>>>
> >>>> This adds basic immutable support for the zpos property. The zpos increases
> >>>> from bottom to top: primary, sprites, cursor.
> >>> I was thinking a bit about how we might go about testing this.
> >>>
> >>> We probably want a basic test that just checks that if any
> >>> plane has a zpos prop then all planes should have it.
> >> This would be a good test for BAT.
> >>> A functional test would stack the planes up in some way and
> >>> compare against a software rendered reference. IIRC there was 
> >>> a zpos test case floating around but that depended on alpha
> >>> blending which we don't necessarily have.
> >> But with semi-overlapping planes you would accomplish the same, without alpha dependency.
> >>
> >> Something like this?
> >>
> >> [BG]   [Sprite 1]    [Cursor]
> >>   [Primary]   [Sprite 2]
> > Should probably be good enough. Though I was pondering is there a
> > way to position an arbitraty number of planes such that the
> > resulting picture has a visible region for every possible
> > combination of planes?
> 
> n planes, width = width / (n + 1)
> 
> position = n * 3/4 * plane_width ? or something
> 
> If each plane has its own color, then it would work..

That's not going to hit all the combinations.

Three is easy, you just position the planes in a triangular sort
of shape. But four already seems hard. Or maybe I'm just not smart
enough. Probably needs some graph theory math proof or something.

I suppose you could do thatr staircase type approach you suggested,
and them swap the planes around a bit. I think that should cover
everything eventually. But I was hoping for a cool way to check
everything from a single frame :)

> 
> >> Perhaps primary fullscreen to prevent issues with hw that doesn't support partial planes?
> > I guess. And maybe a second test that disables the primary
> > so that we can also get the bg color into the picture?
> 
> I don't think we finalized the bg color api yet, else it would be good to have..
On Tuesday, April 16, 2019 10:04 PM, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Tue, Apr 16, 2019 at 08:13:12PM +0200, Maarten Lankhorst wrote:
>
> > Op 16-04-2019 om 15:42 schreef Ville Syrjälä:
> >
> > > On Tue, Apr 16, 2019 at 03:28:15PM +0200, Maarten Lankhorst wrote:
> > >
> > > > Op 16-04-2019 om 15:20 schreef Ville Syrjälä:
> > > >
> > > > > On Sat, Apr 13, 2019 at 11:13:27AM +0000, Simon Ser wrote:
> > > > >
> > > > > > From: Ville Syrjälä ville.syrjala@linux.intel.com
> > > > > > This adds basic immutable support for the zpos property. The zpos increases
> > > > > > from bottom to top: primary, sprites, cursor.
> > > > > > I was thinking a bit about how we might go about testing this.
> > > > >
> > > > > We probably want a basic test that just checks that if any
> > > > > plane has a zpos prop then all planes should have it.
> > > > > This would be a good test for BAT.
> > > > > A functional test would stack the planes up in some way and
> > > > > compare against a software rendered reference. IIRC there was
> > > > > a zpos test case floating around but that depended on alpha
> > > > > blending which we don't necessarily have.
> > > > > But with semi-overlapping planes you would accomplish the same, without alpha dependency.
> > > >
> > > > Something like this?
> > > > [BG] [Sprite 1] [Cursor]
> > > > [Primary] [Sprite 2]
> > > > Should probably be good enough. Though I was pondering is there a
> > > > way to position an arbitraty number of planes such that the
> > > > resulting picture has a visible region for every possible
> > > > combination of planes?
> >
> > n planes, width = width / (n + 1)
> > position = n * 3/4 * plane_width ? or something
> > If each plane has its own color, then it would work..
>
> That's not going to hit all the combinations.
>
> Three is easy, you just position the planes in a triangular sort
> of shape. But four already seems hard. Or maybe I'm just not smart
> enough. Probably needs some graph theory math proof or something.
>
> I suppose you could do thatr staircase type approach you suggested,
> and them swap the planes around a bit. I think that should cover
> everything eventually. But I was hoping for a cool way to check
> everything from a single frame :)

That's an interesting problem.

Goal: an image which contains, for each pair of planes (P1, P2), a
region with these two planes overlapping (and only these two planes).

In terms of graphs, if a plane is a node and a two-plane overlap is an
edge, it means we want a complete graph (each node has an edge to all
other nodes). If we only have square planes, it's already impossible to
get a solution for 4 planes (the graph contains an edge that crosses
another edge [1]).

However if we get non-square planes (ie. we have an alpha channel) it
becomes possible. If n is the number of planes, I can imagine to draw
(n - 1) little squares on each plane (the rest being totally
transparent), each of them intersecting with one of the others planes'
squares. This would mean we'd have n * (n - 1) little squares total,
and n * (n - 1) / 2 couple of squares that intersect (this matches the
number of edges of a complete graph).

Thoughts? Is this clear enough? Is it acceptable to require an alpha
channel? Am I mistaken?

[1]: https://en.wikipedia.org/wiki/File:3-simplex_graph.svg
On Wednesday, April 17, 2019 11:35 PM, Simon Ser <contact@emersion.fr> wrote:
> In terms of graphs, if a plane is a node and a two-plane overlap is an
> edge, it means we want a complete graph (each node has an edge to all
> other nodes). If we only have square planes, it's already impossible to
> get a solution for 4 planes (the graph contains an edge that crosses
> another edge [1]).

The following is only true if all planes have an identical size (the
transformation from planes to the graph is lossy otherwise). It's
possible to choose different plane sizes and still be able to find a
solution without an alpha channel for 4 planes [1]. Thanks Léo Andrès
for pointing this out!

Will investigate a little bit more, to see if I can find a general
formula for n planes.

[1]: https://www.zapashcanon.fr/~leo/lohi2.png
On Thursday, April 18, 2019 8:20 AM, Simon Ser <contact@emersion.fr> wrote:
> On Wednesday, April 17, 2019 11:35 PM, Simon Ser contact@emersion.fr wrote:
>
> > In terms of graphs, if a plane is a node and a two-plane overlap is an
> > edge, it means we want a complete graph (each node has an edge to all
> > other nodes). If we only have square planes, it's already impossible to
> > get a solution for 4 planes (the graph contains an edge that crosses
> > another edge [1]).
>
> The following is only true if all planes have an identical size (the
> transformation from planes to the graph is lossy otherwise). It's
> possible to choose different plane sizes and still be able to find a
> solution without an alpha channel for 4 planes [1]. Thanks Léo Andrès
> for pointing this out!
>
> Will investigate a little bit more, to see if I can find a general
> formula for n planes.
>
> [1]: https://www.zapashcanon.fr/~leo/lohi2.png

Discussed with Ville, and this solution won't work as older hardware
doesn't have an alpha channel for overlay planes.

Discussed with Martin, and the most reasonable solution seems to keep
it simple and draw as many frames as there are planes. Frame i would
show plane i as fullscreen and all planes but plane i as small
squares on top or under it. This simple approach makes it easier to
debug and still uses just a few frames.