| Message ID | 20241104083143.631760-1-suraj.kandpal@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] drm/i915/xe3lpd: Disable HDCP Line Rekeying for Xe3 | expand |
On Mon, Nov 04, 2024 at 02:01:42PM +0530, Suraj Kandpal wrote: > We need to disable HDCP Line Rekeying for Xe3 when we are using an HDMI > encoder. Also remove the Wa comment tag as this follows the bspec and > does not implement the wa. > > 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. > v5: Add the bspec link and remove the Wa comment tag > v6: Rebase over new changes > > Bspec: 69964 > Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com> I think the code changes are fine now (although you seem to have dropped the general ladder re-order that was done in v4; was that intentional or a mistake?), but the subject/commit message seem misleading since the change this patch is making isn't changing if/when rekeying is disabled, all it's doing is changing the bit used. So a commit message more along the lines of drm/i915/xe3lpd: Update HDCP rekying bit The TRANS_DDI_FUNC_CTL bit used to enable/disable HDCP rekeying has moved from bit 12 (Xe2) to bit 15 (Xe3); update the RMW toggle accordingly. Also drop the misleading workaround comment tag on this function since disabling of HDCP rekeying is something that happens on all platforms, not just those impacted by that workaround. If you decide to reinstate the ladder re-order that you had on an earlier version of this patch, you could add another sentence like While we're here, also re-order the if/else ladder to use standard "newest platform first" order. Anyway, with some kind of commit message cleanup, Reviewed-by: Matt Roper <matthew.d.roper@intel.com> Matt > --- > drivers/gpu/drm/i915/display/intel_hdcp.c | 7 +++++-- > drivers/gpu/drm/i915/i915_reg.h | 1 + > 2 files changed, 6 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_hdcp.c b/drivers/gpu/drm/i915/display/intel_hdcp.c > index f6d42ec6949e..8bca532d1176 100644 > --- a/drivers/gpu/drm/i915/display/intel_hdcp.c > +++ b/drivers/gpu/drm/i915/display/intel_hdcp.c > @@ -31,7 +31,6 @@ > #define KEY_LOAD_TRIES 5 > #define HDCP2_LC_RETRY_CNT 3 > > -/* WA: 16022217614 */ > static void > intel_hdcp_disable_hdcp_line_rekeying(struct intel_encoder *encoder, > struct intel_hdcp *hdcp) > @@ -43,7 +42,11 @@ intel_hdcp_disable_hdcp_line_rekeying(struct intel_encoder *encoder, > return; > > if (DISPLAY_VER(display) >= 14) { > - if (IS_DISPLAY_VERx100_STEP(display, 1400, STEP_D0, STEP_FOREVER)) > + 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_VERx100_STEP(display, 1400, STEP_D0, STEP_FOREVER)) > intel_de_rmw(display, MTL_CHICKEN_TRANS(hdcp->cpu_transcoder), > 0, HDCP_LINE_REKEY_DISABLE); > else if (IS_DISPLAY_VERx100_STEP(display, 1401, STEP_B0, STEP_FOREVER) || > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index 22be4a731d27..c160e087972a 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -3819,6 +3819,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.34.1 >
On Mon, Nov 04, 2024 at 09:54:39AM -0800, Matt Roper wrote: > On Mon, Nov 04, 2024 at 02:01:42PM +0530, Suraj Kandpal wrote: > > We need to disable HDCP Line Rekeying for Xe3 when we are using an HDMI > > encoder. Also remove the Wa comment tag as this follows the bspec and > > does not implement the wa. > > > > 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. > > v5: Add the bspec link and remove the Wa comment tag > > v6: Rebase over new changes > > > > Bspec: 69964 > > Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com> > > I think the code changes are fine now (although you seem to have dropped > the general ladder re-order that was done in v4; was that intentional or > a mistake?), but the subject/commit message seem misleading since the > change this patch is making isn't changing if/when rekeying is disabled, > all it's doing is changing the bit used. So a commit message more along > the lines of > > drm/i915/xe3lpd: Update HDCP rekying bit > > The TRANS_DDI_FUNC_CTL bit used to enable/disable HDCP rekeying > has moved from bit 12 (Xe2) to bit 15 (Xe3); update the RMW > toggle accordingly. > > Also drop the misleading workaround comment tag on this function > since disabling of HDCP rekeying is something that happens on > all platforms, not just those impacted by that workaround. > > If you decide to reinstate the ladder re-order that you had on an > earlier version of this patch, you could add another sentence like > > While we're here, also re-order the if/else ladder to use > standard "newest platform first" order. > > Anyway, with some kind of commit message cleanup, > > Reviewed-by: Matt Roper <matthew.d.roper@intel.com> > > > Matt > > > --- > > drivers/gpu/drm/i915/display/intel_hdcp.c | 7 +++++-- > > drivers/gpu/drm/i915/i915_reg.h | 1 + > > 2 files changed, 6 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_hdcp.c b/drivers/gpu/drm/i915/display/intel_hdcp.c > > index f6d42ec6949e..8bca532d1176 100644 > > --- a/drivers/gpu/drm/i915/display/intel_hdcp.c > > +++ b/drivers/gpu/drm/i915/display/intel_hdcp.c > > @@ -31,7 +31,6 @@ > > #define KEY_LOAD_TRIES 5 > > #define HDCP2_LC_RETRY_CNT 3 > > > > -/* WA: 16022217614 */ > > static void > > intel_hdcp_disable_hdcp_line_rekeying(struct intel_encoder *encoder, > > struct intel_hdcp *hdcp) > > @@ -43,7 +42,11 @@ intel_hdcp_disable_hdcp_line_rekeying(struct intel_encoder *encoder, > > return; > > > > if (DISPLAY_VER(display) >= 14) { Actually one other thing...do we even need this outer 'if?' All of the nested conditions work on specific versions that are >= 14 already, so it doesn't appear that this has any effect. Matt > > - if (IS_DISPLAY_VERx100_STEP(display, 1400, STEP_D0, STEP_FOREVER)) > > + 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_VERx100_STEP(display, 1400, STEP_D0, STEP_FOREVER)) > > intel_de_rmw(display, MTL_CHICKEN_TRANS(hdcp->cpu_transcoder), > > 0, HDCP_LINE_REKEY_DISABLE); > > else if (IS_DISPLAY_VERx100_STEP(display, 1401, STEP_B0, STEP_FOREVER) || > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > > index 22be4a731d27..c160e087972a 100644 > > --- a/drivers/gpu/drm/i915/i915_reg.h > > +++ b/drivers/gpu/drm/i915/i915_reg.h > > @@ -3819,6 +3819,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.34.1 > > > > -- > Matt Roper > Graphics Software Engineer > Linux GPU Platform Enablement > Intel Corporation
> -----Original Message----- > From: Roper, Matthew D <matthew.d.roper@intel.com> > Sent: Monday, November 4, 2024 11:25 PM > To: Kandpal, Suraj <suraj.kandpal@intel.com> > Cc: intel-xe@lists.freedesktop.org; intel-gfx@lists.freedesktop.org > Subject: Re: [PATCH 1/2] drm/i915/xe3lpd: Disable HDCP Line Rekeying for Xe3 > > On Mon, Nov 04, 2024 at 02:01:42PM +0530, Suraj Kandpal wrote: > > We need to disable HDCP Line Rekeying for Xe3 when we are using an > > HDMI encoder. Also remove the Wa comment tag as this follows the bspec > > and does not implement the wa. > > > > 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. > > v5: Add the bspec link and remove the Wa comment tag > > v6: Rebase over new changes > > > > Bspec: 69964 > > Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com> > > I think the code changes are fine now (although you seem to have dropped > the general ladder re-order that was done in v4; was that intentional or a > mistake?), but the subject/commit message seem misleading since the > change this patch is making isn't changing if/when rekeying is disabled, all it's > doing is changing the bit used. So a commit message more along the lines of > > drm/i915/xe3lpd: Update HDCP rekying bit > > The TRANS_DDI_FUNC_CTL bit used to enable/disable HDCP rekeying > has moved from bit 12 (Xe2) to bit 15 (Xe3); update the RMW > toggle accordingly. > > Also drop the misleading workaround comment tag on this function > since disabling of HDCP rekeying is something that happens on > all platforms, not just those impacted by that workaround. > > If you decide to reinstate the ladder re-order that you had on an earlier > version of this patch, you could add another sentence like > > While we're here, also re-order the if/else ladder to use > standard "newest platform first" order. > Sure will reladder the checks and update the commit message accordingly must have missed it due to the rebase. Regards, Suraj Kandpal > Anyway, with some kind of commit message cleanup, > > Reviewed-by: Matt Roper <matthew.d.roper@intel.com> > > > Matt > > > --- > > drivers/gpu/drm/i915/display/intel_hdcp.c | 7 +++++-- > > drivers/gpu/drm/i915/i915_reg.h | 1 + > > 2 files changed, 6 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_hdcp.c > > b/drivers/gpu/drm/i915/display/intel_hdcp.c > > index f6d42ec6949e..8bca532d1176 100644 > > --- a/drivers/gpu/drm/i915/display/intel_hdcp.c > > +++ b/drivers/gpu/drm/i915/display/intel_hdcp.c > > @@ -31,7 +31,6 @@ > > #define KEY_LOAD_TRIES 5 > > #define HDCP2_LC_RETRY_CNT 3 > > > > -/* WA: 16022217614 */ > > static void > > intel_hdcp_disable_hdcp_line_rekeying(struct intel_encoder *encoder, > > struct intel_hdcp *hdcp) > > @@ -43,7 +42,11 @@ intel_hdcp_disable_hdcp_line_rekeying(struct > intel_encoder *encoder, > > return; > > > > if (DISPLAY_VER(display) >= 14) { > > - if (IS_DISPLAY_VERx100_STEP(display, 1400, STEP_D0, > STEP_FOREVER)) > > + 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_VERx100_STEP(display, 1400, STEP_D0, > > +STEP_FOREVER)) > > intel_de_rmw(display, MTL_CHICKEN_TRANS(hdcp- > >cpu_transcoder), > > 0, HDCP_LINE_REKEY_DISABLE); > > else if (IS_DISPLAY_VERx100_STEP(display, 1401, STEP_B0, > > STEP_FOREVER) || diff --git a/drivers/gpu/drm/i915/i915_reg.h > > b/drivers/gpu/drm/i915/i915_reg.h index 22be4a731d27..c160e087972a > > 100644 > > --- a/drivers/gpu/drm/i915/i915_reg.h > > +++ b/drivers/gpu/drm/i915/i915_reg.h > > @@ -3819,6 +3819,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.34.1 > > > > -- > Matt Roper > Graphics Software Engineer > Linux GPU Platform Enablement > Intel Corporation
> -----Original Message----- > From: Roper, Matthew D <matthew.d.roper@intel.com> > Sent: Monday, November 4, 2024 11:35 PM > To: Kandpal, Suraj <suraj.kandpal@intel.com> > Cc: intel-xe@lists.freedesktop.org; intel-gfx@lists.freedesktop.org > Subject: Re: [PATCH 1/2] drm/i915/xe3lpd: Disable HDCP Line Rekeying for Xe3 > > On Mon, Nov 04, 2024 at 09:54:39AM -0800, Matt Roper wrote: > > On Mon, Nov 04, 2024 at 02:01:42PM +0530, Suraj Kandpal wrote: > > > We need to disable HDCP Line Rekeying for Xe3 when we are using an > > > HDMI encoder. Also remove the Wa comment tag as this follows the > > > bspec and does not implement the wa. > > > > > > 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. > > > v5: Add the bspec link and remove the Wa comment tag > > > v6: Rebase over new changes > > > > > > Bspec: 69964 > > > Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com> > > > > I think the code changes are fine now (although you seem to have > > dropped the general ladder re-order that was done in v4; was that > > intentional or a mistake?), but the subject/commit message seem > > misleading since the change this patch is making isn't changing > > if/when rekeying is disabled, all it's doing is changing the bit used. > > So a commit message more along the lines of > > > > drm/i915/xe3lpd: Update HDCP rekying bit > > > > The TRANS_DDI_FUNC_CTL bit used to enable/disable HDCP rekeying > > has moved from bit 12 (Xe2) to bit 15 (Xe3); update the RMW > > toggle accordingly. > > > > Also drop the misleading workaround comment tag on this function > > since disabling of HDCP rekeying is something that happens on > > all platforms, not just those impacted by that workaround. > > > > If you decide to reinstate the ladder re-order that you had on an > > earlier version of this patch, you could add another sentence like > > > > While we're here, also re-order the if/else ladder to use > > standard "newest platform first" order. > > > > Anyway, with some kind of commit message cleanup, > > > > Reviewed-by: Matt Roper <matthew.d.roper@intel.com> > > > > > > Matt > > > > > --- > > > drivers/gpu/drm/i915/display/intel_hdcp.c | 7 +++++-- > > > drivers/gpu/drm/i915/i915_reg.h | 1 + > > > 2 files changed, 6 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_hdcp.c > > > b/drivers/gpu/drm/i915/display/intel_hdcp.c > > > index f6d42ec6949e..8bca532d1176 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_hdcp.c > > > +++ b/drivers/gpu/drm/i915/display/intel_hdcp.c > > > @@ -31,7 +31,6 @@ > > > #define KEY_LOAD_TRIES 5 > > > #define HDCP2_LC_RETRY_CNT 3 > > > > > > -/* WA: 16022217614 */ > > > static void > > > intel_hdcp_disable_hdcp_line_rekeying(struct intel_encoder *encoder, > > > struct intel_hdcp *hdcp) > > > @@ -43,7 +42,11 @@ intel_hdcp_disable_hdcp_line_rekeying(struct > intel_encoder *encoder, > > > return; > > > > > > if (DISPLAY_VER(display) >= 14) { > > Actually one other thing...do we even need this outer 'if?' All of the nested > conditions work on specific versions that are >= 14 already, so it doesn't > appear that this has any effect. > Added that in to avoid the code for display_ver < 14 from going through the extra unnecessary If and else if block which it will end up failing anyways also the ladder may grow if the bit gets changed again for the future platforms. Regards, Suraj Kandpal > > Matt > > > > - if (IS_DISPLAY_VERx100_STEP(display, 1400, STEP_D0, > STEP_FOREVER)) > > > + 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_VERx100_STEP(display, 1400, STEP_D0, > > > +STEP_FOREVER)) > > > intel_de_rmw(display, MTL_CHICKEN_TRANS(hdcp- > >cpu_transcoder), > > > 0, HDCP_LINE_REKEY_DISABLE); > > > else if (IS_DISPLAY_VERx100_STEP(display, 1401, STEP_B0, > > > STEP_FOREVER) || diff --git a/drivers/gpu/drm/i915/i915_reg.h > > > b/drivers/gpu/drm/i915/i915_reg.h index 22be4a731d27..c160e087972a > > > 100644 > > > --- a/drivers/gpu/drm/i915/i915_reg.h > > > +++ b/drivers/gpu/drm/i915/i915_reg.h > > > @@ -3819,6 +3819,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.34.1 > > > > > > > -- > > Matt Roper > > Graphics Software Engineer > > Linux GPU Platform Enablement > > Intel Corporation > > -- > Matt Roper > Graphics Software Engineer > Linux GPU Platform Enablement > Intel Corporation
diff --git a/drivers/gpu/drm/i915/display/intel_hdcp.c b/drivers/gpu/drm/i915/display/intel_hdcp.c index f6d42ec6949e..8bca532d1176 100644 --- a/drivers/gpu/drm/i915/display/intel_hdcp.c +++ b/drivers/gpu/drm/i915/display/intel_hdcp.c @@ -31,7 +31,6 @@ #define KEY_LOAD_TRIES 5 #define HDCP2_LC_RETRY_CNT 3 -/* WA: 16022217614 */ static void intel_hdcp_disable_hdcp_line_rekeying(struct intel_encoder *encoder, struct intel_hdcp *hdcp) @@ -43,7 +42,11 @@ intel_hdcp_disable_hdcp_line_rekeying(struct intel_encoder *encoder, return; if (DISPLAY_VER(display) >= 14) { - if (IS_DISPLAY_VERx100_STEP(display, 1400, STEP_D0, STEP_FOREVER)) + 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_VERx100_STEP(display, 1400, STEP_D0, STEP_FOREVER)) intel_de_rmw(display, MTL_CHICKEN_TRANS(hdcp->cpu_transcoder), 0, HDCP_LINE_REKEY_DISABLE); else if (IS_DISPLAY_VERx100_STEP(display, 1401, STEP_B0, STEP_FOREVER) || diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 22be4a731d27..c160e087972a 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -3819,6 +3819,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)
We need to disable HDCP Line Rekeying for Xe3 when we are using an HDMI encoder. Also remove the Wa comment tag as this follows the bspec and does not implement the wa. 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. v5: Add the bspec link and remove the Wa comment tag v6: Rebase over new changes Bspec: 69964 Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com> --- drivers/gpu/drm/i915/display/intel_hdcp.c | 7 +++++-- drivers/gpu/drm/i915/i915_reg.h | 1 + 2 files changed, 6 insertions(+), 2 deletions(-)