On Mon, 04 Mar 2019, Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx> wrote:
> In order to make it easier to bring up new platforms
> without having to take care about all corner cases
> that was previously taken care for previous platforms
> we already use comparative INTEL_GEN statements.
>
> Let's start doing the same with PCH.
>
> The only caveats are:
> - for less-than comparisons we need to be careful
> and check PCH_NONE < pch < PCH_CNP.
> - It is not necessarily a chronological order, but a matter
> of south display compatibility/inheritance.
This scares me a bit, but I understand the reasons. Maybe we need an
IS_PCH_RANGE() macro to complement IS_GEN_RANGE(). But that can come
later as we see how this evolves.
Some notes below.
>
> Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> Cc: Lucas De Marchi <lucas.demarchi@xxxxxxxxx>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
> ---
> drivers/gpu/drm/i915/i915_drv.h | 6 ++++++
> drivers/gpu/drm/i915/i915_irq.c | 7 ++-----
> drivers/gpu/drm/i915/intel_cdclk.c | 2 +-
> drivers/gpu/drm/i915/intel_dp.c | 21 +++++++++------------
> drivers/gpu/drm/i915/intel_panel.c | 5 ++---
> 5 files changed, 20 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index e6be327ba86d..e327736c76a0 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -523,6 +523,12 @@ struct i915_psr {
> u16 su_x_granularity;
> };
>
> +/*
> + * Sorted by south display engine compatibility.
> + * If the new PCH comes with a south display engine that is not
> + * inherited from the latest item, please do not add it to the
> + * end. Instead, add it right after its "parent" PCH.
> + */
> enum intel_pch {
> PCH_NOP = -1, /* PCH without south display */
> PCH_NONE = 0, /* No PCH present */
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index a42eb6394b69..923135d6b781 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -2833,9 +2833,7 @@ gen8_de_irq_handler(struct drm_i915_private *dev_priv, u32 master_ctl)
>
> if (HAS_PCH_ICP(dev_priv))
PCH_TYPE >= ICP?
> icp_irq_handler(dev_priv, iir);
> - else if (HAS_PCH_SPT(dev_priv) ||
> - HAS_PCH_KBP(dev_priv) ||
> - HAS_PCH_CNP(dev_priv))
> + else if (INTEL_PCH_TYPE(dev_priv) >= PCH_SPT)
> spt_irq_handler(dev_priv, iir);
> else
> cpt_irq_handler(dev_priv, iir);
> @@ -4620,8 +4618,7 @@ void intel_irq_init(struct drm_i915_private *dev_priv)
> dev->driver->disable_vblank = gen8_disable_vblank;
> if (IS_GEN9_LP(dev_priv))
> dev_priv->display.hpd_irq_setup = bxt_hpd_irq_setup;
> - else if (HAS_PCH_SPT(dev_priv) || HAS_PCH_KBP(dev_priv) ||
> - HAS_PCH_CNP(dev_priv))
> + else if (INTEL_PCH_TYPE(dev_priv) >= PCH_SPT)
> dev_priv->display.hpd_irq_setup = spt_hpd_irq_setup;
> else
> dev_priv->display.hpd_irq_setup = ilk_hpd_irq_setup;
> diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c
> index 7e5132772477..9d236e4ed26a 100644
> --- a/drivers/gpu/drm/i915/intel_cdclk.c
> +++ b/drivers/gpu/drm/i915/intel_cdclk.c
> @@ -2723,7 +2723,7 @@ static int g4x_hrawclk(struct drm_i915_private *dev_priv)
> */
> void intel_update_rawclk(struct drm_i915_private *dev_priv)
> {
> - if (HAS_PCH_CNP(dev_priv) || HAS_PCH_ICP(dev_priv))
> + if (INTEL_PCH_TYPE(dev_priv) >= PCH_CNP)
> dev_priv->rawclk_freq = cnp_rawclk(dev_priv);
> else if (HAS_PCH_SPLIT(dev_priv))
> dev_priv->rawclk_freq = pch_rawclk(dev_priv);
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index e1a051c0fbfe..acd2336bb214 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -949,8 +949,8 @@ static void intel_pps_get_registers(struct intel_dp *intel_dp,
> regs->pp_stat = PP_STATUS(pps_idx);
> regs->pp_on = PP_ON_DELAYS(pps_idx);
> regs->pp_off = PP_OFF_DELAYS(pps_idx);
> - if (!IS_GEN9_LP(dev_priv) && !HAS_PCH_CNP(dev_priv) &&
> - !HAS_PCH_ICP(dev_priv))
> + if (INTEL_PCH_TYPE(dev_priv) > PCH_NONE &&
> + INTEL_PCH_TYPE(dev_priv) < PCH_CNP)
This is not right, starts to require PCH.
> regs->pp_div = PP_DIVISOR(pps_idx);
> }
>
> @@ -6431,8 +6431,8 @@ intel_pps_readout_hw_state(struct intel_dp *intel_dp, struct edp_power_seq *seq)
>
> pp_on = I915_READ(regs.pp_on);
> pp_off = I915_READ(regs.pp_off);
> - if (!IS_GEN9_LP(dev_priv) && !HAS_PCH_CNP(dev_priv) &&
> - !HAS_PCH_ICP(dev_priv)) {
> + if (INTEL_PCH_TYPE(dev_priv) > PCH_NONE &&
> + INTEL_PCH_TYPE(dev_priv) < PCH_CNP) {
Ditto.
> I915_WRITE(regs.pp_ctrl, pp_ctl);
> pp_div = I915_READ(regs.pp_div);
> }
> @@ -6450,8 +6450,7 @@ intel_pps_readout_hw_state(struct intel_dp *intel_dp, struct edp_power_seq *seq)
> seq->t10 = (pp_off & PANEL_POWER_DOWN_DELAY_MASK) >>
> PANEL_POWER_DOWN_DELAY_SHIFT;
>
> - if (IS_GEN9_LP(dev_priv) || HAS_PCH_CNP(dev_priv) ||
> - HAS_PCH_ICP(dev_priv)) {
> + if (IS_GEN9_LP(dev_priv) || INTEL_PCH_TYPE(dev_priv) >= PCH_CNP) {
> seq->t11_t12 = ((pp_ctl & BXT_POWER_CYCLE_DELAY_MASK) >>
> BXT_POWER_CYCLE_DELAY_SHIFT) * 1000;
> } else {
> @@ -6622,8 +6621,7 @@ intel_dp_init_panel_power_sequencer_registers(struct intel_dp *intel_dp,
> (seq->t10 << PANEL_POWER_DOWN_DELAY_SHIFT);
> /* Compute the divisor for the pp clock, simply match the Bspec
> * formula. */
> - if (IS_GEN9_LP(dev_priv) || HAS_PCH_CNP(dev_priv) ||
> - HAS_PCH_ICP(dev_priv)) {
> + if (IS_GEN9_LP(dev_priv) || INTEL_PCH_TYPE(dev_priv) >= PCH_CNP) {
> pp_div = I915_READ(regs.pp_ctrl);
> pp_div &= ~BXT_POWER_CYCLE_DELAY_MASK;
> pp_div |= (DIV_ROUND_UP(seq->t11_t12, 1000)
> @@ -6659,8 +6657,7 @@ intel_dp_init_panel_power_sequencer_registers(struct intel_dp *intel_dp,
>
> I915_WRITE(regs.pp_on, pp_on);
> I915_WRITE(regs.pp_off, pp_off);
> - if (IS_GEN9_LP(dev_priv) || HAS_PCH_CNP(dev_priv) ||
> - HAS_PCH_ICP(dev_priv))
> + if (IS_GEN9_LP(dev_priv) || INTEL_PCH_TYPE(dev_priv) >= PCH_CNP)
> I915_WRITE(regs.pp_ctrl, pp_div);
> else
> I915_WRITE(regs.pp_div, pp_div);
> @@ -6668,8 +6665,8 @@ intel_dp_init_panel_power_sequencer_registers(struct intel_dp *intel_dp,
> DRM_DEBUG_KMS("panel power sequencer register settings: PP_ON %#x, PP_OFF %#x, PP_DIV %#x\n",
> I915_READ(regs.pp_on),
> I915_READ(regs.pp_off),
> - (IS_GEN9_LP(dev_priv) || HAS_PCH_CNP(dev_priv) ||
> - HAS_PCH_ICP(dev_priv)) ?
> + (IS_GEN9_LP(dev_priv) ||
> + INTEL_PCH_TYPE(dev_priv) >= PCH_CNP) ?
> (I915_READ(regs.pp_ctrl) & BXT_POWER_CYCLE_DELAY_MASK) :
> I915_READ(regs.pp_div));
I think this was all pretty ugly in intel_dp.c before, and this doesn't
make it much better.
I tried to clean it up [1], please consider reviewing those and having
them merged first, after which your change becomes a one-liner in this
file.
Other than that, seems fine.
BR,
Jani.
[1] https://patchwork.freedesktop.org/series/57579/
> }
> diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
> index beca98d2b035..edd5540639b0 100644
> --- a/drivers/gpu/drm/i915/intel_panel.c
> +++ b/drivers/gpu/drm/i915/intel_panel.c
> @@ -1894,15 +1894,14 @@ intel_panel_init_backlight_funcs(struct intel_panel *panel)
> panel->backlight.set = bxt_set_backlight;
> panel->backlight.get = bxt_get_backlight;
> panel->backlight.hz_to_pwm = bxt_hz_to_pwm;
> - } else if (HAS_PCH_CNP(dev_priv) || HAS_PCH_ICP(dev_priv)) {
> + } else if (INTEL_PCH_TYPE(dev_priv) >= PCH_CNP) {
> panel->backlight.setup = cnp_setup_backlight;
> panel->backlight.enable = cnp_enable_backlight;
> panel->backlight.disable = cnp_disable_backlight;
> panel->backlight.set = bxt_set_backlight;
> panel->backlight.get = bxt_get_backlight;
> panel->backlight.hz_to_pwm = cnp_hz_to_pwm;
> - } else if (HAS_PCH_LPT(dev_priv) || HAS_PCH_SPT(dev_priv) ||
> - HAS_PCH_KBP(dev_priv)) {
> + } else if (INTEL_PCH_TYPE(dev_priv) >= PCH_LPT) {
> panel->backlight.setup = lpt_setup_backlight;
> panel->backlight.enable = lpt_enable_backlight;
> panel->backlight.disable = lpt_disable_backlight;
--
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
