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

[Francisco Jerez <currojerez@riseup.net>]

[-- Attachment #1.1.1: Type: text/plain, Size: 3919 bytes --]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> 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.
>
What do you want me to merge it with?  This *is* PATCH 1, and PATCH 5 is
largely unrelated.  I wouldn't have any objection to squashing PATCH 4
into this commit though.

> 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).
>
The constraints are just the hardware's constraints on the L3
partitioning.  The function is meant to implement the details of
programming the L3 configuration, which is different for different gens
even though the overall structure of the L3 partitioning is the same.
Of course the constraints set by specific hardware on the partition
sizes cannot be abstracted out.

I guess that splitting this out into two functions (one for gen7 and
another for gen8) wouldn't hurt much, but open-coding the function in
all its uses (5) would involve duplicating quite some code.

Assuming I split the function into gen7 and gen8 variants, would you
like them to implement a common consistent interface (i.e. the same
prototype that my init_l3_partitioning_workarounds() implements), or
would you like the variant for each gen to implement an ad-hoc interface
according to the partition configs actually needed on each gen?  I
suspect the former would be cleaner.

>> +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?

Sorry?  Does INTEL_INFO() take a drm_device or a drm_i915_private
pointer as argument?  The two types don't seem to be related by an
inheritance relationship or anything similar AFAICT, and most other uses
in this file seem to pass a drm_device as argument even where a
drm_i915_private is available.

> -Chris
>
> -- 
> Chris Wilson, Intel Open Source Technology Centre

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 212 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx