[Intel-gfx] [PATCH] drm/i915/icl: Update forcewake firmware ranges

Wed Apr 15 14:52:15 UTC 2020

On Wed, Apr 15, 2020 at 07:28:18AM -0700, Sripada, Radhakrishna wrote:
> Hi Matt,
> 
> > -----Original Message-----
> > From: Roper, Matthew D <matthew.d.roper at intel.com>
> > Sent: Tuesday, April 14, 2020 11:40 AM
> > To: Sripada, Radhakrishna <radhakrishna.sripada at intel.com>
> > Cc: intel-gfx at lists.freedesktop.org
> > Subject: Re: [PATCH] drm/i915/icl: Update forcewake firmware ranges
> > 
> > On Mon, Apr 13, 2020 at 02:00:03AM -0700, Radhakrishna Sripada wrote:
> > > Some workarounds are not sticking across suspend resume cycles. The
> > > forcewake ranges table has been updated and would reflect the hardware
> > > appropriately.
> > >
> > > Closes: https://gitlab.freedesktop.org/drm/intel/issues/1222
> > 
> > It's unfortunate that they still haven't updated the bspec for this yet.
> > A few comments below based on my understanding of the document we
> > received from the hardware team.
> > 
> > >
> > > Cc: Matt Roper <matthew.d.roper at intel.com>
> > > Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada at intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_uncore.c | 55
> > > +++++++++++++++++++++--------
> > >  1 file changed, 41 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_uncore.c
> > > b/drivers/gpu/drm/i915/intel_uncore.c
> > > index fa86b7ab2d99..c0e21697a44c 100644
> > > --- a/drivers/gpu/drm/i915/intel_uncore.c
> > > +++ b/drivers/gpu/drm/i915/intel_uncore.c
> > > @@ -1098,32 +1098,59 @@ static const struct intel_forcewake_range
> > > __gen11_fw_ranges[] = {
> > 
> > Looks like the first range in this table (0x0 - 0xaff) needs to be changed to '0' (or
> > rather combined with the following entry for a combined 0x0 - 0x1fff range set
> > to '0')
> 
> Comparing with previous gens, my understanding is the '0' is indicative of uncore range
> Of registers. The reserved ranges are marked with "Blitter" force wake range. Am I in the right
> Path making that assumption? Hence made all the subsequent changes according to the assumption.

The final parameter of GEN_FW_RANGE() is a bitmask of domains that we
need to make sure are powered up when we're going to access a register
in that range.  So a 0 means no domains need to be powered up for
registers in that range.  That might be because it's a range that
contains no registers at all (reserved range), or it may be that the
registers in that range simply aren't part of the blitter, render,
media, etc.  power wells and we don't have to worry about powering
anything up.

In cases where there are no actual registers (i.e., reserved ranges), it
doesn't really matter what you put since there shouldn't be any register
reads/writes in that range.  So for those we'll often combine them with
one of the surrounding ranges just to keep the overall forcewake table
smaller and make lookups faster.

Matt

> 
> Thanks,
> Radhakrishna(RK) Sripada
> > 
> > 
> > >  	GEN_FW_RANGE(0x2700, 0x2fff, FORCEWAKE_BLITTER),
> > >  	GEN_FW_RANGE(0x3000, 0x3fff, FORCEWAKE_RENDER),
> > >  	GEN_FW_RANGE(0x4000, 0x51ff, FORCEWAKE_BLITTER),
> > > -	GEN_FW_RANGE(0x5200, 0x7fff, FORCEWAKE_RENDER),
> > > +	GEN_FW_RANGE(0x5200, 0x53ff, FORCEWAKE_RENDER),
> > > +	GEN_FW_RANGE(0x5400, 0x54ff, FORCEWAKE_BLITTER),
> > 
> > In the version of the document I'm looking at, the wake target for this entry is
> > blank, which usually means no wake target is required.  AFAICS, no registers
> > actually fall in this range on gen11, so I'd either set this to '0' to explicitly match
> > the document, or just leave it combined with the two surrounding render ranges
> > for simplicity (and smaller table size).
> > 
> > > +	GEN_FW_RANGE(0x5500, 0x7fff, FORCEWAKE_RENDER),
> > >  	GEN_FW_RANGE(0x8000, 0x813f, FORCEWAKE_BLITTER),
> > >  	GEN_FW_RANGE(0x8140, 0x815f, FORCEWAKE_RENDER),
> > >  	GEN_FW_RANGE(0x8160, 0x82ff, FORCEWAKE_BLITTER),
> > >  	GEN_FW_RANGE(0x8300, 0x84ff, FORCEWAKE_RENDER),
> > > -	GEN_FW_RANGE(0x8500, 0x8bff, FORCEWAKE_BLITTER),
> > > +	GEN_FW_RANGE(0x8500, 0x87ff, FORCEWAKE_BLITTER),
> > > +	GEN_FW_RANGE(0x8800, 0x883f, 0),
> > > +	GEN_FW_RANGE(0x8840, 0x8bff, FORCEWAKE_BLITTER),
> > 
> > I see 0x8840 - 0x8bff as a reserved range with no forcewake target so we should
> > be able to combine it with the range before it.
> > 
> > >  	GEN_FW_RANGE(0x8c00, 0x8cff, FORCEWAKE_RENDER),
> > > -	GEN_FW_RANGE(0x8d00, 0x93ff, FORCEWAKE_BLITTER),
> > > -	GEN_FW_RANGE(0x9400, 0x97ff, FORCEWAKE_ALL),
> > > -	GEN_FW_RANGE(0x9800, 0xafff, FORCEWAKE_BLITTER),
> > > +	GEN_FW_RANGE(0x8d00, 0x94cf, FORCEWAKE_BLITTER),
> > > +	GEN_FW_RANGE(0x94d0, 0x955f, FORCEWAKE_RENDER),
> > > +	GEN_FW_RANGE(0x9560, 0x95ff, 0),
> > > +	GEN_FW_RANGE(0x9600, 0xafff, FORCEWAKE_BLITTER),
> > >  	GEN_FW_RANGE(0xb000, 0xb47f, FORCEWAKE_RENDER),
> > >  	GEN_FW_RANGE(0xb480, 0xdeff, FORCEWAKE_BLITTER),
> > >  	GEN_FW_RANGE(0xdf00, 0xe8ff, FORCEWAKE_RENDER),
> > >  	GEN_FW_RANGE(0xe900, 0x16dff, FORCEWAKE_BLITTER),
> > >  	GEN_FW_RANGE(0x16e00, 0x19fff, FORCEWAKE_RENDER),
> > > -	GEN_FW_RANGE(0x1a000, 0x243ff, FORCEWAKE_BLITTER),
> > > -	GEN_FW_RANGE(0x24400, 0x247ff, FORCEWAKE_RENDER),
> > > -	GEN_FW_RANGE(0x24800, 0x3ffff, FORCEWAKE_BLITTER),
> > > +	GEN_FW_RANGE(0x1a000, 0x23fff, FORCEWAKE_BLITTER),
> > > +	GEN_FW_RANGE(0x24000, 0x2407f, 0),
> > > +	GEN_FW_RANGE(0x24080, 0x2417f, FORCEWAKE_BLITTER),
> > > +	GEN_FW_RANGE(0x24180, 0x242ff, FORCEWAKE_RENDER),
> > > +	GEN_FW_RANGE(0x24300, 0x243ff, FORCEWAKE_BLITTER),
> > > +	GEN_FW_RANGE(0x24400, 0x245ff, FORCEWAKE_RENDER),
> > > +	GEN_FW_RANGE(0x24600, 0x2467f, FORCEWAKE_BLITTER),
> > 
> > This one is a reserved range with no registers, so we can just squash it into the
> > two render ranges on either side of it.
> > 
> > > +	GEN_FW_RANGE(0x24680, 0x247ff, FORCEWAKE_RENDER),
> > > +	GEN_FW_RANGE(0x24800, 0x249ff, FORCEWAKE_BLITTER),
> > 
> > Also reserved with no registers.
> > 
> > > +	GEN_FW_RANGE(0x24a00, 0x24a7f, FORCEWAKE_RENDER),
> > > +	GEN_FW_RANGE(0x24a80, 0x24dff, FORCEWAKE_BLITTER),
> > 
> > Also reserved with no registers.
> > 
> > So I think we should be able to have just use one render range that runs from
> > 0x24400 - 0x24fff.
> > 
> > > +	GEN_FW_RANGE(0x24e00, 0x24fff, FORCEWAKE_RENDER),
> > > +	GEN_FW_RANGE(0x25000, 0x3ffff, FORCEWAKE_BLITTER),
> > >  	GEN_FW_RANGE(0x40000, 0x1bffff, 0),
> > > -	GEN_FW_RANGE(0x1c0000, 0x1c3fff, FORCEWAKE_MEDIA_VDBOX0),
> > > -	GEN_FW_RANGE(0x1c4000, 0x1c7fff, FORCEWAKE_MEDIA_VDBOX1),
> > > -	GEN_FW_RANGE(0x1c8000, 0x1cbfff, FORCEWAKE_MEDIA_VEBOX0),
> > > +	GEN_FW_RANGE(0x1c0000, 0x1c2bff, FORCEWAKE_MEDIA_VDBOX0),
> > > +	GEN_FW_RANGE(0x1c2c00, 0x1c3eff, FORCEWAKE_BLITTER),
> > 
> > This is another reserved range that can be combined into the ranges around it
> > (0x1c0000 - 0x1c3fff).
> > 
> > > +	GEN_FW_RANGE(0x1c3f00, 0x1c3fff, FORCEWAKE_MEDIA_VDBOX0),
> > > +	GEN_FW_RANGE(0x1c4000, 0x1c6bff, FORCEWAKE_MEDIA_VDBOX1),
> > > +	GEN_FW_RANGE(0x1c6c00, 0x1c7eff, FORCEWAKE_BLITTER),
> > > +	GEN_FW_RANGE(0x1c7f00, 0x1c7fff, FORCEWAKE_MEDIA_VDBOX1),
> > 
> > 0x1c4000 - 0x1c7fff are reserved for vdbox1, but that doesn't exist on this
> > platform (we only have vdbox0 and vdbox2 on ICL and only vdbox0 on EHL/JSL).
> > So the three entries above should be combined into a single range with target '0'
> > here.
> > 
> > > +	GEN_FW_RANGE(0x1c8000, 0x1ca0ff, FORCEWAKE_MEDIA_VEBOX0),
> > > +	GEN_FW_RANGE(0x1ca100, 0x1cbeff, FORCEWAKE_BLITTER),
> > 
> > Another reserved range; can squash with the two surrounding ranges.
> > 
> > > +	GEN_FW_RANGE(0x1cbf00, 0x1cbfff, FORCEWAKE_MEDIA_VEBOX0),
> > >  	GEN_FW_RANGE(0x1cc000, 0x1cffff, FORCEWAKE_BLITTER),
> > 
> > Another reserved range.  Either set to 0 or combine with one of the ranges
> > before/after it.
> > 
> > > -	GEN_FW_RANGE(0x1d0000, 0x1d3fff, FORCEWAKE_MEDIA_VDBOX2),
> > > -	GEN_FW_RANGE(0x1d4000, 0x1d7fff, FORCEWAKE_MEDIA_VDBOX3),
> > > -	GEN_FW_RANGE(0x1d8000, 0x1dbfff, FORCEWAKE_MEDIA_VEBOX1)
> > > +	GEN_FW_RANGE(0x1d0000, 0x1d2bff, FORCEWAKE_MEDIA_VDBOX2),
> > > +	GEN_FW_RANGE(0x1d2c00, 0x1d3eff, FORCEWAKE_BLITTER),
> > 
> > Reserved range; can squash with ranges around it.
> > 
> > > +	GEN_FW_RANGE(0x1d3f00, 0x1d3fff, FORCEWAKE_MEDIA_VDBOX2),
> > > +	GEN_FW_RANGE(0x1d4000, 0x1d6bff, FORCEWAKE_MEDIA_VDBOX3),
> > > +	GEN_FW_RANGE(0x1d6c00, 0x1d7eff, FORCEWAKE_BLITTER),
> > > +	GEN_FW_RANGE(0x1d7f00, 0x1d7fff, FORCEWAKE_MEDIA_VDBOX3),
> > > +	GEN_FW_RANGE(0x1d8000, 0x1da0ff, FORCEWAKE_MEDIA_VEBOX1),
> > > +	GEN_FW_RANGE(0x1da100, 0x1dbeff, FORCEWAKE_BLITTER),
> > > +	GEN_FW_RANGE(0x1dbf00, 0x1dbfff, FORCEWAKE_MEDIA_VEBOX1)
> > 
> > As noted above, gen11 doesn't have vdbox1 or vdbox3, so these entries should
> > just be set to '0' (including the reserved ranges that you have listed as blitter
> > right now).
> > 
> > >  };
> > >
> > >  /* *Must* be sorted by offset ranges! See intel_fw_table_check(). */
> > > --
> > > 2.20.1
> > >
> > 
> > --
> > Matt Roper
> > Graphics Software Engineer
> > VTT-OSGC Platform Enablement
> > Intel Corporation
> > (916) 356-2795

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