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 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
