Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> writes:
> Quoting Mika Kuoppala (2019-01-10 15:51:33)
>> Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx>
>
>> > @@ -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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
