Re: [PATCH] [sound] hdac-codec runtime suspended at PM:Suspend.
On Thu, Mar 15, 2018 at 09:02:30PM +0530, Anshuman Gupta wrote: > On Thu, Mar 15, 2018 at 04:12:44PM +0530, Subhransu S. Prusty wrote: > > On Wed, Mar 14, 2018 at 09:07:14PM +0530, Anshuman Gupta wrote: > > > On Wed, Mar 14, 2018 at 11:53:58AM +0100, Rafael J. Wysocki wrote: > > > > On Wed, Mar 14, 2018 at 11:38 AM, Anshuman Gupta > > > > <anshuman.gu...@intel.com> wrote: > > > > > On Mon, Mar 12, 2018 at 12:26:53PM +0100, Rafael J. Wysocki wrote: > > > > >> On Mon, Mar 12, 2018 at 12:17 PM, Anshuman Gupta > > > > >> <anshuman.gu...@intel.com> wrote: > > > > >> > > > > > >> > + if (pm_runtime_status_suspended(dev)) > > > > >> > + return; > > > > >> > > > > >> That, again, is somewhat fragile from the concurrency perspective. > > > > >> > > > > > > > > And here you want to avoid the below if the device is still suspended. > > > Yes, if we do not avoid the code below, complete callback takes about > > > 3 seconds due to snd_hdac_codec_read timed out because hdac controller > > > would be in runtime suspend state. > > > > > > > > Why is the below code located in the ->complete callback anyway? > > > > Shouldn't it be there in the ->resume one? > > > > > > > Yes even i am also having same doubt, why these power down and power up > > > sequences are part of prepare and complete callback. > > > Adding driver author "Subhransu S. Prusty" to provide more inputs on > > > this. > > > > This driver needs a late resume as it receives a jack notification from the > > i915 driver and the skl controller driver resume may not have happened and > > in turn hda controller may not ready. This ensures a synchronization for > > jack event during resume from S3. > Let me give you insight of the issue, this driver blocks the direct complete > of hda controller PCI 00:1f.3, sometimes it takes 280ms to resume hda > controller > from S3 and s2idle. So it does not make sense to suspend/resume hda > controller > when it is already in runtime suspend. I get it now. I think this patch needs rework to address both the issues. > > > > I think this patch defeats the purpose. > Here in this case PCI driver may kick the direct complete for hda controller > https://elixir.bootlin.com/linux/latest/source/drivers/pci/pci-driver.c#L691 > But hdac hdmi codec driver is blocking it. > So i think it will be ok to keep this codec and hda controller in runtime > suspend while entering S3 or s2idle, both can resume by runtime PM as well, > will it brake any audio functionality? There was some PM and jack detection issues (I don't recollect now) because of which this was added. This was due to the gfx display power and hda controller synchronization. The rework may be required in both hdac_hdmi and skylake drivers. Regards, Subhransu > > > > Regards, > > Subhransu > > > > > > >> > /* Power up afg */ > > > > >> > snd_hdac_codec_read(hdac, hdac->afg, 0, > > > > >> > AC_VERB_SET_POWER_STATE, > > > > >> > > > > > >> > AC_PWRST_D0); > > > > >> > -- > > > > >> > 2.7.4 > > > > > > -- > > > Thanks, > > > Anshuman > > > > -- > > -- > Thanks, > Anshuman --
Re: [PATCH] [sound] hdac-codec runtime suspended at PM:Suspend.
On Thu, Mar 15, 2018 at 09:02:30PM +0530, Anshuman Gupta wrote: > On Thu, Mar 15, 2018 at 04:12:44PM +0530, Subhransu S. Prusty wrote: > > On Wed, Mar 14, 2018 at 09:07:14PM +0530, Anshuman Gupta wrote: > > > On Wed, Mar 14, 2018 at 11:53:58AM +0100, Rafael J. Wysocki wrote: > > > > On Wed, Mar 14, 2018 at 11:38 AM, Anshuman Gupta > > > > wrote: > > > > > On Mon, Mar 12, 2018 at 12:26:53PM +0100, Rafael J. Wysocki wrote: > > > > >> On Mon, Mar 12, 2018 at 12:17 PM, Anshuman Gupta > > > > >> wrote: > > > > >> > > > > > >> > + if (pm_runtime_status_suspended(dev)) > > > > >> > + return; > > > > >> > > > > >> That, again, is somewhat fragile from the concurrency perspective. > > > > >> > > > > > > > > And here you want to avoid the below if the device is still suspended. > > > Yes, if we do not avoid the code below, complete callback takes about > > > 3 seconds due to snd_hdac_codec_read timed out because hdac controller > > > would be in runtime suspend state. > > > > > > > > Why is the below code located in the ->complete callback anyway? > > > > Shouldn't it be there in the ->resume one? > > > > > > > Yes even i am also having same doubt, why these power down and power up > > > sequences are part of prepare and complete callback. > > > Adding driver author "Subhransu S. Prusty" to provide more inputs on > > > this. > > > > This driver needs a late resume as it receives a jack notification from the > > i915 driver and the skl controller driver resume may not have happened and > > in turn hda controller may not ready. This ensures a synchronization for > > jack event during resume from S3. > Let me give you insight of the issue, this driver blocks the direct complete > of hda controller PCI 00:1f.3, sometimes it takes 280ms to resume hda > controller > from S3 and s2idle. So it does not make sense to suspend/resume hda > controller > when it is already in runtime suspend. I get it now. I think this patch needs rework to address both the issues. > > > > I think this patch defeats the purpose. > Here in this case PCI driver may kick the direct complete for hda controller > https://elixir.bootlin.com/linux/latest/source/drivers/pci/pci-driver.c#L691 > But hdac hdmi codec driver is blocking it. > So i think it will be ok to keep this codec and hda controller in runtime > suspend while entering S3 or s2idle, both can resume by runtime PM as well, > will it brake any audio functionality? There was some PM and jack detection issues (I don't recollect now) because of which this was added. This was due to the gfx display power and hda controller synchronization. The rework may be required in both hdac_hdmi and skylake drivers. Regards, Subhransu > > > > Regards, > > Subhransu > > > > > > >> > /* Power up afg */ > > > > >> > snd_hdac_codec_read(hdac, hdac->afg, 0, > > > > >> > AC_VERB_SET_POWER_STATE, > > > > >> > > > > > >> > AC_PWRST_D0); > > > > >> > -- > > > > >> > 2.7.4 > > > > > > -- > > > Thanks, > > > Anshuman > > > > -- > > -- > Thanks, > Anshuman --
Re: [PATCH] [sound] hdac-codec runtime suspended at PM:Suspend.
On Wed, Mar 14, 2018 at 09:07:14PM +0530, Anshuman Gupta wrote: > On Wed, Mar 14, 2018 at 11:53:58AM +0100, Rafael J. Wysocki wrote: > > On Wed, Mar 14, 2018 at 11:38 AM, Anshuman Gupta > > <anshuman.gu...@intel.com> wrote: > > > On Mon, Mar 12, 2018 at 12:26:53PM +0100, Rafael J. Wysocki wrote: > > >> On Mon, Mar 12, 2018 at 12:17 PM, Anshuman Gupta > > >> <anshuman.gu...@intel.com> wrote: > > >> > > > >> > + if (pm_runtime_status_suspended(dev)) > > >> > + return; > > >> > > >> That, again, is somewhat fragile from the concurrency perspective. > > >> > > > > And here you want to avoid the below if the device is still suspended. > Yes, if we do not avoid the code below, complete callback takes about > 3 seconds due to snd_hdac_codec_read timed out because hdac controller > would be in runtime suspend state. > > > > Why is the below code located in the ->complete callback anyway? > > Shouldn't it be there in the ->resume one? > > > Yes even i am also having same doubt, why these power down and power up > sequences are part of prepare and complete callback. > Adding driver author "Subhransu S. Prusty" to provide more inputs on this. This driver needs a late resume as it receives a jack notification from the i915 driver and the skl controller driver resume may not have happened and in turn hda controller may not ready. This ensures a synchronization for jack event during resume from S3. I think this patch defeats the purpose. Regards, Subhransu > > >> > /* Power up afg */ > > >> > snd_hdac_codec_read(hdac, hdac->afg, 0, > > >> > AC_VERB_SET_POWER_STATE, > > >> > AC_PWRST_D0); > > >> > -- > > >> > 2.7.4 > > -- > Thanks, > Anshuman --
Re: [PATCH] [sound] hdac-codec runtime suspended at PM:Suspend.
On Wed, Mar 14, 2018 at 09:07:14PM +0530, Anshuman Gupta wrote: > On Wed, Mar 14, 2018 at 11:53:58AM +0100, Rafael J. Wysocki wrote: > > On Wed, Mar 14, 2018 at 11:38 AM, Anshuman Gupta > > wrote: > > > On Mon, Mar 12, 2018 at 12:26:53PM +0100, Rafael J. Wysocki wrote: > > >> On Mon, Mar 12, 2018 at 12:17 PM, Anshuman Gupta > > >> wrote: > > >> > > > >> > + if (pm_runtime_status_suspended(dev)) > > >> > + return; > > >> > > >> That, again, is somewhat fragile from the concurrency perspective. > > >> > > > > And here you want to avoid the below if the device is still suspended. > Yes, if we do not avoid the code below, complete callback takes about > 3 seconds due to snd_hdac_codec_read timed out because hdac controller > would be in runtime suspend state. > > > > Why is the below code located in the ->complete callback anyway? > > Shouldn't it be there in the ->resume one? > > > Yes even i am also having same doubt, why these power down and power up > sequences are part of prepare and complete callback. > Adding driver author "Subhransu S. Prusty" to provide more inputs on this. This driver needs a late resume as it receives a jack notification from the i915 driver and the skl controller driver resume may not have happened and in turn hda controller may not ready. This ensures a synchronization for jack event during resume from S3. I think this patch defeats the purpose. Regards, Subhransu > > >> > /* Power up afg */ > > >> > snd_hdac_codec_read(hdac, hdac->afg, 0, > > >> > AC_VERB_SET_POWER_STATE, > > >> > AC_PWRST_D0); > > >> > -- > > >> > 2.7.4 > > -- > Thanks, > Anshuman --
[PATCH] PM: Add helper to mark last busy and autosuspend
pm_runtime_mark_last_busy and pm_runtime_put_autosuspend are used together
in quite a lot of places. Add a helper for these.
Signed-off-by: Subhransu S. Prusty
---
include/linux/pm_runtime.h | 6 ++
1 file changed, 6 insertions(+)
diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h
index 367f49b..256ec50 100644
--- a/include/linux/pm_runtime.h
+++ b/include/linux/pm_runtime.h
@@ -277,4 +277,10 @@ static inline void pm_runtime_dont_use_autosuspend(struct
device *dev)
__pm_runtime_use_autosuspend(dev, false);
}
+static inline int pm_runtime_last_busy_and_autosuspend(struct device *dev)
+{
+ pm_runtime_mark_last_busy(dev);
+ return pm_runtime_put_autosuspend(dev);
+}
+
#endif
--
1.9.0
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] PM: Add helper to mark last busy and autosuspend
pm_runtime_mark_last_busy and pm_runtime_put_autosuspend are used together
in quite a lot of places. Add a helper for these.
Signed-off-by: Subhransu S. Prusty subhransu.s.pru...@intel.com
---
include/linux/pm_runtime.h | 6 ++
1 file changed, 6 insertions(+)
diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h
index 367f49b..256ec50 100644
--- a/include/linux/pm_runtime.h
+++ b/include/linux/pm_runtime.h
@@ -277,4 +277,10 @@ static inline void pm_runtime_dont_use_autosuspend(struct
device *dev)
__pm_runtime_use_autosuspend(dev, false);
}
+static inline int pm_runtime_last_busy_and_autosuspend(struct device *dev)
+{
+ pm_runtime_mark_last_busy(dev);
+ return pm_runtime_put_autosuspend(dev);
+}
+
#endif
--
1.9.0
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] x86_64: Add memcpy32_toio to write to PCI MMIO
This is needed because the hardware does not support 64-bit moveq
insructions while writing to PCI MMIO.
Signed-off-by: Subhransu S. Prusty
Signed-off-by: Vinod Koul
---
arch/x86/include/asm/io.h | 22 ++
1 file changed, 22 insertions(+)
diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h
index b8237d8..fa095fb 100644
--- a/arch/x86/include/asm/io.h
+++ b/arch/x86/include/asm/io.h
@@ -218,6 +218,28 @@ memcpy_toio(volatile void __iomem *dst, const void *src,
size_t count)
memcpy((void __force *)dst, src, count);
}
+#ifndef CONFIG_X86_64
+#define MEMCPY_TOIO memcpy_toio
+#else
+#define MEMCPY_TOIO memcpy32_toio
+#endif
+
+/**
+ * memcpy32_toio: Copy using writel commands
+ *
+ * This is needed because the hardware does not support
+ * 64-bit moveq insructions while writing to PCI MMIO
+ */
+static inline void memcpy32_toio(void *dst, const void *src, int count)
+{
+ int i;
+ const u32 *src_32 = src;
+ u32 *dst_32 = dst;
+
+ for (i = 0; i < count/sizeof(u32); i++)
+ writel(*src_32++, dst_32++);
+}
+
/*
* ISA space is 'always mapped' on a typical x86 system, no need to
* explicitly ioremap() it. The fact that the ISA IO space is mapped
--
1.9.0
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] x86_64: Add memcpy32_toio to write to PCI MMIO
This is needed because the hardware does not support 64-bit moveq
insructions while writing to PCI MMIO.
Signed-off-by: Subhransu S. Prusty subhransu.s.pru...@intel.com
Signed-off-by: Vinod Koul vinod.k...@intel.com
---
arch/x86/include/asm/io.h | 22 ++
1 file changed, 22 insertions(+)
diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h
index b8237d8..fa095fb 100644
--- a/arch/x86/include/asm/io.h
+++ b/arch/x86/include/asm/io.h
@@ -218,6 +218,28 @@ memcpy_toio(volatile void __iomem *dst, const void *src,
size_t count)
memcpy((void __force *)dst, src, count);
}
+#ifndef CONFIG_X86_64
+#define MEMCPY_TOIO memcpy_toio
+#else
+#define MEMCPY_TOIO memcpy32_toio
+#endif
+
+/**
+ * memcpy32_toio: Copy using writel commands
+ *
+ * This is needed because the hardware does not support
+ * 64-bit moveq insructions while writing to PCI MMIO
+ */
+static inline void memcpy32_toio(void *dst, const void *src, int count)
+{
+ int i;
+ const u32 *src_32 = src;
+ u32 *dst_32 = dst;
+
+ for (i = 0; i count/sizeof(u32); i++)
+ writel(*src_32++, dst_32++);
+}
+
/*
* ISA space is 'always mapped' on a typical x86 system, no need to
* explicitly ioremap() it. The fact that the ISA IO space is mapped
--
1.9.0
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
