| Message ID | 1394059711-7910-2-git-send-email-jbarnes@virtuousgeek.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 2014-03-05 at 14:48 -0800, Jesse Barnes wrote: > This lets us return to userspace more quickly and should improve init > and suspend/resume times as well, allowing us to return to userspace > sooner. IMHO this is a good move towards a full command queue based solution for kms commands, where eventually we have to think less of concurrency. That is if we can queue all the other kms commands too (flip, set_plane). But I don't see why that wouldn't be possible. Btw, why do you have a separate disable and enable queue? --Imre > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > --- > drivers/gpu/drm/i915/i915_drv.c | 2 +- > drivers/gpu/drm/i915/i915_drv.h | 4 +- > drivers/gpu/drm/i915/intel_display.c | 106 ++++++++++++++++++++++++++++------- > drivers/gpu/drm/i915/intel_drv.h | 4 ++ > 4 files changed, 94 insertions(+), 22 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index 08052f3d..29cc079 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -456,7 +456,7 @@ static int i915_drm_freeze(struct drm_device *dev) > */ > mutex_lock(&dev->mode_config.mutex); > list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) > - dev_priv->display.crtc_disable(crtc); > + dev_priv->display._crtc_disable(crtc); > mutex_unlock(&dev->mode_config.mutex); > > intel_modeset_suspend_hw(dev); > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 42f83f2..4c39bb5 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -438,8 +438,8 @@ struct drm_i915_display_funcs { > int (*crtc_mode_set)(struct drm_crtc *crtc, > int x, int y, > struct drm_framebuffer *old_fb); > - void (*crtc_enable)(struct drm_crtc *crtc); > - void (*crtc_disable)(struct drm_crtc *crtc); > + void (*_crtc_enable)(struct drm_crtc *crtc); > + void (*_crtc_disable)(struct drm_crtc *crtc); > void (*off)(struct drm_crtc *crtc); > void (*write_eld)(struct drm_connector *connector, > struct drm_crtc *crtc, > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 46ce940..c066a7d 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -1747,6 +1747,63 @@ static void lpt_disable_pch_transcoder(struct drm_i915_private *dev_priv) > I915_WRITE(_TRANSA_CHICKEN2, val); > } > > +static void intel_sync_crtc(struct drm_crtc *crtc) > +{ > + struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > + struct drm_device *dev = crtc->dev; > + > + WARN(!mutex_is_locked(&intel_crtc->base.mutex), "need crtc mutex\n"); > + > + mutex_unlock(&dev->mode_config.mutex); > + mutex_unlock(&intel_crtc->base.mutex); > + flush_work(&intel_crtc->disable_work); > + flush_work(&intel_crtc->enable_work); > + mutex_lock(&intel_crtc->base.mutex); > + mutex_lock(&dev->mode_config.mutex); > +} > + > +static void intel_crtc_disable_work(struct work_struct *work) > +{ > + struct intel_crtc *intel_crtc = container_of(work, struct intel_crtc, > + disable_work); > + struct drm_i915_private *dev_priv = intel_crtc->base.dev->dev_private; > + struct drm_device *dev = dev_priv->dev; > + > + mutex_lock(&dev->mode_config.mutex); > + mutex_lock(&intel_crtc->base.mutex); > + dev_priv->display._crtc_disable(&intel_crtc->base); > + mutex_unlock(&intel_crtc->base.mutex); > + mutex_unlock(&dev->mode_config.mutex); > +} > + > +void intel_queue_crtc_disable(struct drm_crtc *crtc) > +{ > + struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > + > + schedule_work(&intel_crtc->disable_work); > +} > + > +static void intel_crtc_enable_work(struct work_struct *work) > +{ > + struct intel_crtc *intel_crtc = container_of(work, struct intel_crtc, > + enable_work); > + struct drm_i915_private *dev_priv = intel_crtc->base.dev->dev_private; > + struct drm_device *dev = dev_priv->dev; > + > + mutex_lock(&dev->mode_config.mutex); > + mutex_lock(&intel_crtc->base.mutex); > + dev_priv->display._crtc_enable(&intel_crtc->base); > + mutex_unlock(&intel_crtc->base.mutex); > + mutex_unlock(&dev->mode_config.mutex); > +} > + > +static void intel_queue_crtc_enable(struct drm_crtc *crtc) > +{ > + struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > + > + schedule_work(&intel_crtc->enable_work); > +} > + > /** > * intel_enable_pipe - enable a pipe, asserting requirements > * @crtc: crtc responsible for the pipe > @@ -4309,7 +4366,6 @@ static void intel_crtc_update_sarea(struct drm_crtc *crtc, > void intel_crtc_update_dpms(struct drm_crtc *crtc) > { > struct drm_device *dev = crtc->dev; > - struct drm_i915_private *dev_priv = dev->dev_private; > struct intel_encoder *intel_encoder; > bool enable = false; > > @@ -4317,9 +4373,9 @@ void intel_crtc_update_dpms(struct drm_crtc *crtc) > enable |= intel_encoder->connectors_active; > > if (enable) > - dev_priv->display.crtc_enable(crtc); > + intel_queue_crtc_enable(crtc); > else > - dev_priv->display.crtc_disable(crtc); > + intel_queue_crtc_disable(crtc); > > intel_crtc_update_sarea(crtc, enable); > } > @@ -4334,7 +4390,7 @@ static void intel_crtc_disable(struct drm_crtc *crtc) > /* crtc should still be enabled when we disable it. */ > WARN_ON(!crtc->enabled); > > - dev_priv->display.crtc_disable(crtc); > + dev_priv->display._crtc_disable(crtc); > intel_crtc->eld_vld = false; > intel_crtc_update_sarea(crtc, false); > dev_priv->display.off(crtc); > @@ -7554,6 +7610,8 @@ static int intel_crtc_cursor_set(struct drm_crtc *crtc, > goto fail; > } > > + intel_sync_crtc(crtc); > + > /* we only need to pin inside GTT if cursor is non-phy */ > mutex_lock(&dev->struct_mutex); > if (!INTEL_INFO(dev)->cursor_needs_physical) { > @@ -7639,6 +7697,8 @@ static int intel_crtc_cursor_move(struct drm_crtc *crtc, int x, int y) > intel_crtc->cursor_x = clamp_t(int, x, SHRT_MIN, SHRT_MAX); > intel_crtc->cursor_y = clamp_t(int, y, SHRT_MIN, SHRT_MAX); > > + intel_sync_crtc(crtc); > + > if (intel_crtc->active) > intel_crtc_update_cursor(crtc, intel_crtc->cursor_bo != NULL); > > @@ -8682,6 +8742,8 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc, > if (work == NULL) > return -ENOMEM; > > + intel_sync_crtc(crtc); > + > work->event = event; > work->crtc = crtc; > work->old_fb_obj = to_intel_framebuffer(old_fb)->obj; > @@ -9677,8 +9739,10 @@ static int __intel_set_mode(struct drm_crtc *crtc, > intel_crtc_disable(&intel_crtc->base); > > for_each_intel_crtc_masked(dev, prepare_pipes, intel_crtc) { > - if (intel_crtc->base.enabled) > - dev_priv->display.crtc_disable(&intel_crtc->base); > + if (intel_crtc->base.enabled) { > + intel_queue_crtc_disable(&intel_crtc->base); > + intel_sync_crtc(&intel_crtc->base); > + } > } > > /* crtc->mode is already used by the ->mode_set callbacks, hence we need > @@ -9719,7 +9783,7 @@ static int __intel_set_mode(struct drm_crtc *crtc, > > /* Now enable the clocks, plane, pipe, and connectors that we set up. */ > for_each_intel_crtc_masked(dev, prepare_pipes, intel_crtc) > - dev_priv->display.crtc_enable(&intel_crtc->base); > + intel_queue_crtc_enable(&intel_crtc->base); > > /* FIXME: add subpixel order */ > done: > @@ -9740,8 +9804,9 @@ static int intel_set_mode(struct drm_crtc *crtc, > > ret = __intel_set_mode(crtc, mode, x, y, fb); > > - if (ret == 0) > - intel_modeset_check_state(crtc->dev); > + /* FIXME: check after async crtc enable/disable */ > +// if (ret == 0) > +// intel_modeset_check_state(crtc->dev); > > return ret; > } > @@ -10306,6 +10371,9 @@ static void intel_crtc_init(struct drm_device *dev, int pipe) > dev_priv->pipe_to_crtc_mapping[intel_crtc->pipe] = &intel_crtc->base; > > drm_crtc_helper_add(&intel_crtc->base, &intel_helper_funcs); > + > + INIT_WORK(&intel_crtc->enable_work, intel_crtc_enable_work); > + INIT_WORK(&intel_crtc->disable_work, intel_crtc_disable_work); > } > > enum pipe intel_get_pipe_from_connector(struct intel_connector *connector) > @@ -10716,29 +10784,29 @@ static void intel_init_display(struct drm_device *dev) > if (HAS_DDI(dev)) { > dev_priv->display.get_pipe_config = haswell_get_pipe_config; > dev_priv->display.crtc_mode_set = haswell_crtc_mode_set; > - dev_priv->display.crtc_enable = haswell_crtc_enable; > - dev_priv->display.crtc_disable = haswell_crtc_disable; > + dev_priv->display._crtc_enable = haswell_crtc_enable; > + dev_priv->display._crtc_disable = haswell_crtc_disable; > dev_priv->display.off = haswell_crtc_off; > dev_priv->display.update_plane = ironlake_update_plane; > } else if (HAS_PCH_SPLIT(dev)) { > dev_priv->display.get_pipe_config = ironlake_get_pipe_config; > dev_priv->display.crtc_mode_set = ironlake_crtc_mode_set; > - dev_priv->display.crtc_enable = ironlake_crtc_enable; > - dev_priv->display.crtc_disable = ironlake_crtc_disable; > + dev_priv->display._crtc_enable = ironlake_crtc_enable; > + dev_priv->display._crtc_disable = ironlake_crtc_disable; > dev_priv->display.off = ironlake_crtc_off; > dev_priv->display.update_plane = ironlake_update_plane; > } else if (IS_VALLEYVIEW(dev)) { > dev_priv->display.get_pipe_config = i9xx_get_pipe_config; > dev_priv->display.crtc_mode_set = i9xx_crtc_mode_set; > - dev_priv->display.crtc_enable = valleyview_crtc_enable; > - dev_priv->display.crtc_disable = i9xx_crtc_disable; > + dev_priv->display._crtc_enable = valleyview_crtc_enable; > + dev_priv->display._crtc_disable = i9xx_crtc_disable; > dev_priv->display.off = i9xx_crtc_off; > dev_priv->display.update_plane = i9xx_update_plane; > } else { > dev_priv->display.get_pipe_config = i9xx_get_pipe_config; > dev_priv->display.crtc_mode_set = i9xx_crtc_mode_set; > - dev_priv->display.crtc_enable = i9xx_crtc_enable; > - dev_priv->display.crtc_disable = i9xx_crtc_disable; > + dev_priv->display._crtc_enable = i9xx_crtc_enable; > + dev_priv->display._crtc_disable = i9xx_crtc_disable; > dev_priv->display.off = i9xx_crtc_off; > dev_priv->display.update_plane = i9xx_update_plane; > } > @@ -11142,7 +11210,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc) > * ... */ > plane = crtc->plane; > crtc->plane = !plane; > - dev_priv->display.crtc_disable(&crtc->base); > + intel_queue_crtc_disable(&crtc->base); > crtc->plane = plane; > > /* ... and break all links. */ > @@ -11417,7 +11485,7 @@ void intel_modeset_setup_hw_state(struct drm_device *dev, > intel_modeset_update_staged_output_state(dev); > } > > - intel_modeset_check_state(dev); > +// intel_modeset_check_state(dev); > } > > void intel_modeset_gem_init(struct drm_device *dev) > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index a4ffc02..b90f1f5 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -384,6 +384,9 @@ struct intel_crtc { > /* watermarks currently being used */ > struct intel_pipe_wm active; > } wm; > + > + struct work_struct enable_work; > + struct work_struct disable_work; > }; > > struct intel_plane_wm_parameters { > @@ -736,6 +739,7 @@ void intel_display_set_init_power(struct drm_device *dev, bool enable); > int valleyview_get_vco(struct drm_i915_private *dev_priv); > void intel_mode_from_pipe_config(struct drm_display_mode *mode, > struct intel_crtc_config *pipe_config); > +void intel_queue_crtc_disable(struct drm_crtc *crtc); > > /* intel_dp.c */ > void intel_dp_init(struct drm_device *dev, int output_reg, enum port port);
On Thu, 06 Mar 2014 01:29:14 +0200 Imre Deak <imre.deak@intel.com> wrote: > On Wed, 2014-03-05 at 14:48 -0800, Jesse Barnes wrote: > > This lets us return to userspace more quickly and should improve init > > and suspend/resume times as well, allowing us to return to userspace > > sooner. > > IMHO this is a good move towards a full command queue based solution for > kms commands, where eventually we have to think less of concurrency. > That is if we can queue all the other kms commands too (flip, > set_plane). But I don't see why that wouldn't be possible. > > Btw, why do you have a separate disable and enable queue? As opposed to a dedicated work queue for both combined? I had a separate queue in an earlier patch, but dropped it while debugging some other stuff. We should bring it back to ensure ordering. That would remove the need for a few of the syncs, and would also let us queue a check at the appropriate time on the same queue.
On Wed, 2014-03-05 at 15:39 -0800, Jesse Barnes wrote: > On Thu, 06 Mar 2014 01:29:14 +0200 > Imre Deak <imre.deak@intel.com> wrote: > > > On Wed, 2014-03-05 at 14:48 -0800, Jesse Barnes wrote: > > > This lets us return to userspace more quickly and should improve init > > > and suspend/resume times as well, allowing us to return to userspace > > > sooner. > > > > IMHO this is a good move towards a full command queue based solution for > > kms commands, where eventually we have to think less of concurrency. > > That is if we can queue all the other kms commands too (flip, > > set_plane). But I don't see why that wouldn't be possible. > > > > Btw, why do you have a separate disable and enable queue? > > As opposed to a dedicated work queue for both combined? I had a > separate queue in an earlier patch, but dropped it while debugging some > other stuff. We should bring it back to ensure ordering. That would > remove the need for a few of the syncs, and would also let us queue a > check at the appropriate time on the same queue. Ah, sorry I confused myself. I didn't notice you used the global workqueue. I'm not sure about the ordering guarantees of the global workqueue, but if that's not given then yea a dedicated ordered workqueue would seem better. In any case if this can move towards a more generic command queue solution then it's great. --Imre
On Wed, Mar 05, 2014 at 02:48:26PM -0800, Jesse Barnes wrote: > @@ -7554,6 +7610,8 @@ static int intel_crtc_cursor_set(struct drm_crtc *crtc, > goto fail; > } > > + intel_sync_crtc(crtc); > + > /* we only need to pin inside GTT if cursor is non-phy */ > mutex_lock(&dev->struct_mutex); > if (!INTEL_INFO(dev)->cursor_needs_physical) { > @@ -7639,6 +7697,8 @@ static int intel_crtc_cursor_move(struct drm_crtc *crtc, int x, int y) > intel_crtc->cursor_x = clamp_t(int, x, SHRT_MIN, SHRT_MAX); > intel_crtc->cursor_y = clamp_t(int, y, SHRT_MIN, SHRT_MAX); > > + intel_sync_crtc(crtc); > + > if (intel_crtc->active) > intel_crtc_update_cursor(crtc, intel_crtc->cursor_bo != NULL); > Hmm. Would be much nicer if touching the cursor didn't incur a delay. And it would seem to quite easy to capture the state change and queue it for when the CRTC is re-enabled. > @@ -8682,6 +8742,8 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc, > if (work == NULL) > return -ENOMEM; > > + intel_sync_crtc(crtc); > + > work->event = event; > work->crtc = crtc; > work->old_fb_obj = to_intel_framebuffer(old_fb)->obj; > @@ -9677,8 +9739,10 @@ static int __intel_set_mode(struct drm_crtc *crtc, > intel_crtc_disable(&intel_crtc->base); > > for_each_intel_crtc_masked(dev, prepare_pipes, intel_crtc) { > - if (intel_crtc->base.enabled) > - dev_priv->display.crtc_disable(&intel_crtc->base); > + if (intel_crtc->base.enabled) { > + intel_queue_crtc_disable(&intel_crtc->base); > + intel_sync_crtc(&intel_crtc->base); > + } > } > > /* crtc->mode is already used by the ->mode_set callbacks, hence we need > @@ -9719,7 +9783,7 @@ static int __intel_set_mode(struct drm_crtc *crtc, > > /* Now enable the clocks, plane, pipe, and connectors that we set up. */ > for_each_intel_crtc_masked(dev, prepare_pipes, intel_crtc) > - dev_priv->display.crtc_enable(&intel_crtc->base); > + intel_queue_crtc_enable(&intel_crtc->base); > > /* FIXME: add subpixel order */ > done: > @@ -9740,8 +9804,9 @@ static int intel_set_mode(struct drm_crtc *crtc, > > ret = __intel_set_mode(crtc, mode, x, y, fb); > > - if (ret == 0) > - intel_modeset_check_state(crtc->dev); > + /* FIXME: check after async crtc enable/disable */ > +// if (ret == 0) > +// intel_modeset_check_state(crtc->dev); > > return ret; > } > @@ -10306,6 +10371,9 @@ static void intel_crtc_init(struct drm_device *dev, int pipe) > dev_priv->pipe_to_crtc_mapping[intel_crtc->pipe] = &intel_crtc->base; > > drm_crtc_helper_add(&intel_crtc->base, &intel_helper_funcs); > + > + INIT_WORK(&intel_crtc->enable_work, intel_crtc_enable_work); > + INIT_WORK(&intel_crtc->disable_work, intel_crtc_disable_work); I feel using independent work items (remember the global wq is really a pool of many wq) is horribly prone to deadlocks. We have the usual caveat that this has an implicit API change in that setcrtc can now return before the change is complete - and so userspace may write to a still currently visible scanout. Its not a huge issue (and is a change I am in favour of), it is just a change in behaviour we have to be wary of (which also means stating it in the changelog for future reference). -Chris
On Thu, 6 Mar 2014 09:35:23 +0000 Chris Wilson <chris@chris-wilson.co.uk> wrote: > On Wed, Mar 05, 2014 at 02:48:26PM -0800, Jesse Barnes wrote: > > @@ -7554,6 +7610,8 @@ static int intel_crtc_cursor_set(struct drm_crtc *crtc, > > goto fail; > > } > > > > + intel_sync_crtc(crtc); > > + > > /* we only need to pin inside GTT if cursor is non-phy */ > > mutex_lock(&dev->struct_mutex); > > if (!INTEL_INFO(dev)->cursor_needs_physical) { > > @@ -7639,6 +7697,8 @@ static int intel_crtc_cursor_move(struct drm_crtc *crtc, int x, int y) > > intel_crtc->cursor_x = clamp_t(int, x, SHRT_MIN, SHRT_MAX); > > intel_crtc->cursor_y = clamp_t(int, y, SHRT_MIN, SHRT_MAX); > > > > + intel_sync_crtc(crtc); > > + > > if (intel_crtc->active) > > intel_crtc_update_cursor(crtc, intel_crtc->cursor_bo != NULL); > > > > Hmm. Would be much nicer if touching the cursor didn't incur a delay. > And it would seem to quite easy to capture the state change and queue it > for when the CRTC is re-enabled. Do you think that's worthwhile? I guess we'll block userspace a bit here, but presumably the cursor won't be visible until the mode set completes anyway... But queuing this stuff is another option. > > > @@ -8682,6 +8742,8 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc, > > if (work == NULL) > > return -ENOMEM; > > > > + intel_sync_crtc(crtc); > > + > > work->event = event; > > work->crtc = crtc; > > work->old_fb_obj = to_intel_framebuffer(old_fb)->obj; > > @@ -9677,8 +9739,10 @@ static int __intel_set_mode(struct drm_crtc *crtc, > > intel_crtc_disable(&intel_crtc->base); > > > > for_each_intel_crtc_masked(dev, prepare_pipes, intel_crtc) { > > - if (intel_crtc->base.enabled) > > - dev_priv->display.crtc_disable(&intel_crtc->base); > > + if (intel_crtc->base.enabled) { > > + intel_queue_crtc_disable(&intel_crtc->base); > > + intel_sync_crtc(&intel_crtc->base); > > + } > > } > > > > /* crtc->mode is already used by the ->mode_set callbacks, hence we need > > @@ -9719,7 +9783,7 @@ static int __intel_set_mode(struct drm_crtc *crtc, > > > > /* Now enable the clocks, plane, pipe, and connectors that we set up. */ > > for_each_intel_crtc_masked(dev, prepare_pipes, intel_crtc) > > - dev_priv->display.crtc_enable(&intel_crtc->base); > > + intel_queue_crtc_enable(&intel_crtc->base); > > > > /* FIXME: add subpixel order */ > > done: > > @@ -9740,8 +9804,9 @@ static int intel_set_mode(struct drm_crtc *crtc, > > > > ret = __intel_set_mode(crtc, mode, x, y, fb); > > > > - if (ret == 0) > > - intel_modeset_check_state(crtc->dev); > > + /* FIXME: check after async crtc enable/disable */ > > +// if (ret == 0) > > +// intel_modeset_check_state(crtc->dev); > > > > return ret; > > } > > @@ -10306,6 +10371,9 @@ static void intel_crtc_init(struct drm_device *dev, int pipe) > > dev_priv->pipe_to_crtc_mapping[intel_crtc->pipe] = &intel_crtc->base; > > > > drm_crtc_helper_add(&intel_crtc->base, &intel_helper_funcs); > > + > > + INIT_WORK(&intel_crtc->enable_work, intel_crtc_enable_work); > > + INIT_WORK(&intel_crtc->disable_work, intel_crtc_disable_work); > > I feel using independent work items (remember the global wq is really a > pool of many wq) is horribly prone to deadlocks. > > We have the usual caveat that this has an implicit API change in that > setcrtc can now return before the change is complete - and so userspace > may write to a still currently visible scanout. Its not a huge issue > (and is a change I am in favour of), it is just a change in behaviour we > have to be wary of (which also means stating it in the changelog for future > reference). Yeah that's a good point, and if we're not careful it could result in some visible ugliness.
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 08052f3d..29cc079 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -456,7 +456,7 @@ static int i915_drm_freeze(struct drm_device *dev) */ mutex_lock(&dev->mode_config.mutex); list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) - dev_priv->display.crtc_disable(crtc); + dev_priv->display._crtc_disable(crtc); mutex_unlock(&dev->mode_config.mutex); intel_modeset_suspend_hw(dev); diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 42f83f2..4c39bb5 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -438,8 +438,8 @@ struct drm_i915_display_funcs { int (*crtc_mode_set)(struct drm_crtc *crtc, int x, int y, struct drm_framebuffer *old_fb); - void (*crtc_enable)(struct drm_crtc *crtc); - void (*crtc_disable)(struct drm_crtc *crtc); + void (*_crtc_enable)(struct drm_crtc *crtc); + void (*_crtc_disable)(struct drm_crtc *crtc); void (*off)(struct drm_crtc *crtc); void (*write_eld)(struct drm_connector *connector, struct drm_crtc *crtc, diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 46ce940..c066a7d 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -1747,6 +1747,63 @@ static void lpt_disable_pch_transcoder(struct drm_i915_private *dev_priv) I915_WRITE(_TRANSA_CHICKEN2, val); } +static void intel_sync_crtc(struct drm_crtc *crtc) +{ + struct intel_crtc *intel_crtc = to_intel_crtc(crtc); + struct drm_device *dev = crtc->dev; + + WARN(!mutex_is_locked(&intel_crtc->base.mutex), "need crtc mutex\n"); + + mutex_unlock(&dev->mode_config.mutex); + mutex_unlock(&intel_crtc->base.mutex); + flush_work(&intel_crtc->disable_work); + flush_work(&intel_crtc->enable_work); + mutex_lock(&intel_crtc->base.mutex); + mutex_lock(&dev->mode_config.mutex); +} + +static void intel_crtc_disable_work(struct work_struct *work) +{ + struct intel_crtc *intel_crtc = container_of(work, struct intel_crtc, + disable_work); + struct drm_i915_private *dev_priv = intel_crtc->base.dev->dev_private; + struct drm_device *dev = dev_priv->dev; + + mutex_lock(&dev->mode_config.mutex); + mutex_lock(&intel_crtc->base.mutex); + dev_priv->display._crtc_disable(&intel_crtc->base); + mutex_unlock(&intel_crtc->base.mutex); + mutex_unlock(&dev->mode_config.mutex); +} + +void intel_queue_crtc_disable(struct drm_crtc *crtc) +{ + struct intel_crtc *intel_crtc = to_intel_crtc(crtc); + + schedule_work(&intel_crtc->disable_work); +} + +static void intel_crtc_enable_work(struct work_struct *work) +{ + struct intel_crtc *intel_crtc = container_of(work, struct intel_crtc, + enable_work); + struct drm_i915_private *dev_priv = intel_crtc->base.dev->dev_private; + struct drm_device *dev = dev_priv->dev; + + mutex_lock(&dev->mode_config.mutex); + mutex_lock(&intel_crtc->base.mutex); + dev_priv->display._crtc_enable(&intel_crtc->base); + mutex_unlock(&intel_crtc->base.mutex); + mutex_unlock(&dev->mode_config.mutex); +} + +static void intel_queue_crtc_enable(struct drm_crtc *crtc) +{ + struct intel_crtc *intel_crtc = to_intel_crtc(crtc); + + schedule_work(&intel_crtc->enable_work); +} + /** * intel_enable_pipe - enable a pipe, asserting requirements * @crtc: crtc responsible for the pipe @@ -4309,7 +4366,6 @@ static void intel_crtc_update_sarea(struct drm_crtc *crtc, void intel_crtc_update_dpms(struct drm_crtc *crtc) { struct drm_device *dev = crtc->dev; - struct drm_i915_private *dev_priv = dev->dev_private; struct intel_encoder *intel_encoder; bool enable = false; @@ -4317,9 +4373,9 @@ void intel_crtc_update_dpms(struct drm_crtc *crtc) enable |= intel_encoder->connectors_active; if (enable) - dev_priv->display.crtc_enable(crtc); + intel_queue_crtc_enable(crtc); else - dev_priv->display.crtc_disable(crtc); + intel_queue_crtc_disable(crtc); intel_crtc_update_sarea(crtc, enable); } @@ -4334,7 +4390,7 @@ static void intel_crtc_disable(struct drm_crtc *crtc) /* crtc should still be enabled when we disable it. */ WARN_ON(!crtc->enabled); - dev_priv->display.crtc_disable(crtc); + dev_priv->display._crtc_disable(crtc); intel_crtc->eld_vld = false; intel_crtc_update_sarea(crtc, false); dev_priv->display.off(crtc); @@ -7554,6 +7610,8 @@ static int intel_crtc_cursor_set(struct drm_crtc *crtc, goto fail; } + intel_sync_crtc(crtc); + /* we only need to pin inside GTT if cursor is non-phy */ mutex_lock(&dev->struct_mutex); if (!INTEL_INFO(dev)->cursor_needs_physical) { @@ -7639,6 +7697,8 @@ static int intel_crtc_cursor_move(struct drm_crtc *crtc, int x, int y) intel_crtc->cursor_x = clamp_t(int, x, SHRT_MIN, SHRT_MAX); intel_crtc->cursor_y = clamp_t(int, y, SHRT_MIN, SHRT_MAX); + intel_sync_crtc(crtc); + if (intel_crtc->active) intel_crtc_update_cursor(crtc, intel_crtc->cursor_bo != NULL); @@ -8682,6 +8742,8 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc, if (work == NULL) return -ENOMEM; + intel_sync_crtc(crtc); + work->event = event; work->crtc = crtc; work->old_fb_obj = to_intel_framebuffer(old_fb)->obj; @@ -9677,8 +9739,10 @@ static int __intel_set_mode(struct drm_crtc *crtc, intel_crtc_disable(&intel_crtc->base); for_each_intel_crtc_masked(dev, prepare_pipes, intel_crtc) { - if (intel_crtc->base.enabled) - dev_priv->display.crtc_disable(&intel_crtc->base); + if (intel_crtc->base.enabled) { + intel_queue_crtc_disable(&intel_crtc->base); + intel_sync_crtc(&intel_crtc->base); + } } /* crtc->mode is already used by the ->mode_set callbacks, hence we need @@ -9719,7 +9783,7 @@ static int __intel_set_mode(struct drm_crtc *crtc, /* Now enable the clocks, plane, pipe, and connectors that we set up. */ for_each_intel_crtc_masked(dev, prepare_pipes, intel_crtc) - dev_priv->display.crtc_enable(&intel_crtc->base); + intel_queue_crtc_enable(&intel_crtc->base); /* FIXME: add subpixel order */ done: @@ -9740,8 +9804,9 @@ static int intel_set_mode(struct drm_crtc *crtc, ret = __intel_set_mode(crtc, mode, x, y, fb); - if (ret == 0) - intel_modeset_check_state(crtc->dev); + /* FIXME: check after async crtc enable/disable */ +// if (ret == 0) +// intel_modeset_check_state(crtc->dev); return ret; } @@ -10306,6 +10371,9 @@ static void intel_crtc_init(struct drm_device *dev, int pipe) dev_priv->pipe_to_crtc_mapping[intel_crtc->pipe] = &intel_crtc->base; drm_crtc_helper_add(&intel_crtc->base, &intel_helper_funcs); + + INIT_WORK(&intel_crtc->enable_work, intel_crtc_enable_work); + INIT_WORK(&intel_crtc->disable_work, intel_crtc_disable_work); } enum pipe intel_get_pipe_from_connector(struct intel_connector *connector) @@ -10716,29 +10784,29 @@ static void intel_init_display(struct drm_device *dev) if (HAS_DDI(dev)) { dev_priv->display.get_pipe_config = haswell_get_pipe_config; dev_priv->display.crtc_mode_set = haswell_crtc_mode_set; - dev_priv->display.crtc_enable = haswell_crtc_enable; - dev_priv->display.crtc_disable = haswell_crtc_disable; + dev_priv->display._crtc_enable = haswell_crtc_enable; + dev_priv->display._crtc_disable = haswell_crtc_disable; dev_priv->display.off = haswell_crtc_off; dev_priv->display.update_plane = ironlake_update_plane; } else if (HAS_PCH_SPLIT(dev)) { dev_priv->display.get_pipe_config = ironlake_get_pipe_config; dev_priv->display.crtc_mode_set = ironlake_crtc_mode_set; - dev_priv->display.crtc_enable = ironlake_crtc_enable; - dev_priv->display.crtc_disable = ironlake_crtc_disable; + dev_priv->display._crtc_enable = ironlake_crtc_enable; + dev_priv->display._crtc_disable = ironlake_crtc_disable; dev_priv->display.off = ironlake_crtc_off; dev_priv->display.update_plane = ironlake_update_plane; } else if (IS_VALLEYVIEW(dev)) { dev_priv->display.get_pipe_config = i9xx_get_pipe_config; dev_priv->display.crtc_mode_set = i9xx_crtc_mode_set; - dev_priv->display.crtc_enable = valleyview_crtc_enable; - dev_priv->display.crtc_disable = i9xx_crtc_disable; + dev_priv->display._crtc_enable = valleyview_crtc_enable; + dev_priv->display._crtc_disable = i9xx_crtc_disable; dev_priv->display.off = i9xx_crtc_off; dev_priv->display.update_plane = i9xx_update_plane; } else { dev_priv->display.get_pipe_config = i9xx_get_pipe_config; dev_priv->display.crtc_mode_set = i9xx_crtc_mode_set; - dev_priv->display.crtc_enable = i9xx_crtc_enable; - dev_priv->display.crtc_disable = i9xx_crtc_disable; + dev_priv->display._crtc_enable = i9xx_crtc_enable; + dev_priv->display._crtc_disable = i9xx_crtc_disable; dev_priv->display.off = i9xx_crtc_off; dev_priv->display.update_plane = i9xx_update_plane; } @@ -11142,7 +11210,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc) * ... */ plane = crtc->plane; crtc->plane = !plane; - dev_priv->display.crtc_disable(&crtc->base); + intel_queue_crtc_disable(&crtc->base); crtc->plane = plane; /* ... and break all links. */ @@ -11417,7 +11485,7 @@ void intel_modeset_setup_hw_state(struct drm_device *dev, intel_modeset_update_staged_output_state(dev); } - intel_modeset_check_state(dev); +// intel_modeset_check_state(dev); } void intel_modeset_gem_init(struct drm_device *dev) diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index a4ffc02..b90f1f5 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -384,6 +384,9 @@ struct intel_crtc { /* watermarks currently being used */ struct intel_pipe_wm active; } wm; + + struct work_struct enable_work; + struct work_struct disable_work; }; struct intel_plane_wm_parameters { @@ -736,6 +739,7 @@ void intel_display_set_init_power(struct drm_device *dev, bool enable); int valleyview_get_vco(struct drm_i915_private *dev_priv); void intel_mode_from_pipe_config(struct drm_display_mode *mode, struct intel_crtc_config *pipe_config); +void intel_queue_crtc_disable(struct drm_crtc *crtc); /* intel_dp.c */ void intel_dp_init(struct drm_device *dev, int output_reg, enum port port);
This lets us return to userspace more quickly and should improve init and suspend/resume times as well, allowing us to return to userspace sooner. Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> --- drivers/gpu/drm/i915/i915_drv.c | 2 +- drivers/gpu/drm/i915/i915_drv.h | 4 +- drivers/gpu/drm/i915/intel_display.c | 106 ++++++++++++++++++++++++++++------- drivers/gpu/drm/i915/intel_drv.h | 4 ++ 4 files changed, 94 insertions(+), 22 deletions(-)