Re: [PATCH 1/4] drm: add generic zpos property

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Benjamin,

Thank you for the patch.

I would have started working on this next week, thanks for beating me to it 
:-)

On Wednesday 11 May 2016 12:25:05 Benjamin Gaignard wrote:
> This patch adds support for generic plane's zpos property property with
> well-defined semantics:
> - added zpos properties to plane and plane state structures
> - added helpers for normalizing zpos properties of given set of planes
> - well defined semantics: planes are sorted by zpos values and then plane
>   id value if zpos equals
> 
> Normalized zpos values are calculated automatically when generic
> muttable zpos property has been initialized. Drivers can simply use
> plane_state->normalized_zpos in their atomic_check and/or plane_update
> callbacks without any additional calls to DRM core.
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski@xxxxxxxxxxx>
> 
> Compare to Marek's original patch zpos property is now specific to each
> plane and no more to the core.
> Normalize function take care of the range of per plane defined range
> before set normalized_zpos.
> 
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@xxxxxxxxxx>
> 
> Cc: Inki Dae <inki.dae@xxxxxxxxxxx>
> Cc: Daniel Vetter <daniel@xxxxxxxx>
> Cc: Ville Syrjala <ville.syrjala@xxxxxxxxxxxxxxx>
> Cc: Joonyoung Shim <jy0922.shim@xxxxxxxxxxx>
> Cc: Seung-Woo Kim <sw0312.kim@xxxxxxxxxxx>
> Cc: Andrzej Hajda <a.hajda@xxxxxxxxxxx>
> Cc: Krzysztof Kozlowski <k.kozlowski@xxxxxxxxxxx>
> Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@xxxxxxxxxxx>
> Cc: Tobias Jakobi <tjakobi@xxxxxxxxxxxxxxxxxxxxx>
> Cc: Gustavo Padovan <gustavo@xxxxxxxxxxx>
> Cc: vincent.abriou@xxxxxx
> Cc: fabien.dessenne@xxxxxx
> Cc: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> 
> ---
>  Documentation/DocBook/gpu.tmpl      |  10 ++
>  drivers/gpu/drm/Makefile            |   2 +-
>  drivers/gpu/drm/drm_atomic_helper.c |   6 +
>  drivers/gpu/drm/drm_blend.c         | 284 +++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/drm_crtc_internal.h |   3 +
>  include/drm/drm_crtc.h              |  25 ++++
>  6 files changed, 329 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/gpu/drm/drm_blend.c
> 
> diff --git a/Documentation/DocBook/gpu.tmpl b/Documentation/DocBook/gpu.tmpl
> index 9dd48f7..0df6abb 100644
> --- a/Documentation/DocBook/gpu.tmpl
> +++ b/Documentation/DocBook/gpu.tmpl
> @@ -2151,6 +2151,16 @@ void intel_crt_init(struct drm_device *dev)
>  		the underlying hardware).</td>
>  	</tr>
>  	<tr>
> +	<td valign="top" > "zpos" </td>
> +	<td valign="top" >RANGE</td>
> +	<td valign="top" >Min= driver dependent, Max= driver dependent</td>
> +	<td valign="top" >Plane</td>
> +	<td valign="top" >Plane's 'z' position during blending operation (0 for
> background, highest for frontmost).
> +		If two planes assigned to same CRTC have equal zpos values, the plane
> with higher plane
> +		id is treated as closer to front. Can be IMMUTABLE if driver doesn't
> support changing
> +		planes' order. Exact value range is driver dependent.</td>
> +	</tr>
> +	<tr>
>  	<td rowspan="20" valign="top" >i915</td>
>  	<td rowspan="2" valign="top" >Generic</td>
>  	<td valign="top" >"Broadcast RGB"</td>
> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> index 2bd3e5a..df88253 100644
> --- a/drivers/gpu/drm/Makefile
> +++ b/drivers/gpu/drm/Makefile
> @@ -2,7 +2,7 @@
>  # Makefile for the drm device driver.  This driver provides support for the
> # Direct Rendering Infrastructure (DRI) in XFree86 4.1.0 and higher.
> 
> -drm-y       :=	drm_auth.o drm_bufs.o drm_cache.o \
> +drm-y       :=	drm_auth.o drm_bufs.o drm_blend.o drm_cache.o \
>  		drm_context.o drm_dma.o \
>  		drm_fops.o drm_gem.o drm_ioctl.o drm_irq.o \
>  		drm_lock.o drm_memory.o drm_drv.o drm_vm.o \
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> b/drivers/gpu/drm/drm_atomic_helper.c index 997fd21..f10652f 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -32,6 +32,8 @@
>  #include <drm/drm_atomic_helper.h>
>  #include <linux/fence.h>
> 
> +#include "drm_crtc_internal.h"

