| Message ID | 1409830075-11139-73-git-send-email-damien.lespiau@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 2014-09-04 at 12:27 +0100, Damien Lespiau wrote: > From: Satheeshakrishna M <satheeshakrishna.m@intel.com> > > This patch enables power well 2 required for any aux transaction. > > v2: Implemented Imre's comments > - In EDID/DPCD related routines, request AUX power well in SKL > > v3: Implemented Imre's comments > - Call AUX power well domain unconditionally for all platforms > > v4: Remove the check on the output type for the AUX power domain > > v5: Rebase on top of drm-intel-nightly (Damien) > > v6: Rebase on top of -nightly (minor conflict in intel_drv.h) (Damien) > > v7: Remove platform check while getting power well for port (Imre) > > v8: Fix aux power handling around Vdd on/off (Damien) > > v9: Acquire aux power domain on enabling vdd in > intel_edp_panel_vdd_sanitize() (Satheesh) > > v10: Rebase on top of Ville's patch to return early in this function intead of > having a big indented block. (Damien) > > v12: Rebase on top of Chris EDID caching work (Damien) > > Signed-off-by: Satheeshakrishna M <satheeshakrishna.m@intel.com> (v4, v9) > Signed-off-by: Damien Lespiau <damien.lespiau@intel.com> This patch needs to be rebased on the recent PPS changes at least getting/putting the AUX power domain in pps_lock()/pps_unlock() too. Also it should be squashed into 69/89. Some more comments below. > --- > drivers/gpu/drm/i915/intel_display.c | 21 ++++++++++++ > drivers/gpu/drm/i915/intel_dp.c | 62 ++++++++++++++++++++++++++---------- > drivers/gpu/drm/i915/intel_drv.h | 2 ++ > 3 files changed, 68 insertions(+), 17 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 0a4dd00..abd4201 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -4503,6 +4503,27 @@ intel_display_port_power_domain(struct intel_encoder *intel_encoder) > } > } > > +enum intel_display_power_domain > +intel_display_aux_power_domain(struct intel_encoder *intel_encoder) > +{ > + struct intel_digital_port *intel_dig_port; > + > + intel_dig_port = enc_to_dig_port(&intel_encoder->base); > + switch (intel_dig_port->port) { > + case PORT_A: > + return POWER_DOMAIN_AUX_A; > + case PORT_B: > + return POWER_DOMAIN_AUX_B; > + case PORT_C: > + return POWER_DOMAIN_AUX_C; > + case PORT_D: > + return POWER_DOMAIN_AUX_D; > + default: > + WARN_ON_ONCE(1); > + return 0; > + } > +} > + > static unsigned long get_crtc_power_domains(struct drm_crtc *crtc) > { > struct drm_device *dev = crtc->dev; > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 93bd9bf..a983b40 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -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. With the above change and everywhere else below we'll end up enabling both power domains, though we only need AUX functionality. 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. > DRM_DEBUG_KMS("Turning eDP VDD on\n"); > > if (!edp_have_panel_power(intel_dp)) > @@ -1309,8 +1312,12 @@ static void edp_panel_vdd_off_sync(struct intel_dp *intel_dp) > > power_domain = intel_display_port_power_domain(intel_encoder); > intel_display_power_put(dev_priv, power_domain); > + > + power_domain = intel_display_aux_power_domain(intel_encoder); > + intel_display_power_put(dev_priv, power_domain); > } intel_edp_panel_off() needs to be changed too accordingly. > > + Extra w/s. > static void edp_panel_vdd_work(struct work_struct *__work) > { > struct intel_dp *intel_dp = container_of(to_delayed_work(__work), > @@ -3836,7 +3843,13 @@ g4x_dp_detect(struct intel_dp *intel_dp) > static struct edid * > intel_dp_get_edid(struct intel_dp *intel_dp) > { > + struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); > + struct intel_encoder *intel_encoder = &intel_dig_port->base; > + struct drm_device *dev = intel_encoder->base.dev; > + struct drm_i915_private *dev_priv = dev->dev_private; > struct intel_connector *intel_connector = intel_dp->attached_connector; > + enum intel_display_power_domain power_domain; > + struct edid *edid; > > /* use cached edid if we have one */ > if (intel_connector->edid) { > @@ -3845,9 +3858,16 @@ intel_dp_get_edid(struct intel_dp *intel_dp) > return NULL; > > return drm_edid_duplicate(intel_connector->edid); > - } else > - return drm_get_edid(&intel_connector->base, > - &intel_dp->aux.ddc); > + } else { > + power_domain = intel_display_aux_power_domain(intel_encoder); > + intel_display_power_get(dev_priv, power_domain); This is redundant, the higher level intel_dp_detect(), intel_dp_force() already get/put the needed power domains. > + > + edid = drm_get_edid(&intel_connector->base, &intel_dp->aux.ddc); > + > + intel_display_power_put(dev_priv, power_domain); > + > + return edid; > + } > } > > static void > @@ -3876,24 +3896,30 @@ intel_dp_unset_edid(struct intel_dp *intel_dp) > intel_dp->has_audio = false; > } > > -static enum intel_display_power_domain > -intel_dp_power_get(struct intel_dp *dp) > +static void intel_dp_power_get(struct intel_dp *dp) > { > struct intel_encoder *encoder = &dp_to_dig_port(dp)->base; > + struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); > enum intel_display_power_domain power_domain; > > power_domain = intel_display_port_power_domain(encoder); > intel_display_power_get(to_i915(encoder->base.dev), power_domain); > > - return power_domain; > + power_domain = intel_display_aux_power_domain(encoder); > + intel_display_power_get(dev_priv, power_domain); > } > > -static void > -intel_dp_power_put(struct intel_dp *dp, > - enum intel_display_power_domain power_domain) > +static void intel_dp_power_put(struct intel_dp *dp) > { > struct intel_encoder *encoder = &dp_to_dig_port(dp)->base; > - intel_display_power_put(to_i915(encoder->base.dev), power_domain); > + struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); > + enum intel_display_power_domain power_domain; > + > + power_domain = intel_display_port_power_domain(encoder); > + intel_display_power_put(dev_priv, power_domain); > + > + power_domain = intel_display_aux_power_domain(encoder); > + intel_display_power_put(dev_priv, power_domain); > } > > static enum drm_connector_status > @@ -3904,7 +3930,6 @@ intel_dp_detect(struct drm_connector *connector, bool force) > struct intel_encoder *intel_encoder = &intel_dig_port->base; > struct drm_device *dev = connector->dev; > enum drm_connector_status status; > - enum intel_display_power_domain power_domain; > bool ret; > > DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", > @@ -3918,7 +3943,7 @@ intel_dp_detect(struct drm_connector *connector, bool force) > return connector_status_disconnected; > } > > - power_domain = intel_dp_power_get(intel_dp); > + intel_dp_power_get(intel_dp); Based on the above intel_dp_power_get() would only enable the AUX power domain, which means we could keep the current prototype for intel_dp_power_get()/put(). > > /* Can't disconnect eDP, but you can close the lid... */ > if (is_edp(intel_dp)) > @@ -3949,7 +3974,7 @@ intel_dp_detect(struct drm_connector *connector, bool force) > status = connector_status_connected; > > out: > - intel_dp_power_put(intel_dp, power_domain); > + intel_dp_power_put(intel_dp); > return status; > } > > @@ -3958,7 +3983,6 @@ intel_dp_force(struct drm_connector *connector) > { > struct intel_dp *intel_dp = intel_attached_dp(connector); > struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base; > - enum intel_display_power_domain power_domain; > > DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", > connector->base.id, connector->name); > @@ -3967,11 +3991,11 @@ intel_dp_force(struct drm_connector *connector) > if (connector->status != connector_status_connected) > return; > > - power_domain = intel_dp_power_get(intel_dp); > + intel_dp_power_get(intel_dp); > > intel_dp_set_edid(intel_dp); > > - intel_dp_power_put(intel_dp, power_domain); > + intel_dp_power_put(intel_dp); > > if (intel_encoder->type != INTEL_OUTPUT_EDP) > intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT; > @@ -4205,7 +4229,8 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd) > port_name(intel_dig_port->port), > long_hpd ? "long" : "short"); > > - power_domain = intel_display_port_power_domain(intel_encoder); > + power_domain = intel_display_aux_power_domain(intel_encoder); > + > intel_display_power_get(dev_priv, power_domain); > > if (long_hpd) { > @@ -4648,6 +4673,9 @@ void intel_edp_panel_vdd_sanitize(struct intel_encoder *intel_encoder) > 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); > + > edp_panel_vdd_schedule_off(intel_dp); > } > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 9558f07..a407d04 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -916,6 +916,8 @@ void hsw_disable_ips(struct intel_crtc *crtc); > void intel_display_set_init_power(struct drm_i915_private *dev, bool enable); > enum intel_display_power_domain > intel_display_port_power_domain(struct intel_encoder *intel_encoder); > +enum intel_display_power_domain > +intel_display_aux_power_domain(struct intel_encoder *intel_encoder); > void intel_mode_from_pipe_config(struct drm_display_mode *mode, > struct intel_crtc_config *pipe_config); > int intel_format_to_fourcc(int format);
On Tue, Sep 16, 2014 at 04:19:07PM +0300, Imre Deak wrote: > On Thu, 2014-09-04 at 12:27 +0100, Damien Lespiau wrote: > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > > index 93bd9bf..a983b40 100644 > > --- a/drivers/gpu/drm/i915/intel_dp.c > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > @@ -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. With the above change and > everywhere else below we'll end up enabling both power domains, though > we only need AUX functionality. > > 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. Also this changes shared code so should be split out from the stage 1 enabling. At least if we want to push it to 3.18 since drm-next already closed feature-wise for that kernel. -Daniel
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; 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. > 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. > 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? This patch still need the reworks you mentionned in the previous mail, of course.
On Tue, Sep 16, 2014 at 04:19:07PM +0300, Imre Deak wrote: > This patch needs to be rebased on the recent PPS changes at least > getting/putting the AUX power domain in pps_lock()/pps_unlock() too. > Also it should be squashed into 69/89. Some more comments below. Hum, do we really need to hold a reference to the AUX power in pps_lock(), it doesn't seem that the PPS hw should a specific dependency on the AUX power domain. We might as well just get the "port" power domain if that's enough.
On Fri, Nov 07, 2014 at 01:11:13PM +0000, Damien Lespiau wrote: > On Tue, Sep 16, 2014 at 04:19:07PM +0300, Imre Deak wrote: > > This patch needs to be rebased on the recent PPS changes at least > > getting/putting the AUX power domain in pps_lock()/pps_unlock() too. > > Also it should be squashed into 69/89. Some more comments below. > > Hum, do we really need to hold a reference to the AUX power in > pps_lock(), it doesn't seem that the PPS hw should a specific dependency > on the AUX power domain. We might as well just get the "port" power > domain if that's enough. pps_lock() doesn't *really* need any power domain references. The thing is that the VLV/CHV display power well hooks need to reset the pps_pipe assignment which means they should grab pps_mutex. However the vdd code can grab power domain references while already holding pps_mutex, which gets us into a neat locking inversion with the power domain mutex. The workaround I did was to grab the power domain references always around pps_mutex, so that we dodge the problem. A better solution would be to never grab power domain references while already holding pps_mutex, but I wasn't happy with how the code started to look when I tried that. But I would welcome any efforts to make that happen since the current trick is rather hackish. Also the code is now otherwise cleaner than what it was when I tried to do that, so maybe it's easier now.
On Fri, Nov 07, 2014 at 03:31:20PM +0200, Ville Syrjälä wrote: > On Fri, Nov 07, 2014 at 01:11:13PM +0000, Damien Lespiau wrote: > > On Tue, Sep 16, 2014 at 04:19:07PM +0300, Imre Deak wrote: > > > This patch needs to be rebased on the recent PPS changes at least > > > getting/putting the AUX power domain in pps_lock()/pps_unlock() too. > > > Also it should be squashed into 69/89. Some more comments below. > > > > Hum, do we really need to hold a reference to the AUX power in > > pps_lock(), it doesn't seem that the PPS hw should a specific dependency > > on the AUX power domain. We might as well just get the "port" power > > domain if that's enough. > > pps_lock() doesn't *really* need any power domain references. The thing > is that the VLV/CHV display power well hooks need to reset the pps_pipe > assignment which means they should grab pps_mutex. However the vdd code > can grab power domain references while already holding pps_mutex, which > gets us into a neat locking inversion with the power domain mutex. > > The workaround I did was to grab the power domain references always around > pps_mutex, so that we dodge the problem. A better solution would be to > never grab power domain references while already holding pps_mutex, but > I wasn't happy with how the code started to look when I tried that. But I > would welcome any efforts to make that happen since the current trick is > rather hackish. Also the code is now otherwise cleaner than what it was > when I tried to do that, so maybe it's easier now. Right, but then the remark was that we don't need the aux power domain to keep the power well on on VLV/CHV, so would you be happy to just take the port power domain around pps_lock/unlock().
On Fri, Nov 07, 2014 at 01:49:43PM +0000, Damien Lespiau wrote: > On Fri, Nov 07, 2014 at 03:31:20PM +0200, Ville Syrjälä wrote: > > On Fri, Nov 07, 2014 at 01:11:13PM +0000, Damien Lespiau wrote: > > > On Tue, Sep 16, 2014 at 04:19:07PM +0300, Imre Deak wrote: > > > > This patch needs to be rebased on the recent PPS changes at least > > > > getting/putting the AUX power domain in pps_lock()/pps_unlock() too. > > > > Also it should be squashed into 69/89. Some more comments below. > > > > > > Hum, do we really need to hold a reference to the AUX power in > > > pps_lock(), it doesn't seem that the PPS hw should a specific dependency > > > on the AUX power domain. We might as well just get the "port" power > > > domain if that's enough. > > > > pps_lock() doesn't *really* need any power domain references. The thing > > is that the VLV/CHV display power well hooks need to reset the pps_pipe > > assignment which means they should grab pps_mutex. However the vdd code > > can grab power domain references while already holding pps_mutex, which > > gets us into a neat locking inversion with the power domain mutex. > > > > The workaround I did was to grab the power domain references always around > > pps_mutex, so that we dodge the problem. A better solution would be to > > never grab power domain references while already holding pps_mutex, but > > I wasn't happy with how the code started to look when I tried that. But I > > would welcome any efforts to make that happen since the current trick is > > rather hackish. Also the code is now otherwise cleaner than what it was > > when I tried to do that, so maybe it's easier now. > > Right, but then the remark was that we don't need the aux power domain > to keep the power well on on VLV/CHV, so would you be happy to just take > the port power domain around pps_lock/unlock(). Yeah. Actually just the disp2/pipe-a well is needed, so even the port domains are pointless but I was feeling lazy when I came up with the hack. I (or someone) should also do some tests on vlv/chv to see which power wells are actually needed for AUX. disp2d/pipe-a obviously just for the registers, but then I'm not sure if the phy cmnlane well is needed. On chv the docs do say that the AUX stuff is in the common lane, but IIRC on vlv it was said to be more of a separate lane. Anyway would be nice to know for sure.
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.
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.
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
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've thought the point of case 1 is that we don't have to fire up the entire display clocks and power wells when just doing a few dp aux transactions to probe for outputs. -Daniel
On Tue, Nov 11, 2014 at 12:22:28PM +0000, Damien Lespiau wrote: > 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. This kind of high-freq flip-flop of power domains is why the linux power domain and runtime pm code has a disable timer. I guess as predicted, we'll eventually implement it all ;-) -Daniel
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 0a4dd00..abd4201 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -4503,6 +4503,27 @@ intel_display_port_power_domain(struct intel_encoder *intel_encoder) } } +enum intel_display_power_domain +intel_display_aux_power_domain(struct intel_encoder *intel_encoder) +{ + struct intel_digital_port *intel_dig_port; + + intel_dig_port = enc_to_dig_port(&intel_encoder->base); + switch (intel_dig_port->port) { + case PORT_A: + return POWER_DOMAIN_AUX_A; + case PORT_B: + return POWER_DOMAIN_AUX_B; + case PORT_C: + return POWER_DOMAIN_AUX_C; + case PORT_D: + return POWER_DOMAIN_AUX_D; + default: + WARN_ON_ONCE(1); + return 0; + } +} + static unsigned long get_crtc_power_domains(struct drm_crtc *crtc) { struct drm_device *dev = crtc->dev; diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 93bd9bf..a983b40 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -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); + DRM_DEBUG_KMS("Turning eDP VDD on\n"); if (!edp_have_panel_power(intel_dp)) @@ -1309,8 +1312,12 @@ static void edp_panel_vdd_off_sync(struct intel_dp *intel_dp) power_domain = intel_display_port_power_domain(intel_encoder); intel_display_power_put(dev_priv, power_domain); + + power_domain = intel_display_aux_power_domain(intel_encoder); + intel_display_power_put(dev_priv, power_domain); } + static void edp_panel_vdd_work(struct work_struct *__work) { struct intel_dp *intel_dp = container_of(to_delayed_work(__work), @@ -3836,7 +3843,13 @@ g4x_dp_detect(struct intel_dp *intel_dp) static struct edid * intel_dp_get_edid(struct intel_dp *intel_dp) { + struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); + struct intel_encoder *intel_encoder = &intel_dig_port->base; + struct drm_device *dev = intel_encoder->base.dev; + struct drm_i915_private *dev_priv = dev->dev_private; struct intel_connector *intel_connector = intel_dp->attached_connector; + enum intel_display_power_domain power_domain; + struct edid *edid; /* use cached edid if we have one */ if (intel_connector->edid) { @@ -3845,9 +3858,16 @@ intel_dp_get_edid(struct intel_dp *intel_dp) return NULL; return drm_edid_duplicate(intel_connector->edid); - } else - return drm_get_edid(&intel_connector->base, - &intel_dp->aux.ddc); + } else { + power_domain = intel_display_aux_power_domain(intel_encoder); + intel_display_power_get(dev_priv, power_domain); + + edid = drm_get_edid(&intel_connector->base, &intel_dp->aux.ddc); + + intel_display_power_put(dev_priv, power_domain); + + return edid; + } } static void @@ -3876,24 +3896,30 @@ intel_dp_unset_edid(struct intel_dp *intel_dp) intel_dp->has_audio = false; } -static enum intel_display_power_domain -intel_dp_power_get(struct intel_dp *dp) +static void intel_dp_power_get(struct intel_dp *dp) { struct intel_encoder *encoder = &dp_to_dig_port(dp)->base; + struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); enum intel_display_power_domain power_domain; power_domain = intel_display_port_power_domain(encoder); intel_display_power_get(to_i915(encoder->base.dev), power_domain); - return power_domain; + power_domain = intel_display_aux_power_domain(encoder); + intel_display_power_get(dev_priv, power_domain); } -static void -intel_dp_power_put(struct intel_dp *dp, - enum intel_display_power_domain power_domain) +static void intel_dp_power_put(struct intel_dp *dp) { struct intel_encoder *encoder = &dp_to_dig_port(dp)->base; - intel_display_power_put(to_i915(encoder->base.dev), power_domain); + struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); + enum intel_display_power_domain power_domain; + + power_domain = intel_display_port_power_domain(encoder); + intel_display_power_put(dev_priv, power_domain); + + power_domain = intel_display_aux_power_domain(encoder); + intel_display_power_put(dev_priv, power_domain); } static enum drm_connector_status @@ -3904,7 +3930,6 @@ intel_dp_detect(struct drm_connector *connector, bool force) struct intel_encoder *intel_encoder = &intel_dig_port->base; struct drm_device *dev = connector->dev; enum drm_connector_status status; - enum intel_display_power_domain power_domain; bool ret; DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", @@ -3918,7 +3943,7 @@ intel_dp_detect(struct drm_connector *connector, bool force) return connector_status_disconnected; } - power_domain = intel_dp_power_get(intel_dp); + intel_dp_power_get(intel_dp); /* Can't disconnect eDP, but you can close the lid... */ if (is_edp(intel_dp)) @@ -3949,7 +3974,7 @@ intel_dp_detect(struct drm_connector *connector, bool force) status = connector_status_connected; out: - intel_dp_power_put(intel_dp, power_domain); + intel_dp_power_put(intel_dp); return status; } @@ -3958,7 +3983,6 @@ intel_dp_force(struct drm_connector *connector) { struct intel_dp *intel_dp = intel_attached_dp(connector); struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base; - enum intel_display_power_domain power_domain; DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", connector->base.id, connector->name); @@ -3967,11 +3991,11 @@ intel_dp_force(struct drm_connector *connector) if (connector->status != connector_status_connected) return; - power_domain = intel_dp_power_get(intel_dp); + intel_dp_power_get(intel_dp); intel_dp_set_edid(intel_dp); - intel_dp_power_put(intel_dp, power_domain); + intel_dp_power_put(intel_dp); if (intel_encoder->type != INTEL_OUTPUT_EDP) intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT; @@ -4205,7 +4229,8 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd) port_name(intel_dig_port->port), long_hpd ? "long" : "short"); - power_domain = intel_display_port_power_domain(intel_encoder); + power_domain = intel_display_aux_power_domain(intel_encoder); + intel_display_power_get(dev_priv, power_domain); if (long_hpd) { @@ -4648,6 +4673,9 @@ void intel_edp_panel_vdd_sanitize(struct intel_encoder *intel_encoder) 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); + edp_panel_vdd_schedule_off(intel_dp); } diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 9558f07..a407d04 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -916,6 +916,8 @@ void hsw_disable_ips(struct intel_crtc *crtc); void intel_display_set_init_power(struct drm_i915_private *dev, bool enable); enum intel_display_power_domain intel_display_port_power_domain(struct intel_encoder *intel_encoder); +enum intel_display_power_domain +intel_display_aux_power_domain(struct intel_encoder *intel_encoder); void intel_mode_from_pipe_config(struct drm_display_mode *mode, struct intel_crtc_config *pipe_config); int intel_format_to_fourcc(int format);