[Intel-gfx] [PATCH v2 05/20] drm/i915: Make skl_compute_dbuf_slices() behave consistently for all platforms

[Lisovskiy, Stanislav]

On Mon, 2020-03-02 at 16:50 +0200, Ville Syrjälä wrote:
> On Tue, Feb 25, 2020 at 05:30:57PM +0000, Lisovskiy, Stanislav wrote:
> > On Tue, 2020-02-25 at 19:11 +0200, Ville Syrjala wrote:
> > > From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > 
> > > Currently skl_compute_dbuf_slices() returns 0 for any inactive
> > > pipe
> > > on
> > > icl+, but returns BIT(S1) on pre-icl for any pipe (whether it's
> > > active or
> > > not). Let's make the behaviour consistent and always return 0 for
> > > any
> > > inactive pipe.
> > > 
> > > Cc: Stanislav Lisovskiy <stanislav.lisovskiy at intel.com>
> > > Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_pm.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> > > b/drivers/gpu/drm/i915/intel_pm.c
> > > index a2e78969c0df..640f4c4fd508 100644
> > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > @@ -4408,7 +4408,7 @@ static u8 skl_compute_dbuf_slices(const
> > > struct
> > > intel_crtc_state *crtc_state,
> > >  	 * For anything else just return one slice yet.
> > >  	 * Should be extended for other platforms.
> > >  	 */
> > > -	return BIT(DBUF_S1);
> > > +	return active_pipes & BIT(pipe) ? BIT(DBUF_S1) : 0;
> > 
> > I think the initial idea was this won't be even called if there 
> > are no active pipes at all - skl_ddb_get_pipe_allocation_limits
> > would
> > bail out immediately. If there were some active pipes - then we
> > will
> > have to use slice S1 anyway - because there were simply no other
> > slices
> > available. If some pipes were inactive - they are currently skipped
> > by
> > !crtc_state->hw.active check - so I would just keep it simple and
> > don't
> > call this function for non-active pipes at all.
> 
> That's just going to make the caller more messy by forcing it to
> check for active_pipes 0 vs. not. Ie. we'd be splitting the
> responsibility of computing the dbuf slices for this pipe between
> skl_compute_dbuf_slices() and its caller. Not a good idea IMO.

Well, in that sense I agree. Currently it is just that this check is
anyway there when you get ddb allocation limits. 

Could this be actually even nicer to get one more very simple table for
everything "before-gen11"? We would have it then in a quite unified
looking way.

Stan


>