Re: [Intel-gfx] [PATCH v2] component: do not leave master devres group open after bind

[Takashi Iwai]

On Wed, 22 Sep 2021 10:54:32 +0200,
Kai Vehmanen wrote:
(snip)
> --- a/drivers/base/component.c
> +++ b/drivers/base/component.c
> @@ -246,7 +246,7 @@ static int try_to_bring_up_master(struct master *master,
>   return 0;
>   }
>  
> - if (!devres_open_group(master->parent, NULL, GFP_KERNEL))
> + if (!devres_open_group(master->parent, master, GFP_KERNEL))
>   return -ENOMEM;
>  
>   /* Found all components */
> @@ -258,6 +258,7 @@ static int try_to_bring_up_master(struct master *master,
>   return ret;
>   }
>  
> + devres_close_group(master->parent, NULL);

Just wondering whether we should pass master here instead of NULL,
too?


thanks,

Takashi

Re: [Intel-gfx] [PATCH] ALSA: hda: Release display power reference during shutdown/reboot

[Takashi Iwai]

On Mon, 21 Jun 2021 19:44:15 +0200,
Imre Deak wrote:
> 
> Make sure the HDA driver's display power reference is released during
> shutdown/reboot.
> 
> During the shutdown/reboot sequence the pci device core calls the
> pm_runtime_resume handler for all devices before calling the driver's
> shutdown callback and so the HDA driver's runtime resume callback will
> acquire a display power reference (on HSW/BDW). This triggers a power
> reference held WARN on HSW/BDW in the i915 driver's subsequent shutdown
> handler, which expects all display power references to be released by
> that time.
> 
> Since the HDA controller is stopped in the shutdown handler in any case,
> let's follow here the same sequence as the one during runtime suspend.
> This will also reset the HDA link and drop the display power reference,
> getting rid of the above WARN.

As kbuild bot suggested, __azx_runtime_suspend() is defined only with
CONFIG_PM.  We need either moving the function out of ifdef CONFIG_PM
block, or having CONFIG_PM conditional call there.

I myself have no much preference,  but maybe the latter can be easier
to be cherry-picked to stable kernels.


thanks,

Takashi
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] ALSA: hda: Release display power reference during shutdown/reboot

[Takashi Iwai]

On Tue, 22 Jun 2021 21:58:13 +0200,
Imre Deak wrote:
> 
> On Tue, Jun 22, 2021 at 04:18:14PM +0200, Takashi Iwai wrote:
> > On Mon, 21 Jun 2021 19:44:15 +0200,
> > Imre Deak wrote:
> > > 
> > > Make sure the HDA driver's display power reference is released during
> > > shutdown/reboot.
> > > 
> > > During the shutdown/reboot sequence the pci device core calls the
> > > pm_runtime_resume handler for all devices before calling the driver's
> > > shutdown callback and so the HDA driver's runtime resume callback will
> > > acquire a display power reference (on HSW/BDW). This triggers a power
> > > reference held WARN on HSW/BDW in the i915 driver's subsequent shutdown
> > > handler, which expects all display power references to be released by
> > > that time.
> > > 
> > > Since the HDA controller is stopped in the shutdown handler in any case,
> > > let's follow here the same sequence as the one during runtime suspend.
> > > This will also reset the HDA link and drop the display power reference,
> > > getting rid of the above WARN.
> > 
> > As kbuild bot suggested, __azx_runtime_suspend() is defined only with
> > CONFIG_PM.  We need either moving the function out of ifdef CONFIG_PM
> > block, or having CONFIG_PM conditional call there.
> 
> Thanks, missed that. I think we need to drop the power ref in the !CONFIG_PM
> case as well (since AFAICS then the ref is held after the device is probed), 
> so
> I'd move __azx_runtime_suspend() out of the CONFIG_PM block (and perhaps 
> rename
> it to azx_shutdown_chip).
> 
> > I myself have no much preference,  but maybe the latter can be easier
> > to be cherry-picked to stable kernels.
> 
> To my knowledge this only fixes the book-keeping in the i915 driver, so
> not sure if it's a stable material.
> 
> Trying things now with !CONFIG_PM, I noticed that the HDA codec would still
> keep a separate power reference (which was dropped for me with CONFIG_PM, as
> the codec was runtime suspended). To fix that we'd need something like the
> following (on top of the above changes in a separate patch), any
> comments on it?:

Adding the common dev_shutdown sounds a bit like overkill.
Since it's just a missing shutdown handling in the hd-audio codec
side, does the patch like below work instead?

If this works, feel free to fold into your patch.


thanks,

Takashi

---
diff --git a/sound/pci/hda/hda_bind.c b/sound/pci/hda/hda_bind.c
index 17a25e453f60..e8dee24c309d 100644
--- a/sound/pci/hda/hda_bind.c
+++ b/sound/pci/hda/hda_bind.c
@@ -167,8 +167,11 @@ static void hda_codec_driver_shutdown(struct device *dev)
 {
struct hda_codec *codec = dev_to_hda_codec(dev);
 
-   if (!pm_runtime_suspended(dev) && codec->patch_ops.reboot_notify)
-   codec->patch_ops.reboot_notify(codec);
+   if (!pm_runtime_suspended(dev)) {
+   if (codec->patch_ops.reboot_notify)
+   codec->patch_ops.reboot_notify(codec);
+   snd_hda_codec_display_power(codec, false);
+   }
 }
 
 int __hda_codec_driver_register(struct hda_codec_driver *drv, const char *name,
diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
index 5462f771c2f9..7a717e151156 100644
--- a/sound/pci/hda/hda_codec.c
+++ b/sound/pci/hda/hda_codec.c
@@ -798,7 +798,7 @@ static unsigned int hda_set_power_state(struct hda_codec 
*codec,
unsigned int power_state);
 
 /* enable/disable display power per codec */
-static void codec_display_power(struct hda_codec *codec, bool enable)
+void snd_hda_codec_display_power(struct hda_codec *codec, bool enable)
 {
if (codec->display_power_control)
snd_hdac_display_power(&codec->bus->core, codec->addr, enable);
@@ -810,7 +810,7 @@ void snd_hda_codec_register(struct hda_codec *codec)
if (codec->registered)
return;
if (device_is_registered(hda_codec_dev(codec))) {
-   codec_display_power(codec, true);
+   snd_hda_codec_display_power(codec, true);
pm_runtime_enable(hda_codec_dev(codec));
/* it was powered up in snd_hda_codec_new(), now all done */
snd_hda_power_down(codec);
@@ -836,7 +836,7 @@ static int snd_hda_codec_dev_free(struct snd_device *device)
 */
if (codec->core.type == HDA_DEV_LEGACY)
snd_hdac_device_unregister(&codec->core);
-   codec_display_power(codec, false);
+   snd_hda_codec_display_power(codec, false);
 
/*
 * In the case of ASoC HD-audio bus, the device refcount is released in
@@ -2893,7 +2893,7 @@ static int hda_codec_runtime_suspend(struct device *dev)

Re: [Intel-gfx] [PATCH 1/2] ALSA: hda: Release controller display power during shutdown/reboot

[Takashi Iwai]

On Wed, 23 Jun 2021 15:46:00 +0200,
Imre Deak wrote:
> 
> Make sure the HDA driver's display power reference is released during
> shutdown/reboot.
> 
> During the shutdown/reboot sequence the pci device core calls the
> pm_runtime_resume handler for all devices before calling the driver's
> shutdown callback and so the HDA driver's runtime resume callback will
> acquire a display power reference (on HSW/BDW). This triggers a power
> reference held WARN on HSW/BDW in the i915 driver's subsequent shutdown
> handler, which expects all display power references to be released by
> that time.
> 
> Since the HDA controller is stopped in the shutdown handler in any case,
> let's follow here the same sequence as the one during runtime suspend.
> This will also reset the HDA link and drop the display power reference,
> getting rid of the above WARN.
> 
> Tested on HSW.
> 
> v2:
> - Fix the build for CONFIG_PM=n (Takashi)
> - s/__azx_runtime_suspend/azx_shutdown_chip/
> 
> Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/3618
> References: 
> https://lore.kernel.org/lkml/cea1f9a-52e0-b83-593d-52997fe1a...@er-systems.de
> Reported-and-tested-by: Thomas Voegtle 
> Cc: Takashi Iwai 
> Signed-off-by: Imre Deak 

Thanks, applied both patches now.


Takashi
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH] drm/i915: Fix invalid access to ACPI _DSM objects

[Takashi Iwai]

intel_dsm_platform_mux_info() tries to parse the ACPI package data
from _DSM for the debug information, but it assumes the fixed format
without checking what values are stored in the elements actually.
When an unexpected value is returned from BIOS, it may lead to GPF or
NULL dereference, as reported recently.

Add the checks of the contents in the returned values and skip the
values for invalid cases.

BugLink: http://bugzilla.opensuse.org/show_bug.cgi?id=1184074
Cc: 
Signed-off-by: Takashi Iwai 
---
 drivers/gpu/drm/i915/display/intel_acpi.c | 12 
 1 file changed, 12 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_acpi.c 
b/drivers/gpu/drm/i915/display/intel_acpi.c
index e21fb14d5e07..492ebc0a8257 100644
--- a/drivers/gpu/drm/i915/display/intel_acpi.c
+++ b/drivers/gpu/drm/i915/display/intel_acpi.c
@@ -84,6 +84,11 @@ static void intel_dsm_platform_mux_info(acpi_handle dhandle)
return;
}
 
+   if (!pkg->package.count) {
+   DRM_DEBUG_DRIVER("no connection in _DSM\n");
+   return;
+   }
+
connector_count = &pkg->package.elements[0];
DRM_DEBUG_DRIVER("MUX info connectors: %lld\n",
  (unsigned long long)connector_count->integer.value);
@@ -91,6 +96,13 @@ static void intel_dsm_platform_mux_info(acpi_handle dhandle)
union acpi_object *obj = &pkg->package.elements[i];
union acpi_object *connector_id = &obj->package.elements[0];
union acpi_object *info = &obj->package.elements[1];
+
+   if (obj->type != ACPI_TYPE_PACKAGE || obj->package.count < 2 ||
+   info->type != ACPI_TYPE_BUFFER || info->buffer.length < 4) {
+   DRM_DEBUG_DRIVER("Invalid object for MUX #%d\n", i);
+   continue;
+   }
+
DRM_DEBUG_DRIVER("Connector id: 0x%016llx\n",
  (unsigned long long)connector_id->integer.value);
DRM_DEBUG_DRIVER("  port id: %s\n",
-- 
2.26.2

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Fix invalid access to ACPI _DSM objects

[Takashi Iwai]

On Fri, 02 Apr 2021 09:47:49 +0200,
Takashi Iwai wrote:
> 
> intel_dsm_platform_mux_info() tries to parse the ACPI package data
> from _DSM for the debug information, but it assumes the fixed format
> without checking what values are stored in the elements actually.
> When an unexpected value is returned from BIOS, it may lead to GPF or
> NULL dereference, as reported recently.
> 
> Add the checks of the contents in the returned values and skip the
> values for invalid cases.
> 
> BugLink: http://bugzilla.opensuse.org/show_bug.cgi?id=1184074
> Cc: 
> Signed-off-by: Takashi Iwai 

Scratch this one.  I sent an older version mistakenly.
Will resubmit the right one.


Takashi


> ---
>  drivers/gpu/drm/i915/display/intel_acpi.c | 12 
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_acpi.c 
> b/drivers/gpu/drm/i915/display/intel_acpi.c
> index e21fb14d5e07..492ebc0a8257 100644
> --- a/drivers/gpu/drm/i915/display/intel_acpi.c
> +++ b/drivers/gpu/drm/i915/display/intel_acpi.c
> @@ -84,6 +84,11 @@ static void intel_dsm_platform_mux_info(acpi_handle 
> dhandle)
>   return;
>   }
>  
> + if (!pkg->package.count) {
> + DRM_DEBUG_DRIVER("no connection in _DSM\n");
> + return;
> + }
> +
>   connector_count = &pkg->package.elements[0];
>   DRM_DEBUG_DRIVER("MUX info connectors: %lld\n",
> (unsigned long long)connector_count->integer.value);
> @@ -91,6 +96,13 @@ static void intel_dsm_platform_mux_info(acpi_handle 
> dhandle)
>   union acpi_object *obj = &pkg->package.elements[i];
>   union acpi_object *connector_id = &obj->package.elements[0];
>   union acpi_object *info = &obj->package.elements[1];
> +
> + if (obj->type != ACPI_TYPE_PACKAGE || obj->package.count < 2 ||
> + info->type != ACPI_TYPE_BUFFER || info->buffer.length < 4) {
> + DRM_DEBUG_DRIVER("Invalid object for MUX #%d\n", i);
> + continue;
> + }
> +
>   DRM_DEBUG_DRIVER("Connector id: 0x%016llx\n",
> (unsigned long long)connector_id->integer.value);
>   DRM_DEBUG_DRIVER("  port id: %s\n",
> -- 
> 2.26.2
> 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH v2] drm/i915: Fix invalid access to ACPI _DSM objects

[Takashi Iwai]

intel_dsm_platform_mux_info() tries to parse the ACPI package data
from _DSM for the debug information, but it assumes the fixed format
without checking what values are stored in the elements actually.
When an unexpected value is returned from BIOS, it may lead to GPF or
NULL dereference, as reported recently.

Add the checks of the contents in the returned values and skip the
values for invalid cases.

v1->v2: Check the info contents before dereferencing, too

BugLink: http://bugzilla.opensuse.org/show_bug.cgi?id=1184074
Cc: 
Signed-off-by: Takashi Iwai 
---
 drivers/gpu/drm/i915/display/intel_acpi.c | 22 --
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_acpi.c 
b/drivers/gpu/drm/i915/display/intel_acpi.c
index e21fb14d5e07..833d0c1be4f1 100644
--- a/drivers/gpu/drm/i915/display/intel_acpi.c
+++ b/drivers/gpu/drm/i915/display/intel_acpi.c
@@ -84,13 +84,31 @@ static void intel_dsm_platform_mux_info(acpi_handle dhandle)
return;
}
 
+   if (!pkg->package.count) {
+   DRM_DEBUG_DRIVER("no connection in _DSM\n");
+   return;
+   }
+
connector_count = &pkg->package.elements[0];
DRM_DEBUG_DRIVER("MUX info connectors: %lld\n",
  (unsigned long long)connector_count->integer.value);
for (i = 1; i < pkg->package.count; i++) {
union acpi_object *obj = &pkg->package.elements[i];
-   union acpi_object *connector_id = &obj->package.elements[0];
-   union acpi_object *info = &obj->package.elements[1];
+   union acpi_object *connector_id;
+   union acpi_object *info;
+
+   if (obj->type != ACPI_TYPE_PACKAGE || obj->package.count < 2) {
+   DRM_DEBUG_DRIVER("Invalid object for MUX #%d\n", i);
+   continue;
+   }
+
+   connector_id = &obj->package.elements[0];
+   info = &obj->package.elements[1];
+   if (info->type != ACPI_TYPE_BUFFER || info->buffer.length < 4) {
+   DRM_DEBUG_DRIVER("Invalid info for MUX obj #%d\n", i);
+   continue;
+   }
+
DRM_DEBUG_DRIVER("Connector id: 0x%016llx\n",
  (unsigned long long)connector_id->integer.value);
DRM_DEBUG_DRIVER("  port id: %s\n",
-- 
2.26.2

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2] drm/i915: Fix invalid access to ACPI _DSM objects

[Takashi Iwai]

On Wed, 07 Apr 2021 18:34:46 +0200,
Ville Syrjälä wrote:
> 
> On Fri, Apr 02, 2021 at 10:23:17AM +0200, Takashi Iwai wrote:
> > intel_dsm_platform_mux_info() tries to parse the ACPI package data
> > from _DSM for the debug information, but it assumes the fixed format
> > without checking what values are stored in the elements actually.
> > When an unexpected value is returned from BIOS, it may lead to GPF or
> > NULL dereference, as reported recently.
> > 
> > Add the checks of the contents in the returned values and skip the
> > values for invalid cases.
> > 
> > v1->v2: Check the info contents before dereferencing, too
> > 
> > BugLink: http://bugzilla.opensuse.org/show_bug.cgi?id=1184074
> > Cc: 
> > Signed-off-by: Takashi Iwai 
> > ---
> >  drivers/gpu/drm/i915/display/intel_acpi.c | 22 --
> >  1 file changed, 20 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_acpi.c 
> > b/drivers/gpu/drm/i915/display/intel_acpi.c
> > index e21fb14d5e07..833d0c1be4f1 100644
> > --- a/drivers/gpu/drm/i915/display/intel_acpi.c
> > +++ b/drivers/gpu/drm/i915/display/intel_acpi.c
> > @@ -84,13 +84,31 @@ static void intel_dsm_platform_mux_info(acpi_handle 
> > dhandle)
> > return;
> > }
> >  
> > +   if (!pkg->package.count) {
> > +   DRM_DEBUG_DRIVER("no connection in _DSM\n");
> > +   return;
> > +   }
> > +
> > connector_count = &pkg->package.elements[0];
> > DRM_DEBUG_DRIVER("MUX info connectors: %lld\n",
> >   (unsigned long long)connector_count->integer.value);
> > for (i = 1; i < pkg->package.count; i++) {
> > union acpi_object *obj = &pkg->package.elements[i];
> > -   union acpi_object *connector_id = &obj->package.elements[0];
> > -   union acpi_object *info = &obj->package.elements[1];
> > +   union acpi_object *connector_id;
> > +   union acpi_object *info;
> > +
> > +   if (obj->type != ACPI_TYPE_PACKAGE || obj->package.count < 2) {
> > +   DRM_DEBUG_DRIVER("Invalid object for MUX #%d\n", i);
> > +   continue;
> > +   }
> > +
> > +   connector_id = &obj->package.elements[0];
> 
> You don't want to check connector_id->type as well?

I added only the minimal checks that may lead to Oops.


Takashi

> 
> > +   info = &obj->package.elements[1];
> > +   if (info->type != ACPI_TYPE_BUFFER || info->buffer.length < 4) {
> > +   DRM_DEBUG_DRIVER("Invalid info for MUX obj #%d\n", i);
> > +   continue;
> > +   }
> > +
> > DRM_DEBUG_DRIVER("Connector id: 0x%016llx\n",
> >   (unsigned long long)connector_id->integer.value);
> > DRM_DEBUG_DRIVER("  port id: %s\n",
> > -- 
> > 2.26.2
> > 
> > ___
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel
> 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2] drm/i915: Fix invalid access to ACPI _DSM objects

[Takashi Iwai]

On Wed, 07 Apr 2021 23:28:48 +0200,
Ville Syrjälä wrote:
> 
> On Wed, Apr 07, 2021 at 06:56:15PM +0200, Takashi Iwai wrote:
> > On Wed, 07 Apr 2021 18:34:46 +0200,
> > Ville Syrjälä wrote:
> > > 
> > > On Fri, Apr 02, 2021 at 10:23:17AM +0200, Takashi Iwai wrote:
> > > > intel_dsm_platform_mux_info() tries to parse the ACPI package data
> > > > from _DSM for the debug information, but it assumes the fixed format
> > > > without checking what values are stored in the elements actually.
> > > > When an unexpected value is returned from BIOS, it may lead to GPF or
> > > > NULL dereference, as reported recently.
> > > > 
> > > > Add the checks of the contents in the returned values and skip the
> > > > values for invalid cases.
> > > > 
> > > > v1->v2: Check the info contents before dereferencing, too
> > > > 
> > > > BugLink: http://bugzilla.opensuse.org/show_bug.cgi?id=1184074
> > > > Cc: 
> > > > Signed-off-by: Takashi Iwai 
> > > > ---
> > > >  drivers/gpu/drm/i915/display/intel_acpi.c | 22 --
> > > >  1 file changed, 20 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_acpi.c 
> > > > b/drivers/gpu/drm/i915/display/intel_acpi.c
> > > > index e21fb14d5e07..833d0c1be4f1 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_acpi.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_acpi.c
> > > > @@ -84,13 +84,31 @@ static void intel_dsm_platform_mux_info(acpi_handle 
> > > > dhandle)
> > > > return;
> > > > }
> > > >  
> > > > +   if (!pkg->package.count) {
> > > > +   DRM_DEBUG_DRIVER("no connection in _DSM\n");
> > > > +   return;
> > > > +   }
> > > > +
> > > > connector_count = &pkg->package.elements[0];
> > > > DRM_DEBUG_DRIVER("MUX info connectors: %lld\n",
> > > >   (unsigned long long)connector_count->integer.value);
> > > > for (i = 1; i < pkg->package.count; i++) {
> > > > union acpi_object *obj = &pkg->package.elements[i];
> > > > -   union acpi_object *connector_id = 
> > > > &obj->package.elements[0];
> > > > -   union acpi_object *info = &obj->package.elements[1];
> > > > +   union acpi_object *connector_id;
> > > > +   union acpi_object *info;
> > > > +
> > > > +   if (obj->type != ACPI_TYPE_PACKAGE || 
> > > > obj->package.count < 2) {
> > > > +   DRM_DEBUG_DRIVER("Invalid object for MUX 
> > > > #%d\n", i);
> > > > +   continue;
> > > > +   }
> > > > +
> > > > +   connector_id = &obj->package.elements[0];
> > > 
> > > You don't want to check connector_id->type as well?
> > 
> > I added only the minimal checks that may lead to Oops.
> 
> OK. I guess misinterpreting something else as an integer isn't
> particular dangerous in this case.
> 
> Pushed to drm-intel-next. Thanks.

Great, thanks!

> Oh, could you ask the bug reporter to attach an acpidump to the
> bug? Might be good to have that stuff on record somewhere if/when
> someone wants to actually figure out what's going on here.

OK, I'll ask.

> That said, maybe we should just nuke this whole thing instead?
> Unless I'm missing someting this code doesn't seem to actually
> do anything...

Yeah, that looks nothing but showing the debug information and that
can be checked via acpidump output, too...


Takashi
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2] drm/i915: Fix invalid access to ACPI _DSM objects

[Takashi Iwai]

On Thu, 08 Apr 2021 09:51:18 +0200,
Takashi Iwai wrote:
> 
> On Wed, 07 Apr 2021 23:28:48 +0200,
> Ville Syrjälä wrote:
> > 
> > Oh, could you ask the bug reporter to attach an acpidump to the
> > bug? Might be good to have that stuff on record somewhere if/when
> > someone wants to actually figure out what's going on here.
> 
> OK, I'll ask.

Available at
  acpidump: https://susepaste.org/59777448
  hwinfo: https://susepaste.org/86284020


thanks,

Takashi
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2] drm/i915: Fix invalid access to ACPI _DSM objects

[Takashi Iwai]

On Thu, 08 Apr 2021 18:56:06 +0200,
Ville Syrjälä wrote:
> 
> On Thu, Apr 08, 2021 at 06:34:06PM +0200, Takashi Iwai wrote:
> > On Thu, 08 Apr 2021 09:51:18 +0200,
> > Takashi Iwai wrote:
> > > 
> > > On Wed, 07 Apr 2021 23:28:48 +0200,
> > > Ville Syrjälä wrote:
> > > > 
> > > > Oh, could you ask the bug reporter to attach an acpidump to the
> > > > bug? Might be good to have that stuff on record somewhere if/when
> > > > someone wants to actually figure out what's going on here.
> > > 
> > > OK, I'll ask.
> > 
> > Available at
> >   acpidump: https://susepaste.org/59777448
> >   hwinfo: https://susepaste.org/86284020
> 
> Those will apparently expire real soon. I took local copies out
> of morbid curiosity, but that's not going to help anyone else 
> in the future. I rather wish bug reporting tools would flat out
> refuse to accept any pastebin links.

Yeah, don't worry, I'll upload them to Bugzilla later in anyway.


Takashi
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 3/3] snd/hda: Protect concurrent display_power_status with a mutex

[Takashi Iwai]

On Mon, 14 Jan 2019 18:37:53 +0100,
Chris Wilson wrote:
> 
> Just in case the audio linkage is swapped between components during the
> runtime pm sequence, we need to protect the rpm tracking with a mutex.

It's not clear to me how does this happens.
Could you elaborate a bit more the scenario?


thanks,

Takashi

> 
> Signed-off-by: Chris Wilson 
> Cc: Takashi Iwai 
> Cc: Jani Nikula 
> ---
>  include/sound/hdaudio.h| 1 +
>  sound/hda/hdac_component.c | 7 +++
>  2 files changed, 8 insertions(+)
> 
> diff --git a/include/sound/hdaudio.h b/include/sound/hdaudio.h
> index 39761120ee76..497335b24e18 100644
> --- a/include/sound/hdaudio.h
> +++ b/include/sound/hdaudio.h
> @@ -368,6 +368,7 @@ struct hdac_bus {
>   struct drm_audio_component *audio_component;
>   unsigned long display_power_status;
>   unsigned long display_power_active;
> + struct mutex display_power_lock;
>  
>   /* parameters required for enhanced capabilities */
>   int num_streams;
> diff --git a/sound/hda/hdac_component.c b/sound/hda/hdac_component.c
> index 2702548b788a..ea76c1de2927 100644
> --- a/sound/hda/hdac_component.c
> +++ b/sound/hda/hdac_component.c
> @@ -77,6 +77,7 @@ void snd_hdac_display_power(struct hdac_bus *bus, unsigned 
> int idx, bool enable)
>   if (!acomp || !acomp->ops)
>   return;
>  
> + mutex_lock(&bus->display_power_lock);
>   if (bus->display_power_status) {
>   if (!bus->display_power_active) {
>   unsigned long cookie = -1;
> @@ -98,6 +99,7 @@ void snd_hdac_display_power(struct hdac_bus *bus, unsigned 
> int idx, bool enable)
>   bus->display_power_active = 0;
>   }
>   }
> + mutex_unlock(&bus->display_power_lock);
>  }
>  EXPORT_SYMBOL_GPL(snd_hdac_display_power);
>  
> @@ -290,6 +292,9 @@ int snd_hdac_acomp_init(struct hdac_bus *bus,
>GFP_KERNEL);
>   if (!acomp)
>   return -ENOMEM;
> +
> + mutex_init(&bus->display_power_lock);
> +
>   acomp->audio_ops = aops;
>   bus->audio_component = acomp;
>   devres_add(dev, acomp);
> @@ -336,6 +341,8 @@ int snd_hdac_acomp_exit(struct hdac_bus *bus)
>   bus->display_power_active = 0;
>   bus->display_power_status = 0;
>  
> + mutex_destroy(&bus->display_power_lock);
> +
>   component_master_del(dev, &hdac_component_master_ops);
>  
>   bus->audio_component = NULL;
> -- 
> 2.20.1
> 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 2/3] snd/hda: Track the display_power_status using a cookie

[Takashi Iwai]

On Mon, 14 Jan 2019 18:37:52 +0100,
Chris Wilson wrote:
> 
> drm/i915 is tracking all wakeref owners with a cookie in order to
> identify leaks. To that end, each rpm acquisition ops->get_power is
> assigned a cookie which should be passed to ops->put_power to signify
> its release (and removal from the list of wakeref owners). As snd/hda is
> already using a bool to track current status of display_power extending
> that to an unsigned long to hold the boolean cookie is a trivial
> extension, and will quell all doubt that snd/hda is the cause of the
> device runtime pm leaks.
> 
> Signed-off-by: Chris Wilson 
> Cc: Takashi Iwai 
> Cc: Jani Nikula 
> ---
>  drivers/gpu/drm/i915/intel_audio.c | 10 +-
>  include/drm/drm_audio_component.h  |  7 +--
>  include/sound/hdaudio.h|  4 ++--
>  sound/hda/hdac_component.c | 18 --
>  4 files changed, 24 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_audio.c 
> b/drivers/gpu/drm/i915/intel_audio.c
> index 92e27359c2e3..efc5fb3c2161 100644
> --- a/drivers/gpu/drm/i915/intel_audio.c
> +++ b/drivers/gpu/drm/i915/intel_audio.c
> @@ -741,15 +741,15 @@ void intel_init_audio_hooks(struct drm_i915_private 
> *dev_priv)
>   }
>  }
>  
> -static void i915_audio_component_get_power(struct device *kdev)
> +static unsigned long i915_audio_component_get_power(struct device *kdev)
>  {
> - intel_display_power_get(kdev_to_i915(kdev), POWER_DOMAIN_AUDIO);
> + return intel_display_power_get(kdev_to_i915(kdev), POWER_DOMAIN_AUDIO);
>  }
>  
> -static void i915_audio_component_put_power(struct device *kdev)
> +static void i915_audio_component_put_power(struct device *kdev,
> +unsigned long cookie)
>  {
> - intel_display_power_put_unchecked(kdev_to_i915(kdev),
> -   POWER_DOMAIN_AUDIO);
> + intel_display_power_put(kdev_to_i915(kdev), POWER_DOMAIN_AUDIO, cookie);
>  }
>  
>  static void i915_audio_component_codec_wake_override(struct device *kdev,
> diff --git a/include/drm/drm_audio_component.h 
> b/include/drm/drm_audio_component.h
> index 4923b00328c1..d0c7444319f5 100644
> --- a/include/drm/drm_audio_component.h
> +++ b/include/drm/drm_audio_component.h
> @@ -18,14 +18,17 @@ struct drm_audio_component_ops {
>* @get_power: get the POWER_DOMAIN_AUDIO power well
>*
>* Request the power well to be turned on.
> +  *
> +  * Returns a wakeref cookie to be passed back to the corresponding
> +  * call to @put_power.

Better to explain that the cookie should be non-zero for active
state.


>*/
> - void (*get_power)(struct device *);
> + unsigned long (*get_power)(struct device *);
>   /**
>* @put_power: put the POWER_DOMAIN_AUDIO power well
>*
>* Allow the power well to be turned off.
>*/
> - void (*put_power)(struct device *);
> + void (*put_power)(struct device *, unsigned long);
>   /**
>* @codec_wake_override: Enable/disable codec wake signal
>*/
> diff --git a/include/sound/hdaudio.h b/include/sound/hdaudio.h
> index b4fa1c775251..39761120ee76 100644
> --- a/include/sound/hdaudio.h
> +++ b/include/sound/hdaudio.h
> @@ -366,8 +366,8 @@ struct hdac_bus {
>  
>   /* DRM component interface */
>   struct drm_audio_component *audio_component;
> - long display_power_status;
> - bool display_power_active;
> + unsigned long display_power_status;

You don't need to change this field.  It's irrelevant with this patch
itself.


thanks,

Takashi


> + unsigned long display_power_active;
>  
>   /* parameters required for enhanced capabilities */
>   int num_streams;
> diff --git a/sound/hda/hdac_component.c b/sound/hda/hdac_component.c
> index a6d37b9d6413..2702548b788a 100644
> --- a/sound/hda/hdac_component.c
> +++ b/sound/hda/hdac_component.c
> @@ -79,17 +79,23 @@ void snd_hdac_display_power(struct hdac_bus *bus, 
> unsigned int idx, bool enable)
>  
>   if (bus->display_power_status) {
>   if (!bus->display_power_active) {
> + unsigned long cookie = -1;
> +
>   if (acomp->ops->get_power)
> - acomp->ops->get_power(acomp->dev);
> + cookie = acomp->ops->get_power(acomp->dev);
> +
>   snd_hdac_set_codec_wakeup(bus, true);
>   snd_hdac_set_codec_wakeup(bus, false);
> - bus->display_power_active = true;
> + bus-&g

Re: [Intel-gfx] [PATCH 3/3] snd/hda: Protect concurrent display_power_status with a mutex

[Takashi Iwai]

On Mon, 14 Jan 2019 18:51:31 +0100,
Chris Wilson wrote:
> 
> Quoting Takashi Iwai (2019-01-14 17:46:57)
> > On Mon, 14 Jan 2019 18:37:53 +0100,
> > Chris Wilson wrote:
> > > 
> > > Just in case the audio linkage is swapped between components during the
> > > runtime pm sequence, we need to protect the rpm tracking with a mutex.
> > 
> > It's not clear to me how does this happens.
> > Could you elaborate a bit more the scenario?
> 
> The code is written such that multiple bits within display_power_status
> can be set and cleared simultaneously. There was no serialisation
> mentioned in the routine, so I was fearful that the display_power_active
> here was being accessed concurrently -- and if that was explaining why
> snd/hda appears to be leaking the runtime pm (or at least is holding on
> to the wakeref longer than igt expects, > 10s).

Aha, and does patch actually "fix" the issue...?

It's a simple mutex addition, so no big show, and I'm fine with it,
but just wonder whether it really helped.


thanks,

Takashi
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 3/3] snd/hda: Protect concurrent display_power_status with a mutex

[Takashi Iwai]

On Mon, 14 Jan 2019 21:57:15 +0100,
Chris Wilson wrote:
> 
> Quoting Takashi Iwai (2019-01-14 18:00:02)
> > On Mon, 14 Jan 2019 18:51:31 +0100,
> > Chris Wilson wrote:
> > > 
> > > Quoting Takashi Iwai (2019-01-14 17:46:57)
> > > > On Mon, 14 Jan 2019 18:37:53 +0100,
> > > > Chris Wilson wrote:
> > > > > 
> > > > > Just in case the audio linkage is swapped between components during 
> > > > > the
> > > > > runtime pm sequence, we need to protect the rpm tracking with a mutex.
> > > > 
> > > > It's not clear to me how does this happens.
> > > > Could you elaborate a bit more the scenario?
> > > 
> > > The code is written such that multiple bits within display_power_status
> > > can be set and cleared simultaneously. There was no serialisation
> > > mentioned in the routine, so I was fearful that the display_power_active
> > > here was being accessed concurrently -- and if that was explaining why
> > > snd/hda appears to be leaking the runtime pm (or at least is holding on
> > > to the wakeref longer than igt expects, > 10s).
> > 
> > Aha, and does patch actually "fix" the issue...?
> 
> Not sure yet; it's a temperamental issue in CI and my chief suspect is that
> it's just a misconfiguration of the rpm autosuspend. What may point
> towards the scenario above is if i915.ko module unloading has any impact
> on it.
>  
> > It's a simple mutex addition, so no big show, and I'm fine with it,
> > but just wonder whether it really helped.
> 
> Yeah, it just looked suspicious when reviewing the code, will throw it
> at CI a few times to see if it sticks.

OK, would you re-submit the patchset if it's confirmed to fix the
issue?  Then we should mark it Cc-to-stable as a real fix, at least.


thanks,

Takashi
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] Timing issues between ALSA and i915 drivers

[Takashi Iwai]

On Wed, 16 Jan 2019 18:48:25 +0100,
Pierre-Louis Bossart wrote:
> 
> Hi,
> 
> I could use some feedback on HDMI audio issues exposed during the 4.21
> merge window. By accident (misleading documentation) we ended up
> enabling the Skylake driver instead of the HDaudio legacy, and broke
> audio on a number of Skylake and ApolloLake devices where the
> HDMI/iDISP codec was not detected (bit 2 not set in the
> codec_mask). Linus' Dell XPS13 9350 was the first to be impacted of
> course...
> 
> After debugging a bit, this issue can be resolved by either
> 
> a) compiling both SOUND and DRM as built-ins (y instead of m)
> 
> b) moving the calls snd_hdac_i915_init() to the probe function instead
> of the worker queue (code at
> https://github.com/plbossart/sound/commits/fix/skl-hdmi)

This isn't guaranteed to work, as request_module() might be involved,
I'm afraid.

> Both solutions point to timing issues.
> 
> During internal reviews I was alerted to the fact that the suggested
> fix essentially reverts patch ab1b732d53c18 ('ASoC: Intel: Skylake:
> Move i915 registration to worker thread') which was introduced to
> solve DRM lockup issues.
> 
> In other words, we are at a point where we either have DRM lockups or
> can't detect the HDMI audio codec, none of which are too good.
> 
> I don't have the background for the DRM lockup stuff, nor do I
> understand why snd_hdac_i915_init has to be called from a thread
> context. Is this really a requirement?
> 
> I also don't get what sort of timing issues would come from an
> initialization. What happens on the i915 side and is there some sort
> of mandatory delay between the completion of the i915_init and issuing
> a snd_hdac_chip_readw(bus, STATESTS) to get the codec masks on the
> HDaudio links?

snd_hdac_i915_init() waits for the binding with the i915 audio
component, so a possible solution would be just to delay the audio
component registration at the really last stage, like the change
below.

If this still doesn't work, it'll be more deeply inside, and I have
little clue for now...


thanks,

Takashi

---
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1577,8 +1577,6 @@ static void i915_driver_register(struct drm_i915_private 
*dev_priv)
if (IS_GEN5(dev_priv))
intel_gpu_ips_init(dev_priv);
 
-   intel_audio_init(dev_priv);
-
/*
 * Some ports require correctly set-up hpd registers for detection to
 * work properly (leading to ghost connected connector status), e.g. VGA
@@ -1597,6 +1595,8 @@ static void i915_driver_register(struct drm_i915_private 
*dev_priv)
 
intel_power_domains_enable(dev_priv);
intel_runtime_pm_enable(dev_priv);
+
+   intel_audio_init(dev_priv);
 }
 
 /**
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [alsa-devel] Timing issues between ALSA and i915 drivers

[Takashi Iwai]

On Thu, 17 Jan 2019 20:53:06 +0100,
Pierre-Louis Bossart wrote:
> 
> 
> > I could use some feedback on HDMI audio issues exposed during the
> > 4.21 merge window. By accident (misleading documentation) we ended
> > up enabling the Skylake driver instead of the HDaudio legacy, and
> > broke audio on a number of Skylake and ApolloLake devices where the
> > HDMI/iDISP codec was not detected (bit 2 not set in the
> > codec_mask). Linus' Dell XPS13 9350 was the first to be impacted of
> > course...
> >
> > After debugging a bit, this issue can be resolved by either
> >
> > a) compiling both SOUND and DRM as built-ins (y instead of m)
> >
> > b) moving the calls snd_hdac_i915_init() to the probe function
> > instead of the worker queue (code at
> > https://github.com/plbossart/sound/commits/fix/skl-hdmi)
> >
> > Both solutions point to timing issues.
> >
> > During internal reviews I was alerted to the fact that the suggested
> > fix essentially reverts patch ab1b732d53c18 ('ASoC: Intel: Skylake:
> > Move i915 registration to worker thread') which was introduced to
> > solve DRM lockup issues.
> 
> I tried to narrow down the issue further and my current understanding
> is that the Skylake driver performs link reset operations without the
> display power turned on - which does not look like a very smart thing
> to do in hindsight.
> 
> In other words, it's not really when snd_hdac_i915_init() is called
> that matters as I assumed initially, but more when
> snd_hdac_display_power() is invoked. There are two cases where this
> happens, and for each of them turning the display power on results in
> HDMI detection. The attached diffs split the initialization from the
> power on, which provides a better understanding of the issue.

OK, this makes some sense, and that's the very reason we have
HDA_CODEC_IDX_CONTROLLER for snd_hdac_display_power().  IIRC, we
needed to power on the display for probing of the legacy HDA, too.
Once after that, for the normal operation, the display power is needed
only when you output the HDMI stream.


> What would be really useful at this point is a confirmation that
> snd_hdac_i915_init() cannot be called in the initial probe but does
> need to be executed in a work queue. That would really impact the way
> the initialization sequence is reworked on the Skylake side as well as
> modify the way the SOF driver deals with i915 initialization.

It's needed to be called in a work queue, yes.

Basically you shouldn't call request_module() in the driver's probe
callback.  When the probe callback is called from the module loading,
it blocks the module loading itself, hence loading yet another module
can't work.  A situation might be easier than the past (which
deadlocked), but still it's advised to use either the
request_module_nowait() with the callback or call request_module()
asynchronously from probe.


thanks,

Takashi

> 
> Thanks
> 
> -Pierre
> 
> diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c
> index 60c94836bf5b..56556d06a17f 100644
> --- a/sound/soc/intel/skylake/skl.c
> +++ b/sound/soc/intel/skylake/skl.c
> @@ -767,23 +767,6 @@ static const struct hdac_bus_ops bus_core_ops = {
>   .get_response = snd_hdac_bus_get_response,
>  };
>  
> -static int skl_i915_init(struct hdac_bus *bus)
> -{
> - int err;
> -
> - /*
> -  * The HDMI codec is in GPU so we need to ensure that it is powered
> -  * up and ready for probe
> -  */
> - err = snd_hdac_i915_init(bus);
> - if (err < 0)
> - return err;
> -
> - snd_hdac_display_power(bus, HDA_CODEC_IDX_CONTROLLER, true);
> -
> - return 0;
> -}
> -
>  static void skl_probe_work(struct work_struct *work)
>  {
>   struct skl *skl = container_of(work, struct skl, probe_work);
> @@ -791,12 +774,6 @@ static void skl_probe_work(struct work_struct *work)
>   struct hdac_ext_link *hlink = NULL;
>   int err;
>  
> - if (IS_ENABLED(CONFIG_SND_SOC_HDAC_HDMI)) {
> - err = skl_i915_init(bus);
> - if (err < 0)
> - return;
> - }
> -
>   err = skl_init_chip(bus, true);
>   if (err < 0) {
>   dev_err(bus->dev, "Init chip failed with err: %d\n", err);
> @@ -899,6 +876,11 @@ static int skl_first_init(struct hdac_bus *bus)
>   unsigned short gcap;
>   int cp_streams, pb_streams, start_idx;
>  
> + err = snd_hdac_i915_init(bus);
> + if (err < 0)
> + return err;
> +
> +
>   err = pci_request_regions(pci, "Skylake HD audio");
>   if (err < 0)
>   return err;
> @@ -910,7 +892,10 @@ static int skl_first_init(struct hdac_bus *bus)
>   return -ENXIO;
>   }
>  
> - snd_hdac_bus_reset_link(bus, true);
> + /* this bus_reset_link is unnecessary, and without the display
> +  *  power turned on prevents HDMI from being detected
> +  */
> + //snd_hdac_bus_reset_link(bus, true);
>  
>   snd_hdac_bus_parse_capabilities(bus);
>  
> @@ -962,7 +

Re: [Intel-gfx] [alsa-devel] Timing issues between ALSA and i915 drivers

[Takashi Iwai]

On Thu, 17 Jan 2019 21:47:05 +0100,
Pierre-Louis Bossart wrote:
> 
> 
> >> I tried to narrow down the issue further and my current understanding
> >> is that the Skylake driver performs link reset operations without the
> >> display power turned on - which does not look like a very smart thing
> >> to do in hindsight.
> >>
> >> In other words, it's not really when snd_hdac_i915_init() is called
> >> that matters as I assumed initially, but more when
> >> snd_hdac_display_power() is invoked. There are two cases where this
> >> happens, and for each of them turning the display power on results in
> >> HDMI detection. The attached diffs split the initialization from the
> >> power on, which provides a better understanding of the issue.
> > OK, this makes some sense, and that's the very reason we have
> > HDA_CODEC_IDX_CONTROLLER for snd_hdac_display_power().  IIRC, we
> > needed to power on the display for probing of the legacy HDA, too.
> > Once after that, for the normal operation, the display power is needed
> > only when you output the HDMI stream.
> >
> >
> >> What would be really useful at this point is a confirmation that
> >> snd_hdac_i915_init() cannot be called in the initial probe but does
> >> need to be executed in a work queue. That would really impact the way
> >> the initialization sequence is reworked on the Skylake side as well as
> >> modify the way the SOF driver deals with i915 initialization.
> > It's needed to be called in a work queue, yes.
> >
> > Basically you shouldn't call request_module() in the driver's probe
> > callback.  When the probe callback is called from the module loading,
> > it blocks the module loading itself, hence loading yet another module
> > can't work.  A situation might be easier than the past (which
> > deadlocked), but still it's advised to use either the
> > request_module_nowait() with the callback or call request_module()
> > asynchronously from probe.
> 
> Thanks Takashi, this is very useful. I guess that will require a
> complete rework of the Skylake initialization sequence then, my simple
> code translation isn't enough indeed and the current partition between
> probe/work queue can't comply with both requirements (request module
> asynchronously from probe, display turned on before mucking with
> links).
> 
> We also need this changed for SOF, the i915_init is done in the probe.

Well, snd_hdac_i915_init() itself also calls already request_module()
if i915 is missing, so this should be done in the async code, too...

The complication comes from the fact that HD-audio driver doesn't
necessarily bind with i915 graphics.  Otherwise we could have a fixed
hard dependency that assures more or less i915 probing before
HD-audio (though, i915 probing became async, so it's now harder).


thanks,

Takashi
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 5/6] drm/i915: Expose RPCS (SSEU) configuration to userspace (Gen11 only)

[Takashi Iwai]

or from i915_sw_fence_await_sw_fence_gfp. (Chris Wilson)
> 
> v18:
>  * Update commit message. (Joonas)
>  * Restrict uAPI to VME use case. (Joonas)
> 
> v19:
>  * Rebase.
> 
> v20:
>  * Rebase for ce->active_tracker.
> 
> v21:
>  * Rebase for IS_GEN changes.
> 
> v22:
>  * Reserve uAPI for flags straight away. (Chris Wilson)
> 
> v23:
>  * Rebase for RUNTIME_INFO.
> 
> v24:
>  * Added some headline docs for the uapi usage. (Joonas/Chris)
> 
> v25:
>  * Renamed class/instance to engine_class/engine_instance to avoid clash
>with C++ keyword. (Tony Ye)
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=100899
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107634
> Issue: https://github.com/intel/media-driver/issues/267
> Signed-off-by: Chris Wilson 
> Signed-off-by: Lionel Landwerlin 
> Cc: Dmitry Rogozhkin 
> Cc: Tvrtko Ursulin 
> Cc: Zhipeng Gong 
> Cc: Joonas Lahtinen 
> Cc: Tony Ye 
> Signed-off-by: Tvrtko Ursulin 
> Reviewed-by: Chris Wilson  # v21
> Reviewed-by: Joonas Lahtinen 

The change looks good to me.
Feel free to take my ack
  Acked-by: Takashi Iwai 


thanks,

Takashi
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] snd/hda: Balance hda->need_i915_power across runtime_suspend

[Takashi Iwai]

On Tue, 09 Apr 2019 23:27:41 +0200,
Chris Wilson wrote:
> 
> In runtime_resume, we release the local display_power wakeref if we can
> rely on i915 providing a wakeref along the component. On suspend
> therefore, we should only release the display_power if we kept it from
> runtime_resume.

Hrm, it shouldn't matter.  After the recent code rewrite, the control
isn't refcount any longer, hence it's fine to call
display_power(false) again even if it were already powered off.


thanks,

Takashi

> 
> Fixes: e454ff8e89b6 ("ALSA: hda/intel: Drop superfluous 
> AZX_DCAPS_I915_POWERWELL checks")
> Signed-off-by: Chris Wilson 
> Cc: Takashi Iwai 
> Cc: Imre Deak 
> ---
> This appears to fix the glk-dsi as it performs a pm_runtime_suspend in
> the middle of azx_probe_contime(). Hopefully.
> ---
>  sound/pci/hda/hda_intel.c | 6 +-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> index 2ec91085fa3e..a9faf95c88b5 100644
> --- a/sound/pci/hda/hda_intel.c
> +++ b/sound/pci/hda/hda_intel.c
> @@ -941,10 +941,14 @@ static bool azx_is_pm_ready(struct snd_card *card)
>  
>  static void __azx_runtime_suspend(struct azx *chip)
>  {
> + struct hda_intel *hda = container_of(chip, struct hda_intel, chip);
> +
>   azx_stop_chip(chip);
>   azx_enter_link_reset(chip);
>   azx_clear_irq_pending(chip);
> - display_power(chip, false);
> +
> + if (hda->need_i915_power)
> + display_power(chip, false);
>  }
>  
>  static void __azx_runtime_resume(struct azx *chip, bool from_rt)
> -- 
> 2.20.1
> 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] snd/hda: Balance hda->need_i915_power across runtime_suspend

[Takashi Iwai]

On Wed, 10 Apr 2019 00:53:31 +0200,
Chris Wilson wrote:
> 
> Quoting Takashi Iwai (2019-04-09 22:35:28)
> > On Tue, 09 Apr 2019 23:27:41 +0200,
> > Chris Wilson wrote:
> > > 
> > > In runtime_resume, we release the local display_power wakeref if we can
> > > rely on i915 providing a wakeref along the component. On suspend
> > > therefore, we should only release the display_power if we kept it from
> > > runtime_resume.
> > 
> > Hrm, it shouldn't matter.  After the recent code rewrite, the control
> > isn't refcount any longer, hence it's fine to call
> > display_power(false) again even if it were already powered off.
> 
> That is the puzzle. Have a look at the glk-dsi results,
> https://patchwork.freedesktop.org/series/59253/
> something does appear to go wrong in azx_probe_continue (and seems to be
> avoided by this patch).
> 
> Perhaps something like 
> https://patchwork.freedesktop.org/patch/297656/?series=59257&rev=1
> if the pm_runtime_autosuspend is occurring from a workqueue at the same
> time as we call display_power(false).

Then how about rather a patch like below?


thanks,

Takashi

--- a/sound/hda/hdac_component.c
+++ b/sound/hda/hdac_component.c
@@ -78,18 +78,16 @@ void snd_hdac_display_power(struct hdac_bus *bus, unsigned 
int idx, bool enable)
return;
 
if (bus->display_power_status) {
-   if (!bus->display_power_active) {
+   if (!cmpxchg(&bus->display_power_active, false, true)) {
if (acomp->ops->get_power)
acomp->ops->get_power(acomp->dev);
snd_hdac_set_codec_wakeup(bus, true);
snd_hdac_set_codec_wakeup(bus, false);
-   bus->display_power_active = true;
}
} else {
-   if (bus->display_power_active) {
+   if (cmpxchg(&bus->display_power_active, true, false)) {
if (acomp->ops->put_power)
acomp->ops->put_power(acomp->dev);
-   bus->display_power_active = false;
}
}
 }
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] snd/hda: Balance hda->need_i915_power across runtime_suspend

[Takashi Iwai]

On Wed, 10 Apr 2019 09:59:19 +0200,
Chris Wilson wrote:
> 
> Quoting Takashi Iwai (2019-04-10 06:29:07)
> > On Wed, 10 Apr 2019 00:53:31 +0200,
> > Chris Wilson wrote:
> > > 
> > > Quoting Takashi Iwai (2019-04-09 22:35:28)
> > > > On Tue, 09 Apr 2019 23:27:41 +0200,
> > > > Chris Wilson wrote:
> > > > > 
> > > > > In runtime_resume, we release the local display_power wakeref if we 
> > > > > can
> > > > > rely on i915 providing a wakeref along the component. On suspend
> > > > > therefore, we should only release the display_power if we kept it from
> > > > > runtime_resume.
> > > > 
> > > > Hrm, it shouldn't matter.  After the recent code rewrite, the control
> > > > isn't refcount any longer, hence it's fine to call
> > > > display_power(false) again even if it were already powered off.
> > > 
> > > That is the puzzle. Have a look at the glk-dsi results,
> > > https://patchwork.freedesktop.org/series/59253/
> > > something does appear to go wrong in azx_probe_continue (and seems to be
> > > avoided by this patch).
> > > 
> > > Perhaps something like 
> > > https://patchwork.freedesktop.org/patch/297656/?series=59257&rev=1
> > > if the pm_runtime_autosuspend is occurring from a workqueue at the same
> > > time as we call display_power(false).
> > 
> > Then how about rather a patch like below?
> > 
> > 
> > thanks,
> > 
> > Takashi
> > 
> > --- a/sound/hda/hdac_component.c
> > +++ b/sound/hda/hdac_component.c
> > @@ -78,18 +78,16 @@ void snd_hdac_display_power(struct hdac_bus *bus, 
> > unsigned int idx, bool enable)
> > return;
> >  
> > if (bus->display_power_status) {
> > -   if (!bus->display_power_active) {
> > +   if (!cmpxchg(&bus->display_power_active, false, true)) {
> 
> Except that display_power_active is the wakeref cookie returned by
> get_power to be passed back to put_power.

My patch is for 5.1.

> It seems that the cmpxchg is happy so we can conclude this is a race
> between display_power(false) and pm_runtime_suspend.

I believe it worth to fix this in 5.1 at first.  Let me submit and
queue the fix for 5.1-rc5.


thanks,

Takashi
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] snd/hda: Only get/put display_power once

[Takashi Iwai]

On Wed, 10 Apr 2019 10:17:33 +0200,
Chris Wilson wrote:
> 
> While we only allow a single display power reference, the current
> acquisition/release is racy and a direct call may run concurrently with
> a runtime-pm worker. Prevent the double unreference by atomically
> tracking the display_power_active cookie.
> 
> Testcase: igt/i915_pm_rpm/module-reload #glk-dsi
> Signed-off-by: Chris Wilson 
> Cc: Takashi Iwai 
> Cc: Imre Deak 

I rather prefer a more straightforward conversion, e.g. something like
below.  Checking the returned cookie as the state flag is not quite
intuitive, so revive the boolean state flag, and handle it
atomically.


thanks,

Takashi

--- a/include/sound/hdaudio.h
+++ b/include/sound/hdaudio.h
@@ -367,7 +367,8 @@ struct hdac_bus {
/* DRM component interface */
struct drm_audio_component *audio_component;
long display_power_status;
-   unsigned long display_power_active;
+   bool display_power_active;
+   unsigned long display_cookie;
 
/* parameters required for enhanced capabilities */
int num_streams;
diff --git a/sound/hda/hdac_component.c b/sound/hda/hdac_component.c
index 13915fdc6a54..da20f439578a 100644
--- a/sound/hda/hdac_component.c
+++ b/sound/hda/hdac_component.c
@@ -78,24 +78,19 @@ void snd_hdac_display_power(struct hdac_bus *bus, unsigned 
int idx, bool enable)
return;
 
if (bus->display_power_status) {
-   if (!bus->display_power_active) {
-   unsigned long cookie = -1;
-
+   if (!cmpxchg(&bus->display_power_active, false, true)) {
if (acomp->ops->get_power)
-   cookie = acomp->ops->get_power(acomp->dev);
+   bus->display_cookie =
+   acomp->ops->get_power(acomp->dev);
 
snd_hdac_set_codec_wakeup(bus, true);
snd_hdac_set_codec_wakeup(bus, false);
-   bus->display_power_active = cookie;
}
} else {
-   if (bus->display_power_active) {
-   unsigned long cookie = bus->display_power_active;
-
+   if (xchg(&bus->display_power_active, false)) {
if (acomp->ops->put_power)
-   acomp->ops->put_power(acomp->dev, cookie);
-
-   bus->display_power_active = 0;
+   acomp->ops->put_power(acomp->dev,
+ bus->display_cookie);
}
}
 }
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] snd/hda: Only get/put display_power once

[Takashi Iwai]

On Wed, 10 Apr 2019 12:24:24 +0200,
Chris Wilson wrote:
> 
> Quoting Takashi Iwai (2019-04-10 11:09:47)
> > On Wed, 10 Apr 2019 10:17:33 +0200,
> > Chris Wilson wrote:
> > > 
> > > While we only allow a single display power reference, the current
> > > acquisition/release is racy and a direct call may run concurrently with
> > > a runtime-pm worker. Prevent the double unreference by atomically
> > > tracking the display_power_active cookie.
> > > 
> > > Testcase: igt/i915_pm_rpm/module-reload #glk-dsi
> > > Signed-off-by: Chris Wilson 
> > > Cc: Takashi Iwai 
> > > Cc: Imre Deak 
> > 
> > I rather prefer a more straightforward conversion, e.g. something like
> > below.  Checking the returned cookie as the state flag is not quite
> > intuitive, so revive the boolean state flag, and handle it
> > atomically.
> 
> Access to the cookie itself is not atomic there, and theoretically
> there could be a get/put/get running concurrently. Are you sure don't
> want a refcount and lock here? :)

The refcount is what we want to reduce, so the suitable option would
be a (yet another) mutex although the cmpxchg() should be enough for
normal cases.

> Your call. For the case CI is hitting, it should do the trick (as we are
> only seeing the race on put/put I think). CI will answer in a hour or
> two.

OK, once when it seems working, I'll respin a patch with a mutex
instead.  We have already a one that is used for the link state
change, and this can be reused.


thanks,

Takashi
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] snd/hda: Only get/put display_power once

[Takashi Iwai]

On Wed, 10 Apr 2019 12:44:49 +0200,
Takashi Iwai wrote:
> 
> On Wed, 10 Apr 2019 12:24:24 +0200,
> Chris Wilson wrote:
> > 
> > Quoting Takashi Iwai (2019-04-10 11:09:47)
> > > On Wed, 10 Apr 2019 10:17:33 +0200,
> > > Chris Wilson wrote:
> > > > 
> > > > While we only allow a single display power reference, the current
> > > > acquisition/release is racy and a direct call may run concurrently with
> > > > a runtime-pm worker. Prevent the double unreference by atomically
> > > > tracking the display_power_active cookie.
> > > > 
> > > > Testcase: igt/i915_pm_rpm/module-reload #glk-dsi
> > > > Signed-off-by: Chris Wilson 
> > > > Cc: Takashi Iwai 
> > > > Cc: Imre Deak 
> > > 
> > > I rather prefer a more straightforward conversion, e.g. something like
> > > below.  Checking the returned cookie as the state flag is not quite
> > > intuitive, so revive the boolean state flag, and handle it
> > > atomically.
> > 
> > Access to the cookie itself is not atomic there, and theoretically
> > there could be a get/put/get running concurrently. Are you sure don't
> > want a refcount and lock here? :)
> 
> The refcount is what we want to reduce, so the suitable option would
> be a (yet another) mutex although the cmpxchg() should be enough for
> normal cases.
> 
> > Your call. For the case CI is hitting, it should do the trick (as we are
> > only seeing the race on put/put I think). CI will answer in a hour or
> > two.
> 
> OK, once when it seems working, I'll respin a patch with a mutex
> instead.  We have already a one that is used for the link state
> change, and this can be reused.

It's even simpler, so maybe this is a better way to go...

If this is confirmed to work, feel free to queue via drm tree.
I can't apply this because this is on top of your recent cookie and
sub-component changes that aren't on sound git tree (yet).


thanks,

Takashi

-- 8< --
From: Takashi Iwai 
Subject: [PATCH] ALSA: hda: Fix racy display power access

snd_hdac_display_power() doesn't handle the concurrent calls carefully
enough, and it may lead to the doubly get_power or put_power calls,
when a runtime PM and an async work get called in racy way.

This patch addresses it by reusing the bus->lock mutex that has been
used for protecting the link state change in ext bus code, so that it
can protect against racy display state changes.  The initialization of
bus->lock was moved from snd_hdac_ext_bus_init() to
snd_hdac_bus_init() as well accordingly.

Testcase: igt/i915_pm_rpm/module-reload #glk-dsi
Reported-by: Chris Wilson 
Cc: Imre Deak 
Signed-off-by: Takashi Iwai 
---
 sound/hda/ext/hdac_ext_bus.c | 1 -
 sound/hda/hdac_bus.c | 1 +
 sound/hda/hdac_component.c   | 6 +-
 3 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/sound/hda/ext/hdac_ext_bus.c b/sound/hda/ext/hdac_ext_bus.c
index 9c37d9af3023..ec7715c6b0c0 100644
--- a/sound/hda/ext/hdac_ext_bus.c
+++ b/sound/hda/ext/hdac_ext_bus.c
@@ -107,7 +107,6 @@ int snd_hdac_ext_bus_init(struct hdac_bus *bus, struct 
device *dev,
INIT_LIST_HEAD(&bus->hlink_list);
bus->idx = idx++;
 
-   mutex_init(&bus->lock);
bus->cmd_dma_state = true;
 
return 0;
diff --git a/sound/hda/hdac_bus.c b/sound/hda/hdac_bus.c
index 012305177f68..ad8eee08013f 100644
--- a/sound/hda/hdac_bus.c
+++ b/sound/hda/hdac_bus.c
@@ -38,6 +38,7 @@ int snd_hdac_bus_init(struct hdac_bus *bus, struct device 
*dev,
INIT_WORK(&bus->unsol_work, snd_hdac_bus_process_unsol_events);
spin_lock_init(&bus->reg_lock);
mutex_init(&bus->cmd_mutex);
+   mutex_init(&bus->lock);
bus->irq = -1;
return 0;
 }
diff --git a/sound/hda/hdac_component.c b/sound/hda/hdac_component.c
index 13915fdc6a54..dfe7e755f594 100644
--- a/sound/hda/hdac_component.c
+++ b/sound/hda/hdac_component.c
@@ -69,13 +69,15 @@ void snd_hdac_display_power(struct hdac_bus *bus, unsigned 
int idx, bool enable)
 
dev_dbg(bus->dev, "display power %s\n",
enable ? "enable" : "disable");
+
+   mutex_lock(&bus->lock);
if (enable)
set_bit(idx, &bus->display_power_status);
else
clear_bit(idx, &bus->display_power_status);
 
if (!acomp || !acomp->ops)
-   return;
+   goto unlock;
 
if (bus->display_power_status) {
if (!bus->display_power_active) {
@@ -98,6 +100,8 @@ void snd_hdac_display_power(struct hdac_bus *bus, unsigned 
int idx, bool enable)
bus->display_power_active = 0;
}
}
+ unlock:
+   mutex_unlock(&bus->lock);
 }
 EXPORT_SYMBOL_GPL(snd_hdac_display_power);
 
-- 
2.16.4

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] snd/hda: Only get/put display_power once

[Takashi Iwai]

On Wed, 10 Apr 2019 15:07:28 +0200,
Chris Wilson wrote:
> 
> Quoting Takashi Iwai (2019-04-10 12:03:22)
> > On Wed, 10 Apr 2019 12:44:49 +0200,
> > Takashi Iwai wrote:
> > > 
> > > On Wed, 10 Apr 2019 12:24:24 +0200,
> > > Chris Wilson wrote:
> > > > 
> > > > Quoting Takashi Iwai (2019-04-10 11:09:47)
> > > > > On Wed, 10 Apr 2019 10:17:33 +0200,
> > > > > Chris Wilson wrote:
> > > > > > 
> > > > > > While we only allow a single display power reference, the current
> > > > > > acquisition/release is racy and a direct call may run concurrently 
> > > > > > with
> > > > > > a runtime-pm worker. Prevent the double unreference by atomically
> > > > > > tracking the display_power_active cookie.
> > > > > > 
> > > > > > Testcase: igt/i915_pm_rpm/module-reload #glk-dsi
> > > > > > Signed-off-by: Chris Wilson 
> > > > > > Cc: Takashi Iwai 
> > > > > > Cc: Imre Deak 
> > > > > 
> > > > > I rather prefer a more straightforward conversion, e.g. something like
> > > > > below.  Checking the returned cookie as the state flag is not quite
> > > > > intuitive, so revive the boolean state flag, and handle it
> > > > > atomically.
> > > > 
> > > > Access to the cookie itself is not atomic there, and theoretically
> > > > there could be a get/put/get running concurrently. Are you sure don't
> > > > want a refcount and lock here? :)
> > > 
> > > The refcount is what we want to reduce, so the suitable option would
> > > be a (yet another) mutex although the cmpxchg() should be enough for
> > > normal cases.
> > > 
> > > > Your call. For the case CI is hitting, it should do the trick (as we are
> > > > only seeing the race on put/put I think). CI will answer in a hour or
> > > > two.
> > > 
> > > OK, once when it seems working, I'll respin a patch with a mutex
> > > instead.  We have already a one that is used for the link state
> > > change, and this can be reused.
> > 
> > It's even simpler, so maybe this is a better way to go...
> > 
> > If this is confirmed to work, feel free to queue via drm tree.
> > I can't apply this because this is on top of your recent cookie and
> > sub-component changes that aren't on sound git tree (yet).
> 
> Success \o/
> Reviewed-by: Chris Wilson 
> 
> Ok, we'll plonk it in dinq, but I think it should apply to sound.git?
> Looks fairly separate.

Indeed, it's applicable, so I'm going to queue via sound tree.
I thought it would conflict but that was about the previous version
fiddling with cmpxchg().  This one is simpler, yeah.

> Anyway that can all be resolved in a later merge if required.

Sure, I guess it must be trivial, if any.


Thanks!

Takashi
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] DP audio API changes for identifying displays connected to a port

[Takashi Iwai]

On Wed, 03 Aug 2016 04:14:29 +0200,
Dhinakaran Pandiyan wrote:
> 
> 
> An additional parameter has been added to the API's between i915 and audio
> drivers to identify display devices that can be connected to the same port.
> Currently only the port identity is being shared, but this is not
> sufficient as multiple displays can be driven from the same port
> (E.g. DP MST). This patch makes the API changes with dummy changes in the
> audio driver (Thanks Libin). Implementation to fully enable DP MST audio
> will follow later.

You should put this rather in the patch description, as it's a single
patch.


thanks,

Takashi


> 
> Dhinakaran Pandiyan(1):
> drm/i915/dp: DP audio API changes for MST
> 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915/dp: DP audio API changes for MST

[Takashi Iwai]

On Wed, 03 Aug 2016 04:14:30 +0200,
Dhinakaran Pandiyan wrote:
> 
> DP MST provides the capability to send multiple video and audio streams via
> one single port. This requires the API's between i915 and audio drivers to
> distinguish between audio capable displays connected to a port. This patch
> adds this support via an additional parameter 'int dev_id'. The existing
> parameter 'port' does not change it's meaning.
> 
> dev_id =
>   MST : pipe that the stream originates from
>   Non-MST : -1
> 
> Affected APIs:
> struct i915_audio_component_ops
> -   int (*sync_audio_rate)(struct device *, int port, int rate);
> + int (*sync_audio_rate)(struct device *, int port, int dev_id,
> +  int rate);
> 
> -   int (*get_eld)(struct device *, int port, bool *enabled,
> -   unsigned char *buf, int max_bytes);
> +   int (*get_eld)(struct device *, int port, int dev_id,
> +bool *enabled, unsigned char *buf, int max_bytes);
> 
> struct i915_audio_component_audio_ops
> -   void (*pin_eld_notify)(void *audio_ptr, int port);
> +   void (*pin_eld_notify)(void *audio_ptr, int port, int dev_id);
> 
> This patch makes dummy changes in the audio drivers for build to succeed.

OK, so the explicit dev_id will be passed in future change in the
audio side, right?  It'd be good to write up the grand plan.


> Signed-off-by: Dhinakaran Pandiyan 
> ---
>  drivers/gpu/drm/i915/i915_drv.h|  2 +-
>  drivers/gpu/drm/i915/intel_audio.c | 82 
> +-
>  include/drm/i915_component.h   |  6 +--
>  include/sound/hda_i915.h   | 11 ++---
>  sound/hda/hdac_i915.c  |  9 +++--
>  sound/pci/hda/patch_hdmi.c |  7 ++--
>  6 files changed, 82 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 65ada5d..8e4c8c8 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2054,7 +2054,7 @@ struct drm_i915_private {
>   /* perform PHY state sanity checks? */
>   bool chv_phy_assert[2];
>  
> - struct intel_encoder *dig_port_map[I915_MAX_PORTS];
> + struct intel_encoder *av_enc_map[I915_MAX_PIPES];

Better to have a comment for this field.


> -static int i915_audio_component_sync_audio_rate(struct device *dev,
> - int port, int rate)
> +static struct intel_encoder *get_saved_encoder(struct intel_encoder 
> *av_enc_map[],
> +int port, int dev_id)
> +{
> +
> + enum pipe pipe;
> + struct drm_encoder *encoder;
> +
> + if (dev_id >= 0 && dev_id < I915_MAX_PIPES)
> + return av_enc_map[dev_id];

Actually dev_id >= I915_MAX_PIPES is an invalid call, and worth to
catch with WARN_ON() and bail out.


thanks,

Takashi
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915/dp: DP audio API changes for MST

[Takashi Iwai]

On Thu, 04 Aug 2016 19:35:16 +0200,
Ville Syrjälä wrote:
> 
> On Thu, Aug 04, 2016 at 10:18:52AM -0700, Jim Bride wrote:
> > On Wed, Aug 03, 2016 at 10:08:12PM +0300, Ville Syrjälä wrote:
> > > On Tue, Aug 02, 2016 at 07:14:30PM -0700, Dhinakaran Pandiyan wrote:
> > > > DP MST provides the capability to send multiple video and audio streams 
> > > > via
> > > > one single port. This requires the API's between i915 and audio drivers 
> > > > to
> > > > distinguish between audio capable displays connected to a port. This 
> > > > patch
> > > > adds this support via an additional parameter 'int dev_id'. The existing
> > > > parameter 'port' does not change it's meaning.
> > > > 
> > > > dev_id =
> > > > MST : pipe that the stream originates from
> > > > Non-MST : -1
> > > > 
> > > > Affected APIs:
> > > > struct i915_audio_component_ops
> > > > -   int (*sync_audio_rate)(struct device *, int port, int rate);
> > > > +   int (*sync_audio_rate)(struct device *, int port, int dev_id,
> > > 
> > > Does the term 'dev_id' have some special meaning on the audio side? On
> > > the i915 side things would be less confusing if we just called it
> > > 'pipe'.
> > 
> > Yeah, it does.  All of the documentation on the audio side is written
> > in terms of device ID, so they asked for that nomenclature.
> 
> And is the device ID always the same as the pipe? Until now we've made
> due with passing the port instead of the pipe, so either the audio side
> didn't use the device ID, or its meaning changes based on how we drive
> things, or they dug it out from somewhere else based on the port?

This is my concern, too.  Currently we have a very wild assumption
even for the port mapping.  In the audio side, there is neither port
nor pipe.  There are only the widget node id and the device id.  The
former is supposedly corresponding to the port, and the latter to the
pipe.  But the audio side has absolutely no clue about how these are
connected.


Takashi
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Acquire audio powerwell for HD-Audio registers

[Takashi Iwai]

On Thu, 04 Aug 2016 18:44:11 +0200,
Daniel Vetter wrote:
> 
> On Wed, Aug 03, 2016 at 05:09:00PM +0100, Chris Wilson wrote:
> > On Haswell/Broadwell, the HD-Audio block is inside the HDMI/display
> > power well and so the sna-hda audio codec acquires the display power
> > well while it is operational. However, Skylake separates the powerwells
> > again, but yet we still need the audio powerwell to setup the registers.
> > (But then the hardware uses those registers even while powered off???)
> 
> Yeah feels fishy, but will at least duct-tape over the breakage from the
> audio side. Most likely the reg writes go exactly nowhere and there's a
> bug on the audio side. And this patch doesn't fix that.

Well, if the relevant code paths are only over these callbacks, I
guess the following fix would work instead.

Can anyone check?


Takashi

---
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index 89dacf9b4e6c..88ad391452ae 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -1021,8 +1021,8 @@ static int azx_runtime_resume(struct device *dev)
snd_hdac_i915_set_bclk(bus);
} else {
/* toggle codec wakeup bit for STATESTS read */
-   snd_hdac_set_codec_wakeup(bus, true);
-   snd_hdac_set_codec_wakeup(bus, false);
+   snd_hdac_display_power(bus, true);
+   snd_hdac_display_power(bus, false);
}
}
 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Acquire audio powerwell for HD-Audio registers

[Takashi Iwai]

[dropped stable ML and add a few other relevant people to Cc]

On Thu, 04 Aug 2016 20:05:27 +0200,
Takashi Iwai wrote:
> 
> On Thu, 04 Aug 2016 18:44:11 +0200,
> Daniel Vetter wrote:
> > 
> > On Wed, Aug 03, 2016 at 05:09:00PM +0100, Chris Wilson wrote:
> > > On Haswell/Broadwell, the HD-Audio block is inside the HDMI/display
> > > power well and so the sna-hda audio codec acquires the display power
> > > well while it is operational. However, Skylake separates the powerwells
> > > again, but yet we still need the audio powerwell to setup the registers.
> > > (But then the hardware uses those registers even while powered off???)
> > 
> > Yeah feels fishy, but will at least duct-tape over the breakage from the
> > audio side. Most likely the reg writes go exactly nowhere and there's a
> > bug on the audio side. And this patch doesn't fix that.
> 
> Well, if the relevant code paths are only over these callbacks, I
> guess the following fix would work instead.
> 
> Can anyone check?

Scratch the previous one.  There is another place calling similarly.

Now looking back at the code again, I believe that the better way
would be to properly do power up / down at the resume callbacks.

Below is an untested patch.  Let me know if this actually works.


thanks,

Takashi

-- 8< --
From: Takashi Iwai 
Subject: [PATCH] ALSA: hda - Manage power well properly for resume

For SKL and later Intel chips, we control the power well per codec
basis via link_power callback since the commit [03b135cebc47: ALSA:
hda - remove dependency on i915 power well for SKL].
However, there are a few exceptional cases where the gfx registers are
accessed from the audio driver: namely the wakeup override bit
toggling at (both system and runtime) resume.  This seems causing a
kernel warning when accessed during the power well down (and likely
resulting in the bogus register accesses).

This patch puts the proper power up / down sequence around the resume
code so that the wakeup bit is fiddled properly while the power is
up.  (The other callback, sync_audio_rate, is used only in the PCM
callback, so it's guaranteed in the power-on.)

Also, by this proper power up/down, the instantaneous flip of wakeup
bit in the resume callback that was introduced by the commit
[033ea349a7cd: ALSA: hda - Fix Skylake codec timeout] becomes
superfluous, as snd_hdac_display_power() already does it.  So we can
clean it up together.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=96214
Fixes: 03b135cebc47 ('ALSA: hda - remove dependency on i915 power well for SKL')
Signed-off-by: Takashi Iwai 
---
 sound/pci/hda/hda_intel.c | 32 
 1 file changed, 20 insertions(+), 12 deletions(-)

diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index 89dacf9b4e6c..160c7f713722 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -906,20 +906,23 @@ static int azx_resume(struct device *dev)
struct snd_card *card = dev_get_drvdata(dev);
struct azx *chip;
struct hda_intel *hda;
+   struct hdac_bus *bus;
 
if (!card)
return 0;
 
chip = card->private_data;
hda = container_of(chip, struct hda_intel, chip);
+   bus = azx_bus(chip);
if (chip->disabled || hda->init_failed || !chip->running)
return 0;
 
-   if (chip->driver_caps & AZX_DCAPS_I915_POWERWELL
-   && hda->need_i915_power) {
-   snd_hdac_display_power(azx_bus(chip), true);
-   snd_hdac_i915_set_bclk(azx_bus(chip));
+   if (chip->driver_caps & AZX_DCAPS_I915_POWERWELL) {
+   snd_hdac_display_power(bus, true);
+   if (hda->need_i915_power)
+   snd_hdac_i915_set_bclk(bus);
}
+
if (chip->msi)
if (pci_enable_msi(pci) < 0)
chip->msi = 0;
@@ -929,6 +932,11 @@ static int azx_resume(struct device *dev)
 
hda_intel_init_chip(chip, true);
 
+   /* power down again for link-controlled chips */
+   if ((chip->driver_caps & AZX_DCAPS_I915_POWERWELL) &&
+   !hda->need_i915_power)
+   snd_hdac_display_power(bus, false);
+
snd_power_change_state(card, SNDRV_CTL_POWER_D0);
 
trace_azx_resume(chip);
@@ -1008,6 +1016,7 @@ static int azx_runtime_resume(struct device *dev)
 
chip = card->private_data;
hda = container_of(chip, struct hda_intel, chip);
+   bus = azx_bus(chip);
if (chip->disabled || hda->init_failed)
return 0;
 
@@ -1015,15 +1024,9 @@ static int azx_runtime_resume(struct device *dev)
return 0;
 
if (chip->driver_caps & AZX_DCAPS_I915_POWERWELL) {
-  

Re: [Intel-gfx] [PATCH v10 02/40] i915/snd_hdac: I915 subcomponent for the snd_hdac

[Takashi Iwai]

On Mon, 04 Feb 2019 16:00:18 +0100,
Daniel Vetter wrote:
> 
> On Thu, Jan 31, 2019 at 12:29:18PM +0530, Ramalingam C wrote:
> > From: Daniel Vetter 
> > 
> > Since we need multiple components for I915 for different purposes
> > (Audio & Mei_hdcp), we adopt the subcomponents methodology introduced
> > by the previous patch (mentioned below).
> > 
> > Author: Daniel Vetter 
> > Date:   Mon Jan 28 17:08:20 2019 +0530
> > 
> > components: multiple components for a device
> > 
> > Signed-off-by: Daniel Vetter 
> > Signed-off-by: Ramalingam C 
> > cc: Greg Kroah-Hartman 
> > cc: Russell King 
> > cc: Rafael J. Wysocki 
> > cc: Jaroslav Kysela 
> > cc: Takashi Iwai 
> > cc: Rodrigo Vivi 
> > cc: Jani Nikula 
> 
> Takashi, can you pls take a look and ack for merging through drm-intel? We
> can also do a topic branch in case this conflicts with something on the
> audio side.

I'm fine with the change as long as others agree with this API
extension.

Reviewed-by: Takashi Iwai 


thanks,

Takashi
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PULL] topic/component-typed

[Takashi Iwai]

On Mon, 11 Feb 2019 19:25:12 +0100,
Sam Ravnborg wrote:
> 
> Hi Daniel.
> 
> On Mon, Feb 11, 2019 at 06:15:20PM +0100, Daniel Vetter wrote:
> > Hi all,
> > 
> > Here's the typed component topic branch.
> > 
> > drm-intel maintainers: Please pull, I need this for the mei hdcp work from 
> > Ram.
> > 
> > drm-misc maintainers: Please pull, there's a drm doc patch follow-up
> > that I want to stuff into drm-misc-next.
> > 
> > Greg: The drm side missed our feature cutoff, so will only land in 5.2.
> 
> With all the dependencies why not bend the rule a little and get this in now?
> It is not like this is a huge patchset.

Yeah, that's likely the most straightforward way.

BTW, I tried to pull onto sound git tree for-next branch, and it
worked without conflicts.  So I don't think it needs to be pulled onto
mine.


thanks,

Takashi
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] snd/hda, drm/i915: Track the display_power_status using a cookie

[Takashi Iwai]

On Wed, 13 Feb 2019 16:21:09 +0100,
Chris Wilson wrote:
> 
> drm/i915 is tracking all wakeref owners with a cookie in order to
> identify leaks. To that end, each rpm acquisition ops->get_power is
> assigned a cookie which should be passed to ops->put_power to signify
> its release (and removal from the list of wakeref owners). As snd/hda is
> already using a bool to track current status of display_power extending
> that to an unsigned long to hold the boolean cookie is a trivial
> extension, and will quell all doubt that snd/hda is the cause of the
> device runtime pm leaks.
> 
> v2: Keep using the power abstraction for local wakeref tracking.
> 
> Signed-off-by: Chris Wilson 
> Cc: Takashi Iwai 
> Cc: Jani Nikula 

Feel free to take my ack:
  Reviewed-by: Takashi Iwai 

Or let me know if you guys want to apply this through sound tree for
5.1 merge.


thanks,

Takashi
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] ALSA: hda/hdmi: Disable silent stream on GLK

[Takashi Iwai]

On Wed, 22 Dec 2021 15:53:50 +0100,
Ville Syrjala wrote:
> 
> From: Ville Syrjälä 
> 
> The silent stream stuff recurses back into i915 audio
> component .get_power() from the .pin_eld_notify() hook.
> On GLK this will deadlock as i915 may already be holding
> the relevant modeset locks during .pin_eld_notify() and
> the GLK audio vs. CDCLK workaround will try to grab the
> same locks from .get_power().
> 
> Until someone comes up with a better fix just disable the
> silent stream support on GLK.
> 
> Cc: sta...@vger.kernel.org
> Cc: Harsha Priya 
> Cc: Emmanuel Jillela 
> Cc: Kai Vehmanen 
> Cc: Takashi Iwai 
> Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/2623
> Fixes: 951894cf30f4 ("ALSA: hda/hdmi: Add Intel silent stream support")
> Signed-off-by: Ville Syrjälä 

Thanks, applied now.


Takashi

Re: [Intel-gfx] [PATCH v3] ALSA: hda/i915 - avoid hung task timeout in i915 wait

[Takashi Iwai]

On Wed, 09 Mar 2022 19:24:39 +0100,
Kai Vehmanen wrote:
> 
> If kernel is built with hung task detection enabled and
> CONFIG_DEFAULT_HUNG_TASK_TIMEOUT set to less than 60 seconds,
> snd_hdac_i915_init() will trigger the hung task timeout in case i915 is
> not available and taint the kernel.
> 
> Use wait_for_completion_killable_timeout() for the wait to
> avoid this problem.
> 
> Co-developed-by: Ramalingam C 
> Signed-off-by: Ramalingam C 
> Signed-off-by: Kai Vehmanen 

Applied now.  Thanks.


Takashi

Re: [Intel-gfx] [PATCH v5 2/2] ALSA: hda - identify when audio is provided by a video driver

[Takashi Iwai]

On Sat, 30 Apr 2022 22:04:55 +0200,
Mauro Carvalho Chehab wrote:
> 
> On some devices, the hda driver needs to hook into a video driver,
> in order to be able to properly access the audio hardware and/or
> the power management function.
> 
> That's the case of several snd_hda_intel devices that depends on
> i915 driver.
> 
> Ensure that a proper reference between the snd-hda driver needing
> such binding is shown at /proc/modules, in order to allow userspace
> to know about such binding.
> 
> Signed-off-by: Mauro Carvalho Chehab 

Maybe I was too late to the game (just back from vacation), but FWIW:

Reviewed-by: Takashi Iwai 


thanks,

Takashi

Re: [Intel-gfx] [PATCH] ALSA: hda/i915: Fix one too many pci_dev_put()

[Takashi Iwai]

On Mon, 18 Apr 2022 06:50:32 +0200,
Lucas De Marchi wrote:
> 
> On Sun, Apr 17, 2022 at 01:13:49PM +0300, Kai Vehmanen wrote:
> >Hi,
> >
> >On Fri, 15 Apr 2022, Lucas De Marchi wrote:
> >
> >> pci_get_class() will already unref the pci device passed as argument.
> >> So if it's unconditionally unref'ed, even if the loop is not stopped,
> >
> >thanks Lucas. And yes indeed, overlooked that pci_get_class()
> >will decrement the from device is specified.
> >
> >> --- a/sound/hda/hdac_i915.c
> >> +++ b/sound/hda/hdac_i915.c
> >> @@ -127,11 +127,10 @@ static int i915_gfx_present(struct pci_dev *hdac_pci)
> >>display_dev = pci_get_class(class, display_dev);
> >>
> >>if (display_dev && display_dev->vendor == PCI_VENDOR_ID_INTEL &&
> >> -  connectivity_check(display_dev, hdac_pci))
> >> +  connectivity_check(display_dev, hdac_pci)) {
> >> +  pci_dev_put(display_dev);
> >>match = true;
> >> -
> >> -  pci_dev_put(display_dev);
> >> -
> >> +  }
> >
> >Reviewed-by: Kai Vehmanen 
> 
> I applied this to our topic/core-for-CI branch to unblock CI on
> DG2. Ultimately the target for this is the sound tree though.

The patch looks good, feel free to submit it.


thanks,

Takashi

Re: [Intel-gfx] [PATCH] ALSA: hda/i915: Fix one too many pci_dev_put()

[Takashi Iwai]

On Tue, 19 Apr 2022 08:26:06 +0200,
Lucas De Marchi wrote:
> 
> On Tue, Apr 19, 2022 at 07:54:30AM +0200, Takashi Iwai wrote:
> >On Mon, 18 Apr 2022 06:50:32 +0200,
> >Lucas De Marchi wrote:
> >>
> >> On Sun, Apr 17, 2022 at 01:13:49PM +0300, Kai Vehmanen wrote:
> >> >Hi,
> >> >
> >> >On Fri, 15 Apr 2022, Lucas De Marchi wrote:
> >> >
> >> >> pci_get_class() will already unref the pci device passed as argument.
> >> >> So if it's unconditionally unref'ed, even if the loop is not stopped,
> >> >
> >> >thanks Lucas. And yes indeed, overlooked that pci_get_class()
> >> >will decrement the from device is specified.
> >> >
> >> >> --- a/sound/hda/hdac_i915.c
> >> >> +++ b/sound/hda/hdac_i915.c
> >> >> @@ -127,11 +127,10 @@ static int i915_gfx_present(struct pci_dev 
> >> >> *hdac_pci)
> >> >> display_dev = pci_get_class(class, display_dev);
> >> >>
> >> >> if (display_dev && display_dev->vendor == 
> >> >> PCI_VENDOR_ID_INTEL &&
> >> >> -   connectivity_check(display_dev, hdac_pci))
> >> >> +   connectivity_check(display_dev, hdac_pci)) {
> >> >> +   pci_dev_put(display_dev);
> >> >> match = true;
> >> >> -
> >> >> -   pci_dev_put(display_dev);
> >> >> -
> >> >> +   }
> >> >
> >> >Reviewed-by: Kai Vehmanen 
> >>
> >> I applied this to our topic/core-for-CI branch to unblock CI on
> >> DG2. Ultimately the target for this is the sound tree though.
> >
> >The patch looks good, feel free to submit it.
> 
> not sure if I was clear. This patch is already targeting the sound tree:
> it should apply cleanly.

The original patch hasn't reached to me (we've had the mail server
problem in the last weekend, and that might be the reason).

Could you resubmit?


thanks,

Takashi

Re: [Intel-gfx] [PATCH] ALSA: hda/i915: Fix one too many pci_dev_put()

[Takashi Iwai]

On Tue, 19 Apr 2022 08:40:01 +0200,
Takashi Iwai wrote:
> 
> On Tue, 19 Apr 2022 08:26:06 +0200,
> Lucas De Marchi wrote:
> > 
> > On Tue, Apr 19, 2022 at 07:54:30AM +0200, Takashi Iwai wrote:
> > >On Mon, 18 Apr 2022 06:50:32 +0200,
> > >Lucas De Marchi wrote:
> > >>
> > >> On Sun, Apr 17, 2022 at 01:13:49PM +0300, Kai Vehmanen wrote:
> > >> >Hi,
> > >> >
> > >> >On Fri, 15 Apr 2022, Lucas De Marchi wrote:
> > >> >
> > >> >> pci_get_class() will already unref the pci device passed as argument.
> > >> >> So if it's unconditionally unref'ed, even if the loop is not stopped,
> > >> >
> > >> >thanks Lucas. And yes indeed, overlooked that pci_get_class()
> > >> >will decrement the from device is specified.
> > >> >
> > >> >> --- a/sound/hda/hdac_i915.c
> > >> >> +++ b/sound/hda/hdac_i915.c
> > >> >> @@ -127,11 +127,10 @@ static int i915_gfx_present(struct pci_dev 
> > >> >> *hdac_pci)
> > >> >>   display_dev = pci_get_class(class, display_dev);
> > >> >>
> > >> >>   if (display_dev && display_dev->vendor == 
> > >> >> PCI_VENDOR_ID_INTEL &&
> > >> >> - connectivity_check(display_dev, hdac_pci))
> > >> >> + connectivity_check(display_dev, hdac_pci)) {
> > >> >> + pci_dev_put(display_dev);
> > >> >>   match = true;
> > >> >> -
> > >> >> - pci_dev_put(display_dev);
> > >> >> -
> > >> >> + }
> > >> >
> > >> >Reviewed-by: Kai Vehmanen 
> > >>
> > >> I applied this to our topic/core-for-CI branch to unblock CI on
> > >> DG2. Ultimately the target for this is the sound tree though.
> > >
> > >The patch looks good, feel free to submit it.
> > 
> > not sure if I was clear. This patch is already targeting the sound tree:
> > it should apply cleanly.
> 
> The original patch hasn't reached to me (we've had the mail server
> problem in the last weekend, and that might be the reason).
> 
> Could you resubmit?

Never mind, I could find it in lore.
  https://lore.kernel.org/all/20220416064418.2364582-1-lucas.demar...@intel.com/


Takashi

Re: [Intel-gfx] [PATCH] ALSA: hda/i915: Fix one too many pci_dev_put()

[Takashi Iwai]

On Sat, 16 Apr 2022 08:44:18 +0200,
Lucas De Marchi wrote:
> 
> pci_get_class() will already unref the pci device passed as argument.
> So if it's unconditionally unref'ed, even if the loop is not stopped,
> there will be one too many unref for each device not matched.
> 
> Cc: Kai Vehmanen 
> Cc: Takashi Iwai 
> Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/5701
> Fixes: 0dc2696a4623 ("ALSA: hda/i915 - skip acomp init if no matching 
> display")
> Signed-off-by: Lucas De Marchi 

Thanks, applied now.
But the Fixes commit id was wrong.  I corrected to the right upstream
one, c9db8a30d9f0.


Takashi

Re: [Intel-gfx] [PATCH] ALSA: hda: fix general protection fault in azx_runtime_idle

[Takashi Iwai]

On Wed, 10 Nov 2021 22:03:07 +0100,
Kai Vehmanen wrote:
> 
> Fix a corner case between PCI device driver remove callback and
> runtime PM idle callback.
> 
> Following sequence of events can happen:
>   - at azx_create, context is allocated with devm_kzalloc() and
> stored as pci_set_drvdata()
>   - user-space requests to unbind audio driver
>   - dd.c:__device_release_driver() calls PCI remove
>   - pci-driver.c:pci_device_remove() calls the audio
> driver azx_remove() callback and this is completed
>   - pci-driver.c:pm_runtime_put_sync() leads to a call
> to rpm_idle() which again calls azx_runtime_idle()
>   - the azx context object, as returned by dev_get_drvdata(),
> is no longer valid
>   -> access fault in azx_runtime_idle when executing
>   struct snd_card *card = dev_get_drvdata(dev);
>   chip = card->private_data;
>   if (chip->disabled || hda->init_failed)
> 
> This was discovered by i915_module_load test with 5.15.0 based
> linux-next tree.
> 
> Example log caught by i915_module_load test with linux-next
> https://intel-gfx-ci.01.org/tree/linux-next/
> 
> <4> [264.038232] general protection fault, probably for non-canonical address 
> 0x6b6b6b6b6b6b73f0:  [#1] PREEMPT SMP NOPTI
> <4> [264.038248] CPU: 0 PID: 5374 Comm: i915_module_loa Not tainted 
> 5.15.0-next-20211109-gc8109c2ba35e-next-20211109 #1
> [...]
> <4> [264.038267] RIP: 0010:azx_runtime_idle+0x12/0x60 [snd_hda_intel]
> [...]
> <4> [264.038355] Call Trace:
> <4> [264.038359]  
> <4> [264.038362]  __rpm_callback+0x3d/0x110
> <4> [264.038371]  rpm_idle+0x27f/0x380
> <4> [264.038376]  __pm_runtime_idle+0x3b/0x100
> <4> [264.038382]  pci_device_remove+0x6d/0xa0
> <4> [264.038388]  device_release_driver_internal+0xef/0x1e0
> <4> [264.038395]  unbind_store+0xeb/0x120
> <4> [264.038400]  kernfs_fop_write_iter+0x11a/0x1c0
> 
> Fix the issue by setting drvdata to NULL at end of azx_remove().
> 
> Signed-off-by: Kai Vehmanen 
> ---
>  sound/pci/hda/hda_intel.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> Some non-persistent direct links showing the bug trigger on
> different platforms with linux-next 20211109:
>  - 
> https://intel-gfx-ci.01.org/tree/linux-next/next-20211109/fi-tgl-1115g4/igt@i915_module_l...@reload.html
>  - 
> https://intel-gfx-ci.01.org/tree/linux-next/next-20211109/fi-jsl-1/igt@i915_module_l...@reload.html
> 
> Notably with 2020 linux-next, the bug does not trigger:
>  - 
> https://intel-gfx-ci.01.org/tree/linux-next/next-2020/fi-tgl-1115g4/igt@i915_module_l...@reload.html

Is this the case with CONFIG_DEBUG_KOBJECT_RELEASE?
This would be the only logical explanation I can think of for now.

In anyway, the code change itself looks good, so I took the fix now.


thanks,

Takashi

Re: [Intel-gfx] [PATCH] ALSA: hda: fix general protection fault in azx_runtime_idle

[Takashi Iwai]

On Wed, 10 Nov 2021 23:15:40 +0100,
Kai Vehmanen wrote:
> 
> Hey,
> 
> On Wed, 10 Nov 2021, Takashi Iwai wrote:
> 
> > On Wed, 10 Nov 2021 22:03:07 +0100, Kai Vehmanen wrote:
> > > Fix a corner case between PCI device driver remove callback and
> > > runtime PM idle callback.
> [...]
> > > Some non-persistent direct links showing the bug trigger on
> > > different platforms with linux-next 20211109:
> > >  - 
> > > https://intel-gfx-ci.01.org/tree/linux-next/next-20211109/fi-tgl-1115g4/igt@i915_module_l...@reload.html
> > >  - 
> > > https://intel-gfx-ci.01.org/tree/linux-next/next-20211109/fi-jsl-1/igt@i915_module_l...@reload.html
> > > 
> > > Notably with 2020 linux-next, the bug does not trigger:
> > >  - 
> > > https://intel-gfx-ci.01.org/tree/linux-next/next-2020/fi-tgl-1115g4/igt@i915_module_l...@reload.html
> > 
> > Is this the case with CONFIG_DEBUG_KOBJECT_RELEASE?
> > This would be the only logical explanation I can think of for now.
> 
> hmm, that doesn't seem to be used. Here's a link to kconfig used in the 
> failing CI run:
> https://intel-gfx-ci.01.org/tree/linux-next/next-20211109/kconfig.txt

OK, then it's not due to the delayed release, but the cause should be
the same, I suppose.

> It's still a bit odd, especially given Scott just reported the other HDA 
> related regression in 5.15 today. The two issues don't seem to be related 
> though, although both are fixed by clearing drvdata (but in different 
> places of hda_intel.c).

I don't think it's the same issue, rather a coincidence of the
timing.  There have been many changes in 5.15, after all :)

> I'll try to run some more tests tomorrow. The fix should be good in any 
> case, but it would be interesting to understand better what change made 
> this more (?) likely to hit than before. This is not a new test and the 
> problem happens on fairly old platforms, so something has changed.

A potential problem with the current code is that it doesn't disable
the runtime PM at the release procedure.  Could you try the patch
below?  You can put WARN_ON(!chip) at azx_runtime_idle(), too, for
catching the invalid runtime call.


thanks,

Takashi

--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -1347,8 +1347,13 @@ static void azx_free(struct azx *chip)
if (hda->freed)
return;
 
-   if (azx_has_pm_runtime(chip) && chip->running)
+   if (azx_has_pm_runtime(chip) && chip->running) {
pm_runtime_get_noresume(&pci->dev);
+   pm_runtime_forbid(&pci->dev);
+   pm_runtime_dont_use_autosuspend(&pci->dev);
+   pm_runtime_disable(&pci->dev);
+   }
+
chip->running = 0;
 
azx_del_card_list(chip);
@@ -2320,6 +2325,7 @@ static int azx_probe_continue(struct azx *chip)
set_default_power_save(chip);
 
if (azx_has_pm_runtime(chip)) {
+   pm_runtime_enable(&pci->dev);
pm_runtime_use_autosuspend(&pci->dev);
pm_runtime_allow(&pci->dev);
pm_runtime_put_autosuspend(&pci->dev);

Re: [Intel-gfx] [PATCH] ALSA: hda: fix general protection fault in azx_runtime_idle

[Takashi Iwai]

On Thu, 11 Nov 2021 18:39:36 +0100,
Kai Vehmanen wrote:
> 
> Hi,
> 
> On Thu, 11 Nov 2021, Takashi Iwai wrote:
> 
> > A potential problem with the current code is that it doesn't disable
> > the runtime PM at the release procedure.  Could you try the patch
> > below?  You can put WARN_ON(!chip) at azx_runtime_idle(), too, for
> > catching the invalid runtime call.
> [...]
> > --- a/sound/pci/hda/hda_intel.c
> > +++ b/sound/pci/hda/hda_intel.c
> > @@ -1347,8 +1347,13 @@ static void azx_free(struct azx *chip)
> > if (hda->freed)
> > return;
> >  
> > -   if (azx_has_pm_runtime(chip) && chip->running)
> > +   if (azx_has_pm_runtime(chip) && chip->running) {
> > pm_runtime_get_noresume(&pci->dev);
> > +   pm_runtime_forbid(&pci->dev);
> > +   pm_runtime_dont_use_autosuspend(&pci->dev);
> > +   pm_runtime_disable(&pci->dev);
> > +   }
> > +
> > chip->running = 0;
> 
> Tested with next-20211019 (first next tag where I've seen test failures) 
> and your patch, and this seems to do the trick. I didn't have my drvdata 
> patch included when I ran the test. No rpm_idle() calls 
> anymore after azx_remove(), so the bug is not hit.

So far, so good...

> > azx_del_card_list(chip);
> > @@ -2320,6 +2325,7 @@ static int azx_probe_continue(struct azx *chip)
> > set_default_power_save(chip);
> >  
> > if (azx_has_pm_runtime(chip)) {
> > +   pm_runtime_enable(&pci->dev);
> > pm_runtime_use_autosuspend(&pci->dev);
> 
> This does generate warnings
> [   13.495059] snd_hda_intel :00:1f.3: Unbalanced pm_runtime_enable!
> 
> And later
> [   54.770701] Enabling runtime PM for inactive device (:00:1f.3) with 
> active children
> [   54.770718] WARNING: CPU: 0 PID: 10 at drivers/base/power/runtime.c:1439 
> pm_runtime_enable+0x98/0xb0
> 
> Adding a "pm_runtime_set_active(&pci->dev)" to both azx_free() and 
> azx_probe_continue() seems to help and fix still works.

Ah yes, I was confused as if it were already called in hdac_device.c,
but this was about the HD-audio bus controller, not the codec.

Below is the revised one.


Takashi

-- 8< --
From: Takashi Iwai 
Subject: [PATCH] ALSA: hda: intel: More comprehensive PM runtime setup for
 controller driver

Currently we haven't explicitly enable and allow/forbid the runtime PM
at the probe and the remove phases of HD-audio controller driver, and
this was the reason of a GPF mentioned in the commit e81478bbe7a1
("ALSA: hda: fix general protection fault in azx_runtime_idle");
namely, even after the resources are released, the runtime PM might be
still invoked by the bound graphics driver during the remove of the
controller driver.  Although we've fixed it by clearing the drvdata
reference, it'd be also better to cover the runtime PM issue more
properly.

This patch adds a few more pm_runtime_*() calls at the probe and the
remove time for setting and cleaning up the runtime PM.  Particularly,
now more explicitly pm_runtime_enable() and _disable() get called as
well as pm_runtime_forbid() call at the remove callback, so that a
use-after-free should be avoided.

Reported-by: Kai Vehmanen 
Signed-off-by: Takashi Iwai 
---
 sound/pci/hda/hda_intel.c | 10 +-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index fe51163f2d82..45e85180048c 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -1347,8 +1347,14 @@ static void azx_free(struct azx *chip)
if (hda->freed)
return;
 
-   if (azx_has_pm_runtime(chip) && chip->running)
+   if (azx_has_pm_runtime(chip) && chip->running) {
pm_runtime_get_noresume(&pci->dev);
+   pm_runtime_disable(&pci->dev);
+   pm_runtime_set_suspended(&pci->dev);
+   pm_runtime_forbid(&pci->dev);
+   pm_runtime_dont_use_autosuspend(&pci->dev);
+   }
+
chip->running = 0;
 
azx_del_card_list(chip);
@@ -2322,6 +2328,8 @@ static int azx_probe_continue(struct azx *chip)
if (azx_has_pm_runtime(chip)) {
pm_runtime_use_autosuspend(&pci->dev);
pm_runtime_allow(&pci->dev);
+   pm_runtime_set_active(&pci->dev);
+   pm_runtime_enable(&pci->dev);
pm_runtime_put_autosuspend(&pci->dev);
}
 
-- 
2.31.1

Re: [Intel-gfx] [PATCH] ALSA: hda: fix general protection fault in azx_runtime_idle

[Takashi Iwai]

On Fri, 12 Nov 2021 13:27:34 +0100,
Kai Vehmanen wrote:
> 
> Hi,
> 
> On Fri, 12 Nov 2021, Takashi Iwai wrote:
> 
> > On Thu, 11 Nov 2021 18:39:36 +0100, Kai Vehmanen wrote:
> > > And later
> > > [   54.770701] Enabling runtime PM for inactive device (:00:1f.3) 
> > > with active children
> > > [   54.770718] WARNING: CPU: 0 PID: 10 at 
> > > drivers/base/power/runtime.c:1439 pm_runtime_enable+0x98/0xb0
> > > 
> > > Adding a "pm_runtime_set_active(&pci->dev)" to both azx_free() and 
> > > azx_probe_continue() seems to help and fix still works.
> > 
> > Ah yes, I was confused as if it were already called in hdac_device.c,
> > but this was about the HD-audio bus controller, not the codec.
> > 
> > Below is the revised one.
> [...]
> > Currently we haven't explicitly enable and allow/forbid the runtime PM
> > at the probe and the remove phases of HD-audio controller driver, and
> > this was the reason of a GPF mentioned in the commit e81478bbe7a1
> > ("ALSA: hda: fix general protection fault in azx_runtime_idle");
> > namely, even after the resources are released, the runtime PM might be
> > still invoked by the bound graphics driver during the remove of the
> > controller driver.  Although we've fixed it by clearing the drvdata
> > reference, it'd be also better to cover the runtime PM issue more
> > properly.
> > 
> > This patch adds a few more pm_runtime_*() calls at the probe and the
> > remove time for setting and cleaning up the runtime PM.  Particularly,
> > now more explicitly pm_runtime_enable() and _disable() get called as
> > well as pm_runtime_forbid() call at the remove callback, so that a
> > use-after-free should be avoided.
> > 
> > Reported-by: Kai Vehmanen 
> > Signed-off-by: Takashi Iwai 
> 
> ack, tested this and no warnings anymore.
> 
> Reviewed-by: Kai Vehmanen 
> Tested-by: Kai Vehmanen 

OK, I'll submit and merge the patch.

As the original problem was fixed by NULL checks, I'd push this for
5.16, though.


thanks,

Takashi

Re: [Intel-gfx] [PATCH 2/2] hda/i915: split the wait for the component binding

[Takashi Iwai]

On Thu, 24 Feb 2022 17:34:54 +0100,
Kai Vehmanen wrote:
> 
> Hi,
> 
> On Thu, 24 Feb 2022, Ramalingam C wrote:
> 
> > Split the wait for component binding from i915 in multiples of
> > sysctl_hung_task_timeout_secs. This helps to avoid the possible kworker
> > thread hung detection given below.
> 
> while I understand the problem, I'm not sure whether a simpler option
> should be chosen. Maybe just split the wait_for_completion_timeout()
> into small 5sec iterations, without consulting value of hung_task_timeout.
> This would seem unligned with more mainstream use of 
> wait_for_completion_timeout() in kernel and still do the job.
> 
> I'll loop in Takashi here as well. Basicly the 60 sec timeout in 
> hda/hdac_i915.c is getting caught by hung_task_detection logic in builds
> where the hung_task_timeout is below 60secs.
> 
> I have a patch that tries to avoid hitting the timeout in some of the more 
> common cases:
> "ALSA: hda/i915 - skip acomp init if no matching display"
> https://lists.freedesktop.org/archives/intel-gfx-trybot/2022-February/128278.html
> ... but we'll still be stuck with some configurations where the timeout 
> will be hit. And above needs careful testing.
> 
> One logic comment below as well, but I'll quote the whole patch to give
> context to Takashi.

I agree with Kai, we can just make the wait_for_completion_timeout()
split in a loop independently from hung_task_timeout.  It's simpler,
less error-prone.


thanks,

Takashi

Re: [Intel-gfx] [PATCH] ALSA: hda/i915 - avoid hung task timeout in i915 wait

[Takashi Iwai]

On Tue, 08 Mar 2022 17:29:21 +0100,
Amadeusz SX2awiX4ski wrote:
> 
> On 3/8/2022 3:11 PM, Kai Vehmanen wrote:
> > If kernel is built with hung task detection enabled and
> > CONFIG_DEFAULT_HUNG_TASK_TIMEOUT set to less than 60 seconds,
> > snd_hdac_i915_init() will trigger the hung task timeout in case i915 is
> > not available and taint the kernel.
> >
> > Split the 60sec wait into a loop of smaller waits to avoid this.
> >
> > Cc: Lucas De Marchi 
> > Co-developed-by: Ramalingam C 
> > Signed-off-by: Ramalingam C 
> > Signed-off-by: Kai Vehmanen 
> > ---
> >   sound/hda/hdac_i915.c | 10 ++
> >   1 file changed, 6 insertions(+), 4 deletions(-)
> >
> > diff --git a/sound/hda/hdac_i915.c b/sound/hda/hdac_i915.c
> > index 454474ac5716..6219de739b56 100644
> > --- a/sound/hda/hdac_i915.c
> > +++ b/sound/hda/hdac_i915.c
> > @@ -143,7 +143,8 @@ static bool i915_gfx_present(void)
> >   int snd_hdac_i915_init(struct hdac_bus *bus)
> >   {
> > struct drm_audio_component *acomp;
> > -   int err;
> > +   unsigned long wait = 0;
> 
> I'm not sure if it is best name for variable that is set to 0
> ("false"), maybe `done` would be better? Especially looking at
> condition in the following for loop.

Or, more simply some thing like

/* max 60s timeout */
for (i = 0; i < 60; i++) {
if 
(wait_for_completion_timeout(&acomp->master_bind_complete,
   
msecs_to_jiffies(1000)))
break;
}


Takashi

Re: [Intel-gfx] [PATCH v2] ALSA: hda/i915 - avoid hung task timeout in i915 wait

[Takashi Iwai]

On Wed, 09 Mar 2022 09:36:54 +0100,
Tvrtko Ursulin wrote:
> 
> 
> On 08/03/2022 17:27, Kai Vehmanen wrote:
> > If kernel is built with hung task detection enabled and
> > CONFIG_DEFAULT_HUNG_TASK_TIMEOUT set to less than 60 seconds,
> > snd_hdac_i915_init() will trigger the hung task timeout in case i915 is
> > not available and taint the kernel.
> >
> > Split the 60sec wait into a loop of smaller waits to avoid this.
> >
> > Reviewed-by: Lucas De Marchi 
> > Co-developed-by: Ramalingam C 
> > Signed-off-by: Ramalingam C 
> > Signed-off-by: Kai Vehmanen 
> > ---
> >   sound/hda/hdac_i915.c | 10 ++
> >   1 file changed, 6 insertions(+), 4 deletions(-)
> >
> > Changes V1->V2:
> >   - address local variable naming issue raised by Amadeusz
> > and use Takashi's proposal
> >
> > diff --git a/sound/hda/hdac_i915.c b/sound/hda/hdac_i915.c
> > index 454474ac5716..aa48bed7baf7 100644
> > --- a/sound/hda/hdac_i915.c
> > +++ b/sound/hda/hdac_i915.c
> > @@ -143,7 +143,7 @@ static bool i915_gfx_present(void)
> >   int snd_hdac_i915_init(struct hdac_bus *bus)
> >   {
> > struct drm_audio_component *acomp;
> > -   int err;
> > +   int err, i;
> > if (!i915_gfx_present())
> > return -ENODEV;
> > @@ -159,9 +159,11 @@ int snd_hdac_i915_init(struct hdac_bus *bus)
> > if (!acomp->ops) {
> > if (!IS_ENABLED(CONFIG_MODULES) ||
> > !request_module("i915")) {
> > -   /* 60s timeout */
> 
> Where does this 60s come from and why is the fix to work around
> DEFAULT_HUNG_TASK_TIMEOUT in a hacky way deemed okay?

The 60s timeout comes from the fact that the binding with i915 *might*
be mandatory for HD-audio driver on some platforms, while the binding
couldn't be achieved depending on the dynamic configuration change or
any other reasons, so we don't want to block forver.  And, basically
the hung check is false-positive, and if there is a better way to mark
to skip the hung check, we'd take it. But currently this seems to be
the easiest workaround for avoiding the false-positive checks.

> For instance
> would limiting the wait here to whatever the kconfig is set to be an
> option?

No, the hunk task timeout can be changed dynamically via procfs, hence
the fixed Kconfig won't help at all.


Takashi

Re: [Intel-gfx] [PATCH v2] ALSA: hda/i915 - avoid hung task timeout in i915 wait

[Takashi Iwai]

On Wed, 09 Mar 2022 10:02:13 +0100,
Tvrtko Ursulin wrote:
> 
> 
> On 09/03/2022 08:39, Kai Vehmanen wrote:
> > Hi,
> >
> > On Wed, 9 Mar 2022, Tvrtko Ursulin wrote:
> >
> >>> - /* 60s timeout */
> >>
> >> Where does this 60s come from and why is the fix to work around
> >> DEFAULT_HUNG_TASK_TIMEOUT in a hacky way deemed okay? For instance would
> >> limiting the wait here to whatever the kconfig is set to be an option?
> >
> > this was discussed in
> > https://lists.freedesktop.org/archives/intel-gfx/2022-February/290821.html
> > ... and that thread concluded it's cleaner to split the wait than try
> > to figure out hung-task configuration from middle of audio driver.
> >
> > The 60sec timeout comes from 2019 patch "ALSA: hda: Extend i915 component
> > bind timeout" to fix an issue reported by Paul Menzel (cc'ed).
> >
> > This patch keeps the timeout intact.
> 
> I did not spot discussion touching on the point I raised.
> 
> How about not fight the hung task detector but mark your wait context
> as "I really know what I'm doing - not stuck trust me".

The question is how often this problem hits.  Basically it's a very
corner case, and I even think we may leave as is; that's a matter of
configuration, and lowering such a bar should expect some
side-effect. OTOH, if the problem happens in many cases, it's
beneficial to fix in the core part, indeed.

> Maybe using
> wait_for_completion_killable_timeout would do it since
> snd_hdac_i915_init is allowed to fail with an error already?

It makes it killable -- which is a complete behavior change.


Takashi

Re: [Intel-gfx] [PATCH v2] ALSA: hda/i915 - avoid hung task timeout in i915 wait

[Takashi Iwai]

On Wed, 09 Mar 2022 10:48:49 +0100,
Tvrtko Ursulin wrote:
> 
> 
> On 09/03/2022 09:23, Takashi Iwai wrote:
> > On Wed, 09 Mar 2022 10:02:13 +0100,
> > Tvrtko Ursulin wrote:
> >>
> >>
> >> On 09/03/2022 08:39, Kai Vehmanen wrote:
> >>> Hi,
> >>>
> >>> On Wed, 9 Mar 2022, Tvrtko Ursulin wrote:
> >>>
> >>>>> -   /* 60s timeout */
> >>>>
> >>>> Where does this 60s come from and why is the fix to work around
> >>>> DEFAULT_HUNG_TASK_TIMEOUT in a hacky way deemed okay? For instance would
> >>>> limiting the wait here to whatever the kconfig is set to be an option?
> >>>
> >>> this was discussed in
> >>> https://lists.freedesktop.org/archives/intel-gfx/2022-February/290821.html
> >>> ... and that thread concluded it's cleaner to split the wait than try
> >>> to figure out hung-task configuration from middle of audio driver.
> >>>
> >>> The 60sec timeout comes from 2019 patch "ALSA: hda: Extend i915 component
> >>> bind timeout" to fix an issue reported by Paul Menzel (cc'ed).
> >>>
> >>> This patch keeps the timeout intact.
> >>
> >> I did not spot discussion touching on the point I raised.
> >>
> >> How about not fight the hung task detector but mark your wait context
> >> as "I really know what I'm doing - not stuck trust me".
> >
> > The question is how often this problem hits.  Basically it's a very
> > corner case, and I even think we may leave as is; that's a matter of
> > configuration, and lowering such a bar should expect some
> > side-effect. OTOH, if the problem happens in many cases, it's
> > beneficial to fix in the core part, indeed.
> 
> Yes argument you raise can be made I agree.
> 
> >> Maybe using
> >> wait_for_completion_killable_timeout would do it since
> >> snd_hdac_i915_init is allowed to fail with an error already?
> >
> > It makes it killable -- which is a complete behavior change.
> 
> Complete behaviour change how? Isn't this something ran on probe so
> likelihood of anyone sending SIGKILL to the modprobe process is only
> the init process? And in that case what is the fundamental difference
> in init giving up before the internal 60s in HDA driver does? I don't
> see a difference. Either party decided to abort the wait and code can
> just unwind and propagate the different error codes.

The point is that it does change the actual behavior, and changing the
actual behavior just for working around the corner case like the above
wouldn't be justified without the proper evaluation.

That said, if this behavior change is intentional and even desired,
that's a way to go.


Takashi

Re: [Intel-gfx] pcm_lock deadlock

[Takashi Iwai]

On Tue, 29 Oct 2019 20:10:50 +0100,
Ville Syrjälä wrote:
> 
> Hi Takashi,
> 
> I just got this deadlock when I tried to modprobe i915 on an ELK:
> 
> [  203.716416] 
> [  203.716417] WARNING: possible recursive locking detected
> [  203.716418] 5.4.0-rc5-elk+ #206 Not tainted
> [  203.716419] 
> [  203.716420] kworker/0:1/12 is trying to acquire lock:
> [  203.716421] efb1c138 (&spec->pcm_lock){+.+.}, at: 
> generic_hdmi_init+0x21/0x140 [snd_hda_codec_hdmi]
> [  203.716426] 
>but task is already holding lock:
> [  203.716427] efb1c138 (&spec->pcm_lock){+.+.}, at: 
> check_presence_and_report+0x67/0xb0 [snd_hda_codec_hdmi]
> [  203.716430] 
>other info that might help us debug this:
> [  203.716431]  Possible unsafe locking scenario:
> 
> [  203.716431]CPU0
> [  203.716432]
> [  203.716432]   lock(&spec->pcm_lock);
> [  203.716433]   lock(&spec->pcm_lock);
> [  203.716434] 
> *** DEADLOCK ***
> 
> [  203.716435]  May be due to missing lock nesting notation
> 
> [  203.716436] 3 locks held by kworker/0:1/12:
> [  203.716436]  #0: f14096a0 ((wq_completion)events){+.+.}, at: 
> process_one_work+0x1b8/0x530
> [  203.716442]  #1: f14dbf4c ((work_completion)(&bus->unsol_work)){+.+.}, at: 
> process_one_work+0x1b8/0x530
> [  203.716444]  #2: efb1c138 (&spec->pcm_lock){+.+.}, at: 
> check_presence_and_report+0x67/0xb0 [snd_hda_codec_hdmi]
> [  203.716448] 
>stack backtrace:
> [  203.716449] CPU: 0 PID: 12 Comm: kworker/0:1 Not tainted 5.4.0-rc5-elk+ 
> #206
> [  203.716450] Hardware name: System manufacturer P5Q-EM/P5Q-EM, BIOS 2203
> 07/08/2009
> [  203.716457] Workqueue: events snd_hdac_bus_process_unsol_events 
> [snd_hda_core]
> [  203.716459] Call Trace:
> [  203.716463]  dump_stack+0x66/0x8e
> [  203.716466]  __lock_acquire.cold.62+0x3bf/0x3c7
> [  203.716468]  ? mark_held_locks+0x3f/0x60
> [  203.716470]  ? _raw_spin_unlock_irq+0x22/0x30
> [  203.716478]  ? azx_rirb_get_response+0xd7/0x220 [snd_hda_codec]
> [  203.716479]  ? lockdep_hardirqs_on+0xec/0x1a0
> [  203.716480]  ? _raw_spin_unlock_irq+0x22/0x30
> [  203.716483]  ? trace_hardirqs_on+0x4a/0xf0
> [  203.716484]  ? find_held_lock+0x26/0xb0
> [  203.716486]  lock_acquire+0x74/0x150
> [  203.716488]  ? generic_hdmi_init+0x21/0x140 [snd_hda_codec_hdmi]
> [  203.716490]  __mutex_lock+0x60/0x810
> [  203.716492]  ? generic_hdmi_init+0x21/0x140 [snd_hda_codec_hdmi]
> [  203.716496]  ? snd_hdac_exec_verb+0x16/0x40 [snd_hda_core]
> [  203.716499]  ? codec_read+0x29/0x40 [snd_hda_core]
> [  203.716501]  mutex_lock_nested+0x14/0x20
> [  203.716503]  ? generic_hdmi_init+0x21/0x140 [snd_hda_codec_hdmi]
> [  203.716505]  generic_hdmi_init+0x21/0x140 [snd_hda_codec_hdmi]
> [  203.716507]  generic_hdmi_resume+0x18/0x60 [snd_hda_codec_hdmi]
> [  203.716512]  hda_call_codec_resume+0xc2/0x130 [snd_hda_codec]
> [  203.716517]  hda_codec_runtime_resume+0x2a/0x60 [snd_hda_codec]
> [  203.716520]  __rpm_callback+0x7a/0x140
> [  203.716524]  ? snd_hda_codec_device_new+0x2a0/0x2a0 [snd_hda_codec]
> [  203.716529]  ? snd_hda_codec_device_new+0x2a0/0x2a0 [snd_hda_codec]
> [  203.716531]  rpm_callback+0x1a/0x70
> [  203.716535]  ? snd_hda_codec_device_new+0x2a0/0x2a0 [snd_hda_codec]
> [  203.716537]  rpm_resume+0x52c/0x700
> [  203.716538]  ? _raw_spin_lock_irqsave+0x32/0x40
> [  203.716540]  __pm_runtime_resume+0x43/0x90
> [  203.716543]  snd_hdac_power_up_pm+0x4d/0x50 [snd_hda_core]
> [  203.716546]  hdmi_present_sense+0x34/0x340 [snd_hda_codec_hdmi]
> [  203.716548]  ? finish_task_switch+0x89/0x210
> [  203.716550]  check_presence_and_report+0x7a/0xb0 [snd_hda_codec_hdmi]
> [  203.716553]  hdmi_unsol_event+0x57/0x60 [snd_hda_codec_hdmi]
> [  203.716557]  ? hda_codec_match+0x70/0x70 [snd_hda_codec]
> [  203.716561]  hda_codec_unsol_event+0x12/0x20 [snd_hda_codec]
> [  203.716564]  snd_hdac_bus_process_unsol_events+0x51/0x60 [snd_hda_core]
> [  203.716566]  process_one_work+0x230/0x530
> [  203.716567]  worker_thread+0x37/0x410
> [  203.716569]  kthread+0xf5/0x110
> [  203.716570]  ? process_one_work+0x530/0x530
> [  203.716572]  ? kthread_create_worker_on_cpu+0x20/0x20
> [  203.716574]  ret_from_fork+0x2e/0x38
> 
> Looks like commit ade49db337a9 ("ALSA: hda/hdmi - Allow audio
> component for AMD/ATI and Nvidia HDMI") introduced pcm_lock
> to generic_hdmi_init().

Indeed, that can lead to a deadlock.
The patch below should address the issue.  I'm going to queue it
later.


thanks,

Takashi

-- 8< --
From: Takashi Iwai 
Subject: [PATCH] ALSA: hda - Fix mutex deadlock in H

Re: [Intel-gfx] pcm_lock deadlock

[Takashi Iwai]

On Wed, 30 Oct 2019 14:50:09 +0100,
Ville Syrjälä wrote:
> 
> On Tue, Oct 29, 2019 at 09:52:57PM +0100, Takashi Iwai wrote:
> > On Tue, 29 Oct 2019 20:10:50 +0100,
> > From: Takashi Iwai 
> > Subject: [PATCH] ALSA: hda - Fix mutex deadlock in HDMI codec driver
> > 
> > The commit ade49db337a9 ("ALSA: hda/hdmi - Allow audio component for
> > AMD/ATI and Nvidia HDMI") introduced the spec->pcm_lock mutex lock to
> > the whole generic_hdmi_init() function for avoiding the race with the
> > audio component registration.  However, this caused a dead lock when
> > the unsolicited event is handled without the audio component, as the
> > codec gets runtime-resumed in hdmi_present_sense() which is already
> > inside the spec->pcm_lock in its caller.
> > 
> > For avoiding this deadlock, add a new mutex only for the audio
> > component binding that is used in both generic_hdmi_init() and the
> > audio notifier registration where the jack callbacks are handled /
> > re-registered.
> > 
> > Fixes: ade49db337a9 ("ALSA: hda/hdmi - Allow audio component for AMD/ATI 
> > and Nvidia HDMI")
> > Reported-by: Ville Syrjälä 
> > Signed-off-by: Takashi Iwai 
> > ---
> >  sound/pci/hda/patch_hdmi.c | 9 +
> >  1 file changed, 5 insertions(+), 4 deletions(-)
> > 
> > diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
> > index 795cbda32cbb..d9b5ba361409 100644
> > --- a/sound/pci/hda/patch_hdmi.c
> > +++ b/sound/pci/hda/patch_hdmi.c
> > @@ -145,6 +145,7 @@ struct hdmi_spec {
> > struct snd_array pins; /* struct hdmi_spec_per_pin */
> > struct hdmi_pcm pcm_rec[16];
> > struct mutex pcm_lock;
> > +   struct mutex bind_lock; /* for audio component binding */
> 
> Missing mutex_init() for this guy.

Ouch, fixed now.

> Tested-by: Ville Syrjälä 

Thanks for quick testing.  Now pushed out the right version.
I'll include this to the next pull request to 5.4-rc.


Takashi
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] snd_hda_intel 0000:00:1f.3: No response from codec, resetting bus: last cmd=

[Takashi Iwai]

On Mon, 04 Nov 2019 14:10:24 +0100,
Mika Westerberg wrote:
> 
> Hi,
> 
> On Mon, Nov 04, 2019 at 01:57:54PM +0100, Paul Menzel wrote:
> > Dear Linux folks,
> > 
> > 
> > On the Dell XPS 13 9380 with Debian Sid/unstable with Linux 5.3.7
> > resuming0with Dell’s Thunderbolt TB16 dock connected, Linux spews
> > the errors below.
> 
> I have this machine here so can try to reproduce it as well.
> 
> > ```
> > [0.00] Linux version 5.3.0-1-amd64 (debian-ker...@lists.debian.org) 
> > (gcc version 9.2.1 20191008 (Debian 9.2.1-9)) #1 SMP Debian 5.3.7-1 
> > (2019-10-19)
> > […]
> > [1.596619] pci :00:1f.3: Adding to iommu group 12
> > [   14.536274] snd_hda_intel :00:1f.3: enabling device ( -> 0002)
> > [   14.544100] snd_hda_intel :00:1f.3: bound :00:02.0 (ops 
> > i915_audio_component_bind_ops [i915])
> > [   14.760751] input: HDA Intel PCH Headphone Mic as 
> > /devices/pci:00/:00:1f.3/sound/card0/input16
> > [   14.760790] input: HDA Intel PCH HDMI as 
> > /devices/pci:00/:00:1f.3/sound/card0/input17
> > [  156.614284] snd_hda_intel :00:1f.3: No response from codec, 
> > disabling MSI: last cmd=0x20270503
> > [  157.622232] snd_hda_intel :00:1f.3: No response from codec, 
> > resetting bus: last cmd=0x20270503
> > [  158.626371] snd_hda_intel :00:1f.3: No response from codec, 
> > resetting bus: last cmd=0x20370503
> > [  159.634102] snd_hda_intel :00:1f.3: No response from codec, 
> > resetting bus: last cmd=0x201f0500
> > [  161.678121] snd_hda_intel :00:1f.3: No response from codec, 
> > resetting bus: last cmd=0x20270503
> > [  162.682272] snd_hda_intel :00:1f.3: No response from codec, 
> > resetting bus: last cmd=0x20370503
> > [  163.694234] snd_hda_intel :00:1f.3: No response from codec, 
> > resetting bus: last cmd=0x201f0500
> > [  165.730142] snd_hda_intel :00:1f.3: No response from codec, 
> > resetting bus: last cmd=0x20270503
> > […]
> > ```
> > 
> > In the bug report *[Intel Ice Lake, snd-hda-intel, HDMI] "No
> > response from codec" (after display hotplug?)* [1], note it’s a
> > different model, Takashi comments that this is a Thunderbolt or
> > i915 issue.
> 
> :00:1f.3 is on PCH so not sure how it could be related to
> Thunderbolt, well or i915 for that matter.

It's the HD-audio controller PCI device and both HDMI and onboard
codecs are on that bus.  HDMI codec a shadow device of GPU, so it has
a strong dependency on i915 GPU driver.  The power of HD-audio bus and
codec is controlled over DRM audio component ops, so the power-on must
have been notified to GPU side, but still something seems missing.
ANd, with the dock, the other parties come to play into the game, so
it becomes more complex...


thanks,

Takashi
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] snd_hda_intel 0000:00:1f.3: No response from codec, resetting bus: last cmd=

[Takashi Iwai]

On Mon, 04 Nov 2019 15:58:25 +0100,
Mika Westerberg wrote:
> 
> On Mon, Nov 04, 2019 at 02:19:21PM +0100, Takashi Iwai wrote:
> > On Mon, 04 Nov 2019 14:10:24 +0100,
> > Mika Westerberg wrote:
> > > 
> > > Hi,
> > > 
> > > On Mon, Nov 04, 2019 at 01:57:54PM +0100, Paul Menzel wrote:
> > > > Dear Linux folks,
> > > > 
> > > > 
> > > > On the Dell XPS 13 9380 with Debian Sid/unstable with Linux 5.3.7
> > > > resuming0with Dell’s Thunderbolt TB16 dock connected, Linux spews
> > > > the errors below.
> > > 
> > > I have this machine here so can try to reproduce it as well.
> > > 
> > > > ```
> > > > [0.00] Linux version 5.3.0-1-amd64 
> > > > (debian-ker...@lists.debian.org) (gcc version 9.2.1 20191008 (Debian 
> > > > 9.2.1-9)) #1 SMP Debian 5.3.7-1 (2019-10-19)
> > > > […]
> > > > [1.596619] pci :00:1f.3: Adding to iommu group 12
> > > > [   14.536274] snd_hda_intel :00:1f.3: enabling device ( -> 
> > > > 0002)
> > > > [   14.544100] snd_hda_intel :00:1f.3: bound :00:02.0 (ops 
> > > > i915_audio_component_bind_ops [i915])
> > > > [   14.760751] input: HDA Intel PCH Headphone Mic as 
> > > > /devices/pci:00/:00:1f.3/sound/card0/input16
> > > > [   14.760790] input: HDA Intel PCH HDMI as 
> > > > /devices/pci:00/:00:1f.3/sound/card0/input17
> > > > [  156.614284] snd_hda_intel :00:1f.3: No response from codec, 
> > > > disabling MSI: last cmd=0x20270503
> > > > [  157.622232] snd_hda_intel :00:1f.3: No response from codec, 
> > > > resetting bus: last cmd=0x20270503
> > > > [  158.626371] snd_hda_intel :00:1f.3: No response from codec, 
> > > > resetting bus: last cmd=0x20370503
> > > > [  159.634102] snd_hda_intel :00:1f.3: No response from codec, 
> > > > resetting bus: last cmd=0x201f0500
> > > > [  161.678121] snd_hda_intel :00:1f.3: No response from codec, 
> > > > resetting bus: last cmd=0x20270503
> > > > [  162.682272] snd_hda_intel :00:1f.3: No response from codec, 
> > > > resetting bus: last cmd=0x20370503
> > > > [  163.694234] snd_hda_intel :00:1f.3: No response from codec, 
> > > > resetting bus: last cmd=0x201f0500
> > > > [  165.730142] snd_hda_intel :00:1f.3: No response from codec, 
> > > > resetting bus: last cmd=0x20270503
> > > > […]
> > > > ```
> > > > 
> > > > In the bug report *[Intel Ice Lake, snd-hda-intel, HDMI] "No
> > > > response from codec" (after display hotplug?)* [1], note it’s a
> > > > different model, Takashi comments that this is a Thunderbolt or
> > > > i915 issue.
> > > 
> > > :00:1f.3 is on PCH so not sure how it could be related to
> > > Thunderbolt, well or i915 for that matter.
> > 
> > It's the HD-audio controller PCI device and both HDMI and onboard
> > codecs are on that bus.  HDMI codec a shadow device of GPU, so it has
> > a strong dependency on i915 GPU driver.  The power of HD-audio bus and
> > codec is controlled over DRM audio component ops, so the power-on must
> > have been notified to GPU side, but still something seems missing.
> > ANd, with the dock, the other parties come to play into the game, so
> > it becomes more complex...
> 
> OK, thanks for explaining. Then I guess i915 may be related. However,
> that traffic for sure does not go over Thunderbolt fabric (PCIe and DP
> does).

Well, here HDMI codec I meant is for both HDMI and DP.  Its audio
device is common for both display types.


thanks,

Takashi
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [alsa-devel] USB Type-C monitor flashes once when play a video file after unplug and re-plug the monitor

[Takashi Iwai]

On Fri, 03 Jan 2020 02:57:03 +0100,
 wrote:
> 
> Hi Sirs,
> Here is chromebook SW team from Compal.
> As the mail title, we hit issue that the external monitor will flash once 
> when play video after hot pluging.
> We can reproduce not only on chromebook but also ubuntu 16.04.
> There has higher failure rate with Dell Solomon dock and Dell S2718D monitor.
> 
> We found adding the delay in "sound/pci/hda/patch_hdmi.c " can fix this 
> issue.(as the attachment)
> May need your help to review and advice. Thanks.
> 
> Here is the issue number in gitlab for more detail.
> https://gitlab.freedesktop.org/drm/intel/issues/318

Could you check whether it still happens with the latest upstream
kernel, at least 5.4.y, if it wasn't tested yet?

I don't want to put a long delay just because of random reason unless
it's really mandatory.  I'm wondering whether the recent write-sync
change improves the situation, so let's check the recent code.


thanks,

Takashi

> 
> 
> 
> AJ Cheng
> NID/NID1
> e-mail: aj_ch...@compal.com
> Tel:  +886-2-8797-8599 ext. 17561
> Mobile : +886-932827829
> COMPAL Electronics, Inc.
> 
> [2 flash_once.diff ]
> 
> ___
> Alsa-devel mailing list
> alsa-de...@alsa-project.org
> https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [alsa-devel] USB Type-C monitor flashes once when play a video file after unplug and re-plug the monitor

[Takashi Iwai]

On Tue, 07 Jan 2020 18:24:57 +0100,
Nathan Ciobanu wrote:
> 
> On Mon, Jan 06, 2020 at 08:08:04AM +, lucien_...@compal.com wrote:
> > Hi Takashi
> > 
> > We verified on Ubuntu 19.10 with kernel 5.4.0.0-050400-generic (please 
> > refer to attachment), the result is positive which symptom doesn't happen 
> > anymore once I played music or video sound output through Dell S2718D 
> > Type-C monitor. It seems had some fix in latest kernel.
> 
> Takashi, can you point to the patch series you suspect may have fixed this 
> issue? 

The first suspect would be
2756d9143aa517b97961e85412882b8ce31371a6
ALSA: hda - Fix intermittent CORB/RIRB stall on Intel chips


Takashi

> 
> Thanks,
> Nathan
> > 
> > Thanks.
> > 
> > 
> > -Original Message-
> > From: Takashi Iwai  
> > Sent: Friday, January 3, 2020 5:16 PM
> > To: Cheng. AJ (TPE) 
> > Cc: intel-gfx@lists.freedesktop.org; alsa-de...@alsa-project.org; 
> > nathan.d.ciob...@linux.intel.com; Wang. CindyXT (TPE) 
> > ; Ye. Nelson (TPE) ; Yap. 
> > Shane (TPE) ; Kao. Lucien (TPE) 
> > ; Tseng. Evan (TPE) 
> > Subject: Re: [alsa-devel] USB Type-C monitor flashes once when play a video 
> > file after unplug and re-plug the monitor
> > 
> > On Fri, 03 Jan 2020 02:57:03 +0100,
> >  wrote:
> > > 
> > > Hi Sirs,
> > > Here is chromebook SW team from Compal.
> > > As the mail title, we hit issue that the external monitor will flash once 
> > > when play video after hot pluging.
> > > We can reproduce not only on chromebook but also ubuntu 16.04.
> > > There has higher failure rate with Dell Solomon dock and Dell S2718D 
> > > monitor.
> > > 
> > > We found adding the delay in "sound/pci/hda/patch_hdmi.c " can fix 
> > > this issue.(as the attachment) May need your help to review and advice. 
> > > Thanks.
> > > 
> > > Here is the issue number in gitlab for more detail.
> > > https://gitlab.freedesktop.org/drm/intel/issues/318
> > 
> > Could you check whether it still happens with the latest upstream kernel, 
> > at least 5.4.y, if it wasn't tested yet?
> > 
> > I don't want to put a long delay just because of random reason unless it's 
> > really mandatory.  I'm wondering whether the recent write-sync change 
> > improves the situation, so let's check the recent code.
> > 
> > 
> > thanks,
> > 
> > Takashi
> > 
> > > 
> > > 
> > > 
> > > AJ Cheng
> > > NID/NID1
> > > e-mail: aj_ch...@compal.com<mailto:aj_ch...@compal.com>
> > > Tel:  +886-2-8797-8599 ext. 17561
> > > Mobile : +886-932827829
> > > COMPAL Electronics, Inc.
> > > 
> > > [2 flash_once.diff ]
> > > 
> > > ___
> > > Alsa-devel mailing list
> > > alsa-de...@alsa-project.org
> > > https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
> 
> 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [alsa-devel] USB Type-C monitor flashes once when play a video file after unplug and re-plug the monitor

[Takashi Iwai]

On Wed, 08 Jan 2020 04:07:17 +0100,
 wrote:
> 
> Hi Takashi
> 
> Is the attachment what you suspect? That merged to our kernel v4.19 already, 
> have any suggestions? Thanks.

Then I have no concrete idea.  It might be some other changes in HDMI
codec driver or it might be a fix in i915 graphics side, or even
thunderbolt or whatever, too...


Takashi

> 
> -Original Message-
> From: Takashi Iwai  
> Sent: Wednesday, January 8, 2020 2:57 AM
> To: Nathan Ciobanu 
> Cc: Kao. Lucien (TPE) ; Cheng. AJ (TPE) 
> ; intel-gfx@lists.freedesktop.org; 
> alsa-de...@alsa-project.org; Wang. CindyXT (TPE) ; 
> Ye. Nelson (TPE) ; Yap. Shane (TPE) 
> ; Tseng. Evan (TPE) 
> Subject: Re: [alsa-devel] USB Type-C monitor flashes once when play a video 
> file after unplug and re-plug the monitor
> 
> On Tue, 07 Jan 2020 18:24:57 +0100,
> Nathan Ciobanu wrote:
> > 
> > On Mon, Jan 06, 2020 at 08:08:04AM +, lucien_...@compal.com wrote:
> > > Hi Takashi
> > > 
> > > We verified on Ubuntu 19.10 with kernel 5.4.0.0-050400-generic (please 
> > > refer to attachment), the result is positive which symptom doesn't happen 
> > > anymore once I played music or video sound output through Dell S2718D 
> > > Type-C monitor. It seems had some fix in latest kernel.
> > 
> > Takashi, can you point to the patch series you suspect may have fixed this 
> > issue? 
> 
> The first suspect would be
> 2756d9143aa517b97961e85412882b8ce31371a6
> ALSA: hda - Fix intermittent CORB/RIRB stall on Intel chips
> 
> Takashi
> 
> > 
> > Thanks,
> > Nathan
> > > 
> > > Thanks.
> > > 
> > > 
> > > -Original Message-
> > > From: Takashi Iwai 
> > > Sent: Friday, January 3, 2020 5:16 PM
> > > To: Cheng. AJ (TPE) 
> > > Cc: intel-gfx@lists.freedesktop.org; alsa-de...@alsa-project.org; 
> > > nathan.d.ciob...@linux.intel.com; Wang. CindyXT (TPE) 
> > > ; Ye. Nelson (TPE) ; 
> > > Yap. Shane (TPE) ; Kao. Lucien (TPE) 
> > > ; Tseng. Evan (TPE) 
> > > Subject: Re: [alsa-devel] USB Type-C monitor flashes once when play 
> > > a video file after unplug and re-plug the monitor
> > > 
> > > On Fri, 03 Jan 2020 02:57:03 +0100,
> > >  wrote:
> > > > 
> > > > Hi Sirs,
> > > > Here is chromebook SW team from Compal.
> > > > As the mail title, we hit issue that the external monitor will flash 
> > > > once when play video after hot pluging.
> > > > We can reproduce not only on chromebook but also ubuntu 16.04.
> > > > There has higher failure rate with Dell Solomon dock and Dell S2718D 
> > > > monitor.
> > > > 
> > > > We found adding the delay in "sound/pci/hda/patch_hdmi.c " can fix 
> > > > this issue.(as the attachment) May need your help to review and advice. 
> > > > Thanks.
> > > > 
> > > > Here is the issue number in gitlab for more detail.
> > > > https://gitlab.freedesktop.org/drm/intel/issues/318
> > > 
> > > Could you check whether it still happens with the latest upstream kernel, 
> > > at least 5.4.y, if it wasn't tested yet?
> > > 
> > > I don't want to put a long delay just because of random reason unless 
> > > it's really mandatory.  I'm wondering whether the recent write-sync 
> > > change improves the situation, so let's check the recent code.
> > > 
> > > 
> > > thanks,
> > > 
> > > Takashi
> > > 
> > > > 
> > > > 
> > > > 
> > > > AJ Cheng
> > > > NID/NID1
> > > > e-mail: aj_ch...@compal.com<mailto:aj_ch...@compal.com>
> > > > Tel:  +886-2-8797-8599 ext. 17561
> > > > Mobile : +886-932827829
> > > > COMPAL Electronics, Inc.
> > > > 
> > > > [2 flash_once.diff ]
> > > > 
> > > > ___
> > > > Alsa-devel mailing list
> > > > alsa-de...@alsa-project.org
> > > > https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
> > 
> > 
> 
> ==
> =
> This message may contain information which is private, privileged or
> confidential of Compal Electronics, Inc.
> If you are not the intended recipient of this message, please notify the
> sender and destroy/delete the message.
> Any review, retransmission, dissemination or other use of, or taking of any
> action in reliance upon this information,
> by persons or entities other than the intended recipient is prohibited.
> ==
> =
> 
> 
> [1.2  ]
> 
> [2 0001-UPSTREAM-ALSA-hda-Fix-intermittent-CORB-RIRB-stall-o.patch 
> ]
> 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH] drm/i915/gt: Use scnprintf() for avoiding potential buffer overflow

[Takashi Iwai]

Since snprintf() returns the would-be-output size instead of the
actual output size, the succeeding calls may go beyond the given
buffer limit.  Fix it by replacing with scnprintf().

Signed-off-by: Takashi Iwai 
---
 drivers/gpu/drm/i915/gt/intel_engine_cs.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c 
b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
index 53ac3f00909a..418547bfe03b 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
@@ -1381,13 +1381,13 @@ static void intel_engine_print_registers(struct 
intel_engine_cs *engine,
char hdr[160];
int len;
 
-   len = snprintf(hdr, sizeof(hdr),
+   len = scnprintf(hdr, sizeof(hdr),
   "\t\tActive[%d]: ",
   (int)(port - execlists->active));
if (!i915_request_signaled(rq)) {
struct intel_timeline *tl = get_timeline(rq);
 
-   len += snprintf(hdr + len, sizeof(hdr) - len,
+   len += scnprintf(hdr + len, sizeof(hdr) - len,
"ring:{start:%08x, hwsp:%08x, 
seqno:%08x, runtime:%llums}, ",
i915_ggtt_offset(rq->ring->vma),
tl ? tl->hwsp_offset : 0,
@@ -1398,7 +1398,7 @@ static void intel_engine_print_registers(struct 
intel_engine_cs *engine,
if (tl)
intel_timeline_put(tl);
}
-   snprintf(hdr + len, sizeof(hdr) - len, "rq: ");
+   scnprintf(hdr + len, sizeof(hdr) - len, "rq: ");
print_request(m, rq, hdr);
}
for (port = execlists->pending; (rq = *port); port++) {
-- 
2.16.4

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2] drm/i915: Limit audio CDCLK>=2*BCLK constraint back to GLK only

[Takashi Iwai]

On Wed, 11 Mar 2020 13:16:56 +0100,
Kai Vehmanen wrote:
> 
> Hey,
> 
> On Tue, 10 Mar 2020, Takashi Iwai wrote:
> > On Tue, 10 Mar 2020 19:25:22 +0100, Ville Syrjälä wrote:
> >> On Tue, Mar 10, 2020 at 07:18:58PM +0200, Kai Vehmanen wrote:
> >>> One problematic scenario that this doesn't cover:
> >>>  - a single display is used (at low cdclk), and 
> >>>  - audio block goes to runtime suspend while display stays up. 
> >>> 
> >>> Upon resume (for e.g. UI notification sound), audio will initialize the 
> >>> HDA bus and call get_power() on i915, even if the notification goes to 
> >>> internal speaker. A modeset at this point is potentially very annoying.
> >> 
> >> :( That seems much harder to deal with.
> > 
> > I guess it doesn't happen -- at least with the legacy HD-audio and the
> > recent chip, if I understand correctly.  When the stream is on the
> > analog codec, the HDMI codec is kept closed / runtime-resumed.  And
> > the additional get_power() in the controller side is done only for
> > HSW/BDW (where the HDA-bus is dedicated to HDMI).
> 
> I'm afraid it does -- I double checked the legacy driver code. Judging 
> from history, since commit "ALSA: hda - Fix Skylake codec timeout", 
> azx_runtime_resume() has called "display_power(chip, true)" on all 
> platforms, even if the stream is on analog codec. I just recently fixed 
> this logic to SOF driver as well. If you don't do this and the link 
> parameters are different from hw defaults on i915 side (more likely with 
> ICL and newer), you'll hit problems.

Oh indeed, I overlooked that part.

> I don't currently know of a reliable way to recover. In theory, if HDMI/DP 
> monitor is connected, we could do a delayed call to i915 get_power and 
> reinitialize the HDA controller, but if we are actively streaming to 
> analog codec when this happens, this wouldn't work.
> 
> And until very recent times, this has not really been an issue. With 
> current i915, many conditions have to be met to actually hit this (only 
> affects GLK platforms, you need to be using HDA analog codec, runtime PM 
> needs to be enabled for HDA, etc). Problem is that going forward, there 
> are going to be more platforms that are affected and this starts to show 
> up in more places.

The remaining question is whether this display_power() call is still
needed for the recent chips.  Basically it's there for HSW/BDW type
chips that need already the power for the HDA link that is dedicated
to HDMI.  That is, a patch like below could work for the recent chips
that share both onboard and HDMI codecs.


Takashi

--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -992,9 +992,10 @@ static void __azx_runtime_resume(struct azx *chip, bool 
from_rt)
struct hda_codec *codec;
int status;
 
-   display_power(chip, true);
-   if (hda->need_i915_power)
+   if (hda->need_i915_power) {
+   display_power(chip, true);
snd_hdac_i915_set_bclk(bus);
+   }
 
/* Read STATESTS before controller reset */
status = azx_readw(chip, STATESTS);
@@ -1008,10 +1009,6 @@ static void __azx_runtime_resume(struct azx *chip, bool 
from_rt)
schedule_delayed_work(&codec->jackpoll_work,
  codec->jackpoll_interval);
}
-
-   /* power down again for link-controlled chips */
-   if (!hda->need_i915_power)
-   display_power(chip, false);
 }
 
 #ifdef CONFIG_PM_SLEEP

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2] drm/i915: Limit audio CDCLK>=2*BCLK constraint back to GLK only

[Takashi Iwai]

On Wed, 11 Mar 2020 18:04:24 +0100,
Kai Vehmanen wrote:
> 
> Hey,
> 
> On Wed, 11 Mar 2020, Takashi Iwai wrote:
> 
> > The remaining question is whether this display_power() call is still
> > needed for the recent chips.  Basically it's there for HSW/BDW type
> > chips that need already the power for the HDA link that is dedicated
> > to HDMI.  That is, a patch like below could work for the recent chips
> 
> yes, it is still needed. The newer chips (GLK, ICL and newer) have added 
> sw logic in i915 get_power() that must be run before audio controller is 
> initialized. So calling display_power() here is actually more critical 
> than before, power up from codec driver is too late. If it's not done, 
> you'll get verbs timing out (unless power management is disabled on i915 
> side).

Alright, then it's an unavoidable case.  Then it seems rather natural
to limit the CDCLK statically when the audio device is present on the
system, rather than too frequent up and down...


Takashi
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] snd/hda: Flush interrupts on disabling

[Takashi Iwai]

On Sat, 20 Jul 2019 13:33:37 +0200,
Chris Wilson wrote:
> 
> I was looking at
> 
> <4> [241.835158] general protection fault:  [#1] PREEMPT SMP PTI
> <4> [241.835181] CPU: 1 PID: 214 Comm: kworker/1:3 Tainted: G U   
>  5.2.0-CI-CI_DRM_6509+ #1
> <4> [241.835199] Hardware name: Dell Inc. OptiPlex 745
>  /0GW726, BIOS 2.3.1  05/21/2007
> <4> [241.835234] Workqueue: events snd_hdac_bus_process_unsol_events 
> [snd_hda_core]
> <4> [241.835256] RIP: 0010:input_handle_event+0x16d/0x5e0
> <4> [241.835270] Code: 48 8b 93 58 01 00 00 8b 52 08 89 50 04 8b 83 f8 06 00 
> 00 48 8b 93 00 07 00 00 8d 70 01 48 8d 04 c2 83 e1 08 89 b3 f8 06 00 00 <66> 
> 89 28 66 44 89 60 02 44 89 68 04 8b 93 f8 06 00 00 0f 84 fd fe
> <4> [241.835304] RSP: 0018:c919fda0 EFLAGS: 00010046
> <4> [241.835317] RAX: 6b6b6b6ec6c6c6c3 RBX: 8880290fefc8 RCX: 
> 
> <4> [241.835332] RDX: 6b6b6b6b RSI: 6b6b6b6c RDI: 
> 0046
> <4> [241.835347] RBP: 0005 R08:  R09: 
> 0001
> <4> [241.835362] R10: c919faa0 R11:  R12: 
> 0004
> <4> [241.835377] R13:  R14: 8880290ff1d0 R15: 
> 0293
> <4> [241.835392] FS:  () GS:88803de8() 
> knlGS:
> <4> [241.835409] CS:  0010 DS:  ES:  CR0: 80050033
> <4> [241.835422] CR2: 7ffe9a99e9b7 CR3: 2f588000 CR4: 
> 06e0
> <4> [241.835436] Call Trace:
> <4> [241.835449]  input_event+0x45/0x70
> <4> [241.835464]  snd_jack_report+0xdc/0x100
> <4> [241.835490]  snd_hda_jack_report_sync+0x83/0xc0 [snd_hda_codec]
> <4> [241.835512]  snd_hdac_bus_process_unsol_events+0x5a/0x70 [snd_hda_core]
> <4> [241.835530]  process_one_work+0x245/0x610
> 
> which has the hallmarks of a worker queued from interrupt after it was
> supposedly cancelled (note the POISON_FREE), and I could not see where
> the interrupt would be flushed on shutdown so added the likely suspects.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=74
> Signed-off-by: Chris Wilson 
> Cc: Takashi Iwai 
> Cc: Jaroslav Kysela 

OK, I'll queue this to my for-next branch, as it looks like a right
thing (although not sure whether it fixes actually).


thanks,

Takashi
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] Revert "ALSA: hda - Fix intermittent CORB/RIRB stall on Intel chips"

[Takashi Iwai]

On Thu, 25 Jul 2019 10:03:00 +0200,
Chris Wilson wrote:
> 
> Just a heads up that icl is consistently showing
> 
> <4> [315.478830] snd_hda_intel :00:1f.3: azx_get_response timeout, 
> switching to polling mode: last cmd=0x202f8100
> <4> [316.482799] snd_hda_intel :00:1f.3: No response from codec, 
> disabling MSI: last cmd=0x202f8100
> <3> [508.412915] snd_hda_codec_hdmi hdaudioC0D2: Unable to sync register 
> 0x2f8100. -11
> 
> following commits 2756d9143aa5 ("ALSA: hda - Fix intermittent CORB/RIRB
> stall on Intel chips") and a30f1743e4f5 ("ALSA: line6: sizeof (byte) is
> always 1, use that fact.")

The verb that stalls (0x202f8100) is a read verb (0xf81, Intel
vendor-specific verb for HDMI), so it shouldn't matter whether with or
without write sync, because it needs to read the response in anyway.

If that patch broke anything, it means that something else was already
broken.  Oh well, that ICL crap...

Is it about the runtime PM, or S3 or S4?  The only case we need to
re-issue this verb is only S4, I suppose, so we may skip that in most
cases.


thanks,

Takashi

> 
> Cc: Takashi Iwai 
> ---
>  sound/pci/hda/hda_intel.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> index 324a4b20fba9..fdde80d95966 100644
> --- a/sound/pci/hda/hda_intel.c
> +++ b/sound/pci/hda/hda_intel.c
> @@ -313,10 +313,11 @@ enum {
>  
>  #define AZX_DCAPS_INTEL_SKYLAKE \
>   (AZX_DCAPS_INTEL_PCH_BASE | AZX_DCAPS_PM_RUNTIME |\
> -  AZX_DCAPS_SYNC_WRITE |\
>AZX_DCAPS_SEPARATE_STREAM_TAG | AZX_DCAPS_I915_COMPONENT)
>  
> -#define AZX_DCAPS_INTEL_BROXTON  AZX_DCAPS_INTEL_SKYLAKE
> +#define AZX_DCAPS_INTEL_BROXTON \
> + (AZX_DCAPS_INTEL_PCH_BASE | AZX_DCAPS_PM_RUNTIME |\
> +  AZX_DCAPS_SEPARATE_STREAM_TAG | AZX_DCAPS_I915_COMPONENT)
>  
>  /* quirks for ATI SB / AMD Hudson */
>  #define AZX_DCAPS_PRESET_ATI_SB \
> -- 
> 2.22.0
> 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] Revert "ALSA: hda - Fix intermittent CORB/RIRB stall on Intel chips"

[Takashi Iwai]

On Thu, 25 Jul 2019 10:16:07 +0200,
Takashi Iwai wrote:
> 
> On Thu, 25 Jul 2019 10:03:00 +0200,
> Chris Wilson wrote:
> > 
> > Just a heads up that icl is consistently showing
> > 
> > <4> [315.478830] snd_hda_intel :00:1f.3: azx_get_response timeout, 
> > switching to polling mode: last cmd=0x202f8100
> > <4> [316.482799] snd_hda_intel :00:1f.3: No response from codec, 
> > disabling MSI: last cmd=0x202f8100
> > <3> [508.412915] snd_hda_codec_hdmi hdaudioC0D2: Unable to sync register 
> > 0x2f8100. -11
> > 
> > following commits 2756d9143aa5 ("ALSA: hda - Fix intermittent CORB/RIRB
> > stall on Intel chips") and a30f1743e4f5 ("ALSA: line6: sizeof (byte) is
> > always 1, use that fact.")
> 
> The verb that stalls (0x202f8100) is a read verb (0xf81, Intel
> vendor-specific verb for HDMI), so it shouldn't matter whether with or
> without write sync, because it needs to read the response in anyway.
> 
> If that patch broke anything, it means that something else was already
> broken.  Oh well, that ICL crap...
> 
> Is it about the runtime PM, or S3 or S4?  The only case we need to
> re-issue this verb is only S4, I suppose, so we may skip that in most
> cases.

Now checking the code, and I believe the workaround applied there can
be skipped for non-Haswell chips.  Could you try the patch below in
addition?


thanks,

Takashi

-- 8< --
From: Takashi Iwai 
Subject: [PATCH] ALSA: hda/hdmi - Avoid Haswell-specific power workaround on
 recent chips

The HDMI codec driver applies a vendor-specific verb before turning on
the AFG power on Intel chips.  This was needed for Haswell by some
reason, but apparently this breaks occasionally on other recent Intel
chips.

For papering over the problem, apply the workaround only for Haswell
chips.  The verb 0x781/f81 is restored via regcache later in anyway.

Cc: 
Signed-off-by: Takashi Iwai 
---
 sound/pci/hda/patch_hdmi.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
index 2096993eaf28..68ffe169cf52 100644
--- a/sound/pci/hda/patch_hdmi.c
+++ b/sound/pci/hda/patch_hdmi.c
@@ -2809,7 +2809,8 @@ static int intel_hsw_common_init(struct hda_codec *codec, 
hda_nid_t vendor_nid,
 
codec->display_power_control = 1;
 
-   codec->patch_ops.set_power_state = haswell_set_power_state;
+   if (is_haswell(codec))
+   codec->patch_ops.set_power_state = haswell_set_power_state;
codec->depop_delay = 0;
codec->auto_runtime_pm = 1;
 
-- 
2.16.4


___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] Revert "ALSA: hda - Fix intermittent CORB/RIRB stall on Intel chips"

[Takashi Iwai]

On Thu, 25 Jul 2019 12:21:11 +0200,
Chris Wilson wrote:
> 
> Quoting Chris Wilson (2019-07-25 09:30:25)
> > Quoting Takashi Iwai (2019-07-25 09:26:56)
> > > On Thu, 25 Jul 2019 10:16:07 +0200,
> > > Takashi Iwai wrote:
> > > > 
> > > > On Thu, 25 Jul 2019 10:03:00 +0200,
> > > > Chris Wilson wrote:
> > > > > 
> > > > > Just a heads up that icl is consistently showing
> > > > > 
> > > > > <4> [315.478830] snd_hda_intel :00:1f.3: azx_get_response 
> > > > > timeout, switching to polling mode: last cmd=0x202f8100
> > > > > <4> [316.482799] snd_hda_intel :00:1f.3: No response from codec, 
> > > > > disabling MSI: last cmd=0x202f8100
> > > > > <3> [508.412915] snd_hda_codec_hdmi hdaudioC0D2: Unable to sync 
> > > > > register 0x2f8100. -11
> > > > > 
> > > > > following commits 2756d9143aa5 ("ALSA: hda - Fix intermittent 
> > > > > CORB/RIRB
> > > > > stall on Intel chips") and a30f1743e4f5 ("ALSA: line6: sizeof (byte) 
> > > > > is
> > > > > always 1, use that fact.")
> > > > 
> > > > The verb that stalls (0x202f8100) is a read verb (0xf81, Intel
> > > > vendor-specific verb for HDMI), so it shouldn't matter whether with or
> > > > without write sync, because it needs to read the response in anyway.
> > > > 
> > > > If that patch broke anything, it means that something else was already
> > > > broken.  Oh well, that ICL crap...
> > > > 
> > > > Is it about the runtime PM, or S3 or S4?  The only case we need to
> > > > re-issue this verb is only S4, I suppose, so we may skip that in most
> > > > cases.
> > > 
> > > Now checking the code, and I believe the workaround applied there can
> > > be skipped for non-Haswell chips.  Could you try the patch below in
> > > addition?
> > 
> > Due to the way patchwork works, this patch will now be tested instead of
> > the revert. So watch this space.
> 
> Sadly, no change. Patchwork definitely lists this patch as being the one
> tested, but maybe send it separately just in case.

Hm, does the error indicate the same message ("last cmd=0x202f8100")?
If yes, we might need another workaround.  This is a special verb for
Intel with some black magic to communicate with GPU.  The tweak via
this verb is needed for other platforms to assure the enablement of
all pins and DP 1.2, but it might be incorrect for ICL.

Can anyone at Intel check whether the verb (0x781/0xf81) is still
valid for ICL?

Anyways below is a patch to leave the verb access for ICL.
Let's see...


thanks,

Takashi

-- 8< --
From: Takashi Iwai 
Subject: [PATCH] ALSA: hda/hdmi - Don't apply black magic to Icelake HDMI
 codecs

The Intel-specific verb to enable all pins and DP 1.2 seems causing a
problem on ICL by some reason.  Skip applying the verb for ICL as a
workaround.

Signed-off-by: Takashi Iwai 
---
 sound/pci/hda/patch_hdmi.c | 9 ++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
index 2096993eaf28..7e8236e5eac0 100644
--- a/sound/pci/hda/patch_hdmi.c
+++ b/sound/pci/hda/patch_hdmi.c
@@ -2804,12 +2804,15 @@ static int intel_hsw_common_init(struct hda_codec 
*codec, hda_nid_t vendor_nid,
spec->port_map = port_map;
spec->port_num = port_num;
 
-   intel_haswell_enable_all_pins(codec, true);
-   intel_haswell_fixup_enable_dp12(codec);
+   if (!is_icelake(codec)) {
+   intel_haswell_enable_all_pins(codec, true);
+   intel_haswell_fixup_enable_dp12(codec);
+   }
 
codec->display_power_control = 1;
 
-   codec->patch_ops.set_power_state = haswell_set_power_state;
+   if (!is_icelake(codec))
+   codec->patch_ops.set_power_state = haswell_set_power_state;
codec->depop_delay = 0;
codec->auto_runtime_pm = 1;
 
-- 
2.16.4


___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] Revert "ALSA: hda - Fix intermittent CORB/RIRB stall on Intel chips"

[Takashi Iwai]

On Thu, 25 Jul 2019 14:50:18 +0200,
Chris Wilson wrote:
> 
> Quoting Takashi Iwai (2019-07-25 11:44:08)
> > On Thu, 25 Jul 2019 12:21:11 +0200,
> > Chris Wilson wrote:
> > > 
> > > Quoting Chris Wilson (2019-07-25 09:30:25)
> > > > Quoting Takashi Iwai (2019-07-25 09:26:56)
> > > > > On Thu, 25 Jul 2019 10:16:07 +0200,
> > > > > Takashi Iwai wrote:
> > > > > > 
> > > > > > On Thu, 25 Jul 2019 10:03:00 +0200,
> > > > > > Chris Wilson wrote:
> > > > > > > 
> > > > > > > Just a heads up that icl is consistently showing
> > > > > > > 
> > > > > > > <4> [315.478830] snd_hda_intel :00:1f.3: azx_get_response 
> > > > > > > timeout, switching to polling mode: last cmd=0x202f8100
> > > > > > > <4> [316.482799] snd_hda_intel :00:1f.3: No response from 
> > > > > > > codec, disabling MSI: last cmd=0x202f8100
> > > > > > > <3> [508.412915] snd_hda_codec_hdmi hdaudioC0D2: Unable to sync 
> > > > > > > register 0x2f8100. -11
> > > > > > > 
> > > > > > > following commits 2756d9143aa5 ("ALSA: hda - Fix intermittent 
> > > > > > > CORB/RIRB
> > > > > > > stall on Intel chips") and a30f1743e4f5 ("ALSA: line6: sizeof 
> > > > > > > (byte) is
> > > > > > > always 1, use that fact.")
> > > > > > 
> > > > > > The verb that stalls (0x202f8100) is a read verb (0xf81, Intel
> > > > > > vendor-specific verb for HDMI), so it shouldn't matter whether with 
> > > > > > or
> > > > > > without write sync, because it needs to read the response in anyway.
> > > > > > 
> > > > > > If that patch broke anything, it means that something else was 
> > > > > > already
> > > > > > broken.  Oh well, that ICL crap...
> > > > > > 
> > > > > > Is it about the runtime PM, or S3 or S4?  The only case we need to
> > > > > > re-issue this verb is only S4, I suppose, so we may skip that in 
> > > > > > most
> > > > > > cases.
> > > > > 
> > > > > Now checking the code, and I believe the workaround applied there can
> > > > > be skipped for non-Haswell chips.  Could you try the patch below in
> > > > > addition?
> > > > 
> > > > Due to the way patchwork works, this patch will now be tested instead of
> > > > the revert. So watch this space.
> > > 
> > > Sadly, no change. Patchwork definitely lists this patch as being the one
> > > tested, but maybe send it separately just in case.
> > 
> > Hm, does the error indicate the same message ("last cmd=0x202f8100")?
> > If yes, we might need another workaround.  This is a special verb for
> > Intel with some black magic to communicate with GPU.  The tweak via
> > this verb is needed for other platforms to assure the enablement of
> > all pins and DP 1.2, but it might be incorrect for ICL.
> > 
> > Can anyone at Intel check whether the verb (0x781/0xf81) is still
> > valid for ICL?
> > 
> > Anyways below is a patch to leave the verb access for ICL.
> > Let's see...
> > 
> > 
> > thanks,
> > 
> > Takashi
> > 
> > -- 8< --
> > From: Takashi Iwai 
> > Subject: [PATCH] ALSA: hda/hdmi - Don't apply black magic to Icelake HDMI
> >  codecs
> > 
> > The Intel-specific verb to enable all pins and DP 1.2 seems causing a
> > problem on ICL by some reason.  Skip applying the verb for ICL as a
> > workaround.
> 
> Hmm. Nothing with icl-hda managed to boot. Coincidence?

No idea, honestly speaking, but it's a bad sign.  Let's scratch.


Takashi


> 
> > Signed-off-by: Takashi Iwai 
> > ---
> >  sound/pci/hda/patch_hdmi.c | 9 ++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> > 
> > diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
> > index 2096993eaf28..7e8236e5eac0 100644
> > --- a/sound/pci/hda/patch_hdmi.c
> > +++ b/sound/pci/hda/patch_hdmi.c
> > @@ -2804,12 +2804,15 @@ static int intel_hsw_common_init(struct hda_codec 
> > *codec, hda_nid_t vendor_nid,
> > spec->port_map = port_map;
> > spec->port_num = port_num;
> >  
> > -   intel_haswell_enable_all_pins(codec, true);
> > -   intel_haswell_fixup_enable_dp12(codec);
> > +   if (!is_icelake(codec)) {
> > +   intel_haswell_enable_all_pins(codec, true);
> > +   intel_haswell_fixup_enable_dp12(codec);
> > +   }
> >  
> > codec->display_power_control = 1;
> >  
> > -   codec->patch_ops.set_power_state = haswell_set_power_state;
> > +   if (!is_icelake(codec))
> > +   codec->patch_ops.set_power_state = haswell_set_power_state;
> > codec->depop_delay = 0;
> > codec->auto_runtime_pm = 1;
> >  
> > -- 
> > 2.16.4
> > 
> > 
> 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] Revert "ALSA: hda - Fix intermittent CORB/RIRB stall on Intel chips"

[Takashi Iwai]

On Thu, 25 Jul 2019 12:49:12 +0200,
Chris Wilson wrote:
> 
> Quoting Takashi Iwai (2019-07-25 11:44:08)
> > On Thu, 25 Jul 2019 12:21:11 +0200,
> > Chris Wilson wrote:
> > > 
> > > Quoting Chris Wilson (2019-07-25 09:30:25)
> > > > Quoting Takashi Iwai (2019-07-25 09:26:56)
> > > > > On Thu, 25 Jul 2019 10:16:07 +0200,
> > > > > Takashi Iwai wrote:
> > > > > > 
> > > > > > On Thu, 25 Jul 2019 10:03:00 +0200,
> > > > > > Chris Wilson wrote:
> > > > > > > 
> > > > > > > Just a heads up that icl is consistently showing
> > > > > > > 
> > > > > > > <4> [315.478830] snd_hda_intel :00:1f.3: azx_get_response 
> > > > > > > timeout, switching to polling mode: last cmd=0x202f8100
> > > > > > > <4> [316.482799] snd_hda_intel :00:1f.3: No response from 
> > > > > > > codec, disabling MSI: last cmd=0x202f8100
> > > > > > > <3> [508.412915] snd_hda_codec_hdmi hdaudioC0D2: Unable to sync 
> > > > > > > register 0x2f8100. -11
> > > > > > > 
> > > > > > > following commits 2756d9143aa5 ("ALSA: hda - Fix intermittent 
> > > > > > > CORB/RIRB
> > > > > > > stall on Intel chips") and a30f1743e4f5 ("ALSA: line6: sizeof 
> > > > > > > (byte) is
> > > > > > > always 1, use that fact.")
> > > > > > 
> > > > > > The verb that stalls (0x202f8100) is a read verb (0xf81, Intel
> > > > > > vendor-specific verb for HDMI), so it shouldn't matter whether with 
> > > > > > or
> > > > > > without write sync, because it needs to read the response in anyway.
> > > > > > 
> > > > > > If that patch broke anything, it means that something else was 
> > > > > > already
> > > > > > broken.  Oh well, that ICL crap...
> > > > > > 
> > > > > > Is it about the runtime PM, or S3 or S4?  The only case we need to
> > > > > > re-issue this verb is only S4, I suppose, so we may skip that in 
> > > > > > most
> > > > > > cases.
> > > > > 
> > > > > Now checking the code, and I believe the workaround applied there can
> > > > > be skipped for non-Haswell chips.  Could you try the patch below in
> > > > > addition?
> > > > 
> > > > Due to the way patchwork works, this patch will now be tested instead of
> > > > the revert. So watch this space.
> > > 
> > > Sadly, no change. Patchwork definitely lists this patch as being the one
> > > tested, but maybe send it separately just in case.
> > 
> > Hm, does the error indicate the same message ("last cmd=0x202f8100")?
> 
> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13745/fi-icl-u2/igt@i915_module_l...@reload.html
> <4> [383.858354] snd_hda_intel :00:1f.3: azx_get_response timeout, 
> switching to polling mode: last cmd=0x20170500
> <4> [384.860261] snd_hda_intel :00:1f.3: No response from codec, 
> disabling MSI: last cmd=0x20170500
> <3> [556.636243] snd_hda_codec_hdmi hdaudioC0D2: Unable to sync register 
> 0x2f8100. -11

Looking at the logs around this, you can find:

<7>[  380.741747] [IGT] i915_module_load: executing
<7>[  380.745788] [IGT] i915_module_load: starting subtest reload
<4>[  383.858354] snd_hda_intel :00:1f.3: azx_get_response timeout, 
switching to polling mode: last cmd=0x20170500
<4>[  384.860261] snd_hda_intel :00:1f.3: No response from codec, disabling 
MSI: last cmd=0x20170500
<3>[  556.636243] snd_hda_codec_hdmi hdaudioC0D2: Unable to sync register 
0x2f8100. -11
<3>[  556.636243] snd_hda_codec_hdmi hdaudioC0D2: Unable to sync register 
0x2f8100. -11
<7>[  556.636556] [drm:i915_audio_component_get_eld [i915]] Not valid for port B
<7>[  556.636681] [drm:i915_audio_component_get_eld [i915]] Not valid for port B
<7>[  556.636775] [drm:i915_audio_component_get_eld [i915]] Not valid for port B
<7>[  556.636865] [drm:i915_audio_component_get_eld [i915]] Not valid for port C
<7>[  556.636959] [drm:i915_audio_component_get_eld [i915]] Not valid for port C
<7>[  556.637042] [drm:i915_audio_component_get_eld [i915]] Not valid for port C
<7>[  556.637134] [drm:i915_audio_component_get_eld [i915]] Not valid for port D
<7>[  556.637312] [drm:i915_audio_component_get_eld [

Re: [Intel-gfx] [PATCH] Revert "ALSA: hda - Fix intermittent CORB/RIRB stall on Intel chips"

[Takashi Iwai]

On Thu, 25 Jul 2019 15:57:10 +0200,
Chris Wilson wrote:
> 
> Quoting Takashi Iwai (2019-07-25 14:45:10)
> > On Thu, 25 Jul 2019 12:49:12 +0200,
> > Chris Wilson wrote:
> > > https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13745/fi-icl-u2/igt@i915_module_l...@reload.html
> > > <4> [383.858354] snd_hda_intel :00:1f.3: azx_get_response timeout, 
> > > switching to polling mode: last cmd=0x20170500
> > > <4> [384.860261] snd_hda_intel :00:1f.3: No response from codec, 
> > > disabling MSI: last cmd=0x20170500
> > > <3> [556.636243] snd_hda_codec_hdmi hdaudioC0D2: Unable to sync register 
> > > 0x2f8100. -11
> > 
> > Looking at the logs around this, you can find:
> > 
> > <7>[  380.741747] [IGT] i915_module_load: executing
> > <7>[  380.745788] [IGT] i915_module_load: starting subtest reload
> > <4>[  383.858354] snd_hda_intel :00:1f.3: azx_get_response timeout, 
> > switching to polling mode: last cmd=0x20170500
> > <4>[  384.860261] snd_hda_intel :00:1f.3: No response from codec, 
> > disabling MSI: last cmd=0x20170500
> > <3>[  556.636243] snd_hda_codec_hdmi hdaudioC0D2: Unable to sync register 
> > 0x2f8100. -11
> > <3>[  556.636243] snd_hda_codec_hdmi hdaudioC0D2: Unable to sync register 
> > 0x2f8100. -11
> > <7>[  556.636556] [drm:i915_audio_component_get_eld [i915]] Not valid for 
> > port B
> > <7>[  556.636681] [drm:i915_audio_component_get_eld [i915]] Not valid for 
> > port B
> > <7>[  556.636775] [drm:i915_audio_component_get_eld [i915]] Not valid for 
> > port B
> > <7>[  556.636865] [drm:i915_audio_component_get_eld [i915]] Not valid for 
> > port C
> > <7>[  556.636959] [drm:i915_audio_component_get_eld [i915]] Not valid for 
> > port C
> > <7>[  556.637042] [drm:i915_audio_component_get_eld [i915]] Not valid for 
> > port C
> > <7>[  556.637134] [drm:i915_audio_component_get_eld [i915]] Not valid for 
> > port D
> > <7>[  556.637312] [drm:i915_audio_component_get_eld [i915]] Not valid for 
> > port D
> > <7>[  556.637445] [drm:i915_audio_component_get_eld [i915]] Not valid for 
> > port E
> > <7>[  556.637557] [drm:i915_audio_component_get_eld [i915]] Not valid for 
> > port E
> > <7>[  556.637664] [drm:i915_audio_component_get_eld [i915]] Not valid for 
> > port E
> > <7>[  556.637751] [drm:i915_audio_component_get_eld [i915]] Not valid for 
> > port F
> > <7>[  556.637825] [drm:i915_audio_component_get_eld [i915]] Not valid for 
> > port F
> > <7>[  556.637900] [drm:i915_audio_component_get_eld [i915]] Not valid for 
> > port F
> > <7>[  556.679134] [IGT] i915_module_load: executing
> > <7>[  556.681585] [IGT] i915_module_load: starting subtest reload-no-display
> > 
> > What does it actually do?  First off, there is a big gap in the
> > timestamps between 384 and 556.
> 
> Therein is where our current problem lies. Looking at the run just before
> this pair of commits,
> 
> <6> [405.838716] [IGT] i915_module_load: executing
> <6> [405.841651] [IGT] i915_module_load: starting subtest reload
> <4> [408.976245] snd_hda_intel :00:1f.3: azx_get_response timeout, 
> switching to polling mode: last cmd=0x202f8100
> <4> [409.980171] snd_hda_intel :00:1f.3: No response from codec, 
> disabling MSI: last cmd=0x202f8100
> <3> [410.985180] snd_hda_intel :00:1f.3: azx_get_response timeout, 
> switching to single_cmd mode: last cmd=0x202f8100
> <3> [411.227736] snd_hda_codec_hdmi hdaudioC0D2: Unable to sync register 
> 0x2f8100. -5
> <7> [411.227849] [drm:i915_audio_component_get_eld [i915]] Not valid for port 
> B
> <7> [411.227886] [drm:i915_audio_component_get_eld [i915]] Not valid for port 
> B
> <7> [411.227917] [drm:i915_audio_component_get_eld [i915]] Not valid for port 
> C
> <7> [411.227947] [drm:i915_audio_component_get_eld [i915]] Not valid for port 
> C
> <7> [411.228004] [drm:i915_audio_component_get_eld [i915]] Not valid for port 
> C
> <7> [411.228041] [drm:i915_audio_component_get_eld [i915]] Not valid for port 
> D
> <7> [411.228077] [drm:i915_audio_component_get_eld [i915]] Not valid for port 
> D
> <7> [411.228125] [drm:i915_audio_component_get_eld [i915]] Not valid for port 
> D
> <7> [411.228160] [drm:i915_audio_component_get_eld [i915]] Not valid for port 
> E
> <7> [411.228187] [drm:i915_audio_component_get_eld [i915]] Not valid for port 
> E
> <7>

Re: [Intel-gfx] [PATCH] Revert "ALSA: hda - Drop unsol event handler for Intel HDMI codecs"

[Takashi Iwai]

On Thu, 15 Aug 2019 20:15:51 +0200,
Chris Wilson wrote:
> 
> In the recent snd merge of
> 
> ddf7cb83b0f4 ALSA: hda: Unexport a few more stuff
> 53eff75e5f4d ALSA: hda: Drop export of snd_hdac_bus_add/remove_device()
> ee5f85d9290f ALSA: hda: Add codec on bus address table lately
> f2dbe87c5ac1 ALSA: hda - Drop unsol event handler for Intel HDMI codecs
> 
> module unload dies on all a couple of machines with
> 
> <1> [281.912684] BUG: kernel NULL pointer dereference, address: 
> 
> <1> [281.912697] #PF: supervisor read access in kernel mode
> <1> [281.912703] #PF: error_code(0x) - not-present page
> <6> [281.912709] PGD 0 P4D 0
> <4> [281.912714] Oops:  [#1] PREEMPT SMP PTI
> <4> [281.912721] CPU: 3 PID: 2842 Comm: i915_module_loa Tainted: G U  
>   5.3.0-rc4-CI-CI_DRM_6712+ #1
> <4> [281.912730] Hardware name: Hewlett-Packard HP EliteBook 8440p/172A, BIOS 
> 68CCU Ver. F.24 09/13/2013
> <4> [281.912744] RIP: 0010:__list_del_entry_valid+0x25/0x90
> <4> [281.912751] Code: c3 0f 1f 40 00 48 8b 07 48 b9 00 01 00 00 00 00 ad de 
> 48 8b 57 08 48 39 c8 74 26 48 b9 22 01 00 00 00 00 ad de 48 39 ca 74 2e <48> 
> 8b 32 48 39 fe 75 3a 48 8b 50 08 48 39 f2 75 48 b8 01 00 00 00
> <4> [281.912767] RSP: 0018:c972bca8 EFLAGS: 00010217
> <4> [281.912774] RAX:  RBX: 88812f8933f8 RCX: 
> dead0122
> <4> [281.912782] RDX:  RSI: 88812f8933f8 RDI: 
> 88812f8938a8
> <4> [281.912789] RBP: 88812e7ce7e8 R08:  R09: 
> 
> <4> [281.912796] R10:  R11:  R12: 
> 88812f8938a8
> <4> [281.912803] R13: 88812c8722a8 R14: c972bf08 R15: 
> 888129761df8
> <4> [281.912811] FS:  7fb4913a9e40() GS:888133b8() 
> knlGS:
> <4> [281.912819] CS:  0010 DS:  ES:  CR0: 80050033
> <4> [281.912826] CR2:  CR3: 00012867a000 CR4: 
> 06e0
> <4> [281.912833] Call Trace:
> <4> [281.912844]  snd_hdac_bus_remove_device+0x2e/0xb0 [snd_hda_core]
> <4> [281.912854]  snd_hdac_device_exit+0x31/0x60 [snd_hda_core]
> <4> [281.912866]  snd_hda_codec_dev_release+0x24/0x50 [snd_hda_codec]
> <4> [281.912876]  device_release+0x22/0x80
> <4> [281.912883]  kobject_put+0x86/0x1b0
> <4> [281.912891]  snd_hda_codec_dev_free+0x5c/0x60 [snd_hda_codec]
> <4> [281.912899]  __snd_device_free+0x4a/0x80
> <4> [281.912904]  snd_device_free_all+0x36/0x90
> <4> [281.912911]  release_card_device+0x14/0x60
> <4> [281.912917]  device_release+0x22/0x80
> <4> [281.912923]  kobject_put+0x86/0x1b0
> <4> [281.912928]  snd_card_free+0x60/0x90
> <4> [281.912939]  pci_device_remove+0x36/0xb0
> <4> [281.912946]  device_release_driver_internal+0xd3/0x1b0
> <4> [281.912953]  unbind_store+0xc3/0x120
> <4> [281.912962]  kernfs_fop_write+0x104/0x190
> <4> [281.912971]  vfs_write+0xbd/0x1d0
> <4> [281.912977]  ksys_write+0x8f/0xe0
> <4> [281.912984]  do_syscall_64+0x55/0x1c0
> 
> Cc: Takashi Iwai 

Gah, that's a breakage by the commit ee5f85d9290f ("ALSA: hda: Add
codec on bus address table lately").  Please revert it in your tree,
I'll do it same on sound git tree now.


thanks,

Takashi
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH] Revert "ALSA: hda: Add codec on bus address table lately"

[Takashi Iwai]

This reverts commit ee5f85d9290f ("ALSA: hda: Add codec on bus address
table lately").  The commit caused several regression since I've
overlooked that the function doesn't manage only the caddr_tbl but
also the codec linked list that is referred indirectly in the other
drivers.

Revert for now to make everything back to work.

Fixes: ee5f85d9290f ("ALSA: hda: Add codec on bus address table lately")
Reported-by: Chris Wilson 
Signed-off-by: Takashi Iwai 
---
 sound/hda/hdac_device.c | 21 +
 1 file changed, 9 insertions(+), 12 deletions(-)

diff --git a/sound/hda/hdac_device.c b/sound/hda/hdac_device.c
index bf83d7062ef6..9f3e37511408 100644
--- a/sound/hda/hdac_device.c
+++ b/sound/hda/hdac_device.c
@@ -61,6 +61,10 @@ int snd_hdac_device_init(struct hdac_device *codec, struct 
hdac_bus *bus,
pm_runtime_get_noresume(&codec->dev);
atomic_set(&codec->in_pm, 0);
 
+   err = snd_hdac_bus_add_device(bus, codec);
+   if (err < 0)
+   goto error;
+
/* fill parameters */
codec->vendor_id = snd_hdac_read_parm(codec, AC_NODE_ROOT,
  AC_PAR_VENDOR_ID);
@@ -139,22 +143,15 @@ int snd_hdac_device_register(struct hdac_device *codec)
err = device_add(&codec->dev);
if (err < 0)
return err;
-   err = snd_hdac_bus_add_device(codec->bus, codec);
-   if (err < 0)
-   goto error;
mutex_lock(&codec->widget_lock);
err = hda_widget_sysfs_init(codec);
mutex_unlock(&codec->widget_lock);
-   if (err < 0)
-   goto error_remove;
+   if (err < 0) {
+   device_del(&codec->dev);
+   return err;
+   }
 
return 0;
-
- error_remove:
-   snd_hdac_bus_remove_device(codec->bus, codec);
- error:
-   device_del(&codec->dev);
-   return err;
 }
 EXPORT_SYMBOL_GPL(snd_hdac_device_register);
 
@@ -168,8 +165,8 @@ void snd_hdac_device_unregister(struct hdac_device *codec)
mutex_lock(&codec->widget_lock);
hda_widget_sysfs_exit(codec);
mutex_unlock(&codec->widget_lock);
-   snd_hdac_bus_remove_device(codec->bus, codec);
device_del(&codec->dev);
+   snd_hdac_bus_remove_device(codec->bus, codec);
}
 }
 EXPORT_SYMBOL_GPL(snd_hdac_device_unregister);
-- 
2.16.4

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [alsa-devel] USB Type-C monitor flashes once when play a video file after unplug and re-plug the monitor

[Takashi Iwai]

On Wed, 12 Feb 2020 10:37:30 +0100,
 wrote:
> 
> Hi Takashi
> 
> Sorry to bother you again, during this period I do some experiment then issue 
> may be fixed by my local patch, but I am not sure, however may I have your 
> suggestion if you are available.
> I put snd_hda_codec_read and snd_hda_codec_set_power_to_all ahead then add 
> debug log in function haswell_verify_D0 I found my changes cause cvt_nid and 
> nid could get "power state: 1" every single time after re-plug type-c cable 
> then playing music, you can see flash_once_fixed_messages in details. But 
> without my patch which nid gets "power state: 0" and external type-c monitor 
> flashes once after playing music. Assume my finding is right, do we have 
> chance to submit code to fix issue? Thanks.

Is the issue reproduced with the latest upstream code (or at least
5.4.y stable)?
Then yeah, we need to take a deeper look.


thanks,

Takashi

> 
> With patch
> 2020-02-12T00:58:27.496016-08:00 ERR kernel: [   47.729823] @@@ cvt_nid power 
> state: 1
> 2020-02-12T00:58:27.496024-08:00 ERR kernel: [   47.729883] @@@ nid power 
> state: 1
> 
> Without patch
> 2020-02-12T00:55:36.795016-08:00 ERR kernel: [  125.047502] @@@ cvt_nid power 
> state: 1
> 2020-02-12T00:55:36.795026-08:00 ERR kernel: [  125.047565] @@@ nid power 
> state: 0
> 
> -Original Message-
> From: Takashi Iwai  
> Sent: Wednesday, January 8, 2020 2:18 PM
> To: Kao. Lucien (TPE) 
> Cc: nathan.d.ciob...@linux.intel.com; Cheng. AJ (TPE) ; 
> intel-gfx@lists.freedesktop.org; alsa-de...@alsa-project.org; Wang. CindyXT 
> (TPE) ; Ye. Nelson (TPE) ; 
> Yap. Shane (TPE) ; Tseng. Evan (TPE) 
> 
> Subject: Re: [alsa-devel] USB Type-C monitor flashes once when play a video 
> file after unplug and re-plug the monitor
> 
> On Wed, 08 Jan 2020 04:07:17 +0100,
>  wrote:
> > 
> > Hi Takashi
> > 
> > Is the attachment what you suspect? That merged to our kernel v4.19 
> > already, have any suggestions? Thanks.
> 
> Then I have no concrete idea.  It might be some other changes in HDMI codec 
> driver or it might be a fix in i915 graphics side, or even thunderbolt or 
> whatever, too...
> 
> Takashi
> 
> > 
> > -Original Message-
> > From: Takashi Iwai 
> > Sent: Wednesday, January 8, 2020 2:57 AM
> > To: Nathan Ciobanu 
> > Cc: Kao. Lucien (TPE) ; Cheng. AJ (TPE) 
> > ; intel-gfx@lists.freedesktop.org; 
> > alsa-de...@alsa-project.org; Wang. CindyXT (TPE) 
> > ; Ye. Nelson (TPE) ; 
> > Yap. Shane (TPE) ; Tseng. Evan (TPE) 
> > 
> > Subject: Re: [alsa-devel] USB Type-C monitor flashes once when play a 
> > video file after unplug and re-plug the monitor
> > 
> > On Tue, 07 Jan 2020 18:24:57 +0100,
> > Nathan Ciobanu wrote:
> > > 
> > > On Mon, Jan 06, 2020 at 08:08:04AM +, lucien_...@compal.com wrote:
> > > > Hi Takashi
> > > > 
> > > > We verified on Ubuntu 19.10 with kernel 5.4.0.0-050400-generic (please 
> > > > refer to attachment), the result is positive which symptom doesn't 
> > > > happen anymore once I played music or video sound output through Dell 
> > > > S2718D Type-C monitor. It seems had some fix in latest kernel.
> > > 
> > > Takashi, can you point to the patch series you suspect may have fixed 
> > > this issue? 
> > 
> > The first suspect would be
> > 2756d9143aa517b97961e85412882b8ce31371a6
> > ALSA: hda - Fix intermittent CORB/RIRB stall on Intel chips
> > 
> > Takashi
> > 
> > > 
> > > Thanks,
> > > Nathan
> > > > 
> > > > Thanks.
> > > > 
> > > > 
> > > > -Original Message-
> > > > From: Takashi Iwai 
> > > > Sent: Friday, January 3, 2020 5:16 PM
> > > > To: Cheng. AJ (TPE) 
> > > > Cc: intel-gfx@lists.freedesktop.org; alsa-de...@alsa-project.org; 
> > > > nathan.d.ciob...@linux.intel.com; Wang. CindyXT (TPE) 
> > > > ; Ye. Nelson (TPE) 
> > > > ; Yap. Shane (TPE) ; 
> > > > Kao. Lucien (TPE) ; Tseng. Evan (TPE) 
> > > > 
> > > > Subject: Re: [alsa-devel] USB Type-C monitor flashes once when 
> > > > play a video file after unplug and re-plug the monitor
> > > > 
> > > > On Fri, 03 Jan 2020 02:57:03 +0100,  wrote:
> > > > > 
> > > > > Hi Sirs,
> > > > > Here is chromebook SW team from Compal.
> > > > > As the mail title, we hit issue that the external monitor will flash 
> > > > 

Re: [Intel-gfx] [PATCH] drm/i915/ilk-glk: Fix link training on links with LTTPRs

[Takashi Iwai]

On Tue, 16 Mar 2021 17:54:26 +0100,
Imre Deak wrote:
> 
> The spec requires to use at least 3.2ms for the AUX timeout period if
> there are LT-tunable PHY Repeaters on the link (2.11.2). An upcoming
> spec update makes this more specific, by requiring a 3.2ms minimum
> timeout period for the LTTPR detection reading the 0xF-0xF0007
> range (3.6.5.1).
> 
> Accordingly disable LTTPR detection until GLK, where the maximum timeout
> we can set is only 1.6ms.
> 
> Link training in the non-transparent mode is known to fail at least on
> some SKL systems with a WD19 dock on the link, which exposes an LTTPR
> (see the References below). While this could have different reasons
> besides the too short AUX timeout used, not detecting LTTPRs (and so not
> using the non-transparent LT mode) fixes link training on these systems.
> 
> While at it add a code comment about the platform specific maximum
> timeout values.
> 
> Reported-by: Takashi Iwai 
> References: https://gitlab.freedesktop.org/drm/intel/-/issues/3166
> Fixes: b30edfd8d0b4 ("drm/i915: Switch to LTTPR non-transparent mode link 
> training")
> Cc:  # v5.11
> Cc: Takashi Iwai 
> Signed-off-by: Imre Deak 

Thanks!  I'm now building a test kernel and ask people testing it.
  https://apibugzilla.suse.com/show_bug.cgi?id=1183294

BTW, the patch isn't applicable cleanly to 5.11.x kernel as is.
The file intel_dp_aux.c needs to be changed to intel_dp.c.
Hopefully Greg can manage the file rename.


Takashi

> ---
>  drivers/gpu/drm/i915/display/intel_dp_aux.c   | 7 +++
>  drivers/gpu/drm/i915/display/intel_dp_link_training.c | 8 
>  2 files changed, 15 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_aux.c 
> b/drivers/gpu/drm/i915/display/intel_dp_aux.c
> index eaebf123310a..b581e8acce07 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_aux.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_aux.c
> @@ -133,6 +133,7 @@ static u32 g4x_get_aux_send_ctl(struct intel_dp *intel_dp,
>   else
>   precharge = 5;
>  
> + /* Max timeout value on ILK-BDW: 1.6ms */
>   if (IS_BROADWELL(dev_priv))
>   timeout = DP_AUX_CH_CTL_TIME_OUT_600us;
>   else
> @@ -159,6 +160,12 @@ static u32 skl_get_aux_send_ctl(struct intel_dp 
> *intel_dp,
>   enum phy phy = intel_port_to_phy(i915, dig_port->base.port);
>   u32 ret;
>  
> + /*
> +  * Max timeout values:
> +  * SKL-GLK: 1.6ms
> +  * CNL: 3.2ms
> +  * ICL+: 4ms
> +  */
>   ret = DP_AUX_CH_CTL_SEND_BUSY |
> DP_AUX_CH_CTL_DONE |
> DP_AUX_CH_CTL_INTERRUPT |
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_link_training.c 
> b/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> index 19ba7c7cbaab..de6d70a29b47 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> @@ -123,10 +123,18 @@ intel_dp_set_lttpr_transparent_mode(struct intel_dp 
> *intel_dp, bool enable)
>   */
>  int intel_dp_lttpr_init(struct intel_dp *intel_dp)
>  {
> + struct drm_i915_private *i915 = dp_to_i915(intel_dp);
>   int lttpr_count;
>   bool ret;
>   int i;
>  
> + /*
> +  * Detecting LTTPRs must be avoided on platforms with an AUX timeout
> +  * period < 3.2ms. (see DP Standard v2.0, 2.11.2, 3.6.6.1).
> +  */
> + if (INTEL_GEN(i915) < 10)
> + return 0;
> +
>   if (intel_dp_is_edp(intel_dp))
>   return 0;
>  
> -- 
> 2.25.1
> 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] ALSA: hda/i915 - fix list corruption with concurrent probes

[Takashi Iwai]

On Tue, 06 Oct 2020 18:17:22 +0200,
Kai Vehmanen wrote:
> 
> From: Takashi Iwai 
> 
> Current hdac_i915 uses a static completion instance to wait
> for i915 driver to complete the component bind.
> 
> This design is not safe if multiple HDA controllers are active and
> communicating with different i915 instances, and can lead to list
> corruption and failed audio driver probe.
> 
> Fix the design by moving completion mechanism to common acomp
> code and remove the related code from hdac_i915.
> 
> Co-developed-by: Kai Vehmanen 
> Signed-off-by: Kai Vehmanen 
> Signed-off-by: Takashi Iwai 

Now I applied it with Fixes tag to 7b882fe3e3e8 ("ALSA: hda - handle
multiple i915 device instances").


thanks,

Takashi


> ---
>  include/drm/drm_audio_component.h |  4 
>  sound/hda/hdac_component.c|  3 +++
>  sound/hda/hdac_i915.c | 23 +++
>  3 files changed, 10 insertions(+), 20 deletions(-)
> 
> diff --git a/include/drm/drm_audio_component.h 
> b/include/drm/drm_audio_component.h
> index a45f93487039..0d36bfd1a4cd 100644
> --- a/include/drm/drm_audio_component.h
> +++ b/include/drm/drm_audio_component.h
> @@ -117,6 +117,10 @@ struct drm_audio_component {
>* @audio_ops: Ops implemented by hda driver, called by DRM driver
>*/
>   const struct drm_audio_component_audio_ops *audio_ops;
> + /**
> +  * @master_bind_complete: completion held during component master 
> binding
> +  */
> + struct completion master_bind_complete;
>  };
>  
>  #endif /* _DRM_AUDIO_COMPONENT_H_ */
> diff --git a/sound/hda/hdac_component.c b/sound/hda/hdac_component.c
> index 89126c6fd216..bb37e7e0bd79 100644
> --- a/sound/hda/hdac_component.c
> +++ b/sound/hda/hdac_component.c
> @@ -210,12 +210,14 @@ static int hdac_component_master_bind(struct device 
> *dev)
>   goto module_put;
>   }
>  
> + complete_all(&acomp->master_bind_complete);
>   return 0;
>  
>   module_put:
>   module_put(acomp->ops->owner);
>  out_unbind:
>   component_unbind_all(dev, acomp);
> + complete_all(&acomp->master_bind_complete);
>  
>   return ret;
>  }
> @@ -296,6 +298,7 @@ int snd_hdac_acomp_init(struct hdac_bus *bus,
>   if (!acomp)
>   return -ENOMEM;
>   acomp->audio_ops = aops;
> + init_completion(&acomp->master_bind_complete);
>   bus->audio_component = acomp;
>   devres_add(dev, acomp);
>  
> diff --git a/sound/hda/hdac_i915.c b/sound/hda/hdac_i915.c
> index 5f0a1aa6ad84..454474ac5716 100644
> --- a/sound/hda/hdac_i915.c
> +++ b/sound/hda/hdac_i915.c
> @@ -11,8 +11,6 @@
>  #include 
>  #include 
>  
> -static struct completion bind_complete;
> -
>  #define IS_HSW_CONTROLLER(pci) (((pci)->device == 0x0a0c) || \
>   ((pci)->device == 0x0c0c) || \
>   ((pci)->device == 0x0d0c) || \
> @@ -130,19 +128,6 @@ static bool i915_gfx_present(void)
>   return pci_dev_present(ids);
>  }
>  
> -static int i915_master_bind(struct device *dev,
> - struct drm_audio_component *acomp)
> -{
> - complete_all(&bind_complete);
> - /* clear audio_ops here as it was needed only for completion call */
> - acomp->audio_ops = NULL;
> - return 0;
> -}
> -
> -static const struct drm_audio_component_audio_ops i915_init_ops = {
> - .master_bind = i915_master_bind
> -};
> -
>  /**
>   * snd_hdac_i915_init - Initialize i915 audio component
>   * @bus: HDA core bus
> @@ -163,9 +148,7 @@ int snd_hdac_i915_init(struct hdac_bus *bus)
>   if (!i915_gfx_present())
>   return -ENODEV;
>  
> - init_completion(&bind_complete);
> -
> - err = snd_hdac_acomp_init(bus, &i915_init_ops,
> + err = snd_hdac_acomp_init(bus, NULL,
> i915_component_master_match,
> sizeof(struct i915_audio_component) - 
> sizeof(*acomp));
>   if (err < 0)
> @@ -177,8 +160,8 @@ int snd_hdac_i915_init(struct hdac_bus *bus)
>   if (!IS_ENABLED(CONFIG_MODULES) ||
>   !request_module("i915")) {
>   /* 60s timeout */
> - wait_for_completion_timeout(&bind_complete,
> -msecs_to_jiffies(60 * 1000));
> + 
> wait_for_completion_timeout(&acomp->master_bind_complete,
> + msecs_to_jiffies(60 * 
> 1000));
>   }
>   }
>   if (!acomp->ops) {
> -- 
> 2.28.0
> 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2] drm/i915: Limit audio CDCLK>=2*BCLK constraint back to GLK only

[Takashi Iwai]

On Fri, 06 Mar 2020 17:45:44 +0100,
Kai Vehmanen wrote:
> 
> Hi folks,
> 
> [+Takashi from ALSA]
> 
> On Mon, 6 Jan 2020, Matt Roper wrote:
> >>> On Tue, Dec 31, 2019 at 04:00:07PM +0200, Kai Vehmanen wrote:
>  Revert changes done in commit f6ec9483091f ("drm/i915: extend audio
>  CDCLK>=2*BCLK constraint to more platforms"). Audio drivers
> >
> > Agreed; GLK's minimum cdclk goes down to 79.2 mhz so it makes sense that
> > it needs to be handled differently than CNL (and beyond).  I.e., this
> > isn't a pure revert of the original patch.
> 
> unfortunately it seems this fix that was done is not holding up in wider 
> testing. It now looks we need to enforce the constraint in one form or 
> another for newer platforms as well. We can't revert the revert as that 
> will bring the boot flicker back, so we'll need something else.
> 
> Now as we've gone back-and-forth multiple times, I want to get some early 
> feedback before opting for one path or another.
> 
> To recap in short:
>  - audio driver calls i915 acomp get_power() multiple times during boot
>   -> this can cause annoying flicker at boot on platforms where
>  each get_power() leads to a modeset change
>   - example: https://gitlab.freedesktop.org/drm/intel/issues/913
>  - systems with more complex audio subsystems (DSP enabled) will be 
>calling get_power() many more times (as i915 iDisp link is needed in 
>multiple phases of audio controller, DSP and codecs initialization), 
>making the flicker worse
> 
> I've gone through (once more) possible options, and it starts to seem that 
> trying to minimize # of get_power() cycles is not going to work well in 
> the long run. We could certainly reduce number of distinct get_power 
> calls e.g. during boot by refactoring the audio DSP setup, but there would 
> still be more than a few, and it's not just the boot. We now need to call 
> get_power() when the audio controller comes out from runtime suspend 
> (otherwise the HDA link is not ok between i915 and audio). This can be pretty 
> annoying if there are visible artifacts to the end-user in such a case
> where potentially no HDMI/DP monitors are even connected).
> 
> Similarly on i915 side, it would seem pretty unlikely that we are going
> to get smooth changes of CDCLK. It might work better on some platforms, 
> but generally (depending on the available dividers provided), it's not 
> going to be guaranteed to be flicker-free.
> 
> So how about: We move the glk_force_audio_cdclk() logic from 
> intel_audio.c:i915_audio_component_get_power() to acomp init.
> This has some notable implications:
> 
>  - audio driver can safely call get_power without worrying 
>about creating display artifacts,
>  - on some platforms, with specific HDA link params in BIOS, 
>this will mean some lower CDCLK frequencies, will not be used
>at all
>   - e.g. on ICL system with 96Mhz BCLK for iDisp, 172800 and
> 18 are blocked out, and 192000 is the practical minimum
> unless you unload the audio driver
>   - if BCLK is 48Mhz, no impact to CDCLK selection on ICL
> 
> Any chance to get this through? I understand this effectively removes the 
> lower clocks from some systems, so this needs to be evaluated carefully.
> 
> I don't really have other options on the table now. We could maybe use 
> idle-timers to delay powering off the audio domain until certain amount of 
> inactivity, but this is both ugly and won't solve the full thing. Or we 
> just keep reducing get_power() calls on audio side, and just mitigate the 
> the severity of the flicker -- again not fully solving the problem.
> 
> Thoughts and feedback is welcome.
> 
> Br, Kai

That sounds reasonable to me.  But it's basically the i915 stuff, so
I'd leave the decision to you guys :)

My another quick thought after reading this mail is whether we can
simply remove glk_force_audio_cdclk(false) in
i915_audio_component_put_power().  In this way, a flicker should be
reduced, at most only once at boot time, and the CDCLK is lowered only
when the audio is really used (once).

Or, similarly, it can be put into *_component_bind() and *_unbind()
instead of *_get_power() and *_put_power().  This indicates that the
corresponding audio device really exists.


thanks,

Takashi
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2] drm/i915: Limit audio CDCLK>=2*BCLK constraint back to GLK only

[Takashi Iwai]

On Tue, 10 Mar 2020 19:25:22 +0100,
Ville Syrjälä wrote:
> 
> On Tue, Mar 10, 2020 at 07:18:58PM +0200, Kai Vehmanen wrote:
> > One problematic scenario that this doesn't cover:
> >  - a single display is used (at low cdclk), and 
> >  - audio block goes to runtime suspend while display stays up. 
> > 
> > Upon resume (for e.g. UI notification sound), audio will initialize the 
> > HDA bus and call get_power() on i915, even if the notification goes to 
> > internal speaker. A modeset at this point is potentially very annoying.
> 
> :( That seems much harder to deal with.

I guess it doesn't happen -- at least with the legacy HD-audio and the
recent chip, if I understand correctly.  When the stream is on the
analog codec, the HDMI codec is kept closed / runtime-resumed.  And
the additional get_power() in the controller side is done only for
HSW/BDW (where the HDA-bus is dedicated to HDMI).


Takashi
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Remove unused IRQ chip data of HDMI LPE audio

[Takashi Iwai]

On Mon, 11 Dec 2017 14:20:23 +0100,
Ville Syrjälä wrote:
> 
> On Mon, Dec 11, 2017 at 08:33:33AM +, Anand, Jerome wrote:
> > 
> > 
> > > -Original Message-
> > > From: Thomas Gleixner [mailto:t...@linutronix.de]
> > > Sent: Saturday, December 9, 2017 4:22 AM
> > > To: Ville Syrjälä 
> > > Cc: Chen, Augustine ; intel-
> > > g...@lists.freedesktop.org; alsa-de...@alsa-project.org; Anand, Jerome
> > > ; Bossart, Pierre-louis  > > louis.boss...@intel.com>; ti...@suse.de; Ingo Molnar ;
> > > H. Peter Anvin ; Jiang Liu ; 
> > > Juergen
> > > Gross ; Dou Liyang ; linux-
> > > ker...@vger.kernel.org
> > > Subject: Re: [Intel-gfx] [PATCH] drm/i915: Remove unused IRQ chip data of
> > > HDMI LPE audio
> > > 
> > > On Fri, 8 Dec 2017, Ville Syrjälä wrote:
> > > 
> > > > On Fri, Dec 08, 2017 at 05:33:23PM +0800, Augustine.Chen wrote:
> > > > > The chip data of HDMI LPE audio is set to drm_i915_private which is
> > > > > not consistent with the expectation by x86 APIC driver.
> > > >
> > > > Hmm. Why is the apic code looking at data for an irq chip it hasn't
> > > > created?
> > > >
> > 
> > apic code expects an irq domain to be place as generic approach.
> > 
> > > > Do we need something like
> > > > - dev_priv->lpe_audio.irq = irq_alloc_desc(0);
> > > > + dev_priv->lpe_audio.irq = irq_alloc_desc(-1);
> > > 
> > > #define irq_alloc_desc(node)
> > > 
> > > So instead of handing in node 0 you hand in node -1 which is NUMA_NO_NODE
> > > 
> 
> Ah. I misread the macros. So we already pass irq=-1.
> 
> > 
> > Agree - am not sure whether it will make any difference.
> > 
> > > > That *looks* more correct to me based on a cursory glance at the x86
> > > > code, but I didn't trawl very deeply.
> > > 
> > > The x86 core cares not at all about interrupt chips which are created in 
> > > a driver
> > > and not connected to an actual apic/ioapic/msi interrupt. This interrupt 
> > > chip is
> > > its own thing as we have others in GPIO etc.
> > > 
> > > > > In the case of not enabling CONFIG_CPUMASK_OFFSTACK, this would
> > > > > cause kernel panic while doing CPU hotplug.
> > > 
> > > And why so? Surely not because you set irq_chip_data. That's really no
> > > explanation at all.
> > > 
> > 
> > Ideally, I feel there needs to be an irq domain for mapping the irq numbers 
> > with hwirq.
> > It is not created as part of the hdmi lpe audio bridge.
> > Since the logic to mask/unmask lpe audio interrupts is removed, there is no 
> > need of the
> > Chip data or creation of the domain now.
> 
> There is no need right now. But there might be a need in the future.
> LPE audio isn't even the only piece of hardware whose irq goes through
> the i915 display engine (there's also the ISP and VED). So I would
> much prefer a proper solution instead of sweeping the problem under
> the rug.

IMO, the primary question is whether the usage of irq chip without irq
domain is valid or not.  If an irq domain is mandatory, that's the
thing to be fixed in i915 side.


thanks,

Takashi
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Remove unused IRQ chip data of HDMI LPE audio

[Takashi Iwai]

On Tue, 12 Dec 2017 10:26:08 +0100,
Chen, Augustine wrote:
> > > > > > That *looks* more correct to me based on a cursory glance at the
> > > > > > x86 code, but I didn't trawl very deeply.
> > > > >
> > > > > The x86 core cares not at all about interrupt chips which are
> > > > > created in a driver and not connected to an actual apic/ioapic/msi
> > > > > interrupt. This interrupt chip is its own thing as we have others in 
> > > > > GPIO etc.
> > > > >
> > > > > > > In the case of not enabling CONFIG_CPUMASK_OFFSTACK, this
> > > > > > > would cause kernel panic while doing CPU hotplug.
> > > > >
> > > > > And why so? Surely not because you set irq_chip_data. That's
> > > > > really no explanation at all.
> > > > >
> > > >
> > > > Ideally, I feel there needs to be an irq domain for mapping the irq 
> > > > numbers
> > with hwirq.
> > > > It is not created as part of the hdmi lpe audio bridge.
> > > > Since the logic to mask/unmask lpe audio interrupts is removed,
> > > > there is no need of the Chip data or creation of the domain now.
> > >
> > > There is no need right now. But there might be a need in the future.
> > > LPE audio isn't even the only piece of hardware whose irq goes through
> > > the i915 display engine (there's also the ISP and VED). So I would
> > > much prefer a proper solution instead of sweeping the problem under
> > > the rug.
> > 
> > IMO, the primary question is whether the usage of irq chip without irq 
> > domain is
> > valid or not.  If an irq domain is mandatory, that's the thing to be fixed 
> > in i915
> > side.
> In terms of functionality, the interrupt and hdmi audio work fine
> without irq domain according to the validation.

Sure, otherwise the patch never landed to mainline :)

> And besides, there
> are other drivers with similar implementation which doesn't set
> chip data at all. 

Yes, dropping the chip data should be OK in the driver, but the only
question is whether it is the right fix.

Did you check whether the issue happens with 4.15-rc, too?  This was
never answered in bugzilla.  If I understand correctly, the code
triggering Oops has been changed largely there and it should prune the
non-legacy irqs.


Takashi
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Remove unused IRQ chip data of HDMI LPE audio

[Takashi Iwai]

On Wed, 13 Dec 2017 12:35:54 +0100,
Thomas Gleixner wrote:
> 
> > > On Mon, 11 Dec 2017, Anand, Jerome wrote:
> > > > > On Fri, 8 Dec 2017, Ville Syrjälä wrote:
> > > > >
> > > > > > On Fri, Dec 08, 2017 at 05:33:23PM +0800, Augustine.Chen wrote:
> > > > > > > The chip data of HDMI LPE audio is set to drm_i915_private which
> > > > > > > is not consistent with the expectation by x86 APIC driver.
> > > > > >
> > > > > > Hmm. Why is the apic code looking at data for an irq chip it
> > > > > > hasn't created?
> > > > > >
> > > >
> > > > apic code expects an irq domain to be place as generic approach.
> > > 
> > > APIC code does not even see that interrupt at all. It's completely 
> > > disconnected.
> > > 
> > 
> > That's the problem - APIC just converts the chip data to its internal
> > format and fails.
> 
> How does APIC code end up to touch that interrupt at all? Call stack please.

It's found in the bugzilla referred in the patch:
  https://bugs.freedesktop.org/show_bug.cgi?id=103731

[   87.353072] irq 298 idata->chip->name hdmi_lpe_audio_irqchip
[   87.353072] irq 298 apic_chip_data
[   87.353073] irq 298 data->domain is NULL
[   87.353120] BUG: unable to handle kernel NULL pointer dereference at (null)
[   87.353132] IP: setup_vector_irq+0x1ba/0x230
[   87.353133] PGD 0

If my understanding is correct, it happens only with 4.14 and earlier
kernels where __setup_vector_irq() loops over the all irqs:

static void __setup_vector_irq(int cpu)
{
struct apic_chip_data *data;
struct irq_desc *desc;
int irq, vector;

/* Mark the inuse vectors */
for_each_irq_desc(irq, desc) {
struct irq_data *idata = irq_desc_get_irq_data(desc);

data = apic_chip_data(idata);
if (!data || !cpumask_test_cpu(cpu, data->domain))
continue;


And since we have assigned a non-APIC chip data in the driver, the
code above refers to a wrong object, leading to Oops.

As a further note, the setup_vector_irq() code has been changed in
4.15, and such a reference won't happen any longer.  So the patch
isn't necessary for now, although it's not bad to take as a cleanup.
And we can eventually put Cc to stable there since it actually works
around the issue above for the older kernels -- of course, with more
detailed descriptions about the background.


thanks,

Takashi
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Request driver probe from an async task

[Takashi Iwai]

On Fri, 27 Apr 2018 10:04:13 +0200,
Jani Nikula wrote:
> 
> On Thu, 26 Apr 2018, Imre Deak  wrote:
> > On Thu, Apr 26, 2018 at 03:41:57PM +0300, David Weinehall wrote:
> >> On Fri, Mar 23, 2018 at 08:30:48AM +, Chris Wilson wrote:
> >> > As we are careful not to register external interfaces before the
> >> > internals are brought up, we are not dependent upon a synchronous
> >> > probing and can allow ourselves to be probed from a secondary thread
> >> > during system bootup. We already do relegate some configuration to
> >> > asynchronous tasks (such as setting up the fbdev), now do the entire
> >> > probe.
> >> > 
> >> > References: https://bugs.freedesktop.org/show_bug.cgi?id=105622
> >> > Signed-off-by: Chris Wilson 
> >> 
> >> LGTM, and still seems to apply cleanly.
> >> 
> >> Reviewed-by: David Weinehall 
> >
> > One problem with this is that atm in snd_hdac_i915_init()
> > request_module() is expected to return only once the i915 probe function
> > has run. With async probing this won't be any more the case.
> >
> > +Takashi
> 
> As I wrote to Yang (Cc'd) in the context of another patch, one approach
> that takes care of this would be adding a completion in hdac_i915.c,
> waiting for it with a timeout below request_module("i915") in
> snd_hdac_i915_init(), and completing it in
> hdac_component_master_bind(). How long the timeout should be is anyone's
> guess...

Yes, that's also a workaround I'd take at next.
Also the code can have a vgacon_text_force() check to skip the
nomodeset case, at least, too.


thanks,

Takashi
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v3] drm/i915/dp: DP audio API changes for MST

[Takashi Iwai]

On Thu, 11 Aug 2016 08:26:31 +0200,
Ville Syrjälä wrote:
> 
> > --- a/sound/hda/hdac_i915.c
> > +++ b/sound/hda/hdac_i915.c
> > @@ -201,7 +201,8 @@ static int pin2port(struct hdac_device *codec, 
> > hda_nid_t pin_nid)
> >   * This function sets N/CTS value based on the given sample rate.
> >   * Returns zero for success, or a negative error code.
> >   */
> > -int snd_hdac_sync_audio_rate(struct hdac_device *codec, hda_nid_t nid, int 
> > rate)
> > +int snd_hdac_sync_audio_rate(struct hdac_device *codec, hda_nid_t nid,
> > +int pipe, int rate)
> 
> I thought you'd still want to call it 'dev_id' on the hda side, and just
> do the dev_id->pipe conversion here next to the pin->port conversion?
> Just figured that would be less confusing for people who are just familiar
> with hda and not i915.

Agreed, it's more consistent to have a pair of the same terms
(pipe, port) vs (dev_id, nid).

Except for that, the changes in the audio side look trivial, and it's
probably better to go through drm tree, so feel free to take my ack:

Reviewed-by: Takashi Iwai 


thanks,

Takashi
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] S4 resume breakage with i915 driver

[Takashi Iwai]

Hi,

while debugging our 4.4.x based SLE12-SP2 kernel, we noticed that S4
resume is broken on many machines with i915 gfx, even on the upstream
4.7 kernel.  Originally it was reported by Intel about SKL machines,
but later on, we found that it hits on many other older chips (at
least HSW), too.

This was hard to identify because there have been other S4 resume bugs
until recently.  But even after these fixes, when the system is tested
on i915 gfx, the system gets memory corruption or kernel Oops sooner
or later after a few (usually < 10) S4 cycles.

As the bug happened between 4.2 and 4.3, I bisected and it pointed to
the commit:

  4c436d55b279bbc6b02aac02e7dc683fc09f884e
drm/i915: Enable Resource Streamer state save/restore on MI_SET_CONTEXT

Indeed, reverting this on top of our 4.4.x kernel seems to make S4
working stably (at least on a test machine).

Does this make any sense to you guys?

Since the commit message doesn't give a good explanation about this
change, I wonder what's the purpose of this commit.  Was it merely
optimization?


Some other things to be noted:
- This might be depending on the kernel config, of course.  We've
  stated hitting this after the deferred page init is enabled, for
  example.  But it's just a coincidence, not the cause.

- S3 seems working stably.  Only S4 is the problem.

- The upstream commits I backported onto 4.4.x are:
  65c0554b73c920023cc8998802e508b798113b46
x86/power/64: Fix kernel text mapping corruption during image restoration
  406f992e4a372dafbe3c2cff7efbb2002a5c8ebd
x86 / hibernate: Use hlt_play_dead() when resuming from hibernation

  With these, S4 works very stable on 4.4.x without i915, passed over
  100 S4 cycles.

- 4.8-rc has a mm-related change (the commit
  e6cbd7f2efb433d717af72aa8510a9db6f7a7e05
mm, page_alloc: remove fair zone allocation policy), and this
  commit alone improves the S4 stability by some mystery reason.  But
  the issue with i915 S4 must remain even with 4.8, I believe.  It's
  just a matter of probability.  Hence, for checking the i915 S4
  issue, it'd be easier to test with a slightly older kernel.


thanks,

Takashi
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] S4 resume breakage with i915 driver

[Takashi Iwai]

On Thu, 25 Aug 2016 17:32:41 +0200,
Chris Wilson wrote:
> 
> On Thu, Aug 25, 2016 at 03:11:35PM +0200, Takashi Iwai wrote:
> > Hi,
> > 
> > while debugging our 4.4.x based SLE12-SP2 kernel, we noticed that S4
> > resume is broken on many machines with i915 gfx, even on the upstream
> > 4.7 kernel.  Originally it was reported by Intel about SKL machines,
> > but later on, we found that it hits on many other older chips (at
> > least HSW), too.
> > 
> > This was hard to identify because there have been other S4 resume bugs
> > until recently.  But even after these fixes, when the system is tested
> > on i915 gfx, the system gets memory corruption or kernel Oops sooner
> > or later after a few (usually < 10) S4 cycles.
> > 
> > As the bug happened between 4.2 and 4.3, I bisected and it pointed to
> > the commit:
> > 
> >   4c436d55b279bbc6b02aac02e7dc683fc09f884e
> > drm/i915: Enable Resource Streamer state save/restore on MI_SET_CONTEXT
> > 
> > Indeed, reverting this on top of our 4.4.x kernel seems to make S4
> > working stably (at least on a test machine).
> > 
> > Does this make any sense to you guys?
> 
> It could explain a HSW issue, but not SKL and not BDW by default (using
> execlists).

Hrm, IIRC, SKL didn't work without the commit.  But it doesn't matter
if this doesn't has a bad influence on SKL, either.

> All machine big core, no Braswell or other !llc device?

Yeah, tested only on SKL, HSW and IVY.  No small boxes.

> > Since the commit message doesn't give a good explanation about this
> > change, I wonder what's the purpose of this commit.  Was it merely
> > optimization?
> 
> It's a step towards enabling the Resource Streamer hardware block in the
> GPU, kind of a glorified prefetch. Userspace also has to notify the GPU
> to enable it for a batch as well.
> 
> The only doubt in mind my about that patch was whether the hw ignored
> the RS_RESTORE_STATE if MI_RESTORE_INHIBIT as expected:
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c 
> b/drivers/gpu/drm/i915/i915_gem_context.c
> index 3c97f0e7a003..c618bb86aeb9 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -574,10 +574,15 @@ mi_set_context(struct drm_i915_gem_request *req, u32 
> hw_flags)
>  
> /* These flags are for resource streamer on HSW+ */
> if (IS_HASWELL(dev_priv) || INTEL_GEN(dev_priv) >= 8)
> -   flags |= (HSW_MI_RS_SAVE_STATE_EN | 
> HSW_MI_RS_RESTORE_STATE_EN);
> +   flags |= HSW_MI_RS_SAVE_STATE_EN;
> else if (INTEL_GEN(dev_priv) < 8)
> -   flags |= (MI_SAVE_EXT_STATE_EN | MI_RESTORE_EXT_STATE_EN);
> -
> +   flags |= MI_SAVE_EXT_STATE_EN;
> +   if ((flags & MI_RESTORE_INHIBIT) == 0) {
> +   if (IS_HASWELL(dev_priv) || INTEL_GEN(dev_priv) >= 8)
> +   flags |= HSW_MI_RS_RESTORE_STATE_EN;
> +   else if (INTEL_GEN(dev_priv) < 8)
> +   flags |= MI_RESTORE_EXT_STATE_EN;
> +   }
>  
> 
> But this code is only executed for Haswell, and we only inhibit restore
> from uninitialised contexts. The biggest fear is that we are restoring
> garbage from userspace and making the GPU do strange things.
> 
> That still doesn't explain stray writes into system memory, that is more
> likely our GTT being broken.

Yeah, that's my expectation.  But the bisection didn't reach it.
So maybe this casually surfaced the hidden GTT or cache issue.

> Could you confirm that bisect has any
> impact on the other machines, and of course double check the result?

You're asking bisection on all machines from the scratch for such a
bug taking so long time to reproduce, and especially for i915 code
path, that is known to be deadly difficult due to various merge
commits?  I sincerely decline the offer :)

Yes, the result was double-checked.  This has a positive effect on all
our tested machines.  Maybe But it's hard to tell exactly whether this is
the 100% culprit.  As said, there have been multiple S4 bugs, so far.
IVY worked without this patch (after x86 fixes), but obviously this
had no negative effect, either.


thanks,

Takashi
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] S4 resume breakage with i915 driver

[Takashi Iwai]

On Thu, 25 Aug 2016 18:12:19 +0200,
Chris Wilson wrote:
> 
> > Maybe But it's hard to tell exactly whether this is
> > the 100% culprit.  As said, there have been multiple S4 bugs, so far.
> > IVY worked without this patch (after x86 fixes), but obviously this
> > had no negative effect, either.
> 
> Try
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 95ddd56b89f0..913ccf14c5a9 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1946,6 +1946,7 @@ static int i915_pm_thaw(struct device *dev)
>  /* restore: called after loading the hibernation image. */
>  static int i915_pm_restore_early(struct device *dev)
>  {
> +   intel_gpu_reset(dev_to_i915(dev), ALL_ENGINES);
> return i915_pm_resume_early(dev);
>  }
> 
> as a shot in the dark.

OK, I'll check it.  Maybe the result will be on tomorrow as I'll need
to leave soon from my office.


Thanks!

Takashi
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] S4 resume breakage with i915 driver

[Takashi Iwai]

On Thu, 25 Aug 2016 18:15:37 +0200,
Takashi Iwai wrote:
> 
> On Thu, 25 Aug 2016 18:12:19 +0200,
> Chris Wilson wrote:
> > 
> > > Maybe But it's hard to tell exactly whether this is
> > > the 100% culprit.  As said, there have been multiple S4 bugs, so far.
> > > IVY worked without this patch (after x86 fixes), but obviously this
> > > had no negative effect, either.
> > 
> > Try
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c 
> > b/drivers/gpu/drm/i915/i915_drv.c
> > index 95ddd56b89f0..913ccf14c5a9 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -1946,6 +1946,7 @@ static int i915_pm_thaw(struct device *dev)
> >  /* restore: called after loading the hibernation image. */
> >  static int i915_pm_restore_early(struct device *dev)
> >  {
> > +   intel_gpu_reset(dev_to_i915(dev), ALL_ENGINES);
> > return i915_pm_resume_early(dev);
> >  }
> > 
> > as a shot in the dark.
> 
> OK, I'll check it.  Maybe the result will be on tomorrow as I'll need
> to leave soon from my office.

I had to modify the intel_gpu_reset() call because the test was done
on the older kernel, so it's like:

+   intel_gpu_reset(dev_to_i915(dev)->dev);

And, it seems working on HSW! \o/

A simple trick, better than the magical register write revert.
I'll check other machines, too, to see whether it has any negative
impact.


Thanks!

Takashi
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] S4 resume breakage with i915 driver

[Takashi Iwai]

On Fri, 26 Aug 2016 11:18:00 +0200,
Takashi Iwai wrote:
> 
> On Thu, 25 Aug 2016 18:15:37 +0200,
> Takashi Iwai wrote:
> > 
> > On Thu, 25 Aug 2016 18:12:19 +0200,
> > Chris Wilson wrote:
> > > 
> > > > Maybe But it's hard to tell exactly whether this is
> > > > the 100% culprit.  As said, there have been multiple S4 bugs, so far.
> > > > IVY worked without this patch (after x86 fixes), but obviously this
> > > > had no negative effect, either.
> > > 
> > > Try
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.c 
> > > b/drivers/gpu/drm/i915/i915_drv.c
> > > index 95ddd56b89f0..913ccf14c5a9 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.c
> > > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > > @@ -1946,6 +1946,7 @@ static int i915_pm_thaw(struct device *dev)
> > >  /* restore: called after loading the hibernation image. */
> > >  static int i915_pm_restore_early(struct device *dev)
> > >  {
> > > +   intel_gpu_reset(dev_to_i915(dev), ALL_ENGINES);
> > > return i915_pm_resume_early(dev);
> > >  }
> > > 
> > > as a shot in the dark.
> > 
> > OK, I'll check it.  Maybe the result will be on tomorrow as I'll need
> > to leave soon from my office.
> 
> I had to modify the intel_gpu_reset() call because the test was done
> on the older kernel, so it's like:
> 
> +   intel_gpu_reset(dev_to_i915(dev)->dev);
> 
> And, it seems working on HSW! \o/
> 
> A simple trick, better than the magical register write revert.
> I'll check other machines, too, to see whether it has any negative
> impact.

The test results look good on all machines.


Takashi
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] S4 resume breakage with i915 driver

[Takashi Iwai]

On Mon, 29 Aug 2016 16:09:23 +0200,
Imre Deak wrote:
> 
> On ma, 2016-08-29 at 15:32 +0200, Daniel Vetter wrote:
> > On Fri, Aug 26, 2016 at 02:42:47PM +0300, Imre Deak wrote:
> > > On pe, 2016-08-26 at 14:10 +0300, Imre Deak wrote:
> > > > On pe, 2016-08-26 at 11:39 +0100, Chris Wilson wrote:
> > > > > On Fri, Aug 26, 2016 at 12:25:01PM +0200, Takashi Iwai wrote:
> > > > > > On Fri, 26 Aug 2016 11:18:00 +0200,
> > > > > > Takashi Iwai wrote:
> > > > > > > I had to modify the intel_gpu_reset() call because the test was
> > > > > > > done
> > > > > > > on the older kernel, so it's like:
> > > > > > > 
> > > > > > > +   intel_gpu_reset(dev_to_i915(dev)->dev);
> > > > > > > 
> > > > > > > And, it seems working on HSW! \o/
> > > > > > > 
> > > > > > > A simple trick, better than the magical register write revert.
> > > > > > > I'll check other machines, too, to see whether it has any
> > > > > > > negative
> > > > > > > impact.
> > > > > > 
> > > > > > The test results look good on all machines.
> > > > > 
> > > > > The theory then is that the GPU's are active across the load of the
> > > > > hibernation image and so before the GTT is updated the memory
> > > > > currently
> > > > > in use by the GPU is reused by the system.
> > > > > 
> > > > > The key question then is the memory of boot kernel still in place
> > > > > during
> > > > > the hibernate restore phase?
> > > > 
> > > > Before restoring the image all devices are quiesced by calling their
> > > > freeze callback, so the GPU should be idle already
> > > > in i915_pm_restore_early() already.
> > > 
> > > But this happens in the loader kernel, so if that doesn't have the
> > > driver built-in then the freeze callback won't be called either. So any
> > > possible BIOS related GPU activity/setup should be quiesced from the
> > > restore callback then.
> > 
> > I thought the loader kernel has an entire initrd attached, to allow stuff
> > like typing in the disk encryption passwd. Which means we very much do
> > load i915 in the loader kernel already.
> 
> AFAICS, the hibernation image is restored from a late_initcall and so
> /bin/init etc. won't be run in the loader kernel and so the driver
> won't be loaded if built as a module.

Well, on many systems, it's explicitly triggered from initrd (at
least, (open)SUSE does it so since ages ago).  dracut does it after
the whole driver initializations on initrd, usually.

> But in theory at least it's
> possible that the driver won't even be configured in the loader kernel.
> 
> > So maybe we need to throw a gpu reset into the right hook (shutdown or
> > whatever it was) to make sure the loader kernel really stops all gpu write
> > cycles, including anything done due to power saving context restoring.
> 
> The callback called right before the hibernation image is restored is
> freeze. Shutdown is called only after creating the image, before
> powering off.

Hmm, this always confuses me.  Is the freeze callback called to the
loader kernel?


thanks,

Takashi


> 
> --Imre
> 
> > We already know that the only way to get the gpu off the context image
> > permanently is a gpu reset, so that would make some sense.
> > -Daniel
> > 
> > > 
> > > > > If we need to add a ->shutdown callback (if
> > > > > that is even called before hibernate restore) then we can only fix
> > > > > future kernels and are still susceptible to corruption when booing
> > > > > from
> > > > > old kernels.
> > > > > 
> > > > > Any one familiar with the details of the hibernation restore? (And
> > > > > how
> > > > > much relates to kexec?)
> > > > > -Chris
> > 
> 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/2] ALSA: x86: Fix runtime PM for hdmi-lpe-audio

[Takashi Iwai]

On Thu, 01 Nov 2018 16:24:36 +0100,
Ville Syrjälä wrote:
> 
> On Wed, Oct 24, 2018 at 06:48:24PM +0300, Ville Syrjala wrote:
> > From: Ville Syrjälä 
> > 
> > Commit 46e831abe864 ("drm/i915/lpe: Mark LPE audio runtime pm as
> > "no callbacks"") broke runtime PM with lpe audio. We can no longer
> > runtime suspend the GPU since the sysfs  power/control for the
> > lpe-audio device no longer exists and the device is considered
> > always active. We can fix this by not marking the device as
> > active.
> > 
> > Cc: Chris Wilson 
> > Cc: Takashi Iwai 
> > Cc: Pierre-Louis Bossart 
> > Fixes: 46e831abe864 ("drm/i915/lpe: Mark LPE audio runtime pm as "no 
> > callbacks"")
> > Signed-off-by: Ville Syrjälä 
> 
> Takashi, do you want to take these or should I just smash them
> into drm-intel?

Feel free to go through drm-intel.
  Acked-by: Takashi Iwai 


thanks,

Takashi

> 
> > ---
> >  sound/x86/intel_hdmi_audio.c | 1 -
> >  1 file changed, 1 deletion(-)
> > 
> > diff --git a/sound/x86/intel_hdmi_audio.c b/sound/x86/intel_hdmi_audio.c
> > index 83d76c345940..bbed4acaafd1 100644
> > --- a/sound/x86/intel_hdmi_audio.c
> > +++ b/sound/x86/intel_hdmi_audio.c
> > @@ -1877,7 +1877,6 @@ static int hdmi_lpe_audio_probe(struct 
> > platform_device *pdev)
> >  
> > pm_runtime_use_autosuspend(&pdev->dev);
> > pm_runtime_mark_last_busy(&pdev->dev);
> > -   pm_runtime_set_active(&pdev->dev);
> >  
> > dev_dbg(&pdev->dev, "%s: handle pending notification\n", __func__);
> > for_each_port(card_ctx, port) {
> > -- 
> > 2.18.1
> 
> -- 
> Ville Syrjälä
> Intel
> 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] ACPI / bus: Introduce a list of ids for "always present" devices

[Takashi Iwai]

On Sun, 09 Apr 2017 22:32:46 +0200,
Hans de Goede wrote:
> 
> Hi,
> 
> On 27-02-17 23:53, Takashi Iwai wrote:
> > On Mon, 27 Feb 2017 22:27:58 +0100,
> > Rafael J. Wysocki wrote:
> >>
> >> On Mon, Feb 27, 2017 at 3:40 PM, Takashi Iwai  wrote:
> 
> 
> 
> >>> Oh, this is interesting, please let me join to the party.
> >>>
> >>> We've hit a similar problem, but for other one: namely, it's INT0002
> >>> that is found on a few CHT devices.  It's never bound properly by a
> >>> similar reason, where _STA is always zero on Linux:
> >>>
> >>> Method (_STA, 0, NotSerialized)  // _STA: Status
> >>> {
> >>> If (SOCS <= 0x04)
> >>> {
> >>> Return (0x0F)
> >>> }
> >>> Else
> >>> {
> >>> Return (Zero)
> >>> }
> >>> }
> >>>
> >>> The device is supposed to be a "virtual GPIO" stuff, and the driver
> >>> hasn't been upstreamed from Intel.  Due to the lack of this driver
> >>> (and it's binding), the machine gets spurious IRQ#9 after the PM
> >>> resume, and it locks up at the second time of PM.
> >>
> >> Well, the solution here seems to be to acquire the missing driver
> >> instead of adding quirks to the ACPI core.
> >
> > The driver is available (not upstreamed, though), but it's not bound
> > due to _STA above.  That's why I'm interested in this thread.
> 
> Takashi thanks for pointing me to the INT0002 device / driver to
> fix the spurious IRQ#9 after the PM resume issue. I was seeing this
> on the GPD-win too (when suspending with power-button + wakeup with
> spacebar). I've added a patch to add the INT0002 device to the
> always present device list + cleaned up the INT0002 driver. I plan
> to submit this upstream soon.
> 
> If you want to test this on your device you need these patches:
> 
> https://github.com/jwrdegoede/linux-sunxi/commit/a1a6e92f2665376ed72f575553238a93e88bb037
> https://github.com/jwrdegoede/linux-sunxi/commit/4946f68f8eaa300f42289bf767722d78cf7fa9e2
> https://github.com/jwrdegoede/linux-sunxi/commit/32640c816dd60d17f003ae8306863da01c215afb
> https://github.com/jwrdegoede/linux-sunxi/commit/abb6a9d69690bb2a1a00b184b06cdae43d6ad001

Thanks Hans, these look promising!

One remaining concern is that INT0002 seems serving for other purpose,
too, in drivers/platform/x86/intel_menlow.c for the thermal
management.  I wonder whether we can enable INT0002 unconditionally.


Takashi
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Fix use after free in lpe_audio_platdev_destroy()

[Takashi Iwai]

15]
> > [31908.556622]  i915_driver_unload+0xe4/0x360 [i915]
> > [31908.556858]  i915_pci_remove+0x23/0x30 [i915]
> > [31908.556948]  pci_device_remove+0x5c/0x100
> > [31908.557037]  device_release_driver_internal+0x1db/0x2e0
> > [31908.557129]  driver_detach+0x68/0xc0
> > [31908.557217]  bus_remove_driver+0x8b/0x150
> > [31908.557304]  driver_unregister+0x3e/0x60
> > [31908.557394]  pci_unregister_driver+0x1d/0x110
> > [31908.557653]  i915_exit+0x1a/0x87 [i915]
> > [31908.557741]  SyS_delete_module+0x264/0x2c0
> > [31908.557834]  entry_SYSCALL_64_fastpath+0x1c/0xb1
> > [31908.557919] Memory state around the buggy address:
> > [31908.558005]  8801f7788200: fb fb fb fb fb fb fb fb fb fb fb fb fb fb 
> > fb fb
> > [31908.558127]  ffff8801f7788280: fb fb fb fb fb fb fb fb fb fb fb fb fb fb 
> > fb fb
> > [31908.558255] >8801f7788300: fb fb fb fb fb fb fb fb fb fb fb fb fb fb 
> > fb fb
> > [31908.558374] ^
> > [31908.558467]  8801f7788380: fb fb fb fb fb fb fb fb fb fb fb fb fb fb 
> > fb fb
> > [31908.558595]  8801f7788400: fb fb fb fb fb fb fb fb fb fb fb fb fb fb 
> > fb fb
> > 
> > Fixes: eef57324d926 ("drm/i915: setup bridge for HDMI LPE audio driver")
> > Signed-off-by: Chris Wilson 
> > Cc: Pierre-Louis Bossart 
> > Cc: Jerome Anand 
> > Cc: Jani Nikula 
> > Cc: Takashi Iwai 
> > Signed-off-by: Chris Wilson 
> 
> Ping?

Oh, this fell into a crack as it was sent just before my vacation.

About the change:

> > ---
> >  drivers/gpu/drm/i915/intel_lpe_audio.c | 8 ++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_lpe_audio.c 
> > b/drivers/gpu/drm/i915/intel_lpe_audio.c
> > index 7a5b41b1c024..32000902a204 100644
> > --- a/drivers/gpu/drm/i915/intel_lpe_audio.c
> > +++ b/drivers/gpu/drm/i915/intel_lpe_audio.c
> > @@ -131,8 +131,12 @@ lpe_audio_platdev_create(struct drm_i915_private 
> > *dev_priv)
> >  
> >  static void lpe_audio_platdev_destroy(struct drm_i915_private *dev_priv)
> >  {
> > -   platform_device_unregister(dev_priv->lpe_audio.platdev);
> > -   kfree(dev_priv->lpe_audio.platdev->dev.dma_mask);
> > +   struct platform_device *platdev = dev_priv->lpe_audio.platdev;
> > +
> > +   kfree(platdev->dev.dma_mask);
> > +   platdev->dev.dma_mask = NULL;
> > +
> > +   platform_device_unregister(platdev);

I'm not sure whether it's good idea to fiddle dma_mask bits before the
unregister call.  Interestingly, this is the only driver that calls
kfree() for pdev's dma_mask.  Either we do something wrong, or
everyone forgot this?


thanks,

Takashi
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Fix use after free in lpe_audio_platdev_destroy()

[Takashi Iwai]

On Tue, 11 Apr 2017 23:27:07 +0200,
Chris Wilson wrote:
> 
> On Tue, Apr 11, 2017 at 10:20:56PM +0100, Chris Wilson wrote:
> > On Tue, Apr 11, 2017 at 11:01:57PM +0200, Takashi Iwai wrote:
> > > On Tue, 11 Apr 2017 22:41:12 +0200,
> > > Chris Wilson wrote:
> > > Oh, this fell into a crack as it was sent just before my vacation.
> > > 
> > > About the change:
> > > 
> > > > > ---
> > > > >  drivers/gpu/drm/i915/intel_lpe_audio.c | 8 ++--
> > > > >  1 file changed, 6 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/intel_lpe_audio.c 
> > > > > b/drivers/gpu/drm/i915/intel_lpe_audio.c
> > > > > index 7a5b41b1c024..32000902a204 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_lpe_audio.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_lpe_audio.c
> > > > > @@ -131,8 +131,12 @@ lpe_audio_platdev_create(struct drm_i915_private 
> > > > > *dev_priv)
> > > > >  
> > > > >  static void lpe_audio_platdev_destroy(struct drm_i915_private 
> > > > > *dev_priv)
> > > > >  {
> > > > > - platform_device_unregister(dev_priv->lpe_audio.platdev);
> > > > > - kfree(dev_priv->lpe_audio.platdev->dev.dma_mask);
> > > > > + struct platform_device *platdev = dev_priv->lpe_audio.platdev;
> > > > > +
> > > > > + kfree(platdev->dev.dma_mask);
> > > > > + platdev->dev.dma_mask = NULL;
> > > > > +
> > > > > + platform_device_unregister(platdev);
> > > 
> > > I'm not sure whether it's good idea to fiddle dma_mask bits before the
> > > unregister call.  Interestingly, this is the only driver that calls
> > > kfree() for pdev's dma_mask.  Either we do something wrong, or
> > > everyone forgot this?
> > 
> > Afaict, everyone else chose the blissful ignorance strategy and we are
> > the only fools to try and free the dma_mask.
> > 
> > Would you feel more comfortable with:
> > 
> > void *mask = platdev->dev.dma_mask;
> > 
> > platform_device_unregister(platdev);
> > 
> > /* XXX see platform_device_register_full():
> >  * "This memory isn't freed when the device is put."
> >  * It's not clear why it hasn't been fixed in a decade...
> >  */
> > kfree(mask);
> 
> Still has the issue that unregister may not the final put, it should but
> still...
> 
> I think we just leak the memory. We are not the owner and so shouldn't
> be fiddling around trying to free it.

Agreed, IMO, we should handle better in the platform device side.


thanks,

Takashi
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v3] drm/i915: Fix use after free in lpe_audio_platdev_destroy()

[Takashi Iwai]

15]
> > [31908.556622]  i915_driver_unload+0xe4/0x360 [i915]
> > [31908.556858]  i915_pci_remove+0x23/0x30 [i915]
> > [31908.556948]  pci_device_remove+0x5c/0x100
> > [31908.557037]  device_release_driver_internal+0x1db/0x2e0
> > [31908.557129]  driver_detach+0x68/0xc0
> > [31908.557217]  bus_remove_driver+0x8b/0x150
> > [31908.557304]  driver_unregister+0x3e/0x60
> > [31908.557394]  pci_unregister_driver+0x1d/0x110
> > [31908.557653]  i915_exit+0x1a/0x87 [i915]
> > [31908.557741]  SyS_delete_module+0x264/0x2c0
> > [31908.557834]  entry_SYSCALL_64_fastpath+0x1c/0xb1
> > [31908.557919] Memory state around the buggy address:
> > [31908.558005]  8801f7788200: fb fb fb fb fb fb fb fb fb fb fb fb fb fb 
> > fb fb
> > [31908.558127]  8801f7788280: fb fb fb fb fb fb fb fb fb fb fb fb fb fb 
> > fb fb
> > [31908.558255] >8801f7788300: fb fb fb fb fb fb fb fb fb fb fb fb fb fb 
> > fb fb
> > [31908.558374]     ^
> > [31908.558467]  8801f7788380: fb fb fb fb fb fb fb fb fb fb fb fb fb fb 
> > fb fb
> > [31908.558595]  8801f7788400: fb fb fb fb fb fb fb fb fb fb fb fb fb fb 
> > fb fb
> > 
> > v2: Just leak the memory (8 bytes) as freeing it ourselves is not safe,
> > and we need to coordinate a proper fix in platform_device itself.
> > v3: Use dma_coerce_mask_and_coherent() to skip the kmalloc
> > 
> > Fixes: eef57324d926 ("drm/i915: setup bridge for HDMI LPE audio driver")
> > Signed-off-by: Chris Wilson 
> > Cc: Pierre-Louis Bossart 
> > Cc: Jerome Anand 
> > Cc: Jani Nikula 
> > Cc: Takashi Iwai 
> > Cc: Ville Syrjälä 
> > Signed-off-by: Chris Wilson 
> > ---
> >  drivers/gpu/drm/i915/intel_lpe_audio.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_lpe_audio.c 
> > b/drivers/gpu/drm/i915/intel_lpe_audio.c
> > index d8ca187ae001..dace2fb3154f 100644
> > --- a/drivers/gpu/drm/i915/intel_lpe_audio.c
> > +++ b/drivers/gpu/drm/i915/intel_lpe_audio.c
> > @@ -108,7 +108,6 @@ lpe_audio_platdev_create(struct drm_i915_private 
> > *dev_priv)
> > pinfo.num_res = 2;
> > pinfo.data = pdata;
> > pinfo.size_data = sizeof(*pdata);
> > -   pinfo.dma_mask = DMA_BIT_MASK(32);
> >  
> > spin_lock_init(&pdata->lpe_audio_slock);
> >  
> > @@ -119,6 +118,8 @@ lpe_audio_platdev_create(struct drm_i915_private 
> > *dev_priv)
> > goto err;
> > }
> >  
> > +   dma_coerce_mask_and_coherent(&platdev->dev, DMA_BIT_MASK(32));
> > +
> 
> Not sure how racy that is since we've already registered the platdev at
> that point. The whole platform_register_full() API looks misdesigned to
> me since you can't do stuff between alloc and register.
> 
> We could shovel the dma_coerce_mask_and_coherent() call into
> platform_register_full() itself I suppose. Or we just stop using the
> register_full() stuff and do each step ourselves, but that looks a bit
> tedious.

Agreed.  Through a quick glance, I couldn't find any caller that uses
dev_mask and coherent_mask individually.  We can clean up the whole
mess like below.

The platform_device_register_full() doesn't necessarily support all
use-cases.  If anyone really needs the separated dev_mask from the
coherent_dma_mask, they should do manually.


thanks,

Takashi

---
diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index c4af00385502..d7e160b212b2 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -504,21 +504,8 @@ struct platform_device *platform_device_register_full(
pdev->dev.parent = pdevinfo->parent;
pdev->dev.fwnode = pdevinfo->fwnode;
 
-   if (pdevinfo->dma_mask) {
-   /*
-* This memory isn't freed when the device is put,
-* I don't have a nice idea for that though.  Conceptually
-* dma_mask in struct device should not be a pointer.
-* See http://thread.gmane.org/gmane.linux.kernel.pci/9081
-*/
-   pdev->dev.dma_mask =
-   kmalloc(sizeof(*pdev->dev.dma_mask), GFP_KERNEL);
-   if (!pdev->dev.dma_mask)
-   goto err;
-
-   *pdev->dev.dma_mask = pdevinfo->dma_mask;
-   pdev->dev.coherent_dma_mask = pdevinfo->dma_mask;
-   }
+   if (pdevinfo->dma_mask)
+   dma_coerce_mask_and_coherent(&pdev->dev, pdevinfo->dma_mask);
 
ret = platform_device_add_resources(pdev,
pdevinfo->res, pdevinfo->num_res);
@@ -541,7 +528,6 @@ struct platform_device *platform_device_register_full(
if (ret) {
 err:
ACPI_COMPANION_SET(&pdev->dev, NULL);
-   kfree(pdev->dev.dma_mask);
 
 err_alloc:
platform_device_put(pdev);
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2] drm/i915: Fix use after free in lpe_audio_platdev_destroy()

[Takashi Iwai]

dule+0x264/0x2c0
> [31908.557834]  entry_SYSCALL_64_fastpath+0x1c/0xb1
> [31908.557919] Memory state around the buggy address:
> [31908.558005]  8801f7788200: fb fb fb fb fb fb fb fb fb fb fb fb fb fb 
> fb fb
> [31908.558127]  8801f7788280: fb fb fb fb fb fb fb fb fb fb fb fb fb fb 
> fb fb
> [31908.558255] >8801f7788300: fb fb fb fb fb fb fb fb fb fb fb fb fb fb 
> fb fb
> [31908.558374] ^
> [31908.558467]  8801f7788380: fb fb fb fb fb fb fb fb fb fb fb fb fb fb 
> fb fb
> [31908.558595]  8801f7788400: fb fb fb fb fb fb fb fb fb fb fb fb fb fb 
> fb fb
> 
> v2: Just leak the memory (8 bytes) as freeing it ourselves is not safe,
> and we need to coordinate a proper fix in platform_device itself.
> 
> Fixes: eef57324d926 ("drm/i915: setup bridge for HDMI LPE audio driver")
> Signed-off-by: Chris Wilson 
> Cc: Pierre-Louis Bossart 
> Cc: Jerome Anand 
> Cc: Jani Nikula 
> Cc: Takashi Iwai 
> Signed-off-by: Chris Wilson 

I'm for v2.
  Reviewed-by: Takashi Iwai 


thanks,

Takashi


> ---
>  drivers/gpu/drm/i915/intel_lpe_audio.c | 9 -
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_lpe_audio.c 
> b/drivers/gpu/drm/i915/intel_lpe_audio.c
> index d8ca187ae001..d19053353a2b 100644
> --- a/drivers/gpu/drm/i915/intel_lpe_audio.c
> +++ b/drivers/gpu/drm/i915/intel_lpe_audio.c
> @@ -131,8 +131,15 @@ lpe_audio_platdev_create(struct drm_i915_private 
> *dev_priv)
>  
>  static void lpe_audio_platdev_destroy(struct drm_i915_private *dev_priv)
>  {
> + /* XXX Note that platform_device_register_full() allocates a dma_mask
> +  * and never frees it. We can't free it here as we cannot guarrantee
> +  * this is the last reference (i.e. that the dma_mask will not be
> +  * used after our unregister). We choose to leak the sizeof(u64)
> +  * allocation - it should be fixed in the platform_device rather
> +  * than us fiddle with its internals.
> +  */
> +
>   platform_device_unregister(dev_priv->lpe_audio.platdev);
> - kfree(dev_priv->lpe_audio.platdev->dev.dma_mask);
>  }
>  
>  static void lpe_audio_irq_unmask(struct irq_data *d)
> -- 
> 2.11.0
> 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [alsa-devel] [PATCH 11/11] ALSA: x86: Register multiple PCM devices for the LPE audio card

[Takashi Iwai]

On Wed, 26 Apr 2017 03:58:57 +0200,
Pierre-Louis Bossart wrote:
> 
> On 04/25/2017 03:27 PM, ville.syrj...@linux.intel.com wrote:
> > From: Ville Syrjälä 
> >
> > Now that everything is in place let's register a PCM device for
> > each pipe of the display engine. This will make it possible to
> > actually output audio to multiple displays at the same time. And
> > it avoids modesets on unrelated displays from clobbering up the
> > ELD and whatnot for the display currently doing the playback.
> >
> > The alternative would be to have a PCM device per port, but per-pipe
> > is easier since the hardware actually works that way.
> Very nice. I just tested on a CHT Zotac box which has two connectors
> (1 HDMI and 1 DP), and I get sound concurrently on both, with hdmi
> being listed as device 2 and DP as device 0.
> I thought there were hardware restrictions but you proved me wrong. Kudos.
> 
> The only point that I find weird is that the jacks are reported as
> 'on' on the 3 pipes, is there a way to tie them to an actual cable
> being used?

The pdata check was changed to check port=-1 as the monitor off in the
patch 6.  Maybe the initialization is missing?


Takashi

> 
> [plb@ZOTAC ~]$ amixer -Dhw:0 controls | grep Jack
> numid=5,iface=CARD,name='HDMI/DP,pcm=0 Jack'
> numid=10,iface=CARD,name='HDMI/DP,pcm=1 Jack'
> numid=15,iface=CARD,name='HDMI/DP,pcm=2 Jack'
> [plb@ZOTAC ~]$ amixer -Dhw:0 cget numid=5
> numid=5,iface=CARD,name='HDMI/DP,pcm=0 Jack'
>   ; type=BOOLEAN,access=r---,values=1
>   : values=on
> [plb@ZOTAC ~]$ amixer -Dhw:0 cset numid=5 off
> amixer: Control hw:0 element write error: Operation not permitted
> 
> [plb@ZOTAC ~]$ amixer -Dhw:0 cget numid=10
> numid=10,iface=CARD,name='HDMI/DP,pcm=1 Jack'
>   ; type=BOOLEAN,access=r---,values=1
>   : values=on
> [plb@ZOTAC ~]$ amixer -Dhw:0 cget numid=15
> numid=15,iface=CARD,name='HDMI/DP,pcm=2 Jack'
>   ; type=BOOLEAN,access=r---,values=1
>   : values=on
> 
> The ELD controls do show a null set of values for device 1, maybe the
> jack value should be set in accordance with the ELD validity?
> Also I am wondering if the display number could be used for the PCM
> device number, or as a hint in the device description to help the user
> know which PCM device to use.
> 
> Anyway thanks for this patchset, nicely done.
> 
> >
> > Cc: Takashi Iwai 
> > Cc: Pierre-Louis Bossart 
> > Signed-off-by: Ville Syrjälä 
> > ---
> >   drivers/gpu/drm/i915/intel_lpe_audio.c | 14 -
> >   include/drm/intel_lpe_audio.h  |  6 ++--
> >   sound/x86/intel_hdmi_audio.c   | 53 
> > +++---
> >   sound/x86/intel_hdmi_audio.h   |  3 +-
> >   4 files changed, 34 insertions(+), 42 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_lpe_audio.c 
> > b/drivers/gpu/drm/i915/intel_lpe_audio.c
> > index a593fdf73171..270aa3e3f0e2 100644
> > --- a/drivers/gpu/drm/i915/intel_lpe_audio.c
> > +++ b/drivers/gpu/drm/i915/intel_lpe_audio.c
> > @@ -111,6 +111,7 @@ lpe_audio_platdev_create(struct drm_i915_private 
> > *dev_priv)
> > pinfo.size_data = sizeof(*pdata);
> > pinfo.dma_mask = DMA_BIT_MASK(32);
> >   + pdata->num_pipes = INTEL_INFO(dev_priv)->num_pipes;
> > spin_lock_init(&pdata->lpe_audio_slock);
> > platdev = platform_device_register_full(&pinfo);
> > @@ -318,7 +319,7 @@ void intel_lpe_audio_notify(struct drm_i915_private 
> > *dev_priv,
> > enum pipe pipe, enum port port,
> > const void *eld, int ls_clock, bool dp_output)
> >   {
> > -   unsigned long irq_flags;
> > +   unsigned long irqflags;
> > struct intel_hdmi_lpe_audio_pdata *pdata;
> > struct intel_hdmi_lpe_audio_pipe_pdata *ppdata;
> > u32 audio_enable;
> > @@ -327,14 +328,12 @@ void intel_lpe_audio_notify(struct drm_i915_private 
> > *dev_priv,
> > return;
> > pdata = dev_get_platdata(&dev_priv->lpe_audio.platdev->dev);
> > -   ppdata = &pdata->pipe;
> > +   ppdata = &pdata->pipe[pipe];
> >   - spin_lock_irqsave(&pdata->lpe_audio_slock, irq_flags);
> > +   spin_lock_irqsave(&pdata->lpe_audio_slock, irqflags);
> > audio_enable = I915_READ(VLV_AUD_PORT_EN_DBG(port));
> >   - pdata->pipe_id = pipe;
> > -
> > if (eld != NULL) {
> > memcpy(ppdata->eld, eld, HDMI_MAX_ELD_BYTES);
> > ppdata->port = port;

Re: [Intel-gfx] [alsa-devel] [PATCH 11/11] ALSA: x86: Register multiple PCM devices for the LPE audio card

[Takashi Iwai]

On Wed, 26 Apr 2017 09:04:46 +0200,
Takashi Iwai wrote:
> 
> On Wed, 26 Apr 2017 03:58:57 +0200,
> Pierre-Louis Bossart wrote:
> > 
> > On 04/25/2017 03:27 PM, ville.syrj...@linux.intel.com wrote:
> > > From: Ville Syrjälä 
> > >
> > > Now that everything is in place let's register a PCM device for
> > > each pipe of the display engine. This will make it possible to
> > > actually output audio to multiple displays at the same time. And
> > > it avoids modesets on unrelated displays from clobbering up the
> > > ELD and whatnot for the display currently doing the playback.
> > >
> > > The alternative would be to have a PCM device per port, but per-pipe
> > > is easier since the hardware actually works that way.
> > Very nice. I just tested on a CHT Zotac box which has two connectors
> > (1 HDMI and 1 DP), and I get sound concurrently on both, with hdmi
> > being listed as device 2 and DP as device 0.
> > I thought there were hardware restrictions but you proved me wrong. Kudos.
> > 
> > The only point that I find weird is that the jacks are reported as
> > 'on' on the 3 pipes, is there a way to tie them to an actual cable
> > being used?
> 
> The pdata check was changed to check port=-1 as the monitor off in the
> patch 6.  Maybe the initialization is missing?

I guess the problem is that the hotplug wq is called at the
initialization to retrieve the pdata for all pipes.  It's called with
uninitialized port=0, so all flags are on at init.

And it implies the potential problem: the pdata contains the
information only for a single pipe.  Maybe it should keep the
status/ELD for all three pipes.


Takashi
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 00/11] drm/i915: LPE audio runtime PM and multipipe

[Takashi Iwai]

On Tue, 25 Apr 2017 22:27:19 +0200,
ville.syrj...@linux.intel.com wrote:
> 
> From: Ville Syrjälä 
> 
> I was wondering why my VLV no longer runtime suspended, and after some
> thinking I decided it had to be the LPE audio preventing it. Turns out
> I was right, so here's my attempt at fixing it.
> 
> And while looking at the code I couldn't help but notice that it
> couldn't actually handle multiple pipes playing back audio at the
> same time. And even having multiple displays active even if only
> one was playing audio was probably a recipe for failure. So I
> tried to fix that by registering a separate PCM device for each
> pipe.
> 
> Note that the patch subjects may not reflect the subsystem
> very well since most of these straddle the border between drm
> and alsa. I think I just slapped on drm/i915 to most where
> there was no clear winner.

A nice patchset, thanks for working on it!

One slight concern (other than the jack issue Pierre reported) is the
incompatible behavior from the current version.  With the pipe-based
multiple streams, user would need to choose another one even if the
device has a single HDMI output, which is pretty common on BYT/CHV
tablets.

Maybe it's no big problem as the users are still limited at the
moment.  Or, we may need to handle a bit differently, e.g. assigning
the PCM stream dynamically per hotplug.

In anyway, with the support of multi streams, alsa-lib config needs to
be updated.


Takashi
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 00/11] drm/i915: LPE audio runtime PM and multipipe

[Takashi Iwai]

On Wed, 26 Apr 2017 15:38:53 +0200,
Ville Syrjälä wrote:
> 
> On Wed, Apr 26, 2017 at 09:29:18AM +0200, Takashi Iwai wrote:
> > On Tue, 25 Apr 2017 22:27:19 +0200,
> > ville.syrj...@linux.intel.com wrote:
> > > 
> > > From: Ville Syrjälä 
> > > 
> > > I was wondering why my VLV no longer runtime suspended, and after some
> > > thinking I decided it had to be the LPE audio preventing it. Turns out
> > > I was right, so here's my attempt at fixing it.
> > > 
> > > And while looking at the code I couldn't help but notice that it
> > > couldn't actually handle multiple pipes playing back audio at the
> > > same time. And even having multiple displays active even if only
> > > one was playing audio was probably a recipe for failure. So I
> > > tried to fix that by registering a separate PCM device for each
> > > pipe.
> > > 
> > > Note that the patch subjects may not reflect the subsystem
> > > very well since most of these straddle the border between drm
> > > and alsa. I think I just slapped on drm/i915 to most where
> > > there was no clear winner.
> > 
> > A nice patchset, thanks for working on it!
> > 
> > One slight concern (other than the jack issue Pierre reported) is the
> > incompatible behavior from the current version.  With the pipe-based
> > multiple streams, user would need to choose another one even if the
> > device has a single HDMI output, which is pretty common on BYT/CHV
> > tablets.
> > 
> > Maybe it's no big problem as the users are still limited at the
> > moment.  Or, we may need to handle a bit differently, e.g. assigning
> > the PCM stream dynamically per hotplug.
> 
> Yeah, I tied the PCM device to the pipe to match the hardware. But we
> could certainly register the PCM device per port, and then do a 
> pipe<->port mapping somewhere to make it all work out. One slight
> complication is that not all ports are necessarily present so we
> might have eg. just port B and port D, but no port C. Not sure if
> it would be a bad thing to register a PCM device even for the
> missing ports anyway?
> 
> I don't recall which way HDA works. Device per port I guess? Well,
> for DP SST/HDMI. But for DP MST I presume it's device per stream
> (ie. pipe) even with HDA.

HD-audio creates the PCM device per HD-audio widget representation,
and they correspond to ports, as it seems.  For MST, the situation is
more complex, and we assign the PCM stream dynamically.  It's for
keeping the behavior (more or less) consistent with non-MST.

> > In anyway, with the support of multi streams, alsa-lib config needs to
> > be updated.
> 
> Hmm. I suppose I'll have to take a gander at what's there.

The HDMI PCM definition itself is easy, just just adding more (hdmi.1,
hdmi.2, etc).  The difficult part would be the front and the default
PCM definition, and I have no good idea about it, so far.


Takashi
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [alsa-devel] [PATCH 11/11] ALSA: x86: Register multiple PCM devices for the LPE audio card

[Takashi Iwai]

On Wed, 26 Apr 2017 15:49:06 +0200,
Ville Syrjälä wrote:
> 
> On Wed, Apr 26, 2017 at 09:19:21AM +0200, Takashi Iwai wrote:
> > On Wed, 26 Apr 2017 09:04:46 +0200,
> > Takashi Iwai wrote:
> > > 
> > > On Wed, 26 Apr 2017 03:58:57 +0200,
> > > Pierre-Louis Bossart wrote:
> > > > 
> > > > On 04/25/2017 03:27 PM, ville.syrj...@linux.intel.com wrote:
> > > > > From: Ville Syrjälä 
> > > > >
> > > > > Now that everything is in place let's register a PCM device for
> > > > > each pipe of the display engine. This will make it possible to
> > > > > actually output audio to multiple displays at the same time. And
> > > > > it avoids modesets on unrelated displays from clobbering up the
> > > > > ELD and whatnot for the display currently doing the playback.
> > > > >
> > > > > The alternative would be to have a PCM device per port, but per-pipe
> > > > > is easier since the hardware actually works that way.
> > > > Very nice. I just tested on a CHT Zotac box which has two connectors
> > > > (1 HDMI and 1 DP), and I get sound concurrently on both, with hdmi
> > > > being listed as device 2 and DP as device 0.
> > > > I thought there were hardware restrictions but you proved me wrong. 
> > > > Kudos.
> > > > 
> > > > The only point that I find weird is that the jacks are reported as
> > > > 'on' on the 3 pipes, is there a way to tie them to an actual cable
> > > > being used?
> > > 
> > > The pdata check was changed to check port=-1 as the monitor off in the
> > > patch 6.  Maybe the initialization is missing?
> > 
> > I guess the problem is that the hotplug wq is called at the
> > initialization to retrieve the pdata for all pipes.  It's called with
> > uninitialized port=0, so all flags are on at init.
> 
> Indeed. That will need to be fixed.
> 
> > 
> > And it implies the potential problem: the pdata contains the
> > information only for a single pipe.  Maybe it should keep the
> > status/ELD for all three pipes.
> 
> I already did that.

Oh then it' fine, I just didn't find it.

> That being said, the entire notify vs. wq is pretty racy. So I was
> thinking that maybe we could just eliminate the wq and do whatever is
> needed directly from the notify hook. And then we could eliminate the
> (ab)use of pdata to transfer the ELD and whatnot between the drivers.
> I guess the main complication is the driver load case. I didn't
> really think that one through so far.

One way would be to implement the get_eld() to call at the probe
time.  HD-audio calls such one (provided via component) at probe and
resume time to sync with the actual h/w state.  For LPE audio, it's
even easier, as we may call i915 exported function directly.


thanks,

Takashi
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 01/11] drm/i915: Fix runtime PM for LPE audio

[Takashi Iwai]

On Thu, 27 Apr 2017 18:02:20 +0200,
ville.syrj...@linux.intel.com wrote:
> 
> From: Ville Syrjälä 
> 
> Not calling pm_runtime_enable() means that runtime PM can't be
> enabled at all via sysfs. So we definitely need to call it
> from somewhere.
> 
> Calling it from the driver seems like a bad idea because it
> would have to be paired with a pm_runtime_disable() at driver
> unload time, otherwise the core gets upset. Also if there's
> no LPE audio driver loaded then we couldn't runtime suspend
> i915 either.
> 
> So it looks like a better plan is to call it from i915 when
> we register the platform device. That seems to match how
> pci generally does things. I cargo culted the
> pm_runtime_forbid() and pm_runtime_set_active() calls from
> pci as well.
> 
> The exposed runtime PM API is massive an thorougly misleading, so
> I don't actually know if this is how you're supposed to use the API
> or not. But it seems to work. I can now runtime suspend i915 again
> with or without the LPE audio driver loaded, and reloading the
> LPE audio driver also seems to work.
> 
> Note that powertop won't auto-tune runtime PM for platform devices,
> which is a little annoying. So I'm not sure that leaving runtime
> PM in "on" mode by default is the best choice here. But I've left
> it like that for now at least.

The reason I didn't proactively turn on the runtime PM was that it
often caused a few seconds of pause to the A/V receivers before
actually starting playing.

There is a planned feature to keep sending the silent stream even
after stopping the stream, but it's not implemented yet.

> Also remove the comment about there not being much benefit from
> LPE audio runtime PM. Not allowing runtime PM blocks i915 runtime
> PM, which will also block s0ix, and that could have a measurable
> impact on power consumption.
> 
> Cc: Takashi Iwai 
> Cc: Pierre-Louis Bossart 
> Fixes: 0b6b524f3915 ("ALSA: x86: Don't enable runtime PM as default")
> Signed-off-by: Ville Syrjälä 

Reviewed-by: Takashi Iwai 

IMO, this should be tagged with Cc to stable.


thanks,

Takashi
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 02/11] ALSA: x86: Clear the pdata.notify_lpe_audio pointer before teardown

[Takashi Iwai]

On Thu, 27 Apr 2017 18:02:21 +0200,
ville.syrj...@linux.intel.com wrote:
> 
> From: Ville Syrjälä 
> 
> Clear the notify function pointer in the platform data before we tear
> down the driver. Otherwise i915 would end up calling a stale function
> pointer and possibly explode.
> 
> Cc: Takashi Iwai 
> Cc: Pierre-Louis Bossart 
> Signed-off-by: Ville Syrjälä 

Reviewed-by: Takashi Iwai 

This deserves also Cc to stable, IMO.


thanks,

Takashi
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2 00/11] drm/i915: LPE audio runtime PM and multipipe (v2)

[Takashi Iwai]

On Thu, 27 Apr 2017 18:02:19 +0200,
ville.syrj...@linux.intel.com wrote:
> 
> From: Ville Syrjälä 
> 
> Okay, here's the second attempt at getting multiple pipes playing back
> audio on the VLV/CHV HDMI LPE audio device. The main change from v1 is
> that now the PCM devices are associated with ports instead of pipes,
> so the audio from one device always gets output on the same display.
> 
> I've also tacked on the alsa-lib conf update. No clue whether it's
> really correct or not (the config language isn't a close friend
> of mine).
> 
> BTW I did notice that with LPE audio all the controls say iface=PCM,
> whereas on HDA a bunch of them say iface=MIXER. No idea if that's
> OK or not, just something I spotted when I was comparing the results
> with HDA.

We generally accept both iface types for IEC958 stuff, since
historically many drivers have already mixed them up.  So it's no
problem :)


> Entire series available here:
> git://github.com/vsyrjala/linux.git lpe_audio_multipipe_2
> 
> Cc: Takashi Iwai 
> Cc: Pierre-Louis Bossart 

All look good, and feel free to take my reviewed-by tag
  Reviewed-by: Takashi Iwai 

As said previously, my only slight concern is the compatibility.
But, in the current situation with PulseAudio, only few people would
use this driver, so it shouldn't be so big impact, I suppose.

BTW, which port is used in general on BYT/CHT?

Oh, also, I suppose you want to carry these over i915 tree?
I don't mind either way, I can take them through sound tree if
preferred, too.


thanks,

Takashi
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [alsa-devel] [PATCH v2 00/11] drm/i915: LPE audio runtime PM and multipipe (v2)

[Takashi Iwai]

On Tue, 02 May 2017 22:15:20 +0200,
Pierre-Louis Bossart wrote:
> 
> On 5/2/17 1:27 PM, Ville Syrjälä wrote:
> > On Mon, May 01, 2017 at 08:29:10PM -0500, Pierre-Louis Bossart wrote:
> >>
> >>
> >> On 04/28/2017 02:37 PM, Ville Syrjälä wrote:
> >>> On Fri, Apr 28, 2017 at 12:10:31PM -0500, Pierre-Louis Bossart wrote:
> >>>>
> >>>> On 04/28/2017 03:41 AM, Takashi Iwai wrote:
> >>>>> On Thu, 27 Apr 2017 18:02:19 +0200,
> >>>>> ville.syrj...@linux.intel.com wrote:
> >>>>>> From: Ville Syrjälä 
> >>>>>>
> >>>>>> Okay, here's the second attempt at getting multiple pipes playing back
> >>>>>> audio on the VLV/CHV HDMI LPE audio device. The main change from v1 is
> >>>>>> that now the PCM devices are associated with ports instead of pipes,
> >>>>>> so the audio from one device always gets output on the same display.
> >>>>>>
> >>>>>> I've also tacked on the alsa-lib conf update. No clue whether it's
> >>>>>> really correct or not (the config language isn't a close friend
> >>>>>> of mine).
> >>>>>>
> >>>>>> BTW I did notice that with LPE audio all the controls say iface=PCM,
> >>>>>> whereas on HDA a bunch of them say iface=MIXER. No idea if that's
> >>>>>> OK or not, just something I spotted when I was comparing the results
> >>>>>> with HDA.
> >>>>> We generally accept both iface types for IEC958 stuff, since
> >>>>> historically many drivers have already mixed them up.  So it's no
> >>>>> problem :)
> >>>>>
> >>>>>
> >>>>>> Entire series available here:
> >>>>>> git://github.com/vsyrjala/linux.git lpe_audio_multipipe_2
> >>>>>>
> >>>>>> Cc: Takashi Iwai 
> >>>>>> Cc: Pierre-Louis Bossart 
> >>>>> All look good, and feel free to take my reviewed-by tag
> >>>>> Reviewed-by: Takashi Iwai 
> >>>>>
> >>>>> As said previously, my only slight concern is the compatibility.
> >>>>> But, in the current situation with PulseAudio, only few people would
> >>>>> use this driver, so it shouldn't be so big impact, I suppose.
> >>>>>
> >>>>> BTW, which port is used in general on BYT/CHT?
> >>>>>
> >>>>> Oh, also, I suppose you want to carry these over i915 tree?
> >>>>> I don't mind either way, I can take them through sound tree if
> >>>>> preferred, too.
> >>>> I see frequent oops on startup with this lpe_audio_multipipe_2 branch
> >>>> with my CHT device not booting or no HDMI audio device created.
> >>>> Not sure if these issues are due to the new patches or to the rest of
> >>>> the drm code?
> >>>>
> >>>> [5.529023] BUG: unable to handle kernel NULL pointer dereference
> >>>> at   (null)
> >>>> [5.529143] IP: hdmi_lpe_audio_probe+0x40f/0x650 [snd_hdmi_lpe_audio]
> >>>> [5.529202] PGD 0
> >>>>
> >>>> [5.529242] Oops:  [#1] SMP
> >>>> [5.529274] Modules linked in: snd_soc_sst_atom_hifi2_platform
> >>>> snd_soc_sst_match snd_soc_core snd_compress lpc_ich snd_seq
> >>>> snd_seq_device shpchp snd_hdmi_lpe_audio(+) snd_pcm snd_timer dw_dmac
> >>>> snd soundcore i2c_designware_platform(+) i2c_designware_core
> >>>> spi_pxa2xx_platform acpi_pad mac_hid nfsd auth_rpcgss nfs_acl lockd
> >>>> grace sunrpc ip_tables x_tables hid_generic mmc_block i2c_hid usbhid hid
> >>>> autofs4
> >>>> [5.529605] CPU: 2 PID: 512 Comm: systemd-udevd Not tainted
> >>>> 4.11.0-rc8-test+ #11
> >>>> [5.529671] Hardware name: ZOTAC XX/Cherry Trail FFD, BIOS 5.11
> >>>> 09/28/2016
> >>>> [5.529736] task: 88007485b780 task.stack: c9bfc000
> >>>> [5.529793] RIP: 0010:hdmi_lpe_audio_probe+0x40f/0x650
> >>>> [snd_hdmi_lpe_audio]
> >>>> [5.529855] RSP: 0018:c9bffaf0 EFLAGS: 00010246
> >>>> [5.529904] RAX:  RBX: 880079209898 RCX:
> >>>> 88007920f078
> >>>> [