drm/tegra: Add zpos property for cursor planes
PATCH v3 1/1 drm/tegra: Add zpos property for cursor planes
From: Thierry Reding <[email protected]>
As of commit 4dc55525b095 ("drm: plane: Verify that no or all planes have a zpos property") a warning is emitted if there's a mix of planes with and without a zpos property.
On Tegra, cursor planes are always composited on top of all other planes, which is why they never had a zpos property attached to them. However, since the composition order is fixed, this is trivial to remedy by simply attaching an immutable zpos property to them.
v3: do not hardcode zpos for overlay planes used as cursor (Dmitry)
v2: hardcode cursor plane zpos to 255 instead of 0 (Ville)
--- drivers/gpu/drm/tegra/dc.c | 1 + 1 file changed, 1 insertion(+)
drivers/gpu/drm/tegra/dc.c +1 -0
@@ -957,6 +957,7 @@ static struct drm_plane *tegra_dc_cursor_plane_create(struct drm_device *drm, }drm_plane_helper_add (&plane->base, &tegra_cursor_plane_helper_funcs); +drm_plane_create_zpos_immutable_property (&plane->base,255 );return &plane->base; }
Signature · 1 line
2.24.1
16.06.2020 21:14, Thierry Reding пишет:
Quoted 26 lines
From: Thierry Reding <[email protected]>
As of commit
4dc55525b095("drm: plane: Verify that no or all planes have a zpos property") a warning is emitted if there's a mix of planes with and without a zpos property.On Tegra, cursor planes are always composited on top of all other planes, which is why they never had a zpos property attached to them. However, since the composition order is fixed, this is trivial to remedy by simply attaching an immutable zpos property to them.
v3: do not hardcode zpos for overlay planes used as cursor (Dmitry)
v2: hardcode cursor plane zpos to 255 instead of 0 (Ville)Reported-by Jonathan Hunter <[email protected]>Signed-off-by Thierry Reding <[email protected]>--- drivers/gpu/drm/tegra/dc.c | 1 + 1 file changed, 1 insertion(+)drivers/gpu/drm/tegra/dc.c +0
-0
drivers/gpu/drm/tegra/dc.c static struct drm_plane *tegra_dc_cursor_plane_create(struct drm_device *drm, · line 963
Quoted 7 lines
drivers/gpu/drm/tegra/dc.c +1
-0@@ -957,6 +957,7 @@ static struct drm_plane *tegra_dc_cursor_plane_create(struct drm_device *drm, }drm_plane_helper_add (&plane->base, &tegra_cursor_plane_helper_funcs); +drm_plane_create_zpos_immutable_property (&plane->base,255 );return &plane->base; }
Looks nice, thanks! Since you dropped all other zpos changes for other planes and given that the other planes have 255 for the max zpos, what about to set the cursor's zpos to 256?
Quoted 31 lines · Dmitry Osipenko
16.06.2020 21:14, Thierry Reding пишет:
Quoted 30 lines
From: Thierry Reding <[email protected]>
As of commit
4dc55525b095("drm: plane: Verify that no or all planes have a zpos property") a warning is emitted if there's a mix of planes with and without a zpos property.On Tegra, cursor planes are always composited on top of all other planes, which is why they never had a zpos property attached to them. However, since the composition order is fixed, this is trivial to remedy by simply attaching an immutable zpos property to them.
v3: do not hardcode zpos for overlay planes used as cursor (Dmitry)
v2: hardcode cursor plane zpos to 255 instead of 0 (Ville)Reported-by Jonathan Hunter <[email protected]>Signed-off-by Thierry Reding <[email protected]>--- drivers/gpu/drm/tegra/dc.c | 1 + 1 file changed, 1 insertion(+)drivers/gpu/drm/tegra/dc.c +1
-0@@ -957,6 +957,7 @@ static struct drm_plane *tegra_dc_cursor_plane_create(struct drm_device *drm, }drm_plane_helper_add (&plane->base, &tegra_cursor_plane_helper_funcs); +drm_plane_create_zpos_immutable_property (&plane->base,255 );
Quoted 7 lines
drivers/gpu/drm/tegra/dc.c static struct drm_plane *tegra_dc_cursor_plane_create(struct drm_device *drm, · line 963
Quoted 3 lines
drivers/gpu/drm/tegra/dc.c +0
-0return &plane->base; }Looks nice, thanks! Since you dropped all other zpos changes for other planes and given that the other planes have 255 for the max zpos, what about to set the cursor's zpos to 256?
I'd prefer to have all of the planes' zpos within the same range. By default the other planes will be on the very bottom end of that range and the atomic core will normalize the zpos for all planes anyway, so the cursor plane will end up with a very small normalized zpos in the end.
The zpos documentation already mentions that the behaviour is undefined if two planes have the same zpos value, so I think userspace is going to know how to set these anyway.
It might be worth to do a follow-up patch that is smarter about setting the range of valid values. 0-255 is true on later chips where we actually have a proper "layer depth" register field and I wanted this to be uniform across all generations. Other drivers seem to set the upper limit on the zpos range to be equal to the number of planes available, so that there aren't potentially big gaps in the numbering. That said, since the core already normalizes the zpos for us, I don't see a big benefit in explicitly clipping the range.
Thierry
17.06.2020 17:10, Thierry Reding пишет:
Quoted 50 lines · Dmitry Osipenko
Quoted 31 lines
16.06.2020 21:14, Thierry Reding пишет:
Quoted 30 lines
From: Thierry Reding <[email protected]>
As of commit
4dc55525b095("drm: plane: Verify that no or all planes have a zpos property") a warning is emitted if there's a mix of planes with and without a zpos property.On Tegra, cursor planes are always composited on top of all other planes, which is why they never had a zpos property attached to them. However, since the composition order is fixed, this is trivial to remedy by simply attaching an immutable zpos property to them.
v3: do not hardcode zpos for overlay planes used as cursor (Dmitry)
v2: hardcode cursor plane zpos to 255 instead of 0 (Ville)Reported-by Jonathan Hunter <[email protected]>Signed-off-by Thierry Reding <[email protected]>--- drivers/gpu/drm/tegra/dc.c | 1 + 1 file changed, 1 insertion(+)drivers/gpu/drm/tegra/dc.c +1
-0@@ -957,6 +957,7 @@ static struct drm_plane *tegra_dc_cursor_plane_create(struct drm_device *drm, }drm_plane_helper_add (&plane->base, &tegra_cursor_plane_helper_funcs); +drm_plane_create_zpos_immutable_property (&plane->base,255 );Quoted 7 lines
drivers/gpu/drm/tegra/dc.c static struct drm_plane *tegra_dc_cursor_plane_create(struct drm_device *drm, · line 963
Quoted 3 lines
drivers/gpu/drm/tegra/dc.c +0
-0return &plane->base; }Looks nice, thanks! Since you dropped all other zpos changes for other planes and given that the other planes have 255 for the max zpos, what about to set the cursor's zpos to 256?
I'd prefer to have all of the planes' zpos within the same range. By default the other planes will be on the very bottom end of that range and the atomic core will normalize the zpos for all planes anyway, so the cursor plane will end up with a very small normalized zpos in the end.
The zpos documentation already mentions that the behaviour is undefined if two planes have the same zpos value, so I think userspace is going to know how to set these anyway.
Quoted 8 lines
It might be worth to do a follow-up patch that is smarter about setting the range of valid values. 0-255 is true on later chips where we actually have a proper "layer depth" register field and I wanted this to be uniform across all generations. Other drivers seem to set the upper limit on the zpos range to be equal to the number of planes available, so that there aren't potentially big gaps in the numbering. That said, since the core already normalizes the zpos for us, I don't see a big benefit in explicitly clipping the range.
But the cursor plane doesn't use the "layer depth" register, doesn't it? So the zpos over 255 shouldn't matter in this case.
I know that DRM should normalizes the given zpos, but still it makes me a bit uncomfortable knowing that there are the ranges overlap visible to userspace :)
Quoted 60 lines · Dmitry Osipenko
17.06.2020 17:10, Thierry Reding пишет:
Quoted 50 lines · Dmitry Osipenko
Quoted 31 lines
16.06.2020 21:14, Thierry Reding пишет:
Quoted 30 lines
From: Thierry Reding <[email protected]>
As of commit
4dc55525b095("drm: plane: Verify that no or all planes have a zpos property") a warning is emitted if there's a mix of planes with and without a zpos property.On Tegra, cursor planes are always composited on top of all other planes, which is why they never had a zpos property attached to them. However, since the composition order is fixed, this is trivial to remedy by simply attaching an immutable zpos property to them.
v3: do not hardcode zpos for overlay planes used as cursor (Dmitry)
v2: hardcode cursor plane zpos to 255 instead of 0 (Ville)Reported-by Jonathan Hunter <[email protected]>Signed-off-by Thierry Reding <[email protected]>--- drivers/gpu/drm/tegra/dc.c | 1 + 1 file changed, 1 insertion(+)drivers/gpu/drm/tegra/dc.c +1
-0@@ -957,6 +957,7 @@ static struct drm_plane *tegra_dc_cursor_plane_create(struct drm_device *drm, }drm_plane_helper_add (&plane->base, &tegra_cursor_plane_helper_funcs); +drm_plane_create_zpos_immutable_property (&plane->base,255 );Quoted 7 lines
drivers/gpu/drm/tegra/dc.c static struct drm_plane *tegra_dc_cursor_plane_create(struct drm_device *drm, · line 963
Quoted 3 lines
drivers/gpu/drm/tegra/dc.c +0
-0return &plane->base; }Looks nice, thanks! Since you dropped all other zpos changes for other planes and given that the other planes have 255 for the max zpos, what about to set the cursor's zpos to 256?
I'd prefer to have all of the planes' zpos within the same range. By default the other planes will be on the very bottom end of that range and the atomic core will normalize the zpos for all planes anyway, so the cursor plane will end up with a very small normalized zpos in the end.
The zpos documentation already mentions that the behaviour is undefined if two planes have the same zpos value, so I think userspace is going to know how to set these anyway.
Quoted 8 lines
It might be worth to do a follow-up patch that is smarter about setting the range of valid values. 0-255 is true on later chips where we actually have a proper "layer depth" register field and I wanted this to be uniform across all generations. Other drivers seem to set the upper limit on the zpos range to be equal to the number of planes available, so that there aren't potentially big gaps in the numbering. That said, since the core already normalizes the zpos for us, I don't see a big benefit in explicitly clipping the range.
Quoted 6 lines
But the cursor plane doesn't use the "layer depth" register, doesn't it? So the zpos over 255 shouldn't matter in this case.
I know that DRM should normalizes the given zpos, but still it makes me a bit uncomfortable knowing that there are the ranges overlap visible to userspace :)
Userspace has to be able to deal with this anyway because it can't make any assumptions about what hardware supports underneath. A cursor on a different platform may very well be stackable anywhere in the layout so it must ensure that the cursor always has the highest zpos (provided that that's what it wants). Immutable 255 basically just says that the cursor is always going to be at the top. /If/ userspace then decides to set some other plane's zpos = 255, then we're in the "undefined" behaviour case that the documentation mentions, in which case the behaviour on Tegra would still be sane in showing the cursor on top.
So I don't think there's really an issue with the overlap.
Thierry
17.06.2020 19:37, Thierry Reding пишет:
Quoted 78 lines · Dmitry Osipenko
Quoted 60 lines
17.06.2020 17:10, Thierry Reding пишет:
Quoted 50 lines · Dmitry Osipenko
Quoted 31 lines
16.06.2020 21:14, Thierry Reding пишет:
Quoted 30 lines
From: Thierry Reding <[email protected]>
As of commit
4dc55525b095("drm: plane: Verify that no or all planes have a zpos property") a warning is emitted if there's a mix of planes with and without a zpos property.On Tegra, cursor planes are always composited on top of all other planes, which is why they never had a zpos property attached to them. However, since the composition order is fixed, this is trivial to remedy by simply attaching an immutable zpos property to them.
v3: do not hardcode zpos for overlay planes used as cursor (Dmitry)
v2: hardcode cursor plane zpos to 255 instead of 0 (Ville)Reported-by Jonathan Hunter <[email protected]>Signed-off-by Thierry Reding <[email protected]>--- drivers/gpu/drm/tegra/dc.c | 1 + 1 file changed, 1 insertion(+)drivers/gpu/drm/tegra/dc.c +1
-0@@ -957,6 +957,7 @@ static struct drm_plane *tegra_dc_cursor_plane_create(struct drm_device *drm, }drm_plane_helper_add (&plane->base, &tegra_cursor_plane_helper_funcs); +drm_plane_create_zpos_immutable_property (&plane->base,255 );Quoted 7 lines
drivers/gpu/drm/tegra/dc.c static struct drm_plane *tegra_dc_cursor_plane_create(struct drm_device *drm, · line 963
Quoted 3 lines
drivers/gpu/drm/tegra/dc.c +0
-0return &plane->base; }Looks nice, thanks! Since you dropped all other zpos changes for other planes and given that the other planes have 255 for the max zpos, what about to set the cursor's zpos to 256?
I'd prefer to have all of the planes' zpos within the same range. By default the other planes will be on the very bottom end of that range and the atomic core will normalize the zpos for all planes anyway, so the cursor plane will end up with a very small normalized zpos in the end.
The zpos documentation already mentions that the behaviour is undefined if two planes have the same zpos value, so I think userspace is going to know how to set these anyway.
Quoted 8 lines
It might be worth to do a follow-up patch that is smarter about setting the range of valid values. 0-255 is true on later chips where we actually have a proper "layer depth" register field and I wanted this to be uniform across all generations. Other drivers seem to set the upper limit on the zpos range to be equal to the number of planes available, so that there aren't potentially big gaps in the numbering. That said, since the core already normalizes the zpos for us, I don't see a big benefit in explicitly clipping the range.
Quoted 6 lines
But the cursor plane doesn't use the "layer depth" register, doesn't it? So the zpos over 255 shouldn't matter in this case.
I know that DRM should normalizes the given zpos, but still it makes me a bit uncomfortable knowing that there are the ranges overlap visible to userspace :)
Userspace has to be able to deal with this anyway because it can't make any assumptions about what hardware supports underneath. A cursor on a different platform may very well be stackable anywhere in the layout so it must ensure that the cursor always has the highest zpos (provided that that's what it wants). Immutable 255 basically just says that the cursor is always going to be at the top. /If/ userspace then decides to set some other plane's zpos = 255, then we're in the "undefined" behaviour case that the documentation mentions, in which case the behaviour on Tegra would still be sane in showing the cursor on top.
Quoted 1 line
So I don't think there's really an issue with the overlap.
It should work okay, but if cursor had zpos set to 256 then it would lower the chance for userspace to create the undefined behavior situation.