> -----Original Message-----
> From: Roper, Matthew D <matthew.d.roper@xxxxxxxxx>
> Sent: Monday, November 4, 2024 11:45 PM
> To: Kandpal, Suraj <suraj.kandpal@xxxxxxxxx>
> Cc: intel-xe@xxxxxxxxxxxxxxxxxxxxx; intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH 2/2] drm/i915/hdcp: Handle HDCP Line Rekeying for HDCP
> 1.4
>
> On Mon, Nov 04, 2024 at 02:01:43PM +0530, Suraj Kandpal wrote:
> > TRANS_DDI_FUNC_CTL asks us to disable hdcp line rekeying when not in
> > hdcp 2.2 and we are not using an hdmi transcoder and it need to be
> > enabled when we are using an HDMI transcoder to enable HDCP 1.4.
> > We use intel_de_rmw cycles to update TRANS_DDI_FUNC_CTL register so
> we
> > cannot depend on the value being 0 by default everytime which calls
> > for seprate handling of HDCP 1.4 case.
> >
> > Bspec: 69964, 50493, 50054
> > Signed-off-by: Suraj Kandpal <suraj.kandpal@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/display/intel_hdcp.c | 28
> > +++++++++++++++++++++++
> > 1 file changed, 28 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_hdcp.c
> > b/drivers/gpu/drm/i915/display/intel_hdcp.c
> > index 8bca532d1176..54efba65ef5a 100644
> > --- a/drivers/gpu/drm/i915/display/intel_hdcp.c
> > +++ b/drivers/gpu/drm/i915/display/intel_hdcp.c
> > @@ -31,6 +31,32 @@
> > #define KEY_LOAD_TRIES 5
> > #define HDCP2_LC_RETRY_CNT 3
> >
> > +static void
> > +intel_hdcp_enable_hdcp_line_rekeying(struct intel_encoder *encoder,
> > + struct intel_hdcp *hdcp)
> > +{
> > + struct intel_display *display = to_intel_display(encoder);
> > +
> > + /* Here we assume HDMI is in TMDS mode of operation */
> > + if (encoder->type != INTEL_OUTPUT_HDMI)
> > + return;
> > +
> > + if (DISPLAY_VER(display) >= 14) {
>
> As noted on the previous patch, this outer 'if' doesn't do anything since none
> of the nested if's will match versions less than 14.
>
> > + if (DISPLAY_VER(display) >= 30)
> > + intel_de_rmw(display,
> > + TRANS_DDI_FUNC_CTL(display, hdcp-
> >cpu_transcoder),
> > +
> XE3_TRANS_DDI_HDCP_LINE_REKEY_DISABLE, 0);
> > + else if (IS_DISPLAY_VERx100_STEP(display, 1400, STEP_D0,
> STEP_FOREVER))
> > + intel_de_rmw(display, MTL_CHICKEN_TRANS(hdcp-
> >cpu_transcoder),
> > + HDCP_LINE_REKEY_DISABLE, 0);
> > + else if (IS_DISPLAY_VERx100_STEP(display, 1401, STEP_B0,
> STEP_FOREVER) ||
> > + IS_DISPLAY_VERx100_STEP(display, 2000, STEP_B0,
> STEP_FOREVER))
>
> For new code we should definitely be ordering if/else ladders in descending
> order. So the Xe2 clause here should come before the MTL clause.
>
> Although it might be cleaner to just have a single function that takes a
> boolean parameter to enable/disable rekeying? E.g., something along the
> lines of:
>
> static void
> intel_hdcp_adjust_hdcp_line_rekeying(struct intel_encoder *encoder,
> struct intel_hdcp *hdcp,
> bool enable)
> {
> struct intel_reg reky_reg;
> u32 rekey_bit;
>
> if (DISPLAY_VER(display) >= 30) {
> rekey_reg = TRANS_DDI_FUNC_CTL;
> rekey_bit = XE3_TRANS_DDI_HDCP_LINE_REKEY_DISABLE;
> } else if (DISPLAY_VERx100(display) >= 1401) {
> rekey_reg = TRANS_DDI_FUNC_CTL;
> rekey_bit = TRANS_DDI_HDCP_LINE_REKEY_DISABLE;
> } else if (DISPLAY_VERx100(display) == 1400)
> rekey_reg = MTL_CHICKEN_TRANS(hdcp->cpu_transcoder);
> rekey_bit = HDCP_LINE_REKEY_DISABLE;
> } else {
> return;
> }
>
> intel_de_rmw(display, rekey_reg, rekey_bit,
> enable ? 0 : rekey_bit);
> }
>
> And we can move the stepping-specific workaround implementation to the
> callsite to make it clear that the implementation of enabling/disabling is
> separate from the decision whether to enable/disable (as impacted by the
> workaround).
Also since the constraints remain same for both enablement and disablement we can use the same if else checks
as before using the above mentioned method if that sounds good.
Regards,
Suraj Kandpal
>
>
> Matt
>
> > + intel_de_rmw(display,
> > + TRANS_DDI_FUNC_CTL(display, hdcp-
> >cpu_transcoder),
> > + TRANS_DDI_HDCP_LINE_REKEY_DISABLE,
> 0);
> > + }
> > +}
> > +
> > static void
> > intel_hdcp_disable_hdcp_line_rekeying(struct intel_encoder *encoder,
> > struct intel_hdcp *hdcp)
> > @@ -1051,6 +1077,8 @@ static int intel_hdcp1_enable(struct
> intel_connector *connector)
> > return ret;
> > }
> >
> > + intel_hdcp_enable_hdcp_line_rekeying(connector->encoder, hdcp);
> > +
> > /* Incase of authentication failures, HDCP spec expects reauth. */
> > for (i = 0; i < tries; i++) {
> > ret = intel_hdcp_auth(connector);
> > --
> > 2.34.1
> >
>
> --
> Matt Roper
> Graphics Software Engineer
> Linux GPU Platform Enablement
> Intel Corporation
