[Intel-gfx] [PATCH 2/7] drm/i915/gen11: Link nv12 Y and UV planes in the atomic state, v3.

Mon Sep 24 12:35:13 UTC 2018

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.

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.
>