On Fri, Sep 21, 2018 at 09:35:52PM +0300, Ville Syrjälä wrote:
> On Fri, Sep 21, 2018 at 07:39:40PM +0200, Maarten Lankhorst wrote:
> > To make NV12 working on icl, we need to update 2 planes simultaneously.
> > I've chosen to do this in the CRTC step after plane validation is done,
> > so we know what planes are (in)visible. The linked Y plane will get
> > updated in intel_plane_update_planes_on_crtc(), by the call to
> > update_slave, which gets the master's plane_state as argument.
> >
> > The link requires both planes for atomic_update to work,
> > so make sure skl_ddb_add_affected_planes() adds both states.
> >
> > Changes since v1:
> > - Introduce icl_is_nv12_y_plane(), instead of hardcoding sprite numbers.
> > - Put all the state updating login in intel_plane_atomic_check_with_state().
> > - Clean up changes in intel_plane_atomic_check().
> > Changes since v2:
> > - Fix intel_atomic_get_old_plane_state() to actually return old state.
> > - Move visibility changes to preparation patch.
> > - Only try to find a Y plane on gen11, earlier platforms only require a single plane.
> >
> > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> >
> > fixup Y/UV Linkage
> >
> > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/intel_atomic_plane.c | 106 ++++++++++++++++++----
> > drivers/gpu/drm/i915/intel_display.c | 57 ++++++++++++
> > drivers/gpu/drm/i915/intel_drv.h | 53 +++++++++++
> > drivers/gpu/drm/i915/intel_pm.c | 12 ++-
> > 4 files changed, 210 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
> > index 984bc1f26625..522699085a59 100644
> > --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
> > +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
> > @@ -121,7 +121,11 @@ int intel_plane_atomic_check_with_state(const struct intel_crtc_state *old_crtc_
> > crtc_state->nv12_planes &= ~BIT(intel_plane->id);
> > intel_state->base.visible = false;
> >
> > - /* If this is a cursor plane, no further checks are needed. */
> > + /* Destroy the link */
> > + intel_state->linked_plane = NULL;
> > + intel_state->slave = false;
> > +
> > + /* If this is a cursor or Y plane, no further checks are needed. */
> > if (!intel_state->base.crtc && !old_plane_state->base.crtc)
> > return 0;
> >
> > @@ -142,27 +146,76 @@ int intel_plane_atomic_check_with_state(const struct intel_crtc_state *old_crtc_
> > state);
> > }
> >
> > -static int intel_plane_atomic_check(struct drm_plane *plane,
> > - struct drm_plane_state *new_plane_state)
> > +static int intel_plane_atomic_check(struct drm_plane *drm_plane,
> > + struct drm_plane_state *new_drm_plane_state)
> > {
> > - struct drm_atomic_state *state = new_plane_state->state;
> > - const struct drm_plane_state *old_plane_state =
> > - drm_atomic_get_old_plane_state(state, plane);
> > - struct drm_crtc *crtc = new_plane_state->crtc ?: old_plane_state->crtc;
> > - const struct drm_crtc_state *old_crtc_state;
> > - struct drm_crtc_state *new_crtc_state;
> > -
> > - new_plane_state->visible = false;
> > + struct intel_atomic_state *state =
> > + to_intel_atomic_state(new_drm_plane_state->state);
> > + struct intel_plane *plane = to_intel_plane(drm_plane);
> > + const struct intel_plane_state *old_plane_state =
> > + intel_atomic_get_old_plane_state(state, plane);
> > + struct intel_plane_state *new_plane_state =
> > + to_intel_plane_state(new_drm_plane_state);
> > + struct intel_crtc *crtc = to_intel_crtc(
> > + new_plane_state->base.crtc ?:
> > + old_plane_state->base.crtc);
> > + const struct intel_crtc_state *old_crtc_state;
> > + struct intel_crtc_state *new_crtc_state;
> > + struct intel_plane *linked = old_plane_state->linked_plane;
> > + int ret;
> > + const struct intel_plane_state *old_linked_state;
> > + struct intel_plane_state *new_linked_state = NULL;
> > +
> > + if (linked) {
> > + /*
> > + * Make sure a previously linked plane (and implicitly, the CRTC)
> > + * is part of the atomic commit.
> > + */
> > + if (!intel_atomic_get_new_plane_state(state, linked)) {
> > + new_linked_state = intel_atomic_get_plane_state(state, linked);
> > + if (IS_ERR(new_linked_state))
> > + return PTR_ERR(new_linked_state);
> > + }
> > +
> > + old_linked_state =
> > + intel_atomic_get_old_plane_state(state, linked);
> > +
> > + /*
> > + * This will happen when we're the Y plane. In which case
> > + * old/new_state->crtc are both NULL. We still need to perform
> > + * updates on the linked plane.
> > + */
> > + if (!crtc)
> > + crtc = to_intel_crtc(old_linked_state->base.crtc);
> > +
> > + WARN_ON(!crtc);
> > + }
> > +
> > + new_plane_state->base.visible = false;
> > if (!crtc)
> > return 0;
> >
> > - old_crtc_state = drm_atomic_get_old_crtc_state(state, crtc);
> > - new_crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
> > + old_crtc_state = intel_atomic_get_old_crtc_state(state, crtc);
> > + new_crtc_state = intel_atomic_get_new_crtc_state(state, crtc);
> > +
> > + if (new_linked_state &&
> > + drm_plane_index(&linked->base) < drm_plane_index(&plane->base)) {
> > + /*
> > + * This function is called from drm_atomic_helper_check_planes(), which
> > + * will normally check the newly added plane for us, but since we're
> > + * already in that function, it won't check the plane if our index
> > + * is bigger than the linked index because of the
> > + * for_each_oldnew_plane_in_state() call.
> > + */
> > + new_crtc_state->base.planes_changed = true;
> > + ret = intel_plane_atomic_check_with_state(old_crtc_state, new_crtc_state,
> > + old_linked_state, new_linked_state);
> > + if (ret)
> > + return ret;
> > + }
>
> This is all rather confusing. Can't we just do a preprocessing step
> before check_planes() to add the linked planes as needed, and then
> let the normal check_planes() do its thing?
Also one thing that slightly worries me is what happens if someone adds
more planes to the state after this has all been done. I guess
currently those cases would either come from the tail end of
intel_atomic_check() or from intel_crtc_atomic_check(). Currently it
would appear that wm/ddb is the only piece of code that can do this
sort of thing.
Some ideas how to solve it perhaps:
- Move the linked plane pointer to the core so that get_plane_state()
can immediately pick up both planes
- Add some kind of flag into the top level atomic state that after this
point no new planes. But I guess duplicate_state() is the only thing
we have and that doesn't know anything about the the top level state
- Live with it and tell everyone to be careful? Some kind of
intel_add_affected_planes() might be helpful here to at least give
people one thing to call in the late stages. Wouldn't help if the
call comes from some helper/core function though. And as there is
no use for such a function currently no point adding it I suppose.
The wm stuff has its own thing, which you covered already.
Dunno, maybe I'm just a bit too paranoid here.
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
