Re: [PATCH i-g-t 1/2] lib/igt_psr: add helper to check of given psr mode can be enabled

["Modem, Bhanuprakash" <bhanuprakash.modem@intel.com>]

Hi Kunal,

On 24-01-2024 05:36 pm, Joshi, Kunal1 wrote:
> Hell bhanu
> 
> -----Original Message-----
> From: Modem, Bhanuprakash <bhanuprakash.modem@intel.com>
> Sent: Wednesday, January 24, 2024 5:28 PM
> To: Joshi, Kunal1 <kunal1.joshi@intel.com>; igt-dev@lists.freedesktop.org
> Cc: B, Jeevan <jeevan.b@intel.com>
> Subject: Re: [PATCH i-g-t 1/2] lib/igt_psr: add helper to check of given psr mode can be enabled
> 
> Hi Kunal,
> 
> On 24-01-2024 04:04 pm, Kunal Joshi wrote:
>> Check if given psr mode can be enabled by reading enable_psr modparam
>>
>> Cc: Jeevan B <jeevan.b@intel.com>
>> Cc: Bhanuprakash Modem <bhanuprakash.modem@intel.com>
>> Signed-off-by: Kunal Joshi <kunal1.joshi@intel.com>
>> Reviewed-by: Jeevan B <jeevan.b@intel.com>
>> ---
>>    lib/igt_psr.c | 20 ++++++++++++++++++++
>>    lib/igt_psr.h |  1 +
>>    2 files changed, 21 insertions(+)
>>
>> diff --git a/lib/igt_psr.c b/lib/igt_psr.c index 663bac163..ac214fcfc
>> 100644
>> --- a/lib/igt_psr.c
>> +++ b/lib/igt_psr.c
>> @@ -394,3 +394,23 @@ enum psr_mode psr_get_mode(int debugfs_fd)
>>    
>>    	return PSR_DISABLED;
>>    }
>> +
>> +/**
>> + * is_psr_enable_possible
>> + * Check if given psr mode can be enabled by reading enable_psr
>> + * modparam
>> + *
>> + * Returns:
>> + * True if given psr mode can be enabled, false otherwise.
>> + */
>> +bool is_psr_enable_possible(int drm_fd, enum psr_mode mode) {
>> +	char *param_value;
>> +	int enable_psr;
>> +
>> +	param_value = __igt_params_get(drm_fd, "enable_psr");
>> +	igt_assert_f(param_value, "Could not read enable_psr modparam\n");
> 
> IMHO, Please don't assert here. Instead, allow the user to decide if modparam "enable_psr" is not available.
> 
> - Bhanu
> 
> I think it's fine to assert since we expect enable_psr to be present,
> since the caller may not handle this, one less thing to worry 😊

I am just thinking about the scenario, where the panel is capable of PSR 
but failed to read "enable_psr".

As enable_psr is already present, I guess, we are OK with this change.

Acked-by: Bhanuprakash Modem <bhanuprakash.modem@intel.com>

> 
> I can see this is done similarly at many place
> example :- intel_pipe_output_combo_valid

Then we must review & fix them if required :-)

- Bhanu

> 
>> +	enable_psr = atoi(param_value);
>> +	free(param_value);
>> +	return enable_psr > mode;
>> +}
>> diff --git a/lib/igt_psr.h b/lib/igt_psr.h index 36711c0d4..82a4e8c5e
>> 100644
>> --- a/lib/igt_psr.h
>> +++ b/lib/igt_psr.h
>> @@ -61,5 +61,6 @@ bool i915_psr2_selective_fetch_check(int drm_fd);
>>    
>>    bool i915_psr2_sel_fetch_to_psr1(int drm_fd);
>>    void i915_psr2_sel_fetch_restore(int drm_fd);
>> +bool is_psr_enable_possible(int drm_fd, enum psr_mode mode);
>>    
>>    #endif[
> Thanks and Regards
> [Kunal Joshi]