On Tue, Sep 25, 2018 at 11:01:32AM -0700, Matt Roper wrote:
> 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.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.
> > > 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?
There are a few issues here:
- write to any plane registers apart from SURF will cancel an already
pending plane update. Well, it's not 100% cancelled as some registers
aren't part of the SURF based arming mechanism but IIRC they still
cause a cancellation. This means doing a watermark update before a
pending plane update was latched cancels most of the plane update.
This at least caused the cursor to remain on in when it should have
been turned off.
- overlapping DDB allocations can hard hang the box. So any vblank that
sneaks in while we've partiially reprogrammed the ddb could be a
death sentence. A suggested solution was to turn off the planes
before ddb reprogramming but that didn't work out so well when I
tried it due to the whole cancellation thing.
So I pulled in the ddb/wm programming into the normal plane update.
That means no more accidental cancellations from elsewhere.
And to avoid any ddb overlaps we simply sequence the plane updates
carefully. It's pretty much the same algorithm that we use to avoid
ddb overlaps between pipes, with the exception that we don't need the
vblank waits. So if a vblank does sneak at any point during the
sequence we can be assured that the partially latched state does not
have any overlapping ddb allocations.
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
