On Tue, Sep 25, 2018 at 12:03:47PM +0200, Maarten Lankhorst wrote:
> Op 24-09-18 om 15:18 schreef Ville Syrjälä:
> > 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.
> We should, if we ever have 2 crtc's active and disable 1. Watermarks should be
> distributed over active planes only, which dpms toggle affects.
I think the rule we want aim for is .enable controls watermarks,
.active does not matter. That will guarantee that dpms on will never
fail on account of watermarks. The current way of doing things is
IMO just wrong and we should eventually fix it.
> Only way around setting
> plane_state->visible when plane is inactive and calculate minimum requirements, then
> calculate max requirements.
>
> We would have to fix up all of wm programming and plane programming to make it work.
>
> I don't think the extra complexity is worth the effort, tbh..
> >> 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.
>
> Yeah, fortunately it shouldn't affect this code much, should be easy to rebase. :)
> Though I would like to get rid of skl_next_plane_to_commit, ugh..
That thing is pretty much whole point of the series. It could of
course be written in a different way, but the idea of ddb
allocations driving the plane commit order is key.
>
> Which is probably a confirmation that the nv12 gen11 series isn't making plane programming
> that much more complicated, fortunately.
>
> I would really like to simplify the locking first at some point. It can't be good to sync write everything.
>
> each plane update now does:
>
> spin_lock()
> I915_WRITE_FW(...)
>
> POSTING_READ()
Posting reads we should just nuke I think.
> spin_unlock()
>
> Would be nice if we
Cut off, but I'll assume you wanted to suggest grabbing the lock just
once for the entire pipe commit. I do agree that it would probably be
nicer. But I still maintain that splitting the lock to display engine
vs. "the rest" is probably something we want to do as well. The
annoying thing with that was that I couldn't trick the compiler select
the right lock at compile time, so I think this would involve splitting
I915_READ/WRITE & co. for display engine vs. the rest as well. I have
a feeling I even typed up something like that at one point, just never
had the time to test it.
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
