Re: [Intel-xe] [PATCH 6/6] drm/xe/mocs: LNCF MOCS settings only need to be restored on pre-Xe_HP

[Lucas De Marchi <lucas.demarchi@intel.com>]

On Thu, Feb 16, 2023 at 03:17:24PM -0800, Matt Roper wrote:
>Reprogramming the LNCF MOCS registers on render domain reset is not
>intended to be regular driver programming, but rather the implementation
>of a specific workaround (Wa_1607983814).  This workaround no longer
>applies on Xe_HP any beyond, so we can expect that these registers, like
>the rest of the LNCF/LBCF registers, will maintain their values through
>all engine resets.  We should only add these registers to the GuC's
>save/restore list on platforms that need the workaround.
>
>Furthermore, xe_mocs_init_engine() appears to be another attempt to
>satisfy this same workaround.  This is unnecessary on the Xe driver
>since even on platforms where the workaround is necessary, all
>single-engine resets are initiated by the GuC and thus the GuC will take
>care of saving/restoring these registers.  The only host-initiated
>resets we have in Xe are full GT resets which will already
>(re)initialize these registers as part of the regular xe_mocs_init()
>flow.
>
>Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
>---
> drivers/gpu/drm/xe/xe_execlist.c   |  2 +-
> drivers/gpu/drm/xe/xe_guc_ads.c    | 10 +++++++---
> drivers/gpu/drm/xe/xe_guc_submit.c |  1 -
> drivers/gpu/drm/xe/xe_mocs.c       | 13 -------------
> drivers/gpu/drm/xe/xe_mocs.h       |  1 -
> 5 files changed, 8 insertions(+), 19 deletions(-)
>
>diff --git a/drivers/gpu/drm/xe/xe_execlist.c b/drivers/gpu/drm/xe/xe_execlist.c
>index d555d77cbf49..fd0ebfe7cae3 100644
>--- a/drivers/gpu/drm/xe/xe_execlist.c
>+++ b/drivers/gpu/drm/xe/xe_execlist.c
>@@ -462,7 +462,7 @@ static void execlist_engine_suspend_wait(struct xe_engine *e)
>
> static void execlist_engine_resume(struct xe_engine *e)
> {
>-	xe_mocs_init_engine(e);
>+	/* NIY */

what does NIY mean?  maybe "nop" is more common? And... what about
the execlist backend staying without this? Yep, execlist right now is
not very functioal, but should we be intentionally breaking it?

> }
>
> static const struct xe_engine_ops execlist_engine_ops = {
>diff --git a/drivers/gpu/drm/xe/xe_guc_ads.c b/drivers/gpu/drm/xe/xe_guc_ads.c
>index 0c08cecaca40..a233023a6616 100644
>--- a/drivers/gpu/drm/xe/xe_guc_ads.c
>+++ b/drivers/gpu/drm/xe/xe_guc_ads.c
>@@ -430,6 +430,7 @@ static unsigned int guc_mmio_regset_write(struct xe_guc_ads *ads,
> 					  struct iosys_map *regset_map,
> 					  struct xe_hw_engine *hwe)
> {
>+	struct xe_device *xe = ads_to_xe(ads);
> 	struct xe_hw_engine *hwe_rcs_reset_domain =
> 		xe_gt_any_hw_engine_by_reset_domain(hwe->gt, XE_ENGINE_CLASS_RENDER);
> 	struct xe_reg_sr_entry *entry;
>@@ -464,9 +465,12 @@ static unsigned int guc_mmio_regset_write(struct xe_guc_ads *ads,
> 					  e->reg, e->flags, count++);
> 	}
>
>-	for (i = 0; i < LNCFCMOCS_REG_COUNT; i++) {
>-		guc_mmio_regset_write_one(ads, regset_map,
>-					  GEN9_LNCFCMOCS(i).reg, 0, count++);
>+	/* Wa_1607983814 */
>+	if (GRAPHICS_VER(xe) == 12 && GRAPHICS_VERx100(xe) < 1250) {
>+		for (i = 0; i < LNCFCMOCS_REG_COUNT; i++) {
>+			guc_mmio_regset_write_one(ads, regset_map,
>+						  GEN9_LNCFCMOCS(i).reg, 0, count++);

calculate_regset_size() unconditionally accounts for
LNCFCMOCS_REG_COUNT. Although this is just a "max", maybe we could
remove it from there by moving the if condition to a function
bool needs_wa_1607983814() { ... }

Another idea would be maybe to extend xe_rtp to allow a FUNC() not only
in the match but also in the action. We'd also need to extend it to
allow that function to apply the actions. Humn... if we need it more
than in just one place we can do that in future. For now this does the
job.


>+		}
> 	}
>
> 	XE_BUG_ON(ads->regset_size < (count * sizeof(struct guc_mmio_reg)));
>diff --git a/drivers/gpu/drm/xe/xe_guc_submit.c b/drivers/gpu/drm/xe/xe_guc_submit.c
>index a54f7f82d04d..3766b77a0d90 100644
>--- a/drivers/gpu/drm/xe/xe_guc_submit.c
>+++ b/drivers/gpu/drm/xe/xe_guc_submit.c
>@@ -1267,7 +1267,6 @@ static void guc_engine_resume(struct xe_engine *e)
>
> 	XE_BUG_ON(e->guc->suspend_pending);
>
>-	xe_mocs_init_engine(e);
> 	guc_engine_add_msg(e, msg, RESUME);
> }
>
>diff --git a/drivers/gpu/drm/xe/xe_mocs.c b/drivers/gpu/drm/xe/xe_mocs.c
>index 3b48934d99d4..7e495d699295 100644
>--- a/drivers/gpu/drm/xe/xe_mocs.c
>+++ b/drivers/gpu/drm/xe/xe_mocs.c
>@@ -507,19 +507,6 @@ static void init_l3cc_table(struct xe_gt *gt,
> 	}
> }
>
>-void xe_mocs_init_engine(const struct xe_engine *engine)
>-{
>-	struct xe_mocs_info table;
>-	unsigned int flags;
>-
>-	flags = get_mocs_settings(engine->gt->xe, &table);
>-	if (!flags)
>-		return;
>-
>-	if (flags & HAS_RENDER_L3CC && engine->class == XE_ENGINE_CLASS_RENDER)

do we have any other plans for HAS_RENDER_L3CC?  It seems it's not used.
For any error handling part we could check size, table or
unused_entries_index


Lucas De Marchi

>-		init_l3cc_table(engine->gt, &table);
>-}
>-
> void xe_mocs_init(struct xe_gt *gt)
> {
> 	struct xe_mocs_info table;
>diff --git a/drivers/gpu/drm/xe/xe_mocs.h b/drivers/gpu/drm/xe/xe_mocs.h
>index aba1abe216ab..63500a1d6660 100644
>--- a/drivers/gpu/drm/xe/xe_mocs.h
>+++ b/drivers/gpu/drm/xe/xe_mocs.h
>@@ -11,7 +11,6 @@
> struct xe_engine;
> struct xe_gt;
>
>-void xe_mocs_init_engine(const struct xe_engine *engine);
> void xe_mocs_init(struct xe_gt *gt);
>
> /**
>-- 
>2.39.1
>