[PATCH 00/18] drm/i915/dsi: Fix / cleanup dsi enable / disable sequences

[PATCH 00/18] drm/i915/dsi: Fix / cleanup dsi enable / disable sequences

[Hans de Goede]

Hi All,

So while trying to fix my cherrytrail tablet's screen sometimes not
initializing properly (*) I started working on this series to cleanup /
(minor) refactor the dsi enable / disable code, with as goal to then
change it to match the enable / disable sequences which Ville Syrjälä
recently dug up from within the spec.

The first 2 patches I've send before, but I'm resending them since
the other patches in this series depend on them.

The 3th patch is a (small) functional change, which makes the
patches following it move less code around.

Patches 4 - 9 move some stuff around with as goal to have
intel_dsi_pre_enable() and intel_dsi_pre_disable() +
intel_dsi_post_disable() have the entire (de)init sequence in a
clearly readable one function call per step (more or less)
style, so that the ordering is obvious and we can easily match
the code to the spec

Patches 10 - 18 actually modify the code to match the spec, there are
9 patches here as I've chosen to do one tiny change per patch so that
regressions can be bisected to a commit more specific then a
"change the world" commit.

After this patch-set the dsi enable / disable code is much easier to
read (IMHO), almost fully matches the spec (with the single exception
documented) and still works (for me).

There are still some possible improvements to consider:

1) intel_dsi_pre_enable starts with enabling the pll, but
   intel_dsi_post_disable disables it half-way through the
   sequence, since then it is no longer necessary. From a
   power-management pov this is good, but it is assymetrical

2) intel_dsi_pre_enable calls intel_dsi_prepare before calling
   intel_dsi_device_ready, so intel_dsi_post_disable should
   call intel_dsi_unprepare *after* intel_dsi_clear_device_ready,
   but it calls it *before*. Because of this intel_dsi_unprepare
   itself temporarily clears device-ready and resets it in the end.
   It would be good to move intel_dsi_unprepare to *after*
   intel_dsi_clear_device_ready and drop its toggling of the
   device-ready bit, but I'm not sure if that is safe. Perhaps
   there is a specific reason why this is done this way?

3) While working on this I found the following patch which maybe
   should be merged to the mainline i915 code ?  :
   https://github.com/01org/ProductionKernelQuilts/blob/master/uefi/cht-m1stable/patches/0002-FOR_UPSTREAM-VPG-drm-i915-Move-port-enable-to-after-.patch

   The date on this patch is from long after the decision to
   move the intel_dsi_enable_port call to intel_dsi_pre_enable,
   so it seems to superseed that decision. Maybe we should move the
   intel_dsi_port_enable call from intel_dsi_pre_enable back
   to intel_dsi_enable[_nop] ?

Regards,

Hans


*) The root cause of which turns out to be something different, but since
I had done most of this work when I found that out, I decided to not throw
away my work and instead finish this series and post it upstream
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 01/18] 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

Re: [PATCH 01/18] drm/i915/dsi: Fix swapping of MIPI_SEQ_DEASSERT_RESET / MIPI_SEQ_ASSERT_RESET

[Ville Syrjälä]

On Thu, Dec 01, 2016 at 09:29:08PM +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,

I think we'll still want to keep to the names as they are in the
spec, and instead we'll just call them in the order that looks
wrong + add a comment explaining why we do that.

>  	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 01/18] drm/i915/dsi: Fix swapping of MIPI_SEQ_DEASSERT_RESET / MIPI_SEQ_ASSERT_RESET

[Hans de Goede]

Hi,

On 02-12-16 13:37, Ville Syrjälä wrote:
> On Thu, Dec 01, 2016 at 09:29:08PM +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,
>
> I think we'll still want to keep to the names as they are in the
> spec, and instead we'll just call them in the order that looks
> wrong + add a comment explaining why we do that.

I really want the i915 driver to use the correct names, anything
else will lead to no amount of confusion for people who have
experience with embedded stuff.

How about adding a comment here in the enum, as well as in
patch 8: "drm/i915/dsi: Document the panel enable / disable sequences from the spec"
That the spec has assert / deassert the wrong way around and that
the i915 code is using the correct names ?

Most people will not even have access to the spec, so it seems to
me that having this right in the code, with a comment to warn
people who do have access to the spec is better then the other
way around.

Regards,

Hans



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

Re: [PATCH 01/18] drm/i915/dsi: Fix swapping of MIPI_SEQ_DEASSERT_RESET / MIPI_SEQ_ASSERT_RESET

[Ville Syrjälä]

On Fri, Dec 02, 2016 at 02:34:01PM +0100, Hans de Goede wrote:
> Hi,
> 
> On 02-12-16 13:37, Ville Syrjälä wrote:
> > On Thu, Dec 01, 2016 at 09:29:08PM +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,
> >
> > I think we'll still want to keep to the names as they are in the
> > spec, and instead we'll just call them in the order that looks
> > wrong + add a comment explaining why we do that.
> 
> I really want the i915 driver to use the correct names, anything
> else will lead to no amount of confusion for people who have
> experience with embedded stuff.
> 
> How about adding a comment here in the enum, as well as in
> patch 8: "drm/i915/dsi: Document the panel enable / disable sequences from the spec"
> That the spec has assert / deassert the wrong way around and that
> the i915 code is using the correct names ?

Sure, I could live with that. We definitely need to have that
comment somewhere.

> 
> Most people will not even have access to the spec, so it seems to
> me that having this right in the code, with a comment to warn
> people who do have access to the spec is better then the other
> way around.
> 
> Regards,
> 
> Hans
> 
> 
> 
> >
> >>  	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

[PATCH 02/18] 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: stable@vger.kernel.org
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>
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.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

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

[Ville Syrjälä]

On Thu, Dec 01, 2016 at 09:29:09PM +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: stable@vger.kernel.org
> 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>
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Pushed to dinq. Thanks for the patch.

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

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

[PATCH 03/18] drm/i915/dsi: Move calling of wait_for_dsi_fifo_empty to mipi_exec_send_packet

[Hans de Goede]

Instead of calling wait_for_dsi_fifo_empty on all dsi ports after calling
a drm_panel_foo helper which calls VBT sequences, move it to the VBT
mipi_exec_send_packet helper, which is the one VBT instruction which
actually puts data in the fifo.

This results in a nice cleanup making it clearer what all the steps on
intel_dsi_enable / disable are and this also makes the VBT code properly
wait till a command has actually been send before executing the next
steps (typically a delay) in the VBT sequence.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/gpu/drm/i915/intel_dsi.c           | 12 +-----------
 drivers/gpu/drm/i915/intel_dsi.h           |  2 ++
 drivers/gpu/drm/i915/intel_dsi_panel_vbt.c |  2 ++
 3 files changed, 5 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
index 3bc6213..587ba07 100644
--- a/drivers/gpu/drm/i915/intel_dsi.c
+++ b/drivers/gpu/drm/i915/intel_dsi.c
@@ -80,7 +80,7 @@ enum mipi_dsi_pixel_format pixel_format_from_register_bits(u32 fmt)
 	}
 }
 
-static void wait_for_dsi_fifo_empty(struct intel_dsi *intel_dsi, enum port port)
+void wait_for_dsi_fifo_empty(struct intel_dsi *intel_dsi, enum port port)
 {
 	struct drm_encoder *encoder = &intel_dsi->base.base;
 	struct drm_device *dev = encoder->dev;
@@ -528,9 +528,6 @@ static void intel_dsi_enable(struct intel_encoder *encoder)
 
 		drm_panel_enable(intel_dsi->panel);
 
-		for_each_dsi_port(port, intel_dsi->ports)
-			wait_for_dsi_fifo_empty(intel_dsi, port);
-
 		intel_dsi_port_enable(encoder);
 	}
 
@@ -546,7 +543,6 @@ static void intel_dsi_pre_enable(struct intel_encoder *encoder,
 {
 	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
 	struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
-	enum port port;
 
 	DRM_DEBUG_KMS("\n");
 
@@ -579,9 +575,6 @@ static void intel_dsi_pre_enable(struct intel_encoder *encoder,
 
 	drm_panel_prepare(intel_dsi->panel);
 
-	for_each_dsi_port(port, intel_dsi->ports)
-		wait_for_dsi_fifo_empty(intel_dsi, port);
-
 	/* Enable port in pre-enable phase itself because as per hw team
 	 * recommendation, port should be enabled befor plane & pipe */
 	intel_dsi_enable(encoder);
@@ -652,9 +645,6 @@ static void intel_dsi_disable(struct intel_encoder *encoder)
 	/* if disable packets are sent before sending shutdown packet then in
 	 * some next enable sequence send turn on packet error is observed */
 	drm_panel_disable(intel_dsi->panel);
-
-	for_each_dsi_port(port, intel_dsi->ports)
-		wait_for_dsi_fifo_empty(intel_dsi, port);
 }
 
 static void intel_dsi_clear_device_ready(struct intel_encoder *encoder)
diff --git a/drivers/gpu/drm/i915/intel_dsi.h b/drivers/gpu/drm/i915/intel_dsi.h
index 5967ea6..d567823 100644
--- a/drivers/gpu/drm/i915/intel_dsi.h
+++ b/drivers/gpu/drm/i915/intel_dsi.h
@@ -130,6 +130,8 @@ static inline struct intel_dsi *enc_to_intel_dsi(struct drm_encoder *encoder)
 	return container_of(encoder, struct intel_dsi, base.base);
 }
 
+void wait_for_dsi_fifo_empty(struct intel_dsi *intel_dsi, enum port port);
+
 bool intel_dsi_pll_is_enabled(struct drm_i915_private *dev_priv);
 int intel_compute_dsi_pll(struct intel_encoder *encoder,
 			  struct intel_crtc_state *config);
diff --git a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
index 47cd1b2..b5a02c6 100644
--- a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
+++ b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
@@ -191,6 +191,8 @@ static const u8 *mipi_exec_send_packet(struct intel_dsi *intel_dsi,
 		break;
 	}
 
+	wait_for_dsi_fifo_empty(intel_dsi, port);
+
 out:
 	data += len;
 
-- 
2.9.3

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

[PATCH 04/18] drm/i915/dsi: Merge intel_dsi_disable/enable into their respective callers

[Hans de Goede]

intel_dsi_disable/enable only have one caller, merge them into their
respective callers.

Change msleep(2) into usleep_range(2000, 5000) to make checkpatch happy,
otherwise no funtional changes.

The main advantage of this change is that it makes it easier to
follow all the steps of the panel enable / disable sequence when
reading the code.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/gpu/drm/i915/intel_dsi.c | 109 ++++++++++++++++-----------------------
 1 file changed, 45 insertions(+), 64 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
index 587ba07..b33381a 100644
--- a/drivers/gpu/drm/i915/intel_dsi.c
+++ b/drivers/gpu/drm/i915/intel_dsi.c
@@ -508,32 +508,6 @@ static void intel_dsi_port_disable(struct intel_encoder *encoder)
 	}
 }
 
-static void intel_dsi_enable(struct intel_encoder *encoder)
-{
-	struct drm_device *dev = encoder->base.dev;
-	struct drm_i915_private *dev_priv = to_i915(dev);
-	struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
-	enum port port;
-
-	DRM_DEBUG_KMS("\n");
-
-	if (is_cmd_mode(intel_dsi)) {
-		for_each_dsi_port(port, intel_dsi->ports)
-			I915_WRITE(MIPI_MAX_RETURN_PKT_SIZE(port), 8 * 4);
-	} else {
-		msleep(20); /* XXX */
-		for_each_dsi_port(port, intel_dsi->ports)
-			dpi_send_cmd(intel_dsi, TURN_ON, false, port);
-		msleep(100);
-
-		drm_panel_enable(intel_dsi->panel);
-
-		intel_dsi_port_enable(encoder);
-	}
-
-	intel_panel_enable_backlight(intel_dsi->attached_connector);
-}
-
 static void intel_dsi_prepare(struct intel_encoder *intel_encoder,
 			      struct intel_crtc_state *pipe_config);
 
@@ -543,6 +517,7 @@ static void intel_dsi_pre_enable(struct intel_encoder *encoder,
 {
 	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
 	struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
+	enum port port;
 
 	DRM_DEBUG_KMS("\n");
 
@@ -577,7 +552,21 @@ static void intel_dsi_pre_enable(struct intel_encoder *encoder,
 
 	/* Enable port in pre-enable phase itself because as per hw team
 	 * recommendation, port should be enabled befor plane & pipe */
-	intel_dsi_enable(encoder);
+	if (is_cmd_mode(intel_dsi)) {
+		for_each_dsi_port(port, intel_dsi->ports)
+			I915_WRITE(MIPI_MAX_RETURN_PKT_SIZE(port), 8 * 4);
+	} else {
+		msleep(20); /* XXX */
+		for_each_dsi_port(port, intel_dsi->ports)
+			dpi_send_cmd(intel_dsi, TURN_ON, false, port);
+		msleep(100);
+
+		drm_panel_enable(intel_dsi->panel);
+
+		intel_dsi_port_enable(encoder);
+	}
+
+	intel_panel_enable_backlight(intel_dsi->attached_connector);
 }
 
 static void intel_dsi_enable_nop(struct intel_encoder *encoder,
@@ -611,42 +600,6 @@ static void intel_dsi_pre_disable(struct intel_encoder *encoder,
 	}
 }
 
-static void intel_dsi_disable(struct intel_encoder *encoder)
-{
-	struct drm_device *dev = encoder->base.dev;
-	struct drm_i915_private *dev_priv = to_i915(dev);
-	struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
-	enum port port;
-	u32 temp;
-
-	DRM_DEBUG_KMS("\n");
-
-	if (is_vid_mode(intel_dsi)) {
-		for_each_dsi_port(port, intel_dsi->ports)
-			wait_for_dsi_fifo_empty(intel_dsi, port);
-
-		intel_dsi_port_disable(encoder);
-		msleep(2);
-	}
-
-	for_each_dsi_port(port, intel_dsi->ports) {
-		/* Panel commands can be sent when clock is in LP11 */
-		I915_WRITE(MIPI_DEVICE_READY(port), 0x0);
-
-		intel_dsi_reset_clocks(encoder, port);
-		I915_WRITE(MIPI_EOT_DISABLE(port), CLOCKSTOP);
-
-		temp = I915_READ(MIPI_DSI_FUNC_PRG(port));
-		temp &= ~VID_MODE_FORMAT_MASK;
-		I915_WRITE(MIPI_DSI_FUNC_PRG(port), temp);
-
-		I915_WRITE(MIPI_DEVICE_READY(port), 0x1);
-	}
-	/* if disable packets are sent before sending shutdown packet then in
-	 * some next enable sequence send turn on packet error is observed */
-	drm_panel_disable(intel_dsi->panel);
-}
-
 static void intel_dsi_clear_device_ready(struct intel_encoder *encoder)
 {
 	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
@@ -698,10 +651,38 @@ static void intel_dsi_post_disable(struct intel_encoder *encoder,
 {
 	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
 	struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
+	enum port port;
+	u32 temp;
 
 	DRM_DEBUG_KMS("\n");
 
-	intel_dsi_disable(encoder);
+	if (is_vid_mode(intel_dsi)) {
+		for_each_dsi_port(port, intel_dsi->ports)
+			wait_for_dsi_fifo_empty(intel_dsi, port);
+
+		intel_dsi_port_disable(encoder);
+		usleep_range(2000, 5000);
+	}
+
+	for_each_dsi_port(port, intel_dsi->ports) {
+		/* Panel commands can be sent when clock is in LP11 */
+		I915_WRITE(MIPI_DEVICE_READY(port), 0x0);
+
+		intel_dsi_reset_clocks(encoder, port);
+		I915_WRITE(MIPI_EOT_DISABLE(port), CLOCKSTOP);
+
+		temp = I915_READ(MIPI_DSI_FUNC_PRG(port));
+		temp &= ~VID_MODE_FORMAT_MASK;
+		I915_WRITE(MIPI_DSI_FUNC_PRG(port), temp);
+
+		I915_WRITE(MIPI_DEVICE_READY(port), 0x1);
+	}
+
+	/*
+	 * if disable packets are sent before sending shutdown packet then in
+	 * some next enable sequence send turn on packet error is observed
+	 */
+	drm_panel_disable(intel_dsi->panel);
 
 	intel_dsi_clear_device_ready(encoder);
 
-- 
2.9.3

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

[PATCH 05/18] drm/i915/dsi: Add intel_dsi_unprepare() helper

[Hans de Goede]

The enable path has an intel_dsi_prepare() helper which prepares various
registers for the mode-set. Move the function undoing this to a new
intel_dsi_unprepare() helper for better symmetry between the enable and
disable paths. No functional changes.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/gpu/drm/i915/intel_dsi.c | 38 ++++++++++++++++++++++++--------------
 1 file changed, 24 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
index b33381a..967bae9 100644
--- a/drivers/gpu/drm/i915/intel_dsi.c
+++ b/drivers/gpu/drm/i915/intel_dsi.c
@@ -510,6 +510,7 @@ static void intel_dsi_port_disable(struct intel_encoder *encoder)
 
 static void intel_dsi_prepare(struct intel_encoder *intel_encoder,
 			      struct intel_crtc_state *pipe_config);
+static void intel_dsi_unprepare(struct intel_encoder *encoder);
 
 static void intel_dsi_pre_enable(struct intel_encoder *encoder,
 				 struct intel_crtc_state *pipe_config,
@@ -652,7 +653,6 @@ static void intel_dsi_post_disable(struct intel_encoder *encoder,
 	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
 	struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
 	enum port port;
-	u32 temp;
 
 	DRM_DEBUG_KMS("\n");
 
@@ -664,19 +664,7 @@ static void intel_dsi_post_disable(struct intel_encoder *encoder,
 		usleep_range(2000, 5000);
 	}
 
-	for_each_dsi_port(port, intel_dsi->ports) {
-		/* Panel commands can be sent when clock is in LP11 */
-		I915_WRITE(MIPI_DEVICE_READY(port), 0x0);
-
-		intel_dsi_reset_clocks(encoder, port);
-		I915_WRITE(MIPI_EOT_DISABLE(port), CLOCKSTOP);
-
-		temp = I915_READ(MIPI_DSI_FUNC_PRG(port));
-		temp &= ~VID_MODE_FORMAT_MASK;
-		I915_WRITE(MIPI_DSI_FUNC_PRG(port), temp);
-
-		I915_WRITE(MIPI_DEVICE_READY(port), 0x1);
-	}
+	intel_dsi_unprepare(encoder);
 
 	/*
 	 * if disable packets are sent before sending shutdown packet then in
@@ -1272,6 +1260,28 @@ static void intel_dsi_prepare(struct intel_encoder *intel_encoder,
 	}
 }
 
+static void intel_dsi_unprepare(struct intel_encoder *encoder)
+{
+	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
+	struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
+	enum port port;
+	u32 temp;
+
+	for_each_dsi_port(port, intel_dsi->ports) {
+		/* Panel commands can be sent when clock is in LP11 */
+		I915_WRITE(MIPI_DEVICE_READY(port), 0x0);
+
+		intel_dsi_reset_clocks(encoder, port);
+		I915_WRITE(MIPI_EOT_DISABLE(port), CLOCKSTOP);
+
+		temp = I915_READ(MIPI_DSI_FUNC_PRG(port));
+		temp &= ~VID_MODE_FORMAT_MASK;
+		I915_WRITE(MIPI_DSI_FUNC_PRG(port), temp);
+
+		I915_WRITE(MIPI_DEVICE_READY(port), 0x1);
+	}
+}
+
 static enum drm_connector_status
 intel_dsi_detect(struct drm_connector *connector, bool force)
 {
-- 
2.9.3

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

[PATCH 06/18] drm/i915/dsi: Move disable pll call outside of clear_device_ready()

[Hans de Goede]

On enable intel_dsi_enable() directly calls intel_enable_dsi_pll(),
make intel_dsi_disable() also directly call intel_disable_dsi_pll(),
rather then hiding the call in intel_dsi_clear_device_ready(),
no functional changes.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/gpu/drm/i915/intel_dsi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
index 967bae9..cb15c0a 100644
--- a/drivers/gpu/drm/i915/intel_dsi.c
+++ b/drivers/gpu/drm/i915/intel_dsi.c
@@ -642,8 +642,6 @@ static void intel_dsi_clear_device_ready(struct intel_encoder *encoder)
 		I915_WRITE(MIPI_DEVICE_READY(port), 0x00);
 		usleep_range(2000, 2500);
 	}
-
-	intel_disable_dsi_pll(encoder);
 }
 
 static void intel_dsi_post_disable(struct intel_encoder *encoder,
@@ -674,6 +672,8 @@ static void intel_dsi_post_disable(struct intel_encoder *encoder,
 
 	intel_dsi_clear_device_ready(encoder);
 
+	intel_disable_dsi_pll(encoder);
+
 	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
 		u32 val;
 
-- 
2.9.3

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

Re: [PATCH 06/18] drm/i915/dsi: Move disable pll call outside of clear_device_ready()

[Jani Nikula]

On Thu, 01 Dec 2016, Hans de Goede <hdegoede@redhat.com> wrote:
> On enable intel_dsi_enable() directly calls intel_enable_dsi_pll(),
> make intel_dsi_disable() also directly call intel_disable_dsi_pll(),
> rather then hiding the call in intel_dsi_clear_device_ready(),
> no functional changes.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Pushed this one to drm-intel-next-queued, thanks for the patch.

BR,
Jani.

> ---
>  drivers/gpu/drm/i915/intel_dsi.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
> index 967bae9..cb15c0a 100644
> --- a/drivers/gpu/drm/i915/intel_dsi.c
> +++ b/drivers/gpu/drm/i915/intel_dsi.c
> @@ -642,8 +642,6 @@ static void intel_dsi_clear_device_ready(struct intel_encoder *encoder)
>  		I915_WRITE(MIPI_DEVICE_READY(port), 0x00);
>  		usleep_range(2000, 2500);
>  	}
> -
> -	intel_disable_dsi_pll(encoder);
>  }
>  
>  static void intel_dsi_post_disable(struct intel_encoder *encoder,
> @@ -674,6 +672,8 @@ static void intel_dsi_post_disable(struct intel_encoder *encoder,
>  
>  	intel_dsi_clear_device_ready(encoder);
>  
> +	intel_disable_dsi_pll(encoder);
> +
>  	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
>  		u32 val;

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 07/18] drm/i915/dsi: Move intel_dsi_clear_device_ready()

[Hans de Goede]

Move the intel_dsi_clear_device_ready() function to higher up in
intel_dsi.c this pairs it with intel_dsi_device_ready(); and pairs
intel_dsi_*enable* with intel_dsi_*disable without
intel_dsi_clear_device_ready() sitting in the middle of them.

This commit purely moves code around, it does not make any
changes what-so-ever.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/gpu/drm/i915/intel_dsi.c | 86 ++++++++++++++++++++--------------------
 1 file changed, 43 insertions(+), 43 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
index cb15c0a..2222229 100644
--- a/drivers/gpu/drm/i915/intel_dsi.c
+++ b/drivers/gpu/drm/i915/intel_dsi.c
@@ -445,6 +445,49 @@ static void intel_dsi_device_ready(struct intel_encoder *encoder)
 		bxt_dsi_device_ready(encoder);
 }
 
+static void intel_dsi_clear_device_ready(struct intel_encoder *encoder)
+{
+	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
+	struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
+	enum port port;
+
+	DRM_DEBUG_KMS("\n");
+	for_each_dsi_port(port, intel_dsi->ports) {
+		/* Common bit for both MIPI Port A & MIPI Port C on VLV/CHV */
+		i915_reg_t port_ctrl = IS_BROXTON(dev_priv) ?
+			BXT_MIPI_PORT_CTRL(port) : MIPI_PORT_CTRL(PORT_A);
+		u32 val;
+
+		I915_WRITE(MIPI_DEVICE_READY(port), DEVICE_READY |
+							ULPS_STATE_ENTER);
+		usleep_range(2000, 2500);
+
+		I915_WRITE(MIPI_DEVICE_READY(port), DEVICE_READY |
+							ULPS_STATE_EXIT);
+		usleep_range(2000, 2500);
+
+		I915_WRITE(MIPI_DEVICE_READY(port), DEVICE_READY |
+							ULPS_STATE_ENTER);
+		usleep_range(2000, 2500);
+
+		/* Wait till Clock lanes are in LP-00 state for MIPI Port A
+		 * only. MIPI Port C has no similar bit for checking
+		 */
+		if (intel_wait_for_register(dev_priv,
+					    port_ctrl, AFE_LATCHOUT, 0,
+					    30))
+			DRM_ERROR("DSI LP not going Low\n");
+
+		/* Disable MIPI PHY transparent latch */
+		val = I915_READ(port_ctrl);
+		I915_WRITE(port_ctrl, val & ~LP_OUTPUT_HOLD);
+		usleep_range(1000, 1500);
+
+		I915_WRITE(MIPI_DEVICE_READY(port), 0x00);
+		usleep_range(2000, 2500);
+	}
+}
+
 static void intel_dsi_port_enable(struct intel_encoder *encoder)
 {
 	struct drm_device *dev = encoder->base.dev;
@@ -601,49 +644,6 @@ static void intel_dsi_pre_disable(struct intel_encoder *encoder,
 	}
 }
 
-static void intel_dsi_clear_device_ready(struct intel_encoder *encoder)
-{
-	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
-	struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
-	enum port port;
-
-	DRM_DEBUG_KMS("\n");
-	for_each_dsi_port(port, intel_dsi->ports) {
-		/* Common bit for both MIPI Port A & MIPI Port C on VLV/CHV */
-		i915_reg_t port_ctrl = IS_BROXTON(dev_priv) ?
-			BXT_MIPI_PORT_CTRL(port) : MIPI_PORT_CTRL(PORT_A);
-		u32 val;
-
-		I915_WRITE(MIPI_DEVICE_READY(port), DEVICE_READY |
-							ULPS_STATE_ENTER);
-		usleep_range(2000, 2500);
-
-		I915_WRITE(MIPI_DEVICE_READY(port), DEVICE_READY |
-							ULPS_STATE_EXIT);
-		usleep_range(2000, 2500);
-
-		I915_WRITE(MIPI_DEVICE_READY(port), DEVICE_READY |
-							ULPS_STATE_ENTER);
-		usleep_range(2000, 2500);
-
-		/* Wait till Clock lanes are in LP-00 state for MIPI Port A
-		 * only. MIPI Port C has no similar bit for checking
-		 */
-		if (intel_wait_for_register(dev_priv,
-					    port_ctrl, AFE_LATCHOUT, 0,
-					    30))
-			DRM_ERROR("DSI LP not going Low\n");
-
-		/* Disable MIPI PHY transparent latch */
-		val = I915_READ(port_ctrl);
-		I915_WRITE(port_ctrl, val & ~LP_OUTPUT_HOLD);
-		usleep_range(1000, 1500);
-
-		I915_WRITE(MIPI_DEVICE_READY(port), 0x00);
-		usleep_range(2000, 2500);
-	}
-}
-
 static void intel_dsi_post_disable(struct intel_encoder *encoder,
 				   struct intel_crtc_state *pipe_config,
 				   struct drm_connector_state *conn_state)
-- 
2.9.3

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

[PATCH 08/18] drm/i915/dsi: Document the panel enable / disable sequences from the spec

[Hans de Goede]

Document the DSI panel enable / disable sequences from the spec,
for easy comparison between the code and the spec.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/gpu/drm/i915/intel_dsi.c | 64 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 64 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
index 2222229..85b748d 100644
--- a/drivers/gpu/drm/i915/intel_dsi.c
+++ b/drivers/gpu/drm/i915/intel_dsi.c
@@ -555,6 +555,70 @@ static void intel_dsi_prepare(struct intel_encoder *intel_encoder,
 			      struct intel_crtc_state *pipe_config);
 static void intel_dsi_unprepare(struct intel_encoder *encoder);
 
+/*
+ * Panel enable/disable sequences from the spec:
+ *
+ * v2 sequence for video mode:
+ * - power on
+ * - wait t1+t2
+ * - MIPIDeassertResetPin
+ * - 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
+ * - MIPIAssertResetPin
+ * - wait t3
+ * - power off
+ * - wait t4
+ *
+ * v3 sequence for video mode:
+ * - MIPIPanelPowerOn
+ * - MIPIDeassertResetPin
+ * - 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
+ * - MIPIAssertResetPin
+ * - MIPIPanelPowerOff
+ *
+ * sequence for command mode:
+ * - power on
+ * - wait t1+t2
+ * - MIPIDeassertResetPin
+ * - 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
+ * - MIPIAssertResetPin
+ * - wait t3
+ * - power off
+ * - wait t4
+ */
+
 static void intel_dsi_pre_enable(struct intel_encoder *encoder,
 				 struct intel_crtc_state *pipe_config,
 				 struct drm_connector_state *conn_state)
-- 
2.9.3

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

Re: [PATCH 08/18] drm/i915/dsi: Document the panel enable / disable sequences from the spec

[Jani Nikula]

On Thu, 01 Dec 2016, Hans de Goede <hdegoede@redhat.com> wrote:
> Document the DSI panel enable / disable sequences from the spec,
> for easy comparison between the code and the spec.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

I haven't done a proper review, but patches 1-8 are

Acked-by: Jani Nikula <jani.nikula@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_dsi.c | 64 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 64 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
> index 2222229..85b748d 100644
> --- a/drivers/gpu/drm/i915/intel_dsi.c
> +++ b/drivers/gpu/drm/i915/intel_dsi.c
> @@ -555,6 +555,70 @@ static void intel_dsi_prepare(struct intel_encoder *intel_encoder,
>  			      struct intel_crtc_state *pipe_config);
>  static void intel_dsi_unprepare(struct intel_encoder *encoder);
>  
> +/*
> + * Panel enable/disable sequences from the spec:
> + *
> + * v2 sequence for video mode:
> + * - power on
> + * - wait t1+t2
> + * - MIPIDeassertResetPin
> + * - 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
> + * - MIPIAssertResetPin
> + * - wait t3
> + * - power off
> + * - wait t4
> + *
> + * v3 sequence for video mode:
> + * - MIPIPanelPowerOn
> + * - MIPIDeassertResetPin
> + * - 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
> + * - MIPIAssertResetPin
> + * - MIPIPanelPowerOff
> + *
> + * sequence for command mode:
> + * - power on
> + * - wait t1+t2
> + * - MIPIDeassertResetPin
> + * - 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
> + * - MIPIAssertResetPin
> + * - wait t3
> + * - power off
> + * - wait t4
> + */
> +
>  static void intel_dsi_pre_enable(struct intel_encoder *encoder,
>  				 struct intel_crtc_state *pipe_config,
>  				 struct drm_connector_state *conn_state)

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 09/18] drm/i915/dsi: Make intel_dsi_enable/disable directly exec VBT sequences

[Hans de Goede]

The drm_panel_enable/disable and drm_panel_prepare/unprepare calls are
not fine grained enough to abstract all the different steps we need to
take (and VBT sequences we need to exec) properly. So simply remove the
panel _enable/disable and prepare/unprepare callbacks and instead
export intel_dsi_exec_vbt_sequence() from intel_dsi_panel_vbt.c
and call that from intel_dsi_enable/disable().

No functional changes.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/gpu/drm/i915/intel_dsi.c           | 14 +++++++---
 drivers/gpu/drm/i915/intel_dsi.h           |  3 +++
 drivers/gpu/drm/i915/intel_dsi_panel_vbt.c | 43 ++----------------------------
 3 files changed, 15 insertions(+), 45 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
index 85b748d..cf761e8 100644
--- a/drivers/gpu/drm/i915/intel_dsi.c
+++ b/drivers/gpu/drm/i915/intel_dsi.c
@@ -656,7 +656,10 @@ static void intel_dsi_pre_enable(struct intel_encoder *encoder,
 	/* put device in ready state */
 	intel_dsi_device_ready(encoder);
 
-	drm_panel_prepare(intel_dsi->panel);
+	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_ASSERT_RESET);
+	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_POWER_ON);
+	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_DEASSERT_RESET);
+	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_INIT_OTP);
 
 	/* Enable port in pre-enable phase itself because as per hw team
 	 * recommendation, port should be enabled befor plane & pipe */
