On Thu, Jun 12, 2014 at 10:16:41AM -0700, Rodrigo Vivi wrote:
> The perfect solution for psr_exit is the hardware tracking the changes and
> doing the psr exit by itself. This scenario works for HSW and BDW with some
> environments like Gnome and Wayland.
>
> However there are many other scenarios that this isn't true. Mainly one right
> now is KDE users on HSW and BDW with PSR on. User would miss many screen
> updates. For instances any key typed could be seen only when mouse cursor is
> moved. So this patch introduces the ability of trigger PSR exit on kernel side
> on some common cases that.
>
> Most of the cases are coverred by psr_exit at set_domain. The remaining cases
> are coverred by triggering it at set_domain, busy_ioctl, sw_finish and
> mark_busy.
>
> The downside here might be reducing the residency time on the cases this
> already work very wall like Gnome environment. But so far let's get focused
> on fixinge issues sio PSR couild be used for everybody and we could even
> get it enabled by default. Later we can add some alternatives to choose the
> level of PSR efficiency over boot flag of even over crtc property.
>
> Reviewed-by: Vijay Purushothaman <vijay.a.purushotha...@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.v...@intel.com>
> ---
> drivers/gpu/drm/i915/i915_drv.h | 3 ++
> drivers/gpu/drm/i915/i915_gem.c | 6 +++
> drivers/gpu/drm/i915/intel_display.c | 20 +++++++-
> drivers/gpu/drm/i915/intel_dp.c | 91
> ++++++++++++++++++++++++++++++++----
> drivers/gpu/drm/i915/intel_drv.h | 3 ++
> 5 files changed, 111 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index f36d9eb..64d520f 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -637,6 +637,9 @@ struct i915_psr {
> bool sink_support;
> bool source_ok;
> bool setup_done;
> + bool enabled;
> + bool active;
> + struct delayed_work work;
> };
>
> enum intel_pch {
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 1dc4b74..3637add 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1395,6 +1395,8 @@ i915_gem_set_domain_ioctl(struct drm_device *dev, void
> *data,
> goto unlock;
> }
>
> + intel_edp_psr_exit(dev, true);
> +
> /* Try to flush the object off the GPU without holding the lock.
> * We will repeat the flush holding the lock in the normal manner
> * to catch cases where we are gazumped.
> @@ -1440,6 +1442,8 @@ i915_gem_sw_finish_ioctl(struct drm_device *dev, void
> *data,
> if (ret)
> return ret;
>
> + intel_edp_psr_exit(dev, true);
> +
> obj = to_intel_bo(drm_gem_object_lookup(dev, file, args->handle));
> if (&obj->base == NULL) {
> ret = -ENOENT;
> @@ -4234,6 +4238,8 @@ i915_gem_busy_ioctl(struct drm_device *dev, void *data,
> if (ret)
> return ret;
>
> + intel_edp_psr_exit(dev, true);
> +
> obj = to_intel_bo(drm_gem_object_lookup(dev, file, args->handle));
> if (&obj->base == NULL) {
> ret = -ENOENT;
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index b5cbb28..70dc433 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5017,6 +5017,8 @@ static void intel_connector_check_state(struct
> intel_connector *connector)
> * consider. */
> void intel_connector_dpms(struct drm_connector *connector, int mode)
> {
> + struct drm_device *dev = connector->dev;
> + struct intel_encoder *intel_encoder;
> /* All the simple cases only support two dpms states. */
> if (mode != DRM_MODE_DPMS_ON)
> mode = DRM_MODE_DPMS_OFF;
> @@ -5027,9 +5029,15 @@ void intel_connector_dpms(struct drm_connector
> *connector, int mode)
> connector->dpms = mode;
>
> /* Only need to change hw state when actually enabled */
> - if (connector->encoder)
> - intel_encoder_dpms(to_intel_encoder(connector->encoder), mode);
> + if (connector->encoder) {
> +
> + intel_encoder = to_intel_encoder(connector->encoder);
> + intel_encoder_dpms(intel_encoder, mode);
>
> + if (intel_encoder->type == INTEL_OUTPUT_EDP &&
> + mode == DRM_MODE_DPMS_OFF)
> + intel_edp_psr_exit(dev, false);
Nope. intel_connector_dpms is just the generic implementation we have for
forwarding the drm core ->dpms interface to our own stuff. Sprinkling
random things in here is not ok.
This either needs to be in the crtc_disable or in an encoder->disable
function.
Sorry I didn't spot this earlier.
First two patches merged.
-Daniel
> + }
> intel_modeset_check_state(connector->dev);
> }
>
> @@ -8826,12 +8834,15 @@ out:
> intel_runtime_pm_put(dev_priv);
> }
>
> +
> void intel_mark_fb_busy(struct drm_i915_gem_object *obj,
> struct intel_engine_cs *ring)
> {
> struct drm_device *dev = obj->base.dev;
> struct drm_crtc *crtc;
>
> + intel_edp_psr_exit(dev, true);
> +
> if (!i915.powersave)
> return;
>
> @@ -9300,6 +9311,9 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
> if (work == NULL)
> return -ENOMEM;
>
> + /* Exit PSR early in page flip */
> + intel_edp_psr_exit(dev, true);
> +
> work->event = event;
> work->crtc = crtc;
> work->old_fb_obj = to_intel_framebuffer(old_fb)->obj;
> @@ -11491,6 +11505,8 @@ static void intel_setup_outputs(struct drm_device
> *dev)
> if (SUPPORTS_TV(dev))
> intel_tv_init(dev);
>
> + intel_edp_psr_init(dev);
> +
> list_for_each_entry(encoder, &dev->mode_config.encoder_list, base.head)
> {
> encoder->base.possible_crtcs = encoder->crtc_mask;
> encoder->base.possible_clones =
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 68289ce..f7b2414 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1797,10 +1797,11 @@ static bool intel_edp_psr_match_conditions(struct
> intel_dp *intel_dp)
>
> static void intel_edp_psr_do_enable(struct intel_dp *intel_dp)
> {
> - struct drm_device *dev = intel_dp_to_dev(intel_dp);
> + struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> + struct drm_device *dev = intel_dig_port->base.base.dev;
> + struct drm_i915_private *dev_priv = dev->dev_private;
>
> - if (!intel_edp_psr_match_conditions(intel_dp) ||
> - intel_edp_is_psr_enabled(dev))
> + if (intel_edp_is_psr_enabled(dev))
> return;
>
> /* Enable PSR on the panel */
> @@ -1808,6 +1809,9 @@ static void intel_edp_psr_do_enable(struct intel_dp
> *intel_dp)
>
> /* Enable PSR on the host */
> intel_edp_psr_enable_source(intel_dp);
> +
> + dev_priv->psr.enabled = true;
> + dev_priv->psr.active = true;
> }
>
> void intel_edp_psr_enable(struct intel_dp *intel_dp)
> @@ -1827,8 +1831,7 @@ void intel_edp_psr_enable(struct intel_dp *intel_dp)
> /* Setup PSR once */
> intel_edp_psr_setup(intel_dp);
>
> - if (intel_edp_psr_match_conditions(intel_dp) &&
> - !intel_edp_is_psr_enabled(dev))
> + if (intel_edp_psr_match_conditions(intel_dp))
> intel_edp_psr_do_enable(intel_dp);
> }
>
> @@ -1837,7 +1840,7 @@ void intel_edp_psr_disable(struct intel_dp *intel_dp)
> struct drm_device *dev = intel_dp_to_dev(intel_dp);
> struct drm_i915_private *dev_priv = dev->dev_private;
>
> - if (!intel_edp_is_psr_enabled(dev))
> + if (!dev_priv->psr.enabled)
> return;
>
> I915_WRITE(EDP_PSR_CTL(dev),
> @@ -1847,13 +1850,13 @@ void intel_edp_psr_disable(struct intel_dp *intel_dp)
> if (_wait_for((I915_READ(EDP_PSR_STATUS_CTL(dev)) &
> EDP_PSR_STATUS_STATE_MASK) == 0, 2000, 10))
> DRM_ERROR("Timed out waiting for PSR Idle State\n");
> +
> + dev_priv->psr.enabled = false;
> }
>
> void intel_edp_psr_update(struct drm_device *dev)
> {
> struct drm_i915_private *dev_priv = dev->dev_private;
> - struct intel_encoder *encoder;
> - struct intel_dp *intel_dp = NULL;
>
> if (!HAS_PSR(dev))
> return;
> @@ -1861,6 +1864,17 @@ void intel_edp_psr_update(struct drm_device *dev)
> if (!dev_priv->psr.setup_done)
> return;
>
> + intel_edp_psr_exit(dev, true);
> +}
> +
> +void intel_edp_psr_work(struct work_struct *work)
> +{
> + struct drm_i915_private *dev_priv =
> + container_of(work, typeof(*dev_priv), psr.work.work);
> + struct drm_device *dev = dev_priv->dev;
> + struct intel_encoder *encoder;
> + struct intel_dp *intel_dp = NULL;
> +
> list_for_each_entry(encoder, &dev->mode_config.encoder_list, base.head)
> if (encoder->type == INTEL_OUTPUT_EDP) {
> intel_dp = enc_to_intel_dp(&encoder->base);
> @@ -1868,9 +1882,66 @@ void intel_edp_psr_update(struct drm_device *dev)
> if (!intel_edp_psr_match_conditions(intel_dp))
> intel_edp_psr_disable(intel_dp);
> else
> - if (!intel_edp_is_psr_enabled(dev))
> - intel_edp_psr_do_enable(intel_dp);
> + intel_edp_psr_do_enable(intel_dp);
> + }
> +}
> +
> +void intel_edp_psr_inactivate(struct drm_device *dev)
> +{
> + struct drm_i915_private *dev_priv = dev->dev_private;
> + struct intel_connector *connector;
> + struct intel_encoder *encoder;
> + struct intel_crtc *intel_crtc;
> + struct intel_dp *intel_dp = NULL;
> +
> + list_for_each_entry(connector, &dev->mode_config.connector_list,
> + base.head) {
> +
> + if (connector->base.dpms != DRM_MODE_DPMS_ON)
> + continue;
> +
> + encoder = to_intel_encoder(connector->base.encoder);
> + if (encoder->type == INTEL_OUTPUT_EDP) {
> +
> + intel_dp = enc_to_intel_dp(&encoder->base);
> + intel_crtc = to_intel_crtc(encoder->base.crtc);
> +
> + dev_priv->psr.active = false;
> +
> + I915_WRITE(EDP_PSR_CTL(dev), I915_READ(EDP_PSR_CTL(dev))
> + & ~EDP_PSR_ENABLE);
> }
> + }
> +}
> +
> +void intel_edp_psr_exit(struct drm_device *dev, bool schedule_back)
> +{
> + struct drm_i915_private *dev_priv = dev->dev_private;
> +
> + if (!HAS_PSR(dev))
> + return;
> +
> + if (!dev_priv->psr.setup_done)
> + return;
> +
> + cancel_delayed_work_sync(&dev_priv->psr.work);
> +
> + if (dev_priv->psr.active)
> + intel_edp_psr_inactivate(dev);
> +
> + if (schedule_back)
> + schedule_delayed_work(&dev_priv->psr.work,
> + msecs_to_jiffies(100));
> +}
> +
> +void intel_edp_psr_init(struct drm_device *dev)
> +{
> + struct drm_i915_private *dev_priv = dev->dev_private;
> +
> + if (!HAS_PSR(dev))
> + return;
> +
> + INIT_DELAYED_WORK(&dev_priv->psr.work, intel_edp_psr_work);
> }
>
> static void intel_disable_dp(struct intel_encoder *encoder)
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index 1904ef8..bd90661 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -831,6 +831,9 @@ void intel_edp_psr_enable(struct intel_dp *intel_dp);
> void intel_edp_psr_disable(struct intel_dp *intel_dp);
> void intel_edp_psr_update(struct drm_device *dev);
> void intel_dp_set_drrs_state(struct drm_device *dev, int refresh_rate);
> +void intel_edp_psr_exit(struct drm_device *dev, bool schedule_back);
> +void intel_edp_psr_init(struct drm_device *dev);
> +
>
> /* intel_dsi.c */
> void intel_dsi_init(struct drm_device *dev);
> --
> 1.9.3
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
