Re: [Intel-gfx] [PATCH v5 1/4] drm/i915: Fix modeset handling during gpu reset, v5.

[Ville Syrjälä]

On Mon, Aug 08, 2016 at 11:08:33AM +0200, Maarten Lankhorst wrote:
> Op 08-08-16 om 10:57 schreef Ville Syrjälä:
> > On Mon, Aug 08, 2016 at 10:40:42AM +0200, Maarten Lankhorst wrote:
> >> Op 08-08-16 om 10:05 schreef Ville Syrjälä:
> >>> On Mon, Aug 08, 2016 at 09:52:49AM +0200, Maarten Lankhorst wrote:
>  Hey,
> 
>  Op 05-08-16 om 22:28 schreef ville.syrj...@linux.intel.com:
> > From: Maarten Lankhorst 
> >
> > This function would call drm_modeset_lock_all, while the suspend/resume
> > functions already have their own locking. Fix this by factoring out
> > __intel_display_resume, and calling the atomic helpers for duplicating
> > atomic state and disabling all crtc's during suspend.
> >
> > Changes since v1:
> > - Deal with -EDEADLK right after lock_all and clean up calls
> >   to hw readout.
> > - Always take all modeset locks so updates during gpu reset are blocked.
> > Changes since v2:
> > - Fix deadlock in intel_update_primary_planes.
> > - Move WARN_ON(EDEADLK) to __intel_display_resume.
> > - pctx -> ctx
> > - only call __intel_display_resume on success in intel_display_resume.
> > Changes since v3:
> > - Rebase on top of dev_priv -> dev change.
> > - Use drm_modeset_lock_all_ctx instead of drm_modeset_lock_all.
> > Changes since v4 [by vsyrjala]:
> > - Deal with skip_intermediate_wm
> > - Update comment w.r.t. mode_config.mutex vs. ->detect()
> > - Rebase due to INTEL_GEN() etc.
>  Setting skip_intermediate_wm seems to have already been upstreamed and I 
>  missed it, but
>  this may blow up in .crtc_enable, which programs in the intermediate 
>  wm's which is used
>  until all planes are enabled.
> >>> What blows up and how?
> >>>
> >>> Even if it can blow up we don't have any two stage wm stuff for pre-g4x at
> >>> this time anyway, so -ENOCARE at this point really.
> >>>
>  I fear this may blow up in interesting ways. And it should probably be 
>  using
>  dev_priv->wm.distrust_bios_wm instead like on SKL.
> >>> Sigh. How many ways do we need to do the same thing?
> >>>
> >>> Anywyas, what we should really do is sanitize the current wms better
> >>> at readout time, and then we shouldn't need these flags at all.
> >>>
> >> Yeah, slightly different approach of accomplishing the same. :-/
> >>
> >> distrust_bios_wm pulls in the whole state and recalculates it, while 
> >> sanitize_watermarks runs at the end of initial config.
> >> Maybe get_hw_state for ILK should set the flag too, and  then stuff final 
> >> wm in intermediate. And then kill off the skip_intermediate_wm flag.
> > Or just kill both flags and sanitize better.
> >
> The first flag is used by the watermark sanitization, second flag seems to be 
> useful to make the driver init faster, first commit (either by fbcon or 
> userspace) will incur the real penalty. This is similar to fastset, which is 
> also recalculated on the first modeset.

The skl flag looks to be about the DDB. Irrelevant on other platforms,
except gen2-4 if/when someone decides to implement DDB reallocation
for them.

-- 
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 v5 1/4] drm/i915: Fix modeset handling during gpu reset, v5.

[Maarten Lankhorst]

Op 08-08-16 om 10:57 schreef Ville Syrjälä:
> On Mon, Aug 08, 2016 at 10:40:42AM +0200, Maarten Lankhorst wrote:
>> Op 08-08-16 om 10:05 schreef Ville Syrjälä:
>>> On Mon, Aug 08, 2016 at 09:52:49AM +0200, Maarten Lankhorst wrote:
 Hey,

 Op 05-08-16 om 22:28 schreef ville.syrj...@linux.intel.com:
> From: Maarten Lankhorst 
>
> This function would call drm_modeset_lock_all, while the suspend/resume
> functions already have their own locking. Fix this by factoring out
> __intel_display_resume, and calling the atomic helpers for duplicating
> atomic state and disabling all crtc's during suspend.
>
> Changes since v1:
> - Deal with -EDEADLK right after lock_all and clean up calls
>   to hw readout.
> - Always take all modeset locks so updates during gpu reset are blocked.
> Changes since v2:
> - Fix deadlock in intel_update_primary_planes.
> - Move WARN_ON(EDEADLK) to __intel_display_resume.
> - pctx -> ctx
> - only call __intel_display_resume on success in intel_display_resume.
> Changes since v3:
> - Rebase on top of dev_priv -> dev change.
> - Use drm_modeset_lock_all_ctx instead of drm_modeset_lock_all.
> Changes since v4 [by vsyrjala]:
> - Deal with skip_intermediate_wm
> - Update comment w.r.t. mode_config.mutex vs. ->detect()
> - Rebase due to INTEL_GEN() etc.
 Setting skip_intermediate_wm seems to have already been upstreamed and I 
 missed it, but
 this may blow up in .crtc_enable, which programs in the intermediate wm's 
 which is used
 until all planes are enabled.
>>> What blows up and how?
>>>
>>> Even if it can blow up we don't have any two stage wm stuff for pre-g4x at
>>> this time anyway, so -ENOCARE at this point really.
>>>
 I fear this may blow up in interesting ways. And it should probably be 
 using
 dev_priv->wm.distrust_bios_wm instead like on SKL.
>>> Sigh. How many ways do we need to do the same thing?
>>>
>>> Anywyas, what we should really do is sanitize the current wms better
>>> at readout time, and then we shouldn't need these flags at all.
>>>
>> Yeah, slightly different approach of accomplishing the same. :-/
>>
>> distrust_bios_wm pulls in the whole state and recalculates it, while 
>> sanitize_watermarks runs at the end of initial config.
>> Maybe get_hw_state for ILK should set the flag too, and  then stuff final wm 
>> in intermediate. And then kill off the skip_intermediate_wm flag.
> Or just kill both flags and sanitize better.
>
The first flag is used by the watermark sanitization, second flag seems to be 
useful to make the driver init faster, first commit (either by fbcon or 
userspace) will incur the real penalty. This is similar to fastset, which is 
also recalculated on the first modeset.

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

Re: [Intel-gfx] [PATCH v5 1/4] drm/i915: Fix modeset handling during gpu reset, v5.

[Ville Syrjälä]

On Mon, Aug 08, 2016 at 10:40:42AM +0200, Maarten Lankhorst wrote:
> Op 08-08-16 om 10:05 schreef Ville Syrjälä:
> > On Mon, Aug 08, 2016 at 09:52:49AM +0200, Maarten Lankhorst wrote:
> >> Hey,
> >>
> >> Op 05-08-16 om 22:28 schreef ville.syrj...@linux.intel.com:
> >>> From: Maarten Lankhorst 
> >>>
> >>> This function would call drm_modeset_lock_all, while the suspend/resume
> >>> functions already have their own locking. Fix this by factoring out
> >>> __intel_display_resume, and calling the atomic helpers for duplicating
> >>> atomic state and disabling all crtc's during suspend.
> >>>
> >>> Changes since v1:
> >>> - Deal with -EDEADLK right after lock_all and clean up calls
> >>>   to hw readout.
> >>> - Always take all modeset locks so updates during gpu reset are blocked.
> >>> Changes since v2:
> >>> - Fix deadlock in intel_update_primary_planes.
> >>> - Move WARN_ON(EDEADLK) to __intel_display_resume.
> >>> - pctx -> ctx
> >>> - only call __intel_display_resume on success in intel_display_resume.
> >>> Changes since v3:
> >>> - Rebase on top of dev_priv -> dev change.
> >>> - Use drm_modeset_lock_all_ctx instead of drm_modeset_lock_all.
> >>> Changes since v4 [by vsyrjala]:
> >>> - Deal with skip_intermediate_wm
> >>> - Update comment w.r.t. mode_config.mutex vs. ->detect()
> >>> - Rebase due to INTEL_GEN() etc.
> >> Setting skip_intermediate_wm seems to have already been upstreamed and I 
> >> missed it, but
> >> this may blow up in .crtc_enable, which programs in the intermediate wm's 
> >> which is used
> >> until all planes are enabled.
> > What blows up and how?
> >
> > Even if it can blow up we don't have any two stage wm stuff for pre-g4x at
> > this time anyway, so -ENOCARE at this point really.
> >
> >> I fear this may blow up in interesting ways. And it should probably be 
> >> using
> >> dev_priv->wm.distrust_bios_wm instead like on SKL.
> > Sigh. How many ways do we need to do the same thing?
> >
> > Anywyas, what we should really do is sanitize the current wms better
> > at readout time, and then we shouldn't need these flags at all.
> >
> Yeah, slightly different approach of accomplishing the same. :-/
> 
> distrust_bios_wm pulls in the whole state and recalculates it, while 
> sanitize_watermarks runs at the end of initial config.
> Maybe get_hw_state for ILK should set the flag too, and  then stuff final wm 
> in intermediate. And then kill off the skip_intermediate_wm flag.

Or just kill both flags and sanitize better.

-- 
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 v5 1/4] drm/i915: Fix modeset handling during gpu reset, v5.

[Maarten Lankhorst]

Op 08-08-16 om 10:05 schreef Ville Syrjälä:
> On Mon, Aug 08, 2016 at 09:52:49AM +0200, Maarten Lankhorst wrote:
>> Hey,
>>
>> Op 05-08-16 om 22:28 schreef ville.syrj...@linux.intel.com:
>>> From: Maarten Lankhorst 
>>>
>>> This function would call drm_modeset_lock_all, while the suspend/resume
>>> functions already have their own locking. Fix this by factoring out
>>> __intel_display_resume, and calling the atomic helpers for duplicating
>>> atomic state and disabling all crtc's during suspend.
>>>
>>> Changes since v1:
>>> - Deal with -EDEADLK right after lock_all and clean up calls
>>>   to hw readout.
>>> - Always take all modeset locks so updates during gpu reset are blocked.
>>> Changes since v2:
>>> - Fix deadlock in intel_update_primary_planes.
>>> - Move WARN_ON(EDEADLK) to __intel_display_resume.
>>> - pctx -> ctx
>>> - only call __intel_display_resume on success in intel_display_resume.
>>> Changes since v3:
>>> - Rebase on top of dev_priv -> dev change.
>>> - Use drm_modeset_lock_all_ctx instead of drm_modeset_lock_all.
>>> Changes since v4 [by vsyrjala]:
>>> - Deal with skip_intermediate_wm
>>> - Update comment w.r.t. mode_config.mutex vs. ->detect()
>>> - Rebase due to INTEL_GEN() etc.
>> Setting skip_intermediate_wm seems to have already been upstreamed and I 
>> missed it, but
>> this may blow up in .crtc_enable, which programs in the intermediate wm's 
>> which is used
>> until all planes are enabled.
> What blows up and how?
>
> Even if it can blow up we don't have any two stage wm stuff for pre-g4x at
> this time anyway, so -ENOCARE at this point really.
>
>> I fear this may blow up in interesting ways. And it should probably be using
>> dev_priv->wm.distrust_bios_wm instead like on SKL.
> Sigh. How many ways do we need to do the same thing?
>
> Anywyas, what we should really do is sanitize the current wms better
> at readout time, and then we shouldn't need these flags at all.
>
Yeah, slightly different approach of accomplishing the same. :-/

distrust_bios_wm pulls in the whole state and recalculates it, while 
sanitize_watermarks runs at the end of initial config.
Maybe get_hw_state for ILK should set the flag too, and  then stuff final wm in 
intermediate. And then kill off the skip_intermediate_wm flag.

~Maarten

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

Re: [Intel-gfx] [PATCH v5 1/4] drm/i915: Fix modeset handling during gpu reset, v5.

[Ville Syrjälä]

