[Mesa-dev] [PATCH resend 7/7] i965: Disable HW-binding tables on batch finish for Broadwell

[Abdiel Janulgue]

On 06/03/2015 10:56 PM, Ben Widawsky wrote:
> On Wed, Jun 03, 2015 at 12:34:56PM -0700, Ben Widawsky wrote:
>> On Wed, Jun 03, 2015 at 12:05:34PM -0700, Kenneth Graunke wrote:
>>> On Wednesday, June 03, 2015 09:44:21 AM Ben Widawsky wrote:
>>>> On Wed, Jun 03, 2015 at 12:45:12PM +0300, Ville Syrjälä wrote:
>>>>> On Wed, Jun 03, 2015 at 10:05:25AM +0300, Abdiel Janulgue wrote:
>>>>>>
>>>>>> On 06/02/2015 08:28 PM, Kenneth Graunke wrote:
>>>>>>> On Tuesday, June 02, 2015 03:23:35 PM Abdiel Janulgue wrote:
>>>>>>>>
>>>>>>>> On 06/02/2015 09:31 AM, Kenneth Graunke wrote:
>>>>>>>>> On Monday, June 01, 2015 03:14:30 PM Abdiel Janulgue wrote:
>>>>>>>>>> This is needed since kernel doesn't support RS context save and
>>>>>>>>>> restore on BDW yet. So manually disable hw-generated binding tables
>>>>>>>>>> when done using it in the batch. Otherwise the GPU would no longer
>>>>>>>>>> accept software binding tables submitted by other clients including
>>>>>>>>>> but not limited to the Xorg driver.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Abdiel Janulgue <abdiel.janulgue at linux.intel.com>
>>>>>>>>>> ---
>>>>>>>>>>  src/mesa/drivers/dri/i965/intel_batchbuffer.c | 11 +++++++++++
>>>>>>>>>>  src/mesa/drivers/dri/i965/intel_batchbuffer.h |  3 ++-
>>>>>>>>>>  2 files changed, 13 insertions(+), 1 deletion(-)
>>>>>>>>>
>>>>>>>>> This seems fairly awful.  The kernel should prevent userspace from
>>>>>>>>> breaking other userspace...in the hardware context world, this really
>>>>>>>>> doesn't feel like our job.
>>>>>>>>>
>>>>>>>>> Why didn't you just update your kernel patch for Broadwell?  i.e. make
>>>>>>>>>
>>>>>>>>> drm/i915: Enable Resource Streamer state save/restore in HSW
>>>>>>>>>
>>>>>>>>> do:
>>>>>>>>> +       if (IS_HASWELL(ring->dev) || INTEL_INFO(ring->dev)->gen >= 8)
>>>>>>>>>
>>>>>>>>> instead of:all
>>>>>>>>> +       if (IS_HASWELL(ring->dev))
>>>>>>>>>
>>>>>>>>> It looks like the MI_SET_CONTEXT RS save/restore bits you used on
>>>>>>>>> Haswell still exist on Broadwell.  Do they not work or something?
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>> I was hoping to have a follow-up for GEN8 as well once the initial
>>>>>>>> kernel patches get merged  :)
>>>>>>>
>>>>>>> We should do it all at once - it should be trivial to support both.
>>>>>>>
>>>>>>
>>>>>> I put in the necessary MI_SET_CONTEXT bits for GEN8 and tried it out
>>>>>> this morning. Unfortunately, it doesn't work and hung my system.
>>>>>>
>>>>>> I guess the reason for this is that MI_SET_CONTEXT is not used in GEN8
>>>>>> anymore? I found these comments and quoting from the docs in intel_lrc.c:
>>>>>>
>>>>>> "Instead of the legacy MI_SET_CONTEXT, the way you tell the GPU to
>>>>>> switch to a contexts is via a context execution list, ergo "Execlists".
>>>>>>
>>>>>> Unfortunately, I'm not that familiar with the execlist implementation in
>>>>>> the kernel. I have a hunch that I have to insert somewhere in the global
>>>>>> default context a state that says hw-generated binding tables are
>>>>>> disabled but I'm not quite sure where to put it, probably need to study
>>>>>> it a bit.
>>>>>
>>>>> I suppose you need to at least set the RS context enable bit in the
>>>>> RING_CONTEXT_CONTROL register (in populate_lr_context()).
>>>>>
>>>>
>>>> I just read up on the IRC conversation you had with Ken from last night... Do
>>>> you need a param for HSW anyway? If we don't, then I disagree with Ken FWIW, I don't
>>>> think we should do a param and only enable for ringbuffer mode. For one thing,
>>>> ringbuffer mode is only "supported" on BDW. Not BSW or SKL.  Second, it will be
>>>> disabled by default on the platform that has the getparam, so nobody will
>>>> actually get the RS without a kernel param. Third, we know RS can work with
>>>> execlists because the Windows driver does it. If you need the param anyway for
>>>> HSW, then sure, why not add it.
>>>
>>> That's not really the point.  In order for Mesa to use the resource
>>> streamer, it needs to pass I915_EXEC_RESOURCE_STREAMER to execbuf2.
>>> We also want to know that the kernel will save/restore RS context stuff,
>>> so us turning it on won't screw over other userspace apps.
>>>
>>> We need to detect whether the running kernel meets these requirements.
>>> Adding a I915_PARAM_HAS_RESOURCE_STREAMER getparam seemed like a
>>> reasonable/traditional way of doing that version handshake.
>>>
>>> The only alternative I could think of is submitting a batchbuffer with
>>> I915_EXEC_RESOURCE_STREAMER and seeing if it -EINVALs.  Which is
>>> possible, but seems uglier to me.
>>
>> Right, so that was my first question - if you need it anyway for HSW, then do
>> it. I just didn't think we should be adding such a thing to specifically check
>> for GEN8 ringbuffer. For a long time we were unintentionally saving RS state in
>> the kernel and we wouldn't need something like this, but it's no longer the
>> case.
>>
>> Though flipping the conversation around, it seems we *just* need the param for
>> HSW, and we can then make use of it temporarily on BDW.
>>
> 
> I guess that's not true since we shipped kernels for BDW with ringbuffer mode.
> So yeah, I think I agree that no matter what we need a flag, but you just need a
> flag for ringbuffer/execlists which is probably more reusable than a flag for
> just RS. Just a thought...
> 
>>>
>>> The thinking on execlists was: if people want to land abj's current
>>> patches, that only provide ringbuffer support, the kernel could simply
>>> say "no" to the getparam if in execlist mode (for now).  Then fix that
>>> in a follow-on series.  Ideally, we would just make execlists work
>>> before landing any patches, though, and sidestep that issue.
>>>
>>> I don't want to land HSW-only patches.  BDW is the currently shipping
>>> platform, it would be strange to not support it.
>>>
>>> --Ken
>>
>> Yeah, I agree completely, I wasn't even remotely suggesting this, rather I was
>> suggesting holding off further on merging, I'm sure Abdiel would be thrilled. I
>> guess I just wanted people to remember execlists aren't going away and this
>> isn't the ultimate solution. So basically what you said,
>>
>>> Ideally, we would just make execlists work before landing any patches, though,
>>> and sidestep that issue.
>>
>> I vote that ^^. I'm extreme in that I'd say do this without HSW and deal with
>> the getparam later.
> 

Talked with Ville yesterday and I tried out his suggestion by modifying
the RING_CONTEXT_CONTROL register and that one works perfectly. So now
we have RS save and restore working with execlists too in GEN8!

So I guess the remaining thing I have to do is add the
I915_PARAM_HAS_RESOURCE_STREAMER getparam and skip out the RS
save/restore checks because we have this working for both platforms now