On Fri, Jun 28, 2024 at 09:21:24AM +0000, Manna, Animesh wrote:
>
>
> > -----Original Message-----
> > From: Intel-gfx <intel-gfx-bounces@xxxxxxxxxxxxxxxxxxxxx> On Behalf Of Ville
> > Syrjala
> > Sent: Tuesday, June 25, 2024 12:40 AM
> > To: intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > Subject: [PATCH 03/14] drm/i915/dsb: Hook up DSB error interrupts
> >
> > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> >
> > Enable all DSB error/fault interrupts so that we can see if anything goes
> > terribly wrong.
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > ---
> > .../gpu/drm/i915/display/intel_display_irq.c | 17 ++++++
> > drivers/gpu/drm/i915/display/intel_dsb.c | 58 +++++++++++++++++++
> > drivers/gpu/drm/i915/display/intel_dsb.h | 6 ++
> > drivers/gpu/drm/i915/i915_reg.h | 4 ++
> > 4 files changed, 85 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_irq.c
> > b/drivers/gpu/drm/i915/display/intel_display_irq.c
> > index 5219ba295c74..7169db984651 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_irq.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display_irq.c
> > @@ -14,6 +14,7 @@
> > #include "intel_display_trace.h"
> > #include "intel_display_types.h"
> > #include "intel_dp_aux.h"
> > +#include "intel_dsb.h"
> > #include "intel_fdi_regs.h"
> > #include "intel_fifo_underrun.h"
> > #include "intel_gmbus.h"
> > @@ -1143,6 +1144,17 @@ void gen8_de_irq_handler(struct
> > drm_i915_private *dev_priv, u32 master_ctl)
> >
> > intel_uncore_write(&dev_priv->uncore,
> > GEN8_DE_PIPE_IIR(pipe), iir);
> >
> > + if (HAS_DSB(dev_priv)) {
> > + if (iir & GEN12_DSB_INT(INTEL_DSB_0))
> > + intel_dsb_irq_handler(&dev_priv->display,
> > pipe, INTEL_DSB_0);
> > +
> > + if (iir & GEN12_DSB_INT(INTEL_DSB_1))
> > + intel_dsb_irq_handler(&dev_priv->display,
> > pipe, INTEL_DSB_1);
> > +
> > + if (iir & GEN12_DSB_INT(INTEL_DSB_2))
> > + intel_dsb_irq_handler(&dev_priv->display,
> > pipe, INTEL_DSB_2);
> > + }
> > +
> > if (iir & GEN8_PIPE_VBLANK)
> > intel_handle_vblank(dev_priv, pipe);
> >
> > @@ -1718,6 +1730,11 @@ void gen8_de_irq_postinstall(struct
> > drm_i915_private *dev_priv)
> > de_port_masked |= DSI0_TE | DSI1_TE;
> > }
> >
> > + if (HAS_DSB(dev_priv))
> > + de_pipe_masked |= GEN12_DSB_INT(INTEL_DSB_0) |
> > + GEN12_DSB_INT(INTEL_DSB_1) |
> > + GEN12_DSB_INT(INTEL_DSB_2);
> > +
> > de_pipe_enables = de_pipe_masked |
> > GEN8_PIPE_VBLANK |
> > gen8_de_pipe_underrun_mask(dev_priv) | diff --git
> > a/drivers/gpu/drm/i915/display/intel_dsb.c
> > b/drivers/gpu/drm/i915/display/intel_dsb.c
> > index 2ab3765f6c06..ded696363258 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dsb.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dsb.c
> > @@ -339,6 +339,42 @@ static u32 dsb_chicken(struct intel_crtc *crtc)
> > return DSB_SKIP_WAITS_EN;
> > }
> >
> > +static u32 dsb_error_int_status(struct intel_display *display) {
> > + struct drm_i915_private *i915 = to_i915(display->drm);
> > + u32 errors;
> > +
> > + errors = DSB_GTT_FAULT_INT_STATUS |
> > + DSB_RSPTIMEOUT_INT_STATUS |
> > + DSB_POLL_ERR_INT_STATUS;
> > +
> > + /*
> > + * All the non-existing status bits operate as
> > + * normal r/w bits, so any attempt to clear them
> > + * will just end up setting them. Never do that so
> > + * we won't mistake them for actual error interrupts.
> > + */
> > + if (DISPLAY_VER(i915) >= 14)
> > + errors |= DSB_ATS_FAULT_INT_STATUS;
> > +
> > + return errors;
> > +}
> > +
> > +static u32 dsb_error_int_en(struct intel_display *display) {
> > + struct drm_i915_private *i915 = to_i915(display->drm);
> > + u32 errors;
> > +
> > + errors = DSB_GTT_FAULT_INT_EN |
> > + DSB_RSPTIMEOUT_INT_EN |
> > + DSB_POLL_ERR_INT_EN;
> > +
> > + if (DISPLAY_VER(i915) >= 14)
> > + errors |= DSB_ATS_FAULT_INT_EN;
> > +
> > + return errors;
> > +}
> > +
> > static void _intel_dsb_commit(struct intel_dsb *dsb, u32 ctrl,
> > int dewake_scanline)
> > {
> > @@ -363,6 +399,10 @@ static void _intel_dsb_commit(struct intel_dsb *dsb,
> > u32 ctrl,
> > intel_de_write_fw(display, DSB_CHICKEN(pipe, dsb->id),
> > dsb_chicken(crtc));
> >
> > + intel_de_write_fw(display, DSB_INTERRUPT(pipe, dsb->id),
> > + dsb_error_int_status(display) |
> > DSB_PROG_INT_STATUS |
> > + dsb_error_int_en(display));
> > +
> > intel_de_write_fw(display, DSB_HEAD(pipe, dsb->id),
> > intel_dsb_buffer_ggtt_offset(&dsb->dsb_buf));
> >
> > @@ -430,6 +470,9 @@ void intel_dsb_wait(struct intel_dsb *dsb)
> > dsb->free_pos = 0;
> > dsb->ins_start_offset = 0;
> > intel_de_write_fw(display, DSB_CTRL(pipe, dsb->id), 0);
> > +
> > + intel_de_write_fw(display, DSB_INTERRUPT(pipe, dsb->id),
> > + dsb_error_int_status(display) |
> > DSB_PROG_INT_STATUS);
> > }
> >
> > /**
> > @@ -513,3 +556,18 @@ void intel_dsb_cleanup(struct intel_dsb *dsb)
> > intel_dsb_buffer_cleanup(&dsb->dsb_buf);
> > kfree(dsb);
> > }
> > +
> > +void intel_dsb_irq_handler(struct intel_display *display,
> > + enum pipe pipe, enum intel_dsb_id dsb_id) {
> > + struct intel_crtc *crtc = intel_crtc_for_pipe(to_i915(display->drm),
> > pipe);
> > + u32 tmp, errors;
> > +
> > + tmp = intel_de_read_fw(display, DSB_INTERRUPT(pipe, dsb_id));
> > + intel_de_write_fw(display, DSB_INTERRUPT(pipe, dsb_id), tmp);
> > +
> > + errors = tmp & dsb_error_int_status(display);
> > + if (errors)
> > + drm_err(display->drm, "[CRTC:%d:%s] / DSB %d error
> > interrupt: 0x%x\n",
> > + crtc->base.base.id, crtc->base.name, dsb_id, errors);
>
> Just a nitpick: maybe good to print the errors in a string format instead of hex value.
Yeah that might be nice. But we don't do that for eg. pipe fault
errors either. Perhaps a nice little task for someone to go over
all of these and make them a bit more human friendly...
--
Ville Syrjälä
Intel
