[PATCH 3/7] drm/i915: add intel_display_power_enabled

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi

2013/4/19 Daniel Vetter <daniel at ffwll.ch>:
> On Fri, Apr 19, 2013 at 06:44:47AM +0100, Damien Lespiau wrote:
>> On Thu, Apr 18, 2013 at 04:35:42PM -0300, Paulo Zanoni wrote:
>> > From: Paulo Zanoni <paulo.r.zanoni at intel.com>
>> >
>> > This should replace intel_using_power_well. The idea is that we're
>> > adding the requested power domain as an argument, so this might enable
>> > the code to look less platform-specific and also allows us to easily
>> > add new domains in case we need.
>> >
>> > Requested-by: Daniel Vetter <daniel.vetter at ffwll.ch>
>> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni at intel.com>
>> > ---
>> >  drivers/gpu/drm/i915/intel_display.c |   16 +++++++++++-----
>> >  drivers/gpu/drm/i915/intel_drv.h     |   10 +++++++++-
>> >  drivers/gpu/drm/i915/intel_pm.c      |   23 ++++++++++++++++++-----
>> >  3 files changed, 38 insertions(+), 11 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> > index b86023d..b492eaa 100644
>> > --- a/drivers/gpu/drm/i915/intel_display.c
>> > +++ b/drivers/gpu/drm/i915/intel_display.c
>> > @@ -1227,8 +1227,8 @@ void assert_pipe(struct drm_i915_private *dev_priv,
>> >     if (pipe == PIPE_A && dev_priv->quirks & QUIRK_PIPEA_FORCE)
>> >             state = true;
>> >
>> > -   if (!intel_using_power_well(dev_priv->dev) &&
>> > -       cpu_transcoder != TRANSCODER_EDP) {
>> > +   if (cpu_transcoder != TRANSCODER_EDP &&
>> > +       !intel_display_power_enabled(dev_priv->dev, POWER_DOMAIN_OTHER)) {
>>
>> I'm wondering if we could simplify the look of this and try to avoid
>> using POWER_DOMAIN_OTHER, for instance having a:
>>
>> if (!intel_display_power_enabled(dev, POWER_DOMAIN_TRANSCODER(cpu_transcoder)))
>>       return false;
>>
>> playing a bit with the POWER_DOMAIN_TRANSCODER_* enums to encode the
>> the cpu_transcoder in them.
>
> Well I have my own bikeshed - imo POWER_DOMAIN_TRANSCODER_EDP should be
> called just POWER_DOMAIN_EDP since it also includes the eDP port. So the
> transcoder part in the name is a bit misleading. And I suspect that we
> still miss some DDI A vs everything else checks in the hw state readout
> code ...

I think POWER_DOMAIN_EDP can be misleading. We can have "PIPE_B +
TRANSCODER_EDP" configured (who can guarantee what the xrandr apps
really do?), and we can also have the "eDP on port D" which will never
use TRANSCODER_EDP, it will use PIPE_X + CPU_TRANSCODER_X.

>
> My fear with using more fine-grained domains than what we currently (and
> for the next few generations of hw, hopefully) need is that we spill check
> complexity into code where it's simply not relevant. So I think that the
> code clarity we gain by pushing the "is this thing in one of the special
> domains" into intel_display_power_enabled we'll waste again with
> inconsistency in checking - it's not relevant for most things after all.
> -Daniel
>
>>
>> >             cur_state = false;
>> >     } else {
>> >             reg = PIPECONF(cpu_transcoder);
>> > @@ -3584,6 +3584,7 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
>> >     int pipe = intel_crtc->pipe;
>> >     int plane = intel_crtc->plane;
>> >     enum transcoder cpu_transcoder = intel_crtc->config.cpu_transcoder;
>> > +   enum intel_display_power_domain domain;
>> >
>> >     if (!intel_crtc->active)
>> >             return;
>> > @@ -3607,7 +3608,12 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
>> >     /* XXX: Once we have proper panel fitter state tracking implemented with
>> >      * hardware state read/check support we should switch to only disable
>> >      * the panel fitter when we know it's used. */
>> > -   if (intel_using_power_well(dev)) {
>> > +   if (pipe == PIPE_A)
>> > +           domain = POWER_DOMAIN_PIPE_A_PANEL_FITTER;
>> > +   else
>> > +           domain = POWER_DOMAIN_OTHER;
>> > +
>>
>> Of course, if the above is found useful, same thing for here for the
>> panel fitter function.
>>
>> > +   if (intel_display_power_enabled(dev, domain)) {
>> >             I915_WRITE(PF_CTL(pipe), 0);
>> >             I915_WRITE(PF_WIN_SZ(pipe), 0);
>> >     }
>> > @@ -5859,8 +5865,8 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
>> >     enum transcoder cpu_transcoder = crtc->config.cpu_transcoder;
>> >     uint32_t tmp;
>> >
>>
>> [...]
>>
>> > +enum intel_display_power_domain {
>> > +   POWER_DOMAIN_PIPE_A,
>> > +   POWER_DOMAIN_PIPE_A_PANEL_FITTER,
>> > +   POWER_DOMAIN_TRANSCODER_EDP,
>> > +   POWER_DOMAIN_OTHER,
>> > +};
>> > +
>> >  int intel_pch_rawclk(struct drm_device *dev);
>> >
>> >  int intel_connector_update_modes(struct drm_connector *connector,
>> > @@ -700,7 +707,8 @@ extern void intel_update_fbc(struct drm_device *dev);
>> >  extern void intel_gpu_ips_init(struct drm_i915_private *dev_priv);
>> >  extern void intel_gpu_ips_teardown(void);
>> >
>> > -extern bool intel_using_power_well(struct drm_device *dev);
>> > +extern bool intel_display_power_enabled(struct drm_device *dev,
>> > +                                   enum intel_display_power_domain domain);
>> >  extern void intel_init_power_well(struct drm_device *dev);
>> >  extern void intel_set_power_well(struct drm_device *dev, bool enable);
>> >  extern void intel_enable_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 2557926..1c472d7 100644
>> > --- a/drivers/gpu/drm/i915/intel_pm.c
>> > +++ b/drivers/gpu/drm/i915/intel_pm.c
>> > @@ -4287,15 +4287,28 @@ void intel_init_clock_gating(struct drm_device *dev)
>> >   * enable it, so check if it's enabled and also check if we've requested it to
>> >   * be enabled.
>> >   */
>> > -bool intel_using_power_well(struct drm_device *dev)
>> > +bool intel_display_power_enabled(struct drm_device *dev,
>> > +                            enum intel_display_power_domain domain)
>> >  {
>> >     struct drm_i915_private *dev_priv = dev->dev_private;
>> > +   bool power_well_enabled;
>> >
>> > -   if (IS_HASWELL(dev))
>> > -           return I915_READ(HSW_PWR_WELL_DRIVER) ==
>> > -                  (HSW_PWR_WELL_ENABLE | HSW_PWR_WELL_STATE);
>> > -   else
>> > +   if (!HAS_POWER_WELL(dev))
>> > +           return true;
>> > +
>> > +   power_well_enabled = I915_READ(HSW_PWR_WELL_DRIVER) ==
>> > +                        (HSW_PWR_WELL_ENABLE | HSW_PWR_WELL_STATE);
>> > +
>> > +   switch (domain) {
>> > +   case POWER_DOMAIN_PIPE_A:
>> > +   case POWER_DOMAIN_TRANSCODER_EDP:
>> >             return true;
>> > +   case POWER_DOMAIN_PIPE_A_PANEL_FITTER:
>> > +   case POWER_DOMAIN_OTHER:
>> > +           return power_well_enabled;
>> > +   default:
>> > +           BUG();
>> > +   }
>> >  }
>>
>> --
>> Damien
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch



--
Paulo Zanoni


[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux