Is somebody working on enhancing the psr testcase?
-Sonika
-----Original Message-----
From: Daniel Vetter [mailto:daniel.vetter at ffwll.ch] On Behalf Of Daniel Vetter
Sent: Tuesday, January 20, 2015 3:20 PM
To: Jindal, Sonika
Cc: Daniel Vetter; intel-gfx; Vivi, Rodrigo
Subject: Re: [Intel-gfx] [PATCH] drm/i915/skl: Enabling PSR on Skylake
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 at 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 at 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