Re: [PATCH] OMAPDSS: Check if RPM enabled before trying to change state

[Andy Green <andy.green@linaro.org>]

On 06/26/12 16:32, the mail apparently from Jassi Brar included:
> On 26 June 2012 12:49, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>> On Mon, 2012-06-25 at 22:36 +0530, Jassi Brar wrote:
>
>>>>
>>>> Normally if the driver does dispc_runtime_get() and dispc_read_reg(),
>>>> the first call will enable the HW so the reg read works.
>>>>
>>>> But if the pm_runtime is disabled, say, during system suspend, with your
>>>> patch dispc_runtime_get() will just return 0 without doing anything, and
>>>> the dispc_read_reg() will crash because the HW is disabled (because
>>>> nobody enabled it).
>>>>
>>> Hmm, I am not sure if new calls would/should be made to dispc.c after
>>> the system has suspended and before resumed. That is, anything other
>>> than from runtime_resume/suspend callbacks of DSS, DISPC, HDMI, VENC
>>> and RFBI, which rightly don't touch any dss reg but only
>>> enable/disable a clock.
>>
>> They do touch the registers. For example, dispc's callbacks save and
>> restore the registers. The HW should be fully functional during the
>> callbacks. The point of the callbacks is to suspend/resume the HW in
>> question, which of course requires accessing the HW.
>>
> DISPC being held by HDMI, VENC and RFBI would be the last to suspend
> and first to resume. And it won't have its registers touched between
> dispc_runtime_suspend() and dispc_runtime_resume(), which seems ok to
> me (?)
> HDMI, VENC and RFBI directly fooling around with DISPC regs would have
> been a problem, which isn't the case.
>
>>> As we know, a subsystem should make sure any active work is cleared
>>> out before suspending and set some flag so that nothing runs until it
>>> has resumed. I don't say we can't crash the system with this patch,
>>> but then we would be violating rules of suspend-resume.
>>
>> Let's go back a bit. I feel like I'm missing some pieces of information,
>> as I still don't quite grasp the problem.
>>
>> In the patch you said this fixes an issue with HDMI. Can you tell more
>> about the problem? What code path is being run? Any error messages?
>>
>> I tried system suspend with omap4-sdp and panda, with 3.5-rc2, but
>> neither board seems to wake up from the suspend. Does it work for you?
>>
> Something non-omapdss in vanilla breaks suspend/resume.
> Without this patch I see the upstream's display broken after the
> suspend attempt.
>      $ echo mem > /sys/power/state
>
> I work on TILT tree, which has suspend/resume working after some more
> local patches.
>      http://git.linaro.org/gitweb?p=people/andygreen/kernel-tilt.git;a=shortlog;h=refs/heads/tilt-3.4
>
> I don't have SDP so not sure, but it should simply be testable with
> Panda4460 and the omap4plus_defconfig there.  Please feel free to ask
> if you have any issue checking that out.

We don't have access to Blaze and don't test that tree against it, but 
it's worth trying on PandaBoard ES which we do have and test against 
(omap4plus_defconfig).

Here, mem suspend is working with HDMI raster coming back on resume, but 
we don't always get a desktop redraw (suspending again can "correct" 
that).  Jassi's patches are present in this tree.

A slightly side-issue, I have a TV here that only issues hpd 700ms after 
the Panda provides 5V at the HDMI link.  It has always been touch-and-go 
if dss will recognize it or not, compared to a monitor which issues hpd 
high within some us of the link being powered.

The patches from Jassi about permanently enabling the external HDMI PHY 
chip section that performs level-conversion for hpd, and the existing 
work to use irq management of hpd, seems to have really solved detecting 
that TV for the first time.

-Andy

-- 
Andy Green | TI Landing Team Leader
Linaro.org │ Open source software for ARM SoCs | Follow Linaro
http://facebook.com/pages/Linaro/155974581091106  - 
http://twitter.com/#!/linaroorg - http://linaro.org/linaro-blog