Re: [PATCH 1/2] drm/i915: Only break encoder linked when linked to connector

[Daniel Vetter <daniel@ffwll.ch>]

On Mon, Apr 14, 2014 at 07:26:08PM +0200, Egbert Eich wrote:
> Depending on the SDVO output_flags SDVO may have multiple connectors
> linking to the same encoder (in intel_connector->encoder->base).
> Only one of those connectors should be active (ie link to the encoder
> thru drm_connector->encoder.
> If intel_connector_break_all_links() is called from intel_sanitize_crtc()
> we may brake the crtc connection of an encoder thru an inactive connector
> in which case intel_connector_break_all_links() will not be called again
> for the active connector due to
>    if (connector->encoder->base.crtc != &crtc->base)
>                                 continue;
> in intel_sanitize_crtc().
> This will however leave the drm_connector->encoder linkage for this
> active connector in place. Subsequently this will cause multiple
> warnings in intel_connector_check_state() to trigger and the driver
> will eventually die in drm_encoder_crtc_ok() (because of crtc == NULL).
> 
> To avoid this break the encoder links only if the connector is active
> (ie. has drm_connector->encoder set).
> 
> Signed-off-by: Egbert Eich <eich@suse.de>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 1390ab5..041f847 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11390,6 +11390,8 @@ static void
>  intel_connector_break_all_links(struct intel_connector *connector)
>  {
>  	connector->base.dpms = DRM_MODE_DPMS_OFF;
> +	if (!connector->base.encoder)
> +		return;

Hm, I think to address Chris' concern we should split this into two
pieces: connector_break_all links which only breaks connector->encoder
stuff, used in both places as is. And a new encoder_break_all links which
we'll use in sanitize_crtc in a new encoder loop.

Really nice catch though!
-Daniel
>  	connector->base.encoder = NULL;
>  	connector->encoder->connectors_active = false;
>  	connector->encoder->base.crtc = NULL;
> -- 
> 1.8.4.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch