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 23, 2023 at 09:37:15AM -0800, Matt Roper wrote:
>On Wed, Feb 22, 2023 at 04:11:19PM -0800, Lucas De Marchi wrote:
>> 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?
>
>I assume "NIY" means "not in yet."  That comment is what was used for
>several other unimplemented stubs in this same file, so I just did the
>same here for consistency.

oh... I think this is "not implemented yet". Ok, so it's not a nop, it's
more a "TODO" if this is ever really done.

>
>Removing this shouldn't have any detrimental impact on the current
>execlist implementation.  The programming here is only necessary to
>restore after a single-engine reset, but it's not possible to ever have
>an engine reset today because that was never implemented in Xe (and as
>far as I know, never will be).

ok

thanks
Lucas De Marchi

>
>>
>> > }
>> >
>> > 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() { ... }
>
>Yeah, good point; I'll update that.
>
>
>Matt
>
>>
>> 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
>> >
>
>-- 
>Matt Roper
>Graphics Software Engineer
>Linux GPU Platform Enablement
>Intel Corporation