* [PATCH v2 1/2] CONTRIBUTING: Fix spelling mistake and line length @ 2017-12-07 0:17 Sean Paul 2017-12-07 0:17 ` [PATCH v2 2/2] kms_content_protection: Add Content Protection test Sean Paul 0 siblings, 1 reply; 12+ messages in thread From: Sean Paul @ 2017-12-07 0:17 UTC (permalink / raw) To: intel-gfx; +Cc: seanpaul Noticed while I was reading it. Makes for a good first contribution, I guess. Changes in v2: - None Reviewed-by: Petri Latvala <petri.latvala@intel.com> Signed-off-by: Sean Paul <seanpaul@chromium.org> --- CONTRIBUTING | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/CONTRIBUTING b/CONTRIBUTING index 561c5dd8..ca2ed8a5 100644 --- a/CONTRIBUTING +++ b/CONTRIBUTING @@ -18,13 +18,13 @@ A short list of contribution guidelines: on its first invocation. -- intel-gpu-tools is MIT lincensed and we require contributions to follow the +- intel-gpu-tools is MIT licensed and we require contributions to follow the developer's certificate of origin: http://developercertificate.org/ - When submitting new testcases please follow the naming conventions documented - in the generated documentation. Also please make full use of all the helpers and - convenience macros provided by the igt library. The semantic patch lib/igt.cocci - can help with the more automatic conversions. + in the generated documentation. Also please make full use of all the helpers + and convenience macros provided by the igt library. The semantic patch + lib/igt.cocci can help with the more automatic conversions. - Patches need to be reviewed on the mailing list. Exceptions only apply for testcases and tooling for drivers with just a single contributor (e.g. vc4). -- 2.15.1.424.g9478a66081-goog _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v2 2/2] kms_content_protection: Add Content Protection test 2017-12-07 0:17 [PATCH v2 1/2] CONTRIBUTING: Fix spelling mistake and line length Sean Paul @ 2017-12-07 0:17 ` Sean Paul 2017-12-11 10:53 ` Ramalingam C 2017-12-13 10:37 ` Ramalingam C 0 siblings, 2 replies; 12+ messages in thread From: Sean Paul @ 2017-12-07 0:17 UTC (permalink / raw) To: intel-gfx; +Cc: seanpaul Pretty simple test: - initializes the output - clears the content protection property - verifies that it clears - sets the content protection property to desired - verifies that it transitions to enabled Does this for both legacy and atomic. Changes in v2: - Don't check for i915 gen - Skip test if Content Protection property is absent Signed-off-by: Sean Paul <seanpaul@chromium.org> --- lib/igt_kms.c | 3 +- lib/igt_kms.h | 1 + tests/Makefile.sources | 1 + tests/kms_content_protection.c | 129 +++++++++++++++++++++++++++++++++++++++++ tests/meson.build | 1 + 5 files changed, 134 insertions(+), 1 deletion(-) create mode 100644 tests/kms_content_protection.c diff --git a/lib/igt_kms.c b/lib/igt_kms.c index 125ecb19..907db694 100644 --- a/lib/igt_kms.c +++ b/lib/igt_kms.c @@ -190,7 +190,8 @@ const char *igt_connector_prop_names[IGT_NUM_CONNECTOR_PROPS] = { "scaling mode", "CRTC_ID", "DPMS", - "Broadcast RGB" + "Broadcast RGB", + "Content Protection" }; /* diff --git a/lib/igt_kms.h b/lib/igt_kms.h index 2a480bf3..93e59dc7 100644 --- a/lib/igt_kms.h +++ b/lib/igt_kms.h @@ -118,6 +118,7 @@ enum igt_atomic_connector_properties { IGT_CONNECTOR_CRTC_ID, IGT_CONNECTOR_DPMS, IGT_CONNECTOR_BROADCAST_RGB, + IGT_CONNECTOR_CONTENT_PROTECTION, IGT_NUM_CONNECTOR_PROPS }; diff --git a/tests/Makefile.sources b/tests/Makefile.sources index 34ca71a0..e0466411 100644 --- a/tests/Makefile.sources +++ b/tests/Makefile.sources @@ -179,6 +179,7 @@ TESTS_progs = \ kms_chv_cursor_fail \ kms_color \ kms_concurrent \ + kms_content_protection\ kms_crtc_background_color \ kms_cursor_crc \ kms_cursor_legacy \ diff --git a/tests/kms_content_protection.c b/tests/kms_content_protection.c new file mode 100644 index 00000000..5d61b257 --- /dev/null +++ b/tests/kms_content_protection.c @@ -0,0 +1,129 @@ +/* + * Copyright © 2017 Google, Inc. + * + * Permission is hereby granted, free of charge, to any person obtaining a + * copy of this software and associated documentation files (the "Software"), + * to deal in the Software without restriction, including without limitation + * the rights to use, copy, modify, merge, publish, distribute, sublicense, + * and/or sell copies of the Software, and to permit persons to whom the + * Software is furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice (including the next + * paragraph) shall be included in all copies or substantial portions of the + * Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS + * IN THE SOFTWARE. + * + */ + +#include "igt.h" + +IGT_TEST_DESCRIPTION("Test content protection (HDCP)"); + +typedef struct { + int drm_fd; + igt_display_t display; +} data_t; + +static bool +wait_for_prop_value(igt_output_t *output, uint64_t expected) +{ + uint64_t val; + int i; + + for (i = 0; i < 2000; i++) { + val = igt_output_get_prop(output, + IGT_CONNECTOR_CONTENT_PROTECTION); + if (val == expected) + return true; + usleep(1000); + } + igt_info("prop_value mismatch %ld != %ld\n", val, expected); + return false; +} + +static void +test_pipe_output(igt_display_t *display, const enum pipe pipe, + igt_output_t *output, enum igt_commit_style s) +{ + drmModeModeInfo mode; + igt_plane_t *primary; + struct igt_fb red; + bool ret; + + igt_assert(kmstest_get_connector_default_mode( + display->drm_fd, output->config.connector, &mode)); + + igt_output_override_mode(output, &mode); + igt_output_set_pipe(output, pipe); + + igt_create_color_fb(display->drm_fd, mode.hdisplay, mode.vdisplay, + DRM_FORMAT_XRGB8888, LOCAL_DRM_FORMAT_MOD_NONE, + 1.f, 0.f, 0.f, &red); + primary = igt_output_get_plane_type(output, DRM_PLANE_TYPE_PRIMARY); + igt_plane_set_fb(primary, &red); + igt_display_commit2(display, s); + + igt_output_set_prop_value(output, IGT_CONNECTOR_CONTENT_PROTECTION, 0); + igt_display_commit2(display, s); + + ret = wait_for_prop_value(output, 0); + igt_require_f(ret, "Content Protection not cleared\n"); + + igt_output_set_prop_value(output, IGT_CONNECTOR_CONTENT_PROTECTION, 1); + igt_display_commit2(display, s); + + ret = wait_for_prop_value(output, 2); + igt_require_f(ret, "Content Protection not enabled\n"); + + igt_plane_set_fb(primary, NULL); + igt_output_set_pipe(output, PIPE_NONE); +} + +static void +test_content_protection(igt_display_t *display, enum igt_commit_style s) +{ + igt_output_t *output; + enum pipe pipe; + int valid_tests = 0; + + for_each_pipe_with_valid_output(display, pipe, output) { + if (!output->props[IGT_CONNECTOR_CONTENT_PROTECTION]) + continue; + + test_pipe_output(display, pipe, output, s); + valid_tests++; + } + igt_require_f(valid_tests, "no support for content proteciton found\n"); +} + +igt_main +{ + data_t data = {}; + + igt_fixture { + igt_skip_on_simulation(); + + data.drm_fd = drm_open_driver(DRIVER_ANY); + + igt_display_init(&data.display, data.drm_fd); + } + + + igt_subtest("legacy") + test_content_protection(&data.display, COMMIT_LEGACY); + + igt_subtest("atomic") { + igt_require(data.display.is_atomic); + test_content_protection(&data.display, COMMIT_ATOMIC); + } + + igt_fixture + igt_display_fini(&data.display); +} diff --git a/tests/meson.build b/tests/meson.build index 59ccd9a6..b12c35c0 100644 --- a/tests/meson.build +++ b/tests/meson.build @@ -157,6 +157,7 @@ test_progs = [ 'kms_chv_cursor_fail', 'kms_color', 'kms_concurrent', + 'kms_content_protection', 'kms_crtc_background_color', 'kms_cursor_crc', 'kms_cursor_legacy', -- 2.15.1.424.g9478a66081-goog _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/2] kms_content_protection: Add Content Protection test 2017-12-07 0:17 ` [PATCH v2 2/2] kms_content_protection: Add Content Protection test Sean Paul @ 2017-12-11 10:53 ` Ramalingam C 2017-12-13 10:37 ` Ramalingam C 1 sibling, 0 replies; 12+ messages in thread From: Ramalingam C @ 2017-12-11 10:53 UTC (permalink / raw) To: intel-gfx On Thursday 07 December 2017 05:47 AM, Sean Paul wrote: > Pretty simple test: > - initializes the output > - clears the content protection property > - verifies that it clears > - sets the content protection property to desired > - verifies that it transitions to enabled > > Does this for both legacy and atomic. > > Changes in v2: > - Don't check for i915 gen > - Skip test if Content Protection property is absent > > Signed-off-by: Sean Paul <seanpaul@chromium.org> > --- > lib/igt_kms.c | 3 +- > lib/igt_kms.h | 1 + > tests/Makefile.sources | 1 + > tests/kms_content_protection.c | 129 +++++++++++++++++++++++++++++++++++++++++ > tests/meson.build | 1 + > 5 files changed, 134 insertions(+), 1 deletion(-) > create mode 100644 tests/kms_content_protection.c > > diff --git a/lib/igt_kms.c b/lib/igt_kms.c > index 125ecb19..907db694 100644 > --- a/lib/igt_kms.c > +++ b/lib/igt_kms.c > @@ -190,7 +190,8 @@ const char *igt_connector_prop_names[IGT_NUM_CONNECTOR_PROPS] = { > "scaling mode", > "CRTC_ID", > "DPMS", > - "Broadcast RGB" > + "Broadcast RGB", > + "Content Protection" > }; > > /* > diff --git a/lib/igt_kms.h b/lib/igt_kms.h > index 2a480bf3..93e59dc7 100644 > --- a/lib/igt_kms.h > +++ b/lib/igt_kms.h > @@ -118,6 +118,7 @@ enum igt_atomic_connector_properties { > IGT_CONNECTOR_CRTC_ID, > IGT_CONNECTOR_DPMS, > IGT_CONNECTOR_BROADCAST_RGB, > + IGT_CONNECTOR_CONTENT_PROTECTION, > IGT_NUM_CONNECTOR_PROPS > }; > > diff --git a/tests/Makefile.sources b/tests/Makefile.sources > index 34ca71a0..e0466411 100644 > --- a/tests/Makefile.sources > +++ b/tests/Makefile.sources > @@ -179,6 +179,7 @@ TESTS_progs = \ > kms_chv_cursor_fail \ > kms_color \ > kms_concurrent \ > + kms_content_protection\ > kms_crtc_background_color \ > kms_cursor_crc \ > kms_cursor_legacy \ > diff --git a/tests/kms_content_protection.c b/tests/kms_content_protection.c > new file mode 100644 > index 00000000..5d61b257 > --- /dev/null > +++ b/tests/kms_content_protection.c > @@ -0,0 +1,129 @@ > +/* > + * Copyright © 2017 Google, Inc. > + * > + * Permission is hereby granted, free of charge, to any person obtaining a > + * copy of this software and associated documentation files (the "Software"), > + * to deal in the Software without restriction, including without limitation > + * the rights to use, copy, modify, merge, publish, distribute, sublicense, > + * and/or sell copies of the Software, and to permit persons to whom the > + * Software is furnished to do so, subject to the following conditions: > + * > + * The above copyright notice and this permission notice (including the next > + * paragraph) shall be included in all copies or substantial portions of the > + * Software. > + * > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS > + * IN THE SOFTWARE. > + * > + */ > + > +#include "igt.h" > + > +IGT_TEST_DESCRIPTION("Test content protection (HDCP)"); > + > +typedef struct { > + int drm_fd; > + igt_display_t display; > +} data_t; > + > +static bool > +wait_for_prop_value(igt_output_t *output, uint64_t expected) > +{ > + uint64_t val; > + int i; > + > + for (i = 0; i < 2000; i++) { > + val = igt_output_get_prop(output, > + IGT_CONNECTOR_CONTENT_PROTECTION); > + if (val == expected) > + return true; > + usleep(1000); > + } > + igt_info("prop_value mismatch %ld != %ld\n", val, expected); > + return false; > +} > + > +static void > +test_pipe_output(igt_display_t *display, const enum pipe pipe, > + igt_output_t *output, enum igt_commit_style s) > +{ > + drmModeModeInfo mode; > + igt_plane_t *primary; > + struct igt_fb red; > + bool ret; > + > + igt_assert(kmstest_get_connector_default_mode( > + display->drm_fd, output->config.connector, &mode)); > + > + igt_output_override_mode(output, &mode); > + igt_output_set_pipe(output, pipe); > + > + igt_create_color_fb(display->drm_fd, mode.hdisplay, mode.vdisplay, > + DRM_FORMAT_XRGB8888, LOCAL_DRM_FORMAT_MOD_NONE, > + 1.f, 0.f, 0.f, &red); > + primary = igt_output_get_plane_type(output, DRM_PLANE_TYPE_PRIMARY); > + igt_plane_set_fb(primary, &red); > + igt_display_commit2(display, s); > + > + igt_output_set_prop_value(output, IGT_CONNECTOR_CONTENT_PROTECTION, 0); > + igt_display_commit2(display, s); > + > + ret = wait_for_prop_value(output, 0); > + igt_require_f(ret, "Content Protection not cleared\n"); > + > + igt_output_set_prop_value(output, IGT_CONNECTOR_CONTENT_PROTECTION, 1); > + igt_display_commit2(display, s); > + > + ret = wait_for_prop_value(output, 2); > + igt_require_f(ret, "Content Protection not enabled\n"); > + > + igt_plane_set_fb(primary, NULL); > + igt_output_set_pipe(output, PIPE_NONE); > +} > + > +static void > +test_content_protection(igt_display_t *display, enum igt_commit_style s) > +{ > + igt_output_t *output; > + enum pipe pipe; > + int valid_tests = 0; > + > + for_each_pipe_with_valid_output(display, pipe, output) { > + if (!output->props[IGT_CONNECTOR_CONTENT_PROTECTION]) > + continue; > + > + test_pipe_output(display, pipe, output, s); > + valid_tests++; > + } > + igt_require_f(valid_tests, "no support for content proteciton found\n"); > +} > + > +igt_main > +{ > + data_t data = {}; > + > + igt_fixture { > + igt_skip_on_simulation(); > + > + data.drm_fd = drm_open_driver(DRIVER_ANY); For atomic ioctl, Dont we need drm_open_driver_master() here? --Ram > + > + igt_display_init(&data.display, data.drm_fd); > + } > + > + > + igt_subtest("legacy") > + test_content_protection(&data.display, COMMIT_LEGACY); > + > + igt_subtest("atomic") { > + igt_require(data.display.is_atomic); > + test_content_protection(&data.display, COMMIT_ATOMIC); > + } > + > + igt_fixture > + igt_display_fini(&data.display); > +} > diff --git a/tests/meson.build b/tests/meson.build > index 59ccd9a6..b12c35c0 100644 > --- a/tests/meson.build > +++ b/tests/meson.build > @@ -157,6 +157,7 @@ test_progs = [ > 'kms_chv_cursor_fail', > 'kms_color', > 'kms_concurrent', > + 'kms_content_protection', > 'kms_crtc_background_color', > 'kms_cursor_crc', > 'kms_cursor_legacy', _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/2] kms_content_protection: Add Content Protection test 2017-12-07 0:17 ` [PATCH v2 2/2] kms_content_protection: Add Content Protection test Sean Paul 2017-12-11 10:53 ` Ramalingam C @ 2017-12-13 10:37 ` Ramalingam C 2017-12-19 14:28 ` Ramalingam C 1 sibling, 1 reply; 12+ messages in thread From: Ramalingam C @ 2017-12-13 10:37 UTC (permalink / raw) To: Sean Paul, intel-gfx; +Cc: seanpaul Sean, Adding few more observations. On Thursday 07 December 2017 05:47 AM, Sean Paul wrote: > Pretty simple test: > - initializes the output > - clears the content protection property > - verifies that it clears > - sets the content protection property to desired > - verifies that it transitions to enabled > > Does this for both legacy and atomic. > > Changes in v2: > - Don't check for i915 gen > - Skip test if Content Protection property is absent > > Signed-off-by: Sean Paul <seanpaul@chromium.org> > --- > lib/igt_kms.c | 3 +- > lib/igt_kms.h | 1 + > tests/Makefile.sources | 1 + > tests/kms_content_protection.c | 129 +++++++++++++++++++++++++++++++++++++++++ > tests/meson.build | 1 + > 5 files changed, 134 insertions(+), 1 deletion(-) > create mode 100644 tests/kms_content_protection.c > > diff --git a/lib/igt_kms.c b/lib/igt_kms.c > index 125ecb19..907db694 100644 > --- a/lib/igt_kms.c > +++ b/lib/igt_kms.c > @@ -190,7 +190,8 @@ const char *igt_connector_prop_names[IGT_NUM_CONNECTOR_PROPS] = { > "scaling mode", > "CRTC_ID", > "DPMS", > - "Broadcast RGB" > + "Broadcast RGB", > + "Content Protection" > }; > > /* > diff --git a/lib/igt_kms.h b/lib/igt_kms.h > index 2a480bf3..93e59dc7 100644 > --- a/lib/igt_kms.h > +++ b/lib/igt_kms.h > @@ -118,6 +118,7 @@ enum igt_atomic_connector_properties { > IGT_CONNECTOR_CRTC_ID, > IGT_CONNECTOR_DPMS, > IGT_CONNECTOR_BROADCAST_RGB, > + IGT_CONNECTOR_CONTENT_PROTECTION, > IGT_NUM_CONNECTOR_PROPS > }; > > diff --git a/tests/Makefile.sources b/tests/Makefile.sources > index 34ca71a0..e0466411 100644 > --- a/tests/Makefile.sources > +++ b/tests/Makefile.sources > @@ -179,6 +179,7 @@ TESTS_progs = \ > kms_chv_cursor_fail \ > kms_color \ > kms_concurrent \ > + kms_content_protection\ > kms_crtc_background_color \ > kms_cursor_crc \ > kms_cursor_legacy \ > diff --git a/tests/kms_content_protection.c b/tests/kms_content_protection.c > new file mode 100644 > index 00000000..5d61b257 > --- /dev/null > +++ b/tests/kms_content_protection.c > @@ -0,0 +1,129 @@ > +/* > + * Copyright © 2017 Google, Inc. > + * > + * Permission is hereby granted, free of charge, to any person obtaining a > + * copy of this software and associated documentation files (the "Software"), > + * to deal in the Software without restriction, including without limitation > + * the rights to use, copy, modify, merge, publish, distribute, sublicense, > + * and/or sell copies of the Software, and to permit persons to whom the > + * Software is furnished to do so, subject to the following conditions: > + * > + * The above copyright notice and this permission notice (including the next > + * paragraph) shall be included in all copies or substantial portions of the > + * Software. > + * > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS > + * IN THE SOFTWARE. > + * > + */ > + > +#include "igt.h" > + > +IGT_TEST_DESCRIPTION("Test content protection (HDCP)"); > + > +typedef struct { > + int drm_fd; > + igt_display_t display; > +} data_t; > + > +static bool > +wait_for_prop_value(igt_output_t *output, uint64_t expected) > +{ > + uint64_t val; > + int i; > + > + for (i = 0; i < 2000; i++) { we need 5+Sec to complete the Second part of authentication, in case of repeater with max downstream topology. So we might need to wait for 6000(6Sec) loops!? > + val = igt_output_get_prop(output, > + IGT_CONNECTOR_CONTENT_PROTECTION); > + if (val == expected) > + return true; > + usleep(1000); > + } > + igt_info("prop_value mismatch %ld != %ld\n", val, expected); > + return false; > +} > + > +static void > +test_pipe_output(igt_display_t *display, const enum pipe pipe, > + igt_output_t *output, enum igt_commit_style s) > +{ > + drmModeModeInfo mode; > + igt_plane_t *primary; > + struct igt_fb red; > + bool ret; > + > + igt_assert(kmstest_get_connector_default_mode( > + display->drm_fd, output->config.connector, &mode)); > + > + igt_output_override_mode(output, &mode); > + igt_output_set_pipe(output, pipe); > + > + igt_create_color_fb(display->drm_fd, mode.hdisplay, mode.vdisplay, > + DRM_FORMAT_XRGB8888, LOCAL_DRM_FORMAT_MOD_NONE, > + 1.f, 0.f, 0.f, &red); > + primary = igt_output_get_plane_type(output, DRM_PLANE_TYPE_PRIMARY); > + igt_plane_set_fb(primary, &red); > + igt_display_commit2(display, s); > + > + igt_output_set_prop_value(output, IGT_CONNECTOR_CONTENT_PROTECTION, 0); > + igt_display_commit2(display, s); > + > + ret = wait_for_prop_value(output, 0); > + igt_require_f(ret, "Content Protection not cleared\n"); When expected property state is not met, you might want to use igt_assert_f to fail the subtest. Here you are just skipping the subtest. > + > + igt_output_set_prop_value(output, IGT_CONNECTOR_CONTENT_PROTECTION, 1); > + igt_display_commit2(display, s); > + > + ret = wait_for_prop_value(output, 2); > + igt_require_f(ret, "Content Protection not enabled\n"); same as above. igt_assert_f needed? And when you are done with a connector(output), we cant leave the content protection in desired state. Else when we enable the connector for some other IGT, content protection authentication will be attempted by kernel. So when we are done with a connector, we need to clear the content protection state(set to OFF) of the connector. > + > + igt_plane_set_fb(primary, NULL); > + igt_output_set_pipe(output, PIPE_NONE); > +} > + > +static void > +test_content_protection(igt_display_t *display, enum igt_commit_style s) > +{ > + igt_output_t *output; > + enum pipe pipe; > + int valid_tests = 0; > + > + for_each_pipe_with_valid_output(display, pipe, output) { > + if (!output->props[IGT_CONNECTOR_CONTENT_PROTECTION]) > + continue; > + > + test_pipe_output(display, pipe, output, s); > + valid_tests++; > + } > + igt_require_f(valid_tests, "no support for content proteciton found\n"); > +} > + > +igt_main > +{ > + data_t data = {}; > + > + igt_fixture { > + igt_skip_on_simulation(); > + > + data.drm_fd = drm_open_driver(DRIVER_ANY); You need Master privilege here. Hence might want to use drm_open_driver_master() instead. -Ram > + > + igt_display_init(&data.display, data.drm_fd); > + } > + > + > + igt_subtest("legacy") > + test_content_protection(&data.display, COMMIT_LEGACY); > + > + igt_subtest("atomic") { > + igt_require(data.display.is_atomic); > + test_content_protection(&data.display, COMMIT_ATOMIC); > + } > + > + igt_fixture > + igt_display_fini(&data.display); > +} > diff --git a/tests/meson.build b/tests/meson.build > index 59ccd9a6..b12c35c0 100644 > --- a/tests/meson.build > +++ b/tests/meson.build > @@ -157,6 +157,7 @@ test_progs = [ > 'kms_chv_cursor_fail', > 'kms_color', > 'kms_concurrent', > + 'kms_content_protection', > 'kms_crtc_background_color', > 'kms_cursor_crc', > 'kms_cursor_legacy', _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/2] kms_content_protection: Add Content Protection test 2017-12-13 10:37 ` Ramalingam C @ 2017-12-19 14:28 ` Ramalingam C 2018-01-05 19:29 ` Sean Paul 0 siblings, 1 reply; 12+ messages in thread From: Ramalingam C @ 2017-12-19 14:28 UTC (permalink / raw) To: Sean Paul, intel-gfx; +Cc: seanpaul Adding few more findings from the IGT usage with kernel solutions. On Wednesday 13 December 2017 04:07 PM, Ramalingam C wrote: > Sean, > > Adding few more observations. > > > On Thursday 07 December 2017 05:47 AM, Sean Paul wrote: >> Pretty simple test: >> - initializes the output >> - clears the content protection property >> - verifies that it clears >> - sets the content protection property to desired >> - verifies that it transitions to enabled >> >> Does this for both legacy and atomic. >> >> Changes in v2: >> - Don't check for i915 gen >> - Skip test if Content Protection property is absent >> >> Signed-off-by: Sean Paul <seanpaul@chromium.org> >> --- >> lib/igt_kms.c | 3 +- >> lib/igt_kms.h | 1 + >> tests/Makefile.sources | 1 + >> tests/kms_content_protection.c | 129 >> +++++++++++++++++++++++++++++++++++++++++ >> tests/meson.build | 1 + >> 5 files changed, 134 insertions(+), 1 deletion(-) >> create mode 100644 tests/kms_content_protection.c >> >> diff --git a/lib/igt_kms.c b/lib/igt_kms.c >> index 125ecb19..907db694 100644 >> --- a/lib/igt_kms.c >> +++ b/lib/igt_kms.c >> @@ -190,7 +190,8 @@ const char >> *igt_connector_prop_names[IGT_NUM_CONNECTOR_PROPS] = { >> "scaling mode", >> "CRTC_ID", >> "DPMS", >> - "Broadcast RGB" >> + "Broadcast RGB", >> + "Content Protection" >> }; >> /* >> diff --git a/lib/igt_kms.h b/lib/igt_kms.h >> index 2a480bf3..93e59dc7 100644 >> --- a/lib/igt_kms.h >> +++ b/lib/igt_kms.h >> @@ -118,6 +118,7 @@ enum igt_atomic_connector_properties { >> IGT_CONNECTOR_CRTC_ID, >> IGT_CONNECTOR_DPMS, >> IGT_CONNECTOR_BROADCAST_RGB, >> + IGT_CONNECTOR_CONTENT_PROTECTION, >> IGT_NUM_CONNECTOR_PROPS >> }; >> diff --git a/tests/Makefile.sources b/tests/Makefile.sources >> index 34ca71a0..e0466411 100644 >> --- a/tests/Makefile.sources >> +++ b/tests/Makefile.sources >> @@ -179,6 +179,7 @@ TESTS_progs = \ >> kms_chv_cursor_fail \ >> kms_color \ >> kms_concurrent \ >> + kms_content_protection\ >> kms_crtc_background_color \ >> kms_cursor_crc \ >> kms_cursor_legacy \ >> diff --git a/tests/kms_content_protection.c >> b/tests/kms_content_protection.c >> new file mode 100644 >> index 00000000..5d61b257 >> --- /dev/null >> +++ b/tests/kms_content_protection.c >> @@ -0,0 +1,129 @@ >> +/* >> + * Copyright © 2017 Google, Inc. >> + * >> + * Permission is hereby granted, free of charge, to any person >> obtaining a >> + * copy of this software and associated documentation files (the >> "Software"), >> + * to deal in the Software without restriction, including without >> limitation >> + * the rights to use, copy, modify, merge, publish, distribute, >> sublicense, >> + * and/or sell copies of the Software, and to permit persons to whom >> the >> + * Software is furnished to do so, subject to the following conditions: >> + * >> + * The above copyright notice and this permission notice (including >> the next >> + * paragraph) shall be included in all copies or substantial >> portions of the >> + * Software. >> + * >> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, >> EXPRESS OR >> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF >> MERCHANTABILITY, >> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO >> EVENT SHALL >> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES >> OR OTHER >> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, >> ARISING >> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR >> OTHER DEALINGS >> + * IN THE SOFTWARE. >> + * >> + */ >> + >> +#include "igt.h" >> + >> +IGT_TEST_DESCRIPTION("Test content protection (HDCP)"); >> + >> +typedef struct { >> + int drm_fd; >> + igt_display_t display; >> +} data_t; >> + >> +static bool >> +wait_for_prop_value(igt_output_t *output, uint64_t expected) >> +{ >> + uint64_t val; >> + int i; >> + >> + for (i = 0; i < 2000; i++) { > we need 5+Sec to complete the Second part of authentication, in case > of repeater with max downstream topology. > So we might need to wait for 6000(6Sec) loops!? >> + val = igt_output_get_prop(output, >> + IGT_CONNECTOR_CONTENT_PROTECTION); >> + if (val == expected) >> + return true; >> + usleep(1000); >> + } >> + igt_info("prop_value mismatch %ld != %ld\n", val, expected); >> + return false; >> +} >> + >> +static void >> +test_pipe_output(igt_display_t *display, const enum pipe pipe, >> + igt_output_t *output, enum igt_commit_style s) >> +{ >> + drmModeModeInfo mode; >> + igt_plane_t *primary; >> + struct igt_fb red; >> + bool ret; >> + >> + igt_assert(kmstest_get_connector_default_mode( >> + display->drm_fd, output->config.connector, &mode)); >> + >> + igt_output_override_mode(output, &mode); >> + igt_output_set_pipe(output, pipe); >> + >> + igt_create_color_fb(display->drm_fd, mode.hdisplay, mode.vdisplay, >> + DRM_FORMAT_XRGB8888, LOCAL_DRM_FORMAT_MOD_NONE, >> + 1.f, 0.f, 0.f, &red); >> + primary = igt_output_get_plane_type(output, >> DRM_PLANE_TYPE_PRIMARY); >> + igt_plane_set_fb(primary, &red); >> + igt_display_commit2(display, s); >> + >> + igt_output_set_prop_value(output, >> IGT_CONNECTOR_CONTENT_PROTECTION, 0); >> + igt_display_commit2(display, s); >> + >> + ret = wait_for_prop_value(output, 0); >> + igt_require_f(ret, "Content Protection not cleared\n"); > When expected property state is not met, you might want to use > igt_assert_f to fail the subtest. Here you are just skipping the subtest. >> + >> + igt_output_set_prop_value(output, >> IGT_CONNECTOR_CONTENT_PROTECTION, 1); >> + igt_display_commit2(display, s); >> + >> + ret = wait_for_prop_value(output, 2); As there are range of HDCP sinks which might fail to provide the R0 in 100mSec in the first attempt. But passes the condition in next attempt. In the testing it is observed that many panel's widely used showcase above mentioned behavior. So we need to retry on HDCP enable before declaring the HDCP failure. >> + igt_require_f(ret, "Content Protection not enabled\n"); > same as above. igt_assert_f needed? > > And when you are done with a connector(output), we cant leave the > content protection in desired state. > Else when we enable the connector for some other IGT, content > protection authentication will be attempted by kernel. > So when we are done with a connector, we need to clear the content > protection state(set to OFF) of the connector. > >> + >> + igt_plane_set_fb(primary, NULL); >> + igt_output_set_pipe(output, PIPE_NONE); >> +} >> + >> +static void >> +test_content_protection(igt_display_t *display, enum >> igt_commit_style s) >> +{ >> + igt_output_t *output; >> + enum pipe pipe; >> + int valid_tests = 0; >> + >> + for_each_pipe_with_valid_output(display, pipe, output) { From the testing of this IGT with kernel changes, observed a need of relooking at HDCP subtests definition. Reasoning is as follows. And here we are taking each pipe and their compatible outputs(connectors) for hdcp test. Like PIPEA + HDMI-A-1, PIPEB+HDMI-A-1 etc. But as HDCP is associated to connectors, we need to iterate on hdcp capable connector and choose the compatible pipe. In that way, we will be able test the HDCP on all hdcp capable connectors. And since irrespective of the hdcp status (pass/fail) on a connector, we need to test hdcp on other connectors one after another. So this can be achieved if the test of HDCP on each connector is implemented as different IGT subtests. >> + if (!output->props[IGT_CONNECTOR_CONTENT_PROTECTION]) >> + continue; >> + >> + test_pipe_output(display, pipe, output, s); >> + valid_tests++; >> + } >> + igt_require_f(valid_tests, "no support for content proteciton >> found\n"); >> +} >> + >> +igt_main >> +{ >> + data_t data = {}; >> + >> + igt_fixture { >> + igt_skip_on_simulation(); >> + >> + data.drm_fd = drm_open_driver(DRIVER_ANY); > You need Master privilege here. Hence might want to use > drm_open_driver_master() instead. > > -Ram >> + >> + igt_display_init(&data.display, data.drm_fd); >> + } >> + >> + >> + igt_subtest("legacy") >> + test_content_protection(&data.display, COMMIT_LEGACY); As discussed above, we need to consider connector-x-legacy as a subtest. >> + >> + igt_subtest("atomic") { >> + igt_require(data.display.is_atomic); >> + test_content_protection(&data.display, COMMIT_ATOMIC); As discussed above, we need to consider connector-x-atomic as a subtest. -Ram >> + } >> + >> + igt_fixture >> + igt_display_fini(&data.display); >> +} >> diff --git a/tests/meson.build b/tests/meson.build >> index 59ccd9a6..b12c35c0 100644 >> --- a/tests/meson.build >> +++ b/tests/meson.build >> @@ -157,6 +157,7 @@ test_progs = [ >> 'kms_chv_cursor_fail', >> 'kms_color', >> 'kms_concurrent', >> + 'kms_content_protection', >> 'kms_crtc_background_color', >> 'kms_cursor_crc', >> 'kms_cursor_legacy', > _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/2] kms_content_protection: Add Content Protection test 2017-12-19 14:28 ` Ramalingam C @ 2018-01-05 19:29 ` Sean Paul 2018-01-06 10:44 ` Ramalingam C 0 siblings, 1 reply; 12+ messages in thread From: Sean Paul @ 2018-01-05 19:29 UTC (permalink / raw) To: Ramalingam C; +Cc: Intel Graphics Development On Tue, Dec 19, 2017 at 9:28 AM, Ramalingam C <ramalingam.c@intel.com> wrote: > Adding few more findings from the IGT usage with kernel solutions. > > > > On Wednesday 13 December 2017 04:07 PM, Ramalingam C wrote: >> >> Sean, >> >> Adding few more observations. >> >> >> On Thursday 07 December 2017 05:47 AM, Sean Paul wrote: >>> >>> Pretty simple test: >>> - initializes the output >>> - clears the content protection property >>> - verifies that it clears >>> - sets the content protection property to desired >>> - verifies that it transitions to enabled >>> >>> Does this for both legacy and atomic. >>> >>> Changes in v2: >>> - Don't check for i915 gen >>> - Skip test if Content Protection property is absent >>> >>> Signed-off-by: Sean Paul <seanpaul@chromium.org> >>> --- >>> lib/igt_kms.c | 3 +- >>> lib/igt_kms.h | 1 + >>> tests/Makefile.sources | 1 + >>> tests/kms_content_protection.c | 129 >>> +++++++++++++++++++++++++++++++++++++++++ >>> tests/meson.build | 1 + >>> 5 files changed, 134 insertions(+), 1 deletion(-) >>> create mode 100644 tests/kms_content_protection.c >>> >>> diff --git a/lib/igt_kms.c b/lib/igt_kms.c >>> index 125ecb19..907db694 100644 >>> --- a/lib/igt_kms.c >>> +++ b/lib/igt_kms.c >>> @@ -190,7 +190,8 @@ const char >>> *igt_connector_prop_names[IGT_NUM_CONNECTOR_PROPS] = { >>> "scaling mode", >>> "CRTC_ID", >>> "DPMS", >>> - "Broadcast RGB" >>> + "Broadcast RGB", >>> + "Content Protection" >>> }; >>> /* >>> diff --git a/lib/igt_kms.h b/lib/igt_kms.h >>> index 2a480bf3..93e59dc7 100644 >>> --- a/lib/igt_kms.h >>> +++ b/lib/igt_kms.h >>> @@ -118,6 +118,7 @@ enum igt_atomic_connector_properties { >>> IGT_CONNECTOR_CRTC_ID, >>> IGT_CONNECTOR_DPMS, >>> IGT_CONNECTOR_BROADCAST_RGB, >>> + IGT_CONNECTOR_CONTENT_PROTECTION, >>> IGT_NUM_CONNECTOR_PROPS >>> }; >>> diff --git a/tests/Makefile.sources b/tests/Makefile.sources >>> index 34ca71a0..e0466411 100644 >>> --- a/tests/Makefile.sources >>> +++ b/tests/Makefile.sources >>> @@ -179,6 +179,7 @@ TESTS_progs = \ >>> kms_chv_cursor_fail \ >>> kms_color \ >>> kms_concurrent \ >>> + kms_content_protection\ >>> kms_crtc_background_color \ >>> kms_cursor_crc \ >>> kms_cursor_legacy \ >>> diff --git a/tests/kms_content_protection.c >>> b/tests/kms_content_protection.c >>> new file mode 100644 >>> index 00000000..5d61b257 >>> --- /dev/null >>> +++ b/tests/kms_content_protection.c >>> @@ -0,0 +1,129 @@ >>> +/* >>> + * Copyright © 2017 Google, Inc. >>> + * >>> + * Permission is hereby granted, free of charge, to any person obtaining >>> a >>> + * copy of this software and associated documentation files (the >>> "Software"), >>> + * to deal in the Software without restriction, including without >>> limitation >>> + * the rights to use, copy, modify, merge, publish, distribute, >>> sublicense, >>> + * and/or sell copies of the Software, and to permit persons to whom the >>> + * Software is furnished to do so, subject to the following conditions: >>> + * >>> + * The above copyright notice and this permission notice (including the >>> next >>> + * paragraph) shall be included in all copies or substantial portions of >>> the >>> + * Software. >>> + * >>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, >>> EXPRESS OR >>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF >>> MERCHANTABILITY, >>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT >>> SHALL >>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR >>> OTHER >>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, >>> ARISING >>> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER >>> DEALINGS >>> + * IN THE SOFTWARE. >>> + * >>> + */ >>> + >>> +#include "igt.h" >>> + >>> +IGT_TEST_DESCRIPTION("Test content protection (HDCP)"); >>> + >>> +typedef struct { >>> + int drm_fd; >>> + igt_display_t display; >>> +} data_t; >>> + >>> +static bool >>> +wait_for_prop_value(igt_output_t *output, uint64_t expected) >>> +{ >>> + uint64_t val; >>> + int i; >>> + >>> + for (i = 0; i < 2000; i++) { >> >> we need 5+Sec to complete the Second part of authentication, in case of >> repeater with max downstream topology. >> So we might need to wait for 6000(6Sec) loops!? >>> >>> + val = igt_output_get_prop(output, >>> + IGT_CONNECTOR_CONTENT_PROTECTION); >>> + if (val == expected) >>> + return true; >>> + usleep(1000); >>> + } >>> + igt_info("prop_value mismatch %ld != %ld\n", val, expected); >>> + return false; >>> +} >>> + >>> +static void >>> +test_pipe_output(igt_display_t *display, const enum pipe pipe, >>> + igt_output_t *output, enum igt_commit_style s) >>> +{ >>> + drmModeModeInfo mode; >>> + igt_plane_t *primary; >>> + struct igt_fb red; >>> + bool ret; >>> + >>> + igt_assert(kmstest_get_connector_default_mode( >>> + display->drm_fd, output->config.connector, &mode)); >>> + >>> + igt_output_override_mode(output, &mode); >>> + igt_output_set_pipe(output, pipe); >>> + >>> + igt_create_color_fb(display->drm_fd, mode.hdisplay, mode.vdisplay, >>> + DRM_FORMAT_XRGB8888, LOCAL_DRM_FORMAT_MOD_NONE, >>> + 1.f, 0.f, 0.f, &red); >>> + primary = igt_output_get_plane_type(output, DRM_PLANE_TYPE_PRIMARY); >>> + igt_plane_set_fb(primary, &red); >>> + igt_display_commit2(display, s); >>> + >>> + igt_output_set_prop_value(output, IGT_CONNECTOR_CONTENT_PROTECTION, >>> 0); >>> + igt_display_commit2(display, s); >>> + >>> + ret = wait_for_prop_value(output, 0); >>> + igt_require_f(ret, "Content Protection not cleared\n"); >> >> When expected property state is not met, you might want to use >> igt_assert_f to fail the subtest. Here you are just skipping the subtest. >>> >>> + >>> + igt_output_set_prop_value(output, IGT_CONNECTOR_CONTENT_PROTECTION, >>> 1); >>> + igt_display_commit2(display, s); >>> + >>> + ret = wait_for_prop_value(output, 2); > > > As there are range of HDCP sinks which might fail to provide the R0 in > 100mSec in the first attempt. But passes the condition in next attempt. > In the testing it is observed that many panel's widely used showcase above > mentioned behavior. > > So we need to retry on HDCP enable before declaring the HDCP failure. > Instead of retrying the whole thing, why not just make the 100ms timeout bigger? What values >100ms have you observed? >>> + igt_require_f(ret, "Content Protection not enabled\n"); >> >> same as above. igt_assert_f needed? >> >> And when you are done with a connector(output), we cant leave the content >> protection in desired state. >> Else when we enable the connector for some other IGT, content protection >> authentication will be attempted by kernel. >> So when we are done with a connector, we need to clear the content >> protection state(set to OFF) of the connector. >> >>> + >>> + igt_plane_set_fb(primary, NULL); >>> + igt_output_set_pipe(output, PIPE_NONE); >>> +} >>> + >>> +static void >>> +test_content_protection(igt_display_t *display, enum igt_commit_style s) >>> +{ >>> + igt_output_t *output; >>> + enum pipe pipe; >>> + int valid_tests = 0; >>> + >>> + for_each_pipe_with_valid_output(display, pipe, output) { > > From the testing of this IGT with kernel changes, observed a need of > relooking at HDCP subtests definition. Reasoning is as follows. > > And here we are taking each pipe and their compatible outputs(connectors) > for hdcp test. Like PIPEA + HDMI-A-1, PIPEB+HDMI-A-1 etc. > But as HDCP is associated to connectors, we need to iterate on hdcp capable > connector and choose the compatible pipe. > In that way, we will be able test the HDCP on all hdcp capable connectors. > I'm confused. The documentation for this loop says it tries every combination of connector+pipe for every connected connector. Can you explain what I'm missing? Sean > And since irrespective of the hdcp status (pass/fail) on a connector, we > need to test hdcp on other connectors one after another. > So this can be achieved if the test of HDCP on each connector is implemented > as different IGT subtests. > >>> + if (!output->props[IGT_CONNECTOR_CONTENT_PROTECTION]) >>> + continue; >>> + >>> + test_pipe_output(display, pipe, output, s); >>> + valid_tests++; >>> + } >>> + igt_require_f(valid_tests, "no support for content proteciton >>> found\n"); >>> +} >>> + >>> +igt_main >>> +{ >>> + data_t data = {}; >>> + >>> + igt_fixture { >>> + igt_skip_on_simulation(); >>> + >>> + data.drm_fd = drm_open_driver(DRIVER_ANY); >> >> You need Master privilege here. Hence might want to use >> drm_open_driver_master() instead. >> >> -Ram >>> >>> + >>> + igt_display_init(&data.display, data.drm_fd); >>> + } >>> + >>> + >>> + igt_subtest("legacy") >>> + test_content_protection(&data.display, COMMIT_LEGACY); > > As discussed above, we need to consider connector-x-legacy as a subtest. >>> >>> + >>> + igt_subtest("atomic") { >>> + igt_require(data.display.is_atomic); >>> + test_content_protection(&data.display, COMMIT_ATOMIC); > > As discussed above, we need to consider connector-x-atomic as a subtest. > > -Ram > >>> + } >>> + >>> + igt_fixture >>> + igt_display_fini(&data.display); >>> +} >>> diff --git a/tests/meson.build b/tests/meson.build >>> index 59ccd9a6..b12c35c0 100644 >>> --- a/tests/meson.build >>> +++ b/tests/meson.build >>> @@ -157,6 +157,7 @@ test_progs = [ >>> 'kms_chv_cursor_fail', >>> 'kms_color', >>> 'kms_concurrent', >>> + 'kms_content_protection', >>> 'kms_crtc_background_color', >>> 'kms_cursor_crc', >>> 'kms_cursor_legacy', >> >> > _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/2] kms_content_protection: Add Content Protection test 2018-01-05 19:29 ` Sean Paul @ 2018-01-06 10:44 ` Ramalingam C 2018-01-06 13:00 ` Sean Paul 0 siblings, 1 reply; 12+ messages in thread From: Ramalingam C @ 2018-01-06 10:44 UTC (permalink / raw) To: Sean Paul; +Cc: Intel Graphics Development On Saturday 06 January 2018 12:59 AM, Sean Paul wrote: > On Tue, Dec 19, 2017 at 9:28 AM, Ramalingam C <ramalingam.c@intel.com> wrote: >> Adding few more findings from the IGT usage with kernel solutions. >> >> >> >> On Wednesday 13 December 2017 04:07 PM, Ramalingam C wrote: >>> Sean, >>> >>> Adding few more observations. >>> >>> >>> On Thursday 07 December 2017 05:47 AM, Sean Paul wrote: >>>> Pretty simple test: >>>> - initializes the output >>>> - clears the content protection property >>>> - verifies that it clears >>>> - sets the content protection property to desired >>>> - verifies that it transitions to enabled >>>> >>>> Does this for both legacy and atomic. >>>> >>>> Changes in v2: >>>> - Don't check for i915 gen >>>> - Skip test if Content Protection property is absent >>>> >>>> Signed-off-by: Sean Paul <seanpaul@chromium.org> >>>> --- >>>> lib/igt_kms.c | 3 +- >>>> lib/igt_kms.h | 1 + >>>> tests/Makefile.sources | 1 + >>>> tests/kms_content_protection.c | 129 >>>> +++++++++++++++++++++++++++++++++++++++++ >>>> tests/meson.build | 1 + >>>> 5 files changed, 134 insertions(+), 1 deletion(-) >>>> create mode 100644 tests/kms_content_protection.c >>>> >>>> diff --git a/lib/igt_kms.c b/lib/igt_kms.c >>>> index 125ecb19..907db694 100644 >>>> --- a/lib/igt_kms.c >>>> +++ b/lib/igt_kms.c >>>> @@ -190,7 +190,8 @@ const char >>>> *igt_connector_prop_names[IGT_NUM_CONNECTOR_PROPS] = { >>>> "scaling mode", >>>> "CRTC_ID", >>>> "DPMS", >>>> - "Broadcast RGB" >>>> + "Broadcast RGB", >>>> + "Content Protection" >>>> }; >>>> /* >>>> diff --git a/lib/igt_kms.h b/lib/igt_kms.h >>>> index 2a480bf3..93e59dc7 100644 >>>> --- a/lib/igt_kms.h >>>> +++ b/lib/igt_kms.h >>>> @@ -118,6 +118,7 @@ enum igt_atomic_connector_properties { >>>> IGT_CONNECTOR_CRTC_ID, >>>> IGT_CONNECTOR_DPMS, >>>> IGT_CONNECTOR_BROADCAST_RGB, >>>> + IGT_CONNECTOR_CONTENT_PROTECTION, >>>> IGT_NUM_CONNECTOR_PROPS >>>> }; >>>> diff --git a/tests/Makefile.sources b/tests/Makefile.sources >>>> index 34ca71a0..e0466411 100644 >>>> --- a/tests/Makefile.sources >>>> +++ b/tests/Makefile.sources >>>> @@ -179,6 +179,7 @@ TESTS_progs = \ >>>> kms_chv_cursor_fail \ >>>> kms_color \ >>>> kms_concurrent \ >>>> + kms_content_protection\ >>>> kms_crtc_background_color \ >>>> kms_cursor_crc \ >>>> kms_cursor_legacy \ >>>> diff --git a/tests/kms_content_protection.c >>>> b/tests/kms_content_protection.c >>>> new file mode 100644 >>>> index 00000000..5d61b257 >>>> --- /dev/null >>>> +++ b/tests/kms_content_protection.c >>>> @@ -0,0 +1,129 @@ >>>> +/* >>>> + * Copyright © 2017 Google, Inc. >>>> + * >>>> + * Permission is hereby granted, free of charge, to any person obtaining >>>> a >>>> + * copy of this software and associated documentation files (the >>>> "Software"), >>>> + * to deal in the Software without restriction, including without >>>> limitation >>>> + * the rights to use, copy, modify, merge, publish, distribute, >>>> sublicense, >>>> + * and/or sell copies of the Software, and to permit persons to whom the >>>> + * Software is furnished to do so, subject to the following conditions: >>>> + * >>>> + * The above copyright notice and this permission notice (including the >>>> next >>>> + * paragraph) shall be included in all copies or substantial portions of >>>> the >>>> + * Software. >>>> + * >>>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, >>>> EXPRESS OR >>>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF >>>> MERCHANTABILITY, >>>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT >>>> SHALL >>>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR >>>> OTHER >>>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, >>>> ARISING >>>> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER >>>> DEALINGS >>>> + * IN THE SOFTWARE. >>>> + * >>>> + */ >>>> + >>>> +#include "igt.h" >>>> + >>>> +IGT_TEST_DESCRIPTION("Test content protection (HDCP)"); >>>> + >>>> +typedef struct { >>>> + int drm_fd; >>>> + igt_display_t display; >>>> +} data_t; >>>> + >>>> +static bool >>>> +wait_for_prop_value(igt_output_t *output, uint64_t expected) >>>> +{ >>>> + uint64_t val; >>>> + int i; >>>> + >>>> + for (i = 0; i < 2000; i++) { >>> we need 5+Sec to complete the Second part of authentication, in case of >>> repeater with max downstream topology. >>> So we might need to wait for 6000(6Sec) loops!? >>>> + val = igt_output_get_prop(output, >>>> + IGT_CONNECTOR_CONTENT_PROTECTION); >>>> + if (val == expected) >>>> + return true; >>>> + usleep(1000); >>>> + } >>>> + igt_info("prop_value mismatch %ld != %ld\n", val, expected); >>>> + return false; >>>> +} >>>> + >>>> +static void >>>> +test_pipe_output(igt_display_t *display, const enum pipe pipe, >>>> + igt_output_t *output, enum igt_commit_style s) >>>> +{ >>>> + drmModeModeInfo mode; >>>> + igt_plane_t *primary; >>>> + struct igt_fb red; >>>> + bool ret; >>>> + >>>> + igt_assert(kmstest_get_connector_default_mode( >>>> + display->drm_fd, output->config.connector, &mode)); >>>> + >>>> + igt_output_override_mode(output, &mode); >>>> + igt_output_set_pipe(output, pipe); >>>> + >>>> + igt_create_color_fb(display->drm_fd, mode.hdisplay, mode.vdisplay, >>>> + DRM_FORMAT_XRGB8888, LOCAL_DRM_FORMAT_MOD_NONE, >>>> + 1.f, 0.f, 0.f, &red); >>>> + primary = igt_output_get_plane_type(output, DRM_PLANE_TYPE_PRIMARY); >>>> + igt_plane_set_fb(primary, &red); >>>> + igt_display_commit2(display, s); >>>> + >>>> + igt_output_set_prop_value(output, IGT_CONNECTOR_CONTENT_PROTECTION, >>>> 0); >>>> + igt_display_commit2(display, s); >>>> + >>>> + ret = wait_for_prop_value(output, 0); >>>> + igt_require_f(ret, "Content Protection not cleared\n"); >>> When expected property state is not met, you might want to use >>> igt_assert_f to fail the subtest. Here you are just skipping the subtest. >>>> + >>>> + igt_output_set_prop_value(output, IGT_CONNECTOR_CONTENT_PROTECTION, >>>> 1); >>>> + igt_display_commit2(display, s); >>>> + >>>> + ret = wait_for_prop_value(output, 2); >> >> As there are range of HDCP sinks which might fail to provide the R0 in >> 100mSec in the first attempt. But passes the condition in next attempt. >> In the testing it is observed that many panel's widely used showcase above >> mentioned behavior. >> >> So we need to retry on HDCP enable before declaring the HDCP failure. >> > Instead of retrying the whole thing, why not just make the 100ms > timeout bigger? What values >100ms have you observed? I dont think we can increase the timeout beyond mentioned 100mSec. If we do so it will be non compliance with HDCP spec hence CTS will fail. IMO Retrying will be the best option left. > >>>> + igt_require_f(ret, "Content Protection not enabled\n"); >>> same as above. igt_assert_f needed? >>> >>> And when you are done with a connector(output), we cant leave the content >>> protection in desired state. >>> Else when we enable the connector for some other IGT, content protection >>> authentication will be attempted by kernel. >>> So when we are done with a connector, we need to clear the content >>> protection state(set to OFF) of the connector. >>> >>>> + >>>> + igt_plane_set_fb(primary, NULL); >>>> + igt_output_set_pipe(output, PIPE_NONE); >>>> +} >>>> + >>>> +static void >>>> +test_content_protection(igt_display_t *display, enum igt_commit_style s) >>>> +{ >>>> + igt_output_t *output; >>>> + enum pipe pipe; >>>> + int valid_tests = 0; >>>> + >>>> + for_each_pipe_with_valid_output(display, pipe, output) { >> From the testing of this IGT with kernel changes, observed a need of >> relooking at HDCP subtests definition. Reasoning is as follows. >> >> And here we are taking each pipe and their compatible outputs(connectors) >> for hdcp test. Like PIPEA + HDMI-A-1, PIPEB+HDMI-A-1 etc. >> But as HDCP is associated to connectors, we need to iterate on hdcp capable >> connector and choose the compatible pipe. >> In that way, we will be able test the HDCP on all hdcp capable connectors. >> > I'm confused. The documentation for this loop says it tries every > combination of connector+pipe for every connected connector. Can you > explain what I'm missing? > > Sean Yes. Macro takes each pipe and tests against all interface able outputs. I missed that point. Still it will be efficient to test hdcp on each output with only one compatible pipe. testing with other compatible pipes of the connector has no ROI w.r.t hdcp testing. Considering that macro provides super set of req pipe+output combination, no urgency to modify immediately. >> And since irrespective of the hdcp status (pass/fail) on a connector, we >> need to test hdcp on other connectors one after another. >> So this can be achieved if the test of HDCP on each connector is implemented >> as different IGT subtests. >> >>>> + if (!output->props[IGT_CONNECTOR_CONTENT_PROTECTION]) >>>> + continue; >>>> + >>>> + test_pipe_output(display, pipe, output, s); >>>> + valid_tests++; >>>> + } >>>> + igt_require_f(valid_tests, "no support for content proteciton >>>> found\n"); >>>> +} >>>> + >>>> +igt_main >>>> +{ >>>> + data_t data = {}; >>>> + >>>> + igt_fixture { >>>> + igt_skip_on_simulation(); >>>> + >>>> + data.drm_fd = drm_open_driver(DRIVER_ANY); >>> You need Master privilege here. Hence might want to use >>> drm_open_driver_master() instead. >>> >>> -Ram >>>> + >>>> + igt_display_init(&data.display, data.drm_fd); >>>> + } >>>> + >>>> + >>>> + igt_subtest("legacy") >>>> + test_content_protection(&data.display, COMMIT_LEGACY); >> As discussed above, we need to consider connector-x-legacy as a subtest. >>>> + >>>> + igt_subtest("atomic") { >>>> + igt_require(data.display.is_atomic); >>>> + test_content_protection(&data.display, COMMIT_ATOMIC); >> As discussed above, we need to consider connector-x-atomic as a subtest. For internal testing I have sub tests defined as below. Please see if it helps. Might need some tuning though. igt_main { data_t data = {}; + igt_output_t *output; + enum pipe pipe; + char test_name[50]; + int i; igt_fixture { igt_skip_on_simulation(); @@ -122,13 +109,44 @@ igt_main igt_display_init(&data.display, data.drm_fd); } + /* + * Not using the for_each_connected_output as that mandates + * igt_can_fail(). + */ + for (i = 0; i < data.display.n_outputs; i++) + for_each_if((output = &(data.display.outputs[i]), + igt_output_is_connected(output))) { + if (!output->props[IGT_CONNECTOR_CONTENT_PROTECTION]) + continue; - igt_subtest("legacy") - test_content_protection(&data.display, COMMIT_LEGACY); - - igt_subtest("atomic") { - igt_require(data.display.is_atomic); - test_content_protection(&data.display, COMMIT_ATOMIC); + for_each_pipe_static(pipe) { + if (!igt_pipe_connector_valid(pipe, output)) + continue; + + sprintf(test_name, "HDCP_%s_PIPE-%s_legacy", + output->name, + kmstest_pipe_name(pipe)); + igt_subtest(test_name) + test_content_protection(&data.display, pipe, + output, COMMIT_LEGACY); + + if (!data.display.is_atomic) + break; + + sprintf(test_name, "HDCP_%s_PIPE-%s_atomic", + output->name, + kmstest_pipe_name(pipe)); + igt_subtest(test_name) + test_content_protection(&data.display, pipe, + output, COMMIT_ATOMIC); + + /* + * Testing a output with a pipe is enough for HDCP + * testing. No ROI in testing the connector with other + * pipes. So Break the loop on pipe. + */ + break; + } } igt_fixture >> >> -Ram >> >>>> + } >>>> + >>>> + igt_fixture >>>> + igt_display_fini(&data.display); >>>> +} >>>> diff --git a/tests/meson.build b/tests/meson.build >>>> index 59ccd9a6..b12c35c0 100644 >>>> --- a/tests/meson.build >>>> +++ b/tests/meson.build >>>> @@ -157,6 +157,7 @@ test_progs = [ >>>> 'kms_chv_cursor_fail', >>>> 'kms_color', >>>> 'kms_concurrent', >>>> + 'kms_content_protection', >>>> 'kms_crtc_background_color', >>>> 'kms_cursor_crc', >>>> 'kms_cursor_legacy', >>> _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/2] kms_content_protection: Add Content Protection test 2018-01-06 10:44 ` Ramalingam C @ 2018-01-06 13:00 ` Sean Paul 2018-01-06 13:10 ` Ramalingam C 0 siblings, 1 reply; 12+ messages in thread From: Sean Paul @ 2018-01-06 13:00 UTC (permalink / raw) To: Ramalingam C; +Cc: Intel Graphics Development On Sat, Jan 6, 2018 at 5:44 AM, Ramalingam C <ramalingam.c@intel.com> wrote: > > On Saturday 06 January 2018 12:59 AM, Sean Paul wrote: >> >> On Tue, Dec 19, 2017 at 9:28 AM, Ramalingam C <ramalingam.c@intel.com> >> wrote: >>> >>> Adding few more findings from the IGT usage with kernel solutions. >>> >>> >>> >>> On Wednesday 13 December 2017 04:07 PM, Ramalingam C wrote: >>>> >>>> Sean, >>>> >>>> Adding few more observations. >>>> >>>> >>>> On Thursday 07 December 2017 05:47 AM, Sean Paul wrote: >>>>> >>>>> Pretty simple test: >>>>> - initializes the output >>>>> - clears the content protection property >>>>> - verifies that it clears >>>>> - sets the content protection property to desired >>>>> - verifies that it transitions to enabled >>>>> >>>>> Does this for both legacy and atomic. >>>>> >>>>> Changes in v2: >>>>> - Don't check for i915 gen >>>>> - Skip test if Content Protection property is absent >>>>> >>>>> Signed-off-by: Sean Paul <seanpaul@chromium.org> >>>>> --- >>>>> lib/igt_kms.c | 3 +- >>>>> lib/igt_kms.h | 1 + >>>>> tests/Makefile.sources | 1 + >>>>> tests/kms_content_protection.c | 129 >>>>> +++++++++++++++++++++++++++++++++++++++++ >>>>> tests/meson.build | 1 + >>>>> 5 files changed, 134 insertions(+), 1 deletion(-) >>>>> create mode 100644 tests/kms_content_protection.c >>>>> >>>>> diff --git a/lib/igt_kms.c b/lib/igt_kms.c >>>>> index 125ecb19..907db694 100644 >>>>> --- a/lib/igt_kms.c >>>>> +++ b/lib/igt_kms.c >>>>> @@ -190,7 +190,8 @@ const char >>>>> *igt_connector_prop_names[IGT_NUM_CONNECTOR_PROPS] = { >>>>> "scaling mode", >>>>> "CRTC_ID", >>>>> "DPMS", >>>>> - "Broadcast RGB" >>>>> + "Broadcast RGB", >>>>> + "Content Protection" >>>>> }; >>>>> /* >>>>> diff --git a/lib/igt_kms.h b/lib/igt_kms.h >>>>> index 2a480bf3..93e59dc7 100644 >>>>> --- a/lib/igt_kms.h >>>>> +++ b/lib/igt_kms.h >>>>> @@ -118,6 +118,7 @@ enum igt_atomic_connector_properties { >>>>> IGT_CONNECTOR_CRTC_ID, >>>>> IGT_CONNECTOR_DPMS, >>>>> IGT_CONNECTOR_BROADCAST_RGB, >>>>> + IGT_CONNECTOR_CONTENT_PROTECTION, >>>>> IGT_NUM_CONNECTOR_PROPS >>>>> }; >>>>> diff --git a/tests/Makefile.sources b/tests/Makefile.sources >>>>> index 34ca71a0..e0466411 100644 >>>>> --- a/tests/Makefile.sources >>>>> +++ b/tests/Makefile.sources >>>>> @@ -179,6 +179,7 @@ TESTS_progs = \ >>>>> kms_chv_cursor_fail \ >>>>> kms_color \ >>>>> kms_concurrent \ >>>>> + kms_content_protection\ >>>>> kms_crtc_background_color \ >>>>> kms_cursor_crc \ >>>>> kms_cursor_legacy \ >>>>> diff --git a/tests/kms_content_protection.c >>>>> b/tests/kms_content_protection.c >>>>> new file mode 100644 >>>>> index 00000000..5d61b257 >>>>> --- /dev/null >>>>> +++ b/tests/kms_content_protection.c >>>>> @@ -0,0 +1,129 @@ >>>>> +/* >>>>> + * Copyright © 2017 Google, Inc. >>>>> + * >>>>> + * Permission is hereby granted, free of charge, to any person >>>>> obtaining >>>>> a >>>>> + * copy of this software and associated documentation files (the >>>>> "Software"), >>>>> + * to deal in the Software without restriction, including without >>>>> limitation >>>>> + * the rights to use, copy, modify, merge, publish, distribute, >>>>> sublicense, >>>>> + * and/or sell copies of the Software, and to permit persons to whom >>>>> the >>>>> + * Software is furnished to do so, subject to the following >>>>> conditions: >>>>> + * >>>>> + * The above copyright notice and this permission notice (including >>>>> the >>>>> next >>>>> + * paragraph) shall be included in all copies or substantial portions >>>>> of >>>>> the >>>>> + * Software. >>>>> + * >>>>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, >>>>> EXPRESS OR >>>>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF >>>>> MERCHANTABILITY, >>>>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT >>>>> SHALL >>>>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES >>>>> OR >>>>> OTHER >>>>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, >>>>> ARISING >>>>> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER >>>>> DEALINGS >>>>> + * IN THE SOFTWARE. >>>>> + * >>>>> + */ >>>>> + >>>>> +#include "igt.h" >>>>> + >>>>> +IGT_TEST_DESCRIPTION("Test content protection (HDCP)"); >>>>> + >>>>> +typedef struct { >>>>> + int drm_fd; >>>>> + igt_display_t display; >>>>> +} data_t; >>>>> + >>>>> +static bool >>>>> +wait_for_prop_value(igt_output_t *output, uint64_t expected) >>>>> +{ >>>>> + uint64_t val; >>>>> + int i; >>>>> + >>>>> + for (i = 0; i < 2000; i++) { >>>> >>>> we need 5+Sec to complete the Second part of authentication, in case of >>>> repeater with max downstream topology. >>>> So we might need to wait for 6000(6Sec) loops!? >>>>> >>>>> + val = igt_output_get_prop(output, >>>>> + IGT_CONNECTOR_CONTENT_PROTECTION); >>>>> + if (val == expected) >>>>> + return true; >>>>> + usleep(1000); >>>>> + } >>>>> + igt_info("prop_value mismatch %ld != %ld\n", val, expected); >>>>> + return false; >>>>> +} >>>>> + >>>>> +static void >>>>> +test_pipe_output(igt_display_t *display, const enum pipe pipe, >>>>> + igt_output_t *output, enum igt_commit_style s) >>>>> +{ >>>>> + drmModeModeInfo mode; >>>>> + igt_plane_t *primary; >>>>> + struct igt_fb red; >>>>> + bool ret; >>>>> + >>>>> + igt_assert(kmstest_get_connector_default_mode( >>>>> + display->drm_fd, output->config.connector, &mode)); >>>>> + >>>>> + igt_output_override_mode(output, &mode); >>>>> + igt_output_set_pipe(output, pipe); >>>>> + >>>>> + igt_create_color_fb(display->drm_fd, mode.hdisplay, mode.vdisplay, >>>>> + DRM_FORMAT_XRGB8888, LOCAL_DRM_FORMAT_MOD_NONE, >>>>> + 1.f, 0.f, 0.f, &red); >>>>> + primary = igt_output_get_plane_type(output, >>>>> DRM_PLANE_TYPE_PRIMARY); >>>>> + igt_plane_set_fb(primary, &red); >>>>> + igt_display_commit2(display, s); >>>>> + >>>>> + igt_output_set_prop_value(output, >>>>> IGT_CONNECTOR_CONTENT_PROTECTION, >>>>> 0); >>>>> + igt_display_commit2(display, s); >>>>> + >>>>> + ret = wait_for_prop_value(output, 0); >>>>> + igt_require_f(ret, "Content Protection not cleared\n"); >>>> >>>> When expected property state is not met, you might want to use >>>> igt_assert_f to fail the subtest. Here you are just skipping the >>>> subtest. >>>>> >>>>> + >>>>> + igt_output_set_prop_value(output, >>>>> IGT_CONNECTOR_CONTENT_PROTECTION, >>>>> 1); >>>>> + igt_display_commit2(display, s); >>>>> + >>>>> + ret = wait_for_prop_value(output, 2); >>> >>> >>> As there are range of HDCP sinks which might fail to provide the R0 in >>> 100mSec in the first attempt. But passes the condition in next attempt. >>> In the testing it is observed that many panel's widely used showcase >>> above >>> mentioned behavior. >>> >>> So we need to retry on HDCP enable before declaring the HDCP failure. >>> >> Instead of retrying the whole thing, why not just make the 100ms >> timeout bigger? What values >100ms have you observed? > > I dont think we can increase the timeout beyond mentioned 100mSec. If we do > so it will be non compliance with HDCP spec hence CTS will fail. What's the difference to userspace? Isn't stringing together multiple 100ms timeouts equivalent as one longer timeout? > IMO Retrying will be the best option left. > >> >>>>> + igt_require_f(ret, "Content Protection not enabled\n"); >>>> >>>> same as above. igt_assert_f needed? >>>> >>>> And when you are done with a connector(output), we cant leave the >>>> content >>>> protection in desired state. >>>> Else when we enable the connector for some other IGT, content protection >>>> authentication will be attempted by kernel. >>>> So when we are done with a connector, we need to clear the content >>>> protection state(set to OFF) of the connector. >>>> >>>>> + >>>>> + igt_plane_set_fb(primary, NULL); >>>>> + igt_output_set_pipe(output, PIPE_NONE); >>>>> +} >>>>> + >>>>> +static void >>>>> +test_content_protection(igt_display_t *display, enum igt_commit_style >>>>> s) >>>>> +{ >>>>> + igt_output_t *output; >>>>> + enum pipe pipe; >>>>> + int valid_tests = 0; >>>>> + >>>>> + for_each_pipe_with_valid_output(display, pipe, output) { >>> >>> From the testing of this IGT with kernel changes, observed a need of >>> relooking at HDCP subtests definition. Reasoning is as follows. >>> >>> And here we are taking each pipe and their compatible outputs(connectors) >>> for hdcp test. Like PIPEA + HDMI-A-1, PIPEB+HDMI-A-1 etc. >>> But as HDCP is associated to connectors, we need to iterate on hdcp >>> capable >>> connector and choose the compatible pipe. >>> In that way, we will be able test the HDCP on all hdcp capable >>> connectors. >>> >> I'm confused. The documentation for this loop says it tries every >> combination of connector+pipe for every connected connector. Can you >> explain what I'm missing? >> >> Sean > > Yes. Macro takes each pipe and tests against all interface able outputs. I > missed that point. > Still it will be efficient to test hdcp on each output with only one > compatible pipe. > testing with other compatible pipes of the connector has no ROI w.r.t hdcp > testing. > > Considering that macro provides super set of req pipe+output combination, no > urgency to modify immediately. > Ok, great! I'm relieved I was not going crazy :) Sean >>> And since irrespective of the hdcp status (pass/fail) on a connector, we >>> need to test hdcp on other connectors one after another. >>> So this can be achieved if the test of HDCP on each connector is >>> implemented >>> as different IGT subtests. >>> >>>>> + if (!output->props[IGT_CONNECTOR_CONTENT_PROTECTION]) >>>>> + continue; >>>>> + >>>>> + test_pipe_output(display, pipe, output, s); >>>>> + valid_tests++; >>>>> + } >>>>> + igt_require_f(valid_tests, "no support for content proteciton >>>>> found\n"); >>>>> +} >>>>> + >>>>> +igt_main >>>>> +{ >>>>> + data_t data = {}; >>>>> + >>>>> + igt_fixture { >>>>> + igt_skip_on_simulation(); >>>>> + >>>>> + data.drm_fd = drm_open_driver(DRIVER_ANY); >>>> >>>> You need Master privilege here. Hence might want to use >>>> drm_open_driver_master() instead. >>>> >>>> -Ram >>>>> >>>>> + >>>>> + igt_display_init(&data.display, data.drm_fd); >>>>> + } >>>>> + >>>>> + >>>>> + igt_subtest("legacy") >>>>> + test_content_protection(&data.display, COMMIT_LEGACY); >>> >>> As discussed above, we need to consider connector-x-legacy as a subtest. >>>>> >>>>> + >>>>> + igt_subtest("atomic") { >>>>> + igt_require(data.display.is_atomic); >>>>> + test_content_protection(&data.display, COMMIT_ATOMIC); >>> >>> As discussed above, we need to consider connector-x-atomic as a subtest. > > For internal testing I have sub tests defined as below. Please see if it > helps. > Might need some tuning though. > > igt_main > { > data_t data = {}; > + igt_output_t *output; > + enum pipe pipe; > + char test_name[50]; > + int i; > > igt_fixture { > igt_skip_on_simulation(); > @@ -122,13 +109,44 @@ igt_main > igt_display_init(&data.display, data.drm_fd); > } > > + /* > + * Not using the for_each_connected_output as that mandates > + * igt_can_fail(). > + */ > + for (i = 0; i < data.display.n_outputs; i++) > + for_each_if((output = &(data.display.outputs[i]), > + igt_output_is_connected(output))) { > + if (!output->props[IGT_CONNECTOR_CONTENT_PROTECTION]) > + continue; > > - igt_subtest("legacy") > - test_content_protection(&data.display, COMMIT_LEGACY); > - > - igt_subtest("atomic") { > - igt_require(data.display.is_atomic); > - test_content_protection(&data.display, COMMIT_ATOMIC); > + for_each_pipe_static(pipe) { > + if (!igt_pipe_connector_valid(pipe, output)) > + continue; > + > + sprintf(test_name, "HDCP_%s_PIPE-%s_legacy", > + output->name, > + kmstest_pipe_name(pipe)); > + igt_subtest(test_name) > + test_content_protection(&data.display, pipe, > + output, COMMIT_LEGACY); > + > + if (!data.display.is_atomic) > + break; > + > + sprintf(test_name, "HDCP_%s_PIPE-%s_atomic", > + output->name, > + kmstest_pipe_name(pipe)); > + igt_subtest(test_name) > + test_content_protection(&data.display, pipe, > + output, COMMIT_ATOMIC); > + > + /* > + * Testing a output with a pipe is enough for HDCP > + * testing. No ROI in testing the connector with > other > + * pipes. So Break the loop on pipe. > + */ > + break; > + } > } > > igt_fixture > > > >>> >>> -Ram >>> >>>>> + } >>>>> + >>>>> + igt_fixture >>>>> + igt_display_fini(&data.display); >>>>> +} >>>>> diff --git a/tests/meson.build b/tests/meson.build >>>>> index 59ccd9a6..b12c35c0 100644 >>>>> --- a/tests/meson.build >>>>> +++ b/tests/meson.build >>>>> @@ -157,6 +157,7 @@ test_progs = [ >>>>> 'kms_chv_cursor_fail', >>>>> 'kms_color', >>>>> 'kms_concurrent', >>>>> + 'kms_content_protection', >>>>> 'kms_crtc_background_color', >>>>> 'kms_cursor_crc', >>>>> 'kms_cursor_legacy', >>>> >>>> > _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/2] kms_content_protection: Add Content Protection test 2018-01-06 13:00 ` Sean Paul @ 2018-01-06 13:10 ` Ramalingam C 2018-01-06 15:04 ` Sean Paul 0 siblings, 1 reply; 12+ messages in thread From: Ramalingam C @ 2018-01-06 13:10 UTC (permalink / raw) To: Sean Paul; +Cc: Intel Graphics Development On Saturday 06 January 2018 06:30 PM, Sean Paul wrote: > On Sat, Jan 6, 2018 at 5:44 AM, Ramalingam C <ramalingam.c@intel.com> wrote: >> On Saturday 06 January 2018 12:59 AM, Sean Paul wrote: >>> On Tue, Dec 19, 2017 at 9:28 AM, Ramalingam C <ramalingam.c@intel.com> >>> wrote: >>>> Adding few more findings from the IGT usage with kernel solutions. >>>> >>>> >>>> >>>> On Wednesday 13 December 2017 04:07 PM, Ramalingam C wrote: >>>>> Sean, >>>>> >>>>> Adding few more observations. >>>>> >>>>> >>>>> On Thursday 07 December 2017 05:47 AM, Sean Paul wrote: >>>>>> Pretty simple test: >>>>>> - initializes the output >>>>>> - clears the content protection property >>>>>> - verifies that it clears >>>>>> - sets the content protection property to desired >>>>>> - verifies that it transitions to enabled >>>>>> >>>>>> Does this for both legacy and atomic. >>>>>> >>>>>> Changes in v2: >>>>>> - Don't check for i915 gen >>>>>> - Skip test if Content Protection property is absent >>>>>> >>>>>> Signed-off-by: Sean Paul <seanpaul@chromium.org> >>>>>> --- >>>>>> lib/igt_kms.c | 3 +- >>>>>> lib/igt_kms.h | 1 + >>>>>> tests/Makefile.sources | 1 + >>>>>> tests/kms_content_protection.c | 129 >>>>>> +++++++++++++++++++++++++++++++++++++++++ >>>>>> tests/meson.build | 1 + >>>>>> 5 files changed, 134 insertions(+), 1 deletion(-) >>>>>> create mode 100644 tests/kms_content_protection.c >>>>>> >>>>>> diff --git a/lib/igt_kms.c b/lib/igt_kms.c >>>>>> index 125ecb19..907db694 100644 >>>>>> --- a/lib/igt_kms.c >>>>>> +++ b/lib/igt_kms.c >>>>>> @@ -190,7 +190,8 @@ const char >>>>>> *igt_connector_prop_names[IGT_NUM_CONNECTOR_PROPS] = { >>>>>> "scaling mode", >>>>>> "CRTC_ID", >>>>>> "DPMS", >>>>>> - "Broadcast RGB" >>>>>> + "Broadcast RGB", >>>>>> + "Content Protection" >>>>>> }; >>>>>> /* >>>>>> diff --git a/lib/igt_kms.h b/lib/igt_kms.h >>>>>> index 2a480bf3..93e59dc7 100644 >>>>>> --- a/lib/igt_kms.h >>>>>> +++ b/lib/igt_kms.h >>>>>> @@ -118,6 +118,7 @@ enum igt_atomic_connector_properties { >>>>>> IGT_CONNECTOR_CRTC_ID, >>>>>> IGT_CONNECTOR_DPMS, >>>>>> IGT_CONNECTOR_BROADCAST_RGB, >>>>>> + IGT_CONNECTOR_CONTENT_PROTECTION, >>>>>> IGT_NUM_CONNECTOR_PROPS >>>>>> }; >>>>>> diff --git a/tests/Makefile.sources b/tests/Makefile.sources >>>>>> index 34ca71a0..e0466411 100644 >>>>>> --- a/tests/Makefile.sources >>>>>> +++ b/tests/Makefile.sources >>>>>> @@ -179,6 +179,7 @@ TESTS_progs = \ >>>>>> kms_chv_cursor_fail \ >>>>>> kms_color \ >>>>>> kms_concurrent \ >>>>>> + kms_content_protection\ >>>>>> kms_crtc_background_color \ >>>>>> kms_cursor_crc \ >>>>>> kms_cursor_legacy \ >>>>>> diff --git a/tests/kms_content_protection.c >>>>>> b/tests/kms_content_protection.c >>>>>> new file mode 100644 >>>>>> index 00000000..5d61b257 >>>>>> --- /dev/null >>>>>> +++ b/tests/kms_content_protection.c >>>>>> @@ -0,0 +1,129 @@ >>>>>> +/* >>>>>> + * Copyright © 2017 Google, Inc. >>>>>> + * >>>>>> + * Permission is hereby granted, free of charge, to any person >>>>>> obtaining >>>>>> a >>>>>> + * copy of this software and associated documentation files (the >>>>>> "Software"), >>>>>> + * to deal in the Software without restriction, including without >>>>>> limitation >>>>>> + * the rights to use, copy, modify, merge, publish, distribute, >>>>>> sublicense, >>>>>> + * and/or sell copies of the Software, and to permit persons to whom >>>>>> the >>>>>> + * Software is furnished to do so, subject to the following >>>>>> conditions: >>>>>> + * >>>>>> + * The above copyright notice and this permission notice (including >>>>>> the >>>>>> next >>>>>> + * paragraph) shall be included in all copies or substantial portions >>>>>> of >>>>>> the >>>>>> + * Software. >>>>>> + * >>>>>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, >>>>>> EXPRESS OR >>>>>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF >>>>>> MERCHANTABILITY, >>>>>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT >>>>>> SHALL >>>>>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES >>>>>> OR >>>>>> OTHER >>>>>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, >>>>>> ARISING >>>>>> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER >>>>>> DEALINGS >>>>>> + * IN THE SOFTWARE. >>>>>> + * >>>>>> + */ >>>>>> + >>>>>> +#include "igt.h" >>>>>> + >>>>>> +IGT_TEST_DESCRIPTION("Test content protection (HDCP)"); >>>>>> + >>>>>> +typedef struct { >>>>>> + int drm_fd; >>>>>> + igt_display_t display; >>>>>> +} data_t; >>>>>> + >>>>>> +static bool >>>>>> +wait_for_prop_value(igt_output_t *output, uint64_t expected) >>>>>> +{ >>>>>> + uint64_t val; >>>>>> + int i; >>>>>> + >>>>>> + for (i = 0; i < 2000; i++) { >>>>> we need 5+Sec to complete the Second part of authentication, in case of >>>>> repeater with max downstream topology. >>>>> So we might need to wait for 6000(6Sec) loops!? >>>>>> + val = igt_output_get_prop(output, >>>>>> + IGT_CONNECTOR_CONTENT_PROTECTION); >>>>>> + if (val == expected) >>>>>> + return true; >>>>>> + usleep(1000); >>>>>> + } >>>>>> + igt_info("prop_value mismatch %ld != %ld\n", val, expected); >>>>>> + return false; >>>>>> +} >>>>>> + >>>>>> +static void >>>>>> +test_pipe_output(igt_display_t *display, const enum pipe pipe, >>>>>> + igt_output_t *output, enum igt_commit_style s) >>>>>> +{ >>>>>> + drmModeModeInfo mode; >>>>>> + igt_plane_t *primary; >>>>>> + struct igt_fb red; >>>>>> + bool ret; >>>>>> + >>>>>> + igt_assert(kmstest_get_connector_default_mode( >>>>>> + display->drm_fd, output->config.connector, &mode)); >>>>>> + >>>>>> + igt_output_override_mode(output, &mode); >>>>>> + igt_output_set_pipe(output, pipe); >>>>>> + >>>>>> + igt_create_color_fb(display->drm_fd, mode.hdisplay, mode.vdisplay, >>>>>> + DRM_FORMAT_XRGB8888, LOCAL_DRM_FORMAT_MOD_NONE, >>>>>> + 1.f, 0.f, 0.f, &red); >>>>>> + primary = igt_output_get_plane_type(output, >>>>>> DRM_PLANE_TYPE_PRIMARY); >>>>>> + igt_plane_set_fb(primary, &red); >>>>>> + igt_display_commit2(display, s); >>>>>> + >>>>>> + igt_output_set_prop_value(output, >>>>>> IGT_CONNECTOR_CONTENT_PROTECTION, >>>>>> 0); >>>>>> + igt_display_commit2(display, s); >>>>>> + >>>>>> + ret = wait_for_prop_value(output, 0); >>>>>> + igt_require_f(ret, "Content Protection not cleared\n"); >>>>> When expected property state is not met, you might want to use >>>>> igt_assert_f to fail the subtest. Here you are just skipping the >>>>> subtest. >>>>>> + >>>>>> + igt_output_set_prop_value(output, >>>>>> IGT_CONNECTOR_CONTENT_PROTECTION, >>>>>> 1); >>>>>> + igt_display_commit2(display, s); >>>>>> + >>>>>> + ret = wait_for_prop_value(output, 2); >>>> >>>> As there are range of HDCP sinks which might fail to provide the R0 in >>>> 100mSec in the first attempt. But passes the condition in next attempt. >>>> In the testing it is observed that many panel's widely used showcase >>>> above >>>> mentioned behavior. >>>> >>>> So we need to retry on HDCP enable before declaring the HDCP failure. >>>> >>> Instead of retrying the whole thing, why not just make the 100ms >>> timeout bigger? What values >100ms have you observed? >> I dont think we can increase the timeout beyond mentioned 100mSec. If we do >> so it will be non compliance with HDCP spec hence CTS will fail. > What's the difference to userspace? Isn't stringing together multiple > 100ms timeouts equivalent as one longer timeout? More injustice to userspace, as at kernel auth will fail in 100mSec due to R0 is not available from panel in time. But there is no way userspace to detect the failure immediately. So userspace has to poll property till the max time allowed (5.1Sec)for authentication to detect the failure, only then it can request for re-authentication. > > >> IMO Retrying will be the best option left. >> >>>>>> + igt_require_f(ret, "Content Protection not enabled\n"); >>>>> same as above. igt_assert_f needed? >>>>> >>>>> And when you are done with a connector(output), we cant leave the >>>>> content >>>>> protection in desired state. >>>>> Else when we enable the connector for some other IGT, content protection >>>>> authentication will be attempted by kernel. >>>>> So when we are done with a connector, we need to clear the content >>>>> protection state(set to OFF) of the connector. >>>>> >>>>>> + >>>>>> + igt_plane_set_fb(primary, NULL); >>>>>> + igt_output_set_pipe(output, PIPE_NONE); >>>>>> +} >>>>>> + >>>>>> +static void >>>>>> +test_content_protection(igt_display_t *display, enum igt_commit_style >>>>>> s) >>>>>> +{ >>>>>> + igt_output_t *output; >>>>>> + enum pipe pipe; >>>>>> + int valid_tests = 0; >>>>>> + >>>>>> + for_each_pipe_with_valid_output(display, pipe, output) { >>>> From the testing of this IGT with kernel changes, observed a need of >>>> relooking at HDCP subtests definition. Reasoning is as follows. >>>> >>>> And here we are taking each pipe and their compatible outputs(connectors) >>>> for hdcp test. Like PIPEA + HDMI-A-1, PIPEB+HDMI-A-1 etc. >>>> But as HDCP is associated to connectors, we need to iterate on hdcp >>>> capable >>>> connector and choose the compatible pipe. >>>> In that way, we will be able test the HDCP on all hdcp capable >>>> connectors. >>>> >>> I'm confused. The documentation for this loop says it tries every >>> combination of connector+pipe for every connected connector. Can you >>> explain what I'm missing? >>> >>> Sean >> Yes. Macro takes each pipe and tests against all interface able outputs. I >> missed that point. >> Still it will be efficient to test hdcp on each output with only one >> compatible pipe. >> testing with other compatible pipes of the connector has no ROI w.r.t hdcp >> testing. >> >> Considering that macro provides super set of req pipe+output combination, no >> urgency to modify immediately. >> > Ok, great! I'm relieved I was not going crazy :) > > Sean > > >>>> And since irrespective of the hdcp status (pass/fail) on a connector, we >>>> need to test hdcp on other connectors one after another. >>>> So this can be achieved if the test of HDCP on each connector is >>>> implemented >>>> as different IGT subtests. >>>> >>>>>> + if (!output->props[IGT_CONNECTOR_CONTENT_PROTECTION]) >>>>>> + continue; >>>>>> + >>>>>> + test_pipe_output(display, pipe, output, s); >>>>>> + valid_tests++; >>>>>> + } >>>>>> + igt_require_f(valid_tests, "no support for content proteciton >>>>>> found\n"); >>>>>> +} >>>>>> + >>>>>> +igt_main >>>>>> +{ >>>>>> + data_t data = {}; >>>>>> + >>>>>> + igt_fixture { >>>>>> + igt_skip_on_simulation(); >>>>>> + >>>>>> + data.drm_fd = drm_open_driver(DRIVER_ANY); >>>>> You need Master privilege here. Hence might want to use >>>>> drm_open_driver_master() instead. >>>>> >>>>> -Ram >>>>>> + >>>>>> + igt_display_init(&data.display, data.drm_fd); >>>>>> + } >>>>>> + >>>>>> + >>>>>> + igt_subtest("legacy") >>>>>> + test_content_protection(&data.display, COMMIT_LEGACY); >>>> As discussed above, we need to consider connector-x-legacy as a subtest. >>>>>> + >>>>>> + igt_subtest("atomic") { >>>>>> + igt_require(data.display.is_atomic); >>>>>> + test_content_protection(&data.display, COMMIT_ATOMIC); >>>> As discussed above, we need to consider connector-x-atomic as a subtest. >> For internal testing I have sub tests defined as below. Please see if it >> helps. >> Might need some tuning though. >> >> igt_main >> { >> data_t data = {}; >> + igt_output_t *output; >> + enum pipe pipe; >> + char test_name[50]; >> + int i; >> >> igt_fixture { >> igt_skip_on_simulation(); >> @@ -122,13 +109,44 @@ igt_main >> igt_display_init(&data.display, data.drm_fd); >> } >> >> + /* >> + * Not using the for_each_connected_output as that mandates >> + * igt_can_fail(). >> + */ >> + for (i = 0; i < data.display.n_outputs; i++) >> + for_each_if((output = &(data.display.outputs[i]), >> + igt_output_is_connected(output))) { >> + if (!output->props[IGT_CONNECTOR_CONTENT_PROTECTION]) >> + continue; >> >> - igt_subtest("legacy") >> - test_content_protection(&data.display, COMMIT_LEGACY); >> - >> - igt_subtest("atomic") { >> - igt_require(data.display.is_atomic); >> - test_content_protection(&data.display, COMMIT_ATOMIC); >> + for_each_pipe_static(pipe) { >> + if (!igt_pipe_connector_valid(pipe, output)) >> + continue; >> + >> + sprintf(test_name, "HDCP_%s_PIPE-%s_legacy", >> + output->name, >> + kmstest_pipe_name(pipe)); >> + igt_subtest(test_name) >> + test_content_protection(&data.display, pipe, >> + output, COMMIT_LEGACY); >> + >> + if (!data.display.is_atomic) >> + break; >> + >> + sprintf(test_name, "HDCP_%s_PIPE-%s_atomic", >> + output->name, >> + kmstest_pipe_name(pipe)); >> + igt_subtest(test_name) >> + test_content_protection(&data.display, pipe, >> + output, COMMIT_ATOMIC); >> + >> + /* >> + * Testing a output with a pipe is enough for HDCP >> + * testing. No ROI in testing the connector with >> other >> + * pipes. So Break the loop on pipe. >> + */ >> + break; >> + } >> } >> >> igt_fixture >> >> >> >>>> -Ram >>>> >>>>>> + } >>>>>> + >>>>>> + igt_fixture >>>>>> + igt_display_fini(&data.display); >>>>>> +} >>>>>> diff --git a/tests/meson.build b/tests/meson.build >>>>>> index 59ccd9a6..b12c35c0 100644 >>>>>> --- a/tests/meson.build >>>>>> +++ b/tests/meson.build >>>>>> @@ -157,6 +157,7 @@ test_progs = [ >>>>>> 'kms_chv_cursor_fail', >>>>>> 'kms_color', >>>>>> 'kms_concurrent', >>>>>> + 'kms_content_protection', >>>>>> 'kms_crtc_background_color', >>>>>> 'kms_cursor_crc', >>>>>> 'kms_cursor_legacy', >>>>> _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/2] kms_content_protection: Add Content Protection test 2018-01-06 13:10 ` Ramalingam C @ 2018-01-06 15:04 ` Sean Paul 2018-01-06 15:52 ` Ramalingam C 0 siblings, 1 reply; 12+ messages in thread From: Sean Paul @ 2018-01-06 15:04 UTC (permalink / raw) To: Ramalingam C; +Cc: Intel Graphics Development On Sat, Jan 6, 2018 at 8:10 AM, Ramalingam C <ramalingam.c@intel.com> wrote: > > > On Saturday 06 January 2018 06:30 PM, Sean Paul wrote: >> >> On Sat, Jan 6, 2018 at 5:44 AM, Ramalingam C <ramalingam.c@intel.com> >> wrote: >>> >>> On Saturday 06 January 2018 12:59 AM, Sean Paul wrote: >>>> >>>> On Tue, Dec 19, 2017 at 9:28 AM, Ramalingam C <ramalingam.c@intel.com> >>>> wrote: >>>>> >>>>> Adding few more findings from the IGT usage with kernel solutions. >>>>> >>>>> >>>>> >>>>> On Wednesday 13 December 2017 04:07 PM, Ramalingam C wrote: >>>>>> >>>>>> Sean, >>>>>> >>>>>> Adding few more observations. >>>>>> >>>>>> >>>>>> On Thursday 07 December 2017 05:47 AM, Sean Paul wrote: >>>>>>> >>>>>>> Pretty simple test: >>>>>>> - initializes the output >>>>>>> - clears the content protection property >>>>>>> - verifies that it clears >>>>>>> - sets the content protection property to desired >>>>>>> - verifies that it transitions to enabled >>>>>>> >>>>>>> Does this for both legacy and atomic. >>>>>>> >>>>>>> Changes in v2: >>>>>>> - Don't check for i915 gen >>>>>>> - Skip test if Content Protection property is absent >>>>>>> >>>>>>> Signed-off-by: Sean Paul <seanpaul@chromium.org> >>>>>>> --- >>>>>>> lib/igt_kms.c | 3 +- >>>>>>> lib/igt_kms.h | 1 + >>>>>>> tests/Makefile.sources | 1 + >>>>>>> tests/kms_content_protection.c | 129 >>>>>>> +++++++++++++++++++++++++++++++++++++++++ >>>>>>> tests/meson.build | 1 + >>>>>>> 5 files changed, 134 insertions(+), 1 deletion(-) >>>>>>> create mode 100644 tests/kms_content_protection.c >>>>>>> >>>>>>> diff --git a/lib/igt_kms.c b/lib/igt_kms.c >>>>>>> index 125ecb19..907db694 100644 >>>>>>> --- a/lib/igt_kms.c >>>>>>> +++ b/lib/igt_kms.c >>>>>>> @@ -190,7 +190,8 @@ const char >>>>>>> *igt_connector_prop_names[IGT_NUM_CONNECTOR_PROPS] = { >>>>>>> "scaling mode", >>>>>>> "CRTC_ID", >>>>>>> "DPMS", >>>>>>> - "Broadcast RGB" >>>>>>> + "Broadcast RGB", >>>>>>> + "Content Protection" >>>>>>> }; >>>>>>> /* >>>>>>> diff --git a/lib/igt_kms.h b/lib/igt_kms.h >>>>>>> index 2a480bf3..93e59dc7 100644 >>>>>>> --- a/lib/igt_kms.h >>>>>>> +++ b/lib/igt_kms.h >>>>>>> @@ -118,6 +118,7 @@ enum igt_atomic_connector_properties { >>>>>>> IGT_CONNECTOR_CRTC_ID, >>>>>>> IGT_CONNECTOR_DPMS, >>>>>>> IGT_CONNECTOR_BROADCAST_RGB, >>>>>>> + IGT_CONNECTOR_CONTENT_PROTECTION, >>>>>>> IGT_NUM_CONNECTOR_PROPS >>>>>>> }; >>>>>>> diff --git a/tests/Makefile.sources b/tests/Makefile.sources >>>>>>> index 34ca71a0..e0466411 100644 >>>>>>> --- a/tests/Makefile.sources >>>>>>> +++ b/tests/Makefile.sources >>>>>>> @@ -179,6 +179,7 @@ TESTS_progs = \ >>>>>>> kms_chv_cursor_fail \ >>>>>>> kms_color \ >>>>>>> kms_concurrent \ >>>>>>> + kms_content_protection\ >>>>>>> kms_crtc_background_color \ >>>>>>> kms_cursor_crc \ >>>>>>> kms_cursor_legacy \ >>>>>>> diff --git a/tests/kms_content_protection.c >>>>>>> b/tests/kms_content_protection.c >>>>>>> new file mode 100644 >>>>>>> index 00000000..5d61b257 >>>>>>> --- /dev/null >>>>>>> +++ b/tests/kms_content_protection.c >>>>>>> @@ -0,0 +1,129 @@ >>>>>>> +/* >>>>>>> + * Copyright © 2017 Google, Inc. >>>>>>> + * >>>>>>> + * Permission is hereby granted, free of charge, to any person >>>>>>> obtaining >>>>>>> a >>>>>>> + * copy of this software and associated documentation files (the >>>>>>> "Software"), >>>>>>> + * to deal in the Software without restriction, including without >>>>>>> limitation >>>>>>> + * the rights to use, copy, modify, merge, publish, distribute, >>>>>>> sublicense, >>>>>>> + * and/or sell copies of the Software, and to permit persons to whom >>>>>>> the >>>>>>> + * Software is furnished to do so, subject to the following >>>>>>> conditions: >>>>>>> + * >>>>>>> + * The above copyright notice and this permission notice (including >>>>>>> the >>>>>>> next >>>>>>> + * paragraph) shall be included in all copies or substantial >>>>>>> portions >>>>>>> of >>>>>>> the >>>>>>> + * Software. >>>>>>> + * >>>>>>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, >>>>>>> EXPRESS OR >>>>>>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF >>>>>>> MERCHANTABILITY, >>>>>>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO >>>>>>> EVENT >>>>>>> SHALL >>>>>>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES >>>>>>> OR >>>>>>> OTHER >>>>>>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, >>>>>>> ARISING >>>>>>> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR >>>>>>> OTHER >>>>>>> DEALINGS >>>>>>> + * IN THE SOFTWARE. >>>>>>> + * >>>>>>> + */ >>>>>>> + >>>>>>> +#include "igt.h" >>>>>>> + >>>>>>> +IGT_TEST_DESCRIPTION("Test content protection (HDCP)"); >>>>>>> + >>>>>>> +typedef struct { >>>>>>> + int drm_fd; >>>>>>> + igt_display_t display; >>>>>>> +} data_t; >>>>>>> + >>>>>>> +static bool >>>>>>> +wait_for_prop_value(igt_output_t *output, uint64_t expected) >>>>>>> +{ >>>>>>> + uint64_t val; >>>>>>> + int i; >>>>>>> + >>>>>>> + for (i = 0; i < 2000; i++) { >>>>>> >>>>>> we need 5+Sec to complete the Second part of authentication, in case >>>>>> of >>>>>> repeater with max downstream topology. >>>>>> So we might need to wait for 6000(6Sec) loops!? >>>>>>> >>>>>>> + val = igt_output_get_prop(output, >>>>>>> + IGT_CONNECTOR_CONTENT_PROTECTION); >>>>>>> + if (val == expected) >>>>>>> + return true; >>>>>>> + usleep(1000); >>>>>>> + } >>>>>>> + igt_info("prop_value mismatch %ld != %ld\n", val, expected); >>>>>>> + return false; >>>>>>> +} >>>>>>> + >>>>>>> +static void >>>>>>> +test_pipe_output(igt_display_t *display, const enum pipe pipe, >>>>>>> + igt_output_t *output, enum igt_commit_style s) >>>>>>> +{ >>>>>>> + drmModeModeInfo mode; >>>>>>> + igt_plane_t *primary; >>>>>>> + struct igt_fb red; >>>>>>> + bool ret; >>>>>>> + >>>>>>> + igt_assert(kmstest_get_connector_default_mode( >>>>>>> + display->drm_fd, output->config.connector, &mode)); >>>>>>> + >>>>>>> + igt_output_override_mode(output, &mode); >>>>>>> + igt_output_set_pipe(output, pipe); >>>>>>> + >>>>>>> + igt_create_color_fb(display->drm_fd, mode.hdisplay, >>>>>>> mode.vdisplay, >>>>>>> + DRM_FORMAT_XRGB8888, LOCAL_DRM_FORMAT_MOD_NONE, >>>>>>> + 1.f, 0.f, 0.f, &red); >>>>>>> + primary = igt_output_get_plane_type(output, >>>>>>> DRM_PLANE_TYPE_PRIMARY); >>>>>>> + igt_plane_set_fb(primary, &red); >>>>>>> + igt_display_commit2(display, s); >>>>>>> + >>>>>>> + igt_output_set_prop_value(output, >>>>>>> IGT_CONNECTOR_CONTENT_PROTECTION, >>>>>>> 0); >>>>>>> + igt_display_commit2(display, s); >>>>>>> + >>>>>>> + ret = wait_for_prop_value(output, 0); >>>>>>> + igt_require_f(ret, "Content Protection not cleared\n"); >>>>>> >>>>>> When expected property state is not met, you might want to use >>>>>> igt_assert_f to fail the subtest. Here you are just skipping the >>>>>> subtest. >>>>>>> >>>>>>> + >>>>>>> + igt_output_set_prop_value(output, >>>>>>> IGT_CONNECTOR_CONTENT_PROTECTION, >>>>>>> 1); >>>>>>> + igt_display_commit2(display, s); >>>>>>> + >>>>>>> + ret = wait_for_prop_value(output, 2); >>>>> >>>>> >>>>> As there are range of HDCP sinks which might fail to provide the R0 in >>>>> 100mSec in the first attempt. But passes the condition in next attempt. >>>>> In the testing it is observed that many panel's widely used showcase >>>>> above >>>>> mentioned behavior. >>>>> >>>>> So we need to retry on HDCP enable before declaring the HDCP failure. >>>>> >>>> Instead of retrying the whole thing, why not just make the 100ms >>>> timeout bigger? What values >100ms have you observed? >>> >>> I dont think we can increase the timeout beyond mentioned 100mSec. If we >>> do >>> so it will be non compliance with HDCP spec hence CTS will fail. >> >> What's the difference to userspace? Isn't stringing together multiple >> 100ms timeouts equivalent as one longer timeout? > > More injustice to userspace, as at kernel auth will fail in 100mSec due to > R0 is not available from panel in time. > But there is no way userspace to detect the failure immediately. > So userspace has to poll property till the max time allowed (5.1Sec)for > authentication to detect the failure, > only then it can request for re-authentication. > I still don't understand. Where do you want the retry located? In userspace or kernel? If it's in kernel, there's no difference to userspace between retrying multiple times or just increasing the timeout. If you're arguing for a userspace retry, that's not really relevant here (since we shouldn't be using non-compliant sinks for igt). Sean >> >> >>> IMO Retrying will be the best option left. >>> >>>>>>> + igt_require_f(ret, "Content Protection not enabled\n"); >>>>>> >>>>>> same as above. igt_assert_f needed? >>>>>> >>>>>> And when you are done with a connector(output), we cant leave the >>>>>> content >>>>>> protection in desired state. >>>>>> Else when we enable the connector for some other IGT, content >>>>>> protection >>>>>> authentication will be attempted by kernel. >>>>>> So when we are done with a connector, we need to clear the content >>>>>> protection state(set to OFF) of the connector. >>>>>> >>>>>>> + >>>>>>> + igt_plane_set_fb(primary, NULL); >>>>>>> + igt_output_set_pipe(output, PIPE_NONE); >>>>>>> +} >>>>>>> + >>>>>>> +static void >>>>>>> +test_content_protection(igt_display_t *display, enum >>>>>>> igt_commit_style >>>>>>> s) >>>>>>> +{ >>>>>>> + igt_output_t *output; >>>>>>> + enum pipe pipe; >>>>>>> + int valid_tests = 0; >>>>>>> + >>>>>>> + for_each_pipe_with_valid_output(display, pipe, output) { >>>>> >>>>> From the testing of this IGT with kernel changes, observed a need of >>>>> relooking at HDCP subtests definition. Reasoning is as follows. >>>>> >>>>> And here we are taking each pipe and their compatible >>>>> outputs(connectors) >>>>> for hdcp test. Like PIPEA + HDMI-A-1, PIPEB+HDMI-A-1 etc. >>>>> But as HDCP is associated to connectors, we need to iterate on hdcp >>>>> capable >>>>> connector and choose the compatible pipe. >>>>> In that way, we will be able test the HDCP on all hdcp capable >>>>> connectors. >>>>> >>>> I'm confused. The documentation for this loop says it tries every >>>> combination of connector+pipe for every connected connector. Can you >>>> explain what I'm missing? >>>> >>>> Sean >>> >>> Yes. Macro takes each pipe and tests against all interface able outputs. >>> I >>> missed that point. >>> Still it will be efficient to test hdcp on each output with only one >>> compatible pipe. >>> testing with other compatible pipes of the connector has no ROI w.r.t >>> hdcp >>> testing. >>> >>> Considering that macro provides super set of req pipe+output combination, >>> no >>> urgency to modify immediately. >>> >> Ok, great! I'm relieved I was not going crazy :) >> >> Sean >> >> >>>>> And since irrespective of the hdcp status (pass/fail) on a connector, >>>>> we >>>>> need to test hdcp on other connectors one after another. >>>>> So this can be achieved if the test of HDCP on each connector is >>>>> implemented >>>>> as different IGT subtests. >>>>> >>>>>>> + if (!output->props[IGT_CONNECTOR_CONTENT_PROTECTION]) >>>>>>> + continue; >>>>>>> + >>>>>>> + test_pipe_output(display, pipe, output, s); >>>>>>> + valid_tests++; >>>>>>> + } >>>>>>> + igt_require_f(valid_tests, "no support for content proteciton >>>>>>> found\n"); >>>>>>> +} >>>>>>> + >>>>>>> +igt_main >>>>>>> +{ >>>>>>> + data_t data = {}; >>>>>>> + >>>>>>> + igt_fixture { >>>>>>> + igt_skip_on_simulation(); >>>>>>> + >>>>>>> + data.drm_fd = drm_open_driver(DRIVER_ANY); >>>>>> >>>>>> You need Master privilege here. Hence might want to use >>>>>> drm_open_driver_master() instead. >>>>>> >>>>>> -Ram >>>>>>> >>>>>>> + >>>>>>> + igt_display_init(&data.display, data.drm_fd); >>>>>>> + } >>>>>>> + >>>>>>> + >>>>>>> + igt_subtest("legacy") >>>>>>> + test_content_protection(&data.display, COMMIT_LEGACY); >>>>> >>>>> As discussed above, we need to consider connector-x-legacy as a >>>>> subtest. >>>>>>> >>>>>>> + >>>>>>> + igt_subtest("atomic") { >>>>>>> + igt_require(data.display.is_atomic); >>>>>>> + test_content_protection(&data.display, COMMIT_ATOMIC); >>>>> >>>>> As discussed above, we need to consider connector-x-atomic as a >>>>> subtest. >>> >>> For internal testing I have sub tests defined as below. Please see if it >>> helps. >>> Might need some tuning though. >>> >>> igt_main >>> { >>> data_t data = {}; >>> + igt_output_t *output; >>> + enum pipe pipe; >>> + char test_name[50]; >>> + int i; >>> >>> igt_fixture { >>> igt_skip_on_simulation(); >>> @@ -122,13 +109,44 @@ igt_main >>> igt_display_init(&data.display, data.drm_fd); >>> } >>> >>> + /* >>> + * Not using the for_each_connected_output as that mandates >>> + * igt_can_fail(). >>> + */ >>> + for (i = 0; i < data.display.n_outputs; i++) >>> + for_each_if((output = &(data.display.outputs[i]), >>> + igt_output_is_connected(output))) { >>> + if (!output->props[IGT_CONNECTOR_CONTENT_PROTECTION]) >>> + continue; >>> >>> - igt_subtest("legacy") >>> - test_content_protection(&data.display, COMMIT_LEGACY); >>> - >>> - igt_subtest("atomic") { >>> - igt_require(data.display.is_atomic); >>> - test_content_protection(&data.display, COMMIT_ATOMIC); >>> + for_each_pipe_static(pipe) { >>> + if (!igt_pipe_connector_valid(pipe, output)) >>> + continue; >>> + >>> + sprintf(test_name, "HDCP_%s_PIPE-%s_legacy", >>> + output->name, >>> + kmstest_pipe_name(pipe)); >>> + igt_subtest(test_name) >>> + test_content_protection(&data.display, pipe, >>> + output, COMMIT_LEGACY); >>> + >>> + if (!data.display.is_atomic) >>> + break; >>> + >>> + sprintf(test_name, "HDCP_%s_PIPE-%s_atomic", >>> + output->name, >>> + kmstest_pipe_name(pipe)); >>> + igt_subtest(test_name) >>> + test_content_protection(&data.display, pipe, >>> + output, COMMIT_ATOMIC); >>> + >>> + /* >>> + * Testing a output with a pipe is enough for >>> HDCP >>> + * testing. No ROI in testing the connector with >>> other >>> + * pipes. So Break the loop on pipe. >>> + */ >>> + break; >>> + } >>> } >>> >>> igt_fixture >>> >>> >>> >>>>> -Ram >>>>> >>>>>>> + } >>>>>>> + >>>>>>> + igt_fixture >>>>>>> + igt_display_fini(&data.display); >>>>>>> +} >>>>>>> diff --git a/tests/meson.build b/tests/meson.build >>>>>>> index 59ccd9a6..b12c35c0 100644 >>>>>>> --- a/tests/meson.build >>>>>>> +++ b/tests/meson.build >>>>>>> @@ -157,6 +157,7 @@ test_progs = [ >>>>>>> 'kms_chv_cursor_fail', >>>>>>> 'kms_color', >>>>>>> 'kms_concurrent', >>>>>>> + 'kms_content_protection', >>>>>>> 'kms_crtc_background_color', >>>>>>> 'kms_cursor_crc', >>>>>>> 'kms_cursor_legacy', >>>>>> >>>>>> > _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/2] kms_content_protection: Add Content Protection test 2018-01-06 15:04 ` Sean Paul @ 2018-01-06 15:52 ` Ramalingam C 2018-01-08 13:48 ` Sean Paul 0 siblings, 1 reply; 12+ messages in thread From: Ramalingam C @ 2018-01-06 15:52 UTC (permalink / raw) To: Sean Paul; +Cc: Intel Graphics Development On Saturday 06 January 2018 08:34 PM, Sean Paul wrote: > On Sat, Jan 6, 2018 at 8:10 AM, Ramalingam C <ramalingam.c@intel.com> wrote: >> >> On Saturday 06 January 2018 06:30 PM, Sean Paul wrote: >>> On Sat, Jan 6, 2018 at 5:44 AM, Ramalingam C <ramalingam.c@intel.com> >>> wrote: >>>> On Saturday 06 January 2018 12:59 AM, Sean Paul wrote: >>>>> On Tue, Dec 19, 2017 at 9:28 AM, Ramalingam C <ramalingam.c@intel.com> >>>>> wrote: >>>>>> Adding few more findings from the IGT usage with kernel solutions. >>>>>> >>>>>> >>>>>> >>>>>> On Wednesday 13 December 2017 04:07 PM, Ramalingam C wrote: >>>>>>> Sean, >>>>>>> >>>>>>> Adding few more observations. >>>>>>> >>>>>>> >>>>>>> On Thursday 07 December 2017 05:47 AM, Sean Paul wrote: >>>>>>>> Pretty simple test: >>>>>>>> - initializes the output >>>>>>>> - clears the content protection property >>>>>>>> - verifies that it clears >>>>>>>> - sets the content protection property to desired >>>>>>>> - verifies that it transitions to enabled >>>>>>>> >>>>>>>> Does this for both legacy and atomic. >>>>>>>> >>>>>>>> Changes in v2: >>>>>>>> - Don't check for i915 gen >>>>>>>> - Skip test if Content Protection property is absent >>>>>>>> >>>>>>>> Signed-off-by: Sean Paul <seanpaul@chromium.org> >>>>>>>> --- >>>>>>>> lib/igt_kms.c | 3 +- >>>>>>>> lib/igt_kms.h | 1 + >>>>>>>> tests/Makefile.sources | 1 + >>>>>>>> tests/kms_content_protection.c | 129 >>>>>>>> +++++++++++++++++++++++++++++++++++++++++ >>>>>>>> tests/meson.build | 1 + >>>>>>>> 5 files changed, 134 insertions(+), 1 deletion(-) >>>>>>>> create mode 100644 tests/kms_content_protection.c >>>>>>>> >>>>>>>> diff --git a/lib/igt_kms.c b/lib/igt_kms.c >>>>>>>> index 125ecb19..907db694 100644 >>>>>>>> --- a/lib/igt_kms.c >>>>>>>> +++ b/lib/igt_kms.c >>>>>>>> @@ -190,7 +190,8 @@ const char >>>>>>>> *igt_connector_prop_names[IGT_NUM_CONNECTOR_PROPS] = { >>>>>>>> "scaling mode", >>>>>>>> "CRTC_ID", >>>>>>>> "DPMS", >>>>>>>> - "Broadcast RGB" >>>>>>>> + "Broadcast RGB", >>>>>>>> + "Content Protection" >>>>>>>> }; >>>>>>>> /* >>>>>>>> diff --git a/lib/igt_kms.h b/lib/igt_kms.h >>>>>>>> index 2a480bf3..93e59dc7 100644 >>>>>>>> --- a/lib/igt_kms.h >>>>>>>> +++ b/lib/igt_kms.h >>>>>>>> @@ -118,6 +118,7 @@ enum igt_atomic_connector_properties { >>>>>>>> IGT_CONNECTOR_CRTC_ID, >>>>>>>> IGT_CONNECTOR_DPMS, >>>>>>>> IGT_CONNECTOR_BROADCAST_RGB, >>>>>>>> + IGT_CONNECTOR_CONTENT_PROTECTION, >>>>>>>> IGT_NUM_CONNECTOR_PROPS >>>>>>>> }; >>>>>>>> diff --git a/tests/Makefile.sources b/tests/Makefile.sources >>>>>>>> index 34ca71a0..e0466411 100644 >>>>>>>> --- a/tests/Makefile.sources >>>>>>>> +++ b/tests/Makefile.sources >>>>>>>> @@ -179,6 +179,7 @@ TESTS_progs = \ >>>>>>>> kms_chv_cursor_fail \ >>>>>>>> kms_color \ >>>>>>>> kms_concurrent \ >>>>>>>> + kms_content_protection\ >>>>>>>> kms_crtc_background_color \ >>>>>>>> kms_cursor_crc \ >>>>>>>> kms_cursor_legacy \ >>>>>>>> diff --git a/tests/kms_content_protection.c >>>>>>>> b/tests/kms_content_protection.c >>>>>>>> new file mode 100644 >>>>>>>> index 00000000..5d61b257 >>>>>>>> --- /dev/null >>>>>>>> +++ b/tests/kms_content_protection.c >>>>>>>> @@ -0,0 +1,129 @@ >>>>>>>> +/* >>>>>>>> + * Copyright © 2017 Google, Inc. >>>>>>>> + * >>>>>>>> + * Permission is hereby granted, free of charge, to any person >>>>>>>> obtaining >>>>>>>> a >>>>>>>> + * copy of this software and associated documentation files (the >>>>>>>> "Software"), >>>>>>>> + * to deal in the Software without restriction, including without >>>>>>>> limitation >>>>>>>> + * the rights to use, copy, modify, merge, publish, distribute, >>>>>>>> sublicense, >>>>>>>> + * and/or sell copies of the Software, and to permit persons to whom >>>>>>>> the >>>>>>>> + * Software is furnished to do so, subject to the following >>>>>>>> conditions: >>>>>>>> + * >>>>>>>> + * The above copyright notice and this permission notice (including >>>>>>>> the >>>>>>>> next >>>>>>>> + * paragraph) shall be included in all copies or substantial >>>>>>>> portions >>>>>>>> of >>>>>>>> the >>>>>>>> + * Software. >>>>>>>> + * >>>>>>>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, >>>>>>>> EXPRESS OR >>>>>>>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF >>>>>>>> MERCHANTABILITY, >>>>>>>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO >>>>>>>> EVENT >>>>>>>> SHALL >>>>>>>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES >>>>>>>> OR >>>>>>>> OTHER >>>>>>>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, >>>>>>>> ARISING >>>>>>>> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR >>>>>>>> OTHER >>>>>>>> DEALINGS >>>>>>>> + * IN THE SOFTWARE. >>>>>>>> + * >>>>>>>> + */ >>>>>>>> + >>>>>>>> +#include "igt.h" >>>>>>>> + >>>>>>>> +IGT_TEST_DESCRIPTION("Test content protection (HDCP)"); >>>>>>>> + >>>>>>>> +typedef struct { >>>>>>>> + int drm_fd; >>>>>>>> + igt_display_t display; >>>>>>>> +} data_t; >>>>>>>> + >>>>>>>> +static bool >>>>>>>> +wait_for_prop_value(igt_output_t *output, uint64_t expected) >>>>>>>> +{ >>>>>>>> + uint64_t val; >>>>>>>> + int i; >>>>>>>> + >>>>>>>> + for (i = 0; i < 2000; i++) { >>>>>>> we need 5+Sec to complete the Second part of authentication, in case >>>>>>> of >>>>>>> repeater with max downstream topology. >>>>>>> So we might need to wait for 6000(6Sec) loops!? >>>>>>>> + val = igt_output_get_prop(output, >>>>>>>> + IGT_CONNECTOR_CONTENT_PROTECTION); >>>>>>>> + if (val == expected) >>>>>>>> + return true; >>>>>>>> + usleep(1000); >>>>>>>> + } >>>>>>>> + igt_info("prop_value mismatch %ld != %ld\n", val, expected); >>>>>>>> + return false; >>>>>>>> +} >>>>>>>> + >>>>>>>> +static void >>>>>>>> +test_pipe_output(igt_display_t *display, const enum pipe pipe, >>>>>>>> + igt_output_t *output, enum igt_commit_style s) >>>>>>>> +{ >>>>>>>> + drmModeModeInfo mode; >>>>>>>> + igt_plane_t *primary; >>>>>>>> + struct igt_fb red; >>>>>>>> + bool ret; >>>>>>>> + >>>>>>>> + igt_assert(kmstest_get_connector_default_mode( >>>>>>>> + display->drm_fd, output->config.connector, &mode)); >>>>>>>> + >>>>>>>> + igt_output_override_mode(output, &mode); >>>>>>>> + igt_output_set_pipe(output, pipe); >>>>>>>> + >>>>>>>> + igt_create_color_fb(display->drm_fd, mode.hdisplay, >>>>>>>> mode.vdisplay, >>>>>>>> + DRM_FORMAT_XRGB8888, LOCAL_DRM_FORMAT_MOD_NONE, >>>>>>>> + 1.f, 0.f, 0.f, &red); >>>>>>>> + primary = igt_output_get_plane_type(output, >>>>>>>> DRM_PLANE_TYPE_PRIMARY); >>>>>>>> + igt_plane_set_fb(primary, &red); >>>>>>>> + igt_display_commit2(display, s); >>>>>>>> + >>>>>>>> + igt_output_set_prop_value(output, >>>>>>>> IGT_CONNECTOR_CONTENT_PROTECTION, >>>>>>>> 0); >>>>>>>> + igt_display_commit2(display, s); >>>>>>>> + >>>>>>>> + ret = wait_for_prop_value(output, 0); >>>>>>>> + igt_require_f(ret, "Content Protection not cleared\n"); >>>>>>> When expected property state is not met, you might want to use >>>>>>> igt_assert_f to fail the subtest. Here you are just skipping the >>>>>>> subtest. >>>>>>>> + >>>>>>>> + igt_output_set_prop_value(output, >>>>>>>> IGT_CONNECTOR_CONTENT_PROTECTION, >>>>>>>> 1); >>>>>>>> + igt_display_commit2(display, s); >>>>>>>> + >>>>>>>> + ret = wait_for_prop_value(output, 2); >>>>>> >>>>>> As there are range of HDCP sinks which might fail to provide the R0 in >>>>>> 100mSec in the first attempt. But passes the condition in next attempt. >>>>>> In the testing it is observed that many panel's widely used showcase >>>>>> above >>>>>> mentioned behavior. >>>>>> >>>>>> So we need to retry on HDCP enable before declaring the HDCP failure. >>>>>> >>>>> Instead of retrying the whole thing, why not just make the 100ms >>>>> timeout bigger? What values >100ms have you observed? >>>> I dont think we can increase the timeout beyond mentioned 100mSec. If we >>>> do >>>> so it will be non compliance with HDCP spec hence CTS will fail. >>> What's the difference to userspace? Isn't stringing together multiple >>> 100ms timeouts equivalent as one longer timeout? >> More injustice to userspace, as at kernel auth will fail in 100mSec due to >> R0 is not available from panel in time. >> But there is no way userspace to detect the failure immediately. >> So userspace has to poll property till the max time allowed (5.1Sec)for >> authentication to detect the failure, >> only then it can request for re-authentication. >> > I still don't understand. Where do you want the retry located? In > userspace or kernel? If it's in kernel, there's no difference to > userspace between retrying multiple times or just increasing the > timeout. If you're arguing for a userspace retry, that's not really > relevant here (since we shouldn't be using non-compliant sinks for > igt). IMO retry is needed in kernel space and also in user space. As per my understanding, this hdcp igt will be executed on all CI setups going forward. Considering that I have seen many hdcp capable panels in use shows such failures, i am not sure on practicality of ensuring only HDCP compliant HDMI and DP panels only used on all CI setups. So since it is possible, i would prefer to avoid such hdcp failure reports with a retry from IGT. I leave the final decisions to you(maintainers). BTW just suggesting few points, not intended to argue :) -Ram > > Sean > > >>> >>>> IMO Retrying will be the best option left. >>>> >>>>>>>> + igt_require_f(ret, "Content Protection not enabled\n"); >>>>>>> same as above. igt_assert_f needed? >>>>>>> >>>>>>> And when you are done with a connector(output), we cant leave the >>>>>>> content >>>>>>> protection in desired state. >>>>>>> Else when we enable the connector for some other IGT, content >>>>>>> protection >>>>>>> authentication will be attempted by kernel. >>>>>>> So when we are done with a connector, we need to clear the content >>>>>>> protection state(set to OFF) of the connector. >>>>>>> >>>>>>>> + >>>>>>>> + igt_plane_set_fb(primary, NULL); >>>>>>>> + igt_output_set_pipe(output, PIPE_NONE); >>>>>>>> +} >>>>>>>> + >>>>>>>> +static void >>>>>>>> +test_content_protection(igt_display_t *display, enum >>>>>>>> igt_commit_style >>>>>>>> s) >>>>>>>> +{ >>>>>>>> + igt_output_t *output; >>>>>>>> + enum pipe pipe; >>>>>>>> + int valid_tests = 0; >>>>>>>> + >>>>>>>> + for_each_pipe_with_valid_output(display, pipe, output) { >>>>>> From the testing of this IGT with kernel changes, observed a need of >>>>>> relooking at HDCP subtests definition. Reasoning is as follows. >>>>>> >>>>>> And here we are taking each pipe and their compatible >>>>>> outputs(connectors) >>>>>> for hdcp test. Like PIPEA + HDMI-A-1, PIPEB+HDMI-A-1 etc. >>>>>> But as HDCP is associated to connectors, we need to iterate on hdcp >>>>>> capable >>>>>> connector and choose the compatible pipe. >>>>>> In that way, we will be able test the HDCP on all hdcp capable >>>>>> connectors. >>>>>> >>>>> I'm confused. The documentation for this loop says it tries every >>>>> combination of connector+pipe for every connected connector. Can you >>>>> explain what I'm missing? >>>>> >>>>> Sean >>>> Yes. Macro takes each pipe and tests against all interface able outputs. >>>> I >>>> missed that point. >>>> Still it will be efficient to test hdcp on each output with only one >>>> compatible pipe. >>>> testing with other compatible pipes of the connector has no ROI w.r.t >>>> hdcp >>>> testing. >>>> >>>> Considering that macro provides super set of req pipe+output combination, >>>> no >>>> urgency to modify immediately. >>>> >>> Ok, great! I'm relieved I was not going crazy :) >>> >>> Sean >>> >>> >>>>>> And since irrespective of the hdcp status (pass/fail) on a connector, >>>>>> we >>>>>> need to test hdcp on other connectors one after another. >>>>>> So this can be achieved if the test of HDCP on each connector is >>>>>> implemented >>>>>> as different IGT subtests. >>>>>> >>>>>>>> + if (!output->props[IGT_CONNECTOR_CONTENT_PROTECTION]) >>>>>>>> + continue; >>>>>>>> + >>>>>>>> + test_pipe_output(display, pipe, output, s); >>>>>>>> + valid_tests++; >>>>>>>> + } >>>>>>>> + igt_require_f(valid_tests, "no support for content proteciton >>>>>>>> found\n"); >>>>>>>> +} >>>>>>>> + >>>>>>>> +igt_main >>>>>>>> +{ >>>>>>>> + data_t data = {}; >>>>>>>> + >>>>>>>> + igt_fixture { >>>>>>>> + igt_skip_on_simulation(); >>>>>>>> + >>>>>>>> + data.drm_fd = drm_open_driver(DRIVER_ANY); >>>>>>> You need Master privilege here. Hence might want to use >>>>>>> drm_open_driver_master() instead. >>>>>>> >>>>>>> -Ram >>>>>>>> + >>>>>>>> + igt_display_init(&data.display, data.drm_fd); >>>>>>>> + } >>>>>>>> + >>>>>>>> + >>>>>>>> + igt_subtest("legacy") >>>>>>>> + test_content_protection(&data.display, COMMIT_LEGACY); >>>>>> As discussed above, we need to consider connector-x-legacy as a >>>>>> subtest. >>>>>>>> + >>>>>>>> + igt_subtest("atomic") { >>>>>>>> + igt_require(data.display.is_atomic); >>>>>>>> + test_content_protection(&data.display, COMMIT_ATOMIC); >>>>>> As discussed above, we need to consider connector-x-atomic as a >>>>>> subtest. >>>> For internal testing I have sub tests defined as below. Please see if it >>>> helps. >>>> Might need some tuning though. >>>> >>>> igt_main >>>> { >>>> data_t data = {}; >>>> + igt_output_t *output; >>>> + enum pipe pipe; >>>> + char test_name[50]; >>>> + int i; >>>> >>>> igt_fixture { >>>> igt_skip_on_simulation(); >>>> @@ -122,13 +109,44 @@ igt_main >>>> igt_display_init(&data.display, data.drm_fd); >>>> } >>>> >>>> + /* >>>> + * Not using the for_each_connected_output as that mandates >>>> + * igt_can_fail(). >>>> + */ >>>> + for (i = 0; i < data.display.n_outputs; i++) >>>> + for_each_if((output = &(data.display.outputs[i]), >>>> + igt_output_is_connected(output))) { >>>> + if (!output->props[IGT_CONNECTOR_CONTENT_PROTECTION]) >>>> + continue; >>>> >>>> - igt_subtest("legacy") >>>> - test_content_protection(&data.display, COMMIT_LEGACY); >>>> - >>>> - igt_subtest("atomic") { >>>> - igt_require(data.display.is_atomic); >>>> - test_content_protection(&data.display, COMMIT_ATOMIC); >>>> + for_each_pipe_static(pipe) { >>>> + if (!igt_pipe_connector_valid(pipe, output)) >>>> + continue; >>>> + >>>> + sprintf(test_name, "HDCP_%s_PIPE-%s_legacy", >>>> + output->name, >>>> + kmstest_pipe_name(pipe)); >>>> + igt_subtest(test_name) >>>> + test_content_protection(&data.display, pipe, >>>> + output, COMMIT_LEGACY); >>>> + >>>> + if (!data.display.is_atomic) >>>> + break; >>>> + >>>> + sprintf(test_name, "HDCP_%s_PIPE-%s_atomic", >>>> + output->name, >>>> + kmstest_pipe_name(pipe)); >>>> + igt_subtest(test_name) >>>> + test_content_protection(&data.display, pipe, >>>> + output, COMMIT_ATOMIC); >>>> + >>>> + /* >>>> + * Testing a output with a pipe is enough for >>>> HDCP >>>> + * testing. No ROI in testing the connector with >>>> other >>>> + * pipes. So Break the loop on pipe. >>>> + */ >>>> + break; >>>> + } >>>> } >>>> >>>> igt_fixture >>>> >>>> >>>> >>>>>> -Ram >>>>>> >>>>>>>> + } >>>>>>>> + >>>>>>>> + igt_fixture >>>>>>>> + igt_display_fini(&data.display); >>>>>>>> +} >>>>>>>> diff --git a/tests/meson.build b/tests/meson.build >>>>>>>> index 59ccd9a6..b12c35c0 100644 >>>>>>>> --- a/tests/meson.build >>>>>>>> +++ b/tests/meson.build >>>>>>>> @@ -157,6 +157,7 @@ test_progs = [ >>>>>>>> 'kms_chv_cursor_fail', >>>>>>>> 'kms_color', >>>>>>>> 'kms_concurrent', >>>>>>>> + 'kms_content_protection', >>>>>>>> 'kms_crtc_background_color', >>>>>>>> 'kms_cursor_crc', >>>>>>>> 'kms_cursor_legacy', >>>>>>> _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/2] kms_content_protection: Add Content Protection test 2018-01-06 15:52 ` Ramalingam C @ 2018-01-08 13:48 ` Sean Paul 0 siblings, 0 replies; 12+ messages in thread From: Sean Paul @ 2018-01-08 13:48 UTC (permalink / raw) To: Ramalingam C; +Cc: Intel Graphics Development On Sat, Jan 6, 2018 at 10:52 AM, Ramalingam C <ramalingam.c@intel.com> wrote: > > > On Saturday 06 January 2018 08:34 PM, Sean Paul wrote: >> >> On Sat, Jan 6, 2018 at 8:10 AM, Ramalingam C <ramalingam.c@intel.com> >> wrote: >>> >>> >>> On Saturday 06 January 2018 06:30 PM, Sean Paul wrote: >>>> >>>> On Sat, Jan 6, 2018 at 5:44 AM, Ramalingam C <ramalingam.c@intel.com> >>>> wrote: >>>>> >>>>> On Saturday 06 January 2018 12:59 AM, Sean Paul wrote: >>>>>> >>>>>> On Tue, Dec 19, 2017 at 9:28 AM, Ramalingam C <ramalingam.c@intel.com> >>>>>> wrote: >>>>>>> >>>>>>> Adding few more findings from the IGT usage with kernel solutions. >>>>>>> >>>>>>> >>>>>>> >>>>>>> On Wednesday 13 December 2017 04:07 PM, Ramalingam C wrote: >>>>>>>> >>>>>>>> Sean, >>>>>>>> >>>>>>>> Adding few more observations. >>>>>>>> >>>>>>>> >>>>>>>> On Thursday 07 December 2017 05:47 AM, Sean Paul wrote: >>>>>>>>> >>>>>>>>> Pretty simple test: >>>>>>>>> - initializes the output >>>>>>>>> - clears the content protection property >>>>>>>>> - verifies that it clears >>>>>>>>> - sets the content protection property to desired >>>>>>>>> - verifies that it transitions to enabled >>>>>>>>> >>>>>>>>> Does this for both legacy and atomic. >>>>>>>>> >>>>>>>>> Changes in v2: >>>>>>>>> - Don't check for i915 gen >>>>>>>>> - Skip test if Content Protection property is absent >>>>>>>>> >>>>>>>>> Signed-off-by: Sean Paul <seanpaul@chromium.org> >>>>>>>>> --- >>>>>>>>> lib/igt_kms.c | 3 +- >>>>>>>>> lib/igt_kms.h | 1 + >>>>>>>>> tests/Makefile.sources | 1 + >>>>>>>>> tests/kms_content_protection.c | 129 >>>>>>>>> +++++++++++++++++++++++++++++++++++++++++ >>>>>>>>> tests/meson.build | 1 + >>>>>>>>> 5 files changed, 134 insertions(+), 1 deletion(-) >>>>>>>>> create mode 100644 tests/kms_content_protection.c >>>>>>>>> >>>>>>>>> diff --git a/lib/igt_kms.c b/lib/igt_kms.c >>>>>>>>> index 125ecb19..907db694 100644 >>>>>>>>> --- a/lib/igt_kms.c >>>>>>>>> +++ b/lib/igt_kms.c >>>>>>>>> @@ -190,7 +190,8 @@ const char >>>>>>>>> *igt_connector_prop_names[IGT_NUM_CONNECTOR_PROPS] = { >>>>>>>>> "scaling mode", >>>>>>>>> "CRTC_ID", >>>>>>>>> "DPMS", >>>>>>>>> - "Broadcast RGB" >>>>>>>>> + "Broadcast RGB", >>>>>>>>> + "Content Protection" >>>>>>>>> }; >>>>>>>>> /* >>>>>>>>> diff --git a/lib/igt_kms.h b/lib/igt_kms.h >>>>>>>>> index 2a480bf3..93e59dc7 100644 >>>>>>>>> --- a/lib/igt_kms.h >>>>>>>>> +++ b/lib/igt_kms.h >>>>>>>>> @@ -118,6 +118,7 @@ enum igt_atomic_connector_properties { >>>>>>>>> IGT_CONNECTOR_CRTC_ID, >>>>>>>>> IGT_CONNECTOR_DPMS, >>>>>>>>> IGT_CONNECTOR_BROADCAST_RGB, >>>>>>>>> + IGT_CONNECTOR_CONTENT_PROTECTION, >>>>>>>>> IGT_NUM_CONNECTOR_PROPS >>>>>>>>> }; >>>>>>>>> diff --git a/tests/Makefile.sources b/tests/Makefile.sources >>>>>>>>> index 34ca71a0..e0466411 100644 >>>>>>>>> --- a/tests/Makefile.sources >>>>>>>>> +++ b/tests/Makefile.sources >>>>>>>>> @@ -179,6 +179,7 @@ TESTS_progs = \ >>>>>>>>> kms_chv_cursor_fail \ >>>>>>>>> kms_color \ >>>>>>>>> kms_concurrent \ >>>>>>>>> + kms_content_protection\ >>>>>>>>> kms_crtc_background_color \ >>>>>>>>> kms_cursor_crc \ >>>>>>>>> kms_cursor_legacy \ >>>>>>>>> diff --git a/tests/kms_content_protection.c >>>>>>>>> b/tests/kms_content_protection.c >>>>>>>>> new file mode 100644 >>>>>>>>> index 00000000..5d61b257 >>>>>>>>> --- /dev/null >>>>>>>>> +++ b/tests/kms_content_protection.c >>>>>>>>> @@ -0,0 +1,129 @@ >>>>>>>>> +/* >>>>>>>>> + * Copyright © 2017 Google, Inc. >>>>>>>>> + * >>>>>>>>> + * Permission is hereby granted, free of charge, to any person >>>>>>>>> obtaining >>>>>>>>> a >>>>>>>>> + * copy of this software and associated documentation files (the >>>>>>>>> "Software"), >>>>>>>>> + * to deal in the Software without restriction, including without >>>>>>>>> limitation >>>>>>>>> + * the rights to use, copy, modify, merge, publish, distribute, >>>>>>>>> sublicense, >>>>>>>>> + * and/or sell copies of the Software, and to permit persons to >>>>>>>>> whom >>>>>>>>> the >>>>>>>>> + * Software is furnished to do so, subject to the following >>>>>>>>> conditions: >>>>>>>>> + * >>>>>>>>> + * The above copyright notice and this permission notice >>>>>>>>> (including >>>>>>>>> the >>>>>>>>> next >>>>>>>>> + * paragraph) shall be included in all copies or substantial >>>>>>>>> portions >>>>>>>>> of >>>>>>>>> the >>>>>>>>> + * Software. >>>>>>>>> + * >>>>>>>>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, >>>>>>>>> EXPRESS OR >>>>>>>>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF >>>>>>>>> MERCHANTABILITY, >>>>>>>>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO >>>>>>>>> EVENT >>>>>>>>> SHALL >>>>>>>>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, >>>>>>>>> DAMAGES >>>>>>>>> OR >>>>>>>>> OTHER >>>>>>>>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, >>>>>>>>> ARISING >>>>>>>>> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR >>>>>>>>> OTHER >>>>>>>>> DEALINGS >>>>>>>>> + * IN THE SOFTWARE. >>>>>>>>> + * >>>>>>>>> + */ >>>>>>>>> + >>>>>>>>> +#include "igt.h" >>>>>>>>> + >>>>>>>>> +IGT_TEST_DESCRIPTION("Test content protection (HDCP)"); >>>>>>>>> + >>>>>>>>> +typedef struct { >>>>>>>>> + int drm_fd; >>>>>>>>> + igt_display_t display; >>>>>>>>> +} data_t; >>>>>>>>> + >>>>>>>>> +static bool >>>>>>>>> +wait_for_prop_value(igt_output_t *output, uint64_t expected) >>>>>>>>> +{ >>>>>>>>> + uint64_t val; >>>>>>>>> + int i; >>>>>>>>> + >>>>>>>>> + for (i = 0; i < 2000; i++) { >>>>>>>> >>>>>>>> we need 5+Sec to complete the Second part of authentication, in case >>>>>>>> of >>>>>>>> repeater with max downstream topology. >>>>>>>> So we might need to wait for 6000(6Sec) loops!? >>>>>>>>> >>>>>>>>> + val = igt_output_get_prop(output, >>>>>>>>> + IGT_CONNECTOR_CONTENT_PROTECTION); >>>>>>>>> + if (val == expected) >>>>>>>>> + return true; >>>>>>>>> + usleep(1000); >>>>>>>>> + } >>>>>>>>> + igt_info("prop_value mismatch %ld != %ld\n", val, expected); >>>>>>>>> + return false; >>>>>>>>> +} >>>>>>>>> + >>>>>>>>> +static void >>>>>>>>> +test_pipe_output(igt_display_t *display, const enum pipe pipe, >>>>>>>>> + igt_output_t *output, enum igt_commit_style s) >>>>>>>>> +{ >>>>>>>>> + drmModeModeInfo mode; >>>>>>>>> + igt_plane_t *primary; >>>>>>>>> + struct igt_fb red; >>>>>>>>> + bool ret; >>>>>>>>> + >>>>>>>>> + igt_assert(kmstest_get_connector_default_mode( >>>>>>>>> + display->drm_fd, output->config.connector, &mode)); >>>>>>>>> + >>>>>>>>> + igt_output_override_mode(output, &mode); >>>>>>>>> + igt_output_set_pipe(output, pipe); >>>>>>>>> + >>>>>>>>> + igt_create_color_fb(display->drm_fd, mode.hdisplay, >>>>>>>>> mode.vdisplay, >>>>>>>>> + DRM_FORMAT_XRGB8888, LOCAL_DRM_FORMAT_MOD_NONE, >>>>>>>>> + 1.f, 0.f, 0.f, &red); >>>>>>>>> + primary = igt_output_get_plane_type(output, >>>>>>>>> DRM_PLANE_TYPE_PRIMARY); >>>>>>>>> + igt_plane_set_fb(primary, &red); >>>>>>>>> + igt_display_commit2(display, s); >>>>>>>>> + >>>>>>>>> + igt_output_set_prop_value(output, >>>>>>>>> IGT_CONNECTOR_CONTENT_PROTECTION, >>>>>>>>> 0); >>>>>>>>> + igt_display_commit2(display, s); >>>>>>>>> + >>>>>>>>> + ret = wait_for_prop_value(output, 0); >>>>>>>>> + igt_require_f(ret, "Content Protection not cleared\n"); >>>>>>>> >>>>>>>> When expected property state is not met, you might want to use >>>>>>>> igt_assert_f to fail the subtest. Here you are just skipping the >>>>>>>> subtest. >>>>>>>>> >>>>>>>>> + >>>>>>>>> + igt_output_set_prop_value(output, >>>>>>>>> IGT_CONNECTOR_CONTENT_PROTECTION, >>>>>>>>> 1); >>>>>>>>> + igt_display_commit2(display, s); >>>>>>>>> + >>>>>>>>> + ret = wait_for_prop_value(output, 2); >>>>>>> >>>>>>> >>>>>>> As there are range of HDCP sinks which might fail to provide the R0 >>>>>>> in >>>>>>> 100mSec in the first attempt. But passes the condition in next >>>>>>> attempt. >>>>>>> In the testing it is observed that many panel's widely used showcase >>>>>>> above >>>>>>> mentioned behavior. >>>>>>> >>>>>>> So we need to retry on HDCP enable before declaring the HDCP failure. >>>>>>> >>>>>> Instead of retrying the whole thing, why not just make the 100ms >>>>>> timeout bigger? What values >100ms have you observed? >>>>> >>>>> I dont think we can increase the timeout beyond mentioned 100mSec. If >>>>> we >>>>> do >>>>> so it will be non compliance with HDCP spec hence CTS will fail. >>>> >>>> What's the difference to userspace? Isn't stringing together multiple >>>> 100ms timeouts equivalent as one longer timeout? >>> >>> More injustice to userspace, as at kernel auth will fail in 100mSec due >>> to >>> R0 is not available from panel in time. >>> But there is no way userspace to detect the failure immediately. >>> So userspace has to poll property till the max time allowed (5.1Sec)for >>> authentication to detect the failure, >>> only then it can request for re-authentication. >>> >> I still don't understand. Where do you want the retry located? In >> userspace or kernel? If it's in kernel, there's no difference to >> userspace between retrying multiple times or just increasing the >> timeout. If you're arguing for a userspace retry, that's not really >> relevant here (since we shouldn't be using non-compliant sinks for >> igt). > > IMO retry is needed in kernel space and also in user space. > It's entirely likely this is the right thing to do. However, I'm somewhat allergic to retries on v1 of a patch. I'd rather put the change out into the wild and see what breaks. If something does break, let's figure out what it is and fix that specific issue (in this case, waiting for R0 takes > 100ms). Ideally, I'd like to reserve retries for cases where hardware is being irrational and that can't be mitigated by software. Perhaps this is naive :) Anyways, I'll bump the R0 timeout for v5 and we can go from there. Sean > As per my understanding, this hdcp igt will be executed on all CI setups > going forward. > > Considering that I have seen many hdcp capable panels in use shows such > failures, > i am not sure on practicality of ensuring only HDCP compliant HDMI and DP > panels only used on all CI setups. > So since it is possible, i would prefer to avoid such hdcp failure reports > with a retry from IGT. > > I leave the final decisions to you(maintainers). BTW just suggesting few > points, not intended to argue :) > > -Ram > > >> >> Sean >> >> >>>> >>>>> IMO Retrying will be the best option left. >>>>> >>>>>>>>> + igt_require_f(ret, "Content Protection not enabled\n"); >>>>>>>> >>>>>>>> same as above. igt_assert_f needed? >>>>>>>> >>>>>>>> And when you are done with a connector(output), we cant leave the >>>>>>>> content >>>>>>>> protection in desired state. >>>>>>>> Else when we enable the connector for some other IGT, content >>>>>>>> protection >>>>>>>> authentication will be attempted by kernel. >>>>>>>> So when we are done with a connector, we need to clear the content >>>>>>>> protection state(set to OFF) of the connector. >>>>>>>> >>>>>>>>> + >>>>>>>>> + igt_plane_set_fb(primary, NULL); >>>>>>>>> + igt_output_set_pipe(output, PIPE_NONE); >>>>>>>>> +} >>>>>>>>> + >>>>>>>>> +static void >>>>>>>>> +test_content_protection(igt_display_t *display, enum >>>>>>>>> igt_commit_style >>>>>>>>> s) >>>>>>>>> +{ >>>>>>>>> + igt_output_t *output; >>>>>>>>> + enum pipe pipe; >>>>>>>>> + int valid_tests = 0; >>>>>>>>> + >>>>>>>>> + for_each_pipe_with_valid_output(display, pipe, output) { >>>>>>> >>>>>>> From the testing of this IGT with kernel changes, observed a need >>>>>>> of >>>>>>> relooking at HDCP subtests definition. Reasoning is as follows. >>>>>>> >>>>>>> And here we are taking each pipe and their compatible >>>>>>> outputs(connectors) >>>>>>> for hdcp test. Like PIPEA + HDMI-A-1, PIPEB+HDMI-A-1 etc. >>>>>>> But as HDCP is associated to connectors, we need to iterate on hdcp >>>>>>> capable >>>>>>> connector and choose the compatible pipe. >>>>>>> In that way, we will be able test the HDCP on all hdcp capable >>>>>>> connectors. >>>>>>> >>>>>> I'm confused. The documentation for this loop says it tries every >>>>>> combination of connector+pipe for every connected connector. Can you >>>>>> explain what I'm missing? >>>>>> >>>>>> Sean >>>>> >>>>> Yes. Macro takes each pipe and tests against all interface able >>>>> outputs. >>>>> I >>>>> missed that point. >>>>> Still it will be efficient to test hdcp on each output with only one >>>>> compatible pipe. >>>>> testing with other compatible pipes of the connector has no ROI w.r.t >>>>> hdcp >>>>> testing. >>>>> >>>>> Considering that macro provides super set of req pipe+output >>>>> combination, >>>>> no >>>>> urgency to modify immediately. >>>>> >>>> Ok, great! I'm relieved I was not going crazy :) >>>> >>>> Sean >>>> >>>> >>>>>>> And since irrespective of the hdcp status (pass/fail) on a connector, >>>>>>> we >>>>>>> need to test hdcp on other connectors one after another. >>>>>>> So this can be achieved if the test of HDCP on each connector is >>>>>>> implemented >>>>>>> as different IGT subtests. >>>>>>> >>>>>>>>> + if (!output->props[IGT_CONNECTOR_CONTENT_PROTECTION]) >>>>>>>>> + continue; >>>>>>>>> + >>>>>>>>> + test_pipe_output(display, pipe, output, s); >>>>>>>>> + valid_tests++; >>>>>>>>> + } >>>>>>>>> + igt_require_f(valid_tests, "no support for content proteciton >>>>>>>>> found\n"); >>>>>>>>> +} >>>>>>>>> + >>>>>>>>> +igt_main >>>>>>>>> +{ >>>>>>>>> + data_t data = {}; >>>>>>>>> + >>>>>>>>> + igt_fixture { >>>>>>>>> + igt_skip_on_simulation(); >>>>>>>>> + >>>>>>>>> + data.drm_fd = drm_open_driver(DRIVER_ANY); >>>>>>>> >>>>>>>> You need Master privilege here. Hence might want to use >>>>>>>> drm_open_driver_master() instead. >>>>>>>> >>>>>>>> -Ram >>>>>>>>> >>>>>>>>> + >>>>>>>>> + igt_display_init(&data.display, data.drm_fd); >>>>>>>>> + } >>>>>>>>> + >>>>>>>>> + >>>>>>>>> + igt_subtest("legacy") >>>>>>>>> + test_content_protection(&data.display, COMMIT_LEGACY); >>>>>>> >>>>>>> As discussed above, we need to consider connector-x-legacy as a >>>>>>> subtest. >>>>>>>>> >>>>>>>>> + >>>>>>>>> + igt_subtest("atomic") { >>>>>>>>> + igt_require(data.display.is_atomic); >>>>>>>>> + test_content_protection(&data.display, COMMIT_ATOMIC); >>>>>>> >>>>>>> As discussed above, we need to consider connector-x-atomic as a >>>>>>> subtest. >>>>> >>>>> For internal testing I have sub tests defined as below. Please see if >>>>> it >>>>> helps. >>>>> Might need some tuning though. >>>>> >>>>> igt_main >>>>> { >>>>> data_t data = {}; >>>>> + igt_output_t *output; >>>>> + enum pipe pipe; >>>>> + char test_name[50]; >>>>> + int i; >>>>> >>>>> igt_fixture { >>>>> igt_skip_on_simulation(); >>>>> @@ -122,13 +109,44 @@ igt_main >>>>> igt_display_init(&data.display, data.drm_fd); >>>>> } >>>>> >>>>> + /* >>>>> + * Not using the for_each_connected_output as that mandates >>>>> + * igt_can_fail(). >>>>> + */ >>>>> + for (i = 0; i < data.display.n_outputs; i++) >>>>> + for_each_if((output = &(data.display.outputs[i]), >>>>> + igt_output_is_connected(output))) { >>>>> + if (!output->props[IGT_CONNECTOR_CONTENT_PROTECTION]) >>>>> + continue; >>>>> >>>>> - igt_subtest("legacy") >>>>> - test_content_protection(&data.display, COMMIT_LEGACY); >>>>> - >>>>> - igt_subtest("atomic") { >>>>> - igt_require(data.display.is_atomic); >>>>> - test_content_protection(&data.display, COMMIT_ATOMIC); >>>>> + for_each_pipe_static(pipe) { >>>>> + if (!igt_pipe_connector_valid(pipe, output)) >>>>> + continue; >>>>> + >>>>> + sprintf(test_name, "HDCP_%s_PIPE-%s_legacy", >>>>> + output->name, >>>>> + kmstest_pipe_name(pipe)); >>>>> + igt_subtest(test_name) >>>>> + test_content_protection(&data.display, pipe, >>>>> + output, COMMIT_LEGACY); >>>>> + >>>>> + if (!data.display.is_atomic) >>>>> + break; >>>>> + >>>>> + sprintf(test_name, "HDCP_%s_PIPE-%s_atomic", >>>>> + output->name, >>>>> + kmstest_pipe_name(pipe)); >>>>> + igt_subtest(test_name) >>>>> + test_content_protection(&data.display, pipe, >>>>> + output, COMMIT_ATOMIC); >>>>> + >>>>> + /* >>>>> + * Testing a output with a pipe is enough for >>>>> HDCP >>>>> + * testing. No ROI in testing the connector >>>>> with >>>>> other >>>>> + * pipes. So Break the loop on pipe. >>>>> + */ >>>>> + break; >>>>> + } >>>>> } >>>>> >>>>> igt_fixture >>>>> >>>>> >>>>> >>>>>>> -Ram >>>>>>> >>>>>>>>> + } >>>>>>>>> + >>>>>>>>> + igt_fixture >>>>>>>>> + igt_display_fini(&data.display); >>>>>>>>> +} >>>>>>>>> diff --git a/tests/meson.build b/tests/meson.build >>>>>>>>> index 59ccd9a6..b12c35c0 100644 >>>>>>>>> --- a/tests/meson.build >>>>>>>>> +++ b/tests/meson.build >>>>>>>>> @@ -157,6 +157,7 @@ test_progs = [ >>>>>>>>> 'kms_chv_cursor_fail', >>>>>>>>> 'kms_color', >>>>>>>>> 'kms_concurrent', >>>>>>>>> + 'kms_content_protection', >>>>>>>>> 'kms_crtc_background_color', >>>>>>>>> 'kms_cursor_crc', >>>>>>>>> 'kms_cursor_legacy', >>>>>>>> >>>>>>>> > _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2018-01-08 13:49 UTC | newest] Thread overview: 12+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-12-07 0:17 [PATCH v2 1/2] CONTRIBUTING: Fix spelling mistake and line length Sean Paul 2017-12-07 0:17 ` [PATCH v2 2/2] kms_content_protection: Add Content Protection test Sean Paul 2017-12-11 10:53 ` Ramalingam C 2017-12-13 10:37 ` Ramalingam C 2017-12-19 14:28 ` Ramalingam C 2018-01-05 19:29 ` Sean Paul 2018-01-06 10:44 ` Ramalingam C 2018-01-06 13:00 ` Sean Paul 2018-01-06 13:10 ` Ramalingam C 2018-01-06 15:04 ` Sean Paul 2018-01-06 15:52 ` Ramalingam C 2018-01-08 13:48 ` Sean Paul
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox