Re: [Intel-gfx] [PATCH 3/4] drm/i915/guc: Update CTB helpers to use CT_ERROR

[Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>]

On 1/11/2020 3:11 PM, Michal Wajdeczko wrote:
> Update GuC CTB action helpers to benefit from new CT_ERROR macro.
>
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> ---
>   drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c | 57 ++++++++++++-----------
>   1 file changed, 31 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
> index eb123543392a..1da69425029b 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
> @@ -107,31 +107,40 @@ static int guc_action_register_ct_buffer(struct intel_guc *guc,
>   		sizeof(struct guc_ct_buffer_desc),
>   		type
>   	};
> -	int err;
>   
>   	/* Can't use generic send(), CT registration must go over MMIO */
> -	err = intel_guc_send_mmio(guc, action, ARRAY_SIZE(action), NULL, 0);
> -	if (err)
> -		DRM_ERROR("CT: register %s buffer failed; err=%d\n",
> -			  guc_ct_buffer_type_to_str(type), err);
> +	return intel_guc_send_mmio(guc, action, ARRAY_SIZE(action), NULL, 0);
> +}
> +
> +static int ct_register_buffer(struct intel_guc_ct *ct, u32 desc_addr, u32 type)
> +{
> +	int err = guc_action_register_ct_buffer(ct_to_guc(ct), desc_addr, type);
> +
> +	if (unlikely(err))
> +		CT_ERROR(ct, "Failed to register %s buffer (err=%d)\n",
> +			 guc_ct_buffer_type_to_str(type), err);
>   	return err;
>   }

Now that we're passing ct to this I'm wondering if it makes sense to 
move some of the math from outside in here, e.g:

const u32 i915_ct_to_guc_ct_type [] = {
	[CTB_RECV] = INTEL_GUC_CT_BUFFER_TYPE_RECV;
	[CTB_SEND] = INTEL_GUC_CT_BUFFER_TYPE_SEND;
}

#define CT_DESC_OFFSET 0
#define CT_DESC_SIZE (PAGE_SIZE / 4)
#define CT_BUF_OFFSET (PAGE_SIZE / 2)
#define CT_BUF_SIZE (PAGE_SIZE / 4)
static int ct_register_buffer(struct intel_guc_ct *ct, u32 type)
{
	struct intel_guc *guc = ct_to_guc(ct);
	u32 base = intel_guc_ggtt_offset(guc, ct->vma);
	u32 desc_addr = base + CT_DESC_OFFSET + CT_DESC_SIZE * type;
	u32 buf_addr = base + CT_BUF_OFFSET + CT_BUF_SIZE * type;
	int err;

	ct_buffer_desc_init(ct->ctbs[type].desc, buf_addr, CT_BUF_SIZE);

	err = guc_action_register_ct_buffer(guc, desc_addr,
					    i915_ct_to_guc_ct_type[type]);
	if (unlikely(err))
		CT_ERROR(ct, "Failed to register %s buffer (err=%d)\n",
			 guc_ct_buffer_type_to_str(type), err);
  	return err;
}


And then just call:

      err = ct_register_buffer(ct, CTB_RECV);


Or maybe it's overkill?
With or without the above changes:

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

Daniele

>   
> -static int guc_action_deregister_ct_buffer(struct intel_guc *guc,
> -					   u32 type)
> +static int guc_action_deregister_ct_buffer(struct intel_guc *guc, u32 type)
>   {
>   	u32 action[] = {
>   		INTEL_GUC_ACTION_DEREGISTER_COMMAND_TRANSPORT_BUFFER,
>   		CTB_OWNER_HOST,
>   		type
>   	};
> -	int err;
>   
>   	/* Can't use generic send(), CT deregistration must go over MMIO */
> -	err = intel_guc_send_mmio(guc, action, ARRAY_SIZE(action), NULL, 0);
> -	if (err)
> -		DRM_ERROR("CT: deregister %s buffer failed; err=%d\n",
> -			  guc_ct_buffer_type_to_str(type), err);
> +	return intel_guc_send_mmio(guc, action, ARRAY_SIZE(action), NULL, 0);
> +}
> +
> +static int ct_deregister_buffer(struct intel_guc_ct *ct, u32 type)
> +{
> +	int err = guc_action_deregister_ct_buffer(ct_to_guc(ct), type);
> +
> +	if (unlikely(err))
> +		CT_ERROR(ct, "Failed to deregister %s buffer (err=%d)\n",
> +			 guc_ct_buffer_type_to_str(type), err);
>   	return err;
>   }
>   
> @@ -235,18 +244,17 @@ int intel_guc_ct_enable(struct intel_guc_ct *ct)
>   					PAGE_SIZE/4);
>   	}
>   
> -	/* register buffers, starting wirh RECV buffer
> -	 * descriptors are in first half of the blob
> +	/*
> +	 * Register both CT buffers starting with RECV buffer.
> +	 * Descriptors are in first half of the blob.
>   	 */
> -	err = guc_action_register_ct_buffer(guc,
> -					    base + PAGE_SIZE/4 * CTB_RECV,
> -					    INTEL_GUC_CT_BUFFER_TYPE_RECV);
> +	err = ct_register_buffer(ct, base + PAGE_SIZE/4 * CTB_RECV,
> +				 INTEL_GUC_CT_BUFFER_TYPE_RECV);
>   	if (unlikely(err))
>   		goto err_out;
>   
> -	err = guc_action_register_ct_buffer(guc,
> -					    base + PAGE_SIZE/4 * CTB_SEND,
> -					    INTEL_GUC_CT_BUFFER_TYPE_SEND);
> +	err = ct_register_buffer(ct, base + PAGE_SIZE/4 * CTB_SEND,
> +				 INTEL_GUC_CT_BUFFER_TYPE_SEND);
>   	if (unlikely(err))
>   		goto err_deregister;
>   
> @@ -255,8 +263,7 @@ int intel_guc_ct_enable(struct intel_guc_ct *ct)
>   	return 0;
>   
>   err_deregister:
> -	guc_action_deregister_ct_buffer(guc,
> -					INTEL_GUC_CT_BUFFER_TYPE_RECV);
> +	ct_deregister_buffer(ct, INTEL_GUC_CT_BUFFER_TYPE_RECV);
>   err_out:
>   	CT_ERROR(ct, "Failed to open open CT channel (err=%d)\n", err);
>   	return err;
> @@ -275,10 +282,8 @@ void intel_guc_ct_disable(struct intel_guc_ct *ct)
>   	ct->enabled = false;
>   
>   	if (intel_guc_is_running(guc)) {
> -		guc_action_deregister_ct_buffer(guc,
> -						INTEL_GUC_CT_BUFFER_TYPE_SEND);
> -		guc_action_deregister_ct_buffer(guc,
> -						INTEL_GUC_CT_BUFFER_TYPE_RECV);
> +		ct_deregister_buffer(ct, INTEL_GUC_CT_BUFFER_TYPE_SEND);
> +		ct_deregister_buffer(ct, INTEL_GUC_CT_BUFFER_TYPE_RECV);
>   	}
>   }
>   

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx