Re: [PATCH] drm/i915: Don't use the second dbuf slice on icl

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

On Tue, Jan 22, 2019 at 02:58:46PM +0530, Mahesh Kumar wrote:
> Hi,
> 
> On Mon, Jan 21, 2019 at 9:01 PM Ville Syrjala
> <ville.syrjala@linux.intel.com> wrote:
> >
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > The code managing the dbuf slices is borked and needs some
> > real work to fix. In the meantime let's just stop using the
> > second slice.
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_pm.c | 10 ++++++++--
> >  1 file changed, 8 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index 8b63afa3a221..1e41c899ffe2 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -3618,7 +3618,8 @@ static u8 intel_enabled_dbuf_slices_num(struct drm_i915_private *dev_priv)
> >         enabled_slices = 1;
> >
> >         /* Gen prior to GEN11 have only one DBuf slice */
> > -       if (INTEL_GEN(dev_priv) < 11)
> > +       /* FIXME dbuf slice code is broken: see intel_get_ddb_size() */
> > +       if (1 || INTEL_GEN(dev_priv) < 11)
> >                 return enabled_slices;
> 
> IMHO we may not need this, If we return from above we'll never disable
> second slice in case it's enabled by bios.
> Anyhow code changes in intel_get_ddb_size will take care of enabling
> only one slice.

I suppose. Can't recall if leaving this out caused more warns due
something else getting confused.

> 
> >
> >         if (I915_READ(DBUF_CTL_S2) & DBUF_POWER_STATE)
> > @@ -3827,8 +3828,13 @@ static u16 intel_get_ddb_size(struct drm_i915_private *dev_priv,
> >
> >         /*
> >          * 12GB/s is maximum BW supported by single DBuf slice.
> > +        *
> > +        * FIXME dbuf slice code is broken:
> > +        * - must wait for planes to stop using the slice before powering it off
> 
> AFAIR we're  already doing it and disabling slice only after
> update_crtcs, and skl_update_crtcs code is taking care of waiting for
> vblank in case it's required.

skl_update_crtcs() only deals with reallocation between pipes. It won't
do anything when a single pipe is going from two slices to one slice.

> 
> > +        * - plane straddling both slices is illegal in multi-pipe scenarios
> 
> This is something new :)
> 
> although this change introduce a major limitation with number and size
> of planes we can display, yet
> As code is broken and mentioned conditions need to be taken care of,
> This change should be ok until proper fix.
> 
> ~Mahesh
> 
> > +        * - should validate we stay within the hw bandwidth limits
> >          */
> > -       if (num_active > 1 || total_data_bw >= GBps(12)) {
> > +       if (0 && (num_active > 1 || total_data_bw >= GBps(12))) {
> >                 ddb->enabled_slices = 2;
> >         } else {
> >                 ddb->enabled_slices = 1;
> > --
> > 2.19.2
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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