Re: [PATCH 02/24] drm/i915: Add some more tracked state to intel_pipe_wm

[Daniel Vetter <daniel@ffwll.ch>]

On Mon, Apr 07, 2014 at 11:14:05AM -0300, Paulo Zanoni wrote:
> 2014-03-07 13:32 GMT-03:00  <ville.syrjala@linux.intel.com>:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > intel_pipe_wm will be used to track the state in different stages
> > of the watermark update process. For that we need to keep a bit
> > more state in intel_pipe_wm.
> >
> > We also need to separate the multi-pipe intel_wm_config computation
> > from ilk_compute_wm_parameters() as that one deals with the future
> > state, and we need the intel_wm_config to match the current hardware
> > state at the time we do the watermark merging for multiple pipes.
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Needs minor rebase, but looks correct.

Ok in my eyes this conflict looks a bit tricky, and since I lack the
insight of you two for the watermark code I'd prefer a rebased version.
-Daniel

> 
> <insert complaint about the fact that the watermarks code is pretty
> complex these days>
> 
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> > ---
> >  drivers/gpu/drm/i915/intel_drv.h |  3 ++
> >  drivers/gpu/drm/i915/intel_pm.c  | 64 +++++++++++++++++++++++++++-------------
> >  2 files changed, 46 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 8c9892d..f022a78 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -333,6 +333,9 @@ struct intel_pipe_wm {
> >         struct intel_wm_level wm[5];
> >         uint32_t linetime;
> >         bool fbc_wm_enabled;
> > +       bool pipe_enabled;
> > +       bool sprites_enabled;
> > +       bool sprites_scaled;
> >  };
> >
> >  struct intel_crtc {
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index 26c79ed..e0d1c8b 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -2105,38 +2105,52 @@ static void intel_setup_wm_latency(struct drm_device *dev)
> >  }
> >
> >  static void ilk_compute_wm_parameters(struct drm_crtc *crtc,
> > -                                     struct ilk_pipe_wm_parameters *p,
> > -                                     struct intel_wm_config *config)
> > +                                     struct ilk_pipe_wm_parameters *p)
> >  {
> >         struct drm_device *dev = crtc->dev;
> >         struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> >         enum pipe pipe = intel_crtc->pipe;
> >         struct drm_plane *plane;
> >
> > -       p->active = intel_crtc_active(crtc);
> > -       if (p->active) {
> > -               p->pipe_htotal = intel_crtc->config.adjusted_mode.crtc_htotal;
> > -               p->pixel_rate = ilk_pipe_pixel_rate(dev, crtc);
> > -               p->pri.bytes_per_pixel = crtc->fb->bits_per_pixel / 8;
> > -               p->cur.bytes_per_pixel = 4;
> > -               p->pri.horiz_pixels = intel_crtc->config.pipe_src_w;
> > -               p->cur.horiz_pixels = 64;
> > -               /* TODO: for now, assume primary and cursor planes are always enabled. */
> > -               p->pri.enabled = true;
> > -               p->cur.enabled = true;
> > -       }
> > +       if (!intel_crtc_active(crtc))
> > +               return;
> >
> > -       list_for_each_entry(crtc, &dev->mode_config.crtc_list, head)
> > -               config->num_pipes_active += intel_crtc_active(crtc);
> > +       p->active = true;
> > +       p->pipe_htotal = intel_crtc->config.adjusted_mode.crtc_htotal;
> > +       p->pixel_rate = ilk_pipe_pixel_rate(dev, crtc);
> > +       p->pri.bytes_per_pixel = crtc->fb->bits_per_pixel / 8;
> > +       p->cur.bytes_per_pixel = 4;
> > +       p->pri.horiz_pixels = intel_crtc->config.pipe_src_w;
> > +       p->cur.horiz_pixels = 64;
> > +       /* TODO: for now, assume primary and cursor planes are always enabled. */
> > +       p->pri.enabled = true;
> > +       p->cur.enabled = true;
> >
> >         list_for_each_entry(plane, &dev->mode_config.plane_list, head) {
> >                 struct intel_plane *intel_plane = to_intel_plane(plane);
> >
> > -               if (intel_plane->pipe == pipe)
> > +               if (intel_plane->pipe == pipe) {
> >                         p->spr = intel_plane->wm;
> > +                       break;
> > +               }
> > +       }
> > +}
> >
> > -               config->sprites_enabled |= intel_plane->wm.enabled;
> > -               config->sprites_scaled |= intel_plane->wm.scaled;
> > +static void ilk_compute_wm_config(struct drm_device *dev,
> > +                                 struct intel_wm_config *config)
> > +{
> > +       struct intel_crtc *intel_crtc;
> > +
> > +       /* Compute the currently _active_ config */
> > +       list_for_each_entry(intel_crtc, &dev->mode_config.crtc_list, base.head) {
> > +               const struct intel_pipe_wm *wm = &intel_crtc->wm.active;
> > +
> > +               if (!wm->pipe_enabled)
> > +                       continue;
> > +
> > +               config->sprites_enabled |= wm->sprites_enabled;
> > +               config->sprites_scaled |= wm->sprites_scaled;
> > +               config->num_pipes_active++;
> >         }
> >  }
> >
> > @@ -2159,6 +2173,10 @@ static bool intel_compute_pipe_wm(struct drm_crtc *crtc,
> >         /* LP0 watermarks always use 1/2 DDB partitioning */
> >         ilk_compute_wm_maximums(dev, 0, &config, INTEL_DDB_PART_1_2, &max);
> >
> > +       pipe_wm->pipe_enabled = params->active;
> > +       pipe_wm->sprites_enabled = params->spr.enabled;
> > +       pipe_wm->sprites_scaled = params->spr.scaled;
> > +
> >         /* ILK/SNB: LP2+ watermarks only w/o sprites */
> >         if (INTEL_INFO(dev)->gen <= 6 && params->spr.enabled)
> >                 max_level = 1;
> > @@ -2548,7 +2566,7 @@ static void ilk_update_wm(struct drm_crtc *crtc)
> >         struct intel_pipe_wm lp_wm_1_2 = {}, lp_wm_5_6 = {}, *best_lp_wm;
> >         struct intel_wm_config config = {};
> >
> > -       ilk_compute_wm_parameters(crtc, &params, &config);
> > +       ilk_compute_wm_parameters(crtc, &params);
> >
> >         intel_compute_pipe_wm(crtc, &params, &pipe_wm);
> >
> > @@ -2557,6 +2575,8 @@ static void ilk_update_wm(struct drm_crtc *crtc)
> >
> >         intel_crtc->wm.active = pipe_wm;
> >
> > +       ilk_compute_wm_config(dev, &config);
> > +
> >         ilk_compute_wm_maximums(dev, 1, &config, INTEL_DDB_PART_1_2, &max);
> >         ilk_wm_merge(dev, &config, &max, &lp_wm_1_2);
> >
> > @@ -2623,7 +2643,9 @@ static void ilk_pipe_wm_get_hw_state(struct drm_crtc *crtc)
> >         if (IS_HASWELL(dev) || IS_BROADWELL(dev))
> >                 hw->wm_linetime[pipe] = I915_READ(PIPE_WM_LINETIME(pipe));
> >
> > -       if (intel_crtc_active(crtc)) {
> > +       active->pipe_enabled = intel_crtc_active(crtc);
> > +
> > +       if (active->pipe_enabled) {
> >                 u32 tmp = hw->wm_pipe[pipe];
> >
> >                 /*
> > --
> > 1.8.3.2
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
> -- 
> Paulo Zanoni
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch