| Message ID | 1436797833-11493-8-git-send-email-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Jul 13, 2015 at 04:30:20PM +0200, Maarten Lankhorst wrote: > All non-primary planes get disabled during hw readout, > this reduces complexity and means not having to do some plane > visibility checks during the first commit. > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> I still think calling readout_plane_state from the hw state readout code is the wrong approach. Instead we should consolidate all the plane readout code exclusively into a new intel_plane_readout_hw_state helper which is called only from driver load paths. That should also contain the fb/gem_bo reconstruction loop. For the other 2 users of this code (lid notifiery and resume) we should just force an unconditional plane restore by setting crtc_state->planes_chagned. But I thinkt hat's ok as some follow-up work, once atomic has settled a bit more. -Daniel > --- > drivers/gpu/drm/i915/intel_atomic.c | 7 --- > drivers/gpu/drm/i915/intel_display.c | 86 ++++-------------------------------- > drivers/gpu/drm/i915/intel_drv.h | 1 - > 3 files changed, 8 insertions(+), 86 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c > index b92b8581efc2..dcf4fb458649 100644 > --- a/drivers/gpu/drm/i915/intel_atomic.c > +++ b/drivers/gpu/drm/i915/intel_atomic.c > @@ -98,13 +98,6 @@ int intel_atomic_check(struct drm_device *dev, > return -EINVAL; > } > > - if (crtc_state && > - crtc_state->quirks & PIPE_CONFIG_QUIRK_INITIAL_PLANES) { > - ret = drm_atomic_add_affected_planes(state, &nuclear_crtc->base); > - if (ret) > - return ret; > - } > - > ret = drm_atomic_helper_check_planes(dev, state); > if (ret) > return ret; > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index e4d8acc63823..118187dc76be 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -11783,34 +11783,6 @@ static bool check_encoder_cloning(struct drm_atomic_state *state, > return true; > } > > -static void intel_crtc_check_initial_planes(struct drm_crtc *crtc, > - struct drm_crtc_state *crtc_state) > -{ > - struct intel_crtc_state *pipe_config = > - to_intel_crtc_state(crtc_state); > - struct drm_plane *p; > - unsigned visible_mask = 0; > - > - drm_for_each_plane_mask(p, crtc->dev, crtc_state->plane_mask) { > - struct drm_plane_state *plane_state = > - drm_atomic_get_existing_plane_state(crtc_state->state, p); > - > - if (WARN_ON(!plane_state)) > - continue; > - > - if (!plane_state->fb) > - crtc_state->plane_mask &= > - ~(1 << drm_plane_index(p)); > - else if (to_intel_plane_state(plane_state)->visible) > - visible_mask |= 1 << drm_plane_index(p); > - } > - > - if (!visible_mask) > - return; > - > - pipe_config->quirks &= ~PIPE_CONFIG_QUIRK_INITIAL_PLANES; > -} > - > static int intel_crtc_atomic_check(struct drm_crtc *crtc, > struct drm_crtc_state *crtc_state) > { > @@ -11832,10 +11804,6 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc, > "[CRTC:%i] mismatch between state->active(%i) and crtc->active(%i)\n", > idx, crtc->state->active, intel_crtc->active); > > - /* plane mask is fixed up after all initial planes are calculated */ > - if (pipe_config->quirks & PIPE_CONFIG_QUIRK_INITIAL_PLANES) > - intel_crtc_check_initial_planes(crtc, crtc_state); > - > if (mode_changed && !crtc_state->active) > intel_crtc->atomic.update_wm_post = true; > > @@ -13188,19 +13156,6 @@ intel_modeset_compute_config(struct drm_atomic_state *state) > continue; > } > > - if (pipe_config->quirks & PIPE_CONFIG_QUIRK_INITIAL_PLANES) { > - ret = drm_atomic_add_affected_planes(state, crtc); > - if (ret) > - return ret; > - > - /* > - * We ought to handle i915.fastboot here. > - * If no modeset is required and the primary plane has > - * a fb, update the members of crtc_state as needed, > - * and run the necessary updates during vblank evasion. > - */ > - } > - > modeset = needs_modeset(crtc_state); > recalc = pipe_config->quirks & PIPE_CONFIG_QUIRK_INHERITED_MODE; > > @@ -15460,47 +15415,22 @@ static void readout_plane_state(struct intel_crtc *crtc, > struct intel_crtc_state *crtc_state) > { > struct intel_plane *p; > - struct drm_plane_state *drm_plane_state; > + struct intel_plane_state *plane_state; > bool active = crtc_state->base.active; > > - if (active) { > - crtc_state->quirks |= PIPE_CONFIG_QUIRK_INITIAL_PLANES; > - > - /* apply to previous sw state too */ > - to_intel_crtc_state(crtc->base.state)->quirks |= > - PIPE_CONFIG_QUIRK_INITIAL_PLANES; > - } > - > for_each_intel_plane(crtc->base.dev, p) { > - bool visible = active; > - > if (crtc->pipe != p->pipe) > continue; > > - drm_plane_state = p->base.state; > - > - /* Plane scaler state is not touched here. The first atomic > - * commit will restore all plane scalers to its old state. > - */ > + plane_state = to_intel_plane_state(p->base.state); > > - if (active && p->base.type == DRM_PLANE_TYPE_PRIMARY) { > - visible = primary_get_hw_state(crtc); > - to_intel_plane_state(drm_plane_state)->visible = visible; > - } else { > - /* > - * unknown state, assume it's off to force a transition > - * to on when calculating state changes. > - */ > - to_intel_plane_state(drm_plane_state)->visible = false; > - } > + if (p->base.type == DRM_PLANE_TYPE_PRIMARY) > + plane_state->visible = primary_get_hw_state(crtc); > + else { > + if (active) > + p->disable_plane(&p->base, &crtc->base); > > - if (visible) { > - crtc_state->base.plane_mask |= > - 1 << drm_plane_index(&p->base); > - } else if (crtc_state->base.state) { > - /* Make this unconditional for atomic hw readout. */ > - crtc_state->base.plane_mask &= > - ~(1 << drm_plane_index(&p->base)); > + plane_state->visible = false; > } > } > } > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 09e3581c8441..2c23900b491f 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -341,7 +341,6 @@ struct intel_crtc_state { > */ > #define PIPE_CONFIG_QUIRK_MODE_SYNC_FLAGS (1<<0) /* unreliable sync mode.flags */ > #define PIPE_CONFIG_QUIRK_INHERITED_MODE (1<<1) /* mode inherited from firmware */ > -#define PIPE_CONFIG_QUIRK_INITIAL_PLANES (1<<2) /* planes are in unknown state */ > unsigned long quirks; > > /* Pipe source size (ie. panel fitter input size) > -- > 2.1.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Op 14-07-15 om 11:53 schreef Daniel Vetter: > On Mon, Jul 13, 2015 at 04:30:20PM +0200, Maarten Lankhorst wrote: >> All non-primary planes get disabled during hw readout, >> this reduces complexity and means not having to do some plane >> visibility checks during the first commit. >> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > I still think calling readout_plane_state from the hw state readout code > is the wrong approach. Instead we should consolidate all the plane > readout code exclusively into a new intel_plane_readout_hw_state helper > which is called only from driver load paths. That should also contain the > fb/gem_bo reconstruction loop. Is that a nack? > For the other 2 users of this code (lid notifiery and resume) we should > just force an unconditional plane restore by setting > crtc_state->planes_chagned. But I thinkt hat's ok as some follow-up work, > once atomic has settled a bit more. > If you want to force a plane restore, planes_changed won't do what you think it does. planes_changed is a flag that says one or more planes were added to the crtc_state. You want to do drm_atomic_add_affected_planes for that, and atomic resume would do that since it duplicates all plane state. ~Maarten
On Tue, Jul 14, 2015 at 12:30:38PM +0200, Maarten Lankhorst wrote: > Op 14-07-15 om 11:53 schreef Daniel Vetter: > > On Mon, Jul 13, 2015 at 04:30:20PM +0200, Maarten Lankhorst wrote: > >> All non-primary planes get disabled during hw readout, > >> this reduces complexity and means not having to do some plane > >> visibility checks during the first commit. > >> > >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > I still think calling readout_plane_state from the hw state readout code > > is the wrong approach. Instead we should consolidate all the plane > > readout code exclusively into a new intel_plane_readout_hw_state helper > > which is called only from driver load paths. That should also contain the > > fb/gem_bo reconstruction loop. > Is that a nack? Nope, just a "I think we want to clarify this more". I'd welcome a patch on top, but can be done as part of converting dpms (since that will result in lots of cleanups too). > > For the other 2 users of this code (lid notifiery and resume) we should > > just force an unconditional plane restore by setting > > crtc_state->planes_chagned. But I thinkt hat's ok as some follow-up work, > > once atomic has settled a bit more. > > > If you want to force a plane restore, planes_changed won't do what you think it does. > planes_changed is a flag that says one or more planes were added to the crtc_state. > > You want to do drm_atomic_add_affected_planes for that, and atomic resume would do that > since it duplicates all plane state. Ah right, so should be possible to just consolidate the plane readout code without requiring any changes in the force_restore case. -Daniel
diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c index b92b8581efc2..dcf4fb458649 100644 --- a/drivers/gpu/drm/i915/intel_atomic.c +++ b/drivers/gpu/drm/i915/intel_atomic.c @@ -98,13 +98,6 @@ int intel_atomic_check(struct drm_device *dev, return -EINVAL; } - if (crtc_state && - crtc_state->quirks & PIPE_CONFIG_QUIRK_INITIAL_PLANES) { - ret = drm_atomic_add_affected_planes(state, &nuclear_crtc->base); - if (ret) - return ret; - } - ret = drm_atomic_helper_check_planes(dev, state); if (ret) return ret; diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index e4d8acc63823..118187dc76be 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -11783,34 +11783,6 @@ static bool check_encoder_cloning(struct drm_atomic_state *state, return true; } -static void intel_crtc_check_initial_planes(struct drm_crtc *crtc, - struct drm_crtc_state *crtc_state) -{ - struct intel_crtc_state *pipe_config = - to_intel_crtc_state(crtc_state); - struct drm_plane *p; - unsigned visible_mask = 0; - - drm_for_each_plane_mask(p, crtc->dev, crtc_state->plane_mask) { - struct drm_plane_state *plane_state = - drm_atomic_get_existing_plane_state(crtc_state->state, p); - - if (WARN_ON(!plane_state)) - continue; - - if (!plane_state->fb) - crtc_state->plane_mask &= - ~(1 << drm_plane_index(p)); - else if (to_intel_plane_state(plane_state)->visible) - visible_mask |= 1 << drm_plane_index(p); - } - - if (!visible_mask) - return; - - pipe_config->quirks &= ~PIPE_CONFIG_QUIRK_INITIAL_PLANES; -} - static int intel_crtc_atomic_check(struct drm_crtc *crtc, struct drm_crtc_state *crtc_state) { @@ -11832,10 +11804,6 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc, "[CRTC:%i] mismatch between state->active(%i) and crtc->active(%i)\n", idx, crtc->state->active, intel_crtc->active); - /* plane mask is fixed up after all initial planes are calculated */ - if (pipe_config->quirks & PIPE_CONFIG_QUIRK_INITIAL_PLANES) - intel_crtc_check_initial_planes(crtc, crtc_state); - if (mode_changed && !crtc_state->active) intel_crtc->atomic.update_wm_post = true; @@ -13188,19 +13156,6 @@ intel_modeset_compute_config(struct drm_atomic_state *state) continue; } - if (pipe_config->quirks & PIPE_CONFIG_QUIRK_INITIAL_PLANES) { - ret = drm_atomic_add_affected_planes(state, crtc); - if (ret) - return ret; - - /* - * We ought to handle i915.fastboot here. - * If no modeset is required and the primary plane has - * a fb, update the members of crtc_state as needed, - * and run the necessary updates during vblank evasion. - */ - } - modeset = needs_modeset(crtc_state); recalc = pipe_config->quirks & PIPE_CONFIG_QUIRK_INHERITED_MODE; @@ -15460,47 +15415,22 @@ static void readout_plane_state(struct intel_crtc *crtc, struct intel_crtc_state *crtc_state) { struct intel_plane *p; - struct drm_plane_state *drm_plane_state; + struct intel_plane_state *plane_state; bool active = crtc_state->base.active; - if (active) { - crtc_state->quirks |= PIPE_CONFIG_QUIRK_INITIAL_PLANES; - - /* apply to previous sw state too */ - to_intel_crtc_state(crtc->base.state)->quirks |= - PIPE_CONFIG_QUIRK_INITIAL_PLANES; - } - for_each_intel_plane(crtc->base.dev, p) { - bool visible = active; - if (crtc->pipe != p->pipe) continue; - drm_plane_state = p->base.state; - - /* Plane scaler state is not touched here. The first atomic - * commit will restore all plane scalers to its old state. - */ + plane_state = to_intel_plane_state(p->base.state); - if (active && p->base.type == DRM_PLANE_TYPE_PRIMARY) { - visible = primary_get_hw_state(crtc); - to_intel_plane_state(drm_plane_state)->visible = visible; - } else { - /* - * unknown state, assume it's off to force a transition - * to on when calculating state changes. - */ - to_intel_plane_state(drm_plane_state)->visible = false; - } + if (p->base.type == DRM_PLANE_TYPE_PRIMARY) + plane_state->visible = primary_get_hw_state(crtc); + else { + if (active) + p->disable_plane(&p->base, &crtc->base); - if (visible) { - crtc_state->base.plane_mask |= - 1 << drm_plane_index(&p->base); - } else if (crtc_state->base.state) { - /* Make this unconditional for atomic hw readout. */ - crtc_state->base.plane_mask &= - ~(1 << drm_plane_index(&p->base)); + plane_state->visible = false; } } } diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 09e3581c8441..2c23900b491f 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -341,7 +341,6 @@ struct intel_crtc_state { */ #define PIPE_CONFIG_QUIRK_MODE_SYNC_FLAGS (1<<0) /* unreliable sync mode.flags */ #define PIPE_CONFIG_QUIRK_INHERITED_MODE (1<<1) /* mode inherited from firmware */ -#define PIPE_CONFIG_QUIRK_INITIAL_PLANES (1<<2) /* planes are in unknown state */ unsigned long quirks; /* Pipe source size (ie. panel fitter input size)
All non-primary planes get disabled during hw readout, this reduces complexity and means not having to do some plane visibility checks during the first commit. Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> --- drivers/gpu/drm/i915/intel_atomic.c | 7 --- drivers/gpu/drm/i915/intel_display.c | 86 ++++-------------------------------- drivers/gpu/drm/i915/intel_drv.h | 1 - 3 files changed, 8 insertions(+), 86 deletions(-)