Re: [PATCH v3 4/5] OMAPDSS: APPLY: Remove display dependency from overlay and manager checks

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

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

On Wed, 2012-05-09 at 15:23 +0530, Archit Taneja wrote:
> On Tuesday 08 May 2012 06:05 PM, Archit Taneja wrote:
> > On Tuesday 08 May 2012 05:25 PM, Tomi Valkeinen wrote:
> >> On Tue, 2012-05-08 at 16:52 +0530, Archit Taneja wrote:
> >>> On Tuesday 08 May 2012 04:20 PM, Tomi Valkeinen wrote:
> >>
> >>>> Checking the validity of all the settings is a bit tricky, but
> >>>> currently
> >>>> I think, as a rule of thumb, we should accept any settings when things
> >>>> are disabled. So, until the interface driver sets the timings before
> >>>> enabling the output, all ovl/mgr settings should be allowed. And we
> >>>
> >>> We have 2 ways to go about this, one is to have an initial set of
> >>> 'always valid' values like I have done, the other option is to ignore
> >>> manager timing related checks if the manager is disabled, i.e all
> >>> configs are okay. To implement the second option, I think our function
> >>> dss_check_settings_low() would get more complicated. We would now have
> >>> to pass mp->enabled outside of apply, and pass it to dss_mgr_check()
> >>> which would further need to pass this to dss_ovl_check(). Hence I used
> >>> the first approach.
> >>
> >> I'm not sure about that. We already do it for overlay. In ovl.c we have
> >> dss_ovl_simple_check() and dss_ovl_check(). The simple check sees if the
> >> settings pass basic sanity check. The check sees if all the ovl& mgr
> >> settings are compatible with each other.
> >>
> >> Simple check is done when setting the config, called from
> >> dss_ovl_set_info(). The proper check is done later when the actual
> >> config is about to be taken into use.
> >>
> >> If mp->enabled == false, can't we just skip dss_check_settings_low()
> >> totally for that manager? We skip the check for ovls the same way.
> >
> > Okay, this would work better I guess. If someone tries to enable the
> > manager without setting the timings, that check should fail, and in my
> > approach, it would have passed, and would have led to bad stuff later.
> 
> One change in behaviour with the patch series I see is with the 
> following use case when connected to Pandaboad's DVI interface:
> 
> - Bootup with 1080p resolution
> - Try to set 640x480 timings with sysfs.
> 
> The older behaviour was that the timings were taken, and a skewed image 
> was seen(because the overlay size is larger in deimension)
> 
> The behaviour after this series is that the mgr_set_timings fails with 
> the error:
> 
> omapdss OVERLAY error: overlay 0 horizontally not inside the display 
> area (0 + 1920 >= 640)
> 
> I guess this is better in a way, a user of DSS is supposed to reallocate 
> the framebuffer with the desired size and then change the timings.

Yes, I think it's better. I think it's undefined what happens if the
overlay is larger than the output. In theory the DSS HW could lock-up,
or something. So we have to prevent it.

omapdss could change the size of the overlay automatically, but I
dislike things like that. Low level drivers adjusting the configuration
from the user almost always brings problems. So yes, I also think that
the user of DSS is supposed to handle it correctly.

> But with DVI, the problem is that dpi_set_timings already does a ton of 
> stuff before dss_mgr_set_timings() is called, so we change the dividers 
> to get the new clock, and then realise that the overlay can't fit 
> inside, and we are left nowhere. I guess this is a separate problem and 
> we could tackle it later.

Ah, ok. Yes, I guess we need to roll back the settings in that case. Or,
I wonder, would a check function work, that would take the new timings
and check those but also check if they match the overlays.

Hmm, no, that doesn't solve it totally, as the overlay could be changed
after the check. Again a problem with dss locking, we can't do the
operation atomically...

 Tomi


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