Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757056AbcJMVQz (ORCPT ); Thu, 13 Oct 2016 17:16:55 -0400 Received: from mga05.intel.com ([192.55.52.43]:31307 "EHLO mga05.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751393AbcJMVQe (ORCPT ); Thu, 13 Oct 2016 17:16:34 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.31,489,1473145200"; d="scan'208";a="19812114" Message-ID: <1476393325.2478.48.camel@intel.com> Subject: Re: [PATCH 09/10] drm/i915/gen9: Actually verify WM levels in verify_wm_state() From: Paulo Zanoni To: Lyude , intel-gfx@lists.freedesktop.org Cc: Maarten Lankhorst , Ville =?ISO-8859-1?Q?Syrj=E4l=E4?= , Daniel Vetter , Jani Nikula , David Airlie , dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org Date: Thu, 13 Oct 2016 18:15:25 -0300 In-Reply-To: <1475885497-6094-10-git-send-email-cpaul@redhat.com> References: <1475885497-6094-1-git-send-email-cpaul@redhat.com> <1475885497-6094-10-git-send-email-cpaul@redhat.com> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.20.5 (3.20.5-1.fc24) Mime-Version: 1.0 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6313 Lines: 192 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 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 > Cc: Maarten Lankhorst > Cc: Ville Syrjälä > Cc: Paulo Zanoni > --- >  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); >   } >   } >  }