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 <[email protected]> >>> >>> fixup Y/UV Linkage >>> >>> Signed-off-by: Maarten Lankhorst <[email protected]> >>> --- >>> 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.
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. :) 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. > _______________________________________________ Intel-gfx mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/intel-gfx
