Re: [PATCH v4 0/9] drm/i915: Implement HDCP

From: Sean Paul <seanpaul@chromium.org>
To: Jose Abreu <Jose.Abreu@synopsys.com>
Cc: dri-devel <dri-devel@lists.freedesktop.org>,
	Intel Graphics Development <intel-gfx@lists.freedesktop.org>,
	Daniel Vetter <daniel.vetter@intel.com>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v4 0/9] drm/i915: Implement HDCP
Date: Mon, 18 Dec 2017 14:46:35 -0500	[thread overview]
Message-ID: <CAOw6vb+6eyk8OWnGY7joNCfe5YwPN3XbZPBCi9x3QfWNJuTOyA@mail.gmail.com> (raw)
In-Reply-To: <d7bbf8ad-a6ac-ddea-5910-0d1a108d7ecd@synopsys.com>

On Thu, Dec 7, 2017 at 5:38 AM, Jose Abreu <Jose.Abreu@synopsys.com> wrote:
> Hi Sean,
>
> On 07-12-2017 00:00, Sean Paul wrote:
>> Welcome to version 4 of the patchset. I think we're nearing the finish line
>> (hopefully) now. This set addresses the review feedback from v3. I applied some
>> R-b's from v3 review, and converted others to Cc since other changes were made
>> to the patch, and I didn't want to speak for reviewers.
>>
>> Thanks for all the review feedback!
>
> Thanks for your patches, nice to see HDCP patches making it into DRM!
>
> I'm not familiar with i915 driver/hw but I do am familiar with
> HDCP for HDMI. Here goes a few notes:

Hi Jose,
Thanks for your feedback. Apologies for the delay, I was out of town
on vacation.

> - You should make sure that you are streaming valid TMDS data
> after reading the BKSV. I think by spec you can read BKSV at any
> point but the remaining operations need to be done *after* you
> start sending video, otherwise you can end up with wrong Ri's
> because Ri rotation is done using TMDS clock...

HDCP is occurring after modeset and link training (in DP case), so
we're fine in this respect.

> - Also according to spec you should make sure BSTATUS reports DVI
> state in first read (i.e. before start sending video)

I can't find this in the spec. Can you provide a pointer?

> and that
> HDMI_RESERVED field is tied to one.

I don't think this is required by the spec, and the bit doesn't exist
for HDCP over DP.

> - I think you should clearly indicate that this is for HDCP 1.4
> because HDCP 2.2 is a lot different.

HDCP version is coming in a follow-on set. See previous versions of
this set for the discussion.

>
> Also, is there any possibility to port any of these functions for
> main DRM core? I mean, not the shim ops which seem very specific
> for your HW, but at least a .enable, .disable, .link_check
> callbacks would be useful to help others to also implement HDCP ...
>

This was also discussed in previous versions. Hardware HDCP
implementations vary wildly, so there's not much to share. For the
time being, let's keep this in i915 until we have another platform
that can benefit from shared code.

Thanks again for your review!

Sean

> Best Regards,
> Jose Miguel Abreu
>
>>
>> Sean
>>
>> Sean Paul (9):
>>   drm: Fix link-status kerneldoc line lengths
>>   drm/i915: Add more control to wait_for routines
>>   drm: Add Content Protection property
>>   drm: Add some HDCP related #defines
>>   drm/i915: Add HDCP framework + base implementation
>>   drm/i915: Make use of indexed write GMBUS feature
>>   drm/i915: Add function to output Aksv over GMBUS
>>   drm/i915: Implement HDCP for HDMI
>>   drm/i915: Implement HDCP for DisplayPort
>>
>>  drivers/gpu/drm/drm_atomic.c         |   8 +
>>  drivers/gpu/drm/drm_connector.c      |  87 ++++-
>>  drivers/gpu/drm/drm_sysfs.c          |   1 +
>>  drivers/gpu/drm/i915/Makefile        |   1 +
>>  drivers/gpu/drm/i915/i915_drv.h      |   1 +
>>  drivers/gpu/drm/i915/i915_reg.h      |  85 ++++
>>  drivers/gpu/drm/i915/intel_atomic.c  |   2 +
>>  drivers/gpu/drm/i915/intel_ddi.c     |  36 ++
>>  drivers/gpu/drm/i915/intel_display.c |   4 +
>>  drivers/gpu/drm/i915/intel_dp.c      | 244 +++++++++++-
>>  drivers/gpu/drm/i915/intel_drv.h     | 106 ++++-
>>  drivers/gpu/drm/i915/intel_hdcp.c    | 735 +++++++++++++++++++++++++++++++++++
>>  drivers/gpu/drm/i915/intel_hdmi.c    | 250 ++++++++++++
>>  drivers/gpu/drm/i915/intel_i2c.c     |  81 +++-
>>  drivers/gpu/drm/i915/intel_uncore.c  |  23 +-
>>  drivers/gpu/drm/i915/intel_uncore.h  |  14 +-
>>  include/drm/drm_connector.h          |  16 +
>>  include/drm/drm_dp_helper.h          |  17 +
>>  include/drm/drm_hdcp.h               |  56 +++
>>  include/uapi/drm/drm_mode.h          |   4 +
>>  20 files changed, 1728 insertions(+), 43 deletions(-)
>>  create mode 100644 drivers/gpu/drm/i915/intel_hdcp.c
>>  create mode 100644 include/drm/drm_hdcp.h
>>
>