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@xxxxxxxx>
> Reviewed-by: Vijay Purushothaman <vijay.a.purushothaman@xxxxxxxxx>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
> ---
> 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
