Re: [PATCH v2 07/16] drm/i915/gen9: Allow skl_allocate_pipe_ddb() to operate on in-flight state (v2)

[Maarten Lankhorst <maarten.lankhorst@linux.intel.com>]

Hey,

Op 20-04-16 om 04:26 schreef Matt Roper:
> We eventually want to calculate watermark values at atomic 'check' time
> instead of atomic 'commit' time so that any requested configurations
> that result in impossible watermark requirements are properly rejected.
> The first step along this path is to allocate the DDB at atomic 'check'
> time.  As we perform this transition, allow the main allocation function
> to operate successfully on either an in-flight state or an
> already-commited state.  Once we complete the transition in a future
> patch, we'll come back and remove the unnecessary logic for the
> already-committed case.
>
> v2: Rebase/refactor; we should no longer need to grab extra plane states
>     while allocating the DDB since we can pull cached data rates and
>     minimum block counts from the CRTC state for any planes that aren't
>     being modified by this transaction.
>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h |   6 ++
>  drivers/gpu/drm/i915/intel_pm.c | 179 +++++++++++++++++++++++++++++-----------
>  2 files changed, 139 insertions(+), 46 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 85102ad..e91d39d 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -324,6 +324,12 @@ struct i915_hotplug {
>  			    &dev->mode_config.plane_list,	\
>  			    base.head)
>  
> +#define for_each_intel_plane_mask(dev, intel_plane, plane_mask)		\
> +	list_for_each_entry(intel_plane, &dev->mode_config.plane_list,	\
> +			    base.head)					\
> +		for_each_if ((plane_mask) &				\
> +			     (1 << drm_plane_index(&intel_plane->base)))
> +
>  #define for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane)	\
>  	list_for_each_entry(intel_plane,				\
>  			    &(dev)->mode_config.plane_list,		\
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 479a890..640305a 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -2849,13 +2849,18 @@ skl_wm_plane_id(const struct intel_plane *plane)
>  static void
>  skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
>  				   const struct intel_crtc_state *cstate,
> -				   const struct intel_wm_config *config,
> -				   struct skl_ddb_entry *alloc /* out */)
> +				   struct intel_wm_config *config,
> +				   struct skl_ddb_entry *alloc, /* out */
> +				   int *num_active /* out */)
>  {
> +	struct drm_atomic_state *state = cstate->base.state;
> +	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
> +	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct drm_crtc *for_crtc = cstate->base.crtc;
>  	struct drm_crtc *crtc;
>  	unsigned int pipe_size, ddb_size;
>  	int nth_active_pipe;
> +	int pipe = to_intel_crtc(for_crtc)->pipe;
>  
>  	if (!cstate->base.active) {
>  		alloc->start = 0;
> @@ -2870,25 +2875,59 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
>  
>  	ddb_size -= 4; /* 4 blocks for bypass path allocation */
>  
> -	nth_active_pipe = 0;
> -	for_each_crtc(dev, crtc) {
> -		if (!to_intel_crtc(crtc)->active)
> -			continue;
> +	/*
> +	 * FIXME: At the moment we may be called on either in-flight or fully
> +	 * committed cstate's.  Once we fully move DDB allocation in the check
> +	 * phase, we'll only be called on in-flight states and the 'else'
> +	 * branch here will go away.
> +	 *
> +	 * The 'else' branch is slightly racy here, but it was racy to begin
> +	 * with; since it's going away soon, no effort is made to address that.
> +	 */
> +	if (state) {
> +		/*
> +		 * If the state doesn't change the active CRTC's, then there's
> +		 * no need to recalculate; the existing pipe allocation limits
> +		 * should remain unchanged.  Note that we're safe from racing
> +		 * commits since any racing commit that changes the active CRTC
> +		 * list would need to grab _all_ crtc locks, including the one
> +		 * we currently hold.
> +		 */
> +		if (!intel_state->active_pipe_changes) {
> +			*alloc = dev_priv->wm.skl_hw.ddb.pipe[pipe];
> +			*num_active = hweight32(dev_priv->active_crtcs);
> +			return;
> +		}
>  
> -		if (crtc == for_crtc)
> -			break;
> +		*num_active = hweight32(intel_state->active_crtcs);
> +		nth_active_pipe = hweight32(intel_state->active_crtcs &
> +					    (drm_crtc_mask(for_crtc) - 1));
> +		pipe_size = ddb_size / hweight32(intel_state->active_crtcs);
> +		alloc->start = nth_active_pipe * ddb_size / *num_active;
> +		alloc->end = alloc->start + pipe_size;
> +	} else {
> +		nth_active_pipe = 0;
> +		for_each_crtc(dev, crtc) {
> +			if (!to_intel_crtc(crtc)->active)
> +				continue;
>  
> -		nth_active_pipe++;
> -	}
> +			if (crtc == for_crtc)
> +				break;
>  
> -	pipe_size = ddb_size / config->num_pipes_active;
> -	alloc->start = nth_active_pipe * ddb_size / config->num_pipes_active;
> -	alloc->end = alloc->start + pipe_size;
> +			nth_active_pipe++;
> +		}
> +
> +		pipe_size = ddb_size / config->num_pipes_active;
> +		alloc->start = nth_active_pipe * ddb_size /
> +			config->num_pipes_active;
> +		alloc->end = alloc->start + pipe_size;
> +		*num_active = config->num_pipes_active;
> +	}
>  }
>  
> -static unsigned int skl_cursor_allocation(const struct intel_wm_config *config)
> +static unsigned int skl_cursor_allocation(int num_active)
>  {
> -	if (config->num_pipes_active == 1)
> +	if (num_active == 1)
>  		return 32;
>  
>  	return 8;
> @@ -3054,33 +3093,48 @@ skl_get_total_relative_data_rate(struct intel_crtc_state *intel_cstate)
>  	return total_data_rate;
>  }
>  
> -static void
> +static int
>  skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
>  		      struct skl_ddb_allocation *ddb /* out */)
>  {
> +	struct drm_atomic_state *state = cstate->base.state;
>  	struct drm_crtc *crtc = cstate->base.crtc;
>  	struct drm_device *dev = crtc->dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct intel_wm_config *config = &dev_priv->wm.config;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	struct intel_plane *intel_plane;
> +	struct drm_plane *plane;
> +	struct drm_plane_state *pstate;
>  	enum pipe pipe = intel_crtc->pipe;
>  	struct skl_ddb_entry *alloc = &ddb->pipe[pipe];
>  	uint16_t alloc_size, start, cursor_blocks;
>  	uint16_t *minimum = cstate->wm.skl.minimum_blocks;
>  	uint16_t *y_minimum = cstate->wm.skl.minimum_y_blocks;
>  	unsigned int total_data_rate;
> +	int num_active;
> +	int id, i;
> +
> +	if (!cstate->base.active) {
> +		ddb->pipe[pipe].start = ddb->pipe[pipe].end = 0;
> +		memset(ddb->plane[pipe], 0,
> +		       I915_MAX_PLANES * sizeof(struct skl_ddb_entry));
> +		memset(ddb->y_plane[pipe], 0,
> +		       I915_MAX_PLANES * sizeof(struct skl_ddb_entry));
> +		return 0;
>
^Since these are pointers to arrays, memset(x, 0, sizeof(x)); ?

Also there still seems to be a bogus memset for plane[pipe][PLANE_CURSOR], after plane[pipe] was just zero'd. Should probably be fixed while you're touching this function. :-)

~Maarten
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx