Re: [PATCH V4] drm/i915/skl: SKL CDCLK change on modeset tracking VCO

From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: Marc Herbert <Marc.Herbert@intel.com>
Cc: Intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH V4] drm/i915/skl: SKL CDCLK change on modeset tracking VCO
Date: Fri, 12 Feb 2016 13:25:31 +0200	[thread overview]
Message-ID: <20160212112531.GI23290@intel.com> (raw)
In-Reply-To: <56BD3158.3070407@intel.com>

On Thu, Feb 11, 2016 at 05:11:52PM -0800, Marc Herbert wrote:
> [I'm cheating and doing this code review with the author watching over my shoulder]
> 
> On 11/02/16 15:22, clinton.a.taylor@intel.com wrote:
> > From: Clint Taylor <clinton.a.taylor@intel.com>
> > 
> > Track VCO frequency of SKL instead of the boot CDCLK and allow modeset
> > to set cdclk based on the max required pixel clock based on VCO
> > selected.
> 
> Nit: the main point shouldn't come second.
> 
> > The vco should be tracked at the atomic level and all CRTCs updated if
> > the required vco is changed. At this time the eDP pll is configured
> > inside the encoder which has no visibility into the atomic state.
> 
> should be -> is
> 
> 
> > When eDP v1.4 panel that require the 8640 vco are available this may need
> > to be investigated.
> 
> Just say that 8640 can't be tested yet.
> 
> > V1: initial version
> > V2: add vco tracking in intel_dp_compute_config(), rename
> > skl_boot_cdclk.
> > V3: rebase, V2 feedback not possible as encoders are not aware of
> > atomic.
> > V4: track target vco is atomic state. modeset all CRTCs if vco changes
> > 
> > Signed-off-by: Clint Taylor <clinton.a.taylor@intel.com>
> > Cc: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h      |    2 +-
> >  drivers/gpu/drm/i915/intel_ddi.c     |    2 +-
> >  drivers/gpu/drm/i915/intel_display.c |   97 +++++++++++++++++++++++++++++-----
> >  drivers/gpu/drm/i915/intel_dp.c      |   10 ++--
> >  drivers/gpu/drm/i915/intel_drv.h     |    4 ++
> >  5 files changed, 97 insertions(+), 18 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 8216665..f65dd1a 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1822,7 +1822,7 @@ struct drm_i915_private {
> >  	int num_fence_regs; /* 8 on pre-965, 16 otherwise */
> >  
> >  	unsigned int fsb_freq, mem_freq, is_ddr3;
> > -	unsigned int skl_boot_cdclk;
> > +	unsigned int skl_vco_freq;
> >  	unsigned int cdclk_freq, max_cdclk_freq, atomic_cdclk_freq;
> >  	unsigned int max_dotclk_freq;
> >  	unsigned int hpll_freq;
> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> > index 6d5b09f..285adab 100644
> > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > @@ -2958,7 +2958,7 @@ void intel_ddi_pll_init(struct drm_device *dev)
> >  		int cdclk_freq;
> >  
> >  		cdclk_freq = dev_priv->display.get_display_clock_speed(dev);
> > -		dev_priv->skl_boot_cdclk = cdclk_freq;
> > +		dev_priv->skl_vco_freq = skl_cdclk_get_vco(cdclk_freq);
> 
> - skl_cdclk_get_vco()      and skl_cdclk_frequencies[] should probably be renamed to:
> + skl_get_bios_cdclk_vco() and skl_bios_cdclk_frequencies[]
> 
> to avoid confusion with the (different) mapping used in the new skl_modeset_calc_cdclk()
> function below.

Let's not. This stuff doesn't really have anything to do with the BIOS.
We just want to read out the current hardware state, nothing more.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx