| Message ID | 20220210121313.701004-13-matthew.auld@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Initial support for small BAR recovery | expand |
On 2/10/22 13:13, Matthew Auld wrote: > Starting from DG2+, when dealing with LMEM, we assume that by default > all userspace allocations should be placed in the non-mappable portion > of LMEM. Note that dumb buffers are not included here, since these are > not "GPU accelerated" and likely need CPU access. > > In a later patch userspace will be able to provide a hint if CPU access > to the buffer is needed. > > Signed-off-by: Matthew Auld <matthew.auld@intel.com> > Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com> > --- > drivers/gpu/drm/i915/gem/i915_gem_create.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_create.c b/drivers/gpu/drm/i915/gem/i915_gem_create.c > index 9402d4bf4ffc..cc9ddb943f96 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_create.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_create.c > @@ -424,6 +424,15 @@ i915_gem_create_ext_ioctl(struct drm_device *dev, void *data, > ext_data.n_placements = 1; > } > > + /* > + * TODO: add a userspace hint to force CPU_ACCESS for the object, which > + * can override this. > + */ > + if (!IS_DG1(i915) && (ext_data.n_placements > 1 || > + ext_data.placements[0]->type != > + INTEL_MEMORY_SYSTEM)) > + ext_data.flags |= I915_BO_ALLOC_GPU_ONLY; > + WRT previous review comment here, it would be easier to follow if the bo was marked as a GPU only buffer regardless. Then for example capture and other functions where it actually matters can choose to take action based on, for example, whether the BAR is restricted or not? /Thomas > obj = __i915_gem_object_create_user_ext(i915, args->size, > ext_data.placements, > ext_data.n_placements,
On Fri, 11 Feb 2022 at 09:49, Thomas Hellström <thomas.hellstrom@linux.intel.com> wrote: > > > On 2/10/22 13:13, Matthew Auld wrote: > > Starting from DG2+, when dealing with LMEM, we assume that by default > > all userspace allocations should be placed in the non-mappable portion > > of LMEM. Note that dumb buffers are not included here, since these are > > not "GPU accelerated" and likely need CPU access. > > > > In a later patch userspace will be able to provide a hint if CPU access > > to the buffer is needed. > > > > Signed-off-by: Matthew Auld <matthew.auld@intel.com> > > Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com> > > --- > > drivers/gpu/drm/i915/gem/i915_gem_create.c | 9 +++++++++ > > 1 file changed, 9 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_create.c b/drivers/gpu/drm/i915/gem/i915_gem_create.c > > index 9402d4bf4ffc..cc9ddb943f96 100644 > > --- a/drivers/gpu/drm/i915/gem/i915_gem_create.c > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_create.c > > @@ -424,6 +424,15 @@ i915_gem_create_ext_ioctl(struct drm_device *dev, void *data, > > ext_data.n_placements = 1; > > } > > > > + /* > > + * TODO: add a userspace hint to force CPU_ACCESS for the object, which > > + * can override this. > > + */ > > + if (!IS_DG1(i915) && (ext_data.n_placements > 1 || > > + ext_data.placements[0]->type != > > + INTEL_MEMORY_SYSTEM)) > > + ext_data.flags |= I915_BO_ALLOC_GPU_ONLY; > > + > > WRT previous review comment here, it would be easier to follow if the bo > was marked as a GPU only buffer regardless. Then for example capture and > other functions where it actually matters can choose to take action > based on, for example, whether the BAR is restricted or not? Yeah, I completely forgot about this, sorry. Will fix now. > > /Thomas > > > > > obj = __i915_gem_object_create_user_ext(i915, args->size, > > ext_data.placements, > > ext_data.n_placements,
On 2/11/22 10:52, Matthew Auld wrote: > On Fri, 11 Feb 2022 at 09:49, Thomas Hellström > <thomas.hellstrom@linux.intel.com> wrote: >> >> On 2/10/22 13:13, Matthew Auld wrote: >>> Starting from DG2+, when dealing with LMEM, we assume that by default >>> all userspace allocations should be placed in the non-mappable portion >>> of LMEM. Note that dumb buffers are not included here, since these are >>> not "GPU accelerated" and likely need CPU access. >>> >>> In a later patch userspace will be able to provide a hint if CPU access >>> to the buffer is needed. >>> >>> Signed-off-by: Matthew Auld <matthew.auld@intel.com> >>> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com> >>> --- >>> drivers/gpu/drm/i915/gem/i915_gem_create.c | 9 +++++++++ >>> 1 file changed, 9 insertions(+) >>> >>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_create.c b/drivers/gpu/drm/i915/gem/i915_gem_create.c >>> index 9402d4bf4ffc..cc9ddb943f96 100644 >>> --- a/drivers/gpu/drm/i915/gem/i915_gem_create.c >>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_create.c >>> @@ -424,6 +424,15 @@ i915_gem_create_ext_ioctl(struct drm_device *dev, void *data, >>> ext_data.n_placements = 1; >>> } >>> >>> + /* >>> + * TODO: add a userspace hint to force CPU_ACCESS for the object, which >>> + * can override this. >>> + */ >>> + if (!IS_DG1(i915) && (ext_data.n_placements > 1 || >>> + ext_data.placements[0]->type != >>> + INTEL_MEMORY_SYSTEM)) >>> + ext_data.flags |= I915_BO_ALLOC_GPU_ONLY; >>> + >> WRT previous review comment here, it would be easier to follow if the bo >> was marked as a GPU only buffer regardless. Then for example capture and >> other functions where it actually matters can choose to take action >> based on, for example, whether the BAR is restricted or not? > Yeah, I completely forgot about this, sorry. Will fix now. Actually you did reply, but I forgot to reply to that :). /Thomas
On Fri, 11 Feb 2022 at 09:56, Thomas Hellström <thomas.hellstrom@linux.intel.com> wrote: > > > On 2/11/22 10:52, Matthew Auld wrote: > > On Fri, 11 Feb 2022 at 09:49, Thomas Hellström > > <thomas.hellstrom@linux.intel.com> wrote: > >> > >> On 2/10/22 13:13, Matthew Auld wrote: > >>> Starting from DG2+, when dealing with LMEM, we assume that by default > >>> all userspace allocations should be placed in the non-mappable portion > >>> of LMEM. Note that dumb buffers are not included here, since these are > >>> not "GPU accelerated" and likely need CPU access. > >>> > >>> In a later patch userspace will be able to provide a hint if CPU access > >>> to the buffer is needed. > >>> > >>> Signed-off-by: Matthew Auld <matthew.auld@intel.com> > >>> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com> > >>> --- > >>> drivers/gpu/drm/i915/gem/i915_gem_create.c | 9 +++++++++ > >>> 1 file changed, 9 insertions(+) > >>> > >>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_create.c b/drivers/gpu/drm/i915/gem/i915_gem_create.c > >>> index 9402d4bf4ffc..cc9ddb943f96 100644 > >>> --- a/drivers/gpu/drm/i915/gem/i915_gem_create.c > >>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_create.c > >>> @@ -424,6 +424,15 @@ i915_gem_create_ext_ioctl(struct drm_device *dev, void *data, > >>> ext_data.n_placements = 1; > >>> } > >>> > >>> + /* > >>> + * TODO: add a userspace hint to force CPU_ACCESS for the object, which > >>> + * can override this. > >>> + */ > >>> + if (!IS_DG1(i915) && (ext_data.n_placements > 1 || > >>> + ext_data.placements[0]->type != > >>> + INTEL_MEMORY_SYSTEM)) > >>> + ext_data.flags |= I915_BO_ALLOC_GPU_ONLY; > >>> + > >> WRT previous review comment here, it would be easier to follow if the bo > >> was marked as a GPU only buffer regardless. Then for example capture and > >> other functions where it actually matters can choose to take action > >> based on, for example, whether the BAR is restricted or not? > > Yeah, I completely forgot about this, sorry. Will fix now. > > Actually you did reply, but I forgot to reply to that :). Hmm, should we just drop the IS_DG1() check here(that was my first thought), or go further and still apply even regardless of placements? i.e it would be set on integrated > > /Thomas > >
On 2/11/22 11:00, Matthew Auld wrote: > On Fri, 11 Feb 2022 at 09:56, Thomas Hellström > <thomas.hellstrom@linux.intel.com> wrote: >> >> On 2/11/22 10:52, Matthew Auld wrote: >>> On Fri, 11 Feb 2022 at 09:49, Thomas Hellström >>> <thomas.hellstrom@linux.intel.com> wrote: >>>> On 2/10/22 13:13, Matthew Auld wrote: >>>>> Starting from DG2+, when dealing with LMEM, we assume that by default >>>>> all userspace allocations should be placed in the non-mappable portion >>>>> of LMEM. Note that dumb buffers are not included here, since these are >>>>> not "GPU accelerated" and likely need CPU access. >>>>> >>>>> In a later patch userspace will be able to provide a hint if CPU access >>>>> to the buffer is needed. >>>>> >>>>> Signed-off-by: Matthew Auld <matthew.auld@intel.com> >>>>> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com> >>>>> --- >>>>> drivers/gpu/drm/i915/gem/i915_gem_create.c | 9 +++++++++ >>>>> 1 file changed, 9 insertions(+) >>>>> >>>>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_create.c b/drivers/gpu/drm/i915/gem/i915_gem_create.c >>>>> index 9402d4bf4ffc..cc9ddb943f96 100644 >>>>> --- a/drivers/gpu/drm/i915/gem/i915_gem_create.c >>>>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_create.c >>>>> @@ -424,6 +424,15 @@ i915_gem_create_ext_ioctl(struct drm_device *dev, void *data, >>>>> ext_data.n_placements = 1; >>>>> } >>>>> >>>>> + /* >>>>> + * TODO: add a userspace hint to force CPU_ACCESS for the object, which >>>>> + * can override this. >>>>> + */ >>>>> + if (!IS_DG1(i915) && (ext_data.n_placements > 1 || >>>>> + ext_data.placements[0]->type != >>>>> + INTEL_MEMORY_SYSTEM)) >>>>> + ext_data.flags |= I915_BO_ALLOC_GPU_ONLY; >>>>> + >>>> WRT previous review comment here, it would be easier to follow if the bo >>>> was marked as a GPU only buffer regardless. Then for example capture and >>>> other functions where it actually matters can choose to take action >>>> based on, for example, whether the BAR is restricted or not? >>> Yeah, I completely forgot about this, sorry. Will fix now. >> Actually you did reply, but I forgot to reply to that :). > Hmm, should we just drop the IS_DG1() check here(that was my first > thought), or go further and still apply even regardless of placements? > i.e it would be set on integrated That was my first thought as well, but yes it makes sense to also drop the placement checks and let the placement selection logic handle that later? One alternative approach would also be to invert the thing and have a BO_ALLOC_CPU_REQUIRE, that is set by default on some bos and can be set on the others using the hint, but I figure that needs to be then set also on kernel-only buffer objects. Not sure what is simplest. /Thomas > >> /Thomas >> >>
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_create.c b/drivers/gpu/drm/i915/gem/i915_gem_create.c index 9402d4bf4ffc..cc9ddb943f96 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_create.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_create.c @@ -424,6 +424,15 @@ i915_gem_create_ext_ioctl(struct drm_device *dev, void *data, ext_data.n_placements = 1; } + /* + * TODO: add a userspace hint to force CPU_ACCESS for the object, which + * can override this. + */ + if (!IS_DG1(i915) && (ext_data.n_placements > 1 || + ext_data.placements[0]->type != + INTEL_MEMORY_SYSTEM)) + ext_data.flags |= I915_BO_ALLOC_GPU_ONLY; + obj = __i915_gem_object_create_user_ext(i915, args->size, ext_data.placements, ext_data.n_placements,
Starting from DG2+, when dealing with LMEM, we assume that by default all userspace allocations should be placed in the non-mappable portion of LMEM. Note that dumb buffers are not included here, since these are not "GPU accelerated" and likely need CPU access. In a later patch userspace will be able to provide a hint if CPU access to the buffer is needed. Signed-off-by: Matthew Auld <matthew.auld@intel.com> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com> --- drivers/gpu/drm/i915/gem/i915_gem_create.c | 9 +++++++++ 1 file changed, 9 insertions(+)