Re: [PATCH 4/6] drm/i915/opregion: abstract ASLE presence check

[Radhakrishna Sripada <radhakrishna.sripada@intel.com>]

On Fri, Jan 12, 2024 at 12:17:25PM +0200, Jani Nikula wrote:
> On Thu, 11 Jan 2024, Radhakrishna Sripada <radhakrishna.sripada@intel.com> wrote:
> > On Thu, Jan 11, 2024 at 07:21:17PM +0200, Jani Nikula wrote:
> >> Add a function to check the opregion ASLE presence instead of accessing
> >> the opregion structures directly.
> >> 
> >> Reorder the checks in i915_has_asle() to avoid the function call if
> >> possible.
> >> 
> >> Cc: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
> >> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/display/intel_display_irq.c | 6 +++---
> >>  drivers/gpu/drm/i915/display/intel_opregion.c    | 5 +++++
> >>  drivers/gpu/drm/i915/display/intel_opregion.h    | 6 ++++++
> >>  3 files changed, 14 insertions(+), 3 deletions(-)
> >> 
> >> diff --git a/drivers/gpu/drm/i915/display/intel_display_irq.c b/drivers/gpu/drm/i915/display/intel_display_irq.c
> >> index 99843883cef7..f846c5b108b5 100644
> >> --- a/drivers/gpu/drm/i915/display/intel_display_irq.c
> >> +++ b/drivers/gpu/drm/i915/display/intel_display_irq.c
> >> @@ -266,12 +266,12 @@ void i915_disable_pipestat(struct drm_i915_private *dev_priv,
> >>  	intel_uncore_posting_read(&dev_priv->uncore, reg);
> >>  }
> >>  
> >> -static bool i915_has_asle(struct drm_i915_private *dev_priv)
> >> +static bool i915_has_asle(struct drm_i915_private *i915)
> > Why not move this to intel_opregion.c and export it instead of
> > intel_opregion_asle_present ?
> 
> I'm trying to be conscious of the possible performance impact of making
> calls from the irq code just to find there's nothing to do.
Makes sense.

> 
> >>  {
> >> -	if (!dev_priv->display.opregion.asle)
> >> +	if (!IS_PINEVIEW(i915) && !IS_MOBILE(i915))
> > Can we extend this check to dgfx as well?
> 
> Extend how? This will return early for everything after IVB.
The name of the function is bit misleading as looking at Opregion code
and the spec beyond IVB, asle aka Mailbox 3 is present, just that it is
not used for reading pipestat. It is used to store rvda from where VBT is read.
Extension is not required for this purpose. Might want to clear that unless
I misunderstood the purpose, either way 

Reviewed-by: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
> 
> BR,
> Jani.
> 
> >
> > -Radhakrishna(RK) Sripada
> >
> >>  		return false;
> >>  
> >> -	return IS_PINEVIEW(dev_priv) || IS_MOBILE(dev_priv);
> >> +	return intel_opregion_asle_present(i915);
> >>  }
> >>  
> >>  /**
> >> diff --git a/drivers/gpu/drm/i915/display/intel_opregion.c b/drivers/gpu/drm/i915/display/intel_opregion.c
> >> index 8b9e820971cb..26aacb01f9ec 100644
> >> --- a/drivers/gpu/drm/i915/display/intel_opregion.c
> >> +++ b/drivers/gpu/drm/i915/display/intel_opregion.c
> >> @@ -632,6 +632,11 @@ static void asle_work(struct work_struct *work)
> >>  	asle->aslc = aslc_stat;
> >>  }
> >>  
> >> +bool intel_opregion_asle_present(struct drm_i915_private *i915)
> >> +{
> >> +	return i915->display.opregion.asle;
> >> +}
> >> +
> >>  void intel_opregion_asle_intr(struct drm_i915_private *dev_priv)
> >>  {
> >>  	if (dev_priv->display.opregion.asle)
> >> diff --git a/drivers/gpu/drm/i915/display/intel_opregion.h b/drivers/gpu/drm/i915/display/intel_opregion.h
> >> index 9efadfb72584..d084b30e8703 100644
> >> --- a/drivers/gpu/drm/i915/display/intel_opregion.h
> >> +++ b/drivers/gpu/drm/i915/display/intel_opregion.h
> >> @@ -69,6 +69,7 @@ void intel_opregion_resume(struct drm_i915_private *dev_priv);
> >>  void intel_opregion_suspend(struct drm_i915_private *dev_priv,
> >>  			    pci_power_t state);
> >>  
> >> +bool intel_opregion_asle_present(struct drm_i915_private *i915);
> >>  void intel_opregion_asle_intr(struct drm_i915_private *dev_priv);
> >>  int intel_opregion_notify_encoder(struct intel_encoder *intel_encoder,
> >>  				  bool enable);
> >> @@ -111,6 +112,11 @@ static inline void intel_opregion_suspend(struct drm_i915_private *dev_priv,
> >>  {
> >>  }
> >>  
> >> +static inline bool intel_opregion_asle_present(struct drm_i915_private *i915)
> >> +{
> >> +	return false;
> >> +}
> >> +
> >>  static inline void intel_opregion_asle_intr(struct drm_i915_private *dev_priv)
> >>  {
> >>  }
> >> -- 
> >> 2.39.2
> >> 
> 
> -- 
> Jani Nikula, Intel