Re: [RFT][PATCH v2 1/2] PM / domains: Rework governor code to be more consistent

[Ulf Hansson]

[...]

>>>  static int dev_update_qos_constraint(struct device *dev, void *data)
>>>  {
>>>         s64 *constraint_ns_p = data;
>>> -       s32 constraint_ns = -1;
>>> -
>>> -       if (dev->power.subsys_data && dev->power.subsys_data->domain_data)
>>> -               constraint_ns = dev_gpd_data(dev)->td.effective_constraint_ns;
>>> +       s64 constraint_ns;
>>>
>>> -       if (constraint_ns < 0) {
>>> -               constraint_ns = dev_pm_qos_read_value(dev);
>>> -               constraint_ns *= NSEC_PER_USEC;
>>> -       }
>>> -       if (constraint_ns == 0)
>>> +       if (!dev->power.subsys_data || !dev->power.subsys_data->domain_data)
>>>                 return 0;
>>>
>>>         /*
>>> -        * constraint_ns cannot be negative here, because the device has been
>>> -        * suspended.
>>> +        * Only take suspend-time QoS constraints of devices into account,
>>> +        * because constraints updated after the device has been suspended are
>>> +        * not guaranteed to be taken into account anyway.  In order for them
>>> +        * to take effect, the device has to be resumed and suspended again.
>>>          */
>>
>> This means a change in behavior, because earlier we took into account
>> QoS values for child devices that were not attached to a genpd.
>
> OK
>
> I have overlooked it or rather have forgotten about that.
>
>> Is there any reason you think we should change this, or is it just
>> something you overlooked here?
>>
>> I understand, that if the QoS constraint has been updated after such
>> child device has been suspended, it's tricky to take a correct
>> decision.
>
> Right, but if they are not in a domain, the best we can do is to look
> at the current value.
>
>> To really solve this, we would either have to register a QoS notifier
>> for all children devices that has its parent attached to a genpd, or
>> always runtime resume devices before updating QoS constraints.
>>
>> Non of these options is perfect, so perhaps we should consider a "best
>> effort" approach instead? Whatever that may be.
>
> I think best effort makes most sense.

Okay!

>
> So I guess I'll simply evaluate dev_pm_qos_read_value(dev) if
> subsys_data or subsys_data->domain_data is not there.

Yes.

However, if it returns -1, what value should you pick? 0?

>
> Of course, that doesn't apply to the code in __default_power_down_ok()
> as that only takes device in the domain into account anyway.

Yep, agree!

Kind regards
Uffe