[Intel-gfx] [PATCH] drm/i915: Fix broken mst get_hw_state.

[Jani Nikula]

On Thu, 03 Sep 2015, Ander Conselvan De Oliveira <conselvan2 at gmail.com> wrote:
> On Wed, 2015-09-02 at 16:14 +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 at linux.intel.com>
>> > > ---
>> > > 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.
>
> Fair enough.
>
> Reviewed-by: Ander Conselvan de Oliveira <conselvan2 at gmail.com>

Pushed to drm-intel-next-fixes, thanks for the patch and review.

BR,
Jani.


> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center