[Intel-gfx] [PATCH 16/21] drm/i915: Markup paired operations on display power domains

[Chris Wilson]

Quoting Mika Kuoppala (2019-01-10 15:51:33)
> Chris Wilson <chris at chris-wilson.co.uk> writes:
> > @@ -680,6 +682,8 @@ static int i915_interrupt_info(struct seq_file *m, void *data)
> >       wakeref = intel_runtime_pm_get(dev_priv);
> >  
> >       if (IS_CHERRYVIEW(dev_priv)) {
> > +             intel_wakeref_t pref;
> > +
> 
> In here you are introducing a new, descriptive, power reference for display.
> But then you drop it after using it twice. And can't seem to find
> reason for inconsistency.

pref, pref, pref...

> >               seq_printf(m, "Master Interrupt Control:\t%08x\n",
> >                          I915_READ(GEN8_MASTER_IRQ));
> >  
> > @@ -695,8 +699,9 @@ static int i915_interrupt_info(struct seq_file *m, void *data)
> >                       enum intel_display_power_domain power_domain;
> >  
> >                       power_domain = POWER_DOMAIN_PIPE(pipe);
> > -                     if (!intel_display_power_get_if_enabled(dev_priv,
> > -                                                             power_domain)) {
> > +                     pref = intel_display_power_get_if_enabled(dev_priv,
> > +                                                               power_domain);
> > +                     if (!pref) {

ah, it would have been chosen to fit 80cols.

> >                               seq_printf(m, "Pipe %c power disabled\n",
> >                                          pipe_name(pipe));
> >                               continue;
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index f0a405604b75..44c1b21febba 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -344,6 +344,7 @@ struct intel_csr {
> >       uint32_t mmiodata[8];
> >       uint32_t dc_state;
> >       uint32_t allowed_dc_mask;
> > +     intel_wakeref_t wakeref;
> >  };
> >  
> >  enum i915_cache_level {
> > @@ -1982,6 +1983,7 @@ struct drm_i915_private {
> >                * is a slight delay before we do so.
> >                */
> >               intel_wakeref_t awake;
> > +             intel_wakeref_t power;
> 
> This prolly explains the prefs above :)

Possibly.

> > -     intel_display_power_put(dev_priv, POWER_DOMAIN_PORT_DDI_A_IO);
> > -
> > -     if (intel_dsi->dual_link)
> > -             intel_display_power_put(dev_priv, POWER_DOMAIN_PORT_DDI_B_IO);
> > +     for_each_dsi_port(port, intel_dsi->ports) {
> > +             intel_wakeref_t wakeref;
> > +
> > +             wakeref = fetch_and_zero(&intel_dsi->io_wakeref[port]);
> > +             if (wakeref) {
> > +                     intel_display_power_put(dev_priv,
> > +                                             port == PORT_A ?
> > +                                             POWER_DOMAIN_PORT_DDI_A_IO :
> > +                                             POWER_DOMAIN_PORT_DDI_B_IO,
> > +                                             wakeref);
> > +             }
> > +     }
> 
> Ok, well. I have been trying to figure out what really is going on here.
> 
> First it seems that you fix a bug. We take refs for each dsi port but
> only release once special casing 'dual_link' without this patch.
> 
> Second, all the hw access is before getting the refs, looking
> suspicious.

There's always been two, just this moves into a for(;;). I don't think
it was buggy, but I do think the for(;;) has the advantage of being
clearer that it operates over all the ports and wakerefs.

> > @@ -15808,12 +15827,13 @@ intel_modeset_setup_hw_state(struct drm_device *dev,
> >                            struct drm_modeset_acquire_ctx *ctx)
> >  {
> >       struct drm_i915_private *dev_priv = to_i915(dev);
> > -     struct intel_crtc *crtc;
> >       struct intel_crtc_state *crtc_state;
> >       struct intel_encoder *encoder;
> > +     struct intel_crtc *crtc;
> 
> Not that I mind but I don't understand the reasoning
> behind the change of order on this and on few other places in this
> patch.

Just quietly moving everyone over to a tidy set of variable definitions
(we are not meant to grow Christmas trees as part of the kernel coding
style).

> > -/**
> > - * intel_display_power_put - release a power domain reference
> > - * @dev_priv: i915 device instance
> > - * @domain: power domain to reference
> > - *
> > - * This function drops the power domain reference obtained by
> > - * intel_display_power_get() and might power down the corresponding hardware
> > - * block right away if this is the last reference.
> > - */
> > -void intel_display_power_put(struct drm_i915_private *dev_priv,
> > -                          enum intel_display_power_domain domain)
> > +static void __intel_display_power_put(struct drm_i915_private *dev_priv,
> > +                                   enum intel_display_power_domain domain)
> >  {
> >       struct i915_power_domains *power_domains;
> >       struct i915_power_well *power_well;
> > @@ -1947,10 +1944,34 @@ void intel_display_power_put(struct drm_i915_private *dev_priv,
> >               intel_power_well_put(dev_priv, power_well);
> >  
> >       mutex_unlock(&power_domains->lock);
> > +}
> >  
> > +/**
> > + * intel_display_power_put - release a power domain reference
> 
> +unchecked? or is this in wrong place.

unchecked doesn't exist, you just need a stronger pair of glasses.

So the only API documented interface is the full version that requires
you take ownership of your wakeref, we don't tempt you with the easy
version that is meant to only exist for transitioning
-Chris