Re: [Intel-gfx] [PATCH] drm/i915: Don't use the second dbuf slice on icl
Hi,
On Fri, Jan 25, 2019 at 8:58 PM Ville Syrjälä
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
> > wrote:
> > >
> > > From: Ville Syrjälä
> > >
> > > 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ä
> > > ---
> > > 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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Don't use the second dbuf slice on icl
Hi,
On Fri, Jan 25, 2019 at 8:38 PM Imre Deak 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
> > wrote:
> >
> > From: Ville Syrjälä
> >
> > 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ä
> > ---
> > 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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Don't use the second dbuf slice on icl
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
> wrote:
> >
> > From: Ville Syrjälä
> >
> > 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ä
> > ---
> > 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
Re: [Intel-gfx] [PATCH] drm/i915: Don't use the second dbuf slice on icl
On Mon, Jan 21, 2019 at 05:31:43PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä
>
> 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ä
Reviewed-by: Imre Deak
> ---
> 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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Don't use the second dbuf slice on icl
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
> wrote:
>
> From: Ville Syrjälä
>
> 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ä
> ---
> 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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Don't use the second dbuf slice on icl
Hi,
On Mon, Jan 21, 2019 at 9:01 PM Ville Syrjala
wrote:
>
> From: Ville Syrjälä
>
> 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ä
> ---
> 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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Don't use the second dbuf slice on icl
Hi,
On Mon, Jan 21, 2019 at 9:01 PM Ville Syrjala
wrote:
From: Ville Syrjälä
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ä
---
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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
