On Fri, Apr 19, 2013 at 12:28:30PM -0300, Paulo Zanoni wrote:
> 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.
Yeah, I guess that makes sense.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
