Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757269AbcJMVUU (ORCPT ); Thu, 13 Oct 2016 17:20:20 -0400 Received: from mga01.intel.com ([192.55.52.88]:36250 "EHLO mga01.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757113AbcJMVUK (ORCPT ); Thu, 13 Oct 2016 17:20:10 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.31,489,1473145200"; d="scan'208";a="1044375692" Message-ID: <1476393604.2478.51.camel@intel.com> Subject: Re: [Intel-gfx] [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: David Airlie , linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, Daniel Vetter Date: Thu, 13 Oct 2016 18:20:04 -0300 In-Reply-To: <1476393325.2478.48.camel@intel.com> References: <1475885497-6094-1-git-send-email-cpaul@redhat.com> <1475885497-6094-10-git-send-email-cpaul@redhat.com> <1476393325.2478.48.camel@intel.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: 7240 Lines: 238 Em Qui, 2016-10-13 às 18:15 -0300, Paulo Zanoni escreveu: > 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 :) Also, bikeshed: use %u instead of %d for the unsigned types (plane_res_b, plane_res_l). > > > > > > > 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); > >   } > >   } > >  } > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx