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 <[email protected]> >>>>>>>>>> --- >>>>>>>>>> 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 _______________________________________________ mesa-dev mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/mesa-dev
