Re: [Intel-gfx] [PATCH] drm/i915: Don't sanitize frame start delay if the pipe is off

[Ville Syrjälä]

On Wed, Nov 15, 2017 at 08:17:21PM +, Chris Wilson wrote:
> Quoting Ville Syrjala (2017-11-15 20:04:42)
> > From: Ville Syrjälä 
> > 
> > Avoid touching PIPECONF in intel_sanitize_crtc() unless the pipe is
> > actually on. Should cure some unclaimed register accesses.
> 
> While using crtc->active is consistent with other sanitization, is this
> really a fix for unclaimed register access?
> 
> We should be holding all the powerwells at this moment in bringing up
> the hw, right? So the only unclaimed access would be if the reg didn't
> exist. So are we looking at an invalid cpu_transcoder?

I was thinking we'd have dropped the power references already. But
I guess not. And that should definitely then give unclaimed register
accesses during driver init.

I think these fails are in the "pretend display gets clobbered by GPU
reset" path. And there we don't actually seem to grabbing the init power
reference, which could well explain this. 

Not sure we want to add the init power there either. Most of the readout
code now has the power domain handling in place, so maybe we're close
to being able to nuke the init power thing entirely?

-- 
Ville Syrjälä
Intel OTC
___
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 sanitize frame start delay if the pipe is off

[Ville Syrjälä]

On Wed, Nov 15, 2017 at 09:13:28PM +, Chris Wilson wrote:
> Quoting Ville Syrjala (2017-11-15 20:04:42)
> > From: Ville Syrjälä 
> > 
> > Avoid touching PIPECONF in intel_sanitize_crtc() unless the pipe is
> > actually on. Should cure some unclaimed register accesses.
> + during reset, as we are rather cavalier in our approach to powerdomain
> management.

Amended, and pushed. Thanks.

> 
> > We don't have to sanitize this if the pipe is off since we will
> > overwrite the frame start delay anyway when turning the pipe on.
> > 
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=102249
> > Signed-off-by: Ville Syrjälä 
> Reviewed-by: Chris Wilson 
> 
> Let's not even get started on the handling of modesets vs display reset.
> -Chris

-- 
Ville Syrjälä
Intel OTC
___
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 sanitize frame start delay if the pipe is off

[Chris Wilson]

Quoting Ville Syrjala (2017-11-15 20:04:42)
> From: Ville Syrjälä 
> 
> Avoid touching PIPECONF in intel_sanitize_crtc() unless the pipe is
> actually on. Should cure some unclaimed register accesses.
+ during reset, as we are rather cavalier in our approach to powerdomain
management.

> We don't have to sanitize this if the pipe is off since we will
> overwrite the frame start delay anyway when turning the pipe on.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=102249
> Signed-off-by: Ville Syrjälä 
Reviewed-by: Chris Wilson 

Let's not even get started on the handling of modesets vs display reset.
-Chris
___
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 sanitize frame start delay if the pipe is off

[Chris Wilson]

Quoting Ville Syrjälä (2017-11-15 20:35:41)
> On Wed, Nov 15, 2017 at 08:17:21PM +, Chris Wilson wrote:
> > Quoting Ville Syrjala (2017-11-15 20:04:42)
> > > From: Ville Syrjälä 
> > > 
> > > Avoid touching PIPECONF in intel_sanitize_crtc() unless the pipe is
> > > actually on. Should cure some unclaimed register accesses.
> > 
> > While using crtc->active is consistent with other sanitization, is this
> > really a fix for unclaimed register access?
> > 
> > We should be holding all the powerwells at this moment in bringing up
> > the hw, right? So the only unclaimed access would be if the reg didn't
> > exist. So are we looking at an invalid cpu_transcoder?
> 
> I was thinking we'd have dropped the power references already. But
> I guess not. And that should definitely then give unclaimed register
> accesses during driver init.
> 
> I think these fails are in the "pretend display gets clobbered by GPU
> reset" path. And there we don't actually seem to grabbing the init power
> reference, which could well explain this. 

Hmm, it is in reset's __intel_dsiplay_resume() so that's a likely
candidate.

> Not sure we want to add the init power there either. Most of the readout
> code now has the power domain handling in place, so maybe we're close
> to being able to nuke the init power thing entirely?

But for reset, shouldn't we be taking the display powerwells in
intel_prepare_reset() or is that already there buried away in the
indirection?

Oh, that intel_display_set_init_power() definitely seems out of place
along reset.
-Chris
___
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 sanitize frame start delay if the pipe is off

[Chris Wilson]

Quoting Ville Syrjala (2017-11-15 20:04:42)
> From: Ville Syrjälä 
> 
> Avoid touching PIPECONF in intel_sanitize_crtc() unless the pipe is
> actually on. Should cure some unclaimed register accesses.

While using crtc->active is consistent with other sanitization, is this
really a fix for unclaimed register access?

We should be holding all the powerwells at this moment in bringing up
the hw, right? So the only unclaimed access would be if the reg didn't
exist. So are we looking at an invalid cpu_transcoder?
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH] drm/i915: Don't sanitize frame start delay if the pipe is off

[Ville Syrjala]

From: Ville Syrjälä 

Avoid touching PIPECONF in intel_sanitize_crtc() unless the pipe is
actually on. Should cure some unclaimed register accesses.

We don't have to sanitize this if the pipe is off since we will
overwrite the frame start delay anyway when turning the pipe on.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=102249
Signed-off-by: Ville Syrjälä 
---
 drivers/gpu/drm/i915/intel_display.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c 
b/drivers/gpu/drm/i915/intel_display.c
index 23aa7191024e..e6fcbc5abc75 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -14812,7 +14812,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc,
enum transcoder cpu_transcoder = crtc->config->cpu_transcoder;
 
/* Clear any frame start delays used for debugging left by the BIOS */
-   if (!transcoder_is_dsi(cpu_transcoder)) {
+   if (crtc->active && !transcoder_is_dsi(cpu_transcoder)) {
i915_reg_t reg = PIPECONF(cpu_transcoder);
 
I915_WRITE(reg,
-- 
2.13.6

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx