| Message ID | 1436162033-22654-1-git-send-email-sonika.jindal@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Jul 06, 2015 at 11:23:53AM +0530, Sonika Jindal wrote: > Writing to PCH_PORT_HOTPLUG for each interrupt is not required. > Handle it only if hpd has actually occurred like we handle other > interrupts. > > Signed-off-by: Sonika Jindal <sonika.jindal@intel.com> > --- > Hi, > > I see we don't check for hotplug_trigger before processing the HPD for any of the platform. Is there any reason for this? > For SKL, if I let write to PCH_PORT_HOTPLUG happen for all interrupts, somehow this register gets an invalid value at one point and it zeroes it out. > If I put this check before handling HPD, hotplug behaves fine. > Please let me know if you see any issue with this approach. Nice find, this sounds really intrigueing, at least for cpt/ibx platforms. I'm not sure whether what will happen with atom/i9xx platforms though since the irq bits are different there. But at least bxt has a FIXME comment that suggest we do need to save the sticky bits on those platforms too. If we can fix this up for all platforms then I think a subsequent patch could try to re-enable the hpd checks in the hdmi ->detect function and make use spec compliant. Then after maybe 1-2 kernel releases of testing we'll know whether it really works. But I'd really want to enable this everywhere just to have maximal test coverage - we did have reports on all platforms so it seems a generic issue. Thanks, Daniel > > Thanks, > Sonika > > drivers/gpu/drm/i915/i915_irq.c | 11 ++++++----- > 1 file changed, 6 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > index a6fbe64..2d47372 100644 > --- a/drivers/gpu/drm/i915/i915_irq.c > +++ b/drivers/gpu/drm/i915/i915_irq.c > @@ -1760,11 +1760,12 @@ static void cpt_irq_handler(struct drm_device *dev, u32 pch_iir) > u32 dig_hotplug_reg; > u32 pin_mask, long_mask; > > - dig_hotplug_reg = I915_READ(PCH_PORT_HOTPLUG); > - I915_WRITE(PCH_PORT_HOTPLUG, dig_hotplug_reg); > - > - pch_get_hpd_pins(&pin_mask, &long_mask, hotplug_trigger, dig_hotplug_reg, hpd_cpt); > - intel_hpd_irq_handler(dev, pin_mask, long_mask); > + if (hotplug_trigger) { > + dig_hotplug_reg = I915_READ(PCH_PORT_HOTPLUG); > + I915_WRITE(PCH_PORT_HOTPLUG, dig_hotplug_reg); > + pch_get_hpd_pins(&pin_mask, &long_mask, hotplug_trigger, dig_hotplug_reg, hpd_cpt); > + intel_hpd_irq_handler(dev, pin_mask, long_mask); > + } > > if (pch_iir & SDE_AUDIO_POWER_MASK_CPT) { > int port = ffs((pch_iir & SDE_AUDIO_POWER_MASK_CPT) >> > -- > 1.7.10.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On 7/6/2015 2:06 PM, Daniel Vetter wrote: > On Mon, Jul 06, 2015 at 11:23:53AM +0530, Sonika Jindal wrote: >> Writing to PCH_PORT_HOTPLUG for each interrupt is not required. >> Handle it only if hpd has actually occurred like we handle other >> interrupts. >> >> Signed-off-by: Sonika Jindal <sonika.jindal@intel.com> >> --- >> Hi, >> >> I see we don't check for hotplug_trigger before processing the HPD for any of the platform. Is there any reason for this? >> For SKL, if I let write to PCH_PORT_HOTPLUG happen for all interrupts, somehow this register gets an invalid value at one point and it zeroes it out. >> If I put this check before handling HPD, hotplug behaves fine. >> Please let me know if you see any issue with this approach. > > Nice find, this sounds really intrigueing, at least for cpt/ibx platforms. > I'm not sure whether what will happen with atom/i9xx platforms though > since the irq bits are different there. But at least bxt has a FIXME > comment that suggest we do need to save the sticky bits on those platforms > too. > > If we can fix this up for all platforms then I think a subsequent patch > could try to re-enable the hpd checks in the hdmi ->detect function and > make use spec compliant. Then after maybe 1-2 kernel releases of testing > we'll know whether it really works. > > But I'd really want to enable this everywhere just to have maximal test > coverage - we did have reports on all platforms so it seems a generic > issue. > I think only cpt/ibx suffer from this. Bxt already set port_hotplug_stat only when hpd occurs. Is there a process to get this checked on all platforms, which the fix I suggested? I can only test on SKL as of now. Regards, Sonika > Thanks, Daniel > >> >> Thanks, >> Sonika >> >> drivers/gpu/drm/i915/i915_irq.c | 11 ++++++----- >> 1 file changed, 6 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c >> index a6fbe64..2d47372 100644 >> --- a/drivers/gpu/drm/i915/i915_irq.c >> +++ b/drivers/gpu/drm/i915/i915_irq.c >> @@ -1760,11 +1760,12 @@ static void cpt_irq_handler(struct drm_device *dev, u32 pch_iir) >> u32 dig_hotplug_reg; >> u32 pin_mask, long_mask; >> >> - dig_hotplug_reg = I915_READ(PCH_PORT_HOTPLUG); >> - I915_WRITE(PCH_PORT_HOTPLUG, dig_hotplug_reg); >> - >> - pch_get_hpd_pins(&pin_mask, &long_mask, hotplug_trigger, dig_hotplug_reg, hpd_cpt); >> - intel_hpd_irq_handler(dev, pin_mask, long_mask); >> + if (hotplug_trigger) { >> + dig_hotplug_reg = I915_READ(PCH_PORT_HOTPLUG); >> + I915_WRITE(PCH_PORT_HOTPLUG, dig_hotplug_reg); >> + pch_get_hpd_pins(&pin_mask, &long_mask, hotplug_trigger, dig_hotplug_reg, hpd_cpt); >> + intel_hpd_irq_handler(dev, pin_mask, long_mask); >> + } >> >> if (pch_iir & SDE_AUDIO_POWER_MASK_CPT) { >> int port = ffs((pch_iir & SDE_AUDIO_POWER_MASK_CPT) >> >> -- >> 1.7.10.4 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx >
On Mon, Jul 06, 2015 at 11:23:53AM +0530, Sonika Jindal wrote: > Writing to PCH_PORT_HOTPLUG for each interrupt is not required. > Handle it only if hpd has actually occurred like we handle other > interrupts. > > Signed-off-by: Sonika Jindal <sonika.jindal@intel.com> > --- > Hi, > > I see we don't check for hotplug_trigger before processing the HPD for any of the platform. Is there any reason for this? > For SKL, if I let write to PCH_PORT_HOTPLUG happen for all interrupts, somehow this register gets an invalid value at one point and it zeroes it out. > If I put this check before handling HPD, hotplug behaves fine. > Please let me know if you see any issue with this approach. > > Thanks, > Sonika > > drivers/gpu/drm/i915/i915_irq.c | 11 ++++++----- > 1 file changed, 6 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > index a6fbe64..2d47372 100644 > --- a/drivers/gpu/drm/i915/i915_irq.c > +++ b/drivers/gpu/drm/i915/i915_irq.c > @@ -1760,11 +1760,12 @@ static void cpt_irq_handler(struct drm_device *dev, u32 pch_iir) > u32 dig_hotplug_reg; > u32 pin_mask, long_mask; > > - dig_hotplug_reg = I915_READ(PCH_PORT_HOTPLUG); > - I915_WRITE(PCH_PORT_HOTPLUG, dig_hotplug_reg); > - > - pch_get_hpd_pins(&pin_mask, &long_mask, hotplug_trigger, dig_hotplug_reg, hpd_cpt); > - intel_hpd_irq_handler(dev, pin_mask, long_mask); > + if (hotplug_trigger) { > + dig_hotplug_reg = I915_READ(PCH_PORT_HOTPLUG); > + I915_WRITE(PCH_PORT_HOTPLUG, dig_hotplug_reg); > + pch_get_hpd_pins(&pin_mask, &long_mask, hotplug_trigger, dig_hotplug_reg, hpd_cpt); > + intel_hpd_irq_handler(dev, pin_mask, long_mask); > + } Deja vu. I think I have the same patch (also for IBX) in my stalled (and never posted) port A HPD branch. So yeah, I think this makes sense. You can also move the dig_hotplug_reg, pin_mask, and long_mask declarations into the if block since they're not needed elsewhere. > > if (pch_iir & SDE_AUDIO_POWER_MASK_CPT) { > int port = ffs((pch_iir & SDE_AUDIO_POWER_MASK_CPT) >> > -- > 1.7.10.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On 7/6/2015 2:19 PM, Ville Syrjälä wrote: > On Mon, Jul 06, 2015 at 11:23:53AM +0530, Sonika Jindal wrote: >> Writing to PCH_PORT_HOTPLUG for each interrupt is not required. >> Handle it only if hpd has actually occurred like we handle other >> interrupts. >> >> Signed-off-by: Sonika Jindal <sonika.jindal@intel.com> >> --- >> Hi, >> >> I see we don't check for hotplug_trigger before processing the HPD for any of the platform. Is there any reason for this? >> For SKL, if I let write to PCH_PORT_HOTPLUG happen for all interrupts, somehow this register gets an invalid value at one point and it zeroes it out. >> If I put this check before handling HPD, hotplug behaves fine. >> Please let me know if you see any issue with this approach. >> >> Thanks, >> Sonika >> >> drivers/gpu/drm/i915/i915_irq.c | 11 ++++++----- >> 1 file changed, 6 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c >> index a6fbe64..2d47372 100644 >> --- a/drivers/gpu/drm/i915/i915_irq.c >> +++ b/drivers/gpu/drm/i915/i915_irq.c >> @@ -1760,11 +1760,12 @@ static void cpt_irq_handler(struct drm_device *dev, u32 pch_iir) >> u32 dig_hotplug_reg; >> u32 pin_mask, long_mask; >> >> - dig_hotplug_reg = I915_READ(PCH_PORT_HOTPLUG); >> - I915_WRITE(PCH_PORT_HOTPLUG, dig_hotplug_reg); >> - >> - pch_get_hpd_pins(&pin_mask, &long_mask, hotplug_trigger, dig_hotplug_reg, hpd_cpt); >> - intel_hpd_irq_handler(dev, pin_mask, long_mask); >> + if (hotplug_trigger) { >> + dig_hotplug_reg = I915_READ(PCH_PORT_HOTPLUG); >> + I915_WRITE(PCH_PORT_HOTPLUG, dig_hotplug_reg); >> + pch_get_hpd_pins(&pin_mask, &long_mask, hotplug_trigger, dig_hotplug_reg, hpd_cpt); >> + intel_hpd_irq_handler(dev, pin_mask, long_mask); >> + } > > Deja vu. I think I have the same patch (also for IBX) in my stalled > (and never posted) port A HPD branch. So yeah, I think this > makes sense. > > You can also move the dig_hotplug_reg, pin_mask, and long_mask declarations > into the if block since they're not needed elsewhere. Sure. Do you think I should add this for ibx as well? > > >> >> if (pch_iir & SDE_AUDIO_POWER_MASK_CPT) { >> int port = ffs((pch_iir & SDE_AUDIO_POWER_MASK_CPT) >> >> -- >> 1.7.10.4 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx >
Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6723
-------------------------------------Summary-------------------------------------
Platform Delta drm-intel-nightly Series Applied
ILK 302/302 302/302
SNB 312/316 312/316
IVB 343/343 343/343
BYT -2 287/287 285/287
HSW 380/380 380/380
-------------------------------------Detailed-------------------------------------
Platform Test drm-intel-nightly Series Applied
*BYT igt@gem_partial_pwrite_pread@reads PASS(1) FAIL(1)
*BYT igt@gem_tiled_partial_pwrite_pread@reads PASS(1) FAIL(1)
Note: You need to pay more attention to line start with '*'
On Mon, Jul 06, 2015 at 02:31:56PM +0530, Jindal, Sonika wrote: > > > On 7/6/2015 2:19 PM, Ville Syrjälä wrote: > > On Mon, Jul 06, 2015 at 11:23:53AM +0530, Sonika Jindal wrote: > >> Writing to PCH_PORT_HOTPLUG for each interrupt is not required. > >> Handle it only if hpd has actually occurred like we handle other > >> interrupts. > >> > >> Signed-off-by: Sonika Jindal <sonika.jindal@intel.com> > >> --- > >> Hi, > >> > >> I see we don't check for hotplug_trigger before processing the HPD for any of the platform. Is there any reason for this? > >> For SKL, if I let write to PCH_PORT_HOTPLUG happen for all interrupts, somehow this register gets an invalid value at one point and it zeroes it out. > >> If I put this check before handling HPD, hotplug behaves fine. > >> Please let me know if you see any issue with this approach. > >> > >> Thanks, > >> Sonika > >> > >> drivers/gpu/drm/i915/i915_irq.c | 11 ++++++----- > >> 1 file changed, 6 insertions(+), 5 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > >> index a6fbe64..2d47372 100644 > >> --- a/drivers/gpu/drm/i915/i915_irq.c > >> +++ b/drivers/gpu/drm/i915/i915_irq.c > >> @@ -1760,11 +1760,12 @@ static void cpt_irq_handler(struct drm_device *dev, u32 pch_iir) > >> u32 dig_hotplug_reg; > >> u32 pin_mask, long_mask; > >> > >> - dig_hotplug_reg = I915_READ(PCH_PORT_HOTPLUG); > >> - I915_WRITE(PCH_PORT_HOTPLUG, dig_hotplug_reg); > >> - > >> - pch_get_hpd_pins(&pin_mask, &long_mask, hotplug_trigger, dig_hotplug_reg, hpd_cpt); > >> - intel_hpd_irq_handler(dev, pin_mask, long_mask); > >> + if (hotplug_trigger) { > >> + dig_hotplug_reg = I915_READ(PCH_PORT_HOTPLUG); > >> + I915_WRITE(PCH_PORT_HOTPLUG, dig_hotplug_reg); > >> + pch_get_hpd_pins(&pin_mask, &long_mask, hotplug_trigger, dig_hotplug_reg, hpd_cpt); > >> + intel_hpd_irq_handler(dev, pin_mask, long_mask); > >> + } > > > > Deja vu. I think I have the same patch (also for IBX) in my stalled > > (and never posted) port A HPD branch. So yeah, I think this > > makes sense. > > > > You can also move the dig_hotplug_reg, pin_mask, and long_mask declarations > > into the if block since they're not needed elsewhere. > Sure. Do you think I should add this for ibx as well? Yes.
On Mon, Jul 06, 2015 at 02:11:12PM +0530, Jindal, Sonika wrote: > > > On 7/6/2015 2:06 PM, Daniel Vetter wrote: > >On Mon, Jul 06, 2015 at 11:23:53AM +0530, Sonika Jindal wrote: > >>Writing to PCH_PORT_HOTPLUG for each interrupt is not required. > >>Handle it only if hpd has actually occurred like we handle other > >>interrupts. > >> > >>Signed-off-by: Sonika Jindal <sonika.jindal@intel.com> > >>--- > >>Hi, > >> > >>I see we don't check for hotplug_trigger before processing the HPD for any of the platform. Is there any reason for this? > >>For SKL, if I let write to PCH_PORT_HOTPLUG happen for all interrupts, somehow this register gets an invalid value at one point and it zeroes it out. > >>If I put this check before handling HPD, hotplug behaves fine. > >>Please let me know if you see any issue with this approach. > > > >Nice find, this sounds really intrigueing, at least for cpt/ibx platforms. > >I'm not sure whether what will happen with atom/i9xx platforms though > >since the irq bits are different there. But at least bxt has a FIXME > >comment that suggest we do need to save the sticky bits on those platforms > >too. > > > >If we can fix this up for all platforms then I think a subsequent patch > >could try to re-enable the hpd checks in the hdmi ->detect function and > >make use spec compliant. Then after maybe 1-2 kernel releases of testing > >we'll know whether it really works. > > > >But I'd really want to enable this everywhere just to have maximal test > >coverage - we did have reports on all platforms so it seems a generic > >issue. > > > I think only cpt/ibx suffer from this. Bxt already set port_hotplug_stat > only when hpd occurs. > Is there a process to get this checked on all platforms, which the fix I > suggested? > I can only test on SKL as of now. Right I was looking at an older version of the bxt hpd handler which still had a FIXME. But we probably still have an issue on bxt/i9xx interrupt handlers since when we've done the original revert to stop looking at the hpd live status bits in commit 202adf4b9f5957b26a1cb97267d78e0edb319c5e Author: Daniel Vetter <daniel.vetter@ffwll.ch> Date: Fri Feb 22 00:53:04 2013 +0100 drm/i915: Revert hdmi HDP pin checks we had reports from both ibx/cpt systems and g4x. And vlv/chv/bxt hpd seems derived from that (with live status, enable and status all smashed into one register). Therefore I guess that we need a similar fix of only carefully clearing status bits on these platforms too. I have no idea what it is though, might be good to check how exactly windows is handling hpd on these atom platfroms. -Daniel
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index a6fbe64..2d47372 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -1760,11 +1760,12 @@ static void cpt_irq_handler(struct drm_device *dev, u32 pch_iir) u32 dig_hotplug_reg; u32 pin_mask, long_mask; - dig_hotplug_reg = I915_READ(PCH_PORT_HOTPLUG); - I915_WRITE(PCH_PORT_HOTPLUG, dig_hotplug_reg); - - pch_get_hpd_pins(&pin_mask, &long_mask, hotplug_trigger, dig_hotplug_reg, hpd_cpt); - intel_hpd_irq_handler(dev, pin_mask, long_mask); + if (hotplug_trigger) { + dig_hotplug_reg = I915_READ(PCH_PORT_HOTPLUG); + I915_WRITE(PCH_PORT_HOTPLUG, dig_hotplug_reg); + pch_get_hpd_pins(&pin_mask, &long_mask, hotplug_trigger, dig_hotplug_reg, hpd_cpt); + intel_hpd_irq_handler(dev, pin_mask, long_mask); + } if (pch_iir & SDE_AUDIO_POWER_MASK_CPT) { int port = ffs((pch_iir & SDE_AUDIO_POWER_MASK_CPT) >>
Writing to PCH_PORT_HOTPLUG for each interrupt is not required. Handle it only if hpd has actually occurred like we handle other interrupts. Signed-off-by: Sonika Jindal <sonika.jindal@intel.com> --- Hi, I see we don't check for hotplug_trigger before processing the HPD for any of the platform. Is there any reason for this? For SKL, if I let write to PCH_PORT_HOTPLUG happen for all interrupts, somehow this register gets an invalid value at one point and it zeroes it out. If I put this check before handling HPD, hotplug behaves fine. Please let me know if you see any issue with this approach. Thanks, Sonika drivers/gpu/drm/i915/i915_irq.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)