[Intel-gfx] [PATCH 72/89] drm/i915/skl: Enable/disable power well for aux transaction

[Imre Deak]

On Tue, 2014-11-11 at 12:22 +0000, Damien Lespiau wrote:
> On Mon, Nov 10, 2014 at 09:21:47PM +0200, Imre Deak wrote:
> > On Fri, 2014-11-07 at 12:08 +0000, Damien Lespiau wrote:
> > > On Tue, Sep 16, 2014 at 04:19:07PM +0300, Imre Deak wrote:
> > > > > @@ -1233,6 +1233,9 @@ static bool edp_panel_vdd_on(struct intel_dp *intel_dp)
> > > > >  	power_domain = intel_display_port_power_domain(intel_encoder);
> > > > >  	intel_display_power_get(dev_priv, power_domain);
> > > > >  
> > > > > +	power_domain = intel_display_aux_power_domain(intel_encoder);
> > > > > +	intel_display_power_get(dev_priv, power_domain);
> > > > > +
> > > > 
> > > > The AUX power domains were added to save power when only AUX
> > > > functionality is needed, since then we don't need to power on the power
> > > > domain needed for full port functionality.
> > > 
> > > Hum I'm not sure about this. It seems to me that the value of the AUX
> > > power domain is to be able to shutdown the AUX hardware when AUX is not
> > > needed. That's slightly different from what you're saying;
> > 
> > Ok, I didn't describe all uses cases where the AUX domains are useful,
> > your description here was more complete. To summarize that for later
> > reference:
> > 
> > 1. only AUX (output inactive, we only do a connector detection)
> > 2. only main lanes (most of the time when the output is active)
> > 3. AUX+main lanes (link training/re-training)
> > 4. no AUX, no main lanes (output is inactive)
> > 
> > > The cases where "only AUX functionality is needed" seem very transient
> > > to me, while having the main lanes working and no need for AUX sounds
> > > like the case where it's interesting to have the AUX hw powered down.
> > > Of course, with PSR we can do both.
> > 
> > Perhaps, if case 1. above isn't very frequent. But my arguments were
> > valid even for case 2. and 3.
> 
> I agree with case 2., The training case (3.) is a transient state as
> well where we can have the AUX power well always on. But yes, we should
> make sure to turn off the AUX power domain most of the time when the
> display is on, you're absolutely right on that.
> 
> > > I think it's fine if this patch is not changing anything, at least for
> > > now, until we get to use this power domain to good ends?
> > 
> > Well I'm ok not to change the functionality for now, but I'd still do
> > this by taking here only the AUX power domain. Then by having the same
> > power domain->power well mapping in intel_runtime_pm.c for the AUX
> > domains as for the port domains we keep the existing behavior. This is
> > actually done already for all existing platforms in patch 69/89 in your
> > SKL patchset (except for CHV). Patch 71/89 adds the mappings for SKL and
> > it doesn't include the SKL DDI_A_E,B,C,D power wells in the AUX power
> > domains. I think this would need to be tested if it really works
> > (triggering case 1. above), but you could also just do the easier thing
> > for now and set the AUX mappings to be identical to the corresponding
> > port mappings, as it's done for the other platforms.
> 
> Ok, had another look, and, granted, it looks funny. What I'll try:
> 
> Have us always take the AUX power domain in the AUX ->transfer vfunc.
> This won't toggle the power well on/off for each transfer if we
> correctly wrap known heavy users of the AUX channel, intel_dp_detect(),
> intel_dp_get_edid(), intel_dp_force and intel_dp_hpd_pulse() so we keep
> the power well alive for the duration of those. This will also allow
> one-of AUX transfers from DRM core when the power is down.

Yes, this sounds ok to me. Iiuc this will change all places in
intel_dp.c to take the AUX domain instead of the port domain.
intel_dp_get_hw_state() should still check the port domain, since there
we are only interested in the HW state for the main lanes not the HW
state for AUX.

--Imre