[igt-dev] [PATCH i-g-t 2/2] tests/kms_dp_dsc: Add a subtest to force DSC output BPP
Srivatsa, Anusha
anusha.srivatsa at intel.com
Thu Jun 13 22:33:12 UTC 2019
>-----Original Message-----
>From: Navare, Manasi D
>Sent: Tuesday, June 11, 2019 12:55 PM
>To: Srivatsa, Anusha <anusha.srivatsa at intel.com>
>Cc: igt-dev at lists.freedesktop.org; Latvala, Petri <petri.latvala at intel.com>
>Subject: Re: [PATCH i-g-t 2/2] tests/kms_dp_dsc: Add a subtest to force DSC
>output BPP
>
>On Mon, Jun 10, 2019 at 04:26:17PM -0700, Anusha Srivatsa wrote:
>> This subtest uses the accepted DSC BPPs and tries to force a modeset
>> by setting a certain BPP as the output BPP for a connector.
>>
>> Cc: Petri Latvala <petri.latvala at intel.com>
>> Cc: Manasi Navare <manasi.d.navare at intel.com>
>> Signed-off-by: Anusha Srivatsa <anusha.srivatsa at intel.com>
>> ---
>> tests/kms_dp_dsc.c | 94
>> +++++++++++++++++++++++++++++++++++++++++++++-
>> 1 file changed, 92 insertions(+), 2 deletions(-)
>>
>> diff --git a/tests/kms_dp_dsc.c b/tests/kms_dp_dsc.c index
>> 7f2bf276..f936eb33 100644
>> --- a/tests/kms_dp_dsc.c
>> +++ b/tests/kms_dp_dsc.c
>> @@ -46,7 +46,8 @@
>>
>> enum dsc_test_type
>> {
>> - test_basic_dsc_enable
>> + test_basic_dsc_enable,
>> + test_basic_dsc_enable_bpp
>> };
>>
>> typedef struct {
>> @@ -67,6 +68,7 @@ typedef struct {
>>
>> bool force_dsc_en_orig;
>> int force_dsc_restore_fd = -1;
>> +int new_bpp;
>>
>> static inline void manual(const char *expected) { @@ -157,6 +159,17
>> @@ static void restore_force_dsc_en(void)
>> force_dsc_restore_fd = -1;
>> }
>>
>> +static void force_dp_dsc_enable_bpp(data_t *data) {
>> + char file_name[128] = {0};
>> + char buffer[20];
>> + sprintf(buffer, "%d", new_bpp);
>> + strcpy(file_name, data->conn_name);
>> + strcat(file_name, "/i915_dsc_bpp_slice_support");
>> + igt_debug ("Forcing DSC BPP to %d on %s\n", new_bpp, data-
>>conn_name);
>> + igt_sysfs_write(data->debugfs_fd, file_name, buffer,
>> +sizeof(buffer));
>
>Check the return value of igt_sysfs_write and assert if write failed
Yes. Makes sense.
>> +}
>> +
>> static void test_cleanup(data_t *data) {
>> igt_plane_t *primary;
>> @@ -229,6 +242,44 @@ static void update_display(data_t *data, enum
>dsc_test_type test_type)
>> "Default DSC enable failed on Connector: %s Pipe:
>%s\n",
>> data->conn_name,
>> kmstest_pipe_name(data->pipe));
>> + } else if (test_type == test_basic_dsc_enable_bpp) {
>> + bool enabled;
>> +
>> + igt_debug("DSC is supported on %s\n", data->conn_name);
>> +
>
>Unnecessary newline
>
>> + save_force_dsc_en(data);
>> + force_dp_dsc_enable(data);
>> +
>> + igt_debug("Trying to set BPP to %d\n", new_bpp);
>> +
>> + force_dp_dsc_enable_bpp(data);
>> +
>> + igt_output_set_pipe(data->output, data->pipe);
>> + igt_create_pattern_fb(data->drm_fd, data->mode->hdisplay,
>> + data->mode->vdisplay,
>> + DRM_FORMAT_XRGB8888,
>> + LOCAL_DRM_FORMAT_MOD_NONE,
>> + &data->fb_test_pattern);
>> + primary = igt_output_get_plane_type(data->output,
>> + DRM_PLANE_TYPE_PRIMARY);
>> + /* Now set the output to the desired mode */
>> + igt_plane_set_fb(primary, &data->fb_test_pattern);
>> + igt_display_commit(&data->display);
>> +
>> + /*
>> + * Until we have CRC check support, manually check if RGB test
>> + * pattern has no corruption.
>> + */
>> + manual("RGB test pattern without corruption");
>> +
>> + enabled = is_dp_dsc_enabled(data);
>> + restore_force_dsc_en();
>
>Not sure if we need to restore the DSC BPP value? What does the kernel code set
>the force_dsc_bpp value to if not forced by IGT?
I don't think we need it. Force_enable_bpp gets set iff we are forcing the dsc. If we are not forcing dsc, and the kernel does DSC because of the resolution restrictions, it computes the compressed bpp according to the calculation that are part of compute_dsc_params() and nothing to do with this value.
>
>> +
>> + igt_assert_f(enabled,
>> + "Default DSC BPP enable failed on Connector: %s
>Pipe: %s\n",
>> + data->conn_name,
>> + kmstest_pipe_name(data->pipe));
>> +
>> } else {
>> igt_assert(!"Unknown test type\n");
>> }
>> @@ -256,7 +307,9 @@ igt_main
>> igt_output_t *output;
>> drmModeRes *res;
>> drmModeConnector *connector;
>> - int i, test_conn_cnt, test_cnt;
>> + int i, j, test_conn_cnt, test_cnt;
>> + int dp_dsc_supported_compressed_bpp[] = {8, 10, 12, 15};
>
> May be a add a comment here saying these are the supported compressed BPPs
>on Gen 11
Sure.
>> +
>> int tests[] = {DRM_MODE_CONNECTOR_eDP,
>> DRM_MODE_CONNECTOR_DisplayPort};
>>
>> igt_fixture {
>> @@ -300,6 +353,43 @@ igt_main
>> }
>> igt_skip_on(test_conn_cnt == 0);
>> }
>> +
>> + for (j = 0; j < ARRAY_SIZE(dp_dsc_supported_compressed_bpp);
>j++) {
>> + new_bpp = dp_dsc_supported_compressed_bpp[j];
>> + igt_subtest_f("basic-dsc-enable-%dbpp-%s", new_bpp,
>> + kmstest_connector_type_str(tests[test_cnt]))
>{
>> + test_conn_cnt = 0;
>
>Check the indentation for the code within the igt_subtest_f, it needs one more
>tab
>
>> + for (i = 0; i < res->count_connectors; i++) {
>> + connector =
>drmModeGetConnectorCurrent(data.drm_fd,
>> + res-
>>connectors[i]);
>> + if (connector->connection !=
>DRM_MODE_CONNECTED ||
>> + connector->connector_type !=
>> + tests[test_cnt])
>> + continue;
>> + output =
>igt_output_from_connector(&data.display, connector);
>> + sprintf(data.conn_name, "%s-%d",
>> + kmstest_connector_type_str(connector-
>>connector_type),
>> + connector->connector_type_id);
>> + printf("connector name: %s test_conn_count
>%d\n", data.conn_name,
>> +test_conn_cnt++);
>
>printf is redundant here, if you want you could have this in igt_debug so its
>printed only for debug
Yes I agree. This was for my debugging. I will remove this. I dont think we need it to be in a igt_debug also.
Anusha
>
>> + if(!is_dp_dsc_supported(&data)) {
>> + igt_debug("DSC not supported on connector
>%s \n",
>> + data.conn_name);
>> + continue;
>> + }
>> + if (connector->connector_type ==
>DRM_MODE_CONNECTOR_DisplayPort &&
>> + !is_dp_fec_supported(&data)) {
>> + igt_debug("DSC cannot be enabled
>without FEC on %s\n",
>> + data.conn_name);
>> + continue;
>> + }
>> +
>
>Redundant newline
>
>Manasi
>
>
>> + test_conn_cnt++;
>> + run_test(&data, output,
>test_basic_dsc_enable_bpp);
>> + }
>> + igt_skip_on(test_conn_cnt == 0);
>> + }
>> + }
>> +
>> }
>>
>> igt_fixture {
>> --
>> 2.17.1
>>
More information about the igt-dev
mailing list