On Tue, Sep 25, 2018 at 09:34:29PM +0300, Ville Syrjälä wrote:
> 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.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?
>
> 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.
Wow, I think I've run into this problem before, but never figured out
what the exact root cause was. I tried to write an IGT to capture it a
while back, but the behavior went away with my simpler tests, probably
because I wasn't changing enough settings to trigger the necessary
watermark updates.
The behavior was puzzling since some of the plane updates actually would
take effect (e.g., PLANE_POS being zeroed would move the plane back to
0,0), but others wouldn't (most notable the enable/disable bit in
PLANE_CTL). So the result might be a garbage rectangle in the upper
left corner of the screen or a storm of GTT page faults that would bring
make the system unusable.
Is this actually expected behavior that's documented somewhere in the
bspec, or is it just something you've discovered through
experimentation? I couldn't find any explanation for updates getting
partially unarmed when I went through the bspec a while back, but I may
have overlooked something.
> - 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.
Sounds reasonable. Do you think we should try to land your work before
Maarten's gen11 NV12 patches?
Matt
>
> --
> Ville Syrjälä
> Intel
--
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
