[v8,4/4] drm/i915: Correctly map DBUF slices to pipes
diff mbox series

Message ID 20191213130228.29509-5-stanislav.lisovskiy@intel.com
State New
Headers show
Series
  • Enable second DBuf slice for ICL and TGL
Related show

Commit Message

Stanislav Lisovskiy Dec. 13, 2019, 1:02 p.m. UTC
Added proper DBuf slice mapping to correspondent
pipes, depending on pipe configuration as stated
in BSpec.

v2:
    - Remove unneeded braces
    - Stop using macro for DBuf assignments as
      it seems to reduce readability.

v3: Start using enabled slices mask in dev_priv

v4: Renamed "enabled_slices" used in dev_priv
    to "enabled_dbuf_slices_mask"(Matt Roper)

Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 226 ++++++++++++++++++++++++++++++--
 1 file changed, 216 insertions(+), 10 deletions(-)

Comments

Matt Roper Dec. 14, 2019, 12:52 a.m. UTC | #1
On Fri, Dec 13, 2019 at 03:02:28PM +0200, Stanislav Lisovskiy wrote:
> Added proper DBuf slice mapping to correspondent
> pipes, depending on pipe configuration as stated
> in BSpec.
> 
> v2:
>     - Remove unneeded braces
>     - Stop using macro for DBuf assignments as
>       it seems to reduce readability.
> 
> v3: Start using enabled slices mask in dev_priv
> 
> v4: Renamed "enabled_slices" used in dev_priv
>     to "enabled_dbuf_slices_mask"(Matt Roper)
> 
> Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 226 ++++++++++++++++++++++++++++++--
>  1 file changed, 216 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 111bcafd6e4c..a13052b2c2ef 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3832,13 +3832,30 @@ bool intel_can_enable_sagv(struct intel_atomic_state *state)
>  	return true;
>  }
>  
> +/*
> + * Calculate initial DBuf slice offset, based on slice size
> + * and mask(i.e if slice size is 1024 and second slice is enabled
> + * offset would be 1024)
> + */
> +static u32 icl_get_first_dbuf_slice_offset(u32 dbuf_slice_mask,
> +					   u32 slice_size, u32 ddb_size)
> +{

It might be worth just passing the mask + dev_priv to this function and
let it get slice_size / ddb_size on its own to keep the logic simpler at
the callsite.

> +	u32 offset = 0;
> +
> +	if (!dbuf_slice_mask)
> +		return 0;
> +
> +	offset = (ffs(dbuf_slice_mask) - 1) * slice_size;
> +
> +	WARN_ON(offset >= ddb_size);
> +	return offset;
> +}
> +
>  static u16 intel_get_ddb_size(struct drm_i915_private *dev_priv,
>  			      const struct intel_crtc_state *crtc_state,
>  			      const u64 total_data_rate,
>  			      const int num_active)

I probably should have mentioned it on the previous patch, but most of
these parameters are no longer needed now.

