Re: [PATCH 04/15] drm/i915: Replace call to commit_planes_on_crtc with internal update, v2.

[Matt Roper <matthew.d.roper@intel.com>]

On Wed, Sep 19, 2018 at 03:56:33PM +0200, Maarten Lankhorst wrote:
> drm_atomic_helper_commit_planes_on_crtc calls begin_commit,
> then plane_update hooks, then flush_commit. Because we keep our own
> visibility tracking through plane_state->visible there's no need to
> rely on the atomic hooks for this.
> 
> By explicitly writing our own helper, we can update visible planes
> as needed, which is useful to make NV12 support work as intended.
> 
> Changes since v1:
> - Reword commit message. (Matt Roper)
> - Rename to intel_update_planes_on_crtc(). (Matt)
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

Reviewed-by: Matt Roper <matthew.d.roper@intel.com>

This patch does leave drm_atomic_helper_commit_planes_on_crtc() unused
by any driver.  Not sure if we need to follow up with a patch to remove
it as dead code, or whether we anticipate other drivers wanting to use
it soon.


Matt

> ---
>  drivers/gpu/drm/i915/intel_atomic_plane.c | 41 ++++++++++++-----------
>  drivers/gpu/drm/i915/intel_display.c      | 10 ++++--
>  drivers/gpu/drm/i915/intel_drv.h          |  4 +++
>  3 files changed, 33 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
> index aabebe0d2e9b..e067fb531a29 100644
> --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
> +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
> @@ -164,29 +164,33 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
>  						   to_intel_plane_state(new_plane_state));
>  }
>  
> -static void intel_plane_atomic_update(struct drm_plane *plane,
> -				      struct drm_plane_state *old_state)
> +void intel_update_planes_on_crtc(struct intel_atomic_state *old_state,
> +				 struct intel_crtc *crtc,
> +				 struct intel_crtc_state *old_crtc_state,
> +				 struct intel_crtc_state *new_crtc_state)
>  {
> -	struct intel_atomic_state *state = to_intel_atomic_state(old_state->state);
> -	struct intel_plane *intel_plane = to_intel_plane(plane);
> -	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;
> +	struct intel_plane_state *new_plane_state;
> +	struct intel_plane *plane;
> +	u32 update_mask;
> +	int i;
> +
> +	update_mask = old_crtc_state->active_planes;
> +	update_mask |= new_crtc_state->active_planes;
>  
> -	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));
> +	for_each_new_intel_plane_in_state(old_state, plane, new_plane_state, i) {
> +		if (crtc->pipe != plane->pipe ||
> +		    !(update_mask & BIT(plane->id)))
> +			continue;
>  
> -		trace_intel_update_plane(plane,
> -					 to_intel_crtc(crtc));
> +		if (new_plane_state->base.visible) {
> +			trace_intel_update_plane(&plane->base, crtc);
>  
> -		intel_plane->update_plane(intel_plane,
> -					  new_crtc_state, new_plane_state);
> -	} else {
> -		trace_intel_disable_plane(plane,
> -					  to_intel_crtc(crtc));
> +			plane->update_plane(plane, new_crtc_state, new_plane_state);
> +		} else {
> +			trace_intel_disable_plane(&plane->base, crtc);
>  
> -		intel_plane->disable_plane(intel_plane, to_intel_crtc(crtc));
> +			plane->disable_plane(plane, crtc);
> +		}
>  	}
>  }
>  
> @@ -194,7 +198,6 @@ const struct drm_plane_helper_funcs intel_plane_helper_funcs = {
>  	.prepare_fb = intel_prepare_plane_fb,
>  	.cleanup_fb = intel_cleanup_plane_fb,
>  	.atomic_check = intel_plane_atomic_check,
> -	.atomic_update = intel_plane_atomic_update,
>  };
>  
>  /**
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index f7982f18b8fe..3975f3af4fb4 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -10813,8 +10813,6 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc,
>  }
>  
>  static const struct drm_crtc_helper_funcs intel_helper_funcs = {
> -	.atomic_begin = intel_begin_crtc_commit,
> -	.atomic_flush = intel_finish_crtc_commit,
>  	.atomic_check = intel_crtc_atomic_check,
>  };
>  
> @@ -12483,6 +12481,7 @@ static void intel_update_crtc(struct drm_crtc *crtc,
>  	struct drm_device *dev = crtc->dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	struct intel_crtc_state *old_intel_cstate = to_intel_crtc_state(old_crtc_state);
>  	struct intel_crtc_state *pipe_config = to_intel_crtc_state(new_crtc_state);
>  	bool modeset = needs_modeset(new_crtc_state);
>  	struct intel_plane_state *new_plane_state =
> @@ -12503,7 +12502,12 @@ static void intel_update_crtc(struct drm_crtc *crtc,
>  	if (new_plane_state)
>  		intel_fbc_enable(intel_crtc, pipe_config, new_plane_state);
>  
> -	drm_atomic_helper_commit_planes_on_crtc(old_crtc_state);
> +	intel_begin_crtc_commit(crtc, old_crtc_state);
> +
> +	intel_update_planes_on_crtc(to_intel_atomic_state(state), intel_crtc,
> +				    old_intel_cstate, pipe_config);
> +
> +	intel_finish_crtc_commit(crtc, old_crtc_state);
>  }
>  
>  static void intel_update_crtcs(struct drm_atomic_state *state)
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index bf1c38728a59..8073a85d7178 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -2188,6 +2188,10 @@ 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;
> +void intel_update_planes_on_crtc(struct intel_atomic_state *old_state,
> +				 struct intel_crtc *crtc,
> +				 struct intel_crtc_state *old_crtc_state,
> +				 struct intel_crtc_state *new_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,
> -- 
> 2.18.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx