Re: [PATCH] drm/i915: Add Baytrail PSR Support.

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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





[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux