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

Daniel Vetter
2015-06-17T12:47:03Z

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