[Intel-gfx] [RFC 0/1] Avoid parent bridge rpm on mmap mappings

[Intel-gfx] [RFC 0/1] Avoid parent bridge rpm on mmap mappings

[Anshuman Gupta]

RFC proposal to get community feedback to handle the lmem
mmap memory mappings. We can have two solutions.

1. Avoid rpm for any mmap mapping.
2. Update the page tables while entering to runtime suspend to raise
   a page fault while device in D3.

Solution 2 will incur huge latency versus solution 1 which effectively 
disable the runtime PM.

Caveat with solution 1 that, it doesn't address the case when user 
directly maps PCI Bar resource i.e. below mentioned resource. 
"/sys/bus/pci/devices/0000\:03\:00.0/resource2" 

Anshuman Gupta (1):
  drm/i915/dgfx: Avoid parent bridge rpm on mmap mappings

 drivers/gpu/drm/i915/gem/i915_gem_mman.c | 11 ++++++++
 drivers/gpu/drm/i915/gem/i915_gem_ttm.c  |  8 ++++++
 drivers/gpu/drm/i915/intel_runtime_pm.c  | 35 ++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_runtime_pm.h  |  2 ++
 4 files changed, 56 insertions(+)

-- 
2.26.2

[Intel-gfx] [RFC 1/1] drm/i915/dgfx: Avoid parent bridge rpm on mmap mappings

[Anshuman Gupta]

As per PCIe Spec Section 5.3,
When a Type 1 Function associated with a Switch/Root
Port (a “virtual bridge”) is in a non-D0 power state,
it will emulate the behavior of a conventional PCI bridge
in its handling of Memory, I/O, and Configuration
Requests and Completions. All Memory and I/O requests
flowing Downstream are terminated as Unsupported Requests.

Due to above limitations when Intel DGFX Cards graphics
PCI func's upstream bridge(referred as VSP) enters to D3,
all mmap memory mapping associated with lmem objects
reads 0xff, therefore avoiding VSP runtime suspend
accordingly.

This will make sure that user can read valid data from lmem,
while DGFX Card graphics PCI func is in D3 state.

Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
---
 drivers/gpu/drm/i915/gem/i915_gem_mman.c | 11 ++++++++
 drivers/gpu/drm/i915/gem/i915_gem_ttm.c  |  8 ++++++
 drivers/gpu/drm/i915/intel_runtime_pm.c  | 35 ++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_runtime_pm.h  |  2 ++
 4 files changed, 56 insertions(+)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
index 0c5c43852e24..968bed5b56d3 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
@@ -845,6 +845,10 @@ static void vm_open(struct vm_area_struct *vma)
 	struct drm_i915_gem_object *obj = mmo->obj;
 
 	GEM_BUG_ON(!obj);
+
+	if (i915_gem_object_is_lmem(obj))
+		intel_runtime_pm_get_vsp(to_i915(obj->base.dev));
+
 	i915_gem_object_get(obj);
 }
 
@@ -854,6 +858,10 @@ static void vm_close(struct vm_area_struct *vma)
 	struct drm_i915_gem_object *obj = mmo->obj;
 
 	GEM_BUG_ON(!obj);
+
+	if (i915_gem_object_is_lmem(obj))
+		intel_runtime_pm_put_vsp(to_i915(obj->base.dev));
+
 	i915_gem_object_put(obj);
 }
 
@@ -972,6 +980,9 @@ int i915_gem_mmap(struct file *filp, struct vm_area_struct *vma)
 		return PTR_ERR(anon);
 	}
 
+	if (i915_gem_object_is_lmem(obj))
+		intel_runtime_pm_get_vsp(to_i915(obj->base.dev));
+
 	vma->vm_flags |= VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP | VM_IO;
 
 	/*
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
index 5a5cf332d8a5..bcacd95fdbc1 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
@@ -1101,6 +1101,10 @@ static void ttm_vm_open(struct vm_area_struct *vma)
 		i915_ttm_to_gem(vma->vm_private_data);
 
 	GEM_BUG_ON(!obj);
+
+	if (i915_gem_object_is_lmem(obj))
+		intel_runtime_pm_get_vsp(to_i915(obj->base.dev));
+
 	i915_gem_object_get(obj);
 }
 
@@ -1110,6 +1114,10 @@ static void ttm_vm_close(struct vm_area_struct *vma)
 		i915_ttm_to_gem(vma->vm_private_data);
 
 	GEM_BUG_ON(!obj);
+
+	if (i915_gem_object_is_lmem(obj))
+		intel_runtime_pm_put_vsp(to_i915(obj->base.dev));
+
 	i915_gem_object_put(obj);
 }
 
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index 6ed5786bcd29..a5557918674f 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -646,3 +646,38 @@ void intel_runtime_pm_init_early(struct intel_runtime_pm *rpm)
 
 	init_intel_runtime_pm_wakeref(rpm);
 }
+
+void intel_runtime_pm_get_vsp(struct drm_i915_private *i915)
+{
+	struct pci_dev *pdev = to_pci_dev(i915->drm.dev), *vsp;
+
+	if (!IS_DGFX(i915))
+		return;
+
+	vsp = pci_upstream_bridge(pdev);
+	if (!vsp) {
+		drm_err(&i915->drm, "Failed to get the PCI upstream bridge\n");
+		return;
+	}
+
+	pci_dbg(vsp, "get runtime usage count\n");
+	pm_runtime_get_sync(&vsp->dev);
+}
+
+void intel_runtime_pm_put_vsp(struct drm_i915_private *i915)
+{
+	struct pci_dev *pdev = to_pci_dev(i915->drm.dev), *vsp;
+
+	if (!IS_DGFX(i915))
+		return;
+
+	vsp = pci_upstream_bridge(pdev);
+	if (!vsp) {
+		drm_err(&i915->drm, "Failed to get the PCI upstream bridge\n");
+		return;
+	}
+
+	pci_dbg(vsp, "put runtime usage count\n");
+	pm_runtime_mark_last_busy(&vsp->dev);
+	pm_runtime_put(&vsp->dev);
+}
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.h b/drivers/gpu/drm/i915/intel_runtime_pm.h
index d9160e3ff4af..b86843bf4f5d 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.h
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.h
@@ -173,6 +173,8 @@ void intel_runtime_pm_init_early(struct intel_runtime_pm *rpm);
 void intel_runtime_pm_enable(struct intel_runtime_pm *rpm);
 void intel_runtime_pm_disable(struct intel_runtime_pm *rpm);
 void intel_runtime_pm_driver_release(struct intel_runtime_pm *rpm);
+void intel_runtime_pm_get_vsp(struct drm_i915_private *i915);
+void intel_runtime_pm_put_vsp(struct drm_i915_private *i915);
 
 intel_wakeref_t intel_runtime_pm_get(struct intel_runtime_pm *rpm);
 intel_wakeref_t intel_runtime_pm_get_if_in_use(struct intel_runtime_pm *rpm);
-- 
2.26.2

Re: [Intel-gfx] [RFC 1/1] drm/i915/dgfx: Avoid parent bridge rpm on mmap mappings

[Rodrigo Vivi]

On Mon, Aug 08, 2022 at 04:05:55PM +0530, Anshuman Gupta wrote:
> As per PCIe Spec Section 5.3,
> When a Type 1 Function associated with a Switch/Root
> Port (a “virtual bridge”) is in a non-D0 power state,
> it will emulate the behavior of a conventional PCI bridge
> in its handling of Memory, I/O, and Configuration
> Requests and Completions. All Memory and I/O requests
> flowing Downstream are terminated as Unsupported Requests.
> 
> Due to above limitations when Intel DGFX Cards graphics
> PCI func's upstream bridge(referred as VSP) enters to D3,
> all mmap memory mapping associated with lmem objects
> reads 0xff, therefore avoiding VSP runtime suspend
> accordingly.
> 
> This will make sure that user can read valid data from lmem,
> while DGFX Card graphics PCI func is in D3 state.
> 
> Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
> ---
>  drivers/gpu/drm/i915/gem/i915_gem_mman.c | 11 ++++++++
>  drivers/gpu/drm/i915/gem/i915_gem_ttm.c  |  8 ++++++
>  drivers/gpu/drm/i915/intel_runtime_pm.c  | 35 ++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_runtime_pm.h  |  2 ++
>  4 files changed, 56 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> index 0c5c43852e24..968bed5b56d3 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> @@ -845,6 +845,10 @@ static void vm_open(struct vm_area_struct *vma)
>  	struct drm_i915_gem_object *obj = mmo->obj;
>  
>  	GEM_BUG_ON(!obj);
> +
> +	if (i915_gem_object_is_lmem(obj))
> +		intel_runtime_pm_get_vsp(to_i915(obj->base.dev));
> +
>  	i915_gem_object_get(obj);
>  }
>  
> @@ -854,6 +858,10 @@ static void vm_close(struct vm_area_struct *vma)
>  	struct drm_i915_gem_object *obj = mmo->obj;
>  
>  	GEM_BUG_ON(!obj);
> +
> +	if (i915_gem_object_is_lmem(obj))
> +		intel_runtime_pm_put_vsp(to_i915(obj->base.dev));
> +
>  	i915_gem_object_put(obj);
>  }
>  
> @@ -972,6 +980,9 @@ int i915_gem_mmap(struct file *filp, struct vm_area_struct *vma)
>  		return PTR_ERR(anon);
>  	}
>  
> +	if (i915_gem_object_is_lmem(obj))
> +		intel_runtime_pm_get_vsp(to_i915(obj->base.dev));
> +
>  	vma->vm_flags |= VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP | VM_IO;
>  
>  	/*
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> index 5a5cf332d8a5..bcacd95fdbc1 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> @@ -1101,6 +1101,10 @@ static void ttm_vm_open(struct vm_area_struct *vma)
>  		i915_ttm_to_gem(vma->vm_private_data);
>  
>  	GEM_BUG_ON(!obj);
> +
> +	if (i915_gem_object_is_lmem(obj))
> +		intel_runtime_pm_get_vsp(to_i915(obj->base.dev));
> +
>  	i915_gem_object_get(obj);
>  }
>  
> @@ -1110,6 +1114,10 @@ static void ttm_vm_close(struct vm_area_struct *vma)
>  		i915_ttm_to_gem(vma->vm_private_data);
>  
>  	GEM_BUG_ON(!obj);
> +
> +	if (i915_gem_object_is_lmem(obj))
> +		intel_runtime_pm_put_vsp(to_i915(obj->base.dev));
> +
>  	i915_gem_object_put(obj);
>  }

we need to ensure the runtime pm get / put at dma buf attach & detach as well, no?

>  
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index 6ed5786bcd29..a5557918674f 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -646,3 +646,38 @@ void intel_runtime_pm_init_early(struct intel_runtime_pm *rpm)
>  
>  	init_intel_runtime_pm_wakeref(rpm);
>  }
> +
> +void intel_runtime_pm_get_vsp(struct drm_i915_private *i915)
> +{
> +	struct pci_dev *pdev = to_pci_dev(i915->drm.dev), *vsp;
> +
> +	if (!IS_DGFX(i915))
> +		return;
> +
> +	vsp = pci_upstream_bridge(pdev);
> +	if (!vsp) {
> +		drm_err(&i915->drm, "Failed to get the PCI upstream bridge\n");
> +		return;
> +	}
> +
> +	pci_dbg(vsp, "get runtime usage count\n");

we should always prefer the drm_dbg in our subsystem

> +	pm_runtime_get_sync(&vsp->dev);

why? I believe that grabbing our own ref would be enough to block the
upstream chain. I don't understand why this is such an special case
that we don't see any other driver in the linux tree having to do such
a thing. what am I missing?

> +}
> +
> +void intel_runtime_pm_put_vsp(struct drm_i915_private *i915)
> +{
> +	struct pci_dev *pdev = to_pci_dev(i915->drm.dev), *vsp;
> +
> +	if (!IS_DGFX(i915))
> +		return;
> +
> +	vsp = pci_upstream_bridge(pdev);
> +	if (!vsp) {
> +		drm_err(&i915->drm, "Failed to get the PCI upstream bridge\n");
> +		return;
> +	}
> +
> +	pci_dbg(vsp, "put runtime usage count\n");
> +	pm_runtime_mark_last_busy(&vsp->dev);
> +	pm_runtime_put(&vsp->dev);
> +}
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.h b/drivers/gpu/drm/i915/intel_runtime_pm.h
> index d9160e3ff4af..b86843bf4f5d 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.h
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.h
> @@ -173,6 +173,8 @@ void intel_runtime_pm_init_early(struct intel_runtime_pm *rpm);
>  void intel_runtime_pm_enable(struct intel_runtime_pm *rpm);
>  void intel_runtime_pm_disable(struct intel_runtime_pm *rpm);
>  void intel_runtime_pm_driver_release(struct intel_runtime_pm *rpm);
> +void intel_runtime_pm_get_vsp(struct drm_i915_private *i915);
> +void intel_runtime_pm_put_vsp(struct drm_i915_private *i915);
>  
>  intel_wakeref_t intel_runtime_pm_get(struct intel_runtime_pm *rpm);
>  intel_wakeref_t intel_runtime_pm_get_if_in_use(struct intel_runtime_pm *rpm);
> -- 
> 2.26.2
> 

Re: [Intel-gfx] [RFC 1/1] drm/i915/dgfx: Avoid parent bridge rpm on mmap mappings

[Gupta, Anshuman]

> -----Original Message-----
> From: Vivi, Rodrigo <rodrigo.vivi@intel.com>
> Sent: Tuesday, August 9, 2022 8:36 PM
> To: Gupta, Anshuman <anshuman.gupta@intel.com>
> Cc: intel-gfx@lists.freedesktop.org; daniel@ffwll.ch; Wilson, Chris P
> <chris.p.wilson@intel.com>; dri-devel@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [RFC 1/1] drm/i915/dgfx: Avoid parent bridge rpm on
> mmap mappings
> 
> On Mon, Aug 08, 2022 at 04:05:55PM +0530, Anshuman Gupta wrote:
> > As per PCIe Spec Section 5.3,
> > When a Type 1 Function associated with a Switch/Root Port (a “virtual
> > bridge”) is in a non-D0 power state, it will emulate the behavior of a
> > conventional PCI bridge in its handling of Memory, I/O, and
> > Configuration Requests and Completions. All Memory and I/O requests
> > flowing Downstream are terminated as Unsupported Requests.
> >
> > Due to above limitations when Intel DGFX Cards graphics PCI func's
> > upstream bridge(referred as VSP) enters to D3, all mmap memory mapping
> > associated with lmem objects reads 0xff, therefore avoiding VSP
> > runtime suspend accordingly.
> >
> > This will make sure that user can read valid data from lmem, while
> > DGFX Card graphics PCI func is in D3 state.
> >
> > Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
> > ---
> >  drivers/gpu/drm/i915/gem/i915_gem_mman.c | 11 ++++++++
> > drivers/gpu/drm/i915/gem/i915_gem_ttm.c  |  8 ++++++
> > drivers/gpu/drm/i915/intel_runtime_pm.c  | 35 ++++++++++++++++++++++++
> > drivers/gpu/drm/i915/intel_runtime_pm.h  |  2 ++
> >  4 files changed, 56 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> > b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> > index 0c5c43852e24..968bed5b56d3 100644
> > --- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> > @@ -845,6 +845,10 @@ static void vm_open(struct vm_area_struct *vma)
> >  	struct drm_i915_gem_object *obj = mmo->obj;
> >
> >  	GEM_BUG_ON(!obj);
> > +
> > +	if (i915_gem_object_is_lmem(obj))
> > +		intel_runtime_pm_get_vsp(to_i915(obj->base.dev));
> > +
> >  	i915_gem_object_get(obj);
> >  }
> >
> > @@ -854,6 +858,10 @@ static void vm_close(struct vm_area_struct *vma)
> >  	struct drm_i915_gem_object *obj = mmo->obj;
> >
> >  	GEM_BUG_ON(!obj);
> > +
> > +	if (i915_gem_object_is_lmem(obj))
> > +		intel_runtime_pm_put_vsp(to_i915(obj->base.dev));
> > +
> >  	i915_gem_object_put(obj);
> >  }
> >
> > @@ -972,6 +980,9 @@ int i915_gem_mmap(struct file *filp, struct
> vm_area_struct *vma)
> >  		return PTR_ERR(anon);
> >  	}
> >
> > +	if (i915_gem_object_is_lmem(obj))
> > +		intel_runtime_pm_get_vsp(to_i915(obj->base.dev));
> > +
> >  	vma->vm_flags |= VM_PFNMAP | VM_DONTEXPAND |
> VM_DONTDUMP | VM_IO;
> >
> >  	/*
> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> > b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> > index 5a5cf332d8a5..bcacd95fdbc1 100644
> > --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> > @@ -1101,6 +1101,10 @@ static void ttm_vm_open(struct vm_area_struct
> *vma)
> >  		i915_ttm_to_gem(vma->vm_private_data);
> >
> >  	GEM_BUG_ON(!obj);
> > +
> > +	if (i915_gem_object_is_lmem(obj))
> > +		intel_runtime_pm_get_vsp(to_i915(obj->base.dev));
> > +
> >  	i915_gem_object_get(obj);
> >  }
> >
> > @@ -1110,6 +1114,10 @@ static void ttm_vm_close(struct vm_area_struct
> *vma)
> >  		i915_ttm_to_gem(vma->vm_private_data);
> >
> >  	GEM_BUG_ON(!obj);
> > +
> > +	if (i915_gem_object_is_lmem(obj))
> > +		intel_runtime_pm_put_vsp(to_i915(obj->base.dev));
> > +
> >  	i915_gem_object_put(obj);
> >  }
> 
> we need to ensure the runtime pm get / put at dma buf attach & detach as well,
> no?
AFAIU dma-buf import does migrate an object from lmem to smem , so effectively the memory mapping access will be 
limited to smem. So there will not be any issue here, unless i am missing something . 

Thanks,
Anshuman Gupta.

> 
> >
> > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > index 6ed5786bcd29..a5557918674f 100644
> > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > @@ -646,3 +646,38 @@ void intel_runtime_pm_init_early(struct
> > intel_runtime_pm *rpm)
> >
> >  	init_intel_runtime_pm_wakeref(rpm);
> >  }
> > +
> > +void intel_runtime_pm_get_vsp(struct drm_i915_private *i915) {
> > +	struct pci_dev *pdev = to_pci_dev(i915->drm.dev), *vsp;
> > +
> > +	if (!IS_DGFX(i915))
> > +		return;
> > +
> > +	vsp = pci_upstream_bridge(pdev);
> > +	if (!vsp) {
> > +		drm_err(&i915->drm, "Failed to get the PCI upstream
> bridge\n");
> > +		return;
> > +	}
> > +
> > +	pci_dbg(vsp, "get runtime usage count\n");
> 
> we should always prefer the drm_dbg in our subsystem
This print will be useful in case any leaks from users around munmap.
The device bdf with pci_dbg will be useful in such cases. 
> 
> > +	pm_runtime_get_sync(&vsp->dev);
> 
> why? I believe that grabbing our own ref would be enough to block the
> upstream chain. I don't understand why this is such an special case that we don't
> see any other driver in the linux tree having to do such a thing. what am I
> missing?
IMHO by grabbing our device wakeref will keep it in D0 , with its parent bridge the gfx endpoint 
function can transition to d3(d3hot) to save some power ?

If we are sure there are not any power saving with only gfx endpoint function in D3hot, probably
in that case it will be correct to get the wakeref of our own device.

Thanks,
Anshuman Gupta.   
> 
> > +}
> > +
> > +void intel_runtime_pm_put_vsp(struct drm_i915_private *i915) {
> > +	struct pci_dev *pdev = to_pci_dev(i915->drm.dev), *vsp;
> > +
> > +	if (!IS_DGFX(i915))
> > +		return;
> > +
> > +	vsp = pci_upstream_bridge(pdev);
> > +	if (!vsp) {
> > +		drm_err(&i915->drm, "Failed to get the PCI upstream
> bridge\n");
> > +		return;
> > +	}
> > +
> > +	pci_dbg(vsp, "put runtime usage count\n");
> > +	pm_runtime_mark_last_busy(&vsp->dev);
> > +	pm_runtime_put(&vsp->dev);
> > +}
> > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.h
> > b/drivers/gpu/drm/i915/intel_runtime_pm.h
> > index d9160e3ff4af..b86843bf4f5d 100644
> > --- a/drivers/gpu/drm/i915/intel_runtime_pm.h
> > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.h
> > @@ -173,6 +173,8 @@ void intel_runtime_pm_init_early(struct
> > intel_runtime_pm *rpm);  void intel_runtime_pm_enable(struct
> > intel_runtime_pm *rpm);  void intel_runtime_pm_disable(struct
> > intel_runtime_pm *rpm);  void intel_runtime_pm_driver_release(struct
> > intel_runtime_pm *rpm);
> > +void intel_runtime_pm_get_vsp(struct drm_i915_private *i915); void
> > +intel_runtime_pm_put_vsp(struct drm_i915_private *i915);
> >
> >  intel_wakeref_t intel_runtime_pm_get(struct intel_runtime_pm *rpm);
> > intel_wakeref_t intel_runtime_pm_get_if_in_use(struct intel_runtime_pm
> > *rpm);
> > --
> > 2.26.2
> >

Re: [Intel-gfx] [RFC 1/1] drm/i915/dgfx: Avoid parent bridge rpm on mmap mappings

[Gupta, Anshuman]

> -----Original Message-----
> From: Vivi, Rodrigo <rodrigo.vivi@intel.com>
> Sent: Tuesday, August 9, 2022 8:36 PM
> To: Gupta, Anshuman <anshuman.gupta@intel.com>
> Cc: intel-gfx@lists.freedesktop.org; daniel@ffwll.ch; Wilson, Chris P
> <chris.p.wilson@intel.com>; dri-devel@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [RFC 1/1] drm/i915/dgfx: Avoid parent bridge rpm on
> mmap mappings
> 
> On Mon, Aug 08, 2022 at 04:05:55PM +0530, Anshuman Gupta wrote:
> > As per PCIe Spec Section 5.3,
> > When a Type 1 Function associated with a Switch/Root Port (a “virtual
> > bridge”) is in a non-D0 power state, it will emulate the behavior of a
> > conventional PCI bridge in its handling of Memory, I/O, and
> > Configuration Requests and Completions. All Memory and I/O requests
> > flowing Downstream are terminated as Unsupported Requests.
> >
> > Due to above limitations when Intel DGFX Cards graphics PCI func's
> > upstream bridge(referred as VSP) enters to D3, all mmap memory mapping
> > associated with lmem objects reads 0xff, therefore avoiding VSP
> > runtime suspend accordingly.
> >
> > This will make sure that user can read valid data from lmem, while
> > DGFX Card graphics PCI func is in D3 state.
> >
> > Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
> > ---
> >  drivers/gpu/drm/i915/gem/i915_gem_mman.c | 11 ++++++++
> > drivers/gpu/drm/i915/gem/i915_gem_ttm.c  |  8 ++++++
> > drivers/gpu/drm/i915/intel_runtime_pm.c  | 35 ++++++++++++++++++++++++
> > drivers/gpu/drm/i915/intel_runtime_pm.h  |  2 ++
> >  4 files changed, 56 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> > b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> > index 0c5c43852e24..968bed5b56d3 100644
> > --- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> > @@ -845,6 +845,10 @@ static void vm_open(struct vm_area_struct *vma)
> >  	struct drm_i915_gem_object *obj = mmo->obj;
> >
> >  	GEM_BUG_ON(!obj);
> > +
> > +	if (i915_gem_object_is_lmem(obj))
> > +		intel_runtime_pm_get_vsp(to_i915(obj->base.dev));
> > +
> >  	i915_gem_object_get(obj);
> >  }
> >
> > @@ -854,6 +858,10 @@ static void vm_close(struct vm_area_struct *vma)
> >  	struct drm_i915_gem_object *obj = mmo->obj;
> >
> >  	GEM_BUG_ON(!obj);
> > +
> > +	if (i915_gem_object_is_lmem(obj))
> > +		intel_runtime_pm_put_vsp(to_i915(obj->base.dev));
> > +
> >  	i915_gem_object_put(obj);
> >  }
> >
> > @@ -972,6 +980,9 @@ int i915_gem_mmap(struct file *filp, struct
> vm_area_struct *vma)
> >  		return PTR_ERR(anon);
> >  	}
> >
> > +	if (i915_gem_object_is_lmem(obj))
> > +		intel_runtime_pm_get_vsp(to_i915(obj->base.dev));
> > +
> >  	vma->vm_flags |= VM_PFNMAP | VM_DONTEXPAND |
> VM_DONTDUMP | VM_IO;
> >
> >  	/*
> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> > b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> > index 5a5cf332d8a5..bcacd95fdbc1 100644
> > --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> > @@ -1101,6 +1101,10 @@ static void ttm_vm_open(struct vm_area_struct
> *vma)
> >  		i915_ttm_to_gem(vma->vm_private_data);
> >
> >  	GEM_BUG_ON(!obj);
> > +
> > +	if (i915_gem_object_is_lmem(obj))
> > +		intel_runtime_pm_get_vsp(to_i915(obj->base.dev));
> > +
> >  	i915_gem_object_get(obj);
> >  }
> >
> > @@ -1110,6 +1114,10 @@ static void ttm_vm_close(struct vm_area_struct
> *vma)
> >  		i915_ttm_to_gem(vma->vm_private_data);
> >
> >  	GEM_BUG_ON(!obj);
> > +
> > +	if (i915_gem_object_is_lmem(obj))
> > +		intel_runtime_pm_put_vsp(to_i915(obj->base.dev));
> > +
> >  	i915_gem_object_put(obj);
> >  }
> 
> we need to ensure the runtime pm get / put at dma buf attach & detach as well,
> no?
> 
> >
> > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > index 6ed5786bcd29..a5557918674f 100644
> > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > @@ -646,3 +646,38 @@ void intel_runtime_pm_init_early(struct
> > intel_runtime_pm *rpm)
> >
> >  	init_intel_runtime_pm_wakeref(rpm);
> >  }
> > +
> > +void intel_runtime_pm_get_vsp(struct drm_i915_private *i915) {
> > +	struct pci_dev *pdev = to_pci_dev(i915->drm.dev), *vsp;
> > +
> > +	if (!IS_DGFX(i915))
> > +		return;
> > +
> > +	vsp = pci_upstream_bridge(pdev);
> > +	if (!vsp) {
> > +		drm_err(&i915->drm, "Failed to get the PCI upstream
> bridge\n");
> > +		return;
> > +	}
> > +
> > +	pci_dbg(vsp, "get runtime usage count\n");
> 
> we should always prefer the drm_dbg in our subsystem
> 
> > +	pm_runtime_get_sync(&vsp->dev);
> 
> why? I believe that grabbing our own ref would be enough to block the
> upstream chain. I don't understand why this is such an special case that we don't
> see any other driver in the linux tree having to do such a thing. what am I
> missing?
Hi Rodrigo ,
I was trying to get wakeref for i915 device in i915_gem_map, vm_open, vm_close hook.
But this does not looks like a scalable solution with wakeref cookie in mmap hooks.
Considering the scenario.
I915 object has embedded a wakeref cookie in side gem object.
obj->wakeref = intel_runtime_pm_get(rpm) in i915_gem_mmap and vm_open()
intel_runtime_pm_put(rpm, obj->wakeref) in vm_close().
vm_open() will be called for any fork() and vm_close() will get call for any instance of vm_open as well as for munmap().
Now it will difficult to track the correct obj->wakeref cookie here, it may lead to unrelated wakeref issue.
Till we get a robust solution by invalidating the mmap mapping, can we consider this to as temporary solution.

Thanks,
Anshuman Gupta.

> 
> > +}
> > +
> > +void intel_runtime_pm_put_vsp(struct drm_i915_private *i915) {
> > +	struct pci_dev *pdev = to_pci_dev(i915->drm.dev), *vsp;
> > +
> > +	if (!IS_DGFX(i915))
> > +		return;
> > +
> > +	vsp = pci_upstream_bridge(pdev);
> > +	if (!vsp) {
> > +		drm_err(&i915->drm, "Failed to get the PCI upstream
> bridge\n");
> > +		return;
> > +	}
> > +
> > +	pci_dbg(vsp, "put runtime usage count\n");
> > +	pm_runtime_mark_last_busy(&vsp->dev);
> > +	pm_runtime_put(&vsp->dev);
> > +}
> > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.h
> > b/drivers/gpu/drm/i915/intel_runtime_pm.h
> > index d9160e3ff4af..b86843bf4f5d 100644
> > --- a/drivers/gpu/drm/i915/intel_runtime_pm.h
> > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.h
> > @@ -173,6 +173,8 @@ void intel_runtime_pm_init_early(struct
> > intel_runtime_pm *rpm);  void intel_runtime_pm_enable(struct
> > intel_runtime_pm *rpm);  void intel_runtime_pm_disable(struct
> > intel_runtime_pm *rpm);  void intel_runtime_pm_driver_release(struct
> > intel_runtime_pm *rpm);
> > +void intel_runtime_pm_get_vsp(struct drm_i915_private *i915); void
> > +intel_runtime_pm_put_vsp(struct drm_i915_private *i915);
> >
> >  intel_wakeref_t intel_runtime_pm_get(struct intel_runtime_pm *rpm);
> > intel_wakeref_t intel_runtime_pm_get_if_in_use(struct intel_runtime_pm
> > *rpm);
> > --
> > 2.26.2
> >

[Intel-gfx] ✗ Fi.CI.SPARSE: warning for Avoid parent bridge rpm on mmap mappings

[Patchwork]

== Series Details ==

Series: Avoid parent bridge rpm on mmap mappings
URL   : https://patchwork.freedesktop.org/series/107061/
State : warning

== Summary ==

Error: dim sparse failed
Sparse version: v0.6.2
Fast mode used, each commit won't be checked separately.

[Intel-gfx] ✗ Fi.CI.BAT: failure for Avoid parent bridge rpm on mmap mappings

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 7403 bytes --]

== Series Details ==

Series: Avoid parent bridge rpm on mmap mappings
URL   : https://patchwork.freedesktop.org/series/107061/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_11972 -> Patchwork_107061v1
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_107061v1 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_107061v1, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_107061v1/index.html

Participating hosts (38 -> 39)
------------------------------

  Additional (1): bat-dg2-8 

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_107061v1:

### IGT changes ###

#### Possible regressions ####

  * igt@i915_selftest@live@mman:
    - bat-dg1-5:          [PASS][1] -> [DMESG-FAIL][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11972/bat-dg1-5/igt@i915_selftest@live@mman.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_107061v1/bat-dg1-5/igt@i915_selftest@live@mman.html
    - bat-dg1-6:          [PASS][3] -> [DMESG-FAIL][4]
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11972/bat-dg1-6/igt@i915_selftest@live@mman.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_107061v1/bat-dg1-6/igt@i915_selftest@live@mman.html

  
#### Suppressed ####

  The following results come from untrusted machines, tests, or statuses.
  They do not affect the overall result.

  * igt@i915_pm_rpm@module-reload:
    - {bat-dg2-10}:       NOTRUN -> [DMESG-WARN][5]
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_107061v1/bat-dg2-10/igt@i915_pm_rpm@module-reload.html

  * igt@i915_selftest@live@mman:
    - {bat-dg2-8}:        NOTRUN -> [DMESG-FAIL][6]
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_107061v1/bat-dg2-8/igt@i915_selftest@live@mman.html

  
Known issues
------------

  Here are the changes found in Patchwork_107061v1 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@i915_selftest@live@hangcheck:
    - fi-hsw-g3258:       [PASS][7] -> [INCOMPLETE][8] ([i915#3303] / [i915#4785])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11972/fi-hsw-g3258/igt@i915_selftest@live@hangcheck.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_107061v1/fi-hsw-g3258/igt@i915_selftest@live@hangcheck.html

  * igt@kms_chamelium@common-hpd-after-suspend:
    - fi-bdw-5557u:       NOTRUN -> [SKIP][9] ([fdo#109271] / [fdo#111827])
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_107061v1/fi-bdw-5557u/igt@kms_chamelium@common-hpd-after-suspend.html

  * igt@runner@aborted:
    - fi-hsw-g3258:       NOTRUN -> [FAIL][10] ([fdo#109271] / [i915#4312] / [i915#6246])
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_107061v1/fi-hsw-g3258/igt@runner@aborted.html

  
#### Possible fixes ####

  * igt@core_hotunplug@unbind-rebind:
    - {bat-dg2-10}:       [DMESG-WARN][11] -> [PASS][12]
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11972/bat-dg2-10/igt@core_hotunplug@unbind-rebind.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_107061v1/bat-dg2-10/igt@core_hotunplug@unbind-rebind.html

  * igt@gem_exec_suspend@basic-s0@smem:
    - {bat-adlm-1}:       [DMESG-WARN][13] ([i915#2867]) -> [PASS][14] +1 similar issue
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11972/bat-adlm-1/igt@gem_exec_suspend@basic-s0@smem.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_107061v1/bat-adlm-1/igt@gem_exec_suspend@basic-s0@smem.html

  * igt@i915_selftest@live@requests:
    - {bat-rpls-1}:       [DMESG-FAIL][15] ([i915#5087] / [i915#6257]) -> [PASS][16]
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11972/bat-rpls-1/igt@i915_selftest@live@requests.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_107061v1/bat-rpls-1/igt@i915_selftest@live@requests.html

  * igt@i915_suspend@basic-s3-without-i915:
    - fi-bdw-5557u:       [INCOMPLETE][17] ([i915#146]) -> [PASS][18]
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11972/fi-bdw-5557u/igt@i915_suspend@basic-s3-without-i915.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_107061v1/fi-bdw-5557u/igt@i915_suspend@basic-s3-without-i915.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109285]: https://bugs.freedesktop.org/show_bug.cgi?id=109285
  [fdo#111827]: https://bugs.freedesktop.org/show_bug.cgi?id=111827
  [i915#1072]: https://gitlab.freedesktop.org/drm/intel/issues/1072
  [i915#1155]: https://gitlab.freedesktop.org/drm/intel/issues/1155
  [i915#146]: https://gitlab.freedesktop.org/drm/intel/issues/146
  [i915#2582]: https://gitlab.freedesktop.org/drm/intel/issues/2582
  [i915#2867]: https://gitlab.freedesktop.org/drm/intel/issues/2867
  [i915#3291]: https://gitlab.freedesktop.org/drm/intel/issues/3291
  [i915#3303]: https://gitlab.freedesktop.org/drm/intel/issues/3303
  [i915#3555]: https://gitlab.freedesktop.org/drm/intel/issues/3555
  [i915#3595]: https://gitlab.freedesktop.org/drm/intel/issues/3595
  [i915#3708]: https://gitlab.freedesktop.org/drm/intel/issues/3708
  [i915#4077]: https://gitlab.freedesktop.org/drm/intel/issues/4077
  [i915#4079]: https://gitlab.freedesktop.org/drm/intel/issues/4079
  [i915#4083]: https://gitlab.freedesktop.org/drm/intel/issues/4083
  [i915#4212]: https://gitlab.freedesktop.org/drm/intel/issues/4212
  [i915#4215]: https://gitlab.freedesktop.org/drm/intel/issues/4215
  [i915#4312]: https://gitlab.freedesktop.org/drm/intel/issues/4312
  [i915#4579]: https://gitlab.freedesktop.org/drm/intel/issues/4579
  [i915#4613]: https://gitlab.freedesktop.org/drm/intel/issues/4613
  [i915#4785]: https://gitlab.freedesktop.org/drm/intel/issues/4785
  [i915#4873]: https://gitlab.freedesktop.org/drm/intel/issues/4873
  [i915#4983]: https://gitlab.freedesktop.org/drm/intel/issues/4983
  [i915#5087]: https://gitlab.freedesktop.org/drm/intel/issues/5087
  [i915#5190]: https://gitlab.freedesktop.org/drm/intel/issues/5190
  [i915#5270]: https://gitlab.freedesktop.org/drm/intel/issues/5270
  [i915#5274]: https://gitlab.freedesktop.org/drm/intel/issues/5274
  [i915#5354]: https://gitlab.freedesktop.org/drm/intel/issues/5354
  [i915#6246]: https://gitlab.freedesktop.org/drm/intel/issues/6246
  [i915#6257]: https://gitlab.freedesktop.org/drm/intel/issues/6257


Build changes
-------------

  * Linux: CI_DRM_11972 -> Patchwork_107061v1

  CI-20190529: 20190529
  CI_DRM_11972: ae25b9088f6c94a6f9cd12f747c3dacd8a161502 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_6615: a98a66399db9939b16e1cb3b47055f400834affb @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_107061v1: ae25b9088f6c94a6f9cd12f747c3dacd8a161502 @ git://anongit.freedesktop.org/gfx-ci/linux


### Linux commits

1385a598a3a2 drm/i915/dgfx: Avoid parent bridge rpm on mmap mappings

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_107061v1/index.html

[-- Attachment #2: Type: text/html, Size: 6957 bytes --]