| Message ID | 1395813123-2027-1-git-send-email-kenneth@whitecape.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Mar 25, 2014 at 10:52:03PM -0700, Kenneth Graunke wrote: > Mesa needs to be able to write OACONTROL in order to expose the > Observability Architecture's performance counters via OpenGL. > > Signed-off-by: Kenneth Graunke <kenneth@whitecape.org> Thanks a lot for quickly tracking this down. Now when we've talked about OA a little while ago we concluded that mesa should clear OACONTROL again before the batch ends to make sure that userspace can't unduly observe other processes. So I think it'd be worth to keep track of this with a flag (set when OACONTROL is != 0 and reset when the batch loads 0). Also we need to make sure that userspace sets the right OACONTROL modes (not the one which streams into a global gtt buffer essentially). So some additional work required. I've added a FIXME comment to the code. Brad, can you please look into this, including the corresponding i-g-t which tries to enable OA but doesn't disable it and one which tries to enable it in the continuous mode (which writes to gtt)? Ken's previous mails has the cmds mesa emits. Thanks, Daniel > --- > drivers/gpu/drm/i915/i915_cmd_parser.c | 1 + > drivers/gpu/drm/i915/i915_reg.h | 2 ++ > 2 files changed, 3 insertions(+) > > This patch needs to go before > > commit 6d42f94084b8c69813d7ecd0466c33fe561bf127 > Author: Brad Volkin <bradley.d.volkin@intel.com> > Date: Tue Feb 18 10:15:57 2014 -0800 > > drm/i915: Enable command parsing by default > > in whatever branch gets submitted to Dave Airlie. Or, that commit needs > to be reverted. Otherwise, every OpenGL program will abort. Examples > of programs that abort include GNOME, KDE, Firefox, and glxgears. > > diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i915/i915_cmd_parser.c > index bae7c2f..d4a50b9 100644 > --- a/drivers/gpu/drm/i915/i915_cmd_parser.c > +++ b/drivers/gpu/drm/i915/i915_cmd_parser.c > @@ -415,6 +415,7 @@ static const u32 gen7_render_regs[] = { > GEN7_SO_WRITE_OFFSET(1), > GEN7_SO_WRITE_OFFSET(2), > GEN7_SO_WRITE_OFFSET(3), > + OACONTROL, > }; > > static const u32 gen7_blt_regs[] = { > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index 9f9e2b7..0ebc20d 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -427,6 +427,8 @@ > /* There are the 4 64-bit counter registers, one for each stream output */ > #define GEN7_SO_NUM_PRIMS_WRITTEN(n) (0x5200 + (n) * 8) > > +#define OACONTROL 0x2360 > + > #define _GEN7_PIPEA_DE_LOAD_SL 0x70068 > #define _GEN7_PIPEB_DE_LOAD_SL 0x71068 > #define GEN7_PIPE_DE_LOAD_SL(pipe) _PIPE(pipe, \ > -- > 1.9.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, 26 Mar 2014, Kenneth Graunke <kenneth@whitecape.org> wrote: > Mesa needs to be able to write OACONTROL in order to expose the > Observability Architecture's performance counters via OpenGL. > > Signed-off-by: Kenneth Graunke <kenneth@whitecape.org> > --- > drivers/gpu/drm/i915/i915_cmd_parser.c | 1 + > drivers/gpu/drm/i915/i915_reg.h | 2 ++ > 2 files changed, 3 insertions(+) > > This patch needs to go before > > commit 6d42f94084b8c69813d7ecd0466c33fe561bf127 > Author: Brad Volkin <bradley.d.volkin@intel.com> > Date: Tue Feb 18 10:15:57 2014 -0800 > > drm/i915: Enable command parsing by default > > in whatever branch gets submitted to Dave Airlie. Or, that commit needs > to be reverted. Otherwise, every OpenGL program will abort. Examples > of programs that abort include GNOME, KDE, Firefox, and glxgears. > > diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i915/i915_cmd_parser.c > index bae7c2f..d4a50b9 100644 > --- a/drivers/gpu/drm/i915/i915_cmd_parser.c > +++ b/drivers/gpu/drm/i915/i915_cmd_parser.c > @@ -415,6 +415,7 @@ static const u32 gen7_render_regs[] = { > GEN7_SO_WRITE_OFFSET(1), > GEN7_SO_WRITE_OFFSET(2), > GEN7_SO_WRITE_OFFSET(3), > + OACONTROL, Comment above gen7_render_regs array: * Register whitelists, sorted by increasing register offset. You'll get a DRM_ERROR for this. Daniel, naughty naughty for pushing this already! BR, Jani. > }; > > static const u32 gen7_blt_regs[] = { > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index 9f9e2b7..0ebc20d 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -427,6 +427,8 @@ > /* There are the 4 64-bit counter registers, one for each stream output */ > #define GEN7_SO_NUM_PRIMS_WRITTEN(n) (0x5200 + (n) * 8) > > +#define OACONTROL 0x2360 > + > #define _GEN7_PIPEA_DE_LOAD_SL 0x70068 > #define _GEN7_PIPEB_DE_LOAD_SL 0x71068 > #define GEN7_PIPE_DE_LOAD_SL(pipe) _PIPE(pipe, \ > -- > 1.9.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, Mar 25, 2014 at 11:21:23PM -0700, Daniel Vetter wrote: > On Tue, Mar 25, 2014 at 10:52:03PM -0700, Kenneth Graunke wrote: > > Mesa needs to be able to write OACONTROL in order to expose the > > Observability Architecture's performance counters via OpenGL. > > > > Signed-off-by: Kenneth Graunke <kenneth@whitecape.org> > > Thanks a lot for quickly tracking this down. Now when we've talked about > OA a little while ago we concluded that mesa should clear OACONTROL again > before the batch ends to make sure that userspace can't unduly observe > other processes. So I think it'd be worth to keep track of this with a > flag (set when OACONTROL is != 0 and reset when the batch loads 0). Also > we need to make sure that userspace sets the right OACONTROL modes (not > the one which streams into a global gtt buffer essentially). So some > additional work required. Ok, I'll look into this. And apologies for not catching it myself. If we have to do additional checks on fields within the registers then I suppose we'll need to limit those registers to MI_LOAD_REGISTER_IMM. That might require separate whitelists for MI_LOAD_REGISTER_IMM/MEM. Not the end of the world, but certainly some additional complexity. For the resetting check, are there other registers in the current list that should have this tracking? If so, is 0 the reset value in all cases? Let me know if there is anything in the works that would require additional registers or different uses of any registers. Thanks, Brad > > I've added a FIXME comment to the code. > > Brad, can you please look into this, including the corresponding i-g-t > which tries to enable OA but doesn't disable it and one which tries to > enable it in the continuous mode (which writes to gtt)? Ken's previous > mails has the cmds mesa emits. > > Thanks, Daniel > > --- > > drivers/gpu/drm/i915/i915_cmd_parser.c | 1 + > > drivers/gpu/drm/i915/i915_reg.h | 2 ++ > > 2 files changed, 3 insertions(+) > > > > This patch needs to go before > > > > commit 6d42f94084b8c69813d7ecd0466c33fe561bf127 > > Author: Brad Volkin <bradley.d.volkin@intel.com> > > Date: Tue Feb 18 10:15:57 2014 -0800 > > > > drm/i915: Enable command parsing by default > > > > in whatever branch gets submitted to Dave Airlie. Or, that commit needs > > to be reverted. Otherwise, every OpenGL program will abort. Examples > > of programs that abort include GNOME, KDE, Firefox, and glxgears. > > > > diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i915/i915_cmd_parser.c > > index bae7c2f..d4a50b9 100644 > > --- a/drivers/gpu/drm/i915/i915_cmd_parser.c > > +++ b/drivers/gpu/drm/i915/i915_cmd_parser.c > > @@ -415,6 +415,7 @@ static const u32 gen7_render_regs[] = { > > GEN7_SO_WRITE_OFFSET(1), > > GEN7_SO_WRITE_OFFSET(2), > > GEN7_SO_WRITE_OFFSET(3), > > + OACONTROL, > > }; > > > > static const u32 gen7_blt_regs[] = { > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > > index 9f9e2b7..0ebc20d 100644 > > --- a/drivers/gpu/drm/i915/i915_reg.h > > +++ b/drivers/gpu/drm/i915/i915_reg.h > > @@ -427,6 +427,8 @@ > > /* There are the 4 64-bit counter registers, one for each stream output */ > > #define GEN7_SO_NUM_PRIMS_WRITTEN(n) (0x5200 + (n) * 8) > > > > +#define OACONTROL 0x2360 > > + > > #define _GEN7_PIPEA_DE_LOAD_SL 0x70068 > > #define _GEN7_PIPEB_DE_LOAD_SL 0x71068 > > #define GEN7_PIPE_DE_LOAD_SL(pipe) _PIPE(pipe, \ > > -- > > 1.9.0 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
On Wed, Mar 26, 2014 at 09:03:58AM -0700, Volkin, Bradley D wrote: > On Tue, Mar 25, 2014 at 11:21:23PM -0700, Daniel Vetter wrote: > > On Tue, Mar 25, 2014 at 10:52:03PM -0700, Kenneth Graunke wrote: > > > Mesa needs to be able to write OACONTROL in order to expose the > > > Observability Architecture's performance counters via OpenGL. > > > > > > Signed-off-by: Kenneth Graunke <kenneth@whitecape.org> > > > > Thanks a lot for quickly tracking this down. Now when we've talked about > > OA a little while ago we concluded that mesa should clear OACONTROL again > > before the batch ends to make sure that userspace can't unduly observe > > other processes. So I think it'd be worth to keep track of this with a > > flag (set when OACONTROL is != 0 and reset when the batch loads 0). Also > > we need to make sure that userspace sets the right OACONTROL modes (not > > the one which streams into a global gtt buffer essentially). So some > > additional work required. > > Ok, I'll look into this. And apologies for not catching it myself. > > If we have to do additional checks on fields within the registers then I > suppose we'll need to limit those registers to MI_LOAD_REGISTER_IMM. That > might require separate whitelists for MI_LOAD_REGISTER_IMM/MEM. Not the end > of the world, but certainly some additional complexity. > > For the resetting check, are there other registers in the current list that > should have this tracking? If so, is 0 the reset value in all cases? > > Let me know if there is anything in the works that would require additional > registers or different uses of any registers. Afaik there's no other register we want to reset again. I think all other register we might want to clear are already part of hw contexts, so no chance to leak stuff (e.g. the streamout registers and a end-of-pipe counters). Ken might know of something I've missed. -Daniel
On 03/26/2014 09:38 AM, Daniel Vetter wrote: > On Wed, Mar 26, 2014 at 09:03:58AM -0700, Volkin, Bradley D wrote: >> On Tue, Mar 25, 2014 at 11:21:23PM -0700, Daniel Vetter wrote: >>> On Tue, Mar 25, 2014 at 10:52:03PM -0700, Kenneth Graunke wrote: >>>> Mesa needs to be able to write OACONTROL in order to expose the >>>> Observability Architecture's performance counters via OpenGL. >>>> >>>> Signed-off-by: Kenneth Graunke <kenneth@whitecape.org> >>> >>> Thanks a lot for quickly tracking this down. Now when we've talked about >>> OA a little while ago we concluded that mesa should clear OACONTROL again >>> before the batch ends to make sure that userspace can't unduly observe >>> other processes. So I think it'd be worth to keep track of this with a >>> flag (set when OACONTROL is != 0 and reset when the batch loads 0). Also >>> we need to make sure that userspace sets the right OACONTROL modes (not >>> the one which streams into a global gtt buffer essentially). So some >>> additional work required. >> >> Ok, I'll look into this. And apologies for not catching it myself. >> >> If we have to do additional checks on fields within the registers then I >> suppose we'll need to limit those registers to MI_LOAD_REGISTER_IMM. That >> might require separate whitelists for MI_LOAD_REGISTER_IMM/MEM. Not the end >> of the world, but certainly some additional complexity. >> >> For the resetting check, are there other registers in the current list that >> should have this tracking? If so, is 0 the reset value in all cases? >> >> Let me know if there is anything in the works that would require additional >> registers or different uses of any registers. > > Afaik there's no other register we want to reset again. I think all other > register we might want to clear are already part of hw contexts, so no > chance to leak stuff (e.g. the streamout registers and a end-of-pipe > counters). Ken might know of something I've missed. > -Daniel Right, I think it's just OACONTROL. I don't think there's a need to filter particular values. I don't really buy the snooping problem, though...just because I leave OACONTROL set doesn't mean I'll get useful data. Another context might clobber it, and empirically the numbers seem to reset across RC6 anyway. So in actuality, they're likely to get bogus data. Even if they did somehow miraculously get decent values, it basically gives information akin to 'top', which is unprivileged on every system I've ever used. The other alternative is to have the kernel write OACONTROL to 0 after any batch that alters it. Then the kernel wouldn't need to care what userspace does with it. (I think Daniel preferred having userspace reset it, though.) --Ken
On Wed, Mar 26, 2014 at 10:37:44AM -0700, Kenneth Graunke wrote: > On 03/26/2014 09:38 AM, Daniel Vetter wrote: > > On Wed, Mar 26, 2014 at 09:03:58AM -0700, Volkin, Bradley D wrote: > >> On Tue, Mar 25, 2014 at 11:21:23PM -0700, Daniel Vetter wrote: > >>> On Tue, Mar 25, 2014 at 10:52:03PM -0700, Kenneth Graunke wrote: > >>>> Mesa needs to be able to write OACONTROL in order to expose the > >>>> Observability Architecture's performance counters via OpenGL. > >>>> > >>>> Signed-off-by: Kenneth Graunke <kenneth@whitecape.org> > >>> > >>> Thanks a lot for quickly tracking this down. Now when we've talked about > >>> OA a little while ago we concluded that mesa should clear OACONTROL again > >>> before the batch ends to make sure that userspace can't unduly observe > >>> other processes. So I think it'd be worth to keep track of this with a > >>> flag (set when OACONTROL is != 0 and reset when the batch loads 0). Also > >>> we need to make sure that userspace sets the right OACONTROL modes (not > >>> the one which streams into a global gtt buffer essentially). So some > >>> additional work required. > >> > >> Ok, I'll look into this. And apologies for not catching it myself. > >> > >> If we have to do additional checks on fields within the registers then I > >> suppose we'll need to limit those registers to MI_LOAD_REGISTER_IMM. That > >> might require separate whitelists for MI_LOAD_REGISTER_IMM/MEM. Not the end > >> of the world, but certainly some additional complexity. > >> > >> For the resetting check, are there other registers in the current list that > >> should have this tracking? If so, is 0 the reset value in all cases? > >> > >> Let me know if there is anything in the works that would require additional > >> registers or different uses of any registers. > > > > Afaik there's no other register we want to reset again. I think all other > > register we might want to clear are already part of hw contexts, so no > > chance to leak stuff (e.g. the streamout registers and a end-of-pipe > > counters). Ken might know of something I've missed. > > -Daniel > > Right, I think it's just OACONTROL. I don't think there's a need to > filter particular values. Sorry, I don't follow you here. Can you clarify what you mean r.e. filtering? > > I don't really buy the snooping problem, though...just because I leave > OACONTROL set doesn't mean I'll get useful data. Another context might > clobber it, and empirically the numbers seem to reset across RC6 anyway. > So in actuality, they're likely to get bogus data. > > Even if they did somehow miraculously get decent values, it basically > gives information akin to 'top', which is unprivileged on every system > I've ever used. I tend to agree with this assesment. The data doesn't seem particularly sensitive. Also, I assume this data is similar in nature to what you can already get from graphics perf tools, right? Thanks, Brad > > The other alternative is to have the kernel write OACONTROL to 0 after > any batch that alters it. Then the kernel wouldn't need to care what > userspace does with it. (I think Daniel preferred having userspace > reset it, though.) > > --Ken >
On Wed, Mar 26, 2014 at 11:26:05AM -0700, Volkin, Bradley D wrote: > On Wed, Mar 26, 2014 at 10:37:44AM -0700, Kenneth Graunke wrote: > > On 03/26/2014 09:38 AM, Daniel Vetter wrote: > > > On Wed, Mar 26, 2014 at 09:03:58AM -0700, Volkin, Bradley D wrote: > > >> On Tue, Mar 25, 2014 at 11:21:23PM -0700, Daniel Vetter wrote: > > >>> On Tue, Mar 25, 2014 at 10:52:03PM -0700, Kenneth Graunke wrote: > > >>>> Mesa needs to be able to write OACONTROL in order to expose the > > >>>> Observability Architecture's performance counters via OpenGL. > > >>>> > > >>>> Signed-off-by: Kenneth Graunke <kenneth@whitecape.org> > > >>> > > >>> Thanks a lot for quickly tracking this down. Now when we've talked about > > >>> OA a little while ago we concluded that mesa should clear OACONTROL again > > >>> before the batch ends to make sure that userspace can't unduly observe > > >>> other processes. So I think it'd be worth to keep track of this with a > > >>> flag (set when OACONTROL is != 0 and reset when the batch loads 0). Also > > >>> we need to make sure that userspace sets the right OACONTROL modes (not > > >>> the one which streams into a global gtt buffer essentially). So some > > >>> additional work required. > > >> > > >> Ok, I'll look into this. And apologies for not catching it myself. > > >> > > >> If we have to do additional checks on fields within the registers then I > > >> suppose we'll need to limit those registers to MI_LOAD_REGISTER_IMM. That > > >> might require separate whitelists for MI_LOAD_REGISTER_IMM/MEM. Not the end > > >> of the world, but certainly some additional complexity. > > >> > > >> For the resetting check, are there other registers in the current list that > > >> should have this tracking? If so, is 0 the reset value in all cases? > > >> > > >> Let me know if there is anything in the works that would require additional > > >> registers or different uses of any registers. > > > > > > Afaik there's no other register we want to reset again. I think all other > > > register we might want to clear are already part of hw contexts, so no > > > chance to leak stuff (e.g. the streamout registers and a end-of-pipe > > > counters). Ken might know of something I've missed. > > > -Daniel > > > > Right, I think it's just OACONTROL. I don't think there's a need to > > filter particular values. > > Sorry, I don't follow you here. Can you clarify what you mean r.e. filtering? > > > > > I don't really buy the snooping problem, though...just because I leave > > OACONTROL set doesn't mean I'll get useful data. Another context might > > clobber it, and empirically the numbers seem to reset across RC6 anyway. > > So in actuality, they're likely to get bogus data. > > > > Even if they did somehow miraculously get decent values, it basically > > gives information akin to 'top', which is unprivileged on every system > > I've ever used. > > I tend to agree with this assesment. The data doesn't seem particularly > sensitive. Also, I assume this data is similar in nature to what you can > already get from graphics perf tools, right? I've thought perf is priviledged ... And I've looked at the trigger based stuff again in Bspec and it looks like by default that's all disabled, so even when batches enable the timer nothing should happen. -Daniel
On 03/26/2014 11:26 AM, Volkin, Bradley D wrote: > On Wed, Mar 26, 2014 at 10:37:44AM -0700, Kenneth Graunke wrote: >> On 03/26/2014 09:38 AM, Daniel Vetter wrote: >>> On Wed, Mar 26, 2014 at 09:03:58AM -0700, Volkin, Bradley D wrote: >>>> On Tue, Mar 25, 2014 at 11:21:23PM -0700, Daniel Vetter wrote: >>>>> On Tue, Mar 25, 2014 at 10:52:03PM -0700, Kenneth Graunke wrote: >>>>>> Mesa needs to be able to write OACONTROL in order to expose the >>>>>> Observability Architecture's performance counters via OpenGL. >>>>>> >>>>>> Signed-off-by: Kenneth Graunke <kenneth@whitecape.org> >>>>> >>>>> Thanks a lot for quickly tracking this down. Now when we've talked about >>>>> OA a little while ago we concluded that mesa should clear OACONTROL again >>>>> before the batch ends to make sure that userspace can't unduly observe >>>>> other processes. So I think it'd be worth to keep track of this with a >>>>> flag (set when OACONTROL is != 0 and reset when the batch loads 0). Also >>>>> we need to make sure that userspace sets the right OACONTROL modes (not >>>>> the one which streams into a global gtt buffer essentially). So some >>>>> additional work required. >>>> >>>> Ok, I'll look into this. And apologies for not catching it myself. >>>> >>>> If we have to do additional checks on fields within the registers then I >>>> suppose we'll need to limit those registers to MI_LOAD_REGISTER_IMM. That >>>> might require separate whitelists for MI_LOAD_REGISTER_IMM/MEM. Not the end >>>> of the world, but certainly some additional complexity. >>>> >>>> For the resetting check, are there other registers in the current list that >>>> should have this tracking? If so, is 0 the reset value in all cases? >>>> >>>> Let me know if there is anything in the works that would require additional >>>> registers or different uses of any registers. >>> >>> Afaik there's no other register we want to reset again. I think all other >>> register we might want to clear are already part of hw contexts, so no >>> chance to leak stuff (e.g. the streamout registers and a end-of-pipe >>> counters). Ken might know of something I've missed. >>> -Daniel >> >> Right, I think it's just OACONTROL. I don't think there's a need to >> filter particular values. > > Sorry, I don't follow you here. Can you clarify what you mean r.e. filtering? Oh, I meant I don't think we need to do additional checks on "fields within the registers" (to quote your email) or restrict the values we can write to the registers. Just the ability to write the whole register to any arbitrary value should be sufficient. Thanks Brad! --Ken
On Wed, Mar 26, 2014 at 11:26:05AM -0700, Volkin, Bradley D wrote: > On Wed, Mar 26, 2014 at 10:37:44AM -0700, Kenneth Graunke wrote: > > On 03/26/2014 09:38 AM, Daniel Vetter wrote: > > > On Wed, Mar 26, 2014 at 09:03:58AM -0700, Volkin, Bradley D wrote: > > >> On Tue, Mar 25, 2014 at 11:21:23PM -0700, Daniel Vetter wrote: > > >>> On Tue, Mar 25, 2014 at 10:52:03PM -0700, Kenneth Graunke wrote: > > >>>> Mesa needs to be able to write OACONTROL in order to expose the > > >>>> Observability Architecture's performance counters via OpenGL. > > >>>> > > >>>> Signed-off-by: Kenneth Graunke <kenneth@whitecape.org> > > >>> > > >>> Thanks a lot for quickly tracking this down. Now when we've talked about > > >>> OA a little while ago we concluded that mesa should clear OACONTROL again > > >>> before the batch ends to make sure that userspace can't unduly observe > > >>> other processes. So I think it'd be worth to keep track of this with a > > >>> flag (set when OACONTROL is != 0 and reset when the batch loads 0). Also > > >>> we need to make sure that userspace sets the right OACONTROL modes (not > > >>> the one which streams into a global gtt buffer essentially). So some > > >>> additional work required. > > >> > > >> Ok, I'll look into this. And apologies for not catching it myself. > > >> > > >> If we have to do additional checks on fields within the registers then I > > >> suppose we'll need to limit those registers to MI_LOAD_REGISTER_IMM. That > > >> might require separate whitelists for MI_LOAD_REGISTER_IMM/MEM. Not the end > > >> of the world, but certainly some additional complexity. > > >> > > >> For the resetting check, are there other registers in the current list that > > >> should have this tracking? If so, is 0 the reset value in all cases? > > >> > > >> Let me know if there is anything in the works that would require additional > > >> registers or different uses of any registers. > > > > > > Afaik there's no other register we want to reset again. I think all other > > > register we might want to clear are already part of hw contexts, so no > > > chance to leak stuff (e.g. the streamout registers and a end-of-pipe > > > counters). Ken might know of something I've missed. > > > -Daniel > > > > Right, I think it's just OACONTROL. I don't think there's a need to > > filter particular values. > > Sorry, I don't follow you here. Can you clarify what you mean r.e. filtering? > > > > > I don't really buy the snooping problem, though...just because I leave > > OACONTROL set doesn't mean I'll get useful data. Another context might > > clobber it, and empirically the numbers seem to reset across RC6 anyway. > > So in actuality, they're likely to get bogus data. > > > > Even if they did somehow miraculously get decent values, it basically > > gives information akin to 'top', which is unprivileged on every system > > I've ever used. > > I tend to agree with this assesment. The data doesn't seem particularly > sensitive. Also, I assume this data is similar in nature to what you can > already get from graphics perf tools, right? Another one that blows is igt/gen7_forcewake_mt. Not sure yet whether it's an issue with the test or the checker: https://bugs.freedesktop.org/show_bug.cgi?id=76670 Cheers, Daniel
[snip] On Thu, Mar 27, 2014 at 12:57:21AM -0700, Daniel Vetter wrote: > Another one that blows is igt/gen7_forcewake_mt. Not sure yet whether it's > an issue with the test or the checker: > > https://bugs.freedesktop.org/show_bug.cgi?id=76670 For this one, the parser rejects an MI_STORE_REGISTER_MEM with the GGTT bit set. We don't currently allow that, even from master. It sounds like there might be released versions of the ddx that do this as well. If that's the case, or if there are other situations where tests, etc rely on being able to do whatever they want when setting the I915_DISPATCH_SECURE flag, then I think we might as well stop parsing secure batches and let them go through as before. Thanks, Brad > > Cheers, Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
On Thu, Mar 27, 2014 at 4:57 PM, Volkin, Bradley D <bradley.d.volkin@intel.com> wrote: > On Thu, Mar 27, 2014 at 12:57:21AM -0700, Daniel Vetter wrote: >> Another one that blows is igt/gen7_forcewake_mt. Not sure yet whether it's >> an issue with the test or the checker: >> >> https://bugs.freedesktop.org/show_bug.cgi?id=76670 > > For this one, the parser rejects an MI_STORE_REGISTER_MEM with the GGTT bit > set. We don't currently allow that, even from master. It sounds like there > might be released versions of the ddx that do this as well. If that's the > case, or if there are other situations where tests, etc rely on being able > to do whatever they want when setting the I915_DISPATCH_SECURE flag, then I > think we might as well stop parsing secure batches and let them go through > as before. Well for the testcase I think we can just add the missing flag. If there's indeed shipping userspace out there which is getting these flags wrong then I think we need to silently upgrade them when copying the cmds over to the 2nd batch. But I guess until that need is really established we can hope we don't need this. -Daniel
On 03/27/2014 01:16 PM, Daniel Vetter wrote: > On Thu, Mar 27, 2014 at 4:57 PM, Volkin, Bradley D > <bradley.d.volkin@intel.com> wrote: >> On Thu, Mar 27, 2014 at 12:57:21AM -0700, Daniel Vetter wrote: >>> Another one that blows is igt/gen7_forcewake_mt. Not sure yet whether it's >>> an issue with the test or the checker: >>> >>> https://bugs.freedesktop.org/show_bug.cgi?id=76670 >> >> For this one, the parser rejects an MI_STORE_REGISTER_MEM with the GGTT bit >> set. We don't currently allow that, even from master. It sounds like there >> might be released versions of the ddx that do this as well. If that's the >> case, or if there are other situations where tests, etc rely on being able >> to do whatever they want when setting the I915_DISPATCH_SECURE flag, then I >> think we might as well stop parsing secure batches and let them go through >> as before. > > Well for the testcase I think we can just add the missing flag. If > there's indeed shipping userspace out there which is getting these > flags wrong then I think we need to silently upgrade them when copying > the cmds over to the 2nd batch. But I guess until that need is really > established we can hope we don't need this. > -Daniel Why are we parsing batches with I915_EXEC_SECURE at all? Secure batches are only issued from trusted code which is guaranteed to be running as root. I don't see any benefit to scanning those batches, and there's definitely overhead. I mean, sure, it may be reasonable in the short term as a way to test the command parser, but I certainly hope we don't *ship* that. --Ken
On Thu, Mar 27, 2014 at 10:34 PM, Kenneth Graunke <kenneth@whitecape.org> wrote: > Why are we parsing batches with I915_EXEC_SECURE at all? Secure batches > are only issued from trusted code which is guaranteed to be running as > root. I don't see any benefit to scanning those batches, and there's > definitely overhead. > > I mean, sure, it may be reasonable in the short term as a way to test > the command parser, but I certainly hope we don't *ship* that. Everyone runs X as root, but I kinda want X to also be able to run as non-root. The cmd parser has a special list of drm master register lists which should allow this, but if we just bypass the cmd parser for all normal X installs we'll have 0 test coverage on this. Which means broken like hell. Hence I actually intend to ship this, yes. Chris doesn't like it either really. -Daniel
On 03/27/2014 03:44 PM, Daniel Vetter wrote: > On Thu, Mar 27, 2014 at 10:34 PM, Kenneth Graunke <kenneth@whitecape.org> wrote: >> Why are we parsing batches with I915_EXEC_SECURE at all? Secure batches >> are only issued from trusted code which is guaranteed to be running as >> root. I don't see any benefit to scanning those batches, and there's >> definitely overhead. >> >> I mean, sure, it may be reasonable in the short term as a way to test >> the command parser, but I certainly hope we don't *ship* that. > > Everyone runs X as root, but I kinda want X to also be able to run as > non-root. The cmd parser has a special list of drm master register > lists which should allow this, but if we just bypass the cmd parser > for all normal X installs we'll have 0 test coverage on this. Which > means broken like hell. > > Hence I actually intend to ship this, yes. Chris doesn't like it either really. > -Daniel Seriously? Hurt performance on every user's system just so you can test things? That a classic case of the tail wagging the dog. Why not make a i915.enable_cmd_parser=2 value which enables it all the time and use that when running igt? Clearly being able to test this is valuable, but enabling it universally is *not* OK.
On Thu, Mar 27, 2014 at 01:16:26PM -0700, Daniel Vetter wrote: > On Thu, Mar 27, 2014 at 4:57 PM, Volkin, Bradley D > <bradley.d.volkin@intel.com> wrote: > > On Thu, Mar 27, 2014 at 12:57:21AM -0700, Daniel Vetter wrote: > >> Another one that blows is igt/gen7_forcewake_mt. Not sure yet whether it's > >> an issue with the test or the checker: > >> > >> https://bugs.freedesktop.org/show_bug.cgi?id=76670 > > > > For this one, the parser rejects an MI_STORE_REGISTER_MEM with the GGTT bit > > set. We don't currently allow that, even from master. It sounds like there > > might be released versions of the ddx that do this as well. If that's the > > case, or if there are other situations where tests, etc rely on being able > > to do whatever they want when setting the I915_DISPATCH_SECURE flag, then I > > think we might as well stop parsing secure batches and let them go through > > as before. > > Well for the testcase I think we can just add the missing flag. If Which flag are you referring to here? Or are you just generally trying to say "modify the test to not break the rules"? > there's indeed shipping userspace out there which is getting these > flags wrong then I think we need to silently upgrade them when copying > the cmds over to the 2nd batch. But I guess until that need is really > established we can hope we don't need this. Chris, can you clarify whether shipping ddx sets GGTT bits this way? Thanks, Brad > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
On Thu, Mar 27, 2014 at 04:42:21PM -0700, Volkin, Bradley D wrote: > On Thu, Mar 27, 2014 at 01:16:26PM -0700, Daniel Vetter wrote: > > On Thu, Mar 27, 2014 at 4:57 PM, Volkin, Bradley D > > <bradley.d.volkin@intel.com> wrote: > > > On Thu, Mar 27, 2014 at 12:57:21AM -0700, Daniel Vetter wrote: > > >> Another one that blows is igt/gen7_forcewake_mt. Not sure yet whether it's > > >> an issue with the test or the checker: > > >> > > >> https://bugs.freedesktop.org/show_bug.cgi?id=76670 > > > > > > For this one, the parser rejects an MI_STORE_REGISTER_MEM with the GGTT bit > > > set. We don't currently allow that, even from master. It sounds like there > > > might be released versions of the ddx that do this as well. If that's the > > > case, or if there are other situations where tests, etc rely on being able > > > to do whatever they want when setting the I915_DISPATCH_SECURE flag, then I > > > think we might as well stop parsing secure batches and let them go through > > > as before. > > > > Well for the testcase I think we can just add the missing flag. If > > Which flag are you referring to here? Or are you just generally trying to say > "modify the test to not break the rules"? > > > there's indeed shipping userspace out there which is getting these > > flags wrong then I think we need to silently upgrade them when copying > > the cmds over to the 2nd batch. But I guess until that need is really > > established we can hope we don't need this. > > Chris, can you clarify whether shipping ddx sets GGTT bits this way? There have been no point releases with SRM (as far as I can remember) as no bug reporter said that they made any difference to their hangs. -Chris
On Thu, 27 Mar 2014 16:22:44 -0700 Kenneth Graunke <kenneth@whitecape.org> wrote: > On 03/27/2014 03:44 PM, Daniel Vetter wrote: > > On Thu, Mar 27, 2014 at 10:34 PM, Kenneth Graunke <kenneth@whitecape.org> wrote: > >> Why are we parsing batches with I915_EXEC_SECURE at all? Secure batches > >> are only issued from trusted code which is guaranteed to be running as > >> root. I don't see any benefit to scanning those batches, and there's > >> definitely overhead. > >> > >> I mean, sure, it may be reasonable in the short term as a way to test > >> the command parser, but I certainly hope we don't *ship* that. > > > > Everyone runs X as root, but I kinda want X to also be able to run as > > non-root. The cmd parser has a special list of drm master register > > lists which should allow this, but if we just bypass the cmd parser > > for all normal X installs we'll have 0 test coverage on this. Which > > means broken like hell. > > > > Hence I actually intend to ship this, yes. Chris doesn't like it either really. > > -Daniel > > Seriously? Hurt performance on every user's system just so you can test > things? That a classic case of the tail wagging the dog. > > Why not make a i915.enable_cmd_parser=2 value which enables it all the > time and use that when running igt? Clearly being able to test this is > valuable, but enabling it universally is *not* OK. Daniel, I'm not sure what you mean by 0 coverage. Surely DRI clients count for something? And X shouldn't be submitting all its batches with the secure bit set, right? If so, we ought to fix that and only use it for ones where it's necessary (e.g. wait events or similar). I agree with Ken and Chris here. Chris?
On Fri, May 16, 2014 at 12:05:45PM -0700, Jesse Barnes wrote: > On Thu, 27 Mar 2014 16:22:44 -0700 > Kenneth Graunke <kenneth@whitecape.org> wrote: > > > On 03/27/2014 03:44 PM, Daniel Vetter wrote: > > > On Thu, Mar 27, 2014 at 10:34 PM, Kenneth Graunke <kenneth@whitecape.org> wrote: > > >> Why are we parsing batches with I915_EXEC_SECURE at all? Secure batches > > >> are only issued from trusted code which is guaranteed to be running as > > >> root. I don't see any benefit to scanning those batches, and there's > > >> definitely overhead. > > >> > > >> I mean, sure, it may be reasonable in the short term as a way to test > > >> the command parser, but I certainly hope we don't *ship* that. > > > > > > Everyone runs X as root, but I kinda want X to also be able to run as > > > non-root. The cmd parser has a special list of drm master register > > > lists which should allow this, but if we just bypass the cmd parser > > > for all normal X installs we'll have 0 test coverage on this. Which > > > means broken like hell. > > > > > > Hence I actually intend to ship this, yes. Chris doesn't like it either really. > > > -Daniel > > > > Seriously? Hurt performance on every user's system just so you can test > > things? That a classic case of the tail wagging the dog. > > > > Why not make a i915.enable_cmd_parser=2 value which enables it all the > > time and use that when running igt? Clearly being able to test this is > > valuable, but enabling it universally is *not* OK. > > Daniel, I'm not sure what you mean by 0 coverage. Surely DRI clients > count for something? And X shouldn't be submitting all its batches > with the secure bit set, right? If so, we ought to fix that and only > use it for ones where it's necessary (e.g. wait events or similar). I > agree with Ken and Chris here. > > Chris? We haven't even fixed the major regression from enabling FBC. What's another massive slowdown? Yes, X only sets the secure bit when it pokes the display registers, and those registers should be privileged even with a cmd parser in place (which they are). Daniel's argument presumes that we haven't been patching out the cmd parser all this time anyway. -Chris
On Fri, 16 May 2014 20:20:50 +0100 Chris Wilson <chris@chris-wilson.co.uk> wrote: > On Fri, May 16, 2014 at 12:05:45PM -0700, Jesse Barnes wrote: > > On Thu, 27 Mar 2014 16:22:44 -0700 > > Kenneth Graunke <kenneth@whitecape.org> wrote: > > > > > On 03/27/2014 03:44 PM, Daniel Vetter wrote: > > > > On Thu, Mar 27, 2014 at 10:34 PM, Kenneth Graunke <kenneth@whitecape.org> wrote: > > > >> Why are we parsing batches with I915_EXEC_SECURE at all? Secure batches > > > >> are only issued from trusted code which is guaranteed to be running as > > > >> root. I don't see any benefit to scanning those batches, and there's > > > >> definitely overhead. > > > >> > > > >> I mean, sure, it may be reasonable in the short term as a way to test > > > >> the command parser, but I certainly hope we don't *ship* that. > > > > > > > > Everyone runs X as root, but I kinda want X to also be able to run as > > > > non-root. The cmd parser has a special list of drm master register > > > > lists which should allow this, but if we just bypass the cmd parser > > > > for all normal X installs we'll have 0 test coverage on this. Which > > > > means broken like hell. > > > > > > > > Hence I actually intend to ship this, yes. Chris doesn't like it either really. > > > > -Daniel > > > > > > Seriously? Hurt performance on every user's system just so you can test > > > things? That a classic case of the tail wagging the dog. > > > > > > Why not make a i915.enable_cmd_parser=2 value which enables it all the > > > time and use that when running igt? Clearly being able to test this is > > > valuable, but enabling it universally is *not* OK. > > > > Daniel, I'm not sure what you mean by 0 coverage. Surely DRI clients > > count for something? And X shouldn't be submitting all its batches > > with the secure bit set, right? If so, we ought to fix that and only > > use it for ones where it's necessary (e.g. wait events or similar). I > > agree with Ken and Chris here. > > > > Chris? > > We haven't even fixed the major regression from enabling FBC. What's > another massive slowdown? I thought you had that fixed in the X driver by avoiding front buffer rendering altogether. If that's the case we just need an ioctl to opt out of front buffer tracking, right? Presumably that flag would follow the current DRM_MASTER process... > Yes, X only sets the secure bit when it pokes the display registers, and > those registers should be privileged even with a cmd parser in place > (which they are). > > Daniel's argument presumes that we haven't been patching out the > cmd parser all this time anyway. Yeah I know we have some perf issues as it is; it would be nice if the overhead were so minimal that it didn't matter. But just on principle, scanning secure buffers seems wrong, and I'm trying to understand why Daniel would want it.
On Fri, May 16, 2014 at 12:34:08PM -0700, Jesse Barnes wrote: > On Fri, 16 May 2014 20:20:50 +0100 > Chris Wilson <chris@chris-wilson.co.uk> wrote: > > We haven't even fixed the major regression from enabling FBC. What's > > another massive slowdown? > > I thought you had that fixed in the X driver by avoiding front buffer > rendering altogether. If that's the case we just need an ioctl to opt > out of front buffer tracking, right? Presumably that flag would follow > the current DRM_MASTER process... No. All rendering triggers FBC updates. Ville has patches to fix the majority of that with only nuking FBC when front buffer rendering. (Note that games aren't usually affected by this because FBC is disabled by pageflipping.) The overhead could probably be reduced further by periodic nuking the FBC like (ideally) PSR. And yes X can avoid front buffer rendering altogether. The remaining challenge is to know when to enable it (the kernel doesn't give us any information about FBC or PSR after setting a mode) and when not, i.e. there is already a pageflipping compositor. -Chris
On Fri, 16 May 2014 12:34:08 -0700 Jesse Barnes <jbarnes@virtuousgeek.org> wrote: > On Fri, 16 May 2014 20:20:50 +0100 > Chris Wilson <chris@chris-wilson.co.uk> wrote: > > Yes, X only sets the secure bit when it pokes the display registers, and > > those registers should be privileged even with a cmd parser in place > > (which they are). > > > > Daniel's argument presumes that we haven't been patching out the > > cmd parser all this time anyway. > > Yeah I know we have some perf issues as it is; it would be nice if the > overhead were so minimal that it didn't matter. But just on principle, > scanning secure buffers seems wrong, and I'm trying to understand why > Daniel would want it. Ok Daniel explained on IRC that we actually have a special whitelist for the secure batch case. The idea is to allow a DRM_MASTER to submit secure batches, but still prevent a local root exploit. I suppose that means preventing access to most commands and registers, but allowing a few extra things like wait events and display register updates. I suppose it's not entirely unreasonable, but it does add complexity to the scanner and overhead to all users; not sure it's worth it.
On Fri, May 16, 2014 at 12:53:30PM -0700, Jesse Barnes wrote: > On Fri, 16 May 2014 12:34:08 -0700 > Jesse Barnes <jbarnes@virtuousgeek.org> wrote: > > > On Fri, 16 May 2014 20:20:50 +0100 > > Chris Wilson <chris@chris-wilson.co.uk> wrote: > > > Yes, X only sets the secure bit when it pokes the display registers, and > > > those registers should be privileged even with a cmd parser in place > > > (which they are). > > > > > > Daniel's argument presumes that we haven't been patching out the > > > cmd parser all this time anyway. > > > > Yeah I know we have some perf issues as it is; it would be nice if the > > overhead were so minimal that it didn't matter. But just on principle, > > scanning secure buffers seems wrong, and I'm trying to understand why > > Daniel would want it. > > Ok Daniel explained on IRC that we actually have a special whitelist > for the secure batch case. The idea is to allow a DRM_MASTER to submit > secure batches, but still prevent a local root exploit. I suppose that > means preventing access to most commands and registers, but allowing a > few extra things like wait events and display register updates. Just to clarify further: the additional register whitelist and commands are only based on DRM_MASTER. Setting I915_EXEC_SECURE is not required. So I suppose we could stop scanning batches that have I915_EXEC_SECURE and userspace could stop sending such batches when the parser is fully enabled. Brad > > I suppose it's not entirely unreasonable, but it does add complexity to > the scanner and overhead to all users; not sure it's worth it. > > -- > Jesse Barnes, Intel Open Source Technology Center > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Fri, 16 May 2014 20:49:30 +0100 Chris Wilson <chris@chris-wilson.co.uk> wrote: > On Fri, May 16, 2014 at 12:34:08PM -0700, Jesse Barnes wrote: > > On Fri, 16 May 2014 20:20:50 +0100 > > Chris Wilson <chris@chris-wilson.co.uk> wrote: > > > We haven't even fixed the major regression from enabling FBC. What's > > > another massive slowdown? > > > > I thought you had that fixed in the X driver by avoiding front buffer > > rendering altogether. If that's the case we just need an ioctl to opt > > out of front buffer tracking, right? Presumably that flag would follow > > the current DRM_MASTER process... > > No. All rendering triggers FBC updates. Ville has patches to fix the > majority of that with only nuking FBC when front buffer rendering. (Note > that games aren't usually affected by this because FBC is disabled by > pageflipping.) The overhead could probably be reduced further by periodic > nuking the FBC like (ideally) PSR. And yes X can avoid front buffer > rendering altogether. The remaining challenge is to know when to enable it > (the kernel doesn't give us any information about FBC or PSR after setting a > mode) and when not, i.e. there is already a pageflipping compositor. I thought there was a control bit for when the nuke occurred? If not, yeah I guess we have to go with a sw approach...
On Fri, 16 May 2014 13:12:27 -0700 "Volkin, Bradley D" <bradley.d.volkin@intel.com> wrote: > On Fri, May 16, 2014 at 12:53:30PM -0700, Jesse Barnes wrote: > > On Fri, 16 May 2014 12:34:08 -0700 > > Jesse Barnes <jbarnes@virtuousgeek.org> wrote: > > > > > On Fri, 16 May 2014 20:20:50 +0100 > > > Chris Wilson <chris@chris-wilson.co.uk> wrote: > > > > Yes, X only sets the secure bit when it pokes the display registers, and > > > > those registers should be privileged even with a cmd parser in place > > > > (which they are). > > > > > > > > Daniel's argument presumes that we haven't been patching out the > > > > cmd parser all this time anyway. > > > > > > Yeah I know we have some perf issues as it is; it would be nice if the > > > overhead were so minimal that it didn't matter. But just on principle, > > > scanning secure buffers seems wrong, and I'm trying to understand why > > > Daniel would want it. > > > > Ok Daniel explained on IRC that we actually have a special whitelist > > for the secure batch case. The idea is to allow a DRM_MASTER to submit > > secure batches, but still prevent a local root exploit. I suppose that > > means preventing access to most commands and registers, but allowing a > > few extra things like wait events and display register updates. > > Just to clarify further: the additional register whitelist and commands > are only based on DRM_MASTER. Setting I915_EXEC_SECURE is not required. So > I suppose we could stop scanning batches that have I915_EXEC_SECURE and > userspace could stop sending such batches when the parser is fully enabled. Ah ok, yeah that's another option, but now I understand where Daniel is coming from with testing, since that's not how the current X driver behaves.
diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i915/i915_cmd_parser.c index bae7c2f..d4a50b9 100644 --- a/drivers/gpu/drm/i915/i915_cmd_parser.c +++ b/drivers/gpu/drm/i915/i915_cmd_parser.c @@ -415,6 +415,7 @@ static const u32 gen7_render_regs[] = { GEN7_SO_WRITE_OFFSET(1), GEN7_SO_WRITE_OFFSET(2), GEN7_SO_WRITE_OFFSET(3), + OACONTROL, }; static const u32 gen7_blt_regs[] = { diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 9f9e2b7..0ebc20d 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -427,6 +427,8 @@ /* There are the 4 64-bit counter registers, one for each stream output */ #define GEN7_SO_NUM_PRIMS_WRITTEN(n) (0x5200 + (n) * 8) +#define OACONTROL 0x2360 + #define _GEN7_PIPEA_DE_LOAD_SL 0x70068 #define _GEN7_PIPEB_DE_LOAD_SL 0x71068 #define GEN7_PIPE_DE_LOAD_SL(pipe) _PIPE(pipe, \
Mesa needs to be able to write OACONTROL in order to expose the Observability Architecture's performance counters via OpenGL. Signed-off-by: Kenneth Graunke <kenneth@whitecape.org> --- drivers/gpu/drm/i915/i915_cmd_parser.c | 1 + drivers/gpu/drm/i915/i915_reg.h | 2 ++ 2 files changed, 3 insertions(+) This patch needs to go before commit 6d42f94084b8c69813d7ecd0466c33fe561bf127 Author: Brad Volkin <bradley.d.volkin@intel.com> Date: Tue Feb 18 10:15:57 2014 -0800 drm/i915: Enable command parsing by default in whatever branch gets submitted to Dave Airlie. Or, that commit needs to be reverted. Otherwise, every OpenGL program will abort. Examples of programs that abort include GNOME, KDE, Firefox, and glxgears.