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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.