[PATCH v3 3/7] drm/i915/xe3lpd: Include hblank restriction for xe3lpd

[Kandpal, Suraj]

> -----Original Message-----
> From: Roper, Matthew D <matthew.d.roper at intel.com>
> Sent: Wednesday, October 16, 2024 8:58 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 v3 3/7] drm/i915/xe3lpd: Include hblank restriction for
> xe3lpd
> 
> On Tue, Oct 15, 2024 at 04:11:20PM -0700, Matt Atwood wrote:
> > From: Suraj Kandpal <suraj.kandpal at intel.com>
> >
> > hblank restriction now includes all of xe3.
> >
> > v2: add additional definition instead of function, commit message typo
> > fix and update.
> > v3: restore lost conditional from v2.
> >
> > 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 | 4 ++++
> >  drivers/gpu/drm/i915/i915_reg.h           | 1 +
> >  2 files changed, 5 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_hdcp.c
> > b/drivers/gpu/drm/i915/display/intel_hdcp.c
> > index ed6aa87403e2..a99b41f258e4 100644
> > --- a/drivers/gpu/drm/i915/display/intel_hdcp.c
> > +++ b/drivers/gpu/drm/i915/display/intel_hdcp.c
> > @@ -51,6 +51,10 @@ intel_hdcp_disable_hdcp_line_rekeying(struct
> intel_encoder *encoder,
> >  			intel_de_rmw(display,
> >  				     TRANS_DDI_FUNC_CTL(display, hdcp-
> >cpu_transcoder),
> >  				     0,
> TRANS_DDI_HDCP_LINE_REKEY_DISABLE);
> > +		else if (DISPLAY_VER(display) >=30)
> 
> We might want to take this opportunity to update the if/else ladder with
> standard "newest platform first" rather than continuing to stack these things
> in the wrong order.  Also, you're missing a space after the comparison here.

Okay got it will get the if ladder fixed here

> 
> But more fundamentally I still think there needs to be more justification in
> the commit message for this.  Right now we're editing a function that's
> labelled "/* WA: 16022217614 */" which implies we shouldn't be doing real
> work here on platforms that the workaround doesn't apply to (as is the case
> for Xe3).  So we probably need to move that comment down to the specific
> conditions for the older platforms that are impacted (and maybe re-write it
> in standard Wa_16022217614 notation to make it more greppable), and also
> add a bspec reference to the commit message that reviewers can check to
> confirm that we really should be disabling rekeying on platforms not
> impacted by the workaround.

You are right this the commit message and subject here should have been
"Disable HDCP Line Rekeying for Xe3"

And "We need to disable HDCP Line Rekeying for Xe3 when we are using an HDMI encoder"
as this is what the bspec expects of us and really not the WA (which was applied only for a few gen for the early stepping versions)
we are really not restricting the hblank here since this pruning modes based on if HDCP is enabled at this point wont be possible.
As HDCP get enabled after modes are already pruned so when modes are being pruned HDCP will not be disabled.

Regards,
Suraj Kandpal
> 
> > +			intel_de_rmw(display,
> > +				     TRANS_DDI_FUNC_CTL(display, hdcp-
> >cpu_transcoder),
> > +				     0,
> XE3_TRANS_DDI_HDCP_LINE_REKEY_DISABLE);
> >  	}
> >  }
> >
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > b/drivers/gpu/drm/i915/i915_reg.h index d30459f8d1cb..fc30e0056b07
> > 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -3833,6 +3833,7 @@ enum skl_power_gate {
> >  #define  TRANS_DDI_EDP_INPUT_C_ONOFF	(6 << 12)
> >  #define  TRANS_DDI_EDP_INPUT_D_ONOFF	(7 << 12)
> >  #define  TRANS_DDI_HDCP_LINE_REKEY_DISABLE	REG_BIT(12)
> > +#define  XE3_TRANS_DDI_HDCP_LINE_REKEY_DISABLE	REG_BIT(15)
> 
> This line isn't sorted properly.
> 
> 
> Matt
> 
> >  #define  TRANS_DDI_MST_TRANSPORT_SELECT_MASK
> 	REG_GENMASK(11, 10)
> >  #define  TRANS_DDI_MST_TRANSPORT_SELECT(trans)	\
> >  	REG_FIELD_PREP(TRANS_DDI_MST_TRANSPORT_SELECT_MASK,
> trans)
> > --
> > 2.45.0
> >
> 
> --
> Matt Roper
> Graphics Software Engineer
> Linux GPU Platform Enablement
> Intel Corporation