drm_atomic_helper_normalize_zpos() is declared in <drm/drm_crtc.h>, why do you 
need to include drm_crtc_internal.h ?

>  /**
>   * DOC: overview
>   *
> @@ -587,6 +589,10 @@ drm_atomic_helper_check_planes(struct drm_device *dev,
>  	struct drm_plane_state *plane_state;
>  	int i, ret = 0;
> 
> +	ret = drm_atomic_helper_normalize_zpos(dev, state);
> +	if (ret)
> +		return ret;
> +
>  	for_each_plane_in_state(state, plane, plane_state, i) {
>  		const struct drm_plane_helper_funcs *funcs;
> 
> diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c
> new file mode 100644
> index 0000000..7017715
> --- /dev/null
> +++ b/drivers/gpu/drm/drm_blend.c
> @@ -0,0 +1,284 @@

[snip]

> +#include <linux/slab.h>
> +#include <linux/sort.h>
> +#include <linux/export.h>
> +#include <drm/drmP.h>
> +#include <drm/drm_crtc.h>
> +#include <drm/drm_atomic.h>

Nitpicking, could you please keep header files alphabetically sorted ? It 
helps locating and avoiding duplicates.

> +#include "drm_internal.h"
> +
> +#define MAX(a, b) (((a) > (b)) ? (a) : (b))

linux/kernel.h has max and max_t, is there really a need for a new macro ?

> +/**
> + * drm_plane_atomic_set_zpos_property - set plane zpos property
> + * @plane: drm plane
> + * @state: drm plane state
> + * @property: must be the zpos property associated to the plane
> + * @val: zpos value to be set
> + *
> + * Returns:
> + * Zero on success, negative errno on failure.
> + */
> +int drm_plane_atomic_set_zpos_property(struct drm_plane *plane,
> +				       struct drm_plane_state *state,
> +				       struct drm_property *property,
> +				       uint64_t val)
> +{
> +	struct drm_mode_object *obj = &plane->base;
> +	int i;
> +
> +	for (i = 0; i < obj->properties->count; i++) {
> +		if (obj->properties->properties[i] == property) {
> +			state->zpos = val;
> +			return 0;
> +		}
> +	}
> +
> +	return -EINVAL;
> +}
> +EXPORT_SYMBOL(drm_plane_atomic_set_zpos_property);

Why do you need this function instead of adding zpos support to 
drm_atomic_plane_set_property() ? Same question for the get handler.

