From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga17.intel.com (mga17.intel.com [192.55.52.151]) by gabe.freedesktop.org (Postfix) with ESMTPS id 2A35910E146 for ; Wed, 14 Jun 2023 12:43:42 +0000 (UTC) Message-ID: Date: Wed, 14 Jun 2023 18:13:21 +0530 To: "Sharma, Swati2" , Kamil Konieczny , References: <20230531065407.362625-1-swati2.sharma@intel.com> <20230531065407.362625-4-swati2.sharma@intel.com> <20230601135559.3cnml6oqozfooxkb@kamilkon-desk1> Content-Language: en-US From: "Nautiyal, Ankit K" In-Reply-To: Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 8bit MIME-Version: 1.0 Subject: Re: [igt-dev] [PATCH i-g-t v3 3/3] tests: add igt_is_dsc_supported_by_source() List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: igt-dev-bounces@lists.freedesktop.org Sender: "igt-dev" List-ID: 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 >>> Cc: Kamil Konieczny >>> Signed-off-by: Swati Sharma >>> --- >>>   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 >>>   #include >>>   #include >>> +#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 > >> >>> +    } 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 >>>