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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> 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@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.
> 

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?

> 
> > +}
> > +
> > +/*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

> 
> > +	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;
> >   }





[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux