Skip to content
This repository has been archived by the owner on Oct 8, 2024. It is now read-only.

Hdcp 2.2 #471

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Hdcp 2.2 #471

wants to merge 2 commits into from

Conversation

harishkrupo
Copy link
Contributor

No description provided.

@stripes416
Copy link
Contributor

stripes416 commented Oct 17, 2018

@harishkrupo Your series failed Travis CI due to 'check_patch.py' -- basically failing the clang format check. Please run it locally, fix any errors, and resubmit for review. This

@harishkrupo
Copy link
Contributor Author

I ran clang-format-diff-6.0 before submitting. I am not sure why it fails here. maybe a difference in the version used in travis?

@js0701
Copy link
Contributor

js0701 commented Oct 18, 2018

clang-format-diff-3.9 is what I am using.

@js0701
Copy link
Contributor

js0701 commented Oct 18, 2018

@harishkrupo I am also submiiting PR for HDCP for Beta release.
may need your rebase.
however, I agree the basic logic in this PR

@harishkrupo
Copy link
Contributor Author

harishkrupo commented Oct 18, 2018

clang-format-diff-3.9 is what I am using.

Okay will downgrade my tool and will try.

@harishkrupo I am also submitting PR for HDCP for Beta release.
may need your rebase.
however, I agree the basic logic in this PR

Sure, will rebase once you merge it :)

@stripes416
Copy link
Contributor

Travis-Ci uses clang-format 3.8 I think.

@harishkrupo
Copy link
Contributor Author

hmm, there is a discrepancy between clang-format-diff 6.0 and 3.9. I have pushed the above patches checked with 3.9 as that is the version we use in travis.
Not sure whether to accept 6.0's output or 3.9's.

@stripes416
Copy link
Contributor

Yeah, you are right, it does use 3.9. The system that Travis is on runs Xenial 16.04, which uses 3.9. That is currently the recommend OS version for Celadon systems -- so we should stick with that version imo.

stripes416
stripes416 previously approved these changes Oct 19, 2018
Copy link
Contributor

@stripes416 stripes416 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me -- with merge conflicts resolved of course :)

@harishkrupo
Copy link
Contributor Author

@js0701 @stripes416 Patches rebased. Requires another review.

wsi/drm/drmdisplay.cpp Outdated Show resolved Hide resolved
wsi/drm/drmdisplay.cpp Outdated Show resolved Hide resolved
wsi/drm/drmdisplay.cpp Outdated Show resolved Hide resolved
@js0701
Copy link
Contributor

js0701 commented Oct 23, 2018

@harishkrupo HDCP daemon should also be on Linux. Let's sync with VPG/IOTG before merge this.
Thanks

@harishkrupo
Copy link
Contributor Author

harishkrupo commented Oct 23, 2018

@js0701 @stripes416

@js0701 Are you referring to HDCP Daemon?
I gave a quick glance through their code and found some issues (maybe?)

  • When the property is set, they first try to set DRM master. I am not sure if that will be allowed because IAHWC doesn't provide the mechanism to drop the privileges. If that call fails they return EBUSY.
  • They try to set the property 3 times which is okay but they don't wait for ~5 seconds before trying again. This time is mandated by the HDCP 2.2 spec.
  • They don't have a polling thread to identify if the protection is down due to some failure.

I am not against using the daemon. One upside of using it would be that we could completely remove the HDCP bits in IAHWC (SRM, protection, etc) and directly use that as they provide support for these. The downside would be that it will add another dependency to IAHWC.

@js0701
Copy link
Contributor

js0701 commented Oct 25, 2018

@harishkrupo
Romli, Khairul in IOTG is owning HDCP daemon in Android.
We can sync with him for those issues

@harishkrupo
Copy link
Contributor Author

@js0701 Sure.

@stripes416 stripes416 changed the title Hdcp 2.2 Hdcp 2.2 - WIP Oct 25, 2018
Jira: None
Test: ./testlayers -f 120 -j jsonconfigs/kmscube1layer.json &> log.txt
      Check log.txt to see if HDCP 1.4/2.2 was successfully enabled.

Signed-off-by: Harish Krupo <[email protected]>
@harishkrupo
Copy link
Contributor Author

@js0701 I observed that the HDCP daemon runs a thread to continually monitor the HDCP status. However, they don't wait for ~15 seconds before ensuring that the connection is authenticated. I have pushed a patch removing all the thread related code and just simply set the connector property and wait for the said amount of time before declaring failure or success.

@harishkrupo harishkrupo changed the title Hdcp 2.2 - WIP Hdcp 2.2 Nov 12, 2018
With HDCP 2.2 a new connector property is added for marking the content type. This
property controls the type of authentication that the kernel needs to perform.
The content type property takes 2 values, 0 (TYPE0) and 1 (TYPE1). When the client
marks the content as TYPE0, the kernel can choose whether to go for HDCP 1.4 or
HDCP 2.2 authentication but when the client marks the content as TYPE1, the kernel
is only allowed to attempt HDCP 2.2 authentication. Failing which, HDCP fails to
enable.
The spec states that the source needs to wait at least 5 seconds before declaring
success/failure in authentication. Along with this wait, the kernel re-tries 3 times,
thus the compositor needs to wait for ~15 seconds.

Jira: None
Test: Test introduced in jsonlayerstest.cpp in the previous patch
      Test it by running ./testlayers -f 120 -j jsonconfigs/kmscube1layer.json

Change-Id: I5ee780157f074d295701c4f1aff9fc0f6a6d37a8
Signed-off-by: Harish Krupo <[email protected]>
Copy link
Contributor

@stripes416 stripes416 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@harishkrupo This looks good to me -- Good Job! Once you rebase onto current master and it passes Travis, it should be good to merge.

wsi/drm/drmdisplay.cpp Show resolved Hide resolved
@stripes416
Copy link
Contributor

Wait for @js0701 to merge (note to self!)

@js0701
Copy link
Contributor

js0701 commented Nov 14, 2018

@harishkrupo
Current the no checking status mechanism is working well.
We had checked with VPG CP team and IOTG that HWC should only do simple mode settings.
Instead of anything about HDCP protocal itself. The protocal implementatinon should be through
daemon and kernel

@harishkrupo
Copy link
Contributor Author

harishkrupo commented Nov 14, 2018

@js0701 I agree that it is working correctly. It works for me too without any waits. But we shouldn't assume about the type of the connector/sink. We must wait for the mandated time. If not here then it should be done at the daemon.
I didn't see the wait in the daemon so I added the wait here. If I am to remove this, then I think it is a fair expectation that a corresponding change must be done on the Daemon side.

}

if ((current_protection_support_ && val == HWCContentProtection::kEnabled) ||
(!current_protection_support_ && val == HWCContentProtection::kDesired))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when and how the status will change from kDesired to kEnabled? Is it possible in the sleep time frame it is already changed to kEnabled, you can not get the kDesired state.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry, could you please explain more about the sleep time frame?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean in the SetHDCPState() there is 'usleep(100 * 1000);' for every iteration. is it possible in this time frame, the state changes from kUnDisired->kDesired->kEnabled? for such case, your logic in Check state will not return true.

Copy link

@kromli kromli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a question regarding this changes. Although spec mentioned that it needs 5.1 sec for the aunthencation process, have this changes been validated against HDCP Compliance test?

I would rather have the KMD able to pass the compliance test before we could have hwcomposer changes in.

@js0701
Copy link
Contributor

js0701 commented Feb 27, 2019

@kromli
Can you check HDCP daemon has wait logic or not? As we talked in the email, the HWC should not worry about HDCP protocal itself
However, we still have the consern @harishkrupo raised here

}

drmModeConnectorSetProperty(gpu_fd_, connector_, hdcp_id_prop_, value);
ETRACE("Ignored Content type. \n");
/* Wait for Enable should be 5.1 Sec * 3(kernel reattempt cnt) */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

possible to wait up to 15s blocking hwc service such long time seems not acceptable. I would suggest not blocking in hwc.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, agreed. This should be done by the hdcp daemon. I had previously raised it as a concern.
Once the daemon does it, We can remove this here.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants