| Message ID | 1396377447-2177-20-git-send-email-przanoni@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Apr 01, 2014 at 03:37:27PM -0300, Paulo Zanoni wrote: > From: Paulo Zanoni <paulo.r.zanoni@intel.com> > > We should only enable interrupts at postinstall. > > And now on ILK/SNB/IVB/HSW the irq_preinstall and irq_postinstall > functions leave the hardware in the same state. > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > --- > drivers/gpu/drm/i915/i915_irq.c | 10 ++++------ > 1 file changed, 4 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > index fd1ef09..75f1997 100644 > --- a/drivers/gpu/drm/i915/i915_irq.c > +++ b/drivers/gpu/drm/i915/i915_irq.c > @@ -2912,6 +2912,8 @@ static void ironlake_irq_reset(struct drm_device *dev) > { > struct drm_i915_private *dev_priv = dev->dev_private; > > + I915_WRITE(HWSTAM, 0xffffffff); > + > GEN5_IRQ_RESET(DE); > if (IS_GEN7(dev)) > I915_WRITE(GEN7_ERR_INT, 0xffffffff); > @@ -2923,10 +2925,6 @@ static void ironlake_irq_reset(struct drm_device *dev) > > static void ironlake_irq_preinstall(struct drm_device *dev) > { > - drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private; > - > - I915_WRITE(HWSTAM, 0xeffe); > - > ironlake_irq_reset(dev); > } Follow-up patch to remove this wrapper functions and just put foo_irq_reset into the preinstall hooks? In any case entire series merged (with one tiny rebase conflict since drm_i915_private_t is now gone). -Daniel > > @@ -3099,6 +3097,8 @@ static int ironlake_irq_postinstall(struct drm_device *dev) > > dev_priv->irq_mask = ~display_mask; > > + I915_WRITE(HWSTAM, 0xeffe); > + > ibx_irq_pre_postinstall(dev); > > GEN5_IRQ_INIT(DE, dev_priv->irq_mask, display_mask | extra_mask); > @@ -3354,8 +3354,6 @@ static void ironlake_irq_uninstall(struct drm_device *dev) > > intel_hpd_irq_uninstall(dev_priv); > > - I915_WRITE(HWSTAM, 0xffffffff); > - > ironlake_irq_reset(dev); > } > > -- > 1.8.5.3 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, Apr 01, 2014 at 03:37:27PM -0300, Paulo Zanoni wrote: > From: Paulo Zanoni <paulo.r.zanoni@intel.com> > > We should only enable interrupts at postinstall. > > And now on ILK/SNB/IVB/HSW the irq_preinstall and irq_postinstall > functions leave the hardware in the same state. > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> Orthogonal note to this patch, I'm wondering why we enable any interrupt reporting in the HW status page, I don't see anywhere we read that information back? Any idea?
On Wed, Apr 02, 2014 at 12:21:45PM +0100, Damien Lespiau wrote: > On Tue, Apr 01, 2014 at 03:37:27PM -0300, Paulo Zanoni wrote: > > From: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > > We should only enable interrupts at postinstall. > > > > And now on ILK/SNB/IVB/HSW the irq_preinstall and irq_postinstall > > functions leave the hardware in the same state. > > > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > Orthogonal note to this patch, I'm wondering why we enable any interrupt > reporting in the HW status page, I don't see anywhere we read that > information back? Any idea? Back in the SNB days, interrupts broke without this piece of magic. -Chris
On Wed, Apr 02, 2014 at 12:23:51PM +0100, Chris Wilson wrote: > On Wed, Apr 02, 2014 at 12:21:45PM +0100, Damien Lespiau wrote: > > On Tue, Apr 01, 2014 at 03:37:27PM -0300, Paulo Zanoni wrote: > > > From: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > > > > We should only enable interrupts at postinstall. > > > > > > And now on ILK/SNB/IVB/HSW the irq_preinstall and irq_postinstall > > > functions leave the hardware in the same state. > > > > > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > > Orthogonal note to this patch, I'm wondering why we enable any interrupt > > reporting in the HW status page, I don't see anywhere we read that > > information back? Any idea? > > Back in the SNB days, interrupts broke without this piece of magic. To be more precise, iirc, it was a step towards getting coherent breadcrumb writes into the HWS. As it turns out, there were more steps required. Considering that it can now probably be dropped again? Any takers? -Chris
2014-04-02 8:27 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>: > On Wed, Apr 02, 2014 at 12:23:51PM +0100, Chris Wilson wrote: >> On Wed, Apr 02, 2014 at 12:21:45PM +0100, Damien Lespiau wrote: >> > On Tue, Apr 01, 2014 at 03:37:27PM -0300, Paulo Zanoni wrote: >> > > From: Paulo Zanoni <paulo.r.zanoni@intel.com> >> > > >> > > We should only enable interrupts at postinstall. >> > > >> > > And now on ILK/SNB/IVB/HSW the irq_preinstall and irq_postinstall >> > > functions leave the hardware in the same state. >> > > >> > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> >> > >> > Orthogonal note to this patch, I'm wondering why we enable any interrupt >> > reporting in the HW status page, I don't see anywhere we read that >> > information back? Any idea? >> >> Back in the SNB days, interrupts broke without this piece of magic. > > To be more precise, iirc, it was a step towards getting coherent > breadcrumb writes into the HWS. As it turns out, there were more steps > required. Considering that it can now probably be dropped again? Any > takers? I noticed the HWSTAM code looks weird, but I don't really know much about why it's there, so I decided to not add any regressions. I wrote this patch a long time ago, and gave up on upstreaming it, but in case anybody wants, I can send it to the list: http://cgit.freedesktop.org/~pzanoni/linux/commit/?h=c8-wip&id=7ebe245a2c55379ddc7b36f1fb440215c23f1570 . Look at the commit message, it may be an interesting starting point if anybody wants to dig on the issue... > -Chris > > -- > Chris Wilson, Intel Open Source Technology Centre
On Wed, Apr 02, 2014 at 11:14:58AM -0300, Paulo Zanoni wrote: > 2014-04-02 8:27 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>: > > On Wed, Apr 02, 2014 at 12:23:51PM +0100, Chris Wilson wrote: > >> On Wed, Apr 02, 2014 at 12:21:45PM +0100, Damien Lespiau wrote: > >> > On Tue, Apr 01, 2014 at 03:37:27PM -0300, Paulo Zanoni wrote: > >> > > From: Paulo Zanoni <paulo.r.zanoni@intel.com> > >> > > > >> > > We should only enable interrupts at postinstall. > >> > > > >> > > And now on ILK/SNB/IVB/HSW the irq_preinstall and irq_postinstall > >> > > functions leave the hardware in the same state. > >> > > > >> > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > >> > > >> > Orthogonal note to this patch, I'm wondering why we enable any interrupt > >> > reporting in the HW status page, I don't see anywhere we read that > >> > information back? Any idea? > >> > >> Back in the SNB days, interrupts broke without this piece of magic. > > > > To be more precise, iirc, it was a step towards getting coherent > > breadcrumb writes into the HWS. As it turns out, there were more steps > > required. Considering that it can now probably be dropped again? Any > > takers? > > I noticed the HWSTAM code looks weird, but I don't really know much > about why it's there, so I decided to not add any regressions. I wrote > this patch a long time ago, and gave up on upstreaming it, but in case > anybody wants, I can send it to the list: > http://cgit.freedesktop.org/~pzanoni/linux/commit/?h=c8-wip&id=7ebe245a2c55379ddc7b36f1fb440215c23f1570 > . Look at the commit message, it may be an interesting starting point > if anybody wants to dig on the issue... Yup, that is very interesting. That may be the minimal piece of HWSTAM magic we need, and be an interim step to removing HWSTAM entirely. -Chris
On Wed, Apr 02, 2014 at 04:28:06PM +0100, Chris Wilson wrote: > On Wed, Apr 02, 2014 at 11:14:58AM -0300, Paulo Zanoni wrote: > > 2014-04-02 8:27 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>: > > > On Wed, Apr 02, 2014 at 12:23:51PM +0100, Chris Wilson wrote: > > >> On Wed, Apr 02, 2014 at 12:21:45PM +0100, Damien Lespiau wrote: > > >> > On Tue, Apr 01, 2014 at 03:37:27PM -0300, Paulo Zanoni wrote: > > >> > > From: Paulo Zanoni <paulo.r.zanoni@intel.com> > > >> > > > > >> > > We should only enable interrupts at postinstall. > > >> > > > > >> > > And now on ILK/SNB/IVB/HSW the irq_preinstall and irq_postinstall > > >> > > functions leave the hardware in the same state. > > >> > > > > >> > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > >> > > > >> > Orthogonal note to this patch, I'm wondering why we enable any interrupt > > >> > reporting in the HW status page, I don't see anywhere we read that > > >> > information back? Any idea? > > >> > > >> Back in the SNB days, interrupts broke without this piece of magic. > > > > > > To be more precise, iirc, it was a step towards getting coherent > > > breadcrumb writes into the HWS. As it turns out, there were more steps > > > required. Considering that it can now probably be dropped again? Any > > > takers? > > > > I noticed the HWSTAM code looks weird, but I don't really know much > > about why it's there, so I decided to not add any regressions. I wrote > > this patch a long time ago, and gave up on upstreaming it, but in case > > anybody wants, I can send it to the list: > > http://cgit.freedesktop.org/~pzanoni/linux/commit/?h=c8-wip&id=7ebe245a2c55379ddc7b36f1fb440215c23f1570 > > . Look at the commit message, it may be an interesting starting point > > if anybody wants to dig on the issue... > > Yup, that is very interesting. That may be the minimal piece of HWSTAM > magic we need, and be an interim step to removing HWSTAM entirely. Would you mind resending: - a patch with Chris' comment above, that's alone is useful for the next guy looking at the code, - a patch with the change (Maybe reusing GT_RENDER_USER_INTERRUPT as HWSTAM uses the same definitions as the other interrupts registers)? I'm sure we can get some reviews on the first, hopefully some testing on the second.
On Wed, Apr 02, 2014 at 05:08:34PM +0100, Damien Lespiau wrote: > On Wed, Apr 02, 2014 at 04:28:06PM +0100, Chris Wilson wrote: > > On Wed, Apr 02, 2014 at 11:14:58AM -0300, Paulo Zanoni wrote: > > > 2014-04-02 8:27 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>: > > > > On Wed, Apr 02, 2014 at 12:23:51PM +0100, Chris Wilson wrote: > > > >> On Wed, Apr 02, 2014 at 12:21:45PM +0100, Damien Lespiau wrote: > > > >> > On Tue, Apr 01, 2014 at 03:37:27PM -0300, Paulo Zanoni wrote: > > > >> > > From: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > >> > > > > > >> > > We should only enable interrupts at postinstall. > > > >> > > > > > >> > > And now on ILK/SNB/IVB/HSW the irq_preinstall and irq_postinstall > > > >> > > functions leave the hardware in the same state. > > > >> > > > > > >> > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > >> > > > > >> > Orthogonal note to this patch, I'm wondering why we enable any interrupt > > > >> > reporting in the HW status page, I don't see anywhere we read that > > > >> > information back? Any idea? > > > >> > > > >> Back in the SNB days, interrupts broke without this piece of magic. > > > > > > > > To be more precise, iirc, it was a step towards getting coherent > > > > breadcrumb writes into the HWS. As it turns out, there were more steps > > > > required. Considering that it can now probably be dropped again? Any > > > > takers? > > > > > > I noticed the HWSTAM code looks weird, but I don't really know much > > > about why it's there, so I decided to not add any regressions. I wrote > > > this patch a long time ago, and gave up on upstreaming it, but in case > > > anybody wants, I can send it to the list: > > > http://cgit.freedesktop.org/~pzanoni/linux/commit/?h=c8-wip&id=7ebe245a2c55379ddc7b36f1fb440215c23f1570 > > > . Look at the commit message, it may be an interesting starting point > > > if anybody wants to dig on the issue... > > > > Yup, that is very interesting. That may be the minimal piece of HWSTAM > > magic we need, and be an interim step to removing HWSTAM entirely. > > Would you mind resending: > - a patch with Chris' comment above, that's alone is useful for the > next guy looking at the code, > - a patch with the change (Maybe reusing GT_RENDER_USER_INTERRUPT as > HWSTAM uses the same definitions as the other interrupts registers)? > > I'm sure we can get some reviews on the first, hopefully some testing on > the second. Seconded. Documenting what we're doing is always good ;-) -Daniel
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index fd1ef09..75f1997 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -2912,6 +2912,8 @@ static void ironlake_irq_reset(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; + I915_WRITE(HWSTAM, 0xffffffff); + GEN5_IRQ_RESET(DE); if (IS_GEN7(dev)) I915_WRITE(GEN7_ERR_INT, 0xffffffff); @@ -2923,10 +2925,6 @@ static void ironlake_irq_reset(struct drm_device *dev) static void ironlake_irq_preinstall(struct drm_device *dev) { - drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private; - - I915_WRITE(HWSTAM, 0xeffe); - ironlake_irq_reset(dev); } @@ -3099,6 +3097,8 @@ static int ironlake_irq_postinstall(struct drm_device *dev) dev_priv->irq_mask = ~display_mask; + I915_WRITE(HWSTAM, 0xeffe); + ibx_irq_pre_postinstall(dev); GEN5_IRQ_INIT(DE, dev_priv->irq_mask, display_mask | extra_mask); @@ -3354,8 +3354,6 @@ static void ironlake_irq_uninstall(struct drm_device *dev) intel_hpd_irq_uninstall(dev_priv); - I915_WRITE(HWSTAM, 0xffffffff); - ironlake_irq_reset(dev); }