Re: [PATCH v6 09/12] x86/process: Clear PASID state for a newly forked/cloned thread
On Mon, Jul 13, 2020 at 4:48 PM Fenghua Yu wrote:
>
> The PASID state has to be cleared on forks, since the child has a
> different address space. The PASID is also cleared for thread clone. While
> it would be correct to inherit the PASID in this case, it is unknown
> whether the new task will use ENQCMD. Giving it the PASID "just in case"
> would have the downside of increased context switch overhead to setting
> the PASID MSR.
>
> Since #GP faults have to be handled on any threads that were created before
> the PASID was assigned to the mm of the process, newly created threads
> might as well be treated in a consistent way.
Just how much context switch overhead are we talking about here?
Unless the CPU works differently than I expect, I would guess you are
saving exactly zero cycles. What am I missing?
--Andy
>
> Suggested-by: Thomas Gleixner
> Signed-off-by: Fenghua Yu
> Reviewed-by: Tony Luck
> ---
> v2:
> - Modify init_task_pasid().
>
> arch/x86/kernel/process.c | 18 ++
> 1 file changed, 18 insertions(+)
>
> diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
> index f362ce0d5ac0..1b1492e337a6 100644
> --- a/arch/x86/kernel/process.c
> +++ b/arch/x86/kernel/process.c
> @@ -121,6 +121,21 @@ static int set_new_tls(struct task_struct *p, unsigned
> long tls)
> return do_set_thread_area_64(p, ARCH_SET_FS, tls);
> }
>
> +/* Initialize the PASID state for the forked/cloned thread. */
> +static void init_task_pasid(struct task_struct *task)
> +{
> + struct ia32_pasid_state *ppasid;
> +
> + /*
> +* Initialize the PASID state so that the PASID MSR will be
> +* initialized to its initial state (0) by XRSTORS when the task is
> +* scheduled for the first time.
> +*/
> + ppasid = get_xsave_addr(>thread.fpu.state.xsave,
> XFEATURE_PASID);
> + if (ppasid)
> + ppasid->pasid = INIT_PASID;
> +}
> +
> int copy_thread_tls(unsigned long clone_flags, unsigned long sp,
> unsigned long arg, struct task_struct *p, unsigned long
> tls)
> {
> @@ -174,6 +189,9 @@ int copy_thread_tls(unsigned long clone_flags, unsigned
> long sp,
> task_user_gs(p) = get_user_gs(current_pt_regs());
> #endif
>
> + if (static_cpu_has(X86_FEATURE_ENQCMD))
> + init_task_pasid(p);
> +
> /* Set a new TLS for the child thread? */
> if (clone_flags & CLONE_SETTLS)
> ret = set_new_tls(p, tls);
> --
> 2.19.1
>
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v6 12/12] x86/traps: Fix up invalid PASID
On Mon, Jul 13, 2020 at 4:48 PM Fenghua Yu wrote: > > A #GP fault is generated when ENQCMD instruction is executed without > a valid PASID value programmed in the current thread's PASID MSR. The > #GP fault handler will initialize the MSR if a PASID has been allocated > for this process. Let's take a step back here. Why are we trying to avoid IPIs? If you call munmap(), you IPI other CPUs running tasks in the current mm. If you do perf_event_open() and thus acquire RDPMC permission, you IPI other CPUs running tasks in the current mm. If you call modify_ldt(), you IPI other CPUs running tasks in the current mm. These events can all happen more than once per process. Now we have ENQCMD. An mm can be assigned a PASID *once* in the model that these patches support. Why not just send an IPI using essentially identical code to the LDT sync or the CR4.PCE sync? --Andy ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v6 12/12] x86/traps: Fix up invalid PASID
On Mon, Jul 13, 2020 at 4:48 PM Fenghua Yu wrote:
>
> A #GP fault is generated when ENQCMD instruction is executed without
> a valid PASID value programmed in the current thread's PASID MSR. The
> #GP fault handler will initialize the MSR if a PASID has been allocated
> for this process.
>
> Decoding the user instruction is ugly and sets a bad architecture
> precedent. It may not function if the faulting instruction is modified
> after #GP.
>
> Thomas suggested to provide a reason for the #GP caused by executing ENQCMD
> without a valid PASID value programmed. #GP error codes are 16 bits and all
> 16 bits are taken. Refer to SDM Vol 3, Chapter 16.13 for details. The other
> choice was to reflect the error code in an MSR. ENQCMD can also cause #GP
> when loading from the source operand, so its not fully comprehending all
> the reasons. Rather than special case the ENQCMD, in future Intel may
> choose a different fault mechanism for such cases if recovery is needed on
> #GP.
Decoding the user instruction is ugly and sets a bad architecture
precedent, but we already do it in #GP for UMIP. So I'm unconvinced.
Memo to Intel, though: you REALLY need to start thinking about what
the heck an OS is supposed to do with all these new faults you're
coming up with. The new #NM for TILE is utterly nonsensical. Sure,
it works for an OS that does not use CR0.TS and as long as no one
tries to extend the same mechanism for some new optional piece of
state, but as soon as Intel tries to use the same mechanism for
anything else, it falls apart.
Please do better.
> +
> +/*
> + * Write the current task's PASID MSR/state. This is called only when PASID
> + * is enabled.
> + */
> +static void fpu__pasid_write(u32 pasid)
> +{
> + u64 msr_val = pasid | MSR_IA32_PASID_VALID;
> +
> + fpregs_lock();
> +
> + /*
> +* If the MSR is active and owned by the current task's FPU, it can
> +* be directly written.
> +*
> +* Otherwise, write the fpstate.
> +*/
> + if (!test_thread_flag(TIF_NEED_FPU_LOAD)) {
> + wrmsrl(MSR_IA32_PASID, msr_val);
> + } else {
> + struct ia32_pasid_state *ppasid_state;
> +
> + ppasid_state =
> get_xsave_addr(>thread.fpu.state.xsave,
> + XFEATURE_PASID);
> + /*
> +* ppasid_state shouldn't be NULL because XFEATURE_PASID
> +* is enabled.
> +*/
> + WARN_ON_ONCE(!ppasid_state);
> + ppasid_state->pasid = msr_val;
WARN instead of BUG is nice, but you'll immediate oops if this fails.
How about:
if (!WARN_ON_ONCE(!ppasid_state))
ppasid_state->pasid = msr_val;
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH] drm/amdgpu: Fix regression in adjusting power table/profile
My bisect resulted in the same conclusion, that the problem began with
edad8312cbbf9a33c86873fc4093664f150dd5c1.
That commit has a LOT of changes, so I'm having problems following what
might be relevant, so in case Hawking or Dennis have any insight they
could contribute towards letting us know where to look, I've added them
to the CC list.
If you guys know why those GPU reset changes would have effected the
sysfs interfaces in this way, it could save me a bunch of investigation
time.
Thanks in advance,
Matt
On 7/31/20 2:20 PM, Paweł Gronowski wrote:
> Hello again,
>
> I just finished a bisect of amd-staging-drm-next and it looks like
> the hang is first introduced in edad8312cbbf9a33c86873fc4093664f150dd5c1
> ("drm/amdgpu: fix system hang issue during GPU reset").
>
> It is a bit tricky, because it is commited on top of my first faulty patch
> 7173949df45482 ("drm/amdgpu: Fix NULL dereference in dpm sysfs handlers") so
> it needs to be reverted fix the premature -INVAL.
>
> Test case:
> sudo sh -c 'echo "s 0 305 750" >
> /sys/class/drm/card0/device/pp_od_clk_voltage'
> Results:
> edad8312cbbf9a3 + revert 7173949df45482 = hang
> edad8312cbbf9a3~1 + revert 7173949df45482 = no hang
>
> Could you confirm that you get the same results?
>
> Thanks,
> Paweł Gronowski
>
>
> On Fri, Jul 31, 2020 at 03:34:40PM +0200, Paweł Gronowski wrote:
>> Hey Matt,
>>
>> I have just tested the amd-staging-drm-next branch
>> (dd654c76d6e854afad716ded899e4404734aaa10) with my patches reverted
>> and I can reproduce your issue with:
>>
>> sudo sh -c 'echo "s 0 305 750" >
>> /sys/class/drm/card0/device/pp_od_clk_voltage'
>>
>> Which makes the sh hang with 100% usage.
>>
>> The issue does not happen on the mainline
>> (d8b9faec54ae4bc2fff68bcd0befa93ace8256ce)
>> both without and with my patches reapplied.
>> So the problem must be related to some commit that is present in the
>> amd-staging-drm-next but not in the mainline.
>>
>>
>> Paweł Gronowski
>>
>> On Thu, Jul 30, 2020 at 06:34:14PM -0600, Matt Coffin wrote:
>>> Hey Pawel,
>>>
>>> I did confirm that this patch *introduced* the issue both with the
>>> bisect, and by testing reverting it.
>>>
>>> Now, there's a lot of fragile pieces in the dpm handling, so it could be
>>> this patch's interaction with something else that's causing it and it
>>> may well not be the fault of this code, but this is the patch that
>>> introduced the issue.
>>>
>>> I'll have some more time tomorrow to try to get down to root cause here,
>>> so maybe I'll have more to offer then.
>>>
>>> Thanks for taking a look,
>>> Matt
>>>
>>> On 7/30/20 6:31 PM, Paweł Gronowski wrote:
Hello Matt,
Thank you for your testing. It seems that my gpu (RX 570) does not support
the
vc setting so I can not exactly reproduce the issue. However I did trace
the
code path the test case takes and it seems to correctly pass through the
while
loop that parses the input and fails only in amdgpu_dpm_odn_edit_dpm_table.
The 'parameter' array is populated the same way as the original code did.
Since
the amdgpu_dpm_odn_edit_dpm_table is reached, I think that your problem is
unfortunately caused by something else.
Paweł Gronowski
On Thu, Jul 30, 2020 at 08:49:41AM -0600, Matt Coffin wrote:
> Hello all, I just did some testing with this applied, and while it no
> longer returns -EINVAL, running `sudo sh -c 'echo "vc 2 2150 1195" >
> /sys/class/drm/card1/device/pp_od_clk_voltage'` results in `sh` spiking
> to, and staying at 100% CPU usage, with no indicating information in
> `dmesg` from the kernel.
>
> It appeared to work at least ONCE, but potentially not after.
>
> This is not unique to Navi, and caused the problem on a POLARIS10 card
> as well.
>
> Sorry for the bad news, and thanks for any insight you may have,
> Matt Coffin
>
> On 7/29/20 8:53 PM, Alex Deucher wrote:
>> On Wed, Jul 29, 2020 at 10:20 PM Paweł Gronowski wrote:
>>>
>>> Regression was introduced in commit 38e0c89a19fd
>>> ("drm/amdgpu: Fix NULL dereference in dpm sysfs handlers") which
>>> made the set_pp_od_clk_voltage and set_pp_power_profile_mode return
>>> -EINVAL for previously valid input. This was caused by an empty
>>> string (starting at the \0 character) being passed to the kstrtol.
>>>
>>> Signed-off-by: Paweł Gronowski
>>
>> Applied. Thanks!
>>
>> Alex
>>
>>> ---
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c | 9 +++--
>>> 1 file changed, 7 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
>>> index ebb8a28ff002..cbf623ff03bd 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
>>> @@ -778,12 +778,14 @@ static ssize_t
>>>
Re: [PATCH] drm/amdgpu: Fix regression in adjusting power table/profile
I actually *just* finished my bisect, and arrived at the same
conclusion. The hang appears to be introduced in
edad8312cbbf9a33c86873fc4093664f150dd5c1.
There are some conflicts with an automatic `git revert`, so I'm picking
through the changes now to fully understand what happened and come up
with a fix.
Thanks again for the help,
Matt
On 7/31/20 2:20 PM, Paweł Gronowski wrote:
> Hello again,
>
> I just finished a bisect of amd-staging-drm-next and it looks like
> the hang is first introduced in edad8312cbbf9a33c86873fc4093664f150dd5c1
> ("drm/amdgpu: fix system hang issue during GPU reset").
>
> It is a bit tricky, because it is commited on top of my first faulty patch
> 7173949df45482 ("drm/amdgpu: Fix NULL dereference in dpm sysfs handlers") so
> it needs to be reverted fix the premature -INVAL.
>
> Test case:
> sudo sh -c 'echo "s 0 305 750" >
> /sys/class/drm/card0/device/pp_od_clk_voltage'
> Results:
> edad8312cbbf9a3 + revert 7173949df45482 = hang
> edad8312cbbf9a3~1 + revert 7173949df45482 = no hang
>
> Could you confirm that you get the same results?
>
> Thanks,
> Paweł Gronowski
>
>
> On Fri, Jul 31, 2020 at 03:34:40PM +0200, Paweł Gronowski wrote:
>> Hey Matt,
>>
>> I have just tested the amd-staging-drm-next branch
>> (dd654c76d6e854afad716ded899e4404734aaa10) with my patches reverted
>> and I can reproduce your issue with:
>>
>> sudo sh -c 'echo "s 0 305 750" >
>> /sys/class/drm/card0/device/pp_od_clk_voltage'
>>
>> Which makes the sh hang with 100% usage.
>>
>> The issue does not happen on the mainline
>> (d8b9faec54ae4bc2fff68bcd0befa93ace8256ce)
>> both without and with my patches reapplied.
>> So the problem must be related to some commit that is present in the
>> amd-staging-drm-next but not in the mainline.
>>
>>
>> Paweł Gronowski
>>
>> On Thu, Jul 30, 2020 at 06:34:14PM -0600, Matt Coffin wrote:
>>> Hey Pawel,
>>>
>>> I did confirm that this patch *introduced* the issue both with the
>>> bisect, and by testing reverting it.
>>>
>>> Now, there's a lot of fragile pieces in the dpm handling, so it could be
>>> this patch's interaction with something else that's causing it and it
>>> may well not be the fault of this code, but this is the patch that
>>> introduced the issue.
>>>
>>> I'll have some more time tomorrow to try to get down to root cause here,
>>> so maybe I'll have more to offer then.
>>>
>>> Thanks for taking a look,
>>> Matt
>>>
>>> On 7/30/20 6:31 PM, Paweł Gronowski wrote:
Hello Matt,
Thank you for your testing. It seems that my gpu (RX 570) does not support
the
vc setting so I can not exactly reproduce the issue. However I did trace
the
code path the test case takes and it seems to correctly pass through the
while
loop that parses the input and fails only in amdgpu_dpm_odn_edit_dpm_table.
The 'parameter' array is populated the same way as the original code did.
Since
the amdgpu_dpm_odn_edit_dpm_table is reached, I think that your problem is
unfortunately caused by something else.
Paweł Gronowski
On Thu, Jul 30, 2020 at 08:49:41AM -0600, Matt Coffin wrote:
> Hello all, I just did some testing with this applied, and while it no
> longer returns -EINVAL, running `sudo sh -c 'echo "vc 2 2150 1195" >
> /sys/class/drm/card1/device/pp_od_clk_voltage'` results in `sh` spiking
> to, and staying at 100% CPU usage, with no indicating information in
> `dmesg` from the kernel.
>
> It appeared to work at least ONCE, but potentially not after.
>
> This is not unique to Navi, and caused the problem on a POLARIS10 card
> as well.
>
> Sorry for the bad news, and thanks for any insight you may have,
> Matt Coffin
>
> On 7/29/20 8:53 PM, Alex Deucher wrote:
>> On Wed, Jul 29, 2020 at 10:20 PM Paweł Gronowski wrote:
>>>
>>> Regression was introduced in commit 38e0c89a19fd
>>> ("drm/amdgpu: Fix NULL dereference in dpm sysfs handlers") which
>>> made the set_pp_od_clk_voltage and set_pp_power_profile_mode return
>>> -EINVAL for previously valid input. This was caused by an empty
>>> string (starting at the \0 character) being passed to the kstrtol.
>>>
>>> Signed-off-by: Paweł Gronowski
>>
>> Applied. Thanks!
>>
>> Alex
>>
>>> ---
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c | 9 +++--
>>> 1 file changed, 7 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
>>> index ebb8a28ff002..cbf623ff03bd 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
>>> @@ -778,12 +778,14 @@ static ssize_t
>>> amdgpu_set_pp_od_clk_voltage(struct device *dev,
>>> tmp_str++;
>>> while (isspace(*++tmp_str));
>>>
>>> - while ((sub_str = strsep(_str, delimiter))
Re: [PATCH 08/17] drm/amd/powerplay: add Renoir support for gpu metrics export(V2)
Acked-by: Nirmoy Das
On 7/31/20 4:43 AM, Evan Quan wrote:
Add Renoir gpu metrics export interface.
V2: use memcpy to make code more compact
Change-Id: Ic83265536eeaa9e458dc395b2be18ea49da4c68a
Signed-off-by: Evan Quan
Reviewed-by: Alex Deucher
---
drivers/gpu/drm/amd/powerplay/inc/smu_v12_0.h | 2 +
drivers/gpu/drm/amd/powerplay/renoir_ppt.c| 80 ++-
drivers/gpu/drm/amd/powerplay/smu_v12_0.c | 12 +++
3 files changed, 91 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/amd/powerplay/inc/smu_v12_0.h
b/drivers/gpu/drm/amd/powerplay/inc/smu_v12_0.h
index 02de3b6199e5..fa2e8cb07967 100644
--- a/drivers/gpu/drm/amd/powerplay/inc/smu_v12_0.h
+++ b/drivers/gpu/drm/amd/powerplay/inc/smu_v12_0.h
@@ -60,5 +60,7 @@ int smu_v12_0_set_soft_freq_limited_range(struct smu_context
*smu, enum smu_clk_
int smu_v12_0_set_driver_table_location(struct smu_context *smu);
+void smu_v12_0_init_gpu_metrics_v2_0(struct gpu_metrics_v2_0 *gpu_metrics);
+
#endif
#endif
diff --git a/drivers/gpu/drm/amd/powerplay/renoir_ppt.c
b/drivers/gpu/drm/amd/powerplay/renoir_ppt.c
index 575ae4be98a2..61e8700a7bdb 100644
--- a/drivers/gpu/drm/amd/powerplay/renoir_ppt.c
+++ b/drivers/gpu/drm/amd/powerplay/renoir_ppt.c
@@ -166,18 +166,32 @@ static int renoir_init_smc_tables(struct smu_context *smu)
smu_table->clocks_table = kzalloc(sizeof(DpmClocks_t), GFP_KERNEL);
if (!smu_table->clocks_table)
- return -ENOMEM;
+ goto err0_out;
smu_table->metrics_table = kzalloc(sizeof(SmuMetrics_t), GFP_KERNEL);
if (!smu_table->metrics_table)
- return -ENOMEM;
+ goto err1_out;
smu_table->metrics_time = 0;
smu_table->watermarks_table = kzalloc(sizeof(Watermarks_t), GFP_KERNEL);
if (!smu_table->watermarks_table)
- return -ENOMEM;
+ goto err2_out;
+
+ smu_table->gpu_metrics_table_size = sizeof(struct gpu_metrics_v2_0);
+ smu_table->gpu_metrics_table =
kzalloc(smu_table->gpu_metrics_table_size, GFP_KERNEL);
+ if (!smu_table->gpu_metrics_table)
+ goto err3_out;
return 0;
+
+err3_out:
+ kfree(smu_table->watermarks_table);
+err2_out:
+ kfree(smu_table->metrics_table);
+err1_out:
+ kfree(smu_table->clocks_table);
+err0_out:
+ return -ENOMEM;
}
/**
@@ -995,6 +1009,65 @@ static bool renoir_is_dpm_running(struct smu_context *smu)
}
+static ssize_t renoir_get_gpu_metrics(struct smu_context *smu,
+ void **table)
+{
+ struct smu_table_context *smu_table = >smu_table;
+ struct gpu_metrics_v2_0 *gpu_metrics =
+ (struct gpu_metrics_v2_0 *)smu_table->gpu_metrics_table;
+ SmuMetrics_t metrics;
+ int ret = 0;
+
+ ret = renoir_get_metrics_table(smu, );
+ if (ret)
+ return ret;
+
+ smu_v12_0_init_gpu_metrics_v2_0(gpu_metrics);
+
+ gpu_metrics->temperature_gfx = metrics.GfxTemperature;
+ gpu_metrics->temperature_soc = metrics.SocTemperature;
+ memcpy(_metrics->temperature_core[0],
+ [0],
+ sizeof(uint16_t) * 8);
+ gpu_metrics->temperature_l3[0] = metrics.L3Temperature[0];
+ gpu_metrics->temperature_l3[1] = metrics.L3Temperature[1];
+
+ gpu_metrics->average_gfx_activity = metrics.AverageGfxActivity;
+ gpu_metrics->average_mm_activity = metrics.AverageUvdActivity;
+
+ gpu_metrics->average_socket_power = metrics.CurrentSocketPower;
+ gpu_metrics->average_cpu_power = metrics.Power[0];
+ gpu_metrics->average_soc_power = metrics.Power[1];
+ memcpy(_metrics->average_core_power[0],
+ [0],
+ sizeof(uint16_t) * 8);
+
+ gpu_metrics->average_gfxclk_frequency = metrics.AverageGfxclkFrequency;
+ gpu_metrics->average_socclk_frequency = metrics.AverageSocclkFrequency;
+ gpu_metrics->average_fclk_frequency = metrics.AverageFclkFrequency;
+ gpu_metrics->average_vclk_frequency = metrics.AverageVclkFrequency;
+
+ gpu_metrics->current_gfxclk = metrics.ClockFrequency[CLOCK_GFXCLK];
+ gpu_metrics->current_socclk = metrics.ClockFrequency[CLOCK_SOCCLK];
+ gpu_metrics->current_uclk = metrics.ClockFrequency[CLOCK_UMCCLK];
+ gpu_metrics->current_fclk = metrics.ClockFrequency[CLOCK_FCLK];
+ gpu_metrics->current_vclk = metrics.ClockFrequency[CLOCK_VCLK];
+ gpu_metrics->current_dclk = metrics.ClockFrequency[CLOCK_DCLK];
+ memcpy(_metrics->current_coreclk[0],
+ [0],
+ sizeof(uint16_t) * 8);
+ gpu_metrics->current_l3clk[0] = metrics.L3Frequency[0];
+ gpu_metrics->current_l3clk[1] = metrics.L3Frequency[1];
+
+ gpu_metrics->throttle_status = metrics.ThrottlerStatus;
+
+ gpu_metrics->fan_pwm = metrics.FanPwm;
+
+ *table = (void *)gpu_metrics;
+
+ return sizeof(struct gpu_metrics_v2_0);
+}
+
Re: [PATCH 1/2] drm/amdgpu: fix reload KMD hang on KIQ
In gfx_v10_0_sw_fini the KIQ ring gets freed. Wouldn't that be the right place to stop the KIQ? Otherwise KIQ will hang as soon as someone allocates the memory that was previously used for the KIQ ring buffer and overwrites it with something that's not a valid PM4 packet. Regards, Felix Am 2020-07-31 um 3:51 a.m. schrieb Monk Liu: > KIQ will hang if we try below steps: > modprobe amdgpu > rmmod amdgpu > modprobe amdgpu sched_hw_submission=4 > > the cause is that due to KIQ is always living there even > after we unload KMD thus when doing the realod of KMD > KIQ will crash upon its register programed with different > values with the previous configuration (the config > like HQD addr, ring size, is easily changed if we alter > the sched_hw_submission) > > the fix is we must inactive KIQ first before touching any > of its registgers > > Signed-off-by: Monk Liu > --- > 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 db9f1e8..f571e25 100644 > --- a/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c > +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c > @@ -6433,6 +6433,9 @@ static int gfx_v10_0_kiq_init_register(struct > amdgpu_ring *ring) > struct v10_compute_mqd *mqd = ring->mqd_ptr; > int j; > > + /* activate the queue */ > + WREG32_SOC15(GC, 0, mmCP_HQD_ACTIVE, 0); > + > /* disable wptr polling */ > WREG32_FIELD15(GC, 0, CP_PQ_WPTR_POLL_CNTL, EN, 0); > ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 2/2] drm/amdgpu: introduce a new parameter to configure how many KCQ we want(v5)
Am 2020-07-31 um 3:51 a.m. schrieb Monk Liu:
> what:
> the MQD's save and restore of KCQ (kernel compute queue)
> cost lots of clocks during world switch which impacts a lot
> to multi-VF performance
>
> how:
> introduce a paramter to control the number of KCQ to avoid
> performance drop if there is no kernel compute queue needed
>
> notes:
> this paramter only affects gfx 8/9/10
>
> v2:
> refine namings
>
> v3:
> choose queues for each ring to that try best to cross pipes evenly.
>
> v4:
> fix indentation
> some cleanupsin the gfx_compute_queue_acquire()
>
> v5:
> further fix on indentations
> more cleanupsin gfx_compute_queue_acquire()
>
> TODO:
> in the future we will let hypervisor driver to set this paramter
> automatically thus no need for user to configure it through
> modprobe in virtual machine
>
> Signed-off-by: Monk Liu
This patch is Reviewed-by: Felix Kuehling
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu.h| 1 +
> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 5 +++
> drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c| 4 +++
> drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c| 49
> --
> drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c | 30 +-
> drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c | 29 +-
> drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c | 31 ++-
> 7 files changed, 76 insertions(+), 73 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index e97c088..de11136 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> @@ -201,6 +201,7 @@ extern int amdgpu_si_support;
> #ifdef CONFIG_DRM_AMDGPU_CIK
> extern int amdgpu_cik_support;
> #endif
> +extern int amdgpu_num_kcq;
>
> #define AMDGPU_VM_MAX_NUM_CTX4096
> #define AMDGPU_SG_THRESHOLD (256*1024*1024)
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 62ecac9..cf445bab 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -1199,6 +1199,11 @@ static int amdgpu_device_check_arguments(struct
> amdgpu_device *adev)
>
> amdgpu_gmc_tmz_set(adev);
>
> + if (amdgpu_num_kcq > 8 || amdgpu_num_kcq < 0) {
> + amdgpu_num_kcq = 8;
> + dev_warn(adev->dev, "set kernel compute queue number to 8 due
> to invalid paramter provided by user\n");
> + }
> +
> return 0;
> }
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> index 6291f5f..b545c40 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> @@ -150,6 +150,7 @@ int amdgpu_noretry;
> int amdgpu_force_asic_type = -1;
> int amdgpu_tmz = 0;
> int amdgpu_reset_method = -1; /* auto */
> +int amdgpu_num_kcq = -1;
>
> struct amdgpu_mgpu_info mgpu_info = {
> .mutex = __MUTEX_INITIALIZER(mgpu_info.mutex),
> @@ -765,6 +766,9 @@ module_param_named(tmz, amdgpu_tmz, int, 0444);
> MODULE_PARM_DESC(reset_method, "GPU reset method (-1 = auto (default), 0 =
> legacy, 1 = mode0, 2 = mode1, 3 = mode2, 4 = baco)");
> module_param_named(reset_method, amdgpu_reset_method, int, 0444);
>
> +MODULE_PARM_DESC(num_kcq, "number of kernel compute queue user want to setup
> (8 if set to greater than 8 or less than 0, only affect gfx 8+)");
> +module_param_named(num_kcq, amdgpu_num_kcq, int, 0444);
> +
> static const struct pci_device_id pciidlist[] = {
> #ifdef CONFIG_DRM_AMDGPU_SI
> {0x1002, 0x6780, PCI_ANY_ID, PCI_ANY_ID, 0, 0, CHIP_TAHITI},
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
> index 8eff017..0cd9de6 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
> @@ -202,40 +202,29 @@ bool amdgpu_gfx_is_high_priority_compute_queue(struct
> amdgpu_device *adev,
>
> void amdgpu_gfx_compute_queue_acquire(struct amdgpu_device *adev)
> {
> - int i, queue, pipe, mec;
> + int i, queue, pipe;
> bool multipipe_policy = amdgpu_gfx_is_multipipe_capable(adev);
> -
> - /* policy for amdgpu compute queue ownership */
> - for (i = 0; i < AMDGPU_MAX_COMPUTE_QUEUES; ++i) {
> - queue = i % adev->gfx.mec.num_queue_per_pipe;
> - pipe = (i / adev->gfx.mec.num_queue_per_pipe)
> - % adev->gfx.mec.num_pipe_per_mec;
> - mec = (i / adev->gfx.mec.num_queue_per_pipe)
> - / adev->gfx.mec.num_pipe_per_mec;
> -
> - /* we've run out of HW */
> - if (mec >= adev->gfx.mec.num_mec)
> - break;
> -
> - if (multipipe_policy) {
> - /* policy: amdgpu owns the first two queues of the
> first MEC */
> - if (mec == 0 && queue < 2)
> -
Re: [Linux-kernel-mentees] [PATCH] drm/amdgpu: Prevent kernel-infoleak in amdgpu_info_ioctl()
Am 31.07.20 um 09:10 schrieb Greg Kroah-Hartman:
On Fri, Jul 31, 2020 at 08:57:53AM +0200, Christian König wrote:
Am 31.07.20 um 08:53 schrieb Greg Kroah-Hartman:
On Thu, Jul 30, 2020 at 05:09:07PM -0400, Luben Tuikov wrote:
On 2020-07-29 9:49 a.m., Alex Deucher wrote:
On Wed, Jul 29, 2020 at 4:11 AM Christian König
wrote:
Am 28.07.20 um 21:29 schrieb Peilin Ye:
Compiler leaves a 4-byte hole near the end of `dev_info`, causing
amdgpu_info_ioctl() to copy uninitialized kernel stack memory to userspace
when `size` is greater than 356.
In 2015 we tried to fix this issue by doing `= {};` on `dev_info`, which
unfortunately does not initialize that 4-byte hole. Fix it by using
memset() instead.
Cc: sta...@vger.kernel.org
Fixes: c193fa91b918 ("drm/amdgpu: information leak in amdgpu_info_ioctl()")
Fixes: d38ceaf99ed0 ("drm/amdgpu: add core driver (v4)")
Suggested-by: Dan Carpenter
Signed-off-by: Peilin Ye
Reviewed-by: Christian König
I can't count how many of those we have fixed over the years.
At some point we should probably document that using "= {}" or "= { 0 }"
in the kernel is a really bad idea and should be avoided.
Moreover, it seems like different compilers seem to behave relatively
differently with these and we often get reports of warnings with these
on clang. When in doubt, memset.
There are quite a few of those under drivers/gpu/drm, for "amd/", "scheduler/"
drm*.c files,
$find . \( -regex "./drm.*\.c" -or -regex "./amd/.*\.c" -or -regex "./scheduler/.*\.c" \)
-exec egrep -n -- " *= *{ *(|NULL|0) *}" \{\} \+ | wc -l
374
$_
Out of which only 16 are of the non-ISO C variety, "= {}",
$find . \( -regex "./drm.*\.c" -or -regex "./amd/.*\.c" -or -regex "./scheduler/.*\.c" \)
-exec egrep -n -- " *= *{ *}" \{\} \+ | wc -l
16
$_
Perhaps the latter are the more pressing ones, since it is a C++ initializer
and not a ISO C one.
It only matters when we care copying the data to userspace, if it all
stays in the kernel, all is fine.
Well only as long as you don't try to compute a CRC32, MD5 or any
fingerprint for a hash from the bytes from the structure.
Then it fails horrible and you wonder why the code doesn't works as
expected.
True, but the number of times I have ever needed to do that to a
structure for a driver is, um, never...
If a structure ever needs to have that happen to it, I would sure hope
the developer was aware of padding fields, otherwise, well, someone
needs to take away their C language certification :)
Well it is very likely that stack allocated structures have the same
values in the padding bytes most of the time. So the problem is very
subtle and hard to detect.
We've seen enough problems with that over the last ~10 years that I'm
clearly in favor of adding something to checkpatch.pl to spill out a
warning if "= { }" is used for zero initialization.
Alternatively some of the people who know gcc/clang better than I do
could come up with a warning that you shouldn't cast a structure with
uninitialized padding to void* or u8*.
I mean KASAN is already doing a great job detecting that kind of stuff,
but for this you still need to hit the offending code path.
Thanks,
Christian.
thanks,
greg k-h
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH 2/2] drm/amdgpu: introduce a new parameter to configure how many KCQ we want(v5)
what:
the MQD's save and restore of KCQ (kernel compute queue)
cost lots of clocks during world switch which impacts a lot
to multi-VF performance
how:
introduce a paramter to control the number of KCQ to avoid
performance drop if there is no kernel compute queue needed
notes:
this paramter only affects gfx 8/9/10
v2:
refine namings
v3:
choose queues for each ring to that try best to cross pipes evenly.
v4:
fix indentation
some cleanupsin the gfx_compute_queue_acquire()
v5:
further fix on indentations
more cleanupsin gfx_compute_queue_acquire()
TODO:
in the future we will let hypervisor driver to set this paramter
automatically thus no need for user to configure it through
modprobe in virtual machine
Signed-off-by: Monk Liu
---
drivers/gpu/drm/amd/amdgpu/amdgpu.h| 1 +
drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 5 +++
drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c| 4 +++
drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c| 49 --
drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c | 30 +-
drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c | 29 +-
drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c | 31 ++-
7 files changed, 76 insertions(+), 73 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index e97c088..de11136 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -201,6 +201,7 @@ extern int amdgpu_si_support;
#ifdef CONFIG_DRM_AMDGPU_CIK
extern int amdgpu_cik_support;
#endif
+extern int amdgpu_num_kcq;
#define AMDGPU_VM_MAX_NUM_CTX 4096
#define AMDGPU_SG_THRESHOLD(256*1024*1024)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 62ecac9..cf445bab 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -1199,6 +1199,11 @@ static int amdgpu_device_check_arguments(struct
amdgpu_device *adev)
amdgpu_gmc_tmz_set(adev);
+ if (amdgpu_num_kcq > 8 || amdgpu_num_kcq < 0) {
+ amdgpu_num_kcq = 8;
+ dev_warn(adev->dev, "set kernel compute queue number to 8 due
to invalid paramter provided by user\n");
+ }
+
return 0;
}
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index 6291f5f..b545c40 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -150,6 +150,7 @@ int amdgpu_noretry;
int amdgpu_force_asic_type = -1;
int amdgpu_tmz = 0;
int amdgpu_reset_method = -1; /* auto */
+int amdgpu_num_kcq = -1;
struct amdgpu_mgpu_info mgpu_info = {
.mutex = __MUTEX_INITIALIZER(mgpu_info.mutex),
@@ -765,6 +766,9 @@ module_param_named(tmz, amdgpu_tmz, int, 0444);
MODULE_PARM_DESC(reset_method, "GPU reset method (-1 = auto (default), 0 =
legacy, 1 = mode0, 2 = mode1, 3 = mode2, 4 = baco)");
module_param_named(reset_method, amdgpu_reset_method, int, 0444);
+MODULE_PARM_DESC(num_kcq, "number of kernel compute queue user want to setup
(8 if set to greater than 8 or less than 0, only affect gfx 8+)");
+module_param_named(num_kcq, amdgpu_num_kcq, int, 0444);
+
static const struct pci_device_id pciidlist[] = {
#ifdef CONFIG_DRM_AMDGPU_SI
{0x1002, 0x6780, PCI_ANY_ID, PCI_ANY_ID, 0, 0, CHIP_TAHITI},
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
index 8eff017..0cd9de6 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
@@ -202,40 +202,29 @@ bool amdgpu_gfx_is_high_priority_compute_queue(struct
amdgpu_device *adev,
void amdgpu_gfx_compute_queue_acquire(struct amdgpu_device *adev)
{
- int i, queue, pipe, mec;
+ int i, queue, pipe;
bool multipipe_policy = amdgpu_gfx_is_multipipe_capable(adev);
-
- /* policy for amdgpu compute queue ownership */
- for (i = 0; i < AMDGPU_MAX_COMPUTE_QUEUES; ++i) {
- queue = i % adev->gfx.mec.num_queue_per_pipe;
- pipe = (i / adev->gfx.mec.num_queue_per_pipe)
- % adev->gfx.mec.num_pipe_per_mec;
- mec = (i / adev->gfx.mec.num_queue_per_pipe)
- / adev->gfx.mec.num_pipe_per_mec;
-
- /* we've run out of HW */
- if (mec >= adev->gfx.mec.num_mec)
- break;
-
- if (multipipe_policy) {
- /* policy: amdgpu owns the first two queues of the
first MEC */
- if (mec == 0 && queue < 2)
- set_bit(i, adev->gfx.mec.queue_bitmap);
- } else {
- /* policy: amdgpu owns all queues in the first pipe */
- if (mec == 0 && pipe == 0)
- set_bit(i, adev->gfx.mec.queue_bitmap);
+ int max_queues_per_mec =
[PATCH 1/2] drm/amdgpu: fix reload KMD hang on KIQ
KIQ will hang if we try below steps: modprobe amdgpu rmmod amdgpu modprobe amdgpu sched_hw_submission=4 the cause is that due to KIQ is always living there even after we unload KMD thus when doing the realod of KMD KIQ will crash upon its register programed with different values with the previous configuration (the config like HQD addr, ring size, is easily changed if we alter the sched_hw_submission) the fix is we must inactive KIQ first before touching any of its registgers Signed-off-by: Monk Liu --- 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 db9f1e8..f571e25 100644 --- a/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c @@ -6433,6 +6433,9 @@ static int gfx_v10_0_kiq_init_register(struct amdgpu_ring *ring) struct v10_compute_mqd *mqd = ring->mqd_ptr; int j; + /* activate the queue */ + WREG32_SOC15(GC, 0, mmCP_HQD_ACTIVE, 0); + /* disable wptr polling */ WREG32_FIELD15(GC, 0, CP_PQ_WPTR_POLL_CNTL, EN, 0); -- 2.7.4 ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
RE: [PATCH] drm/amdgpu: introduce a new parameter to configure how many KCQ we want(v4)
[AMD Official Use Only - Internal Distribution Only]
Please check V5 and see if it looks better
I use automatic indentation on those lines
_
Monk Liu|GPU Virtualization Team |AMD
-Original Message-
From: amd-gfx On Behalf Of Liu, Monk
Sent: Friday, July 31, 2020 3:43 PM
To: Kuehling, Felix ; amd-gfx@lists.freedesktop.org
Subject: RE: [PATCH] drm/amdgpu: introduce a new parameter to configure how
many KCQ we want(v4)
[AMD Official Use Only - Internal Distribution Only]
[AMD Official Use Only - Internal Distribution Only]
Felix
I cannot let the indentation look perfect when it goes out from the email ,
don't know why
It looks well in my VIM although
_
Monk Liu|GPU Virtualization Team |AMD
-Original Message-
From: Kuehling, Felix
Sent: Friday, July 31, 2020 12:54 PM
To: Liu, Monk ; amd-gfx@lists.freedesktop.org
Subject: Re: [PATCH] drm/amdgpu: introduce a new parameter to configure how
many KCQ we want(v4)
Am 2020-07-30 um 11:42 p.m. schrieb Monk Liu:
> what:
> the MQD's save and restore of KCQ (kernel compute queue) cost lots of
> clocks during world switch which impacts a lot to multi-VF performance
>
> how:
> introduce a paramter to control the number of KCQ to avoid performance
> drop if there is no kernel compute queue needed
>
> notes:
> this paramter only affects gfx 8/9/10
>
> v2:
> refine namings
>
> v3:
> choose queues for each ring to that try best to cross pipes evenly.
>
> v4:
> fix indentation
> some cleanupsin the gfx_compute_queue_acquire() function
>
> TODO:
> in the future we will let hypervisor driver to set this paramter
> automatically thus no need for user to configure it through modprobe
> in virtual machine
>
> Signed-off-by: Monk Liu
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu.h| 1 +
> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 5 +++
> drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c| 4 +++
> drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c| 52
> +-
> drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c | 30 +
> drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c | 29 +
> drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c | 31 +-
> 7 files changed, 80 insertions(+), 72 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index e97c088..de11136 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> @@ -201,6 +201,7 @@ extern int amdgpu_si_support; #ifdef
> CONFIG_DRM_AMDGPU_CIK extern int amdgpu_cik_support; #endif
> +extern int amdgpu_num_kcq;
>
> #define AMDGPU_VM_MAX_NUM_CTX4096
> #define AMDGPU_SG_THRESHOLD(256*1024*1024)
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 62ecac9..cf445bab 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -1199,6 +1199,11 @@ static int amdgpu_device_check_arguments(struct
> amdgpu_device *adev)
>
> amdgpu_gmc_tmz_set(adev);
>
> +if (amdgpu_num_kcq > 8 || amdgpu_num_kcq < 0) { amdgpu_num_kcq = 8;
> +dev_warn(adev->dev, "set kernel compute queue number to 8 due to
> +invalid paramter provided by user\n"); }
> +
> return 0;
> }
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> index 6291f5f..b545c40 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> @@ -150,6 +150,7 @@ int amdgpu_noretry; int amdgpu_force_asic_type =
> -1; int amdgpu_tmz = 0; int amdgpu_reset_method = -1; /* auto */
> +int amdgpu_num_kcq = -1;
>
> struct amdgpu_mgpu_info mgpu_info = { .mutex =
> __MUTEX_INITIALIZER(mgpu_info.mutex),
> @@ -765,6 +766,9 @@ module_param_named(tmz, amdgpu_tmz, int, 0444);
> MODULE_PARM_DESC(reset_method, "GPU reset method (-1 = auto (default),
> 0 = legacy, 1 = mode0, 2 = mode1, 3 = mode2, 4 = baco)");
> module_param_named(reset_method, amdgpu_reset_method, int, 0444);
>
> +MODULE_PARM_DESC(num_kcq, "number of kernel compute queue user want
> +to setup (8 if set to greater than 8 or less than 0, only affect gfx
> +8+)"); module_param_named(num_kcq, amdgpu_num_kcq, int, 0444);
> +
> static const struct pci_device_id pciidlist[] = { #ifdef
> CONFIG_DRM_AMDGPU_SI {0x1002, 0x6780, PCI_ANY_ID, PCI_ANY_ID, 0, 0,
> CHIP_TAHITI}, diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
> index 8eff017..b43df8e 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
> @@ -202,40 +202,34 @@ bool
> amdgpu_gfx_is_high_priority_compute_queue(struct amdgpu_device *adev,
>
> void amdgpu_gfx_compute_queue_acquire(struct amdgpu_device *adev) {
> -int i, queue, pipe, mec;
> +int i, queue, pipe;
> bool multipipe_policy = amdgpu_gfx_is_multipipe_capable(adev);
> +int
RE: [PATCH] drm/amdgpu: introduce a new parameter to configure how many KCQ we want(v4)
[AMD Official Use Only - Internal Distribution Only]
Felix
I cannot let the indentation look perfect when it goes out from the email ,
don't know why
It looks well in my VIM although
_
Monk Liu|GPU Virtualization Team |AMD
-Original Message-
From: Kuehling, Felix
Sent: Friday, July 31, 2020 12:54 PM
To: Liu, Monk ; amd-gfx@lists.freedesktop.org
Subject: Re: [PATCH] drm/amdgpu: introduce a new parameter to configure how
many KCQ we want(v4)
Am 2020-07-30 um 11:42 p.m. schrieb Monk Liu:
> what:
> the MQD's save and restore of KCQ (kernel compute queue) cost lots of
> clocks during world switch which impacts a lot to multi-VF performance
>
> how:
> introduce a paramter to control the number of KCQ to avoid performance
> drop if there is no kernel compute queue needed
>
> notes:
> this paramter only affects gfx 8/9/10
>
> v2:
> refine namings
>
> v3:
> choose queues for each ring to that try best to cross pipes evenly.
>
> v4:
> fix indentation
> some cleanupsin the gfx_compute_queue_acquire() function
>
> TODO:
> in the future we will let hypervisor driver to set this paramter
> automatically thus no need for user to configure it through modprobe
> in virtual machine
>
> Signed-off-by: Monk Liu
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu.h| 1 +
> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 5 +++
> drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c| 4 +++
> drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c| 52
> +-
> drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c | 30 +
> drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c | 29 +
> drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c | 31 +-
> 7 files changed, 80 insertions(+), 72 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index e97c088..de11136 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> @@ -201,6 +201,7 @@ extern int amdgpu_si_support; #ifdef
> CONFIG_DRM_AMDGPU_CIK extern int amdgpu_cik_support; #endif
> +extern int amdgpu_num_kcq;
>
> #define AMDGPU_VM_MAX_NUM_CTX4096
> #define AMDGPU_SG_THRESHOLD(256*1024*1024)
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 62ecac9..cf445bab 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -1199,6 +1199,11 @@ static int amdgpu_device_check_arguments(struct
> amdgpu_device *adev)
>
> amdgpu_gmc_tmz_set(adev);
>
> +if (amdgpu_num_kcq > 8 || amdgpu_num_kcq < 0) {
> +amdgpu_num_kcq = 8;
> +dev_warn(adev->dev, "set kernel compute queue number to 8 due to invalid
> paramter provided by user\n");
> +}
> +
> return 0;
> }
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> index 6291f5f..b545c40 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> @@ -150,6 +150,7 @@ int amdgpu_noretry; int amdgpu_force_asic_type =
> -1; int amdgpu_tmz = 0; int amdgpu_reset_method = -1; /* auto */
> +int amdgpu_num_kcq = -1;
>
> struct amdgpu_mgpu_info mgpu_info = {
> .mutex = __MUTEX_INITIALIZER(mgpu_info.mutex),
> @@ -765,6 +766,9 @@ module_param_named(tmz, amdgpu_tmz, int, 0444);
> MODULE_PARM_DESC(reset_method, "GPU reset method (-1 = auto (default),
> 0 = legacy, 1 = mode0, 2 = mode1, 3 = mode2, 4 = baco)");
> module_param_named(reset_method, amdgpu_reset_method, int, 0444);
>
> +MODULE_PARM_DESC(num_kcq, "number of kernel compute queue user want
> +to setup (8 if set to greater than 8 or less than 0, only affect gfx
> +8+)"); module_param_named(num_kcq, amdgpu_num_kcq, int, 0444);
> +
> static const struct pci_device_id pciidlist[] = { #ifdef
> CONFIG_DRM_AMDGPU_SI
> {0x1002, 0x6780, PCI_ANY_ID, PCI_ANY_ID, 0, 0, CHIP_TAHITI}, diff
> --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
> index 8eff017..b43df8e 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
> @@ -202,40 +202,34 @@ bool
> amdgpu_gfx_is_high_priority_compute_queue(struct amdgpu_device *adev,
>
> void amdgpu_gfx_compute_queue_acquire(struct amdgpu_device *adev) {
> -int i, queue, pipe, mec;
> +int i, queue, pipe;
> bool multipipe_policy = amdgpu_gfx_is_multipipe_capable(adev);
> +int max_queues_per_mec = min(adev->gfx.mec.num_pipe_per_mec *
> + adev->gfx.mec.num_queue_per_pipe,
> + adev->gfx.num_compute_rings);
> +
> +if (multipipe_policy) {
> +/* policy: make queues evenly cross all pipes on MEC1 only */
> +for (i = 0; i < max_queues_per_mec; i++) {
> +pipe = i % adev->gfx.mec.num_pipe_per_mec;
> +queue = (i / adev->gfx.mec.num_pipe_per_mec) %
> +adev->gfx.mec.num_queue_per_pipe;
> +
> +set_bit(pipe * adev->gfx.mec.num_queue_per_pipe + queue,
> +adev->gfx.mec.queue_bitmap);
Indentation looks
Re: [Linux-kernel-mentees] [PATCH] drm/amdgpu: Prevent kernel-infoleak in amdgpu_info_ioctl()
On Fri, Jul 31, 2020 at 08:57:53AM +0200, Christian König wrote:
> Am 31.07.20 um 08:53 schrieb Greg Kroah-Hartman:
> > On Thu, Jul 30, 2020 at 05:09:07PM -0400, Luben Tuikov wrote:
> > > On 2020-07-29 9:49 a.m., Alex Deucher wrote:
> > > > On Wed, Jul 29, 2020 at 4:11 AM Christian König
> > > > wrote:
> > > > > Am 28.07.20 um 21:29 schrieb Peilin Ye:
> > > > > > Compiler leaves a 4-byte hole near the end of `dev_info`, causing
> > > > > > amdgpu_info_ioctl() to copy uninitialized kernel stack memory to
> > > > > > userspace
> > > > > > when `size` is greater than 356.
> > > > > >
> > > > > > In 2015 we tried to fix this issue by doing `= {};` on `dev_info`,
> > > > > > which
> > > > > > unfortunately does not initialize that 4-byte hole. Fix it by using
> > > > > > memset() instead.
> > > > > >
> > > > > > Cc: sta...@vger.kernel.org
> > > > > > Fixes: c193fa91b918 ("drm/amdgpu: information leak in
> > > > > > amdgpu_info_ioctl()")
> > > > > > Fixes: d38ceaf99ed0 ("drm/amdgpu: add core driver (v4)")
> > > > > > Suggested-by: Dan Carpenter
> > > > > > Signed-off-by: Peilin Ye
> > > > > Reviewed-by: Christian König
> > > > >
> > > > > I can't count how many of those we have fixed over the years.
> > > > >
> > > > > At some point we should probably document that using "= {}" or "= { 0
> > > > > }"
> > > > > in the kernel is a really bad idea and should be avoided.
> > > > Moreover, it seems like different compilers seem to behave relatively
> > > > differently with these and we often get reports of warnings with these
> > > > on clang. When in doubt, memset.
> > > There are quite a few of those under drivers/gpu/drm, for "amd/",
> > > "scheduler/"
> > > drm*.c files,
> > >
> > > $find . \( -regex "./drm.*\.c" -or -regex "./amd/.*\.c" -or -regex
> > > "./scheduler/.*\.c" \) -exec egrep -n -- " *= *{ *(|NULL|0) *}" \{\} \+ |
> > > wc -l
> > > 374
> > > $_
> > >
> > > Out of which only 16 are of the non-ISO C variety, "= {}",
> > >
> > > $find . \( -regex "./drm.*\.c" -or -regex "./amd/.*\.c" -or -regex
> > > "./scheduler/.*\.c" \) -exec egrep -n -- " *= *{ *}" \{\} \+ | wc -l
> > > 16
> > > $_
> > >
> > > Perhaps the latter are the more pressing ones, since it is a C++
> > > initializer and not a ISO C one.
> > It only matters when we care copying the data to userspace, if it all
> > stays in the kernel, all is fine.
>
> Well only as long as you don't try to compute a CRC32, MD5 or any
> fingerprint for a hash from the bytes from the structure.
>
> Then it fails horrible and you wonder why the code doesn't works as
> expected.
True, but the number of times I have ever needed to do that to a
structure for a driver is, um, never...
If a structure ever needs to have that happen to it, I would sure hope
the developer was aware of padding fields, otherwise, well, someone
needs to take away their C language certification :)
thanks,
greg k-h
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [Linux-kernel-mentees] [PATCH] drm/amdgpu: Prevent kernel-infoleak in amdgpu_info_ioctl()
On Thu, Jul 30, 2020 at 05:09:07PM -0400, Luben Tuikov wrote:
> On 2020-07-29 9:49 a.m., Alex Deucher wrote:
> > On Wed, Jul 29, 2020 at 4:11 AM Christian König
> > wrote:
> >>
> >> Am 28.07.20 um 21:29 schrieb Peilin Ye:
> >>> Compiler leaves a 4-byte hole near the end of `dev_info`, causing
> >>> amdgpu_info_ioctl() to copy uninitialized kernel stack memory to userspace
> >>> when `size` is greater than 356.
> >>>
> >>> In 2015 we tried to fix this issue by doing `= {};` on `dev_info`, which
> >>> unfortunately does not initialize that 4-byte hole. Fix it by using
> >>> memset() instead.
> >>>
> >>> Cc: sta...@vger.kernel.org
> >>> Fixes: c193fa91b918 ("drm/amdgpu: information leak in
> >>> amdgpu_info_ioctl()")
> >>> Fixes: d38ceaf99ed0 ("drm/amdgpu: add core driver (v4)")
> >>> Suggested-by: Dan Carpenter
> >>> Signed-off-by: Peilin Ye
> >>
> >> Reviewed-by: Christian König
> >>
> >> I can't count how many of those we have fixed over the years.
> >>
> >> At some point we should probably document that using "= {}" or "= { 0 }"
> >> in the kernel is a really bad idea and should be avoided.
> >
> > Moreover, it seems like different compilers seem to behave relatively
> > differently with these and we often get reports of warnings with these
> > on clang. When in doubt, memset.
>
> There are quite a few of those under drivers/gpu/drm, for "amd/", "scheduler/"
> drm*.c files,
>
> $find . \( -regex "./drm.*\.c" -or -regex "./amd/.*\.c" -or -regex
> "./scheduler/.*\.c" \) -exec egrep -n -- " *= *{ *(|NULL|0) *}" \{\} \+ | wc
> -l
> 374
> $_
>
> Out of which only 16 are of the non-ISO C variety, "= {}",
>
> $find . \( -regex "./drm.*\.c" -or -regex "./amd/.*\.c" -or -regex
> "./scheduler/.*\.c" \) -exec egrep -n -- " *= *{ *}" \{\} \+ | wc -l
> 16
> $_
>
> Perhaps the latter are the more pressing ones, since it is a C++ initializer
> and not a ISO C one.
It only matters when we care copying the data to userspace, if it all
stays in the kernel, all is fine.
thanks,
greg k-h
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [Linux-kernel-mentees] [PATCH] drm/amdgpu: Prevent kernel-infoleak in amdgpu_info_ioctl()
Am 31.07.20 um 08:53 schrieb Greg Kroah-Hartman:
On Thu, Jul 30, 2020 at 05:09:07PM -0400, Luben Tuikov wrote:
On 2020-07-29 9:49 a.m., Alex Deucher wrote:
On Wed, Jul 29, 2020 at 4:11 AM Christian König
wrote:
Am 28.07.20 um 21:29 schrieb Peilin Ye:
Compiler leaves a 4-byte hole near the end of `dev_info`, causing
amdgpu_info_ioctl() to copy uninitialized kernel stack memory to userspace
when `size` is greater than 356.
In 2015 we tried to fix this issue by doing `= {};` on `dev_info`, which
unfortunately does not initialize that 4-byte hole. Fix it by using
memset() instead.
Cc: sta...@vger.kernel.org
Fixes: c193fa91b918 ("drm/amdgpu: information leak in amdgpu_info_ioctl()")
Fixes: d38ceaf99ed0 ("drm/amdgpu: add core driver (v4)")
Suggested-by: Dan Carpenter
Signed-off-by: Peilin Ye
Reviewed-by: Christian König
I can't count how many of those we have fixed over the years.
At some point we should probably document that using "= {}" or "= { 0 }"
in the kernel is a really bad idea and should be avoided.
Moreover, it seems like different compilers seem to behave relatively
differently with these and we often get reports of warnings with these
on clang. When in doubt, memset.
There are quite a few of those under drivers/gpu/drm, for "amd/", "scheduler/"
drm*.c files,
$find . \( -regex "./drm.*\.c" -or -regex "./amd/.*\.c" -or -regex "./scheduler/.*\.c" \)
-exec egrep -n -- " *= *{ *(|NULL|0) *}" \{\} \+ | wc -l
374
$_
Out of which only 16 are of the non-ISO C variety, "= {}",
$find . \( -regex "./drm.*\.c" -or -regex "./amd/.*\.c" -or -regex "./scheduler/.*\.c" \)
-exec egrep -n -- " *= *{ *}" \{\} \+ | wc -l
16
$_
Perhaps the latter are the more pressing ones, since it is a C++ initializer
and not a ISO C one.
It only matters when we care copying the data to userspace, if it all
stays in the kernel, all is fine.
Well only as long as you don't try to compute a CRC32, MD5 or any
fingerprint for a hash from the bytes from the structure.
Then it fails horrible and you wonder why the code doesn't works as
expected.
Regards,
Christian.
thanks,
greg k-h
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 17/17] drm/amdgpu: move vram usage by vbios to mman (v2)
Am 30.07.20 um 22:04 schrieb Alex Deucher:
It's related to the memory manager so move it there.
v2: inline the structure
Signed-off-by: Alex Deucher
Reviewed-by: Christian König
---
drivers/gpu/drm/amd/amdgpu/amdgpu.h | 12 ---
drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c | 4 ++--
.../gpu/drm/amd/amdgpu/amdgpu_atomfirmware.c | 4 ++--
drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 20 +--
drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h | 6 ++
drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c | 6 +++---
6 files changed, 23 insertions(+), 29 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index 44fd0ef7394f..81f6412eb54f 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -649,16 +649,6 @@ struct amdgpu_atcs {
struct amdgpu_atcs_functions functions;
};
-/*
- * Firmware VRAM reservation
- */
-struct amdgpu_fw_vram_usage {
- u64 start_offset;
- u64 size;
- struct amdgpu_bo *reserved_bo;
- void *va;
-};
-
/*
* CGS
*/
@@ -942,8 +932,6 @@ struct amdgpu_device {
struct delayed_work delayed_init_work;
struct amdgpu_virt virt;
- /* firmware VRAM reservation */
- struct amdgpu_fw_vram_usage fw_vram_usage;
/* link all shadow bo */
struct list_headshadow_list;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c
index 29f767e026e4..e33f63712b46 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c
@@ -1786,9 +1786,9 @@ static int amdgpu_atombios_allocate_fb_scratch(struct
amdgpu_device *adev)
(uint32_t)(ATOM_VRAM_BLOCK_SRIOV_MSG_SHARE_RESERVATION
<<
ATOM_VRAM_OPERATION_FLAGS_SHIFT)) {
/* Firmware request VRAM reservation for SR-IOV */
- adev->fw_vram_usage.start_offset = (start_addr &
+ adev->mman.fw_vram_usage_start_offset = (start_addr &
(~ATOM_VRAM_OPERATION_FLAGS_MASK)) << 10;
- adev->fw_vram_usage.size = size << 10;
+ adev->mman.fw_vram_usage_size = size << 10;
/* Use the default scratch size */
usage_bytes = 0;
} else {
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_atomfirmware.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_atomfirmware.c
index 1279053324f9..17c010d0431f 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_atomfirmware.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_atomfirmware.c
@@ -89,9 +89,9 @@ int amdgpu_atomfirmware_allocate_fb_scratch(struct
amdgpu_device *adev)
(uint32_t)(ATOM_VRAM_BLOCK_SRIOV_MSG_SHARE_RESERVATION
<<
ATOM_VRAM_OPERATION_FLAGS_SHIFT)) {
/* Firmware request VRAM reservation for SR-IOV */
- adev->fw_vram_usage.start_offset = (start_addr &
+ adev->mman.fw_vram_usage_start_offset = (start_addr &
(~ATOM_VRAM_OPERATION_FLAGS_MASK)) << 10;
- adev->fw_vram_usage.size = size << 10;
+ adev->mman.fw_vram_usage_size = size << 10;
/* Use the default scratch size */
usage_bytes = 0;
} else {
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
index ea9b5b39f640..c7421aa32946 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
@@ -1766,8 +1766,8 @@ static struct ttm_bo_driver amdgpu_bo_driver = {
*/
static void amdgpu_ttm_fw_reserve_vram_fini(struct amdgpu_device *adev)
{
- amdgpu_bo_free_kernel(>fw_vram_usage.reserved_bo,
- NULL, >fw_vram_usage.va);
+ amdgpu_bo_free_kernel(>mman.fw_vram_usage_reserved_bo,
+ NULL, >mman.fw_vram_usage_va);
}
/**
@@ -1781,19 +1781,19 @@ static int amdgpu_ttm_fw_reserve_vram_init(struct
amdgpu_device *adev)
{
uint64_t vram_size = adev->gmc.visible_vram_size;
- adev->fw_vram_usage.va = NULL;
- adev->fw_vram_usage.reserved_bo = NULL;
+ adev->mman.fw_vram_usage_va = NULL;
+ adev->mman.fw_vram_usage_reserved_bo = NULL;
- if (adev->fw_vram_usage.size == 0 ||
- adev->fw_vram_usage.size > vram_size)
+ if (adev->mman.fw_vram_usage_size == 0 ||
+ adev->mman.fw_vram_usage_size > vram_size)
return 0;
return amdgpu_bo_create_kernel_at(adev,
- adev->fw_vram_usage.start_offset,
- adev->fw_vram_usage.size,
+ adev->mman.fw_vram_usage_start_offset,
+
