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

[lyude@redhat.com]

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@gmail.com>
> ---
>  .../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)

Otherwise:

Reviewed-by: Lyude Paul <lyude@redhat.com>

>  	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;