Re: [Intel-gfx] [PATCH] drm/i915/dsi: Always use unconditional msleep() for the panel_on_delay

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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
> > 

-- 
Ville Syrjälä
Intel