ACPI REV override for Dell XPS 13 9343

ACPI REV override for Dell XPS 13 9343

[Cezary Rojewski]

Hello,

It's been a while since catpt-driver [1] has been introduced to provide 
full support for Broadwell (BDW) machines with Intel DSP. For BDW, audio 
devices can make use of DSP only in I2S mode. In 2015 Rafael and Dominik 
provided quirk [2] for Dell XPS 13 9343. Given the description:

_For example, based on what ACPI exports as the supported revision, Dell 
XPS 13 (2015) configures its audio device to either work in HDA mode or 
in I2S mode, where the former is supposed to be used on Linux until the 
latter is fully supported (in the kernel as well as in user space)._

It's clear that such configuration was not fully supported back then. I 
believe now it is. Perhaps it is time to let the quirk in mention go? By 
that I mean just the relevant entry, not the ACPI_REV_OVERRIDE_POSSIBLE 
functionality as a whole.


[1]: 
https://lore.kernel.org/all/20200929141247.8058-1-cezary.rojewski@intel.com/
[2]: https://lore.kernel.org/alsa-devel/5115175.tbCl1JQVaV@vostro.rjw.lan/


Regards,
Czarek

Re: ACPI REV override for Dell XPS 13 9343

[Pierre-Louis Bossart]

On 6/13/22 05:53, Cezary Rojewski wrote:
> Hello,
> 
> It's been a while since catpt-driver [1] has been introduced to provide
> full support for Broadwell (BDW) machines with Intel DSP. For BDW, audio
> devices can make use of DSP only in I2S mode. In 2015 Rafael and Dominik
> provided quirk [2] for Dell XPS 13 9343. Given the description:
> 
> _For example, based on what ACPI exports as the supported revision, Dell
> XPS 13 (2015) configures its audio device to either work in HDA mode or
> in I2S mode, where the former is supposed to be used on Linux until the
> latter is fully supported (in the kernel as well as in user space)._
> 
> It's clear that such configuration was not fully supported back then. I
> believe now it is. Perhaps it is time to let the quirk in mention go? By
> that I mean just the relevant entry, not the ACPI_REV_OVERRIDE_POSSIBLE
> functionality as a whole.

This should be a distribution or power-user decision to enable the I2S
version IMHO.

There is nothing new in terms of functionality with the I2S version, so
limited added-value that doesn't offset the added risk due to the
dependencies on mixer settings that may or may not be installed (UCM, etc).

If it ain't broke don't fix it.

Re: ACPI REV override for Dell XPS 13 9343

[Cezary Rojewski]

On 2022-06-13 3:05 PM, Pierre-Louis Bossart wrote:
> On 6/13/22 05:53, Cezary Rojewski wrote:
>> Hello,
>>
>> It's been a while since catpt-driver [1] has been introduced to provide
>> full support for Broadwell (BDW) machines with Intel DSP. For BDW, audio
>> devices can make use of DSP only in I2S mode. In 2015 Rafael and Dominik
>> provided quirk [2] for Dell XPS 13 9343. Given the description:
>>
>> _For example, based on what ACPI exports as the supported revision, Dell
>> XPS 13 (2015) configures its audio device to either work in HDA mode or
>> in I2S mode, where the former is supposed to be used on Linux until the
>> latter is fully supported (in the kernel as well as in user space)._
>>
>> It's clear that such configuration was not fully supported back then. I
>> believe now it is. Perhaps it is time to let the quirk in mention go? By
>> that I mean just the relevant entry, not the ACPI_REV_OVERRIDE_POSSIBLE
>> functionality as a whole.
> 
> This should be a distribution or power-user decision to enable the I2S
> version IMHO.
> 
> There is nothing new in terms of functionality with the I2S version, so
> limited added-value that doesn't offset the added risk due to the
> dependencies on mixer settings that may or may not be installed (UCM, etc).
> 
> If it ain't broke don't fix it.


Not much of a fan of the last statement. I believe challenging status 
quo is the right thing to do. We do not want to bloat the kernel with 
unnecessary quirks.

The broadwell-rt286 UCM is part of alsa-ucm-conf repo for years now.


Regards,
Czarek

Re: ACPI REV override for Dell XPS 13 9343

