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

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

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
>