[Intel-gfx] [PATCH] drm/i915/gvt: Implement WaForceWakeRenderDuringMmioTLBInvalidate

[Zhenyu Wang]

On 2016.10.20 17:20:04 +0200, Arkadiusz Hiler wrote:
> On Thu, Oct 20, 2016 at 05:29:36PM +0300, Mika Kuoppala wrote:
> > Arkadiusz Hiler <arkadiusz.hiler at intel.com> writes:
> > 
> > > When invalidating RCS TLB the device can enter RC6 state interrupting
> > > the process, therefore the need for render forcewake for the whole
> > > procedure.
> > >
> > > This WA is needed for all production SKL SKUs.
> > >
> > > References: HSD#2136899, HSD#1404391274
> > > Cc: Mika Kuoppala <mika.kuoppala at intel.com>
> > > Cc: Zhenyu Wang <zhenyuw at linux.intel.com>
> > > Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler at intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/gvt/render.c | 11 +++++++++++
> > >  1 file changed, 11 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/i915/gvt/render.c b/drivers/gpu/drm/i915/gvt/render.c
> > > index f54ab85..f5000ea 100644
> > > --- a/drivers/gpu/drm/i915/gvt/render.c
> > > +++ b/drivers/gpu/drm/i915/gvt/render.c
> > > @@ -134,11 +134,22 @@ static void handle_tlb_pending_event(struct intel_vgpu *vgpu, int ring_id)
> > >  
> > >  	reg = _MMIO(regs[ring_id]);
> > >
> > 
> > Ok not so familiar with the gvt side but I assume this is the host
> > side code and thus the vgpu is not active at this stage.
> 
> That's my understanding as well. It's a code that is setting up gvt for
> further use (shadow context to be exact). It's called indirectly from
> intel_gvt_create_vgpu.
> 

yes, it's for host not for vgpu to handle context switch state for vgpu.

> > Then you could avoid some of the implicit fw dancing
> > by:
> > 
> > diff --git a/drivers/gpu/drm/i915/gvt/render.c b/drivers/gpu/drm/i915/gvt/render.c
> > index feebb65..93ba156 100644
> > --- a/drivers/gpu/drm/i915/gvt/render.c
> > +++ b/drivers/gpu/drm/i915/gvt/render.c
> > @@ -118,6 +118,7 @@ static u32 gen9_render_mocs_L3[32];
> >  static void handle_tlb_pending_event(struct intel_vgpu *vgpu, int ring_id)
> >  {
> >         struct drm_i915_private *dev_priv = vgpu->gvt->dev_priv;
> > +       enum forcewake_domains fw;
> >         i915_reg_t reg;
> >         u32 regs[] = {
> >                 [RCS] = 0x4260,
> > @@ -135,11 +136,21 @@ static void handle_tlb_pending_event(struct intel_vgpu *vgpu, int ring_id)
> >  
> >         reg = _MMIO(regs[ring_id]);
> >  
> > -       I915_WRITE(reg, 0x1);
> > +       fw = intel_uncore_forcewake_for_reg(dev_priv, reg,
> > +                                           FW_REG_READ | FW_REG_WRITE);
> >  
> > -       if (wait_for_atomic((I915_READ(reg) == 0), 50))
> > +       if (ring_id == RCS && IS_SKYLAKE(dev_priv))
> > +               fw |= FORCEWAKE_RENDER;
> > +
> > +       intel_uncore_forcewake_get(dev_priv, fw);
> > +
> > +       I915_WRITE_FW(reg, 0x1);
> > +
> > +       if (wait_for_atomic((I915_READ_FW(reg) == 0), 50))
> >                 gvt_err("timeout in invalidate ring (%d) tlb\n", ring_id);
> >  
> > +       intel_uncore_forcewake_put(dev_priv, fw);
> > +
> > 
> 
> I can go with it, although I do not have strong preference. I think my
> version is a little bit easier to follow, but his is less error prone,
> as you check for the WA SKU only once, during setting the FW.
> 
> Any recommendations?
> 

I like this one to be safer. Would Mika like to send another one or I
just take your commit message?

thanks

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 163 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/intel-gfx/attachments/20161021/186730ad/attachment.sig>