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.janul...@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: > > > > > >>> + 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. _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
