Re: [PATCH] drm/xe: Label RING_CONTEXT_CONTROL as masked

["Dixit, Ashutosh" <ashutosh.dixit@intel.com>]

On Thu, 04 Apr 2024 05:46:17 -0700, Lucas De Marchi wrote:
>
> On Wed, Apr 03, 2024 at 02:02:07PM -0700, Ashutosh Dixit wrote:
> > RING_CONTEXT_CONTROL is a masked register.
> >
> > Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
>
> Due to the way we end up using it, it's not a problem right now:
>
> drivers/gpu/drm/xe/xe_lrc.c-static void set_context_control(u32 *regs, struct xe_hw_engine *hwe)
> drivers/gpu/drm/xe/xe_lrc.c-{
> drivers/gpu/drm/xe/xe_lrc.c-    regs[CTX_CONTEXT_CONTROL] = _MASKED_BIT_ENABLE(CTX_CTRL_INHIBIT_SYN_CTX_SWITCH) |
> drivers/gpu/drm/xe/xe_lrc.c:                                _MASKED_BIT_DISABLE(CTX_CTRL_ENGINE_CTX_RESTORE_INHIBIT) |
> drivers/gpu/drm/xe/xe_lrc.c:                                CTX_CTRL_ENGINE_CTX_RESTORE_INHIBIT;
>
> it doesn't really use the  masked flag from the register.
> For consistency, and if we ever change the approach, it does look good.
>
> Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com>
>
> That _MASKED_BIT_DISABLE(CTX_CTRL_ENGINE_CTX_RESTORE_INHIBIT) |
> CTX_CTRL_ENGINE_CTX_RESTORE_INHIBIT looks odd. Commit
> b4b9457ae9b619c106ee464c3d75ee52c0b69575 (from xe brach, not drm-xe)
> blames me while refactoring set_context_control() that always received
> true as argument (heheh... commit message from those early days were a
> piece of art). Could you prep a patch on top changing that to
> ENABLE()?

The code actually "works". I have just added the clean-up to v2, changing this to:

	regs[CTX_CONTEXT_CONTROL] = _MASKED_BIT_ENABLE(CTX_CTRL_INHIBIT_SYN_CTX_SWITCH |
						       CTX_CTRL_ENGINE_CTX_RESTORE_INHIBIT);

Thanks.
--
Ashutosh


>
> > ---
> > drivers/gpu/drm/xe/regs/xe_engine_regs.h | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/xe/regs/xe_engine_regs.h b/drivers/gpu/drm/xe/regs/xe_engine_regs.h
> > index a08528d9c76b..af71b87d8030 100644
> > --- a/drivers/gpu/drm/xe/regs/xe_engine_regs.h
> > +++ b/drivers/gpu/drm/xe/regs/xe_engine_regs.h
> > @@ -122,7 +122,7 @@
> > #define RING_EXECLIST_STATUS_LO(base)		XE_REG((base) + 0x234)
> > #define RING_EXECLIST_STATUS_HI(base)		XE_REG((base) + 0x234 + 4)
> >
> > -#define RING_CONTEXT_CONTROL(base)		XE_REG((base) + 0x244)
> > +#define RING_CONTEXT_CONTROL(base)		XE_REG((base) + 0x244, XE_REG_OPTION_MASKED)
> > #define	  CTX_CTRL_INHIBIT_SYN_CTX_SWITCH	REG_BIT(3)
> > #define	  CTX_CTRL_ENGINE_CTX_RESTORE_INHIBIT	REG_BIT(0)
> >
> > --
> > 2.41.0
> >