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

[Wang, Elaine]

> -----Original Message-----
> From: Jani Nikula [mailto:jani.nikula at linux.intel.com]
> Sent: Thursday, December 22, 2016 6:48 PM
> To: Wang, Elaine <elaine.wang at intel.com>; intel-gfx at lists.freedesktop.org;
> Wang, Elaine <elaine.wang at intel.com>
> Cc: Chris Wilson <chris at chris-wilson.co.uk>; Joonas Lahtinen
> <joonas.lahtinen at linux.intel.com>
> Subject: Re: [PATCH v2 2/2] drm/i915: Check num_pipes before initializing or
> calling display hooks
> 
> On Mon, 19 Dec 2016, Wang Elaine <elaine.wang at intel.com> wrote:
> > From: Elaine Wang <elaine.wang at 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 at chris-wilson.co.uk>
> > Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
> > Cc: Jani Nikula <jani.nikula at linux.intel.com>
> > Signed-off-by: Elaine Wang <elaine.wang at 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.
> 
I see. Thanks for the comment. I'll update this patch later.
Thanks,
Elaine
> 
> 
> >  		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