From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 577D7C32772 for ; Tue, 23 Aug 2022 21:48:21 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 2A36710E311; Tue, 23 Aug 2022 21:48:19 +0000 (UTC) Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by gabe.freedesktop.org (Postfix) with ESMTPS id 8CDB610E455; Tue, 23 Aug 2022 21:48:06 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1661291286; x=1692827286; h=message-id:date:mime-version:subject:to:cc:references: from:in-reply-to:content-transfer-encoding; bh=bw4Ds13CePXLYMTtWNasckF7SzL1i+QEDzeQP0uwAaw=; b=Xh39q+RPYWvwM30Ihk/kr2hq9pQQQqxNdoXQs6WrLpAh8k2a26cztrnz UiA73W8kOi1XevV7+CjyXeR/37W34U5GalVUUp2QSls0iKOi2i6X6A+m+ tpDz3/W0EbLdZOAmNJx4QuHnWhrtb4yY0q2cVt+2TXWdD8mzzsB6A2gog Nxwft90YSjCw7E1lUM9t02q0WcJ5pXm6N5VowDe6UEmtGKeQC4Mlgn5NU JfvcHwQUG1VOyJu9vaVstAS1YGrgMo3XnVMntqI2s/7Hys9pezdvkv7E1 AniG9z765htAxKkW4wPuDv7qvU3FQ8O/jI1hv0mdtkhKtRt5gtEMJHoAZ w==; X-IronPort-AV: E=McAfee;i="6500,9779,10448"; a="319855109" X-IronPort-AV: E=Sophos;i="5.93,258,1654585200"; d="scan'208";a="319855109" Received: from orsmga008.jf.intel.com ([10.7.209.65]) by fmsmga101.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 23 Aug 2022 14:48:06 -0700 X-IronPort-AV: E=Sophos;i="5.93,258,1654585200"; d="scan'208";a="638834040" Received: from ahajda-mobl.ger.corp.intel.com (HELO [10.213.12.34]) ([10.213.12.34]) by orsmga008-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 23 Aug 2022 14:48:03 -0700 Message-ID: <45384b73-bfd0-083c-98dc-e2cef51411ee@intel.com> Date: Tue, 23 Aug 2022 23:48:01 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101 Firefox/91.0 Thunderbird/91.12.0 Content-Language: en-US To: imre.deak@intel.com References: <20220722125143.1604709-1-andrzej.hajda@intel.com> <20220722125143.1604709-4-andrzej.hajda@intel.com> From: Andrzej Hajda Organization: Intel Technology Poland sp. z o.o. - ul. Slowackiego 173, 80-298 Gdansk - KRS 101882 - NIP 957-07-52-316 In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Intel-gfx] [PATCH v6 3/4] drm/i915/display: add hotplug.suspended flag X-BeenThere: intel-gfx@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Intel graphics driver community testing & development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org, Rodrigo Vivi Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" On 22.08.2022 19:27, Imre Deak wrote: > On Fri, Jul 22, 2022 at 02:51:42PM +0200, Andrzej Hajda wrote: >> HPD events during driver removal can be generated by hardware and >> software frameworks - drm_dp_mst, the former we can avoid by disabling >> interrupts, the latter can be triggered by any drm_dp_mst transaction, >> and this is too late. Introducing suspended flag allows to solve this >> chicken-egg problem. > intel_hpd_cancel_work() is always called after suspending MST and > disabling IRQs (with the order I suggested in patch 1). If both of these > have disabled the corresponding functionality (MST, IRQs) properly with > all their MST/IRQ scheduled works guaranteed to not get rescheduled, > then it's not clear how could either intel_hpd_trigger_irq() or an IRQ > work run. So the problematic sequence would need a better explanation. I am not familiar with MST but as I understand from earlier discussion MST framework can be called during driver removal code even after intel_dp_mst_suspend. And since MST transfer can timeout it can trigger drm_dp_mst_wait_tx_reply --> mgr->cbs->poll_hpd_irq(mgr) --> intel_dp_mst_poll_hpd_irq --> intel_hpd_trigger_irq. So actually this patch supersedes the 1st patch. > > There's also already > dev_priv->runtime_pm.irqs_enabled > showing if hotplug interrupts are disabled (along with all other IRQs). So it is slightly different, this patch introduces flag indicating if HPD is enabled, we can have IRQs not related to HPD, and HPD events not related to IRQs. Regards Andrzej > >> Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/5950 >> Signed-off-by: Andrzej Hajda >> Reviewed-by: Arun R Murthy >> --- >> drivers/gpu/drm/i915/display/intel_display.c | 2 +- >> drivers/gpu/drm/i915/display/intel_hotplug.c | 11 ++++++++++- >> drivers/gpu/drm/i915/display/intel_hotplug.h | 2 +- >> drivers/gpu/drm/i915/i915_driver.c | 4 ++-- >> drivers/gpu/drm/i915/i915_drv.h | 2 ++ >> 5 files changed, 16 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c >> index f1c765ac7ab8aa..cd6139bb36151b 100644 >> --- a/drivers/gpu/drm/i915/display/intel_display.c >> +++ b/drivers/gpu/drm/i915/display/intel_display.c >> @@ -9022,7 +9022,7 @@ void intel_modeset_driver_remove_noirq(struct drm_i915_private *i915) >> intel_dp_mst_suspend(i915); >> >> /* MST is the last user of HPD work */ >> - intel_hpd_cancel_work(i915); >> + intel_hpd_suspend(i915); >> >> /* poll work can call into fbdev, hence clean that up afterwards */ >> intel_fbdev_fini(i915); >> diff --git a/drivers/gpu/drm/i915/display/intel_hotplug.c b/drivers/gpu/drm/i915/display/intel_hotplug.c >> index 5f8b4f481cff9a..e1d384cb99df6b 100644 >> --- a/drivers/gpu/drm/i915/display/intel_hotplug.c >> +++ b/drivers/gpu/drm/i915/display/intel_hotplug.c >> @@ -303,6 +303,8 @@ static void i915_digport_work_func(struct work_struct *work) >> u32 old_bits = 0; >> >> spin_lock_irq(&dev_priv->irq_lock); >> + if (dev_priv->hotplug.suspended) >> + return spin_unlock_irq(&dev_priv->irq_lock); >> long_port_mask = dev_priv->hotplug.long_port_mask; >> dev_priv->hotplug.long_port_mask = 0; >> short_port_mask = dev_priv->hotplug.short_port_mask; >> @@ -353,6 +355,8 @@ void intel_hpd_trigger_irq(struct intel_digital_port *dig_port) >> struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev); >> >> spin_lock_irq(&i915->irq_lock); >> + if (i915->hotplug.suspended) >> + return spin_unlock_irq(&i915->irq_lock); >> i915->hotplug.short_port_mask |= BIT(dig_port->base.port); >> spin_unlock_irq(&i915->irq_lock); >> >> @@ -475,6 +479,9 @@ void intel_hpd_irq_handler(struct drm_i915_private *dev_priv, >> >> spin_lock(&dev_priv->irq_lock); >> >> + if (dev_priv->hotplug.suspended) >> + return spin_unlock(&dev_priv->irq_lock); >> + >> /* >> * Determine whether ->hpd_pulse() exists for each pin, and >> * whether we have a short or a long pulse. This is needed >> @@ -603,6 +610,7 @@ void intel_hpd_init(struct drm_i915_private *dev_priv) >> * just to make the assert_spin_locked checks happy. >> */ >> spin_lock_irq(&dev_priv->irq_lock); >> + dev_priv->hotplug.suspended = false; >> intel_hpd_irq_setup(dev_priv); >> spin_unlock_irq(&dev_priv->irq_lock); >> } >> @@ -721,13 +729,14 @@ void intel_hpd_init_work(struct drm_i915_private *dev_priv) >> intel_hpd_irq_storm_reenable_work); >> } >> >> -void intel_hpd_cancel_work(struct drm_i915_private *dev_priv) >> +void intel_hpd_suspend(struct drm_i915_private *dev_priv) >> { >> if (!HAS_DISPLAY(dev_priv)) >> return; >> >> spin_lock_irq(&dev_priv->irq_lock); >> >> + dev_priv->hotplug.suspended = true; >> dev_priv->hotplug.long_port_mask = 0; >> dev_priv->hotplug.short_port_mask = 0; >> dev_priv->hotplug.event_bits = 0; >> diff --git a/drivers/gpu/drm/i915/display/intel_hotplug.h b/drivers/gpu/drm/i915/display/intel_hotplug.h >> index b87e95d606e668..54bddc4dd63421 100644 >> --- a/drivers/gpu/drm/i915/display/intel_hotplug.h >> +++ b/drivers/gpu/drm/i915/display/intel_hotplug.h >> @@ -23,7 +23,7 @@ void intel_hpd_irq_handler(struct drm_i915_private *dev_priv, >> void intel_hpd_trigger_irq(struct intel_digital_port *dig_port); >> void intel_hpd_init(struct drm_i915_private *dev_priv); >> void intel_hpd_init_work(struct drm_i915_private *dev_priv); >> -void intel_hpd_cancel_work(struct drm_i915_private *dev_priv); >> +void intel_hpd_suspend(struct drm_i915_private *dev_priv); >> enum hpd_pin intel_hpd_pin_default(struct drm_i915_private *dev_priv, >> enum port port); >> bool intel_hpd_disable(struct drm_i915_private *dev_priv, enum hpd_pin pin); >> diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c >> index deb8a8b76965a1..57a063a306e3a4 100644 >> --- a/drivers/gpu/drm/i915/i915_driver.c >> +++ b/drivers/gpu/drm/i915/i915_driver.c >> @@ -1092,7 +1092,7 @@ void i915_driver_shutdown(struct drm_i915_private *i915) >> intel_dp_mst_suspend(i915); >> >> intel_runtime_pm_disable_interrupts(i915); >> - intel_hpd_cancel_work(i915); >> + intel_hpd_suspend(i915); >> >> intel_suspend_encoders(i915); >> intel_shutdown_encoders(i915); >> @@ -1161,7 +1161,7 @@ static int i915_drm_suspend(struct drm_device *dev) >> intel_dp_mst_suspend(dev_priv); >> >> intel_runtime_pm_disable_interrupts(dev_priv); >> - intel_hpd_cancel_work(dev_priv); >> + intel_hpd_suspend(dev_priv); >> >> intel_suspend_encoders(dev_priv); >> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h >> index d25647be25d18b..dc1562b95d7ade 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.h >> +++ b/drivers/gpu/drm/i915/i915_drv.h >> @@ -106,6 +106,8 @@ struct vlv_s0ix_state; >> #define HPD_STORM_DEFAULT_THRESHOLD 50 >> >> struct i915_hotplug { >> + bool suspended; >> + >> struct delayed_work hotplug_work; >> >> const u32 *hpd, *pch_hpd; >> -- >> 2.25.1 >>