| Message ID | 20241018200311.67324-4-matthew.s.atwood@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add xe3lpd edp enabling | expand |
On Fri, Oct 18, 2024 at 01:03:07PM -0700, Matt Atwood wrote: > From: Suraj Kandpal <suraj.kandpal@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. 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@intel.com> > Signed-off-by: Matt Atwood <matthew.s.atwood@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 >
> -----Original Message----- > From: Roper, Matthew D <matthew.d.roper@intel.com> > Sent: Wednesday, October 23, 2024 11:22 PM > To: Atwood, Matthew S <matthew.s.atwood@intel.com> > Cc: intel-xe@lists.freedesktop.org; intel-gfx@lists.freedesktop.org; Kandpal, > Suraj <suraj.kandpal@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@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 > 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@intel.com> > > Signed-off-by: Matt Atwood <matthew.s.atwood@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
On Thu, Oct 24, 2024 at 02:52:14AM +0000, Kandpal, Suraj wrote: > > > > -----Original Message----- > > From: Roper, Matthew D <matthew.d.roper@intel.com> > > Sent: Wednesday, October 23, 2024 11:22 PM > > To: Atwood, Matthew S <matthew.s.atwood@intel.com> > > Cc: intel-xe@lists.freedesktop.org; intel-gfx@lists.freedesktop.org; Kandpal, > > Suraj <suraj.kandpal@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@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. 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@intel.com> > > > Signed-off-by: Matt Atwood <matthew.s.atwood@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
> -----Original Message----- > From: Roper, Matthew D <matthew.d.roper@intel.com> > Sent: Saturday, October 26, 2024 12:28 AM > To: Kandpal, Suraj <suraj.kandpal@intel.com> > Cc: Atwood, Matthew S <matthew.s.atwood@intel.com>; intel- > xe@lists.freedesktop.org; intel-gfx@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@intel.com> > > > Sent: Wednesday, October 23, 2024 11:22 PM > > > To: Atwood, Matthew S <matthew.s.atwood@intel.com> > > > Cc: intel-xe@lists.freedesktop.org; intel-gfx@lists.freedesktop.org; > > > Kandpal, Suraj <suraj.kandpal@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@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@intel.com> > > > > Signed-off-by: Matt Atwood <matthew.s.atwood@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
On Sun, Oct 27, 2024 at 09:11:07PM -0700, Kandpal, Suraj wrote: > > > > -----Original Message----- > > From: Roper, Matthew D <matthew.d.roper@intel.com> > > Sent: Saturday, October 26, 2024 12:28 AM > > To: Kandpal, Suraj <suraj.kandpal@intel.com> > > Cc: Atwood, Matthew S <matthew.s.atwood@intel.com>; intel- > > xe@lists.freedesktop.org; intel-gfx@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@intel.com> > > > > Sent: Wednesday, October 23, 2024 11:22 PM > > > > To: Atwood, Matthew S <matthew.s.atwood@intel.com> > > > > Cc: intel-xe@lists.freedesktop.org; intel-gfx@lists.freedesktop.org; > > > > Kandpal, Suraj <suraj.kandpal@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@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. Can HDCP only be enabled/disabled during a full modeset or can it be changed on a non-modeset pipe update? If it only changes on full modesets, then I agree that the initialization in intel_ddi_enable_transcoder_func() probably covers it (although I'm not sure why we've been using RMW cycles everywhere in that case rather than just setting the proper value during the initial register write). It sounds like nothing has truly changed logic-wise here in Xe3 compared to MTL or Xe2; the only real change is that the register bit has switched to a new location again. Matt > > 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@intel.com> > > > > > Signed-off-by: Matt Atwood <matthew.s.atwood@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
> -----Original Message----- > From: Roper, Matthew D <matthew.d.roper@intel.com> > Sent: Wednesday, October 30, 2024 5:33 AM > To: Kandpal, Suraj <suraj.kandpal@intel.com> > Cc: Nautiyal, Ankit K <ankit.k.nautiyal@intel.com>; Atwood, Matthew S > <matthew.s.atwood@intel.com>; intel-xe@lists.freedesktop.org; intel- > gfx@lists.freedesktop.org > Subject: Re: [PATCH v4 3/7] drm/i915/xe3lpd: Disable HDCP Line Rekeying for > Xe3 > > On Sun, Oct 27, 2024 at 09:11:07PM -0700, Kandpal, Suraj wrote: > > > > > > > -----Original Message----- > > > From: Roper, Matthew D <matthew.d.roper@intel.com> > > > Sent: Saturday, October 26, 2024 12:28 AM > > > To: Kandpal, Suraj <suraj.kandpal@intel.com> > > > Cc: Atwood, Matthew S <matthew.s.atwood@intel.com>; intel- > > > xe@lists.freedesktop.org; intel-gfx@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@intel.com> > > > > > Sent: Wednesday, October 23, 2024 11:22 PM > > > > > To: Atwood, Matthew S <matthew.s.atwood@intel.com> > > > > > Cc: intel-xe@lists.freedesktop.org; > > > > > intel-gfx@lists.freedesktop.org; Kandpal, Suraj > > > > > <suraj.kandpal@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@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. > I think I get your point ill float a separate patch that takes care of enabling HDCP line rekeying for HDCP 1.4 In the meantime can you review this patch which is just a modification due to the register bit change I have Sent a new patch in this link https://patchwork.freedesktop.org/series/140625/ > Can HDCP only be enabled/disabled during a full modeset or can it be > changed on a non-modeset pipe update? If it only changes on full modesets, > then I agree that the initialization in > intel_ddi_enable_transcoder_func() probably covers it (although I'm not sure > why we've been using RMW cycles everywhere in that case rather than just > setting the proper value during the initial register write). > > It sounds like nothing has truly changed logic-wise here in Xe3 compared to > MTL or Xe2; the only real change is that the register bit has switched to a new > location again. Yes that's true Regards, Suraj Kandpal > > > Matt > > > > > 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@intel.com> > > > > > > Signed-off-by: Matt Atwood <matthew.s.atwood@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 > > -- > 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 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)