Re: [PATCH v2 17/18] drm/i915: Wa32bitGeneralStateOffset & Wa32bitInstructionBaseOffset

From: Daniel Vetter <daniel@ffwll.ch>
To: Chris Wilson <chris@chris-wilson.co.uk>,
	Michel Thierry <michel.thierry@intel.com>,
	intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH v2 17/18] drm/i915: Wa32bitGeneralStateOffset & Wa32bitInstructionBaseOffset
Date: Wed, 17 Jun 2015 14:49:47 +0200	[thread overview]
Message-ID: <20150617124947.GW23637@phenom.ffwll.local> (raw)
In-Reply-To: <20150610180903.GF12731@nuc-i3427.alporthouse.com>

On Wed, Jun 10, 2015 at 07:09:03PM +0100, Chris Wilson wrote:
> On Wed, Jun 10, 2015 at 05:46:54PM +0100, Michel Thierry wrote:
> > There are some allocations that must be only referenced by 32bit
> > offsets. To limit the chances of having the first 4GB already full,
> > objects not requiring this workaround use DRM_MM_SEARCH_BELOW/
> > DRM_MM_CREATE_TOP flags
> > 
> > User must pass I915_EXEC_SUPPORTS_48BADDRESS flag to indicate it can
> > be allocated above the 32b address range.
> 
> This should be a per-object flag not per-execbuffer.

We need both. This one to opt into the large address space, the per-object
one to apply the w/a. Also libdrm/mesa patches for this are still missing.
-Daniel

>  
> > The flag is ignored in 32b PPGTT.
> > 
> > v2: Changed flag logic from neeeds_32b, to supports_48b.
> > 
> > Signed-off-by: Michel Thierry <michel.thierry@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h            |  1 +
> >  drivers/gpu/drm/i915/i915_gem.c            | 19 ++++++++++++++--
> >  drivers/gpu/drm/i915/i915_gem_execbuffer.c | 36 +++++++++++++++++++++---------
> >  include/uapi/drm/i915_drm.h                |  4 +++-
> >  4 files changed, 47 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 95f59d3..d73dddb 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -2714,6 +2714,7 @@ void i915_gem_vma_destroy(struct i915_vma *vma);
> >  #define PIN_OFFSET_BIAS	(1<<3)
> >  #define PIN_USER	(1<<4)
> >  #define PIN_UPDATE	(1<<5)
> > +#define PIN_FULL_RANGE	(1<<6)
> 
> Halfway through our flags. We should get around to putting a reminder
> that 1<<11 is the last flag.
> 
> >  #define PIN_OFFSET_MASK (~4095)
> >  int __must_check
> >  i915_gem_object_pin(struct drm_i915_gem_object *obj,
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index 35690ef..e6325a4a 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -3672,6 +3672,8 @@ i915_gem_object_bind_to_vm(struct drm_i915_gem_object *obj,
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	u32 fence_alignment, unfenced_alignment;
> >  	u64 size, fence_size;
> > +	u32 search_flag = DRM_MM_SEARCH_DEFAULT;
> > +	u32 alloc_flag = DRM_MM_CREATE_DEFAULT;
> >  	u64 start =
> >  		flags & PIN_OFFSET_BIAS ? flags & PIN_OFFSET_MASK : 0;
> >  	u64 end =
> > @@ -3713,6 +3715,19 @@ i915_gem_object_bind_to_vm(struct drm_i915_gem_object *obj,
> >  						   obj->tiling_mode,
> >  						   false);
> >  		size = flags & PIN_MAPPABLE ? fence_size : obj->base.size;
> > +
> > +		/* Wa32bitGeneralStateOffset & Wa32bitInstructionBaseOffset,
> > +		 * limit address to 4GB-1 for objects requiring this wa; for
> > +		 * others, set alloc flag to TOP.
> > +		 */
> > +		if (USES_FULL_48BIT_PPGTT(dev)) {
> > +			if (flags & PIN_FULL_RANGE) {
> > +				search_flag = DRM_MM_SEARCH_BELOW;
> > +				alloc_flag = DRM_MM_CREATE_TOP;
> > +			} else {
> > +				end = ((4ULL << GEN8_PDPE_SHIFT) - 1);
> 
> Looking at this, I think this is better as two flags. I have used
> SEARCH_BELOW in the past to try and keep objects out of the mappable
> aperture. Having that as separate flag is quite useful in its own right.
> 
> Then the second flag is PIN_BELOW_4G which we can set by default (and
> cleared when the user specifies EXEC_OBJECT_48BIT). Not so sure, but I
> think with the right combination of flags you can avoid having device
> and w/a specific logic here. (This should be mechanism, push the policy
> out to the boundary, preferrably into userspace.)
> 
> All the i915_gem_execbuffer changes are wrong due to the flag not being
> on the object.
> -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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx