Hi,
hmm, I think a few review comments from Takashi were missed. See inline:
On Mon, 29 Jun 2020, Harsha Priya wrote:
> --- a/sound/pci/hda/Kconfig
> +++ b/sound/pci/hda/Kconfig
> @@ -232,4 +232,20 @@ config SND_HDA_POWER_SAVE_DEFAULT
>
> endif
>
> +config SND_HDA_INTEL_HDMI_SILENT_STREAM
> + bool "Enable Silent Stream always for HDMI"
> + depends on SND_HDA_INTEL
Takashi requested to limit this to Intel hardware only, and despite the
its name, SND_HDA_INTEL is not sufficient to do this.
I think best would be to take silent stream into use in
intel_hsw_common_init(). That indirection would also protect against
user-space modifying the module parameter value during execution (it is
only evaluated in intel_hsw_common_init()).
> + if (enable_silent_stream && eld->eld_valid) {
> + int pm_ret;
> +
> + if (!monitor_prev && monitor_next && eld->eld_valid) {
> + pm_ret = snd_hda_power_up_pm(codec);
> + if (pm_ret < 0)
> + codec_err(codec,
> + "Monitor plugged-in, Failed to power up codec ret=[%d]\n",
> + pm_ret);
> + silent_stream_enable(codec, per_pin);
> + } else if (monitor_prev && !monitor_next && !eld->eld_valid) {
> + pm_ret = snd_hda_power_down_pm(codec);
> + if (pm_ret < 0)
> + codec_err(codec,
> + "Monitor plugged-out, Failed to power down codec ret=[%d]\n",
> + pm_ret);
> + }
> + }
There a bug in above, outer "if" checks for "eld_valid" while inner
"else-if" checks for "!eld_valid" -> latter can never be reached.
Takashi, I was checking older code history on the acomp interface, and at
least on Intel platforms using acomp, it is sufficient to check for
eld->monitor_present. I.e. in
» eld->eld_size = snd_hdac_acomp_get_eld(&codec->core, per_pin->pin_nid,
» » » » per_pin->dev_id, &eld->monitor_present,
» » » » eld->eld_buffer, ELD_MAX_SIZE);
» eld->eld_valid = (eld->eld_size > 0);
... monitor_present will always be 0 if eld_size<0 (and eld_valid is
set to 0). An error monitor_present will not be set, but
sync_eld_via_acomp() initialized the value to 0 for this case.
The additional checks in the patch are thus harmless, but make the code a
bit suspicious looking. If the above did not hold, you could have a
sequence like
monitor_prev=0, monitor_next=1, eld_valid=1 -> power-up
monitor_prev=1, monitor_next=0, eld_valid=0 -> no op
monitor_prev=0, monitor_next=1, eld_valid=1 -> power-up again
.. so the refcounting would go out of sync. So I'd rather just track the
two variables.
Br, Kai
