Re: [External] Re: [PATCH v4 3/3] platform/x86: thinkpad_acpi: Add platform profile support

[Mark Pearson <markpearson@lenovo.com>]

Hi Hans,

Sorry for the slow reply on this one - I went and did some 
investigation/testing first (and the US came back from Thanksgiving with 
a vengence of meetings....)

On 28/11/2020 09:55, Hans de Goede wrote:
> Hi,
> 
> On 11/26/20 5:51 PM, Mark Pearson wrote:
<snip>
>>
>>    drivers/platform/x86/thinkpad_acpi.c | 306 ++++++++++++++++++++++++++-
>>   1 file changed, 305 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
>> index 6a4c54db38fb..8463170391f5 100644
>> --- a/drivers/platform/x86/thinkpad_acpi.c
>> +++ b/drivers/platform/x86/thinkpad_acpi.c
>> @@ -72,6 +72,7 @@
>>   #include <linux/uaccess.h>
>>   #include <acpi/battery.h>
>>   #include <acpi/video.h>
>> +#include <linux/platform_profile.h>
> 
> Please group this together with the other linux/foo.h includes.
Ack.

Is it OK if I tidy up the list to be alphabetical as seems generally 
preferred, or would you rather I didn't mess with it apart from the one 
small adjustment?
> 
>>   
<snip>
>> +}
>> +
>> +static int dytc_perfmode_get(int *perfmode, int *funcmode)
>> +{
>> +	int output, err, cmd_err;
>> +
>> +	if (!dytc_profile_available)
>> +		return -ENODEV;
>> +
>> +	err = dytc_command(DYTC_CMD_GET, &output);
>> +	if (err)
>> +		return err;
>> +
>> +	*funcmode = (output >> DYTC_GET_FUNCTION_BIT) & 0xF;
>> +	if (*funcmode == DYTC_FUNCTION_CQL) {
>> +		int dummy;
>> +		/*
>> +		 * We can't get the mode when in CQL mode - so we disable CQL
>> +		 * mode retrieve the mode and then enable it again.
>> +		 * As disabling/enabling CQL triggers an event we set a flag to
>> +		 * ignore these events. This will be cleared by the event handler
>> +		 */
>> +		dytc_ignore_next_event = true;
>> +		err = dytc_command(DYTC_DISABLE_CQL, &dummy);
>> +		if (err)
>> +			return err;
>> +
>> +		cmd_err = dytc_command(DYTC_CMD_GET, &output);
>> +		/* Check return condition after we've restored CQL state */
>> +
>> +		/* Again ignore this event */
>> +		dytc_ignore_next_event = true;
> 
> Are we sure the event-handler will have run before we do this second
> setting of the ignore_next_event bool? Maybe make it an atomic integer
> and increment / decrement the variable ?
> 
> E.g.:
> 
> Declaration:
> 
> static atomic_t dytc_ignore_next_event = ATOMIC_INIT();
> 
> Ignore next event:
> 		atomic_inc(&dytc_ignore_next_event);
> 		
> Check if event should be ignored:
> 
> 		if (!atomic_add_unless(&dytc_ignore_next_event, -1, 0))
> 			dytc_profile_refresh();
> 
> Note atomic_add_unless may needs some explanation, it adds -1 unless
> the atomic_t already contains 0. And it returns true if the addition
> was done. so if it returns true then dytc_ignore_next_event was not 0
> (it might be zero afterwards).
> 
> IOW if atomic_add_unless returns true then dytc_ignore_next_event was true,
> so then we should NOT continue with the refresh.
> 
In my testing the event handler always ran first, but the atomic 
approach is much nicer - thank you for the suggestion.
I've played a bit with this and tried a few things over the last few 
days and it has been working nicely
One thing I noticed is I think I need to add a mutex to protect so that 
a FN key press won't interfere with a user space access and vice-versa.

> 
> 
> 
>> +		err = dytc_command(DYTC_ENABLE_CQL, &dummy);
>> +		if (err)
>> +			return err;
>> +		if (cmd_err)
>> +			return cmd_err;
>> +	}
>> +	*perfmode = (output >> DYTC_GET_MODE_BIT) & 0xF;
>> +	return 0;
>> +}
>> +
>> +/*
>> + * dytc_profile_get: Function to register with platform_profile
>> + * handler. Returns current platform profile.
>> + */
>> +int dytc_profile_get(enum platform_profile_option *profile)
>> +{
>> +	int funcmode, perfmode;
>> +	int err;
>> +
>> +	err = dytc_perfmode_get(&perfmode, &funcmode);
>> +	if (err)
>> +		return err;
> 
> Can't we used a cached value here ? I presume we get an
> event when this is changed by the hotkey ? Esp. with the
> whole enable/disable CQL dance getting the value seems a
> bit expensive, so using a cached value might be better?

Agreed - I'll implement.
> 
>> +
>> +	/* Convert Lenovo DYTC profile to platform_profile */
>> +	err = convert_dytc_to_profile(perfmode, profile);
>> +	if (err)
>> +		return err;
>> +
>> +	dytc_current_profile = *profile;
>> +	return 0;
>> +}
>> +
>> +/*
>> + * dytc_profile_set: Function to register with platform_profile
>> + * handler. Sets current platform profile.
>> + */
>> +int dytc_profile_set(enum platform_profile_option profile)
>> +{
>> +	int cur_perfmode, cur_funcmode;
>> +	int output;
>> +	int err;
>> +
>> +	if (!dytc_profile_available)
>> +		return -ENODEV;
>> +
>> +	if (profile == platform_profile_balance) {
>> +		/* To get back to balance mode we just issue a reset command */
>> +		err = dytc_command(DYTC_CMD_RESET, &output);
>> +		if (err)
>> +			return err;
>> +	} else {
>> +		int perfmode;
>> +		int cmd_err;
>> +
>> +		err = convert_profile_to_dytc(profile, &perfmode);
>> +		if (err)
>> +			return err;
>> +
>> +		/* Determine if we are in CQL mode. This alters the commands we do */
>> +		err = dytc_perfmode_get(&cur_perfmode, &cur_funcmode);
>> +		if (err)
>> +			return err;
>> +
>> +		if (cur_funcmode == DYTC_FUNCTION_CQL) {
>> +			/* To set the mode we need to disable CQL first*/
>> +			dytc_ignore_next_event = true; /* Ignore event */
>> +			err = dytc_command(DYTC_DISABLE_CQL, &output);
>> +			if (err)
>> +				return err;
>> +		}
> 
> This seems somewhat duplicated from the get() code-path. Also you already doing
> a DYTC_DISABLE_CQL and DYTC_ENABLE_CQL in dytc_perfmode_get, which is not necessary
> to just get the funcmode which is all you need here AFAICT.
> 
> IOW it seems that when CQL is active you are now doing:
> 
> 1. dytc_perfmode_get() calls DYTC_CMD_GET
> 2. dytc_perfmode_get() calls DYTC_DISABLE_CQL
> 3. dytc_perfmode_get() calls DYTC_CMD_GET again, result is ignored (not used by dytc_profile_set)
> 4. dytc_perfmode_get() calls DYTC_ENABLE_CQL
> 5. dytc_profile_set() calls DYTC_DISABLE_CQL
> 6. dytc_profile_set() calls DYTC_SET_COMMAND
> 7. dytc_profile_set() calls DYTC_ENABLE_CQL
> 
> And you can really skip step 2-4 here.
> 
> I think it would be good to add a bunch of helpers:
> 
> 1. dytc_get_modes() -> DYTC_CMD_GET wrapper gets both modes, sets perfmode
> to -1 when funcmode is CQL
> 2. dytc_disable_cql_if_necessary() which takes funcmode as argument and is
> a no-op when funcmode != CQL
> 3. dytc_re_enable_cql_if_necessary() idem.
> 
> And then the flow in dytc_perfmode_get could look something like this
> (pseudo code minus error handling):
> 
> 	dytc_get_modes(&funcmode, &perfmode)
> 	if (funcmode != CQL) /* or alternatively check for perfmode != -1 */
> 		return success;
> 
> 	dytc_disable_cql_if_necessary(funcmode);
> 	dytc_get_modes(NULL, &perfmode);
> 	dytc_disable_cql_if_necessary(funcmode);
> 
> And in the non-balanced path of dytc_profile_set:
> 
> 	dytc_get_modes(&funcmode, NULL)
> 
> 	dytc_disable_cql_if_necessary(funcmode);
> 	dytc_set_mode(...);
> 	dytc_disable_cql_if_necessary(funcmode);
> 
> Note the NULL could be a dummy, but I find NULL a bit cleaner
> (at the cost of having to check for it in dytc_get_modes).
> 
> This is is just from a quick peek, when you implement this
> it might turn out to be less then ideal, IOW this is just
> a suggestion, feel free to deviate.

Agreed - and thank you for the suggestions. I did prototype a similar 
method and it has worked out nicely. I've got a bit more cleanup but the 
code is better than it was.
> 
> ###
> 
> Since this will require a bit of work, timing wise (wrt the 5.11 merge-window)
> it might be best to just keep this patch as is for v5, and only change
> patch 1 and 2 of the set, so that those will hopefully be ready for
> merging in time for the 5.11 window. I plan to pick this one up
> once 5.11-rc1 is out (and has the necessary ACPI bits) so we have some
> more time to get this one in shape.
> 
> For patch 1/2 the most important thing is to have a consumer of the
> new internal APIs (almost) ready and this code fulfills that in
> its current form.
OK - I think that makes sense. Just curious though - will you then just 
accept the platform_profile pieces (1 & 2)? Would it make it easier if I 
just push the updated first two patches and drop thinkpad_acpi.c for now 
(it will follow shortly, but is going to be a couple more days) or would 
you rather have everything and just pick the bits you want?

I've got the v5 ready (I think) for the platform profile and am still 
working on thinkpad_acpi.c with the improvements from above. I think 
I'll be a couple more days there.

Thank you!
Mark