Re: [PATCH v8 1/6] drm/xe/guc: Prepare GuC register list and update ADS size for error capture

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

On Mon, 2024-05-06 at 18:47 -0700, Zhanjun Dong wrote:

alan:snip (missed a few more smaller ones)

> diff --git a/drivers/gpu/drm/xe/xe_guc_ads.c b/drivers/gpu/drm/xe/xe_guc_ads.c
> index 6a5eb21748b1..3d4563d3ca4f 100644
> --- a/drivers/gpu/drm/xe/xe_guc_ads.c
> +++ b/drivers/gpu/drm/xe/xe_guc_ads.c
> @@ -17,6 +17,8 @@
>  #include "xe_gt.h"
>  #include "xe_gt_ccs_mode.h"
>  #include "xe_guc.h"
> +#include "xe_guc_capture.h"
> +#include "xe_guc_capture_fwif.h"
>  #include "xe_guc_ct.h"
>  #include "xe_hw_engine.h"
>  #include "xe_lrc.h"
> @@ -126,6 +128,8 @@ struct __guc_ads_blob {
>  #define info_map_read(xe_, map_, field_) \
>         xe_map_rd_field(xe_, map_, 0, struct guc_gt_system_info, field_)
>  
> +static int guc_capture_prep_lists(struct xe_guc_ads *ads);
> +
alan:nit; one more thing, it looks like you don't need this prototype definition here now after rev8?

> diff --git a/drivers/gpu/drm/xe/xe_guc_capture.c b/drivers/gpu/drm/xe/xe_guc_capture.c
> new file mode 100644
> index 000000000000..b21a8ef85c4e
> --- /dev/null
> +++ b/drivers/gpu/drm/xe/xe_guc_capture.c
> @@ -0,0 +1,352 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright © 2021-2022 Intel Corporation
> + */
> +
> +#include <linux/types.h>
> +
> +#include <drm/drm_managed.h>
> +#include <drm/drm_print.h>
> +
> +#include "abi/guc_actions_abi.h"
> +#include "regs/xe_regs.h"
> +#include "regs/xe_engine_regs.h"
alan: alphabetical order (i believe this is not a nit from other reviews ive seen)
> +#include "regs/xe_gt_regs.h"
> +#include "regs/xe_guc_regs.h"
> +
> +#include "xe_bo.h"
> +#include "xe_device.h"
> +#include "xe_exec_queue_types.h"
> +#include "xe_gt.h"
> +#include "xe_gt_printk.h"
> +#include "xe_guc.h"
> +#include "xe_guc_capture.h"
> +#include "xe_guc_capture_fwif.h"
> +#include "xe_guc_ct.h"
> +#include "xe_guc_log.h"
> +#include "xe_gt_mcr.h"
alan: alphabetical order
> +#include "xe_guc_submit.h"
> +#include "xe_hw_engine_types.h"
> +#include "xe_macros.h"
> +#include "xe_map.h"
> 
alan:snip

> diff --git a/drivers/gpu/drm/xe/xe_guc_capture_fwif.h b/drivers/gpu/drm/xe/xe_guc_capture_fwif.h
> new file mode 100644
> index 000000000000..79bc277afaa8
> --- /dev/null
> +++ b/drivers/gpu/drm/xe/xe_guc_capture_fwif.h
> @@ -0,0 +1,168 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright © 2021-2022 Intel Corporation
> + */
> +
> +#ifndef _XE_GUC_CAPTURE_FWIF_H
> +#define _XE_GUC_CAPTURE_FWIF_H
> +
> +#include <linux/types.h>
> +#include "regs/xe_reg_defs.h"
> +#include "xe_guc_fwif.h"

+
+#include <linux/types.h>
+#include "regs/xe_reg_defs.h"
+#include "xe_guc_fwif.h"
alan: as per other header inclusions, i belive the coding standard is to separate
the inclusions into chunks (line separated) based on groups where each group is
the top level header folder like this as a fake example. however, i cant find
the rule written down anywhere so i am labelling this as a nit for now:

+#include <linux/foo1.h>
+#include <linux/foo2.h>
+#include <linux/foo3.h>

#include <drm/drm_foo1.h>
#include <drm/drm_foo2.h>

+#include "regs/xe_reg_foo1.h"
+#include "xe_foo1.h"
+#include "xe_gt_foo1.h"
+#include "xe_guc_foo1.h"

alan:snip