@@ -669,7 +672,8 @@ static void intel_dsi_pre_enable(struct intel_encoder *encoder,
 			dpi_send_cmd(intel_dsi, TURN_ON, false, port);
 		msleep(100);
 
-		drm_panel_enable(intel_dsi->panel);
+		intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_DISPLAY_ON);
+		intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_BACKLIGHT_ON);
 
 		intel_dsi_port_enable(encoder);
 	}
@@ -732,7 +736,8 @@ static void intel_dsi_post_disable(struct intel_encoder *encoder,
 	 * if disable packets are sent before sending shutdown packet then in
 	 * some next enable sequence send turn on packet error is observed
 	 */
-	drm_panel_disable(intel_dsi->panel);
+	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_BACKLIGHT_OFF);
+	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_DISPLAY_OFF);
 
 	intel_dsi_clear_device_ready(encoder);
 
@@ -746,7 +751,8 @@ static void intel_dsi_post_disable(struct intel_encoder *encoder,
 		I915_WRITE(DSPCLK_GATE_D, val);
 	}
 
-	drm_panel_unprepare(intel_dsi->panel);
+	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_ASSERT_RESET);
+	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_POWER_OFF);
 
 	msleep(intel_dsi->panel_off_delay);
 
diff --git a/drivers/gpu/drm/i915/intel_dsi.h b/drivers/gpu/drm/i915/intel_dsi.h
index d567823..5486491 100644
--- a/drivers/gpu/drm/i915/intel_dsi.h
+++ b/drivers/gpu/drm/i915/intel_dsi.h
@@ -132,6 +132,9 @@ static inline struct intel_dsi *enc_to_intel_dsi(struct drm_encoder *encoder)
 
 void wait_for_dsi_fifo_empty(struct intel_dsi *intel_dsi, enum port port);
 
