* [PATCH 0/8] Clean up ironlake clock computation code
@ 2016-03-14 8:55 Ander Conselvan de Oliveira
2016-03-14 8:55 ` [PATCH 1/8] drm/i915: Remove checks for clone config with LVDS in ILK+ dpll code Ander Conselvan de Oliveira
` (7 more replies)
0 siblings, 8 replies; 21+ messages in thread
From: Ander Conselvan de Oliveira @ 2016-03-14 8:55 UTC (permalink / raw)
To: intel-gfx; +Cc: Ander Conselvan de Oliveira
Hi,
I dropped the patch that deletes the LVDS downclocking code and rebased
the other patches.
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Ander Conselvan de Oliveira (8):
drm/i915: Remove checks for clone config with LVDS in ILK+ dpll code
drm/i915: Merge ironlake_get_refclk() into its only caller
drm/i915: Fold intel_ironlake_limit() into clock computation function
drm/i915: Call g4x_find_best_dpll() directly from ILK+ code
drm/i915: Simplify ironlake reduced clock logic a bit
drm/i915: Don't calculate a new clock in ILK+ code if it is already
set
drm/i915: Remove PCH type checks from ironlake_crtc_compute_clock()
drm/i915: Simplify ironlake_crtc_compute_clock() CPU eDP case
drivers/gpu/drm/i915/intel_display.c | 167 +++++++++++++----------------------
1 file changed, 60 insertions(+), 107 deletions(-)
--
2.4.3
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 21+ messages in thread* [PATCH 1/8] drm/i915: Remove checks for clone config with LVDS in ILK+ dpll code 2016-03-14 8:55 [PATCH 0/8] Clean up ironlake clock computation code Ander Conselvan de Oliveira @ 2016-03-14 8:55 ` Ander Conselvan de Oliveira 2016-03-14 13:51 ` Ville Syrjälä 2016-03-14 8:55 ` [PATCH 2/8] drm/i915: Merge ironlake_get_refclk() into its only caller Ander Conselvan de Oliveira ` (6 subsequent siblings) 7 siblings, 1 reply; 21+ messages in thread From: Ander Conselvan de Oliveira @ 2016-03-14 8:55 UTC (permalink / raw) To: intel-gfx; +Cc: Ander Conselvan de Oliveira LVDS is not cloneable, so the check is unnecessary. Removing it makes the surrouding code a bit simpler. Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> --- drivers/gpu/drm/i915/intel_display.c | 31 ++++--------------------------- 1 file changed, 4 insertions(+), 27 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 2d151ad..e7d6584 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -8593,30 +8593,9 @@ static int ironlake_get_refclk(struct intel_crtc_state *crtc_state) { struct drm_device *dev = crtc_state->base.crtc->dev; struct drm_i915_private *dev_priv = dev->dev_private; - struct drm_atomic_state *state = crtc_state->base.state; - struct drm_connector *connector; - struct drm_connector_state *connector_state; - struct intel_encoder *encoder; - int num_connectors = 0, i; - bool is_lvds = false; - - for_each_connector_in_state(state, connector, connector_state, i) { - if (connector_state->crtc != crtc_state->base.crtc) - continue; - - encoder = to_intel_encoder(connector_state->best_encoder); - - switch (encoder->type) { - case INTEL_OUTPUT_LVDS: - is_lvds = true; - break; - default: - break; - } - num_connectors++; - } - if (is_lvds && intel_panel_use_ssc(dev_priv) && num_connectors < 2) { + 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); return dev_priv->vbt.lvds_ssc_freq; @@ -8842,7 +8821,7 @@ static uint32_t ironlake_compute_dpll(struct intel_crtc *intel_crtc, struct drm_connector_state *connector_state; struct intel_encoder *encoder; uint32_t dpll; - int factor, num_connectors = 0, i; + int factor, i; bool is_lvds = false, is_sdvo = false; for_each_connector_in_state(state, connector, connector_state, i) { @@ -8862,8 +8841,6 @@ static uint32_t ironlake_compute_dpll(struct intel_crtc *intel_crtc, default: break; } - - num_connectors++; } /* Enable autotuning of the PLL clock (if permissible) */ @@ -8917,7 +8894,7 @@ static uint32_t ironlake_compute_dpll(struct intel_crtc *intel_crtc, break; } - if (is_lvds && intel_panel_use_ssc(dev_priv) && num_connectors < 2) + if (is_lvds && intel_panel_use_ssc(dev_priv)) dpll |= PLLB_REF_INPUT_SPREADSPECTRUMIN; else dpll |= PLL_REF_INPUT_DREFCLK; -- 2.4.3 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH 1/8] drm/i915: Remove checks for clone config with LVDS in ILK+ dpll code 2016-03-14 8:55 ` [PATCH 1/8] drm/i915: Remove checks for clone config with LVDS in ILK+ dpll code Ander Conselvan de Oliveira @ 2016-03-14 13:51 ` Ville Syrjälä 2016-03-14 13:55 ` Conselvan De Oliveira, Ander 0 siblings, 1 reply; 21+ messages in thread From: Ville Syrjälä @ 2016-03-14 13:51 UTC (permalink / raw) To: Ander Conselvan de Oliveira; +Cc: intel-gfx On Mon, Mar 14, 2016 at 10:55:40AM +0200, Ander Conselvan de Oliveira wrote: > LVDS is not cloneable, so the check is unnecessary. Removing it makes > the surrouding code a bit simpler. > > Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 31 ++++--------------------------- > 1 file changed, 4 insertions(+), 27 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 2d151ad..e7d6584 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -8593,30 +8593,9 @@ static int ironlake_get_refclk(struct intel_crtc_state *crtc_state) > { > struct drm_device *dev = crtc_state->base.crtc->dev; > struct drm_i915_private *dev_priv = dev->dev_private; > - struct drm_atomic_state *state = crtc_state->base.state; > - struct drm_connector *connector; > - struct drm_connector_state *connector_state; > - struct intel_encoder *encoder; > - int num_connectors = 0, i; > - bool is_lvds = false; > - > - for_each_connector_in_state(state, connector, connector_state, i) { > - if (connector_state->crtc != crtc_state->base.crtc) > - continue; > - > - encoder = to_intel_encoder(connector_state->best_encoder); > - > - switch (encoder->type) { > - case INTEL_OUTPUT_LVDS: > - is_lvds = true; > - break; > - default: > - break; > - } > - num_connectors++; > - } > > - if (is_lvds && intel_panel_use_ssc(dev_priv) && num_connectors < 2) { We have the exact same checks in the gmch code as well. For consistency you should change those as well. > + 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); > return dev_priv->vbt.lvds_ssc_freq; > @@ -8842,7 +8821,7 @@ static uint32_t ironlake_compute_dpll(struct intel_crtc *intel_crtc, > struct drm_connector_state *connector_state; > struct intel_encoder *encoder; > uint32_t dpll; > - int factor, num_connectors = 0, i; > + int factor, i; > bool is_lvds = false, is_sdvo = false; > > for_each_connector_in_state(state, connector, connector_state, i) { > @@ -8862,8 +8841,6 @@ static uint32_t ironlake_compute_dpll(struct intel_crtc *intel_crtc, > default: > break; > } > - > - num_connectors++; > } > > /* Enable autotuning of the PLL clock (if permissible) */ > @@ -8917,7 +8894,7 @@ static uint32_t ironlake_compute_dpll(struct intel_crtc *intel_crtc, > break; > } > > - if (is_lvds && intel_panel_use_ssc(dev_priv) && num_connectors < 2) > + if (is_lvds && intel_panel_use_ssc(dev_priv)) > dpll |= PLLB_REF_INPUT_SPREADSPECTRUMIN; > else > dpll |= PLL_REF_INPUT_DREFCLK; > -- > 2.4.3 > > _______________________________________________ > 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] 21+ messages in thread
* Re: [PATCH 1/8] drm/i915: Remove checks for clone config with LVDS in ILK+ dpll code 2016-03-14 13:51 ` Ville Syrjälä @ 2016-03-14 13:55 ` Conselvan De Oliveira, Ander 2016-03-14 14:02 ` Ville Syrjälä 0 siblings, 1 reply; 21+ messages in thread From: Conselvan De Oliveira, Ander @ 2016-03-14 13:55 UTC (permalink / raw) To: ville.syrjala@linux.intel.com; +Cc: intel-gfx@lists.freedesktop.org On Mon, 2016-03-14 at 15:51 +0200, Ville Syrjälä wrote: > On Mon, Mar 14, 2016 at 10:55:40AM +0200, Ander Conselvan de Oliveira wrote: > > LVDS is not cloneable, so the check is unnecessary. Removing it makes > > the surrouding code a bit simpler. > > > > Signed-off-by: Ander Conselvan de Oliveira < > > ander.conselvan.de.oliveira@intel.com> > > --- > > drivers/gpu/drm/i915/intel_display.c | 31 ++++--------------------------- > > 1 file changed, 4 insertions(+), 27 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c > > b/drivers/gpu/drm/i915/intel_display.c > > index 2d151ad..e7d6584 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -8593,30 +8593,9 @@ static int ironlake_get_refclk(struct > > intel_crtc_state *crtc_state) > > { > > struct drm_device *dev = crtc_state->base.crtc->dev; > > struct drm_i915_private *dev_priv = dev->dev_private; > > - struct drm_atomic_state *state = crtc_state->base.state; > > - struct drm_connector *connector; > > - struct drm_connector_state *connector_state; > > - struct intel_encoder *encoder; > > - int num_connectors = 0, i; > > - bool is_lvds = false; > > - > > - for_each_connector_in_state(state, connector, connector_state, i) { > > - if (connector_state->crtc != crtc_state->base.crtc) > > - continue; > > - > > - encoder = to_intel_encoder(connector_state->best_encoder); > > - > > - switch (encoder->type) { > > - case INTEL_OUTPUT_LVDS: > > - is_lvds = true; > > - break; > > - default: > > - break; > > - } > > - num_connectors++; > > - } > > > > - if (is_lvds && intel_panel_use_ssc(dev_priv) && num_connectors < 2) > > { > > We have the exact same checks in the gmch code as well. For consistency > you should change those as well. True. Would it be ok in a follow-up patch? I did that today now that I started doing some clean ups for i9xx_crtc_compute_clock(). If not, I can resend. Thanks, Ander > > > + 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); > > return dev_priv->vbt.lvds_ssc_freq; > > @@ -8842,7 +8821,7 @@ static uint32_t ironlake_compute_dpll(struct > > intel_crtc *intel_crtc, > > struct drm_connector_state *connector_state; > > struct intel_encoder *encoder; > > uint32_t dpll; > > - int factor, num_connectors = 0, i; > > + int factor, i; > > bool is_lvds = false, is_sdvo = false; > > > > for_each_connector_in_state(state, connector, connector_state, i) { > > @@ -8862,8 +8841,6 @@ static uint32_t ironlake_compute_dpll(struct > > intel_crtc *intel_crtc, > > default: > > break; > > } > > - > > - num_connectors++; > > } > > > > /* Enable autotuning of the PLL clock (if permissible) */ > > @@ -8917,7 +8894,7 @@ static uint32_t ironlake_compute_dpll(struct > > intel_crtc *intel_crtc, > > break; > > } > > > > - if (is_lvds && intel_panel_use_ssc(dev_priv) && num_connectors < 2) > > + if (is_lvds && intel_panel_use_ssc(dev_priv)) > > dpll |= PLLB_REF_INPUT_SPREADSPECTRUMIN; > > else > > dpll |= PLL_REF_INPUT_DREFCLK; > > -- > > 2.4.3 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > --------------------------------------------------------------------- Intel Finland Oy Registered Address: PL 281, 00181 Helsinki Business Identity Code: 0357606 - 4 Domiciled in Helsinki This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies. _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/8] drm/i915: Remove checks for clone config with LVDS in ILK+ dpll code 2016-03-14 13:55 ` Conselvan De Oliveira, Ander @ 2016-03-14 14:02 ` Ville Syrjälä 0 siblings, 0 replies; 21+ messages in thread From: Ville Syrjälä @ 2016-03-14 14:02 UTC (permalink / raw) To: Conselvan De Oliveira, Ander; +Cc: intel-gfx@lists.freedesktop.org On Mon, Mar 14, 2016 at 01:55:49PM +0000, Conselvan De Oliveira, Ander wrote: > On Mon, 2016-03-14 at 15:51 +0200, Ville Syrjälä wrote: > > On Mon, Mar 14, 2016 at 10:55:40AM +0200, Ander Conselvan de Oliveira wrote: > > > LVDS is not cloneable, so the check is unnecessary. Removing it makes > > > the surrouding code a bit simpler. > > > > > > Signed-off-by: Ander Conselvan de Oliveira < > > > ander.conselvan.de.oliveira@intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_display.c | 31 ++++--------------------------- > > > 1 file changed, 4 insertions(+), 27 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c > > > b/drivers/gpu/drm/i915/intel_display.c > > > index 2d151ad..e7d6584 100644 > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > @@ -8593,30 +8593,9 @@ static int ironlake_get_refclk(struct > > > intel_crtc_state *crtc_state) > > > { > > > struct drm_device *dev = crtc_state->base.crtc->dev; > > > struct drm_i915_private *dev_priv = dev->dev_private; > > > - struct drm_atomic_state *state = crtc_state->base.state; > > > - struct drm_connector *connector; > > > - struct drm_connector_state *connector_state; > > > - struct intel_encoder *encoder; > > > - int num_connectors = 0, i; > > > - bool is_lvds = false; > > > - > > > - for_each_connector_in_state(state, connector, connector_state, i) { > > > - if (connector_state->crtc != crtc_state->base.crtc) > > > - continue; > > > - > > > - encoder = to_intel_encoder(connector_state->best_encoder); > > > - > > > - switch (encoder->type) { > > > - case INTEL_OUTPUT_LVDS: > > > - is_lvds = true; > > > - break; > > > - default: > > > - break; > > > - } > > > - num_connectors++; > > > - } > > > > > > - if (is_lvds && intel_panel_use_ssc(dev_priv) && num_connectors < 2) > > > { > > > > We have the exact same checks in the gmch code as well. For consistency > > you should change those as well. > > True. Would it be ok in a follow-up patch? I did that today now that I started > doing some clean ups for i9xx_crtc_compute_clock(). If not, I can resend. Followup is fine by me. I had some dpll code unification patches in my lvds_downclock branch too, feel free to steal any if you think they are helpful. Hoping I can reduce that branch as much as possuble before I start bombing the list with it :P > > Thanks, > Ander > > > > > > + 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); > > > return dev_priv->vbt.lvds_ssc_freq; > > > @@ -8842,7 +8821,7 @@ static uint32_t ironlake_compute_dpll(struct > > > intel_crtc *intel_crtc, > > > struct drm_connector_state *connector_state; > > > struct intel_encoder *encoder; > > > uint32_t dpll; > > > - int factor, num_connectors = 0, i; > > > + int factor, i; > > > bool is_lvds = false, is_sdvo = false; > > > > > > for_each_connector_in_state(state, connector, connector_state, i) { > > > @@ -8862,8 +8841,6 @@ static uint32_t ironlake_compute_dpll(struct > > > intel_crtc *intel_crtc, > > > default: > > > break; > > > } > > > - > > > - num_connectors++; > > > } > > > > > > /* Enable autotuning of the PLL clock (if permissible) */ > > > @@ -8917,7 +8894,7 @@ static uint32_t ironlake_compute_dpll(struct > > > intel_crtc *intel_crtc, > > > break; > > > } > > > > > > - if (is_lvds && intel_panel_use_ssc(dev_priv) && num_connectors < 2) > > > + if (is_lvds && intel_panel_use_ssc(dev_priv)) > > > dpll |= PLLB_REF_INPUT_SPREADSPECTRUMIN; > > > else > > > dpll |= PLL_REF_INPUT_DREFCLK; > > > -- > > > 2.4.3 > > > > > > _______________________________________________ > > > 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] 21+ messages in thread
* [PATCH 2/8] drm/i915: Merge ironlake_get_refclk() into its only caller 2016-03-14 8:55 [PATCH 0/8] Clean up ironlake clock computation code Ander Conselvan de Oliveira 2016-03-14 8:55 ` [PATCH 1/8] drm/i915: Remove checks for clone config with LVDS in ILK+ dpll code Ander Conselvan de Oliveira @ 2016-03-14 8:55 ` Ander Conselvan de Oliveira 2016-03-14 13:55 ` Ville Syrjälä 2016-03-14 8:55 ` [PATCH 3/8] drm/i915: Fold intel_ironlake_limit() into clock computation function Ander Conselvan de Oliveira ` (5 subsequent siblings) 7 siblings, 1 reply; 21+ messages in thread From: Ander Conselvan de Oliveira @ 2016-03-14 8:55 UTC (permalink / raw) To: intel-gfx; +Cc: Ander Conselvan de Oliveira A previous patche made ironlake_get_refclk() very simple, so merge it into its only caller. Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> --- drivers/gpu/drm/i915/intel_display.c | 24 ++++++++---------------- 1 file changed, 8 insertions(+), 16 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index e7d6584..07b5244 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -8589,21 +8589,6 @@ void intel_init_pch_refclk(struct drm_device *dev) lpt_init_pch_refclk(dev); } -static int ironlake_get_refclk(struct intel_crtc_state *crtc_state) -{ - struct drm_device *dev = crtc_state->base.crtc->dev; - struct drm_i915_private *dev_priv = dev->dev_private; - - 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); - return dev_priv->vbt.lvds_ssc_freq; - } - - return 120000; -} - static void ironlake_set_pipeconf(struct drm_crtc *crtc) { struct drm_i915_private *dev_priv = crtc->dev->dev_private; @@ -8775,7 +8760,14 @@ static bool ironlake_compute_clocks(struct drm_crtc *crtc, const intel_limit_t *limit; bool ret; - refclk = ironlake_get_refclk(crtc_state); + 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; + } else { + refclk = 120000; + } /* * Returns a set of divisors for the desired target clock with the given -- 2.4.3 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH 2/8] drm/i915: Merge ironlake_get_refclk() into its only caller 2016-03-14 8:55 ` [PATCH 2/8] drm/i915: Merge ironlake_get_refclk() into its only caller Ander Conselvan de Oliveira @ 2016-03-14 13:55 ` Ville Syrjälä 2016-03-14 14:02 ` Conselvan De Oliveira, Ander 0 siblings, 1 reply; 21+ messages in thread From: Ville Syrjälä @ 2016-03-14 13:55 UTC (permalink / raw) To: Ander Conselvan de Oliveira; +Cc: intel-gfx On Mon, Mar 14, 2016 at 10:55:41AM +0200, Ander Conselvan de Oliveira wrote: > A previous patche made ironlake_get_refclk() very simple, so merge > it into its only caller. Again I'd like to keep the pch and gmch code as similar as possible. So could do the same for the gmch code. I already had a patch in my lvds_downclock branch that moved some of the gmch platform differences out from i9xx_get_refclk() into the caller, so I guess might as well move the whole thing I suppose. > > Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 24 ++++++++---------------- > 1 file changed, 8 insertions(+), 16 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index e7d6584..07b5244 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -8589,21 +8589,6 @@ void intel_init_pch_refclk(struct drm_device *dev) > lpt_init_pch_refclk(dev); > } > > -static int ironlake_get_refclk(struct intel_crtc_state *crtc_state) > -{ > - struct drm_device *dev = crtc_state->base.crtc->dev; > - struct drm_i915_private *dev_priv = dev->dev_private; > - > - 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); > - return dev_priv->vbt.lvds_ssc_freq; > - } > - > - return 120000; > -} > - > static void ironlake_set_pipeconf(struct drm_crtc *crtc) > { > struct drm_i915_private *dev_priv = crtc->dev->dev_private; > @@ -8775,7 +8760,14 @@ static bool ironlake_compute_clocks(struct drm_crtc *crtc, > const intel_limit_t *limit; > bool ret; > > - refclk = ironlake_get_refclk(crtc_state); > + 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; > + } else { > + refclk = 120000; > + } > > /* > * Returns a set of divisors for the desired target clock with the given > -- > 2.4.3 > > _______________________________________________ > 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] 21+ messages in thread
* Re: [PATCH 2/8] drm/i915: Merge ironlake_get_refclk() into its only caller 2016-03-14 13:55 ` Ville Syrjälä @ 2016-03-14 14:02 ` Conselvan De Oliveira, Ander 0 siblings, 0 replies; 21+ messages in thread From: Conselvan De Oliveira, Ander @ 2016-03-14 14:02 UTC (permalink / raw) To: ville.syrjala@linux.intel.com; +Cc: intel-gfx@lists.freedesktop.org On Mon, 2016-03-14 at 15:55 +0200, Ville Syrjälä wrote: > On Mon, Mar 14, 2016 at 10:55:41AM +0200, Ander Conselvan de Oliveira wrote: > > A previous patche made ironlake_get_refclk() very simple, so merge > > it into its only caller. > > Again I'd like to keep the pch and gmch code as similar as possible. > So could do the same for the gmch code. > > I already had a patch in my lvds_downclock branch that moved some of > the gmch platform differences out from i9xx_get_refclk() into the > caller, so I guess might as well move the whole thing I suppose. I think we should just split i9xx_crtc_compute_clock() into more platform specific functions and kill i9xx_get_reclk(), intel_limit() and ->find_dpll(). We are jumping through hoops to make the code look like it's generic, but there is a lot of platform specific details. IMO it would be a lot easier to read that way. I wrote some patches going into that direction today. The end result looks like the following: static int chv_crtc_compute_clock(struct intel_crtc *crtc, struct intel_crtc_state *crtc_state) { int refclk; bool ok; const intel_limit_t *limit; memset(&crtc_state->dpll_hw_state, 0, sizeof(crtc_state->dpll_hw_state)); if (crtc_state->has_dsi_encoder) return 0; limit = &intel_limits_chv; if (!crtc_state->clock_set) { refclk = 100000; ok = chv_find_best_dpll(limit, crtc_state, crtc_state->port_clock, refclk, NULL, &crtc_state->dpll); if (!ok) { DRM_ERROR("Couldn't find PLL settings for mode!\n"); return -EINVAL; } } chv_compute_dpll(crtc, crtc_state); return 0; } static int vlv_crtc_compute_clock(struct intel_crtc *crtc, struct intel_crtc_state *crtc_state) { int refclk; bool ok; const intel_limit_t *limit; memset(&crtc_state->dpll_hw_state, 0, sizeof(crtc_state->dpll_hw_state)); if (crtc_state->has_dsi_encoder) return 0; limit = &intel_limits_vlv; if (!crtc_state->clock_set) { refclk = 100000; ok = vlv_find_best_dpll(limit, crtc_state, crtc_state->port_clock, refclk, NULL, &crtc_state->dpll); if (!ok) { DRM_ERROR("Couldn't find PLL settings for mode!\n"); return -EINVAL; } } vlv_compute_dpll(crtc, crtc_state); return 0; } static int gen2_crtc_compute_clock(struct intel_crtc *crtc, struct intel_crtc_state *crtc_state) { struct drm_device *dev = crtc->base.dev; struct drm_i915_private *dev_priv = dev->dev_private; int refclk; const intel_limit_t *limit; memset(&crtc_state->dpll_hw_state, 0, sizeof(crtc_state->dpll_hw_state)); if (intel_pipe_will_have_type(crtc_state, INTEL_OUTPUT_LVDS) && intel_panel_use_ssc(dev_priv)) { refclk = dev_priv->vbt.lvds_ssc_freq; DRM_DEBUG_KMS("using SSC reference clock of %d kHz\n", refclk); } else { refclk = 48000; } if (intel_pipe_will_have_type(crtc_state, INTEL_OUTPUT_LVDS)) limit = &intel_limits_i8xx_lvds; else if (intel_pipe_will_have_type(crtc_state, INTEL_OUTPUT_DVO)) limit = &intel_limits_i8xx_dvo; else limit = &intel_limits_i8xx_dac; if (!crtc_state->clock_set && !i9xx_find_best_dpll(limit, crtc_state, crtc_state->port_clock, refclk, NULL, &crtc_state->dpll)) { DRM_ERROR("Couldn't find PLL settings for mode!\n"); return -EINVAL; } i8xx_compute_dpll(crtc, crtc_state, NULL); return 0; } static int i9xx_crtc_compute_clock(struct intel_crtc *crtc, struct intel_crtc_state *crtc_state) { struct drm_device *dev = crtc->base.dev; struct drm_i915_private *dev_priv = dev->dev_private; int refclk; bool ok; const intel_limit_t *limit; memset(&crtc_state->dpll_hw_state, 0, sizeof(crtc_state->dpll_hw_state)); if (crtc_state->has_dsi_encoder) return 0; if (intel_pipe_will_have_type(crtc_state, INTEL_OUTPUT_LVDS) && intel_panel_use_ssc(dev_priv)) { refclk = dev_priv->vbt.lvds_ssc_freq; DRM_DEBUG_KMS("using SSC reference clock of %d kHz\n", refclk); } else { refclk = 96000; } if (!crtc_state->clock_set) { /* * Returns a set of divisors for the desired target clock with * the given 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); ok = dev_priv->display.find_dpll(limit, crtc_state, crtc_state->port_clock, refclk, NULL, &crtc_state->dpll); if (!ok) { DRM_ERROR("Couldn't find PLL settings for mode!\n"); return -EINVAL; } } i9xx_compute_dpll(crtc, crtc_state, NULL); return 0; } Ander > > > > > Signed-off-by: Ander Conselvan de Oliveira < > > ander.conselvan.de.oliveira@intel.com> > > --- > > drivers/gpu/drm/i915/intel_display.c | 24 ++++++++---------------- > > 1 file changed, 8 insertions(+), 16 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c > > b/drivers/gpu/drm/i915/intel_display.c > > index e7d6584..07b5244 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -8589,21 +8589,6 @@ void intel_init_pch_refclk(struct drm_device *dev) > > lpt_init_pch_refclk(dev); > > } > > > > -static int ironlake_get_refclk(struct intel_crtc_state *crtc_state) > > -{ > > - struct drm_device *dev = crtc_state->base.crtc->dev; > > - struct drm_i915_private *dev_priv = dev->dev_private; > > - > > - 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); > > - return dev_priv->vbt.lvds_ssc_freq; > > - } > > - > > - return 120000; > > -} > > - > > static void ironlake_set_pipeconf(struct drm_crtc *crtc) > > { > > struct drm_i915_private *dev_priv = crtc->dev->dev_private; > > @@ -8775,7 +8760,14 @@ static bool ironlake_compute_clocks(struct drm_crtc > > *crtc, > > const intel_limit_t *limit; > > bool ret; > > > > - refclk = ironlake_get_refclk(crtc_state); > > + 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; > > + } else { > > + refclk = 120000; > > + } > > > > /* > > * Returns a set of divisors for the desired target clock with the > > given > > -- > > 2.4.3 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > --------------------------------------------------------------------- Intel Finland Oy Registered Address: PL 281, 00181 Helsinki Business Identity Code: 0357606 - 4 Domiciled in Helsinki This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies. _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 3/8] drm/i915: Fold intel_ironlake_limit() into clock computation function 2016-03-14 8:55 [PATCH 0/8] Clean up ironlake clock computation code Ander Conselvan de Oliveira 2016-03-14 8:55 ` [PATCH 1/8] drm/i915: Remove checks for clone config with LVDS in ILK+ dpll code Ander Conselvan de Oliveira 2016-03-14 8:55 ` [PATCH 2/8] drm/i915: Merge ironlake_get_refclk() into its only caller Ander Conselvan de Oliveira @ 2016-03-14 8:55 ` Ander Conselvan de Oliveira 2016-03-14 11:46 ` Maarten Lankhorst 2016-03-14 8:55 ` [PATCH 4/8] drm/i915: Call g4x_find_best_dpll() directly from ILK+ code Ander Conselvan de Oliveira ` (4 subsequent siblings) 7 siblings, 1 reply; 21+ messages in thread From: Ander Conselvan de Oliveira @ 2016-03-14 8:55 UTC (permalink / raw) To: intel-gfx; +Cc: 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)) { @@ -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); -- 2.4.3 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH 3/8] drm/i915: Fold intel_ironlake_limit() into clock computation function 2016-03-14 8:55 ` [PATCH 3/8] drm/i915: Fold intel_ironlake_limit() into clock computation function Ander Conselvan de Oliveira @ 2016-03-14 11:46 ` Maarten Lankhorst 2016-03-14 11:53 ` Ander Conselvan De Oliveira 0 siblings, 1 reply; 21+ messages in thread From: Maarten Lankhorst @ 2016-03-14 11:46 UTC (permalink / raw) To: Ander Conselvan de Oliveira, intel-gfx 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 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 3/8] drm/i915: Fold intel_ironlake_limit() into clock computation function 2016-03-14 11:46 ` Maarten Lankhorst @ 2016-03-14 11:53 ` Ander Conselvan De Oliveira 2016-03-14 11:59 ` Maarten Lankhorst 0 siblings, 1 reply; 21+ messages in thread From: Ander Conselvan De Oliveira @ 2016-03-14 11:53 UTC (permalink / raw) To: Maarten Lankhorst, intel-gfx 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 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 3/8] drm/i915: Fold intel_ironlake_limit() into clock computation function 2016-03-14 11:53 ` Ander Conselvan De Oliveira @ 2016-03-14 11:59 ` Maarten Lankhorst 0 siblings, 0 replies; 21+ messages in thread From: Maarten Lankhorst @ 2016-03-14 11:59 UTC (permalink / raw) To: Ander Conselvan De Oliveira, intel-gfx 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 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 4/8] drm/i915: Call g4x_find_best_dpll() directly from ILK+ code 2016-03-14 8:55 [PATCH 0/8] Clean up ironlake clock computation code Ander Conselvan de Oliveira ` (2 preceding siblings ...) 2016-03-14 8:55 ` [PATCH 3/8] drm/i915: Fold intel_ironlake_limit() into clock computation function Ander Conselvan de Oliveira @ 2016-03-14 8:55 ` Ander Conselvan de Oliveira 2016-03-14 8:55 ` [PATCH 5/8] drm/i915: Simplify ironlake reduced clock logic a bit Ander Conselvan de Oliveira ` (3 subsequent siblings) 7 siblings, 0 replies; 21+ messages in thread From: Ander Conselvan de Oliveira @ 2016-03-14 8:55 UTC (permalink / raw) To: intel-gfx; +Cc: Ander Conselvan de Oliveira The call to dev_priv->display.find_dpll() is already in platform specific code, so avoid the extra detour. Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> --- drivers/gpu/drm/i915/intel_display.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index ea71430..1d24254 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -8765,9 +8765,8 @@ 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. */ - ret = dev_priv->display.find_dpll(limit, crtc_state, - crtc_state->port_clock, - refclk, NULL, clock); + ret = g4x_find_best_dpll(limit, crtc_state, crtc_state->port_clock, + refclk, NULL, clock); if (!ret) return false; -- 2.4.3 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH 5/8] drm/i915: Simplify ironlake reduced clock logic a bit 2016-03-14 8:55 [PATCH 0/8] Clean up ironlake clock computation code Ander Conselvan de Oliveira ` (3 preceding siblings ...) 2016-03-14 8:55 ` [PATCH 4/8] drm/i915: Call g4x_find_best_dpll() directly from ILK+ code Ander Conselvan de Oliveira @ 2016-03-14 8:55 ` Ander Conselvan de Oliveira 2016-03-14 8:55 ` [PATCH 6/8] drm/i915: Don't calculate a new clock in ILK+ code if it is already set Ander Conselvan de Oliveira ` (2 subsequent siblings) 7 siblings, 0 replies; 21+ messages in thread From: Ander Conselvan de Oliveira @ 2016-03-14 8:55 UTC (permalink / raw) To: intel-gfx; +Cc: Ander Conselvan de Oliveira Check has_reduced_clock only once when setting dpll_hw_state, making the code slightly more readable. Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> --- drivers/gpu/drm/i915/intel_display.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 1d24254..7034667 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -8921,6 +8921,8 @@ static int ironlake_crtc_compute_clock(struct intel_crtc *crtc, fp = i9xx_dpll_compute_fp(&crtc_state->dpll); if (has_reduced_clock) fp2 = i9xx_dpll_compute_fp(&reduced_clock); + else + fp2 = fp; dpll = ironlake_compute_dpll(crtc, crtc_state, &fp, &reduced_clock, @@ -8928,10 +8930,7 @@ static int ironlake_crtc_compute_clock(struct intel_crtc *crtc, crtc_state->dpll_hw_state.dpll = dpll; crtc_state->dpll_hw_state.fp0 = fp; - if (has_reduced_clock) - crtc_state->dpll_hw_state.fp1 = fp2; - else - crtc_state->dpll_hw_state.fp1 = fp; + crtc_state->dpll_hw_state.fp1 = fp2; pll = intel_get_shared_dpll(crtc, crtc_state, NULL); if (pll == NULL) { -- 2.4.3 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH 6/8] drm/i915: Don't calculate a new clock in ILK+ code if it is already set 2016-03-14 8:55 [PATCH 0/8] Clean up ironlake clock computation code Ander Conselvan de Oliveira ` (4 preceding siblings ...) 2016-03-14 8:55 ` [PATCH 5/8] drm/i915: Simplify ironlake reduced clock logic a bit Ander Conselvan de Oliveira @ 2016-03-14 8:55 ` Ander Conselvan de Oliveira 2016-03-14 11:51 ` Maarten Lankhorst 2016-03-14 8:55 ` [PATCH 7/8] drm/i915: Remove PCH type checks from ironlake_crtc_compute_clock() Ander Conselvan de Oliveira 2016-03-14 8:55 ` [PATCH 8/8] drm/i915: Simplify ironlake_crtc_compute_clock() CPU eDP case Ander Conselvan de Oliveira 7 siblings, 1 reply; 21+ messages in thread From: Ander Conselvan de Oliveira @ 2016-03-14 8:55 UTC (permalink / raw) To: intel-gfx; +Cc: Ander Conselvan de Oliveira Remove the clock calculation from ironlake_crtc_compute_clock() when the encoder compute_config() already set one. The value was just thrown away in that case. Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> --- drivers/gpu/drm/i915/intel_display.c | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 7034667..bf0416d 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -8889,7 +8889,7 @@ static int ironlake_crtc_compute_clock(struct intel_crtc *crtc, struct drm_device *dev = crtc->base.dev; intel_clock_t clock, reduced_clock; u32 dpll = 0, fp = 0, fp2 = 0; - bool ok, has_reduced_clock = false; + bool has_reduced_clock = false; bool is_lvds = false; struct intel_shared_dpll *pll; @@ -8901,14 +8901,15 @@ static int ironlake_crtc_compute_clock(struct intel_crtc *crtc, WARN(!(HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev)), "Unexpected PCH type %d\n", INTEL_PCH_TYPE(dev)); - ok = ironlake_compute_clocks(&crtc->base, crtc_state, &clock, - &has_reduced_clock, &reduced_clock); - if (!ok && !crtc_state->clock_set) { - DRM_ERROR("Couldn't find PLL settings for mode!\n"); - return -EINVAL; - } - /* Compat-code for transition, will disappear. */ if (!crtc_state->clock_set) { + if (!ironlake_compute_clocks(&crtc->base, crtc_state, &clock, + &has_reduced_clock, + &reduced_clock)) { + DRM_ERROR("Couldn't find PLL settings for mode!\n"); + return -EINVAL; + } + + /* Compat-code for transition, will disappear. */ crtc_state->dpll.n = clock.n; crtc_state->dpll.m1 = clock.m1; crtc_state->dpll.m2 = clock.m2; -- 2.4.3 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH 6/8] drm/i915: Don't calculate a new clock in ILK+ code if it is already set 2016-03-14 8:55 ` [PATCH 6/8] drm/i915: Don't calculate a new clock in ILK+ code if it is already set Ander Conselvan de Oliveira @ 2016-03-14 11:51 ` Maarten Lankhorst 2016-03-14 13:01 ` Ander Conselvan De Oliveira 0 siblings, 1 reply; 21+ messages in thread From: Maarten Lankhorst @ 2016-03-14 11:51 UTC (permalink / raw) To: Ander Conselvan de Oliveira, intel-gfx Op 14-03-16 om 09:55 schreef Ander Conselvan de Oliveira: > Remove the clock calculation from ironlake_crtc_compute_clock() when the > encoder compute_config() already set one. The value was just thrown away > in that case. > > Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> > It was thrown away, but it could still reject based on the limits, which this patch changes. This might be made more clear in the commit message. _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 6/8] drm/i915: Don't calculate a new clock in ILK+ code if it is already set 2016-03-14 11:51 ` Maarten Lankhorst @ 2016-03-14 13:01 ` Ander Conselvan De Oliveira 2016-03-14 13:15 ` Maarten Lankhorst 0 siblings, 1 reply; 21+ messages in thread From: Ander Conselvan De Oliveira @ 2016-03-14 13:01 UTC (permalink / raw) To: Maarten Lankhorst, intel-gfx [-- Attachment #1: Type: text/plain, Size: 1020 bytes --] On Mon, 2016-03-14 at 12:51 +0100, Maarten Lankhorst wrote: > Op 14-03-16 om 09:55 schreef Ander Conselvan de Oliveira: > > Remove the clock calculation from ironlake_crtc_compute_clock() when the > > encoder compute_config() already set one. The value was just thrown away > > in that case. > > > > Signed-off-by: Ander Conselvan de Oliveira < > > ander.conselvan.de.oliveira@intel.com> > > > It was thrown away, but it could still reject based on the limits, which this > patch changes. > This might be made more clear in the commit message. Good point. To be honest, I didn't very this as carefully as I should have before sending and missed that detail. It turns out that change is safe. To verify I extracted the relevant code and run it with all possible port clocks we could have with either the sdvo or the dp encoder setting the clock. See the attached C file. I was too lazy to actually understand what the g4x_find_best_dpll() does. Anyway, I'll send another version with a note about this. Thanks, Ander [-- Attachment #2: ilk-pll.c --] [-- Type: text/x-csrc, Size: 11354 bytes --] /* * Copyright © 2006-2007 Intel Corporation * * Permission is hereby granted, free of charge, to any person obtaining a * copy of this software and associated documentation files (the "Software"), * to deal in the Software without restriction, including without limitation * the rights to use, copy, modify, merge, publish, distribute, sublicense, * and/or sell copies of the Software, and to permit persons to whom the * Software is furnished to do so, subject to the following conditions: * * The above copyright notice and this permission notice (including the next * paragraph) shall be included in all copies or substantial portions of the * Software. * * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER * DEALINGS IN THE SOFTWARE. * * Authors: * Eric Anholt <eric@anholt.net> */ #include <string.h> #include <stdbool.h> #include <stdint.h> #include <stdlib.h> #include <stdio.h> typedef struct dpll { /* given values */ int n; int m1, m2; int p1, p2; /* derived values */ int dot; int vco; int m; int p; } intel_clock_t; typedef struct { int min, max; } intel_range_t; typedef struct { int dot_limit; int p2_slow, p2_fast; } intel_p2_t; typedef struct intel_limit intel_limit_t; struct intel_limit { intel_range_t dot, vco, n, m, m1, m2, p, p1; intel_p2_t p2; }; /* Ironlake / Sandybridge * * We calculate clock using (register_value + 2) for N/M1/M2, so here * the range value for them is (actual_value - 2). */ static const intel_limit_t intel_limits_ironlake_dac = { .dot = { .min = 25000, .max = 350000 }, .vco = { .min = 1760000, .max = 3510000 }, .n = { .min = 1, .max = 5 }, .m = { .min = 79, .max = 127 }, .m1 = { .min = 12, .max = 22 }, .m2 = { .min = 5, .max = 9 }, .p = { .min = 5, .max = 80 }, .p1 = { .min = 1, .max = 8 }, .p2 = { .dot_limit = 225000, .p2_slow = 10, .p2_fast = 5 }, }; static const intel_limit_t intel_limits_ironlake_single_lvds = { .dot = { .min = 25000, .max = 350000 }, .vco = { .min = 1760000, .max = 3510000 }, .n = { .min = 1, .max = 3 }, .m = { .min = 79, .max = 118 }, .m1 = { .min = 12, .max = 22 }, .m2 = { .min = 5, .max = 9 }, .p = { .min = 28, .max = 112 }, .p1 = { .min = 2, .max = 8 }, .p2 = { .dot_limit = 225000, .p2_slow = 14, .p2_fast = 14 }, }; static const intel_limit_t intel_limits_ironlake_dual_lvds = { .dot = { .min = 25000, .max = 350000 }, .vco = { .min = 1760000, .max = 3510000 }, .n = { .min = 1, .max = 3 }, .m = { .min = 79, .max = 127 }, .m1 = { .min = 12, .max = 22 }, .m2 = { .min = 5, .max = 9 }, .p = { .min = 14, .max = 56 }, .p1 = { .min = 2, .max = 8 }, .p2 = { .dot_limit = 225000, .p2_slow = 7, .p2_fast = 7 }, }; /* LVDS 100mhz refclk limits. */ static const intel_limit_t intel_limits_ironlake_single_lvds_100m = { .dot = { .min = 25000, .max = 350000 }, .vco = { .min = 1760000, .max = 3510000 }, .n = { .min = 1, .max = 2 }, .m = { .min = 79, .max = 126 }, .m1 = { .min = 12, .max = 22 }, .m2 = { .min = 5, .max = 9 }, .p = { .min = 28, .max = 112 }, .p1 = { .min = 2, .max = 8 }, .p2 = { .dot_limit = 225000, .p2_slow = 14, .p2_fast = 14 }, }; static const intel_limit_t intel_limits_ironlake_dual_lvds_100m = { .dot = { .min = 25000, .max = 350000 }, .vco = { .min = 1760000, .max = 3510000 }, .n = { .min = 1, .max = 3 }, .m = { .min = 79, .max = 126 }, .m1 = { .min = 12, .max = 22 }, .m2 = { .min = 5, .max = 9 }, .p = { .min = 14, .max = 42 }, .p1 = { .min = 2, .max = 6 }, .p2 = { .dot_limit = 225000, .p2_slow = 7, .p2_fast = 7 }, }; /* stubs */ #define WARN_ON(x) (x) #define INTEL_OUTPUT_LVDS 1 struct drm_device { void *dev_private; }; struct drm_crtc { struct drm_device *dev; }; struct intel_crtc_state { struct { struct drm_crtc *crtc; } base; int port_clock; struct dpll dpll; bool clock_set; }; static bool intel_pipe_will_have_type(const void *a, int b) { return false; } static bool intel_is_dual_link_lvds(const void *a) { return false; } static bool intel_panel_use_ssc(const void *b) { return false; } /* --------------------- */ /* * Divide positive or negative dividend by positive divisor and round * to closest integer. Result is undefined for negative divisors and * for negative dividends if the divisor variable type is unsigned. */ #define DIV_ROUND_CLOSEST(x, divisor)( \ { \ typeof(x) __x = x; \ typeof(divisor) __d = divisor; \ (((typeof(x))-1) > 0 || \ ((typeof(divisor))-1) > 0 || (__x) > 0) ? \ (((__x) + ((__d) / 2)) / (__d)) : \ (((__x) - ((__d) / 2)) / (__d)); \ } \ ) #define INTELPllInvalid(s) do { /* DRM_DEBUG(s); */ return false; } while (0) /** * Returns whether the given set of divisors are valid for a given refclk with * the given connectors. */ static bool intel_PLL_is_valid(struct drm_device *dev, const intel_limit_t *limit, const intel_clock_t *clock) { if (clock->n < limit->n.min || limit->n.max < clock->n) INTELPllInvalid("n out of range\n"); if (clock->p1 < limit->p1.min || limit->p1.max < clock->p1) INTELPllInvalid("p1 out of range\n"); if (clock->m2 < limit->m2.min || limit->m2.max < clock->m2) INTELPllInvalid("m2 out of range\n"); if (clock->m1 < limit->m1.min || limit->m1.max < clock->m1) INTELPllInvalid("m1 out of range\n"); if (clock->m1 <= clock->m2) INTELPllInvalid("m1 <= m2\n"); if (clock->p < limit->p.min || limit->p.max < clock->p) INTELPllInvalid("p out of range\n"); if (clock->m < limit->m.min || limit->m.max < clock->m) INTELPllInvalid("m out of range\n"); if (clock->vco < limit->vco.min || limit->vco.max < clock->vco) INTELPllInvalid("vco out of range\n"); /* XXX: We may need to be checking "Dot clock" depending on the multiplier, * connector, etc., rather than just a single range. */ if (clock->dot < limit->dot.min || limit->dot.max < clock->dot) INTELPllInvalid("dot out of range\n"); return true; } static uint32_t i9xx_dpll_compute_m(struct dpll *dpll) { return 5 * (dpll->m1 + 2) + (dpll->m2 + 2); } static int i9xx_calc_dpll_params(int refclk, intel_clock_t *clock) { clock->m = i9xx_dpll_compute_m(clock); clock->p = clock->p1 * clock->p2; if (WARN_ON(clock->n + 2 == 0 || clock->p == 0)) return 0; clock->vco = DIV_ROUND_CLOSEST(refclk * clock->m, clock->n + 2); clock->dot = DIV_ROUND_CLOSEST(clock->vco, clock->p); return clock->dot; } static int i9xx_select_p2_div(const intel_limit_t *limit, const struct intel_crtc_state *crtc_state, int target) { struct drm_device *dev = crtc_state->base.crtc->dev; if (intel_pipe_will_have_type(crtc_state, INTEL_OUTPUT_LVDS)) { /* * For LVDS just rely on its current settings for dual-channel. * We haven't figured out how to reliably set up different * single/dual channel state, if we even can. */ if (intel_is_dual_link_lvds(dev)) return limit->p2.p2_fast; else return limit->p2.p2_slow; } else { if (target < limit->p2.dot_limit) return limit->p2.p2_slow; else return limit->p2.p2_fast; } } static bool g4x_find_best_dpll(const intel_limit_t *limit, struct intel_crtc_state *crtc_state, int target, int refclk, intel_clock_t *match_clock, intel_clock_t *best_clock) { struct drm_device *dev = crtc_state->base.crtc->dev; intel_clock_t clock; int max_n; bool found = false; /* approximately equals target * 0.00585 */ int err_most = (target >> 8) + (target >> 9); memset(best_clock, 0, sizeof(*best_clock)); clock.p2 = i9xx_select_p2_div(limit, crtc_state, target); max_n = limit->n.max; /* based on hardware requirement, prefer smaller n to precision */ for (clock.n = limit->n.min; clock.n <= max_n; clock.n++) { /* based on hardware requirement, prefere larger m1,m2 */ for (clock.m1 = limit->m1.max; clock.m1 >= limit->m1.min; clock.m1--) { for (clock.m2 = limit->m2.max; clock.m2 >= limit->m2.min; clock.m2--) { for (clock.p1 = limit->p1.max; clock.p1 >= limit->p1.min; clock.p1--) { int this_err; i9xx_calc_dpll_params(refclk, &clock); if (!intel_PLL_is_valid(dev, limit, &clock)) continue; this_err = abs(clock.dot - target); if (this_err < err_most) { *best_clock = clock; err_most = this_err; max_n = clock.n; found = true; } } } } } return found; } static bool ironlake_compute_clocks(struct drm_crtc *crtc, struct intel_crtc_state *crtc_state, intel_clock_t *clock, bool *has_reduced_clock, intel_clock_t *reduced_clock) { struct drm_device *dev = crtc->dev; struct drm_i915_private *dev_priv = dev->dev_private; int refclk; const intel_limit_t *limit; bool ret; refclk = 120000; 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; } /* * Returns a set of divisors for the desired target clock with the given * refclk, or FALSE. The returned values represent the clock equation: * reflck * (5 * (m1 + 2) + (m2 + 2)) / (n + 2) / p1 / p2. */ ret = g4x_find_best_dpll(limit, crtc_state, crtc_state->port_clock, refclk, NULL, clock); if (!ret) return false; return true; } static void i9xx_adjust_sdvo_tv_clock(struct intel_crtc_state *pipe_config) { unsigned dotclock = pipe_config->port_clock; struct dpll *clock = &pipe_config->dpll; /* SDVO TV has fixed PLL values depend on its clock range, this mirrors vbios setting. */ if (dotclock >= 100000 && dotclock < 140500) { clock->p1 = 2; clock->p2 = 10; clock->n = 3; clock->m1 = 16; clock->m2 = 8; } else if (dotclock >= 140500 && dotclock <= 200000) { clock->p1 = 1; clock->p2 = 10; clock->n = 6; clock->m1 = 12; clock->m2 = 8; } else { fprintf(stderr, "SDVO TV clock out of range: %i\n", dotclock); } pipe_config->clock_set = true; } #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0])) int main() { struct intel_crtc_state crtc_state; struct drm_crtc crtc; int test_clocks[] = { 100000, 140500, 200000, 162000, 270000}; intel_clock_t clock; crtc_state.base.crtc = &crtc; for (int i = 0; i < ARRAY_SIZE(test_clocks); i++) { bool ok; crtc_state.port_clock = test_clocks[i]; ok = ironlake_compute_clocks(&crtc, &crtc_state, &clock, NULL, NULL); printf("clock %d: %s\n", test_clocks[i], ok ? "passed" : "failed"); } for (int i = 100000; i <= 270000; i++) { bool ok; crtc_state.port_clock = i; ok = ironlake_compute_clocks(&crtc, &crtc_state, &clock, NULL, NULL); if (!ok) printf("clock %d: %s\n", i, ok ? "passed" : "failed"); } return 0; } [-- Attachment #3: Type: text/plain, Size: 160 bytes --] _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 6/8] drm/i915: Don't calculate a new clock in ILK+ code if it is already set 2016-03-14 13:01 ` Ander Conselvan De Oliveira @ 2016-03-14 13:15 ` Maarten Lankhorst 0 siblings, 0 replies; 21+ messages in thread From: Maarten Lankhorst @ 2016-03-14 13:15 UTC (permalink / raw) To: Ander Conselvan De Oliveira, intel-gfx Op 14-03-16 om 14:01 schreef Ander Conselvan De Oliveira: > On Mon, 2016-03-14 at 12:51 +0100, Maarten Lankhorst wrote: >> Op 14-03-16 om 09:55 schreef Ander Conselvan de Oliveira: >>> Remove the clock calculation from ironlake_crtc_compute_clock() when the >>> encoder compute_config() already set one. The value was just thrown away >>> in that case. >>> >>> Signed-off-by: Ander Conselvan de Oliveira < >>> ander.conselvan.de.oliveira@intel.com> >>> >> It was thrown away, but it could still reject based on the limits, which this >> patch changes. >> This might be made more clear in the commit message. > Good point. To be honest, I didn't very this as carefully as I should have > before sending and missed that detail. It turns out that change is safe. To > verify I extracted the relevant code and run it with all possible port clocks we > could have with either the sdvo or the dp encoder setting the clock. See the > attached C file. I was too lazy to actually understand what the > g4x_find_best_dpll() does. > I'm not sure this would have mattered even if the limits were different, since they wouldn't be used. Just something to make a note of. ~Maarten _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 7/8] drm/i915: Remove PCH type checks from ironlake_crtc_compute_clock() 2016-03-14 8:55 [PATCH 0/8] Clean up ironlake clock computation code Ander Conselvan de Oliveira ` (5 preceding siblings ...) 2016-03-14 8:55 ` [PATCH 6/8] drm/i915: Don't calculate a new clock in ILK+ code if it is already set Ander Conselvan de Oliveira @ 2016-03-14 8:55 ` Ander Conselvan de Oliveira 2016-03-14 8:55 ` [PATCH 8/8] drm/i915: Simplify ironlake_crtc_compute_clock() CPU eDP case Ander Conselvan de Oliveira 7 siblings, 0 replies; 21+ messages in thread From: Ander Conselvan de Oliveira @ 2016-03-14 8:55 UTC (permalink / raw) To: intel-gfx; +Cc: Ander Conselvan de Oliveira The checks were added in commit 5dc5298bb3e5 ("drm/i915: add proper CPU/PCH checks to crtc_mode_set functions") in a time when there was doubts on what PCHs would be supported by HSW. There are similar checks for PCH type in intel_detect_pch() and the function pointers are initialized based on platform/pch information, so the removed WARN can't ever be reached. v2: Rebase without patch that drops lvds downclock code. (Ville) Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> --- drivers/gpu/drm/i915/intel_display.c | 4 ---- 1 file changed, 4 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index bf0416d..212c53c 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -8886,7 +8886,6 @@ static uint32_t ironlake_compute_dpll(struct intel_crtc *intel_crtc, static int ironlake_crtc_compute_clock(struct intel_crtc *crtc, struct intel_crtc_state *crtc_state) { - struct drm_device *dev = crtc->base.dev; intel_clock_t clock, reduced_clock; u32 dpll = 0, fp = 0, fp2 = 0; bool has_reduced_clock = false; @@ -8898,9 +8897,6 @@ static int ironlake_crtc_compute_clock(struct intel_crtc *crtc, is_lvds = intel_pipe_will_have_type(crtc_state, INTEL_OUTPUT_LVDS); - WARN(!(HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev)), - "Unexpected PCH type %d\n", INTEL_PCH_TYPE(dev)); - if (!crtc_state->clock_set) { if (!ironlake_compute_clocks(&crtc->base, crtc_state, &clock, &has_reduced_clock, -- 2.4.3 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH 8/8] drm/i915: Simplify ironlake_crtc_compute_clock() CPU eDP case 2016-03-14 8:55 [PATCH 0/8] Clean up ironlake clock computation code Ander Conselvan de Oliveira ` (6 preceding siblings ...) 2016-03-14 8:55 ` [PATCH 7/8] drm/i915: Remove PCH type checks from ironlake_crtc_compute_clock() Ander Conselvan de Oliveira @ 2016-03-14 8:55 ` Ander Conselvan de Oliveira 2016-03-14 12:01 ` Maarten Lankhorst 7 siblings, 1 reply; 21+ messages in thread From: Ander Conselvan de Oliveira @ 2016-03-14 8:55 UTC (permalink / raw) To: intel-gfx; +Cc: Ander Conselvan de Oliveira None of the code in ironlake_crtc_compute_clock() is relevant for CPU eDP. The CPU eDP PLL is turned on and off in ironlake_edp_pll_{on,off} from the DP code and that doesn't depend on the crtc_state->dpll values, so just return early in that case. v2: Rebase without patch that drops lvds downclock code. (Ville) Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> --- drivers/gpu/drm/i915/intel_display.c | 47 ++++++++++++++++++------------------ 1 file changed, 23 insertions(+), 24 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 212c53c..0ddba81 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -8889,13 +8889,16 @@ static int ironlake_crtc_compute_clock(struct intel_crtc *crtc, intel_clock_t clock, reduced_clock; u32 dpll = 0, fp = 0, fp2 = 0; bool has_reduced_clock = false; - bool is_lvds = false; struct intel_shared_dpll *pll; memset(&crtc_state->dpll_hw_state, 0, sizeof(crtc_state->dpll_hw_state)); - is_lvds = intel_pipe_will_have_type(crtc_state, INTEL_OUTPUT_LVDS); + crtc->lowfreq_avail = false; + + /* CPU eDP is the only output that doesn't need a PCH PLL of its own. */ + if (!crtc_state->has_pch_encoder) + return 0; if (!crtc_state->clock_set) { if (!ironlake_compute_clocks(&crtc->base, crtc_state, &clock, @@ -8913,34 +8916,30 @@ static int ironlake_crtc_compute_clock(struct intel_crtc *crtc, crtc_state->dpll.p2 = clock.p2; } - /* CPU eDP is the only output that doesn't need a PCH PLL of its own. */ - if (crtc_state->has_pch_encoder) { - fp = i9xx_dpll_compute_fp(&crtc_state->dpll); - if (has_reduced_clock) - fp2 = i9xx_dpll_compute_fp(&reduced_clock); - else - fp2 = fp; + fp = i9xx_dpll_compute_fp(&crtc_state->dpll); + if (has_reduced_clock) + fp2 = i9xx_dpll_compute_fp(&reduced_clock); + else + fp2 = fp; - dpll = ironlake_compute_dpll(crtc, crtc_state, - &fp, &reduced_clock, - has_reduced_clock ? &fp2 : NULL); + dpll = ironlake_compute_dpll(crtc, crtc_state, + &fp, &reduced_clock, + has_reduced_clock ? &fp2 : NULL); - crtc_state->dpll_hw_state.dpll = dpll; - crtc_state->dpll_hw_state.fp0 = fp; - crtc_state->dpll_hw_state.fp1 = fp2; + crtc_state->dpll_hw_state.dpll = dpll; + crtc_state->dpll_hw_state.fp0 = fp; + crtc_state->dpll_hw_state.fp1 = fp2; - pll = intel_get_shared_dpll(crtc, crtc_state, NULL); - if (pll == NULL) { - DRM_DEBUG_DRIVER("failed to find PLL for pipe %c\n", - pipe_name(crtc->pipe)); - return -EINVAL; - } + pll = intel_get_shared_dpll(crtc, crtc_state, NULL); + if (pll == NULL) { + DRM_DEBUG_DRIVER("failed to find PLL for pipe %c\n", + pipe_name(crtc->pipe)); + return -EINVAL; } - if (is_lvds && has_reduced_clock) + if (intel_pipe_will_have_type(crtc_state, INTEL_OUTPUT_LVDS) && + has_reduced_clock) crtc->lowfreq_avail = true; - else - crtc->lowfreq_avail = false; return 0; } -- 2.4.3 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH 8/8] drm/i915: Simplify ironlake_crtc_compute_clock() CPU eDP case 2016-03-14 8:55 ` [PATCH 8/8] drm/i915: Simplify ironlake_crtc_compute_clock() CPU eDP case Ander Conselvan de Oliveira @ 2016-03-14 12:01 ` Maarten Lankhorst 0 siblings, 0 replies; 21+ messages in thread From: Maarten Lankhorst @ 2016-03-14 12:01 UTC (permalink / raw) To: Ander Conselvan de Oliveira, intel-gfx Op 14-03-16 om 09:55 schreef Ander Conselvan de Oliveira: > None of the code in ironlake_crtc_compute_clock() is relevant for CPU > eDP. The CPU eDP PLL is turned on and off in ironlake_edp_pll_{on,off} > from the DP code and that doesn't depend on the crtc_state->dpll values, > so just return early in that case. > > v2: Rebase without patch that drops lvds downclock code. (Ville) > Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 47 ++++++++++++++++++------------------ > 1 file changed, 23 insertions(+), 24 deletions(-) > For the whole series: Reviwed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2016-03-14 14:03 UTC | newest] Thread overview: 21+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-03-14 8:55 [PATCH 0/8] Clean up ironlake clock computation code Ander Conselvan de Oliveira 2016-03-14 8:55 ` [PATCH 1/8] drm/i915: Remove checks for clone config with LVDS in ILK+ dpll code Ander Conselvan de Oliveira 2016-03-14 13:51 ` Ville Syrjälä 2016-03-14 13:55 ` Conselvan De Oliveira, Ander 2016-03-14 14:02 ` Ville Syrjälä 2016-03-14 8:55 ` [PATCH 2/8] drm/i915: Merge ironlake_get_refclk() into its only caller Ander Conselvan de Oliveira 2016-03-14 13:55 ` Ville Syrjälä 2016-03-14 14:02 ` Conselvan De Oliveira, Ander 2016-03-14 8:55 ` [PATCH 3/8] drm/i915: Fold intel_ironlake_limit() into clock computation function Ander Conselvan de Oliveira 2016-03-14 11:46 ` Maarten Lankhorst 2016-03-14 11:53 ` Ander Conselvan De Oliveira 2016-03-14 11:59 ` Maarten Lankhorst 2016-03-14 8:55 ` [PATCH 4/8] drm/i915: Call g4x_find_best_dpll() directly from ILK+ code Ander Conselvan de Oliveira 2016-03-14 8:55 ` [PATCH 5/8] drm/i915: Simplify ironlake reduced clock logic a bit Ander Conselvan de Oliveira 2016-03-14 8:55 ` [PATCH 6/8] drm/i915: Don't calculate a new clock in ILK+ code if it is already set Ander Conselvan de Oliveira 2016-03-14 11:51 ` Maarten Lankhorst 2016-03-14 13:01 ` Ander Conselvan De Oliveira 2016-03-14 13:15 ` Maarten Lankhorst 2016-03-14 8:55 ` [PATCH 7/8] drm/i915: Remove PCH type checks from ironlake_crtc_compute_clock() Ander Conselvan de Oliveira 2016-03-14 8:55 ` [PATCH 8/8] drm/i915: Simplify ironlake_crtc_compute_clock() CPU eDP case Ander Conselvan de Oliveira 2016-03-14 12:01 ` Maarten Lankhorst
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.