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

[Imre Deak]

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.

> > With the above change and everywhere else below we'll end up enabling
> > both power domains, though we only need AUX functionality.
> 
> If we're powering up the panel that's probably to use it very soon, so I
> don't really see the value not powering the main lanes at the same time,
> they are going to be used for training very soon? I'm probably missing
> something.

Again depends how important case 1. is. But my point was that in all the
functions where this patch takes the AUX power domain (after rebasing
the edp vdd on/off and the pps_lock/unlock functions) we only want to
enable the resources needed for AUX communication. The power domain
needed for main port functionality (that is the port power domain) is
already taken in display.modeset_global_resources() for case 2. and 3.
above.

> > The power wells needed for AUX are a subset of those needed for full
> > port functionality on all platforms (at least atm), so this patch won't
> > change anything. The patch would make sense, if you requested only the
> > AUX domains.
> 
> 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.

--Imre

> This patch still need the reworks you mentionned in the previous mail,
> of course.