From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga18.intel.com (mga18.intel.com [134.134.136.126]) by gabe.freedesktop.org (Postfix) with ESMTPS id D51AA10E432 for ; Wed, 14 Jun 2023 09:34:01 +0000 (UTC) Message-ID: Date: Wed, 14 Jun 2023 15:03:55 +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> <20230613110110.3vjnpjtvuvn55jsv@kamilkon-desk1> <20230614092235.vbdjieyg6sexxhnq@kamilkon-desk1> From: "Sharma, Swati2" In-Reply-To: <20230614092235.vbdjieyg6sexxhnq@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: On 14-Jun-23 2:52 PM, Kamil Konieczny wrote: > Hi Swati, > > On 2023-06-14 at 10:30:25 +0530, Sharma, Swati2 wrote: >> >> 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 >>>>>> 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) { >>> -------------------------------------------- ^^^^ >>> >>> 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. > > Than you for explanation, maybe it will be worth to add > here some comment or igt_debug print line in > 12 case, > it is not a blocker, > > Reviewed-by: Kamil Konieczny Thanks Kamil for the review. Will surely add it as igt_debug print line. > >> >>> >>> 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 >>>>>>