On Wed, Apr 15, 2020 at 07:28:18AM -0700, Sripada, Radhakrishna wrote: > Hi Matt, > > > -----Original Message----- > > From: Roper, Matthew D <matthew.d.roper@xxxxxxxxx> > > Sent: Tuesday, April 14, 2020 11:40 AM > > To: Sripada, Radhakrishna <radhakrishna.sripada@xxxxxxxxx> > > Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx > > 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@xxxxxxxxx> > > > Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada@xxxxxxxxx> > > > --- > > > 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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
