With legacy helpers all the routing was already set up when calling best_encoder and so could be inspected. But with atomic it's staged, hence we need a new atomic compliant callback for drivers which need to inspect the requested state and can't just decided the best encoder statically.
This is needed to fix up i915 dp mst where we need to pick the right encoder depending upon the requested CRTC for the connector.
v2: Don't forget to amend the kerneldoc
Cc: Chris Wilson chris@chris-wilson.co.uk Cc: Linus Torvalds torvalds@linux-foundation.org Cc: Theodore Ts'o tytso@mit.edu Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/drm_atomic_helper.c | 7 ++++++- include/drm/drm_crtc_helper.h | 3 +++ 2 files changed, 9 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index aac212297b49..8694ca9beee3 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -196,7 +196,12 @@ update_connector_routing(struct drm_atomic_state *state, int conn_idx) }
funcs = connector->helper_private; - new_encoder = funcs->best_encoder(connector); + + if (funcs->atomic_best_encoder) + new_encoder = funcs->atomic_best_encoder(connector, + connector_state); + else + new_encoder = funcs->best_encoder(connector);
if (!new_encoder) { DRM_DEBUG_ATOMIC("No suitable encoder found for [CONNECTOR:%d:%s]\n", diff --git a/include/drm/drm_crtc_helper.h b/include/drm/drm_crtc_helper.h index c8fc187061de..918aa68b5199 100644 --- a/include/drm/drm_crtc_helper.h +++ b/include/drm/drm_crtc_helper.h @@ -168,6 +168,7 @@ struct drm_encoder_helper_funcs { * @get_modes: get mode list for this connector * @mode_valid: is this mode valid on the given connector? (optional) * @best_encoder: return the preferred encoder for this connector + * @atomic_best_encoder: atomic version of @best_encoder * * The helper operations are called by the mid-layer CRTC helper. */ @@ -176,6 +177,8 @@ struct drm_connector_helper_funcs { enum drm_mode_status (*mode_valid)(struct drm_connector *connector, struct drm_display_mode *mode); struct drm_encoder *(*best_encoder)(struct drm_connector *connector); + struct drm_encoder *(*atomic_best_encoder)(struct drm_connector *connector, + struct drm_connector_state *connector_state); };
extern void drm_helper_disable_unused_functions(struct drm_device *dev);
In
commit 8c7b5ccb729870e606321b3703e2c2e698c49a95 Author: Ander Conselvan de Oliveira ander.conselvan.de.oliveira@intel.com Date: Tue Apr 21 17:13:19 2015 +0300
drm/i915: Use atomic helpers for computing changed flags
we've switched over to the atomic version to compute the crtc->encoder->connector routing from the i915 variant. That one relies upon the ->best_encoder callback, but the i915-private version relied upon intel_find_encoder. Which didn't matter except for dp mst, where the encoder depends upon the selected crtc.
Fix this functional bug by implemented a correct atomic-state based encoder selector for dp mst.
Note that we can't get rid of the legacy best_encoder callback since the fbdev emulation uses that still. That means it's incorrect there still, but that's been the case ever since i915 dp mst support was merged so not a regression. Best to fix that by converting fbdev over to atomic too.
Cc: Chris Wilson chris@chris-wilson.co.uk Cc: Linus Torvalds torvalds@linux-foundation.org Cc: Theodore Ts'o tytso@mit.edu Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/i915/intel_dp_mst.c | 11 +++++++++++ 1 file changed, 11 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c index 6e4cc5334f47..600afdbef8c9 100644 --- a/drivers/gpu/drm/i915/intel_dp_mst.c +++ b/drivers/gpu/drm/i915/intel_dp_mst.c @@ -357,6 +357,16 @@ intel_dp_mst_mode_valid(struct drm_connector *connector, return MODE_OK; }
+static struct drm_encoder *intel_mst_atomic_best_encoder(struct drm_connector *connector, + struct drm_connector_state *state) +{ + struct intel_connector *intel_connector = to_intel_connector(connector); + struct intel_dp *intel_dp = intel_connector->mst_port; + struct intel_crtc *crtc = to_intel_crtc(state->crtc); + + return &intel_dp->mst_encoders[crtc->pipe]->base.base; +} + static struct drm_encoder *intel_mst_best_encoder(struct drm_connector *connector) { struct intel_connector *intel_connector = to_intel_connector(connector); @@ -367,6 +377,7 @@ static struct drm_encoder *intel_mst_best_encoder(struct drm_connector *connecto static const struct drm_connector_helper_funcs intel_dp_mst_connector_helper_funcs = { .get_modes = intel_dp_mst_get_modes, .mode_valid = intel_dp_mst_mode_valid, + .atomic_best_encoder = intel_mst_atomic_best_encoder, .best_encoder = intel_mst_best_encoder, };
Apparently been in there since forever and fairly easy to hit when hotplugging really fast. I can do that since my mst hub has a manual button to flick the hpd line for reprobing. The resulting WARNING spam isn't pretty.
Cc: Dave Airlie airlied@gmail.com Cc: stable@vger.kernel.org Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/drm_dp_mst_topology.c | 1 - 1 file changed, 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index 778bbb6425b8..b0487c9f018c 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -1294,7 +1294,6 @@ retry: goto retry; } DRM_DEBUG_KMS("failed to dpcd write %d %d\n", tosend, ret); - WARN(1, "fail\n");
return -EIO; }
On Mon, Aug 03, 2015 at 05:24:10PM +0200, Daniel Vetter wrote:
Apparently been in there since forever and fairly easy to hit when hotplugging really fast. I can do that since my mst hub has a manual button to flick the hpd line for reprobing. The resulting WARNING spam isn't pretty.
Cc: Dave Airlie airlied@gmail.com Cc: stable@vger.kernel.org Signed-off-by: Daniel Vetter daniel.vetter@intel.com
drivers/gpu/drm/drm_dp_mst_topology.c | 1 - 1 file changed, 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index 778bbb6425b8..b0487c9f018c 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -1294,7 +1294,6 @@ retry: goto retry; } DRM_DEBUG_KMS("failed to dpcd write %d %d\n", tosend, ret);
WARN(1, "fail\n"); return -EIO;}
Reviewed-by: Thierry Reding treding@nvidia.com
We've had a few issues with atomic where subtle bugs in the encoder picking logic lead to accidental self-stealing of the encoder, resulting in a NULL connector_state->crtc in update_connector_routing and subsequent.
Linus applied some duct-tape for an mst regression in
commit 27667f4744fc5a0f3e50910e78740bac5670d18b Author: Linus Torvalds torvalds@linux-foundation.org Date: Wed Jul 29 22:18:16 2015 -0700
i915: temporary fix for DP MST docking station NULL pointer dereference
But that was incomplete (the code will still oops when debuggin is enabled) and mangled the state even further. So instead WARN and bail out as the more future-proof option.
Cc: Theodore Ts'o tytso@mit.edu Cc: Linus Torvalds torvalds@linux-foundation.org Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/drm_atomic_helper.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 8694ca9beee3..9dcc7280e572 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -234,13 +234,14 @@ update_connector_routing(struct drm_atomic_state *state, int conn_idx) } }
+ if (WARN_ON(!connector_state->crtc)) + return -EINVAL; + connector_state->best_encoder = new_encoder; - if (connector_state->crtc) { - idx = drm_crtc_index(connector_state->crtc); + idx = drm_crtc_index(connector_state->crtc);
- crtc_state = state->crtc_states[idx]; - crtc_state->mode_changed = true; - } + crtc_state = state->crtc_states[idx]; + crtc_state->mode_changed = true;
DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] using [ENCODER:%d:%s] on [CRTC:%d]\n", connector->base.id,
For the series:
Reviewed-by: Ander Conselvan de Oliveira conselvan2@gmail.com
On Mon, 2015-08-03 at 17:24 +0200, Daniel Vetter wrote:
We've had a few issues with atomic where subtle bugs in the encoder picking logic lead to accidental self-stealing of the encoder, resulting in a NULL connector_state->crtc in update_connector_routing and subsequent.
Linus applied some duct-tape for an mst regression in
commit 27667f4744fc5a0f3e50910e78740bac5670d18b Author: Linus Torvalds torvalds@linux-foundation.org Date: Wed Jul 29 22:18:16 2015 -0700
i915: temporary fix for DP MST docking station NULL pointer dereferenceBut that was incomplete (the code will still oops when debuggin is enabled) and mangled the state even further. So instead WARN and bail out as the more future-proof option.
Cc: Theodore Ts'o tytso@mit.edu Cc: Linus Torvalds torvalds@linux-foundation.org Signed-off-by: Daniel Vetter daniel.vetter@intel.com
drivers/gpu/drm/drm_atomic_helper.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 8694ca9beee3..9dcc7280e572 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -234,13 +234,14 @@ update_connector_routing(struct drm_atomic_state *state, int conn_idx) } }
- if (WARN_ON(!connector_state->crtc))
return -EINVAL;- connector_state->best_encoder = new_encoder;
- if (connector_state->crtc) {
idx = drm_crtc_index(connector_state->crtc);
- idx = drm_crtc_index(connector_state->crtc);
crtc_state = state->crtc_states[idx];crtc_state->mode_changed = true;- }
crtc_state = state->crtc_states[idx];
crtc_state->mode_changed = true;
DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] using [ENCODER:%d:%s] on [CRTC:%d]\n", connector->base.id,
On Tue, Aug 04, 2015 at 11:56:08AM +0300, Ander Conselvan De Oliveira wrote:
For the series:
Reviewed-by: Ander Conselvan de Oliveira conselvan2@gmail.com
Thanks for the review. -Daniel
On Mon, 2015-08-03 at 17:24 +0200, Daniel Vetter wrote:
We've had a few issues with atomic where subtle bugs in the encoder picking logic lead to accidental self-stealing of the encoder, resulting in a NULL connector_state->crtc in update_connector_routing and subsequent.
Linus applied some duct-tape for an mst regression in
commit 27667f4744fc5a0f3e50910e78740bac5670d18b Author: Linus Torvalds torvalds@linux-foundation.org Date: Wed Jul 29 22:18:16 2015 -0700
i915: temporary fix for DP MST docking station NULL pointer dereferenceBut that was incomplete (the code will still oops when debuggin is enabled) and mangled the state even further. So instead WARN and bail out as the more future-proof option.
Cc: Theodore Ts'o tytso@mit.edu Cc: Linus Torvalds torvalds@linux-foundation.org Signed-off-by: Daniel Vetter daniel.vetter@intel.com
drivers/gpu/drm/drm_atomic_helper.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 8694ca9beee3..9dcc7280e572 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -234,13 +234,14 @@ update_connector_routing(struct drm_atomic_state *state, int conn_idx) } }
- if (WARN_ON(!connector_state->crtc))
return -EINVAL;- connector_state->best_encoder = new_encoder;
- if (connector_state->crtc) {
idx = drm_crtc_index(connector_state->crtc);
- idx = drm_crtc_index(connector_state->crtc);
crtc_state = state->crtc_states[idx];crtc_state->mode_changed = true;- }
crtc_state = state->crtc_states[idx];
crtc_state->mode_changed = true;
DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] using [ENCODER:%d:%s] on [CRTC:%d]\n", connector->base.id,
On Mon, Aug 03, 2015 at 05:24:11PM +0200, Daniel Vetter wrote:
We've had a few issues with atomic where subtle bugs in the encoder picking logic lead to accidental self-stealing of the encoder, resulting in a NULL connector_state->crtc in update_connector_routing and subsequent.
Linus applied some duct-tape for an mst regression in
commit 27667f4744fc5a0f3e50910e78740bac5670d18b Author: Linus Torvalds torvalds@linux-foundation.org Date: Wed Jul 29 22:18:16 2015 -0700
i915: temporary fix for DP MST docking station NULL pointer dereferenceBut that was incomplete (the code will still oops when debuggin is enabled) and mangled the state even further. So instead WARN and bail out as the more future-proof option.
Cc: Theodore Ts'o tytso@mit.edu Cc: Linus Torvalds torvalds@linux-foundation.org Signed-off-by: Daniel Vetter daniel.vetter@intel.com
drivers/gpu/drm/drm_atomic_helper.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 8694ca9beee3..9dcc7280e572 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -234,13 +234,14 @@ update_connector_routing(struct drm_atomic_state *state, int conn_idx) } }
- if (WARN_ON(!connector_state->crtc))
return -EINVAL;- connector_state->best_encoder = new_encoder;
- if (connector_state->crtc) {
idx = drm_crtc_index(connector_state->crtc);
- idx = drm_crtc_index(connector_state->crtc);
crtc_state = state->crtc_states[idx];crtc_state->mode_changed = true;- }
crtc_state = state->crtc_states[idx];
crtc_state->mode_changed = true;
DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] using [ENCODER:%d:%s] on [CRTC:%d]\n", connector->base.id,
Reviewed-by: Thierry Reding treding@nvidia.com
On Mon, Aug 03, 2015 at 05:24:08PM +0200, Daniel Vetter wrote:
With legacy helpers all the routing was already set up when calling best_encoder and so could be inspected. But with atomic it's staged, hence we need a new atomic compliant callback for drivers which need to inspect the requested state and can't just decided the best encoder statically.
This is needed to fix up i915 dp mst where we need to pick the right encoder depending upon the requested CRTC for the connector.
v2: Don't forget to amend the kerneldoc
Cc: Chris Wilson chris@chris-wilson.co.uk Cc: Linus Torvalds torvalds@linux-foundation.org Cc: Theodore Ts'o tytso@mit.edu Signed-off-by: Daniel Vetter daniel.vetter@intel.com
drivers/gpu/drm/drm_atomic_helper.c | 7 ++++++- include/drm/drm_crtc_helper.h | 3 +++ 2 files changed, 9 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index aac212297b49..8694ca9beee3 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -196,7 +196,12 @@ update_connector_routing(struct drm_atomic_state *state, int conn_idx) }
funcs = connector->helper_private;
- new_encoder = funcs->best_encoder(connector);
if (funcs->atomic_best_encoder)
new_encoder = funcs->atomic_best_encoder(connector,connector_state);else
new_encoder = funcs->best_encoder(connector);if (!new_encoder) { DRM_DEBUG_ATOMIC("No suitable encoder found for [CONNECTOR:%d:%s]\n",
diff --git a/include/drm/drm_crtc_helper.h b/include/drm/drm_crtc_helper.h index c8fc187061de..918aa68b5199 100644 --- a/include/drm/drm_crtc_helper.h +++ b/include/drm/drm_crtc_helper.h @@ -168,6 +168,7 @@ struct drm_encoder_helper_funcs {
- @get_modes: get mode list for this connector
- @mode_valid: is this mode valid on the given connector? (optional)
- @best_encoder: return the preferred encoder for this connector
- @atomic_best_encoder: atomic version of @best_encoder
This could arguably be more verbose, but the limits of kerneldoc don't make that easy. Perhaps a more verbose description could be provided in done in the DRM DocBook, along with other documentation about atomic mode-setting.
Irrespective of that, this makes sense, so:
Acked-by: Thierry Reding treding@nvidia.com
dri-devel@lists.freedesktop.org
-
Ander Conselvan De Oliveira -
Daniel Vetter -
Daniel Vetter -
Thierry Reding