[Pierre-Louis Bossart]

>>> It's been a while since catpt-driver [1] has been introduced to provide
>>> full support for Broadwell (BDW) machines with Intel DSP. For BDW, audio
>>> devices can make use of DSP only in I2S mode. In 2015 Rafael and Dominik
>>> provided quirk [2] for Dell XPS 13 9343. Given the description:
>>>
>>> _For example, based on what ACPI exports as the supported revision, Dell
>>> XPS 13 (2015) configures its audio device to either work in HDA mode or
>>> in I2S mode, where the former is supposed to be used on Linux until the
>>> latter is fully supported (in the kernel as well as in user space)._
>>>
>>> It's clear that such configuration was not fully supported back then. I
>>> believe now it is. Perhaps it is time to let the quirk in mention go? By
>>> that I mean just the relevant entry, not the ACPI_REV_OVERRIDE_POSSIBLE
>>> functionality as a whole.
>>
>> This should be a distribution or power-user decision to enable the I2S
>> version IMHO.
>>
>> There is nothing new in terms of functionality with the I2S version, so
>> limited added-value that doesn't offset the added risk due to the
>> dependencies on mixer settings that may or may not be installed (UCM,
>> etc).
>>
>> If it ain't broke don't fix it.

to clarify: the HDaudio mode works just fine, there are currently zero
issues or missing functionality reported by users.

> Not much of a fan of the last statement. I believe challenging status
> quo is the right thing to do. We do not want to bloat the kernel with
> unnecessary quirks.
> 
> The broadwell-rt286 UCM is part of alsa-ucm-conf repo for years now.

The hardware supports two modes (HDaudio and DSP + I2S), the former has
been in use without any issues for years.

Even if you make DSP + I2S the default, you've got to leave HDaudio as a
fallback, so there would be no change at the kernel level.

It' very very hard to remove stuff, and in this case there is limited
evidence that distributions use the DSP + I2S mode. You could deprecate
the ACPI_OVERRIDE but you've got to leave time for distributions to switch.

Challenging the status quo is great, but let's keep downstream in mind,
shall we?

The only option we removed was Medfield several years ago, but we had
clear evidence that no one would be affected by such a change in the
upstream kernel.

Re: ACPI REV override for Dell XPS 13 9343

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 457 bytes --]

On Tue, Jun 14, 2022 at 11:51:01AM -0500, Pierre-Louis Bossart wrote:

> It' very very hard to remove stuff, and in this case there is limited
> evidence that distributions use the DSP + I2S mode. You could deprecate
> the ACPI_OVERRIDE but you've got to leave time for distributions to switch.

At the time that quirk was done I don't think there was even DSP support
- it was just plain I2S AFAIR, no firmware loaded or at least only in
passthrough mode.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: ACPI REV override for Dell XPS 13 9343

[Cezary Rojewski]

On 2022-06-14 6:51 PM, Pierre-Louis Bossart wrote:

>>> If it ain't broke don't fix it.
> 
> to clarify: the HDaudio mode works just fine, there are currently zero
> issues or missing functionality reported by users.
> 
>> Not much of a fan of the last statement. I believe challenging status
>> quo is the right thing to do. We do not want to bloat the kernel with
>> unnecessary quirks.
>>
>> The broadwell-rt286 UCM is part of alsa-ucm-conf repo for years now.
> 
> The hardware supports two modes (HDaudio and DSP + I2S), the former has
> been in use without any issues for years.
> 
> Even if you make DSP + I2S the default, you've got to leave HDaudio as a
> fallback, so there would be no change at the kernel level.
> 
> It' very very hard to remove stuff, and in this case there is limited
> evidence that distributions use the DSP + I2S mode. You could deprecate
> the ACPI_OVERRIDE but you've got to leave time for distributions to switch.
> 
> Challenging the status quo is great, but let's keep downstream in mind,
> shall we?
> 
> The only option we removed was Medfield several years ago, but we had
> clear evidence that no one would be affected by such a change in the
> upstream kernel.


If the entry has to stay for the fallback reasons, then it can't be just 
removed. And if so, regardless of the default it's going to be there - 
no point in providing a patch.

Thanks for the feedback!


Regards,
Czarek