[Intel-gfx] [PATCH v2 2/2] kms_content_protection: Add Content Protection test
Ramalingam C
ramalingam.c at intel.com
Sat Jan 6 15:52:52 UTC 2018
On Saturday 06 January 2018 08:34 PM, Sean Paul wrote:
> On Sat, Jan 6, 2018 at 8:10 AM, Ramalingam C <ramalingam.c at 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 at 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 at 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 at 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',
>>>>>>>
More information about the Intel-gfx
mailing list