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

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

 



On Fri, Feb 07, 2014 at 04:05:26PM -0200, Rodrigo Vivi wrote:
> On Fri, Feb 7, 2014 at 3:24 PM, Ville Syrjälä
> <ville.syrjala@xxxxxxxxxxxxxxx> wrote:
> > On Wed, Feb 05, 2014 at 05:04:31PM -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.
> >> v7: Remove inactivate from set_domain. Chris warned this was semanticaly
> >>     wrong.
> >> v8: Accept Ville's suggestions: Use register's names matching spec and
> >>     warn if transition took longer than it should.
> >> v9: New version with delayed work to get PSR back. Disabling it on
> >>     set_domain but not rescheduing it back until next finish_page_flip.
> >>
> >> Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> >> 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      |   5 +-
> >>  drivers/gpu/drm/i915/i915_gem.c      |  12 ++
> >>  drivers/gpu/drm/i915/i915_reg.h      |  37 +++++
> >>  drivers/gpu/drm/i915/i915_suspend.c  |   2 +-
> >>  drivers/gpu/drm/i915/intel_display.c |  18 ++-
> >>  drivers/gpu/drm/i915/intel_dp.c      | 256 ++++++++++++++++++++++++++++++-----
> >>  drivers/gpu/drm/i915/intel_drv.h     |   1 +
> >>  8 files changed, 323 insertions(+), 44 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> >> index bc8707f..2949c48 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_PSRSTAT(PIPE_A)) &
> >> +                             VLV_EDP_PSR_CURR_STATE_MASK;
> >> +                     statB = I915_READ(VLV_PSRSTAT(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 21470be..87c346a 100644
> >> --- a/drivers/gpu/drm/i915/i915_drv.h
> >> +++ b/drivers/gpu/drm/i915/i915_drv.h
> >> @@ -747,7 +747,9 @@ struct i915_psr {
> >>       bool sink_support;
> >>       bool source_ok;
> >>       bool setup_done;
> >> +     bool active;
> >>       struct mutex lock;
> >> +     struct delayed_work work;
> >>  };
> >>
> >>  enum intel_pch {
> >> @@ -1867,7 +1869,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..5ef1380 100644
> >> --- a/drivers/gpu/drm/i915/i915_gem.c
> >> +++ b/drivers/gpu/drm/i915/i915_gem.c
> >> @@ -1256,6 +1256,14 @@ i915_gem_set_domain_ioctl(struct drm_device *dev, void *data,
> >>               goto unlock;
> >>       }
> >>
> >> +     /* Here we don't reschedule work to get PSR back because userspace
> >> +     * can set domain and take as much as time it wants to write it.
> >> +     * We only reschedule it back on next finish_page_flip. There is the
> >> +     * risk of PSR be inactive forever depending on how compositor works,
> >> +     * but this is the safest way to avoid loosing screen updates.
> >> +     */
> >> +     intel_edp_psr_inactivate(dev, false);
> >
> > This will still block until PSR is deactivated (aux communication and
> > all). That still seems like a bad idea to do while holding struct_mutex.
> 
> So, what do you suggest here?

Same as before. The disabling could be done from a workqueue.

> 
> >
> >> +
> >>       /* 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 +1307,8 @@ i915_gem_sw_finish_ioctl(struct drm_device *dev, void *data,
> >>       if (ret)
> >>               return ret;
> >>
> >> +     intel_edp_psr_inactivate(dev, true);
> >> +
> >>       obj = to_intel_bo(drm_gem_object_lookup(dev, file, args->handle));
> >>       if (&obj->base == NULL) {
> >>               ret = -ENOENT;
> >> @@ -4059,6 +4069,8 @@ i915_gem_busy_ioctl(struct drm_device *dev, void *data,
> >>       if (ret)
> >>               return ret;
> >>
> >> +     intel_edp_psr_inactivate(dev, true);
> >> +
> >>       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..a5815d0 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_PSRCTL(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_VSCSDP(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_PSRSTAT(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/i915_suspend.c b/drivers/gpu/drm/i915/i915_suspend.c
> >> index ffcba21..6b31020 100644
> >> --- a/drivers/gpu/drm/i915/i915_suspend.c
> >> +++ b/drivers/gpu/drm/i915/i915_suspend.c
> >> @@ -289,8 +289,8 @@ static void i915_restore_display(struct drm_device *dev)
> >>       }
> >>
> >>       /* Force a full PSR setup on resume */
> >> +     intel_edp_psr_inactivate(dev, false);
> >>       dev_priv->psr.setup_done = false;
> >> -     intel_edp_psr_update(dev);
> >>
> >>       /* only restore FBC info on the platform that supports FBC*/
> >>       intel_disable_fbc(dev);
> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >> index 1a9aa19..92d6df4 100644
> >> --- a/drivers/gpu/drm/i915/intel_display.c
> >> +++ b/drivers/gpu/drm/i915/intel_display.c
> >> @@ -4451,9 +4451,13 @@ static void intel_connector_check_state(struct intel_connector *connector)
> >>   * consider. */
> >>  void intel_connector_dpms(struct drm_connector *connector, int mode)
> >>  {
> >> +     struct drm_device *dev = connector->dev;
> >> +
> >>       /* All the simple cases only support two dpms states. */
> >> -     if (mode != DRM_MODE_DPMS_ON)
> >> +     if (mode != DRM_MODE_DPMS_ON) {
> >>               mode = DRM_MODE_DPMS_OFF;
> >> +             intel_edp_psr_inactivate(dev, false);
> >
> > Maybe check if this connector even has PSR enabled?
> 
> the psr_inactivate already does that by checking psr.setup_done and psr.active.

I meant that it will now disable PSR for all displays even if the
DPMS mode was changed for some other display. Well, I guess it's
somewhat rare to only do DPMS for a subset of active displays.

> 
> >
> >> +     }
> >>
> >>       if (mode == connector->dpms)
> >>               return;
> >> @@ -7501,6 +7505,8 @@ static void intel_crtc_update_cursor(struct drm_crtc *crtc,
> >>       u32 base = 0, pos = 0;
> >>       bool visible;
> >>
> >> +     intel_edp_psr_inactivate(dev, true);
> >
> > Don't need to inactivate unless the cursor is or was visible.
> 
> so, below next retur after check visible and cursor_visible should be
> enough, right?

Yeah that should be fine.

> >
> >> +
> >>       if (on)
> >>               base = intel_crtc->cursor_addr;
> >>
> >> @@ -8230,12 +8236,15 @@ void intel_mark_idle(struct drm_device *dev)
> >>               gen6_rps_idle(dev->dev_private);
> >>  }
> >>
> >> +
> >>  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;
> >>
> >> +     intel_edp_psr_inactivate(dev, true);
> >> +
> >>       if (!i915.powersave)
> >>               return;
> >>
> >> @@ -8336,6 +8345,10 @@ static void do_intel_finish_page_flip(struct drm_device *dev,
> >>       queue_work(dev_priv->wq, &work->work);
> >>
> >>       trace_i915_flip_complete(intel_crtc->plane, work->pending_flip_obj);
> >> +
> >> +     /* Get PSR back after set_domain inactivated it without rescheduling
> >> +     * it back. */
> >> +     schedule_delayed_work(&dev_priv->psr.work, msecs_to_jiffies(5000));
> >>  }
> >>
> >>  void intel_finish_page_flip(struct drm_device *dev, int pipe)
> >> @@ -8688,6 +8701,9 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
> >>       if (work == NULL)
> >>               return -ENOMEM;
> >>
> >> +     /* Inactivate PSR early in page flip */
> >> +     intel_edp_psr_inactivate(dev, true);
> >> +
> >>       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 80054bb..576c204 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_PSRSTAT(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,
> >> @@ -1624,32 +1638,60 @@ static void intel_edp_psr_write_vsc(struct intel_dp *intel_dp,
> >>       POSTING_READ(ctl_reg);
> >>  }
> >>
> >> +static void intel_edp_psr_work(struct work_struct *work)
> >> +{
> >> +     struct drm_i915_private *dev_priv =
> >> +             container_of(work, typeof(*dev_priv), psr.work.work);
> >> +     struct drm_device *dev = dev_priv->dev;
> >> +
> >> +     intel_edp_psr_update(dev);
> >> +}
> >> +
> >>  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;
> >>
> >> -     mutex_init(&dev_priv->psr.lock);
> >> -
> >> -     /* 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);
> >> +     if (IS_VALLEYVIEW(dev)) {
> >> +             val  = I915_READ(VLV_VSCSDP(PIPE_A));
> >> +             val &= ~VLV_EDP_PSR_SDP_FREQ_MASK;
> >> +             val |= VLV_EDP_PSR_SDP_FREQ_EVFRAME;
> >> +             I915_WRITE(VLV_VSCSDP(PIPE_A), val);
> >> +
> >> +             val  = I915_READ(VLV_VSCSDP(PIPE_B));
> >> +             val &= ~VLV_EDP_PSR_SDP_FREQ_MASK;
> >> +             val |= VLV_EDP_PSR_SDP_FREQ_EVFRAME;
> >> +             I915_WRITE(VLV_VSCSDP(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);
> >> +     }
> >>
> >>       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);
> >> @@ -1680,6 +1722,27 @@ 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 = 0;
> >> +
> >> +     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_PSRCTL(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);
> >> @@ -1721,8 +1784,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;
> >>       }
> >> @@ -1767,23 +1830,45 @@ 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;
> >>
> >>       /* 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)
> >> @@ -1791,16 +1876,43 @@ void intel_edp_psr_enable(struct intel_dp *intel_dp)
> >>       struct drm_device *dev = intel_dp_to_dev(intel_dp);
> >>       struct drm_i915_private *dev_priv = dev->dev_private;
> >>
> >> +     if (!is_edp_psr(intel_dp))
> >> +             return;
> >> +
> >>       /* Setup PSR once */
> >>       intel_edp_psr_setup(intel_dp);
> >>
> >>       mutex_lock(&dev_priv->psr.lock);
> >> -     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);
> >>       mutex_unlock(&dev_priv->psr.lock);
> >>  }
> >>
> >> +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;
> >> +
> >> +     if (!dev_priv->psr.setup_done)
> >> +             return;
> >> +
> >> +     intel_edp_psr_inactivate(dev, false);
> >> +
> >> +     if (wait_for((I915_READ(VLV_PSRSTAT(intel_crtc->pipe)) &
> >> +                   VLV_EDP_PSR_IN_TRANS) == 0, 250))
> >> +             WARN(1, "PSR transition took longer than expected\n");
> >> +
> >> +     val = I915_READ(VLV_PSRCTL(intel_crtc->pipe));
> >> +     val &= ~VLV_EDP_PSR_ACTIVE_ENTRY;
> >> +     val &= ~VLV_EDP_PSR_ENABLE;
> >> +     val &= ~VLV_EDP_PSR_MODE_MASK;
> >> +     I915_WRITE(VLV_PSRCTL(intel_crtc->pipe), val);
> >> +}
> >> +
> >>  void intel_edp_psr_disable(struct intel_dp *intel_dp)
> >>  {
> >>       struct drm_device *dev = intel_dp_to_dev(intel_dp);
> >> @@ -1823,6 +1935,7 @@ void intel_edp_psr_update(struct drm_device *dev)
> >>       struct drm_i915_private *dev_priv = dev->dev_private;
> >>       struct intel_encoder *encoder;
> >>       struct intel_dp *intel_dp = NULL;
> >> +     struct intel_crtc *intel_crtc;
> >>
> >>       if (!dev_priv->psr.setup_done)
> >>               return;
> >> @@ -1830,26 +1943,93 @@ void intel_edp_psr_update(struct drm_device *dev)
> >>       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))
> >> -                             return;
> >> +                     intel_crtc = to_intel_crtc(encoder->base.crtc);
> >>
> >>                       mutex_lock(&dev_priv->psr.lock);
> >> -                     if (!intel_edp_psr_match_conditions(intel_dp))
> >> -                             intel_edp_psr_disable(intel_dp);
> >> -                     else
> >> +                     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);
> >>                       mutex_unlock(&dev_priv->psr.lock);
> >>               }
> >>  }
> >>
> >> +void intel_edp_psr_do_inactivate(struct drm_device *dev)
> >> +{
> >> +     struct drm_i915_private *dev_priv = dev->dev_private;
> >> +     struct intel_connector *connector;
> >> +     struct intel_encoder *encoder;
> >> +     struct intel_crtc *intel_crtc;
> >> +     struct intel_dp *intel_dp = NULL;
> >> +
> >> +     list_for_each_entry(connector, &dev->mode_config.connector_list,
> >> +                         base.head) {
> >> +
> >> +             if (connector->base.dpms != DRM_MODE_DPMS_ON)
> >> +                     continue;
> >> +
> >> +             encoder = to_intel_encoder(connector->base.encoder);
> >> +             if (encoder->type == INTEL_OUTPUT_EDP) {
> >> +
> >> +                     intel_dp = enc_to_intel_dp(&encoder->base);
> >> +                     intel_crtc = to_intel_crtc(encoder->base.crtc);
> >
> > Still digging through the encoder and crtc pointers w/o holding any
> > modeset locks. Feels dangerous. intel_edp_psr_match_conditions() does
> > the same kind of stuff, and that too now gets called outside modeset
> > codepaths.
> 
> I don't see an easy way to solve this with current sturcture here
> since we need the pipe and intel_dp pointer.
> Maybe on the future rework putiing psr inside pipe_config and in a
> scruct with all needed info without require to interate anywhere.
> But on the current structure I don't see an easy solution, do you?

Well, if it was done from a workqueue anyway, it could just grab all
modeset locks. Would just need to make sure we wouldn't cause deadlocks
by doing that.

> 
> >> +
> >> +                     if (!vlv_edp_is_psr_enabled_on_pipe(dev,
> >> +                                                         intel_crtc->pipe))
> >> +                             continue;
> >> +
> >> +                     dev_priv->psr.active = false;
> >> +
> >> +                     I915_WRITE(VLV_PSRCTL(intel_crtc->pipe),
> >> +                                VLV_EDP_PSR_RESET);
> >> +                     /* WaClearPSRReset:vlv */
> >> +                     I915_WRITE(VLV_PSRCTL(intel_crtc->pipe), 0);
> >> +
> >> +                     intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
> >> +             }
> >> +     }
> >> +}
> >> +
> >> +void intel_edp_psr_inactivate(struct drm_device *dev, bool schedule_back)
> >> +{
> >> +     struct drm_i915_private *dev_priv = dev->dev_private;
> >> +
> >> +     if (!IS_VALLEYVIEW(dev))
> >> +             return;
> >> +
> >> +     if (!dev_priv->psr.setup_done)
> >> +             return;
> >> +
> >> +     /* If it was requested to not turn psr back delayed work must be
> >> +      * canceled even if it is already on inactivated state. */
> >> +     if (!schedule_back)
> >> +             cancel_delayed_work_sync(&dev_priv->psr.work);
> >> +
> >> +     if (!dev_priv->psr.active)
> >> +             return;
> >
> > What if psr gets re-activated just after you checked above?
> 
> Shouldn't be dangerus... psr.activate idea is to minimize I915_WRITE,
> mainly when calling busy ioctls in sequence...
> But I'm open to suggestions.

I think if this did happen and there are no more updates to the
screen for a while, this would leave PSR enabled, with potentially
stale screen contents.

> 
> >
> >> +
> >> +     cancel_delayed_work_sync(&dev_priv->psr.work);
> >> +
> >> +     intel_edp_psr_do_inactivate(dev);
> >> +
> >> +     if (schedule_back)
> >> +             schedule_delayed_work(&dev_priv->psr.work,
> >> +                                   msecs_to_jiffies(5000));
> >> +}
> >> +
> >>  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);
> >> @@ -1906,6 +2086,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)
> >> @@ -3846,8 +4027,11 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
> >>               BUG();
> >>       }
> >>
> >> -     if (is_edp(intel_dp))
> >> +     if (is_edp(intel_dp)) {
> >>               intel_dp_init_panel_power_sequencer(dev, intel_dp, &power_seq);
> >> +             INIT_DELAYED_WORK(&dev_priv->psr.work, intel_edp_psr_work);
> >> +             mutex_init(&dev_priv->psr.lock);
> >
> > Initializing global state from a per-connector init function seems a bit
> > weird.
> 
> I thought it as well, but didn't find another place.
> My first attempt was at psr_setup, but it is called again after
> suspend/resume and it was breaking things up.
> suggestions?

Some modeset init function sounds like the right place.

> 
> >
> >> +     }
> >>
> >>       error = intel_dp_i2c_init(intel_dp, intel_connector, name);
> >>       WARN(error, "intel_dp_i2c_init failed with error %d for port %c\n",
> >> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> >> index 71c1371..d8103f9 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, bool schedule_back);
> >>
> >>
> >>  /* intel_dsi.c */
> >> --
> >> 1.8.1.2
> >
> > --
> > Ville Syrjälä
> > Intel OTC
> 
> Hi Ville, thank you very much for your comments. Coding is improving a
> lot along with stability here.
> Please help me to get this structure merged than we rework all psr
> later to avoid that loop over connectors and encoders....
> 
> Thanks,
> 
> -- 
> Rodrigo Vivi
> Blog: http://blog.vivi.eng.br

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
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