| Message ID | 20180104131158.20795-1-peter.ujfalusi@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, On 04/01/18 15:11, 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. > > Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com> > --- > Hi, > > 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 | 24 +++++++++++++++++++++++- > drivers/gpu/drm/omapdrm/omap_plane.c | 10 +++++----- > 2 files changed, 28 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c > index 1b8154e58d18..ff0b17f8bb76 100644 > --- a/drivers/gpu/drm/omapdrm/omap_crtc.c > +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c > @@ -486,6 +486,28 @@ 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 *p1, *p2; > + const struct drm_plane_state *pstate1, *pstate2; > + > + drm_atomic_crtc_state_for_each_plane_state(p1, pstate1, state) { > + drm_atomic_crtc_state_for_each_plane_state(p2, pstate2, state) { > + if (drm_plane_index(p1) == drm_plane_index(p2)) > + continue; > + > + if (pstate1->zpos == pstate2->zpos) { > + DBG("crtc%u: zpos must be unique (zpos: %u)", > + crtc->index, pstate1->zpos); > + return -EINVAL; > + } > + } > + } > + > + return 0; > +} I think this works correctly, but it still does extra checks. If we have two planes on the screen, the code first checks if plane0 has the same zpos as plane1. Then it checks if plane1 has the same zpos as plane0. Not a big problem with the amount of planes we have, though. I think that can be easily avoided with for loops. First for loop goes through all planes. The inner one goes through planes which are after the current one from the outer loop. But that means you can't use drm_atomic_crtc_state_for_each_plane_state(), so if the resulting code would be much larger, maybe it's not worth it. > + > static int omap_crtc_atomic_check(struct drm_crtc *crtc, > struct drm_crtc_state *state) > { > @@ -509,7 +531,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..c1f93bfae7a5 100644 > --- a/drivers/gpu/drm/omapdrm/omap_plane.c > +++ b/drivers/gpu/drm/omapdrm/omap_plane.c > @@ -31,6 +31,7 @@ > struct omap_plane { > struct drm_plane base; > enum omap_plane_id id; > + int idx; The base plane object already has index field. I believe it's available after drm_universal_plane_init() call. Tomi
Hi, On 2018-01-05 12:05, Tomi Valkeinen wrote: > Hi, > > On 04/01/18 15:11, 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. >> >> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com> >> --- >> Hi, >> >> 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 | 24 +++++++++++++++++++++++- >> drivers/gpu/drm/omapdrm/omap_plane.c | 10 +++++----- >> 2 files changed, 28 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c >> index 1b8154e58d18..ff0b17f8bb76 100644 >> --- a/drivers/gpu/drm/omapdrm/omap_crtc.c >> +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c >> @@ -486,6 +486,28 @@ 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 *p1, *p2; >> + const struct drm_plane_state *pstate1, *pstate2; >> + >> + drm_atomic_crtc_state_for_each_plane_state(p1, pstate1, state) { >> + drm_atomic_crtc_state_for_each_plane_state(p2, pstate2, state) { >> + if (drm_plane_index(p1) == drm_plane_index(p2)) >> + continue; >> + >> + if (pstate1->zpos == pstate2->zpos) { >> + DBG("crtc%u: zpos must be unique (zpos: %u)", >> + crtc->index, pstate1->zpos); >> + return -EINVAL; >> + } >> + } >> + } >> + >> + return 0; >> +} > > I think this works correctly, but it still does extra checks. If we have > two planes on the screen, the code first checks if plane0 has the same > zpos as plane1. Then it checks if plane1 has the same zpos as plane0. > > Not a big problem with the amount of planes we have, though. It does extra check also if we have more planes as the second loop would start from the first plane and does unnecessary, already done checks. > I think that can be easily avoided with for loops. First for loop goes > through all planes. The inner one goes through planes which are after > the current one from the outer loop. But that means you can't use > drm_atomic_crtc_state_for_each_plane_state(), so if the resulting code > would be much larger, maybe it's not worth it. Let's see how it will look with a loop. > >> + >> static int omap_crtc_atomic_check(struct drm_crtc *crtc, >> struct drm_crtc_state *state) >> { >> @@ -509,7 +531,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..c1f93bfae7a5 100644 >> --- a/drivers/gpu/drm/omapdrm/omap_plane.c >> +++ b/drivers/gpu/drm/omapdrm/omap_plane.c >> @@ -31,6 +31,7 @@ >> struct omap_plane { >> struct drm_plane base; >> enum omap_plane_id id; >> + int idx; > > The base plane object already has index field. I believe it's available > after drm_universal_plane_init() call. True, I can use drm_plane_index(plane) when it is needed. - Péter Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c index 1b8154e58d18..ff0b17f8bb76 100644 --- a/drivers/gpu/drm/omapdrm/omap_crtc.c +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c @@ -486,6 +486,28 @@ 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 *p1, *p2; + const struct drm_plane_state *pstate1, *pstate2; + + drm_atomic_crtc_state_for_each_plane_state(p1, pstate1, state) { + drm_atomic_crtc_state_for_each_plane_state(p2, pstate2, state) { + if (drm_plane_index(p1) == drm_plane_index(p2)) + continue; + + if (pstate1->zpos == pstate2->zpos) { + DBG("crtc%u: zpos must be unique (zpos: %u)", + crtc->index, pstate1->zpos); + return -EINVAL; + } + } + } + + return 0; +} + static int omap_crtc_atomic_check(struct drm_crtc *crtc, struct drm_crtc_state *state) { @@ -509,7 +531,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..c1f93bfae7a5 100644 --- a/drivers/gpu/drm/omapdrm/omap_plane.c +++ b/drivers/gpu/drm/omapdrm/omap_plane.c @@ -31,6 +31,7 @@ struct omap_plane { struct drm_plane base; enum omap_plane_id id; + int idx; const char *name; }; @@ -97,8 +98,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 = omap_plane->idx; priv->dispc_ops->ovl_enable(omap_plane->id, false); } @@ -194,8 +194,7 @@ static void omap_plane_reset(struct drm_plane *plane) * 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; + plane->state->zpos = omap_plane->idx; } static int omap_plane_atomic_set_property(struct drm_plane *plane, @@ -282,6 +281,7 @@ struct drm_plane *omap_plane_init(struct drm_device *dev, for (nformats = 0; formats[nformats]; ++nformats) ; omap_plane->id = id; + omap_plane->idx = idx; omap_plane->name = plane_id_to_name[id]; plane = &omap_plane->base; @@ -295,7 +295,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. Enforce that all planes must have unique zpos on the given crtc. Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com> --- Hi, 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 | 24 +++++++++++++++++++++++- drivers/gpu/drm/omapdrm/omap_plane.c | 10 +++++----- 2 files changed, 28 insertions(+), 6 deletions(-)