[Intel-gfx] [PATCH V5] drm/i915/skl: SKL CDCLK change on modeset tracking VCO
Thulasimani, Sivakumar
sivakumar.thulasimani at intel.com
Tue Feb 16 02:44:14 UTC 2016
On 2/15/2016 6:46 PM, Ville Syrjälä wrote:
> On Fri, Feb 12, 2016 at 06:06:10PM -0800, clinton.a.taylor at intel.com wrote:
>> From: Clint Taylor <clinton.a.taylor at intel.com>
>>
>> Set cdclk based on the max required pixel clock based on VCO
>> selected. Track boot vco instead of boot cdclk.
>>
>> The vco is now tracked at the atomic level and all CRTCs updated if
>> the required vco is changed. Not tested with eDP v1.4 panels that
>> require 8640 vco due to availability.
>>
>> 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
>> V5: rename atomic variable, cleaner if/else logic, use existing vco if
>> encoder does not return a new vco value. check_patch.pl cleanup
>>
>> Signed-off-by: Clint Taylor <clinton.a.taylor at intel.com>
>> Cc: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= <ville.syrjala at 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 | 114 +++++++++++++++++++++++++++++-----
>> drivers/gpu/drm/i915/intel_dp.c | 6 +-
>> drivers/gpu/drm/i915/intel_drv.h | 4 ++
>> 5 files changed, 111 insertions(+), 17 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);
>> if (skl_sanitize_cdclk(dev_priv))
>> DRM_DEBUG_KMS("Sanitized cdclk programmed by pre-os\n");
>> if (!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_ENABLE))
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 9e2273b..c283abd 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -5663,7 +5663,7 @@ static unsigned int skl_cdclk_decimal(unsigned int freq)
>> return (freq - 1000) / 500;
>> }
>>
>> -static unsigned int skl_cdclk_get_vco(unsigned int freq)
>> +unsigned int skl_cdclk_get_vco(unsigned int freq)
>> {
>> unsigned int i;
>>
>> @@ -5821,17 +5821,21 @@ void skl_uninit_cdclk(struct drm_i915_private *dev_priv)
>>
>> void skl_init_cdclk(struct drm_i915_private *dev_priv)
>> {
>> - unsigned int required_vco;
>> + unsigned int cdclk;
>>
>> /* DPLL0 not enabled (happens on early BIOS versions) */
>> if (!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_ENABLE)) {
>> /* enable DPLL0 */
>> - required_vco = skl_cdclk_get_vco(dev_priv->skl_boot_cdclk);
>> - skl_dpll0_enable(dev_priv, required_vco);
>> + if (dev_priv->skl_vco_freq != 8640)
>> + dev_priv->skl_vco_freq = 8100;
>> + skl_dpll0_enable(dev_priv, dev_priv->skl_vco_freq);
>> + cdclk = ((dev_priv->skl_vco_freq == 8100) ? 337500 : 308570);
>> + } else {
>> + cdclk = dev_priv->cdclk_freq;
>> }
>>
>> - /* set CDCLK to the frequency the BIOS chose */
>> - skl_set_cdclk(dev_priv, dev_priv->skl_boot_cdclk);
>> + /* set CDCLK to the lowest frequency, Modeset follows */
>> + skl_set_cdclk(dev_priv, cdclk);
>>
>> /* enable DBUF power */
>> I915_WRITE(DBUF_CTL, I915_READ(DBUF_CTL) | DBUF_POWER_REQUEST);
>> @@ -5847,7 +5851,7 @@ int skl_sanitize_cdclk(struct drm_i915_private *dev_priv)
>> {
>> uint32_t lcpll1 = I915_READ(LCPLL1_CTL);
>> uint32_t cdctl = I915_READ(CDCLK_CTL);
>> - int freq = dev_priv->skl_boot_cdclk;
>> + int freq = dev_priv->cdclk_freq;
>>
>> /*
>> * check if the pre-os intialized the display
>> @@ -5871,11 +5875,7 @@ int skl_sanitize_cdclk(struct drm_i915_private *dev_priv)
>> /* All well; nothing to sanitize */
>> return false;
>> sanitize:
>> - /*
>> - * As of now initialize with max cdclk till
>> - * we get dynamic cdclk support
>> - * */
>> - dev_priv->skl_boot_cdclk = dev_priv->max_cdclk_freq;
>> +
>> skl_init_cdclk(dev_priv);
>>
>> /* we did have to sanitize */
>> @@ -9845,6 +9845,70 @@ static void broadwell_modeset_commit_cdclk(struct drm_atomic_state *old_state)
>> broadwell_set_cdclk(dev, req_cdclk);
>> }
>>
>> +static int skl_modeset_calc_cdclk(struct drm_atomic_state *state)
>> +{
>> + struct drm_i915_private *dev_priv = to_i915(state->dev);
>> + const int max_pixclk = ilk_max_pixel_rate(state);
>> + int cdclk;
>> +
>> + /*
>> + * FIXME should also account for plane ratio
>> + * once 64bpp pixel formats are supported.
>> + */
>> +
>> + if (to_intel_atomic_state(state)->cdclk_pll_vco == 8640) {
not necessarily a comment for this patch (since bugs are waiting on this
:) )
if we can take advantage of VCO programming for non edp scenarios
we can program lower CD Clocks and save more power. i.e assume you
have MIPI with pixel clock less than 300MHz and full HD HDMI. both
can be driven by 308MHz but since we have tied our cd clock
programming to VCO we might end up with 337.5MHz.
to rephrase it, CD clock is controlled by two variables vco &
cd clock programmed, we control only one today in the
cd clock calc and commit path which is not
optimal.
regards,
Sivakumar
>> + /* vco 8640 */
>> + if (max_pixclk > 540000)
>> + cdclk = 617140;
>> + else if (max_pixclk > 432000)
>> + cdclk = 540000;
>> + else if (max_pixclk > 308570)
>> + cdclk = 432000;
>> + else
>> + cdclk = 308570;
>> + } else {
>> + /* VCO 8100 */
>> + if (max_pixclk > 540000)
>> + cdclk = 675000;
>> + else if (max_pixclk > 450000)
>> + cdclk = 540000;
>> + else if (max_pixclk > 337500)
>> + cdclk = 450000;
>> + else
>> + cdclk = 337500;
>> + }
>> +
>> + /*
>> + * FIXME move the cdclk caclulation to
>> + * compute_config() so we can fail gracegully.
>> + */
>> + if (cdclk > dev_priv->max_cdclk_freq) {
>> + DRM_ERROR("requested cdclk (%d kHz) exceeds max (%d kHz)\n",
>> + cdclk, dev_priv->max_cdclk_freq);
>> + cdclk = dev_priv->max_cdclk_freq;
>> + }
>> +
>> + to_intel_atomic_state(state)->cdclk = cdclk;
>> +
>> + return 0;
>> +}
>> +
>> +static void skl_modeset_commit_cdclk(struct drm_atomic_state *old_state)
>> +{
>> + struct drm_device *dev = old_state->dev;
>> + struct drm_i915_private *dev_priv = dev->dev_private;
>> + unsigned int req_cdclk = to_intel_atomic_state(old_state)->cdclk;
>> +
>> + /*
>> + * FIXME disable/enable PLL should wrap set_cdclk()
>> + */
>> + skl_set_cdclk(dev_priv, req_cdclk);
>> +
>> + if (to_intel_atomic_state(old_state)->cdclk_pll_vco) {
>> + dev_priv->skl_vco_freq = to_intel_atomic_state(old_state)->cdclk_pll_vco;
> The if statement seems pointless. We should never get here without a
> valid vco in the state. Or if we do, there is a bug somewhere else.
>
>> + }
>> +}
>> +
>> static int haswell_crtc_compute_clock(struct intel_crtc *crtc,
>> struct intel_crtc_state *crtc_state)
>> {
>> @@ -13219,11 +13283,14 @@ static int intel_modeset_all_pipes(struct drm_atomic_state *state)
>>
>> static int intel_modeset_checks(struct drm_atomic_state *state)
>> {
>> + struct drm_device *dev = state->dev;
>> struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
>> struct drm_i915_private *dev_priv = state->dev->dev_private;
>> struct drm_crtc *crtc;
>> struct drm_crtc_state *crtc_state;
>> int ret = 0, i;
>> + unsigned int target_vco = 0;
>> + bool cdclk_change, vco_change;
>>
>> if (!check_digital_port_conflicts(state)) {
>> DRM_DEBUG_KMS("rejecting conflicting digital port configuration\n");
>> @@ -13248,9 +13315,22 @@ static int intel_modeset_checks(struct drm_atomic_state *state)
>> * adjusted_mode bits in the crtc directly.
>> */
>> if (dev_priv->display.modeset_calc_cdclk) {
>> + if (IS_SKYLAKE(dev) || IS_KABYLAKE(dev)) {
>> + target_vco = to_intel_atomic_state(state)->cdclk_pll_vco;
>> + if (!target_vco)
>> + to_intel_atomic_state(state)->cdclk_pll_vco =
>> + dev_priv->skl_vco_freq;
>> + }
>> +
>> ret = dev_priv->display.modeset_calc_cdclk(state);
>> + if (ret < 0)
>> + return ret;
>> +
>> + cdclk_change = intel_state->dev_cdclk != dev_priv->cdclk_freq;
>> + vco_change = ((IS_SKYLAKE(dev) || IS_KABYLAKE(dev)) &&
>> + (target_vco) && (dev_priv->skl_vco_freq != target_vco));
> I don't think you need all those SKL||KBL checks. And target_vco seems
> like a rather pointless variable. Oh and cdclk_change and vco_change
> could have tighter scope. Eg. something as simple as this ought
> to work I think:
>
> if (dev_priv->display.modeset_calc_cdclk) {
> bool cdclk_change, vco_change;
>
> if (intel_state->cdclk_pll_vco == 0)
> intel_state->cdclk_pll_vco = dev_priv->skl_vco_freq;
>
> ret = dev_priv->display.modeset_calc_cdclk(state);
> if (ret)
> return ret;
>
> cdclk_change = intel_state->dev_cdclk != dev_priv->cdclk_freq;
> vco_change = intel_state->skl_vco_freq != dev_priv->skl_vco_freq;
> ...
>
> Otherwise it looks pretty good to me.
>
>>
>> - if (!ret && intel_state->dev_cdclk != dev_priv->cdclk_freq)
>> + if (cdclk_change || vco_change)
>> ret = intel_modeset_all_pipes(state);
>>
>> if (ret < 0)
>> @@ -15002,6 +15082,11 @@ static void intel_init_display(struct drm_device *dev)
>> broxton_modeset_commit_cdclk;
>> dev_priv->display.modeset_calc_cdclk =
>> broxton_modeset_calc_cdclk;
>> + } else if (IS_SKYLAKE(dev) || IS_KABYLAKE(dev)) {
>> + dev_priv->display.modeset_commit_cdclk =
>> + skl_modeset_commit_cdclk;
>> + dev_priv->display.modeset_calc_cdclk =
>> + skl_modeset_calc_cdclk;
>> }
>>
>> switch (INTEL_INFO(dev)->gen) {
>> @@ -15725,7 +15810,8 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
>> if (crtc_state->base.active) {
>> dev_priv->active_crtcs |= 1 << crtc->pipe;
>>
>> - if (IS_BROADWELL(dev_priv)) {
>> + if (IS_BROADWELL(dev_priv) || IS_SKYLAKE(dev_priv) ||
>> + IS_KABYLAKE(dev_priv)) {
>> pixclk = ilk_pipe_pixel_rate(crtc_state);
>>
>> /* pixel rate mustn't exceed 95% of cdclk with IPS on BDW */
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index a073f04..afa21b6 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -1245,6 +1245,7 @@ static void
>> skl_edp_set_pll_config(struct intel_crtc_state *pipe_config)
>> {
>> u32 ctrl1;
>> + u32 vco = 8100;
>>
>> memset(&pipe_config->dpll_hw_state, 0,
>> sizeof(pipe_config->dpll_hw_state));
>> @@ -1277,13 +1278,16 @@ skl_edp_set_pll_config(struct intel_crtc_state *pipe_config)
>> case 108000:
>> ctrl1 |= DPLL_CTRL1_LINK_RATE(DPLL_CTRL1_LINK_RATE_1080,
>> SKL_DPLL0);
>> + vco = 8640;
>> break;
>> case 216000:
>> ctrl1 |= DPLL_CTRL1_LINK_RATE(DPLL_CTRL1_LINK_RATE_2160,
>> SKL_DPLL0);
>> + vco = 8640;
>> break;
>> -
>> }
>> +
>> + to_intel_atomic_state(pipe_config->base.state)->cdclk_pll_vco = vco;
>> pipe_config->dpll_hw_state.ctrl1 = ctrl1;
>> }
>>
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index 878172a..47936d4 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -260,6 +260,9 @@ struct intel_atomic_state {
>>
>> struct intel_shared_dpll_config shared_dpll[I915_NUM_PLLS];
>> struct intel_wm_config wm_config;
>> +
>> + /* SKL/KBL Only */
>> + unsigned int cdclk_pll_vco;
>> };
>>
>> struct intel_plane_state {
>> @@ -1191,6 +1194,7 @@ void bxt_disable_dc9(struct drm_i915_private *dev_priv);
>> void skl_init_cdclk(struct drm_i915_private *dev_priv);
>> int skl_sanitize_cdclk(struct drm_i915_private *dev_priv);
>> void skl_uninit_cdclk(struct drm_i915_private *dev_priv);
>> +unsigned int skl_cdclk_get_vco(unsigned int freq);
>> void skl_enable_dc6(struct drm_i915_private *dev_priv);
>> void skl_disable_dc6(struct drm_i915_private *dev_priv);
>> void intel_dp_get_m_n(struct intel_crtc *crtc,
>> --
>> 1.7.9.5
More information about the Intel-gfx
mailing list