[Mesa-dev] [PATCH] i965: Allow 48-bit addressing on Gen8+.
Kenneth Graunke
kenneth at whitecape.org
Tue Feb 27 20:56:29 UTC 2018
On Tuesday, February 27, 2018 12:35:32 AM PST Chris Wilson wrote:
> Quoting Kenneth Graunke (2018-02-27 00:05:46)
> > +static bool
> > +gem_supports_48b_addresses(int fd)
> > +{
> > + struct drm_i915_gem_exec_object2 obj = {
> > + .flags = EXEC_OBJECT_SUPPORTS_48B_ADDRESS,
> > + };
> > +
> > + struct drm_i915_gem_execbuffer2 execbuf = {
> > + .buffers_ptr = (uintptr_t)&obj,
> > + .buffer_count = 1,
> > + .rsvd1 = 0xffffffu,
> > + };
> > +
> > + int ret = drmIoctl(fd, DRM_IOCTL_I915_GEM_EXECBUFFER2, &execbuf);
> > +
> > + return ret == -1 && errno == ENOENT;
>
> Note that this reports ENOENT due to the unknown ctx id which is
> evaluated before the object now. But since you are only looking for the
> flag, that's ok as the behaviour changed after the flag was introduced.
Alright. I just stole this from anv, so I figured it was correct.
> > +}
> > +
> > /**
> > * Initializes the GEM buffer manager, which uses the kernel to allocate, map,
> > * and manage map buffer objections.
> > @@ -1418,6 +1439,8 @@ brw_bufmgr_init(struct gen_device_info *devinfo, int fd)
> >
> > bufmgr->has_llc = devinfo->has_llc;
> > bufmgr->has_mmap_wc = gem_param(fd, I915_PARAM_MMAP_VERSION) > 0;
> > + bufmgr->supports_48b_addresses =
> > + devinfo->gen >= 8 && gem_supports_48b_addresses(fd);
> >
> > init_cache_buckets(bufmgr);
> >
> > diff --git a/src/mesa/drivers/dri/i965/brw_misc_state.c b/src/mesa/drivers/dri/i965/brw_misc_state.c
> > index c4ef6812bff..29d74876c27 100644
> > --- a/src/mesa/drivers/dri/i965/brw_misc_state.c
> > +++ b/src/mesa/drivers/dri/i965/brw_misc_state.c
> > @@ -634,6 +634,12 @@ brw_upload_state_base_address(struct brw_context *brw)
> > }
> >
> > if (devinfo->gen >= 8) {
> > + /* STATE_BASE_ADDRESS has issues with 48-bit address spaces. If the
> > + * address + size as seen by STATE_BASE_ADDRESS overflows 48 bits,
> > + * the GPU appears to treat all accesses to the buffer as being out
> > + * of bounds and returns zero. To work around this, we pin all SBAs
> > + * to the bottom 4GB.
> > + */
>
> We could do with a quick explanation as to why we can't program the max
> size. Aiui, it's because of dynamic resize and so you cannot program
> here the size of object as reported to the kernel.
I don't think it has anything to do with dynamic resize. I think Jason
told me that it seems to compute an "upper bound address" by doing base
address + size and if that's at the very end, it can wrap from 2**48 to
0, causing the upper bound to be less than the base, at which point it
assumes zero size and all data reads back as 0.
If that's the actual problem, we don't need to restrict it to 4GB, we
just need to restrict it away from the very end...
> > diff --git a/src/mesa/drivers/dri/i965/intel_batchbuffer.c b/src/mesa/drivers/dri/i965/intel_batchbuffer.c > index 26718e0d1a2..0a8d3a80b64 100644
> > --- a/src/mesa/drivers/dri/i965/intel_batchbuffer.c
> > +++ b/src/mesa/drivers/dri/i965/intel_batchbuffer.c
> > @@ -1093,6 +1093,21 @@ emit_reloc(struct intel_batchbuffer *batch,
> > unsigned int index = add_exec_bo(batch, target);
> > struct drm_i915_gem_exec_object2 *entry = &batch->validation_list[index];
> >
> > + if (reloc_flags & RELOC_32BIT) {
> > + /* Restrict this buffer to the low 32 bits of the address space.
> > + *
> > + * Altering the validation list flags restricts it for this batch,
> > + * but we also alter the BO's kflags to restrict it permanently
> > + * (until the BO is destroyed and put back in the cache). Buffers
> > + * may stay bound across batches, and we want keep it constrained.
> > + */
>
> Hmm, I think the intent is more along the lines of the buffer may be
> reused for non-r32b targets between batches, but we want to avoid
> ping-pong migrations into r32b (as moving an active buffer will stall,
> hmm, can fix if needs must), so once we go r32b we never go back.
Yeah, that's a better explanation. How about:
/* Restrict this buffer to the low 32 bits of the address space,
* both now, and forever. Altering the validation list's flags
* restricts it for this batch, but we also modify the BO's kflags
* to restrict it permanently. This avoids ping-ponging the BO
* between the low 32 bits and the full address space. Moving an
* active buffer can cause stalls.
*/
> > + target->kflags &= ~EXEC_OBJECT_SUPPORTS_48B_ADDRESS;
> > + entry->flags &= ~EXEC_OBJECT_SUPPORTS_48B_ADDRESS;
> > +
> > + /* RELOC_32BIT is not an EXEC_OBJECT_* flag, so get rid of it. */
> > + reloc_flags &= ~RELOC_32BIT;
> > + }
> > +
> > if (reloc_flags)
> > entry->flags |= reloc_flags & batch->valid_reloc_flags;
>
> > diff --git a/src/mesa/drivers/dri/i965/brw_bufmgr.c b/src/mesa/drivers/dri/i965/brw_bufmgr.c
> > index fb180289a0c..2e54adb3ed2 100644
> > --- a/src/mesa/drivers/dri/i965/brw_bufmgr.c
> > +++ b/src/mesa/drivers/dri/i965/brw_bufmgr.c
> > @@ -119,6 +119,7 @@ struct brw_bufmgr {
> > bool has_llc:1;
> > bool has_mmap_wc:1;
> > bool bo_reuse:1;
> > + bool supports_48b_addresses:1;
> > };
> >
> > static int bo_set_tiling_internal(struct brw_bo *bo, uint32_t tiling_mode,
> > @@ -409,6 +410,8 @@ retry:
> > bo->reusable = true;
> > bo->cache_coherent = bufmgr->has_llc;
> > bo->index = -1;
> > + if (bufmgr->supports_48b_addresses)
> > + bo->kflags = EXEC_OBJECT_SUPPORTS_48B_ADDRESS;
>
> bo->kflags = bufmgr->supports_48b_addresses << 3 ?
> (hidden behind a bit of syntactic sugar)
> -Chris
I don't see how that's clearer than using the actual EXEC_OBJECT_*
define from the kernel uABI, or what the point of the new syntatic
sugar would be...this is already only two lines of obvious code...
--Ken
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180227/725eb834/attachment.sig>
More information about the mesa-dev
mailing list