| Message ID | 1457945747-2161-4-git-send-email-ander.conselvan.de.oliveira@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Op 14-03-16 om 09:55 schreef Ander Conselvan de Oliveira: > The funcion intel_ironlake_limit() is only called by the crtc compute > clock path. By merging it into ironlake_compute_clocks(), the code gets > clearer, since there's no more if-ladders to follow. > > Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 56 +++++++++++++++--------------------- > 1 file changed, 23 insertions(+), 33 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 07b5244..ea71430 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -566,30 +566,6 @@ static bool intel_pipe_will_have_type(const struct intel_crtc_state *crtc_state, > } > > static const intel_limit_t * > -intel_ironlake_limit(struct intel_crtc_state *crtc_state, int refclk) > -{ > - struct drm_device *dev = crtc_state->base.crtc->dev; > - const intel_limit_t *limit; > - > - if (intel_pipe_will_have_type(crtc_state, INTEL_OUTPUT_LVDS)) { > - if (intel_is_dual_link_lvds(dev)) { > - if (refclk == 100000) > - limit = &intel_limits_ironlake_dual_lvds_100m; > - else > - limit = &intel_limits_ironlake_dual_lvds; > - } else { > - if (refclk == 100000) > - limit = &intel_limits_ironlake_single_lvds_100m; > - else > - limit = &intel_limits_ironlake_single_lvds; > - } > - } else > - limit = &intel_limits_ironlake_dac; > - > - return limit; > -} > - > -static const intel_limit_t * > intel_g4x_limit(struct intel_crtc_state *crtc_state) > { > struct drm_device *dev = crtc_state->base.crtc->dev; > @@ -619,8 +595,8 @@ intel_limit(struct intel_crtc_state *crtc_state, int refclk) > > if (IS_BROXTON(dev)) > limit = &intel_limits_bxt; > - else if (HAS_PCH_SPLIT(dev)) > - limit = intel_ironlake_limit(crtc_state, refclk); > + else if (WARN_ON(HAS_PCH_SPLIT(dev))) > + limit = NULL; > else if (IS_G4X(dev)) { > limit = intel_g4x_limit(crtc_state); > } else if (IS_PINEVIEW(dev)) { I'm curious, when is intel_limits_bxt ever used? Seems like dead code.. It would appear it uses haswell_crtc_compute_clock, which never calls into intel_limit(). ~Maarten
On Mon, 2016-03-14 at 12:46 +0100, Maarten Lankhorst wrote: > Op 14-03-16 om 09:55 schreef Ander Conselvan de Oliveira: > > The funcion intel_ironlake_limit() is only called by the crtc compute > > clock path. By merging it into ironlake_compute_clocks(), the code gets > > clearer, since there's no more if-ladders to follow. > > > > Signed-off-by: Ander Conselvan de Oliveira < > > ander.conselvan.de.oliveira@intel.com> > > --- > > drivers/gpu/drm/i915/intel_display.c | 56 +++++++++++++++------------------ > > --- > > 1 file changed, 23 insertions(+), 33 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c > > b/drivers/gpu/drm/i915/intel_display.c > > index 07b5244..ea71430 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -566,30 +566,6 @@ static bool intel_pipe_will_have_type(const struct > > intel_crtc_state *crtc_state, > > } > > > > static const intel_limit_t * > > -intel_ironlake_limit(struct intel_crtc_state *crtc_state, int refclk) > > -{ > > - struct drm_device *dev = crtc_state->base.crtc->dev; > > - const intel_limit_t *limit; > > - > > - if (intel_pipe_will_have_type(crtc_state, INTEL_OUTPUT_LVDS)) { > > - if (intel_is_dual_link_lvds(dev)) { > > - if (refclk == 100000) > > - limit = > > &intel_limits_ironlake_dual_lvds_100m; > > - else > > - limit = &intel_limits_ironlake_dual_lvds; > > - } else { > > - if (refclk == 100000) > > - limit = > > &intel_limits_ironlake_single_lvds_100m; > > - else > > - limit = &intel_limits_ironlake_single_lvds; > > - } > > - } else > > - limit = &intel_limits_ironlake_dac; > > - > > - return limit; > > -} > > - > > -static const intel_limit_t * > > intel_g4x_limit(struct intel_crtc_state *crtc_state) > > { > > struct drm_device *dev = crtc_state->base.crtc->dev; > > @@ -619,8 +595,8 @@ intel_limit(struct intel_crtc_state *crtc_state, int > > refclk) > > > > if (IS_BROXTON(dev)) > > limit = &intel_limits_bxt; > > - else if (HAS_PCH_SPLIT(dev)) > > - limit = intel_ironlake_limit(crtc_state, refclk); > > + else if (WARN_ON(HAS_PCH_SPLIT(dev))) > > + limit = NULL; > > else if (IS_G4X(dev)) { > > limit = intel_g4x_limit(crtc_state); > > } else if (IS_PINEVIEW(dev)) { > I'm curious, when is intel_limits_bxt ever used? Seems like dead code.. > > It would appear it uses haswell_crtc_compute_clock, which never calls into > intel_limit(). It is called from bxt_find_best_dpll(), which is called form the broxton shared dpll code. I just wrote a patch this morning to make that function reference intel_limits_bxt directly. I want to get rid of intel_limit() altogether if possible, since those if-ladders get confusing really fast. Ander
Op 14-03-16 om 12:53 schreef Ander Conselvan De Oliveira: > On Mon, 2016-03-14 at 12:46 +0100, Maarten Lankhorst wrote: >> Op 14-03-16 om 09:55 schreef Ander Conselvan de Oliveira: >>> The funcion intel_ironlake_limit() is only called by the crtc compute >>> clock path. By merging it into ironlake_compute_clocks(), the code gets >>> clearer, since there's no more if-ladders to follow. >>> >>> Signed-off-by: Ander Conselvan de Oliveira < >>> ander.conselvan.de.oliveira@intel.com> >>> --- >>> drivers/gpu/drm/i915/intel_display.c | 56 +++++++++++++++------------------ >>> --- >>> 1 file changed, 23 insertions(+), 33 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/intel_display.c >>> b/drivers/gpu/drm/i915/intel_display.c >>> index 07b5244..ea71430 100644 >>> --- a/drivers/gpu/drm/i915/intel_display.c >>> +++ b/drivers/gpu/drm/i915/intel_display.c >>> @@ -566,30 +566,6 @@ static bool intel_pipe_will_have_type(const struct >>> intel_crtc_state *crtc_state, >>> } >>> >>> static const intel_limit_t * >>> -intel_ironlake_limit(struct intel_crtc_state *crtc_state, int refclk) >>> -{ >>> - struct drm_device *dev = crtc_state->base.crtc->dev; >>> - const intel_limit_t *limit; >>> - >>> - if (intel_pipe_will_have_type(crtc_state, INTEL_OUTPUT_LVDS)) { >>> - if (intel_is_dual_link_lvds(dev)) { >>> - if (refclk == 100000) >>> - limit = >>> &intel_limits_ironlake_dual_lvds_100m; >>> - else >>> - limit = &intel_limits_ironlake_dual_lvds; >>> - } else { >>> - if (refclk == 100000) >>> - limit = >>> &intel_limits_ironlake_single_lvds_100m; >>> - else >>> - limit = &intel_limits_ironlake_single_lvds; >>> - } >>> - } else >>> - limit = &intel_limits_ironlake_dac; >>> - >>> - return limit; >>> -} >>> - >>> -static const intel_limit_t * >>> intel_g4x_limit(struct intel_crtc_state *crtc_state) >>> { >>> struct drm_device *dev = crtc_state->base.crtc->dev; >>> @@ -619,8 +595,8 @@ intel_limit(struct intel_crtc_state *crtc_state, int >>> refclk) >>> >>> if (IS_BROXTON(dev)) >>> limit = &intel_limits_bxt; >>> - else if (HAS_PCH_SPLIT(dev)) >>> - limit = intel_ironlake_limit(crtc_state, refclk); >>> + else if (WARN_ON(HAS_PCH_SPLIT(dev))) >>> + limit = NULL; >>> else if (IS_G4X(dev)) { >>> limit = intel_g4x_limit(crtc_state); >>> } else if (IS_PINEVIEW(dev)) { >> I'm curious, when is intel_limits_bxt ever used? Seems like dead code.. >> >> It would appear it uses haswell_crtc_compute_clock, which never calls into >> intel_limit(). > It is called from bxt_find_best_dpll(), which is called form the broxton shared > dpll code. I just wrote a patch this morning to make that function reference > intel_limits_bxt directly. I want to get rid of intel_limit() altogether if > possible, since those if-ladders get confusing really fast. > Ah, no idea why I missed it. But indeed, best get rid of it. On that you can add my r-b, same for this series if CI is happy. :) ~Maarten
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 07b5244..ea71430 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -566,30 +566,6 @@ static bool intel_pipe_will_have_type(const struct intel_crtc_state *crtc_state, } static const intel_limit_t * -intel_ironlake_limit(struct intel_crtc_state *crtc_state, int refclk) -{ - struct drm_device *dev = crtc_state->base.crtc->dev; - const intel_limit_t *limit; - - if (intel_pipe_will_have_type(crtc_state, INTEL_OUTPUT_LVDS)) { - if (intel_is_dual_link_lvds(dev)) { - if (refclk == 100000) - limit = &intel_limits_ironlake_dual_lvds_100m; - else - limit = &intel_limits_ironlake_dual_lvds; - } else { - if (refclk == 100000) - limit = &intel_limits_ironlake_single_lvds_100m; - else - limit = &intel_limits_ironlake_single_lvds; - } - } else - limit = &intel_limits_ironlake_dac; - - return limit; -} - -static const intel_limit_t * intel_g4x_limit(struct intel_crtc_state *crtc_state) { struct drm_device *dev = crtc_state->base.crtc->dev; @@ -619,8 +595,8 @@ intel_limit(struct intel_crtc_state *crtc_state, int refclk) if (IS_BROXTON(dev)) limit = &intel_limits_bxt; - else if (HAS_PCH_SPLIT(dev)) - limit = intel_ironlake_limit(crtc_state, refclk); + else if (WARN_ON(HAS_PCH_SPLIT(dev))) + limit = NULL; else if (IS_G4X(dev)) { limit = intel_g4x_limit(crtc_state); } else if (IS_PINEVIEW(dev)) { @@ -8760,13 +8736,28 @@ static bool ironlake_compute_clocks(struct drm_crtc *crtc, const intel_limit_t *limit; bool ret; - if (intel_pipe_will_have_type(crtc_state, INTEL_OUTPUT_LVDS) && - intel_panel_use_ssc(dev_priv)) { - DRM_DEBUG_KMS("using SSC reference clock of %d kHz\n", - dev_priv->vbt.lvds_ssc_freq); - refclk = dev_priv->vbt.lvds_ssc_freq; + refclk = 120000; + + if (intel_pipe_will_have_type(crtc_state, INTEL_OUTPUT_LVDS)) { + if (intel_panel_use_ssc(dev_priv)) { + DRM_DEBUG_KMS("using SSC reference clock of %d kHz\n", + dev_priv->vbt.lvds_ssc_freq); + refclk = dev_priv->vbt.lvds_ssc_freq; + } + + if (intel_is_dual_link_lvds(dev)) { + if (refclk == 100000) + limit = &intel_limits_ironlake_dual_lvds_100m; + else + limit = &intel_limits_ironlake_dual_lvds; + } else { + if (refclk == 100000) + limit = &intel_limits_ironlake_single_lvds_100m; + else + limit = &intel_limits_ironlake_single_lvds; + } } else { - refclk = 120000; + limit = &intel_limits_ironlake_dac; } /* @@ -8774,7 +8765,6 @@ static bool ironlake_compute_clocks(struct drm_crtc *crtc, * refclk, or FALSE. The returned values represent the clock equation: * reflck * (5 * (m1 + 2) + (m2 + 2)) / (n + 2) / p1 / p2. */ - limit = intel_limit(crtc_state, refclk); ret = dev_priv->display.find_dpll(limit, crtc_state, crtc_state->port_clock, refclk, NULL, clock);
The funcion intel_ironlake_limit() is only called by the crtc compute clock path. By merging it into ironlake_compute_clocks(), the code gets clearer, since there's no more if-ladders to follow. Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> --- drivers/gpu/drm/i915/intel_display.c | 56 +++++++++++++++--------------------- 1 file changed, 23 insertions(+), 33 deletions(-)