On Thu, 2023-09-28 at 11:12 +1000, Dave Airlie wrote:
> On Thu, 28 Sept 2023 at 07:55, Faith Ekstrand
> <faith.ekstrand@xxxxxxxxxxxxx> wrote:
> >
> > On Wed, 2023-09-27 at 03:22 +0200, Danilo Krummrich wrote:
> > > Report the maximum number of IBs that can be pushed with a single
> > > DRM_IOCTL_NOUVEAU_EXEC through DRM_IOCTL_NOUVEAU_GETPARAM.
> > >
> > > While the maximum number of IBs per ring might vary between
> > > chipsets,
> > > the kernel will make sure that userspace can only push a fraction
> > > of
> > > the
> > > maximum number of IBs per ring per job, such that we avoid a
> > > situation
> > > where there's only a single job occupying the ring, which could
> > > potentially lead to the ring run dry.
> > >
> > > Using DRM_IOCTL_NOUVEAU_GETPARAM to report the maximum number of
> > > IBs
> > > that can be pushed with a single DRM_IOCTL_NOUVEAU_EXEC implies
> > > that
> > > all channels of a given device have the same ring size.
> >
> > There's a bunch of nouveau kernel details I don't know here but the
> > interface looks good and I prefer it to a #define in the header.
> >
> > Acked-by: Faith Ekstrand <faith.ekstrand@xxxxxxxxxxxxx>
>
> For the series
>
> Reviewed-by: Dave Airlie <airlied@xxxxxxxxxx>
>
> we should probably land this in drm-misc-fixes, since it would be
> good
> to have in 6.6
Agreed. My Mesa patch should handle both the case where we have the
getparam and when we don't. However, I'd rather just make it part of
the new UAPI from the start and have a hard requirement on it since it
may reduce the current maximum in the header.
~Faith
> Dave.
>
> >
> >
> > > Signed-off-by: Danilo Krummrich <dakr@xxxxxxxxxx>
> > > ---
> > > drivers/gpu/drm/nouveau/nouveau_abi16.c | 19 +++++++++++++++++++
> > > drivers/gpu/drm/nouveau/nouveau_chan.c | 2 +-
> > > drivers/gpu/drm/nouveau/nouveau_dma.h | 3 +++
> > > drivers/gpu/drm/nouveau/nouveau_exec.c | 7 ++++---
> > > drivers/gpu/drm/nouveau/nouveau_exec.h | 5 +++++
> > > include/uapi/drm/nouveau_drm.h | 10 ++++++++++
> > > 6 files changed, 42 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/nouveau/nouveau_abi16.c
> > > b/drivers/gpu/drm/nouveau/nouveau_abi16.c
> > > index 30afbec9e3b1..1a198689b391 100644
> > > --- a/drivers/gpu/drm/nouveau/nouveau_abi16.c
> > > +++ b/drivers/gpu/drm/nouveau/nouveau_abi16.c
> > > @@ -31,6 +31,7 @@
> > >
> > > #include "nouveau_drv.h"
> > > #include "nouveau_dma.h"
> > > +#include "nouveau_exec.h"
> > > #include "nouveau_gem.h"
> > > #include "nouveau_chan.h"
> > > #include "nouveau_abi16.h"
> > > @@ -183,6 +184,20 @@ nouveau_abi16_fini(struct nouveau_abi16
> > > *abi16)
> > > cli->abi16 = NULL;
> > > }
> > >
> > > +static inline unsigned int
> > > +getparam_dma_ib_max(struct nvif_device *device)
> > > +{
> > > + const struct nvif_mclass dmas[] = {
> > > + { NV03_CHANNEL_DMA, 0 },
> > > + { NV10_CHANNEL_DMA, 0 },
> > > + { NV17_CHANNEL_DMA, 0 },
> > > + { NV40_CHANNEL_DMA, 0 },
> > > + {}
> > > + };
> > > +
> > > + return nvif_mclass(&device->object, dmas) < 0 ?
> > > NV50_DMA_IB_MAX : 0;
> > > +}
> > > +
> > > int
> > > nouveau_abi16_ioctl_getparam(ABI16_IOCTL_ARGS)
> > > {
> > > @@ -247,6 +262,10 @@
> > > nouveau_abi16_ioctl_getparam(ABI16_IOCTL_ARGS)
> > > case NOUVEAU_GETPARAM_GRAPH_UNITS:
> > > getparam->value = nvkm_gr_units(gr);
> > > break;
> > > + case NOUVEAU_GETPARAM_EXEC_PUSH_MAX:
> > > + getparam->value = getparam_dma_ib_max(device) /
> > > + NOUVEAU_EXEC_PUSH_MAX_DIV;
> > > + break;
> > > default:
> > > NV_PRINTK(dbg, cli, "unknown parameter %lld\n",
> > > getparam->param);
> > > return -EINVAL;
> > > diff --git a/drivers/gpu/drm/nouveau/nouveau_chan.c
> > > b/drivers/gpu/drm/nouveau/nouveau_chan.c
> > > index ac56f4689ee3..c3c2ab887978 100644
> > > --- a/drivers/gpu/drm/nouveau/nouveau_chan.c
> > > +++ b/drivers/gpu/drm/nouveau/nouveau_chan.c
> > > @@ -456,7 +456,7 @@ nouveau_channel_init(struct nouveau_channel
> > > *chan, u32 vram, u32 gart)
> > > chan->user_get = 0x44;
> > > chan->user_get_hi = 0x60;
> > > chan->dma.ib_base = 0x10000 / 4;
> > > - chan->dma.ib_max = (0x02000 / 8) - 1;
> > > + chan->dma.ib_max = NV50_DMA_IB_MAX;
> > > chan->dma.ib_put = 0;
> > > chan->dma.ib_free = chan->dma.ib_max - chan-
> > > > dma.ib_put;
> > > chan->dma.max = chan->dma.ib_base;
> > > diff --git a/drivers/gpu/drm/nouveau/nouveau_dma.h
> > > b/drivers/gpu/drm/nouveau/nouveau_dma.h
> > > index 1744d95b233e..c52cda82353e 100644
> > > --- a/drivers/gpu/drm/nouveau/nouveau_dma.h
> > > +++ b/drivers/gpu/drm/nouveau/nouveau_dma.h
> > > @@ -49,6 +49,9 @@ void nv50_dma_push(struct nouveau_channel *,
> > > u64
> > > addr, u32 length,
> > > /* Maximum push buffer size. */
> > > #define NV50_DMA_PUSH_MAX_LENGTH 0x7fffff
> > >
> > > +/* Maximum IBs per ring. */
> > > +#define NV50_DMA_IB_MAX ((0x02000 / 8) - 1)
> > > +
> > > /* Object handles - for stuff that's doesn't use handle ==
> > > oclass.
> > > */
> > > enum {
> > > NvDmaFB = 0x80000002,
> > > diff --git a/drivers/gpu/drm/nouveau/nouveau_exec.c
> > > b/drivers/gpu/drm/nouveau/nouveau_exec.c
> > > index ba6913a3efb6..5b5c4a77b8e6 100644
> > > --- a/drivers/gpu/drm/nouveau/nouveau_exec.c
> > > +++ b/drivers/gpu/drm/nouveau/nouveau_exec.c
> > > @@ -346,7 +346,7 @@ nouveau_exec_ioctl_exec(struct drm_device
> > > *dev,
> > > struct nouveau_channel *chan = NULL;
> > > struct nouveau_exec_job_args args = {};
> > > struct drm_nouveau_exec *req = data;
> > > - int ret = 0;
> > > + int push_max, ret = 0;
> > >
> > > if (unlikely(!abi16))
> > > return -ENOMEM;
> > > @@ -371,9 +371,10 @@ nouveau_exec_ioctl_exec(struct drm_device
> > > *dev,
> > > if (!chan->dma.ib_max)
> > > return nouveau_abi16_put(abi16, -ENOSYS);
> > >
> > > - if (unlikely(req->push_count > NOUVEAU_GEM_MAX_PUSH)) {
> > > + push_max = chan->dma.ib_max / NOUVEAU_EXEC_PUSH_MAX_DIV;
> > > + if (unlikely(req->push_count > push_max)) {
> > > NV_PRINTK(err, cli, "pushbuf push count exceeds
> > > limit: %d max %d\n",
> > > - req->push_count, NOUVEAU_GEM_MAX_PUSH);
> > > + req->push_count, push_max);
> > > return nouveau_abi16_put(abi16, -EINVAL);
> > > }
> > >
> > > diff --git a/drivers/gpu/drm/nouveau/nouveau_exec.h
> > > b/drivers/gpu/drm/nouveau/nouveau_exec.h
> > > index b815de2428f3..c6270452e4b5 100644
> > > --- a/drivers/gpu/drm/nouveau/nouveau_exec.h
> > > +++ b/drivers/gpu/drm/nouveau/nouveau_exec.h
> > > @@ -6,6 +6,11 @@
> > > #include "nouveau_drv.h"
> > > #include "nouveau_sched.h"
> > >
> > > +/* Divider to limit the number of IBs per job to half the size
> > > of
> > > the ring in
> > > + * order to avoid the ring running dry between submissions.
> > > + */
> > > +#define NOUVEAU_EXEC_PUSH_MAX_DIV 2
> > > +
> > > struct nouveau_exec_job_args {
> > > struct drm_file *file_priv;
> > > struct nouveau_sched_entity *sched_entity;
> > > diff --git a/include/uapi/drm/nouveau_drm.h
> > > b/include/uapi/drm/nouveau_drm.h
> > > index 8d7402c13e56..eaf9f248619f 100644
> > > --- a/include/uapi/drm/nouveau_drm.h
> > > +++ b/include/uapi/drm/nouveau_drm.h
> > > @@ -44,6 +44,16 @@ extern "C" {
> > > #define NOUVEAU_GETPARAM_PTIMER_TIME 14
> > > #define NOUVEAU_GETPARAM_HAS_BO_USAGE 15
> > > #define NOUVEAU_GETPARAM_HAS_PAGEFLIP 16
> > > +
> > > +/**
> > > + * @NOUVEAU_GETPARAM_EXEC_PUSH_MAX
> > > + *
> > > + * Query the maximum amount of IBs that can be pushed through a
> > > single
> > > + * &drm_nouveau_exec structure and hence a single
> > > &DRM_IOCTL_NOUVEAU_EXEC
> > > + * ioctl().
> > > + */
> > > +#define NOUVEAU_GETPARAM_EXEC_PUSH_MAX 17
> > > +
> > > struct drm_nouveau_getparam {
> > > __u64 param;
> > > __u64 value;
> >
