Re: [PATCH v2 1/4] drm/i915/dp: Assume panel power is off if runtime suspended''

[Imre Deak <imre.deak@intel.com>]

On Thu, Oct 10, 2024 at 12:59:43AM +0300, Cavitt, Jonathan wrote:
> -----Original Message-----
> From: Deak, Imre <imre.deak@intel.com>
> Sent: Wednesday, October 9, 2024 2:26 PM
> To: Cavitt, Jonathan <jonathan.cavitt@intel.com>
> Cc: intel-xe@lists.freedesktop.org; intel-gfx@lists.freedesktop.org
> Subject: Re: [PATCH v2 1/4] drm/i915/dp: Assume panel power is off if runtime suspended
> >
> > On Wed, Oct 09, 2024 at 11:35:56PM +0300, Cavitt, Jonathan wrote:
> > > -----Original Message-----
> > > From: Intel-xe <intel-xe-bounces@lists.freedesktop.org> On Behalf Of Imre Deak
> > > Sent: Wednesday, October 9, 2024 12:44 PM
> > > To: intel-xe@lists.freedesktop.org; intel-gfx@lists.freedesktop.org
> > > Subject: [PATCH v2 1/4] drm/i915/dp: Assume panel power is off if runtime suspended
> > > >
> > > > If the device is runtime suspended the eDP panel power is also off.
> > > > Ignore a short HPD on eDP if the device is suspended accordingly,
> > > > instead of checking the panel power state via the PPS registers for the
> > > > same purpose. The latter involves runtime resuming the device
> > > > unnecessarily, in a frequent scenario where the panel generates a
> > > > spurious short HPD after disabling the panel power and the device is
> > > > runtime suspended.
> > > >
> > > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/display/intel_dp.c                   | 5 ++++-
> > > >  drivers/gpu/drm/i915/intel_runtime_pm.h                   | 8 +++++++-
> > > >  drivers/gpu/drm/xe/compat-i915-headers/intel_runtime_pm.h | 8 ++++++++
> > > >  3 files changed, 19 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> > > > index fbb096be02ade..3eff35dd59b8a 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > > > @@ -85,6 +85,7 @@
> > > >  #include "intel_pch_display.h"
> > > >  #include "intel_pps.h"
> > > >  #include "intel_psr.h"
> > > > +#include "intel_runtime_pm.h"
> > > >  #include "intel_quirks.h"
> > > >  #include "intel_tc.h"
> > > >  #include "intel_vdsc.h"
> > > > @@ -6054,7 +6055,9 @@ intel_dp_hpd_pulse(struct intel_digital_port *dig_port, bool long_hpd)
> > > >       u8 dpcd[DP_RECEIVER_CAP_SIZE];
> > > >
> > > >       if (dig_port->base.type == INTEL_OUTPUT_EDP &&
> > > > -         (long_hpd || !intel_pps_have_panel_power_or_vdd(intel_dp))) {
> > > > +         (long_hpd ||
> > > > +          intel_runtime_pm_suspended(&i915->runtime_pm) ||
> > > > +          !intel_pps_have_panel_power_or_vdd(intel_dp))) {
> > >
> > > From what I'm reading, I'm fairly certain that
> > > "i915->runtime_pm->kdev" is equivalent to "i915->drm.dev".
> > > At least, this seems to be the case according to this comment on
> > > the intel_runtime_pm struct in intel_runtime_pm.h:
> > >
> > > "       struct device *kdev; /* points to i915->drm.dev */"
> > >
> > > So, "intel_runtime_pm_suspended(&i915->runtime_pm)" seems
> > > to be logically equivalent to
> > > "pm_runtime_suspended(i915->drm.dev)", which would mean we
> > > wouldn't need to declare the new helper function
> > > "intel_runtime_pm_suspended" as both want to operate
> > > pm_runtime_suspended on the same relative path for their target
> > > drm device.
> > >
> > > Though, perhaps I'm missing some other reasons we would want
> > > the additional helper function besides,
> >
> > Yes, I was surprised too but drivers/gpu/drm/i915/intel_runtime_pm.h is
> > not included by xe, even when drivers/gpu/drm/i915/display is built for
> > it. IIUC for this and other headers the xe specific ones will be
> > included instead from drivers/gpu/drm/xe/compat-i915-headers. Some of
> > these in turn like i915_irq.h will revert back including the original
> > one from drivers/gpu/drm/i915.
> 
> Sorry, let me clarify.  When I said "perhaps I'm missing some other
> reasons we would want the additional helper function", I was
> referring to intel_runtime_pm_suspended as a whole, not just the
> mirror in compat-i915-headers.
> 
> Basically, my question was why we use intel_runtime_pm_suspended,
> when pm_runtime_suspended, at least at first glance, would also work
> by itself?

I think all use of the driver's runtime PM interface - i.e. all runtime
PM calls outside of intel_runtime_pm.c - should happen via the
intel_runtime_pm struct pointer, which is opaque for the caller.

> -Jonathan Cavitt
> 
> >
> > > so I won't block on this:
> > >
> > > Reviewed-by: Jonathan Cavitt <jonathan.cavitt@intel.com>
> > > -Jonathan Cavitt
> > >
> > > >               /*
> > > >                * vdd off can generate a long/short pulse on eDP which
> > > >                * would require vdd on to handle it, and thus we
> > > > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.h b/drivers/gpu/drm/i915/intel_runtime_pm.h
> > > > index 126f8320f86eb..e22669d61e954 100644
> > > > --- a/drivers/gpu/drm/i915/intel_runtime_pm.h
> > > > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.h
> > > > @@ -96,10 +96,16 @@ intel_rpm_wakelock_count(int wakeref_count)
> > > >       return wakeref_count >> INTEL_RPM_WAKELOCK_SHIFT;
> > > >  }
> > > >
> > > > +static inline bool
> > > > +intel_runtime_pm_suspended(struct intel_runtime_pm *rpm)
> > > > +{
> > > > +     return pm_runtime_suspended(rpm->kdev);
> > > > +}
> > > > +
> > > >  static inline void
> > > >  assert_rpm_device_not_suspended(struct intel_runtime_pm *rpm)
> > > >  {
> > > > -     WARN_ONCE(pm_runtime_suspended(rpm->kdev),
> > > > +     WARN_ONCE(intel_runtime_pm_suspended(rpm),
> > > >                 "Device suspended during HW access\n");
> > > >  }
> > > >
> > > > diff --git a/drivers/gpu/drm/xe/compat-i915-headers/intel_runtime_pm.h b/drivers/gpu/drm/xe/compat-i915-headers/intel_runtime_pm.h
> > > > index cba587ceba1b6..274042bff1bec 100644
> > > > --- a/drivers/gpu/drm/xe/compat-i915-headers/intel_runtime_pm.h
> > > > +++ b/drivers/gpu/drm/xe/compat-i915-headers/intel_runtime_pm.h
> > > > @@ -20,6 +20,14 @@ static inline void enable_rpm_wakeref_asserts(void *rpm)
> > > >  {
> > > >  }
> > > >
> > > > +static inline bool
> > > > +intel_runtime_pm_suspended(struct xe_runtime_pm *pm)
> > > > +{
> > > > +     struct xe_device *xe = container_of(pm, struct xe_device, runtime_pm);
> > > > +
> > > > +     return pm_runtime_suspended(xe->drm.dev);
> > > > +}
> > > > +
> > > >  static inline intel_wakeref_t intel_runtime_pm_get(struct xe_runtime_pm *pm)
> > > >  {
> > > >       struct xe_device *xe = container_of(pm, struct xe_device, runtime_pm);
> > > > --
> > > > 2.44.2
> > > >
> > > >
> >