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

[Damien Lespiau]

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.

-- 
Damien