AMDGPU and 16B stack alignment
Hello!
The x86 kernel is compiled with an 8B stack alignment via
`-mpreferred-stack-boundary=3` for GCC since 3.6-rc1 via
commit d9b0cde91c60 ("x86-64, gcc: Use
-mpreferred-stack-boundary=3 if supported")
or `-mstack-alignment=8` for Clang. Parts of the AMDGPU driver are
compiled with 16B stack alignment.
Generally, the stack alignment is part of the ABI. Linking together two
different translation units with differing stack alignment is dangerous,
particularly when the translation unit with the smaller stack alignment
makes calls into the translation unit with the larger stack alignment.
While 8B aligned stacks are sometimes also 16B aligned, they are not
always.
Multiple users have reported General Protection Faults (GPF) when using
the AMDGPU driver compiled with Clang. Clang is placing objects in stack
slots assuming the stack is 16B aligned, and selecting instructions that
require 16B aligned memory operands. At runtime, syscalls handling 8B
stack aligned code calls into code that assumes 16B stack alignment.
When the stack is a multiple of 8B but not 16B, these instructions
result in a GPF.
GCC doesn't select instructions with alignment requirements, so the GPFs
aren't observed, but it is still considered an ABI breakage to mix and
match stack alignment.
I have patches that basically remove -mpreferred-stack-boundary=4 and
-mstack-alignment=16 from AMDGPU:
https://github.com/ClangBuiltLinux/linux/issues/735#issuecomment-541247601
Yuxuan has tested with Clang and GCC and reported it fixes the GPF's observed.
I've split the patch into 4; same commit message but different Fixes
tags so that they backport to stable on finer granularity. 2 questions
BEFORE I send the series:
1. Would you prefer 4 patches with unique `fixes` tags, or 1 patch?
2. Was there or is there still a good reason for the stack alignment mismatch?
(Further, I think we can use -msse2 for BOTH clang+gcc after my patch,
but I don't have hardware to test on. I'm happy to write/send the
follow up patch, but I'd need help testing).
--
Thanks,
~Nick Desaulniers
RE: [PATCH] drm/amdgpu: No need to check gfxoff status after enable gfxoff feature
Acked-by: Evan Quan
-Original Message-
From: amd-gfx On Behalf Of chen gong
Sent: Monday, October 14, 2019 10:58 AM
To: amd-gfx@lists.freedesktop.org
Cc: Gong, Curry
Subject: [PATCH] drm/amdgpu: No need to check gfxoff status after enable gfxoff
feature
smu_send_smc_msg(smu, SMU_MSG_AllowGfxOff) Just turn on a switch.
As to when GPU get into "GFXoff" will be up to drawing load.
So we can not sure which state GPU should be in after enable gfxoff feature.
Signed-off-by: chen gong
---
drivers/gpu/drm/amd/powerplay/smu_v12_0.c | 9 -
1 file changed, 9 deletions(-)
diff --git a/drivers/gpu/drm/amd/powerplay/smu_v12_0.c
b/drivers/gpu/drm/amd/powerplay/smu_v12_0.c
index c9691d0..cac4269 100644
--- a/drivers/gpu/drm/amd/powerplay/smu_v12_0.c
+++ b/drivers/gpu/drm/amd/powerplay/smu_v12_0.c
@@ -244,15 +244,6 @@ static int smu_v12_0_gfx_off_control(struct smu_context
*smu, bool enable)
if (enable) {
ret = smu_send_smc_msg(smu, SMU_MSG_AllowGfxOff);
- /* confirm gfx is back to "off" state, timeout is 5 seconds */
- while (!(smu_v12_0_get_gfxoff_status(smu) == 0)) {
- msleep(10);
- timeout--;
- if (timeout == 0) {
- DRM_ERROR("enable gfxoff timeout and
failed!\n");
- break;
- }
- }
} else {
ret = smu_send_smc_msg(smu, SMU_MSG_DisallowGfxOff);
--
2.7.4
___
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/drm_edid: correct VIC and HDMI_VIC under HDMI 2.0
> -Original Message- > From: Ville Syrjälä > Sent: Friday, October 4, 2019 10:24 PM > To: Lin, Wayne > Cc: dri-de...@lists.freedesktop.org; amd-gfx@lists.freedesktop.org; Li, Sun > peng (Leo) ; Kazlauskas, Nicholas > > Subject: Re: [PATCH] drm/drm_edid: correct VIC and HDMI_VIC under HDMI > 2.0 > > On Fri, Oct 04, 2019 at 10:41:20AM +, Lin, Wayne wrote: > > > > > > > > From: Ville Syrjälä > > Sent: Thursday, October 3, 2019 21:29 > > To: Lin, Wayne > > Cc: dri-de...@lists.freedesktop.org ; > > amd-gfx@lists.freedesktop.org ; Li, Sun > > peng (Leo) ; Kazlauskas, Nicholas > > > > Subject: Re: [PATCH] drm/drm_edid: correct VIC and HDMI_VIC under HDMI > > 2.0 > > > > On Thu, Oct 03, 2019 at 06:49:05AM +, Lin, Wayne wrote: > > > > > > > > > > > > From: Ville Syrjälä > > > Sent: Wednesday, October 2, 2019 19:58 > > > To: Lin, Wayne > > > Cc: dri-de...@lists.freedesktop.org > > > ; amd-gfx@lists.freedesktop.org > > > ; Li, Sun peng (Leo) > > > ; Kazlauskas, Nicholas > > > > > > Subject: Re: [PATCH] drm/drm_edid: correct VIC and HDMI_VIC under > > > HDMI 2.0 > > > > > > On Tue, Sep 24, 2019 at 01:26:21PM +0800, Wayne Lin wrote: > > > > In HDMI 1.4 defines 4k modes without specific aspect ratio. > > > > However, in HDMI 2.0, adds aspect ratio attribute to distinguish > > > > different 4k modes. > > > > > > > > According to Appendix E of HDMI 2.0 spec, source should use VSIF > > > > to indicate VIC mode only when the mode is one defined in HDMI 1.4b 4K > modes. > > > > Otherwise, use AVI infoframes to convey VIC. > > > > > > > > eg: VIC_103 should use AVI infoframes and VIC_93 use VSIF > > > > > > > > When the sink is HDMI 2.0, current code in > > > > drm_hdmi_avi_infoframe_from_display_mode will also force mode > > > > VIC_103 to have VIC value 0. This violates the spec and needs to be > corrected. > > > > > > > Where is that being done? We only set the AVI VIC to zero if we're > > > > going to use the HDMI VIC instead. > > > > > > Appreciate for your time and apologize for not explaining it clearly. > > > Current code in drm_hdmi_avi_infoframe_from_display_mode() will call > > > drm_match_hdmi_mode() to set up vendor_if_vic. By checking > > > drm_valid_hdmi_vic(vendor_if_vic) to see if the vic info should be > > > conveyed by avi or not. > > > > > > But in drm_match_hdmi_mode(), code doesn't enable match_flags with > > > DRM_MODE_MATCH_ASPECT_RATIO. I think it's due to HDMI1.4b doesn't > > > specify 4K mode conveyed by HDMI VIC with particular aspect ratio. > > > But in Appendix E of HDMI 2.0 spec, it specify only 4k modes with > particular aspect ratio should use VSIF to convey. > > > Hence, when the sink support HDMI 2.0 and set the mode to be > > > VIC_103, calling > > > drm_match_hdmi_mode(mode) will return vendor_if_vic = 3 (VIC_93 and > > > VIC_103 are having the same timing but different aspect ratio). Thereafter > will set the frame->video_code to 0. > > > However, VIC_103 should use AVI VIC according to HDMI 2.0 spec (only > > > VIC: 93, 94, 95 & > > > 98 should use VSIF). > > > > > > This patch try to revise that, when the sink support HDMI 2.0, > > > drm_match_hdmi_mode() should also take aspect ratio into consideration. > > > But for easy reading, I add another function > "drm_match_hdmi_1_4_mode" to do so. > > > > > Seems rather convoluted. I think we should just add the aspect > > > ratios to edid_4k_modes[]. Or is there some problem with that approach? > > > > Thanks for your time. > > > > Since HDMI 1.4b doesn't require edid_4k_modes[] with specific aspect > > ratio, modes as the same timing in edid_4k_modes[] but with different > > aspect ratios are also expected to convey VIC by VSIF to HDMI 1.4 > > sink. Might can't guarantee that there wont't be any compatibility side > > effect > with that approach when the sink is HDMI 1.4b . > > I think adding them should be fine. But while checking the existing code I > noticed a few problems, so I sent out some fixes (cc:d you). Thanks for your time and sorry for late reply. I will try out those received fixes and leave messages there. > > -- > Ville Syrjälä > Intel -- Wayne Lin ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 5/6] drm/amdgpu/dm/mst: Report possible_crtcs incorrectly, for now
On Fri, Oct 11, 2019 at 04:51:13PM -0400, Lyude Paul wrote: > a little late but: i915 does have this hack (or rather-possible_crtcs with MST > in i915 has been broken for a while and got fixed, but had to get reverted > because of this issue), it's where this originally came from. Hm since this is widespread I think we should check for this when we register connectors (either in drm_dev_register, or hotplugged ones). I think just validating that all encoder->possible_crtc match and WARN_ON if not would be really good. 2nd option would be to do that in the GETENCODERS ioctl. That would at least keep the encoders useful for driver-internal stuff. We could then un-revert the i915 patch again. Either way I think we should have this hack + comment with links to the offending userspace in common code, not duplicated over all drivers. -Daniel > > On Wed, 2019-10-09 at 17:01 +0200, Daniel Vetter wrote: > > On Fri, Sep 27, 2019 at 11:27:41AM -0400, Sean Paul wrote: > > > On Thu, Sep 26, 2019 at 06:51:07PM -0400, Lyude Paul wrote: > > > > This commit is seperate from the previous one to make it easier to > > > > revert in the future. Basically, there's multiple userspace applications > > > > that interpret possible_crtcs very wrong: > > > > > > > > https://gitlab.freedesktop.org/xorg/xserver/merge_requests/277 > > > > https://gitlab.gnome.org/GNOME/mutter/issues/759 > > > > > > > > While work is ongoing to fix these issues in userspace, we need to > > > > report ->possible_crtcs incorrectly for now in order to avoid > > > > introducing a regression in in userspace. Once these issues get fixed, > > > > this commit should be reverted. > > > > > > > > Signed-off-by: Lyude Paul > > > > Cc: Ville Syrjälä > > > > --- > > > > drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 11 +++ > > > > 1 file changed, 11 insertions(+) > > > > > > > > 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 b404f1ae6df7..fe8ac801d7a5 100644 > > > > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > > > > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > > > > @@ -4807,6 +4807,17 @@ static int amdgpu_dm_crtc_init(struct > > > > amdgpu_display_manager *dm, > > > > if (!acrtc->mst_encoder) > > > > goto fail; > > > > > > > > + /* > > > > +* FIXME: This is a hack to workaround the following issues: > > > > +* > > > > +* https://gitlab.gnome.org/GNOME/mutter/issues/759 > > > > +* > > > > https://gitlab.freedesktop.org/xorg/xserver/merge_requests/277 > > > > +* > > > > +* One these issues are closed, this should be removed > > > > > > Even when these issues are closed, we'll still be introducing a regression > > > if we > > > revert this change. Time for actually_possible_crtcs? :) > > > > > > You also might want to briefly explain the u/s bug in case the links go > > > sour. > > > > > > > +*/ > > > > + acrtc->mst_encoder->base.possible_crtcs = > > > > + amdgpu_dm_get_encoder_crtc_mask(dm->adev); > > > > > > Why don't we put this hack in amdgpu_dm_dp_create_fake_mst_encoder()? > > > > If we don't have the same hack for i915 mst I think we shouldn't merge > > this ... broken userspace is broken. > > -Daniel > -- > Cheers, > Lyude Paul > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 1/2] drm/amdgpu/uvd6: fix allocation size in enc ring test
Am 11.10.19 um 22:50 schrieb Alex Deucher: We need to allocate a large enough buffer for the session info, otherwise the IB test can overwrite other memory. Bug: https://bugzilla.kernel.org/show_bug.cgi?id=204241 Signed-off-by: Alex Deucher Acked-by: Christian König for the series. --- drivers/gpu/drm/amd/amdgpu/uvd_v6_0.c | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/uvd_v6_0.c b/drivers/gpu/drm/amd/amdgpu/uvd_v6_0.c index 670784a78512..909bc2ce791f 100644 --- a/drivers/gpu/drm/amd/amdgpu/uvd_v6_0.c +++ b/drivers/gpu/drm/amd/amdgpu/uvd_v6_0.c @@ -215,12 +215,12 @@ static int uvd_v6_0_enc_get_create_msg(struct amdgpu_ring *ring, uint32_t handle uint64_t dummy; int i, r; - r = amdgpu_job_alloc_with_ib(ring->adev, ib_size_dw * 4, ); + r = amdgpu_job_alloc_with_ib(ring->adev, (ib_size_dw * 4) + 1024, ); if (r) return r; ib = >ibs[0]; - dummy = ib->gpu_addr + 1024; + dummy = ib->gpu_addr + (ib_size_dw * 4); ib->length_dw = 0; ib->ptr[ib->length_dw++] = 0x0018; @@ -277,12 +277,12 @@ static int uvd_v6_0_enc_get_destroy_msg(struct amdgpu_ring *ring, uint64_t dummy; int i, r; - r = amdgpu_job_alloc_with_ib(ring->adev, ib_size_dw * 4, ); + r = amdgpu_job_alloc_with_ib(ring->adev, (ib_size_dw * 4) + 1024, ); if (r) return r; ib = >ibs[0]; - dummy = ib->gpu_addr + 1024; + dummy = ib->gpu_addr + (ib_size_dw * 4); ib->length_dw = 0; ib->ptr[ib->length_dw++] = 0x0018; ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 7/8] drm/amdgpu: reserve vram for memory training
Am 12.10.19 um 01:23 schrieb Tuikov, Luben:
> On 2019-10-10 11:50 p.m., Tianci Yin wrote:
>> From: "Tianci.Yin"
>>
>> memory training using specific fixed vram segment, reserve these
>> segments before anyone may allocate it.
>>
>> Change-Id: I1436755813a565608a2857a683f535377620a637
>> Reviewed-by: Alex Deucher
>> Signed-off-by: Tianci.Yin
>> ---
>> drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 96 +
>> 1 file changed, 96 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>> index 9da6350a4ba2..42d0fcb98382 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>> @@ -1667,6 +1667,93 @@ static int amdgpu_ttm_fw_reserve_vram_init(struct
>> amdgpu_device *adev)
>>>fw_vram_usage.va);
>> }
>>
>> +/*
>> + * Memoy training reservation functions
>> + */
>> +
>> +/**
>> + * amdgpu_ttm_training_reserve_vram_fini - free memory training reserved
>> vram
>> + *
>> + * @adev: amdgpu_device pointer
>> + *
>> + * free memory training reserved vram if it has been reserved.
>> + */
>> +static int amdgpu_ttm_training_reserve_vram_fini(struct amdgpu_device *adev)
>> +{
>> +struct psp_memory_training_context *ctx = >psp.mem_train_ctx;
>> +
>> +ctx->init = PSP_MEM_TRAIN_NOT_SUPPORT;
>> +if (ctx->c2p_bo) {
>> +amdgpu_bo_free_kernel(>c2p_bo, NULL, NULL);
>> +ctx->c2p_bo = NULL;
>> +}
> Generally it is a good idea to paragraph your code.
> So an empty line between if-statements is a good idea.
> However, there is no need in:
>
> ret = f(x);
> if (ret) {
>
> }
>
> if (blah) {
>
> }
>
> The above are two (2) well-formed paragraphs.
Additional to that amdgpu_bo_free_kernel() just like kfree() is NULL
safe for the reason that you shouldn't need "if"s like that one.
E.g. just write:
amdgpu_bo_free_kernel(>c2p_bo...);
and you are done.
Regards,
Christian.
>
>> +if (ctx->p2c_bo) {
>> +amdgpu_bo_free_kernel(>p2c_bo, NULL, NULL);
>> +ctx->p2c_bo = NULL;
>> +}
>> +
>> +return 0;
>> +}
>> +
>> +/**
>> + * amdgpu_ttm_training_reserve_vram_init - create bo vram reservation from
>> memory training
>> + *
>> + * @adev: amdgpu_device pointer
>> + *
>> + * create bo vram reservation from memory training.
>> + */
>> +static int amdgpu_ttm_training_reserve_vram_init(struct amdgpu_device *adev)
>> +{
>> +int ret;
>> +struct psp_memory_training_context *ctx = >psp.mem_train_ctx;
>> +
>> +memset(ctx, 0, sizeof(*ctx));
>> +if (!adev->fw_vram_usage.mem_train_support) {
>> +DRM_DEBUG("memory training does not support!\n");
>> +return 0;
>> +}
>> +
>> +ctx->c2p_train_data_offset = adev->fw_vram_usage.mem_train_fb_loc;
>> +ctx->p2c_train_data_offset = (adev->gmc.mc_vram_size -
>> GDDR6_MEM_TRAINING_OFFSET);
>> +ctx->train_data_size = GDDR6_MEM_TRAINING_DATA_SIZE_IN_BYTES;
>> +
>> +
>> DRM_DEBUG("train_data_size:%llx,p2c_train_data_offset:%llx,c2p_train_data_offset:%llx.\n",
>> + ctx->train_data_size,
>> + ctx->p2c_train_data_offset,
>> + ctx->c2p_train_data_offset);
>> +
>> +ret = amdgpu_bo_create_kernel_at(adev,
>> + ctx->p2c_train_data_offset,
>> + ctx->train_data_size,
>> + AMDGPU_GEM_DOMAIN_VRAM,
>> + >p2c_bo,
>> + NULL);
>> +if (ret) {
>> +DRM_ERROR("alloc p2c_bo failed(%d)!\n", ret);
>> +ret = -ENOMEM;
>> +goto err_out;
>> +}
> NAK!
> Why are you re-writing the error code from "amdgpu_bo_create_kenrel_at()"?
> Pass the error as is.
>
>> +
>> +ret = amdgpu_bo_create_kernel_at(adev,
>> + ctx->c2p_train_data_offset,
>> + ctx->train_data_size,
>> + AMDGPU_GEM_DOMAIN_VRAM,
>> + >c2p_bo,
>> + NULL);
>> +if (ret) {
>> +DRM_ERROR("alloc c2p_bo failed(%d)!\n", ret);
>> +ret = -ENOMEM;
>> +goto err_out;
>> +}
> NAK!
> Why are you re-writing the error code from "amdgpu_bo_create_kenrel_at()"?
> Pass the error as is.
>
>> +
>> +ctx->init = PSP_MEM_TRAIN_RESERVE_SUCCESS;
>> +return 0;
>> +
>> +err_out:
> Yes... well "err_out" could be any identifier, including
> a variable, as our variables follow snake-notation, all lowercase.
>
> Back at the turn of this century, Linux followed capitalized
> goto labels to distinguish them from anything else around
> in the kernel code:
>
> goto Bad_err;
> ...
>
> return 0;
> Bad_err:
> return bad_gift;
> }
>
> To distinguish that a capitalized identifier is a goto
Re: [PATCH 7/8] drm/amdgpu: reserve vram for memory training
Thanks very much Christian!
Rico
From: Koenig, Christian
Sent: Monday, October 14, 2019 16:26
To: Tuikov, Luben ; Yin, Tianci (Rico)
; amd-gfx@lists.freedesktop.org
Cc: Deucher, Alexander
Subject: Re: [PATCH 7/8] drm/amdgpu: reserve vram for memory training
Am 12.10.19 um 01:23 schrieb Tuikov, Luben:
> On 2019-10-10 11:50 p.m., Tianci Yin wrote:
>> From: "Tianci.Yin"
>>
>> memory training using specific fixed vram segment, reserve these
>> segments before anyone may allocate it.
>>
>> Change-Id: I1436755813a565608a2857a683f535377620a637
>> Reviewed-by: Alex Deucher
>> Signed-off-by: Tianci.Yin
>> ---
>> drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 96 +
>> 1 file changed, 96 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>> index 9da6350a4ba2..42d0fcb98382 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>> @@ -1667,6 +1667,93 @@ static int amdgpu_ttm_fw_reserve_vram_init(struct
>> amdgpu_device *adev)
>> >fw_vram_usage.va);
>> }
>>
>> +/*
>> + * Memoy training reservation functions
>> + */
>> +
>> +/**
>> + * amdgpu_ttm_training_reserve_vram_fini - free memory training reserved
>> vram
>> + *
>> + * @adev: amdgpu_device pointer
>> + *
>> + * free memory training reserved vram if it has been reserved.
>> + */
>> +static int amdgpu_ttm_training_reserve_vram_fini(struct amdgpu_device *adev)
>> +{
>> +struct psp_memory_training_context *ctx = >psp.mem_train_ctx;
>> +
>> +ctx->init = PSP_MEM_TRAIN_NOT_SUPPORT;
>> +if (ctx->c2p_bo) {
>> +amdgpu_bo_free_kernel(>c2p_bo, NULL, NULL);
>> +ctx->c2p_bo = NULL;
>> +}
> Generally it is a good idea to paragraph your code.
> So an empty line between if-statements is a good idea.
> However, there is no need in:
>
> ret = f(x);
> if (ret) {
>
> }
>
> if (blah) {
>
> }
>
> The above are two (2) well-formed paragraphs.
Additional to that amdgpu_bo_free_kernel() just like kfree() is NULL
safe for the reason that you shouldn't need "if"s like that one.
E.g. just write:
amdgpu_bo_free_kernel(>c2p_bo...);
and you are done.
Regards,
Christian.
>
>> +if (ctx->p2c_bo) {
>> +amdgpu_bo_free_kernel(>p2c_bo, NULL, NULL);
>> +ctx->p2c_bo = NULL;
>> +}
>> +
>> +return 0;
>> +}
>> +
>> +/**
>> + * amdgpu_ttm_training_reserve_vram_init - create bo vram reservation from
>> memory training
>> + *
>> + * @adev: amdgpu_device pointer
>> + *
>> + * create bo vram reservation from memory training.
>> + */
>> +static int amdgpu_ttm_training_reserve_vram_init(struct amdgpu_device *adev)
>> +{
>> +int ret;
>> +struct psp_memory_training_context *ctx = >psp.mem_train_ctx;
>> +
>> +memset(ctx, 0, sizeof(*ctx));
>> +if (!adev->fw_vram_usage.mem_train_support) {
>> +DRM_DEBUG("memory training does not support!\n");
>> +return 0;
>> +}
>> +
>> +ctx->c2p_train_data_offset = adev->fw_vram_usage.mem_train_fb_loc;
>> +ctx->p2c_train_data_offset = (adev->gmc.mc_vram_size -
>> GDDR6_MEM_TRAINING_OFFSET);
>> +ctx->train_data_size = GDDR6_MEM_TRAINING_DATA_SIZE_IN_BYTES;
>> +
>> +
>> DRM_DEBUG("train_data_size:%llx,p2c_train_data_offset:%llx,c2p_train_data_offset:%llx.\n",
>> + ctx->train_data_size,
>> + ctx->p2c_train_data_offset,
>> + ctx->c2p_train_data_offset);
>> +
>> +ret = amdgpu_bo_create_kernel_at(adev,
>> + ctx->p2c_train_data_offset,
>> + ctx->train_data_size,
>> + AMDGPU_GEM_DOMAIN_VRAM,
>> + >p2c_bo,
>> + NULL);
>> +if (ret) {
>> +DRM_ERROR("alloc p2c_bo failed(%d)!\n", ret);
>> +ret = -ENOMEM;
>> +goto err_out;
>> +}
> NAK!
> Why are you re-writing the error code from "amdgpu_bo_create_kenrel_at()"?
> Pass the error as is.
>
>> +
>> +ret = amdgpu_bo_create_kernel_at(adev,
>> + ctx->c2p_train_data_offset,
>> + ctx->train_data_size,
>> + AMDGPU_GEM_DOMAIN_VRAM,
>> + >c2p_bo,
>> + NULL);
>> +if (ret) {
>> +DRM_ERROR("alloc c2p_bo failed(%d)!\n", ret);
>> +ret = -ENOMEM;
>> +goto err_out;
>> +}
> NAK!
> Why are you re-writing the error code from "amdgpu_bo_create_kenrel_at()"?
> Pass the error as is.
>
>> +
>> +ctx->init = PSP_MEM_TRAIN_RESERVE_SUCCESS;
>> +return 0;
>> +
>> +err_out:
> Yes... well "err_out" could be any identifier, including
> a variable, as our variables follow snake-notation, all lowercase.
>
>
Re: [PATCH 1/2] drm/amdgpu/uvd6: fix allocation size in enc ring test
On Mon, Oct 14, 2019 at 5:06 AM Christian König wrote: > > Am 11.10.19 um 22:50 schrieb Alex Deucher: > > We need to allocate a large enough buffer for the > > session info, otherwise the IB test can overwrite > > other memory. > > > > Bug: https://bugzilla.kernel.org/show_bug.cgi?id=204241 > > Signed-off-by: Alex Deucher > > Acked-by: Christian König for the series. + Leo, James Seems like we still overwrite the buffer. Do you know how big the session buffer needs to be? Is it different for UVD and VCN? Alex > > > --- > > drivers/gpu/drm/amd/amdgpu/uvd_v6_0.c | 8 > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/uvd_v6_0.c > > b/drivers/gpu/drm/amd/amdgpu/uvd_v6_0.c > > index 670784a78512..909bc2ce791f 100644 > > --- a/drivers/gpu/drm/amd/amdgpu/uvd_v6_0.c > > +++ b/drivers/gpu/drm/amd/amdgpu/uvd_v6_0.c > > @@ -215,12 +215,12 @@ static int uvd_v6_0_enc_get_create_msg(struct > > amdgpu_ring *ring, uint32_t handle > > uint64_t dummy; > > int i, r; > > > > - r = amdgpu_job_alloc_with_ib(ring->adev, ib_size_dw * 4, ); > > + r = amdgpu_job_alloc_with_ib(ring->adev, (ib_size_dw * 4) + 1024, > > ); > > if (r) > > return r; > > > > ib = >ibs[0]; > > - dummy = ib->gpu_addr + 1024; > > + dummy = ib->gpu_addr + (ib_size_dw * 4); > > > > ib->length_dw = 0; > > ib->ptr[ib->length_dw++] = 0x0018; > > @@ -277,12 +277,12 @@ static int uvd_v6_0_enc_get_destroy_msg(struct > > amdgpu_ring *ring, > > uint64_t dummy; > > int i, r; > > > > - r = amdgpu_job_alloc_with_ib(ring->adev, ib_size_dw * 4, ); > > + r = amdgpu_job_alloc_with_ib(ring->adev, (ib_size_dw * 4) + 1024, > > ); > > if (r) > > return r; > > > > ib = >ibs[0]; > > - dummy = ib->gpu_addr + 1024; > > + dummy = ib->gpu_addr + (ib_size_dw * 4); > > > > ib->length_dw = 0; > > ib->ptr[ib->length_dw++] = 0x0018; > ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH] SWDEV-206718 drm/amdgpu: Fix tdr3 could hang with slow compute issue
Acked-by: Alex Deucher
From: amd-gfx on behalf of Deng, Emily
Sent: Saturday, October 12, 2019 1:36 AM
To: Deng, Emily ; amd-gfx@lists.freedesktop.org
Subject: RE: [PATCH] SWDEV-206718 drm/amdgpu: Fix tdr3 could hang with slow
compute issue
Ping
Best wishes
Emily Deng
>-Original Message-
>From: Emily Deng
>Sent: Wednesday, October 9, 2019 6:52 PM
>To: amd-gfx@lists.freedesktop.org
>Cc: Deng, Emily
>Subject: [PATCH] SWDEV-206718 drm/amdgpu: Fix tdr3 could hang with slow
>compute issue
>
>When index is 1, need to set compute ring timeout for sriov and passthrough.
>
>Signed-off-by: Emily Deng
>---
> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 5 -
> drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c| 6 --
> 2 files changed, 8 insertions(+), 3 deletions(-)
>
>diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>index 53ce227..2f5a015 100644
>--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>@@ -2664,8 +2664,11 @@ static int
>amdgpu_device_get_job_timeout_settings(struct amdgpu_device *adev)
> * There is only one value specified and
> * it should apply to all non-compute jobs.
> */
>- if (index == 1)
>+ if (index == 1) {
>adev->sdma_timeout = adev->video_timeout = adev-
>>gfx_timeout;
>+ if (amdgpu_sriov_vf(adev) ||
>amdgpu_passthrough(adev))
>+ adev->compute_timeout = adev->gfx_timeout;
>+ }
>}
>
>return ret;
>diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>index a88ea74..311abc8 100644
>--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>@@ -250,9 +250,11 @@ module_param_named(msi, amdgpu_msi, int, 0444);
> * By default(with no lockup_timeout settings), the timeout for all non-
>compute(GFX, SDMA and Video)
> * jobs is 1. And there is no timeout enforced on compute jobs.
> */
>-MODULE_PARM_DESC(lockup_timeout, "GPU lockup timeout in ms (default:
>1 for non-compute jobs and infinity timeout for compute jobs."
>+MODULE_PARM_DESC(lockup_timeout, "GPU lockup timeout in ms (default:
>for bare metal 1 for non-compute jobs and infinity timeout for compute
>jobs; "
>+ "for passthrough or sriov, 1 for all jobs."
>" 0: keep default value. negative: infinity timeout), "
>- "format is [Non-Compute] or [GFX,Compute,SDMA,Video]");
>+ "format: for bare metal [Non-Compute] or
>[GFX,Compute,SDMA,Video]; "
>+ "for passthrough or sriov [all jobs] or
>[GFX,Compute,SDMA,Video].");
> module_param_string(lockup_timeout, amdgpu_lockup_timeout,
>sizeof(amdgpu_lockup_timeout), 0444);
>
> /**
>--
>2.7.4
___
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/2] drm/amdgpu/uvd6: fix allocation size in enc ring test
Am 14.10.19 um 15:01 schrieb Alex Deucher: > On Mon, Oct 14, 2019 at 5:06 AM Christian König > wrote: >> Am 11.10.19 um 22:50 schrieb Alex Deucher: >>> We need to allocate a large enough buffer for the >>> session info, otherwise the IB test can overwrite >>> other memory. >>> >>> Bug: https://bugzilla.kernel.org/show_bug.cgi?id=204241 >>> Signed-off-by: Alex Deucher >> Acked-by: Christian König for the series. > + Leo, James > > Seems like we still overwrite the buffer. Do you know how big the > session buffer needs to be? Is it different for UVD and VCN? At least originally we allocated a separate 4KB BO in VRAM for this. The message was quite large IIRC. Christian. > > Alex > >>> --- >>>drivers/gpu/drm/amd/amdgpu/uvd_v6_0.c | 8 >>>1 file changed, 4 insertions(+), 4 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/amd/amdgpu/uvd_v6_0.c >>> b/drivers/gpu/drm/amd/amdgpu/uvd_v6_0.c >>> index 670784a78512..909bc2ce791f 100644 >>> --- a/drivers/gpu/drm/amd/amdgpu/uvd_v6_0.c >>> +++ b/drivers/gpu/drm/amd/amdgpu/uvd_v6_0.c >>> @@ -215,12 +215,12 @@ static int uvd_v6_0_enc_get_create_msg(struct >>> amdgpu_ring *ring, uint32_t handle >>>uint64_t dummy; >>>int i, r; >>> >>> - r = amdgpu_job_alloc_with_ib(ring->adev, ib_size_dw * 4, ); >>> + r = amdgpu_job_alloc_with_ib(ring->adev, (ib_size_dw * 4) + 1024, >>> ); >>>if (r) >>>return r; >>> >>>ib = >ibs[0]; >>> - dummy = ib->gpu_addr + 1024; >>> + dummy = ib->gpu_addr + (ib_size_dw * 4); >>> >>>ib->length_dw = 0; >>>ib->ptr[ib->length_dw++] = 0x0018; >>> @@ -277,12 +277,12 @@ static int uvd_v6_0_enc_get_destroy_msg(struct >>> amdgpu_ring *ring, >>>uint64_t dummy; >>>int i, r; >>> >>> - r = amdgpu_job_alloc_with_ib(ring->adev, ib_size_dw * 4, ); >>> + r = amdgpu_job_alloc_with_ib(ring->adev, (ib_size_dw * 4) + 1024, >>> ); >>>if (r) >>>return r; >>> >>>ib = >ibs[0]; >>> - dummy = ib->gpu_addr + 1024; >>> + dummy = ib->gpu_addr + (ib_size_dw * 4); >>> >>>ib->length_dw = 0; >>>ib->ptr[ib->length_dw++] = 0x0018; ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH] drm/amd/powerplay: enable Arcturus runtime VCN dpm on/off
Reviewed-by: Alex Deucher
From: amd-gfx on behalf of Quan, Evan
Sent: Friday, October 11, 2019 11:29 PM
To: amd-gfx@lists.freedesktop.org
Cc: Quan, Evan
Subject: [PATCH] drm/amd/powerplay: enable Arcturus runtime VCN dpm on/off
Enable runtime VCN DPM on/off on Arcturus.
Change-Id: Ie7d94d67cb4c622c96acced1b5ef0f4e63db5aad
Signed-off-by: Evan Quan
---
drivers/gpu/drm/amd/amdgpu/vcn_v2_5.c| 7 +
drivers/gpu/drm/amd/powerplay/arcturus_ppt.c | 30
2 files changed, 37 insertions(+)
diff --git a/drivers/gpu/drm/amd/amdgpu/vcn_v2_5.c
b/drivers/gpu/drm/amd/amdgpu/vcn_v2_5.c
index 2608c932a775..d270df892223 100644
--- a/drivers/gpu/drm/amd/amdgpu/vcn_v2_5.c
+++ b/drivers/gpu/drm/amd/amdgpu/vcn_v2_5.c
@@ -25,6 +25,7 @@
#include
#include "amdgpu.h"
#include "amdgpu_vcn.h"
+#include "amdgpu_pm.h"
#include "soc15.h"
#include "soc15d.h"
#include "vcn_v2_0.h"
@@ -709,6 +710,9 @@ static int vcn_v2_5_start(struct amdgpu_device *adev)
uint32_t rb_bufsz, tmp;
int i, j, k, r;
+ if (adev->pm.dpm_enabled)
+ amdgpu_dpm_enable_uvd(adev, true);
+
for (i = 0; i < adev->vcn.num_vcn_inst; ++i) {
if (adev->vcn.harvest_config & (1 << i))
continue;
@@ -939,6 +943,9 @@ static int vcn_v2_5_stop(struct amdgpu_device *adev)
~UVD_POWER_STATUS__UVD_POWER_STATUS_MASK);
}
+ if (adev->pm.dpm_enabled)
+ amdgpu_dpm_enable_uvd(adev, false);
+
return 0;
}
diff --git a/drivers/gpu/drm/amd/powerplay/arcturus_ppt.c
b/drivers/gpu/drm/amd/powerplay/arcturus_ppt.c
index a00b60968909..6731fed5458e 100644
--- a/drivers/gpu/drm/amd/powerplay/arcturus_ppt.c
+++ b/drivers/gpu/drm/amd/powerplay/arcturus_ppt.c
@@ -1907,6 +1907,35 @@ static bool arcturus_is_dpm_running(struct smu_context
*smu)
return !!(feature_enabled & SMC_DPM_FEATURE);
}
+static int arcturus_dpm_set_uvd_enable(struct smu_context *smu, bool enable)
+{
+ struct smu_power_context *smu_power = >smu_power;
+ struct smu_power_gate *power_gate = _power->power_gate;
+ int ret = 0;
+
+ if (enable) {
+ if (!smu_feature_is_enabled(smu, SMU_FEATURE_VCN_PG_BIT)) {
+ ret = smu_feature_set_enabled(smu,
SMU_FEATURE_VCN_PG_BIT, 1);
+ if (ret) {
+ pr_err("[EnableVCNDPM] failed!\n");
+ return ret;
+ }
+ }
+ power_gate->vcn_gated = false;
+ } else {
+ if (smu_feature_is_enabled(smu, SMU_FEATURE_VCN_PG_BIT)) {
+ ret = smu_feature_set_enabled(smu,
SMU_FEATURE_VCN_PG_BIT, 0);
+ if (ret) {
+ pr_err("[DisableVCNDPM] failed!\n");
+ return ret;
+ }
+ }
+ power_gate->vcn_gated = true;
+ }
+
+ return ret;
+}
+
static const struct pptable_funcs arcturus_ppt_funcs = {
/* translate smu index into arcturus specific index */
.get_smu_msg_index = arcturus_get_smu_msg_index,
@@ -1945,6 +1974,7 @@ static const struct pptable_funcs arcturus_ppt_funcs = {
.dump_pptable = arcturus_dump_pptable,
.get_power_limit = arcturus_get_power_limit,
.is_dpm_running = arcturus_is_dpm_running,
+ .dpm_set_uvd_enable = arcturus_dpm_set_uvd_enable,
};
void arcturus_set_ppt_funcs(struct smu_context *smu)
--
2.23.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
