Re: [Mesa-dev] [PATCH resend 7/7] i965: Disable HW-binding tables on batch finish for Broadwell
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 >> --- >> 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 t
Re: [Mesa-dev] [PATCH resend 7/7] i965: Disable HW-binding tables on batch finish for Broadwell
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 > > > > > --- > > > > > 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
Re: [Mesa-dev] [PATCH resend 7/7] i965: Disable HW-binding tables on batch finish for Broadwell
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 > > > > --- > > > > 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.
Re: [Mesa-dev] [PATCH resend 7/7] i965: Disable HW-binding tables on batch finish for Broadwell
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 > > > --- > > > 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. 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 signature.asc Description: This is a digitally signed message part. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH resend 7/7] i965: Disable HW-binding tables on batch finish for Broadwell
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 > > --- > > 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. --- I'd guess there probably isn't a lot you should have to do for execlists. Unlike MI_SET_CONTEXT, RS was already a thing that Windows relied on when execlists were developed. To expand just a bit on Ville's point... I didn't look at your kernel patches, but you'd need to move any sort of enable or workaround bits to the populate context function (the only register I see in the context state is MI_RS_CONTROL, which I assume you don't need to prepopulate). The context descriptor is the other thing which replaces the old style contexts - I see nothing there about resource streamer though. If you send an error state, I'll take a look. > -- > Ville Syrjälä > Intel OTC ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH resend 7/7] i965: Disable HW-binding tables on batch finish for Broadwell
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 > --- > 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()). -- Ville Syrjälä Intel OTC ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH resend 7/7] i965: Disable HW-binding tables on batch finish for Broadwell
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 --- 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. What do you think of these couple of options I have in mind? 1) We temporarily put this mesa workaround in place until we figure out how to do this in the kernel and remove it once we put the proper solution into place 2) Skip out the GEN8 implementation altogether until we find solution for kernel? I myself am in favor of initially to having the basic parts in place then we can move forward from that afterwards. What do you think? -Abdiel ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH resend 7/7] i965: Disable HW-binding tables on batch finish for Broadwell
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 > >> --- > >> 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. signature.asc Description: This is a digitally signed message part. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH resend 7/7] i965: Disable HW-binding tables on batch finish for Broadwell
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 >> --- >> 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 :) ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH resend 7/7] i965: Disable HW-binding tables on batch finish for Broadwell
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 > --- > 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? signature.asc Description: This is a digitally signed message part. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH resend 7/7] i965: Disable HW-binding tables on batch finish for Broadwell
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
---
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(-)
diff --git a/src/mesa/drivers/dri/i965/intel_batchbuffer.c
b/src/mesa/drivers/dri/i965/intel_batchbuffer.c
index caeb31b..4244cab 100644
--- a/src/mesa/drivers/dri/i965/intel_batchbuffer.c
+++ b/src/mesa/drivers/dri/i965/intel_batchbuffer.c
@@ -33,6 +33,7 @@
#include "intel_fbo.h"
#include "brw_context.h"
#include "brw_state.h"
+#include "brw_defines.h"
#include
#include
@@ -337,6 +338,7 @@ _intel_batchbuffer_flush(struct brw_context *brw,
const char *file, int line)
{
int ret;
+ struct intel_batchbuffer *batch = &brw->batch;
if (brw->batch.used == 0)
return 0;
@@ -361,6 +363,15 @@ _intel_batchbuffer_flush(struct brw_context *brw,
brw_finish_batch(brw);
+ if (brw->has_resource_streamer && brw->gen >=8 && batch->ring != BLT_RING) {
+ intel_batchbuffer_emit_dword(brw,
+ _3DSTATE_BINDING_TABLE_POOL_ALLOC << 16 |
+ (4 - 2));
+ intel_batchbuffer_emit_dword(brw, 0);
+ intel_batchbuffer_emit_dword(brw, 0);
+ intel_batchbuffer_emit_dword(brw, 0);
+ }
+
/* Mark the end of the buffer. */
intel_batchbuffer_emit_dword(brw, MI_BATCH_BUFFER_END);
if (brw->batch.used & 1) {
diff --git a/src/mesa/drivers/dri/i965/intel_batchbuffer.h
b/src/mesa/drivers/dri/i965/intel_batchbuffer.h
index 7bdd836..0a8ad7f 100644
--- a/src/mesa/drivers/dri/i965/intel_batchbuffer.h
+++ b/src/mesa/drivers/dri/i965/intel_batchbuffer.h
@@ -26,8 +26,9 @@ extern "C" {
* - 3 DWords for MI_REPORT_PERF_COUNT itself on Gen6+. ==> 12 bytes.
* On Ironlake, it's 6 DWords, but we have some slack due to the lack of
* Sandybridge PIPE_CONTROL madness.
+ * - 4 Dwords for disabling RS on batch end ==> 16 bytes
*/
-#define BATCH_RESERVED 146
+#define BATCH_RESERVED 162
struct intel_batchbuffer;
--
1.9.1
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
