Re: [PATCH v2 03/10] drm/nouveau/disp: route GSP-RM display MMIO through nvkm_disp_func hooks

[lyude]

Some comments below:

On Thu, 2026-08-20 at 20:49 +0400, Mohamed Ahmed wrote:
> The GSP-RM display code in rm/r535/disp.c borrows a few
> register-programming routines from engine/disp (the head-timing
> interrupt handler, vblank enables, armed head state and scanout
> position
> readback, the AVI/VSI infoframe writers and the GCP AVMute write) and
> so
> far picked them by name, which means it has to know which chip it
> runs
> on the moment a generation changes any of them.
> 
> Give nvkm_disp_func a .gsp table that each chip fills with exactly
> those
> hooks, add tu102_gsp_disp (TU1xx) and ga102_gsp_disp (GA10x onwards)
> carrying the current functions, hand them to r535_disp_new() instead
> of
> the full hardware tables, and make rm/r535/disp.c call through the
> hooks. r535_head becomes four forwarders, r535_sor_hdmi gets
> infoframe
> forwarders, r535_sor_hdmi_audio() calls the GCP hook, and the
> interrupt
> handler and its vector come from the table (intr_low_latency selects
> the
> second DISP interrupt instance for chips that raise head timing on a
> separate vector). The tables are per chip even though the two
> currently
> coincide, so a generation that changes a hook only touches its own
> file.
> rm/r535/disp.c no longer contains chip-specific register code, and a
> new
> display generation only has to provide its own table. No functional
> change.
> 
> Signed-off-by: Mohamed Ahmed <mohamedahmedegypt2001@xxxxxxxxx>
> ---
>  .../gpu/drm/nouveau/nvkm/engine/disp/ga102.c  | 16 ++++-
>  .../gpu/drm/nouveau/nvkm/engine/disp/priv.h   | 19 ++++++
>  .../gpu/drm/nouveau/nvkm/engine/disp/tu102.c  | 16 ++++-
>  .../nouveau/nvkm/subdev/gsp/rm/r535/disp.c    | 60 ++++++++++++++++-
> --
>  4 files changed, 100 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/nouveau/nvkm/engine/disp/ga102.c
> b/drivers/gpu/drm/nouveau/nvkm/engine/disp/ga102.c
> index ab0a85c92430..b48ed7146396 100644
> --- a/drivers/gpu/drm/nouveau/nvkm/engine/disp/ga102.c
> +++ b/drivers/gpu/drm/nouveau/nvkm/engine/disp/ga102.c
> @@ -144,12 +144,26 @@ ga102_disp = {
>  	},
>  };
>  
> +static const struct nvkm_disp_func
> +ga102_gsp_disp = {
> +	.uevent = &gv100_disp_chan_uevent,
> +	.ramht_size = 0x2000,
> +	.gsp.intr = tu102_disp_intr,
> +	.gsp.head_state = gv100_head_state,
> +	.gsp.head_rgpos = gv100_head_rgpos,
> +	.gsp.vblank_get = tu102_head_vblank_get,
> +	.gsp.vblank_put = tu102_head_vblank_put,
> +	.gsp.hdmi_gcp = tu102_sor_hdmi_gcp,
> +	.gsp.hdmi_infoframe_avi = gv100_sor_hdmi_infoframe_avi,
> +	.gsp.hdmi_infoframe_vsi = gv100_sor_hdmi_infoframe_vsi,
> +};
> +
>  int
>  ga102_disp_new(struct nvkm_device *device, enum nvkm_subdev_type
> type, int inst,
>  	       struct nvkm_disp **pdisp)
>  {
>  	if (nvkm_gsp_rm(device->gsp))
> -		return r535_disp_new(&ga102_disp, device, type,
> inst, pdisp);
> +		return r535_disp_new(&ga102_gsp_disp, device, type,
> inst, pdisp);
>  
>  	return nvkm_disp_new_(&ga102_disp, device, type, inst,
> pdisp);
>  }
> diff --git a/drivers/gpu/drm/nouveau/nvkm/engine/disp/priv.h
> b/drivers/gpu/drm/nouveau/nvkm/engine/disp/priv.h
> index 722ec340e12a..3cb903741fb8 100644
> --- a/drivers/gpu/drm/nouveau/nvkm/engine/disp/priv.h
> +++ b/drivers/gpu/drm/nouveau/nvkm/engine/disp/priv.h
> @@ -5,6 +5,8 @@
>  #include <engine/disp.h>
>  #include <core/enum.h>
>  struct nvkm_head;
> +struct nvkm_head_state;
> +struct nvkm_ior;
>  struct nvkm_outp;
>  struct dcb_output;
>  
> @@ -34,6 +36,23 @@ struct nvkm_disp_func {
>  		int (*new)(struct nvkm_disp *, int id);
>  	} wndw, head, dac, sor, pior;
>  
> +	/* Register programming that the GSP-RM display path
> (rm/r535) needs from
> +	 * the chip, everything else on that path goes through RM.
> Every hook
> +	 * is called unconditionally.
> +	 */
> +	struct {
> +		irqreturn_t (*intr)(struct nvkm_inth *);
> +		/* Head-timing interrupts arrive on a second DISP
> vector. */
> +		bool intr_low_latency;
> +		void (*head_state)(struct nvkm_head *, struct
> nvkm_head_state *);
> +		void (*head_rgpos)(struct nvkm_head *, u16 *hline,
> u16 *vline);

This looks mostly fine. As far as I can tell though, it seems like
there's no actual behavioral differences between the gsp's head_state
and the non-GSP head_state, same for head_rgpos. Is it possible for us
to drop these two callbacks and keep using nvkm_head_func for that?
Perhaps by having a second nvkm_head_func that we call back down to
from RM's?

FWIW by the way, I think if we end up with say - a nvkm_head_func
struct that only has head_state/head_rgpos filled and nothing else
(e.g. using it without GSP would break things) that's probably fine
since booting these cards without GSP isn't possible on nouveau anyhow.

> +		void (*vblank_get)(struct nvkm_head *);
> +		void (*vblank_put)(struct nvkm_head *);
> +		void (*hdmi_gcp)(struct nvkm_ior *, int head, bool
> enable);
> +		void (*hdmi_infoframe_avi)(struct nvkm_ior *, int
> head, void *data, u32 size);
> +		void (*hdmi_infoframe_vsi)(struct nvkm_ior *, int
> head, void *data, u32 size);
> +	} gsp;
> +
>  	u16 ramht_size;
>  
>  	struct nvkm_sclass root;
> diff --git a/drivers/gpu/drm/nouveau/nvkm/engine/disp/tu102.c
> b/drivers/gpu/drm/nouveau/nvkm/engine/disp/tu102.c
> index 6cfd52c9056f..9db3cac487e3 100644
> --- a/drivers/gpu/drm/nouveau/nvkm/engine/disp/tu102.c
> +++ b/drivers/gpu/drm/nouveau/nvkm/engine/disp/tu102.c
> @@ -295,12 +295,26 @@ tu102_disp = {
>  	},
>  };
>  
> +static const struct nvkm_disp_func
> +tu102_gsp_disp = {
> +	.uevent = &gv100_disp_chan_uevent,
> +	.ramht_size = 0x2000,
> +	.gsp.intr = tu102_disp_intr,
> +	.gsp.head_state = gv100_head_state,
> +	.gsp.head_rgpos = gv100_head_rgpos,
> +	.gsp.vblank_get = tu102_head_vblank_get,
> +	.gsp.vblank_put = tu102_head_vblank_put,
> +	.gsp.hdmi_gcp = tu102_sor_hdmi_gcp,
> +	.gsp.hdmi_infoframe_avi = gv100_sor_hdmi_infoframe_avi,
> +	.gsp.hdmi_infoframe_vsi = gv100_sor_hdmi_infoframe_vsi,
> +};
> +
>  int
>  tu102_disp_new(struct nvkm_device *device, enum nvkm_subdev_type
> type, int inst,
>  	       struct nvkm_disp **pdisp)
>  {
>  	if (nvkm_gsp_rm(device->gsp))
> -		return r535_disp_new(&tu102_disp, device, type,
> inst, pdisp);
> +		return r535_disp_new(&tu102_gsp_disp, device, type,
> inst, pdisp);
>  
>  	return nvkm_disp_new_(&tu102_disp, device, type, inst,
> pdisp);
>  }
> 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 cd4451e62512..f3e55253bcbc 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
> @@ -547,7 +547,19 @@ r535_sor_hdmi_audio(struct nvkm_ior *sor, int
> head, bool enable)
>  {
>  	r535_sor_hdmi_ctrl_audio(sor->asy.outp, enable);
>  	r535_sor_hdmi_ctrl_audio_mute(sor->asy.outp, !enable);
> -	tu102_sor_hdmi_gcp(sor, head, enable);
> +	sor->disp->func->gsp.hdmi_gcp(sor, head, enable);
> +}
> +
> +static void
> +r535_sor_hdmi_infoframe_avi(struct nvkm_ior *sor, int head, void
> *data, u32 size)
> +{
> +	sor->disp->func->gsp.hdmi_infoframe_avi(sor, head, data,
> size);
> +}
> +
> +static void
> +r535_sor_hdmi_infoframe_vsi(struct nvkm_ior *sor, int head, void
> *data, u32 size)
> +{
> +	sor->disp->func->gsp.hdmi_infoframe_vsi(sor, head, data,
> size);
>  }
>  
>  static void
> @@ -575,8 +587,8 @@ r535_sor_hdmi = {
>  	.ctrl = r535_sor_hdmi_ctrl,
>  	.scdc = r535_sor_hdmi_scdc,
>  	/*TODO: SF_USER -> KMS. */
> -	.infoframe_avi = gv100_sor_hdmi_infoframe_avi,
> -	.infoframe_vsi = gv100_sor_hdmi_infoframe_vsi,
> +	.infoframe_avi = r535_sor_hdmi_infoframe_avi,
> +	.infoframe_vsi = r535_sor_hdmi_infoframe_vsi,
>  	.audio = r535_sor_hdmi_audio,
>  };
>  
> @@ -601,12 +613,36 @@ r535_sor_cnt(struct nvkm_disp *disp, unsigned
> long *pmask)
>  	return 4;
>  }
>  
> +static void
> +r535_head_state(struct nvkm_head *head, struct nvkm_head_state
> *state)
> +{
> +	head->disp->func->gsp.head_state(head, state);
> +}
> +
> +static void
> +r535_head_rgpos(struct nvkm_head *head, u16 *hline, u16 *vline)
> +{
> +	head->disp->func->gsp.head_rgpos(head, hline, vline);
> +}
> +
> +static void
> +r535_head_vblank_get(struct nvkm_head *head)
> +{
> +	head->disp->func->gsp.vblank_get(head);
> +}
> +
> +static void
> +r535_head_vblank_put(struct nvkm_head *head)
> +{
> +	head->disp->func->gsp.vblank_put(head);
> +}
> +
>  static const struct nvkm_head_func
>  r535_head = {
> -	.state = gv100_head_state,
> -	.rgpos = gv100_head_rgpos,
> -	.vblank_get = tu102_head_vblank_get,
> -	.vblank_put = tu102_head_vblank_put,
> +	.state = r535_head_state,
> +	.rgpos = r535_head_rgpos,
> +	.vblank_get = r535_head_vblank_get,
> +	.vblank_put = r535_head_vblank_put,
>  };
>  
>  static struct nvkm_conn *
> @@ -1650,12 +1686,17 @@ r535_disp_oneinit(struct nvkm_disp *disp)
>  	if (ret)
>  		return ret;
>  
> -	ret = nvkm_gsp_intr_stall(gsp, disp->engine.subdev.type,
> disp->engine.subdev.inst);
> +	/* 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.
> +	 */
> +	ret = nvkm_gsp_intr_stall(gsp, disp->engine.subdev.type,
> +				  disp->func->gsp.intr_low_latency ?
> 1 : disp->engine.subdev.inst);
>  	if (ret < 0)
>  		return ret;
>  
>  	ret = nvkm_inth_add(&device->vfn->intr, ret,
> NVKM_INTR_PRIO_NORMAL, &disp->engine.subdev,
> -			    tu102_disp_intr, &disp-
> >engine.subdev.inth);
> +			    disp->func->gsp.intr, &disp-
> >engine.subdev.inth);
>  	if (ret)
>  		return ret;
>  
> @@ -1688,6 +1729,7 @@ r535_disp_new(const struct nvkm_disp_func *hw,
> struct nvkm_device *device,
>  	rm->uevent = hw->uevent;
>  	rm->sor.cnt = r535_sor_cnt;
>  	rm->sor.new = r535_sor_new;
> +	rm->gsp = hw->gsp;
>  	rm->ramht_size = hw->ramht_size;
>  
>  	rm->root.oclass = gpu->disp.class.root;