From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) by gabe.freedesktop.org (Postfix) with ESMTPS id C943210E09E for ; Fri, 2 Jun 2023 07:24:11 +0000 (UTC) Message-ID: Date: Fri, 2 Jun 2023 12:54:01 +0530 MIME-Version: 1.0 Content-Language: en-US To: Kamil Konieczny , igt-dev@lists.freedesktop.org, Ankit Nautiyal References: <20230531065407.362625-1-swati2.sharma@intel.com> <20230531065407.362625-4-swati2.sharma@intel.com> <20230601135559.3cnml6oqozfooxkb@kamilkon-desk1> From: "Sharma, Swati2" In-Reply-To: <20230601135559.3cnml6oqozfooxkb@kamilkon-desk1> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit 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: 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. > >> + } 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 >>