* [PATCH v3] drm/amdgpu: add workarounds for VCN TMZ issue on CHIP_RAVEN @ 2022-03-14 2:45 Lang Yu 2022-03-14 7:51 ` Christian König 2022-03-15 16:09 ` Paul Menzel 0 siblings, 2 replies; 12+ messages in thread From: Lang Yu @ 2022-03-14 2:45 UTC (permalink / raw) To: amd-gfx; +Cc: Alex Deucher, Huang Rui, Lang Yu, Christian Koenig It is a hardware issue that VCN can't handle a GTT backing stored TMZ buffer on CHIP_RAVEN series ASIC. Move such a TMZ buffer to VRAM domain before command submission as a wrokaround. v2: - Use patch_cs_in_place callback. v3: - Bail out early if unsecure IBs. Suggested-by: Christian König <christian.koenig@amd.com> Signed-off-by: Lang Yu <Lang.Yu@amd.com> --- drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c | 71 +++++++++++++++++++++++++++ 1 file changed, 71 insertions(+) diff --git a/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c b/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c index 7bbb9ba6b80b..1ac9e06d3a4d 100644 --- a/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c +++ b/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c @@ -24,6 +24,7 @@ #include <linux/firmware.h> #include "amdgpu.h" +#include "amdgpu_cs.h" #include "amdgpu_vcn.h" #include "amdgpu_pm.h" #include "soc15.h" @@ -1905,6 +1906,75 @@ static const struct amd_ip_funcs vcn_v1_0_ip_funcs = { .set_powergating_state = vcn_v1_0_set_powergating_state, }; +/* + * It is a hardware issue that VCN can't handle a GTT TMZ buffer on + * CHIP_RAVEN series ASIC. Move such a GTT TMZ buffer to VRAM domain + * before command submission as a workaround. + */ +static int vcn_v1_0_validate_bo(struct amdgpu_cs_parser *parser, + struct amdgpu_job *job, + uint64_t addr) +{ + struct ttm_operation_ctx ctx = { false, false }; + struct amdgpu_fpriv *fpriv = parser->filp->driver_priv; + struct amdgpu_vm *vm = &fpriv->vm; + struct amdgpu_bo_va_mapping *mapping; + struct amdgpu_bo *bo; + int r; + + addr &= AMDGPU_GMC_HOLE_MASK; + if (addr & 0x7) { + DRM_ERROR("VCN messages must be 8 byte aligned!\n"); + return -EINVAL; + } + + mapping = amdgpu_vm_bo_lookup_mapping(vm, addr/AMDGPU_GPU_PAGE_SIZE); + if (!mapping || !mapping->bo_va || !mapping->bo_va->base.bo) + return -EINVAL; + + bo = mapping->bo_va->base.bo; + if (!(bo->flags & AMDGPU_GEM_CREATE_ENCRYPTED)) + return 0; + + amdgpu_bo_placement_from_domain(bo, AMDGPU_GEM_DOMAIN_VRAM); + r = ttm_bo_validate(&bo->tbo, &bo->placement, &ctx); + if (r) { + DRM_ERROR("Failed validating the VCN message BO (%d)!\n", r); + return r; + } + + return r; +} + +static int vcn_v1_0_ring_patch_cs_in_place(struct amdgpu_cs_parser *p, + struct amdgpu_job *job, + struct amdgpu_ib *ib) +{ + uint32_t msg_lo = 0, msg_hi = 0; + int i, r; + + if (!(ib->flags & AMDGPU_IB_FLAGS_SECURE)) + return 0; + + for (i = 0; i < ib->length_dw; i += 2) { + uint32_t reg = amdgpu_ib_get_value(ib, i); + uint32_t val = amdgpu_ib_get_value(ib, i + 1); + + if (reg == PACKET0(p->adev->vcn.internal.data0, 0)) { + msg_lo = val; + } else if (reg == PACKET0(p->adev->vcn.internal.data1, 0)) { + msg_hi = val; + } else if (reg == PACKET0(p->adev->vcn.internal.cmd, 0)) { + r = vcn_v1_0_validate_bo(p, job, + ((u64)msg_hi) << 32 | msg_lo); + if (r) + return r; + } + } + + return 0; +} + static const struct amdgpu_ring_funcs vcn_v1_0_dec_ring_vm_funcs = { .type = AMDGPU_RING_TYPE_VCN_DEC, .align_mask = 0xf, @@ -1914,6 +1984,7 @@ static const struct amdgpu_ring_funcs vcn_v1_0_dec_ring_vm_funcs = { .get_rptr = vcn_v1_0_dec_ring_get_rptr, .get_wptr = vcn_v1_0_dec_ring_get_wptr, .set_wptr = vcn_v1_0_dec_ring_set_wptr, + .patch_cs_in_place = vcn_v1_0_ring_patch_cs_in_place, .emit_frame_size = 6 + 6 + /* hdp invalidate / flush */ SOC15_FLUSH_GPU_TLB_NUM_WREG * 6 + -- 2.25.1 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v3] drm/amdgpu: add workarounds for VCN TMZ issue on CHIP_RAVEN 2022-03-14 2:45 [PATCH v3] drm/amdgpu: add workarounds for VCN TMZ issue on CHIP_RAVEN Lang Yu @ 2022-03-14 7:51 ` Christian König 2022-03-15 14:28 ` Lang Yu 2022-03-15 16:09 ` Paul Menzel 1 sibling, 1 reply; 12+ messages in thread From: Christian König @ 2022-03-14 7:51 UTC (permalink / raw) To: Lang Yu, amd-gfx; +Cc: Alex Deucher, Huang Rui Am 14.03.22 um 03:45 schrieb Lang Yu: > It is a hardware issue that VCN can't handle a GTT > backing stored TMZ buffer on CHIP_RAVEN series ASIC. > > Move such a TMZ buffer to VRAM domain before command > submission as a wrokaround. > > v2: > - Use patch_cs_in_place callback. > > v3: > - Bail out early if unsecure IBs. > > Suggested-by: Christian König <christian.koenig@amd.com> > Signed-off-by: Lang Yu <Lang.Yu@amd.com> Reviewed-by: Christian König <christian.koenig@amd.com> But I would ask Leo to give his ok as well. Thanks, Christian. > --- > drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c | 71 +++++++++++++++++++++++++++ > 1 file changed, 71 insertions(+) > > diff --git a/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c b/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c > index 7bbb9ba6b80b..1ac9e06d3a4d 100644 > --- a/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c > +++ b/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c > @@ -24,6 +24,7 @@ > #include <linux/firmware.h> > > #include "amdgpu.h" > +#include "amdgpu_cs.h" > #include "amdgpu_vcn.h" > #include "amdgpu_pm.h" > #include "soc15.h" > @@ -1905,6 +1906,75 @@ static const struct amd_ip_funcs vcn_v1_0_ip_funcs = { > .set_powergating_state = vcn_v1_0_set_powergating_state, > }; > > +/* > + * It is a hardware issue that VCN can't handle a GTT TMZ buffer on > + * CHIP_RAVEN series ASIC. Move such a GTT TMZ buffer to VRAM domain > + * before command submission as a workaround. > + */ > +static int vcn_v1_0_validate_bo(struct amdgpu_cs_parser *parser, > + struct amdgpu_job *job, > + uint64_t addr) > +{ > + struct ttm_operation_ctx ctx = { false, false }; > + struct amdgpu_fpriv *fpriv = parser->filp->driver_priv; > + struct amdgpu_vm *vm = &fpriv->vm; > + struct amdgpu_bo_va_mapping *mapping; > + struct amdgpu_bo *bo; > + int r; > + > + addr &= AMDGPU_GMC_HOLE_MASK; > + if (addr & 0x7) { > + DRM_ERROR("VCN messages must be 8 byte aligned!\n"); > + return -EINVAL; > + } > + > + mapping = amdgpu_vm_bo_lookup_mapping(vm, addr/AMDGPU_GPU_PAGE_SIZE); > + if (!mapping || !mapping->bo_va || !mapping->bo_va->base.bo) > + return -EINVAL; > + > + bo = mapping->bo_va->base.bo; > + if (!(bo->flags & AMDGPU_GEM_CREATE_ENCRYPTED)) > + return 0; > + > + amdgpu_bo_placement_from_domain(bo, AMDGPU_GEM_DOMAIN_VRAM); > + r = ttm_bo_validate(&bo->tbo, &bo->placement, &ctx); > + if (r) { > + DRM_ERROR("Failed validating the VCN message BO (%d)!\n", r); > + return r; > + } > + > + return r; > +} > + > +static int vcn_v1_0_ring_patch_cs_in_place(struct amdgpu_cs_parser *p, > + struct amdgpu_job *job, > + struct amdgpu_ib *ib) > +{ > + uint32_t msg_lo = 0, msg_hi = 0; > + int i, r; > + > + if (!(ib->flags & AMDGPU_IB_FLAGS_SECURE)) > + return 0; > + > + for (i = 0; i < ib->length_dw; i += 2) { > + uint32_t reg = amdgpu_ib_get_value(ib, i); > + uint32_t val = amdgpu_ib_get_value(ib, i + 1); > + > + if (reg == PACKET0(p->adev->vcn.internal.data0, 0)) { > + msg_lo = val; > + } else if (reg == PACKET0(p->adev->vcn.internal.data1, 0)) { > + msg_hi = val; > + } else if (reg == PACKET0(p->adev->vcn.internal.cmd, 0)) { > + r = vcn_v1_0_validate_bo(p, job, > + ((u64)msg_hi) << 32 | msg_lo); > + if (r) > + return r; > + } > + } > + > + return 0; > +} > + > static const struct amdgpu_ring_funcs vcn_v1_0_dec_ring_vm_funcs = { > .type = AMDGPU_RING_TYPE_VCN_DEC, > .align_mask = 0xf, > @@ -1914,6 +1984,7 @@ static const struct amdgpu_ring_funcs vcn_v1_0_dec_ring_vm_funcs = { > .get_rptr = vcn_v1_0_dec_ring_get_rptr, > .get_wptr = vcn_v1_0_dec_ring_get_wptr, > .set_wptr = vcn_v1_0_dec_ring_set_wptr, > + .patch_cs_in_place = vcn_v1_0_ring_patch_cs_in_place, > .emit_frame_size = > 6 + 6 + /* hdp invalidate / flush */ > SOC15_FLUSH_GPU_TLB_NUM_WREG * 6 + ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3] drm/amdgpu: add workarounds for VCN TMZ issue on CHIP_RAVEN 2022-03-14 7:51 ` Christian König @ 2022-03-15 14:28 ` Lang Yu 0 siblings, 0 replies; 12+ messages in thread From: Lang Yu @ 2022-03-15 14:28 UTC (permalink / raw) To: Leo Liu; +Cc: Alex Deucher, Huang Rui, Christian König, amd-gfx On 03/14/ , Christian König wrote: > Am 14.03.22 um 03:45 schrieb Lang Yu: > > It is a hardware issue that VCN can't handle a GTT > > backing stored TMZ buffer on CHIP_RAVEN series ASIC. > > > > Move such a TMZ buffer to VRAM domain before command > > submission as a wrokaround. > > > > v2: > > - Use patch_cs_in_place callback. > > > > v3: > > - Bail out early if unsecure IBs. > > > > Suggested-by: Christian König <christian.koenig@amd.com> > > Signed-off-by: Lang Yu <Lang.Yu@amd.com> > > Reviewed-by: Christian König <christian.koenig@amd.com> > > But I would ask Leo to give his ok as well. Hi Leo, Do you have any commnets on this? Thanks. Regards, Lang > Thanks, > Christian. > > > --- > > drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c | 71 +++++++++++++++++++++++++++ > > 1 file changed, 71 insertions(+) > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c b/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c > > index 7bbb9ba6b80b..1ac9e06d3a4d 100644 > > --- a/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c > > +++ b/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c > > @@ -24,6 +24,7 @@ > > #include <linux/firmware.h> > > #include "amdgpu.h" > > +#include "amdgpu_cs.h" > > #include "amdgpu_vcn.h" > > #include "amdgpu_pm.h" > > #include "soc15.h" > > @@ -1905,6 +1906,75 @@ static const struct amd_ip_funcs vcn_v1_0_ip_funcs = { > > .set_powergating_state = vcn_v1_0_set_powergating_state, > > }; > > +/* > > + * It is a hardware issue that VCN can't handle a GTT TMZ buffer on > > + * CHIP_RAVEN series ASIC. Move such a GTT TMZ buffer to VRAM domain > > + * before command submission as a workaround. > > + */ > > +static int vcn_v1_0_validate_bo(struct amdgpu_cs_parser *parser, > > + struct amdgpu_job *job, > > + uint64_t addr) > > +{ > > + struct ttm_operation_ctx ctx = { false, false }; > > + struct amdgpu_fpriv *fpriv = parser->filp->driver_priv; > > + struct amdgpu_vm *vm = &fpriv->vm; > > + struct amdgpu_bo_va_mapping *mapping; > > + struct amdgpu_bo *bo; > > + int r; > > + > > + addr &= AMDGPU_GMC_HOLE_MASK; > > + if (addr & 0x7) { > > + DRM_ERROR("VCN messages must be 8 byte aligned!\n"); > > + return -EINVAL; > > + } > > + > > + mapping = amdgpu_vm_bo_lookup_mapping(vm, addr/AMDGPU_GPU_PAGE_SIZE); > > + if (!mapping || !mapping->bo_va || !mapping->bo_va->base.bo) > > + return -EINVAL; > > + > > + bo = mapping->bo_va->base.bo; > > + if (!(bo->flags & AMDGPU_GEM_CREATE_ENCRYPTED)) > > + return 0; > > + > > + amdgpu_bo_placement_from_domain(bo, AMDGPU_GEM_DOMAIN_VRAM); > > + r = ttm_bo_validate(&bo->tbo, &bo->placement, &ctx); > > + if (r) { > > + DRM_ERROR("Failed validating the VCN message BO (%d)!\n", r); > > + return r; > > + } > > + > > + return r; > > +} > > + > > +static int vcn_v1_0_ring_patch_cs_in_place(struct amdgpu_cs_parser *p, > > + struct amdgpu_job *job, > > + struct amdgpu_ib *ib) > > +{ > > + uint32_t msg_lo = 0, msg_hi = 0; > > + int i, r; > > + > > + if (!(ib->flags & AMDGPU_IB_FLAGS_SECURE)) > > + return 0; > > + > > + for (i = 0; i < ib->length_dw; i += 2) { > > + uint32_t reg = amdgpu_ib_get_value(ib, i); > > + uint32_t val = amdgpu_ib_get_value(ib, i + 1); > > + > > + if (reg == PACKET0(p->adev->vcn.internal.data0, 0)) { > > + msg_lo = val; > > + } else if (reg == PACKET0(p->adev->vcn.internal.data1, 0)) { > > + msg_hi = val; > > + } else if (reg == PACKET0(p->adev->vcn.internal.cmd, 0)) { > > + r = vcn_v1_0_validate_bo(p, job, > > + ((u64)msg_hi) << 32 | msg_lo); > > + if (r) > > + return r; > > + } > > + } > > + > > + return 0; > > +} > > + > > static const struct amdgpu_ring_funcs vcn_v1_0_dec_ring_vm_funcs = { > > .type = AMDGPU_RING_TYPE_VCN_DEC, > > .align_mask = 0xf, > > @@ -1914,6 +1984,7 @@ static const struct amdgpu_ring_funcs vcn_v1_0_dec_ring_vm_funcs = { > > .get_rptr = vcn_v1_0_dec_ring_get_rptr, > > .get_wptr = vcn_v1_0_dec_ring_get_wptr, > > .set_wptr = vcn_v1_0_dec_ring_set_wptr, > > + .patch_cs_in_place = vcn_v1_0_ring_patch_cs_in_place, > > .emit_frame_size = > > 6 + 6 + /* hdp invalidate / flush */ > > SOC15_FLUSH_GPU_TLB_NUM_WREG * 6 + > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3] drm/amdgpu: add workarounds for VCN TMZ issue on CHIP_RAVEN 2022-03-14 2:45 [PATCH v3] drm/amdgpu: add workarounds for VCN TMZ issue on CHIP_RAVEN Lang Yu 2022-03-14 7:51 ` Christian König @ 2022-03-15 16:09 ` Paul Menzel 2022-03-16 1:27 ` Lang Yu 1 sibling, 1 reply; 12+ messages in thread From: Paul Menzel @ 2022-03-15 16:09 UTC (permalink / raw) To: Lang Yu; +Cc: Alex Deucher, Huang Rui, Christian Koenig, amd-gfx Dear Lang, Am 14.03.22 um 03:45 schrieb Lang Yu: Thank you for your patch. A shorter commit message summary would be: > drm/amdgpu: Work around VNC TMZ issue on CHIP_RAVEN > It is a hardware issue that VCN can't handle a GTT > backing stored TMZ buffer on CHIP_RAVEN series ASIC. Where is that documented, and how can this be reproduced? > Move such a TMZ buffer to VRAM domain before command > submission as a wrokaround. workaround Please use a text width of 75 characters per line. > v2: > - Use patch_cs_in_place callback. > > v3: > - Bail out early if unsecure IBs. > > Suggested-by: Christian König <christian.koenig@amd.com> > Signed-off-by: Lang Yu <Lang.Yu@amd.com> > --- > drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c | 71 +++++++++++++++++++++++++++ > 1 file changed, 71 insertions(+) > > diff --git a/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c b/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c > index 7bbb9ba6b80b..1ac9e06d3a4d 100644 > --- a/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c > +++ b/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c > @@ -24,6 +24,7 @@ > #include <linux/firmware.h> > > #include "amdgpu.h" > +#include "amdgpu_cs.h" > #include "amdgpu_vcn.h" > #include "amdgpu_pm.h" > #include "soc15.h" > @@ -1905,6 +1906,75 @@ static const struct amd_ip_funcs vcn_v1_0_ip_funcs = { > .set_powergating_state = vcn_v1_0_set_powergating_state, > }; > > +/* > + * It is a hardware issue that VCN can't handle a GTT TMZ buffer on > + * CHIP_RAVEN series ASIC. Move such a GTT TMZ buffer to VRAM domain > + * before command submission as a workaround. > + */ > +static int vcn_v1_0_validate_bo(struct amdgpu_cs_parser *parser, > + struct amdgpu_job *job, > + uint64_t addr) > +{ > + struct ttm_operation_ctx ctx = { false, false }; > + struct amdgpu_fpriv *fpriv = parser->filp->driver_priv; > + struct amdgpu_vm *vm = &fpriv->vm; > + struct amdgpu_bo_va_mapping *mapping; > + struct amdgpu_bo *bo; > + int r; > + > + addr &= AMDGPU_GMC_HOLE_MASK; > + if (addr & 0x7) { > + DRM_ERROR("VCN messages must be 8 byte aligned!\n"); > + return -EINVAL; > + } > + > + mapping = amdgpu_vm_bo_lookup_mapping(vm, addr/AMDGPU_GPU_PAGE_SIZE); > + if (!mapping || !mapping->bo_va || !mapping->bo_va->base.bo) > + return -EINVAL; > + > + bo = mapping->bo_va->base.bo; > + if (!(bo->flags & AMDGPU_GEM_CREATE_ENCRYPTED)) > + return 0; > + > + amdgpu_bo_placement_from_domain(bo, AMDGPU_GEM_DOMAIN_VRAM); > + r = ttm_bo_validate(&bo->tbo, &bo->placement, &ctx); > + if (r) { > + DRM_ERROR("Failed validating the VCN message BO (%d)!\n", r); to validate > + return r; > + } > + > + return r; > +} > + > +static int vcn_v1_0_ring_patch_cs_in_place(struct amdgpu_cs_parser *p, > + struct amdgpu_job *job, > + struct amdgpu_ib *ib) > +{ > + uint32_t msg_lo = 0, msg_hi = 0; > + int i, r; Use unsigned int for the counter variable? > + > + if (!(ib->flags & AMDGPU_IB_FLAGS_SECURE)) > + return 0; > + > + for (i = 0; i < ib->length_dw; i += 2) { > + uint32_t reg = amdgpu_ib_get_value(ib, i); > + uint32_t val = amdgpu_ib_get_value(ib, i + 1); > + > + if (reg == PACKET0(p->adev->vcn.internal.data0, 0)) { > + msg_lo = val; > + } else if (reg == PACKET0(p->adev->vcn.internal.data1, 0)) { > + msg_hi = val; > + } else if (reg == PACKET0(p->adev->vcn.internal.cmd, 0)) { > + r = vcn_v1_0_validate_bo(p, job, > + ((u64)msg_hi) << 32 | msg_lo); > + if (r) > + return r; > + } > + } > + > + return 0; > +} > + > static const struct amdgpu_ring_funcs vcn_v1_0_dec_ring_vm_funcs = { > .type = AMDGPU_RING_TYPE_VCN_DEC, > .align_mask = 0xf, > @@ -1914,6 +1984,7 @@ static const struct amdgpu_ring_funcs vcn_v1_0_dec_ring_vm_funcs = { > .get_rptr = vcn_v1_0_dec_ring_get_rptr, > .get_wptr = vcn_v1_0_dec_ring_get_wptr, > .set_wptr = vcn_v1_0_dec_ring_set_wptr, > + .patch_cs_in_place = vcn_v1_0_ring_patch_cs_in_place, > .emit_frame_size = > 6 + 6 + /* hdp invalidate / flush */ > SOC15_FLUSH_GPU_TLB_NUM_WREG * 6 + Kind regards, Paul ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3] drm/amdgpu: add workarounds for VCN TMZ issue on CHIP_RAVEN 2022-03-15 16:09 ` Paul Menzel @ 2022-03-16 1:27 ` Lang Yu 2022-03-16 5:25 ` Paul Menzel 0 siblings, 1 reply; 12+ messages in thread From: Lang Yu @ 2022-03-16 1:27 UTC (permalink / raw) To: Paul Menzel; +Cc: Alex Deucher, Huang Rui, Christian Koenig, amd-gfx On 03/15/ , Paul Menzel wrote: > Dear Lang, > > > Am 14.03.22 um 03:45 schrieb Lang Yu: > > Thank you for your patch. A shorter commit message summary would be: > > > drm/amdgpu: Work around VNC TMZ issue on CHIP_RAVEN > > > It is a hardware issue that VCN can't handle a GTT > > backing stored TMZ buffer on CHIP_RAVEN series ASIC. > > Where is that documented, and how can this be reproduced? It is documented in AMD internal Confluence and JIRA. Secure playback with a low VRAM config(thus TMZ buffer will be allocted in GTT domain) may reproduce this issue. > > Move such a TMZ buffer to VRAM domain before command > > submission as a wrokaround. > > workaround > > Please use a text width of 75 characters per line. Thanks for your comments. I will take care about this. > > v2: > > - Use patch_cs_in_place callback. > > > > v3: > > - Bail out early if unsecure IBs. > > > > Suggested-by: Christian König <christian.koenig@amd.com> > > Signed-off-by: Lang Yu <Lang.Yu@amd.com> > > --- > > drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c | 71 +++++++++++++++++++++++++++ > > 1 file changed, 71 insertions(+) > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c b/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c > > index 7bbb9ba6b80b..1ac9e06d3a4d 100644 > > --- a/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c > > +++ b/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c > > @@ -24,6 +24,7 @@ > > #include <linux/firmware.h> > > #include "amdgpu.h" > > +#include "amdgpu_cs.h" > > #include "amdgpu_vcn.h" > > #include "amdgpu_pm.h" > > #include "soc15.h" > > @@ -1905,6 +1906,75 @@ static const struct amd_ip_funcs vcn_v1_0_ip_funcs = { > > .set_powergating_state = vcn_v1_0_set_powergating_state, > > }; > > +/* > > + * It is a hardware issue that VCN can't handle a GTT TMZ buffer on > > + * CHIP_RAVEN series ASIC. Move such a GTT TMZ buffer to VRAM domain > > + * before command submission as a workaround. > > + */ > > +static int vcn_v1_0_validate_bo(struct amdgpu_cs_parser *parser, > > + struct amdgpu_job *job, > > + uint64_t addr) > > +{ > > + struct ttm_operation_ctx ctx = { false, false }; > > + struct amdgpu_fpriv *fpriv = parser->filp->driver_priv; > > + struct amdgpu_vm *vm = &fpriv->vm; > > + struct amdgpu_bo_va_mapping *mapping; > > + struct amdgpu_bo *bo; > > + int r; > > + > > + addr &= AMDGPU_GMC_HOLE_MASK; > > + if (addr & 0x7) { > > + DRM_ERROR("VCN messages must be 8 byte aligned!\n"); > > + return -EINVAL; > > + } > > + > > + mapping = amdgpu_vm_bo_lookup_mapping(vm, addr/AMDGPU_GPU_PAGE_SIZE); > > + if (!mapping || !mapping->bo_va || !mapping->bo_va->base.bo) > > + return -EINVAL; > > + > > + bo = mapping->bo_va->base.bo; > > + if (!(bo->flags & AMDGPU_GEM_CREATE_ENCRYPTED)) > > + return 0; > > + > > + amdgpu_bo_placement_from_domain(bo, AMDGPU_GEM_DOMAIN_VRAM); > > + r = ttm_bo_validate(&bo->tbo, &bo->placement, &ctx); > > + if (r) { > > + DRM_ERROR("Failed validating the VCN message BO (%d)!\n", r); > > to validate > > > + return r; > > + } > > + > > + return r; > > +} > > + > > +static int vcn_v1_0_ring_patch_cs_in_place(struct amdgpu_cs_parser *p, > > + struct amdgpu_job *job, > > + struct amdgpu_ib *ib) > > +{ > > + uint32_t msg_lo = 0, msg_hi = 0; > > + int i, r; > > Use unsigned int for the counter variable? You can see both signed and unsigned are used in kernel. (e.g., mm/page_alloc.c). For this case, I think it makes no difference. In a word, we should consider the specific context. Regards, Lang > > + > > + if (!(ib->flags & AMDGPU_IB_FLAGS_SECURE)) > > + return 0; > > + > > + for (i = 0; i < ib->length_dw; i += 2) { > > + uint32_t reg = amdgpu_ib_get_value(ib, i); > > + uint32_t val = amdgpu_ib_get_value(ib, i + 1); > > + > > + if (reg == PACKET0(p->adev->vcn.internal.data0, 0)) { > > + msg_lo = val; > > + } else if (reg == PACKET0(p->adev->vcn.internal.data1, 0)) { > > + msg_hi = val; > > + } else if (reg == PACKET0(p->adev->vcn.internal.cmd, 0)) { > > + r = vcn_v1_0_validate_bo(p, job, > > + ((u64)msg_hi) << 32 | msg_lo); > > + if (r) > > + return r; > > + } > > + } > > + > > + return 0; > > +} > > + > > static const struct amdgpu_ring_funcs vcn_v1_0_dec_ring_vm_funcs = { > > .type = AMDGPU_RING_TYPE_VCN_DEC, > > .align_mask = 0xf, > > @@ -1914,6 +1984,7 @@ static const struct amdgpu_ring_funcs vcn_v1_0_dec_ring_vm_funcs = { > > .get_rptr = vcn_v1_0_dec_ring_get_rptr, > > .get_wptr = vcn_v1_0_dec_ring_get_wptr, > > .set_wptr = vcn_v1_0_dec_ring_set_wptr, > > + .patch_cs_in_place = vcn_v1_0_ring_patch_cs_in_place, > > .emit_frame_size = > > 6 + 6 + /* hdp invalidate / flush */ > > SOC15_FLUSH_GPU_TLB_NUM_WREG * 6 + > > > Kind regards, > > Paul ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3] drm/amdgpu: add workarounds for VCN TMZ issue on CHIP_RAVEN 2022-03-16 1:27 ` Lang Yu @ 2022-03-16 5:25 ` Paul Menzel 2022-03-16 6:21 ` Lang Yu 0 siblings, 1 reply; 12+ messages in thread From: Paul Menzel @ 2022-03-16 5:25 UTC (permalink / raw) To: Lang Yu; +Cc: Alex Deucher, Huang Rui, Christian König, amd-gfx Dear Lang, Am 16.03.22 um 02:27 schrieb Lang Yu: > On 03/15/ , Paul Menzel wrote: >> Am 14.03.22 um 03:45 schrieb Lang Yu: >> >> Thank you for your patch. A shorter commit message summary would be: >> >>> drm/amdgpu: Work around VNC TMZ issue on CHIP_RAVEN >> >>> It is a hardware issue that VCN can't handle a GTT >>> backing stored TMZ buffer on CHIP_RAVEN series ASIC. >> >> Where is that documented, and how can this be reproduced? > > It is documented in AMD internal Confluence and JIRA. > Secure playback with a low VRAM config(thus TMZ buffer > will be allocted in GTT domain) may reproduce this issue. It’d be great if as much of the details from this non-publicly accessible information could be added to the commit message, and a way to reproduce this as there does not seem to be a test for this. (Also I guess a tag with a reference to the internal issue would be acceptable, so in case more question surface in the future.) >>> Move such a TMZ buffer to VRAM domain before command >>> submission as a wrokaround. >> >> workaround >> >> Please use a text width of 75 characters per line. > > Thanks for your comments. I will take care about this. > >>> v2: >>> - Use patch_cs_in_place callback. >>> >>> v3: >>> - Bail out early if unsecure IBs. >>> >>> Suggested-by: Christian König <christian.koenig@amd.com> >>> Signed-off-by: Lang Yu <Lang.Yu@amd.com> >>> --- >>> drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c | 71 +++++++++++++++++++++++++++ >>> 1 file changed, 71 insertions(+) >>> >>> diff --git a/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c b/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c >>> index 7bbb9ba6b80b..1ac9e06d3a4d 100644 >>> --- a/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c >>> +++ b/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c >>> @@ -24,6 +24,7 @@ >>> #include <linux/firmware.h> >>> #include "amdgpu.h" >>> +#include "amdgpu_cs.h" >>> #include "amdgpu_vcn.h" >>> #include "amdgpu_pm.h" >>> #include "soc15.h" >>> @@ -1905,6 +1906,75 @@ static const struct amd_ip_funcs vcn_v1_0_ip_funcs = { >>> .set_powergating_state = vcn_v1_0_set_powergating_state, >>> }; >>> +/* >>> + * It is a hardware issue that VCN can't handle a GTT TMZ buffer on >>> + * CHIP_RAVEN series ASIC. Move such a GTT TMZ buffer to VRAM domain >>> + * before command submission as a workaround. >>> + */ >>> +static int vcn_v1_0_validate_bo(struct amdgpu_cs_parser *parser, >>> + struct amdgpu_job *job, >>> + uint64_t addr) >>> +{ >>> + struct ttm_operation_ctx ctx = { false, false }; >>> + struct amdgpu_fpriv *fpriv = parser->filp->driver_priv; >>> + struct amdgpu_vm *vm = &fpriv->vm; >>> + struct amdgpu_bo_va_mapping *mapping; >>> + struct amdgpu_bo *bo; >>> + int r; >>> + >>> + addr &= AMDGPU_GMC_HOLE_MASK; >>> + if (addr & 0x7) { >>> + DRM_ERROR("VCN messages must be 8 byte aligned!\n"); >>> + return -EINVAL; >>> + } >>> + >>> + mapping = amdgpu_vm_bo_lookup_mapping(vm, addr/AMDGPU_GPU_PAGE_SIZE); >>> + if (!mapping || !mapping->bo_va || !mapping->bo_va->base.bo) >>> + return -EINVAL; >>> + >>> + bo = mapping->bo_va->base.bo; >>> + if (!(bo->flags & AMDGPU_GEM_CREATE_ENCRYPTED)) >>> + return 0; >>> + >>> + amdgpu_bo_placement_from_domain(bo, AMDGPU_GEM_DOMAIN_VRAM); >>> + r = ttm_bo_validate(&bo->tbo, &bo->placement, &ctx); >>> + if (r) { >>> + DRM_ERROR("Failed validating the VCN message BO (%d)!\n", r); >> >> to validate >> >>> + return r; >>> + } >>> + >>> + return r; >>> +} >>> + >>> +static int vcn_v1_0_ring_patch_cs_in_place(struct amdgpu_cs_parser *p, >>> + struct amdgpu_job *job, >>> + struct amdgpu_ib *ib) >>> +{ >>> + uint32_t msg_lo = 0, msg_hi = 0; >>> + int i, r; >> >> Use unsigned int for the counter variable? > > You can see both signed and unsigned are used in kernel. > (e.g., mm/page_alloc.c). > > For this case, I think it makes no difference. > > In a word, we should consider the specific context. Although it makes technically no difference, I prefer to use the best matching type to convey the intention of the variable to the reader. But you are right, there is no hard rule for it. Kind regards, Paul >>> + >>> + if (!(ib->flags & AMDGPU_IB_FLAGS_SECURE)) >>> + return 0; >>> + >>> + for (i = 0; i < ib->length_dw; i += 2) { >>> + uint32_t reg = amdgpu_ib_get_value(ib, i); >>> + uint32_t val = amdgpu_ib_get_value(ib, i + 1); >>> + >>> + if (reg == PACKET0(p->adev->vcn.internal.data0, 0)) { >>> + msg_lo = val; >>> + } else if (reg == PACKET0(p->adev->vcn.internal.data1, 0)) { >>> + msg_hi = val; >>> + } else if (reg == PACKET0(p->adev->vcn.internal.cmd, 0)) { >>> + r = vcn_v1_0_validate_bo(p, job, >>> + ((u64)msg_hi) << 32 | msg_lo); >>> + if (r) >>> + return r; >>> + } >>> + } >>> + >>> + return 0; >>> +} >>> + >>> static const struct amdgpu_ring_funcs vcn_v1_0_dec_ring_vm_funcs = { >>> .type = AMDGPU_RING_TYPE_VCN_DEC, >>> .align_mask = 0xf, >>> @@ -1914,6 +1984,7 @@ static const struct amdgpu_ring_funcs vcn_v1_0_dec_ring_vm_funcs = { >>> .get_rptr = vcn_v1_0_dec_ring_get_rptr, >>> .get_wptr = vcn_v1_0_dec_ring_get_wptr, >>> .set_wptr = vcn_v1_0_dec_ring_set_wptr, >>> + .patch_cs_in_place = vcn_v1_0_ring_patch_cs_in_place, >>> .emit_frame_size = >>> 6 + 6 + /* hdp invalidate / flush */ >>> SOC15_FLUSH_GPU_TLB_NUM_WREG * 6 + >> >> >> Kind regards, >> >> Paul ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3] drm/amdgpu: add workarounds for VCN TMZ issue on CHIP_RAVEN 2022-03-16 5:25 ` Paul Menzel @ 2022-03-16 6:21 ` Lang Yu 2022-03-16 9:41 ` Christian König 0 siblings, 1 reply; 12+ messages in thread From: Lang Yu @ 2022-03-16 6:21 UTC (permalink / raw) To: Paul Menzel; +Cc: Alex Deucher, Huang Rui, Christian König, amd-gfx On 03/16/ , Paul Menzel wrote: > Dear Lang, > > > Am 16.03.22 um 02:27 schrieb Lang Yu: > > On 03/15/ , Paul Menzel wrote: > > > > Am 14.03.22 um 03:45 schrieb Lang Yu: > > > > > > Thank you for your patch. A shorter commit message summary would be: > > > > > > > drm/amdgpu: Work around VNC TMZ issue on CHIP_RAVEN > > > > > > > It is a hardware issue that VCN can't handle a GTT > > > > backing stored TMZ buffer on CHIP_RAVEN series ASIC. > > > > > > Where is that documented, and how can this be reproduced? > > > > It is documented in AMD internal Confluence and JIRA. > > Secure playback with a low VRAM config(thus TMZ buffer > > will be allocted in GTT domain) may reproduce this issue. > > It’d be great if as much of the details from this non-publicly accessible > information could be added to the commit message, and a way to reproduce > this as there does not seem to be a test for this. (Also I guess a tag with > a reference to the internal issue would be acceptable, so in case more > question surface in the future.) Thanks. I will add an internal link. Regards, Lang > > > > Move such a TMZ buffer to VRAM domain before command > > > > submission as a wrokaround. > > > > > > workaround > > > > > > Please use a text width of 75 characters per line. > > > > Thanks for your comments. I will take care about this. > > > > > > v2: > > > > - Use patch_cs_in_place callback. > > > > > > > > v3: > > > > - Bail out early if unsecure IBs. > > > > > > > > Suggested-by: Christian König <christian.koenig@amd.com> > > > > Signed-off-by: Lang Yu <Lang.Yu@amd.com> > > > > --- > > > > drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c | 71 +++++++++++++++++++++++++++ > > > > 1 file changed, 71 insertions(+) > > > > > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c b/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c > > > > index 7bbb9ba6b80b..1ac9e06d3a4d 100644 > > > > --- a/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c > > > > +++ b/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c > > > > @@ -24,6 +24,7 @@ > > > > #include <linux/firmware.h> > > > > #include "amdgpu.h" > > > > +#include "amdgpu_cs.h" > > > > #include "amdgpu_vcn.h" > > > > #include "amdgpu_pm.h" > > > > #include "soc15.h" > > > > @@ -1905,6 +1906,75 @@ static const struct amd_ip_funcs vcn_v1_0_ip_funcs = { > > > > .set_powergating_state = vcn_v1_0_set_powergating_state, > > > > }; > > > > +/* > > > > + * It is a hardware issue that VCN can't handle a GTT TMZ buffer on > > > > + * CHIP_RAVEN series ASIC. Move such a GTT TMZ buffer to VRAM domain > > > > + * before command submission as a workaround. > > > > + */ > > > > +static int vcn_v1_0_validate_bo(struct amdgpu_cs_parser *parser, > > > > + struct amdgpu_job *job, > > > > + uint64_t addr) > > > > +{ > > > > + struct ttm_operation_ctx ctx = { false, false }; > > > > + struct amdgpu_fpriv *fpriv = parser->filp->driver_priv; > > > > + struct amdgpu_vm *vm = &fpriv->vm; > > > > + struct amdgpu_bo_va_mapping *mapping; > > > > + struct amdgpu_bo *bo; > > > > + int r; > > > > + > > > > + addr &= AMDGPU_GMC_HOLE_MASK; > > > > + if (addr & 0x7) { > > > > + DRM_ERROR("VCN messages must be 8 byte aligned!\n"); > > > > + return -EINVAL; > > > > + } > > > > + > > > > + mapping = amdgpu_vm_bo_lookup_mapping(vm, addr/AMDGPU_GPU_PAGE_SIZE); > > > > + if (!mapping || !mapping->bo_va || !mapping->bo_va->base.bo) > > > > + return -EINVAL; > > > > + > > > > + bo = mapping->bo_va->base.bo; > > > > + if (!(bo->flags & AMDGPU_GEM_CREATE_ENCRYPTED)) > > > > + return 0; > > > > + > > > > + amdgpu_bo_placement_from_domain(bo, AMDGPU_GEM_DOMAIN_VRAM); > > > > + r = ttm_bo_validate(&bo->tbo, &bo->placement, &ctx); > > > > + if (r) { > > > > + DRM_ERROR("Failed validating the VCN message BO (%d)!\n", r); > > > > > > to validate > > > > > > > + return r; > > > > + } > > > > + > > > > + return r; > > > > +} > > > > + > > > > +static int vcn_v1_0_ring_patch_cs_in_place(struct amdgpu_cs_parser *p, > > > > + struct amdgpu_job *job, > > > > + struct amdgpu_ib *ib) > > > > +{ > > > > + uint32_t msg_lo = 0, msg_hi = 0; > > > > + int i, r; > > > > > > Use unsigned int for the counter variable? > > > > You can see both signed and unsigned are used in kernel. > > (e.g., mm/page_alloc.c). > > > > For this case, I think it makes no difference. > > > > In a word, we should consider the specific context. > > Although it makes technically no difference, I prefer to use the best > matching type to convey the intention of the variable to the reader. But you > are right, there is no hard rule for it. > > > Kind regards, > > Paul > > > > > + > > > > + if (!(ib->flags & AMDGPU_IB_FLAGS_SECURE)) > > > > + return 0; > > > > + > > > > + for (i = 0; i < ib->length_dw; i += 2) { > > > > + uint32_t reg = amdgpu_ib_get_value(ib, i); > > > > + uint32_t val = amdgpu_ib_get_value(ib, i + 1); > > > > + > > > > + if (reg == PACKET0(p->adev->vcn.internal.data0, 0)) { > > > > + msg_lo = val; > > > > + } else if (reg == PACKET0(p->adev->vcn.internal.data1, 0)) { > > > > + msg_hi = val; > > > > + } else if (reg == PACKET0(p->adev->vcn.internal.cmd, 0)) { > > > > + r = vcn_v1_0_validate_bo(p, job, > > > > + ((u64)msg_hi) << 32 | msg_lo); > > > > + if (r) > > > > + return r; > > > > + } > > > > + } > > > > + > > > > + return 0; > > > > +} > > > > + > > > > static const struct amdgpu_ring_funcs vcn_v1_0_dec_ring_vm_funcs = { > > > > .type = AMDGPU_RING_TYPE_VCN_DEC, > > > > .align_mask = 0xf, > > > > @@ -1914,6 +1984,7 @@ static const struct amdgpu_ring_funcs vcn_v1_0_dec_ring_vm_funcs = { > > > > .get_rptr = vcn_v1_0_dec_ring_get_rptr, > > > > .get_wptr = vcn_v1_0_dec_ring_get_wptr, > > > > .set_wptr = vcn_v1_0_dec_ring_set_wptr, > > > > + .patch_cs_in_place = vcn_v1_0_ring_patch_cs_in_place, > > > > .emit_frame_size = > > > > 6 + 6 + /* hdp invalidate / flush */ > > > > SOC15_FLUSH_GPU_TLB_NUM_WREG * 6 + > > > > > > > > > Kind regards, > > > > > > Paul ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3] drm/amdgpu: add workarounds for VCN TMZ issue on CHIP_RAVEN 2022-03-16 6:21 ` Lang Yu @ 2022-03-16 9:41 ` Christian König 2022-03-16 9:52 ` Lang Yu 2022-03-16 9:57 ` Paul Menzel 0 siblings, 2 replies; 12+ messages in thread From: Christian König @ 2022-03-16 9:41 UTC (permalink / raw) To: Lang Yu, Paul Menzel Cc: Alex Deucher, Huang Rui, Christian König, amd-gfx Am 16.03.22 um 07:21 schrieb Lang Yu: > On 03/16/ , Paul Menzel wrote: >> Dear Lang, >> >> >> Am 16.03.22 um 02:27 schrieb Lang Yu: >>> On 03/15/ , Paul Menzel wrote: >>>> Am 14.03.22 um 03:45 schrieb Lang Yu: >>>> >>>> Thank you for your patch. A shorter commit message summary would be: >>>> >>>>> drm/amdgpu: Work around VNC TMZ issue on CHIP_RAVEN >>>>> It is a hardware issue that VCN can't handle a GTT >>>>> backing stored TMZ buffer on CHIP_RAVEN series ASIC. >>>> Where is that documented, and how can this be reproduced? >>> It is documented in AMD internal Confluence and JIRA. >>> Secure playback with a low VRAM config(thus TMZ buffer >>> will be allocted in GTT domain) may reproduce this issue. >> It’d be great if as much of the details from this non-publicly accessible >> information could be added to the commit message, and a way to reproduce >> this as there does not seem to be a test for this. (Also I guess a tag with >> a reference to the internal issue would be acceptable, so in case more >> question surface in the future.) > Thanks. I will add an internal link. Lang, please don't! This isn't an information which is expected to be made public. Regards, Christian. > > Regards, > Lang > >>>>> Move such a TMZ buffer to VRAM domain before command >>>>> submission as a wrokaround. >>>> workaround >>>> >>>> Please use a text width of 75 characters per line. >>> Thanks for your comments. I will take care about this. >>> >>>>> v2: >>>>> - Use patch_cs_in_place callback. >>>>> >>>>> v3: >>>>> - Bail out early if unsecure IBs. >>>>> >>>>> Suggested-by: Christian König <christian.koenig@amd.com> >>>>> Signed-off-by: Lang Yu <Lang.Yu@amd.com> >>>>> --- >>>>> drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c | 71 +++++++++++++++++++++++++++ >>>>> 1 file changed, 71 insertions(+) >>>>> >>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c b/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c >>>>> index 7bbb9ba6b80b..1ac9e06d3a4d 100644 >>>>> --- a/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c >>>>> +++ b/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c >>>>> @@ -24,6 +24,7 @@ >>>>> #include <linux/firmware.h> >>>>> #include "amdgpu.h" >>>>> +#include "amdgpu_cs.h" >>>>> #include "amdgpu_vcn.h" >>>>> #include "amdgpu_pm.h" >>>>> #include "soc15.h" >>>>> @@ -1905,6 +1906,75 @@ static const struct amd_ip_funcs vcn_v1_0_ip_funcs = { >>>>> .set_powergating_state = vcn_v1_0_set_powergating_state, >>>>> }; >>>>> +/* >>>>> + * It is a hardware issue that VCN can't handle a GTT TMZ buffer on >>>>> + * CHIP_RAVEN series ASIC. Move such a GTT TMZ buffer to VRAM domain >>>>> + * before command submission as a workaround. >>>>> + */ >>>>> +static int vcn_v1_0_validate_bo(struct amdgpu_cs_parser *parser, >>>>> + struct amdgpu_job *job, >>>>> + uint64_t addr) >>>>> +{ >>>>> + struct ttm_operation_ctx ctx = { false, false }; >>>>> + struct amdgpu_fpriv *fpriv = parser->filp->driver_priv; >>>>> + struct amdgpu_vm *vm = &fpriv->vm; >>>>> + struct amdgpu_bo_va_mapping *mapping; >>>>> + struct amdgpu_bo *bo; >>>>> + int r; >>>>> + >>>>> + addr &= AMDGPU_GMC_HOLE_MASK; >>>>> + if (addr & 0x7) { >>>>> + DRM_ERROR("VCN messages must be 8 byte aligned!\n"); >>>>> + return -EINVAL; >>>>> + } >>>>> + >>>>> + mapping = amdgpu_vm_bo_lookup_mapping(vm, addr/AMDGPU_GPU_PAGE_SIZE); >>>>> + if (!mapping || !mapping->bo_va || !mapping->bo_va->base.bo) >>>>> + return -EINVAL; >>>>> + >>>>> + bo = mapping->bo_va->base.bo; >>>>> + if (!(bo->flags & AMDGPU_GEM_CREATE_ENCRYPTED)) >>>>> + return 0; >>>>> + >>>>> + amdgpu_bo_placement_from_domain(bo, AMDGPU_GEM_DOMAIN_VRAM); >>>>> + r = ttm_bo_validate(&bo->tbo, &bo->placement, &ctx); >>>>> + if (r) { >>>>> + DRM_ERROR("Failed validating the VCN message BO (%d)!\n", r); >>>> to validate >>>> >>>>> + return r; >>>>> + } >>>>> + >>>>> + return r; >>>>> +} >>>>> + >>>>> +static int vcn_v1_0_ring_patch_cs_in_place(struct amdgpu_cs_parser *p, >>>>> + struct amdgpu_job *job, >>>>> + struct amdgpu_ib *ib) >>>>> +{ >>>>> + uint32_t msg_lo = 0, msg_hi = 0; >>>>> + int i, r; >>>> Use unsigned int for the counter variable? >>> You can see both signed and unsigned are used in kernel. >>> (e.g., mm/page_alloc.c). >>> >>> For this case, I think it makes no difference. >>> >>> In a word, we should consider the specific context. >> Although it makes technically no difference, I prefer to use the best >> matching type to convey the intention of the variable to the reader. But you >> are right, there is no hard rule for it. >> >> >> Kind regards, >> >> Paul >> >>>>> + >>>>> + if (!(ib->flags & AMDGPU_IB_FLAGS_SECURE)) >>>>> + return 0; >>>>> + >>>>> + for (i = 0; i < ib->length_dw; i += 2) { >>>>> + uint32_t reg = amdgpu_ib_get_value(ib, i); >>>>> + uint32_t val = amdgpu_ib_get_value(ib, i + 1); >>>>> + >>>>> + if (reg == PACKET0(p->adev->vcn.internal.data0, 0)) { >>>>> + msg_lo = val; >>>>> + } else if (reg == PACKET0(p->adev->vcn.internal.data1, 0)) { >>>>> + msg_hi = val; >>>>> + } else if (reg == PACKET0(p->adev->vcn.internal.cmd, 0)) { >>>>> + r = vcn_v1_0_validate_bo(p, job, >>>>> + ((u64)msg_hi) << 32 | msg_lo); >>>>> + if (r) >>>>> + return r; >>>>> + } >>>>> + } >>>>> + >>>>> + return 0; >>>>> +} >>>>> + >>>>> static const struct amdgpu_ring_funcs vcn_v1_0_dec_ring_vm_funcs = { >>>>> .type = AMDGPU_RING_TYPE_VCN_DEC, >>>>> .align_mask = 0xf, >>>>> @@ -1914,6 +1984,7 @@ static const struct amdgpu_ring_funcs vcn_v1_0_dec_ring_vm_funcs = { >>>>> .get_rptr = vcn_v1_0_dec_ring_get_rptr, >>>>> .get_wptr = vcn_v1_0_dec_ring_get_wptr, >>>>> .set_wptr = vcn_v1_0_dec_ring_set_wptr, >>>>> + .patch_cs_in_place = vcn_v1_0_ring_patch_cs_in_place, >>>>> .emit_frame_size = >>>>> 6 + 6 + /* hdp invalidate / flush */ >>>>> SOC15_FLUSH_GPU_TLB_NUM_WREG * 6 + >>>> >>>> Kind regards, >>>> >>>> Paul ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3] drm/amdgpu: add workarounds for VCN TMZ issue on CHIP_RAVEN 2022-03-16 9:41 ` Christian König @ 2022-03-16 9:52 ` Lang Yu 2022-03-16 9:57 ` Paul Menzel 1 sibling, 0 replies; 12+ messages in thread From: Lang Yu @ 2022-03-16 9:52 UTC (permalink / raw) To: Christian König Cc: Alex Deucher, Paul Menzel, Huang Rui, Christian König, amd-gfx On 03/16/ , Christian König wrote: > Am 16.03.22 um 07:21 schrieb Lang Yu: > > On 03/16/ , Paul Menzel wrote: > > > Dear Lang, > > > > > > > > > Am 16.03.22 um 02:27 schrieb Lang Yu: > > > > On 03/15/ , Paul Menzel wrote: > > > > > Am 14.03.22 um 03:45 schrieb Lang Yu: > > > > > > > > > > Thank you for your patch. A shorter commit message summary would be: > > > > > > > > > > > drm/amdgpu: Work around VNC TMZ issue on CHIP_RAVEN > > > > > > It is a hardware issue that VCN can't handle a GTT > > > > > > backing stored TMZ buffer on CHIP_RAVEN series ASIC. > > > > > Where is that documented, and how can this be reproduced? > > > > It is documented in AMD internal Confluence and JIRA. > > > > Secure playback with a low VRAM config(thus TMZ buffer > > > > will be allocted in GTT domain) may reproduce this issue. > > > It’d be great if as much of the details from this non-publicly accessible > > > information could be added to the commit message, and a way to reproduce > > > this as there does not seem to be a test for this. (Also I guess a tag with > > > a reference to the internal issue would be acceptable, so in case more > > > question surface in the future.) > > Thanks. I will add an internal link. > > Lang, please don't! > > This isn't an information which is expected to be made public. Okay, got it. Regards, Lang > Regards, > Christian. > > > > > Regards, > > Lang > > > > > > > > Move such a TMZ buffer to VRAM domain before command > > > > > > submission as a wrokaround. > > > > > workaround > > > > > > > > > > Please use a text width of 75 characters per line. > > > > Thanks for your comments. I will take care about this. > > > > > > > > > > v2: > > > > > > - Use patch_cs_in_place callback. > > > > > > > > > > > > v3: > > > > > > - Bail out early if unsecure IBs. > > > > > > > > > > > > Suggested-by: Christian König <christian.koenig@amd.com> > > > > > > Signed-off-by: Lang Yu <Lang.Yu@amd.com> > > > > > > --- > > > > > > drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c | 71 +++++++++++++++++++++++++++ > > > > > > 1 file changed, 71 insertions(+) > > > > > > > > > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c b/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c > > > > > > index 7bbb9ba6b80b..1ac9e06d3a4d 100644 > > > > > > --- a/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c > > > > > > +++ b/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c > > > > > > @@ -24,6 +24,7 @@ > > > > > > #include <linux/firmware.h> > > > > > > #include "amdgpu.h" > > > > > > +#include "amdgpu_cs.h" > > > > > > #include "amdgpu_vcn.h" > > > > > > #include "amdgpu_pm.h" > > > > > > #include "soc15.h" > > > > > > @@ -1905,6 +1906,75 @@ static const struct amd_ip_funcs vcn_v1_0_ip_funcs = { > > > > > > .set_powergating_state = vcn_v1_0_set_powergating_state, > > > > > > }; > > > > > > +/* > > > > > > + * It is a hardware issue that VCN can't handle a GTT TMZ buffer on > > > > > > + * CHIP_RAVEN series ASIC. Move such a GTT TMZ buffer to VRAM domain > > > > > > + * before command submission as a workaround. > > > > > > + */ > > > > > > +static int vcn_v1_0_validate_bo(struct amdgpu_cs_parser *parser, > > > > > > + struct amdgpu_job *job, > > > > > > + uint64_t addr) > > > > > > +{ > > > > > > + struct ttm_operation_ctx ctx = { false, false }; > > > > > > + struct amdgpu_fpriv *fpriv = parser->filp->driver_priv; > > > > > > + struct amdgpu_vm *vm = &fpriv->vm; > > > > > > + struct amdgpu_bo_va_mapping *mapping; > > > > > > + struct amdgpu_bo *bo; > > > > > > + int r; > > > > > > + > > > > > > + addr &= AMDGPU_GMC_HOLE_MASK; > > > > > > + if (addr & 0x7) { > > > > > > + DRM_ERROR("VCN messages must be 8 byte aligned!\n"); > > > > > > + return -EINVAL; > > > > > > + } > > > > > > + > > > > > > + mapping = amdgpu_vm_bo_lookup_mapping(vm, addr/AMDGPU_GPU_PAGE_SIZE); > > > > > > + if (!mapping || !mapping->bo_va || !mapping->bo_va->base.bo) > > > > > > + return -EINVAL; > > > > > > + > > > > > > + bo = mapping->bo_va->base.bo; > > > > > > + if (!(bo->flags & AMDGPU_GEM_CREATE_ENCRYPTED)) > > > > > > + return 0; > > > > > > + > > > > > > + amdgpu_bo_placement_from_domain(bo, AMDGPU_GEM_DOMAIN_VRAM); > > > > > > + r = ttm_bo_validate(&bo->tbo, &bo->placement, &ctx); > > > > > > + if (r) { > > > > > > + DRM_ERROR("Failed validating the VCN message BO (%d)!\n", r); > > > > > to validate > > > > > > > > > > > + return r; > > > > > > + } > > > > > > + > > > > > > + return r; > > > > > > +} > > > > > > + > > > > > > +static int vcn_v1_0_ring_patch_cs_in_place(struct amdgpu_cs_parser *p, > > > > > > + struct amdgpu_job *job, > > > > > > + struct amdgpu_ib *ib) > > > > > > +{ > > > > > > + uint32_t msg_lo = 0, msg_hi = 0; > > > > > > + int i, r; > > > > > Use unsigned int for the counter variable? > > > > You can see both signed and unsigned are used in kernel. > > > > (e.g., mm/page_alloc.c). > > > > > > > > For this case, I think it makes no difference. > > > > > > > > In a word, we should consider the specific context. > > > Although it makes technically no difference, I prefer to use the best > > > matching type to convey the intention of the variable to the reader. But you > > > are right, there is no hard rule for it. > > > > > > > > > Kind regards, > > > > > > Paul > > > > > > > > > + > > > > > > + if (!(ib->flags & AMDGPU_IB_FLAGS_SECURE)) > > > > > > + return 0; > > > > > > + > > > > > > + for (i = 0; i < ib->length_dw; i += 2) { > > > > > > + uint32_t reg = amdgpu_ib_get_value(ib, i); > > > > > > + uint32_t val = amdgpu_ib_get_value(ib, i + 1); > > > > > > + > > > > > > + if (reg == PACKET0(p->adev->vcn.internal.data0, 0)) { > > > > > > + msg_lo = val; > > > > > > + } else if (reg == PACKET0(p->adev->vcn.internal.data1, 0)) { > > > > > > + msg_hi = val; > > > > > > + } else if (reg == PACKET0(p->adev->vcn.internal.cmd, 0)) { > > > > > > + r = vcn_v1_0_validate_bo(p, job, > > > > > > + ((u64)msg_hi) << 32 | msg_lo); > > > > > > + if (r) > > > > > > + return r; > > > > > > + } > > > > > > + } > > > > > > + > > > > > > + return 0; > > > > > > +} > > > > > > + > > > > > > static const struct amdgpu_ring_funcs vcn_v1_0_dec_ring_vm_funcs = { > > > > > > .type = AMDGPU_RING_TYPE_VCN_DEC, > > > > > > .align_mask = 0xf, > > > > > > @@ -1914,6 +1984,7 @@ static const struct amdgpu_ring_funcs vcn_v1_0_dec_ring_vm_funcs = { > > > > > > .get_rptr = vcn_v1_0_dec_ring_get_rptr, > > > > > > .get_wptr = vcn_v1_0_dec_ring_get_wptr, > > > > > > .set_wptr = vcn_v1_0_dec_ring_set_wptr, > > > > > > + .patch_cs_in_place = vcn_v1_0_ring_patch_cs_in_place, > > > > > > .emit_frame_size = > > > > > > 6 + 6 + /* hdp invalidate / flush */ > > > > > > SOC15_FLUSH_GPU_TLB_NUM_WREG * 6 + > > > > > > > > > > Kind regards, > > > > > > > > > > Paul > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3] drm/amdgpu: add workarounds for VCN TMZ issue on CHIP_RAVEN 2022-03-16 9:41 ` Christian König 2022-03-16 9:52 ` Lang Yu @ 2022-03-16 9:57 ` Paul Menzel 2022-03-16 10:08 ` Christian König 1 sibling, 1 reply; 12+ messages in thread From: Paul Menzel @ 2022-03-16 9:57 UTC (permalink / raw) To: Christian König Cc: Alex Deucher, Huang Rui, Lang Yu, Christian König, amd-gfx Dear Christian, Am 16.03.22 um 10:41 schrieb Christian König: > Am 16.03.22 um 07:21 schrieb Lang Yu: >> On 03/16/ , Paul Menzel wrote: >>> Am 16.03.22 um 02:27 schrieb Lang Yu: >>>> On 03/15/ , Paul Menzel wrote: >>>>> Am 14.03.22 um 03:45 schrieb Lang Yu: >>>>> >>>>> Thank you for your patch. A shorter commit message summary would be: >>>>> >>>>>> drm/amdgpu: Work around VNC TMZ issue on CHIP_RAVEN >>>>>> It is a hardware issue that VCN can't handle a GTT >>>>>> backing stored TMZ buffer on CHIP_RAVEN series ASIC. >>>>> Where is that documented, and how can this be reproduced? >>>> It is documented in AMD internal Confluence and JIRA. >>>> Secure playback with a low VRAM config(thus TMZ buffer >>>> will be allocted in GTT domain) may reproduce this issue. >>> It’d be great if as much of the details from this non-publicly >>> accessible >>> information could be added to the commit message, and a way to reproduce >>> this as there does not seem to be a test for this. (Also I guess a >>> tag with >>> a reference to the internal issue would be acceptable, so in case more >>> question surface in the future.) >> Thanks. I will add an internal link. > > Lang, please don't! > > This isn't an information which is expected to be made public. Well, how are then even the AMD folks able to link a (upstream) commit to an issue? If it’s not possible, even more detailed information about the issue including how to reproduce it needs to be part of the commit message. Kind regards, Paul PS: The coreboot project includes references to Google’s internal bug/issue tracker, but also has the requirement that the commit can be reviewed and tested without access to it. >>>>>> Move such a TMZ buffer to VRAM domain before command >>>>>> submission as a wrokaround. >>>>> workaround >>>>> >>>>> Please use a text width of 75 characters per line. >>>> Thanks for your comments. I will take care about this. >>>> >>>>>> v2: >>>>>> - Use patch_cs_in_place callback. >>>>>> >>>>>> v3: >>>>>> - Bail out early if unsecure IBs. >>>>>> >>>>>> Suggested-by: Christian König <christian.koenig@amd.com> >>>>>> Signed-off-by: Lang Yu <Lang.Yu@amd.com> >>>>>> --- >>>>>> drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c | 71 +++++++++++++++++++++++++++ >>>>>> 1 file changed, 71 insertions(+) >>>>>> >>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c >>>>>> b/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c >>>>>> index 7bbb9ba6b80b..1ac9e06d3a4d 100644 >>>>>> --- a/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c >>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c >>>>>> @@ -24,6 +24,7 @@ >>>>>> #include <linux/firmware.h> >>>>>> #include "amdgpu.h" >>>>>> +#include "amdgpu_cs.h" >>>>>> #include "amdgpu_vcn.h" >>>>>> #include "amdgpu_pm.h" >>>>>> #include "soc15.h" >>>>>> @@ -1905,6 +1906,75 @@ static const struct amd_ip_funcs >>>>>> vcn_v1_0_ip_funcs = { >>>>>> .set_powergating_state = vcn_v1_0_set_powergating_state, >>>>>> }; >>>>>> +/* >>>>>> + * It is a hardware issue that VCN can't handle a GTT TMZ buffer on >>>>>> + * CHIP_RAVEN series ASIC. Move such a GTT TMZ buffer to VRAM domain >>>>>> + * before command submission as a workaround. >>>>>> + */ >>>>>> +static int vcn_v1_0_validate_bo(struct amdgpu_cs_parser *parser, >>>>>> + struct amdgpu_job *job, >>>>>> + uint64_t addr) >>>>>> +{ >>>>>> + struct ttm_operation_ctx ctx = { false, false }; >>>>>> + struct amdgpu_fpriv *fpriv = parser->filp->driver_priv; >>>>>> + struct amdgpu_vm *vm = &fpriv->vm; >>>>>> + struct amdgpu_bo_va_mapping *mapping; >>>>>> + struct amdgpu_bo *bo; >>>>>> + int r; >>>>>> + >>>>>> + addr &= AMDGPU_GMC_HOLE_MASK; >>>>>> + if (addr & 0x7) { >>>>>> + DRM_ERROR("VCN messages must be 8 byte aligned!\n"); >>>>>> + return -EINVAL; >>>>>> + } >>>>>> + >>>>>> + mapping = amdgpu_vm_bo_lookup_mapping(vm, addr/AMDGPU_GPU_PAGE_SIZE); >>>>>> + if (!mapping || !mapping->bo_va || !mapping->bo_va->base.bo) >>>>>> + return -EINVAL; >>>>>> + >>>>>> + bo = mapping->bo_va->base.bo; >>>>>> + if (!(bo->flags & AMDGPU_GEM_CREATE_ENCRYPTED)) >>>>>> + return 0; >>>>>> + >>>>>> + amdgpu_bo_placement_from_domain(bo, AMDGPU_GEM_DOMAIN_VRAM); >>>>>> + r = ttm_bo_validate(&bo->tbo, &bo->placement, &ctx); >>>>>> + if (r) { >>>>>> + DRM_ERROR("Failed validating the VCN message BO (%d)!\n", >>>>>> r); >>>>> to validate >>>>> >>>>>> + return r; >>>>>> + } >>>>>> + >>>>>> + return r; >>>>>> +} >>>>>> + >>>>>> +static int vcn_v1_0_ring_patch_cs_in_place(struct >>>>>> amdgpu_cs_parser *p, >>>>>> + struct amdgpu_job *job, >>>>>> + struct amdgpu_ib *ib) >>>>>> +{ >>>>>> + uint32_t msg_lo = 0, msg_hi = 0; >>>>>> + int i, r; >>>>> Use unsigned int for the counter variable? >>>> You can see both signed and unsigned are used in kernel. >>>> (e.g., mm/page_alloc.c). >>>> >>>> For this case, I think it makes no difference. >>>> >>>> In a word, we should consider the specific context. >>> Although it makes technically no difference, I prefer to use the best >>> matching type to convey the intention of the variable to the reader. >>> But you >>> are right, there is no hard rule for it. >>> >>> >>> Kind regards, >>> >>> Paul >>> >>>>>> + >>>>>> + if (!(ib->flags & AMDGPU_IB_FLAGS_SECURE)) >>>>>> + return 0; >>>>>> + >>>>>> + for (i = 0; i < ib->length_dw; i += 2) { >>>>>> + uint32_t reg = amdgpu_ib_get_value(ib, i); >>>>>> + uint32_t val = amdgpu_ib_get_value(ib, i + 1); >>>>>> + >>>>>> + if (reg == PACKET0(p->adev->vcn.internal.data0, 0)) { >>>>>> + msg_lo = val; >>>>>> + } else if (reg == PACKET0(p->adev->vcn.internal.data1, 0)) { >>>>>> + msg_hi = val; >>>>>> + } else if (reg == PACKET0(p->adev->vcn.internal.cmd, 0)) { >>>>>> + r = vcn_v1_0_validate_bo(p, job, >>>>>> + ((u64)msg_hi) << 32 | msg_lo); >>>>>> + if (r) >>>>>> + return r; >>>>>> + } >>>>>> + } >>>>>> + >>>>>> + return 0; >>>>>> +} >>>>>> + >>>>>> static const struct amdgpu_ring_funcs >>>>>> vcn_v1_0_dec_ring_vm_funcs = { >>>>>> .type = AMDGPU_RING_TYPE_VCN_DEC, >>>>>> .align_mask = 0xf, >>>>>> @@ -1914,6 +1984,7 @@ static const struct amdgpu_ring_funcs >>>>>> vcn_v1_0_dec_ring_vm_funcs = { >>>>>> .get_rptr = vcn_v1_0_dec_ring_get_rptr, >>>>>> .get_wptr = vcn_v1_0_dec_ring_get_wptr, >>>>>> .set_wptr = vcn_v1_0_dec_ring_set_wptr, >>>>>> + .patch_cs_in_place = vcn_v1_0_ring_patch_cs_in_place, >>>>>> .emit_frame_size = >>>>>> 6 + 6 + /* hdp invalidate / flush */ >>>>>> SOC15_FLUSH_GPU_TLB_NUM_WREG * 6 + >>>>> >>>>> Kind regards, >>>>> >>>>> Paul > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3] drm/amdgpu: add workarounds for VCN TMZ issue on CHIP_RAVEN 2022-03-16 9:57 ` Paul Menzel @ 2022-03-16 10:08 ` Christian König 2022-03-18 7:04 ` Paul Menzel 0 siblings, 1 reply; 12+ messages in thread From: Christian König @ 2022-03-16 10:08 UTC (permalink / raw) To: Paul Menzel Cc: Alex Deucher, Huang Rui, Lang Yu, Christian König, amd-gfx Am 16.03.22 um 10:57 schrieb Paul Menzel: > Dear Christian, > > > Am 16.03.22 um 10:41 schrieb Christian König: >> Am 16.03.22 um 07:21 schrieb Lang Yu: >>> On 03/16/ , Paul Menzel wrote: > >>>> Am 16.03.22 um 02:27 schrieb Lang Yu: >>>>> On 03/15/ , Paul Menzel wrote: >>>>>> Am 14.03.22 um 03:45 schrieb Lang Yu: >>>>>> >>>>>> Thank you for your patch. A shorter commit message summary would be: >>>>>> >>>>>>> drm/amdgpu: Work around VNC TMZ issue on CHIP_RAVEN >>>>>>> It is a hardware issue that VCN can't handle a GTT >>>>>>> backing stored TMZ buffer on CHIP_RAVEN series ASIC. >>>>>> Where is that documented, and how can this be reproduced? >>>>> It is documented in AMD internal Confluence and JIRA. >>>>> Secure playback with a low VRAM config(thus TMZ buffer >>>>> will be allocted in GTT domain) may reproduce this issue. >>>> It’d be great if as much of the details from this non-publicly >>>> accessible >>>> information could be added to the commit message, and a way to >>>> reproduce >>>> this as there does not seem to be a test for this. (Also I guess a >>>> tag with >>>> a reference to the internal issue would be acceptable, so in case more >>>> question surface in the future.) >>> Thanks. I will add an internal link. >> >> Lang, please don't! >> >> This isn't an information which is expected to be made public. > > Well, how are then even the AMD folks able to link a (upstream) commit > to an issue? Well quite simply: We don't do that since it isn't necessary. > > If it’s not possible, even more detailed information about the issue > including how to reproduce it needs to be part of the commit message. No, why should we do that? It's an AMD internal hardware problem which we add a software workaround for here. The hardware details why and what are completely irrelevant to the public. All that we need to document is that VCN can't handle GTT on Raven, and that's exactly what the commit message is doing. That's perfectly enough to write a test case. Regards, Christian. > > > Kind regards, > > Paul > > > PS: The coreboot project includes references to Google’s internal > bug/issue tracker, but also has the requirement that the commit can be > reviewed and tested without access to it. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3] drm/amdgpu: add workarounds for VCN TMZ issue on CHIP_RAVEN 2022-03-16 10:08 ` Christian König @ 2022-03-18 7:04 ` Paul Menzel 0 siblings, 0 replies; 12+ messages in thread From: Paul Menzel @ 2022-03-18 7:04 UTC (permalink / raw) To: Christian König Cc: Alex Deucher, Huang Rui, Lang Yu, Christian König, amd-gfx Dear Christian, Am 16.03.22 um 11:08 schrieb Christian König: > Am 16.03.22 um 10:57 schrieb Paul Menzel: >> Am 16.03.22 um 10:41 schrieb Christian König: >>> Am 16.03.22 um 07:21 schrieb Lang Yu: >>>> On 03/16/ , Paul Menzel wrote: >> >>>>> Am 16.03.22 um 02:27 schrieb Lang Yu: >>>>>> On 03/15/ , Paul Menzel wrote: >>>>>>> Am 14.03.22 um 03:45 schrieb Lang Yu: >>>>>>> >>>>>>> Thank you for your patch. A shorter commit message summary would be: >>>>>>> >>>>>>>> drm/amdgpu: Work around VNC TMZ issue on CHIP_RAVEN >>>>>>>> It is a hardware issue that VCN can't handle a GTT >>>>>>>> backing stored TMZ buffer on CHIP_RAVEN series ASIC. >>>>>>> Where is that documented, and how can this be reproduced? >>>>>> It is documented in AMD internal Confluence and JIRA. >>>>>> Secure playback with a low VRAM config(thus TMZ buffer >>>>>> will be allocted in GTT domain) may reproduce this issue. >>>>> It’d be great if as much of the details from this non-publicly accessible >>>>> information could be added to the commit message, and a way to reproduce >>>>> this as there does not seem to be a test for this. (Also I guess a tag with >>>>> a reference to the internal issue would be acceptable, so in case more >>>>> question surface in the future.) >>>> Thanks. I will add an internal link. >>> >>> Lang, please don't! >>> >>> This isn't an information which is expected to be made public. >> >> Well, how are then even the AMD folks able to link a (upstream) commit >> to an issue? > > Well quite simply: We don't do that since it isn't necessary. What other ways do you (or future AMD developers) have then? (I would also use *helpful* or *useful*.) (In two years, when maybe nobody of the current AMD developers work at AMD anymore, and a user bisects a problems to this patch I could imagine it would help the future AMD developers to have this connection.) >> If it’s not possible, even more detailed information about the issue >> including how to reproduce it needs to be part of the commit message. > > No, why should we do that? It's an AMD internal hardware problem which > we add a software workaround for here. The hardware details why and what > are completely irrelevant to the public. > > All that we need to document is that VCN can't handle GTT on Raven, and > that's exactly what the commit message is doing. That's perfectly enough > to write a test case. Thank you for clarifying, but I am not interested in the hardware details, but how to reproduce and test the issue. And according to Lang this information is present in the issue. Seeing how complex the graphics driver are, a lot of documentation is not publicly available, a recipe to manually reproduce and test the issue is most helpful. Kind regards, Paul ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2022-03-18 7:05 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-03-14 2:45 [PATCH v3] drm/amdgpu: add workarounds for VCN TMZ issue on CHIP_RAVEN Lang Yu 2022-03-14 7:51 ` Christian König 2022-03-15 14:28 ` Lang Yu 2022-03-15 16:09 ` Paul Menzel 2022-03-16 1:27 ` Lang Yu 2022-03-16 5:25 ` Paul Menzel 2022-03-16 6:21 ` Lang Yu 2022-03-16 9:41 ` Christian König 2022-03-16 9:52 ` Lang Yu 2022-03-16 9:57 ` Paul Menzel 2022-03-16 10:08 ` Christian König 2022-03-18 7:04 ` Paul Menzel
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.