Hi Laurent,
2016-05-11 13:24 GMT+02:00 Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>:
> 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 ?
drm_atomic_helper_normalize_zpos() is declared in drm_crtc_internal.h
not into <drm/drm_crtc.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.
>
sure, I will for v2
>> +#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 ?
ok I will replace by max_t
>
>> +/**
>> + * 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.
>
Be able to have a per plane zpos property was a Ville request because
all the planes may not have the same min/max zpos.
The consequences of this design are that we can cache the property in
drm_mode_config
and we need to have helpers functions
drm_plane_atomic_{set,get}_zpos_property to be able find
the property attached to the drm_plane.
>> + 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;
>
It is how Marek have code it but you solution is more readable.
>> +}
>> +
>> +/**
>> + * 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) ?
No because since each plane could have different min/max you could
have gaps between two plane.
For example if planeX range is [0,2] and planeY range is [4,6] we need
to be sure the min value of planeY is 4
>
>> + 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.
I will change it to done
>
>> + 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 :-)
I will fix that.
>
>> * @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.
>
ok done
>> +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
>
--
Benjamin Gaignard
Graphic Working Group
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
