On Mon, 05 Feb 2024, Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx> wrote:
> On 2/2/2024 12:37 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.
>>
>> Signed-off-by: Suraj Kandpal <suraj.kandpal@xxxxxxxxx>
>> ---
>> drivers/gpu/drm/xe/display/xe_hdcp_gsc.c | 188 ++++++++++++++++++++++-
>> 1 file changed, 184 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/display/xe_hdcp_gsc.c b/drivers/gpu/drm/xe/display/xe_hdcp_gsc.c
>> index 0f11a39333e2..eca941d7b281 100644
>> --- a/drivers/gpu/drm/xe/display/xe_hdcp_gsc.c
>> +++ b/drivers/gpu/drm/xe/display/xe_hdcp_gsc.c
>> @@ -3,8 +3,24 @@
>> * Copyright 2023, Intel Corporation.
>> */
>>
>> +#include "abi/gsc_command_header_abi.h"
>
> My original idea was for the users to not include this header and rely
> on the size provided by the emit functions. I see you use the check the
> input size, which I didn't do on the proxy side because the buffer is
> sized to be big enough for all possible commands. Overall not a blocker,
> just consider the option.
>
>> #include "i915_drv.h"
>
> Do you actually need i915_drv.h? You shouldn't be using any structure
> from i915 here. If you just need it for the pointers to struct
> drm_i915_private, just add a forward declaration for the structure.
Xe side it really must be struct xe_device, not drm_i915_private.
See xe Makefile.
BR,
Jani.
>
>> #include "intel_hdcp_gsc.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 HOST_SESSION_CLIENT_MASK GENMASK_ULL(63, 56)
>> +#define HDCP_GSC_MESSAGE_SIZE sizeof(struct intel_hdcp_gsc_message)
>
> this define is unused. Also, intel_hdcp_gsc_message is not the actual
> message, but just contains a pointer to the object that holds the message.
>
>> +#define HDCP_GSC_HEADER_SIZE sizeof(struct intel_gsc_mtl_header)
>>
>> bool intel_hdcp_gsc_cs_required(struct drm_i915_private *i915)
>> {
>> @@ -13,22 +29,186 @@ bool intel_hdcp_gsc_cs_required(struct drm_i915_private *i915)
>>
>> bool intel_hdcp_gsc_check_status(struct drm_i915_private *i915)
>> {
>> - return false;
>> + return true;
>
> Shouldn't you actually do a check in here?
>
>> +}
>> +
>> +/*This function helps allocate memory for the command that we will send to gsc cs */
>> +static int intel_hdcp_gsc_initialize_message(struct drm_i915_private *i915,
>
> Having a drm_i915_private here that is actually an xe_device is really
> weird. I understand that the aim is to re-use most of the display code
> from i915, but if you need to different back-ends maybe just have the
> function accept a void pointer and then internally cast it to
> drm_i915_private or xe_device based on the driver, or use struct
> intel_display and cast it back to i915 or Xe with a container_of. I'll
> leave the final comment on this to someone that has more understanding
> than me of what's going on on the display side of things.
>
>> + 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(i915);
>> + bo = xe_bo_create_pin_map(i915, xe_device_get_root_tile(i915), NULL, PAGE_SIZE * 2,
>> + ttm_bo_type_kernel,
>> + XE_BO_CREATE_SYSTEM_BIT |
>> + XE_BO_CREATE_GGTT_BIT);
>> +
>> + if (IS_ERR(bo)) {
>> + drm_err(&i915->drm, "Failed to allocate bo for HDCP streaming command!\n");
>> + ret = err;
>> + goto out;
>> + }
>> +
>> + cmd_in = xe_bo_ggtt_addr(bo);
>> +
>> + if (iosys_map_is_null(&bo->vmap)) {
>
> this can't happen, if the bo fails to map then xe_bo_create_pin_map will
> return an error.
>
>> + drm_err(&i915->drm, "Failed to map gsc message page!\n");
>> + ret = PTR_ERR(&bo->vmap);
>> + goto out;
>> + }
>> +
>> + cmd_out = cmd_in + PAGE_SIZE;
>> +
>> + xe_map_memset(i915, &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(i915);
>> + return ret;
>> +}
>> +
>> +static int intel_hdcp_gsc_hdcp2_init(struct drm_i915_private *i915)
>> +{
>> + 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
>> + */
>> + i915->display.hdcp.hdcp_message = hdcp_message;
>> + ret = intel_hdcp_gsc_initialize_message(i915, hdcp_message);
>> +
>> + if (ret)
>> + drm_err(&i915->drm, "Could not initialize hdcp_message\n");
>
> Don't you need a kfree in this error path? alternatively you can use
> drmm_kzalloc so that it is always automatically freed.
>
>> +
>> + return ret;
>> }
>>
>> int intel_hdcp_gsc_init(struct drm_i915_private *i915)
>> {
>> - drm_info(&i915->drm, "HDCP support not yet implemented\n");
>> - return -ENODEV;
>> + struct i915_hdcp_arbiter *data;
>> + int ret;
>> +
>> + data = kzalloc(sizeof(*data), GFP_KERNEL);
>> + if (!data)
>> + return -ENOMEM;
>> +
>> + mutex_lock(&i915->display.hdcp.hdcp_mutex);
>> + i915->display.hdcp.arbiter = data;
>> + i915->display.hdcp.arbiter->hdcp_dev = i915->drm.dev;
>> + i915->display.hdcp.arbiter->ops = &gsc_hdcp_ops;
>
> Does this patch compile on its own? As far as I can see gsc_hdcp_ops is
> added in the next patch.
>
>> + ret = intel_hdcp_gsc_hdcp2_init(i915);
>> + mutex_unlock(&i915->display.hdcp.hdcp_mutex);
>> +
>> + return ret;
>
> Here as well missing the kfree on error
>
>> }
>>
>> void intel_hdcp_gsc_fini(struct drm_i915_private *i915)
>> {
>> + struct intel_hdcp_gsc_message *hdcp_message =
>> + i915->display.hdcp.hdcp_message;
>> +
>> + xe_bo_unpin_map_no_vm(hdcp_message->hdcp_bo);
>> + kfree(hdcp_message);
>> +
>> }
>>
>> +static int xe_gsc_send_sync(struct drm_i915_private *i915,
>> + struct intel_hdcp_gsc_message *hdcp_message,
>> + u32 msg_size_in, u32 msg_size_out,
>> + u32 addr_in_off, u32 addr_out_off,
>
> Those 2 variables are unused.
>
>> + size_t msg_out_len)
>> +{
>> + 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(&i915->drm, "failed to send gsc HDCP msg (%d)\n", ret);
>> + return ret;
>> + }
>> +
>> + ret = xe_gsc_check_and_update_pending(i915, map, 0, map, addr_out_off);
>
> This returns a bool, so you can call it directly inside the if statement
> instead of casting the return to int.
>
>> +
>> + if (ret)
>> + return -EAGAIN;
>> +
>> + ret = xe_gsc_read_out_header(i915, map, addr_out_off,
>> + sizeof(struct hdcp_cmd_header), NULL);
>
> Note that here you're only checking that the message is at least as big
> as struct hdcp_cmd_header, but if there was an error and the only thing
> in the message was the header it'll still pass. This links with a
> comment below.
>
>> +
>> + return ret;
>> +}
>> 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)
>> {
>> - 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_off = 0, addr_out_off;
>> + 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 = i915->display.hdcp.hdcp_message;
>> + addr_out_off = PAGE_SIZE;
>> +
>> + get_random_bytes(&host_session_id, sizeof(u64));
>> + host_session_id = host_session_id & ~HOST_SESSION_CLIENT_MASK;
>
> Can you move this host session code to a dedicated function in
> xe_gsc_submit.c? that way we can re-use it for PXP. You can also drop
> the re-definition of HOST_SESSION_CLIENT_MASK because that's already in
> that file.
>
>> + xe_device_mem_access_get(i915);
>> + addr_in_off = xe_gsc_emit_header(i915, &hdcp_message->hdcp_bo->vmap,
>
> Note that this function does not return the input offset, but the next
> writable location (that's why I called it wr_offset in other code)
>
>> + addr_in_off,
>> + HECI_MEADDRESS_HDCP, host_session_id,
>> + msg_in_len);
>> +
>> + xe_map_memcpy_to(i915, &hdcp_message->hdcp_bo->vmap, addr_in_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(i915, hdcp_message, msg_size_in, msg_size_out,
>> + addr_in_off, addr_out_off, msg_out_len);
>> +
>> + /* Only try again if gsc says so */
>> + if (ret != -EAGAIN)
>> + break;
>> +
>> + msleep(50);
>> +
>> + } while (++tries < 20);
>> +
>> + if (ret)
>> + goto out;
>> +
>> + xe_map_memcpy_from(i915, msg_out, &hdcp_message->hdcp_bo->vmap,
>> + addr_out_off + HDCP_GSC_HEADER_SIZE,
>> + msg_out_len);
>
> here you are copying msg_out_len, but you haven't checked if the GSC has
> actually written that much, you only checked that you had struct
> hdcp_cmd_header.
>
> Daniele
>
>> +
>> +out:
>> + xe_device_mem_access_put(i915);
>> + return ret;
>> }
>
--
Jani Nikula, Intel
