Re: [PATCH] drm/bridge/synopsys: dw-hdmi: re-run dw_hdmi_setup when setting mode

[Andrzej Hajda]

On 14.08.2018 07:28, Icenowy Zheng wrote:
> å 2018-08-13äç 13:49 +0200ïAndrzej Hajdaåéï
>> On 25.07.2018 05:56, Icenowy Zheng wrote:
>>> Currently dw_hdmi_setup is only run when the dw-hdmi bridge is
>>> enabled,
>>> with the mode set last time.
>>>
>>> When the bridge is enabled before any mode is set (this may happen
>>> when
>>> booting), the mode won't be set at all, some setup steps will be
>>> skipped or fail, and the HDMI output may not work.
>> I guess, it should not happen. Could you show the stack-trace.
> Mysteriously dw_hdmi_setup isn't called at all currently when booting
> in thie situation. I added dump_stack() at the head of the function,
> and it's only triggered when I re-plug the monitor.

I think more informative would be stack trace from bridge enable, which
happens before any modeset, this is something suspicious.

Anyway if dw_hdmi_setup is not called at boot it clearly shows there is
issue with power-on logic in dw-hdmi.c, not with mode_set.
Quick look at dw_hdmi_update_power shows it is not straightforward, and
could be buggy.

>
> In this case I got:
> ```
> [   46.891513] CPU: 0 PID: 73 Comm: irq/34-1ee0000. Not tainted 4.18.0+
> #152
> [   46.898290] Hardware name: Allwinner sun8i Family
> [   46.903008] [<c010efac>] (unwind_backtrace) from [<c010bfd0>]
> (show_stack+0x10/0x14)
> [   46.910746] [<c010bfd0>] (show_stack) from [<c06f59c4>]
> (dump_stack+0x88/0x9c)
> [   46.917966] [<c06f59c4>] (dump_stack) from [<c04444d8>]
> (dw_hdmi_update_power+0xb8/0x12cc)
> [   46.926225] [<c04444d8>] (dw_hdmi_update_power) from [<c044593c>]
> (dw_hdmi_bridge_enable+0x2c/0x70)
> [   46.935262] [<c044593c>] (dw_hdmi_bridge_enable) from [<c04261f0>]
> (drm_bridge_enable+0x24/0x34)
> [   46.944040] [<c04261f0>] (drm_bridge_enable) from [<c0404fd0>]
> (drm_atomic_helper_commit_modeset_enables+0x9c/0x180)
> [   46.954552] [<c0404fd0>] (drm_atomic_helper_commit_modeset_enables)
> from [<c040879c>] (drm_atomic_helper_commit_tail_rpm+0x24/0x64)
> [   46.966361] [<c040879c>] (drm_atomic_helper_commit_tail_rpm) from
> [<c040861c>] (commit_tail+0x40/0x6c)
> [   46.975657] [<c040861c>] (commit_tail) from [<c040870c>]
> (drm_atomic_helper_commit+0xbc/0x128)
> [   46.984259] [<c040870c>] (drm_atomic_helper_commit) from
> [<c040ac70>] (restore_fbdev_mode_atomic+0x1cc/0x220)
> [   46.994164] [<c040ac70>] (restore_fbdev_mode_atomic) from
> [<c040df90>] (drm_fb_helper_restore_fbdev_mode_unlocked+0x54/0xa0)
> [   47.005369] [<c040df90>] (drm_fb_helper_restore_fbdev_mode_unlocked)
> from [<c040e00c>] (drm_fb_helper_set_par+0x30/0x54)
> [   47.016226] [<c040e00c>] (drm_fb_helper_set_par) from [<c040df08>]
> (drm_fb_helper_hotplug_event.part.11+0xa0/0xa8)
> [   47.026563] [<c040df08>] (drm_fb_helper_hotplug_event.part.11) from
> [<c03fee0c>] (drm_helper_hpd_irq_event+0x110/0x118)
> [   47.037334] [<c03fee0c>] (drm_helper_hpd_irq_event) from
> [<c0445888>] (dw_hdmi_irq+0x10c/0x194)
> [   47.046025] [<c0445888>] (dw_hdmi_irq) from [<c01626a8>]
> (irq_thread_fn+0x1c/0x54)
> [   47.053589] [<c01626a8>] (irq_thread_fn) from [<c01629c4>]
> (irq_thread+0x158/0x21c)
> [   47.061241] [<c01629c4>] (irq_thread) from [<c013a324>]
> (kthread+0x148/0x150)
> [   47.068373] [<c013a324>] (kthread) from [<c01010e8>]
> (ret_from_fork+0x14/0x2c)
> [   47.075584] Exception stack(0xee8bbfb0 to 0xee8bbff8)
> [   47.080630] bfa0:                                     00000000
> 00000000 00000000 00000000
> [   47.088797] bfc0: 00000000 00000000 00000000 00000000 00000000
> 00000000 00000000 00000000
> [   47.096964] bfe0: 00000000 00000000 00000000 00000000 00000013
> 00000000
> ```
>
>>> Re-run dw_hdmi_setup when setting mode, in order to prevent such
>>> situation.
>> mode_set is run with hardware disabled, thus usually it should not
>> touch
>> hardware.
> However I think there's many instances now where some hardware setup is
> performed in mode_set.

It does not prove it is correct way :)

If you look at docs [1] for descriptions of *mode_set* callbacks for
various components (crtc,encoder,bridge), you will see it should not be
used to program HW in case of runtime_pm drivers, and since dw-hdmi is
used by multiple platforms you cannot assume it is or it will not be the
case. IMO whole mode_set callback can be removed, as it performs
unnecessary work.


>
>>
>>> Signed-off-by: Icenowy Zheng <icenowy@xxxxxxx>
>>> ---
>>>  drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 1 +
>>>  1 file changed, 1 insertion(+)
>>>
>>> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>>> b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>>> index 5971976284bf..e2f832182afe 100644
>>> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>>> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>>> @@ -2007,6 +2007,7 @@ static void dw_hdmi_bridge_mode_set(struct
>>> drm_bridge *bridge,
>>>  
>>>  	/* Store the display mode for plugin/DKMS poweron events */
>>>  	memcpy(&hdmi->previous_mode, mode, sizeof(hdmi-
>>>> previous_mode));
>>> +	dw_hdmi_setup(hdmi, mode);
>> This hdmi->previous_mode also looks strange, it is current mode and
>> moreover it is always available from crtc state, there is no point in
>> copying it to private field.
> Sorry I don't know about this. Maybe you should ask the original author
> of dw-hdmi?

Sorry for noise, it is not introduced by your patch, but just related to it.

Regards
Andrzej

>
>> Regards
>> Andrzej
>>>  
>>>  	mutex_unlock(&hdmi->mutex);
>>>  }
>>
>
>