Re: Rework TTMs busy handling
Hi, Christian On Tue, 2024-01-09 at 08:47 +0100, Christian König wrote: > Hi guys, > > I'm trying to make this functionality a bit more useful for years now > since we multiple reports that behavior of drivers can be suboptimal > when multiple placements be given. > > So basically instead of hacking around the TTM behavior in the driver > once more I've gone ahead and changed the idle/busy placement list > into idle/busy placement flags. This not only saves a bunch of code, > but also allows setting some placements as fallback which are used if > allocating from the preferred ones didn't worked. > > Zack pointed out that some removed VMWGFX code was brought back > because > of rebasing, fixed in this version. > > Intel CI seems to be happy with those patches, so any more comments? Looks like Xe changes are missing? (xe is now in drm-tip). I also have some doubts about the naming "idle" vs "busy", since an elaborate eviction mechanism would probably at some point want to check for gpu idle vs gpu busy, and this might create some confusion moving forward for people confusing busy as in memory overcommit with busy as in gpu activity. I can't immediately think of something better, though. /Thomas > > Regards, > Christian. > >
Re: Rework TTMs busy handling
Am 09.01.24 um 09:14 schrieb Thomas Hellström: Hi, Christian On Tue, 2024-01-09 at 08:47 +0100, Christian König wrote: Hi guys, I'm trying to make this functionality a bit more useful for years now since we multiple reports that behavior of drivers can be suboptimal when multiple placements be given. So basically instead of hacking around the TTM behavior in the driver once more I've gone ahead and changed the idle/busy placement list into idle/busy placement flags. This not only saves a bunch of code, but also allows setting some placements as fallback which are used if allocating from the preferred ones didn't worked. Zack pointed out that some removed VMWGFX code was brought back because of rebasing, fixed in this version. Intel CI seems to be happy with those patches, so any more comments? Looks like Xe changes are missing? (xe is now in drm-tip). I also have some doubts about the naming "idle" vs "busy", since an elaborate eviction mechanism would probably at some point want to check for gpu idle vs gpu busy, and this might create some confusion moving forward for people confusing busy as in memory overcommit with busy as in gpu activity. I can't immediately think of something better, though. Yeah, I was wondering about that as well. Especially since I wanted to add some more flags in the future when for example a bandwidth quota how much memory can be moved in/out is exceeded. Something like phase1, phase2, phase3 etc..., but that's also not very descriptive either. Going to take a look at XE as well, thanks for the notice. Regards, Christian. /Thomas Regards, Christian.
Re: [PATCH 2/5] drm/ttm: return ENOSPC from ttm_bo_mem_space
On Tue, 2024-01-09 at 08:47 +0100, Christian König wrote:
> Only convert it to ENOMEM in ttm_bo_validate.
>
Could we have a more elaborate commit description here, (why is this
change needed)?
> Signed-off-by: Christian König
> ---
> drivers/gpu/drm/ttm/ttm_bo.c | 5 -
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c
> b/drivers/gpu/drm/ttm/ttm_bo.c
> index edf10618fe2b..8c1eaa74fa21 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
> @@ -830,7 +830,7 @@ int ttm_bo_mem_space(struct ttm_buffer_object
> *bo,
> goto error;
> }
>
> - ret = -ENOMEM;
> + ret = -ENOSPC;
> if (!type_found) {
> pr_err(TTM_PFX "No compatible memory type found\n");
> ret = -EINVAL;
> @@ -916,6 +916,9 @@ int ttm_bo_validate(struct ttm_buffer_object *bo,
> return -EINVAL;
>
> ret = ttm_bo_move_buffer(bo, placement, ctx);
> + /* For backward compatibility with userspace */
> + if (ret == -ENOSPC)
> + return -ENOMEM;
> if (ret)
> return ret;
>
Re: [PATCH 1/3] drm/nouveau: include drm/drm_edid.h only where needed
On Mon, 08 Jan 2024, Danilo Krummrich wrote:
> On 1/4/24 21:16, Jani Nikula wrote:
>> Including drm_edid.h from nouveau_connector.h causes the rebuild of 15
>> files when drm_edid.h is modified, while there are only a few files that
>> actually need to include drm_edid.h.
>>
>> Cc: Karol Herbst
>> Cc: Lyude Paul
>> Cc: Danilo Krummrich
>> Cc: nouveau@lists.freedesktop.org
>> Signed-off-by: Jani Nikula
>
> Reviewed-by: Danilo Krummrich
Are you going to pick this up via the nouveau tree, or shall I apply it
to drm-misc-next?
BR,
Jani.
>
>> ---
>> drivers/gpu/drm/nouveau/dispnv50/head.c | 1 +
>> drivers/gpu/drm/nouveau/nouveau_connector.h | 2 +-
>> 2 files changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/nouveau/dispnv50/head.c
>> b/drivers/gpu/drm/nouveau/dispnv50/head.c
>> index 5f490fbf1877..83355dbc15ee 100644
>> --- a/drivers/gpu/drm/nouveau/dispnv50/head.c
>> +++ b/drivers/gpu/drm/nouveau/dispnv50/head.c
>> @@ -32,6 +32,7 @@
>>
>> #include
>> #include
>> +#include
>> #include
>> #include "nouveau_connector.h"
>>
>> diff --git a/drivers/gpu/drm/nouveau/nouveau_connector.h
>> b/drivers/gpu/drm/nouveau/nouveau_connector.h
>> index a2df4918340c..0608cabed058 100644
>> --- a/drivers/gpu/drm/nouveau/nouveau_connector.h
>> +++ b/drivers/gpu/drm/nouveau/nouveau_connector.h
>> @@ -35,7 +35,6 @@
>>
>> #include
>> #include
>> -#include
>> #include
>> #include
>>
>> @@ -44,6 +43,7 @@
>>
>> struct nvkm_i2c_port;
>> struct dcb_output;
>> +struct edid;
>>
>> #ifdef CONFIG_DRM_NOUVEAU_BACKLIGHT
>> struct nouveau_backlight {
>
--
Jani Nikula, Intel
Re: [PATCH -next] drm/nouveau: uapi: fix kerneldoc warnings
On Mon, 08 Jan 2024, Danilo Krummrich wrote:
> On 12/25/23 07:51, Vegard Nossum wrote:
>> As of commit b77fdd6a48e6 ("scripts/kernel-doc: restore warning for
>> Excess struct/union"), we see the following warnings when running 'make
>> htmldocs':
>>
>>./include/uapi/drm/nouveau_drm.h:292: warning: Excess struct member
>> 'DRM_NOUVEAU_VM_BIND_OP_MAP' description in 'drm_nouveau_vm_bind_op'
>>./include/uapi/drm/nouveau_drm.h:292: warning: Excess struct member
>> 'DRM_NOUVEAU_VM_BIND_OP_UNMAP' description in 'drm_nouveau_vm_bind_op'
>>./include/uapi/drm/nouveau_drm.h:292: warning: Excess struct member
>> 'DRM_NOUVEAU_VM_BIND_SPARSE' description in 'drm_nouveau_vm_bind_op'
>>./include/uapi/drm/nouveau_drm.h:336: warning: Excess struct member
>> 'DRM_NOUVEAU_VM_BIND_RUN_ASYNC' description in 'drm_nouveau_vm_bind'
>>
>> The problem is that these values are #define constants, but had kerneldoc
>> comments attached to them as if they were actual struct members.
>>
>> There are a number of ways we could fix this, but I chose to draw
>> inspiration from include/uapi/drm/i915_drm.h, which pulls them into the
>> corresponding kerneldoc comment for the struct member that they are
>> intended to be used with.
>>
>> To keep the diff readable, there are a number of things I _didn't_ do in
>> this patch, but which we should also consider:
>>
>> - This is pretty good documentation, but it ends up in gpu/driver-uapi,
>>which is part of subsystem-apis/ when it really ought to display under
>>userspace-api/ (the "Linux kernel user-space API guide" book of the
>>documentation).
>
> I agree, it indeed looks like this would make sense, same goes for
> gpu/drm-uapi.rst.
>
> @Jani, Sima: Was this intentional? Or can we change it?
I have no recollection of this, but overall I'd say do what makes sense,
and where things are easiest to find.
BR,
Jani.
>
>>
>> - More generally, we might want a warning if include/uapi/ files are
>>kerneldoc'd outside userspace-api/.
>>
>> - I'd consider it cleaner if the #defines appeared between the kerneldoc
>>for the member and the member itself (which is something other DRM-
>>related UAPI docs do).
>>
>> - The %IDENTIFIER kerneldoc syntax is intended for "constants", and is
>>more appropriate in this context than ``IDENTIFIER`` or &IDENTIFIER.
>>The DRM docs aren't very consistent on this.
>>
>> Cc: Randy Dunlap
>> Cc: Jonathan Corbet
>> Signed-off-by: Vegard Nossum
>
> Applied to drm-misc-next, thanks!
>
>> ---
>> include/uapi/drm/nouveau_drm.h | 56 --
>> 1 file changed, 27 insertions(+), 29 deletions(-)
>>
>> diff --git a/include/uapi/drm/nouveau_drm.h b/include/uapi/drm/nouveau_drm.h
>> index 0bade1592f34..c95ef8a4d94a 100644
>> --- a/include/uapi/drm/nouveau_drm.h
>> +++ b/include/uapi/drm/nouveau_drm.h
>> @@ -238,34 +238,32 @@ struct drm_nouveau_vm_init {
>> struct drm_nouveau_vm_bind_op {
>> /**
>> * @op: the operation type
>> + *
>> + * Supported values:
>> + *
>> + * %DRM_NOUVEAU_VM_BIND_OP_MAP - Map a GEM object to the GPU's VA
>> + * space. Optionally, the &DRM_NOUVEAU_VM_BIND_SPARSE flag can be
>> + * passed to instruct the kernel to create sparse mappings for the
>> + * given range.
>> + *
>> + * %DRM_NOUVEAU_VM_BIND_OP_UNMAP - Unmap an existing mapping in the
>> + * GPU's VA space. If the region the mapping is located in is a
>> + * sparse region, new sparse mappings are created where the unmapped
>> + * (memory backed) mapping was mapped previously. To remove a sparse
>> + * region the &DRM_NOUVEAU_VM_BIND_SPARSE must be set.
>> */
>> __u32 op;
>> -/**
>> - * @DRM_NOUVEAU_VM_BIND_OP_MAP:
>> - *
>> - * Map a GEM object to the GPU's VA space. Optionally, the
>> - * &DRM_NOUVEAU_VM_BIND_SPARSE flag can be passed to instruct the kernel to
>> - * create sparse mappings for the given range.
>> - */
>> #define DRM_NOUVEAU_VM_BIND_OP_MAP 0x0
>> -/**
>> - * @DRM_NOUVEAU_VM_BIND_OP_UNMAP:
>> - *
>> - * Unmap an existing mapping in the GPU's VA space. If the region the
>> mapping
>> - * is located in is a sparse region, new sparse mappings are created where
>> the
>> - * unmapped (memory backed) mapping was mapped previously. To remove a
>> sparse
>> - * region the &DRM_NOUVEAU_VM_BIND_SPARSE must be set.
>> - */
>> #define DRM_NOUVEAU_VM_BIND_OP_UNMAP 0x1
>> /**
>> * @flags: the flags for a &drm_nouveau_vm_bind_op
>> + *
>> + * Supported values:
>> + *
>> + * %DRM_NOUVEAU_VM_BIND_SPARSE - Indicates that an allocated VA
>> + * space region should be sparse.
>> */
>> __u32 flags;
>> -/**
>> - * @DRM_NOUVEAU_VM_BIND_SPARSE:
>> - *
>> - * Indicates that an allocated VA space region should be sparse.
>> - */
>> #define DRM_NOUVEAU_VM_BIND_SPARSE (1 << 8)
>> /**
>> * @handle: the handle of the DRM GEM object to map
>> @@ -301,17 +299,17 @@ struct drm_nouveau_vm
Re: [PATCH 1/3] drm/nouveau: include drm/drm_edid.h only where needed
On 1/9/24 10:59, Jani Nikula wrote:
On Mon, 08 Jan 2024, Danilo Krummrich wrote:
On 1/4/24 21:16, Jani Nikula wrote:
Including drm_edid.h from nouveau_connector.h causes the rebuild of 15
files when drm_edid.h is modified, while there are only a few files that
actually need to include drm_edid.h.
Cc: Karol Herbst
Cc: Lyude Paul
Cc: Danilo Krummrich
Cc: nouveau@lists.freedesktop.org
Signed-off-by: Jani Nikula
Reviewed-by: Danilo Krummrich
Are you going to pick this up via the nouveau tree, or shall I apply it
to drm-misc-next?
We don't maintain a separate tree, hence feel free to apply it to drm-misc-next.
- Danilo
BR,
Jani.
---
drivers/gpu/drm/nouveau/dispnv50/head.c | 1 +
drivers/gpu/drm/nouveau/nouveau_connector.h | 2 +-
2 files changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/nouveau/dispnv50/head.c
b/drivers/gpu/drm/nouveau/dispnv50/head.c
index 5f490fbf1877..83355dbc15ee 100644
--- a/drivers/gpu/drm/nouveau/dispnv50/head.c
+++ b/drivers/gpu/drm/nouveau/dispnv50/head.c
@@ -32,6 +32,7 @@
#include
#include
+#include
#include
#include "nouveau_connector.h"
diff --git a/drivers/gpu/drm/nouveau/nouveau_connector.h b/drivers/gpu/drm/nouveau/nouveau_connector.h
index a2df4918340c..0608cabed058 100644
--- a/drivers/gpu/drm/nouveau/nouveau_connector.h
+++ b/drivers/gpu/drm/nouveau/nouveau_connector.h
@@ -35,7 +35,6 @@
#include
#include
-#include
#include
#include
@@ -44,6 +43,7 @@
struct nvkm_i2c_port;
struct dcb_output;
+struct edid;
#ifdef CONFIG_DRM_NOUVEAU_BACKLIGHT
struct nouveau_backlight {
[PATCH] nouveau/gsp: handle engines in runl without nonstall interrupts.
From: Dave Airlie
It appears on TU106 GPUs (2070), that some of the nvdec engines
are in the runlist but have no valid nonstall interrupt, nouveau
didn't handle that too well.
This should let nouveau/gsp work on those.
Cc: sta...@vger.kernel.org # v6.7+
---
drivers/gpu/drm/nouveau/nvkm/engine/fifo/ga100.c | 4
drivers/gpu/drm/nouveau/nvkm/engine/fifo/r535.c | 2 +-
drivers/gpu/drm/nouveau/nvkm/subdev/gsp/base.c | 8 ++--
3 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nvkm/engine/fifo/ga100.c
b/drivers/gpu/drm/nouveau/nvkm/engine/fifo/ga100.c
index c8ce7ff18713..e74493a4569e 100644
--- a/drivers/gpu/drm/nouveau/nvkm/engine/fifo/ga100.c
+++ b/drivers/gpu/drm/nouveau/nvkm/engine/fifo/ga100.c
@@ -550,6 +550,10 @@ ga100_fifo_nonstall_ctor(struct nvkm_fifo *fifo)
struct nvkm_engn *engn = list_first_entry(&runl->engns,
typeof(*engn), head);
runl->nonstall.vector = engn->func->nonstall(engn);
+
+ /* if no nonstall vector just keep going */
+ if (runl->nonstall.vector == -1)
+ continue;
if (runl->nonstall.vector < 0) {
RUNL_ERROR(runl, "nonstall %d", runl->nonstall.vector);
return runl->nonstall.vector;
diff --git a/drivers/gpu/drm/nouveau/nvkm/engine/fifo/r535.c
b/drivers/gpu/drm/nouveau/nvkm/engine/fifo/r535.c
index b903785056b5..3454c7d29502 100644
--- a/drivers/gpu/drm/nouveau/nvkm/engine/fifo/r535.c
+++ b/drivers/gpu/drm/nouveau/nvkm/engine/fifo/r535.c
@@ -351,7 +351,7 @@ r535_engn_nonstall(struct nvkm_engn *engn)
int ret;
ret = nvkm_gsp_intr_nonstall(subdev->device->gsp, subdev->type,
subdev->inst);
- WARN_ON(ret < 0);
+ WARN_ON(ret == -ENOENT);
return ret;
}
diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/base.c
b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/base.c
index 04bceaa28a19..da1bebb896f7 100644
--- a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/base.c
+++ b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/base.c
@@ -25,12 +25,8 @@ int
nvkm_gsp_intr_nonstall(struct nvkm_gsp *gsp, enum nvkm_subdev_type type, int
inst)
{
for (int i = 0; i < gsp->intr_nr; i++) {
- if (gsp->intr[i].type == type && gsp->intr[i].inst == inst) {
- if (gsp->intr[i].nonstall != ~0)
- return gsp->intr[i].nonstall;
-
- return -EINVAL;
- }
+ if (gsp->intr[i].type == type && gsp->intr[i].inst == inst)
+ return gsp->intr[i].nonstall;
}
return -ENOENT;
--
2.43.0
