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@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx>
> >> >> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
> >> >> ---
> >> >> 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.
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