+void intel_dsi_exec_vbt_sequence(struct intel_dsi *intel_dsi,
+				 enum mipi_seq seq_id);
+
 bool intel_dsi_pll_is_enabled(struct drm_i915_private *dev_priv);
 int intel_compute_dsi_pll(struct intel_encoder *encoder,
 			  struct intel_crtc_state *config);
diff --git a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
index b5a02c6..f71f913 100644
--- a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
+++ b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
@@ -400,10 +400,9 @@ static const char *sequence_name(enum mipi_seq seq_id)
 		return "(unknown)";
 }
 
-static void generic_exec_sequence(struct drm_panel *panel, enum mipi_seq seq_id)
+void intel_dsi_exec_vbt_sequence(struct intel_dsi *intel_dsi,
+				 enum mipi_seq seq_id)
 {
-	struct vbt_panel *vbt_panel = to_vbt_panel(panel);
-	struct intel_dsi *intel_dsi = vbt_panel->intel_dsi;
 	struct drm_i915_private *dev_priv = to_i915(intel_dsi->base.base.dev);
 	const u8 *data;
 	fn_mipi_elem_exec mipi_elem_exec;
@@ -467,40 +466,6 @@ static void generic_exec_sequence(struct drm_panel *panel, enum mipi_seq seq_id)
 	}
 }
 
-static int vbt_panel_prepare(struct drm_panel *panel)
-{
-	generic_exec_sequence(panel, MIPI_SEQ_ASSERT_RESET);
-	generic_exec_sequence(panel, MIPI_SEQ_POWER_ON);
-	generic_exec_sequence(panel, MIPI_SEQ_DEASSERT_RESET);
-	generic_exec_sequence(panel, MIPI_SEQ_INIT_OTP);
-
-	return 0;
-}
-
-static int vbt_panel_unprepare(struct drm_panel *panel)
-{
-	generic_exec_sequence(panel, MIPI_SEQ_ASSERT_RESET);
-	generic_exec_sequence(panel, MIPI_SEQ_POWER_OFF);
-
-	return 0;
-}
-
-static int vbt_panel_enable(struct drm_panel *panel)
-{
-	generic_exec_sequence(panel, MIPI_SEQ_DISPLAY_ON);
-	generic_exec_sequence(panel, MIPI_SEQ_BACKLIGHT_ON);
-
-	return 0;
-}
-
-static int vbt_panel_disable(struct drm_panel *panel)
-{
-	generic_exec_sequence(panel, MIPI_SEQ_BACKLIGHT_OFF);
-	generic_exec_sequence(panel, MIPI_SEQ_DISPLAY_OFF);
-
-	return 0;
-}
-
 static int vbt_panel_get_modes(struct drm_panel *panel)
 {
 	struct vbt_panel *vbt_panel = to_vbt_panel(panel);
@@ -524,10 +489,6 @@ static int vbt_panel_get_modes(struct drm_panel *panel)
 }
 
 static const struct drm_panel_funcs vbt_panel_funcs = {
-	.disable = vbt_panel_disable,
-	.unprepare = vbt_panel_unprepare,
-	.prepare = vbt_panel_prepare,
-	.enable = vbt_panel_enable,
 	.get_modes = vbt_panel_get_modes,
 };
 
-- 
2.9.3

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

Re: [PATCH 09/18] drm/i915/dsi: Make intel_dsi_enable/disable directly exec VBT sequences

[Jani Nikula]

On Thu, 01 Dec 2016, Hans de Goede <hdegoede@redhat.com> wrote:
> The drm_panel_enable/disable and drm_panel_prepare/unprepare calls are
> not fine grained enough to abstract all the different steps we need to
> take (and VBT sequences we need to exec) properly. So simply remove the
> panel _enable/disable and prepare/unprepare callbacks and instead
> export intel_dsi_exec_vbt_sequence() from intel_dsi_panel_vbt.c
> and call that from intel_dsi_enable/disable().
>
> No functional changes.

No functional changes, but this is quite a big design change between
intel_dsi.c and intel_dsi_panel_vbt.c. Originally the idea was to
separate the dsi core code and the panel specific code, letting one
write a panel driver that was not based on values in VBT. This patch
changes that.

Now I'm not sure if there'll ever be a panel driver not based on VBT,
and perhaps the current drm_panel based interface between two is not
enough in the end, nor prettiest. But after this patch, we might just as
well throw out the drm_panel interface, and refactor the division
between the two files completely. Indeed, if we accept the direction set
in this patch, that refactoring would be the logical next step.

I have not made up my mind about this yet. An alternative would be to
amend the drm_panel interface to achieve the kind of granularity you're
after in the follow-up patches. Indeed, such patches have been sent in
the past.

BR,
Jani.

>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/gpu/drm/i915/intel_dsi.c           | 14 +++++++---
>  drivers/gpu/drm/i915/intel_dsi.h           |  3 +++
>  drivers/gpu/drm/i915/intel_dsi_panel_vbt.c | 43 ++----------------------------
>  3 files changed, 15 insertions(+), 45 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
> index 85b748d..cf761e8 100644
> --- a/drivers/gpu/drm/i915/intel_dsi.c
> +++ b/drivers/gpu/drm/i915/intel_dsi.c
> @@ -656,7 +656,10 @@ static void intel_dsi_pre_enable(struct intel_encoder *encoder,
>  	/* put device in ready state */
>  	intel_dsi_device_ready(encoder);
>  
> -	drm_panel_prepare(intel_dsi->panel);
> +	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_ASSERT_RESET);
> +	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_POWER_ON);
> +	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_DEASSERT_RESET);
> +	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_INIT_OTP);
>  
>  	/* Enable port in pre-enable phase itself because as per hw team
>  	 * recommendation, port should be enabled befor plane & pipe */
> @@ -669,7 +672,8 @@ static void intel_dsi_pre_enable(struct intel_encoder *encoder,
>  			dpi_send_cmd(intel_dsi, TURN_ON, false, port);
>  		msleep(100);
>  
> -		drm_panel_enable(intel_dsi->panel);
> +		intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_DISPLAY_ON);
> +		intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_BACKLIGHT_ON);
>  
>  		intel_dsi_port_enable(encoder);
>  	}
> @@ -732,7 +736,8 @@ static void intel_dsi_post_disable(struct intel_encoder *encoder,
>  	 * if disable packets are sent before sending shutdown packet then in
>  	 * some next enable sequence send turn on packet error is observed
>  	 */
> -	drm_panel_disable(intel_dsi->panel);
> +	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_BACKLIGHT_OFF);
> +	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_DISPLAY_OFF);
>  
>  	intel_dsi_clear_device_ready(encoder);
>  
> @@ -746,7 +751,8 @@ static void intel_dsi_post_disable(struct intel_encoder *encoder,
>  		I915_WRITE(DSPCLK_GATE_D, val);
>  	}
>  
> -	drm_panel_unprepare(intel_dsi->panel);
> +	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_ASSERT_RESET);
> +	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_POWER_OFF);
>  
>  	msleep(intel_dsi->panel_off_delay);
>  
> diff --git a/drivers/gpu/drm/i915/intel_dsi.h b/drivers/gpu/drm/i915/intel_dsi.h
> index d567823..5486491 100644
> --- a/drivers/gpu/drm/i915/intel_dsi.h
> +++ b/drivers/gpu/drm/i915/intel_dsi.h
> @@ -132,6 +132,9 @@ static inline struct intel_dsi *enc_to_intel_dsi(struct drm_encoder *encoder)
>  
>  void wait_for_dsi_fifo_empty(struct intel_dsi *intel_dsi, enum port port);
>  
> +void intel_dsi_exec_vbt_sequence(struct intel_dsi *intel_dsi,
> +				 enum mipi_seq seq_id);
> +
>  bool intel_dsi_pll_is_enabled(struct drm_i915_private *dev_priv);
>  int intel_compute_dsi_pll(struct intel_encoder *encoder,
>  			  struct intel_crtc_state *config);
> diff --git a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
> index b5a02c6..f71f913 100644
> --- a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
> +++ b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
> @@ -400,10 +400,9 @@ static const char *sequence_name(enum mipi_seq seq_id)
>  		return "(unknown)";
>  }
>  
> -static void generic_exec_sequence(struct drm_panel *panel, enum mipi_seq seq_id)
> +void intel_dsi_exec_vbt_sequence(struct intel_dsi *intel_dsi,
> +				 enum mipi_seq seq_id)
>  {
> -	struct vbt_panel *vbt_panel = to_vbt_panel(panel);
> -	struct intel_dsi *intel_dsi = vbt_panel->intel_dsi;
>  	struct drm_i915_private *dev_priv = to_i915(intel_dsi->base.base.dev);
>  	const u8 *data;
>  	fn_mipi_elem_exec mipi_elem_exec;
> @@ -467,40 +466,6 @@ static void generic_exec_sequence(struct drm_panel *panel, enum mipi_seq seq_id)
>  	}
>  }
>  
> -static int vbt_panel_prepare(struct drm_panel *panel)
> -{
> -	generic_exec_sequence(panel, MIPI_SEQ_ASSERT_RESET);
> -	generic_exec_sequence(panel, MIPI_SEQ_POWER_ON);
> -	generic_exec_sequence(panel, MIPI_SEQ_DEASSERT_RESET);
> -	generic_exec_sequence(panel, MIPI_SEQ_INIT_OTP);
> -
> -	return 0;
> -}
> -
> -static int vbt_panel_unprepare(struct drm_panel *panel)
> -{
> -	generic_exec_sequence(panel, MIPI_SEQ_ASSERT_RESET);
> -	generic_exec_sequence(panel, MIPI_SEQ_POWER_OFF);
> -
> -	return 0;
> -}
> -
> -static int vbt_panel_enable(struct drm_panel *panel)
> -{
> -	generic_exec_sequence(panel, MIPI_SEQ_DISPLAY_ON);
> -	generic_exec_sequence(panel, MIPI_SEQ_BACKLIGHT_ON);
> -
> -	return 0;
> -}
> -
> -static int vbt_panel_disable(struct drm_panel *panel)
> -{
> -	generic_exec_sequence(panel, MIPI_SEQ_BACKLIGHT_OFF);
> -	generic_exec_sequence(panel, MIPI_SEQ_DISPLAY_OFF);
> -
> -	return 0;
> -}
> -
>  static int vbt_panel_get_modes(struct drm_panel *panel)
>  {
>  	struct vbt_panel *vbt_panel = to_vbt_panel(panel);
> @@ -524,10 +489,6 @@ static int vbt_panel_get_modes(struct drm_panel *panel)
>  }
>  
>  static const struct drm_panel_funcs vbt_panel_funcs = {
> -	.disable = vbt_panel_disable,
> -	.unprepare = vbt_panel_unprepare,
> -	.prepare = vbt_panel_prepare,
> -	.enable = vbt_panel_enable,
>  	.get_modes = vbt_panel_get_modes,
>  };

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 09/18] drm/i915/dsi: Make intel_dsi_enable/disable directly exec VBT sequences

[Hans de Goede]

Hi,

On 02-12-16 11:31, Jani Nikula wrote:
> On Thu, 01 Dec 2016, Hans de Goede <hdegoede@redhat.com> wrote:
>> The drm_panel_enable/disable and drm_panel_prepare/unprepare calls are
>> not fine grained enough to abstract all the different steps we need to
>> take (and VBT sequences we need to exec) properly. So simply remove the
>> panel _enable/disable and prepare/unprepare callbacks and instead
>> export intel_dsi_exec_vbt_sequence() from intel_dsi_panel_vbt.c
>> and call that from intel_dsi_enable/disable().
>>
>> No functional changes.
>
> No functional changes, but this is quite a big design change between
> intel_dsi.c and intel_dsi_panel_vbt.c.

True, I did this because adding a callback per init step to the
drm_panel interface felt like it will result in a game of
whack a mole (adding more and more callbacks). See the end of
this mail for a proposal with leaves the drm_panel interface
in place, while also avoiding this problem.

> Originally the idea was to
> separate the dsi core code and the panel specific code, letting one
> write a panel driver that was not based on values in VBT. This patch
> changes that.

I see, I was not aware of this history.

> Now I'm not sure if there'll ever be a panel driver not based on VBT,
> and perhaps the current drm_panel based interface between two is not
> enough in the end, nor prettiest. But after this patch, we might just as
> well throw out the drm_panel interface, and refactor the division
> between the two files completely. Indeed, if we accept the direction set
> in this patch, that refactoring would be the logical next step.

Is intel_dsi.c the only user of the drm_panel interface ? The name
suggests it is not intel specific.

> I have not made up my mind about this yet. An alternative would be to
> amend the drm_panel interface to achieve the kind of granularity you're
> after in the follow-up patches. Indeed, such patches have been sent in
> the past.

How about we add a "drm_panel_exec_sequence" callback to the
drm_panel interface, which takes an enum argument which (de)init
step to do ?

Then we can easily add extra init steps later by extending the
enum, and panel implementations which do not care about certain
steps can simply treat these as nops. This avoids the need
to add a ton of callbacks to the drm_panel struct.

If there are no other users, we could then also remove the
enable/disable and prepare/unprepare pairs from drm_panel
now, I left those in place assuming that intel_dsi.c was
not the only user of drm_panel.

Regards,

Hans




>
> BR,
> Jani.
>
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  drivers/gpu/drm/i915/intel_dsi.c           | 14 +++++++---
>>  drivers/gpu/drm/i915/intel_dsi.h           |  3 +++
>>  drivers/gpu/drm/i915/intel_dsi_panel_vbt.c | 43 ++----------------------------
>>  3 files changed, 15 insertions(+), 45 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
>> index 85b748d..cf761e8 100644
>> --- a/drivers/gpu/drm/i915/intel_dsi.c
>> +++ b/drivers/gpu/drm/i915/intel_dsi.c
>> @@ -656,7 +656,10 @@ static void intel_dsi_pre_enable(struct intel_encoder *encoder,
>>  	/* put device in ready state */
>>  	intel_dsi_device_ready(encoder);
>>
>> -	drm_panel_prepare(intel_dsi->panel);
>> +	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_ASSERT_RESET);
>> +	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_POWER_ON);
>> +	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_DEASSERT_RESET);
>> +	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_INIT_OTP);
>>
>>  	/* Enable port in pre-enable phase itself because as per hw team
>>  	 * recommendation, port should be enabled befor plane & pipe */
>> @@ -669,7 +672,8 @@ static void intel_dsi_pre_enable(struct intel_encoder *encoder,
>>  			dpi_send_cmd(intel_dsi, TURN_ON, false, port);
>>  		msleep(100);
>>
>> -		drm_panel_enable(intel_dsi->panel);
>> +		intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_DISPLAY_ON);
>> +		intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_BACKLIGHT_ON);
>>
>>  		intel_dsi_port_enable(encoder);
>>  	}
>> @@ -732,7 +736,8 @@ static void intel_dsi_post_disable(struct intel_encoder *encoder,
>>  	 * if disable packets are sent before sending shutdown packet then in
>>  	 * some next enable sequence send turn on packet error is observed
>>  	 */
>> -	drm_panel_disable(intel_dsi->panel);
>> +	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_BACKLIGHT_OFF);
>> +	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_DISPLAY_OFF);
>>
>>  	intel_dsi_clear_device_ready(encoder);
>>
>> @@ -746,7 +751,8 @@ static void intel_dsi_post_disable(struct intel_encoder *encoder,
>>  		I915_WRITE(DSPCLK_GATE_D, val);
>>  	}
>>
>> -	drm_panel_unprepare(intel_dsi->panel);
>> +	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_ASSERT_RESET);
>> +	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_POWER_OFF);
>>
>>  	msleep(intel_dsi->panel_off_delay);
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dsi.h b/drivers/gpu/drm/i915/intel_dsi.h
>> index d567823..5486491 100644
>> --- a/drivers/gpu/drm/i915/intel_dsi.h
>> +++ b/drivers/gpu/drm/i915/intel_dsi.h
>> @@ -132,6 +132,9 @@ static inline struct intel_dsi *enc_to_intel_dsi(struct drm_encoder *encoder)
>>
>>  void wait_for_dsi_fifo_empty(struct intel_dsi *intel_dsi, enum port port);
>>
>> +void intel_dsi_exec_vbt_sequence(struct intel_dsi *intel_dsi,
>> +				 enum mipi_seq seq_id);
>> +
>>  bool intel_dsi_pll_is_enabled(struct drm_i915_private *dev_priv);
>>  int intel_compute_dsi_pll(struct intel_encoder *encoder,
>>  			  struct intel_crtc_state *config);
>> diff --git a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
>> index b5a02c6..f71f913 100644
>> --- a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
>> +++ b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
>> @@ -400,10 +400,9 @@ static const char *sequence_name(enum mipi_seq seq_id)
>>  		return "(unknown)";
>>  }
>>
>> -static void generic_exec_sequence(struct drm_panel *panel, enum mipi_seq seq_id)
>> +void intel_dsi_exec_vbt_sequence(struct intel_dsi *intel_dsi,
>> +				 enum mipi_seq seq_id)
>>  {
>> -	struct vbt_panel *vbt_panel = to_vbt_panel(panel);
>> -	struct intel_dsi *intel_dsi = vbt_panel->intel_dsi;
>>  	struct drm_i915_private *dev_priv = to_i915(intel_dsi->base.base.dev);
>>  	const u8 *data;
>>  	fn_mipi_elem_exec mipi_elem_exec;
>> @@ -467,40 +466,6 @@ static void generic_exec_sequence(struct drm_panel *panel, enum mipi_seq seq_id)
>>  	}
>>  }
>>
>> -static int vbt_panel_prepare(struct drm_panel *panel)
>> -{
>> -	generic_exec_sequence(panel, MIPI_SEQ_ASSERT_RESET);
>> -	generic_exec_sequence(panel, MIPI_SEQ_POWER_ON);
>> -	generic_exec_sequence(panel, MIPI_SEQ_DEASSERT_RESET);
>> -	generic_exec_sequence(panel, MIPI_SEQ_INIT_OTP);
>> -
>> -	return 0;
>> -}
>> -
>> -static int vbt_panel_unprepare(struct drm_panel *panel)
>> -{
>> -	generic_exec_sequence(panel, MIPI_SEQ_ASSERT_RESET);
>> -	generic_exec_sequence(panel, MIPI_SEQ_POWER_OFF);
>> -
>> -	return 0;
>> -}
>> -
>> -static int vbt_panel_enable(struct drm_panel *panel)
>> -{
>> -	generic_exec_sequence(panel, MIPI_SEQ_DISPLAY_ON);
>> -	generic_exec_sequence(panel, MIPI_SEQ_BACKLIGHT_ON);
>> -
>> -	return 0;
>> -}
>> -
>> -static int vbt_panel_disable(struct drm_panel *panel)
>> -{
>> -	generic_exec_sequence(panel, MIPI_SEQ_BACKLIGHT_OFF);
>> -	generic_exec_sequence(panel, MIPI_SEQ_DISPLAY_OFF);
>> -
>> -	return 0;
>> -}
>> -
>>  static int vbt_panel_get_modes(struct drm_panel *panel)
>>  {
>>  	struct vbt_panel *vbt_panel = to_vbt_panel(panel);
>> @@ -524,10 +489,6 @@ static int vbt_panel_get_modes(struct drm_panel *panel)
>>  }
>>
>>  static const struct drm_panel_funcs vbt_panel_funcs = {
>> -	.disable = vbt_panel_disable,
>> -	.unprepare = vbt_panel_unprepare,
>> -	.prepare = vbt_panel_prepare,
>> -	.enable = vbt_panel_enable,
>>  	.get_modes = vbt_panel_get_modes,
>>  };
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 09/18] drm/i915/dsi: Make intel_dsi_enable/disable directly exec VBT sequences

[Jani Nikula]

On Fri, 02 Dec 2016, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi,
>
> On 02-12-16 11:31, Jani Nikula wrote:
>> On Thu, 01 Dec 2016, Hans de Goede <hdegoede@redhat.com> wrote:
>>> The drm_panel_enable/disable and drm_panel_prepare/unprepare calls are
>>> not fine grained enough to abstract all the different steps we need to
>>> take (and VBT sequences we need to exec) properly. So simply remove the
>>> panel _enable/disable and prepare/unprepare callbacks and instead
>>> export intel_dsi_exec_vbt_sequence() from intel_dsi_panel_vbt.c
>>> and call that from intel_dsi_enable/disable().
>>>
>>> No functional changes.
>>
>> No functional changes, but this is quite a big design change between
>> intel_dsi.c and intel_dsi_panel_vbt.c.
>
> True, I did this because adding a callback per init step to the
> drm_panel interface felt like it will result in a game of
> whack a mole (adding more and more callbacks). See the end of
> this mail for a proposal with leaves the drm_panel interface
> in place, while also avoiding this problem.
>
>> Originally the idea was to
>> separate the dsi core code and the panel specific code, letting one
>> write a panel driver that was not based on values in VBT. This patch
>> changes that.
>
> I see, I was not aware of this history.
>
>> Now I'm not sure if there'll ever be a panel driver not based on VBT,
>> and perhaps the current drm_panel based interface between two is not
>> enough in the end, nor prettiest. But after this patch, we might just as
>> well throw out the drm_panel interface, and refactor the division
>> between the two files completely. Indeed, if we accept the direction set
>> in this patch, that refactoring would be the logical next step.
>
> Is intel_dsi.c the only user of the drm_panel interface ? The name
> suggests it is not intel specific.

Just a quick note here, it's not i915 specific, it's used in other
drivers.

BR,
Jani.

