| Message ID | 20180105113037.28653-1-peter.ujfalusi@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@ti.com> > --- > 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;
Hi Laurent, On 2018-01-05 16:04, Laurent Pinchart wrote: > 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 ? Planes with identical zpos is only 'valid' _if_ they are not overlapping, if they do overlap then it is - imho - not a valid configuration anyway (which one should be on top?). Based on my tests the plane with lower planeID is going to disappear from the screen if we have duplicated zpos. > 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. Two planes occupying the same position on the screen is not valid (again, imho). If application wants to swap two planes, then it must disable one, move the other to the new position, then enable and move the first plane. > >> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com> >> --- >> 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; Yes, it is much simpler. > >> + 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; > - Péter Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
On 08/01/18 10:20, Peter Ujfalusi wrote: > Hi Laurent, > > On 2018-01-05 16:04, Laurent Pinchart wrote: >> 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 ? > > Planes with identical zpos is only 'valid' _if_ they are not > overlapping, if they do overlap then it is - imho - not a valid > configuration anyway (which one should be on top?). For DSS it's clear. It is an invalid HW configuration to have multiple planes with the same zpos in the same crtc. I believe the result is undefined HW behavior. So we either return an error, or the kernel normalizes zpos'es. Normalizing means the kernel is guessing what the end result should be, so I like error better. Tomi
Hello, On Monday, 8 January 2018 10:59:29 EET Tomi Valkeinen wrote: > On 08/01/18 10:20, Peter Ujfalusi wrote: > > On 2018-01-05 16:04, Laurent Pinchart wrote: > >> 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 ? > > > > Planes with identical zpos is only 'valid' _if_ they are not > > overlapping, if they do overlap then it is - imho - not a valid > > configuration anyway (which one should be on top?). > > For DSS it's clear. It is an invalid HW configuration to have multiple > planes with the same zpos in the same crtc. I believe the result is > undefined HW behavior. > > So we either return an error, or the kernel normalizes zpos'es. > Normalizing means the kernel is guessing what the end result should be, > so I like error better. I wouldn't call that guessing :-) Duplicate zpos values result in plane being sorted based on the plane ID (this is obviously implementation-dependent, I mean this is the currently implemented behaviour), which I don't think is an issue in itself. A userspace zpos API that resolves conflicting zpos values that way isn't a broken API, even if its behaviour might be considered a bit complex or cumbersome. I'm not against forbidding duplicate zpos values, but I think the rationale should be captured in the kernel message. There's also a risk of breaking non-atomic userspace (as explained in my previous e-mail), as well as atomic userspace that doesn't set zpos explicitly if run after an application that changed the zpos values. True, that would today result in an undefined behaviour, so this might not be considered a problem.
Hi Peter, On Monday, 8 January 2018 10:20:24 EET Peter Ujfalusi wrote: > On 2018-01-05 16:04, Laurent Pinchart wrote: > > 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 ? > > Planes with identical zpos is only 'valid' _if_ they are not > overlapping, if they do overlap then it is - imho - not a valid > configuration anyway (which one should be on top?). > Based on my tests the plane with lower planeID is going to disappear > from the screen if we have duplicated zpos. Please see my reply to Tomi on this topic. I'm not against the change, but I think the rationale should be captured in the commit message. > > 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. > > Two planes occupying the same position on the screen is not valid > (again, imho). At the hardware level for the DSS, sure. According to the KMS API, however, it is valid, even if the conflict resolution is driver-dependent. > If application wants to swap two planes, then it must disable one, move the > other to the new position, then enable and move the first plane. Applications don't do that at the moment, so there's a risk of breakage. As the current behaviour is undefined we might not considered that as a problem, but there's a risk of returning an error for an operation that currently succeeds. Personally I think all applications should move to the atomic API and handle zpos explicitly so I don't mind too much :) > >> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com> > >> --- > >> 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(-)
On 08/01/18 12:43, Laurent Pinchart wrote: > Hello, > > On Monday, 8 January 2018 10:59:29 EET Tomi Valkeinen wrote: >> On 08/01/18 10:20, Peter Ujfalusi wrote: >>> On 2018-01-05 16:04, Laurent Pinchart wrote: >>>> 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 ? >>> >>> Planes with identical zpos is only 'valid' _if_ they are not >>> overlapping, if they do overlap then it is - imho - not a valid >>> configuration anyway (which one should be on top?). >> >> For DSS it's clear. It is an invalid HW configuration to have multiple >> planes with the same zpos in the same crtc. I believe the result is >> undefined HW behavior. >> >> So we either return an error, or the kernel normalizes zpos'es. >> Normalizing means the kernel is guessing what the end result should be, >> so I like error better. > > I wouldn't call that guessing :-) Duplicate zpos values result in plane being > sorted based on the plane ID (this is obviously implementation-dependent, I > mean this is the currently implemented behaviour), which I don't think is an > issue in itself. A userspace zpos API that resolves conflicting zpos values > that way isn't a broken API, even if its behaviour might be considered a bit > complex or cumbersome. > > I'm not against forbidding duplicate zpos values, but I think the rationale > should be captured in the kernel message. > > There's also a risk of breaking non-atomic userspace (as explained in my I think they are broken already (on omapdrm): the driver doesn't deal with this at the moment in any way, which leads to undefined behavior. I may remember wrong, but I think I have seen sync losts connected to bad z setup. But you are right, it's also possible that nothing bad seems to happen, if the only side effect is just that a plane disappears for a moment. And if this behavior for non-atocic apps is normal, and other drivers allow it, then I do agree that we have to normalize instead of returning an error. > previous e-mail), as well as atomic userspace that doesn't set zpos explicitly > if run after an application that changed the zpos values. True, that would > today result in an undefined behaviour, so this might not be considered a > problem. Yes, this is a subject I have complained a few times: DRM keeping the state. I think that will be a problem with many other properties too. An app could change a platform specific property, which no other app is aware of, and after that no other app would work correctly. I believe each app has to know all the DRM properties and set them accordingly on startup, and/or each app has to reset the properties when exiting. Unfortunately I think both cases are not realistic. Tomi
Hi Tomi, On Monday, 8 January 2018 12:58:28 EET Tomi Valkeinen wrote: > On 08/01/18 12:43, Laurent Pinchart wrote: > > On Monday, 8 January 2018 10:59:29 EET Tomi Valkeinen wrote: > >> On 08/01/18 10:20, Peter Ujfalusi wrote: > >>> On 2018-01-05 16:04, Laurent Pinchart wrote: > >>>> 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 ? > >>> > >>> Planes with identical zpos is only 'valid' _if_ they are not > >>> overlapping, if they do overlap then it is - imho - not a valid > >>> configuration anyway (which one should be on top?). > >> > >> For DSS it's clear. It is an invalid HW configuration to have multiple > >> planes with the same zpos in the same crtc. I believe the result is > >> undefined HW behavior. > >> > >> So we either return an error, or the kernel normalizes zpos'es. > >> Normalizing means the kernel is guessing what the end result should be, > >> so I like error better. > > > > I wouldn't call that guessing :-) Duplicate zpos values result in plane > > being sorted based on the plane ID (this is obviously > > implementation-dependent, I mean this is the currently implemented > > behaviour), which I don't think is an issue in itself. A userspace zpos > > API that resolves conflicting zpos values that way isn't a broken API, > > even if its behaviour might be considered a bit complex or cumbersome. > > > > I'm not against forbidding duplicate zpos values, but I think the > > rationale > > should be captured in the kernel message. > > > > There's also a risk of breaking non-atomic userspace (as explained in my > > I think they are broken already (on omapdrm): the driver doesn't deal > with this at the moment in any way, which leads to undefined behavior. I > may remember wrong, but I think I have seen sync losts connected to bad > z setup. > > But you are right, it's also possible that nothing bad seems to happen, > if the only side effect is just that a plane disappears for a moment. > > And if this behavior for non-atocic apps is normal, and other drivers > allow it, then I do agree that we have to normalize instead of returning > an error. > > > previous e-mail), as well as atomic userspace that doesn't set zpos > > explicitly if run after an application that changed the zpos values. > > True, that would today result in an undefined behaviour, so this might > > not be considered a problem. > > Yes, this is a subject I have complained a few times: DRM keeping the > state. I think that will be a problem with many other properties too. An > app could change a platform specific property, which no other app is > aware of, and after that no other app would work correctly. > > I believe each app has to know all the DRM properties and set them > accordingly on startup, and/or each app has to reset the properties when > exiting. Unfortunately I think both cases are not realistic. I agree with you. I'd prefer restoring a sane default state on last close. Is there any reason not to do so ?
On 08/01/18 13:07, Laurent Pinchart wrote: >> Yes, this is a subject I have complained a few times: DRM keeping the >> state. I think that will be a problem with many other properties too. An >> app could change a platform specific property, which no other app is >> aware of, and after that no other app would work correctly. >> >> I believe each app has to know all the DRM properties and set them >> accordingly on startup, and/or each app has to reset the properties when >> exiting. Unfortunately I think both cases are not realistic. > > I agree with you. I'd prefer restoring a sane default state on last close. Is > there any reason not to do so ? I have to say I can't remember, but it probably was about fbdev, legacy, on PCs you don't usually encounter problems related to this, or things like that.
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; + } + } + } + + 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;
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 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(-)