[PATCH 3/4] drm/xe/hdcp: Enable HDCP for XE
Daniele Ceraolo Spurio
daniele.ceraolospurio at intel.com
Fri Feb 9 01:11:09 UTC 2024
On 2/7/2024 3:35 AM, Suraj Kandpal wrote:
> Enable HDCP for Xe by defining functions which take care of
> interaction of HDCP as a client with the GSC CS interface.
>
> --v2
> -add kfree at appropriate place [Daniele]
> -forward declare drm_i915_private [Daniele]
I don't see the forward declaration, just the switch to xe_device (which
is better).
> -remove useless define [Daniele]
> -move host session logic to xe_gsc_submit.c [Daniele]
> -call xe_gsc_check_and_update_pending directly in an if condition
> [Daniele]
> -use xe_device instead of drm_i915_private [Daniele]
>
> Signed-off-by: Suraj Kandpal <suraj.kandpal at intel.com>
> ---
> drivers/gpu/drm/i915/display/intel_hdcp_gsc.c | 4 +-
> .../gpu/drm/xe/abi/gsc_command_header_abi.h | 2 +
> drivers/gpu/drm/xe/display/xe_hdcp_gsc.c | 184 +++++++++++++++++-
> drivers/gpu/drm/xe/xe_gsc_submit.c | 19 ++
> drivers/gpu/drm/xe/xe_gsc_submit.h | 1 +
> 5 files changed, 202 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_hdcp_gsc.c b/drivers/gpu/drm/i915/display/intel_hdcp_gsc.c
> index e44f60f00e8b..9e895f714f90 100644
> --- a/drivers/gpu/drm/i915/display/intel_hdcp_gsc.c
> +++ b/drivers/gpu/drm/i915/display/intel_hdcp_gsc.c
> @@ -123,8 +123,10 @@ static int intel_hdcp_gsc_hdcp2_init(struct drm_i915_private *i915)
> i915->display.hdcp.hdcp_message = hdcp_message;
> ret = intel_hdcp_gsc_initialize_message(i915, hdcp_message);
>
> - if (ret)
> + if (ret) {
> drm_err(&i915->drm, "Could not initialize hdcp_message\n");
> + kfree(hdcp_message);
> + }
AFAIU intel_hdcp_gsc.c is not used in Xe, so IMO you should send this
change as an i915 patch.
>
> return ret;
> }
> diff --git a/drivers/gpu/drm/xe/abi/gsc_command_header_abi.h b/drivers/gpu/drm/xe/abi/gsc_command_header_abi.h
> index a4c2646803b5..d2fbf71439be 100644
> --- a/drivers/gpu/drm/xe/abi/gsc_command_header_abi.h
> +++ b/drivers/gpu/drm/xe/abi/gsc_command_header_abi.h
> @@ -21,6 +21,8 @@ struct intel_gsc_mtl_header {
>
> /* FW allows host to decide host_session handle as it sees fit. */
> u64 host_session_handle;
> +#define HECI_MEADDRESS_PXP 17
> +#define HECI_MEADDRESS_HDCP 18
This was purposely not defined here, and the PXP definition already
exists in the separate header for the PXP stuff. The idea is that each
client should have its own header with its own definitions.
>
> /* handle generated by FW for messages that need to be re-submitted */
> u64 gsc_message_handle;
> diff --git a/drivers/gpu/drm/xe/display/xe_hdcp_gsc.c b/drivers/gpu/drm/xe/display/xe_hdcp_gsc.c
> index ca17dfbc3fe9..d95c1b3b2d9c 100644
> --- a/drivers/gpu/drm/xe/display/xe_hdcp_gsc.c
> +++ b/drivers/gpu/drm/xe/display/xe_hdcp_gsc.c
> @@ -3,11 +3,26 @@
> * Copyright 2023, Intel Corporation.
> */
>
> -#include "i915_drv.h"
> +#include <linux/delay.h>
> +
> +#include "abi/gsc_command_header_abi.h"
> #include "intel_hdcp_gsc.h"
> #include "xe_gt.h"
> #include "xe_gsc_proxy.h"
> #include "xe_pm.h"
> +#include "xe_bo.h"
> +#include "xe_map.h"
> +#include "xe_gsc_submit.h"
> +
> +#define HECI_MEADDRESS_HDCP 18
> +
> +struct intel_hdcp_gsc_message {
> + struct xe_bo *hdcp_bo;
> + u64 hdcp_cmd_in;
> + u64 hdcp_cmd_out;
> +};
> +
> +#define HDCP_GSC_HEADER_SIZE sizeof(struct intel_gsc_mtl_header)
>
> bool intel_hdcp_gsc_cs_required(struct drm_i915_private *i915)
> {
> @@ -37,19 +52,174 @@ bool intel_hdcp_gsc_check_status(struct xe_device *xe)
> return ret;
> }
>
> -int intel_hdcp_gsc_init(struct drm_i915_private *i915)
> +/*This function helps allocate memory for the command that we will send to gsc cs */
> +static int intel_hdcp_gsc_initialize_message(struct xe_device *xe,
> + struct intel_hdcp_gsc_message *hdcp_message)
> +{
> + struct xe_bo *bo = NULL;
> + u64 cmd_in, cmd_out;
> + int err, ret = 0;
> +
> + /* allocate object of two page for HDCP command memory and store it */
> + xe_device_mem_access_get(xe);
> + bo = xe_bo_create_pin_map(xe, xe_device_get_root_tile(xe), NULL, PAGE_SIZE * 2,
> + ttm_bo_type_kernel,
> + XE_BO_CREATE_SYSTEM_BIT |
> + XE_BO_CREATE_GGTT_BIT);
> +
> + if (IS_ERR(bo)) {
> + drm_err(&xe->drm, "Failed to allocate bo for HDCP streaming command!\n");
> + ret = err;
err is unset here.
> + goto out;
> + }
> +
> + cmd_in = xe_bo_ggtt_addr(bo);
> + cmd_out = cmd_in + PAGE_SIZE;
> + xe_map_memset(xe, &bo->vmap, 0, 0, bo->size);
> +
> + hdcp_message->hdcp_bo = bo;
> + hdcp_message->hdcp_cmd_in = cmd_in;
> + hdcp_message->hdcp_cmd_out = cmd_out;
> +out:
> + xe_device_mem_access_put(xe);
> + return ret;
> +}
> +
> +static int intel_hdcp_gsc_hdcp2_init(struct xe_device *xe)
> +{
> + struct intel_hdcp_gsc_message *hdcp_message;
> + int ret;
> +
> + hdcp_message = kzalloc(sizeof(*hdcp_message), GFP_KERNEL);
> +
> + if (!hdcp_message)
> + return -ENOMEM;
> +
> + /*
> + * NOTE: No need to lock the comp mutex here as it is already
> + * going to be taken before this function called
> + */
> + xe->display.hdcp.hdcp_message = hdcp_message;
might be worth moving this assignment to after the init, so we can skip
it in case of errors
> + ret = intel_hdcp_gsc_initialize_message(xe, hdcp_message);
> +
> + if (ret)
> + drm_err(&xe->drm, "Could not initialize hdcp_message\n");
> +
> + return ret;
still missing the kfree(hdcp_message) in case of error?
> +}
> +
> +int intel_hdcp_gsc_init(struct xe_device *xe)
> +{
> + struct i915_hdcp_arbiter *data;
> + int ret;
> +
> + data = kzalloc(sizeof(*data), GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
> +
> + mutex_lock(&xe->display.hdcp.hdcp_mutex);
> + xe->display.hdcp.arbiter = data;
> + xe->display.hdcp.arbiter->hdcp_dev = xe->drm.dev;
> + xe->display.hdcp.arbiter->ops = &gsc_hdcp_ops;
> + ret = intel_hdcp_gsc_hdcp2_init(xe);
> + if (ret)
> + kfree(data);
> +
> + mutex_unlock(&xe->display.hdcp.hdcp_mutex);
> +
> + return ret;
> +}
> +
> +void intel_hdcp_gsc_fini(struct xe_device *xe)
> {
> - drm_info(&i915->drm, "HDCP support not yet implemented\n");
> - return -ENODEV;
> + struct intel_hdcp_gsc_message *hdcp_message =
> + xe->display.hdcp.hdcp_message;
> +
Should we check for hdcp_message != NULL ? in case we hit an error in
the init path.
> + xe_bo_unpin_map_no_vm(hdcp_message->hdcp_bo);
> + kfree(hdcp_message);
> +
> }
>
> -void intel_hdcp_gsc_fini(struct drm_i915_private *i915)
> +static int xe_gsc_send_sync(struct xe_device *xe,
> + struct intel_hdcp_gsc_message *hdcp_message,
> + u32 msg_size_in, u32 msg_size_out,
> + u32 addr_out_off)
> {
> + struct xe_gt *gt = hdcp_message->hdcp_bo->tile->media_gt;
> + struct iosys_map *map = &hdcp_message->hdcp_bo->vmap;
> + struct xe_gsc *gsc = >->uc.gsc;
> + int ret;
> +
> + ret = xe_gsc_pkt_submit_kernel(gsc, hdcp_message->hdcp_cmd_in, msg_size_in,
> + hdcp_message->hdcp_cmd_out, msg_size_out);
> + if (ret) {
> + drm_err(&xe->drm, "failed to send gsc HDCP msg (%d)\n", ret);
> + return ret;
> + }
> +
> + if (xe_gsc_check_and_update_pending(xe, map, 0, map, addr_out_off))
> + return -EAGAIN;
> +
> + ret = xe_gsc_read_out_header(xe, map, addr_out_off,
> + sizeof(struct hdcp_cmd_header), NULL);
> +
> + return ret;
> }
>
> -ssize_t intel_hdcp_gsc_msg_send(struct drm_i915_private *i915, u8 *msg_in,
> +ssize_t intel_hdcp_gsc_msg_send(struct xe_device *xe, u8 *msg_in,
> size_t msg_in_len, u8 *msg_out,
> size_t msg_out_len)
> {
> - return -ENODEV;
> + const size_t max_msg_size = PAGE_SIZE - HDCP_GSC_HEADER_SIZE;
> + struct intel_hdcp_gsc_message *hdcp_message;
> + u64 host_session_id;
> + u32 msg_size_in, msg_size_out, addr_in_wr_off = 0, addr_out_off;
I think we try to avoid too many variables on one line, especially if
only some of them are initialized.
> + int ret, tries = 0;
> +
> + if (msg_in_len > max_msg_size || msg_out_len > max_msg_size) {
> + ret = -ENOSPC;
> + goto out;
> + }
> +
> + msg_size_in = msg_in_len + HDCP_GSC_HEADER_SIZE;
> + msg_size_out = msg_out_len + HDCP_GSC_HEADER_SIZE;
> + hdcp_message = xe->display.hdcp.hdcp_message;
> + addr_out_off = PAGE_SIZE;
> +
> + get_random_bytes(&host_session_id, sizeof(u64));
> + host_session_id = xe_gsc_get_host_session_id(HECI_MEADDRESS_HDCP);
You added a function for this, shouldn't you call that?
> + xe_device_mem_access_get(xe);
> + addr_in_wr_off = xe_gsc_emit_header(xe, &hdcp_message->hdcp_bo->vmap,
> + addr_in_wr_off, HECI_MEADDRESS_HDCP,
> + host_session_id, msg_in_len);
> + xe_map_memcpy_to(xe, &hdcp_message->hdcp_bo->vmap, addr_in_wr_off,
> + msg_in, msg_in_len);
> + /*
> + * Keep sending request in case the pending bit is set no need to add
> + * message handle as we are using same address hence loc. of header is
> + * same and it will contain the message handle. we will send the message
> + * 20 times each message 50 ms apart
> + */
> + do {
> + ret = xe_gsc_send_sync(xe, hdcp_message, msg_size_in, msg_size_out,
> + addr_out_off);
> +
> + /* Only try again if gsc says so */
> + if (ret != -EAGAIN)
> + break;
> +
> + msleep(50);
> +
> + } while (++tries < 20);
> +
> + if (ret)
> + goto out;
> +
If you move xe_gsc_read_out_header here, you can use the returned offset
instead of "addr_out_off + HDCP_GSC_HEADER_SIZE" that you have below.
Not a blocker.
> + xe_map_memcpy_from(xe, msg_out, &hdcp_message->hdcp_bo->vmap,
> + addr_out_off + HDCP_GSC_HEADER_SIZE,
> + msg_out_len);
> +
> +out:
> + xe_device_mem_access_put(xe);
> + return ret;
> }
> diff --git a/drivers/gpu/drm/xe/xe_gsc_submit.c b/drivers/gpu/drm/xe/xe_gsc_submit.c
> index 348994b271be..57793b0acfc3 100644
> --- a/drivers/gpu/drm/xe/xe_gsc_submit.c
> +++ b/drivers/gpu/drm/xe/xe_gsc_submit.c
> @@ -33,6 +33,7 @@
> * include the client id in the top 8 bits of the handle.
> */
> #define HOST_SESSION_CLIENT_MASK GENMASK_ULL(63, 56)
> +#define HOST_SESSION_PXP_SINGLE BIT_ULL(60)
this PXP_SINGLE is not needed
>
> static struct xe_gt *
> gsc_to_gt(struct xe_gsc *gsc)
> @@ -40,6 +41,24 @@ gsc_to_gt(struct xe_gsc *gsc)
> return container_of(gsc, struct xe_gt, uc.gsc);
> }
>
> +/**
> + * xe_gsc_get_host_session_id - Create host session id based on HECI
> + * client address
> + * @heci_client_id: client id identifying the type of command (see abi for values)
> + *
> + * Returns: random host_session_id which can be used to send messages to gsc cs
> + */
> +u64 xe_gsc_get_host_session_id(u8 heci_client_id)
> +{
> + u64 host_session_id;
> +
> + get_random_bytes(&host_session_id, sizeof(u64));
> + host_session_id &= ~HOST_SESSION_CLIENT_MASK;
> + if (host_session_id && heci_client_id == HECI_MEADDRESS_PXP)
> + host_session_id |= HOST_SESSION_PXP_SINGLE;
same here, this PXP exception is not needed
Daniele
> + return host_session_id;
> +};
> +
> /**
> * xe_gsc_emit_header - write the MTL GSC header in memory
> * @xe: the Xe device
> diff --git a/drivers/gpu/drm/xe/xe_gsc_submit.h b/drivers/gpu/drm/xe/xe_gsc_submit.h
> index 1939855031a6..f3359b6659b8 100644
> --- a/drivers/gpu/drm/xe/xe_gsc_submit.h
> +++ b/drivers/gpu/drm/xe/xe_gsc_submit.h
> @@ -28,4 +28,5 @@ int xe_gsc_read_out_header(struct xe_device *xe,
> int xe_gsc_pkt_submit_kernel(struct xe_gsc *gsc, u64 addr_in, u32 size_in,
> u64 addr_out, u32 size_out);
>
> +u64 xe_gsc_get_host_session_id(u8 heci_client_id);
> #endif
More information about the Intel-gfx
mailing list