On Thu, Dec 15, 2016 at 03:29:43PM +0100, Maarten Lankhorst wrote:
> If the crtc was brought up with audio before the driver loads,
> then crtc_disable will remove a refcount to audio that doesn't exist
> before.
>
> Fortunately we already set power domains on readout, so we can just add
> the power domain handling to get_crtc_power_domains, which will update
> the power domains correctly in all cases.
>
> This was found when testing module reload on CI with the crtc enabled,
> which resulted in the following warn after module reload + modeset:
>
> [ 24.197041] ------------[ cut here ]------------
> [ 24.197075] WARNING: CPU: 0 PID: 99 at drivers/gpu/drm/i915/intel_runtime_pm.c:1790 intel_display_power_put+0x134/0x140 [i915]
> [ 24.197076] Use count on domain AUDIO is already zero
> [ 24.197098] CPU: 0 PID: 99 Comm: kworker/u8:2 Not tainted 4.9.0-CI-Trybot_393+ #1
> [ 24.197099] Hardware name: /NUC6i5SYB, BIOS SYSKLi35.86A.0042.2016.0409.1246 04/09/2016
> [ 24.197102] Workqueue: events_unbound async_run_entry_fn
> [ 24.197105] ffffc900003c7688 ffffffff81435b35 ffffc900003c76d8 0000000000000000
> [ 24.197107] ffffc900003c76c8 ffffffff8107e4d6 000006fe5dc36f28 ffff88025dc30054
> [ 24.197109] ffff88025dc36f28 ffff88025dc30000 ffff88025dc30000 0000000000000015
> [ 24.197110] Call Trace:
> [ 24.197113] [<ffffffff81435b35>] dump_stack+0x67/0x92
> [ 24.197116] [<ffffffff8107e4d6>] __warn+0xc6/0xe0
> [ 24.197118] [<ffffffff8107e53a>] warn_slowpath_fmt+0x4a/0x50
> [ 24.197149] [<ffffffffa039b4b4>] intel_display_power_put+0x134/0x140 [i915]
> [ 24.197187] [<ffffffffa04217dd>] intel_disable_ddi+0x4d/0x80 [i915]
> [ 24.197223] [<ffffffffa03f388f>] intel_encoders_disable.isra.74+0x7f/0x90 [i915]
> [ 24.197257] [<ffffffffa03f6c05>] haswell_crtc_disable+0x55/0x170 [i915]
> [ 24.197292] [<ffffffffa03fec88>] intel_atomic_commit_tail+0x108/0xfd0 [i915]
> [ 24.197295] [<ffffffff810d47c6>] ? __lock_is_held+0x66/0x90
> [ 24.197330] [<ffffffffa03fff79>] intel_atomic_commit+0x429/0x560 [i915]
> [ 24.197332] [<ffffffff81570186>] ?drm_atomic_add_affected_connectors+0x56/0xf0
> [ 24.197334] [<ffffffff8156f726>] drm_atomic_commit+0x46/0x50
> [ 24.197336] [<ffffffff81553f87>] restore_fbdev_mode+0x147/0x270
> [ 24.197337] [<ffffffff81555bee>] drm_fb_helper_restore_fbdev_mode_unlocked+0x2e/0x70
> [ 24.197339] [<ffffffff81555aa8>] drm_fb_helper_set_par+0x28/0x50
> [ 24.197374] [<ffffffffa041c7d3>] intel_fbdev_set_par+0x13/0x70 [i915]
> [ 24.197376] [<ffffffff8149e07a>] fbcon_init+0x57a/0x600
> [ 24.197379] [<ffffffff81514b71>] visual_init+0xd1/0x130
> [ 24.197381] [<ffffffff8151603c>] do_bind_con_driver+0x1bc/0x3a0
> [ 24.197384] [<ffffffff81516521>] do_take_over_console+0x111/0x180
> [ 24.197386] [<ffffffff8149e152>] do_fbcon_takeover+0x52/0xb0
> [ 24.197387] [<ffffffff814a12c3>] fbcon_event_notify+0x723/0x850
> [ 24.197390] [<ffffffff810a4830>] ?__blocking_notifier_call_chain+0x30/0x70
> [ 24.197392] [<ffffffff810a44a4>] notifier_call_chain+0x34/0xa0
> [ 24.197394] [<ffffffff810a4848>] __blocking_notifier_call_chain+0x48/0x70
> [ 24.197397] [<ffffffff810a4881>] blocking_notifier_call_chain+0x11/0x20
> [ 24.197398] [<ffffffff814a4556>] fb_notifier_call_chain+0x16/0x20
> [ 24.197400] [<ffffffff814a678c>] register_framebuffer+0x24c/0x330
> [ 24.197402] [<ffffffff815558d9>] drm_fb_helper_initial_config+0x219/0x3c0
> [ 24.197436] [<ffffffffa041d373>] intel_fbdev_initial_config+0x13/0x30 [i915]
> [ 24.197438] [<ffffffff810a5d44>] async_run_entry_fn+0x34/0x140
> [ 24.197440] [<ffffffff8109c26c>] process_one_work+0x1ec/0x6b0
> [ 24.197442] [<ffffffff8109c1e6>] ? process_one_work+0x166/0x6b0
> [ 24.197445] [<ffffffff8109c779>] worker_thread+0x49/0x490
> [ 24.197447] [<ffffffff8109c730>] ? process_one_work+0x6b0/0x6b0
> [ 24.197448] [<ffffffff810a2a9b>] kthread+0xeb/0x110
> [ 24.197451] [<ffffffff810a29b0>] ? kthread_park+0x60/0x60
> [ 24.197453] [<ffffffff818241a7>] ret_from_fork+0x27/0x40
> [ 24.197476] ---[ end trace bda64b683b8e8162 ]---
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
Do we still need this with patch 3? I know it'd be nice if we could
faithfully restore any state we can also program, but then that's also a
lot of complexity ...
Otoh patch 3 means we'll stop testing a lot of the fastboot code while
reloading the driver. But then that's been the thing in the past, and as
long as we still boot up we have at least some test coverage fo the
fastboot code (I'm mostly concerned about the plane/buffer readout code,
since that's not covered by the state checker).
But for now I'd say let's just go with patch 3 only.
-Daniel
> ---
> drivers/gpu/drm/i915/intel_ddi.c | 14 ++------------
> drivers/gpu/drm/i915/intel_display.c | 4 ++++
> drivers/gpu/drm/i915/intel_dp_mst.c | 9 ++-------
> 3 files changed, 8 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index d808a2ccc29e..8c9ce850760b 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -1835,8 +1835,6 @@ static void intel_enable_ddi(struct intel_encoder *intel_encoder,
> struct drm_connector_state *conn_state)
> {
> struct drm_encoder *encoder = &intel_encoder->base;
> - struct drm_crtc *crtc = encoder->crtc;
> - struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> struct drm_i915_private *dev_priv = to_i915(encoder->dev);
> enum port port = intel_ddi_get_encoder_port(intel_encoder);
> int type = intel_encoder->type;
> @@ -1863,10 +1861,8 @@ static void intel_enable_ddi(struct intel_encoder *intel_encoder,
> intel_edp_drrs_enable(intel_dp, pipe_config);
> }
>
> - if (intel_crtc->config->has_audio) {
> - intel_display_power_get(dev_priv, POWER_DOMAIN_AUDIO);
> + if (pipe_config->has_audio)
> intel_audio_codec_enable(intel_encoder, pipe_config, conn_state);
> - }
> }
>
> static void intel_disable_ddi(struct intel_encoder *intel_encoder,
> @@ -1874,16 +1870,10 @@ static void intel_disable_ddi(struct intel_encoder *intel_encoder,
> struct drm_connector_state *old_conn_state)
> {
> struct drm_encoder *encoder = &intel_encoder->base;
> - struct drm_crtc *crtc = encoder->crtc;
> - struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> int type = intel_encoder->type;
> - struct drm_device *dev = encoder->dev;
> - struct drm_i915_private *dev_priv = to_i915(dev);
>
> - if (intel_crtc->config->has_audio) {
> + if (old_crtc_state->has_audio)
> intel_audio_codec_disable(intel_encoder);
> - intel_display_power_put(dev_priv, POWER_DOMAIN_AUDIO);
> - }
>
> if (type == INTEL_OUTPUT_EDP) {
> struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index ac587ca3b2af..0daa54633450 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5735,6 +5735,7 @@ static unsigned long get_crtc_power_domains(struct drm_crtc *crtc,
> struct intel_crtc_state *crtc_state)
> {
> struct drm_device *dev = crtc->dev;
> + struct drm_i915_private *dev_priv = to_i915(dev);
> struct drm_encoder *encoder;
> struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> enum pipe pipe = intel_crtc->pipe;
> @@ -5756,6 +5757,9 @@ static unsigned long get_crtc_power_domains(struct drm_crtc *crtc,
> mask |= BIT(intel_display_port_power_domain(intel_encoder));
> }
>
> + if (HAS_DDI(dev_priv) && crtc_state->has_audio)
> + mask |= BIT(POWER_DOMAIN_AUDIO);
> +
> if (crtc_state->shared_dpll)
> mask |= BIT(POWER_DOMAIN_PLLS);
>
> diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
> index 205fe4748ec5..6c5382494fee 100644
> --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> @@ -87,7 +87,6 @@ static void intel_mst_disable_dp(struct intel_encoder *encoder,
> struct intel_dp *intel_dp = &intel_dig_port->dp;
> struct intel_connector *connector =
> to_intel_connector(old_conn_state->connector);
> - struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> int ret;
>
> DRM_DEBUG_KMS("%d\n", intel_dp->active_mst_links);
> @@ -98,10 +97,8 @@ static void intel_mst_disable_dp(struct intel_encoder *encoder,
> if (ret) {
> DRM_ERROR("failed to update payload %d\n", ret);
> }
> - if (old_crtc_state->has_audio) {
> + if (old_crtc_state->has_audio)
> intel_audio_codec_disable(encoder);
> - intel_display_power_put(dev_priv, POWER_DOMAIN_AUDIO);
> - }
> }
>
> static void intel_mst_post_disable_dp(struct intel_encoder *encoder,
> @@ -214,10 +211,8 @@ static void intel_mst_enable_dp(struct intel_encoder *encoder,
> ret = drm_dp_check_act_status(&intel_dp->mst_mgr);
>
> ret = drm_dp_update_payload_part2(&intel_dp->mst_mgr);
> - if (pipe_config->has_audio) {
> - intel_display_power_get(dev_priv, POWER_DOMAIN_AUDIO);
> + if (pipe_config->has_audio)
> intel_audio_codec_enable(encoder, pipe_config, conn_state);
> - }
> }
>
> static bool intel_dp_mst_enc_get_hw_state(struct intel_encoder *encoder,
> --
> 2.7.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
