Re: [igt-dev] [PATCH i-g-t v3 3/3] tests: add igt_is_dsc_supported_by_source()

["Sharma, Swati2" <swati2.sharma@intel.com>]

On 13-Jun-23 4:31 PM, Kamil Konieczny wrote:
> Hi Swati,
> 
> On 2023-06-02 at 12:54:01 +0530, Sharma, Swati2 wrote:
>> Hi Kamil,
>>
>> Thanks for the reviews. Please find my reply inline.
>>
>> On 01-Jun-23 7:25 PM, Kamil Konieczny wrote:
>>> Hi Swati,
>>>
>>> On 2023-05-31 at 12:24:07 +0530, Swati Sharma wrote:
>>>> Instead of assuming dsc is supported on gen11+ platforms,
>>>> lets use has_dsc flag in i915_capability to check hardware
>>>> support.
>>>>
>>>> v2: -moved changes to lib (Ankit)
>>>> v3: -keep includes in alphabetical order (Kamil)
>>>>       -don't use igt_require() in lib (Kamil)
>>>>       -improve return statement (Kamil)
>>>>
>>>> Cc: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
>>>> Cc: Kamil Konieczny <kamil.konieczny@linux.intel.com>
>>>> Signed-off-by: Swati Sharma <swati2.sharma@intel.com>
>>>> ---
>>>>    lib/igt_dsc.c               | 22 ++++++++++++++++++++++
>>>>    lib/igt_dsc.h               |  1 +
>>>>    tests/i915/kms_dsc.c        |  2 +-
>>>>    tests/i915/kms_dsc_helper.c | 10 ++++++++++
>>>>    tests/i915/kms_dsc_helper.h |  1 +
>>>>    tests/kms_invalid_mode.c    |  3 +--
>>>>    6 files changed, 36 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/lib/igt_dsc.c b/lib/igt_dsc.c
>>>> index 6cbd8bae..76a420c1 100644
>>>> --- a/lib/igt_dsc.c
>>>> +++ b/lib/igt_dsc.c
>>>> @@ -6,6 +6,7 @@
>>>>    #include <unistd.h>
>>>>    #include <fcntl.h>
>>>>    #include <string.h>
>>>> +#include "igt_core.h"
>>>>    #include "igt_dsc.h"
>>>>    #include "igt_sysfs.h"
>>>> @@ -41,6 +42,27 @@ static int write_dsc_debugfs(int drmfd, char *connector_name, const char *file_n
>>>>    	return ret;
>>>>    }
>>>> +/*
>>>> + * igt_is_dsc_supported_by_source:
>>>> + * @drmfd: A drm file descriptor
>>>> + *
>>>> + * Returns: True if DSC is supported by source, false otherwise.
>>>> + */
>>>> +bool igt_is_dsc_supported_by_source(int drmfd)
>>>> +{
>>>> +	char buf[4096];
>>>> +	int dir, res;
>>>> +
>>>> +	dir = igt_debugfs_dir(drmfd);
>>>> +	igt_assert(dir >= 0);
>>>> +
>>>> +	res = igt_debugfs_simple_read(dir, "i915_capabilities",
>>>> +				      buf, sizeof(buf));
>>>> +	close(dir);
>>>> +
>>>> +	return res > 0 ? strstr(buf, "has_dsc: yes") : 0;
>>>> +}
>>>> +
>>>>    /*
>>>>     * igt_is_dsc_supported_by_sink:
>>>>     * @drmfd: A drm file descriptor
>>>> diff --git a/lib/igt_dsc.h b/lib/igt_dsc.h
>>>> index 241fc0ac..b58743b5 100644
>>>> --- a/lib/igt_dsc.h
>>>> +++ b/lib/igt_dsc.h
>>>> @@ -9,6 +9,7 @@
>>>>    #include "igt_fb.h"
>>>>    #include "igt_kms.h"
>>>> +bool igt_is_dsc_supported_by_source(int drmfd);
>>>>    bool igt_is_dsc_supported_by_sink(int drmfd, char *connector_name);
>>>>    bool igt_is_fec_supported(int drmfd, char *connector_name);
>>>>    bool igt_is_dsc_enabled(int drmfd, char *connector_name);
>>>> diff --git a/tests/i915/kms_dsc.c b/tests/i915/kms_dsc.c
>>>> index d142fae2..b46c4449 100644
>>>> --- a/tests/i915/kms_dsc.c
>>>> +++ b/tests/i915/kms_dsc.c
>>>> @@ -265,7 +265,7 @@ igt_main
>>>>    		igt_install_exit_handler(kms_dsc_exit_handler);
>>>>    		igt_display_require(&data.display, data.drm_fd);
>>>>    		igt_display_require_output(&data.display);
>>>> -		igt_require(data.disp_ver >= 11);
>>>> +		igt_require(is_dsc_supported_by_source(data.drm_fd));
>>>>    	}
>>>>    	igt_describe("Tests basic display stream compression functionality if supported "
>>>> diff --git a/tests/i915/kms_dsc_helper.c b/tests/i915/kms_dsc_helper.c
>>>> index c90d833d..61f76dde 100644
>>>> --- a/tests/i915/kms_dsc_helper.c
>>>> +++ b/tests/i915/kms_dsc_helper.c
>>>> @@ -53,6 +53,16 @@ void kms_dsc_exit_handler(int sig)
>>>>    	restore_force_dsc_en();
>>>>    }
>>>> +bool is_dsc_supported_by_source(int drmfd)
>>>> +{
>>>> +	if (!igt_is_dsc_supported_by_source(drmfd)) {
>>>> +		igt_debug("DSC not supported by source\n");
>>>> +		return false;
>>>> +	}
>>>> +
>>>> +	return true;
>>>> +}
>>>> +
>>>>    bool is_dsc_supported_by_sink(int drmfd, igt_output_t *output)
>>>>    {
>>>>    	if (!igt_is_dsc_supported_by_sink(drmfd, output->name)) {
>>>> diff --git a/tests/i915/kms_dsc_helper.h b/tests/i915/kms_dsc_helper.h
>>>> index f66191c7..2109bd76 100644
>>>> --- a/tests/i915/kms_dsc_helper.h
>>>> +++ b/tests/i915/kms_dsc_helper.h
>>>> @@ -27,6 +27,7 @@ void save_force_dsc_en(int drmfd, igt_output_t *output);
>>>>    void restore_force_dsc_en(void);
>>>>    void kms_dsc_exit_handler(int sig);
>>>>    bool is_dsc_supported_by_sink(int drmfd, igt_output_t *output);
>>>> +bool is_dsc_supported_by_source(int drmfd);
>>>>    bool check_gen11_dp_constraint(int drmfd, igt_output_t *output, enum pipe pipe);
>>>>    bool check_gen11_bpc_constraint(int drmfd, igt_output_t *output, int input_bpc);
>>>>    void force_dsc_output_format(int drmfd, igt_output_t *output,
>>>> diff --git a/tests/kms_invalid_mode.c b/tests/kms_invalid_mode.c
>>>> index 63da3a1c..e4ab65f2 100644
>>>> --- a/tests/kms_invalid_mode.c
>>>> +++ b/tests/kms_invalid_mode.c
>>>> @@ -60,9 +60,8 @@ can_bigjoiner(data_t *data)
>>>>    	if (intel_display_ver(devid) > 12) {
> -------------------------------------------- ^^^^
> 
>>>>    		igt_debug("Platform supports uncompressed bigjoiner\n");
>>>>    		return true;
>>>> -	} else if (intel_display_ver(devid) >= 11) {
> -------------------------------------------- ^^^^
> 
> If you insist on removing this check above " >= 11"
> maybe also remove check of " > 12 " ? Then you could just write:
> 
> 	if (igt_is_dsc_supported_by_source(data->drm_fd) &&
> 	    igt_is_dsc_supported_by_sink(data->drm_fd, data->output->name)) {
> 
>     		igt_debug("Platform supports uncompressed bigjoiner\n");
> 		return true;
> 	}

There are 2 things here:
1. if disp_ver > 12 then platform supports "uncompressed" big joiner, 
here we don't have to check dsc support since its uncompressed.
2. if disp_ver >= 11 && disp_ver <= 12 then platform supports 
"compressed" big joiner and we need to check dsc support which instead 
of using via gen check we are checking with debugfs now.

So, big joiner check we still need only dsc check is not required.

> 
> This way you could also support any future change when new
> display will not support it.
> 
> Regards,
> Kamil
> 
>>> ---------------------------------------------- ^
>>> imho you should keep it here unless there are platforms <= 10 with
>>> that capability ?
>>
>> i915 driver supports dsc from display_ver >= 11
>> The whole point of removing this check is to get info from i915_cap to know
>> if h/w supports dsc or not.
>>
>>> Also previous if had braces { } so you should also use them after else:
>>
>> okay.
>>
>>>
>>>> +	} else if (igt_is_dsc_supported_by_source(data->drm_fd))
>>> ------------- ^
>>>>    		return igt_is_dsc_supported_by_sink(data->drm_fd, data->output->name);
>>>> -	}
>>>
>>> imho better:
>>> 	} else if (intel_display_ver(devid) >= 11) {
>>> 		return igt_is_dsc_supported_by_source(data->drm_fd) &&
>>> 		       igt_is_dsc_supported_by_sink(data->drm_fd, data->output->name);
>>> 	}
>>>
>>> Regards,
>>> Kamil
>>>
>>>> -	}
>>>>    	return false;
>>>>    }
>>>> -- 
>>>> 2.25.1
>>>>