[PATCH 0/2] drm blend/omap: normalized zpos handling

[PATCH 0/2] drm blend/omap: normalized zpos handling

[Peter Ujfalusi]

Hi,

The following two patch will change the omapdrm to use the normalized_zpos when
configuring the zpos/zorder of the planes.

In OMAP it is possible to move planes between crtcs freely and it is possible to
move even the primary plane from one crtc to another, where it should not be
treated as primary anymore (the crtc's primary plane is the primary for the
crtc).

The first patch will adjust the sorting of the planes for the given crtc to make
sure that the crtc's primary plane, if it has zpos=0 is going to be the bottom
plane. If the zpos of the primary plane is changed by user space then we obey
this and don't force the bottom position.

Regards,
Peter
---
Peter Ujfalusi (2):
  drm/blend: Account also the primary plane of the crtc for
    normalized_zpos
  drm/omap: Normalize the zpos and use the normalized_zpos in runtime

 drivers/gpu/drm/drm_blend.c          |  6 +++++-
 drivers/gpu/drm/omapdrm/omap_drv.c   | 26 +++++++++++++++++++++++++-
 drivers/gpu/drm/omapdrm/omap_plane.c |  2 +-
 3 files changed, 31 insertions(+), 3 deletions(-)

-- 
Peter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/2] drm/blend: Account also the primary plane of the crtc for normalized_zpos

[Peter Ujfalusi]

Make sure that the primary plane will get normalized_zpos=0 if it's zpos is
set to 0, avoiding other planes to be placed in the background.

If user space wants to move the primary plane forward, it can set the zpos
of the plane.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 drivers/gpu/drm/drm_blend.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c
index 4c62dff14893..bdc4f714afb8 100644
--- a/drivers/gpu/drm/drm_blend.c
+++ b/drivers/gpu/drm/drm_blend.c
@@ -301,7 +301,11 @@ static int drm_atomic_state_zpos_cmp(const void *a, const void *b)
 	const struct drm_plane_state *sa = *(struct drm_plane_state **)a;
 	const struct drm_plane_state *sb = *(struct drm_plane_state **)b;
 
-	if (sa->zpos != sb->zpos)
+	if (sa->plane == sa->crtc->primary && sa->zpos == 0)
+		return -1;
+	else if (sb->plane == sb->crtc->primary && sb->zpos == 0)
+		return 1;
+	else if (sa->zpos != sb->zpos)
 		return sa->zpos - sb->zpos;
 	else
 		return sa->plane->base.id - sb->plane->base.id;
-- 
Peter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/2] drm/omap: Normalize the zpos and use the normalized_zpos in runtime

[Peter Ujfalusi]

To avoid zpos collision, use the normalized_zpos when configuring the
zorder of the plane.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 drivers/gpu/drm/omapdrm/omap_drv.c   | 26 +++++++++++++++++++++++++-
 drivers/gpu/drm/omapdrm/omap_plane.c |  2 +-
 2 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
index 6bfc2d9ebb46..230df6d8edd1 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.c
+++ b/drivers/gpu/drm/omapdrm/omap_drv.c
@@ -21,6 +21,7 @@
 
 #include <drm/drm_atomic.h>
 #include <drm/drm_atomic_helper.h>
+#include <drm/drm_blend.h>
 #include <drm/drm_crtc_helper.h>
 #include <drm/drm_fb_helper.h>
 
