[Intel-gfx] [PATCH 1/6] drm/i915: Implement L3 partitioning set-up from the workaround list.

[Chris Wilson]

On Wed, Oct 07, 2015 at 02:44:00PM +0300, Francisco Jerez wrote:
> This programs the L3 configuration based on the sizes given for each
> partition as arguments.  The relevant register writes are added to the
> workaround list so that they are re-applied to each context while it's
> initialized, preventing state leaks from other userspace processes
> which may have modified the L3 partitioning from its boot-up state,
> since all relevant registers are part of the software and hardware
> command checker whitelists.
> 
> Some userspace clients (DDX and current versions of Mesa not patched
> with my L3 partitioning series [1]) assume that the L3 configuration,
> in particular the URB size, comes up in certain state when a context
> is created, but nothing in the kernel guarantees this assumption, the
> registers that control the partitioning of the L3 cache were being
> left untouched.
> 
> Note that the VLV_L3SQCREG1_SQGHPCI_DEFAULT macro defined here has the
> same value as the previously defined VLV_B0_WA_L3SQCREG1_VALUE, but
> the latter will be removed in a future commit.
> 
> [1] http://lists.freedesktop.org/archives/mesa-dev/2015-September/093550.html

Merge this with 1 + 5 (or was it 4?)- not only does it introduce a
temporary unused function, but we have to jump between patches to see
whether the function is safe (especially given those BUGs), and you add
all w/a in the same patch so bisection is not improved.

Looking at the function itself, I am not convinced that it actually adds
anything over calling setting up the WA from the vfuncs - at least the
bdw/gen7 split is redundant (we split at the vfunc then call one
function where we replit again, but with nasty constraints on the
interface of the function for different gen, it's not a function for the
faint of heart).

> +static int init_l3_partitioning_workarounds(struct intel_engine_cs *ring,
> +					    unsigned int n_urb,
> +					    unsigned int n_ro,
> +					    unsigned int n_dc,
> +					    unsigned int n_all)
> +{
> +	struct drm_device *dev = ring->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +	if (INTEL_INFO(dev)->gen >= 8) {

Why use dev here? You already have dev_priv, so why chase the pointer
again?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre