Re: [PATCH v2 09/13] OMAPDSS: SDI: Create a function to set timings

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

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

On Wed, 2012-08-15 at 00:38 +0530, Archit Taneja wrote:
> On Tuesday 14 August 2012 11:03 PM, Tomi Valkeinen wrote:
> > On Tue, 2012-08-14 at 22:26 +0530, Archit Taneja wrote:
> >> On Tuesday 14 August 2012 07:14 PM, Tomi Valkeinen wrote:
> >
> >> I guess it depends on how drm/fb want to use it. I guess an output
> >> should have a set_timings() kind of op if it can do it seamlessly. I
> >> guess we can do that easily in DPI, for example, we could reduce the fps
> >> from 60 to 30 without causing an artefacts(I think). For outputs which
> >
> > Yes, that kind of thing is easy to do by just changing the pck divider,
> > which is in a shadow register. I mean, "easy" in theory, at least. Our
> > clock calculation doesn't work like that currently, though, so it could
> > end up changing DSS fck.
> >
> >> can't do it, we could remove the set_timings totally.
> >
> > But we do need set_timings for other outputs also (like sdi). We just
> > can't change them just like that. Only outputs that do not need timings
> > are DSI command mode and rfbi.
> >
> >> However, it'll be kind of inconsistent for some outputs to set timings,
> >> and for others to not, and if in the future drm/fb gets exposed to ops
> >> too, we may have dirty checks to see if set_timings is populated or not.
> >>
> >> The easiest way would be to make all set_timings just update the copy of
> >> the timings output has, and expect drm/fb to disable and re enable the
> >> panel. We may end up doing unnecessary gpio resets and configuration of
> >> the panels though.
> >
> > I think changing things like timings is quite a rare operation. The only
> > case it'd be necessary to change timings often, with speed, and without
> > artifacts would be the fps drop you mentioned, for lower power use with
> > panels that don't mind the fps drop.
> >
> > If I understood correctly, Rob said that drm already disables the output
> > when changing the mode, when I asked if it's ok for the apply's
> > set_timings to require the output to be off.
> >
> > In any case this is not a big issue, I mean, it's not causing any
> > problems. Somebody is going to disable the output anyway when changing
> > the timings. Perhaps even these patches are good, because they make the
> > set_timings consistent across the output drivers (don't they?).
> 
> Yes, they do, there isn't a set_timings for RFBI though, only a 
> set_size, and DSI has set_timings for video mode and a set_size for 
> command mode, I haven't put checks in the ops for a panel driver to 
> wrongly call set_timings in commmand mode, and set_size in video mode.

Ok. Well, perhaps we should go forward with these patches then. They
make things consistent, and we don't really know which would be the best
way to handle this, so the method in your patches is as good as some
other.

The dssdev->state needs to be removed at some point, but that can be a
separate task.

> I haven't done that yet because a future patch of mine will have a DSI 
> specific op called set_operation_mode() to make us independent of 
> dssdev->panel.dsi_mode, there is no guarantee that the panel driver to 
> first call set_operation_mode(), and then set_timings(), so I'm not sure 
> yet how to deal with that. Probably having a mode/state which says that 
> a field is unintialized might help, but that would overcomplicate things.

Well, we can define that set_operation_mode needs to be called before
any other dsi functions. They are kernel drivers, we can presume they
act correctly (although we should of course try to handle error cases
anyway). If they don't, we need to fix them.

If you want to be extra safe there, you could add third value to the
dsi_mode enum: UNDEFINED or such (I guess this is what you meant also).
By default dsi's mode would be undefined, and functions could check it.
But I agree it'd add lots of checks all around.

 Tomi


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