> -----Original Message----- > From: Roper, Matthew D <matthew.d.roper@xxxxxxxxx> > Sent: Wednesday, April 15, 2020 7:52 AM > To: Sripada, Radhakrishna <radhakrishna.sripada@xxxxxxxxx> > Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx > Subject: Re: [PATCH] drm/i915/icl: Update forcewake firmware ranges > > 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. > Thank you for the explanation Matt. I will update the patch and send the next version. - RK > > 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
