| Message ID | 1437037166-9339-10-git-send-email-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jul 16, 2015 at 10:59:22AM +0200, Maarten Lankhorst wrote: > This is handled by the atomic core now, no need to check this for ourself. > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> For all these "Remove ..." patches I think it'd be better to rewrite the changed code to use atomic state for whatever it does directly and stop using any of the legacy state (whether drm core or i915 legacy state). If we do that conversion it's possible to review whether there's any cases we're no longer checking. Trying to do that while we just rip out code makes that harder. hw state checker would then only compare hw state against atomic state, and it would be the job of update_legacy_state and friends to make sure atomic state matches up with legacy state. -Daniel > --- > drivers/gpu/drm/i915/intel_display.c | 19 +------------------ > 1 file changed, 1 insertion(+), 18 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 2ca50e589ea4..30ece88703f0 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -12715,8 +12715,7 @@ check_crtc_state(struct drm_device *dev) > struct intel_crtc_state pipe_config; > > for_each_intel_crtc(dev, crtc) { > - bool enabled = false; > - bool active = false; > + bool active; > > memset(&pipe_config, 0, sizeof(pipe_config)); > > @@ -12726,22 +12725,6 @@ check_crtc_state(struct drm_device *dev) > I915_STATE_WARN(crtc->active && !crtc->base.state->enable, > "active crtc, but not enabled in sw tracking\n"); > > - for_each_intel_encoder(dev, encoder) { > - if (encoder->base.crtc != &crtc->base) > - continue; > - enabled = true; > - if (encoder->connectors_active) > - active = true; > - } > - > - I915_STATE_WARN(active != crtc->active, > - "crtc's computed active state doesn't match tracked active state " > - "(expected %i, found %i)\n", active, crtc->active); > - I915_STATE_WARN(enabled != crtc->base.state->enable, > - "crtc's computed enabled state doesn't match tracked enabled state " > - "(expected %i, found %i)\n", enabled, > - crtc->base.state->enable); > - > active = dev_priv->display.get_pipe_config(crtc, > &pipe_config); > > -- > 2.1.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Hey, Op 16-07-15 om 11:24 schreef Daniel Vetter: > On Thu, Jul 16, 2015 at 10:59:22AM +0200, Maarten Lankhorst wrote: >> This is handled by the atomic core now, no need to check this for ourself. >> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > For all these "Remove ..." patches I think it'd be better to rewrite the > changed code to use atomic state for whatever it does directly and stop > using any of the legacy state (whether drm core or i915 legacy state). If > we do that conversion it's possible to review whether there's any cases > we're no longer checking. Trying to do that while we just rip out code > makes that harder. > > hw state checker would then only compare hw state against atomic state, > and it would be the job of update_legacy_state and friends to make sure > atomic state matches up with legacy state. > I think converting the hw state checker to take atomic state would be a lot of work, which should really be its own followup patch series.
Op 16-07-15 om 17:01 schreef Daniel Vetter: > On Thu, Jul 16, 2015 at 11:38:33AM +0200, Maarten Lankhorst wrote: >> Hey, >> >> Op 16-07-15 om 11:24 schreef Daniel Vetter: >>> On Thu, Jul 16, 2015 at 10:59:22AM +0200, Maarten Lankhorst wrote: >>>> This is handled by the atomic core now, no need to check this for ourself. >>>> >>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >>> For all these "Remove ..." patches I think it'd be better to rewrite the >>> changed code to use atomic state for whatever it does directly and stop >>> using any of the legacy state (whether drm core or i915 legacy state). If >>> we do that conversion it's possible to review whether there's any cases >>> we're no longer checking. Trying to do that while we just rip out code >>> makes that harder. >>> >>> hw state checker would then only compare hw state against atomic state, >>> and it would be the job of update_legacy_state and friends to make sure >>> atomic state matches up with legacy state. >>> >> I think converting the hw state checker to take atomic state would be a lot of work, >> which should really be its own followup patch series. > Yeah that's kinda my point, I'd like to split this off from at least the > dpms series. Or is connectors_active causing troubles? > -Daniel No, but I thought connectors_active would be a good end goal for this series. :)
On Thu, Jul 16, 2015 at 11:38:33AM +0200, Maarten Lankhorst wrote: > Hey, > > Op 16-07-15 om 11:24 schreef Daniel Vetter: > > On Thu, Jul 16, 2015 at 10:59:22AM +0200, Maarten Lankhorst wrote: > >> This is handled by the atomic core now, no need to check this for ourself. > >> > >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > For all these "Remove ..." patches I think it'd be better to rewrite the > > changed code to use atomic state for whatever it does directly and stop > > using any of the legacy state (whether drm core or i915 legacy state). If > > we do that conversion it's possible to review whether there's any cases > > we're no longer checking. Trying to do that while we just rip out code > > makes that harder. > > > > hw state checker would then only compare hw state against atomic state, > > and it would be the job of update_legacy_state and friends to make sure > > atomic state matches up with legacy state. > > > I think converting the hw state checker to take atomic state would be a lot of work, > which should really be its own followup patch series. Yeah that's kinda my point, I'd like to split this off from at least the dpms series. Or is connectors_active causing troubles? -Daniel
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 2ca50e589ea4..30ece88703f0 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -12715,8 +12715,7 @@ check_crtc_state(struct drm_device *dev) struct intel_crtc_state pipe_config; for_each_intel_crtc(dev, crtc) { - bool enabled = false; - bool active = false; + bool active; memset(&pipe_config, 0, sizeof(pipe_config)); @@ -12726,22 +12725,6 @@ check_crtc_state(struct drm_device *dev) I915_STATE_WARN(crtc->active && !crtc->base.state->enable, "active crtc, but not enabled in sw tracking\n"); - for_each_intel_encoder(dev, encoder) { - if (encoder->base.crtc != &crtc->base) - continue; - enabled = true; - if (encoder->connectors_active) - active = true; - } - - I915_STATE_WARN(active != crtc->active, - "crtc's computed active state doesn't match tracked active state " - "(expected %i, found %i)\n", active, crtc->active); - I915_STATE_WARN(enabled != crtc->base.state->enable, - "crtc's computed enabled state doesn't match tracked enabled state " - "(expected %i, found %i)\n", enabled, - crtc->base.state->enable); - active = dev_priv->display.get_pipe_config(crtc, &pipe_config);
This is handled by the atomic core now, no need to check this for ourself. Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> --- drivers/gpu/drm/i915/intel_display.c | 19 +------------------ 1 file changed, 1 insertion(+), 18 deletions(-)