| Message ID | 55FAB434.4030304@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Sep 17, 2015 at 02:38:12PM +0200, Maarten Lankhorst wrote: > Hey, > > It's worh nothing this series may have a soft dependency on Ville's cleanups, > because else pixel_rate == 0 during boot. :( > > This patch breaks when the initial sprite watermarks are set on ironlake, but no sprite is enabled. > > A naive fix is below, without it my laptop won't power the screen because intermediate wm calculation fails. :( > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index 41a4dbaf600b..bf874ff97024 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -2457,7 +2457,7 @@ static int ilk_compute_intermediate_wm(struct drm_device *dev, > > a_wm->enable &= b_wm->enable; > a_wm->pri_val = max(a_wm->pri_val, b_wm->pri_val); > - a_wm->spr_val = max(a_wm->spr_val, b_wm->spr_val); > + a_wm->spr_val = a->sprites_enabled ? max(a_wm->spr_val, b_wm->spr_val) : 0; > a_wm->cur_val = max(a_wm->cur_val, b_wm->cur_val); > a_wm->fbc_val = max(a_wm->fbc_val, b_wm->fbc_val); > } Hmm. That could work, but feels a bit wrong. Would be nice if we could always keep the watermarks totally consistent with the plane configuration. One thing we could do during init is simply calculate the optimal watermarks for the current config and just blast them in. Then when the first plane update/modeset happens the current state would actually be sane wrt. the current plane config. > > > Op 15-09-15 om 04:19 schreef Matt Roper: > > <snip> > > @@ -13202,6 +13229,20 @@ static int intel_atomic_commit(struct drm_device *dev, > > /* FIXME: add subpixel order */ > > > > drm_atomic_helper_wait_for_vblanks(dev, state); > > + > > + /* > > + * Now that the vblank has passed, we can go ahead and program the > > + * optimal watermarks on platforms that need two-step watermark > > + * programming. > > + * > > + * TODO: Move this (and other cleanup) to an async worker eventually. > > + */ > > + for_each_crtc_in_state(state, crtc, crtc_state, i) { > > + intel_cstate = to_intel_crtc_state(crtc->state); > > + > > + dev_priv->display.optimize_watermarks(intel_cstate); > > + } > ^Breaks on my skylake with a null pointer deref; I think you might want to hide it behind a if. :-) > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Op 17-09-15 om 15:05 schreef Ville Syrjälä: > On Thu, Sep 17, 2015 at 02:38:12PM +0200, Maarten Lankhorst wrote: >> Hey, >> >> It's worh nothing this series may have a soft dependency on Ville's cleanups, >> because else pixel_rate == 0 during boot. :( >> >> This patch breaks when the initial sprite watermarks are set on ironlake, but no sprite is enabled. >> >> A naive fix is below, without it my laptop won't power the screen because intermediate wm calculation fails. :( >> >> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c >> index 41a4dbaf600b..bf874ff97024 100644 >> --- a/drivers/gpu/drm/i915/intel_pm.c >> +++ b/drivers/gpu/drm/i915/intel_pm.c >> @@ -2457,7 +2457,7 @@ static int ilk_compute_intermediate_wm(struct drm_device *dev, >> >> a_wm->enable &= b_wm->enable; >> a_wm->pri_val = max(a_wm->pri_val, b_wm->pri_val); >> - a_wm->spr_val = max(a_wm->spr_val, b_wm->spr_val); >> + a_wm->spr_val = a->sprites_enabled ? max(a_wm->spr_val, b_wm->spr_val) : 0; >> a_wm->cur_val = max(a_wm->cur_val, b_wm->cur_val); >> a_wm->fbc_val = max(a_wm->fbc_val, b_wm->fbc_val); >> } > Hmm. That could work, but feels a bit wrong. Would be nice if we could > always keep the watermarks totally consistent with the plane > configuration. > > One thing we could do during init is simply calculate the optimal > watermarks for the current config and just blast them in. Then when > the first plane update/modeset happens the current state would > actually be sane wrt. the current plane config. > That would work too, as long as we end up with something sane I'm all for it. :-)
On Thu, Sep 17, 2015 at 03:24:00PM +0200, Maarten Lankhorst wrote: > Op 17-09-15 om 15:05 schreef Ville Syrjälä: > > On Thu, Sep 17, 2015 at 02:38:12PM +0200, Maarten Lankhorst wrote: > >> Hey, > >> > >> It's worh nothing this series may have a soft dependency on Ville's cleanups, > >> because else pixel_rate == 0 during boot. :( > >> > >> This patch breaks when the initial sprite watermarks are set on ironlake, but no sprite is enabled. > >> > >> A naive fix is below, without it my laptop won't power the screen because intermediate wm calculation fails. :( > >> > >> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > >> index 41a4dbaf600b..bf874ff97024 100644 > >> --- a/drivers/gpu/drm/i915/intel_pm.c > >> +++ b/drivers/gpu/drm/i915/intel_pm.c > >> @@ -2457,7 +2457,7 @@ static int ilk_compute_intermediate_wm(struct drm_device *dev, > >> > >> a_wm->enable &= b_wm->enable; > >> a_wm->pri_val = max(a_wm->pri_val, b_wm->pri_val); > >> - a_wm->spr_val = max(a_wm->spr_val, b_wm->spr_val); > >> + a_wm->spr_val = a->sprites_enabled ? max(a_wm->spr_val, b_wm->spr_val) : 0; > >> a_wm->cur_val = max(a_wm->cur_val, b_wm->cur_val); > >> a_wm->fbc_val = max(a_wm->fbc_val, b_wm->fbc_val); > >> } > > Hmm. That could work, but feels a bit wrong. Would be nice if we could > > always keep the watermarks totally consistent with the plane > > configuration. > > > > One thing we could do during init is simply calculate the optimal > > watermarks for the current config and just blast them in. Then when > > the first plane update/modeset happens the current state would > > actually be sane wrt. the current plane config. > > > That would work too, as long as we end up with something sane I'm all for it. :-) Yeah blasting in our own preferred config is probably simpler - adding all kinds of hw state readout for this seems overkill to me. And adding special cases all over is imo bad since we're really not any good at testing all the various corner-cases ... -Daniel
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 41a4dbaf600b..bf874ff97024 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -2457,7 +2457,7 @@ static int ilk_compute_intermediate_wm(struct drm_device *dev, a_wm->enable &= b_wm->enable; a_wm->pri_val = max(a_wm->pri_val, b_wm->pri_val); - a_wm->spr_val = max(a_wm->spr_val, b_wm->spr_val); + a_wm->spr_val = a->sprites_enabled ? max(a_wm->spr_val, b_wm->spr_val) : 0; a_wm->cur_val = max(a_wm->cur_val, b_wm->cur_val); a_wm->fbc_val = max(a_wm->fbc_val, b_wm->fbc_val); }