[Intel-gfx] [PATCH resend 05/15] drm/i915/dsi: Document the panel enable / disable sequences from the spec

[Hans de Goede]

Hi,

On 25-02-17 11:31, Hans de Goede wrote:
> Hi Bob,
>
> Thank you for the review of this patch-set.
>
> On 24-02-17 18:00, Bob Paauwe wrote:
>> On Mon, 20 Feb 2017 15:08:35 +0100
>> Hans de Goede <hdegoede at redhat.com> wrote:
>>
>>> Document the DSI panel enable / disable sequences from the spec,
>>> for easy comparison between the code and the spec.
>>>
>>> Signed-off-by: Hans de Goede <hdegoede at redhat.com>
>>> Acked-by: Jani Nikula <jani.nikula at intel.com>
>>> ---
>>>  drivers/gpu/drm/i915/intel_dsi.c | 68 ++++++++++++++++++++++++++++++++++++++++
>>>  1 file changed, 68 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
>>> index 072f99b..8808f87 100644
>>> --- a/drivers/gpu/drm/i915/intel_dsi.c
>>> +++ b/drivers/gpu/drm/i915/intel_dsi.c
>>> @@ -552,6 +552,74 @@ static void intel_dsi_prepare(struct intel_encoder *intel_encoder,
>>>                    struct intel_crtc_state *pipe_config);
>>>  static void intel_dsi_unprepare(struct intel_encoder *encoder);
>>>
>>> +/*
>>> + * Panel enable/disable sequences from the VBT spec.
>>> + *
>>> + * Note the spec has AssertReset / DeassertReset swapped from their
>>> + * usual naming. We use the normal names to avoid confusion (so below
>>> + * they are swapped compared to the spec).
>>> + *
>>> + * v2 sequence for video mode:
>>> + * - power on
>>> + * - wait t1+t2
>>> + * - MIPIDeassertResetPin
>>> + * - clk/data lines to lp-11
>>> + * - MIPISendInitialDcsCmds
>>> + * - turn on DPI
>>> + * - MIPIDisplayOn
>>> + * - wait t5
>>> + * - backlight on
>>> + * ...
>>> + * - backlight off
>>> + * - wait t6
>>> + * - MIPIDisplayOff
>>> + * - turn off DPI
>>> + * - clk/data lines to lp-00
>>> + * - MIPIAssertResetPin
>>> + * - wait t3
>>> + * - power off
>>> + * - wait t4
>>> + *
>>> + * v3 sequence for video mode:
>>> + * - MIPIPanelPowerOn
>>> + * - MIPIDeassertResetPin
>>> + * - set clk/data lines to lp-11
>>> + * - MIPISendInitialDcsCmds (LP)
>>> + * - turn on DPI
>>> + * - MIPITearOn (command mode only) + MIPIDisplayOn (LP and HS)
>>
>> What does (command mode only) mean in the video mode sequence?  My
>> assumption is that TearOn is only used in command mode, and as such,
>> shouldn't be done in video mode.  Is that wrong?
>
> I don't know I don't have access to the VBT spec I merely copied
> the contents of the comment this is adding from a mail which contained
> the sequences extracted from the doc by someone at Intel (Jani IIRC).
>
>>
>>> + * - MIPIBacklightOn
>>
>> What's the difference between MIPIBacklightOn here and "backlight on"
>> in the v2 sequence?
>
> AFAICT there is none, again copy pasted from the mail as is.

Wrong, after working on this a bit again I remember now what the difference
is, in the v2 sequence power-on and backlight on are left to the driver,
which is e.g. why the code has:

         if (intel_dsi->gpio_panel)
                 gpiod_set_value_cansleep(intel_dsi->gpio_panel, 1);

Which is really necessary for v2 VBTs only, where as for v3 we've
this call:

         intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_POWER_ON);

Which is a nop for v2 VBTs since they don't define that sequence.

So for v2 I've added the following extra paragraph to the comment:

  * Steps starting with MIPI refer to VBT sequences, note that for v2
  * VBTs several steps which have a VBT in v2 are expected to be handled
  * directly by the driver, by directly driving gpios for example.


>> It might be cleaner to make this a table with V2 and V3 as columns,
>> that would also help to highlight the differences in the two versions.
>> Maybe even put the command mode sequence in a third column if it
>> doesn't make the lines too long.
>
> I can try once we've cleared up the other bits.

Done, using a table with columns is a good idea and I've done so
for v2.

Regards,

Hans


>
>>> + * ...
>>> + * - MIPIBacklightOff
>>> + * - turn off DPI
>>> + * - MIPITearOff + MIPIDisplayOff (LP)
>>> + * - clk/data lines to lp-00
>>> + * - MIPIAssertResetPin
>>> + * - MIPIPanelPowerOff
>>> + *
>>> + * sequence for command mode:
>>
>> Is this the sequence for both V2 and V3 or only V3?
>
> I think command mode is V3 only, but I'm not sure. I only have
> access to video mode devices myself.
>
>>> + * - power on
>>> + * - wait t1+t2
>>> + * - MIPIDeassertResetPin
>>> + * - clk/data lines to lp-11
>>> + * - MIPISendInitialDcsCmds
>>> + * - MIPITearOn
>>> + * - MIPIDisplayOn
>>> + * - set pipe to dsr mode
>>> + * - wait t5
>>> + * - backlight on
>>> + * ... issue write_mem_start/write_mem_continue commands ...
>>> + * - backlight off
>>> + * - wait t6
>>> + * - disable pipe dsr mode
>>> + * - MIPITearOff
>>> + * - MIPIDisplayOff
>>> + * - clk/data lines to lp-00
>>> + * - MIPIAssertResetPin
>>> + * - wait t3
>>> + * - power off
>>> + * - wait t4
>>> + */
>>> +
>>>  static void intel_dsi_pre_enable(struct intel_encoder *encoder,
>>>                   struct intel_crtc_state *pipe_config,
>>>                   struct drm_connector_state *conn_state)
>>
>>
>>
>
> Regards,
>
> Hans