Thanks, I will a version 7 to fix those last details and send a pull request.
2016-07-27 15:58 GMT+02:00 Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>:
> Hi Benjamin,
>
> Thank you for the patch.
>
> On Thursday 21 Jul 2016 10:52:00 Benjamin Gaignard wrote:
>> From: Marek Szyprowski <m.szyprowski@xxxxxxxxxxx>
>>
>> version 6:
>> - add zpos in gpu documentation file
>> - merge Ville patch about zpos initial value and API improvement.
>> I have split Ville patch between zpos core and drivers
>>
>> version 5:
>> - remove zpos range check and comeback to 0 to N-1
>> normalization algorithm
>>
>> version 4:
>> - make sure that normalized zpos value is stay
>> in the defined property range and warn user if not
>>
>> 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/gpu/kms-properties.csv | 1 +
>> drivers/gpu/drm/Makefile | 2 +-
>> drivers/gpu/drm/drm_atomic.c | 4 +
>> drivers/gpu/drm/drm_atomic_helper.c | 7 +
>> drivers/gpu/drm/drm_blend.c | 240 ++++++++++++++++++++++++++++++++
>> drivers/gpu/drm/drm_crtc_internal.h | 4 +
>> include/drm/drm_crtc.h | 30 +++++
>> 7 files changed, 287 insertions(+), 1 deletion(-)
>> create mode 100644 drivers/gpu/drm/drm_blend.c
>>
>> diff --git a/Documentation/gpu/kms-properties.csv
>> b/Documentation/gpu/kms-properties.csv index b6fcaf6..3587ea2 100644
>> --- a/Documentation/gpu/kms-properties.csv
>> +++ b/Documentation/gpu/kms-properties.csv
>> @@ -17,6 +17,7 @@ DRM,Generic,“rotation”,BITMASK,"{ 0, ""rotate-0"" }, { 1,
>> ""rotate-90"" }, { ,,“CRTC_H”,RANGE,"Min=0, Max=UINT_MAX",Plane,Scanout
>> CRTC (destination) height (atomic)
>> ,,“FB_ID”,OBJECT,DRM_MODE_OBJECT_FB,Plane,Scanout framebuffer (atomic)
>> ,,“CRTC_ID”,OBJECT,DRM_MODE_OBJECT_CRTC,Plane,CRTC that plane is attached
>> to (atomic)
>> +,,“zpos”,RANGE,"Min=0, Max=UINT_MAX",Plane,Zorder of the plane
>
> How about a real description ? :-) Maybe something like "Z-order of the plane.
> Planes with higher Z-order values are displayed on top, planes with identical
> Z-order values are display in an undefined order" ?
>
>> ,DVI-I,“subconnector”,ENUM,"{ “Unknown”, “DVI-D”, “DVI-A” }",Connector,TBD
>> ,,“select subconnector”,ENUM,"{ “Automatic”, “DVI-D”, “DVI-A”
>> }",Connector,TBD ,TV,“subconnector”,ENUM,"{ ""Unknown"", ""Composite"",
>> ""SVIDEO"", ""Component"", ""SCART"" }",Connector,TBD
>
> [snip]
>
>> diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c
>> new file mode 100644
>> index 0000000..9567233
>> --- /dev/null
>> +++ b/drivers/gpu/drm/drm_blend.c
>> @@ -0,0 +1,240 @@
>> +/*
>> + * Copyright (C) 2016 Samsung Electronics Co.Ltd
>> + * Authors:
>> + * Marek Szyprowski <m.szyprowski@xxxxxxxxxxx>
>> + *
>> + * DRM core plane blending related functions
>> + *
>> + * Permission to use, copy, modify, distribute, and sell this software and
>> its + * documentation for any purpose is hereby granted without fee,
>> provided that + * the above copyright notice appear in all copies and that
>> both that copyright + * notice and this permission notice appear in
>> supporting documentation, and + * that the name of the copyright holders
>> not be used in advertising or + * publicity pertaining to distribution of
>> the software without specific, + * written prior permission. The copyright
>> holders make no representations + * about the suitability of this software
>> for any purpose. It is provided "as + * is" without express or implied
>> warranty.
>> + *
>> + * THE COPYRIGHT HOLDERS DISCLAIM ALL WARRANTIES WITH REGARD TO THIS
>> SOFTWARE, + * INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND
>> FITNESS, IN NO + * EVENT SHALL THE COPYRIGHT HOLDERS BE LIABLE FOR ANY
>> SPECIAL, INDIRECT OR + * CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER
>> RESULTING FROM LOSS OF USE, + * DATA OR PROFITS, WHETHER IN AN ACTION OF
>> CONTRACT, NEGLIGENCE OR OTHER + * TORTIOUS ACTION, ARISING OUT OF OR IN
>> CONNECTION WITH THE USE OR PERFORMANCE + * OF THIS SOFTWARE.
>> + */
>> +#include <drm/drmP.h>
>> +#include <drm/drm_atomic.h>
>> +#include <drm/drm_crtc.h>
>> +#include <linux/export.h>
>> +#include <linux/slab.h>
>> +#include <linux/sort.h>
>> +
>> +#include "drm_internal.h"
>> +
>> +/**
>> + * drm_plane_create_zpos_property - create mutable zpos property
>> + * @plane: drm plane
>> + * @zpos: initial value of zpos property
>> + * @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 zpos,
>> + 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;
>> +
>> + drm_object_attach_property(&plane->base, prop, zpos);
>> +
>> + plane->zpos_property = prop;
>> +
>> + if (plane->state) {
>> + plane->state->zpos = zpos;
>> + plane->state->normalized_zpos = zpos;
>> + }
>> +
>> + return 0;
>> +}
>> +EXPORT_SYMBOL(drm_plane_create_zpos_property);
>> +
>> +/**
>> + * drm_plane_create_zpos_immutable_property - create immuttable zpos
>> property + * @plane: drm plane
>> + * @zpos: 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 zpos)
>> +{
>> + struct drm_property *prop;
>> +
>> + prop = drm_property_create_range(plane->dev, DRM_MODE_PROP_IMMUTABLE,
>> + "zpos", zpos, zpos);
>> + if (!prop)
>> + return -ENOMEM;
>> +
>> + drm_object_attach_property(&plane->base, prop, zpos);
>> +
>> + plane->zpos_property = prop;
>> +
>> + if (plane->state) {
>> + plane->state->zpos = zpos;
>> + plane->state->normalized_zpos = zpos;
>> + }
>> +
>> + 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;
>> +
>> + 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)
>
> As Ville mentioned I think you can make this function static.
>
>> +{
>> + 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, n = 0;
>> + int ret = 0;
>> +
>> + DRM_DEBUG_ATOMIC("[CRTC:%d:%s] calculating normalized zpos values\n",
>> + crtc->base.id, crtc->name);
>> +
>> + states = kmalloc_array(total_planes, sizeof(*states), GFP_TEMPORARY);
>> + 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 done;
>> + }
>> + 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; i < n; i++) {
>> + plane = states[i]->plane;
>> +
>> + states[i]->normalized_zpos = i;
>> + DRM_DEBUG_ATOMIC("[PLANE:%d:%s] normalized zpos value %d\n",
>> + plane->base.id, plane->name, i);
>> + }
>> + crtc_state->zpos_changed = true;
>> +
>> +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);
>
> And this one doesn't need to be exported as it's called by the DRM core only.
>
> [snip]
>
>> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
>> index 9e6ab4a..69c2092 100644
>> --- a/include/drm/drm_crtc.h
>> +++ b/include/drm/drm_crtc.h
>> @@ -308,6 +308,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
>> @@ -344,6 +345,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:
>> @@ -1396,6 +1398,9 @@ struct drm_connector {
>> * @src_w: width of visible portion of plane (in 16.16)
>> * @src_h: height of visible portion of plane (in 16.16)
>> * @rotation: rotation of the plane
>> + * @zpos: priority of the given plane on crtc (optional)
>> + * @normalized_zpos: normalized value of zpos: unique, range from 0 to N-1
>> + * for given crtc
>
> You might want to define N.
>
> Apart from that,
>
> Acked-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> Tested-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
>
>> * @state: backpointer to global drm_atomic_state
>> */
>> struct drm_plane_state {
>> @@ -1416,6 +1421,10 @@ struct drm_plane_state {
>> /* Plane rotation */
>> unsigned int rotation;
>>
>> + /* Plane zpos */
>> + unsigned int zpos;
>> + unsigned int normalized_zpos;
>> +
>> struct drm_atomic_state *state;
>> };
>
> [snip]
>
> --
> Regards,
>
> Laurent Pinchart
>
--
Benjamin Gaignard
Graphic Study Group
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
