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@xxxxxxxxx> > Reported-by: Jan-Michael Brummer <jan.brummer@xxxxxxxxx> > Tested-by: Hans de Goede <hdegoede@xxxxxxxxxx> > Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx> > --- > 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 -- Ville Syrjälä Intel OTC _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
