[PATCH 2/2] drm/i915/hdcp: Handle HDCP Line Rekeying for HDCP 1.4

[Kandpal, Suraj]

> -----Original Message-----
> From: Roper, Matthew D <matthew.d.roper at intel.com>
> Sent: Monday, November 4, 2024 11:45 PM
> To: Kandpal, Suraj <suraj.kandpal at intel.com>
> Cc: intel-xe at lists.freedesktop.org; intel-gfx at lists.freedesktop.org
> 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 at intel.com>
> > ---
> >  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).

Sure will separate this out as a different patch altogether as for the stepping specific implementation
In bspec 50054 it specifically state for which steppings MTL_CHICKEN_TRANS needs to have its bit set/unset

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