@@ -54,6 +55,29 @@ MODULE_PARM_DESC(displays,
  *                 devices
  */
 
+int omap_atomic_helper_check(struct drm_device *dev,
+			     struct drm_atomic_state *state)
+{
+	int ret;
+
+	ret = drm_atomic_helper_check_modeset(dev, state);
+	if (ret)
+		return ret;
+
+	ret = drm_atomic_normalize_zpos(dev, state);
+	if (ret)
+		return ret;
+
+	ret = drm_atomic_helper_check_planes(dev, state);
+	if (ret)
+		return ret;
+
+	if (state->legacy_cursor_update)
+		state->async_update = !drm_atomic_helper_async_check(dev, state);
+
+	return ret;
+}
+
 static void omap_atomic_wait_for_completion(struct drm_device *dev,
 					    struct drm_atomic_state *old_state)
 {
@@ -133,7 +157,7 @@ static const struct drm_mode_config_helper_funcs omap_mode_config_helper_funcs =
 static const struct drm_mode_config_funcs omap_mode_config_funcs = {
 	.fb_create = omap_framebuffer_create,
 	.output_poll_changed = drm_fb_helper_output_poll_changed,
-	.atomic_check = drm_atomic_helper_check,
+	.atomic_check = omap_atomic_helper_check,
 	.atomic_commit = drm_atomic_helper_commit,
 };
 
diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c b/drivers/gpu/drm/omapdrm/omap_plane.c
index bbbdd560e503..abd78b511e6d 100644
--- a/drivers/gpu/drm/omapdrm/omap_plane.c
+++ b/drivers/gpu/drm/omapdrm/omap_plane.c
@@ -65,7 +65,7 @@ static void omap_plane_atomic_update(struct drm_plane *plane,
 	info.rotation_type = OMAP_DSS_ROT_NONE;
 	info.rotation = DRM_MODE_ROTATE_0;
 	info.global_alpha = 0xff;
-	info.zorder = state->zpos;
+	info.zorder = state->normalized_zpos;
 
 	/* update scanout: */
 	omap_framebuffer_update_scanout(state->fb, state, &info);
-- 
Peter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm/blend: Account also the primary plane of the crtc for normalized_zpos

[Ville Syrjälä]

On Thu, Dec 21, 2017 at 02:11:00PM +0200, Peter Ujfalusi wrote:
> Make sure that the primary plane will get normalized_zpos=0 if it's zpos is
> set to 0, avoiding other planes to be placed in the background.

Can you describe the actual "bad" configuration?

Without knowing the details this looks like it's making things
more complicated for no particularly good reason. If you're worried
about clients that don't set zpos, then I think you should just
assign the default zpos values better and/or allocate the planes
in a better order. But it's definitely possible I'm missing the
reason why you're doing this.

> 
> If user space wants to move the primary plane forward, it can set the zpos
> of the plane.
> 
> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
> ---
>  drivers/gpu/drm/drm_blend.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c
> index 4c62dff14893..bdc4f714afb8 100644
> --- a/drivers/gpu/drm/drm_blend.c
> +++ b/drivers/gpu/drm/drm_blend.c
> @@ -301,7 +301,11 @@ static int drm_atomic_state_zpos_cmp(const void *a, const void *b)
>  	const struct drm_plane_state *sa = *(struct drm_plane_state **)a;
>  	const struct drm_plane_state *sb = *(struct drm_plane_state **)b;
>  
> -	if (sa->zpos != sb->zpos)
> +	if (sa->plane == sa->crtc->primary && sa->zpos == 0)
> +		return -1;
> +	else if (sb->plane == sb->crtc->primary && sb->zpos == 0)
> +		return 1;
> +	else if (sa->zpos != sb->zpos)
>  		return sa->zpos - sb->zpos;
>  	else
>  		return sa->plane->base.id - sb->plane->base.id;
> -- 
> Peter
> 
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] drm/omap: Normalize the zpos and use the normalized_zpos in runtime

[Daniel Vetter]

On Thu, Dec 21, 2017 at 02:11:01PM +0200, Peter Ujfalusi wrote:
> To avoid zpos collision, use the normalized_zpos when configuring the
> zorder of the plane.
> 
> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
> ---
>  drivers/gpu/drm/omapdrm/omap_drv.c   | 26 +++++++++++++++++++++++++-
>  drivers/gpu/drm/omapdrm/omap_plane.c |  2 +-
>  2 files changed, 26 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
> index 6bfc2d9ebb46..230df6d8edd1 100644
> --- a/drivers/gpu/drm/omapdrm/omap_drv.c
> +++ b/drivers/gpu/drm/omapdrm/omap_drv.c
> @@ -21,6 +21,7 @@
>  
>  #include <drm/drm_atomic.h>
>  #include <drm/drm_atomic_helper.h>
> +#include <drm/drm_blend.h>
>  #include <drm/drm_crtc_helper.h>
>  #include <drm/drm_fb_helper.h>
>  
> @@ -54,6 +55,29 @@ MODULE_PARM_DESC(displays,
>   *                 devices
>   */
>  
> +int omap_atomic_helper_check(struct drm_device *dev,
> +			     struct drm_atomic_state *state)
> +{
> +	int ret;
> +
> +	ret = drm_atomic_helper_check_modeset(dev, state);
> +	if (ret)
> +		return ret;
> +
> +	ret = drm_atomic_normalize_zpos(dev, state);
> +	if (ret)
> +		return ret;

Maybe we should call this by default from helpers instead of forcing
everyone to reinvent this particular wheel?

exynos and sti have the exact same code as you do here, ans rcar could
also reuse it. That feels like we should just fix up
drm_atomic_helper_check instead of patching all the drivers. And as long
as you never change zpos it shouldn't ever run.
-Daniel

> +
> +	ret = drm_atomic_helper_check_planes(dev, state);
> +	if (ret)
> +		return ret;
> +
> +	if (state->legacy_cursor_update)
> +		state->async_update = !drm_atomic_helper_async_check(dev, state);
> +
> +	return ret;
> +}
> +
>  static void omap_atomic_wait_for_completion(struct drm_device *dev,
>  					    struct drm_atomic_state *old_state)
>  {
> @@ -133,7 +157,7 @@ static const struct drm_mode_config_helper_funcs omap_mode_config_helper_funcs =
>  static const struct drm_mode_config_funcs omap_mode_config_funcs = {
>  	.fb_create = omap_framebuffer_create,
>  	.output_poll_changed = drm_fb_helper_output_poll_changed,
> -	.atomic_check = drm_atomic_helper_check,
> +	.atomic_check = omap_atomic_helper_check,
>  	.atomic_commit = drm_atomic_helper_commit,
>  };
>  
> diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c b/drivers/gpu/drm/omapdrm/omap_plane.c
> index bbbdd560e503..abd78b511e6d 100644
> --- a/drivers/gpu/drm/omapdrm/omap_plane.c
> +++ b/drivers/gpu/drm/omapdrm/omap_plane.c
> @@ -65,7 +65,7 @@ static void omap_plane_atomic_update(struct drm_plane *plane,
>  	info.rotation_type = OMAP_DSS_ROT_NONE;
>  	info.rotation = DRM_MODE_ROTATE_0;
>  	info.global_alpha = 0xff;
> -	info.zorder = state->zpos;
> +	info.zorder = state->normalized_zpos;
>  
>  	/* update scanout: */
>  	omap_framebuffer_update_scanout(state->fb, state, &info);
> -- 
> Peter
> 
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm/blend: Account also the primary plane of the crtc for normalized_zpos

[Tomi Valkeinen]

On 21/12/17 14:55, Ville Syrjälä wrote:
> On Thu, Dec 21, 2017 at 02:11:00PM +0200, Peter Ujfalusi wrote:
>> Make sure that the primary plane will get normalized_zpos=0 if it's zpos is
>> set to 0, avoiding other planes to be placed in the background.
> 
> Can you describe the actual "bad" configuration?
> 
> Without knowing the details this looks like it's making things
> more complicated for no particularly good reason. If you're worried
> about clients that don't set zpos, then I think you should just
> assign the default zpos values better and/or allocate the planes
> in a better order. But it's definitely possible I'm missing the
> reason why you're doing this.

Let's say we have two displays and two planes. First display will use 
crtc0 and plane0 as primary plane, the second display crtc1, plane1. The 
zpos of primary planes will be set to 0 by default.

The userspace wants to use the second display, with an overlay plane. So 
it takes the plane0 and moves it to crtc1. Now the second display has 
two planes with zpos 0, and normalize_zpos will fix it according to the 
plane id, i.e. the primary plane of the second display will be on top of 
the "overlay" plane.

I don't think other default zpos values and/or allocating planes in 
better order can solve this.

If the userspace is required to understand and set zpos, then this patch 
is not needed. But at least in my test scripts I've hit this a few times =).

  Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. 
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm/blend: Account also the primary plane of the crtc for normalized_zpos

[Ville Syrjälä]

On Thu, Dec 21, 2017 at 03:44:56PM +0200, Tomi Valkeinen wrote:
> On 21/12/17 14:55, Ville Syrjälä wrote:
> > On Thu, Dec 21, 2017 at 02:11:00PM +0200, Peter Ujfalusi wrote:
> >> Make sure that the primary plane will get normalized_zpos=0 if it's zpos is
> >> set to 0, avoiding other planes to be placed in the background.
> > 
> > Can you describe the actual "bad" configuration?
> > 
> > Without knowing the details this looks like it's making things
> > more complicated for no particularly good reason. If you're worried
> > about clients that don't set zpos, then I think you should just
> > assign the default zpos values better and/or allocate the planes
> > in a better order. But it's definitely possible I'm missing the
> > reason why you're doing this.
> 
> Let's say we have two displays and two planes. First display will use 
> crtc0 and plane0 as primary plane, the second display crtc1, plane1. The 
> zpos of primary planes will be set to 0 by default.
> 
> The userspace wants to use the second display, with an overlay plane. So 
> it takes the plane0 and moves it to crtc1. Now the second display has 
> two planes with zpos 0, and normalize_zpos will fix it according to the 
> plane id, i.e. the primary plane of the second display will be on top of 
> the "overlay" plane.
> 
> I don't think other default zpos values and/or allocating planes in 
> better order can solve this.

Hmm. True. OTOH this messes up the simple "plane id is used to resolve
zpos conflicts" logic.

Also since you have multiple primary planes on the same crtc, which
primary plane is the "real primary" for the crtc? How would userspace
even determine that? I guess the rule could be that the primary planes
have to be registered in the same order as the crtcs?

> 
> If the userspace is required to understand and set zpos, then this patch 
> is not needed. But at least in my test scripts I've hit this a few times =).

I think it would be nice if we can just make it a rule that any
userspace that moves planes between crtcs has to know about zpos.
Otherwise it's pretty much pure luck what stacking order you would
get.

And my idea for planes that can move between crtcs is that the default
zpos should be assigned globally, probably matching the plane id
order as well. Ie. no two planes would default to the same zpos
value. And only in case of hardware that has no planes that can
move between crtcs you would allocate the default zpos per-crtc.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm/blend: Account also the primary plane of the crtc for normalized_zpos

[Tomi Valkeinen]

On 21/12/17 16:20, Ville Syrjälä wrote:
> On Thu, Dec 21, 2017 at 03:44:56PM +0200, Tomi Valkeinen wrote:
>> On 21/12/17 14:55, Ville Syrjälä wrote:
>>> On Thu, Dec 21, 2017 at 02:11:00PM +0200, Peter Ujfalusi wrote:
>>>> Make sure that the primary plane will get normalized_zpos=0 if it's zpos is
>>>> set to 0, avoiding other planes to be placed in the background.
>>>
>>> Can you describe the actual "bad" configuration?
>>>
>>> Without knowing the details this looks like it's making things
>>> more complicated for no particularly good reason. If you're worried
>>> about clients that don't set zpos, then I think you should just
>>> assign the default zpos values better and/or allocate the planes
>>> in a better order. But it's definitely possible I'm missing the
>>> reason why you're doing this.
>>
>> Let's say we have two displays and two planes. First display will use
>> crtc0 and plane0 as primary plane, the second display crtc1, plane1. The
>> zpos of primary planes will be set to 0 by default.
>>
>> The userspace wants to use the second display, with an overlay plane. So
>> it takes the plane0 and moves it to crtc1. Now the second display has
>> two planes with zpos 0, and normalize_zpos will fix it according to the
>> plane id, i.e. the primary plane of the second display will be on top of
>> the "overlay" plane.
>>
>> I don't think other default zpos values and/or allocating planes in
>> better order can solve this.
> 
> Hmm. True. OTOH this messes up the simple "plane id is used to resolve
> zpos conflicts" logic.
> 
> Also since you have multiple primary planes on the same crtc, which
> primary plane is the "real primary" for the crtc? How would userspace
> even determine that? I guess the rule could be that the primary planes
> have to be registered in the same order as the crtcs?

Hmm, true.

>> If the userspace is required to understand and set zpos, then this patch
>> is not needed. But at least in my test scripts I've hit this a few times =).
> 
> I think it would be nice if we can just make it a rule that any
> userspace that moves planes between crtcs has to know about zpos.
> Otherwise it's pretty much pure luck what stacking order you would
> get.

Yes, but how does the userspace know when it is moving planes between crtcs?

If the userspace knows this, then it knows which primary plane is for 
which crtc, right?

And if it doesn't know this, then the userspace cannot associate any 
plane to any crtc (except what it configures itself).

So... If using legacy modesetting (and non-universal planes), there's no 
problem, primary planes are fixed and at low zpos. If using atomic 
modesetting and universal planes, there's really no (default) 
association between planes and crtcs, and "primary plane" doesn't have 
much meaning. Is that correct?

If so... Then I guess the atomic modesetting client essentially randomly 
picks which plane it uses as a "root plane" (if it even needs such), and 
which planes as overlay planes? If that's the case, then this patch 
doesn't quite fix the issue...

  Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. 
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm/blend: Account also the primary plane of the crtc for normalized_zpos

[Ville Syrjälä]

On Thu, Dec 21, 2017 at 04:31:47PM +0200, Tomi Valkeinen wrote:
> On 21/12/17 16:20, Ville Syrjälä wrote:
> > On Thu, Dec 21, 2017 at 03:44:56PM +0200, Tomi Valkeinen wrote:
> >> On 21/12/17 14:55, Ville Syrjälä wrote:
> >>> On Thu, Dec 21, 2017 at 02:11:00PM +0200, Peter Ujfalusi wrote:
> >>>> Make sure that the primary plane will get normalized_zpos=0 if it's zpos is
> >>>> set to 0, avoiding other planes to be placed in the background.
> >>>
> >>> Can you describe the actual "bad" configuration?
> >>>
> >>> Without knowing the details this looks like it's making things
> >>> more complicated for no particularly good reason. If you're worried
> >>> about clients that don't set zpos, then I think you should just
> >>> assign the default zpos values better and/or allocate the planes
> >>> in a better order. But it's definitely possible I'm missing the
> >>> reason why you're doing this.
> >>
> >> Let's say we have two displays and two planes. First display will use
> >> crtc0 and plane0 as primary plane, the second display crtc1, plane1. The
> >> zpos of primary planes will be set to 0 by default.
> >>
> >> The userspace wants to use the second display, with an overlay plane. So
> >> it takes the plane0 and moves it to crtc1. Now the second display has
> >> two planes with zpos 0, and normalize_zpos will fix it according to the
> >> plane id, i.e. the primary plane of the second display will be on top of
> >> the "overlay" plane.
> >>
> >> I don't think other default zpos values and/or allocating planes in
> >> better order can solve this.
> > 
> > Hmm. True. OTOH this messes up the simple "plane id is used to resolve
> > zpos conflicts" logic.
> > 
> > Also since you have multiple primary planes on the same crtc, which
> > primary plane is the "real primary" for the crtc? How would userspace
> > even determine that? I guess the rule could be that the primary planes
> > have to be registered in the same order as the crtcs?
> 
> Hmm, true.
> 
> >> If the userspace is required to understand and set zpos, then this patch
> >> is not needed. But at least in my test scripts I've hit this a few times =).
> > 
> > I think it would be nice if we can just make it a rule that any
> > userspace that moves planes between crtcs has to know about zpos.
> > Otherwise it's pretty much pure luck what stacking order you would
> > get.
> 
> Yes, but how does the userspace know when it is moving planes between crtcs?
> 
> If the userspace knows this, then it knows which primary plane is for 
> which crtc, right?
> 
> And if it doesn't know this, then the userspace cannot associate any 
> plane to any crtc (except what it configures itself).
> 
> So... If using legacy modesetting (and non-universal planes), there's no 
> problem, primary planes are fixed and at low zpos. If using atomic 
> modesetting and universal planes, there's really no (default) 
> association between planes and crtcs, and "primary plane" doesn't have 
> much meaning. Is that correct?
> 
> If so... Then I guess the atomic modesetting client essentially randomly 
> picks which plane it uses as a "root plane" (if it even needs such), and 
> which planes as overlay planes? If that's the case, then this patch 
> doesn't quite fix the issue...

I'm not sure anyone has really thought how userspace would/should assign
planes to crtcs. My only idea so far has been the crtc and their
preferred primary planes should be registered in the same order. But
someone should then also implement that same policy in userspace when
it's trying to figure out which plane to use. There are other
heuristics it should probably use, like preferring to pick a primary
plane for any fullscreen surface.

I guess from functional point of view it shouldn't matter which plane
you pick as long as the plane's user visible capabilities are
sufficient. But there might be user invisible power saving features and
whatnot that only work with specific planes. So from that point of view
maybe the order in which the planes get registered is important. Or we
could maybe come up with some kind of plane usage hints in the uapi
which could help userspace decide?

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] drm/omap: Normalize the zpos and use the normalized_zpos in runtime

[Peter Ujfalusi]

On 2017-12-21 15:17, Daniel Vetter wrote:
> On Thu, Dec 21, 2017 at 02:11:01PM +0200, Peter Ujfalusi wrote:
>> To avoid zpos collision, use the normalized_zpos when configuring the
>> zorder of the plane.
>>
>> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
>> ---
>>  drivers/gpu/drm/omapdrm/omap_drv.c   | 26 +++++++++++++++++++++++++-
>>  drivers/gpu/drm/omapdrm/omap_plane.c |  2 +-
>>  2 files changed, 26 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
>> index 6bfc2d9ebb46..230df6d8edd1 100644
>> --- a/drivers/gpu/drm/omapdrm/omap_drv.c
>> +++ b/drivers/gpu/drm/omapdrm/omap_drv.c
>> @@ -21,6 +21,7 @@
>>  
>>  #include <drm/drm_atomic.h>
>>  #include <drm/drm_atomic_helper.h>
>> +#include <drm/drm_blend.h>
>>  #include <drm/drm_crtc_helper.h>
>>  #include <drm/drm_fb_helper.h>
>>  
>> @@ -54,6 +55,29 @@ MODULE_PARM_DESC(displays,
>>   *                 devices
>>   */
>>  
>> +int omap_atomic_helper_check(struct drm_device *dev,
>> +			     struct drm_atomic_state *state)
>> +{
>> +	int ret;
>> +
>> +	ret = drm_atomic_helper_check_modeset(dev, state);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = drm_atomic_normalize_zpos(dev, state);
>> +	if (ret)
>> +		return ret;
> 
> Maybe we should call this by default from helpers instead of forcing
> everyone to reinvent this particular wheel?

> exynos and sti have the exact same code as you do here, ans rcar could
> also reuse it. That feels like we should just fix up
> drm_atomic_helper_check instead of patching all the drivers. And as long
> as you never change zpos it shouldn't ever run.

It used to be done within drm_atomic_helper_check_planes() which is
called from the drm_atomic_helper_check(), but commit
38d868e41c4b9 drm: Don't force all planes to be added to the state due
to zpos

removed it. Drivers need to do this by themselves if they want to use
the normalized_zpos.

> -Daniel
> 
>> +
>> +	ret = drm_atomic_helper_check_planes(dev, state);
>> +	if (ret)
>> +		return ret;
>> +
>> +	if (state->legacy_cursor_update)
>> +		state->async_update = !drm_atomic_helper_async_check(dev, state);
>> +
>> +	return ret;
>> +}
>> +
>>  static void omap_atomic_wait_for_completion(struct drm_device *dev,
>>  					    struct drm_atomic_state *old_state)
>>  {
>> @@ -133,7 +157,7 @@ static const struct drm_mode_config_helper_funcs omap_mode_config_helper_funcs =
>>  static const struct drm_mode_config_funcs omap_mode_config_funcs = {
>>  	.fb_create = omap_framebuffer_create,
>>  	.output_poll_changed = drm_fb_helper_output_poll_changed,
>> -	.atomic_check = drm_atomic_helper_check,
>> +	.atomic_check = omap_atomic_helper_check,
>>  	.atomic_commit = drm_atomic_helper_commit,
>>  };
>>  
>> diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c b/drivers/gpu/drm/omapdrm/omap_plane.c
>> index bbbdd560e503..abd78b511e6d 100644
>> --- a/drivers/gpu/drm/omapdrm/omap_plane.c
>> +++ b/drivers/gpu/drm/omapdrm/omap_plane.c
>> @@ -65,7 +65,7 @@ static void omap_plane_atomic_update(struct drm_plane *plane,
>>  	info.rotation_type = OMAP_DSS_ROT_NONE;
>>  	info.rotation = DRM_MODE_ROTATE_0;
>>  	info.global_alpha = 0xff;
>> -	info.zorder = state->zpos;
>> +	info.zorder = state->normalized_zpos;
>>  
>>  	/* update scanout: */
>>  	omap_framebuffer_update_scanout(state->fb, state, &info);
>> -- 
>> Peter
>>
>> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
>> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
>>
> 

- Péter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm/blend: Account also the primary plane of the crtc for normalized_zpos

[Tomi Valkeinen]

On 21/12/17 17:12, Ville Syrjälä wrote:

>> If the userspace knows this, then it knows which primary plane is for
>> which crtc, right?
>>
>> And if it doesn't know this, then the userspace cannot associate any
>> plane to any crtc (except what it configures itself).
>>
>> So... If using legacy modesetting (and non-universal planes), there's no
>> problem, primary planes are fixed and at low zpos. If using atomic
>> modesetting and universal planes, there's really no (default)
>> association between planes and crtcs, and "primary plane" doesn't have
>> much meaning. Is that correct?
>>
>> If so... Then I guess the atomic modesetting client essentially randomly
>> picks which plane it uses as a "root plane" (if it even needs such), and
>> which planes as overlay planes? If that's the case, then this patch
>> doesn't quite fix the issue...
> 
> I'm not sure anyone has really thought how userspace would/should assign
> planes to crtcs. My only idea so far has been the crtc and their
> preferred primary planes should be registered in the same order. But
> someone should then also implement that same policy in userspace when
> it's trying to figure out which plane to use. There are other
> heuristics it should probably use, like preferring to pick a primary
> plane for any fullscreen surface.
> 
> I guess from functional point of view it shouldn't matter which plane
> you pick as long as the plane's user visible capabilities are
> sufficient. But there might be user invisible power saving features and
> whatnot that only work with specific planes. So from that point of view
> maybe the order in which the planes get registered is important. Or we
> could maybe come up with some kind of plane usage hints in the uapi
> which could help userspace decide?

I was thinking about this, and... maybe there isn't even any problem (at 
least for OMAP). So lets say we set the default plane zpos to the plane 
index, and that the primary planes are reserved first (i.e. have lower 
zpos than overlay planes).

We have three different cases:

Legacy modesetting: No problems, primary plane is always at the bottom. 
If the userspace uses 2+ overlay planes, it can expect the zpos to be 
based on the plane id, or it can set the zpos.

Atomic+Universal, the application uses one primary plane, and zero or 
more overlay planes: No problems here, the situation is the same as for 
legacy.

Atomic+Universal, the application uses more than one primary plane, and 
zero or more overlay planes: in this case the app _has_ to manage zpos, 
because using two primary planes in a single screen, and expecting it to 
work by default, doesn't make sense.

If the above "rules" are valid, then there's no need for this patch.

But one question I don't have a good answer is that why would we want to 
normalize the zpos, instead of returning an error? If the above rules 
are valid, I think returning an error is better than normalizing and 
hiding the bad configuration.

  Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. 
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm/blend: Account also the primary plane of the crtc for normalized_zpos

[Ville Syrjälä]

On Fri, Dec 22, 2017 at 11:16:47AM +0200, Tomi Valkeinen wrote:
> On 21/12/17 17:12, Ville Syrjälä wrote:
> 
> >> If the userspace knows this, then it knows which primary plane is for
> >> which crtc, right?
> >>
> >> And if it doesn't know this, then the userspace cannot associate any
> >> plane to any crtc (except what it configures itself).
> >>
> >> So... If using legacy modesetting (and non-universal planes), there's no
> >> problem, primary planes are fixed and at low zpos. If using atomic
> >> modesetting and universal planes, there's really no (default)
> >> association between planes and crtcs, and "primary plane" doesn't have
> >> much meaning. Is that correct?
> >>
> >> If so... Then I guess the atomic modesetting client essentially randomly
> >> picks which plane it uses as a "root plane" (if it even needs such), and
> >> which planes as overlay planes? If that's the case, then this patch
> >> doesn't quite fix the issue...
> > 
> > I'm not sure anyone has really thought how userspace would/should assign
> > planes to crtcs. My only idea so far has been the crtc and their
> > preferred primary planes should be registered in the same order. But
> > someone should then also implement that same policy in userspace when
> > it's trying to figure out which plane to use. There are other
> > heuristics it should probably use, like preferring to pick a primary
> > plane for any fullscreen surface.
> > 
> > I guess from functional point of view it shouldn't matter which plane
> > you pick as long as the plane's user visible capabilities are
> > sufficient. But there might be user invisible power saving features and
> > whatnot that only work with specific planes. So from that point of view
> > maybe the order in which the planes get registered is important. Or we
> > could maybe come up with some kind of plane usage hints in the uapi
> > which could help userspace decide?
> 
> I was thinking about this, and... maybe there isn't even any problem (at 
> least for OMAP). So lets say we set the default plane zpos to the plane 
> index, and that the primary planes are reserved first (i.e. have lower 
> zpos than overlay planes).
> 
> We have three different cases:
> 
> Legacy modesetting: No problems, primary plane is always at the bottom. 
> If the userspace uses 2+ overlay planes, it can expect the zpos to be 
> based on the plane id, or it can set the zpos.
> 
> Atomic+Universal, the application uses one primary plane, and zero or 
> more overlay planes: No problems here, the situation is the same as for 
> legacy.
> 
> Atomic+Universal, the application uses more than one primary plane, and 
> zero or more overlay planes: in this case the app _has_ to manage zpos, 
> because using two primary planes in a single screen, and expecting it to 
> work by default, doesn't make sense.
> 
> If the above "rules" are valid, then there's no need for this patch.
> 
> But one question I don't have a good answer is that why would we want to 
> normalize the zpos, instead of returning an error? If the above rules 
> are valid, I think returning an error is better than normalizing and 
> hiding the bad configuration.

IIRC I argued against the normalization, but some people really
wanted it for whatever reason.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm/blend: Account also the primary plane of the crtc for normalized_zpos

[Peter Ujfalusi]

Hi,

On 2017-12-22 12:12, Ville Syrjälä wrote:
> On Fri, Dec 22, 2017 at 11:16:47AM +0200, Tomi Valkeinen wrote:
>> On 21/12/17 17:12, Ville Syrjälä wrote:
>>
>>>> If the userspace knows this, then it knows which primary plane is for
>>>> which crtc, right?
>>>>
>>>> And if it doesn't know this, then the userspace cannot associate any
>>>> plane to any crtc (except what it configures itself).
>>>>
>>>> So... If using legacy modesetting (and non-universal planes), there's no
>>>> problem, primary planes are fixed and at low zpos. If using atomic
>>>> modesetting and universal planes, there's really no (default)
>>>> association between planes and crtcs, and "primary plane" doesn't have
>>>> much meaning. Is that correct?
>>>>
>>>> If so... Then I guess the atomic modesetting client essentially randomly
>>>> picks which plane it uses as a "root plane" (if it even needs such), and
>>>> which planes as overlay planes? If that's the case, then this patch
>>>> doesn't quite fix the issue...
>>>
>>> I'm not sure anyone has really thought how userspace would/should assign
>>> planes to crtcs. My only idea so far has been the crtc and their
>>> preferred primary planes should be registered in the same order. But
>>> someone should then also implement that same policy in userspace when
>>> it's trying to figure out which plane to use. There are other
>>> heuristics it should probably use, like preferring to pick a primary
>>> plane for any fullscreen surface.
>>>
>>> I guess from functional point of view it shouldn't matter which plane
>>> you pick as long as the plane's user visible capabilities are
>>> sufficient. But there might be user invisible power saving features and
>>> whatnot that only work with specific planes. So from that point of view
>>> maybe the order in which the planes get registered is important. Or we
>>> could maybe come up with some kind of plane usage hints in the uapi
>>> which could help userspace decide?
>>
>> I was thinking about this, and... maybe there isn't even any problem (at 
>> least for OMAP). So lets say we set the default plane zpos to the plane 
>> index, and that the primary planes are reserved first (i.e. have lower 
>> zpos than overlay planes).
>>
>> We have three different cases:
>>
>> Legacy modesetting: No problems, primary plane is always at the bottom. 
>> If the userspace uses 2+ overlay planes, it can expect the zpos to be 
>> based on the plane id, or it can set the zpos.
>>
>> Atomic+Universal, the application uses one primary plane, and zero or 
>> more overlay planes: No problems here, the situation is the same as for 
>> legacy.
>>
>> Atomic+Universal, the application uses more than one primary plane, and 
>> zero or more overlay planes: in this case the app _has_ to manage zpos, 
>> because using two primary planes in a single screen, and expecting it to 
>> work by default, doesn't make sense.
>>
>> If the above "rules" are valid, then there's no need for this patch.
>>
>> But one question I don't have a good answer is that why would we want to 
>> normalize the zpos, instead of returning an error? If the above rules 
>> are valid, I think returning an error is better than normalizing and 
>> hiding the bad configuration.
> 
> IIRC I argued against the normalization, but some people really
> wanted it for whatever reason.

OK, please ignore this series, I'll send a patch instead next year.

- Péter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] drm/omap: Normalize the zpos and use the normalized_zpos in runtime

[Daniel Vetter]

On Fri, Dec 22, 2017 at 08:38:38AM +0200, Peter Ujfalusi wrote:
> 
> 
> On 2017-12-21 15:17, Daniel Vetter wrote:
> > On Thu, Dec 21, 2017 at 02:11:01PM +0200, Peter Ujfalusi wrote:
> >> To avoid zpos collision, use the normalized_zpos when configuring the
> >> zorder of the plane.
> >>
> >> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
> >> ---
> >>  drivers/gpu/drm/omapdrm/omap_drv.c   | 26 +++++++++++++++++++++++++-
> >>  drivers/gpu/drm/omapdrm/omap_plane.c |  2 +-
> >>  2 files changed, 26 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
> >> index 6bfc2d9ebb46..230df6d8edd1 100644
> >> --- a/drivers/gpu/drm/omapdrm/omap_drv.c
> >> +++ b/drivers/gpu/drm/omapdrm/omap_drv.c
> >> @@ -21,6 +21,7 @@
> >>  
> >>  #include <drm/drm_atomic.h>
> >>  #include <drm/drm_atomic_helper.h>
> >> +#include <drm/drm_blend.h>
> >>  #include <drm/drm_crtc_helper.h>
> >>  #include <drm/drm_fb_helper.h>
> >>  
> >> @@ -54,6 +55,29 @@ MODULE_PARM_DESC(displays,
> >>   *                 devices
> >>   */
> >>  
> >> +int omap_atomic_helper_check(struct drm_device *dev,
> >> +			     struct drm_atomic_state *state)
> >> +{
> >> +	int ret;
> >> +
> >> +	ret = drm_atomic_helper_check_modeset(dev, state);
> >> +	if (ret)
> >> +		return ret;
> >> +
> >> +	ret = drm_atomic_normalize_zpos(dev, state);
> >> +	if (ret)
> >> +		return ret;
> > 
> > Maybe we should call this by default from helpers instead of forcing
> > everyone to reinvent this particular wheel?
> 
> > exynos and sti have the exact same code as you do here, ans rcar could
> > also reuse it. That feels like we should just fix up
> > drm_atomic_helper_check instead of patching all the drivers. And as long
> > as you never change zpos it shouldn't ever run.
> 
> It used to be done within drm_atomic_helper_check_planes() which is
> called from the drm_atomic_helper_check(), but commit
> 38d868e41c4b9 drm: Don't force all planes to be added to the state due
> to zpos
> 
> removed it. Drivers need to do this by themselves if they want to use
> the normalized_zpos.

Oh right, I forgot about all that again.
-Daniel

> 
> > -Daniel
> > 
> >> +
> >> +	ret = drm_atomic_helper_check_planes(dev, state);
> >> +	if (ret)
> >> +		return ret;
> >> +
> >> +	if (state->legacy_cursor_update)
> >> +		state->async_update = !drm_atomic_helper_async_check(dev, state);
> >> +
> >> +	return ret;
> >> +}
> >> +
> >>  static void omap_atomic_wait_for_completion(struct drm_device *dev,
> >>  					    struct drm_atomic_state *old_state)
> >>  {
> >> @@ -133,7 +157,7 @@ static const struct drm_mode_config_helper_funcs omap_mode_config_helper_funcs =
> >>  static const struct drm_mode_config_funcs omap_mode_config_funcs = {
> >>  	.fb_create = omap_framebuffer_create,
> >>  	.output_poll_changed = drm_fb_helper_output_poll_changed,
> >> -	.atomic_check = drm_atomic_helper_check,
> >> +	.atomic_check = omap_atomic_helper_check,
> >>  	.atomic_commit = drm_atomic_helper_commit,
> >>  };
> >>  
> >> diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c b/drivers/gpu/drm/omapdrm/omap_plane.c
> >> index bbbdd560e503..abd78b511e6d 100644
> >> --- a/drivers/gpu/drm/omapdrm/omap_plane.c
> >> +++ b/drivers/gpu/drm/omapdrm/omap_plane.c
> >> @@ -65,7 +65,7 @@ static void omap_plane_atomic_update(struct drm_plane *plane,
> >>  	info.rotation_type = OMAP_DSS_ROT_NONE;
> >>  	info.rotation = DRM_MODE_ROTATE_0;
> >>  	info.global_alpha = 0xff;
> >> -	info.zorder = state->zpos;
> >> +	info.zorder = state->normalized_zpos;
> >>  
> >>  	/* update scanout: */
> >>  	omap_framebuffer_update_scanout(state->fb, state, &info);
> >> -- 
> >> Peter
> >>
> >> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
> >> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
> >>
> > 
> 
> - Péter
> 
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm/blend: Account also the primary plane of the crtc for normalized_zpos

[Daniel Vetter]

On Fri, Dec 22, 2017 at 05:15:05PM +0200, Peter Ujfalusi wrote:
> Hi,
> 
> On 2017-12-22 12:12, Ville Syrjälä wrote:
> > On Fri, Dec 22, 2017 at 11:16:47AM +0200, Tomi Valkeinen wrote:
> >> On 21/12/17 17:12, Ville Syrjälä wrote:
> >>
> >>>> If the userspace knows this, then it knows which primary plane is for
> >>>> which crtc, right?
> >>>>
> >>>> And if it doesn't know this, then the userspace cannot associate any
> >>>> plane to any crtc (except what it configures itself).
> >>>>
> >>>> So... If using legacy modesetting (and non-universal planes), there's no
> >>>> problem, primary planes are fixed and at low zpos. If using atomic
> >>>> modesetting and universal planes, there's really no (default)
> >>>> association between planes and crtcs, and "primary plane" doesn't have
> >>>> much meaning. Is that correct?
> >>>>
> >>>> If so... Then I guess the atomic modesetting client essentially randomly
> >>>> picks which plane it uses as a "root plane" (if it even needs such), and
> >>>> which planes as overlay planes? If that's the case, then this patch
> >>>> doesn't quite fix the issue...
> >>>
> >>> I'm not sure anyone has really thought how userspace would/should assign
> >>> planes to crtcs. My only idea so far has been the crtc and their
> >>> preferred primary planes should be registered in the same order. But
> >>> someone should then also implement that same policy in userspace when
> >>> it's trying to figure out which plane to use. There are other
> >>> heuristics it should probably use, like preferring to pick a primary
> >>> plane for any fullscreen surface.
> >>>
> >>> I guess from functional point of view it shouldn't matter which plane
> >>> you pick as long as the plane's user visible capabilities are
> >>> sufficient. But there might be user invisible power saving features and
> >>> whatnot that only work with specific planes. So from that point of view
> >>> maybe the order in which the planes get registered is important. Or we
> >>> could maybe come up with some kind of plane usage hints in the uapi
> >>> which could help userspace decide?
> >>
> >> I was thinking about this, and... maybe there isn't even any problem (at 
> >> least for OMAP). So lets say we set the default plane zpos to the plane 
> >> index, and that the primary planes are reserved first (i.e. have lower 
> >> zpos than overlay planes).
> >>
> >> We have three different cases:
> >>
> >> Legacy modesetting: No problems, primary plane is always at the bottom. 
> >> If the userspace uses 2+ overlay planes, it can expect the zpos to be 
> >> based on the plane id, or it can set the zpos.
> >>
> >> Atomic+Universal, the application uses one primary plane, and zero or 
> >> more overlay planes: No problems here, the situation is the same as for 
> >> legacy.
> >>
> >> Atomic+Universal, the application uses more than one primary plane, and 
> >> zero or more overlay planes: in this case the app _has_ to manage zpos, 
> >> because using two primary planes in a single screen, and expecting it to 
> >> work by default, doesn't make sense.
> >>
> >> If the above "rules" are valid, then there's no need for this patch.
> >>
> >> But one question I don't have a good answer is that why would we want to 
> >> normalize the zpos, instead of returning an error? If the above rules 
> >> are valid, I think returning an error is better than normalizing and 
> >> hiding the bad configuration.
> > 
> > IIRC I argued against the normalization, but some people really
> > wanted it for whatever reason.
> 
> OK, please ignore this series, I'll send a patch instead next year.

So now we end up with a bunch of kms drivers that normalize zpos, and a
bunch of others which rejects duplicated zpos.

That sounds even worse. Can we pls try to be consistent (even if it turns
out to be a not-so-great uapi decision, it's uapi, so let's not make
things worse by making it inconsistent).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm/blend: Account also the primary plane of the crtc for normalized_zpos

[Laurent Pinchart]

Hi Daniel,

On Tuesday, 9 January 2018 14:42:55 EET Daniel Vetter wrote:
> On Fri, Dec 22, 2017 at 05:15:05PM +0200, Peter Ujfalusi wrote:
> > On 2017-12-22 12:12, Ville Syrjälä wrote:
> > > On Fri, Dec 22, 2017 at 11:16:47AM +0200, Tomi Valkeinen wrote:
> > >> On 21/12/17 17:12, Ville Syrjälä wrote:
> > >>>> If the userspace knows this, then it knows which primary plane is for
> > >>>> which crtc, right?
> > >>>> 
> > >>>> And if it doesn't know this, then the userspace cannot associate any
> > >>>> plane to any crtc (except what it configures itself).
> > >>>> 
> > >>>> So... If using legacy modesetting (and non-universal planes), there's
> > >>>> no
> > >>>> problem, primary planes are fixed and at low zpos. If using atomic
> > >>>> modesetting and universal planes, there's really no (default)
> > >>>> association between planes and crtcs, and "primary plane" doesn't
> > >>>> have
> > >>>> much meaning. Is that correct?
> > >>>> 
> > >>>> If so... Then I guess the atomic modesetting client essentially
> > >>>> randomly
> > >>>> picks which plane it uses as a "root plane" (if it even needs such),
> > >>>> and
> > >>>> which planes as overlay planes? If that's the case, then this patch
> > >>>> doesn't quite fix the issue...
> > >>> 
> > >>> I'm not sure anyone has really thought how userspace would/should
> > >>> assign
> > >>> planes to crtcs. My only idea so far has been the crtc and their
> > >>> preferred primary planes should be registered in the same order. But
> > >>> someone should then also implement that same policy in userspace when
> > >>> it's trying to figure out which plane to use. There are other
> > >>> heuristics it should probably use, like preferring to pick a primary
> > >>> plane for any fullscreen surface.
> > >>> 
> > >>> I guess from functional point of view it shouldn't matter which plane
> > >>> you pick as long as the plane's user visible capabilities are
> > >>> sufficient. But there might be user invisible power saving features
> > >>> and
> > >>> whatnot that only work with specific planes. So from that point of
> > >>> view
> > >>> maybe the order in which the planes get registered is important. Or we
> > >>> could maybe come up with some kind of plane usage hints in the uapi
> > >>> which could help userspace decide?
> > >> 
> > >> I was thinking about this, and... maybe there isn't even any problem
> > >> (at
> > >> least for OMAP). So lets say we set the default plane zpos to the plane
> > >> index, and that the primary planes are reserved first (i.e. have lower
> > >> zpos than overlay planes).
> > >> 
> > >> We have three different cases:
> > >> 
> > >> Legacy modesetting: No problems, primary plane is always at the bottom.
> > >> If the userspace uses 2+ overlay planes, it can expect the zpos to be
> > >> based on the plane id, or it can set the zpos.
> > >> 
> > >> Atomic+Universal, the application uses one primary plane, and zero or
> > >> more overlay planes: No problems here, the situation is the same as for
> > >> legacy.
> > >> 
> > >> Atomic+Universal, the application uses more than one primary plane, and
> > >> zero or more overlay planes: in this case the app _has_ to manage zpos,
> > >> because using two primary planes in a single screen, and expecting it
> > >> to
> > >> work by default, doesn't make sense.
> > >> 
> > >> If the above "rules" are valid, then there's no need for this patch.
> > >> 
> > >> But one question I don't have a good answer is that why would we want
> > >> to
> > >> normalize the zpos, instead of returning an error? If the above rules
> > >> are valid, I think returning an error is better than normalizing and
> > >> hiding the bad configuration.
> > > 
> > > IIRC I argued against the normalization, but some people really
> > > wanted it for whatever reason.
> > 
> > OK, please ignore this series, I'll send a patch instead next year.
> 
> So now we end up with a bunch of kms drivers that normalize zpos, and a
> bunch of others which rejects duplicated zpos.
> 
> That sounds even worse. Can we pls try to be consistent (even if it turns
> out to be a not-so-great uapi decision, it's uapi, so let's not make
> things worse by making it inconsistent).

For what it's worth, I'd tend to disallow duplicate zpos values. That forces 
userspace to user atomic and to handle zpos explicitly, and that's exactly why 
it's my preference as not handling zpos explicitly in userspace will lead to 
random behaviour at best.

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm/blend: Account also the primary plane of the crtc for normalized_zpos

[Daniel Vetter]

On Tue, Jan 09, 2018 at 03:40:36PM +0200, Laurent Pinchart wrote:
> Hi Daniel,
> 
> On Tuesday, 9 January 2018 14:42:55 EET Daniel Vetter wrote:
> > On Fri, Dec 22, 2017 at 05:15:05PM +0200, Peter Ujfalusi wrote:
> > > On 2017-12-22 12:12, Ville Syrjälä wrote:
> > > > On Fri, Dec 22, 2017 at 11:16:47AM +0200, Tomi Valkeinen wrote:
> > > >> On 21/12/17 17:12, Ville Syrjälä wrote:
> > > >>>> If the userspace knows this, then it knows which primary plane is for
> > > >>>> which crtc, right?
> > > >>>> 
> > > >>>> And if it doesn't know this, then the userspace cannot associate any
> > > >>>> plane to any crtc (except what it configures itself).
> > > >>>> 
> > > >>>> So... If using legacy modesetting (and non-universal planes), there's
> > > >>>> no
> > > >>>> problem, primary planes are fixed and at low zpos. If using atomic
> > > >>>> modesetting and universal planes, there's really no (default)
> > > >>>> association between planes and crtcs, and "primary plane" doesn't
> > > >>>> have
> > > >>>> much meaning. Is that correct?
> > > >>>> 
> > > >>>> If so... Then I guess the atomic modesetting client essentially
> > > >>>> randomly
> > > >>>> picks which plane it uses as a "root plane" (if it even needs such),
> > > >>>> and
> > > >>>> which planes as overlay planes? If that's the case, then this patch
> > > >>>> doesn't quite fix the issue...
> > > >>> 
> > > >>> I'm not sure anyone has really thought how userspace would/should
> > > >>> assign
> > > >>> planes to crtcs. My only idea so far has been the crtc and their
> > > >>> preferred primary planes should be registered in the same order. But
> > > >>> someone should then also implement that same policy in userspace when
> > > >>> it's trying to figure out which plane to use. There are other
> > > >>> heuristics it should probably use, like preferring to pick a primary
> > > >>> plane for any fullscreen surface.
> > > >>> 
> > > >>> I guess from functional point of view it shouldn't matter which plane
> > > >>> you pick as long as the plane's user visible capabilities are
> > > >>> sufficient. But there might be user invisible power saving features
> > > >>> and
> > > >>> whatnot that only work with specific planes. So from that point of
> > > >>> view
> > > >>> maybe the order in which the planes get registered is important. Or we
> > > >>> could maybe come up with some kind of plane usage hints in the uapi
> > > >>> which could help userspace decide?
> > > >> 
> > > >> I was thinking about this, and... maybe there isn't even any problem
> > > >> (at
> > > >> least for OMAP). So lets say we set the default plane zpos to the plane
> > > >> index, and that the primary planes are reserved first (i.e. have lower
> > > >> zpos than overlay planes).
> > > >> 
> > > >> We have three different cases:
> > > >> 
> > > >> Legacy modesetting: No problems, primary plane is always at the bottom.
> > > >> If the userspace uses 2+ overlay planes, it can expect the zpos to be
> > > >> based on the plane id, or it can set the zpos.
> > > >> 
> > > >> Atomic+Universal, the application uses one primary plane, and zero or
> > > >> more overlay planes: No problems here, the situation is the same as for
> > > >> legacy.
> > > >> 
> > > >> Atomic+Universal, the application uses more than one primary plane, and
> > > >> zero or more overlay planes: in this case the app _has_ to manage zpos,
> > > >> because using two primary planes in a single screen, and expecting it
> > > >> to
> > > >> work by default, doesn't make sense.
> > > >> 
> > > >> If the above "rules" are valid, then there's no need for this patch.
> > > >> 
> > > >> But one question I don't have a good answer is that why would we want
> > > >> to
> > > >> normalize the zpos, instead of returning an error? If the above rules
> > > >> are valid, I think returning an error is better than normalizing and
> > > >> hiding the bad configuration.
> > > > 
> > > > IIRC I argued against the normalization, but some people really
> > > > wanted it for whatever reason.
> > > 
> > > OK, please ignore this series, I'll send a patch instead next year.
> > 
> > So now we end up with a bunch of kms drivers that normalize zpos, and a
> > bunch of others which rejects duplicated zpos.
> > 
> > That sounds even worse. Can we pls try to be consistent (even if it turns
> > out to be a not-so-great uapi decision, it's uapi, so let's not make
> > things worse by making it inconsistent).
> 
> For what it's worth, I'd tend to disallow duplicate zpos values. That forces 
> userspace to user atomic and to handle zpos explicitly, and that's exactly why 
> it's my preference as not handling zpos explicitly in userspace will lead to 
> random behaviour at best.

I don't care what we're going with tbf, except it should be consistent
across drivers ... Everyone just implementing their flavour of bikeshed in
their driver is kinda uncool.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel