Repository navigation
Update changes of snd_hdac_display_power() from upstream - #455
Conversation
|
@bardliao I presume u have tested this to confirm that suspend/resume works with HDMI playback on glk? |
|
@ranj063 I tested it on up2. HDMI playback works fine after suspend/resume. But we will see underrun and ipc timed out after resume if we suspend during HDMI playback. This issue also happens without the PR. |
How about glk? |
|
@ranj063 I don't have glk at hand. I think we can test it once Chrome team decide to cherry-pick Takashi's patch to their code base. |
plbossart
left a comment
plbossart
left a comment
There was a problem hiding this comment.
We also want to change the exit, the comment below does not apply any longer
/*
* we don't need to decrease the refcount with
* hda_codec_i915_put() on exit since the device cannot be
* active
*/
Make unified suspend / resume helpers and call them from both the runtime- and the system-PM callbacks for simplifying code. There are slight changes of call orders, but there shouldn't be any functional difference after refactoring. Signed-off-by: Takashi Iwai <tiwai@suse.de> (cherry picked from commit 3baffc4)
The current HD-audio code manages the DRM audio power via too complex redirections, and this seems even still unbalanced in a corner case as Intel DRM CI has been intermittently reporting. This patch is a big surgery for addressing the complexity and the possible unbalance. Basically the patch changes the display PM in the following ways: - Both HD-audio controller and codec drivers call a single helper, snd_hdac_display_power(). (Formerly, the display power control from a codec was done indirectly via link_power bus ops.) - snd_hdac_display_power() receives the codec address index. For turning on/off from the controller, pass HDA_CODEC_IDX_CONTROLLER. - snd_hdac_display_power() doesn't manage refcounts any longer, but keeps the power status in bitmap. If any of controller or codecs is turned on, the function updates the DRM power state via get_power() or put_power(). Also this refactor allows us more cleanup: - The link_power bus ops is dropped, so there is no longer indirect management, as mentioned in the above. - hdac_device link_power_control flag is moved to hda_codec display_power_control flag, as it's only for HDA legacy. Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=106525 Signed-off-by: Takashi Iwai <tiwai@suse.de> (cherry picked from commit 029d92c)
snd_hdac_display_power() can be called even for a HDA controller without DRM binding. The same is true for other helpers, snd_hdac_i915_set_bclk() and snd_hdac_set_codec_wakeup(). So all superfluous AZX_DCAPS_I915_POWERWELL checks in hda_intel.c can be dropped, and the definition of AZX_DCAPS_I915_POWERWELL itself can be removed as well. This simplifies the code a lot. Signed-off-by: Takashi Iwai <tiwai@suse.de> (cherry picked from commit e454ff8)
When an error occurs in azx_probe_continue(), we should release the display power. However, the current code ignores it and releases the display power only for HSW/BDW cases. Fix it. Signed-off-by: Takashi Iwai <tiwai@suse.de> (cherry picked from commit 457f3c8)
After the recent refactoring, snd_hdac_display_power() doesn't return any error, hence it can be defined to return void. This makes many error checks redundant and allows us to reduce them gracefully. Signed-off-by: Takashi Iwai <tiwai@suse.de> (cherry picked from commit 4f799e7)
The display power is in unbalance at removing the driver since it misses the snd_hdac_display_power(OFF) call. Acked-by: Mark Brown <broonie@kernel.org> Signed-off-by: Takashi Iwai <tiwai@suse.de> (cherry picked from commit 77a4967)
We've excluded the display_power_control flag for Intel HSW and BDW codecs as the HD-audio controllers of the corresponding platforms take care of the display power as well. But the recent refactoring separates the controller and the codec power accounting, so it's fine to call the display PM even for HSW/BDW codecs. This is less confusing since we can avoid this well-hidden condition. Signed-off-by: Takashi Iwai <tiwai@suse.de> (cherry picked from commit 46594d3)
The prototype of snd_hdac_display_power() has changed. pass HDA_CODEC_IDX_CONTROLLER since i915 is controller. Signed-off-by: Bard liao <yung-chuan.liao@linux.intel.com>
snd_hdac_display_power() is already a void function therefore we don't need to return any value from hda_codec_i915_get() nor hda_codec_i915_put(). Signed-off-by: Bard liao <yung-chuan.liao@linux.intel.com>
|
@plbossart Thanks. The comment is removed. |
Ah sorry, I didn't mean to remove the comment only, but also add a display_power(false) as done by the skylake driver. |
|
@plbossart Sorry I misunderstood your comment. Probably it is not necessary to add a display_power(false) in |
|
@bardliao this routine wasn't part of the original skylake but maybe it's missing. Do you want to ask Takashi if this is intentional or a miss due to search/replace? |
|
@plbossart According to Takashi's reply, I think it is missed. |
This PR include Takashi's snd_hdac_display_power() related patches and the necessary changes on SOF side.
Fix #400