> +/**
> + * drm_plane_atomic_get_zpos_property - set plane zpos property
> + * @plane: drm plane
> + * @state: drm plane state
> + * @property: must be the zpos property associated to the plane
> + * @val: zpos value to be retrieve
> + *
> + * Returns:
> + * Zero on success, negative errno on failure.
> + */
> +int drm_plane_atomic_get_zpos_property(struct drm_plane *plane,
> +				       const struct drm_plane_state *state,
> +				       struct drm_property *property,
> +				       uint64_t *val)
> +{
> +	struct drm_mode_object *obj = &plane->base;
> +	int i;
> +
> +	for (i = 0; i < obj->properties->count; i++) {
> +		if (obj->properties->properties[i] == property) {
> +			*val = state->zpos;
> +			return 0;
> +		}
> +	}
> +
> +	return -EINVAL;
> +}
> +EXPORT_SYMBOL(drm_plane_atomic_get_zpos_property);
> +
> +/**
> + * drm_plane_create_zpos_property - create mutable zpos property
> + * @plane: drm plane
> + * @min: minimal possible value of zpos property
> + * @max: maximal possible value of zpos property
> + *
> + * This function initializes generic mutable zpos property and enables
> support
> + * for it in drm core. Drivers can then attach this property to planes to
> enable
> + * support for configurable planes arrangement during blending operation.
> + * Once mutable zpos property has been enabled, the DRM core will
> automatically
> + * calculate drm_plane_state->normalized_zpos values. Usually min 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
> + * information, see drm_mode_create_zpos_immutable_property() function). In
> such
> + * case driver should also assign proper initial zpos values for all planes
> in
> + * its plane_reset() callback, so the planes will be always sorted
> properly.
> + *
> + * Returns:
> + * Zero on success, negative errno on failure.
> + */
> +int drm_plane_create_zpos_property(struct drm_plane *plane,
> +				   unsigned int min, unsigned int max)
> +{
> +	struct drm_property *prop;
> +
> +	prop = drm_property_create_range(plane->dev, 0, "zpos", min, max);
> +	if (!prop)
> +		return -ENOMEM;
> +

You should cache prop (in drm_mode_config) instead of recreating it for each 
plane, like done for the other properties. Same in the next function.

> +	drm_object_attach_property(&plane->base, prop, min);
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_plane_create_zpos_property);
> +
> +/**
> + * drm_plane_create_zpos_immutable_property - create immuttable zpos
> property
> + * @plane: drm plane
> + * @min: minimal possible value of zpos property
> + * @max: maximal possible value of zpos property
> + *
> + * This function initializes generic immutable zpos property and enables
> + * support for it in drm core. Using this property driver lets userspace
> + * to get the arrangement of the planes for blending operation and notifies
> + * it that the hardware (or driver) doesn't support changing of the
> planes'
> + * order.
> + *
> + * Returns:
> + * Zero on success, negative errno on failure.
> + */
> +int drm_plane_create_zpos_immutable_property(struct drm_plane *plane,
> +					     unsigned int min, unsigned int max)
> +{
> +	struct drm_property *prop;
> +
> +	prop = drm_property_create_range(plane->dev, DRM_MODE_PROP_IMMUTABLE,
> +					 "zpos", min, max);

Can two properties exist with the same name but different flags (immutable and 
mutable) ? Or with different min/max values ? I don't expect min/max to be 
different for every plane, so maybe a function that creates the zpos property 
once with min/max values and a second one that attaches the property to planes 
would be a better API.

> +	if (!prop)
> +		return -ENOMEM;
> +
> +	drm_object_attach_property(&plane->base, prop, min);
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_plane_create_zpos_immutable_property);
> +
> +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;
> +	int zpos_a = (sa->zpos << 16) + sa->plane->base.id;
> +	int zpos_b = (sb->zpos << 16) + sb->plane->base.id;
> +
> +	return zpos_a - zpos_b;

Instead of playing tricks with the ids that will create a dependency on the 
plane id value range, how about

	if (sa->zpos != sb->zpos)
		return sa->zpos - sb->zpos;
	else
		return sa->plane->base.id - sb->plane->base.id;

