Re: [PATCH 4/7] drm/i915: Move load time display/audio callback init earlier

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

On to, 2016-03-10 at 10:58 +0200, Ander Conselvan De Oliveira wrote:
> On Wed, 2016-03-09 at 17:31 +0200, Imre Deak wrote:
> > All of this is SW only initialization so we can move them earlier.
> > Move
> > the mutex init where the rest of the locks are inited. While at it
> > also
> > convert dev to dev_priv.
> > 
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_dma.c      |  3 ++
> >  drivers/gpu/drm/i915/intel_audio.c   | 12 +++---
> >  drivers/gpu/drm/i915/intel_display.c | 77 ++++++++++++++++------
> > -------------
> > -
> >  drivers/gpu/drm/i915/intel_drv.h     |  3 +-
> >  4 files changed, 45 insertions(+), 50 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_dma.c
> > b/drivers/gpu/drm/i915/i915_dma.c
> > index 9b3ed00..55b0c62 100644
> > --- a/drivers/gpu/drm/i915/i915_dma.c
> > +++ b/drivers/gpu/drm/i915/i915_dma.c
> > @@ -1016,6 +1016,7 @@ int i915_driver_load(struct drm_device *dev,
> > unsigned
> > long flags)
> >  	mutex_init(&dev_priv->modeset_restore_lock);
> >  	mutex_init(&dev_priv->av_mutex);
> >  	mutex_init(&dev_priv->wm.wm_mutex);
> > +	mutex_init(&dev_priv->pps_mutex);
> >  
> >  	ret = i915_workqueues_init(dev_priv);
> >  	if (ret < 0)
> > @@ -1028,6 +1029,8 @@ int i915_driver_load(struct drm_device *dev,
> > unsigned
> > long flags)
> >  	intel_init_dpio(dev_priv);
> >  	intel_power_domains_init(dev_priv);
> >  	intel_irq_init(dev_priv);
> > +	intel_init_display_callbacks(dev_priv);
> > +	intel_init_audio_callbacks(dev_priv);
> >  
> >  	intel_runtime_pm_get(dev_priv);
> >  
> > diff --git a/drivers/gpu/drm/i915/intel_audio.c
> > b/drivers/gpu/drm/i915/intel_audio.c
> > index 30f9214..1936752 100644
> > --- a/drivers/gpu/drm/i915/intel_audio.c
> > +++ b/drivers/gpu/drm/i915/intel_audio.c
> > @@ -567,20 +567,18 @@ void intel_audio_codec_disable(struct
> > intel_encoder
> > *intel_encoder)
> >   * intel_init_audio - Set up chip specific audio functions
> >   * @dev: drm device
> >   */
> > -void intel_init_audio(struct drm_device *dev)
> > +void intel_init_audio_callbacks(struct drm_i915_private *dev_priv)
> >  {
> > -	struct drm_i915_private *dev_priv = dev->dev_private;
> > -
> > -	if (IS_G4X(dev)) {
> > +	if (IS_G4X(dev_priv)) {
> >  		dev_priv->display.audio_codec_enable =
> > g4x_audio_codec_enable;
> >  		dev_priv->display.audio_codec_disable =
> > g4x_audio_codec_disable;
> > -	} else if (IS_VALLEYVIEW(dev) || IS_CHERRYVIEW(dev)) {
> > +	} else if (IS_VALLEYVIEW(dev_priv) ||
> > IS_CHERRYVIEW(dev_priv)) {
> >  		dev_priv->display.audio_codec_enable =
> > ilk_audio_codec_enable;
> >  		dev_priv->display.audio_codec_disable =
> > ilk_audio_codec_disable;
> > -	} else if (IS_HASWELL(dev) || INTEL_INFO(dev)->gen >= 8) {
> > +	} else if (IS_HASWELL(dev_priv) || INTEL_INFO(dev_priv)-
> > >gen >= 8) {
> >  		dev_priv->display.audio_codec_enable =
> > hsw_audio_codec_enable;
> >  		dev_priv->display.audio_codec_disable =
> > hsw_audio_codec_disable;
> > -	} else if (HAS_PCH_SPLIT(dev)) {
> > +	} else if (HAS_PCH_SPLIT(dev_priv)) {
> >  		dev_priv->display.audio_codec_enable =
> > ilk_audio_codec_enable;
> >  		dev_priv->display.audio_codec_disable =
> > ilk_audio_codec_disable;
> >  	}
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index 62d36a7..5170718 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -15095,22 +15095,20 @@ static const struct drm_mode_config_funcs
> > intel_mode_funcs = {
> >  };
> >  
> >  /* Set up chip specific display functions */
> > -static void intel_init_display(struct drm_device *dev)
> > +void intel_init_display_callbacks(struct drm_i915_private
> > *dev_priv)
> >  {
> > -	struct drm_i915_private *dev_priv = dev->dev_private;
> > -
> > -	if (HAS_PCH_SPLIT(dev) || IS_G4X(dev))
> > +	if (HAS_PCH_SPLIT(dev_priv) || IS_G4X(dev_priv))
> >  		dev_priv->display.find_dpll = g4x_find_best_dpll;
> > -	else if (IS_CHERRYVIEW(dev))
> > +	else if (IS_CHERRYVIEW(dev_priv))
> >  		dev_priv->display.find_dpll = chv_find_best_dpll;
> > -	else if (IS_VALLEYVIEW(dev))
> > +	else if (IS_VALLEYVIEW(dev_priv))
> >  		dev_priv->display.find_dpll = vlv_find_best_dpll;
> > -	else if (IS_PINEVIEW(dev))
> > +	else if (IS_PINEVIEW(dev_priv))
> >  		dev_priv->display.find_dpll = pnv_find_best_dpll;
> >  	else
> >  		dev_priv->display.find_dpll = i9xx_find_best_dpll;
> >  
> > -	if (INTEL_INFO(dev)->gen >= 9) {
> > +	if (INTEL_INFO(dev_priv)->gen >= 9) {
> >  		dev_priv->display.get_pipe_config =
> > haswell_get_pipe_config;
> >  		dev_priv->display.get_initial_plane_config =
> >  			skylake_get_initial_plane_config;
> > @@ -15118,7 +15116,7 @@ static void intel_init_display(struct
> > drm_device *dev)
> >  			haswell_crtc_compute_clock;
> >  		dev_priv->display.crtc_enable =
> > haswell_crtc_enable;
> >  		dev_priv->display.crtc_disable =
> > haswell_crtc_disable;
> > -	} else if (HAS_DDI(dev)) {
> > +	} else if (HAS_DDI(dev_priv)) {
> >  		dev_priv->display.get_pipe_config =
> > haswell_get_pipe_config;
> >  		dev_priv->display.get_initial_plane_config =
> >  			ironlake_get_initial_plane_config;
> > @@ -15126,7 +15124,7 @@ static void intel_init_display(struct
> > drm_device *dev)
> >  			haswell_crtc_compute_clock;
> >  		dev_priv->display.crtc_enable =
> > haswell_crtc_enable;
> >  		dev_priv->display.crtc_disable =
> > haswell_crtc_disable;
> > -	} else if (HAS_PCH_SPLIT(dev)) {
> > +	} else if (HAS_PCH_SPLIT(dev_priv)) {
> >  		dev_priv->display.get_pipe_config =
> > ironlake_get_pipe_config;
> >  		dev_priv->display.get_initial_plane_config =
> >  			ironlake_get_initial_plane_config;
> > @@ -15134,7 +15132,7 @@ static void intel_init_display(struct
> > drm_device *dev)
> >  			ironlake_crtc_compute_clock;
> >  		dev_priv->display.crtc_enable =
> > ironlake_crtc_enable;
> >  		dev_priv->display.crtc_disable =
> > ironlake_crtc_disable;
> > -	} else if (IS_VALLEYVIEW(dev) || IS_CHERRYVIEW(dev)) {
> > +	} else if (IS_VALLEYVIEW(dev_priv) ||
> > IS_CHERRYVIEW(dev_priv)) {
> >  		dev_priv->display.get_pipe_config =
> > i9xx_get_pipe_config;
> >  		dev_priv->display.get_initial_plane_config =
> >  			i9xx_get_initial_plane_config;
> > @@ -15151,89 +15149,89 @@ static void intel_init_display(struct
> > drm_device
> > *dev)
> >  	}
> >  
> >  	/* Returns the core display clock speed */
> > -	if (IS_SKYLAKE(dev) || IS_KABYLAKE(dev))
> > +	if (IS_SKYLAKE(dev_priv) || IS_KABYLAKE(dev_priv))
> >  		dev_priv->display.get_display_clock_speed =
> >  			skylake_get_display_clock_speed;
> > -	else if (IS_BROXTON(dev))
> > +	else if (IS_BROXTON(dev_priv))
> >  		dev_priv->display.get_display_clock_speed =
> >  			broxton_get_display_clock_speed;
> > -	else if (IS_BROADWELL(dev))
> > +	else if (IS_BROADWELL(dev_priv))
> >  		dev_priv->display.get_display_clock_speed =
> >  			broadwell_get_display_clock_speed;
> > -	else if (IS_HASWELL(dev))
> > +	else if (IS_HASWELL(dev_priv))
> >  		dev_priv->display.get_display_clock_speed =
> >  			haswell_get_display_clock_speed;
> > -	else if (IS_VALLEYVIEW(dev) || IS_CHERRYVIEW(dev))
> > +	else if (IS_VALLEYVIEW(dev_priv) ||
> > IS_CHERRYVIEW(dev_priv))
> >  		dev_priv->display.get_display_clock_speed =
> >  			valleyview_get_display_clock_speed;
> > -	else if (IS_GEN5(dev))
> > +	else if (IS_GEN5(dev_priv))
> >  		dev_priv->display.get_display_clock_speed =
> >  			ilk_get_display_clock_speed;
> > -	else if (IS_I945G(dev) || IS_BROADWATER(dev) ||
> > -		 IS_GEN6(dev) || IS_IVYBRIDGE(dev))
> > +	else if (IS_I945G(dev_priv) || IS_BROADWATER(dev_priv) ||
> > +		 IS_GEN6(dev_priv) || IS_IVYBRIDGE(dev_priv))
> >  		dev_priv->display.get_display_clock_speed =
> >  			i945_get_display_clock_speed;
> > -	else if (IS_GM45(dev))
> > +	else if (IS_GM45(dev_priv))
> >  		dev_priv->display.get_display_clock_speed =
> >  			gm45_get_display_clock_speed;
> > -	else if (IS_CRESTLINE(dev))
> > +	else if (IS_CRESTLINE(dev_priv))
> >  		dev_priv->display.get_display_clock_speed =
> >  			i965gm_get_display_clock_speed;
> > -	else if (IS_PINEVIEW(dev))
> > +	else if (IS_PINEVIEW(dev_priv))
> >  		dev_priv->display.get_display_clock_speed =
> >  			pnv_get_display_clock_speed;
> > -	else if (IS_G33(dev) || IS_G4X(dev))
> > +	else if (IS_G33(dev_priv) || IS_G4X(dev_priv))
> >  		dev_priv->display.get_display_clock_speed =
> >  			g33_get_display_clock_speed;
> > -	else if (IS_I915G(dev))
> > +	else if (IS_I915G(dev_priv))
> >  		dev_priv->display.get_display_clock_speed =
> >  			i915_get_display_clock_speed;
> > -	else if (IS_I945GM(dev) || IS_845G(dev))
> > +	else if (IS_I945GM(dev_priv) || IS_845G(dev_priv))
> >  		dev_priv->display.get_display_clock_speed =
> >  			i9xx_misc_get_display_clock_speed;
> > -	else if (IS_I915GM(dev))
> > +	else if (IS_I915GM(dev_priv))
> >  		dev_priv->display.get_display_clock_speed =
> >  			i915gm_get_display_clock_speed;
> > -	else if (IS_I865G(dev))
> > +	else if (IS_I865G(dev_priv))
> >  		dev_priv->display.get_display_clock_speed =
> >  			i865_get_display_clock_speed;
> > -	else if (IS_I85X(dev))
> > +	else if (IS_I85X(dev_priv))
> >  		dev_priv->display.get_display_clock_speed =
> >  			i85x_get_display_clock_speed;
> >  	else { /* 830 */
> > -		WARN(!IS_I830(dev), "Unknown platform. Assuming
> > 133 MHz
> > CDCLK\n");
> > +		WARN(!IS_I830(dev_priv), "Unknown platform.
> > Assuming 133 MHz
> > CDCLK\n");
> >  		dev_priv->display.get_display_clock_speed =
> >  			i830_get_display_clock_speed;
> >  	}
> >  
> > -	if (IS_GEN5(dev)) {
> > +	if (IS_GEN5(dev_priv)) {
> >  		dev_priv->display.fdi_link_train =
> > ironlake_fdi_link_train;
> > -	} else if (IS_GEN6(dev)) {
> > +	} else if (IS_GEN6(dev_priv)) {
> >  		dev_priv->display.fdi_link_train =
> > gen6_fdi_link_train;
> > -	} else if (IS_IVYBRIDGE(dev)) {
> > +	} else if (IS_IVYBRIDGE(dev_priv)) {
> >  		/* FIXME: detect B0+ stepping and use auto
> > training */
> >  		dev_priv->display.fdi_link_train =
> > ivb_manual_fdi_link_train;
> > -	} else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
> > +	} else if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
> > {
> >  		dev_priv->display.fdi_link_train =
> > hsw_fdi_link_train;
> > -		if (IS_BROADWELL(dev)) {
> > +		if (IS_BROADWELL(dev_priv)) {
> >  			dev_priv->display.modeset_commit_cdclk =
> >  				broadwell_modeset_commit_cdclk;
> >  			dev_priv->display.modeset_calc_cdclk =
> >  				broadwell_modeset_calc_cdclk;
> >  		}
> > -	} else if (IS_VALLEYVIEW(dev) || IS_CHERRYVIEW(dev)) {
> > +	} else if (IS_VALLEYVIEW(dev_priv) ||
> > IS_CHERRYVIEW(dev_priv)) {
> >  		dev_priv->display.modeset_commit_cdclk =
> >  			valleyview_modeset_commit_cdclk;
> >  		dev_priv->display.modeset_calc_cdclk =
> >  			valleyview_modeset_calc_cdclk;
> > -	} else if (IS_BROXTON(dev)) {
> > +	} else if (IS_BROXTON(dev_priv)) {
> >  		dev_priv->display.modeset_commit_cdclk =
> >  			broxton_modeset_commit_cdclk;
> >  		dev_priv->display.modeset_calc_cdclk =
> >  			broxton_modeset_calc_cdclk;
> >  	}
> 
> Would it make sense to change these if ladders into a structs of function
> pointers and point to those from struct intel_device_info? The HAS_PCH_SPLIT()
> check could be a bit tricky, but those usually go as
> 
>   if (HAS_DDI())
> 	...
>   else if (HAS_PCH_SPLIT())
> 	...,
> 
> so they usually mean ILK, SNB and IVB.

Yes, a good idea, would clarify things.

> I'm not saying this should be part of this series, but just wanted to throw the
> idea out there.

I'm not sure yet how many variations of new function structs we need to
spawn depending on platform differences that you also mention (AFAICS
there are similar ones for older platforms too). We would also need to
remove first the conditional assignments to callbacks (see the next
patch). But I think after having all callback initializations collected
to one place, we'd have a better overview and could go for what you
suggested.

--Imre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx