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

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

On Tue, Oct 23, 2018 at 04:25:14PM +0200, Maarten Lankhorst wrote:
> Op 22-10-18 om 17:48 schreef Ville Syrjälä:
> > On Mon, Oct 22, 2018 at 03:51:52PM +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)
> >> Changes since v4:
> >> - Clear plane links in icl_check_nv12_planes() for clarity.
> >> - Only pass crtc_state to icl_check_nv12_planes().
> >> - Use for_each_new_intel_plane_in_state() in icl_check_nv12_planes.
> >> - Rename aux to linked. (Ville)
> >>
> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/intel_atomic_plane.c | 19 +++++
> >>  drivers/gpu/drm/i915/intel_display.c      | 95 +++++++++++++++++++++++
> >>  drivers/gpu/drm/i915/intel_drv.h          | 53 +++++++++++++
> >>  drivers/gpu/drm/i915/intel_pm.c           | 12 ++-
> >>  4 files changed, 178 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
> >> index b957ad63cd87..7d3685075201 100644
> >> --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
> >> +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
> >> @@ -188,6 +188,25 @@ void intel_update_planes_on_crtc(struct intel_atomic_state *old_state,
> >>  			trace_intel_update_plane(&plane->base, crtc);
> >>  
> >>  			plane->update_plane(plane, new_crtc_state, new_plane_state);
> >> +		} else if (new_plane_state->slave) {
> >> +			struct intel_plane *master =
> >> +				new_plane_state->linked_plane;
> >> +
> >> +			/*
> >> +			 * We update the slave plane from this function because
> >> +			 * programming it from the master plane's update_plane
> >> +			 * callback runs into issues when the Y plane is
> >> +			 * reassigned, disabled or used by a different plane.
> >> +			 *
> >> +			 * The slave plane is updated with the master plane's
> >> +			 * plane_state.
> >> +			 */
> >> +			new_plane_state =
> >> +				intel_atomic_get_new_plane_state(old_state, master);
> >> +
> >> +			trace_intel_update_plane(&plane->base, crtc);
> >> +
> >> +			plane->update_slave(plane, new_crtc_state, new_plane_state);
> >>  		} else {
> >>  			trace_intel_disable_plane(&plane->base, crtc);
> >>  
> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >> index 68c9aeeee05c..680895982e4f 100644
> >> --- a/drivers/gpu/drm/i915/intel_display.c
> >> +++ b/drivers/gpu/drm/i915/intel_display.c
> >> @@ -10723,6 +10723,95 @@ static bool check_single_encoder_cloning(struct drm_atomic_state *state,
> >>  	return true;
> >>  }
> >>  
> >> +static int icl_add_linked_planes(struct intel_atomic_state *state)
> >> +{
> >> +	struct intel_plane *plane, *linked;
> >> +	struct intel_plane_state *plane_state, *linked_plane_state;
> >> +	int i;
> >> +
> >> +	for_each_new_intel_plane_in_state(state, plane, plane_state, i) {
> >> +		linked = plane_state->linked_plane;
> >> +
> >> +		if (!linked)
> >> +			continue;
> >> +
> >> +		linked_plane_state = intel_atomic_get_plane_state(state, linked);
> >> +		if (IS_ERR(linked_plane_state))
> >> +			return PTR_ERR(linked_plane_state);
> > WARN_ON(linked_plane_state->linked != plane);
> > WARN_ON(linked_plane_state->slave == plane_state->slave);
> >
> > perhaps?
> Those will trigger because linked_plane_state might already have been cleared. :)

OK, I'm totally confused now. Who cleared them? And why did they clear them
only half way?

> 
> Thanks for the reviews, tested on fi-icl-u and seems the smalll changes didn't break stuff, so will push it in an hour.
> 
> ~Maarten

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