| Message ID | 20170531075646.15158-1-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, On Wednesday 31 May 2017 01:26 PM, Maarten Lankhorst wrote: > On some systems there can be a race condition in which no crtc state is > added to the first atomic commit. This results in all crtc's having a > null DDB allocation, causing a FIFO underrun on any update until the > first modeset. > > Reported-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Inspired-by: Mahesh Kumar <mahesh1.kumar@intel.com> > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Fixes: 98d39494d375 ("drm/i915/gen9: Compute DDB allocation at atomic > check time (v4)") > Cc: <stable@vger.kernel.org> # v4.8+ > Cc: Mahesh Kumar <mahesh1.kumar@intel.com> > Cc: Matt Roper <matthew.d.roper@intel.com> > --- > drivers/gpu/drm/i915/intel_pm.c | 18 ++++++++++++++++++ > 1 file changed, 18 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index a488e068c3d6..8daa382b47f3 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -5037,10 +5037,27 @@ skl_compute_wm(struct drm_atomic_state *state) > struct drm_crtc_state *cstate; > struct intel_atomic_state *intel_state = to_intel_atomic_state(state); > struct skl_wm_values *results = &intel_state->wm_results; > + struct drm_device *dev = state->dev; > struct skl_pipe_wm *pipe_wm; > bool changed = false; > int ret, i; > > + if (to_i915(dev)->wm.distrust_bios_wm) { > + changed = true; > + > + /* > + * The first commit after driver load must update the ddb > + * allocations, so make sure we at least hold a lock to > + * inspect the state required for watermark updates. > + * > + * Below we'll grab the required crtc state. > + */ > + ret = drm_modeset_lock(&dev->mode_config.connection_mutex, > + state->acquire_ctx); Do we need to get lock here? We will anyway go ahead & get the connection_mutex lock during skl_compute_ddb based on distruct_bios_wm flag. -Mahesh > + if (ret) > + return ret; > + } > + > /* > * If this transaction isn't actually touching any CRTC's, don't > * bother with watermark calculation. Note that if we pass this > @@ -5051,6 +5068,7 @@ skl_compute_wm(struct drm_atomic_state *state) > */ > for_each_new_crtc_in_state(state, crtc, cstate, i) > changed = true; > + > if (!changed) > return 0; >
Op 31-05-17 om 12:42 schreef Mahesh Kumar: > Hi, > > > On Wednesday 31 May 2017 01:26 PM, Maarten Lankhorst wrote: >> On some systems there can be a race condition in which no crtc state is >> added to the first atomic commit. This results in all crtc's having a >> null DDB allocation, causing a FIFO underrun on any update until the >> first modeset. >> >> Reported-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >> Inspired-by: Mahesh Kumar <mahesh1.kumar@intel.com> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >> Fixes: 98d39494d375 ("drm/i915/gen9: Compute DDB allocation at atomic >> check time (v4)") >> Cc: <stable@vger.kernel.org> # v4.8+ >> Cc: Mahesh Kumar <mahesh1.kumar@intel.com> >> Cc: Matt Roper <matthew.d.roper@intel.com> >> --- >> drivers/gpu/drm/i915/intel_pm.c | 18 ++++++++++++++++++ >> 1 file changed, 18 insertions(+) >> >> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c >> index a488e068c3d6..8daa382b47f3 100644 >> --- a/drivers/gpu/drm/i915/intel_pm.c >> +++ b/drivers/gpu/drm/i915/intel_pm.c >> @@ -5037,10 +5037,27 @@ skl_compute_wm(struct drm_atomic_state *state) >> struct drm_crtc_state *cstate; >> struct intel_atomic_state *intel_state = to_intel_atomic_state(state); >> struct skl_wm_values *results = &intel_state->wm_results; >> + struct drm_device *dev = state->dev; >> struct skl_pipe_wm *pipe_wm; >> bool changed = false; >> int ret, i; >> + if (to_i915(dev)->wm.distrust_bios_wm) { >> + changed = true; >> + >> + /* >> + * The first commit after driver load must update the ddb >> + * allocations, so make sure we at least hold a lock to >> + * inspect the state required for watermark updates. >> + * >> + * Below we'll grab the required crtc state. >> + */ >> + ret = drm_modeset_lock(&dev->mode_config.connection_mutex, >> + state->acquire_ctx); > Do we need to get lock here? > We will anyway go ahead & get the connection_mutex lock during skl_compute_ddb based on distruct_bios_wm flag. Oh right, missed that one, yeah ignore it. :) I'll send the original patch again, thanks for catching. ~Maarten
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index a488e068c3d6..8daa382b47f3 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -5037,10 +5037,27 @@ skl_compute_wm(struct drm_atomic_state *state) struct drm_crtc_state *cstate; struct intel_atomic_state *intel_state = to_intel_atomic_state(state); struct skl_wm_values *results = &intel_state->wm_results; + struct drm_device *dev = state->dev; struct skl_pipe_wm *pipe_wm; bool changed = false; int ret, i; + if (to_i915(dev)->wm.distrust_bios_wm) { + changed = true; + + /* + * The first commit after driver load must update the ddb + * allocations, so make sure we at least hold a lock to + * inspect the state required for watermark updates. + * + * Below we'll grab the required crtc state. + */ + ret = drm_modeset_lock(&dev->mode_config.connection_mutex, + state->acquire_ctx); + if (ret) + return ret; + } + /* * If this transaction isn't actually touching any CRTC's, don't * bother with watermark calculation. Note that if we pass this @@ -5051,6 +5068,7 @@ skl_compute_wm(struct drm_atomic_state *state) */ for_each_new_crtc_in_state(state, crtc, cstate, i) changed = true; + if (!changed) return 0;
On some systems there can be a race condition in which no crtc state is added to the first atomic commit. This results in all crtc's having a null DDB allocation, causing a FIFO underrun on any update until the first modeset. Reported-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Inspired-by: Mahesh Kumar <mahesh1.kumar@intel.com> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Fixes: 98d39494d375 ("drm/i915/gen9: Compute DDB allocation at atomic check time (v4)") Cc: <stable@vger.kernel.org> # v4.8+ Cc: Mahesh Kumar <mahesh1.kumar@intel.com> Cc: Matt Roper <matthew.d.roper@intel.com> --- drivers/gpu/drm/i915/intel_pm.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)