Re: [Intel-xe] [PATCH v2 02/11] drm/xe: Sort includes

[Jani Nikula <jani.nikula@linux.intel.com>]

On Thu, 16 Feb 2023, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
> Sort includes and split them in blocks:
>
> 1) .h corresponding to the .c. Example: xe_bb.c should have a "#include
>    "xe_bb.h" first.

Personally, I've never liked this, because the local headers could have
stuff that impacts global headers. And if there are conflicts, you kind
of want to have your local one look like the culprit (although modern
compilers are pretty good at showing both sites).

Note that to keep headers self-contained, this is not enough for headers
not associated with a .c, which is why we have the HDRTEST rule in i915.

> 2) #include <linux/...>
> 3) #include <drm/...>
> 4) local includes
> 5) i915 includes
>
> This is accomplished by running
> `clang-format --style=file -i --sort-includes drivers/gpu/drm/xe/*.c`
> and ignoring all the changes after the includes. There are also some
> manual tweaks to split the blocks.

Do you have the style file somewhere?

>
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_bb.c                  |  4 ++--
>  drivers/gpu/drm/xe/xe_bo.c                  |  2 --
>  drivers/gpu/drm/xe/xe_bo_evict.c            |  2 +-
>  drivers/gpu/drm/xe/xe_debugfs.c             |  2 +-
>  drivers/gpu/drm/xe/xe_device.c              |  9 ++++-----
>  drivers/gpu/drm/xe/xe_display.c             | 12 ++++++------
>  drivers/gpu/drm/xe/xe_dma_buf.c             |  8 +++-----
>  drivers/gpu/drm/xe/xe_engine.c              |  4 ++--
>  drivers/gpu/drm/xe/xe_exec.c                |  2 +-
>  drivers/gpu/drm/xe/xe_execlist.c            |  9 ++++-----
>  drivers/gpu/drm/xe/xe_force_wake.c          |  4 ++--
>  drivers/gpu/drm/xe/xe_ggtt.c                |  7 +++----
>  drivers/gpu/drm/xe/xe_gt.c                  |  2 +-
>  drivers/gpu/drm/xe/xe_gt_clock.c            |  8 ++++----
>  drivers/gpu/drm/xe/xe_gt_debugfs.c          |  2 +-
>  drivers/gpu/drm/xe/xe_gt_mcr.c              |  2 +-
>  drivers/gpu/drm/xe/xe_gt_pagefault.c        |  2 +-
>  drivers/gpu/drm/xe/xe_gt_sysfs.c            |  4 +++-
>  drivers/gpu/drm/xe/xe_gt_tlb_invalidation.c |  2 +-
>  drivers/gpu/drm/xe/xe_gt_topology.c         |  2 +-
>  drivers/gpu/drm/xe/xe_guc.c                 | 13 +++++++------
>  drivers/gpu/drm/xe/xe_guc_ads.c             |  5 +++--
>  drivers/gpu/drm/xe/xe_guc_ct.c              |  4 ++--
>  drivers/gpu/drm/xe/xe_guc_debugfs.c         |  2 +-
>  drivers/gpu/drm/xe/xe_guc_hwconfig.c        |  2 +-
>  drivers/gpu/drm/xe/xe_guc_log.c             |  2 +-
>  drivers/gpu/drm/xe/xe_guc_pc.c              | 12 +++++++-----
>  drivers/gpu/drm/xe/xe_guc_submit.c          |  6 +++---
>  drivers/gpu/drm/xe/xe_huc.c                 |  2 +-
>  drivers/gpu/drm/xe/xe_huc_debugfs.c         |  2 +-
>  drivers/gpu/drm/xe/xe_hw_engine.c           |  3 +--
>  drivers/gpu/drm/xe/xe_hw_fence.c            |  1 -
>  drivers/gpu/drm/xe/xe_irq.c                 |  5 +++--
>  drivers/gpu/drm/xe/xe_lrc.c                 |  7 +++----
>  drivers/gpu/drm/xe/xe_migrate.c             | 11 ++++++-----
>  drivers/gpu/drm/xe/xe_mmio.c                |  3 +--
>  drivers/gpu/drm/xe/xe_mocs.c                |  4 ++--
>  drivers/gpu/drm/xe/xe_module.c              |  1 +
>  drivers/gpu/drm/xe/xe_pci.c                 |  5 ++---
>  drivers/gpu/drm/xe/xe_pcode.c               | 10 ++++------
>  drivers/gpu/drm/xe/xe_pm.c                  |  4 ++--
>  drivers/gpu/drm/xe/xe_preempt_fence.c       |  2 +-
>  drivers/gpu/drm/xe/xe_pt.c                  |  4 ++--
>  drivers/gpu/drm/xe/xe_query.c               | 11 ++++++-----
>  drivers/gpu/drm/xe/xe_reg_sr.c              |  5 ++---
>  drivers/gpu/drm/xe/xe_reg_whitelist.c       |  7 +++----
>  drivers/gpu/drm/xe/xe_ring_ops.c            |  4 ++--
>  drivers/gpu/drm/xe/xe_rtp.c                 |  1 -
>  drivers/gpu/drm/xe/xe_sa.c                  |  3 ++-
>  drivers/gpu/drm/xe/xe_sched_job.c           |  1 -
>  drivers/gpu/drm/xe/xe_sync.c                |  5 +++--
>  drivers/gpu/drm/xe/xe_ttm_gtt_mgr.c         |  2 +-
>  drivers/gpu/drm/xe/xe_ttm_stolen_mgr.c      |  2 +-
>  drivers/gpu/drm/xe/xe_ttm_vram_mgr.c        |  2 +-
>  drivers/gpu/drm/xe/xe_tuning.c              |  2 +-
>  drivers/gpu/drm/xe/xe_uc.c                  |  4 ++--
>  drivers/gpu/drm/xe/xe_vm.c                  |  2 +-
>  drivers/gpu/drm/xe/xe_vm_madvise.c          |  8 +++++---
>  drivers/gpu/drm/xe/xe_wopcm.c               |  2 +-
>  59 files changed, 128 insertions(+), 132 deletions(-)
>
> diff --git a/drivers/gpu/drm/xe/xe_bb.c b/drivers/gpu/drm/xe/xe_bb.c
> index 8b9209571fd0..a25079d4e710 100644
> --- a/drivers/gpu/drm/xe/xe_bb.c
> +++ b/drivers/gpu/drm/xe/xe_bb.c
> @@ -2,12 +2,12 @@
>  /*
>   * Copyright © 2022 Intel Corporation
>   */
> -

Nitpick, throughout the series, I would leave a blank line after the
SPDX and copyright boilerplate, regardless of whether it's an #include
or header guard #ifdef that follows. They're not comments about what
follows, which is what it feels like when they're directly attached to
something that follows without a blank line.

>  #include "xe_bb.h"
> -#include "xe_sa.h"
> +
>  #include "xe_device.h"
>  #include "xe_engine_types.h"
>  #include "xe_hw_fence.h"
> +#include "xe_sa.h"
>  #include "xe_sched_job.h"
>  #include "xe_vm_types.h"
>

...

> diff --git a/drivers/gpu/drm/xe/xe_uc.c b/drivers/gpu/drm/xe/xe_uc.c
> index 7886c8b85397..a27e6c1a6aff 100644
> --- a/drivers/gpu/drm/xe/xe_uc.c
> +++ b/drivers/gpu/drm/xe/xe_uc.c
> @@ -3,13 +3,13 @@
>   * Copyright © 2022 Intel Corporation
>   */
>  
> +#include "xe_uc.h"
>  #include "xe_device.h"
> -#include "xe_huc.h"
>  #include "xe_gt.h"
>  #include "xe_guc.h"
>  #include "xe_guc_pc.h"
>  #include "xe_guc_submit.h"
> -#include "xe_uc.h"
> +#include "xe_huc.h"
>  #include "xe_uc_fw.h"
>  #include "xe_wopcm.h"

Here, having the .h corresponding to the .c just looks like it's not
properly sorted.

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Graphics Center