Re: [PATCH 2/2] drm/i915: split out i915_gtt_view_types.h from i915_vma_types.h

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

One opinion - consider it a nit, but maybe since all of the content of this new
header display specific, maybe instead of "i915_ggtt_view_types", why not "i915_plane_gtt_types"
(or i915_display_gtt_types), if u plan to also expand to things across plane<->pipe


...alan

On Mon, 2025-02-24 at 18:00 +0200, Jani Nikula wrote:
> In the interest of limiting the display dependencies on i915 core
> headers, split out i915_gtt_view_types.h from i915_vma_types.h, and only
> include the new header from intel_display_types.h.
> 
> Reuse the new header from xe compat code too, failing build if partial
> view is used in display code.
> 
> Side note: Why would we ever have set enum i915_gtt_view_type values to
> size of each type?! What an insane hack.
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  .../drm/i915/display/intel_display_types.h    |  2 +-
>  drivers/gpu/drm/i915/i915_gtt_view_types.h    | 59 +++++++++++++++
>  drivers/gpu/drm/i915/i915_vma_types.h         | 52 +------------
>  .../compat-i915-headers/i915_gtt_view_types.h |  7 ++
>  .../xe/compat-i915-headers/i915_vma_types.h   | 74 -------------------
>  5 files changed, 69 insertions(+), 125 deletions(-)
>  create mode 100644 drivers/gpu/drm/i915/i915_gtt_view_types.h
>  create mode 100644 drivers/gpu/drm/xe/compat-i915-headers/i915_gtt_view_types.h
>  delete mode 100644 drivers/gpu/drm/xe/compat-i915-headers/i915_vma_types.h
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> index 12723ba13eb6..a856cbcf102f 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -42,7 +42,7 @@
>  #include <drm/intel/i915_hdcp_interface.h>
>  #include <uapi/drm/i915_drm.h>
>  
> -#include "i915_vma_types.h"
> +#include "i915_gtt_view_types.h"
>  #include "intel_bios.h"
>  #include "intel_display.h"
>  #include "intel_display_conversion.h"
> diff --git a/drivers/gpu/drm/i915/i915_gtt_view_types.h b/drivers/gpu/drm/i915/i915_gtt_view_types.h
> new file mode 100644
> index 000000000000..c084f67bc880
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/i915_gtt_view_types.h
> @@ -0,0 +1,59 @@
> +/* SPDX-License-Identifier: MIT */
> +/* Copyright © 2025 Intel Corporation */
> +
> +#ifndef __I915_GTT_VIEW_TYPES_H__
> +#define __I915_GTT_VIEW_TYPES_H__
> +
> +#include <linux/types.h>
> +
> +struct intel_remapped_plane_info {
> +       /* in gtt pages */
> +       u32 offset:31;
> +       u32 linear:1;
> +       union {
> +               /* in gtt pages for !linear */
> +               struct {
> +                       u16 width;
> +                       u16 height;
> +                       u16 src_stride;
> +                       u16 dst_stride;
> +               };
> +
> +               /* in gtt pages for linear */
> +               u32 size;
> +       };
> +} __packed;
> +
> +struct intel_rotation_info {
> +       struct intel_remapped_plane_info plane[2];
> +} __packed;
> +
> +struct intel_partial_info {
> +       u64 offset;
> +       unsigned int size;
> +} __packed;
> +
> +struct intel_remapped_info {
> +       struct intel_remapped_plane_info plane[4];
> +       /* in gtt pages */
> +       u32 plane_alignment;
> +} __packed;
> +
> +enum i915_gtt_view_type {
> +       I915_GTT_VIEW_NORMAL = 0,
> +       I915_GTT_VIEW_ROTATED = sizeof(struct intel_rotation_info),
> +       I915_GTT_VIEW_PARTIAL = sizeof(struct intel_partial_info),
> +       I915_GTT_VIEW_REMAPPED = sizeof(struct intel_remapped_info),
> +};
> +
> +struct i915_gtt_view {
> +       enum i915_gtt_view_type type;
> +       union {
> +               /* Members need to contain no holes/padding */
> +               struct intel_partial_info partial;
> +               struct intel_rotation_info rotated;
> +               struct intel_remapped_info remapped;
> +       };
> +};
> +
> +#endif /* __I915_GTT_VIEW_TYPES_H__ */
> diff --git a/drivers/gpu/drm/i915/i915_vma_types.h b/drivers/gpu/drm/i915/i915_vma_types.h
> index 559de74d0b11..a499a3bea874 100644
> --- a/drivers/gpu/drm/i915/i915_vma_types.h
> +++ b/drivers/gpu/drm/i915/i915_vma_types.h
> @@ -32,6 +32,8 @@
>  
>  #include "gem/i915_gem_object_types.h"
>  
> +#include "i915_gtt_view_types.h"
> +
>  /**
>   * DOC: Global GTT views
>   *
> @@ -95,46 +97,6 @@
>  
>  struct i915_vma_resource;
>  
> -struct intel_remapped_plane_info {
> -       /* in gtt pages */
> -       u32 offset:31;
> -       u32 linear:1;
> -       union {
> -               /* in gtt pages for !linear */
> -               struct {
> -                       u16 width;
> -                       u16 height;
> -                       u16 src_stride;
> -                       u16 dst_stride;
> -               };
> -
> -               /* in gtt pages for linear */
> -               u32 size;
> -       };
> -} __packed;
> -
> -struct intel_remapped_info {
> -       struct intel_remapped_plane_info plane[4];
> -       /* in gtt pages */
> -       u32 plane_alignment;
> -} __packed;
> -
> -struct intel_rotation_info {
> -       struct intel_remapped_plane_info plane[2];
> -} __packed;
> -
> -struct intel_partial_info {
> -       u64 offset;
> -       unsigned int size;
> -} __packed;
> -
> -enum i915_gtt_view_type {
> -       I915_GTT_VIEW_NORMAL = 0,
> -       I915_GTT_VIEW_ROTATED = sizeof(struct intel_rotation_info),
> -       I915_GTT_VIEW_PARTIAL = sizeof(struct intel_partial_info),
> -       I915_GTT_VIEW_REMAPPED = sizeof(struct intel_remapped_info),
> -};
> -
>  static inline void assert_i915_gem_gtt_types(void)
>  {
>         BUILD_BUG_ON(sizeof(struct intel_rotation_info) != 2 * sizeof(u32) + 8 * sizeof(u16));
> @@ -160,16 +122,6 @@ static inline void assert_i915_gem_gtt_types(void)
>         }
>  }
>  
> -struct i915_gtt_view {
> -       enum i915_gtt_view_type type;
> -       union {
> -               /* Members need to contain no holes/padding */
> -               struct intel_partial_info partial;
> -               struct intel_rotation_info rotated;
> -               struct intel_remapped_info remapped;
> -       };
> -};
> -
>  /**
>   * DOC: Virtual Memory Address
>   *
> diff --git a/drivers/gpu/drm/xe/compat-i915-headers/i915_gtt_view_types.h b/drivers/gpu/drm/xe/compat-i915-
> headers/i915_gtt_view_types.h
> new file mode 100644
> index 000000000000..b261910cd6f9
> --- /dev/null
> +++ b/drivers/gpu/drm/xe/compat-i915-headers/i915_gtt_view_types.h
> @@ -0,0 +1,7 @@
> +/* SPDX-License-Identifier: MIT */
> +/* Copyright © 2025 Intel Corporation */
> +
> +#include "../../i915/i915_gtt_view_types.h"
> +
> +/* Partial view not supported in xe, fail build if used. */
> +#define I915_GTT_VIEW_PARTIAL
> diff --git a/drivers/gpu/drm/xe/compat-i915-headers/i915_vma_types.h b/drivers/gpu/drm/xe/compat-i915-
> headers/i915_vma_types.h
> deleted file mode 100644
> index e7aaf50f5485..000000000000
> --- a/drivers/gpu/drm/xe/compat-i915-headers/i915_vma_types.h
> +++ /dev/null
> @@ -1,74 +0,0 @@
> -/* SPDX-License-Identifier: MIT */
> -/*
> - * Copyright © 2023 Intel Corporation
> - */
> -
> -#include <linux/types.h>
> -#include <linux/build_bug.h>
> -
> -/* XX: Figure out how to handle this vma mapping in xe */
> -struct intel_remapped_plane_info {
> -       /* in gtt pages */
> -       u32 offset:31;
> -       u32 linear:1;
> -       union {
> -               /* in gtt pages for !linear */
> -               struct {
> -                       u16 width;
> -                       u16 height;
> -                       u16 src_stride;
> -                       u16 dst_stride;
> -               };
> -
> -               /* in gtt pages for linear */
> -               u32 size;
> -       };
> -} __packed;
> -
> -struct intel_remapped_info {
> -       struct intel_remapped_plane_info plane[4];
> -       /* in gtt pages */
> -       u32 plane_alignment;
> -} __packed;
> -
> -struct intel_rotation_info {
> -       struct intel_remapped_plane_info plane[2];
> -} __packed;
> -
> -enum i915_gtt_view_type {
> -       I915_GTT_VIEW_NORMAL = 0,
> -       I915_GTT_VIEW_ROTATED = sizeof(struct intel_rotation_info),
> -       I915_GTT_VIEW_REMAPPED = sizeof(struct intel_remapped_info),
> -};
> -
> -static inline void assert_i915_gem_gtt_types(void)
> -{
> -       BUILD_BUG_ON(sizeof(struct intel_rotation_info) != 2 * sizeof(u32) + 8 * sizeof(u16));
> -       BUILD_BUG_ON(sizeof(struct intel_remapped_info) != 5 * sizeof(u32) + 16 * sizeof(u16));
> -
> -       /* Check that rotation/remapped shares offsets for simplicity */
> -       BUILD_BUG_ON(offsetof(struct intel_remapped_info, plane[0]) !=
> -                    offsetof(struct intel_rotation_info, plane[0]));
> -       BUILD_BUG_ON(offsetofend(struct intel_remapped_info, plane[1]) !=
> -                    offsetofend(struct intel_rotation_info, plane[1]));
> -
> -       /* As we encode the size of each branch inside the union into its type,
> -        * we have to be careful that each branch has a unique size.
> -        */
> -       switch ((enum i915_gtt_view_type)0) {
> -       case I915_GTT_VIEW_NORMAL:
> -       case I915_GTT_VIEW_ROTATED:
> -       case I915_GTT_VIEW_REMAPPED:
> -               /* gcc complains if these are identical cases */
> -               break;
> -       }
> -}
> -
> -struct i915_gtt_view {
> -       enum i915_gtt_view_type type;
> -       union {
> -               /* Members need to contain no holes/padding */
> -               struct intel_rotation_info rotated;
> -               struct intel_remapped_info remapped;
> -       };
> -};
> -- 
> 2.39.5
>