| Message ID | 20170220140845.1714-6-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, 20 Feb 2017 15:08:35 +0100 Hans de Goede <hdegoede@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@redhat.com> > Acked-by: Jani Nikula <jani.nikula@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? > + * - MIPIBacklightOn What's the difference between MIPIBacklightOn here and "backlight on" in the v2 sequence? 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. > + * ... > + * - 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? > + * - 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)
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@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@redhat.com> >> Acked-by: Jani Nikula <jani.nikula@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. > 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. >> + * ... >> + * - 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
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@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@redhat.com> >>> Acked-by: Jani Nikula <jani.nikula@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
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) + * - MIPIBacklightOn + * ... + * - MIPIBacklightOff + * - turn off DPI + * - MIPITearOff + MIPIDisplayOff (LP) + * - clk/data lines to lp-00 + * - MIPIAssertResetPin + * - MIPIPanelPowerOff + * + * sequence for command mode: + * - 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)