Re: [PATCH v2 3/8] drm/i915/gen11: Link nv12 Y and UV planes in the atomic state, v4.

[Maarten Lankhorst <maarten.lankhorst@linux.intel.com>]

Op 19-10-18 om 19:14 schreef Ville Syrjälä:
> On Fri, Oct 19, 2018 at 04:22:29PM +0200, Maarten Lankhorst wrote:
>> Op 18-10-18 om 18:00 schreef Ville Syrjälä:
>>> On Thu, Oct 18, 2018 at 01:51:29PM +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.
>>>> Changes since v3:
>>>> - Fix checkpatch warning about to_intel_crtc() usage.
>>>> - Add affected planes from icl_add_linked_planes() before check_planes(),
>>>>   it's a cleaner way to do this. (Ville)
>>>>
>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>> ---
>>>>  drivers/gpu/drm/i915/intel_atomic_plane.c | 74 ++++++++++++++++-----
>>>>  drivers/gpu/drm/i915/intel_display.c      | 81 +++++++++++++++++++++++
>>>>  drivers/gpu/drm/i915/intel_drv.h          | 53 +++++++++++++++
>>>>  drivers/gpu/drm/i915/intel_pm.c           | 12 +++-
>>>>  4 files changed, 202 insertions(+), 18 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
>>>> index b957ad63cd87..154ea3dc344f 100644
>>>> --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
>>>> +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
>>>> @@ -122,7 +122,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;
>>>>  
>>>> @@ -143,27 +147,44 @@ 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)
>>> My new cunning plane is to call these _plane, _new_plane_state etc.
>>> It should discourage people from using them and the aliasing
>>> intel_ types at the same time. And it avoids polluting the namespace
>>> for things we don't really want to use. 
>>>
>>> I already snuck in some uses of this ;)
>>>
>>>>  {
>>>> -	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;
>>>> +
>>>> +	if (linked && !crtc) {
>>>> +		const struct intel_plane_state *old_linked_state =
>>>> +			intel_atomic_get_old_plane_state(state, linked);
>>> Plane without a crtc that happens to have a linked plane
>>> attached to it...
>>>
>>> I guess that implies that 'plane' here is the slave and
>>> it was already active during the previous state (otherwise
>>> it would not have been linked to the other plane). So that
>>> means the master plane must have a valid crtc in its old
>>> plane state.
>>>
>>> Did I decode that correctly?
>> Correct.
>>> Maybe what we want to do here is to just always clear the
>>> active_planes bit for the slave in the master plane's
>>> old crtc's new crtc state (quite the mouthful), and then
>>> run through the normal check_plane stuff for the slave
>>> with its own crtc (if it has one). In practice it doesn't
>>> really make any difference I suppose since our planes
>>> can't move between crtcs, but logically it would make
>>> more sense to me.
>> Even if we could move planes,  we can't move planes between CRTC's in a single atomic commit.
>> First comes the disabling, then comes the moving.
>>
>> I think it's less of a mess of doing it this way, it keeps intel_plane_atomic_check_with_state() obvious.
> It's rather non-obvious. I had to think about it for a while.
>
> I think it would be much clearer to keep it all in one self
> contained function instead of spreading it across several
> functions.
>
> remove_plane_link()
> {
> 	if (plane_state->linked && plane_state->slave) {
> 		old_linked_state = get_old_plane_state(plane_state->linked);
> 		new_crtc_state = get_new_crtc_state(old_linked_state->crtc);
> 		new_crtc_state->active_planes &= ~BIT(plane->id);
> 	}
>
> 	plane_state->linked = NULL;
> 	plane_state->slave = false;
> }
>
> or something.
>
Ok, v5 coming up, bringing much more clarity. :)

It turns out to be easier and cleaner to remove the plane link as
a separate pass in icl_check_nv12_planes() before restoring new links.

~Maarten

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx