New async patch for resume

New async patch for resume

[Jesse Barnes]

In digging into the async crtc stuff, I found it was going to be really
difficult to prevent other functions from getting clobbered by a delayed
crtc enable or disable.  Daniel's work to remove a bunch of the
->mode_set callbacks is a good start, but we still end up doing some reg
reads and writes in the mode set path today.  Until those are cleared up
and we somehow enforce a rule that all hw changes go through the crtc
enable/disable paths with everything else staged in multithread safe structs,
I don't think the async crtc approach will be solid.

So, since resume is the biggest issue here anyway, I've tried making
just the resume mode set asynchronous.  Even this is a bit tricky, since
we need to apply any pending mode set at certain points, then check
whether the crtc we're operating on in any given path is still active.
I think I've caught those cases here, but if we have more we can use the
intel_sync_mode_set() call with appropriate post-call checks (after we
re-acquire the corresponding crtc mutex).

Feedback welcome.  This has seen light testing on my BYT and really
reduces the time spend in the i915 _thaw function, letting userspace
start running much sooner than before.

Thanks,
Jesse

[PATCH] resume timings

[Jesse Barnes]

---
 drivers/gpu/drm/i915/i915_drv.c      | 6 ++++++
 drivers/gpu/drm/i915/intel_display.c | 9 +++++++++
 2 files changed, 15 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 0f9e836..a2036b2 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -625,6 +625,8 @@ static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
+	DRM_ERROR("start thaw\n");
+
 	if (drm_core_check_feature(dev, DRIVER_MODESET) &&
 	    restore_gtt_mappings) {
 		mutex_lock(&dev->struct_mutex);
@@ -652,7 +654,9 @@ static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings)
 
 		intel_modeset_init_hw(dev);
 
+		DRM_ERROR("hw state start\n");
 		schedule_work(&dev_priv->hw_restore_work);
+		DRM_ERROR("hw state end\n");
 
 		/*
 		 * ... but also need to make sure that hotplug processing
@@ -685,6 +689,8 @@ static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings)
 	mutex_unlock(&dev_priv->modeset_restore_lock);
 
 	intel_runtime_pm_put(dev_priv);
+
+	DRM_ERROR("end thaw\n");
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 6d6b0f2..472b7f0 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12175,7 +12175,9 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
 	struct intel_encoder *encoder;
 	int i;
 
+	DRM_ERROR("\n");
 	intel_modeset_readout_hw_state(dev);
+	DRM_ERROR("readout complete\n");
 
 	/*
 	 * Now that we have the config, copy it to each CRTC struct
@@ -12197,12 +12199,16 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
 		intel_sanitize_encoder(encoder);
 	}
 
+	DRM_ERROR("done with encoder sanitize\n");
+
 	for_each_pipe(pipe) {
 		crtc = to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
 		intel_sanitize_crtc(crtc);
 		intel_dump_pipe_config(crtc, &crtc->config, "[setup_hw_state]");
 	}
 
+	DRM_ERROR("done with crtc sanitize\n");
+
 	for (i = 0; i < dev_priv->num_shared_dpll; i++) {
 		struct intel_shared_dpll *pll = &dev_priv->shared_dplls[i];
 
@@ -12215,6 +12221,8 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
 		pll->on = false;
 	}
 
+	DRM_ERROR("done with pll enable\n");
+
 	if (HAS_PCH_SPLIT(dev))
 		ilk_wm_get_hw_state(dev);
 
@@ -12231,6 +12239,7 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
 
 			__intel_set_mode(crtc, &crtc->mode, crtc->x, crtc->y,
 					 crtc->primary->fb);
+			DRM_ERROR("done with modeset on %d\n", pipe);
 		}
 	} else {
 		intel_modeset_update_staged_output_state(dev);
-- 
1.9.1

Re: New async patch for resume

[Daniel Vetter]

On Thu, Jun 05, 2014 at 08:48:37AM -0700, Jesse Barnes wrote:
> In digging into the async crtc stuff, I found it was going to be really
> difficult to prevent other functions from getting clobbered by a delayed
> crtc enable or disable.  Daniel's work to remove a bunch of the
> ->mode_set callbacks is a good start, but we still end up doing some reg
> reads and writes in the mode set path today.  Until those are cleared up
> and we somehow enforce a rule that all hw changes go through the crtc
> enable/disable paths with everything else staged in multithread safe structs,
> I don't think the async crtc approach will be solid.

Just a quick note: Once the runtim pm for dpms code is in, there's 0 hw
touching going on in mode_set callbacks. The only thing we still do in
there is computing the dpll config.

So if this is somehow annoying your feature work I think we should wait a
bit until that's landed.

> So, since resume is the biggest issue here anyway, I've tried making
> just the resume mode set asynchronous.  Even this is a bit tricky, since
> we need to apply any pending mode set at certain points, then check
> whether the crtc we're operating on in any given path is still active.
> I think I've caught those cases here, but if we have more we can use the
> intel_sync_mode_set() call with appropriate post-call checks (after we
> re-acquire the corresponding crtc mutex).
> 
> Feedback welcome.  This has seen light testing on my BYT and really
> reduces the time spend in the i915 _thaw function, letting userspace
> start running much sooner than before.

I think we've very quickly discussed this on irc, so here's the recap. Rob
Clark's atomic modeset also has an async mode. And we have an awful lot of
code to port over to atomic modeset, and I'm vary of adding more i915
custom solutions which some poor schlock (Ville or me, most likely) needs
to convert.

So from that pov I'd really prefer we'd pour this effort into pushing
Rob's and Ville's patches forward. Of course the delaying probing can go
in, and also the other stuff you're working on I think.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: New async patch for resume

[Jesse Barnes]

On Fri, 6 Jun 2014 18:36:46 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Thu, Jun 05, 2014 at 08:48:37AM -0700, Jesse Barnes wrote:
> > In digging into the async crtc stuff, I found it was going to be really
> > difficult to prevent other functions from getting clobbered by a delayed
> > crtc enable or disable.  Daniel's work to remove a bunch of the
> > ->mode_set callbacks is a good start, but we still end up doing some reg
> > reads and writes in the mode set path today.  Until those are cleared up
> > and we somehow enforce a rule that all hw changes go through the crtc
> > enable/disable paths with everything else staged in multithread safe structs,
> > I don't think the async crtc approach will be solid.
> 
> Just a quick note: Once the runtim pm for dpms code is in, there's 0 hw
> touching going on in mode_set callbacks. The only thing we still do in
> there is computing the dpll config.
> 
> So if this is somehow annoying your feature work I think we should wait a
> bit until that's landed.

I still worry about synchronizing multiple delayed CRTC enable/disable
calls vs. mode set and struct synchronization... I need to think about
it harder.

> 
> > So, since resume is the biggest issue here anyway, I've tried making
> > just the resume mode set asynchronous.  Even this is a bit tricky, since
> > we need to apply any pending mode set at certain points, then check
> > whether the crtc we're operating on in any given path is still active.
> > I think I've caught those cases here, but if we have more we can use the
> > intel_sync_mode_set() call with appropriate post-call checks (after we
> > re-acquire the corresponding crtc mutex).
> > 
> > Feedback welcome.  This has seen light testing on my BYT and really
> > reduces the time spend in the i915 _thaw function, letting userspace
> > start running much sooner than before.
> 
> I think we've very quickly discussed this on irc, so here's the recap. Rob
> Clark's atomic modeset also has an async mode. And we have an awful lot of
> code to port over to atomic modeset, and I'm vary of adding more i915
> custom solutions which some poor schlock (Ville or me, most likely) needs
> to convert.
> 
> So from that pov I'd really prefer we'd pour this effort into pushing
> Rob's and Ville's patches forward. Of course the delaying probing can go
> in, and also the other stuff you're working on I think.

I was dreaming about this last night and I think that's really the only
safe way to do it.  I was trying to avoid touching the top level DRM
code, but really I think that's the only way to avoid races or
introduce all sorts of trickery.

I don't want to wait long though... I guess I'll pull up Rob's latest
stuff and see if there's anything I can to do help move things along.

-- 
Jesse Barnes, Intel Open Source Technology Center