>
>> I have not made up my mind about this yet. An alternative would be to
>> amend the drm_panel interface to achieve the kind of granularity you're
>> after in the follow-up patches. Indeed, such patches have been sent in
>> the past.
>
> How about we add a "drm_panel_exec_sequence" callback to the
> drm_panel interface, which takes an enum argument which (de)init
> step to do ?
>
> Then we can easily add extra init steps later by extending the
> enum, and panel implementations which do not care about certain
> steps can simply treat these as nops. This avoids the need
> to add a ton of callbacks to the drm_panel struct.
>
> If there are no other users, we could then also remove the
> enable/disable and prepare/unprepare pairs from drm_panel
> now, I left those in place assuming that intel_dsi.c was
> not the only user of drm_panel.
>
> Regards,
>
> Hans
>
>
>
>
>>
>> BR,
>> Jani.
>>
>>>
>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>> ---
>>>  drivers/gpu/drm/i915/intel_dsi.c           | 14 +++++++---
>>>  drivers/gpu/drm/i915/intel_dsi.h           |  3 +++
>>>  drivers/gpu/drm/i915/intel_dsi_panel_vbt.c | 43 ++----------------------------
>>>  3 files changed, 15 insertions(+), 45 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
>>> index 85b748d..cf761e8 100644
>>> --- a/drivers/gpu/drm/i915/intel_dsi.c
>>> +++ b/drivers/gpu/drm/i915/intel_dsi.c
>>> @@ -656,7 +656,10 @@ static void intel_dsi_pre_enable(struct intel_encoder *encoder,
>>>  	/* put device in ready state */
>>>  	intel_dsi_device_ready(encoder);
>>>
>>> -	drm_panel_prepare(intel_dsi->panel);
>>> +	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_ASSERT_RESET);
>>> +	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_POWER_ON);
>>> +	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_DEASSERT_RESET);
>>> +	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_INIT_OTP);
>>>
>>>  	/* Enable port in pre-enable phase itself because as per hw team
>>>  	 * recommendation, port should be enabled befor plane & pipe */
>>> @@ -669,7 +672,8 @@ static void intel_dsi_pre_enable(struct intel_encoder *encoder,
>>>  			dpi_send_cmd(intel_dsi, TURN_ON, false, port);
>>>  		msleep(100);
>>>
>>> -		drm_panel_enable(intel_dsi->panel);
>>> +		intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_DISPLAY_ON);
>>> +		intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_BACKLIGHT_ON);
>>>
>>>  		intel_dsi_port_enable(encoder);
>>>  	}
>>> @@ -732,7 +736,8 @@ static void intel_dsi_post_disable(struct intel_encoder *encoder,
>>>  	 * if disable packets are sent before sending shutdown packet then in
>>>  	 * some next enable sequence send turn on packet error is observed
>>>  	 */
>>> -	drm_panel_disable(intel_dsi->panel);
>>> +	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_BACKLIGHT_OFF);
>>> +	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_DISPLAY_OFF);
>>>
>>>  	intel_dsi_clear_device_ready(encoder);
>>>
>>> @@ -746,7 +751,8 @@ static void intel_dsi_post_disable(struct intel_encoder *encoder,
>>>  		I915_WRITE(DSPCLK_GATE_D, val);
>>>  	}
>>>
>>> -	drm_panel_unprepare(intel_dsi->panel);
>>> +	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_ASSERT_RESET);
>>> +	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_POWER_OFF);
>>>
>>>  	msleep(intel_dsi->panel_off_delay);
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_dsi.h b/drivers/gpu/drm/i915/intel_dsi.h
>>> index d567823..5486491 100644
>>> --- a/drivers/gpu/drm/i915/intel_dsi.h
>>> +++ b/drivers/gpu/drm/i915/intel_dsi.h
>>> @@ -132,6 +132,9 @@ static inline struct intel_dsi *enc_to_intel_dsi(struct drm_encoder *encoder)
>>>
>>>  void wait_for_dsi_fifo_empty(struct intel_dsi *intel_dsi, enum port port);
>>>
>>> +void intel_dsi_exec_vbt_sequence(struct intel_dsi *intel_dsi,
>>> +				 enum mipi_seq seq_id);
>>> +
>>>  bool intel_dsi_pll_is_enabled(struct drm_i915_private *dev_priv);
>>>  int intel_compute_dsi_pll(struct intel_encoder *encoder,
>>>  			  struct intel_crtc_state *config);
>>> diff --git a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
>>> index b5a02c6..f71f913 100644
>>> --- a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
>>> +++ b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
>>> @@ -400,10 +400,9 @@ static const char *sequence_name(enum mipi_seq seq_id)
>>>  		return "(unknown)";
>>>  }
>>>
>>> -static void generic_exec_sequence(struct drm_panel *panel, enum mipi_seq seq_id)
>>> +void intel_dsi_exec_vbt_sequence(struct intel_dsi *intel_dsi,
>>> +				 enum mipi_seq seq_id)
>>>  {
>>> -	struct vbt_panel *vbt_panel = to_vbt_panel(panel);
>>> -	struct intel_dsi *intel_dsi = vbt_panel->intel_dsi;
>>>  	struct drm_i915_private *dev_priv = to_i915(intel_dsi->base.base.dev);
>>>  	const u8 *data;
>>>  	fn_mipi_elem_exec mipi_elem_exec;
>>> @@ -467,40 +466,6 @@ static void generic_exec_sequence(struct drm_panel *panel, enum mipi_seq seq_id)
>>>  	}
>>>  }
>>>
>>> -static int vbt_panel_prepare(struct drm_panel *panel)
>>> -{
>>> -	generic_exec_sequence(panel, MIPI_SEQ_ASSERT_RESET);
>>> -	generic_exec_sequence(panel, MIPI_SEQ_POWER_ON);
>>> -	generic_exec_sequence(panel, MIPI_SEQ_DEASSERT_RESET);
>>> -	generic_exec_sequence(panel, MIPI_SEQ_INIT_OTP);
>>> -
>>> -	return 0;
>>> -}
>>> -
>>> -static int vbt_panel_unprepare(struct drm_panel *panel)
>>> -{
>>> -	generic_exec_sequence(panel, MIPI_SEQ_ASSERT_RESET);
>>> -	generic_exec_sequence(panel, MIPI_SEQ_POWER_OFF);
>>> -
>>> -	return 0;
>>> -}
>>> -
>>> -static int vbt_panel_enable(struct drm_panel *panel)
>>> -{
>>> -	generic_exec_sequence(panel, MIPI_SEQ_DISPLAY_ON);
>>> -	generic_exec_sequence(panel, MIPI_SEQ_BACKLIGHT_ON);
>>> -
>>> -	return 0;
>>> -}
>>> -
>>> -static int vbt_panel_disable(struct drm_panel *panel)
>>> -{
>>> -	generic_exec_sequence(panel, MIPI_SEQ_BACKLIGHT_OFF);
>>> -	generic_exec_sequence(panel, MIPI_SEQ_DISPLAY_OFF);
>>> -
>>> -	return 0;
>>> -}
>>> -
>>>  static int vbt_panel_get_modes(struct drm_panel *panel)
>>>  {
>>>  	struct vbt_panel *vbt_panel = to_vbt_panel(panel);
>>> @@ -524,10 +489,6 @@ static int vbt_panel_get_modes(struct drm_panel *panel)
>>>  }
>>>
>>>  static const struct drm_panel_funcs vbt_panel_funcs = {
>>> -	.disable = vbt_panel_disable,
>>> -	.unprepare = vbt_panel_unprepare,
>>> -	.prepare = vbt_panel_prepare,
>>> -	.enable = vbt_panel_enable,
>>>  	.get_modes = vbt_panel_get_modes,
>>>  };
>>

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 10/18] drm/i915/dsi: Drop bogus MIPI_SEQ_ASSERT_RESET before POWER_ON

[Hans de Goede]

MIPI_SEQ_ASSERT_RESET before POWER_ON is not necessary for 2 reasons:
1) The reset should already be asserted before intel_dsi_pre_enable()
   gets called
2) Most (some?) VBTs will ensure reset was asserted in their
   MIPI_SEQ_DEASSERT_RESET themselves

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/gpu/drm/i915/intel_dsi.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
index cf761e8..1f16211 100644
--- a/drivers/gpu/drm/i915/intel_dsi.c
+++ b/drivers/gpu/drm/i915/intel_dsi.c
@@ -656,7 +656,6 @@ static void intel_dsi_pre_enable(struct intel_encoder *encoder,
 	/* put device in ready state */
 	intel_dsi_device_ready(encoder);
 
-	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_ASSERT_RESET);
 	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_POWER_ON);
 	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_DEASSERT_RESET);
 	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_INIT_OTP);
-- 
2.9.3

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

[PATCH 11/18] drm/i915/dsi: Move MIPI_SEQ_POWER_ON/OFF calls together with pmic gpio calls

[Hans de Goede]

Now that we are no longer bound to the drm_panel_ callbacks, call
MIPI_SEQ_POWER_ON/OFF at the proper place.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/gpu/drm/i915/intel_dsi.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
index 1f16211..8927c7a 100644
--- a/drivers/gpu/drm/i915/intel_dsi.c
+++ b/drivers/gpu/drm/i915/intel_dsi.c
@@ -638,10 +638,10 @@ static void intel_dsi_pre_enable(struct intel_encoder *encoder,
 
 	intel_dsi_prepare(encoder, pipe_config);
 
-	/* Panel Enable over CRC PMIC */
+	/* Power on, try both CRC pmic gpio and VBT */
 	if (intel_dsi->gpio_panel)
 		gpiod_set_value_cansleep(intel_dsi->gpio_panel, 1);
-
+	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_POWER_ON);
 	msleep(intel_dsi->panel_on_delay);
 
 	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
@@ -656,7 +656,6 @@ static void intel_dsi_pre_enable(struct intel_encoder *encoder,
 	/* put device in ready state */
 	intel_dsi_device_ready(encoder);
 
-	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_POWER_ON);
 	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_DEASSERT_RESET);
 	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_INIT_OTP);
 
@@ -751,11 +750,10 @@ static void intel_dsi_post_disable(struct intel_encoder *encoder,
 	}
 
 	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_ASSERT_RESET);
-	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_POWER_OFF);
 
+	/* Power off, try both CRC pmic gpio and VBT */
 	msleep(intel_dsi->panel_off_delay);
-
-	/* Panel Disable over CRC PMIC */
+	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_POWER_OFF);
 	if (intel_dsi->gpio_panel)
 		gpiod_set_value_cansleep(intel_dsi->gpio_panel, 0);
 
-- 
2.9.3

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

[PATCH 12/18] drm/i915/dsi: Group DPOunit clock gate workaround with PLL enable

[Hans de Goede]

Move the DPOunit clock gate workaround to directly after the PLL enable.

The exact location of the workaround does not matter and there are 2
reasons to group it with the PLL enable:

1) This moves it out of the middle of the init sequence from the spec,
   making it easier to follow the init sequence / compare it to the spec

2) It is grouped with the pll disable call in intel_dsi_post_disable,
   so for consistency it should be grouped with the pll enable in
   intel_dsi_pre_enable

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/gpu/drm/i915/intel_dsi.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
index 8927c7a..9e3cf54 100644
--- a/drivers/gpu/drm/i915/intel_dsi.c
+++ b/drivers/gpu/drm/i915/intel_dsi.c
@@ -636,14 +636,6 @@ static void intel_dsi_pre_enable(struct intel_encoder *encoder,
 	intel_disable_dsi_pll(encoder);
 	intel_enable_dsi_pll(encoder, pipe_config);
 
-	intel_dsi_prepare(encoder, pipe_config);
-
-	/* Power on, try both CRC pmic gpio and VBT */
-	if (intel_dsi->gpio_panel)
-		gpiod_set_value_cansleep(intel_dsi->gpio_panel, 1);
-	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_POWER_ON);
-	msleep(intel_dsi->panel_on_delay);
-
 	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
 		u32 val;
 
@@ -653,6 +645,14 @@ static void intel_dsi_pre_enable(struct intel_encoder *encoder,
 		I915_WRITE(DSPCLK_GATE_D, val);
 	}
 
+	intel_dsi_prepare(encoder, pipe_config);
+
+	/* Power on, try both CRC pmic gpio and VBT */
+	if (intel_dsi->gpio_panel)
+		gpiod_set_value_cansleep(intel_dsi->gpio_panel, 1);
+	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_POWER_ON);
+	msleep(intel_dsi->panel_on_delay);
+
 	/* put device in ready state */
 	intel_dsi_device_ready(encoder);
 
-- 
2.9.3

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

[PATCH 13/18] drm/i915/dsi: Execute MIPI_SEQ_DEASSERT_RESET before calling device_ready()

[Hans de Goede]

Execute MIPI_SEQ_DEASSERT_RESET before putting the device in ready
state (LP-11), this is the sequence in which things should be done
according to the spec.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/gpu/drm/i915/intel_dsi.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
index 9e3cf54..03a7a7c 100644
--- a/drivers/gpu/drm/i915/intel_dsi.c
+++ b/drivers/gpu/drm/i915/intel_dsi.c
@@ -653,10 +653,13 @@ static void intel_dsi_pre_enable(struct intel_encoder *encoder,
 	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_POWER_ON);
 	msleep(intel_dsi->panel_on_delay);
 
-	/* put device in ready state */
+	/* Deassert reset */
+	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_DEASSERT_RESET);
+
+	/* Put device in ready state (LP-11) */
 	intel_dsi_device_ready(encoder);
 
