[Intel-gfx] [PATCH] drm/i915: Force PSR exit by inactivating it.

[Chris Wilson]

On Fri, Jun 13, 2014 at 05:10:03AM -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.
> 
> v2: remove exit from connector_dpms. Daniel pointed this is the wrong way and
> also this isn't needed for BDW and HSW anyway.
> 
> Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
> Reviewed-by: Vijay Purushothaman <vijay.a.purushothaman at intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi at 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 |  8 ++++
>  drivers/gpu/drm/i915/intel_dp.c      | 91 ++++++++++++++++++++++++++++++++----
>  drivers/gpu/drm/i915/intel_drv.h     |  3 ++
>  5 files changed, 101 insertions(+), 10 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;

What is your locking strategy?

>  };
>  
>  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);

This is broken as you reenable PSR. As last time, it is perfectly valid
for the client to continue to write through the GTT onto the scanout
indefinitely after a single set-domain call. Bonus points for ignoring
earlier review about pin_display and checking for writes, and instead
doing it for everything.

> +
>  	/* 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);

EVERY SINGLE CALL TO BUSY?

This is instead of tracking front buffer writes? Which you do anyway.
You interchange invalidate/flush patterns for psr exit with no
explanation as to what the strategy is meant to be.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre