[Intel-gfx] [PATCH] drm/i915: Fix DSI panels with v1 MIPI sequences without a DEASSERT sequence v2
Hans de Goede
hdegoede at redhat.com
Sat Jan 27 14:31:31 UTC 2018
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 at tabos.org>
>> Reported-by: Jan-Michael Brummer <jan.brummer at tabos.org>
>> Tested-by: Hans de Goede <hdegoede at redhat.com>
>> Signed-off-by: Hans de Goede <hdegoede at 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
More information about the Intel-gfx
mailing list