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

["Nautiyal, Ankit K" <ankit.k.nautiyal@intel.com>]

On 6/2/2023 12:54 PM, 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) {
>> ---------------------------------------------- ^
>> 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.

LGTM.

As suggested by Kamil, should have braces if previous if block had braces.

With that fixed this is:

Reviewed-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>

>
>>
>>> +    } 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
>>>