[PATCH] drm/atomic-helper: Calculate normalized zpos values

[PATCH] drm/atomic-helper: Calculate normalized zpos values

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

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.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/gpu/drm/drm_atomic_helper.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 71d712f1b56a..6c3ceb3a5dc0 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -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;
-- 
2.14.1

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

Re: [PATCH] drm/atomic-helper: Calculate normalized zpos values

[Ville Syrjälä]

On Mon, Nov 13, 2017 at 02:48:20PM +0100, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> 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")

> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 71d712f1b56a..6c3ceb3a5dc0 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -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;
> -- 
> 2.14.1
> 
> _______________________________________________
> 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] drm/atomic-helper: Calculate normalized zpos values

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 1782 bytes --]

On Mon, Nov 13, 2017 at 04:14:05PM +0200, Ville Syrjälä wrote:
> On Mon, Nov 13, 2017 at 02:48:20PM +0100, Thierry Reding wrote:
> > From: Thierry Reding <treding@nvidia.com>
> > 
> > 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

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

Re: [PATCH] drm/atomic-helper: Calculate normalized zpos values

[Ville Syrjälä]

On Mon, Nov 13, 2017 at 09:56:19PM +0100, Thierry Reding wrote:
> On Mon, Nov 13, 2017 at 04:14:05PM +0200, Ville Syrjälä wrote:
> > On Mon, Nov 13, 2017 at 02:48:20PM +0100, Thierry Reding wrote:
> > > From: Thierry Reding <treding@nvidia.com>
> > > 
> > > 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.

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.

> 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.

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.

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

Re: [PATCH] drm/atomic-helper: Calculate normalized zpos values

[Daniel Vetter]

On Mon, Nov 13, 2017 at 09:56:19PM +0100, Thierry Reding wrote:
> On Mon, Nov 13, 2017 at 04:14:05PM +0200, Ville Syrjälä wrote:
> > On Mon, Nov 13, 2017 at 02:48:20PM +0100, Thierry Reding wrote:
> > > From: Thierry Reding <treding@nvidia.com>
> > > 
> > > 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.

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

> 
> 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
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel


-- 
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