| Message ID | 1398347368-2459-1-git-send-email-przanoni@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Apr 24, 2014 at 10:49:28AM -0300, Paulo Zanoni wrote: > From: Paulo Zanoni <paulo.r.zanoni@intel.com> > > This was suggested by Chris on his review to the first version of > "drm/i915: get power domain in case the BIOS enabled eDP VDD". Well, > at least that's what I understood from his comment :) > > v2: - Make it intel_encoder_power_get/put > - Don't call the new functions on functions where we need both > get() and put(), so we don't end up calling > intel_display_port_power_domain() more than needed. > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > --- > drivers/gpu/drm/i915/intel_dp.c | 19 +++++-------------- > drivers/gpu/drm/i915/intel_drv.h | 2 ++ > drivers/gpu/drm/i915/intel_pm.c | 24 ++++++++++++++++++++++++ > 3 files changed, 31 insertions(+), 14 deletions(-) > > > I am not really sure if this patch is worth it... I certainly won't be mad if we > drop it. The diffstat is not good. I'm not sure either. In fact I had this same idea already when I saw Imre's power well patches for the first time, but then I thought that it doesn't help that much and decided to keep the idea to myself. The patch itself looks correct however, and it doesn't make the code any worse IMHO so: Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index a421c81..8201b60 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -1069,7 +1069,6 @@ static bool _edp_panel_vdd_on(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_i915_private *dev_priv = dev->dev_private; > - enum intel_display_power_domain power_domain; > u32 pp; > u32 pp_stat_reg, pp_ctrl_reg; > bool need_to_disable = !intel_dp->want_panel_vdd; > @@ -1082,8 +1081,7 @@ static bool _edp_panel_vdd_on(struct intel_dp *intel_dp) > if (edp_have_panel_vdd(intel_dp)) > return need_to_disable; > > - power_domain = intel_display_port_power_domain(intel_encoder); > - intel_display_power_get(dev_priv, power_domain); > + intel_encoder_power_get(intel_encoder); > > DRM_DEBUG_KMS("Turning eDP VDD on\n"); > > @@ -1133,7 +1131,6 @@ static void edp_panel_vdd_off_sync(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; > - enum intel_display_power_domain power_domain; > > DRM_DEBUG_KMS("Turning eDP VDD off\n"); > > @@ -1153,8 +1150,7 @@ static void edp_panel_vdd_off_sync(struct intel_dp *intel_dp) > if ((pp & POWER_TARGET_ON) == 0) > intel_dp->last_power_cycle = jiffies; > > - power_domain = intel_display_port_power_domain(intel_encoder); > - intel_display_power_put(dev_priv, power_domain); > + intel_encoder_power_put(intel_encoder); > } > } > > @@ -1242,7 +1238,6 @@ void intel_edp_panel_off(struct intel_dp *intel_dp) > struct intel_encoder *intel_encoder = &intel_dig_port->base; > struct drm_device *dev = intel_dp_to_dev(intel_dp); > struct drm_i915_private *dev_priv = dev->dev_private; > - enum intel_display_power_domain power_domain; > u32 pp; > u32 pp_ctrl_reg; > > @@ -1272,8 +1267,7 @@ void intel_edp_panel_off(struct intel_dp *intel_dp) > wait_panel_off(intel_dp); > > /* We got a reference when we enabled the VDD. */ > - power_domain = intel_display_port_power_domain(intel_encoder); > - intel_display_power_put(dev_priv, power_domain); > + intel_encoder_power_put(intel_encoder); > } > > void intel_edp_backlight_on(struct intel_dp *intel_dp) > @@ -3798,11 +3792,8 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp, > > /* The VDD bit needs a power domain reference, so if the bit is already > * enabled when we boot, grab this reference. */ > - if (edp_have_panel_vdd(intel_dp)) { > - enum intel_display_power_domain power_domain; > - power_domain = intel_display_port_power_domain(intel_encoder); > - intel_display_power_get(dev_priv, power_domain); > - } > + if (edp_have_panel_vdd(intel_dp)) > + intel_encoder_power_get(intel_encoder); > > /* Cache DPCD and EDID for edp. */ > intel_edp_panel_vdd_on(intel_dp); > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index b885df1..5f27b51 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -920,6 +920,8 @@ void intel_display_power_get(struct drm_i915_private *dev_priv, > enum intel_display_power_domain domain); > void intel_display_power_put(struct drm_i915_private *dev_priv, > enum intel_display_power_domain domain); > +void intel_encoder_power_get(struct intel_encoder *encoder); > +void intel_encoder_power_put(struct intel_encoder *encoder); > void intel_power_domains_init_hw(struct drm_i915_private *dev_priv); > void intel_init_gt_powersave(struct drm_device *dev); > void intel_cleanup_gt_powersave(struct drm_device *dev); > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index 75c1c76..8ed8d53 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -5734,6 +5734,30 @@ void intel_display_power_put(struct drm_i915_private *dev_priv, > intel_runtime_pm_put(dev_priv); > } > > +/* > + * Short version for when you don't need to use both get() and put() on the same > + * function. If you need both, consider calling intel_display_power_get/put() > + * directly so you will save a call to intel_display_port_power_domain(). > + */ > +void intel_encoder_power_get(struct intel_encoder *encoder) > +{ > + struct drm_i915_private *dev_priv = encoder->base.dev->dev_private; > + enum intel_display_power_domain power_domain; > + > + power_domain = intel_display_port_power_domain(encoder); > + intel_display_power_get(dev_priv, power_domain); > +} > + > +/* See the comment above intel_encoder_power_get(). */ > +void intel_encoder_power_put(struct intel_encoder *encoder) > +{ > + struct drm_i915_private *dev_priv = encoder->base.dev->dev_private; > + enum intel_display_power_domain power_domain; > + > + power_domain = intel_display_port_power_domain(encoder); > + intel_display_power_put(dev_priv, power_domain); > +} > + > static struct i915_power_domains *hsw_pwr; > > /* Display audio driver power well request */ > -- > 1.9.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thu, Apr 24, 2014 at 05:11:10PM +0300, Ville Syrjälä wrote: > On Thu, Apr 24, 2014 at 10:49:28AM -0300, Paulo Zanoni wrote: > > From: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > > This was suggested by Chris on his review to the first version of > > "drm/i915: get power domain in case the BIOS enabled eDP VDD". Well, > > at least that's what I understood from his comment :) > > > > v2: - Make it intel_encoder_power_get/put > > - Don't call the new functions on functions where we need both > > get() and put(), so we don't end up calling > > intel_display_port_power_domain() more than needed. > > > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > --- > > drivers/gpu/drm/i915/intel_dp.c | 19 +++++-------------- > > drivers/gpu/drm/i915/intel_drv.h | 2 ++ > > drivers/gpu/drm/i915/intel_pm.c | 24 ++++++++++++++++++++++++ > > 3 files changed, 31 insertions(+), 14 deletions(-) > > > > > > I am not really sure if this patch is worth it... I certainly won't be mad if we > > drop it. The diffstat is not good. > > I'm not sure either. In fact I had this same idea already when I saw > Imre's power well patches for the first time, but then I thought that > it doesn't help that much and decided to keep the idea to myself. If you guys ask about my color choices I have to admit that I'm not a huge fan of intel_display_port_power_domain. In roughly object-oriented code functions with giant switch statements which use the object class to decide what to do tend to be a red flag ... In case you care for a reference see the switch statement code smell in Martin Fowler's refactoring. But that was just my bikeshed, tbh I don't care strongly enough here. -Daniel
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index a421c81..8201b60 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -1069,7 +1069,6 @@ static bool _edp_panel_vdd_on(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_i915_private *dev_priv = dev->dev_private; - enum intel_display_power_domain power_domain; u32 pp; u32 pp_stat_reg, pp_ctrl_reg; bool need_to_disable = !intel_dp->want_panel_vdd; @@ -1082,8 +1081,7 @@ static bool _edp_panel_vdd_on(struct intel_dp *intel_dp) if (edp_have_panel_vdd(intel_dp)) return need_to_disable; - power_domain = intel_display_port_power_domain(intel_encoder); - intel_display_power_get(dev_priv, power_domain); + intel_encoder_power_get(intel_encoder); DRM_DEBUG_KMS("Turning eDP VDD on\n"); @@ -1133,7 +1131,6 @@ static void edp_panel_vdd_off_sync(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; - enum intel_display_power_domain power_domain; DRM_DEBUG_KMS("Turning eDP VDD off\n"); @@ -1153,8 +1150,7 @@ static void edp_panel_vdd_off_sync(struct intel_dp *intel_dp) if ((pp & POWER_TARGET_ON) == 0) intel_dp->last_power_cycle = jiffies; - power_domain = intel_display_port_power_domain(intel_encoder); - intel_display_power_put(dev_priv, power_domain); + intel_encoder_power_put(intel_encoder); } } @@ -1242,7 +1238,6 @@ void intel_edp_panel_off(struct intel_dp *intel_dp) struct intel_encoder *intel_encoder = &intel_dig_port->base; struct drm_device *dev = intel_dp_to_dev(intel_dp); struct drm_i915_private *dev_priv = dev->dev_private; - enum intel_display_power_domain power_domain; u32 pp; u32 pp_ctrl_reg; @@ -1272,8 +1267,7 @@ void intel_edp_panel_off(struct intel_dp *intel_dp) wait_panel_off(intel_dp); /* We got a reference when we enabled the VDD. */ - power_domain = intel_display_port_power_domain(intel_encoder); - intel_display_power_put(dev_priv, power_domain); + intel_encoder_power_put(intel_encoder); } void intel_edp_backlight_on(struct intel_dp *intel_dp) @@ -3798,11 +3792,8 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp, /* The VDD bit needs a power domain reference, so if the bit is already * enabled when we boot, grab this reference. */ - if (edp_have_panel_vdd(intel_dp)) { - enum intel_display_power_domain power_domain; - power_domain = intel_display_port_power_domain(intel_encoder); - intel_display_power_get(dev_priv, power_domain); - } + if (edp_have_panel_vdd(intel_dp)) + intel_encoder_power_get(intel_encoder); /* Cache DPCD and EDID for edp. */ intel_edp_panel_vdd_on(intel_dp); diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index b885df1..5f27b51 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -920,6 +920,8 @@ void intel_display_power_get(struct drm_i915_private *dev_priv, enum intel_display_power_domain domain); void intel_display_power_put(struct drm_i915_private *dev_priv, enum intel_display_power_domain domain); +void intel_encoder_power_get(struct intel_encoder *encoder); +void intel_encoder_power_put(struct intel_encoder *encoder); void intel_power_domains_init_hw(struct drm_i915_private *dev_priv); void intel_init_gt_powersave(struct drm_device *dev); void intel_cleanup_gt_powersave(struct drm_device *dev); diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 75c1c76..8ed8d53 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -5734,6 +5734,30 @@ void intel_display_power_put(struct drm_i915_private *dev_priv, intel_runtime_pm_put(dev_priv); } +/* + * Short version for when you don't need to use both get() and put() on the same + * function. If you need both, consider calling intel_display_power_get/put() + * directly so you will save a call to intel_display_port_power_domain(). + */ +void intel_encoder_power_get(struct intel_encoder *encoder) +{ + struct drm_i915_private *dev_priv = encoder->base.dev->dev_private; + enum intel_display_power_domain power_domain; + + power_domain = intel_display_port_power_domain(encoder); + intel_display_power_get(dev_priv, power_domain); +} + +/* See the comment above intel_encoder_power_get(). */ +void intel_encoder_power_put(struct intel_encoder *encoder) +{ + struct drm_i915_private *dev_priv = encoder->base.dev->dev_private; + enum intel_display_power_domain power_domain; + + power_domain = intel_display_port_power_domain(encoder); + intel_display_power_put(dev_priv, power_domain); +} + static struct i915_power_domains *hsw_pwr; /* Display audio driver power well request */