On Mon, Sep 24, 2018 at 04:18:10PM +0300, Ville Syrjälä wrote:
> On Mon, Sep 24, 2018 at 02:35:13PM +0200, Maarten Lankhorst wrote:
> > Op 21-09-18 om 21:31 schreef Ville Syrjälä:
> > > 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.lankho...@linux.intel.com>
> > >>>
> > >>> fixup Y/UV Linkage
> > >>>
> > >>> Signed-off-by: Maarten Lankhorst <maarten.lankho...@linux.intel.com>
> > >>> ---
> > >>> 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.
> > I think this shouldn't happen, and WM is special. The only time where you
> > want to add more planes is before check_planes().
> > Otherwise you can't rerun any validation as required.
>
> You shouldn't need validation for eg. dpms on/off. I guess we currently
> do that although we shouldn't have to.
>
> >
> > I've now added a function icl_add_linked_planes helper that iterates over
> > all planes in
> > the state, and adds any linked planes to the transaction.
> >
> > This is run right before drm_atomic_helper_check_planes(), so we're sure
> > that all linked
> > planes are added, without doing it from intel_plane_atomic_check()
> >
> > WM will continue to do its own thing, since it's a design error IMO that it
> > even adds
> > planes to the state to begin with. :)
>
> It pretty much has to. The design error we have at the moment is not
> programming the watermarks from the update_plane()/disable_plane().
> That one I've attempted to fix in:
> git://github.com/vsyrjala/linux.git skl_plane_update_ddb_sequence
>
> And supposedly that one does fix bugs related to watermarks vs.
> plane updates.
Where did the bugs arise? Were we unsuccessful at actually evading the
vblank (leading to the planes and watermarks taking effect on different
vblanks) or is it something else?
Matt
>
> >
> > Which is the last option you mentioned below.
> >
> > > 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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
