| Message ID | 20220613144800.326124-24-maxime@cerno.tech (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/vc4: Misc fixes | expand |
On 6/13/22 07:47, Maxime Ripard wrote: > From: Dave Stevenson <dave.stevenson@raspberrypi.com> > > The BCM2835-37 found in the RaspberryPi 0 to 3 have a power domain > attached to the HDMI block, handled in Linux through runtime_pm. > > That power domain is shared with the VEC block, so even if we put our > runtime_pm reference in the HDMI driver it would keep being on. If the > VEC is disabled though, the power domain would be disabled and we would > lose any initialization done in our bind implementation. > > That initialization involves calling the reset function and initializing > the CEC registers. > > Let's move the initialization to our runtime_resume implementation so > that we initialize everything properly if we ever need to. > > Fixes: c86b41214362 ("drm/vc4: hdmi: Move the HSM clock enable to runtime_pm") > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com> > Signed-off-by: Maxime Ripard <maxime@cerno.tech> After seeing the same warning as Stefan reported in the link below, but on the Raspberry Pi 4B: https://www.spinics.net/lists/dri-devel/msg354170.html a separate bisection effort led me to this commit, before is fine, after produces 4 warnings during boot, see attached log. Is there a fix that we can try that would also cover the Raspberry Pi 4B? Is it possible that this series precipitates the problem: https://www.spinics.net/lists/arm-kernel/msg984638.html
On 8/4/22 16:11, Florian Fainelli wrote: > On 6/13/22 07:47, Maxime Ripard wrote: >> From: Dave Stevenson <dave.stevenson@raspberrypi.com> >> >> The BCM2835-37 found in the RaspberryPi 0 to 3 have a power domain >> attached to the HDMI block, handled in Linux through runtime_pm. >> >> That power domain is shared with the VEC block, so even if we put our >> runtime_pm reference in the HDMI driver it would keep being on. If the >> VEC is disabled though, the power domain would be disabled and we would >> lose any initialization done in our bind implementation. >> >> That initialization involves calling the reset function and initializing >> the CEC registers. >> >> Let's move the initialization to our runtime_resume implementation so >> that we initialize everything properly if we ever need to. >> >> Fixes: c86b41214362 ("drm/vc4: hdmi: Move the HSM clock enable to >> runtime_pm") >> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com> >> Signed-off-by: Maxime Ripard <maxime@cerno.tech> > > After seeing the same warning as Stefan reported in the link below, but > on the Raspberry Pi 4B: > > https://www.spinics.net/lists/dri-devel/msg354170.html > > a separate bisection effort led me to this commit, before is fine, after > produces 4 warnings during boot, see attached log. > > Is there a fix that we can try that would also cover the Raspberry Pi > 4B? Is it possible that this series precipitates the problem: > > https://www.spinics.net/lists/arm-kernel/msg984638.html Maxime, Dave, anything you would want me to try? Still seeing these warnings with net-next-6.0-11220-g15205c2829ca Would be nice to see those fixes before 6.0 final, thanks!
Hi Florian, Am 05.08.22 um 01:11 schrieb Florian Fainelli: > On 6/13/22 07:47, Maxime Ripard wrote: >> From: Dave Stevenson <dave.stevenson@raspberrypi.com> >> >> The BCM2835-37 found in the RaspberryPi 0 to 3 have a power domain >> attached to the HDMI block, handled in Linux through runtime_pm. >> >> That power domain is shared with the VEC block, so even if we put our >> runtime_pm reference in the HDMI driver it would keep being on. If the >> VEC is disabled though, the power domain would be disabled and we would >> lose any initialization done in our bind implementation. >> >> That initialization involves calling the reset function and initializing >> the CEC registers. >> >> Let's move the initialization to our runtime_resume implementation so >> that we initialize everything properly if we ever need to. >> >> Fixes: c86b41214362 ("drm/vc4: hdmi: Move the HSM clock enable to >> runtime_pm") >> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com> >> Signed-off-by: Maxime Ripard <maxime@cerno.tech> > > After seeing the same warning as Stefan reported in the link below, > but on the Raspberry Pi 4B: > > https://www.spinics.net/lists/dri-devel/msg354170.html > > a separate bisection effort led me to this commit, before is fine, > after produces 4 warnings during boot, see attached log. Which config do you use (multi_v7_defconfig + LPAE or arm64/defconfig)? > > Is there a fix that we can try that would also cover the Raspberry Pi > 4B? Is it possible that this series precipitates the problem: > > https://www.spinics.net/lists/arm-kernel/msg984638.html I don't think this is related because this is a different driver. Best regards
Hallo Stefan, On 8/9/22 13:16, Stefan Wahren wrote: > Hi Florian, > > Am 05.08.22 um 01:11 schrieb Florian Fainelli: >> On 6/13/22 07:47, Maxime Ripard wrote: >>> From: Dave Stevenson <dave.stevenson@raspberrypi.com> >>> >>> The BCM2835-37 found in the RaspberryPi 0 to 3 have a power domain >>> attached to the HDMI block, handled in Linux through runtime_pm. >>> >>> That power domain is shared with the VEC block, so even if we put our >>> runtime_pm reference in the HDMI driver it would keep being on. If the >>> VEC is disabled though, the power domain would be disabled and we would >>> lose any initialization done in our bind implementation. >>> >>> That initialization involves calling the reset function and initializing >>> the CEC registers. >>> >>> Let's move the initialization to our runtime_resume implementation so >>> that we initialize everything properly if we ever need to. >>> >>> Fixes: c86b41214362 ("drm/vc4: hdmi: Move the HSM clock enable to >>> runtime_pm") >>> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com> >>> Signed-off-by: Maxime Ripard <maxime@cerno.tech> >> >> After seeing the same warning as Stefan reported in the link below, >> but on the Raspberry Pi 4B: >> >> https://www.spinics.net/lists/dri-devel/msg354170.html >> >> a separate bisection effort led me to this commit, before is fine, >> after produces 4 warnings during boot, see attached log. > Which config do you use (multi_v7_defconfig + LPAE or arm64/defconfig)? This was actually bcm2835_defconfig copied over to arch/arm64/configs/ and slightly modified to enable PCIe, here is it: https://gist.github.com/481999edc11b823d0c3e87ecf1693d26 >> >> Is there a fix that we can try that would also cover the Raspberry Pi >> 4B? Is it possible that this series precipitates the problem: >> >> https://www.spinics.net/lists/arm-kernel/msg984638.html > > I don't think this is related because this is a different driver. > > Best regards >
Hi Florian On Tue, 9 Aug 2022 at 20:02, Florian Fainelli <f.fainelli@gmail.com> wrote: > > On 8/4/22 16:11, Florian Fainelli wrote: > > On 6/13/22 07:47, Maxime Ripard wrote: > >> From: Dave Stevenson <dave.stevenson@raspberrypi.com> > >> > >> The BCM2835-37 found in the RaspberryPi 0 to 3 have a power domain > >> attached to the HDMI block, handled in Linux through runtime_pm. > >> > >> That power domain is shared with the VEC block, so even if we put our > >> runtime_pm reference in the HDMI driver it would keep being on. If the > >> VEC is disabled though, the power domain would be disabled and we would > >> lose any initialization done in our bind implementation. > >> > >> That initialization involves calling the reset function and initializing > >> the CEC registers. > >> > >> Let's move the initialization to our runtime_resume implementation so > >> that we initialize everything properly if we ever need to. > >> > >> Fixes: c86b41214362 ("drm/vc4: hdmi: Move the HSM clock enable to > >> runtime_pm") > >> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com> > >> Signed-off-by: Maxime Ripard <maxime@cerno.tech> > > > > After seeing the same warning as Stefan reported in the link below, but > > on the Raspberry Pi 4B: > > > > https://www.spinics.net/lists/dri-devel/msg354170.html > > > > a separate bisection effort led me to this commit, before is fine, after > > produces 4 warnings during boot, see attached log. > > > > Is there a fix that we can try that would also cover the Raspberry Pi > > 4B? Is it possible that this series precipitates the problem: > > > > https://www.spinics.net/lists/arm-kernel/msg984638.html > > Maxime, Dave, anything you would want me to try? Still seeing these > warnings with net-next-6.0-11220-g15205c2829ca Strange as we don't see this warning on the vendor kernel which is doing exactly the same. We are largely still on 5.15 as LTS though, so 5.19 hasn't had much bashing in that regard. Your callstack implies it's only sequencing. vc4_hdmi_bind is manually calling vc4_hdmi_runtime_resume (and hence initialising registers) before the call to pm_runtime_set_active and pm_runtime_enable, hence the pm accounting check in vc4_hdmi_write fails. pm_runtime always seems like black magic to me :-/ Do we need to manually power up here, or can we call pm_runtime_enable without touching the state, and then resume in the normal manner? ie something simple like pm_runtime_enable(dev); pm_runtime_resume_and_get(dev); The resume_and_get will call vc4_hdmi_runtime_resume and hence initialise the block, but it will have sorted the pm accounting first. Otherwise we mess with the order to be: pm_runtime_get_noresume(dev); pm_runtime_set_active(dev); ret = vc4_hdmi_runtime_resume(dev); if (ret) goto err_put_ddc; //This error handling needs to be checked pm_runtime_enable(dev); I have no feel for which is the "correct" approach in terms of pm_runtime, so will defer to others in that regard. Dave > Would be nice to see those fixes before 6.0 final, thanks! > -- > Florian
Hi Florian, Am 09.08.22 um 21:02 schrieb Florian Fainelli: > On 8/4/22 16:11, Florian Fainelli wrote: >> On 6/13/22 07:47, Maxime Ripard wrote: >>> From: Dave Stevenson <dave.stevenson@raspberrypi.com> >>> >>> The BCM2835-37 found in the RaspberryPi 0 to 3 have a power domain >>> attached to the HDMI block, handled in Linux through runtime_pm. >>> >>> That power domain is shared with the VEC block, so even if we put our >>> runtime_pm reference in the HDMI driver it would keep being on. If the >>> VEC is disabled though, the power domain would be disabled and we would >>> lose any initialization done in our bind implementation. >>> >>> That initialization involves calling the reset function and >>> initializing >>> the CEC registers. >>> >>> Let's move the initialization to our runtime_resume implementation so >>> that we initialize everything properly if we ever need to. >>> >>> Fixes: c86b41214362 ("drm/vc4: hdmi: Move the HSM clock enable to >>> runtime_pm") >>> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com> >>> Signed-off-by: Maxime Ripard <maxime@cerno.tech> >> >> After seeing the same warning as Stefan reported in the link below, >> but on the Raspberry Pi 4B: >> >> https://www.spinics.net/lists/dri-devel/msg354170.html >> >> a separate bisection effort led me to this commit, before is fine, >> after produces 4 warnings during boot, see attached log. >> >> Is there a fix that we can try that would also cover the Raspberry Pi >> 4B? Is it possible that this series precipitates the problem: >> >> https://www.spinics.net/lists/arm-kernel/msg984638.html > > Maxime, Dave, anything you would want me to try? Still seeing these > warnings with net-next-6.0-11220-g15205c2829ca At first this issue doesn't occur in Linux 5.19. So it's something new. I was able to reproduce with todays linux-next, but interestingly it doesn't occur in drm-misc-next. > > Would be nice to see those fixes before 6.0 final, thanks!
On Wed, Aug 10, 2022 at 10:33:48PM +0200, Stefan Wahren wrote: > Hi Florian, > > Am 09.08.22 um 21:02 schrieb Florian Fainelli: > > On 8/4/22 16:11, Florian Fainelli wrote: > > > On 6/13/22 07:47, Maxime Ripard wrote: > > > > From: Dave Stevenson <dave.stevenson@raspberrypi.com> > > > > > > > > The BCM2835-37 found in the RaspberryPi 0 to 3 have a power domain > > > > attached to the HDMI block, handled in Linux through runtime_pm. > > > > > > > > That power domain is shared with the VEC block, so even if we put our > > > > runtime_pm reference in the HDMI driver it would keep being on. If the > > > > VEC is disabled though, the power domain would be disabled and we would > > > > lose any initialization done in our bind implementation. > > > > > > > > That initialization involves calling the reset function and > > > > initializing > > > > the CEC registers. > > > > > > > > Let's move the initialization to our runtime_resume implementation so > > > > that we initialize everything properly if we ever need to. > > > > > > > > Fixes: c86b41214362 ("drm/vc4: hdmi: Move the HSM clock enable > > > > to runtime_pm") > > > > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com> > > > > Signed-off-by: Maxime Ripard <maxime@cerno.tech> > > > > > > After seeing the same warning as Stefan reported in the link below, > > > but on the Raspberry Pi 4B: > > > > > > https://www.spinics.net/lists/dri-devel/msg354170.html > > > > > > a separate bisection effort led me to this commit, before is fine, > > > after produces 4 warnings during boot, see attached log. > > > > > > Is there a fix that we can try that would also cover the Raspberry > > > Pi 4B? Is it possible that this series precipitates the problem: > > > > > > https://www.spinics.net/lists/arm-kernel/msg984638.html > > > > Maxime, Dave, anything you would want me to try? Still seeing these > > warnings with net-next-6.0-11220-g15205c2829ca > > At first this issue doesn't occur in Linux 5.19. So it's something new. I > was able to reproduce with todays linux-next, but interestingly it doesn't > occur in drm-misc-next. Yeah, it should be fixed by https://lore.kernel.org/all/20220629123510.1915022-38-maxime@cerno.tech/ https://lore.kernel.org/all/20220629123510.1915022-39-maxime@cerno.tech/ Both patches apparently didn't make the cut for the merge window, if it works for you we can always queue them in drm-misc-fixes Maxime
On 8/15/22 07:12, Maxime Ripard wrote: > On Wed, Aug 10, 2022 at 10:33:48PM +0200, Stefan Wahren wrote: >> Hi Florian, >> >> Am 09.08.22 um 21:02 schrieb Florian Fainelli: >>> On 8/4/22 16:11, Florian Fainelli wrote: >>>> On 6/13/22 07:47, Maxime Ripard wrote: >>>>> From: Dave Stevenson <dave.stevenson@raspberrypi.com> >>>>> >>>>> The BCM2835-37 found in the RaspberryPi 0 to 3 have a power domain >>>>> attached to the HDMI block, handled in Linux through runtime_pm. >>>>> >>>>> That power domain is shared with the VEC block, so even if we put our >>>>> runtime_pm reference in the HDMI driver it would keep being on. If the >>>>> VEC is disabled though, the power domain would be disabled and we would >>>>> lose any initialization done in our bind implementation. >>>>> >>>>> That initialization involves calling the reset function and >>>>> initializing >>>>> the CEC registers. >>>>> >>>>> Let's move the initialization to our runtime_resume implementation so >>>>> that we initialize everything properly if we ever need to. >>>>> >>>>> Fixes: c86b41214362 ("drm/vc4: hdmi: Move the HSM clock enable >>>>> to runtime_pm") >>>>> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com> >>>>> Signed-off-by: Maxime Ripard <maxime@cerno.tech> >>>> >>>> After seeing the same warning as Stefan reported in the link below, >>>> but on the Raspberry Pi 4B: >>>> >>>> https://www.spinics.net/lists/dri-devel/msg354170.html >>>> >>>> a separate bisection effort led me to this commit, before is fine, >>>> after produces 4 warnings during boot, see attached log. >>>> >>>> Is there a fix that we can try that would also cover the Raspberry >>>> Pi 4B? Is it possible that this series precipitates the problem: >>>> >>>> https://www.spinics.net/lists/arm-kernel/msg984638.html >>> >>> Maxime, Dave, anything you would want me to try? Still seeing these >>> warnings with net-next-6.0-11220-g15205c2829ca >> >> At first this issue doesn't occur in Linux 5.19. So it's something new. I >> was able to reproduce with todays linux-next, but interestingly it doesn't >> occur in drm-misc-next. > > Yeah, it should be fixed by > https://lore.kernel.org/all/20220629123510.1915022-38-maxime@cerno.tech/ > https://lore.kernel.org/all/20220629123510.1915022-39-maxime@cerno.tech/ > > Both patches apparently didn't make the cut for the merge window, if it > works for you we can always queue them in drm-misc-fixes Both of these patches eliminate the warning, I don't have a good set-up yet for ensuring that HDMI/V3dD is functional however: Tested-by: Florian Fainelli <f.fainelli@gmail.com>
On Mon, Aug 15, 2022 at 09:52:59AM -0700, Florian Fainelli wrote: > On 8/15/22 07:12, Maxime Ripard wrote: > > On Wed, Aug 10, 2022 at 10:33:48PM +0200, Stefan Wahren wrote: > > > Hi Florian, > > > > > > Am 09.08.22 um 21:02 schrieb Florian Fainelli: > > > > On 8/4/22 16:11, Florian Fainelli wrote: > > > > > On 6/13/22 07:47, Maxime Ripard wrote: > > > > > > From: Dave Stevenson <dave.stevenson@raspberrypi.com> > > > > > > > > > > > > The BCM2835-37 found in the RaspberryPi 0 to 3 have a power domain > > > > > > attached to the HDMI block, handled in Linux through runtime_pm. > > > > > > > > > > > > That power domain is shared with the VEC block, so even if we put our > > > > > > runtime_pm reference in the HDMI driver it would keep being on. If the > > > > > > VEC is disabled though, the power domain would be disabled and we would > > > > > > lose any initialization done in our bind implementation. > > > > > > > > > > > > That initialization involves calling the reset function and > > > > > > initializing > > > > > > the CEC registers. > > > > > > > > > > > > Let's move the initialization to our runtime_resume implementation so > > > > > > that we initialize everything properly if we ever need to. > > > > > > > > > > > > Fixes: c86b41214362 ("drm/vc4: hdmi: Move the HSM clock enable > > > > > > to runtime_pm") > > > > > > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com> > > > > > > Signed-off-by: Maxime Ripard <maxime@cerno.tech> > > > > > > > > > > After seeing the same warning as Stefan reported in the link below, > > > > > but on the Raspberry Pi 4B: > > > > > > > > > > https://www.spinics.net/lists/dri-devel/msg354170.html > > > > > > > > > > a separate bisection effort led me to this commit, before is fine, > > > > > after produces 4 warnings during boot, see attached log. > > > > > > > > > > Is there a fix that we can try that would also cover the Raspberry > > > > > Pi 4B? Is it possible that this series precipitates the problem: > > > > > > > > > > https://www.spinics.net/lists/arm-kernel/msg984638.html > > > > > > > > Maxime, Dave, anything you would want me to try? Still seeing these > > > > warnings with net-next-6.0-11220-g15205c2829ca > > > > > > At first this issue doesn't occur in Linux 5.19. So it's something new. I > > > was able to reproduce with todays linux-next, but interestingly it doesn't > > > occur in drm-misc-next. > > > > Yeah, it should be fixed by > > https://lore.kernel.org/all/20220629123510.1915022-38-maxime@cerno.tech/ > > https://lore.kernel.org/all/20220629123510.1915022-39-maxime@cerno.tech/ > > > > Both patches apparently didn't make the cut for the merge window, if it > > works for you we can always queue them in drm-misc-fixes > > Both of these patches eliminate the warning, I don't have a good set-up yet > for ensuring that HDMI/V3dD is functional however: > > Tested-by: Florian Fainelli <f.fainelli@gmail.com> I ended up applying it today (without your Tested-by, since it was partial though) Maxime
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c index 8142efa2d479..654c4116b669 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.c +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c @@ -2543,8 +2543,6 @@ static int vc4_hdmi_cec_init(struct vc4_hdmi *vc4_hdmi) struct cec_connector_info conn_info; struct platform_device *pdev = vc4_hdmi->pdev; struct device *dev = &pdev->dev; - unsigned long flags; - u32 value; int ret; if (!of_find_property(dev->of_node, "interrupts", NULL)) { @@ -2563,15 +2561,6 @@ static int vc4_hdmi_cec_init(struct vc4_hdmi *vc4_hdmi) cec_fill_conn_info_from_drm(&conn_info, &vc4_hdmi->connector); cec_s_conn_info(vc4_hdmi->cec_adap, &conn_info); - spin_lock_irqsave(&vc4_hdmi->hw_lock, flags); - value = HDMI_READ(HDMI_CEC_CNTRL_1); - /* Set the logical address to Unregistered */ - value |= VC4_HDMI_CEC_ADDR_MASK; - HDMI_WRITE(HDMI_CEC_CNTRL_1, value); - spin_unlock_irqrestore(&vc4_hdmi->hw_lock, flags); - - vc4_hdmi_cec_update_clk_div(vc4_hdmi); - if (vc4_hdmi->variant->external_irq_controller) { ret = request_threaded_irq(platform_get_irq_byname(pdev, "cec-rx"), vc4_cec_irq_handler_rx_bare, @@ -2587,10 +2576,6 @@ static int vc4_hdmi_cec_init(struct vc4_hdmi *vc4_hdmi) if (ret) goto err_remove_cec_rx_handler; } else { - spin_lock_irqsave(&vc4_hdmi->hw_lock, flags); - HDMI_WRITE(HDMI_CEC_CPU_MASK_SET, 0xffffffff); - spin_unlock_irqrestore(&vc4_hdmi->hw_lock, flags); - ret = request_threaded_irq(platform_get_irq(pdev, 0), vc4_cec_irq_handler, vc4_cec_irq_handler_thread, 0, @@ -2641,7 +2626,6 @@ static int vc4_hdmi_cec_init(struct vc4_hdmi *vc4_hdmi) } static void vc4_hdmi_cec_exit(struct vc4_hdmi *vc4_hdmi) {}; - #endif static int vc4_hdmi_build_regset(struct vc4_hdmi *vc4_hdmi, @@ -2870,12 +2854,34 @@ static int __maybe_unused vc4_hdmi_runtime_suspend(struct device *dev) static int vc4_hdmi_runtime_resume(struct device *dev) { struct vc4_hdmi *vc4_hdmi = dev_get_drvdata(dev); + unsigned long __maybe_unused flags; + u32 __maybe_unused value; int ret; ret = clk_prepare_enable(vc4_hdmi->hsm_clock); if (ret) return ret; + if (vc4_hdmi->variant->reset) + vc4_hdmi->variant->reset(vc4_hdmi); + +#ifdef CONFIG_DRM_VC4_HDMI_CEC + spin_lock_irqsave(&vc4_hdmi->hw_lock, flags); + value = HDMI_READ(HDMI_CEC_CNTRL_1); + /* Set the logical address to Unregistered */ + value |= VC4_HDMI_CEC_ADDR_MASK; + HDMI_WRITE(HDMI_CEC_CNTRL_1, value); + spin_unlock_irqrestore(&vc4_hdmi->hw_lock, flags); + + vc4_hdmi_cec_update_clk_div(vc4_hdmi); + + if (!vc4_hdmi->variant->external_irq_controller) { + spin_lock_irqsave(&vc4_hdmi->hw_lock, flags); + HDMI_WRITE(HDMI_CEC_CPU_MASK_SET, 0xffffffff); + spin_unlock_irqrestore(&vc4_hdmi->hw_lock, flags); + } +#endif + return 0; } @@ -2965,9 +2971,6 @@ static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data) pm_runtime_set_active(dev); pm_runtime_enable(dev); - if (vc4_hdmi->variant->reset) - vc4_hdmi->variant->reset(vc4_hdmi); - if ((of_device_is_compatible(dev->of_node, "brcm,bcm2711-hdmi0") || of_device_is_compatible(dev->of_node, "brcm,bcm2711-hdmi1")) && HDMI_READ(HDMI_VID_CTL) & VC4_HD_VID_CTL_ENABLE) {