Re: [PATCH 1/7] drm/i915/display: simplify conditional compilation on runtime PM debug

[Jani Nikula <jani.nikula@intel.com>]

On Thu, 28 Nov 2024, Imre Deak <imre.deak@intel.com> wrote:
> On Thu, Nov 28, 2024 at 02:31:22PM +0200, Imre Deak wrote:
>> On Wed, Nov 27, 2024 at 07:06:02PM +0200, Jani Nikula wrote:
>> > Simplify conditional compilation on CONFIG_DRM_I915_DEBUG_RUNTIME_PM.
>> > Hide it all inside intel_display_power.c.
>> > 
>> > This will unnecessarily pass in the wakeref also when debug is disabled,
>> > but it should not matter a whole lot.
>> 
>> Ftr: there are a lot of callers of these functions and so this change
>> removing the optimization increases the code by >1kB in the non-debug
>> build:
>> 
>> $ size ~/i915-opt.ko  ~/i915-noopt.ko
>>    text	   data	    bss	    dec	    hex	filename
>> 3346869	 325789	   7680	3680338	 382852	/home/imre/i915-opt.ko
>> 3345708	 325773	   7680	3679161	 3823b9	/home/imre/i915-noopt.ko
>
> sorry, confused up the image names, correctly it is:
>
> $ size ~/i915-opt.ko  ~/i915-noopt.ko
>    text	   data	    bss	    dec	    hex	filename
> 3345708	 325773	   7680	3679161	 3823b9	/home/imre/i915-opt.ko
> 3346869	 325789	   7680	3680338	 382852	/home/imre/i915-noopt.ko

Thanks, I dropped this and posted v2. This is really not the important
part of the series, shouldn't have included it in the first place. I'll
look into it again later.

BR,
Jani.


