* [PATCH 0/3] XE HDCP Enablement
@ 2024-02-02 8:37 Suraj Kandpal
2024-02-02 8:37 ` [PATCH 1/3] drm/i915/hdcp: Move intel_hdcp_gsc_message def away from header file Suraj Kandpal
` (4 more replies)
0 siblings, 5 replies; 12+ messages in thread
From: Suraj Kandpal @ 2024-02-02 8:37 UTC (permalink / raw)
To: intel-gfx, intel-xe
Cc: daniele.ceraolospurio, chaitanya.kumar.borah, ankit.k.nautiyal,
Suraj Kandpal
This patch series enables HDCP on XE.
Mainly includes rewriting functions that were responsible for
sending hdcp messages via gsc cs.
Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
Suraj Kandpal (3):
drm/i915/hdcp: Move intel_hdcp_gsc_message def away from header file
drm/xe/hdcp: Enable HDCP for XE
drm/xe/hdcp: Add intel_hdcp_gsc_message to Makefile
drivers/gpu/drm/i915/display/intel_hdcp_gsc.c | 6 +
drivers/gpu/drm/i915/display/intel_hdcp_gsc.h | 7 +-
drivers/gpu/drm/xe/Makefile | 1 +
drivers/gpu/drm/xe/display/xe_hdcp_gsc.c | 207 +++++++++++++++++-
4 files changed, 211 insertions(+), 10 deletions(-)
--
2.25.1
^ permalink raw reply [flat|nested] 12+ messages in thread* [PATCH 1/3] drm/i915/hdcp: Move intel_hdcp_gsc_message def away from header file 2024-02-02 8:37 [PATCH 0/3] XE HDCP Enablement Suraj Kandpal @ 2024-02-02 8:37 ` Suraj Kandpal 2024-02-02 8:37 ` [PATCH 2/3] drm/xe/hdcp: Enable HDCP for XE Suraj Kandpal ` (3 subsequent siblings) 4 siblings, 0 replies; 12+ messages in thread From: Suraj Kandpal @ 2024-02-02 8:37 UTC (permalink / raw) To: intel-gfx, intel-xe Cc: daniele.ceraolospurio, chaitanya.kumar.borah, ankit.k.nautiyal, Suraj Kandpal Move intel_hdcp_gsc_message definition into intel_hdcp_gsc_message.c so that intel_hdcp_gsc_message can be redefined for xe as needed. Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com> --- drivers/gpu/drm/i915/display/intel_hdcp_gsc.c | 6 ++++++ drivers/gpu/drm/i915/display/intel_hdcp_gsc.h | 7 +------ 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/drivers/gpu/drm/i915/display/intel_hdcp_gsc.c b/drivers/gpu/drm/i915/display/intel_hdcp_gsc.c index 18117b789b16..e44f60f00e8b 100644 --- a/drivers/gpu/drm/i915/display/intel_hdcp_gsc.c +++ b/drivers/gpu/drm/i915/display/intel_hdcp_gsc.c @@ -13,6 +13,12 @@ #include "intel_hdcp_gsc.h" #include "intel_hdcp_gsc_message.h" +struct intel_hdcp_gsc_message { + struct i915_vma *vma; + void *hdcp_cmd_in; + void *hdcp_cmd_out; +}; + bool intel_hdcp_gsc_cs_required(struct drm_i915_private *i915) { return DISPLAY_VER(i915) >= 14; diff --git a/drivers/gpu/drm/i915/display/intel_hdcp_gsc.h b/drivers/gpu/drm/i915/display/intel_hdcp_gsc.h index eba2057c5a9e..5f610df61cc9 100644 --- a/drivers/gpu/drm/i915/display/intel_hdcp_gsc.h +++ b/drivers/gpu/drm/i915/display/intel_hdcp_gsc.h @@ -10,12 +10,7 @@ #include <linux/types.h> struct drm_i915_private; - -struct intel_hdcp_gsc_message { - struct i915_vma *vma; - void *hdcp_cmd_in; - void *hdcp_cmd_out; -}; +struct intel_hdcp_gsc_message; bool intel_hdcp_gsc_cs_required(struct drm_i915_private *i915); ssize_t intel_hdcp_gsc_msg_send(struct drm_i915_private *i915, u8 *msg_in, -- 2.25.1 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 2/3] drm/xe/hdcp: Enable HDCP for XE 2024-02-02 8:37 [PATCH 0/3] XE HDCP Enablement Suraj Kandpal 2024-02-02 8:37 ` [PATCH 1/3] drm/i915/hdcp: Move intel_hdcp_gsc_message def away from header file Suraj Kandpal @ 2024-02-02 8:37 ` Suraj Kandpal 2024-02-03 0:41 ` kernel test robot 2024-02-05 22:50 ` Daniele Ceraolo Spurio 2024-02-02 8:37 ` [PATCH 3/3] drm/xe/hdcp: Add intel_hdcp_gsc_message to Makefile Suraj Kandpal ` (2 subsequent siblings) 4 siblings, 2 replies; 12+ messages in thread From: Suraj Kandpal @ 2024-02-02 8:37 UTC (permalink / raw) To: intel-gfx, intel-xe Cc: daniele.ceraolospurio, chaitanya.kumar.borah, ankit.k.nautiyal, Suraj Kandpal 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" #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; } -- 2.25.1 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 2/3] drm/xe/hdcp: Enable HDCP for XE 2024-02-02 8:37 ` [PATCH 2/3] drm/xe/hdcp: Enable HDCP for XE Suraj Kandpal @ 2024-02-03 0:41 ` kernel test robot 2024-02-05 22:50 ` Daniele Ceraolo Spurio 1 sibling, 0 replies; 12+ messages in thread From: kernel test robot @ 2024-02-03 0:41 UTC (permalink / raw) To: Suraj Kandpal, intel-gfx, intel-xe Cc: llvm, oe-kbuild-all, daniele.ceraolospurio, chaitanya.kumar.borah, ankit.k.nautiyal, Suraj Kandpal 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 -- 0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests/wiki ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/3] drm/xe/hdcp: Enable HDCP for XE 2024-02-02 8:37 ` [PATCH 2/3] drm/xe/hdcp: Enable HDCP for XE Suraj Kandpal 2024-02-03 0:41 ` kernel test robot @ 2024-02-05 22:50 ` Daniele Ceraolo Spurio 2024-02-06 16:24 ` Kandpal, Suraj 2024-02-07 9:40 ` Jani Nikula 1 sibling, 2 replies; 12+ messages in thread From: Daniele Ceraolo Spurio @ 2024-02-05 22:50 UTC (permalink / raw) To: Suraj Kandpal, intel-gfx, intel-xe Cc: chaitanya.kumar.borah, ankit.k.nautiyal 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; > } ^ permalink raw reply [flat|nested] 12+ messages in thread
* RE: [PATCH 2/3] drm/xe/hdcp: Enable HDCP for XE 2024-02-05 22:50 ` Daniele Ceraolo Spurio @ 2024-02-06 16:24 ` Kandpal, Suraj 2024-02-06 16:51 ` Daniele Ceraolo Spurio 2024-02-07 9:40 ` Jani Nikula 1 sibling, 1 reply; 12+ messages in thread From: Kandpal, Suraj @ 2024-02-06 16:24 UTC (permalink / raw) To: Ceraolo Spurio, Daniele, intel-gfx@lists.freedesktop.org, intel-xe@lists.freedesktop.org Cc: Borah, Chaitanya Kumar, Nautiyal, Ankit K > 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; > > } ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/3] drm/xe/hdcp: Enable HDCP for XE 2024-02-06 16:24 ` Kandpal, Suraj @ 2024-02-06 16:51 ` Daniele Ceraolo Spurio 0 siblings, 0 replies; 12+ messages in thread From: Daniele Ceraolo Spurio @ 2024-02-06 16:51 UTC (permalink / raw) To: Kandpal, Suraj, intel-gfx@lists.freedesktop.org, intel-xe@lists.freedesktop.org Cc: Borah, Chaitanya Kumar, Nautiyal, Ankit K 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; >>> } ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/3] drm/xe/hdcp: Enable HDCP for XE 2024-02-05 22:50 ` Daniele Ceraolo Spurio 2024-02-06 16:24 ` Kandpal, Suraj @ 2024-02-07 9:40 ` Jani Nikula 1 sibling, 0 replies; 12+ messages in thread From: Jani Nikula @ 2024-02-07 9:40 UTC (permalink / raw) To: Daniele Ceraolo Spurio, Suraj Kandpal, intel-gfx, intel-xe Cc: chaitanya.kumar.borah, ankit.k.nautiyal 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; >> } > -- Jani Nikula, Intel ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 3/3] drm/xe/hdcp: Add intel_hdcp_gsc_message to Makefile 2024-02-02 8:37 [PATCH 0/3] XE HDCP Enablement Suraj Kandpal 2024-02-02 8:37 ` [PATCH 1/3] drm/i915/hdcp: Move intel_hdcp_gsc_message def away from header file Suraj Kandpal 2024-02-02 8:37 ` [PATCH 2/3] drm/xe/hdcp: Enable HDCP for XE Suraj Kandpal @ 2024-02-02 8:37 ` Suraj Kandpal 2024-02-02 17:34 ` ✗ Fi.CI.SPARSE: warning for XE HDCP Enablement Patchwork 2024-02-02 18:11 ` ✗ Fi.CI.BAT: failure " Patchwork 4 siblings, 0 replies; 12+ messages in thread From: Suraj Kandpal @ 2024-02-02 8:37 UTC (permalink / raw) To: intel-gfx, intel-xe Cc: daniele.ceraolospurio, chaitanya.kumar.borah, ankit.k.nautiyal, Suraj Kandpal Add intel_hdcp_gsc_message to Makefile and add corresponding changes to xe_hdcp_gsc.c to make it build. Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com> --- drivers/gpu/drm/xe/Makefile | 1 + drivers/gpu/drm/xe/display/xe_hdcp_gsc.c | 19 +++++++++++++++++++ 2 files changed, 20 insertions(+) diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile index c531210695db..2b654c908ff3 100644 --- a/drivers/gpu/drm/xe/Makefile +++ b/drivers/gpu/drm/xe/Makefile @@ -254,6 +254,7 @@ xe-$(CONFIG_DRM_XE_DISPLAY) += \ i915-display/intel_global_state.o \ i915-display/intel_gmbus.o \ i915-display/intel_hdcp.o \ + i915-display/intel_hdcp_gsc_message.o \ i915-display/intel_hdmi.o \ i915-display/intel_hotplug.o \ i915-display/intel_hotplug_irq.o \ diff --git a/drivers/gpu/drm/xe/display/xe_hdcp_gsc.c b/drivers/gpu/drm/xe/display/xe_hdcp_gsc.c index eca941d7b281..549108060613 100644 --- a/drivers/gpu/drm/xe/display/xe_hdcp_gsc.c +++ b/drivers/gpu/drm/xe/display/xe_hdcp_gsc.c @@ -3,9 +3,12 @@ * Copyright 2023, Intel Corporation. */ +#include <drm/i915_hdcp_interface.h> + #include "abi/gsc_command_header_abi.h" #include "i915_drv.h" #include "intel_hdcp_gsc.h" +#include "intel_hdcp_gsc_message.h" #include "xe_bo.h" #include "xe_map.h" #include "xe_gsc_submit.h" @@ -97,6 +100,22 @@ static int intel_hdcp_gsc_hdcp2_init(struct drm_i915_private *i915) return ret; } +static const struct i915_hdcp_ops gsc_hdcp_ops = { + .initiate_hdcp2_session = intel_hdcp_gsc_initiate_session, + .verify_receiver_cert_prepare_km = + intel_hdcp_gsc_verify_receiver_cert_prepare_km, + .verify_hprime = intel_hdcp_gsc_verify_hprime, + .store_pairing_info = intel_hdcp_gsc_store_pairing_info, + .initiate_locality_check = intel_hdcp_gsc_initiate_locality_check, + .verify_lprime = intel_hdcp_gsc_verify_lprime, + .get_session_key = intel_hdcp_gsc_get_session_key, + .repeater_check_flow_prepare_ack = + intel_hdcp_gsc_repeater_check_flow_prepare_ack, + .verify_mprime = intel_hdcp_gsc_verify_mprime, + .enable_hdcp_authentication = intel_hdcp_gsc_enable_authentication, + .close_hdcp_session = intel_hdcp_gsc_close_session, +}; + int intel_hdcp_gsc_init(struct drm_i915_private *i915) { struct i915_hdcp_arbiter *data; -- 2.25.1 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* ✗ Fi.CI.SPARSE: warning for XE HDCP Enablement 2024-02-02 8:37 [PATCH 0/3] XE HDCP Enablement Suraj Kandpal ` (2 preceding siblings ...) 2024-02-02 8:37 ` [PATCH 3/3] drm/xe/hdcp: Add intel_hdcp_gsc_message to Makefile Suraj Kandpal @ 2024-02-02 17:34 ` Patchwork 2024-02-02 18:11 ` ✗ Fi.CI.BAT: failure " Patchwork 4 siblings, 0 replies; 12+ messages in thread From: Patchwork @ 2024-02-02 17:34 UTC (permalink / raw) To: Suraj Kandpal; +Cc: intel-gfx == Series Details == Series: XE HDCP Enablement URL : https://patchwork.freedesktop.org/series/129456/ State : warning == Summary == Error: dim sparse failed Sparse version: v0.6.2 Fast mode used, each commit won't be checked separately. - +./arch/x86/include/asm/bitops.h:116:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:116:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:147:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:147:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:149:9: warning: unreplaced symbol 'oldbit' +./arch/x86/include/asm/bitops.h:149:9: warning: unreplaced symbol 'oldbit' +./arch/x86/include/asm/bitops.h:153:26: warning: unreplaced symbol 'oldbit' +./arch/x86/include/asm/bitops.h:153:26: warning: unreplaced symbol 'oldbit' +./arch/x86/include/asm/bitops.h:155:16: warning: unreplaced symbol 'oldbit' +./arch/x86/include/asm/bitops.h:155:16: warning: unreplaced symbol 'oldbit' +./arch/x86/include/asm/bitops.h:155:9: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:155:9: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:173:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:173:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:175:9: warning: unreplaced symbol 'oldbit' +./arch/x86/include/asm/bitops.h:175:9: warning: unreplaced symbol 'oldbit' +./arch/x86/include/asm/bitops.h:179:35: warning: unreplaced symbol 'oldbit' +./arch/x86/include/asm/bitops.h:179:35: warning: unreplaced symbol 'oldbit' +./arch/x86/include/asm/bitops.h:181:16: warning: unreplaced symbol 'oldbit' +./arch/x86/include/asm/bitops.h:181:16: warning: unreplaced symbol 'oldbit' +./arch/x86/include/asm/bitops.h:181:9: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:181:9: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:185:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:185:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:187:9: warning: unreplaced symbol 'oldbit' +./arch/x86/include/asm/bitops.h:187:9: warning: unreplaced symbol 'oldbit' +./arch/x86/include/asm/bitops.h:191:35: warning: unreplaced symbol 'oldbit' +./arch/x86/include/asm/bitops.h:191:35: warning: unreplaced symbol 'oldbit' +./arch/x86/include/asm/bitops.h:194:16: warning: unreplaced symbol 'oldbit' +./arch/x86/include/asm/bitops.h:194:16: warning: unreplaced symbol 'oldbit' +./arch/x86/include/asm/bitops.h:194:9: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:194:9: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:236:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:236:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:238:9: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:238:9: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:66:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:66:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:92:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:92:1: warning: unreplaced symbol 'return' +./include/asm-generic/bitops/generic-non-atomic.h:100:17: warning: unreplaced symbol 'old' +./include/asm-generic/bitops/generic-non-atomic.h:100:17: warning: unreplaced symbol 'old' +./include/asm-generic/bitops/generic-non-atomic.h:100:23: warning: unreplaced symbol 'mask' +./include/asm-generic/bitops/generic-non-atomic.h:100:23: warning: unreplaced symbol 'mask' +./include/asm-generic/bitops/generic-non-atomic.h:100:9: warning: unreplaced symbol 'return' +./include/asm-generic/bitops/generic-non-atomic.h:100:9: warning: unreplaced symbol 'return' +./include/asm-generic/bitops/generic-non-atomic.h:105:1: warning: unreplaced symbol 'return' +./include/asm-generic/bitops/generic-non-atomic.h:105:1: warning: unreplaced symbol 'return' +./include/asm-generic/bitops/generic-non-atomic.h:107:9: warning: unreplaced symbol 'mask' +./include/asm-generic/bitops/generic-non-atomic.h:107:9: warning: unreplaced symbol 'mask' +./include/asm-generic/bitops/generic-non-atomic.h:108:9: warning: unreplaced symbol 'p' +./include/asm-generic/bitops/generic-non-atomic.h:108:9: warning: unreplaced symbol 'p' +./include/asm-generic/bitops/generic-non-atomic.h:109:9: warning: unreplaced symbol 'old' +./include/asm-generic/bitops/generic-non-atomic.h:109:9: warning: unreplaced symbol 'old' +./include/asm-generic/bitops/generic-non-atomic.h:111:10: warning: unreplaced symbol 'p' +./include/asm-generic/bitops/generic-non-atomic.h:111:10: warning: unreplaced symbol 'p' +./include/asm-generic/bitops/generic-non-atomic.h:111:14: warning: unreplaced symbol 'old' +./include/asm-generic/bitops/generic-non-atomic.h:111:14: warning: unreplaced symbol 'old' +./include/asm-generic/bitops/generic-non-atomic.h:111:20: warning: unreplaced symbol 'mask' +./include/asm-generic/bitops/generic-non-atomic.h:111:20: warning: unreplaced symbol 'mask' +./include/asm-generic/bitops/generic-non-atomic.h:112:17: warning: unreplaced symbol 'old' +./include/asm-generic/bitops/generic-non-atomic.h:112:17: warning: unreplaced symbol 'old' +./include/asm-generic/bitops/generic-non-atomic.h:112:23: warning: unreplaced symbol 'mask' +./include/asm-generic/bitops/generic-non-atomic.h:112:23: warning: unreplaced symbol 'mask' +./include/asm-generic/bitops/generic-non-atomic.h:112:9: warning: unreplaced symbol 'return' +./include/asm-generic/bitops/generic-non-atomic.h:112:9: warning: unreplaced symbol 'return' +./include/asm-generic/bitops/generic-non-atomic.h:121:1: warning: unreplaced symbol 'return' +./include/asm-generic/bitops/generic-non-atomic.h:121:1: warning: unreplaced symbol 'return' +./include/asm-generic/bitops/generic-non-atomic.h:128:9: warning: unreplaced symbol 'return' +./include/asm-generic/bitops/generic-non-atomic.h:128:9: warning: unreplaced symbol 'return' +./include/asm-generic/bitops/generic-non-atomic.h:166:1: warning: unreplaced symbol 'return' +./include/asm-generic/bitops/generic-non-atomic.h:166:1: warning: unreplaced symbol 'return' +./include/asm-generic/bitops/generic-non-atomic.h:168:9: warning: unreplaced symbol 'p' +./include/asm-generic/bitops/generic-non-atomic.h:168:9: warning: unreplaced symbol 'p' +./include/asm-generic/bitops/generic-non-atomic.h:169:9: warning: unreplaced symbol 'mask' +./include/asm-generic/bitops/generic-non-atomic.h:169:9: warning: unreplaced symbol 'mask' +./include/asm-generic/bitops/generic-non-atomic.h:170:9: warning: unreplaced symbol 'val' +./include/asm-generic/bitops/generic-non-atomic.h:170:9: warning: unreplaced symbol 'val' +./include/asm-generic/bitops/generic-non-atomic.h:172:19: warning: unreplaced symbol 'val' +./include/asm-generic/bitops/generic-non-atomic.h:172:19: warning: unreplaced symbol 'val' +./include/asm-generic/bitops/generic-non-atomic.h:172:25: warning: unreplaced symbol 'mask' +./include/asm-generic/bitops/generic-non-atomic.h:172:25: warning: unreplaced symbol 'mask' +./include/asm-generic/bitops/generic-non-atomic.h:172:9: warning: unreplaced symbol 'return' +./include/asm-generic/bitops/generic-non-atomic.h:172:9: warning: unreplaced symbol 'return' +./include/asm-generic/bitops/generic-non-atomic.h:28:1: warning: unreplaced symbol 'return' +./include/asm-generic/bitops/generic-non-atomic.h:28:1: warning: unreplaced symbol 'return' +./include/asm-generic/bitops/generic-non-atomic.h:30:9: warning: unreplaced symbol 'mask' +./include/asm-generic/bitops/generic-non-atomic.h:30:9: warning: unreplaced symbol 'mask' +./include/asm-generic/bitops/generic-non-atomic.h:31:9: warning: unreplaced symbol 'p' +./include/asm-generic/bitops/generic-non-atomic.h:31:9: warning: unreplaced symbol 'p' +./include/asm-generic/bitops/generic-non-atomic.h:33:10: warning: unreplaced symbol 'p' +./include/asm-generic/bitops/generic-non-atomic.h:33:10: warning: unreplaced symbol 'p' +./include/asm-generic/bitops/generic-non-atomic.h:33:16: warning: unreplaced symbol 'mask' +./include/asm-generic/bitops/generic-non-atomic.h:33:16: warning: unreplaced symbol 'mask' +./include/asm-generic/bitops/generic-non-atomic.h:37:1: warning: unreplaced symbol 'return' +./include/asm-generic/bitops/generic-non-atomic.h:37:1: warning: unreplaced symbol 'return' +./include/asm-generic/bitops/generic-non-atomic.h:39:9: warning: unreplaced symbol 'mask' +./include/asm-generic/bitops/generic-non-atomic.h:39:9: warning: unreplaced symbol 'mask' +./include/asm-generic/bitops/generic-non-atomic.h:40:9: warning: unreplaced symbol 'p' +./include/asm-generic/bitops/generic-non-atomic.h:40:9: warning: unreplaced symbol 'p' +./include/asm-generic/bitops/generic-non-atomic.h:42:10: warning: unreplaced symbol 'p' +./include/asm-generic/bitops/generic-non-atomic.h:42:10: warning: unreplaced symbol 'p' +./include/asm-generic/bitops/generic-non-atomic.h:42:16: warning: unreplaced symbol 'mask' +./include/asm-generic/bitops/generic-non-atomic.h:42:16: warning: unreplaced symbol 'mask' +./include/asm-generic/bitops/generic-non-atomic.h:55:1: warning: unreplaced symbol 'return' +./include/asm-generic/bitops/generic-non-atomic.h:55:1: warning: unreplaced symbol 'return' +./include/asm-generic/bitops/generic-non-atomic.h:57:9: warning: unreplaced symbol 'mask' +./include/asm-generic/bitops/generic-non-atomic.h:57:9: warning: unreplaced symbol 'mask' +./include/asm-generic/bitops/generic-non-atomic.h:58:9: warning: unreplaced symbol 'p' +./include/asm-generic/bitops/generic-non-atomic.h:58:9: warning: unreplaced symbol 'p' +./include/asm-generic/bitops/generic-non-atomic.h:60:10: warning: unreplaced symbol 'p' +./include/asm-generic/bitops/generic-non-atomic.h:60:10: warning: unreplaced symbol 'p' +./include/asm-generic/bitops/generic-non-atomic.h:60:15: warning: unreplaced symbol 'mask' +./include/asm-generic/bitops/generic-non-atomic.h:60:15: warning: unreplaced symbol 'mask' +./include/asm-generic/bitops/generic-non-atomic.h:73:1: warning: unreplaced symbol 'return' +./include/asm-generic/bitops/generic-non-atomic.h:73:1: warning: unreplaced symbol 'return' +./include/asm-generic/bitops/generic-non-atomic.h:75:9: warning: unreplaced symbol 'mask' +./include/asm-generic/bitops/generic-non-atomic.h:75:9: warning: unreplaced symbol 'mask' +./include/asm-generic/bitops/generic-non-atomic.h:76:9: warning: unreplaced symbol 'p' +./include/asm-generic/bitops/generic-non-atomic.h:76:9: warning: unreplaced symbol 'p' +./include/asm-generic/bitops/generic-non-atomic.h:77:9: warning: unreplaced symbol 'old' +./include/asm-generic/bitops/generic-non-atomic.h:77:9: warning: unreplaced symbol 'old' +./include/asm-generic/bitops/generic-non-atomic.h:79:10: warning: unreplaced symbol 'p' +./include/asm-generic/bitops/generic-non-atomic.h:79:10: warning: unreplaced symbol 'p' +./include/asm-generic/bitops/generic-non-atomic.h:79:14: warning: unreplaced symbol 'old' +./include/asm-generic/bitops/generic-non-atomic.h:79:14: warning: unreplaced symbol 'old' +./include/asm-generic/bitops/generic-non-atomic.h:79:20: warning: unreplaced symbol 'mask' +./include/asm-generic/bitops/generic-non-atomic.h:79:20: warning: unreplaced symbol 'mask' +./include/asm-generic/bitops/generic-non-atomic.h:80:17: warning: unreplaced symbol 'old' +./include/asm-generic/bitops/generic-non-atomic.h:80:17: warning: unreplaced symbol 'old' +./include/asm-generic/bitops/generic-non-atomic.h:80:23: warning: unreplaced symbol 'mask' +./include/asm-generic/bitops/generic-non-atomic.h:80:23: warning: unreplaced symbol 'mask' +./include/asm-generic/bitops/generic-non-atomic.h:80:9: warning: unreplaced symbol 'return' +./include/asm-generic/bitops/generic-non-atomic.h:80:9: warning: unreplaced symbol 'return' +./include/asm-generic/bitops/generic-non-atomic.h:93:1: warning: unreplaced symbol 'return' +./include/asm-generic/bitops/generic-non-atomic.h:93:1: warning: unreplaced symbol 'return' +./include/asm-generic/bitops/generic-non-atomic.h:95:9: warning: unreplaced symbol 'mask' +./include/asm-generic/bitops/generic-non-atomic.h:95:9: warning: unreplaced symbol 'mask' +./include/asm-generic/bitops/generic-non-atomic.h:96:9: warning: unreplaced symbol 'p' +./include/asm-generic/bitops/generic-non-atomic.h:96:9: warning: unreplaced symbol 'p' +./include/asm-generic/bitops/generic-non-atomic.h:97:9: warning: unreplaced symbol 'old' +./include/asm-generic/bitops/generic-non-atomic.h:97:9: warning: unreplaced symbol 'old' +./include/asm-generic/bitops/generic-non-atomic.h:99:10: warning: unreplaced symbol 'p' +./include/asm-generic/bitops/generic-non-atomic.h:99:10: warning: unreplaced symbol 'p' +./include/asm-generic/bitops/generic-non-atomic.h:99:14: warning: unreplaced symbol 'old' +./include/asm-generic/bitops/generic-non-atomic.h:99:14: warning: unreplaced symbol 'old' +./include/asm-generic/bitops/generic-non-atomic.h:99:21: warning: unreplaced symbol 'mask' +./include/asm-generic/bitops/generic-non-atomic.h:99:21: warning: unreplaced symbol 'mask' +./include/asm-generic/bitops/instrumented-non-atomic.h:100:9: warning: unreplaced symbol 'return' +./include/asm-generic/bitops/instrumented-non-atomic.h:100:9: warning: unreplaced symbol 'return' +./include/asm-generic/bitops/instrumented-non-atomic.h:112:1: warning: unreplaced symbol 'return' +./include/asm-generic/bitops/instrumented-non-atomic.h:112:1: warning: unreplaced symbol 'return' +./include/asm-generic/bitops/instrumented-non-atomic.h:115:9: warning: unreplaced symbol 'return' +./include/asm-generic/bitops/instrumented-non-atomic.h:115:9: warning: unreplaced symbol 'return' +./include/asm-generic/bitops/instrumented-non-atomic.h:127:1: warning: unreplaced symbol 'return' +./include/asm-generic/bitops/instrumented-non-atomic.h:127:1: warning: unreplaced symbol 'return' +./include/asm-generic/bitops/instrumented-non-atomic.h:130:9: warning: unreplaced symbol 'return' +./include/asm-generic/bitops/instrumented-non-atomic.h:130:9: warning: unreplaced symbol 'return' +./include/asm-generic/bitops/instrumented-non-atomic.h:139:1: warning: unreplaced symbol 'return' +./include/asm-generic/bitops/instrumented-non-atomic.h:139:1: warning: unreplaced symbol 'return' +./include/asm-generic/bitops/instrumented-non-atomic.h:142:9: warning: unreplaced symbol 'return' +./include/asm-generic/bitops/instrumented-non-atomic.h:142:9: warning: unreplaced symbol 'return' +./include/asm-generic/bitops/instrumented-non-atomic.h:26:1: warning: unreplaced symbol 'return' +./include/asm-generic/bitops/instrumented-non-atomic.h:26:1: warning: unreplaced symbol 'return' +./include/asm-generic/bitops/instrumented-non-atomic.h:42:1: warning: unreplaced symbol 'return' +./include/asm-generic/bitops/instrumented-non-atomic.h:42:1: warning: unreplaced symbol 'return' +./include/asm-generic/bitops/instrumented-non-atomic.h:58:1: warning: unreplaced symbol 'return' +./include/asm-generic/bitops/instrumented-non-atomic.h:58:1: warning: unreplaced symbol 'return' +./include/asm-generic/bitops/instrumented-non-atomic.h:97:1: warning: unreplaced symbol 'return' +./include/asm-generic/bitops/instrumented-non-atomic.h:97:1: warning: unreplaced symbol 'return' ^ permalink raw reply [flat|nested] 12+ messages in thread
* ✗ Fi.CI.BAT: failure for XE HDCP Enablement 2024-02-02 8:37 [PATCH 0/3] XE HDCP Enablement Suraj Kandpal ` (3 preceding siblings ...) 2024-02-02 17:34 ` ✗ Fi.CI.SPARSE: warning for XE HDCP Enablement Patchwork @ 2024-02-02 18:11 ` Patchwork 4 siblings, 0 replies; 12+ messages in thread From: Patchwork @ 2024-02-02 18:11 UTC (permalink / raw) To: Suraj Kandpal; +Cc: intel-gfx [-- Attachment #1: Type: text/plain, Size: 4178 bytes --] == Series Details == Series: XE HDCP Enablement URL : https://patchwork.freedesktop.org/series/129456/ State : failure == Summary == CI Bug Log - changes from CI_DRM_14212 -> Patchwork_129456v1 ==================================================== Summary ------- **FAILURE** Serious unknown changes coming with Patchwork_129456v1 absolutely need to be verified manually. If you think the reported changes have nothing to do with the changes introduced in Patchwork_129456v1, please notify your bug team (I915-ci-infra@lists.freedesktop.org) to allow them to document this new failure mode, which will reduce false positives in CI. External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129456v1/index.html Participating hosts (38 -> 35) ------------------------------ Additional (1): fi-bsw-n3050 Missing (4): bat-mtlp-8 bat-arls-2 fi-snb-2520m fi-pnv-d510 Possible new issues ------------------- Here are the unknown changes that may have been introduced in Patchwork_129456v1: ### IGT changes ### #### Possible regressions #### * igt@kms_flip@basic-flip-vs-dpms@d-edp1: - bat-adlp-6: [PASS][1] -> [INCOMPLETE][2] [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14212/bat-adlp-6/igt@kms_flip@basic-flip-vs-dpms@d-edp1.html [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129456v1/bat-adlp-6/igt@kms_flip@basic-flip-vs-dpms@d-edp1.html Known issues ------------ Here are the changes found in Patchwork_129456v1 that come from known issues: ### CI changes ### #### Issues hit #### * boot: - bat-jsl-1: [PASS][3] -> [FAIL][4] ([i915#8293]) [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14212/bat-jsl-1/boot.html [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129456v1/bat-jsl-1/boot.html ### IGT changes ### #### Issues hit #### * igt@gem_lmem_swapping@random-engines: - fi-bsw-n3050: NOTRUN -> [SKIP][5] ([fdo#109271]) +15 other tests skip [5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129456v1/fi-bsw-n3050/igt@gem_lmem_swapping@random-engines.html #### Possible fixes #### * igt@i915_selftest@live@gt_pm: - bat-adln-1: [DMESG-FAIL][6] ([i915#10010]) -> [PASS][7] [6]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14212/bat-adln-1/igt@i915_selftest@live@gt_pm.html [7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129456v1/bat-adln-1/igt@i915_selftest@live@gt_pm.html #### Warnings #### * igt@i915_module_load@reload: - fi-kbl-7567u: [DMESG-WARN][8] ([i915#180] / [i915#8585]) -> [DMESG-WARN][9] ([i915#180] / [i915#1982] / [i915#8585]) [8]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14212/fi-kbl-7567u/igt@i915_module_load@reload.html [9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129456v1/fi-kbl-7567u/igt@i915_module_load@reload.html {name}: This element is suppressed. This means it is ignored when computing the status of the difference (SUCCESS, WARNING, or FAILURE). [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271 [i915#10010]: https://gitlab.freedesktop.org/drm/intel/issues/10010 [i915#180]: https://gitlab.freedesktop.org/drm/intel/issues/180 [i915#1982]: https://gitlab.freedesktop.org/drm/intel/issues/1982 [i915#8293]: https://gitlab.freedesktop.org/drm/intel/issues/8293 [i915#8585]: https://gitlab.freedesktop.org/drm/intel/issues/8585 Build changes ------------- * Linux: CI_DRM_14212 -> Patchwork_129456v1 CI-20190529: 20190529 CI_DRM_14212: 1dd92467500a5ead3e44bbdfe15e064dd79b65ef @ git://anongit.freedesktop.org/gfx-ci/linux IGT_7702: bf075a74ece1956fc0e554291591b9da3eab54cf @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git Patchwork_129456v1: 1dd92467500a5ead3e44bbdfe15e064dd79b65ef @ git://anongit.freedesktop.org/gfx-ci/linux ### Linux commits 2227076489a5 drm/xe/hdcp: Add intel_hdcp_gsc_message to Makefile e2dc59dfb1d1 drm/xe/hdcp: Enable HDCP for XE 8b3a1e157076 drm/i915/hdcp: Move intel_hdcp_gsc_message def away from header file == Logs == For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129456v1/index.html [-- Attachment #2: Type: text/html, Size: 5040 bytes --] ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 0/3] XE HDCP Enablement @ 2024-02-06 16:47 Suraj Kandpal 2024-02-06 16:47 ` [PATCH 2/3] drm/xe/hdcp: Enable HDCP for XE Suraj Kandpal 0 siblings, 1 reply; 12+ messages in thread From: Suraj Kandpal @ 2024-02-06 16:47 UTC (permalink / raw) To: intel-gfx, intel-xe Cc: daniele.ceraolospurio, chaitanya.kumar.borah, ankit.k.nautiyal, Suraj Kandpal This patch series enables HDCP on XE. Mainly includes rewriting functions that were responsible for sending hdcp messages via gsc cs. Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com> Suraj Kandpal (3): drm/i915/hdcp: Move intel_hdcp_gsc_message def away from header file drm/xe/hdcp: Enable HDCP for XE drm/xe/hdcp: Add intel_hdcp_gsc_message to Makefile drivers/gpu/drm/i915/display/intel_hdcp_gsc.c | 10 +- drivers/gpu/drm/i915/display/intel_hdcp_gsc.h | 7 +- drivers/gpu/drm/xe/Makefile | 1 + .../gpu/drm/xe/abi/gsc_command_header_abi.h | 2 + drivers/gpu/drm/xe/display/xe_hdcp_gsc.c | 197 +++++++++++++++++- drivers/gpu/drm/xe/xe_gsc_submit.c | 19 ++ drivers/gpu/drm/xe/xe_gsc_submit.h | 1 + 7 files changed, 225 insertions(+), 12 deletions(-) -- 2.25.1 ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 2/3] drm/xe/hdcp: Enable HDCP for XE 2024-02-06 16:47 [PATCH 0/3] " Suraj Kandpal @ 2024-02-06 16:47 ` Suraj Kandpal 0 siblings, 0 replies; 12+ messages in thread From: Suraj Kandpal @ 2024-02-06 16:47 UTC (permalink / raw) To: intel-gfx, intel-xe Cc: daniele.ceraolospurio, chaitanya.kumar.borah, ankit.k.nautiyal, Suraj Kandpal Enable HDCP for Xe by defining functions which take care of interaction of HDCP as a client with the GSC CS interface. --v2 -add kfree at appropriate place [Daniele] -forward declare drm_i915_private [Daniele] -remove useless define [Daniele] -move host session logic to xe_gsc_submit.c [Daniele] -call xe_gsc_check_and_update_pending directly in an if condition [Daniele] Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com> --- drivers/gpu/drm/i915/display/intel_hdcp_gsc.c | 4 +- .../gpu/drm/xe/abi/gsc_command_header_abi.h | 2 + drivers/gpu/drm/xe/display/xe_hdcp_gsc.c | 180 +++++++++++++++++- drivers/gpu/drm/xe/xe_gsc_submit.c | 19 ++ drivers/gpu/drm/xe/xe_gsc_submit.h | 1 + 5 files changed, 200 insertions(+), 6 deletions(-) diff --git a/drivers/gpu/drm/i915/display/intel_hdcp_gsc.c b/drivers/gpu/drm/i915/display/intel_hdcp_gsc.c index e44f60f00e8b..9e895f714f90 100644 --- a/drivers/gpu/drm/i915/display/intel_hdcp_gsc.c +++ b/drivers/gpu/drm/i915/display/intel_hdcp_gsc.c @@ -123,8 +123,10 @@ static int intel_hdcp_gsc_hdcp2_init(struct drm_i915_private *i915) i915->display.hdcp.hdcp_message = hdcp_message; ret = intel_hdcp_gsc_initialize_message(i915, hdcp_message); - if (ret) + if (ret) { drm_err(&i915->drm, "Could not initialize hdcp_message\n"); + kfree(hdcp_message); + } return ret; } diff --git a/drivers/gpu/drm/xe/abi/gsc_command_header_abi.h b/drivers/gpu/drm/xe/abi/gsc_command_header_abi.h index a4c2646803b5..d2fbf71439be 100644 --- a/drivers/gpu/drm/xe/abi/gsc_command_header_abi.h +++ b/drivers/gpu/drm/xe/abi/gsc_command_header_abi.h @@ -21,6 +21,8 @@ struct intel_gsc_mtl_header { /* FW allows host to decide host_session handle as it sees fit. */ u64 host_session_handle; +#define HECI_MEADDRESS_PXP 17 +#define HECI_MEADDRESS_HDCP 18 /* handle generated by FW for messages that need to be re-submitted */ u64 gsc_message_handle; diff --git a/drivers/gpu/drm/xe/display/xe_hdcp_gsc.c b/drivers/gpu/drm/xe/display/xe_hdcp_gsc.c index 0f11a39333e2..dbcd10617a1b 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 "i915_drv.h" +#include <linux/delay.h> + +#include "abi/gsc_command_header_abi.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 drm_i915_private; +struct intel_hdcp_gsc_message { + struct xe_bo *hdcp_bo; + u64 hdcp_cmd_in; + u64 hdcp_cmd_out; +}; + +#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,176 @@ 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); + 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); + if (ret) + kfree(data); + + 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_out_off) +{ + 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; + } + + if (xe_gsc_check_and_update_pending(i915, map, 0, map, addr_out_off)) + 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_wr_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 = xe_gsc_get_host_session_id(HECI_MEADDRESS_HDCP); + xe_device_mem_access_get(i915); + addr_in_wr_off = xe_gsc_emit_header(i915, &hdcp_message->hdcp_bo->vmap, + addr_in_wr_off, HECI_MEADDRESS_HDCP, + host_session_id, msg_in_len); + xe_map_memcpy_to(i915, &hdcp_message->hdcp_bo->vmap, addr_in_wr_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_out_off); + + /* 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; } diff --git a/drivers/gpu/drm/xe/xe_gsc_submit.c b/drivers/gpu/drm/xe/xe_gsc_submit.c index 348994b271be..57793b0acfc3 100644 --- a/drivers/gpu/drm/xe/xe_gsc_submit.c +++ b/drivers/gpu/drm/xe/xe_gsc_submit.c @@ -33,6 +33,7 @@ * include the client id in the top 8 bits of the handle. */ #define HOST_SESSION_CLIENT_MASK GENMASK_ULL(63, 56) +#define HOST_SESSION_PXP_SINGLE BIT_ULL(60) static struct xe_gt * gsc_to_gt(struct xe_gsc *gsc) @@ -40,6 +41,24 @@ gsc_to_gt(struct xe_gsc *gsc) return container_of(gsc, struct xe_gt, uc.gsc); } +/** + * xe_gsc_get_host_session_id - Create host session id based on HECI + * client address + * @heci_client_id: client id identifying the type of command (see abi for values) + * + * Returns: random host_session_id which can be used to send messages to gsc cs + */ +u64 xe_gsc_get_host_session_id(u8 heci_client_id) +{ + u64 host_session_id; + + get_random_bytes(&host_session_id, sizeof(u64)); + host_session_id &= ~HOST_SESSION_CLIENT_MASK; + if (host_session_id && heci_client_id == HECI_MEADDRESS_PXP) + host_session_id |= HOST_SESSION_PXP_SINGLE; + return host_session_id; +}; + /** * xe_gsc_emit_header - write the MTL GSC header in memory * @xe: the Xe device diff --git a/drivers/gpu/drm/xe/xe_gsc_submit.h b/drivers/gpu/drm/xe/xe_gsc_submit.h index 1939855031a6..f3359b6659b8 100644 --- a/drivers/gpu/drm/xe/xe_gsc_submit.h +++ b/drivers/gpu/drm/xe/xe_gsc_submit.h @@ -28,4 +28,5 @@ int xe_gsc_read_out_header(struct xe_device *xe, int xe_gsc_pkt_submit_kernel(struct xe_gsc *gsc, u64 addr_in, u32 size_in, u64 addr_out, u32 size_out); +u64 xe_gsc_get_host_session_id(u8 heci_client_id); #endif -- 2.25.1 ^ permalink raw reply related [flat|nested] 12+ messages in thread
end of thread, other threads:[~2024-02-07 9:40 UTC | newest] Thread overview: 12+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-02-02 8:37 [PATCH 0/3] XE HDCP Enablement Suraj Kandpal 2024-02-02 8:37 ` [PATCH 1/3] drm/i915/hdcp: Move intel_hdcp_gsc_message def away from header file Suraj Kandpal 2024-02-02 8:37 ` [PATCH 2/3] drm/xe/hdcp: Enable HDCP for XE Suraj Kandpal 2024-02-03 0:41 ` kernel test robot 2024-02-05 22:50 ` Daniele Ceraolo Spurio 2024-02-06 16:24 ` Kandpal, Suraj 2024-02-06 16:51 ` Daniele Ceraolo Spurio 2024-02-07 9:40 ` Jani Nikula 2024-02-02 8:37 ` [PATCH 3/3] drm/xe/hdcp: Add intel_hdcp_gsc_message to Makefile Suraj Kandpal 2024-02-02 17:34 ` ✗ Fi.CI.SPARSE: warning for XE HDCP Enablement Patchwork 2024-02-02 18:11 ` ✗ Fi.CI.BAT: failure " Patchwork -- strict thread matches above, loose matches on Subject: below -- 2024-02-06 16:47 [PATCH 0/3] " Suraj Kandpal 2024-02-06 16:47 ` [PATCH 2/3] drm/xe/hdcp: Enable HDCP for XE Suraj Kandpal
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).