[Intel-gfx] [PATCH 2/7] drm/i915/gen11: Link nv12 Y and UV planes in the atomic state, v3.
Maarten Lankhorst
maarten.lankhorst at linux.intel.com
Tue Sep 25 10:03:47 UTC 2018
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 at linux.intel.com>
>>>>>
>>>>> fixup Y/UV Linkage
>>>>>
>>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst at 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.
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. 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..
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()
spin_unlock()
Would be nice if we
~Maarten
More information about the Intel-gfx
mailing list