Re: [Intel-gfx] [PATCH v16 6/7] drm/i915: Protect intel_dbuf_slices_update with mutex

From: Matt Roper <matthew.d.roper@intel.com>
To: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH v16 6/7] drm/i915: Protect intel_dbuf_slices_update with mutex
Date: Tue, 28 Jan 2020 15:33:11 -0800	[thread overview]
Message-ID: <20200128233311.GH22783@mdroper-desk1.amr.corp.intel.com> (raw)
In-Reply-To: <20200124084456.2961-7-stanislav.lisovskiy@intel.com>

On Fri, Jan 24, 2020 at 10:44:55AM +0200, Stanislav Lisovskiy wrote:
> Now using power_domain mutex to protect from race condition, which
> can occur because intel_dbuf_slices_update might be running in
> parallel to gen9_dc_off_power_well_enable being called from
> intel_dp_detect for instance, which causes assertion triggered by
> race condition, as gen9_assert_dbuf_enabled might preempt this
> when registers were already updated, while dev_priv was not.

I may be overlooking something, but I think your next patch already
takes care of this by ensuring we only do dbuf updates during modesets.
We already had POWER_DOMAIN_MODESET in our various DC_OFF_POWER_DOMAINS
definitions which would ensure that the "DC off" power well is enabled
(and DC states themselves are disabled) for the entire duration of the
modeset process.

If we need this, I'm not sure whether it's a good idea to use
power_domains->lock rather than a new, dedicated lock.  Anything that
touches power domains in any manner grabs this lock, even though we only
really care about it for stopping races with the specific "DC off" power
well.

Also, if we bisect to the point right before these last two patches,
don't we have a problem since there's a point in the git history where
we potentially face a race?

Matt

> 
> Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display_power.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c b/drivers/gpu/drm/i915/display/intel_display_power.c
> index 96b38252578b..99ddc21e004c 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_power.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_power.c
> @@ -4404,12 +4404,22 @@ void icl_dbuf_slices_update(struct drm_i915_private *dev_priv,
>  {
>  	int i;
>  	int max_slices = INTEL_INFO(dev_priv)->num_supported_dbuf_slices;
> +	struct i915_power_domains *power_domains = &dev_priv->power_domains;
>  
>  	WARN(hweight8(req_slices) > max_slices,
>  	     "Invalid number of dbuf slices requested\n");
>  
>  	DRM_DEBUG_KMS("Updating dbuf slices to 0x%x\n", req_slices);
>  
> +	/*
> +	 * Might be running this in parallel to gen9_dc_off_power_well_enable
> +	 * being called from intel_dp_detect for instance,
> +	 * which causes assertion triggered by race condition,
> +	 * as gen9_assert_dbuf_enabled might preempt this when registers
> +	 * were already updated, while dev_priv was not.
> +	 */
> +	mutex_lock(&power_domains->lock);
> +
>  	for (i = 0; i < max_slices; i++) {
>  		intel_dbuf_slice_set(dev_priv,
>  				     _DBUF_CTL_S(i),
> @@ -4417,6 +4427,8 @@ void icl_dbuf_slices_update(struct drm_i915_private *dev_priv,
>  	}
>  
>  	dev_priv->enabled_dbuf_slices_mask = req_slices;
> +
> +	mutex_unlock(&power_domains->lock);
>  }
>  
>  static void icl_dbuf_enable(struct drm_i915_private *dev_priv)
> -- 
> 2.24.1.485.gad05a3d8e5
> 

-- 
Matt Roper
Graphics Software Engineer
VTT-OSGC Platform Enablement
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx