Skip to contentSkip to conversation

drm/atomic-helper: Calculate normalized zpos values

PATCH NAK Thierry Reding 3 participants 5 messages Nov 13, 2017 – Nov 20, 2017

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 drm/atomic-helper: Calculate normalized zpos values

From: Thierry Reding <[email protected]>

kerneldoc for drm_plane_create_zpos_property() says that the DRM core will automatically calculate the normalized zpos values, but it won't currently do so. Modify the atomic helpers to behave as documented.

---
 drivers/gpu/drm/drm_atomic_helper.c | 4 ++++
 1 file changed, 4 insertions(+)
drivers/gpu/drm/drm_atomic_helper.c +4 -0
index 71d712f1b56a..6c3ceb3a5dc0 100644
@@ -791,6 +791,10 @@ int drm_atomic_helper_check(struct drm_device *dev,
 	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;
Signature · 6 lines
2.14.1

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
#1 lore raw
Quoted 5 lines · Thierry Reding

From: Thierry Reding <[email protected]>

kerneldoc for drm_plane_create_zpos_property() says that the DRM core will automatically calculate the normalized zpos values, but it won't currently do so. Modify the atomic helpers to behave as documented.

NAK

See commit 38d868e41c4b ("drm: Don't force all planes to be added to the state due to zpos")

Quoted 28 lines
---
 drivers/gpu/drm/drm_atomic_helper.c | 4 ++++
 1 file changed, 4 insertions(+)
drivers/gpu/drm/drm_atomic_helper.c +4 -0
index 71d712f1b56a..6c3ceb3a5dc0 100644
@@ -791,6 +791,10 @@ int drm_atomic_helper_check(struct drm_device *dev,
 	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;
Signature · 6 lines
2.14.1

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Signature · 6 lines
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
#2 lore raw
Quoted 11 lines · Ville Syrjälä
Quoted 5 lines · Thierry Reding

From: Thierry Reding <[email protected]>

kerneldoc for drm_plane_create_zpos_property() says that the DRM core will automatically calculate the normalized zpos values, but it won't currently do so. Modify the atomic helpers to behave as documented.

NAK

See commit 38d868e41c4b ("drm: Don't force all planes to be added to the state due to zpos")

Thanks, that's interesting background. It's a little unfortunate that I'll have to go and add a custom ->atomic_check() just because of this.

This is something in general that's been bugging me. How are we supposed to keep all of the custom ->atomic_check() implementations in sync with the atomic helpers? It looks to me like most drivers will at some point copy the atomic helper to their driver and then make driver-specific changes to them. But then when one of the helpers gets updated, the same changes are usually not propagated to the drivers that originally copied from the helpers.

One way I've seen used (and have used myself) occasionally is for the driver-specific implementations to "subclass" the helpers by calling the helper first and then calling the driver-specific bits. That helps a lot but will obviously not work if ordering is important.

Any ideas on how we can improve that? Other than periodically checking the git log for the helpers and updating drivers? I suppose if one were to closely follow the mailing list one might notice early on, and maybe speak up and have the changes applied to the drivers in the same patch as the helpers. But I don't think that's going to work for every driver.

Thierry

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

#3 lore raw
Quoted 8 lines · Thierry Reding
Quoted 7 lines · Ville Syrjälä
Quoted 5 lines · Thierry Reding

From: Thierry Reding <[email protected]>

kerneldoc for drm_plane_create_zpos_property() says that the DRM core will automatically calculate the normalized zpos values, but it won't currently do so. Modify the atomic helpers to behave as documented.

Quoted 7 lines
Quoted 4 lines

NAK

See commit 38d868e41c4b ("drm: Don't force all planes to be added to the state due to zpos")

Thanks, that's interesting background. It's a little unfortunate that I'll have to go and add a custom ->atomic_check() just because of this.

IIRC I did propose having two variants in the helper, but Daniel didn't like that. Of course that might not scale too well if more variants would become necessary, so I do understand the desire to avoid many helper variants.

Quoted 13 lines

This is something in general that's been bugging me. How are we supposed to keep all of the custom ->atomic_check() implementations in sync with the atomic helpers? It looks to me like most drivers will at some point copy the atomic helper to their driver and then make driver-specific changes to them. But then when one of the helpers gets updated, the same changes are usually not propagated to the drivers that originally copied from the helpers.

One way I've seen used (and have used myself) occasionally is for the driver-specific implementations to "subclass" the helpers by calling the helper first and then calling the driver-specific bits. That helps a lot but will obviously not work if ordering is important.

Quoted 5 lines

Any ideas on how we can improve that? Other than periodically checking the git log for the helpers and updating drivers? I suppose if one were to closely follow the mailing list one might notice early on, and maybe speak up and have the changes applied to the drivers in the same patch as the helpers. But I don't think that's going to work for every driver.

Yeah, I guess keeping a close eye on or periodically checking for changes is the best one can do.

Of course people shouldn't be breaking other drivers with their changes to the helper. So if something gets added there that's really needed in the open coded implementations as well, the patch author should at least attempt to make those changes and make sure all relevant driver maintainers are aware part of the process. That won't magically land you new features though, unless the people implementing those are feeling extra generous.

And I guess nothing beats regular and thorough testing if you're concerned about other people breaking your stuff.

Signature · 6 lines
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
#4 lore raw
In reply to Thierry Reding
Quoted 8 lines · Thierry Reding
Quoted 7 lines · Ville Syrjälä
Quoted 5 lines · Thierry Reding

From: Thierry Reding <[email protected]>

kerneldoc for drm_plane_create_zpos_property() says that the DRM core will automatically calculate the normalized zpos values, but it won't currently do so. Modify the atomic helpers to behave as documented.

Quoted 7 lines
Quoted 4 lines

NAK

See commit 38d868e41c4b ("drm: Don't force all planes to be added to the state due to zpos")

Thanks, that's interesting background. It's a little unfortunate that I'll have to go and add a custom ->atomic_check() just because of this.

I'm behind on mails still, but is there a patch to fix the misleading kerneldoc?
-Daniel

Quoted 28 lines

This is something in general that's been bugging me. How are we supposed to keep all of the custom ->atomic_check() implementations in sync with the atomic helpers? It looks to me like most drivers will at some point copy the atomic helper to their driver and then make driver-specific changes to them. But then when one of the helpers gets updated, the same changes are usually not propagated to the drivers that originally copied from the helpers.

One way I've seen used (and have used myself) occasionally is for the driver-specific implementations to "subclass" the helpers by calling the helper first and then calling the driver-specific bits. That helps a lot but will obviously not work if ordering is important.

Any ideas on how we can improve that? Other than periodically checking the git log for the helpers and updating drivers? I suppose if one were to closely follow the mailing list one might notice early on, and maybe speak up and have the changes applied to the drivers in the same patch as the helpers. But I don't think that's going to work for every driver.

Thierry

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Signature · 7 lines
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel