Re: [PATCH] drm/panel: auo novatek 1080p video mode panel

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3058 bytes --]

On Fri, Aug 07, 2015 at 12:11:31PM -0400, Rob Clark wrote:
> On Fri, Aug 7, 2015 at 9:19 AM, Thierry Reding <thierry.reding@gmail.com> wrote:
> > On Tue, Jul 21, 2015 at 03:36:02PM -0400, Rob Clark wrote:
[...]
> >> +static int auo_panel_on(struct auo_panel *auo)
> >> +{
> >> +     struct mipi_dsi_device *dsi = auo->dsi;
> >> +     int ret;
> >> +
> >> +     dsi->mode_flags |= MIPI_DSI_MODE_LPM;
> >
> > This is weird.
> >
> >> +     ret = mipi_dsi_dcs_set_display_on(dsi);
> >> +     if (ret < 0)
> >> +             return ret;
> >> +
> >> +     msleep(40);
> >> +
> >> +     return 0;
> >> +}
> >> +
> >> +static int auo_panel_off(struct auo_panel *auo)
> >> +{
> >> +     struct mipi_dsi_device *dsi = auo->dsi;
> >> +     int ret;
> >> +
> >> +     dsi->mode_flags &= ~MIPI_DSI_MODE_LPM;
> >
> > And this even more. Doesn't the panel work when you simply send
> > everything in low-power mode?
> 
> I wouldn't expect low power mode to have enough bandwidth for the
> video signal.. but otoh it seems like I need to use lpm for
> power-on/off sequence.  Maybe we should wrap that up in a helper to
> enable/disable lpm?  But that seemed a bit overkill.

I think there's a misunderstanding, which arguable might stem from a
lack of documentation. The intention for MIPI_DSI_MODE_LPM was to be
used in conjunction with "host-driven" command mode.

Perhaps I should elaborate on the vocabulary here: Tegra supports two
types of command mode: "host-driven" and "DC-driven". Host driven
command mode is used to perform panel setup (using DCS and vendor-
specific commands). "DC-driven" command mode is used to update the
framebuffer using write_memory_start and write_memory_continue DCS
commands directly generated by the DSI controller.

In the latter case you'd obviously want to run in high-speed mode to
achieve the throughput necessary to drive you panel at the requested
resolution and framerate. In the former your device should be able to
receive command in both high speed and low power modes. However some
hardware is known not to work with high speed command transmission.
MIPI_DSI_MODE_LPM is targetted at these cases, so that display drivers
know not to attempt high-speed transmission of initial command packets.

Note how MIPI_DSI_MODE_LPM translates to MIPI_DSI_MSG_USE_LPM when
transferring messages (see mipi_dsi_device_transfer()). Looking at the
comment for the MIPI_DSI_MODE_LPM definition I realize that it isn't
very precise, but I have trouble coming up with anything better.
Perhaps:

	/* transmit command messages (non-video data) in low power mode */
	#define MIPI_DSI_MODE_LPM		BIT(11)

Any ideas?

On a semi-related note, some of the other flags are rather badly
documented. I do see that both Exynos and MSM implement most of these
(specifically the MIPI_DSI_MODE_VIDEO_H* ones), perhaps the comments
for all of those should be revisited. Ideally they'd be annotated with a
reference to the spec, like we do for MIPI_DSI_CLOCK_NON_CONTINUOUS.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]