-	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_DEASSERT_RESET);
+	/* Send initialization commands in LP mode */
 	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_INIT_OTP);
 
 	/* Enable port in pre-enable phase itself because as per hw team
@@ -737,6 +740,7 @@ static void intel_dsi_post_disable(struct intel_encoder *encoder,
 	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_BACKLIGHT_OFF);
 	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_DISPLAY_OFF);
 
+	/* Transition to LP-00 */
 	intel_dsi_clear_device_ready(encoder);
 
 	intel_disable_dsi_pll(encoder);
@@ -749,6 +753,7 @@ static void intel_dsi_post_disable(struct intel_encoder *encoder,
 		I915_WRITE(DSPCLK_GATE_D, val);
 	}
 
+	/* Assert reset */
 	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_ASSERT_RESET);
 
 	/* Power off, try both CRC pmic gpio and VBT */
-- 
2.9.3

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

[PATCH 14/18] drm/i915/dsi: Group MIPI_SEQ_BACKLIGHT_ON/OFF with panel_[en|dis]able_backlight

[Hans de Goede]

Execute the MIPI_SEQ_BACKLIGHT_ON/OFF VBT sequences at the same time as
we call intel_panel_enable_backlight() / intel_panel_disable_backlight().

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/gpu/drm/i915/intel_dsi.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
index 03a7a7c..9b0c9152 100644
--- a/drivers/gpu/drm/i915/intel_dsi.c
+++ b/drivers/gpu/drm/i915/intel_dsi.c
@@ -674,12 +674,13 @@ static void intel_dsi_pre_enable(struct intel_encoder *encoder,
 		msleep(100);
 
 		intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_DISPLAY_ON);
-		intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_BACKLIGHT_ON);
 
 		intel_dsi_port_enable(encoder);
 	}
 
+	/* Enable backlight, both pwm and VBT */
 	intel_panel_enable_backlight(intel_dsi->attached_connector);
+	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_BACKLIGHT_ON);
 }
 
 static void intel_dsi_enable_nop(struct intel_encoder *encoder,
@@ -703,6 +704,8 @@ static void intel_dsi_pre_disable(struct intel_encoder *encoder,
 
 	DRM_DEBUG_KMS("\n");
 
+	/* Disable backlight, both VBT and pwm */
+	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_BACKLIGHT_OFF);
 	intel_panel_disable_backlight(intel_dsi->attached_connector);
 
 	if (is_vid_mode(intel_dsi)) {
@@ -737,7 +740,6 @@ static void intel_dsi_post_disable(struct intel_encoder *encoder,
 	 * if disable packets are sent before sending shutdown packet then in
 	 * some next enable sequence send turn on packet error is observed
 	 */
-	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_BACKLIGHT_OFF);
 	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_DISPLAY_OFF);
 
 	/* Transition to LP-00 */
-- 
2.9.3

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

[PATCH 15/18] drm/i915/dsi: Document always using v3 SHUTDOWN / MIPI_SEQ_DISPLAY_OFF order

[Hans de Goede]

According to the spec for v2 VBTs we should call MIPI_SEQ_DISPLAY_OFF
before sending SHUTDOWN, where as for v3 VBTs we should send SHUTDOWN
first.

Since the v2 order has known issues, we use the v3 order everywhere,
add a comment documenting this.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/gpu/drm/i915/intel_dsi.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
index 9b0c9152..7761806 100644
--- a/drivers/gpu/drm/i915/intel_dsi.c
+++ b/drivers/gpu/drm/i915/intel_dsi.c
@@ -708,6 +708,11 @@ static void intel_dsi_pre_disable(struct intel_encoder *encoder,
 	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_BACKLIGHT_OFF);
 	intel_panel_disable_backlight(intel_dsi->attached_connector);
 
+	/*
+	 * XXX: According to the spec we should send SHUTDOWN before
+	 * MIPI_SEQ_DISPLAY_OFF only for v3+ VBTs, but testing in the field
+	 * has shown that we should do this for v2 VBTs too?
+	 */
 	if (is_vid_mode(intel_dsi)) {
 		/* Send Shutdown command to the panel in LP mode */
 		for_each_dsi_port(port, intel_dsi->ports)
@@ -739,6 +744,8 @@ static void intel_dsi_post_disable(struct intel_encoder *encoder,
 	/*
 	 * if disable packets are sent before sending shutdown packet then in
 	 * some next enable sequence send turn on packet error is observed
+	 * XXX spec specifies SHUTDOWN before MIPI_SEQ_DISPLAY_OFF for
+	 * v3 VBTs, but not for v2 VBTs?
 	 */
 	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_DISPLAY_OFF);
 
-- 
2.9.3

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

[PATCH 16/18] drm/i915/dsi: Execute MIPI_SEQ_TEAR_OFF from intel_dsi_post_disable

[Hans de Goede]

For v3 VBTs we should call MIPI_SEQ_TEAR_OFF before
MIPI_SEQ_DISPLAY_OFF, for non v3 VBTs this is a nop.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/gpu/drm/i915/intel_dsi.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
index 7761806..e3ecb5a 100644
--- a/drivers/gpu/drm/i915/intel_dsi.c
+++ b/drivers/gpu/drm/i915/intel_dsi.c
@@ -747,6 +747,7 @@ static void intel_dsi_post_disable(struct intel_encoder *encoder,
 	 * XXX spec specifies SHUTDOWN before MIPI_SEQ_DISPLAY_OFF for
 	 * v3 VBTs, but not for v2 VBTs?
 	 */
+	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_TEAR_OFF);
 	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_DISPLAY_OFF);
 
 	/* Transition to LP-00 */
-- 
2.9.3

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

[PATCH 17/18] drm/i915/dsi: Call MIPI_SEQ_TEAR_ON and DISPLAY_ON for cmd-mode (untested)

[Hans de Goede]

According to the spec we should call MIPI_SEQ_TEAR_ON and DISPLAY_ON
on enable for cmd-mode, just like we already call their counterparts
on disable. Note: untested, my panel is a vid-mode panel.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/gpu/drm/i915/intel_dsi.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
index e3ecb5a..bf956a3 100644
--- a/drivers/gpu/drm/i915/intel_dsi.c
+++ b/drivers/gpu/drm/i915/intel_dsi.c
@@ -667,6 +667,8 @@ static void intel_dsi_pre_enable(struct intel_encoder *encoder,
 	if (is_cmd_mode(intel_dsi)) {
 		for_each_dsi_port(port, intel_dsi->ports)
 			I915_WRITE(MIPI_MAX_RETURN_PKT_SIZE(port), 8 * 4);
+		intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_TEAR_ON);
+		intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_DISPLAY_ON);
 	} else {
 		msleep(20); /* XXX */
 		for_each_dsi_port(port, intel_dsi->ports)
-- 
2.9.3

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

[PATCH 18/18] drm/i915/dsi: Skip delays for v3 VBTs in vid-mode

[Hans de Goede]

For v3 VBTs in vid-mode the delays are part of the VBT sequences, so
we should not also delay ourselves otherwise we get double delays.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/gpu/drm/i915/intel_dsi.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
index bf956a3..b60612a 100644
--- a/drivers/gpu/drm/i915/intel_dsi.c
+++ b/drivers/gpu/drm/i915/intel_dsi.c
@@ -555,6 +555,17 @@ static void intel_dsi_prepare(struct intel_encoder *intel_encoder,
 			      struct intel_crtc_state *pipe_config);
 static void intel_dsi_unprepare(struct intel_encoder *encoder);
 
+static void intel_dsi_msleep(struct intel_dsi *intel_dsi, int msec)
+{
+	struct drm_i915_private *dev_priv = to_i915(intel_dsi->base.base.dev);
+
+	/* For v3 VBTs in vid-mode the delays are part of the VBT sequences */
+	if (is_vid_mode(intel_dsi) && dev_priv->vbt.dsi.seq_version >= 3)
+		return;
+
+	msleep(msec);
+}
+
 /*
  * Panel enable/disable sequences from the spec:
  *
@@ -651,7 +662,7 @@ static void intel_dsi_pre_enable(struct intel_encoder *encoder,
 	if (intel_dsi->gpio_panel)
 		gpiod_set_value_cansleep(intel_dsi->gpio_panel, 1);
 	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_POWER_ON);
-	msleep(intel_dsi->panel_on_delay);
+	intel_dsi_msleep(intel_dsi, intel_dsi->panel_on_delay);
 
 	/* Deassert reset */
 	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_DEASSERT_RESET);
@@ -673,7 +684,7 @@ static void intel_dsi_pre_enable(struct intel_encoder *encoder,
 		msleep(20); /* XXX */
 		for_each_dsi_port(port, intel_dsi->ports)
 			dpi_send_cmd(intel_dsi, TURN_ON, false, port);
-		msleep(100);
+		intel_dsi_msleep(intel_dsi, 100);
 
 		intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_DISPLAY_ON);
 
@@ -769,7 +780,7 @@ static void intel_dsi_post_disable(struct intel_encoder *encoder,
 	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_ASSERT_RESET);
 
 	/* Power off, try both CRC pmic gpio and VBT */
-	msleep(intel_dsi->panel_off_delay);
+	intel_dsi_msleep(intel_dsi, intel_dsi->panel_off_delay);
 	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_POWER_OFF);
 	if (intel_dsi->gpio_panel)
 		gpiod_set_value_cansleep(intel_dsi->gpio_panel, 0);
@@ -778,7 +789,7 @@ static void intel_dsi_post_disable(struct intel_encoder *encoder,
 	 * FIXME As we do with eDP, just make a note of the time here
 	 * and perform the wait before the next panel power on.
 	 */
-	msleep(intel_dsi->panel_pwr_cycle_delay);
+	intel_dsi_msleep(intel_dsi, intel_dsi->panel_pwr_cycle_delay);
 }
 
 static bool intel_dsi_get_hw_state(struct intel_encoder *encoder,
-- 
2.9.3

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