>
>> > Cc: Imre Deak <imre.deak@intel.com>
>> > Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> > ---
>> >  .../drm/i915/display/intel_display_power.c    | 49 +++++++++-------
>> >  .../drm/i915/display/intel_display_power.h    | 56 +++----------------
>> >  2 files changed, 37 insertions(+), 68 deletions(-)
>> > 
>> > diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c b/drivers/gpu/drm/i915/display/intel_display_power.c
>> > index 59dee2dc0552..fe94ef310f6b 100644
>> > --- a/drivers/gpu/drm/i915/display/intel_display_power.c
>> > +++ b/drivers/gpu/drm/i915/display/intel_display_power.c
>> > @@ -706,10 +706,10 @@ intel_display_power_put_async_work(struct work_struct *work)
>> >   * The power down is delayed by @delay_ms if this is >= 0, or by a default
>> >   * 100 ms otherwise.
>> >   */
>> > -void __intel_display_power_put_async(struct drm_i915_private *i915,
>> > -				     enum intel_display_power_domain domain,
>> > -				     intel_wakeref_t wakeref,
>> > -				     int delay_ms)
>> > +static void __intel_display_power_put_async(struct drm_i915_private *i915,
>> > +					    enum intel_display_power_domain domain,
>> > +					    intel_wakeref_t wakeref,
>> > +					    int delay_ms)
>> >  {
>> >  	struct i915_power_domains *power_domains = &i915->display.power.domains;
>> >  	struct intel_runtime_pm *rpm = &i915->runtime_pm;
>> > @@ -750,6 +750,27 @@ void __intel_display_power_put_async(struct drm_i915_private *i915,
>> >  	intel_runtime_pm_put(rpm, wakeref);
>> >  }
>> >  
>> > +void intel_display_power_put_async(struct drm_i915_private *i915,
>> > +				   enum intel_display_power_domain domain,
>> > +				   intel_wakeref_t wakeref)
>> > +{
>> > +	if (!IS_ENABLED(CONFIG_DRM_I915_DEBUG_RUNTIME_PM))
>> > +		wakeref = INTEL_WAKEREF_DEF;
>> > +
>> > +	__intel_display_power_put_async(i915, domain, wakeref, -1);
>> > +}
>> > +
>> > +void intel_display_power_put_async_delay(struct drm_i915_private *i915,
>> > +					 enum intel_display_power_domain domain,
>> > +					 intel_wakeref_t wakeref,
>> > +					 int delay_ms)
>> > +{
>> > +	if (!IS_ENABLED(CONFIG_DRM_I915_DEBUG_RUNTIME_PM))
>> > +		wakeref = INTEL_WAKEREF_DEF;
>> > +
>> > +	__intel_display_power_put_async(i915, domain, wakeref, delay_ms);
>> > +}
>> > +
>> >  /**
>> >   * intel_display_power_flush_work - flushes the async display power disabling work
>> >   * @i915: i915 device instance
>> > @@ -807,7 +828,6 @@ intel_display_power_flush_work_sync(struct drm_i915_private *i915)
>> >  	drm_WARN_ON(&i915->drm, power_domains->async_put_wakeref);
>> >  }
>> >  
>> > -#if IS_ENABLED(CONFIG_DRM_I915_DEBUG_RUNTIME_PM)
>> >  /**
>> >   * intel_display_power_put - release a power domain reference
>> >   * @dev_priv: i915 device instance
>> > @@ -818,6 +838,7 @@ intel_display_power_flush_work_sync(struct drm_i915_private *i915)
>> >   * intel_display_power_get() and might power down the corresponding hardware
>> >   * block right away if this is the last reference.
>> >   */
>> > +#if IS_ENABLED(CONFIG_DRM_I915_DEBUG_RUNTIME_PM)
>> >  void intel_display_power_put(struct drm_i915_private *dev_priv,
>> >  			     enum intel_display_power_domain domain,
>> >  			     intel_wakeref_t wakeref)
>> > @@ -826,21 +847,9 @@ void intel_display_power_put(struct drm_i915_private *dev_priv,
>> >  	intel_runtime_pm_put(&dev_priv->runtime_pm, wakeref);
>> >  }
>> >  #else
>> > -/**
>> > - * intel_display_power_put_unchecked - release an unchecked power domain reference
>> > - * @dev_priv: i915 device instance
>> > - * @domain: power domain to reference
>> > - *
>> > - * This function drops the power domain reference obtained by
>> > - * intel_display_power_get() and might power down the corresponding hardware
>> > - * block right away if this is the last reference.
>> > - *
>> > - * This function is only for the power domain code's internal use to suppress wakeref
>> > - * tracking when the correspondig debug kconfig option is disabled, should not
>> > - * be used otherwise.
>> > - */
>> > -void intel_display_power_put_unchecked(struct drm_i915_private *dev_priv,
>> > -				       enum intel_display_power_domain domain)
>> > +void intel_display_power_put(struct drm_i915_private *dev_priv,
>> > +			     enum intel_display_power_domain domain,
>> > +			     intel_wakeref_t wakeref)
>> >  {
>> >  	__intel_display_power_put(dev_priv, domain);
>> >  	intel_runtime_pm_put_unchecked(&dev_priv->runtime_pm);
>> > diff --git a/drivers/gpu/drm/i915/display/intel_display_power.h b/drivers/gpu/drm/i915/display/intel_display_power.h
>> > index 688f3b60b5c5..c6bd4f122487 100644
>> > --- a/drivers/gpu/drm/i915/display/intel_display_power.h
>> > +++ b/drivers/gpu/drm/i915/display/intel_display_power.h
>> > @@ -190,60 +190,20 @@ intel_wakeref_t intel_display_power_get(struct drm_i915_private *dev_priv,
>> >  intel_wakeref_t
>> >  intel_display_power_get_if_enabled(struct drm_i915_private *dev_priv,
>> >  				   enum intel_display_power_domain domain);
>> > -void __intel_display_power_put_async(struct drm_i915_private *i915,
>> > -				     enum intel_display_power_domain domain,
>> > -				     intel_wakeref_t wakeref,
>> > -				     int delay_ms);
>> >  void intel_display_power_flush_work(struct drm_i915_private *i915);
>> > -#if IS_ENABLED(CONFIG_DRM_I915_DEBUG_RUNTIME_PM)
>> > +
>> >  void intel_display_power_put(struct drm_i915_private *dev_priv,
>> >  			     enum intel_display_power_domain domain,
>> >  			     intel_wakeref_t wakeref);
>> > -static inline void
>> > -intel_display_power_put_async(struct drm_i915_private *i915,
>> > -			      enum intel_display_power_domain domain,
>> > -			      intel_wakeref_t wakeref)
>> > -{
>> > -	__intel_display_power_put_async(i915, domain, wakeref, -1);
>> > -}
>> >  
>> > -static inline void
>> > -intel_display_power_put_async_delay(struct drm_i915_private *i915,
>> > -				    enum intel_display_power_domain domain,
>> > -				    intel_wakeref_t wakeref,
>> > -				    int delay_ms)
>> > -{
>> > -	__intel_display_power_put_async(i915, domain, wakeref, delay_ms);
>> > -}
>> > -#else
>> > -void intel_display_power_put_unchecked(struct drm_i915_private *dev_priv,
>> > -				       enum intel_display_power_domain domain);
>> > +void intel_display_power_put_async(struct drm_i915_private *i915,
>> > +				   enum intel_display_power_domain domain,
>> > +				   intel_wakeref_t wakeref);
>> >  
>> > -static inline void
>> > -intel_display_power_put(struct drm_i915_private *i915,
>> > -			enum intel_display_power_domain domain,
>> > -			intel_wakeref_t wakeref)
>> > -{
>> > -	intel_display_power_put_unchecked(i915, domain);
>> > -}
>> > -
>> > -static inline void
>> > -intel_display_power_put_async(struct drm_i915_private *i915,
>> > -			      enum intel_display_power_domain domain,
>> > -			      intel_wakeref_t wakeref)
>> > -{
>> > -	__intel_display_power_put_async(i915, domain, INTEL_WAKEREF_DEF, -1);
>> > -}
>> > -
>> > -static inline void
>> > -intel_display_power_put_async_delay(struct drm_i915_private *i915,
>> > -				    enum intel_display_power_domain domain,
>> > -				    intel_wakeref_t wakeref,
>> > -				    int delay_ms)
>> > -{
>> > -	__intel_display_power_put_async(i915, domain, INTEL_WAKEREF_DEF, delay_ms);
>> > -}
>> > -#endif
>> > +void intel_display_power_put_async_delay(struct drm_i915_private *i915,
>> > +					 enum intel_display_power_domain domain,
>> > +					 intel_wakeref_t wakeref,
>> > +					 int delay_ms);
>> >  
>> >  void
>> >  intel_display_power_get_in_set(struct drm_i915_private *i915,
>> > -- 
>> > 2.39.5
>> > 

-- 
Jani Nikula, Intel