[PATCH v4 3/7] drm/i915/xe3lpd: Disable HDCP Line Rekeying for Xe3

[Kandpal, Suraj]

> -----Original Message-----
> From: Roper, Matthew D <matthew.d.roper at intel.com>
> Sent: Saturday, October 26, 2024 12:28 AM
> To: Kandpal, Suraj <suraj.kandpal at intel.com>
> Cc: Atwood, Matthew S <matthew.s.atwood at intel.com>; intel-
> xe at lists.freedesktop.org; intel-gfx at lists.freedesktop.org
> Subject: Re: [PATCH v4 3/7] drm/i915/xe3lpd: Disable HDCP Line Rekeying for
> Xe3
> 
> On Thu, Oct 24, 2024 at 02:52:14AM +0000, Kandpal, Suraj wrote:
> >
> >
> > > -----Original Message-----
> > > From: Roper, Matthew D <matthew.d.roper at intel.com>
> > > Sent: Wednesday, October 23, 2024 11:22 PM
> > > To: Atwood, Matthew S <matthew.s.atwood at intel.com>
> > > Cc: intel-xe at lists.freedesktop.org; intel-gfx at lists.freedesktop.org;
> > > Kandpal, Suraj <suraj.kandpal at intel.com>
> > > Subject: Re: [PATCH v4 3/7] drm/i915/xe3lpd: Disable HDCP Line
> > > Rekeying for
> > > Xe3
> > >
> > > On Fri, Oct 18, 2024 at 01:03:07PM -0700, Matt Atwood wrote:
> > > > From: Suraj Kandpal <suraj.kandpal at intel.com>
> > > >
> > > > We need to disable HDCP Line Rekeying for Xe3 when we are using an
> > > > HDMI encoder.
> > >
> > > This is still missing the "why" for this change.  Is there a bspec
> > > reference that gives the details?  From the description of the bit
> > > itself, it sounds like the setting here (for both Xe3 and earlier
> > > Xe2) should be based on the HDCP version rather than the
> platform/stepping.
> > >
> > > As mentioned previously, this entire function is labeled as "/* WA:
> > > 16022217614 */"  If we're now using this function for something
> > > other than that specific workaround, then we need to fix/move that
> comment.
> > >
> > >
> >
> > Bspec: 68933
> 
> I think you pasted the wrong number here?  This is a generic page that just
> has links to four transcoder DDI registers and nothing else.  It doesn't have
> anything to do with HDCP rekeying.
> 
> Maybe you meant 69964 (which is one of the four links from the page
> above) that gives the register definition of TRANS_DDI_FUNC_CTL?  But the
> info there implies that we're not really handling this properly since it says that
> we need to enable/disable rekeying based on the HDCP version.  We're
> disabling for HDCP 2.0 and above here (correct), but I don't see where we're
> handling the enabling for HDCP 1.4 and earlier?
> Unless I'm overlooking something, it seems like the driver always updates
> TRANS_DDI_FUNC_CTL with a rmw cycle rather than building a new value
> from scratch, so we can't really rely on the bit being 0 by default for the cases
> where rekeying should be enabled.

>From what I can see TRANS_DDI_FUNC_CTL is written via intel_ddi_enable_transcoder_func()
Which fills in the values to be written by intel_ddi_transcoder_func_reg_val_get where the line rekey bit
ends up being 0 by default which make me believe that separate handling for HDCP 1.4 case may not be required.

Regards,
Suraj Kandpal 

> 
> 
> Matt
> 
> >
> >
> > > Matt
> > >
> > > >
> > > > v2: add additional definition instead of function, commit message
> > > > typo fix and update.
> > > > v3: restore lost conditional from v2.
> > > > v4: subject line and subject message updated, fix the if ladder
> > > > order, fix the bit definition order.
> > > >
> > > > Signed-off-by: Suraj Kandpal <suraj.kandpal at intel.com>
> > > > Signed-off-by: Matt Atwood <matthew.s.atwood at intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/display/intel_hdcp.c | 10 +++++++---
> > > >  drivers/gpu/drm/i915/i915_reg.h           |  1 +
> > > >  2 files changed, 8 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_hdcp.c
> > > > b/drivers/gpu/drm/i915/display/intel_hdcp.c
> > > > index ed6aa87403e2..70dfc9d4d6ac 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_hdcp.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_hdcp.c
> > > > @@ -43,14 +43,18 @@ intel_hdcp_disable_hdcp_line_rekeying(struct
> > > intel_encoder *encoder,
> > > >  		return;
> > > >
> > > >  	if (DISPLAY_VER(display) >= 14) {
> > > > -		if (IS_DISPLAY_VER_STEP(display, IP_VER(14, 0), STEP_D0,
> > > STEP_FOREVER))
> > > > -			intel_de_rmw(display, MTL_CHICKEN_TRANS(hdcp-
> > > >cpu_transcoder),
> > > > -				     0, HDCP_LINE_REKEY_DISABLE);
> > > > +		if (DISPLAY_VER(display) >= 30)
> > > > +			intel_de_rmw(display,
> > > > +				     TRANS_DDI_FUNC_CTL(display, hdcp-
> > > >cpu_transcoder),
> > > > +				     0,
> > > XE3_TRANS_DDI_HDCP_LINE_REKEY_DISABLE);
> > > >  		else if (IS_DISPLAY_VER_STEP(display, IP_VER(14, 1), STEP_B0,
> > > STEP_FOREVER) ||
> > > >  			 IS_DISPLAY_VER_STEP(display, IP_VER(20, 0),
> > > STEP_B0, STEP_FOREVER))
> > > >  			intel_de_rmw(display,
> > > >  				     TRANS_DDI_FUNC_CTL(display, hdcp-
> cpu_transcoder),
> > > >  				     0,
> > > TRANS_DDI_HDCP_LINE_REKEY_DISABLE);
> > > > +		else if (IS_DISPLAY_VER_STEP(display, IP_VER(14, 0), STEP_D0,
> > > STEP_FOREVER))
> > > > +			intel_de_rmw(display, MTL_CHICKEN_TRANS(hdcp-
> > > >cpu_transcoder),
> > > > +				     0, HDCP_LINE_REKEY_DISABLE);
> > > >  	}
> > > >  }
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > > > b/drivers/gpu/drm/i915/i915_reg.h index 89e4381f8baa..8d758947f301
> > > > 100644
> > > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > > @@ -3817,6 +3817,7 @@ enum skl_power_gate {
> > > >  #define  TRANS_DDI_PVSYNC		(1 << 17)
> > > >  #define  TRANS_DDI_PHSYNC		(1 << 16)
> > > >  #define  TRANS_DDI_PORT_SYNC_ENABLE	REG_BIT(15)
> > > > +#define  XE3_TRANS_DDI_HDCP_LINE_REKEY_DISABLE	REG_BIT(15)
> > > >  #define  TRANS_DDI_EDP_INPUT_MASK	(7 << 12)
> > > >  #define  TRANS_DDI_EDP_INPUT_A_ON	(0 << 12)
> > > >  #define  TRANS_DDI_EDP_INPUT_A_ONOFF	(4 << 12)
> > > > --
> > > > 2.45.0
> > > >
> > >
> > > --
> > > Matt Roper
> > > Graphics Software Engineer
> > > Linux GPU Platform Enablement
> > > Intel Corporation
> 
> --
> Matt Roper
> Graphics Software Engineer
> Linux GPU Platform Enablement
> Intel Corporation