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
>
>>
>> > 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
--
Rodrigo Vivi
Blog: http://blog.vivi.eng.br
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
