| Message ID | 20190121153143.13604-1-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Don't use the second dbuf slice on icl | expand |
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 change in intel_get_ddb_size will take care of enabling only one slice. > > 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 were already doing it and disabling slice only after update_crtcs, and skl_update_crtc code is taking care of waiting for vblank in case it's required. > + * - 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
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. > > 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. > + * - 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
On Tue, Jan 22, 2019 at 02:49:13PM +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, It's easier to pretend that we have only a single slice. Not sure for instance if the lack of the above would lead to state check errors (due to other problems in the dbuf silce implementation). > If we return from above we'll never disable > second slice in case it's enabled by bios. We'll disable whenever we need to, that is during suspend/driver unload. We can ignore the rest of the cases I think. > Anyhow code change in intel_get_ddb_size will take care of enabling > only one slice. > > > > > 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 were already doing it and disabling slice only after > update_crtcs, and skl_update_crtc code is taking care of waiting for > vblank in case it's required. > > > + * - 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 > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Mon, Jan 21, 2019 at 05:31:43PM +0200, Ville Syrjala 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> Reviewed-by: Imre Deak <imre.deak@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; > > 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 > + * - plane straddling both slices is illegal in multi-pipe scenarios > + * - 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
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
Hi, On Fri, Jan 25, 2019 at 8:38 PM Imre Deak <imre.deak@intel.com> wrote: > > On Tue, Jan 22, 2019 at 02:49:13PM +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, > > It's easier to pretend that we have only a single slice. Not sure for > instance if the lack of the above would lead to state check errors (due > to other problems in the dbuf silce implementation). > > > If we return from above we'll never disable > > second slice in case it's enabled by bios. > > We'll disable whenever we need to, that is during suspend/driver unload. > We can ignore the rest of the cases I think. exactly, It will get disabled during suspend/unload, but as we are not using second slice, it's going to consume some power until then. This function is to tell us H/W state, So we should assume that second slice is disabled without checking H/W state. ~Mahesh > > > Anyhow code change in intel_get_ddb_size will take care of enabling > > only one slice. > > > > > > > > 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 were already doing it and disabling slice only after > > update_crtcs, and skl_update_crtc code is taking care of waiting for > > vblank in case it's required. > > > > > + * - 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 > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Hi, On Fri, Jan 25, 2019 at 8:58 PM Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > > 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. This will not cause any S/W state warning. Only H/W slice will not get disabled until suspend, If enabled by bios. > > > > > > > > > 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. hmm agree, I think I missed that part. although we'll disable the slice only after updating the pipe, yet it'll not wait for vblank before disabling slice. that part need fixing :) ~Mahesh > > > > > > + * - 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
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; 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 + * - plane straddling both slices is illegal in multi-pipe scenarios + * - 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;