Skip to content

[SQUASH ME] ASoC: SOF: Intel: hda: release display power for D0ix - #1767

Closed
kv2019i wants to merge 3 commits into
thesofproject:topic/sof-devfrom
kv2019i:fix/tgl-hdmi-fix-part2
Closed

[SQUASH ME] ASoC: SOF: Intel: hda: release display power for D0ix#1767
kv2019i wants to merge 3 commits into
thesofproject:topic/sof-devfrom
kv2019i:fix/tgl-hdmi-fix-part2

Conversation

@kv2019i

@kv2019i kv2019i commented Feb 6, 2020

Copy link
Copy Markdown
Collaborator

Display driver will emit a warning if system enters S3 with
wakerefs to some power domain active. Make sure SOF releases
display power when it enters D0ix, and wakes it back up upon
resume.

Now that we have more invocations of the display power helper
functions, put the the ifdefs and the check for codec_mask,
into hda_codec_i915_display_power() directly to reduce
clutter.

Should be squashed with
"ASoC: SOF: Intel: hda: fix ordering bug in resume flow"

Signed-off-by: Kai Vehmanen kai.vehmanen@linux.intel.com

Display driver will emit a warning if system enters S3 with
wakerefs to some power domain active. Make sure SOF releases
display power when it enters D0ix, and wakes it back up upon
resume.

Now that we have more invocations of the display power helper
functions, put the the ifdefs and the check for codec_mask,
into hda_codec_i915_display_power() directly to reduce
clutter.

Should be squashed with
"ASoC: SOF: Intel: hda: fix ordering bug in resume flow"

Signed-off-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
@kv2019i

kv2019i commented Feb 6, 2020

Copy link
Copy Markdown
Collaborator Author

This is in immediate follow-up to #1764 which has a regression to D0ix flow. @plbossart -- if this looks ok, I can squash with #1764 and send to alsa-devel.

snd_hdac_ext_bus_link_get() does not work correctly in case
there are multiple codecs on the bus. It unconditionally
resets the bus->codec_mask value. This overwrites the initial
value discovered at probe time when all codecs on the link
were powered up.

Signed-off-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
ranj063
ranj063 previously approved these changes Feb 6, 2020

@ranj063 ranj063 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks @kv2019i

@ranj063

ranj063 commented Feb 6, 2020

Copy link
Copy Markdown
Collaborator

@kv2019i I just realized that these won't apply on Marks tree without the D0i3 patches. Maybe we should combine them?

@kv2019i

kv2019i commented Feb 6, 2020

Copy link
Copy Markdown
Collaborator Author

@kv2019i I just realized that these won't apply on Marks tree without the D0i3 patches. Maybe we should combine them?

@ranj063 True. I can either rebase on top of Mark's tree and send (this needs to be squashed to the first patch of #1764)), or we send these via @plbossart 's next pull and avoid the conflict with your D0i3 changes. I'm ok with both.

@plbossart plbossart left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kv2019i some changes look at bit suspicious?

Comment thread sound/soc/sof/intel/hda-codec.c
Comment thread sound/soc/sof/intel/hda-dsp.c
dev_dbg(bus->dev, "codec_mask = 0x%lx\n", codec_mask);
snd_hdac_chip_writew(bus, STATESTS, codec_mask);
if (!bus->codec_mask)
bus->codec_mask = codec_mask;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you clarify in the commit message what the impact of overriding the bus->codec_mask is? the patch looks ok, but it's not clear why it's needed.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@plbossart Ack. So basicly my patch to power up/down display, at runtime suspend/resume, depends on codec_mask being correct. If hdac library changes it on its own, our PM runtime flow stops working correctly. We could copy the data to a SOF local variable outside HDAC, but it seems that's not really reasonable -- there doesn't seem to be any reason for hdac_ext_controller.c to modify codec_mask like this.

When sending upstream, this should patch should be first in the series (as later patches depend on it). I can still do this :)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, I'll approve then and let you send all the i915-related upstream on top of Mark's tree. Please add my reviewed-by tag.

when it's merged, we'll deal with conflicts with Ranjani's changes and send a v2.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if there's really no reason for hdac_ext_controller to modify .codec_mask maybe that should be fixed instead?

@plbossart

Copy link
Copy Markdown
Member

@kv2019i I just realized that these won't apply on Marks tree without the D0i3 patches. Maybe we should combine them?

@ranj063 True. I can either rebase on top of Mark's tree and send (this needs to be squashed to the first patch of #1764)), or we send these via @plbossart 's next pull and avoid the conflict with your D0i3 changes. I'm ok with both.

It's probably better to have first those i915 fixes first, then re-send the v2 version of Ranjani's patches. It makes our life more complicated but others doing backports will thank us. And this is easier to apply to -stable as well.

plbossart
plbossart previously approved these changes Feb 6, 2020

@plbossart plbossart left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @kv2019i

@plbossart plbossart added the upstream Patch has been sent to upstream (e.g. alsa-devel) label Feb 6, 2020
@kv2019i

kv2019i commented Feb 6, 2020

Copy link
Copy Markdown
Collaborator Author

Thanks @kv2019i

Ack @plbossart and @ranj063 . I'll send to the list with your signed-offs. Takashi is also making lot of HDMI changes, so he might have some further comments, let's see.

@kv2019i

kv2019i commented Feb 6, 2020

Copy link
Copy Markdown
Collaborator Author

Sent upstream (squashed and a few more very minor modifications): https://mailman.alsa-project.org/pipermail/alsa-devel/2020-February/162509.html

Incremental changes to following patches sent usptream:

ALSA: hda: do not override bus codec_mask in link_get()
ASoC: SOF: Intel: hda: fix ordering bug in resume flow
ASoC: SOF: Intel: hda: move i915 init earlier

Signed-off-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
@kv2019i
kv2019i dismissed stale reviews from plbossart and ranj063 via d31cd92 February 7, 2020 15:04
@kv2019i

kv2019i commented Feb 7, 2020

Copy link
Copy Markdown
Collaborator Author

Added one incremental patch to align with the version sent upstream.

@plbossart

Copy link
Copy Markdown
Member

@kv2019i this can be closed, yes?

@kv2019i

kv2019i commented Feb 14, 2020

Copy link
Copy Markdown
Collaborator Author

Merged via upstream rebase, closing.

@kv2019i kv2019i closed this Feb 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

upstream Patch has been sent to upstream (e.g. alsa-devel)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants