| Message ID | 20201118124058.26021-1-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/dsi: Use unconditional msleep for the panel_on_delay when there is no reset-deassert MIPI-sequence | expand |
Hi, On 11/18/20 1:40 PM, Hans de Goede wrote: > Commit 25b4620ee822 ("drm/i915/dsi: Skip delays for v3 VBTs in vid-mode") > added an intel_dsi_msleep() helper which skips sleeping if the > MIPI-sequences have a version of 3 or newer and the panel is in vid-mode; > and it moved a bunch of msleep-s over to this new helper. > > This was based on my reading of 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. > > Given that this code has been used on a lot of different devices without > issues until now, it seems that my interpretation of the spec here is > mostly correct. > > But now I have encountered one device, an Acer Aspire Switch 10 E > SW3-016, where the panel will not light up unless we do actually honor the > panel_on_delay after exexuting the MIPI_SEQ_PANEL_ON sequence. > > What seems to set this model apart is that it is lacking a > MIPI_SEQ_DEASSERT_RESET sequence, which is where the power-on > delay usually happens. > > Fix the panel not lighting up on this model by using an unconditional > msleep(panel_on_delay) instead of intel_dsi_msleep() when there is > no MIPI_SEQ_DEASSERT_RESET sequence. > > Fixes: 25b4620ee822 ("drm/i915/dsi: Skip delays for v3 VBTs in vid-mode") > Signed-off-by: Hans de Goede <hdegoede@redhat.com> Ping can I get a review/ack for this please? Regards, Hans > --- > drivers/gpu/drm/i915/display/vlv_dsi.c | 10 +++++++--- > 1 file changed, 7 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/vlv_dsi.c b/drivers/gpu/drm/i915/display/vlv_dsi.c > index 194c239ab6b1..ef673277b36d 100644 > --- a/drivers/gpu/drm/i915/display/vlv_dsi.c > +++ b/drivers/gpu/drm/i915/display/vlv_dsi.c > @@ -816,10 +816,14 @@ static void intel_dsi_pre_enable(struct intel_atomic_state *state, > intel_dsi_prepare(encoder, pipe_config); > > intel_dsi_vbt_exec_sequence(intel_dsi, MIPI_SEQ_POWER_ON); > - intel_dsi_msleep(intel_dsi, intel_dsi->panel_on_delay); > > - /* Deassert reset */ > - intel_dsi_vbt_exec_sequence(intel_dsi, MIPI_SEQ_DEASSERT_RESET); > + if (dev_priv->vbt.dsi.sequence[MIPI_SEQ_DEASSERT_RESET]) { > + intel_dsi_msleep(intel_dsi, intel_dsi->panel_on_delay); > + /* Deassert reset */ > + intel_dsi_vbt_exec_sequence(intel_dsi, MIPI_SEQ_DEASSERT_RESET); > + } else { > + msleep(intel_dsi->panel_on_delay); > + } > > if (IS_GEMINILAKE(dev_priv)) { > glk_cold_boot = glk_dsi_enable_io(encoder); >
On Wed, Nov 18, 2020 at 01:40:58PM +0100, Hans de Goede wrote: > Commit 25b4620ee822 ("drm/i915/dsi: Skip delays for v3 VBTs in vid-mode") > added an intel_dsi_msleep() helper which skips sleeping if the > MIPI-sequences have a version of 3 or newer and the panel is in vid-mode; > and it moved a bunch of msleep-s over to this new helper. > > This was based on my reading of 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. > > Given that this code has been used on a lot of different devices without > issues until now, it seems that my interpretation of the spec here is > mostly correct. > > But now I have encountered one device, an Acer Aspire Switch 10 E > SW3-016, where the panel will not light up unless we do actually honor the > panel_on_delay after exexuting the MIPI_SEQ_PANEL_ON sequence. > > What seems to set this model apart is that it is lacking a > MIPI_SEQ_DEASSERT_RESET sequence, which is where the power-on > delay usually happens. > > Fix the panel not lighting up on this model by using an unconditional > msleep(panel_on_delay) instead of intel_dsi_msleep() when there is > no MIPI_SEQ_DEASSERT_RESET sequence. > > Fixes: 25b4620ee822 ("drm/i915/dsi: Skip delays for v3 VBTs in vid-mode") > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > drivers/gpu/drm/i915/display/vlv_dsi.c | 10 +++++++--- > 1 file changed, 7 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/vlv_dsi.c b/drivers/gpu/drm/i915/display/vlv_dsi.c > index 194c239ab6b1..ef673277b36d 100644 > --- a/drivers/gpu/drm/i915/display/vlv_dsi.c > +++ b/drivers/gpu/drm/i915/display/vlv_dsi.c > @@ -816,10 +816,14 @@ static void intel_dsi_pre_enable(struct intel_atomic_state *state, > intel_dsi_prepare(encoder, pipe_config); > > intel_dsi_vbt_exec_sequence(intel_dsi, MIPI_SEQ_POWER_ON); > - intel_dsi_msleep(intel_dsi, intel_dsi->panel_on_delay); > > - /* Deassert reset */ > - intel_dsi_vbt_exec_sequence(intel_dsi, MIPI_SEQ_DEASSERT_RESET); > + if (dev_priv->vbt.dsi.sequence[MIPI_SEQ_DEASSERT_RESET]) { > + intel_dsi_msleep(intel_dsi, intel_dsi->panel_on_delay); > + /* Deassert reset */ > + intel_dsi_vbt_exec_sequence(intel_dsi, MIPI_SEQ_DEASSERT_RESET); > + } else { > + msleep(intel_dsi->panel_on_delay); > + } Could perhaps use a comment ot explain to the reader what's going on. Looks sane enough to me, and if we get this wrong we just get a bigger delay than necessary I guess. So mostly harmless. Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > if (IS_GEMINILAKE(dev_priv)) { > glk_cold_boot = glk_dsi_enable_io(encoder); > -- > 2.28.0
Hi, On 11/24/20 4:49 PM, Ville Syrjälä wrote: > On Wed, Nov 18, 2020 at 01:40:58PM +0100, Hans de Goede wrote: >> Commit 25b4620ee822 ("drm/i915/dsi: Skip delays for v3 VBTs in vid-mode") >> added an intel_dsi_msleep() helper which skips sleeping if the >> MIPI-sequences have a version of 3 or newer and the panel is in vid-mode; >> and it moved a bunch of msleep-s over to this new helper. >> >> This was based on my reading of 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. >> >> Given that this code has been used on a lot of different devices without >> issues until now, it seems that my interpretation of the spec here is >> mostly correct. >> >> But now I have encountered one device, an Acer Aspire Switch 10 E >> SW3-016, where the panel will not light up unless we do actually honor the >> panel_on_delay after exexuting the MIPI_SEQ_PANEL_ON sequence. >> >> What seems to set this model apart is that it is lacking a >> MIPI_SEQ_DEASSERT_RESET sequence, which is where the power-on >> delay usually happens. >> >> Fix the panel not lighting up on this model by using an unconditional >> msleep(panel_on_delay) instead of intel_dsi_msleep() when there is >> no MIPI_SEQ_DEASSERT_RESET sequence. >> >> Fixes: 25b4620ee822 ("drm/i915/dsi: Skip delays for v3 VBTs in vid-mode") >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> drivers/gpu/drm/i915/display/vlv_dsi.c | 10 +++++++--- >> 1 file changed, 7 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/display/vlv_dsi.c b/drivers/gpu/drm/i915/display/vlv_dsi.c >> index 194c239ab6b1..ef673277b36d 100644 >> --- a/drivers/gpu/drm/i915/display/vlv_dsi.c >> +++ b/drivers/gpu/drm/i915/display/vlv_dsi.c >> @@ -816,10 +816,14 @@ static void intel_dsi_pre_enable(struct intel_atomic_state *state, >> intel_dsi_prepare(encoder, pipe_config); >> >> intel_dsi_vbt_exec_sequence(intel_dsi, MIPI_SEQ_POWER_ON); >> - intel_dsi_msleep(intel_dsi, intel_dsi->panel_on_delay); >> >> - /* Deassert reset */ >> - intel_dsi_vbt_exec_sequence(intel_dsi, MIPI_SEQ_DEASSERT_RESET); >> + if (dev_priv->vbt.dsi.sequence[MIPI_SEQ_DEASSERT_RESET]) { >> + intel_dsi_msleep(intel_dsi, intel_dsi->panel_on_delay); >> + /* Deassert reset */ >> + intel_dsi_vbt_exec_sequence(intel_dsi, MIPI_SEQ_DEASSERT_RESET); >> + } else { >> + msleep(intel_dsi->panel_on_delay); >> + } > > Could perhaps use a comment ot explain to the reader what's going on. > > Looks sane enough to me, and if we get this wrong we just get a bigger > delay than necessary I guess. So mostly harmless. > > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> Thank you, and sorry for being slow with getting around to pushing this to drm-intel-next. I've just pushed it to drm-intel-next with a comment added above the if (and dropped the single line comment inside the if), so this now looks like this: /* * 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 (dev_priv->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); } (the code is unchanged from when you reviewed it). Regards, Hans
diff --git a/drivers/gpu/drm/i915/display/vlv_dsi.c b/drivers/gpu/drm/i915/display/vlv_dsi.c index 194c239ab6b1..ef673277b36d 100644 --- a/drivers/gpu/drm/i915/display/vlv_dsi.c +++ b/drivers/gpu/drm/i915/display/vlv_dsi.c @@ -816,10 +816,14 @@ static void intel_dsi_pre_enable(struct intel_atomic_state *state, intel_dsi_prepare(encoder, pipe_config); intel_dsi_vbt_exec_sequence(intel_dsi, MIPI_SEQ_POWER_ON); - intel_dsi_msleep(intel_dsi, intel_dsi->panel_on_delay); - /* Deassert reset */ - intel_dsi_vbt_exec_sequence(intel_dsi, MIPI_SEQ_DEASSERT_RESET); + if (dev_priv->vbt.dsi.sequence[MIPI_SEQ_DEASSERT_RESET]) { + intel_dsi_msleep(intel_dsi, intel_dsi->panel_on_delay); + /* Deassert reset */ + intel_dsi_vbt_exec_sequence(intel_dsi, MIPI_SEQ_DEASSERT_RESET); + } else { + msleep(intel_dsi->panel_on_delay); + } if (IS_GEMINILAKE(dev_priv)) { glk_cold_boot = glk_dsi_enable_io(encoder);
Commit 25b4620ee822 ("drm/i915/dsi: Skip delays for v3 VBTs in vid-mode") added an intel_dsi_msleep() helper which skips sleeping if the MIPI-sequences have a version of 3 or newer and the panel is in vid-mode; and it moved a bunch of msleep-s over to this new helper. This was based on my reading of 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. Given that this code has been used on a lot of different devices without issues until now, it seems that my interpretation of the spec here is mostly correct. But now I have encountered one device, an Acer Aspire Switch 10 E SW3-016, where the panel will not light up unless we do actually honor the panel_on_delay after exexuting the MIPI_SEQ_PANEL_ON sequence. What seems to set this model apart is that it is lacking a MIPI_SEQ_DEASSERT_RESET sequence, which is where the power-on delay usually happens. Fix the panel not lighting up on this model by using an unconditional msleep(panel_on_delay) instead of intel_dsi_msleep() when there is no MIPI_SEQ_DEASSERT_RESET sequence. Fixes: 25b4620ee822 ("drm/i915/dsi: Skip delays for v3 VBTs in vid-mode") Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/gpu/drm/i915/display/vlv_dsi.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)