[PATCH] drm/amdgpu: Move to a per-IB secure flag (TMZ)
Luben Tuikov
luben.tuikov at amd.com
Thu Feb 13 22:00:36 UTC 2020
On 2020-02-13 05:30, Christian König wrote:
> Am 13.02.20 um 01:54 schrieb Luben Tuikov:
>> Move from a per-CS secure flag (TMZ) to a per-IB
>> secure flag.
>
> Well that seems to make a lot of sense to me, but need to bit of time to
> read into the PM4 handling of TMZ.
Note that this patch is _not_ adding new functionality,
or anything new. It is not adding new PM3 or even PM4
functionality. Nothing new here.
It only moves the hierarchy from CS to IB of the
secure flag. That's all.
I think it is safe to submit it now, as it would alleviate
libdrm secure buffer submission.
>
> Especially what is the "start" parameter good for?
Note that this is existing code. It must've been reviewed
by someone when it was submitted and git-blame is good
with that.
The "start" parameter controls whether the packet
switches the ring to TMZ or non-TMZ processing.
Regards,
Luben
>
> Regards,
> Christian.
>
>>
>> Signed-off-by: Luben Tuikov <luben.tuikov at amd.com>
>> ---
>> drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 2 --
>> drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c | 23 ++++++++++++++++++++---
>> drivers/gpu/drm/amd/amdgpu/amdgpu_job.h | 3 ---
>> drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h | 9 ++++-----
>> drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c | 23 +++++++----------------
>> drivers/gpu/drm/amd/amdgpu/gfx_v6_0.c | 3 +--
>> drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c | 3 +--
>> drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c | 3 +--
>> drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c | 20 ++++++--------------
>> include/uapi/drm/amdgpu_drm.h | 7 ++++---
>> 10 files changed, 44 insertions(+), 52 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>> index 80ba6dfc54e2..f9fa6e104fef 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>> @@ -231,8 +231,6 @@ static int amdgpu_cs_parser_init(struct amdgpu_cs_parser *p, union drm_amdgpu_cs
>> if (ret)
>> goto free_all_kdata;
>>
>> - p->job->secure = cs->in.flags & AMDGPU_CS_FLAGS_SECURE;
>> -
>> if (p->ctx->vram_lost_counter != p->job->vram_lost_counter) {
>> ret = -ECANCELED;
>> goto free_all_kdata;
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
>> index 6e0f97afb030..cae81914c821 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
>> @@ -132,6 +132,7 @@ int amdgpu_ib_schedule(struct amdgpu_ring *ring, unsigned num_ibs,
>> uint64_t fence_ctx;
>> uint32_t status = 0, alloc_size;
>> unsigned fence_flags = 0;
>> + bool secure;
>>
>> unsigned i;
>> int r = 0;
>> @@ -213,9 +214,10 @@ int amdgpu_ib_schedule(struct amdgpu_ring *ring, unsigned num_ibs,
>> if (job && ring->funcs->emit_cntxcntl) {
>> status |= job->preamble_status;
>> status |= job->preemption_status;
>> - amdgpu_ring_emit_cntxcntl(ring, status, job->secure);
>> + amdgpu_ring_emit_cntxcntl(ring, status);
>> }
>>
>> + secure = false;
>> for (i = 0; i < num_ibs; ++i) {
>> ib = &ibs[i];
>>
>> @@ -227,12 +229,27 @@ int amdgpu_ib_schedule(struct amdgpu_ring *ring, unsigned num_ibs,
>> !amdgpu_sriov_vf(adev)) /* for SRIOV preemption, Preamble CE ib must be inserted anyway */
>> continue;
>>
>> + /* If this IB is TMZ, add frame TMZ start packet,
>> + * else, turn off TMZ.
>> + */
>> + if (ib->flags & AMDGPU_IB_FLAGS_SECURE && ring->funcs->emit_tmz) {
>> + if (!secure) {
>> + secure = true;
>> + amdgpu_ring_emit_tmz(ring, true);
>> + }
>> + } else if (secure) {
>> + secure = false;
>> + amdgpu_ring_emit_tmz(ring, false);
>> + }
>> +
>> amdgpu_ring_emit_ib(ring, job, ib, status);
>> status &= ~AMDGPU_HAVE_CTX_SWITCH;
>> }
>>
>> - if (ring->funcs->emit_tmz)
>> - amdgpu_ring_emit_tmz(ring, false, job ? job->secure : false);
>> + if (secure) {
>> + secure = false;
>> + amdgpu_ring_emit_tmz(ring, false);
>> + }
>>
>> #ifdef CONFIG_X86_64
>> if (!(adev->flags & AMD_IS_APU))
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.h
>> index 31c4444b0203..2e2110dddb76 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.h
>> @@ -61,9 +61,6 @@ struct amdgpu_job {
>> /* user fence handling */
>> uint64_t uf_addr;
>> uint64_t uf_sequence;
>> -
>> - /* the job is due to a secure command submission */
>> - bool secure;
>> };
>>
>> int amdgpu_job_alloc(struct amdgpu_device *adev, unsigned num_ibs,
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
>> index 5134d0dd6dc2..930316e60155 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
>> @@ -158,8 +158,7 @@ struct amdgpu_ring_funcs {
>> void (*begin_use)(struct amdgpu_ring *ring);
>> void (*end_use)(struct amdgpu_ring *ring);
>> void (*emit_switch_buffer) (struct amdgpu_ring *ring);
>> - void (*emit_cntxcntl) (struct amdgpu_ring *ring, uint32_t flags,
>> - bool trusted);
>> + void (*emit_cntxcntl) (struct amdgpu_ring *ring, uint32_t flags);
>> void (*emit_rreg)(struct amdgpu_ring *ring, uint32_t reg);
>> void (*emit_wreg)(struct amdgpu_ring *ring, uint32_t reg, uint32_t val);
>> void (*emit_reg_wait)(struct amdgpu_ring *ring, uint32_t reg,
>> @@ -167,7 +166,7 @@ struct amdgpu_ring_funcs {
>> void (*emit_reg_write_reg_wait)(struct amdgpu_ring *ring,
>> uint32_t reg0, uint32_t reg1,
>> uint32_t ref, uint32_t mask);
>> - void (*emit_tmz)(struct amdgpu_ring *ring, bool start, bool trusted);
>> + void (*emit_tmz)(struct amdgpu_ring *ring, bool start);
>> /* priority functions */
>> void (*set_priority) (struct amdgpu_ring *ring,
>> enum drm_sched_priority priority);
>> @@ -243,12 +242,12 @@ struct amdgpu_ring {
>> #define amdgpu_ring_emit_gds_switch(r, v, db, ds, wb, ws, ab, as) (r)->funcs->emit_gds_switch((r), (v), (db), (ds), (wb), (ws), (ab), (as))
>> #define amdgpu_ring_emit_hdp_flush(r) (r)->funcs->emit_hdp_flush((r))
>> #define amdgpu_ring_emit_switch_buffer(r) (r)->funcs->emit_switch_buffer((r))
>> -#define amdgpu_ring_emit_cntxcntl(r, d, s) (r)->funcs->emit_cntxcntl((r), (d), (s))
>> +#define amdgpu_ring_emit_cntxcntl(r, d) (r)->funcs->emit_cntxcntl((r), (d))
>> #define amdgpu_ring_emit_rreg(r, d) (r)->funcs->emit_rreg((r), (d))
>> #define amdgpu_ring_emit_wreg(r, d, v) (r)->funcs->emit_wreg((r), (d), (v))
>> #define amdgpu_ring_emit_reg_wait(r, d, v, m) (r)->funcs->emit_reg_wait((r), (d), (v), (m))
>> #define amdgpu_ring_emit_reg_write_reg_wait(r, d0, d1, v, m) (r)->funcs->emit_reg_write_reg_wait((r), (d0), (d1), (v), (m))
>> -#define amdgpu_ring_emit_tmz(r, b, s) (r)->funcs->emit_tmz((r), (b), (s))
>> +#define amdgpu_ring_emit_tmz(r, b) (r)->funcs->emit_tmz((r), (b))
>> #define amdgpu_ring_pad_ib(r, ib) ((r)->funcs->pad_ib((r), (ib)))
>> #define amdgpu_ring_init_cond_exec(r) (r)->funcs->init_cond_exec((r))
>> #define amdgpu_ring_patch_cond_exec(r,o) (r)->funcs->patch_cond_exec((r),(o))
>> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
>> index 4e25b39ac14f..1a9787a0177e 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
>> @@ -256,8 +256,7 @@ static int gfx_v10_0_rlc_backdoor_autoload_enable(struct amdgpu_device *adev);
>> static int gfx_v10_0_wait_for_rlc_autoload_complete(struct amdgpu_device *adev);
>> static void gfx_v10_0_ring_emit_ce_meta(struct amdgpu_ring *ring, bool resume);
>> static void gfx_v10_0_ring_emit_de_meta(struct amdgpu_ring *ring, bool resume);
>> -static void gfx_v10_0_ring_emit_tmz(struct amdgpu_ring *ring, bool start,
>> - bool trusted);
>> +static void gfx_v10_0_ring_emit_tmz(struct amdgpu_ring *ring, bool start);
>>
>> static void gfx10_kiq_set_resources(struct amdgpu_ring *kiq_ring, uint64_t queue_mask)
>> {
>> @@ -4564,8 +4563,7 @@ static void gfx_v10_0_ring_emit_sb(struct amdgpu_ring *ring)
>> }
>>
>> static void gfx_v10_0_ring_emit_cntxcntl(struct amdgpu_ring *ring,
>> - uint32_t flags,
>> - bool trusted)
>> + uint32_t flags)
>> {
>> uint32_t dw2 = 0;
>>
>> @@ -4573,8 +4571,6 @@ static void gfx_v10_0_ring_emit_cntxcntl(struct amdgpu_ring *ring,
>> gfx_v10_0_ring_emit_ce_meta(ring,
>> flags & AMDGPU_IB_PREEMPTED ? true : false);
>>
>> - gfx_v10_0_ring_emit_tmz(ring, true, trusted);
>> -
>> dw2 |= 0x80000000; /* set load_enable otherwise this package is just NOPs */
>> if (flags & AMDGPU_HAVE_CTX_SWITCH) {
>> /* set load_global_config & load_global_uconfig */
>> @@ -4731,17 +4727,12 @@ static void gfx_v10_0_ring_emit_de_meta(struct amdgpu_ring *ring, bool resume)
>> sizeof(de_payload) >> 2);
>> }
>>
>> -static void gfx_v10_0_ring_emit_tmz(struct amdgpu_ring *ring, bool start,
>> - bool trusted)
>> +static void gfx_v10_0_ring_emit_tmz(struct amdgpu_ring *ring, bool start)
>> {
>> - amdgpu_ring_write(ring, PACKET3(PACKET3_FRAME_CONTROL, 0));
>> - /*
>> - * cmd = 0: frame begin
>> - * cmd = 1: frame end
>> - */
>> - amdgpu_ring_write(ring,
>> - ((amdgpu_is_tmz(ring->adev) && trusted) ? FRAME_TMZ : 0)
>> - | FRAME_CMD(start ? 0 : 1));
>> + if (amdgpu_is_tmz(ring->adev)) {
>> + amdgpu_ring_write(ring, PACKET3(PACKET3_FRAME_CONTROL, 0));
>> + amdgpu_ring_write(ring, FRAME_TMZ | FRAME_CMD(start ? 0 : 1));
>> + }
>> }
>>
>> static void gfx_v10_0_ring_emit_rreg(struct amdgpu_ring *ring, uint32_t reg)
>> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v6_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v6_0.c
>> index 95bb2422b27c..31f44d05e606 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v6_0.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v6_0.c
>> @@ -2973,8 +2973,7 @@ static uint64_t gfx_v6_0_get_gpu_clock_counter(struct amdgpu_device *adev)
>> return clock;
>> }
>>
>> -static void gfx_v6_ring_emit_cntxcntl(struct amdgpu_ring *ring, uint32_t flags,
>> - bool trusted)
>> +static void gfx_v6_ring_emit_cntxcntl(struct amdgpu_ring *ring, uint32_t flags)
>> {
>> if (flags & AMDGPU_HAVE_CTX_SWITCH)
>> gfx_v6_0_ring_emit_vgt_flush(ring);
>> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
>> index 648d767d14e7..8f20a5dd44fe 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
>> @@ -2320,8 +2320,7 @@ static void gfx_v7_0_ring_emit_ib_compute(struct amdgpu_ring *ring,
>> amdgpu_ring_write(ring, control);
>> }
>>
>> -static void gfx_v7_ring_emit_cntxcntl(struct amdgpu_ring *ring, uint32_t flags,
>> - bool trusted)
>> +static void gfx_v7_ring_emit_cntxcntl(struct amdgpu_ring *ring, uint32_t flags)
>> {
>> uint32_t dw2 = 0;
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
>> index a46ec1c9846e..fa245973de12 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
>> @@ -6386,8 +6386,7 @@ static void gfx_v8_ring_emit_sb(struct amdgpu_ring *ring)
>> amdgpu_ring_write(ring, 0);
>> }
>>
>> -static void gfx_v8_ring_emit_cntxcntl(struct amdgpu_ring *ring, uint32_t flags,
>> - bool trusted)
>> +static void gfx_v8_ring_emit_cntxcntl(struct amdgpu_ring *ring, uint32_t flags)
>> {
>> uint32_t dw2 = 0;
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
>> index e5753746cbf9..9d3805488832 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
>> @@ -5212,29 +5212,21 @@ static void gfx_v9_0_ring_emit_de_meta(struct amdgpu_ring *ring)
>> amdgpu_ring_write_multiple(ring, (void *)&de_payload, sizeof(de_payload) >> 2);
>> }
>>
>> -static void gfx_v9_0_ring_emit_tmz(struct amdgpu_ring *ring, bool start,
>> - bool trusted)
>> +static void gfx_v9_0_ring_emit_tmz(struct amdgpu_ring *ring, bool start)
>> {
>> - amdgpu_ring_write(ring, PACKET3(PACKET3_FRAME_CONTROL, 0));
>> - /*
>> - * cmd = 0: frame begin
>> - * cmd = 1: frame end
>> - */
>> - amdgpu_ring_write(ring,
>> - ((amdgpu_is_tmz(ring->adev) && trusted) ? FRAME_TMZ : 0)
>> - | FRAME_CMD(start ? 0 : 1));
>> + if (amdgpu_is_tmz(ring->adev)) {
>> + amdgpu_ring_write(ring, PACKET3(PACKET3_FRAME_CONTROL, 0));
>> + amdgpu_ring_write(ring, FRAME_TMZ | FRAME_CMD(start ? 0 : 1));
>> + }
>> }
>>
>> -static void gfx_v9_ring_emit_cntxcntl(struct amdgpu_ring *ring, uint32_t flags,
>> - bool trusted)
>> +static void gfx_v9_ring_emit_cntxcntl(struct amdgpu_ring *ring, uint32_t flags)
>> {
>> uint32_t dw2 = 0;
>>
>> if (amdgpu_sriov_vf(ring->adev))
>> gfx_v9_0_ring_emit_ce_meta(ring);
>>
>> - gfx_v9_0_ring_emit_tmz(ring, true, trusted);
>> -
>> dw2 |= 0x80000000; /* set load_enable otherwise this package is just NOPs */
>> if (flags & AMDGPU_HAVE_CTX_SWITCH) {
>> /* set load_global_config & load_global_uconfig */
>> diff --git a/include/uapi/drm/amdgpu_drm.h b/include/uapi/drm/amdgpu_drm.h
>> index 918ac3548cd3..eaf94a421901 100644
>> --- a/include/uapi/drm/amdgpu_drm.h
>> +++ b/include/uapi/drm/amdgpu_drm.h
>> @@ -556,9 +556,6 @@ struct drm_amdgpu_cs_chunk {
>> __u64 chunk_data;
>> };
>>
>> -/* Flag the command submission as secure */
>> -#define AMDGPU_CS_FLAGS_SECURE (1 << 0)
>> -
>> struct drm_amdgpu_cs_in {
>> /** Rendering context id */
>> __u32 ctx_id;
>> @@ -599,6 +596,10 @@ union drm_amdgpu_cs {
>> */
>> #define AMDGPU_IB_FLAG_RESET_GDS_MAX_WAVE_ID (1 << 4)
>>
>> +/* Flag the IB as secure (TMZ)
>> + */
>> +#define AMDGPU_IB_FLAGS_SECURE (1 << 5)
>> +
>> struct drm_amdgpu_cs_chunk_ib {
>> __u32 _pad;
>> /** AMDGPU_IB_FLAG_* */
>
More information about the amd-gfx
mailing list