Re: [PATCH] drm: plane: Verify that no or all planes have a zpos property

[Sam Ravnborg <sam@ravnborg.org>]

Hi Laurent.

On Sat, Apr 04, 2020 at 09:12:53PM +0300, Laurent Pinchart wrote:
> The zpos property is used by userspace to sort the order of planes.
> While the property is not mandatory for drivers to implement, mixing
> planes with and without zpos confuses userspace, and shall not be
> allowed. Clarify this in the documentation and warn at runtime if the
> drivers mixes planes with and without zpos properties.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Looks good.
Acked-by: Sam Ravnborg <sam@ravnborg.org>

> ---
>  drivers/gpu/drm/drm_blend.c | 10 ++++++----
>  drivers/gpu/drm/drm_plane.c |  9 +++++++++
>  2 files changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c
> index 88eedee018d3..f1dcad96f341 100644
> --- a/drivers/gpu/drm/drm_blend.c
> +++ b/drivers/gpu/drm/drm_blend.c
> @@ -135,7 +135,9 @@
>   *	are underneath planes with higher Z position values. Two planes with the
>   *	same Z position value have undefined ordering. Note that the Z position
>   *	value can also be immutable, to inform userspace about the hard-coded
> - *	stacking of planes, see drm_plane_create_zpos_immutable_property().
> + *	stacking of planes, see drm_plane_create_zpos_immutable_property(). If
> + *	any plane has a zpos property (either mutable or immutable), then all
> + *	planes shall have a zpos property.
>   *
>   * pixel blend mode:
>   *	Pixel blend mode is set up with drm_plane_create_blend_mode_property().
> @@ -344,10 +346,10 @@ EXPORT_SYMBOL(drm_rotation_simplify);
>   * should be set to 0 and max to maximal number of planes for given crtc - 1.
>   *
>   * If zpos of some planes cannot be changed (like fixed background or
> - * cursor/topmost planes), driver should adjust min/max values and assign those
> - * planes immutable zpos property with lower or higher values (for more
> + * cursor/topmost planes), drivers shall adjust the min/max values and assign
> + * those planes immutable zpos properties with lower or higher values (for more
>   * information, see drm_plane_create_zpos_immutable_property() function). In such
> - * case driver should also assign proper initial zpos values for all planes in
> + * case drivers shall also assign proper initial zpos values for all planes in
>   * its plane_reset() callback, so the planes will be always sorted properly.
>   *
>   * See also drm_atomic_normalize_zpos().
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index d6ad60ab0d38..efb9c16ddc21 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -289,6 +289,8 @@ EXPORT_SYMBOL(drm_universal_plane_init);
>  
>  int drm_plane_register_all(struct drm_device *dev)
>  {
> +	unsigned int num_planes = 0;
> +	unsigned int num_zpos = 0;
>  	struct drm_plane *plane;
>  	int ret = 0;
>  
> @@ -297,8 +299,15 @@ int drm_plane_register_all(struct drm_device *dev)
>  			ret = plane->funcs->late_register(plane);
>  		if (ret)
>  			return ret;
> +
> +		if (plane->zpos_property)
> +			num_zpos++;
> +		num_planes++;
>  	}
>  
> +	drm_WARN(dev, num_planes != num_zpos,
> +		 "Mixing planes with and without zpos property is invalid\n");
> +
>  	return 0;
>  }
>  
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel