[Intel-gfx] [PATCH v2] drm/i915/mcr: Hold GT forcewake during steering operations

[Matt Roper]

On Tue, Oct 24, 2023 at 02:02:17PM +0200, Andi Shyti wrote:
> Hi Matt,
> 
> On Thu, Oct 19, 2023 at 10:02:42AM -0700, Matt Roper wrote:
> > The steering control and semaphore registers are inside an "always on"
> > power domain with respect to RC6.  However there are some issues if
> > higher-level platform sleep states are entering/exiting at the same time
> > these registers are accessed.  Grabbing GT forcewake and holding it over
> > the entire lock/steer/unlock cycle ensures that those sleep states have
> > been fully exited before we access these registers.
> > 
> > This is expected to become a formally documented/numbered workaround
> > soon.
> > 
> > Note that this patch alone isn't expected to have an immediately
> > noticeable impact on MCR (mis)behavior; an upcoming pcode firmware
> > update will also be necessary to provide the other half of this
> > workaround.
> 
> right... I did try this, but so fare we hold the forcewake when
> calling mcr_lock().
> 
> > v2:
> >  - Move the forcewake inside the Xe_LPG-specific IP version check.  This
> >    should only be necessary on platforms that have a steering semaphore.
> > 
> > Fixes: 4186e2185b4f ("drm/i915/gt: Add dedicated MCR lock")
> 
> Is this the right Fixes tag? This is adding the spinlock around
> MCR, but the power domain needs to be taken independently from
> the lock... I think the right fix here is
> 
> Fixes: 3100240bf846 ("drm/i915/mtl: Add hardware-level lock for steering")

Yeah, good point, this is a better target.

> Cc: <stable at vger.kernel.org> # v6.3+

No stable kernels support MTL (even if they have some of the commits,
it's all dead code).  We don't want to tag things for stable if they
don't meet the stable kernel requirements.

> 
> When the access to the hardware was added.
> 
> BTW, given that currently we hold the forcewake already, is this
> really a fix or is this more looking at the future? Is the Fixes
> tag necessary?

I'm not 100% sure we hold forcewake in all the cases where we work with
MCR registers.  For example, some of the big ones like wa_list_apply()
don't grab forcewake until after we've already acquired the MCR
semaphore.


Matt

> 
> > Cc: Radhakrishna Sripada <radhakrishna.sripada at intel.com>
> > Cc: Jonathan Cavitt <jonathan.cavitt at intel.com>
> > Signed-off-by: Matt Roper <matthew.d.roper at intel.com>
> 
> In any case,
> 
> Reviewed-by: Andi Shyti <andi.shyti at linux.intel.com>
> 
> Andi

-- 
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation