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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
