| Message ID | 20180109114556.23515-1-peter.ujfalusi@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jan 09, 2018 at 01:45:56PM +0200, 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. > > Planes with identical zpos value will result undefined behavior: > disappearing planes, screen flickering and it is not supported by the > hardware. > > Enforce that all planes must have unique zpos on the given crtc by > returning error when duplicate zpos value is requested. > > Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com> > --- > Hi, > > 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. 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 >
Hi Peter, Thank you for the patch. On Tuesday, 9 January 2018 13:45:56 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. > > Planes with identical zpos value will result undefined behavior: > disappearing planes, screen flickering and it is not supported by the > hardware. > > Enforce that all planes must have unique zpos on the given crtc by > returning error when duplicate zpos value is requested. > > Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > Hi, > > 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. > > 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;
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. 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. > 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
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
On 01/09/2018 04:40 PM, Daniel Vetter wrote: > 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 https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html have this to say about zpos: "priority of the given plane on crtc (optional) Note that multiple active planes on the same crtc can have an identical zpos value. The rule to solving the conflict is to compare the plane object IDs; the plane with a higher ID must be stacked on top of a plane with a lower ID." It implies that the driver should not try to be clever about the normalization of the zpos. Even if it make sense. Considering only crtc->primary is a bit flowed anyway as for the sake of completeness the crtc->cursor plane should have been kept on top at the same time. >> 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". I'm fine with using the current normalization as it is and refer to the UAPI doc if user space is not complying with it. But then, should the normalization be forced in the core for all drivers to get consistency? > -Daniel > >> >>> Thanks, Daniel
On Wed, Jan 10, 2018 at 06:28:51PM +0200, Peter Ujfalusi wrote: > On 01/09/2018 04:40 PM, Daniel Vetter wrote: > > 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 https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html have this to say > about zpos: > "priority of the given plane on crtc (optional) Note that multiple active > planes on the same crtc can have an identical zpos value. The rule to solving > the conflict is to compare the plane object IDs; the plane with a higher ID > must be stacked on top of a plane with a lower ID." > > It implies that the driver should not try to be clever about the normalization > of the zpos. Even if it make sense. > > Considering only crtc->primary is a bit flowed anyway as for the sake of > completeness the crtc->cursor plane should have been kept on top at the same time. > > >> 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". > > I'm fine with using the current normalization as it is and refer to the UAPI > doc if user space is not complying with it. > But then, should the normalization be forced in the core for all drivers to > get consistency? We had that, but then removed it again for reasons I no longer entirely understand. I guess we can keep it as-is for now, or if you want you can float a patch to move it back into the main helpers. -Daniel
Hi Daniel, On Thursday, 11 January 2018 10:19:50 EET Daniel Vetter wrote: > On Wed, Jan 10, 2018 at 06:28:51PM +0200, Peter Ujfalusi wrote: > > On 01/09/2018 04:40 PM, Daniel Vetter wrote: > >> On Tue, Jan 09, 2018 at 04:18:48PM +0200, Peter Ujfalusi wrote: > >>> 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 https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html have this to say > > about zpos: > > "priority of the given plane on crtc (optional) Note that multiple active > > planes on the same crtc can have an identical zpos value. The rule to > > solving the conflict is to compare the plane object IDs; the plane with a > > higher ID must be stacked on top of a plane with a lower ID." > > > > It implies that the driver should not try to be clever about the > > normalization of the zpos. Even if it make sense. > > > > Considering only crtc->primary is a bit flowed anyway as for the sake of > > completeness the crtc->cursor plane should have been kept on top at the > > same time. > > > >>> 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". > > > > I'm fine with using the current normalization as it is and refer to the > > UAPI doc if user space is not complying with it. > > But then, should the normalization be forced in the core for all drivers > > to get consistency? > > We had that, but then removed it again for reasons I no longer entirely > understand. I guess we can keep it as-is for now, or if you want you can > float a patch to move it back into the main helpers. The problem is that zpos normalization requires accessing the state of all enabled planes for a CRTC in order to compute (and store) the normalized zpos values. This thus forces all planes to be added to the commit state, even when the commit doesn't touch the zpos property. I assume this caused issues (possibly performance issues) in drivers that then performed hardware setup of all planes as a result. It could be possible to implement zpos normalization in a more efficient way, or to make it possible for driver to optimize hardware setup when plane states have not changed. For instance (thinking out loud) we could add a bit to the plane state to tell whether anything has changed, drivers could then easily skip those planes.
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;
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. Planes with identical zpos value will result undefined behavior: disappearing planes, screen flickering and it is not supported by the hardware. Enforce that all planes must have unique zpos on the given crtc by returning error when duplicate zpos value is requested. Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com> --- Hi, 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. 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(-)