| Message ID | 1480448429-27739-1-git-send-email-imre.deak@intel.com |
|---|---|
| State | Accepted |
| Commit | 908764f6d0bd1ba496cb8da33b9b98297ed27351 |
| Headers | show |
| Series |
"drm/i915/lspcon: Enable AUX interrupts for resume time initialization"
( rev:
2
)
in
Intel GFX |
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 8dac298..2cea2ef 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -1582,18 +1582,21 @@ static int i915_drm_resume(struct drm_device *dev) intel_opregion_setup(dev_priv); intel_init_pch_refclk(dev_priv); - drm_mode_config_reset(dev); /* * Interrupts have to be enabled before any batches are run. If not the * GPU will hang. i915_gem_init_hw() will initiate batches to * update/restore the context. * + * drm_mode_config_reset() needs AUX interrupts. + * * Modeset enabling in intel_modeset_init_hw() also needs working * interrupts. */ intel_runtime_pm_enable_interrupts(dev_priv); + drm_mode_config_reset(dev); + mutex_lock(&dev->struct_mutex); if (i915_gem_init_hw(dev)) { DRM_ERROR("failed to re-initialize GPU, declaring wedged!\n");
On Tue, Nov 29, 2016 at 09:40:29PM +0200, Imre Deak wrote: > For LSPCON initialization during system resume we need AUX > functionality, but we call the corresponding encoder reset hook with all > interrupts disabled. Without interrupts we'll do a poll-wait for AUX > transfer completions, which adds a significant delay if the transfers > timeout/need to be retried for some reason. > > Fix this by enabling interrupts before calling the reset hooks. Note > that while this will enable AUX interrupts it will keep HPD interrupts > disabled, in a similar way to the init time output setup code. > > This issue existed since LSPCON support was added. > > v2: > - Rebased on drm-tip. > > Cc: Shashank Sharma <shashank.sharma@intel.com> > Signed-off-by: Imre Deak <imre.deak@intel.com> > --- > drivers/gpu/drm/i915/i915_drv.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index 8dac298..2cea2ef 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -1582,18 +1582,21 @@ static int i915_drm_resume(struct drm_device *dev) > intel_opregion_setup(dev_priv); > > intel_init_pch_refclk(dev_priv); > - drm_mode_config_reset(dev); > > /* > * Interrupts have to be enabled before any batches are run. If not the > * GPU will hang. i915_gem_init_hw() will initiate batches to > * update/restore the context. > * > + * drm_mode_config_reset() needs AUX interrupts. > + * > * Modeset enabling in intel_modeset_init_hw() also needs working > * interrupts. > */ > intel_runtime_pm_enable_interrupts(dev_priv); > > + drm_mode_config_reset(dev); > + Didn't look like anything should seriously explode. Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> There is a slight concern on g4x/vlv/chv that an AUX interrupts would trigger the hpd irq handler, which doesn't realize it's supposed to ignore the actual hpd bits in PORT_HOTPLUG_STAT. So any aux before we enable hpd processing for real could do something bad. So I guess we should add some kind of software tracking for that stuff like we have for PIPESTAT. > mutex_lock(&dev->struct_mutex); > if (i915_gem_init_hw(dev)) { > DRM_ERROR("failed to re-initialize GPU, declaring wedged!\n"); > -- > 2.5.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, 2016-11-29 at 21:55 +0200, Ville Syrjälä wrote: > On Tue, Nov 29, 2016 at 09:40:29PM +0200, Imre Deak wrote: > > For LSPCON initialization during system resume we need AUX > > functionality, but we call the corresponding encoder reset hook with all > > interrupts disabled. Without interrupts we'll do a poll-wait for AUX > > transfer completions, which adds a significant delay if the transfers > > timeout/need to be retried for some reason. > > > > Fix this by enabling interrupts before calling the reset hooks. Note > > that while this will enable AUX interrupts it will keep HPD interrupts > > disabled, in a similar way to the init time output setup code. > > > > This issue existed since LSPCON support was added. > > > > v2: > > - Rebased on drm-tip. > > > > Cc: Shashank Sharma <shashank.sharma@intel.com> > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > --- > > drivers/gpu/drm/i915/i915_drv.c | 5 ++++- > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > > index 8dac298..2cea2ef 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.c > > +++ b/drivers/gpu/drm/i915/i915_drv.c > > @@ -1582,18 +1582,21 @@ static int i915_drm_resume(struct drm_device *dev) > > intel_opregion_setup(dev_priv); > > > > intel_init_pch_refclk(dev_priv); > > - drm_mode_config_reset(dev); > > > > /* > > * Interrupts have to be enabled before any batches are run. If not the > > * GPU will hang. i915_gem_init_hw() will initiate batches to > > * update/restore the context. > > * > > + * drm_mode_config_reset() needs AUX interrupts. > > + * > > * Modeset enabling in intel_modeset_init_hw() also needs working > > * interrupts. > > */ > > intel_runtime_pm_enable_interrupts(dev_priv); > > > > + drm_mode_config_reset(dev); > > + > > Didn't look like anything should seriously explode. > > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > There is a slight concern on g4x/vlv/chv that an AUX interrupts would > trigger the hpd irq handler, which doesn't realize it's supposed to > ignore the actual hpd bits in PORT_HOTPLUG_STAT. So any aux before we > enable hpd processing for real could do something bad. So I guess we > should add some kind of software tracking for that stuff like we have > for PIPESTAT. Didn't think about that, but BSpec tells me those are masked by the HPD IRQ enable bits in PORT_HOTPLUG_EN and those we enable only later. Otherwise this would be also a problem during output setup time. --Imre > > > mutex_lock(&dev->struct_mutex); > > if (i915_gem_init_hw(dev)) { > > DRM_ERROR("failed to re-initialize GPU, declaring wedged!\n"); > > -- > > 2.5.0 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx >
On Tue, Nov 29, 2016 at 10:14:23PM +0200, Imre Deak wrote: > On Tue, 2016-11-29 at 21:55 +0200, Ville Syrjälä wrote: > > On Tue, Nov 29, 2016 at 09:40:29PM +0200, Imre Deak wrote: > > > For LSPCON initialization during system resume we need AUX > > > functionality, but we call the corresponding encoder reset hook with all > > > interrupts disabled. Without interrupts we'll do a poll-wait for AUX > > > transfer completions, which adds a significant delay if the transfers > > > timeout/need to be retried for some reason. > > > > > > Fix this by enabling interrupts before calling the reset hooks. Note > > > that while this will enable AUX interrupts it will keep HPD interrupts > > > disabled, in a similar way to the init time output setup code. > > > > > > This issue existed since LSPCON support was added. > > > > > > v2: > > > - Rebased on drm-tip. > > > > > > Cc: Shashank Sharma <shashank.sharma@intel.com> > > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > > --- > > > drivers/gpu/drm/i915/i915_drv.c | 5 ++++- > > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > > > index 8dac298..2cea2ef 100644 > > > --- a/drivers/gpu/drm/i915/i915_drv.c > > > +++ b/drivers/gpu/drm/i915/i915_drv.c > > > @@ -1582,18 +1582,21 @@ static int i915_drm_resume(struct drm_device *dev) > > > intel_opregion_setup(dev_priv); > > > > > > intel_init_pch_refclk(dev_priv); > > > - drm_mode_config_reset(dev); > > > > > > /* > > > * Interrupts have to be enabled before any batches are run. If not the > > > * GPU will hang. i915_gem_init_hw() will initiate batches to > > > * update/restore the context. > > > * > > > + * drm_mode_config_reset() needs AUX interrupts. > > > + * > > > * Modeset enabling in intel_modeset_init_hw() also needs working > > > * interrupts. > > > */ > > > intel_runtime_pm_enable_interrupts(dev_priv); > > > > > > + drm_mode_config_reset(dev); > > > + > > > > Didn't look like anything should seriously explode. > > > > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > There is a slight concern on g4x/vlv/chv that an AUX interrupts would > > trigger the hpd irq handler, which doesn't realize it's supposed to > > ignore the actual hpd bits in PORT_HOTPLUG_STAT. So any aux before we > > enable hpd processing for real could do something bad. So I guess we > > should add some kind of software tracking for that stuff like we have > > for PIPESTAT. > > Didn't think about that, but BSpec tells me those are masked by the HPD > IRQ enable bits in PORT_HOTPLUG_EN and those we enable only later. > Otherwise this would be also a problem during output setup time. Hmm. Are they really masked? I though it's just an IER effectively. > > --Imre > > > > > > mutex_lock(&dev->struct_mutex); > > > if (i915_gem_init_hw(dev)) { > > > DRM_ERROR("failed to re-initialize GPU, declaring wedged!\n"); > > > -- > > > 2.5.0 > > > > > > _______________________________________________ > > > Intel-gfx mailing list > > > Intel-gfx@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > >
On Tue, 2016-11-29 at 22:27 +0200, Ville Syrjälä wrote: > On Tue, Nov 29, 2016 at 10:14:23PM +0200, Imre Deak wrote: > > On Tue, 2016-11-29 at 21:55 +0200, Ville Syrjälä wrote: > > > On Tue, Nov 29, 2016 at 09:40:29PM +0200, Imre Deak wrote: > > > > For LSPCON initialization during system resume we need AUX > > > > functionality, but we call the corresponding encoder reset hook > > > > with all > > > > interrupts disabled. Without interrupts we'll do a poll-wait > > > > for AUX > > > > transfer completions, which adds a significant delay if the > > > > transfers > > > > timeout/need to be retried for some reason. > > > > > > > > Fix this by enabling interrupts before calling the reset hooks. > > > > Note > > > > that while this will enable AUX interrupts it will keep HPD > > > > interrupts > > > > disabled, in a similar way to the init time output setup code. > > > > > > > > This issue existed since LSPCON support was added. > > > > > > > > v2: > > > > - Rebased on drm-tip. > > > > > > > > Cc: Shashank Sharma <shashank.sharma@intel.com> > > > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > > > --- > > > > drivers/gpu/drm/i915/i915_drv.c | 5 ++++- > > > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c > > > > b/drivers/gpu/drm/i915/i915_drv.c > > > > index 8dac298..2cea2ef 100644 > > > > --- a/drivers/gpu/drm/i915/i915_drv.c > > > > +++ b/drivers/gpu/drm/i915/i915_drv.c > > > > @@ -1582,18 +1582,21 @@ static int i915_drm_resume(struct > > > > drm_device *dev) > > > > intel_opregion_setup(dev_priv); > > > > > > > > intel_init_pch_refclk(dev_priv); > > > > - drm_mode_config_reset(dev); > > > > > > > > /* > > > > * Interrupts have to be enabled before any batches > > > > are run. If not the > > > > * GPU will hang. i915_gem_init_hw() will initiate > > > > batches to > > > > * update/restore the context. > > > > * > > > > + * drm_mode_config_reset() needs AUX interrupts. > > > > + * > > > > * Modeset enabling in intel_modeset_init_hw() also > > > > needs working > > > > * interrupts. > > > > */ > > > > intel_runtime_pm_enable_interrupts(dev_priv); > > > > > > > > + drm_mode_config_reset(dev); > > > > + > > > > > > Didn't look like anything should seriously explode. > > > > > > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > There is a slight concern on g4x/vlv/chv that an AUX interrupts > > > would > > > trigger the hpd irq handler, which doesn't realize it's supposed > > > to > > > ignore the actual hpd bits in PORT_HOTPLUG_STAT. So any aux > > > before we > > > enable hpd processing for real could do something bad. So I guess > > > we > > > should add some kind of software tracking for that stuff like we > > > have > > > for PIPESTAT. > > > > Didn't think about that, but BSpec tells me those are masked by the > > HPD > > IRQ enable bits in PORT_HOTPLUG_EN and those we enable only later. > > Otherwise this would be also a problem during output setup time. > > Hmm. Are they really masked? I though it's just an IER effectively. I only tried for real on BXT/SKL where I had to enable the interrupts (in PCH_PORT_HOTPLUG) for HPD sensing. The CHV BSpec suggests the same for the live state bits, but yes it's not clear if the long/short detect bits are completely masked by the enable flags or they are just not propagated if not enabled. Will give it a try tomorrow. > > > > > --Imre > > > > > > > > > mutex_lock(&dev->struct_mutex); > > > > if (i915_gem_init_hw(dev)) { > > > > DRM_ERROR("failed to re-initialize GPU, > > > > declaring wedged!\n"); > > > > -- > > > > 2.5.0 > > > > > > > > _______________________________________________ > > > > Intel-gfx mailing list > > > > Intel-gfx@lists.freedesktop.org > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > >
On Tue, Nov 29, 2016 at 10:41:45PM +0200, Imre Deak wrote: > On Tue, 2016-11-29 at 22:27 +0200, Ville Syrjälä wrote: > > On Tue, Nov 29, 2016 at 10:14:23PM +0200, Imre Deak wrote: > > > On Tue, 2016-11-29 at 21:55 +0200, Ville Syrjälä wrote: > > > > On Tue, Nov 29, 2016 at 09:40:29PM +0200, Imre Deak wrote: > > > > > For LSPCON initialization during system resume we need AUX > > > > > functionality, but we call the corresponding encoder reset hook > > > > > with all > > > > > interrupts disabled. Without interrupts we'll do a poll-wait > > > > > for AUX > > > > > transfer completions, which adds a significant delay if the > > > > > transfers > > > > > timeout/need to be retried for some reason. > > > > > > > > > > Fix this by enabling interrupts before calling the reset hooks. > > > > > Note > > > > > that while this will enable AUX interrupts it will keep HPD > > > > > interrupts > > > > > disabled, in a similar way to the init time output setup code. > > > > > > > > > > This issue existed since LSPCON support was added. > > > > > > > > > > v2: > > > > > - Rebased on drm-tip. > > > > > > > > > > Cc: Shashank Sharma <shashank.sharma@intel.com> > > > > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > > > > --- > > > > > drivers/gpu/drm/i915/i915_drv.c | 5 ++++- > > > > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c > > > > > b/drivers/gpu/drm/i915/i915_drv.c > > > > > index 8dac298..2cea2ef 100644 > > > > > --- a/drivers/gpu/drm/i915/i915_drv.c > > > > > +++ b/drivers/gpu/drm/i915/i915_drv.c > > > > > @@ -1582,18 +1582,21 @@ static int i915_drm_resume(struct > > > > > drm_device *dev) > > > > > intel_opregion_setup(dev_priv); > > > > > > > > > > intel_init_pch_refclk(dev_priv); > > > > > - drm_mode_config_reset(dev); > > > > > > > > > > /* > > > > > * Interrupts have to be enabled before any batches > > > > > are run. If not the > > > > > * GPU will hang. i915_gem_init_hw() will initiate > > > > > batches to > > > > > * update/restore the context. > > > > > * > > > > > + * drm_mode_config_reset() needs AUX interrupts. > > > > > + * > > > > > * Modeset enabling in intel_modeset_init_hw() also > > > > > needs working > > > > > * interrupts. > > > > > */ > > > > > intel_runtime_pm_enable_interrupts(dev_priv); > > > > > > > > > > + drm_mode_config_reset(dev); > > > > > + > > > > > > > > Didn't look like anything should seriously explode. > > > > > > > > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > > > There is a slight concern on g4x/vlv/chv that an AUX interrupts > > > > would > > > > trigger the hpd irq handler, which doesn't realize it's supposed > > > > to > > > > ignore the actual hpd bits in PORT_HOTPLUG_STAT. So any aux > > > > before we > > > > enable hpd processing for real could do something bad. So I guess > > > > we > > > > should add some kind of software tracking for that stuff like we > > > > have > > > > for PIPESTAT. > > > > > > Didn't think about that, but BSpec tells me those are masked by the > > > HPD > > > IRQ enable bits in PORT_HOTPLUG_EN and those we enable only later. > > > Otherwise this would be also a problem during output setup time. > > > > Hmm. Are they really masked? I though it's just an IER effectively. > > I only tried for real on BXT/SKL where I had to enable the interrupts > (in PCH_PORT_HOTPLUG) for HPD sensing. The CHV BSpec suggests the same > for the live state bits, but yes it's not clear if the long/short > detect bits are completely masked by the enable flags or they are just > not propagated if not enabled. Will give it a try tomorrow. Hmm. Yeah, we did in fact chat about this. Already forgot. Spec seems to suggest you are correct. But checking on actual hw is always a good idea. > > > > > > > > > --Imre > > > > > > > > > > > > mutex_lock(&dev->struct_mutex); > > > > > if (i915_gem_init_hw(dev)) { > > > > > DRM_ERROR("failed to re-initialize GPU, > > > > > declaring wedged!\n"); > > > > > -- > > > > > 2.5.0 > > > > > > > > > > _______________________________________________ > > > > > Intel-gfx mailing list > > > > > Intel-gfx@lists.freedesktop.org > > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > > >
On ti, 2016-11-29 at 23:00 +0200, Ville Syrjälä wrote: > [...] > > > > > There is a slight concern on g4x/vlv/chv that an AUX interrupts > > > > > would > > > > > trigger the hpd irq handler, which doesn't realize it's supposed > > > > > to > > > > > ignore the actual hpd bits in PORT_HOTPLUG_STAT. So any aux > > > > > before we > > > > > enable hpd processing for real could do something bad. So I guess > > > > > we > > > > > should add some kind of software tracking for that stuff like we > > > > > have > > > > > for PIPESTAT. > > > > > > > > Didn't think about that, but BSpec tells me those are masked by the > > > > HPD > > > > IRQ enable bits in PORT_HOTPLUG_EN and those we enable only later. > > > > Otherwise this would be also a problem during output setup time. > > > > > > Hmm. Are they really masked? I though it's just an IER effectively. > > > > I only tried for real on BXT/SKL where I had to enable the interrupts > > (in PCH_PORT_HOTPLUG) for HPD sensing. The CHV BSpec suggests the same > > for the live state bits, but yes it's not clear if the long/short > > detect bits are completely masked by the enable flags or they are just > > not propagated if not enabled. Will give it a try tomorrow. > > Hmm. Yeah, we did in fact chat about this. Already forgot. Spec seems to > suggest you are correct. But checking on actual hw is always a good > idea. Checked now both on BXT and VLV both the live state and short/long detect bits in the hotplug_stat reg are masked by the hotplug_en bits. (And we clear any stale short/long bits during IRQ reset.) --Imre
On Tue, Nov 29, 2016 at 09:40:29PM +0200, Imre Deak wrote: > For LSPCON initialization during system resume we need AUX > functionality, but we call the corresponding encoder reset hook with all > interrupts disabled. Without interrupts we'll do a poll-wait for AUX > transfer completions, which adds a significant delay if the transfers > timeout/need to be retried for some reason. > > Fix this by enabling interrupts before calling the reset hooks. Note > that while this will enable AUX interrupts it will keep HPD interrupts > disabled, in a similar way to the init time output setup code. > > This issue existed since LSPCON support was added. > > v2: > - Rebased on drm-tip. > > Cc: Shashank Sharma <shashank.sharma@intel.com> > Signed-off-by: Imre Deak <imre.deak@intel.com> Tested-by: David Weinehall <david.weinehall@linux.intel.com> > --- > drivers/gpu/drm/i915/i915_drv.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index 8dac298..2cea2ef 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -1582,18 +1582,21 @@ static int i915_drm_resume(struct drm_device *dev) > intel_opregion_setup(dev_priv); > > intel_init_pch_refclk(dev_priv); > - drm_mode_config_reset(dev); > > /* > * Interrupts have to be enabled before any batches are run. If not the > * GPU will hang. i915_gem_init_hw() will initiate batches to > * update/restore the context. > * > + * drm_mode_config_reset() needs AUX interrupts. > + * > * Modeset enabling in intel_modeset_init_hw() also needs working > * interrupts. > */ > intel_runtime_pm_enable_interrupts(dev_priv); > > + drm_mode_config_reset(dev); > + > mutex_lock(&dev->struct_mutex); > if (i915_gem_init_hw(dev)) { > DRM_ERROR("failed to re-initialize GPU, declaring wedged!\n"); > -- > 2.5.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
For LSPCON initialization during system resume we need AUX functionality, but we call the corresponding encoder reset hook with all interrupts disabled. Without interrupts we'll do a poll-wait for AUX transfer completions, which adds a significant delay if the transfers timeout/need to be retried for some reason. Fix this by enabling interrupts before calling the reset hooks. Note that while this will enable AUX interrupts it will keep HPD interrupts disabled, in a similar way to the init time output setup code. This issue existed since LSPCON support was added. v2: - Rebased on drm-tip. Cc: Shashank Sharma <shashank.sharma@intel.com> Signed-off-by: Imre Deak <imre.deak@intel.com> --- drivers/gpu/drm/i915/i915_drv.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)