| Message ID | 20240202083737.1088306-3-suraj.kandpal@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | XE HDCP Enablement | expand |
Hi Suraj,
kernel test robot noticed the following build warnings:
[auto build test WARNING on drm-intel/for-linux-next]
[also build test WARNING on drm-intel/for-linux-next-fixes drm-xe/drm-xe-next drm-tip/drm-tip linus/master v6.8-rc2 next-20240202]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Suraj-Kandpal/drm-i915-hdcp-Move-intel_hdcp_gsc_message-def-away-from-header-file/20240202-164840
base: git://anongit.freedesktop.org/drm-intel for-linux-next
patch link: https://lore.kernel.org/r/20240202083737.1088306-3-suraj.kandpal%40intel.com
patch subject: [PATCH 2/3] drm/xe/hdcp: Enable HDCP for XE
config: s390-allmodconfig (https://download.01.org/0day-ci/archive/20240203/202402030852.sSijMp2S-lkp@intel.com/config)
compiler: clang version 19.0.0git (https://github.com/llvm/llvm-project 7dd790db8b77c4a833c06632e903dc4f13877a64)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240203/202402030852.sSijMp2S-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202402030852.sSijMp2S-lkp@intel.com/
All warnings (new ones prefixed by >>):
In file included from drivers/gpu/drm/xe/display/xe_hdcp_gsc.c:7:
In file included from drivers/gpu/drm/xe/compat-i915-headers/i915_drv.h:15:
In file included from drivers/gpu/drm/xe/compat-i915-headers/gem/i915_gem_object.h:11:
In file included from drivers/gpu/drm/xe/xe_bo.h:11:
In file included from drivers/gpu/drm/xe/xe_bo_types.h:9:
In file included from include/linux/iosys-map.h:10:
In file included from include/linux/io.h:13:
In file included from arch/s390/include/asm/io.h:78:
include/asm-generic/io.h:547:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
547 | val = __raw_readb(PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:560:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
560 | val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
| ~~~~~~~~~~ ^
include/uapi/linux/byteorder/big_endian.h:37:59: note: expanded from macro '__le16_to_cpu'
37 | #define __le16_to_cpu(x) __swab16((__force __u16)(__le16)(x))
| ^
include/uapi/linux/swab.h:102:54: note: expanded from macro '__swab16'
102 | #define __swab16(x) (__u16)__builtin_bswap16((__u16)(x))
| ^
In file included from drivers/gpu/drm/xe/display/xe_hdcp_gsc.c:7:
In file included from drivers/gpu/drm/xe/compat-i915-headers/i915_drv.h:15:
In file included from drivers/gpu/drm/xe/compat-i915-headers/gem/i915_gem_object.h:11:
In file included from drivers/gpu/drm/xe/xe_bo.h:11:
In file included from drivers/gpu/drm/xe/xe_bo_types.h:9:
In file included from include/linux/iosys-map.h:10:
In file included from include/linux/io.h:13:
In file included from arch/s390/include/asm/io.h:78:
include/asm-generic/io.h:573:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
573 | val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
| ~~~~~~~~~~ ^
include/uapi/linux/byteorder/big_endian.h:35:59: note: expanded from macro '__le32_to_cpu'
35 | #define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x))
| ^
include/uapi/linux/swab.h:115:54: note: expanded from macro '__swab32'
115 | #define __swab32(x) (__u32)__builtin_bswap32((__u32)(x))
| ^
In file included from drivers/gpu/drm/xe/display/xe_hdcp_gsc.c:7:
In file included from drivers/gpu/drm/xe/compat-i915-headers/i915_drv.h:15:
In file included from drivers/gpu/drm/xe/compat-i915-headers/gem/i915_gem_object.h:11:
In file included from drivers/gpu/drm/xe/xe_bo.h:11:
In file included from drivers/gpu/drm/xe/xe_bo_types.h:9:
In file included from include/linux/iosys-map.h:10:
In file included from include/linux/io.h:13:
In file included from arch/s390/include/asm/io.h:78:
include/asm-generic/io.h:584:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
584 | __raw_writeb(value, PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:594:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
594 | __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:604:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
604 | __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:692:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
692 | readsb(PCI_IOBASE + addr, buffer, count);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:700:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
700 | readsw(PCI_IOBASE + addr, buffer, count);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:708:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
708 | readsl(PCI_IOBASE + addr, buffer, count);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:717:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
717 | writesb(PCI_IOBASE + addr, buffer, count);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:726:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
726 | writesw(PCI_IOBASE + addr, buffer, count);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:735:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
735 | writesl(PCI_IOBASE + addr, buffer, count);
| ~~~~~~~~~~ ^
>> drivers/gpu/drm/xe/display/xe_hdcp_gsc.c:53:9: warning: variable 'err' is uninitialized when used here [-Wuninitialized]
53 | ret = err;
| ^~~
drivers/gpu/drm/xe/display/xe_hdcp_gsc.c:41:9: note: initialize the variable 'err' to silence this warning
41 | int err, ret = 0;
| ^
| = 0
drivers/gpu/drm/xe/display/xe_hdcp_gsc.c:105:23: error: invalid application of 'sizeof' to an incomplete type 'struct i915_hdcp_arbiter'
105 | data = kzalloc(sizeof(*data), GFP_KERNEL);
| ^~~~~~~
drivers/gpu/drm/i915/display/intel_display_core.h:37:8: note: forward declaration of 'struct i915_hdcp_arbiter'
37 | struct i915_hdcp_arbiter;
| ^
drivers/gpu/drm/xe/display/xe_hdcp_gsc.c:111:28: error: incomplete definition of type 'struct i915_hdcp_arbiter'
111 | i915->display.hdcp.arbiter->hdcp_dev = i915->drm.dev;
| ~~~~~~~~~~~~~~~~~~~~~~~~~~^
drivers/gpu/drm/i915/display/intel_display_core.h:37:8: note: forward declaration of 'struct i915_hdcp_arbiter'
37 | struct i915_hdcp_arbiter;
| ^
drivers/gpu/drm/xe/display/xe_hdcp_gsc.c:112:28: error: incomplete definition of type 'struct i915_hdcp_arbiter'
112 | i915->display.hdcp.arbiter->ops = &gsc_hdcp_ops;
| ~~~~~~~~~~~~~~~~~~~~~~~~~~^
drivers/gpu/drm/i915/display/intel_display_core.h:37:8: note: forward declaration of 'struct i915_hdcp_arbiter'
37 | struct i915_hdcp_arbiter;
| ^
drivers/gpu/drm/xe/display/xe_hdcp_gsc.c:112:37: error: use of undeclared identifier 'gsc_hdcp_ops'
112 | i915->display.hdcp.arbiter->ops = &gsc_hdcp_ops;
| ^
drivers/gpu/drm/xe/display/xe_hdcp_gsc.c:153:10: error: invalid application of 'sizeof' to an incomplete type 'struct hdcp_cmd_header'
153 | sizeof(struct hdcp_cmd_header), NULL);
| ^ ~~~~~~~~~~~~~~~~~~~~~~~~
drivers/gpu/drm/xe/display/xe_hdcp_gsc.c:153:24: note: forward declaration of 'struct hdcp_cmd_header'
153 | sizeof(struct hdcp_cmd_header), NULL);
| ^
13 warnings and 5 errors generated.
vim +/err +53 drivers/gpu/drm/xe/display/xe_hdcp_gsc.c
34
35 /*This function helps allocate memory for the command that we will send to gsc cs */
36 static int intel_hdcp_gsc_initialize_message(struct drm_i915_private *i915,
37 struct intel_hdcp_gsc_message *hdcp_message)
38 {
39 struct xe_bo *bo = NULL;
40 u64 cmd_in, cmd_out;
41 int err, ret = 0;
42
43 /* allocate object of two page for HDCP command memory and store it */
44
45 xe_device_mem_access_get(i915);
46 bo = xe_bo_create_pin_map(i915, xe_device_get_root_tile(i915), NULL, PAGE_SIZE * 2,
47 ttm_bo_type_kernel,
48 XE_BO_CREATE_SYSTEM_BIT |
49 XE_BO_CREATE_GGTT_BIT);
50
51 if (IS_ERR(bo)) {
52 drm_err(&i915->drm, "Failed to allocate bo for HDCP streaming command!\n");
> 53 ret = err;
54 goto out;
55 }
56
57 cmd_in = xe_bo_ggtt_addr(bo);
58
59 if (iosys_map_is_null(&bo->vmap)) {
60 drm_err(&i915->drm, "Failed to map gsc message page!\n");
61 ret = PTR_ERR(&bo->vmap);
62 goto out;
63 }
64
65 cmd_out = cmd_in + PAGE_SIZE;
66
67 xe_map_memset(i915, &bo->vmap, 0, 0, bo->size);
68
69 hdcp_message->hdcp_bo = bo;
70 hdcp_message->hdcp_cmd_in = cmd_in;
71 hdcp_message->hdcp_cmd_out = cmd_out;
72 out:
73 xe_device_mem_access_put(i915);
74 return ret;
75 }
76
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@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. > #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; > }
> 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@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? > > > +} > > + > > +/*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 = >->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; > > }
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@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 = >->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; >>> }
On Mon, 05 Feb 2024, Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> 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@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. 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; >> } >
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" #include "i915_drv.h" #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) +#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; +} + +/*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, + 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)) { + 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"); + + 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; + ret = intel_hdcp_gsc_hdcp2_init(i915); + mutex_unlock(&i915->display.hdcp.hdcp_mutex); + + return ret; } 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, + 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); + + if (ret) + return -EAGAIN; + + ret = xe_gsc_read_out_header(i915, 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, 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; + xe_device_mem_access_get(i915); + addr_in_off = xe_gsc_emit_header(i915, &hdcp_message->hdcp_bo->vmap, + 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); + +out: + xe_device_mem_access_put(i915); + return ret; }
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@intel.com> --- drivers/gpu/drm/xe/display/xe_hdcp_gsc.c | 188 ++++++++++++++++++++++- 1 file changed, 184 insertions(+), 4 deletions(-)