| Message ID | 20170526151546.25025-2-mahesh1.kumar@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, May 26, 2017 at 8:15 AM, Mahesh Kumar <mahesh1.kumar@intel.com> wrote: > Don't trust cached DDB values. Recalculate the ddb value if cached value > is zero. > > If i915 is build as a module, there may be a race condition when > cursor_disable call comes even before intel_fbdev_initial_config. > Which may lead to cached value being 0. And further commit will fail > until a modeset. > > Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com> > --- > drivers/gpu/drm/i915/intel_pm.c | 15 ++++++++++----- > 1 file changed, 10 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index 936eef1634c7..b67be1355e39 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -3721,6 +3721,7 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device *dev, > struct drm_i915_private *dev_priv = to_i915(dev); > struct drm_crtc *for_crtc = cstate->base.crtc; > unsigned int pipe_size, ddb_size; > + unsigned int active_crtcs; > int nth_active_pipe; > > if (WARN_ON(!state) || !cstate->base.active) { > @@ -3731,10 +3732,11 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device *dev, > } > > if (intel_state->active_pipe_changes) > - *num_active = hweight32(intel_state->active_crtcs); > + active_crtcs = intel_state->active_crtcs; > else > - *num_active = hweight32(dev_priv->active_crtcs); > + active_crtcs = dev_priv->active_crtcs; > > + *num_active = hweight32(active_crtcs); > ddb_size = INTEL_INFO(dev_priv)->ddb_size; > WARN_ON(ddb_size == 0); > > @@ -3754,12 +3756,15 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device *dev, > * copy from old state to be sure > */ > *alloc = to_intel_crtc_state(for_crtc->state)->wm.skl.ddb; > - return; > + if (!skl_ddb_entry_size(alloc)) > + DRM_DEBUG_KMS("Cached pipe DDB is 0 recalculate\n"); > + else > + return; I see 2 different patches inside this patch here. One is this chunk here that does what commit message tells. > } > > - nth_active_pipe = hweight32(intel_state->active_crtcs & > + nth_active_pipe = hweight32(active_crtcs & > (drm_crtc_mask(for_crtc) - 1)); > - pipe_size = ddb_size / hweight32(intel_state->active_crtcs); > + pipe_size = ddb_size / hweight32(active_crtcs); While this is fixing another bug where we were still using intel_state->active_crtcs here even when we use dev_priv->active_crtcs for num_active. Am I missing or misunderstanding something? > alloc->start = nth_active_pipe * ddb_size / *num_active; > alloc->end = alloc->start + pipe_size; > } > -- > 2.11.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Hi, On Saturday 27 May 2017 02:53 AM, Rodrigo Vivi wrote: > On Fri, May 26, 2017 at 8:15 AM, Mahesh Kumar <mahesh1.kumar@intel.com> wrote: >> Don't trust cached DDB values. Recalculate the ddb value if cached value >> is zero. >> >> If i915 is build as a module, there may be a race condition when >> cursor_disable call comes even before intel_fbdev_initial_config. >> Which may lead to cached value being 0. And further commit will fail >> until a modeset. >> >> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com> >> --- >> drivers/gpu/drm/i915/intel_pm.c | 15 ++++++++++----- >> 1 file changed, 10 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c >> index 936eef1634c7..b67be1355e39 100644 >> --- a/drivers/gpu/drm/i915/intel_pm.c >> +++ b/drivers/gpu/drm/i915/intel_pm.c >> @@ -3721,6 +3721,7 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device *dev, >> struct drm_i915_private *dev_priv = to_i915(dev); >> struct drm_crtc *for_crtc = cstate->base.crtc; >> unsigned int pipe_size, ddb_size; >> + unsigned int active_crtcs; >> int nth_active_pipe; >> >> if (WARN_ON(!state) || !cstate->base.active) { >> @@ -3731,10 +3732,11 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device *dev, >> } >> >> if (intel_state->active_pipe_changes) >> - *num_active = hweight32(intel_state->active_crtcs); >> + active_crtcs = intel_state->active_crtcs; >> else >> - *num_active = hweight32(dev_priv->active_crtcs); >> + active_crtcs = dev_priv->active_crtcs; >> >> + *num_active = hweight32(active_crtcs); >> ddb_size = INTEL_INFO(dev_priv)->ddb_size; >> WARN_ON(ddb_size == 0); >> >> @@ -3754,12 +3756,15 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device *dev, >> * copy from old state to be sure >> */ >> *alloc = to_intel_crtc_state(for_crtc->state)->wm.skl.ddb; >> - return; >> + if (!skl_ddb_entry_size(alloc)) >> + DRM_DEBUG_KMS("Cached pipe DDB is 0 recalculate\n"); >> + else >> + return; > I see 2 different patches inside this patch here. One is this chunk > here that does what commit message tells. > >> } >> >> - nth_active_pipe = hweight32(intel_state->active_crtcs & >> + nth_active_pipe = hweight32(active_crtcs & >> (drm_crtc_mask(for_crtc) - 1)); >> - pipe_size = ddb_size / hweight32(intel_state->active_crtcs); >> + pipe_size = ddb_size / hweight32(active_crtcs); > While this is fixing another bug where we were still using > intel_state->active_crtcs here even when we use dev_priv->active_crtcs > for num_active. Thanks for review :) This one is prerequisite for the first part. In earlier scenario, we will reach to this point only if (intel_state->active_pipe_changes) is true. So it was ok to get num of active crtcs from "intel_state->active_crtcs". But after above change we may reach to this point even if "intel_state->active_pipe_changes" is not true. So we should get the active_crtcs from correct struct. Thats why I think this should be part of same patch. Please let me know if you still have different opinion. thanks, -Mahesh > > Am I missing or misunderstanding something? > >> alloc->start = nth_active_pipe * ddb_size / *num_active; >> alloc->end = alloc->start + pipe_size; >> } >> -- >> 2.11.0 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx > >
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 936eef1634c7..b67be1355e39 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -3721,6 +3721,7 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device *dev, struct drm_i915_private *dev_priv = to_i915(dev); struct drm_crtc *for_crtc = cstate->base.crtc; unsigned int pipe_size, ddb_size; + unsigned int active_crtcs; int nth_active_pipe; if (WARN_ON(!state) || !cstate->base.active) { @@ -3731,10 +3732,11 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device *dev, } if (intel_state->active_pipe_changes) - *num_active = hweight32(intel_state->active_crtcs); + active_crtcs = intel_state->active_crtcs; else - *num_active = hweight32(dev_priv->active_crtcs); + active_crtcs = dev_priv->active_crtcs; + *num_active = hweight32(active_crtcs); ddb_size = INTEL_INFO(dev_priv)->ddb_size; WARN_ON(ddb_size == 0); @@ -3754,12 +3756,15 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device *dev, * copy from old state to be sure */ *alloc = to_intel_crtc_state(for_crtc->state)->wm.skl.ddb; - return; + if (!skl_ddb_entry_size(alloc)) + DRM_DEBUG_KMS("Cached pipe DDB is 0 recalculate\n"); + else + return; } - nth_active_pipe = hweight32(intel_state->active_crtcs & + nth_active_pipe = hweight32(active_crtcs & (drm_crtc_mask(for_crtc) - 1)); - pipe_size = ddb_size / hweight32(intel_state->active_crtcs); + pipe_size = ddb_size / hweight32(active_crtcs); alloc->start = nth_active_pipe * ddb_size / *num_active; alloc->end = alloc->start + pipe_size; }
Don't trust cached DDB values. Recalculate the ddb value if cached value is zero. If i915 is build as a module, there may be a race condition when cursor_disable call comes even before intel_fbdev_initial_config. Which may lead to cached value being 0. And further commit will fail until a modeset. Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com> --- drivers/gpu/drm/i915/intel_pm.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-)