Re: [RFC 0/2] Add Pooled EU support

Siluvery, Arun
2015-07-13T15:00:10Z

On 11/07/2015 20:09, Chris Wilson wrote:
> On Sat, Jul 11, 2015 at 08:05:05PM +0100, Chris Wilson wrote:
>> On Fri, Jul 10, 2015 at 06:35:18PM +0100, Arun Siluvery wrote:
>>> These patches enabled Pooled EU support for BXT, they are implemented
>>> by Armin Reese. I am sending these patches in its current form for comments.
>>>
>>> These patches modify Golden batch to have a set of modification values
>>> where we can change the commands based on Gen. The commands to enable
>>> Pooled EU are inserted after MI_BATCH_BUFFER_END. If the given Gen
>>> supports this feature, modification values are used to replace
>>> MI_BATCH_BUFFER_END so we send commands to enable Pooled EU. These
>>> commands need to be part of this batch because they are to be
>>> initialized only once. Userspace will have option to query the
>>> availability of this feature, those changes are not included in
>>> this series.
>>
>> Would it not just be simpler to execute 2 batches? First holding the
>> basic and common state for the gen, the second using subgen. That we
>> have a chunk of binary data is nasty, but at least we can point to the
>> generator and be able to decipher it and recreate it as required. Doing
>> binary patching on top, on that path lies madness.

I like this idea of sending 2 batches if that is acceptable. In this
case we don't have to touch the golden batch and hence the generator
tool and also not worry about using the correct binary blob as header.

the setup in this case would be,

1. send golden batch
2. prepare and send batch to configure pooled EU as per subslice and EU
count

Why we have a separate tool in the first place, is it not possible to
carry all of them in code or are there any restrictions in doing so?

>>
>> What is the minimum instruction sequence required to be able to setup the
>> default EU state? Is it small enough that carrying it as code in the
>> kernel is viable (and readable)?
>>
setting up of pooled EU configuration is only few instructions, it can
be added to the driver.
>> (That actually is critical here as currently we have to juggle multiple
>> sources and look very carefully at what is being patched - I am not
>> confident that we will not introduce mistakes in a week's time, let
>> alone a year or two.)
>
> The alternative is to just say that the patch table is also
> autogenerated and for that to be simple and clear, and far more
> documentated as it relies on a strict protocol.

The patch table is also auto generated using intel_null_state_gen tool
but it is patched based on Gen.

regards
Arun

> -Chris