| Message ID | 20230422184359.56503-1-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/dsi: Always use unconditional msleep() for the panel_on_delay | expand |
Hi, On 4/22/23 22:33, Patchwork wrote: > *Patch Details* > *Series:* drm/i915/dsi: Always use unconditional msleep() for the panel_on_delay > *URL:* https://patchwork.freedesktop.org/series/116858/ <https://patchwork.freedesktop.org/series/116858/> > *State:* failure > *Details:* https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116858v1/index.html <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116858v1/index.html> > > > CI Bug Log - changes from CI_DRM_13043_full -> Patchwork_116858v1_full > > > Summary > > *FAILURE* > > Serious unknown changes coming with Patchwork_116858v1_full absolutely need to be > verified manually. > > If you think the reported changes have nothing to do with the changes > introduced in Patchwork_116858v1_full, please notify your bug team to allow them > to document this new failure mode, which will reduce false positives in CI. > > > Participating hosts (7 -> 7) > > No changes in participating hosts > > > Possible new issues > > Here are the unknown changes that may have been introduced in Patchwork_116858v1_full: > > > IGT changes > > > Possible regressions > > * igt@gem_ppgtt@blt-vs-render-ctx0: > o shard-snb: PASS <https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13043/shard-snb6/igt@gem_ppgtt@blt-vs-render-ctx0.html> -> FAIL <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116858v1/shard-snb1/igt@gem_ppgtt@blt-vs-render-ctx0.html> The only touched file is not used on snb, so this is a false positive. Regards, Hans
On Sat, Apr 22, 2023 at 08:43:59PM +0200, Hans de Goede wrote: > The intel_dsi_msleep() helper skips sleeping if the MIPI-sequences have > a version of 3 or newer and the panel is in vid-mode. > > This is based on the big comment around line 730 which starts with > "Panel enable/disable sequences from the VBT spec.", where > the "v3 video mode seq" column does not have any wait t# entries. > > Commit 6fdb335f1c9c ("drm/i915/dsi: Use unconditional msleep for > the panel_on_delay when there is no reset-deassert MIPI-sequence") > switched to a direct msleep() instead of intel_dsi_msleep() > when there is no MIPI_SEQ_DEASSERT_RESET sequence, to fix > the panel on an Acer Aspire Switch 10 E SW3-016 not turning on. > > This was done under the assumption that when there is a v3 > MIPI_SEQ_DEASSERT_RESET sequence it will take care of any > necessary delays. > > On the Nextbook Ares 8A (a Cherry Trail device like the Acer SW3-016) > there is a MIPI_SEQ_DEASSERT_RESET sequence, yet panel_on_delay > must still be honored otherwise the panel will not turn on. > > Switch to always using an unconditional msleep() for > the panel_on_delay instead of making this depend on > the presence of a MIPI_SEQ_DEASSERT_RESET sequence. I just checked what Windows does, and at least for icl+ it always waits for the panel power delays regardless of what the VBT MIPI sequences are doing. So I suspect we should just get rid of intel_dsi_msleep() entirely and do what the power sequencing delays tell us. Anything else is untested territory. If the VBT actually wanted us to skip the delays then it should really be setting them to zero. > > Fixes: 6fdb335f1c9c ("drm/i915/dsi: Use unconditional msleep for the panel_on_delay when there is no reset-deassert MIPI-sequence") > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > drivers/gpu/drm/i915/display/vlv_dsi.c | 18 +++--------------- > 1 file changed, 3 insertions(+), 15 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/vlv_dsi.c b/drivers/gpu/drm/i915/display/vlv_dsi.c > index a6d6d8b33f3f..1b87f8f5f7fd 100644 > --- a/drivers/gpu/drm/i915/display/vlv_dsi.c > +++ b/drivers/gpu/drm/i915/display/vlv_dsi.c > @@ -788,7 +788,6 @@ static void intel_dsi_pre_enable(struct intel_atomic_state *state, > { > struct intel_dsi *intel_dsi = enc_to_intel_dsi(encoder); > struct intel_crtc *crtc = to_intel_crtc(pipe_config->uapi.crtc); > - struct intel_connector *connector = to_intel_connector(conn_state->connector); > struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > enum pipe pipe = crtc->pipe; > enum port port; > @@ -836,21 +835,10 @@ static void intel_dsi_pre_enable(struct intel_atomic_state *state, > if (!IS_GEMINILAKE(dev_priv)) > intel_dsi_prepare(encoder, pipe_config); > > + /* Give the panel time to power-on and then deassert its reset */ > intel_dsi_vbt_exec_sequence(intel_dsi, MIPI_SEQ_POWER_ON); > - > - /* > - * Give the panel time to power-on and then deassert its reset. > - * Depending on the VBT MIPI sequences version the deassert-seq > - * may contain the necessary delay, intel_dsi_msleep() will skip > - * the delay in that case. If there is no deassert-seq, then an > - * unconditional msleep is used to give the panel time to power-on. > - */ > - if (connector->panel.vbt.dsi.sequence[MIPI_SEQ_DEASSERT_RESET]) { > - intel_dsi_msleep(intel_dsi, intel_dsi->panel_on_delay); > - intel_dsi_vbt_exec_sequence(intel_dsi, MIPI_SEQ_DEASSERT_RESET); > - } else { > - msleep(intel_dsi->panel_on_delay); > - } > + msleep(intel_dsi->panel_on_delay); > + intel_dsi_vbt_exec_sequence(intel_dsi, MIPI_SEQ_DEASSERT_RESET); > > if (IS_GEMINILAKE(dev_priv)) { > glk_cold_boot = glk_dsi_enable_io(encoder); > -- > 2.39.2
Hi Ville, On 4/24/23 20:34, Ville Syrjälä wrote: > On Sat, Apr 22, 2023 at 08:43:59PM +0200, Hans de Goede wrote: >> The intel_dsi_msleep() helper skips sleeping if the MIPI-sequences have >> a version of 3 or newer and the panel is in vid-mode. >> >> This is based on the big comment around line 730 which starts with >> "Panel enable/disable sequences from the VBT spec.", where >> the "v3 video mode seq" column does not have any wait t# entries. >> >> Commit 6fdb335f1c9c ("drm/i915/dsi: Use unconditional msleep for >> the panel_on_delay when there is no reset-deassert MIPI-sequence") >> switched to a direct msleep() instead of intel_dsi_msleep() >> when there is no MIPI_SEQ_DEASSERT_RESET sequence, to fix >> the panel on an Acer Aspire Switch 10 E SW3-016 not turning on. >> >> This was done under the assumption that when there is a v3 >> MIPI_SEQ_DEASSERT_RESET sequence it will take care of any >> necessary delays. >> >> On the Nextbook Ares 8A (a Cherry Trail device like the Acer SW3-016) >> there is a MIPI_SEQ_DEASSERT_RESET sequence, yet panel_on_delay >> must still be honored otherwise the panel will not turn on. >> >> Switch to always using an unconditional msleep() for >> the panel_on_delay instead of making this depend on >> the presence of a MIPI_SEQ_DEASSERT_RESET sequence. > > I just checked what Windows does, and at least for icl+ it > always waits for the panel power delays regardless of what > the VBT MIPI sequences are doing. > > So I suspect we should just get rid of intel_dsi_msleep() > entirely and do what the power sequencing delays tell us. > Anything else is untested territory. If the VBT actually > wanted us to skip the delays then it should really be > setting them to zero. So I checked and there are only 4 (before this patch) / 3 (after this patch) callers of intel_dsi_msleep(). So just getting rid of it entirely sounds good to me. Shall I prepare a v2 patch which does this ? Regards, Hans >> Fixes: 6fdb335f1c9c ("drm/i915/dsi: Use unconditional msleep for the panel_on_delay when there is no reset-deassert MIPI-sequence") >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> drivers/gpu/drm/i915/display/vlv_dsi.c | 18 +++--------------- >> 1 file changed, 3 insertions(+), 15 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/display/vlv_dsi.c b/drivers/gpu/drm/i915/display/vlv_dsi.c >> index a6d6d8b33f3f..1b87f8f5f7fd 100644 >> --- a/drivers/gpu/drm/i915/display/vlv_dsi.c >> +++ b/drivers/gpu/drm/i915/display/vlv_dsi.c >> @@ -788,7 +788,6 @@ static void intel_dsi_pre_enable(struct intel_atomic_state *state, >> { >> struct intel_dsi *intel_dsi = enc_to_intel_dsi(encoder); >> struct intel_crtc *crtc = to_intel_crtc(pipe_config->uapi.crtc); >> - struct intel_connector *connector = to_intel_connector(conn_state->connector); >> struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); >> enum pipe pipe = crtc->pipe; >> enum port port; >> @@ -836,21 +835,10 @@ static void intel_dsi_pre_enable(struct intel_atomic_state *state, >> if (!IS_GEMINILAKE(dev_priv)) >> intel_dsi_prepare(encoder, pipe_config); >> >> + /* Give the panel time to power-on and then deassert its reset */ >> intel_dsi_vbt_exec_sequence(intel_dsi, MIPI_SEQ_POWER_ON); >> - >> - /* >> - * Give the panel time to power-on and then deassert its reset. >> - * Depending on the VBT MIPI sequences version the deassert-seq >> - * may contain the necessary delay, intel_dsi_msleep() will skip >> - * the delay in that case. If there is no deassert-seq, then an >> - * unconditional msleep is used to give the panel time to power-on. >> - */ >> - if (connector->panel.vbt.dsi.sequence[MIPI_SEQ_DEASSERT_RESET]) { >> - intel_dsi_msleep(intel_dsi, intel_dsi->panel_on_delay); >> - intel_dsi_vbt_exec_sequence(intel_dsi, MIPI_SEQ_DEASSERT_RESET); >> - } else { >> - msleep(intel_dsi->panel_on_delay); >> - } >> + msleep(intel_dsi->panel_on_delay); >> + intel_dsi_vbt_exec_sequence(intel_dsi, MIPI_SEQ_DEASSERT_RESET); >> >> if (IS_GEMINILAKE(dev_priv)) { >> glk_cold_boot = glk_dsi_enable_io(encoder); >> -- >> 2.39.2 >
On Mon, Apr 24, 2023 at 08:54:27PM +0200, Hans de Goede wrote: > Hi Ville, > > On 4/24/23 20:34, Ville Syrjälä wrote: > > On Sat, Apr 22, 2023 at 08:43:59PM +0200, Hans de Goede wrote: > >> The intel_dsi_msleep() helper skips sleeping if the MIPI-sequences have > >> a version of 3 or newer and the panel is in vid-mode. > >> > >> This is based on the big comment around line 730 which starts with > >> "Panel enable/disable sequences from the VBT spec.", where > >> the "v3 video mode seq" column does not have any wait t# entries. > >> > >> Commit 6fdb335f1c9c ("drm/i915/dsi: Use unconditional msleep for > >> the panel_on_delay when there is no reset-deassert MIPI-sequence") > >> switched to a direct msleep() instead of intel_dsi_msleep() > >> when there is no MIPI_SEQ_DEASSERT_RESET sequence, to fix > >> the panel on an Acer Aspire Switch 10 E SW3-016 not turning on. > >> > >> This was done under the assumption that when there is a v3 > >> MIPI_SEQ_DEASSERT_RESET sequence it will take care of any > >> necessary delays. > >> > >> On the Nextbook Ares 8A (a Cherry Trail device like the Acer SW3-016) > >> there is a MIPI_SEQ_DEASSERT_RESET sequence, yet panel_on_delay > >> must still be honored otherwise the panel will not turn on. > >> > >> Switch to always using an unconditional msleep() for > >> the panel_on_delay instead of making this depend on > >> the presence of a MIPI_SEQ_DEASSERT_RESET sequence. > > > > I just checked what Windows does, and at least for icl+ it > > always waits for the panel power delays regardless of what > > the VBT MIPI sequences are doing. > > > > So I suspect we should just get rid of intel_dsi_msleep() > > entirely and do what the power sequencing delays tell us. > > Anything else is untested territory. If the VBT actually > > wanted us to skip the delays then it should really be > > setting them to zero. > > So I checked and there are only 4 (before this patch) / > 3 (after this patch) callers of intel_dsi_msleep(). > > So just getting rid of it entirely sounds good to me. > > Shall I prepare a v2 patch which does this ? Please do. > > Regards, > > Hans > > > > > > >> Fixes: 6fdb335f1c9c ("drm/i915/dsi: Use unconditional msleep for the panel_on_delay when there is no reset-deassert MIPI-sequence") > >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> > >> --- > >> drivers/gpu/drm/i915/display/vlv_dsi.c | 18 +++--------------- > >> 1 file changed, 3 insertions(+), 15 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/i915/display/vlv_dsi.c b/drivers/gpu/drm/i915/display/vlv_dsi.c > >> index a6d6d8b33f3f..1b87f8f5f7fd 100644 > >> --- a/drivers/gpu/drm/i915/display/vlv_dsi.c > >> +++ b/drivers/gpu/drm/i915/display/vlv_dsi.c > >> @@ -788,7 +788,6 @@ static void intel_dsi_pre_enable(struct intel_atomic_state *state, > >> { > >> struct intel_dsi *intel_dsi = enc_to_intel_dsi(encoder); > >> struct intel_crtc *crtc = to_intel_crtc(pipe_config->uapi.crtc); > >> - struct intel_connector *connector = to_intel_connector(conn_state->connector); > >> struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > >> enum pipe pipe = crtc->pipe; > >> enum port port; > >> @@ -836,21 +835,10 @@ static void intel_dsi_pre_enable(struct intel_atomic_state *state, > >> if (!IS_GEMINILAKE(dev_priv)) > >> intel_dsi_prepare(encoder, pipe_config); > >> > >> + /* Give the panel time to power-on and then deassert its reset */ > >> intel_dsi_vbt_exec_sequence(intel_dsi, MIPI_SEQ_POWER_ON); > >> - > >> - /* > >> - * Give the panel time to power-on and then deassert its reset. > >> - * Depending on the VBT MIPI sequences version the deassert-seq > >> - * may contain the necessary delay, intel_dsi_msleep() will skip > >> - * the delay in that case. If there is no deassert-seq, then an > >> - * unconditional msleep is used to give the panel time to power-on. > >> - */ > >> - if (connector->panel.vbt.dsi.sequence[MIPI_SEQ_DEASSERT_RESET]) { > >> - intel_dsi_msleep(intel_dsi, intel_dsi->panel_on_delay); > >> - intel_dsi_vbt_exec_sequence(intel_dsi, MIPI_SEQ_DEASSERT_RESET); > >> - } else { > >> - msleep(intel_dsi->panel_on_delay); > >> - } > >> + msleep(intel_dsi->panel_on_delay); > >> + intel_dsi_vbt_exec_sequence(intel_dsi, MIPI_SEQ_DEASSERT_RESET); > >> > >> if (IS_GEMINILAKE(dev_priv)) { > >> glk_cold_boot = glk_dsi_enable_io(encoder); > >> -- > >> 2.39.2 > >
diff --git a/drivers/gpu/drm/i915/display/vlv_dsi.c b/drivers/gpu/drm/i915/display/vlv_dsi.c index a6d6d8b33f3f..1b87f8f5f7fd 100644 --- a/drivers/gpu/drm/i915/display/vlv_dsi.c +++ b/drivers/gpu/drm/i915/display/vlv_dsi.c @@ -788,7 +788,6 @@ static void intel_dsi_pre_enable(struct intel_atomic_state *state, { struct intel_dsi *intel_dsi = enc_to_intel_dsi(encoder); struct intel_crtc *crtc = to_intel_crtc(pipe_config->uapi.crtc); - struct intel_connector *connector = to_intel_connector(conn_state->connector); struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); enum pipe pipe = crtc->pipe; enum port port; @@ -836,21 +835,10 @@ static void intel_dsi_pre_enable(struct intel_atomic_state *state, if (!IS_GEMINILAKE(dev_priv)) intel_dsi_prepare(encoder, pipe_config); + /* Give the panel time to power-on and then deassert its reset */ intel_dsi_vbt_exec_sequence(intel_dsi, MIPI_SEQ_POWER_ON); - - /* - * Give the panel time to power-on and then deassert its reset. - * Depending on the VBT MIPI sequences version the deassert-seq - * may contain the necessary delay, intel_dsi_msleep() will skip - * the delay in that case. If there is no deassert-seq, then an - * unconditional msleep is used to give the panel time to power-on. - */ - if (connector->panel.vbt.dsi.sequence[MIPI_SEQ_DEASSERT_RESET]) { - intel_dsi_msleep(intel_dsi, intel_dsi->panel_on_delay); - intel_dsi_vbt_exec_sequence(intel_dsi, MIPI_SEQ_DEASSERT_RESET); - } else { - msleep(intel_dsi->panel_on_delay); - } + msleep(intel_dsi->panel_on_delay); + intel_dsi_vbt_exec_sequence(intel_dsi, MIPI_SEQ_DEASSERT_RESET); if (IS_GEMINILAKE(dev_priv)) { glk_cold_boot = glk_dsi_enable_io(encoder);
The intel_dsi_msleep() helper skips sleeping if the MIPI-sequences have a version of 3 or newer and the panel is in vid-mode. This is based on the big comment around line 730 which starts with "Panel enable/disable sequences from the VBT spec.", where the "v3 video mode seq" column does not have any wait t# entries. Commit 6fdb335f1c9c ("drm/i915/dsi: Use unconditional msleep for the panel_on_delay when there is no reset-deassert MIPI-sequence") switched to a direct msleep() instead of intel_dsi_msleep() when there is no MIPI_SEQ_DEASSERT_RESET sequence, to fix the panel on an Acer Aspire Switch 10 E SW3-016 not turning on. This was done under the assumption that when there is a v3 MIPI_SEQ_DEASSERT_RESET sequence it will take care of any necessary delays. On the Nextbook Ares 8A (a Cherry Trail device like the Acer SW3-016) there is a MIPI_SEQ_DEASSERT_RESET sequence, yet panel_on_delay must still be honored otherwise the panel will not turn on. Switch to always using an unconditional msleep() for the panel_on_delay instead of making this depend on the presence of a MIPI_SEQ_DEASSERT_RESET sequence. Fixes: 6fdb335f1c9c ("drm/i915/dsi: Use unconditional msleep for the panel_on_delay when there is no reset-deassert MIPI-sequence") Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/gpu/drm/i915/display/vlv_dsi.c | 18 +++--------------- 1 file changed, 3 insertions(+), 15 deletions(-)