[Intel-gfx] [PATCH 4/5] drm/i915: add a new perf configuration execbuf parameter

[Lionel Landwerlin]

On 21/05/2019 18:48, Chris Wilson wrote:
> Quoting Lionel Landwerlin (2019-05-21 18:19:50)
>> On 21/05/2019 18:07, Chris Wilson wrote:
>>> Quoting Lionel Landwerlin (2019-05-21 15:08:54)
>>>> +       if (eb->oa_config &&
>>>> +           eb->oa_config != eb->i915->perf.oa.exclusive_stream->oa_config) {
>>> But the oa_config is not ordered with respect to requests, and the
>>> registers changed here are not context saved and so may be changed by a
>>> third party before execution. The first party must presumably dropped
>>> the perf_fd before then, so maybe you don't care? Hmm, doesn't even take
>>> a third party as the perf_fd owner may change their mind for different
>>> contexts and be surprised when the wrong set is used.
>>
>> The OA config batch should be ordered with regard to the MI_BBS we're
>> doing just below right?
> But you only emit if the oa_config has changed. Ergo, it may have
> changed between submission and execution.
>
>> It's written before in the ring buffer.
>>
>>
>> That essentially all we need so that as the perf queries run in the
>> batch supplied by the application runs with the configuration needed.
>>
>> If the application shares the perf FD and someone else runs another
>> configuration, it's the application fault.
>>
>> It needs to hold the perf FD for as long as it's doing perf queries and
>> not allow anybody else to interact with the OA configuration.
> If one app is trying to use different configs on different contexts
> (which seems reasonable if it is trying to sample different stats?) then
> it can be caught out. The order of execution is not the same as the
> order of submission (unless we enforce it by e.g. defining the
> perf.oa_config as a barrier).


Thanks, I think I see the problem. It's pretty much the same as the sseu 
reconfiguration.

Looking at the code it seems that the barrier is gone for sseu and I'm 
afraid that sounds like what's needed here :(


-Lionel

>
>
> Another way would be to unconditionally emit the BB_START for the
> oa_vma, and instead do the early exit with a MI_CONDITIONAL_BB_END by
> comparing against a value stashed in the engine hwsp. You could do a
> predicated BB_START instead, but that looks to be more of a nuisance.
> -Chris
>