[Intel-gfx] [RFC 0/1] DGFX pin_map with rpm

[Intel-gfx] [RFC 0/1] DGFX pin_map with rpm

[Anshuman Gupta]

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

[Intel-gfx] [RFC 1/1] drm/i915/dgfx: Handling of pin_map against rpm

[Anshuman Gupta]

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

[Intel-gfx] ✗ Fi.CI.BUILD: failure for DGFX pin_map with rpm

[Patchwork]

== 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

Re: [Intel-gfx] [RFC 1/1] drm/i915/dgfx: Handling of pin_map against rpm

[Tvrtko Ursulin]

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;
>   

Re: [Intel-gfx] [RFC 1/1] drm/i915/dgfx: Handling of pin_map against rpm

[Tvrtko Ursulin]

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;
>   

Re: [Intel-gfx] [RFC 1/1] drm/i915/dgfx: Handling of pin_map against rpm

[kernel test robot]

Hi Anshuman,

[FYI, it's a private test report for your RFC patch.]
[auto build test ERROR on drm-tip/drm-tip]

url:    https://github.com/intel-lab-lkp/linux/commits/Anshuman-Gupta/DGFX-pin_map-with-rpm/20220915-183530
base:   git://anongit.freedesktop.org/drm/drm-tip drm-tip
config: x86_64-randconfig-a005 (https://download.01.org/0day-ci/archive/20220916/202209160041.bepKmLCd-lkp@intel.com/config)
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/b2fef19743a4f93d15f05b06003e884335953797
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Anshuman-Gupta/DGFX-pin_map-with-rpm/20220915-183530
        git checkout b2fef19743a4f93d15f05b06003e884335953797
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/gpu/

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All error/warnings (new ones prefixed by >>):

   In file included from drivers/gpu/drm/i915/i915_driver.c:52:
   In file included from drivers/gpu/drm/i915/display/intel_display_types.h:49:
   In file included from drivers/gpu/drm/i915/i915_vma.h:34:
>> drivers/gpu/drm/i915/gem/i915_gem_object.h:504:2: error: implicit declaration of function 'mutext_lock' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
           mutext_lock(obj->wakeref_lock);
           ^
>> drivers/gpu/drm/i915/gem/i915_gem_object.h:506:25: error: implicit declaration of function 'to_i915' is invalid in C99 [-Werror,-Wimplicit-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:49: error: member reference type 'int' is not a pointer
                   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' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
           mutext_unlock(obj->wakeref_lock);
           ^
   drivers/gpu/drm/i915/gem/i915_gem_object.h:507:2: note: did you mean 'mutex_unlock'?
   include/linux/mutex.h:218:13: note: 'mutex_unlock' declared here
   extern void mutex_unlock(struct mutex *lock);
               ^
   In file included from drivers/gpu/drm/i915/i915_driver.c:66:
   In file included from drivers/gpu/drm/i915/gem/i915_gem_context.h:12:
   In file included from drivers/gpu/drm/i915/gt/intel_context.h:14:
>> drivers/gpu/drm/i915/i915_drv.h:417:40: error: static declaration of 'to_i915' follows non-static declaration
   static inline struct drm_i915_private *to_i915(const struct drm_device *dev)
                                          ^
   drivers/gpu/drm/i915/gem/i915_gem_object.h:506:25: note: previous implicit declaration is here
                   intel_runtime_pm_put(&to_i915(obj->base.dev)->runtime_pm, obj->wakeref);
                                         ^
   5 errors generated.
--
   In file included from drivers/gpu/drm/i915/gt/gen6_engine_cs.c:7:
   In file included from drivers/gpu/drm/i915/gt/intel_engine.h:18:
   In file included from drivers/gpu/drm/i915/gt/intel_gt_types.h:19:
   In file included from drivers/gpu/drm/i915/gt/uc/intel_uc.h:9:
   In file included from drivers/gpu/drm/i915/gt/uc/intel_guc.h:19:
   In file included from drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h:13:
   In file included from drivers/gpu/drm/i915/i915_vma.h:34:
>> drivers/gpu/drm/i915/gem/i915_gem_object.h:504:2: error: implicit declaration of function 'mutext_lock' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
           mutext_lock(obj->wakeref_lock);
           ^
>> drivers/gpu/drm/i915/gem/i915_gem_object.h:506:25: error: implicit declaration of function 'to_i915' is invalid in C99 [-Werror,-Wimplicit-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:49: error: member reference type 'int' is not a pointer
                   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' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
           mutext_unlock(obj->wakeref_lock);
           ^
   drivers/gpu/drm/i915/gem/i915_gem_object.h:507:2: note: did you mean 'mutex_unlock'?
   include/linux/mutex.h:218:13: note: 'mutex_unlock' declared here
   extern void mutex_unlock(struct mutex *lock);
               ^
   4 errors generated.
--
   In file included from drivers/gpu/drm/i915/gem/i915_gem_internal.c:11:
   In file included from drivers/gpu/drm/i915/i915_drv.h:47:
   In file included from drivers/gpu/drm/i915/gt/intel_engine.h:18:
   In file included from drivers/gpu/drm/i915/gt/intel_gt_types.h:19:
   In file included from drivers/gpu/drm/i915/gt/uc/intel_uc.h:9:
   In file included from drivers/gpu/drm/i915/gt/uc/intel_guc.h:19:
   In file included from drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h:13:
   In file included from drivers/gpu/drm/i915/i915_vma.h:34:
>> drivers/gpu/drm/i915/gem/i915_gem_object.h:504:2: error: implicit declaration of function 'mutext_lock' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
           mutext_lock(obj->wakeref_lock);
           ^
>> drivers/gpu/drm/i915/gem/i915_gem_object.h:506:25: error: implicit declaration of function 'to_i915' is invalid in C99 [-Werror,-Wimplicit-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:49: error: member reference type 'int' is not a pointer
                   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' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
           mutext_unlock(obj->wakeref_lock);
           ^
   drivers/gpu/drm/i915/gem/i915_gem_object.h:507:2: note: did you mean 'mutex_unlock'?
   include/linux/mutex.h:218:13: note: 'mutex_unlock' declared here
   extern void mutex_unlock(struct mutex *lock);
               ^
   In file included from drivers/gpu/drm/i915/gem/i915_gem_internal.c:11:
>> drivers/gpu/drm/i915/i915_drv.h:417:40: error: static declaration of 'to_i915' follows non-static declaration
   static inline struct drm_i915_private *to_i915(const struct drm_device *dev)
                                          ^
   drivers/gpu/drm/i915/gem/i915_gem_object.h:506:25: note: previous implicit declaration is here
                   intel_runtime_pm_put(&to_i915(obj->base.dev)->runtime_pm, obj->wakeref);
                                         ^
>> drivers/gpu/drm/i915/gem/i915_gem_internal.c:161:6: warning: shift count >= width of type [-Wshift-count-overflow]
           if (overflows_type(size, obj->base.size))
               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/gpu/drm/i915/i915_utils.h:116:32: note: expanded from macro 'overflows_type'
           (sizeof(x) > sizeof(T) && (x) >> BITS_PER_TYPE(T))
                                         ^  ~~~~~~~~~~~~~~~~
   1 warning and 5 errors generated.
--
   In file included from drivers/gpu/drm/i915/i915_query.c:9:
   In file included from drivers/gpu/drm/i915/i915_drv.h:47:
   In file included from drivers/gpu/drm/i915/gt/intel_engine.h:18:
   In file included from drivers/gpu/drm/i915/gt/intel_gt_types.h:19:
   In file included from drivers/gpu/drm/i915/gt/uc/intel_uc.h:9:
   In file included from drivers/gpu/drm/i915/gt/uc/intel_guc.h:19:
   In file included from drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h:13:
   In file included from drivers/gpu/drm/i915/i915_vma.h:34:
>> drivers/gpu/drm/i915/gem/i915_gem_object.h:504:2: error: implicit declaration of function 'mutext_lock' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
           mutext_lock(obj->wakeref_lock);
           ^
>> drivers/gpu/drm/i915/gem/i915_gem_object.h:506:25: error: implicit declaration of function 'to_i915' is invalid in C99 [-Werror,-Wimplicit-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:49: error: member reference type 'int' is not a pointer
                   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' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
           mutext_unlock(obj->wakeref_lock);
           ^
   drivers/gpu/drm/i915/gem/i915_gem_object.h:507:2: note: did you mean 'mutex_unlock'?
   include/linux/mutex.h:218:13: note: 'mutex_unlock' declared here
   extern void mutex_unlock(struct mutex *lock);
               ^
   In file included from drivers/gpu/drm/i915/i915_query.c:9:
>> drivers/gpu/drm/i915/i915_drv.h:417:40: error: static declaration of 'to_i915' follows non-static declaration
   static inline struct drm_i915_private *to_i915(const struct drm_device *dev)
                                          ^
   drivers/gpu/drm/i915/gem/i915_gem_object.h:506:25: note: previous implicit declaration is here
                   intel_runtime_pm_put(&to_i915(obj->base.dev)->runtime_pm, obj->wakeref);
                                         ^
   drivers/gpu/drm/i915/i915_query.c:584:7: warning: shift count >= width of type [-Wshift-count-overflow]
                   if (overflows_type(item.query_id - 1, unsigned long))
                       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/gpu/drm/i915/i915_utils.h:116:32: note: expanded from macro 'overflows_type'
           (sizeof(x) > sizeof(T) && (x) >> BITS_PER_TYPE(T))
                                         ^  ~~~~~~~~~~~~~~~~
   1 warning and 5 errors generated.
--
   In file included from drivers/gpu/drm/i915/display/intel_fb.c:10:
   In file included from drivers/gpu/drm/i915/i915_drv.h:47:
   In file included from drivers/gpu/drm/i915/gt/intel_engine.h:18:
   In file included from drivers/gpu/drm/i915/gt/intel_gt_types.h:19:
   In file included from drivers/gpu/drm/i915/gt/uc/intel_uc.h:9:
   In file included from drivers/gpu/drm/i915/gt/uc/intel_guc.h:19:
   In file included from drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h:13:
   In file included from drivers/gpu/drm/i915/i915_vma.h:34:
>> drivers/gpu/drm/i915/gem/i915_gem_object.h:504:2: error: implicit declaration of function 'mutext_lock' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
           mutext_lock(obj->wakeref_lock);
           ^
>> drivers/gpu/drm/i915/gem/i915_gem_object.h:506:25: error: implicit declaration of function 'to_i915' is invalid in C99 [-Werror,-Wimplicit-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:49: error: member reference type 'int' is not a pointer
                   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' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
           mutext_unlock(obj->wakeref_lock);
           ^
   drivers/gpu/drm/i915/gem/i915_gem_object.h:507:2: note: did you mean 'mutex_unlock'?
   include/linux/mutex.h:218:13: note: 'mutex_unlock' declared here
   extern void mutex_unlock(struct mutex *lock);
               ^
   In file included from drivers/gpu/drm/i915/display/intel_fb.c:10:
>> drivers/gpu/drm/i915/i915_drv.h:417:40: error: static declaration of 'to_i915' follows non-static declaration
   static inline struct drm_i915_private *to_i915(const struct drm_device *dev)
                                          ^
   drivers/gpu/drm/i915/gem/i915_gem_object.h:506:25: note: previous implicit declaration is here
                   intel_runtime_pm_put(&to_i915(obj->base.dev)->runtime_pm, obj->wakeref);
                                         ^
>> drivers/gpu/drm/i915/display/intel_fb.c:1385:3: warning: shift count >= width of type [-Wshift-count-overflow]
                   assign_chk_ovf(i915, remap_info->size,
                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/gpu/drm/i915/display/intel_fb.c:1356:28: note: expanded from macro 'assign_chk_ovf'
           drm_WARN_ON(&(i915)->drm, overflows_type(val, var)); \
           ~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/gpu/drm/i915/i915_utils.h:116:32: note: expanded from macro 'overflows_type'
           (sizeof(x) > sizeof(T) && (x) >> BITS_PER_TYPE(T))
                                         ^
   include/drm/drm_print.h:593:19: note: expanded from macro 'drm_WARN_ON'
           drm_WARN((drm), (x), "%s",                                      \
           ~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/drm/drm_print.h:583:7: note: expanded from macro 'drm_WARN'
           WARN(condition, "%s %s: " format,                               \
           ~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/asm-generic/bug.h:131:25: note: expanded from macro 'WARN'
           int __ret_warn_on = !!(condition);                              \
                                  ^~~~~~~~~
   1 warning and 5 errors generated.


vim +/mutext_lock +504 drivers/gpu/drm/i915/gem/i915_gem_object.h

   492	
   493	/**
   494	 * i915_gem_object_unpin_map - releases an earlier mapping
   495	 * @obj: the object to unmap
   496	 *
   497	 * After pinning the object and mapping its pages, once you are finished
   498	 * with your access, call i915_gem_object_unpin_map() to release the pin
   499	 * upon the mapping. Once the pin count reaches zero, that mapping may be
   500	 * removed.
   501	 */
   502	static inline void i915_gem_object_unpin_map(struct drm_i915_gem_object *obj)
   503	{
 > 504		mutext_lock(obj->wakeref_lock);
   505		if (!--obj->wakeref_count)
 > 506			intel_runtime_pm_put(&to_i915(obj->base.dev)->runtime_pm, obj->wakeref);
 > 507		mutext_unlock(obj->wakeref_lock);
   508	
   509		i915_gem_object_unpin_pages(obj);
   510	}
   511	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [Intel-gfx] [RFC 1/1] drm/i915/dgfx: Handling of pin_map against rpm

[Gupta, Anshuman]

> -----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;
> >

Re: [Intel-gfx] [RFC 1/1] drm/i915/dgfx: Handling of pin_map against rpm

[Gupta, Anshuman]

> -----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;
> >

Re: [Intel-gfx] [RFC 1/1] drm/i915/dgfx: Handling of pin_map against rpm

[Tvrtko Ursulin]

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;
>   

Re: [Intel-gfx] [RFC 1/1] drm/i915/dgfx: Handling of pin_map against rpm

[Gupta, Anshuman]

> -----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;
> >

Re: [Intel-gfx] [RFC 1/1] drm/i915/dgfx: Handling of pin_map against rpm

[Tvrtko Ursulin]

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