Re: [Intel-gfx] [PATCH] drm/i915/dp: DP PHY compliance for JSL
On Fri, Jun 12, 2020 at 12:38:59PM -0700, Manasi Navare wrote:
> On Fri, Jun 12, 2020 at 10:21:31PM +0300, Ville Syrjälä wrote:
> > On Fri, Jun 12, 2020 at 12:12:25PM -0700, Manasi Navare wrote:
> > > On Fri, Jun 12, 2020 at 10:01:19PM +0300, Ville Syrjälä wrote:
> > > > On Fri, Jun 12, 2020 at 11:44:13AM -0700, Manasi Navare wrote:
> > > > > On Fri, Jun 12, 2020 at 09:36:37PM +0300, Ville Syrjälä wrote:
> > > > > > On Fri, Jun 12, 2020 at 11:25:42AM -0700, Manasi Navare wrote:
> > > > > > > On Fri, Jun 05, 2020 at 12:03:19AM +0300, Ville Syrjälä wrote:
> > > > > > > > On Thu, Jun 04, 2020 at 08:01:03PM +, Almahallawy, Khaled
> > > > > > > > wrote:
> > > > > > > > > On Thu, 2020-06-04 at 22:06 +0300, Ville Syrjälä wrote:
> > > > > > > > > > On Thu, Jun 04, 2020 at 10:33:48AM +0530, Vidya Srinivas
> > > > > > > > > > wrote:
> > > > > > > > > > > Signed-off-by: Khaled Almahallawy
> > > > > > > > > > >
> > > > > > > > > > > Signed-off-by: Vidya Srinivas
> > > > > > > > > > > ---
> > > > > > > > > > > drivers/gpu/drm/i915/display/intel_dp.c | 40
> > > > > > > > > > > ++---
> > > > > > > > > > > 1 file changed, 32 insertions(+), 8 deletions(-)
> > > > > > > > > > >
> > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> > > > > > > > > > > b/drivers/gpu/drm/i915/display/intel_dp.c
> > > > > > > > > > > index 7223367171d1..44663e8ac9a1 100644
> > > > > > > > > > > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > > > > > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > > > > > > > > > > @@ -5470,22 +5470,32 @@
> > > > > > > > > > > intel_dp_autotest_phy_ddi_disable(struct
> > > > > > > > > > > intel_dp *intel_dp)
> > > > > > > > > > > struct drm_i915_private *dev_priv = to_i915(dev);
> > > > > > > > > > > struct intel_crtc *crtc = to_intel_crtc(intel_dig_port-
> > > > > > > > > > > >base.base.crtc);
> > > > > > > > > > > enum pipe pipe = crtc->pipe;
> > > > > > > > > > > - u32 trans_ddi_func_ctl_value, trans_conf_value,
> > > > > > > > > > > dp_tp_ctl_value;
> > > > > > > > > > > + u32 trans_ddi_func_ctl_value, trans_conf_value,
> > > > > > > > > > > dp_tp_ctl_value, trans_ddi_port_mask;
> > > > > > > > > > > + enum port port = intel_dig_port->base.port;
> > > > > > > > > > > + i915_reg_t dp_tp_reg;
> > > > > > > > > > > +
> > > > > > > > > > > + if (IS_ELKHARTLAKE(dev_priv)) {
> > > > > > > > > > > + dp_tp_reg = DP_TP_CTL(port);
> > > > > > > > > > > + trans_ddi_port_mask = TRANS_DDI_PORT_MASK;
> > > > > > > > > > > + } else if (IS_TIGERLAKE(dev_priv)) {
> > > > > > > > > > > + dp_tp_reg = TGL_DP_TP_CTL(pipe);
> > > > > > > > > > > + trans_ddi_port_mask = TGL_TRANS_DDI_PORT_MASK;
> > > > > > > > > > > + }
> > > > > > > > > > >
> > > > > > > > > > > trans_ddi_func_ctl_value = intel_de_read(dev_priv,
> > > > > > > > > > >
> > > > > > > > > > > TRANS_DDI_FUNC_CTL(pip
> > > > > > > > > > > e));
> > > > > > > > > > > trans_conf_value = intel_de_read(dev_priv,
> > > > > > > > > > > PIPECONF(pipe));
> > > > > > > > > > > - dp_tp_ctl_value = intel_de_read(dev_priv,
> > > > > > > > > > > TGL_DP_TP_CTL(pipe));
> > > > > > > > > > >
> > > > > > > > > > > + dp_tp_ctl_value = intel_de_read(dev_priv, dp_tp_reg);
> > > > > > > > > > > trans_ddi_func_ctl_value &= ~(TRANS_DDI_FUNC_ENABLE |
> > > > > > > > > > > - TGL_TRANS_DDI_PORT_MASK);
> > > > > > > > > > > + trans_ddi_port_mask);
> > > > > > > > > > > trans_conf_value &= ~PIPECONF_ENABLE;
> > > > > > > > > > > dp_tp_ctl_value &= ~DP_TP_CTL_ENABLE;
> > > > > > > > > > >
> > > > > > > > > > > intel_de_write(dev_priv, PIPECONF(pipe),
> > > > > > > > > > > trans_conf_value);
> > > > > > > > > > > intel_de_write(dev_priv, TRANS_DDI_FUNC_CTL(pipe),
> > > > > > > > > > > trans_ddi_func_ctl_value);
> > > > > > > > > > > - intel_de_write(dev_priv, TGL_DP_TP_CTL(pipe),
> > > > > > > > > > > dp_tp_ctl_value);
> > > > > > > > > > > + intel_de_write(dev_priv, dp_tp_reg, dp_tp_ctl_value);
> > > > > > > > > >
> > > > > > > > > > All this ad-hoc modeset code really should not exist. It's
> > > > > > > > > > going to
> > > > > > > > > > have different bugs than the norma modeset paths, so
> > > > > > > > > > compliance
> > > > > > > > > > testing
> > > > > > > > > > this special code proves absolutely nothing about the
> > > > > > > > > > normal modeset
> > > > > > > > > > code. IMO someone needs to take up the task of rewrtiting
> > > > > > > > > > all this to
> > > > > > > > > > just perform normal modesets.
> > > > > > > > >
> > > > > > > > > Agree. I've just found that we get kernel NULL pointer
> > > > > > > > > dereference and
> > > > > > > > > panic when we try to access to_intel_crtc(intel_dig_port-
> > > > > > > > > >base.base.crtc).
> > > > > > > >
> > > > > > > > Yeah, that's a legacy pointer which should no longer be
Re: [Intel-gfx] [PATCH] drm/i915/dp: DP PHY compliance for JSL
On Fri, Jun 12, 2020 at 10:21:31PM +0300, Ville Syrjälä wrote:
> On Fri, Jun 12, 2020 at 12:12:25PM -0700, Manasi Navare wrote:
> > On Fri, Jun 12, 2020 at 10:01:19PM +0300, Ville Syrjälä wrote:
> > > On Fri, Jun 12, 2020 at 11:44:13AM -0700, Manasi Navare wrote:
> > > > On Fri, Jun 12, 2020 at 09:36:37PM +0300, Ville Syrjälä wrote:
> > > > > On Fri, Jun 12, 2020 at 11:25:42AM -0700, Manasi Navare wrote:
> > > > > > On Fri, Jun 05, 2020 at 12:03:19AM +0300, Ville Syrjälä wrote:
> > > > > > > On Thu, Jun 04, 2020 at 08:01:03PM +, Almahallawy, Khaled
> > > > > > > wrote:
> > > > > > > > On Thu, 2020-06-04 at 22:06 +0300, Ville Syrjälä wrote:
> > > > > > > > > On Thu, Jun 04, 2020 at 10:33:48AM +0530, Vidya Srinivas
> > > > > > > > > wrote:
> > > > > > > > > > Signed-off-by: Khaled Almahallawy
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: Vidya Srinivas
> > > > > > > > > > ---
> > > > > > > > > > drivers/gpu/drm/i915/display/intel_dp.c | 40
> > > > > > > > > > ++---
> > > > > > > > > > 1 file changed, 32 insertions(+), 8 deletions(-)
> > > > > > > > > >
> > > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> > > > > > > > > > b/drivers/gpu/drm/i915/display/intel_dp.c
> > > > > > > > > > index 7223367171d1..44663e8ac9a1 100644
> > > > > > > > > > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > > > > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > > > > > > > > > @@ -5470,22 +5470,32 @@
> > > > > > > > > > intel_dp_autotest_phy_ddi_disable(struct
> > > > > > > > > > intel_dp *intel_dp)
> > > > > > > > > > struct drm_i915_private *dev_priv = to_i915(dev);
> > > > > > > > > > struct intel_crtc *crtc = to_intel_crtc(intel_dig_port-
> > > > > > > > > > >base.base.crtc);
> > > > > > > > > > enum pipe pipe = crtc->pipe;
> > > > > > > > > > - u32 trans_ddi_func_ctl_value, trans_conf_value,
> > > > > > > > > > dp_tp_ctl_value;
> > > > > > > > > > + u32 trans_ddi_func_ctl_value, trans_conf_value,
> > > > > > > > > > dp_tp_ctl_value, trans_ddi_port_mask;
> > > > > > > > > > + enum port port = intel_dig_port->base.port;
> > > > > > > > > > + i915_reg_t dp_tp_reg;
> > > > > > > > > > +
> > > > > > > > > > + if (IS_ELKHARTLAKE(dev_priv)) {
> > > > > > > > > > + dp_tp_reg = DP_TP_CTL(port);
> > > > > > > > > > + trans_ddi_port_mask = TRANS_DDI_PORT_MASK;
> > > > > > > > > > + } else if (IS_TIGERLAKE(dev_priv)) {
> > > > > > > > > > + dp_tp_reg = TGL_DP_TP_CTL(pipe);
> > > > > > > > > > + trans_ddi_port_mask = TGL_TRANS_DDI_PORT_MASK;
> > > > > > > > > > + }
> > > > > > > > > >
> > > > > > > > > > trans_ddi_func_ctl_value = intel_de_read(dev_priv,
> > > > > > > > > >
> > > > > > > > > > TRANS_DDI_FUNC_CTL(pip
> > > > > > > > > > e));
> > > > > > > > > > trans_conf_value = intel_de_read(dev_priv,
> > > > > > > > > > PIPECONF(pipe));
> > > > > > > > > > - dp_tp_ctl_value = intel_de_read(dev_priv,
> > > > > > > > > > TGL_DP_TP_CTL(pipe));
> > > > > > > > > >
> > > > > > > > > > + dp_tp_ctl_value = intel_de_read(dev_priv, dp_tp_reg);
> > > > > > > > > > trans_ddi_func_ctl_value &= ~(TRANS_DDI_FUNC_ENABLE |
> > > > > > > > > > - TGL_TRANS_DDI_PORT_MASK);
> > > > > > > > > > + trans_ddi_port_mask);
> > > > > > > > > > trans_conf_value &= ~PIPECONF_ENABLE;
> > > > > > > > > > dp_tp_ctl_value &= ~DP_TP_CTL_ENABLE;
> > > > > > > > > >
> > > > > > > > > > intel_de_write(dev_priv, PIPECONF(pipe),
> > > > > > > > > > trans_conf_value);
> > > > > > > > > > intel_de_write(dev_priv, TRANS_DDI_FUNC_CTL(pipe),
> > > > > > > > > >trans_ddi_func_ctl_value);
> > > > > > > > > > - intel_de_write(dev_priv, TGL_DP_TP_CTL(pipe),
> > > > > > > > > > dp_tp_ctl_value);
> > > > > > > > > > + intel_de_write(dev_priv, dp_tp_reg, dp_tp_ctl_value);
> > > > > > > > >
> > > > > > > > > All this ad-hoc modeset code really should not exist. It's
> > > > > > > > > going to
> > > > > > > > > have different bugs than the norma modeset paths, so
> > > > > > > > > compliance
> > > > > > > > > testing
> > > > > > > > > this special code proves absolutely nothing about the normal
> > > > > > > > > modeset
> > > > > > > > > code. IMO someone needs to take up the task of rewrtiting all
> > > > > > > > > this to
> > > > > > > > > just perform normal modesets.
> > > > > > > >
> > > > > > > > Agree. I've just found that we get kernel NULL pointer
> > > > > > > > dereference and
> > > > > > > > panic when we try to access to_intel_crtc(intel_dig_port-
> > > > > > > > >base.base.crtc).
> > > > > > >
> > > > > > > Yeah, that's a legacy pointer which should no longer be used at
> > > > > > > all
> > > > > > > with atomic drivers. I'm slowly trying to clear out all this
> > > > > > > legacy
> > > > > > > cruft. The next step I had hoped to take
Re: [Intel-gfx] [PATCH] drm/i915/dp: DP PHY compliance for JSL
On Fri, Jun 12, 2020 at 12:12:25PM -0700, Manasi Navare wrote:
> On Fri, Jun 12, 2020 at 10:01:19PM +0300, Ville Syrjälä wrote:
> > On Fri, Jun 12, 2020 at 11:44:13AM -0700, Manasi Navare wrote:
> > > On Fri, Jun 12, 2020 at 09:36:37PM +0300, Ville Syrjälä wrote:
> > > > On Fri, Jun 12, 2020 at 11:25:42AM -0700, Manasi Navare wrote:
> > > > > On Fri, Jun 05, 2020 at 12:03:19AM +0300, Ville Syrjälä wrote:
> > > > > > On Thu, Jun 04, 2020 at 08:01:03PM +, Almahallawy, Khaled wrote:
> > > > > > > On Thu, 2020-06-04 at 22:06 +0300, Ville Syrjälä wrote:
> > > > > > > > On Thu, Jun 04, 2020 at 10:33:48AM +0530, Vidya Srinivas wrote:
> > > > > > > > > Signed-off-by: Khaled Almahallawy
> > > > > > > > >
> > > > > > > > > Signed-off-by: Vidya Srinivas
> > > > > > > > > ---
> > > > > > > > > drivers/gpu/drm/i915/display/intel_dp.c | 40
> > > > > > > > > ++---
> > > > > > > > > 1 file changed, 32 insertions(+), 8 deletions(-)
> > > > > > > > >
> > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> > > > > > > > > b/drivers/gpu/drm/i915/display/intel_dp.c
> > > > > > > > > index 7223367171d1..44663e8ac9a1 100644
> > > > > > > > > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > > > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > > > > > > > > @@ -5470,22 +5470,32 @@
> > > > > > > > > intel_dp_autotest_phy_ddi_disable(struct
> > > > > > > > > intel_dp *intel_dp)
> > > > > > > > > struct drm_i915_private *dev_priv = to_i915(dev);
> > > > > > > > > struct intel_crtc *crtc = to_intel_crtc(intel_dig_port-
> > > > > > > > > >base.base.crtc);
> > > > > > > > > enum pipe pipe = crtc->pipe;
> > > > > > > > > - u32 trans_ddi_func_ctl_value, trans_conf_value,
> > > > > > > > > dp_tp_ctl_value;
> > > > > > > > > + u32 trans_ddi_func_ctl_value, trans_conf_value,
> > > > > > > > > dp_tp_ctl_value, trans_ddi_port_mask;
> > > > > > > > > + enum port port = intel_dig_port->base.port;
> > > > > > > > > + i915_reg_t dp_tp_reg;
> > > > > > > > > +
> > > > > > > > > + if (IS_ELKHARTLAKE(dev_priv)) {
> > > > > > > > > + dp_tp_reg = DP_TP_CTL(port);
> > > > > > > > > + trans_ddi_port_mask = TRANS_DDI_PORT_MASK;
> > > > > > > > > + } else if (IS_TIGERLAKE(dev_priv)) {
> > > > > > > > > + dp_tp_reg = TGL_DP_TP_CTL(pipe);
> > > > > > > > > + trans_ddi_port_mask = TGL_TRANS_DDI_PORT_MASK;
> > > > > > > > > + }
> > > > > > > > >
> > > > > > > > > trans_ddi_func_ctl_value = intel_de_read(dev_priv,
> > > > > > > > >
> > > > > > > > > TRANS_DDI_FUNC_CTL(pip
> > > > > > > > > e));
> > > > > > > > > trans_conf_value = intel_de_read(dev_priv,
> > > > > > > > > PIPECONF(pipe));
> > > > > > > > > - dp_tp_ctl_value = intel_de_read(dev_priv,
> > > > > > > > > TGL_DP_TP_CTL(pipe));
> > > > > > > > >
> > > > > > > > > + dp_tp_ctl_value = intel_de_read(dev_priv, dp_tp_reg);
> > > > > > > > > trans_ddi_func_ctl_value &= ~(TRANS_DDI_FUNC_ENABLE |
> > > > > > > > > - TGL_TRANS_DDI_PORT_MASK);
> > > > > > > > > + trans_ddi_port_mask);
> > > > > > > > > trans_conf_value &= ~PIPECONF_ENABLE;
> > > > > > > > > dp_tp_ctl_value &= ~DP_TP_CTL_ENABLE;
> > > > > > > > >
> > > > > > > > > intel_de_write(dev_priv, PIPECONF(pipe),
> > > > > > > > > trans_conf_value);
> > > > > > > > > intel_de_write(dev_priv, TRANS_DDI_FUNC_CTL(pipe),
> > > > > > > > > trans_ddi_func_ctl_value);
> > > > > > > > > - intel_de_write(dev_priv, TGL_DP_TP_CTL(pipe),
> > > > > > > > > dp_tp_ctl_value);
> > > > > > > > > + intel_de_write(dev_priv, dp_tp_reg, dp_tp_ctl_value);
> > > > > > > >
> > > > > > > > All this ad-hoc modeset code really should not exist. It's
> > > > > > > > going to
> > > > > > > > have different bugs than the norma modeset paths, so compliance
> > > > > > > > testing
> > > > > > > > this special code proves absolutely nothing about the normal
> > > > > > > > modeset
> > > > > > > > code. IMO someone needs to take up the task of rewrtiting all
> > > > > > > > this to
> > > > > > > > just perform normal modesets.
> > > > > > >
> > > > > > > Agree. I've just found that we get kernel NULL pointer
> > > > > > > dereference and
> > > > > > > panic when we try to access to_intel_crtc(intel_dig_port-
> > > > > > > >base.base.crtc).
> > > > > >
> > > > > > Yeah, that's a legacy pointer which should no longer be used at all
> > > > > > with atomic drivers. I'm slowly trying to clear out all this legacy
> > > > > > cruft. The next step I had hoped to take was
> > > > > > https://patchwork.freedesktop.org/series/76993/ but then this
> > > > > > compliacnce stuff landed and threw another wrench into the works.
> > > > >
> > > > > We had several discussions on design of DP PHY compliance and the
> > > > > patches
Re: [Intel-gfx] [PATCH] drm/i915/dp: DP PHY compliance for JSL
On Fri, Jun 12, 2020 at 10:01:19PM +0300, Ville Syrjälä wrote:
> On Fri, Jun 12, 2020 at 11:44:13AM -0700, Manasi Navare wrote:
> > On Fri, Jun 12, 2020 at 09:36:37PM +0300, Ville Syrjälä wrote:
> > > On Fri, Jun 12, 2020 at 11:25:42AM -0700, Manasi Navare wrote:
> > > > On Fri, Jun 05, 2020 at 12:03:19AM +0300, Ville Syrjälä wrote:
> > > > > On Thu, Jun 04, 2020 at 08:01:03PM +, Almahallawy, Khaled wrote:
> > > > > > On Thu, 2020-06-04 at 22:06 +0300, Ville Syrjälä wrote:
> > > > > > > On Thu, Jun 04, 2020 at 10:33:48AM +0530, Vidya Srinivas wrote:
> > > > > > > > Signed-off-by: Khaled Almahallawy
> > > > > > > > Signed-off-by: Vidya Srinivas
> > > > > > > > ---
> > > > > > > > drivers/gpu/drm/i915/display/intel_dp.c | 40
> > > > > > > > ++---
> > > > > > > > 1 file changed, 32 insertions(+), 8 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> > > > > > > > b/drivers/gpu/drm/i915/display/intel_dp.c
> > > > > > > > index 7223367171d1..44663e8ac9a1 100644
> > > > > > > > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > > > > > > > @@ -5470,22 +5470,32 @@ intel_dp_autotest_phy_ddi_disable(struct
> > > > > > > > intel_dp *intel_dp)
> > > > > > > > struct drm_i915_private *dev_priv = to_i915(dev);
> > > > > > > > struct intel_crtc *crtc = to_intel_crtc(intel_dig_port-
> > > > > > > > >base.base.crtc);
> > > > > > > > enum pipe pipe = crtc->pipe;
> > > > > > > > - u32 trans_ddi_func_ctl_value, trans_conf_value,
> > > > > > > > dp_tp_ctl_value;
> > > > > > > > + u32 trans_ddi_func_ctl_value, trans_conf_value,
> > > > > > > > dp_tp_ctl_value, trans_ddi_port_mask;
> > > > > > > > + enum port port = intel_dig_port->base.port;
> > > > > > > > + i915_reg_t dp_tp_reg;
> > > > > > > > +
> > > > > > > > + if (IS_ELKHARTLAKE(dev_priv)) {
> > > > > > > > + dp_tp_reg = DP_TP_CTL(port);
> > > > > > > > + trans_ddi_port_mask = TRANS_DDI_PORT_MASK;
> > > > > > > > + } else if (IS_TIGERLAKE(dev_priv)) {
> > > > > > > > + dp_tp_reg = TGL_DP_TP_CTL(pipe);
> > > > > > > > + trans_ddi_port_mask = TGL_TRANS_DDI_PORT_MASK;
> > > > > > > > + }
> > > > > > > >
> > > > > > > > trans_ddi_func_ctl_value = intel_de_read(dev_priv,
> > > > > > > >
> > > > > > > > TRANS_DDI_FUNC_CTL(pip
> > > > > > > > e));
> > > > > > > > trans_conf_value = intel_de_read(dev_priv,
> > > > > > > > PIPECONF(pipe));
> > > > > > > > - dp_tp_ctl_value = intel_de_read(dev_priv,
> > > > > > > > TGL_DP_TP_CTL(pipe));
> > > > > > > >
> > > > > > > > + dp_tp_ctl_value = intel_de_read(dev_priv, dp_tp_reg);
> > > > > > > > trans_ddi_func_ctl_value &= ~(TRANS_DDI_FUNC_ENABLE |
> > > > > > > > - TGL_TRANS_DDI_PORT_MASK);
> > > > > > > > + trans_ddi_port_mask);
> > > > > > > > trans_conf_value &= ~PIPECONF_ENABLE;
> > > > > > > > dp_tp_ctl_value &= ~DP_TP_CTL_ENABLE;
> > > > > > > >
> > > > > > > > intel_de_write(dev_priv, PIPECONF(pipe),
> > > > > > > > trans_conf_value);
> > > > > > > > intel_de_write(dev_priv, TRANS_DDI_FUNC_CTL(pipe),
> > > > > > > >trans_ddi_func_ctl_value);
> > > > > > > > - intel_de_write(dev_priv, TGL_DP_TP_CTL(pipe),
> > > > > > > > dp_tp_ctl_value);
> > > > > > > > + intel_de_write(dev_priv, dp_tp_reg, dp_tp_ctl_value);
> > > > > > >
> > > > > > > All this ad-hoc modeset code really should not exist. It's going
> > > > > > > to
> > > > > > > have different bugs than the norma modeset paths, so compliance
> > > > > > > testing
> > > > > > > this special code proves absolutely nothing about the normal
> > > > > > > modeset
> > > > > > > code. IMO someone needs to take up the task of rewrtiting all
> > > > > > > this to
> > > > > > > just perform normal modesets.
> > > > > >
> > > > > > Agree. I've just found that we get kernel NULL pointer dereference
> > > > > > and
> > > > > > panic when we try to access to_intel_crtc(intel_dig_port-
> > > > > > >base.base.crtc).
> > > > >
> > > > > Yeah, that's a legacy pointer which should no longer be used at all
> > > > > with atomic drivers. I'm slowly trying to clear out all this legacy
> > > > > cruft. The next step I had hoped to take was
> > > > > https://patchwork.freedesktop.org/series/76993/ but then this
> > > > > compliacnce stuff landed and threw another wrench into the works.
> > > >
> > > > We had several discussions on design of DP PHY compliance and the
> > > > patches were on the M-L
> > > > for quite some time without anyone giving feedback on the actual design
> > > > of whether they should
> > > > happen through modeset or directly from the PHY comp request short
> > > > pulse.
Re: [Intel-gfx] [PATCH] drm/i915/dp: DP PHY compliance for JSL
On Fri, Jun 12, 2020 at 11:44:13AM -0700, Manasi Navare wrote:
> On Fri, Jun 12, 2020 at 09:36:37PM +0300, Ville Syrjälä wrote:
> > On Fri, Jun 12, 2020 at 11:25:42AM -0700, Manasi Navare wrote:
> > > On Fri, Jun 05, 2020 at 12:03:19AM +0300, Ville Syrjälä wrote:
> > > > On Thu, Jun 04, 2020 at 08:01:03PM +, Almahallawy, Khaled wrote:
> > > > > On Thu, 2020-06-04 at 22:06 +0300, Ville Syrjälä wrote:
> > > > > > On Thu, Jun 04, 2020 at 10:33:48AM +0530, Vidya Srinivas wrote:
> > > > > > > Signed-off-by: Khaled Almahallawy
> > > > > > > Signed-off-by: Vidya Srinivas
> > > > > > > ---
> > > > > > > drivers/gpu/drm/i915/display/intel_dp.c | 40
> > > > > > > ++---
> > > > > > > 1 file changed, 32 insertions(+), 8 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> > > > > > > b/drivers/gpu/drm/i915/display/intel_dp.c
> > > > > > > index 7223367171d1..44663e8ac9a1 100644
> > > > > > > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > > > > > > @@ -5470,22 +5470,32 @@ intel_dp_autotest_phy_ddi_disable(struct
> > > > > > > intel_dp *intel_dp)
> > > > > > > struct drm_i915_private *dev_priv = to_i915(dev);
> > > > > > > struct intel_crtc *crtc = to_intel_crtc(intel_dig_port-
> > > > > > > >base.base.crtc);
> > > > > > > enum pipe pipe = crtc->pipe;
> > > > > > > - u32 trans_ddi_func_ctl_value, trans_conf_value,
> > > > > > > dp_tp_ctl_value;
> > > > > > > + u32 trans_ddi_func_ctl_value, trans_conf_value,
> > > > > > > dp_tp_ctl_value, trans_ddi_port_mask;
> > > > > > > + enum port port = intel_dig_port->base.port;
> > > > > > > + i915_reg_t dp_tp_reg;
> > > > > > > +
> > > > > > > + if (IS_ELKHARTLAKE(dev_priv)) {
> > > > > > > + dp_tp_reg = DP_TP_CTL(port);
> > > > > > > + trans_ddi_port_mask = TRANS_DDI_PORT_MASK;
> > > > > > > + } else if (IS_TIGERLAKE(dev_priv)) {
> > > > > > > + dp_tp_reg = TGL_DP_TP_CTL(pipe);
> > > > > > > + trans_ddi_port_mask = TGL_TRANS_DDI_PORT_MASK;
> > > > > > > + }
> > > > > > >
> > > > > > > trans_ddi_func_ctl_value = intel_de_read(dev_priv,
> > > > > > >TRANS_DDI_FUNC_CTL(pip
> > > > > > > e));
> > > > > > > trans_conf_value = intel_de_read(dev_priv, PIPECONF(pipe));
> > > > > > > - dp_tp_ctl_value = intel_de_read(dev_priv, TGL_DP_TP_CTL(pipe));
> > > > > > >
> > > > > > > + dp_tp_ctl_value = intel_de_read(dev_priv, dp_tp_reg);
> > > > > > > trans_ddi_func_ctl_value &= ~(TRANS_DDI_FUNC_ENABLE |
> > > > > > > - TGL_TRANS_DDI_PORT_MASK);
> > > > > > > + trans_ddi_port_mask);
> > > > > > > trans_conf_value &= ~PIPECONF_ENABLE;
> > > > > > > dp_tp_ctl_value &= ~DP_TP_CTL_ENABLE;
> > > > > > >
> > > > > > > intel_de_write(dev_priv, PIPECONF(pipe), trans_conf_value);
> > > > > > > intel_de_write(dev_priv, TRANS_DDI_FUNC_CTL(pipe),
> > > > > > > trans_ddi_func_ctl_value);
> > > > > > > - intel_de_write(dev_priv, TGL_DP_TP_CTL(pipe), dp_tp_ctl_value);
> > > > > > > + intel_de_write(dev_priv, dp_tp_reg, dp_tp_ctl_value);
> > > > > >
> > > > > > All this ad-hoc modeset code really should not exist. It's going to
> > > > > > have different bugs than the norma modeset paths, so compliance
> > > > > > testing
> > > > > > this special code proves absolutely nothing about the normal modeset
> > > > > > code. IMO someone needs to take up the task of rewrtiting all this
> > > > > > to
> > > > > > just perform normal modesets.
> > > > >
> > > > > Agree. I've just found that we get kernel NULL pointer dereference and
> > > > > panic when we try to access to_intel_crtc(intel_dig_port-
> > > > > >base.base.crtc).
> > > >
> > > > Yeah, that's a legacy pointer which should no longer be used at all
> > > > with atomic drivers. I'm slowly trying to clear out all this legacy
> > > > cruft. The next step I had hoped to take was
> > > > https://patchwork.freedesktop.org/series/76993/ but then this
> > > > compliacnce stuff landed and threw another wrench into the works.
> > >
> > > We had several discussions on design of DP PHY compliance and the patches
> > > were on the M-L
> > > for quite some time without anyone giving feedback on the actual design
> > > of whether they should
> > > happen through modeset or directly from the PHY comp request short pulse.
> > > My first feedback was also that this should happen through a complete
> > > modeset where after we get
> > > PHY comp request we send a uevent like we do for link layer compliance
> > > and then trigger a full modeset.
> > > But honestly that was just a lot of overhead and
> > > The reason we decided to go with this ad hoc approach was that with PHY
> > > compliance request,
> > > nothing really changes in terms of link parameters so we do not need to
> > > go through
> > > a complete modeset request unlike link layer
Re: [Intel-gfx] [PATCH] drm/i915/dp: DP PHY compliance for JSL
On Fri, Jun 12, 2020 at 09:36:37PM +0300, Ville Syrjälä wrote:
> On Fri, Jun 12, 2020 at 11:25:42AM -0700, Manasi Navare wrote:
> > On Fri, Jun 05, 2020 at 12:03:19AM +0300, Ville Syrjälä wrote:
> > > On Thu, Jun 04, 2020 at 08:01:03PM +, Almahallawy, Khaled wrote:
> > > > On Thu, 2020-06-04 at 22:06 +0300, Ville Syrjälä wrote:
> > > > > On Thu, Jun 04, 2020 at 10:33:48AM +0530, Vidya Srinivas wrote:
> > > > > > Signed-off-by: Khaled Almahallawy
> > > > > > Signed-off-by: Vidya Srinivas
> > > > > > ---
> > > > > > drivers/gpu/drm/i915/display/intel_dp.c | 40
> > > > > > ++---
> > > > > > 1 file changed, 32 insertions(+), 8 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> > > > > > b/drivers/gpu/drm/i915/display/intel_dp.c
> > > > > > index 7223367171d1..44663e8ac9a1 100644
> > > > > > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > > > > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > > > > > @@ -5470,22 +5470,32 @@ intel_dp_autotest_phy_ddi_disable(struct
> > > > > > intel_dp *intel_dp)
> > > > > > struct drm_i915_private *dev_priv = to_i915(dev);
> > > > > > struct intel_crtc *crtc = to_intel_crtc(intel_dig_port-
> > > > > > >base.base.crtc);
> > > > > > enum pipe pipe = crtc->pipe;
> > > > > > - u32 trans_ddi_func_ctl_value, trans_conf_value,
> > > > > > dp_tp_ctl_value;
> > > > > > + u32 trans_ddi_func_ctl_value, trans_conf_value,
> > > > > > dp_tp_ctl_value, trans_ddi_port_mask;
> > > > > > + enum port port = intel_dig_port->base.port;
> > > > > > + i915_reg_t dp_tp_reg;
> > > > > > +
> > > > > > + if (IS_ELKHARTLAKE(dev_priv)) {
> > > > > > + dp_tp_reg = DP_TP_CTL(port);
> > > > > > + trans_ddi_port_mask = TRANS_DDI_PORT_MASK;
> > > > > > + } else if (IS_TIGERLAKE(dev_priv)) {
> > > > > > + dp_tp_reg = TGL_DP_TP_CTL(pipe);
> > > > > > + trans_ddi_port_mask = TGL_TRANS_DDI_PORT_MASK;
> > > > > > + }
> > > > > >
> > > > > > trans_ddi_func_ctl_value = intel_de_read(dev_priv,
> > > > > > TRANS_DDI_FUNC_CTL(pip
> > > > > > e));
> > > > > > trans_conf_value = intel_de_read(dev_priv, PIPECONF(pipe));
> > > > > > - dp_tp_ctl_value = intel_de_read(dev_priv, TGL_DP_TP_CTL(pipe));
> > > > > >
> > > > > > + dp_tp_ctl_value = intel_de_read(dev_priv, dp_tp_reg);
> > > > > > trans_ddi_func_ctl_value &= ~(TRANS_DDI_FUNC_ENABLE |
> > > > > > - TGL_TRANS_DDI_PORT_MASK);
> > > > > > + trans_ddi_port_mask);
> > > > > > trans_conf_value &= ~PIPECONF_ENABLE;
> > > > > > dp_tp_ctl_value &= ~DP_TP_CTL_ENABLE;
> > > > > >
> > > > > > intel_de_write(dev_priv, PIPECONF(pipe), trans_conf_value);
> > > > > > intel_de_write(dev_priv, TRANS_DDI_FUNC_CTL(pipe),
> > > > > >trans_ddi_func_ctl_value);
> > > > > > - intel_de_write(dev_priv, TGL_DP_TP_CTL(pipe), dp_tp_ctl_value);
> > > > > > + intel_de_write(dev_priv, dp_tp_reg, dp_tp_ctl_value);
> > > > >
> > > > > All this ad-hoc modeset code really should not exist. It's going to
> > > > > have different bugs than the norma modeset paths, so compliance
> > > > > testing
> > > > > this special code proves absolutely nothing about the normal modeset
> > > > > code. IMO someone needs to take up the task of rewrtiting all this to
> > > > > just perform normal modesets.
> > > >
> > > > Agree. I've just found that we get kernel NULL pointer dereference and
> > > > panic when we try to access to_intel_crtc(intel_dig_port-
> > > > >base.base.crtc).
> > >
> > > Yeah, that's a legacy pointer which should no longer be used at all
> > > with atomic drivers. I'm slowly trying to clear out all this legacy
> > > cruft. The next step I had hoped to take was
> > > https://patchwork.freedesktop.org/series/76993/ but then this
> > > compliacnce stuff landed and threw another wrench into the works.
> >
> > We had several discussions on design of DP PHY compliance and the patches
> > were on the M-L
> > for quite some time without anyone giving feedback on the actual design of
> > whether they should
> > happen through modeset or directly from the PHY comp request short pulse.
> > My first feedback was also that this should happen through a complete
> > modeset where after we get
> > PHY comp request we send a uevent like we do for link layer compliance and
> > then trigger a full modeset.
> > But honestly that was just a lot of overhead and
> > The reason we decided to go with this ad hoc approach was that with PHY
> > compliance request,
> > nothing really changes in terms of link parameters so we do not need to go
> > through
> > a complete modeset request unlike link layer compliance where we need to do
> > compute config
> > all over again to do the link params computation.
> >
> > Every PHY comp request first sends a link layer comp request that does a
> > full
Re: [Intel-gfx] [PATCH] drm/i915/dp: DP PHY compliance for JSL
On Fri, Jun 12, 2020 at 11:33:45AM -0700, Manasi Navare wrote:
> On Thu, Jun 04, 2020 at 08:01:03PM +, Almahallawy, Khaled wrote:
> > On Thu, 2020-06-04 at 22:06 +0300, Ville Syrjälä wrote:
> > > On Thu, Jun 04, 2020 at 10:33:48AM +0530, Vidya Srinivas wrote:
> > > > Signed-off-by: Khaled Almahallawy
> > > > Signed-off-by: Vidya Srinivas
> > > > ---
> > > > drivers/gpu/drm/i915/display/intel_dp.c | 40
> > > > ++---
> > > > 1 file changed, 32 insertions(+), 8 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> > > > b/drivers/gpu/drm/i915/display/intel_dp.c
> > > > index 7223367171d1..44663e8ac9a1 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > > > @@ -5470,22 +5470,32 @@ intel_dp_autotest_phy_ddi_disable(struct
> > > > intel_dp *intel_dp)
> > > > struct drm_i915_private *dev_priv = to_i915(dev);
> > > > struct intel_crtc *crtc = to_intel_crtc(intel_dig_port-
> > > > >base.base.crtc);
> > > > enum pipe pipe = crtc->pipe;
> > > > - u32 trans_ddi_func_ctl_value, trans_conf_value,
> > > > dp_tp_ctl_value;
> > > > + u32 trans_ddi_func_ctl_value, trans_conf_value,
> > > > dp_tp_ctl_value, trans_ddi_port_mask;
> > > > + enum port port = intel_dig_port->base.port;
> > > > + i915_reg_t dp_tp_reg;
> > > > +
> > > > + if (IS_ELKHARTLAKE(dev_priv)) {
> > > > + dp_tp_reg = DP_TP_CTL(port);
> > > > + trans_ddi_port_mask = TRANS_DDI_PORT_MASK;
> > > > + } else if (IS_TIGERLAKE(dev_priv)) {
> > > > + dp_tp_reg = TGL_DP_TP_CTL(pipe);
> > > > + trans_ddi_port_mask = TGL_TRANS_DDI_PORT_MASK;
> > > > + }
> > > >
> > > > trans_ddi_func_ctl_value = intel_de_read(dev_priv,
> > > > TRANS_DDI_FUNC_CTL(pip
> > > > e));
> > > > trans_conf_value = intel_de_read(dev_priv, PIPECONF(pipe));
> > > > - dp_tp_ctl_value = intel_de_read(dev_priv, TGL_DP_TP_CTL(pipe));
> > > >
> > > > + dp_tp_ctl_value = intel_de_read(dev_priv, dp_tp_reg);
> > > > trans_ddi_func_ctl_value &= ~(TRANS_DDI_FUNC_ENABLE |
> > > > - TGL_TRANS_DDI_PORT_MASK);
> > > > + trans_ddi_port_mask);
> > > > trans_conf_value &= ~PIPECONF_ENABLE;
> > > > dp_tp_ctl_value &= ~DP_TP_CTL_ENABLE;
> > > >
> > > > intel_de_write(dev_priv, PIPECONF(pipe), trans_conf_value);
> > > > intel_de_write(dev_priv, TRANS_DDI_FUNC_CTL(pipe),
> > > >trans_ddi_func_ctl_value);
> > > > - intel_de_write(dev_priv, TGL_DP_TP_CTL(pipe), dp_tp_ctl_value);
> > > > + intel_de_write(dev_priv, dp_tp_reg, dp_tp_ctl_value);
> > >
> > > All this ad-hoc modeset code really should not exist. It's going to
> > > have different bugs than the norma modeset paths, so compliance
> > > testing
> > > this special code proves absolutely nothing about the normal modeset
> > > code. IMO someone needs to take up the task of rewrtiting all this to
> > > just perform normal modesets.
>
> But isnt that behaviour of the scope against the compliance spec?
scope?
> The PHY request as per the VESA compliance spec should only come through
> a short pulse.
> Yes if it comes through a long pulse, it will reset the link and this whole
> code will fall apart.
I am not saying anything about how the sink signals the requests.
That's just an implementation detail that doesn't really matter.
>
> Manasi
>
> >
> > Agree. I've just found that we get kernel NULL pointer dereference and
> > panic when we try to access to_intel_crtc(intel_dig_port-
> > >base.base.crtc). This is because we didn't realize when we developed
> > the code that test scope has an option to send PHY test request on Long
> > HPD. Current desing assume PHY test request on short HPD. Because of
> > that we got the following error
> >
> >
> > [ 106.810882] i915 :00:02.0: [drm:intel_hpd_irq_handler [i915]]
> > digital hpd on [ENCODER:308:DDI F] - long
> > [ 106.810916] i915 :00:02.0: [drm:intel_hpd_irq_handler [i915]]
> > Received HPD interrupt on PIN 9 - cnt: 10
> > [ 106.811026] i915 :00:02.0: [drm:intel_dp_hpd_pulse [i915]] got
> > hpd irq on [ENCODER:308:DDI F] - long
> > [ 106.811095] i915 :00:02.0: [drm:i915_hotplug_work_func [i915]]
> > running encoder hotplug functions
> > [ 106.811184] i915 :00:02.0: [drm:i915_hotplug_work_func [i915]]
> > Connector DP-3 (pin 9) received hotplug event. (retry 0)
> > [ 106.811227] i915 :00:02.0: [drm:intel_dp_detect [i915]]
> > [CONNECTOR:309:DP-3]
> > [ 106.811292] i915 :00:02.0: [drm:intel_power_well_enable [i915]]
> > enabling TC cold off
> > [ 106.811365] i915 :00:02.0: [drm:tgl_tc_cold_request [i915]] TC
> > cold block succeeded
> > [ 106.811489] i915 :00:02.0:
Re: [Intel-gfx] [PATCH] drm/i915/dp: DP PHY compliance for JSL
On Fri, Jun 12, 2020 at 11:25:42AM -0700, Manasi Navare wrote:
> On Fri, Jun 05, 2020 at 12:03:19AM +0300, Ville Syrjälä wrote:
> > On Thu, Jun 04, 2020 at 08:01:03PM +, Almahallawy, Khaled wrote:
> > > On Thu, 2020-06-04 at 22:06 +0300, Ville Syrjälä wrote:
> > > > On Thu, Jun 04, 2020 at 10:33:48AM +0530, Vidya Srinivas wrote:
> > > > > Signed-off-by: Khaled Almahallawy
> > > > > Signed-off-by: Vidya Srinivas
> > > > > ---
> > > > > drivers/gpu/drm/i915/display/intel_dp.c | 40
> > > > > ++---
> > > > > 1 file changed, 32 insertions(+), 8 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> > > > > b/drivers/gpu/drm/i915/display/intel_dp.c
> > > > > index 7223367171d1..44663e8ac9a1 100644
> > > > > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > > > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > > > > @@ -5470,22 +5470,32 @@ intel_dp_autotest_phy_ddi_disable(struct
> > > > > intel_dp *intel_dp)
> > > > > struct drm_i915_private *dev_priv = to_i915(dev);
> > > > > struct intel_crtc *crtc = to_intel_crtc(intel_dig_port-
> > > > > >base.base.crtc);
> > > > > enum pipe pipe = crtc->pipe;
> > > > > - u32 trans_ddi_func_ctl_value, trans_conf_value,
> > > > > dp_tp_ctl_value;
> > > > > + u32 trans_ddi_func_ctl_value, trans_conf_value,
> > > > > dp_tp_ctl_value, trans_ddi_port_mask;
> > > > > + enum port port = intel_dig_port->base.port;
> > > > > + i915_reg_t dp_tp_reg;
> > > > > +
> > > > > + if (IS_ELKHARTLAKE(dev_priv)) {
> > > > > + dp_tp_reg = DP_TP_CTL(port);
> > > > > + trans_ddi_port_mask = TRANS_DDI_PORT_MASK;
> > > > > + } else if (IS_TIGERLAKE(dev_priv)) {
> > > > > + dp_tp_reg = TGL_DP_TP_CTL(pipe);
> > > > > + trans_ddi_port_mask = TGL_TRANS_DDI_PORT_MASK;
> > > > > + }
> > > > >
> > > > > trans_ddi_func_ctl_value = intel_de_read(dev_priv,
> > > > >TRANS_DDI_FUNC_CTL(pip
> > > > > e));
> > > > > trans_conf_value = intel_de_read(dev_priv, PIPECONF(pipe));
> > > > > - dp_tp_ctl_value = intel_de_read(dev_priv, TGL_DP_TP_CTL(pipe));
> > > > >
> > > > > + dp_tp_ctl_value = intel_de_read(dev_priv, dp_tp_reg);
> > > > > trans_ddi_func_ctl_value &= ~(TRANS_DDI_FUNC_ENABLE |
> > > > > - TGL_TRANS_DDI_PORT_MASK);
> > > > > + trans_ddi_port_mask);
> > > > > trans_conf_value &= ~PIPECONF_ENABLE;
> > > > > dp_tp_ctl_value &= ~DP_TP_CTL_ENABLE;
> > > > >
> > > > > intel_de_write(dev_priv, PIPECONF(pipe), trans_conf_value);
> > > > > intel_de_write(dev_priv, TRANS_DDI_FUNC_CTL(pipe),
> > > > > trans_ddi_func_ctl_value);
> > > > > - intel_de_write(dev_priv, TGL_DP_TP_CTL(pipe), dp_tp_ctl_value);
> > > > > + intel_de_write(dev_priv, dp_tp_reg, dp_tp_ctl_value);
> > > >
> > > > All this ad-hoc modeset code really should not exist. It's going to
> > > > have different bugs than the norma modeset paths, so compliance
> > > > testing
> > > > this special code proves absolutely nothing about the normal modeset
> > > > code. IMO someone needs to take up the task of rewrtiting all this to
> > > > just perform normal modesets.
> > >
> > > Agree. I've just found that we get kernel NULL pointer dereference and
> > > panic when we try to access to_intel_crtc(intel_dig_port-
> > > >base.base.crtc).
> >
> > Yeah, that's a legacy pointer which should no longer be used at all
> > with atomic drivers. I'm slowly trying to clear out all this legacy
> > cruft. The next step I had hoped to take was
> > https://patchwork.freedesktop.org/series/76993/ but then this
> > compliacnce stuff landed and threw another wrench into the works.
>
> We had several discussions on design of DP PHY compliance and the patches
> were on the M-L
> for quite some time without anyone giving feedback on the actual design of
> whether they should
> happen through modeset or directly from the PHY comp request short pulse.
> My first feedback was also that this should happen through a complete modeset
> where after we get
> PHY comp request we send a uevent like we do for link layer compliance and
> then trigger a full modeset.
> But honestly that was just a lot of overhead and
> The reason we decided to go with this ad hoc approach was that with PHY
> compliance request,
> nothing really changes in terms of link parameters so we do not need to go
> through
> a complete modeset request unlike link layer compliance where we need to do
> compute config
> all over again to do the link params computation.
>
> Every PHY comp request first sends a link layer comp request that does a full
> modeset
> and sets up the desired link rate/lane count.
> Then with PHY request, all we need to do is disable pipe conf, dp_tp_ctl, set
> the PHY patterns
> and renable the pipe conf and
Re: [Intel-gfx] [PATCH] drm/i915/dp: DP PHY compliance for JSL
On Thu, Jun 04, 2020 at 08:01:03PM +, Almahallawy, Khaled wrote:
> On Thu, 2020-06-04 at 22:06 +0300, Ville Syrjälä wrote:
> > On Thu, Jun 04, 2020 at 10:33:48AM +0530, Vidya Srinivas wrote:
> > > Signed-off-by: Khaled Almahallawy
> > > Signed-off-by: Vidya Srinivas
> > > ---
> > > drivers/gpu/drm/i915/display/intel_dp.c | 40
> > > ++---
> > > 1 file changed, 32 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> > > b/drivers/gpu/drm/i915/display/intel_dp.c
> > > index 7223367171d1..44663e8ac9a1 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > > @@ -5470,22 +5470,32 @@ intel_dp_autotest_phy_ddi_disable(struct
> > > intel_dp *intel_dp)
> > > struct drm_i915_private *dev_priv = to_i915(dev);
> > > struct intel_crtc *crtc = to_intel_crtc(intel_dig_port-
> > > >base.base.crtc);
> > > enum pipe pipe = crtc->pipe;
> > > - u32 trans_ddi_func_ctl_value, trans_conf_value,
> > > dp_tp_ctl_value;
> > > + u32 trans_ddi_func_ctl_value, trans_conf_value,
> > > dp_tp_ctl_value, trans_ddi_port_mask;
> > > + enum port port = intel_dig_port->base.port;
> > > + i915_reg_t dp_tp_reg;
> > > +
> > > + if (IS_ELKHARTLAKE(dev_priv)) {
> > > + dp_tp_reg = DP_TP_CTL(port);
> > > + trans_ddi_port_mask = TRANS_DDI_PORT_MASK;
> > > + } else if (IS_TIGERLAKE(dev_priv)) {
> > > + dp_tp_reg = TGL_DP_TP_CTL(pipe);
> > > + trans_ddi_port_mask = TGL_TRANS_DDI_PORT_MASK;
> > > + }
> > >
> > > trans_ddi_func_ctl_value = intel_de_read(dev_priv,
> > >TRANS_DDI_FUNC_CTL(pip
> > > e));
> > > trans_conf_value = intel_de_read(dev_priv, PIPECONF(pipe));
> > > - dp_tp_ctl_value = intel_de_read(dev_priv, TGL_DP_TP_CTL(pipe));
> > >
> > > + dp_tp_ctl_value = intel_de_read(dev_priv, dp_tp_reg);
> > > trans_ddi_func_ctl_value &= ~(TRANS_DDI_FUNC_ENABLE |
> > > - TGL_TRANS_DDI_PORT_MASK);
> > > + trans_ddi_port_mask);
> > > trans_conf_value &= ~PIPECONF_ENABLE;
> > > dp_tp_ctl_value &= ~DP_TP_CTL_ENABLE;
> > >
> > > intel_de_write(dev_priv, PIPECONF(pipe), trans_conf_value);
> > > intel_de_write(dev_priv, TRANS_DDI_FUNC_CTL(pipe),
> > > trans_ddi_func_ctl_value);
> > > - intel_de_write(dev_priv, TGL_DP_TP_CTL(pipe), dp_tp_ctl_value);
> > > + intel_de_write(dev_priv, dp_tp_reg, dp_tp_ctl_value);
> >
> > All this ad-hoc modeset code really should not exist. It's going to
> > have different bugs than the norma modeset paths, so compliance
> > testing
> > this special code proves absolutely nothing about the normal modeset
> > code. IMO someone needs to take up the task of rewrtiting all this to
> > just perform normal modesets.
But isnt that behaviour of the scope against the compliance spec?
The PHY request as per the VESA compliance spec should only come through
a short pulse.
Yes if it comes through a long pulse, it will reset the link and this whole
code will fall apart.
Manasi
>
> Agree. I've just found that we get kernel NULL pointer dereference and
> panic when we try to access to_intel_crtc(intel_dig_port-
> >base.base.crtc). This is because we didn't realize when we developed
> the code that test scope has an option to send PHY test request on Long
> HPD. Current desing assume PHY test request on short HPD. Because of
> that we got the following error
>
>
> [ 106.810882] i915 :00:02.0: [drm:intel_hpd_irq_handler [i915]]
> digital hpd on [ENCODER:308:DDI F] - long
> [ 106.810916] i915 :00:02.0: [drm:intel_hpd_irq_handler [i915]]
> Received HPD interrupt on PIN 9 - cnt: 10
> [ 106.811026] i915 :00:02.0: [drm:intel_dp_hpd_pulse [i915]] got
> hpd irq on [ENCODER:308:DDI F] - long
> [ 106.811095] i915 :00:02.0: [drm:i915_hotplug_work_func [i915]]
> running encoder hotplug functions
> [ 106.811184] i915 :00:02.0: [drm:i915_hotplug_work_func [i915]]
> Connector DP-3 (pin 9) received hotplug event. (retry 0)
> [ 106.811227] i915 :00:02.0: [drm:intel_dp_detect [i915]]
> [CONNECTOR:309:DP-3]
> [ 106.811292] i915 :00:02.0: [drm:intel_power_well_enable [i915]]
> enabling TC cold off
> [ 106.811365] i915 :00:02.0: [drm:tgl_tc_cold_request [i915]] TC
> cold block succeeded
> [ 106.811489] i915 :00:02.0: [drm:__intel_tc_port_lock [i915]]
> Port F/TC#3: TC port mode reset (tbt-alt -> dp-alt)
> [ 106.811663] i915 :00:02.0: [drm:intel_power_well_enable [i915]]
> enabling AUX F TC3
> [ 106.812449] [drm:drm_dp_dpcd_read] AUX F/port F: 0x0 AUX ->
> (ret= 15) 12 14 04 80 00 00 01 00 00 00 00 00 00 00 00
> [ 106.812484] i915 :00:02.0: [drm:intel_dp_read_dpcd [i915]] DPCD:
> 12 14 04 80 00 00 01 00 00 00 00 00 00 00 00
> [ 106.813266] [drm:drm_dp_dpcd_read] AUX F/port F: 0x00400 AUX ->
> (ret= 12) 00 00 00 00 00 00 00 00 00 00 00 00
> [ 106.813271] [drm:drm_dp_read_desc] DP sink:
Re: [Intel-gfx] [PATCH] drm/i915/dp: DP PHY compliance for JSL
On Fri, Jun 05, 2020 at 12:03:19AM +0300, Ville Syrjälä wrote:
> On Thu, Jun 04, 2020 at 08:01:03PM +, Almahallawy, Khaled wrote:
> > On Thu, 2020-06-04 at 22:06 +0300, Ville Syrjälä wrote:
> > > On Thu, Jun 04, 2020 at 10:33:48AM +0530, Vidya Srinivas wrote:
> > > > Signed-off-by: Khaled Almahallawy
> > > > Signed-off-by: Vidya Srinivas
> > > > ---
> > > > drivers/gpu/drm/i915/display/intel_dp.c | 40
> > > > ++---
> > > > 1 file changed, 32 insertions(+), 8 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> > > > b/drivers/gpu/drm/i915/display/intel_dp.c
> > > > index 7223367171d1..44663e8ac9a1 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > > > @@ -5470,22 +5470,32 @@ intel_dp_autotest_phy_ddi_disable(struct
> > > > intel_dp *intel_dp)
> > > > struct drm_i915_private *dev_priv = to_i915(dev);
> > > > struct intel_crtc *crtc = to_intel_crtc(intel_dig_port-
> > > > >base.base.crtc);
> > > > enum pipe pipe = crtc->pipe;
> > > > - u32 trans_ddi_func_ctl_value, trans_conf_value,
> > > > dp_tp_ctl_value;
> > > > + u32 trans_ddi_func_ctl_value, trans_conf_value,
> > > > dp_tp_ctl_value, trans_ddi_port_mask;
> > > > + enum port port = intel_dig_port->base.port;
> > > > + i915_reg_t dp_tp_reg;
> > > > +
> > > > + if (IS_ELKHARTLAKE(dev_priv)) {
> > > > + dp_tp_reg = DP_TP_CTL(port);
> > > > + trans_ddi_port_mask = TRANS_DDI_PORT_MASK;
> > > > + } else if (IS_TIGERLAKE(dev_priv)) {
> > > > + dp_tp_reg = TGL_DP_TP_CTL(pipe);
> > > > + trans_ddi_port_mask = TGL_TRANS_DDI_PORT_MASK;
> > > > + }
> > > >
> > > > trans_ddi_func_ctl_value = intel_de_read(dev_priv,
> > > > TRANS_DDI_FUNC_CTL(pip
> > > > e));
> > > > trans_conf_value = intel_de_read(dev_priv, PIPECONF(pipe));
> > > > - dp_tp_ctl_value = intel_de_read(dev_priv, TGL_DP_TP_CTL(pipe));
> > > >
> > > > + dp_tp_ctl_value = intel_de_read(dev_priv, dp_tp_reg);
> > > > trans_ddi_func_ctl_value &= ~(TRANS_DDI_FUNC_ENABLE |
> > > > - TGL_TRANS_DDI_PORT_MASK);
> > > > + trans_ddi_port_mask);
> > > > trans_conf_value &= ~PIPECONF_ENABLE;
> > > > dp_tp_ctl_value &= ~DP_TP_CTL_ENABLE;
> > > >
> > > > intel_de_write(dev_priv, PIPECONF(pipe), trans_conf_value);
> > > > intel_de_write(dev_priv, TRANS_DDI_FUNC_CTL(pipe),
> > > >trans_ddi_func_ctl_value);
> > > > - intel_de_write(dev_priv, TGL_DP_TP_CTL(pipe), dp_tp_ctl_value);
> > > > + intel_de_write(dev_priv, dp_tp_reg, dp_tp_ctl_value);
> > >
> > > All this ad-hoc modeset code really should not exist. It's going to
> > > have different bugs than the norma modeset paths, so compliance
> > > testing
> > > this special code proves absolutely nothing about the normal modeset
> > > code. IMO someone needs to take up the task of rewrtiting all this to
> > > just perform normal modesets.
> >
> > Agree. I've just found that we get kernel NULL pointer dereference and
> > panic when we try to access to_intel_crtc(intel_dig_port-
> > >base.base.crtc).
>
> Yeah, that's a legacy pointer which should no longer be used at all
> with atomic drivers. I'm slowly trying to clear out all this legacy
> cruft. The next step I had hoped to take was
> https://patchwork.freedesktop.org/series/76993/ but then this
> compliacnce stuff landed and threw another wrench into the works.
We had several discussions on design of DP PHY compliance and the patches were
on the M-L
for quite some time without anyone giving feedback on the actual design of
whether they should
happen through modeset or directly from the PHY comp request short pulse.
My first feedback was also that this should happen through a complete modeset
where after we get
PHY comp request we send a uevent like we do for link layer compliance and then
trigger a full modeset.
But honestly that was just a lot of overhead and
The reason we decided to go with this ad hoc approach was that with PHY
compliance request,
nothing really changes in terms of link parameters so we do not need to go
through
a complete modeset request unlike link layer compliance where we need to do
compute config
all over again to do the link params computation.
Every PHY comp request first sends a link layer comp request that does a full
modeset
and sets up the desired link rate/lane count.
Then with PHY request, all we need to do is disable pipe conf, dp_tp_ctl, set
the PHY patterns
and renable the pipe conf and dp_tp_ctl without interfering and doing anything
with a full modeset.
Now i think if we need to scale this to other platforms, can we add a per
platform hook
for handle_phy_request that gets the correct
Re: [Intel-gfx] [PATCH] drm/i915/dp: DP PHY compliance for JSL
On Thu, Jun 04, 2020 at 08:01:03PM +, Almahallawy, Khaled wrote:
> On Thu, 2020-06-04 at 22:06 +0300, Ville Syrjälä wrote:
> > On Thu, Jun 04, 2020 at 10:33:48AM +0530, Vidya Srinivas wrote:
> > > Signed-off-by: Khaled Almahallawy
> > > Signed-off-by: Vidya Srinivas
> > > ---
> > > drivers/gpu/drm/i915/display/intel_dp.c | 40
> > > ++---
> > > 1 file changed, 32 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> > > b/drivers/gpu/drm/i915/display/intel_dp.c
> > > index 7223367171d1..44663e8ac9a1 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > > @@ -5470,22 +5470,32 @@ intel_dp_autotest_phy_ddi_disable(struct
> > > intel_dp *intel_dp)
> > > struct drm_i915_private *dev_priv = to_i915(dev);
> > > struct intel_crtc *crtc = to_intel_crtc(intel_dig_port-
> > > >base.base.crtc);
> > > enum pipe pipe = crtc->pipe;
> > > - u32 trans_ddi_func_ctl_value, trans_conf_value,
> > > dp_tp_ctl_value;
> > > + u32 trans_ddi_func_ctl_value, trans_conf_value,
> > > dp_tp_ctl_value, trans_ddi_port_mask;
> > > + enum port port = intel_dig_port->base.port;
> > > + i915_reg_t dp_tp_reg;
> > > +
> > > + if (IS_ELKHARTLAKE(dev_priv)) {
> > > + dp_tp_reg = DP_TP_CTL(port);
> > > + trans_ddi_port_mask = TRANS_DDI_PORT_MASK;
> > > + } else if (IS_TIGERLAKE(dev_priv)) {
> > > + dp_tp_reg = TGL_DP_TP_CTL(pipe);
> > > + trans_ddi_port_mask = TGL_TRANS_DDI_PORT_MASK;
> > > + }
> > >
> > > trans_ddi_func_ctl_value = intel_de_read(dev_priv,
> > >TRANS_DDI_FUNC_CTL(pip
> > > e));
> > > trans_conf_value = intel_de_read(dev_priv, PIPECONF(pipe));
> > > - dp_tp_ctl_value = intel_de_read(dev_priv, TGL_DP_TP_CTL(pipe));
> > >
> > > + dp_tp_ctl_value = intel_de_read(dev_priv, dp_tp_reg);
> > > trans_ddi_func_ctl_value &= ~(TRANS_DDI_FUNC_ENABLE |
> > > - TGL_TRANS_DDI_PORT_MASK);
> > > + trans_ddi_port_mask);
> > > trans_conf_value &= ~PIPECONF_ENABLE;
> > > dp_tp_ctl_value &= ~DP_TP_CTL_ENABLE;
> > >
> > > intel_de_write(dev_priv, PIPECONF(pipe), trans_conf_value);
> > > intel_de_write(dev_priv, TRANS_DDI_FUNC_CTL(pipe),
> > > trans_ddi_func_ctl_value);
> > > - intel_de_write(dev_priv, TGL_DP_TP_CTL(pipe), dp_tp_ctl_value);
> > > + intel_de_write(dev_priv, dp_tp_reg, dp_tp_ctl_value);
> >
> > All this ad-hoc modeset code really should not exist. It's going to
> > have different bugs than the norma modeset paths, so compliance
> > testing
> > this special code proves absolutely nothing about the normal modeset
> > code. IMO someone needs to take up the task of rewrtiting all this to
> > just perform normal modesets.
>
> Agree. I've just found that we get kernel NULL pointer dereference and
> panic when we try to access to_intel_crtc(intel_dig_port-
> >base.base.crtc).
Yeah, that's a legacy pointer which should no longer be used at all
with atomic drivers. I'm slowly trying to clear out all this legacy
cruft. The next step I had hoped to take was
https://patchwork.freedesktop.org/series/76993/ but then this
compliacnce stuff landed and threw another wrench into the works.
--
Ville Syrjälä
Intel
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Intel-gfx] [PATCH] drm/i915/dp: DP PHY compliance for JSL
On Thu, 2020-06-04 at 22:06 +0300, Ville Syrjälä wrote:
> On Thu, Jun 04, 2020 at 10:33:48AM +0530, Vidya Srinivas wrote:
> > Signed-off-by: Khaled Almahallawy
> > Signed-off-by: Vidya Srinivas
> > ---
> > drivers/gpu/drm/i915/display/intel_dp.c | 40
> > ++---
> > 1 file changed, 32 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> > b/drivers/gpu/drm/i915/display/intel_dp.c
> > index 7223367171d1..44663e8ac9a1 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > @@ -5470,22 +5470,32 @@ intel_dp_autotest_phy_ddi_disable(struct
> > intel_dp *intel_dp)
> > struct drm_i915_private *dev_priv = to_i915(dev);
> > struct intel_crtc *crtc = to_intel_crtc(intel_dig_port-
> > >base.base.crtc);
> > enum pipe pipe = crtc->pipe;
> > - u32 trans_ddi_func_ctl_value, trans_conf_value,
> > dp_tp_ctl_value;
> > + u32 trans_ddi_func_ctl_value, trans_conf_value,
> > dp_tp_ctl_value, trans_ddi_port_mask;
> > + enum port port = intel_dig_port->base.port;
> > + i915_reg_t dp_tp_reg;
> > +
> > + if (IS_ELKHARTLAKE(dev_priv)) {
> > + dp_tp_reg = DP_TP_CTL(port);
> > + trans_ddi_port_mask = TRANS_DDI_PORT_MASK;
> > + } else if (IS_TIGERLAKE(dev_priv)) {
> > + dp_tp_reg = TGL_DP_TP_CTL(pipe);
> > + trans_ddi_port_mask = TGL_TRANS_DDI_PORT_MASK;
> > + }
> >
> > trans_ddi_func_ctl_value = intel_de_read(dev_priv,
> > TRANS_DDI_FUNC_CTL(pip
> > e));
> > trans_conf_value = intel_de_read(dev_priv, PIPECONF(pipe));
> > - dp_tp_ctl_value = intel_de_read(dev_priv, TGL_DP_TP_CTL(pipe));
> >
> > + dp_tp_ctl_value = intel_de_read(dev_priv, dp_tp_reg);
> > trans_ddi_func_ctl_value &= ~(TRANS_DDI_FUNC_ENABLE |
> > - TGL_TRANS_DDI_PORT_MASK);
> > + trans_ddi_port_mask);
> > trans_conf_value &= ~PIPECONF_ENABLE;
> > dp_tp_ctl_value &= ~DP_TP_CTL_ENABLE;
> >
> > intel_de_write(dev_priv, PIPECONF(pipe), trans_conf_value);
> > intel_de_write(dev_priv, TRANS_DDI_FUNC_CTL(pipe),
> >trans_ddi_func_ctl_value);
> > - intel_de_write(dev_priv, TGL_DP_TP_CTL(pipe), dp_tp_ctl_value);
> > + intel_de_write(dev_priv, dp_tp_reg, dp_tp_ctl_value);
>
> All this ad-hoc modeset code really should not exist. It's going to
> have different bugs than the norma modeset paths, so compliance
> testing
> this special code proves absolutely nothing about the normal modeset
> code. IMO someone needs to take up the task of rewrtiting all this to
> just perform normal modesets.
Agree. I've just found that we get kernel NULL pointer dereference and
panic when we try to access to_intel_crtc(intel_dig_port-
>base.base.crtc). This is because we didn't realize when we developed
the code that test scope has an option to send PHY test request on Long
HPD. Current desing assume PHY test request on short HPD. Because of
that we got the following error
[ 106.810882] i915 :00:02.0: [drm:intel_hpd_irq_handler [i915]]
digital hpd on [ENCODER:308:DDI F] - long
[ 106.810916] i915 :00:02.0: [drm:intel_hpd_irq_handler [i915]]
Received HPD interrupt on PIN 9 - cnt: 10
[ 106.811026] i915 :00:02.0: [drm:intel_dp_hpd_pulse [i915]] got
hpd irq on [ENCODER:308:DDI F] - long
[ 106.811095] i915 :00:02.0: [drm:i915_hotplug_work_func [i915]]
running encoder hotplug functions
[ 106.811184] i915 :00:02.0: [drm:i915_hotplug_work_func [i915]]
Connector DP-3 (pin 9) received hotplug event. (retry 0)
[ 106.811227] i915 :00:02.0: [drm:intel_dp_detect [i915]]
[CONNECTOR:309:DP-3]
[ 106.811292] i915 :00:02.0: [drm:intel_power_well_enable [i915]]
enabling TC cold off
[ 106.811365] i915 :00:02.0: [drm:tgl_tc_cold_request [i915]] TC
cold block succeeded
[ 106.811489] i915 :00:02.0: [drm:__intel_tc_port_lock [i915]]
Port F/TC#3: TC port mode reset (tbt-alt -> dp-alt)
[ 106.811663] i915 :00:02.0: [drm:intel_power_well_enable [i915]]
enabling AUX F TC3
[ 106.812449] [drm:drm_dp_dpcd_read] AUX F/port F: 0x0 AUX ->
(ret= 15) 12 14 04 80 00 00 01 00 00 00 00 00 00 00 00
[ 106.812484] i915 :00:02.0: [drm:intel_dp_read_dpcd [i915]] DPCD:
12 14 04 80 00 00 01 00 00 00 00 00 00 00 00
[ 106.813266] [drm:drm_dp_dpcd_read] AUX F/port F: 0x00400 AUX ->
(ret= 12) 00 00 00 00 00 00 00 00 00 00 00 00
[ 106.813271] [drm:drm_dp_read_desc] DP sink: OUI 00-00-00 dev-ID HW-
rev 0.0 SW-rev 0.0 quirks 0x
[ 106.813891] [drm:drm_dp_dpcd_read] AUX F/port F: 0x00200 AUX ->
(ret= 1) 01
[ 106.813940] i915 :00:02.0: [drm:intel_dp_print_rates [i915]]
source rates: 162000, 216000, 27, 324000, 432000, 54, 648000,
81
[ 106.813974] i915 :00:02.0: [drm:intel_dp_print_rates [i915]]
sink rates: 162000, 27, 54
[ 106.814007] i915 :00:02.0: [drm:intel_dp_print_rates [i915]]
common
Re: [Intel-gfx] [PATCH] drm/i915/dp: DP PHY compliance for JSL
On Thu, Jun 04, 2020 at 10:33:48AM +0530, Vidya Srinivas wrote:
> Signed-off-by: Khaled Almahallawy
> Signed-off-by: Vidya Srinivas
> ---
> drivers/gpu/drm/i915/display/intel_dp.c | 40
> ++---
> 1 file changed, 32 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> b/drivers/gpu/drm/i915/display/intel_dp.c
> index 7223367171d1..44663e8ac9a1 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -5470,22 +5470,32 @@ intel_dp_autotest_phy_ddi_disable(struct intel_dp
> *intel_dp)
> struct drm_i915_private *dev_priv = to_i915(dev);
> struct intel_crtc *crtc = to_intel_crtc(intel_dig_port->base.base.crtc);
> enum pipe pipe = crtc->pipe;
> - u32 trans_ddi_func_ctl_value, trans_conf_value, dp_tp_ctl_value;
> + u32 trans_ddi_func_ctl_value, trans_conf_value, dp_tp_ctl_value,
> trans_ddi_port_mask;
> + enum port port = intel_dig_port->base.port;
> + i915_reg_t dp_tp_reg;
> +
> + if (IS_ELKHARTLAKE(dev_priv)) {
> + dp_tp_reg = DP_TP_CTL(port);
> + trans_ddi_port_mask = TRANS_DDI_PORT_MASK;
> + } else if (IS_TIGERLAKE(dev_priv)) {
> + dp_tp_reg = TGL_DP_TP_CTL(pipe);
> + trans_ddi_port_mask = TGL_TRANS_DDI_PORT_MASK;
> + }
>
> trans_ddi_func_ctl_value = intel_de_read(dev_priv,
>TRANS_DDI_FUNC_CTL(pipe));
> trans_conf_value = intel_de_read(dev_priv, PIPECONF(pipe));
> - dp_tp_ctl_value = intel_de_read(dev_priv, TGL_DP_TP_CTL(pipe));
>
> + dp_tp_ctl_value = intel_de_read(dev_priv, dp_tp_reg);
> trans_ddi_func_ctl_value &= ~(TRANS_DDI_FUNC_ENABLE |
> - TGL_TRANS_DDI_PORT_MASK);
> + trans_ddi_port_mask);
> trans_conf_value &= ~PIPECONF_ENABLE;
> dp_tp_ctl_value &= ~DP_TP_CTL_ENABLE;
>
> intel_de_write(dev_priv, PIPECONF(pipe), trans_conf_value);
> intel_de_write(dev_priv, TRANS_DDI_FUNC_CTL(pipe),
> trans_ddi_func_ctl_value);
> - intel_de_write(dev_priv, TGL_DP_TP_CTL(pipe), dp_tp_ctl_value);
> + intel_de_write(dev_priv, dp_tp_reg, dp_tp_ctl_value);
All this ad-hoc modeset code really should not exist. It's going to
have different bugs than the norma modeset paths, so compliance testing
this special code proves absolutely nothing about the normal modeset
code. IMO someone needs to take up the task of rewrtiting all this to
just perform normal modesets.
> }
>
> static void
> @@ -5497,20 +5507,28 @@ intel_dp_autotest_phy_ddi_enable(struct intel_dp
> *intel_dp, uint8_t lane_cnt)
> enum port port = intel_dig_port->base.port;
> struct intel_crtc *crtc = to_intel_crtc(intel_dig_port->base.base.crtc);
> enum pipe pipe = crtc->pipe;
> - u32 trans_ddi_func_ctl_value, trans_conf_value, dp_tp_ctl_value;
> + u32 trans_ddi_func_ctl_value, trans_conf_value, dp_tp_ctl_value,
> trans_ddi_sel_port;
> + i915_reg_t dp_tp_reg;
> +
> + if (IS_ELKHARTLAKE(dev_priv)) {
> + dp_tp_reg = DP_TP_CTL(port);
> + trans_ddi_sel_port = TRANS_DDI_SELECT_PORT(port);
> + } else if (IS_TIGERLAKE(dev_priv)) {
> + dp_tp_reg = TGL_DP_TP_CTL(pipe);
> + trans_ddi_sel_port = TGL_TRANS_DDI_SELECT_PORT(port);
> + }
>
> trans_ddi_func_ctl_value = intel_de_read(dev_priv,
>TRANS_DDI_FUNC_CTL(pipe));
> trans_conf_value = intel_de_read(dev_priv, PIPECONF(pipe));
> dp_tp_ctl_value = intel_de_read(dev_priv, TGL_DP_TP_CTL(pipe));
> -
> trans_ddi_func_ctl_value |= TRANS_DDI_FUNC_ENABLE |
> - TGL_TRANS_DDI_SELECT_PORT(port);
> + trans_ddi_sel_port;
> trans_conf_value |= PIPECONF_ENABLE;
> dp_tp_ctl_value |= DP_TP_CTL_ENABLE;
>
> intel_de_write(dev_priv, PIPECONF(pipe), trans_conf_value);
> - intel_de_write(dev_priv, TGL_DP_TP_CTL(pipe), dp_tp_ctl_value);
> + intel_de_write(dev_priv, dp_tp_reg, dp_tp_ctl_value);
> intel_de_write(dev_priv, TRANS_DDI_FUNC_CTL(pipe),
> trans_ddi_func_ctl_value);
> }
> @@ -5557,6 +5575,7 @@ static u8 intel_dp_autotest_phy_pattern(struct intel_dp
> *intel_dp)
> static void intel_dp_handle_test_request(struct intel_dp *intel_dp)
> {
> struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> + struct drm_i915_private *dev_priv = i915;
> u8 response = DP_TEST_NAK;
> u8 request = 0;
> int status;
> @@ -5582,6 +5601,11 @@ static void intel_dp_handle_test_request(struct
> intel_dp *intel_dp)
> response = intel_dp_autotest_edid(intel_dp);
> break;
> case DP_TEST_LINK_PHY_TEST_PATTERN:
> + if (!IS_ELKHARTLAKE(dev_priv) || !IS_TIGERLAKE(dev_priv)) {
> +
[PATCH] drm/i915/dp: DP PHY compliance for JSL
Signed-off-by: Khaled Almahallawy
Signed-off-by: Vidya Srinivas
---
drivers/gpu/drm/i915/display/intel_dp.c | 40 ++---
1 file changed, 32 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
b/drivers/gpu/drm/i915/display/intel_dp.c
index 7223367171d1..44663e8ac9a1 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -5470,22 +5470,32 @@ intel_dp_autotest_phy_ddi_disable(struct intel_dp
*intel_dp)
struct drm_i915_private *dev_priv = to_i915(dev);
struct intel_crtc *crtc = to_intel_crtc(intel_dig_port->base.base.crtc);
enum pipe pipe = crtc->pipe;
- u32 trans_ddi_func_ctl_value, trans_conf_value, dp_tp_ctl_value;
+ u32 trans_ddi_func_ctl_value, trans_conf_value, dp_tp_ctl_value,
trans_ddi_port_mask;
+ enum port port = intel_dig_port->base.port;
+ i915_reg_t dp_tp_reg;
+
+ if (IS_ELKHARTLAKE(dev_priv)) {
+ dp_tp_reg = DP_TP_CTL(port);
+ trans_ddi_port_mask = TRANS_DDI_PORT_MASK;
+ } else if (IS_TIGERLAKE(dev_priv)) {
+ dp_tp_reg = TGL_DP_TP_CTL(pipe);
+ trans_ddi_port_mask = TGL_TRANS_DDI_PORT_MASK;
+ }
trans_ddi_func_ctl_value = intel_de_read(dev_priv,
TRANS_DDI_FUNC_CTL(pipe));
trans_conf_value = intel_de_read(dev_priv, PIPECONF(pipe));
- dp_tp_ctl_value = intel_de_read(dev_priv, TGL_DP_TP_CTL(pipe));
+ dp_tp_ctl_value = intel_de_read(dev_priv, dp_tp_reg);
trans_ddi_func_ctl_value &= ~(TRANS_DDI_FUNC_ENABLE |
- TGL_TRANS_DDI_PORT_MASK);
+ trans_ddi_port_mask);
trans_conf_value &= ~PIPECONF_ENABLE;
dp_tp_ctl_value &= ~DP_TP_CTL_ENABLE;
intel_de_write(dev_priv, PIPECONF(pipe), trans_conf_value);
intel_de_write(dev_priv, TRANS_DDI_FUNC_CTL(pipe),
trans_ddi_func_ctl_value);
- intel_de_write(dev_priv, TGL_DP_TP_CTL(pipe), dp_tp_ctl_value);
+ intel_de_write(dev_priv, dp_tp_reg, dp_tp_ctl_value);
}
static void
@@ -5497,20 +5507,28 @@ intel_dp_autotest_phy_ddi_enable(struct intel_dp
*intel_dp, uint8_t lane_cnt)
enum port port = intel_dig_port->base.port;
struct intel_crtc *crtc = to_intel_crtc(intel_dig_port->base.base.crtc);
enum pipe pipe = crtc->pipe;
- u32 trans_ddi_func_ctl_value, trans_conf_value, dp_tp_ctl_value;
+ u32 trans_ddi_func_ctl_value, trans_conf_value, dp_tp_ctl_value,
trans_ddi_sel_port;
+ i915_reg_t dp_tp_reg;
+
+ if (IS_ELKHARTLAKE(dev_priv)) {
+ dp_tp_reg = DP_TP_CTL(port);
+ trans_ddi_sel_port = TRANS_DDI_SELECT_PORT(port);
+ } else if (IS_TIGERLAKE(dev_priv)) {
+ dp_tp_reg = TGL_DP_TP_CTL(pipe);
+ trans_ddi_sel_port = TGL_TRANS_DDI_SELECT_PORT(port);
+ }
trans_ddi_func_ctl_value = intel_de_read(dev_priv,
TRANS_DDI_FUNC_CTL(pipe));
trans_conf_value = intel_de_read(dev_priv, PIPECONF(pipe));
dp_tp_ctl_value = intel_de_read(dev_priv, TGL_DP_TP_CTL(pipe));
-
trans_ddi_func_ctl_value |= TRANS_DDI_FUNC_ENABLE |
- TGL_TRANS_DDI_SELECT_PORT(port);
+ trans_ddi_sel_port;
trans_conf_value |= PIPECONF_ENABLE;
dp_tp_ctl_value |= DP_TP_CTL_ENABLE;
intel_de_write(dev_priv, PIPECONF(pipe), trans_conf_value);
- intel_de_write(dev_priv, TGL_DP_TP_CTL(pipe), dp_tp_ctl_value);
+ intel_de_write(dev_priv, dp_tp_reg, dp_tp_ctl_value);
intel_de_write(dev_priv, TRANS_DDI_FUNC_CTL(pipe),
trans_ddi_func_ctl_value);
}
@@ -5557,6 +5575,7 @@ static u8 intel_dp_autotest_phy_pattern(struct intel_dp
*intel_dp)
static void intel_dp_handle_test_request(struct intel_dp *intel_dp)
{
struct drm_i915_private *i915 = dp_to_i915(intel_dp);
+ struct drm_i915_private *dev_priv = i915;
u8 response = DP_TEST_NAK;
u8 request = 0;
int status;
@@ -5582,6 +5601,11 @@ static void intel_dp_handle_test_request(struct intel_dp
*intel_dp)
response = intel_dp_autotest_edid(intel_dp);
break;
case DP_TEST_LINK_PHY_TEST_PATTERN:
+ if (!IS_ELKHARTLAKE(dev_priv) || !IS_TIGERLAKE(dev_priv)) {
+ drm_dbg_kms(>drm,
+ "PHY compliance for platform not supported\n");
+ return;
+ }
drm_dbg_kms(>drm, "PHY_PATTERN test requested\n");
response = intel_dp_autotest_phy_pattern(intel_dp);
break;
--
2.7.4
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
