Re: [PATCH] drm/i915: Fix DSI panels with v1 MIPI sequences without a DEASSERT sequence v2

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel