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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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.

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