Re: [PATCH] drm/i915/skl: Enabling PSR on Skylake

[Daniel Vetter <daniel@ffwll.ch>]

On Mon, Jan 19, 2015 at 05:10:58PM +0530, sonika wrote:
> 
> On Saturday 17 January 2015 09:54 AM, Daniel Vetter wrote:
> >On Fri, Jan 16, 2015 at 02:07:26PM +0530, Sonika Jindal wrote:
> >>Mainly taking care of some register offsets, otherwise things are similar to
> >>hsw. Also, programming ddi aux to use hardcoded values for psr data select.
> >>
> >>v2: introduce  EDP_PSR_AUX_BASE macro (Chris)
> >>v3: Moving to HW tracking for SKL+ platforms, so activating source psr during
> >>psr_enabling and then avoiding psr entries and exits for each frontbuffer
> >>updates.
> >>
> >>Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>
> >>---
> >>  drivers/gpu/drm/i915/i915_drv.h          |    3 ++-
> >>  drivers/gpu/drm/i915/i915_reg.h          |   19 +++++++++++++------
> >>  drivers/gpu/drm/i915/intel_frontbuffer.c |    7 +++++--
> >>  drivers/gpu/drm/i915/intel_psr.c         |   18 +++++++++++++++++-
> >>  4 files changed, 37 insertions(+), 10 deletions(-)
> >>
> >>diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> >>index 42c69ca..ee5cd3b 100644
> >>--- a/drivers/gpu/drm/i915/i915_drv.h
> >>+++ b/drivers/gpu/drm/i915/i915_drv.h
> >>@@ -2355,7 +2355,8 @@ struct drm_i915_cmd_table {
> >>  #define HAS_DDI(dev) (INTEL_INFO(dev)->has_ddi)
> >>  #define HAS_FPGA_DBG_UNCLAIMED(dev) (INTEL_INFO(dev)->has_fpga_dbg)
> >>  #define HAS_PSR(dev) (IS_HASWELL(dev) || IS_BROADWELL(dev) || \
> >>- IS_VALLEYVIEW(dev) || IS_CHERRYVIEW(dev))
> >>+ IS_VALLEYVIEW(dev) || IS_CHERRYVIEW(dev) || \
> >>+ IS_SKYLAKE(dev))
> >>  #define HAS_RUNTIME_PM(dev) (IS_GEN6(dev) || IS_HASWELL(dev) || \
> >>   IS_BROADWELL(dev) || IS_VALLEYVIEW(dev))
> >>  #define HAS_RC6(dev) (INTEL_INFO(dev)->gen >= 6)
> >>diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> >>index a828cf5..068c8da 100644
> >>--- a/drivers/gpu/drm/i915/i915_reg.h
> >>+++ b/drivers/gpu/drm/i915/i915_reg.h
> >>@@ -2619,12 +2619,14 @@ enum skl_disp_power_wells {
> >>  #define   EDP_PSR_TP1_TIME_0us (3<<4)
> >>  #define   EDP_PSR_IDLE_FRAME_SHIFT 0
> >>
> >>-#define EDP_PSR_AUX_CTL(dev) (EDP_PSR_BASE(dev) + 0x10)
> >>-#define EDP_PSR_AUX_DATA1(dev) (EDP_PSR_BASE(dev) + 0x14)
> >>-#define EDP_PSR_AUX_DATA2(dev) (EDP_PSR_BASE(dev) + 0x18)
> >>-#define EDP_PSR_AUX_DATA3(dev) (EDP_PSR_BASE(dev) + 0x1c)
> >>-#define EDP_PSR_AUX_DATA4(dev) (EDP_PSR_BASE(dev) + 0x20)
> >>-#define EDP_PSR_AUX_DATA5(dev) (EDP_PSR_BASE(dev) + 0x24)
> >>+#define EDP_PSR_AUX_BASE(dev) (INTEL_INFO(dev)->gen >= 9 ? \
> >>+ 0x64000 : EDP_PSR_BASE(dev))
> >>+#define EDP_PSR_AUX_CTL(dev) (EDP_PSR_AUX_BASE(dev) + 0x10)
> >>+#define EDP_PSR_AUX_DATA1(dev) (EDP_PSR_AUX_BASE(dev) + 0x14)
> >>+#define EDP_PSR_AUX_DATA2(dev) (EDP_PSR_AUX_BASE(dev) + 0x18)
> >>+#define EDP_PSR_AUX_DATA3(dev) (EDP_PSR_AUX_BASE(dev) + 0x1c)
> >>+#define EDP_PSR_AUX_DATA4(dev) (EDP_PSR_AUX_BASE(dev) + 0x20)
> >>+#define EDP_PSR_AUX_DATA5(dev) (EDP_PSR_AUX_BASE(dev) + 0x24)
> >>
> >>  #define EDP_PSR_STATUS_CTL(dev) (EDP_PSR_BASE(dev) + 0x40)
> >>  #define   EDP_PSR_STATUS_STATE_MASK (7<<29)
> >>@@ -3771,6 +3773,11 @@ enum skl_disp_power_wells {
> >>  #define   DP_AUX_CH_CTL_PRECHARGE_TEST    (1 << 11)
> >>  #define   DP_AUX_CH_CTL_BIT_CLOCK_2X_MASK    (0x7ff)
> >>  #define   DP_AUX_CH_CTL_BIT_CLOCK_2X_SHIFT   0
> >>+#define   DP_AUX_CH_CTL_PSR_DATA_AUX_REG_SKL (1 << 14)
> >>+#define   DP_AUX_CH_CTL_FS_DATA_AUX_REG_SKL (1 << 13)
> >>+#define   DP_AUX_CH_CTL_GTC_DATA_AUX_REG_SKL (1 << 12)
> >>+#define   DP_AUX_CH_CTL_FW_SYNC_PULSE_SKL_MASK (1f << 5)
> >>+#define   DP_AUX_CH_CTL_FW_SYNC_PULSE_SKL(c) (((c) - 1) << 5)
> >>  #define   DP_AUX_CH_CTL_SYNC_PULSE_SKL(c)   ((c) - 1)
> >>
> >>  /*
> >>diff --git a/drivers/gpu/drm/i915/intel_frontbuffer.c b/drivers/gpu/drm/i915/intel_frontbuffer.c
> >>index 79f6d72..010d550 100644
> >>--- a/drivers/gpu/drm/i915/intel_frontbuffer.c
> >>+++ b/drivers/gpu/drm/i915/intel_frontbuffer.c
> >>@@ -156,7 +156,9 @@ void intel_fb_obj_invalidate(struct drm_i915_gem_object *obj,
> >>
> >>   intel_mark_fb_busy(dev, obj->frontbuffer_bits, ring);
> >>
> >>- intel_psr_invalidate(dev, obj->frontbuffer_bits);
> >>+
> >>+ if (INTEL_INFO(dev)->gen < 9)
> >>+ intel_psr_invalidate(dev, obj->frontbuffer_bits);
> >>  }
> >>
> >>  /**
> >>@@ -182,7 +184,8 @@ void intel_frontbuffer_flush(struct drm_device *dev,
> >>
> >>   intel_mark_fb_busy(dev, frontbuffer_bits, NULL);
> >>
> >>- intel_psr_flush(dev, frontbuffer_bits);
> >>+ if (INTEL_INFO(dev)->gen < 9)
> >>+ intel_psr_flush(dev, frontbuffer_bits);
> >I'm pretty sure the hw isn't good enough yet to detect everything,
> >unfortunately the testcase is also not quite ready yet. In any case these
> >changes should be moved into the inel_psr_* functions and in a separate
> >patch.
> When ubuntu boots up and stops at the login screen, the text I enter comes
> slowly, looks like psr exit and entry is not very prompt at that time.
> But when I try with SW tracking, I see that PSR doesn't get enabled at all
> at that point!
> Also, I don't see calls to frontbuffer invalidate and flush at that point.
> So, with SW tracking too we will have cases where we don't enter psr.
> Any inputs on why frontbuffer calls are not received during login screen?
> 
> Please note that after login, it works fine if I type something on the
> terminal or do some other
> things.

I have no idea why or what exactly the ubuntu screen does in it's login
function. Just please note that there are _lots_ more desktop environments
on linux than just ubunut's flavour of gnome, so we really need to have
that psr testcase working (which covers them all) instead of manual
testing.
-Daniel

> >Also someone needs to re-review the psr igt testcase to make sure it
> >covers everything. Ville could do that too since he's done the fbc
> >testcase.
> >-Daniel
> >
> >>   /*
> >>   * FIXME: Unconditional fbc flushing here is a rather gross hack and
> >>diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> >>index dd0e6e0..6d2cdb8 100644
> >>--- a/drivers/gpu/drm/i915/intel_psr.c
> >>+++ b/drivers/gpu/drm/i915/intel_psr.c
> >>@@ -173,11 +173,24 @@ static void hsw_psr_enable_sink(struct intel_dp *intel_dp)
> >>   I915_WRITE(EDP_PSR_AUX_DATA1(dev) + i,
> >>     intel_dp_pack_aux(&aux_msg[i], sizeof(aux_msg) - i));
> >>
> >>- I915_WRITE(EDP_PSR_AUX_CTL(dev),
> >>+ if (INTEL_INFO(dev)->gen >= 9) {
> >>+ uint32_t val;
> >>+
> >>+ val = I915_READ(EDP_PSR_AUX_CTL(dev));
> >>+ val &= ~DP_AUX_CH_CTL_TIME_OUT_MASK;
> >>+ val |= DP_AUX_CH_CTL_TIME_OUT_1600us;
> >>+ val &= ~DP_AUX_CH_CTL_MESSAGE_SIZE_MASK;
> >>+ val |= (sizeof(aux_msg) << DP_AUX_CH_CTL_MESSAGE_SIZE_SHIFT);
> >>+ /* Use hardcoded data values for PSR */
> >>+ val &= ~DP_AUX_CH_CTL_PSR_DATA_AUX_REG_SKL;
> >>+ I915_WRITE(EDP_PSR_AUX_CTL(dev), val);
> >>+ } else {
> >>+ I915_WRITE(EDP_PSR_AUX_CTL(dev),
> >>     DP_AUX_CH_CTL_TIME_OUT_400us |
> >>     (sizeof(aux_msg) << DP_AUX_CH_CTL_MESSAGE_SIZE_SHIFT) |
> >>     (precharge << DP_AUX_CH_CTL_PRECHARGE_2US_SHIFT) |
> >>     (aux_clock_divider << DP_AUX_CH_CTL_BIT_CLOCK_2X_SHIFT));
> >>+ }
> >>  }
> >>
> >>  static void vlv_psr_enable_source(struct intel_dp *intel_dp)
> >>@@ -355,6 +368,9 @@ void intel_psr_enable(struct intel_dp *intel_dp)
> >>
> >>   /* Enable PSR on the panel */
> >>   hsw_psr_enable_sink(intel_dp);
> >>+
> >>+ if (INTEL_INFO(dev)->gen >= 9)
> >>+ intel_psr_activate(intel_dp);
> >>   } else {
> >>   vlv_psr_setup_vsc(intel_dp);
> >>
> >>--
> >>1.7.10.4
> >>
> >>_______________________________________________
> >>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