* [PATCH 0/4] Small clocking code refactor @ 2017-02-17 13:22 Paulo Zanoni 2017-02-17 13:22 ` [PATCH 1/4] drm/i915: kill {bdw, bxt}_modeset_calc_cdclk Paulo Zanoni ` (4 more replies) 0 siblings, 5 replies; 14+ messages in thread From: Paulo Zanoni @ 2017-02-17 13:22 UTC (permalink / raw) To: intel-gfx; +Cc: Paulo Zanoni Hi I've been trying to understand the clocking code and spotted some possible improvements. None of these changes are actually necessary for anything, but IMHO they make the code a little easier to read and later extend. Feel free to bikeshed or even NAK my proposals. Thanks, Paulo Paulo Zanoni (4): drm/i915: kill {bdw,bxt}_modeset_calc_cdclk drm/i915: add intel_calc_cdclk() drm/i915: remove potentially confusing IS_G4X checks drm/i915: reorganize the get_cdclk assignment drivers/gpu/drm/i915/intel_cdclk.c | 257 ++++++++++++++++--------------------- 1 file changed, 112 insertions(+), 145 deletions(-) -- 2.7.4 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 1/4] drm/i915: kill {bdw, bxt}_modeset_calc_cdclk 2017-02-17 13:22 [PATCH 0/4] Small clocking code refactor Paulo Zanoni @ 2017-02-17 13:22 ` Paulo Zanoni 2017-02-17 13:51 ` Ville Syrjälä 2017-02-18 15:13 ` David Weinehall 2017-02-17 13:22 ` [PATCH 2/4] drm/i915: add intel_calc_cdclk() Paulo Zanoni ` (3 subsequent siblings) 4 siblings, 2 replies; 14+ messages in thread From: Paulo Zanoni @ 2017-02-17 13:22 UTC (permalink / raw) To: intel-gfx; +Cc: Paulo Zanoni The functions are pretty much the same, except for the CDCLK and VCO calculations. Add BDW support to vlv_modeset_calc_cdclk() and add BXT/GLK support to skl_modeset_calc_cdclk(). The two reamining functions are still very similar, except for the fact that the vlv version doesn't touch the VCO. Further patches could unify them even more if that's desired. While at it, merge some lines that can fit 80 columns in those functions. Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> --- drivers/gpu/drm/i915/intel_cdclk.c | 120 ++++++++++--------------------------- 1 file changed, 30 insertions(+), 90 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c index d643c0c..d505ff1 100644 --- a/drivers/gpu/drm/i915/intel_cdclk.c +++ b/drivers/gpu/drm/i915/intel_cdclk.c @@ -1499,45 +1499,18 @@ static int intel_max_pixel_rate(struct drm_atomic_state *state) static int vlv_modeset_calc_cdclk(struct drm_atomic_state *state) { struct drm_i915_private *dev_priv = to_i915(state->dev); - int max_pixclk = intel_max_pixel_rate(state); - struct intel_atomic_state *intel_state = - to_intel_atomic_state(state); - int cdclk; - - cdclk = vlv_calc_cdclk(dev_priv, max_pixclk); - - if (cdclk > dev_priv->max_cdclk_freq) { - DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds max (%d kHz)\n", - cdclk, dev_priv->max_cdclk_freq); - return -EINVAL; - } - - intel_state->cdclk.logical.cdclk = cdclk; - - if (!intel_state->active_crtcs) { - cdclk = vlv_calc_cdclk(dev_priv, 0); - - intel_state->cdclk.actual.cdclk = cdclk; - } else { - intel_state->cdclk.actual = - intel_state->cdclk.logical; - } - - return 0; -} - -static int bdw_modeset_calc_cdclk(struct drm_atomic_state *state) -{ - struct drm_i915_private *dev_priv = to_i915(state->dev); struct intel_atomic_state *intel_state = to_intel_atomic_state(state); int max_pixclk = intel_max_pixel_rate(state); int cdclk; /* - * FIXME should also account for plane ratio - * once 64bpp pixel formats are supported. + * FIXME: Broadwell should also account for plane ratio once 64bpp pixel + * formats are supported. */ - cdclk = bdw_calc_cdclk(max_pixclk); + if (IS_BROADWELL(dev_priv)) + cdclk = bdw_calc_cdclk(max_pixclk); + else + cdclk = vlv_calc_cdclk(dev_priv, max_pixclk); if (cdclk > dev_priv->max_cdclk_freq) { DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds max (%d kHz)\n", @@ -1548,12 +1521,14 @@ static int bdw_modeset_calc_cdclk(struct drm_atomic_state *state) intel_state->cdclk.logical.cdclk = cdclk; if (!intel_state->active_crtcs) { - cdclk = bdw_calc_cdclk(0); + if (IS_BROADWELL(dev_priv)) + cdclk = bdw_calc_cdclk(0); + else + cdclk = vlv_calc_cdclk(dev_priv, 0); intel_state->cdclk.actual.cdclk = cdclk; } else { - intel_state->cdclk.actual = - intel_state->cdclk.logical; + intel_state->cdclk.actual = intel_state->cdclk.logical; } return 0; @@ -1561,57 +1536,26 @@ static int bdw_modeset_calc_cdclk(struct drm_atomic_state *state) static int skl_modeset_calc_cdclk(struct drm_atomic_state *state) { - struct intel_atomic_state *intel_state = to_intel_atomic_state(state); struct drm_i915_private *dev_priv = to_i915(state->dev); - const int max_pixclk = intel_max_pixel_rate(state); + struct intel_atomic_state *intel_state = to_intel_atomic_state(state); + int max_pixclk = intel_max_pixel_rate(state); int cdclk, vco; - vco = intel_state->cdclk.logical.vco; - if (!vco) - vco = dev_priv->skl_preferred_vco_freq; - /* - * FIXME should also account for plane ratio - * once 64bpp pixel formats are supported. + * FIXME: Skylake/Kabylake should also account for plane ratio once + * 64bpp pixel formats are supported. */ - cdclk = skl_calc_cdclk(max_pixclk, vco); - - if (cdclk > dev_priv->max_cdclk_freq) { - DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds max (%d kHz)\n", - cdclk, dev_priv->max_cdclk_freq); - return -EINVAL; - } - - intel_state->cdclk.logical.vco = vco; - intel_state->cdclk.logical.cdclk = cdclk; - - if (!intel_state->active_crtcs) { - cdclk = skl_calc_cdclk(0, vco); - - intel_state->cdclk.actual.vco = vco; - intel_state->cdclk.actual.cdclk = cdclk; - } else { - intel_state->cdclk.actual = - intel_state->cdclk.logical; - } - - return 0; -} - -static int bxt_modeset_calc_cdclk(struct drm_atomic_state *state) -{ - struct drm_i915_private *dev_priv = to_i915(state->dev); - int max_pixclk = intel_max_pixel_rate(state); - struct intel_atomic_state *intel_state = - to_intel_atomic_state(state); - int cdclk, vco; - if (IS_GEMINILAKE(dev_priv)) { cdclk = glk_calc_cdclk(max_pixclk); vco = glk_de_pll_vco(dev_priv, cdclk); - } else { + } else if (IS_BROXTON(dev_priv)) { cdclk = bxt_calc_cdclk(max_pixclk); vco = bxt_de_pll_vco(dev_priv, cdclk); + } else { + vco = intel_state->cdclk.logical.vco; + if (!vco) + vco = dev_priv->skl_preferred_vco_freq; + cdclk = skl_calc_cdclk(max_pixclk, vco); } if (cdclk > dev_priv->max_cdclk_freq) { @@ -1627,16 +1571,17 @@ static int bxt_modeset_calc_cdclk(struct drm_atomic_state *state) if (IS_GEMINILAKE(dev_priv)) { cdclk = glk_calc_cdclk(0); vco = glk_de_pll_vco(dev_priv, cdclk); - } else { + } else if (IS_BROXTON(dev_priv)) { cdclk = bxt_calc_cdclk(0); vco = bxt_de_pll_vco(dev_priv, cdclk); + } else { + cdclk = skl_calc_cdclk(0, vco); } intel_state->cdclk.actual.vco = vco; intel_state->cdclk.actual.cdclk = cdclk; } else { - intel_state->cdclk.actual = - intel_state->cdclk.logical; + intel_state->cdclk.actual = intel_state->cdclk.logical; } return 0; @@ -1823,24 +1768,19 @@ void intel_init_cdclk_hooks(struct drm_i915_private *dev_priv) { if (IS_CHERRYVIEW(dev_priv)) { dev_priv->display.set_cdclk = chv_set_cdclk; - dev_priv->display.modeset_calc_cdclk = - vlv_modeset_calc_cdclk; + dev_priv->display.modeset_calc_cdclk = vlv_modeset_calc_cdclk; } else if (IS_VALLEYVIEW(dev_priv)) { dev_priv->display.set_cdclk = vlv_set_cdclk; - dev_priv->display.modeset_calc_cdclk = - vlv_modeset_calc_cdclk; + dev_priv->display.modeset_calc_cdclk = vlv_modeset_calc_cdclk; } else if (IS_BROADWELL(dev_priv)) { dev_priv->display.set_cdclk = bdw_set_cdclk; - dev_priv->display.modeset_calc_cdclk = - bdw_modeset_calc_cdclk; + dev_priv->display.modeset_calc_cdclk = vlv_modeset_calc_cdclk; } else if (IS_GEN9_LP(dev_priv)) { dev_priv->display.set_cdclk = bxt_set_cdclk; - dev_priv->display.modeset_calc_cdclk = - bxt_modeset_calc_cdclk; + dev_priv->display.modeset_calc_cdclk = skl_modeset_calc_cdclk; } else if (IS_GEN9_BC(dev_priv)) { dev_priv->display.set_cdclk = skl_set_cdclk; - dev_priv->display.modeset_calc_cdclk = - skl_modeset_calc_cdclk; + dev_priv->display.modeset_calc_cdclk = skl_modeset_calc_cdclk; } if (IS_GEN9_BC(dev_priv)) -- 2.7.4 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 1/4] drm/i915: kill {bdw, bxt}_modeset_calc_cdclk 2017-02-17 13:22 ` [PATCH 1/4] drm/i915: kill {bdw, bxt}_modeset_calc_cdclk Paulo Zanoni @ 2017-02-17 13:51 ` Ville Syrjälä 2017-02-18 15:13 ` David Weinehall 1 sibling, 0 replies; 14+ messages in thread From: Ville Syrjälä @ 2017-02-17 13:51 UTC (permalink / raw) To: Paulo Zanoni; +Cc: intel-gfx On Fri, Feb 17, 2017 at 11:22:04AM -0200, Paulo Zanoni wrote: > The functions are pretty much the same, except for the CDCLK and VCO > calculations. Add BDW support to vlv_modeset_calc_cdclk() and add > BXT/GLK support to skl_modeset_calc_cdclk(). The two reamining > functions are still very similar, except for the fact that the vlv > version doesn't touch the VCO. Further patches could unify them even > more if that's desired. > > While at it, merge some lines that can fit 80 columns in those > functions. > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > --- > drivers/gpu/drm/i915/intel_cdclk.c | 120 ++++++++++--------------------------- > 1 file changed, 30 insertions(+), 90 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c > index d643c0c..d505ff1 100644 > --- a/drivers/gpu/drm/i915/intel_cdclk.c > +++ b/drivers/gpu/drm/i915/intel_cdclk.c > @@ -1499,45 +1499,18 @@ static int intel_max_pixel_rate(struct drm_atomic_state *state) > static int vlv_modeset_calc_cdclk(struct drm_atomic_state *state) > { > struct drm_i915_private *dev_priv = to_i915(state->dev); > - int max_pixclk = intel_max_pixel_rate(state); > - struct intel_atomic_state *intel_state = > - to_intel_atomic_state(state); > - int cdclk; > - > - cdclk = vlv_calc_cdclk(dev_priv, max_pixclk); > - > - if (cdclk > dev_priv->max_cdclk_freq) { > - DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds max (%d kHz)\n", > - cdclk, dev_priv->max_cdclk_freq); > - return -EINVAL; > - } > - > - intel_state->cdclk.logical.cdclk = cdclk; > - > - if (!intel_state->active_crtcs) { > - cdclk = vlv_calc_cdclk(dev_priv, 0); > - > - intel_state->cdclk.actual.cdclk = cdclk; > - } else { > - intel_state->cdclk.actual = > - intel_state->cdclk.logical; > - } > - > - return 0; > -} > - > -static int bdw_modeset_calc_cdclk(struct drm_atomic_state *state) > -{ > - struct drm_i915_private *dev_priv = to_i915(state->dev); > struct intel_atomic_state *intel_state = to_intel_atomic_state(state); > int max_pixclk = intel_max_pixel_rate(state); > int cdclk; > > /* > - * FIXME should also account for plane ratio > - * once 64bpp pixel formats are supported. > + * FIXME: Broadwell should also account for plane ratio once 64bpp pixel > + * formats are supported. BTW these restrictions affect pretty much all platforms, so specifying the platforms in the comment is rather redundant. > */ > - cdclk = bdw_calc_cdclk(max_pixclk); > + if (IS_BROADWELL(dev_priv)) > + cdclk = bdw_calc_cdclk(max_pixclk); > + else > + cdclk = vlv_calc_cdclk(dev_priv, max_pixclk); > > if (cdclk > dev_priv->max_cdclk_freq) { > DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds max (%d kHz)\n", > @@ -1548,12 +1521,14 @@ static int bdw_modeset_calc_cdclk(struct drm_atomic_state *state) > intel_state->cdclk.logical.cdclk = cdclk; > > if (!intel_state->active_crtcs) { > - cdclk = bdw_calc_cdclk(0); > + if (IS_BROADWELL(dev_priv)) > + cdclk = bdw_calc_cdclk(0); > + else > + cdclk = vlv_calc_cdclk(dev_priv, 0); > > intel_state->cdclk.actual.cdclk = cdclk; > } else { > - intel_state->cdclk.actual = > - intel_state->cdclk.logical; > + intel_state->cdclk.actual = intel_state->cdclk.logical; > } > > return 0; > @@ -1561,57 +1536,26 @@ static int bdw_modeset_calc_cdclk(struct drm_atomic_state *state) > > static int skl_modeset_calc_cdclk(struct drm_atomic_state *state) > { > - struct intel_atomic_state *intel_state = to_intel_atomic_state(state); > struct drm_i915_private *dev_priv = to_i915(state->dev); > - const int max_pixclk = intel_max_pixel_rate(state); > + struct intel_atomic_state *intel_state = to_intel_atomic_state(state); > + int max_pixclk = intel_max_pixel_rate(state); > int cdclk, vco; > > - vco = intel_state->cdclk.logical.vco; > - if (!vco) > - vco = dev_priv->skl_preferred_vco_freq; > - > /* > - * FIXME should also account for plane ratio > - * once 64bpp pixel formats are supported. > + * FIXME: Skylake/Kabylake should also account for plane ratio once > + * 64bpp pixel formats are supported. > */ > - cdclk = skl_calc_cdclk(max_pixclk, vco); > - > - if (cdclk > dev_priv->max_cdclk_freq) { > - DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds max (%d kHz)\n", > - cdclk, dev_priv->max_cdclk_freq); > - return -EINVAL; > - } > - > - intel_state->cdclk.logical.vco = vco; > - intel_state->cdclk.logical.cdclk = cdclk; > - > - if (!intel_state->active_crtcs) { > - cdclk = skl_calc_cdclk(0, vco); > - > - intel_state->cdclk.actual.vco = vco; > - intel_state->cdclk.actual.cdclk = cdclk; > - } else { > - intel_state->cdclk.actual = > - intel_state->cdclk.logical; > - } > - > - return 0; > -} > - > -static int bxt_modeset_calc_cdclk(struct drm_atomic_state *state) > -{ > - struct drm_i915_private *dev_priv = to_i915(state->dev); > - int max_pixclk = intel_max_pixel_rate(state); > - struct intel_atomic_state *intel_state = > - to_intel_atomic_state(state); > - int cdclk, vco; > - > if (IS_GEMINILAKE(dev_priv)) { > cdclk = glk_calc_cdclk(max_pixclk); > vco = glk_de_pll_vco(dev_priv, cdclk); > - } else { > + } else if (IS_BROXTON(dev_priv)) { > cdclk = bxt_calc_cdclk(max_pixclk); > vco = bxt_de_pll_vco(dev_priv, cdclk); > + } else { > + vco = intel_state->cdclk.logical.vco; > + if (!vco) > + vco = dev_priv->skl_preferred_vco_freq; > + cdclk = skl_calc_cdclk(max_pixclk, vco); > } > > if (cdclk > dev_priv->max_cdclk_freq) { > @@ -1627,16 +1571,17 @@ static int bxt_modeset_calc_cdclk(struct drm_atomic_state *state) > if (IS_GEMINILAKE(dev_priv)) { > cdclk = glk_calc_cdclk(0); > vco = glk_de_pll_vco(dev_priv, cdclk); > - } else { > + } else if (IS_BROXTON(dev_priv)) { > cdclk = bxt_calc_cdclk(0); > vco = bxt_de_pll_vco(dev_priv, cdclk); > + } else { > + cdclk = skl_calc_cdclk(0, vco); > } > > intel_state->cdclk.actual.vco = vco; > intel_state->cdclk.actual.cdclk = cdclk; > } else { > - intel_state->cdclk.actual = > - intel_state->cdclk.logical; > + intel_state->cdclk.actual = intel_state->cdclk.logical; > } > > return 0; > @@ -1823,24 +1768,19 @@ void intel_init_cdclk_hooks(struct drm_i915_private *dev_priv) > { > if (IS_CHERRYVIEW(dev_priv)) { > dev_priv->display.set_cdclk = chv_set_cdclk; > - dev_priv->display.modeset_calc_cdclk = > - vlv_modeset_calc_cdclk; > + dev_priv->display.modeset_calc_cdclk = vlv_modeset_calc_cdclk; > } else if (IS_VALLEYVIEW(dev_priv)) { > dev_priv->display.set_cdclk = vlv_set_cdclk; > - dev_priv->display.modeset_calc_cdclk = > - vlv_modeset_calc_cdclk; > + dev_priv->display.modeset_calc_cdclk = vlv_modeset_calc_cdclk; > } else if (IS_BROADWELL(dev_priv)) { > dev_priv->display.set_cdclk = bdw_set_cdclk; > - dev_priv->display.modeset_calc_cdclk = > - bdw_modeset_calc_cdclk; > + dev_priv->display.modeset_calc_cdclk = vlv_modeset_calc_cdclk; > } else if (IS_GEN9_LP(dev_priv)) { > dev_priv->display.set_cdclk = bxt_set_cdclk; > - dev_priv->display.modeset_calc_cdclk = > - bxt_modeset_calc_cdclk; > + dev_priv->display.modeset_calc_cdclk = skl_modeset_calc_cdclk; > } else if (IS_GEN9_BC(dev_priv)) { > dev_priv->display.set_cdclk = skl_set_cdclk; > - dev_priv->display.modeset_calc_cdclk = > - skl_modeset_calc_cdclk; > + dev_priv->display.modeset_calc_cdclk = skl_modeset_calc_cdclk; > } > > if (IS_GEN9_BC(dev_priv)) > -- > 2.7.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ville Syrjälä Intel OTC _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/4] drm/i915: kill {bdw, bxt}_modeset_calc_cdclk 2017-02-17 13:22 ` [PATCH 1/4] drm/i915: kill {bdw, bxt}_modeset_calc_cdclk Paulo Zanoni 2017-02-17 13:51 ` Ville Syrjälä @ 2017-02-18 15:13 ` David Weinehall 1 sibling, 0 replies; 14+ messages in thread From: David Weinehall @ 2017-02-18 15:13 UTC (permalink / raw) To: Paulo Zanoni; +Cc: intel-gfx On Fri, Feb 17, 2017 at 11:22:04AM -0200, Paulo Zanoni wrote: > The functions are pretty much the same, except for the CDCLK and VCO > calculations. Add BDW support to vlv_modeset_calc_cdclk() and add > BXT/GLK support to skl_modeset_calc_cdclk(). The two reamining s/reamining/remaining/ > functions are still very similar, except for the fact that the vlv > version doesn't touch the VCO. Further patches could unify them even > more if that's desired. > > While at it, merge some lines that can fit 80 columns in those > functions. > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > --- > drivers/gpu/drm/i915/intel_cdclk.c | 120 ++++++++++--------------------------- > 1 file changed, 30 insertions(+), 90 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c > index d643c0c..d505ff1 100644 > --- a/drivers/gpu/drm/i915/intel_cdclk.c > +++ b/drivers/gpu/drm/i915/intel_cdclk.c > @@ -1499,45 +1499,18 @@ static int intel_max_pixel_rate(struct drm_atomic_state *state) > static int vlv_modeset_calc_cdclk(struct drm_atomic_state *state) > { > struct drm_i915_private *dev_priv = to_i915(state->dev); > - int max_pixclk = intel_max_pixel_rate(state); > - struct intel_atomic_state *intel_state = > - to_intel_atomic_state(state); > - int cdclk; > - > - cdclk = vlv_calc_cdclk(dev_priv, max_pixclk); > - > - if (cdclk > dev_priv->max_cdclk_freq) { > - DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds max (%d kHz)\n", > - cdclk, dev_priv->max_cdclk_freq); > - return -EINVAL; > - } > - > - intel_state->cdclk.logical.cdclk = cdclk; > - > - if (!intel_state->active_crtcs) { > - cdclk = vlv_calc_cdclk(dev_priv, 0); > - > - intel_state->cdclk.actual.cdclk = cdclk; > - } else { > - intel_state->cdclk.actual = > - intel_state->cdclk.logical; > - } > - > - return 0; > -} > - > -static int bdw_modeset_calc_cdclk(struct drm_atomic_state *state) > -{ > - struct drm_i915_private *dev_priv = to_i915(state->dev); > struct intel_atomic_state *intel_state = to_intel_atomic_state(state); > int max_pixclk = intel_max_pixel_rate(state); > int cdclk; > > /* > - * FIXME should also account for plane ratio > - * once 64bpp pixel formats are supported. > + * FIXME: Broadwell should also account for plane ratio once 64bpp pixel > + * formats are supported. > */ > - cdclk = bdw_calc_cdclk(max_pixclk); > + if (IS_BROADWELL(dev_priv)) > + cdclk = bdw_calc_cdclk(max_pixclk); > + else > + cdclk = vlv_calc_cdclk(dev_priv, max_pixclk); > > if (cdclk > dev_priv->max_cdclk_freq) { > DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds max (%d kHz)\n", > @@ -1548,12 +1521,14 @@ static int bdw_modeset_calc_cdclk(struct drm_atomic_state *state) > intel_state->cdclk.logical.cdclk = cdclk; > > if (!intel_state->active_crtcs) { > - cdclk = bdw_calc_cdclk(0); > + if (IS_BROADWELL(dev_priv)) > + cdclk = bdw_calc_cdclk(0); > + else > + cdclk = vlv_calc_cdclk(dev_priv, 0); > > intel_state->cdclk.actual.cdclk = cdclk; > } else { > - intel_state->cdclk.actual = > - intel_state->cdclk.logical; > + intel_state->cdclk.actual = intel_state->cdclk.logical; > } > > return 0; > @@ -1561,57 +1536,26 @@ static int bdw_modeset_calc_cdclk(struct drm_atomic_state *state) > > static int skl_modeset_calc_cdclk(struct drm_atomic_state *state) > { > - struct intel_atomic_state *intel_state = to_intel_atomic_state(state); > struct drm_i915_private *dev_priv = to_i915(state->dev); > - const int max_pixclk = intel_max_pixel_rate(state); > + struct intel_atomic_state *intel_state = to_intel_atomic_state(state); > + int max_pixclk = intel_max_pixel_rate(state); > int cdclk, vco; > > - vco = intel_state->cdclk.logical.vco; > - if (!vco) > - vco = dev_priv->skl_preferred_vco_freq; > - > /* > - * FIXME should also account for plane ratio > - * once 64bpp pixel formats are supported. > + * FIXME: Skylake/Kabylake should also account for plane ratio once > + * 64bpp pixel formats are supported. > */ > - cdclk = skl_calc_cdclk(max_pixclk, vco); > - > - if (cdclk > dev_priv->max_cdclk_freq) { > - DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds max (%d kHz)\n", > - cdclk, dev_priv->max_cdclk_freq); > - return -EINVAL; > - } > - > - intel_state->cdclk.logical.vco = vco; > - intel_state->cdclk.logical.cdclk = cdclk; > - > - if (!intel_state->active_crtcs) { > - cdclk = skl_calc_cdclk(0, vco); > - > - intel_state->cdclk.actual.vco = vco; > - intel_state->cdclk.actual.cdclk = cdclk; > - } else { > - intel_state->cdclk.actual = > - intel_state->cdclk.logical; > - } > - > - return 0; > -} > - > -static int bxt_modeset_calc_cdclk(struct drm_atomic_state *state) > -{ > - struct drm_i915_private *dev_priv = to_i915(state->dev); > - int max_pixclk = intel_max_pixel_rate(state); > - struct intel_atomic_state *intel_state = > - to_intel_atomic_state(state); > - int cdclk, vco; > - > if (IS_GEMINILAKE(dev_priv)) { > cdclk = glk_calc_cdclk(max_pixclk); > vco = glk_de_pll_vco(dev_priv, cdclk); > - } else { > + } else if (IS_BROXTON(dev_priv)) { > cdclk = bxt_calc_cdclk(max_pixclk); > vco = bxt_de_pll_vco(dev_priv, cdclk); > + } else { > + vco = intel_state->cdclk.logical.vco; > + if (!vco) > + vco = dev_priv->skl_preferred_vco_freq; > + cdclk = skl_calc_cdclk(max_pixclk, vco); > } > > if (cdclk > dev_priv->max_cdclk_freq) { > @@ -1627,16 +1571,17 @@ static int bxt_modeset_calc_cdclk(struct drm_atomic_state *state) > if (IS_GEMINILAKE(dev_priv)) { > cdclk = glk_calc_cdclk(0); > vco = glk_de_pll_vco(dev_priv, cdclk); > - } else { > + } else if (IS_BROXTON(dev_priv)) { > cdclk = bxt_calc_cdclk(0); > vco = bxt_de_pll_vco(dev_priv, cdclk); > + } else { > + cdclk = skl_calc_cdclk(0, vco); > } > > intel_state->cdclk.actual.vco = vco; > intel_state->cdclk.actual.cdclk = cdclk; > } else { > - intel_state->cdclk.actual = > - intel_state->cdclk.logical; > + intel_state->cdclk.actual = intel_state->cdclk.logical; > } > > return 0; > @@ -1823,24 +1768,19 @@ void intel_init_cdclk_hooks(struct drm_i915_private *dev_priv) > { > if (IS_CHERRYVIEW(dev_priv)) { > dev_priv->display.set_cdclk = chv_set_cdclk; > - dev_priv->display.modeset_calc_cdclk = > - vlv_modeset_calc_cdclk; > + dev_priv->display.modeset_calc_cdclk = vlv_modeset_calc_cdclk; > } else if (IS_VALLEYVIEW(dev_priv)) { > dev_priv->display.set_cdclk = vlv_set_cdclk; > - dev_priv->display.modeset_calc_cdclk = > - vlv_modeset_calc_cdclk; > + dev_priv->display.modeset_calc_cdclk = vlv_modeset_calc_cdclk; > } else if (IS_BROADWELL(dev_priv)) { > dev_priv->display.set_cdclk = bdw_set_cdclk; > - dev_priv->display.modeset_calc_cdclk = > - bdw_modeset_calc_cdclk; > + dev_priv->display.modeset_calc_cdclk = vlv_modeset_calc_cdclk; > } else if (IS_GEN9_LP(dev_priv)) { > dev_priv->display.set_cdclk = bxt_set_cdclk; > - dev_priv->display.modeset_calc_cdclk = > - bxt_modeset_calc_cdclk; > + dev_priv->display.modeset_calc_cdclk = skl_modeset_calc_cdclk; > } else if (IS_GEN9_BC(dev_priv)) { > dev_priv->display.set_cdclk = skl_set_cdclk; > - dev_priv->display.modeset_calc_cdclk = > - skl_modeset_calc_cdclk; > + dev_priv->display.modeset_calc_cdclk = skl_modeset_calc_cdclk; > } > > if (IS_GEN9_BC(dev_priv)) > -- > 2.7.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 2/4] drm/i915: add intel_calc_cdclk() 2017-02-17 13:22 [PATCH 0/4] Small clocking code refactor Paulo Zanoni 2017-02-17 13:22 ` [PATCH 1/4] drm/i915: kill {bdw, bxt}_modeset_calc_cdclk Paulo Zanoni @ 2017-02-17 13:22 ` Paulo Zanoni 2017-02-17 13:49 ` Ville Syrjälä 2017-02-17 13:22 ` [PATCH 3/4] drm/i915: remove potentially confusing IS_G4X checks Paulo Zanoni ` (2 subsequent siblings) 4 siblings, 1 reply; 14+ messages in thread From: Paulo Zanoni @ 2017-02-17 13:22 UTC (permalink / raw) To: intel-gfx; +Cc: Paulo Zanoni Each x_modeset_calc_cdclk() has to do the same platform checks twice, so extract them to a single function. This way, the platform checks are all in the same place, and the platform-common code gets rid of all the platform-specific checks, which IMHO makes the code easier to read. Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> --- drivers/gpu/drm/i915/intel_cdclk.c | 84 ++++++++++++++++++++------------------ 1 file changed, 45 insertions(+), 39 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c index d505ff1..6efc5f4 100644 --- a/drivers/gpu/drm/i915/intel_cdclk.c +++ b/drivers/gpu/drm/i915/intel_cdclk.c @@ -1496,6 +1496,47 @@ static int intel_max_pixel_rate(struct drm_atomic_state *state) return max_pixel_rate; } +static void intel_calc_cdclk(struct intel_atomic_state *state, int max_pixclk, + int *cdclk, int *vco) +{ + struct drm_i915_private *dev_priv = to_i915(state->base.dev); + + switch (INTEL_INFO(dev_priv)->platform) { + case INTEL_VALLEYVIEW: + case INTEL_CHERRYVIEW: + *cdclk = vlv_calc_cdclk(dev_priv, max_pixclk); + break; + case INTEL_BROADWELL: + /* + * FIXME: should also account for plane ratio once 64bpp pixel + * formats are supported. + */ + *cdclk = bdw_calc_cdclk(max_pixclk); + break; + case INTEL_SKYLAKE: + case INTEL_KABYLAKE: + /* + * FIXME: should also account for plane ratio once 64bpp pixel + * formats are supported. + */ + *vco = state->cdclk.logical.vco; + if (!*vco) + *vco = dev_priv->skl_preferred_vco_freq; + *cdclk = skl_calc_cdclk(max_pixclk, *vco); + break; + case INTEL_BROXTON: + *cdclk = bxt_calc_cdclk(max_pixclk); + *vco = bxt_de_pll_vco(dev_priv, *cdclk); + break; + case INTEL_GEMINILAKE: + *cdclk = glk_calc_cdclk(max_pixclk); + *vco = glk_de_pll_vco(dev_priv, *cdclk); + break; + default: + MISSING_CASE(INTEL_INFO(dev_priv)->platform); + } +} + static int vlv_modeset_calc_cdclk(struct drm_atomic_state *state) { struct drm_i915_private *dev_priv = to_i915(state->dev); @@ -1503,14 +1544,7 @@ static int vlv_modeset_calc_cdclk(struct drm_atomic_state *state) int max_pixclk = intel_max_pixel_rate(state); int cdclk; - /* - * FIXME: Broadwell should also account for plane ratio once 64bpp pixel - * formats are supported. - */ - if (IS_BROADWELL(dev_priv)) - cdclk = bdw_calc_cdclk(max_pixclk); - else - cdclk = vlv_calc_cdclk(dev_priv, max_pixclk); + intel_calc_cdclk(intel_state, max_pixclk, &cdclk, NULL); if (cdclk > dev_priv->max_cdclk_freq) { DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds max (%d kHz)\n", @@ -1521,11 +1555,7 @@ static int vlv_modeset_calc_cdclk(struct drm_atomic_state *state) intel_state->cdclk.logical.cdclk = cdclk; if (!intel_state->active_crtcs) { - if (IS_BROADWELL(dev_priv)) - cdclk = bdw_calc_cdclk(0); - else - cdclk = vlv_calc_cdclk(dev_priv, 0); - + intel_calc_cdclk(intel_state, 0, &cdclk, NULL); intel_state->cdclk.actual.cdclk = cdclk; } else { intel_state->cdclk.actual = intel_state->cdclk.logical; @@ -1541,22 +1571,7 @@ static int skl_modeset_calc_cdclk(struct drm_atomic_state *state) int max_pixclk = intel_max_pixel_rate(state); int cdclk, vco; - /* - * FIXME: Skylake/Kabylake should also account for plane ratio once - * 64bpp pixel formats are supported. - */ - if (IS_GEMINILAKE(dev_priv)) { - cdclk = glk_calc_cdclk(max_pixclk); - vco = glk_de_pll_vco(dev_priv, cdclk); - } else if (IS_BROXTON(dev_priv)) { - cdclk = bxt_calc_cdclk(max_pixclk); - vco = bxt_de_pll_vco(dev_priv, cdclk); - } else { - vco = intel_state->cdclk.logical.vco; - if (!vco) - vco = dev_priv->skl_preferred_vco_freq; - cdclk = skl_calc_cdclk(max_pixclk, vco); - } + intel_calc_cdclk(intel_state, max_pixclk, &cdclk, &vco); if (cdclk > dev_priv->max_cdclk_freq) { DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds max (%d kHz)\n", @@ -1568,16 +1583,7 @@ static int skl_modeset_calc_cdclk(struct drm_atomic_state *state) intel_state->cdclk.logical.cdclk = cdclk; if (!intel_state->active_crtcs) { - if (IS_GEMINILAKE(dev_priv)) { - cdclk = glk_calc_cdclk(0); - vco = glk_de_pll_vco(dev_priv, cdclk); - } else if (IS_BROXTON(dev_priv)) { - cdclk = bxt_calc_cdclk(0); - vco = bxt_de_pll_vco(dev_priv, cdclk); - } else { - cdclk = skl_calc_cdclk(0, vco); - } - + intel_calc_cdclk(intel_state, 0, &cdclk, &vco); intel_state->cdclk.actual.vco = vco; intel_state->cdclk.actual.cdclk = cdclk; } else { -- 2.7.4 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 2/4] drm/i915: add intel_calc_cdclk() 2017-02-17 13:22 ` [PATCH 2/4] drm/i915: add intel_calc_cdclk() Paulo Zanoni @ 2017-02-17 13:49 ` Ville Syrjälä 2017-02-17 20:37 ` Paulo Zanoni 0 siblings, 1 reply; 14+ messages in thread From: Ville Syrjälä @ 2017-02-17 13:49 UTC (permalink / raw) To: Paulo Zanoni; +Cc: intel-gfx On Fri, Feb 17, 2017 at 11:22:05AM -0200, Paulo Zanoni wrote: > Each x_modeset_calc_cdclk() has to do the same platform checks twice, > so extract them to a single function. This way, the platform checks > are all in the same place, and the platform-common code gets rid of > all the platform-specific checks, which IMHO makes the code easier to > read. > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > --- > drivers/gpu/drm/i915/intel_cdclk.c | 84 ++++++++++++++++++++------------------ > 1 file changed, 45 insertions(+), 39 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c > index d505ff1..6efc5f4 100644 > --- a/drivers/gpu/drm/i915/intel_cdclk.c > +++ b/drivers/gpu/drm/i915/intel_cdclk.c > @@ -1496,6 +1496,47 @@ static int intel_max_pixel_rate(struct drm_atomic_state *state) > return max_pixel_rate; > } > > +static void intel_calc_cdclk(struct intel_atomic_state *state, int max_pixclk, > + int *cdclk, int *vco) > +{ > + struct drm_i915_private *dev_priv = to_i915(state->base.dev); > + > + switch (INTEL_INFO(dev_priv)->platform) { > + case INTEL_VALLEYVIEW: > + case INTEL_CHERRYVIEW: > + *cdclk = vlv_calc_cdclk(dev_priv, max_pixclk); > + break; > + case INTEL_BROADWELL: > + /* > + * FIXME: should also account for plane ratio once 64bpp pixel > + * formats are supported. > + */ > + *cdclk = bdw_calc_cdclk(max_pixclk); > + break; > + case INTEL_SKYLAKE: > + case INTEL_KABYLAKE: > + /* > + * FIXME: should also account for plane ratio once 64bpp pixel > + * formats are supported. > + */ > + *vco = state->cdclk.logical.vco; > + if (!*vco) > + *vco = dev_priv->skl_preferred_vco_freq; > + *cdclk = skl_calc_cdclk(max_pixclk, *vco); > + break; > + case INTEL_BROXTON: > + *cdclk = bxt_calc_cdclk(max_pixclk); > + *vco = bxt_de_pll_vco(dev_priv, *cdclk); > + break; > + case INTEL_GEMINILAKE: > + *cdclk = glk_calc_cdclk(max_pixclk); > + *vco = glk_de_pll_vco(dev_priv, *cdclk); > + break; > + default: > + MISSING_CASE(INTEL_INFO(dev_priv)->platform); > + } > +} How about just replacing the .modeset_calc_cdclk() vfunc with a slightly lower level vfunc that just computes the cdclk/vco/whatever without containing the active_crtcs logic? Then we should have just intel_modeset_calc_cdclk() { .calc_cdclk(logical, max_pixclk); /* * maybe keep the max_cdclk check here, although it that * happens I think we have a bug somewhere, so perhaps * just convert it into a WARN, or drop entirely. */ if (!active_crtcs) .calc_cdclk(actual, 0); else actual = logical; } > + > static int vlv_modeset_calc_cdclk(struct drm_atomic_state *state) > { > struct drm_i915_private *dev_priv = to_i915(state->dev); > @@ -1503,14 +1544,7 @@ static int vlv_modeset_calc_cdclk(struct drm_atomic_state *state) > int max_pixclk = intel_max_pixel_rate(state); > int cdclk; > > - /* > - * FIXME: Broadwell should also account for plane ratio once 64bpp pixel > - * formats are supported. > - */ > - if (IS_BROADWELL(dev_priv)) > - cdclk = bdw_calc_cdclk(max_pixclk); > - else > - cdclk = vlv_calc_cdclk(dev_priv, max_pixclk); > + intel_calc_cdclk(intel_state, max_pixclk, &cdclk, NULL); > > if (cdclk > dev_priv->max_cdclk_freq) { > DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds max (%d kHz)\n", > @@ -1521,11 +1555,7 @@ static int vlv_modeset_calc_cdclk(struct drm_atomic_state *state) > intel_state->cdclk.logical.cdclk = cdclk; > > if (!intel_state->active_crtcs) { > - if (IS_BROADWELL(dev_priv)) > - cdclk = bdw_calc_cdclk(0); > - else > - cdclk = vlv_calc_cdclk(dev_priv, 0); > - > + intel_calc_cdclk(intel_state, 0, &cdclk, NULL); > intel_state->cdclk.actual.cdclk = cdclk; > } else { > intel_state->cdclk.actual = intel_state->cdclk.logical; > @@ -1541,22 +1571,7 @@ static int skl_modeset_calc_cdclk(struct drm_atomic_state *state) > int max_pixclk = intel_max_pixel_rate(state); > int cdclk, vco; > > - /* > - * FIXME: Skylake/Kabylake should also account for plane ratio once > - * 64bpp pixel formats are supported. > - */ > - if (IS_GEMINILAKE(dev_priv)) { > - cdclk = glk_calc_cdclk(max_pixclk); > - vco = glk_de_pll_vco(dev_priv, cdclk); > - } else if (IS_BROXTON(dev_priv)) { > - cdclk = bxt_calc_cdclk(max_pixclk); > - vco = bxt_de_pll_vco(dev_priv, cdclk); > - } else { > - vco = intel_state->cdclk.logical.vco; > - if (!vco) > - vco = dev_priv->skl_preferred_vco_freq; > - cdclk = skl_calc_cdclk(max_pixclk, vco); > - } > + intel_calc_cdclk(intel_state, max_pixclk, &cdclk, &vco); > > if (cdclk > dev_priv->max_cdclk_freq) { > DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds max (%d kHz)\n", > @@ -1568,16 +1583,7 @@ static int skl_modeset_calc_cdclk(struct drm_atomic_state *state) > intel_state->cdclk.logical.cdclk = cdclk; > > if (!intel_state->active_crtcs) { > - if (IS_GEMINILAKE(dev_priv)) { > - cdclk = glk_calc_cdclk(0); > - vco = glk_de_pll_vco(dev_priv, cdclk); > - } else if (IS_BROXTON(dev_priv)) { > - cdclk = bxt_calc_cdclk(0); > - vco = bxt_de_pll_vco(dev_priv, cdclk); > - } else { > - cdclk = skl_calc_cdclk(0, vco); > - } > - > + intel_calc_cdclk(intel_state, 0, &cdclk, &vco); > intel_state->cdclk.actual.vco = vco; > intel_state->cdclk.actual.cdclk = cdclk; > } else { > -- > 2.7.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ville Syrjälä Intel OTC _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/4] drm/i915: add intel_calc_cdclk() 2017-02-17 13:49 ` Ville Syrjälä @ 2017-02-17 20:37 ` Paulo Zanoni 2017-02-17 20:48 ` Ville Syrjälä 0 siblings, 1 reply; 14+ messages in thread From: Paulo Zanoni @ 2017-02-17 20:37 UTC (permalink / raw) To: Ville Syrjälä; +Cc: intel-gfx Em Sex, 2017-02-17 às 15:49 +0200, Ville Syrjälä escreveu: > On Fri, Feb 17, 2017 at 11:22:05AM -0200, Paulo Zanoni wrote: > > > > Each x_modeset_calc_cdclk() has to do the same platform checks > > twice, > > so extract them to a single function. This way, the platform checks > > are all in the same place, and the platform-common code gets rid of > > all the platform-specific checks, which IMHO makes the code easier > > to > > read. > > > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > --- > > drivers/gpu/drm/i915/intel_cdclk.c | 84 ++++++++++++++++++++---- > > -------------- > > 1 file changed, 45 insertions(+), 39 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_cdclk.c > > b/drivers/gpu/drm/i915/intel_cdclk.c > > index d505ff1..6efc5f4 100644 > > --- a/drivers/gpu/drm/i915/intel_cdclk.c > > +++ b/drivers/gpu/drm/i915/intel_cdclk.c > > @@ -1496,6 +1496,47 @@ static int intel_max_pixel_rate(struct > > drm_atomic_state *state) > > return max_pixel_rate; > > } > > > > +static void intel_calc_cdclk(struct intel_atomic_state *state, int > > max_pixclk, > > + int *cdclk, int *vco) > > +{ > > + struct drm_i915_private *dev_priv = to_i915(state- > > >base.dev); > > + > > + switch (INTEL_INFO(dev_priv)->platform) { > > + case INTEL_VALLEYVIEW: > > + case INTEL_CHERRYVIEW: > > + *cdclk = vlv_calc_cdclk(dev_priv, max_pixclk); > > + break; > > + case INTEL_BROADWELL: > > + /* > > + * FIXME: should also account for plane ratio once > > 64bpp pixel > > + * formats are supported. > > + */ > > + *cdclk = bdw_calc_cdclk(max_pixclk); > > + break; > > + case INTEL_SKYLAKE: > > + case INTEL_KABYLAKE: > > + /* > > + * FIXME: should also account for plane ratio once > > 64bpp pixel > > + * formats are supported. > > + */ > > + *vco = state->cdclk.logical.vco; > > + if (!*vco) > > + *vco = dev_priv->skl_preferred_vco_freq; > > + *cdclk = skl_calc_cdclk(max_pixclk, *vco); > > + break; > > + case INTEL_BROXTON: > > + *cdclk = bxt_calc_cdclk(max_pixclk); > > + *vco = bxt_de_pll_vco(dev_priv, *cdclk); > > + break; > > + case INTEL_GEMINILAKE: > > + *cdclk = glk_calc_cdclk(max_pixclk); > > + *vco = glk_de_pll_vco(dev_priv, *cdclk); > > + break; > > + default: > > + MISSING_CASE(INTEL_INFO(dev_priv)->platform); > > + } > > +} > > How about just replacing the .modeset_calc_cdclk() vfunc with a > slightly > lower level vfunc that just computes the cdclk/vco/whatever without > containing the active_crtcs logic? > > Then we should have just > > intel_modeset_calc_cdclk() > { > .calc_cdclk(logical, max_pixclk); > > /* > * maybe keep the max_cdclk check here, although it that > * happens I think we have a bug somewhere, so perhaps > * just convert it into a WARN, or drop entirely. > */ > > if (!active_crtcs) > .calc_cdclk(actual, 0); > else > actual = logical; > } Yeah, the code above is definitely a next step to the changes I did. I'm just not a big fan of the .calc_cdclk vfunc since it will be just 2 lines for each platform. Unless I inline them with the *real* x_calc_cdclk() funcs we have today, but then I'll have to check their other callers. So I'll take a look and try to submit a new patch. > > > > > > + > > static int vlv_modeset_calc_cdclk(struct drm_atomic_state *state) > > { > > struct drm_i915_private *dev_priv = to_i915(state->dev); > > @@ -1503,14 +1544,7 @@ static int vlv_modeset_calc_cdclk(struct > > drm_atomic_state *state) > > int max_pixclk = intel_max_pixel_rate(state); > > int cdclk; > > > > - /* > > - * FIXME: Broadwell should also account for plane ratio > > once 64bpp pixel > > - * formats are supported. > > - */ > > - if (IS_BROADWELL(dev_priv)) > > - cdclk = bdw_calc_cdclk(max_pixclk); > > - else > > - cdclk = vlv_calc_cdclk(dev_priv, max_pixclk); > > + intel_calc_cdclk(intel_state, max_pixclk, &cdclk, NULL); > > > > if (cdclk > dev_priv->max_cdclk_freq) { > > DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds > > max (%d kHz)\n", > > @@ -1521,11 +1555,7 @@ static int vlv_modeset_calc_cdclk(struct > > drm_atomic_state *state) > > intel_state->cdclk.logical.cdclk = cdclk; > > > > if (!intel_state->active_crtcs) { > > - if (IS_BROADWELL(dev_priv)) > > - cdclk = bdw_calc_cdclk(0); > > - else > > - cdclk = vlv_calc_cdclk(dev_priv, 0); > > - > > + intel_calc_cdclk(intel_state, 0, &cdclk, NULL); > > intel_state->cdclk.actual.cdclk = cdclk; > > } else { > > intel_state->cdclk.actual = intel_state- > > >cdclk.logical; > > @@ -1541,22 +1571,7 @@ static int skl_modeset_calc_cdclk(struct > > drm_atomic_state *state) > > int max_pixclk = intel_max_pixel_rate(state); > > int cdclk, vco; > > > > - /* > > - * FIXME: Skylake/Kabylake should also account for plane > > ratio once > > - * 64bpp pixel formats are supported. > > - */ > > - if (IS_GEMINILAKE(dev_priv)) { > > - cdclk = glk_calc_cdclk(max_pixclk); > > - vco = glk_de_pll_vco(dev_priv, cdclk); > > - } else if (IS_BROXTON(dev_priv)) { > > - cdclk = bxt_calc_cdclk(max_pixclk); > > - vco = bxt_de_pll_vco(dev_priv, cdclk); > > - } else { > > - vco = intel_state->cdclk.logical.vco; > > - if (!vco) > > - vco = dev_priv->skl_preferred_vco_freq; > > - cdclk = skl_calc_cdclk(max_pixclk, vco); > > - } > > + intel_calc_cdclk(intel_state, max_pixclk, &cdclk, &vco); > > > > if (cdclk > dev_priv->max_cdclk_freq) { > > DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds > > max (%d kHz)\n", > > @@ -1568,16 +1583,7 @@ static int skl_modeset_calc_cdclk(struct > > drm_atomic_state *state) > > intel_state->cdclk.logical.cdclk = cdclk; > > > > if (!intel_state->active_crtcs) { > > - if (IS_GEMINILAKE(dev_priv)) { > > - cdclk = glk_calc_cdclk(0); > > - vco = glk_de_pll_vco(dev_priv, cdclk); > > - } else if (IS_BROXTON(dev_priv)) { > > - cdclk = bxt_calc_cdclk(0); > > - vco = bxt_de_pll_vco(dev_priv, cdclk); > > - } else { > > - cdclk = skl_calc_cdclk(0, vco); > > - } > > - > > + intel_calc_cdclk(intel_state, 0, &cdclk, &vco); > > intel_state->cdclk.actual.vco = vco; > > intel_state->cdclk.actual.cdclk = cdclk; > > } else { > > -- > > 2.7.4 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/4] drm/i915: add intel_calc_cdclk() 2017-02-17 20:37 ` Paulo Zanoni @ 2017-02-17 20:48 ` Ville Syrjälä 0 siblings, 0 replies; 14+ messages in thread From: Ville Syrjälä @ 2017-02-17 20:48 UTC (permalink / raw) To: Paulo Zanoni; +Cc: intel-gfx On Fri, Feb 17, 2017 at 06:37:23PM -0200, Paulo Zanoni wrote: > Em Sex, 2017-02-17 às 15:49 +0200, Ville Syrjälä escreveu: > > On Fri, Feb 17, 2017 at 11:22:05AM -0200, Paulo Zanoni wrote: > > > > > > Each x_modeset_calc_cdclk() has to do the same platform checks > > > twice, > > > so extract them to a single function. This way, the platform checks > > > are all in the same place, and the platform-common code gets rid of > > > all the platform-specific checks, which IMHO makes the code easier > > > to > > > read. > > > > > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_cdclk.c | 84 ++++++++++++++++++++---- > > > -------------- > > > 1 file changed, 45 insertions(+), 39 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_cdclk.c > > > b/drivers/gpu/drm/i915/intel_cdclk.c > > > index d505ff1..6efc5f4 100644 > > > --- a/drivers/gpu/drm/i915/intel_cdclk.c > > > +++ b/drivers/gpu/drm/i915/intel_cdclk.c > > > @@ -1496,6 +1496,47 @@ static int intel_max_pixel_rate(struct > > > drm_atomic_state *state) > > > return max_pixel_rate; > > > } > > > > > > +static void intel_calc_cdclk(struct intel_atomic_state *state, int > > > max_pixclk, > > > + int *cdclk, int *vco) > > > +{ > > > + struct drm_i915_private *dev_priv = to_i915(state- > > > >base.dev); > > > + > > > + switch (INTEL_INFO(dev_priv)->platform) { > > > + case INTEL_VALLEYVIEW: > > > + case INTEL_CHERRYVIEW: > > > + *cdclk = vlv_calc_cdclk(dev_priv, max_pixclk); > > > + break; > > > + case INTEL_BROADWELL: > > > + /* > > > + * FIXME: should also account for plane ratio once > > > 64bpp pixel > > > + * formats are supported. > > > + */ > > > + *cdclk = bdw_calc_cdclk(max_pixclk); > > > + break; > > > + case INTEL_SKYLAKE: > > > + case INTEL_KABYLAKE: > > > + /* > > > + * FIXME: should also account for plane ratio once > > > 64bpp pixel > > > + * formats are supported. > > > + */ > > > + *vco = state->cdclk.logical.vco; > > > + if (!*vco) > > > + *vco = dev_priv->skl_preferred_vco_freq; > > > + *cdclk = skl_calc_cdclk(max_pixclk, *vco); > > > + break; > > > + case INTEL_BROXTON: > > > + *cdclk = bxt_calc_cdclk(max_pixclk); > > > + *vco = bxt_de_pll_vco(dev_priv, *cdclk); > > > + break; > > > + case INTEL_GEMINILAKE: > > > + *cdclk = glk_calc_cdclk(max_pixclk); > > > + *vco = glk_de_pll_vco(dev_priv, *cdclk); > > > + break; > > > + default: > > > + MISSING_CASE(INTEL_INFO(dev_priv)->platform); > > > + } > > > +} > > > > How about just replacing the .modeset_calc_cdclk() vfunc with a > > slightly > > lower level vfunc that just computes the cdclk/vco/whatever without > > containing the active_crtcs logic? > > > > Then we should have just > > > > intel_modeset_calc_cdclk() > > { > > .calc_cdclk(logical, max_pixclk); > > > > /* > > * maybe keep the max_cdclk check here, although it that > > * happens I think we have a bug somewhere, so perhaps > > * just convert it into a WARN, or drop entirely. > > */ > > > > if (!active_crtcs) > > .calc_cdclk(actual, 0); > > else > > actual = logical; > > } > > Yeah, the code above is definitely a next step to the changes I did. > > I'm just not a big fan of the .calc_cdclk vfunc since it will be just 2 > lines for each platform. Unless I inline them with the *real* > x_calc_cdclk() funcs we have today, but then I'll have to check their > other callers. So I'll take a look and try to submit a new patch. At some point I had this idea of making the cdclk computation more data driven. As in we'd store the various possible cdclk steps, required guarbands etc. in some structure and thus avoid all the calc_cdclk() functions which are mostly just if ladders with slightly different numbers in them. But I never actually tried it, so not sure how pretty/ugly it would turn out to be. In the meantime, I think I'd prefer two line functions over the switch statement. For one, it would allow us to keep the calculation part right next to the other cdclk stuff for said platform. One thing that's a slight concern is the future dvfs stuff we talked about. I've not yet fully thought out where that needs to be done, but it might be that some of it might nicely land in these two line function (making them at least three lines ;). -- Ville Syrjälä Intel OTC _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 3/4] drm/i915: remove potentially confusing IS_G4X checks 2017-02-17 13:22 [PATCH 0/4] Small clocking code refactor Paulo Zanoni 2017-02-17 13:22 ` [PATCH 1/4] drm/i915: kill {bdw, bxt}_modeset_calc_cdclk Paulo Zanoni 2017-02-17 13:22 ` [PATCH 2/4] drm/i915: add intel_calc_cdclk() Paulo Zanoni @ 2017-02-17 13:22 ` Paulo Zanoni 2017-02-17 13:22 ` [PATCH 4/4] drm/i915: reorganize the get_cdclk assignment Paulo Zanoni 2017-02-17 15:22 ` ✓ Fi.CI.BAT: success for Small clocking code refactor Patchwork 4 siblings, 0 replies; 14+ messages in thread From: Paulo Zanoni @ 2017-02-17 13:22 UTC (permalink / raw) To: intel-gfx; +Cc: Paulo Zanoni The IS_G4X macro is defined as IS_G45 || IS_GM45. We have two points in our code where we have an if statement checking for GM45 followed by an else if statement checking for IS_G4X. This can be confusing since the IS_G4X check won't be catching the previously-checked GM45. Someone quickly trying to check which functions run on each platform may end up getting confused while reading the code. Fix the potential confusion by limiting the else if statements to only check for the platform that was not already checked earlier in the if ladder. Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> --- drivers/gpu/drm/i915/intel_cdclk.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c index 6efc5f4..7c92dc7 100644 --- a/drivers/gpu/drm/i915/intel_cdclk.c +++ b/drivers/gpu/drm/i915/intel_cdclk.c @@ -223,7 +223,7 @@ static unsigned int intel_hpll_vco(struct drm_i915_private *dev_priv) /* FIXME other chipsets? */ if (IS_GM45(dev_priv)) vco_table = ctg_vco; - else if (IS_G4X(dev_priv)) + else if (IS_G45(dev_priv)) vco_table = elk_vco; else if (IS_I965GM(dev_priv)) vco_table = cl_vco; @@ -1805,7 +1805,7 @@ void intel_init_cdclk_hooks(struct drm_i915_private *dev_priv) dev_priv->display.get_cdclk = fixed_450mhz_get_cdclk; else if (IS_GM45(dev_priv)) dev_priv->display.get_cdclk = gm45_get_cdclk; - else if (IS_G4X(dev_priv)) + else if (IS_G45(dev_priv)) dev_priv->display.get_cdclk = g33_get_cdclk; else if (IS_I965GM(dev_priv)) dev_priv->display.get_cdclk = i965gm_get_cdclk; -- 2.7.4 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 4/4] drm/i915: reorganize the get_cdclk assignment 2017-02-17 13:22 [PATCH 0/4] Small clocking code refactor Paulo Zanoni ` (2 preceding siblings ...) 2017-02-17 13:22 ` [PATCH 3/4] drm/i915: remove potentially confusing IS_G4X checks Paulo Zanoni @ 2017-02-17 13:22 ` Paulo Zanoni 2017-02-17 14:05 ` Ville Syrjälä 2017-02-17 15:22 ` ✓ Fi.CI.BAT: success for Small clocking code refactor Patchwork 4 siblings, 1 reply; 14+ messages in thread From: Paulo Zanoni @ 2017-02-17 13:22 UTC (permalink / raw) To: intel-gfx; +Cc: Paulo Zanoni Possible problems of the current if-ladder: - It's a huge if ladder with almost a different check for each of our platforms. - It mixes 3 different types of checks: IS_GENX, IS_PLATFORM and IS_GROUP_OF_PLATFORMS. - As demonstrated by the recent IS_G4X commit, it's not easy to be sure if a platform down on the list isn't also checked earlier. - As demonstrated by the WARN at the end, it's not easy to be sure if we're actually checking for every single platform. Possible advantages of the new switch statement: - It may be easier for the compiler to optimize stuff (I didn't check this), especially since the values are labels of an enum. - The compiler will tell us in case we miss some platform. - All platforms are explicitly there instead of maybe hidden in some check for a certain group of platforms such as IS_GEN9_BC. Possible disadvantages with the new code: - A few lines bigger. Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> --- drivers/gpu/drm/i915/intel_cdclk.c | 103 ++++++++++++++++++++++--------------- 1 file changed, 62 insertions(+), 41 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c index 7c92dc7..58a2f5c 100644 --- a/drivers/gpu/drm/i915/intel_cdclk.c +++ b/drivers/gpu/drm/i915/intel_cdclk.c @@ -1789,49 +1789,70 @@ void intel_init_cdclk_hooks(struct drm_i915_private *dev_priv) dev_priv->display.modeset_calc_cdclk = skl_modeset_calc_cdclk; } - if (IS_GEN9_BC(dev_priv)) - dev_priv->display.get_cdclk = skl_get_cdclk; - else if (IS_GEN9_LP(dev_priv)) - dev_priv->display.get_cdclk = bxt_get_cdclk; - else if (IS_BROADWELL(dev_priv)) - dev_priv->display.get_cdclk = bdw_get_cdclk; - else if (IS_HASWELL(dev_priv)) - dev_priv->display.get_cdclk = hsw_get_cdclk; - else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) - dev_priv->display.get_cdclk = vlv_get_cdclk; - else if (IS_GEN6(dev_priv) || IS_IVYBRIDGE(dev_priv)) + switch (INTEL_INFO(dev_priv)->platform) { + case INTEL_PLATFORM_UNINITIALIZED: + MISSING_CASE(INTEL_INFO(dev_priv)->platform); + /* Fall through. */ + case INTEL_I830: + dev_priv->display.get_cdclk = fixed_133mhz_get_cdclk; + break; + case INTEL_I845G: + dev_priv->display.get_cdclk = fixed_200mhz_get_cdclk; + break; + case INTEL_I85X: + dev_priv->display.get_cdclk = i85x_get_cdclk; + break; + case INTEL_I865G: + dev_priv->display.get_cdclk = fixed_266mhz_get_cdclk; + break; + case INTEL_I915G: + dev_priv->display.get_cdclk = fixed_333mhz_get_cdclk; + break; + case INTEL_I915GM: + dev_priv->display.get_cdclk = i915gm_get_cdclk; + break; + case INTEL_I945G: + case INTEL_I965G: + case INTEL_SANDYBRIDGE: + case INTEL_IVYBRIDGE: dev_priv->display.get_cdclk = fixed_400mhz_get_cdclk; - else if (IS_GEN5(dev_priv)) - dev_priv->display.get_cdclk = fixed_450mhz_get_cdclk; - else if (IS_GM45(dev_priv)) - dev_priv->display.get_cdclk = gm45_get_cdclk; - else if (IS_G45(dev_priv)) + break; + case INTEL_I945GM: + dev_priv->display.get_cdclk = i945gm_get_cdclk; + break; + case INTEL_G33: + case INTEL_G45: dev_priv->display.get_cdclk = g33_get_cdclk; - else if (IS_I965GM(dev_priv)) - dev_priv->display.get_cdclk = i965gm_get_cdclk; - else if (IS_I965G(dev_priv)) - dev_priv->display.get_cdclk = fixed_400mhz_get_cdclk; - else if (IS_PINEVIEW(dev_priv)) + break; + case INTEL_PINEVIEW: dev_priv->display.get_cdclk = pnv_get_cdclk; - else if (IS_G33(dev_priv)) - dev_priv->display.get_cdclk = g33_get_cdclk; - else if (IS_I945GM(dev_priv)) - dev_priv->display.get_cdclk = i945gm_get_cdclk; - else if (IS_I945G(dev_priv)) - dev_priv->display.get_cdclk = fixed_400mhz_get_cdclk; - else if (IS_I915GM(dev_priv)) - dev_priv->display.get_cdclk = i915gm_get_cdclk; - else if (IS_I915G(dev_priv)) - dev_priv->display.get_cdclk = fixed_333mhz_get_cdclk; - else if (IS_I865G(dev_priv)) - dev_priv->display.get_cdclk = fixed_266mhz_get_cdclk; - else if (IS_I85X(dev_priv)) - dev_priv->display.get_cdclk = i85x_get_cdclk; - else if (IS_I845G(dev_priv)) - dev_priv->display.get_cdclk = fixed_200mhz_get_cdclk; - else { /* 830 */ - WARN(!IS_I830(dev_priv), - "Unknown platform. Assuming 133 MHz CDCLK\n"); - dev_priv->display.get_cdclk = fixed_133mhz_get_cdclk; + break; + case INTEL_I965GM: + dev_priv->display.get_cdclk = i965gm_get_cdclk; + break; + case INTEL_GM45: + dev_priv->display.get_cdclk = gm45_get_cdclk; + break; + case INTEL_IRONLAKE: + dev_priv->display.get_cdclk = fixed_450mhz_get_cdclk; + break; + case INTEL_VALLEYVIEW: + case INTEL_CHERRYVIEW: + dev_priv->display.get_cdclk = vlv_get_cdclk; + break; + case INTEL_HASWELL: + dev_priv->display.get_cdclk = hsw_get_cdclk; + break; + case INTEL_BROADWELL: + dev_priv->display.get_cdclk = bdw_get_cdclk; + break; + case INTEL_SKYLAKE: + case INTEL_KABYLAKE: + dev_priv->display.get_cdclk = skl_get_cdclk; + break; + case INTEL_BROXTON: + case INTEL_GEMINILAKE: + dev_priv->display.get_cdclk = bxt_get_cdclk; + break; } } -- 2.7.4 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 4/4] drm/i915: reorganize the get_cdclk assignment 2017-02-17 13:22 ` [PATCH 4/4] drm/i915: reorganize the get_cdclk assignment Paulo Zanoni @ 2017-02-17 14:05 ` Ville Syrjälä 2017-02-17 15:17 ` Paulo Zanoni 0 siblings, 1 reply; 14+ messages in thread From: Ville Syrjälä @ 2017-02-17 14:05 UTC (permalink / raw) To: Paulo Zanoni; +Cc: intel-gfx On Fri, Feb 17, 2017 at 11:22:07AM -0200, Paulo Zanoni wrote: > Possible problems of the current if-ladder: > - It's a huge if ladder with almost a different check for each of > our platforms. > - It mixes 3 different types of checks: IS_GENX, IS_PLATFORM and > IS_GROUP_OF_PLATFORMS. > - As demonstrated by the recent IS_G4X commit, it's not easy to be > sure if a platform down on the list isn't also checked earlier. > - As demonstrated by the WARN at the end, it's not easy to be sure > if we're actually checking for every single platform. > > Possible advantages of the new switch statement: > - It may be easier for the compiler to optimize stuff (I didn't > check this), especially since the values are labels of an enum. > - The compiler will tell us in case we miss some platform. > - All platforms are explicitly there instead of maybe hidden in some > check for a certain group of platforms such as IS_GEN9_BC. Performance is a bit of a moot point since this is run exaclty once, but the IS_GEN9_BC() stuff I tend to agree with. I don't really like those macros at all since they don't actully mean anything as far as the hardware features go. > > Possible disadvantages with the new code: > - A few lines bigger. > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > --- > drivers/gpu/drm/i915/intel_cdclk.c | 103 ++++++++++++++++++++++--------------- > 1 file changed, 62 insertions(+), 41 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c > index 7c92dc7..58a2f5c 100644 > --- a/drivers/gpu/drm/i915/intel_cdclk.c > +++ b/drivers/gpu/drm/i915/intel_cdclk.c > @@ -1789,49 +1789,70 @@ void intel_init_cdclk_hooks(struct drm_i915_private *dev_priv) > dev_priv->display.modeset_calc_cdclk = skl_modeset_calc_cdclk; > } > > - if (IS_GEN9_BC(dev_priv)) > - dev_priv->display.get_cdclk = skl_get_cdclk; > - else if (IS_GEN9_LP(dev_priv)) > - dev_priv->display.get_cdclk = bxt_get_cdclk; > - else if (IS_BROADWELL(dev_priv)) > - dev_priv->display.get_cdclk = bdw_get_cdclk; > - else if (IS_HASWELL(dev_priv)) > - dev_priv->display.get_cdclk = hsw_get_cdclk; > - else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) > - dev_priv->display.get_cdclk = vlv_get_cdclk; > - else if (IS_GEN6(dev_priv) || IS_IVYBRIDGE(dev_priv)) > + switch (INTEL_INFO(dev_priv)->platform) { > + case INTEL_PLATFORM_UNINITIALIZED: Just default: ? > + MISSING_CASE(INTEL_INFO(dev_priv)->platform); > + /* Fall through. */ > + case INTEL_I830: > + dev_priv->display.get_cdclk = fixed_133mhz_get_cdclk; > + break; > + case INTEL_I845G: > + dev_priv->display.get_cdclk = fixed_200mhz_get_cdclk; > + break; > + case INTEL_I85X: > + dev_priv->display.get_cdclk = i85x_get_cdclk; > + break; > + case INTEL_I865G: > + dev_priv->display.get_cdclk = fixed_266mhz_get_cdclk; > + break; > + case INTEL_I915G: > + dev_priv->display.get_cdclk = fixed_333mhz_get_cdclk; > + break; > + case INTEL_I915GM: > + dev_priv->display.get_cdclk = i915gm_get_cdclk; > + break; > + case INTEL_I945G: > + case INTEL_I965G: > + case INTEL_SANDYBRIDGE: > + case INTEL_IVYBRIDGE: I don't particularly like this disorder. I just managed to get the list into some sort of sane order recently. > dev_priv->display.get_cdclk = fixed_400mhz_get_cdclk; > - else if (IS_GEN5(dev_priv)) > - dev_priv->display.get_cdclk = fixed_450mhz_get_cdclk; > - else if (IS_GM45(dev_priv)) > - dev_priv->display.get_cdclk = gm45_get_cdclk; > - else if (IS_G45(dev_priv)) > + break; > + case INTEL_I945GM: > + dev_priv->display.get_cdclk = i945gm_get_cdclk; > + break; > + case INTEL_G33: > + case INTEL_G45: More disorder. > dev_priv->display.get_cdclk = g33_get_cdclk; > - else if (IS_I965GM(dev_priv)) > - dev_priv->display.get_cdclk = i965gm_get_cdclk; > - else if (IS_I965G(dev_priv)) > - dev_priv->display.get_cdclk = fixed_400mhz_get_cdclk; > - else if (IS_PINEVIEW(dev_priv)) > + break; > + case INTEL_PINEVIEW: > dev_priv->display.get_cdclk = pnv_get_cdclk; > - else if (IS_G33(dev_priv)) > - dev_priv->display.get_cdclk = g33_get_cdclk; > - else if (IS_I945GM(dev_priv)) > - dev_priv->display.get_cdclk = i945gm_get_cdclk; > - else if (IS_I945G(dev_priv)) > - dev_priv->display.get_cdclk = fixed_400mhz_get_cdclk; > - else if (IS_I915GM(dev_priv)) > - dev_priv->display.get_cdclk = i915gm_get_cdclk; > - else if (IS_I915G(dev_priv)) > - dev_priv->display.get_cdclk = fixed_333mhz_get_cdclk; > - else if (IS_I865G(dev_priv)) > - dev_priv->display.get_cdclk = fixed_266mhz_get_cdclk; > - else if (IS_I85X(dev_priv)) > - dev_priv->display.get_cdclk = i85x_get_cdclk; > - else if (IS_I845G(dev_priv)) > - dev_priv->display.get_cdclk = fixed_200mhz_get_cdclk; > - else { /* 830 */ > - WARN(!IS_I830(dev_priv), > - "Unknown platform. Assuming 133 MHz CDCLK\n"); > - dev_priv->display.get_cdclk = fixed_133mhz_get_cdclk; > + break; > + case INTEL_I965GM: > + dev_priv->display.get_cdclk = i965gm_get_cdclk; > + break; > + case INTEL_GM45: > + dev_priv->display.get_cdclk = gm45_get_cdclk; > + break; > + case INTEL_IRONLAKE: > + dev_priv->display.get_cdclk = fixed_450mhz_get_cdclk; > + break; > + case INTEL_VALLEYVIEW: > + case INTEL_CHERRYVIEW: > + dev_priv->display.get_cdclk = vlv_get_cdclk; > + break; > + case INTEL_HASWELL: > + dev_priv->display.get_cdclk = hsw_get_cdclk; > + break; > + case INTEL_BROADWELL: > + dev_priv->display.get_cdclk = bdw_get_cdclk; > + break; > + case INTEL_SKYLAKE: > + case INTEL_KABYLAKE: > + dev_priv->display.get_cdclk = skl_get_cdclk; > + break; > + case INTEL_BROXTON: > + case INTEL_GEMINILAKE: > + dev_priv->display.get_cdclk = bxt_get_cdclk; > + break; > } > } > -- > 2.7.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ville Syrjälä Intel OTC _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 4/4] drm/i915: reorganize the get_cdclk assignment 2017-02-17 14:05 ` Ville Syrjälä @ 2017-02-17 15:17 ` Paulo Zanoni 2017-02-17 15:28 ` Ville Syrjälä 0 siblings, 1 reply; 14+ messages in thread From: Paulo Zanoni @ 2017-02-17 15:17 UTC (permalink / raw) To: Ville Syrjälä; +Cc: intel-gfx Em Sex, 2017-02-17 às 16:05 +0200, Ville Syrjälä escreveu: > On Fri, Feb 17, 2017 at 11:22:07AM -0200, Paulo Zanoni wrote: > > > > Possible problems of the current if-ladder: > > - It's a huge if ladder with almost a different check for each of > > our platforms. > > - It mixes 3 different types of checks: IS_GENX, IS_PLATFORM and > > IS_GROUP_OF_PLATFORMS. > > - As demonstrated by the recent IS_G4X commit, it's not easy to > > be > > sure if a platform down on the list isn't also checked earlier. > > - As demonstrated by the WARN at the end, it's not easy to be > > sure > > if we're actually checking for every single platform. > > > > Possible advantages of the new switch statement: > > - It may be easier for the compiler to optimize stuff (I didn't > > check this), especially since the values are labels of an enum. > > - The compiler will tell us in case we miss some platform. > > - All platforms are explicitly there instead of maybe hidden in > > some > > check for a certain group of platforms such as IS_GEN9_BC. > > Performance is a bit of a moot point since this is run exaclty once, > but > the IS_GEN9_BC() stuff I tend to agree with. I don't really like > those > macros at all since they don't actully mean anything as far as the > hardware features go. I think they make some sense when they're a single check. But when we have tons of checks for tons of platforms, I don't know. > > > > > > > Possible disadvantages with the new code: > > - A few lines bigger. > > > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > --- > > drivers/gpu/drm/i915/intel_cdclk.c | 103 ++++++++++++++++++++++--- > > ------------ > > 1 file changed, 62 insertions(+), 41 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_cdclk.c > > b/drivers/gpu/drm/i915/intel_cdclk.c > > index 7c92dc7..58a2f5c 100644 > > --- a/drivers/gpu/drm/i915/intel_cdclk.c > > +++ b/drivers/gpu/drm/i915/intel_cdclk.c > > @@ -1789,49 +1789,70 @@ void intel_init_cdclk_hooks(struct > > drm_i915_private *dev_priv) > > dev_priv->display.modeset_calc_cdclk = > > skl_modeset_calc_cdclk; > > } > > > > - if (IS_GEN9_BC(dev_priv)) > > - dev_priv->display.get_cdclk = skl_get_cdclk; > > - else if (IS_GEN9_LP(dev_priv)) > > - dev_priv->display.get_cdclk = bxt_get_cdclk; > > - else if (IS_BROADWELL(dev_priv)) > > - dev_priv->display.get_cdclk = bdw_get_cdclk; > > - else if (IS_HASWELL(dev_priv)) > > - dev_priv->display.get_cdclk = hsw_get_cdclk; > > - else if (IS_VALLEYVIEW(dev_priv) || > > IS_CHERRYVIEW(dev_priv)) > > - dev_priv->display.get_cdclk = vlv_get_cdclk; > > - else if (IS_GEN6(dev_priv) || IS_IVYBRIDGE(dev_priv)) > > + switch (INTEL_INFO(dev_priv)->platform) { > > + case INTEL_PLATFORM_UNINITIALIZED: > > Just default: ? If we add a default case the compiler will stop complaining in case we don't explicitly list every platform. It's a trade-off, I really think the current way is slightly better, but I won't oppose in case you still think it's better adding the default case. > > > > > + MISSING_CASE(INTEL_INFO(dev_priv)->platform); > > + /* Fall through. */ > > + case INTEL_I830: > > + dev_priv->display.get_cdclk = > > fixed_133mhz_get_cdclk; > > + break; > > + case INTEL_I845G: > > + dev_priv->display.get_cdclk = > > fixed_200mhz_get_cdclk; > > + break; > > + case INTEL_I85X: > > + dev_priv->display.get_cdclk = i85x_get_cdclk; > > + break; > > + case INTEL_I865G: > > + dev_priv->display.get_cdclk = > > fixed_266mhz_get_cdclk; > > + break; > > + case INTEL_I915G: > > + dev_priv->display.get_cdclk = > > fixed_333mhz_get_cdclk; > > + break; > > + case INTEL_I915GM: > > + dev_priv->display.get_cdclk = i915gm_get_cdclk; > > + break; > > + case INTEL_I945G: > > + case INTEL_I965G: > > + case INTEL_SANDYBRIDGE: > > + case INTEL_IVYBRIDGE: > > I don't particularly like this disorder. I just managed to get the > list into some sort of sane order recently. My original thought here was that since the compiler will actually complain in case we miss some platform, keeping a strict order is not as meaningful as it was before. But I was also wondering if this was actually better or not, so I can change this. But I did notice you sorted the list. In fact, I originally wrote this commit against a tree without your improvements, so one of the reasons I cited in the commit message was the mess of an ordering we had at that time :). > > > > > dev_priv->display.get_cdclk = > > fixed_400mhz_get_cdclk; > > - else if (IS_GEN5(dev_priv)) > > - dev_priv->display.get_cdclk = > > fixed_450mhz_get_cdclk; > > - else if (IS_GM45(dev_priv)) > > - dev_priv->display.get_cdclk = gm45_get_cdclk; > > - else if (IS_G45(dev_priv)) > > + break; > > + case INTEL_I945GM: > > + dev_priv->display.get_cdclk = i945gm_get_cdclk; > > + break; > > + case INTEL_G33: > > + case INTEL_G45: > > More disorder. > > > > > dev_priv->display.get_cdclk = g33_get_cdclk; > > - else if (IS_I965GM(dev_priv)) > > - dev_priv->display.get_cdclk = i965gm_get_cdclk; > > - else if (IS_I965G(dev_priv)) > > - dev_priv->display.get_cdclk = > > fixed_400mhz_get_cdclk; > > - else if (IS_PINEVIEW(dev_priv)) > > + break; > > + case INTEL_PINEVIEW: > > dev_priv->display.get_cdclk = pnv_get_cdclk; > > - else if (IS_G33(dev_priv)) > > - dev_priv->display.get_cdclk = g33_get_cdclk; > > - else if (IS_I945GM(dev_priv)) > > - dev_priv->display.get_cdclk = i945gm_get_cdclk; > > - else if (IS_I945G(dev_priv)) > > - dev_priv->display.get_cdclk = > > fixed_400mhz_get_cdclk; > > - else if (IS_I915GM(dev_priv)) > > - dev_priv->display.get_cdclk = i915gm_get_cdclk; > > - else if (IS_I915G(dev_priv)) > > - dev_priv->display.get_cdclk = > > fixed_333mhz_get_cdclk; > > - else if (IS_I865G(dev_priv)) > > - dev_priv->display.get_cdclk = > > fixed_266mhz_get_cdclk; > > - else if (IS_I85X(dev_priv)) > > - dev_priv->display.get_cdclk = i85x_get_cdclk; > > - else if (IS_I845G(dev_priv)) > > - dev_priv->display.get_cdclk = > > fixed_200mhz_get_cdclk; > > - else { /* 830 */ > > - WARN(!IS_I830(dev_priv), > > - "Unknown platform. Assuming 133 MHz > > CDCLK\n"); > > - dev_priv->display.get_cdclk = > > fixed_133mhz_get_cdclk; > > + break; > > + case INTEL_I965GM: > > + dev_priv->display.get_cdclk = i965gm_get_cdclk; > > + break; > > + case INTEL_GM45: > > + dev_priv->display.get_cdclk = gm45_get_cdclk; > > + break; > > + case INTEL_IRONLAKE: > > + dev_priv->display.get_cdclk = > > fixed_450mhz_get_cdclk; > > + break; > > + case INTEL_VALLEYVIEW: > > + case INTEL_CHERRYVIEW: > > + dev_priv->display.get_cdclk = vlv_get_cdclk; > > + break; > > + case INTEL_HASWELL: > > + dev_priv->display.get_cdclk = hsw_get_cdclk; > > + break; > > + case INTEL_BROADWELL: > > + dev_priv->display.get_cdclk = bdw_get_cdclk; > > + break; > > + case INTEL_SKYLAKE: > > + case INTEL_KABYLAKE: > > + dev_priv->display.get_cdclk = skl_get_cdclk; > > + break; > > + case INTEL_BROXTON: > > + case INTEL_GEMINILAKE: > > + dev_priv->display.get_cdclk = bxt_get_cdclk; > > + break; > > } > > } > > -- > > 2.7.4 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 4/4] drm/i915: reorganize the get_cdclk assignment 2017-02-17 15:17 ` Paulo Zanoni @ 2017-02-17 15:28 ` Ville Syrjälä 0 siblings, 0 replies; 14+ messages in thread From: Ville Syrjälä @ 2017-02-17 15:28 UTC (permalink / raw) To: Paulo Zanoni; +Cc: intel-gfx On Fri, Feb 17, 2017 at 01:17:22PM -0200, Paulo Zanoni wrote: > Em Sex, 2017-02-17 às 16:05 +0200, Ville Syrjälä escreveu: > > On Fri, Feb 17, 2017 at 11:22:07AM -0200, Paulo Zanoni wrote: > > > > > > Possible problems of the current if-ladder: > > > - It's a huge if ladder with almost a different check for each of > > > our platforms. > > > - It mixes 3 different types of checks: IS_GENX, IS_PLATFORM and > > > IS_GROUP_OF_PLATFORMS. > > > - As demonstrated by the recent IS_G4X commit, it's not easy to > > > be > > > sure if a platform down on the list isn't also checked earlier. > > > - As demonstrated by the WARN at the end, it's not easy to be > > > sure > > > if we're actually checking for every single platform. > > > > > > Possible advantages of the new switch statement: > > > - It may be easier for the compiler to optimize stuff (I didn't > > > check this), especially since the values are labels of an enum. > > > - The compiler will tell us in case we miss some platform. > > > - All platforms are explicitly there instead of maybe hidden in > > > some > > > check for a certain group of platforms such as IS_GEN9_BC. > > > > Performance is a bit of a moot point since this is run exaclty once, > > but > > the IS_GEN9_BC() stuff I tend to agree with. I don't really like > > those > > macros at all since they don't actully mean anything as far as the > > hardware features go. > > I think they make some sense when they're a single check. But when we > have tons of checks for tons of platforms, I don't know. The problem problem is that they basically mean different things in different parts of the driver. So you anyway have to mentally expand the list out to figure out what's really going on. > > > > > > > > > > > > Possible disadvantages with the new code: > > > - A few lines bigger. > > > > > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_cdclk.c | 103 ++++++++++++++++++++++--- > > > ------------ > > > 1 file changed, 62 insertions(+), 41 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_cdclk.c > > > b/drivers/gpu/drm/i915/intel_cdclk.c > > > index 7c92dc7..58a2f5c 100644 > > > --- a/drivers/gpu/drm/i915/intel_cdclk.c > > > +++ b/drivers/gpu/drm/i915/intel_cdclk.c > > > @@ -1789,49 +1789,70 @@ void intel_init_cdclk_hooks(struct > > > drm_i915_private *dev_priv) > > > dev_priv->display.modeset_calc_cdclk = > > > skl_modeset_calc_cdclk; > > > } > > > > > > - if (IS_GEN9_BC(dev_priv)) > > > - dev_priv->display.get_cdclk = skl_get_cdclk; > > > - else if (IS_GEN9_LP(dev_priv)) > > > - dev_priv->display.get_cdclk = bxt_get_cdclk; > > > - else if (IS_BROADWELL(dev_priv)) > > > - dev_priv->display.get_cdclk = bdw_get_cdclk; > > > - else if (IS_HASWELL(dev_priv)) > > > - dev_priv->display.get_cdclk = hsw_get_cdclk; > > > - else if (IS_VALLEYVIEW(dev_priv) || > > > IS_CHERRYVIEW(dev_priv)) > > > - dev_priv->display.get_cdclk = vlv_get_cdclk; > > > - else if (IS_GEN6(dev_priv) || IS_IVYBRIDGE(dev_priv)) > > > + switch (INTEL_INFO(dev_priv)->platform) { > > > + case INTEL_PLATFORM_UNINITIALIZED: > > > > Just default: ? > > If we add a default case the compiler will stop complaining in case we > don't explicitly list every platform. It's a trade-off, I really think > the current way is slightly better, but I won't oppose in case you > still think it's better adding the default case. Nah. Getting the compiler to do the work for us seems like a decent idea. > > > > > > > > > > + MISSING_CASE(INTEL_INFO(dev_priv)->platform); > > > + /* Fall through. */ > > > + case INTEL_I830: > > > + dev_priv->display.get_cdclk = > > > fixed_133mhz_get_cdclk; > > > + break; > > > + case INTEL_I845G: > > > + dev_priv->display.get_cdclk = > > > fixed_200mhz_get_cdclk; > > > + break; > > > + case INTEL_I85X: > > > + dev_priv->display.get_cdclk = i85x_get_cdclk; > > > + break; > > > + case INTEL_I865G: > > > + dev_priv->display.get_cdclk = > > > fixed_266mhz_get_cdclk; > > > + break; > > > + case INTEL_I915G: > > > + dev_priv->display.get_cdclk = > > > fixed_333mhz_get_cdclk; > > > + break; > > > + case INTEL_I915GM: > > > + dev_priv->display.get_cdclk = i915gm_get_cdclk; > > > + break; > > > + case INTEL_I945G: > > > + case INTEL_I965G: > > > + case INTEL_SANDYBRIDGE: > > > + case INTEL_IVYBRIDGE: > > > > I don't particularly like this disorder. I just managed to get the > > list into some sort of sane order recently. > > My original thought here was that since the compiler will actually > complain in case we miss some platform, keeping a strict order is not > as meaningful as it was before. But I was also wondering if this was > actually better or not, so I can change this. And unsorted list is quite hard to verify visually for correctness. We might have a function pointer assigned for each platform, but if you actually want to verify that it's the correct one in each case then going through the list is IMO much easier when it's in a decent order. > > But I did notice you sorted the list. In fact, I originally wrote this > commit against a tree without your improvements, so one of the reasons > I cited in the commit message was the mess of an ordering we had at > that time :). > > > > > > > > > dev_priv->display.get_cdclk = > > > fixed_400mhz_get_cdclk; > > > - else if (IS_GEN5(dev_priv)) > > > - dev_priv->display.get_cdclk = > > > fixed_450mhz_get_cdclk; > > > - else if (IS_GM45(dev_priv)) > > > - dev_priv->display.get_cdclk = gm45_get_cdclk; > > > - else if (IS_G45(dev_priv)) > > > + break; > > > + case INTEL_I945GM: > > > + dev_priv->display.get_cdclk = i945gm_get_cdclk; > > > + break; > > > + case INTEL_G33: > > > + case INTEL_G45: > > > > More disorder. > > > > > > > > dev_priv->display.get_cdclk = g33_get_cdclk; > > > - else if (IS_I965GM(dev_priv)) > > > - dev_priv->display.get_cdclk = i965gm_get_cdclk; > > > - else if (IS_I965G(dev_priv)) > > > - dev_priv->display.get_cdclk = > > > fixed_400mhz_get_cdclk; > > > - else if (IS_PINEVIEW(dev_priv)) > > > + break; > > > + case INTEL_PINEVIEW: > > > dev_priv->display.get_cdclk = pnv_get_cdclk; > > > - else if (IS_G33(dev_priv)) > > > - dev_priv->display.get_cdclk = g33_get_cdclk; > > > - else if (IS_I945GM(dev_priv)) > > > - dev_priv->display.get_cdclk = i945gm_get_cdclk; > > > - else if (IS_I945G(dev_priv)) > > > - dev_priv->display.get_cdclk = > > > fixed_400mhz_get_cdclk; > > > - else if (IS_I915GM(dev_priv)) > > > - dev_priv->display.get_cdclk = i915gm_get_cdclk; > > > - else if (IS_I915G(dev_priv)) > > > - dev_priv->display.get_cdclk = > > > fixed_333mhz_get_cdclk; > > > - else if (IS_I865G(dev_priv)) > > > - dev_priv->display.get_cdclk = > > > fixed_266mhz_get_cdclk; > > > - else if (IS_I85X(dev_priv)) > > > - dev_priv->display.get_cdclk = i85x_get_cdclk; > > > - else if (IS_I845G(dev_priv)) > > > - dev_priv->display.get_cdclk = > > > fixed_200mhz_get_cdclk; > > > - else { /* 830 */ > > > - WARN(!IS_I830(dev_priv), > > > - "Unknown platform. Assuming 133 MHz > > > CDCLK\n"); > > > - dev_priv->display.get_cdclk = > > > fixed_133mhz_get_cdclk; > > > + break; > > > + case INTEL_I965GM: > > > + dev_priv->display.get_cdclk = i965gm_get_cdclk; > > > + break; > > > + case INTEL_GM45: > > > + dev_priv->display.get_cdclk = gm45_get_cdclk; > > > + break; > > > + case INTEL_IRONLAKE: > > > + dev_priv->display.get_cdclk = > > > fixed_450mhz_get_cdclk; > > > + break; > > > + case INTEL_VALLEYVIEW: > > > + case INTEL_CHERRYVIEW: > > > + dev_priv->display.get_cdclk = vlv_get_cdclk; > > > + break; > > > + case INTEL_HASWELL: > > > + dev_priv->display.get_cdclk = hsw_get_cdclk; > > > + break; > > > + case INTEL_BROADWELL: > > > + dev_priv->display.get_cdclk = bdw_get_cdclk; > > > + break; > > > + case INTEL_SKYLAKE: > > > + case INTEL_KABYLAKE: > > > + dev_priv->display.get_cdclk = skl_get_cdclk; > > > + break; > > > + case INTEL_BROXTON: > > > + case INTEL_GEMINILAKE: > > > + dev_priv->display.get_cdclk = bxt_get_cdclk; > > > + break; > > > } > > > } > > > -- > > > 2.7.4 > > > > > > _______________________________________________ > > > Intel-gfx mailing list > > > Intel-gfx@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- Ville Syrjälä Intel OTC _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 14+ messages in thread
* ✓ Fi.CI.BAT: success for Small clocking code refactor 2017-02-17 13:22 [PATCH 0/4] Small clocking code refactor Paulo Zanoni ` (3 preceding siblings ...) 2017-02-17 13:22 ` [PATCH 4/4] drm/i915: reorganize the get_cdclk assignment Paulo Zanoni @ 2017-02-17 15:22 ` Patchwork 4 siblings, 0 replies; 14+ messages in thread From: Patchwork @ 2017-02-17 15:22 UTC (permalink / raw) To: Paulo Zanoni; +Cc: intel-gfx == Series Details == Series: Small clocking code refactor URL : https://patchwork.freedesktop.org/series/19840/ State : success == Summary == Series 19840v1 Small clocking code refactor https://patchwork.freedesktop.org/api/1.0/series/19840/revisions/1/mbox/ fi-bdw-5557u total:252 pass:241 dwarn:0 dfail:0 fail:0 skip:11 fi-bsw-n3050 total:252 pass:213 dwarn:0 dfail:0 fail:0 skip:39 fi-bxt-j4205 total:252 pass:233 dwarn:0 dfail:0 fail:0 skip:19 fi-bxt-t5700 total:83 pass:70 dwarn:0 dfail:0 fail:0 skip:12 fi-byt-j1900 total:252 pass:225 dwarn:0 dfail:0 fail:0 skip:27 fi-byt-n2820 total:252 pass:221 dwarn:0 dfail:0 fail:0 skip:31 fi-hsw-4770 total:252 pass:236 dwarn:0 dfail:0 fail:0 skip:16 fi-hsw-4770r total:252 pass:236 dwarn:0 dfail:0 fail:0 skip:16 fi-ilk-650 total:252 pass:202 dwarn:0 dfail:0 fail:0 skip:50 fi-ivb-3520m total:252 pass:234 dwarn:0 dfail:0 fail:0 skip:18 fi-ivb-3770 total:252 pass:234 dwarn:0 dfail:0 fail:0 skip:18 fi-kbl-7500u total:252 pass:234 dwarn:0 dfail:0 fail:0 skip:18 fi-skl-6260u total:252 pass:242 dwarn:0 dfail:0 fail:0 skip:10 fi-skl-6700hq total:252 pass:235 dwarn:0 dfail:0 fail:0 skip:17 fi-skl-6700k total:252 pass:230 dwarn:4 dfail:0 fail:0 skip:18 fi-skl-6770hq total:252 pass:242 dwarn:0 dfail:0 fail:0 skip:10 fi-snb-2520m total:252 pass:224 dwarn:0 dfail:0 fail:0 skip:28 fi-snb-2600 total:252 pass:223 dwarn:0 dfail:0 fail:0 skip:29 2b7ce9512d9770350bc2a59652cc7bf469bc544a drm-tip: 2017y-02m-17d-12h-20m-31s UTC integration manifest 2d2db47 drm/i915: reorganize the get_cdclk assignment e370ecb drm/i915: remove potentially confusing IS_G4X checks 78520fc drm/i915: add intel_calc_cdclk() c82d3ef drm/i915: kill {bdw, bxt}_modeset_calc_cdclk == Logs == For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_3880/ _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2017-02-18 15:13 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-02-17 13:22 [PATCH 0/4] Small clocking code refactor Paulo Zanoni 2017-02-17 13:22 ` [PATCH 1/4] drm/i915: kill {bdw, bxt}_modeset_calc_cdclk Paulo Zanoni 2017-02-17 13:51 ` Ville Syrjälä 2017-02-18 15:13 ` David Weinehall 2017-02-17 13:22 ` [PATCH 2/4] drm/i915: add intel_calc_cdclk() Paulo Zanoni 2017-02-17 13:49 ` Ville Syrjälä 2017-02-17 20:37 ` Paulo Zanoni 2017-02-17 20:48 ` Ville Syrjälä 2017-02-17 13:22 ` [PATCH 3/4] drm/i915: remove potentially confusing IS_G4X checks Paulo Zanoni 2017-02-17 13:22 ` [PATCH 4/4] drm/i915: reorganize the get_cdclk assignment Paulo Zanoni 2017-02-17 14:05 ` Ville Syrjälä 2017-02-17 15:17 ` Paulo Zanoni 2017-02-17 15:28 ` Ville Syrjälä 2017-02-17 15:22 ` ✓ Fi.CI.BAT: success for Small clocking code refactor Patchwork
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.