From: Michal Wajdeczko <michal.wajdeczko@intel.com>
To: "Michał Winiarski" <michal.winiarski@intel.com>,
intel-xe@lists.freedesktop.org
Cc: Lucas De Marchi <lucas.demarchi@intel.com>,
Matt Roper <matthew.d.roper@intel.com>
Subject: Re: [Intel-xe] [PATCH v4 14/22] drm/xe: Add a helper for DRM device-lifetime BO create
Date: Wed, 29 Nov 2023 10:38:44 +0100 [thread overview]
Message-ID: <e74cb84b-4ecf-45fd-b730-e0009ec76351@intel.com> (raw)
In-Reply-To: <20231129011624.836843-15-michal.winiarski@intel.com>
On 29.11.2023 02:16, Michał Winiarski wrote:
> A helper for managed BO allocations makes it possible to remove specific
> "fini" actions and will simplify the following patches adding ability to
> execute a release action for specific BO directly.
>
> Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
> ---
> drivers/gpu/drm/xe/xe_bo.c | 33 ++++++++++++++++++++++++++++
> drivers/gpu/drm/xe/xe_bo.h | 4 ++++
> drivers/gpu/drm/xe/xe_ggtt.c | 6 +----
> drivers/gpu/drm/xe/xe_guc_ads.c | 20 +++--------------
> drivers/gpu/drm/xe/xe_guc_ct.c | 8 +++----
> drivers/gpu/drm/xe/xe_guc_hwconfig.c | 18 +++------------
> drivers/gpu/drm/xe/xe_guc_log.c | 19 +++-------------
> drivers/gpu/drm/xe/xe_guc_pc.c | 9 +++-----
> drivers/gpu/drm/xe/xe_hw_engine.c | 8 +++----
> drivers/gpu/drm/xe/xe_uc_fw.c | 9 ++++----
> 10 files changed, 60 insertions(+), 74 deletions(-)
>
> diff --git a/drivers/gpu/drm/xe/xe_bo.c b/drivers/gpu/drm/xe/xe_bo.c
> index c5aa01c0861c7..7f5b616a7bbeb 100644
> --- a/drivers/gpu/drm/xe/xe_bo.c
> +++ b/drivers/gpu/drm/xe/xe_bo.c
> @@ -9,6 +9,7 @@
>
> #include <drm/drm_drv.h>
> #include <drm/drm_gem_ttm_helper.h>
> +#include <drm/drm_managed.h>
> #include <drm/ttm/ttm_device.h>
> #include <drm/ttm/ttm_placement.h>
> #include <drm/ttm/ttm_tt.h>
> @@ -1493,6 +1494,38 @@ struct xe_bo *xe_bo_create_from_data(struct xe_device *xe, struct xe_tile *tile,
> return bo;
> }
>
> +static void __xe_bo_release(struct drm_device *drm, void *arg)
nit: __release_xe_bo() ? this is a drm action, not a xe_bo function
> +{
> + xe_bo_unpin_map_no_vm(arg);
> +}
> +
> +struct xe_bo *xe_managed_bo_create_pin_map(struct xe_device *xe, struct xe_tile *tile,
> + size_t size, u32 flags)
> +{
> + struct xe_bo *bo;
> + int ret;
> +
> + bo = xe_bo_create_pin_map(xe, tile, NULL, size, ttm_bo_type_kernel, flags);
> + if (IS_ERR(bo))
> + return bo;
> +
> + ret = drmm_add_action_or_reset(&xe->drm, __xe_bo_release, bo);
> + if (ret)
> + return ERR_PTR(ret);
> +
> + return bo;
> +}
> +
> +struct xe_bo *xe_managed_bo_create_from_data(struct xe_device *xe, struct xe_tile *tile,
> + const void *data, size_t size, u32 flags)
> +{
> + struct xe_bo *bo = xe_managed_bo_create_pin_map(xe, tile, size, flags);
> +
> + xe_map_memcpy_to(xe, &bo->vmap, 0, data, size);
will crash if "bo" is a ERR_PTR
> +
> + return bo;
> +}
> +
> /*
> * XXX: This is in the VM bind data path, likely should calculate this once and
> * store, with a recalculation if the BO is moved.
> diff --git a/drivers/gpu/drm/xe/xe_bo.h b/drivers/gpu/drm/xe/xe_bo.h
> index 9f373f6d25f22..2cf32713bde9e 100644
> --- a/drivers/gpu/drm/xe/xe_bo.h
> +++ b/drivers/gpu/drm/xe/xe_bo.h
> @@ -109,6 +109,10 @@ struct xe_bo *xe_bo_create_pin_map_at(struct xe_device *xe, struct xe_tile *tile
> struct xe_bo *xe_bo_create_from_data(struct xe_device *xe, struct xe_tile *tile,
> const void *data, size_t size,
> enum ttm_bo_type type, u32 flags);
> +struct xe_bo *xe_managed_bo_create_pin_map(struct xe_device *xe, struct xe_tile *tile,
> + size_t size, u32 flags);
> +struct xe_bo *xe_managed_bo_create_from_data(struct xe_device *xe, struct xe_tile *tile,
> + const void *data, size_t size, u32 flags);
nit: isn't "xe_device *xe" redundant if we also pass a "xe_tile *tile" ?
>
> int xe_bo_placement_for_flags(struct xe_device *xe, struct xe_bo *bo,
> u32 bo_flags);
> diff --git a/drivers/gpu/drm/xe/xe_ggtt.c b/drivers/gpu/drm/xe/xe_ggtt.c
> index fab3cc04882da..8dc783e9120d2 100644
> --- a/drivers/gpu/drm/xe/xe_ggtt.c
> +++ b/drivers/gpu/drm/xe/xe_ggtt.c
> @@ -108,7 +108,6 @@ static void ggtt_fini(struct drm_device *drm, void *arg)
> {
> struct xe_ggtt *ggtt = arg;
>
> - xe_bo_unpin_map_no_vm(ggtt->scratch);
> ggtt->scratch = NULL;
> }
>
> @@ -225,10 +224,7 @@ int xe_ggtt_init(struct xe_ggtt *ggtt)
> else
> flags |= XE_BO_CREATE_VRAM_IF_DGFX(ggtt->tile);
>
> - ggtt->scratch = xe_bo_create_pin_map(xe, ggtt->tile, NULL, XE_PAGE_SIZE,
> - ttm_bo_type_kernel,
> - flags);
> -
> + ggtt->scratch = xe_managed_bo_create_pin_map(xe, ggtt->tile, XE_PAGE_SIZE, flags);
> if (IS_ERR(ggtt->scratch)) {
> err = PTR_ERR(ggtt->scratch);
> goto err;
> diff --git a/drivers/gpu/drm/xe/xe_guc_ads.c b/drivers/gpu/drm/xe/xe_guc_ads.c
> index 88789826e7817..2f5ff090aa6bd 100644
> --- a/drivers/gpu/drm/xe/xe_guc_ads.c
> +++ b/drivers/gpu/drm/xe/xe_guc_ads.c
> @@ -202,13 +202,6 @@ static size_t guc_ads_size(struct xe_guc_ads *ads)
> guc_ads_private_data_size(ads);
> }
>
> -static void guc_ads_fini(struct drm_device *drm, void *arg)
> -{
> - struct xe_guc_ads *ads = arg;
> -
> - xe_bo_unpin_map_no_vm(ads->bo);
> -}
> -
> static bool needs_wa_1607983814(struct xe_device *xe)
> {
> return GRAPHICS_VERx100(xe) < 1250;
> @@ -274,25 +267,18 @@ int xe_guc_ads_init(struct xe_guc_ads *ads)
> struct xe_gt *gt = ads_to_gt(ads);
> struct xe_tile *tile = gt_to_tile(gt);
> struct xe_bo *bo;
> - int err;
>
> ads->golden_lrc_size = calculate_golden_lrc_size(ads);
> ads->regset_size = calculate_regset_size(gt);
>
> - bo = xe_bo_create_pin_map(xe, tile, NULL, guc_ads_size(ads) +
> - MAX_GOLDEN_LRC_SIZE,
> - ttm_bo_type_kernel,
> - XE_BO_CREATE_VRAM_IF_DGFX(tile) |
> - XE_BO_CREATE_GGTT_BIT);
> + bo = xe_managed_bo_create_pin_map(xe, tile, guc_ads_size(ads) + MAX_GOLDEN_LRC_SIZE,
> + XE_BO_CREATE_VRAM_IF_DGFX(tile) |
> + XE_BO_CREATE_GGTT_BIT);
> if (IS_ERR(bo))
> return PTR_ERR(bo);
>
> ads->bo = bo;
>
> - err = drmm_add_action_or_reset(&xe->drm, guc_ads_fini, ads);
> - if (err)
> - return err;
> -
> return 0;
> }
>
> diff --git a/drivers/gpu/drm/xe/xe_guc_ct.c b/drivers/gpu/drm/xe/xe_guc_ct.c
> index c44e750746958..93c886ef6fdb4 100644
> --- a/drivers/gpu/drm/xe/xe_guc_ct.c
> +++ b/drivers/gpu/drm/xe/xe_guc_ct.c
> @@ -112,7 +112,6 @@ static void guc_ct_fini(struct drm_device *drm, void *arg)
> struct xe_guc_ct *ct = arg;
>
> xa_destroy(&ct->fence_lookup);
> - xe_bo_unpin_map_no_vm(ct->bo);
> }
>
> static void g2h_worker_func(struct work_struct *w);
> @@ -146,10 +145,9 @@ int xe_guc_ct_init(struct xe_guc_ct *ct)
>
> primelockdep(ct);
>
> - bo = xe_bo_create_pin_map(xe, tile, NULL, guc_ct_size(),
> - ttm_bo_type_kernel,
> - XE_BO_CREATE_VRAM_IF_DGFX(tile) |
> - XE_BO_CREATE_GGTT_BIT);
> + bo = xe_managed_bo_create_pin_map(xe, tile, guc_ct_size(),
> + XE_BO_CREATE_VRAM_IF_DGFX(tile) |
> + XE_BO_CREATE_GGTT_BIT);
> if (IS_ERR(bo))
> return PTR_ERR(bo);
>
> diff --git a/drivers/gpu/drm/xe/xe_guc_hwconfig.c b/drivers/gpu/drm/xe/xe_guc_hwconfig.c
> index 57d325ec8ce32..8e3db5d7192c3 100644
> --- a/drivers/gpu/drm/xe/xe_guc_hwconfig.c
> +++ b/drivers/gpu/drm/xe/xe_guc_hwconfig.c
> @@ -47,13 +47,6 @@ static int guc_hwconfig_copy(struct xe_guc *guc)
> return 0;
> }
>
> -static void guc_hwconfig_fini(struct drm_device *drm, void *arg)
> -{
> - struct xe_guc *guc = arg;
> -
> - xe_bo_unpin_map_no_vm(guc->hwconfig.bo);
> -}
> -
> int xe_guc_hwconfig_init(struct xe_guc *guc)
> {
> struct xe_device *xe = guc_to_xe(guc);
> @@ -83,19 +76,14 @@ int xe_guc_hwconfig_init(struct xe_guc *guc)
> if (!size)
> return -EINVAL;
>
> - bo = xe_bo_create_pin_map(xe, tile, NULL, PAGE_ALIGN(size),
> - ttm_bo_type_kernel,
> - XE_BO_CREATE_VRAM_IF_DGFX(tile) |
> - XE_BO_CREATE_GGTT_BIT);
> + bo = xe_managed_bo_create_pin_map(xe, tile, PAGE_ALIGN(size),
> + XE_BO_CREATE_VRAM_IF_DGFX(tile) |
> + XE_BO_CREATE_GGTT_BIT);
> if (IS_ERR(bo))
> return PTR_ERR(bo);
> guc->hwconfig.bo = bo;
> guc->hwconfig.size = size;
>
> - err = drmm_add_action_or_reset(&xe->drm, guc_hwconfig_fini, guc);
> - if (err)
> - return err;
> -
> return guc_hwconfig_copy(guc);
> }
>
> diff --git a/drivers/gpu/drm/xe/xe_guc_log.c b/drivers/gpu/drm/xe/xe_guc_log.c
> index 27c3827bfd054..bcd2f4d34081d 100644
> --- a/drivers/gpu/drm/xe/xe_guc_log.c
> +++ b/drivers/gpu/drm/xe/xe_guc_log.c
> @@ -77,24 +77,15 @@ void xe_guc_log_print(struct xe_guc_log *log, struct drm_printer *p)
> }
> }
>
> -static void guc_log_fini(struct drm_device *drm, void *arg)
> -{
> - struct xe_guc_log *log = arg;
> -
> - xe_bo_unpin_map_no_vm(log->bo);
> -}
> -
> int xe_guc_log_init(struct xe_guc_log *log)
> {
> struct xe_device *xe = log_to_xe(log);
> struct xe_tile *tile = gt_to_tile(log_to_gt(log));
> struct xe_bo *bo;
> - int err;
>
> - bo = xe_bo_create_pin_map(xe, tile, NULL, guc_log_size(),
> - ttm_bo_type_kernel,
> - XE_BO_CREATE_VRAM_IF_DGFX(tile) |
> - XE_BO_CREATE_GGTT_BIT);
> + bo = xe_managed_bo_create_pin_map(xe, tile, guc_log_size(),
> + XE_BO_CREATE_VRAM_IF_DGFX(tile) |
> + XE_BO_CREATE_GGTT_BIT);
> if (IS_ERR(bo))
> return PTR_ERR(bo);
>
> @@ -102,9 +93,5 @@ int xe_guc_log_init(struct xe_guc_log *log)
> log->bo = bo;
> log->level = xe_modparam.guc_log_level;
>
> - err = drmm_add_action_or_reset(&xe->drm, guc_log_fini, log);
> - if (err)
> - return err;
> -
> return 0;
> }
> diff --git a/drivers/gpu/drm/xe/xe_guc_pc.c b/drivers/gpu/drm/xe/xe_guc_pc.c
> index e9dd6c3d750bd..f5009a7968afa 100644
> --- a/drivers/gpu/drm/xe/xe_guc_pc.c
> +++ b/drivers/gpu/drm/xe/xe_guc_pc.c
> @@ -936,7 +936,6 @@ void xe_guc_pc_fini(struct xe_guc_pc *pc)
> XE_WARN_ON(xe_guc_pc_gucrc_disable(pc));
> XE_WARN_ON(xe_guc_pc_stop(pc));
> sysfs_remove_files(pc_to_gt(pc)->sysfs, pc_attrs);
> - xe_bo_unpin_map_no_vm(pc->bo);
> mutex_destroy(&pc->freq_lock);
> }
>
> @@ -955,11 +954,9 @@ int xe_guc_pc_init(struct xe_guc_pc *pc)
>
> mutex_init(&pc->freq_lock);
>
> - bo = xe_bo_create_pin_map(xe, tile, NULL, size,
> - ttm_bo_type_kernel,
> - XE_BO_CREATE_VRAM_IF_DGFX(tile) |
> - XE_BO_CREATE_GGTT_BIT);
> -
> + bo = xe_managed_bo_create_pin_map(xe, tile, size,
> + XE_BO_CREATE_VRAM_IF_DGFX(tile) |
> + XE_BO_CREATE_GGTT_BIT);
> if (IS_ERR(bo))
> return PTR_ERR(bo);
>
> diff --git a/drivers/gpu/drm/xe/xe_hw_engine.c b/drivers/gpu/drm/xe/xe_hw_engine.c
> index e831e63c5e485..34990f396a1a8 100644
> --- a/drivers/gpu/drm/xe/xe_hw_engine.c
> +++ b/drivers/gpu/drm/xe/xe_hw_engine.c
> @@ -238,8 +238,6 @@ static void hw_engine_fini(struct drm_device *drm, void *arg)
> xe_execlist_port_destroy(hwe->exl_port);
> xe_lrc_finish(&hwe->kernel_lrc);
>
> - xe_bo_unpin_map_no_vm(hwe->hwsp);
> -
> hwe->gt = NULL;
> }
>
> @@ -427,9 +425,9 @@ static int hw_engine_init(struct xe_gt *gt, struct xe_hw_engine *hwe,
> xe_reg_sr_apply_mmio(&hwe->reg_sr, gt);
> xe_reg_sr_apply_whitelist(hwe);
>
> - hwe->hwsp = xe_bo_create_pin_map(xe, tile, NULL, SZ_4K, ttm_bo_type_kernel,
> - XE_BO_CREATE_VRAM_IF_DGFX(tile) |
> - XE_BO_CREATE_GGTT_BIT);
> + hwe->hwsp = xe_managed_bo_create_pin_map(xe, tile, SZ_4K,
> + XE_BO_CREATE_VRAM_IF_DGFX(tile) |
> + XE_BO_CREATE_GGTT_BIT);
> if (IS_ERR(hwe->hwsp)) {
> err = PTR_ERR(hwe->hwsp);
> goto err_name;
> diff --git a/drivers/gpu/drm/xe/xe_uc_fw.c b/drivers/gpu/drm/xe/xe_uc_fw.c
> index 25778887d846f..5b86c51941335 100644
> --- a/drivers/gpu/drm/xe/xe_uc_fw.c
> +++ b/drivers/gpu/drm/xe/xe_uc_fw.c
> @@ -266,7 +266,6 @@ static void uc_fw_fini(struct drm_device *drm, void *arg)
> if (!xe_uc_fw_is_available(uc_fw))
> return;
>
> - xe_bo_unpin_map_no_vm(uc_fw->bo);
> xe_uc_fw_change_status(uc_fw, XE_UC_FIRMWARE_SELECTED);
> }
>
> @@ -575,10 +574,9 @@ int xe_uc_fw_init(struct xe_uc_fw *uc_fw)
> if (err)
> goto fail;
>
> - obj = xe_bo_create_from_data(xe, tile, fw->data, fw->size,
> - ttm_bo_type_kernel,
> - XE_BO_CREATE_VRAM_IF_DGFX(tile) |
> - XE_BO_CREATE_GGTT_BIT);
> + obj = xe_managed_bo_create_from_data(xe, tile, fw->data, fw->size,
> + XE_BO_CREATE_VRAM_IF_DGFX(tile) |
> + XE_BO_CREATE_GGTT_BIT);
> if (IS_ERR(obj)) {
> drm_notice(&xe->drm, "%s firmware %s: failed to create / populate bo",
> xe_uc_fw_type_repr(uc_fw->type), uc_fw->path);
> @@ -609,6 +607,7 @@ int xe_uc_fw_init(struct xe_uc_fw *uc_fw)
> xe_uc_fw_type_repr(uc_fw->type), XE_UC_FIRMWARE_URL);
>
> release_firmware(fw); /* OK even if fw is NULL */
> +
> return err;
> }
>
next prev parent reply other threads:[~2023-11-29 9:38 UTC|newest]
Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-11-29 1:16 [Intel-xe] [PATCH v4 00/22] drm/xe: Probe tweaks and reordering Michał Winiarski
2023-11-29 1:16 ` [Intel-xe] [PATCH v4 01/22] drm/xe: Skip calling drm_dev_put on probe error Michał Winiarski
2023-11-29 1:16 ` [Intel-xe] [PATCH v4 02/22] drm/xe: Use managed pci_enable_device Michał Winiarski
2023-11-29 16:02 ` Matt Roper
2023-11-29 1:16 ` [Intel-xe] [PATCH v4 03/22] drm/xe/irq: Don't call pci_free_irq_vectors Michał Winiarski
2023-11-29 1:16 ` [Intel-xe] [PATCH v4 04/22] drm/xe: Move xe_set_dma_info outside of MMIO setup Michał Winiarski
2023-11-29 1:16 ` [Intel-xe] [PATCH v4 05/22] drm/xe: Move xe_mmio_probe_tiles " Michał Winiarski
2023-11-29 1:16 ` [Intel-xe] [PATCH v4 06/22] drm/xe: Split xe_info_init Michał Winiarski
2023-11-29 1:16 ` [Intel-xe] [PATCH v4 07/22] drm/xe: Introduce xe_tile_init_early and use at earlier point in probe Michał Winiarski
2023-11-29 1:16 ` [Intel-xe] [PATCH v4 08/22] drm/xe: Map the entire BAR0 and hold onto the initial mapping Michał Winiarski
2023-11-29 1:16 ` [Intel-xe] [PATCH v4 09/22] drm/xe/device: Introduce xe_device_probe_early Michał Winiarski
2023-11-29 1:16 ` [Intel-xe] [PATCH v4 10/22] drm/xe: Don't "peek" into GMD_ID Michał Winiarski
2023-11-29 1:16 ` [Intel-xe] [PATCH v4 11/22] drm/xe: Move system memory management init to earlier point in probe Michał Winiarski
2023-11-29 1:16 ` [Intel-xe] [PATCH v4 12/22] drm/xe: Move force_wake " Michał Winiarski
2023-11-29 1:16 ` [Intel-xe] [PATCH v4 13/22] drm/xe: Reorder GGTT " Michał Winiarski
2023-11-29 1:16 ` [Intel-xe] [PATCH v4 14/22] drm/xe: Add a helper for DRM device-lifetime BO create Michał Winiarski
2023-11-29 9:38 ` Michal Wajdeczko [this message]
2023-11-29 20:32 ` Michał Winiarski
2023-11-29 1:16 ` [Intel-xe] [PATCH v4 15/22] drm/xe/uc: Split xe_uc_fw_init Michał Winiarski
2023-11-29 1:16 ` [Intel-xe] [PATCH v4 16/22] drm/xe/uc: Store firmware binary in system-memory backed BO Michał Winiarski
2023-11-29 1:16 ` [Intel-xe] [PATCH v4 17/22] drm/xe/uc: Extract xe_uc_sanitize_reset Michał Winiarski
2023-11-29 1:16 ` [Intel-xe] [PATCH v4 18/22] drm/xe/guc: Split GuC params used for "hwconfig" and "post-hwconfig" Michał Winiarski
2023-11-29 1:16 ` [Intel-xe] [PATCH v4 19/22] drm/managed: Add drmm_release_action Michał Winiarski
2023-11-29 9:43 ` Michal Wajdeczko
2023-11-29 17:52 ` Rodrigo Vivi
2023-11-29 22:17 ` Michał Winiarski
2023-11-29 1:16 ` [Intel-xe] [PATCH v4 20/22] drm/xe/guc: Allocate GuC data structures in system memory for initial load Michał Winiarski
2023-11-29 1:16 ` [Intel-xe] [PATCH v4 21/22] drm/xe/guc: Move GuC power control init to "post-hwconfig" Michał Winiarski
2023-11-29 1:16 ` [Intel-xe] [PATCH v4 22/22] drm/xe: Initialize GuC earlier during probe Michał Winiarski
2023-11-29 21:48 ` Welty, Brian
2023-11-29 22:20 ` Michał Winiarski
2023-11-29 4:48 ` [Intel-xe] ✗ CI.Patch_applied: failure for drm/xe: Probe tweaks and reordering (rev3) Patchwork
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=e74cb84b-4ecf-45fd-b730-e0009ec76351@intel.com \
--to=michal.wajdeczko@intel.com \
--cc=intel-xe@lists.freedesktop.org \
--cc=lucas.demarchi@intel.com \
--cc=matthew.d.roper@intel.com \
--cc=michal.winiarski@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox