From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: Jani Nikula <jani.nikula@intel.com>
Cc: intel-gfx@lists.freedesktop.org, Dave Airlie <airlied@gmail.com>,
Dave Airlie <airlied@redhat.com>,
"Sarvela, Tomi P" <tomi.p.sarvela@intel.com>
Subject: Re: [Intel-gfx] [PATCH 01/24] drm/i915/uncore: split the fw get function into separate vfunc
Date: Sat, 2 Oct 2021 22:27:13 +0300 [thread overview]
Message-ID: <YViykYGH9KG1B2f/@intel.com> (raw)
In-Reply-To: <41c3e1896162fc08e22e40b00642791365a8c00e.1632869550.git.jani.nikula@intel.com>
On Wed, Sep 29, 2021 at 01:57:45AM +0300, Jani Nikula wrote:
> From: Dave Airlie <airlied@redhat.com>
>
> constify it while here. drop the put function since it was never
> overloaded and always has done the same thing, no point in
> indirecting it for show.
>
> Reviewed-by: Jani Nikula <jani.nikula@intel.com>
> Signed-off-by: Dave Airlie <airlied@redhat.com>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
This has totally broken snb and ivb machines. Total death
ensues somewhere in uncore init after some backtraces fly by.
Didn't get any logs out to disk unfortunately. Please revert.
Sadly CI is still afraid to report when machines disappear.
For the bat report you at least get a list of machines that
were awol, but the shard run seems to not even mention that
all snbs suddenly vanished.
I've said it before and I'll say it again. We really should
*not* be loading i915 when the machine boots. That way we'd
at least get the machine up and running and can report that
loading i915 is the thing that killed it...
> ---
> drivers/gpu/drm/i915/intel_uncore.c | 70 ++++++++++++++++-------------
> drivers/gpu/drm/i915/intel_uncore.h | 7 +--
> 2 files changed, 43 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index 678a99de07fe..aee572af8052 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -36,6 +36,12 @@
>
> #define __raw_posting_read(...) ((void)__raw_uncore_read32(__VA_ARGS__))
>
> +static void
> +fw_domains_get(struct intel_uncore *uncore, enum forcewake_domains fw_domains)
> +{
> + uncore->fw_get_funcs->force_wake_get(uncore, fw_domains);
> +}
> +
> void
> intel_uncore_mmio_debug_init_early(struct intel_uncore_mmio_debug *mmio_debug)
> {
> @@ -248,7 +254,7 @@ fw_domain_put(const struct intel_uncore_forcewake_domain *d)
> }
>
> static void
> -fw_domains_get(struct intel_uncore *uncore, enum forcewake_domains fw_domains)
> +fw_domains_get_normal(struct intel_uncore *uncore, enum forcewake_domains fw_domains)
> {
> struct intel_uncore_forcewake_domain *d;
> unsigned int tmp;
> @@ -396,7 +402,7 @@ intel_uncore_fw_release_timer(struct hrtimer *timer)
>
> GEM_BUG_ON(!domain->wake_count);
> if (--domain->wake_count == 0)
> - uncore->funcs.force_wake_put(uncore, domain->mask);
> + fw_domains_put(uncore, domain->mask);
>
> spin_unlock_irqrestore(&uncore->lock, irqflags);
>
> @@ -454,7 +460,7 @@ intel_uncore_forcewake_reset(struct intel_uncore *uncore)
>
> fw = uncore->fw_domains_active;
> if (fw)
> - uncore->funcs.force_wake_put(uncore, fw);
> + fw_domains_put(uncore, fw);
>
> fw_domains_reset(uncore, uncore->fw_domains);
> assert_forcewakes_inactive(uncore);
> @@ -562,7 +568,7 @@ static void forcewake_early_sanitize(struct intel_uncore *uncore,
> intel_uncore_forcewake_reset(uncore);
> if (restore_forcewake) {
> spin_lock_irq(&uncore->lock);
> - uncore->funcs.force_wake_get(uncore, restore_forcewake);
> + fw_domains_get(uncore, restore_forcewake);
>
> if (intel_uncore_has_fifo(uncore))
> uncore->fifo_count = fifo_free_entries(uncore);
> @@ -623,7 +629,7 @@ static void __intel_uncore_forcewake_get(struct intel_uncore *uncore,
> }
>
> if (fw_domains)
> - uncore->funcs.force_wake_get(uncore, fw_domains);
> + fw_domains_get(uncore, fw_domains);
> }
>
> /**
> @@ -644,7 +650,7 @@ void intel_uncore_forcewake_get(struct intel_uncore *uncore,
> {
> unsigned long irqflags;
>
> - if (!uncore->funcs.force_wake_get)
> + if (!uncore->fw_get_funcs)
> return;
>
> assert_rpm_wakelock_held(uncore->rpm);
> @@ -711,7 +717,7 @@ void intel_uncore_forcewake_get__locked(struct intel_uncore *uncore,
> {
> lockdep_assert_held(&uncore->lock);
>
> - if (!uncore->funcs.force_wake_get)
> + if (!uncore->fw_get_funcs)
> return;
>
> __intel_uncore_forcewake_get(uncore, fw_domains);
> @@ -733,7 +739,7 @@ static void __intel_uncore_forcewake_put(struct intel_uncore *uncore,
> continue;
> }
>
> - uncore->funcs.force_wake_put(uncore, domain->mask);
> + fw_domains_put(uncore, domain->mask);
> }
> }
>
> @@ -750,7 +756,7 @@ void intel_uncore_forcewake_put(struct intel_uncore *uncore,
> {
> unsigned long irqflags;
>
> - if (!uncore->funcs.force_wake_put)
> + if (!uncore->fw_get_funcs)
> return;
>
> spin_lock_irqsave(&uncore->lock, irqflags);
> @@ -769,7 +775,7 @@ void intel_uncore_forcewake_flush(struct intel_uncore *uncore,
> struct intel_uncore_forcewake_domain *domain;
> unsigned int tmp;
>
> - if (!uncore->funcs.force_wake_put)
> + if (!uncore->fw_get_funcs)
> return;
>
> fw_domains &= uncore->fw_domains;
> @@ -793,7 +799,7 @@ void intel_uncore_forcewake_put__locked(struct intel_uncore *uncore,
> {
> lockdep_assert_held(&uncore->lock);
>
> - if (!uncore->funcs.force_wake_put)
> + if (!uncore->fw_get_funcs)
> return;
>
> __intel_uncore_forcewake_put(uncore, fw_domains);
> @@ -801,7 +807,7 @@ void intel_uncore_forcewake_put__locked(struct intel_uncore *uncore,
>
> void assert_forcewakes_inactive(struct intel_uncore *uncore)
> {
> - if (!uncore->funcs.force_wake_get)
> + if (!uncore->fw_get_funcs)
> return;
>
> drm_WARN(&uncore->i915->drm, uncore->fw_domains_active,
> @@ -818,7 +824,7 @@ void assert_forcewakes_active(struct intel_uncore *uncore,
> if (!IS_ENABLED(CONFIG_DRM_I915_DEBUG_RUNTIME_PM))
> return;
>
> - if (!uncore->funcs.force_wake_get)
> + if (!uncore->fw_get_funcs)
> return;
>
> spin_lock_irq(&uncore->lock);
> @@ -1587,7 +1593,7 @@ static noinline void ___force_wake_auto(struct intel_uncore *uncore,
> for_each_fw_domain_masked(domain, fw_domains, uncore, tmp)
> fw_domain_arm_timer(domain);
>
> - uncore->funcs.force_wake_get(uncore, fw_domains);
> + fw_domains_get(uncore, fw_domains);
> }
>
> static inline void __force_wake_auto(struct intel_uncore *uncore,
> @@ -1837,6 +1843,18 @@ static void intel_uncore_fw_domains_fini(struct intel_uncore *uncore)
> fw_domain_fini(uncore, d->id);
> }
>
> +static const struct intel_uncore_fw_get uncore_get_fallback = {
> + .force_wake_get = fw_domains_get_with_fallback
> +};
> +
> +static const struct intel_uncore_fw_get uncore_get_normal = {
> + .force_wake_get = fw_domains_get_normal,
> +};
> +
> +static const struct intel_uncore_fw_get uncore_get_thread_status = {
> + .force_wake_get = fw_domains_get_with_thread_status
> +};
> +
> static int intel_uncore_fw_domains_init(struct intel_uncore *uncore)
> {
> struct drm_i915_private *i915 = uncore->i915;
> @@ -1852,8 +1870,7 @@ static int intel_uncore_fw_domains_init(struct intel_uncore *uncore)
> intel_engine_mask_t emask = INTEL_INFO(i915)->platform_engine_mask;
> int i;
>
> - uncore->funcs.force_wake_get = fw_domains_get_with_fallback;
> - uncore->funcs.force_wake_put = fw_domains_put;
> + uncore->fw_get_funcs = &uncore_get_fallback;
> fw_domain_init(uncore, FW_DOMAIN_ID_RENDER,
> FORCEWAKE_RENDER_GEN9,
> FORCEWAKE_ACK_RENDER_GEN9);
> @@ -1878,8 +1895,7 @@ static int intel_uncore_fw_domains_init(struct intel_uncore *uncore)
> FORCEWAKE_ACK_MEDIA_VEBOX_GEN11(i));
> }
> } else if (IS_GRAPHICS_VER(i915, 9, 10)) {
> - uncore->funcs.force_wake_get = fw_domains_get_with_fallback;
> - uncore->funcs.force_wake_put = fw_domains_put;
> + uncore->fw_get_funcs = &uncore_get_fallback;
> fw_domain_init(uncore, FW_DOMAIN_ID_RENDER,
> FORCEWAKE_RENDER_GEN9,
> FORCEWAKE_ACK_RENDER_GEN9);
> @@ -1889,16 +1905,13 @@ static int intel_uncore_fw_domains_init(struct intel_uncore *uncore)
> fw_domain_init(uncore, FW_DOMAIN_ID_MEDIA,
> FORCEWAKE_MEDIA_GEN9, FORCEWAKE_ACK_MEDIA_GEN9);
> } else if (IS_VALLEYVIEW(i915) || IS_CHERRYVIEW(i915)) {
> - uncore->funcs.force_wake_get = fw_domains_get;
> - uncore->funcs.force_wake_put = fw_domains_put;
> + uncore->fw_get_funcs = &uncore_get_normal;
> fw_domain_init(uncore, FW_DOMAIN_ID_RENDER,
> FORCEWAKE_VLV, FORCEWAKE_ACK_VLV);
> fw_domain_init(uncore, FW_DOMAIN_ID_MEDIA,
> FORCEWAKE_MEDIA_VLV, FORCEWAKE_ACK_MEDIA_VLV);
> } else if (IS_HASWELL(i915) || IS_BROADWELL(i915)) {
> - uncore->funcs.force_wake_get =
> - fw_domains_get_with_thread_status;
> - uncore->funcs.force_wake_put = fw_domains_put;
> + uncore->fw_get_funcs = &uncore_get_thread_status;
> fw_domain_init(uncore, FW_DOMAIN_ID_RENDER,
> FORCEWAKE_MT, FORCEWAKE_ACK_HSW);
> } else if (IS_IVYBRIDGE(i915)) {
> @@ -1913,9 +1926,7 @@ static int intel_uncore_fw_domains_init(struct intel_uncore *uncore)
> * (correctly) interpreted by the test below as MT
> * forcewake being disabled.
> */
> - uncore->funcs.force_wake_get =
> - fw_domains_get_with_thread_status;
> - uncore->funcs.force_wake_put = fw_domains_put;
> + uncore->fw_get_funcs = &uncore_get_thread_status;
>
> /* We need to init first for ECOBUS access and then
> * determine later if we want to reinit, in case of MT access is
> @@ -1946,9 +1957,7 @@ static int intel_uncore_fw_domains_init(struct intel_uncore *uncore)
> FORCEWAKE, FORCEWAKE_ACK);
> }
> } else if (GRAPHICS_VER(i915) == 6) {
> - uncore->funcs.force_wake_get =
> - fw_domains_get_with_thread_status;
> - uncore->funcs.force_wake_put = fw_domains_put;
> + uncore->fw_get_funcs = &uncore_get_thread_status;
> fw_domain_init(uncore, FW_DOMAIN_ID_RENDER,
> FORCEWAKE, FORCEWAKE_ACK);
> }
> @@ -2165,8 +2174,7 @@ int intel_uncore_init_mmio(struct intel_uncore *uncore)
> }
>
> /* make sure fw funcs are set if and only if we have fw*/
> - GEM_BUG_ON(intel_uncore_has_forcewake(uncore) != !!uncore->funcs.force_wake_get);
> - GEM_BUG_ON(intel_uncore_has_forcewake(uncore) != !!uncore->funcs.force_wake_put);
> + GEM_BUG_ON(intel_uncore_has_forcewake(uncore) != !!uncore->fw_get_funcs);
> GEM_BUG_ON(intel_uncore_has_forcewake(uncore) != !!uncore->funcs.read_fw_domains);
> GEM_BUG_ON(intel_uncore_has_forcewake(uncore) != !!uncore->funcs.write_fw_domains);
>
> diff --git a/drivers/gpu/drm/i915/intel_uncore.h b/drivers/gpu/drm/i915/intel_uncore.h
> index 2f31c50eeae2..3248e4e2c540 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.h
> +++ b/drivers/gpu/drm/i915/intel_uncore.h
> @@ -84,12 +84,12 @@ enum forcewake_domains {
> FORCEWAKE_ALL = BIT(FW_DOMAIN_ID_COUNT) - 1,
> };
>
> -struct intel_uncore_funcs {
> +struct intel_uncore_fw_get {
> void (*force_wake_get)(struct intel_uncore *uncore,
> enum forcewake_domains domains);
> - void (*force_wake_put)(struct intel_uncore *uncore,
> - enum forcewake_domains domains);
> +};
>
> +struct intel_uncore_funcs {
> enum forcewake_domains (*read_fw_domains)(struct intel_uncore *uncore,
> i915_reg_t r);
> enum forcewake_domains (*write_fw_domains)(struct intel_uncore *uncore,
> @@ -150,6 +150,7 @@ struct intel_uncore {
> unsigned int shadowed_reg_table_entries;
>
> struct notifier_block pmic_bus_access_nb;
> + const struct intel_uncore_fw_get *fw_get_funcs;
> struct intel_uncore_funcs funcs;
>
> unsigned int fifo_count;
> --
> 2.30.2
--
Ville Syrjälä
Intel
next prev parent reply other threads:[~2021-10-02 19:27 UTC|newest]
Thread overview: 36+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-09-28 22:57 [Intel-gfx] [PATCH 00/24] i915/display: split and constify vtable, again Jani Nikula
2021-09-28 22:57 ` [Intel-gfx] [PATCH 01/24] drm/i915/uncore: split the fw get function into separate vfunc Jani Nikula
2021-10-02 19:27 ` Ville Syrjälä [this message]
2021-10-04 0:33 ` Dave Airlie
2021-10-04 7:03 ` Saarinen, Jani
2021-10-04 7:45 ` Jani Nikula
2021-10-04 7:44 ` Sarvela, Tomi P
2021-09-28 22:57 ` [Intel-gfx] [PATCH 02/24] drm/i915/pm: drop get_fifo_size vfunc Jani Nikula
2021-09-28 22:57 ` [Intel-gfx] [PATCH 03/24] drm/i915: make update_wm take a dev_priv Jani Nikula
2021-09-28 22:57 ` [Intel-gfx] [PATCH 04/24] drm/i915/wm: provide wrappers around watermark vfuncs calls (v3) Jani Nikula
2021-09-28 22:57 ` [Intel-gfx] [PATCH 05/24] drm/i915: add wrappers around cdclk vtable funcs Jani Nikula
2021-09-28 22:57 ` [Intel-gfx] [PATCH 06/24] drm/i915/display: add intel_fdi_link_train wrapper Jani Nikula
2021-09-28 22:57 ` [Intel-gfx] [PATCH 07/24] drm/i915: split clock gating init from display vtable Jani Nikula
2021-09-28 22:57 ` [Intel-gfx] [PATCH 08/24] drm/i915: split watermark vfuncs " Jani Nikula
2021-09-28 22:57 ` [Intel-gfx] [PATCH 09/24] drm/i915: split color functions " Jani Nikula
2021-09-28 22:57 ` [Intel-gfx] [PATCH 10/24] drm/i915: split audio " Jani Nikula
2021-09-28 22:57 ` [Intel-gfx] [PATCH 11/24] drm/i915: split cdclk " Jani Nikula
2021-09-28 22:57 ` [Intel-gfx] [PATCH 12/24] drm/i915: split irq hotplug function " Jani Nikula
2021-09-28 22:57 ` [Intel-gfx] [PATCH 13/24] drm/i915: split fdi link training " Jani Nikula
2021-09-28 22:57 ` [Intel-gfx] [PATCH 14/24] drm/i915: split the dpll clock compute out " Jani Nikula
2021-09-28 22:57 ` [Intel-gfx] [PATCH 15/24] drm/i915: constify fdi link training vtable Jani Nikula
2021-09-28 22:58 ` [Intel-gfx] [PATCH 16/24] drm/i915: constify hotplug function vtable Jani Nikula
2021-09-28 22:58 ` [Intel-gfx] [PATCH 17/24] drm/i915: constify color " Jani Nikula
2021-09-28 22:58 ` [Intel-gfx] [PATCH 18/24] drm/i915: constify the audio " Jani Nikula
2021-09-28 22:58 ` [Intel-gfx] [PATCH 19/24] drm/i915: constify the dpll clock vtable Jani Nikula
2021-09-28 22:58 ` [Intel-gfx] [PATCH 20/24] drm/i915: constify the cdclk vtable Jani Nikula
2021-09-28 22:58 ` [Intel-gfx] [PATCH 21/24] drm/i915: drop unused function ptr and comments Jani Nikula
2021-09-28 22:58 ` [Intel-gfx] [PATCH 22/24] drm/i915: constify display function vtable Jani Nikula
2021-09-28 22:58 ` [Intel-gfx] [PATCH 23/24] drm/i915: constify clock gating init vtable Jani Nikula
2021-09-28 22:58 ` [Intel-gfx] [PATCH 24/24] drm/i915: constify display wm vtable Jani Nikula
2021-09-28 23:38 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for i915/display: split and constify vtable, again Patchwork
2021-09-29 0:09 ` [Intel-gfx] ✓ Fi.CI.BAT: success " Patchwork
2021-09-29 3:12 ` [Intel-gfx] ✓ Fi.CI.IGT: " Patchwork
2021-09-29 6:59 ` [Intel-gfx] [PATCH 00/24] " Jani Nikula
-- strict thread matches above, loose matches on Subject: below --
2021-09-22 14:29 [Intel-gfx] [PATCH 00/24] i915/display: split and constify vtable (v6) Jani Nikula
2021-09-22 14:29 ` [Intel-gfx] [PATCH 01/24] drm/i915/uncore: split the fw get function into separate vfunc Jani Nikula
2021-09-14 18:24 [Intel-gfx] [PATCH 00/24] i915/display: split and constify vtable (v5) Jani Nikula
2021-09-14 18:24 ` [Intel-gfx] [PATCH 01/24] drm/i915/uncore: split the fw get function into separate vfunc Jani Nikula
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=YViykYGH9KG1B2f/@intel.com \
--to=ville.syrjala@linux.intel.com \
--cc=airlied@gmail.com \
--cc=airlied@redhat.com \
--cc=intel-gfx@lists.freedesktop.org \
--cc=jani.nikula@intel.com \
--cc=tomi.p.sarvela@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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox