| Message ID | 1465510495-30302-3-git-send-email-matthew.d.roper@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Op 10-06-16 om 00:14 schreef Matt Roper: > When we sanitize our DDB and watermark info during the first atomic > commit, we need to calculate the total data rate. Since we haven't > explicitly added the planes for each CRTC to our atomic state, the total > data rate calculation will try to use the cached values from a previous > commit (which are 0 since there was no previous commit); this result is > incorrect if we inherited any active planes from the BIOS. > > During our very first atomic commit, we need to explicitly add all > active planes to the atomic state to ensure that valid data rate values > are calculated for them. Subsequent commits will then have valid cached > values to fall back on. > > Reported-by: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> > Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
On Thu, Jun 09, 2016 at 03:14:54PM -0700, Matt Roper wrote: > When we sanitize our DDB and watermark info during the first atomic > commit, we need to calculate the total data rate. Since we haven't > explicitly added the planes for each CRTC to our atomic state, the total > data rate calculation will try to use the cached values from a previous > commit (which are 0 since there was no previous commit); this result is > incorrect if we inherited any active planes from the BIOS. > > During our very first atomic commit, we need to explicitly add all > active planes to the atomic state to ensure that valid data rate values > are calculated for them. Subsequent commits will then have valid cached > values to fall back on. > > Reported-by: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> > Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com> > --- > drivers/gpu/drm/i915/intel_pm.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index 0cd38ca..ba08639 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -3933,6 +3933,18 @@ skl_compute_ddb(struct drm_atomic_state *state) > if (IS_ERR(cstate)) > return PTR_ERR(cstate); > > + /* > + * If this is our first commit after hw readout, we don't have > + * valid data rate values cached. Add all planes to ensure we > + * calculate a valid data rate. > + */ > + if (dev_priv->wm.distrust_bios_wm) { > + ret = drm_atomic_add_affected_planes(state, > + &intel_crtc->base); Again, imo should be pulled out. Other wm code probably has the exact same bug. -Daniel > + if (ret) > + return ret; > + } > + > ret = skl_allocate_pipe_ddb(cstate, ddb); > if (ret) > return ret; > -- > 2.1.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Mon, Jun 13, 2016 at 04:50:31PM +0200, Daniel Vetter wrote: > On Thu, Jun 09, 2016 at 03:14:54PM -0700, Matt Roper wrote: > > When we sanitize our DDB and watermark info during the first atomic > > commit, we need to calculate the total data rate. Since we haven't > > explicitly added the planes for each CRTC to our atomic state, the total > > data rate calculation will try to use the cached values from a previous > > commit (which are 0 since there was no previous commit); this result is > > incorrect if we inherited any active planes from the BIOS. > > > > During our very first atomic commit, we need to explicitly add all > > active planes to the atomic state to ensure that valid data rate values > > are calculated for them. Subsequent commits will then have valid cached > > values to fall back on. > > > > Reported-by: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> > > Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> > > Signed-off-by: Matt Roper <matthew.d.roper@intel.com> > > --- > > drivers/gpu/drm/i915/intel_pm.c | 12 ++++++++++++ > > 1 file changed, 12 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > > index 0cd38ca..ba08639 100644 > > --- a/drivers/gpu/drm/i915/intel_pm.c > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > @@ -3933,6 +3933,18 @@ skl_compute_ddb(struct drm_atomic_state *state) > > if (IS_ERR(cstate)) > > return PTR_ERR(cstate); > > > > + /* > > + * If this is our first commit after hw readout, we don't have > > + * valid data rate values cached. Add all planes to ensure we > > + * calculate a valid data rate. > > + */ > > + if (dev_priv->wm.distrust_bios_wm) { > > + ret = drm_atomic_add_affected_planes(state, > > + &intel_crtc->base); > > Again, imo should be pulled out. Other wm code probably has the exact same > bug. > -Daniel By "pulled out" do you mean ensure the initial data rates are calculated at readout time? As I mentioned on the other email, we don't actually read out non-primary plane states, plane scaler states, etc., so we don't actually have all the information we need to handle this at readout time; out hardware and software states aren't truly in sync until the first atomic commit happens. We should get better at reading out more hardware state, but that's a bit more involved and I've been short on upstream development time lately so I wanted to make sure I had a timely fix for the regressions Tvrtko reported. Matt > > > + if (ret) > > + return ret; > > + } > > + > > ret = skl_allocate_pipe_ddb(cstate, ddb); > > if (ret) > > return ret; > > -- > > 2.1.4 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
On Fri, Jun 17, 2016 at 02:03:07PM -0700, Matt Roper wrote: > On Mon, Jun 13, 2016 at 04:50:31PM +0200, Daniel Vetter wrote: > > On Thu, Jun 09, 2016 at 03:14:54PM -0700, Matt Roper wrote: > > > When we sanitize our DDB and watermark info during the first atomic > > > commit, we need to calculate the total data rate. Since we haven't > > > explicitly added the planes for each CRTC to our atomic state, the total > > > data rate calculation will try to use the cached values from a previous > > > commit (which are 0 since there was no previous commit); this result is > > > incorrect if we inherited any active planes from the BIOS. > > > > > > During our very first atomic commit, we need to explicitly add all > > > active planes to the atomic state to ensure that valid data rate values > > > are calculated for them. Subsequent commits will then have valid cached > > > values to fall back on. > > > > > > Reported-by: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> > > > Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> > > > Signed-off-by: Matt Roper <matthew.d.roper@intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_pm.c | 12 ++++++++++++ > > > 1 file changed, 12 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > > > index 0cd38ca..ba08639 100644 > > > --- a/drivers/gpu/drm/i915/intel_pm.c > > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > > @@ -3933,6 +3933,18 @@ skl_compute_ddb(struct drm_atomic_state *state) > > > if (IS_ERR(cstate)) > > > return PTR_ERR(cstate); > > > > > > + /* > > > + * If this is our first commit after hw readout, we don't have > > > + * valid data rate values cached. Add all planes to ensure we > > > + * calculate a valid data rate. > > > + */ > > > + if (dev_priv->wm.distrust_bios_wm) { > > > + ret = drm_atomic_add_affected_planes(state, > > > + &intel_crtc->base); > > > > Again, imo should be pulled out. Other wm code probably has the exact same > > bug. > > -Daniel > > By "pulled out" do you mean ensure the initial data rates are calculated > at readout time? As I mentioned on the other email, we don't actually > read out non-primary plane states, plane scaler states, etc., so we > don't actually have all the information we need to handle this at > readout time; out hardware and software states aren't truly in sync > until the first atomic commit happens. > > We should get better at reading out more hardware state, but that's a > bit more involved and I've been short on upstream development time > lately so I wanted to make sure I had a timely fix for the regressions > Tvrtko reported. One more I forgot in the other reply: We force-disable non-primary planes. Hence wm state for those are kinda irrelevant. Again I think it's best to group all this together with the plane readout/fb reconstruction code, so that we have all the plane readout logic in one place. I know that we already need to have the split between modeset readoud and plane reconstruction, and that's imo bad enough. If possible I'd like if we don't have to split it further into a separate plane/wm fixup path that's hiding in the atomic_check code somewhere. -Daniel > > > Matt > > > > > > + if (ret) > > > + return ret; > > > + } > > > + > > > ret = skl_allocate_pipe_ddb(cstate, ddb); > > > if (ret) > > > return ret; > > > -- > > > 2.1.4 > > > > > > _______________________________________________ > > > Intel-gfx mailing list > > > Intel-gfx@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > -- > > Daniel Vetter > > Software Engineer, Intel Corporation > > http://blog.ffwll.ch > > -- > Matt Roper > Graphics Software Engineer > IoTG Platform Enabling & Development > Intel Corporation > (916) 356-2795
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 0cd38ca..ba08639 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -3933,6 +3933,18 @@ skl_compute_ddb(struct drm_atomic_state *state) if (IS_ERR(cstate)) return PTR_ERR(cstate); + /* + * If this is our first commit after hw readout, we don't have + * valid data rate values cached. Add all planes to ensure we + * calculate a valid data rate. + */ + if (dev_priv->wm.distrust_bios_wm) { + ret = drm_atomic_add_affected_planes(state, + &intel_crtc->base); + if (ret) + return ret; + } + ret = skl_allocate_pipe_ddb(cstate, ddb); if (ret) return ret;
When we sanitize our DDB and watermark info during the first atomic commit, we need to calculate the total data rate. Since we haven't explicitly added the planes for each CRTC to our atomic state, the total data rate calculation will try to use the cached values from a previous commit (which are 0 since there was no previous commit); this result is incorrect if we inherited any active planes from the BIOS. During our very first atomic commit, we need to explicitly add all active planes to the atomic state to ensure that valid data rate values are calculated for them. Subsequent commits will then have valid cached values to fall back on. Reported-by: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> Signed-off-by: Matt Roper <matthew.d.roper@intel.com> --- drivers/gpu/drm/i915/intel_pm.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)