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 <[email protected]>
> ---
> .../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;
lmpx.com only provides a reader for public news (NNTP) servers. It is not
affiliated with the servers or forums shown here and is not responsible for
the content of articles, which is written by their respective authors.