Re: [RFC 03/34] drm/xe: Fix display runtime_pm handling

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

On Mon, Feb 05, 2024 at 09:11:37AM +0000, Matthew Auld wrote:
> On 26/01/2024 20:30, Rodrigo Vivi wrote:
> > i915's intel_runtime_pm_get_if_in_use actually calls the
> > pm_runtime_get_if_active() with ign_usage_count = false, but Xe
> > was erroneously calling it with true because of the mem_access cases.
> 
> Good catch.
> 
> > This can lead to unbalanced references.
> 
> Is there an actual imbalance here though? Is it not just a case of being
> overzealous in keeping the device awake when it is not currently "in_use" vs
> "if_active"? If the api increments the usage count we will still decrement
> it later, regardless of active vs in-use, AFAICT.

Indeed.

s/This can lead to unbalanced references./This can lead to unnecessary
references getting hold here and device never getting into the runtime
suspended state./g

> 
> > 
> > Let's use directly the 'if_in_use' function provided by linux/pm_runtime.
> > 
> > Also, already start this new function protected from the runtime
> > recursion, since runtime_pm will need to call for display functions
> > for a proper D3Cold flow.
> > 
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> >   .../gpu/drm/xe/compat-i915-headers/i915_drv.h   |  2 +-
> >   drivers/gpu/drm/xe/xe_pm.c                      | 17 +++++++++++++++++
> >   drivers/gpu/drm/xe/xe_pm.h                      |  1 +
> >   3 files changed, 19 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/xe/compat-i915-headers/i915_drv.h b/drivers/gpu/drm/xe/compat-i915-headers/i915_drv.h
> > index 420eba0e4be0..ad5864d1dd74 100644
> > --- a/drivers/gpu/drm/xe/compat-i915-headers/i915_drv.h
> > +++ b/drivers/gpu/drm/xe/compat-i915-headers/i915_drv.h
> > @@ -177,7 +177,7 @@ static inline intel_wakeref_t intel_runtime_pm_get_if_in_use(struct xe_runtime_p
> >   {
> >   	struct xe_device *xe = container_of(pm, struct xe_device, runtime_pm);
> > -	return xe_pm_runtime_get_if_active(xe);
> > +	return xe_pm_runtime_get_if_in_use(xe);
> >   }
> >   static inline void intel_runtime_pm_put_unchecked(struct xe_runtime_pm *pm)
> > diff --git a/drivers/gpu/drm/xe/xe_pm.c b/drivers/gpu/drm/xe/xe_pm.c
> > index bd35fe9f6227..19f88cb7715b 100644
> > --- a/drivers/gpu/drm/xe/xe_pm.c
> > +++ b/drivers/gpu/drm/xe/xe_pm.c
> > @@ -417,6 +417,23 @@ int xe_pm_runtime_get_if_active(struct xe_device *xe)
> >   	return pm_runtime_get_if_active(xe->drm.dev, true);
> >   }
> > +/**
> > + * xe_pm_runtime_get_if_in_use - Get a runtime_pm reference and resume if needed
> > + * @xe: xe device instance
> > + *
> > + * Returns: True if device is awake and the reference was taken, false otherwise.
> > + */
> > +bool xe_pm_runtime_get_if_in_use(struct xe_device *xe)
> > +{
> > +	if (xe_pm_read_callback_task(xe) == current) {
> > +		/* The device is awake, grab the ref and move on */
> > +		pm_runtime_get_noresume(xe->drm.dev);
> > +		return true;
> > +	}
> > +
> > +	return pm_runtime_get_if_in_use(xe->drm.dev) >= 0;
> 
> This is doing atomic_inc_not_zero() underneath for the "in_use" case AFAICT.
> If the usage count is zero it doesn't increment it and returns 0. Does that
> not lead to an imbalance? Should this rather be > 0?
> 
> > +}
> > +
> >   /**
> >    * xe_pm_assert_unbounded_bridge - Disable PM on unbounded pcie parent bridge
> >    * @xe: xe device instance
> > diff --git a/drivers/gpu/drm/xe/xe_pm.h b/drivers/gpu/drm/xe/xe_pm.h
> > index 64a97c6726a7..9d372cbf388b 100644
> > --- a/drivers/gpu/drm/xe/xe_pm.h
> > +++ b/drivers/gpu/drm/xe/xe_pm.h
> > @@ -28,6 +28,7 @@ int xe_pm_runtime_resume(struct xe_device *xe);
> >   int xe_pm_runtime_get(struct xe_device *xe);
> >   int xe_pm_runtime_put(struct xe_device *xe);
> >   int xe_pm_runtime_get_if_active(struct xe_device *xe);
> > +bool xe_pm_runtime_get_if_in_use(struct xe_device *xe);
> >   void xe_pm_assert_unbounded_bridge(struct xe_device *xe);
> >   int xe_pm_set_vram_threshold(struct xe_device *xe, u32 threshold);
> >   void xe_pm_d3cold_allowed_toggle(struct xe_device *xe);