Re: [Intel-gfx] [PATCH v4 6/7] drm/i915/mtl: Add function to send command to GSC CS
I've send offline email to clear up session cleanup behavior for hdcp (maybe its a PXP-only thing). But still wanna pursue a bit more on below portion: On Fri, 2022-12-23 at 09:07 +, Kandpal, Suraj wrote: > > > > > > > + memset(cmd, 0, obj->base.size); > > > > Alan: question: how often is this hdcp message being created, pinned,cleared > > and used to send message? > > Is this very infrequent - such as only during initial port connection > > establishment > > or if in unlikely cases of dp/hdmi link-disruption.. > > Or is intel_hdcp_check_work something that has to exercise these gsc > > messages > > frequently (such as every few seconds)? > > It is more frequent so during the link estabilishment and then every few > seconds > To check if link in working right through check work but saving message in > hdcp > In intel hdcp does not seem right as message that need to be sent vary in > size. > ,creation pinning and clearance makes it simpler and cleaner. > > > > I am just wondering if its the latter, whether its more efficient to > > initialize and > > store the hdcp_message structure into intel_hdcp if hdcp on connector is > > enabled and freed (intel_free_hdcp_gsc_message) only at port disabling time? > > Also, in any case, would the entire object need to be memset? (I'm > > wondering if > > we only need to memset the mtl-gsc-header and leave the rest since gsc hdcp > > service is able to determine the range of valid bytes based on the hdcp > > command > > and thus we don't need to memset the entire object) > > > > Alan:[snip] > > > > memsetting of the entire object would be required as the range of valid bytes > can change depending on > Various use cases one example would be if Km is already stored in fw or not > this keeps it > more cleaner and less prone to error > > 1. Is there a max size for the hdcp communications throughout the various types of message packets through out the various state-machine stages of the hdcp link life-cycle? If there is a deterministic max size, we can allocate that up front and keep re-using the same object. 2. Is the "range of valid bytes" consistent for the same HDCP-command? Based on patch #5 it looks that way. For exmaple, if the heci packet contained HDCP command HDCP_CMD_WIRED_AAA, then do we know for sure that command AAA will always have a specific fixed-size structure (size of struct aaa)? And for command HDCP_CMD_WIRED_BBB, it is a different structure but also has a fixed-size (size of struct bbb). If this the case, then we dont need to memset the entire object, we only need to ensure the memory size that corresponds to the structure of the specific HDCP command is initialized correctly - i.e. ensure we fill up every dword in that structure (and not need a memset for the full 4K - or whatever buffer size was allocated?). I'm assuming the security-firmware hdcp-service would only read data up until the expected fixed-size struct depending on the command.
Re: [Intel-gfx] [PATCH v4 6/7] drm/i915/mtl: Add function to send command to GSC CS
>
> Most of it looks good - but I have two issues:
> 1. I believe there are some reuse efficiency gaps depending on the frequency
> of
> these hdcp-gsc messages... but it might not be an issue depending on the
> answers to the questions i have below.
> 2. Missing the session-cleanup request
>
> ...alan
>
>
> On Thu, 2022-12-22 at 12:13 +0530, Suraj Kandpal wrote:
> >
> Alan:[snip]
> > +/*This function helps allocate memory for the command that we will
> > +send to gsc cs */ static int intel_initialize_hdcp_gsc_message(struct
> drm_i915_private *i915,
> > +struct intel_hdcp_gsc_message
> *hdcp_message) {
> > + struct intel_gt *gt = i915->media_gt;
> > + struct drm_i915_gem_object *obj = NULL;
> > + struct i915_vma *vma = NULL;
> > + void *cmd;
> > + int err;
> > +
> > + hdcp_message->obj = NULL;
> > + hdcp_message->hdcp_cmd = NULL;
> > + hdcp_message->vma = NULL;
> Alan: is this unnecessary? caller is using kzalloc for this structure.
So I mimicked Daniele message initialization function that he had made for pxp
tee streaming
And it seemed like an essential step although I don’t see any risk in removing
them
>
> > + /* allocate object of one page for HDCP command memory and store it
> */
> > + obj = i915_gem_object_create_shmem(gt->i915, PAGE_SIZE);
> > +
> > + if (IS_ERR(obj)) {
> > + drm_err(>i915->drm, "Failed to allocate HDCP streaming
> > +command!\n");
> Alan: nit: 'i915' instead of 'gt->i915' Also applies for other cases in this
> function.
>
Ohh I missed that somehow will get it fixed
>
> Alan:[snip]
>
> > + memset(cmd, 0, obj->base.size);
>
> Alan: question: how often is this hdcp message being created, pinned,cleared
> and used to send message?
> Is this very infrequent - such as only during initial port connection
> establishment
> or if in unlikely cases of dp/hdmi link-disruption..
> Or is intel_hdcp_check_work something that has to exercise these gsc messages
> frequently (such as every few seconds)?
It is more frequent so during the link estabilishment and then every few seconds
To check if link in working right through check work but saving message in hdcp
In intel hdcp does not seem right as message that need to be sent vary in size.
,creation pinning and clearance makes it simpler and cleaner.
> I am just wondering if its the latter, whether its more efficient to
> initialize and
> store the hdcp_message structure into intel_hdcp if hdcp on connector is
> enabled and freed (intel_free_hdcp_gsc_message) only at port disabling time?
> Also, in any case, would the entire object need to be memset? (I'm wondering
> if
> we only need to memset the mtl-gsc-header and leave the rest since gsc hdcp
> service is able to determine the range of valid bytes based on the hdcp
> command
> and thus we don't need to memset the entire object)
>
> Alan:[snip]
>
memsetting of the entire object would be required as the range of valid bytes
can change depending on
Various use cases one example would be if Km is already stored in fw or not
this keeps it
more cleaner and less prone to error
>
>
> > +static int intel_gsc_send_sync(struct drm_i915_private *i915,
> > + struct intel_gsc_mtl_header *header, u64 addr,
> > + size_t msg_out_len)
> > +{
> > + struct intel_gt *gt = i915->media_gt;
> > + int ret;
> > +
> > + header->flags = 0;
> Alan: question: Should this be preserving the session-cleanup bit? (Im not
> familiar with the session-cleanup steps).
>
Neither am I but from my discussions with Daniele offline with
> Alan:[snip]
>
> > +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) {
>
> Alan:[snip]
>
> > + header = hdcp_message->hdcp_cmd;
> > + addr = i915_ggtt_offset(hdcp_message->vma);
> > +
> > + memset(header, 0, sizeof(*header));
>
> Alan: nit: intel_initialize_hdcp_gsc_message is already mem-setting (at least
> the
> header)
This does seem to be redundant code.
>
> Alan:[snip]
>
> > + /* we use the same mem for the reply, so header is in the same loc */
> > + reply_size = header->message_size - sizeof(*header);
> > + if (reply_size > msg_out_len) {
> > + drm_warn(>drm, "caller with insufficient HDCP reply size
> %u (%d)\n",
> > +reply_size, (u32)msg_out_len);
> > + reply_size = msg_out_len;
> > + } else if (reply_size != msg_out_len) {
> > + drm_dbg_kms(>drm, "caller unexpected HCDP reply size
> %u (%d)\n",
> > + reply_size, (u32)msg_out_len);
> > + }
> > +
> > + memcpy(msg_out, hdcp_message->hdcp_cmd + sizeof(*header),
> > +msg_out_len);
> > +
> > +err:
> > + intel_free_hdcp_gsc_message(hdcp_message);
> Alan: I believe you had not sent the session-cleanup (with zero payload)
> request
> before before freeing the objects and
Re: [Intel-gfx] [PATCH v4 6/7] drm/i915/mtl: Add function to send command to GSC CS
Most of it looks good - but I have two issues:
1. I believe there are some reuse efficiency gaps depending on the frequency
of these hdcp-gsc messages... but it might not be an issue depending on the
answers to the
questions i have below.
2. Missing the session-cleanup request
...alan
On Thu, 2022-12-22 at 12:13 +0530, Suraj Kandpal wrote:
>
Alan:[snip]
> +/*This function helps allocate memory for the command that we will send to
> gsc cs */
> +static int intel_initialize_hdcp_gsc_message(struct drm_i915_private *i915,
> + struct intel_hdcp_gsc_message
> *hdcp_message)
> +{
> + struct intel_gt *gt = i915->media_gt;
> + struct drm_i915_gem_object *obj = NULL;
> + struct i915_vma *vma = NULL;
> + void *cmd;
> + int err;
> +
> + hdcp_message->obj = NULL;
> + hdcp_message->hdcp_cmd = NULL;
> + hdcp_message->vma = NULL;
Alan: is this unnecessary? caller is using kzalloc for this structure.
> + /* allocate object of one page for HDCP command memory and store it */
> + obj = i915_gem_object_create_shmem(gt->i915, PAGE_SIZE);
> +
> + if (IS_ERR(obj)) {
> + drm_err(>i915->drm, "Failed to allocate HDCP streaming
> command!\n");
Alan: nit: 'i915' instead of 'gt->i915' Also applies for other cases in this
function.
Alan:[snip]
> + memset(cmd, 0, obj->base.size);
Alan: question: how often is this hdcp message being created, pinned,cleared
and used to send message?
Is this very infrequent - such as only during initial port connection
establishment or if in unlikely cases of dp/hdmi link-disruption..
Or is intel_hdcp_check_work something that has to exercise these gsc messages
frequently (such as every few seconds)?
I am just wondering if its the latter, whether its more efficient to initialize
and store the hdcp_message structure into intel_hdcp
if hdcp on connector is enabled and freed (intel_free_hdcp_gsc_message) only at
port disabling time?
Also, in any case, would the entire object need to be memset? (I'm wondering if
we only need to memset the mtl-gsc-header and leave
the rest since gsc hdcp service is able to determine the range of valid bytes
based on the hdcp command and thus we don't need to
memset the entire object)
Alan:[snip]
> +static int intel_gsc_send_sync(struct drm_i915_private *i915,
> +struct intel_gsc_mtl_header *header, u64 addr,
> +size_t msg_out_len)
> +{
> + struct intel_gt *gt = i915->media_gt;
> + int ret;
> +
> + header->flags = 0;
Alan: question: Should this be preserving the session-cleanup bit? (Im not
familiar with the session-cleanup steps).
Alan:[snip]
> +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)
> +{
Alan:[snip]
> + header = hdcp_message->hdcp_cmd;
> + addr = i915_ggtt_offset(hdcp_message->vma);
> +
> + memset(header, 0, sizeof(*header));
Alan: nit: intel_initialize_hdcp_gsc_message is already mem-setting (at least
the header)
Alan:[snip]
> + /* we use the same mem for the reply, so header is in the same loc */
> + reply_size = header->message_size - sizeof(*header);
> + if (reply_size > msg_out_len) {
> + drm_warn(>drm, "caller with insufficient HDCP reply size
> %u (%d)\n",
> + reply_size, (u32)msg_out_len);
> + reply_size = msg_out_len;
> + } else if (reply_size != msg_out_len) {
> + drm_dbg_kms(>drm, "caller unexpected HCDP reply size %u
> (%d)\n",
> + reply_size, (u32)msg_out_len);
> + }
> +
> + memcpy(msg_out, hdcp_message->hdcp_cmd + sizeof(*header), msg_out_len);
> +
> +err:
> + intel_free_hdcp_gsc_message(hdcp_message);
Alan: I believe you had not sent the session-cleanup (with zero payload)
request before before freeing the objects and discarding the host-session-id
that was used. I'm not
sure if the GSC firmware needs to clear resources for the hdcp services... if
so, we should either call the session-cleanup inside
intel_free_hdcp_gsc_message.
Alan:[snip]
