* [PATCH] drm/i915/gem/mman: only allow WC for lmem
@ 2021-06-02 9:36 Matthew Auld
2021-06-02 12:00 ` Thomas Hellström
0 siblings, 1 reply; 4+ messages in thread
From: Matthew Auld @ 2021-06-02 9:36 UTC (permalink / raw)
To: intel-gfx; +Cc: Thomas Hellström, dri-devel, Daniel Vetter
For dgfx where we now have lmem and ttm, we can only support single mmap
mode for the lifetime of the object, and for lmem objects this should be
WC, so reject all other mapping modes for mmap_offset, including if the
object can be placed in both smem and lmem.
Signed-off-by: Matthew Auld <matthew.auld@intel.com>
Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
---
drivers/gpu/drm/i915/gem/i915_gem_mman.c | 4 ++++
drivers/gpu/drm/i915/gem/i915_gem_object.c | 22 ++++++++++++++++++++++
drivers/gpu/drm/i915/gem/i915_gem_object.h | 4 ++++
3 files changed, 30 insertions(+)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
index fd1c9714f8d8..32f88f236771 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
@@ -689,6 +689,10 @@ __assign_mmap_offset(struct drm_file *file,
goto out;
}
+ if (mmap_type != I915_MMAP_TYPE_WC &&
+ i915_gem_object_placements_contain_type(obj, INTEL_MEMORY_LOCAL))
+ return -ENODEV;
+
mmo = mmap_offset_attach(obj, mmap_type, file);
if (IS_ERR(mmo)) {
err = PTR_ERR(mmo);
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c b/drivers/gpu/drm/i915/gem/i915_gem_object.c
index 2be6109d0093..d4b0da8ed969 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
@@ -403,6 +403,28 @@ int i915_gem_object_read_from_page(struct drm_i915_gem_object *obj, u64 offset,
return 0;
}
+/**
+ * i915_gem_object_placements_contain_type - Check whether the object can be
+ * placed at certain memory type
+ * @obj: Pointer to the object
+ * @type: The memory type to check
+ *
+ * Return: True if the object can be placed in @type. False otherwise.
+ */
+bool i915_gem_object_placements_contain_type(struct drm_i915_gem_object *obj,
+ enum intel_memory_type type)
+{
+ unsigned int i;
+
+ /* TODO: consider maybe storing as a mask when doing gem_create_ext */
+ for (i = 0; i < obj->mm.n_placements; i++) {
+ if (obj->mm.placements[i]->type == type)
+ return true;
+ }
+
+ return false;
+}
+
void i915_gem_init__objects(struct drm_i915_private *i915)
{
INIT_WORK(&i915->mm.free_work, __i915_gem_free_work);
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h b/drivers/gpu/drm/i915/gem/i915_gem_object.h
index 2ebd79537aea..4d6ea9e07df0 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_object.h
+++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h
@@ -12,6 +12,7 @@
#include <drm/drm_device.h>
#include "display/intel_frontbuffer.h"
+#include "intel_memory_region.h"
#include "i915_gem_object_types.h"
#include "i915_gem_gtt.h"
#include "i915_vma_types.h"
@@ -587,6 +588,9 @@ int i915_gem_object_read_from_page(struct drm_i915_gem_object *obj, u64 offset,
bool i915_gem_object_is_shmem(const struct drm_i915_gem_object *obj);
+bool i915_gem_object_placements_contain_type(struct drm_i915_gem_object *obj,
+ enum intel_memory_type type);
+
#ifdef CONFIG_MMU_NOTIFIER
static inline bool
i915_gem_object_is_userptr(struct drm_i915_gem_object *obj)
--
2.26.3
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] drm/i915/gem/mman: only allow WC for lmem
2021-06-02 9:36 [PATCH] drm/i915/gem/mman: only allow WC for lmem Matthew Auld
@ 2021-06-02 12:00 ` Thomas Hellström
2021-06-08 9:57 ` Matthew Auld
0 siblings, 1 reply; 4+ messages in thread
From: Thomas Hellström @ 2021-06-02 12:00 UTC (permalink / raw)
To: Matthew Auld, intel-gfx; +Cc: Daniel Vetter, dri-devel
Hi,
On 6/2/21 11:36 AM, Matthew Auld wrote:
> For dgfx where we now have lmem and ttm, we can only support single mmap
> mode for the lifetime of the object, and for lmem objects this should be
> WC, so reject all other mapping modes for mmap_offset, including if the
> object can be placed in both smem and lmem.
>
> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
> drivers/gpu/drm/i915/gem/i915_gem_mman.c | 4 ++++
> drivers/gpu/drm/i915/gem/i915_gem_object.c | 22 ++++++++++++++++++++++
> drivers/gpu/drm/i915/gem/i915_gem_object.h | 4 ++++
> 3 files changed, 30 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> index fd1c9714f8d8..32f88f236771 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> @@ -689,6 +689,10 @@ __assign_mmap_offset(struct drm_file *file,
> goto out;
> }
>
> + if (mmap_type != I915_MMAP_TYPE_WC &&
> + i915_gem_object_placements_contain_type(obj, INTEL_MEMORY_LOCAL))
> + return -ENODEV;
> +
I think we will also have the restriction that any other objects on DGFX
can only be mapped cached? At least that's what the TTM code is
implementing currently.
> mmo = mmap_offset_attach(obj, mmap_type, file);
> if (IS_ERR(mmo)) {
> err = PTR_ERR(mmo);
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c b/drivers/gpu/drm/i915/gem/i915_gem_object.c
> index 2be6109d0093..d4b0da8ed969 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
> @@ -403,6 +403,28 @@ int i915_gem_object_read_from_page(struct drm_i915_gem_object *obj, u64 offset,
> return 0;
> }
>
> +/**
> + * i915_gem_object_placements_contain_type - Check whether the object can be
> + * placed at certain memory type
> + * @obj: Pointer to the object
> + * @type: The memory type to check
> + *
> + * Return: True if the object can be placed in @type. False otherwise.
> + */
> +bool i915_gem_object_placements_contain_type(struct drm_i915_gem_object *obj,
> + enum intel_memory_type type)
> +{
> + unsigned int i;
> +
> + /* TODO: consider maybe storing as a mask when doing gem_create_ext */
> + for (i = 0; i < obj->mm.n_placements; i++) {
> + if (obj->mm.placements[i]->type == type)
> + return true;
> + }
> +
> + return false;
> +}
> +
Do we need something for the in-kernel mappings as well? Or just return
a mapping with the only allowed caching mode?
/Thomas
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] drm/i915/gem/mman: only allow WC for lmem
2021-06-02 12:00 ` Thomas Hellström
@ 2021-06-08 9:57 ` Matthew Auld
2021-06-08 10:06 ` Thomas Hellström
0 siblings, 1 reply; 4+ messages in thread
From: Matthew Auld @ 2021-06-08 9:57 UTC (permalink / raw)
To: Thomas Hellström, intel-gfx; +Cc: Daniel Vetter, dri-devel
On 02/06/2021 13:00, Thomas Hellström wrote:
> Hi,
>
> On 6/2/21 11:36 AM, Matthew Auld wrote:
>> For dgfx where we now have lmem and ttm, we can only support single mmap
>> mode for the lifetime of the object, and for lmem objects this should be
>> WC, so reject all other mapping modes for mmap_offset, including if the
>> object can be placed in both smem and lmem.
>>
>> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
>> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>> ---
>> drivers/gpu/drm/i915/gem/i915_gem_mman.c | 4 ++++
>> drivers/gpu/drm/i915/gem/i915_gem_object.c | 22 ++++++++++++++++++++++
>> drivers/gpu/drm/i915/gem/i915_gem_object.h | 4 ++++
>> 3 files changed, 30 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
>> b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
>> index fd1c9714f8d8..32f88f236771 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
>> @@ -689,6 +689,10 @@ __assign_mmap_offset(struct drm_file *file,
>> goto out;
>> }
>> + if (mmap_type != I915_MMAP_TYPE_WC &&
>> + i915_gem_object_placements_contain_type(obj,
>> INTEL_MEMORY_LOCAL))
>> + return -ENODEV;
>> +
>
> I think we will also have the restriction that any other objects on DGFX
> can only be mapped cached? At least that's what the TTM code is
> implementing currently.
Yeah, with this patch the caching mode should now at least be consistent
for lmem objects, for smem we still need more patches.
>
>
>> mmo = mmap_offset_attach(obj, mmap_type, file);
>> if (IS_ERR(mmo)) {
>> err = PTR_ERR(mmo);
>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c
>> b/drivers/gpu/drm/i915/gem/i915_gem_object.c
>> index 2be6109d0093..d4b0da8ed969 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
>> @@ -403,6 +403,28 @@ int i915_gem_object_read_from_page(struct
>> drm_i915_gem_object *obj, u64 offset,
>> return 0;
>> }
>> +/**
>> + * i915_gem_object_placements_contain_type - Check whether the object
>> can be
>> + * placed at certain memory type
>> + * @obj: Pointer to the object
>> + * @type: The memory type to check
>> + *
>> + * Return: True if the object can be placed in @type. False otherwise.
>> + */
>> +bool i915_gem_object_placements_contain_type(struct
>> drm_i915_gem_object *obj,
>> + enum intel_memory_type type)
>> +{
>> + unsigned int i;
>> +
>> + /* TODO: consider maybe storing as a mask when doing
>> gem_create_ext */
>> + for (i = 0; i < obj->mm.n_placements; i++) {
>> + if (obj->mm.placements[i]->type == type)
>> + return true;
>> + }
>> +
>> + return false;
>> +}
>> +
>
> Do we need something for the in-kernel mappings as well? Or just return
> a mapping with the only allowed caching mode?
For lmem everything should already be WC for in-kernel mappings. For
everything else which uses pin_map() we will need to default to cached.
I guess just add a different helper for this? We should probably also
adjust the obj->cache_level for dg1.
>
> /Thomas
>
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] drm/i915/gem/mman: only allow WC for lmem
2021-06-08 9:57 ` Matthew Auld
@ 2021-06-08 10:06 ` Thomas Hellström
0 siblings, 0 replies; 4+ messages in thread
From: Thomas Hellström @ 2021-06-08 10:06 UTC (permalink / raw)
To: Matthew Auld, intel-gfx; +Cc: Daniel Vetter, dri-devel
On Tue, 2021-06-08 at 10:57 +0100, Matthew Auld wrote:
> On 02/06/2021 13:00, Thomas Hellström wrote:
> > Hi,
> >
> > On 6/2/21 11:36 AM, Matthew Auld wrote:
> > > For dgfx where we now have lmem and ttm, we can only support
> > > single mmap
> > > mode for the lifetime of the object, and for lmem objects this
> > > should be
> > > WC, so reject all other mapping modes for mmap_offset, including
> > > if the
> > > object can be placed in both smem and lmem.
> > >
> > > Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> > > Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > ---
> > > drivers/gpu/drm/i915/gem/i915_gem_mman.c | 4 ++++
> > > drivers/gpu/drm/i915/gem/i915_gem_object.c | 22
> > > ++++++++++++++++++++++
> > > drivers/gpu/drm/i915/gem/i915_gem_object.h | 4 ++++
> > > 3 files changed, 30 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> > > b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> > > index fd1c9714f8d8..32f88f236771 100644
> > > --- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> > > @@ -689,6 +689,10 @@ __assign_mmap_offset(struct drm_file *file,
> > > goto out;
> > > }
> > > + if (mmap_type != I915_MMAP_TYPE_WC &&
> > > + i915_gem_object_placements_contain_type(obj,
> > > INTEL_MEMORY_LOCAL))
> > > + return -ENODEV;
> > > +
> >
> > I think we will also have the restriction that any other objects on
> > DGFX
> > can only be mapped cached? At least that's what the TTM code is
> > implementing currently.
>
> Yeah, with this patch the caching mode should now at least be
> consistent
> for lmem objects, for smem we still need more patches.
>
> >
> >
> > > mmo = mmap_offset_attach(obj, mmap_type, file);
> > > if (IS_ERR(mmo)) {
> > > err = PTR_ERR(mmo);
> > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c
> > > b/drivers/gpu/drm/i915/gem/i915_gem_object.c
> > > index 2be6109d0093..d4b0da8ed969 100644
> > > --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
> > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
> > > @@ -403,6 +403,28 @@ int i915_gem_object_read_from_page(struct
> > > drm_i915_gem_object *obj, u64 offset,
> > > return 0;
> > > }
> > > +/**
> > > + * i915_gem_object_placements_contain_type - Check whether the
> > > object
> > > can be
> > > + * placed at certain memory type
> > > + * @obj: Pointer to the object
> > > + * @type: The memory type to check
> > > + *
> > > + * Return: True if the object can be placed in @type. False
> > > otherwise.
> > > + */
> > > +bool i915_gem_object_placements_contain_type(struct
> > > drm_i915_gem_object *obj,
> > > + enum intel_memory_type type)
> > > +{
> > > + unsigned int i;
> > > +
> > > + /* TODO: consider maybe storing as a mask when doing
> > > gem_create_ext */
> > > + for (i = 0; i < obj->mm.n_placements; i++) {
> > > + if (obj->mm.placements[i]->type == type)
> > > + return true;
> > > + }
> > > +
> > > + return false;
> > > +}
> > > +
> >
> > Do we need something for the in-kernel mappings as well? Or just
> > return
> > a mapping with the only allowed caching mode?
>
> For lmem everything should already be WC for in-kernel mappings. For
> everything else which uses pin_map() we will need to default to
> cached.
> I guess just add a different helper for this? We should probably also
> adjust the obj->cache_level for dg1.
Note that objects that have LMEM in the allowed placement list, but are
migrated to SMEM for some reason (we haven't really decided the policy
for this yet, but perhaps for dma-buf export reasons or just being
evicted with smem as an allowable placement) will still be WC-only,
which was Daniel's recommendation to begin with, (but we can flip
caching mode on eviction / migration if we'd want to).
Currently we don't flip GEM region when evicting even if SMEM is an
allowed placement, because the object may then end up stuck in SMEM.
Not sure if we want to expose a user-space madvise-type hint for this?
/Thomas
>
> >
> > /Thomas
> >
> >
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-06-08 10:06 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-02 9:36 [PATCH] drm/i915/gem/mman: only allow WC for lmem Matthew Auld
2021-06-02 12:00 ` Thomas Hellström
2021-06-08 9:57 ` Matthew Auld
2021-06-08 10:06 ` Thomas Hellström
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).