Em Ter, 2017-11-14 às 22:34 +0200, Ville Syrjälä escreveu:
> On Tue, Nov 14, 2017 at 06:12:41PM -0200, Paulo Zanoni wrote:
> > Em Qui, 2017-11-02 às 17:17 +0200, Ville Syrjala escreveu:
> > > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > >
> > > Apparently there are some machines that put semi-sensible looking
> > > values
> > > into the stolen "reserved" base and size, except those values are
> > > actually
> > > outside the stolen memory. There is a bit in the register which
> > > supposedly could tell us whether the reserved area is even
> > > enabled or
> > > not. Let's check for that before we go trusting the base and
> > > size.
> >
> > If this is such a problem since g4x, why didn't we spot it earlier?
> > It
> > would be nice if you could explain in the commit message (or at
> > least
> > in this email) what are the consequences you're seeing that made
> > you
> > realize about this problem. Did something actually explode? I'm
> > genuinely curious.
>
> CI is getting ping-ping on the SNB shards. Apparently the BIOS leaves
> some garbage in the the base+size fields of the register, and
> sometimes
> that results in the reserved area landing inside stolen (at which
> point
> we just reduce the size of stolen and continue), and sometimes it
> lands
> somewhere outside (at which point we just disable stolen entirely).
> Hence the FBC tests sometimes find enough stolen, sometimes not.
Couldn't it be that we're failing to read the "size of stolen"
registers instead of the "stolen reserved stuff" regs? All of the
stolen registers have a history of controversy...
>
> >
> >
> > Now talking about the patch itself:
> >
> > If we're going to assume random bits instead of a full-zero in
> > (possibly) uninitialized stuff, shouldn't we first check bit 1,
> > which
> > is supposed to tell us if the whole reg is locked or not?
>
> Not sure we can trust the BIOS to always set the lock bit. I suppose
> it
> really should, but I don't recall seeing anything in the docs saying
> that not setting the lock bit would somehow disable the reserved
> area.
The way things are worded, it suggests that if the lock bit is zero,
everything else could be garbage. Bit 0 is only locked after bit 1 is
locked. It doesn't make sense to me to trust bit 0 without trusting bit
1.
So my understanding is that as long as the enable bit is set the
> hardware will prevent us from accessing the are, regardless of
> whether
> the settings have been locked down or not.
>
> This wouldn't be the first lock bit some BIOS versions forget to set
> BTW.
But it could be worth checking this now that you have access to the
machines that have the random stuff...
>
> >
> > if ((reg_val & 0x3) != 0x3)
> > ignore stolen reserved stuff;
> >
> > Anyway, this patch without my suggestions is probably better than
> > the
> > current situation so:
> > Reviewed-by: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
> > but please feel investigate the bit 1 thing and CC me on v2 or a
> > possible follow-up patch with conclusions.
> >
> >
> > > Cc: Tomi Sarvela <tomi.p.sarvela@xxxxxxxxx>
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > > ---
> > > drivers/gpu/drm/i915/i915_gem_stolen.c | 30
> > > ++++++++++++++++++++++++++++++
> > > drivers/gpu/drm/i915/i915_reg.h | 2 ++
> > > 2 files changed, 32 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c
> > > b/drivers/gpu/drm/i915/i915_gem_stolen.c
> > > index 03e7abc7e043..44a5dbc8c23b 100644
> > > --- a/drivers/gpu/drm/i915/i915_gem_stolen.c
> > > +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
> > > @@ -294,6 +294,12 @@ static void g4x_get_stolen_reserved(struct
> > > drm_i915_private *dev_priv,
> > > ELK_STOLEN_RESERVED);
> > > dma_addr_t stolen_top = dev_priv->mm.stolen_base + ggtt-
> > > > stolen_size;
> > >
> > >
> > > + if ((reg_val & G4X_STOLEN_RESERVED_ENABLE) == 0) {
> > > + *base = 0;
> > > + *size = 0;
> > > + return;
> > > + }
> > > +
> > > *base = (reg_val & G4X_STOLEN_RESERVED_ADDR2_MASK) <<
> > > 16;
> > >
> > > WARN_ON((reg_val & G4X_STOLEN_RESERVED_ADDR1_MASK) <
> > > *base);
> > > @@ -313,6 +319,12 @@ static void gen6_get_stolen_reserved(struct
> > > drm_i915_private *dev_priv,
> > > {
> > > uint32_t reg_val = I915_READ(GEN6_STOLEN_RESERVED);
> > >
> > > + if ((reg_val & GEN6_STOLEN_RESERVED_ENABLE) == 0) {
> > > + *base = 0;
> > > + *size = 0;
> > > + return;
> > > + }
> > > +
> > > *base = reg_val & GEN6_STOLEN_RESERVED_ADDR_MASK;
> > >
> > > switch (reg_val & GEN6_STOLEN_RESERVED_SIZE_MASK) {
> > > @@ -339,6 +351,12 @@ static void gen7_get_stolen_reserved(struct
> > > drm_i915_private *dev_priv,
> > > {
> > > uint32_t reg_val = I915_READ(GEN6_STOLEN_RESERVED);
> > >
> > > + if ((reg_val & GEN6_STOLEN_RESERVED_ENABLE) == 0) {
> > > + *base = 0;
> > > + *size = 0;
> > > + return;
> > > + }
> > > +
> > > *base = reg_val & GEN7_STOLEN_RESERVED_ADDR_MASK;
> > >
> > > switch (reg_val & GEN7_STOLEN_RESERVED_SIZE_MASK) {
> > > @@ -359,6 +377,12 @@ static void chv_get_stolen_reserved(struct
> > > drm_i915_private *dev_priv,
> > > {
> > > uint32_t reg_val = I915_READ(GEN6_STOLEN_RESERVED);
> > >
> > > + if ((reg_val & GEN6_STOLEN_RESERVED_ENABLE) == 0) {
> > > + *base = 0;
> > > + *size = 0;
> > > + return;
> > > + }
> > > +
> > > *base = reg_val & GEN6_STOLEN_RESERVED_ADDR_MASK;
> > >
> > > switch (reg_val & GEN8_STOLEN_RESERVED_SIZE_MASK) {
> > > @@ -387,6 +411,12 @@ static void bdw_get_stolen_reserved(struct
> > > drm_i915_private *dev_priv,
> > > uint32_t reg_val = I915_READ(GEN6_STOLEN_RESERVED);
> > > dma_addr_t stolen_top;
> > >
> > > + if ((reg_val & GEN6_STOLEN_RESERVED_ENABLE) == 0) {
> > > + *base = 0;
> > > + *size = 0;
> > > + return;
> > > + }
> > > +
> > > stolen_top = dev_priv->mm.stolen_base + ggtt-
> > > >stolen_size;
> > >
> > > *base = reg_val & GEN6_STOLEN_RESERVED_ADDR_MASK;
> > > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > > b/drivers/gpu/drm/i915/i915_reg.h
> > > index f0f8f6059652..dc2cbc428d1b 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -382,6 +382,7 @@ static inline bool
> > > i915_mmio_reg_valid(i915_reg_t
> > > reg)
> > > #define GEN8_STOLEN_RESERVED_2M (1 << 7)
> > > #define GEN8_STOLEN_RESERVED_4M (2 << 7)
> > > #define GEN8_STOLEN_RESERVED_8M (3 << 7)
> > > +#define GEN6_STOLEN_RESERVED_ENABLE (1 << 0)
> > >
> > > /* VGA stuff */
> > >
> > > @@ -3398,6 +3399,7 @@ enum i915_power_well_id {
> > > #define ELK_STOLEN_RESERVED _MMIO(MCHBAR_MIRROR_B
> > > ASE
> > > + 0x48)
> > > #define G4X_STOLEN_RESERVED_ADDR1_MASK (0xFFFF << 16)
> > > #define G4X_STOLEN_RESERVED_ADDR2_MASK (0xFFF << 4)
> > > +#define G4X_STOLEN_RESERVED_ENABLE (1 << 0)
> > >
> > > /* Memory controller frequency in MCHBAR for Haswell (possible
> > > SNB+)
> > > */
> > > #define DCLK _MMIO(MCHBAR_MIRROR_BASE_SNB + 0x5e04)
>
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
