Re: [Nouveau] [PATCH 1/2] drm/nouveau/kms/nv50-: Program notifier offset before requesting disp caps
On Mon, 2020-08-31 at 14:26 +1000, Ben Skeggs wrote: > On Wed, 26 Aug 2020 at 02:52, Lyude Paul wrote: > > On Tue, 2020-08-25 at 08:28 +1000, Ben Skeggs wrote: > > > On Tue, 25 Aug 2020 at 04:33, Lyude Paul wrote: > > > > Not entirely sure why this never came up when I originally tested this > > > > (maybe some BIOSes already have this setup?) but the ->caps_init vfunc > > > > appears to cause the display engine to throw an exception on driver > > > > init, at least on my ThinkPad P72: > > > > > > > > nouveau :01:00.0: disp: chid 0 mthd 008c data 508c > > > > 102b > > > > > > > > This is magic nvidia speak for "You need to have the DMA notifier offset > > > > programmed before you can call NV507D_GET_CAPABILITIES." So, let's fix > > > > this by doing that, and also perform an update afterwards to prevent > > > > racing with the GPU when reading capabilities. > > > > > > > > Changes since v1: > > > > * Don't just program the DMA notifier offset, make sure to actually > > > > perform an update > > > I'm not sure there's a need to send an Update() method here, I believe > > > GetCapabilities() is an action method on its own right? > > > > > > > I'm not entirely sure about this part tbh. I do know that we need to call > > GetCapabilities() _after_ the DMA notifier offset is programmed. But, my > > assumption was that if GetCapabilities() requires a DMA notifier offset to > > store > > its results in, we'd probably want to fire an update or something to make > > sure > > that we're not reading before it finishes writing capabilities? > We definitely want to *wait* on GetCapabilities() finishing, I believe > it should also update the notifier the same (or similar) way Update() > does. But I don't think we want to send an Update() here, it'll > actually trigger a modeset (which, on earlier HW, will tear down the > boot mode. Not sure about current HW, it might preserve state), and > we may not want that to happen there. I'm not so sure about that, as it seems like the notifier times out without the update: [5.142033] nouveau :1f:00.0: DRM: [DRM/:kmsChanPush] : 00040088 mthd 0x0088 size 1 - core507d_init [5.142037] nouveau :1f:00.0: DRM: [DRM/:kmsChanPush] 0004: f000-> NV507D_SET_CONTEXT_DMA_NOTIFIER [5.142041] nouveau :1f:00.0: DRM: [DRM/:kmsChanPush] 0008: 00040084 mthd 0x0084 size 1 - core507d_caps_init [5.142044] nouveau :1f:00.0: DRM: [DRM/:kmsChanPush] 000c: 8000-> NV507D_SET_NOTIFIER_CONTROL [5.142047] nouveau :1f:00.0: DRM: [DRM/:kmsChanPush] 0010: 0004008c mthd 0x008c size 1 - core507d_caps_init [5.142050] nouveau :1f:00.0: DRM: [DRM/:kmsChanPush] 0014: -> NV507D_GET_CAPABILITIES [7.142026] nouveau :1f:00.0: DRM: core notifier timeout [7.142700] nouveau :1f:00.0: DRM: sor-0002-0fc1 caps: dp_interlace=0 [7.142708] nouveau :1f:00.0: DRM: sor-0002-0fc4 caps: dp_interlace=0 [7.142715] nouveau :1f:00.0: DRM: sor-0002-0f42 caps: dp_interlace=0 [7.142829] nouveau :1f:00.0: DRM: sor-0006-0f82 caps: dp_interlace=0 [7.142842] nouveau :1f:00.0: DRM: sor-0002-0f82 caps: dp_interlace=0 [7.142849] nouveau :1f:00.0: DRM: failed to create encoder 1/8/0: -19 [7.142851] nouveau :1f:00.0: DRM: Virtual-1 has no encoders, removing Any other alternatives to UPDATE we might want to try? > > Ben. > > > > Ben. > > > > > > > Signed-off-by: Lyude Paul > > > > Fixes: 4a2cb4181b07 ("drm/nouveau/kms/nv50-: Probe SOR and PIOR caps for > > > > DP > > > > interlacing support") > > > > Cc: # v5.8+ > > > > --- > > > > drivers/gpu/drm/nouveau/dispnv50/core507d.c | 25 - > > > > 1 file changed, 19 insertions(+), 6 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/nouveau/dispnv50/core507d.c > > > > b/drivers/gpu/drm/nouveau/dispnv50/core507d.c > > > > index e341f572c2696..5e86feec3b720 100644 > > > > --- a/drivers/gpu/drm/nouveau/dispnv50/core507d.c > > > > +++ b/drivers/gpu/drm/nouveau/dispnv50/core507d.c > > > > @@ -65,13 +65,26 @@ core507d_ntfy_init(struct nouveau_bo *bo, u32 > > > > offset) > > > > int > > > > core507d_caps_init(struct nouveau_drm *drm, struct nv50_disp *disp) > > > > { > > > > - u32 *push = evo_wait(>core->chan, 2); > > > > + struct nv50_core *core = disp->core; > > > > + u32 interlock[NV50_DISP_INTERLOCK__SIZE] = {0}; > > > > + u32 *push; > > > > > > > > - if (push) { > > > > - evo_mthd(push, 0x008c, 1); > > > > - evo_data(push, 0x0); > > > > - evo_kick(push, >core->chan); > > > > - } > > > > + core->func->ntfy_init(disp->sync, NV50_DISP_CORE_NTFY); > > > > + > > > > + push = evo_wait(>chan, 4); > > > > + if (!push) > > > > + return 0; > > > > + > > > > + evo_mthd(push, 0x0084, 1); > > > > + evo_data(push, 0x8000 |
Re: [Nouveau] [PATCH 1/2] drm/nouveau/kms/nv50-: Program notifier offset before requesting disp caps
On Wed, 26 Aug 2020 at 02:52, Lyude Paul wrote: > > On Tue, 2020-08-25 at 08:28 +1000, Ben Skeggs wrote: > > On Tue, 25 Aug 2020 at 04:33, Lyude Paul wrote: > > > Not entirely sure why this never came up when I originally tested this > > > (maybe some BIOSes already have this setup?) but the ->caps_init vfunc > > > appears to cause the display engine to throw an exception on driver > > > init, at least on my ThinkPad P72: > > > > > > nouveau :01:00.0: disp: chid 0 mthd 008c data 508c > > > 102b > > > > > > This is magic nvidia speak for "You need to have the DMA notifier offset > > > programmed before you can call NV507D_GET_CAPABILITIES." So, let's fix > > > this by doing that, and also perform an update afterwards to prevent > > > racing with the GPU when reading capabilities. > > > > > > Changes since v1: > > > * Don't just program the DMA notifier offset, make sure to actually > > > perform an update > > I'm not sure there's a need to send an Update() method here, I believe > > GetCapabilities() is an action method on its own right? > > > > I'm not entirely sure about this part tbh. I do know that we need to call > GetCapabilities() _after_ the DMA notifier offset is programmed. But, my > assumption was that if GetCapabilities() requires a DMA notifier offset to > store > its results in, we'd probably want to fire an update or something to make sure > that we're not reading before it finishes writing capabilities? We definitely want to *wait* on GetCapabilities() finishing, I believe it should also update the notifier the same (or similar) way Update() does. But I don't think we want to send an Update() here, it'll actually trigger a modeset (which, on earlier HW, will tear down the boot mode. Not sure about current HW, it might preserve state), and we may not want that to happen there. Ben. > > > Ben. > > > > > Signed-off-by: Lyude Paul > > > Fixes: 4a2cb4181b07 ("drm/nouveau/kms/nv50-: Probe SOR and PIOR caps for > > > DP > > > interlacing support") > > > Cc: # v5.8+ > > > --- > > > drivers/gpu/drm/nouveau/dispnv50/core507d.c | 25 - > > > 1 file changed, 19 insertions(+), 6 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/nouveau/dispnv50/core507d.c > > > b/drivers/gpu/drm/nouveau/dispnv50/core507d.c > > > index e341f572c2696..5e86feec3b720 100644 > > > --- a/drivers/gpu/drm/nouveau/dispnv50/core507d.c > > > +++ b/drivers/gpu/drm/nouveau/dispnv50/core507d.c > > > @@ -65,13 +65,26 @@ core507d_ntfy_init(struct nouveau_bo *bo, u32 offset) > > > int > > > core507d_caps_init(struct nouveau_drm *drm, struct nv50_disp *disp) > > > { > > > - u32 *push = evo_wait(>core->chan, 2); > > > + struct nv50_core *core = disp->core; > > > + u32 interlock[NV50_DISP_INTERLOCK__SIZE] = {0}; > > > + u32 *push; > > > > > > - if (push) { > > > - evo_mthd(push, 0x008c, 1); > > > - evo_data(push, 0x0); > > > - evo_kick(push, >core->chan); > > > - } > > > + core->func->ntfy_init(disp->sync, NV50_DISP_CORE_NTFY); > > > + > > > + push = evo_wait(>chan, 4); > > > + if (!push) > > > + return 0; > > > + > > > + evo_mthd(push, 0x0084, 1); > > > + evo_data(push, 0x8000 | NV50_DISP_CORE_NTFY); > > > + evo_mthd(push, 0x008c, 1); > > > + evo_data(push, 0x0); > > > + evo_kick(push, >chan); > > > + > > > + core->func->update(core, interlock, false); > > > + if (core->func->ntfy_wait_done(disp->sync, NV50_DISP_CORE_NTFY, > > > + core->chan.base.device)) > > > + NV_ERROR(drm, "core notifier timeout\n"); > > > > > > return 0; > > > } > > > -- > > > 2.26.2 > > > > > > ___ > > > Nouveau mailing list > > > Nouveau@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/nouveau > -- > Sincerely, > Lyude Paul (she/her) > Software Engineer at Red Hat > ___ Nouveau mailing list Nouveau@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/nouveau
Re: [Nouveau] [PATCH 1/2] drm/nouveau/kms/nv50-: Program notifier offset before requesting disp caps
On Tue, 2020-08-25 at 08:28 +1000, Ben Skeggs wrote: > On Tue, 25 Aug 2020 at 04:33, Lyude Paul wrote: > > Not entirely sure why this never came up when I originally tested this > > (maybe some BIOSes already have this setup?) but the ->caps_init vfunc > > appears to cause the display engine to throw an exception on driver > > init, at least on my ThinkPad P72: > > > > nouveau :01:00.0: disp: chid 0 mthd 008c data 508c 102b > > > > This is magic nvidia speak for "You need to have the DMA notifier offset > > programmed before you can call NV507D_GET_CAPABILITIES." So, let's fix > > this by doing that, and also perform an update afterwards to prevent > > racing with the GPU when reading capabilities. > > > > Changes since v1: > > * Don't just program the DMA notifier offset, make sure to actually > > perform an update > I'm not sure there's a need to send an Update() method here, I believe > GetCapabilities() is an action method on its own right? > I'm not entirely sure about this part tbh. I do know that we need to call GetCapabilities() _after_ the DMA notifier offset is programmed. But, my assumption was that if GetCapabilities() requires a DMA notifier offset to store its results in, we'd probably want to fire an update or something to make sure that we're not reading before it finishes writing capabilities? > Ben. > > > Signed-off-by: Lyude Paul > > Fixes: 4a2cb4181b07 ("drm/nouveau/kms/nv50-: Probe SOR and PIOR caps for DP > > interlacing support") > > Cc: # v5.8+ > > --- > > drivers/gpu/drm/nouveau/dispnv50/core507d.c | 25 - > > 1 file changed, 19 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/gpu/drm/nouveau/dispnv50/core507d.c > > b/drivers/gpu/drm/nouveau/dispnv50/core507d.c > > index e341f572c2696..5e86feec3b720 100644 > > --- a/drivers/gpu/drm/nouveau/dispnv50/core507d.c > > +++ b/drivers/gpu/drm/nouveau/dispnv50/core507d.c > > @@ -65,13 +65,26 @@ core507d_ntfy_init(struct nouveau_bo *bo, u32 offset) > > int > > core507d_caps_init(struct nouveau_drm *drm, struct nv50_disp *disp) > > { > > - u32 *push = evo_wait(>core->chan, 2); > > + struct nv50_core *core = disp->core; > > + u32 interlock[NV50_DISP_INTERLOCK__SIZE] = {0}; > > + u32 *push; > > > > - if (push) { > > - evo_mthd(push, 0x008c, 1); > > - evo_data(push, 0x0); > > - evo_kick(push, >core->chan); > > - } > > + core->func->ntfy_init(disp->sync, NV50_DISP_CORE_NTFY); > > + > > + push = evo_wait(>chan, 4); > > + if (!push) > > + return 0; > > + > > + evo_mthd(push, 0x0084, 1); > > + evo_data(push, 0x8000 | NV50_DISP_CORE_NTFY); > > + evo_mthd(push, 0x008c, 1); > > + evo_data(push, 0x0); > > + evo_kick(push, >chan); > > + > > + core->func->update(core, interlock, false); > > + if (core->func->ntfy_wait_done(disp->sync, NV50_DISP_CORE_NTFY, > > + core->chan.base.device)) > > + NV_ERROR(drm, "core notifier timeout\n"); > > > > return 0; > > } > > -- > > 2.26.2 > > > > ___ > > Nouveau mailing list > > Nouveau@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/nouveau -- Sincerely, Lyude Paul (she/her) Software Engineer at Red Hat ___ Nouveau mailing list Nouveau@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/nouveau
Re: [Nouveau] [PATCH 1/2] drm/nouveau/kms/nv50-: Program notifier offset before requesting disp caps
On Tue, 25 Aug 2020 at 04:33, Lyude Paul wrote: > > Not entirely sure why this never came up when I originally tested this > (maybe some BIOSes already have this setup?) but the ->caps_init vfunc > appears to cause the display engine to throw an exception on driver > init, at least on my ThinkPad P72: > > nouveau :01:00.0: disp: chid 0 mthd 008c data 508c 102b > > This is magic nvidia speak for "You need to have the DMA notifier offset > programmed before you can call NV507D_GET_CAPABILITIES." So, let's fix > this by doing that, and also perform an update afterwards to prevent > racing with the GPU when reading capabilities. > > Changes since v1: > * Don't just program the DMA notifier offset, make sure to actually > perform an update I'm not sure there's a need to send an Update() method here, I believe GetCapabilities() is an action method on its own right? Ben. > > Signed-off-by: Lyude Paul > Fixes: 4a2cb4181b07 ("drm/nouveau/kms/nv50-: Probe SOR and PIOR caps for DP > interlacing support") > Cc: # v5.8+ > --- > drivers/gpu/drm/nouveau/dispnv50/core507d.c | 25 - > 1 file changed, 19 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/nouveau/dispnv50/core507d.c > b/drivers/gpu/drm/nouveau/dispnv50/core507d.c > index e341f572c2696..5e86feec3b720 100644 > --- a/drivers/gpu/drm/nouveau/dispnv50/core507d.c > +++ b/drivers/gpu/drm/nouveau/dispnv50/core507d.c > @@ -65,13 +65,26 @@ core507d_ntfy_init(struct nouveau_bo *bo, u32 offset) > int > core507d_caps_init(struct nouveau_drm *drm, struct nv50_disp *disp) > { > - u32 *push = evo_wait(>core->chan, 2); > + struct nv50_core *core = disp->core; > + u32 interlock[NV50_DISP_INTERLOCK__SIZE] = {0}; > + u32 *push; > > - if (push) { > - evo_mthd(push, 0x008c, 1); > - evo_data(push, 0x0); > - evo_kick(push, >core->chan); > - } > + core->func->ntfy_init(disp->sync, NV50_DISP_CORE_NTFY); > + > + push = evo_wait(>chan, 4); > + if (!push) > + return 0; > + > + evo_mthd(push, 0x0084, 1); > + evo_data(push, 0x8000 | NV50_DISP_CORE_NTFY); > + evo_mthd(push, 0x008c, 1); > + evo_data(push, 0x0); > + evo_kick(push, >chan); > + > + core->func->update(core, interlock, false); > + if (core->func->ntfy_wait_done(disp->sync, NV50_DISP_CORE_NTFY, > + core->chan.base.device)) > + NV_ERROR(drm, "core notifier timeout\n"); > > return 0; > } > -- > 2.26.2 > > ___ > Nouveau mailing list > Nouveau@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/nouveau ___ Nouveau mailing list Nouveau@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/nouveau
[Nouveau] [PATCH 1/2] drm/nouveau/kms/nv50-: Program notifier offset before requesting disp caps
Not entirely sure why this never came up when I originally tested this (maybe some BIOSes already have this setup?) but the ->caps_init vfunc appears to cause the display engine to throw an exception on driver init, at least on my ThinkPad P72: nouveau :01:00.0: disp: chid 0 mthd 008c data 508c 102b This is magic nvidia speak for "You need to have the DMA notifier offset programmed before you can call NV507D_GET_CAPABILITIES." So, let's fix this by doing that, and also perform an update afterwards to prevent racing with the GPU when reading capabilities. Changes since v1: * Don't just program the DMA notifier offset, make sure to actually perform an update Signed-off-by: Lyude Paul Fixes: 4a2cb4181b07 ("drm/nouveau/kms/nv50-: Probe SOR and PIOR caps for DP interlacing support") Cc: # v5.8+ --- drivers/gpu/drm/nouveau/dispnv50/core507d.c | 25 - 1 file changed, 19 insertions(+), 6 deletions(-) diff --git a/drivers/gpu/drm/nouveau/dispnv50/core507d.c b/drivers/gpu/drm/nouveau/dispnv50/core507d.c index e341f572c2696..5e86feec3b720 100644 --- a/drivers/gpu/drm/nouveau/dispnv50/core507d.c +++ b/drivers/gpu/drm/nouveau/dispnv50/core507d.c @@ -65,13 +65,26 @@ core507d_ntfy_init(struct nouveau_bo *bo, u32 offset) int core507d_caps_init(struct nouveau_drm *drm, struct nv50_disp *disp) { - u32 *push = evo_wait(>core->chan, 2); + struct nv50_core *core = disp->core; + u32 interlock[NV50_DISP_INTERLOCK__SIZE] = {0}; + u32 *push; - if (push) { - evo_mthd(push, 0x008c, 1); - evo_data(push, 0x0); - evo_kick(push, >core->chan); - } + core->func->ntfy_init(disp->sync, NV50_DISP_CORE_NTFY); + + push = evo_wait(>chan, 4); + if (!push) + return 0; + + evo_mthd(push, 0x0084, 1); + evo_data(push, 0x8000 | NV50_DISP_CORE_NTFY); + evo_mthd(push, 0x008c, 1); + evo_data(push, 0x0); + evo_kick(push, >chan); + + core->func->update(core, interlock, false); + if (core->func->ntfy_wait_done(disp->sync, NV50_DISP_CORE_NTFY, + core->chan.base.device)) + NV_ERROR(drm, "core notifier timeout\n"); return 0; } -- 2.26.2 ___ Nouveau mailing list Nouveau@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/nouveau