>  {
> -	struct drm_atomic_state *state = crtc_state->uapi.state;
> -	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
>  	u16 ddb_size = INTEL_INFO(dev_priv)->ddb_size;
>  
>  	WARN_ON(ddb_size == 0);
> @@ -3846,12 +3863,13 @@ static u16 intel_get_ddb_size(struct drm_i915_private *dev_priv,
>  	if (INTEL_GEN(dev_priv) < 11)
>  		return ddb_size - 4; /* 4 blocks for bypass path allocation */
>  
> -	intel_state->enabled_dbuf_slices_mask = DBUF_S1_BIT;
> -	ddb_size /= 2;
> -
>  	return ddb_size;
>  }
>  
> +u32 i915_possible_dbuf_slices(struct drm_i915_private *dev_priv,
> +			      int pipe, u32 active_pipes,
> +			      const struct intel_crtc_state *crtc_state);
> +
>  static void
>  skl_ddb_get_pipe_allocation_limits(struct drm_i915_private *dev_priv,
>  				   const struct intel_crtc_state *crtc_state,
> @@ -3866,7 +3884,14 @@ skl_ddb_get_pipe_allocation_limits(struct drm_i915_private *dev_priv,
>  	u32 pipe_width = 0, total_width = 0, width_before_pipe = 0;
>  	enum pipe for_pipe = to_intel_crtc(for_crtc)->pipe;
>  	u16 ddb_size;
> +	u32 ddb_range_size;
>  	u32 i;
> +	u32 dbuf_slice_mask;
> +	u32 active_pipes;
> +	u32 offset;
> +	u32 slice_size;
> +	u32 total_slice_mask;
> +	u32 start, end;
>  
>  	if (WARN_ON(!state) || !crtc_state->hw.active) {
>  		alloc->start = 0;
> @@ -3875,14 +3900,19 @@ skl_ddb_get_pipe_allocation_limits(struct drm_i915_private *dev_priv,
>  		return;
>  	}
>  
> -	if (intel_state->active_pipe_changes)
> +	if (intel_state->active_pipe_changes) {
>  		*num_active = hweight8(intel_state->active_pipes);
> -	else
> +		active_pipes = intel_state->active_pipes;
> +	} else {
>  		*num_active = hweight8(dev_priv->active_pipes);
> +		active_pipes = dev_priv->active_pipes;
> +	}

Might be slightly more intuitive to move the num_active assignment
outside the 'if' as

        *num_active = hweight8(active_pipes);

Up to you; doesn't really matter.

>  
>  	ddb_size = intel_get_ddb_size(dev_priv, crtc_state, total_data_rate,
>  				      *num_active);
>  
> +	slice_size = ddb_size / INTEL_INFO(dev_priv)->num_supported_dbuf_slices;
> +
>  	/*
>  	 * If the state doesn't change the active CRTC's or there is no
>  	 * modeset request, then there's no need to recalculate;
> @@ -3900,18 +3930,68 @@ skl_ddb_get_pipe_allocation_limits(struct drm_i915_private *dev_priv,
>  		return;
>  	}
>  
> +	/*
> +	 * Get allowed DBuf slices for correspondent pipe and platform.
> +	 */
> +	dbuf_slice_mask = i915_possible_dbuf_slices(dev_priv, for_pipe,
> +						    active_pipes, crtc_state);
> +
> +	DRM_DEBUG_KMS("DBuf slice mask %x pipe %d active pipes %x\n",
> +		      dbuf_slice_mask,
> +		      for_pipe, active_pipes);
> +
> +	/*
> +	 * Figure out at which DBuf slice we start, i.e if we start at Dbuf S2
> +	 * and slice size is 1024, the offset would be 1024
> +	 */
> +	offset = icl_get_first_dbuf_slice_offset(dbuf_slice_mask,
> +						 slice_size, ddb_size);
> +
> +	/*
> +	 * Figure out total size of allowed DBuf slices, which is basically
> +	 * a number of allowed slices for that pipe multiplied by slice size.
> +	 * Inside of this
> +	 * range ddb entries are still allocated in proportion to display width.
> +	 */
> +	ddb_range_size = hweight8(dbuf_slice_mask) * slice_size;
> +
>  	/*
>  	 * Watermark/ddb requirement highly depends upon width of the
>  	 * framebuffer, So instead of allocating DDB equally among pipes
>  	 * distribute DDB based on resolution/width of the display.
>  	 */
> +	total_slice_mask = dbuf_slice_mask;
>  	for_each_new_intel_crtc_in_state(intel_state, crtc, crtc_state, i) {
>  		const struct drm_display_mode *adjusted_mode =
>  			&crtc_state->hw.adjusted_mode;
>  		enum pipe pipe = crtc->pipe;
>  		int hdisplay, vdisplay;
> +		u32 pipe_dbuf_slice_mask =
> +					i915_possible_dbuf_slices(dev_priv,
> +								  pipe,
> +								  active_pipes,
> +								  crtc_state);
> +
> +		if (!crtc_state->hw.active)
> +			continue;

You might want to do this check before we go through the bother of
executing i915_possible_dbuf_slices.

> +
> +		/*
> +		 * According to BSpec pipe can share one dbuf slice with another
> +		 * pipes or pipe can use multiple dbufs, in both cases we
> +		 * account for other pipes only if they have exactly same mask.
> +		 * However we need to account how many slices we should enable
> +		 * in total.
> +		 */
> +		total_slice_mask |= pipe_dbuf_slice_mask;
>  
> -		if (!crtc_state->hw.enable)
> +		/*
> +		 * Do not account pipes using other slice sets
> +		 * luckily as of current BSpec slice sets do not partially
> +		 * intersect(pipes share either same one slice or same slice set
> +		 * i.e no partial intersection), so it is enough to check for
> +		 * equality for now.
> +		 */
> +		if (dbuf_slice_mask != pipe_dbuf_slice_mask)
>  			continue;
>  
>  		drm_mode_get_hv_timing(adjusted_mode, &hdisplay, &vdisplay);
> @@ -3923,8 +4003,19 @@ skl_ddb_get_pipe_allocation_limits(struct drm_i915_private *dev_priv,
>  			pipe_width = hdisplay;
>  	}
>  
> -	alloc->start = ddb_size * width_before_pipe / total_width;
> -	alloc->end = ddb_size * (width_before_pipe + pipe_width) / total_width;
> +	intel_state->enabled_dbuf_slices_mask = total_slice_mask;
> +
> +	start = ddb_range_size * width_before_pipe / total_width;
> +	end = ddb_range_size * (width_before_pipe + pipe_width) / total_width;

I found it a bit confusing that "total_slice_mask" is a mask of all
active CRTCs, whereas total_width is only the width of the subset of
active CRTCs that share our slice mask (i.e., different meanings of
"total").  I think the logic here is correct, but some variable renaming
might make it more obvious what's going on?


> +
> +	alloc->start = offset + start;
> +	alloc->end = offset + end;
> +
> +	DRM_DEBUG_KMS("Pipe %d ddb %d-%d\n", for_pipe,
> +		      alloc->start, alloc->end);
> +	DRM_DEBUG_KMS("Enabled ddb slices mask %x num supported %d\n",
> +		      intel_state->enabled_dbuf_slices_mask,
> +		      INTEL_INFO(dev_priv)->num_supported_dbuf_slices);
>  }
>  
>  static int skl_compute_wm_params(const struct intel_crtc_state *crtc_state,
> @@ -4094,6 +4185,121 @@ skl_plane_downscale_amount(const struct intel_crtc_state *crtc_state,
>  	return mul_fixed16(downscale_w, downscale_h);
>  }
>  
> +struct dbuf_slice_conf_entry {
> +	u32 active_pipes;
> +	u32 dbuf_mask[I915_MAX_PIPES];
> +};
> +
> +/*
> + * Table taken from Bspec 12716
> + * Pipes do have some preferred DBuf slice affinity,
> + * plus there are some hardcoded requirements on how
> + * those should be distributed for multipipe scenarios.
> + * For more DBuf slices algorithm can get even more messy
> + * and less readable, so decided to use a table almost
> + * as is from BSpec itself - that way it is at least easier
> + * to compare, change and check.
> + */
> +static struct dbuf_slice_conf_entry icl_allowed_dbufs[] = {
> +	{ BIT(PIPE_A), { DBUF_S1_BIT, 0, 0, 0 } },
> +	{ BIT(PIPE_B), { 0, DBUF_S1_BIT, 0, 0 } },
> +	{ BIT(PIPE_C), { 0, 0, DBUF_S2_BIT, 0 } },
> +	{ BIT(PIPE_A) | BIT(PIPE_B), { DBUF_S1_BIT, DBUF_S2_BIT, 0, 0 } },
> +	{ BIT(PIPE_A) | BIT(PIPE_C), { DBUF_S1_BIT, 0, DBUF_S2_BIT, 0 } },
> +	{ BIT(PIPE_B) | BIT(PIPE_C), { 0, DBUF_S1_BIT, DBUF_S2_BIT, 0 } },
> +	{ BIT(PIPE_A) | BIT(PIPE_B) | BIT(PIPE_C),
> +		{ DBUF_S1_BIT, DBUF_S1_BIT, DBUF_S2_BIT, 0 } }

I believe it's legal to leave ending array elements out of definitions,
in which case they'll just be 0.  So you can drop the fourth element of
all these dbuf_mask arrays since that's more intuitive given these
platforms only actually have three pipes.

> +};
> +
> +/*
> + * Table taken from Bspec 49255
> + * Pipes do have some preferred DBuf slice affinity,
> + * plus there are some hardcoded requirements on how
> + * those should be distributed for multipipe scenarios.
> + * For more DBuf slices algorithm can get even more messy
> + * and less readable, so decided to use a table almost
> + * as is from BSpec itself - that way it is at least easier
> + * to compare, change and check.
> + */
> +static struct dbuf_slice_conf_entry tgl_allowed_dbufs[] = {
> +	{ BIT(PIPE_A), { DBUF_S1_BIT | DBUF_S2_BIT, 0, 0, 0 } },
> +	{ BIT(PIPE_B), { 0, DBUF_S1_BIT | DBUF_S2_BIT, 0, 0 } },
> +	{ BIT(PIPE_C), { 0, 0, DBUF_S1_BIT | DBUF_S2_BIT, 0 } },
> +	{ BIT(PIPE_D), { 0, 0, 0, DBUF_S1_BIT | DBUF_S2_BIT } },
> +	{ BIT(PIPE_A) | BIT(PIPE_B), { DBUF_S2_BIT, DBUF_S1_BIT, 0, 0 } },
> +	{ BIT(PIPE_A) | BIT(PIPE_C), { DBUF_S1_BIT, 0, DBUF_S2_BIT, 0 } },
> +	{ BIT(PIPE_A) | BIT(PIPE_D), { DBUF_S1_BIT, 0, 0, DBUF_S2_BIT } },
> +	{ BIT(PIPE_B) | BIT(PIPE_C), { 0, DBUF_S1_BIT, DBUF_S2_BIT, 0 } },
> +	{ BIT(PIPE_B) | BIT(PIPE_D), { 0, DBUF_S1_BIT, 0, DBUF_S2_BIT } },
> +	{ BIT(PIPE_C) | BIT(PIPE_D), { 0, 0, DBUF_S2_BIT, DBUF_S2_BIT } },
                                             ^^^^^^^^^^^
I think this one is supposed to be S1 for pipe C.


> +	{ BIT(PIPE_A) | BIT(PIPE_B) | BIT(PIPE_C),
> +		{ DBUF_S1_BIT, DBUF_S1_BIT, DBUF_S2_BIT, 0 } },
> +	{ BIT(PIPE_A) | BIT(PIPE_B) | BIT(PIPE_D),
> +		{ DBUF_S1_BIT, DBUF_S1_BIT, 0, DBUF_S2_BIT } },
> +	{ BIT(PIPE_A) | BIT(PIPE_C) | BIT(PIPE_D),
> +		{ DBUF_S1_BIT, 0, DBUF_S2_BIT, DBUF_S2_BIT } },
> +	{ BIT(PIPE_B) | BIT(PIPE_C) | BIT(PIPE_D),
> +		{ 0, DBUF_S1_BIT, DBUF_S2_BIT, DBUF_S2_BIT } },
> +	{ BIT(PIPE_A) | BIT(PIPE_B) | BIT(PIPE_C) | BIT(PIPE_D),
> +		{ DBUF_S1_BIT, DBUF_S1_BIT, DBUF_S2_BIT, DBUF_S2_BIT } },
> +};
> +
> +static u32 i915_find_pipe_conf(int pipe,
> +			       u32 active_pipes,
> +			       const struct dbuf_slice_conf_entry *dbuf_slices,
> +			       int size)
> +{
> +	int i;
> +
> +	for (i = 0; i < size; i++) {
> +		if (dbuf_slices[i].active_pipes == active_pipes)
> +			return dbuf_slices[i].dbuf_mask[pipe];
> +	}
> +	return 0;
> +}
> +
> +/*
> + * This function finds an entry with same enabled pipe configuration and
> + * returns correspondent DBuf slice mask as stated in BSpec for particular
> + * platform.
> + */
> +static u32 icl_possible_dbuf_slices(int pipe,
> +				    u32 active_pipes,
> +				    const struct intel_crtc_state *crtc_state)
> +{
> +	return i915_find_pipe_conf(pipe, active_pipes,
> +				   icl_allowed_dbufs,
> +				   ARRAY_SIZE(icl_allowed_dbufs));

Should we be handling the <88.8% pipe ratio stuff here?  I.e., an extra
condition that if # pipes == 1 and the pipe ratio is less than 88.8%,
return both S1 and S2?  Or will that come in a future patch (in which
case a TODO/FIXME comment might be appropriate)?


> +}
> +
> +static u32 tgl_possible_dbuf_slices(int pipe,
> +				    u32 active_pipes,
> +				    const struct intel_crtc_state *crtc_state)
> +{
> +	return i915_find_pipe_conf(pipe, active_pipes,
> +				   tgl_allowed_dbufs,
> +				   ARRAY_SIZE(tgl_allowed_dbufs));
> +}

Seems like these two functions (as currently written) are so simple that
we can just invoke i915_find_pipe_conf directly in
i915_possible_dbuf_slices.  Might also be worth considering using an
empty table entry as a terminator rather than requiring than an explicit
length be passed.

> +
> +u32 i915_possible_dbuf_slices(struct drm_i915_private *dev_priv,

This can be static I think?

> +			      int pipe, u32 active_pipes,
> +			      const struct intel_crtc_state *crtc_state)
> +{
> +	if (IS_GEN(dev_priv, 11))
> +		return icl_possible_dbuf_slices(pipe,
> +						active_pipes,
> +						crtc_state);
> +	else if (IS_GEN(dev_priv, 12))
> +		return tgl_possible_dbuf_slices(pipe,
> +						active_pipes,
> +						crtc_state);
> +	/*
> +	 * For anything else just return one slice yet.
> +	 * Should be extended for other platforms.
> +	 */
> +	return DBUF_S1_BIT;
> +}

None of the three functions above seem to actually use crtc_state, so I
think we can drop that parameter.


> +
>  static u64
>  skl_plane_relative_data_rate(const struct intel_crtc_state *crtc_state,
>  			     const struct intel_plane_state *plane_state,
> -- 
> 2.17.1
>
Stanislav Lisovskiy Dec. 16, 2019, 2:31 p.m. UTC | #2
On Fri, 2019-12-13 at 16:52 -0800, Matt Roper wrote:
> On Fri, Dec 13, 2019 at 03:02:28PM +0200, Stanislav Lisovskiy wrote:
> > Added proper DBuf slice mapping to correspondent
> > pipes, depending on pipe configuration as stated
> > in BSpec.
> > 
> > v2:
> >     - Remove unneeded braces
> >     - Stop using macro for DBuf assignments as
> >       it seems to reduce readability.
> > 
> > v3: Start using enabled slices mask in dev_priv
> > 
> > v4: Renamed "enabled_slices" used in dev_priv
> >     to "enabled_dbuf_slices_mask"(Matt Roper)
> > 
> > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_pm.c | 226
> > ++++++++++++++++++++++++++++++--
> >  1 file changed, 216 insertions(+), 10 deletions(-)

Hi Matt, 

thanks for good comments, as usual. 
Most of the issues I've addressed in a recent series,
for some which I didn't I will reply inline here.

> > 
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> > b/drivers/gpu/drm/i915/intel_pm.c
> > index 111bcafd6e4c..a13052b2c2ef 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -3832,13 +3832,30 @@ bool intel_can_enable_sagv(struct
> > intel_atomic_state *state)
> >  	return true;
> >  }
> >  
> > +/*
> > + * Calculate initial DBuf slice offset, based on slice size
> > + * and mask(i.e if slice size is 1024 and second slice is enabled
> > + * offset would be 1024)
> > + */
> > +static u32 icl_get_first_dbuf_slice_offset(u32 dbuf_slice_mask,
> > +					   u32 slice_size, u32
> > ddb_size)
> > +{
> 
> It might be worth just passing the mask + dev_priv to this function
> and
> let it get slice_size / ddb_size on its own to keep the logic simpler
> at
> the callsite.

The thing is that this is called only at single place and also
at call site I still do need slice_size for other calculations.
So I would still leave it there, as leaving only dev_priv here 
as parameter won't simply things at callsite as it will have to
get those parameters anyway.

> 
> > +	u32 offset = 0;
> > +
> > +	if (!dbuf_slice_mask)
> > +		return 0;
> > +
> > +	offset = (ffs(dbuf_slice_mask) - 1) * slice_size;
> > +
> > +	WARN_ON(offset >= ddb_size);
> > +	return offset;
> > +}
> > +
> >  static u16 intel_get_ddb_size(struct drm_i915_private *dev_priv,
> >  			      const struct intel_crtc_state
> > *crtc_state,
> >  			      const u64 total_data_rate,
> >  			      const int num_active)
> 
> I probably should have mentioned it on the previous patch, but most
> of
> these parameters are no longer needed now.

> 
> >  {
> > -	struct drm_atomic_state *state = crtc_state->uapi.state;
> > -	struct intel_atomic_state *intel_state =
> > to_intel_atomic_state(state);
> >  	u16 ddb_size = INTEL_INFO(dev_priv)->ddb_size;
> >  
> >  	WARN_ON(ddb_size == 0);
> > @@ -3846,12 +3863,13 @@ static u16 intel_get_ddb_size(struct
> > drm_i915_private *dev_priv,
> >  	if (INTEL_GEN(dev_priv) < 11)
> >  		return ddb_size - 4; /* 4 blocks for bypass path
> > allocation */
> >  
> > -	intel_state->enabled_dbuf_slices_mask = DBUF_S1_BIT;
> > -	ddb_size /= 2;
> > -
> >  	return ddb_size;
> >  }
> >  
> > +u32 i915_possible_dbuf_slices(struct drm_i915_private *dev_priv,
> > +			      int pipe, u32 active_pipes,
> > +			      const struct intel_crtc_state
> > *crtc_state);
> > +
> >  static void
> >  skl_ddb_get_pipe_allocation_limits(struct drm_i915_private
> > *dev_priv,
> >  				   const struct intel_crtc_state
> > *crtc_state,
> > @@ -3866,7 +3884,14 @@ skl_ddb_get_pipe_allocation_limits(struct
> > drm_i915_private *dev_priv,
> >  	u32 pipe_width = 0, total_width = 0, width_before_pipe = 0;
> >  	enum pipe for_pipe = to_intel_crtc(for_crtc)->pipe;
> >  	u16 ddb_size;
> > +	u32 ddb_range_size;
> >  	u32 i;
> > +	u32 dbuf_slice_mask;
> > +	u32 active_pipes;
> > +	u32 offset;
> > +	u32 slice_size;
> > +	u32 total_slice_mask;
> > +	u32 start, end;
> >  
> >  	if (WARN_ON(!state) || !crtc_state->hw.active) {
> >  		alloc->start = 0;
> > @@ -3875,14 +3900,19 @@ skl_ddb_get_pipe_allocation_limits(struct
> > drm_i915_private *dev_priv,
> >  		return;
> >  	}
> >  
> > -	if (intel_state->active_pipe_changes)
> > +	if (intel_state->active_pipe_changes) {
> >  		*num_active = hweight8(intel_state->active_pipes);
> > -	else
> > +		active_pipes = intel_state->active_pipes;
> > +	} else {
> >  		*num_active = hweight8(dev_priv->active_pipes);
> > +		active_pipes = dev_priv->active_pipes;
> > +	}
> 
> Might be slightly more intuitive to move the num_active assignment
> outside the 'if' as
> 
>         *num_active = hweight8(active_pipes);
> 
> Up to you; doesn't really matter.
> 
> >  
> >  	ddb_size = intel_get_ddb_size(dev_priv, crtc_state,
> > total_data_rate,
> >  				      *num_active);
> >  
> > +	slice_size = ddb_size / INTEL_INFO(dev_priv)-
> > >num_supported_dbuf_slices;
> > +
> >  	/*
> >  	 * If the state doesn't change the active CRTC's or there is no
> >  	 * modeset request, then there's no need to recalculate;
> > @@ -3900,18 +3930,68 @@ skl_ddb_get_pipe_allocation_limits(struct
> > drm_i915_private *dev_priv,
> >  		return;
> >  	}
> >  
> > +	/*
> > +	 * Get allowed DBuf slices for correspondent pipe and platform.
> > +	 */
> > +	dbuf_slice_mask = i915_possible_dbuf_slices(dev_priv, for_pipe,
> > +						    active_pipes,
> > crtc_state);
> > +
> > +	DRM_DEBUG_KMS("DBuf slice mask %x pipe %d active pipes %x\n",
> > +		      dbuf_slice_mask,
> > +		      for_pipe, active_pipes);
> > +
> > +	/*
> > +	 * Figure out at which DBuf slice we start, i.e if we start at
> > Dbuf S2
> > +	 * and slice size is 1024, the offset would be 1024
> > +	 */
> > +	offset = icl_get_first_dbuf_slice_offset(dbuf_slice_mask,
> > +						 slice_size, ddb_size);
> > +
> > +	/*
> > +	 * Figure out total size of allowed DBuf slices, which is
> > basically
> > +	 * a number of allowed slices for that pipe multiplied by slice
> > size.
> > +	 * Inside of this
> > +	 * range ddb entries are still allocated in proportion to
> > display width.
> > +	 */
> > +	ddb_range_size = hweight8(dbuf_slice_mask) * slice_size;
> > +
> >  	/*
> >  	 * Watermark/ddb requirement highly depends upon width of the
> >  	 * framebuffer, So instead of allocating DDB equally among
> > pipes
> >  	 * distribute DDB based on resolution/width of the display.
> >  	 */
> > +	total_slice_mask = dbuf_slice_mask;
> >  	for_each_new_intel_crtc_in_state(intel_state, crtc, crtc_state,
> > i) {
> >  		const struct drm_display_mode *adjusted_mode =
> >  			&crtc_state->hw.adjusted_mode;
> >  		enum pipe pipe = crtc->pipe;
> >  		int hdisplay, vdisplay;
> > +		u32 pipe_dbuf_slice_mask =
> > +					i915_possible_dbuf_slices(dev_p
> > riv,
> > +								  pipe,
> > +								  activ
> > e_pipes,
> > +								  crtc_
> > state);
> > +
> > +		if (!crtc_state->hw.active)
> > +			continue;
> 
> You might want to do this check before we go through the bother of
> executing i915_possible_dbuf_slices.
> 
> > +
> > +		/*
> > +		 * According to BSpec pipe can share one dbuf slice
> > with another
> > +		 * pipes or pipe can use multiple dbufs, in both cases
> > we
> > +		 * account for other pipes only if they have exactly
> > same mask.
> > +		 * However we need to account how many slices we should
> > enable
> > +		 * in total.
> > +		 */
> > +		total_slice_mask |= pipe_dbuf_slice_mask;
> >  
> > -		if (!crtc_state->hw.enable)
> > +		/*
> > +		 * Do not account pipes using other slice sets
> > +		 * luckily as of current BSpec slice sets do not
> > partially
> > +		 * intersect(pipes share either same one slice or same
> > slice set
> > +		 * i.e no partial intersection), so it is enough to
> > check for
> > +		 * equality for now.
> > +		 */
> > +		if (dbuf_slice_mask != pipe_dbuf_slice_mask)
> >  			continue;
> >  
> >  		drm_mode_get_hv_timing(adjusted_mode, &hdisplay,
> > &vdisplay);
> > @@ -3923,8 +4003,19 @@ skl_ddb_get_pipe_allocation_limits(struct
> > drm_i915_private *dev_priv,
> >  			pipe_width = hdisplay;
> >  	}
> >  
> > -	alloc->start = ddb_size * width_before_pipe / total_width;
> > -	alloc->end = ddb_size * (width_before_pipe + pipe_width) /
> > total_width;
> > +	intel_state->enabled_dbuf_slices_mask = total_slice_mask;
> > +
> > +	start = ddb_range_size * width_before_pipe / total_width;
> > +	end = ddb_range_size * (width_before_pipe + pipe_width) /
> > total_width;
> 
> I found it a bit confusing that "total_slice_mask" is a mask of all
> active CRTCs, whereas total_width is only the width of the subset of
> active CRTCs that share our slice mask (i.e., different meanings of
> "total").  I think the logic here is correct, but some variable
> renaming
> might make it more obvious what's going on?
> 
> 
> > +
> > +	alloc->start = offset + start;
> > +	alloc->end = offset + end;
> > +
> > +	DRM_DEBUG_KMS("Pipe %d ddb %d-%d\n", for_pipe,
> > +		      alloc->start, alloc->end);
> > +	DRM_DEBUG_KMS("Enabled ddb slices mask %x num supported %d\n",
> > +		      intel_state->enabled_dbuf_slices_mask,
> > +		      INTEL_INFO(dev_priv)->num_supported_dbuf_slices);
> >  }
> >  
> >  static int skl_compute_wm_params(const struct intel_crtc_state
> > *crtc_state,
> > @@ -4094,6 +4185,121 @@ skl_plane_downscale_amount(const struct
> > intel_crtc_state *crtc_state,
> >  	return mul_fixed16(downscale_w, downscale_h);
> >  }
> >  
> > +struct dbuf_slice_conf_entry {
> > +	u32 active_pipes;
> > +	u32 dbuf_mask[I915_MAX_PIPES];
> > +};
> > +
> > +/*
> > + * Table taken from Bspec 12716
> > + * Pipes do have some preferred DBuf slice affinity,
> > + * plus there are some hardcoded requirements on how
> > + * those should be distributed for multipipe scenarios.
> > + * For more DBuf slices algorithm can get even more messy
> > + * and less readable, so decided to use a table almost
> > + * as is from BSpec itself - that way it is at least easier
> > + * to compare, change and check.
> > + */
> > +static struct dbuf_slice_conf_entry icl_allowed_dbufs[] = {
> > +	{ BIT(PIPE_A), { DBUF_S1_BIT, 0, 0, 0 } },
> > +	{ BIT(PIPE_B), { 0, DBUF_S1_BIT, 0, 0 } },
> > +	{ BIT(PIPE_C), { 0, 0, DBUF_S2_BIT, 0 } },
> > +	{ BIT(PIPE_A) | BIT(PIPE_B), { DBUF_S1_BIT, DBUF_S2_BIT, 0, 0 }
> > },
> > +	{ BIT(PIPE_A) | BIT(PIPE_C), { DBUF_S1_BIT, 0, DBUF_S2_BIT, 0 }
> > },
> > +	{ BIT(PIPE_B) | BIT(PIPE_C), { 0, DBUF_S1_BIT, DBUF_S2_BIT, 0 }
> > },
> > +	{ BIT(PIPE_A) | BIT(PIPE_B) | BIT(PIPE_C),
> > +		{ DBUF_S1_BIT, DBUF_S1_BIT, DBUF_S2_BIT, 0 } }
> 
> I believe it's legal to leave ending array elements out of
> definitions,
> in which case they'll just be 0.  So you can drop the fourth element
> of
> all these dbuf_mask arrays since that's more intuitive given these
> platforms only actually have three pipes.
> 
> > +};
> > +
> > +/*
> > + * Table taken from Bspec 49255
> > + * Pipes do have some preferred DBuf slice affinity,
> > + * plus there are some hardcoded requirements on how
> > + * those should be distributed for multipipe scenarios.
> > + * For more DBuf slices algorithm can get even more messy
> > + * and less readable, so decided to use a table almost
> > + * as is from BSpec itself - that way it is at least easier
> > + * to compare, change and check.
> > + */
> > +static struct dbuf_slice_conf_entry tgl_allowed_dbufs[] = {
> > +	{ BIT(PIPE_A), { DBUF_S1_BIT | DBUF_S2_BIT, 0, 0, 0 } },
> > +	{ BIT(PIPE_B), { 0, DBUF_S1_BIT | DBUF_S2_BIT, 0, 0 } },
> > +	{ BIT(PIPE_C), { 0, 0, DBUF_S1_BIT | DBUF_S2_BIT, 0 } },
> > +	{ BIT(PIPE_D), { 0, 0, 0, DBUF_S1_BIT | DBUF_S2_BIT } },
> > +	{ BIT(PIPE_A) | BIT(PIPE_B), { DBUF_S2_BIT, DBUF_S1_BIT, 0, 0 }
> > },
> > +	{ BIT(PIPE_A) | BIT(PIPE_C), { DBUF_S1_BIT, 0, DBUF_S2_BIT, 0 }
> > },
> > +	{ BIT(PIPE_A) | BIT(PIPE_D), { DBUF_S1_BIT, 0, 0, DBUF_S2_BIT }
> > },
> > +	{ BIT(PIPE_B) | BIT(PIPE_C), { 0, DBUF_S1_BIT, DBUF_S2_BIT, 0 }
> > },
> > +	{ BIT(PIPE_B) | BIT(PIPE_D), { 0, DBUF_S1_BIT, 0, DBUF_S2_BIT }
> > },
> > +	{ BIT(PIPE_C) | BIT(PIPE_D), { 0, 0, DBUF_S2_BIT, DBUF_S2_BIT }
> > },
> 
>                                              ^^^^^^^^^^^
> I think this one is supposed to be S1 for pipe C.
> 
> 
> > +	{ BIT(PIPE_A) | BIT(PIPE_B) | BIT(PIPE_C),
> > +		{ DBUF_S1_BIT, DBUF_S1_BIT, DBUF_S2_BIT, 0 } },
> > +	{ BIT(PIPE_A) | BIT(PIPE_B) | BIT(PIPE_D),
> > +		{ DBUF_S1_BIT, DBUF_S1_BIT, 0, DBUF_S2_BIT } },
> > +	{ BIT(PIPE_A) | BIT(PIPE_C) | BIT(PIPE_D),
> > +		{ DBUF_S1_BIT, 0, DBUF_S2_BIT, DBUF_S2_BIT } },
> > +	{ BIT(PIPE_B) | BIT(PIPE_C) | BIT(PIPE_D),
> > +		{ 0, DBUF_S1_BIT, DBUF_S2_BIT, DBUF_S2_BIT } },
> > +	{ BIT(PIPE_A) | BIT(PIPE_B) | BIT(PIPE_C) | BIT(PIPE_D),
> > +		{ DBUF_S1_BIT, DBUF_S1_BIT, DBUF_S2_BIT, DBUF_S2_BIT }
> > },
> > +};
> > +
> > +static u32 i915_find_pipe_conf(int pipe,
> > +			       u32 active_pipes,
> > +			       const struct dbuf_slice_conf_entry
> > *dbuf_slices,
> > +			       int size)
> > +{
> > +	int i;
> > +
> > +	for (i = 0; i < size; i++) {
> > +		if (dbuf_slices[i].active_pipes == active_pipes)
> > +			return dbuf_slices[i].dbuf_mask[pipe];
> > +	}
> > +	return 0;
> > +}
> > +
> > +/*
> > + * This function finds an entry with same enabled pipe
> > configuration and
> > + * returns correspondent DBuf slice mask as stated in BSpec for
> > particular
> > + * platform.
> > + */
> > +static u32 icl_possible_dbuf_slices(int pipe,
> > +				    u32 active_pipes,
> > +				    const struct intel_crtc_state
> > *crtc_state)
> > +{
> > +	return i915_find_pipe_conf(pipe, active_pipes,
> > +				   icl_allowed_dbufs,
> > +				   ARRAY_SIZE(icl_allowed_dbufs));
> 
> Should we be handling the <88.8% pipe ratio stuff here?  I.e., an
> extra
> condition that if # pipes == 1 and the pipe ratio is less than 88.8%,
> return both S1 and S2?  Or will that come in a future patch (in which
> case a TODO/FIXME comment might be appropriate)?

I have added a comment in the code. Problem is that the whole
pipe_ratio thing turned out to be completely wrong(see BSpec bugs). 
So I completely excluded that stuff until that constraint is
fully clarified. 

> 
> 
> > +}
> > +
> > +static u32 tgl_possible_dbuf_slices(int pipe,
> > +				    u32 active_pipes,
> > +				    const struct intel_crtc_state
> > *crtc_state)
> > +{
> > +	return i915_find_pipe_conf(pipe, active_pipes,
> > +				   tgl_allowed_dbufs,
> > +				   ARRAY_SIZE(tgl_allowed_dbufs));
> > +}
> 
> Seems like these two functions (as currently written) are so simple
> that
> we can just invoke i915_find_pipe_conf directly in
> i915_possible_dbuf_slices.  Might also be worth considering using an
> empty table entry as a terminator rather than requiring than an
> explicit
> length be passed.
> 
> > +
> > +u32 i915_possible_dbuf_slices(struct drm_i915_private *dev_priv,
> 
> This can be static I think?
> 
> > +			      int pipe, u32 active_pipes,
> > +			      const struct intel_crtc_state
> > *crtc_state)
> > +{
> > +	if (IS_GEN(dev_priv, 11))
> > +		return icl_possible_dbuf_slices(pipe,
> > +						active_pipes,
> > +						crtc_state);
> > +	else if (IS_GEN(dev_priv, 12))
> > +		return tgl_possible_dbuf_slices(pipe,
> > +						active_pipes,
> > +						crtc_state);
> > +	/*
> > +	 * For anything else just return one slice yet.
> > +	 * Should be extended for other platforms.
> > +	 */
> > +	return DBUF_S1_BIT;
> > +}
> 
> None of the three functions above seem to actually use crtc_state, so
> I
> think we can drop that parameter.

I would still leave it, as once we get that BSpec "pipe_ratio" or
something else clarified most likely we'll still need crtc_state..


Stan
> 
> 
> > +
> >  static u64
> >  skl_plane_relative_data_rate(const struct intel_crtc_state
> > *crtc_state,
> >  			     const struct intel_plane_state
> > *plane_state,
> > -- 
> > 2.17.1
> > 
> 
>
Ville Syrjälä Dec. 18, 2019, 6:05 p.m. UTC | #3
On Fri, Dec 13, 2019 at 03:02:28PM +0200, Stanislav Lisovskiy wrote:
> Added proper DBuf slice mapping to correspondent
> pipes, depending on pipe configuration as stated
> in BSpec.
> 
> v2:
>     - Remove unneeded braces
>     - Stop using macro for DBuf assignments as
>       it seems to reduce readability.
> 
> v3: Start using enabled slices mask in dev_priv
> 
> v4: Renamed "enabled_slices" used in dev_priv
>     to "enabled_dbuf_slices_mask"(Matt Roper)
> 
> Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 226 ++++++++++++++++++++++++++++++--
>  1 file changed, 216 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 111bcafd6e4c..a13052b2c2ef 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3832,13 +3832,30 @@ bool intel_can_enable_sagv(struct intel_atomic_state *state)
>  	return true;
>  }
>  
> +/*
> + * Calculate initial DBuf slice offset, based on slice size
> + * and mask(i.e if slice size is 1024 and second slice is enabled
> + * offset would be 1024)
> + */
> +static u32 icl_get_first_dbuf_slice_offset(u32 dbuf_slice_mask,
> +					   u32 slice_size, u32 ddb_size)

s/u32/unsigned int/ or something. No point in using sized types when
we're not dealing with some thing that is actually that size.

> +{
> +	u32 offset = 0;
> +
> +	if (!dbuf_slice_mask)
> +		return 0;
> +
> +	offset = (ffs(dbuf_slice_mask) - 1) * slice_size;
> +
> +	WARN_ON(offset >= ddb_size);
> +	return offset;
> +}
> +
>  static u16 intel_get_ddb_size(struct drm_i915_private *dev_priv,
>  			      const struct intel_crtc_state *crtc_state,
>  			      const u64 total_data_rate,
>  			      const int num_active)
>  {
> -	struct drm_atomic_state *state = crtc_state->uapi.state;
> -	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
>  	u16 ddb_size = INTEL_INFO(dev_priv)->ddb_size;
>  
>  	WARN_ON(ddb_size == 0);
> @@ -3846,12 +3863,13 @@ static u16 intel_get_ddb_size(struct drm_i915_private *dev_priv,
>  	if (INTEL_GEN(dev_priv) < 11)
>  		return ddb_size - 4; /* 4 blocks for bypass path allocation */
>  
> -	intel_state->enabled_dbuf_slices_mask = DBUF_S1_BIT;
> -	ddb_size /= 2;
> -
>  	return ddb_size;
>  }
>  
> +u32 i915_possible_dbuf_slices(struct drm_i915_private *dev_priv,
> +			      int pipe, u32 active_pipes,
> +			      const struct intel_crtc_state *crtc_state);
> +
>  static void
>  skl_ddb_get_pipe_allocation_limits(struct drm_i915_private *dev_priv,
>  				   const struct intel_crtc_state *crtc_state,
> @@ -3866,7 +3884,14 @@ skl_ddb_get_pipe_allocation_limits(struct drm_i915_private *dev_priv,
>  	u32 pipe_width = 0, total_width = 0, width_before_pipe = 0;
>  	enum pipe for_pipe = to_intel_crtc(for_crtc)->pipe;
>  	u16 ddb_size;
> +	u32 ddb_range_size;
>  	u32 i;
> +	u32 dbuf_slice_mask;
> +	u32 active_pipes;
> +	u32 offset;
> +	u32 slice_size;
> +	u32 total_slice_mask;
> +	u32 start, end;
>  
>  	if (WARN_ON(!state) || !crtc_state->hw.active) {
>  		alloc->start = 0;
> @@ -3875,14 +3900,19 @@ skl_ddb_get_pipe_allocation_limits(struct drm_i915_private *dev_priv,
>  		return;
>  	}
>  
> -	if (intel_state->active_pipe_changes)
> +	if (intel_state->active_pipe_changes) {
>  		*num_active = hweight8(intel_state->active_pipes);
> -	else
> +		active_pipes = intel_state->active_pipes;
> +	} else {
>  		*num_active = hweight8(dev_priv->active_pipes);
> +		active_pipes = dev_priv->active_pipes;
> +	}
>  
>  	ddb_size = intel_get_ddb_size(dev_priv, crtc_state, total_data_rate,
>  				      *num_active);
>  
> +	slice_size = ddb_size / INTEL_INFO(dev_priv)->num_supported_dbuf_slices;
> +
>  	/*
>  	 * If the state doesn't change the active CRTC's or there is no
>  	 * modeset request, then there's no need to recalculate;
> @@ -3900,18 +3930,68 @@ skl_ddb_get_pipe_allocation_limits(struct drm_i915_private *dev_priv,
>  		return;
>  	}
>  
> +	/*
> +	 * Get allowed DBuf slices for correspondent pipe and platform.
> +	 */
> +	dbuf_slice_mask = i915_possible_dbuf_slices(dev_priv, for_pipe,
> +						    active_pipes, crtc_state);
> +
> +	DRM_DEBUG_KMS("DBuf slice mask %x pipe %d active pipes %x\n",
> +		      dbuf_slice_mask,
> +		      for_pipe, active_pipes);
> +
> +	/*
> +	 * Figure out at which DBuf slice we start, i.e if we start at Dbuf S2
> +	 * and slice size is 1024, the offset would be 1024
> +	 */
> +	offset = icl_get_first_dbuf_slice_offset(dbuf_slice_mask,
> +						 slice_size, ddb_size);
> +
> +	/*
> +	 * Figure out total size of allowed DBuf slices, which is basically
> +	 * a number of allowed slices for that pipe multiplied by slice size.
> +	 * Inside of this
> +	 * range ddb entries are still allocated in proportion to display width.
> +	 */
> +	ddb_range_size = hweight8(dbuf_slice_mask) * slice_size;
> +
>  	/*
>  	 * Watermark/ddb requirement highly depends upon width of the
>  	 * framebuffer, So instead of allocating DDB equally among pipes
>  	 * distribute DDB based on resolution/width of the display.
>  	 */
> +	total_slice_mask = dbuf_slice_mask;
>  	for_each_new_intel_crtc_in_state(intel_state, crtc, crtc_state, i) {
>  		const struct drm_display_mode *adjusted_mode =
>  			&crtc_state->hw.adjusted_mode;
>  		enum pipe pipe = crtc->pipe;
>  		int hdisplay, vdisplay;
> +		u32 pipe_dbuf_slice_mask =
> +					i915_possible_dbuf_slices(dev_priv,
> +								  pipe,
> +								  active_pipes,
> +								  crtc_state);
> +
> +		if (!crtc_state->hw.active)
> +			continue;
> +
> +		/*
> +		 * According to BSpec pipe can share one dbuf slice with another
> +		 * pipes or pipe can use multiple dbufs, in both cases we
> +		 * account for other pipes only if they have exactly same mask.
> +		 * However we need to account how many slices we should enable
> +		 * in total.
> +		 */
> +		total_slice_mask |= pipe_dbuf_slice_mask;
>  
> -		if (!crtc_state->hw.enable)
> +		/*
> +		 * Do not account pipes using other slice sets
> +		 * luckily as of current BSpec slice sets do not partially
> +		 * intersect(pipes share either same one slice or same slice set
> +		 * i.e no partial intersection), so it is enough to check for
> +		 * equality for now.
> +		 */
> +		if (dbuf_slice_mask != pipe_dbuf_slice_mask)
>  			continue;
>  
>  		drm_mode_get_hv_timing(adjusted_mode, &hdisplay, &vdisplay);
> @@ -3923,8 +4003,19 @@ skl_ddb_get_pipe_allocation_limits(struct drm_i915_private *dev_priv,
>  			pipe_width = hdisplay;
>  	}
>  
> -	alloc->start = ddb_size * width_before_pipe / total_width;
> -	alloc->end = ddb_size * (width_before_pipe + pipe_width) / total_width;
> +	intel_state->enabled_dbuf_slices_mask = total_slice_mask;
> +
> +	start = ddb_range_size * width_before_pipe / total_width;
> +	end = ddb_range_size * (width_before_pipe + pipe_width) / total_width;
> +
> +	alloc->start = offset + start;
> +	alloc->end = offset + end;
> +
> +	DRM_DEBUG_KMS("Pipe %d ddb %d-%d\n", for_pipe,
> +		      alloc->start, alloc->end);
> +	DRM_DEBUG_KMS("Enabled ddb slices mask %x num supported %d\n",
> +		      intel_state->enabled_dbuf_slices_mask,
> +		      INTEL_INFO(dev_priv)->num_supported_dbuf_slices);
>  }

That function really is quite the monster. Probably needs to be split up
into sensible chunks... later.

>  
>  static int skl_compute_wm_params(const struct intel_crtc_state *crtc_state,
> @@ -4094,6 +4185,121 @@ skl_plane_downscale_amount(const struct intel_crtc_state *crtc_state,
>  	return mul_fixed16(downscale_w, downscale_h);
>  }
>  
> +struct dbuf_slice_conf_entry {
> +	u32 active_pipes;
> +	u32 dbuf_mask[I915_MAX_PIPES];

u8 will do for everything.

> +};
> +
> +/*
> + * Table taken from Bspec 12716
> + * Pipes do have some preferred DBuf slice affinity,
> + * plus there are some hardcoded requirements on how
> + * those should be distributed for multipipe scenarios.
> + * For more DBuf slices algorithm can get even more messy
> + * and less readable, so decided to use a table almost
> + * as is from BSpec itself - that way it is at least easier
> + * to compare, change and check.
> + */
> +static struct dbuf_slice_conf_entry icl_allowed_dbufs[] = {
> +	{ BIT(PIPE_A), { DBUF_S1_BIT, 0, 0, 0 } },

Still don't like that DBUF_S1_BIT. I still think it should be an
enum dbuf_slice + explcit BIT(DBUF_S1) where needed.

And please use named initializers instead of sprinkling zeroes.
IMO this should just look something like:

	{ .active_pipes = BIT(PIPE_A) | BIT(PIPE_B),
	  .dbuf_slices = { [PIPE_A] = BIT(DBUF_S1),
	  		   [PIPE_B] = BIT(DBUF_S2),
			 },
	},

> +	{ BIT(PIPE_B), { 0, DBUF_S1_BIT, 0, 0 } },
> +	{ BIT(PIPE_C), { 0, 0, DBUF_S2_BIT, 0 } },
> +	{ BIT(PIPE_A) | BIT(PIPE_B), { DBUF_S1_BIT, DBUF_S2_BIT, 0, 0 } },
> +	{ BIT(PIPE_A) | BIT(PIPE_C), { DBUF_S1_BIT, 0, DBUF_S2_BIT, 0 } },
> +	{ BIT(PIPE_B) | BIT(PIPE_C), { 0, DBUF_S1_BIT, DBUF_S2_BIT, 0 } },
> +	{ BIT(PIPE_A) | BIT(PIPE_B) | BIT(PIPE_C),
> +		{ DBUF_S1_BIT, DBUF_S1_BIT, DBUF_S2_BIT, 0 } }
> +};
> +
> +/*
> + * Table taken from Bspec 49255
> + * Pipes do have some preferred DBuf slice affinity,
> + * plus there are some hardcoded requirements on how
> + * those should be distributed for multipipe scenarios.
> + * For more DBuf slices algorithm can get even more messy
> + * and less readable, so decided to use a table almost
> + * as is from BSpec itself - that way it is at least easier
> + * to compare, change and check.
> + */
> +static struct dbuf_slice_conf_entry tgl_allowed_dbufs[] = {
> +	{ BIT(PIPE_A), { DBUF_S1_BIT | DBUF_S2_BIT, 0, 0, 0 } },
> +	{ BIT(PIPE_B), { 0, DBUF_S1_BIT | DBUF_S2_BIT, 0, 0 } },
> +	{ BIT(PIPE_C), { 0, 0, DBUF_S1_BIT | DBUF_S2_BIT, 0 } },
> +	{ BIT(PIPE_D), { 0, 0, 0, DBUF_S1_BIT | DBUF_S2_BIT } },
> +	{ BIT(PIPE_A) | BIT(PIPE_B), { DBUF_S2_BIT, DBUF_S1_BIT, 0, 0 } },
> +	{ BIT(PIPE_A) | BIT(PIPE_C), { DBUF_S1_BIT, 0, DBUF_S2_BIT, 0 } },
> +	{ BIT(PIPE_A) | BIT(PIPE_D), { DBUF_S1_BIT, 0, 0, DBUF_S2_BIT } },
> +	{ BIT(PIPE_B) | BIT(PIPE_C), { 0, DBUF_S1_BIT, DBUF_S2_BIT, 0 } },
> +	{ BIT(PIPE_B) | BIT(PIPE_D), { 0, DBUF_S1_BIT, 0, DBUF_S2_BIT } },
> +	{ BIT(PIPE_C) | BIT(PIPE_D), { 0, 0, DBUF_S2_BIT, DBUF_S2_BIT } },

I believe pipe C should use S1 here.

> +	{ BIT(PIPE_A) | BIT(PIPE_B) | BIT(PIPE_C),
> +		{ DBUF_S1_BIT, DBUF_S1_BIT, DBUF_S2_BIT, 0 } },
> +	{ BIT(PIPE_A) | BIT(PIPE_B) | BIT(PIPE_D),
> +		{ DBUF_S1_BIT, DBUF_S1_BIT, 0, DBUF_S2_BIT } },
> +	{ BIT(PIPE_A) | BIT(PIPE_C) | BIT(PIPE_D),
> +		{ DBUF_S1_BIT, 0, DBUF_S2_BIT, DBUF_S2_BIT } },
> +	{ BIT(PIPE_B) | BIT(PIPE_C) | BIT(PIPE_D),
> +		{ 0, DBUF_S1_BIT, DBUF_S2_BIT, DBUF_S2_BIT } },
> +	{ BIT(PIPE_A) | BIT(PIPE_B) | BIT(PIPE_C) | BIT(PIPE_D),
> +		{ DBUF_S1_BIT, DBUF_S1_BIT, DBUF_S2_BIT, DBUF_S2_BIT } },
> +};
> +
> +static u32 i915_find_pipe_conf(int pipe,

enum pipe pipe

> +			       u32 active_pipes,
> +			       const struct dbuf_slice_conf_entry *dbuf_slices,
> +			       int size)

Could just add a sentinel to the end of the arrays and avoid this
'size' stuff.

The function name is still very confusing. icl_compute_dbuf_slices()
or something perhaps. Or even without a prefix since it's static
anyway.

> +{
> +	int i;
> +
> +	for (i = 0; i < size; i++) {
> +		if (dbuf_slices[i].active_pipes == active_pipes)
> +			return dbuf_slices[i].dbuf_mask[pipe];
> +	}
> +	return 0;
> +}
> +
> +/*
> + * This function finds an entry with same enabled pipe configuration and
> + * returns correspondent DBuf slice mask as stated in BSpec for particular
> + * platform.
> + */
> +static u32 icl_possible_dbuf_slices(int pipe,
> +				    u32 active_pipes,
> +				    const struct intel_crtc_state *crtc_state)
> +{
> +	return i915_find_pipe_conf(pipe, active_pipes,
> +				   icl_allowed_dbufs,
> +				   ARRAY_SIZE(icl_allowed_dbufs));
> +}
> +
> +static u32 tgl_possible_dbuf_slices(int pipe,
> +				    u32 active_pipes,
> +				    const struct intel_crtc_state *crtc_state)
> +{
> +	return i915_find_pipe_conf(pipe, active_pipes,
> +				   tgl_allowed_dbufs,
> +				   ARRAY_SIZE(tgl_allowed_dbufs));
> +}

These wrappers aren't really doing anything for us.

> +
> +u32 i915_possible_dbuf_slices(struct drm_i915_private *dev_priv,

dev_priv and pipe are redundant parameters. Both
can be extracted from crtc_state. I would put crtc_state as the first
parameter. Matches common practice more closely.

s/i915/skl/ in the function name.

> +			      int pipe, u32 active_pipes,
> +			      const struct intel_crtc_state *crtc_state)
> +{
> +	if (IS_GEN(dev_priv, 11))
> +		return icl_possible_dbuf_slices(pipe,
> +						active_pipes,
> +						crtc_state);
> +	else if (IS_GEN(dev_priv, 12))
> +		return tgl_possible_dbuf_slices(pipe,
> +						active_pipes,
> +						crtc_state);

The established convention is to go from new platforms
to old platforms, if possible.

> +	/*
> +	 * For anything else just return one slice yet.
> +	 * Should be extended for other platforms.
> +	 */
> +	return DBUF_S1_BIT;
> +}
> +
>  static u64
>  skl_plane_relative_data_rate(const struct intel_crtc_state *crtc_state,
>  			     const struct intel_plane_state *plane_state,
> -- 
> 2.17.1
Stanislav Lisovskiy Dec. 19, 2019, 11:58 a.m. UTC | #4
On Wed, 2019-12-18 at 20:05 +0200, Ville Syrjälä wrote:
> On Fri, Dec 13, 2019 at 03:02:28PM +0200, Stanislav Lisovskiy wrote:
> > Added proper DBuf slice mapping to correspondent
> > pipes, depending on pipe configuration as stated
> > in BSpec.
> > 
> > v2:
> >     - Remove unneeded braces
> >     - Stop using macro for DBuf assignments as
> >       it seems to reduce readability.
> > 
> > v3: Start using enabled slices mask in dev_priv
> > 
> > v4: Renamed "enabled_slices" used in dev_priv
> >     to "enabled_dbuf_slices_mask"(Matt Roper)
> > 
> > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_pm.c | 226
> > ++++++++++++++++++++++++++++++--
> >  1 file changed, 216 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> > b/drivers/gpu/drm/i915/intel_pm.c
> > index 111bcafd6e4c..a13052b2c2ef 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -3832,13 +3832,30 @@ bool intel_can_enable_sagv(struct
> > intel_atomic_state *state)
> >  	return true;
> >  }
> >  
> > +/*
> > + * Calculate initial DBuf slice offset, based on slice size
> > + * and mask(i.e if slice size is 1024 and second slice is enabled
> > + * offset would be 1024)
> > + */
> > +static u32 icl_get_first_dbuf_slice_offset(u32 dbuf_slice_mask,
> > +					   u32 slice_size, u32
> > ddb_size)
> 
> s/u32/unsigned int/ or something. No point in using sized types when
> we're not dealing with some thing that is actually that size.
> 
> > +{
> > +	u32 offset = 0;
> > +
> > +	if (!dbuf_slice_mask)
> > +		return 0;
> > +
> > +	offset = (ffs(dbuf_slice_mask) - 1) * slice_size;
> > +
> > +	WARN_ON(offset >= ddb_size);
> > +	return offset;
> > +}
> > +
> >  static u16 intel_get_ddb_size(struct drm_i915_private *dev_priv,
> >  			      const struct intel_crtc_state
> > *crtc_state,
> >  			      const u64 total_data_rate,
> >  			      const int num_active)
> >  {
> > -	struct drm_atomic_state *state = crtc_state->uapi.state;
> > -	struct intel_atomic_state *intel_state =
> > to_intel_atomic_state(state);
> >  	u16 ddb_size = INTEL_INFO(dev_priv)->ddb_size;
> >  
> >  	WARN_ON(ddb_size == 0);
> > @@ -3846,12 +3863,13 @@ static u16 intel_get_ddb_size(struct
> > drm_i915_private *dev_priv,
> >  	if (INTEL_GEN(dev_priv) < 11)
> >  		return ddb_size - 4; /* 4 blocks for bypass path
> > allocation */
> >  
> > -	intel_state->enabled_dbuf_slices_mask = DBUF_S1_BIT;
> > -	ddb_size /= 2;
> > -
> >  	return ddb_size;
> >  }
> >  
> > +u32 i915_possible_dbuf_slices(struct drm_i915_private *dev_priv,
> > +			      int pipe, u32 active_pipes,
> > +			      const struct intel_crtc_state
> > *crtc_state);
> > +
> >  static void
> >  skl_ddb_get_pipe_allocation_limits(struct drm_i915_private
> > *dev_priv,
> >  				   const struct intel_crtc_state
> > *crtc_state,
> > @@ -3866,7 +3884,14 @@ skl_ddb_get_pipe_allocation_limits(struct
> > drm_i915_private *dev_priv,
> >  	u32 pipe_width = 0, total_width = 0, width_before_pipe = 0;
> >  	enum pipe for_pipe = to_intel_crtc(for_crtc)->pipe;
> >  	u16 ddb_size;
> > +	u32 ddb_range_size;
> >  	u32 i;
> > +	u32 dbuf_slice_mask;
> > +	u32 active_pipes;
> > +	u32 offset;
> > +	u32 slice_size;
> > +	u32 total_slice_mask;
> > +	u32 start, end;
> >  
> >  	if (WARN_ON(!state) || !crtc_state->hw.active) {
> >  		alloc->start = 0;
> > @@ -3875,14 +3900,19 @@ skl_ddb_get_pipe_allocation_limits(struct
> > drm_i915_private *dev_priv,
> >  		return;
> >  	}
> >  
> > -	if (intel_state->active_pipe_changes)
> > +	if (intel_state->active_pipe_changes) {
> >  		*num_active = hweight8(intel_state->active_pipes);
> > -	else
> > +		active_pipes = intel_state->active_pipes;
> > +	} else {
> >  		*num_active = hweight8(dev_priv->active_pipes);
> > +		active_pipes = dev_priv->active_pipes;
> > +	}
> >  
> >  	ddb_size = intel_get_ddb_size(dev_priv, crtc_state,
> > total_data_rate,
> >  				      *num_active);
> >  
> > +	slice_size = ddb_size / INTEL_INFO(dev_priv)-
> > >num_supported_dbuf_slices;
> > +
> >  	/*
> >  	 * If the state doesn't change the active CRTC's or there is no
> >  	 * modeset request, then there's no need to recalculate;
> > @@ -3900,18 +3930,68 @@ skl_ddb_get_pipe_allocation_limits(struct
> > drm_i915_private *dev_priv,
> >  		return;
> >  	}
> >  
> > +	/*
> > +	 * Get allowed DBuf slices for correspondent pipe and platform.
> > +	 */
> > +	dbuf_slice_mask = i915_possible_dbuf_slices(dev_priv, for_pipe,
> > +						    active_pipes,
> > crtc_state);
> > +
> > +	DRM_DEBUG_KMS("DBuf slice mask %x pipe %d active pipes %x\n",
> > +		      dbuf_slice_mask,
> > +		      for_pipe, active_pipes);
> > +
> > +	/*
> > +	 * Figure out at which DBuf slice we start, i.e if we start at
> > Dbuf S2
> > +	 * and slice size is 1024, the offset would be 1024
> > +	 */
> > +	offset = icl_get_first_dbuf_slice_offset(dbuf_slice_mask,
> > +						 slice_size, ddb_size);
> > +
> > +	/*
> > +	 * Figure out total size of allowed DBuf slices, which is
> > basically
> > +	 * a number of allowed slices for that pipe multiplied by slice
> > size.
> > +	 * Inside of this
> > +	 * range ddb entries are still allocated in proportion to
> > display width.
> > +	 */
> > +	ddb_range_size = hweight8(dbuf_slice_mask) * slice_size;
> > +
> >  	/*
> >  	 * Watermark/ddb requirement highly depends upon width of the
> >  	 * framebuffer, So instead of allocating DDB equally among
> > pipes
> >  	 * distribute DDB based on resolution/width of the display.
> >  	 */
> > +	total_slice_mask = dbuf_slice_mask;
> >  	for_each_new_intel_crtc_in_state(intel_state, crtc, crtc_state,
> > i) {
> >  		const struct drm_display_mode *adjusted_mode =
> >  			&crtc_state->hw.adjusted_mode;
> >  		enum pipe pipe = crtc->pipe;
> >  		int hdisplay, vdisplay;
> > +		u32 pipe_dbuf_slice_mask =
> > +					i915_possible_dbuf_slices(dev_p
> > riv,
> > +								  pipe,
> > +								  activ
> > e_pipes,
> > +								  crtc_
> > state);
> > +
> > +		if (!crtc_state->hw.active)
> > +			continue;
> > +
> > +		/*
> > +		 * According to BSpec pipe can share one dbuf slice
> > with another
> > +		 * pipes or pipe can use multiple dbufs, in both cases
> > we
> > +		 * account for other pipes only if they have exactly
> > same mask.
> > +		 * However we need to account how many slices we should
> > enable
> > +		 * in total.
> > +		 */
> > +		total_slice_mask |= pipe_dbuf_slice_mask;
> >  
> > -		if (!crtc_state->hw.enable)
> > +		/*
> > +		 * Do not account pipes using other slice sets
> > +		 * luckily as of current BSpec slice sets do not
> > partially
> > +		 * intersect(pipes share either same one slice or same
> > slice set
> > +		 * i.e no partial intersection), so it is enough to
> > check for
> > +		 * equality for now.
> > +		 */
> > +		if (dbuf_slice_mask != pipe_dbuf_slice_mask)
> >  			continue;
> >  
> >  		drm_mode_get_hv_timing(adjusted_mode, &hdisplay,
> > &vdisplay);
> > @@ -3923,8 +4003,19 @@ skl_ddb_get_pipe_allocation_limits(struct
> > drm_i915_private *dev_priv,
> >  			pipe_width = hdisplay;
> >  	}
> >  
> > -	alloc->start = ddb_size * width_before_pipe / total_width;
> > -	alloc->end = ddb_size * (width_before_pipe + pipe_width) /
> > total_width;
> > +	intel_state->enabled_dbuf_slices_mask = total_slice_mask;
> > +
> > +	start = ddb_range_size * width_before_pipe / total_width;
> > +	end = ddb_range_size * (width_before_pipe + pipe_width) /
> > total_width;
> > +
> > +	alloc->start = offset + start;
> > +	alloc->end = offset + end;
> > +
> > +	DRM_DEBUG_KMS("Pipe %d ddb %d-%d\n", for_pipe,
> > +		      alloc->start, alloc->end);
> > +	DRM_DEBUG_KMS("Enabled ddb slices mask %x num supported %d\n",
> > +		      intel_state->enabled_dbuf_slices_mask,
> > +		      INTEL_INFO(dev_priv)->num_supported_dbuf_slices);
> >  }
> 
> That function really is quite the monster. Probably needs to be split
> up
> into sensible chunks... later.
> 
> >  
> >  static int skl_compute_wm_params(const struct intel_crtc_state
> > *crtc_state,
> > @@ -4094,6 +4185,121 @@ skl_plane_downscale_amount(const struct
> > intel_crtc_state *crtc_state,
> >  	return mul_fixed16(downscale_w, downscale_h);
> >  }
> >  
> > +struct dbuf_slice_conf_entry {
> > +	u32 active_pipes;
> > +	u32 dbuf_mask[I915_MAX_PIPES];
> 
> u8 will do for everything.
> 
> > +};
> > +
> > +/*
> > + * Table taken from Bspec 12716
> > + * Pipes do have some preferred DBuf slice affinity,
> > + * plus there are some hardcoded requirements on how
> > + * those should be distributed for multipipe scenarios.
> > + * For more DBuf slices algorithm can get even more messy
> > + * and less readable, so decided to use a table almost
> > + * as is from BSpec itself - that way it is at least easier
> > + * to compare, change and check.
> > + */
> > +static struct dbuf_slice_conf_entry icl_allowed_dbufs[] = {
> > +	{ BIT(PIPE_A), { DBUF_S1_BIT, 0, 0, 0 } },
> 
> Still don't like that DBUF_S1_BIT. I still think it should be an
> enum dbuf_slice + explcit BIT(DBUF_S1) where needed.
> 
> And please use named initializers instead of sprinkling zeroes.
> IMO this should just look something like:
> 
> 	{ .active_pipes = BIT(PIPE_A) | BIT(PIPE_B),
> 	  .dbuf_slices = { [PIPE_A] = BIT(DBUF_S1),
> 	  		   [PIPE_B] = BIT(DBUF_S2),
> 			 },
> 	},
> 
> > +	{ BIT(PIPE_B), { 0, DBUF_S1_BIT, 0, 0 } },
> > +	{ BIT(PIPE_C), { 0, 0, DBUF_S2_BIT, 0 } },
> > +	{ BIT(PIPE_A) | BIT(PIPE_B), { DBUF_S1_BIT, DBUF_S2_BIT, 0, 0 }
> > },
> > +	{ BIT(PIPE_A) | BIT(PIPE_C), { DBUF_S1_BIT, 0, DBUF_S2_BIT, 0 }
> > },
> > +	{ BIT(PIPE_B) | BIT(PIPE_C), { 0, DBUF_S1_BIT, DBUF_S2_BIT, 0 }
> > },
> > +	{ BIT(PIPE_A) | BIT(PIPE_B) | BIT(PIPE_C),
> > +		{ DBUF_S1_BIT, DBUF_S1_BIT, DBUF_S2_BIT, 0 } }
> > +};
> > +
> > +/*
> > + * Table taken from Bspec 49255
> > + * Pipes do have some preferred DBuf slice affinity,
> > + * plus there are some hardcoded requirements on how
> > + * those should be distributed for multipipe scenarios.
> > + * For more DBuf slices algorithm can get even more messy
> > + * and less readable, so decided to use a table almost
> > + * as is from BSpec itself - that way it is at least easier
> > + * to compare, change and check.
> > + */
> > +static struct dbuf_slice_conf_entry tgl_allowed_dbufs[] = {
> > +	{ BIT(PIPE_A), { DBUF_S1_BIT | DBUF_S2_BIT, 0, 0, 0 } },
> > +	{ BIT(PIPE_B), { 0, DBUF_S1_BIT | DBUF_S2_BIT, 0, 0 } },
> > +	{ BIT(PIPE_C), { 0, 0, DBUF_S1_BIT | DBUF_S2_BIT, 0 } },
> > +	{ BIT(PIPE_D), { 0, 0, 0, DBUF_S1_BIT | DBUF_S2_BIT } },
> > +	{ BIT(PIPE_A) | BIT(PIPE_B), { DBUF_S2_BIT, DBUF_S1_BIT, 0, 0 }
> > },
> > +	{ BIT(PIPE_A) | BIT(PIPE_C), { DBUF_S1_BIT, 0, DBUF_S2_BIT, 0 }
> > },
> > +	{ BIT(PIPE_A) | BIT(PIPE_D), { DBUF_S1_BIT, 0, 0, DBUF_S2_BIT }
> > },
> > +	{ BIT(PIPE_B) | BIT(PIPE_C), { 0, DBUF_S1_BIT, DBUF_S2_BIT, 0 }
> > },
> > +	{ BIT(PIPE_B) | BIT(PIPE_D), { 0, DBUF_S1_BIT, 0, DBUF_S2_BIT }
> > },
> > +	{ BIT(PIPE_C) | BIT(PIPE_D), { 0, 0, DBUF_S2_BIT, DBUF_S2_BIT }
> > },
> 
> I believe pipe C should use S1 here.

Thanks for spotting. Actually my generator says it as well, just didn't
use it for checking. BTW can you review that one as well - I think
it might be useful to keep in igt/tools, to avoid exactly typos like
this. 

> 
> > +	{ BIT(PIPE_A) | BIT(PIPE_B) | BIT(PIPE_C),
> > +		{ DBUF_S1_BIT, DBUF_S1_BIT, DBUF_S2_BIT, 0 } },
> > +	{ BIT(PIPE_A) | BIT(PIPE_B) | BIT(PIPE_D),
> > +		{ DBUF_S1_BIT, DBUF_S1_BIT, 0, DBUF_S2_BIT } },
> > +	{ BIT(PIPE_A) | BIT(PIPE_C) | BIT(PIPE_D),
> > +		{ DBUF_S1_BIT, 0, DBUF_S2_BIT, DBUF_S2_BIT } },
> > +	{ BIT(PIPE_B) | BIT(PIPE_C) | BIT(PIPE_D),
> > +		{ 0, DBUF_S1_BIT, DBUF_S2_BIT, DBUF_S2_BIT } },
> > +	{ BIT(PIPE_A) | BIT(PIPE_B) | BIT(PIPE_C) | BIT(PIPE_D),
> > +		{ DBUF_S1_BIT, DBUF_S1_BIT, DBUF_S2_BIT, DBUF_S2_BIT }
> > },
> > +};
> > +
> > +static u32 i915_find_pipe_conf(int pipe,
> 
> enum pipe pipe
> 
> > +			       u32 active_pipes,
> > +			       const struct dbuf_slice_conf_entry
> > *dbuf_slices,
> > +			       int size)
> 
> Could just add a sentinel to the end of the arrays and avoid this
> 'size' stuff.
> 
> The function name is still very confusing. icl_compute_dbuf_slices()
> or something perhaps. Or even without a prefix since it's static
> anyway.
> 
> > +{
> > +	int i;
> > +
> > +	for (i = 0; i < size; i++) {
> > +		if (dbuf_slices[i].active_pipes == active_pipes)
> > +			return dbuf_slices[i].dbuf_mask[pipe];
> > +	}
> > +	return 0;
> > +}
> > +
> > +/*
> > + * This function finds an entry with same enabled pipe
> > configuration and
> > + * returns correspondent DBuf slice mask as stated in BSpec for
> > particular
> > + * platform.
> > + */
> > +static u32 icl_possible_dbuf_slices(int pipe,
> > +				    u32 active_pipes,
> > +				    const struct intel_crtc_state
> > *crtc_state)
> > +{
> > +	return i915_find_pipe_conf(pipe, active_pipes,
> > +				   icl_allowed_dbufs,
> > +				   ARRAY_SIZE(icl_allowed_dbufs));
> > +}
> > +
> > +static u32 tgl_possible_dbuf_slices(int pipe,
> > +				    u32 active_pipes,
> > +				    const struct intel_crtc_state
> > *crtc_state)
> > +{
> > +	return i915_find_pipe_conf(pipe, active_pipes,
> > +				   tgl_allowed_dbufs,
> > +				   ARRAY_SIZE(tgl_allowed_dbufs));
> > +}
> 
> These wrappers aren't really doing anything for us.
> 
> > +
> > +u32 i915_possible_dbuf_slices(struct drm_i915_private *dev_priv,
> 
> dev_priv and pipe are redundant parameters. Both
> can be extracted from crtc_state. I would put crtc_state as the first
> parameter. Matches common practice more closely.
> 
> s/i915/skl/ in the function name.
> 
> > +			      int pipe, u32 active_pipes,
> > +			      const struct intel_crtc_state
> > *crtc_state)
> > +{
> > +	if (IS_GEN(dev_priv, 11))
> > +		return icl_possible_dbuf_slices(pipe,
> > +						active_pipes,
> > +						crtc_state);
> > +	else if (IS_GEN(dev_priv, 12))
> > +		return tgl_possible_dbuf_slices(pipe,
> > +						active_pipes,
> > +						crtc_state);
> 
> The established convention is to go from new platforms
> to old platforms, if possible.
> 
> > +	/*
> > +	 * For anything else just return one slice yet.
> > +	 * Should be extended for other platforms.
> > +	 */
> > +	return DBUF_S1_BIT;
> > +}
> > +
> >  static u64
> >  skl_plane_relative_data_rate(const struct intel_crtc_state
> > *crtc_state,
> >  			     const struct intel_plane_state
> > *plane_state,
> > -- 
> > 2.17.1
> 
>
Stanislav Lisovskiy Dec. 19, 2019, 12:02 p.m. UTC | #5
On Wed, 2019-12-18 at 20:05 +0200, Ville Syrjälä wrote:
> On Fri, Dec 13, 2019 at 03:02:28PM +0200, Stanislav Lisovskiy wrote:
> > Added proper DBuf slice mapping to correspondent
> > pipes, depending on pipe configuration as stated
> > in BSpec.
> > 
> > v2:
> >     - Remove unneeded braces
> >     - Stop using macro for DBuf assignments as
> >       it seems to reduce readability.
> > 
> > v3: Start using enabled slices mask in dev_priv
> > 
> > v4: Renamed "enabled_slices" used in dev_priv
> >     to "enabled_dbuf_slices_mask"(Matt Roper)
> > 
> > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_pm.c | 226
> > ++++++++++++++++++++++++++++++--
> >  1 file changed, 216 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> > b/drivers/gpu/drm/i915/intel_pm.c
> > index 111bcafd6e4c..a13052b2c2ef 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -3832,13 +3832,30 @@ bool intel_can_enable_sagv(struct
> > intel_atomic_state *state)
> >  	return true;
> >  }
> >  
> > +/*
> > + * Calculate initial DBuf slice offset, based on slice size
> > + * and mask(i.e if slice size is 1024 and second slice is enabled
> > + * offset would be 1024)
> > + */
> > +static u32 icl_get_first_dbuf_slice_offset(u32 dbuf_slice_mask,
> > +					   u32 slice_size, u32
> > ddb_size)
> 
> s/u32/unsigned int/ or something. No point in using sized types when
> we're not dealing with some thing that is actually that size.
> 
> > +{
> > +	u32 offset = 0;
> > +
> > +	if (!dbuf_slice_mask)
> > +		return 0;
> > +
> > +	offset = (ffs(dbuf_slice_mask) - 1) * slice_size;
> > +
> > +	WARN_ON(offset >= ddb_size);
> > +	return offset;
> > +}
> > +
> >  static u16 intel_get_ddb_size(struct drm_i915_private *dev_priv,
> >  			      const struct intel_crtc_state
> > *crtc_state,
> >  			      const u64 total_data_rate,
> >  			      const int num_active)
> >  {
> > -	struct drm_atomic_state *state = crtc_state->uapi.state;
> > -	struct intel_atomic_state *intel_state =
> > to_intel_atomic_state(state);
> >  	u16 ddb_size = INTEL_INFO(dev_priv)->ddb_size;
> >  
> >  	WARN_ON(ddb_size == 0);
> > @@ -3846,12 +3863,13 @@ static u16 intel_get_ddb_size(struct
> > drm_i915_private *dev_priv,
> >  	if (INTEL_GEN(dev_priv) < 11)
> >  		return ddb_size - 4; /* 4 blocks for bypass path
> > allocation */
> >  
> > -	intel_state->enabled_dbuf_slices_mask = DBUF_S1_BIT;
> > -	ddb_size /= 2;
> > -
> >  	return ddb_size;
> >  }
> >  
> > +u32 i915_possible_dbuf_slices(struct drm_i915_private *dev_priv,
> > +			      int pipe, u32 active_pipes,
> > +			      const struct intel_crtc_state
> > *crtc_state);
> > +
> >  static void
> >  skl_ddb_get_pipe_allocation_limits(struct drm_i915_private
> > *dev_priv,
> >  				   const struct intel_crtc_state
> > *crtc_state,
> > @@ -3866,7 +3884,14 @@ skl_ddb_get_pipe_allocation_limits(struct
> > drm_i915_private *dev_priv,
> >  	u32 pipe_width = 0, total_width = 0, width_before_pipe = 0;
> >  	enum pipe for_pipe = to_intel_crtc(for_crtc)->pipe;
> >  	u16 ddb_size;
> > +	u32 ddb_range_size;
> >  	u32 i;
> > +	u32 dbuf_slice_mask;
> > +	u32 active_pipes;
> > +	u32 offset;
> > +	u32 slice_size;
> > +	u32 total_slice_mask;
> > +	u32 start, end;
> >  
> >  	if (WARN_ON(!state) || !crtc_state->hw.active) {
> >  		alloc->start = 0;
> > @@ -3875,14 +3900,19 @@ skl_ddb_get_pipe_allocation_limits(struct
> > drm_i915_private *dev_priv,
> >  		return;
> >  	}
> >  
> > -	if (intel_state->active_pipe_changes)
> > +	if (intel_state->active_pipe_changes) {
> >  		*num_active = hweight8(intel_state->active_pipes);
> > -	else
> > +		active_pipes = intel_state->active_pipes;
> > +	} else {
> >  		*num_active = hweight8(dev_priv->active_pipes);
> > +		active_pipes = dev_priv->active_pipes;
> > +	}
> >  
> >  	ddb_size = intel_get_ddb_size(dev_priv, crtc_state,
> > total_data_rate,
> >  				      *num_active);
> >  
> > +	slice_size = ddb_size / INTEL_INFO(dev_priv)-
> > >num_supported_dbuf_slices;
> > +
> >  	/*
> >  	 * If the state doesn't change the active CRTC's or there is no
> >  	 * modeset request, then there's no need to recalculate;
> > @@ -3900,18 +3930,68 @@ skl_ddb_get_pipe_allocation_limits(struct
> > drm_i915_private *dev_priv,
> >  		return;
> >  	}
> >  
> > +	/*
> > +	 * Get allowed DBuf slices for correspondent pipe and platform.
> > +	 */
> > +	dbuf_slice_mask = i915_possible_dbuf_slices(dev_priv, for_pipe,
> > +						    active_pipes,
> > crtc_state);
> > +
> > +	DRM_DEBUG_KMS("DBuf slice mask %x pipe %d active pipes %x\n",
> > +		      dbuf_slice_mask,
> > +		      for_pipe, active_pipes);
> > +
> > +	/*
> > +	 * Figure out at which DBuf slice we start, i.e if we start at
> > Dbuf S2
> > +	 * and slice size is 1024, the offset would be 1024
> > +	 */
> > +	offset = icl_get_first_dbuf_slice_offset(dbuf_slice_mask,
> > +						 slice_size, ddb_size);
> > +
> > +	/*
> > +	 * Figure out total size of allowed DBuf slices, which is
> > basically
> > +	 * a number of allowed slices for that pipe multiplied by slice
> > size.
> > +	 * Inside of this
> > +	 * range ddb entries are still allocated in proportion to
> > display width.
> > +	 */
> > +	ddb_range_size = hweight8(dbuf_slice_mask) * slice_size;
> > +
> >  	/*
> >  	 * Watermark/ddb requirement highly depends upon width of the
> >  	 * framebuffer, So instead of allocating DDB equally among
> > pipes
> >  	 * distribute DDB based on resolution/width of the display.
> >  	 */
> > +	total_slice_mask = dbuf_slice_mask;
> >  	for_each_new_intel_crtc_in_state(intel_state, crtc, crtc_state,
> > i) {
> >  		const struct drm_display_mode *adjusted_mode =
> >  			&crtc_state->hw.adjusted_mode;
> >  		enum pipe pipe = crtc->pipe;
> >  		int hdisplay, vdisplay;
> > +		u32 pipe_dbuf_slice_mask =
> > +					i915_possible_dbuf_slices(dev_p
> > riv,
> > +								  pipe,
> > +								  activ
> > e_pipes,
> > +								  crtc_
> > state);
> > +
> > +		if (!crtc_state->hw.active)
> > +			continue;
> > +
> > +		/*
> > +		 * According to BSpec pipe can share one dbuf slice
> > with another
> > +		 * pipes or pipe can use multiple dbufs, in both cases
> > we
> > +		 * account for other pipes only if they have exactly
> > same mask.
> > +		 * However we need to account how many slices we should
> > enable
> > +		 * in total.
> > +		 */
> > +		total_slice_mask |= pipe_dbuf_slice_mask;
> >  
> > -		if (!crtc_state->hw.enable)
> > +		/*
> > +		 * Do not account pipes using other slice sets
> > +		 * luckily as of current BSpec slice sets do not
> > partially
> > +		 * intersect(pipes share either same one slice or same
> > slice set
> > +		 * i.e no partial intersection), so it is enough to
> > check for
> > +		 * equality for now.
> > +		 */
> > +		if (dbuf_slice_mask != pipe_dbuf_slice_mask)
> >  			continue;
> >  
> >  		drm_mode_get_hv_timing(adjusted_mode, &hdisplay,
> > &vdisplay);
> > @@ -3923,8 +4003,19 @@ skl_ddb_get_pipe_allocation_limits(struct
> > drm_i915_private *dev_priv,
> >  			pipe_width = hdisplay;
> >  	}
> >  
> > -	alloc->start = ddb_size * width_before_pipe / total_width;
> > -	alloc->end = ddb_size * (width_before_pipe + pipe_width) /
> > total_width;
> > +	intel_state->enabled_dbuf_slices_mask = total_slice_mask;
> > +
> > +	start = ddb_range_size * width_before_pipe / total_width;
> > +	end = ddb_range_size * (width_before_pipe + pipe_width) /
> > total_width;
> > +
> > +	alloc->start = offset + start;
> > +	alloc->end = offset + end;
> > +
> > +	DRM_DEBUG_KMS("Pipe %d ddb %d-%d\n", for_pipe,
> > +		      alloc->start, alloc->end);
> > +	DRM_DEBUG_KMS("Enabled ddb slices mask %x num supported %d\n",
> > +		      intel_state->enabled_dbuf_slices_mask,
> > +		      INTEL_INFO(dev_priv)->num_supported_dbuf_slices);
> >  }
> 
> That function really is quite the monster. Probably needs to be split
> up
> into sensible chunks... later.
> 
> >  
> >  static int skl_compute_wm_params(const struct intel_crtc_state
> > *crtc_state,
> > @@ -4094,6 +4185,121 @@ skl_plane_downscale_amount(const struct
> > intel_crtc_state *crtc_state,
> >  	return mul_fixed16(downscale_w, downscale_h);
> >  }
> >  
> > +struct dbuf_slice_conf_entry {
> > +	u32 active_pipes;
> > +	u32 dbuf_mask[I915_MAX_PIPES];
> 
> u8 will do for everything.
> 
> > +};
> > +
> > +/*
> > + * Table taken from Bspec 12716
> > + * Pipes do have some preferred DBuf slice affinity,
> > + * plus there are some hardcoded requirements on how
> > + * those should be distributed for multipipe scenarios.
> > + * For more DBuf slices algorithm can get even more messy
> > + * and less readable, so decided to use a table almost
> > + * as is from BSpec itself - that way it is at least easier
> > + * to compare, change and check.
> > + */
> > +static struct dbuf_slice_conf_entry icl_allowed_dbufs[] = {
> > +	{ BIT(PIPE_A), { DBUF_S1_BIT, 0, 0, 0 } },
> 
> Still don't like that DBUF_S1_BIT. I still think it should be an
> enum dbuf_slice + explcit BIT(DBUF_S1) where needed.
> 
> And please use named initializers instead of sprinkling zeroes.
> IMO this should just look something like:
> 
> 	{ .active_pipes = BIT(PIPE_A) | BIT(PIPE_B),
> 	  .dbuf_slices = { [PIPE_A] = BIT(DBUF_S1),
> 	  		   [PIPE_B] = BIT(DBUF_S2),
> 			 },
> 	},
> 
> > +	{ BIT(PIPE_B), { 0, DBUF_S1_BIT, 0, 0 } },
> > +	{ BIT(PIPE_C), { 0, 0, DBUF_S2_BIT, 0 } },
> > +	{ BIT(PIPE_A) | BIT(PIPE_B), { DBUF_S1_BIT, DBUF_S2_BIT, 0, 0 }
> > },
> > +	{ BIT(PIPE_A) | BIT(PIPE_C), { DBUF_S1_BIT, 0, DBUF_S2_BIT, 0 }
> > },
> > +	{ BIT(PIPE_B) | BIT(PIPE_C), { 0, DBUF_S1_BIT, DBUF_S2_BIT, 0 }
> > },
> > +	{ BIT(PIPE_A) | BIT(PIPE_B) | BIT(PIPE_C),
> > +		{ DBUF_S1_BIT, DBUF_S1_BIT, DBUF_S2_BIT, 0 } }
> > +};
> > +
> > +/*
> > + * Table taken from Bspec 49255
> > + * Pipes do have some preferred DBuf slice affinity,
> > + * plus there are some hardcoded requirements on how
> > + * those should be distributed for multipipe scenarios.
> > + * For more DBuf slices algorithm can get even more messy
> > + * and less readable, so decided to use a table almost
> > + * as is from BSpec itself - that way it is at least easier
> > + * to compare, change and check.
> > + */
> > +static struct dbuf_slice_conf_entry tgl_allowed_dbufs[] = {
> > +	{ BIT(PIPE_A), { DBUF_S1_BIT | DBUF_S2_BIT, 0, 0, 0 } },
> > +	{ BIT(PIPE_B), { 0, DBUF_S1_BIT | DBUF_S2_BIT, 0, 0 } },
> > +	{ BIT(PIPE_C), { 0, 0, DBUF_S1_BIT | DBUF_S2_BIT, 0 } },
> > +	{ BIT(PIPE_D), { 0, 0, 0, DBUF_S1_BIT | DBUF_S2_BIT } },
> > +	{ BIT(PIPE_A) | BIT(PIPE_B), { DBUF_S2_BIT, DBUF_S1_BIT, 0, 0 }
> > },
> > +	{ BIT(PIPE_A) | BIT(PIPE_C), { DBUF_S1_BIT, 0, DBUF_S2_BIT, 0 }
> > },
> > +	{ BIT(PIPE_A) | BIT(PIPE_D), { DBUF_S1_BIT, 0, 0, DBUF_S2_BIT }
> > },
> > +	{ BIT(PIPE_B) | BIT(PIPE_C), { 0, DBUF_S1_BIT, DBUF_S2_BIT, 0 }
> > },
> > +	{ BIT(PIPE_B) | BIT(PIPE_D), { 0, DBUF_S1_BIT, 0, DBUF_S2_BIT }
> > },
> > +	{ BIT(PIPE_C) | BIT(PIPE_D), { 0, 0, DBUF_S2_BIT, DBUF_S2_BIT }
> > },
> 
> I believe pipe C should use S1 here.

Ah it was actually already fixed in recent revision. Still we do need
to generate those with a tool I think.. I didn't use it here - and that
was the outcome..

The tools is here(wink): 
https://patchwork.freedesktop.org/series/70493/

Stan

> 
> > +	{ BIT(PIPE_A) | BIT(PIPE_B) | BIT(PIPE_C),
> > +		{ DBUF_S1_BIT, DBUF_S1_BIT, DBUF_S2_BIT, 0 } },
> > +	{ BIT(PIPE_A) | BIT(PIPE_B) | BIT(PIPE_D),
> > +		{ DBUF_S1_BIT, DBUF_S1_BIT, 0, DBUF_S2_BIT } },
> > +	{ BIT(PIPE_A) | BIT(PIPE_C) | BIT(PIPE_D),
> > +		{ DBUF_S1_BIT, 0, DBUF_S2_BIT, DBUF_S2_BIT } },
> > +	{ BIT(PIPE_B) | BIT(PIPE_C) | BIT(PIPE_D),
> > +		{ 0, DBUF_S1_BIT, DBUF_S2_BIT, DBUF_S2_BIT } },
> > +	{ BIT(PIPE_A) | BIT(PIPE_B) | BIT(PIPE_C) | BIT(PIPE_D),
> > +		{ DBUF_S1_BIT, DBUF_S1_BIT, DBUF_S2_BIT, DBUF_S2_BIT }
> > },
> > +};
> > +
> > +static u32 i915_find_pipe_conf(int pipe,
> 
> enum pipe pipe
> 
> > +			       u32 active_pipes,
> > +			       const struct dbuf_slice_conf_entry
> > *dbuf_slices,
> > +			       int size)
> 
> Could just add a sentinel to the end of the arrays and avoid this
> 'size' stuff.
> 
> The function name is still very confusing. icl_compute_dbuf_slices()
> or something perhaps. Or even without a prefix since it's static
> anyway.
> 
> > +{
> > +	int i;
> > +
> > +	for (i = 0; i < size; i++) {
> > +		if (dbuf_slices[i].active_pipes == active_pipes)
> > +			return dbuf_slices[i].dbuf_mask[pipe];
> > +	}
> > +	return 0;
> > +}
> > +
> > +/*
> > + * This function finds an entry with same enabled pipe
> > configuration and
> > + * returns correspondent DBuf slice mask as stated in BSpec for
> > particular
> > + * platform.
> > + */
> > +static u32 icl_possible_dbuf_slices(int pipe,
> > +				    u32 active_pipes,
> > +				    const struct intel_crtc_state
> > *crtc_state)
> > +{
> > +	return i915_find_pipe_conf(pipe, active_pipes,
> > +				   icl_allowed_dbufs,
> > +				   ARRAY_SIZE(icl_allowed_dbufs));
> > +}
> > +
> > +static u32 tgl_possible_dbuf_slices(int pipe,
> > +				    u32 active_pipes,
> > +				    const struct intel_crtc_state
> > *crtc_state)
> > +{
> > +	return i915_find_pipe_conf(pipe, active_pipes,
> > +				   tgl_allowed_dbufs,
> > +				   ARRAY_SIZE(tgl_allowed_dbufs));
> > +}
> 
> These wrappers aren't really doing anything for us.
> 
> > +
> > +u32 i915_possible_dbuf_slices(struct drm_i915_private *dev_priv,
> 
> dev_priv and pipe are redundant parameters. Both
> can be extracted from crtc_state. I would put crtc_state as the first
> parameter. Matches common practice more closely.
> 
> s/i915/skl/ in the function name.
> 
> > +			      int pipe, u32 active_pipes,
> > +			      const struct intel_crtc_state
> > *crtc_state)
> > +{
> > +	if (IS_GEN(dev_priv, 11))
> > +		return icl_possible_dbuf_slices(pipe,
> > +						active_pipes,
> > +						crtc_state);
> > +	else if (IS_GEN(dev_priv, 12))
> > +		return tgl_possible_dbuf_slices(pipe,
> > +						active_pipes,
> > +						crtc_state);
> 
> The established convention is to go from new platforms
> to old platforms, if possible.
> 
> > +	/*
> > +	 * For anything else just return one slice yet.
> > +	 * Should be extended for other platforms.
> > +	 */
> > +	return DBUF_S1_BIT;
> > +}
> > +
> >  static u64
> >  skl_plane_relative_data_rate(const struct intel_crtc_state
> > *crtc_state,
> >  			     const struct intel_plane_state
> > *plane_state,
> > -- 
> > 2.17.1
> 
>

Patch
diff mbox series

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 111bcafd6e4c..a13052b2c2ef 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3832,13 +3832,30 @@  bool intel_can_enable_sagv(struct intel_atomic_state *state)
 	return true;
 }
 
+/*
+ * Calculate initial DBuf slice offset, based on slice size
+ * and mask(i.e if slice size is 1024 and second slice is enabled
+ * offset would be 1024)
+ */
+static u32 icl_get_first_dbuf_slice_offset(u32 dbuf_slice_mask,
+					   u32 slice_size, u32 ddb_size)
+{
+	u32 offset = 0;
+
+	if (!dbuf_slice_mask)
+		return 0;
+
+	offset = (ffs(dbuf_slice_mask) - 1) * slice_size;
+
+	WARN_ON(offset >= ddb_size);
+	return offset;
+}
+
 static u16 intel_get_ddb_size(struct drm_i915_private *dev_priv,
 			      const struct intel_crtc_state *crtc_state,
 			      const u64 total_data_rate,
 			      const int num_active)
 {
-	struct drm_atomic_state *state = crtc_state->uapi.state;
-	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
 	u16 ddb_size = INTEL_INFO(dev_priv)->ddb_size;
 
 	WARN_ON(ddb_size == 0);
@@ -3846,12 +3863,13 @@  static u16 intel_get_ddb_size(struct drm_i915_private *dev_priv,
 	if (INTEL_GEN(dev_priv) < 11)
 		return ddb_size - 4; /* 4 blocks for bypass path allocation */
 
-	intel_state->enabled_dbuf_slices_mask = DBUF_S1_BIT;
-	ddb_size /= 2;
-
 	return ddb_size;
 }
 
+u32 i915_possible_dbuf_slices(struct drm_i915_private *dev_priv,
+			      int pipe, u32 active_pipes,
+			      const struct intel_crtc_state *crtc_state);
+
 static void
 skl_ddb_get_pipe_allocation_limits(struct drm_i915_private *dev_priv,
 				   const struct intel_crtc_state *crtc_state,
@@ -3866,7 +3884,14 @@  skl_ddb_get_pipe_allocation_limits(struct drm_i915_private *dev_priv,
 	u32 pipe_width = 0, total_width = 0, width_before_pipe = 0;
 	enum pipe for_pipe = to_intel_crtc(for_crtc)->pipe;
 	u16 ddb_size;
+	u32 ddb_range_size;
 	u32 i;
+	u32 dbuf_slice_mask;
+	u32 active_pipes;
+	u32 offset;
+	u32 slice_size;
+	u32 total_slice_mask;
+	u32 start, end;
 
 	if (WARN_ON(!state) || !crtc_state->hw.active) {
 		alloc->start = 0;
@@ -3875,14 +3900,19 @@  skl_ddb_get_pipe_allocation_limits(struct drm_i915_private *dev_priv,
 		return;
 	}
 
-	if (intel_state->active_pipe_changes)
+	if (intel_state->active_pipe_changes) {
 		*num_active = hweight8(intel_state->active_pipes);
-	else
+		active_pipes = intel_state->active_pipes;
+	} else {
 		*num_active = hweight8(dev_priv->active_pipes);
+		active_pipes = dev_priv->active_pipes;
+	}
 
 	ddb_size = intel_get_ddb_size(dev_priv, crtc_state, total_data_rate,
 				      *num_active);
 
+	slice_size = ddb_size / INTEL_INFO(dev_priv)->num_supported_dbuf_slices;
+
 	/*
 	 * If the state doesn't change the active CRTC's or there is no
 	 * modeset request, then there's no need to recalculate;
@@ -3900,18 +3930,68 @@  skl_ddb_get_pipe_allocation_limits(struct drm_i915_private *dev_priv,
 		return;
 	}
 
+	/*
+	 * Get allowed DBuf slices for correspondent pipe and platform.
+	 */
+	dbuf_slice_mask = i915_possible_dbuf_slices(dev_priv, for_pipe,
+						    active_pipes, crtc_state);
+
+	DRM_DEBUG_KMS("DBuf slice mask %x pipe %d active pipes %x\n",
+		      dbuf_slice_mask,
+		      for_pipe, active_pipes);
+
+	/*
+	 * Figure out at which DBuf slice we start, i.e if we start at Dbuf S2
+	 * and slice size is 1024, the offset would be 1024
+	 */
+	offset = icl_get_first_dbuf_slice_offset(dbuf_slice_mask,
+						 slice_size, ddb_size);
+
+	/*
+	 * Figure out total size of allowed DBuf slices, which is basically
+	 * a number of allowed slices for that pipe multiplied by slice size.
+	 * Inside of this
+	 * range ddb entries are still allocated in proportion to display width.
+	 */
+	ddb_range_size = hweight8(dbuf_slice_mask) * slice_size;
+
 	/*
 	 * Watermark/ddb requirement highly depends upon width of the
 	 * framebuffer, So instead of allocating DDB equally among pipes
 	 * distribute DDB based on resolution/width of the display.
 	 */
+	total_slice_mask = dbuf_slice_mask;
 	for_each_new_intel_crtc_in_state(intel_state, crtc, crtc_state, i) {
 		const struct drm_display_mode *adjusted_mode =
 			&crtc_state->hw.adjusted_mode;
 		enum pipe pipe = crtc->pipe;
 		int hdisplay, vdisplay;
+		u32 pipe_dbuf_slice_mask =
+					i915_possible_dbuf_slices(dev_priv,
+								  pipe,
+								  active_pipes,
+								  crtc_state);
+
+		if (!crtc_state->hw.active)
+			continue;
+
+		/*
+		 * According to BSpec pipe can share one dbuf slice with another
+		 * pipes or pipe can use multiple dbufs, in both cases we
+		 * account for other pipes only if they have exactly same mask.
+		 * However we need to account how many slices we should enable
+		 * in total.
+		 */
+		total_slice_mask |= pipe_dbuf_slice_mask;
 
-		if (!crtc_state->hw.enable)
+		/*
+		 * Do not account pipes using other slice sets
+		 * luckily as of current BSpec slice sets do not partially
+		 * intersect(pipes share either same one slice or same slice set
+		 * i.e no partial intersection), so it is enough to check for
+		 * equality for now.
+		 */
+		if (dbuf_slice_mask != pipe_dbuf_slice_mask)
 			continue;
 
 		drm_mode_get_hv_timing(adjusted_mode, &hdisplay, &vdisplay);
@@ -3923,8 +4003,19 @@  skl_ddb_get_pipe_allocation_limits(struct drm_i915_private *dev_priv,
 			pipe_width = hdisplay;
 	}
 
-	alloc->start = ddb_size * width_before_pipe / total_width;
-	alloc->end = ddb_size * (width_before_pipe + pipe_width) / total_width;
+	intel_state->enabled_dbuf_slices_mask = total_slice_mask;
+
+	start = ddb_range_size * width_before_pipe / total_width;
+	end = ddb_range_size * (width_before_pipe + pipe_width) / total_width;
+
+	alloc->start = offset + start;
+	alloc->end = offset + end;
+
+	DRM_DEBUG_KMS("Pipe %d ddb %d-%d\n", for_pipe,
+		      alloc->start, alloc->end);
+	DRM_DEBUG_KMS("Enabled ddb slices mask %x num supported %d\n",
+		      intel_state->enabled_dbuf_slices_mask,
+		      INTEL_INFO(dev_priv)->num_supported_dbuf_slices);
 }
 
 static int skl_compute_wm_params(const struct intel_crtc_state *crtc_state,
@@ -4094,6 +4185,121 @@  skl_plane_downscale_amount(const struct intel_crtc_state *crtc_state,
 	return mul_fixed16(downscale_w, downscale_h);
 }
 
+struct dbuf_slice_conf_entry {
+	u32 active_pipes;
+	u32 dbuf_mask[I915_MAX_PIPES];
+};
+
+/*
+ * Table taken from Bspec 12716
+ * Pipes do have some preferred DBuf slice affinity,
+ * plus there are some hardcoded requirements on how
+ * those should be distributed for multipipe scenarios.
+ * For more DBuf slices algorithm can get even more messy
+ * and less readable, so decided to use a table almost
+ * as is from BSpec itself - that way it is at least easier
+ * to compare, change and check.
+ */
+static struct dbuf_slice_conf_entry icl_allowed_dbufs[] = {
+	{ BIT(PIPE_A), { DBUF_S1_BIT, 0, 0, 0 } },
+	{ BIT(PIPE_B), { 0, DBUF_S1_BIT, 0, 0 } },
+	{ BIT(PIPE_C), { 0, 0, DBUF_S2_BIT, 0 } },
+	{ BIT(PIPE_A) | BIT(PIPE_B), { DBUF_S1_BIT, DBUF_S2_BIT, 0, 0 } },
+	{ BIT(PIPE_A) | BIT(PIPE_C), { DBUF_S1_BIT, 0, DBUF_S2_BIT, 0 } },
+	{ BIT(PIPE_B) | BIT(PIPE_C), { 0, DBUF_S1_BIT, DBUF_S2_BIT, 0 } },
+	{ BIT(PIPE_A) | BIT(PIPE_B) | BIT(PIPE_C),
+		{ DBUF_S1_BIT, DBUF_S1_BIT, DBUF_S2_BIT, 0 } }
+};
+
+/*
+ * Table taken from Bspec 49255
+ * Pipes do have some preferred DBuf slice affinity,
+ * plus there are some hardcoded requirements on how
+ * those should be distributed for multipipe scenarios.
+ * For more DBuf slices algorithm can get even more messy
+ * and less readable, so decided to use a table almost
+ * as is from BSpec itself - that way it is at least easier
+ * to compare, change and check.
+ */
+static struct dbuf_slice_conf_entry tgl_allowed_dbufs[] = {
+	{ BIT(PIPE_A), { DBUF_S1_BIT | DBUF_S2_BIT, 0, 0, 0 } },
+	{ BIT(PIPE_B), { 0, DBUF_S1_BIT | DBUF_S2_BIT, 0, 0 } },
+	{ BIT(PIPE_C), { 0, 0, DBUF_S1_BIT | DBUF_S2_BIT, 0 } },
+	{ BIT(PIPE_D), { 0, 0, 0, DBUF_S1_BIT | DBUF_S2_BIT } },
+	{ BIT(PIPE_A) | BIT(PIPE_B), { DBUF_S2_BIT, DBUF_S1_BIT, 0, 0 } },
+	{ BIT(PIPE_A) | BIT(PIPE_C), { DBUF_S1_BIT, 0, DBUF_S2_BIT, 0 } },
+	{ BIT(PIPE_A) | BIT(PIPE_D), { DBUF_S1_BIT, 0, 0, DBUF_S2_BIT } },
+	{ BIT(PIPE_B) | BIT(PIPE_C), { 0, DBUF_S1_BIT, DBUF_S2_BIT, 0 } },
+	{ BIT(PIPE_B) | BIT(PIPE_D), { 0, DBUF_S1_BIT, 0, DBUF_S2_BIT } },
+	{ BIT(PIPE_C) | BIT(PIPE_D), { 0, 0, DBUF_S2_BIT, DBUF_S2_BIT } },
+	{ BIT(PIPE_A) | BIT(PIPE_B) | BIT(PIPE_C),
+		{ DBUF_S1_BIT, DBUF_S1_BIT, DBUF_S2_BIT, 0 } },
+	{ BIT(PIPE_A) | BIT(PIPE_B) | BIT(PIPE_D),
+		{ DBUF_S1_BIT, DBUF_S1_BIT, 0, DBUF_S2_BIT } },
+	{ BIT(PIPE_A) | BIT(PIPE_C) | BIT(PIPE_D),
+		{ DBUF_S1_BIT, 0, DBUF_S2_BIT, DBUF_S2_BIT } },
+	{ BIT(PIPE_B) | BIT(PIPE_C) | BIT(PIPE_D),
+		{ 0, DBUF_S1_BIT, DBUF_S2_BIT, DBUF_S2_BIT } },
+	{ BIT(PIPE_A) | BIT(PIPE_B) | BIT(PIPE_C) | BIT(PIPE_D),
+		{ DBUF_S1_BIT, DBUF_S1_BIT, DBUF_S2_BIT, DBUF_S2_BIT } },
+};
+
+static u32 i915_find_pipe_conf(int pipe,
+			       u32 active_pipes,
+			       const struct dbuf_slice_conf_entry *dbuf_slices,
+			       int size)
+{
+	int i;
+
+	for (i = 0; i < size; i++) {
+		if (dbuf_slices[i].active_pipes == active_pipes)
+			return dbuf_slices[i].dbuf_mask[pipe];
+	}
+	return 0;
+}
+
+/*
+ * This function finds an entry with same enabled pipe configuration and
+ * returns correspondent DBuf slice mask as stated in BSpec for particular
+ * platform.
+ */
+static u32 icl_possible_dbuf_slices(int pipe,
+				    u32 active_pipes,
+				    const struct intel_crtc_state *crtc_state)
+{
+	return i915_find_pipe_conf(pipe, active_pipes,
+				   icl_allowed_dbufs,
+				   ARRAY_SIZE(icl_allowed_dbufs));
+}
+
+static u32 tgl_possible_dbuf_slices(int pipe,
+				    u32 active_pipes,
+				    const struct intel_crtc_state *crtc_state)
+{
+	return i915_find_pipe_conf(pipe, active_pipes,
+				   tgl_allowed_dbufs,
+				   ARRAY_SIZE(tgl_allowed_dbufs));
+}
+
+u32 i915_possible_dbuf_slices(struct drm_i915_private *dev_priv,
+			      int pipe, u32 active_pipes,
+			      const struct intel_crtc_state *crtc_state)
+{
+	if (IS_GEN(dev_priv, 11))
+		return icl_possible_dbuf_slices(pipe,
+						active_pipes,
+						crtc_state);
+	else if (IS_GEN(dev_priv, 12))
+		return tgl_possible_dbuf_slices(pipe,
+						active_pipes,
+						crtc_state);
+	/*
+	 * For anything else just return one slice yet.
+	 * Should be extended for other platforms.
+	 */
+	return DBUF_S1_BIT;
+}
+
 static u64
 skl_plane_relative_data_rate(const struct intel_crtc_state *crtc_state,
 			     const struct intel_plane_state *plane_state,