The atomic helper sets connector_state->connector, which the i915 code didn't. This will become a problem when we start using it.
Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com Acked-by: Thierry Reding treding@nvidia.com --- drivers/gpu/drm/i915/intel_display.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 7dd7200d3ba9..1e42309ec40a 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -6477,13 +6477,11 @@ static void intel_connector_check_state(struct intel_connector *connector)
int intel_connector_init(struct intel_connector *connector) { - struct drm_connector_state *connector_state; + drm_atomic_helper_connector_reset(&connector->base);
- connector_state = kzalloc(sizeof *connector_state, GFP_KERNEL); - if (!connector_state) + if (!connector->base.state) return -ENOMEM;
- connector->base.state = connector_state; return 0; }
This is useful for drivers that subclass connector_state, like tegra.
Changes since v1: - Docbook updates.
Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com --- drivers/gpu/drm/drm_atomic_helper.c | 30 ++++++++++++++++++++++++++---- include/drm/drm_atomic_helper.h | 2 ++ 2 files changed, 28 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 63f925b75357..27dd68f946e6 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -2607,6 +2607,28 @@ void drm_atomic_helper_plane_destroy_state(struct drm_plane *plane, EXPORT_SYMBOL(drm_atomic_helper_plane_destroy_state);
/** + * __drm_atomic_helper_connector_reset - reset state on connector + * @connector: drm connector + * @conn_state: connector state to assign + * + * Initializes the newly allocated @conn_state and assigns it to + * #connector ->state, usually required when initializing the drivers + * or when called from the ->reset hook. + * + * This is useful for drivers that subclass the connector state. + */ +void +__drm_atomic_helper_connector_reset(struct drm_connector *connector, + struct drm_connector_state *conn_state) +{ + if (conn_state) + conn_state->connector = connector; + + connector->state = conn_state; +} +EXPORT_SYMBOL(__drm_atomic_helper_connector_reset); + +/** * drm_atomic_helper_connector_reset - default ->reset hook for connectors * @connector: drm connector * @@ -2616,11 +2638,11 @@ EXPORT_SYMBOL(drm_atomic_helper_plane_destroy_state); */ void drm_atomic_helper_connector_reset(struct drm_connector *connector) { - kfree(connector->state); - connector->state = kzalloc(sizeof(*connector->state), GFP_KERNEL); + struct drm_connector_state *conn_state = + kzalloc(sizeof(*conn_state), GFP_KERNEL);
- if (connector->state) - connector->state->connector = connector; + kfree(connector->state); + __drm_atomic_helper_connector_reset(connector, conn_state); } EXPORT_SYMBOL(drm_atomic_helper_connector_reset);
diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h index a286cce98720..89d008dc08e2 100644 --- a/include/drm/drm_atomic_helper.h +++ b/include/drm/drm_atomic_helper.h @@ -126,6 +126,8 @@ void __drm_atomic_helper_plane_destroy_state(struct drm_plane *plane, void drm_atomic_helper_plane_destroy_state(struct drm_plane *plane, struct drm_plane_state *state);
+void __drm_atomic_helper_connector_reset(struct drm_connector *connector, + struct drm_connector_state *conn_state); void drm_atomic_helper_connector_reset(struct drm_connector *connector); void __drm_atomic_helper_connector_duplicate_state(struct drm_connector *connector,
On Mon, Jan 04, 2016 at 12:53:16PM +0100, Maarten Lankhorst wrote:
This is useful for drivers that subclass connector_state, like tegra.
Changes since v1:
- Docbook updates.
Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com
drivers/gpu/drm/drm_atomic_helper.c | 30 ++++++++++++++++++++++++++---- include/drm/drm_atomic_helper.h | 2 ++ 2 files changed, 28 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 63f925b75357..27dd68f946e6 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -2607,6 +2607,28 @@ void drm_atomic_helper_plane_destroy_state(struct drm_plane *plane, EXPORT_SYMBOL(drm_atomic_helper_plane_destroy_state);
/**
- __drm_atomic_helper_connector_reset - reset state on connector
- @connector: drm connector
- @conn_state: connector state to assign
- Initializes the newly allocated @conn_state and assigns it to
- #connector ->state, usually required when initializing the drivers
- or when called from the ->reset hook.
- This is useful for drivers that subclass the connector state.
- */
+void +__drm_atomic_helper_connector_reset(struct drm_connector *connector,
struct drm_connector_state *conn_state)+{
- if (conn_state)
conn_state->connector = connector;- connector->state = conn_state;
+} +EXPORT_SYMBOL(__drm_atomic_helper_connector_reset);
It's a bit strange that we don't have __*reset functions for crtc/plane now, especially since that would uncover that probably we should move cleaning up the old state into this helper (to share more code). That can be done by simply calling ->atomic_state_destroy. Callers would then only allocate a suitable struct, call this func and init any driver-private state.
But this here won't hurt, so merged it. -Daniel
+/**
- drm_atomic_helper_connector_reset - default ->reset hook for connectors
- @connector: drm connector
@@ -2616,11 +2638,11 @@ EXPORT_SYMBOL(drm_atomic_helper_plane_destroy_state); */ void drm_atomic_helper_connector_reset(struct drm_connector *connector) {
- kfree(connector->state);
- connector->state = kzalloc(sizeof(*connector->state), GFP_KERNEL);
- struct drm_connector_state *conn_state =
kzalloc(sizeof(*conn_state), GFP_KERNEL);
- if (connector->state)
connector->state->connector = connector;
- kfree(connector->state);
- __drm_atomic_helper_connector_reset(connector, conn_state);
} EXPORT_SYMBOL(drm_atomic_helper_connector_reset);
diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h index a286cce98720..89d008dc08e2 100644 --- a/include/drm/drm_atomic_helper.h +++ b/include/drm/drm_atomic_helper.h @@ -126,6 +126,8 @@ void __drm_atomic_helper_plane_destroy_state(struct drm_plane *plane, void drm_atomic_helper_plane_destroy_state(struct drm_plane *plane, struct drm_plane_state *state);
+void __drm_atomic_helper_connector_reset(struct drm_connector *connector,
struct drm_connector_state *conn_state);void drm_atomic_helper_connector_reset(struct drm_connector *connector); void __drm_atomic_helper_connector_duplicate_state(struct drm_connector *connector, -- 2.1.0
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Changes since v1: - Do not reset if state allocation fails.
Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com Acked-by: Thierry Reding treding@nvidia.com #irc --- drivers/gpu/drm/tegra/dsi.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/tegra/dsi.c b/drivers/gpu/drm/tegra/dsi.c index 50d46ae3786b..44e102799195 100644 --- a/drivers/gpu/drm/tegra/dsi.c +++ b/drivers/gpu/drm/tegra/dsi.c @@ -745,14 +745,13 @@ static void tegra_dsi_soft_reset(struct tegra_dsi *dsi)
static void tegra_dsi_connector_reset(struct drm_connector *connector) { - struct tegra_dsi_state *state; - - kfree(connector->state); - connector->state = NULL; + struct tegra_dsi_state *state = + kzalloc(sizeof(*state), GFP_KERNEL);
- state = kzalloc(sizeof(*state), GFP_KERNEL); - if (state) - connector->state = &state->base; + if (state) { + kfree(connector->state); + __drm_atomic_helper_connector_reset(connector, &state->base); + } }
static struct drm_connector_state *
It can be useful to iterate over connectors without grabbing connection_mutex. It can also be used to see how many connectors are on a crtc without iterating over the list.
Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com --- drivers/gpu/drm/drm_atomic.c | 11 +++++++++++ include/drm/drm_crtc.h | 3 +++ 2 files changed, 14 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 6a21e5c378c1..14b321580517 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -1063,10 +1063,21 @@ drm_atomic_set_crtc_for_connector(struct drm_connector_state *conn_state, { struct drm_crtc_state *crtc_state;
+ if (conn_state->crtc && conn_state->crtc != crtc) { + crtc_state = drm_atomic_get_existing_crtc_state(conn_state->state, + conn_state->crtc); + + crtc_state->connector_mask &= + ~(1 << drm_connector_index(conn_state->connector)); + } + if (crtc) { crtc_state = drm_atomic_get_crtc_state(conn_state->state, crtc); if (IS_ERR(crtc_state)) return PTR_ERR(crtc_state); + + crtc_state->connector_mask |= + 1 << drm_connector_index(conn_state->connector); }
conn_state->crtc = crtc; diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index c2f98ba2bb98..dd0db4ceab26 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -301,6 +301,7 @@ struct drm_plane_helper_funcs; * @active_changed: crtc_state->active has been toggled. * @connectors_changed: connectors to this crtc have been updated * @plane_mask: bitmask of (1 << drm_plane_index(plane)) of attached planes + * @connector_mask: bitmask of (1 << drm_connector_index(connector)) of attached connectors * @last_vblank_count: for helpers and drivers to capture the vblank of the * update to ensure framebuffer cleanup isn't done too early * @adjusted_mode: for use by helpers and drivers to compute adjusted mode timings @@ -334,6 +335,8 @@ struct drm_crtc_state { */ u32 plane_mask;
+ u32 connector_mask; + /* last_vblank_count: for vblank waits before cleanup */ u32 last_vblank_count;
On Mon, Jan 04, 2016 at 12:53:18PM +0100, Maarten Lankhorst wrote:
It can be useful to iterate over connectors without grabbing connection_mutex. It can also be used to see how many connectors are on a crtc without iterating over the list.
Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com
Merged up to this patch here, thanks. -Daniel
drivers/gpu/drm/drm_atomic.c | 11 +++++++++++ include/drm/drm_crtc.h | 3 +++ 2 files changed, 14 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 6a21e5c378c1..14b321580517 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -1063,10 +1063,21 @@ drm_atomic_set_crtc_for_connector(struct drm_connector_state *conn_state, { struct drm_crtc_state *crtc_state;
if (conn_state->crtc && conn_state->crtc != crtc) {
crtc_state = drm_atomic_get_existing_crtc_state(conn_state->state,conn_state->crtc);crtc_state->connector_mask &=~(1 << drm_connector_index(conn_state->connector));}
if (crtc) { crtc_state = drm_atomic_get_crtc_state(conn_state->state, crtc); if (IS_ERR(crtc_state)) return PTR_ERR(crtc_state);
crtc_state->connector_mask |=1 << drm_connector_index(conn_state->connector);}
conn_state->crtc = crtc;
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index c2f98ba2bb98..dd0db4ceab26 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -301,6 +301,7 @@ struct drm_plane_helper_funcs;
- @active_changed: crtc_state->active has been toggled.
- @connectors_changed: connectors to this crtc have been updated
- @plane_mask: bitmask of (1 << drm_plane_index(plane)) of attached planes
- @connector_mask: bitmask of (1 << drm_connector_index(connector)) of attached connectors
- @last_vblank_count: for helpers and drivers to capture the vblank of the
- update to ensure framebuffer cleanup isn't done too early
- @adjusted_mode: for use by helpers and drivers to compute adjusted mode timings
@@ -334,6 +335,8 @@ struct drm_crtc_state { */ u32 plane_mask;
- u32 connector_mask;
- /* last_vblank_count: for vblank waits before cleanup */ u32 last_vblank_count;
-- 2.1.0
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com --- drivers/gpu/drm/i915/intel_display.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 1e42309ec40a..b76778d76035 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -15421,6 +15421,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc) WARN_ON(drm_atomic_set_mode_for_crtc(crtc->base.state, NULL) < 0); crtc->base.state->active = crtc->active; crtc->base.enabled = crtc->active; + crtc->base.state->connector_mask = 0;
/* Because we only establish the connector -> encoder -> * crtc links if something is active, this means the @@ -15456,20 +15457,24 @@ static void intel_sanitize_encoder(struct intel_encoder *encoder) { struct intel_connector *connector; struct drm_device *dev = encoder->base.dev; + struct drm_crtc *crtc = encoder->base.crtc; bool active = false;
/* We need to check both for a crtc link (meaning that the * encoder is active and trying to read from a pipe) and the * pipe itself being active. */ - bool has_active_crtc = encoder->base.crtc && - to_intel_crtc(encoder->base.crtc)->active; + bool has_active_crtc = crtc && crtc->state->active;
for_each_intel_connector(dev, connector) { if (connector->base.encoder != &encoder->base) continue;
active = true; - break; + if (!has_active_crtc) + break; + + crtc->state->connector_mask |= + 1 << drm_connector_index(&connector->base); }
if (active && !has_active_crtc) {
On Mon, Jan 04, 2016 at 12:53:19PM +0100, Maarten Lankhorst wrote:
Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com
drivers/gpu/drm/i915/intel_display.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 1e42309ec40a..b76778d76035 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -15421,6 +15421,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc) WARN_ON(drm_atomic_set_mode_for_crtc(crtc->base.state, NULL) < 0); crtc->base.state->active = crtc->active; crtc->base.enabled = crtc->active;
crtc->base.state->connector_mask = 0;/* Because we only establish the connector -> encoder ->
- crtc links if something is active, this means the
@@ -15456,20 +15457,24 @@ static void intel_sanitize_encoder(struct intel_encoder *encoder) { struct intel_connector *connector; struct drm_device *dev = encoder->base.dev;
struct drm_crtc *crtc = encoder->base.crtc; bool active = false;
/* We need to check both for a crtc link (meaning that the
- encoder is active and trying to read from a pipe) and the
- pipe itself being active. */
- bool has_active_crtc = encoder->base.crtc &&
to_intel_crtc(encoder->base.crtc)->active;
bool has_active_crtc = crtc && crtc->state->active;
for_each_intel_connector(dev, connector) { if (connector->base.encoder != &encoder->base) continue;
active = true;
break;
if (!has_active_crtc)break;crtc->state->connector_mask |=1 << drm_connector_index(&connector->base);
I still think this is the wrong place. Imo this should be done in intel_modeset_update_connector_atomic_state. It'd be great if we could somehow share the logic with drm_atomic_set_crtc_for_connector even, but that's probably over the top. -Daniel
}
if (active && !has_active_crtc) {
2.1.0
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Op 05-01-16 om 09:35 schreef Daniel Vetter:
On Mon, Jan 04, 2016 at 12:53:19PM +0100, Maarten Lankhorst wrote:
Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com
drivers/gpu/drm/i915/intel_display.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 1e42309ec40a..b76778d76035 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -15421,6 +15421,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc) WARN_ON(drm_atomic_set_mode_for_crtc(crtc->base.state, NULL) < 0); crtc->base.state->active = crtc->active; crtc->base.enabled = crtc->active;
crtc->base.state->connector_mask = 0;/* Because we only establish the connector -> encoder ->
- crtc links if something is active, this means the
@@ -15456,20 +15457,24 @@ static void intel_sanitize_encoder(struct intel_encoder *encoder) { struct intel_connector *connector; struct drm_device *dev = encoder->base.dev;
struct drm_crtc *crtc = encoder->base.crtc; bool active = false;
/* We need to check both for a crtc link (meaning that the
- encoder is active and trying to read from a pipe) and the
- pipe itself being active. */
- bool has_active_crtc = encoder->base.crtc &&
to_intel_crtc(encoder->base.crtc)->active;
bool has_active_crtc = crtc && crtc->state->active;
for_each_intel_connector(dev, connector) { if (connector->base.encoder != &encoder->base) continue;
active = true;
break;
if (!has_active_crtc)break;crtc->state->connector_mask |=1 << drm_connector_index(&connector->base);I still think this is the wrong place. Imo this should be done in intel_modeset_update_connector_atomic_state. It'd be great if we could somehow share the logic with drm_atomic_set_crtc_for_connector even, but that's probably over the top.
No it should be done sooner. I want to be able use it anywhere in the .crtc_disable calls without worrying about it..
~Maarten
On Tue, Jan 05, 2016 at 10:05:21AM +0100, Maarten Lankhorst wrote:
Op 05-01-16 om 09:35 schreef Daniel Vetter:
On Mon, Jan 04, 2016 at 12:53:19PM +0100, Maarten Lankhorst wrote:
Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com
drivers/gpu/drm/i915/intel_display.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 1e42309ec40a..b76778d76035 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -15421,6 +15421,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc) WARN_ON(drm_atomic_set_mode_for_crtc(crtc->base.state, NULL) < 0); crtc->base.state->active = crtc->active; crtc->base.enabled = crtc->active;
crtc->base.state->connector_mask = 0;/* Because we only establish the connector -> encoder ->
- crtc links if something is active, this means the
@@ -15456,20 +15457,24 @@ static void intel_sanitize_encoder(struct intel_encoder *encoder) { struct intel_connector *connector; struct drm_device *dev = encoder->base.dev;
struct drm_crtc *crtc = encoder->base.crtc; bool active = false;
/* We need to check both for a crtc link (meaning that the
- encoder is active and trying to read from a pipe) and the
- pipe itself being active. */
- bool has_active_crtc = encoder->base.crtc &&
to_intel_crtc(encoder->base.crtc)->active;
bool has_active_crtc = crtc && crtc->state->active;
for_each_intel_connector(dev, connector) { if (connector->base.encoder != &encoder->base) continue;
active = true;
break;
if (!has_active_crtc)break;crtc->state->connector_mask |=1 << drm_connector_index(&connector->base);I still think this is the wrong place. Imo this should be done in intel_modeset_update_connector_atomic_state. It'd be great if we could somehow share the logic with drm_atomic_set_crtc_for_connector even, but that's probably over the top.
No it should be done sooner. I want to be able use it anywhere in the .crtc_disable calls without worrying about it..
Well I don't want to split things up all over. Atm our state recover is a complete mess, and we need to start recovering some order in it. Updating related things at completely different places without even a comment stating why that's required is imo a no-go. -Daniel
Op 05-01-16 om 10:10 schreef Daniel Vetter:
On Tue, Jan 05, 2016 at 10:05:21AM +0100, Maarten Lankhorst wrote:
Op 05-01-16 om 09:35 schreef Daniel Vetter:
On Mon, Jan 04, 2016 at 12:53:19PM +0100, Maarten Lankhorst wrote:
Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com
drivers/gpu/drm/i915/intel_display.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 1e42309ec40a..b76778d76035 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -15421,6 +15421,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc) WARN_ON(drm_atomic_set_mode_for_crtc(crtc->base.state, NULL) < 0); crtc->base.state->active = crtc->active; crtc->base.enabled = crtc->active;
crtc->base.state->connector_mask = 0;/* Because we only establish the connector -> encoder ->
- crtc links if something is active, this means the
@@ -15456,20 +15457,24 @@ static void intel_sanitize_encoder(struct intel_encoder *encoder) { struct intel_connector *connector; struct drm_device *dev = encoder->base.dev;
struct drm_crtc *crtc = encoder->base.crtc; bool active = false;
/* We need to check both for a crtc link (meaning that the
- encoder is active and trying to read from a pipe) and the
- pipe itself being active. */
- bool has_active_crtc = encoder->base.crtc &&
to_intel_crtc(encoder->base.crtc)->active;
bool has_active_crtc = crtc && crtc->state->active;
for_each_intel_connector(dev, connector) { if (connector->base.encoder != &encoder->base) continue;
active = true;
break;
if (!has_active_crtc)break;crtc->state->connector_mask |=1 << drm_connector_index(&connector->base);I still think this is the wrong place. Imo this should be done in intel_modeset_update_connector_atomic_state. It'd be great if we could somehow share the logic with drm_atomic_set_crtc_for_connector even, but that's probably over the top.
No it should be done sooner. I want to be able use it anywhere in the .crtc_disable calls without worrying about it..
Well I don't want to split things up all over. Atm our state recover is a complete mess, and we need to start recovering some order in it. Updating related things at completely different places without even a comment stating why that's required is imo a no-go.
Ok so if I resubmit with a comment in this hunk it will be acceptable?
~Maarten
drm/i915: Update connector_mask during readout, v2.
The connector_mask may be used any time during the non-atomic .crtc_disable which is called before the full atomic state is set up and needs to be accurate for that reason.
Changes since v1: - Update connector_mask in readout_hw_state and add a comment.
Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com --- diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index fde49d90c37f..547862f6951e 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -15602,6 +15602,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc) WARN_ON(drm_atomic_set_mode_for_crtc(crtc->base.state, NULL) < 0); crtc->base.state->active = crtc->active; crtc->base.enabled = crtc->active; + crtc->base.state->connector_mask = 0;
/* Because we only establish the connector -> encoder -> * crtc links if something is active, this means the @@ -15828,7 +15829,21 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev) for_each_intel_connector(dev, connector) { if (connector->get_hw_state(connector)) { connector->base.dpms = DRM_MODE_DPMS_ON; - connector->base.encoder = &connector->encoder->base; + + encoder = connector->encoder; + connector->base.encoder = &encoder->base; + + if (encoder->base.crtc && + encoder->base.crtc->state->active) { + /* + * This has to be done during hardware readout + * because anything calling .crtc_disable may + * rely on the connector_mask being accurate. + */ + encoder->base.crtc->state->connector_mask |= + 1 << drm_connector_index(&connector->base); + } + } else { connector->base.dpms = DRM_MODE_DPMS_OFF; connector->base.encoder = NULL;
Now that connector_mask is reliable there's no need for this function any more.
Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com --- drivers/gpu/drm/drm_atomic.c | 30 ------------------------------ drivers/gpu/drm/drm_atomic_helper.c | 10 ++++------ drivers/gpu/drm/vc4/vc4_crtc.c | 2 +- include/drm/drm_atomic.h | 4 ---- 4 files changed, 5 insertions(+), 41 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 14b321580517..d3ed12447fbb 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -1183,36 +1183,6 @@ drm_atomic_add_affected_planes(struct drm_atomic_state *state, EXPORT_SYMBOL(drm_atomic_add_affected_planes);
/** - * drm_atomic_connectors_for_crtc - count number of connected outputs - * @state: atomic state - * @crtc: DRM crtc - * - * This function counts all connectors which will be connected to @crtc - * according to @state. Useful to recompute the enable state for @crtc. - */ -int -drm_atomic_connectors_for_crtc(struct drm_atomic_state *state, - struct drm_crtc *crtc) -{ - struct drm_connector *connector; - struct drm_connector_state *conn_state; - - int i, num_connected_connectors = 0; - - for_each_connector_in_state(state, connector, conn_state, i) { - if (conn_state->crtc == crtc) - num_connected_connectors++; - } - - DRM_DEBUG_ATOMIC("State %p has %i connectors for [CRTC:%d:%s]\n", - state, num_connected_connectors, - crtc->base.id, crtc->name); - - return num_connected_connectors; -} -EXPORT_SYMBOL(drm_atomic_connectors_for_crtc); - -/** * drm_atomic_legacy_backoff - locking backoff for legacy ioctls * @state: atomic state * diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 27dd68f946e6..13b771cb0d35 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -464,7 +464,8 @@ drm_atomic_helper_check_modeset(struct drm_device *dev, * crtc only changed its mode but has the same set of connectors. */ for_each_crtc_in_state(state, crtc, crtc_state, i) { - int num_connectors; + bool has_connectors = + !!crtc_state->connector_mask;
/* * We must set ->active_changed after walking connectors for @@ -493,10 +494,7 @@ drm_atomic_helper_check_modeset(struct drm_device *dev, if (ret != 0) return ret;
- num_connectors = drm_atomic_connectors_for_crtc(state, - crtc); - - if (crtc_state->enable != !!num_connectors) { + if (crtc_state->enable != has_connectors) { DRM_DEBUG_ATOMIC("[CRTC:%d:%s] enabled/connectors mismatch\n", crtc->base.id, crtc->name);
@@ -1755,7 +1753,7 @@ static int update_output_state(struct drm_atomic_state *state, if (crtc == set->crtc) continue;
- if (!drm_atomic_connectors_for_crtc(state, crtc)) { + if (!crtc_state->connector_mask) { ret = drm_atomic_set_mode_prop_for_crtc(crtc_state, NULL); if (ret < 0) diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c index 2168a99d59aa..aa7ed24a41c7 100644 --- a/drivers/gpu/drm/vc4/vc4_crtc.c +++ b/drivers/gpu/drm/vc4/vc4_crtc.c @@ -327,7 +327,7 @@ static int vc4_crtc_atomic_check(struct drm_crtc *crtc, /* The pixelvalve can only feed one encoder (and encoders are * 1:1 with connectors.) */ - if (drm_atomic_connectors_for_crtc(state->state, crtc) > 1) + if (hweight32(state->connector_mask) > 1) return -EINVAL;
drm_atomic_crtc_state_for_each_plane(plane, state) { diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h index d8576ac55693..d3eaa5df187a 100644 --- a/include/drm/drm_atomic.h +++ b/include/drm/drm_atomic.h @@ -130,10 +130,6 @@ int __must_check drm_atomic_add_affected_planes(struct drm_atomic_state *state, struct drm_crtc *crtc);
-int -drm_atomic_connectors_for_crtc(struct drm_atomic_state *state, - struct drm_crtc *crtc); - void drm_atomic_legacy_backoff(struct drm_atomic_state *state);
void
On Mon, Jan 04, 2016 at 12:53:20PM +0100, Maarten Lankhorst wrote:
Now that connector_mask is reliable there's no need for this function any more.
Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com
Since this doesn't touch i915 I figured I can merge this one too. So except for the previous i915 patch it's now all in drm-misc. -Daniel
drivers/gpu/drm/drm_atomic.c | 30 ------------------------------ drivers/gpu/drm/drm_atomic_helper.c | 10 ++++------ drivers/gpu/drm/vc4/vc4_crtc.c | 2 +- include/drm/drm_atomic.h | 4 ---- 4 files changed, 5 insertions(+), 41 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 14b321580517..d3ed12447fbb 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -1183,36 +1183,6 @@ drm_atomic_add_affected_planes(struct drm_atomic_state *state, EXPORT_SYMBOL(drm_atomic_add_affected_planes);
/**
- drm_atomic_connectors_for_crtc - count number of connected outputs
- @state: atomic state
- @crtc: DRM crtc
- This function counts all connectors which will be connected to @crtc
- according to @state. Useful to recompute the enable state for @crtc.
- */
-int -drm_atomic_connectors_for_crtc(struct drm_atomic_state *state,
struct drm_crtc *crtc)-{
- struct drm_connector *connector;
- struct drm_connector_state *conn_state;
- int i, num_connected_connectors = 0;
- for_each_connector_in_state(state, connector, conn_state, i) {
if (conn_state->crtc == crtc)num_connected_connectors++;- }
- DRM_DEBUG_ATOMIC("State %p has %i connectors for [CRTC:%d:%s]\n",
state, num_connected_connectors,crtc->base.id, crtc->name);- return num_connected_connectors;
-} -EXPORT_SYMBOL(drm_atomic_connectors_for_crtc);
-/**
- drm_atomic_legacy_backoff - locking backoff for legacy ioctls
- @state: atomic state
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 27dd68f946e6..13b771cb0d35 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -464,7 +464,8 @@ drm_atomic_helper_check_modeset(struct drm_device *dev, * crtc only changed its mode but has the same set of connectors. */ for_each_crtc_in_state(state, crtc, crtc_state, i) {
int num_connectors;
bool has_connectors =!!crtc_state->connector_mask;/*
- We must set ->active_changed after walking connectors for
@@ -493,10 +494,7 @@ drm_atomic_helper_check_modeset(struct drm_device *dev, if (ret != 0) return ret;
num_connectors = drm_atomic_connectors_for_crtc(state,crtc);if (crtc_state->enable != !!num_connectors) {
if (crtc_state->enable != has_connectors) { DRM_DEBUG_ATOMIC("[CRTC:%d:%s] enabled/connectors mismatch\n", crtc->base.id, crtc->name);@@ -1755,7 +1753,7 @@ static int update_output_state(struct drm_atomic_state *state, if (crtc == set->crtc) continue;
if (!drm_atomic_connectors_for_crtc(state, crtc)) {
if (!crtc_state->connector_mask) { ret = drm_atomic_set_mode_prop_for_crtc(crtc_state, NULL); if (ret < 0)diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c index 2168a99d59aa..aa7ed24a41c7 100644 --- a/drivers/gpu/drm/vc4/vc4_crtc.c +++ b/drivers/gpu/drm/vc4/vc4_crtc.c @@ -327,7 +327,7 @@ static int vc4_crtc_atomic_check(struct drm_crtc *crtc, /* The pixelvalve can only feed one encoder (and encoders are * 1:1 with connectors.) */
- if (drm_atomic_connectors_for_crtc(state->state, crtc) > 1)
if (hweight32(state->connector_mask) > 1) return -EINVAL;
drm_atomic_crtc_state_for_each_plane(plane, state) {
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h index d8576ac55693..d3eaa5df187a 100644 --- a/include/drm/drm_atomic.h +++ b/include/drm/drm_atomic.h @@ -130,10 +130,6 @@ int __must_check drm_atomic_add_affected_planes(struct drm_atomic_state *state, struct drm_crtc *crtc);
-int -drm_atomic_connectors_for_crtc(struct drm_atomic_state *state,
struct drm_crtc *crtc);void drm_atomic_legacy_backoff(struct drm_atomic_state *state);
void
2.1.0
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Maarten Lankhorst