[Intel-gfx] [PATCH v4 6/7] drm/i915/mtl: Add function to send command to GSC CS

[Kandpal, Suraj]

> 
> 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(&gt->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(&i915->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(&i915->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.

From my discussion with Daniele offline while I prepared this patch for internal I was told session
Cleanup need not be done by hdcp hence preservation of session bit too  is not necessary

Regards,
Suraj Kandpal
> 
> Alan:[snip]