| Message ID | 1452789130-20878-1-git-send-email-mika.kuoppala@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jan 14, 2016 at 06:32:10PM +0200, Mika Kuoppala wrote: > CI/Bat got following (shortened) trace on byt and also > on bsw: > > ------------[ cut here ]----------- > Unclaimed register detected before reading register 0x186500 > Call Trace: > __unclaimed_reg_debug+0x68/0x80 [i915] > vlv_read32+0x2de/0x370 [i915] > intel_set_memory_cxsr+0x87/0x1a0 [i915] > intel_pre_plane_update+0xb3/0xf0 [i915] > intel_atomic_commit+0x3b5/0x17c0 [i915] > ... > ---[ end trace 6387a0ad001bb39f ]--- > > Fix this by limiting pre plane update only to active crtcs. > > References: https://bugs.freedesktop.org/show_bug.cgi?id=93698 > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index aa24f79d85bf..a134a698d97d 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -13580,9 +13580,8 @@ static int intel_atomic_commit(struct drm_device *dev, > if (!needs_modeset(crtc->state)) > continue; > > - intel_pre_plane_update(intel_crtc); > - > if (crtc_state->active) { > + intel_pre_plane_update(intel_crtc); I think you'll want to deal with the other one too (the one in the crtc enable/plane update path). Actually I think the plane update stuff should be split into a separate loop from the crtc_enable stuff, but that's a separate topic. Hmm. And there's a post_plane_update there that looks a bit too lonely as well. Something really should be done to make this code less convoluted. A modeset/plane update shouldn't be this hard to get right. > intel_crtc_disable_planes(crtc, crtc_state->plane_mask); > dev_priv->display.crtc_disable(crtc); > intel_crtc->active = false; > -- > 2.5.0
On Thu, Jan 14, 2016 at 06:32:10PM +0200, Mika Kuoppala wrote: > CI/Bat got following (shortened) trace on byt and also > on bsw: > > ------------[ cut here ]----------- > Unclaimed register detected before reading register 0x186500 > Call Trace: > __unclaimed_reg_debug+0x68/0x80 [i915] > vlv_read32+0x2de/0x370 [i915] > intel_set_memory_cxsr+0x87/0x1a0 [i915] > intel_pre_plane_update+0xb3/0xf0 [i915] > intel_atomic_commit+0x3b5/0x17c0 [i915] > ... > ---[ end trace 6387a0ad001bb39f ]--- > > Fix this by limiting pre plane update only to active crtcs. > > References: https://bugs.freedesktop.org/show_bug.cgi?id=93698 > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index aa24f79d85bf..a134a698d97d 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -13580,9 +13580,8 @@ static int intel_atomic_commit(struct drm_device *dev, > if (!needs_modeset(crtc->state)) > continue; > > - intel_pre_plane_update(intel_crtc); > - > if (crtc_state->active) { > + intel_pre_plane_update(intel_crtc); Won't this change prevent us from setting up watermarks before turning on the CRTC in a disabled->enabled transition? Matt > intel_crtc_disable_planes(crtc, crtc_state->plane_mask); > dev_priv->display.crtc_disable(crtc); > intel_crtc->active = false; > -- > 2.5.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Op 14-01-16 om 17:52 schreef Ville Syrjälä: > On Thu, Jan 14, 2016 at 06:32:10PM +0200, Mika Kuoppala wrote: >> CI/Bat got following (shortened) trace on byt and also >> on bsw: >> >> ------------[ cut here ]----------- >> Unclaimed register detected before reading register 0x186500 >> Call Trace: >> __unclaimed_reg_debug+0x68/0x80 [i915] >> vlv_read32+0x2de/0x370 [i915] >> intel_set_memory_cxsr+0x87/0x1a0 [i915] >> intel_pre_plane_update+0xb3/0xf0 [i915] >> intel_atomic_commit+0x3b5/0x17c0 [i915] >> ... >> ---[ end trace 6387a0ad001bb39f ]--- >> >> Fix this by limiting pre plane update only to active crtcs. >> >> References: https://bugs.freedesktop.org/show_bug.cgi?id=93698 >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> >> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com> >> --- >> drivers/gpu/drm/i915/intel_display.c | 3 +-- >> 1 file changed, 1 insertion(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >> index aa24f79d85bf..a134a698d97d 100644 >> --- a/drivers/gpu/drm/i915/intel_display.c >> +++ b/drivers/gpu/drm/i915/intel_display.c >> @@ -13580,9 +13580,8 @@ static int intel_atomic_commit(struct drm_device *dev, >> if (!needs_modeset(crtc->state)) >> continue; >> >> - intel_pre_plane_update(intel_crtc); >> - >> if (crtc_state->active) { >> + intel_pre_plane_update(intel_crtc); > I think you'll want to deal with the other one too (the one in the crtc > enable/plane update path). Actually I think the plane update stuff > should be split into a separate loop from the crtc_enable stuff, but > that's a separate topic. > > Hmm. And there's a post_plane_update there that looks a bit too > lonely as well. Something really should be done to make this code > less convoluted. A modeset/plane update shouldn't be this hard to > get right. > > I understand the idea from this comment, but nothing in pre_plane_update should run when crtc was not active. What function is called that does? ~Maarten
Maarten Lankhorst <maarten.lankhorst@linux.intel.com> writes: > Op 14-01-16 om 17:52 schreef Ville Syrjälä: >> On Thu, Jan 14, 2016 at 06:32:10PM +0200, Mika Kuoppala wrote: >>> CI/Bat got following (shortened) trace on byt and also >>> on bsw: >>> >>> ------------[ cut here ]----------- >>> Unclaimed register detected before reading register 0x186500 >>> Call Trace: >>> __unclaimed_reg_debug+0x68/0x80 [i915] >>> vlv_read32+0x2de/0x370 [i915] >>> intel_set_memory_cxsr+0x87/0x1a0 [i915] >>> intel_pre_plane_update+0xb3/0xf0 [i915] >>> intel_atomic_commit+0x3b5/0x17c0 [i915] >>> ... >>> ---[ end trace 6387a0ad001bb39f ]--- >>> >>> Fix this by limiting pre plane update only to active crtcs. >>> >>> References: https://bugs.freedesktop.org/show_bug.cgi?id=93698 >>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> >>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >>> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com> >>> --- >>> drivers/gpu/drm/i915/intel_display.c | 3 +-- >>> 1 file changed, 1 insertion(+), 2 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >>> index aa24f79d85bf..a134a698d97d 100644 >>> --- a/drivers/gpu/drm/i915/intel_display.c >>> +++ b/drivers/gpu/drm/i915/intel_display.c >>> @@ -13580,9 +13580,8 @@ static int intel_atomic_commit(struct drm_device *dev, >>> if (!needs_modeset(crtc->state)) >>> continue; >>> >>> - intel_pre_plane_update(intel_crtc); >>> - >>> if (crtc_state->active) { >>> + intel_pre_plane_update(intel_crtc); >> I think you'll want to deal with the other one too (the one in the crtc >> enable/plane update path). Actually I think the plane update stuff >> should be split into a separate loop from the crtc_enable stuff, but >> that's a separate topic. >> >> Hmm. And there's a post_plane_update there that looks a bit too >> lonely as well. Something really should be done to make this code >> less convoluted. A modeset/plane update shouldn't be this hard to >> get right. >> >> > I understand the idea from this comment, but nothing in pre_plane_update should run when crtc was not active. What function is called that does? > intel_set_memory_cxsr() did the access with all the power wells off. So perhaps my commit message was off. Don't know if crtc state is what matters, but evidence points to the powers being off regardless. -Mika > ~Maarten
On Mon, Jan 18, 2016 at 11:23:21AM +0100, Maarten Lankhorst wrote: > Op 14-01-16 om 17:52 schreef Ville Syrjälä: > > On Thu, Jan 14, 2016 at 06:32:10PM +0200, Mika Kuoppala wrote: > >> CI/Bat got following (shortened) trace on byt and also > >> on bsw: > >> > >> ------------[ cut here ]----------- > >> Unclaimed register detected before reading register 0x186500 > >> Call Trace: > >> __unclaimed_reg_debug+0x68/0x80 [i915] > >> vlv_read32+0x2de/0x370 [i915] > >> intel_set_memory_cxsr+0x87/0x1a0 [i915] > >> intel_pre_plane_update+0xb3/0xf0 [i915] > >> intel_atomic_commit+0x3b5/0x17c0 [i915] > >> ... > >> ---[ end trace 6387a0ad001bb39f ]--- > >> > >> Fix this by limiting pre plane update only to active crtcs. > >> > >> References: https://bugs.freedesktop.org/show_bug.cgi?id=93698 > >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > >> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > >> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com> > >> --- > >> drivers/gpu/drm/i915/intel_display.c | 3 +-- > >> 1 file changed, 1 insertion(+), 2 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > >> index aa24f79d85bf..a134a698d97d 100644 > >> --- a/drivers/gpu/drm/i915/intel_display.c > >> +++ b/drivers/gpu/drm/i915/intel_display.c > >> @@ -13580,9 +13580,8 @@ static int intel_atomic_commit(struct drm_device *dev, > >> if (!needs_modeset(crtc->state)) > >> continue; > >> > >> - intel_pre_plane_update(intel_crtc); > >> - > >> if (crtc_state->active) { > >> + intel_pre_plane_update(intel_crtc); > > I think you'll want to deal with the other one too (the one in the crtc > > enable/plane update path). Actually I think the plane update stuff > > should be split into a separate loop from the crtc_enable stuff, but > > that's a separate topic. > > > > Hmm. And there's a post_plane_update there that looks a bit too > > lonely as well. Something really should be done to make this code > > less convoluted. A modeset/plane update shouldn't be this hard to > > get right. > > > > > I understand the idea from this comment, but nothing in pre_plane_update should run when crtc was not active. What function is called that does? Staring at the code a bit. I would assume it's due to intel_plane_atomic_calc_changes() setting 'pipe_config->disable_cxsr = true' when the plane gets either turned off or on. This sort of stuff makes me wish again that we had a separate atomic state for the disable case. Using the same state flag for both the disable and enable phases of the operation is very confusing. Would be even more confusing if we required that flag to have different values for the disable and enable phases.
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index aa24f79d85bf..a134a698d97d 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -13580,9 +13580,8 @@ static int intel_atomic_commit(struct drm_device *dev, if (!needs_modeset(crtc->state)) continue; - intel_pre_plane_update(intel_crtc); - if (crtc_state->active) { + intel_pre_plane_update(intel_crtc); intel_crtc_disable_planes(crtc, crtc_state->plane_mask); dev_priv->display.crtc_disable(crtc); intel_crtc->active = false;
CI/Bat got following (shortened) trace on byt and also on bsw: ------------[ cut here ]----------- Unclaimed register detected before reading register 0x186500 Call Trace: __unclaimed_reg_debug+0x68/0x80 [i915] vlv_read32+0x2de/0x370 [i915] intel_set_memory_cxsr+0x87/0x1a0 [i915] intel_pre_plane_update+0xb3/0xf0 [i915] intel_atomic_commit+0x3b5/0x17c0 [i915] ... ---[ end trace 6387a0ad001bb39f ]--- Fix this by limiting pre plane update only to active crtcs. References: https://bugs.freedesktop.org/show_bug.cgi?id=93698 Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com> --- drivers/gpu/drm/i915/intel_display.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)