Hi Peter,
Thank you for the patch and sorry for the late review.
On Friday, 5 January 2018 13:30:37 EET Peter Ujfalusi wrote:
> Use the plane index as default zpos for all planes. Even if the
> application is not setting zpos/zorder explicitly we will have unique zpos
> for each plane.
>
> Enforce that all planes must have unique zpos on the given crtc.
Could you explain the rationale for that in the commit message, what's wrong
with duplicate zpos values ?
Isn't there a risk of breaking the non-atomic userspace with this ? Without
atomic commits userspace can't change the zpos of multiple planes in one go,
so it might be impossible to reorder planes without going through a state that
has duplicated zpos values.
> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@xxxxxx>
> ---
> Hi,
>
> Changes since v3:
> - Use drm_plane_index() instead of storing the same index wothin omap_plane
> struct
> - Optimize the zpos validation loop so we avoid extra checks.
>
> Changes since v2:
> - The check for duplicate zpos is moved to omap_crtc
>
> Changes since v1:
> - Dropped the zpos normalization related patches
> - New patch based on the discussion, see commit message.
>
> Regards,
> Peter
>
> drivers/gpu/drm/omapdrm/omap_crtc.c | 36 ++++++++++++++++++++++++++++++++-
> drivers/gpu/drm/omapdrm/omap_plane.c | 15 ++++-----------
> 2 files changed, 39 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c
> b/drivers/gpu/drm/omapdrm/omap_crtc.c index cc85c16cbc2a..c68e3a4f5b7d
> 100644
> --- a/drivers/gpu/drm/omapdrm/omap_crtc.c
> +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
> @@ -452,6 +452,40 @@ static void omap_crtc_mode_set_nofb(struct drm_crtc
> *crtc) }
> }
>
> +static int omap_crtc_validate_zpos(struct drm_crtc *crtc,
> + struct drm_crtc_state *state)
> +{
> + struct drm_device *ddev = crtc->dev;
> + struct drm_plane *p1;
> + const struct drm_plane_state *pstate1;
> +
> + /* Check the crts's planes against duplicated zpos value */
> + drm_atomic_crtc_state_for_each_plane_state(p1, pstate1, state) {
> + struct drm_plane *p2 = p1;
> +
> + list_for_each_entry_continue(p2, &ddev->mode_config.plane_list,
> + head) {
> + const struct drm_plane_state *pstate2;
> +
> + if (!(state->plane_mask & (1 << drm_plane_index(p2))))
> + continue;
> +
> + pstate2 = __drm_atomic_get_current_plane_state(
> + state->state, p2);
> + if (!pstate2)
> + continue;
> +
> + if (pstate1->zpos == pstate2->zpos) {
> + DBG("crtc%u: zpos must be unique (zpos: %u)",
> + crtc->index, pstate1->zpos);
> + return -EINVAL;
> + }
> + }
> + }
That seems a bit complicated to me. Couldn't we use a single loop and a zpos
bitfield ? Something like the following (untested) code.
struct drm_device *ddev = crtc->dev;
const struct drm_plane_state *plane_state;
struct drm_plane *plane;
unsigned int zpos_mask = 0;
/* Check the crts's planes against duplicated zpos value */
drm_atomic_crtc_state_for_each_plane_state(plane, plane_state, state) {
if (zpos_mask & BIT(plane_state->zpos)) {
DBG("crtc%u: zpos must be unique (zpos: %u)",
crtc->index, plane_state->zpos);
return -EINVAL;
}
zpos_mask |= BIT(plane_state->zpos);
}
return 0;
> + return 0;
> +}
> +
> static int omap_crtc_atomic_check(struct drm_crtc *crtc,
> struct drm_crtc_state *state)
> {
> @@ -475,7 +509,7 @@ static int omap_crtc_atomic_check(struct drm_crtc *crtc,
> omap_crtc_state->rotation = pri_state->rotation;
> }
>
> - return 0;
> + return omap_crtc_validate_zpos(crtc, state);
> }
>
> static void omap_crtc_atomic_begin(struct drm_crtc *crtc,
> diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c
> b/drivers/gpu/drm/omapdrm/omap_plane.c index 15e5d5d325c6..d0057a23a5ac
> 100644
> --- a/drivers/gpu/drm/omapdrm/omap_plane.c
> +++ b/drivers/gpu/drm/omapdrm/omap_plane.c
> @@ -99,8 +99,7 @@ static void omap_plane_atomic_disable(struct drm_plane
> *plane, struct omap_plane *omap_plane = to_omap_plane(plane);
>
> plane->state->rotation = DRM_MODE_ROTATE_0;
> - plane->state->zpos = plane->type == DRM_PLANE_TYPE_PRIMARY
> - ? 0 : omap_plane->id;
> + plane->state->zpos = drm_plane_index(plane);
>
> priv->dispc_ops->ovl_enable(omap_plane->id, false);
> }
> @@ -186,18 +185,12 @@ void omap_plane_install_properties(struct drm_plane
> *plane,
>
> static void omap_plane_reset(struct drm_plane *plane)
> {
> - struct omap_plane *omap_plane = to_omap_plane(plane);
> -
> drm_atomic_helper_plane_reset(plane);
> if (!plane->state)
> return;
>
> - /*
> - * Set the zpos default depending on whether we are a primary or overlay
> - * plane.
> - */
> - plane->state->zpos = plane->type == DRM_PLANE_TYPE_PRIMARY
> - ? 0 : omap_plane->id;
> + /* Set the zpos to the plane index. */
> + plane->state->zpos = drm_plane_index(plane);
> }
>
> static int omap_plane_atomic_set_property(struct drm_plane *plane,
> @@ -297,7 +290,7 @@ struct drm_plane *omap_plane_init(struct drm_device
> *dev, drm_plane_helper_add(plane, &omap_plane_helper_funcs);
>
> omap_plane_install_properties(plane, &plane->base);
> - drm_plane_create_zpos_property(plane, 0, 0, num_planes - 1);
> + drm_plane_create_zpos_property(plane, idx, 0, num_planes - 1);
>
> return plane;
--
Regards,
Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
