Re: [PATCH 28/40] drm/i915: Add cherryview_update_wm()

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Thu, Jul 31, 2014 at 05:57:33PM -0300, Paulo Zanoni wrote:
> 2014-06-27 20:04 GMT-03:00  <ville.syrjala@linux.intel.com>:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > CHV has a third pipe so we need to compute the watermarks for its
> > planes. Add cherryview_update_wm() to do just that.
> 
> Ok, so basically the only real difference between this code and VLV's
> code is when you enable CXSR: on VLV you just enable CXSR after the
> other WM registers are already written. I wonder if this is to prevent
> any intermediate situations where the previous WM values did not allow
> CXSR, so enabling it first would result in errors/underruns. On this
> case, the CHV function would need to do the same thing as VLV, right?
> Do you have any specific reason for keeping the CXSR code different on
> CHV?

I think the difference is just due to me copy pasting the VLV function
before Imre's cxsr fixes went in. I'll respin the patch based on the
latest VLV code.

> 
> Also, instead of adding a new function, you could probably just
> rewrite vlv_update_wm to use for_each_pipe() instead of the current
> method. You'd define plane_wm[num_pipes] arrays instead of one
> variable per pipe, then you would be able to use the same function for
> both VLV and CHV. Anyway, I don't think we should block your patch
> based on this suggestion, so if you just provide a good explanation
> for the CXSR question - or a new patch - I'll give a R-B tag.

I did consider it, but I didn't want to start refactoring too much in
this patch. We might be able to unify more of the gmch watermark code
using your suggestion, or even making it just recompute the watermarks
for the current pipe. But that's better left for another patch/series.

> 
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_pm.c | 77 ++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 76 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index cb0b4b4..346dced 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -1364,6 +1364,81 @@ static void valleyview_update_wm(struct drm_crtc *crtc)
> >                    (cursor_sr << DSPFW_CURSOR_SR_SHIFT));
> >  }
> >
> > +static void cherryview_update_wm(struct drm_crtc *crtc)
> > +{
> > +       struct drm_device *dev = crtc->dev;
> > +       static const int sr_latency_ns = 12000;
> > +       struct drm_i915_private *dev_priv = dev->dev_private;
> > +       int planea_wm, planeb_wm, planec_wm;
> > +       int cursora_wm, cursorb_wm, cursorc_wm;
> > +       int plane_sr, cursor_sr;
> > +       int ignore_plane_sr, ignore_cursor_sr;
> > +       unsigned int enabled = 0;
> > +
> > +       vlv_update_drain_latency(dev);
> > +
> > +       if (g4x_compute_wm0(dev, PIPE_A,
> > +                           &valleyview_wm_info, latency_ns,
> > +                           &valleyview_cursor_wm_info, latency_ns,
> > +                           &planea_wm, &cursora_wm))
> > +               enabled |= 1 << PIPE_A;
> > +
> > +       if (g4x_compute_wm0(dev, PIPE_B,
> > +                           &valleyview_wm_info, latency_ns,
> > +                           &valleyview_cursor_wm_info, latency_ns,
> > +                           &planeb_wm, &cursorb_wm))
> > +               enabled |= 1 << PIPE_B;
> > +
> > +       if (g4x_compute_wm0(dev, PIPE_C,
> > +                           &valleyview_wm_info, latency_ns,
> > +                           &valleyview_cursor_wm_info, latency_ns,
> > +                           &planec_wm, &cursorc_wm))
> > +               enabled |= 1 << PIPE_C;
> > +
> > +       if (single_plane_enabled(enabled) &&
> > +           g4x_compute_srwm(dev, ffs(enabled) - 1,
> > +                            sr_latency_ns,
> > +                            &valleyview_wm_info,
> > +                            &valleyview_cursor_wm_info,
> > +                            &plane_sr, &ignore_cursor_sr) &&
> > +           g4x_compute_srwm(dev, ffs(enabled) - 1,
> > +                            2*sr_latency_ns,
> > +                            &valleyview_wm_info,
> > +                            &valleyview_cursor_wm_info,
> > +                            &ignore_plane_sr, &cursor_sr)) {
> > +               I915_WRITE(FW_BLC_SELF_VLV, FW_CSPWRDWNEN);
> > +       } else {
> > +               I915_WRITE(FW_BLC_SELF_VLV,
> > +                          I915_READ(FW_BLC_SELF_VLV) & ~FW_CSPWRDWNEN);
> > +               plane_sr = cursor_sr = 0;
> > +       }
> > +
> > +       DRM_DEBUG_KMS("Setting FIFO watermarks - A: plane=%d, cursor=%d, "
> > +                     "B: plane=%d, cursor=%d, C: plane=%d, cursor=%d, "
> > +                     "SR: plane=%d, cursor=%d\n",
> > +                     planea_wm, cursora_wm,
> > +                     planeb_wm, cursorb_wm,
> > +                     planec_wm, cursorc_wm,
> > +                     plane_sr, cursor_sr);
> > +
> > +       I915_WRITE(DSPFW1,
> > +                  (plane_sr << DSPFW_SR_SHIFT) |
> > +                  (cursorb_wm << DSPFW_CURSORB_SHIFT) |
> > +                  (planeb_wm << DSPFW_PLANEB_SHIFT) |
> > +                  (planea_wm << DSPFW_PLANEA_SHIFT));
> > +       I915_WRITE(DSPFW2,
> > +                  (I915_READ(DSPFW2) & ~DSPFW_CURSORA_MASK) |
> > +                  (cursora_wm << DSPFW_CURSORA_SHIFT));
> > +       I915_WRITE(DSPFW3,
> > +                  (I915_READ(DSPFW3) & ~DSPFW_CURSOR_SR_MASK) |
> > +                  (cursor_sr << DSPFW_CURSOR_SR_SHIFT));
> > +       I915_WRITE(DSPFW9_CHV,
> > +                  (I915_READ(DSPFW9_CHV) & ~(DSPFW_PLANEC_MASK |
> > +                                             DSPFW_CURSORC_MASK)) |
> > +                  (planec_wm << DSPFW_PLANEC_SHIFT) |
> > +                  (cursorc_wm << DSPFW_CURSORC_SHIFT));
> > +}
> > +
> >  static void g4x_update_wm(struct drm_crtc *crtc)
> >  {
> >         struct drm_device *dev = crtc->dev;
> > @@ -7046,7 +7121,7 @@ void intel_init_pm(struct drm_device *dev)
> >                 else if (INTEL_INFO(dev)->gen == 8)
> >                         dev_priv->display.init_clock_gating = gen8_init_clock_gating;
> >         } else if (IS_CHERRYVIEW(dev)) {
> > -               dev_priv->display.update_wm = valleyview_update_wm;
> > +               dev_priv->display.update_wm = cherryview_update_wm;
> >                 dev_priv->display.init_clock_gating =
> >                         cherryview_init_clock_gating;
> >         } else if (IS_VALLEYVIEW(dev)) {
> > --
> > 1.8.5.5
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
> -- 
> Paulo Zanoni

-- 
Ville Syrjälä
Intel OTC