Re: [PATCH 2/2] drm/i915/hdcp: Modify intel_gsc_send_sync function

["Ceraolo Spurio, Daniele" <daniele.ceraolospurio@xxxxxxxxx>]

On 5/29/2023 4:49 AM, Suraj Kandpal wrote:
> Modify intel_gsc_send_sync() to take into account header_out
> and addr_out so as to use them to verify the message send status.
>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx>
> Cc: Alan Previn <alan.previn.teres.alexis@xxxxxxxxx>
> Cc: Ankit Nautiyal <ankit.k.nautiyal@xxxxxxxxx>
> Signed-off-by: Suraj Kandpal <suraj.kandpal@xxxxxxxxx>

I think this patch should be squashed with the previous one, because if 
I understand correctly the code won't work with just the first one, 
which could be a problem with bisection. The changes themselves look ok 
to me though, so for the unified patch (with the small fix I pointed out 
in the other reply):

Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx>

Daniele

> ---
>   drivers/gpu/drm/i915/display/intel_hdcp_gsc.c | 29 +++++++++++--------
>   1 file changed, 17 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_hdcp_gsc.c b/drivers/gpu/drm/i915/display/intel_hdcp_gsc.c
> index 72d1e261d0a9..5f29c3c559fa 100644
> --- a/drivers/gpu/drm/i915/display/intel_hdcp_gsc.c
> +++ b/drivers/gpu/drm/i915/display/intel_hdcp_gsc.c
> @@ -726,38 +726,42 @@ void intel_hdcp_gsc_fini(struct drm_i915_private *i915)
>   }
>   
>   static int intel_gsc_send_sync(struct drm_i915_private *i915,
> -			       struct intel_gsc_mtl_header *header, u64 addr,
> +			       struct intel_gsc_mtl_header *header_in,
> +			       struct intel_gsc_mtl_header *header_out,
> +			       u64 addr_in, u64 addr_out,
>   			       size_t msg_out_len)
>   {
>   	struct intel_gt *gt = i915->media_gt;
>   	int ret;
>   
> -	header->flags = 0;
> -	ret = intel_gsc_uc_heci_cmd_submit_packet(&gt->uc.gsc, addr,
> -						  header->message_size,
> -						  addr,
> -						  msg_out_len + sizeof(*header));
> +	ret = intel_gsc_uc_heci_cmd_submit_packet(&gt->uc.gsc, addr_in,
> +						  header_in->message_size,
> +						  addr_out,
> +						  msg_out_len + sizeof(*header_out));
>   	if (ret) {
>   		drm_err(&i915->drm, "failed to send gsc HDCP msg (%d)\n", ret);
>   		return ret;
>   	}
>   
>   	/*
> -	 * Checking validity marker for memory sanity
> +	 * Checking validity marker and header status to see if some error has
> +	 * blocked us from sending message to gsc cs
>   	 */
> -	if (header->validity_marker != GSC_HECI_VALIDITY_MARKER) {
> +	if (header_out->validity_marker != GSC_HECI_VALIDITY_MARKER) {
>   		drm_err(&i915->drm, "invalid validity marker\n");
>   		return -EINVAL;
>   	}
>   
> -	if (header->status != 0) {
> +	if (header_out->status != 0) {
>   		drm_err(&i915->drm, "header status indicates error %d\n",
> -			header->status);
> +			header_out->status);
>   		return -EINVAL;
>   	}
>   
> -	if (header->flags & GSC_OUTFLAG_MSG_PENDING)
> +	if (header_out->flags & GSC_OUTFLAG_MSG_PENDING) {
> +		header_in->gsc_message_handle = header_out->gsc_message_handle;
>   		return -EAGAIN;
> +	}
>   
>   	return 0;
>   }
> @@ -809,7 +813,8 @@ ssize_t intel_hdcp_gsc_msg_send(struct drm_i915_private *i915, u8 *msg_in,
>   	 * 20 times each message 50 ms apart
>   	 */
>   	do {
> -		ret = intel_gsc_send_sync(i915, header_in, addr_in, msg_out_len);
> +		ret = intel_gsc_send_sync(i915, header_in, header_out, addr_in,
> +					  addr_out, msg_out_len);
>   
>   		/* Only try again if gsc says so */
>   		if (ret != -EAGAIN)