From: Jani Nikula <jani.nikula@linux.intel.com>
To: Rodrigo Vivi <rodrigo.vivi@intel.com>, intel-gfx@lists.freedesktop.org
Cc: Lucas De Marchi <lucas.demarchi@intel.com>
Subject: Re: [PATCH 3/3] drm/i915: Start using comparative INTEL_PCH_TYPE
Date: Tue, 05 Mar 2019 16:10:04 +0200 [thread overview]
Message-ID: <87zhq9bef7.fsf@intel.com> (raw)
In-Reply-To: <20190304224830.8481-3-rodrigo.vivi@intel.com>
On Mon, 04 Mar 2019, Rodrigo Vivi <rodrigo.vivi@intel.com> 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@linux.intel.com>
> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
> 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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
next prev parent reply other threads:[~2019-03-05 14:07 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-03-04 22:48 [PATCH 1/3] drm/i915/gen11+: First assume next platforms will inherit stuff Rodrigo Vivi
2019-03-04 22:48 ` [PATCH 2/3] drm/i915: Move PCH_NOP to -1 Rodrigo Vivi
2019-03-05 17:38 ` Lucas De Marchi
2019-03-04 22:48 ` [PATCH 3/3] drm/i915: Start using comparative INTEL_PCH_TYPE Rodrigo Vivi
2019-03-05 14:10 ` Jani Nikula [this message]
2019-03-05 17:08 ` Lucas De Marchi
2019-03-05 17:16 ` Jani Nikula
2019-03-05 17:42 ` Lucas De Marchi
2019-03-05 18:38 ` Rodrigo Vivi
2019-03-05 18:48 ` Lucas De Marchi
2019-03-05 19:46 ` Jani Nikula
2019-03-05 20:24 ` Rodrigo Vivi
2019-03-05 0:05 ` ✗ Fi.CI.SPARSE: warning for series starting with [1/3] drm/i915/gen11+: First assume next platforms will inherit stuff Patchwork
2019-03-05 1:12 ` ✗ Fi.CI.BAT: failure " Patchwork
2019-03-05 17:12 ` [PATCH 1/3] " Lucas De Marchi
2019-03-05 17:43 ` Tvrtko Ursulin
2019-03-05 18:36 ` Lucas De Marchi
-- strict thread matches above, loose matches on Subject: below --
2019-03-08 21:42 Rodrigo Vivi
2019-03-08 21:43 ` [PATCH 3/3] drm/i915: Start using comparative INTEL_PCH_TYPE Rodrigo Vivi
2019-03-13 17:30 ` Lucas De Marchi
2019-03-13 20:02 ` Rodrigo Vivi
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87zhq9bef7.fsf@intel.com \
--to=jani.nikula@linux.intel.com \
--cc=intel-gfx@lists.freedesktop.org \
--cc=lucas.demarchi@intel.com \
--cc=rodrigo.vivi@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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.