Re: [PATCH 05/15] drm/i915: Pass crtc_state and connector_state to encoder functions

From: Daniel Vetter <daniel@ffwll.ch>
To: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 05/15] drm/i915: Pass crtc_state and connector_state to encoder functions
Date: Tue, 23 Aug 2016 10:33:48 +0200	[thread overview]
Message-ID: <20160823083348.GA10980@phenom.ffwll.local> (raw)
In-Reply-To: <a03baca0-de9c-8260-0e11-85aed652a713@linux.intel.com>

On Tue, Aug 23, 2016 at 10:18:08AM +0200, Maarten Lankhorst wrote:
> Op 22-08-16 om 16:32 schreef Daniel Vetter:
> > On Mon, Aug 22, 2016 at 10:06:22AM +0200, Maarten Lankhorst wrote:
> >> Op 18-08-16 om 15:30 schreef Daniel Vetter:
> >>> On Tue, Aug 09, 2016 at 05:04:04PM +0200, Maarten Lankhorst wrote:
> >>>> This is mostly code churn, with exception of a few places:
> >>>> - intel_display.c has changes in intel_sanitize_encoder
> >>>> - intel_ddi.c has intel_ddi_fdi_disable calling intel_ddi_post_disable,
> >>>>   and required a function change. Also affects intel_display.c
> >>>> - intel_dp_mst.c passes a NULL crtc_state and conn_state to
> >>>>   intel_ddi_post_disable for shutting down the real encoder.
> >>> I didn't find this NULL callsite. Outdated commit message?
> >> intel_dp_mst.c:
> >>
> >> +		intel_dig_port->base.post_disable(&intel_dig_port->base,
> >> +						  old_crtc_state, NULL);
> >>
> >> Though it seems I accidentally pass old_crtc_state, that one should be NULL too.
> > Indeed I somehow missed that one. Still, why do we need to pass NULL
> > there? We should be able to wire up the state passing completely.
> In MST mode we have to do some enabling of the port regardless of crtc/MST connector.
> 
> If we would pass conn_state, then conn_state->connector !=
> intel_dig_port->connector and conn_state->best_encoder !=
> to_intel_encoder(intel_dig_port).  Why even bother passing conn_state at
> that point?
> 
> We also shouldn't pass crtc_state, because in that case the disabling
> sequence may potentially be different depending on which crtc is
> disabled last. Nice source of bugs. :)

Ok, makes sense. Otoh I think we should just make sure that dp port
relevant parts of the crtc all match between all the mst streams.

But yeah passing the connector state doesn't make sense I guess.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx