| Message ID | 20180126075207.311-1-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Jan 26, 2018 at 08:52:07AM +0100, Hans de Goede wrote: > So far models of the Dell Venue 8 Pro, with a panel with MIPI panel > index = 3, one of which has been kindly provided to me by Jan Brummer, > where not working with the i915 driver, giving a black screen on the > first modeset. > > The problem with at least these Dells is that their VBT defines a MIPI > ASSERT sequence, but not a DEASSERT sequence. Instead they DEASSERT the > reset in their INIT_OTP sequence, but the deassert must be done before > calling intel_dsi_device_ready(), so that is too late. > > Simply doing the INIT_OTP sequence earlier is not enough to fix this, > because the INIT_OTP sequence also sends various MIPI packets to the > panel, which can only happen after calling intel_dsi_device_ready(). > > This commit fixes this by splitting the INIT_OTP sequence into everything > before the first DSI packet and everything else, including the first DSI > packet. The first part (everything before the first DSI packet) is then > used as deassert sequence. > > Changed in v2: > -Split the init OTP sequence into a deassert reset and the actual init > OTP sequence, instead of calling it earlier and then having the first > mipi_exec_send_packet() call call intel_dsi_device_ready(). > > BugLink: https://bugs.freedesktop.org/show_bug.cgi?id=82880 > Related: https://bugs.freedesktop.org/show_bug.cgi?id=101205 > Cc: Jan-Michael Brummer <jan.brummer@tabos.org> > Reported-by: Jan-Michael Brummer <jan.brummer@tabos.org> > Tested-by: Hans de Goede <hdegoede@redhat.com> > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > drivers/gpu/drm/i915/intel_dsi.c | 1 + > drivers/gpu/drm/i915/intel_dsi.h | 2 + > drivers/gpu/drm/i915/intel_dsi_vbt.c | 82 ++++++++++++++++++++++++++++++++++++ > 3 files changed, 85 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c > index f67d321376e4..b59ef34d25f6 100644 > --- a/drivers/gpu/drm/i915/intel_dsi.c > +++ b/drivers/gpu/drm/i915/intel_dsi.c > @@ -1642,6 +1642,7 @@ static void intel_dsi_encoder_destroy(struct drm_encoder *encoder) > if (intel_dsi->gpio_panel) > gpiod_put(intel_dsi->gpio_panel); > > + kfree(intel_dsi->deassert_seq); > intel_encoder_destroy(encoder); > } > > diff --git a/drivers/gpu/drm/i915/intel_dsi.h b/drivers/gpu/drm/i915/intel_dsi.h > index 7afeb9580f41..5895588144ad 100644 > --- a/drivers/gpu/drm/i915/intel_dsi.h > +++ b/drivers/gpu/drm/i915/intel_dsi.h > @@ -46,6 +46,8 @@ struct intel_dsi { > > struct intel_connector *attached_connector; > > + u8 *deassert_seq; > + Should this perhaps live next to the other DSI VBT stuff? I think that might make more sense. And should probably also move the intel_dsi_fixup_dsi_sequences() call to parse_mipi_sequence() as well since we're actually modifying dev_priv->vbt.data. Doing that from the encoder init doesn't really feel right to me. Jani, any thoughts? > /* bit mask of ports being driven */ > u16 ports; > > diff --git a/drivers/gpu/drm/i915/intel_dsi_vbt.c b/drivers/gpu/drm/i915/intel_dsi_vbt.c > index 91c07b0c8db9..84664f79cbef 100644 > --- a/drivers/gpu/drm/i915/intel_dsi_vbt.c > +++ b/drivers/gpu/drm/i915/intel_dsi_vbt.c > @@ -499,6 +499,86 @@ int intel_dsi_vbt_get_modes(struct intel_dsi *intel_dsi) > return 1; > } > > +/* > + * Get len of pre-fixed deassert from init OTP, skip all delay + gpio operands > + * and stop at the first DSI packet op. > + */ > +static int intel_vbi_get_deassert_len(const u8 *data, int total) > +{ > + int index, len; if (WARN_ON(seq_version != 1)) return 0; or something might be nice here to document the requirements and to deter anyone from using this with other seq versions. > + > + /* index = 1 to skip sequence byte */ > + for (index = 1; index < total; index += len) { > + switch (data[index]) { > + case MIPI_SEQ_ELEM_SEND_PKT: > + return index; What if this is the first element? Hmm. It does seem to work out in the end. We do end up with an empty deassert sequence, but I guess hat's fine. > + case MIPI_SEQ_ELEM_DELAY: > + len = 5; /* 1 byte for operand + uint32 */ > + break; > + case MIPI_SEQ_ELEM_GPIO: > + len = 3; /* 1 byte for op, 1 for gpio_nr, 1 for value */ > + break; > + default: > + return 0; > + } > + } > + > + return 0; > +} > + > +/* > + * Some v1 VBT MIPI sequences do the deassert in the init OTP sequence. > + * The deassert must be done before calling intel_dsi_device_ready, so for > + * these devices we split the init OTP sequence into a deassert sequence and > + * the actual init OTP part. > + */ > +static void intel_dsi_fixup_dsi_sequences(struct intel_dsi *intel_dsi) > +{ > + struct drm_i915_private *dev_priv = to_i915(intel_dsi->base.base.dev); > + int init_otp_index, len; > + u8 *init_otp; > + > + /* Limit this to VLV for now. */ > + if (!IS_VALLEYVIEW(dev_priv)) > + return; Not sure v1 sequences even exist on other platforms. But no harm in having this check anyway. > + > + /* Limit this to v1 vid-mode sequences */ > + if (intel_dsi->operation_mode != INTEL_DSI_VIDEO_MODE || > + dev_priv->vbt.dsi.seq_version != 1) > + return; > + > + /* Only do this if there are otp and assert seqs and no deassert seq */ > + if (!dev_priv->vbt.dsi.sequence[MIPI_SEQ_INIT_OTP] || > + !dev_priv->vbt.dsi.sequence[MIPI_SEQ_ASSERT_RESET] || > + dev_priv->vbt.dsi.sequence[MIPI_SEQ_DEASSERT_RESET]) > + return; > + > + /* The deassert-sequence ends at the first DSI packet */ > + init_otp_index = dev_priv->vbt.dsi.sequence[MIPI_SEQ_INIT_OTP] - > + (const u8 *)dev_priv->vbt.dsi.data; Why the cast? > + init_otp = dev_priv->vbt.dsi.data + init_otp_index; > + len = dev_priv->vbt.dsi.size - init_otp_index; > + len = intel_vbi_get_deassert_len(init_otp, len); > + if (!len) > + return; > + > + DRM_DEBUG_KMS("Using init OTP fragment to deassert reset\n"); > + > + /* Copy the fragment, update seq byte and terminate it */ > + intel_dsi->deassert_seq = kmemdup(init_otp, len + 1, GFP_KERNEL); > + if (!intel_dsi->deassert_seq) > + return; > + intel_dsi->deassert_seq[0] = MIPI_SEQ_DEASSERT_RESET; > + intel_dsi->deassert_seq[len] = MIPI_SEQ_ELEM_END; > + /* Use the copy for deassert */ > + dev_priv->vbt.dsi.sequence[MIPI_SEQ_DEASSERT_RESET] = > + intel_dsi->deassert_seq; > + /* Replace the last byte of the fragment with init OTP seq byte */ > + init_otp[len - 1] = MIPI_SEQ_INIT_OTP; > + /* And make MIPI_MIPI_SEQ_INIT_OTP point to it */ > + dev_priv->vbt.dsi.sequence[MIPI_SEQ_INIT_OTP] = init_otp + len - 1; > +} > + > bool intel_dsi_vbt_init(struct intel_dsi *intel_dsi, u16 panel_id) > { > struct drm_device *dev = intel_dsi->base.base.dev; > @@ -794,5 +874,7 @@ bool intel_dsi_vbt_init(struct intel_dsi *intel_dsi, u16 panel_id) > mipi_dsi_attach(intel_dsi->dsi_hosts[port]->device); > } > > + intel_dsi_fixup_dsi_sequences(intel_dsi); > + > return true; > } > -- > 2.14.3
Hi, On 26-01-18 17:38, Ville Syrjälä wrote: > On Fri, Jan 26, 2018 at 08:52:07AM +0100, Hans de Goede wrote: >> So far models of the Dell Venue 8 Pro, with a panel with MIPI panel >> index = 3, one of which has been kindly provided to me by Jan Brummer, >> where not working with the i915 driver, giving a black screen on the >> first modeset. >> >> The problem with at least these Dells is that their VBT defines a MIPI >> ASSERT sequence, but not a DEASSERT sequence. Instead they DEASSERT the >> reset in their INIT_OTP sequence, but the deassert must be done before >> calling intel_dsi_device_ready(), so that is too late. >> >> Simply doing the INIT_OTP sequence earlier is not enough to fix this, >> because the INIT_OTP sequence also sends various MIPI packets to the >> panel, which can only happen after calling intel_dsi_device_ready(). >> >> This commit fixes this by splitting the INIT_OTP sequence into everything >> before the first DSI packet and everything else, including the first DSI >> packet. The first part (everything before the first DSI packet) is then >> used as deassert sequence. >> >> Changed in v2: >> -Split the init OTP sequence into a deassert reset and the actual init >> OTP sequence, instead of calling it earlier and then having the first >> mipi_exec_send_packet() call call intel_dsi_device_ready(). >> >> BugLink: https://bugs.freedesktop.org/show_bug.cgi?id=82880 >> Related: https://bugs.freedesktop.org/show_bug.cgi?id=101205 >> Cc: Jan-Michael Brummer <jan.brummer@tabos.org> >> Reported-by: Jan-Michael Brummer <jan.brummer@tabos.org> >> Tested-by: Hans de Goede <hdegoede@redhat.com> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> drivers/gpu/drm/i915/intel_dsi.c | 1 + >> drivers/gpu/drm/i915/intel_dsi.h | 2 + >> drivers/gpu/drm/i915/intel_dsi_vbt.c | 82 ++++++++++++++++++++++++++++++++++++ >> 3 files changed, 85 insertions(+) >> >> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c >> index f67d321376e4..b59ef34d25f6 100644 >> --- a/drivers/gpu/drm/i915/intel_dsi.c >> +++ b/drivers/gpu/drm/i915/intel_dsi.c >> @@ -1642,6 +1642,7 @@ static void intel_dsi_encoder_destroy(struct drm_encoder *encoder) >> if (intel_dsi->gpio_panel) >> gpiod_put(intel_dsi->gpio_panel); >> >> + kfree(intel_dsi->deassert_seq); >> intel_encoder_destroy(encoder); >> } >> >> diff --git a/drivers/gpu/drm/i915/intel_dsi.h b/drivers/gpu/drm/i915/intel_dsi.h >> index 7afeb9580f41..5895588144ad 100644 >> --- a/drivers/gpu/drm/i915/intel_dsi.h >> +++ b/drivers/gpu/drm/i915/intel_dsi.h >> @@ -46,6 +46,8 @@ struct intel_dsi { >> >> struct intel_connector *attached_connector; >> >> + u8 *deassert_seq; >> + > > Should this perhaps live next to the other DSI VBT stuff? I think that > might make more sense. And should probably also move the > intel_dsi_fixup_dsi_sequences() call to parse_mipi_sequence() as well > since we're actually modifying dev_priv->vbt.data. Doing that from the > encoder init doesn't really feel right to me. Sure works for me, shall I submit a v3 with this changed, or do you want to wait a bit for Jani's input on this? > Jani, any thoughts? > >> /* bit mask of ports being driven */ >> u16 ports; >> >> diff --git a/drivers/gpu/drm/i915/intel_dsi_vbt.c b/drivers/gpu/drm/i915/intel_dsi_vbt.c >> index 91c07b0c8db9..84664f79cbef 100644 >> --- a/drivers/gpu/drm/i915/intel_dsi_vbt.c >> +++ b/drivers/gpu/drm/i915/intel_dsi_vbt.c >> @@ -499,6 +499,86 @@ int intel_dsi_vbt_get_modes(struct intel_dsi *intel_dsi) >> return 1; >> } >> >> +/* >> + * Get len of pre-fixed deassert from init OTP, skip all delay + gpio operands >> + * and stop at the first DSI packet op. >> + */ >> +static int intel_vbi_get_deassert_len(const u8 *data, int total) >> +{ >> + int index, len; > > if (WARN_ON(seq_version != 1)) > return 0; > > or something might be nice here to document the requirements and > to deter anyone from using this with other seq versions. Ok, will add for v3. >> + >> + /* index = 1 to skip sequence byte */ >> + for (index = 1; index < total; index += len) { >> + switch (data[index]) { >> + case MIPI_SEQ_ELEM_SEND_PKT: >> + return index; > > What if this is the first element? Ah good one, I did not think about that. > Hmm. It does seem to work out in the end. We do end up with > an empty deassert sequence, but I guess hat's fine. Ack, lets keep it as is then. >> + case MIPI_SEQ_ELEM_DELAY: >> + len = 5; /* 1 byte for operand + uint32 */ >> + break; >> + case MIPI_SEQ_ELEM_GPIO: >> + len = 3; /* 1 byte for op, 1 for gpio_nr, 1 for value */ >> + break; >> + default: >> + return 0; >> + } >> + } >> + >> + return 0; >> +} >> + >> +/* >> + * Some v1 VBT MIPI sequences do the deassert in the init OTP sequence. >> + * The deassert must be done before calling intel_dsi_device_ready, so for >> + * these devices we split the init OTP sequence into a deassert sequence and >> + * the actual init OTP part. >> + */ >> +static void intel_dsi_fixup_dsi_sequences(struct intel_dsi *intel_dsi) >> +{ >> + struct drm_i915_private *dev_priv = to_i915(intel_dsi->base.base.dev); >> + int init_otp_index, len; >> + u8 *init_otp; >> + >> + /* Limit this to VLV for now. */ >> + if (!IS_VALLEYVIEW(dev_priv)) >> + return; > > Not sure v1 sequences even exist on other platforms. But no > harm in having this check anyway. Ack. >> + >> + /* Limit this to v1 vid-mode sequences */ >> + if (intel_dsi->operation_mode != INTEL_DSI_VIDEO_MODE || >> + dev_priv->vbt.dsi.seq_version != 1) >> + return; >> + >> + /* Only do this if there are otp and assert seqs and no deassert seq */ >> + if (!dev_priv->vbt.dsi.sequence[MIPI_SEQ_INIT_OTP] || >> + !dev_priv->vbt.dsi.sequence[MIPI_SEQ_ASSERT_RESET] || >> + dev_priv->vbt.dsi.sequence[MIPI_SEQ_DEASSERT_RESET]) >> + return; >> + >> + /* The deassert-sequence ends at the first DSI packet */ >> + init_otp_index = dev_priv->vbt.dsi.sequence[MIPI_SEQ_INIT_OTP] - >> + (const u8 *)dev_priv->vbt.dsi.data; > > Why the cast? dev_priv->vbt.dsi.sequence[MIPI_SEQ_INIT_OTP] is const u8 *, dev_priv->vbt.dsi.data u8 *, I was expecting gcc to complain without the cast, but at least current gcc does not complain. Still might be best to keep the cast to avoid future compiler warnings. >> + init_otp = dev_priv->vbt.dsi.data + init_otp_index; >> + len = dev_priv->vbt.dsi.size - init_otp_index; >> + len = intel_vbi_get_deassert_len(init_otp, len); >> + if (!len) >> + return; >> + >> + DRM_DEBUG_KMS("Using init OTP fragment to deassert reset\n"); >> + >> + /* Copy the fragment, update seq byte and terminate it */ >> + intel_dsi->deassert_seq = kmemdup(init_otp, len + 1, GFP_KERNEL); >> + if (!intel_dsi->deassert_seq) >> + return; >> + intel_dsi->deassert_seq[0] = MIPI_SEQ_DEASSERT_RESET; >> + intel_dsi->deassert_seq[len] = MIPI_SEQ_ELEM_END; >> + /* Use the copy for deassert */ >> + dev_priv->vbt.dsi.sequence[MIPI_SEQ_DEASSERT_RESET] = >> + intel_dsi->deassert_seq; >> + /* Replace the last byte of the fragment with init OTP seq byte */ >> + init_otp[len - 1] = MIPI_SEQ_INIT_OTP; >> + /* And make MIPI_MIPI_SEQ_INIT_OTP point to it */ >> + dev_priv->vbt.dsi.sequence[MIPI_SEQ_INIT_OTP] = init_otp + len - 1; >> +} >> + >> bool intel_dsi_vbt_init(struct intel_dsi *intel_dsi, u16 panel_id) >> { >> struct drm_device *dev = intel_dsi->base.base.dev; >> @@ -794,5 +874,7 @@ bool intel_dsi_vbt_init(struct intel_dsi *intel_dsi, u16 panel_id) >> mipi_dsi_attach(intel_dsi->dsi_hosts[port]->device); >> } >> >> + intel_dsi_fixup_dsi_sequences(intel_dsi); >> + >> return true; >> } >> -- >> 2.14.3 > Regards, Hans
diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c index f67d321376e4..b59ef34d25f6 100644 --- a/drivers/gpu/drm/i915/intel_dsi.c +++ b/drivers/gpu/drm/i915/intel_dsi.c @@ -1642,6 +1642,7 @@ static void intel_dsi_encoder_destroy(struct drm_encoder *encoder) if (intel_dsi->gpio_panel) gpiod_put(intel_dsi->gpio_panel); + kfree(intel_dsi->deassert_seq); intel_encoder_destroy(encoder); } diff --git a/drivers/gpu/drm/i915/intel_dsi.h b/drivers/gpu/drm/i915/intel_dsi.h index 7afeb9580f41..5895588144ad 100644 --- a/drivers/gpu/drm/i915/intel_dsi.h +++ b/drivers/gpu/drm/i915/intel_dsi.h @@ -46,6 +46,8 @@ struct intel_dsi { struct intel_connector *attached_connector; + u8 *deassert_seq; + /* bit mask of ports being driven */ u16 ports; diff --git a/drivers/gpu/drm/i915/intel_dsi_vbt.c b/drivers/gpu/drm/i915/intel_dsi_vbt.c index 91c07b0c8db9..84664f79cbef 100644 --- a/drivers/gpu/drm/i915/intel_dsi_vbt.c +++ b/drivers/gpu/drm/i915/intel_dsi_vbt.c @@ -499,6 +499,86 @@ int intel_dsi_vbt_get_modes(struct intel_dsi *intel_dsi) return 1; } +/* + * Get len of pre-fixed deassert from init OTP, skip all delay + gpio operands + * and stop at the first DSI packet op. + */ +static int intel_vbi_get_deassert_len(const u8 *data, int total) +{ + int index, len; + + /* index = 1 to skip sequence byte */ + for (index = 1; index < total; index += len) { + switch (data[index]) { + case MIPI_SEQ_ELEM_SEND_PKT: + return index; + case MIPI_SEQ_ELEM_DELAY: + len = 5; /* 1 byte for operand + uint32 */ + break; + case MIPI_SEQ_ELEM_GPIO: + len = 3; /* 1 byte for op, 1 for gpio_nr, 1 for value */ + break; + default: + return 0; + } + } + + return 0; +} + +/* + * Some v1 VBT MIPI sequences do the deassert in the init OTP sequence. + * The deassert must be done before calling intel_dsi_device_ready, so for + * these devices we split the init OTP sequence into a deassert sequence and + * the actual init OTP part. + */ +static void intel_dsi_fixup_dsi_sequences(struct intel_dsi *intel_dsi) +{ + struct drm_i915_private *dev_priv = to_i915(intel_dsi->base.base.dev); + int init_otp_index, len; + u8 *init_otp; + + /* Limit this to VLV for now. */ + if (!IS_VALLEYVIEW(dev_priv)) + return; + + /* Limit this to v1 vid-mode sequences */ + if (intel_dsi->operation_mode != INTEL_DSI_VIDEO_MODE || + dev_priv->vbt.dsi.seq_version != 1) + return; + + /* Only do this if there are otp and assert seqs and no deassert seq */ + if (!dev_priv->vbt.dsi.sequence[MIPI_SEQ_INIT_OTP] || + !dev_priv->vbt.dsi.sequence[MIPI_SEQ_ASSERT_RESET] || + dev_priv->vbt.dsi.sequence[MIPI_SEQ_DEASSERT_RESET]) + return; + + /* The deassert-sequence ends at the first DSI packet */ + init_otp_index = dev_priv->vbt.dsi.sequence[MIPI_SEQ_INIT_OTP] - + (const u8 *)dev_priv->vbt.dsi.data; + init_otp = dev_priv->vbt.dsi.data + init_otp_index; + len = dev_priv->vbt.dsi.size - init_otp_index; + len = intel_vbi_get_deassert_len(init_otp, len); + if (!len) + return; + + DRM_DEBUG_KMS("Using init OTP fragment to deassert reset\n"); + + /* Copy the fragment, update seq byte and terminate it */ + intel_dsi->deassert_seq = kmemdup(init_otp, len + 1, GFP_KERNEL); + if (!intel_dsi->deassert_seq) + return; + intel_dsi->deassert_seq[0] = MIPI_SEQ_DEASSERT_RESET; + intel_dsi->deassert_seq[len] = MIPI_SEQ_ELEM_END; + /* Use the copy for deassert */ + dev_priv->vbt.dsi.sequence[MIPI_SEQ_DEASSERT_RESET] = + intel_dsi->deassert_seq; + /* Replace the last byte of the fragment with init OTP seq byte */ + init_otp[len - 1] = MIPI_SEQ_INIT_OTP; + /* And make MIPI_MIPI_SEQ_INIT_OTP point to it */ + dev_priv->vbt.dsi.sequence[MIPI_SEQ_INIT_OTP] = init_otp + len - 1; +} + bool intel_dsi_vbt_init(struct intel_dsi *intel_dsi, u16 panel_id) { struct drm_device *dev = intel_dsi->base.base.dev; @@ -794,5 +874,7 @@ bool intel_dsi_vbt_init(struct intel_dsi *intel_dsi, u16 panel_id) mipi_dsi_attach(intel_dsi->dsi_hosts[port]->device); } + intel_dsi_fixup_dsi_sequences(intel_dsi); + return true; }