Re: [PATCH v2 2/2] drm/i915: Check num_pipes before initializing or calling display hooks

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

On Mon, 19 Dec 2016, Wang Elaine <elaine.wang@intel.com> wrote:
> From: Elaine Wang <elaine.wang@intel.com>
>
> when num_pipes is zero, it indicates display doesn't exist, so there
> is no need to initialize display hooks. And to avoid calling these
> uninitialized display hooks, respect num_pipes at the beginning of
> intel_modeset_init_hw and intel_init_clock_gating.
>
> intel_init_pm() calls FBC init function and then initializes
> water mark hooks. Both aren't needed when display doesn't exist. Add
> checking num_pipes at the beginning of intel_init_pm().
>
> v2: Move one check from caller to callee for consistency.
>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Signed-off-by: Elaine Wang <elaine.wang@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c |  6 ++++++
>  drivers/gpu/drm/i915/intel_pm.c      | 10 +++++++++-
>  2 files changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 9cc5dbf..e079ea7 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -16030,6 +16030,9 @@ static void intel_atomic_state_free(struct drm_atomic_state *state)
>   */
>  void intel_init_display_hooks(struct drm_i915_private *dev_priv)
>  {
> +	if (INTEL_INFO(dev_priv)->num_pipes == 0)
> +		return;
> +
>  	if (INTEL_INFO(dev_priv)->gen >= 9) {
>  		dev_priv->display.get_pipe_config = haswell_get_pipe_config;
>  		dev_priv->display.get_initial_plane_config =
> @@ -16412,6 +16415,9 @@ void intel_modeset_init_hw(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  
> +	if (INTEL_INFO(dev_priv)->num_pipes == 0)
> +		return;
> +
>  	intel_update_cdclk(dev_priv);
>  
>  	dev_priv->atomic_cdclk_freq = dev_priv->cdclk_freq;
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index d0834b3..6438ada 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -7619,7 +7619,8 @@ static void i830_init_clock_gating(struct drm_i915_private *dev_priv)
>  
>  void intel_init_clock_gating(struct drm_i915_private *dev_priv)
>  {
> -	dev_priv->display.init_clock_gating(dev_priv);
> +	if (INTEL_INFO(dev_priv)->num_pipes)
> +		dev_priv->display.init_clock_gating(dev_priv);
>  }
>  
>  void intel_suspend_hw(struct drm_i915_private *dev_priv)
> @@ -7644,6 +7645,10 @@ static void nop_init_clock_gating(struct drm_i915_private *dev_priv)
>   */
>  void intel_init_clock_gating_hooks(struct drm_i915_private *dev_priv)
>  {
> +
> +	if (INTEL_INFO(dev_priv)->num_pipes == 0)
> +		return;
> +

Looks like this would be neater by doing

	if (INTEL_INFO(dev_priv)->num_pipes == 0)
		dev_priv->display.init_clock_gating = nop_init_clock_gating;
	else if (IS_SKYLAKE(dev_priv))
		...

and you can leave out the check in intel_init_clock_gating().

We really want to minimize the amount of these checks sprinkled around.

BR,
Jani.



>  		dev_priv->display.init_clock_gating = skylake_init_clock_gating;
>  	else if (IS_KABYLAKE(dev_priv))
> @@ -7685,6 +7690,9 @@ void intel_init_clock_gating_hooks(struct drm_i915_private *dev_priv)
>  /* Set up chip specific power management-related functions */
>  void intel_init_pm(struct drm_i915_private *dev_priv)
>  {
> +	if (INTEL_INFO(dev_priv)->num_pipes == 0)
> +		return;
> +
>  	intel_fbc_init(dev_priv);
>  
>  	/* For cxsr */

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx