Message ID | 1475602652-17326-1-git-send-email-paulo.r.zanoni@intel.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Em Ter, 2016-10-04 às 14:37 -0300, Paulo Zanoni escreveu: > With the previous code we were only recomputing the DDB partitioning > for the CRTCs included in the atomic commit, so any other active > CRTCs > would end up having their DDB registers zeroed. In this patch we make > sure that the computed state starts as a copy of the current > partitioning, and then we only zero the DDBs that we're actually > going to recompute. > > How to reproduce the bug: > 1 - Enable the primary plane on pipe A > 2 - Enable the primary plane on pipe B > 3 - Enable the sprite plane on pipe A Forgot to mention: sprite or cursor. > > Step 3 will zero the DDB partitioning for pipe B since it's not > included in the commit that enabled the sprite for pipe A. > > I expect this to fix many FIFO underrun problems on gen9+. > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=96226 > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=96828 > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97450 > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97596 > Bugzilla: https://www.phoronix.com/scan.php?page=news_item&px=Intel-S > kylake-Multi-Screen-Woes > Cc: stable@vger.kernel.org > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > --- > drivers/gpu/drm/i915/intel_pm.c | 12 ++++++++++-- > 1 file changed, 10 insertions(+), 2 deletions(-) > > I still have to confirm whether this closes the above bugs, but it > certainly fixes the problem I described. > > diff --git a/drivers/gpu/drm/i915/intel_pm.c > b/drivers/gpu/drm/i915/intel_pm.c > index 425544b..0c2e252 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -3362,13 +3362,15 @@ skl_allocate_pipe_ddb(struct intel_crtc_state > *cstate, > int num_active; > int id, i; > > + /* Clear the partitioning for disabled planes. */ > + memset(ddb->plane[pipe], 0, sizeof(ddb->plane[pipe])); > + memset(ddb->y_plane[pipe], 0, sizeof(ddb->y_plane[pipe])); > + > if (WARN_ON(!state)) > return 0; > > if (!cstate->base.active) { > ddb->pipe[pipe].start = ddb->pipe[pipe].end = 0; > - memset(ddb->plane[pipe], 0, sizeof(ddb- > >plane[pipe])); > - memset(ddb->y_plane[pipe], 0, sizeof(ddb- > >y_plane[pipe])); > return 0; > } > > @@ -4054,6 +4056,12 @@ skl_compute_ddb(struct drm_atomic_state > *state) > intel_state->wm_results.dirty_pipes = ~0; > } > > + /* > + * We're not recomputing for the pipes not included in the > commit, so > + * make sure we start with the current state. > + */ > + memcpy(ddb, &dev_priv->wm.skl_hw.ddb, sizeof(*ddb)); > + > for_each_intel_crtc_mask(dev, intel_crtc, realloc_pipes) { > struct intel_crtc_state *cstate; >
Op 04-10-16 om 19:44 schreef Paulo Zanoni: > Em Ter, 2016-10-04 às 14:37 -0300, Paulo Zanoni escreveu: >> With the previous code we were only recomputing the DDB partitioning >> for the CRTCs included in the atomic commit, so any other active >> CRTCs >> would end up having their DDB registers zeroed. In this patch we make >> sure that the computed state starts as a copy of the current >> partitioning, and then we only zero the DDBs that we're actually >> going to recompute. >> >> How to reproduce the bug: >> 1 - Enable the primary plane on pipe A >> 2 - Enable the primary plane on pipe B >> 3 - Enable the sprite plane on pipe A > Forgot to mention: sprite or cursor. Testcase: kms_cursor_legacy.cursorA-vs-flipB-atomic-transitions Fails with: (kms_cursor_legacy:1962) igt-kms-CRITICAL: Last errno: 22, Invalid argument (kms_cursor_legacy:1962) igt-kms-CRITICAL: error: -22 != 0 The test succeeds with this patch applied, but it still has FIFO underruns. Oh well it's progress..
Sorry about how long this took! Anyway, finally got to testing it Reviewed-by: Lyude <cpaul@redhat.com> On Tue, 2016-10-04 at 14:37 -0300, Paulo Zanoni wrote: > With the previous code we were only recomputing the DDB partitioning > for the CRTCs included in the atomic commit, so any other active > CRTCs > would end up having their DDB registers zeroed. In this patch we make > sure that the computed state starts as a copy of the current > partitioning, and then we only zero the DDBs that we're actually > going to recompute. > > How to reproduce the bug: > 1 - Enable the primary plane on pipe A > 2 - Enable the primary plane on pipe B > 3 - Enable the sprite plane on pipe A > > Step 3 will zero the DDB partitioning for pipe B since it's not > included in the commit that enabled the sprite for pipe A. > > I expect this to fix many FIFO underrun problems on gen9+. > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=96226 > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=96828 > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97450 > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97596 > Bugzilla: https://www.phoronix.com/scan.php?page=news_item&px=Intel-S > kylake-Multi-Screen-Woes > Cc: stable@vger.kernel.org > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > --- > drivers/gpu/drm/i915/intel_pm.c | 12 ++++++++++-- > 1 file changed, 10 insertions(+), 2 deletions(-) > > I still have to confirm whether this closes the above bugs, but it > certainly fixes the problem I described. > > diff --git a/drivers/gpu/drm/i915/intel_pm.c > b/drivers/gpu/drm/i915/intel_pm.c > index 425544b..0c2e252 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -3362,13 +3362,15 @@ skl_allocate_pipe_ddb(struct intel_crtc_state > *cstate, > int num_active; > int id, i; > > + /* Clear the partitioning for disabled planes. */ > + memset(ddb->plane[pipe], 0, sizeof(ddb->plane[pipe])); > + memset(ddb->y_plane[pipe], 0, sizeof(ddb->y_plane[pipe])); > + > if (WARN_ON(!state)) > return 0; > > if (!cstate->base.active) { > ddb->pipe[pipe].start = ddb->pipe[pipe].end = 0; > - memset(ddb->plane[pipe], 0, sizeof(ddb- > >plane[pipe])); > - memset(ddb->y_plane[pipe], 0, sizeof(ddb- > >y_plane[pipe])); > return 0; > } > > @@ -4054,6 +4056,12 @@ skl_compute_ddb(struct drm_atomic_state > *state) > intel_state->wm_results.dirty_pipes = ~0; > } > > + /* > + * We're not recomputing for the pipes not included in the > commit, so > + * make sure we start with the current state. > + */ > + memcpy(ddb, &dev_priv->wm.skl_hw.ddb, sizeof(*ddb)); > + > for_each_intel_crtc_mask(dev, intel_crtc, realloc_pipes) { > struct intel_crtc_state *cstate; >
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 425544b..0c2e252 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -3362,13 +3362,15 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate, int num_active; int id, i; + /* Clear the partitioning for disabled planes. */ + memset(ddb->plane[pipe], 0, sizeof(ddb->plane[pipe])); + memset(ddb->y_plane[pipe], 0, sizeof(ddb->y_plane[pipe])); + if (WARN_ON(!state)) return 0; if (!cstate->base.active) { ddb->pipe[pipe].start = ddb->pipe[pipe].end = 0; - memset(ddb->plane[pipe], 0, sizeof(ddb->plane[pipe])); - memset(ddb->y_plane[pipe], 0, sizeof(ddb->y_plane[pipe])); return 0; } @@ -4054,6 +4056,12 @@ skl_compute_ddb(struct drm_atomic_state *state) intel_state->wm_results.dirty_pipes = ~0; } + /* + * We're not recomputing for the pipes not included in the commit, so + * make sure we start with the current state. + */ + memcpy(ddb, &dev_priv->wm.skl_hw.ddb, sizeof(*ddb)); + for_each_intel_crtc_mask(dev, intel_crtc, realloc_pipes) { struct intel_crtc_state *cstate;
With the previous code we were only recomputing the DDB partitioning for the CRTCs included in the atomic commit, so any other active CRTCs would end up having their DDB registers zeroed. In this patch we make sure that the computed state starts as a copy of the current partitioning, and then we only zero the DDBs that we're actually going to recompute. How to reproduce the bug: 1 - Enable the primary plane on pipe A 2 - Enable the primary plane on pipe B 3 - Enable the sprite plane on pipe A Step 3 will zero the DDB partitioning for pipe B since it's not included in the commit that enabled the sprite for pipe A. I expect this to fix many FIFO underrun problems on gen9+. Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=96226 Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=96828 Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97450 Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97596 Bugzilla: https://www.phoronix.com/scan.php?page=news_item&px=Intel-Skylake-Multi-Screen-Woes Cc: stable@vger.kernel.org Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> --- drivers/gpu/drm/i915/intel_pm.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) I still have to confirm whether this closes the above bugs, but it certainly fixes the problem I described.