[Intel-gfx] [RFC-v4 05/21] drm/i915/pxp: Func to send hardware session termination
Huang, Sean Z
sean.z.huang at intel.com
Tue Jan 19 08:31:46 UTC 2021
+Rodrigo,
Hi Chris,
Thanks for your feedback. Replied inline below, and have made several modifications according to your feedback.
May I know if you still have concern regarding the latest rev23? https://patchwork.freedesktop.org/series/84620/#rev23
Regarding the concern about 915->pxp_tee_comp_mutex in https://patchwork.freedesktop.org/patch/406639/?series=84620&rev=4
The pxp_tee_comp_mutex is to protect pxp_tee_comp_added to prevent the race condition that intel_pxp_tee_component_init() and intel_pxp_tee_component_fini() are called at the same time.
We can see the similar usage case for HDCP, but please educate me if there is something I miss.
https://elixir.bootlin.com/linux/v5.11-rc3/source/drivers/gpu/drm/i915/display/intel_hdcp.c#L2025
https://elixir.bootlin.com/linux/v5.11-rc3/source/drivers/gpu/drm/i915/display/intel_hdcp.c#L2232
Best regards,
Sean
-----Original Message-----
From: Chris Wilson <chris at chris-wilson.co.uk>
Sent: Thursday, December 10, 2020 1:19 AM
To: Huang, Sean Z <sean.z.huang at intel.com>; Intel-gfx at lists.freedesktop.org
Subject: Re: [Intel-gfx] [RFC-v4 05/21] drm/i915/pxp: Func to send hardware session termination
Quoting Huang, Sean Z (2020-12-10 07:24:19)
> Implement the functions to allow PXP to send a GPU command, in order
> to terminate the hardware session, so hardware can recycle this
> session slot for the next usage.
>
> Signed-off-by: Huang, Sean Z <sean.z.huang at intel.com>
> ---
> drivers/gpu/drm/i915/Makefile | 1 +
> drivers/gpu/drm/i915/pxp/intel_pxp_cmd.c | 156
> +++++++++++++++++++++++ drivers/gpu/drm/i915/pxp/intel_pxp_cmd.h |
> 18 +++
> 3 files changed, 175 insertions(+)
> create mode 100644 drivers/gpu/drm/i915/pxp/intel_pxp_cmd.c
> create mode 100644 drivers/gpu/drm/i915/pxp/intel_pxp_cmd.h
>
> diff --git a/drivers/gpu/drm/i915/Makefile
> b/drivers/gpu/drm/i915/Makefile index 0710cc522f38..2da904cda49f
> 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -258,6 +258,7 @@ i915-y += i915_perf.o
> i915-$(CONFIG_DRM_I915_PXP) += \
> pxp/intel_pxp.o \
> pxp/intel_pxp_arb.o \
> + pxp/intel_pxp_cmd.o \
> pxp/intel_pxp_context.o \
> pxp/intel_pxp_tee.o
>
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_cmd.c
> b/drivers/gpu/drm/i915/pxp/intel_pxp_cmd.c
> new file mode 100644
> index 000000000000..e531ea9f3cdc
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_cmd.c
> @@ -0,0 +1,156 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright(c) 2020, Intel Corporation. All rights reserved.
> + */
> +
> +#include "intel_pxp_cmd.h"
> +#include "i915_drv.h"
> +#include "gt/intel_context.h"
> +#include "gt/intel_engine_pm.h"
> +
> +struct i915_vma *intel_pxp_cmd_get_batch(struct intel_pxp *pxp,
> + struct intel_context *ce,
> + struct intel_gt_buffer_pool_node *pool,
> + u32 *cmd_buf, int
> +cmd_size_in_dw) {
> + struct i915_vma *batch = ERR_PTR(-EINVAL);
> + struct intel_gt *gt = container_of(pxp, struct intel_gt, pxp);
> + u32 *cmd;
> +
> + if (!ce || !ce->engine || !cmd_buf)
> + return ERR_PTR(-EINVAL);
Really?
> + if (cmd_size_in_dw * 4 > PAGE_SIZE) {
Pardon?
//sean: remove the check
> + drm_err(>->i915->drm, "Failed to %s, invalid cmd_size_id_dw=[%d]\n",
> + __func__, cmd_size_in_dw);
> + return ERR_PTR(-EINVAL);
> + }
> +
> + cmd = i915_gem_object_pin_map(pool->obj, I915_MAP_FORCE_WC);
> + if (IS_ERR(cmd)) {
> + drm_err(>->i915->drm, "Failed to i915_gem_object_pin_map()\n");
> + return ERR_PTR(-EINVAL);
> + }
> +
> + memcpy(cmd, cmd_buf, cmd_size_in_dw * 4);
Why did you bother creating a temporary?
[memcpy32]
//sean: because when this function was called, the PXP caller has completed the GPU command programming in cmd_buf so here is to copy the cmd_buf to cmd which is mapped to graphics address.
> + if (drm_debug_enabled(DRM_UT_DRIVER)) {
> + print_hex_dump(KERN_DEBUG, "cmd binaries:",
> + DUMP_PREFIX_OFFSET, 4, 4, cmd,
> + cmd_size_in_dw * 4, true);
More sillyness again.
> + }
> +
Flush the map after a write
// sean, yes apply the change accordingly in rev23
> + i915_gem_object_unpin_map(pool->obj);
> +
> + batch = i915_vma_instance(pool->obj, ce->vm, NULL);
> + if (IS_ERR(batch)) {
> + drm_err(>->i915->drm, "Failed to i915_vma_instance()\n");
> + return batch;
> + }
> +
> + return batch;
> +}
> +
> +int intel_pxp_cmd_submit(struct intel_pxp *pxp, u32 *cmd, int
> +cmd_size_in_dw) {
> + int err = -EINVAL;
> + struct i915_vma *batch;
> + struct i915_request *rq;
> + struct intel_context *ce = NULL;
> + bool is_engine_pm_get = false;
> + bool is_batch_vma_pin = false;
> + bool is_skip_req_on_err = false;
> + bool is_engine_get_pool = false;
//sean: I have remove those bool and use label at rev23, thanks.
Please implement onion unwind correctly.
And a general plea to be consistent with the rest of the code in laying out locals.
> + struct intel_gt_buffer_pool_node *pool = NULL;
> + struct intel_gt *gt = container_of(pxp, struct intel_gt, pxp);
> +
> + if (!HAS_ENGINE(gt, VCS0) ||
> + !gt->engine[VCS0]->kernel_context) {
What are you doing here if you don't have a VCS? Surely you should not have even bothered to bind PXP in that case?
Don't assume the first available VCS engine has the HW instance id of 0.
The kernel_context exists if the engine exists.
// sean: Yes at rev23 I no long assume VCS0 exists.
> + err = -EINVAL;
> + goto end;
> + }
> +
> + if (!cmd || (cmd_size_in_dw * 4) > PAGE_SIZE) {
Fix your debug code.
//sean: okay, I removed the log print.
> + drm_err(>->i915->drm, "Failed to %s bad params\n", __func__);
> + return -EINVAL;
> + }
> +
> + ce = gt->engine[VCS0]->kernel_context;
DO NOT USE THE KERNEL CONTEXT. Unless you really know what you are doing and can guarantee that under no circumstances does it break.
// I need access the context for submit the GPU command to VCS.
> + intel_engine_pm_get(ce->engine);
> + is_engine_pm_get = true;
> +
> + pool = intel_gt_get_buffer_pool(gt, PAGE_SIZE);
If only you knew what the desired size actually was.
> + if (IS_ERR(pool)) {
> + drm_err(>->i915->drm, "Failed to intel_engine_get_pool()\n");
> + goto end;
> + }
> + is_engine_get_pool = true;
> +
> + batch = intel_pxp_cmd_get_batch(pxp, ce, pool, cmd, cmd_size_in_dw);
> + if (IS_ERR(batch)) {
> + drm_err(>->i915->drm, "Failed to intel_pxp_cmd_get_batch()\n");
> + goto end;
> + }
> +
> + err = i915_vma_pin(batch, 0, 0, PIN_USER);
> + if (err) {
> + drm_err(>->i915->drm, "Failed to i915_vma_pin()\n");
> + goto end;
> + }
> + is_batch_vma_pin = true;
> +
> + rq = intel_context_create_request(ce);
> + if (IS_ERR(rq)) {
> + drm_err(>->i915->drm, "Failed to intel_context_create_request()\n");
> + goto end;
> + }
> + is_skip_req_on_err = true;
> +
> + err = intel_gt_buffer_pool_mark_active(pool, rq);
> + if (err) {
> + drm_err(>->i915->drm, "Failed to intel_engine_pool_mark_active()\n");
> + goto end;
> + }
> +
> + i915_vma_lock(batch);
> + err = i915_request_await_object(rq, batch->obj, false);
> + if (!err)
> + err = i915_vma_move_to_active(batch, rq, 0);
> + i915_vma_unlock(batch);
> + if (err) {
> + drm_err(>->i915->drm, "Failed to i915_request_await_object()\n");
> + goto end;
> + }
> +
> + if (ce->engine->emit_init_breadcrumb) {
> + err = ce->engine->emit_init_breadcrumb(rq);
> + if (err) {
> + drm_err(>->i915->drm, "Failed to emit_init_breadcrumb()\n");
> + goto end;
> + }
> + }
> +
> + err = ce->engine->emit_bb_start(rq, batch->node.start,
> + batch->node.size, 0);
If this is not privileged, why is it done in the kernel? What prevents everyone else from issuing the very same commands to give themselves control?
// According to our design, the PXP default session is owned by kernel space so it's the reason we need to submit this command, to terminate the PXP default session, from kernel instead of user space.
> + if (err) {
> + drm_err(>->i915->drm, "Failed to emit_bb_start()\n");
> + goto end;
> + }
> +
> + i915_request_add(rq);
> +
> +end:
Consistency would be to use out:
> + if (unlikely(err) && is_skip_req_on_err)
> + i915_request_set_error_once(rq, err);
Why?
// sean: I think you're correct, I should just return the err not just for once.
> + if (is_batch_vma_pin)
> + i915_vma_unpin(batch);
> +
> + if (is_engine_get_pool)
> + intel_gt_buffer_pool_put(pool);
> +
> + if (is_engine_pm_get)
> + intel_engine_pm_put(ce->engine);
Style wise, highly inconsistent. Just use labels, spare the locals.
// sean: Changed accordingly, now we use label at rev23.
> + return err;
> +}
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_cmd.h
> b/drivers/gpu/drm/i915/pxp/intel_pxp_cmd.h
> new file mode 100644
> index 000000000000..d04463962421
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_cmd.h
> @@ -0,0 +1,18 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright(c) 2020, Intel Corporation. All rights reserved.
> + */
> +
> +#ifndef __INTEL_PXP_CMD_H__
> +#define __INTEL_PXP_CMD_H__
> +
> +#include "gt/intel_gt_buffer_pool.h"
> +#include "intel_pxp.h"
> +
> +struct i915_vma *intel_pxp_cmd_get_batch(struct intel_pxp *pxp,
> + struct intel_context *ce,
> + struct intel_gt_buffer_pool_node *pool,
> + u32 *cmd_buf, int
> +cmd_size_in_dw);
> +
> +int intel_pxp_cmd_submit(struct intel_pxp *pxp, u32 *cmd, int
> +cmd_size_in_dw); #endif /* __INTEL_PXP_SM_H__ */
> --
> 2.17.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
More information about the Intel-gfx
mailing list