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

[Ander Conselvan De Oliveira <conselvan2@gmail.com>]

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.

> 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.

Ander

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