Re: [PATCH 04/13] drm/i915: Unify and export gen9+ port_clock calculation.

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Wed, Oct 04, 2017 at 07:38:16PM +0000, Rodrigo Vivi wrote:
> On Wed, Oct 04, 2017 at 06:39:19AM +0000, Mika Kahola wrote:
> > On Tue, 2017-10-03 at 00:06 -0700, Rodrigo Vivi wrote:
> > > On Cannonlake the DVFS level selection depends on the
> > > port clock.
> > > 
> > > So let's re-org in a way that we can easily export without
> > > duplicating any code.
> > > 
> > > v2: Rebased on changes on previous patches
> > > 
> > > Cc: Mika Kahola <mika.kahola@intel.com>
> > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_ddi.c | 21 ++++++++++++++++++---
> > >  drivers/gpu/drm/i915/intel_drv.h |  3 ++-
> > >  2 files changed, 20 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c
> > > b/drivers/gpu/drm/i915/intel_ddi.c
> > > index 92eabb6cc1ab..ee64b1a50453 100644
> > > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > > @@ -1297,7 +1297,7 @@ static void cnl_ddi_clock_get(struct
> > > intel_encoder *encoder,
> > >  
> > >  	pll_id = intel_get_shared_dpll_id(dev_priv, pipe_config-
> > > >shared_dpll);
> > >  
> > > -	pipe_config->port_clock = cnl_calc_pll_link(dev_priv,
> > > pll_id);
> > > +	pipe_config->port_clock = intel_ddi_port_clock(dev_priv,
> > > pll_id);
> > >  
> > >  	ddi_dotclock_get(pipe_config);
> > >  }
> > > @@ -1353,7 +1353,7 @@ static void skl_ddi_clock_get(struct
> > > intel_encoder *encoder,
> > >  
> > >  	pll_id = intel_get_shared_dpll_id(dev_priv, pipe_config-
> > > >shared_dpll);
> > >  
> > > -	pipe_config->port_clock = skl_calc_pll_link(dev_priv,
> > > pll_id);
> > > +	pipe_config->port_clock = intel_ddi_port_clock(dev_priv,
> > > pll_id);
> > >  
> > >  	ddi_dotclock_get(pipe_config);
> > >  }
> > > @@ -1437,11 +1437,26 @@ static void bxt_ddi_clock_get(struct
> > > intel_encoder *encoder,
> > >  	enum port port = intel_ddi_get_encoder_port(encoder);
> > >  	enum intel_dpll_id pll_id = port;
> > >  
> > > -	pipe_config->port_clock = bxt_calc_pll_link(dev_priv,
> > > pll_id);
> > > +	pipe_config->port_clock = intel_ddi_port_clock(dev_priv,
> > > pll_id);
> > >  
> > >  	ddi_dotclock_get(pipe_config);
> > >  }
> > >  
> > > +int intel_ddi_port_clock(struct drm_i915_private *dev_priv,
> > > +			 enum intel_dpll_id pll_id)
> > > +{
> > > +	if (IS_GEN9_BC(dev_priv)) {
> > > +		return skl_calc_pll_link(dev_priv, pll_id);
> > > +	} else if (IS_GEN9_LP(dev_priv)) {
> > > +		return bxt_calc_pll_link(dev_priv, pll_id);
> > > +	} else if (IS_CANNONLAKE(dev_priv)) {
> > > +		return cnl_calc_pll_link(dev_priv, pll_id);
> > > +	} else {
> > > +		MISSING_CASE(INTEL_GEN(dev_priv));
> > > +		return 0;
> > > +	}
> > > +}
> > Personally, I feel that we wouldn't need to unify this. We call
> > platform specific functions from this intel_ddi_port_clock() and this
> > is called from platform specific *_ddi_clock_get() routine. Why not
> > just keep all platform specific functions in one place?
> 
> Yeap, it is indeed silly to unify since we are just calling it
> from the specific functions.
> 
> Let's drop this patch ;)

oh no! wait! I was rebasing here and I'm not sure I want to drop this patch.

my OCD prefers to export general functions and leave platform specific
functions as static instead of exporting all specific platforms one.
Apparently in the future we will always need this port clock for dvfs
functions called from both places cdclk and dpll so every platform
exporting a specific function won't be ideal.

so imo it would be good if we could organize this one starting here
already.

> 
> > 
> > Paolo, any opinions on this one?
> > 
> > > +
> > >  void intel_ddi_clock_get(struct intel_encoder *encoder,
> > >  			 struct intel_crtc_state *pipe_config)
> > >  {
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > b/drivers/gpu/drm/i915/intel_drv.h
> > > index 0cab667fff57..fe4650d6db03 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -1293,7 +1293,8 @@ bool intel_ddi_is_audio_enabled(struct
> > > drm_i915_private *dev_priv,
> > >  				 struct intel_crtc *intel_crtc);
> > >  void intel_ddi_get_config(struct intel_encoder *encoder,
> > >  			  struct intel_crtc_state *pipe_config);
> > > -
> > > +int intel_ddi_port_clock(struct drm_i915_private *dev_priv,
> > > +			 enum intel_dpll_id pll_id);
> > >  void intel_ddi_clock_get(struct intel_encoder *encoder,
> > >  			 struct intel_crtc_state *pipe_config);
> > >  void intel_ddi_set_vc_payload_alloc(const struct intel_crtc_state
> > > *crtc_state,
> > -- 
> > Mika Kahola - Intel OTC
> > 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx