Re: [Intel-gfx] [PATCH 1/1] drm/i915: re-disable RC6p on Sandy Bridge
On Wed, Dec 21, 2022 at 09:09:17AM -0500, Rodrigo Vivi wrote:
> On Tue, Dec 20, 2022 at 12:32:05PM +0200, Ville Syrjälä wrote:
> > On Mon, Dec 19, 2022 at 06:29:27PM +0100, Sasa Dragic wrote:
> > > RC6p on Sandy Bridge got re-enabled over time, causing visual glitches
> > > and GPU hangs.
> > >
> > > Disabled originally in commit 1c8ecf80fdee ("drm/i915: do not enable
> > > RC6p on Sandy Bridge").
> >
> > re cover letter:
> > > It was originally disabled in commit 1c8ecf80fdee ("drm/i915: do not
> > > enable RC6p on Sandy Bridge"), and subsequently re-enabled by
> > > 13c5a577b342 ("drm/i915/gt: Select the deepest available parking mode
> > > for rc6"), which seems to focus only on Ivy Bridge.
> >
> > That only kicks in while parked (ie. fully idle from
> > software POV). I think the real bad commit is
> > fb6db0f5bf1d ("drm/i915: Remove unsafe i915.enable_rc6")
> > which seems to affects which rc6 level is selected while
> > unparked.
> >
> > We should mention both of those commits here:
> > Fixes: fb6db0f5bf1d ("drm/i915: Remove unsafe i915.enable_rc6")
> > Fixes: 13c5a577b342 ("drm/i915/gt: Select the deepest available parking
> > mode for rc6")
> >
> > Also we want
> > Cc: sta...@vger.kernel.org
> >
> > We can add those while pushing, so no need to resend for that.
>
> agreed.
>
> Reviewed-by: Rodrigo Vivi
Pushed to drm-intel-gt-next. Thanks for the patch and review.
>
> >
> > >
> > > Signed-off-by: Sasa Dragic
> > > ---
> > > drivers/gpu/drm/i915/i915_pci.c | 3 ++-
> > > 1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_pci.c
> > > b/drivers/gpu/drm/i915/i915_pci.c
> > > index 668e9da52584..69377564028a 100644
> > > --- a/drivers/gpu/drm/i915/i915_pci.c
> > > +++ b/drivers/gpu/drm/i915/i915_pci.c
> > > @@ -423,7 +423,8 @@ static const struct intel_device_info ilk_m_info = {
> > > .has_coherent_ggtt = true, \
> > > .has_llc = 1, \
> > > .has_rc6 = 1, \
> > > - .has_rc6p = 1, \
> > > + /* snb does support rc6p, but enabling it causes various issues */ \
> > > + .has_rc6p = 0, \
> >
> > The one downside of doing it this way is that we also lose
> > the debugfs/sysfs files so we can't monitor rc6+/++
> > residency anymore to make sure they are not entered.
> >
> > I think ideally we'd split this into two parts: which rc6
> > states the hw actually has vs. which rc6 states we actually
> > want to use. But at least for the time being I think this
> > simple should be sufficient, and should be easy to backport
> > to stable releases.
> >
> > > .has_rps = true, \
> > > .dma_mask_size = 40, \
> > > .__runtime.ppgtt_type = INTEL_PPGTT_ALIASING, \
> > > --
> > > 2.37.2
> >
> > --
> > Ville Syrjälä
> > Intel
--
Ville Syrjälä
Intel
Re: [Intel-gfx] [PATCH 1/1] drm/i915: re-disable RC6p on Sandy Bridge
On Tue, Dec 20, 2022 at 12:32:05PM +0200, Ville Syrjälä wrote:
> On Mon, Dec 19, 2022 at 06:29:27PM +0100, Sasa Dragic wrote:
> > RC6p on Sandy Bridge got re-enabled over time, causing visual glitches
> > and GPU hangs.
> >
> > Disabled originally in commit 1c8ecf80fdee ("drm/i915: do not enable
> > RC6p on Sandy Bridge").
>
> re cover letter:
> > It was originally disabled in commit 1c8ecf80fdee ("drm/i915: do not
> > enable RC6p on Sandy Bridge"), and subsequently re-enabled by
> > 13c5a577b342 ("drm/i915/gt: Select the deepest available parking mode
> > for rc6"), which seems to focus only on Ivy Bridge.
>
> That only kicks in while parked (ie. fully idle from
> software POV). I think the real bad commit is
> fb6db0f5bf1d ("drm/i915: Remove unsafe i915.enable_rc6")
> which seems to affects which rc6 level is selected while
> unparked.
>
> We should mention both of those commits here:
> Fixes: fb6db0f5bf1d ("drm/i915: Remove unsafe i915.enable_rc6")
> Fixes: 13c5a577b342 ("drm/i915/gt: Select the deepest available parking mode
> for rc6")
>
> Also we want
> Cc: sta...@vger.kernel.org
>
> We can add those while pushing, so no need to resend for that.
agreed.
Reviewed-by: Rodrigo Vivi
>
> >
> > Signed-off-by: Sasa Dragic
> > ---
> > drivers/gpu/drm/i915/i915_pci.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_pci.c
> > b/drivers/gpu/drm/i915/i915_pci.c
> > index 668e9da52584..69377564028a 100644
> > --- a/drivers/gpu/drm/i915/i915_pci.c
> > +++ b/drivers/gpu/drm/i915/i915_pci.c
> > @@ -423,7 +423,8 @@ static const struct intel_device_info ilk_m_info = {
> > .has_coherent_ggtt = true, \
> > .has_llc = 1, \
> > .has_rc6 = 1, \
> > - .has_rc6p = 1, \
> > + /* snb does support rc6p, but enabling it causes various issues */ \
> > + .has_rc6p = 0, \
>
> The one downside of doing it this way is that we also lose
> the debugfs/sysfs files so we can't monitor rc6+/++
> residency anymore to make sure they are not entered.
>
> I think ideally we'd split this into two parts: which rc6
> states the hw actually has vs. which rc6 states we actually
> want to use. But at least for the time being I think this
> simple should be sufficient, and should be easy to backport
> to stable releases.
>
> > .has_rps = true, \
> > .dma_mask_size = 40, \
> > .__runtime.ppgtt_type = INTEL_PPGTT_ALIASING, \
> > --
> > 2.37.2
>
> --
> Ville Syrjälä
> Intel
Re: [Intel-gfx] [PATCH 1/1] drm/i915: re-disable RC6p on Sandy Bridge
On Tue, Dec 20, 2022 at 11:32 AM Ville Syrjälä
wrote:
>
> On Mon, Dec 19, 2022 at 06:29:27PM +0100, Sasa Dragic wrote:
> > RC6p on Sandy Bridge got re-enabled over time, causing visual glitches
> > and GPU hangs.
> >
> > Disabled originally in commit 1c8ecf80fdee ("drm/i915: do not enable
> > RC6p on Sandy Bridge").
>
> re cover letter:
> > It was originally disabled in commit 1c8ecf80fdee ("drm/i915: do not
> > enable RC6p on Sandy Bridge"), and subsequently re-enabled by
> > 13c5a577b342 ("drm/i915/gt: Select the deepest available parking mode
> > for rc6"), which seems to focus only on Ivy Bridge.
>
> That only kicks in while parked (ie. fully idle from
> software POV). I think the real bad commit is
> fb6db0f5bf1d ("drm/i915: Remove unsafe i915.enable_rc6")
> which seems to affects which rc6 level is selected while
> unparked.
You are correct. Although I'd like to add that most of the glitching
happens when system is switching to / from fully idle (e.g. running
glxgears in background reduces symptoms tenfold).
> We should mention both of those commits here:
> Fixes: fb6db0f5bf1d ("drm/i915: Remove unsafe i915.enable_rc6")
> Fixes: 13c5a577b342 ("drm/i915/gt: Select the deepest available parking mode
> for rc6")
>
> Also we want
> Cc: sta...@vger.kernel.org
>
> We can add those while pushing, so no need to resend for that.
Ok, thanks.
> >
> > Signed-off-by: Sasa Dragic
> > ---
> > drivers/gpu/drm/i915/i915_pci.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_pci.c
> > b/drivers/gpu/drm/i915/i915_pci.c
> > index 668e9da52584..69377564028a 100644
> > --- a/drivers/gpu/drm/i915/i915_pci.c
> > +++ b/drivers/gpu/drm/i915/i915_pci.c
> > @@ -423,7 +423,8 @@ static const struct intel_device_info ilk_m_info = {
> > .has_coherent_ggtt = true, \
> > .has_llc = 1, \
> > .has_rc6 = 1, \
> > - .has_rc6p = 1, \
> > + /* snb does support rc6p, but enabling it causes various issues */ \
> > + .has_rc6p = 0, \
>
> The one downside of doing it this way is that we also lose
> the debugfs/sysfs files so we can't monitor rc6+/++
> residency anymore to make sure they are not entered.
>
> I think ideally we'd split this into two parts: which rc6
> states the hw actually has vs. which rc6 states we actually
> want to use. But at least for the time being I think this
> simple should be sufficient, and should be easy to backport
> to stable releases.
Agreed.
> > .has_rps = true, \
> > .dma_mask_size = 40, \
> > .__runtime.ppgtt_type = INTEL_PPGTT_ALIASING, \
> > --
> > 2.37.2
>
> --
> Ville Syrjälä
> Intel
Regards,
Sasa
Re: [Intel-gfx] [PATCH 1/1] drm/i915: re-disable RC6p on Sandy Bridge
On Mon, Dec 19, 2022 at 06:29:27PM +0100, Sasa Dragic wrote:
> RC6p on Sandy Bridge got re-enabled over time, causing visual glitches
> and GPU hangs.
>
> Disabled originally in commit 1c8ecf80fdee ("drm/i915: do not enable
> RC6p on Sandy Bridge").
re cover letter:
> It was originally disabled in commit 1c8ecf80fdee ("drm/i915: do not
> enable RC6p on Sandy Bridge"), and subsequently re-enabled by
> 13c5a577b342 ("drm/i915/gt: Select the deepest available parking mode
> for rc6"), which seems to focus only on Ivy Bridge.
That only kicks in while parked (ie. fully idle from
software POV). I think the real bad commit is
fb6db0f5bf1d ("drm/i915: Remove unsafe i915.enable_rc6")
which seems to affects which rc6 level is selected while
unparked.
We should mention both of those commits here:
Fixes: fb6db0f5bf1d ("drm/i915: Remove unsafe i915.enable_rc6")
Fixes: 13c5a577b342 ("drm/i915/gt: Select the deepest available parking mode
for rc6")
Also we want
Cc: sta...@vger.kernel.org
We can add those while pushing, so no need to resend for that.
>
> Signed-off-by: Sasa Dragic
> ---
> drivers/gpu/drm/i915/i915_pci.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c
> index 668e9da52584..69377564028a 100644
> --- a/drivers/gpu/drm/i915/i915_pci.c
> +++ b/drivers/gpu/drm/i915/i915_pci.c
> @@ -423,7 +423,8 @@ static const struct intel_device_info ilk_m_info = {
> .has_coherent_ggtt = true, \
> .has_llc = 1, \
> .has_rc6 = 1, \
> - .has_rc6p = 1, \
> + /* snb does support rc6p, but enabling it causes various issues */ \
> + .has_rc6p = 0, \
The one downside of doing it this way is that we also lose
the debugfs/sysfs files so we can't monitor rc6+/++
residency anymore to make sure they are not entered.
I think ideally we'd split this into two parts: which rc6
states the hw actually has vs. which rc6 states we actually
want to use. But at least for the time being I think this
simple should be sufficient, and should be easy to backport
to stable releases.
> .has_rps = true, \
> .dma_mask_size = 40, \
> .__runtime.ppgtt_type = INTEL_PPGTT_ALIASING, \
> --
> 2.37.2
--
Ville Syrjälä
Intel
[Intel-gfx] [PATCH 1/1] drm/i915: re-disable RC6p on Sandy Bridge
RC6p on Sandy Bridge got re-enabled over time, causing visual glitches
and GPU hangs.
Disabled originally in commit 1c8ecf80fdee ("drm/i915: do not enable
RC6p on Sandy Bridge").
Signed-off-by: Sasa Dragic
---
drivers/gpu/drm/i915/i915_pci.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c
index 668e9da52584..69377564028a 100644
--- a/drivers/gpu/drm/i915/i915_pci.c
+++ b/drivers/gpu/drm/i915/i915_pci.c
@@ -423,7 +423,8 @@ static const struct intel_device_info ilk_m_info = {
.has_coherent_ggtt = true, \
.has_llc = 1, \
.has_rc6 = 1, \
- .has_rc6p = 1, \
+ /* snb does support rc6p, but enabling it causes various issues */ \
+ .has_rc6p = 0, \
.has_rps = true, \
.dma_mask_size = 40, \
.__runtime.ppgtt_type = INTEL_PPGTT_ALIASING, \
--
2.37.2
