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

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.

> +
> +		if (WARN_ON(!old_linked_state))
> +			return -EINVAL;
> +
> +		crtc = to_intel_crtc(old_linked_state->base.crtc);
> +		if (WARN_ON(!crtc))
> +			return -EINVAL;
> +	}
> +
> +	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);
>  
> -	return intel_plane_atomic_check_with_state(to_intel_crtc_state(old_crtc_state),
> -						   to_intel_crtc_state(new_crtc_state),
> -						   to_intel_plane_state(old_plane_state),
> -						   to_intel_plane_state(new_plane_state));
> +	return intel_plane_atomic_check_with_state(old_crtc_state, new_crtc_state,
> +						   old_plane_state, new_plane_state);
>  }
>  
>  void intel_update_planes_on_crtc(struct intel_atomic_state *old_state,
> @@ -188,6 +209,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 8e1b3677e131..cbb3fb1d5ad4 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -10721,6 +10721,80 @@ 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);
> +	}
> +
> +	return 0;
> +}
> +
> +static int icl_check_nv12_planes(struct drm_i915_private *dev_priv,
> +				 struct intel_crtc *crtc,
> +				 struct intel_crtc_state *crtc_state)

Could pass just the crtc_state and dig the rest out here.

> +{
> +	struct intel_atomic_state *state = to_intel_atomic_state(crtc_state->base.state);
> +	struct intel_plane *plane, *aux;
> +
> +	if (INTEL_GEN(dev_priv) < 11 || !crtc_state->nv12_planes)
> +		return 0;
> +
> +	for_each_intel_plane_on_crtc(&dev_priv->drm, crtc, plane) {
> +		struct intel_plane_state *plane_state, *aux_state;
> +		struct drm_plane_state *drm_aux_state = NULL;

Still don't like the 'aux' in these names.
Just linked/linked_plane_state' to match the rest of the
code?

> +
> +		if (!(crtc_state->nv12_planes & BIT(plane->id)))
> +			continue;
> +
> +		plane_state = intel_atomic_get_new_plane_state(state, plane);
> +		if (!plane_state)
> +			continue;
> +
> +		for_each_intel_plane_on_crtc(&dev_priv->drm, crtc, aux) {
> +			if (!icl_is_nv12_y_plane(aux->id))
> +				continue;
> +
> +			if (crtc_state->active_planes & BIT(aux->id))
> +				continue;
> +
> +			drm_aux_state = drm_atomic_get_plane_state(&state->base, &aux->base);

intel_atomic_get_plane() and get rid of the aliasing states?

> +			if (IS_ERR(drm_aux_state))
> +				return PTR_ERR(drm_aux_state);
> +
> +			break;
> +		}
> +
> +		if (!drm_aux_state) {
> +			DRM_DEBUG_KMS("Need %d free Y planes for NV12\n",
> +				      hweight8(crtc_state->nv12_planes));
> +
> +			return -EINVAL;
> +		}
> +
> +		plane_state->linked_plane = aux;
> +
> +		aux_state = to_intel_plane_state(drm_aux_state);
> +		aux_state->slave = true;
> +		aux_state->linked_plane = plane;
> +		crtc_state->active_planes |= BIT(aux->id);
> +		DRM_DEBUG_KMS("Using %s as Y plane for %s\n", aux->base.name, plane->base.name);
> +	}
> +
> +	return 0;
> +}
> +
>  static int intel_crtc_atomic_check(struct drm_crtc *crtc,
>  				   struct drm_crtc_state *crtc_state)
>  {
> @@ -10792,6 +10866,9 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc,
>  		if (mode_changed)
>  			ret = skl_update_scaler_crtc(pipe_config);
>  
> +		if (!ret)
> +			ret = icl_check_nv12_planes(dev_priv, intel_crtc,
> +						    pipe_config);
>  		if (!ret)
>  			ret = skl_check_pipe_max_pixel_rate(intel_crtc,
>  							    pipe_config);
> @@ -12458,6 +12535,10 @@ static int intel_atomic_check(struct drm_device *dev,
>  		intel_state->cdclk.logical = dev_priv->cdclk.logical;
>  	}
>  
> +	ret = icl_add_linked_planes(intel_state);
> +	if (ret)
> +		return ret;
> +
>  	ret = drm_atomic_helper_check_planes(dev, state);
>  	if (ret)
>  		return ret;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index b5d6f6887c13..272de906a001 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -539,6 +539,26 @@ struct intel_plane_state {
>  	 */
>  	int scaler_id;
>  
> +	/*
> +	 * linked_plane:
> +	 *
> +	 * ICL planar formats require 2 planes that are updated as pairs.
> +	 * This member is used to make sure the other plane is also updated
> +	 * when required, and for update_slave() to find the correct
> +	 * plane_state to pass as argument.
> +	 */
> +	struct intel_plane *linked_plane;
> +
> +	/*
> +	 * slave:
> +	 * If set don't update use the linked plane's state for updating
> +	 * this plane during atomic commit with the update_slave() callback.
> +	 *
> +	 * It's also used by the watermark code to ignore wm calculations on
> +	 * this plane. They're calculated by the linked plane's wm code.
> +	 */
> +	bool slave;
> +
>  	struct drm_intel_sprite_colorkey ckey;
>  };
>  
> @@ -983,6 +1003,9 @@ struct intel_plane {
>  	void (*update_plane)(struct intel_plane *plane,
>  			     const struct intel_crtc_state *crtc_state,
>  			     const struct intel_plane_state *plane_state);
> +	void (*update_slave)(struct intel_plane *plane,
> +			     const struct intel_crtc_state *crtc_state,
> +			     const struct intel_plane_state *plane_state);
>  	void (*disable_plane)(struct intel_plane *plane,
>  			      struct intel_crtc *crtc);
>  	bool (*get_hw_state)(struct intel_plane *plane, enum pipe *pipe);
> @@ -1351,6 +1374,27 @@ hdmi_to_dig_port(struct intel_hdmi *intel_hdmi)
>  	return container_of(intel_hdmi, struct intel_digital_port, hdmi);
>  }
>  
> +static inline struct intel_plane_state *
> +intel_atomic_get_plane_state(struct intel_atomic_state *state,
> +				 struct intel_plane *plane)
> +{
> +	struct drm_plane_state *ret =
> +		drm_atomic_get_plane_state(&state->base, &plane->base);
> +
> +	if (IS_ERR(ret))
> +		return ERR_CAST(ret);
> +
> +	return to_intel_plane_state(ret);
> +}
> +
> +static inline struct intel_plane_state *
> +intel_atomic_get_old_plane_state(struct intel_atomic_state *state,
> +				 struct intel_plane *plane)
> +{
> +	return to_intel_plane_state(drm_atomic_get_old_plane_state(&state->base,
> +								   &plane->base));
> +}
> +
>  static inline struct intel_plane_state *
>  intel_atomic_get_new_plane_state(struct intel_atomic_state *state,
>  				 struct intel_plane *plane)
> @@ -2158,6 +2202,15 @@ struct intel_plane *
>  skl_universal_plane_create(struct drm_i915_private *dev_priv,
>  			   enum pipe pipe, enum plane_id plane_id);
>  
> +static inline bool icl_is_nv12_y_plane(enum plane_id id)
> +{
> +	/* Don't need to do a gen check, these planes are only available on gen11 */
> +	if (id == PLANE_SPRITE4 || id == PLANE_SPRITE5)
> +		return true;
> +
> +	return false;
> +}
> +
>  /* intel_tv.c */
>  void intel_tv_init(struct drm_i915_private *dev_priv);
>  
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 3b136fdfd24f..d003c08bd9e4 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -5153,11 +5153,12 @@ skl_ddb_add_affected_planes(struct intel_crtc_state *cstate)
>  	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
>  	struct skl_ddb_allocation *new_ddb = &intel_state->wm_results.ddb;
>  	struct skl_ddb_allocation *cur_ddb = &dev_priv->wm.skl_hw.ddb;
> -	struct drm_plane_state *plane_state;
>  	struct drm_plane *plane;
>  	enum pipe pipe = intel_crtc->pipe;
>  
>  	drm_for_each_plane_mask(plane, dev, cstate->base.plane_mask) {
> +		struct drm_plane_state *plane_state;
> +		struct intel_plane *linked;
>  		enum plane_id plane_id = to_intel_plane(plane)->id;
>  
>  		if (skl_ddb_entry_equal(&cur_ddb->plane[pipe][plane_id],
> @@ -5169,6 +5170,15 @@ skl_ddb_add_affected_planes(struct intel_crtc_state *cstate)
>  		plane_state = drm_atomic_get_plane_state(state, plane);
>  		if (IS_ERR(plane_state))
>  			return PTR_ERR(plane_state);
> +
> +		/* Make sure linked plane is updated too */
> +		linked = to_intel_plane_state(plane_state)->linked_plane;
> +		if (!linked)
> +			continue;
> +
> +		plane_state = drm_atomic_get_plane_state(state, &linked->base);
> +		if (IS_ERR(plane_state))
> +			return PTR_ERR(plane_state);
>  	}
>  
>  	return 0;
> -- 
> 2.19.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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