From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga06.intel.com ([134.134.136.31]:11017 "EHLO mga06.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750912AbdEaKjA (ORCPT ); Wed, 31 May 2017 06:39:00 -0400 Subject: Re: [PATCH] drm/i915: Always recompute watermarks when distrust_bios_wm is set To: Maarten Lankhorst , intel-gfx@lists.freedesktop.org References: <20170526151546.25025-2-mahesh1.kumar@intel.com> <20170531075646.15158-1-maarten.lankhorst@linux.intel.com> Cc: "# v4 . 8+" , Matt Roper From: Mahesh Kumar Message-ID: <4b0803df-ee99-f4a4-1b36-aa1ec3f47ff6@intel.com> Date: Wed, 31 May 2017 16:12:47 +0530 MIME-Version: 1.0 In-Reply-To: <20170531075646.15158-1-maarten.lankhorst@linux.intel.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Sender: stable-owner@vger.kernel.org List-ID: 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 > Inspired-by: Mahesh Kumar > Signed-off-by: Maarten Lankhorst > Fixes: 98d39494d375 ("drm/i915/gen9: Compute DDB allocation at atomic > check time (v4)") > Cc: # v4.8+ > Cc: Mahesh Kumar > Cc: Matt Roper > --- > 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; >