[PATCH] drm/i915/dsi: Use unconditional msleep for the panel_on_delay when there is no reset-deassert MIPI-sequence

[PATCH] drm/i915/dsi: Use unconditional msleep for the panel_on_delay when there is no reset-deassert MIPI-sequence

[Hans de Goede]

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);
+	}
 
 	if (IS_GEMINILAKE(dev_priv)) {
 		glk_cold_boot = glk_dsi_enable_io(encoder);
-- 
2.28.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/i915/dsi: Use unconditional msleep for the panel_on_delay when there is no reset-deassert MIPI-sequence

[Hans de Goede]

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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/i915/dsi: Use unconditional msleep for the panel_on_delay when there is no reset-deassert MIPI-sequence

[Ville Syrjälä]

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

-- 
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/i915/dsi: Use unconditional msleep for the panel_on_delay when there is no reset-deassert MIPI-sequence

[Hans de Goede]

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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel