Re: [PATCH 42/65] OMAPDSS: DSI: call mgr_enable/disable for cmd mode displays

[Tomi Valkeinen <tomi.valkeinen@ti.com>]

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

On Wed, 2011-11-23 at 15:40 +0530, Archit Taneja wrote:
> On Tuesday 22 November 2011 02:51 PM, Tomi Valkeinen wrote:
> > The current code uses dsi_video_mode_enable/disable functions to
> > enable/disable DISPC output for video mode displays. For command mode
> > displays we have no notion in the DISPC side of whether the panel is
> > enabled, except when a dss_start_update() call is made.
> >
> > However, to properly maintain the DISPC state in apply.c, we need to
> > know if a manager used for a manual update display is currently in use.
> >
> > This patch achieves that by changing dsi_video_mode_enable/disable to
> > dsi_enable/disable_video_output, which is called by both video and
> > command mode displays. For video mode displays it starts the actual
> > pixel stream, as it did before. For command mode displays it doesn't do
> > anything else than mark that the manager is currently in use.
> 
> dsi_video_mode_enable() doesn't only enable the DISPC output, it also 
> sends the long packet header to start video mode transfer.
> 
> I think it would be better if we had 2 separate functions, one which 
> starts/stops DSI video mode, and the other which enables/disables the 
> DISPC video port.
> 
> This way, a manual update panel would need to call only 
> dsi_enable/disable_video_output(which just enables or disables the 
> manager), whereas a video mode panel will need to call both.
> 
> This is just a suggestion though. It's probably okay to have both in the 
> same function too. We might have to separate them out later if we were 
> planning to standardise mipi dsi across SoCs.

If you think from the panel driver's point of view, it doesn't know
about DISPC. It just wants to enable the video stream (on video mode
displays).

If we had two functions, could only the first be used? I.e. is it
possible to just enable the video mode transfer, without enabling DISPC?
If not, I'm not sure what would be the use for two separate functions.
And even if it can, I'm not sure what use it would be to enable only the
video mode output without the actual pixel data from DISPC.

It is true that the function in thsi patch is not the best one. For
command mode display it's more about reserving the ovl manager for use
than actually enabling it. Then again, how I thought the function's
purpose was that it enables the DSI video output, but because for
command mode there's need to trigger the actual frame transfer later,
the function doesn't start the pixel feed for command mode displays. It
just "prepares" the output.

But even if we had the functions separated, the function called by video
mode and command mode displays would be different, as for the former one
it enables the pixel stream, and for latter one it just reserves the
output.

So, I'm fine with changing the function, but the reasoning for what
functions we have and what they do should come from the panel driver's
perspective, not because of OMAP DSS's HW details.

 Tomi


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]