* [PATCH 1/4] drm/i915: Roll intel_crtc->atomic into intel_crtc_state
2015-09-10 1:57 [PATCH 0/4] Intermediate state for modesets Matt Roper
@ 2015-09-10 1:57 ` Matt Roper
2015-09-10 1:57 ` [PATCH 2/4] drm/atomic/helper: Allow duplication of in-flight states Matt Roper
` (2 subsequent siblings)
3 siblings, 0 replies; 7+ messages in thread
From: Matt Roper @ 2015-09-10 1:57 UTC (permalink / raw)
To: intel-gfx
Way back at the beginning of i915's atomic conversion I added
intel_crtc->atomic as a temporary dumping ground for "stuff to do
outside vblank evasion" flags since CRTC states weren't properly wired
up and tracked at that time. We've had proper CRTC state tracking for a
while now, so there's really no reason for this hack to continue to
exist. Moving forward we want to store intermediate crtc/plane state
data for modesets in addition to the final state, so moving these fields
into the proper state object allows us to properly compute them for both
the intermediate and final state.
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
drivers/gpu/drm/i915/intel_atomic.c | 16 ++++++-
drivers/gpu/drm/i915/intel_display.c | 83 ++++++++++++++++++------------------
drivers/gpu/drm/i915/intel_drv.h | 42 +++++++-----------
3 files changed, 73 insertions(+), 68 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
index 9336e80..3ffc385 100644
--- a/drivers/gpu/drm/i915/intel_atomic.c
+++ b/drivers/gpu/drm/i915/intel_atomic.c
@@ -101,6 +101,20 @@ intel_crtc_duplicate_state(struct drm_crtc *crtc)
crtc_state->base.crtc = crtc;
+ crtc_state->wait_for_flips = false;
+ crtc_state->disable_fbc = false;
+ crtc_state->disable_ips = false;
+ crtc_state->disable_cxsr = false;
+ crtc_state->pre_disable_primary = false;
+ crtc_state->update_wm_pre = false;
+ crtc_state->update_wm_post = false;
+ crtc_state->disabled_planes = 0;
+ crtc_state->fb_bits = 0;
+ crtc_state->wait_vblank = false;
+ crtc_state->update_fbc = false;
+ crtc_state->post_enable_primary = false;
+ crtc_state->update_sprite_watermarks = 0;
+
return &crtc_state->base;
}
@@ -212,7 +226,7 @@ int intel_atomic_setup_scalers(struct drm_device *dev,
* minimum required validation.
*/
if (plane->type == DRM_PLANE_TYPE_PRIMARY)
- intel_crtc->atomic.wait_for_flips = true;
+ crtc_state->wait_for_flips = true;
crtc_state->base.planes_changed = true;
}
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index af48c1e..46931f9 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4710,44 +4710,42 @@ intel_pre_disable_primary(struct drm_crtc *crtc)
static void intel_post_plane_update(struct intel_crtc *crtc)
{
- struct intel_crtc_atomic_commit *atomic = &crtc->atomic;
+ struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->base.state);
struct drm_device *dev = crtc->base.dev;
struct drm_i915_private *dev_priv = dev->dev_private;
struct drm_plane *plane;
- if (atomic->wait_vblank)
+ if (cstate->wait_vblank)
intel_wait_for_vblank(dev, crtc->pipe);
- intel_frontbuffer_flip(dev, atomic->fb_bits);
+ intel_frontbuffer_flip(dev, cstate->fb_bits);
- if (atomic->disable_cxsr)
+ if (cstate->disable_cxsr)
crtc->wm.cxsr_allowed = true;
- if (crtc->atomic.update_wm_post)
+ if (cstate->update_wm_post)
intel_update_watermarks(&crtc->base);
- if (atomic->update_fbc)
+ if (cstate->update_fbc)
intel_fbc_update(dev_priv);
- if (atomic->post_enable_primary)
+ if (cstate->post_enable_primary)
intel_post_enable_primary(&crtc->base);
- drm_for_each_plane_mask(plane, dev, atomic->update_sprite_watermarks)
+ drm_for_each_plane_mask(plane, dev, cstate->update_sprite_watermarks)
intel_update_sprite_watermarks(plane, &crtc->base,
0, 0, 0, false, false);
-
- memset(atomic, 0, sizeof(*atomic));
}
static void intel_pre_plane_update(struct intel_crtc *crtc)
{
+ struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->base.state);
struct drm_device *dev = crtc->base.dev;
struct drm_i915_private *dev_priv = dev->dev_private;
- struct intel_crtc_atomic_commit *atomic = &crtc->atomic;
struct drm_plane *p;
/* Track fb's for any planes being disabled */
- drm_for_each_plane_mask(p, dev, atomic->disabled_planes) {
+ drm_for_each_plane_mask(p, dev, cstate->disabled_planes) {
struct intel_plane *plane = to_intel_plane(p);
mutex_lock(&dev->struct_mutex);
@@ -4756,19 +4754,19 @@ static void intel_pre_plane_update(struct intel_crtc *crtc)
mutex_unlock(&dev->struct_mutex);
}
- if (atomic->wait_for_flips)
+ if (cstate->wait_for_flips)
intel_crtc_wait_for_pending_flips(&crtc->base);
- if (atomic->disable_fbc)
+ if (cstate->disable_fbc)
intel_fbc_disable_crtc(crtc);
- if (crtc->atomic.disable_ips)
+ if (cstate->disable_ips)
hsw_disable_ips(crtc);
- if (atomic->pre_disable_primary)
+ if (cstate->pre_disable_primary)
intel_pre_disable_primary(&crtc->base);
- if (atomic->disable_cxsr) {
+ if (cstate->disable_cxsr) {
crtc->wm.cxsr_allowed = false;
intel_set_memory_cxsr(dev_priv, false);
}
@@ -11529,6 +11527,8 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
{
struct drm_crtc *crtc = crtc_state->crtc;
struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+ struct intel_crtc_state *intel_crtc_state =
+ to_intel_crtc_state(crtc_state);
struct drm_plane *plane = plane_state->plane;
struct drm_device *dev = crtc->dev;
struct drm_i915_private *dev_priv = dev->dev_private;
@@ -11543,10 +11543,10 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
bool turn_off, turn_on, visible, was_visible;
struct drm_framebuffer *fb = plane_state->fb;
- if (crtc_state && INTEL_INFO(dev)->gen >= 9 &&
+ if (INTEL_INFO(dev)->gen >= 9 &&
plane->type != DRM_PLANE_TYPE_CURSOR) {
ret = skl_update_scaler_plane(
- to_intel_crtc_state(crtc_state),
+ intel_crtc_state,
to_intel_plane_state(plane_state));
if (ret)
return ret;
@@ -11558,7 +11558,7 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
* get called by the plane helpers.
*/
if (old_plane_state->base.fb && !fb)
- intel_crtc->atomic.disabled_planes |= 1 << i;
+ intel_crtc_state->disabled_planes |= 1 << i;
was_visible = old_plane_state->visible;
visible = to_intel_plane_state(plane_state)->visible;
@@ -11583,35 +11583,35 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
turn_off, turn_on, mode_changed);
if (turn_on) {
- intel_crtc->atomic.update_wm_pre = true;
+ intel_crtc_state->update_wm_pre = true;
/* must disable cxsr around plane enable/disable */
if (plane->type != DRM_PLANE_TYPE_CURSOR) {
- intel_crtc->atomic.disable_cxsr = true;
+ intel_crtc_state->disable_cxsr = true;
/* to potentially re-enable cxsr */
- intel_crtc->atomic.wait_vblank = true;
- intel_crtc->atomic.update_wm_post = true;
+ intel_crtc_state->wait_vblank = true;
+ intel_crtc_state->update_wm_post = true;
}
} else if (turn_off) {
- intel_crtc->atomic.update_wm_post = true;
+ intel_crtc_state->update_wm_post = true;
/* must disable cxsr around plane enable/disable */
if (plane->type != DRM_PLANE_TYPE_CURSOR) {
if (is_crtc_enabled)
- intel_crtc->atomic.wait_vblank = true;
- intel_crtc->atomic.disable_cxsr = true;
+ intel_crtc_state->wait_vblank = true;
+ intel_crtc_state->disable_cxsr = true;
}
} else if (intel_wm_need_update(plane, plane_state)) {
- intel_crtc->atomic.update_wm_pre = true;
+ intel_crtc_state->update_wm_pre = true;
}
if (visible || was_visible)
- intel_crtc->atomic.fb_bits |=
+ intel_crtc_state->fb_bits |=
to_intel_plane(plane)->frontbuffer_bit;
switch (plane->type) {
case DRM_PLANE_TYPE_PRIMARY:
- intel_crtc->atomic.wait_for_flips = true;
- intel_crtc->atomic.pre_disable_primary = turn_off;
- intel_crtc->atomic.post_enable_primary = turn_on;
+ intel_crtc_state->wait_for_flips = true;
+ intel_crtc_state->pre_disable_primary = turn_off;
+ intel_crtc_state->post_enable_primary = turn_on;
if (turn_off) {
/*
@@ -11622,9 +11622,9 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
* update_primary_plane function IPS needs to be
* disable.
*/
- intel_crtc->atomic.disable_ips = true;
+ intel_crtc_state->disable_ips = true;
- intel_crtc->atomic.disable_fbc = true;
+ intel_crtc_state->disable_fbc = true;
}
/*
@@ -11642,7 +11642,7 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
INTEL_INFO(dev)->gen <= 4 && !IS_G4X(dev) &&
dev_priv->fbc.crtc == intel_crtc &&
plane_state->rotation != BIT(DRM_ROTATE_0))
- intel_crtc->atomic.disable_fbc = true;
+ intel_crtc_state->disable_fbc = true;
/*
* BDW signals flip done immediately if the plane
@@ -11650,16 +11650,16 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
* armed to occur at the next vblank :(
*/
if (turn_on && IS_BROADWELL(dev))
- intel_crtc->atomic.wait_vblank = true;
+ intel_crtc_state->wait_vblank = true;
- intel_crtc->atomic.update_fbc |= visible || mode_changed;
+ intel_crtc_state->update_fbc |= visible || mode_changed;
break;
case DRM_PLANE_TYPE_CURSOR:
break;
case DRM_PLANE_TYPE_OVERLAY:
if (turn_off && !mode_changed) {
- intel_crtc->atomic.wait_vblank = true;
- intel_crtc->atomic.update_sprite_watermarks |=
+ intel_crtc_state->wait_vblank = true;
+ intel_crtc_state->update_sprite_watermarks |=
1 << i;
}
}
@@ -11734,7 +11734,7 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc,
}
if (mode_changed && !crtc_state->active)
- intel_crtc->atomic.update_wm_post = true;
+ pipe_config->update_wm_post = true;
if (mode_changed && crtc_state->enable &&
dev_priv->display.crtc_compute_clock &&
@@ -13444,8 +13444,9 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc,
{
struct drm_device *dev = crtc->dev;
struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+ struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->state);
- if (intel_crtc->atomic.update_wm_pre)
+ if (cstate->update_wm_pre)
intel_update_watermarks(crtc);
/* Perform vblank evasion around commit operation */
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 46484e4..8b7e3d9 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -456,6 +456,22 @@ struct intel_crtc_state {
/* w/a for waiting 2 vblanks during crtc enable */
enum pipe hsw_workaround_pipe;
+
+ /* Sleepable operations to perform before commit */
+ bool wait_for_flips;
+ bool disable_fbc;
+ bool disable_ips;
+ bool disable_cxsr;
+ bool pre_disable_primary;
+ bool update_wm_pre, update_wm_post;
+ unsigned disabled_planes;
+
+ /* Sleepable operations to perform after commit */
+ unsigned fb_bits;
+ bool wait_vblank;
+ bool update_fbc;
+ bool post_enable_primary;
+ unsigned update_sprite_watermarks;
};
struct vlv_wm_state {
@@ -489,30 +505,6 @@ struct skl_pipe_wm {
uint32_t linetime;
};
-/*
- * Tracking of operations that need to be performed at the beginning/end of an
- * atomic commit, outside the atomic section where interrupts are disabled.
- * These are generally operations that grab mutexes or might otherwise sleep
- * and thus can't be run with interrupts disabled.
- */
-struct intel_crtc_atomic_commit {
- /* Sleepable operations to perform before commit */
- bool wait_for_flips;
- bool disable_fbc;
- bool disable_ips;
- bool disable_cxsr;
- bool pre_disable_primary;
- bool update_wm_pre, update_wm_post;
- unsigned disabled_planes;
-
- /* Sleepable operations to perform after commit */
- unsigned fb_bits;
- bool wait_vblank;
- bool update_fbc;
- bool post_enable_primary;
- unsigned update_sprite_watermarks;
-};
-
struct intel_crtc {
struct drm_crtc base;
enum pipe pipe;
@@ -566,8 +558,6 @@ struct intel_crtc {
unsigned start_vbl_count;
ktime_t start_vbl_time;
- struct intel_crtc_atomic_commit atomic;
-
/* scalers available on this crtc */
int num_scalers;
--
2.1.4
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 7+ messages in thread* [PATCH 2/4] drm/atomic/helper: Allow duplication of in-flight states
2015-09-10 1:57 [PATCH 0/4] Intermediate state for modesets Matt Roper
2015-09-10 1:57 ` [PATCH 1/4] drm/i915: Roll intel_crtc->atomic into intel_crtc_state Matt Roper
@ 2015-09-10 1:57 ` Matt Roper
2015-09-10 1:57 ` [PATCH 3/4] drm/i915: Calculate an intermediate atomic state for modesets (v2) Matt Roper
2015-09-10 1:57 ` [PATCH 4/4] drm/i915: Update modeset programming to use intermediate state (v2) Matt Roper
3 siblings, 0 replies; 7+ messages in thread
From: Matt Roper @ 2015-09-10 1:57 UTC (permalink / raw)
To: intel-gfx; +Cc: dri-devel
Drivers may wish to duplicate in-flight states internally, but the
__drm_atomic_helper_{crtc,plane}_duplicate_state helper functions can
only be used to make a copy of state that has already been committed to
obj->state. Let's update the helpers to take any existing state object
as their first parameter rather than the DRM object pointer. We can
update handful of existing callsites to just pass obj->state for it.
Cc: dri-devel@lists.freedesktop.org
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
drivers/gpu/drm/drm_atomic_helper.c | 12 ++++++------
drivers/gpu/drm/i915/intel_atomic.c | 2 +-
drivers/gpu/drm/i915/intel_atomic_plane.c | 2 +-
drivers/gpu/drm/omapdrm/omap_plane.c | 2 +-
drivers/gpu/drm/rcar-du/rcar_du_plane.c | 2 +-
drivers/gpu/drm/tegra/dc.c | 4 ++--
include/drm/drm_atomic_helper.h | 4 ++--
7 files changed, 14 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 9941167..915b462 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -2149,10 +2149,10 @@ EXPORT_SYMBOL(drm_atomic_helper_crtc_reset);
* Copies atomic state from a CRTC's current state and resets inferred values.
* This is useful for drivers that subclass the CRTC state.
*/
-void __drm_atomic_helper_crtc_duplicate_state(struct drm_crtc *crtc,
+void __drm_atomic_helper_crtc_duplicate_state(struct drm_crtc_state *from,
struct drm_crtc_state *state)
{
- memcpy(state, crtc->state, sizeof(*state));
+ memcpy(state, from, sizeof(*state));
if (state->mode_blob)
drm_property_reference_blob(state->mode_blob);
@@ -2181,7 +2181,7 @@ drm_atomic_helper_crtc_duplicate_state(struct drm_crtc *crtc)
state = kmalloc(sizeof(*state), GFP_KERNEL);
if (state)
- __drm_atomic_helper_crtc_duplicate_state(crtc, state);
+ __drm_atomic_helper_crtc_duplicate_state(crtc->state, state);
return state;
}
@@ -2248,10 +2248,10 @@ EXPORT_SYMBOL(drm_atomic_helper_plane_reset);
* Copies atomic state from a plane's current state. This is useful for
* drivers that subclass the plane state.
*/
-void __drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane,
+void __drm_atomic_helper_plane_duplicate_state(struct drm_plane_state *from,
struct drm_plane_state *state)
{
- memcpy(state, plane->state, sizeof(*state));
+ memcpy(state, from, sizeof(*state));
if (state->fb)
drm_framebuffer_reference(state->fb);
@@ -2275,7 +2275,7 @@ drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane)
state = kmalloc(sizeof(*state), GFP_KERNEL);
if (state)
- __drm_atomic_helper_plane_duplicate_state(plane, state);
+ __drm_atomic_helper_plane_duplicate_state(plane->state, state);
return state;
}
diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
index 3ffc385..45b21ac 100644
--- a/drivers/gpu/drm/i915/intel_atomic.c
+++ b/drivers/gpu/drm/i915/intel_atomic.c
@@ -97,7 +97,7 @@ intel_crtc_duplicate_state(struct drm_crtc *crtc)
if (!crtc_state)
return NULL;
- __drm_atomic_helper_crtc_duplicate_state(crtc, &crtc_state->base);
+ __drm_atomic_helper_crtc_duplicate_state(crtc->state, &crtc_state->base);
crtc_state->base.crtc = crtc;
diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
index f1ab8e4..afbbfe0 100644
--- a/drivers/gpu/drm/i915/intel_atomic_plane.c
+++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
@@ -87,7 +87,7 @@ intel_plane_duplicate_state(struct drm_plane *plane)
state = &intel_state->base;
- __drm_atomic_helper_plane_duplicate_state(plane, state);
+ __drm_atomic_helper_plane_duplicate_state(plane->state, state);
return state;
}
diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c b/drivers/gpu/drm/omapdrm/omap_plane.c
index 09e363b..8962e6b 100644
--- a/drivers/gpu/drm/omapdrm/omap_plane.c
+++ b/drivers/gpu/drm/omapdrm/omap_plane.c
@@ -258,7 +258,7 @@ omap_plane_atomic_duplicate_state(struct drm_plane *plane)
if (copy == NULL)
return NULL;
- __drm_atomic_helper_plane_duplicate_state(plane, ©->base);
+ __drm_atomic_helper_plane_duplicate_state(plane->state, ©->base);
return ©->base;
}
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_plane.c b/drivers/gpu/drm/rcar-du/rcar_du_plane.c
index c669864..e050f54 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_plane.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_plane.c
@@ -310,7 +310,7 @@ rcar_du_plane_atomic_duplicate_state(struct drm_plane *plane)
if (copy == NULL)
return NULL;
- __drm_atomic_helper_plane_duplicate_state(plane, ©->state);
+ __drm_atomic_helper_plane_duplicate_state(plane->state, ©->state);
return ©->state;
}
diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
index b4af4ab..efd1fc1 100644
--- a/drivers/gpu/drm/tegra/dc.c
+++ b/drivers/gpu/drm/tegra/dc.c
@@ -455,7 +455,7 @@ static struct drm_plane_state *tegra_plane_atomic_duplicate_state(struct drm_pla
if (!copy)
return NULL;
- __drm_atomic_helper_plane_duplicate_state(plane, ©->base);
+ __drm_atomic_helper_plane_duplicate_state(plane->state, ©->base);
copy->tiling = state->tiling;
copy->format = state->format;
copy->swap = state->swap;
@@ -1036,7 +1036,7 @@ tegra_crtc_atomic_duplicate_state(struct drm_crtc *crtc)
if (!copy)
return NULL;
- __drm_atomic_helper_crtc_duplicate_state(crtc, ©->base);
+ __drm_atomic_helper_crtc_duplicate_state(crtc->state, ©->base);
copy->clk = state->clk;
copy->pclk = state->pclk;
copy->div = state->div;
diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
index 1547eb4..570ebff 100644
--- a/include/drm/drm_atomic_helper.h
+++ b/include/drm/drm_atomic_helper.h
@@ -93,7 +93,7 @@ int drm_atomic_helper_connector_dpms(struct drm_connector *connector,
/* default implementations for state handling */
void drm_atomic_helper_crtc_reset(struct drm_crtc *crtc);
-void __drm_atomic_helper_crtc_duplicate_state(struct drm_crtc *crtc,
+void __drm_atomic_helper_crtc_duplicate_state(struct drm_crtc_state *from,
struct drm_crtc_state *state);
struct drm_crtc_state *
drm_atomic_helper_crtc_duplicate_state(struct drm_crtc *crtc);
@@ -103,7 +103,7 @@ void drm_atomic_helper_crtc_destroy_state(struct drm_crtc *crtc,
struct drm_crtc_state *state);
void drm_atomic_helper_plane_reset(struct drm_plane *plane);
-void __drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane,
+void __drm_atomic_helper_plane_duplicate_state(struct drm_plane_state *from,
struct drm_plane_state *state);
struct drm_plane_state *
drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane);
--
2.1.4
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 7+ messages in thread* [PATCH 3/4] drm/i915: Calculate an intermediate atomic state for modesets (v2)
2015-09-10 1:57 [PATCH 0/4] Intermediate state for modesets Matt Roper
2015-09-10 1:57 ` [PATCH 1/4] drm/i915: Roll intel_crtc->atomic into intel_crtc_state Matt Roper
2015-09-10 1:57 ` [PATCH 2/4] drm/atomic/helper: Allow duplication of in-flight states Matt Roper
@ 2015-09-10 1:57 ` Matt Roper
2015-09-10 1:57 ` [PATCH 4/4] drm/i915: Update modeset programming to use intermediate state (v2) Matt Roper
3 siblings, 0 replies; 7+ messages in thread
From: Matt Roper @ 2015-09-10 1:57 UTC (permalink / raw)
To: intel-gfx
To simplify the modeset process for atomic, start calculating an
"intermediate" state. This state only modifies CRTC's that undergo a
modeset (or disable)...in that case it will be a copy of the final state
with ->active set to false (which will in turn cause various derived
state to be calculated differently). This state represents the status
of the CRTC and planes at the point in which the CRTC's have been
disabled even if the CRTC will be running again once the full modeset is
complete. This will allow us to calculate proper derived state fields
for this point in the modeset process which should in turn should help
us properly perform tasks like watermark calculation with less special
cases.
For review simplicity, this patch simply allocates and calculates the
intermediate state, but doesn't actually do anything with it yet.
The next patch will start actually using this intermediate state data.
v2:
- Use a whole new top-level state object rather than just sticking
extra plane state and crtc state arrays in the existing
intel_atomic_state object. (Ville)
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
drivers/gpu/drm/i915/intel_atomic.c | 142 +++++++++++++++++++++++++-----
drivers/gpu/drm/i915/intel_atomic_plane.c | 36 +++++---
drivers/gpu/drm/i915/intel_display.c | 3 +-
drivers/gpu/drm/i915/intel_drv.h | 14 +++
4 files changed, 160 insertions(+), 35 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
index 45b21ac..0487073 100644
--- a/drivers/gpu/drm/i915/intel_atomic.c
+++ b/drivers/gpu/drm/i915/intel_atomic.c
@@ -74,32 +74,23 @@ intel_connector_atomic_get_property(struct drm_connector *connector,
}
/*
- * intel_crtc_duplicate_state - duplicate crtc state
- * @crtc: drm crtc
- *
- * Allocates and returns a copy of the crtc state (both common and
- * Intel-specific) for the specified crtc.
- *
- * Returns: The newly allocated crtc state, or NULL on failure.
+ * Duplicate any CRTC state (not just an already committed state).
*/
-struct drm_crtc_state *
-intel_crtc_duplicate_state(struct drm_crtc *crtc)
+static struct drm_crtc_state *
+__intel_crtc_duplicate_state(struct drm_crtc_state *cs)
{
- struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
struct intel_crtc_state *crtc_state;
- if (WARN_ON(!intel_crtc->config))
+ if (WARN_ON(!cs))
crtc_state = kzalloc(sizeof(*crtc_state), GFP_KERNEL);
else
- crtc_state = kmemdup(intel_crtc->config,
- sizeof(*intel_crtc->config), GFP_KERNEL);
-
+ crtc_state = kmemdup(cs, sizeof(*crtc_state), GFP_KERNEL);
if (!crtc_state)
return NULL;
- __drm_atomic_helper_crtc_duplicate_state(crtc->state, &crtc_state->base);
+ __drm_atomic_helper_crtc_duplicate_state(cs, &crtc_state->base);
- crtc_state->base.crtc = crtc;
+ crtc_state->base.crtc = cs->crtc;
crtc_state->wait_for_flips = false;
crtc_state->disable_fbc = false;
@@ -118,6 +109,21 @@ intel_crtc_duplicate_state(struct drm_crtc *crtc)
return &crtc_state->base;
}
+/*
+ * intel_crtc_duplicate_state - duplicate crtc state
+ * @crtc: drm crtc
+ *
+ * Allocates and returns a copy of the crtc state (both common and
+ * Intel-specific) for the specified crtc.
+ *
+ * Returns: The newly allocated crtc state, or NULL on failure.
+ */
+struct drm_crtc_state *
+intel_crtc_duplicate_state(struct drm_crtc *crtc)
+{
+ return __intel_crtc_duplicate_state(crtc->state);
+}
+
/**
* intel_crtc_destroy_state - destroy crtc state
* @crtc: drm crtc
@@ -315,17 +321,113 @@ intel_atomic_state_alloc(struct drm_device *dev)
{
struct intel_atomic_state *state = kzalloc(sizeof(*state), GFP_KERNEL);
- if (!state || drm_atomic_state_init(dev, &state->base) < 0) {
- kfree(state);
- return NULL;
- }
+ if (!state || drm_atomic_state_init(dev, &state->base) < 0)
+ goto fail;
+
+ /* Also allocate an intermediate state */
+ state->intermediate = kzalloc(sizeof(*state->intermediate), GFP_KERNEL);
+ if (!state->intermediate ||
+ drm_atomic_state_init(dev, state->intermediate) < 0)
+ goto fail;
+ state->intermediate->acquire_ctx = state->base.acquire_ctx;
return &state->base;
+
+fail:
+ if (state)
+ kfree(state->intermediate);
+ drm_atomic_state_default_release(&state->base);
+ kfree(state);
+ return NULL;
+}
+
+void
+intel_atomic_state_free(struct drm_atomic_state *s)
+{
+ struct intel_atomic_state *state = to_intel_atomic_state(s);
+
+ if (WARN_ON(!s))
+ return;
+
+ drm_atomic_state_default_release(state->intermediate);
+ drm_atomic_state_default_release(s);
}
void intel_atomic_state_clear(struct drm_atomic_state *s)
{
struct intel_atomic_state *state = to_intel_atomic_state(s);
+
+ drm_atomic_state_default_clear(state->intermediate);
drm_atomic_state_default_clear(&state->base);
state->dpll_set = false;
}
+
+/**
+ * intel_atomic_check_planes - validate state object for planes changes
+ * @dev: DRM device
+ * @state: the driver state object
+ *
+ * An Intel-specific replacement for drm_atomic_helper_check_planes(). In
+ * addition to calling the ->atomic_check() entrypoints for all plane/crtc
+ * states present in the atomic transaction, it also creates an additional
+ * 'intermediate' state that corresponds to the point in the commit pipeline
+ * where any CRTC's performing a modeset (or disable) have been disabled but
+ * not yet reenabled. Specifically, crtc_state->active = false for each CRTC
+ * involved in a modeset and the corresponding derived state is updated to
+ * reflect that.
+ */
+int
+intel_atomic_check_planes(struct drm_device *dev,
+ struct drm_atomic_state *state)
+{
+ struct intel_atomic_state *istate = to_intel_atomic_state(state);
+ struct drm_atomic_state *inter = istate->intermediate;
+ struct drm_crtc *crtc;
+ struct drm_crtc_state *cstate;
+ struct drm_plane *plane;
+ struct drm_plane_state *pstate;
+ unsigned needs_ms = 0;
+ int i, ret;
+
+ /*
+ * Start by checking calculating/checking the final state as usual;
+ * if that doesn't work, then there's no point in worrying about
+ * intermediate states.
+ */
+ ret = drm_atomic_helper_check_planes(dev, state);
+ if (ret)
+ return ret;
+
+ /*
+ * For CRTC's undergoing a modeset, copy CRTC and plane state into
+ * the intermediate state, but update crtc_state->active = false.
+ */
+ for_each_crtc_in_state(state, crtc, cstate, i) {
+ if (!drm_atomic_crtc_needs_modeset(cstate))
+ continue;
+
+ needs_ms |= drm_crtc_mask(crtc);
+ inter->crtcs[i] = crtc;
+ inter->crtc_states[i] = __intel_crtc_duplicate_state(cstate);
+ inter->crtc_states[i]->state = inter;
+ inter->crtc_states[i]->active = false;
+
+ if (drm_atomic_crtc_needs_modeset(cstate))
+ istate->any_ms = true;
+ }
+ for_each_plane_in_state(state, plane, pstate, i) {
+ crtc = plane->state->crtc ?: pstate->crtc;
+ if (!crtc || !(needs_ms & drm_crtc_mask(crtc)))
+ continue;
+
+ inter->planes[i] = plane;
+ inter->plane_states[i] = __intel_plane_duplicate_state(pstate);
+ inter->plane_states[i]->state = inter;
+ }
+
+ /*
+ * Run all the plane/crtc check handlers to update the derived state
+ * fields to reflect the ->active change.
+ */
+ return drm_atomic_helper_check_planes(dev, inter);
+}
diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
index afbbfe0..b893853 100644
--- a/drivers/gpu/drm/i915/intel_atomic_plane.c
+++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
@@ -61,38 +61,46 @@ intel_create_plane_state(struct drm_plane *plane)
return state;
}
-/**
- * intel_plane_duplicate_state - duplicate plane state
- * @plane: drm plane
- *
- * Allocates and returns a copy of the plane state (both common and
- * Intel-specific) for the specified plane.
- *
- * Returns: The newly allocated plane state, or NULL on failure.
+/*
+ * Duplicate any plane state (not just an already committed state).
*/
struct drm_plane_state *
-intel_plane_duplicate_state(struct drm_plane *plane)
+__intel_plane_duplicate_state(struct drm_plane_state *ps)
{
struct drm_plane_state *state;
struct intel_plane_state *intel_state;
- if (WARN_ON(!plane->state))
- intel_state = intel_create_plane_state(plane);
+ if (WARN_ON(!ps))
+ intel_state = intel_create_plane_state(ps->plane);
else
- intel_state = kmemdup(plane->state, sizeof(*intel_state),
- GFP_KERNEL);
+ intel_state = kmemdup(ps, sizeof(*intel_state), GFP_KERNEL);
if (!intel_state)
return NULL;
state = &intel_state->base;
- __drm_atomic_helper_plane_duplicate_state(plane->state, state);
+ __drm_atomic_helper_plane_duplicate_state(ps, state);
return state;
}
/**
+ * intel_plane_duplicate_state - duplicate plane state
+ * @plane: drm plane
+ *
+ * Allocates and returns a copy of the plane state (both common and
+ * Intel-specific) for the specified plane.
+ *
+ * Returns: The newly allocated plane state, or NULL on failure.
+ */
+struct drm_plane_state *
+intel_plane_duplicate_state(struct drm_plane *plane)
+{
+ return __intel_plane_duplicate_state(plane->state);
+}
+
+/**
* intel_plane_destroy_state - destroy plane state
* @plane: drm plane
* @state: state object to destroy
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 46931f9..5081a9e 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13024,7 +13024,7 @@ static int intel_atomic_check(struct drm_device *dev,
to_intel_atomic_state(state)->cdclk =
to_i915(state->dev)->cdclk_freq;
- return drm_atomic_helper_check_planes(state->dev, state);
+ return intel_atomic_check_planes(state->dev, state);
}
/**
@@ -14312,6 +14312,7 @@ static const struct drm_mode_config_funcs intel_mode_funcs = {
.atomic_check = intel_atomic_check,
.atomic_commit = intel_atomic_commit,
.atomic_state_alloc = intel_atomic_state_alloc,
+ .atomic_state_free = intel_atomic_state_free,
.atomic_state_clear = intel_atomic_state_clear,
};
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 8b7e3d9..e2aa7b6 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -237,6 +237,15 @@ typedef struct dpll {
struct intel_atomic_state {
struct drm_atomic_state base;
+ /*
+ * A second top-level state object that holds the intermediate state
+ * that the driver passes through when performing a modeset or
+ * disabling CRTC's. This state corresponds to the point where we've
+ * disabled the CRTC's undergoing modesets.
+ */
+ struct drm_atomic_state *intermediate;
+
+ bool any_ms;
unsigned int cdclk;
bool dpll_set;
struct intel_shared_dpll_config shared_dpll[I915_NUM_PLLS];
@@ -1419,6 +1428,7 @@ struct drm_crtc_state *intel_crtc_duplicate_state(struct drm_crtc *crtc);
void intel_crtc_destroy_state(struct drm_crtc *crtc,
struct drm_crtc_state *state);
struct drm_atomic_state *intel_atomic_state_alloc(struct drm_device *dev);
+void intel_atomic_state_free(struct drm_atomic_state *s);
void intel_atomic_state_clear(struct drm_atomic_state *);
struct intel_shared_dpll_config *
intel_atomic_get_shared_dpll_state(struct drm_atomic_state *s);
@@ -1437,6 +1447,8 @@ intel_atomic_get_crtc_state(struct drm_atomic_state *state,
int intel_atomic_setup_scalers(struct drm_device *dev,
struct intel_crtc *intel_crtc,
struct intel_crtc_state *crtc_state);
+int intel_atomic_check_planes(struct drm_device *dev,
+ struct drm_atomic_state *state);
/* intel_atomic_plane.c */
struct intel_plane_state *intel_create_plane_state(struct drm_plane *plane);
@@ -1444,5 +1456,7 @@ struct drm_plane_state *intel_plane_duplicate_state(struct drm_plane *plane);
void intel_plane_destroy_state(struct drm_plane *plane,
struct drm_plane_state *state);
extern const struct drm_plane_helper_funcs intel_plane_helper_funcs;
+struct drm_plane_state *
+__intel_plane_duplicate_state(struct drm_plane_state *ps);
#endif /* __INTEL_DRV_H__ */
--
2.1.4
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 7+ messages in thread* [PATCH 4/4] drm/i915: Update modeset programming to use intermediate state (v2)
2015-09-10 1:57 [PATCH 0/4] Intermediate state for modesets Matt Roper
` (2 preceding siblings ...)
2015-09-10 1:57 ` [PATCH 3/4] drm/i915: Calculate an intermediate atomic state for modesets (v2) Matt Roper
@ 2015-09-10 1:57 ` Matt Roper
2015-09-10 5:40 ` Maarten Lankhorst
3 siblings, 1 reply; 7+ messages in thread
From: Matt Roper @ 2015-09-10 1:57 UTC (permalink / raw)
To: intel-gfx
As suggested by Ville, the general flow should now roughly follow:
// whatever the user wanted
compute_final_atomic_state()
// include all crtcs in the intermediate state which are
// getting disabled (even temporarily to perform a modeset)
compute_intermediate_atomic_state()
ret = check_state_change(old, intermediate)
ret = check_state_change(intermediate, new)
// commit all planes in one go to make them pop out as
// atomically as possible
for_each_crtc_in(intermediate) {
commit_planes()
}
for_each_crtc_in(intermediate) {
disable_crtc()
}
for_each_crtc_in(new) {
if (!currently_active)
crtc_enable()
}
// commit all planes in one go to make them pop in as atomically
// as possible
for_each_crtc_in(new) {
commit_planes()
}
v2: Because we're potentially performing two state swaps here, the
actual set of FB's that need to be cleaned up at the end of the
process may need to be fetched from the intermediate state rather
than the final state, so use our own intel_cleanup_planes() rather
than the helper version.
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
I know Maarten had some reservations about this approach, so hopefully
providing an implementation will allow us to continue the discussion and come
to an agreement on whether or not intermediate states are the way to go.
drivers/gpu/drm/i915/intel_display.c | 104 +++++++++++++++++++++++++++--------
1 file changed, 80 insertions(+), 24 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 5081a9e..d63ad1d 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13027,6 +13027,36 @@ static int intel_atomic_check(struct drm_device *dev,
return intel_atomic_check_planes(state->dev, state);
}
+/*
+ * An Intel-specific version of drm_atomic_helper_cleanup_planes().
+ * Since we've potentially swapped to an intermediate state before
+ * swapping again to the final state, the actual framebuffers that
+ * need to be cleaned up will have been swapped into the intermediate
+ * state object for any CRTC's that are undergoing a modeset, so we
+ * need to grab them from there instead of the old_state object.
+ */
+static void
+intel_cleanup_planes(struct drm_device *dev,
+ struct drm_atomic_state *old_state)
+{
+ struct drm_atomic_state *inter =
+ to_intel_atomic_state(old_state)->intermediate;
+ struct drm_plane *plane;
+ struct drm_plane_state *plane_state;
+ int i;
+
+ for_each_plane_in_state(old_state, plane, plane_state, i) {
+ const struct drm_plane_helper_funcs *funcs;
+
+ funcs = plane->helper_private;
+
+ plane_state = inter->plane_states[i] ?: plane_state;
+
+ if (funcs->cleanup_fb)
+ funcs->cleanup_fb(plane, plane_state);
+ }
+}
+
/**
* intel_atomic_commit - commit validated state object
* @dev: DRM device
@@ -13048,11 +13078,12 @@ static int intel_atomic_commit(struct drm_device *dev,
bool async)
{
struct drm_i915_private *dev_priv = dev->dev_private;
+ struct intel_atomic_state *istate = to_intel_atomic_state(state);
+ struct drm_atomic_state *inter = istate->intermediate;
struct drm_crtc *crtc;
- struct drm_crtc_state *crtc_state;
+ struct drm_crtc_state *old_crtc_state;
int ret = 0;
int i;
- bool any_ms = false;
if (async) {
DRM_DEBUG_KMS("i915 does not yet support async commit\n");
@@ -13063,59 +13094,84 @@ static int intel_atomic_commit(struct drm_device *dev,
if (ret)
return ret;
- drm_atomic_helper_swap_state(dev, state);
+ if (!istate->any_ms) {
+ /*
+ * If there's no modeset involved, just move to the final state
+ * and jump directly to the final plane updates.
+ */
+ drm_atomic_helper_swap_state(dev, state);
+ intel_modeset_update_crtc_state(state);
+ goto nomodeset;
+ }
- for_each_crtc_in_state(state, crtc, crtc_state, i) {
- struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+ /* Swap in intermediate state */
+ drm_atomic_helper_swap_state(dev, inter);
- if (!needs_modeset(crtc->state))
- continue;
+ /*
+ * Commit all planes in one go to make them pop out as atomically
+ * as possible.
+ */
+ for_each_crtc_in_state(inter, crtc, old_crtc_state, i) {
+ struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
- any_ms = true;
intel_pre_plane_update(intel_crtc);
+ drm_atomic_helper_commit_planes_on_crtc(old_crtc_state);
+ intel_post_plane_update(intel_crtc);
+ }
+
+ for_each_crtc_in_state(inter, crtc, old_crtc_state, i) {
+ struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
- if (crtc_state->active) {
- intel_crtc_disable_planes(crtc, crtc_state->plane_mask);
+ if (old_crtc_state->active) {
+ intel_crtc_disable_planes(crtc,
+ old_crtc_state->plane_mask);
dev_priv->display.crtc_disable(crtc);
intel_crtc->active = false;
intel_disable_shared_dpll(intel_crtc);
}
}
+ /* Commit final state to the underlying crtc/plane objects */
+ drm_atomic_helper_swap_state(dev, state);
+
/* Only after disabling all output pipelines that will be changed can we
* update the the output configuration. */
intel_modeset_update_crtc_state(state);
+ intel_shared_dpll_commit(state);
- if (any_ms) {
- intel_shared_dpll_commit(state);
-
- drm_atomic_helper_update_legacy_modeset_state(state->dev, state);
- modeset_update_crtc_power_domains(state);
- }
+ drm_atomic_helper_update_legacy_modeset_state(state->dev, state);
+ modeset_update_crtc_power_domains(state);
- /* Now enable the clocks, plane, pipe, and connectors that we set up. */
- for_each_crtc_in_state(state, crtc, crtc_state, i) {
- struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+ /* Now enable the clocks, pipe, and connectors that we set up. */
+ for_each_crtc_in_state(state, crtc, old_crtc_state, i) {
bool modeset = needs_modeset(crtc->state);
if (modeset && crtc->state->active) {
update_scanline_offset(to_intel_crtc(crtc));
dev_priv->display.crtc_enable(crtc);
}
+ }
- if (!modeset)
- intel_pre_plane_update(intel_crtc);
+nomodeset:
- drm_atomic_helper_commit_planes_on_crtc(crtc_state);
+ /*
+ * Commit all planes in one go to make them pop in as atomically as
+ * possible.
+ */
+ for_each_crtc_in_state(state, crtc, old_crtc_state, i) {
+ struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+
+ intel_pre_plane_update(intel_crtc);
+ drm_atomic_helper_commit_planes_on_crtc(old_crtc_state);
intel_post_plane_update(intel_crtc);
}
/* FIXME: add subpixel order */
drm_atomic_helper_wait_for_vblanks(dev, state);
- drm_atomic_helper_cleanup_planes(dev, state);
+ intel_cleanup_planes(dev, state);
- if (any_ms)
+ if (istate->any_ms)
intel_modeset_check_state(dev, state);
drm_atomic_state_free(state);
--
2.1.4
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH 4/4] drm/i915: Update modeset programming to use intermediate state (v2)
2015-09-10 1:57 ` [PATCH 4/4] drm/i915: Update modeset programming to use intermediate state (v2) Matt Roper
@ 2015-09-10 5:40 ` Maarten Lankhorst
2015-09-22 19:55 ` Ville Syrjälä
0 siblings, 1 reply; 7+ messages in thread
From: Maarten Lankhorst @ 2015-09-10 5:40 UTC (permalink / raw)
To: Matt Roper, intel-gfx
Op 10-09-15 om 03:57 schreef Matt Roper:
> As suggested by Ville, the general flow should now roughly follow:
>
> // whatever the user wanted
> compute_final_atomic_state()
>
> // include all crtcs in the intermediate state which are
> // getting disabled (even temporarily to perform a modeset)
> compute_intermediate_atomic_state()
>
> ret = check_state_change(old, intermediate)
> ret = check_state_change(intermediate, new)
>
> // commit all planes in one go to make them pop out as
> // atomically as possible
> for_each_crtc_in(intermediate) {
> commit_planes()
> }
>
> for_each_crtc_in(intermediate) {
> disable_crtc()
> }
>
> for_each_crtc_in(new) {
> if (!currently_active)
> crtc_enable()
> }
>
> // commit all planes in one go to make them pop in as atomically
> // as possible
> for_each_crtc_in(new) {
> commit_planes()
> }
>
> v2: Because we're potentially performing two state swaps here, the
> actual set of FB's that need to be cleaned up at the end of the
> process may need to be fetched from the intermediate state rather
> than the final state, so use our own intel_cleanup_planes() rather
> than the helper version.
>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
> I know Maarten had some reservations about this approach, so hopefully
> providing an implementation will allow us to continue the discussion and come
> to an agreement on whether or not intermediate states are the way to go.
I still don't like it. Intermediate wm's should be calculated in the check function, if it
can potentially fail.
The final state should be swapped in right away, not any intermediate state or async
modesets will never work.
And nothing should depend on the current state in the crtc_disable callbacks, if it
does it's a bug or it needs to get passed the old crtc_state so it knows what to disable.
This is probably why crtc->config is not dead yet.
Not committing DPLL state right after swap_state is a special case right now, but
that's easily fixed by changing pll->active from a refcount to a crtc mask.
~Maarten
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH 4/4] drm/i915: Update modeset programming to use intermediate state (v2)
2015-09-10 5:40 ` Maarten Lankhorst
@ 2015-09-22 19:55 ` Ville Syrjälä
0 siblings, 0 replies; 7+ messages in thread
From: Ville Syrjälä @ 2015-09-22 19:55 UTC (permalink / raw)
To: Maarten Lankhorst; +Cc: intel-gfx
On Thu, Sep 10, 2015 at 07:40:25AM +0200, Maarten Lankhorst wrote:
> Op 10-09-15 om 03:57 schreef Matt Roper:
> > As suggested by Ville, the general flow should now roughly follow:
> >
> > // whatever the user wanted
> > compute_final_atomic_state()
> >
> > // include all crtcs in the intermediate state which are
> > // getting disabled (even temporarily to perform a modeset)
> > compute_intermediate_atomic_state()
> >
> > ret = check_state_change(old, intermediate)
> > ret = check_state_change(intermediate, new)
> >
> > // commit all planes in one go to make them pop out as
> > // atomically as possible
> > for_each_crtc_in(intermediate) {
> > commit_planes()
> > }
> >
> > for_each_crtc_in(intermediate) {
> > disable_crtc()
> > }
> >
> > for_each_crtc_in(new) {
> > if (!currently_active)
> > crtc_enable()
> > }
> >
> > // commit all planes in one go to make them pop in as atomically
> > // as possible
> > for_each_crtc_in(new) {
> > commit_planes()
> > }
> >
> > v2: Because we're potentially performing two state swaps here, the
> > actual set of FB's that need to be cleaned up at the end of the
> > process may need to be fetched from the intermediate state rather
> > than the final state, so use our own intel_cleanup_planes() rather
> > than the helper version.
> >
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> > ---
> > I know Maarten had some reservations about this approach, so hopefully
> > providing an implementation will allow us to continue the discussion and come
> > to an agreement on whether or not intermediate states are the way to go.
> I still don't like it. Intermediate wm's should be calculated in the check function, if it
> can potentially fail.
You're mixing intermediate wms and intermediate atomic state. What the
latter buys us is:
- easy to reason about things: pipe/planes/etc. can only transition
on<->off, no need for those silly needs_modeset checks all over the
place.
- watermark _programming_ happens naturally from plane commit, no need
to sprinkle wm updates all over the place. there would an intermediate
and final wm values for both atomic states. The only special case
would probably be a single wm update call after the pipe has been
disabled, and that's only because we probably won't get a vblank irq
there anymore.
- cxsr programming also happens naturally from the wm updates. With the
current way of doing things, the only reasonable way of making this
work is probably to force disable cxsr around the entire modeset.
> The final state should be swapped in right away, not any intermediate state or async
> modesets will never work.
I think that's just a big failing of the current atomic design. If we
actually had proper separation of the user state and internal state we
wouldn't have any problems.
> And nothing should depend on the current state in the crtc_disable callbacks, if it
> does it's a bug or it needs to get passed the old crtc_state so it knows what to disable.
> This is probably why crtc->config is not dead yet.
>
> Not committing DPLL state right after swap_state is a special case right now, but
> that's easily fixed by changing pll->active from a refcount to a crtc mask.
>
> ~Maarten
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 7+ messages in thread