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

[Mika Kuoppala <mika.kuoppala@linux.intel.com>]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> Quoting Mika Kuoppala (2019-01-10 15:51:33)
>> Chris Wilson <chris@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.

You should have adopted it then fully! :)
>
>> >                               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.

Agreed that your patch makes it more clear.

I am still suspicious about the ordering, hopefully there is
upper lever pdomain to cover access. But that is not
problem for this patch.

Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>

>
>> > @@ -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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx