Re: drm/amdkfd: Change pasid's type to unsigned int
[AMD Official Use Only - Internal Distribution Only] Hi Fenghua, I am okay with the idea. Regards, Yong From: Fenghua Yu Sent: Friday, May 22, 2020 5:21 PM To: Kuehling, Felix Cc: Zhao, Yong ; amd-gfx@lists.freedesktop.org Subject: Re: drm/amdkfd: Change pasid's type to unsigned int Hi, Felix, On Fri, May 22, 2020 at 03:40:06PM -0400, Felix Kuehling wrote: > Hi Fenghua, > > The PASID width in KFD is currently limited to 16 bits. I believe this > reflects what our hardware can handle. KFD will never allocate a PASID > bigger than 16 bits. That said, I'm OK with changing this field in the > kfd_process structure to unsigned int. Generally, I find uint16_t in > structures not very useful except in tightly packed structures such as > packet formats or ioctl arguments. Thank you very much for your insight! I'm writing the patch set to define pasid as "unsigned int" consistently in iommu. I'll put the amdkfd changes (only a few changes including this pasid change in struct kfd_processin) one patch and send it to you for review. -Fenghua ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH v2] drm/amdkfd: Track SDMA utilization per process
Track SDMA usage on a per process basis and report it through sysfs.
The value in the sysfs file indicates the amount of time SDMA has
been in-use by this process since the creation of the process.
This value is in microsecond granularity.
v2:
- Remove unnecessary checks for pdd is kfd_procfs_show().
- Make counter part of the kfd_sdma_activity_handler_workarea
structure.
Signed-off-by: Mukul Joshi
---
.../drm/amd/amdkfd/kfd_device_queue_manager.c | 57
.../drm/amd/amdkfd/kfd_device_queue_manager.h | 2 +
drivers/gpu/drm/amd/amdkfd/kfd_priv.h | 16 ++-
drivers/gpu/drm/amd/amdkfd/kfd_process.c | 130 --
4 files changed, 191 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
index e9c4867abeff..49f72d0f7be7 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
@@ -153,6 +153,52 @@ void decrement_queue_count(struct device_queue_manager
*dqm,
dqm->active_cp_queue_count--;
}
+int read_sdma_queue_counter(struct queue *q, uint64_t *val)
+{
+ int ret;
+ uint64_t tmp = 0;
+
+ if (!q || !val)
+ return -EINVAL;
+ /*
+* SDMA activity counter is stored at queue's RPTR + 0x8 location.
+*/
+ if (!access_ok((const void __user *)((uint64_t)q->properties.read_ptr +
+ sizeof(uint64_t)), sizeof(uint64_t))) {
+ pr_err("Can't access sdma queue activity counter\n");
+ return -EFAULT;
+ }
+
+ ret = get_user(tmp, (uint64_t *)((uint64_t)(q->properties.read_ptr) +
+ sizeof(uint64_t)));
+ if (!ret) {
+ *val = tmp;
+ }
+
+ return ret;
+}
+
+static int update_sdma_queue_past_activity_stats(struct kfd_process_device
*pdd,
+struct queue *q)
+{
+ int ret;
+ uint64_t val = 0;
+
+ if (!pdd)
+ return -ENODEV;
+
+ ret = read_sdma_queue_counter(q, );
+ if (ret) {
+ pr_err("Failed to read SDMA queue counter for queue: %d\n",
+ q->properties.queue_id);
+ return ret;
+ }
+
+ WRITE_ONCE(pdd->sdma_past_activity_counter,
pdd->sdma_past_activity_counter + val);
+
+ return ret;
+}
+
static int allocate_doorbell(struct qcm_process_device *qpd, struct queue *q)
{
struct kfd_dev *dev = qpd->dqm->dev;
@@ -487,6 +533,12 @@ static int destroy_queue_nocpsch_locked(struct
device_queue_manager *dqm,
if (retval == -ETIME)
qpd->reset_wavefronts = true;
+ /* Get the SDMA queue stats */
+if ((q->properties.type == KFD_QUEUE_TYPE_SDMA) ||
+(q->properties.type == KFD_QUEUE_TYPE_SDMA_XGMI)) {
+update_sdma_queue_past_activity_stats(qpd_to_pdd(qpd), q);
+}
+
mqd_mgr->free_mqd(mqd_mgr, q->mqd, q->mqd_mem_obj);
list_del(>list);
@@ -1468,6 +1520,11 @@ static int destroy_queue_cpsch(struct
device_queue_manager *dqm,
}
}
+ /* Get the SDMA queue stats */
+ if ((q->properties.type == KFD_QUEUE_TYPE_SDMA) ||
+ (q->properties.type == KFD_QUEUE_TYPE_SDMA_XGMI)) {
+ update_sdma_queue_past_activity_stats(qpd_to_pdd(qpd), q);
+ }
/*
* Unconditionally decrement this counter, regardless of the queue's
* type
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.h
b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.h
index 4afa015c69b1..894bcf877f9e 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.h
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.h
@@ -251,4 +251,6 @@ static inline void dqm_unlock(struct device_queue_manager
*dqm)
mutex_unlock(>lock_hidden);
}
+int read_sdma_queue_counter(struct queue *q, uint64_t *val);
+
#endif /* KFD_DEVICE_QUEUE_MANAGER_H_ */
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
index f70f789c3cb3..fae139b77c0a 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
@@ -633,7 +633,14 @@ enum kfd_pdd_bound {
PDD_BOUND_SUSPENDED,
};
-#define MAX_VRAM_FILENAME_LEN 11
+#define MAX_SYSFS_FILENAME_LEN 11
+
+/*
+ * SDMA counter runs at 100MHz frequency.
+ * We display SDMA activity in microsecond granularity in sysfs.
+ * As a result, the divisor is 100.
+ */
+#define SDMA_ACTIVITY_DIVISOR 100
/* Data that is per-process-per device. */
struct kfd_process_device {
@@ -681,7 +688,12 @@ struct kfd_process_device {
/* VRAM usage */
uint64_t vram_usage;
struct attribute attr_vram;
- char vram_filename[MAX_VRAM_FILENAME_LEN];
+ char
Re: slow rx 5600 xt fps
so yea, looks like the compositing wasnt happening on the amdgpu, so thats why when i would see 300fps for glxgears etc. also, the whole thing about "monitor updating once every 3 seconds" when i close the lid is because mutter will go down to 1fps when it detects that the lid is closed. i setup the compositor to use the graphics card (by simply using a custom xorg.conf with the Screen's device section being the amd device) and now it runs perfectly. ill write up a lil blog post or something to explain it. will link it in this thread if yall are curious. but really it boils down to "yall are right" so the fix for now is simply to use a single xorg.conf which specifically says to use the device for the X screen thanks alot for yalls help On Thu, May 21, 2020 at 4:21 PM Javad Karabi wrote: > > the files i attached are using the amdgpu ddx > > also, one thing to note: i just switched to modesetting but it seems > it has the same issue. > i got it working last night, forgot what i changed. but that was one > of things i changed. but here are the files for when i use the amdgpu > ddx > > On Thu, May 21, 2020 at 2:15 PM Alex Deucher wrote: > > > > Please provide your dmesg output and xorg log. > > > > Alex > > > > On Thu, May 21, 2020 at 3:03 PM Javad Karabi wrote: > > > > > > Alex, > > > yea, youre totally right i was overcomplicating it lol > > > so i was able to get the radeon to run super fast, by doing as you > > > suggested and blacklisting i915. > > > (had to use module_blacklist= though because modprobe.blacklist still > > > allows i915, if a dependency wants to load it) > > > but with one caveat: > > > using the amdgpu driver, there was some error saying something about > > > telling me that i need to add BusID to my device or something. > > > maybe amdgpu wasnt able to find the card or something, i dont > > > remember. so i used modesetting instead and it seemed to work. > > > i will try going back to amdgpu and seeing what that error message was. > > > i recall you saying that modesetting doesnt have some features that > > > amdgpu provides. > > > what are some examples of that? > > > is the direction that graphics drivers are going, to be simply used as > > > "modesetting" via xorg? > > > > > > On Wed, May 20, 2020 at 10:12 PM Alex Deucher > > > wrote: > > > > > > > > I think you are overcomplicating things. Just try and get X running > > > > on just the AMD GPU on bare metal. Introducing virtualization is just > > > > adding more uncertainty. If you can't configure X to not use the > > > > integrated GPU, just blacklist the i915 driver (append > > > > modprobe.blacklist=i915 to the kernel command line in grub) and X > > > > should come up on the dGPU. > > > > > > > > Alex > > > > > > > > On Wed, May 20, 2020 at 6:05 PM Javad Karabi > > > > wrote: > > > > > > > > > > Thanks Alex, > > > > > Here's my plan: > > > > > > > > > > since my laptop's os is pretty customized, e.g. compiling my own > > > > > kernel, building latest xorg, latest xorg-driver-amdgpu, etc etc, > > > > > im going to use the intel iommu and pass through my rx 5600 into a > > > > > virtual machine, which will be a 100% stock ubuntu installation. > > > > > then, inside that vm, i will continue to debug > > > > > > > > > > does that sound like it would make sense for testing? for example, > > > > > with that scenario, it adds the iommu into the mix, so who knows if > > > > > that causes performance issues. but i think its worth a shot, to see > > > > > if a stock kernel will handle it better > > > > > > > > > > also, quick question: > > > > > from what i understand, a thunderbolt 3 pci express connection should > > > > > handle 8 GT/s x4, however, along the chain of bridges to my device, i > > > > > notice that the bridge closest to the graphics card is at 2.5 GT/s > > > > > x4, and it also says "downgraded" (this is via the lspci output) > > > > > > > > > > now, when i boot into windows, it _also_ says 2.5 GT/s x4, and it > > > > > runs extremely well. no issues at all. > > > > > > > > > > so my question is: the fact that the bridge is at 2.5 GT/s x4, and > > > > > not at its theoretical "full link speed" of 8 GT/s x4, do you suppose > > > > > that _could_ be an issue? > > > > > i do not think so, because, like i said, in windows it also reports > > > > > that link speed. > > > > > i would assume that you would want the fastest link speed possible, > > > > > because i would assume that of _all_ tb3 pci express devices, a GPU > > > > > would be the #1 most demanding on the link > > > > > > > > > > just curious if you think 2.5 GT/s could be the bottleneck > > > > > > > > > > i will pass through the device into a ubuntu vm and let you know how > > > > > it goes. thanks > > > > > > > > > > > > > > > > > > > > On Tue, May 19, 2020 at 9:29 PM Alex Deucher > > > > > wrote: > > > > >> > > > > >> On Tue, May 19, 2020 at 9:16 PM Javad Karabi > > > > >> wrote: > > > > >> > > > > > >> > thanks for the answers
[PATCH] drm/amdgpu/gmc10: program the smallK fragment size
Explicitly set the smallk size to 0 (4k). This is the hw default, but set it anyway just in case something else changed it. Signed-off-by: Alex Deucher --- drivers/gpu/drm/amd/amdgpu/gfxhub_v2_0.c | 4 drivers/gpu/drm/amd/amdgpu/mmhub_v2_0.c | 4 2 files changed, 8 insertions(+) diff --git a/drivers/gpu/drm/amd/amdgpu/gfxhub_v2_0.c b/drivers/gpu/drm/amd/amdgpu/gfxhub_v2_0.c index cc866c367939..6939edfc5232 100644 --- a/drivers/gpu/drm/amd/amdgpu/gfxhub_v2_0.c +++ b/drivers/gpu/drm/amd/amdgpu/gfxhub_v2_0.c @@ -181,6 +181,10 @@ static void gfxhub_v2_0_init_cache_regs(struct amdgpu_device *adev) tmp = REG_SET_FIELD(tmp, GCVM_L2_CNTL4, VMC_TAP_PDE_REQUEST_PHYSICAL, 0); tmp = REG_SET_FIELD(tmp, GCVM_L2_CNTL4, VMC_TAP_PTE_REQUEST_PHYSICAL, 0); WREG32_SOC15(GC, 0, mmGCVM_L2_CNTL4, tmp); + + tmp = mmGCVM_L2_CNTL5_DEFAULT; + tmp = REG_SET_FIELD(tmp, GCVM_L2_CNTL5, L2_CACHE_SMALLK_FRAGMENT_SIZE, 0); + WREG32_SOC15(GC, 0, mmGCVM_L2_CNTL5, tmp); } static void gfxhub_v2_0_enable_system_domain(struct amdgpu_device *adev) diff --git a/drivers/gpu/drm/amd/amdgpu/mmhub_v2_0.c b/drivers/gpu/drm/amd/amdgpu/mmhub_v2_0.c index fb3f228458e5..616309e85d6e 100644 --- a/drivers/gpu/drm/amd/amdgpu/mmhub_v2_0.c +++ b/drivers/gpu/drm/amd/amdgpu/mmhub_v2_0.c @@ -164,6 +164,10 @@ static void mmhub_v2_0_init_cache_regs(struct amdgpu_device *adev) tmp = REG_SET_FIELD(tmp, MMVM_L2_CNTL4, VMC_TAP_PDE_REQUEST_PHYSICAL, 0); tmp = REG_SET_FIELD(tmp, MMVM_L2_CNTL4, VMC_TAP_PTE_REQUEST_PHYSICAL, 0); WREG32_SOC15(MMHUB, 0, mmMMVM_L2_CNTL4, tmp); + + tmp = mmMMVM_L2_CNTL5_DEFAULT; + tmp = REG_SET_FIELD(tmp, MMVM_L2_CNTL5, L2_CACHE_SMALLK_FRAGMENT_SIZE, 0); + WREG32_SOC15(GC, 0, mmMMVM_L2_CNTL5, tmp); } static void mmhub_v2_0_enable_system_domain(struct amdgpu_device *adev) -- 2.25.4 ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: drm/amdkfd: Change pasid's type to unsigned int
Hi, Felix, On Fri, May 22, 2020 at 03:40:06PM -0400, Felix Kuehling wrote: > Hi Fenghua, > > The PASID width in KFD is currently limited to 16 bits. I believe this > reflects what our hardware can handle. KFD will never allocate a PASID > bigger than 16 bits. That said, I'm OK with changing this field in the > kfd_process structure to unsigned int. Generally, I find uint16_t in > structures not very useful except in tightly packed structures such as > packet formats or ioctl arguments. Thank you very much for your insight! I'm writing the patch set to define pasid as "unsigned int" consistently in iommu. I'll put the amdkfd changes (only a few changes including this pasid change in struct kfd_processin) one patch and send it to you for review. -Fenghua ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
drm/amdkfd: Change pasid's type to unsigned int
Hi, Yong, In commit: 6027b1bf6071fc61a5aa11b9922a2e0e91bff1ea drm/amdkfd: Use hex print format for pasid pasid's type was change to "uint16_t" from "unsigned int" in struct kfd_process. But, pasid is a 20-bit value according to PCIe spec and other places in amdkfd (plus other iommu code) define pasid as "unsigned int". If defined as uint16_t, pasid will overflow if its value is bigger than 16-bit. Did I miss anything? Should we change pasid's type back to "unsigned int"? [a little background: pasid is defined as various types including "int", "unsigned int", "u32" in iommu. I'm changing pasid's types to "unsigned int" so that the types are consistent in iommu] Thanks. -Fenghua ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH] drm/amd/amdgpu: Fix the CGCG setting is overwritten for SRIOV.
On Fri, May 22, 2020 at 3:39 PM Gavin Wan wrote:
>
> For SRIOV, since the CP_INT_CNTL_RING0 is programed on host side.
> The Guest should not program CP_INT_CNTL_RING0 again.
>
> Change-Id: Ic336fab3b23b8371c9e9e192182a3ba14a8db8e1
> Signed-off-by: Gavin Wan
Acked-by: Alex Deucher
> ---
> drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c | 7 ++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
> b/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
> index bd5dd4f64311..4d6928cfc269 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
> @@ -4558,7 +4558,12 @@ static void gfx_v10_0_constants_init(struct
> amdgpu_device *adev)
> static void gfx_v10_0_enable_gui_idle_interrupt(struct amdgpu_device *adev,
>bool enable)
> {
> - u32 tmp = RREG32_SOC15(GC, 0, mmCP_INT_CNTL_RING0);
> + u32 tmp;
> +
> + if (amdgpu_sriov_vf(adev))
> + return;
> +
> + tmp = RREG32_SOC15(GC, 0, mmCP_INT_CNTL_RING0);
>
> tmp = REG_SET_FIELD(tmp, CP_INT_CNTL_RING0, CNTX_BUSY_INT_ENABLE,
> enable ? 1 : 0);
> --
> 2.25.1
>
> ___
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH] drm/amdgpu: fix leftover drm_gem_object_put_unlocked call
On Fri, 22 May 2020 at 20:43, Emil Velikov wrote:
>
> On Fri, 22 May 2020 at 20:38, Simon Ser wrote:
> >
> > drm_gem_object_put_unlocked has been renamed to drm_gem_object_put.
> >
> > Signed-off-by: Simon Ser
> > Fixes: e07ddb0ce7cd ("drm/amd: remove _unlocked suffix in
> > drm_gem_object_put_unlocked")
> Wrong tag it seems. At that commit, the amdgpu code uses it's own
> wrapper - amdgpu_bo_unref()
>
Alex, team, this seems like merge clash.
With the Fixes tag dropped, the patch is:
Reviewed-by: Emil Velikov
-Emil
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH] drm/amdgpu: fix leftover drm_gem_object_put_unlocked call
On Fri, 22 May 2020 at 20:38, Simon Ser wrote:
>
> drm_gem_object_put_unlocked has been renamed to drm_gem_object_put.
>
> Signed-off-by: Simon Ser
> Fixes: e07ddb0ce7cd ("drm/amd: remove _unlocked suffix in
> drm_gem_object_put_unlocked")
Wrong tag it seems. At that commit, the amdgpu code uses it's own
wrapper - amdgpu_bo_unref()
-Emil
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH] drm/amdgpu: fix leftover drm_gem_object_put_unlocked call
Am 2020-05-22 um 3:38 p.m. schrieb Simon Ser:
> drm_gem_object_put_unlocked has been renamed to drm_gem_object_put.
Alex, I guess you'll need to apply this patch when you include
e07ddb0ce7cd in a pull request to Dave Airlie. I don't think it makes
sense to apply this on amd-kfd-staging until the branch gets rebased again.
Regards,
Felix
>
> Signed-off-by: Simon Ser
> Fixes: e07ddb0ce7cd ("drm/amd: remove _unlocked suffix in
> drm_gem_object_put_unlocked")
> Cc: Alex Deucher
> Cc: Christian König
> Cc: David (ChunMing) Zhou
> Cc: Emil Velikov
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> index da8b31a53291..c99fb92ae991 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> @@ -1354,7 +1354,7 @@ int amdgpu_amdkfd_gpuvm_free_memory_of_gpu(
> }
>
> /* Free the BO*/
> - drm_gem_object_put_unlocked(>bo->tbo.base);
> + drm_gem_object_put(>bo->tbo.base);
> mutex_destroy(>lock);
> kfree(mem);
>
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
RE: [PATCH] drm/amd/amdgpu: Fix the CGCG setting is overwritten for SRIOV.
HI Alex,
I fixed it as your suggestion.
Thanks,
Gavin
-Original Message-
From: Alex Deucher
Sent: Friday, May 22, 2020 3:11 PM
To: Wan, Gavin
Cc: amd-gfx list
Subject: Re: [PATCH] drm/amd/amdgpu: Fix the CGCG setting is overwritten for
SRIOV.
On Fri, May 22, 2020 at 2:20 PM Gavin Wan wrote:
>
> For SRIOV, since the CP_INT_CNTL_RING0 is programed on host side.
> The Guest should not program CP_INT_CNTL_RING0 again.
>
> Change-Id: Ic336fab3b23b8371c9e9e192182a3ba14a8db8e1
> Signed-off-by: Gavin Wan
> ---
> drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
> b/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
> index bd5dd4f64311..39275bf79448 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
> @@ -4558,6 +4558,9 @@ static void gfx_v10_0_constants_init(struct
> amdgpu_device *adev) static void gfx_v10_0_enable_gui_idle_interrupt(struct
> amdgpu_device *adev,
>bool enable) {
> + if (amdgpu_sriov_vf(adev))
> + return;
> +
This needs to be below the stack variable declarations or you'll get a warning.
Alex
> u32 tmp = RREG32_SOC15(GC, 0, mmCP_INT_CNTL_RING0);
>
> tmp = REG_SET_FIELD(tmp, CP_INT_CNTL_RING0,
> CNTX_BUSY_INT_ENABLE,
> --
> 2.25.1
>
> ___
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flist
> s.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfxdata=02%7C01%7CGa
> vin.Wan%40amd.com%7C23c1770b248841c7032a08d7fe83d940%7C3dd8961fe4884e6
> 08e11a82d994e183d%7C0%7C0%7C637257714548695750sdata=PaWH5hLNb3N1Z
> lalw4GsjeeB46xzCVxXDWBROndcKsk%3Dreserved=0
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: drm/amdkfd: Change pasid's type to unsigned int
Hi Fenghua, The PASID width in KFD is currently limited to 16 bits. I believe this reflects what our hardware can handle. KFD will never allocate a PASID bigger than 16 bits. That said, I'm OK with changing this field in the kfd_process structure to unsigned int. Generally, I find uint16_t in structures not very useful except in tightly packed structures such as packet formats or ioctl arguments. Regards, Felix Am 2020-05-22 um 3:25 p.m. schrieb Fenghua Yu: > Hi, Yong, > > In commit: 6027b1bf6071fc61a5aa11b9922a2e0e91bff1ea > drm/amdkfd: Use hex print format for pasid > > pasid's type was change to "uint16_t" from "unsigned int" in > struct kfd_process. > > But, pasid is a 20-bit value according to PCIe spec and other places > in amdkfd (plus other iommu code) define pasid as "unsigned int". > If defined as uint16_t, pasid will overflow if its value is bigger than > 16-bit. > > Did I miss anything? Should we change pasid's type back to "unsigned int"? > > [a little background: pasid is defined as various types including "int", > "unsigned int", "u32" in iommu. I'm changing pasid's types to "unsigned int" > so that the types are consistent in iommu] > > Thanks. > > -Fenghua ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH] drm/amd/amdgpu: Fix the CGCG setting is overwritten for SRIOV.
For SRIOV, since the CP_INT_CNTL_RING0 is programed on host side.
The Guest should not program CP_INT_CNTL_RING0 again.
Change-Id: Ic336fab3b23b8371c9e9e192182a3ba14a8db8e1
Signed-off-by: Gavin Wan
---
drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c | 7 ++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
b/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
index bd5dd4f64311..4d6928cfc269 100644
--- a/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
@@ -4558,7 +4558,12 @@ static void gfx_v10_0_constants_init(struct
amdgpu_device *adev)
static void gfx_v10_0_enable_gui_idle_interrupt(struct amdgpu_device *adev,
bool enable)
{
- u32 tmp = RREG32_SOC15(GC, 0, mmCP_INT_CNTL_RING0);
+ u32 tmp;
+
+ if (amdgpu_sriov_vf(adev))
+ return;
+
+ tmp = RREG32_SOC15(GC, 0, mmCP_INT_CNTL_RING0);
tmp = REG_SET_FIELD(tmp, CP_INT_CNTL_RING0, CNTX_BUSY_INT_ENABLE,
enable ? 1 : 0);
--
2.25.1
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH] drm/amdgpu: fix leftover drm_gem_object_put_unlocked call
drm_gem_object_put_unlocked has been renamed to drm_gem_object_put.
Signed-off-by: Simon Ser
Fixes: e07ddb0ce7cd ("drm/amd: remove _unlocked suffix in
drm_gem_object_put_unlocked")
Cc: Alex Deucher
Cc: Christian König
Cc: David (ChunMing) Zhou
Cc: Emil Velikov
---
drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
index da8b31a53291..c99fb92ae991 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
@@ -1354,7 +1354,7 @@ int amdgpu_amdkfd_gpuvm_free_memory_of_gpu(
}
/* Free the BO*/
- drm_gem_object_put_unlocked(>bo->tbo.base);
+ drm_gem_object_put(>bo->tbo.base);
mutex_destroy(>lock);
kfree(mem);
--
2.26.2
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
RE: [PATCH] drm/amd/amdgpu: Fix the CGCG setting is overwritten for SRIOV.
Fixed it as Monk and Hawking suggestion. Now it only has one checking in
function gfx_v10_0_enable_gui_idle_interrupt.
BTW, I update the commit, but it send out an another email.
Thanks,
Gavin
-Original Message-
From: Zhang, Hawking
Sent: Friday, May 22, 2020 2:17 AM
To: Liu, Monk ; Chen, Guchun ; Wan,
Gavin ; amd-gfx@lists.freedesktop.org
Cc: Wan, Gavin
Subject: RE: [PATCH] drm/amd/amdgpu: Fix the CGCG setting is overwritten for
SRIOV.
[AMD Public Use]
Or make it in more reasonable place.
Regards,
Hawking
-Original Message-
From: Zhang, Hawking
Sent: Friday, May 22, 2020 14:16
To: Liu, Monk ; Chen, Guchun ; Wan,
Gavin ; amd-gfx@lists.freedesktop.org
Cc: Wan, Gavin
Subject: RE: [PATCH] drm/amd/amdgpu: Fix the CGCG setting is overwritten for
SRIOV.
[AMD Public Use]
Yes, please try best effort to not introduce guest/one_vf/mult_vf check.
Regards,
Hawking
-Original Message-
From: Liu, Monk
Sent: Friday, May 22, 2020 14:12
To: Liu, Monk ; Zhang, Hawking ; Chen,
Guchun ; Wan, Gavin ;
amd-gfx@lists.freedesktop.org
Cc: Wan, Gavin
Subject: RE: [PATCH] drm/amd/amdgpu: Fix the CGCG setting is overwritten for
SRIOV.
Gavin
Looks the only place you need to change is the part of avoid touching
"CP_INT_CNTL_RING0" which is handled by GIM now
Others looks not needed at all
_
Monk Liu|GPU Virtualization Team |AMD
-Original Message-
From: amd-gfx On Behalf Of Liu, Monk
Sent: Friday, May 22, 2020 1:52 PM
To: Zhang, Hawking ; Chen, Guchun ;
Wan, Gavin ; amd-gfx@lists.freedesktop.org
Cc: Wan, Gavin
Subject: RE: [PATCH] drm/amd/amdgpu: Fix the CGCG setting is overwritten for
SRIOV.
Sounds a good idea
@Wan, Gavin can you try hawking's advice ?
_
Monk Liu|GPU Virtualization Team |AMD
-Original Message-
From: amd-gfx On Behalf Of Zhang,
Hawking
Sent: Friday, May 22, 2020 1:09 PM
To: Chen, Guchun ; Wan, Gavin ;
amd-gfx@lists.freedesktop.org
Cc: Wan, Gavin
Subject: RE: [PATCH] drm/amd/amdgpu: Fix the CGCG setting is overwritten for
SRIOV.
[AMD Public Use]
Can we leverage existing CG flags to control this rather than add
amdgpu_sriov_vf(adev) check everywhere?
If GC CG feature is programmed by host. We can just mask out the following
flags for guest driver case (amdgpu_sriov_vf(adev)).
AMD_CG_SUPPORT_GFX_MGCG |
AMD_CG_SUPPORT_GFX_CGLS |
AMD_CG_SUPPORT_GFX_CGCG |
AMD_CG_SUPPORT_GFX_CGLS |
AMD_CG_SUPPORT_GFX_3D_CGCG |
AMD_CG_SUPPORT_GFX_3D_CGLS
There are too many amdgpu_sriov_vf(adev) Check in amdgpu driver, which actually
add unnecessary sustaining effort.
Regards,
Hawking
-Original Message-
From: amd-gfx On Behalf Of Chen, Guchun
Sent: Friday, May 22, 2020 11:47
To: Wan, Gavin ; amd-gfx@lists.freedesktop.org
Cc: Wan, Gavin
Subject: RE: [PATCH] drm/amd/amdgpu: Fix the CGCG setting is overwritten for
SRIOV.
[AMD Public Use]
Please see one comment below.
Regards,
Guchun
-Original Message-
From: amd-gfx On Behalf Of Gavin Wan
Sent: Friday, May 22, 2020 3:53 AM
To: amd-gfx@lists.freedesktop.org
Cc: Wan, Gavin
Subject: [PATCH] drm/amd/amdgpu: Fix the CGCG setting is overwritten for SRIOV.
For SRIOV, since the CGCG is set on host side. The Guest should not program
CGCG again.
The patch ignores setting CGCG for SRIOV.
Change-Id: Ic336fab3b23b8371c9e9e192182a3ba14a8db8e1
Signed-off-by: Gavin Wan
---
drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c | 15 +++
1 file changed, 15 insertions(+)
diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
b/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
index bd5dd4f64311..52b6e4759cf3 100644
--- a/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
@@ -4558,6 +4558,9 @@ static void gfx_v10_0_constants_init(struct amdgpu_device
*adev) static void gfx_v10_0_enable_gui_idle_interrupt(struct amdgpu_device
*adev,
bool enable)
{
+ if (amdgpu_sriov_vf(adev))
+ return;
+
[Guchun]This coding style is not correct. You should put the check after the
declare of 'u32 tmp'.
Maybe it's better to split below line to declare and execution parts
respectively.
u32 tmp = RREG32_SOC15(GC, 0, mmCP_INT_CNTL_RING0);
tmp = REG_SET_FIELD(tmp, CP_INT_CNTL_RING0, CNTX_BUSY_INT_ENABLE, @@
-6842,6 +6845,9 @@ static void
gfx_v10_0_update_medium_grain_clock_gating(struct amdgpu_device *ade {
uint32_t data, def;
+ if (amdgpu_sriov_vf(adev))
+ return;
+
/* It is disabled by HW by default */
if (enable && (adev->cg_flags & AMD_CG_SUPPORT_GFX_MGCG)) {
/* 0 - Disable some blocks' MGCG */
@@ -6911,6 +6917,9 @@ static void gfx_v10_0_update_3d_clock_gating(struct
amdgpu_device *adev, {
uint32_t data, def;
+ if (amdgpu_sriov_vf(adev))
+ return;
+
/* Enable 3D
Re: [PATCH] drm/amd/display: Make BREAK_TO_DEBUGGER() a debug print
On 2020-05-22 2:03 p.m., Nicholas Kazlauskas wrote:
> [Why]
> Warnings in the kernel are generally treated as errors.
>
> The BREAK_TO_DEBUGGER macro is not a critical error or warning, but
> rather intended for developer use to help investigate behavior and
> sequences for other issues.
>
> We do still make use of DC_ERROR/ASSERT(0) in various places in the
> code for things that are genuine issues.
>
> Since most developers don't actually KGDB while debugging the kernel
> these essentially would have no value on their own since the KGDB
> breakpoint wouldn't trigger - ASSERT(0) was used as a shortcut to get
> a stacktrace.
>
> [How]
> Turn it into a DRM_DEBUG_DRIVER print instead. We unfortunately lose
> the stacktrace, but we still do retain some of the useful debug
> information this offers by having at least the function and line
> number loggable.
>
> If KGDB is supported in the kernel this will still trigger a real
> breakpoint as well.
>
> Cc: Harry Wentland
> Cc: Leo Li
> Cc: Bhawanpreet Lakha
> Cc: Rodrigo Siqueira
> Signed-off-by: Nicholas Kazlauskas
Reviewed-by: Harry Wentland
Harry
> ---
> drivers/gpu/drm/amd/display/dc/os_types.h | 10 +-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/display/dc/os_types.h
> b/drivers/gpu/drm/amd/display/dc/os_types.h
> index 6d7bca562eec..604ceb6c0375 100644
> --- a/drivers/gpu/drm/amd/display/dc/os_types.h
> +++ b/drivers/gpu/drm/amd/display/dc/os_types.h
> @@ -111,7 +111,15 @@
> #define ASSERT(expr) WARN_ON_ONCE(!(expr))
> #endif
>
> -#define BREAK_TO_DEBUGGER() ASSERT(0)
> +#if defined(CONFIG_HAVE_KGDB) || defined(CONFIG_KGDB)
> +#define BREAK_TO_DEBUGGER() \
> + do { \
> + DRM_DEBUG_DRIVER("%s():%d\n", __func__, __LINE__); \
> + kgdb_breakpoint(); \
> + } while (0)
> +#else
> +#define BREAK_TO_DEBUGGER() DRM_DEBUG_DRIVER("%s():%d\n", __func__, __LINE__)
> +#endif
>
> #define DC_ERR(...) do { \
> dm_error(__VA_ARGS__); \
>
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH] drm/amd/amdgpu: Fix the CGCG setting is overwritten for SRIOV.
For SRIOV, since the CP_INT_CNTL_RING0 is programed on host side.
The Guest should not program CP_INT_CNTL_RING0 again.
Change-Id: Ic336fab3b23b8371c9e9e192182a3ba14a8db8e1
Signed-off-by: Gavin Wan
---
drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
b/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
index bd5dd4f64311..39275bf79448 100644
--- a/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
@@ -4558,6 +4558,9 @@ static void gfx_v10_0_constants_init(struct amdgpu_device
*adev)
static void gfx_v10_0_enable_gui_idle_interrupt(struct amdgpu_device *adev,
bool enable)
{
+ if (amdgpu_sriov_vf(adev))
+ return;
+
u32 tmp = RREG32_SOC15(GC, 0, mmCP_INT_CNTL_RING0);
tmp = REG_SET_FIELD(tmp, CP_INT_CNTL_RING0, CNTX_BUSY_INT_ENABLE,
--
2.25.1
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH] drm/[radeon|amdgpu]: Replace one-element array and use struct_size() helper
On Fri, May 22, 2020 at 1:39 PM Gustavo A. R. Silva
wrote:
>
> The current codebase makes use of one-element arrays in the following
> form:
>
> struct something {
> int length;
> u8 data[1];
> };
>
> struct something *instance;
>
> instance = kmalloc(sizeof(*instance) + size, GFP_KERNEL);
> instance->length = size;
> memcpy(instance->data, source, size);
>
> but the preferred mechanism to declare variable-length types such as
> these ones is a flexible array member[1][2], introduced in C99:
>
> struct foo {
> int stuff;
> struct boo array[];
> };
>
> By making use of the mechanism above, we will get a compiler warning
> in case the flexible array does not occur last in the structure, which
> will help us prevent some kind of undefined behavior bugs from being
> inadvertently introduced[3] to the codebase from now on. So, replace
> the one-element array with a flexible-array member.
>
> Also, make use of the new struct_size() helper to properly calculate the
> size of struct SISLANDS_SMC_SWSTATE.
>
> This issue was found with the help of Coccinelle and, audited and fixed
> _manually_.
>
> [1] https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html
> [2] https://github.com/KSPP/linux/issues/21
> [3] commit 76497732932f ("cxgb3/l2t: Fix undefined behaviour")
>
> Signed-off-by: Gustavo A. R. Silva
Applied. Thanks!
Alex
> ---
> drivers/gpu/drm/amd/amdgpu/si_dpm.c | 5 ++---
> drivers/gpu/drm/amd/amdgpu/sislands_smc.h | 2 +-
> drivers/gpu/drm/radeon/si_dpm.c | 5 ++---
> 3 files changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/si_dpm.c
> b/drivers/gpu/drm/amd/amdgpu/si_dpm.c
> index c00ba4b23c9a6..0fc56c5bac080 100644
> --- a/drivers/gpu/drm/amd/amdgpu/si_dpm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/si_dpm.c
> @@ -5715,10 +5715,9 @@ static int si_upload_sw_state(struct amdgpu_device
> *adev,
> int ret;
> u32 address = si_pi->state_table_start +
> offsetof(SISLANDS_SMC_STATETABLE, driverState);
> - u32 state_size = sizeof(SISLANDS_SMC_SWSTATE) +
> - ((new_state->performance_level_count - 1) *
> -sizeof(SISLANDS_SMC_HW_PERFORMANCE_LEVEL));
> SISLANDS_SMC_SWSTATE *smc_state = _pi->smc_statetable.driverState;
> + size_t state_size = struct_size(smc_state, levels,
> + new_state->performance_level_count);
>
> memset(smc_state, 0, state_size);
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/sislands_smc.h
> b/drivers/gpu/drm/amd/amdgpu/sislands_smc.h
> index d2930eceaf3c8..a089dbf8f7a93 100644
> --- a/drivers/gpu/drm/amd/amdgpu/sislands_smc.h
> +++ b/drivers/gpu/drm/amd/amdgpu/sislands_smc.h
> @@ -186,7 +186,7 @@ struct SISLANDS_SMC_SWSTATE
> uint8_t levelCount;
> uint8_t padding2;
> uint8_t padding3;
> -SISLANDS_SMC_HW_PERFORMANCE_LEVEL levels[1];
> +SISLANDS_SMC_HW_PERFORMANCE_LEVEL levels[];
> };
>
> typedef struct SISLANDS_SMC_SWSTATE SISLANDS_SMC_SWSTATE;
> diff --git a/drivers/gpu/drm/radeon/si_dpm.c b/drivers/gpu/drm/radeon/si_dpm.c
> index a167e1c36d243..bab01ca864c63 100644
> --- a/drivers/gpu/drm/radeon/si_dpm.c
> +++ b/drivers/gpu/drm/radeon/si_dpm.c
> @@ -5253,10 +5253,9 @@ static int si_upload_sw_state(struct radeon_device
> *rdev,
> int ret;
> u32 address = si_pi->state_table_start +
> offsetof(SISLANDS_SMC_STATETABLE, driverState);
> - u32 state_size = sizeof(SISLANDS_SMC_SWSTATE) +
> - ((new_state->performance_level_count - 1) *
> -sizeof(SISLANDS_SMC_HW_PERFORMANCE_LEVEL));
> SISLANDS_SMC_SWSTATE *smc_state = _pi->smc_statetable.driverState;
> + size_t state_size = struct_size(smc_state, levels,
> + new_state->performance_level_count);
>
> memset(smc_state, 0, state_size);
>
> --
> 2.26.2
>
> ___
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v2] drm/radeon/dpm: Replace one-element array and use struct_size() helper
On Fri, May 22, 2020 at 1:29 PM Gustavo A. R. Silva
wrote:
>
> The current codebase makes use of one-element arrays in the following
> form:
>
> struct something {
> int length;
> u8 data[1];
> };
>
> struct something *instance;
>
> instance = kmalloc(sizeof(*instance) + size, GFP_KERNEL);
> instance->length = size;
> memcpy(instance->data, source, size);
>
> but the preferred mechanism to declare variable-length types such as
> these ones is a flexible array member[1][2], introduced in C99:
>
> struct foo {
> int stuff;
> struct boo array[];
> };
>
> By making use of the mechanism above, we will get a compiler warning
> in case the flexible array does not occur last in the structure, which
> will help us prevent some kind of undefined behavior bugs from being
> inadvertently introduced[3] to the codebase from now on. So, replace
> the one-element array with a flexible-array member.
>
> Also, make use of the new struct_size() helper to properly calculate the
> size of struct NISLANDS_SMC_SWSTATE.
>
> This issue was found with the help of Coccinelle and, audited and fixed
> _manually_.
>
> [1] https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html
> [2] https://github.com/KSPP/linux/issues/21
> [3] commit 76497732932f ("cxgb3/l2t: Fix undefined behaviour")
>
> Reviewed-by: Christian König
> Signed-off-by: Gustavo A. R. Silva
Applied. Thanks!
Alex
> ---
> Changes in v2:
> - Use type size_t instead of u16 for state_size variable.
>
> drivers/gpu/drm/amd/amdgpu/si_dpm.h | 2 +-
> drivers/gpu/drm/radeon/ni_dpm.c | 7 ---
> 2 files changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/si_dpm.h
> b/drivers/gpu/drm/amd/amdgpu/si_dpm.h
> index 6b7d292b919f3..bc0be6818e218 100644
> --- a/drivers/gpu/drm/amd/amdgpu/si_dpm.h
> +++ b/drivers/gpu/drm/amd/amdgpu/si_dpm.h
> @@ -781,7 +781,7 @@ struct NISLANDS_SMC_SWSTATE
> uint8_t levelCount;
> uint8_t padding2;
> uint8_t padding3;
> -NISLANDS_SMC_HW_PERFORMANCE_LEVEL levels[1];
> +NISLANDS_SMC_HW_PERFORMANCE_LEVEL levels[];
> };
>
> typedef struct NISLANDS_SMC_SWSTATE NISLANDS_SMC_SWSTATE;
> diff --git a/drivers/gpu/drm/radeon/ni_dpm.c b/drivers/gpu/drm/radeon/ni_dpm.c
> index b57c37ddd164c..abb6345bfae32 100644
> --- a/drivers/gpu/drm/radeon/ni_dpm.c
> +++ b/drivers/gpu/drm/radeon/ni_dpm.c
> @@ -2685,11 +2685,12 @@ static int ni_upload_sw_state(struct radeon_device
> *rdev,
> struct rv7xx_power_info *pi = rv770_get_pi(rdev);
> u16 address = pi->state_table_start +
> offsetof(NISLANDS_SMC_STATETABLE, driverState);
> - u16 state_size = sizeof(NISLANDS_SMC_SWSTATE) +
> - ((NISLANDS_MAX_SMC_PERFORMANCE_LEVELS_PER_SWSTATE - 1) *
> sizeof(NISLANDS_SMC_HW_PERFORMANCE_LEVEL));
> + NISLANDS_SMC_SWSTATE *smc_state;
> + size_t state_size = struct_size(smc_state, levels,
> + NISLANDS_MAX_SMC_PERFORMANCE_LEVELS_PER_SWSTATE);
> int ret;
> - NISLANDS_SMC_SWSTATE *smc_state = kzalloc(state_size, GFP_KERNEL);
>
> + smc_state = kzalloc(state_size, GFP_KERNEL);
> if (smc_state == NULL)
> return -ENOMEM;
>
> --
> 2.26.2
>
> ___
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH] drm/amd/display: Make BREAK_TO_DEBUGGER() a debug print
[Why]
Warnings in the kernel are generally treated as errors.
The BREAK_TO_DEBUGGER macro is not a critical error or warning, but
rather intended for developer use to help investigate behavior and
sequences for other issues.
We do still make use of DC_ERROR/ASSERT(0) in various places in the
code for things that are genuine issues.
Since most developers don't actually KGDB while debugging the kernel
these essentially would have no value on their own since the KGDB
breakpoint wouldn't trigger - ASSERT(0) was used as a shortcut to get
a stacktrace.
[How]
Turn it into a DRM_DEBUG_DRIVER print instead. We unfortunately lose
the stacktrace, but we still do retain some of the useful debug
information this offers by having at least the function and line
number loggable.
If KGDB is supported in the kernel this will still trigger a real
breakpoint as well.
Cc: Harry Wentland
Cc: Leo Li
Cc: Bhawanpreet Lakha
Cc: Rodrigo Siqueira
Signed-off-by: Nicholas Kazlauskas
---
drivers/gpu/drm/amd/display/dc/os_types.h | 10 +-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/display/dc/os_types.h
b/drivers/gpu/drm/amd/display/dc/os_types.h
index 6d7bca562eec..604ceb6c0375 100644
--- a/drivers/gpu/drm/amd/display/dc/os_types.h
+++ b/drivers/gpu/drm/amd/display/dc/os_types.h
@@ -111,7 +111,15 @@
#define ASSERT(expr) WARN_ON_ONCE(!(expr))
#endif
-#define BREAK_TO_DEBUGGER() ASSERT(0)
+#if defined(CONFIG_HAVE_KGDB) || defined(CONFIG_KGDB)
+#define BREAK_TO_DEBUGGER() \
+ do { \
+ DRM_DEBUG_DRIVER("%s():%d\n", __func__, __LINE__); \
+ kgdb_breakpoint(); \
+ } while (0)
+#else
+#define BREAK_TO_DEBUGGER() DRM_DEBUG_DRIVER("%s():%d\n", __func__, __LINE__)
+#endif
#define DC_ERR(...) do { \
dm_error(__VA_ARGS__); \
--
2.17.1
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH] drm/amdgpu/smu10: Replace one-element array and use struct_size() helper
On Fri, May 22, 2020 at 1:46 PM Gustavo A. R. Silva wrote: > > On Wed, May 20, 2020 at 09:42:27AM +0200, Christian König wrote: > > > > > > Signed-off-by: Gustavo A. R. Silva > > > > Acked-by: Christian König > > > > May I suggest that we add a section how to correctly do this to > > Documentation/process/coding-style.rst or similar document? > > > > That's already on my list. :) > > > I've seen a bunch of different approaches and some even doesn't work with > > some gcc versions and result in a broken binary. > > > > Do you have an example of that one that doesn't work with some GCC > versions? It'd be interesting to take a look... https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/gpu/drm/radeon/radeon_atombios.c?id=a7ee824a6255e347ea76e2f00827e81bbe01004e Alex > > Thanks > -- > Gustavo > ___ > amd-gfx mailing list > amd-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/amd-gfx ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH] drm/amdgpu/smu10: Replace one-element array and use struct_size() helper
On Wed, May 20, 2020 at 09:42:27AM +0200, Christian König wrote: > > > > Signed-off-by: Gustavo A. R. Silva > > Acked-by: Christian König > > May I suggest that we add a section how to correctly do this to > Documentation/process/coding-style.rst or similar document? > That's already on my list. :) > I've seen a bunch of different approaches and some even doesn't work with > some gcc versions and result in a broken binary. > Do you have an example of that one that doesn't work with some GCC versions? It'd be interesting to take a look... Thanks -- Gustavo ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH] drm/[radeon|amdgpu]: Replace one-element array and use struct_size() helper
The current codebase makes use of one-element arrays in the following
form:
struct something {
int length;
u8 data[1];
};
struct something *instance;
instance = kmalloc(sizeof(*instance) + size, GFP_KERNEL);
instance->length = size;
memcpy(instance->data, source, size);
but the preferred mechanism to declare variable-length types such as
these ones is a flexible array member[1][2], introduced in C99:
struct foo {
int stuff;
struct boo array[];
};
By making use of the mechanism above, we will get a compiler warning
in case the flexible array does not occur last in the structure, which
will help us prevent some kind of undefined behavior bugs from being
inadvertently introduced[3] to the codebase from now on. So, replace
the one-element array with a flexible-array member.
Also, make use of the new struct_size() helper to properly calculate the
size of struct SISLANDS_SMC_SWSTATE.
This issue was found with the help of Coccinelle and, audited and fixed
_manually_.
[1] https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html
[2] https://github.com/KSPP/linux/issues/21
[3] commit 76497732932f ("cxgb3/l2t: Fix undefined behaviour")
Signed-off-by: Gustavo A. R. Silva
---
drivers/gpu/drm/amd/amdgpu/si_dpm.c | 5 ++---
drivers/gpu/drm/amd/amdgpu/sislands_smc.h | 2 +-
drivers/gpu/drm/radeon/si_dpm.c | 5 ++---
3 files changed, 5 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/si_dpm.c
b/drivers/gpu/drm/amd/amdgpu/si_dpm.c
index c00ba4b23c9a6..0fc56c5bac080 100644
--- a/drivers/gpu/drm/amd/amdgpu/si_dpm.c
+++ b/drivers/gpu/drm/amd/amdgpu/si_dpm.c
@@ -5715,10 +5715,9 @@ static int si_upload_sw_state(struct amdgpu_device *adev,
int ret;
u32 address = si_pi->state_table_start +
offsetof(SISLANDS_SMC_STATETABLE, driverState);
- u32 state_size = sizeof(SISLANDS_SMC_SWSTATE) +
- ((new_state->performance_level_count - 1) *
-sizeof(SISLANDS_SMC_HW_PERFORMANCE_LEVEL));
SISLANDS_SMC_SWSTATE *smc_state = _pi->smc_statetable.driverState;
+ size_t state_size = struct_size(smc_state, levels,
+ new_state->performance_level_count);
memset(smc_state, 0, state_size);
diff --git a/drivers/gpu/drm/amd/amdgpu/sislands_smc.h
b/drivers/gpu/drm/amd/amdgpu/sislands_smc.h
index d2930eceaf3c8..a089dbf8f7a93 100644
--- a/drivers/gpu/drm/amd/amdgpu/sislands_smc.h
+++ b/drivers/gpu/drm/amd/amdgpu/sislands_smc.h
@@ -186,7 +186,7 @@ struct SISLANDS_SMC_SWSTATE
uint8_t levelCount;
uint8_t padding2;
uint8_t padding3;
-SISLANDS_SMC_HW_PERFORMANCE_LEVEL levels[1];
+SISLANDS_SMC_HW_PERFORMANCE_LEVEL levels[];
};
typedef struct SISLANDS_SMC_SWSTATE SISLANDS_SMC_SWSTATE;
diff --git a/drivers/gpu/drm/radeon/si_dpm.c b/drivers/gpu/drm/radeon/si_dpm.c
index a167e1c36d243..bab01ca864c63 100644
--- a/drivers/gpu/drm/radeon/si_dpm.c
+++ b/drivers/gpu/drm/radeon/si_dpm.c
@@ -5253,10 +5253,9 @@ static int si_upload_sw_state(struct radeon_device *rdev,
int ret;
u32 address = si_pi->state_table_start +
offsetof(SISLANDS_SMC_STATETABLE, driverState);
- u32 state_size = sizeof(SISLANDS_SMC_SWSTATE) +
- ((new_state->performance_level_count - 1) *
-sizeof(SISLANDS_SMC_HW_PERFORMANCE_LEVEL));
SISLANDS_SMC_SWSTATE *smc_state = _pi->smc_statetable.driverState;
+ size_t state_size = struct_size(smc_state, levels,
+ new_state->performance_level_count);
memset(smc_state, 0, state_size);
--
2.26.2
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH v2] drm/radeon/dpm: Replace one-element array and use struct_size() helper
The current codebase makes use of one-element arrays in the following
form:
struct something {
int length;
u8 data[1];
};
struct something *instance;
instance = kmalloc(sizeof(*instance) + size, GFP_KERNEL);
instance->length = size;
memcpy(instance->data, source, size);
but the preferred mechanism to declare variable-length types such as
these ones is a flexible array member[1][2], introduced in C99:
struct foo {
int stuff;
struct boo array[];
};
By making use of the mechanism above, we will get a compiler warning
in case the flexible array does not occur last in the structure, which
will help us prevent some kind of undefined behavior bugs from being
inadvertently introduced[3] to the codebase from now on. So, replace
the one-element array with a flexible-array member.
Also, make use of the new struct_size() helper to properly calculate the
size of struct NISLANDS_SMC_SWSTATE.
This issue was found with the help of Coccinelle and, audited and fixed
_manually_.
[1] https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html
[2] https://github.com/KSPP/linux/issues/21
[3] commit 76497732932f ("cxgb3/l2t: Fix undefined behaviour")
Reviewed-by: Christian König
Signed-off-by: Gustavo A. R. Silva
---
Changes in v2:
- Use type size_t instead of u16 for state_size variable.
drivers/gpu/drm/amd/amdgpu/si_dpm.h | 2 +-
drivers/gpu/drm/radeon/ni_dpm.c | 7 ---
2 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/si_dpm.h
b/drivers/gpu/drm/amd/amdgpu/si_dpm.h
index 6b7d292b919f3..bc0be6818e218 100644
--- a/drivers/gpu/drm/amd/amdgpu/si_dpm.h
+++ b/drivers/gpu/drm/amd/amdgpu/si_dpm.h
@@ -781,7 +781,7 @@ struct NISLANDS_SMC_SWSTATE
uint8_t levelCount;
uint8_t padding2;
uint8_t padding3;
-NISLANDS_SMC_HW_PERFORMANCE_LEVEL levels[1];
+NISLANDS_SMC_HW_PERFORMANCE_LEVEL levels[];
};
typedef struct NISLANDS_SMC_SWSTATE NISLANDS_SMC_SWSTATE;
diff --git a/drivers/gpu/drm/radeon/ni_dpm.c b/drivers/gpu/drm/radeon/ni_dpm.c
index b57c37ddd164c..abb6345bfae32 100644
--- a/drivers/gpu/drm/radeon/ni_dpm.c
+++ b/drivers/gpu/drm/radeon/ni_dpm.c
@@ -2685,11 +2685,12 @@ static int ni_upload_sw_state(struct radeon_device
*rdev,
struct rv7xx_power_info *pi = rv770_get_pi(rdev);
u16 address = pi->state_table_start +
offsetof(NISLANDS_SMC_STATETABLE, driverState);
- u16 state_size = sizeof(NISLANDS_SMC_SWSTATE) +
- ((NISLANDS_MAX_SMC_PERFORMANCE_LEVELS_PER_SWSTATE - 1) *
sizeof(NISLANDS_SMC_HW_PERFORMANCE_LEVEL));
+ NISLANDS_SMC_SWSTATE *smc_state;
+ size_t state_size = struct_size(smc_state, levels,
+ NISLANDS_MAX_SMC_PERFORMANCE_LEVELS_PER_SWSTATE);
int ret;
- NISLANDS_SMC_SWSTATE *smc_state = kzalloc(state_size, GFP_KERNEL);
+ smc_state = kzalloc(state_size, GFP_KERNEL);
if (smc_state == NULL)
return -ENOMEM;
--
2.26.2
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH] drm/radeon/dpm: Replace one-element array and use struct_size() helper
On Fri, May 22, 2020 at 09:00:09AM +0200, Christian König wrote: > > +++ b/drivers/gpu/drm/radeon/ni_dpm.c > > @@ -2685,11 +2685,12 @@ static int ni_upload_sw_state(struct radeon_device > > *rdev, > > struct rv7xx_power_info *pi = rv770_get_pi(rdev); > > u16 address = pi->state_table_start + > > offsetof(NISLANDS_SMC_STATETABLE, driverState); > > - u16 state_size = sizeof(NISLANDS_SMC_SWSTATE) + > > - ((NISLANDS_MAX_SMC_PERFORMANCE_LEVELS_PER_SWSTATE - 1) * > > sizeof(NISLANDS_SMC_HW_PERFORMANCE_LEVEL)); > > + NISLANDS_SMC_SWSTATE *smc_state; > > + u16 state_size = struct_size(smc_state, levels, > > + NISLANDS_MAX_SMC_PERFORMANCE_LEVELS_PER_SWSTATE); > > Probably better to use size_t instead of u16 here. With that fixed feel free > to stick my rb on the patch. > Sure thing. I'll send v2, shortly. Thanks, Christian. -- Gustavo ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v2] drm/amdgpu: fix device attribute node create failed with multi gpu
On Fri, May 22, 2020 at 11:41 AM Kevin Wang wrote:
>
> the origin design will use varible of "attr->states" to save node
> supported states on current gpu device, but for multi gpu device, when
> probe second gpu device, the driver will check attribute node states
> from previous gpu device wthether to create attribute node.
> it will cause other gpu device create attribute node faild.
>
> 1. add member attr_list into amdgpu_device to link supported device attribute
> node.
> 2. add new structure "struct amdgpu_device_attr_entry{}" to track device
> attribute state.
> 3. drop member "states" from amdgpu_device_attr.
>
> v2:
> 1. move "attr_list" into amdgpu_pm and rename to "pm_attr_list".
> 2. refine create & remove device node functions parameter.
>
> fix:
> drm/amdgpu: optimize amdgpu device attribute code
>
> Signed-off-by: Kevin Wang
> Reviewed-by: Alex Deucher
Looks good. Thanks!
Alex
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_dpm.h | 1 +
> drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c | 85 ++---
> drivers/gpu/drm/amd/amdgpu/amdgpu_pm.h | 13 ++--
> 3 files changed, 58 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_dpm.h
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_dpm.h
> index 956f6c710670..6a8aae70a0e6 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_dpm.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_dpm.h
> @@ -450,6 +450,7 @@ struct amdgpu_pm {
>
> /* Used for I2C access to various EEPROMs on relevant ASICs */
> struct i2c_adapter smu_i2c;
> + struct list_headpm_attr_list;
> };
>
> #define R600_SSTU_DFLT 0
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
> index 55815b899942..dd5be3bb4bb1 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
> @@ -1725,7 +1725,7 @@ static struct amdgpu_device_attr amdgpu_device_attrs[]
> = {
> };
>
> static int default_attr_update(struct amdgpu_device *adev, struct
> amdgpu_device_attr *attr,
> - uint32_t mask)
> + uint32_t mask, enum amdgpu_device_attr_states
> *states)
> {
> struct device_attribute *dev_attr = >dev_attr;
> const char *attr_name = dev_attr->attr.name;
> @@ -1733,7 +1733,7 @@ static int default_attr_update(struct amdgpu_device
> *adev, struct amdgpu_device_
> enum amd_asic_type asic_type = adev->asic_type;
>
> if (!(attr->flags & mask)) {
> - attr->states = ATTR_STATE_UNSUPPORTED;
> + *states = ATTR_STATE_UNSUPPORTED;
> return 0;
> }
>
> @@ -1741,34 +1741,34 @@ static int default_attr_update(struct amdgpu_device
> *adev, struct amdgpu_device_
>
> if (DEVICE_ATTR_IS(pp_dpm_socclk)) {
> if (asic_type <= CHIP_VEGA10)
> - attr->states = ATTR_STATE_UNSUPPORTED;
> + *states = ATTR_STATE_UNSUPPORTED;
> } else if (DEVICE_ATTR_IS(pp_dpm_dcefclk)) {
> if (asic_type <= CHIP_VEGA10 || asic_type == CHIP_ARCTURUS)
> - attr->states = ATTR_STATE_UNSUPPORTED;
> + *states = ATTR_STATE_UNSUPPORTED;
> } else if (DEVICE_ATTR_IS(pp_dpm_fclk)) {
> if (asic_type < CHIP_VEGA20)
> - attr->states = ATTR_STATE_UNSUPPORTED;
> + *states = ATTR_STATE_UNSUPPORTED;
> } else if (DEVICE_ATTR_IS(pp_dpm_pcie)) {
> if (asic_type == CHIP_ARCTURUS)
> - attr->states = ATTR_STATE_UNSUPPORTED;
> + *states = ATTR_STATE_UNSUPPORTED;
> } else if (DEVICE_ATTR_IS(pp_od_clk_voltage)) {
> - attr->states = ATTR_STATE_UNSUPPORTED;
> + *states = ATTR_STATE_UNSUPPORTED;
> if ((is_support_sw_smu(adev) && adev->smu.od_enabled) ||
> (!is_support_sw_smu(adev) && hwmgr->od_enabled))
> - attr->states = ATTR_STATE_UNSUPPORTED;
> + *states = ATTR_STATE_UNSUPPORTED;
> } else if (DEVICE_ATTR_IS(mem_busy_percent)) {
> if (adev->flags & AMD_IS_APU || asic_type == CHIP_VEGA10)
> - attr->states = ATTR_STATE_UNSUPPORTED;
> + *states = ATTR_STATE_UNSUPPORTED;
> } else if (DEVICE_ATTR_IS(pcie_bw)) {
> /* PCIe Perf counters won't work on APU nodes */
> if (adev->flags & AMD_IS_APU)
> - attr->states = ATTR_STATE_UNSUPPORTED;
> + *states = ATTR_STATE_UNSUPPORTED;
> } else if (DEVICE_ATTR_IS(unique_id)) {
> if (!adev->unique_id)
> - attr->states = ATTR_STATE_UNSUPPORTED;
> + *states = ATTR_STATE_UNSUPPORTED;
> } else if (DEVICE_ATTR_IS(pp_features)) {
>
Re: [PATCH] drm/amdgpu: fix device attribute node create failed with multi gpu
[AMD Official Use Only - Internal Distribution Only]
From: Alex Deucher
Sent: Friday, May 22, 2020 11:16 PM
To: Wang, Kevin(Yang)
Cc: amd-gfx list ; Deucher, Alexander
; Zhang, Hawking
Subject: Re: [PATCH] drm/amdgpu: fix device attribute node create failed with
multi gpu
On Fri, May 22, 2020 at 10:57 AM Kevin Wang wrote:
>
> the origin design will use varible of "attr->states" to save node
> supported states on current gpu device, but for multi gpu device, when
> probe second gpu device, the driver will check attribute node states
> from previous gpu device wthether to create attribute node.
> it will cause other gpu device create attribute node faild.
>
> 1. add member attr_list into amdgpu_device to link supported device attribute
> node.
> 2. add new structure "struct amdgpu_device_attr_entry{}" to track device
> attribute state.
>
> fix:
> drm/amdgpu: optimize amdgpu device attribute code
>
> Signed-off-by: Kevin Wang
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu.h| 1 +
> drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c | 75 +++---
> drivers/gpu/drm/amd/amdgpu/amdgpu_pm.h | 13 +++--
> 3 files changed, 52 insertions(+), 37 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index bfce0931f9c1..b84146339ea3 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> @@ -995,6 +995,7 @@ struct amdgpu_device {
> charserial[16];
>
> struct amdgpu_autodump autodump;
> + struct list_headattr_list;
Might want to call this pm_attr_list or even move this to the
amdgpu_pm struct, but either way, assuming you've tested this with
multiple GPUs:
Reviewed-by: Alex Deucher
[kevin]:
thanks, the patch v2 will fix it.
and this patch is test passed on local witl multi gpu.
> };
>
> static inline struct amdgpu_device *amdgpu_ttm_adev(struct ttm_bo_device
> *bdev)
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
> index 55815b899942..ac99aa0a16a8 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
> @@ -1725,7 +1725,7 @@ static struct amdgpu_device_attr amdgpu_device_attrs[]
> = {
> };
>
> static int default_attr_update(struct amdgpu_device *adev, struct
> amdgpu_device_attr *attr,
> - uint32_t mask)
> + uint32_t mask, enum amdgpu_device_attr_states
> *states)
> {
> struct device_attribute *dev_attr = >dev_attr;
> const char *attr_name = dev_attr->attr.name;
> @@ -1733,7 +1733,7 @@ static int default_attr_update(struct amdgpu_device
> *adev, struct amdgpu_device_
> enum amd_asic_type asic_type = adev->asic_type;
>
> if (!(attr->flags & mask)) {
> - attr->states = ATTR_STATE_UNSUPPORTED;
> + *states = ATTR_STATE_UNSUPPORTED;
> return 0;
> }
>
> @@ -1741,34 +1741,34 @@ static int default_attr_update(struct amdgpu_device
> *adev, struct amdgpu_device_
>
> if (DEVICE_ATTR_IS(pp_dpm_socclk)) {
> if (asic_type <= CHIP_VEGA10)
> - attr->states = ATTR_STATE_UNSUPPORTED;
> + *states = ATTR_STATE_UNSUPPORTED;
> } else if (DEVICE_ATTR_IS(pp_dpm_dcefclk)) {
> if (asic_type <= CHIP_VEGA10 || asic_type == CHIP_ARCTURUS)
> - attr->states = ATTR_STATE_UNSUPPORTED;
> + *states = ATTR_STATE_UNSUPPORTED;
> } else if (DEVICE_ATTR_IS(pp_dpm_fclk)) {
> if (asic_type < CHIP_VEGA20)
> - attr->states = ATTR_STATE_UNSUPPORTED;
> + *states = ATTR_STATE_UNSUPPORTED;
> } else if (DEVICE_ATTR_IS(pp_dpm_pcie)) {
> if (asic_type == CHIP_ARCTURUS)
> - attr->states = ATTR_STATE_UNSUPPORTED;
> + *states = ATTR_STATE_UNSUPPORTED;
> } else if (DEVICE_ATTR_IS(pp_od_clk_voltage)) {
> - attr->states = ATTR_STATE_UNSUPPORTED;
> + *states = ATTR_STATE_UNSUPPORTED;
> if ((is_support_sw_smu(adev) && adev->smu.od_enabled) ||
> (!is_support_sw_smu(adev) && hwmgr->od_enabled))
> - attr->states = ATTR_STATE_UNSUPPORTED;
> + *states = ATTR_STATE_UNSUPPORTED;
> } else if (DEVICE_ATTR_IS(mem_busy_percent)) {
> if (adev->flags & AMD_IS_APU || asic_type == CHIP_VEGA10)
> - attr->states = ATTR_STATE_UNSUPPORTED;
> + *states = ATTR_STATE_UNSUPPORTED;
> } else if (DEVICE_ATTR_IS(pcie_bw)) {
> /* PCIe Perf counters won't work on APU nodes */
> if (adev->flags & AMD_IS_APU)
> - attr->states =
[PATCH v2] drm/amdgpu: fix device attribute node create failed with multi gpu
the origin design will use varible of "attr->states" to save node
supported states on current gpu device, but for multi gpu device, when
probe second gpu device, the driver will check attribute node states
from previous gpu device wthether to create attribute node.
it will cause other gpu device create attribute node faild.
1. add member attr_list into amdgpu_device to link supported device attribute
node.
2. add new structure "struct amdgpu_device_attr_entry{}" to track device
attribute state.
3. drop member "states" from amdgpu_device_attr.
v2:
1. move "attr_list" into amdgpu_pm and rename to "pm_attr_list".
2. refine create & remove device node functions parameter.
fix:
drm/amdgpu: optimize amdgpu device attribute code
Signed-off-by: Kevin Wang
Reviewed-by: Alex Deucher
---
drivers/gpu/drm/amd/amdgpu/amdgpu_dpm.h | 1 +
drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c | 85 ++---
drivers/gpu/drm/amd/amdgpu/amdgpu_pm.h | 13 ++--
3 files changed, 58 insertions(+), 41 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_dpm.h
b/drivers/gpu/drm/amd/amdgpu/amdgpu_dpm.h
index 956f6c710670..6a8aae70a0e6 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_dpm.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_dpm.h
@@ -450,6 +450,7 @@ struct amdgpu_pm {
/* Used for I2C access to various EEPROMs on relevant ASICs */
struct i2c_adapter smu_i2c;
+ struct list_headpm_attr_list;
};
#define R600_SSTU_DFLT 0
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
index 55815b899942..dd5be3bb4bb1 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
@@ -1725,7 +1725,7 @@ static struct amdgpu_device_attr amdgpu_device_attrs[] = {
};
static int default_attr_update(struct amdgpu_device *adev, struct
amdgpu_device_attr *attr,
- uint32_t mask)
+ uint32_t mask, enum amdgpu_device_attr_states
*states)
{
struct device_attribute *dev_attr = >dev_attr;
const char *attr_name = dev_attr->attr.name;
@@ -1733,7 +1733,7 @@ static int default_attr_update(struct amdgpu_device
*adev, struct amdgpu_device_
enum amd_asic_type asic_type = adev->asic_type;
if (!(attr->flags & mask)) {
- attr->states = ATTR_STATE_UNSUPPORTED;
+ *states = ATTR_STATE_UNSUPPORTED;
return 0;
}
@@ -1741,34 +1741,34 @@ static int default_attr_update(struct amdgpu_device
*adev, struct amdgpu_device_
if (DEVICE_ATTR_IS(pp_dpm_socclk)) {
if (asic_type <= CHIP_VEGA10)
- attr->states = ATTR_STATE_UNSUPPORTED;
+ *states = ATTR_STATE_UNSUPPORTED;
} else if (DEVICE_ATTR_IS(pp_dpm_dcefclk)) {
if (asic_type <= CHIP_VEGA10 || asic_type == CHIP_ARCTURUS)
- attr->states = ATTR_STATE_UNSUPPORTED;
+ *states = ATTR_STATE_UNSUPPORTED;
} else if (DEVICE_ATTR_IS(pp_dpm_fclk)) {
if (asic_type < CHIP_VEGA20)
- attr->states = ATTR_STATE_UNSUPPORTED;
+ *states = ATTR_STATE_UNSUPPORTED;
} else if (DEVICE_ATTR_IS(pp_dpm_pcie)) {
if (asic_type == CHIP_ARCTURUS)
- attr->states = ATTR_STATE_UNSUPPORTED;
+ *states = ATTR_STATE_UNSUPPORTED;
} else if (DEVICE_ATTR_IS(pp_od_clk_voltage)) {
- attr->states = ATTR_STATE_UNSUPPORTED;
+ *states = ATTR_STATE_UNSUPPORTED;
if ((is_support_sw_smu(adev) && adev->smu.od_enabled) ||
(!is_support_sw_smu(adev) && hwmgr->od_enabled))
- attr->states = ATTR_STATE_UNSUPPORTED;
+ *states = ATTR_STATE_UNSUPPORTED;
} else if (DEVICE_ATTR_IS(mem_busy_percent)) {
if (adev->flags & AMD_IS_APU || asic_type == CHIP_VEGA10)
- attr->states = ATTR_STATE_UNSUPPORTED;
+ *states = ATTR_STATE_UNSUPPORTED;
} else if (DEVICE_ATTR_IS(pcie_bw)) {
/* PCIe Perf counters won't work on APU nodes */
if (adev->flags & AMD_IS_APU)
- attr->states = ATTR_STATE_UNSUPPORTED;
+ *states = ATTR_STATE_UNSUPPORTED;
} else if (DEVICE_ATTR_IS(unique_id)) {
if (!adev->unique_id)
- attr->states = ATTR_STATE_UNSUPPORTED;
+ *states = ATTR_STATE_UNSUPPORTED;
} else if (DEVICE_ATTR_IS(pp_features)) {
if (adev->flags & AMD_IS_APU || asic_type <= CHIP_VEGA10)
- attr->states = ATTR_STATE_UNSUPPORTED;
+ *states = ATTR_STATE_UNSUPPORTED;
}
if (asic_type == CHIP_ARCTURUS) {
@@ -1789,27 +1789,29 @@ static int
Re: [PATCH] drm/amdgpu: fix device attribute node create failed with multi gpu
On Fri, May 22, 2020 at 10:57 AM Kevin Wang wrote:
>
> the origin design will use varible of "attr->states" to save node
> supported states on current gpu device, but for multi gpu device, when
> probe second gpu device, the driver will check attribute node states
> from previous gpu device wthether to create attribute node.
> it will cause other gpu device create attribute node faild.
>
> 1. add member attr_list into amdgpu_device to link supported device attribute
> node.
> 2. add new structure "struct amdgpu_device_attr_entry{}" to track device
> attribute state.
>
> fix:
> drm/amdgpu: optimize amdgpu device attribute code
>
> Signed-off-by: Kevin Wang
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu.h| 1 +
> drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c | 75 +++---
> drivers/gpu/drm/amd/amdgpu/amdgpu_pm.h | 13 +++--
> 3 files changed, 52 insertions(+), 37 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index bfce0931f9c1..b84146339ea3 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> @@ -995,6 +995,7 @@ struct amdgpu_device {
> charserial[16];
>
> struct amdgpu_autodump autodump;
> + struct list_headattr_list;
Might want to call this pm_attr_list or even move this to the
amdgpu_pm struct, but either way, assuming you've tested this with
multiple GPUs:
Reviewed-by: Alex Deucher
> };
>
> static inline struct amdgpu_device *amdgpu_ttm_adev(struct ttm_bo_device
> *bdev)
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
> index 55815b899942..ac99aa0a16a8 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
> @@ -1725,7 +1725,7 @@ static struct amdgpu_device_attr amdgpu_device_attrs[]
> = {
> };
>
> static int default_attr_update(struct amdgpu_device *adev, struct
> amdgpu_device_attr *attr,
> - uint32_t mask)
> + uint32_t mask, enum amdgpu_device_attr_states
> *states)
> {
> struct device_attribute *dev_attr = >dev_attr;
> const char *attr_name = dev_attr->attr.name;
> @@ -1733,7 +1733,7 @@ static int default_attr_update(struct amdgpu_device
> *adev, struct amdgpu_device_
> enum amd_asic_type asic_type = adev->asic_type;
>
> if (!(attr->flags & mask)) {
> - attr->states = ATTR_STATE_UNSUPPORTED;
> + *states = ATTR_STATE_UNSUPPORTED;
> return 0;
> }
>
> @@ -1741,34 +1741,34 @@ static int default_attr_update(struct amdgpu_device
> *adev, struct amdgpu_device_
>
> if (DEVICE_ATTR_IS(pp_dpm_socclk)) {
> if (asic_type <= CHIP_VEGA10)
> - attr->states = ATTR_STATE_UNSUPPORTED;
> + *states = ATTR_STATE_UNSUPPORTED;
> } else if (DEVICE_ATTR_IS(pp_dpm_dcefclk)) {
> if (asic_type <= CHIP_VEGA10 || asic_type == CHIP_ARCTURUS)
> - attr->states = ATTR_STATE_UNSUPPORTED;
> + *states = ATTR_STATE_UNSUPPORTED;
> } else if (DEVICE_ATTR_IS(pp_dpm_fclk)) {
> if (asic_type < CHIP_VEGA20)
> - attr->states = ATTR_STATE_UNSUPPORTED;
> + *states = ATTR_STATE_UNSUPPORTED;
> } else if (DEVICE_ATTR_IS(pp_dpm_pcie)) {
> if (asic_type == CHIP_ARCTURUS)
> - attr->states = ATTR_STATE_UNSUPPORTED;
> + *states = ATTR_STATE_UNSUPPORTED;
> } else if (DEVICE_ATTR_IS(pp_od_clk_voltage)) {
> - attr->states = ATTR_STATE_UNSUPPORTED;
> + *states = ATTR_STATE_UNSUPPORTED;
> if ((is_support_sw_smu(adev) && adev->smu.od_enabled) ||
> (!is_support_sw_smu(adev) && hwmgr->od_enabled))
> - attr->states = ATTR_STATE_UNSUPPORTED;
> + *states = ATTR_STATE_UNSUPPORTED;
> } else if (DEVICE_ATTR_IS(mem_busy_percent)) {
> if (adev->flags & AMD_IS_APU || asic_type == CHIP_VEGA10)
> - attr->states = ATTR_STATE_UNSUPPORTED;
> + *states = ATTR_STATE_UNSUPPORTED;
> } else if (DEVICE_ATTR_IS(pcie_bw)) {
> /* PCIe Perf counters won't work on APU nodes */
> if (adev->flags & AMD_IS_APU)
> - attr->states = ATTR_STATE_UNSUPPORTED;
> + *states = ATTR_STATE_UNSUPPORTED;
> } else if (DEVICE_ATTR_IS(unique_id)) {
> if (!adev->unique_id)
> - attr->states = ATTR_STATE_UNSUPPORTED;
> + *states = ATTR_STATE_UNSUPPORTED;
> } else if (DEVICE_ATTR_IS(pp_features)) {
> if (adev->flags & AMD_IS_APU || asic_type <=
Re: [PATCH] drm/amd/display: Handle GPU reset for DC block
On 2020-05-22 10:45 a.m., Alex Deucher wrote:
On Thu, May 21, 2020 at 5:39 PM Bhawanpreet Lakha
wrote:
[Why]
Previously we used the s3 codepath for gpu reset. This can lead to issues in
certain case where we end of waiting for fences which will never come (because
parts of the hw are off due to gpu reset) and we end up waiting forever causing
a deadlock.
[How]
Handle GPU reset separately from normal s3 case. We essentially need to redo
everything we do in s3, but avoid any drm calls.
For GPU reset case
suspend:
-Acquire DC lock
-Cache current dc_state
-Commit 0 stream/planes to dc (this puts dc into a state where it can
be
powered off)
-Disable interrupts
resume
-Edit cached state to force full update
-Commit cached state from suspend
-Build stream and plane updates from the cached state
-Commit stream/plane updates
-Enable interrupts
-Release DC lock
v2:
-Formatting
-Release dc_state
Signed-off-by: Bhawanpreet Lakha
Acked-by: Alex Deucher
Looks good to me now.
Reviewed-by: Nicholas Kazlauskas
Regards,
Nicholas Kazlauskas
---
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 182 +-
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 1 +
2 files changed, 182 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index 60fe64aef11b..4110ff8580b7 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -1521,10 +1521,114 @@ static int dm_hw_fini(void *handle)
return 0;
}
+
+static int dm_enable_vblank(struct drm_crtc *crtc);
+static void dm_disable_vblank(struct drm_crtc *crtc);
+
+static void dm_gpureset_toggle_interrupts(struct amdgpu_device *adev,
+struct dc_state *state, bool enable)
+{
+ enum dc_irq_source irq_source;
+ struct amdgpu_crtc *acrtc;
+ int rc = -EBUSY;
+ int i = 0;
+
+ for (i = 0; i < state->stream_count; i++) {
+ acrtc = get_crtc_by_otg_inst(
+ adev, state->stream_status[i].primary_otg_inst);
+
+ if (acrtc && state->stream_status[i].plane_count != 0) {
+ irq_source = IRQ_TYPE_PFLIP + acrtc->otg_inst;
+ rc = dc_interrupt_set(adev->dm.dc, irq_source, enable)
? 0 : -EBUSY;
+ DRM_DEBUG("crtc %d - vupdate irq %sabling: r=%d\n",
+ acrtc->crtc_id, enable ? "en" : "dis", rc);
+ if (rc)
+ DRM_WARN("Failed to %s pflip interrupts\n",
+enable ? "enable" : "disable");
+
+ if (enable) {
+ rc = dm_enable_vblank(>base);
+ if (rc)
+ DRM_WARN("Failed to enable vblank
interrupts\n");
+ } else {
+ dm_disable_vblank(>base);
+ }
+
+ }
+ }
+
+}
+
+enum dc_status amdgpu_dm_commit_zero_streams(struct dc *dc)
+{
+ struct dc_state *context = NULL;
+ enum dc_status res = DC_ERROR_UNEXPECTED;
+ int i;
+ struct dc_stream_state *del_streams[MAX_PIPES];
+ int del_streams_count = 0;
+
+ memset(del_streams, 0, sizeof(del_streams));
+
+ context = dc_create_state(dc);
+ if (context == NULL)
+ goto context_alloc_fail;
+
+ dc_resource_state_copy_construct_current(dc, context);
+
+ /* First remove from context all streams */
+ for (i = 0; i < context->stream_count; i++) {
+ struct dc_stream_state *stream = context->streams[i];
+
+ del_streams[del_streams_count++] = stream;
+ }
+
+ /* Remove all planes for removed streams and then remove the streams */
+ for (i = 0; i < del_streams_count; i++) {
+ if (!dc_rem_all_planes_for_stream(dc, del_streams[i], context))
{
+ res = DC_FAIL_DETACH_SURFACES;
+ goto fail;
+ }
+
+ res = dc_remove_stream_from_ctx(dc, context, del_streams[i]);
+ if (res != DC_OK)
+ goto fail;
+ }
+
+
+ res = dc_validate_global_state(dc, context, false);
+
+ if (res != DC_OK) {
+ DRM_ERROR("%s:resource validation failed, dc_status:%d\n",
__func__, res);
+ goto fail;
+ }
+
+ res = dc_commit_state(dc, context);
+
+fail:
+ dc_release_state(context);
+
+context_alloc_fail:
+ return res;
+}
+
static int dm_suspend(void *handle)
{
struct amdgpu_device *adev = handle;
struct amdgpu_display_manager *dm = >dm;
+ int ret = 0;
+
+ if (adev->in_gpu_reset) {
+
[PATCH] drm/amdgpu: fix device attribute node create failed with multi gpu
the origin design will use varible of "attr->states" to save node
supported states on current gpu device, but for multi gpu device, when
probe second gpu device, the driver will check attribute node states
from previous gpu device wthether to create attribute node.
it will cause other gpu device create attribute node faild.
1. add member attr_list into amdgpu_device to link supported device attribute
node.
2. add new structure "struct amdgpu_device_attr_entry{}" to track device
attribute state.
fix:
drm/amdgpu: optimize amdgpu device attribute code
Signed-off-by: Kevin Wang
---
drivers/gpu/drm/amd/amdgpu/amdgpu.h| 1 +
drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c | 75 +++---
drivers/gpu/drm/amd/amdgpu/amdgpu_pm.h | 13 +++--
3 files changed, 52 insertions(+), 37 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index bfce0931f9c1..b84146339ea3 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -995,6 +995,7 @@ struct amdgpu_device {
charserial[16];
struct amdgpu_autodump autodump;
+ struct list_headattr_list;
};
static inline struct amdgpu_device *amdgpu_ttm_adev(struct ttm_bo_device *bdev)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
index 55815b899942..ac99aa0a16a8 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
@@ -1725,7 +1725,7 @@ static struct amdgpu_device_attr amdgpu_device_attrs[] = {
};
static int default_attr_update(struct amdgpu_device *adev, struct
amdgpu_device_attr *attr,
- uint32_t mask)
+ uint32_t mask, enum amdgpu_device_attr_states
*states)
{
struct device_attribute *dev_attr = >dev_attr;
const char *attr_name = dev_attr->attr.name;
@@ -1733,7 +1733,7 @@ static int default_attr_update(struct amdgpu_device
*adev, struct amdgpu_device_
enum amd_asic_type asic_type = adev->asic_type;
if (!(attr->flags & mask)) {
- attr->states = ATTR_STATE_UNSUPPORTED;
+ *states = ATTR_STATE_UNSUPPORTED;
return 0;
}
@@ -1741,34 +1741,34 @@ static int default_attr_update(struct amdgpu_device
*adev, struct amdgpu_device_
if (DEVICE_ATTR_IS(pp_dpm_socclk)) {
if (asic_type <= CHIP_VEGA10)
- attr->states = ATTR_STATE_UNSUPPORTED;
+ *states = ATTR_STATE_UNSUPPORTED;
} else if (DEVICE_ATTR_IS(pp_dpm_dcefclk)) {
if (asic_type <= CHIP_VEGA10 || asic_type == CHIP_ARCTURUS)
- attr->states = ATTR_STATE_UNSUPPORTED;
+ *states = ATTR_STATE_UNSUPPORTED;
} else if (DEVICE_ATTR_IS(pp_dpm_fclk)) {
if (asic_type < CHIP_VEGA20)
- attr->states = ATTR_STATE_UNSUPPORTED;
+ *states = ATTR_STATE_UNSUPPORTED;
} else if (DEVICE_ATTR_IS(pp_dpm_pcie)) {
if (asic_type == CHIP_ARCTURUS)
- attr->states = ATTR_STATE_UNSUPPORTED;
+ *states = ATTR_STATE_UNSUPPORTED;
} else if (DEVICE_ATTR_IS(pp_od_clk_voltage)) {
- attr->states = ATTR_STATE_UNSUPPORTED;
+ *states = ATTR_STATE_UNSUPPORTED;
if ((is_support_sw_smu(adev) && adev->smu.od_enabled) ||
(!is_support_sw_smu(adev) && hwmgr->od_enabled))
- attr->states = ATTR_STATE_UNSUPPORTED;
+ *states = ATTR_STATE_UNSUPPORTED;
} else if (DEVICE_ATTR_IS(mem_busy_percent)) {
if (adev->flags & AMD_IS_APU || asic_type == CHIP_VEGA10)
- attr->states = ATTR_STATE_UNSUPPORTED;
+ *states = ATTR_STATE_UNSUPPORTED;
} else if (DEVICE_ATTR_IS(pcie_bw)) {
/* PCIe Perf counters won't work on APU nodes */
if (adev->flags & AMD_IS_APU)
- attr->states = ATTR_STATE_UNSUPPORTED;
+ *states = ATTR_STATE_UNSUPPORTED;
} else if (DEVICE_ATTR_IS(unique_id)) {
if (!adev->unique_id)
- attr->states = ATTR_STATE_UNSUPPORTED;
+ *states = ATTR_STATE_UNSUPPORTED;
} else if (DEVICE_ATTR_IS(pp_features)) {
if (adev->flags & AMD_IS_APU || asic_type <= CHIP_VEGA10)
- attr->states = ATTR_STATE_UNSUPPORTED;
+ *states = ATTR_STATE_UNSUPPORTED;
}
if (asic_type == CHIP_ARCTURUS) {
@@ -1794,22 +1794,24 @@ static int amdgpu_device_attr_create(struct
amdgpu_device *adev,
int ret = 0;
struct device_attribute *dev_attr = >dev_attr;
const char *name = dev_attr->attr.name;
+ enum
Re: [RFC] Deprecate AGP GART support for Radeon/Nouveau/TTM
On Fri, May 22, 2020 at 6:41 AM Christian König wrote: > > Am 20.05.20 um 18:18 schrieb Alex Deucher: > > On Wed, May 20, 2020 at 10:43 AM Christian König > > wrote: > >> Am 13.05.20 um 13:03 schrieb Christian König: > >>> Unfortunately AGP is still to widely used as we could just drop support > >>> for using its GART. > >>> > >>> Not using the AGP GART also doesn't mean a loss in functionality since > >>> drivers will just fallback to the driver specific PCI GART. > >>> > >>> For now just deprecate the code and don't enable the AGP GART in TTM even > >>> when general AGP support is available. > >> So I've used an ancient system (32bit) to setup a test box for this. > >> > >> > >> The first GPU I could test is an RV280 (Radeon 9200 PRO) which is easily > >> 15 years old. > >> > >> What happens in AGP mode is that glxgears shows artifacts during > >> rendering on this system. > >> > >> In PCI mode those rendering artifacts are gone and glxgears seems to > >> draw everything correctly now. > >> > >> Performance is obviously not comparable, cause in AGP we don't render > >> all triangles correctly. > >> > >> > >> The second GPU I could test is an RV630 PRO (Radeon HD 2600 PRO AGP) > >> which is more than 10 years old. > >> > >> As far as I can tell this one works in both AGP and PCIe mode perfectly > >> fine. > >> > >> Since this is only a 32bit system I couldn't really test any OpenGL game > >> that well. > >> > >> But for glxgears switching from AGP to PCIe mode seems to result in a > >> roughly 5% performance drop. > >> > >> The surprising reason for this is not the better TLB performance, but > >> the lack of USWC support for the PCIe GART in radeon. > >> > >> > >> So if anybody wants to get his hands dirty and squeeze a bit more > >> performance out of the old hardware, porting USWC from amdgpu to radeon > >> shouldn't be to much of a problem. > > We do support USWC on radeon, although I think we had separate flags > > for cached and WC. That said we had a lot of problems with WC on 32 > > bit (see radeon_bo_create()). The other problem is that, at least on > > the really old radeons, the PCI gart didn't support snooped and > > unsnooped. It was always snooped. It wasn't until pcie that the gart > > hw got support for both. For AGP, the expectation was that AGP > > provided the uncached memory. > > Oh, indeed. I didn't remembered that. > > Interesting is that in this case I have no idea where the performance > difference is coming from. > > > > >> > >> Summing it up I'm still leaning towards disabling AGP completely by > >> default for radeon and deprecate it in TTM as well. > >> > >> Thoughts? Especially Alex what do you think. > > Works for me. > > I will take that as an rb and commit at least the first patch. Yeah, Reviewed-by: Alex Deucher > > Thanks, > Christian. > > > > > Alex > ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH] drm/amd/display: Handle GPU reset for DC block
On Thu, May 21, 2020 at 5:39 PM Bhawanpreet Lakha
wrote:
>
> [Why]
> Previously we used the s3 codepath for gpu reset. This can lead to issues in
> certain case where we end of waiting for fences which will never come (because
> parts of the hw are off due to gpu reset) and we end up waiting forever
> causing
> a deadlock.
>
> [How]
> Handle GPU reset separately from normal s3 case. We essentially need to redo
> everything we do in s3, but avoid any drm calls.
>
> For GPU reset case
>
> suspend:
> -Acquire DC lock
> -Cache current dc_state
> -Commit 0 stream/planes to dc (this puts dc into a state where it can
> be
> powered off)
> -Disable interrupts
> resume
> -Edit cached state to force full update
> -Commit cached state from suspend
> -Build stream and plane updates from the cached state
> -Commit stream/plane updates
> -Enable interrupts
> -Release DC lock
>
> v2:
> -Formatting
> -Release dc_state
>
> Signed-off-by: Bhawanpreet Lakha
Acked-by: Alex Deucher
> ---
> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 182 +-
> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 1 +
> 2 files changed, 182 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index 60fe64aef11b..4110ff8580b7 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -1521,10 +1521,114 @@ static int dm_hw_fini(void *handle)
> return 0;
> }
>
> +
> +static int dm_enable_vblank(struct drm_crtc *crtc);
> +static void dm_disable_vblank(struct drm_crtc *crtc);
> +
> +static void dm_gpureset_toggle_interrupts(struct amdgpu_device *adev,
> +struct dc_state *state, bool enable)
> +{
> + enum dc_irq_source irq_source;
> + struct amdgpu_crtc *acrtc;
> + int rc = -EBUSY;
> + int i = 0;
> +
> + for (i = 0; i < state->stream_count; i++) {
> + acrtc = get_crtc_by_otg_inst(
> + adev,
> state->stream_status[i].primary_otg_inst);
> +
> + if (acrtc && state->stream_status[i].plane_count != 0) {
> + irq_source = IRQ_TYPE_PFLIP + acrtc->otg_inst;
> + rc = dc_interrupt_set(adev->dm.dc, irq_source,
> enable) ? 0 : -EBUSY;
> + DRM_DEBUG("crtc %d - vupdate irq %sabling: r=%d\n",
> + acrtc->crtc_id, enable ? "en" : "dis", rc);
> + if (rc)
> + DRM_WARN("Failed to %s pflip interrupts\n",
> +enable ? "enable" : "disable");
> +
> + if (enable) {
> + rc = dm_enable_vblank(>base);
> + if (rc)
> + DRM_WARN("Failed to enable vblank
> interrupts\n");
> + } else {
> + dm_disable_vblank(>base);
> + }
> +
> + }
> + }
> +
> +}
> +
> +enum dc_status amdgpu_dm_commit_zero_streams(struct dc *dc)
> +{
> + struct dc_state *context = NULL;
> + enum dc_status res = DC_ERROR_UNEXPECTED;
> + int i;
> + struct dc_stream_state *del_streams[MAX_PIPES];
> + int del_streams_count = 0;
> +
> + memset(del_streams, 0, sizeof(del_streams));
> +
> + context = dc_create_state(dc);
> + if (context == NULL)
> + goto context_alloc_fail;
> +
> + dc_resource_state_copy_construct_current(dc, context);
> +
> + /* First remove from context all streams */
> + for (i = 0; i < context->stream_count; i++) {
> + struct dc_stream_state *stream = context->streams[i];
> +
> + del_streams[del_streams_count++] = stream;
> + }
> +
> + /* Remove all planes for removed streams and then remove the streams
> */
> + for (i = 0; i < del_streams_count; i++) {
> + if (!dc_rem_all_planes_for_stream(dc, del_streams[i],
> context)) {
> + res = DC_FAIL_DETACH_SURFACES;
> + goto fail;
> + }
> +
> + res = dc_remove_stream_from_ctx(dc, context, del_streams[i]);
> + if (res != DC_OK)
> + goto fail;
> + }
> +
> +
> + res = dc_validate_global_state(dc, context, false);
> +
> + if (res != DC_OK) {
> + DRM_ERROR("%s:resource validation failed, dc_status:%d\n",
> __func__, res);
> + goto fail;
> + }
> +
> + res = dc_commit_state(dc, context);
> +
> +fail:
> + dc_release_state(context);
> +
> +context_alloc_fail:
> + return res;
> +}
> +
> static int dm_suspend(void *handle)
> {
> struct
Re: [PATCH 1/3] Revert "drm/amdgpu: off by one in amdgpu_device_attr_create_groups() error handling"
On Fri, May 22, 2020 at 6:25 AM Christian König
wrote:
>
> Well what exactly is going wrong here?
>
> At least this one looks correct to me, and if it blocks the revert we
> should probably squash it in there as well.
I can squash them together. These were just dependences. The issue
is the third patch.
Alex
>
> Christian.
>
> Am 21.05.20 um 22:23 schrieb Alex Deucher:
> > This reverts commit b41d9df2b680b96913cc3ccf929252e2dce71b24.
> >
> > Signed-off-by: Alex Deucher
> > ---
> > drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
> > index 2fc51f815eaa..72bbb8175b22 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
> > @@ -1843,8 +1843,9 @@ static int amdgpu_device_attr_create_groups(struct
> > amdgpu_device *adev,
> > return 0;
> >
> > failed:
> > - while (i--)
> > + for (; i > 0; i--) {
> > amdgpu_device_attr_remove(adev, [i]);
> > + }
> >
> > return ret;
> > }
>
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [RFC] Deprecate AGP GART support for Radeon/Nouveau/TTM
On 2020-05-22 12:40 p.m., Christian König wrote: > Am 20.05.20 um 18:25 schrieb Michel Dänzer: >> On 2020-05-20 4:43 p.m., Christian König wrote: >>> Am 13.05.20 um 13:03 schrieb Christian König: Unfortunately AGP is still to widely used as we could just drop support for using its GART. Not using the AGP GART also doesn't mean a loss in functionality since drivers will just fallback to the driver specific PCI GART. For now just deprecate the code and don't enable the AGP GART in TTM even when general AGP support is available. >>> So I've used an ancient system (32bit) to setup a test box for this. >>> >>> >>> The first GPU I could test is an RV280 (Radeon 9200 PRO) which is easily >>> 15 years old. >>> >>> What happens in AGP mode is that glxgears shows artifacts during >>> rendering on this system. >>> >>> In PCI mode those rendering artifacts are gone and glxgears seems to >>> draw everything correctly now. >>> >>> Performance is obviously not comparable, cause in AGP we don't render >>> all triangles correctly. >>> >>> >>> The second GPU I could test is an RV630 PRO (Radeon HD 2600 PRO AGP) >>> which is more than 10 years old. >>> >>> As far as I can tell this one works in both AGP and PCIe mode perfectly >>> fine. >>> >>> Since this is only a 32bit system I couldn't really test any OpenGL game >>> that well. >>> >>> But for glxgears switching from AGP to PCIe mode seems to result in a >>> roughly 5% performance drop. >>> >>> The surprising reason for this is not the better TLB performance, but >>> the lack of USWC support for the PCIe GART in radeon. >> I suspect the main reason it's only 5% is that PCIe GART page tables are >> stored in VRAM, so they don't need to be fetched across the PCIe link >> (and presumably it has more than one TLB entry as well). The difference >> is much bigger with native AGP ASICs with PCI GART. > > Do you have some hardware you could give that a try on? As I mentioned before, I tested this many times on my AGP PowerBooks back in the day. The result was always a similar, big hit with PCI GART vs AGP (even just 1x). I haven't seen any reason to believe this has changed. > While I agree that it means a performance regression, this is a rather > high motivation to go ahead with at least the first patch. I totally agree with the benefits, I just want everyone to be honest and clear about the performance hit with native AGP Radeons, which already have very weak performance by today's standards even with AGP. -- Earthling Michel Dänzer | https://redhat.com Libre software enthusiast | Mesa and X developer ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [RFC] Deprecate AGP GART support for Radeon/Nouveau/TTM
Am 20.05.20 um 18:18 schrieb Alex Deucher: On Wed, May 20, 2020 at 10:43 AM Christian König wrote: Am 13.05.20 um 13:03 schrieb Christian König: Unfortunately AGP is still to widely used as we could just drop support for using its GART. Not using the AGP GART also doesn't mean a loss in functionality since drivers will just fallback to the driver specific PCI GART. For now just deprecate the code and don't enable the AGP GART in TTM even when general AGP support is available. So I've used an ancient system (32bit) to setup a test box for this. The first GPU I could test is an RV280 (Radeon 9200 PRO) which is easily 15 years old. What happens in AGP mode is that glxgears shows artifacts during rendering on this system. In PCI mode those rendering artifacts are gone and glxgears seems to draw everything correctly now. Performance is obviously not comparable, cause in AGP we don't render all triangles correctly. The second GPU I could test is an RV630 PRO (Radeon HD 2600 PRO AGP) which is more than 10 years old. As far as I can tell this one works in both AGP and PCIe mode perfectly fine. Since this is only a 32bit system I couldn't really test any OpenGL game that well. But for glxgears switching from AGP to PCIe mode seems to result in a roughly 5% performance drop. The surprising reason for this is not the better TLB performance, but the lack of USWC support for the PCIe GART in radeon. So if anybody wants to get his hands dirty and squeeze a bit more performance out of the old hardware, porting USWC from amdgpu to radeon shouldn't be to much of a problem. We do support USWC on radeon, although I think we had separate flags for cached and WC. That said we had a lot of problems with WC on 32 bit (see radeon_bo_create()). The other problem is that, at least on the really old radeons, the PCI gart didn't support snooped and unsnooped. It was always snooped. It wasn't until pcie that the gart hw got support for both. For AGP, the expectation was that AGP provided the uncached memory. Oh, indeed. I didn't remembered that. Interesting is that in this case I have no idea where the performance difference is coming from. Summing it up I'm still leaning towards disabling AGP completely by default for radeon and deprecate it in TTM as well. Thoughts? Especially Alex what do you think. Works for me. I will take that as an rb and commit at least the first patch. Thanks, Christian. Alex ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [RFC] Deprecate AGP GART support for Radeon/Nouveau/TTM
Am 20.05.20 um 18:25 schrieb Michel Dänzer: On 2020-05-20 4:43 p.m., Christian König wrote: Am 13.05.20 um 13:03 schrieb Christian König: Unfortunately AGP is still to widely used as we could just drop support for using its GART. Not using the AGP GART also doesn't mean a loss in functionality since drivers will just fallback to the driver specific PCI GART. For now just deprecate the code and don't enable the AGP GART in TTM even when general AGP support is available. So I've used an ancient system (32bit) to setup a test box for this. The first GPU I could test is an RV280 (Radeon 9200 PRO) which is easily 15 years old. What happens in AGP mode is that glxgears shows artifacts during rendering on this system. In PCI mode those rendering artifacts are gone and glxgears seems to draw everything correctly now. Performance is obviously not comparable, cause in AGP we don't render all triangles correctly. The second GPU I could test is an RV630 PRO (Radeon HD 2600 PRO AGP) which is more than 10 years old. As far as I can tell this one works in both AGP and PCIe mode perfectly fine. Since this is only a 32bit system I couldn't really test any OpenGL game that well. But for glxgears switching from AGP to PCIe mode seems to result in a roughly 5% performance drop. The surprising reason for this is not the better TLB performance, but the lack of USWC support for the PCIe GART in radeon. I suspect the main reason it's only 5% is that PCIe GART page tables are stored in VRAM, so they don't need to be fetched across the PCIe link (and presumably it has more than one TLB entry as well). The difference is much bigger with native AGP ASICs with PCI GART. Do you have some hardware you could give that a try on? I mean the first card I put into the system obviously only worked correctly with AGP disabled. While I agree that it means a performance regression, this is a rather high motivation to go ahead with at least the first patch. Christian. ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 2/2] drm/amdgpu: Advise if unable to resize BAR
Am 21.05.20 um 23:32 schrieb Alex Deucher:
On Thu, May 21, 2020 at 4:45 PM Alan Swanson wrote:
Even with the "Above 4G decoding" (or similar) BIOS option enabled,
many BIOS do not assign the PCI root bus a 64-bit address space.
If available, "MMIOH Base" and "MMIO High Size" (or similar) BIOS
options should allow mapping to the desired address spaces.
Signed-off-by: Alan Swanson
---
Useful to know why bar resizing isn't happening.
This will spam a lot of people and probably cause confusion. I'd
prefer to drop this one.
Agreed, you can just look at /proc/iomem to figure out if resources
above 4GB are available or not.
Christian.
Alex
drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 2f0e8da7b..39a7f7212 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -919,8 +919,10 @@ int amdgpu_device_resize_fb_bar(struct amdgpu_device *adev)
}
/* Trying to resize is pointless without a root hub window above 4GB */
- if (!res)
+ if (!res) {
+ DRM_INFO("Unable to resize BAR as PCI bus address space below
4GB.");
return 0;
+ }
/* Disable memory decoding while we change the BAR addresses and size
*/
pci_read_config_word(adev->pdev, PCI_COMMAND, );
--
2.26.2
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 1/3] Revert "drm/amdgpu: off by one in amdgpu_device_attr_create_groups() error handling"
Well what exactly is going wrong here?
At least this one looks correct to me, and if it blocks the revert we
should probably squash it in there as well.
Christian.
Am 21.05.20 um 22:23 schrieb Alex Deucher:
This reverts commit b41d9df2b680b96913cc3ccf929252e2dce71b24.
Signed-off-by: Alex Deucher
---
drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
index 2fc51f815eaa..72bbb8175b22 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
@@ -1843,8 +1843,9 @@ static int amdgpu_device_attr_create_groups(struct
amdgpu_device *adev,
return 0;
failed:
- while (i--)
+ for (; i > 0; i--) {
amdgpu_device_attr_remove(adev, [i]);
+ }
return ret;
}
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH] drm/amdgpu: print warning when input address is invalid
LGTM
Acked-by: Nirmoy Das
On 5/22/20 12:00 PM, Guchun Chen wrote:
This will assist debug in error injection case.
Signed-off-by: Guchun Chen
Reviewed-by: Tao Zhou
---
drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
index 50fe08bf2f72..9475891ee989 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
@@ -318,6 +318,9 @@ static ssize_t amdgpu_ras_debugfs_ctrl_write(struct file
*f, const char __user *
case 2:
if ((data.inject.address >= adev->gmc.mc_vram_size) ||
(data.inject.address >= RAS_UMC_INJECT_ADDR_LIMIT)) {
+ dev_warn(adev->dev, "RAS WARN: input address "
+ "0x%llx is invalid.",
+ data.inject.address);
ret = -EINVAL;
break;
}
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH] drm/amdgpu: print warning when input address is invalid
This will assist debug in error injection case.
Signed-off-by: Guchun Chen
Reviewed-by: Tao Zhou
---
drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
index 50fe08bf2f72..9475891ee989 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
@@ -318,6 +318,9 @@ static ssize_t amdgpu_ras_debugfs_ctrl_write(struct file
*f, const char __user *
case 2:
if ((data.inject.address >= adev->gmc.mc_vram_size) ||
(data.inject.address >= RAS_UMC_INJECT_ADDR_LIMIT)) {
+ dev_warn(adev->dev, "RAS WARN: input address "
+ "0x%llx is invalid.",
+ data.inject.address);
ret = -EINVAL;
break;
}
--
2.17.1
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 1/1] drm/amdgpu: Sync with VM root BO when switching VM to CPU update mode
Am 21.05.20 um 19:06 schrieb Felix Kuehling:
Am 2020-05-21 um 9:50 a.m. schrieb Christian König:
Am 21.05.20 um 00:51 schrieb Felix Kuehling:
This fixes an intermittent bug where a root PD clear operation still in
progress could overwrite a PDE update done by the CPU, resulting in a
VM fault.
Mhm, maybe better add this to amdgpu_vm_cpu_prepare().
This way we could (in theory) switch between CPU and SDMA based
updates on the fly elsewhere as well.
That won't work. I want to wait for FENCE_OWNER_VM fences, so I need to
use FENCE_OWNER_UNDEFINED. But then I would also end up waiting for
FENCE_OWNER_KFD eviction fences, which would trigger unwanted evictions.
This works OK in amdgpu_vm_make_compute because it runs before the
eviction fence is attached to the VM.
Ok, in this case the patch is Reviewed-by: Christian König
.
Regards,
Felix
Christian.
Fixes: 108b4d928c03 ("drm/amd/amdgpu: Update VM function pointer")
Reported-by: Jay Cornwall
Tested-by: Jay Cornwall
Signed-off-by: Felix Kuehling
---
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 11 +--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 414a0b1c2e5a..7417754e9141 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -3000,10 +3000,17 @@ int amdgpu_vm_make_compute(struct
amdgpu_device *adev, struct amdgpu_vm *vm,
!amdgpu_gmc_vram_full_visible(>gmc)),
"CPU update of VM recommended only for large BAR system\n");
- if (vm->use_cpu_for_update)
+ if (vm->use_cpu_for_update) {
+ /* Sync with last SDMA update/clear before switching to CPU */
+ r = amdgpu_bo_sync_wait(vm->root.base.bo,
+ AMDGPU_FENCE_OWNER_UNDEFINED, true);
+ if (r)
+ goto free_idr;
+
vm->update_funcs = _vm_cpu_funcs;
- else
+ } else {
vm->update_funcs = _vm_sdma_funcs;
+ }
dma_fence_put(vm->last_update);
vm->last_update = NULL;
vm->is_compute_context = true;
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
RE: [PATCH] drm/amdgpu: print warning when input address is invalid
[AMD Public Use]
Thanks Tao. I will address that when pushing.
Regards,
Guchun
-Original Message-
From: Zhou1, Tao
Sent: Friday, May 22, 2020 4:51 PM
To: Chen, Guchun ; amd-gfx@lists.freedesktop.org; Zhang,
Hawking ; Clements, John ; Li,
Dennis
Subject: RE: [PATCH] drm/amdgpu: print warning when input address is invalid
[AMD Public Use]
OK, but I still suggest adding "RAS" in the print to indicate its module, with
this addressed the patch is:
Reviewed-by: Tao Zhou
> -Original Message-
> From: Chen, Guchun
> Sent: 2020年5月22日 16:31
> To: Zhou1, Tao ; amd-gfx@lists.freedesktop.org;
> Zhang, Hawking ; Clements, John
> ; Li, Dennis
> Subject: RE: [PATCH] drm/amdgpu: print warning when input address is
> invalid
>
> [AMD Public Use]
>
> Hi Tao,
>
> Please see my response inline.
>
> Regards,
> Guchun
>
> -Original Message-
> From: Zhou1, Tao
> Sent: Friday, May 22, 2020 4:11 PM
> To: Chen, Guchun ; amd-
> g...@lists.freedesktop.org; Zhang, Hawking ;
> Clements, John ; Li, Dennis
> Subject: RE: [PATCH] drm/amdgpu: print warning when input address is
> invalid
>
> [AMD Official Use Only - Internal Distribution Only]
>
>
>
> > -Original Message-
> > From: Chen, Guchun
> > Sent: 2020年5月22日 15:53
> > To: amd-gfx@lists.freedesktop.org; Zhang, Hawking
> > ; Zhou1, Tao ;
> Clements,
> > John ; Li, Dennis
> > Cc: Chen, Guchun
> > Subject: [PATCH] drm/amdgpu: print warning when input address is
> > invalid
> >
> > This will assist debug.
> >
> > Signed-off-by: Guchun Chen
> > ---
> > drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
> > index 6e911ca97038..5c73f444eaef 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
> > @@ -318,6 +318,8 @@ static ssize_t
> > amdgpu_ras_debugfs_ctrl_write(struct
> > file *f, const char __user *
> > case 2:
> > if ((data.inject.address >= adev->gmc.mc_vram_size) ||
> > (data.inject.address >= RAS_UMC_INJECT_ADDR_LIMIT)) {
> > + dev_warn(adev->dev, "Input address 0x%llx is
> > invalid.",
> > + data.inject.address);
>
> [Tao] How about this way:
> dev_warn(adev->dev, "RAS injection address 0x%llx exceeds limit 0x%llx.",
> data.inject.address,
> RAS_UMC_INJECT_ADDR_LIMIT); [Guchun]There are two cases for invalid
> input address, one is limited by board vram size, and one is by
> RAS_UMC_INJECT_ADDR_LIMIT.
> So it's not necessary to distinguish here, just print warning to let
> user knows the input address is not correct, and this error injection is
> blocked.
>
> > ret = -EINVAL;
> > break;
> > }
> > --
> > 2.17.1
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
RE: [PATCH] drm/amdgpu: print warning when input address is invalid
[AMD Public Use]
OK, but I still suggest adding "RAS" in the print to indicate its module, with
this addressed the patch is:
Reviewed-by: Tao Zhou
> -Original Message-
> From: Chen, Guchun
> Sent: 2020年5月22日 16:31
> To: Zhou1, Tao ; amd-gfx@lists.freedesktop.org;
> Zhang, Hawking ; Clements, John
> ; Li, Dennis
> Subject: RE: [PATCH] drm/amdgpu: print warning when input address is
> invalid
>
> [AMD Public Use]
>
> Hi Tao,
>
> Please see my response inline.
>
> Regards,
> Guchun
>
> -Original Message-
> From: Zhou1, Tao
> Sent: Friday, May 22, 2020 4:11 PM
> To: Chen, Guchun ; amd-
> g...@lists.freedesktop.org; Zhang, Hawking ;
> Clements, John ; Li, Dennis
>
> Subject: RE: [PATCH] drm/amdgpu: print warning when input address is
> invalid
>
> [AMD Official Use Only - Internal Distribution Only]
>
>
>
> > -Original Message-
> > From: Chen, Guchun
> > Sent: 2020年5月22日 15:53
> > To: amd-gfx@lists.freedesktop.org; Zhang, Hawking
> > ; Zhou1, Tao ;
> Clements,
> > John ; Li, Dennis
> > Cc: Chen, Guchun
> > Subject: [PATCH] drm/amdgpu: print warning when input address is
> > invalid
> >
> > This will assist debug.
> >
> > Signed-off-by: Guchun Chen
> > ---
> > drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
> > index 6e911ca97038..5c73f444eaef 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
> > @@ -318,6 +318,8 @@ static ssize_t
> > amdgpu_ras_debugfs_ctrl_write(struct
> > file *f, const char __user *
> > case 2:
> > if ((data.inject.address >= adev->gmc.mc_vram_size) ||
> > (data.inject.address >= RAS_UMC_INJECT_ADDR_LIMIT)) {
> > + dev_warn(adev->dev, "Input address 0x%llx is
> > invalid.",
> > + data.inject.address);
>
> [Tao] How about this way:
> dev_warn(adev->dev, "RAS injection address 0x%llx exceeds limit 0x%llx.",
> data.inject.address,
> RAS_UMC_INJECT_ADDR_LIMIT); [Guchun]There are two cases for invalid
> input address, one is limited by board vram size, and one is by
> RAS_UMC_INJECT_ADDR_LIMIT.
> So it's not necessary to distinguish here, just print warning to let user
> knows
> the input address is not correct, and this error injection is blocked.
>
> > ret = -EINVAL;
> > break;
> > }
> > --
> > 2.17.1
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
RE: [PATCH] drm/amdgpu: print warning when input address is invalid
[AMD Public Use]
Hi Tao,
Please see my response inline.
Regards,
Guchun
-Original Message-
From: Zhou1, Tao
Sent: Friday, May 22, 2020 4:11 PM
To: Chen, Guchun ; amd-gfx@lists.freedesktop.org; Zhang,
Hawking ; Clements, John ; Li,
Dennis
Subject: RE: [PATCH] drm/amdgpu: print warning when input address is invalid
[AMD Official Use Only - Internal Distribution Only]
> -Original Message-
> From: Chen, Guchun
> Sent: 2020年5月22日 15:53
> To: amd-gfx@lists.freedesktop.org; Zhang, Hawking
> ; Zhou1, Tao ; Clements,
> John ; Li, Dennis
> Cc: Chen, Guchun
> Subject: [PATCH] drm/amdgpu: print warning when input address is
> invalid
>
> This will assist debug.
>
> Signed-off-by: Guchun Chen
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
> index 6e911ca97038..5c73f444eaef 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
> @@ -318,6 +318,8 @@ static ssize_t
> amdgpu_ras_debugfs_ctrl_write(struct
> file *f, const char __user *
> case 2:
> if ((data.inject.address >= adev->gmc.mc_vram_size) ||
> (data.inject.address >= RAS_UMC_INJECT_ADDR_LIMIT)) {
> + dev_warn(adev->dev, "Input address 0x%llx is
> invalid.",
> + data.inject.address);
[Tao] How about this way:
dev_warn(adev->dev, "RAS injection address 0x%llx exceeds limit 0x%llx.",
data.inject.address, RAS_UMC_INJECT_ADDR_LIMIT);
[Guchun]There are two cases for invalid input address, one is limited by board
vram size, and one is by RAS_UMC_INJECT_ADDR_LIMIT.
So it's not necessary to distinguish here, just print warning to let user knows
the input address is not correct, and this error injection is blocked.
> ret = -EINVAL;
> break;
> }
> --
> 2.17.1
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
RE: [PATCH] drm/amdgpu: print warning when input address is invalid
[AMD Official Use Only - Internal Distribution Only]
> -Original Message-
> From: Chen, Guchun
> Sent: 2020年5月22日 15:53
> To: amd-gfx@lists.freedesktop.org; Zhang, Hawking
> ; Zhou1, Tao ;
> Clements, John ; Li, Dennis
>
> Cc: Chen, Guchun
> Subject: [PATCH] drm/amdgpu: print warning when input address is invalid
>
> This will assist debug.
>
> Signed-off-by: Guchun Chen
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
> index 6e911ca97038..5c73f444eaef 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
> @@ -318,6 +318,8 @@ static ssize_t amdgpu_ras_debugfs_ctrl_write(struct
> file *f, const char __user *
> case 2:
> if ((data.inject.address >= adev->gmc.mc_vram_size) ||
> (data.inject.address >= RAS_UMC_INJECT_ADDR_LIMIT)) {
> + dev_warn(adev->dev, "Input address 0x%llx is
> invalid.",
> + data.inject.address);
[Tao] How about this way:
dev_warn(adev->dev, "RAS injection address 0x%llx exceeds limit 0x%llx.",
data.inject.address, RAS_UMC_INJECT_ADDR_LIMIT);
> ret = -EINVAL;
> break;
> }
> --
> 2.17.1
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH] drm/amdgpu: print warning when input address is invalid
This will assist debug.
Signed-off-by: Guchun Chen
---
drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
index 6e911ca97038..5c73f444eaef 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
@@ -318,6 +318,8 @@ static ssize_t amdgpu_ras_debugfs_ctrl_write(struct file
*f, const char __user *
case 2:
if ((data.inject.address >= adev->gmc.mc_vram_size) ||
(data.inject.address >= RAS_UMC_INJECT_ADDR_LIMIT)) {
+ dev_warn(adev->dev, "Input address 0x%llx is invalid.",
+ data.inject.address);
ret = -EINVAL;
break;
}
--
2.17.1
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 1/3] drm/amdgpu: move discovery gfx config fetching
Acked-by: Christian König
Am 22.05.20 um 08:06 schrieb Quan, Evan:
[AMD Official Use Only - Internal Distribution Only]
Patch1, 2 are reviewed-by: Evan Quan
Patch3 is acked-by: Evan Quan
-Original Message-
From: amd-gfx On Behalf Of Alex Deucher
Sent: Thursday, May 21, 2020 10:16 PM
To: amd-gfx list
Cc: Deucher, Alexander
Subject: Re: [PATCH 1/3] drm/amdgpu: move discovery gfx config fetching
Ping on this series? It fixes an ordering issue for raven2.
Alex
On Fri, May 15, 2020 at 2:31 PM Alex Deucher wrote:
Move it into the fw_info function since it's logically part of the
same functionality.
Signed-off-by: Alex Deucher
---
drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 7 +++
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index cc41e8f5ad14..bab1be7abdf0 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -1617,8 +1617,10 @@ static int amdgpu_device_parse_gpu_info_fw(struct
amdgpu_device *adev)
(const struct gpu_info_firmware_v1_0
*)(adev->firmware.gpu_info_fw->data +
le32_to_cpu(hdr->header.ucode_array_offset_bytes));
- if (amdgpu_discovery && adev->asic_type >= CHIP_NAVI10)
+ if (amdgpu_discovery && adev->asic_type >= CHIP_NAVI10) {
+ amdgpu_discovery_get_gfx_info(adev);
goto parse_soc_bounding_box;
+ }
adev->gfx.config.max_shader_engines =
le32_to_cpu(gpu_info_fw->gc_num_se);
adev->gfx.config.max_cu_per_sh =
le32_to_cpu(gpu_info_fw->gc_num_cu_per_sh);
@@ -1768,9 +1770,6 @@ static int amdgpu_device_ip_early_init(struct
amdgpu_device *adev)
if (r)
return r;
- if (amdgpu_discovery && adev->asic_type >= CHIP_NAVI10)
- amdgpu_discovery_get_gfx_info(adev);
-
amdgpu_amdkfd_device_probe(adev);
if (amdgpu_sriov_vf(adev)) {
--
2.25.4
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfxdata=02%7C01%7Cevan.quan%40amd.com%7Cb692d163dea04a8d9f3208d7fd918cbd%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637256673921983400sdata=mkNxi6pmkJCpIWNmzHhdmMk6%2BcYR%2BAYJcwwCvoDhlqs%3Dreserved=0
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH] drm/radeon/dpm: Replace one-element array and use struct_size() helper
Am 22.05.20 um 03:25 schrieb Gustavo A. R. Silva:
The current codebase makes use of one-element arrays in the following
form:
struct something {
int length;
u8 data[1];
};
struct something *instance;
instance = kmalloc(sizeof(*instance) + size, GFP_KERNEL);
instance->length = size;
memcpy(instance->data, source, size);
but the preferred mechanism to declare variable-length types such as
these ones is a flexible array member[1][2], introduced in C99:
struct foo {
int stuff;
struct boo array[];
};
By making use of the mechanism above, we will get a compiler warning
in case the flexible array does not occur last in the structure, which
will help us prevent some kind of undefined behavior bugs from being
inadvertently introduced[3] to the codebase from now on. So, replace
the one-element array with a flexible-array member.
Also, make use of the new struct_size() helper to properly calculate the
size of struct NISLANDS_SMC_SWSTATE.
This issue was found with the help of Coccinelle and, audited and fixed
_manually_.
[1]
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgcc.gnu.org%2Fonlinedocs%2Fgcc%2FZero-Length.htmldata=02%7C01%7Cchristian.koenig%40amd.com%7C7dd54e944eff4d17178008d7fdee62d7%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637257072615411745sdata=fQHnbXZpsgiMjHOr%2By0Uq12jpCYEYbdX5K26iNkwyeM%3Dreserved=0
[2]
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FKSPP%2Flinux%2Fissues%2F21data=02%7C01%7Cchristian.koenig%40amd.com%7C7dd54e944eff4d17178008d7fdee62d7%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637257072615411745sdata=gg20YupmqsaW%2Bg3VyJL%2BkjE3kFwnWF1RD1D2QP04OLk%3Dreserved=0
[3] commit 76497732932f ("cxgb3/l2t: Fix undefined behaviour")
Signed-off-by: Gustavo A. R. Silva
---
drivers/gpu/drm/amd/amdgpu/si_dpm.h | 2 +-
drivers/gpu/drm/radeon/ni_dpm.c | 7 ---
2 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/si_dpm.h
b/drivers/gpu/drm/amd/amdgpu/si_dpm.h
index 6b7d292b919f3..bc0be6818e218 100644
--- a/drivers/gpu/drm/amd/amdgpu/si_dpm.h
+++ b/drivers/gpu/drm/amd/amdgpu/si_dpm.h
@@ -781,7 +781,7 @@ struct NISLANDS_SMC_SWSTATE
uint8_t levelCount;
uint8_t padding2;
uint8_t padding3;
-NISLANDS_SMC_HW_PERFORMANCE_LEVEL levels[1];
+NISLANDS_SMC_HW_PERFORMANCE_LEVEL levels[];
};
typedef struct NISLANDS_SMC_SWSTATE NISLANDS_SMC_SWSTATE;
diff --git a/drivers/gpu/drm/radeon/ni_dpm.c b/drivers/gpu/drm/radeon/ni_dpm.c
index b57c37ddd164c..7d81dde509dc9 100644
--- a/drivers/gpu/drm/radeon/ni_dpm.c
+++ b/drivers/gpu/drm/radeon/ni_dpm.c
@@ -2685,11 +2685,12 @@ static int ni_upload_sw_state(struct radeon_device
*rdev,
struct rv7xx_power_info *pi = rv770_get_pi(rdev);
u16 address = pi->state_table_start +
offsetof(NISLANDS_SMC_STATETABLE, driverState);
- u16 state_size = sizeof(NISLANDS_SMC_SWSTATE) +
- ((NISLANDS_MAX_SMC_PERFORMANCE_LEVELS_PER_SWSTATE - 1) *
sizeof(NISLANDS_SMC_HW_PERFORMANCE_LEVEL));
+ NISLANDS_SMC_SWSTATE *smc_state;
+ u16 state_size = struct_size(smc_state, levels,
+ NISLANDS_MAX_SMC_PERFORMANCE_LEVELS_PER_SWSTATE);
Probably better to use size_t instead of u16 here. With that fixed feel
free to stick my rb on the patch.
Regards,
Christian.
int ret;
- NISLANDS_SMC_SWSTATE *smc_state = kzalloc(state_size, GFP_KERNEL);
+ smc_state = kzalloc(state_size, GFP_KERNEL);
if (smc_state == NULL)
return -ENOMEM;
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
RE: [PATCH] drm/amd/amdgpu: Fix the CGCG setting is overwritten for SRIOV.
[AMD Public Use]
Or make it in more reasonable place.
Regards,
Hawking
-Original Message-
From: Zhang, Hawking
Sent: Friday, May 22, 2020 14:16
To: Liu, Monk ; Chen, Guchun ; Wan,
Gavin ; amd-gfx@lists.freedesktop.org
Cc: Wan, Gavin
Subject: RE: [PATCH] drm/amd/amdgpu: Fix the CGCG setting is overwritten for
SRIOV.
[AMD Public Use]
Yes, please try best effort to not introduce guest/one_vf/mult_vf check.
Regards,
Hawking
-Original Message-
From: Liu, Monk
Sent: Friday, May 22, 2020 14:12
To: Liu, Monk ; Zhang, Hawking ; Chen,
Guchun ; Wan, Gavin ;
amd-gfx@lists.freedesktop.org
Cc: Wan, Gavin
Subject: RE: [PATCH] drm/amd/amdgpu: Fix the CGCG setting is overwritten for
SRIOV.
Gavin
Looks the only place you need to change is the part of avoid touching
"CP_INT_CNTL_RING0" which is handled by GIM now
Others looks not needed at all
_
Monk Liu|GPU Virtualization Team |AMD
-Original Message-
From: amd-gfx On Behalf Of Liu, Monk
Sent: Friday, May 22, 2020 1:52 PM
To: Zhang, Hawking ; Chen, Guchun ;
Wan, Gavin ; amd-gfx@lists.freedesktop.org
Cc: Wan, Gavin
Subject: RE: [PATCH] drm/amd/amdgpu: Fix the CGCG setting is overwritten for
SRIOV.
Sounds a good idea
@Wan, Gavin can you try hawking's advice ?
_
Monk Liu|GPU Virtualization Team |AMD
-Original Message-
From: amd-gfx On Behalf Of Zhang,
Hawking
Sent: Friday, May 22, 2020 1:09 PM
To: Chen, Guchun ; Wan, Gavin ;
amd-gfx@lists.freedesktop.org
Cc: Wan, Gavin
Subject: RE: [PATCH] drm/amd/amdgpu: Fix the CGCG setting is overwritten for
SRIOV.
[AMD Public Use]
Can we leverage existing CG flags to control this rather than add
amdgpu_sriov_vf(adev) check everywhere?
If GC CG feature is programmed by host. We can just mask out the following
flags for guest driver case (amdgpu_sriov_vf(adev)).
AMD_CG_SUPPORT_GFX_MGCG |
AMD_CG_SUPPORT_GFX_CGLS |
AMD_CG_SUPPORT_GFX_CGCG |
AMD_CG_SUPPORT_GFX_CGLS |
AMD_CG_SUPPORT_GFX_3D_CGCG |
AMD_CG_SUPPORT_GFX_3D_CGLS
There are too many amdgpu_sriov_vf(adev) Check in amdgpu driver, which actually
add unnecessary sustaining effort.
Regards,
Hawking
-Original Message-
From: amd-gfx On Behalf Of Chen, Guchun
Sent: Friday, May 22, 2020 11:47
To: Wan, Gavin ; amd-gfx@lists.freedesktop.org
Cc: Wan, Gavin
Subject: RE: [PATCH] drm/amd/amdgpu: Fix the CGCG setting is overwritten for
SRIOV.
[AMD Public Use]
Please see one comment below.
Regards,
Guchun
-Original Message-
From: amd-gfx On Behalf Of Gavin Wan
Sent: Friday, May 22, 2020 3:53 AM
To: amd-gfx@lists.freedesktop.org
Cc: Wan, Gavin
Subject: [PATCH] drm/amd/amdgpu: Fix the CGCG setting is overwritten for SRIOV.
For SRIOV, since the CGCG is set on host side. The Guest should not program
CGCG again.
The patch ignores setting CGCG for SRIOV.
Change-Id: Ic336fab3b23b8371c9e9e192182a3ba14a8db8e1
Signed-off-by: Gavin Wan
---
drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c | 15 +++
1 file changed, 15 insertions(+)
diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
b/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
index bd5dd4f64311..52b6e4759cf3 100644
--- a/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
@@ -4558,6 +4558,9 @@ static void gfx_v10_0_constants_init(struct amdgpu_device
*adev) static void gfx_v10_0_enable_gui_idle_interrupt(struct amdgpu_device
*adev,
bool enable)
{
+ if (amdgpu_sriov_vf(adev))
+ return;
+
[Guchun]This coding style is not correct. You should put the check after the
declare of 'u32 tmp'.
Maybe it's better to split below line to declare and execution parts
respectively.
u32 tmp = RREG32_SOC15(GC, 0, mmCP_INT_CNTL_RING0);
tmp = REG_SET_FIELD(tmp, CP_INT_CNTL_RING0, CNTX_BUSY_INT_ENABLE, @@
-6842,6 +6845,9 @@ static void
gfx_v10_0_update_medium_grain_clock_gating(struct amdgpu_device *ade {
uint32_t data, def;
+ if (amdgpu_sriov_vf(adev))
+ return;
+
/* It is disabled by HW by default */
if (enable && (adev->cg_flags & AMD_CG_SUPPORT_GFX_MGCG)) {
/* 0 - Disable some blocks' MGCG */
@@ -6911,6 +6917,9 @@ static void gfx_v10_0_update_3d_clock_gating(struct
amdgpu_device *adev, {
uint32_t data, def;
+ if (amdgpu_sriov_vf(adev))
+ return;
+
/* Enable 3D CGCG/CGLS */
if (enable && (adev->cg_flags & AMD_CG_SUPPORT_GFX_3D_CGCG)) {
/* write cmd to clear cgcg/cgls ov */ @@ -6953,6 +6962,9 @@
static void gfx_v10_0_update_coarse_grain_clock_gating(struct amdgpu_device
*ade {
uint32_t def, data;
+ if (amdgpu_sriov_vf(adev))
+ return;
+
if (enable && (adev->cg_flags & AMD_CG_SUPPORT_GFX_CGCG)) {
def = data = RREG32_SOC15(GC, 0,
RE: [PATCH] drm/amd/amdgpu: Fix the CGCG setting is overwritten for SRIOV.
[AMD Public Use]
Yes, please try best effort to not introduce guest/one_vf/mult_vf check.
Regards,
Hawking
-Original Message-
From: Liu, Monk
Sent: Friday, May 22, 2020 14:12
To: Liu, Monk ; Zhang, Hawking ; Chen,
Guchun ; Wan, Gavin ;
amd-gfx@lists.freedesktop.org
Cc: Wan, Gavin
Subject: RE: [PATCH] drm/amd/amdgpu: Fix the CGCG setting is overwritten for
SRIOV.
Gavin
Looks the only place you need to change is the part of avoid touching
"CP_INT_CNTL_RING0" which is handled by GIM now
Others looks not needed at all
_
Monk Liu|GPU Virtualization Team |AMD
-Original Message-
From: amd-gfx On Behalf Of Liu, Monk
Sent: Friday, May 22, 2020 1:52 PM
To: Zhang, Hawking ; Chen, Guchun ;
Wan, Gavin ; amd-gfx@lists.freedesktop.org
Cc: Wan, Gavin
Subject: RE: [PATCH] drm/amd/amdgpu: Fix the CGCG setting is overwritten for
SRIOV.
Sounds a good idea
@Wan, Gavin can you try hawking's advice ?
_
Monk Liu|GPU Virtualization Team |AMD
-Original Message-
From: amd-gfx On Behalf Of Zhang,
Hawking
Sent: Friday, May 22, 2020 1:09 PM
To: Chen, Guchun ; Wan, Gavin ;
amd-gfx@lists.freedesktop.org
Cc: Wan, Gavin
Subject: RE: [PATCH] drm/amd/amdgpu: Fix the CGCG setting is overwritten for
SRIOV.
[AMD Public Use]
Can we leverage existing CG flags to control this rather than add
amdgpu_sriov_vf(adev) check everywhere?
If GC CG feature is programmed by host. We can just mask out the following
flags for guest driver case (amdgpu_sriov_vf(adev)).
AMD_CG_SUPPORT_GFX_MGCG |
AMD_CG_SUPPORT_GFX_CGLS |
AMD_CG_SUPPORT_GFX_CGCG |
AMD_CG_SUPPORT_GFX_CGLS |
AMD_CG_SUPPORT_GFX_3D_CGCG |
AMD_CG_SUPPORT_GFX_3D_CGLS
There are too many amdgpu_sriov_vf(adev) Check in amdgpu driver, which actually
add unnecessary sustaining effort.
Regards,
Hawking
-Original Message-
From: amd-gfx On Behalf Of Chen, Guchun
Sent: Friday, May 22, 2020 11:47
To: Wan, Gavin ; amd-gfx@lists.freedesktop.org
Cc: Wan, Gavin
Subject: RE: [PATCH] drm/amd/amdgpu: Fix the CGCG setting is overwritten for
SRIOV.
[AMD Public Use]
Please see one comment below.
Regards,
Guchun
-Original Message-
From: amd-gfx On Behalf Of Gavin Wan
Sent: Friday, May 22, 2020 3:53 AM
To: amd-gfx@lists.freedesktop.org
Cc: Wan, Gavin
Subject: [PATCH] drm/amd/amdgpu: Fix the CGCG setting is overwritten for SRIOV.
For SRIOV, since the CGCG is set on host side. The Guest should not program
CGCG again.
The patch ignores setting CGCG for SRIOV.
Change-Id: Ic336fab3b23b8371c9e9e192182a3ba14a8db8e1
Signed-off-by: Gavin Wan
---
drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c | 15 +++
1 file changed, 15 insertions(+)
diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
b/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
index bd5dd4f64311..52b6e4759cf3 100644
--- a/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
@@ -4558,6 +4558,9 @@ static void gfx_v10_0_constants_init(struct amdgpu_device
*adev) static void gfx_v10_0_enable_gui_idle_interrupt(struct amdgpu_device
*adev,
bool enable)
{
+ if (amdgpu_sriov_vf(adev))
+ return;
+
[Guchun]This coding style is not correct. You should put the check after the
declare of 'u32 tmp'.
Maybe it's better to split below line to declare and execution parts
respectively.
u32 tmp = RREG32_SOC15(GC, 0, mmCP_INT_CNTL_RING0);
tmp = REG_SET_FIELD(tmp, CP_INT_CNTL_RING0, CNTX_BUSY_INT_ENABLE, @@
-6842,6 +6845,9 @@ static void
gfx_v10_0_update_medium_grain_clock_gating(struct amdgpu_device *ade {
uint32_t data, def;
+ if (amdgpu_sriov_vf(adev))
+ return;
+
/* It is disabled by HW by default */
if (enable && (adev->cg_flags & AMD_CG_SUPPORT_GFX_MGCG)) {
/* 0 - Disable some blocks' MGCG */
@@ -6911,6 +6917,9 @@ static void gfx_v10_0_update_3d_clock_gating(struct
amdgpu_device *adev, {
uint32_t data, def;
+ if (amdgpu_sriov_vf(adev))
+ return;
+
/* Enable 3D CGCG/CGLS */
if (enable && (adev->cg_flags & AMD_CG_SUPPORT_GFX_3D_CGCG)) {
/* write cmd to clear cgcg/cgls ov */ @@ -6953,6 +6962,9 @@
static void gfx_v10_0_update_coarse_grain_clock_gating(struct amdgpu_device
*ade {
uint32_t def, data;
+ if (amdgpu_sriov_vf(adev))
+ return;
+
if (enable && (adev->cg_flags & AMD_CG_SUPPORT_GFX_CGCG)) {
def = data = RREG32_SOC15(GC, 0, mmRLC_CGTT_MGCG_OVERRIDE);
/* unset CGCG override */
@@ -6994,6 +7006,9 @@ static void
gfx_v10_0_update_coarse_grain_clock_gating(struct amdgpu_device *ade static
int gfx_v10_0_update_gfx_clock_gating(struct amdgpu_device *adev,
bool enable)
{
+ if (amdgpu_sriov_vf(adev))
RE: [PATCH] drm/amd/amdgpu: Fix the CGCG setting is overwritten for SRIOV.
Gavin
Looks the only place you need to change is the part of avoid touching
"CP_INT_CNTL_RING0" which is handled by GIM now
Others looks not needed at all
_
Monk Liu|GPU Virtualization Team |AMD
-Original Message-
From: amd-gfx On Behalf Of Liu, Monk
Sent: Friday, May 22, 2020 1:52 PM
To: Zhang, Hawking ; Chen, Guchun ;
Wan, Gavin ; amd-gfx@lists.freedesktop.org
Cc: Wan, Gavin
Subject: RE: [PATCH] drm/amd/amdgpu: Fix the CGCG setting is overwritten for
SRIOV.
Sounds a good idea
@Wan, Gavin can you try hawking's advice ?
_
Monk Liu|GPU Virtualization Team |AMD
-Original Message-
From: amd-gfx On Behalf Of Zhang,
Hawking
Sent: Friday, May 22, 2020 1:09 PM
To: Chen, Guchun ; Wan, Gavin ;
amd-gfx@lists.freedesktop.org
Cc: Wan, Gavin
Subject: RE: [PATCH] drm/amd/amdgpu: Fix the CGCG setting is overwritten for
SRIOV.
[AMD Public Use]
Can we leverage existing CG flags to control this rather than add
amdgpu_sriov_vf(adev) check everywhere?
If GC CG feature is programmed by host. We can just mask out the following
flags for guest driver case (amdgpu_sriov_vf(adev)).
AMD_CG_SUPPORT_GFX_MGCG |
AMD_CG_SUPPORT_GFX_CGLS |
AMD_CG_SUPPORT_GFX_CGCG |
AMD_CG_SUPPORT_GFX_CGLS |
AMD_CG_SUPPORT_GFX_3D_CGCG |
AMD_CG_SUPPORT_GFX_3D_CGLS
There are too many amdgpu_sriov_vf(adev) Check in amdgpu driver, which actually
add unnecessary sustaining effort.
Regards,
Hawking
-Original Message-
From: amd-gfx On Behalf Of Chen, Guchun
Sent: Friday, May 22, 2020 11:47
To: Wan, Gavin ; amd-gfx@lists.freedesktop.org
Cc: Wan, Gavin
Subject: RE: [PATCH] drm/amd/amdgpu: Fix the CGCG setting is overwritten for
SRIOV.
[AMD Public Use]
Please see one comment below.
Regards,
Guchun
-Original Message-
From: amd-gfx On Behalf Of Gavin Wan
Sent: Friday, May 22, 2020 3:53 AM
To: amd-gfx@lists.freedesktop.org
Cc: Wan, Gavin
Subject: [PATCH] drm/amd/amdgpu: Fix the CGCG setting is overwritten for SRIOV.
For SRIOV, since the CGCG is set on host side. The Guest should not program
CGCG again.
The patch ignores setting CGCG for SRIOV.
Change-Id: Ic336fab3b23b8371c9e9e192182a3ba14a8db8e1
Signed-off-by: Gavin Wan
---
drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c | 15 +++
1 file changed, 15 insertions(+)
diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
b/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
index bd5dd4f64311..52b6e4759cf3 100644
--- a/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
@@ -4558,6 +4558,9 @@ static void gfx_v10_0_constants_init(struct amdgpu_device
*adev) static void gfx_v10_0_enable_gui_idle_interrupt(struct amdgpu_device
*adev,
bool enable)
{
+ if (amdgpu_sriov_vf(adev))
+ return;
+
[Guchun]This coding style is not correct. You should put the check after the
declare of 'u32 tmp'.
Maybe it's better to split below line to declare and execution parts
respectively.
u32 tmp = RREG32_SOC15(GC, 0, mmCP_INT_CNTL_RING0);
tmp = REG_SET_FIELD(tmp, CP_INT_CNTL_RING0, CNTX_BUSY_INT_ENABLE, @@
-6842,6 +6845,9 @@ static void
gfx_v10_0_update_medium_grain_clock_gating(struct amdgpu_device *ade {
uint32_t data, def;
+ if (amdgpu_sriov_vf(adev))
+ return;
+
/* It is disabled by HW by default */
if (enable && (adev->cg_flags & AMD_CG_SUPPORT_GFX_MGCG)) {
/* 0 - Disable some blocks' MGCG */
@@ -6911,6 +6917,9 @@ static void gfx_v10_0_update_3d_clock_gating(struct
amdgpu_device *adev, {
uint32_t data, def;
+ if (amdgpu_sriov_vf(adev))
+ return;
+
/* Enable 3D CGCG/CGLS */
if (enable && (adev->cg_flags & AMD_CG_SUPPORT_GFX_3D_CGCG)) {
/* write cmd to clear cgcg/cgls ov */ @@ -6953,6 +6962,9 @@
static void gfx_v10_0_update_coarse_grain_clock_gating(struct amdgpu_device
*ade {
uint32_t def, data;
+ if (amdgpu_sriov_vf(adev))
+ return;
+
if (enable && (adev->cg_flags & AMD_CG_SUPPORT_GFX_CGCG)) {
def = data = RREG32_SOC15(GC, 0, mmRLC_CGTT_MGCG_OVERRIDE);
/* unset CGCG override */
@@ -6994,6 +7006,9 @@ static void
gfx_v10_0_update_coarse_grain_clock_gating(struct amdgpu_device *ade static
int gfx_v10_0_update_gfx_clock_gating(struct amdgpu_device *adev,
bool enable)
{
+ if (amdgpu_sriov_vf(adev))
+ return;
+
amdgpu_gfx_rlc_enter_safe_mode(adev);
if (enable) {
--
2.25.1
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
RE: [PATCH 1/3] drm/amdgpu: move discovery gfx config fetching
[AMD Official Use Only - Internal Distribution Only]
Patch1, 2 are reviewed-by: Evan Quan
Patch3 is acked-by: Evan Quan
-Original Message-
From: amd-gfx On Behalf Of Alex Deucher
Sent: Thursday, May 21, 2020 10:16 PM
To: amd-gfx list
Cc: Deucher, Alexander
Subject: Re: [PATCH 1/3] drm/amdgpu: move discovery gfx config fetching
Ping on this series? It fixes an ordering issue for raven2.
Alex
On Fri, May 15, 2020 at 2:31 PM Alex Deucher wrote:
>
> Move it into the fw_info function since it's logically part of the
> same functionality.
>
> Signed-off-by: Alex Deucher
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 7 +++
> 1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index cc41e8f5ad14..bab1be7abdf0 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -1617,8 +1617,10 @@ static int amdgpu_device_parse_gpu_info_fw(struct
> amdgpu_device *adev)
> (const struct gpu_info_firmware_v1_0
> *)(adev->firmware.gpu_info_fw->data +
>
> le32_to_cpu(hdr->header.ucode_array_offset_bytes));
>
> - if (amdgpu_discovery && adev->asic_type >= CHIP_NAVI10)
> + if (amdgpu_discovery && adev->asic_type >= CHIP_NAVI10) {
> + amdgpu_discovery_get_gfx_info(adev);
> goto parse_soc_bounding_box;
> + }
>
> adev->gfx.config.max_shader_engines =
> le32_to_cpu(gpu_info_fw->gc_num_se);
> adev->gfx.config.max_cu_per_sh =
> le32_to_cpu(gpu_info_fw->gc_num_cu_per_sh);
> @@ -1768,9 +1770,6 @@ static int amdgpu_device_ip_early_init(struct
> amdgpu_device *adev)
> if (r)
> return r;
>
> - if (amdgpu_discovery && adev->asic_type >= CHIP_NAVI10)
> - amdgpu_discovery_get_gfx_info(adev);
> -
> amdgpu_amdkfd_device_probe(adev);
>
> if (amdgpu_sriov_vf(adev)) {
> --
> 2.25.4
>
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfxdata=02%7C01%7Cevan.quan%40amd.com%7Cb692d163dea04a8d9f3208d7fd918cbd%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637256673921983400sdata=mkNxi6pmkJCpIWNmzHhdmMk6%2BcYR%2BAYJcwwCvoDhlqs%3Dreserved=0
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
