Re: [PATCH v2 08/10] drm/nouveau/gsp: fix vblank interrupts on GB20x

[Mohamed Ahmed]

On Sat, Aug 22, 2026 at 2:12 AM <lyude@xxxxxxxxxx> wrote:
>
> Some comments regarding both patch #2 and this patch below:
>
>
> On Thu, 2026-08-20 at 20:49 +0400, Mohamed Ahmed wrote:
> > The GSP path programs per-head timing (vblank) interrupts the same
> > way on
> > every generation. NVD5.0 (GB20x) reworked the FE interrupt frontend
> > around four message-based kernel vectors (high latency, low latency,
> > PMU,
> > and GSP) and moved RM head-timing interrupts to the dedicated low-
> > latency
> > vector:
> >
> >  - The enable is NV_PDISP_FE_RM_INTR_EN1_HEAD_TIMING, 0x611ef0 +
> >    head*4 (570.144 kernel_head_0501.c, renamed kernel_head_0502.c
> > from
> >    575.51.02 on, and v05_01 dev_disp.h).
> >
> >  - The vector is reported as a separate interrupt table entry,
> >    MC_ENGINE_IDX_DISP_LOW (intr_gb202.c, intrCacheDispIntrVectors).
> >
> >  - The vector must be re-armed through NV_PDISP_FE_INTR_RETRIGGER(1)
> >    at 0x611f34 after servicing (kdispServiceInterrupt ->
> >    kdispIntrRetrigger_v05_01).
> >
> > The event latch (0x611800), per-head status (0x611c00), and dispatch
> > summary (0x611ec0) the interrupt handler uses are unchanged on GB20x
> > (kheadReadPendingVblank_v03_00 and kheadResetPendingLastData_v03_00
> > remain for DISPv0502+).
> >
> > On GB20x the old code enables head timing onto the legacy vector,
> > leaves
> > its handler there, and never re-arms the message-based vectors. Page
> > flips still complete (nv50 sends those events from the commit path),
> > so
> > the desktop looks fine while DRM vblank waits and vblank sequence
> > queries
> > are affected.
> >
> > Supply GB20x vblank enables and an interrupt handler that re-arms the
> > vector after servicing through gb202_gsp_disp, translate the low-
> > latency
> > interrupt table entry as a second NVKM_ENGINE_DISP instance, and flag
> > the table so r535_disp_oneinit() attaches the handler to that
> > instance.
> >
> > Signed-off-by: Mohamed Ahmed <mohamedahmedegypt2001@xxxxxxxxx>
> > ---
> >  .../gpu/drm/nouveau/nvkm/engine/disp/gb202.c  | 42
> > +++++++++++++++++--
> >  .../nouveau/nvkm/subdev/gsp/rm/r535/disp.c    |  5 ++-
> >  .../drm/nouveau/nvkm/subdev/gsp/rm/r570/gsp.c |  9 ++++
> >  3 files changed, 52 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/nouveau/nvkm/engine/disp/gb202.c
> > b/drivers/gpu/drm/nouveau/nvkm/engine/disp/gb202.c
> > index a66c820be9fe..f78669bafd64 100644
> > --- a/drivers/gpu/drm/nouveau/nvkm/engine/disp/gb202.c
> > +++ b/drivers/gpu/drm/nouveau/nvkm/engine/disp/gb202.c
> > @@ -130,6 +130,40 @@ gb202_head_state(struct nvkm_head *head, struct
> > nvkm_head_state *state)
> >       }
> >  }
> >
> > +/* NVD5.0 (GB20x and later) moved the RM head-timing interrupt
> > enable to
> > + * the low-latency vector's EN1 block. The event latch is unchanged.
> > + */
> > +static void
> > +gb202_head_vblank_put(struct nvkm_head *head)
> > +{
> > +     struct nvkm_device *device = head->disp-
> > >engine.subdev.device;
> > +
> > +     nvkm_mask(device, 0x611ef0 + (head->id * 4), 0x00000002,
> > 0x00000000);
> > +}
> > +
> > +static void
> > +gb202_head_vblank_get(struct nvkm_head *head)
> > +{
> > +     struct nvkm_device *device = head->disp-
> > >engine.subdev.device;
> > +
> > +     nvkm_wr32(device, 0x611800 + (head->id * 4), 0x00000002);
> > +     nvkm_mask(device, 0x611ef0 + (head->id * 4), 0x00000002,
> > 0x00000002);
> > +}
> > +
> > +static irqreturn_t
> > +gb202_disp_intr(struct nvkm_inth *inth)
> > +{
> > +     struct nvkm_disp *disp = container_of(inth, typeof(*disp),
> > engine.subdev.inth);
> > +     irqreturn_t ret = tu102_disp_intr(inth);
> > +
> > +     /* The FE interrupt vectors are message-based on NVD5.0. Re-
> > arm the
> > +      * low-latency vector so it fires again for any event that
> > latched
> > +      * while we were servicing.
> > +      */
> > +     nvkm_wr32(disp->engine.subdev.device, 0x611f34, 0x00000001);
> > +     return ret;
> > +}
> > +
> >  /* GB20x is GSP-only. This table supplies the register programming
> > the
> >   * GSP-RM display path needs from the chip.
> >   */
> > @@ -137,11 +171,13 @@ static const struct nvkm_disp_func
> >  gb202_gsp_disp = {
> >       .uevent = &gv100_disp_chan_uevent,
> >       .ramht_size = 0x2000,
> > -     .gsp.intr = tu102_disp_intr,
> > +     /* Head timing arrives on the dedicated low-latency vector.
> > */
> > +     .gsp.intr = gb202_disp_intr,
> > +     .gsp.intr_low_latency = true,
> >       .gsp.head_state = gb202_head_state,
> >       .gsp.head_rgpos = gv100_head_rgpos,
> > -     .gsp.vblank_get = tu102_head_vblank_get,
> > -     .gsp.vblank_put = tu102_head_vblank_put,
> > +     .gsp.vblank_get = gb202_head_vblank_get,
> > +     .gsp.vblank_put = gb202_head_vblank_put,
> >       .gsp.hdmi_gcp = gb202_sor_hdmi_gcp,
> >       /* The legacy AVI unit is unchanged on GB20x. */
> >       .gsp.hdmi_infoframe_avi = gv100_sor_hdmi_infoframe_avi,
> > diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/disp.c
> > b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/disp.c
> > index 3a8ff621ed62..a95f78c4502f 100644
> > --- a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/disp.c
> > +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/disp.c
> > @@ -1705,7 +1705,10 @@ r535_disp_oneinit(struct nvkm_disp *disp)
> >
> >       /* Chips that raise head-timing interrupts on a separate
> > low-latency
> >        * vector report it as a second DISP interrupt table entry,
> > exposed
> > -      * as instance 1 by the RM engine-index translation.
> > +      * as instance 1 by the RM engine-index translation (see
> > +      * r570_gsp_xlat_mc_engine_idx()). Their high-latency vector
> > +      * (instance 0) is left unhandled as no event nouveau
> > enables is
> > +      * routed to it, and without a handler it stays masked.
> >        */
>
> I didn't notice it until I got to this patch, but is it possible you
> mistakenly added the intr_low_latency stuff a little early with patch
> #2 and meant to add it here?
>
> (doesn't matter to me too much either way, whatever you intended works
> fine with me)
>
Yeah it was a rebase mistake and then I figured I could salvage it as
a bit of future-proofing in #2. I can try to fix it properly though.
> Otherwise:
>
> Reviewed-by: Lyude Paul <lyude@xxxxxxxxxx>
>
> >       ret = nvkm_gsp_intr_stall(gsp, disp->engine.subdev.type,
> >                                 disp->func->gsp.intr_low_latency ?
> > 1 : disp->engine.subdev.inst);
> > diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r570/gsp.c
> > b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r570/gsp.c
> > index 3e391646d8f7..b45781cd0dfd 100644
> > --- a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r570/gsp.c
> > +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r570/gsp.c
> > @@ -44,6 +44,15 @@ r570_gsp_xlat_mc_engine_idx(u32 mc_engine_idx,
> > enum nvkm_subdev_type *ptype, int
> >               *ptype = NVKM_ENGINE_DISP;
> >               *pinst = 0;
> >               return true;
> > +     case MC_ENGINE_IDX_DISP_LOW:
> > +             /* GB20x+ report a separate low-latency display
> > vector, used
> > +              * for head-timing interrupts. Expose it as a second
> > DISP
> > +              * interrupt instance. r535_disp_oneinit() attaches
> > the
> > +              * handler to it when the chip's
> > gsp.intr_low_latency is set.
> > +              */
> > +             *ptype = NVKM_ENGINE_DISP;
> > +             *pinst = 1;
> > +             return true;
> >       case MC_ENGINE_IDX_CE0 ... MC_ENGINE_IDX_CE19:
> >               *ptype = NVKM_ENGINE_CE;
> >               *pinst = mc_engine_idx - MC_ENGINE_IDX_CE0;
>