[Beignet] [PATCH] intel: Check that we can reserve the zero-offset

[Song, Ruiling]

> -----Original Message-----
> From: 'Chris Wilson' [mailto:chris at chris-wilson.co.uk]
> Sent: Sunday, November 20, 2016 8:45 PM
> To: Song, Ruiling <ruiling.song at intel.com>
> Cc: beignet at lists.freedesktop.org
> Subject: Re: [Beignet] [PATCH] intel: Check that we can reserve the zero-offset
> 
> On Sun, Nov 20, 2016 at 12:14:50PM +0000, Song, Ruiling wrote:
> >
> >
> > > -----Original Message-----
> > > From: Beignet [mailto:beignet-bounces at lists.freedesktop.org] On Behalf Of
> > > Chris Wilson
> > > Sent: Monday, November 14, 2016 9:35 PM
> > > To: beignet at lists.freedesktop.org
> > > Cc: Chris Wilson <chris at chris-wilson.co.uk>
> > > Subject: [Beignet] [PATCH] intel: Check that we can reserve the zero-offset
> > >
> > > commit ff57cee0519d ("ocl20/runtime: take the first 64KB page table
> > > entries") tries to allocate a bo at 0 offset, but failed to take into
> > > account that something may already be allocated there that it is not
> > > allowed to evict (particularly when not using full-ppgtt separation).
> > > Failure to do so causes all execution to subsequentally fail with
> > > "drm_intel_gem_bo_context_exec() failed: Device or resource busy"
> > >
> > > Reported-by: Kenneth Johansson <ken at kenjo.org>
> > > Fixes: https://bugs.freedesktop.org/show_bug.cgi?id=98647
> > > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > > ---
> > >  src/intel/intel_driver.c | 12 ++++++++----
> > >  1 file changed, 8 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/src/intel/intel_driver.c b/src/intel/intel_driver.c
> > > index 363e018..a6074ae 100644
> > > --- a/src/intel/intel_driver.c
> > > +++ b/src/intel/intel_driver.c
> > > @@ -143,10 +143,14 @@ intel_driver_context_init(intel_driver_t *driver)
> > >  #ifdef HAS_BO_SET_SOFTPIN
> > >    drm_intel_bo *bo = dri_bo_alloc(driver->bufmgr, "null_bo", 64*1024,
> 4096);
> > >    drm_intel_bo_set_softpin_offset(bo, 0);
> > > -  // don't reuse it, that would make two bo trying to bind to same address,
> > > -  // which is un-reasonable.
> > > -  drm_intel_bo_disable_reuse(bo);
> > Please don't remove the " drm_intel_bo_disable_reuse()". when the bo is
> reused, the soft-pinned flag is not cleared in libdrm.
> > So when it is reused as an ordinary bo(not soft-pinned), it will again ask the
> kmd to bind it to the old zero offset which is not as expected.
> 
> Then fix libdrm. Better yet, just don't use libdrm_intel. But that is a
> really nasty bug that should have been filed and fixed long ago.
> 
> > > -  driver->null_bo = bo;
> > > +  drm_intel_bo_map(bo, true);
> > > +  *(uint32_t *)bo->virtual = MI_BATCH_BUFFER_END;
> > > +  if (drm_intel_gem_bo_context_exec(bo, driver->ctx, 0, 0) == 0) {
> > > +    drm_intel_bo_map(bo, true);
> > > +    driver->null_bo = memset(bo->virtual, 0, 64*1024);
> > And the null_bo should points to the 'bo', but you let it points to bo->virtual,
> any reason?
> > I notice you mapped the bo twice without unmap. Do we need unmap() here?
> > I am sorry for the late reply.
> 
> No, thats just me trying to prevent an information leak at the same
> time.
Could you explain a little bit more? Why making the "null_bo points to bo->virtual" and "not doing unmap" would prevent information leak?

- Ruiling
> -Chris
> 
> --
> Chris Wilson, Intel Open Source Technology Centre