linux-kernel - Re: [PATCH v7] drm/i915/skl: Add support for the SAGV, fix underrun hangs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20160803145301.GF32025@intel.com>
Date:	Wed, 3 Aug 2016 07:53:01 -0700
From:	Matt Roper <matthew.d.roper@...el.com>
To:	Lyude <cpaul@...hat.com>
Cc:	intel-gfx@...ts.freedesktop.org,
	Ville Syrjälä 
	<ville.syrjala@...ux.intel.com>,
	Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
	Daniel Vetter <daniel.vetter@...ll.ch>, stable@...r.kernel.org,
	Daniel Vetter <daniel.vetter@...el.com>,
	Jani Nikula <jani.nikula@...ux.intel.com>,
	David Airlie <airlied@...ux.ie>,
	dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v7] drm/i915/skl: Add support for the SAGV, fix
 underrun hangs

On Wed, Aug 03, 2016 at 09:50:23AM -0400, Lyude wrote:
...
> +int
> +skl_disable_sagv(struct drm_i915_private *dev_priv)
> +{
> +	int ret, result;
> +
> +	if (IS_BROXTON(dev_priv))
> +		return 0;
> +	if (!dev_priv->skl_sagv_enabled)
> +		return 0;
> +
> +	mutex_lock(&dev_priv->rps.hw_lock);
> +	DRM_DEBUG_KMS("Disabling the SAGV\n");
> +
> +	/* bspec says to keep retrying for at least 1 ms */
> +	ret = wait_for(result = skl_do_sagv_disable(dev_priv), 1);
> +	mutex_unlock(&dev_priv->rps.hw_lock);
> +
> +	if (ret == -ETIMEDOUT)
> +		DRM_ERROR("Request to disable SAGV timed out\n");
> +	else {

I think you might have missed my note here on the last patch, but we
need to fix the minor coding style issue here (if either branch of an
if/else needs braces, you need to put them on both branches).

> +		if (result == 1)

As I mentioned before, the 1=off looks confusing if someone isn't
looking at the bspec carefully.  Using a #define for the various 0x1's
in this patch might help clarify the code slightly.

With those minor changes made,

Reviewed-by: Matt Roper <matthew.d.roper@...el.com>


Matt

> +			dev_priv->skl_sagv_enabled = false;
> +
> +		ret = result;
> +	}
> +
> +	return ret;
> +}
> +
> +static void
>  skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
>  				   const struct intel_crtc_state *cstate,
>  				   struct skl_ddb_entry *alloc, /* out */
> @@ -4236,6 +4346,8 @@ void skl_wm_get_hw_state(struct drm_device *dev)
>  		/* Easy/common case; just sanitize DDB now if everything off */
>  		memset(ddb, 0, sizeof(*ddb));
>  	}
> +
> +	skl_sagv_get_hw_state(dev_priv);
>  }
>  
>  static void ilk_pipe_wm_get_hw_state(struct drm_crtc *crtc)
> -- 
> 2.7.4
> 

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795