Em Sex, 2016-10-07 Ã s 20:11 -0400, Lyude escreveu: > Thanks to Paulo Zanoni for indirectly pointing this out. > > Looks like we never actually added any code for checking whether or > not > we actually wrote watermark levels properly. Let's fix that.
Thanks for doing this! Reviewed-by: Paulo Zanoni <paulo.r.zanoni at intel.com> A check that would have prevented one of the bugs I solved would be "if plane is active, then level 0 must be enabled, and DDB partitioning size must be non-zero". I'll put this in my TODO list, but I won't complain if somebody does it first :) > > Signed-off-by: Lyude <cpaul at redhat.com> > Cc: Maarten Lankhorst <maarten.lankhorst at linux.intel.com> > Cc: Ville Syrjälä <ville.syrjala at linux.intel.com> > Cc: Paulo Zanoni <paulo.r.zanoni at intel.com> > --- >  drivers/gpu/drm/i915/intel_display.c | 100 > +++++++++++++++++++++++++++++------ >  1 file changed, 84 insertions(+), 16 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c > b/drivers/gpu/drm/i915/intel_display.c > index 39400a0..2c682bc 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -13444,30 +13444,66 @@ static void verify_wm_state(struct drm_crtc > *crtc, >  struct drm_device *dev = crtc->dev; >  struct drm_i915_private *dev_priv = to_i915(dev); >  struct skl_ddb_allocation hw_ddb, *sw_ddb; > - struct skl_ddb_entry *hw_entry, *sw_entry; > + struct skl_pipe_wm hw_wm, *sw_wm; > + struct skl_plane_wm *hw_plane_wm, *sw_plane_wm; > + struct skl_ddb_entry *hw_ddb_entry, *sw_ddb_entry; >  struct intel_crtc *intel_crtc = to_intel_crtc(crtc); >  const enum pipe pipe = intel_crtc->pipe; > - int plane; > + int plane, level, max_level = ilk_wm_max_level(dev); >  >  if (INTEL_INFO(dev)->gen < 9 || !new_state->active) >  return; >  > + skl_pipe_wm_get_hw_state(crtc, &hw_wm); > + sw_wm = &intel_crtc->wm.active.skl; > + >  skl_ddb_get_hw_state(dev_priv, &hw_ddb); >  sw_ddb = &dev_priv->wm.skl_hw.ddb; >  >  /* planes */ >  for_each_plane(dev_priv, pipe, plane) { > - hw_entry = &hw_ddb.plane[pipe][plane]; > - sw_entry = &sw_ddb->plane[pipe][plane]; > + hw_plane_wm = &hw_wm.planes[plane]; > + sw_plane_wm = &sw_wm->planes[plane]; >  > - if (skl_ddb_entry_equal(hw_entry, sw_entry)) > - continue; > + /* Watermarks */ > + for (level = 0; level <= max_level; level++) { > + if (skl_wm_level_equals(&hw_plane_wm- > >wm[level], > + &sw_plane_wm- > >wm[level])) > + continue; > + > + DRM_ERROR("mismatch in WM pipe %c plane %d > level %d (expected e=%d b=%d l=%d, got e=%d b=%d l=%d)\n", > +   pipe_name(pipe), plane + 1, level, > +   sw_plane_wm->wm[level].plane_en, > +   sw_plane_wm- > >wm[level].plane_res_b, > +   sw_plane_wm- > >wm[level].plane_res_l, > +   hw_plane_wm->wm[level].plane_en, > +   hw_plane_wm- > >wm[level].plane_res_b, > +   hw_plane_wm- > >wm[level].plane_res_l); > + } >  > - DRM_ERROR("mismatch in DDB state pipe %c plane %d " > -   "(expected (%u,%u), found (%u,%u))\n", > -   pipe_name(pipe), plane + 1, > -   sw_entry->start, sw_entry->end, > -   hw_entry->start, hw_entry->end); > + if (!skl_wm_level_equals(&hw_plane_wm->trans_wm, > +  &sw_plane_wm->trans_wm)) { > + DRM_ERROR("mismatch in trans WM pipe %c > plane %d (expected e=%d b=%d l=%d, got e=%d b=%d l=%d)\n", > +   pipe_name(pipe), plane + 1, > +   sw_plane_wm->trans_wm.plane_en, > +   sw_plane_wm->trans_wm.plane_res_b, > +   sw_plane_wm->trans_wm.plane_res_l, > +   hw_plane_wm->trans_wm.plane_en, > +   hw_plane_wm->trans_wm.plane_res_b, > +   hw_plane_wm- > >trans_wm.plane_res_l); > + } > + > + /* DDB */ > + hw_ddb_entry = &hw_ddb.plane[pipe][plane]; > + sw_ddb_entry = &sw_ddb->plane[pipe][plane]; > + > + if (!skl_ddb_entry_equal(hw_ddb_entry, > sw_ddb_entry)) { > + DRM_ERROR("mismatch in DDB state pipe %c > plane %d " > +   "(expected (%u,%u), found > (%u,%u))\n", > +   pipe_name(pipe), plane + 1, > +   sw_ddb_entry->start, sw_ddb_entry- > >end, > +   hw_ddb_entry->start, hw_ddb_entry- > >end); > + } >  } >  >  /* > @@ -13477,15 +13513,47 @@ static void verify_wm_state(struct drm_crtc > *crtc, >   * once the plane becomes visible, we can skip this check >   */ >  if (intel_crtc->cursor_addr) { > - hw_entry = &hw_ddb.plane[pipe][PLANE_CURSOR]; > - sw_entry = &sw_ddb->plane[pipe][PLANE_CURSOR]; > + hw_plane_wm = &hw_wm.planes[PLANE_CURSOR]; > + sw_plane_wm = &sw_wm->planes[PLANE_CURSOR]; > + > + /* Watermarks */ > + for (level = 0; level <= max_level; level++) { > + if (skl_wm_level_equals(&hw_plane_wm- > >wm[level], > + &sw_plane_wm- > >wm[level])) > + continue; > + > + DRM_ERROR("mismatch in WM pipe %c cursor > level %d (expected e=%d b=%d l=%d, got e=%d b=%d l=%d)\n", > +   pipe_name(pipe), level, > +   sw_plane_wm->wm[level].plane_en, > +   sw_plane_wm- > >wm[level].plane_res_b, > +   sw_plane_wm- > >wm[level].plane_res_l, > +   hw_plane_wm->wm[level].plane_en, > +   hw_plane_wm- > >wm[level].plane_res_b, > +   hw_plane_wm- > >wm[level].plane_res_l); > + } > + > + if (!skl_wm_level_equals(&hw_plane_wm->trans_wm, > +  &sw_plane_wm->trans_wm)) { > + DRM_ERROR("mismatch in trans WM pipe %c > cursor (expected e=%d b=%d l=%d, got e=%d b=%d l=%d)\n", > +   pipe_name(pipe), > +   sw_plane_wm->trans_wm.plane_en, > +   sw_plane_wm->trans_wm.plane_res_b, > +   sw_plane_wm->trans_wm.plane_res_l, > +   hw_plane_wm->trans_wm.plane_en, > +   hw_plane_wm->trans_wm.plane_res_b, > +   hw_plane_wm- > >trans_wm.plane_res_l); > + } > + > + /* DDB */ > + hw_ddb_entry = &hw_ddb.plane[pipe][PLANE_CURSOR]; > + sw_ddb_entry = &sw_ddb->plane[pipe][PLANE_CURSOR]; >  > - if (!skl_ddb_entry_equal(hw_entry, sw_entry)) { > + if (!skl_ddb_entry_equal(hw_ddb_entry, > sw_ddb_entry)) { >  DRM_ERROR("mismatch in DDB state pipe %c > cursor " >    "(expected (%u,%u), found > (%u,%u))\n", >    pipe_name(pipe), > -   sw_entry->start, sw_entry->end, > -   hw_entry->start, hw_entry->end); > +   sw_ddb_entry->start, sw_ddb_entry- > >end, > +   hw_ddb_entry->start, hw_ddb_entry- > >end); >  } >  } >  }