| Message ID | 20171102151737.23336-3-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Em Qui, 2017-11-02 às 17:17 +0200, Ville Syrjala escreveu: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > While I have no solid proof that ILK follows the ELK path when it > comes to the stolen memory reserved area, there are some hints that > it might be the case. Unfortunately my ILK doesn't have this enabled, > and no way to enable it via the BIOS it seems. > > So let's have ILK use the ELK code path, and let's toss in a WARN > into the code to see if we catch anyone with an ILK that has this > enabled to further analyze the situation. > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/i915_gem_stolen.c | 18 +++++++++++------- > 1 file changed, 11 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c > b/drivers/gpu/drm/i915/i915_gem_stolen.c > index 4f9377b5f4ae..1877ae9a1d9b 100644 > --- a/drivers/gpu/drm/i915/i915_gem_stolen.c > +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c > @@ -300,6 +300,12 @@ static void g4x_get_stolen_reserved(struct > drm_i915_private *dev_priv, > return; > } > > + /* > + * Whether ILK really reuses the ELK register for this is > unclear. > + * Let's see if we catch anyone with this supposedly enabled > on ILK. > + */ > + WARN(IS_GEN5(dev_priv), "ILK stolen reserved found? > 0x%08x\n", reg_val); Since we're going to introduce an unconditional WARN, we may as well print the value of GEN6_STOLEN_RESERVED, just in case? Also, this will probably scare a lot of users. Maybe minimizing it to DRM_ERROR would help. We could also consider expanding the message a little bit more and explain that it's there for debugging purposes and should be reported back to us? I'll let the Maintainers make the decision on whether it's fine to add a WARN like that. Please ping them. Anyway, just like you, I don't have the documents to back up the claims of the patch, so giving a R-B tag is quite hard. Acked-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > + > *base = (reg_val & G4X_STOLEN_RESERVED_ADDR2_MASK) << 16; > > WARN_ON((reg_val & G4X_STOLEN_RESERVED_ADDR1_MASK) < *base); > @@ -466,14 +472,12 @@ int i915_gem_init_stolen(struct > drm_i915_private *dev_priv) > case 3: > break; > case 4: > - if (IS_G4X(dev_priv)) > - g4x_get_stolen_reserved(dev_priv, > - &reserved_base, > &reserved_size); > - break; > + if (!IS_G4X(dev_priv)) > + break; > + /* fall through */ > case 5: > - /* Assume the gen6 maximum for the older platforms. > */ > - reserved_size = 1024 * 1024; > - reserved_base = stolen_top - reserved_size; > + g4x_get_stolen_reserved(dev_priv, > + &reserved_base, > &reserved_size); > break; > case 6: > gen6_get_stolen_reserved(dev_priv,
On Tue, Nov 14, 2017 at 06:59:21PM -0200, Paulo Zanoni wrote: > Em Qui, 2017-11-02 às 17:17 +0200, Ville Syrjala escreveu: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > While I have no solid proof that ILK follows the ELK path when it > > comes to the stolen memory reserved area, there are some hints that > > it might be the case. Unfortunately my ILK doesn't have this enabled, > > and no way to enable it via the BIOS it seems. > > > > So let's have ILK use the ELK code path, and let's toss in a WARN > > into the code to see if we catch anyone with an ILK that has this > > enabled to further analyze the situation. > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > --- > > drivers/gpu/drm/i915/i915_gem_stolen.c | 18 +++++++++++------- > > 1 file changed, 11 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c > > b/drivers/gpu/drm/i915/i915_gem_stolen.c > > index 4f9377b5f4ae..1877ae9a1d9b 100644 > > --- a/drivers/gpu/drm/i915/i915_gem_stolen.c > > +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c > > @@ -300,6 +300,12 @@ static void g4x_get_stolen_reserved(struct > > drm_i915_private *dev_priv, > > return; > > } > > > > + /* > > + * Whether ILK really reuses the ELK register for this is > > unclear. > > + * Let's see if we catch anyone with this supposedly enabled > > on ILK. > > + */ > > + WARN(IS_GEN5(dev_priv), "ILK stolen reserved found? > > 0x%08x\n", reg_val); > > Since we're going to introduce an unconditional WARN, we may as well > print the value of GEN6_STOLEN_RESERVED, just in case? I ruled out the SNB and CTG registers already based on the results on my ILK. The SNB register doesn't exist at all (gives all 1s) which makes sense since that entire range of registers seems to have been introduced with SNB. And the value in the CTG register didn't make any sense for this. So the ELK register is the only viable candidate really. > > Also, this will probably scare a lot of users. Maybe minimizing it to > DRM_ERROR would help. We could also consider expanding the message a > little bit more and explain that it's there for debugging purposes and > should be reported back to us? If it looks too bening people might not bother reporting it ;) While it is a little nasty to do it this way, I don't really have any better ideas. Fortunately we can always kill it with cc:stable if/when we get a report, so it should die off reasonably quickly. And I guess it might also be the case that no ILK uses this anywhere. On the ELK here a BIOS update seems to have locked this down entirely and I can no longer test the modes where it was reserving stolen for this :( > > I'll let the Maintainers make the decision on whether it's fine to add > a WARN like that. Please ping them. > > Anyway, just like you, I don't have the documents to back up the claims > of the patch, so giving a R-B tag is quite hard. > > Acked-by: Paulo Zanoni <paulo.r.zanoni@intel.com> Much appereciated. > > > > > > + > > *base = (reg_val & G4X_STOLEN_RESERVED_ADDR2_MASK) << 16; > > > > WARN_ON((reg_val & G4X_STOLEN_RESERVED_ADDR1_MASK) < *base); > > @@ -466,14 +472,12 @@ int i915_gem_init_stolen(struct > > drm_i915_private *dev_priv) > > case 3: > > break; > > case 4: > > - if (IS_G4X(dev_priv)) > > - g4x_get_stolen_reserved(dev_priv, > > - &reserved_base, > > &reserved_size); > > - break; > > + if (!IS_G4X(dev_priv)) > > + break; > > + /* fall through */ > > case 5: > > - /* Assume the gen6 maximum for the older platforms. > > */ > > - reserved_size = 1024 * 1024; > > - reserved_base = stolen_top - reserved_size; > > + g4x_get_stolen_reserved(dev_priv, > > + &reserved_base, > > &reserved_size); > > break; > > case 6: > > gen6_get_stolen_reserved(dev_priv,
diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c index 4f9377b5f4ae..1877ae9a1d9b 100644 --- a/drivers/gpu/drm/i915/i915_gem_stolen.c +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c @@ -300,6 +300,12 @@ static void g4x_get_stolen_reserved(struct drm_i915_private *dev_priv, return; } + /* + * Whether ILK really reuses the ELK register for this is unclear. + * Let's see if we catch anyone with this supposedly enabled on ILK. + */ + WARN(IS_GEN5(dev_priv), "ILK stolen reserved found? 0x%08x\n", reg_val); + *base = (reg_val & G4X_STOLEN_RESERVED_ADDR2_MASK) << 16; WARN_ON((reg_val & G4X_STOLEN_RESERVED_ADDR1_MASK) < *base); @@ -466,14 +472,12 @@ int i915_gem_init_stolen(struct drm_i915_private *dev_priv) case 3: break; case 4: - if (IS_G4X(dev_priv)) - g4x_get_stolen_reserved(dev_priv, - &reserved_base, &reserved_size); - break; + if (!IS_G4X(dev_priv)) + break; + /* fall through */ case 5: - /* Assume the gen6 maximum for the older platforms. */ - reserved_size = 1024 * 1024; - reserved_base = stolen_top - reserved_size; + g4x_get_stolen_reserved(dev_priv, + &reserved_base, &reserved_size); break; case 6: gen6_get_stolen_reserved(dev_priv,