On Mon, Aug 08, 2016 at 09:52:49AM +0200, Maarten Lankhorst wrote:
> Hey,
> 
> Op 05-08-16 om 22:28 schreef ville.syrj...@linux.intel.com:
> > From: Maarten Lankhorst 
> >
> > This function would call drm_modeset_lock_all, while the suspend/resume
> > functions already have their own locking. Fix this by factoring out
> > __intel_display_resume, and calling the atomic helpers for duplicating
> > atomic state and disabling all crtc's during suspend.
> >
> > Changes since v1:
> > - Deal with -EDEADLK right after lock_all and clean up calls
> >   to hw readout.
> > - Always take all modeset locks so updates during gpu reset are blocked.
> > Changes since v2:
> > - Fix deadlock in intel_update_primary_planes.
> > - Move WARN_ON(EDEADLK) to __intel_display_resume.
> > - pctx -> ctx
> > - only call __intel_display_resume on success in intel_display_resume.
> > Changes since v3:
> > - Rebase on top of dev_priv -> dev change.
> > - Use drm_modeset_lock_all_ctx instead of drm_modeset_lock_all.
> > Changes since v4 [by vsyrjala]:
> > - Deal with skip_intermediate_wm
> > - Update comment w.r.t. mode_config.mutex vs. ->detect()
> > - Rebase due to INTEL_GEN() etc.
> 
> Setting skip_intermediate_wm seems to have already been upstreamed and I 
> missed it, but
> this may blow up in .crtc_enable, which programs in the intermediate wm's 
> which is used
> until all planes are enabled.

What blows up and how?

Even if it can blow up we don't have any two stage wm stuff for pre-g4x at
this time anyway, so -ENOCARE at this point really.

> 
> I fear this may blow up in interesting ways. And it should probably be using
> dev_priv->wm.distrust_bios_wm instead like on SKL.

Sigh. How many ways do we need to do the same thing?

Anywyas, what we should really do is sanitize the current wms better
at readout time, and then we shouldn't need these flags at all.

-- 
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 v5 1/4] drm/i915: Fix modeset handling during gpu reset, v5.

[Maarten Lankhorst]

Hey,

Op 05-08-16 om 22:28 schreef ville.syrj...@linux.intel.com:
> From: Maarten Lankhorst 
>
> This function would call drm_modeset_lock_all, while the suspend/resume
> functions already have their own locking. Fix this by factoring out
> __intel_display_resume, and calling the atomic helpers for duplicating
> atomic state and disabling all crtc's during suspend.
>
> Changes since v1:
> - Deal with -EDEADLK right after lock_all and clean up calls
>   to hw readout.
> - Always take all modeset locks so updates during gpu reset are blocked.
> Changes since v2:
> - Fix deadlock in intel_update_primary_planes.
> - Move WARN_ON(EDEADLK) to __intel_display_resume.
> - pctx -> ctx
> - only call __intel_display_resume on success in intel_display_resume.
> Changes since v3:
> - Rebase on top of dev_priv -> dev change.
> - Use drm_modeset_lock_all_ctx instead of drm_modeset_lock_all.
> Changes since v4 [by vsyrjala]:
> - Deal with skip_intermediate_wm
> - Update comment w.r.t. mode_config.mutex vs. ->detect()
> - Rebase due to INTEL_GEN() etc.

Setting skip_intermediate_wm seems to have already been upstreamed and I missed 
it, but
this may blow up in .crtc_enable, which programs in the intermediate wm's which 
is used
until all planes are enabled.

I fear this may blow up in interesting ways. And it should probably be using
dev_priv->wm.distrust_bios_wm instead like on SKL.

~Maarten

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

[Intel-gfx] [PATCH v5 1/4] drm/i915: Fix modeset handling during gpu reset, v5.

[ville . syrjala]

From: Maarten Lankhorst 

This function would call drm_modeset_lock_all, while the suspend/resume
functions already have their own locking. Fix this by factoring out
__intel_display_resume, and calling the atomic helpers for duplicating
atomic state and disabling all crtc's during suspend.

Changes since v1:
- Deal with -EDEADLK right after lock_all and clean up calls
  to hw readout.
