[PATCH 1/2] drm/i915/dsi: Fix swapping of MIPI_SEQ_DEASSERT_RESET / MIPI_SEQ_ASSERT_RESET

[PATCH 1/2] drm/i915/dsi: Fix swapping of MIPI_SEQ_DEASSERT_RESET / MIPI_SEQ_ASSERT_RESET

[Hans de Goede]

Looking at the ADF code from the Android kernel sources for a
cherrytrail tablet I noticed that it is calling the
MIPI_SEQ_ASSERT_RESET sequence from the panel prepare hook.

Until commit b1cb1bd29189 ("drm/i915/dsi: update reset and power sequences
in panel prepare/unprepare hooks") the mainline i915 code was doing the
same. That commits effectively swaps the calling of MIPI_SEQ_ASSERT_RESET /
MIPI_SEQ_DEASSERT_RESET.

Looking at the naming of the sequences that is the right thing to do,
but the problem is, that the old mainline code and the ADF code was
actually calling the right sequence (tested on a cube iwork8 air tablet),
and the swapping of the calling breaks things.

This breakage was likely not noticed in testing because on cherrytrail,
currently chv_exec_gpio ends up disabling the gpio pins rather then
setting them (this is fixed in the next patch in this patch-set).

This commit fixes the swapping by fixing MIPI_SEQ_ASSERT/DEASSERT_RESET's
places in the enum defining them, so that their (new) names match their
actual use.

Fixes: b1cb1bd29189 ("drm/i915/dsi: update reset and power sequences...")
Cc: Jani Nikula <jani.nikula@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/gpu/drm/i915/intel_bios.h          | 4 ++--
 drivers/gpu/drm/i915/intel_dsi_panel_vbt.c | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_bios.h b/drivers/gpu/drm/i915/intel_bios.h
index 8405b5a..642a5eb 100644
--- a/drivers/gpu/drm/i915/intel_bios.h
+++ b/drivers/gpu/drm/i915/intel_bios.h
@@ -49,11 +49,11 @@ struct edp_power_seq {
 /* MIPI Sequence Block definitions */
 enum mipi_seq {
 	MIPI_SEQ_END = 0,
-	MIPI_SEQ_ASSERT_RESET,
+	MIPI_SEQ_DEASSERT_RESET,
 	MIPI_SEQ_INIT_OTP,
 	MIPI_SEQ_DISPLAY_ON,
 	MIPI_SEQ_DISPLAY_OFF,
-	MIPI_SEQ_DEASSERT_RESET,
+	MIPI_SEQ_ASSERT_RESET,
 	MIPI_SEQ_BACKLIGHT_ON,		/* sequence block v2+ */
 	MIPI_SEQ_BACKLIGHT_OFF,		/* sequence block v2+ */
 	MIPI_SEQ_TEAR_ON,		/* sequence block v2+ */
diff --git a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
index 0d8ff00..579d2f5 100644
--- a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
+++ b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
@@ -376,11 +376,11 @@ static const fn_mipi_elem_exec exec_elem[] = {
  */
 
 static const char * const seq_name[] = {
-	[MIPI_SEQ_ASSERT_RESET] = "MIPI_SEQ_ASSERT_RESET",
+	[MIPI_SEQ_DEASSERT_RESET] = "MIPI_SEQ_DEASSERT_RESET",
 	[MIPI_SEQ_INIT_OTP] = "MIPI_SEQ_INIT_OTP",
 	[MIPI_SEQ_DISPLAY_ON] = "MIPI_SEQ_DISPLAY_ON",
 	[MIPI_SEQ_DISPLAY_OFF]  = "MIPI_SEQ_DISPLAY_OFF",
-	[MIPI_SEQ_DEASSERT_RESET] = "MIPI_SEQ_DEASSERT_RESET",
+	[MIPI_SEQ_ASSERT_RESET] = "MIPI_SEQ_ASSERT_RESET",
 	[MIPI_SEQ_BACKLIGHT_ON] = "MIPI_SEQ_BACKLIGHT_ON",
 	[MIPI_SEQ_BACKLIGHT_OFF] = "MIPI_SEQ_BACKLIGHT_OFF",
 	[MIPI_SEQ_TEAR_ON] = "MIPI_SEQ_TEAR_ON",
-- 
2.9.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/2] drm/i915/dsi: Fix chv_exec_gpio disabling the GPIOs it is setting

[Hans de Goede]

Set the CHV_GPIO_GPIOEN bit when updating GPIOs from chv_exec_gpio.

Fixes: a0a6d4ffd2ad ("drm/i915/dsi: add support for gpio elements on CHV")
Cc: Jani Nikula <jani.nikula@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/gpu/drm/i915/intel_dsi_panel_vbt.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
index 579d2f5..47cd1b2 100644
--- a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
+++ b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
@@ -300,7 +300,8 @@ static void chv_exec_gpio(struct drm_i915_private *dev_priv,
 	mutex_lock(&dev_priv->sb_lock);
 	vlv_iosf_sb_write(dev_priv, port, cfg1, 0);
 	vlv_iosf_sb_write(dev_priv, port, cfg0,
-			  CHV_GPIO_GPIOCFG_GPO | CHV_GPIO_GPIOTXSTATE(value));
+			  CHV_GPIO_GPIOEN | CHV_GPIO_GPIOCFG_GPO |
+			  CHV_GPIO_GPIOTXSTATE(value));
 	mutex_unlock(&dev_priv->sb_lock);
 }
 
-- 
2.9.3

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

Re: [PATCH 2/2] drm/i915/dsi: Fix chv_exec_gpio disabling the GPIOs it is setting

[Ville Syrjälä]

On Tue, Nov 29, 2016 at 01:38:58PM +0100, Hans de Goede wrote:
> Set the CHV_GPIO_GPIOEN bit when updating GPIOs from chv_exec_gpio.
> 
> Fixes: a0a6d4ffd2ad ("drm/i915/dsi: add support for gpio elements on CHV")
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/gpu/drm/i915/intel_dsi_panel_vbt.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
> index 579d2f5..47cd1b2 100644
> --- a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
> +++ b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
> @@ -300,7 +300,8 @@ static void chv_exec_gpio(struct drm_i915_private *dev_priv,
>  	mutex_lock(&dev_priv->sb_lock);
>  	vlv_iosf_sb_write(dev_priv, port, cfg1, 0);
>  	vlv_iosf_sb_write(dev_priv, port, cfg0,
> -			  CHV_GPIO_GPIOCFG_GPO | CHV_GPIO_GPIOTXSTATE(value));
> +			  CHV_GPIO_GPIOEN | CHV_GPIO_GPIOCFG_GPO |
> +			  CHV_GPIO_GPIOTXSTATE(value));

Indeed.

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: stable@vger.kernel.org

>  	mutex_unlock(&dev_priv->sb_lock);
>  }
>  
> -- 
> 2.9.3

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

Re: [PATCH 1/2] drm/i915/dsi: Fix swapping of MIPI_SEQ_DEASSERT_RESET / MIPI_SEQ_ASSERT_RESET

[Ville Syrjälä]

On Tue, Nov 29, 2016 at 01:38:57PM +0100, Hans de Goede wrote:
> Looking at the ADF code from the Android kernel sources for a
> cherrytrail tablet I noticed that it is calling the
> MIPI_SEQ_ASSERT_RESET sequence from the panel prepare hook.
> 
> Until commit b1cb1bd29189 ("drm/i915/dsi: update reset and power sequences
> in panel prepare/unprepare hooks") the mainline i915 code was doing the
> same. That commits effectively swaps the calling of MIPI_SEQ_ASSERT_RESET /
> MIPI_SEQ_DEASSERT_RESET.
> 
> Looking at the naming of the sequences that is the right thing to do,
> but the problem is, that the old mainline code and the ADF code was
> actually calling the right sequence (tested on a cube iwork8 air tablet),
> and the swapping of the calling breaks things.
> 
> This breakage was likely not noticed in testing because on cherrytrail,
> currently chv_exec_gpio ends up disabling the gpio pins rather then
> setting them (this is fixed in the next patch in this patch-set).
> 
> This commit fixes the swapping by fixing MIPI_SEQ_ASSERT/DEASSERT_RESET's
> places in the enum defining them, so that their (new) names match their
> actual use.
> 
> Fixes: b1cb1bd29189 ("drm/i915/dsi: update reset and power sequences...")
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/gpu/drm/i915/intel_bios.h          | 4 ++--
>  drivers/gpu/drm/i915/intel_dsi_panel_vbt.c | 4 ++--
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_bios.h b/drivers/gpu/drm/i915/intel_bios.h
> index 8405b5a..642a5eb 100644
> --- a/drivers/gpu/drm/i915/intel_bios.h
> +++ b/drivers/gpu/drm/i915/intel_bios.h
> @@ -49,11 +49,11 @@ struct edp_power_seq {
>  /* MIPI Sequence Block definitions */
>  enum mipi_seq {
>  	MIPI_SEQ_END = 0,
> -	MIPI_SEQ_ASSERT_RESET,
> +	MIPI_SEQ_DEASSERT_RESET,
>  	MIPI_SEQ_INIT_OTP,
>  	MIPI_SEQ_DISPLAY_ON,
>  	MIPI_SEQ_DISPLAY_OFF,
> -	MIPI_SEQ_DEASSERT_RESET,
> +	MIPI_SEQ_ASSERT_RESET,

That naming would be against the spec, so I don't think we want to do it
like this. Unless the spec is totally wrong, that is.

Can you provide the VBT for the affected machine so other people can
have a look at it?

>  	MIPI_SEQ_BACKLIGHT_ON,		/* sequence block v2+ */
>  	MIPI_SEQ_BACKLIGHT_OFF,		/* sequence block v2+ */
>  	MIPI_SEQ_TEAR_ON,		/* sequence block v2+ */
> diff --git a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
> index 0d8ff00..579d2f5 100644
> --- a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
> +++ b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
> @@ -376,11 +376,11 @@ static const fn_mipi_elem_exec exec_elem[] = {
>   */
>  
>  static const char * const seq_name[] = {
> -	[MIPI_SEQ_ASSERT_RESET] = "MIPI_SEQ_ASSERT_RESET",
> +	[MIPI_SEQ_DEASSERT_RESET] = "MIPI_SEQ_DEASSERT_RESET",
>  	[MIPI_SEQ_INIT_OTP] = "MIPI_SEQ_INIT_OTP",
>  	[MIPI_SEQ_DISPLAY_ON] = "MIPI_SEQ_DISPLAY_ON",
>  	[MIPI_SEQ_DISPLAY_OFF]  = "MIPI_SEQ_DISPLAY_OFF",
> -	[MIPI_SEQ_DEASSERT_RESET] = "MIPI_SEQ_DEASSERT_RESET",
> +	[MIPI_SEQ_ASSERT_RESET] = "MIPI_SEQ_ASSERT_RESET",
>  	[MIPI_SEQ_BACKLIGHT_ON] = "MIPI_SEQ_BACKLIGHT_ON",
>  	[MIPI_SEQ_BACKLIGHT_OFF] = "MIPI_SEQ_BACKLIGHT_OFF",
>  	[MIPI_SEQ_TEAR_ON] = "MIPI_SEQ_TEAR_ON",
> -- 
> 2.9.3

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

Re: [PATCH 1/2] drm/i915/dsi: Fix swapping of MIPI_SEQ_DEASSERT_RESET / MIPI_SEQ_ASSERT_RESET

[Hans de Goede]

Hi,

Thanks for the quick reply.

On 29-11-16 13:48, Ville Syrjälä wrote:
> On Tue, Nov 29, 2016 at 01:38:57PM +0100, Hans de Goede wrote:
>> Looking at the ADF code from the Android kernel sources for a
>> cherrytrail tablet I noticed that it is calling the
>> MIPI_SEQ_ASSERT_RESET sequence from the panel prepare hook.
>>
>> Until commit b1cb1bd29189 ("drm/i915/dsi: update reset and power sequences
>> in panel prepare/unprepare hooks") the mainline i915 code was doing the
>> same. That commits effectively swaps the calling of MIPI_SEQ_ASSERT_RESET /
>> MIPI_SEQ_DEASSERT_RESET.
>>
>> Looking at the naming of the sequences that is the right thing to do,
>> but the problem is, that the old mainline code and the ADF code was
>> actually calling the right sequence (tested on a cube iwork8 air tablet),
>> and the swapping of the calling breaks things.
>>
>> This breakage was likely not noticed in testing because on cherrytrail,
>> currently chv_exec_gpio ends up disabling the gpio pins rather then
>> setting them (this is fixed in the next patch in this patch-set).
>>
>> This commit fixes the swapping by fixing MIPI_SEQ_ASSERT/DEASSERT_RESET's
>> places in the enum defining them, so that their (new) names match their
>> actual use.
>>
>> Fixes: b1cb1bd29189 ("drm/i915/dsi: update reset and power sequences...")
>> Cc: Jani Nikula <jani.nikula@intel.com>
>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  drivers/gpu/drm/i915/intel_bios.h          | 4 ++--
>>  drivers/gpu/drm/i915/intel_dsi_panel_vbt.c | 4 ++--
>>  2 files changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_bios.h b/drivers/gpu/drm/i915/intel_bios.h
>> index 8405b5a..642a5eb 100644
>> --- a/drivers/gpu/drm/i915/intel_bios.h
>> +++ b/drivers/gpu/drm/i915/intel_bios.h
>> @@ -49,11 +49,11 @@ struct edp_power_seq {
>>  /* MIPI Sequence Block definitions */
>>  enum mipi_seq {
>>  	MIPI_SEQ_END = 0,
>> -	MIPI_SEQ_ASSERT_RESET,
>> +	MIPI_SEQ_DEASSERT_RESET,
>>  	MIPI_SEQ_INIT_OTP,
>>  	MIPI_SEQ_DISPLAY_ON,
>>  	MIPI_SEQ_DISPLAY_OFF,
>> -	MIPI_SEQ_DEASSERT_RESET,
>> +	MIPI_SEQ_ASSERT_RESET,
>
> That naming would be against the spec, so I don't think we want to do it
> like this. Unless the spec is totally wrong, that is.

Given that both the ADF code and the i915 driver until now have been using
assert on prepare and deassert on unprepare I'm inclined to believe that
the spec is wrong. Is the spec available somewhere public ?

Also if you look at the v1 sequences with the new names:

	MIPI_SEQ_DEASSERT_RESET,
  	MIPI_SEQ_INIT_OTP,
  	MIPI_SEQ_DISPLAY_ON,
  	MIPI_SEQ_DISPLAY_OFF,
	MIPI_SEQ_ASSERT_RESET,

Then they are exactly in the order as one would call them on
enable, followed by disable, which I believe is not a coincidence.

> Can you provide the VBT for the affected machine so other people can
> have a look at it?

Sure I can do that, what would be the easiest way (both for me to
produce and for you to consume) to do this ?

While developing this set, I've added printk calls to the code executing the
sequences, there are 2 gpios involved nr 60 (backlight enable AFAICT, also used
by the BACKLIGHT sequences) and 72 (reset / panel_enable ?).
When efifb is up both are 1 / high.

With the OLD naming, MIPI_SEQ_ASSERT_RESET does:

gpio 72 high
delay
gpio 72 low
delay
gpio 72 high

And DEASSERT does:

gpio 72 low
gpio 60 low

So with the old naming DEASSERT leaves the panel disabled / in reset and
the backlight disabled, which is exactly what we do not want...

I guess it would be good if someone could check if my patch helps
or not on other tablets which use these sequences.

Regards,

Hans


p.s.

I'm also trying to come up with some patches which properly
integrate pwm-lpss with the i915 driver instead of it
throwing a "Failed to own the pwm chip" error. But as soon
as I hook up things so that pwm_get() returns the pwm-lpss
pwm0 I hit:

https://bugs.freedesktop.org/show_bug.cgi?id=97330

I get the same pipe-a stuck (or not seeing vblank irqs?)
problems sometimes without the pwm-lpss driver in the loop
but then only sometimes and with pwm-lpss I get this
problem when loading the i915 driver most of the time
(but not all the time).

I've been debugging this a couple of evenings in a row
now (*) but no luck so far before I fixed the gpio and
assert/deassert swapping I had commented out the
chv_exec_gpio call otherwise the screen would go off
and never come back, with that call commented I was
seeing the same issue.

I was hoping that properly resetting the screen when
fbcon does its disable / re-enable dance would fix this,
but it does not :|  Any hints would be greatly appreciated.

*) This series is one result of this, I also have some
designware i2c pmic mux fixes I need to post


>>  	MIPI_SEQ_BACKLIGHT_ON,		/* sequence block v2+ */
>>  	MIPI_SEQ_BACKLIGHT_OFF,		/* sequence block v2+ */
>>  	MIPI_SEQ_TEAR_ON,		/* sequence block v2+ */
>> diff --git a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
>> index 0d8ff00..579d2f5 100644
>> --- a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
>> +++ b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
>> @@ -376,11 +376,11 @@ static const fn_mipi_elem_exec exec_elem[] = {
>>   */
>>
>>  static const char * const seq_name[] = {
>> -	[MIPI_SEQ_ASSERT_RESET] = "MIPI_SEQ_ASSERT_RESET",
>> +	[MIPI_SEQ_DEASSERT_RESET] = "MIPI_SEQ_DEASSERT_RESET",
>>  	[MIPI_SEQ_INIT_OTP] = "MIPI_SEQ_INIT_OTP",
>>  	[MIPI_SEQ_DISPLAY_ON] = "MIPI_SEQ_DISPLAY_ON",
>>  	[MIPI_SEQ_DISPLAY_OFF]  = "MIPI_SEQ_DISPLAY_OFF",
>> -	[MIPI_SEQ_DEASSERT_RESET] = "MIPI_SEQ_DEASSERT_RESET",
>> +	[MIPI_SEQ_ASSERT_RESET] = "MIPI_SEQ_ASSERT_RESET",
>>  	[MIPI_SEQ_BACKLIGHT_ON] = "MIPI_SEQ_BACKLIGHT_ON",
>>  	[MIPI_SEQ_BACKLIGHT_OFF] = "MIPI_SEQ_BACKLIGHT_OFF",
>>  	[MIPI_SEQ_TEAR_ON] = "MIPI_SEQ_TEAR_ON",
>> --
>> 2.9.3
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm/i915/dsi: Fix swapping of MIPI_SEQ_DEASSERT_RESET / MIPI_SEQ_ASSERT_RESET

[Ville Syrjälä]

On Tue, Nov 29, 2016 at 02:06:20PM +0100, Hans de Goede wrote:
> Hi,
> 
> Thanks for the quick reply.
> 
> On 29-11-16 13:48, Ville Syrjälä wrote:
> > On Tue, Nov 29, 2016 at 01:38:57PM +0100, Hans de Goede wrote:
> >> Looking at the ADF code from the Android kernel sources for a
> >> cherrytrail tablet I noticed that it is calling the
> >> MIPI_SEQ_ASSERT_RESET sequence from the panel prepare hook.
> >>
> >> Until commit b1cb1bd29189 ("drm/i915/dsi: update reset and power sequences
> >> in panel prepare/unprepare hooks") the mainline i915 code was doing the
> >> same. That commits effectively swaps the calling of MIPI_SEQ_ASSERT_RESET /
> >> MIPI_SEQ_DEASSERT_RESET.
> >>
> >> Looking at the naming of the sequences that is the right thing to do,
> >> but the problem is, that the old mainline code and the ADF code was
> >> actually calling the right sequence (tested on a cube iwork8 air tablet),
> >> and the swapping of the calling breaks things.
> >>
> >> This breakage was likely not noticed in testing because on cherrytrail,
> >> currently chv_exec_gpio ends up disabling the gpio pins rather then
> >> setting them (this is fixed in the next patch in this patch-set).
> >>
> >> This commit fixes the swapping by fixing MIPI_SEQ_ASSERT/DEASSERT_RESET's
> >> places in the enum defining them, so that their (new) names match their
> >> actual use.
> >>
> >> Fixes: b1cb1bd29189 ("drm/i915/dsi: update reset and power sequences...")
> >> Cc: Jani Nikula <jani.nikula@intel.com>
> >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >> ---
> >>  drivers/gpu/drm/i915/intel_bios.h          | 4 ++--
> >>  drivers/gpu/drm/i915/intel_dsi_panel_vbt.c | 4 ++--
> >>  2 files changed, 4 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_bios.h b/drivers/gpu/drm/i915/intel_bios.h
> >> index 8405b5a..642a5eb 100644
> >> --- a/drivers/gpu/drm/i915/intel_bios.h
> >> +++ b/drivers/gpu/drm/i915/intel_bios.h
> >> @@ -49,11 +49,11 @@ struct edp_power_seq {
> >>  /* MIPI Sequence Block definitions */
> >>  enum mipi_seq {
> >>  	MIPI_SEQ_END = 0,
> >> -	MIPI_SEQ_ASSERT_RESET,
> >> +	MIPI_SEQ_DEASSERT_RESET,
> >>  	MIPI_SEQ_INIT_OTP,
> >>  	MIPI_SEQ_DISPLAY_ON,
> >>  	MIPI_SEQ_DISPLAY_OFF,
> >> -	MIPI_SEQ_DEASSERT_RESET,
> >> +	MIPI_SEQ_ASSERT_RESET,
> >
> > That naming would be against the spec, so I don't think we want to do it
> > like this. Unless the spec is totally wrong, that is.
> 
> Given that both the ADF code and the i915 driver until now have been using
> assert on prepare and deassert on unprepare I'm inclined to believe that
> the spec is wrong. Is the spec available somewhere public ?

I don't think so. And sadly even if it would it wouldn't really help
since about the only thing it says is:

00 – Reserved
01 - MIPIAssertResetPin
02 – MIPISendInitialDcsCmds (Use this sequence type for sending initialization commands in LP mode)
03 - MIPIDisplayOn (Use this sequence type for sending initialization commands in HS mode)
04 – MIPIDisplayOff (Use this sequence type for sending DisplayOff commands in LP mode)
05 – MIPIDeassertResetPin
06 – MIPIBacklightOn
07 - MIPIBacklightOff
08 – MIPITearOn
09 - MIPITearOff
10 - MIPIPanelPowerOn
11 - MIPIPanelPowerOff
Others – Reserved

So pretty much useless if you actually want to write a working driver.

> 
> Also if you look at the v1 sequences with the new names:
> 
> 	MIPI_SEQ_DEASSERT_RESET,
>   	MIPI_SEQ_INIT_OTP,
>   	MIPI_SEQ_DISPLAY_ON,
>   	MIPI_SEQ_DISPLAY_OFF,
> 	MIPI_SEQ_ASSERT_RESET,
> 
> Then they are exactly in the order as one would call them on
> enable, followed by disable, which I believe is not a coincidence.
> 
> > Can you provide the VBT for the affected machine so other people can
> > have a look at it?
> 
> Sure I can do that, what would be the easiest way (both for me to
> produce and for you to consume) to do this ?

/sys/kernel/debug/dri/0/i915_opregion

For the best chance of preserving the dump for posterity I would
suggest filing a new bug and attaching it there.

https://bugs.freedesktop.org/enter_bug.cgi?product=DRI&component=DRM/Intel

> 
> While developing this set, I've added printk calls to the code executing the
> sequences, there are 2 gpios involved nr 60 (backlight enable AFAICT, also used
> by the BACKLIGHT sequences) and 72 (reset / panel_enable ?).
> When efifb is up both are 1 / high.
> 
> With the OLD naming, MIPI_SEQ_ASSERT_RESET does:
> 
> gpio 72 high
> delay
> gpio 72 low
> delay
> gpio 72 high

Hmm. OK so it just toggles the reset pin it seems.

> 
> And DEASSERT does:
> 
> gpio 72 low
> gpio 60 low

And this leaves the reset pin asserted, assuming it's active low,
which your patch would seem to confirm.

> 
> So with the old naming DEASSERT leaves the panel disabled / in reset and
> the backlight disabled, which is exactly what we do not want...

Right. Hmm. If we do flip them over like you suggest I think we'll at
least need a big comment to inform people why we seem to go against the
spec.

I just filed a bug against the spec, but given past history I'm not
expecting that to result in much of anything TBH.

> 
> I guess it would be good if someone could check if my patch helps
> or not on other tablets which use these sequences.

Unfortunaltey my VLV doesn't have them.

Jani has a CHT surface tablet I think, so that might have something?

We would probably want to look at a BXT VBT too. Mika, can you grab one?

I think we might want to attach them all to the same bug so that
everyone can get them easily.

> 
> Regards,
> 
> Hans
> 
> 
> p.s.
> 
> I'm also trying to come up with some patches which properly
> integrate pwm-lpss with the i915 driver instead of it
> throwing a "Failed to own the pwm chip" error. But as soon
> as I hook up things so that pwm_get() returns the pwm-lpss
> pwm0 I hit:
> 
> https://bugs.freedesktop.org/show_bug.cgi?id=97330
> 
> I get the same pipe-a stuck (or not seeing vblank irqs?)
> problems sometimes without the pwm-lpss driver in the loop
> but then only sometimes and with pwm-lpss I get this
> problem when loading the i915 driver most of the time
> (but not all the time).
> 
> I've been debugging this a couple of evenings in a row
> now (*) but no luck so far before I fixed the gpio and
> assert/deassert swapping I had commented out the
> chv_exec_gpio call otherwise the screen would go off
> and never come back, with that call commented I was
> seeing the same issue.
> 
> I was hoping that properly resetting the screen when
> fbcon does its disable / re-enable dance would fix this,
> but it does not :|  Any hints would be greatly appreciated.

I don't have any good ideas right now. Some ordering/timing
problem perhaps in the enable/disable sequences.

> 
> *) This series is one result of this, I also have some
> designware i2c pmic mux fixes I need to post
> 
> 
> >>  	MIPI_SEQ_BACKLIGHT_ON,		/* sequence block v2+ */
> >>  	MIPI_SEQ_BACKLIGHT_OFF,		/* sequence block v2+ */
> >>  	MIPI_SEQ_TEAR_ON,		/* sequence block v2+ */
> >> diff --git a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
> >> index 0d8ff00..579d2f5 100644
> >> --- a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
> >> +++ b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
> >> @@ -376,11 +376,11 @@ static const fn_mipi_elem_exec exec_elem[] = {
> >>   */
> >>
> >>  static const char * const seq_name[] = {
> >> -	[MIPI_SEQ_ASSERT_RESET] = "MIPI_SEQ_ASSERT_RESET",
> >> +	[MIPI_SEQ_DEASSERT_RESET] = "MIPI_SEQ_DEASSERT_RESET",
> >>  	[MIPI_SEQ_INIT_OTP] = "MIPI_SEQ_INIT_OTP",
> >>  	[MIPI_SEQ_DISPLAY_ON] = "MIPI_SEQ_DISPLAY_ON",
> >>  	[MIPI_SEQ_DISPLAY_OFF]  = "MIPI_SEQ_DISPLAY_OFF",
> >> -	[MIPI_SEQ_DEASSERT_RESET] = "MIPI_SEQ_DEASSERT_RESET",
> >> +	[MIPI_SEQ_ASSERT_RESET] = "MIPI_SEQ_ASSERT_RESET",
> >>  	[MIPI_SEQ_BACKLIGHT_ON] = "MIPI_SEQ_BACKLIGHT_ON",
> >>  	[MIPI_SEQ_BACKLIGHT_OFF] = "MIPI_SEQ_BACKLIGHT_OFF",
> >>  	[MIPI_SEQ_TEAR_ON] = "MIPI_SEQ_TEAR_ON",
> >> --
> >> 2.9.3
> >

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915/dsi: Fix swapping of MIPI_SEQ_DEASSERT_RESET / MIPI_SEQ_ASSERT_RESET

[Ville Syrjälä]

On Tue, Nov 29, 2016 at 04:06:31PM +0200, Ville Syrjälä wrote:
> On Tue, Nov 29, 2016 at 02:06:20PM +0100, Hans de Goede wrote:
> > Hi,
> > 
> > Thanks for the quick reply.
> > 
> > On 29-11-16 13:48, Ville Syrjälä wrote:
> > > On Tue, Nov 29, 2016 at 01:38:57PM +0100, Hans de Goede wrote:
> > >> Looking at the ADF code from the Android kernel sources for a
> > >> cherrytrail tablet I noticed that it is calling the
> > >> MIPI_SEQ_ASSERT_RESET sequence from the panel prepare hook.
> > >>
> > >> Until commit b1cb1bd29189 ("drm/i915/dsi: update reset and power sequences
> > >> in panel prepare/unprepare hooks") the mainline i915 code was doing the
> > >> same. That commits effectively swaps the calling of MIPI_SEQ_ASSERT_RESET /
> > >> MIPI_SEQ_DEASSERT_RESET.
> > >>
> > >> Looking at the naming of the sequences that is the right thing to do,
> > >> but the problem is, that the old mainline code and the ADF code was
> > >> actually calling the right sequence (tested on a cube iwork8 air tablet),
> > >> and the swapping of the calling breaks things.
> > >>
> > >> This breakage was likely not noticed in testing because on cherrytrail,
> > >> currently chv_exec_gpio ends up disabling the gpio pins rather then
> > >> setting them (this is fixed in the next patch in this patch-set).
> > >>
> > >> This commit fixes the swapping by fixing MIPI_SEQ_ASSERT/DEASSERT_RESET's
> > >> places in the enum defining them, so that their (new) names match their
> > >> actual use.
> > >>
> > >> Fixes: b1cb1bd29189 ("drm/i915/dsi: update reset and power sequences...")
> > >> Cc: Jani Nikula <jani.nikula@intel.com>
> > >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > >> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> > >> ---
> > >>  drivers/gpu/drm/i915/intel_bios.h          | 4 ++--
> > >>  drivers/gpu/drm/i915/intel_dsi_panel_vbt.c | 4 ++--
> > >>  2 files changed, 4 insertions(+), 4 deletions(-)
> > >>
> > >> diff --git a/drivers/gpu/drm/i915/intel_bios.h b/drivers/gpu/drm/i915/intel_bios.h
> > >> index 8405b5a..642a5eb 100644
> > >> --- a/drivers/gpu/drm/i915/intel_bios.h
> > >> +++ b/drivers/gpu/drm/i915/intel_bios.h
> > >> @@ -49,11 +49,11 @@ struct edp_power_seq {
> > >>  /* MIPI Sequence Block definitions */
> > >>  enum mipi_seq {
> > >>  	MIPI_SEQ_END = 0,
> > >> -	MIPI_SEQ_ASSERT_RESET,
> > >> +	MIPI_SEQ_DEASSERT_RESET,
> > >>  	MIPI_SEQ_INIT_OTP,
> > >>  	MIPI_SEQ_DISPLAY_ON,
> > >>  	MIPI_SEQ_DISPLAY_OFF,
> > >> -	MIPI_SEQ_DEASSERT_RESET,
> > >> +	MIPI_SEQ_ASSERT_RESET,
> > >
> > > That naming would be against the spec, so I don't think we want to do it
> > > like this. Unless the spec is totally wrong, that is.
> > 
> > Given that both the ADF code and the i915 driver until now have been using
> > assert on prepare and deassert on unprepare I'm inclined to believe that
> > the spec is wrong. Is the spec available somewhere public ?
> 
> I don't think so. And sadly even if it would it wouldn't really help
> since about the only thing it says is:
> 
> 00 – Reserved
> 01 - MIPIAssertResetPin
> 02 – MIPISendInitialDcsCmds (Use this sequence type for sending initialization commands in LP mode)
> 03 - MIPIDisplayOn (Use this sequence type for sending initialization commands in HS mode)
> 04 – MIPIDisplayOff (Use this sequence type for sending DisplayOff commands in LP mode)
> 05 – MIPIDeassertResetPin
> 06 – MIPIBacklightOn
> 07 - MIPIBacklightOff
> 08 – MIPITearOn
> 09 - MIPITearOff
> 10 - MIPIPanelPowerOn
> 11 - MIPIPanelPowerOff
> Others – Reserved
> 
> So pretty much useless if you actually want to write a working driver.
> 
> > 
> > Also if you look at the v1 sequences with the new names:
> > 
> > 	MIPI_SEQ_DEASSERT_RESET,
> >   	MIPI_SEQ_INIT_OTP,
> >   	MIPI_SEQ_DISPLAY_ON,
> >   	MIPI_SEQ_DISPLAY_OFF,
> > 	MIPI_SEQ_ASSERT_RESET,
> > 
> > Then they are exactly in the order as one would call them on
> > enable, followed by disable, which I believe is not a coincidence.
> > 
> > > Can you provide the VBT for the affected machine so other people can
> > > have a look at it?
> > 
> > Sure I can do that, what would be the easiest way (both for me to
> > produce and for you to consume) to do this ?
> 
> /sys/kernel/debug/dri/0/i915_opregion
> 
> For the best chance of preserving the dump for posterity I would
> suggest filing a new bug and attaching it there.
> 
> https://bugs.freedesktop.org/enter_bug.cgi?product=DRI&component=DRM/Intel
> 
> > 
> > While developing this set, I've added printk calls to the code executing the
> > sequences, there are 2 gpios involved nr 60 (backlight enable AFAICT, also used
> > by the BACKLIGHT sequences) and 72 (reset / panel_enable ?).
> > When efifb is up both are 1 / high.
> > 
> > With the OLD naming, MIPI_SEQ_ASSERT_RESET does:
> > 
> > gpio 72 high
> > delay
> > gpio 72 low
> > delay
> > gpio 72 high
> 
> Hmm. OK so it just toggles the reset pin it seems.
> 
> > 
> > And DEASSERT does:
> > 
> > gpio 72 low
> > gpio 60 low
> 
> And this leaves the reset pin asserted, assuming it's active low,
> which your patch would seem to confirm.
> 
> > 
> > So with the old naming DEASSERT leaves the panel disabled / in reset and
> > the backlight disabled, which is exactly what we do not want...
> 
> Right. Hmm. If we do flip them over like you suggest I think we'll at
> least need a big comment to inform people why we seem to go against the
> spec.
> 
> I just filed a bug against the spec, but given past history I'm not
> expecting that to result in much of anything TBH.

Actually I did get a useful response. So yeah, looks like whoever wrote
the spec was standing on their head or something since the names are
opposite of how a normal person would make them.

I'll try transcode the diagram I've seen to text:

v2 sequence for video mode:
- power on
- wait t1+t2
- MIPIAssertResetPin
- clk/data lines to lp-11
- MIPISendInitialDcsCmds
- turn on DPI
- MIPIDisplayOn
- wait t5
- backlight on
...
- backlight off
- wait t6
- MIPIDisplayOff
- turn off DPI
- clk/data lines to lp-00
- MIPIDeassertResetPin
- wait t3
- power off
- wait t4

v3 sequence for video mode:
- MIPIPanelPowerOn
- MIPIAssertResetPin
- set clk/data lines to lp-11
- MIPISendInitialDcsCmds (LP)
- turn on DPI
- MIPITearOn (command mode only) + MIPIDisplayOn (LP and HS)
- MIPIBacklightOn
...
- MIPIBacklightOff
- turn off DPI
- MIPITearOff + MIPIDisplayOff (LP)
- clk/data lines to lp-00
- MIPIDeassertResetPin
- MIPIPanelPowerOff

If I'm reading that right all the delays are part of the
VBT seqeuences already, so no need to do them in the driver.

sequence for command mode:
- power on
- wait t1+t2
- MIPIAssertResetPin
- clk/data lines to lp-11
- MIPISendInitialDcsCmds
- MIPITearOn
- MIPIDisplayOn
- set pipe to dsr mode
- wait t5
- backlight on
... issue write_mem_start/write_mem_continue commands ...
- backlight off
- wait t6
- disable pipe dsr mode
- MIPITearOff
- MIPIDisplayOff
- clk/data lines to lp-00
- MIPIDeassertResetPin
- wait t3
- power off
- wait t4

Obviously there's a bit of other weirdness in those sequences as well
(eg. the v3 video mode sequence making references to operations done
only in command mode), and also the MIPIDisplayOff vs. DPI off ordering
is different between v2 and v3 sequences. And the cmd mode sequence
looks more like the v2 sequence, so not sure what's the story with v3
cmd mode really.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915/dsi: Fix swapping of MIPI_SEQ_DEASSERT_RESET / MIPI_SEQ_ASSERT_RESET

[Hans de Goede]

Hi,

On 29-11-16 19:57, Ville Syrjälä wrote:
> On Tue, Nov 29, 2016 at 04:06:31PM +0200, Ville Syrjälä wrote:
>> On Tue, Nov 29, 2016 at 02:06:20PM +0100, Hans de Goede wrote:
>>> Hi,
>>>
>>> Thanks for the quick reply.
>>>
>>> On 29-11-16 13:48, Ville Syrjälä wrote:
>>>> On Tue, Nov 29, 2016 at 01:38:57PM +0100, Hans de Goede wrote:
>>>>> Looking at the ADF code from the Android kernel sources for a
>>>>> cherrytrail tablet I noticed that it is calling the
>>>>> MIPI_SEQ_ASSERT_RESET sequence from the panel prepare hook.
>>>>>
>>>>> Until commit b1cb1bd29189 ("drm/i915/dsi: update reset and power sequences
>>>>> in panel prepare/unprepare hooks") the mainline i915 code was doing the
>>>>> same. That commits effectively swaps the calling of MIPI_SEQ_ASSERT_RESET /
>>>>> MIPI_SEQ_DEASSERT_RESET.
>>>>>
>>>>> Looking at the naming of the sequences that is the right thing to do,
>>>>> but the problem is, that the old mainline code and the ADF code was
>>>>> actually calling the right sequence (tested on a cube iwork8 air tablet),
>>>>> and the swapping of the calling breaks things.
>>>>>
>>>>> This breakage was likely not noticed in testing because on cherrytrail,
>>>>> currently chv_exec_gpio ends up disabling the gpio pins rather then
>>>>> setting them (this is fixed in the next patch in this patch-set).
>>>>>
>>>>> This commit fixes the swapping by fixing MIPI_SEQ_ASSERT/DEASSERT_RESET's
>>>>> places in the enum defining them, so that their (new) names match their
>>>>> actual use.
>>>>>
>>>>> Fixes: b1cb1bd29189 ("drm/i915/dsi: update reset and power sequences...")
>>>>> Cc: Jani Nikula <jani.nikula@intel.com>
>>>>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>>> ---
>>>>>  drivers/gpu/drm/i915/intel_bios.h          | 4 ++--
>>>>>  drivers/gpu/drm/i915/intel_dsi_panel_vbt.c | 4 ++--
>>>>>  2 files changed, 4 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/i915/intel_bios.h b/drivers/gpu/drm/i915/intel_bios.h
>>>>> index 8405b5a..642a5eb 100644
>>>>> --- a/drivers/gpu/drm/i915/intel_bios.h
>>>>> +++ b/drivers/gpu/drm/i915/intel_bios.h
>>>>> @@ -49,11 +49,11 @@ struct edp_power_seq {
>>>>>  /* MIPI Sequence Block definitions */
>>>>>  enum mipi_seq {
>>>>>  	MIPI_SEQ_END = 0,
>>>>> -	MIPI_SEQ_ASSERT_RESET,
>>>>> +	MIPI_SEQ_DEASSERT_RESET,
>>>>>  	MIPI_SEQ_INIT_OTP,
>>>>>  	MIPI_SEQ_DISPLAY_ON,
>>>>>  	MIPI_SEQ_DISPLAY_OFF,
>>>>> -	MIPI_SEQ_DEASSERT_RESET,
>>>>> +	MIPI_SEQ_ASSERT_RESET,
>>>>
>>>> That naming would be against the spec, so I don't think we want to do it
>>>> like this. Unless the spec is totally wrong, that is.
>>>
>>> Given that both the ADF code and the i915 driver until now have been using
>>> assert on prepare and deassert on unprepare I'm inclined to believe that
>>> the spec is wrong. Is the spec available somewhere public ?
>>
>> I don't think so. And sadly even if it would it wouldn't really help
>> since about the only thing it says is:
>>
>> 00 – Reserved
>> 01 - MIPIAssertResetPin
>> 02 – MIPISendInitialDcsCmds (Use this sequence type for sending initialization commands in LP mode)
>> 03 - MIPIDisplayOn (Use this sequence type for sending initialization commands in HS mode)
>> 04 – MIPIDisplayOff (Use this sequence type for sending DisplayOff commands in LP mode)
>> 05 – MIPIDeassertResetPin
>> 06 – MIPIBacklightOn
>> 07 - MIPIBacklightOff
>> 08 – MIPITearOn
>> 09 - MIPITearOff
>> 10 - MIPIPanelPowerOn
>> 11 - MIPIPanelPowerOff
>> Others – Reserved
>>
>> So pretty much useless if you actually want to write a working driver.
>>
>>>
>>> Also if you look at the v1 sequences with the new names:
>>>
>>> 	MIPI_SEQ_DEASSERT_RESET,
>>>   	MIPI_SEQ_INIT_OTP,
>>>   	MIPI_SEQ_DISPLAY_ON,
>>>   	MIPI_SEQ_DISPLAY_OFF,
>>> 	MIPI_SEQ_ASSERT_RESET,
>>>
>>> Then they are exactly in the order as one would call them on
>>> enable, followed by disable, which I believe is not a coincidence.
>>>
>>>> Can you provide the VBT for the affected machine so other people can
>>>> have a look at it?
>>>
>>> Sure I can do that, what would be the easiest way (both for me to
>>> produce and for you to consume) to do this ?
>>
>> /sys/kernel/debug/dri/0/i915_opregion
>>
>> For the best chance of preserving the dump for posterity I would
>> suggest filing a new bug and attaching it there.
>>
>> https://bugs.freedesktop.org/enter_bug.cgi?product=DRI&component=DRM/Intel
>>
>>>
>>> While developing this set, I've added printk calls to the code executing the
>>> sequences, there are 2 gpios involved nr 60 (backlight enable AFAICT, also used
>>> by the BACKLIGHT sequences) and 72 (reset / panel_enable ?).
>>> When efifb is up both are 1 / high.
>>>
>>> With the OLD naming, MIPI_SEQ_ASSERT_RESET does:
>>>
>>> gpio 72 high
>>> delay
>>> gpio 72 low
>>> delay
>>> gpio 72 high
>>
>> Hmm. OK so it just toggles the reset pin it seems.
>>
>>>
>>> And DEASSERT does:
>>>
>>> gpio 72 low
>>> gpio 60 low
>>
>> And this leaves the reset pin asserted, assuming it's active low,
>> which your patch would seem to confirm.
>>
>>>
>>> So with the old naming DEASSERT leaves the panel disabled / in reset and
>>> the backlight disabled, which is exactly what we do not want...
>>
>> Right. Hmm. If we do flip them over like you suggest I think we'll at
>> least need a big comment to inform people why we seem to go against the
>> spec.
>>
>> I just filed a bug against the spec, but given past history I'm not
>> expecting that to result in much of anything TBH.
>
> Actually I did get a useful response. So yeah, looks like whoever wrote
> the spec was standing on their head or something since the names are
> opposite of how a normal person would make them.

Great, so I guess this means that I no longer need to file a bug
with my VBT ? And that my patch can be merged as is?

> I'll try transcode the diagram I've seen to text:
>
> v2 sequence for video mode:
> - power on
> - wait t1+t2
> - MIPIAssertResetPin
> - clk/data lines to lp-11
> - MIPISendInitialDcsCmds
> - turn on DPI
> - MIPIDisplayOn
> - wait t5
> - backlight on
> ...
> - backlight off
> - wait t6
> - MIPIDisplayOff
> - turn off DPI
> - clk/data lines to lp-00
> - MIPIDeassertResetPin
> - wait t3
> - power off
> - wait t4
>
> v3 sequence for video mode:
> - MIPIPanelPowerOn
> - MIPIAssertResetPin
> - set clk/data lines to lp-11
> - MIPISendInitialDcsCmds (LP)
> - turn on DPI
> - MIPITearOn (command mode only) + MIPIDisplayOn (LP and HS)
> - MIPIBacklightOn
> ...
> - MIPIBacklightOff
> - turn off DPI
> - MIPITearOff + MIPIDisplayOff (LP)
> - clk/data lines to lp-00
> - MIPIDeassertResetPin
> - MIPIPanelPowerOff
>
> If I'm reading that right all the delays are part of the
> VBT seqeuences already, so no need to do them in the driver.

Yes, that seems to match what printk stracing of the
VBT sequences on my tablet shows, all the delays are
already there.

I will try to modify the driver to reproduce the
sequence above as close as possible and see if that
fixes my init problems (I've a video mode panel).


>
> sequence for command mode:
> - power on
> - wait t1+t2
> - MIPIAssertResetPin
> - clk/data lines to lp-11
> - MIPISendInitialDcsCmds
> - MIPITearOn
> - MIPIDisplayOn
> - set pipe to dsr mode
> - wait t5
> - backlight on
> ... issue write_mem_start/write_mem_continue commands ...
> - backlight off
> - wait t6
> - disable pipe dsr mode
> - MIPITearOff
> - MIPIDisplayOff
> - clk/data lines to lp-00
> - MIPIDeassertResetPin
> - wait t3
> - power off
> - wait t4
>
> Obviously there's a bit of other weirdness in those sequences as well
> (eg. the v3 video mode sequence making references to operations done
> only in command mode), and also the MIPIDisplayOff vs. DPI off ordering
> is different between v2 and v3 sequences. And the cmd mode sequence
> looks more like the v2 sequence, so not sure what's the story with v3
> cmd mode really.
>

Question what are the values for the t1 .. t6 times ?
I don't think I will need those for my tablet, but eventually we should
probably try to make the driver match the above sequences in all cases,
I at least want to prepare for that in any patches I send.

Regards,

Hans


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915/dsi: Fix swapping of MIPI_SEQ_DEASSERT_RESET / MIPI_SEQ_ASSERT_RESET

[Hans de Goede]

Hi,

On 29-11-16 14:06, Hans de Goede wrote:

<Snip>

> p.s.
>
> I'm also trying to come up with some patches which properly
> integrate pwm-lpss with the i915 driver instead of it
> throwing a "Failed to own the pwm chip" error. But as soon
> as I hook up things so that pwm_get() returns the pwm-lpss
> pwm0 I hit:
>
> https://bugs.freedesktop.org/show_bug.cgi?id=97330
>
> I get the same pipe-a stuck (or not seeing vblank irqs?)
> problems sometimes without the pwm-lpss driver in the loop
> but then only sometimes and with pwm-lpss I get this
> problem when loading the i915 driver most of the time
> (but not all the time).
>
> I've been debugging this a couple of evenings in a row
> now, but no luck sofar.

Ok, a couple of more evenings of debugging + working on
this today has finally resulted in me finding a fix :)

The problem is that on driver load we call
chv_pipe_power_well_ops.enable() which calls
vlv_init_display_clock_gating() which clears
the DPOUNIT_CLOCK_GATE_DISABLE bit in DSPCLK_GATE_D
while the DSI panel is being driven by PIPE-A, as
already documented in intel_dsi_pre_enable()
allowing the DPOUNIT to get gated while DSI
active will result in a stuck PIPE-A.

I've written a patch for this, I'll send this
right after this mail, and attach it to bug:
https://bugs.freedesktop.org/show_bug.cgi?id=97330

Which I believe is the same issue.

Regards,

Hans
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx