On Tue, 2023-01-10 at 16:27 +0530, Suraj Kandpal wrote:
> Add function that takes care of sending command to gsc cs. We start
> of with allocation of memory for our command intel_hdcp_gsc_message that
> contains gsc cs memory header as directed in specs followed by the
> actual payload hdcp message that we want to send.
> Spec states that we need to poll pending bit of response header around
> 20 times each try being 50ms apart hence adding that to current
> gsc_msg_send function
> Also we use the same function to take care of both sending and receiving
> hence no separate function to get the response.
>
Alan: [snip]
> +
> +/*This function helps allocate memory for the command that we will send to gsc cs */
> +static int intel_initialize_hdcp_gsc_message(struct drm_i915_private *i915,
> + struct intel_hdcp_gsc_message *hdcp_message)
> +{
> + struct intel_gt *gt = i915->media_gt;
> + struct drm_i915_gem_object *obj = NULL;
> + struct i915_vma *vma = NULL;
> + void *cmd;
> + int err;
> +
> + /* allocate object of one page for HDCP command memory and store it */
> + obj = i915_gem_object_create_shmem(i915, PAGE_SIZE);
> +
> + if (IS_ERR(obj)) {
> + drm_err(&i915->drm, "Failed to allocate HDCP streaming command!\n");
> + return PTR_ERR(obj);
> + }
> +
> + cmd = i915_gem_object_pin_map_unlocked(obj, i915_coherent_map_type(i915, obj, true));
> + if (IS_ERR(cmd)) {
> + drm_err(&i915->drm, "Failed to map gsc message page!\n");
> + err = PTR_ERR(cmd);
> + goto out_unpin;
> + }
> +
> + vma = i915_vma_instance(obj, >->ggtt->vm, NULL);
> + if (IS_ERR(vma)) {
> + err = PTR_ERR(vma);
> + goto out_unmap;
> + }
> +
> + err = i915_vma_pin(vma, 0, 0, PIN_GLOBAL);
> + if (err)
> + goto out_unmap;
> +
> + memset(cmd, 0, obj->base.size);
> +
Alan: I'm still not convinced why we need to memset the whole 4K page (especially considering you are allocating, using and freeing this buffer during establishment and then again and again every few second while hdcp link is up). Also, in intel_hdcp_gsc_msg_send, i see the following code:
"memcpy(hdcp_message->hdcp_cmd + sizeof(*header), msg_in, msg_in_len);"
Thus I dont see why the firmware would ever overfetch AND decode data past the valid message being sent (hdcp_cmd_header->command_id). If it does, then wouldnt that be a GSC fw bug?
> + hdcp_message->obj = obj;
> + hdcp_message->hdcp_cmd = cmd;
> + hdcp_message->vma = vma;
> +
> + return 0;
> +
> +out_unmap:
> + i915_gem_object_unpin_map(obj);
> +out_unpin:
> + i915_gem_object_put(obj);
> + return err;
> +}
> +
> +static void intel_free_hdcp_gsc_message(struct intel_hdcp_gsc_message *hdcp_message)
> +{
> + struct drm_i915_gem_object *obj = fetch_and_zero(&hdcp_message->obj);
> +
> + if (!obj)
> + return;
> +
> + if (hdcp_message->vma)
> + i915_vma_unpin(fetch_and_zero(&hdcp_message->vma));
> +
> + i915_gem_object_unpin_map(obj);
> + i915_gem_object_put(obj);
Alan: I realize now that the hdcp_message->obj is not used anywhere else except for here.
If you are also unpinning the vma in the same go, you can just i915_vma_unpin_and_release with I915_VMA_RELEASE_MAP
and it'll do the unmapping and unpinning for you. That way, you won't
need to store the hdcp_message->obj.
> + kfree(hdcp_message);
> +}
> +
Alan: [snip]
> +/*
> + * This function can now be used for sending requests and will also handle
> + * receipt of reply messages hence no different function of message retrieval
> + * is required. We will initialize intel_hdcp_gsc_message structure then add
> + * gsc cs memory header as stated in specs after which the normal HDCP payload
> + * will follow
> + */
> +ssize_t intel_hdcp_gsc_msg_send(struct drm_i915_private *i915, u8 *msg_in,
> + size_t msg_in_len, u8 *msg_out, size_t msg_out_len)
> +{
> + struct intel_gt *gt = i915->media_gt;
> + struct intel_gsc_mtl_header *header;
> + const size_t max_msg_size = PAGE_SIZE - sizeof(*header);
> + struct intel_hdcp_gsc_message *hdcp_message;
> + u64 addr, host_session_id;
> + u32 reply_size, msg_size;
> + int ret, tries = 0;
> +
> + if (!intel_uc_uses_gsc_uc(>->uc))
> + return -ENODEV;
> +
> + if (msg_in_len > max_msg_size || msg_out_len > max_msg_size)
> + return -ENOSPC;
> +
> + hdcp_message = kzalloc(sizeof(*hdcp_message), GFP_KERNEL);
> +
> + if (!hdcp_message)
> + return -ENOMEM;
> +
> + ret = intel_initialize_hdcp_gsc_message(i915, hdcp_message);
> +
> + if (ret) {
> + drm_err(&i915->drm,
> + "Could not initialize hdcp_message\n");
> + goto err;
> + }
> +
> + header = hdcp_message->hdcp_cmd;
> + addr = i915_ggtt_offset(hdcp_message->vma);
> +
> + memset(header, 0, sizeof(*header));
Alan: we know this is redundant code if intel_initialize_hdcp_gsc_message memsets the the entire hdcp_message object.
However, i rather we eliminate the huge 4k memset in intel_initialize_hdcp_gsc_message and keep this one.
> + msg_size = msg_in_len + sizeof(*header);
> + get_random_bytes(&host_session_id, sizeof(u64));
> + intel_gsc_uc_heci_cmd_emit_mtl_header(header, HECI_MEADDRESS_HDCP,
> + msg_size, host_session_id);
> + memcpy(hdcp_message->hdcp_cmd + sizeof(*header), 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 = intel_gsc_send_sync(i915, header, addr, msg_out_len);
> +
> + /* Only try again if gsc says so */
> + if (ret != -EAGAIN)
> + break;
> +
> + msleep(50);
> +
> + } while (++tries < 20);
> +
> + if (ret)
> + goto err;
> +
> + /* we use the same mem for the reply, so header is in the same loc */
> + reply_size = header->message_size - sizeof(*header);
> + if (reply_size > msg_out_len) {
> + drm_warn(&i915->drm, "caller with insufficient HDCP reply size %u (%d)\n",
> + reply_size, (u32)msg_out_len);
> + reply_size = msg_out_len;
> + } else if (reply_size != msg_out_len) {
> + drm_dbg_kms(&i915->drm, "caller unexpected HCDP reply size %u (%d)\n",
> + reply_size, (u32)msg_out_len);
> + }
> +
> + memcpy(msg_out, hdcp_message->hdcp_cmd + sizeof(*header), msg_out_len);
> +
> +err:
> + intel_free_hdcp_gsc_message(hdcp_message);
> + return ret;
> +}
> diff --git a/drivers/gpu/drm/i915/display/intel_hdcp_gsc.h b/drivers/gpu/drm/i915/display/intel_hdcp_gsc.h
> new file mode 100644
> index 000000000000..582f4992be76
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/display/intel_hdcp_gsc.h
> @@ -0,0 +1,18 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright © 2023 Intel Corporation
> + */
> +
> +#ifndef __INTEL_HDCP_GSC_H__
> +#define __INTEL_HDCP_GSC_H__
> +
> +#include <linux/types.h>
> +#include <linux/err.h>
Alan: should this be alphabetical?
Alan:[snip]
>
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_heci_cmd_submit.h b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_heci_cmd_submit.h
> index cf610dfca7a5..78fe04f911b9 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_heci_cmd_submit.h
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_heci_cmd_submit.h
> @@ -23,6 +23,8 @@ struct intel_gsc_mtl_header {
> #define MTL_GSC_HEADER_VERSION 1
>
> u64 host_session_handle;
> +#define HOST_SESSION_MASK REG_GENMASK64(63, 60)
> +#define HOST_SESSION_PXP_SINGLE BIT_ULL(60)
Alan: Daniele communicated this through MTL-PXP series that has this same hunk above: Since this bitmask is something we want in driver side, we need to add some comments like "FW allows host to decide the host_session_handle as it see fits. For internal tracibility, lets reserve select bits to differentiate the caller-target subsystem."
> u64 gsc_message_handle;
>
> u32 message_size; /* lower 20 bits only, upper 12 are reserved */
> @@ -35,6 +37,7 @@ struct intel_gsc_mtl_header {
> * Bits 16-31: Extension Size
> */
> u32 flags;
> +#define INTEL_GSC_MSG_PENDING 1
Alan: Nit: Based on internal spec, some flags are inputs and some are output, perhaps we add a comment on a naming format and change this to GSC_OUTFLAG_MSG_PENDING?
>
> u32 status;
> } __packed;
> @@ -42,4 +45,7 @@ struct intel_gsc_mtl_header {
> int intel_gsc_uc_heci_cmd_submit_packet(struct intel_gsc_uc *gsc,
> u64 addr_in, u32 size_in,
> u64 addr_out, u32 size_out);
> +void intel_gsc_uc_heci_cmd_emit_mtl_header(struct intel_gsc_mtl_header *header,
> + u8 heci_client_id, u32 message_size,
> + u64 host_session_id);
> #endif
> --
> 2.25.1
>
