On Wed, Sep 02, 2015 at 04:14:27PM +0200, Maarten Lankhorst wrote:
> Op 02-09-15 om 16:07 schreef Ander Conselvan De Oliveira:
> > On Thu, 2015-08-27 at 13:13 +0200, Maarten Lankhorst wrote:
> >> connector->encoder is initialized as NULL. Fix this by setting it in
> >> during pre enable. MST connectors are not read out during initial hw
> >> readout, and have no fixed encoder mappings. So it's harmless to
> >> return false when the connector has never been assigned to an encoder.
> >>
> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> >> ---
> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >> index 738178a0ac96..e3922d973db0 100644
> >> --- a/drivers/gpu/drm/i915/intel_display.c
> >> +++ b/drivers/gpu/drm/i915/intel_display.c
> >> @@ -6283,7 +6283,7 @@ static void intel_connector_check_state(struct intel_connector *connector)
> >> connector->base.name);
> >>
> >> if (connector->get_hw_state(connector)) {
> >> - struct drm_encoder *encoder = &connector->encoder->base;
> >> + struct intel_encoder *encoder = connector->encoder;
> >> struct drm_connector_state *conn_state = connector->base.state;
> >>
> >> I915_STATE_WARN(!crtc,
> >> @@ -6295,13 +6295,13 @@ static void intel_connector_check_state(struct intel_connector *connector)
> >> I915_STATE_WARN(!crtc->state->active,
> >> "connector is active, but attached crtc isn't\n");
> >>
> >> - if (!encoder)
> >> + if (!encoder || encoder->type == INTEL_OUTPUT_DP_MST)
> >> return;
> >>
> >> - I915_STATE_WARN(conn_state->best_encoder != encoder,
> >> + I915_STATE_WARN(conn_state->best_encoder != &encoder->base,
> >> "atomic encoder doesn't match attached encoder\n");
> >>
> >> - I915_STATE_WARN(conn_state->crtc != encoder->crtc,
> >> + I915_STATE_WARN(conn_state->crtc != encoder->base.crtc,
> >> "attached encoder crtc differs from connector crtc\n");
> >> } else {
> >> I915_STATE_WARN(crtc && crtc->state->active,
> >> diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
> >> index ebf205462631..d606721b1788 100644
> >> --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> >> +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> >> @@ -159,6 +159,11 @@ static void intel_mst_pre_enable_dp(struct intel_encoder *encoder)
> >> return;
> >> }
> >>
> >> + /* MST encoders are bound to a crtc, not to a connector,
> >> + * force the mapping here for get_hw_state.
> >> + */
> >> + found->encoder = encoder;
> >> +
> >> DRM_DEBUG_KMS("%d\n", intel_dp->active_mst_links);
> >> intel_mst->port = found->port;
> >>
> >> @@ -392,7 +397,7 @@ static const struct drm_encoder_funcs intel_dp_mst_enc_funcs = {
> >>
> >> static bool intel_dp_mst_get_hw_state(struct intel_connector *connector)
> >> {
> >> - if (connector->encoder) {
> >> + if (connector->encoder && connector->base.state->crtc) {
> > Is this here to cover the case where we disable this connector and assign the encoder to a different
> > connector? I guess this should work since if we disable the connector than crtc will be null, and
> > we'll assign a proper encoder when re-enabling. But now I'm wondering if it would make sense to set
> > connector->encoder back to NULL in post_disable.
> >
> Yes, it's harmless to keep it non-null if conn_state->crtc is checked though. Saves another pointless loop.
Hm in get_hw_state functions we really shouldn't look at state pointers
like connector->encoder or state->crtc at all. It /should/ be all
free-standing for fastboot to actually work.
But I guess that's a lot more than just one patch ;-)
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
