Re: [Intel-xe] [PATCH 2/7] drm/xe/guc: Handle RCU_MODE as masked from definition

[Rodrigo Vivi <rodrigo.vivi@kernel.org>]

On Fri, May 05, 2023 at 10:08:48AM -0700, Lucas De Marchi wrote:
> On Fri, May 05, 2023 at 12:55:28PM -0400, Rodrigo Vivi wrote:
> > On Fri, Apr 28, 2023 at 11:23:27PM -0700, Lucas De Marchi wrote:
> > > guc_mmio_regset_write() had a flags for the registers to be added to the
> > > GuC's regset list. The only register actually using that was RCU_MODE,
> > > but it was setting the flags to a bogus value. From
> > > struct xe_guc_fwif.h,
> > > 
> > > 	#define GUC_REGSET_MASKED               BIT(0)
> > > 	#define GUC_REGSET_MASKED_WITH_VALUE    BIT(2)
> > > 	#define GUC_REGSET_RESTORE_ONLY         BIT(3)
> > > 
> > > Cross checking with i915, the only flag to set in RCU_MODE is
> > > GUC_REGSET_MASKED. That can be done automatically from the register, as
> > > long as the definition is correct.
> > > 
> > > Add the XE_REG_OPTION_MASKED annotation to RCU_MODE and kill the "flags"
> > > field in guc_mmio_regset_write(): guc_mmio_regset_write_one() can decide
> > > that based on the register being passed.
> > > 
> > > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> > 
> > I'm still trying to get familiarized with XE_REG_OPTION_MASKED
> > but this patch looks right to me:
> 
> basically the definition if the register is masked or not comes from
> bspec: if the spec says the upper 16bits are the mask, we add the define
> in the header saying so.
> 
> XE_REG(1) -> creates a normal register with no flags/options
> XE_REG(1, XE_REG_OPTION_MASKED) -> creates a masked register
> XE_REG_MCR(1) -> creates an MCR register: here it applies both a
> flag/option (for places that deal with that in runtime, e.g. RTP
> tables), and a different type, for the rest of the driver.

it makes sense! thanks for the explanation!

> 
> it will be much easier to eventually convert to an auto-generated
> header so these options come directly from the spec, which is my
> master plan here :)

\o/
ack!

> 
> > 
> > 
> > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> 
> thanks
> Lucas De Marchi