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

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, Jan 29, 2015 at 09:27:14AM +0530, sonika wrote:
> 
> On Wednesday 28 January 2015 09:32 PM, Daniel Vetter wrote:
> >On Thu, Jan 22, 2015 at 02:30:54PM +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.
> >>v4: Using SKL DDI AUX regs instead of changing PSR_AUX regs definition (Rodrigo)
> >>
> >>Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>
> >>Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> >>---
> >>  drivers/gpu/drm/i915/i915_drv.h          |    3 ++-
> >>  drivers/gpu/drm/i915/i915_reg.h          |    5 +++++
> >>  drivers/gpu/drm/i915/intel_frontbuffer.c |    7 +++++--
> >>  drivers/gpu/drm/i915/intel_psr.c         |   26 ++++++++++++++++++++++++--
> >>  4 files changed, 36 insertions(+), 5 deletions(-)
> >>
> >>diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> >>index 66f0c60..3d24872 100644
> >>--- a/drivers/gpu/drm/i915/i915_drv.h
> >>+++ b/drivers/gpu/drm/i915/i915_drv.h
> >>@@ -2371,7 +2371,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 a39bb03..a6f06fa 100644
> >>--- a/drivers/gpu/drm/i915/i915_reg.h
> >>+++ b/drivers/gpu/drm/i915/i915_reg.h
> >>@@ -3748,6 +3748,11 @@ enum punit_power_well {
> >>  #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);
> >Again no, not going to take wholesale filtering of the sw invalidate
> >paths. This needs to be properly tested and pushed down into the psr
> >specific invalidate/flush functions on a per-function basis.
> >
> >I've dropped these two hunks and merged the patch.
> >-Daniel
> Hi Daniel,
> Even SW tracking doesn't work in many cases, like I reported earlier in
> ubuntu login screen where we don't get frontbuffer flushes and we don't
> enter PSR at all with SW tracking.
> I see similar behavior even in fbcon mode. So, I am not sure how you can say
> that SW tracking is the only right way.

In some cases the sw tracking isn't especially accurate (cpu based
frontbuffer rendering to the gtt). Paulo has seen a similar issue with
fbc, and since hw tracking works correctly for that case his patches
filter that source of sw invalidates out. Paulo should be back from his
vacation next week, so please ping him when he's back.

> If there are cases where HW tracking fails (and I know a few), we need to
> fix them. I can move this gen check to the intel_psr_* function if that is
> the major concern.

Yeah, the check should be pushed down imo, in the core sw tracking code
it's a bit in the wrong layer.
-Daniel
-- 
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