Re: [Intel-gfx] [PATCH] drm/i915: Add OACONTROL to the command parser register whitelist.
On Thu, 27 Mar 2014 16:22:44 -0700 Kenneth Graunke kenn...@whitecape.org wrote: On 03/27/2014 03:44 PM, Daniel Vetter wrote: On Thu, Mar 27, 2014 at 10:34 PM, Kenneth Graunke kenn...@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? -- Jesse Barnes, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Add OACONTROL to the command parser register whitelist.
On Fri, May 16, 2014 at 12:05:45PM -0700, Jesse Barnes wrote: On Thu, 27 Mar 2014 16:22:44 -0700 Kenneth Graunke kenn...@whitecape.org wrote: On 03/27/2014 03:44 PM, Daniel Vetter wrote: On Thu, Mar 27, 2014 at 10:34 PM, Kenneth Graunke kenn...@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 -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Add OACONTROL to the command parser register whitelist.
On Fri, 16 May 2014 20:20:50 +0100 Chris Wilson ch...@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 kenn...@whitecape.org wrote: On 03/27/2014 03:44 PM, Daniel Vetter wrote: On Thu, Mar 27, 2014 at 10:34 PM, Kenneth Graunke kenn...@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. -- Jesse Barnes, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Add OACONTROL to the command parser register whitelist.
On Fri, 16 May 2014 12:34:08 -0700 Jesse Barnes jbar...@virtuousgeek.org wrote: On Fri, 16 May 2014 20:20:50 +0100 Chris Wilson ch...@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. -- Jesse Barnes, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Add OACONTROL to the command parser register whitelist.
On Fri, May 16, 2014 at 12:53:30PM -0700, Jesse Barnes wrote: On Fri, 16 May 2014 12:34:08 -0700 Jesse Barnes jbar...@virtuousgeek.org wrote: On Fri, 16 May 2014 20:20:50 +0100 Chris Wilson ch...@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 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Add OACONTROL to the command parser register whitelist.
On Fri, 16 May 2014 20:49:30 +0100 Chris Wilson ch...@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 ch...@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... -- Jesse Barnes, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Add OACONTROL to the command parser register whitelist.
On Fri, 16 May 2014 13:12:27 -0700 Volkin, Bradley D bradley.d.vol...@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 jbar...@virtuousgeek.org wrote: On Fri, 16 May 2014 20:20:50 +0100 Chris Wilson ch...@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. -- Jesse Barnes, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Add OACONTROL to the command parser register whitelist.
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.vol...@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 -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Add OACONTROL to the command parser register whitelist.
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 kenn...@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 -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Add OACONTROL to the command parser register whitelist.
[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 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Add OACONTROL to the command parser register whitelist.
On Thu, Mar 27, 2014 at 4:57 PM, Volkin, Bradley D bradley.d.vol...@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 -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Add OACONTROL to the command parser register whitelist.
On 03/27/2014 01:16 PM, Daniel Vetter wrote: On Thu, Mar 27, 2014 at 4:57 PM, Volkin, Bradley D bradley.d.vol...@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 signature.asc Description: OpenPGP digital signature ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Add OACONTROL to the command parser register whitelist.
On Thu, Mar 27, 2014 at 10:34 PM, Kenneth Graunke kenn...@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 -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Add OACONTROL to the command parser register whitelist.
On 03/27/2014 03:44 PM, Daniel Vetter wrote: On Thu, Mar 27, 2014 at 10:34 PM, Kenneth Graunke kenn...@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. signature.asc Description: OpenPGP digital signature ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Add OACONTROL to the command parser register whitelist.
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.vol...@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 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Add OACONTROL to the command parser register whitelist.
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 kenn...@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.vol...@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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Add OACONTROL to the command parser register whitelist.
On Wed, 26 Mar 2014, Kenneth Graunke kenn...@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 kenn...@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.vol...@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
--
Jani Nikula, Intel Open Source Technology Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Add OACONTROL to the command parser register whitelist.
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 kenn...@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.vol...@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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Add OACONTROL to the command parser register whitelist.
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 kenn...@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 -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Add OACONTROL to the command parser register whitelist.
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 kenn...@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 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Add OACONTROL to the command parser register whitelist.
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 kenn...@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 -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Add OACONTROL to the command parser register whitelist.
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 kenn...@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 signature.asc Description: OpenPGP digital signature ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
