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

[Mika Kahola <mika.kahola@intel.com>]

On Wed, 2017-10-04 at 14:26 -0700, Rodrigo Vivi wrote:
> 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.
Ok, fair enough :) to be future proof we can export only general
functions and leave platform specific routines as static ones.

Therefore, you can bash my r-b

Reviewed-by: Mika Kahola <mika.kahola@intel.com>

> 
> > 
> > 
> > > 
> > > 
> > > 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