| Message ID | 1465510495-30302-2-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: > intel_state->active_crtcs is usually only initialized when doing a > modeset. During our first atomic commit after boot, we're effectively > faking a modeset to sanitize the DDB/wm setup, so ensure that this field > gets initialized before use. > > 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 | 11 ++++++++++- > 1 file changed, 10 insertions(+), 1 deletion(-) Ah right. Code assumes that it's valid. However this still leaves open a hole in case a modeset changes active_crtcs during the first commit. intel_state->active_crtcs is set at the same time intel_state->modeset is, so resend with if (!intel_state->modeset) intel_state->active_crtcs = dev_priv->active_crtcs; ? ~Maarten
Op 13-06-16 om 10:59 schreef Maarten Lankhorst: > Op 10-06-16 om 00:14 schreef Matt Roper: >> intel_state->active_crtcs is usually only initialized when doing a >> modeset. During our first atomic commit after boot, we're effectively >> faking a modeset to sanitize the DDB/wm setup, so ensure that this field >> gets initialized before use. >> >> 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 | 11 ++++++++++- >> 1 file changed, 10 insertions(+), 1 deletion(-) > Ah right. Code assumes that it's valid. > > However this still leaves open a hole in case a modeset changes active_crtcs during the first commit. > > intel_state->active_crtcs is set at the same time intel_state->modeset is, so > resend with if (!intel_state->modeset) intel_state->active_crtcs = dev_priv->active_crtcs; ? Hm you need connection_mutex lock for that one, so need to lock that first before assigning active_crtcs. You can grab that lock unconditionally, since we already hold it in case of modeset.. ~Maarten
On Thu, Jun 09, 2016 at 03:14:53PM -0700, Matt Roper wrote: > intel_state->active_crtcs is usually only initialized when doing a > modeset. During our first atomic commit after boot, we're effectively > faking a modeset to sanitize the DDB/wm setup, so ensure that this field > gets initialized before use. > > 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 | 11 ++++++++++- > 1 file changed, 10 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index 658a756..0cd38ca 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -3896,9 +3896,18 @@ skl_compute_ddb(struct drm_atomic_state *state) > * pretend that all pipes switched active status so that we'll > * ensure a full DDB recompute. > */ > - if (dev_priv->wm.distrust_bios_wm) > + if (dev_priv->wm.distrust_bios_wm) { > intel_state->active_pipe_changes = ~0; > > + /* > + * We usually only initialize intel_state->active_crtcs if we > + * we're doing a modeset; make sure this field is always > + * initialized during the sanitization process that happens > + * on the first commit too. > + */ > + intel_state->active_crtcs = dev_priv->active_crtcs; > + } Can't we move input sanitization out of this? Imo mixing up atomic check/compute config code with hw state restoring is way too fragile. Also, why exactly do we have active_crtcs? Seems to just be duplicated state that can get out of sync, we have too many of those already ... -Daniel > + > /* > * If the modeset changes which CRTC's are active, we need to > * recompute the DDB allocation for *all* active pipes, even > -- > 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:49:49PM +0200, Daniel Vetter wrote: > On Thu, Jun 09, 2016 at 03:14:53PM -0700, Matt Roper wrote: > > intel_state->active_crtcs is usually only initialized when doing a > > modeset. During our first atomic commit after boot, we're effectively > > faking a modeset to sanitize the DDB/wm setup, so ensure that this field > > gets initialized before use. > > > > 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 | 11 ++++++++++- > > 1 file changed, 10 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > > index 658a756..0cd38ca 100644 > > --- a/drivers/gpu/drm/i915/intel_pm.c > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > @@ -3896,9 +3896,18 @@ skl_compute_ddb(struct drm_atomic_state *state) > > * pretend that all pipes switched active status so that we'll > > * ensure a full DDB recompute. > > */ > > - if (dev_priv->wm.distrust_bios_wm) > > + if (dev_priv->wm.distrust_bios_wm) { > > intel_state->active_pipe_changes = ~0; > > > > + /* > > + * We usually only initialize intel_state->active_crtcs if we > > + * we're doing a modeset; make sure this field is always > > + * initialized during the sanitization process that happens > > + * on the first commit too. > > + */ > > + intel_state->active_crtcs = dev_priv->active_crtcs; > > + } > > Can't we move input sanitization out of this? Imo mixing up atomic > check/compute config code with hw state restoring is way too fragile. Handling this at readout time is tricky since we don't actually reconstruct things like framebuffers until much later in the process. Also, if I remember correctly, we don't actually read out sufficient hardware state on any platform right now (e.g., non-primary planes aren't read, gen9 plane scalers and such are never read, etc.). So to truly sanitize properly/safely, we need to run through a real atomic commit to make sure that our sanitized values actually match how the hardware is programmed. I decided to do the sanitization steps as part of the first real commit rather than trigger an extra "modeset" on driver boot, but I can look at the other approach if you think it's better. Long term we should probably try harder to read out more complete hardware state, but for now I figured it was better to get a short-term fix since there are real regressions (as reported by Tvrtko) and I might not have time to do a more complicated hw-readout rework series for a little while. > > Also, why exactly do we have active_crtcs? Seems to just be duplicated > state that can get out of sync, we have too many of those already ... > -Daniel I think it would be harder to reconstruct this information without the active_crtcs field. You'd have to grab the CRTC state (and related locks) for CRTC's that weren't actually part of the current commit. Technically we have to do that anyway on gen9 for DDB allocation reasons, but I don't think that's the case for other platforms iirc. Matt > > > + > > /* > > * If the modeset changes which CRTC's are active, we need to > > * recompute the DDB allocation for *all* active pipes, even > > -- > > 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:00:02PM -0700, Matt Roper wrote: > On Mon, Jun 13, 2016 at 04:49:49PM +0200, Daniel Vetter wrote: > > On Thu, Jun 09, 2016 at 03:14:53PM -0700, Matt Roper wrote: > > > intel_state->active_crtcs is usually only initialized when doing a > > > modeset. During our first atomic commit after boot, we're effectively > > > faking a modeset to sanitize the DDB/wm setup, so ensure that this field > > > gets initialized before use. > > > > > > 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 | 11 ++++++++++- > > > 1 file changed, 10 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > > > index 658a756..0cd38ca 100644 > > > --- a/drivers/gpu/drm/i915/intel_pm.c > > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > > @@ -3896,9 +3896,18 @@ skl_compute_ddb(struct drm_atomic_state *state) > > > * pretend that all pipes switched active status so that we'll > > > * ensure a full DDB recompute. > > > */ > > > - if (dev_priv->wm.distrust_bios_wm) > > > + if (dev_priv->wm.distrust_bios_wm) { > > > intel_state->active_pipe_changes = ~0; > > > > > > + /* > > > + * We usually only initialize intel_state->active_crtcs if we > > > + * we're doing a modeset; make sure this field is always > > > + * initialized during the sanitization process that happens > > > + * on the first commit too. > > > + */ > > > + intel_state->active_crtcs = dev_priv->active_crtcs; > > > + } > > > > Can't we move input sanitization out of this? Imo mixing up atomic > > check/compute config code with hw state restoring is way too fragile. > > Handling this at readout time is tricky since we don't actually > reconstruct things like framebuffers until much later in the process. > Also, if I remember correctly, we don't actually read out sufficient > hardware state on any platform right now (e.g., non-primary planes > aren't read, gen9 plane scalers and such are never read, etc.). So to > truly sanitize properly/safely, we need to run through a real atomic > commit to make sure that our sanitized values actually match how the > hardware is programmed. I decided to do the sanitization steps as part > of the first real commit rather than trigger an extra "modeset" on > driver boot, but I can look at the other approach if you think it's > better. > > Long term we should probably try harder to read out more complete > hardware state, but for now I figured it was better to get a short-term > fix since there are real regressions (as reported by Tvrtko) and I might > not have time to do a more complicated hw-readout rework series for a > little while. I'm just unhappy with gunking up the atomic check/commit paths. Could we stuff this into the plane state readout/fixup paths instead? That also has the benefit of keeping all the readout logic more together in one place. > > Also, why exactly do we have active_crtcs? Seems to just be duplicated > > state that can get out of sync, we have too many of those already ... > > -Daniel > > I think it would be harder to reconstruct this information without the > active_crtcs field. You'd have to grab the CRTC state (and related > locks) for CRTC's that weren't actually part of the current commit. > Technically we have to do that anyway on gen9 for DDB allocation > reasons, but I don't think that's the case for other platforms iirc. Yeah, if the use case is strictly read-only (like we have active_planes on the crtc) then I think it's ok. Might even be something the core helpers could/should track for us in drm_atomic_state. But for that we first need to create a real drm_device->mode_config->state pointer. Seems a bit excessive for just active_crtcs. -Daniel > > > Matt > > > > > > + > > > /* > > > * If the modeset changes which CRTC's are active, we need to > > > * recompute the DDB allocation for *all* active pipes, even > > > -- > > > 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 658a756..0cd38ca 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -3896,9 +3896,18 @@ skl_compute_ddb(struct drm_atomic_state *state) * pretend that all pipes switched active status so that we'll * ensure a full DDB recompute. */ - if (dev_priv->wm.distrust_bios_wm) + if (dev_priv->wm.distrust_bios_wm) { intel_state->active_pipe_changes = ~0; + /* + * We usually only initialize intel_state->active_crtcs if we + * we're doing a modeset; make sure this field is always + * initialized during the sanitization process that happens + * on the first commit too. + */ + intel_state->active_crtcs = dev_priv->active_crtcs; + } + /* * If the modeset changes which CRTC's are active, we need to * recompute the DDB allocation for *all* active pipes, even
intel_state->active_crtcs is usually only initialized when doing a modeset. During our first atomic commit after boot, we're effectively faking a modeset to sanitize the DDB/wm setup, so ensure that this field gets initialized before use. 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 | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)