* [PATCH] drm/bridge/synopsys: dw-hdmi: re-run dw_hdmi_setup when setting mode @ 2018-07-25 3:56 ` Icenowy Zheng 2018-08-13 11:49 ` Andrzej Hajda 0 siblings, 1 reply; 4+ messages in thread From: Icenowy Zheng @ 2018-07-25 3:56 UTC (permalink / raw) To: Archit Taneja, Andrzej Hajda, Laurent Pinchart, Neil Armstrong, Maxime Ripard, Jernej Skrabec, Daniel Vetter Cc: dri-devel, linux-kernel, Icenowy Zheng 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. Re-run dw_hdmi_setup when setting mode, in order to prevent such situation. Signed-off-by: Icenowy Zheng <icenowy@aosc.io> --- 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); mutex_unlock(&hdmi->mutex); } -- 2.18.0 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] drm/bridge/synopsys: dw-hdmi: re-run dw_hdmi_setup when setting mode 2018-07-25 3:56 ` [PATCH] drm/bridge/synopsys: dw-hdmi: re-run dw_hdmi_setup when setting mode Icenowy Zheng @ 2018-08-13 11:49 ` Andrzej Hajda 2018-08-14 5:28 ` Icenowy Zheng 0 siblings, 1 reply; 4+ messages in thread From: Andrzej Hajda @ 2018-08-13 11:49 UTC (permalink / raw) To: Icenowy Zheng, Archit Taneja, Laurent Pinchart, Neil Armstrong, Maxime Ripard, Jernej Skrabec, Daniel Vetter Cc: dri-devel, linux-kernel 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. > > 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. > > Signed-off-by: Icenowy Zheng <icenowy@aosc.io> > --- > 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. Regards Andrzej > > mutex_unlock(&hdmi->mutex); > } ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] drm/bridge/synopsys: dw-hdmi: re-run dw_hdmi_setup when setting mode 2018-08-13 11:49 ` Andrzej Hajda @ 2018-08-14 5:28 ` Icenowy Zheng 2018-08-14 9:18 ` Andrzej Hajda 0 siblings, 1 reply; 4+ messages in thread From: Icenowy Zheng @ 2018-08-14 5:28 UTC (permalink / raw) To: Andrzej Hajda, Archit Taneja, Laurent Pinchart, Neil Armstrong, Maxime Ripard, Jernej Skrabec, Daniel Vetter Cc: dri-devel, linux-kernel 在 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. 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. > > > > > > Signed-off-by: Icenowy Zheng <icenowy@aosc.io> > > --- > > 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? > > Regards > Andrzej > > > > mutex_unlock(&hdmi->mutex); > > } > > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] drm/bridge/synopsys: dw-hdmi: re-run dw_hdmi_setup when setting mode 2018-08-14 5:28 ` Icenowy Zheng @ 2018-08-14 9:18 ` Andrzej Hajda 0 siblings, 0 replies; 4+ messages in thread From: Andrzej Hajda @ 2018-08-14 9:18 UTC (permalink / raw) To: Icenowy Zheng, Archit Taneja, Laurent Pinchart, Neil Armstrong, Maxime Ripard, Jernej Skrabec, Daniel Vetter Cc: dri-devel, linux-kernel 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@aosc.io> >>> --- >>> 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); >>> } >> > > ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2018-08-14 9:18 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <CGME20180725035712epcas1p25df72b8af38438b7bb66a2729fdff445@epcas1p2.samsung.com> 2018-07-25 3:56 ` [PATCH] drm/bridge/synopsys: dw-hdmi: re-run dw_hdmi_setup when setting mode Icenowy Zheng 2018-08-13 11:49 ` Andrzej Hajda 2018-08-14 5:28 ` Icenowy Zheng 2018-08-14 9:18 ` Andrzej Hajda
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).