Re: [Intel-gfx] [PATCH] drm/i915: Add NO_VLV_DISP_PW_DPIO_CMN_BC_INIT quirk
On Wed, Oct 27, 2021 at 08:39:57PM +0200, Hans de Goede wrote: > Hi, > > On 10/27/21 15:38, Ville Syrjälä wrote: > > On Sun, Oct 24, 2021 at 05:50:10PM +0200, Hans de Goede wrote: > >> Add a NO_VLV_DISP_PW_DPIO_CMN_BC_INIT quirk to fix i915 not working on > >> the Xiaomi Mi Pad 2 (with CHT x5-Z8500 SoC). > >> > >> The Xiaomi Mi Pad 2 uses quite an unusual hardware-design for a Cherry > >> Trail tablet. It deviates from the typical reference design based tablets > >> in many ways. > >> > >> The Mi Pad 2 does not have any DisplayPort or HDMI outouts. I suspect that > >> as part of its unusual design it also has some supply rail which is only > >> used for DisplayPort or HDMI not connected. > > > > Do we have the VBT somewhere (preferable attached to a bug report)? > > Maybe we can avoid an ugly quirk. > > I agree that solving this in a way where we can avoid the quirk would be > great. > > I've filed an issue for this here now: > > https://gitlab.freedesktop.org/drm/intel/-/issues/4385 > > This has a dump of /sys/kernel/debug/dri/0/i915_vbt as well as > dmesg output from a boot with drm.debug=0x1e attached (from a boot > with this patch, since otherwise the system hangs). > > >> > >> Force-enabling the dpio-common-bc powerwell as the i915 normal does at boot > >> appears to cause the P-Unit to hang. When booting with a serial-usb console > >> the following errors are logged before the system freezes: > >> > >> i915 :00:02.0: [drm] *ERROR* timeout setting power well state > >> (f3ff) > >> i915 :00:02.0: [drm] *ERROR* Display PHY 0 is not power up > > > > Hmm. I wonder if we're missing a clock or something... > > > > Either of these do anything different? > > > > --- a/drivers/gpu/drm/i915/display/intel_display_power.c > > +++ b/drivers/gpu/drm/i915/display/intel_display_power.c > > @@ -1419,6 +1419,10 @@ static void vlv_display_power_well_init(struct > > drm_i915_private *dev_priv) > > for_each_pipe(dev_priv, pipe) { > > u32 val = intel_de_read(dev_priv, DPLL(pipe)); > > > > + val |= DPLL_SSC_REF_CLK_CHV; > > or > > + val &= ~DPLL_SSC_REF_CLK_CHV; > > > > val |= DPLL_REF_CLK_ENABLE_VLV | DPLL_VGA_MODE_DIS; > > if (pipe != PIPE_A) > > val |= DPLL_INTEGRATED_CRI_CLK_VLV; > > > > The hang gets triggered from chv_dpio_cmn_power_well_enable() which does not > call vlv_display_power_well_init() at all, it directly calls > vlv_set_power_well() > without first calling vlv_display_power_well_init() . > > Note the same goes for vlv_dpio_cmn_power_well_enable(). Only the > vlv_display_power_well_enable() / chv_pipe_power_well_enable() call > vlv_display_power_well_init(). > > Note I can still give the suggested change a try if you want, > the "display" powerwell is listed first and has DOMAIN_INIT set, > so assuming for_each_power_domain_well() goes through the domains in > the order they are listed, then vlv_display_power_well_init() will > still run first. But it would seem to be wrong if enabling one domain > depends on things setup by another domain ? The power wells are hierarchical. Also power wells != power domains. -- Ville Syrjälä Intel
Re: [Intel-gfx] [PATCH] drm/i915: Add NO_VLV_DISP_PW_DPIO_CMN_BC_INIT quirk
Hi, On 10/27/21 15:38, Ville Syrjälä wrote: > On Sun, Oct 24, 2021 at 05:50:10PM +0200, Hans de Goede wrote: >> Add a NO_VLV_DISP_PW_DPIO_CMN_BC_INIT quirk to fix i915 not working on >> the Xiaomi Mi Pad 2 (with CHT x5-Z8500 SoC). >> >> The Xiaomi Mi Pad 2 uses quite an unusual hardware-design for a Cherry >> Trail tablet. It deviates from the typical reference design based tablets >> in many ways. >> >> The Mi Pad 2 does not have any DisplayPort or HDMI outouts. I suspect that >> as part of its unusual design it also has some supply rail which is only >> used for DisplayPort or HDMI not connected. > > Do we have the VBT somewhere (preferable attached to a bug report)? > Maybe we can avoid an ugly quirk. I agree that solving this in a way where we can avoid the quirk would be great. I've filed an issue for this here now: https://gitlab.freedesktop.org/drm/intel/-/issues/4385 This has a dump of /sys/kernel/debug/dri/0/i915_vbt as well as dmesg output from a boot with drm.debug=0x1e attached (from a boot with this patch, since otherwise the system hangs). >> >> Force-enabling the dpio-common-bc powerwell as the i915 normal does at boot >> appears to cause the P-Unit to hang. When booting with a serial-usb console >> the following errors are logged before the system freezes: >> >> i915 :00:02.0: [drm] *ERROR* timeout setting power well state >> (f3ff) >> i915 :00:02.0: [drm] *ERROR* Display PHY 0 is not power up > > Hmm. I wonder if we're missing a clock or something... > > Either of these do anything different? > > --- a/drivers/gpu/drm/i915/display/intel_display_power.c > +++ b/drivers/gpu/drm/i915/display/intel_display_power.c > @@ -1419,6 +1419,10 @@ static void vlv_display_power_well_init(struct > drm_i915_private *dev_priv) > for_each_pipe(dev_priv, pipe) { > u32 val = intel_de_read(dev_priv, DPLL(pipe)); > > + val |= DPLL_SSC_REF_CLK_CHV; > or > + val &= ~DPLL_SSC_REF_CLK_CHV; > > val |= DPLL_REF_CLK_ENABLE_VLV | DPLL_VGA_MODE_DIS; > if (pipe != PIPE_A) > val |= DPLL_INTEGRATED_CRI_CLK_VLV; > The hang gets triggered from chv_dpio_cmn_power_well_enable() which does not call vlv_display_power_well_init() at all, it directly calls vlv_set_power_well() without first calling vlv_display_power_well_init() . Note the same goes for vlv_dpio_cmn_power_well_enable(). Only the vlv_display_power_well_enable() / chv_pipe_power_well_enable() call vlv_display_power_well_init(). Note I can still give the suggested change a try if you want, the "display" powerwell is listed first and has DOMAIN_INIT set, so assuming for_each_power_domain_well() goes through the domains in the order they are listed, then vlv_display_power_well_init() will still run first. But it would seem to be wrong if enabling one domain depends on things setup by another domain ? Regards, Hans
Re: [Intel-gfx] [PATCH] drm/i915: Add NO_VLV_DISP_PW_DPIO_CMN_BC_INIT quirk
On Sun, Oct 24, 2021 at 05:50:10PM +0200, Hans de Goede wrote: > Add a NO_VLV_DISP_PW_DPIO_CMN_BC_INIT quirk to fix i915 not working on > the Xiaomi Mi Pad 2 (with CHT x5-Z8500 SoC). > > The Xiaomi Mi Pad 2 uses quite an unusual hardware-design for a Cherry > Trail tablet. It deviates from the typical reference design based tablets > in many ways. > > The Mi Pad 2 does not have any DisplayPort or HDMI outouts. I suspect that > as part of its unusual design it also has some supply rail which is only > used for DisplayPort or HDMI not connected. Do we have the VBT somewhere (preferable attached to a bug report)? Maybe we can avoid an ugly quirk. > > Force-enabling the dpio-common-bc powerwell as the i915 normal does at boot > appears to cause the P-Unit to hang. When booting with a serial-usb console > the following errors are logged before the system freezes: > > i915 :00:02.0: [drm] *ERROR* timeout setting power well state > (f3ff) > i915 :00:02.0: [drm] *ERROR* Display PHY 0 is not power up Hmm. I wonder if we're missing a clock or something... Either of these do anything different? --- a/drivers/gpu/drm/i915/display/intel_display_power.c +++ b/drivers/gpu/drm/i915/display/intel_display_power.c @@ -1419,6 +1419,10 @@ static void vlv_display_power_well_init(struct drm_i915_private *dev_priv) for_each_pipe(dev_priv, pipe) { u32 val = intel_de_read(dev_priv, DPLL(pipe)); + val |= DPLL_SSC_REF_CLK_CHV; or + val &= ~DPLL_SSC_REF_CLK_CHV; val |= DPLL_REF_CLK_ENABLE_VLV | DPLL_VGA_MODE_DIS; if (pipe != PIPE_A) val |= DPLL_INTEGRATED_CRI_CLK_VLV; -- Ville Syrjälä Intel
Re: [Intel-gfx] [PATCH] drm/i915: Add NO_VLV_DISP_PW_DPIO_CMN_BC_INIT quirk
Hi, On 10/25/21 10:25, Jani Nikula wrote: > On Sun, 24 Oct 2021, Hans de Goede wrote: >> Add a NO_VLV_DISP_PW_DPIO_CMN_BC_INIT quirk to fix i915 not working on >> the Xiaomi Mi Pad 2 (with CHT x5-Z8500 SoC). >> >> The Xiaomi Mi Pad 2 uses quite an unusual hardware-design for a Cherry >> Trail tablet. It deviates from the typical reference design based tablets >> in many ways. >> >> The Mi Pad 2 does not have any DisplayPort or HDMI outouts. I suspect that >> as part of its unusual design it also has some supply rail which is only >> used for DisplayPort or HDMI not connected. >> >> Force-enabling the dpio-common-bc powerwell as the i915 normal does at boot >> appears to cause the P-Unit to hang. When booting with a serial-usb console >> the following errors are logged before the system freezes: >> >> i915 :00:02.0: [drm] *ERROR* timeout setting power well state >> (f3ff) >> i915 :00:02.0: [drm] *ERROR* Display PHY 0 is not power up >> [ cut here ] >> i915 :00:02.0: DPIO read pipe A reg 0x8170 == 0x >> WARNING: CPU: 3 PID: 258 at drivers/gpu/drm/i915/intel_sideband.c:257 >> vlv_dpio_read+0x95/0xb0 [i915] >> ... >> Call Trace: >> chv_dpio_cmn_power_well_enable+0xab/0x210 [i915] >> __intel_display_power_get_domain.part.0+0xa0/0xc0 [i915] >> intel_power_domains_init_hw+0x26d/0x760 [i915] >> intel_modeset_init_noirq+0x5d/0x270 [i915] >> i915_driver_probe+0x6b6/0xd10 [i915] >> ... >> >> If I disable the WARN about the register being 0x, so that the >> system can log some more dmesg output over the serial console before >> freezing, the following errors are also logged: >> >> i915 :00:02.0: [drm] *ERROR* timeout setting power well state >> (fcfff3ff) >> i915 :00:02.0: [drm] *ERROR* Display PHY 1 is not power up >> >> With this patch to disable the force-enabling of the PHY 0 / dpio-common-bc >> powerwell in place, this error for PHY 1 goes away. So it seems that trying >> the force-enabling of the PHY 0 / dpio-common-bc powerwell freezes the >> P-Unit, causing the subsequent enabling of PHY 1 to also fail (and causing >> the entire system to freeze within seconds). >> >> With this patch the PHY 1 error disappears and the entire system works. >> >> Note this change also moves the intel_init_quirks() call a bit up inside >> intel_modeset_init_noirq() this is necessary so that the quirk is set >> before the intel_power_domains_init_hw() call. This is harmless, all that >> intel_init_quirks() does is set some bits in drm_i915_private.quirks and >> make some drm_info() log calls. >> >> Reported-by: Tsuchiya Yuto >> Signed-off-by: Hans de Goede >> --- >> drivers/gpu/drm/i915/display/intel_display.c | 4 ++-- >> .../gpu/drm/i915/display/intel_display_power.c | 16 ++-- >> drivers/gpu/drm/i915/display/intel_quirks.c | 10 ++ >> drivers/gpu/drm/i915/i915_drv.h | 1 + >> 4 files changed, 27 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/display/intel_display.c >> b/drivers/gpu/drm/i915/display/intel_display.c >> index 015854b5078c..1fb885cc86c9 100644 >> --- a/drivers/gpu/drm/i915/display/intel_display.c >> +++ b/drivers/gpu/drm/i915/display/intel_display.c >> @@ -12467,6 +12467,8 @@ int intel_modeset_init_noirq(struct drm_i915_private >> *i915) >> if (ret) >> goto cleanup_bios; >> >> +intel_init_quirks(i915); >> + >> /* FIXME: completely on the wrong abstraction layer */ >> intel_power_domains_init_hw(i915, false); >> >> @@ -12501,8 +12503,6 @@ int intel_modeset_init_noirq(struct drm_i915_private >> *i915) >> INIT_WORK(>atomic_helper.free_work, >>intel_atomic_helper_free_state_worker); >> >> -intel_init_quirks(i915); >> - >> intel_fbc_init(i915); >> >> return 0; >> diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c >> b/drivers/gpu/drm/i915/display/intel_display_power.c >> index cce1a926fcc1..eeaba3dc064b 100644 >> --- a/drivers/gpu/drm/i915/display/intel_display_power.c >> +++ b/drivers/gpu/drm/i915/display/intel_display_power.c >> @@ -2090,8 +2090,14 @@ __intel_display_power_get_domain(struct >> drm_i915_private *dev_priv, >> if (intel_display_power_grab_async_put_ref(dev_priv, domain)) >> return; >> >> -for_each_power_domain_well(dev_priv, power_well, BIT_ULL(domain)) >> +for_each_power_domain_well(dev_priv, power_well, BIT_ULL(domain)) { >> +if (domain == POWER_DOMAIN_INIT && >> +(dev_priv->quirks & QUIRK_NO_VLV_DISP_PW_DPIO_CMN_BC_INIT) >> && >> +power_well->desc->id == VLV_DISP_PW_DPIO_CMN_BC) >> +continue; >> + >> intel_power_well_get(dev_priv, power_well); >> +} > > Cc: Imre > > There has got to be a way to hide this better. Having this here is > unacceptable. Thank you for your quick review. For a first quick hack I just removed
Re: [Intel-gfx] [PATCH] drm/i915: Add NO_VLV_DISP_PW_DPIO_CMN_BC_INIT quirk
On Sun, 24 Oct 2021, Hans de Goede wrote: > Add a NO_VLV_DISP_PW_DPIO_CMN_BC_INIT quirk to fix i915 not working on > the Xiaomi Mi Pad 2 (with CHT x5-Z8500 SoC). > > The Xiaomi Mi Pad 2 uses quite an unusual hardware-design for a Cherry > Trail tablet. It deviates from the typical reference design based tablets > in many ways. > > The Mi Pad 2 does not have any DisplayPort or HDMI outouts. I suspect that > as part of its unusual design it also has some supply rail which is only > used for DisplayPort or HDMI not connected. > > Force-enabling the dpio-common-bc powerwell as the i915 normal does at boot > appears to cause the P-Unit to hang. When booting with a serial-usb console > the following errors are logged before the system freezes: > > i915 :00:02.0: [drm] *ERROR* timeout setting power well state > (f3ff) > i915 :00:02.0: [drm] *ERROR* Display PHY 0 is not power up > [ cut here ] > i915 :00:02.0: DPIO read pipe A reg 0x8170 == 0x > WARNING: CPU: 3 PID: 258 at drivers/gpu/drm/i915/intel_sideband.c:257 > vlv_dpio_read+0x95/0xb0 [i915] > ... > Call Trace: > chv_dpio_cmn_power_well_enable+0xab/0x210 [i915] > __intel_display_power_get_domain.part.0+0xa0/0xc0 [i915] > intel_power_domains_init_hw+0x26d/0x760 [i915] > intel_modeset_init_noirq+0x5d/0x270 [i915] > i915_driver_probe+0x6b6/0xd10 [i915] > ... > > If I disable the WARN about the register being 0x, so that the > system can log some more dmesg output over the serial console before > freezing, the following errors are also logged: > > i915 :00:02.0: [drm] *ERROR* timeout setting power well state > (fcfff3ff) > i915 :00:02.0: [drm] *ERROR* Display PHY 1 is not power up > > With this patch to disable the force-enabling of the PHY 0 / dpio-common-bc > powerwell in place, this error for PHY 1 goes away. So it seems that trying > the force-enabling of the PHY 0 / dpio-common-bc powerwell freezes the > P-Unit, causing the subsequent enabling of PHY 1 to also fail (and causing > the entire system to freeze within seconds). > > With this patch the PHY 1 error disappears and the entire system works. > > Note this change also moves the intel_init_quirks() call a bit up inside > intel_modeset_init_noirq() this is necessary so that the quirk is set > before the intel_power_domains_init_hw() call. This is harmless, all that > intel_init_quirks() does is set some bits in drm_i915_private.quirks and > make some drm_info() log calls. > > Reported-by: Tsuchiya Yuto > Signed-off-by: Hans de Goede > --- > drivers/gpu/drm/i915/display/intel_display.c | 4 ++-- > .../gpu/drm/i915/display/intel_display_power.c | 16 ++-- > drivers/gpu/drm/i915/display/intel_quirks.c | 10 ++ > drivers/gpu/drm/i915/i915_drv.h | 1 + > 4 files changed, 27 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c > b/drivers/gpu/drm/i915/display/intel_display.c > index 015854b5078c..1fb885cc86c9 100644 > --- a/drivers/gpu/drm/i915/display/intel_display.c > +++ b/drivers/gpu/drm/i915/display/intel_display.c > @@ -12467,6 +12467,8 @@ int intel_modeset_init_noirq(struct drm_i915_private > *i915) > if (ret) > goto cleanup_bios; > > + intel_init_quirks(i915); > + > /* FIXME: completely on the wrong abstraction layer */ > intel_power_domains_init_hw(i915, false); > > @@ -12501,8 +12503,6 @@ int intel_modeset_init_noirq(struct drm_i915_private > *i915) > INIT_WORK(>atomic_helper.free_work, > intel_atomic_helper_free_state_worker); > > - intel_init_quirks(i915); > - > intel_fbc_init(i915); > > return 0; > diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c > b/drivers/gpu/drm/i915/display/intel_display_power.c > index cce1a926fcc1..eeaba3dc064b 100644 > --- a/drivers/gpu/drm/i915/display/intel_display_power.c > +++ b/drivers/gpu/drm/i915/display/intel_display_power.c > @@ -2090,8 +2090,14 @@ __intel_display_power_get_domain(struct > drm_i915_private *dev_priv, > if (intel_display_power_grab_async_put_ref(dev_priv, domain)) > return; > > - for_each_power_domain_well(dev_priv, power_well, BIT_ULL(domain)) > + for_each_power_domain_well(dev_priv, power_well, BIT_ULL(domain)) { > + if (domain == POWER_DOMAIN_INIT && > + (dev_priv->quirks & QUIRK_NO_VLV_DISP_PW_DPIO_CMN_BC_INIT) > && > + power_well->desc->id == VLV_DISP_PW_DPIO_CMN_BC) > + continue; > + > intel_power_well_get(dev_priv, power_well); > + } Cc: Imre There has got to be a way to hide this better. Having this here is unacceptable. BR, Jani. > > power_domains->domain_use_count[domain]++; > } > @@ -2184,8 +2190,14 @@ __intel_display_power_put_domain(struct > drm_i915_private *dev_priv, > >