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

["Teres Alexis, Alan Previn" <alan.previn.teres.alexis@intel.com>]

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


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

Alan:[snip]