Re: [Intel-gfx] [PATCH] drm/i915: Reserving some Multi-thread forcewake bits.

[Matt Roper <matthew.d.roper@intel.com>]

On Wed, Apr 13, 2022 at 05:39:27PM -0400, Rodrigo Vivi wrote:
> Bit 0: Currently bit used by i915. Ideally only i915 touches it
>        in a Linux stack.
> 
> Bits 1 and 2: A while ago we were using Bit 1 for i915 and bit 2
>        	      for the user space, until commit 7130630323c5 ("drm/i915:
> 	      Use fallback forcewake if primary ack missing") changed it
> 	      to bit 1.

That commit didn't change the bits, just the notation used to describe
them.  0x1 == BIT(0) and 0x2 == BIT(1) so no functional change.

In general userspace shouldn't ever be using forcewake and the very few
exceptions to that rule aren't using the definitions in our kernel
register header anyway so I don't see much value in trying to reserve
bits in our kernel header.  I believe the only userspace users of
forcewake are/were:

 * The old Intel-specific DDX driver (which has now been replaced by the
   vendor-agnostic xf86-video-modesetting) used to grab forcewake via
   the command streamer while waiting for scanline on hsw-gen9

        b[3] = MI_LOAD_REGISTER_IMM | 1;
        b[4] = 0xa188; /* FORCEWAKE_MT */
        b[5] = 2 << 16 | 2;

   So the usage of bit 1 (i.e., 0x2) is hardcoded into the DDX; you'd
   need to update the DDX itself if you're worried about clashes with
   pcode on those old platforms.

   Honestly I don't know if the above register update even lands...at
   least for modern platforms bspec page 45546 doesn't list 0xa188 as a
   register that userspace has permission to update via the command
   streamer (it would probably be a security concern if it was!), so
   this old DDX strategy of using an LRI instruction to update the
   register shouldn't be something we even need to consider going
   forward.

 * debug tools like intel_reg that run as root can manipulate registers
   directly, including the forcewake register.  But the bits that get
   used are up to whoever is running the tool; the definitions in i915
   code don't matter.

 * There's an "i915_forcewake_user" debugfs entry that holds forcewake
   while userspace holds an open file descriptor on it.  But usage of
   that debugfs still utilizes the FORCEWAKE_KERNEL bit rather than the
   "userspace" bit.

Since FORCEWAKE_USER is completely unused by i915, I'd suggest just
dropping the definition so that people don't even get the bad idea that
manipulating forcewake from userspace is okay.  Just leave
FORCEWAKE_KERNEL and FORCEWAKE_KERNEL_FALLBACK as the only ones defined
since from our point of view those are the only ones that matter.


Matt

> 	      Now we have a situation where PCODE is also using this bit-1
> 	      in one case, while it should actually be using the Bit-3.
> 	      So, let's redirect users back to bit-2 and mark this 1 as
> 	      reserved.
> 
> Bit 3: Let's reserve for PCODE.
> 
> Bit 4: Let's reserve for PSMI.
> 
> Cc: Tilak Tangudu <tilak.tangudu@intel.com>
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/gt/intel_gt_regs.h | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_regs.h b/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> index 0a5c2648aaf0..15ceaaace4d9 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> +++ b/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> @@ -1399,8 +1399,11 @@
>  #define FORCEWAKE_MT_ACK			_MMIO(0x130040)
>  #define FORCEWAKE_ACK_HSW			_MMIO(0x130044)
>  #define FORCEWAKE_ACK_GT_GEN9			_MMIO(0x130044)
> -#define   FORCEWAKE_KERNEL			BIT(0)
> -#define   FORCEWAKE_USER			BIT(1)
> +#define   FORCEWAKE_KERNEL			BIT(0) /* For i915 use only */
> +#define   FORCEWAKE_RSVD			BIT(1)
> +#define   FORCEWAKE_USER			BIT(2)
> +#define   FORCEWAKE_PCODE			BIT(3)
> +#define   FORCEWAKE_PSMI			BIT(4)
>  #define   FORCEWAKE_KERNEL_FALLBACK		BIT(15)
>  #define FORCEWAKE_ACK				_MMIO(0x130090)
>  #define VLV_GTLC_WAKE_CTRL			_MMIO(0x130090)
> -- 
> 2.34.1
> 

-- 
Matt Roper
Graphics Software Engineer
VTT-OSGC Platform Enablement
Intel Corporation
(916) 356-2795