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

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

 



I will merge your changes for the next version, thanks

zpos property is not a new property sti, exynos and r-car already used it.
I know some not upstreamed code use it to manage zordering.
For example I have wrote an Android hwcomposer using this property:
https://git.linaro.org/people/benjamin.gaignard/hwcomposer.git/blob/HEAD:/hwcomposer.cpp#l591

I hope those patches could help to standardize zordering in userland.



2016-07-20 12:11 GMT+02:00 Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>:
> On Thu, Jul 07, 2016 at 03:01:21PM +0200, Benjamin Gaignard wrote:
>> From: Marek Szyprowski <m.szyprowski@xxxxxxxxxxx>
>>
>> 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>
>> ---
>>  drivers/gpu/drm/Makefile            |   2 +-
>>  drivers/gpu/drm/drm_atomic.c        |   4 +
>>  drivers/gpu/drm/drm_atomic_helper.c |   6 +
>>  drivers/gpu/drm/drm_blend.c         | 227 ++++++++++++++++++++++++++++++++++++
>>  drivers/gpu/drm/drm_crtc_internal.h |   4 +
>>  include/drm/drm_crtc.h              |  30 +++++
>>  6 files changed, 272 insertions(+), 1 deletion(-)
>>  create mode 100644 drivers/gpu/drm/drm_blend.c
>>
>> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
>> index e3dba6f..7fbcf3f 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.c b/drivers/gpu/drm/drm_atomic.c
>> index 3cee084..e503b8f 100644
>> --- a/drivers/gpu/drm/drm_atomic.c
>> +++ b/drivers/gpu/drm/drm_atomic.c
>> @@ -712,6 +712,8 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
>>               state->src_h = val;
>>       } else if (property == config->rotation_property) {
>>               state->rotation = val;
>> +     } else if (property == plane->zpos_property) {
>> +             state->zpos = val;
>>       } else if (plane->funcs->atomic_set_property) {
>>               return plane->funcs->atomic_set_property(plane, state,
>>                               property, val);
>> @@ -768,6 +770,8 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
>>               *val = state->src_h;
>>       } else if (property == config->rotation_property) {
>>               *val = state->rotation;
>> +     } else if (property == plane->zpos_property) {
>> +             *val = state->zpos;
>>       } else if (plane->funcs->atomic_get_property) {
>>               return plane->funcs->atomic_get_property(plane, state, property, val);
>>       } else {
>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
>> index de7fddc..0694ae1 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"
>> +
>>  /**
>>   * DOC: overview
>>   *
>> @@ -592,6 +594,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..5ae6e0e
>> --- /dev/null
>> +++ b/drivers/gpu/drm/drm_blend.c
>> @@ -0,0 +1,227 @@
>> +/*
>> + * 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
>> + * @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)
>
> I think we also want to pass the initial value here, and set the plane
> state accordingly.
>
>> +{
>> +     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, min);
>> +
>> +     plane->zpos_property = prop;
>> +     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)
>
> What's the point of passing min/max here? I would think we'd just want
> the one value. And the earlier comment about the plane state holds here
> as well.
>
>> +{
>> +     struct drm_property *prop;
>> +
>> +     prop = drm_property_create_range(plane->dev, DRM_MODE_PROP_IMMUTABLE,
>> +                                      "zpos", min, max);
>> +     if (!prop)
>> +             return -ENOMEM;
>> +
>> +     drm_object_attach_property(&plane->base, prop, min);
>> +
>> +     plane->zpos_property = prop;
>> +     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)
>> +{
>> +     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;
>
> This needs to be reset back in duplicate_state.
>
> I did those changes locally, and then I went ahead and implemented zpos
> support for some Intel platforms, mainly to make sure this thing is good
> enough for us. It doesn't result in the prettiest thing, but it does
> appear to work at least:
> git://github.com/vsyrjala/linux.git vlv_chv_zpos
>
> I still need to write some actual igt test for this stuff I think. And
> then we have the "do we have actual userspace for this?" problem. Was
> there an atomic weston branch somewhere that can use zpos?
>
>> +
>> +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 47a500b..0c34e6d 100644
>> --- a/drivers/gpu/drm/drm_crtc_internal.h
>> +++ b/drivers/gpu/drm/drm_crtc_internal.h
>> @@ -128,3 +128,7 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
>>
>>  int drm_modeset_register_all(struct drm_device *dev);
>>  void drm_modeset_unregister_all(struct drm_device *dev);
>> +
>> +/* 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 f5469d3..3936ddf 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
>>   * @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;
>>  };
>>
>> @@ -1675,6 +1684,7 @@ enum drm_plane_type {
>>   * @properties: property tracking for this plane
>>   * @type: type of plane (overlay, primary, cursor)
>>   * @state: current atomic state for this plane
>> + * @zpos_property: zpos property for this plane
>>   * @helper_private: mid-layer private data
>>   */
>>  struct drm_plane {
>> @@ -1716,6 +1726,8 @@ struct drm_plane {
>>       const struct drm_plane_helper_funcs *helper_private;
>>
>>       struct drm_plane_state *state;
>> +
>> +     struct drm_property *zpos_property;
>>  };
>>
>>  /**
>> @@ -2773,6 +2785,24 @@ extern void drm_crtc_enable_color_mgmt(struct drm_crtc *crtc,
>>                                      uint degamma_lut_size,
>>                                      bool has_ctm,
>>                                      uint gamma_lut_size);
>> +
>> +int drm_plane_atomic_set_zpos_property(struct drm_plane *plane,
>> +                                    struct drm_plane_state *state,
>> +                                    struct drm_property *property,
>> +                                    uint64_t val);
>> +
>> +int drm_plane_atomic_get_zpos_property(struct drm_plane *plane,
>> +                                    const struct drm_plane_state *state,
>> +                                    struct drm_property *property,
>> +                                    uint64_t *val);
>> +
>> +int drm_plane_create_zpos_property(struct drm_plane *plane,
>> +                                unsigned int min, unsigned int max);
>> +
>> +int drm_plane_create_zpos_immutable_property(struct drm_plane *plane,
>> +                                          unsigned int min,
>> +                                          unsigned int max);
>> +
>>  /* Helpers */
>>  struct drm_mode_object *drm_mode_object_find(struct drm_device *dev,
>>                                            uint32_t id, uint32_t type);
>> --
>> 1.9.1
>
> --
> Ville Syrjälä
> Intel OTC



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



[Index of Archives]     [Linux SoC Development]     [Linux Rockchip Development]     [Linux USB Development]     [Video for Linux]     [Linux Audio Users]     [Linux SCSI]     [Yosemite News]

  Powered by Linux