Re: [PATCH] drm/amd/amdgpu: introduce DRM_AMDGPU_WERROR
[Public] Reviewed-by: Kenny Ho
Re: [PATCH] drm/amd/amdgpu: introduce DRM_AMDGPU_WERROR
[Public]
On 5/30/23 11:24, Hamza Mahfooz wrote:
> I am able to get clean builds with this enabled on GCC 11-13 and Clang
> 15, at least as of commit e786aef0869c ("drm/amd/display: remove unused
> definition") on amd-staging-drm-next.
Did you try intentionally introducing a warning to see if the build indeed fail?
Re: [PATCH v2] drm/amd/display: enable more strict compile checks
[AMD Official Use Only - General] I ran some experiment yesterday to turn on CONFIG_WERROR and here are some results (summary: gcc 12 seems ok but gcc 13 gives a lot more error, but not necessarily in our module.) Build with gcc13 using Fedora 38: http://zuul.linux.amd.com/t/osg/build/722ad77affed4f988d72051a84979e9f/log/job-output.txt#2924 Build with gcc12 using Fedora 37: http://zuul.linux.amd.com/t/osg/build/9e90f08bbeb044a2997a41b6cdc13f29/log/job-output.txt#7534 Kenny From: Alex Deucher Sent: Wednesday, May 24, 2023 3:50 PM To: Kuehling, Felix Cc: Russell, Kent ; Ho, Kenny ; Mahfooz, Hamza ; Li, Sun peng (Leo) ; Wentland, Harry ; Pan, Xinhui ; Siqueira, Rodrigo ; linux-ker...@vger.kernel.org ; dri-devel@lists.freedesktop.org ; amd-...@lists.freedesktop.org ; Daniel Vetter ; Deucher, Alexander ; David Airlie ; Koenig, Christian Subject: Re: [PATCH v2] drm/amd/display: enable more strict compile checks On Wed, May 24, 2023 at 3:46 PM Felix Kuehling wrote: > > Sure, I think we tried enabling warnings as errors before and had to > revert it because of weird compiler quirks or the variety of compiler > versions that need to be supported. > > Alex, are you planning to upstream this, or is this only to enforce more > internal discipline about not ignoring warnings? I'd like to upstream it. Upstream already has CONFIG_WERROR as a config option, but it's been problematic to enable in CI because of various breakages outside of the driver and in different compilers. That said, I don't know how much trouble enabling it will cause with various compilers in the wild. Alex > > Regards, >Felix > > > On 2023-05-24 15:41, Russell, Kent wrote: > > > > [AMD Official Use Only - General] > > > > > > (Adding Felix in CC) > > > > I’m a fan of adding it to KFD as well. Felix, can you foresee any > > issues here? > > > > Kent > > > > *From:* amd-gfx *On Behalf Of > > *Ho, Kenny > > *Sent:* Wednesday, May 24, 2023 3:23 PM > > *To:* Alex Deucher ; Mahfooz, Hamza > > > > *Cc:* Li, Sun peng (Leo) ; Wentland, Harry > > ; Pan, Xinhui ; Siqueira, > > Rodrigo ; linux-ker...@vger.kernel.org; > > dri-devel@lists.freedesktop.org; amd-...@lists.freedesktop.org; Daniel > > Vetter ; Deucher, Alexander > > ; David Airlie ; Koenig, > > Christian > > *Subject:* Re: [PATCH v2] drm/amd/display: enable more strict compile > > checks > > > > [AMD Official Use Only - General] > > > > [AMD Official Use Only - General] > > > > (+ Felix) > > > > Should we do the same for other modules under amd (amdkfd)? I was > > going to enable full kernel werror in the kconfig used by my CI but > > this is probably better. > > > > Kenny > > > > > > > > *From:*Alex Deucher > > *Sent:* Wednesday, May 24, 2023 3:22 PM > > *To:* Mahfooz, Hamza > > *Cc:* amd-...@lists.freedesktop.org ; > > Li, Sun peng (Leo) ; Ho, Kenny ; > > Pan, Xinhui ; Siqueira, Rodrigo > > ; linux-ker...@vger.kernel.org > > ; dri-devel@lists.freedesktop.org > > ; Daniel Vetter ; > > Deucher, Alexander ; David Airlie > > ; Wentland, Harry ; Koenig, > > Christian > > *Subject:* Re: [PATCH v2] drm/amd/display: enable more strict compile > > checks > > > > On Wed, May 24, 2023 at 3:20 PM Hamza Mahfooz > > wrote: > > > > > > Currently, there are quite a number of issues that are quite easy for > > > the CI to catch, that slip through the cracks. Among them, there are > > > unused variable and indentation issues. Also, we should consider all > > > warnings to be compile errors, since the community will eventually end > > > up complaining about them. So, enable -Werror, -Wunused and > > > -Wmisleading-indentation for all kernel builds. > > > > > > Cc: Alex Deucher > > > Cc: Harry Wentland > > > Cc: Kenny Ho > > > Signed-off-by: Hamza Mahfooz > > > --- > > > v2: fix grammatical error > > > --- > > > drivers/gpu/drm/amd/display/Makefile | 2 ++ > > > 1 file changed, 2 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/amd/display/Makefile > > b/drivers/gpu/drm/amd/display/Makefile > > > index 0d610cb376bb..3c44162ebe21 100644 > > > --- a/drivers/gpu/drm/amd/display/Makefile > > > +++ b/drivers/gpu/drm/amd/display/Makefile > > > @@ -26,6 +26,8 @@ > > > > > > AMDDALPATH = $(RELATIVE_AMD_DISPLAY_PATH) > > > > > > +subdir-ccflags-y += -Werror -Wunused -Wmisleading-indentation > > > + > > > > Care to enable this for the rest of amdgpu as well? Or send out an > > additional patch to do that? Either way: > > Reviewed-by: Alex Deucher > > > > Alex > > > > > subdir-ccflags-y += -I$(FULL_AMD_DISPLAY_PATH)/dc/inc/ > > > subdir-ccflags-y += -I$(FULL_AMD_DISPLAY_PATH)/dc/inc/hw > > > subdir-ccflags-y += -I$(FULL_AMD_DISPLAY_PATH)/dc/clk_mgr > > > -- > > > 2.40.1 > > > > >
Re: [PATCH v2] drm/amd/display: enable more strict compile checks
[AMD Official Use Only - General] (+ Felix) Should we do the same for other modules under amd (amdkfd)? I was going to enable full kernel werror in the kconfig used by my CI but this is probably better. Kenny From: Alex Deucher Sent: Wednesday, May 24, 2023 3:22 PM To: Mahfooz, Hamza Cc: amd-...@lists.freedesktop.org ; Li, Sun peng (Leo) ; Ho, Kenny ; Pan, Xinhui ; Siqueira, Rodrigo ; linux-ker...@vger.kernel.org ; dri-devel@lists.freedesktop.org ; Daniel Vetter ; Deucher, Alexander ; David Airlie ; Wentland, Harry ; Koenig, Christian Subject: Re: [PATCH v2] drm/amd/display: enable more strict compile checks On Wed, May 24, 2023 at 3:20 PM Hamza Mahfooz wrote: > > Currently, there are quite a number of issues that are quite easy for > the CI to catch, that slip through the cracks. Among them, there are > unused variable and indentation issues. Also, we should consider all > warnings to be compile errors, since the community will eventually end > up complaining about them. So, enable -Werror, -Wunused and > -Wmisleading-indentation for all kernel builds. > > Cc: Alex Deucher > Cc: Harry Wentland > Cc: Kenny Ho > Signed-off-by: Hamza Mahfooz > --- > v2: fix grammatical error > --- > drivers/gpu/drm/amd/display/Makefile | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/gpu/drm/amd/display/Makefile > b/drivers/gpu/drm/amd/display/Makefile > index 0d610cb376bb..3c44162ebe21 100644 > --- a/drivers/gpu/drm/amd/display/Makefile > +++ b/drivers/gpu/drm/amd/display/Makefile > @@ -26,6 +26,8 @@ > > AMDDALPATH = $(RELATIVE_AMD_DISPLAY_PATH) > > +subdir-ccflags-y += -Werror -Wunused -Wmisleading-indentation > + Care to enable this for the rest of amdgpu as well? Or send out an additional patch to do that? Either way: Reviewed-by: Alex Deucher Alex > subdir-ccflags-y += -I$(FULL_AMD_DISPLAY_PATH)/dc/inc/ > subdir-ccflags-y += -I$(FULL_AMD_DISPLAY_PATH)/dc/inc/hw > subdir-ccflags-y += -I$(FULL_AMD_DISPLAY_PATH)/dc/clk_mgr > -- > 2.40.1 >
Re: [PATCH v2 00/11] new cgroup controller for gpu/drm subsystem
[AMD Official Use Only - Internal Distribution Only] Hi Tejun, Thanks for taking the time to reply. Perhaps we can even narrow things down to just gpu.weight/gpu.compute.weight as a start? In this aspect, is the key objection to the current implementation of gpu.compute.weight the work-conserving bit? This work-conserving requirement is probably what I have missed for the last two years (and hence going in circle.) If this is the case, can you clarify/confirm the followings? 1) Is resource scheduling goal of cgroup purely for the purpose of throughput? (at the expense of other scheduling goals such as latency.) 2) If 1) is true, under what circumstances will the "Allocations" resource distribution model (as defined in the cgroup-v2) be acceptable? 3) If 1) is true, are things like cpuset from cgroup v1 no longer acceptable going forward? To be clear, while some have framed this (time sharing vs spatial sharing) as a partisan issue, it is in fact a technical one. I have implemented the gpu cgroup support this way because we have a class of users that value low latency/low jitter/predictability/synchronicity. For example, they would like 4 tasks to share a GPU and they would like the tasks to start and finish at the same time. What is the rationale behind picking the Weight model over Allocations as the first acceptable implementation? Can't we have both work-conserving and non-work-conserving ways of distributing GPU resources? If we can, why not allow non-work-conserving implementation first, especially when we have users asking for such functionality? Regards, Kenny From: Tejun Heo on behalf of Tejun Heo Sent: Monday, April 13, 2020 3:11 PM To: Kenny Ho Cc: Ho, Kenny ; cgro...@vger.kernel.org ; dri-devel ; amd-gfx list ; Deucher, Alexander ; Koenig, Christian ; Kuehling, Felix ; Greathouse, Joseph ; jspa...@cray.com Subject: Re: [PATCH v2 00/11] new cgroup controller for gpu/drm subsystem Hello, Kenny. On Tue, Mar 24, 2020 at 02:49:27PM -0400, Kenny Ho wrote: > Can you elaborate more on what are the missing pieces? Sorry about the long delay, but I think we've been going in circles for quite a while now. Let's try to make it really simple as the first step. How about something like the following? * gpu.weight (should it be gpu.compute.weight? idk) - A single number per-device weight similar to io.weight, which distributes computation resources in work-conserving way. * gpu.memory.high - A single number per-device on-device memory limit. The above two, if works well, should already be plenty useful. And my guess is that getting the above working well will be plenty challenging already even though it's already excluding work-conserving memory distribution. So, let's please do that as the first step and see what more would be needed from there. Thanks. -- tejun ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 2/8] drm/radeon: don't use ttm bo->offset
[AMD Official Use Only - Internal Distribution Only]
I believe bo->tbo.mem.mem_type is of uint32_t type and not an enum, is the
index lookup method safe? (i.e., how do you deal with the possibility of having
value TTM_PL_PRIV or above or are you suggesting those are not possible for
this function.)
Kenny
From: Tuikov, Luben
Sent: Thursday, March 5, 2020 2:19 PM
To: Nirmoy Das ; dri-devel@lists.freedesktop.org
Cc: Zhou, David(ChunMing) ; thellst...@vmware.com
; airl...@linux.ie ; Ho, Kenny
; brian.we...@intel.com ;
maarten.lankho...@linux.intel.com ;
amd-...@lists.freedesktop.org ; Das, Nirmoy
; linux-graphics-maintai...@vmware.com
; bske...@redhat.com
; dan...@ffwll.ch ; Deucher, Alexander
; s...@poorly.run ; Koenig,
Christian ; kra...@redhat.com
Subject: Re: [PATCH 2/8] drm/radeon: don't use ttm bo->offset
On 2020-03-05 08:29, Nirmoy Das wrote:
> Calculate GPU offset in radeon_bo_gpu_offset without depending on
> bo->offset.
>
> Signed-off-by: Nirmoy Das
> Reviewed-and-tested-by: Christian König
> ---
> drivers/gpu/drm/radeon/radeon.h| 1 +
> drivers/gpu/drm/radeon/radeon_object.h | 16 +++-
> drivers/gpu/drm/radeon/radeon_ttm.c| 4 +---
> 3 files changed, 17 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/radeon/radeon.h b/drivers/gpu/drm/radeon/radeon.h
> index 30e32adc1fc6..b7c3fb2bfb54 100644
> --- a/drivers/gpu/drm/radeon/radeon.h
> +++ b/drivers/gpu/drm/radeon/radeon.h
> @@ -2828,6 +2828,7 @@ extern void radeon_ttm_set_active_vram_size(struct
> radeon_device *rdev, u64 size
> extern void radeon_program_register_sequence(struct radeon_device *rdev,
> const u32 *registers,
> const u32 array_size);
> +struct radeon_device *radeon_get_rdev(struct ttm_bo_device *bdev);
>
> /*
> * vm
> diff --git a/drivers/gpu/drm/radeon/radeon_object.h
> b/drivers/gpu/drm/radeon/radeon_object.h
> index d23f2ed4126e..60275b822f79 100644
> --- a/drivers/gpu/drm/radeon/radeon_object.h
> +++ b/drivers/gpu/drm/radeon/radeon_object.h
> @@ -90,7 +90,21 @@ static inline void radeon_bo_unreserve(struct radeon_bo
> *bo)
> */
> static inline u64 radeon_bo_gpu_offset(struct radeon_bo *bo)
> {
> - return bo->tbo.offset;
> + struct radeon_device *rdev;
> + u64 start = 0;
> +
> + rdev = radeon_get_rdev(bo->tbo.bdev);
> +
> + switch (bo->tbo.mem.mem_type) {
> + case TTM_PL_TT:
> + start = rdev->mc.gtt_start;
> + break;
> + case TTM_PL_VRAM:
> + start = rdev->mc.vram_start;
> + break;
> + }
> +
> + return (bo->tbo.mem.start << PAGE_SHIFT) + start;
> }
You're removing a "return bo->tbo.offset" and adding a
switch-case statement. So, then, now instead of an instant
lookup, you're adding branching. You're adding comparison
and branching. Do you think that's better? Faster? Smaller?
I've written before about this for this patch: Why not create a map,
whose index is "mem_type" which references the desired
address? No comparison, no branching. Just an index-dereference
and a value:
return rdev->mc.mem_start_map[bo->tbo.mem.mem_type];
Obviously, you'll have to create "mem_start_map".
That's a NAK from me on this patch using comparison
and branching to return static data lookup value.
Regards,
Luben
>
> static inline unsigned long radeon_bo_size(struct radeon_bo *bo)
> diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c
> b/drivers/gpu/drm/radeon/radeon_ttm.c
> index badf1b6d1549..1c8303468e8f 100644
> --- a/drivers/gpu/drm/radeon/radeon_ttm.c
> +++ b/drivers/gpu/drm/radeon/radeon_ttm.c
> @@ -56,7 +56,7 @@
> static int radeon_ttm_debugfs_init(struct radeon_device *rdev);
> static void radeon_ttm_debugfs_fini(struct radeon_device *rdev);
>
> -static struct radeon_device *radeon_get_rdev(struct ttm_bo_device *bdev)
> +struct radeon_device *radeon_get_rdev(struct ttm_bo_device *bdev)
> {
>struct radeon_mman *mman;
>struct radeon_device *rdev;
> @@ -82,7 +82,6 @@ static int radeon_init_mem_type(struct ttm_bo_device *bdev,
> uint32_t type,
>break;
>case TTM_PL_TT:
>man->func = _bo_manager_func;
> - man->gpu_offset = rdev->mc.gtt_start;
>man->available_caching = TTM_PL_MASK_CACHING;
>man->default_caching = TTM_PL_FLAG_CACHED;
>man->flags = TTM_MEMTYPE_FLAG_MAPPABLE | TTM_MEMTYPE_FLAG_CMA;
> @@ -104,7 +103,6 @@ static int radeon_init_mem_type(struct ttm_bo_device
> *bdev, uint32_t type,
>case TTM_PL_VRAM:
>
Re: [Intel-gfx] [PATCH RFC 2/5] cgroup: Add mechanism to register vendor specific DRM devices
Hey Matt, On Fri, Nov 30, 2018 at 5:22 PM Matt Roper wrote: > I think Joonas is describing something closer in > design to the cgroup-v2 "cpu" controller, which partitions the general > time/usage allocated to via cgroup; afaiu, "cpu" doesn't really care > which specific core the tasks run on, just the relative weights that > determine how much time they get to run on any of the cores. Depending on the level of optimization one wants to do, I think people care about which cpu core a task runs on. Modern processors are no longer a monolithic 'thing'. At least for AMD, there are multiple cpus on a core complex (CCX), multiple CCX on a die, and multiple dies on a processor. A task running on cpu 0 and cpu 1 on die 0 will behave very differently from a task running on core 0s on die 0 and die 1 on the same socket. (https://en.wikichip.org/wiki/amd/microarchitectures/zen#Die-die_memory_latencies) It's not just an AMD thing either. Here is an open issue on Intel's architecture: https://github.com/kubernetes/kubernetes/issues/67355 and a proposed solution using cpu affinity https://github.com/kubernetes/community/blob/630acc487c80e4981a232cdd8400eb8207119788/keps/sig-node/0030-qos-class-cpu-affinity.md#proposal (by one of your colleagues.) The time-based sharing below is also something we are thinking about, but it's personally not as exciting as the resource-based sharing for me because the time-share use case has already been addressed by our SRIOV/virtualization products. We can potentially have different level of time sharing using cgroup though (in addition to SRIOV), potentially trading efficiency against isolation. That said, I think the time-based approach maybe orthogonal to the resource-based approach (orthogonal in the sense that both are needed depending on the usage.) Regards, Kenny > It sounds like with your hardware, your kernel driver is able to specify > exactly which subset of GPU EU's a specific GPU context winds up running > on. However I think there are a lot of platforms that don't allow that > kind of low-level control. E.g., I don't think we can do that on Intel > hardware; we have a handful of high-level GPU engines that we can submit > different types of batchbuffers to (render, blitter, media, etc.). What > we can do is use GPU preemption to limit how much time specific GPU > contexts get to run on the render engine before the engine is reclaimed > for use by a different context. > > Using a %gputime approach like Joonas is suggesting could be handled in > a driver by reserving specific subsets of EU's on hardware like yours > that's capable of doing that, whereas it could be mostly handled on > other types of hardware via GPU engine preemption. > > I think either approach "gpu_euset" or "%gputime" should map well to a > cgroup controller implementation. Granted, neither one solves the > specific use case I was working on earlier this year where we need > unfair (starvation-okay) scheduling that will run contexts strictly > according to priority (i.e., lower priority contexts will never run at > all unless all higher priority contexts have completed all of their > submitted work), but that's a pretty specialized use case that we'll > probably need to handle in a different manner anyway. > > > Matt > > > > Regards, > > Kennny > > > > > > > > > That combined with the "GPU memory usable" property should be a good > > > > > starting point to start subdividing the GPU resources for multiple > > > > > users. > > > > > > > > > > Regards, Joonas > > > > > > > > > > > > > > > > > Your feedback is highly appreciated. > > > > > > > > > > > > Best Regards, > > > > > > Harish > > > > > > > > > > > > > > > > > > > > > > > > From: amd-gfx on behalf of > > > > > > Tejun Heo > > > > > > Sent: Tuesday, November 20, 2018 5:30 PM > > > > > > To: Ho, Kenny > > > > > > Cc: cgro...@vger.kernel.org; intel-...@lists.freedesktop.org; > > > > > > y2ke...@gmail.com; amd-...@lists.freedesktop.org; > > > > > > dri-devel@lists.freedesktop.org > > > > > > Subject: Re: [PATCH RFC 2/5] cgroup: Add mechanism to register > > > > > > vendor specific DRM devices > > > > > > > > > > > > > > > > > > Hello, > > > > > > > > > > > > On Tue, Nov 20, 2018 at 10:21:14PM +, Ho, Kenny wrote: > > > &
Re: [Intel-gfx] [PATCH RFC 2/5] cgroup: Add mechanism to register vendor specific DRM devices
On Wed, Nov 28, 2018 at 4:14 AM Joonas Lahtinen wrote: > So we can only choose the lowest common denominator, right? > > Any core count out of total core count should translate nicely into a > fraction, so what would be the problem with percentage amounts? I don't think having an abstracted resource necessarily equate 'lowest'. The issue with percentage is the lack of precision. If you look at cpuset cgroup, you can see the specification can be very precise: # /bin/echo 1-4,6 > cpuset.cpus -> set cpus list to cpus 1,2,3,4,6 (https://www.kernel.org/doc/Documentation/cgroup-v1/cpusets.txt) The driver can translate something like this to core count and then to percentage and handle accordingly while the reverse is not possible. (You can't tell which set of CUs/EUs a user want from a percentage request.) It's also not clear to me, from user/application/admin/resource management perspective, how the base core counts of a GPU is relevant to the workload (since percentage is a 'relative' quantity.) For example, let say a workload wants to use 256 'cores', does it matter if that workload is put on a GPU with 1024 cores or a GPU with 4096 cores total? I am not dismissing the possible need for percentage. I just think there should be a way to accommodate more than just the 'lowest'. Regards, Kennny > > > That combined with the "GPU memory usable" property should be a good > > > starting point to start subdividing the GPU resources for multiple > > > users. > > > > > > Regards, Joonas > > > > > > > > > > > Your feedback is highly appreciated. > > > > > > > > Best Regards, > > > > Harish > > > > > > > > > > > > > > > > From: amd-gfx on behalf of > > > > Tejun Heo > > > > Sent: Tuesday, November 20, 2018 5:30 PM > > > > To: Ho, Kenny > > > > Cc: cgro...@vger.kernel.org; intel-...@lists.freedesktop.org; > > > > y2ke...@gmail.com; amd-...@lists.freedesktop.org; > > > > dri-devel@lists.freedesktop.org > > > > Subject: Re: [PATCH RFC 2/5] cgroup: Add mechanism to register vendor > > > > specific DRM devices > > > > > > > > > > > > Hello, > > > > > > > > On Tue, Nov 20, 2018 at 10:21:14PM +, Ho, Kenny wrote: > > > > > By this reply, are you suggesting that vendor specific resources > > > > > will never be acceptable to be managed under cgroup? Let say a user > > > > > > > > I wouldn't say never but whatever which gets included as a cgroup > > > > controller should have clearly defined resource abstractions and the > > > > control schemes around them including support for delegation. AFAICS, > > > > gpu side still seems to have a long way to go (and it's not clear > > > > whether that's somewhere it will or needs to end up). > > > > > > > > > want to have similar functionality as what cgroup is offering but to > > > > > manage vendor specific resources, what would you suggest as a > > > > > solution? When you say keeping vendor specific resource regulation > > > > > inside drm or specific drivers, do you mean we should replicate the > > > > > cgroup infrastructure there or do you mean either drm or specific > > > > > driver should query existing hierarchy (such as device or perhaps > > > > > cpu) for the process organization information? > > > > > > > > > > To put the questions in more concrete terms, let say a user wants to > > > > > expose certain part of a gpu to a particular cgroup similar to the > > > > > way selective cpu cores are exposed to a cgroup via cpuset, how > > > > > should we go about enabling such functionality? > > > > > > > > Do what the intel driver or bpf is doing? It's not difficult to hook > > > > into cgroup for identification purposes. > > > > > > > > Thanks. > > > > > > > > -- > > > > tejun > > > > ___ > > > > amd-gfx mailing list > > > > amd-...@lists.freedesktop.org > > > > https://lists.freedesktop.org/mailman/listinfo/amd-gfx > > > > > > > > > > > > amd-gfx Info Page - freedesktop.org > > > > lists.freedesktop.org > > > > To see the collection of prior postings to the list, visit the amd-gfx > > > > Archives.. Using amd-gfx: To post a message to all the list members, > > > > send email to amd-...@lists.freedesktop.org. You can subscribe to the > > > > list, or change your existing subscription, in the sections below. > > > > > > > > ___ > > > > Intel-gfx mailing list > > > > intel-...@lists.freedesktop.org > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
RE: [Intel-gfx] [PATCH RFC 2/5] cgroup: Add mechanism to register vendor specific DRM devices
On Tue, Nov 27, 2018 at 4:46 AM Joonas Lahtinen wrote: > I think a more abstract property "% of GPU (processing power)" might > be a more universal approach. One can then implement that through > subdividing the resources or timeslicing them, depending on the GPU > topology. > > Leasing 1/8th, 1/4th or 1/2 of the GPU would probably be the most > applicable to cloud provider usecases, too. At least that's what I > see done for the CPUs today. I think there are opportunities to slice the gpu in more than one way (similar to the way it is done for cpu.) We can potentially frame resources as continuous or discrete. Percentage definitely fits well for continuous measurements such as time/time slices but I think there are places for discrete units such as core counts as well. Regards, Kenny > That combined with the "GPU memory usable" property should be a good > starting point to start subdividing the GPU resources for multiple > users. > > Regards, Joonas > > > > > Your feedback is highly appreciated. > > > > Best Regards, > > Harish > > > > > > > > From: amd-gfx on behalf of Tejun > > Heo > > Sent: Tuesday, November 20, 2018 5:30 PM > > To: Ho, Kenny > > Cc: cgro...@vger.kernel.org; intel-...@lists.freedesktop.org; > > y2ke...@gmail.com; amd-...@lists.freedesktop.org; > > dri-devel@lists.freedesktop.org > > Subject: Re: [PATCH RFC 2/5] cgroup: Add mechanism to register vendor > > specific DRM devices > > > > > > Hello, > > > > On Tue, Nov 20, 2018 at 10:21:14PM +, Ho, Kenny wrote: > > > By this reply, are you suggesting that vendor specific resources > > > will never be acceptable to be managed under cgroup? Let say a user > > > > I wouldn't say never but whatever which gets included as a cgroup > > controller should have clearly defined resource abstractions and the > > control schemes around them including support for delegation. AFAICS, > > gpu side still seems to have a long way to go (and it's not clear > > whether that's somewhere it will or needs to end up). > > > > > want to have similar functionality as what cgroup is offering but to > > > manage vendor specific resources, what would you suggest as a > > > solution? When you say keeping vendor specific resource regulation > > > inside drm or specific drivers, do you mean we should replicate the > > > cgroup infrastructure there or do you mean either drm or specific > > > driver should query existing hierarchy (such as device or perhaps > > > cpu) for the process organization information? > > > > > > To put the questions in more concrete terms, let say a user wants to > > > expose certain part of a gpu to a particular cgroup similar to the > > > way selective cpu cores are exposed to a cgroup via cpuset, how > > > should we go about enabling such functionality? > > > > Do what the intel driver or bpf is doing? It's not difficult to hook > > into cgroup for identification purposes. > > > > Thanks. > > > > -- > > tejun > > ___ > > amd-gfx mailing list > > amd-...@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/amd-gfx > > > > > > amd-gfx Info Page - freedesktop.org > > lists.freedesktop.org > > To see the collection of prior postings to the list, visit the amd-gfx > > Archives.. Using amd-gfx: To post a message to all the list members, send > > email to amd-...@lists.freedesktop.org. You can subscribe to the list, or > > change your existing subscription, in the sections below. > > > > ___ > > Intel-gfx mailing list > > intel-...@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
RE: [PATCH RFC 4/5] drm/amdgpu: Add accounting of command submission via DRM cgroup
On Fri, Nov 23, 2018 at 1:13 PM Koenig, Christian wrote: > Am 23.11.18 um 18:36 schrieb Eric Anholt: > > Christian König writes: > >> Am 20.11.18 um 21:57 schrieb Eric Anholt: > >>> Kenny Ho writes: > Account for the number of command submitted to amdgpu by type on a per > cgroup basis, for the purpose of profiling/monitoring applications. > >>> For profiling other drivers, I've used perf tracepoints, which let you > >>> get useful timelines of multiple events in the driver. Have you made > >>> use of this stat for productive profiling? > >> Yes, but this is not related to profiling at all. > >> > >> What we want to do is to limit the resource usage of processes. > > That sounds great, and something I'd be interested in for vc4. However, > > as far as I saw explained here, this patch doesn't let you limit > > resource usage of a process and is only useful for > > "profiling/monitoring" so I'm wondering how it is useful for that > > purpose. > > Ok, good to know. I haven't looked at this in deep, but if this is just > for accounting that would certainly be missing the goal. The end goal is to have limit in place. The current patch is mostly to illustrate the structure of the controller and get some early feedback. I will have more soon. Regards, Kenny ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH RFC 2/5] cgroup: Add mechanism to register vendor specific DRM devices
(resending because previous email switched to HTML mode and was filtered out) Hi Tejun, On Tue, Nov 20, 2018 at 5:30 PM Tejun Heo wrote: > On Tue, Nov 20, 2018 at 10:21:14PM +0000, Ho, Kenny wrote: > > By this reply, are you suggesting that vendor specific resources > > will never be acceptable to be managed under cgroup? Let say a user > > I wouldn't say never but whatever which gets included as a cgroup > controller should have clearly defined resource abstractions and the > control schemes around them including support for delegation. AFAICS, > gpu side still seems to have a long way to go (and it's not clear > whether that's somewhere it will or needs to end up). Right, I totally understand that it's not obvious from this RFC because the 'resource' counting demonstrated in this RFC is trivial in nature, mostly to illustrate the 'vendor' concept. The structure of this patch actually give us the ability to support both abstracted resources you mentioned and vendor specific resources. It is probably not obvious as the RFC only includes two resources and they are both vendor specific. To be clear, I am not saying there aren't abstracted resources in drm, there are (we are still working on those). What I am saying is that not all resources are abstracted and for the purpose of this RFC I was hoping to get some feedback on the vendor specific parts early just so that we don't go down the wrong path. That said, I think I am getting a better sense of what you are saying. Please correct me if I misinterpreted: your concern is that abstracting by vendor is too high level and it's too much of a free-for-all. Instead, resources should be abstracted at the controller level even if it's only available to a specific vendor (or even a specific product from a specific vendor). Is that a fair read? A couple of additional side questions: * Is statistic/accounting-only use cases like those enabled by cpuacct controller no longer sufficient? If it is still sufficient, can you elaborate more on what you mean by having control schemes and supporting delegation? * When you wrote delegation, do you mean delegation in the sense described in https://www.kernel.org/doc/Documentation/cgroup-v2.txt ? > > To put the questions in more concrete terms, let say a user wants to > > expose certain part of a gpu to a particular cgroup similar to the > > way selective cpu cores are exposed to a cgroup via cpuset, how > > should we go about enabling such functionality? > > Do what the intel driver or bpf is doing? It's not difficult to hook > into cgroup for identification purposes. Does intel driver or bpf present an interface file in cgroupfs for users to configure the core selection like cpuset? I must admit I am not too familiar with the bpf case as I was referencing mostly the way rdma was implemented when putting this RFC together. Perhaps I wasn't communicating clearly so let me see if I can illustrate this discussion with a hypothetical but concrete example using our competitor's product. Nvidia has something called Tensor Cores in some of their GPUs and the purpose of those cores is to accelerate matrix operations for machine learning applications. This is something unique to Nvidia and to my knowledge no one else has something like it. These cores are different from regular shader processors and there are multiple of them in a GPU. Under the structure of this RFC, if Nvidia wants to make Tensor Cores manageable via cgroup (with the "Allocation" distribution model let say), they will probably have an interface file called "drm.nvidia.tensor_core", in which only nvidia's GPUs will be listed. If a GPU has TC, it will have a positive count, otherwise 0. If I understand you correctly Tejun, is that they should not do that. What they should do is have an abstracted resource, possibly named "drm.matrix_accelerator" where all drm devices available on a system will be listed. All GPUs except some Nvidia's will have a count of 0. Or perhaps that is not sufficiently abstracted so instead there should be just "drm.cores" instead and that file list both device, core types and count. For one vendor they may have shader proc, texture map unit, tensor core, ray tracing cores as types. Others may have ALUs, EUs and subslices. Is that an accurate representation of what you are recommending? Regards, Kenny ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH RFC 2/5] cgroup: Add mechanism to register vendor specific DRM devices
Hi Tejun, On Tue, Nov 20, 2018 at 5:30 PM Tejun Heo wrote: > On Tue, Nov 20, 2018 at 10:21:14PM +0000, Ho, Kenny wrote: > > By this reply, are you suggesting that vendor specific resources > > will never be acceptable to be managed under cgroup? Let say a user > > I wouldn't say never but whatever which gets included as a cgroup > controller should have clearly defined resource abstractions and the > control schemes around them including support for delegation. AFAICS, > gpu side still seems to have a long way to go (and it's not clear > whether that's somewhere it will or needs to end up). Right, I totally understand that it's not obvious from this RFC because the 'resource' counting demonstrated in this RFC is trivial in nature, mostly to illustrate the 'vendor' concept. The structure of this patch actually give us the ability to support both abstracted resources you mentioned and vendor specific resources. But it is probably not very clear as the RFC only includes two resources and they are both vendor specific. To be clear, I am not saying there aren't abstracted resources in drm, there are (we are still working on those). What I am saying is that not all resources are abstracted and for the purpose of this RFC I was hoping to get some feedback on the vendor specific parts early just so that we don't go down the wrong path. That said, I think I am getting a better sense of what you are saying. Please correct me if I misinterpreted: your concern is that abstracting by vendor is too high level and it's too much of a free-for-all. Instead, resources should be abstracted at the controller level even if it's only available to a specific vendor (or even a specific product from a specific vendor). Is that a fair read? A couple of additional side questions: * Is statistic/accounting-only use cases like those enabled by cpuacct controller no longer sufficient? If it is still sufficient, can you elaborate more on what you mean by having control schemes and supporting delegation? * When you wrote delegation, do you mean delegation in the sense described in https://www.kernel.org/doc/Documentation/cgroup-v2.txt ? > > To put the questions in more concrete terms, let say a user wants to > > expose certain part of a gpu to a particular cgroup similar to the > > way selective cpu cores are exposed to a cgroup via cpuset, how > > should we go about enabling such functionality? > > Do what the intel driver or bpf is doing? It's not difficult to hook > into cgroup for identification purposes. Does intel driver or bpf present an interface file in cgroupfs for users to configure the core selection like cpuset? I must admit I am not too familiar with the bpf case as I was referencing mostly the way rdma was implemented when putting this RFC together. Perhaps I wasn't communicating clearly so let me see if I can illustrate this discussion with a hypothetical but concrete example using our competitor's product. Nvidia has something called Tensor Cores in some of their GPUs and the purpose of those cores is to accelerate matrix operations for machine learning applications. This is something unique to Nvidia and to my knowledge no one else has something like it. These cores are different from regular shader processors and there are multiple of them in a GPU. Under the structure of this RFC, if Nvidia wants to make Tensor Cores manageable via cgroup (with the "Allocation" distribution model let say), they will probably have an interface file called "drm.nvidia.tensor_core", in which only nvidia's GPUs will be listed. If a GPU has TC, it will have a positive count, otherwise 0. If I understand you correctly Tejun, is that they should not do that. What they should do is have an abstracted resource, possibly named "drm.matrix_accelerator" where all drm devices available on a system will be listed. All GPUs except some Nvidia's will have a count of 0. Or perhaps that is not sufficiently abstracted so instead there should be just "drm.cores" instead and that file list both device, core types and count. For one vendor they may have shader proc, texture map unit, tensor core, ray tracing cores as types. Others may have ALUs, EUs and subslices. Is that an accurate representation of what you are recommending? Regards, Kenny ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
RE: [PATCH RFC 2/5] cgroup: Add mechanism to register vendor specific DRM devices
Hi Tejun, Thanks for the reply. A few clarifying questions: On Tue, Nov 20, 2018 at 3:21 PM Tejun Heo wrote: > So, I'm still pretty negative about adding drm controller at this > point. There isn't enough of common resource model defined yet and > until that gets sorted out I think it's in the best interest of > everyone involved to keep it inside drm or specific driver proper. By this reply, are you suggesting that vendor specific resources will never be acceptable to be managed under cgroup? Let say a user want to have similar functionality as what cgroup is offering but to manage vendor specific resources, what would you suggest as a solution? When you say keeping vendor specific resource regulation inside drm or specific drivers, do you mean we should replicate the cgroup infrastructure there or do you mean either drm or specific driver should query existing hierarchy (such as device or perhaps cpu) for the process organization information? To put the questions in more concrete terms, let say a user wants to expose certain part of a gpu to a particular cgroup similar to the way selective cpu cores are exposed to a cgroup via cpuset, how should we go about enabling such functionality? Regards, Kenny ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
