* [Intel-gfx] [RFC 0/1] DGFX pin_map with rpm @ 2022-09-15 10:33 Anshuman Gupta 2022-09-15 10:33 ` [Intel-gfx] [RFC 1/1] drm/i915/dgfx: Handling of pin_map against rpm Anshuman Gupta 2022-09-15 11:06 ` [Intel-gfx] ✗ Fi.CI.BUILD: failure for DGFX pin_map with rpm Patchwork 0 siblings, 2 replies; 10+ messages in thread From: Anshuman Gupta @ 2022-09-15 10:33 UTC (permalink / raw) To: intel-gfx; +Cc: matthew.auld, rodrigo.vivi As per PCIe Spec Section 5.3.1.4.1 When a PCIe function is in d3hot state, Configuration and Message requests are the only TLPs accepted by a Function in the D3hot state. All other received Requests must be handled as Unsupported Requests, and all received Completions may optionally be handled as Unexpected Completions. Therefore all PCIe iomem transaction requires to keep the PCIe endpoint function in D0 state. This RFC proposal is depends on the assumption with my understanding of code, we can't partially pin_map the lmem gem object, with considering that every caller of i915_gem_onject_pin_map() does not need to grab the wakeref. We can embedded the wakeref inside the gem object itself. Requesting community to provide the feedback on this proposal. Anshuman Gupta (1): drm/i915/dgfx: Handling of pin_map against rpm drivers/gpu/drm/i915/gem/i915_gem_object.c | 2 ++ drivers/gpu/drm/i915/gem/i915_gem_object.h | 5 +++++ drivers/gpu/drm/i915/gem/i915_gem_object_types.h | 14 ++++++++++++++ drivers/gpu/drm/i915/gem/i915_gem_pages.c | 8 ++++++++ 4 files changed, 29 insertions(+) -- 2.26.2 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [Intel-gfx] [RFC 1/1] drm/i915/dgfx: Handling of pin_map against rpm 2022-09-15 10:33 [Intel-gfx] [RFC 0/1] DGFX pin_map with rpm Anshuman Gupta @ 2022-09-15 10:33 ` Anshuman Gupta 2022-09-15 14:17 ` Tvrtko Ursulin ` (2 more replies) 2022-09-15 11:06 ` [Intel-gfx] ✗ Fi.CI.BUILD: failure for DGFX pin_map with rpm Patchwork 1 sibling, 3 replies; 10+ messages in thread From: Anshuman Gupta @ 2022-09-15 10:33 UTC (permalink / raw) To: intel-gfx; +Cc: matthew.auld, rodrigo.vivi If i915 gem obj lies in lmem, then i915_gem_object_pin_map need to grab a rpm wakeref to make sure gfx PCIe endpoint function stays in D0 state during any access to mapping returned by i915_gem_object_pin_map(). Subsequently i915_gem_object_upin_map will put the wakref as well. Cc: Matthew Auld <matthew.auld@intel.com> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> Cc: Andi Shyti <andi.shyti@linux.intel.com> Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com> --- drivers/gpu/drm/i915/gem/i915_gem_object.c | 2 ++ drivers/gpu/drm/i915/gem/i915_gem_object.h | 5 +++++ drivers/gpu/drm/i915/gem/i915_gem_object_types.h | 14 ++++++++++++++ drivers/gpu/drm/i915/gem/i915_gem_pages.c | 8 ++++++++ 4 files changed, 29 insertions(+) diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c b/drivers/gpu/drm/i915/gem/i915_gem_object.c index 85482a04d158..f291f990838d 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c @@ -95,6 +95,7 @@ void i915_gem_object_init(struct drm_i915_gem_object *obj, mutex_init(&obj->mm.get_page.lock); INIT_RADIX_TREE(&obj->mm.get_dma_page.radix, GFP_KERNEL | __GFP_NOWARN); mutex_init(&obj->mm.get_dma_page.lock); + mutex_init(&obj->wakeref_lock); } /** @@ -110,6 +111,7 @@ void __i915_gem_object_fini(struct drm_i915_gem_object *obj) { mutex_destroy(&obj->mm.get_page.lock); mutex_destroy(&obj->mm.get_dma_page.lock); + mutex_destroy(&obj->wakeref_lock); dma_resv_fini(&obj->base._resv); } diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h b/drivers/gpu/drm/i915/gem/i915_gem_object.h index 7317d4102955..b31ac6e4c272 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_object.h +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h @@ -501,6 +501,11 @@ static inline void i915_gem_object_flush_map(struct drm_i915_gem_object *obj) */ static inline void i915_gem_object_unpin_map(struct drm_i915_gem_object *obj) { + mutext_lock(obj->wakeref_lock); + if (!--obj->wakeref_count) + intel_runtime_pm_put(&to_i915(obj->base.dev)->runtime_pm, obj->wakeref); + mutext_unlock(obj->wakeref_lock); + i915_gem_object_unpin_pages(obj); } diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h index 9f6b14ec189a..34aff95a1984 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h +++ b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h @@ -657,6 +657,20 @@ struct drm_i915_gem_object { void *gvt_info; }; + + /** + * wakeref to protect the i915 lmem iomem mappings. + * We don't pin_map an object partially that makes easy + * to track the wakeref cookie, if wakeref is already held + * then we don't need to grab it again for other pin_map. + * first pin_map will grab the wakeref and last unpin_map + * will put the wakeref. + */ + intel_wakeref_t wakeref; + unsigned int wakeref_count; + + /** protects the wakeref_count wakeref cookie against multiple pin_map and unpin_map */ + struct mutex wakeref_lock; }; static inline struct drm_i915_gem_object * diff --git a/drivers/gpu/drm/i915/gem/i915_gem_pages.c b/drivers/gpu/drm/i915/gem/i915_gem_pages.c index 4df50b049cea..b638b5413280 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_pages.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_pages.c @@ -370,6 +370,14 @@ void *i915_gem_object_pin_map(struct drm_i915_gem_object *obj, assert_object_held(obj); + if (i915_gem_object_is_lmem(obj)) { + mutex_lock(&obj->wakeref_lock); + if (!obj->wakeref_count++) + obj->wakeref = + intel_runtime_pm_get(&to_i915(obj->base.dev)->runtime_pm); + mutex_unlock(&obj->wakeref_lock); + } + pinned = !(type & I915_MAP_OVERRIDE); type &= ~I915_MAP_OVERRIDE; -- 2.26.2 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [Intel-gfx] [RFC 1/1] drm/i915/dgfx: Handling of pin_map against rpm 2022-09-15 10:33 ` [Intel-gfx] [RFC 1/1] drm/i915/dgfx: Handling of pin_map against rpm Anshuman Gupta @ 2022-09-15 14:17 ` Tvrtko Ursulin 2022-09-15 16:49 ` Gupta, Anshuman 2022-09-15 14:32 ` Tvrtko Ursulin 2022-09-15 17:07 ` Tvrtko Ursulin 2 siblings, 1 reply; 10+ messages in thread From: Tvrtko Ursulin @ 2022-09-15 14:17 UTC (permalink / raw) To: Anshuman Gupta, intel-gfx; +Cc: matthew.auld, rodrigo.vivi On 15/09/2022 11:33, Anshuman Gupta wrote: > If i915 gem obj lies in lmem, then i915_gem_object_pin_map > need to grab a rpm wakeref to make sure gfx PCIe endpoint > function stays in D0 state during any access to mapping > returned by i915_gem_object_pin_map(). > Subsequently i915_gem_object_upin_map will put the wakref as well. > > Cc: Matthew Auld <matthew.auld@intel.com> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Cc: Andi Shyti <andi.shyti@linux.intel.com> > Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com> > --- > drivers/gpu/drm/i915/gem/i915_gem_object.c | 2 ++ > drivers/gpu/drm/i915/gem/i915_gem_object.h | 5 +++++ > drivers/gpu/drm/i915/gem/i915_gem_object_types.h | 14 ++++++++++++++ > drivers/gpu/drm/i915/gem/i915_gem_pages.c | 8 ++++++++ > 4 files changed, 29 insertions(+) > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c b/drivers/gpu/drm/i915/gem/i915_gem_object.c > index 85482a04d158..f291f990838d 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c > @@ -95,6 +95,7 @@ void i915_gem_object_init(struct drm_i915_gem_object *obj, > mutex_init(&obj->mm.get_page.lock); > INIT_RADIX_TREE(&obj->mm.get_dma_page.radix, GFP_KERNEL | __GFP_NOWARN); > mutex_init(&obj->mm.get_dma_page.lock); > + mutex_init(&obj->wakeref_lock); > } > > /** > @@ -110,6 +111,7 @@ void __i915_gem_object_fini(struct drm_i915_gem_object *obj) > { > mutex_destroy(&obj->mm.get_page.lock); > mutex_destroy(&obj->mm.get_dma_page.lock); > + mutex_destroy(&obj->wakeref_lock); > dma_resv_fini(&obj->base._resv); > } > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h b/drivers/gpu/drm/i915/gem/i915_gem_object.h > index 7317d4102955..b31ac6e4c272 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_object.h > +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h > @@ -501,6 +501,11 @@ static inline void i915_gem_object_flush_map(struct drm_i915_gem_object *obj) > */ > static inline void i915_gem_object_unpin_map(struct drm_i915_gem_object *obj) > { > + mutext_lock(obj->wakeref_lock); > + if (!--obj->wakeref_count) > + intel_runtime_pm_put(&to_i915(obj->base.dev)->runtime_pm, obj->wakeref); > + mutext_unlock(obj->wakeref_lock); > + > i915_gem_object_unpin_pages(obj); > } > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h > index 9f6b14ec189a..34aff95a1984 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h > +++ b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h > @@ -657,6 +657,20 @@ struct drm_i915_gem_object { > > void *gvt_info; > }; > + > + /** > + * wakeref to protect the i915 lmem iomem mappings. > + * We don't pin_map an object partially that makes easy > + * to track the wakeref cookie, if wakeref is already held > + * then we don't need to grab it again for other pin_map. > + * first pin_map will grab the wakeref and last unpin_map > + * will put the wakeref. > + */ > + intel_wakeref_t wakeref; > + unsigned int wakeref_count; > + > + /** protects the wakeref_count wakeref cookie against multiple pin_map and unpin_map */ > + struct mutex wakeref_lock; On one side it feels wasteful to have counters per object. But then I also notice pin_map is only allowed under the obj dma_resv locked - meaning that lock is already held. So you possibly don't need a new mutex, someone more up to date please confirm. Option B - trading space efficieny for one more atomic - would be to track it on the level of i915 and maybe use atomic_t? Would we have to worry about overflow more in this case? Hm some protection regardless of the option will be needed just in case. Regards, Tvrtko > }; > > static inline struct drm_i915_gem_object * > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_pages.c b/drivers/gpu/drm/i915/gem/i915_gem_pages.c > index 4df50b049cea..b638b5413280 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_pages.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_pages.c > @@ -370,6 +370,14 @@ void *i915_gem_object_pin_map(struct drm_i915_gem_object *obj, > > assert_object_held(obj); > > + if (i915_gem_object_is_lmem(obj)) { > + mutex_lock(&obj->wakeref_lock); > + if (!obj->wakeref_count++) > + obj->wakeref = > + intel_runtime_pm_get(&to_i915(obj->base.dev)->runtime_pm); > + mutex_unlock(&obj->wakeref_lock); > + } > + > pinned = !(type & I915_MAP_OVERRIDE); > type &= ~I915_MAP_OVERRIDE; > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Intel-gfx] [RFC 1/1] drm/i915/dgfx: Handling of pin_map against rpm 2022-09-15 14:17 ` Tvrtko Ursulin @ 2022-09-15 16:49 ` Gupta, Anshuman 0 siblings, 0 replies; 10+ messages in thread From: Gupta, Anshuman @ 2022-09-15 16:49 UTC (permalink / raw) To: Tvrtko Ursulin, intel-gfx@lists.freedesktop.org Cc: Auld, Matthew, Vivi, Rodrigo > -----Original Message----- > From: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> > Sent: Thursday, September 15, 2022 7:48 PM > To: Gupta, Anshuman <anshuman.gupta@intel.com>; intel- > gfx@lists.freedesktop.org > Cc: Auld, Matthew <matthew.auld@intel.com>; Vivi, Rodrigo > <rodrigo.vivi@intel.com> > Subject: Re: [Intel-gfx] [RFC 1/1] drm/i915/dgfx: Handling of pin_map against > rpm > > > On 15/09/2022 11:33, Anshuman Gupta wrote: > > If i915 gem obj lies in lmem, then i915_gem_object_pin_map need to > > grab a rpm wakeref to make sure gfx PCIe endpoint function stays in D0 > > state during any access to mapping returned by > > i915_gem_object_pin_map(). > > Subsequently i915_gem_object_upin_map will put the wakref as well. > > > > Cc: Matthew Auld <matthew.auld@intel.com> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > Cc: Andi Shyti <andi.shyti@linux.intel.com> > > Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com> > > --- > > drivers/gpu/drm/i915/gem/i915_gem_object.c | 2 ++ > > drivers/gpu/drm/i915/gem/i915_gem_object.h | 5 +++++ > > drivers/gpu/drm/i915/gem/i915_gem_object_types.h | 14 ++++++++++++++ > > drivers/gpu/drm/i915/gem/i915_gem_pages.c | 8 ++++++++ > > 4 files changed, 29 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c > > b/drivers/gpu/drm/i915/gem/i915_gem_object.c > > index 85482a04d158..f291f990838d 100644 > > --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c > > @@ -95,6 +95,7 @@ void i915_gem_object_init(struct drm_i915_gem_object > *obj, > > mutex_init(&obj->mm.get_page.lock); > > INIT_RADIX_TREE(&obj->mm.get_dma_page.radix, GFP_KERNEL | > __GFP_NOWARN); > > mutex_init(&obj->mm.get_dma_page.lock); > > + mutex_init(&obj->wakeref_lock); > > } > > > > /** > > @@ -110,6 +111,7 @@ void __i915_gem_object_fini(struct > drm_i915_gem_object *obj) > > { > > mutex_destroy(&obj->mm.get_page.lock); > > mutex_destroy(&obj->mm.get_dma_page.lock); > > + mutex_destroy(&obj->wakeref_lock); > > dma_resv_fini(&obj->base._resv); > > } > > > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h > > b/drivers/gpu/drm/i915/gem/i915_gem_object.h > > index 7317d4102955..b31ac6e4c272 100644 > > --- a/drivers/gpu/drm/i915/gem/i915_gem_object.h > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h > > @@ -501,6 +501,11 @@ static inline void i915_gem_object_flush_map(struct > drm_i915_gem_object *obj) > > */ > > static inline void i915_gem_object_unpin_map(struct drm_i915_gem_object > *obj) > > { > > + mutext_lock(obj->wakeref_lock); > > + if (!--obj->wakeref_count) > > + intel_runtime_pm_put(&to_i915(obj->base.dev)->runtime_pm, > obj->wakeref); > > + mutext_unlock(obj->wakeref_lock); > > + > > i915_gem_object_unpin_pages(obj); > > } > > > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h > > b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h > > index 9f6b14ec189a..34aff95a1984 100644 > > --- a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h > > @@ -657,6 +657,20 @@ struct drm_i915_gem_object { > > > > void *gvt_info; > > }; > > + > > + /** > > + * wakeref to protect the i915 lmem iomem mappings. > > + * We don't pin_map an object partially that makes easy > > + * to track the wakeref cookie, if wakeref is already held > > + * then we don't need to grab it again for other pin_map. > > + * first pin_map will grab the wakeref and last unpin_map > > + * will put the wakeref. > > + */ > > + intel_wakeref_t wakeref; > > + unsigned int wakeref_count; > > + > > + /** protects the wakeref_count wakeref cookie against multiple > pin_map and unpin_map */ > > + struct mutex wakeref_lock; > > On one side it feels wasteful to have counters per object. But then I also notice > pin_map is only allowed under the obj dma_resv locked - meaning that lock is > already held. So you possibly don't need a new mutex, someone more up to date > please confirm. True , but like i915_gem_object_pin_map_unlocked() will release the gem_object_lock after pin_map, so we need some protection in unpin_map(). > > Option B - trading space efficieny for one more atomic - would be to track it on > the level of i915 and maybe use atomic_t? Would we have to worry about > overflow more in this case? Hm some protection regardless of the option will be > needed just in case. I did not understand you comment , is it about using the atomic_t variable for wakeref_count ? If that is the feedback, i can do that if community is agree on this RFC proposal. Thanks, Anshuman Gupta. > > Regards, > > Tvrtko > > > }; > > > > static inline struct drm_i915_gem_object * diff --git > > a/drivers/gpu/drm/i915/gem/i915_gem_pages.c > > b/drivers/gpu/drm/i915/gem/i915_gem_pages.c > > index 4df50b049cea..b638b5413280 100644 > > --- a/drivers/gpu/drm/i915/gem/i915_gem_pages.c > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_pages.c > > @@ -370,6 +370,14 @@ void *i915_gem_object_pin_map(struct > > drm_i915_gem_object *obj, > > > > assert_object_held(obj); > > > > + if (i915_gem_object_is_lmem(obj)) { > > + mutex_lock(&obj->wakeref_lock); > > + if (!obj->wakeref_count++) > > + obj->wakeref = > > + intel_runtime_pm_get(&to_i915(obj- > >base.dev)->runtime_pm); > > + mutex_unlock(&obj->wakeref_lock); > > + } > > + > > pinned = !(type & I915_MAP_OVERRIDE); > > type &= ~I915_MAP_OVERRIDE; > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Intel-gfx] [RFC 1/1] drm/i915/dgfx: Handling of pin_map against rpm 2022-09-15 10:33 ` [Intel-gfx] [RFC 1/1] drm/i915/dgfx: Handling of pin_map against rpm Anshuman Gupta 2022-09-15 14:17 ` Tvrtko Ursulin @ 2022-09-15 14:32 ` Tvrtko Ursulin 2022-09-15 16:41 ` Gupta, Anshuman 2022-09-15 17:07 ` Tvrtko Ursulin 2 siblings, 1 reply; 10+ messages in thread From: Tvrtko Ursulin @ 2022-09-15 14:32 UTC (permalink / raw) To: Anshuman Gupta, intel-gfx; +Cc: matthew.auld, rodrigo.vivi On 15/09/2022 11:33, Anshuman Gupta wrote: > If i915 gem obj lies in lmem, then i915_gem_object_pin_map > need to grab a rpm wakeref to make sure gfx PCIe endpoint > function stays in D0 state during any access to mapping > returned by i915_gem_object_pin_map(). > Subsequently i915_gem_object_upin_map will put the wakref as well. > > Cc: Matthew Auld <matthew.auld@intel.com> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Cc: Andi Shyti <andi.shyti@linux.intel.com> > Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com> > --- > drivers/gpu/drm/i915/gem/i915_gem_object.c | 2 ++ > drivers/gpu/drm/i915/gem/i915_gem_object.h | 5 +++++ > drivers/gpu/drm/i915/gem/i915_gem_object_types.h | 14 ++++++++++++++ > drivers/gpu/drm/i915/gem/i915_gem_pages.c | 8 ++++++++ > 4 files changed, 29 insertions(+) > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c b/drivers/gpu/drm/i915/gem/i915_gem_object.c > index 85482a04d158..f291f990838d 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c > @@ -95,6 +95,7 @@ void i915_gem_object_init(struct drm_i915_gem_object *obj, > mutex_init(&obj->mm.get_page.lock); > INIT_RADIX_TREE(&obj->mm.get_dma_page.radix, GFP_KERNEL | __GFP_NOWARN); > mutex_init(&obj->mm.get_dma_page.lock); > + mutex_init(&obj->wakeref_lock); > } > > /** > @@ -110,6 +111,7 @@ void __i915_gem_object_fini(struct drm_i915_gem_object *obj) > { > mutex_destroy(&obj->mm.get_page.lock); > mutex_destroy(&obj->mm.get_dma_page.lock); > + mutex_destroy(&obj->wakeref_lock); > dma_resv_fini(&obj->base._resv); > } > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h b/drivers/gpu/drm/i915/gem/i915_gem_object.h > index 7317d4102955..b31ac6e4c272 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_object.h > +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h > @@ -501,6 +501,11 @@ static inline void i915_gem_object_flush_map(struct drm_i915_gem_object *obj) > */ > static inline void i915_gem_object_unpin_map(struct drm_i915_gem_object *obj) > { > + mutext_lock(obj->wakeref_lock); > + if (!--obj->wakeref_count) Bug here - incremented only for i915_gem_object_is_lmem, decremented for all. a) Use i915_gem_object_is_lmem here? Is the answer it gives stable across object lifetime considering placements and migration? b) Store the wakeref in object and release if present? Regards, Tvrtko > + intel_runtime_pm_put(&to_i915(obj->base.dev)->runtime_pm, obj->wakeref); > + mutext_unlock(obj->wakeref_lock); > + > i915_gem_object_unpin_pages(obj); > } > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h > index 9f6b14ec189a..34aff95a1984 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h > +++ b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h > @@ -657,6 +657,20 @@ struct drm_i915_gem_object { > > void *gvt_info; > }; > + > + /** > + * wakeref to protect the i915 lmem iomem mappings. > + * We don't pin_map an object partially that makes easy > + * to track the wakeref cookie, if wakeref is already held > + * then we don't need to grab it again for other pin_map. > + * first pin_map will grab the wakeref and last unpin_map > + * will put the wakeref. > + */ > + intel_wakeref_t wakeref; > + unsigned int wakeref_count; > + > + /** protects the wakeref_count wakeref cookie against multiple pin_map and unpin_map */ > + struct mutex wakeref_lock; > }; > > static inline struct drm_i915_gem_object * > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_pages.c b/drivers/gpu/drm/i915/gem/i915_gem_pages.c > index 4df50b049cea..b638b5413280 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_pages.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_pages.c > @@ -370,6 +370,14 @@ void *i915_gem_object_pin_map(struct drm_i915_gem_object *obj, > > assert_object_held(obj); > > + if (i915_gem_object_is_lmem(obj)) { > + mutex_lock(&obj->wakeref_lock); > + if (!obj->wakeref_count++) > + obj->wakeref = > + intel_runtime_pm_get(&to_i915(obj->base.dev)->runtime_pm); > + mutex_unlock(&obj->wakeref_lock); > + } > + > pinned = !(type & I915_MAP_OVERRIDE); > type &= ~I915_MAP_OVERRIDE; > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Intel-gfx] [RFC 1/1] drm/i915/dgfx: Handling of pin_map against rpm 2022-09-15 14:32 ` Tvrtko Ursulin @ 2022-09-15 16:41 ` Gupta, Anshuman 0 siblings, 0 replies; 10+ messages in thread From: Gupta, Anshuman @ 2022-09-15 16:41 UTC (permalink / raw) To: Tvrtko Ursulin, intel-gfx@lists.freedesktop.org Cc: Auld, Matthew, Vivi, Rodrigo > -----Original Message----- > From: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> > Sent: Thursday, September 15, 2022 8:03 PM > To: Gupta, Anshuman <anshuman.gupta@intel.com>; intel- > gfx@lists.freedesktop.org > Cc: Auld, Matthew <matthew.auld@intel.com>; Vivi, Rodrigo > <rodrigo.vivi@intel.com> > Subject: Re: [Intel-gfx] [RFC 1/1] drm/i915/dgfx: Handling of pin_map against > rpm > > > On 15/09/2022 11:33, Anshuman Gupta wrote: > > If i915 gem obj lies in lmem, then i915_gem_object_pin_map need to > > grab a rpm wakeref to make sure gfx PCIe endpoint function stays in D0 > > state during any access to mapping returned by > > i915_gem_object_pin_map(). > > Subsequently i915_gem_object_upin_map will put the wakref as well. > > > > Cc: Matthew Auld <matthew.auld@intel.com> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > Cc: Andi Shyti <andi.shyti@linux.intel.com> > > Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com> > > --- > > drivers/gpu/drm/i915/gem/i915_gem_object.c | 2 ++ > > drivers/gpu/drm/i915/gem/i915_gem_object.h | 5 +++++ > > drivers/gpu/drm/i915/gem/i915_gem_object_types.h | 14 ++++++++++++++ > > drivers/gpu/drm/i915/gem/i915_gem_pages.c | 8 ++++++++ > > 4 files changed, 29 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c > > b/drivers/gpu/drm/i915/gem/i915_gem_object.c > > index 85482a04d158..f291f990838d 100644 > > --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c > > @@ -95,6 +95,7 @@ void i915_gem_object_init(struct drm_i915_gem_object > *obj, > > mutex_init(&obj->mm.get_page.lock); > > INIT_RADIX_TREE(&obj->mm.get_dma_page.radix, GFP_KERNEL | > __GFP_NOWARN); > > mutex_init(&obj->mm.get_dma_page.lock); > > + mutex_init(&obj->wakeref_lock); > > } > > > > /** > > @@ -110,6 +111,7 @@ void __i915_gem_object_fini(struct > drm_i915_gem_object *obj) > > { > > mutex_destroy(&obj->mm.get_page.lock); > > mutex_destroy(&obj->mm.get_dma_page.lock); > > + mutex_destroy(&obj->wakeref_lock); > > dma_resv_fini(&obj->base._resv); > > } > > > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h > > b/drivers/gpu/drm/i915/gem/i915_gem_object.h > > index 7317d4102955..b31ac6e4c272 100644 > > --- a/drivers/gpu/drm/i915/gem/i915_gem_object.h > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h > > @@ -501,6 +501,11 @@ static inline void i915_gem_object_flush_map(struct > drm_i915_gem_object *obj) > > */ > > static inline void i915_gem_object_unpin_map(struct drm_i915_gem_object > *obj) > > { > > + mutext_lock(obj->wakeref_lock); > > + if (!--obj->wakeref_count) > > Bug here - incremented only for i915_gem_object_is_lmem, decremented for > all. Thanks for pointing it out, how about like below cond? if (obj->wakeref_count) { /* wakere_count will non zero only in case of object was in lmem */ obj->wakeref_count-- if (!obj->wakeref_count) intel_runtime_pm_put(&to_i915(obj->base.dev)->runtime_pm, obj->wakeref); } > > a) > Use i915_gem_object_is_lmem here? Is the answer it gives stable across object > lifetime considering placements and migration? AFAIU After pin_map() the caller may drop the object lock, so eviction can take place. But as we had already taken the wakeref for object, we need to release it. > > b) > Store the wakeref in object and release if present? If there are two pin_map for the the same object, then unpin_map() will release the wakeref and the other pin_map() is not protected. It is my understanding, can it happen ? Thanks, Anshuman Gupta. > > Regards, > > Tvrtko > > > + intel_runtime_pm_put(&to_i915(obj->base.dev)->runtime_pm, > obj->wakeref); > > + mutext_unlock(obj->wakeref_lock); > > + > > i915_gem_object_unpin_pages(obj); > > } > > > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h > > b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h > > index 9f6b14ec189a..34aff95a1984 100644 > > --- a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h > > @@ -657,6 +657,20 @@ struct drm_i915_gem_object { > > > > void *gvt_info; > > }; > > + > > + /** > > + * wakeref to protect the i915 lmem iomem mappings. > > + * We don't pin_map an object partially that makes easy > > + * to track the wakeref cookie, if wakeref is already held > > + * then we don't need to grab it again for other pin_map. > > + * first pin_map will grab the wakeref and last unpin_map > > + * will put the wakeref. > > + */ > > + intel_wakeref_t wakeref; > > + unsigned int wakeref_count; > > + > > + /** protects the wakeref_count wakeref cookie against multiple > pin_map and unpin_map */ > > + struct mutex wakeref_lock; > > }; > > > > static inline struct drm_i915_gem_object * diff --git > > a/drivers/gpu/drm/i915/gem/i915_gem_pages.c > > b/drivers/gpu/drm/i915/gem/i915_gem_pages.c > > index 4df50b049cea..b638b5413280 100644 > > --- a/drivers/gpu/drm/i915/gem/i915_gem_pages.c > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_pages.c > > @@ -370,6 +370,14 @@ void *i915_gem_object_pin_map(struct > > drm_i915_gem_object *obj, > > > > assert_object_held(obj); > > > > + if (i915_gem_object_is_lmem(obj)) { > > + mutex_lock(&obj->wakeref_lock); > > + if (!obj->wakeref_count++) > > + obj->wakeref = > > + intel_runtime_pm_get(&to_i915(obj- > >base.dev)->runtime_pm); > > + mutex_unlock(&obj->wakeref_lock); > > + } > > + > > pinned = !(type & I915_MAP_OVERRIDE); > > type &= ~I915_MAP_OVERRIDE; > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Intel-gfx] [RFC 1/1] drm/i915/dgfx: Handling of pin_map against rpm 2022-09-15 10:33 ` [Intel-gfx] [RFC 1/1] drm/i915/dgfx: Handling of pin_map against rpm Anshuman Gupta 2022-09-15 14:17 ` Tvrtko Ursulin 2022-09-15 14:32 ` Tvrtko Ursulin @ 2022-09-15 17:07 ` Tvrtko Ursulin 2022-09-16 10:30 ` Gupta, Anshuman 2 siblings, 1 reply; 10+ messages in thread From: Tvrtko Ursulin @ 2022-09-15 17:07 UTC (permalink / raw) To: Anshuman Gupta, intel-gfx; +Cc: matthew.auld, rodrigo.vivi On 15/09/2022 11:33, Anshuman Gupta wrote: > If i915 gem obj lies in lmem, then i915_gem_object_pin_map > need to grab a rpm wakeref to make sure gfx PCIe endpoint > function stays in D0 state during any access to mapping > returned by i915_gem_object_pin_map(). > Subsequently i915_gem_object_upin_map will put the wakref as well. Another thing to check are perma pinned contexts. Follow the flow from intel_engine_create_pinned_context to intel_engine_destroy_pinned_context. If you find out that kernel (&co) contexts are pinned for the duration of i915 load/bind and that they use lmem objects, that would mean wakeref is held for the duration of i915 loaded state. Defeating the point and making the solution effectively equal to just disabling RPM. Regards, Tvrtko > Cc: Matthew Auld <matthew.auld@intel.com> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Cc: Andi Shyti <andi.shyti@linux.intel.com> > Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com> > --- > drivers/gpu/drm/i915/gem/i915_gem_object.c | 2 ++ > drivers/gpu/drm/i915/gem/i915_gem_object.h | 5 +++++ > drivers/gpu/drm/i915/gem/i915_gem_object_types.h | 14 ++++++++++++++ > drivers/gpu/drm/i915/gem/i915_gem_pages.c | 8 ++++++++ > 4 files changed, 29 insertions(+) > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c b/drivers/gpu/drm/i915/gem/i915_gem_object.c > index 85482a04d158..f291f990838d 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c > @@ -95,6 +95,7 @@ void i915_gem_object_init(struct drm_i915_gem_object *obj, > mutex_init(&obj->mm.get_page.lock); > INIT_RADIX_TREE(&obj->mm.get_dma_page.radix, GFP_KERNEL | __GFP_NOWARN); > mutex_init(&obj->mm.get_dma_page.lock); > + mutex_init(&obj->wakeref_lock); > } > > /** > @@ -110,6 +111,7 @@ void __i915_gem_object_fini(struct drm_i915_gem_object *obj) > { > mutex_destroy(&obj->mm.get_page.lock); > mutex_destroy(&obj->mm.get_dma_page.lock); > + mutex_destroy(&obj->wakeref_lock); > dma_resv_fini(&obj->base._resv); > } > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h b/drivers/gpu/drm/i915/gem/i915_gem_object.h > index 7317d4102955..b31ac6e4c272 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_object.h > +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h > @@ -501,6 +501,11 @@ static inline void i915_gem_object_flush_map(struct drm_i915_gem_object *obj) > */ > static inline void i915_gem_object_unpin_map(struct drm_i915_gem_object *obj) > { > + mutext_lock(obj->wakeref_lock); > + if (!--obj->wakeref_count) > + intel_runtime_pm_put(&to_i915(obj->base.dev)->runtime_pm, obj->wakeref); > + mutext_unlock(obj->wakeref_lock); > + > i915_gem_object_unpin_pages(obj); > } > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h > index 9f6b14ec189a..34aff95a1984 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h > +++ b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h > @@ -657,6 +657,20 @@ struct drm_i915_gem_object { > > void *gvt_info; > }; > + > + /** > + * wakeref to protect the i915 lmem iomem mappings. > + * We don't pin_map an object partially that makes easy > + * to track the wakeref cookie, if wakeref is already held > + * then we don't need to grab it again for other pin_map. > + * first pin_map will grab the wakeref and last unpin_map > + * will put the wakeref. > + */ > + intel_wakeref_t wakeref; > + unsigned int wakeref_count; > + > + /** protects the wakeref_count wakeref cookie against multiple pin_map and unpin_map */ > + struct mutex wakeref_lock; > }; > > static inline struct drm_i915_gem_object * > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_pages.c b/drivers/gpu/drm/i915/gem/i915_gem_pages.c > index 4df50b049cea..b638b5413280 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_pages.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_pages.c > @@ -370,6 +370,14 @@ void *i915_gem_object_pin_map(struct drm_i915_gem_object *obj, > > assert_object_held(obj); > > + if (i915_gem_object_is_lmem(obj)) { > + mutex_lock(&obj->wakeref_lock); > + if (!obj->wakeref_count++) > + obj->wakeref = > + intel_runtime_pm_get(&to_i915(obj->base.dev)->runtime_pm); > + mutex_unlock(&obj->wakeref_lock); > + } > + > pinned = !(type & I915_MAP_OVERRIDE); > type &= ~I915_MAP_OVERRIDE; > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Intel-gfx] [RFC 1/1] drm/i915/dgfx: Handling of pin_map against rpm 2022-09-15 17:07 ` Tvrtko Ursulin @ 2022-09-16 10:30 ` Gupta, Anshuman 2022-09-16 10:48 ` Tvrtko Ursulin 0 siblings, 1 reply; 10+ messages in thread From: Gupta, Anshuman @ 2022-09-16 10:30 UTC (permalink / raw) To: Tvrtko Ursulin, intel-gfx@lists.freedesktop.org Cc: Auld, Matthew, Vivi, Rodrigo > -----Original Message----- > From: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> > Sent: Thursday, September 15, 2022 10:37 PM > To: Gupta, Anshuman <anshuman.gupta@intel.com>; intel- > gfx@lists.freedesktop.org > Cc: Auld, Matthew <matthew.auld@intel.com>; Vivi, Rodrigo > <rodrigo.vivi@intel.com> > Subject: Re: [Intel-gfx] [RFC 1/1] drm/i915/dgfx: Handling of pin_map against > rpm > > > On 15/09/2022 11:33, Anshuman Gupta wrote: > > If i915 gem obj lies in lmem, then i915_gem_object_pin_map need to > > grab a rpm wakeref to make sure gfx PCIe endpoint function stays in D0 > > state during any access to mapping returned by > > i915_gem_object_pin_map(). > > Subsequently i915_gem_object_upin_map will put the wakref as well. > > Another thing to check are perma pinned contexts. Follow the flow from > intel_engine_create_pinned_context to intel_engine_destroy_pinned_context. > If you find out that kernel (&co) contexts are pinned for the duration of i915 > load/bind and that they use lmem objects, that would mean wakeref is held for > the duration of i915 loaded state. Defeating the point and making the solution > effectively equal to just disabling RPM. That’s correct intel_ring_pin can pin_map the lmem object. if (i915_vma_is_map_and_fenceable(vma)) { addr = (void __force *)i915_vma_pin_iomap(vma); } else { int type = i915_coherent_map_type(vma->vm->i915, vma->obj, false); addr = i915_gem_object_pin_map(vma->obj, type); } If that is the case this RFC proposal will not work and in that case every caller of i915_gem_object_pin_map need to grab the wakreref before accessing the retuned address by pin_map. Any inputs from you side for any other approach. Thanks, Anshuman Gupta. > > Regards, > > Tvrtko > > > Cc: Matthew Auld <matthew.auld@intel.com> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > Cc: Andi Shyti <andi.shyti@linux.intel.com> > > Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com> > > --- > > drivers/gpu/drm/i915/gem/i915_gem_object.c | 2 ++ > > drivers/gpu/drm/i915/gem/i915_gem_object.h | 5 +++++ > > drivers/gpu/drm/i915/gem/i915_gem_object_types.h | 14 ++++++++++++++ > > drivers/gpu/drm/i915/gem/i915_gem_pages.c | 8 ++++++++ > > 4 files changed, 29 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c > > b/drivers/gpu/drm/i915/gem/i915_gem_object.c > > index 85482a04d158..f291f990838d 100644 > > --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c > > @@ -95,6 +95,7 @@ void i915_gem_object_init(struct drm_i915_gem_object > *obj, > > mutex_init(&obj->mm.get_page.lock); > > INIT_RADIX_TREE(&obj->mm.get_dma_page.radix, GFP_KERNEL | > __GFP_NOWARN); > > mutex_init(&obj->mm.get_dma_page.lock); > > + mutex_init(&obj->wakeref_lock); > > } > > > > /** > > @@ -110,6 +111,7 @@ void __i915_gem_object_fini(struct > drm_i915_gem_object *obj) > > { > > mutex_destroy(&obj->mm.get_page.lock); > > mutex_destroy(&obj->mm.get_dma_page.lock); > > + mutex_destroy(&obj->wakeref_lock); > > dma_resv_fini(&obj->base._resv); > > } > > > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h > > b/drivers/gpu/drm/i915/gem/i915_gem_object.h > > index 7317d4102955..b31ac6e4c272 100644 > > --- a/drivers/gpu/drm/i915/gem/i915_gem_object.h > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h > > @@ -501,6 +501,11 @@ static inline void i915_gem_object_flush_map(struct > drm_i915_gem_object *obj) > > */ > > static inline void i915_gem_object_unpin_map(struct drm_i915_gem_object > *obj) > > { > > + mutext_lock(obj->wakeref_lock); > > + if (!--obj->wakeref_count) > > + intel_runtime_pm_put(&to_i915(obj->base.dev)->runtime_pm, > obj->wakeref); > > + mutext_unlock(obj->wakeref_lock); > > + > > i915_gem_object_unpin_pages(obj); > > } > > > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h > > b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h > > index 9f6b14ec189a..34aff95a1984 100644 > > --- a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h > > @@ -657,6 +657,20 @@ struct drm_i915_gem_object { > > > > void *gvt_info; > > }; > > + > > + /** > > + * wakeref to protect the i915 lmem iomem mappings. > > + * We don't pin_map an object partially that makes easy > > + * to track the wakeref cookie, if wakeref is already held > > + * then we don't need to grab it again for other pin_map. > > + * first pin_map will grab the wakeref and last unpin_map > > + * will put the wakeref. > > + */ > > + intel_wakeref_t wakeref; > > + unsigned int wakeref_count; > > + > > + /** protects the wakeref_count wakeref cookie against multiple > pin_map and unpin_map */ > > + struct mutex wakeref_lock; > > }; > > > > static inline struct drm_i915_gem_object * diff --git > > a/drivers/gpu/drm/i915/gem/i915_gem_pages.c > > b/drivers/gpu/drm/i915/gem/i915_gem_pages.c > > index 4df50b049cea..b638b5413280 100644 > > --- a/drivers/gpu/drm/i915/gem/i915_gem_pages.c > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_pages.c > > @@ -370,6 +370,14 @@ void *i915_gem_object_pin_map(struct > > drm_i915_gem_object *obj, > > > > assert_object_held(obj); > > > > + if (i915_gem_object_is_lmem(obj)) { > > + mutex_lock(&obj->wakeref_lock); > > + if (!obj->wakeref_count++) > > + obj->wakeref = > > + intel_runtime_pm_get(&to_i915(obj- > >base.dev)->runtime_pm); > > + mutex_unlock(&obj->wakeref_lock); > > + } > > + > > pinned = !(type & I915_MAP_OVERRIDE); > > type &= ~I915_MAP_OVERRIDE; > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Intel-gfx] [RFC 1/1] drm/i915/dgfx: Handling of pin_map against rpm 2022-09-16 10:30 ` Gupta, Anshuman @ 2022-09-16 10:48 ` Tvrtko Ursulin 0 siblings, 0 replies; 10+ messages in thread From: Tvrtko Ursulin @ 2022-09-16 10:48 UTC (permalink / raw) To: Gupta, Anshuman, intel-gfx@lists.freedesktop.org Cc: Auld, Matthew, Vivi, Rodrigo On 16/09/2022 11:30, Gupta, Anshuman wrote: >> -----Original Message----- >> From: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> >> Sent: Thursday, September 15, 2022 10:37 PM >> To: Gupta, Anshuman <anshuman.gupta@intel.com>; intel- >> gfx@lists.freedesktop.org >> Cc: Auld, Matthew <matthew.auld@intel.com>; Vivi, Rodrigo >> <rodrigo.vivi@intel.com> >> Subject: Re: [Intel-gfx] [RFC 1/1] drm/i915/dgfx: Handling of pin_map against >> rpm >> >> >> On 15/09/2022 11:33, Anshuman Gupta wrote: >>> If i915 gem obj lies in lmem, then i915_gem_object_pin_map need to >>> grab a rpm wakeref to make sure gfx PCIe endpoint function stays in D0 >>> state during any access to mapping returned by >>> i915_gem_object_pin_map(). >>> Subsequently i915_gem_object_upin_map will put the wakref as well. >> >> Another thing to check are perma pinned contexts. Follow the flow from >> intel_engine_create_pinned_context to intel_engine_destroy_pinned_context. >> If you find out that kernel (&co) contexts are pinned for the duration of i915 >> load/bind and that they use lmem objects, that would mean wakeref is held for >> the duration of i915 loaded state. Defeating the point and making the solution >> effectively equal to just disabling RPM. > That’s correct intel_ring_pin can pin_map the lmem object. > if (i915_vma_is_map_and_fenceable(vma)) { > addr = (void __force *)i915_vma_pin_iomap(vma); > } else { > int type = i915_coherent_map_type(vma->vm->i915, vma->obj, false); > > addr = i915_gem_object_pin_map(vma->obj, type); > } > > If that is the case this RFC proposal will not work and in that case Right, or LRC state for perma pinned contexts is probably even more clear cut. every caller of i915_gem_object_pin_map need to grab the wakreref before > accessing the retuned address by pin_map. Any inputs from you side for any other approach. I didn't quite get what you meant here. My point was that if my thinking that perma pinned contexts would hold the wakeref permanently is correct, that would prevent the approach from this patch to have a different effect from just disabling RPM. Would unpinning the perma pinned contexts on GT park be feasible? It's not a 5 minute question and unfortunately I don't have enough time to go deep into this problem space. Like Hopefully someone else can jump in. Regards, Tvrtko ^ permalink raw reply [flat|nested] 10+ messages in thread
* [Intel-gfx] ✗ Fi.CI.BUILD: failure for DGFX pin_map with rpm 2022-09-15 10:33 [Intel-gfx] [RFC 0/1] DGFX pin_map with rpm Anshuman Gupta 2022-09-15 10:33 ` [Intel-gfx] [RFC 1/1] drm/i915/dgfx: Handling of pin_map against rpm Anshuman Gupta @ 2022-09-15 11:06 ` Patchwork 1 sibling, 0 replies; 10+ messages in thread From: Patchwork @ 2022-09-15 11:06 UTC (permalink / raw) To: Anshuman Gupta; +Cc: intel-gfx == Series Details == Series: DGFX pin_map with rpm URL : https://patchwork.freedesktop.org/series/108596/ State : failure == Summary == Error: make failed CALL scripts/checksyscalls.sh CALL scripts/atomic/check-atomics.sh DESCEND objtool CHK include/generated/compile.h CC [M] drivers/gpu/drm/i915/i915_driver.o In file included from ./drivers/gpu/drm/i915/i915_vma.h:34, from drivers/gpu/drm/i915/display/intel_display_types.h:49, from drivers/gpu/drm/i915/i915_driver.c:52: ./drivers/gpu/drm/i915/gem/i915_gem_object.h: In function ‘i915_gem_object_unpin_map’: ./drivers/gpu/drm/i915/gem/i915_gem_object.h:504:2: error: implicit declaration of function ‘mutext_lock’; did you mean ‘mutex_lock’? [-Werror=implicit-function-declaration] mutext_lock(obj->wakeref_lock); ^~~~~~~~~~~ mutex_lock ./drivers/gpu/drm/i915/gem/i915_gem_object.h:506:25: error: implicit declaration of function ‘to_i915’ [-Werror=implicit-function-declaration] intel_runtime_pm_put(&to_i915(obj->base.dev)->runtime_pm, obj->wakeref); ^~~~~~~ ./drivers/gpu/drm/i915/gem/i915_gem_object.h:506:47: error: invalid type argument of ‘->’ (have ‘int’) intel_runtime_pm_put(&to_i915(obj->base.dev)->runtime_pm, obj->wakeref); ^~ ./drivers/gpu/drm/i915/gem/i915_gem_object.h:507:2: error: implicit declaration of function ‘mutext_unlock’; did you mean ‘mutex_unlock’? [-Werror=implicit-function-declaration] mutext_unlock(obj->wakeref_lock); ^~~~~~~~~~~~~ mutex_unlock In file included from ./drivers/gpu/drm/i915/gt/intel_context.h:14, from drivers/gpu/drm/i915/gem/i915_gem_context.h:12, from drivers/gpu/drm/i915/i915_driver.c:66: ./drivers/gpu/drm/i915/i915_drv.h: At top level: ./drivers/gpu/drm/i915/i915_drv.h:417:40: error: conflicting types for ‘to_i915’ static inline struct drm_i915_private *to_i915(const struct drm_device *dev) ^~~~~~~ In file included from ./drivers/gpu/drm/i915/i915_vma.h:34, from drivers/gpu/drm/i915/display/intel_display_types.h:49, from drivers/gpu/drm/i915/i915_driver.c:52: ./drivers/gpu/drm/i915/gem/i915_gem_object.h:506:25: note: previous implicit declaration of ‘to_i915’ was here intel_runtime_pm_put(&to_i915(obj->base.dev)->runtime_pm, obj->wakeref); ^~~~~~~ cc1: all warnings being treated as errors scripts/Makefile.build:249: recipe for target 'drivers/gpu/drm/i915/i915_driver.o' failed make[4]: *** [drivers/gpu/drm/i915/i915_driver.o] Error 1 scripts/Makefile.build:465: recipe for target 'drivers/gpu/drm/i915' failed make[3]: *** [drivers/gpu/drm/i915] Error 2 scripts/Makefile.build:465: recipe for target 'drivers/gpu/drm' failed make[2]: *** [drivers/gpu/drm] Error 2 scripts/Makefile.build:465: recipe for target 'drivers/gpu' failed make[1]: *** [drivers/gpu] Error 2 Makefile:1852: recipe for target 'drivers' failed make: *** [drivers] Error 2 ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2022-09-16 10:49 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-09-15 10:33 [Intel-gfx] [RFC 0/1] DGFX pin_map with rpm Anshuman Gupta 2022-09-15 10:33 ` [Intel-gfx] [RFC 1/1] drm/i915/dgfx: Handling of pin_map against rpm Anshuman Gupta 2022-09-15 14:17 ` Tvrtko Ursulin 2022-09-15 16:49 ` Gupta, Anshuman 2022-09-15 14:32 ` Tvrtko Ursulin 2022-09-15 16:41 ` Gupta, Anshuman 2022-09-15 17:07 ` Tvrtko Ursulin 2022-09-16 10:30 ` Gupta, Anshuman 2022-09-16 10:48 ` Tvrtko Ursulin 2022-09-15 11:06 ` [Intel-gfx] ✗ Fi.CI.BUILD: failure for DGFX pin_map with rpm Patchwork
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox