| Message ID | 1638858770-22594-2-git-send-email-spujar@nvidia.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/3] ALSA: hda/tegra: Skip reset on BPMP devices | expand |
07.12.2021 20:37, Sameer Pujar пишет: .. >> HDA won't be reset while display is active on T186+. > > No. HDA reset is applied whenever power-domain is ungated. It can happen > when either HDA or display device becomes active. I said "display is active", where do you see "becomes active"? > So I don't think that > it is inconsistent. If display is already active, then shared power domain is already ungated. It won't be ungated second time in a row, HDA won't get the reset.
On 12/7/2021 11:32 PM, Dmitry Osipenko wrote > If display is already active, then shared power domain is already > ungated. If display is already active, then shared power domain is already ungated. HDA reset is already applied during this ungate. In other words, HDA would be reset as well when display ungates power-domain.
08.12.2021 08:22, Sameer Pujar пишет: > > > On 12/7/2021 11:32 PM, Dmitry Osipenko wrote >> If display is already active, then shared power domain is already >> ungated. > > If display is already active, then shared power domain is already > ungated. HDA reset is already applied during this ungate. In other > words, HDA would be reset as well when display ungates power-domain. Now, if you'll reload the HDA driver module while display is active, you'll get a different reset behaviour. HDA hardware will be reset on pre-T186, on T186+ it won't be reset. Please make v2 using devm_reset_control_bulk_get_exclusive(), skipping the non-existent reset, or move the workaround to the BPMP driver like I suggested in the other reply.
On 12/8/2021 5:35 PM, Dmitry Osipenko wrote: > 08.12.2021 08:22, Sameer Pujar пишет: >> >> On 12/7/2021 11:32 PM, Dmitry Osipenko wrote >>> If display is already active, then shared power domain is already >>> ungated. >> If display is already active, then shared power domain is already >> ungated. HDA reset is already applied during this ungate. In other >> words, HDA would be reset as well when display ungates power-domain. > Now, if you'll reload the HDA driver module while display is active, > you'll get a different reset behaviour. HDA hardware will be reset on > pre-T186, on T186+ it won't be reset. How the reset behavior is different? At this point when HDA driver is loaded the HW is already reset during display ungate. What matters, during HDA driver load, is whether the HW is in predictable state or not and the answer is yes. So I am not sure what problem you are referring to. Question is, if BPMP already ensures this, then why driver needs to take care of it.
14.12.2021 09:02, Sameer Pujar пишет: > > > On 12/8/2021 5:35 PM, Dmitry Osipenko wrote: >> 08.12.2021 08:22, Sameer Pujar пишет: >>> >>> On 12/7/2021 11:32 PM, Dmitry Osipenko wrote >>>> If display is already active, then shared power domain is already >>>> ungated. >>> If display is already active, then shared power domain is already >>> ungated. HDA reset is already applied during this ungate. In other >>> words, HDA would be reset as well when display ungates power-domain. >> Now, if you'll reload the HDA driver module while display is active, >> you'll get a different reset behaviour. HDA hardware will be reset on >> pre-T186, on T186+ it won't be reset. > > How the reset behavior is different? At this point when HDA driver is > loaded the HW is already reset during display ungate. What matters, > during HDA driver load, is whether the HW is in predictable state or not > and the answer is yes. So I am not sure what problem you are referring > to. Question is, if BPMP already ensures this, then why driver needs to > take care of it. 1. Enable display 2. Play audio over HDMI 3. HDA hardware now is in dirty state 4. Reload HDA driver 5. In your case HDA is kept in dirty state, in my no
14.12.2021 09:09, Dmitry Osipenko пишет: > 14.12.2021 09:02, Sameer Pujar пишет: >> >> >> On 12/8/2021 5:35 PM, Dmitry Osipenko wrote: >>> 08.12.2021 08:22, Sameer Pujar пишет: >>>> >>>> On 12/7/2021 11:32 PM, Dmitry Osipenko wrote >>>>> If display is already active, then shared power domain is already >>>>> ungated. >>>> If display is already active, then shared power domain is already >>>> ungated. HDA reset is already applied during this ungate. In other >>>> words, HDA would be reset as well when display ungates power-domain. >>> Now, if you'll reload the HDA driver module while display is active, >>> you'll get a different reset behaviour. HDA hardware will be reset on >>> pre-T186, on T186+ it won't be reset. >> >> How the reset behavior is different? At this point when HDA driver is >> loaded the HW is already reset during display ungate. What matters, >> during HDA driver load, is whether the HW is in predictable state or not >> and the answer is yes. So I am not sure what problem you are referring >> to. Question is, if BPMP already ensures this, then why driver needs to >> take care of it. > > 1. Enable display > 2. Play audio over HDMI > 3. HDA hardware now is in dirty state > 4. Reload HDA driver > 5. In your case HDA is kept in dirty state, in my no > The power domain is shared by display and HDA, is this correct? If yes, then the shared power domain will be turned off only when all its clients are turned off, i.e. both display and HDA simultaneously.
On 12/14/2021 11:39 AM, Dmitry Osipenko wrote: > 14.12.2021 09:02, Sameer Pujar пишет: >> >> On 12/8/2021 5:35 PM, Dmitry Osipenko wrote: >>> 08.12.2021 08:22, Sameer Pujar пишет: >>>> On 12/7/2021 11:32 PM, Dmitry Osipenko wrote >>>>> If display is already active, then shared power domain is already >>>>> ungated. >>>> If display is already active, then shared power domain is already >>>> ungated. HDA reset is already applied during this ungate. In other >>>> words, HDA would be reset as well when display ungates power-domain. >>> Now, if you'll reload the HDA driver module while display is active, >>> you'll get a different reset behaviour. HDA hardware will be reset on >>> pre-T186, on T186+ it won't be reset. >> How the reset behavior is different? At this point when HDA driver is >> loaded the HW is already reset during display ungate. What matters, >> during HDA driver load, is whether the HW is in predictable state or not >> and the answer is yes. So I am not sure what problem you are referring >> to. Question is, if BPMP already ensures this, then why driver needs to >> take care of it. > 1. Enable display > 2. Play audio over HDMI > 3. HDA hardware now is in dirty state Why this would be a dirty state? It is rather a functional state. Isn't it? Power-domain is ON while all this happens. Another point is, with present logic the reset is not applied for every runtime PM resume of HDA device, which is confusing. It depends on the state of 'chip->running' flag and I don't see this getting cleared anywhere. Would you say subsequent HDA playback happen under a dirty state?
14.12.2021 10:22, Sameer Pujar пишет: ... >>> How the reset behavior is different? At this point when HDA driver is >>> loaded the HW is already reset during display ungate. What matters, >>> during HDA driver load, is whether the HW is in predictable state or not >>> and the answer is yes. So I am not sure what problem you are referring >>> to. Question is, if BPMP already ensures this, then why driver needs to >>> take care of it. >> 1. Enable display >> 2. Play audio over HDMI > >> 3. HDA hardware now is in dirty state > > Why this would be a dirty state? It is rather a functional state. Isn't > it? Power-domain is ON while all this happens. In general state should be a functional, but we shouldn't assume that. There is always a possibility for a subtle bug in a driver that may put h/w into a bad state. Full hardware reset is encouraged by users. > Another point is, with present logic the reset is not applied for every > runtime PM resume of HDA device, which is confusing. It depends on the > state of 'chip->running' flag and I don't see this getting cleared > anywhere. Would you say subsequent HDA playback happen under a dirty state? This is a good point. There should be another potential problem in the HDA driver for newer SoCs because apparently we don't re-initialize HDA controller properly after runtime PM resume. See hda_tegra_first_init() that is invoked only during driver probe, it configures FPCI_DBG_CFG_2 register on T194, which isn't done by hda_tegra_init(), and thus, this register may be in reset state after resume from RPM suspend. It should be a bug in the HDA driver that needs to be fixed. On older SoCs: HDA resides in the APB power domain which could be disabled only across system suspend/resume. HDA is only clock-gated during runtime PM suspend. On newer SoCs: HDA power state could be lost after RPM suspend/resume, depending on the state of display. I'm wondering whether HDMI playback works after DPMS on T194+, I assume this case was never tested properly. It looks like it should be safe to reset HDA on runtime PM resume regardless of the chip->running, and thus, we could remove that check and reset HDA unconditionally. Will great if you could check/test and improve this in the driver. I'm also wondering whether snd_power_change_state() should be moved into RPM callbacks and whether this function does anything practically useful on Tegra at all.
On Tue, 14 Dec 2021 14:56:12 +0100, Dmitry Osipenko wrote: > > 14.12.2021 10:22, Sameer Pujar пишет: > ... > >>> How the reset behavior is different? At this point when HDA driver is > >>> loaded the HW is already reset during display ungate. What matters, > >>> during HDA driver load, is whether the HW is in predictable state or not > >>> and the answer is yes. So I am not sure what problem you are referring > >>> to. Question is, if BPMP already ensures this, then why driver needs to > >>> take care of it. > >> 1. Enable display > >> 2. Play audio over HDMI > > > >> 3. HDA hardware now is in dirty state > > > > Why this would be a dirty state? It is rather a functional state. Isn't > > it? Power-domain is ON while all this happens. > > In general state should be a functional, but we shouldn't assume that. > There is always a possibility for a subtle bug in a driver that may put > h/w into a bad state. Full hardware reset is encouraged by users. > > > Another point is, with present logic the reset is not applied for every > > runtime PM resume of HDA device, which is confusing. It depends on the > > state of 'chip->running' flag and I don't see this getting cleared > > anywhere. Would you say subsequent HDA playback happen under a dirty state? > > This is a good point. There should be another potential problem in the > HDA driver for newer SoCs because apparently we don't re-initialize HDA > controller properly after runtime PM resume. > > See hda_tegra_first_init() that is invoked only during driver probe, it > configures FPCI_DBG_CFG_2 register on T194, which isn't done by > hda_tegra_init(), and thus, this register may be in reset state after > resume from RPM suspend. It should be a bug in the HDA driver that needs > to be fixed. > > On older SoCs: HDA resides in the APB power domain which could be > disabled only across system suspend/resume. HDA is only clock-gated > during runtime PM suspend. > > On newer SoCs: HDA power state could be lost after RPM suspend/resume, > depending on the state of display. I'm wondering whether HDMI playback > works after DPMS on T194+, I assume this case was never tested properly. > > It looks like it should be safe to reset HDA on runtime PM resume > regardless of the chip->running, and thus, we could remove that check > and reset HDA unconditionally. Will great if you could check/test and > improve this in the driver. > > I'm also wondering whether snd_power_change_state() should be moved into > RPM callbacks and whether this function does anything practically useful > on Tegra at all. This call is mostly for ALSA core stuff, and not necessarily reflecting the exact device power state. The major role is for controlling / blocking the device accesses at the system suspend/resume, so it's correct to set only in the system suspend/resume callbacks, not in runtime PM. Takashi
14.12.2021 17:29, Takashi Iwai пишет: >> I'm also wondering whether snd_power_change_state() should be moved into >> RPM callbacks and whether this function does anything practically useful >> on Tegra at all. > This call is mostly for ALSA core stuff, and not necessarily > reflecting the exact device power state. The major role is for > controlling / blocking the device accesses at the system > suspend/resume, so it's correct to set only in the system > suspend/resume callbacks, not in runtime PM. > Thank you for the clarification.
On 12/14/2021 7:26 PM, Dmitry Osipenko wrote: > 14.12.2021 10:22, Sameer Pujar пишет: > ... >>>> How the reset behavior is different? At this point when HDA driver is >>>> loaded the HW is already reset during display ungate. What matters, >>>> during HDA driver load, is whether the HW is in predictable state or not >>>> and the answer is yes. So I am not sure what problem you are referring >>>> to. Question is, if BPMP already ensures this, then why driver needs to >>>> take care of it. >>> 1. Enable display >>> 2. Play audio over HDMI >>> 3. HDA hardware now is in dirty state >> Why this would be a dirty state? It is rather a functional state. Isn't >> it? Power-domain is ON while all this happens. > In general state should be a functional, but we shouldn't assume that. > There is always a possibility for a subtle bug in a driver that may put > h/w into a bad state. Full hardware reset is encouraged by users. OK. I will prepare a v2 by just skipping the invalid reset for Tegra194. > >> Another point is, with present logic the reset is not applied for every >> runtime PM resume of HDA device, which is confusing. It depends on the >> state of 'chip->running' flag and I don't see this getting cleared >> anywhere. Would you say subsequent HDA playback happen under a dirty state? > This is a good point. There should be another potential problem in the > HDA driver for newer SoCs because apparently we don't re-initialize HDA > controller properly after runtime PM resume. > > See hda_tegra_first_init() that is invoked only during driver probe, it > configures FPCI_DBG_CFG_2 register on T194, which isn't done by > hda_tegra_init(), and thus, this register may be in reset state after > resume from RPM suspend. It should be a bug in the HDA driver that needs > to be fixed. > > On older SoCs: HDA resides in the APB power domain which could be > disabled only across system suspend/resume. HDA is only clock-gated > during runtime PM suspend. > > On newer SoCs: HDA power state could be lost after RPM suspend/resume, > depending on the state of display. I'm wondering whether HDMI playback > works after DPMS on T194+, I assume this case was never tested properly. > > It looks like it should be safe to reset HDA on runtime PM resume > regardless of the chip->running, and thus, we could remove that check > and reset HDA unconditionally. Will great if you could check/test and > improve this in the driver. There seems to be multiple issues. I will work on this separately and send a separate series. Presently basic function is broken on Tegra194 and will first send v2 to fix the regression. Thanks for review.
diff --git a/sound/pci/hda/hda_tegra.c b/sound/pci/hda/hda_tegra.c index ea700395..862141e 100644 --- a/sound/pci/hda/hda_tegra.c +++ b/sound/pci/hda/hda_tegra.c @@ -68,6 +68,10 @@ */ #define TEGRA194_NUM_SDO_LINES 4 +struct hda_data { + unsigned int do_reset:1; +}; + struct hda_tegra { struct azx chip; struct device *dev; @@ -76,6 +80,7 @@ struct hda_tegra { unsigned int nclocks; void __iomem *regs; struct work_struct probe_work; + const struct hda_data *data; }; #ifdef CONFIG_PM @@ -427,8 +432,13 @@ static int hda_tegra_create(struct snd_card *card, return 0; } +static const struct hda_data tegra30_data = { + .do_reset = 1, +}; + static const struct of_device_id hda_tegra_match[] = { - { .compatible = "nvidia,tegra30-hda" }, + { .compatible = "nvidia,tegra30-hda", .data = &tegra30_data }, + { .compatible = "nvidia,tegra186-hda" }, { .compatible = "nvidia,tegra194-hda" }, {}, }; @@ -449,6 +459,8 @@ static int hda_tegra_probe(struct platform_device *pdev) hda->dev = &pdev->dev; chip = &hda->chip; + hda->data = of_device_get_match_data(&pdev->dev); + err = snd_card_new(&pdev->dev, SNDRV_DEFAULT_IDX1, SNDRV_DEFAULT_STR1, THIS_MODULE, 0, &card); if (err < 0) { @@ -456,10 +468,12 @@ static int hda_tegra_probe(struct platform_device *pdev) return err; } - hda->reset = devm_reset_control_array_get_exclusive(&pdev->dev); - if (IS_ERR(hda->reset)) { - err = PTR_ERR(hda->reset); - goto out_free; + if (hda->data && hda->data->do_reset) { + hda->reset = devm_reset_control_array_get_exclusive(&pdev->dev); + if (IS_ERR(hda->reset)) { + err = PTR_ERR(hda->reset); + goto out_free; + } } hda->clocks[hda->nclocks++].id = "hda";
HDA regression is recently reported on Tegra194 based platforms. This happens because "hda2codec_2x" reset does not really exist in Tegra194 and it causes probe failure. All the HDA based audio tests fail at the moment. This underlying issue is exposed by commit c045ceb5a145 ("reset: tegra-bpmp: Handle errors in BPMP response") which now checks return code of BPMP command response. The failure can be fixed by avoiding above reset in the driver, but the explicit reset is not necessary for Tegra devices which depend on BPMP. On such devices, BPMP ensures reset application during unpowergate calls. Hence skip reset on these devices which is applicable for Tegra186 and later. Signed-off-by: Sameer Pujar <spujar@nvidia.com> Cc: stable@vger.kernel.org Depends-on: 87f0e46e7559 ("ALSA: hda/tegra: Reset hardware") --- sound/pci/hda/hda_tegra.c | 24 +++++++++++++++++++----- 1 file changed, 19 insertions(+), 5 deletions(-)