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