From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marek Szyprowski Subject: Re: [PATCH 1/4] drm: add support for generic zpos property Date: Thu, 07 Jan 2016 15:33:36 +0100 Message-ID: <568E7740.8090709@samsung.com> References: <1451998373-13708-1-git-send-email-m.szyprowski@samsung.com> <1451998373-13708-2-git-send-email-m.szyprowski@samsung.com> <20160107135922.GO8076@phenom.ffwll.local> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: quoted-printable Return-path: In-reply-to: <20160107135922.GO8076@phenom.ffwll.local> Sender: owner-linux-mm@kvack.org To: intel-gfx@lists.freedesktop.org, Linux MM , Peter Zijlstra , Jens Axboe , "Paul E. McKenney" , Christoph Lameter , Hugh Dickins , Benjamin Gaignard List-Id: intel-gfx@lists.freedesktop.org Hello, On 2016-01-07 14:59, Daniel Vetter wrote: > On Tue, Jan 05, 2016 at 01:52:50PM +0100, Marek Szyprowski wrote: >> This patch adds support for generic plane's zpos property property wit= h >> well-defined semantics: >> - added zpos properties to drm core 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 pl= ane >> id value if zpos equals >> >> Signed-off-by: Marek Szyprowski > lgtm I think. Longer-term we want to think whether we don't want to > extract such extensions into separate files, and push the kerneldoc int= o > an overview DOC: section in there. Just to keep things more closely > together. Benjamin with drm/sti also needs this, so cc'ing him. Besides sti and exynos, zpos is also already implemented in rcar, mdp5=20 and omap drivers. I'm not sure what should be done in case of omap, which uses=20 this property with different name ("zorder" instead of "zpos"). >> --- >> Documentation/DocBook/gpu.tmpl | 14 ++++++++-- >> drivers/gpu/drm/drm_atomic.c | 4 +++ >> drivers/gpu/drm/drm_atomic_helper.c | 52 +++++++++++++++++++++++++++= ++++++++++ >> drivers/gpu/drm/drm_crtc.c | 13 ++++++++++ >> include/drm/drm_atomic_helper.h | 2 ++ >> include/drm/drm_crtc.h | 13 ++++++++++ >> 6 files changed, 96 insertions(+), 2 deletions(-) >> >> diff --git a/Documentation/DocBook/gpu.tmpl b/Documentation/DocBook/gp= u.tmpl >> index 6c6e81a9eaf4..e81acd999891 100644 >> --- a/Documentation/DocBook/gpu.tmpl >> +++ b/Documentation/DocBook/gpu.tmpl >> @@ -2004,7 +2004,7 @@ void intel_crt_init(struct drm_device *dev) >> Description/Restrictions >> >> >> - DRM >> + DRM >> Generic >> =E2=80=9Crotation=E2=80=9D >> BITMASK >> @@ -2256,7 +2256,7 @@ void intel_crt_init(struct drm_device *dev) >> property to suggest an Y offset for a connector= >> >> >> - Optional >> + Optional >> =E2=80=9Cscaling mode=E2=80=9D >> ENUM >> { "None", "Full", "Center", "Full aspect" } >> @@ -2280,6 +2280,16 @@ void intel_crt_init(struct drm_device *dev) >> TBD >> >> >> + "zpos" >> + RANGE >> + Min=3D0, Max=3D255 >> + Plane >> + Plane's 'z' position during blending (0 for back= ground, 255 for frontmost). >> + If two planes assigned to same CRTC have equal zpos values, the pla= ne with higher plane >> + id is treated as closer to front. Can be IMMUTABLE if driver doesn'= t support changing >> + plane's order. >> + >> + >> i915 >> Generic >> "Broadcast RGB" >> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic= .c >> index 6a21e5c378c1..97bb069cb6a3 100644 >> --- a/drivers/gpu/drm/drm_atomic.c >> +++ b/drivers/gpu/drm/drm_atomic.c >> @@ -614,6 +614,8 @@ int drm_atomic_plane_set_property(struct drm_plane= *plane, >> state->src_h =3D val; >> } else if (property =3D=3D config->rotation_property) { >> state->rotation =3D val; >> + } else if (property =3D=3D config->zpos_property) { >> + state->zpos =3D val; >> } else if (plane->funcs->atomic_set_property) { >> return plane->funcs->atomic_set_property(plane, state, >> property, val); >> @@ -670,6 +672,8 @@ drm_atomic_plane_get_property(struct drm_plane *pl= ane, >> *val =3D state->src_h; >> } else if (property =3D=3D config->rotation_property) { >> *val =3D state->rotation; >> + } else if (property =3D=3D config->zpos_property) { >> + *val =3D state->zpos; >> } else if (plane->funcs->atomic_get_property) { >> return plane->funcs->atomic_get_property(plane, state, property, v= al); >> } else { >> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm= _atomic_helper.c >> index 268d37f26960..de3ca33eb696 100644 >> --- a/drivers/gpu/drm/drm_atomic_helper.c >> +++ b/drivers/gpu/drm/drm_atomic_helper.c >> @@ -31,6 +31,7 @@ >> #include >> #include >> #include >> +#include >> =20 >> /** >> * DOC: overview >> @@ -2781,3 +2782,54 @@ void drm_atomic_helper_connector_destroy_state(= struct drm_connector *connector, >> kfree(state); >> } >> EXPORT_SYMBOL(drm_atomic_helper_connector_destroy_state); >> + >> +/** >> + * __drm_atomic_helper_plane_zpos_cmp - compare zpos value of two pla= nes >> + * @a: pointer to first plane >> + * @b: pointer to second plane > Generally we don't do kerneldoc for non-exported functions. If you want > just keep the text itself as a comment, or better move it into the > official interface docs for drm_atomic_helper_normalize_zpos(). okay >> + * >> + * This function is used for comparing two planes while sorting them = to assign >> + * a normalized zpos values. Planes are compared first by their zpos = values, >> + * then in case they equal, by plane id. >> + */ >> +static int __drm_atomic_helper_plane_zpos_cmp(const void *a, const vo= id *b) >> +{ >> + const struct drm_plane *pa =3D *(struct drm_plane **)a; >> + const struct drm_plane *pb =3D *(struct drm_plane **)b; >> + int zpos_a =3D 0, zpos_b =3D 0; >> + >> + if (pa->state) >> + zpos_a =3D pa->state->zpos << 16; >> + if (pb->state) >> + zpos_b =3D pb->state->zpos << 16; >> + >> + zpos_a +=3D pa->base.id; >> + zpos_b +=3D pb->base.id; >> + >> + return zpos_a - zpos_b; >> +} >> + >> +/** >> + * drm_atomic_helper_normalize_zpos - calculate normalized zpos value= s >> + * @planes: arrays of pointer to planes to consider for normalization >> + * @count: number of planes in the above array >> + * >> + * This function takes arrays of pointers to planes and calculates no= rmalized >> + * zpos value for them taking into account each planes[i]->state->zpo= s value >> + * and plane's id (if zpos equals). The plane[i]->state->normalized_z= pos is >> + * then filled with uniqe values from 0 to count-1. >> + * Note: a side effect of this function is the fact that the planes a= rray will >> + * be modified (sorted). It is up to the called to construct planes a= rray with >> + * all planes that have been assigned to given crtc. >> + */ >> +void drm_atomic_helper_normalize_zpos(struct drm_plane *planes[], int= count) >> +{ >> + int i; >> + >> + sort(planes, count, sizeof(struct drm_plane *), >> + __drm_atomic_helper_plane_zpos_cmp, NULL); >> + for (i =3D 0; i < count; i++) >> + if (planes[i]->state) >> + planes[i]->state->normalized_zpos =3D i; >> +} >> +EXPORT_SYMBOL(drm_atomic_helper_normalize_zpos); >> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c >> index 62fa95fa5471..51474ea179f6 100644 >> --- a/drivers/gpu/drm/drm_crtc.c >> +++ b/drivers/gpu/drm/drm_crtc.c >> @@ -5879,6 +5879,19 @@ struct drm_property *drm_mode_create_rotation_p= roperty(struct drm_device *dev, >> } >> EXPORT_SYMBOL(drm_mode_create_rotation_property); >> =20 > Please add minimal kerneldoc for these two, with the suggestions that > drivers should use drm_atomic_helper_normalize_zpos(). Also, I'd > duplicated the semantic definition here (and explain why immutable > exists) from the gpu.tmpl table. > >> +struct drm_property *drm_plane_create_zpos_property(struct drm_device= *dev) >> +{ >> + return drm_property_create_range(dev, 0, "zpos", 0, 255); > The zpos property should be stored in dev_priv->mode_config.zpos_prop. = We > need it there so that generic core code (in drm_atomic_plane_set_proper= ty) > can decode it. That way drivers only need to set up&attach it the prop, > but otherwise can deal with just the decoded values. Okay. > Another consideration: Should we reset zpos to something sane in the fb= dev > helpers, like we do for rotation? Yes and no. fbdev will enable only one (primary) plane per crtc, so zpos=20 set to zero will be properly "normalized" and then interpreted by the driver.=20 However if no fbdev is used and the driver doesn't subclass drm_plane_state, then drm_atomic_helper_plane_reset() should set it to some sane initial=20 values (the best would be to use initial values passed to=20 drm_object_attach_property() on plane initialization). The only problem is that I didn't find any good=20 place to store the initial zpos value. In case of exynos, drm_plane_state is=20 subclassed, so I can set it in my own exynos_drm_plane_reset() method. > Thanks, Daniel >> ... Best regards --=20 Marek Szyprowski, PhD Samsung R&D Institute Poland -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: email@kvack.org