| Message ID | 1512460094-4615-4-git-send-email-kevin.rogovin@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Quoting kevin.rogovin@intel.com (2017-12-05 07:48:14) > From: Kevin Rogovin <kevin.rogovin@intel.com> > > --- > src/mesa/drivers/dri/i965/intel_batchbuffer.c | 27 ++++++++++++++++++++++++++- > 1 file changed, 26 insertions(+), 1 deletion(-) > > diff --git a/src/mesa/drivers/dri/i965/intel_batchbuffer.c b/src/mesa/drivers/dri/i965/intel_batchbuffer.c > index 216073129b..53b3eaf49b 100644 > --- a/src/mesa/drivers/dri/i965/intel_batchbuffer.c > +++ b/src/mesa/drivers/dri/i965/intel_batchbuffer.c > @@ -804,7 +804,8 @@ static int > submit_batch(struct brw_context *brw, int in_fence_fd, int *out_fence_fd) > { > const struct gen_device_info *devinfo = &brw->screen->devinfo; > - __DRIscreen *dri_screen = brw->screen->driScrnPriv; > + struct intel_screen *screen = brw->screen; > + __DRIscreen *dri_screen = screen->driScrnPriv; > struct intel_batchbuffer *batch = &brw->batch; > int ret = 0; > > @@ -875,10 +876,34 @@ submit_batch(struct brw_context *brw, int in_fence_fd, int *out_fence_fd) > batch->validation_list[index] = tmp; > } > > + if (unlikely(screen->debug_batchbuffer.enabled)) { > + simple_mtx_lock(&screen->debug_batchbuffer.mutex); > + } Per context, then you can remove the locking. It's fitting since the scratch page is per-context anyway. > + > ret = execbuffer(dri_screen->fd, batch, hw_ctx, > 4 * USED_BATCH(*batch), > in_fence_fd, out_fence_fd, flags); > > + if (unlikely(screen->debug_batchbuffer.enabled)) { > + struct drm_i915_scratch_page sc; > + int ret; Tie this into INTEL_DEBUG & SYNC, then you can do all the synchronous operations in one place. -Chris
Hi,
> Per context, then you can remove the locking. It's fitting since the scratch page is per-context anyway.
The scratch page is per context? This I did not know, I thought it was per PPGTT. If that is the case, then my proposed interface to get/set the scratch page contents is wrong because it does not pass the HW context id.
-Kevin
Quoting Rogovin, Kevin (2017-12-05 10:30:04) > Hi, > > > Per context, then you can remove the locking. It's fitting since the scratch page is per-context anyway. > > The scratch page is per context? This I did not know, I thought it was per PPGTT. If that is the case, then my proposed interface to get/set the scratch page contents is wrong because it does not pass the HW context id. Yes, it per-vm, which is per-ctx on everything you want to investigate on. gen4-7 it is a global GTT with a global scratch, and just a mutex inside one process is not going to give you atomicity. -Chris
Thanks, I will make a v2 and post it shortly to correct for my terribly embarrassing omission caused by even more terribly embarrassing ignorance. -Kevin -----Original Message----- From: Chris Wilson [mailto:chris@chris-wilson.co.uk] Sent: Tuesday, December 5, 2017 12:39 PM To: Rogovin, Kevin <kevin.rogovin@intel.com>; intel-gfx@lists.freedesktop.org Subject: RE: [Intel-gfx] [PATCH 3/3] i965: check scratch page in a locked fashion on each ioctl Quoting Rogovin, Kevin (2017-12-05 10:30:04) > Hi, > > > Per context, then you can remove the locking. It's fitting since the scratch page is per-context anyway. > > The scratch page is per context? This I did not know, I thought it was per PPGTT. If that is the case, then my proposed interface to get/set the scratch page contents is wrong because it does not pass the HW context id. Yes, it per-vm, which is per-ctx on everything you want to investigate on. gen4-7 it is a global GTT with a global scratch, and just a mutex inside one process is not going to give you atomicity. -Chris
On Tue, 2017-12-05 at 10:41 +0000, Rogovin, Kevin wrote: > Thanks, I will make a v2 and post it shortly to correct for my > terribly embarrassing omission caused by even more terribly > embarrassing ignorance. To avoid v3, do concept the code around suggested existing I915_GEM_CONTEXT_GETPARAM and I915_GEM_CONTEXT_SETPARAM ioctls. Just add a new parameter I915_CONTEXT_PARAM_SCRATCH_PAGE. The interface should become pretty bullet-proof that way. Regards, Joonas > > -Kevin > > -----Original Message----- > From: Chris Wilson [mailto:chris@chris-wilson.co.uk] > Sent: Tuesday, December 5, 2017 12:39 PM > To: Rogovin, Kevin <kevin.rogovin@intel.com>; intel-gfx@lists.freedes > ktop.org > Subject: RE: [Intel-gfx] [PATCH 3/3] i965: check scratch page in a > locked fashion on each ioctl > > Quoting Rogovin, Kevin (2017-12-05 10:30:04) > > Hi, > > > > > Per context, then you can remove the locking. It's fitting since > > > the scratch page is per-context anyway. > > > > The scratch page is per context? This I did not know, I thought it > > was per PPGTT. If that is the case, then my proposed interface to > > get/set the scratch page contents is wrong because it does not pass > > the HW context id. > > Yes, it per-vm, which is per-ctx on everything you want to > investigate on. gen4-7 it is a global GTT with a global scratch, and > just a mutex inside one process is not going to give you atomicity. > -Chris > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/src/mesa/drivers/dri/i965/intel_batchbuffer.c b/src/mesa/drivers/dri/i965/intel_batchbuffer.c index 216073129b..53b3eaf49b 100644 --- a/src/mesa/drivers/dri/i965/intel_batchbuffer.c +++ b/src/mesa/drivers/dri/i965/intel_batchbuffer.c @@ -804,7 +804,8 @@ static int submit_batch(struct brw_context *brw, int in_fence_fd, int *out_fence_fd) { const struct gen_device_info *devinfo = &brw->screen->devinfo; - __DRIscreen *dri_screen = brw->screen->driScrnPriv; + struct intel_screen *screen = brw->screen; + __DRIscreen *dri_screen = screen->driScrnPriv; struct intel_batchbuffer *batch = &brw->batch; int ret = 0; @@ -875,10 +876,34 @@ submit_batch(struct brw_context *brw, int in_fence_fd, int *out_fence_fd) batch->validation_list[index] = tmp; } + if (unlikely(screen->debug_batchbuffer.enabled)) { + simple_mtx_lock(&screen->debug_batchbuffer.mutex); + } + ret = execbuffer(dri_screen->fd, batch, hw_ctx, 4 * USED_BATCH(*batch), in_fence_fd, out_fence_fd, flags); + if (unlikely(screen->debug_batchbuffer.enabled)) { + struct drm_i915_scratch_page sc; + int ret; + + while (brw_bo_busy(batch->bo)) { + usleep(10); + } + + sc.buffer_size = screen->debug_batchbuffer.buffer_size; + sc.buffer_ptr = (__u64)(uintptr_t) screen->debug_batchbuffer.tmp; + + ret = drmIoctl(dri_screen->fd, DRM_IOCTL_I915_READ_SCRATCH_PAGE, &sc); + assert(ret == 0); + assert(sc.buffer_size == screen->debug_batchbuffer.buffer_size); + assert(memcmp(screen->debug_batchbuffer.tmp, + screen->debug_batchbuffer.noise_values, + screen->debug_batchbuffer.buffer_size) == 0); + simple_mtx_unlock(&screen->debug_batchbuffer.mutex); + } + throttle(brw); }
From: Kevin Rogovin <kevin.rogovin@intel.com> --- src/mesa/drivers/dri/i965/intel_batchbuffer.c | 27 ++++++++++++++++++++++++++- 1 file changed, 26 insertions(+), 1 deletion(-)