> +}
> +
> +/**
> + * drm_atomic_helper_crtc_normalize_zpos - calculate normalized zpos values
> + * @crtc: crtc with planes, which have to be considered for normalization
> + * @crtc_state: new atomic state to apply
> + *
> + * This function checks new states of all planes assigned to given crtc and
> + * calculates normalized zpos value for them. Planes are compared first by
> their
> + * zpos values, then by plane id (if zpos equals). Plane with lowest zpos
> value
> + * is at the bottom. The plane_state->normalized_zpos is then filled with
> unique
> + * values from 0 to number of active planes in crtc minus one.
> + *
> + * RETURNS
> + * Zero for success or -errno
> + */
> +int drm_atomic_helper_crtc_normalize_zpos(struct drm_crtc *crtc,
> +					  struct drm_crtc_state *crtc_state)
> +{
> +	struct drm_atomic_state *state = crtc_state->state;
> +	struct drm_device *dev = crtc->dev;
> +	int total_planes = dev->mode_config.num_total_plane;
> +	struct drm_plane_state **states;
> +	struct drm_plane *plane;
> +	int i, zpos, n = 0;
> +	int ret = 0;
> +
> +	DRM_DEBUG_ATOMIC("[CRTC:%d:%s] calculating normalized zpos values\n",
> +			 crtc->base.id, crtc->name);
> +
> +	states = kmalloc(total_planes * sizeof(*states), GFP_KERNEL);
> +	if (!states)
> +		return -ENOMEM;
> +
> +	/*
> +	 * Normalization process might create new states for planes which
> +	 * normalized_zpos has to be recalculated.
> +	 */
> +	drm_for_each_plane_mask(plane, dev, crtc_state->plane_mask) {
> +		struct drm_plane_state *plane_state =
> +			drm_atomic_get_plane_state(state, plane);
> +		if (IS_ERR(plane_state)) {
> +			ret = PTR_ERR(plane_state);
> +			goto fail;
> +		}
> +		states[n++] = plane_state;
> +		DRM_DEBUG_ATOMIC("[PLANE:%d:%s] processing zpos value %d\n",
> +				 plane->base.id, plane->name,
> +				 plane_state->zpos);
> +	}
> +
> +	sort(states, n, sizeof(*states), drm_atomic_state_zpos_cmp, NULL);
> +
> +	for (i = 0, zpos = 0; i < n; i++, zpos++) {
> +		plane = states[i]->plane;
> +
> +		zpos = MAX(zpos, states[i]->zpos);
> +
> +		states[i]->normalized_zpos = zpos;

Can't you just use i instead of MAX(zpos, states[i]->zpos) ?

> +		DRM_DEBUG_ATOMIC("[PLANE:%d:%s] normalized zpos value %d\n",
> +				 plane->base.id, plane->name, zpos);
> +	}
> +	crtc_state->zpos_changed = true;
> +
> +fail:

We reach the label in the normal code path too, I would thus call it done or 
exit instead of fail.

> +	kfree(states);
> +	return ret;
> +}
> +EXPORT_SYMBOL(drm_atomic_helper_crtc_normalize_zpos);
> +
> +/**
> + * drm_atomic_helper_normalize_zpos - calculate normalized zpos values for
> all
> + *				      crtcs
> + * @dev: DRM device
> + * @state: atomic state of DRM device
> + *
> + * This function calculates normalized zpos value for all modified planes
> in
> + * the provided atomic state of DRM device. For more information, see
> + * drm_atomic_helper_crtc_normalize_zpos() function.
> + *
> + * RETURNS
> + * Zero for success or -errno
> + */
> +int drm_atomic_helper_normalize_zpos(struct drm_device *dev,
> +				     struct drm_atomic_state *state)
> +{
> +	struct drm_crtc *crtc;
> +	struct drm_crtc_state *crtc_state;
> +	struct drm_plane *plane;
> +	struct drm_plane_state *plane_state;
> +	int i, ret = 0;
> +
> +	for_each_plane_in_state(state, plane, plane_state, i) {
> +		crtc = plane_state->crtc;
> +		if (!crtc)
> +			continue;
> +		if (plane->state->zpos != plane_state->zpos) {
> +			crtc_state =
> +				drm_atomic_get_existing_crtc_state(state, crtc);
> +			crtc_state->zpos_changed = true;
> +		}
> +	}
> +
> +	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> +		if (crtc_state->plane_mask != crtc->state->plane_mask ||
> +		    crtc_state->zpos_changed) {
> +			ret = drm_atomic_helper_crtc_normalize_zpos(crtc,
> +								    crtc_state);
> +			if (ret)
> +				return ret;
> +		}
> +	}
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_atomic_helper_normalize_zpos);
> diff --git a/drivers/gpu/drm/drm_crtc_internal.h
> b/drivers/gpu/drm/drm_crtc_internal.h index a78c138..37d9068 100644
> --- a/drivers/gpu/drm/drm_crtc_internal.h
> +++ b/drivers/gpu/drm/drm_crtc_internal.h
> @@ -42,3 +42,6 @@ int drm_atomic_get_property(struct drm_mode_object *obj,
>  int drm_mode_atomic_ioctl(struct drm_device *dev,
>  			  void *data, struct drm_file *file_priv);
> 
> +/* drm_blend.c */
> +int drm_atomic_helper_normalize_zpos(struct drm_device *dev,
> +				     struct drm_atomic_state *state);
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index d1559cd..a34bc29 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -305,6 +305,7 @@ struct drm_plane_helper_funcs;
>   * @mode_changed: crtc_state->mode or crtc_state->enable has been changed
>   * @active_changed: crtc_state->active has been toggled.
>   * @connectors_changed: connectors to this crtc have been updated
> + * @zpos_changed: zpos values of planes on this crtc have been updated
>   * @color_mgmt_changed: color management properties have changed (degamma
> or *	gamma LUT or CSC matrix)
>   * @plane_mask: bitmask of (1 << drm_plane_index(plane)) of attached planes
> @@ -340,6 +341,7 @@ struct drm_crtc_state {
>  	bool mode_changed : 1;
>  	bool active_changed : 1;
>  	bool connectors_changed : 1;
> +	bool zpos_changed : 1;
>  	bool color_mgmt_changed : 1;
> 
>  	/* attached planes bitmask:
> @@ -1263,6 +1265,9 @@ struct drm_connector {
>   *	plane (in 16.16)
>   * @src_w: width of visible portion of plane (in 16.16)
>   * @src_h: height of visible portion of plane (in 16.16)
> + * @zpos: priority of the given plane on crtc (optional)
> + * @normalized_zpos: normalized value of zpos: unique, range from 0 to N
> + *	 for given crtc

If N is the number of active planes that would be 0 to N-1. If N is the number 
of active planes minus one then this is correct :-)

>   * @state: backpointer to global drm_atomic_state
>   */
>  struct drm_plane_state {
> @@ -1283,6 +1288,10 @@ struct drm_plane_state {
>  	/* Plane rotation */
>  	unsigned int rotation;
> 
> +	/* Plane zpos */
> +	unsigned int zpos;
> +	unsigned int normalized_zpos;
> +
>  	struct drm_atomic_state *state;
>  };
> 
> @@ -2554,6 +2563,22 @@ extern struct drm_property
> *drm_mode_create_rotation_property(struct drm_device extern unsigned int
> drm_rotation_simplify(unsigned int rotation,
>  					  unsigned int supported_rotations);
> 
> +extern int drm_plane_atomic_set_zpos_property(struct drm_plane *plane,
> +					      struct drm_plane_state *state,
> +					      struct drm_property *property,
> +					      uint64_t val);
> +

You can drop the extern keywords, they're not needed. I know that lots of 
functions in this header file use extern, but not all of them do, and moving 
forward I don't see a big reason to keep using unneeded keywords.

> +extern int drm_plane_atomic_get_zpos_property(struct drm_plane *plane,
> +					      const struct drm_plane_state *state,
> +					      struct drm_property *property,
> +					      uint64_t *val);
> +
> +extern int drm_plane_create_zpos_property(struct drm_plane *plane,
> +					  unsigned int min, unsigned int max);

Shouldn't this be named drm_plane_attach_zpos_property() instead ? The main 
purpose of the function is to attach the property, even if it has to create it 
on first invocation as a side effect.

> +extern int drm_plane_create_zpos_immutable_property(struct drm_plane
> *plane,
> +						    unsigned int min,
> +						    unsigned int max);
> + 
>  /* Helpers */
> 
>  static inline struct drm_plane *drm_plane_find(struct drm_device *dev,

-- 
Regards,

Laurent Pinchart

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




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux