Re: [PATCH 4/7] drm/i915: Pass proper old/new states to intel_plane_atomic_check_with_state()

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

On Wed, Aug 23, 2017 at 06:22:23PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Eliminate plane->state and crtc->state usage from
> intel_plane_atomic_check_with_state() and its callers. Instead pass the
> proper states in or dig them up from the top level atomic state.
> 
> Note that intel_plane_atomic_check_with_state() itself isn't allowed to
> use the top level atomic state as there is none when it gets called from
> the legacy cursor short circuit path.
> 
> v2: Rename some variables for easier comprehension (Maarten)
> 
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

This patch would still need some review love... In the meantime I
pushed the first three patches from the series. Thanks for the reviews
thus far.

> ---
>  drivers/gpu/drm/i915/intel_atomic_plane.c | 49 +++++++++++++++++++------------
>  drivers/gpu/drm/i915/intel_display.c      | 12 ++++----
>  drivers/gpu/drm/i915/intel_drv.h          | 16 ++++++++--
>  3 files changed, 51 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
> index ee76fab7bb6f..8e6dc159f64d 100644
> --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
> +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
> @@ -107,7 +107,9 @@ intel_plane_destroy_state(struct drm_plane *plane,
>  	drm_atomic_helper_plane_destroy_state(plane, state);
>  }
>  
> -int intel_plane_atomic_check_with_state(struct intel_crtc_state *crtc_state,
> +int intel_plane_atomic_check_with_state(const struct intel_crtc_state *old_crtc_state,
> +					struct intel_crtc_state *crtc_state,
> +					const struct intel_plane_state *old_plane_state,
>  					struct intel_plane_state *intel_state)
>  {
>  	struct drm_plane *plane = intel_state->base.plane;
> @@ -124,7 +126,7 @@ int intel_plane_atomic_check_with_state(struct intel_crtc_state *crtc_state,
>  	 * anything driver-specific we need to test in that case, so
>  	 * just return success.
>  	 */
> -	if (!intel_state->base.crtc && !plane->state->crtc)
> +	if (!intel_state->base.crtc && !old_plane_state->base.crtc)
>  		return 0;
>  
>  	/* Clip all planes to CRTC size, or 0x0 if CRTC is disabled */
> @@ -194,16 +196,21 @@ int intel_plane_atomic_check_with_state(struct intel_crtc_state *crtc_state,
>  	else
>  		crtc_state->active_planes &= ~BIT(intel_plane->id);
>  
> -	return intel_plane_atomic_calc_changes(&crtc_state->base, state);
> +	return intel_plane_atomic_calc_changes(old_crtc_state,
> +					       &crtc_state->base,
> +					       old_plane_state,
> +					       state);
>  }
>  
>  static int intel_plane_atomic_check(struct drm_plane *plane,
> -				    struct drm_plane_state *state)
> +				    struct drm_plane_state *new_plane_state)
>  {
> -	struct drm_crtc *crtc = state->crtc;
> -	struct drm_crtc_state *drm_crtc_state;
> -
> -	crtc = crtc ? crtc : plane->state->crtc;
> +	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;
>  
>  	/*
>  	 * Both crtc and plane->crtc could be NULL if we're updating a
> @@ -214,29 +221,33 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
>  	if (!crtc)
>  		return 0;
>  
> -	drm_crtc_state = drm_atomic_get_existing_crtc_state(state->state, crtc);
> -	if (WARN_ON(!drm_crtc_state))
> -		return -EINVAL;
> +	old_crtc_state = drm_atomic_get_old_crtc_state(state, crtc);
> +	new_crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
>  
> -	return intel_plane_atomic_check_with_state(to_intel_crtc_state(drm_crtc_state),
> -						   to_intel_plane_state(state));
> +	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));
>  }
>  
>  static void intel_plane_atomic_update(struct drm_plane *plane,
>  				      struct drm_plane_state *old_state)
>  {
> +	struct intel_atomic_state *state = to_intel_atomic_state(old_state->state);
>  	struct intel_plane *intel_plane = to_intel_plane(plane);
> -	struct intel_plane_state *intel_state =
> -		to_intel_plane_state(plane->state);
> -	struct drm_crtc *crtc = plane->state->crtc ?: old_state->crtc;
> +	const struct intel_plane_state *new_plane_state =
> +		intel_atomic_get_new_plane_state(state, intel_plane);
> +	struct drm_crtc *crtc = new_plane_state->base.crtc ?: old_state->crtc;
> +
> +	if (new_plane_state->base.visible) {
> +		const struct intel_crtc_state *new_crtc_state =
> +			intel_atomic_get_new_crtc_state(state, to_intel_crtc(crtc));
>  
> -	if (intel_state->base.visible) {
>  		trace_intel_update_plane(plane,
>  					 to_intel_crtc(crtc));
>  
>  		intel_plane->update_plane(intel_plane,
> -					  to_intel_crtc_state(crtc->state),
> -					  intel_state);
> +					  new_crtc_state, new_plane_state);
>  	} else {
>  		trace_intel_disable_plane(plane,
>  					  to_intel_crtc(crtc));
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index cee4db0a1626..50617f390dce 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -10317,7 +10317,7 @@ static bool intel_wm_need_update(struct drm_plane *plane,
>  	return false;
>  }
>  
> -static bool needs_scaling(struct intel_plane_state *state)
> +static bool needs_scaling(const struct intel_plane_state *state)
>  {
>  	int src_w = drm_rect_width(&state->base.src) >> 16;
>  	int src_h = drm_rect_height(&state->base.src) >> 16;
> @@ -10327,7 +10327,9 @@ static bool needs_scaling(struct intel_plane_state *state)
>  	return (src_w != dst_w || src_h != dst_h);
>  }
>  
> -int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
> +int intel_plane_atomic_calc_changes(const struct intel_crtc_state *old_crtc_state,
> +				    struct drm_crtc_state *crtc_state,
> +				    const struct intel_plane_state *old_plane_state,
>  				    struct drm_plane_state *plane_state)
>  {
>  	struct intel_crtc_state *pipe_config = to_intel_crtc_state(crtc_state);
> @@ -10336,10 +10338,8 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
>  	struct intel_plane *plane = to_intel_plane(plane_state->plane);
>  	struct drm_device *dev = crtc->dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
> -	struct intel_plane_state *old_plane_state =
> -		to_intel_plane_state(plane->base.state);
>  	bool mode_changed = needs_modeset(crtc_state);
> -	bool was_crtc_enabled = crtc->state->active;
> +	bool was_crtc_enabled = old_crtc_state->base.active;
>  	bool is_crtc_enabled = crtc_state->active;
>  	bool turn_off, turn_on, visible, was_visible;
>  	struct drm_framebuffer *fb = plane_state->fb;
> @@ -13137,6 +13137,8 @@ intel_legacy_cursor_update(struct drm_plane *plane,
>  	new_plane_state->crtc_h = crtc_h;
>  
>  	ret = intel_plane_atomic_check_with_state(to_intel_crtc_state(crtc->state),
> +						  to_intel_crtc_state(crtc->state), /* FIXME need a new crtc state? */
> +						  to_intel_plane_state(plane->state),
>  						  to_intel_plane_state(new_plane_state));
>  	if (ret)
>  		goto out_free;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index e2e9b8215864..6a6b2567ed0a 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1190,6 +1190,14 @@ 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_new_plane_state(struct intel_atomic_state *state,
> +				 struct intel_plane *plane)
> +{
> +	return to_intel_plane_state(drm_atomic_get_new_plane_state(&state->base,
> +								   &plane->base));
> +}
> +
>  static inline struct intel_crtc_state *
>  intel_atomic_get_old_crtc_state(struct intel_atomic_state *state,
>  				struct intel_crtc *crtc)
> @@ -1418,7 +1426,9 @@ int intel_plane_atomic_set_property(struct drm_plane *plane,
>  				    struct drm_plane_state *state,
>  				    struct drm_property *property,
>  				    uint64_t val);
> -int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
> +int intel_plane_atomic_calc_changes(const struct intel_crtc_state *old_crtc_state,
> +				    struct drm_crtc_state *crtc_state,
> +				    const struct intel_plane_state *old_plane_state,
>  				    struct drm_plane_state *plane_state);
>  
>  void assert_pch_transcoder_disabled(struct drm_i915_private *dev_priv,
> @@ -1977,7 +1987,9 @@ struct drm_plane_state *intel_plane_duplicate_state(struct drm_plane *plane);
>  void intel_plane_destroy_state(struct drm_plane *plane,
>  			       struct drm_plane_state *state);
>  extern const struct drm_plane_helper_funcs intel_plane_helper_funcs;
> -int intel_plane_atomic_check_with_state(struct intel_crtc_state *crtc_state,
> +int intel_plane_atomic_check_with_state(const struct intel_crtc_state *old_crtc_state,
> +					struct intel_crtc_state *crtc_state,
> +					const struct intel_plane_state *old_plane_state,
>  					struct intel_plane_state *intel_state);
>  
>  /* intel_color.c */
> -- 
> 2.13.0

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