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-m1stabl...
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
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",
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
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
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
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); }
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));}
-- 2.9.3
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;
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);
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) {
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;
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;
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)
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)
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)
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, };
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,
};
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,
};
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,
};
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);
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);
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);
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 */
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 */
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);
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 */
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)
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,
dri-devel@lists.freedesktop.org
-
Hans de Goede -
Jani Nikula -
Ville Syrjälä