Re: [PATCH v5] drm/omap: plane zpos/zorder management improvements

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Jan 09, 2018 at 04:18:48PM +0200, Peter Ujfalusi wrote:
> Hi,
> 
> On 2018-01-09 14:44, Daniel Vetter wrote:
> >> Changes since v4:
> >> - further simplify the zpos checking by using a mask and a single loop
> >> - Commit message has been extended
> >>
> >> 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.
> > 
> > Sorry for jumping in late to the party, but except when you have a really,
> > really, really good reason for why omapdrm has to not normalize zpos like
> > the other drivers do in this case, then I think we should be consistent.
> > 
> > An inconsistent kms uapi is a lot worse than an uapi with some design
> > issues: The latter just means we eventually need v2, the former guarantees
> > we need v2.
> 
> Even if the v2 contains the "drm/blend: Account also the primary plane
> of the crtc for normalized_zpos"?
> It is to ensure that the crtc->primary plane is going to have zpos = 0,
> even if it's plane_id is higher.

It's a bit a hack, but imo makes sense, given where we are with uapi.
Except it sounds like we have more bikesheds going on about what exactly
zpos is supposed to do.

> As it was discussed we have use case when we have two (or more) crtcs,
> each with one plane (they are the primary planes for the given crtc) and
> user moves one of the plane from one crtc to another, where it is no
> longer the primary plane, but still holds zpos = 0.
> 
> In this case we prefer to keep the actual primary plane of the crtc at
> the bottom and stack the new planes on top.

Yeah that all sounds reasonable. Or we state that userspace in that case
better readjust zpos to make it non-ambiguous. Or something else.

Just something that's consistent across drivers. I'm totally fine with
"organically grown uapi with lots of cruds and hacks".
-Daniel

> 
> > Thanks, Daniel
> > 
> >>
> >> Regards,
> >> Peter
> >>
> >>  drivers/gpu/drm/omapdrm/omap_crtc.c  | 23 ++++++++++++++++++++++-
> >>  drivers/gpu/drm/omapdrm/omap_plane.c | 15 ++++-----------
> >>  2 files changed, 26 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c
> >> index 1b8154e58d18..941a6440fc8e 100644
> >> --- a/drivers/gpu/drm/omapdrm/omap_crtc.c
> >> +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
> >> @@ -486,6 +486,27 @@ 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_plane *plane;
> >> +	const struct drm_plane_state *pstate;
> >> +	unsigned int zpos_mask = 0;
> >> +
> >> +	/* Check the crts's planes against duplicated zpos value */
> >> +	drm_atomic_crtc_state_for_each_plane_state(plane, pstate, state) {
> >> +		if (zpos_mask & BIT(pstate->zpos)) {
> >> +			DBG("crtc%u: zpos must be unique (zpos: %u)",
> >> +			    crtc->index, pstate->zpos);
> >> +			return -EINVAL;
> >> +		}
> >> +
> >> +		zpos_mask |= BIT(pstate->zpos);
> >> +	}
> >> +
> >> +	return 0;
> >> +}
> >> +
> >>  static int omap_crtc_atomic_check(struct drm_crtc *crtc,
> >>  				struct drm_crtc_state *state)
> >>  {
> >> @@ -509,7 +530,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 7d789d1551a1..39f25210bef1 100644
> >> --- a/drivers/gpu/drm/omapdrm/omap_plane.c
> >> +++ b/drivers/gpu/drm/omapdrm/omap_plane.c
> >> @@ -97,8 +97,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);
> >>  }
> >> @@ -184,18 +183,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,
> >> @@ -295,7 +288,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;
> >>  
> >> -- 
> >> Peter
> >>
> >> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
> >> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
> >>
> > 
> 
> - Péter
> 
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel