[Intel-gfx] [PATCH 04/19] drm/i915: Allocate a crtc_state also when the crtc is being disabled

[Daniel Vetter]

On Fri, Mar 20, 2015 at 12:06:46PM +0200, Ander Conselvan De Oliveira wrote:
> On Fri, 2015-03-20 at 10:51 +0100, Daniel Vetter wrote:
> > On Fri, Mar 20, 2015 at 10:40:37AM +0200, Ander Conselvan De Oliveira wrote:
> > > On Thu, 2015-03-19 at 23:23 +0000, Konduru, Chandra wrote:
> > > > 1) 
> > > > Consider two simple scenarios:  
> > > > Case1: User code moving HDMI from A to B:
> > > > drmModeSetCrtc(crtcA, HDMI);
> > > > drmModeSetCrtc(crtcB, HDMI); /* moving HDMI to pipe B */
> > > > 
> > > > Case2: User code turning off HDMI:
> > > > drmModeSetCrtc(crtcA, HDMI);
> > > > drmModeSetCrtc(crtcA, disable);
> > > > 
> > > > In both cases, driver will be disabling crtc for pipe A. 
> > > > In case 1, there is no associated crtc_state or compute & commit but 
> > > > directly triggering crtc_disable(crtcA).
> > > > In case 2, there is associated crtc_state and associated compute and setmode
> > > > calls crtc_disable(crtcA);
> > > > 
> > > > Won't this cause trouble for low level functions (disable clocks, connectors, 
> > > > encoders, planes etc. etc...) acting on variables being computed and staged 
> > > > in their respective states?
> > > >     where case 1 calls with current crtc->config, 
> > > >     and case 2 calls crt->config which is computed crtc_state
> > > 
> > > It is inconsistent, yes. But at the moment, for the disable case, we
> > > just duplicate the crtc_state and set crtc_state->base.enable = false.
> > > As things stand at the moment, the net effect should be the same: we
> > > call the disable hook before changing the current state, and after we
> > > change the states, the only field that changed was
> > > crtc_state->base.enable. The only difference is what does
> > > intel_crtc->config points to.
> > 
> > I didn't digg into full details but don't we need to at least clear out
> > shared resources like plls too? Or is that all guarded with checks for
> > crtc_state->enable?
> 
> We don't update crtc_state->shared_dpll during disable. But the crtc off
> function calls intel_put_shared_dpll() that removes the crtc from the
> shared dpll's crtc mask (in global state). When we enable we allocate a
> new state that has crtc_state->shared_dpll = DPLL_ID_PRIVATE.
> 
> Bottom line is, we don't rely on the old state when enabling a crtc.

Ah right I've missed that. Need to keep an eye out for this bit to make
sure we keep it working. Also it's another case where we change the states
still after the check phase, which is a big no-no in atomic land.

> > Overall it sounds like clearing the crtc state properly when duplicating
> > will be a lot of tricky fun.
> 
> Yeah, sounds like there's still "fun" times ahead.

Indeed.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch