[PATCH 2/3] drm/xe/hdcp: Enable HDCP for XE

[Daniele Ceraolo Spurio]

On 2/6/2024 8:24 AM, Kandpal, Suraj wrote:
>> Subject: Re: [PATCH 2/3] drm/xe/hdcp: Enable HDCP for XE
>>
>>
>>
>> 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 at intel.com>
>>> ---
>>>    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.
>>
> Right
>
>>>    #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.
>>
> True will get rid of it
>
>>> +#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?
> Not sure which function would check if gsc and gsc proxy is loaded or not
> Any idea?

gsc_proxy_init_done() in xe_gsc_proxy.c . It's not exposed right now 
because there was no user outside of the file.

Note that, differently from i915, Xe is very explicit with pm and 
forcewake management, so you'll have to take both a pm and a forcewake 
ref before calling that function.


>
>>> +}
>>> +
>>> +/*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.
>>
> I understand it looks weird but display code seems to be following this convention for now
> till a decision is made on how display code redundancy is removed maybe Ankit can further
> back this design or comment on it.
>
>>> +					     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.
>>
>   Ok got it
>
>>> +		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.
>>
> Let me have a look at this
>
>>> +
>>> +	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.
> No it needs the next patch separated them for reviews will squash them and send it for merging

ok. Also, I just realized that you're accessing i915->display, which is 
incorrect because the i915 pointer is actually an xe_device object under 
the hood and therefore can have its display substructure at a different 
memory location. You need to cast it to xe_device and do xe->display, 
otherwise you might be accessing the wrong memory location.

Daniele

>
>>> +	ret = intel_hdcp_gsc_hdcp2_init(i915);
>>> +	mutex_unlock(&i915->display.hdcp.hdcp_mutex);
>>> +
>>> +	return ret;
>> Here as well missing the kfree on error
>>
> Will fix this
>
>>>    }
>>>
>>>    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.
> Will clean that up
>
>>> +			    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 = &gt->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.
> True let me update that
>
>>> +
>>> +	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.
>>
> This was changed in my latest patch series that you had reviewed in which now readout header also checks the status .
>
>>> +
>>> +	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.
>>
> Will get this done
>
>>> +	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)
>>
> Yes aware of that will rename the variable to avoid confusion
>
>>> +					 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.
> So normally hdcp messages return variable messages even for the same cmd depending on the key being already stored or not so as long as I have minimum size and status does not indicate error it should be fine.
>
> Regards,
> Suraj Kandpal
>
>> Daniele
>>
>>> +
>>> +out:
>>> +	xe_device_mem_access_put(i915);
>>> +	return ret;
>>>    }