| Message ID | 20170220140845.1714-8-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, 20 Feb 2017 15:08:37 +0100 Hans de Goede <hdegoede@redhat.com> wrote: > 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> Using words like "should" and some? don't inspire a lot of confidence that this is the right thing to do. Is there some way to verify that these two conditions are true? > --- > 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 78d5884..4ebf308 100644 > --- a/drivers/gpu/drm/i915/intel_dsi.c > +++ b/drivers/gpu/drm/i915/intel_dsi.c > @@ -669,7 +669,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);
Hi, On 24-02-17 18:00, Bob Paauwe wrote: > On Mon, 20 Feb 2017 15:08:37 +0100 > Hans de Goede <hdegoede@redhat.com> wrote: > >> 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> > > Using words like "should" and some? don't inspire a lot of confidence > that this is the right thing to do. Is there some way to verify that > these two conditions are true? Well the i915 code should never call intel_dsi_[pre_]enable without first having called intel_dsi_post_disable() which does: intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_ASSERT_RESET); Already, also this is undoing a recent (4.9 kernel IIRC) change, before that the i915 did not call the MIPI_SEQ_ASSERT_RESET sequence from the enable path. Also doing this is against the VBT spec. Regards, Hans > >> --- >> 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 78d5884..4ebf308 100644 >> --- a/drivers/gpu/drm/i915/intel_dsi.c >> +++ b/drivers/gpu/drm/i915/intel_dsi.c >> @@ -669,7 +669,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); > > >
On Sat, 25 Feb 2017 11:35:03 +0100 Hans de Goede <hdegoede@redhat.com> wrote: > Hi, > > On 24-02-17 18:00, Bob Paauwe wrote: > > On Mon, 20 Feb 2017 15:08:37 +0100 > > Hans de Goede <hdegoede@redhat.com> wrote: > > > >> 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> > > > > Using words like "should" and some? don't inspire a lot of confidence > > that this is the right thing to do. Is there some way to verify that > > these two conditions are true? > > Well the i915 code should never call intel_dsi_[pre_]enable without > first having called intel_dsi_post_disable() which does: > > intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_ASSERT_RESET); > > Already, also this is undoing a recent (4.9 kernel IIRC) change, > before that the i915 did not call the MIPI_SEQ_ASSERT_RESET > sequence from the enable path. Also doing this is against the VBT > spec. > > Regards, > > Hans > So let's try to make the commit message state that. Using what you just wrote maybe something like: intel_dsi_post_disable(), which does the MIPI_SEQ_ASSERT_RESET, will always be called at some point before intel_dsi_pre_enable() making the MIPI_SEQ_ASSERT_RESET in intel_dsi_pre_enable() redundent. In addition, calling MIPI_SEQ_ASSERT_RESET in the enable path goes against the VBT spec. With the commit message updated the change looks to be correct. > > > > > >> --- > >> 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 78d5884..4ebf308 100644 > >> --- a/drivers/gpu/drm/i915/intel_dsi.c > >> +++ b/drivers/gpu/drm/i915/intel_dsi.c > >> @@ -669,7 +669,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); > > > > > >
diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c index 78d5884..4ebf308 100644 --- a/drivers/gpu/drm/i915/intel_dsi.c +++ b/drivers/gpu/drm/i915/intel_dsi.c @@ -669,7 +669,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);
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(-)