| Message ID | 1390999674-7982-1-git-send-email-rodrigo.vivi@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Jan 29, 2014 at 10:47:54AM -0200, Rodrigo Vivi wrote: > This patch adds PSR Support to Baytrail. > > Baytrail cannot easily detect screen updates and force PSR exit. > So we inactivate it on {busy_ioctl, set_domain, sw_finish and mark_busy > and update to enable it back on next display mark_idle. > > v2: Also inactivate PSR on cursor update. > v3: Inactivate PSR on mark_busy, dset_domain and sw_finish_ioctl, and > early on page flip besides avoid initializing inactive/active flag > more than once. > v4: Fix identation issues. > v5: Rebase and add Baytrail per pipe support although leaving PIPE_B > support disabled by for now since it isn't working properly yet. > v6: Removing forgotten comment and useless clkgating definition. Not set-domain. This is semantically a flush and so should be after the damage is done. -Chris
On Wed, Jan 29, 2014 at 11:12 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote: > On Wed, Jan 29, 2014 at 10:47:54AM -0200, Rodrigo Vivi wrote: >> This patch adds PSR Support to Baytrail. >> >> Baytrail cannot easily detect screen updates and force PSR exit. >> So we inactivate it on {busy_ioctl, set_domain, sw_finish and mark_busy >> and update to enable it back on next display mark_idle. >> >> v2: Also inactivate PSR on cursor update. >> v3: Inactivate PSR on mark_busy, dset_domain and sw_finish_ioctl, and >> early on page flip besides avoid initializing inactive/active flag >> more than once. >> v4: Fix identation issues. >> v5: Rebase and add Baytrail per pipe support although leaving PIPE_B >> support disabled by for now since it isn't working properly yet. >> v6: Removing forgotten comment and useless clkgating definition. > > Not set-domain. This is semantically a flush and so should be after the > damage is done. Yep, I semantically I agree, but if we let to inactivate psr after damage is done we will miss screen updates. This was the safest way to get psr enabled and fully working and passing crc tests. If you have another place to suggest i'd be glad in do some tests here, but for now this is the more stable place I know about. > -Chris > > -- > Chris Wilson, Intel Open Source Technology Centre
On Wed, Jan 29, 2014 at 11:24:44AM -0200, Rodrigo Vivi wrote: > On Wed, Jan 29, 2014 at 11:12 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote: > > On Wed, Jan 29, 2014 at 10:47:54AM -0200, Rodrigo Vivi wrote: > >> This patch adds PSR Support to Baytrail. > >> > >> Baytrail cannot easily detect screen updates and force PSR exit. > >> So we inactivate it on {busy_ioctl, set_domain, sw_finish and mark_busy > >> and update to enable it back on next display mark_idle. > >> > >> v2: Also inactivate PSR on cursor update. > >> v3: Inactivate PSR on mark_busy, dset_domain and sw_finish_ioctl, and > >> early on page flip besides avoid initializing inactive/active flag > >> more than once. > >> v4: Fix identation issues. > >> v5: Rebase and add Baytrail per pipe support although leaving PIPE_B > >> support disabled by for now since it isn't working properly yet. > >> v6: Removing forgotten comment and useless clkgating definition. > > > > Not set-domain. This is semantically a flush and so should be after the > > damage is done. > > Yep, I semantically I agree, but if we let to inactivate psr after > damage is done we will miss screen updates. > This was the safest way to get psr enabled and fully working and > passing crc tests. > If you have another place to suggest i'd be glad in do some tests > here, but for now this is the more stable place I know about. It's the test that are at fault here for not following the established ABI imo. -Chris
On Wed, Jan 29, 2014 at 11:27 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote: > On Wed, Jan 29, 2014 at 11:24:44AM -0200, Rodrigo Vivi wrote: >> On Wed, Jan 29, 2014 at 11:12 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote: >> > On Wed, Jan 29, 2014 at 10:47:54AM -0200, Rodrigo Vivi wrote: >> >> This patch adds PSR Support to Baytrail. >> >> >> >> Baytrail cannot easily detect screen updates and force PSR exit. >> >> So we inactivate it on {busy_ioctl, set_domain, sw_finish and mark_busy >> >> and update to enable it back on next display mark_idle. >> >> >> >> v2: Also inactivate PSR on cursor update. >> >> v3: Inactivate PSR on mark_busy, dset_domain and sw_finish_ioctl, and >> >> early on page flip besides avoid initializing inactive/active flag >> >> more than once. >> >> v4: Fix identation issues. >> >> v5: Rebase and add Baytrail per pipe support although leaving PIPE_B >> >> support disabled by for now since it isn't working properly yet. >> >> v6: Removing forgotten comment and useless clkgating definition. >> > >> > Not set-domain. This is semantically a flush and so should be after the >> > damage is done. >> >> Yep, I semantically I agree, but if we let to inactivate psr after >> damage is done we will miss screen updates. >> This was the safest way to get psr enabled and fully working and >> passing crc tests. >> If you have another place to suggest i'd be glad in do some tests >> here, but for now this is the more stable place I know about. > > It's the test that are at fault here for not following the established > ABI imo. this makes sense. do we have this established ABI documented somewhere? and what is missing on test? is it a busy ioctl in the end? but anyway, doing this on set_domain we could fix the psr on environments that doesn't follow this abi like KDE. > -Chris > > -- > Chris Wilson, Intel Open Source Technology Centre
On Wed, Jan 29, 2014 at 11:54:00AM -0200, Rodrigo Vivi wrote: > On Wed, Jan 29, 2014 at 11:27 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote: > > On Wed, Jan 29, 2014 at 11:24:44AM -0200, Rodrigo Vivi wrote: > >> On Wed, Jan 29, 2014 at 11:12 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote: > >> > On Wed, Jan 29, 2014 at 10:47:54AM -0200, Rodrigo Vivi wrote: > >> >> This patch adds PSR Support to Baytrail. > >> >> > >> >> Baytrail cannot easily detect screen updates and force PSR exit. > >> >> So we inactivate it on {busy_ioctl, set_domain, sw_finish and mark_busy > >> >> and update to enable it back on next display mark_idle. > >> >> > >> >> v2: Also inactivate PSR on cursor update. > >> >> v3: Inactivate PSR on mark_busy, dset_domain and sw_finish_ioctl, and > >> >> early on page flip besides avoid initializing inactive/active flag > >> >> more than once. > >> >> v4: Fix identation issues. > >> >> v5: Rebase and add Baytrail per pipe support although leaving PIPE_B > >> >> support disabled by for now since it isn't working properly yet. > >> >> v6: Removing forgotten comment and useless clkgating definition. > >> > > >> > Not set-domain. This is semantically a flush and so should be after the > >> > damage is done. > >> > >> Yep, I semantically I agree, but if we let to inactivate psr after > >> damage is done we will miss screen updates. > >> This was the safest way to get psr enabled and fully working and > >> passing crc tests. > >> If you have another place to suggest i'd be glad in do some tests > >> here, but for now this is the more stable place I know about. > > > > It's the test that are at fault here for not following the established > > ABI imo. > > this makes sense. > > do we have this established ABI documented somewhere? Only what was established as required to make things work a few years ago... > and what is missing on test? is it a busy ioctl in the end? busy or even a sw_finish. I see it already did the sw_finish. We have in the past talked about formalizing the gap in the documentation with a new call... > but anyway, doing this on set_domain we could fix the psr on > environments that doesn't follow this abi like KDE. As we have discussed in the past, that is an issue in the ddx that short-circuits the required flush if there was only GTT damage. -Chris
ok, got it. So, the correct here is to remove inactivate from set_domain and add gem_bo_busy call on MMAP_GTT testcase? On Wed, Jan 29, 2014 at 12:02 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote: > On Wed, Jan 29, 2014 at 11:54:00AM -0200, Rodrigo Vivi wrote: >> On Wed, Jan 29, 2014 at 11:27 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote: >> > On Wed, Jan 29, 2014 at 11:24:44AM -0200, Rodrigo Vivi wrote: >> >> On Wed, Jan 29, 2014 at 11:12 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote: >> >> > On Wed, Jan 29, 2014 at 10:47:54AM -0200, Rodrigo Vivi wrote: >> >> >> This patch adds PSR Support to Baytrail. >> >> >> >> >> >> Baytrail cannot easily detect screen updates and force PSR exit. >> >> >> So we inactivate it on {busy_ioctl, set_domain, sw_finish and mark_busy >> >> >> and update to enable it back on next display mark_idle. >> >> >> >> >> >> v2: Also inactivate PSR on cursor update. >> >> >> v3: Inactivate PSR on mark_busy, dset_domain and sw_finish_ioctl, and >> >> >> early on page flip besides avoid initializing inactive/active flag >> >> >> more than once. >> >> >> v4: Fix identation issues. >> >> >> v5: Rebase and add Baytrail per pipe support although leaving PIPE_B >> >> >> support disabled by for now since it isn't working properly yet. >> >> >> v6: Removing forgotten comment and useless clkgating definition. >> >> > >> >> > Not set-domain. This is semantically a flush and so should be after the >> >> > damage is done. >> >> >> >> Yep, I semantically I agree, but if we let to inactivate psr after >> >> damage is done we will miss screen updates. >> >> This was the safest way to get psr enabled and fully working and >> >> passing crc tests. >> >> If you have another place to suggest i'd be glad in do some tests >> >> here, but for now this is the more stable place I know about. >> > >> > It's the test that are at fault here for not following the established >> > ABI imo. >> >> this makes sense. >> >> do we have this established ABI documented somewhere? > > Only what was established as required to make things work a few years > ago... > >> and what is missing on test? is it a busy ioctl in the end? > > busy or even a sw_finish. I see it already did the sw_finish. We have in > the past talked about formalizing the gap in the documentation with a > new call... > >> but anyway, doing this on set_domain we could fix the psr on >> environments that doesn't follow this abi like KDE. > > As we have discussed in the past, that is an issue in the ddx that > short-circuits the required flush if there was only GTT damage. > -Chris > > -- > Chris Wilson, Intel Open Source Technology Centre
On Wed, Jan 29, 2014 at 12:23:15PM -0200, Rodrigo Vivi wrote: > ok, got it. > > So, the correct here is to remove inactivate from set_domain and add > gem_bo_busy call on MMAP_GTT testcase? That would match what we should do today. It's just whether we take the opportunity to define a consistent behaviour... -Chris
On Wed, Jan 29, 2014 at 10:47:54AM -0200, Rodrigo Vivi wrote: > This patch adds PSR Support to Baytrail. > > Baytrail cannot easily detect screen updates and force PSR exit. > So we inactivate it on {busy_ioctl, set_domain, sw_finish and mark_busy > and update to enable it back on next display mark_idle. > > v2: Also inactivate PSR on cursor update. > v3: Inactivate PSR on mark_busy, dset_domain and sw_finish_ioctl, and > early on page flip besides avoid initializing inactive/active flag > more than once. > v4: Fix identation issues. > v5: Rebase and add Baytrail per pipe support although leaving PIPE_B > support disabled by for now since it isn't working properly yet. > v6: Removing forgotten comment and useless clkgating definition. > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com> > --- > drivers/gpu/drm/i915/i915_debugfs.c | 36 ++++++- > drivers/gpu/drm/i915/i915_drv.h | 4 +- > drivers/gpu/drm/i915/i915_gem.c | 9 ++ > drivers/gpu/drm/i915/i915_reg.h | 37 +++++++ > drivers/gpu/drm/i915/intel_display.c | 15 ++- > drivers/gpu/drm/i915/intel_dp.c | 204 +++++++++++++++++++++++++++++------ > drivers/gpu/drm/i915/intel_drv.h | 1 + > 7 files changed, 267 insertions(+), 39 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c > index 4b852c6..c28de65 100644 > --- a/drivers/gpu/drm/i915/i915_debugfs.c > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > @@ -1900,6 +1900,8 @@ static int i915_edp_psr_status(struct seq_file *m, void *data) > struct drm_device *dev = node->minor->dev; > struct drm_i915_private *dev_priv = dev->dev_private; > u32 psrperf = 0; > + u32 statA = 0; > + u32 statB = 0; > bool enabled = false; > > intel_runtime_pm_get(dev_priv); > @@ -1907,14 +1909,38 @@ static int i915_edp_psr_status(struct seq_file *m, void *data) > seq_printf(m, "Sink_Support: %s\n", yesno(dev_priv->psr.sink_support)); > seq_printf(m, "Source_OK: %s\n", yesno(dev_priv->psr.source_ok)); > > - enabled = HAS_PSR(dev) && > - I915_READ(EDP_PSR_CTL(dev)) & EDP_PSR_ENABLE; > - seq_printf(m, "Enabled: %s\n", yesno(enabled)); > + if (HAS_PSR(dev)) { > + if (IS_VALLEYVIEW(dev)) { > + statA = I915_READ(VLV_EDP_PSR_STATUS_CTL(PIPE_A)) & > + VLV_EDP_PSR_CURR_STATE_MASK; > + statB = I915_READ(VLV_EDP_PSR_STATUS_CTL(PIPE_B)) & > + VLV_EDP_PSR_CURR_STATE_MASK; > + enabled = ((statA == VLV_EDP_PSR_ACTIVE_NORFB_UP) || > + (statA == VLV_EDP_PSR_ACTIVE_SF_UPDATE) || > + (statB == VLV_EDP_PSR_ACTIVE_NORFB_UP) || > + (statB == VLV_EDP_PSR_ACTIVE_SF_UPDATE)); > + } else > + enabled = I915_READ(EDP_PSR_CTL(dev)) & EDP_PSR_ENABLE; > + } > + seq_printf(m, "Enabled: %s", yesno(enabled)); > > - if (HAS_PSR(dev)) > + if (IS_VALLEYVIEW(dev)) { > + if ((statA == VLV_EDP_PSR_ACTIVE_NORFB_UP) || > + (statA == VLV_EDP_PSR_ACTIVE_SF_UPDATE)) > + seq_puts(m, " pipe A"); > + if ((statB == VLV_EDP_PSR_ACTIVE_NORFB_UP) || > + (statB == VLV_EDP_PSR_ACTIVE_SF_UPDATE)) > + seq_puts(m, " pipe B"); > + } > + > + seq_puts(m, "\n"); > + > + /* VLV PSR has no kind of performance counter */ > + if (HAS_PSR(dev) && !IS_VALLEYVIEW(dev)) { > psrperf = I915_READ(EDP_PSR_PERF_CNT(dev)) & > EDP_PSR_PERF_CNT_MASK; > - seq_printf(m, "Performance_Counter: %u\n", psrperf); > + seq_printf(m, "Performance_Counter: %u\n", psrperf); > + } > > intel_runtime_pm_put(dev_priv); > return 0; > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 7c53d4d..34dee24 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -747,6 +747,7 @@ struct i915_psr { > bool sink_support; > bool source_ok; > bool setup_done; > + bool active; > }; > > enum intel_pch { > @@ -1866,7 +1867,8 @@ struct drm_i915_file_private { > > #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)) > +#define HAS_PSR(dev) (IS_HASWELL(dev) || IS_BROADWELL(dev) || \ > + IS_VALLEYVIEW(dev)) > #define HAS_PC8(dev) (IS_HASWELL(dev)) /* XXX HSW:ULX */ > #define HAS_RUNTIME_PM(dev) (IS_HASWELL(dev)) > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index 39770f7..01137fe 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -1256,6 +1256,9 @@ i915_gem_set_domain_ioctl(struct drm_device *dev, void *data, > goto unlock; > } > > + if (IS_VALLEYVIEW(dev)) > + intel_edp_psr_inactivate(dev); > + > /* 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. > @@ -1299,6 +1302,9 @@ i915_gem_sw_finish_ioctl(struct drm_device *dev, void *data, > if (ret) > return ret; > > + if (IS_VALLEYVIEW(dev)) > + intel_edp_psr_inactivate(dev); > + > obj = to_intel_bo(drm_gem_object_lookup(dev, file, args->handle)); > if (&obj->base == NULL) { > ret = -ENOENT; > @@ -4059,6 +4065,9 @@ i915_gem_busy_ioctl(struct drm_device *dev, void *data, > if (ret) > return ret; > > + if (IS_VALLEYVIEW(dev)) > + intel_edp_psr_inactivate(dev); The locking for PSR seems to be as fubar as for FBC. Also the front buffer tracking is missing, but I guess I need to make that work for FBC first, and then we need to figure out how to tie it in w/ PSR. > + > obj = to_intel_bo(drm_gem_object_lookup(dev, file, args->handle)); > if (&obj->base == NULL) { > ret = -ENOENT; > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index cbbaf26..2039d71 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -1968,6 +1968,43 @@ > #define BCLRPAT(pipe) _PIPE(pipe, _BCLRPAT_A, _BCLRPAT_B) > #define VSYNCSHIFT(trans) _TRANSCODER(trans, _VSYNCSHIFT_A, _VSYNCSHIFT_B) > > +/* VLV eDP PSR registers */ > +#define _PSRCTLA (VLV_DISPLAY_BASE + 0x60090) > +#define _PSRCTLB (VLV_DISPLAY_BASE + 0x61090) > +#define VLV_EDP_PSR_ENABLE (1<<0) > +#define VLV_EDP_PSR_RESET (1<<1) > +#define VLV_EDP_PSR_MODE_MASK (7<<2) > +#define VLV_EDP_PSR_MODE_HW_TIMER (1<<3) > +#define VLV_EDP_PSR_MODE_SW_TIMER (1<<2) > +#define VLV_EDP_PSR_SINGLE_FRAME_UPDATE (1<<7) > +#define VLV_EDP_PSR_ACTIVE_ENTRY (1<<8) > +#define VLV_EDP_PSR_SRC_TRANSMITTER_STATE (1<<9) > +#define VLV_EDP_PSR_DBL_FRAME (1<<10) > +#define VLV_EDP_PSR_FRAME_COUNT_MASK (0xff<<16) > +#define VLV_EDP_PSR_IDLE_FRAME_SHIFT 16 > +#define VLV_EDP_PSR_INT_TRANSITION (1<<24) > +#define VLV_EDP_PSR_CTL(pipe) _PIPE(pipe, _PSRCTLA, _PSRCTLB) Can we just name the PSR registers like in the spec? Eg. just VLV_PSRCTL(). Would make it easier to compare things w/ the spec. > + > +#define _VSCSDPA (VLV_DISPLAY_BASE + 0x600a0) > +#define _VSCSDPB (VLV_DISPLAY_BASE + 0x610a0) > +#define VLV_EDP_PSR_SDP_FREQ_MASK (3<<30) > +#define VLV_EDP_PSR_SDP_FREQ_ONCE (1<<31) > +#define VLV_EDP_PSR_SDP_FREQ_EVFRAME (1<<30) > +#define VLV_EDP_VSC_SDP_REG(pipe) _PIPE(pipe, _VSCSDPA, _VSCSDPB) > + > +#define _PSRSTATA (VLV_DISPLAY_BASE + 0x60094) > +#define _PSRSTATB (VLV_DISPLAY_BASE + 0x61094) > +#define VLV_EDP_PSR_LAST_STATE_MASK (7<<3) > +#define VLV_EDP_PSR_CURR_STATE_MASK 7 > +#define VLV_EDP_PSR_DISABLED (0<<0) > +#define VLV_EDP_PSR_INACTIVE (1<<0) > +#define VLV_EDP_PSR_IN_TRANS_TO_ACTIVE (2<<0) > +#define VLV_EDP_PSR_ACTIVE_NORFB_UP (3<<0) > +#define VLV_EDP_PSR_ACTIVE_SF_UPDATE (4<<0) > +#define VLV_EDP_PSR_EXIT (5<<0) > +#define VLV_EDP_PSR_IN_TRANS (1<<7) > +#define VLV_EDP_PSR_STATUS_CTL(pipe) _PIPE(pipe, _PSRSTATA, _PSRSTATB) > + > /* HSW+ eDP PSR registers */ > #define EDP_PSR_BASE(dev) (IS_HASWELL(dev) ? 0x64800 : 0x6f800) > #define EDP_PSR_CTL(dev) (EDP_PSR_BASE(dev) + 0) > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 1a9aa19..081c8e2 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -7501,6 +7501,9 @@ static void intel_crtc_update_cursor(struct drm_crtc *crtc, > u32 base = 0, pos = 0; > bool visible; > > + if (IS_VALLEYVIEW(dev)) > + intel_edp_psr_inactivate(dev); > + > if (on) > base = intel_crtc->cursor_addr; > > @@ -8228,16 +8231,20 @@ void intel_mark_idle(struct drm_device *dev) > > if (dev_priv->info->gen >= 6) > gen6_rps_idle(dev->dev_private); > + > + if (IS_VALLEYVIEW(dev)) > + intel_edp_psr_update(dev); > } > > + > void intel_mark_fb_busy(struct drm_i915_gem_object *obj, > struct intel_ring_buffer *ring) > { > struct drm_device *dev = obj->base.dev; > struct drm_crtc *crtc; > > - if (!i915.powersave) > - return; > + if (IS_VALLEYVIEW(dev)) > + intel_edp_psr_inactivate(dev); > > list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) { > if (!crtc->fb) > @@ -8688,6 +8695,10 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc, nav> if (work == NULL) > return -ENOMEM; > > + /* Inactivate PSR early in page flip */ > + if (IS_VALLEYVIEW(dev)) > + intel_edp_psr_inactivate(dev); > + > work->event = event; > work->crtc = crtc; > work->old_fb_obj = to_intel_framebuffer(old_fb)->obj; > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 30d4350..e9a0ace 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -1578,21 +1578,35 @@ static void intel_dp_get_config(struct intel_encoder *encoder, > } > } > > -static bool is_edp_psr(struct drm_device *dev) > +static bool is_edp_psr(struct intel_dp *intel_dp) > +{ > + return intel_dp->psr_dpcd[0] & DP_PSR_IS_SUPPORTED; > +} > + > +static bool vlv_edp_is_psr_enabled_on_pipe(struct drm_device *dev, int pipe) > { > struct drm_i915_private *dev_priv = dev->dev_private; > + uint32_t val; > > - return dev_priv->psr.sink_support; > + val = I915_READ(VLV_EDP_PSR_STATUS_CTL(pipe)) & > + VLV_EDP_PSR_CURR_STATE_MASK; > + return (val == VLV_EDP_PSR_ACTIVE_NORFB_UP) || > + (val == VLV_EDP_PSR_ACTIVE_SF_UPDATE); > } > > static bool intel_edp_is_psr_enabled(struct drm_device *dev) > { > struct drm_i915_private *dev_priv = dev->dev_private; > > - if (!HAS_PSR(dev)) > - return false; > + if (HAS_PSR(dev)) { > + if (IS_VALLEYVIEW(dev)) > + return vlv_edp_is_psr_enabled_on_pipe(dev, PIPE_A) || > + vlv_edp_is_psr_enabled_on_pipe(dev, PIPE_B); > + else > + return I915_READ(EDP_PSR_CTL(dev)) & EDP_PSR_ENABLE; > + } > > - return I915_READ(EDP_PSR_CTL(dev)) & EDP_PSR_ENABLE; > + return false; > } > > static void intel_edp_psr_write_vsc(struct intel_dp *intel_dp, > @@ -1626,28 +1640,49 @@ static void intel_edp_psr_write_vsc(struct intel_dp *intel_dp, > > static void intel_edp_psr_setup(struct intel_dp *intel_dp) > { > - struct drm_device *dev = intel_dp_to_dev(intel_dp); > + struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); > + struct drm_device *dev = intel_dig_port->base.base.dev; > struct drm_i915_private *dev_priv = dev->dev_private; > struct edp_vsc_psr psr_vsc; > + uint32_t val; > > if (dev_priv->psr.setup_done) > return; > > - /* Prepare VSC packet as per EDP 1.3 spec, Table 3.10 */ > - memset(&psr_vsc, 0, sizeof(psr_vsc)); > - psr_vsc.sdp_header.HB0 = 0; > - psr_vsc.sdp_header.HB1 = 0x7; > - psr_vsc.sdp_header.HB2 = 0x2; > - psr_vsc.sdp_header.HB3 = 0x8; > - intel_edp_psr_write_vsc(intel_dp, &psr_vsc); > + if (IS_VALLEYVIEW(dev)) { > + val = I915_READ(VLV_EDP_VSC_SDP_REG(PIPE_A)); > + val &= ~VLV_EDP_PSR_SDP_FREQ_MASK; > + val |= VLV_EDP_PSR_SDP_FREQ_EVFRAME; > + I915_WRITE(VLV_EDP_VSC_SDP_REG(PIPE_A), val); > + > + val = I915_READ(VLV_EDP_VSC_SDP_REG(PIPE_B)); > + val &= ~VLV_EDP_PSR_SDP_FREQ_MASK; > + val |= VLV_EDP_PSR_SDP_FREQ_EVFRAME; > + I915_WRITE(VLV_EDP_VSC_SDP_REG(PIPE_B), val); > + } else { > + /* Prepare VSC packet as per EDP 1.3 spec, Table 3.10 */ > + memset(&psr_vsc, 0, sizeof(psr_vsc)); > + psr_vsc.sdp_header.HB0 = 0; > + psr_vsc.sdp_header.HB1 = 0x7; > + psr_vsc.sdp_header.HB2 = 0x2; > + psr_vsc.sdp_header.HB3 = 0x8; > + intel_edp_psr_write_vsc(intel_dp, &psr_vsc); > > - /* Avoid continuous PSR exit by masking memup and hpd */ > - I915_WRITE(EDP_PSR_DEBUG_CTL(dev), EDP_PSR_DEBUG_MASK_MEMUP | > - EDP_PSR_DEBUG_MASK_HPD | EDP_PSR_DEBUG_MASK_LPSP); > + /* Avoid continuous PSR exit by masking memup and hpd */ > + I915_WRITE(EDP_PSR_DEBUG_CTL(dev), EDP_PSR_DEBUG_MASK_MEMUP | > + EDP_PSR_DEBUG_MASK_HPD | EDP_PSR_DEBUG_MASK_LPSP); > + } > > dev_priv->psr.setup_done = true; > } > > +static void vlv_edp_psr_enable_sink(struct intel_dp *intel_dp) > +{ > + /* Enable PSR in sink */ > + intel_dp_aux_native_write_1(intel_dp, DP_PSR_EN_CFG, > + DP_PSR_ENABLE); Don't we want the same main-link poweroff logic as HSW? Or maybe we should just keep the main link on always as long as we can't enter the low power states w/ DPLL/pipe/port off. Did you already figure out why that's not happening? Looking at the PSRSTAT registers, my guess is that D0i1 is where we end up currently, and that doesn't actually turn off anything but the planes (to stop memory fetches). D0i2 would turn off everything. But I guess we should anyway do this in steps. First getting the current stuff in, then trying to get into D0i2 state, and finally getting the display power well turned off when in PSR. I think once we get to working on D0i2, we'll need to move the PSR wakeup to happen from a workqueue since it essentially requires a full modeset. Even now in your code it's somewhat questionable since you're doing stuff like aux transfers while holding struct_mutex. > +} > + > static void intel_edp_psr_enable_sink(struct intel_dp *intel_dp) > { > struct drm_device *dev = intel_dp_to_dev(intel_dp); > @@ -1678,6 +1713,28 @@ static void intel_edp_psr_enable_sink(struct intel_dp *intel_dp) > (aux_clock_divider << DP_AUX_CH_CTL_BIT_CLOCK_2X_SHIFT)); > } > > +static void vlv_edp_psr_enable_source(struct intel_dp *intel_dp) > +{ > + struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); > + struct drm_device *dev = intel_dig_port->base.base.dev; > + struct drm_i915_private *dev_priv = dev->dev_private; > + struct intel_crtc *intel_crtc = > + to_intel_crtc(intel_dig_port->base.base.crtc); > + > + uint32_t idle_frames = 1; > + uint32_t val; > + > + val = I915_READ(VLV_EDP_PSR_CTL(intel_crtc->pipe)); I don't think we want to preserve anything here. We need to make sure everything is initialized correctly rather than trusting some old junk in the register. > + val |= VLV_EDP_PSR_ENABLE; > + val &= ~VLV_EDP_PSR_MODE_MASK; > + > + val |= VLV_EDP_PSR_MODE_HW_TIMER; > + val &= ~VLV_EDP_PSR_FRAME_COUNT_MASK; > + val |= idle_frames << EDP_PSR_IDLE_FRAME_SHIFT; > + > + I915_WRITE(VLV_EDP_PSR_CTL(intel_crtc->pipe), val); > +} > + > static void intel_edp_psr_enable_source(struct intel_dp *intel_dp) > { > struct drm_device *dev = intel_dp_to_dev(intel_dp); > @@ -1719,8 +1776,8 @@ static bool intel_edp_psr_match_conditions(struct intel_dp *intel_dp) > return false; > } > > - if ((intel_encoder->type != INTEL_OUTPUT_EDP) || > - (dig_port->port != PORT_A)) { > + if (HAS_DDI(dev) && ((intel_encoder->type != INTEL_OUTPUT_EDP) || > + (dig_port->port != PORT_A))) { > DRM_DEBUG_KMS("HSW ties PSR to DDI A (eDP)\n"); > return false; > } > @@ -1765,37 +1822,83 @@ static bool intel_edp_psr_match_conditions(struct intel_dp *intel_dp) > return false; > } > > + /* Baytrail supports per-pipe PSR configuration, however PSR on > + * PIPE_B isn't working properly. So let's keep it disabled for now. */ > + if (IS_VALLEYVIEW(dev) && intel_crtc->pipe != PIPE_A) { > + DRM_DEBUG_KMS("PSR on BYT isn't enabled on pipe B.\n"); > + return false; > + } > + > dev_priv->psr.source_ok = true; > return true; > } > > static void intel_edp_psr_do_enable(struct intel_dp *intel_dp) > { > - struct drm_device *dev = intel_dp_to_dev(intel_dp); > + struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); > + struct drm_device *dev = intel_dig_port->base.base.dev; > + struct drm_i915_private *dev_priv = dev->dev_private; > + struct intel_crtc *intel_crtc = > + to_intel_crtc(intel_dig_port->base.base.crtc); > > - if (!intel_edp_psr_match_conditions(intel_dp) || > - intel_edp_is_psr_enabled(dev)) > - return; > + if (IS_VALLEYVIEW(dev)) { > + if (vlv_edp_is_psr_enabled_on_pipe(dev, intel_crtc->pipe)) > + return; > + } else > + if (intel_edp_is_psr_enabled(dev)) > + return; > > /* Setup PSR once */ > intel_edp_psr_setup(intel_dp); > > /* Enable PSR on the panel */ > - intel_edp_psr_enable_sink(intel_dp); > + if (IS_VALLEYVIEW(dev)) > + vlv_edp_psr_enable_sink(intel_dp); > + else > + intel_edp_psr_enable_sink(intel_dp); > > /* Enable PSR on the host */ > - intel_edp_psr_enable_source(intel_dp); > + if (IS_VALLEYVIEW(dev)) > + vlv_edp_psr_enable_source(intel_dp); > + else > + intel_edp_psr_enable_source(intel_dp); > + > + dev_priv->psr.active = true; > } > > void intel_edp_psr_enable(struct intel_dp *intel_dp) > { > - struct drm_device *dev = intel_dp_to_dev(intel_dp); > + if (!is_edp_psr(intel_dp)) > + return; > > - if (intel_edp_psr_match_conditions(intel_dp) && > - !intel_edp_is_psr_enabled(dev)) > + if (intel_edp_psr_match_conditions(intel_dp)) > intel_edp_psr_do_enable(intel_dp); > } > > +void vlv_edp_psr_disable(struct intel_dp *intel_dp) > +{ > + struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); > + struct drm_device *dev = intel_dig_port->base.base.dev; > + struct drm_i915_private *dev_priv = dev->dev_private; > + struct intel_crtc *intel_crtc = > + to_intel_crtc(intel_dig_port->base.base.crtc); > + uint32_t val = I915_READ(VLV_EDP_PSR_STATUS_CTL(intel_crtc->pipe)); > + > + if (!dev_priv->psr.setup_done) > + return; > + > + intel_edp_psr_inactivate(dev); > + > + if (val & VLV_EDP_PSR_IN_TRANS) > + udelay(250); Might we want a warning if the bit doesn't go down in expected time? > + > + val = I915_READ(VLV_EDP_PSR_CTL(intel_crtc->pipe)); > + val &= ~VLV_EDP_PSR_ACTIVE_ENTRY; > + val &= ~VLV_EDP_PSR_ENABLE; > + val &= ~VLV_EDP_PSR_MODE_MASK; > + I915_WRITE(VLV_EDP_PSR_CTL(intel_crtc->pipe), val); > +} > + > void intel_edp_psr_disable(struct intel_dp *intel_dp) > { > struct drm_device *dev = intel_dp_to_dev(intel_dp); > @@ -1817,28 +1920,66 @@ void intel_edp_psr_update(struct drm_device *dev) > { > struct intel_encoder *encoder; > struct intel_dp *intel_dp = NULL; > + struct intel_crtc *intel_crtc; > > list_for_each_entry(encoder, &dev->mode_config.encoder_list, base.head) > if (encoder->type == INTEL_OUTPUT_EDP) { > intel_dp = enc_to_intel_dp(&encoder->base); > > - if (!is_edp_psr(dev)) > + if (!is_edp_psr(intel_dp)) > return; > > - if (!intel_edp_psr_match_conditions(intel_dp)) > - intel_edp_psr_disable(intel_dp); > - else > + intel_crtc = to_intel_crtc(encoder->base.crtc); > + > + if (!intel_edp_psr_match_conditions(intel_dp)) { > + if (IS_VALLEYVIEW(dev)) > + vlv_edp_psr_disable(intel_dp); > + else > + intel_edp_psr_disable(intel_dp); > + } else > if (!intel_edp_is_psr_enabled(dev)) > intel_edp_psr_do_enable(intel_dp); > } > } > > +void intel_edp_psr_inactivate(struct drm_device *dev) > +{ > + struct drm_i915_private *dev_priv = dev->dev_private; > + struct intel_encoder *encoder; > + struct intel_crtc *intel_crtc; > + struct intel_dp *intel_dp = NULL; > + > + if (!dev_priv->psr.setup_done || !dev_priv->psr.active) > + return; > + > + list_for_each_entry(encoder, &dev->mode_config.encoder_list, base.head) > + if (encoder->type == INTEL_OUTPUT_EDP) { > + intel_dp = enc_to_intel_dp(&encoder->base); > + intel_crtc = to_intel_crtc(encoder->base.crtc); > + > + if (!vlv_edp_is_psr_enabled_on_pipe(dev, > + intel_crtc->pipe)) > + continue; > + > + dev_priv->psr.active = false; > + I915_WRITE(VLV_EDP_PSR_CTL(intel_crtc->pipe), > + VLV_EDP_PSR_RESET); > + /* WaClearPSRReset:vlv */ > + I915_WRITE(VLV_EDP_PSR_CTL(intel_crtc->pipe), 0); > + > + intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON); > + } > +} > + > static void intel_disable_dp(struct intel_encoder *encoder) > { > struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base); > enum port port = dp_to_dig_port(intel_dp)->port; > struct drm_device *dev = encoder->base.dev; > > + if (IS_VALLEYVIEW(dev)) > + vlv_edp_psr_disable(intel_dp); > + > /* Make sure the panel is off before trying to change the mode. But also > * ensure that we have vdd while we switch off the panel. */ > intel_edp_backlight_off(intel_dp); > @@ -1895,6 +2036,7 @@ static void vlv_enable_dp(struct intel_encoder *encoder) > struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base); > > intel_edp_backlight_on(intel_dp); > + intel_edp_psr_enable(intel_dp); > } > > static void g4x_pre_enable_dp(struct intel_encoder *encoder) > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 71c1371..82026ef 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -748,6 +748,7 @@ void intel_edp_panel_off(struct intel_dp *intel_dp); > void intel_edp_psr_enable(struct intel_dp *intel_dp); > void intel_edp_psr_disable(struct intel_dp *intel_dp); > void intel_edp_psr_update(struct drm_device *dev); > +void intel_edp_psr_inactivate(struct drm_device *dev); > > > /* intel_dsi.c */ > -- > 1.8.1.2
On Wed, Jan 29, 2014 at 12:56 PM, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > On Wed, Jan 29, 2014 at 10:47:54AM -0200, Rodrigo Vivi wrote: >> This patch adds PSR Support to Baytrail. >> >> Baytrail cannot easily detect screen updates and force PSR exit. >> So we inactivate it on {busy_ioctl, set_domain, sw_finish and mark_busy >> and update to enable it back on next display mark_idle. >> >> v2: Also inactivate PSR on cursor update. >> v3: Inactivate PSR on mark_busy, dset_domain and sw_finish_ioctl, and >> early on page flip besides avoid initializing inactive/active flag >> more than once. >> v4: Fix identation issues. >> v5: Rebase and add Baytrail per pipe support although leaving PIPE_B >> support disabled by for now since it isn't working properly yet. >> v6: Removing forgotten comment and useless clkgating definition. >> >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> >> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com> >> --- >> drivers/gpu/drm/i915/i915_debugfs.c | 36 ++++++- >> drivers/gpu/drm/i915/i915_drv.h | 4 +- >> drivers/gpu/drm/i915/i915_gem.c | 9 ++ >> drivers/gpu/drm/i915/i915_reg.h | 37 +++++++ >> drivers/gpu/drm/i915/intel_display.c | 15 ++- >> drivers/gpu/drm/i915/intel_dp.c | 204 +++++++++++++++++++++++++++++------ >> drivers/gpu/drm/i915/intel_drv.h | 1 + >> 7 files changed, 267 insertions(+), 39 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c >> index 4b852c6..c28de65 100644 >> --- a/drivers/gpu/drm/i915/i915_debugfs.c >> +++ b/drivers/gpu/drm/i915/i915_debugfs.c >> @@ -1900,6 +1900,8 @@ static int i915_edp_psr_status(struct seq_file *m, void *data) >> struct drm_device *dev = node->minor->dev; >> struct drm_i915_private *dev_priv = dev->dev_private; >> u32 psrperf = 0; >> + u32 statA = 0; >> + u32 statB = 0; >> bool enabled = false; >> >> intel_runtime_pm_get(dev_priv); >> @@ -1907,14 +1909,38 @@ static int i915_edp_psr_status(struct seq_file *m, void *data) >> seq_printf(m, "Sink_Support: %s\n", yesno(dev_priv->psr.sink_support)); >> seq_printf(m, "Source_OK: %s\n", yesno(dev_priv->psr.source_ok)); >> >> - enabled = HAS_PSR(dev) && >> - I915_READ(EDP_PSR_CTL(dev)) & EDP_PSR_ENABLE; >> - seq_printf(m, "Enabled: %s\n", yesno(enabled)); >> + if (HAS_PSR(dev)) { >> + if (IS_VALLEYVIEW(dev)) { >> + statA = I915_READ(VLV_EDP_PSR_STATUS_CTL(PIPE_A)) & >> + VLV_EDP_PSR_CURR_STATE_MASK; >> + statB = I915_READ(VLV_EDP_PSR_STATUS_CTL(PIPE_B)) & >> + VLV_EDP_PSR_CURR_STATE_MASK; >> + enabled = ((statA == VLV_EDP_PSR_ACTIVE_NORFB_UP) || >> + (statA == VLV_EDP_PSR_ACTIVE_SF_UPDATE) || >> + (statB == VLV_EDP_PSR_ACTIVE_NORFB_UP) || >> + (statB == VLV_EDP_PSR_ACTIVE_SF_UPDATE)); >> + } else >> + enabled = I915_READ(EDP_PSR_CTL(dev)) & EDP_PSR_ENABLE; >> + } >> + seq_printf(m, "Enabled: %s", yesno(enabled)); >> >> - if (HAS_PSR(dev)) >> + if (IS_VALLEYVIEW(dev)) { >> + if ((statA == VLV_EDP_PSR_ACTIVE_NORFB_UP) || >> + (statA == VLV_EDP_PSR_ACTIVE_SF_UPDATE)) >> + seq_puts(m, " pipe A"); >> + if ((statB == VLV_EDP_PSR_ACTIVE_NORFB_UP) || >> + (statB == VLV_EDP_PSR_ACTIVE_SF_UPDATE)) >> + seq_puts(m, " pipe B"); >> + } >> + >> + seq_puts(m, "\n"); >> + >> + /* VLV PSR has no kind of performance counter */ >> + if (HAS_PSR(dev) && !IS_VALLEYVIEW(dev)) { >> psrperf = I915_READ(EDP_PSR_PERF_CNT(dev)) & >> EDP_PSR_PERF_CNT_MASK; >> - seq_printf(m, "Performance_Counter: %u\n", psrperf); >> + seq_printf(m, "Performance_Counter: %u\n", psrperf); >> + } >> >> intel_runtime_pm_put(dev_priv); >> return 0; >> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h >> index 7c53d4d..34dee24 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.h >> +++ b/drivers/gpu/drm/i915/i915_drv.h >> @@ -747,6 +747,7 @@ struct i915_psr { >> bool sink_support; >> bool source_ok; >> bool setup_done; >> + bool active; >> }; >> >> enum intel_pch { >> @@ -1866,7 +1867,8 @@ struct drm_i915_file_private { >> >> #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)) >> +#define HAS_PSR(dev) (IS_HASWELL(dev) || IS_BROADWELL(dev) || \ >> + IS_VALLEYVIEW(dev)) >> #define HAS_PC8(dev) (IS_HASWELL(dev)) /* XXX HSW:ULX */ >> #define HAS_RUNTIME_PM(dev) (IS_HASWELL(dev)) >> >> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c >> index 39770f7..01137fe 100644 >> --- a/drivers/gpu/drm/i915/i915_gem.c >> +++ b/drivers/gpu/drm/i915/i915_gem.c >> @@ -1256,6 +1256,9 @@ i915_gem_set_domain_ioctl(struct drm_device *dev, void *data, >> goto unlock; >> } >> >> + if (IS_VALLEYVIEW(dev)) >> + intel_edp_psr_inactivate(dev); >> + >> /* 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. >> @@ -1299,6 +1302,9 @@ i915_gem_sw_finish_ioctl(struct drm_device *dev, void *data, >> if (ret) >> return ret; >> >> + if (IS_VALLEYVIEW(dev)) >> + intel_edp_psr_inactivate(dev); >> + >> obj = to_intel_bo(drm_gem_object_lookup(dev, file, args->handle)); >> if (&obj->base == NULL) { >> ret = -ENOENT; >> @@ -4059,6 +4065,9 @@ i915_gem_busy_ioctl(struct drm_device *dev, void *data, >> if (ret) >> return ret; >> >> + if (IS_VALLEYVIEW(dev)) >> + intel_edp_psr_inactivate(dev); > > The locking for PSR seems to be as fubar as for FBC. Also the front > buffer tracking is missing, but I guess I need to make that work for FBC > first, and then we need to figure out how to tie it in w/ PSR. > agree... I'll wait your fbc work. >> + >> obj = to_intel_bo(drm_gem_object_lookup(dev, file, args->handle)); >> if (&obj->base == NULL) { >> ret = -ENOENT; >> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h >> index cbbaf26..2039d71 100644 >> --- a/drivers/gpu/drm/i915/i915_reg.h >> +++ b/drivers/gpu/drm/i915/i915_reg.h >> @@ -1968,6 +1968,43 @@ >> #define BCLRPAT(pipe) _PIPE(pipe, _BCLRPAT_A, _BCLRPAT_B) >> #define VSYNCSHIFT(trans) _TRANSCODER(trans, _VSYNCSHIFT_A, _VSYNCSHIFT_B) >> >> +/* VLV eDP PSR registers */ >> +#define _PSRCTLA (VLV_DISPLAY_BASE + 0x60090) >> +#define _PSRCTLB (VLV_DISPLAY_BASE + 0x61090) >> +#define VLV_EDP_PSR_ENABLE (1<<0) >> +#define VLV_EDP_PSR_RESET (1<<1) >> +#define VLV_EDP_PSR_MODE_MASK (7<<2) >> +#define VLV_EDP_PSR_MODE_HW_TIMER (1<<3) >> +#define VLV_EDP_PSR_MODE_SW_TIMER (1<<2) >> +#define VLV_EDP_PSR_SINGLE_FRAME_UPDATE (1<<7) >> +#define VLV_EDP_PSR_ACTIVE_ENTRY (1<<8) >> +#define VLV_EDP_PSR_SRC_TRANSMITTER_STATE (1<<9) >> +#define VLV_EDP_PSR_DBL_FRAME (1<<10) >> +#define VLV_EDP_PSR_FRAME_COUNT_MASK (0xff<<16) >> +#define VLV_EDP_PSR_IDLE_FRAME_SHIFT 16 >> +#define VLV_EDP_PSR_INT_TRANSITION (1<<24) >> +#define VLV_EDP_PSR_CTL(pipe) _PIPE(pipe, _PSRCTLA, _PSRCTLB) > > Can we just name the PSR registers like in the spec? Eg. just > VLV_PSRCTL(). Would make it easier to compare things w/ the spec. > Done. >> + >> +#define _VSCSDPA (VLV_DISPLAY_BASE + 0x600a0) >> +#define _VSCSDPB (VLV_DISPLAY_BASE + 0x610a0) >> +#define VLV_EDP_PSR_SDP_FREQ_MASK (3<<30) >> +#define VLV_EDP_PSR_SDP_FREQ_ONCE (1<<31) >> +#define VLV_EDP_PSR_SDP_FREQ_EVFRAME (1<<30) >> +#define VLV_EDP_VSC_SDP_REG(pipe) _PIPE(pipe, _VSCSDPA, _VSCSDPB) >> + >> +#define _PSRSTATA (VLV_DISPLAY_BASE + 0x60094) >> +#define _PSRSTATB (VLV_DISPLAY_BASE + 0x61094) >> +#define VLV_EDP_PSR_LAST_STATE_MASK (7<<3) >> +#define VLV_EDP_PSR_CURR_STATE_MASK 7 >> +#define VLV_EDP_PSR_DISABLED (0<<0) >> +#define VLV_EDP_PSR_INACTIVE (1<<0) >> +#define VLV_EDP_PSR_IN_TRANS_TO_ACTIVE (2<<0) >> +#define VLV_EDP_PSR_ACTIVE_NORFB_UP (3<<0) >> +#define VLV_EDP_PSR_ACTIVE_SF_UPDATE (4<<0) >> +#define VLV_EDP_PSR_EXIT (5<<0) >> +#define VLV_EDP_PSR_IN_TRANS (1<<7) >> +#define VLV_EDP_PSR_STATUS_CTL(pipe) _PIPE(pipe, _PSRSTATA, _PSRSTATB) >> + >> /* HSW+ eDP PSR registers */ >> #define EDP_PSR_BASE(dev) (IS_HASWELL(dev) ? 0x64800 : 0x6f800) >> #define EDP_PSR_CTL(dev) (EDP_PSR_BASE(dev) + 0) >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >> index 1a9aa19..081c8e2 100644 >> --- a/drivers/gpu/drm/i915/intel_display.c >> +++ b/drivers/gpu/drm/i915/intel_display.c >> @@ -7501,6 +7501,9 @@ static void intel_crtc_update_cursor(struct drm_crtc *crtc, >> u32 base = 0, pos = 0; >> bool visible; >> >> + if (IS_VALLEYVIEW(dev)) >> + intel_edp_psr_inactivate(dev); >> + >> if (on) >> base = intel_crtc->cursor_addr; >> >> @@ -8228,16 +8231,20 @@ void intel_mark_idle(struct drm_device *dev) >> >> if (dev_priv->info->gen >= 6) >> gen6_rps_idle(dev->dev_private); >> + >> + if (IS_VALLEYVIEW(dev)) >> + intel_edp_psr_update(dev); >> } >> >> + >> void intel_mark_fb_busy(struct drm_i915_gem_object *obj, >> struct intel_ring_buffer *ring) >> { >> struct drm_device *dev = obj->base.dev; >> struct drm_crtc *crtc; >> >> - if (!i915.powersave) >> - return; >> + if (IS_VALLEYVIEW(dev)) >> + intel_edp_psr_inactivate(dev); >> >> list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) { >> if (!crtc->fb) >> @@ -8688,6 +8695,10 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc, > nav> if (work == NULL) sorry, I didn't get this.. what did you mean? >> return -ENOMEM; >> >> + /* Inactivate PSR early in page flip */ >> + if (IS_VALLEYVIEW(dev)) >> + intel_edp_psr_inactivate(dev); >> + >> work->event = event; >> work->crtc = crtc; >> work->old_fb_obj = to_intel_framebuffer(old_fb)->obj; >> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c >> index 30d4350..e9a0ace 100644 >> --- a/drivers/gpu/drm/i915/intel_dp.c >> +++ b/drivers/gpu/drm/i915/intel_dp.c >> @@ -1578,21 +1578,35 @@ static void intel_dp_get_config(struct intel_encoder *encoder, >> } >> } >> >> -static bool is_edp_psr(struct drm_device *dev) >> +static bool is_edp_psr(struct intel_dp *intel_dp) >> +{ >> + return intel_dp->psr_dpcd[0] & DP_PSR_IS_SUPPORTED; >> +} >> + >> +static bool vlv_edp_is_psr_enabled_on_pipe(struct drm_device *dev, int pipe) >> { >> struct drm_i915_private *dev_priv = dev->dev_private; >> + uint32_t val; >> >> - return dev_priv->psr.sink_support; >> + val = I915_READ(VLV_EDP_PSR_STATUS_CTL(pipe)) & >> + VLV_EDP_PSR_CURR_STATE_MASK; >> + return (val == VLV_EDP_PSR_ACTIVE_NORFB_UP) || >> + (val == VLV_EDP_PSR_ACTIVE_SF_UPDATE); >> } >> >> static bool intel_edp_is_psr_enabled(struct drm_device *dev) >> { >> struct drm_i915_private *dev_priv = dev->dev_private; >> >> - if (!HAS_PSR(dev)) >> - return false; >> + if (HAS_PSR(dev)) { >> + if (IS_VALLEYVIEW(dev)) >> + return vlv_edp_is_psr_enabled_on_pipe(dev, PIPE_A) || >> + vlv_edp_is_psr_enabled_on_pipe(dev, PIPE_B); >> + else >> + return I915_READ(EDP_PSR_CTL(dev)) & EDP_PSR_ENABLE; >> + } >> >> - return I915_READ(EDP_PSR_CTL(dev)) & EDP_PSR_ENABLE; >> + return false; >> } >> >> static void intel_edp_psr_write_vsc(struct intel_dp *intel_dp, >> @@ -1626,28 +1640,49 @@ static void intel_edp_psr_write_vsc(struct intel_dp *intel_dp, >> >> static void intel_edp_psr_setup(struct intel_dp *intel_dp) >> { >> - struct drm_device *dev = intel_dp_to_dev(intel_dp); >> + struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); >> + struct drm_device *dev = intel_dig_port->base.base.dev; >> struct drm_i915_private *dev_priv = dev->dev_private; >> struct edp_vsc_psr psr_vsc; >> + uint32_t val; >> >> if (dev_priv->psr.setup_done) >> return; >> >> - /* Prepare VSC packet as per EDP 1.3 spec, Table 3.10 */ >> - memset(&psr_vsc, 0, sizeof(psr_vsc)); >> - psr_vsc.sdp_header.HB0 = 0; >> - psr_vsc.sdp_header.HB1 = 0x7; >> - psr_vsc.sdp_header.HB2 = 0x2; >> - psr_vsc.sdp_header.HB3 = 0x8; >> - intel_edp_psr_write_vsc(intel_dp, &psr_vsc); >> + if (IS_VALLEYVIEW(dev)) { >> + val = I915_READ(VLV_EDP_VSC_SDP_REG(PIPE_A)); >> + val &= ~VLV_EDP_PSR_SDP_FREQ_MASK; >> + val |= VLV_EDP_PSR_SDP_FREQ_EVFRAME; >> + I915_WRITE(VLV_EDP_VSC_SDP_REG(PIPE_A), val); >> + >> + val = I915_READ(VLV_EDP_VSC_SDP_REG(PIPE_B)); >> + val &= ~VLV_EDP_PSR_SDP_FREQ_MASK; >> + val |= VLV_EDP_PSR_SDP_FREQ_EVFRAME; >> + I915_WRITE(VLV_EDP_VSC_SDP_REG(PIPE_B), val); >> + } else { >> + /* Prepare VSC packet as per EDP 1.3 spec, Table 3.10 */ >> + memset(&psr_vsc, 0, sizeof(psr_vsc)); >> + psr_vsc.sdp_header.HB0 = 0; >> + psr_vsc.sdp_header.HB1 = 0x7; >> + psr_vsc.sdp_header.HB2 = 0x2; >> + psr_vsc.sdp_header.HB3 = 0x8; >> + intel_edp_psr_write_vsc(intel_dp, &psr_vsc); >> >> - /* Avoid continuous PSR exit by masking memup and hpd */ >> - I915_WRITE(EDP_PSR_DEBUG_CTL(dev), EDP_PSR_DEBUG_MASK_MEMUP | >> - EDP_PSR_DEBUG_MASK_HPD | EDP_PSR_DEBUG_MASK_LPSP); >> + /* Avoid continuous PSR exit by masking memup and hpd */ >> + I915_WRITE(EDP_PSR_DEBUG_CTL(dev), EDP_PSR_DEBUG_MASK_MEMUP | >> + EDP_PSR_DEBUG_MASK_HPD | EDP_PSR_DEBUG_MASK_LPSP); >> + } >> >> dev_priv->psr.setup_done = true; >> } >> >> +static void vlv_edp_psr_enable_sink(struct intel_dp *intel_dp) >> +{ >> + /* Enable PSR in sink */ >> + intel_dp_aux_native_write_1(intel_dp, DP_PSR_EN_CFG, >> + DP_PSR_ENABLE); > > Don't we want the same main-link poweroff logic as HSW? I can't remember why I'm not disabling main link here... So I did a quickly try now and failure on tests was huge... so let's keep it simple for now ;) > Or maybe we > should just keep the main link on always as long as we can't enter > the low power states w/ DPLL/pipe/port off. > > Did you already figure out why that's not happening? Looking at the > PSRSTAT registers, my guess is that D0i1 is where we end up currently, > and that doesn't actually turn off anything but the planes (to stop > memory fetches). D0i2 would turn off everything. no idea.. :( > But I guess we should anyway do this in steps. First getting the > current stuff in, then trying to get into D0i2 state, and finally > getting the display power well turned off when in PSR. Agree. > > I think once we get to working on D0i2, we'll need to move the PSR > wakeup to happen from a workqueue since it essentially requires a > full modeset. Even now in your code it's somewhat questionable > since you're doing stuff like aux transfers while holding > struct_mutex. uhm... anything we should try now to improve? > >> +} >> + >> static void intel_edp_psr_enable_sink(struct intel_dp *intel_dp) >> { >> struct drm_device *dev = intel_dp_to_dev(intel_dp); >> @@ -1678,6 +1713,28 @@ static void intel_edp_psr_enable_sink(struct intel_dp *intel_dp) >> (aux_clock_divider << DP_AUX_CH_CTL_BIT_CLOCK_2X_SHIFT)); >> } >> >> +static void vlv_edp_psr_enable_source(struct intel_dp *intel_dp) >> +{ >> + struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); >> + struct drm_device *dev = intel_dig_port->base.base.dev; >> + struct drm_i915_private *dev_priv = dev->dev_private; >> + struct intel_crtc *intel_crtc = >> + to_intel_crtc(intel_dig_port->base.base.crtc); >> + >> + uint32_t idle_frames = 1; >> + uint32_t val; >> + >> + val = I915_READ(VLV_EDP_PSR_CTL(intel_crtc->pipe)); > > I don't think we want to preserve anything here. We need to make sure > everything is initialized correctly rather than trusting some old junk > in the register. Done. > >> + val |= VLV_EDP_PSR_ENABLE; >> + val &= ~VLV_EDP_PSR_MODE_MASK; >> + >> + val |= VLV_EDP_PSR_MODE_HW_TIMER; >> + val &= ~VLV_EDP_PSR_FRAME_COUNT_MASK; >> + val |= idle_frames << EDP_PSR_IDLE_FRAME_SHIFT; >> + >> + I915_WRITE(VLV_EDP_PSR_CTL(intel_crtc->pipe), val); >> +} >> + >> static void intel_edp_psr_enable_source(struct intel_dp *intel_dp) >> { >> struct drm_device *dev = intel_dp_to_dev(intel_dp); >> @@ -1719,8 +1776,8 @@ static bool intel_edp_psr_match_conditions(struct intel_dp *intel_dp) >> return false; >> } >> >> - if ((intel_encoder->type != INTEL_OUTPUT_EDP) || >> - (dig_port->port != PORT_A)) { >> + if (HAS_DDI(dev) && ((intel_encoder->type != INTEL_OUTPUT_EDP) || >> + (dig_port->port != PORT_A))) { >> DRM_DEBUG_KMS("HSW ties PSR to DDI A (eDP)\n"); >> return false; >> } >> @@ -1765,37 +1822,83 @@ static bool intel_edp_psr_match_conditions(struct intel_dp *intel_dp) >> return false; >> } >> >> + /* Baytrail supports per-pipe PSR configuration, however PSR on >> + * PIPE_B isn't working properly. So let's keep it disabled for now. */ >> + if (IS_VALLEYVIEW(dev) && intel_crtc->pipe != PIPE_A) { >> + DRM_DEBUG_KMS("PSR on BYT isn't enabled on pipe B.\n"); >> + return false; >> + } >> + >> dev_priv->psr.source_ok = true; >> return true; >> } >> >> static void intel_edp_psr_do_enable(struct intel_dp *intel_dp) >> { >> - struct drm_device *dev = intel_dp_to_dev(intel_dp); >> + struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); >> + struct drm_device *dev = intel_dig_port->base.base.dev; >> + struct drm_i915_private *dev_priv = dev->dev_private; >> + struct intel_crtc *intel_crtc = >> + to_intel_crtc(intel_dig_port->base.base.crtc); >> >> - if (!intel_edp_psr_match_conditions(intel_dp) || >> - intel_edp_is_psr_enabled(dev)) >> - return; >> + if (IS_VALLEYVIEW(dev)) { >> + if (vlv_edp_is_psr_enabled_on_pipe(dev, intel_crtc->pipe)) >> + return; >> + } else >> + if (intel_edp_is_psr_enabled(dev)) >> + return; >> >> /* Setup PSR once */ >> intel_edp_psr_setup(intel_dp); >> >> /* Enable PSR on the panel */ >> - intel_edp_psr_enable_sink(intel_dp); >> + if (IS_VALLEYVIEW(dev)) >> + vlv_edp_psr_enable_sink(intel_dp); >> + else >> + intel_edp_psr_enable_sink(intel_dp); >> >> /* Enable PSR on the host */ >> - intel_edp_psr_enable_source(intel_dp); >> + if (IS_VALLEYVIEW(dev)) >> + vlv_edp_psr_enable_source(intel_dp); >> + else >> + intel_edp_psr_enable_source(intel_dp); >> + >> + dev_priv->psr.active = true; >> } >> >> void intel_edp_psr_enable(struct intel_dp *intel_dp) >> { >> - struct drm_device *dev = intel_dp_to_dev(intel_dp); >> + if (!is_edp_psr(intel_dp)) >> + return; >> >> - if (intel_edp_psr_match_conditions(intel_dp) && >> - !intel_edp_is_psr_enabled(dev)) >> + if (intel_edp_psr_match_conditions(intel_dp)) >> intel_edp_psr_do_enable(intel_dp); >> } >> >> +void vlv_edp_psr_disable(struct intel_dp *intel_dp) >> +{ >> + struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); >> + struct drm_device *dev = intel_dig_port->base.base.dev; >> + struct drm_i915_private *dev_priv = dev->dev_private; >> + struct intel_crtc *intel_crtc = >> + to_intel_crtc(intel_dig_port->base.base.crtc); >> + uint32_t val = I915_READ(VLV_EDP_PSR_STATUS_CTL(intel_crtc->pipe)); >> + >> + if (!dev_priv->psr.setup_done) >> + return; >> + >> + intel_edp_psr_inactivate(dev); >> + >> + if (val & VLV_EDP_PSR_IN_TRANS) >> + udelay(250); > > Might we want a warning if the bit doesn't go down in expected time? Done. > >> + >> + val = I915_READ(VLV_EDP_PSR_CTL(intel_crtc->pipe)); >> + val &= ~VLV_EDP_PSR_ACTIVE_ENTRY; >> + val &= ~VLV_EDP_PSR_ENABLE; >> + val &= ~VLV_EDP_PSR_MODE_MASK; >> + I915_WRITE(VLV_EDP_PSR_CTL(intel_crtc->pipe), val); >> +} >> + >> void intel_edp_psr_disable(struct intel_dp *intel_dp) >> { >> struct drm_device *dev = intel_dp_to_dev(intel_dp); >> @@ -1817,28 +1920,66 @@ void intel_edp_psr_update(struct drm_device *dev) >> { >> struct intel_encoder *encoder; >> struct intel_dp *intel_dp = NULL; >> + struct intel_crtc *intel_crtc; >> >> list_for_each_entry(encoder, &dev->mode_config.encoder_list, base.head) >> if (encoder->type == INTEL_OUTPUT_EDP) { >> intel_dp = enc_to_intel_dp(&encoder->base); >> >> - if (!is_edp_psr(dev)) >> + if (!is_edp_psr(intel_dp)) >> return; >> >> - if (!intel_edp_psr_match_conditions(intel_dp)) >> - intel_edp_psr_disable(intel_dp); >> - else >> + intel_crtc = to_intel_crtc(encoder->base.crtc); >> + >> + if (!intel_edp_psr_match_conditions(intel_dp)) { >> + if (IS_VALLEYVIEW(dev)) >> + vlv_edp_psr_disable(intel_dp); >> + else >> + intel_edp_psr_disable(intel_dp); >> + } else >> if (!intel_edp_is_psr_enabled(dev)) >> intel_edp_psr_do_enable(intel_dp); >> } >> } >> >> +void intel_edp_psr_inactivate(struct drm_device *dev) >> +{ >> + struct drm_i915_private *dev_priv = dev->dev_private; >> + struct intel_encoder *encoder; >> + struct intel_crtc *intel_crtc; >> + struct intel_dp *intel_dp = NULL; >> + >> + if (!dev_priv->psr.setup_done || !dev_priv->psr.active) >> + return; >> + >> + list_for_each_entry(encoder, &dev->mode_config.encoder_list, base.head) >> + if (encoder->type == INTEL_OUTPUT_EDP) { >> + intel_dp = enc_to_intel_dp(&encoder->base); >> + intel_crtc = to_intel_crtc(encoder->base.crtc); >> + >> + if (!vlv_edp_is_psr_enabled_on_pipe(dev, >> + intel_crtc->pipe)) >> + continue; >> + >> + dev_priv->psr.active = false; >> + I915_WRITE(VLV_EDP_PSR_CTL(intel_crtc->pipe), >> + VLV_EDP_PSR_RESET); >> + /* WaClearPSRReset:vlv */ >> + I915_WRITE(VLV_EDP_PSR_CTL(intel_crtc->pipe), 0); >> + >> + intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON); >> + } >> +} >> + >> static void intel_disable_dp(struct intel_encoder *encoder) >> { >> struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base); >> enum port port = dp_to_dig_port(intel_dp)->port; >> struct drm_device *dev = encoder->base.dev; >> >> + if (IS_VALLEYVIEW(dev)) >> + vlv_edp_psr_disable(intel_dp); >> + >> /* Make sure the panel is off before trying to change the mode. But also >> * ensure that we have vdd while we switch off the panel. */ >> intel_edp_backlight_off(intel_dp); >> @@ -1895,6 +2036,7 @@ static void vlv_enable_dp(struct intel_encoder *encoder) >> struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base); >> >> intel_edp_backlight_on(intel_dp); >> + intel_edp_psr_enable(intel_dp); >> } >> >> static void g4x_pre_enable_dp(struct intel_encoder *encoder) >> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h >> index 71c1371..82026ef 100644 >> --- a/drivers/gpu/drm/i915/intel_drv.h >> +++ b/drivers/gpu/drm/i915/intel_drv.h >> @@ -748,6 +748,7 @@ void intel_edp_panel_off(struct intel_dp *intel_dp); >> void intel_edp_psr_enable(struct intel_dp *intel_dp); >> void intel_edp_psr_disable(struct intel_dp *intel_dp); >> void intel_edp_psr_update(struct drm_device *dev); >> +void intel_edp_psr_inactivate(struct drm_device *dev); >> >> >> /* intel_dsi.c */ >> -- >> 1.8.1.2 > > -- > Ville Syrjälä > Intel OTC Thank you very much!
On Wed, Jan 29, 2014 at 01:47:00PM -0200, Rodrigo Vivi wrote: > On Wed, Jan 29, 2014 at 12:56 PM, Ville Syrjälä > <ville.syrjala@linux.intel.com> wrote: > > On Wed, Jan 29, 2014 at 10:47:54AM -0200, Rodrigo Vivi wrote: > >> This patch adds PSR Support to Baytrail. > >> > >> Baytrail cannot easily detect screen updates and force PSR exit. > >> So we inactivate it on {busy_ioctl, set_domain, sw_finish and mark_busy > >> and update to enable it back on next display mark_idle. > >> > >> v2: Also inactivate PSR on cursor update. > >> v3: Inactivate PSR on mark_busy, dset_domain and sw_finish_ioctl, and > >> early on page flip besides avoid initializing inactive/active flag > >> more than once. > >> v4: Fix identation issues. > >> v5: Rebase and add Baytrail per pipe support although leaving PIPE_B > >> support disabled by for now since it isn't working properly yet. > >> v6: Removing forgotten comment and useless clkgating definition. > >> > >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > >> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com> > >> --- > >> drivers/gpu/drm/i915/i915_debugfs.c | 36 ++++++- > >> drivers/gpu/drm/i915/i915_drv.h | 4 +- > >> drivers/gpu/drm/i915/i915_gem.c | 9 ++ > >> drivers/gpu/drm/i915/i915_reg.h | 37 +++++++ > >> drivers/gpu/drm/i915/intel_display.c | 15 ++- > >> drivers/gpu/drm/i915/intel_dp.c | 204 +++++++++++++++++++++++++++++------ > >> drivers/gpu/drm/i915/intel_drv.h | 1 + > >> 7 files changed, 267 insertions(+), 39 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c > >> index 4b852c6..c28de65 100644 > >> --- a/drivers/gpu/drm/i915/i915_debugfs.c > >> +++ b/drivers/gpu/drm/i915/i915_debugfs.c > >> @@ -1900,6 +1900,8 @@ static int i915_edp_psr_status(struct seq_file *m, void *data) > >> struct drm_device *dev = node->minor->dev; > >> struct drm_i915_private *dev_priv = dev->dev_private; > >> u32 psrperf = 0; > >> + u32 statA = 0; > >> + u32 statB = 0; > >> bool enabled = false; > >> > >> intel_runtime_pm_get(dev_priv); > >> @@ -1907,14 +1909,38 @@ static int i915_edp_psr_status(struct seq_file *m, void *data) > >> seq_printf(m, "Sink_Support: %s\n", yesno(dev_priv->psr.sink_support)); > >> seq_printf(m, "Source_OK: %s\n", yesno(dev_priv->psr.source_ok)); > >> > >> - enabled = HAS_PSR(dev) && > >> - I915_READ(EDP_PSR_CTL(dev)) & EDP_PSR_ENABLE; > >> - seq_printf(m, "Enabled: %s\n", yesno(enabled)); > >> + if (HAS_PSR(dev)) { > >> + if (IS_VALLEYVIEW(dev)) { > >> + statA = I915_READ(VLV_EDP_PSR_STATUS_CTL(PIPE_A)) & > >> + VLV_EDP_PSR_CURR_STATE_MASK; > >> + statB = I915_READ(VLV_EDP_PSR_STATUS_CTL(PIPE_B)) & > >> + VLV_EDP_PSR_CURR_STATE_MASK; > >> + enabled = ((statA == VLV_EDP_PSR_ACTIVE_NORFB_UP) || > >> + (statA == VLV_EDP_PSR_ACTIVE_SF_UPDATE) || > >> + (statB == VLV_EDP_PSR_ACTIVE_NORFB_UP) || > >> + (statB == VLV_EDP_PSR_ACTIVE_SF_UPDATE)); > >> + } else > >> + enabled = I915_READ(EDP_PSR_CTL(dev)) & EDP_PSR_ENABLE; > >> + } > >> + seq_printf(m, "Enabled: %s", yesno(enabled)); > >> > >> - if (HAS_PSR(dev)) > >> + if (IS_VALLEYVIEW(dev)) { > >> + if ((statA == VLV_EDP_PSR_ACTIVE_NORFB_UP) || > >> + (statA == VLV_EDP_PSR_ACTIVE_SF_UPDATE)) > >> + seq_puts(m, " pipe A"); > >> + if ((statB == VLV_EDP_PSR_ACTIVE_NORFB_UP) || > >> + (statB == VLV_EDP_PSR_ACTIVE_SF_UPDATE)) > >> + seq_puts(m, " pipe B"); > >> + } > >> + > >> + seq_puts(m, "\n"); > >> + > >> + /* VLV PSR has no kind of performance counter */ > >> + if (HAS_PSR(dev) && !IS_VALLEYVIEW(dev)) { > >> psrperf = I915_READ(EDP_PSR_PERF_CNT(dev)) & > >> EDP_PSR_PERF_CNT_MASK; > >> - seq_printf(m, "Performance_Counter: %u\n", psrperf); > >> + seq_printf(m, "Performance_Counter: %u\n", psrperf); > >> + } > >> > >> intel_runtime_pm_put(dev_priv); > >> return 0; > >> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > >> index 7c53d4d..34dee24 100644 > >> --- a/drivers/gpu/drm/i915/i915_drv.h > >> +++ b/drivers/gpu/drm/i915/i915_drv.h > >> @@ -747,6 +747,7 @@ struct i915_psr { > >> bool sink_support; > >> bool source_ok; > >> bool setup_done; > >> + bool active; > >> }; > >> > >> enum intel_pch { > >> @@ -1866,7 +1867,8 @@ struct drm_i915_file_private { > >> > >> #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)) > >> +#define HAS_PSR(dev) (IS_HASWELL(dev) || IS_BROADWELL(dev) || \ > >> + IS_VALLEYVIEW(dev)) > >> #define HAS_PC8(dev) (IS_HASWELL(dev)) /* XXX HSW:ULX */ > >> #define HAS_RUNTIME_PM(dev) (IS_HASWELL(dev)) > >> > >> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > >> index 39770f7..01137fe 100644 > >> --- a/drivers/gpu/drm/i915/i915_gem.c > >> +++ b/drivers/gpu/drm/i915/i915_gem.c > >> @@ -1256,6 +1256,9 @@ i915_gem_set_domain_ioctl(struct drm_device *dev, void *data, > >> goto unlock; > >> } > >> > >> + if (IS_VALLEYVIEW(dev)) > >> + intel_edp_psr_inactivate(dev); > >> + > >> /* 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. > >> @@ -1299,6 +1302,9 @@ i915_gem_sw_finish_ioctl(struct drm_device *dev, void *data, > >> if (ret) > >> return ret; > >> > >> + if (IS_VALLEYVIEW(dev)) > >> + intel_edp_psr_inactivate(dev); > >> + > >> obj = to_intel_bo(drm_gem_object_lookup(dev, file, args->handle)); > >> if (&obj->base == NULL) { > >> ret = -ENOENT; > >> @@ -4059,6 +4065,9 @@ i915_gem_busy_ioctl(struct drm_device *dev, void *data, > >> if (ret) > >> return ret; > >> > >> + if (IS_VALLEYVIEW(dev)) > >> + intel_edp_psr_inactivate(dev); > > > > The locking for PSR seems to be as fubar as for FBC. Also the front > > buffer tracking is missing, but I guess I need to make that work for FBC > > first, and then we need to figure out how to tie it in w/ PSR. > > > > agree... I'll wait your fbc work. > > >> + > >> obj = to_intel_bo(drm_gem_object_lookup(dev, file, args->handle)); > >> if (&obj->base == NULL) { > >> ret = -ENOENT; > >> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > >> index cbbaf26..2039d71 100644 > >> --- a/drivers/gpu/drm/i915/i915_reg.h > >> +++ b/drivers/gpu/drm/i915/i915_reg.h > >> @@ -1968,6 +1968,43 @@ > >> #define BCLRPAT(pipe) _PIPE(pipe, _BCLRPAT_A, _BCLRPAT_B) > >> #define VSYNCSHIFT(trans) _TRANSCODER(trans, _VSYNCSHIFT_A, _VSYNCSHIFT_B) > >> > >> +/* VLV eDP PSR registers */ > >> +#define _PSRCTLA (VLV_DISPLAY_BASE + 0x60090) > >> +#define _PSRCTLB (VLV_DISPLAY_BASE + 0x61090) > >> +#define VLV_EDP_PSR_ENABLE (1<<0) > >> +#define VLV_EDP_PSR_RESET (1<<1) > >> +#define VLV_EDP_PSR_MODE_MASK (7<<2) > >> +#define VLV_EDP_PSR_MODE_HW_TIMER (1<<3) > >> +#define VLV_EDP_PSR_MODE_SW_TIMER (1<<2) > >> +#define VLV_EDP_PSR_SINGLE_FRAME_UPDATE (1<<7) > >> +#define VLV_EDP_PSR_ACTIVE_ENTRY (1<<8) > >> +#define VLV_EDP_PSR_SRC_TRANSMITTER_STATE (1<<9) > >> +#define VLV_EDP_PSR_DBL_FRAME (1<<10) > >> +#define VLV_EDP_PSR_FRAME_COUNT_MASK (0xff<<16) > >> +#define VLV_EDP_PSR_IDLE_FRAME_SHIFT 16 > >> +#define VLV_EDP_PSR_INT_TRANSITION (1<<24) > >> +#define VLV_EDP_PSR_CTL(pipe) _PIPE(pipe, _PSRCTLA, _PSRCTLB) > > > > Can we just name the PSR registers like in the spec? Eg. just > > VLV_PSRCTL(). Would make it easier to compare things w/ the spec. > > > > Done. > > >> + > >> +#define _VSCSDPA (VLV_DISPLAY_BASE + 0x600a0) > >> +#define _VSCSDPB (VLV_DISPLAY_BASE + 0x610a0) > >> +#define VLV_EDP_PSR_SDP_FREQ_MASK (3<<30) > >> +#define VLV_EDP_PSR_SDP_FREQ_ONCE (1<<31) > >> +#define VLV_EDP_PSR_SDP_FREQ_EVFRAME (1<<30) > >> +#define VLV_EDP_VSC_SDP_REG(pipe) _PIPE(pipe, _VSCSDPA, _VSCSDPB) > >> + > >> +#define _PSRSTATA (VLV_DISPLAY_BASE + 0x60094) > >> +#define _PSRSTATB (VLV_DISPLAY_BASE + 0x61094) > >> +#define VLV_EDP_PSR_LAST_STATE_MASK (7<<3) > >> +#define VLV_EDP_PSR_CURR_STATE_MASK 7 > >> +#define VLV_EDP_PSR_DISABLED (0<<0) > >> +#define VLV_EDP_PSR_INACTIVE (1<<0) > >> +#define VLV_EDP_PSR_IN_TRANS_TO_ACTIVE (2<<0) > >> +#define VLV_EDP_PSR_ACTIVE_NORFB_UP (3<<0) > >> +#define VLV_EDP_PSR_ACTIVE_SF_UPDATE (4<<0) > >> +#define VLV_EDP_PSR_EXIT (5<<0) > >> +#define VLV_EDP_PSR_IN_TRANS (1<<7) > >> +#define VLV_EDP_PSR_STATUS_CTL(pipe) _PIPE(pipe, _PSRSTATA, _PSRSTATB) > >> + > >> /* HSW+ eDP PSR registers */ > >> #define EDP_PSR_BASE(dev) (IS_HASWELL(dev) ? 0x64800 : 0x6f800) > >> #define EDP_PSR_CTL(dev) (EDP_PSR_BASE(dev) + 0) > >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > >> index 1a9aa19..081c8e2 100644 > >> --- a/drivers/gpu/drm/i915/intel_display.c > >> +++ b/drivers/gpu/drm/i915/intel_display.c > >> @@ -7501,6 +7501,9 @@ static void intel_crtc_update_cursor(struct drm_crtc *crtc, > >> u32 base = 0, pos = 0; > >> bool visible; > >> > >> + if (IS_VALLEYVIEW(dev)) > >> + intel_edp_psr_inactivate(dev); > >> + > >> if (on) > >> base = intel_crtc->cursor_addr; > >> > >> @@ -8228,16 +8231,20 @@ void intel_mark_idle(struct drm_device *dev) > >> > >> if (dev_priv->info->gen >= 6) > >> gen6_rps_idle(dev->dev_private); > >> + > >> + if (IS_VALLEYVIEW(dev)) > >> + intel_edp_psr_update(dev); > >> } > >> > >> + > >> void intel_mark_fb_busy(struct drm_i915_gem_object *obj, > >> struct intel_ring_buffer *ring) > >> { > >> struct drm_device *dev = obj->base.dev; > >> struct drm_crtc *crtc; > >> > >> - if (!i915.powersave) > >> - return; > >> + if (IS_VALLEYVIEW(dev)) > >> + intel_edp_psr_inactivate(dev); > >> > >> list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) { > >> if (!crtc->fb) > >> @@ -8688,6 +8695,10 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc, > > nav> if (work == NULL) > > sorry, I didn't get this.. > what did you mean? Nothing. Just managed to misplace a few characters here I guess. I blame vim :) > > >> return -ENOMEM; > >> > >> + /* Inactivate PSR early in page flip */ > >> + if (IS_VALLEYVIEW(dev)) > >> + intel_edp_psr_inactivate(dev); > >> + > >> work->event = event; > >> work->crtc = crtc; > >> work->old_fb_obj = to_intel_framebuffer(old_fb)->obj; > >> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > >> index 30d4350..e9a0ace 100644 > >> --- a/drivers/gpu/drm/i915/intel_dp.c > >> +++ b/drivers/gpu/drm/i915/intel_dp.c > >> @@ -1578,21 +1578,35 @@ static void intel_dp_get_config(struct intel_encoder *encoder, > >> } > >> } > >> > >> -static bool is_edp_psr(struct drm_device *dev) > >> +static bool is_edp_psr(struct intel_dp *intel_dp) > >> +{ > >> + return intel_dp->psr_dpcd[0] & DP_PSR_IS_SUPPORTED; > >> +} > >> + > >> +static bool vlv_edp_is_psr_enabled_on_pipe(struct drm_device *dev, int pipe) > >> { > >> struct drm_i915_private *dev_priv = dev->dev_private; > >> + uint32_t val; > >> > >> - return dev_priv->psr.sink_support; > >> + val = I915_READ(VLV_EDP_PSR_STATUS_CTL(pipe)) & > >> + VLV_EDP_PSR_CURR_STATE_MASK; > >> + return (val == VLV_EDP_PSR_ACTIVE_NORFB_UP) || > >> + (val == VLV_EDP_PSR_ACTIVE_SF_UPDATE); > >> } > >> > >> static bool intel_edp_is_psr_enabled(struct drm_device *dev) > >> { > >> struct drm_i915_private *dev_priv = dev->dev_private; > >> > >> - if (!HAS_PSR(dev)) > >> - return false; > >> + if (HAS_PSR(dev)) { > >> + if (IS_VALLEYVIEW(dev)) > >> + return vlv_edp_is_psr_enabled_on_pipe(dev, PIPE_A) || > >> + vlv_edp_is_psr_enabled_on_pipe(dev, PIPE_B); > >> + else > >> + return I915_READ(EDP_PSR_CTL(dev)) & EDP_PSR_ENABLE; > >> + } > >> > >> - return I915_READ(EDP_PSR_CTL(dev)) & EDP_PSR_ENABLE; > >> + return false; > >> } > >> > >> static void intel_edp_psr_write_vsc(struct intel_dp *intel_dp, > >> @@ -1626,28 +1640,49 @@ static void intel_edp_psr_write_vsc(struct intel_dp *intel_dp, > >> > >> static void intel_edp_psr_setup(struct intel_dp *intel_dp) > >> { > >> - struct drm_device *dev = intel_dp_to_dev(intel_dp); > >> + struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); > >> + struct drm_device *dev = intel_dig_port->base.base.dev; > >> struct drm_i915_private *dev_priv = dev->dev_private; > >> struct edp_vsc_psr psr_vsc; > >> + uint32_t val; > >> > >> if (dev_priv->psr.setup_done) > >> return; > >> > >> - /* Prepare VSC packet as per EDP 1.3 spec, Table 3.10 */ > >> - memset(&psr_vsc, 0, sizeof(psr_vsc)); > >> - psr_vsc.sdp_header.HB0 = 0; > >> - psr_vsc.sdp_header.HB1 = 0x7; > >> - psr_vsc.sdp_header.HB2 = 0x2; > >> - psr_vsc.sdp_header.HB3 = 0x8; > >> - intel_edp_psr_write_vsc(intel_dp, &psr_vsc); > >> + if (IS_VALLEYVIEW(dev)) { > >> + val = I915_READ(VLV_EDP_VSC_SDP_REG(PIPE_A)); > >> + val &= ~VLV_EDP_PSR_SDP_FREQ_MASK; > >> + val |= VLV_EDP_PSR_SDP_FREQ_EVFRAME; > >> + I915_WRITE(VLV_EDP_VSC_SDP_REG(PIPE_A), val); > >> + > >> + val = I915_READ(VLV_EDP_VSC_SDP_REG(PIPE_B)); > >> + val &= ~VLV_EDP_PSR_SDP_FREQ_MASK; > >> + val |= VLV_EDP_PSR_SDP_FREQ_EVFRAME; > >> + I915_WRITE(VLV_EDP_VSC_SDP_REG(PIPE_B), val); > >> + } else { > >> + /* Prepare VSC packet as per EDP 1.3 spec, Table 3.10 */ > >> + memset(&psr_vsc, 0, sizeof(psr_vsc)); > >> + psr_vsc.sdp_header.HB0 = 0; > >> + psr_vsc.sdp_header.HB1 = 0x7; > >> + psr_vsc.sdp_header.HB2 = 0x2; > >> + psr_vsc.sdp_header.HB3 = 0x8; > >> + intel_edp_psr_write_vsc(intel_dp, &psr_vsc); > >> > >> - /* Avoid continuous PSR exit by masking memup and hpd */ > >> - I915_WRITE(EDP_PSR_DEBUG_CTL(dev), EDP_PSR_DEBUG_MASK_MEMUP | > >> - EDP_PSR_DEBUG_MASK_HPD | EDP_PSR_DEBUG_MASK_LPSP); > >> + /* Avoid continuous PSR exit by masking memup and hpd */ > >> + I915_WRITE(EDP_PSR_DEBUG_CTL(dev), EDP_PSR_DEBUG_MASK_MEMUP | > >> + EDP_PSR_DEBUG_MASK_HPD | EDP_PSR_DEBUG_MASK_LPSP); > >> + } > >> > >> dev_priv->psr.setup_done = true; > >> } > >> > >> +static void vlv_edp_psr_enable_sink(struct intel_dp *intel_dp) > >> +{ > >> + /* Enable PSR in sink */ > >> + intel_dp_aux_native_write_1(intel_dp, DP_PSR_EN_CFG, > >> + DP_PSR_ENABLE); > > > > Don't we want the same main-link poweroff logic as HSW? > > > I can't remember why I'm not disabling main link here... So I did a > quickly try now and failure on tests was huge... so let's keep it > simple for now ;) I think you are disabling the main link currently. Or at least you're telling the sink that main link is going to be turned off, and you also leave the VLV_EDP_PSR_SRC_TRANSMITTER_STATE bit unset, so I think that should make the main link turn off. Well, since the pipe and port remain active, I don't know if the link actually turns off. I would assume that if you don't turn off the link, it would be easier to keep things happy. A bit weird that you had the opposite result. BTW now that I look at the HSW code, it seems buggy. When the sink doesn't require link training, we tell it that we're going to turn the link off, otherwise we tell it link will be on. But then we actually do the opposite in intel_edp_psr_enable_source(). > > > Or maybe we > > should just keep the main link on always as long as we can't enter > > the low power states w/ DPLL/pipe/port off. > > > > Did you already figure out why that's not happening? Looking at the > > PSRSTAT registers, my guess is that D0i1 is where we end up currently, > > and that doesn't actually turn off anything but the planes (to stop > > memory fetches). D0i2 would turn off everything. > > no idea.. :( Did you check the status bit BTW? Does it say D0i1 or D0i2 when you're in PSR? > > > But I guess we should anyway do this in steps. First getting the > > current stuff in, then trying to get into D0i2 state, and finally > > getting the display power well turned off when in PSR. > > Agree. > > > > > I think once we get to working on D0i2, we'll need to move the PSR > > wakeup to happen from a workqueue since it essentially requires a > > full modeset. Even now in your code it's somewhat questionable > > since you're doing stuff like aux transfers while holding > > struct_mutex. > > uhm... anything we should try now to improve? Hmm. We already seem to do some PSR setup while holding struct_mutex, but we don't hold it always. Although I think w/ HSW we end up protecting the PSR state w/ the modeset locks since we only fiddle with it during modesets. So for HSW we should just move the PSR update call in set_base outside struct_mutex. But then for VLV, you want to call it from various places that already hold struct_mutex, so it's getting messy. Just moving it to a workqueue would seem like the easiest option here, and then you could grab the modeset locks in the work function. > > > > >> +} > >> + > >> static void intel_edp_psr_enable_sink(struct intel_dp *intel_dp) > >> { > >> struct drm_device *dev = intel_dp_to_dev(intel_dp); > >> @@ -1678,6 +1713,28 @@ static void intel_edp_psr_enable_sink(struct intel_dp *intel_dp) > >> (aux_clock_divider << DP_AUX_CH_CTL_BIT_CLOCK_2X_SHIFT)); > >> } > >> > >> +static void vlv_edp_psr_enable_source(struct intel_dp *intel_dp) > >> +{ > >> + struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); > >> + struct drm_device *dev = intel_dig_port->base.base.dev; > >> + struct drm_i915_private *dev_priv = dev->dev_private; > >> + struct intel_crtc *intel_crtc = > >> + to_intel_crtc(intel_dig_port->base.base.crtc); > >> + > >> + uint32_t idle_frames = 1; > >> + uint32_t val; > >> + > >> + val = I915_READ(VLV_EDP_PSR_CTL(intel_crtc->pipe)); > > > > I don't think we want to preserve anything here. We need to make sure > > everything is initialized correctly rather than trusting some old junk > > in the register. > > Done. > > > > >> + val |= VLV_EDP_PSR_ENABLE; > >> + val &= ~VLV_EDP_PSR_MODE_MASK; > >> + > >> + val |= VLV_EDP_PSR_MODE_HW_TIMER; > >> + val &= ~VLV_EDP_PSR_FRAME_COUNT_MASK; > >> + val |= idle_frames << EDP_PSR_IDLE_FRAME_SHIFT; > >> + > >> + I915_WRITE(VLV_EDP_PSR_CTL(intel_crtc->pipe), val); > >> +} > >> + > >> static void intel_edp_psr_enable_source(struct intel_dp *intel_dp) > >> { > >> struct drm_device *dev = intel_dp_to_dev(intel_dp); > >> @@ -1719,8 +1776,8 @@ static bool intel_edp_psr_match_conditions(struct intel_dp *intel_dp) > >> return false; > >> } > >> > >> - if ((intel_encoder->type != INTEL_OUTPUT_EDP) || > >> - (dig_port->port != PORT_A)) { > >> + if (HAS_DDI(dev) && ((intel_encoder->type != INTEL_OUTPUT_EDP) || > >> + (dig_port->port != PORT_A))) { > >> DRM_DEBUG_KMS("HSW ties PSR to DDI A (eDP)\n"); > >> return false; > >> } > >> @@ -1765,37 +1822,83 @@ static bool intel_edp_psr_match_conditions(struct intel_dp *intel_dp) > >> return false; > >> } > >> > >> + /* Baytrail supports per-pipe PSR configuration, however PSR on > >> + * PIPE_B isn't working properly. So let's keep it disabled for now. */ > >> + if (IS_VALLEYVIEW(dev) && intel_crtc->pipe != PIPE_A) { > >> + DRM_DEBUG_KMS("PSR on BYT isn't enabled on pipe B.\n"); > >> + return false; > >> + } > >> + > >> dev_priv->psr.source_ok = true; > >> return true; > >> } > >> > >> static void intel_edp_psr_do_enable(struct intel_dp *intel_dp) > >> { > >> - struct drm_device *dev = intel_dp_to_dev(intel_dp); > >> + struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); > >> + struct drm_device *dev = intel_dig_port->base.base.dev; > >> + struct drm_i915_private *dev_priv = dev->dev_private; > >> + struct intel_crtc *intel_crtc = > >> + to_intel_crtc(intel_dig_port->base.base.crtc); > >> > >> - if (!intel_edp_psr_match_conditions(intel_dp) || > >> - intel_edp_is_psr_enabled(dev)) > >> - return; > >> + if (IS_VALLEYVIEW(dev)) { > >> + if (vlv_edp_is_psr_enabled_on_pipe(dev, intel_crtc->pipe)) > >> + return; > >> + } else > >> + if (intel_edp_is_psr_enabled(dev)) > >> + return; > >> > >> /* Setup PSR once */ > >> intel_edp_psr_setup(intel_dp); > >> > >> /* Enable PSR on the panel */ > >> - intel_edp_psr_enable_sink(intel_dp); > >> + if (IS_VALLEYVIEW(dev)) > >> + vlv_edp_psr_enable_sink(intel_dp); > >> + else > >> + intel_edp_psr_enable_sink(intel_dp); > >> > >> /* Enable PSR on the host */ > >> - intel_edp_psr_enable_source(intel_dp); > >> + if (IS_VALLEYVIEW(dev)) > >> + vlv_edp_psr_enable_source(intel_dp); > >> + else > >> + intel_edp_psr_enable_source(intel_dp); > >> + > >> + dev_priv->psr.active = true; > >> } > >> > >> void intel_edp_psr_enable(struct intel_dp *intel_dp) > >> { > >> - struct drm_device *dev = intel_dp_to_dev(intel_dp); > >> + if (!is_edp_psr(intel_dp)) > >> + return; > >> > >> - if (intel_edp_psr_match_conditions(intel_dp) && > >> - !intel_edp_is_psr_enabled(dev)) > >> + if (intel_edp_psr_match_conditions(intel_dp)) > >> intel_edp_psr_do_enable(intel_dp); > >> } > >> > >> +void vlv_edp_psr_disable(struct intel_dp *intel_dp) > >> +{ > >> + struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); > >> + struct drm_device *dev = intel_dig_port->base.base.dev; > >> + struct drm_i915_private *dev_priv = dev->dev_private; > >> + struct intel_crtc *intel_crtc = > >> + to_intel_crtc(intel_dig_port->base.base.crtc); > >> + uint32_t val = I915_READ(VLV_EDP_PSR_STATUS_CTL(intel_crtc->pipe)); > >> + > >> + if (!dev_priv->psr.setup_done) > >> + return; > >> + > >> + intel_edp_psr_inactivate(dev); > >> + > >> + if (val & VLV_EDP_PSR_IN_TRANS) > >> + udelay(250); > > > > Might we want a warning if the bit doesn't go down in expected time? > > Done. > > > > >> + > >> + val = I915_READ(VLV_EDP_PSR_CTL(intel_crtc->pipe)); > >> + val &= ~VLV_EDP_PSR_ACTIVE_ENTRY; > >> + val &= ~VLV_EDP_PSR_ENABLE; > >> + val &= ~VLV_EDP_PSR_MODE_MASK; > >> + I915_WRITE(VLV_EDP_PSR_CTL(intel_crtc->pipe), val); > >> +} > >> + > >> void intel_edp_psr_disable(struct intel_dp *intel_dp) > >> { > >> struct drm_device *dev = intel_dp_to_dev(intel_dp); > >> @@ -1817,28 +1920,66 @@ void intel_edp_psr_update(struct drm_device *dev) > >> { > >> struct intel_encoder *encoder; > >> struct intel_dp *intel_dp = NULL; > >> + struct intel_crtc *intel_crtc; > >> > >> list_for_each_entry(encoder, &dev->mode_config.encoder_list, base.head) > >> if (encoder->type == INTEL_OUTPUT_EDP) { > >> intel_dp = enc_to_intel_dp(&encoder->base); > >> > >> - if (!is_edp_psr(dev)) > >> + if (!is_edp_psr(intel_dp)) > >> return; > >> > >> - if (!intel_edp_psr_match_conditions(intel_dp)) > >> - intel_edp_psr_disable(intel_dp); > >> - else > >> + intel_crtc = to_intel_crtc(encoder->base.crtc); > >> + > >> + if (!intel_edp_psr_match_conditions(intel_dp)) { > >> + if (IS_VALLEYVIEW(dev)) > >> + vlv_edp_psr_disable(intel_dp); > >> + else > >> + intel_edp_psr_disable(intel_dp); > >> + } else > >> if (!intel_edp_is_psr_enabled(dev)) > >> intel_edp_psr_do_enable(intel_dp); > >> } > >> } > >> > >> +void intel_edp_psr_inactivate(struct drm_device *dev) > >> +{ > >> + struct drm_i915_private *dev_priv = dev->dev_private; > >> + struct intel_encoder *encoder; > >> + struct intel_crtc *intel_crtc; > >> + struct intel_dp *intel_dp = NULL; > >> + > >> + if (!dev_priv->psr.setup_done || !dev_priv->psr.active) > >> + return; > >> + > >> + list_for_each_entry(encoder, &dev->mode_config.encoder_list, base.head) > >> + if (encoder->type == INTEL_OUTPUT_EDP) { > >> + intel_dp = enc_to_intel_dp(&encoder->base); > >> + intel_crtc = to_intel_crtc(encoder->base.crtc); > >> + > >> + if (!vlv_edp_is_psr_enabled_on_pipe(dev, > >> + intel_crtc->pipe)) > >> + continue; > >> + > >> + dev_priv->psr.active = false; > >> + I915_WRITE(VLV_EDP_PSR_CTL(intel_crtc->pipe), > >> + VLV_EDP_PSR_RESET); > >> + /* WaClearPSRReset:vlv */ > >> + I915_WRITE(VLV_EDP_PSR_CTL(intel_crtc->pipe), 0); > >> + > >> + intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON); > >> + } > >> +} > >> + > >> static void intel_disable_dp(struct intel_encoder *encoder) > >> { > >> struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base); > >> enum port port = dp_to_dig_port(intel_dp)->port; > >> struct drm_device *dev = encoder->base.dev; > >> > >> + if (IS_VALLEYVIEW(dev)) > >> + vlv_edp_psr_disable(intel_dp); > >> + > >> /* Make sure the panel is off before trying to change the mode. But also > >> * ensure that we have vdd while we switch off the panel. */ > >> intel_edp_backlight_off(intel_dp); > >> @@ -1895,6 +2036,7 @@ static void vlv_enable_dp(struct intel_encoder *encoder) > >> struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base); > >> > >> intel_edp_backlight_on(intel_dp); > >> + intel_edp_psr_enable(intel_dp); > >> } > >> > >> static void g4x_pre_enable_dp(struct intel_encoder *encoder) > >> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > >> index 71c1371..82026ef 100644 > >> --- a/drivers/gpu/drm/i915/intel_drv.h > >> +++ b/drivers/gpu/drm/i915/intel_drv.h > >> @@ -748,6 +748,7 @@ void intel_edp_panel_off(struct intel_dp *intel_dp); > >> void intel_edp_psr_enable(struct intel_dp *intel_dp); > >> void intel_edp_psr_disable(struct intel_dp *intel_dp); > >> void intel_edp_psr_update(struct drm_device *dev); > >> +void intel_edp_psr_inactivate(struct drm_device *dev); > >> > >> > >> /* intel_dsi.c */ > >> -- > >> 1.8.1.2 > > > > -- > > Ville Syrjälä > > Intel OTC > > Thank you very much! > > -- > Rodrigo Vivi > Blog: http://blog.vivi.eng.br
On Wed, Jan 29, 2014 at 2:38 PM, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > On Wed, Jan 29, 2014 at 01:47:00PM -0200, Rodrigo Vivi wrote: >> On Wed, Jan 29, 2014 at 12:56 PM, Ville Syrjälä >> <ville.syrjala@linux.intel.com> wrote: >> > On Wed, Jan 29, 2014 at 10:47:54AM -0200, Rodrigo Vivi wrote: >> >> This patch adds PSR Support to Baytrail. >> >> >> >> Baytrail cannot easily detect screen updates and force PSR exit. >> >> So we inactivate it on {busy_ioctl, set_domain, sw_finish and mark_busy >> >> and update to enable it back on next display mark_idle. >> >> >> >> v2: Also inactivate PSR on cursor update. >> >> v3: Inactivate PSR on mark_busy, dset_domain and sw_finish_ioctl, and >> >> early on page flip besides avoid initializing inactive/active flag >> >> more than once. >> >> v4: Fix identation issues. >> >> v5: Rebase and add Baytrail per pipe support although leaving PIPE_B >> >> support disabled by for now since it isn't working properly yet. >> >> v6: Removing forgotten comment and useless clkgating definition. >> >> >> >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> >> >> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com> >> >> --- >> >> drivers/gpu/drm/i915/i915_debugfs.c | 36 ++++++- >> >> drivers/gpu/drm/i915/i915_drv.h | 4 +- >> >> drivers/gpu/drm/i915/i915_gem.c | 9 ++ >> >> drivers/gpu/drm/i915/i915_reg.h | 37 +++++++ >> >> drivers/gpu/drm/i915/intel_display.c | 15 ++- >> >> drivers/gpu/drm/i915/intel_dp.c | 204 +++++++++++++++++++++++++++++------ >> >> drivers/gpu/drm/i915/intel_drv.h | 1 + >> >> 7 files changed, 267 insertions(+), 39 deletions(-) >> >> >> >> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c >> >> index 4b852c6..c28de65 100644 >> >> --- a/drivers/gpu/drm/i915/i915_debugfs.c >> >> +++ b/drivers/gpu/drm/i915/i915_debugfs.c >> >> @@ -1900,6 +1900,8 @@ static int i915_edp_psr_status(struct seq_file *m, void *data) >> >> struct drm_device *dev = node->minor->dev; >> >> struct drm_i915_private *dev_priv = dev->dev_private; >> >> u32 psrperf = 0; >> >> + u32 statA = 0; >> >> + u32 statB = 0; >> >> bool enabled = false; >> >> >> >> intel_runtime_pm_get(dev_priv); >> >> @@ -1907,14 +1909,38 @@ static int i915_edp_psr_status(struct seq_file *m, void *data) >> >> seq_printf(m, "Sink_Support: %s\n", yesno(dev_priv->psr.sink_support)); >> >> seq_printf(m, "Source_OK: %s\n", yesno(dev_priv->psr.source_ok)); >> >> >> >> - enabled = HAS_PSR(dev) && >> >> - I915_READ(EDP_PSR_CTL(dev)) & EDP_PSR_ENABLE; >> >> - seq_printf(m, "Enabled: %s\n", yesno(enabled)); >> >> + if (HAS_PSR(dev)) { >> >> + if (IS_VALLEYVIEW(dev)) { >> >> + statA = I915_READ(VLV_EDP_PSR_STATUS_CTL(PIPE_A)) & >> >> + VLV_EDP_PSR_CURR_STATE_MASK; >> >> + statB = I915_READ(VLV_EDP_PSR_STATUS_CTL(PIPE_B)) & >> >> + VLV_EDP_PSR_CURR_STATE_MASK; >> >> + enabled = ((statA == VLV_EDP_PSR_ACTIVE_NORFB_UP) || >> >> + (statA == VLV_EDP_PSR_ACTIVE_SF_UPDATE) || >> >> + (statB == VLV_EDP_PSR_ACTIVE_NORFB_UP) || >> >> + (statB == VLV_EDP_PSR_ACTIVE_SF_UPDATE)); >> >> + } else >> >> + enabled = I915_READ(EDP_PSR_CTL(dev)) & EDP_PSR_ENABLE; >> >> + } >> >> + seq_printf(m, "Enabled: %s", yesno(enabled)); >> >> >> >> - if (HAS_PSR(dev)) >> >> + if (IS_VALLEYVIEW(dev)) { >> >> + if ((statA == VLV_EDP_PSR_ACTIVE_NORFB_UP) || >> >> + (statA == VLV_EDP_PSR_ACTIVE_SF_UPDATE)) >> >> + seq_puts(m, " pipe A"); >> >> + if ((statB == VLV_EDP_PSR_ACTIVE_NORFB_UP) || >> >> + (statB == VLV_EDP_PSR_ACTIVE_SF_UPDATE)) >> >> + seq_puts(m, " pipe B"); >> >> + } >> >> + >> >> + seq_puts(m, "\n"); >> >> + >> >> + /* VLV PSR has no kind of performance counter */ >> >> + if (HAS_PSR(dev) && !IS_VALLEYVIEW(dev)) { >> >> psrperf = I915_READ(EDP_PSR_PERF_CNT(dev)) & >> >> EDP_PSR_PERF_CNT_MASK; >> >> - seq_printf(m, "Performance_Counter: %u\n", psrperf); >> >> + seq_printf(m, "Performance_Counter: %u\n", psrperf); >> >> + } >> >> >> >> intel_runtime_pm_put(dev_priv); >> >> return 0; >> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h >> >> index 7c53d4d..34dee24 100644 >> >> --- a/drivers/gpu/drm/i915/i915_drv.h >> >> +++ b/drivers/gpu/drm/i915/i915_drv.h >> >> @@ -747,6 +747,7 @@ struct i915_psr { >> >> bool sink_support; >> >> bool source_ok; >> >> bool setup_done; >> >> + bool active; >> >> }; >> >> >> >> enum intel_pch { >> >> @@ -1866,7 +1867,8 @@ struct drm_i915_file_private { >> >> >> >> #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)) >> >> +#define HAS_PSR(dev) (IS_HASWELL(dev) || IS_BROADWELL(dev) || \ >> >> + IS_VALLEYVIEW(dev)) >> >> #define HAS_PC8(dev) (IS_HASWELL(dev)) /* XXX HSW:ULX */ >> >> #define HAS_RUNTIME_PM(dev) (IS_HASWELL(dev)) >> >> >> >> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c >> >> index 39770f7..01137fe 100644 >> >> --- a/drivers/gpu/drm/i915/i915_gem.c >> >> +++ b/drivers/gpu/drm/i915/i915_gem.c >> >> @@ -1256,6 +1256,9 @@ i915_gem_set_domain_ioctl(struct drm_device *dev, void *data, >> >> goto unlock; >> >> } >> >> >> >> + if (IS_VALLEYVIEW(dev)) >> >> + intel_edp_psr_inactivate(dev); >> >> + >> >> /* 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. >> >> @@ -1299,6 +1302,9 @@ i915_gem_sw_finish_ioctl(struct drm_device *dev, void *data, >> >> if (ret) >> >> return ret; >> >> >> >> + if (IS_VALLEYVIEW(dev)) >> >> + intel_edp_psr_inactivate(dev); >> >> + >> >> obj = to_intel_bo(drm_gem_object_lookup(dev, file, args->handle)); >> >> if (&obj->base == NULL) { >> >> ret = -ENOENT; >> >> @@ -4059,6 +4065,9 @@ i915_gem_busy_ioctl(struct drm_device *dev, void *data, >> >> if (ret) >> >> return ret; >> >> >> >> + if (IS_VALLEYVIEW(dev)) >> >> + intel_edp_psr_inactivate(dev); >> > >> > The locking for PSR seems to be as fubar as for FBC. Also the front >> > buffer tracking is missing, but I guess I need to make that work for FBC >> > first, and then we need to figure out how to tie it in w/ PSR. >> > >> >> agree... I'll wait your fbc work. >> >> >> + >> >> obj = to_intel_bo(drm_gem_object_lookup(dev, file, args->handle)); >> >> if (&obj->base == NULL) { >> >> ret = -ENOENT; >> >> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h >> >> index cbbaf26..2039d71 100644 >> >> --- a/drivers/gpu/drm/i915/i915_reg.h >> >> +++ b/drivers/gpu/drm/i915/i915_reg.h >> >> @@ -1968,6 +1968,43 @@ >> >> #define BCLRPAT(pipe) _PIPE(pipe, _BCLRPAT_A, _BCLRPAT_B) >> >> #define VSYNCSHIFT(trans) _TRANSCODER(trans, _VSYNCSHIFT_A, _VSYNCSHIFT_B) >> >> >> >> +/* VLV eDP PSR registers */ >> >> +#define _PSRCTLA (VLV_DISPLAY_BASE + 0x60090) >> >> +#define _PSRCTLB (VLV_DISPLAY_BASE + 0x61090) >> >> +#define VLV_EDP_PSR_ENABLE (1<<0) >> >> +#define VLV_EDP_PSR_RESET (1<<1) >> >> +#define VLV_EDP_PSR_MODE_MASK (7<<2) >> >> +#define VLV_EDP_PSR_MODE_HW_TIMER (1<<3) >> >> +#define VLV_EDP_PSR_MODE_SW_TIMER (1<<2) >> >> +#define VLV_EDP_PSR_SINGLE_FRAME_UPDATE (1<<7) >> >> +#define VLV_EDP_PSR_ACTIVE_ENTRY (1<<8) >> >> +#define VLV_EDP_PSR_SRC_TRANSMITTER_STATE (1<<9) >> >> +#define VLV_EDP_PSR_DBL_FRAME (1<<10) >> >> +#define VLV_EDP_PSR_FRAME_COUNT_MASK (0xff<<16) >> >> +#define VLV_EDP_PSR_IDLE_FRAME_SHIFT 16 >> >> +#define VLV_EDP_PSR_INT_TRANSITION (1<<24) >> >> +#define VLV_EDP_PSR_CTL(pipe) _PIPE(pipe, _PSRCTLA, _PSRCTLB) >> > >> > Can we just name the PSR registers like in the spec? Eg. just >> > VLV_PSRCTL(). Would make it easier to compare things w/ the spec. >> > >> >> Done. >> >> >> + >> >> +#define _VSCSDPA (VLV_DISPLAY_BASE + 0x600a0) >> >> +#define _VSCSDPB (VLV_DISPLAY_BASE + 0x610a0) >> >> +#define VLV_EDP_PSR_SDP_FREQ_MASK (3<<30) >> >> +#define VLV_EDP_PSR_SDP_FREQ_ONCE (1<<31) >> >> +#define VLV_EDP_PSR_SDP_FREQ_EVFRAME (1<<30) >> >> +#define VLV_EDP_VSC_SDP_REG(pipe) _PIPE(pipe, _VSCSDPA, _VSCSDPB) >> >> + >> >> +#define _PSRSTATA (VLV_DISPLAY_BASE + 0x60094) >> >> +#define _PSRSTATB (VLV_DISPLAY_BASE + 0x61094) >> >> +#define VLV_EDP_PSR_LAST_STATE_MASK (7<<3) >> >> +#define VLV_EDP_PSR_CURR_STATE_MASK 7 >> >> +#define VLV_EDP_PSR_DISABLED (0<<0) >> >> +#define VLV_EDP_PSR_INACTIVE (1<<0) >> >> +#define VLV_EDP_PSR_IN_TRANS_TO_ACTIVE (2<<0) >> >> +#define VLV_EDP_PSR_ACTIVE_NORFB_UP (3<<0) >> >> +#define VLV_EDP_PSR_ACTIVE_SF_UPDATE (4<<0) >> >> +#define VLV_EDP_PSR_EXIT (5<<0) >> >> +#define VLV_EDP_PSR_IN_TRANS (1<<7) >> >> +#define VLV_EDP_PSR_STATUS_CTL(pipe) _PIPE(pipe, _PSRSTATA, _PSRSTATB) >> >> + >> >> /* HSW+ eDP PSR registers */ >> >> #define EDP_PSR_BASE(dev) (IS_HASWELL(dev) ? 0x64800 : 0x6f800) >> >> #define EDP_PSR_CTL(dev) (EDP_PSR_BASE(dev) + 0) >> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >> >> index 1a9aa19..081c8e2 100644 >> >> --- a/drivers/gpu/drm/i915/intel_display.c >> >> +++ b/drivers/gpu/drm/i915/intel_display.c >> >> @@ -7501,6 +7501,9 @@ static void intel_crtc_update_cursor(struct drm_crtc *crtc, >> >> u32 base = 0, pos = 0; >> >> bool visible; >> >> >> >> + if (IS_VALLEYVIEW(dev)) >> >> + intel_edp_psr_inactivate(dev); >> >> + >> >> if (on) >> >> base = intel_crtc->cursor_addr; >> >> >> >> @@ -8228,16 +8231,20 @@ void intel_mark_idle(struct drm_device *dev) >> >> >> >> if (dev_priv->info->gen >= 6) >> >> gen6_rps_idle(dev->dev_private); >> >> + >> >> + if (IS_VALLEYVIEW(dev)) >> >> + intel_edp_psr_update(dev); >> >> } >> >> >> >> + >> >> void intel_mark_fb_busy(struct drm_i915_gem_object *obj, >> >> struct intel_ring_buffer *ring) >> >> { >> >> struct drm_device *dev = obj->base.dev; >> >> struct drm_crtc *crtc; >> >> >> >> - if (!i915.powersave) >> >> - return; >> >> + if (IS_VALLEYVIEW(dev)) >> >> + intel_edp_psr_inactivate(dev); >> >> >> >> list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) { >> >> if (!crtc->fb) >> >> @@ -8688,6 +8695,10 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc, >> > nav> if (work == NULL) >> >> sorry, I didn't get this.. >> what did you mean? > > Nothing. Just managed to misplace a few characters here I guess. I blame > vim :) > >> >> >> return -ENOMEM; >> >> >> >> + /* Inactivate PSR early in page flip */ >> >> + if (IS_VALLEYVIEW(dev)) >> >> + intel_edp_psr_inactivate(dev); >> >> + >> >> work->event = event; >> >> work->crtc = crtc; >> >> work->old_fb_obj = to_intel_framebuffer(old_fb)->obj; >> >> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c >> >> index 30d4350..e9a0ace 100644 >> >> --- a/drivers/gpu/drm/i915/intel_dp.c >> >> +++ b/drivers/gpu/drm/i915/intel_dp.c >> >> @@ -1578,21 +1578,35 @@ static void intel_dp_get_config(struct intel_encoder *encoder, >> >> } >> >> } >> >> >> >> -static bool is_edp_psr(struct drm_device *dev) >> >> +static bool is_edp_psr(struct intel_dp *intel_dp) >> >> +{ >> >> + return intel_dp->psr_dpcd[0] & DP_PSR_IS_SUPPORTED; >> >> +} >> >> + >> >> +static bool vlv_edp_is_psr_enabled_on_pipe(struct drm_device *dev, int pipe) >> >> { >> >> struct drm_i915_private *dev_priv = dev->dev_private; >> >> + uint32_t val; >> >> >> >> - return dev_priv->psr.sink_support; >> >> + val = I915_READ(VLV_EDP_PSR_STATUS_CTL(pipe)) & >> >> + VLV_EDP_PSR_CURR_STATE_MASK; >> >> + return (val == VLV_EDP_PSR_ACTIVE_NORFB_UP) || >> >> + (val == VLV_EDP_PSR_ACTIVE_SF_UPDATE); >> >> } >> >> >> >> static bool intel_edp_is_psr_enabled(struct drm_device *dev) >> >> { >> >> struct drm_i915_private *dev_priv = dev->dev_private; >> >> >> >> - if (!HAS_PSR(dev)) >> >> - return false; >> >> + if (HAS_PSR(dev)) { >> >> + if (IS_VALLEYVIEW(dev)) >> >> + return vlv_edp_is_psr_enabled_on_pipe(dev, PIPE_A) || >> >> + vlv_edp_is_psr_enabled_on_pipe(dev, PIPE_B); >> >> + else >> >> + return I915_READ(EDP_PSR_CTL(dev)) & EDP_PSR_ENABLE; >> >> + } >> >> >> >> - return I915_READ(EDP_PSR_CTL(dev)) & EDP_PSR_ENABLE; >> >> + return false; >> >> } >> >> >> >> static void intel_edp_psr_write_vsc(struct intel_dp *intel_dp, >> >> @@ -1626,28 +1640,49 @@ static void intel_edp_psr_write_vsc(struct intel_dp *intel_dp, >> >> >> >> static void intel_edp_psr_setup(struct intel_dp *intel_dp) >> >> { >> >> - struct drm_device *dev = intel_dp_to_dev(intel_dp); >> >> + struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); >> >> + struct drm_device *dev = intel_dig_port->base.base.dev; >> >> struct drm_i915_private *dev_priv = dev->dev_private; >> >> struct edp_vsc_psr psr_vsc; >> >> + uint32_t val; >> >> >> >> if (dev_priv->psr.setup_done) >> >> return; >> >> >> >> - /* Prepare VSC packet as per EDP 1.3 spec, Table 3.10 */ >> >> - memset(&psr_vsc, 0, sizeof(psr_vsc)); >> >> - psr_vsc.sdp_header.HB0 = 0; >> >> - psr_vsc.sdp_header.HB1 = 0x7; >> >> - psr_vsc.sdp_header.HB2 = 0x2; >> >> - psr_vsc.sdp_header.HB3 = 0x8; >> >> - intel_edp_psr_write_vsc(intel_dp, &psr_vsc); >> >> + if (IS_VALLEYVIEW(dev)) { >> >> + val = I915_READ(VLV_EDP_VSC_SDP_REG(PIPE_A)); >> >> + val &= ~VLV_EDP_PSR_SDP_FREQ_MASK; >> >> + val |= VLV_EDP_PSR_SDP_FREQ_EVFRAME; >> >> + I915_WRITE(VLV_EDP_VSC_SDP_REG(PIPE_A), val); >> >> + >> >> + val = I915_READ(VLV_EDP_VSC_SDP_REG(PIPE_B)); >> >> + val &= ~VLV_EDP_PSR_SDP_FREQ_MASK; >> >> + val |= VLV_EDP_PSR_SDP_FREQ_EVFRAME; >> >> + I915_WRITE(VLV_EDP_VSC_SDP_REG(PIPE_B), val); >> >> + } else { >> >> + /* Prepare VSC packet as per EDP 1.3 spec, Table 3.10 */ >> >> + memset(&psr_vsc, 0, sizeof(psr_vsc)); >> >> + psr_vsc.sdp_header.HB0 = 0; >> >> + psr_vsc.sdp_header.HB1 = 0x7; >> >> + psr_vsc.sdp_header.HB2 = 0x2; >> >> + psr_vsc.sdp_header.HB3 = 0x8; >> >> + intel_edp_psr_write_vsc(intel_dp, &psr_vsc); >> >> >> >> - /* Avoid continuous PSR exit by masking memup and hpd */ >> >> - I915_WRITE(EDP_PSR_DEBUG_CTL(dev), EDP_PSR_DEBUG_MASK_MEMUP | >> >> - EDP_PSR_DEBUG_MASK_HPD | EDP_PSR_DEBUG_MASK_LPSP); >> >> + /* Avoid continuous PSR exit by masking memup and hpd */ >> >> + I915_WRITE(EDP_PSR_DEBUG_CTL(dev), EDP_PSR_DEBUG_MASK_MEMUP | >> >> + EDP_PSR_DEBUG_MASK_HPD | EDP_PSR_DEBUG_MASK_LPSP); >> >> + } >> >> >> >> dev_priv->psr.setup_done = true; >> >> } >> >> >> >> +static void vlv_edp_psr_enable_sink(struct intel_dp *intel_dp) >> >> +{ >> >> + /* Enable PSR in sink */ >> >> + intel_dp_aux_native_write_1(intel_dp, DP_PSR_EN_CFG, >> >> + DP_PSR_ENABLE); >> > >> > Don't we want the same main-link poweroff logic as HSW? >> >> >> I can't remember why I'm not disabling main link here... So I did a >> quickly try now and failure on tests was huge... so let's keep it >> simple for now ;) > > I think you are disabling the main link currently. Or at least you're > telling the sink that main link is going to be turned off, and you also > leave the VLV_EDP_PSR_SRC_TRANSMITTER_STATE bit unset, so I think that > should make the main link turn off. Well, since the pipe and port remain > active, I don't know if the link actually turns off. > > I would assume that if you don't turn off the link, it would be easier > to keep things happy. A bit weird that you had the opposite result. > > BTW now that I look at the HSW code, it seems buggy. When the sink > doesn't require link training, we tell it that we're going to turn the > link off, otherwise we tell it link will be on. But then we actually do > the opposite in intel_edp_psr_enable_source(). afaI-can-remember I followed hsw-pm guide on this... but I might be wrong... I'll comeback to psr hsw later anyway than I verify that and fix if needed. > >> >> > Or maybe we >> > should just keep the main link on always as long as we can't enter >> > the low power states w/ DPLL/pipe/port off. >> > >> > Did you already figure out why that's not happening? Looking at the >> > PSRSTAT registers, my guess is that D0i1 is where we end up currently, >> > and that doesn't actually turn off anything but the planes (to stop >> > memory fetches). D0i2 would turn off everything. >> >> no idea.. :( > > Did you check the status bit BTW? Does it say D0i1 or D0i2 when you're > in PSR? D0i1 > >> >> > But I guess we should anyway do this in steps. First getting the >> > current stuff in, then trying to get into D0i2 state, and finally >> > getting the display power well turned off when in PSR. >> >> Agree. >> >> > >> > I think once we get to working on D0i2, we'll need to move the PSR >> > wakeup to happen from a workqueue since it essentially requires a >> > full modeset. Even now in your code it's somewhat questionable >> > since you're doing stuff like aux transfers while holding >> > struct_mutex. >> >> uhm... anything we should try now to improve? > > Hmm. We already seem to do some PSR setup while holding struct_mutex, > but we don't hold it always. Although I think w/ HSW we end up > protecting the PSR state w/ the modeset locks since we only fiddle > with it during modesets. So for HSW we should just move the PSR update > call in set_base outside struct_mutex. I'm planning to change it on HSW to allow a kind of inactivate as well... > > But then for VLV, you want to call it from various places that already > hold struct_mutex, so it's getting messy. Just moving it to a workqueue > would seem like the easiest option here, and then you could grab the > modeset locks in the work function. uhm... I'm not sure... I don't think we need to delay the psr_enable... the only issue I see here is double setup or psr-enable along with psr-reset (inactivate)... don't you think a simple mutex for psr_setup would solve in a simpler way? > >> >> > >> >> +} >> >> + >> >> static void intel_edp_psr_enable_sink(struct intel_dp *intel_dp) >> >> { >> >> struct drm_device *dev = intel_dp_to_dev(intel_dp); >> >> @@ -1678,6 +1713,28 @@ static void intel_edp_psr_enable_sink(struct intel_dp *intel_dp) >> >> (aux_clock_divider << DP_AUX_CH_CTL_BIT_CLOCK_2X_SHIFT)); >> >> } >> >> >> >> +static void vlv_edp_psr_enable_source(struct intel_dp *intel_dp) >> >> +{ >> >> + struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); >> >> + struct drm_device *dev = intel_dig_port->base.base.dev; >> >> + struct drm_i915_private *dev_priv = dev->dev_private; >> >> + struct intel_crtc *intel_crtc = >> >> + to_intel_crtc(intel_dig_port->base.base.crtc); >> >> + >> >> + uint32_t idle_frames = 1; >> >> + uint32_t val; >> >> + >> >> + val = I915_READ(VLV_EDP_PSR_CTL(intel_crtc->pipe)); >> > >> > I don't think we want to preserve anything here. We need to make sure >> > everything is initialized correctly rather than trusting some old junk >> > in the register. >> >> Done. >> >> > >> >> + val |= VLV_EDP_PSR_ENABLE; >> >> + val &= ~VLV_EDP_PSR_MODE_MASK; >> >> + >> >> + val |= VLV_EDP_PSR_MODE_HW_TIMER; >> >> + val &= ~VLV_EDP_PSR_FRAME_COUNT_MASK; >> >> + val |= idle_frames << EDP_PSR_IDLE_FRAME_SHIFT; >> >> + >> >> + I915_WRITE(VLV_EDP_PSR_CTL(intel_crtc->pipe), val); >> >> +} >> >> + >> >> static void intel_edp_psr_enable_source(struct intel_dp *intel_dp) >> >> { >> >> struct drm_device *dev = intel_dp_to_dev(intel_dp); >> >> @@ -1719,8 +1776,8 @@ static bool intel_edp_psr_match_conditions(struct intel_dp *intel_dp) >> >> return false; >> >> } >> >> >> >> - if ((intel_encoder->type != INTEL_OUTPUT_EDP) || >> >> - (dig_port->port != PORT_A)) { >> >> + if (HAS_DDI(dev) && ((intel_encoder->type != INTEL_OUTPUT_EDP) || >> >> + (dig_port->port != PORT_A))) { >> >> DRM_DEBUG_KMS("HSW ties PSR to DDI A (eDP)\n"); >> >> return false; >> >> } >> >> @@ -1765,37 +1822,83 @@ static bool intel_edp_psr_match_conditions(struct intel_dp *intel_dp) >> >> return false; >> >> } >> >> >> >> + /* Baytrail supports per-pipe PSR configuration, however PSR on >> >> + * PIPE_B isn't working properly. So let's keep it disabled for now. */ >> >> + if (IS_VALLEYVIEW(dev) && intel_crtc->pipe != PIPE_A) { >> >> + DRM_DEBUG_KMS("PSR on BYT isn't enabled on pipe B.\n"); >> >> + return false; >> >> + } >> >> + >> >> dev_priv->psr.source_ok = true; >> >> return true; >> >> } >> >> >> >> static void intel_edp_psr_do_enable(struct intel_dp *intel_dp) >> >> { >> >> - struct drm_device *dev = intel_dp_to_dev(intel_dp); >> >> + struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); >> >> + struct drm_device *dev = intel_dig_port->base.base.dev; >> >> + struct drm_i915_private *dev_priv = dev->dev_private; >> >> + struct intel_crtc *intel_crtc = >> >> + to_intel_crtc(intel_dig_port->base.base.crtc); >> >> >> >> - if (!intel_edp_psr_match_conditions(intel_dp) || >> >> - intel_edp_is_psr_enabled(dev)) >> >> - return; >> >> + if (IS_VALLEYVIEW(dev)) { >> >> + if (vlv_edp_is_psr_enabled_on_pipe(dev, intel_crtc->pipe)) >> >> + return; >> >> + } else >> >> + if (intel_edp_is_psr_enabled(dev)) >> >> + return; >> >> >> >> /* Setup PSR once */ >> >> intel_edp_psr_setup(intel_dp); >> >> >> >> /* Enable PSR on the panel */ >> >> - intel_edp_psr_enable_sink(intel_dp); >> >> + if (IS_VALLEYVIEW(dev)) >> >> + vlv_edp_psr_enable_sink(intel_dp); >> >> + else >> >> + intel_edp_psr_enable_sink(intel_dp); >> >> >> >> /* Enable PSR on the host */ >> >> - intel_edp_psr_enable_source(intel_dp); >> >> + if (IS_VALLEYVIEW(dev)) >> >> + vlv_edp_psr_enable_source(intel_dp); >> >> + else >> >> + intel_edp_psr_enable_source(intel_dp); >> >> + >> >> + dev_priv->psr.active = true; >> >> } >> >> >> >> void intel_edp_psr_enable(struct intel_dp *intel_dp) >> >> { >> >> - struct drm_device *dev = intel_dp_to_dev(intel_dp); >> >> + if (!is_edp_psr(intel_dp)) >> >> + return; >> >> >> >> - if (intel_edp_psr_match_conditions(intel_dp) && >> >> - !intel_edp_is_psr_enabled(dev)) >> >> + if (intel_edp_psr_match_conditions(intel_dp)) >> >> intel_edp_psr_do_enable(intel_dp); >> >> } >> >> >> >> +void vlv_edp_psr_disable(struct intel_dp *intel_dp) >> >> +{ >> >> + struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); >> >> + struct drm_device *dev = intel_dig_port->base.base.dev; >> >> + struct drm_i915_private *dev_priv = dev->dev_private; >> >> + struct intel_crtc *intel_crtc = >> >> + to_intel_crtc(intel_dig_port->base.base.crtc); >> >> + uint32_t val = I915_READ(VLV_EDP_PSR_STATUS_CTL(intel_crtc->pipe)); >> >> + >> >> + if (!dev_priv->psr.setup_done) >> >> + return; >> >> + >> >> + intel_edp_psr_inactivate(dev); >> >> + >> >> + if (val & VLV_EDP_PSR_IN_TRANS) >> >> + udelay(250); >> > >> > Might we want a warning if the bit doesn't go down in expected time? >> >> Done. >> >> > >> >> + >> >> + val = I915_READ(VLV_EDP_PSR_CTL(intel_crtc->pipe)); >> >> + val &= ~VLV_EDP_PSR_ACTIVE_ENTRY; >> >> + val &= ~VLV_EDP_PSR_ENABLE; >> >> + val &= ~VLV_EDP_PSR_MODE_MASK; >> >> + I915_WRITE(VLV_EDP_PSR_CTL(intel_crtc->pipe), val); >> >> +} >> >> + >> >> void intel_edp_psr_disable(struct intel_dp *intel_dp) >> >> { >> >> struct drm_device *dev = intel_dp_to_dev(intel_dp); >> >> @@ -1817,28 +1920,66 @@ void intel_edp_psr_update(struct drm_device *dev) >> >> { >> >> struct intel_encoder *encoder; >> >> struct intel_dp *intel_dp = NULL; >> >> + struct intel_crtc *intel_crtc; >> >> >> >> list_for_each_entry(encoder, &dev->mode_config.encoder_list, base.head) >> >> if (encoder->type == INTEL_OUTPUT_EDP) { >> >> intel_dp = enc_to_intel_dp(&encoder->base); >> >> >> >> - if (!is_edp_psr(dev)) >> >> + if (!is_edp_psr(intel_dp)) >> >> return; >> >> >> >> - if (!intel_edp_psr_match_conditions(intel_dp)) >> >> - intel_edp_psr_disable(intel_dp); >> >> - else >> >> + intel_crtc = to_intel_crtc(encoder->base.crtc); >> >> + >> >> + if (!intel_edp_psr_match_conditions(intel_dp)) { >> >> + if (IS_VALLEYVIEW(dev)) >> >> + vlv_edp_psr_disable(intel_dp); >> >> + else >> >> + intel_edp_psr_disable(intel_dp); >> >> + } else >> >> if (!intel_edp_is_psr_enabled(dev)) >> >> intel_edp_psr_do_enable(intel_dp); >> >> } >> >> } >> >> >> >> +void intel_edp_psr_inactivate(struct drm_device *dev) >> >> +{ >> >> + struct drm_i915_private *dev_priv = dev->dev_private; >> >> + struct intel_encoder *encoder; >> >> + struct intel_crtc *intel_crtc; >> >> + struct intel_dp *intel_dp = NULL; >> >> + >> >> + if (!dev_priv->psr.setup_done || !dev_priv->psr.active) >> >> + return; >> >> + >> >> + list_for_each_entry(encoder, &dev->mode_config.encoder_list, base.head) >> >> + if (encoder->type == INTEL_OUTPUT_EDP) { >> >> + intel_dp = enc_to_intel_dp(&encoder->base); >> >> + intel_crtc = to_intel_crtc(encoder->base.crtc); >> >> + >> >> + if (!vlv_edp_is_psr_enabled_on_pipe(dev, >> >> + intel_crtc->pipe)) >> >> + continue; >> >> + >> >> + dev_priv->psr.active = false; >> >> + I915_WRITE(VLV_EDP_PSR_CTL(intel_crtc->pipe), >> >> + VLV_EDP_PSR_RESET); >> >> + /* WaClearPSRReset:vlv */ >> >> + I915_WRITE(VLV_EDP_PSR_CTL(intel_crtc->pipe), 0); >> >> + >> >> + intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON); >> >> + } >> >> +} >> >> + >> >> static void intel_disable_dp(struct intel_encoder *encoder) >> >> { >> >> struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base); >> >> enum port port = dp_to_dig_port(intel_dp)->port; >> >> struct drm_device *dev = encoder->base.dev; >> >> >> >> + if (IS_VALLEYVIEW(dev)) >> >> + vlv_edp_psr_disable(intel_dp); >> >> + >> >> /* Make sure the panel is off before trying to change the mode. But also >> >> * ensure that we have vdd while we switch off the panel. */ >> >> intel_edp_backlight_off(intel_dp); >> >> @@ -1895,6 +2036,7 @@ static void vlv_enable_dp(struct intel_encoder *encoder) >> >> struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base); >> >> >> >> intel_edp_backlight_on(intel_dp); >> >> + intel_edp_psr_enable(intel_dp); >> >> } >> >> >> >> static void g4x_pre_enable_dp(struct intel_encoder *encoder) >> >> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h >> >> index 71c1371..82026ef 100644 >> >> --- a/drivers/gpu/drm/i915/intel_drv.h >> >> +++ b/drivers/gpu/drm/i915/intel_drv.h >> >> @@ -748,6 +748,7 @@ void intel_edp_panel_off(struct intel_dp *intel_dp); >> >> void intel_edp_psr_enable(struct intel_dp *intel_dp); >> >> void intel_edp_psr_disable(struct intel_dp *intel_dp); >> >> void intel_edp_psr_update(struct drm_device *dev); >> >> +void intel_edp_psr_inactivate(struct drm_device *dev); >> >> >> >> >> >> /* intel_dsi.c */ >> >> -- >> >> 1.8.1.2 >> > >> > -- >> > Ville Syrjälä >> > Intel OTC >> >> Thank you very much! >> >> -- >> Rodrigo Vivi >> Blog: http://blog.vivi.eng.br > > -- > Ville Syrjälä > Intel OTC
On Wed, Jan 29, 2014 at 03:48:21PM -0200, Rodrigo Vivi wrote: > On Wed, Jan 29, 2014 at 2:38 PM, Ville Syrjälä > <ville.syrjala@linux.intel.com> wrote: > > On Wed, Jan 29, 2014 at 01:47:00PM -0200, Rodrigo Vivi wrote: > >> On Wed, Jan 29, 2014 at 12:56 PM, Ville Syrjälä > >> <ville.syrjala@linux.intel.com> wrote: > >> > On Wed, Jan 29, 2014 at 10:47:54AM -0200, Rodrigo Vivi wrote: > >> >> This patch adds PSR Support to Baytrail. > >> >> > >> >> Baytrail cannot easily detect screen updates and force PSR exit. > >> >> So we inactivate it on {busy_ioctl, set_domain, sw_finish and mark_busy > >> >> and update to enable it back on next display mark_idle. > >> >> > >> >> v2: Also inactivate PSR on cursor update. > >> >> v3: Inactivate PSR on mark_busy, dset_domain and sw_finish_ioctl, and > >> >> early on page flip besides avoid initializing inactive/active flag > >> >> more than once. > >> >> v4: Fix identation issues. > >> >> v5: Rebase and add Baytrail per pipe support although leaving PIPE_B > >> >> support disabled by for now since it isn't working properly yet. > >> >> v6: Removing forgotten comment and useless clkgating definition. > >> >> > >> >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > >> >> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com> > >> >> --- > >> >> drivers/gpu/drm/i915/i915_debugfs.c | 36 ++++++- > >> >> drivers/gpu/drm/i915/i915_drv.h | 4 +- > >> >> drivers/gpu/drm/i915/i915_gem.c | 9 ++ > >> >> drivers/gpu/drm/i915/i915_reg.h | 37 +++++++ > >> >> drivers/gpu/drm/i915/intel_display.c | 15 ++- > >> >> drivers/gpu/drm/i915/intel_dp.c | 204 +++++++++++++++++++++++++++++------ > >> >> drivers/gpu/drm/i915/intel_drv.h | 1 + > >> >> 7 files changed, 267 insertions(+), 39 deletions(-) > >> >> > >> >> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c > >> >> index 4b852c6..c28de65 100644 > >> >> --- a/drivers/gpu/drm/i915/i915_debugfs.c > >> >> +++ b/drivers/gpu/drm/i915/i915_debugfs.c > >> >> @@ -1900,6 +1900,8 @@ static int i915_edp_psr_status(struct seq_file *m, void *data) > >> >> struct drm_device *dev = node->minor->dev; > >> >> struct drm_i915_private *dev_priv = dev->dev_private; > >> >> u32 psrperf = 0; > >> >> + u32 statA = 0; > >> >> + u32 statB = 0; > >> >> bool enabled = false; > >> >> > >> >> intel_runtime_pm_get(dev_priv); > >> >> @@ -1907,14 +1909,38 @@ static int i915_edp_psr_status(struct seq_file *m, void *data) > >> >> seq_printf(m, "Sink_Support: %s\n", yesno(dev_priv->psr.sink_support)); > >> >> seq_printf(m, "Source_OK: %s\n", yesno(dev_priv->psr.source_ok)); > >> >> > >> >> - enabled = HAS_PSR(dev) && > >> >> - I915_READ(EDP_PSR_CTL(dev)) & EDP_PSR_ENABLE; > >> >> - seq_printf(m, "Enabled: %s\n", yesno(enabled)); > >> >> + if (HAS_PSR(dev)) { > >> >> + if (IS_VALLEYVIEW(dev)) { > >> >> + statA = I915_READ(VLV_EDP_PSR_STATUS_CTL(PIPE_A)) & > >> >> + VLV_EDP_PSR_CURR_STATE_MASK; > >> >> + statB = I915_READ(VLV_EDP_PSR_STATUS_CTL(PIPE_B)) & > >> >> + VLV_EDP_PSR_CURR_STATE_MASK; > >> >> + enabled = ((statA == VLV_EDP_PSR_ACTIVE_NORFB_UP) || > >> >> + (statA == VLV_EDP_PSR_ACTIVE_SF_UPDATE) || > >> >> + (statB == VLV_EDP_PSR_ACTIVE_NORFB_UP) || > >> >> + (statB == VLV_EDP_PSR_ACTIVE_SF_UPDATE)); > >> >> + } else > >> >> + enabled = I915_READ(EDP_PSR_CTL(dev)) & EDP_PSR_ENABLE; > >> >> + } > >> >> + seq_printf(m, "Enabled: %s", yesno(enabled)); > >> >> > >> >> - if (HAS_PSR(dev)) > >> >> + if (IS_VALLEYVIEW(dev)) { > >> >> + if ((statA == VLV_EDP_PSR_ACTIVE_NORFB_UP) || > >> >> + (statA == VLV_EDP_PSR_ACTIVE_SF_UPDATE)) > >> >> + seq_puts(m, " pipe A"); > >> >> + if ((statB == VLV_EDP_PSR_ACTIVE_NORFB_UP) || > >> >> + (statB == VLV_EDP_PSR_ACTIVE_SF_UPDATE)) > >> >> + seq_puts(m, " pipe B"); > >> >> + } > >> >> + > >> >> + seq_puts(m, "\n"); > >> >> + > >> >> + /* VLV PSR has no kind of performance counter */ > >> >> + if (HAS_PSR(dev) && !IS_VALLEYVIEW(dev)) { > >> >> psrperf = I915_READ(EDP_PSR_PERF_CNT(dev)) & > >> >> EDP_PSR_PERF_CNT_MASK; > >> >> - seq_printf(m, "Performance_Counter: %u\n", psrperf); > >> >> + seq_printf(m, "Performance_Counter: %u\n", psrperf); > >> >> + } > >> >> > >> >> intel_runtime_pm_put(dev_priv); > >> >> return 0; > >> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > >> >> index 7c53d4d..34dee24 100644 > >> >> --- a/drivers/gpu/drm/i915/i915_drv.h > >> >> +++ b/drivers/gpu/drm/i915/i915_drv.h > >> >> @@ -747,6 +747,7 @@ struct i915_psr { > >> >> bool sink_support; > >> >> bool source_ok; > >> >> bool setup_done; > >> >> + bool active; > >> >> }; > >> >> > >> >> enum intel_pch { > >> >> @@ -1866,7 +1867,8 @@ struct drm_i915_file_private { > >> >> > >> >> #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)) > >> >> +#define HAS_PSR(dev) (IS_HASWELL(dev) || IS_BROADWELL(dev) || \ > >> >> + IS_VALLEYVIEW(dev)) > >> >> #define HAS_PC8(dev) (IS_HASWELL(dev)) /* XXX HSW:ULX */ > >> >> #define HAS_RUNTIME_PM(dev) (IS_HASWELL(dev)) > >> >> > >> >> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > >> >> index 39770f7..01137fe 100644 > >> >> --- a/drivers/gpu/drm/i915/i915_gem.c > >> >> +++ b/drivers/gpu/drm/i915/i915_gem.c > >> >> @@ -1256,6 +1256,9 @@ i915_gem_set_domain_ioctl(struct drm_device *dev, void *data, > >> >> goto unlock; > >> >> } > >> >> > >> >> + if (IS_VALLEYVIEW(dev)) > >> >> + intel_edp_psr_inactivate(dev); > >> >> + > >> >> /* 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. > >> >> @@ -1299,6 +1302,9 @@ i915_gem_sw_finish_ioctl(struct drm_device *dev, void *data, > >> >> if (ret) > >> >> return ret; > >> >> > >> >> + if (IS_VALLEYVIEW(dev)) > >> >> + intel_edp_psr_inactivate(dev); > >> >> + > >> >> obj = to_intel_bo(drm_gem_object_lookup(dev, file, args->handle)); > >> >> if (&obj->base == NULL) { > >> >> ret = -ENOENT; > >> >> @@ -4059,6 +4065,9 @@ i915_gem_busy_ioctl(struct drm_device *dev, void *data, > >> >> if (ret) > >> >> return ret; > >> >> > >> >> + if (IS_VALLEYVIEW(dev)) > >> >> + intel_edp_psr_inactivate(dev); > >> > > >> > The locking for PSR seems to be as fubar as for FBC. Also the front > >> > buffer tracking is missing, but I guess I need to make that work for FBC > >> > first, and then we need to figure out how to tie it in w/ PSR. > >> > > >> > >> agree... I'll wait your fbc work. > >> > >> >> + > >> >> obj = to_intel_bo(drm_gem_object_lookup(dev, file, args->handle)); > >> >> if (&obj->base == NULL) { > >> >> ret = -ENOENT; > >> >> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > >> >> index cbbaf26..2039d71 100644 > >> >> --- a/drivers/gpu/drm/i915/i915_reg.h > >> >> +++ b/drivers/gpu/drm/i915/i915_reg.h > >> >> @@ -1968,6 +1968,43 @@ > >> >> #define BCLRPAT(pipe) _PIPE(pipe, _BCLRPAT_A, _BCLRPAT_B) > >> >> #define VSYNCSHIFT(trans) _TRANSCODER(trans, _VSYNCSHIFT_A, _VSYNCSHIFT_B) > >> >> > >> >> +/* VLV eDP PSR registers */ > >> >> +#define _PSRCTLA (VLV_DISPLAY_BASE + 0x60090) > >> >> +#define _PSRCTLB (VLV_DISPLAY_BASE + 0x61090) > >> >> +#define VLV_EDP_PSR_ENABLE (1<<0) > >> >> +#define VLV_EDP_PSR_RESET (1<<1) > >> >> +#define VLV_EDP_PSR_MODE_MASK (7<<2) > >> >> +#define VLV_EDP_PSR_MODE_HW_TIMER (1<<3) > >> >> +#define VLV_EDP_PSR_MODE_SW_TIMER (1<<2) > >> >> +#define VLV_EDP_PSR_SINGLE_FRAME_UPDATE (1<<7) > >> >> +#define VLV_EDP_PSR_ACTIVE_ENTRY (1<<8) > >> >> +#define VLV_EDP_PSR_SRC_TRANSMITTER_STATE (1<<9) > >> >> +#define VLV_EDP_PSR_DBL_FRAME (1<<10) > >> >> +#define VLV_EDP_PSR_FRAME_COUNT_MASK (0xff<<16) > >> >> +#define VLV_EDP_PSR_IDLE_FRAME_SHIFT 16 > >> >> +#define VLV_EDP_PSR_INT_TRANSITION (1<<24) > >> >> +#define VLV_EDP_PSR_CTL(pipe) _PIPE(pipe, _PSRCTLA, _PSRCTLB) > >> > > >> > Can we just name the PSR registers like in the spec? Eg. just > >> > VLV_PSRCTL(). Would make it easier to compare things w/ the spec. > >> > > >> > >> Done. > >> > >> >> + > >> >> +#define _VSCSDPA (VLV_DISPLAY_BASE + 0x600a0) > >> >> +#define _VSCSDPB (VLV_DISPLAY_BASE + 0x610a0) > >> >> +#define VLV_EDP_PSR_SDP_FREQ_MASK (3<<30) > >> >> +#define VLV_EDP_PSR_SDP_FREQ_ONCE (1<<31) > >> >> +#define VLV_EDP_PSR_SDP_FREQ_EVFRAME (1<<30) > >> >> +#define VLV_EDP_VSC_SDP_REG(pipe) _PIPE(pipe, _VSCSDPA, _VSCSDPB) > >> >> + > >> >> +#define _PSRSTATA (VLV_DISPLAY_BASE + 0x60094) > >> >> +#define _PSRSTATB (VLV_DISPLAY_BASE + 0x61094) > >> >> +#define VLV_EDP_PSR_LAST_STATE_MASK (7<<3) > >> >> +#define VLV_EDP_PSR_CURR_STATE_MASK 7 > >> >> +#define VLV_EDP_PSR_DISABLED (0<<0) > >> >> +#define VLV_EDP_PSR_INACTIVE (1<<0) > >> >> +#define VLV_EDP_PSR_IN_TRANS_TO_ACTIVE (2<<0) > >> >> +#define VLV_EDP_PSR_ACTIVE_NORFB_UP (3<<0) > >> >> +#define VLV_EDP_PSR_ACTIVE_SF_UPDATE (4<<0) > >> >> +#define VLV_EDP_PSR_EXIT (5<<0) > >> >> +#define VLV_EDP_PSR_IN_TRANS (1<<7) > >> >> +#define VLV_EDP_PSR_STATUS_CTL(pipe) _PIPE(pipe, _PSRSTATA, _PSRSTATB) > >> >> + > >> >> /* HSW+ eDP PSR registers */ > >> >> #define EDP_PSR_BASE(dev) (IS_HASWELL(dev) ? 0x64800 : 0x6f800) > >> >> #define EDP_PSR_CTL(dev) (EDP_PSR_BASE(dev) + 0) > >> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > >> >> index 1a9aa19..081c8e2 100644 > >> >> --- a/drivers/gpu/drm/i915/intel_display.c > >> >> +++ b/drivers/gpu/drm/i915/intel_display.c > >> >> @@ -7501,6 +7501,9 @@ static void intel_crtc_update_cursor(struct drm_crtc *crtc, > >> >> u32 base = 0, pos = 0; > >> >> bool visible; > >> >> > >> >> + if (IS_VALLEYVIEW(dev)) > >> >> + intel_edp_psr_inactivate(dev); > >> >> + > >> >> if (on) > >> >> base = intel_crtc->cursor_addr; > >> >> > >> >> @@ -8228,16 +8231,20 @@ void intel_mark_idle(struct drm_device *dev) > >> >> > >> >> if (dev_priv->info->gen >= 6) > >> >> gen6_rps_idle(dev->dev_private); > >> >> + > >> >> + if (IS_VALLEYVIEW(dev)) > >> >> + intel_edp_psr_update(dev); > >> >> } > >> >> > >> >> + > >> >> void intel_mark_fb_busy(struct drm_i915_gem_object *obj, > >> >> struct intel_ring_buffer *ring) > >> >> { > >> >> struct drm_device *dev = obj->base.dev; > >> >> struct drm_crtc *crtc; > >> >> > >> >> - if (!i915.powersave) > >> >> - return; > >> >> + if (IS_VALLEYVIEW(dev)) > >> >> + intel_edp_psr_inactivate(dev); > >> >> > >> >> list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) { > >> >> if (!crtc->fb) > >> >> @@ -8688,6 +8695,10 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc, > >> > nav> if (work == NULL) > >> > >> sorry, I didn't get this.. > >> what did you mean? > > > > Nothing. Just managed to misplace a few characters here I guess. I blame > > vim :) > > > >> > >> >> return -ENOMEM; > >> >> > >> >> + /* Inactivate PSR early in page flip */ > >> >> + if (IS_VALLEYVIEW(dev)) > >> >> + intel_edp_psr_inactivate(dev); > >> >> + > >> >> work->event = event; > >> >> work->crtc = crtc; > >> >> work->old_fb_obj = to_intel_framebuffer(old_fb)->obj; > >> >> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > >> >> index 30d4350..e9a0ace 100644 > >> >> --- a/drivers/gpu/drm/i915/intel_dp.c > >> >> +++ b/drivers/gpu/drm/i915/intel_dp.c > >> >> @@ -1578,21 +1578,35 @@ static void intel_dp_get_config(struct intel_encoder *encoder, > >> >> } > >> >> } > >> >> > >> >> -static bool is_edp_psr(struct drm_device *dev) > >> >> +static bool is_edp_psr(struct intel_dp *intel_dp) > >> >> +{ > >> >> + return intel_dp->psr_dpcd[0] & DP_PSR_IS_SUPPORTED; > >> >> +} > >> >> + > >> >> +static bool vlv_edp_is_psr_enabled_on_pipe(struct drm_device *dev, int pipe) > >> >> { > >> >> struct drm_i915_private *dev_priv = dev->dev_private; > >> >> + uint32_t val; > >> >> > >> >> - return dev_priv->psr.sink_support; > >> >> + val = I915_READ(VLV_EDP_PSR_STATUS_CTL(pipe)) & > >> >> + VLV_EDP_PSR_CURR_STATE_MASK; > >> >> + return (val == VLV_EDP_PSR_ACTIVE_NORFB_UP) || > >> >> + (val == VLV_EDP_PSR_ACTIVE_SF_UPDATE); > >> >> } > >> >> > >> >> static bool intel_edp_is_psr_enabled(struct drm_device *dev) > >> >> { > >> >> struct drm_i915_private *dev_priv = dev->dev_private; > >> >> > >> >> - if (!HAS_PSR(dev)) > >> >> - return false; > >> >> + if (HAS_PSR(dev)) { > >> >> + if (IS_VALLEYVIEW(dev)) > >> >> + return vlv_edp_is_psr_enabled_on_pipe(dev, PIPE_A) || > >> >> + vlv_edp_is_psr_enabled_on_pipe(dev, PIPE_B); > >> >> + else > >> >> + return I915_READ(EDP_PSR_CTL(dev)) & EDP_PSR_ENABLE; > >> >> + } > >> >> > >> >> - return I915_READ(EDP_PSR_CTL(dev)) & EDP_PSR_ENABLE; > >> >> + return false; > >> >> } > >> >> > >> >> static void intel_edp_psr_write_vsc(struct intel_dp *intel_dp, > >> >> @@ -1626,28 +1640,49 @@ static void intel_edp_psr_write_vsc(struct intel_dp *intel_dp, > >> >> > >> >> static void intel_edp_psr_setup(struct intel_dp *intel_dp) > >> >> { > >> >> - struct drm_device *dev = intel_dp_to_dev(intel_dp); > >> >> + struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); > >> >> + struct drm_device *dev = intel_dig_port->base.base.dev; > >> >> struct drm_i915_private *dev_priv = dev->dev_private; > >> >> struct edp_vsc_psr psr_vsc; > >> >> + uint32_t val; > >> >> > >> >> if (dev_priv->psr.setup_done) > >> >> return; > >> >> > >> >> - /* Prepare VSC packet as per EDP 1.3 spec, Table 3.10 */ > >> >> - memset(&psr_vsc, 0, sizeof(psr_vsc)); > >> >> - psr_vsc.sdp_header.HB0 = 0; > >> >> - psr_vsc.sdp_header.HB1 = 0x7; > >> >> - psr_vsc.sdp_header.HB2 = 0x2; > >> >> - psr_vsc.sdp_header.HB3 = 0x8; > >> >> - intel_edp_psr_write_vsc(intel_dp, &psr_vsc); > >> >> + if (IS_VALLEYVIEW(dev)) { > >> >> + val = I915_READ(VLV_EDP_VSC_SDP_REG(PIPE_A)); > >> >> + val &= ~VLV_EDP_PSR_SDP_FREQ_MASK; > >> >> + val |= VLV_EDP_PSR_SDP_FREQ_EVFRAME; > >> >> + I915_WRITE(VLV_EDP_VSC_SDP_REG(PIPE_A), val); > >> >> + > >> >> + val = I915_READ(VLV_EDP_VSC_SDP_REG(PIPE_B)); > >> >> + val &= ~VLV_EDP_PSR_SDP_FREQ_MASK; > >> >> + val |= VLV_EDP_PSR_SDP_FREQ_EVFRAME; > >> >> + I915_WRITE(VLV_EDP_VSC_SDP_REG(PIPE_B), val); > >> >> + } else { > >> >> + /* Prepare VSC packet as per EDP 1.3 spec, Table 3.10 */ > >> >> + memset(&psr_vsc, 0, sizeof(psr_vsc)); > >> >> + psr_vsc.sdp_header.HB0 = 0; > >> >> + psr_vsc.sdp_header.HB1 = 0x7; > >> >> + psr_vsc.sdp_header.HB2 = 0x2; > >> >> + psr_vsc.sdp_header.HB3 = 0x8; > >> >> + intel_edp_psr_write_vsc(intel_dp, &psr_vsc); > >> >> > >> >> - /* Avoid continuous PSR exit by masking memup and hpd */ > >> >> - I915_WRITE(EDP_PSR_DEBUG_CTL(dev), EDP_PSR_DEBUG_MASK_MEMUP | > >> >> - EDP_PSR_DEBUG_MASK_HPD | EDP_PSR_DEBUG_MASK_LPSP); > >> >> + /* Avoid continuous PSR exit by masking memup and hpd */ > >> >> + I915_WRITE(EDP_PSR_DEBUG_CTL(dev), EDP_PSR_DEBUG_MASK_MEMUP | > >> >> + EDP_PSR_DEBUG_MASK_HPD | EDP_PSR_DEBUG_MASK_LPSP); > >> >> + } > >> >> > >> >> dev_priv->psr.setup_done = true; > >> >> } > >> >> > >> >> +static void vlv_edp_psr_enable_sink(struct intel_dp *intel_dp) > >> >> +{ > >> >> + /* Enable PSR in sink */ > >> >> + intel_dp_aux_native_write_1(intel_dp, DP_PSR_EN_CFG, > >> >> + DP_PSR_ENABLE); > >> > > >> > Don't we want the same main-link poweroff logic as HSW? > >> > >> > >> I can't remember why I'm not disabling main link here... So I did a > >> quickly try now and failure on tests was huge... so let's keep it > >> simple for now ;) > > > > I think you are disabling the main link currently. Or at least you're > > telling the sink that main link is going to be turned off, and you also > > leave the VLV_EDP_PSR_SRC_TRANSMITTER_STATE bit unset, so I think that > > should make the main link turn off. Well, since the pipe and port remain > > active, I don't know if the link actually turns off. > > > > I would assume that if you don't turn off the link, it would be easier > > to keep things happy. A bit weird that you had the opposite result. > > > > BTW now that I look at the HSW code, it seems buggy. When the sink > > doesn't require link training, we tell it that we're going to turn the > > link off, otherwise we tell it link will be on. But then we actually do > > the opposite in intel_edp_psr_enable_source(). > > afaI-can-remember I followed hsw-pm guide on this... but I might be wrong... > I'll comeback to psr hsw later anyway than I verify that and fix if needed. > > > > >> > >> > Or maybe we > >> > should just keep the main link on always as long as we can't enter > >> > the low power states w/ DPLL/pipe/port off. > >> > > >> > Did you already figure out why that's not happening? Looking at the > >> > PSRSTAT registers, my guess is that D0i1 is where we end up currently, > >> > and that doesn't actually turn off anything but the planes (to stop > >> > memory fetches). D0i2 would turn off everything. > >> > >> no idea.. :( > > > > Did you check the status bit BTW? Does it say D0i1 or D0i2 when you're > > in PSR? > > D0i1 OK, as I suspected then. > > > > >> > >> > But I guess we should anyway do this in steps. First getting the > >> > current stuff in, then trying to get into D0i2 state, and finally > >> > getting the display power well turned off when in PSR. > >> > >> Agree. > >> > >> > > >> > I think once we get to working on D0i2, we'll need to move the PSR > >> > wakeup to happen from a workqueue since it essentially requires a > >> > full modeset. Even now in your code it's somewhat questionable > >> > since you're doing stuff like aux transfers while holding > >> > struct_mutex. > >> > >> uhm... anything we should try now to improve? > > > > Hmm. We already seem to do some PSR setup while holding struct_mutex, > > but we don't hold it always. Although I think w/ HSW we end up > > protecting the PSR state w/ the modeset locks since we only fiddle > > with it during modesets. So for HSW we should just move the PSR update > > call in set_base outside struct_mutex. > > I'm planning to change it on HSW to allow a kind of inactivate as well... > > > > > But then for VLV, you want to call it from various places that already > > hold struct_mutex, so it's getting messy. Just moving it to a workqueue > > would seem like the easiest option here, and then you could grab the > > modeset locks in the work function. > > uhm... I'm not sure... I don't think we need to delay the > psr_enable... the only issue I see here is double setup or psr-enable > along with psr-reset (inactivate)... don't you think a simple mutex > for psr_setup would solve in a simpler way? intel_edp_psr_match_conditions() in its current definitely needs to be protected by crtc.mutex to not race w/ page flips and modesets. Currently you call it from the idle work, without any locks, so that's already a problem. And we can't simply grab modeset locks there since those are banned on the dev_priv->wq workqueue. One idea might be to call intel_edp_psr_match_conditions() only from places where we already have the crtc mutex. That's actually just the modeset code, since we can't change tiling via page flips, so page flips don't need to call it. Hmm. Oh we'd also need to call it from the sprite code too. Anyways, then based on the result we'd set some per-crtc flag to remind us whether PSR can be enabled for the pipe. Then when we're ready to re-enable PSR we can just check the flag. That could be protected by some psr mutex. Also we need to at least disable PSR from some places where we already hold struct_mutex, and holding that sucker needlessly is a bad idea. So offloading the PSR disable to a workqueue seems like the sane approach here since it already involves stuff like aux writes. So even if we have the psr mutex and flag thingy it would still need to be done from a workqueue.
On Wed, Jan 29, 2014 at 02:26:03PM +0000, Chris Wilson wrote: > On Wed, Jan 29, 2014 at 12:23:15PM -0200, Rodrigo Vivi wrote: > > ok, got it. > > > > So, the correct here is to remove inactivate from set_domain and add > > gem_bo_busy call on MMAP_GTT testcase? > > That would match what we should do today. It's just whether we take the > opportunity to define a consistent behaviour... Afaik we don't do a gem_bo_busy after gtt access, at least no everwhere. Which sucks, but meh. At least I've thought that the busy is just after gpu rendering. -Daniel
On Wed, Jan 29, 2014 at 3:56 PM, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > I think once we get to working on D0i2, we'll need to move the PSR > wakeup to happen from a workqueue since it essentially requires a > full modeset. Even now in your code it's somewhat questionable > since you're doing stuff like aux transfers while holding > struct_mutex. Imo we can fix this up later on by pushing it into a workqueue. It's not great that we block while hodling struct_mutex, but imo there's too many loose pieces around psr so I prefer we start by merging something pessimistic, but known to work properly. Then we can tune and improve with the knowledge that the crc testcase will catch regressions. -Daniel
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index 4b852c6..c28de65 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -1900,6 +1900,8 @@ static int i915_edp_psr_status(struct seq_file *m, void *data) struct drm_device *dev = node->minor->dev; struct drm_i915_private *dev_priv = dev->dev_private; u32 psrperf = 0; + u32 statA = 0; + u32 statB = 0; bool enabled = false; intel_runtime_pm_get(dev_priv); @@ -1907,14 +1909,38 @@ static int i915_edp_psr_status(struct seq_file *m, void *data) seq_printf(m, "Sink_Support: %s\n", yesno(dev_priv->psr.sink_support)); seq_printf(m, "Source_OK: %s\n", yesno(dev_priv->psr.source_ok)); - enabled = HAS_PSR(dev) && - I915_READ(EDP_PSR_CTL(dev)) & EDP_PSR_ENABLE; - seq_printf(m, "Enabled: %s\n", yesno(enabled)); + if (HAS_PSR(dev)) { + if (IS_VALLEYVIEW(dev)) { + statA = I915_READ(VLV_EDP_PSR_STATUS_CTL(PIPE_A)) & + VLV_EDP_PSR_CURR_STATE_MASK; + statB = I915_READ(VLV_EDP_PSR_STATUS_CTL(PIPE_B)) & + VLV_EDP_PSR_CURR_STATE_MASK; + enabled = ((statA == VLV_EDP_PSR_ACTIVE_NORFB_UP) || + (statA == VLV_EDP_PSR_ACTIVE_SF_UPDATE) || + (statB == VLV_EDP_PSR_ACTIVE_NORFB_UP) || + (statB == VLV_EDP_PSR_ACTIVE_SF_UPDATE)); + } else + enabled = I915_READ(EDP_PSR_CTL(dev)) & EDP_PSR_ENABLE; + } + seq_printf(m, "Enabled: %s", yesno(enabled)); - if (HAS_PSR(dev)) + if (IS_VALLEYVIEW(dev)) { + if ((statA == VLV_EDP_PSR_ACTIVE_NORFB_UP) || + (statA == VLV_EDP_PSR_ACTIVE_SF_UPDATE)) + seq_puts(m, " pipe A"); + if ((statB == VLV_EDP_PSR_ACTIVE_NORFB_UP) || + (statB == VLV_EDP_PSR_ACTIVE_SF_UPDATE)) + seq_puts(m, " pipe B"); + } + + seq_puts(m, "\n"); + + /* VLV PSR has no kind of performance counter */ + if (HAS_PSR(dev) && !IS_VALLEYVIEW(dev)) { psrperf = I915_READ(EDP_PSR_PERF_CNT(dev)) & EDP_PSR_PERF_CNT_MASK; - seq_printf(m, "Performance_Counter: %u\n", psrperf); + seq_printf(m, "Performance_Counter: %u\n", psrperf); + } intel_runtime_pm_put(dev_priv); return 0; diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 7c53d4d..34dee24 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -747,6 +747,7 @@ struct i915_psr { bool sink_support; bool source_ok; bool setup_done; + bool active; }; enum intel_pch { @@ -1866,7 +1867,8 @@ struct drm_i915_file_private { #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)) +#define HAS_PSR(dev) (IS_HASWELL(dev) || IS_BROADWELL(dev) || \ + IS_VALLEYVIEW(dev)) #define HAS_PC8(dev) (IS_HASWELL(dev)) /* XXX HSW:ULX */ #define HAS_RUNTIME_PM(dev) (IS_HASWELL(dev)) diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 39770f7..01137fe 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -1256,6 +1256,9 @@ i915_gem_set_domain_ioctl(struct drm_device *dev, void *data, goto unlock; } + if (IS_VALLEYVIEW(dev)) + intel_edp_psr_inactivate(dev); + /* 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. @@ -1299,6 +1302,9 @@ i915_gem_sw_finish_ioctl(struct drm_device *dev, void *data, if (ret) return ret; + if (IS_VALLEYVIEW(dev)) + intel_edp_psr_inactivate(dev); + obj = to_intel_bo(drm_gem_object_lookup(dev, file, args->handle)); if (&obj->base == NULL) { ret = -ENOENT; @@ -4059,6 +4065,9 @@ i915_gem_busy_ioctl(struct drm_device *dev, void *data, if (ret) return ret; + if (IS_VALLEYVIEW(dev)) + intel_edp_psr_inactivate(dev); + obj = to_intel_bo(drm_gem_object_lookup(dev, file, args->handle)); if (&obj->base == NULL) { ret = -ENOENT; diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index cbbaf26..2039d71 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -1968,6 +1968,43 @@ #define BCLRPAT(pipe) _PIPE(pipe, _BCLRPAT_A, _BCLRPAT_B) #define VSYNCSHIFT(trans) _TRANSCODER(trans, _VSYNCSHIFT_A, _VSYNCSHIFT_B) +/* VLV eDP PSR registers */ +#define _PSRCTLA (VLV_DISPLAY_BASE + 0x60090) +#define _PSRCTLB (VLV_DISPLAY_BASE + 0x61090) +#define VLV_EDP_PSR_ENABLE (1<<0) +#define VLV_EDP_PSR_RESET (1<<1) +#define VLV_EDP_PSR_MODE_MASK (7<<2) +#define VLV_EDP_PSR_MODE_HW_TIMER (1<<3) +#define VLV_EDP_PSR_MODE_SW_TIMER (1<<2) +#define VLV_EDP_PSR_SINGLE_FRAME_UPDATE (1<<7) +#define VLV_EDP_PSR_ACTIVE_ENTRY (1<<8) +#define VLV_EDP_PSR_SRC_TRANSMITTER_STATE (1<<9) +#define VLV_EDP_PSR_DBL_FRAME (1<<10) +#define VLV_EDP_PSR_FRAME_COUNT_MASK (0xff<<16) +#define VLV_EDP_PSR_IDLE_FRAME_SHIFT 16 +#define VLV_EDP_PSR_INT_TRANSITION (1<<24) +#define VLV_EDP_PSR_CTL(pipe) _PIPE(pipe, _PSRCTLA, _PSRCTLB) + +#define _VSCSDPA (VLV_DISPLAY_BASE + 0x600a0) +#define _VSCSDPB (VLV_DISPLAY_BASE + 0x610a0) +#define VLV_EDP_PSR_SDP_FREQ_MASK (3<<30) +#define VLV_EDP_PSR_SDP_FREQ_ONCE (1<<31) +#define VLV_EDP_PSR_SDP_FREQ_EVFRAME (1<<30) +#define VLV_EDP_VSC_SDP_REG(pipe) _PIPE(pipe, _VSCSDPA, _VSCSDPB) + +#define _PSRSTATA (VLV_DISPLAY_BASE + 0x60094) +#define _PSRSTATB (VLV_DISPLAY_BASE + 0x61094) +#define VLV_EDP_PSR_LAST_STATE_MASK (7<<3) +#define VLV_EDP_PSR_CURR_STATE_MASK 7 +#define VLV_EDP_PSR_DISABLED (0<<0) +#define VLV_EDP_PSR_INACTIVE (1<<0) +#define VLV_EDP_PSR_IN_TRANS_TO_ACTIVE (2<<0) +#define VLV_EDP_PSR_ACTIVE_NORFB_UP (3<<0) +#define VLV_EDP_PSR_ACTIVE_SF_UPDATE (4<<0) +#define VLV_EDP_PSR_EXIT (5<<0) +#define VLV_EDP_PSR_IN_TRANS (1<<7) +#define VLV_EDP_PSR_STATUS_CTL(pipe) _PIPE(pipe, _PSRSTATA, _PSRSTATB) + /* HSW+ eDP PSR registers */ #define EDP_PSR_BASE(dev) (IS_HASWELL(dev) ? 0x64800 : 0x6f800) #define EDP_PSR_CTL(dev) (EDP_PSR_BASE(dev) + 0) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 1a9aa19..081c8e2 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -7501,6 +7501,9 @@ static void intel_crtc_update_cursor(struct drm_crtc *crtc, u32 base = 0, pos = 0; bool visible; + if (IS_VALLEYVIEW(dev)) + intel_edp_psr_inactivate(dev); + if (on) base = intel_crtc->cursor_addr; @@ -8228,16 +8231,20 @@ void intel_mark_idle(struct drm_device *dev) if (dev_priv->info->gen >= 6) gen6_rps_idle(dev->dev_private); + + if (IS_VALLEYVIEW(dev)) + intel_edp_psr_update(dev); } + void intel_mark_fb_busy(struct drm_i915_gem_object *obj, struct intel_ring_buffer *ring) { struct drm_device *dev = obj->base.dev; struct drm_crtc *crtc; - if (!i915.powersave) - return; + if (IS_VALLEYVIEW(dev)) + intel_edp_psr_inactivate(dev); list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) { if (!crtc->fb) @@ -8688,6 +8695,10 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc, if (work == NULL) return -ENOMEM; + /* Inactivate PSR early in page flip */ + if (IS_VALLEYVIEW(dev)) + intel_edp_psr_inactivate(dev); + work->event = event; work->crtc = crtc; work->old_fb_obj = to_intel_framebuffer(old_fb)->obj; diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 30d4350..e9a0ace 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -1578,21 +1578,35 @@ static void intel_dp_get_config(struct intel_encoder *encoder, } } -static bool is_edp_psr(struct drm_device *dev) +static bool is_edp_psr(struct intel_dp *intel_dp) +{ + return intel_dp->psr_dpcd[0] & DP_PSR_IS_SUPPORTED; +} + +static bool vlv_edp_is_psr_enabled_on_pipe(struct drm_device *dev, int pipe) { struct drm_i915_private *dev_priv = dev->dev_private; + uint32_t val; - return dev_priv->psr.sink_support; + val = I915_READ(VLV_EDP_PSR_STATUS_CTL(pipe)) & + VLV_EDP_PSR_CURR_STATE_MASK; + return (val == VLV_EDP_PSR_ACTIVE_NORFB_UP) || + (val == VLV_EDP_PSR_ACTIVE_SF_UPDATE); } static bool intel_edp_is_psr_enabled(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; - if (!HAS_PSR(dev)) - return false; + if (HAS_PSR(dev)) { + if (IS_VALLEYVIEW(dev)) + return vlv_edp_is_psr_enabled_on_pipe(dev, PIPE_A) || + vlv_edp_is_psr_enabled_on_pipe(dev, PIPE_B); + else + return I915_READ(EDP_PSR_CTL(dev)) & EDP_PSR_ENABLE; + } - return I915_READ(EDP_PSR_CTL(dev)) & EDP_PSR_ENABLE; + return false; } static void intel_edp_psr_write_vsc(struct intel_dp *intel_dp, @@ -1626,28 +1640,49 @@ static void intel_edp_psr_write_vsc(struct intel_dp *intel_dp, static void intel_edp_psr_setup(struct intel_dp *intel_dp) { - struct drm_device *dev = intel_dp_to_dev(intel_dp); + struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); + struct drm_device *dev = intel_dig_port->base.base.dev; struct drm_i915_private *dev_priv = dev->dev_private; struct edp_vsc_psr psr_vsc; + uint32_t val; if (dev_priv->psr.setup_done) return; - /* Prepare VSC packet as per EDP 1.3 spec, Table 3.10 */ - memset(&psr_vsc, 0, sizeof(psr_vsc)); - psr_vsc.sdp_header.HB0 = 0; - psr_vsc.sdp_header.HB1 = 0x7; - psr_vsc.sdp_header.HB2 = 0x2; - psr_vsc.sdp_header.HB3 = 0x8; - intel_edp_psr_write_vsc(intel_dp, &psr_vsc); + if (IS_VALLEYVIEW(dev)) { + val = I915_READ(VLV_EDP_VSC_SDP_REG(PIPE_A)); + val &= ~VLV_EDP_PSR_SDP_FREQ_MASK; + val |= VLV_EDP_PSR_SDP_FREQ_EVFRAME; + I915_WRITE(VLV_EDP_VSC_SDP_REG(PIPE_A), val); + + val = I915_READ(VLV_EDP_VSC_SDP_REG(PIPE_B)); + val &= ~VLV_EDP_PSR_SDP_FREQ_MASK; + val |= VLV_EDP_PSR_SDP_FREQ_EVFRAME; + I915_WRITE(VLV_EDP_VSC_SDP_REG(PIPE_B), val); + } else { + /* Prepare VSC packet as per EDP 1.3 spec, Table 3.10 */ + memset(&psr_vsc, 0, sizeof(psr_vsc)); + psr_vsc.sdp_header.HB0 = 0; + psr_vsc.sdp_header.HB1 = 0x7; + psr_vsc.sdp_header.HB2 = 0x2; + psr_vsc.sdp_header.HB3 = 0x8; + intel_edp_psr_write_vsc(intel_dp, &psr_vsc); - /* Avoid continuous PSR exit by masking memup and hpd */ - I915_WRITE(EDP_PSR_DEBUG_CTL(dev), EDP_PSR_DEBUG_MASK_MEMUP | - EDP_PSR_DEBUG_MASK_HPD | EDP_PSR_DEBUG_MASK_LPSP); + /* Avoid continuous PSR exit by masking memup and hpd */ + I915_WRITE(EDP_PSR_DEBUG_CTL(dev), EDP_PSR_DEBUG_MASK_MEMUP | + EDP_PSR_DEBUG_MASK_HPD | EDP_PSR_DEBUG_MASK_LPSP); + } dev_priv->psr.setup_done = true; } +static void vlv_edp_psr_enable_sink(struct intel_dp *intel_dp) +{ + /* Enable PSR in sink */ + intel_dp_aux_native_write_1(intel_dp, DP_PSR_EN_CFG, + DP_PSR_ENABLE); +} + static void intel_edp_psr_enable_sink(struct intel_dp *intel_dp) { struct drm_device *dev = intel_dp_to_dev(intel_dp); @@ -1678,6 +1713,28 @@ static void intel_edp_psr_enable_sink(struct intel_dp *intel_dp) (aux_clock_divider << DP_AUX_CH_CTL_BIT_CLOCK_2X_SHIFT)); } +static void vlv_edp_psr_enable_source(struct intel_dp *intel_dp) +{ + struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); + struct drm_device *dev = intel_dig_port->base.base.dev; + struct drm_i915_private *dev_priv = dev->dev_private; + struct intel_crtc *intel_crtc = + to_intel_crtc(intel_dig_port->base.base.crtc); + + uint32_t idle_frames = 1; + uint32_t val; + + val = I915_READ(VLV_EDP_PSR_CTL(intel_crtc->pipe)); + val |= VLV_EDP_PSR_ENABLE; + val &= ~VLV_EDP_PSR_MODE_MASK; + + val |= VLV_EDP_PSR_MODE_HW_TIMER; + val &= ~VLV_EDP_PSR_FRAME_COUNT_MASK; + val |= idle_frames << EDP_PSR_IDLE_FRAME_SHIFT; + + I915_WRITE(VLV_EDP_PSR_CTL(intel_crtc->pipe), val); +} + static void intel_edp_psr_enable_source(struct intel_dp *intel_dp) { struct drm_device *dev = intel_dp_to_dev(intel_dp); @@ -1719,8 +1776,8 @@ static bool intel_edp_psr_match_conditions(struct intel_dp *intel_dp) return false; } - if ((intel_encoder->type != INTEL_OUTPUT_EDP) || - (dig_port->port != PORT_A)) { + if (HAS_DDI(dev) && ((intel_encoder->type != INTEL_OUTPUT_EDP) || + (dig_port->port != PORT_A))) { DRM_DEBUG_KMS("HSW ties PSR to DDI A (eDP)\n"); return false; } @@ -1765,37 +1822,83 @@ static bool intel_edp_psr_match_conditions(struct intel_dp *intel_dp) return false; } + /* Baytrail supports per-pipe PSR configuration, however PSR on + * PIPE_B isn't working properly. So let's keep it disabled for now. */ + if (IS_VALLEYVIEW(dev) && intel_crtc->pipe != PIPE_A) { + DRM_DEBUG_KMS("PSR on BYT isn't enabled on pipe B.\n"); + return false; + } + dev_priv->psr.source_ok = true; return true; } static void intel_edp_psr_do_enable(struct intel_dp *intel_dp) { - struct drm_device *dev = intel_dp_to_dev(intel_dp); + struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); + struct drm_device *dev = intel_dig_port->base.base.dev; + struct drm_i915_private *dev_priv = dev->dev_private; + struct intel_crtc *intel_crtc = + to_intel_crtc(intel_dig_port->base.base.crtc); - if (!intel_edp_psr_match_conditions(intel_dp) || - intel_edp_is_psr_enabled(dev)) - return; + if (IS_VALLEYVIEW(dev)) { + if (vlv_edp_is_psr_enabled_on_pipe(dev, intel_crtc->pipe)) + return; + } else + if (intel_edp_is_psr_enabled(dev)) + return; /* Setup PSR once */ intel_edp_psr_setup(intel_dp); /* Enable PSR on the panel */ - intel_edp_psr_enable_sink(intel_dp); + if (IS_VALLEYVIEW(dev)) + vlv_edp_psr_enable_sink(intel_dp); + else + intel_edp_psr_enable_sink(intel_dp); /* Enable PSR on the host */ - intel_edp_psr_enable_source(intel_dp); + if (IS_VALLEYVIEW(dev)) + vlv_edp_psr_enable_source(intel_dp); + else + intel_edp_psr_enable_source(intel_dp); + + dev_priv->psr.active = true; } void intel_edp_psr_enable(struct intel_dp *intel_dp) { - struct drm_device *dev = intel_dp_to_dev(intel_dp); + if (!is_edp_psr(intel_dp)) + return; - if (intel_edp_psr_match_conditions(intel_dp) && - !intel_edp_is_psr_enabled(dev)) + if (intel_edp_psr_match_conditions(intel_dp)) intel_edp_psr_do_enable(intel_dp); } +void vlv_edp_psr_disable(struct intel_dp *intel_dp) +{ + struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); + struct drm_device *dev = intel_dig_port->base.base.dev; + struct drm_i915_private *dev_priv = dev->dev_private; + struct intel_crtc *intel_crtc = + to_intel_crtc(intel_dig_port->base.base.crtc); + uint32_t val = I915_READ(VLV_EDP_PSR_STATUS_CTL(intel_crtc->pipe)); + + if (!dev_priv->psr.setup_done) + return; + + intel_edp_psr_inactivate(dev); + + if (val & VLV_EDP_PSR_IN_TRANS) + udelay(250); + + val = I915_READ(VLV_EDP_PSR_CTL(intel_crtc->pipe)); + val &= ~VLV_EDP_PSR_ACTIVE_ENTRY; + val &= ~VLV_EDP_PSR_ENABLE; + val &= ~VLV_EDP_PSR_MODE_MASK; + I915_WRITE(VLV_EDP_PSR_CTL(intel_crtc->pipe), val); +} + void intel_edp_psr_disable(struct intel_dp *intel_dp) { struct drm_device *dev = intel_dp_to_dev(intel_dp); @@ -1817,28 +1920,66 @@ void intel_edp_psr_update(struct drm_device *dev) { struct intel_encoder *encoder; struct intel_dp *intel_dp = NULL; + struct intel_crtc *intel_crtc; list_for_each_entry(encoder, &dev->mode_config.encoder_list, base.head) if (encoder->type == INTEL_OUTPUT_EDP) { intel_dp = enc_to_intel_dp(&encoder->base); - if (!is_edp_psr(dev)) + if (!is_edp_psr(intel_dp)) return; - if (!intel_edp_psr_match_conditions(intel_dp)) - intel_edp_psr_disable(intel_dp); - else + intel_crtc = to_intel_crtc(encoder->base.crtc); + + if (!intel_edp_psr_match_conditions(intel_dp)) { + if (IS_VALLEYVIEW(dev)) + vlv_edp_psr_disable(intel_dp); + else + intel_edp_psr_disable(intel_dp); + } else if (!intel_edp_is_psr_enabled(dev)) intel_edp_psr_do_enable(intel_dp); } } +void intel_edp_psr_inactivate(struct drm_device *dev) +{ + struct drm_i915_private *dev_priv = dev->dev_private; + struct intel_encoder *encoder; + struct intel_crtc *intel_crtc; + struct intel_dp *intel_dp = NULL; + + if (!dev_priv->psr.setup_done || !dev_priv->psr.active) + return; + + list_for_each_entry(encoder, &dev->mode_config.encoder_list, base.head) + if (encoder->type == INTEL_OUTPUT_EDP) { + intel_dp = enc_to_intel_dp(&encoder->base); + intel_crtc = to_intel_crtc(encoder->base.crtc); + + if (!vlv_edp_is_psr_enabled_on_pipe(dev, + intel_crtc->pipe)) + continue; + + dev_priv->psr.active = false; + I915_WRITE(VLV_EDP_PSR_CTL(intel_crtc->pipe), + VLV_EDP_PSR_RESET); + /* WaClearPSRReset:vlv */ + I915_WRITE(VLV_EDP_PSR_CTL(intel_crtc->pipe), 0); + + intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON); + } +} + static void intel_disable_dp(struct intel_encoder *encoder) { struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base); enum port port = dp_to_dig_port(intel_dp)->port; struct drm_device *dev = encoder->base.dev; + if (IS_VALLEYVIEW(dev)) + vlv_edp_psr_disable(intel_dp); + /* Make sure the panel is off before trying to change the mode. But also * ensure that we have vdd while we switch off the panel. */ intel_edp_backlight_off(intel_dp); @@ -1895,6 +2036,7 @@ static void vlv_enable_dp(struct intel_encoder *encoder) struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base); intel_edp_backlight_on(intel_dp); + intel_edp_psr_enable(intel_dp); } static void g4x_pre_enable_dp(struct intel_encoder *encoder) diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 71c1371..82026ef 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -748,6 +748,7 @@ void intel_edp_panel_off(struct intel_dp *intel_dp); void intel_edp_psr_enable(struct intel_dp *intel_dp); void intel_edp_psr_disable(struct intel_dp *intel_dp); void intel_edp_psr_update(struct drm_device *dev); +void intel_edp_psr_inactivate(struct drm_device *dev); /* intel_dsi.c */
This patch adds PSR Support to Baytrail. Baytrail cannot easily detect screen updates and force PSR exit. So we inactivate it on {busy_ioctl, set_domain, sw_finish and mark_busy and update to enable it back on next display mark_idle. v2: Also inactivate PSR on cursor update. v3: Inactivate PSR on mark_busy, dset_domain and sw_finish_ioctl, and early on page flip besides avoid initializing inactive/active flag more than once. v4: Fix identation issues. v5: Rebase and add Baytrail per pipe support although leaving PIPE_B support disabled by for now since it isn't working properly yet. v6: Removing forgotten comment and useless clkgating definition. Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com> --- drivers/gpu/drm/i915/i915_debugfs.c | 36 ++++++- drivers/gpu/drm/i915/i915_drv.h | 4 +- drivers/gpu/drm/i915/i915_gem.c | 9 ++ drivers/gpu/drm/i915/i915_reg.h | 37 +++++++ drivers/gpu/drm/i915/intel_display.c | 15 ++- drivers/gpu/drm/i915/intel_dp.c | 204 +++++++++++++++++++++++++++++------ drivers/gpu/drm/i915/intel_drv.h | 1 + 7 files changed, 267 insertions(+), 39 deletions(-)