- Always take all modeset locks so updates during gpu reset are blocked.
Changes since v2:
- Fix deadlock in intel_update_primary_planes.
- Move WARN_ON(EDEADLK) to __intel_display_resume.
- pctx -> ctx
- only call __intel_display_resume on success in intel_display_resume.
Changes since v3:
- Rebase on top of dev_priv -> dev change.
- Use drm_modeset_lock_all_ctx instead of drm_modeset_lock_all.
Changes since v4 [by vsyrjala]:
- Deal with skip_intermediate_wm
- Update comment w.r.t. mode_config.mutex vs. ->detect()
- Rebase due to INTEL_GEN() etc.

Signed-off-by: Maarten Lankhorst 
Fixes: e2c8b8701e2d ("drm/i915: Use atomic helpers for suspend, v2.")
Cc: sta...@vger.kernel.org
Tested-by: Ville Syrjälä 
Signed-off-by: Ville Syrjälä 
---
 drivers/gpu/drm/i915/i915_drv.h  |   1 +
 drivers/gpu/drm/i915/intel_display.c | 170 ++-
 2 files changed, 111 insertions(+), 60 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index feec00f769e1..94a333c44bee 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1840,6 +1840,7 @@ struct drm_i915_private {
enum modeset_restore modeset_restore;
struct mutex modeset_restore_lock;
struct drm_atomic_state *modeset_restore_state;
+   struct drm_modeset_acquire_ctx reset_ctx;
 
struct list_head vm_list; /* Global list of all address spaces */
struct i915_ggtt ggtt; /* VM representing the global address space */
diff --git a/drivers/gpu/drm/i915/intel_display.c 
b/drivers/gpu/drm/i915/intel_display.c
index 9cbf5431c1e3..1e2fb444eb93 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3093,40 +3093,110 @@ static void intel_update_primary_planes(struct 
drm_device *dev)
 
for_each_crtc(dev, crtc) {
struct intel_plane *plane = to_intel_plane(crtc->primary);
-   struct intel_plane_state *plane_state;
-
-   drm_modeset_lock_crtc(crtc, >base);
-   plane_state = to_intel_plane_state(plane->base.state);
+   struct intel_plane_state *plane_state =
+   to_intel_plane_state(plane->base.state);
 
if (plane_state->visible)
plane->update_plane(>base,
to_intel_crtc_state(crtc->state),
plane_state);
+   }
+}
+
+static int
+__intel_display_resume(struct drm_device *dev,
+  struct drm_atomic_state *state)
+{
+   struct drm_crtc_state *crtc_state;
+   struct drm_crtc *crtc;
+   int i, ret;
 
-   drm_modeset_unlock_crtc(crtc);
+   intel_modeset_setup_hw_state(dev);
+   i915_redisable_vga(dev);
+
+   if (!state)
+   return 0;
+
+   for_each_crtc_in_state(state, crtc, crtc_state, i) {
+   /*
+* Force recalculation even if we restore
+* current state. With fast modeset this may not result
+* in a modeset when the state is compatible.
+*/
+   crtc_state->mode_changed = true;
}
+
+   /* ignore any reset values/BIOS leftovers in the WM registers */
+   to_intel_atomic_state(state)->skip_intermediate_wm = true;
+
+   ret = drm_atomic_commit(state);
+
+   WARN_ON(ret == -EDEADLK);
+   return ret;
 }
 
 void intel_prepare_reset(struct drm_i915_private *dev_priv)
 {
+   struct drm_device *dev = _priv->drm;
+   struct drm_modeset_acquire_ctx *ctx = _priv->reset_ctx;
+   struct drm_atomic_state *state;
+   int ret;
+
/* no reset support for gen2 */
if (IS_GEN2(dev_priv))
return;
 
-   /* reset doesn't touch the display */
+   /*
+* Need mode_config.mutex so that we don't
+* trample ongoing ->detect() and whatnot.
+*/
+   mutex_lock(>mode_config.mutex);
+   drm_modeset_acquire_init(ctx, 0);
+   while (1) {
+   ret = drm_modeset_lock_all_ctx(dev, ctx);
+   if (ret != -EDEADLK)
+   break;
+
+   drm_modeset_backoff(ctx);
+   }
+
+   /* reset doesn't touch the display, but flips might get nuked anyway, */
if (INTEL_GEN(dev_priv) >= 5 || IS_G4X(dev_priv))
return;
 
-   drm_modeset_lock_all(_priv->drm);
/*
 *