[Intel-gfx] [PATCH 03/15] drm/i915: add intel_power_well_is_down

[Paulo Zanoni]

Hi

2013/3/15 Ben Widawsky <ben at bwidawsk.net>:
> On Thu, Mar 07, 2013 at 12:26:23AM +0100, Daniel Vetter wrote:
>> On Wed, Mar 06, 2013 at 08:03:10PM -0300, Paulo Zanoni wrote:
>> > From: Paulo Zanoni <paulo.r.zanoni at intel.com>
>> >
>> > It returns true if we're not supposed to touch the registers on the
>> > power down well.
>> >
>> > For now there's just one caller, but I'm going to add more.
>> >
>> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni at intel.com>
>> > ---
>> >  drivers/gpu/drm/i915/intel_display.c |    4 ++--
>> >  drivers/gpu/drm/i915/intel_drv.h     |    1 +
>> >  drivers/gpu/drm/i915/intel_pm.c      |   16 ++++++++++++++++
>> >  3 files changed, 19 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> > index 502cb28..bd27336 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 (IS_HASWELL(dev_priv->dev) && cpu_transcoder != TRANSCODER_EDP &&
>> > -       !(I915_READ(HSW_PWR_WELL_DRIVER) & HSW_PWR_WELL_ENABLE)) {
>> > +   if (intel_power_well_is_down(dev_priv->dev) &&
>>
>> The name here feels a bit too generic given that we already have on hsw
>> different display c states with different requirements and different
>> pieces of hw not working.
>>
>> Can't thinkg of anything better than intel_display_power_well_is_down
>> though ...
>> -Daniel
>>
>> > +       cpu_transcoder != TRANSCODER_EDP) {
>> >             cur_state = false;
>> >     } else {
>> >             reg = PIPECONF(cpu_transcoder);
>> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> > index 010e998..28c4789 100644
>> > --- a/drivers/gpu/drm/i915/intel_drv.h
>> > +++ b/drivers/gpu/drm/i915/intel_drv.h
>> > @@ -671,6 +671,7 @@ 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_power_well_is_down(struct drm_device *dev);
>> >  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 5479363..90562bc 100644
>> > --- a/drivers/gpu/drm/i915/intel_pm.c
>> > +++ b/drivers/gpu/drm/i915/intel_pm.c
>> > @@ -4070,6 +4070,22 @@ void intel_init_clock_gating(struct drm_device *dev)
>> >     dev_priv->display.init_clock_gating(dev);
>> >  }
>> >
>> > +/**
>> > + * Returns true if we're not supposed to touch the registers on the power down
>> > + * well. Notice that we don't check whether the power well is actually off, we
>> > + * just check if our driver told the hardware that it doesn't need the power
>> > + * well enabled.
>> > + */
>
> I agree with Denial that the name sucks because your comment clearly
> contradicts what the function is actually called. Can't think of
> anything better either.

intel_using_power_well?

>
> In the bikeshed realm, I think it makes more sense to do the IS_HASWELL
> check in your pipe assertion, but I'll assume that you have a good usage
> as you mention later.

I want to make all the power well functions work correctly on Gens
that don't have a power well. We already introduced bugs related to
this in the past, so let's be defensive.

>
> Reviewed-by: Ben Widawsky <ben at bwidawsk.net>
>
>> > +bool intel_power_well_is_down(struct drm_device *dev)
>> > +{
>> > +   struct drm_i915_private *dev_priv = dev->dev_private;
>> > +
>> > +   if (IS_HASWELL(dev))
>> > +           return !(I915_READ(HSW_PWR_WELL_DRIVER) & HSW_PWR_WELL_ENABLE);
>> > +   else
>> > +           return false;
>> > +}
>> > +
>> >  void intel_set_power_well(struct drm_device *dev, bool enable)
>> >  {
>> >     struct drm_i915_private *dev_priv = dev->dev_private;
>> > --
>> > 1.7.10.4
>> >
>> > _______________________________________________
>> > 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
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Ben Widawsky, Intel Open Source Technology Center



-- 
Paulo Zanoni