From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga07.intel.com ([134.134.136.100]:4251 "EHLO mga07.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752099AbdA2E76 (ORCPT ); Sat, 28 Jan 2017 23:59:58 -0500 Subject: Re: [PATCH 1/4] drm/i915/gen9+: Enable hotplug detection early To: imre.deak@intel.com References: <1485509961-9010-1-git-send-email-imre.deak@intel.com> <1485509961-9010-2-git-send-email-imre.deak@intel.com> <20170128075409.GA32065@ideak-desk.fi.intel.com> Cc: intel-gfx@lists.freedesktop.org, Jani Nikula , =?UTF-8?B?VmlsbGUgU3lyasOkbMOk?= , Daniel Vetter , stable@vger.kernel.org From: "Sharma, Shashank" Message-ID: <8383a201-23b4-e96e-047e-a3287d0d9377@intel.com> Date: Sun, 29 Jan 2017 10:26:21 +0530 MIME-Version: 1.0 In-Reply-To: <20170128075409.GA32065@ideak-desk.fi.intel.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 8bit Sender: stable-owner@vger.kernel.org List-ID: Regards Shashank On 1/28/2017 1:24 PM, Imre Deak wrote: > On Sat, Jan 28, 2017 at 10:24:19AM +0530, Sharma, Shashank wrote: >> Regards >> >> Shashank >> >> >> On 1/27/2017 3:09 PM, Imre Deak wrote: >>> For LSPCON resume time initialization we need to sample the >>> corresponding pin's HPD level, but this is only available when HPD >>> detection is enabled. Currently we enable detection only when enabling >>> HPD interrupts which is too late, so bring the enabling of detection >>> earlier. >>> >>> This is needed by the next patch. >>> >>> Cc: Shashank Sharma >>> Cc: Jani Nikula >>> Cc: Ville Syrj�l� >>> Cc: Daniel Vetter >>> Cc: # v4.9+ >>> Signed-off-by: Imre Deak >>> --- >>> drivers/gpu/drm/i915/i915_irq.c | 71 +++++++++++++++++++++++++++++------------ >>> 1 file changed, 51 insertions(+), 20 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c >>> index 8440d8b..6daf522 100644 >>> --- a/drivers/gpu/drm/i915/i915_irq.c >>> +++ b/drivers/gpu/drm/i915/i915_irq.c >>> @@ -3142,24 +3142,33 @@ static void ibx_hpd_irq_setup(struct drm_i915_private *dev_priv) >>> I915_WRITE(PCH_PORT_HOTPLUG, hotplug); >>> } >>> +static void spt_hpd_detection_setup(struct drm_i915_private *dev_priv) >>> +{ >>> + u32 hotplug; >>> + >>> + /* Enable digital hotplug on the PCH */ >>> + hotplug = I915_READ(PCH_PORT_HOTPLUG); >>> + hotplug |= PORTA_HOTPLUG_ENABLE | >>> + PORTB_HOTPLUG_ENABLE | >>> + PORTC_HOTPLUG_ENABLE | >>> + PORTD_HOTPLUG_ENABLE; >>> + I915_WRITE(PCH_PORT_HOTPLUG, hotplug); >>> + >>> + hotplug = I915_READ(PCH_PORT_HOTPLUG2); >>> + hotplug |= PORTE_HOTPLUG_ENABLE; >>> + I915_WRITE(PCH_PORT_HOTPLUG2, hotplug); >>> +} >>> + >>> static void spt_hpd_irq_setup(struct drm_i915_private *dev_priv) >>> { >>> - u32 hotplug_irqs, hotplug, enabled_irqs; >>> + u32 hotplug_irqs, enabled_irqs; >>> hotplug_irqs = SDE_HOTPLUG_MASK_SPT; >>> enabled_irqs = intel_hpd_enabled_irqs(dev_priv, hpd_spt); >>> ibx_display_interrupt_update(dev_priv, hotplug_irqs, enabled_irqs); >>> - /* Enable digital hotplug on the PCH */ >>> - hotplug = I915_READ(PCH_PORT_HOTPLUG); >>> - hotplug |= PORTD_HOTPLUG_ENABLE | PORTC_HOTPLUG_ENABLE | >>> - PORTB_HOTPLUG_ENABLE | PORTA_HOTPLUG_ENABLE; >>> - I915_WRITE(PCH_PORT_HOTPLUG, hotplug); >>> - >>> - hotplug = I915_READ(PCH_PORT_HOTPLUG2); >>> - hotplug |= PORTE_HOTPLUG_ENABLE; >>> - I915_WRITE(PCH_PORT_HOTPLUG2, hotplug); >>> + spt_hpd_detection_setup(dev_priv); >>> } >>> static void ilk_hpd_irq_setup(struct drm_i915_private *dev_priv) >>> @@ -3196,18 +3205,15 @@ static void ilk_hpd_irq_setup(struct drm_i915_private *dev_priv) >>> ibx_hpd_irq_setup(dev_priv); >>> } >>> -static void bxt_hpd_irq_setup(struct drm_i915_private *dev_priv) >>> +static void __bxt_hpd_detection_setup(struct drm_i915_private *dev_priv, >>> + u32 enabled_irqs) >>> { >>> - u32 hotplug_irqs, hotplug, enabled_irqs; >>> - >>> - enabled_irqs = intel_hpd_enabled_irqs(dev_priv, hpd_bxt); >>> - hotplug_irqs = BXT_DE_PORT_HOTPLUG_MASK; >>> - >>> - bdw_update_port_irq(dev_priv, hotplug_irqs, enabled_irqs); >>> + u32 hotplug; >>> hotplug = I915_READ(PCH_PORT_HOTPLUG); >>> - hotplug |= PORTC_HOTPLUG_ENABLE | PORTB_HOTPLUG_ENABLE | >>> - PORTA_HOTPLUG_ENABLE; >>> + hotplug |= PORTA_HOTPLUG_ENABLE | >>> + PORTB_HOTPLUG_ENABLE | >>> + PORTC_HOTPLUG_ENABLE; >>> DRM_DEBUG_KMS("Invert bit setting: hp_ctl:%x hp_port:%x\n", >>> hotplug, enabled_irqs); >>> @@ -3217,7 +3223,6 @@ static void bxt_hpd_irq_setup(struct drm_i915_private *dev_priv) >>> * For BXT invert bit has to be set based on AOB design >>> * for HPD detection logic, update it based on VBT fields. >>> */ >>> - >>> if ((enabled_irqs & BXT_DE_PORT_HP_DDIA) && >>> intel_bios_is_port_hpd_inverted(dev_priv, PORT_A)) >>> hotplug |= BXT_DDIA_HPD_INVERT; >>> @@ -3231,6 +3236,23 @@ static void bxt_hpd_irq_setup(struct drm_i915_private *dev_priv) >>> I915_WRITE(PCH_PORT_HOTPLUG, hotplug); >>> } >>> +static void bxt_hpd_detection_setup(struct drm_i915_private *dev_priv) >>> +{ >>> + __bxt_hpd_detection_setup(dev_priv, BXT_DE_PORT_HOTPLUG_MASK); >> Do we need another layer of internal function ? Why cant the content of >> __bxt_hpd_detection_setup be here, just >> like spd_hpd_detection_setup, as they both are static to the file ? > The port configuration is platform specific and so I wanted to keep it > close to where the rest of HPD setup is done. The caller of > bxt_hpd_detection_setup() only cares about enabling detection on _all_ > ports as the other *_hpd_detection_setup() helpers. I am not very convinced, but seems no harm too :) >>> +} >>> + >>> +static void bxt_hpd_irq_setup(struct drm_i915_private *dev_priv) >>> +{ >>> + u32 hotplug_irqs, enabled_irqs; >>> + >>> + enabled_irqs = intel_hpd_enabled_irqs(dev_priv, hpd_bxt); >>> + hotplug_irqs = BXT_DE_PORT_HOTPLUG_MASK; >>> + >>> + bdw_update_port_irq(dev_priv, hotplug_irqs, enabled_irqs); >>> + >>> + __bxt_hpd_detection_setup(dev_priv, enabled_irqs); >>> +} >>> + >>> static void ibx_irq_postinstall(struct drm_device *dev) >>> { >>> struct drm_i915_private *dev_priv = to_i915(dev); >>> @@ -3246,6 +3268,12 @@ static void ibx_irq_postinstall(struct drm_device *dev) >>> gen5_assert_iir_is_zero(dev_priv, SDEIIR); >>> I915_WRITE(SDEIMR, ~mask); >>> + >>> + if (HAS_PCH_IBX(dev_priv) || HAS_PCH_CPT(dev_priv) || >>> + HAS_PCH_LPT(dev_priv)) >>> + ; /* TODO: Enable HPD detection on older PCH platforms too */ >>> + else >>> + spt_hpd_detection_setup(dev_priv); >>> } >>> static void gen5_gt_irq_postinstall(struct drm_device *dev) >>> @@ -3457,6 +3485,9 @@ static void gen8_de_irq_postinstall(struct drm_i915_private *dev_priv) >>> GEN5_IRQ_INIT(GEN8_DE_PORT_, ~de_port_masked, de_port_enables); >>> GEN5_IRQ_INIT(GEN8_DE_MISC_, ~de_misc_masked, de_misc_masked); >>> + >>> + if (IS_GEN9_LP(dev_priv)) >> I have not done the delta analysis between hotplug interrupts, but this will >> be true for GLK too. >> Should we bother ? > The GLK HPD detection setup and interrupt setup and handling is the same as > on BXT. See bxt_hpd_irq_setup() and bxt_hpd_irq_handler() that are > called on GLK as well. Yep, I was wandering about SCDC kind of interrupts, which are new in GLK, but probably I should take care of it, while writing the code for that. Reviewed-by: Shashank Sharma >> - Shashank >>> + bxt_hpd_detection_setup(dev_priv); >>> } >>> static int gen8_irq_postinstall(struct drm_device *dev)