* [PATCH] drm/i915/dp: DP PHY compliance for JSL
@ 2020-06-04 5:03 Vidya Srinivas
2020-06-04 19:06 ` [Intel-gfx] " Ville Syrjälä
` (3 more replies)
0 siblings, 4 replies; 23+ messages in thread
From: Vidya Srinivas @ 2020-06-04 5:03 UTC (permalink / raw)
To: intel-gfx, dri-devel
Cc: Vidya Srinivas, animesh.manna, manasi.d.navare, uma.shankar,
shawn.c.lee, Khaled Almahallawy
Signed-off-by: Khaled Almahallawy <khaled.almahallawy@intel.com>
Signed-off-by: Vidya Srinivas <vidya.srinivas@intel.com>
---
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(&i915->drm,
+ "PHY compliance for platform not supported\n");
+ return;
+ }
drm_dbg_kms(&i915->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
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915/dp: DP PHY compliance for JSL
2020-06-04 5:03 [PATCH] drm/i915/dp: DP PHY compliance for JSL Vidya Srinivas
@ 2020-06-04 19:06 ` Ville Syrjälä
2020-06-04 20:01 ` Almahallawy, Khaled
2020-09-04 4:13 ` [PATCH 1/3] drm/dp: Add PHY_TEST_PATTERN CP2520 Pattern 2 and 3 Vidya Srinivas
` (2 subsequent siblings)
3 siblings, 1 reply; 23+ messages in thread
From: Ville Syrjälä @ 2020-06-04 19:06 UTC (permalink / raw)
To: Vidya Srinivas; +Cc: intel-gfx, dri-devel
On Thu, Jun 04, 2020 at 10:33:48AM +0530, Vidya Srinivas wrote:
> Signed-off-by: Khaled Almahallawy <khaled.almahallawy@intel.com>
> Signed-off-by: Vidya Srinivas <vidya.srinivas@intel.com>
> ---
> 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)) {
> + drm_dbg_kms(&i915->drm,
> + "PHY compliance for platform not supported\n");
> + return;
> + }
> drm_dbg_kms(&i915->drm, "PHY_PATTERN test requested\n");
> response = intel_dp_autotest_phy_pattern(intel_dp);
> break;
> --
> 2.7.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915/dp: DP PHY compliance for JSL
2020-06-04 19:06 ` [Intel-gfx] " Ville Syrjälä
@ 2020-06-04 20:01 ` Almahallawy, Khaled
2020-06-04 21:03 ` Ville Syrjälä
2020-06-12 18:33 ` Manasi Navare
0 siblings, 2 replies; 23+ messages in thread
From: Almahallawy, Khaled @ 2020-06-04 20:01 UTC (permalink / raw)
To: ville.syrjala, Taylor, Clinton A, Srinivas, Vidya; +Cc: intel-gfx, dri-devel
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 <khaled.almahallawy@intel.com>
> > Signed-off-by: Vidya Srinivas <vidya.srinivas@intel.com>
> > ---
> > 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 0000:00:02.0: [drm:intel_hpd_irq_handler [i915]]
digital hpd on [ENCODER:308:DDI F] - long
[ 106.810916] i915 0000:00:02.0: [drm:intel_hpd_irq_handler [i915]]
Received HPD interrupt on PIN 9 - cnt: 10
[ 106.811026] i915 0000:00:02.0: [drm:intel_dp_hpd_pulse [i915]] got
hpd irq on [ENCODER:308:DDI F] - long
[ 106.811095] i915 0000:00:02.0: [drm:i915_hotplug_work_func [i915]]
running encoder hotplug functions
[ 106.811184] i915 0000:00:02.0: [drm:i915_hotplug_work_func [i915]]
Connector DP-3 (pin 9) received hotplug event. (retry 0)
[ 106.811227] i915 0000:00:02.0: [drm:intel_dp_detect [i915]]
[CONNECTOR:309:DP-3]
[ 106.811292] i915 0000:00:02.0: [drm:intel_power_well_enable [i915]]
enabling TC cold off
[ 106.811365] i915 0000:00:02.0: [drm:tgl_tc_cold_request [i915]] TC
cold block succeeded
[ 106.811489] i915 0000:00:02.0: [drm:__intel_tc_port_lock [i915]]
Port F/TC#3: TC port mode reset (tbt-alt -> dp-alt)
[ 106.811663] i915 0000:00:02.0: [drm:intel_power_well_enable [i915]]
enabling AUX F TC3
[ 106.812449] [drm:drm_dp_dpcd_read] AUX F/port F: 0x00000 AUX ->
(ret= 15) 12 14 04 80 00 00 01 00 00 00 00 00 00 00 00
[ 106.812484] i915 0000: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 0x0000
[ 106.813891] [drm:drm_dp_dpcd_read] AUX F/port F: 0x00200 AUX ->
(ret= 1) 01
[ 106.813940] i915 0000:00:02.0: [drm:intel_dp_print_rates [i915]]
source rates: 162000, 216000, 270000, 324000, 432000, 540000, 648000,
810000
[ 106.813974] i915 0000:00:02.0: [drm:intel_dp_print_rates [i915]]
sink rates: 162000, 270000, 540000
[ 106.814007] i915 0000:00:02.0: [drm:intel_dp_print_rates [i915]]
common rates: 162000, 270000, 540000
[ 106.814550] [drm:drm_dp_dpcd_read] AUX F/port F: 0x00021 AUX ->
(ret= 1) 00
[ 106.814583] i915 0000:00:02.0: [drm:intel_dp_detect [i915]]
[ENCODER:308:DDI F] MST support: port: yes, sink: no, modparam: yes
.....
[ 106.927291] i915 0000:00:02.0: [drm:intel_dp_check_service_irq
[i915]] PHY_PATTERN test requested
[ 106.927897] [drm:drm_dp_dpcd_read] AUX F/port F: 0x00219 AUX ->
(ret= 1) 0a
[ 106.928507] [drm:drm_dp_dpcd_read] AUX F/port F: 0x00220 AUX ->
(ret= 1) 04
[ 106.929143] [drm:drm_dp_dpcd_read] AUX F/port F: 0x00248 AUX ->
(ret= 1) 00
[ 106.929824] [drm:drm_dp_dpcd_read] AUX F/port F: 0x00202 AUX ->
(ret= 6) 00 00 80 00 00 00
[ 106.929830] BUG: kernel NULL pointer dereference, address:
0000000000000578
[ 106.936809] #PF: supervisor read access in kernel mode
[ 106.941953] #PF: error_code(0x0000) - not-present page
[ 106.947082] PGD 0 P4D 0
[ 106.949643] Oops: 0000 [#1] PREEMPT SMP NOPTI
[ 106.954010] CPU: 6 PID: 200 Comm: kworker/6:2 Not tainted 5.7.0-rc7-
CI-CI_DRM_8566+ #5
[ 106.975251] Workqueue: events i915_hotplug_work_func [i915]
[ 106.980887] RIP: 0010:intel_dp_process_phy_request+0x94/0x5a0 [i915]
[ 106.987239] Code: 48 83 c4 20 5b 5d 41 5c 41 5d 41 5e 41 5f c3 48 8d
74 24 12 4c 89 f7 e8 3a 3e 00 00 49 8b 86 28 ff ff ff 49 8b 9e d8 fe ff
ff <48> 63 80 78 05 00 00 8b 93 54 0d 00 00 48 8d ab e8 0e 00 00 48 89
[ 107.005890] RSP: 0018:ffffc9000046fb20 EFLAGS: 00010246
I plan to temporarily fix this issue by ignoreing scope request on long
HPD, until we have modeset based implementation.
> > }
> >
> > 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(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));
> > -
> > 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(&i915->drm,
> > + "PHY compliance for platform not
> > supported\n");
> > + return;
> > + }
> > drm_dbg_kms(&i915->drm, "PHY_PATTERN test
> > requested\n");
> > response = intel_dp_autotest_phy_pattern(intel_dp);
> > break;
> > --
> > 2.7.4
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915/dp: DP PHY compliance for JSL
2020-06-04 20:01 ` Almahallawy, Khaled
@ 2020-06-04 21:03 ` Ville Syrjälä
2020-06-12 18:25 ` Manasi Navare
2020-06-12 18:33 ` Manasi Navare
1 sibling, 1 reply; 23+ messages in thread
From: Ville Syrjälä @ 2020-06-04 21:03 UTC (permalink / raw)
To: Almahallawy, Khaled; +Cc: intel-gfx, Srinivas, Vidya, dri-devel
On Thu, Jun 04, 2020 at 08:01:03PM +0000, 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 <khaled.almahallawy@intel.com>
> > > Signed-off-by: Vidya Srinivas <vidya.srinivas@intel.com>
> > > ---
> > > 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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915/dp: DP PHY compliance for JSL
2020-06-04 21:03 ` Ville Syrjälä
@ 2020-06-12 18:25 ` Manasi Navare
2020-06-12 18:36 ` Ville Syrjälä
0 siblings, 1 reply; 23+ messages in thread
From: Manasi Navare @ 2020-06-12 18:25 UTC (permalink / raw)
To: Ville Syrjälä
Cc: intel-gfx, Srinivas, Vidya, dri-devel, Almahallawy, Khaled
On Fri, Jun 05, 2020 at 12:03:19AM +0300, Ville Syrjälä wrote:
> On Thu, Jun 04, 2020 at 08:01:03PM +0000, 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 <khaled.almahallawy@intel.com>
> > > > Signed-off-by: Vidya Srinivas <vidya.srinivas@intel.com>
> > > > ---
> > > > 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 DP_TP_CTL etc and sets up the PHY patterns and
reenables the already set link?
We have thoroughly tested this using the scopes and DPR 100 and it has been working correctly
with the existing IGT compliance tool so IMO no need to rewrite the entire set of patches.
Ville, Khaled ?
Regards
Manasi
>
> --
> Ville Syrjälä
> Intel
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915/dp: DP PHY compliance for JSL
2020-06-04 20:01 ` Almahallawy, Khaled
2020-06-04 21:03 ` Ville Syrjälä
@ 2020-06-12 18:33 ` Manasi Navare
2020-06-12 18:39 ` Ville Syrjälä
1 sibling, 1 reply; 23+ messages in thread
From: Manasi Navare @ 2020-06-12 18:33 UTC (permalink / raw)
To: Almahallawy, Khaled; +Cc: intel-gfx, dri-devel, Srinivas, Vidya
On Thu, Jun 04, 2020 at 08:01:03PM +0000, 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 <khaled.almahallawy@intel.com>
> > > Signed-off-by: Vidya Srinivas <vidya.srinivas@intel.com>
> > > ---
> > > 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 0000:00:02.0: [drm:intel_hpd_irq_handler [i915]]
> digital hpd on [ENCODER:308:DDI F] - long
> [ 106.810916] i915 0000:00:02.0: [drm:intel_hpd_irq_handler [i915]]
> Received HPD interrupt on PIN 9 - cnt: 10
> [ 106.811026] i915 0000:00:02.0: [drm:intel_dp_hpd_pulse [i915]] got
> hpd irq on [ENCODER:308:DDI F] - long
> [ 106.811095] i915 0000:00:02.0: [drm:i915_hotplug_work_func [i915]]
> running encoder hotplug functions
> [ 106.811184] i915 0000:00:02.0: [drm:i915_hotplug_work_func [i915]]
> Connector DP-3 (pin 9) received hotplug event. (retry 0)
> [ 106.811227] i915 0000:00:02.0: [drm:intel_dp_detect [i915]]
> [CONNECTOR:309:DP-3]
> [ 106.811292] i915 0000:00:02.0: [drm:intel_power_well_enable [i915]]
> enabling TC cold off
> [ 106.811365] i915 0000:00:02.0: [drm:tgl_tc_cold_request [i915]] TC
> cold block succeeded
> [ 106.811489] i915 0000:00:02.0: [drm:__intel_tc_port_lock [i915]]
> Port F/TC#3: TC port mode reset (tbt-alt -> dp-alt)
> [ 106.811663] i915 0000:00:02.0: [drm:intel_power_well_enable [i915]]
> enabling AUX F TC3
> [ 106.812449] [drm:drm_dp_dpcd_read] AUX F/port F: 0x00000 AUX ->
> (ret= 15) 12 14 04 80 00 00 01 00 00 00 00 00 00 00 00
> [ 106.812484] i915 0000: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 0x0000
> [ 106.813891] [drm:drm_dp_dpcd_read] AUX F/port F: 0x00200 AUX ->
> (ret= 1) 01
> [ 106.813940] i915 0000:00:02.0: [drm:intel_dp_print_rates [i915]]
> source rates: 162000, 216000, 270000, 324000, 432000, 540000, 648000,
> 810000
> [ 106.813974] i915 0000:00:02.0: [drm:intel_dp_print_rates [i915]]
> sink rates: 162000, 270000, 540000
> [ 106.814007] i915 0000:00:02.0: [drm:intel_dp_print_rates [i915]]
> common rates: 162000, 270000, 540000
> [ 106.814550] [drm:drm_dp_dpcd_read] AUX F/port F: 0x00021 AUX ->
> (ret= 1) 00
> [ 106.814583] i915 0000:00:02.0: [drm:intel_dp_detect [i915]]
> [ENCODER:308:DDI F] MST support: port: yes, sink: no, modparam: yes
>
> .....
>
> [ 106.927291] i915 0000:00:02.0: [drm:intel_dp_check_service_irq
> [i915]] PHY_PATTERN test requested
> [ 106.927897] [drm:drm_dp_dpcd_read] AUX F/port F: 0x00219 AUX ->
> (ret= 1) 0a
> [ 106.928507] [drm:drm_dp_dpcd_read] AUX F/port F: 0x00220 AUX ->
> (ret= 1) 04
> [ 106.929143] [drm:drm_dp_dpcd_read] AUX F/port F: 0x00248 AUX ->
> (ret= 1) 00
> [ 106.929824] [drm:drm_dp_dpcd_read] AUX F/port F: 0x00202 AUX ->
> (ret= 6) 00 00 80 00 00 00
> [ 106.929830] BUG: kernel NULL pointer dereference, address:
> 0000000000000578
> [ 106.936809] #PF: supervisor read access in kernel mode
> [ 106.941953] #PF: error_code(0x0000) - not-present page
> [ 106.947082] PGD 0 P4D 0
> [ 106.949643] Oops: 0000 [#1] PREEMPT SMP NOPTI
> [ 106.954010] CPU: 6 PID: 200 Comm: kworker/6:2 Not tainted 5.7.0-rc7-
> CI-CI_DRM_8566+ #5
> [ 106.975251] Workqueue: events i915_hotplug_work_func [i915]
> [ 106.980887] RIP: 0010:intel_dp_process_phy_request+0x94/0x5a0 [i915]
> [ 106.987239] Code: 48 83 c4 20 5b 5d 41 5c 41 5d 41 5e 41 5f c3 48 8d
> 74 24 12 4c 89 f7 e8 3a 3e 00 00 49 8b 86 28 ff ff ff 49 8b 9e d8 fe ff
> ff <48> 63 80 78 05 00 00 8b 93 54 0d 00 00 48 8d ab e8 0e 00 00 48 89
> [ 107.005890] RSP: 0018:ffffc9000046fb20 EFLAGS: 00010246
>
> I plan to temporarily fix this issue by ignoreing scope request on long
> HPD, until we have modeset based implementation.
>
> > > }
> > >
> > > 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(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));
> > > -
> > > 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(&i915->drm,
> > > + "PHY compliance for platform not
> > > supported\n");
> > > + return;
> > > + }
> > > drm_dbg_kms(&i915->drm, "PHY_PATTERN test
> > > requested\n");
> > > response = intel_dp_autotest_phy_pattern(intel_dp);
> > > break;
> > > --
> > > 2.7.4
> > >
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
> >
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915/dp: DP PHY compliance for JSL
2020-06-12 18:25 ` Manasi Navare
@ 2020-06-12 18:36 ` Ville Syrjälä
2020-06-12 18:44 ` Manasi Navare
0 siblings, 1 reply; 23+ messages in thread
From: Ville Syrjälä @ 2020-06-12 18:36 UTC (permalink / raw)
To: Manasi Navare; +Cc: intel-gfx, Srinivas, Vidya, dri-devel, Almahallawy, Khaled
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 +0000, 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 <khaled.almahallawy@intel.com>
> > > > > Signed-off-by: Vidya Srinivas <vidya.srinivas@intel.com>
> > > > > ---
> > > > > 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 DP_TP_CTL etc and sets up the PHY patterns and
> reenables the already set link?
>
> We have thoroughly tested this using the scopes and DPR 100 and it has been working correctly
> with the existing IGT compliance tool so IMO no need to rewrite the entire set of patches.
>
> Ville, Khaled ?
You're just multiplying the amount of work and bugs we have
for every platform.
And as said testing some special compliance paths proves
pretty much nothing about the real code paths. So the only
point of that code AFAICS it to tick some "we haz
compliance code?" checkbox in some random spreadsheet instead
of actually providing evidence that our real code works
correctly.
--
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915/dp: DP PHY compliance for JSL
2020-06-12 18:33 ` Manasi Navare
@ 2020-06-12 18:39 ` Ville Syrjälä
0 siblings, 0 replies; 23+ messages in thread
From: Ville Syrjälä @ 2020-06-12 18:39 UTC (permalink / raw)
To: Manasi Navare; +Cc: intel-gfx, Srinivas, Vidya, dri-devel, Almahallawy, Khaled
On Fri, Jun 12, 2020 at 11:33:45AM -0700, Manasi Navare wrote:
> On Thu, Jun 04, 2020 at 08:01:03PM +0000, 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 <khaled.almahallawy@intel.com>
> > > > Signed-off-by: Vidya Srinivas <vidya.srinivas@intel.com>
> > > > ---
> > > > 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 0000:00:02.0: [drm:intel_hpd_irq_handler [i915]]
> > digital hpd on [ENCODER:308:DDI F] - long
> > [ 106.810916] i915 0000:00:02.0: [drm:intel_hpd_irq_handler [i915]]
> > Received HPD interrupt on PIN 9 - cnt: 10
> > [ 106.811026] i915 0000:00:02.0: [drm:intel_dp_hpd_pulse [i915]] got
> > hpd irq on [ENCODER:308:DDI F] - long
> > [ 106.811095] i915 0000:00:02.0: [drm:i915_hotplug_work_func [i915]]
> > running encoder hotplug functions
> > [ 106.811184] i915 0000:00:02.0: [drm:i915_hotplug_work_func [i915]]
> > Connector DP-3 (pin 9) received hotplug event. (retry 0)
> > [ 106.811227] i915 0000:00:02.0: [drm:intel_dp_detect [i915]]
> > [CONNECTOR:309:DP-3]
> > [ 106.811292] i915 0000:00:02.0: [drm:intel_power_well_enable [i915]]
> > enabling TC cold off
> > [ 106.811365] i915 0000:00:02.0: [drm:tgl_tc_cold_request [i915]] TC
> > cold block succeeded
> > [ 106.811489] i915 0000:00:02.0: [drm:__intel_tc_port_lock [i915]]
> > Port F/TC#3: TC port mode reset (tbt-alt -> dp-alt)
> > [ 106.811663] i915 0000:00:02.0: [drm:intel_power_well_enable [i915]]
> > enabling AUX F TC3
> > [ 106.812449] [drm:drm_dp_dpcd_read] AUX F/port F: 0x00000 AUX ->
> > (ret= 15) 12 14 04 80 00 00 01 00 00 00 00 00 00 00 00
> > [ 106.812484] i915 0000: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 0x0000
> > [ 106.813891] [drm:drm_dp_dpcd_read] AUX F/port F: 0x00200 AUX ->
> > (ret= 1) 01
> > [ 106.813940] i915 0000:00:02.0: [drm:intel_dp_print_rates [i915]]
> > source rates: 162000, 216000, 270000, 324000, 432000, 540000, 648000,
> > 810000
> > [ 106.813974] i915 0000:00:02.0: [drm:intel_dp_print_rates [i915]]
> > sink rates: 162000, 270000, 540000
> > [ 106.814007] i915 0000:00:02.0: [drm:intel_dp_print_rates [i915]]
> > common rates: 162000, 270000, 540000
> > [ 106.814550] [drm:drm_dp_dpcd_read] AUX F/port F: 0x00021 AUX ->
> > (ret= 1) 00
> > [ 106.814583] i915 0000:00:02.0: [drm:intel_dp_detect [i915]]
> > [ENCODER:308:DDI F] MST support: port: yes, sink: no, modparam: yes
> >
> > .....
> >
> > [ 106.927291] i915 0000:00:02.0: [drm:intel_dp_check_service_irq
> > [i915]] PHY_PATTERN test requested
> > [ 106.927897] [drm:drm_dp_dpcd_read] AUX F/port F: 0x00219 AUX ->
> > (ret= 1) 0a
> > [ 106.928507] [drm:drm_dp_dpcd_read] AUX F/port F: 0x00220 AUX ->
> > (ret= 1) 04
> > [ 106.929143] [drm:drm_dp_dpcd_read] AUX F/port F: 0x00248 AUX ->
> > (ret= 1) 00
> > [ 106.929824] [drm:drm_dp_dpcd_read] AUX F/port F: 0x00202 AUX ->
> > (ret= 6) 00 00 80 00 00 00
> > [ 106.929830] BUG: kernel NULL pointer dereference, address:
> > 0000000000000578
> > [ 106.936809] #PF: supervisor read access in kernel mode
> > [ 106.941953] #PF: error_code(0x0000) - not-present page
> > [ 106.947082] PGD 0 P4D 0
> > [ 106.949643] Oops: 0000 [#1] PREEMPT SMP NOPTI
> > [ 106.954010] CPU: 6 PID: 200 Comm: kworker/6:2 Not tainted 5.7.0-rc7-
> > CI-CI_DRM_8566+ #5
> > [ 106.975251] Workqueue: events i915_hotplug_work_func [i915]
> > [ 106.980887] RIP: 0010:intel_dp_process_phy_request+0x94/0x5a0 [i915]
> > [ 106.987239] Code: 48 83 c4 20 5b 5d 41 5c 41 5d 41 5e 41 5f c3 48 8d
> > 74 24 12 4c 89 f7 e8 3a 3e 00 00 49 8b 86 28 ff ff ff 49 8b 9e d8 fe ff
> > ff <48> 63 80 78 05 00 00 8b 93 54 0d 00 00 48 8d ab e8 0e 00 00 48 89
> > [ 107.005890] RSP: 0018:ffffc9000046fb20 EFLAGS: 00010246
> >
> > I plan to temporarily fix this issue by ignoreing scope request on long
> > HPD, until we have modeset based implementation.
> >
> > > > }
> > > >
> > > > 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(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));
> > > > -
> > > > 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(&i915->drm,
> > > > + "PHY compliance for platform not
> > > > supported\n");
> > > > + return;
> > > > + }
> > > > drm_dbg_kms(&i915->drm, "PHY_PATTERN test
> > > > requested\n");
> > > > response = intel_dp_autotest_phy_pattern(intel_dp);
> > > > break;
> > > > --
> > > > 2.7.4
> > > >
> > > > _______________________________________________
> > > > Intel-gfx mailing list
> > > > Intel-gfx@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > >
> > >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915/dp: DP PHY compliance for JSL
2020-06-12 18:36 ` Ville Syrjälä
@ 2020-06-12 18:44 ` Manasi Navare
2020-06-12 19:01 ` Ville Syrjälä
0 siblings, 1 reply; 23+ messages in thread
From: Manasi Navare @ 2020-06-12 18:44 UTC (permalink / raw)
To: Ville Syrjälä
Cc: intel-gfx, Srinivas, Vidya, dri-devel, Almahallawy, Khaled
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 +0000, 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 <khaled.almahallawy@intel.com>
> > > > > > Signed-off-by: Vidya Srinivas <vidya.srinivas@intel.com>
> > > > > > ---
> > > > > > 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 DP_TP_CTL etc and sets up the PHY patterns and
> > reenables the already set link?
> >
> > We have thoroughly tested this using the scopes and DPR 100 and it has been working correctly
> > with the existing IGT compliance tool so IMO no need to rewrite the entire set of patches.
> >
> > Ville, Khaled ?
>
> You're just multiplying the amount of work and bugs we have
> for every platform.
>
> And as said testing some special compliance paths proves
> pretty much nothing about the real code paths. So the only
> point of that code AFAICS it to tick some "we haz
> compliance code?" checkbox in some random spreadsheet instead
> of actually providing evidence that our real code works
> correctly.
>
I thougt the whole point of PHY compliance is not to be able to see if the
driver can do a modeset but just to confirm that driver is able to send
the requested patterns out on already enabled link. So shouldnt doing this
directly through the phy request handling on short pulse suffice?
But if we want to insert this in the modeset what should be the flow:
- AFter getting PHY request, store the requested PHY patterns, send a uevent
- This will trigger a complete modeset, in this path for atomic check, see
if PHY compliance test active then ignore recomputing the parameters and
also in the commit tail, only disable the Pipeconf, dp_tp_ctl and send these patterns
and then reenable?
Manasi
> --
> Ville Syrjälä
> Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915/dp: DP PHY compliance for JSL
2020-06-12 18:44 ` Manasi Navare
@ 2020-06-12 19:01 ` Ville Syrjälä
2020-06-12 19:12 ` Manasi Navare
0 siblings, 1 reply; 23+ messages in thread
From: Ville Syrjälä @ 2020-06-12 19:01 UTC (permalink / raw)
To: Manasi Navare; +Cc: intel-gfx, Srinivas, Vidya, dri-devel, Almahallawy, Khaled
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 +0000, 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 <khaled.almahallawy@intel.com>
> > > > > > > Signed-off-by: Vidya Srinivas <vidya.srinivas@intel.com>
> > > > > > > ---
> > > > > > > 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 DP_TP_CTL etc and sets up the PHY patterns and
> > > reenables the already set link?
> > >
> > > We have thoroughly tested this using the scopes and DPR 100 and it has been working correctly
> > > with the existing IGT compliance tool so IMO no need to rewrite the entire set of patches.
> > >
> > > Ville, Khaled ?
> >
> > You're just multiplying the amount of work and bugs we have
> > for every platform.
> >
> > And as said testing some special compliance paths proves
> > pretty much nothing about the real code paths. So the only
> > point of that code AFAICS it to tick some "we haz
> > compliance code?" checkbox in some random spreadsheet instead
> > of actually providing evidence that our real code works
> > correctly.
> >
>
> I thougt the whole point of PHY compliance is not to be able to see if the
> driver can do a modeset but just to confirm that driver is able to send
> the requested patterns out on already enabled link. So shouldnt doing this
> directly through the phy request handling on short pulse suffice?
You're not proving the driver proper can transmit the requested stuff,
you're only proving the special compliance code can do that. I could
easily break the normal codepaths and yet this magic compliance thing
could still indicate that everything is hunky dory.
>
> But if we want to insert this in the modeset what should be the flow:
> - AFter getting PHY request, store the requested PHY patterns, send a uevent
You don't really need any uevent. We coukd do the stuff directly from
the hotplug work.
> - This will trigger a complete modeset, in this path for atomic check, see
> if PHY compliance test active then ignore recomputing the parameters and
> also in the commit tail, only disable the Pipeconf, dp_tp_ctl and send these patterns
> and then reenable?
We should just do a full modeset if possible. Randomly turning the
pipe/etc. on/off without following the proper modeset sequence is
dubious at best.
--
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915/dp: DP PHY compliance for JSL
2020-06-12 19:01 ` Ville Syrjälä
@ 2020-06-12 19:12 ` Manasi Navare
2020-06-12 19:21 ` Ville Syrjälä
0 siblings, 1 reply; 23+ messages in thread
From: Manasi Navare @ 2020-06-12 19:12 UTC (permalink / raw)
To: Ville Syrjälä
Cc: intel-gfx, Srinivas, Vidya, dri-devel, Almahallawy, Khaled
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 +0000, 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 <khaled.almahallawy@intel.com>
> > > > > > > > Signed-off-by: Vidya Srinivas <vidya.srinivas@intel.com>
> > > > > > > > ---
> > > > > > > > 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 DP_TP_CTL etc and sets up the PHY patterns and
> > > > reenables the already set link?
> > > >
> > > > We have thoroughly tested this using the scopes and DPR 100 and it has been working correctly
> > > > with the existing IGT compliance tool so IMO no need to rewrite the entire set of patches.
> > > >
> > > > Ville, Khaled ?
> > >
> > > You're just multiplying the amount of work and bugs we have
> > > for every platform.
> > >
> > > And as said testing some special compliance paths proves
> > > pretty much nothing about the real code paths. So the only
> > > point of that code AFAICS it to tick some "we haz
> > > compliance code?" checkbox in some random spreadsheet instead
> > > of actually providing evidence that our real code works
> > > correctly.
> > >
> >
> > I thougt the whole point of PHY compliance is not to be able to see if the
> > driver can do a modeset but just to confirm that driver is able to send
> > the requested patterns out on already enabled link. So shouldnt doing this
> > directly through the phy request handling on short pulse suffice?
>
> You're not proving the driver proper can transmit the requested stuff,
> you're only proving the special compliance code can do that. I could
> easily break the normal codepaths and yet this magic compliance thing
> could still indicate that everything is hunky dory.
>
> >
> > But if we want to insert this in the modeset what should be the flow:
> > - AFter getting PHY request, store the requested PHY patterns, send a uevent
>
> You don't really need any uevent. We coukd do the stuff directly from
> the hotplug work.
>
> > - This will trigger a complete modeset, in this path for atomic check, see
> > if PHY compliance test active then ignore recomputing the parameters and
> > also in the commit tail, only disable the Pipeconf, dp_tp_ctl and send these patterns
> > and then reenable?
>
> We should just do a full modeset if possible. Randomly turning the
> pipe/etc. on/off without following the proper modeset sequence is
> dubious at best.
how do we trigger a full modeset directly from the hotplug work just from
within the kernel? We faced the same problem with link layer compliance
and hence we decided to send the uevent there to trigger a ful modeset.
How do you suggest we do otherwise?
Manasi
>
> --
> Ville Syrjälä
> Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915/dp: DP PHY compliance for JSL
2020-06-12 19:12 ` Manasi Navare
@ 2020-06-12 19:21 ` Ville Syrjälä
2020-06-12 19:38 ` Manasi Navare
0 siblings, 1 reply; 23+ messages in thread
From: Ville Syrjälä @ 2020-06-12 19:21 UTC (permalink / raw)
To: Manasi Navare; +Cc: intel-gfx, Srinivas, Vidya, dri-devel, Almahallawy, Khaled
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 +0000, 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 <khaled.almahallawy@intel.com>
> > > > > > > > > Signed-off-by: Vidya Srinivas <vidya.srinivas@intel.com>
> > > > > > > > > ---
> > > > > > > > > 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 DP_TP_CTL etc and sets up the PHY patterns and
> > > > > reenables the already set link?
> > > > >
> > > > > We have thoroughly tested this using the scopes and DPR 100 and it has been working correctly
> > > > > with the existing IGT compliance tool so IMO no need to rewrite the entire set of patches.
> > > > >
> > > > > Ville, Khaled ?
> > > >
> > > > You're just multiplying the amount of work and bugs we have
> > > > for every platform.
> > > >
> > > > And as said testing some special compliance paths proves
> > > > pretty much nothing about the real code paths. So the only
> > > > point of that code AFAICS it to tick some "we haz
> > > > compliance code?" checkbox in some random spreadsheet instead
> > > > of actually providing evidence that our real code works
> > > > correctly.
> > > >
> > >
> > > I thougt the whole point of PHY compliance is not to be able to see if the
> > > driver can do a modeset but just to confirm that driver is able to send
> > > the requested patterns out on already enabled link. So shouldnt doing this
> > > directly through the phy request handling on short pulse suffice?
> >
> > You're not proving the driver proper can transmit the requested stuff,
> > you're only proving the special compliance code can do that. I could
> > easily break the normal codepaths and yet this magic compliance thing
> > could still indicate that everything is hunky dory.
> >
>
> > >
> > > But if we want to insert this in the modeset what should be the flow:
> > > - AFter getting PHY request, store the requested PHY patterns, send a uevent
> >
> > You don't really need any uevent. We coukd do the stuff directly from
> > the hotplug work.
> >
> > > - This will trigger a complete modeset, in this path for atomic check, see
> > > if PHY compliance test active then ignore recomputing the parameters and
> > > also in the commit tail, only disable the Pipeconf, dp_tp_ctl and send these patterns
> > > and then reenable?
> >
> > We should just do a full modeset if possible. Randomly turning the
> > pipe/etc. on/off without following the proper modeset sequence is
> > dubious at best.
>
> how do we trigger a full modeset directly from the hotplug work just from
> within the kernel? We faced the same problem with link layer compliance
> and hence we decided to send the uevent there to trigger a ful modeset.
The full modeset via userspace route is only needed if the resolution
needs to be changed since that's something userspace gets to decide.
If the current mode is still OK we can directly trigger the modeset
from the kernel. Not sure if we do or not.
We do a full modeset for HDMI when the sink forgets that scrambling
was supposed to be on, and I'm a bit tempted to do the same for
plain old DP retraining to get rid of the special case code for
that (and to actually follow the modeset seqeunce properly when
doing retraining).
--
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915/dp: DP PHY compliance for JSL
2020-06-12 19:21 ` Ville Syrjälä
@ 2020-06-12 19:38 ` Manasi Navare
2020-06-15 16:19 ` Ville Syrjälä
0 siblings, 1 reply; 23+ messages in thread
From: Manasi Navare @ 2020-06-12 19:38 UTC (permalink / raw)
To: Ville Syrjälä
Cc: intel-gfx, Srinivas, Vidya, dri-devel, Almahallawy, Khaled
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 +0000, 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 <khaled.almahallawy@intel.com>
> > > > > > > > > > Signed-off-by: Vidya Srinivas <vidya.srinivas@intel.com>
> > > > > > > > > > ---
> > > > > > > > > > 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 DP_TP_CTL etc and sets up the PHY patterns and
> > > > > > reenables the already set link?
> > > > > >
> > > > > > We have thoroughly tested this using the scopes and DPR 100 and it has been working correctly
> > > > > > with the existing IGT compliance tool so IMO no need to rewrite the entire set of patches.
> > > > > >
> > > > > > Ville, Khaled ?
> > > > >
> > > > > You're just multiplying the amount of work and bugs we have
> > > > > for every platform.
> > > > >
> > > > > And as said testing some special compliance paths proves
> > > > > pretty much nothing about the real code paths. So the only
> > > > > point of that code AFAICS it to tick some "we haz
> > > > > compliance code?" checkbox in some random spreadsheet instead
> > > > > of actually providing evidence that our real code works
> > > > > correctly.
> > > > >
> > > >
> > > > I thougt the whole point of PHY compliance is not to be able to see if the
> > > > driver can do a modeset but just to confirm that driver is able to send
> > > > the requested patterns out on already enabled link. So shouldnt doing this
> > > > directly through the phy request handling on short pulse suffice?
> > >
> > > You're not proving the driver proper can transmit the requested stuff,
> > > you're only proving the special compliance code can do that. I could
> > > easily break the normal codepaths and yet this magic compliance thing
> > > could still indicate that everything is hunky dory.
> > >
> >
> > > >
> > > > But if we want to insert this in the modeset what should be the flow:
> > > > - AFter getting PHY request, store the requested PHY patterns, send a uevent
> > >
> > > You don't really need any uevent. We coukd do the stuff directly from
> > > the hotplug work.
> > >
> > > > - This will trigger a complete modeset, in this path for atomic check, see
> > > > if PHY compliance test active then ignore recomputing the parameters and
> > > > also in the commit tail, only disable the Pipeconf, dp_tp_ctl and send these patterns
> > > > and then reenable?
> > >
> > > We should just do a full modeset if possible. Randomly turning the
> > > pipe/etc. on/off without following the proper modeset sequence is
> > > dubious at best.
> >
> > how do we trigger a full modeset directly from the hotplug work just from
> > within the kernel? We faced the same problem with link layer compliance
> > and hence we decided to send the uevent there to trigger a ful modeset.
>
> The full modeset via userspace route is only needed if the resolution
> needs to be changed since that's something userspace gets to decide.
> If the current mode is still OK we can directly trigger the modeset
> from the kernel. Not sure if we do or not.
>
> We do a full modeset for HDMI when the sink forgets that scrambling
> was supposed to be on, and I'm a bit tempted to do the same for
> plain old DP retraining to get rid of the special case code for
> that (and to actually follow the modeset seqeunce properly when
> doing retraining).
>
For retraining we dont have any special case code right, we just fallback and then send uevent.
Oh but do you mean like getting rid of setting the link status and forcing a full modeset etc?
So for PHY compliance, we do something similar to calling modeset_pipe() from
intel_hdmi_reset_link()? So call this modeset_pipe from intel_dp_autotest_phy_pattern() after
storing the requested phy patterns in a compliance struct?
Manasi
> --
> Ville Syrjälä
> Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915/dp: DP PHY compliance for JSL
2020-06-12 19:38 ` Manasi Navare
@ 2020-06-15 16:19 ` Ville Syrjälä
0 siblings, 0 replies; 23+ messages in thread
From: Ville Syrjälä @ 2020-06-15 16:19 UTC (permalink / raw)
To: Manasi Navare; +Cc: intel-gfx, Srinivas, Vidya, dri-devel, Almahallawy, Khaled
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 +0000, 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 <khaled.almahallawy@intel.com>
> > > > > > > > > > > Signed-off-by: Vidya Srinivas <vidya.srinivas@intel.com>
> > > > > > > > > > > ---
> > > > > > > > > > > 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 DP_TP_CTL etc and sets up the PHY patterns and
> > > > > > > reenables the already set link?
> > > > > > >
> > > > > > > We have thoroughly tested this using the scopes and DPR 100 and it has been working correctly
> > > > > > > with the existing IGT compliance tool so IMO no need to rewrite the entire set of patches.
> > > > > > >
> > > > > > > Ville, Khaled ?
> > > > > >
> > > > > > You're just multiplying the amount of work and bugs we have
> > > > > > for every platform.
> > > > > >
> > > > > > And as said testing some special compliance paths proves
> > > > > > pretty much nothing about the real code paths. So the only
> > > > > > point of that code AFAICS it to tick some "we haz
> > > > > > compliance code?" checkbox in some random spreadsheet instead
> > > > > > of actually providing evidence that our real code works
> > > > > > correctly.
> > > > > >
> > > > >
> > > > > I thougt the whole point of PHY compliance is not to be able to see if the
> > > > > driver can do a modeset but just to confirm that driver is able to send
> > > > > the requested patterns out on already enabled link. So shouldnt doing this
> > > > > directly through the phy request handling on short pulse suffice?
> > > >
> > > > You're not proving the driver proper can transmit the requested stuff,
> > > > you're only proving the special compliance code can do that. I could
> > > > easily break the normal codepaths and yet this magic compliance thing
> > > > could still indicate that everything is hunky dory.
> > > >
> > >
> > > > >
> > > > > But if we want to insert this in the modeset what should be the flow:
> > > > > - AFter getting PHY request, store the requested PHY patterns, send a uevent
> > > >
> > > > You don't really need any uevent. We coukd do the stuff directly from
> > > > the hotplug work.
> > > >
> > > > > - This will trigger a complete modeset, in this path for atomic check, see
> > > > > if PHY compliance test active then ignore recomputing the parameters and
> > > > > also in the commit tail, only disable the Pipeconf, dp_tp_ctl and send these patterns
> > > > > and then reenable?
> > > >
> > > > We should just do a full modeset if possible. Randomly turning the
> > > > pipe/etc. on/off without following the proper modeset sequence is
> > > > dubious at best.
> > >
> > > how do we trigger a full modeset directly from the hotplug work just from
> > > within the kernel? We faced the same problem with link layer compliance
> > > and hence we decided to send the uevent there to trigger a ful modeset.
> >
> > The full modeset via userspace route is only needed if the resolution
> > needs to be changed since that's something userspace gets to decide.
> > If the current mode is still OK we can directly trigger the modeset
> > from the kernel. Not sure if we do or not.
> >
> > We do a full modeset for HDMI when the sink forgets that scrambling
> > was supposed to be on, and I'm a bit tempted to do the same for
> > plain old DP retraining to get rid of the special case code for
> > that (and to actually follow the modeset seqeunce properly when
> > doing retraining).
> >
>
> For retraining we dont have any special case code right,
Yes we do. intel_dp_retrain_link().
> we just fallback and then send uevent.
> Oh but do you mean like getting rid of setting the link status and forcing a full modeset etc?
>
> So for PHY compliance, we do something similar to calling modeset_pipe() from
> intel_hdmi_reset_link()? So call this modeset_pipe from intel_dp_autotest_phy_pattern() after
> storing the requested phy patterns in a compliance struct?
The modeset moust be moved into the hotplug work. The dig_port work
shouldn't do anything except stash the request somewhere and kick off
the hotplug work.
--
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 1/3] drm/dp: Add PHY_TEST_PATTERN CP2520 Pattern 2 and 3
2020-06-04 5:03 [PATCH] drm/i915/dp: DP PHY compliance for JSL Vidya Srinivas
2020-06-04 19:06 ` [Intel-gfx] " Ville Syrjälä
@ 2020-09-04 4:13 ` Vidya Srinivas
2020-09-04 4:13 ` [PATCH] drm/i915/dp: DP PHY compliance for EHL/JSL Vidya Srinivas
` (2 more replies)
2020-09-04 4:14 ` [PATCH 1/3] drm/dp: Add PHY_TEST_PATTERN CP2520 Pattern 2 and 3 Vidya Srinivas
2020-09-04 4:29 ` [PATCH] " Vidya Srinivas
3 siblings, 3 replies; 23+ messages in thread
From: Vidya Srinivas @ 2020-09-04 4:13 UTC (permalink / raw)
To: intel-gfx, dri-devel; +Cc: Khaled Almahallawy
From: Khaled Almahallawy <khaled.almahallawy@intel.com>
Add the missing CP2520 pattern 2 and 3 phy compliance patterns
v2: cosemtic changes
Reviewed-by: Manasi Navare <manasi.d.navare@intel.com> (v1)
Signed-off-by: Khaled Almahallawy <khaled.almahallawy@intel.com>
---
drivers/gpu/drm/drm_dp_helper.c | 2 +-
include/drm/drm_dp_helper.h | 4 +++-
2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index a3c82e726057..d0fb78c6aca6 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -1583,7 +1583,7 @@ int drm_dp_get_phy_test_pattern(struct drm_dp_aux *aux,
return err;
break;
- case DP_PHY_TEST_PATTERN_CP2520:
+ case DP_PHY_TEST_PATTERN_CP2520_PAT1:
err = drm_dp_dpcd_read(aux, DP_TEST_HBR2_SCRAMBLER_RESET,
&data->hbr2_reset,
sizeof(data->hbr2_reset));
diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index e2d2df5e869e..73285b4c25a0 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -708,7 +708,9 @@
# define DP_PHY_TEST_PATTERN_ERROR_COUNT 0x2
# define DP_PHY_TEST_PATTERN_PRBS7 0x3
# define DP_PHY_TEST_PATTERN_80BIT_CUSTOM 0x4
-# define DP_PHY_TEST_PATTERN_CP2520 0x5
+# define DP_PHY_TEST_PATTERN_CP2520_PAT1 0x5
+# define DP_PHY_TEST_PATTERN_CP2520_PAT2 0x6
+# define DP_PHY_TEST_PATTERN_CP2520_PAT3 0x7
#define DP_TEST_HBR2_SCRAMBLER_RESET 0x24A
#define DP_TEST_80BIT_CUSTOM_PATTERN_7_0 0x250
--
2.7.4
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH] drm/i915/dp: DP PHY compliance for EHL/JSL
2020-09-04 4:13 ` [PATCH 1/3] drm/dp: Add PHY_TEST_PATTERN CP2520 Pattern 2 and 3 Vidya Srinivas
@ 2020-09-04 4:13 ` Vidya Srinivas
2020-09-04 4:13 ` [PATCH 2/3] drm/i915/dp: TPS4 PHY test pattern compliance support Vidya Srinivas
2020-09-04 4:13 ` [PATCH 3/3] [RFC] drm/i915/dp: DP PHY compliance for EHL/JSL Vidya Srinivas
2 siblings, 0 replies; 23+ messages in thread
From: Vidya Srinivas @ 2020-09-04 4:13 UTC (permalink / raw)
To: intel-gfx, dri-devel; +Cc: Vidya Srinivas, Khaled Almahallawy
v2: Rebased patch on top of:
https://patchwork.freedesktop.org/series/79779/
Fixed phy patterns for JSL/EHL
Add TPS4 support for JSL/EHL
Signed-off-by: Khaled Almahallawy <khaled.almahallawy@intel.com>
Signed-off-by: Vidya Srinivas <vidya.srinivas@intel.com>
---
drivers/gpu/drm/i915/display/intel_dp.c | 81 +++++++++++++++++++++++++--------
drivers/gpu/drm/i915/i915_reg.h | 18 ++++++--
2 files changed, 78 insertions(+), 21 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index a8a3ffcef5dc..1773f3d5d0f4 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -5405,25 +5405,32 @@ static void intel_dp_phy_pattern_update(struct intel_dp *intel_dp)
enum pipe pipe = crtc->pipe;
u32 pattern_val, dp_tp_ctl;
+ i915_reg_t dp_comp_reg;
+
+ if (IS_ELKHARTLAKE(dev_priv))
+ dp_comp_reg = EHL_DDI_DP_COMP_CTL(dig_port->base.port);
+ else if (IS_TIGERLAKE(dev_priv))
+ dp_comp_reg = DDI_DP_COMP_CTL(pipe);
+
switch (data->phy_pattern) {
case DP_PHY_TEST_PATTERN_NONE:
DRM_DEBUG_KMS("Disable Phy Test Pattern\n");
- intel_de_write(dev_priv, DDI_DP_COMP_CTL(pipe), 0x0);
+ intel_de_write(dev_priv, dp_comp_reg, 0x0);
break;
case DP_PHY_TEST_PATTERN_D10_2:
DRM_DEBUG_KMS("Set D10.2 Phy Test Pattern\n");
- intel_de_write(dev_priv, DDI_DP_COMP_CTL(pipe),
+ intel_de_write(dev_priv, dp_comp_reg,
DDI_DP_COMP_CTL_ENABLE | DDI_DP_COMP_CTL_D10_2);
break;
case DP_PHY_TEST_PATTERN_ERROR_COUNT:
DRM_DEBUG_KMS("Set Error Count Phy Test Pattern\n");
- intel_de_write(dev_priv, DDI_DP_COMP_CTL(pipe),
+ intel_de_write(dev_priv, dp_comp_reg,
DDI_DP_COMP_CTL_ENABLE |
DDI_DP_COMP_CTL_SCRAMBLED_0);
break;
case DP_PHY_TEST_PATTERN_PRBS7:
DRM_DEBUG_KMS("Set PRBS7 Phy Test Pattern\n");
- intel_de_write(dev_priv, DDI_DP_COMP_CTL(pipe),
+ intel_de_write(dev_priv, dp_comp_reg,
DDI_DP_COMP_CTL_ENABLE | DDI_DP_COMP_CTL_PRBS7);
break;
case DP_PHY_TEST_PATTERN_80BIT_CUSTOM:
@@ -5432,14 +5439,27 @@ static void intel_dp_phy_pattern_update(struct intel_dp *intel_dp)
* current firmware of DPR-100 could not set it, so hardcoding
* now for complaince test.
*/
- DRM_DEBUG_KMS("Set 80Bit Custom Phy Test Pattern 0x3e0f83e0 0x0f83e0f8 0x0000f83e\n");
+ DRM_DEBUG_KMS("Set 80Bit Custom Phy Test Pattern \
+ 0x3e0f83e0 0x0f83e0f8 0x0000f83e\n");
pattern_val = 0x3e0f83e0;
- intel_de_write(dev_priv, DDI_DP_COMP_PAT(pipe, 0), pattern_val);
+ if (IS_ELKHARTLAKE(dev_priv))
+ intel_de_write(dev_priv, EHL_DDI_DP_COMP_PAT(dig_port->base.port, 0),
+ pattern_val);
+ else
+ intel_de_write(dev_priv, DDI_DP_COMP_PAT(pipe, 0), pattern_val);
pattern_val = 0x0f83e0f8;
- intel_de_write(dev_priv, DDI_DP_COMP_PAT(pipe, 1), pattern_val);
+ if (IS_ELKHARTLAKE(dev_priv))
+ intel_de_write(dev_priv, EHL_DDI_DP_COMP_PAT(dig_port->base.port, 1),
+ pattern_val);
+ else
+ intel_de_write(dev_priv, DDI_DP_COMP_PAT(pipe, 1), pattern_val);
pattern_val = 0x0000f83e;
- intel_de_write(dev_priv, DDI_DP_COMP_PAT(pipe, 2), pattern_val);
- intel_de_write(dev_priv, DDI_DP_COMP_CTL(pipe),
+ if (IS_ELKHARTLAKE(dev_priv))
+ intel_de_write(dev_priv, EHL_DDI_DP_COMP_PAT(dig_port->base.port, 2),
+ pattern_val);
+ else
+ intel_de_write(dev_priv, DDI_DP_COMP_PAT(pipe, 2), pattern_val);
+ intel_de_write(dev_priv, dp_comp_reg,
DDI_DP_COMP_CTL_ENABLE |
DDI_DP_COMP_CTL_CUSTOM80);
break;
@@ -5451,7 +5471,7 @@ static void intel_dp_phy_pattern_update(struct intel_dp *intel_dp)
*/
DRM_DEBUG_KMS("Set HBR2 compliance Phy Test Pattern\n");
pattern_val = 0xFB;
- intel_de_write(dev_priv, DDI_DP_COMP_CTL(pipe),
+ intel_de_write(dev_priv, dp_comp_reg,
DDI_DP_COMP_CTL_ENABLE | DDI_DP_COMP_CTL_HBR2 |
pattern_val);
break;
@@ -5478,22 +5498,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(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;
+ i915_reg_t dp_tp_reg;
+
+ if (IS_ELKHARTLAKE(dev_priv)) {
+ dp_tp_reg = DP_TP_CTL(dig_port->base.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
@@ -5505,20 +5535,29 @@ intel_dp_autotest_phy_ddi_enable(struct intel_dp *intel_dp, uint8_t lane_cnt)
enum port port = dig_port->base.port;
struct intel_crtc *crtc = to_intel_crtc(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);
}
@@ -5565,6 +5604,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;
@@ -5590,6 +5630,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(&i915->drm,
+ "PHY compliance for platform not supported\n");
+ return;
+ }
drm_dbg_kms(&i915->drm, "PHY_PATTERN test requested\n");
response = intel_dp_autotest_phy_pattern(intel_dp);
break;
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 4850890918dc..7d3b6779661f 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -10026,10 +10026,16 @@ enum skl_power_gate {
#define DDI_BUF_BALANCE_LEG_ENABLE (1 << 31)
#define DDI_BUF_TRANS_HI(port, i) _MMIO(_PORT(port, _DDI_BUF_TRANS_A, _DDI_BUF_TRANS_B) + (i) * 8 + 4)
+/* EHL/JSL DP compliance control */
+#define _EHL_DDI_DP_COMP_CTL_A 0x640F0
+#define _EHL_DDI_DP_COMP_CTL_B 0x641F0
+#define EHL_DDI_DP_COMP_CTL(port) \
+ _MMIO_PORT(port, _EHL_DDI_DP_COMP_CTL_A, _EHL_DDI_DP_COMP_CTL_B)
+
/* DDI DP Compliance Control */
-#define _DDI_DP_COMP_CTL_A 0x605F0
-#define _DDI_DP_COMP_CTL_B 0x615F0
-#define DDI_DP_COMP_CTL(pipe) _MMIO_PIPE(pipe, _DDI_DP_COMP_CTL_A, _DDI_DP_COMP_CTL_B)
+#define _DDI_DP_COMP_CTL_A 0x605F0
+#define _DDI_DP_COMP_CTL_B 0x615F0
+#define DDI_DP_COMP_CTL(pipe) _MMIO_PIPE(pipe, _DDI_DP_COMP_CTL_A, _DDI_DP_COMP_CTL_B)
#define DDI_DP_COMP_CTL_ENABLE (1 << 31)
#define DDI_DP_COMP_CTL_D10_2 (0 << 28)
#define DDI_DP_COMP_CTL_SCRAMBLED_0 (1 << 28)
@@ -10039,6 +10045,12 @@ enum skl_power_gate {
#define DDI_DP_COMP_CTL_SCRAMBLED_1 (5 << 28)
#define DDI_DP_COMP_CTL_HBR2_RESET (0xFC << 0)
+/* EHL */
+#define _EHL_DDI_DP_COMP_PAT_A 0x640F4
+#define _EHL_DDI_DP_COMP_PAT_B 0x641F4
+#define EHL_DDI_DP_COMP_PAT(port, i) \
+ _MMIO(_PORT(port, _EHL_DDI_DP_COMP_PAT_A, _EHL_DDI_DP_COMP_PAT_B) + (i) * 4)
+
/* DDI DP Compliance Pattern */
#define _DDI_DP_COMP_PAT_A 0x605F4
#define _DDI_DP_COMP_PAT_B 0x615F4
--
2.7.4
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 2/3] drm/i915/dp: TPS4 PHY test pattern compliance support
2020-09-04 4:13 ` [PATCH 1/3] drm/dp: Add PHY_TEST_PATTERN CP2520 Pattern 2 and 3 Vidya Srinivas
2020-09-04 4:13 ` [PATCH] drm/i915/dp: DP PHY compliance for EHL/JSL Vidya Srinivas
@ 2020-09-04 4:13 ` Vidya Srinivas
2020-09-04 4:13 ` [PATCH 3/3] [RFC] drm/i915/dp: DP PHY compliance for EHL/JSL Vidya Srinivas
2 siblings, 0 replies; 23+ messages in thread
From: Vidya Srinivas @ 2020-09-04 4:13 UTC (permalink / raw)
To: intel-gfx, dri-devel; +Cc: Khaled Almahallawy
From: Khaled Almahallawy <khaled.almahallawy@intel.com>
Adding support for TPS4 (CP2520 Pattern 3) PHY pattern source tests.
v2: uniform bit names TP4a/b/c (Manasi)
Signed-off-by: Khaled Almahallawy <khaled.almahallawy@intel.com>
Reviewed-by: Manasi Navare <manasi.d.navare@intel.com>
Tested-by: Khaled Almahallawy <khaled.almahallawy@intel.com>
---
drivers/gpu/drm/i915/display/intel_dp.c | 14 ++++++++++++--
drivers/gpu/drm/i915/i915_reg.h | 4 ++++
2 files changed, 16 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index 04231ca5643b..a8a3ffcef5dc 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -5403,7 +5403,7 @@ static void intel_dp_phy_pattern_update(struct intel_dp *intel_dp)
&intel_dp->compliance.test_data.phytest;
struct intel_crtc *crtc = to_intel_crtc(dig_port->base.base.crtc);
enum pipe pipe = crtc->pipe;
- u32 pattern_val;
+ u32 pattern_val, dp_tp_ctl;
switch (data->phy_pattern) {
case DP_PHY_TEST_PATTERN_NONE:
@@ -5443,7 +5443,7 @@ static void intel_dp_phy_pattern_update(struct intel_dp *intel_dp)
DDI_DP_COMP_CTL_ENABLE |
DDI_DP_COMP_CTL_CUSTOM80);
break;
- case DP_PHY_TEST_PATTERN_CP2520:
+ case DP_PHY_TEST_PATTERN_CP2520_PAT1:
/*
* FIXME: Ideally pattern should come from DPCD 0x24A. As
* current firmware of DPR-100 could not set it, so hardcoding
@@ -5455,6 +5455,16 @@ static void intel_dp_phy_pattern_update(struct intel_dp *intel_dp)
DDI_DP_COMP_CTL_ENABLE | DDI_DP_COMP_CTL_HBR2 |
pattern_val);
break;
+ case DP_PHY_TEST_PATTERN_CP2520_PAT3:
+ DRM_DEBUG_KMS("Set TPS4 Phy Test Pattern\n");
+ intel_de_write(dev_priv, DDI_DP_COMP_CTL(pipe), 0x0);
+ dp_tp_ctl = intel_de_read(dev_priv, TGL_DP_TP_CTL(pipe));
+ dp_tp_ctl &= ~DP_TP_CTL_TRAIN_PAT4_SEL_MASK;
+ dp_tp_ctl |= DP_TP_CTL_TRAIN_PAT4_SEL_TP4a;
+ dp_tp_ctl &= ~DP_TP_CTL_LINK_TRAIN_MASK;
+ dp_tp_ctl |= DP_TP_CTL_LINK_TRAIN_PAT4;
+ intel_de_write(dev_priv, TGL_DP_TP_CTL(pipe), dp_tp_ctl);
+ break;
default:
WARN(1, "Invalid Phy Test Pattern\n");
}
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index ab4b1abd4364..4850890918dc 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -9974,6 +9974,10 @@ enum skl_power_gate {
#define DP_TP_CTL_MODE_SST (0 << 27)
#define DP_TP_CTL_MODE_MST (1 << 27)
#define DP_TP_CTL_FORCE_ACT (1 << 25)
+#define DP_TP_CTL_TRAIN_PAT4_SEL_MASK (3 << 19)
+#define DP_TP_CTL_TRAIN_PAT4_SEL_TP4a (0 << 19)
+#define DP_TP_CTL_TRAIN_PAT4_SEL_TP4b (1 << 19)
+#define DP_TP_CTL_TRAIN_PAT4_SEL_TP4c (2 << 19)
#define DP_TP_CTL_ENHANCED_FRAME_ENABLE (1 << 18)
#define DP_TP_CTL_FDI_AUTOTRAIN (1 << 15)
#define DP_TP_CTL_LINK_TRAIN_MASK (7 << 8)
--
2.7.4
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 3/3] [RFC] drm/i915/dp: DP PHY compliance for EHL/JSL
2020-09-04 4:13 ` [PATCH 1/3] drm/dp: Add PHY_TEST_PATTERN CP2520 Pattern 2 and 3 Vidya Srinivas
2020-09-04 4:13 ` [PATCH] drm/i915/dp: DP PHY compliance for EHL/JSL Vidya Srinivas
2020-09-04 4:13 ` [PATCH 2/3] drm/i915/dp: TPS4 PHY test pattern compliance support Vidya Srinivas
@ 2020-09-04 4:13 ` Vidya Srinivas
2 siblings, 0 replies; 23+ messages in thread
From: Vidya Srinivas @ 2020-09-04 4:13 UTC (permalink / raw)
To: intel-gfx, dri-devel; +Cc: Vidya Srinivas, Khaled Almahallawy
Please Note: Comment from Ville could not be addressed
as his comments are with respect to base implementation
(design) which are already merged. We need JSL changes
for compliance. Hence pushing the required changes
on top of existing design. Apoligies for that.
v2: Rebased patch on top of Khaled's (yet to be merged):
https://patchwork.freedesktop.org/series/79779/
Fixed phy patterns for JSL/EHL
Add TPS4 support for JSL/EHL
Signed-off-by: Khaled Almahallawy <khaled.almahallawy@intel.com>
Signed-off-by: Vidya Srinivas <vidya.srinivas@intel.com>
---
drivers/gpu/drm/i915/display/intel_dp.c | 81 +++++++++++++++++++++++++--------
drivers/gpu/drm/i915/i915_reg.h | 18 ++++++--
2 files changed, 78 insertions(+), 21 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index a8a3ffcef5dc..1773f3d5d0f4 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -5405,25 +5405,32 @@ static void intel_dp_phy_pattern_update(struct intel_dp *intel_dp)
enum pipe pipe = crtc->pipe;
u32 pattern_val, dp_tp_ctl;
+ i915_reg_t dp_comp_reg;
+
+ if (IS_ELKHARTLAKE(dev_priv))
+ dp_comp_reg = EHL_DDI_DP_COMP_CTL(dig_port->base.port);
+ else if (IS_TIGERLAKE(dev_priv))
+ dp_comp_reg = DDI_DP_COMP_CTL(pipe);
+
switch (data->phy_pattern) {
case DP_PHY_TEST_PATTERN_NONE:
DRM_DEBUG_KMS("Disable Phy Test Pattern\n");
- intel_de_write(dev_priv, DDI_DP_COMP_CTL(pipe), 0x0);
+ intel_de_write(dev_priv, dp_comp_reg, 0x0);
break;
case DP_PHY_TEST_PATTERN_D10_2:
DRM_DEBUG_KMS("Set D10.2 Phy Test Pattern\n");
- intel_de_write(dev_priv, DDI_DP_COMP_CTL(pipe),
+ intel_de_write(dev_priv, dp_comp_reg,
DDI_DP_COMP_CTL_ENABLE | DDI_DP_COMP_CTL_D10_2);
break;
case DP_PHY_TEST_PATTERN_ERROR_COUNT:
DRM_DEBUG_KMS("Set Error Count Phy Test Pattern\n");
- intel_de_write(dev_priv, DDI_DP_COMP_CTL(pipe),
+ intel_de_write(dev_priv, dp_comp_reg,
DDI_DP_COMP_CTL_ENABLE |
DDI_DP_COMP_CTL_SCRAMBLED_0);
break;
case DP_PHY_TEST_PATTERN_PRBS7:
DRM_DEBUG_KMS("Set PRBS7 Phy Test Pattern\n");
- intel_de_write(dev_priv, DDI_DP_COMP_CTL(pipe),
+ intel_de_write(dev_priv, dp_comp_reg,
DDI_DP_COMP_CTL_ENABLE | DDI_DP_COMP_CTL_PRBS7);
break;
case DP_PHY_TEST_PATTERN_80BIT_CUSTOM:
@@ -5432,14 +5439,27 @@ static void intel_dp_phy_pattern_update(struct intel_dp *intel_dp)
* current firmware of DPR-100 could not set it, so hardcoding
* now for complaince test.
*/
- DRM_DEBUG_KMS("Set 80Bit Custom Phy Test Pattern 0x3e0f83e0 0x0f83e0f8 0x0000f83e\n");
+ DRM_DEBUG_KMS("Set 80Bit Custom Phy Test Pattern \
+ 0x3e0f83e0 0x0f83e0f8 0x0000f83e\n");
pattern_val = 0x3e0f83e0;
- intel_de_write(dev_priv, DDI_DP_COMP_PAT(pipe, 0), pattern_val);
+ if (IS_ELKHARTLAKE(dev_priv))
+ intel_de_write(dev_priv, EHL_DDI_DP_COMP_PAT(dig_port->base.port, 0),
+ pattern_val);
+ else
+ intel_de_write(dev_priv, DDI_DP_COMP_PAT(pipe, 0), pattern_val);
pattern_val = 0x0f83e0f8;
- intel_de_write(dev_priv, DDI_DP_COMP_PAT(pipe, 1), pattern_val);
+ if (IS_ELKHARTLAKE(dev_priv))
+ intel_de_write(dev_priv, EHL_DDI_DP_COMP_PAT(dig_port->base.port, 1),
+ pattern_val);
+ else
+ intel_de_write(dev_priv, DDI_DP_COMP_PAT(pipe, 1), pattern_val);
pattern_val = 0x0000f83e;
- intel_de_write(dev_priv, DDI_DP_COMP_PAT(pipe, 2), pattern_val);
- intel_de_write(dev_priv, DDI_DP_COMP_CTL(pipe),
+ if (IS_ELKHARTLAKE(dev_priv))
+ intel_de_write(dev_priv, EHL_DDI_DP_COMP_PAT(dig_port->base.port, 2),
+ pattern_val);
+ else
+ intel_de_write(dev_priv, DDI_DP_COMP_PAT(pipe, 2), pattern_val);
+ intel_de_write(dev_priv, dp_comp_reg,
DDI_DP_COMP_CTL_ENABLE |
DDI_DP_COMP_CTL_CUSTOM80);
break;
@@ -5451,7 +5471,7 @@ static void intel_dp_phy_pattern_update(struct intel_dp *intel_dp)
*/
DRM_DEBUG_KMS("Set HBR2 compliance Phy Test Pattern\n");
pattern_val = 0xFB;
- intel_de_write(dev_priv, DDI_DP_COMP_CTL(pipe),
+ intel_de_write(dev_priv, dp_comp_reg,
DDI_DP_COMP_CTL_ENABLE | DDI_DP_COMP_CTL_HBR2 |
pattern_val);
break;
@@ -5478,22 +5498,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(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;
+ i915_reg_t dp_tp_reg;
+
+ if (IS_ELKHARTLAKE(dev_priv)) {
+ dp_tp_reg = DP_TP_CTL(dig_port->base.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
@@ -5505,20 +5535,29 @@ intel_dp_autotest_phy_ddi_enable(struct intel_dp *intel_dp, uint8_t lane_cnt)
enum port port = dig_port->base.port;
struct intel_crtc *crtc = to_intel_crtc(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);
}
@@ -5565,6 +5604,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;
@@ -5590,6 +5630,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(&i915->drm,
+ "PHY compliance for platform not supported\n");
+ return;
+ }
drm_dbg_kms(&i915->drm, "PHY_PATTERN test requested\n");
response = intel_dp_autotest_phy_pattern(intel_dp);
break;
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 4850890918dc..7d3b6779661f 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -10026,10 +10026,16 @@ enum skl_power_gate {
#define DDI_BUF_BALANCE_LEG_ENABLE (1 << 31)
#define DDI_BUF_TRANS_HI(port, i) _MMIO(_PORT(port, _DDI_BUF_TRANS_A, _DDI_BUF_TRANS_B) + (i) * 8 + 4)
+/* EHL/JSL DP compliance control */
+#define _EHL_DDI_DP_COMP_CTL_A 0x640F0
+#define _EHL_DDI_DP_COMP_CTL_B 0x641F0
+#define EHL_DDI_DP_COMP_CTL(port) \
+ _MMIO_PORT(port, _EHL_DDI_DP_COMP_CTL_A, _EHL_DDI_DP_COMP_CTL_B)
+
/* DDI DP Compliance Control */
-#define _DDI_DP_COMP_CTL_A 0x605F0
-#define _DDI_DP_COMP_CTL_B 0x615F0
-#define DDI_DP_COMP_CTL(pipe) _MMIO_PIPE(pipe, _DDI_DP_COMP_CTL_A, _DDI_DP_COMP_CTL_B)
+#define _DDI_DP_COMP_CTL_A 0x605F0
+#define _DDI_DP_COMP_CTL_B 0x615F0
+#define DDI_DP_COMP_CTL(pipe) _MMIO_PIPE(pipe, _DDI_DP_COMP_CTL_A, _DDI_DP_COMP_CTL_B)
#define DDI_DP_COMP_CTL_ENABLE (1 << 31)
#define DDI_DP_COMP_CTL_D10_2 (0 << 28)
#define DDI_DP_COMP_CTL_SCRAMBLED_0 (1 << 28)
@@ -10039,6 +10045,12 @@ enum skl_power_gate {
#define DDI_DP_COMP_CTL_SCRAMBLED_1 (5 << 28)
#define DDI_DP_COMP_CTL_HBR2_RESET (0xFC << 0)
+/* EHL */
+#define _EHL_DDI_DP_COMP_PAT_A 0x640F4
+#define _EHL_DDI_DP_COMP_PAT_B 0x641F4
+#define EHL_DDI_DP_COMP_PAT(port, i) \
+ _MMIO(_PORT(port, _EHL_DDI_DP_COMP_PAT_A, _EHL_DDI_DP_COMP_PAT_B) + (i) * 4)
+
/* DDI DP Compliance Pattern */
#define _DDI_DP_COMP_PAT_A 0x605F4
#define _DDI_DP_COMP_PAT_B 0x615F4
--
2.7.4
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 1/3] drm/dp: Add PHY_TEST_PATTERN CP2520 Pattern 2 and 3
2020-06-04 5:03 [PATCH] drm/i915/dp: DP PHY compliance for JSL Vidya Srinivas
2020-06-04 19:06 ` [Intel-gfx] " Ville Syrjälä
2020-09-04 4:13 ` [PATCH 1/3] drm/dp: Add PHY_TEST_PATTERN CP2520 Pattern 2 and 3 Vidya Srinivas
@ 2020-09-04 4:14 ` Vidya Srinivas
2020-09-04 4:14 ` [PATCH 2/3] drm/i915/dp: TPS4 PHY test pattern compliance support Vidya Srinivas
2020-09-04 4:14 ` [PATCH 3/3] [RFC] drm/i915/dp: DP PHY compliance for EHL/JSL Vidya Srinivas
2020-09-04 4:29 ` [PATCH] " Vidya Srinivas
3 siblings, 2 replies; 23+ messages in thread
From: Vidya Srinivas @ 2020-09-04 4:14 UTC (permalink / raw)
To: intel-gfx, dri-devel; +Cc: Khaled Almahallawy
From: Khaled Almahallawy <khaled.almahallawy@intel.com>
Add the missing CP2520 pattern 2 and 3 phy compliance patterns
v2: cosemtic changes
Reviewed-by: Manasi Navare <manasi.d.navare@intel.com> (v1)
Signed-off-by: Khaled Almahallawy <khaled.almahallawy@intel.com>
---
drivers/gpu/drm/drm_dp_helper.c | 2 +-
include/drm/drm_dp_helper.h | 4 +++-
2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index a3c82e726057..d0fb78c6aca6 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -1583,7 +1583,7 @@ int drm_dp_get_phy_test_pattern(struct drm_dp_aux *aux,
return err;
break;
- case DP_PHY_TEST_PATTERN_CP2520:
+ case DP_PHY_TEST_PATTERN_CP2520_PAT1:
err = drm_dp_dpcd_read(aux, DP_TEST_HBR2_SCRAMBLER_RESET,
&data->hbr2_reset,
sizeof(data->hbr2_reset));
diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index e2d2df5e869e..73285b4c25a0 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -708,7 +708,9 @@
# define DP_PHY_TEST_PATTERN_ERROR_COUNT 0x2
# define DP_PHY_TEST_PATTERN_PRBS7 0x3
# define DP_PHY_TEST_PATTERN_80BIT_CUSTOM 0x4
-# define DP_PHY_TEST_PATTERN_CP2520 0x5
+# define DP_PHY_TEST_PATTERN_CP2520_PAT1 0x5
+# define DP_PHY_TEST_PATTERN_CP2520_PAT2 0x6
+# define DP_PHY_TEST_PATTERN_CP2520_PAT3 0x7
#define DP_TEST_HBR2_SCRAMBLER_RESET 0x24A
#define DP_TEST_80BIT_CUSTOM_PATTERN_7_0 0x250
--
2.7.4
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 2/3] drm/i915/dp: TPS4 PHY test pattern compliance support
2020-09-04 4:14 ` [PATCH 1/3] drm/dp: Add PHY_TEST_PATTERN CP2520 Pattern 2 and 3 Vidya Srinivas
@ 2020-09-04 4:14 ` Vidya Srinivas
2020-09-04 4:14 ` [PATCH 3/3] [RFC] drm/i915/dp: DP PHY compliance for EHL/JSL Vidya Srinivas
1 sibling, 0 replies; 23+ messages in thread
From: Vidya Srinivas @ 2020-09-04 4:14 UTC (permalink / raw)
To: intel-gfx, dri-devel; +Cc: Khaled Almahallawy
From: Khaled Almahallawy <khaled.almahallawy@intel.com>
Adding support for TPS4 (CP2520 Pattern 3) PHY pattern source tests.
v2: uniform bit names TP4a/b/c (Manasi)
Signed-off-by: Khaled Almahallawy <khaled.almahallawy@intel.com>
Reviewed-by: Manasi Navare <manasi.d.navare@intel.com>
Tested-by: Khaled Almahallawy <khaled.almahallawy@intel.com>
---
drivers/gpu/drm/i915/display/intel_dp.c | 14 ++++++++++++--
drivers/gpu/drm/i915/i915_reg.h | 4 ++++
2 files changed, 16 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index 04231ca5643b..a8a3ffcef5dc 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -5403,7 +5403,7 @@ static void intel_dp_phy_pattern_update(struct intel_dp *intel_dp)
&intel_dp->compliance.test_data.phytest;
struct intel_crtc *crtc = to_intel_crtc(dig_port->base.base.crtc);
enum pipe pipe = crtc->pipe;
- u32 pattern_val;
+ u32 pattern_val, dp_tp_ctl;
switch (data->phy_pattern) {
case DP_PHY_TEST_PATTERN_NONE:
@@ -5443,7 +5443,7 @@ static void intel_dp_phy_pattern_update(struct intel_dp *intel_dp)
DDI_DP_COMP_CTL_ENABLE |
DDI_DP_COMP_CTL_CUSTOM80);
break;
- case DP_PHY_TEST_PATTERN_CP2520:
+ case DP_PHY_TEST_PATTERN_CP2520_PAT1:
/*
* FIXME: Ideally pattern should come from DPCD 0x24A. As
* current firmware of DPR-100 could not set it, so hardcoding
@@ -5455,6 +5455,16 @@ static void intel_dp_phy_pattern_update(struct intel_dp *intel_dp)
DDI_DP_COMP_CTL_ENABLE | DDI_DP_COMP_CTL_HBR2 |
pattern_val);
break;
+ case DP_PHY_TEST_PATTERN_CP2520_PAT3:
+ DRM_DEBUG_KMS("Set TPS4 Phy Test Pattern\n");
+ intel_de_write(dev_priv, DDI_DP_COMP_CTL(pipe), 0x0);
+ dp_tp_ctl = intel_de_read(dev_priv, TGL_DP_TP_CTL(pipe));
+ dp_tp_ctl &= ~DP_TP_CTL_TRAIN_PAT4_SEL_MASK;
+ dp_tp_ctl |= DP_TP_CTL_TRAIN_PAT4_SEL_TP4a;
+ dp_tp_ctl &= ~DP_TP_CTL_LINK_TRAIN_MASK;
+ dp_tp_ctl |= DP_TP_CTL_LINK_TRAIN_PAT4;
+ intel_de_write(dev_priv, TGL_DP_TP_CTL(pipe), dp_tp_ctl);
+ break;
default:
WARN(1, "Invalid Phy Test Pattern\n");
}
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index ab4b1abd4364..4850890918dc 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -9974,6 +9974,10 @@ enum skl_power_gate {
#define DP_TP_CTL_MODE_SST (0 << 27)
#define DP_TP_CTL_MODE_MST (1 << 27)
#define DP_TP_CTL_FORCE_ACT (1 << 25)
+#define DP_TP_CTL_TRAIN_PAT4_SEL_MASK (3 << 19)
+#define DP_TP_CTL_TRAIN_PAT4_SEL_TP4a (0 << 19)
+#define DP_TP_CTL_TRAIN_PAT4_SEL_TP4b (1 << 19)
+#define DP_TP_CTL_TRAIN_PAT4_SEL_TP4c (2 << 19)
#define DP_TP_CTL_ENHANCED_FRAME_ENABLE (1 << 18)
#define DP_TP_CTL_FDI_AUTOTRAIN (1 << 15)
#define DP_TP_CTL_LINK_TRAIN_MASK (7 << 8)
--
2.7.4
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 3/3] [RFC] drm/i915/dp: DP PHY compliance for EHL/JSL
2020-09-04 4:14 ` [PATCH 1/3] drm/dp: Add PHY_TEST_PATTERN CP2520 Pattern 2 and 3 Vidya Srinivas
2020-09-04 4:14 ` [PATCH 2/3] drm/i915/dp: TPS4 PHY test pattern compliance support Vidya Srinivas
@ 2020-09-04 4:14 ` Vidya Srinivas
1 sibling, 0 replies; 23+ messages in thread
From: Vidya Srinivas @ 2020-09-04 4:14 UTC (permalink / raw)
To: intel-gfx, dri-devel; +Cc: Vidya Srinivas, Khaled Almahallawy
Please Note: Comment from Ville could not be addressed
as his comments are with respect to base implementation
(design) which are already merged. We need JSL changes
for compliance. Hence pushing the required changes
on top of existing design. Apoligies for that.
v2: Rebased patch on top of Khaled's (yet to be merged):
https://patchwork.freedesktop.org/series/79779/
Fixed phy patterns for JSL/EHL
Add TPS4 support for JSL/EHL
Signed-off-by: Khaled Almahallawy <khaled.almahallawy@intel.com>
Signed-off-by: Vidya Srinivas <vidya.srinivas@intel.com>
---
drivers/gpu/drm/i915/display/intel_dp.c | 81 +++++++++++++++++++++++++--------
drivers/gpu/drm/i915/i915_reg.h | 18 ++++++--
2 files changed, 78 insertions(+), 21 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index a8a3ffcef5dc..1773f3d5d0f4 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -5405,25 +5405,32 @@ static void intel_dp_phy_pattern_update(struct intel_dp *intel_dp)
enum pipe pipe = crtc->pipe;
u32 pattern_val, dp_tp_ctl;
+ i915_reg_t dp_comp_reg;
+
+ if (IS_ELKHARTLAKE(dev_priv))
+ dp_comp_reg = EHL_DDI_DP_COMP_CTL(dig_port->base.port);
+ else if (IS_TIGERLAKE(dev_priv))
+ dp_comp_reg = DDI_DP_COMP_CTL(pipe);
+
switch (data->phy_pattern) {
case DP_PHY_TEST_PATTERN_NONE:
DRM_DEBUG_KMS("Disable Phy Test Pattern\n");
- intel_de_write(dev_priv, DDI_DP_COMP_CTL(pipe), 0x0);
+ intel_de_write(dev_priv, dp_comp_reg, 0x0);
break;
case DP_PHY_TEST_PATTERN_D10_2:
DRM_DEBUG_KMS("Set D10.2 Phy Test Pattern\n");
- intel_de_write(dev_priv, DDI_DP_COMP_CTL(pipe),
+ intel_de_write(dev_priv, dp_comp_reg,
DDI_DP_COMP_CTL_ENABLE | DDI_DP_COMP_CTL_D10_2);
break;
case DP_PHY_TEST_PATTERN_ERROR_COUNT:
DRM_DEBUG_KMS("Set Error Count Phy Test Pattern\n");
- intel_de_write(dev_priv, DDI_DP_COMP_CTL(pipe),
+ intel_de_write(dev_priv, dp_comp_reg,
DDI_DP_COMP_CTL_ENABLE |
DDI_DP_COMP_CTL_SCRAMBLED_0);
break;
case DP_PHY_TEST_PATTERN_PRBS7:
DRM_DEBUG_KMS("Set PRBS7 Phy Test Pattern\n");
- intel_de_write(dev_priv, DDI_DP_COMP_CTL(pipe),
+ intel_de_write(dev_priv, dp_comp_reg,
DDI_DP_COMP_CTL_ENABLE | DDI_DP_COMP_CTL_PRBS7);
break;
case DP_PHY_TEST_PATTERN_80BIT_CUSTOM:
@@ -5432,14 +5439,27 @@ static void intel_dp_phy_pattern_update(struct intel_dp *intel_dp)
* current firmware of DPR-100 could not set it, so hardcoding
* now for complaince test.
*/
- DRM_DEBUG_KMS("Set 80Bit Custom Phy Test Pattern 0x3e0f83e0 0x0f83e0f8 0x0000f83e\n");
+ DRM_DEBUG_KMS("Set 80Bit Custom Phy Test Pattern \
+ 0x3e0f83e0 0x0f83e0f8 0x0000f83e\n");
pattern_val = 0x3e0f83e0;
- intel_de_write(dev_priv, DDI_DP_COMP_PAT(pipe, 0), pattern_val);
+ if (IS_ELKHARTLAKE(dev_priv))
+ intel_de_write(dev_priv, EHL_DDI_DP_COMP_PAT(dig_port->base.port, 0),
+ pattern_val);
+ else
+ intel_de_write(dev_priv, DDI_DP_COMP_PAT(pipe, 0), pattern_val);
pattern_val = 0x0f83e0f8;
- intel_de_write(dev_priv, DDI_DP_COMP_PAT(pipe, 1), pattern_val);
+ if (IS_ELKHARTLAKE(dev_priv))
+ intel_de_write(dev_priv, EHL_DDI_DP_COMP_PAT(dig_port->base.port, 1),
+ pattern_val);
+ else
+ intel_de_write(dev_priv, DDI_DP_COMP_PAT(pipe, 1), pattern_val);
pattern_val = 0x0000f83e;
- intel_de_write(dev_priv, DDI_DP_COMP_PAT(pipe, 2), pattern_val);
- intel_de_write(dev_priv, DDI_DP_COMP_CTL(pipe),
+ if (IS_ELKHARTLAKE(dev_priv))
+ intel_de_write(dev_priv, EHL_DDI_DP_COMP_PAT(dig_port->base.port, 2),
+ pattern_val);
+ else
+ intel_de_write(dev_priv, DDI_DP_COMP_PAT(pipe, 2), pattern_val);
+ intel_de_write(dev_priv, dp_comp_reg,
DDI_DP_COMP_CTL_ENABLE |
DDI_DP_COMP_CTL_CUSTOM80);
break;
@@ -5451,7 +5471,7 @@ static void intel_dp_phy_pattern_update(struct intel_dp *intel_dp)
*/
DRM_DEBUG_KMS("Set HBR2 compliance Phy Test Pattern\n");
pattern_val = 0xFB;
- intel_de_write(dev_priv, DDI_DP_COMP_CTL(pipe),
+ intel_de_write(dev_priv, dp_comp_reg,
DDI_DP_COMP_CTL_ENABLE | DDI_DP_COMP_CTL_HBR2 |
pattern_val);
break;
@@ -5478,22 +5498,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(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;
+ i915_reg_t dp_tp_reg;
+
+ if (IS_ELKHARTLAKE(dev_priv)) {
+ dp_tp_reg = DP_TP_CTL(dig_port->base.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
@@ -5505,20 +5535,29 @@ intel_dp_autotest_phy_ddi_enable(struct intel_dp *intel_dp, uint8_t lane_cnt)
enum port port = dig_port->base.port;
struct intel_crtc *crtc = to_intel_crtc(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);
}
@@ -5565,6 +5604,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;
@@ -5590,6 +5630,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(&i915->drm,
+ "PHY compliance for platform not supported\n");
+ return;
+ }
drm_dbg_kms(&i915->drm, "PHY_PATTERN test requested\n");
response = intel_dp_autotest_phy_pattern(intel_dp);
break;
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 4850890918dc..7d3b6779661f 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -10026,10 +10026,16 @@ enum skl_power_gate {
#define DDI_BUF_BALANCE_LEG_ENABLE (1 << 31)
#define DDI_BUF_TRANS_HI(port, i) _MMIO(_PORT(port, _DDI_BUF_TRANS_A, _DDI_BUF_TRANS_B) + (i) * 8 + 4)
+/* EHL/JSL DP compliance control */
+#define _EHL_DDI_DP_COMP_CTL_A 0x640F0
+#define _EHL_DDI_DP_COMP_CTL_B 0x641F0
+#define EHL_DDI_DP_COMP_CTL(port) \
+ _MMIO_PORT(port, _EHL_DDI_DP_COMP_CTL_A, _EHL_DDI_DP_COMP_CTL_B)
+
/* DDI DP Compliance Control */
-#define _DDI_DP_COMP_CTL_A 0x605F0
-#define _DDI_DP_COMP_CTL_B 0x615F0
-#define DDI_DP_COMP_CTL(pipe) _MMIO_PIPE(pipe, _DDI_DP_COMP_CTL_A, _DDI_DP_COMP_CTL_B)
+#define _DDI_DP_COMP_CTL_A 0x605F0
+#define _DDI_DP_COMP_CTL_B 0x615F0
+#define DDI_DP_COMP_CTL(pipe) _MMIO_PIPE(pipe, _DDI_DP_COMP_CTL_A, _DDI_DP_COMP_CTL_B)
#define DDI_DP_COMP_CTL_ENABLE (1 << 31)
#define DDI_DP_COMP_CTL_D10_2 (0 << 28)
#define DDI_DP_COMP_CTL_SCRAMBLED_0 (1 << 28)
@@ -10039,6 +10045,12 @@ enum skl_power_gate {
#define DDI_DP_COMP_CTL_SCRAMBLED_1 (5 << 28)
#define DDI_DP_COMP_CTL_HBR2_RESET (0xFC << 0)
+/* EHL */
+#define _EHL_DDI_DP_COMP_PAT_A 0x640F4
+#define _EHL_DDI_DP_COMP_PAT_B 0x641F4
+#define EHL_DDI_DP_COMP_PAT(port, i) \
+ _MMIO(_PORT(port, _EHL_DDI_DP_COMP_PAT_A, _EHL_DDI_DP_COMP_PAT_B) + (i) * 4)
+
/* DDI DP Compliance Pattern */
#define _DDI_DP_COMP_PAT_A 0x605F4
#define _DDI_DP_COMP_PAT_B 0x615F4
--
2.7.4
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH] [RFC] drm/i915/dp: DP PHY compliance for EHL/JSL
2020-06-04 5:03 [PATCH] drm/i915/dp: DP PHY compliance for JSL Vidya Srinivas
` (2 preceding siblings ...)
2020-09-04 4:14 ` [PATCH 1/3] drm/dp: Add PHY_TEST_PATTERN CP2520 Pattern 2 and 3 Vidya Srinivas
@ 2020-09-04 4:29 ` Vidya Srinivas
2020-09-10 4:00 ` Vidya Srinivas
3 siblings, 1 reply; 23+ messages in thread
From: Vidya Srinivas @ 2020-09-04 4:29 UTC (permalink / raw)
To: intel-gfx, dri-devel; +Cc: Vidya Srinivas, Khaled Almahallawy
Please Note: Comment from Ville could not be addressed
as his comments are with respect to base implementation
(design) which are already merged. We need JSL changes
for compliance. Hence pushing the required changes
on top of existing design. Apoligies for that.
v2: Rebased patch on top of Khaled's (yet to be merged):
https://patchwork.freedesktop.org/series/79779/
Fixed phy patterns for JSL/EHL
Add TPS4 support for JSL/EHL
Signed-off-by: Khaled Almahallawy <khaled.almahallawy@intel.com>
Signed-off-by: Vidya Srinivas <vidya.srinivas@intel.com>
---
drivers/gpu/drm/i915/display/intel_dp.c | 81 +++++++++++++++++++++++++--------
drivers/gpu/drm/i915/i915_reg.h | 18 ++++++--
2 files changed, 78 insertions(+), 21 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index a8a3ffcef5dc..1773f3d5d0f4 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -5405,25 +5405,32 @@ static void intel_dp_phy_pattern_update(struct intel_dp *intel_dp)
enum pipe pipe = crtc->pipe;
u32 pattern_val, dp_tp_ctl;
+ i915_reg_t dp_comp_reg;
+
+ if (IS_ELKHARTLAKE(dev_priv))
+ dp_comp_reg = EHL_DDI_DP_COMP_CTL(dig_port->base.port);
+ else if (IS_TIGERLAKE(dev_priv))
+ dp_comp_reg = DDI_DP_COMP_CTL(pipe);
+
switch (data->phy_pattern) {
case DP_PHY_TEST_PATTERN_NONE:
DRM_DEBUG_KMS("Disable Phy Test Pattern\n");
- intel_de_write(dev_priv, DDI_DP_COMP_CTL(pipe), 0x0);
+ intel_de_write(dev_priv, dp_comp_reg, 0x0);
break;
case DP_PHY_TEST_PATTERN_D10_2:
DRM_DEBUG_KMS("Set D10.2 Phy Test Pattern\n");
- intel_de_write(dev_priv, DDI_DP_COMP_CTL(pipe),
+ intel_de_write(dev_priv, dp_comp_reg,
DDI_DP_COMP_CTL_ENABLE | DDI_DP_COMP_CTL_D10_2);
break;
case DP_PHY_TEST_PATTERN_ERROR_COUNT:
DRM_DEBUG_KMS("Set Error Count Phy Test Pattern\n");
- intel_de_write(dev_priv, DDI_DP_COMP_CTL(pipe),
+ intel_de_write(dev_priv, dp_comp_reg,
DDI_DP_COMP_CTL_ENABLE |
DDI_DP_COMP_CTL_SCRAMBLED_0);
break;
case DP_PHY_TEST_PATTERN_PRBS7:
DRM_DEBUG_KMS("Set PRBS7 Phy Test Pattern\n");
- intel_de_write(dev_priv, DDI_DP_COMP_CTL(pipe),
+ intel_de_write(dev_priv, dp_comp_reg,
DDI_DP_COMP_CTL_ENABLE | DDI_DP_COMP_CTL_PRBS7);
break;
case DP_PHY_TEST_PATTERN_80BIT_CUSTOM:
@@ -5432,14 +5439,27 @@ static void intel_dp_phy_pattern_update(struct intel_dp *intel_dp)
* current firmware of DPR-100 could not set it, so hardcoding
* now for complaince test.
*/
- DRM_DEBUG_KMS("Set 80Bit Custom Phy Test Pattern 0x3e0f83e0 0x0f83e0f8 0x0000f83e\n");
+ DRM_DEBUG_KMS("Set 80Bit Custom Phy Test Pattern \
+ 0x3e0f83e0 0x0f83e0f8 0x0000f83e\n");
pattern_val = 0x3e0f83e0;
- intel_de_write(dev_priv, DDI_DP_COMP_PAT(pipe, 0), pattern_val);
+ if (IS_ELKHARTLAKE(dev_priv))
+ intel_de_write(dev_priv, EHL_DDI_DP_COMP_PAT(dig_port->base.port, 0),
+ pattern_val);
+ else
+ intel_de_write(dev_priv, DDI_DP_COMP_PAT(pipe, 0), pattern_val);
pattern_val = 0x0f83e0f8;
- intel_de_write(dev_priv, DDI_DP_COMP_PAT(pipe, 1), pattern_val);
+ if (IS_ELKHARTLAKE(dev_priv))
+ intel_de_write(dev_priv, EHL_DDI_DP_COMP_PAT(dig_port->base.port, 1),
+ pattern_val);
+ else
+ intel_de_write(dev_priv, DDI_DP_COMP_PAT(pipe, 1), pattern_val);
pattern_val = 0x0000f83e;
- intel_de_write(dev_priv, DDI_DP_COMP_PAT(pipe, 2), pattern_val);
- intel_de_write(dev_priv, DDI_DP_COMP_CTL(pipe),
+ if (IS_ELKHARTLAKE(dev_priv))
+ intel_de_write(dev_priv, EHL_DDI_DP_COMP_PAT(dig_port->base.port, 2),
+ pattern_val);
+ else
+ intel_de_write(dev_priv, DDI_DP_COMP_PAT(pipe, 2), pattern_val);
+ intel_de_write(dev_priv, dp_comp_reg,
DDI_DP_COMP_CTL_ENABLE |
DDI_DP_COMP_CTL_CUSTOM80);
break;
@@ -5451,7 +5471,7 @@ static void intel_dp_phy_pattern_update(struct intel_dp *intel_dp)
*/
DRM_DEBUG_KMS("Set HBR2 compliance Phy Test Pattern\n");
pattern_val = 0xFB;
- intel_de_write(dev_priv, DDI_DP_COMP_CTL(pipe),
+ intel_de_write(dev_priv, dp_comp_reg,
DDI_DP_COMP_CTL_ENABLE | DDI_DP_COMP_CTL_HBR2 |
pattern_val);
break;
@@ -5478,22 +5498,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(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;
+ i915_reg_t dp_tp_reg;
+
+ if (IS_ELKHARTLAKE(dev_priv)) {
+ dp_tp_reg = DP_TP_CTL(dig_port->base.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
@@ -5505,20 +5535,29 @@ intel_dp_autotest_phy_ddi_enable(struct intel_dp *intel_dp, uint8_t lane_cnt)
enum port port = dig_port->base.port;
struct intel_crtc *crtc = to_intel_crtc(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);
}
@@ -5565,6 +5604,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;
@@ -5590,6 +5630,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(&i915->drm,
+ "PHY compliance for platform not supported\n");
+ return;
+ }
drm_dbg_kms(&i915->drm, "PHY_PATTERN test requested\n");
response = intel_dp_autotest_phy_pattern(intel_dp);
break;
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 4850890918dc..7d3b6779661f 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -10026,10 +10026,16 @@ enum skl_power_gate {
#define DDI_BUF_BALANCE_LEG_ENABLE (1 << 31)
#define DDI_BUF_TRANS_HI(port, i) _MMIO(_PORT(port, _DDI_BUF_TRANS_A, _DDI_BUF_TRANS_B) + (i) * 8 + 4)
+/* EHL/JSL DP compliance control */
+#define _EHL_DDI_DP_COMP_CTL_A 0x640F0
+#define _EHL_DDI_DP_COMP_CTL_B 0x641F0
+#define EHL_DDI_DP_COMP_CTL(port) \
+ _MMIO_PORT(port, _EHL_DDI_DP_COMP_CTL_A, _EHL_DDI_DP_COMP_CTL_B)
+
/* DDI DP Compliance Control */
-#define _DDI_DP_COMP_CTL_A 0x605F0
-#define _DDI_DP_COMP_CTL_B 0x615F0
-#define DDI_DP_COMP_CTL(pipe) _MMIO_PIPE(pipe, _DDI_DP_COMP_CTL_A, _DDI_DP_COMP_CTL_B)
+#define _DDI_DP_COMP_CTL_A 0x605F0
+#define _DDI_DP_COMP_CTL_B 0x615F0
+#define DDI_DP_COMP_CTL(pipe) _MMIO_PIPE(pipe, _DDI_DP_COMP_CTL_A, _DDI_DP_COMP_CTL_B)
#define DDI_DP_COMP_CTL_ENABLE (1 << 31)
#define DDI_DP_COMP_CTL_D10_2 (0 << 28)
#define DDI_DP_COMP_CTL_SCRAMBLED_0 (1 << 28)
@@ -10039,6 +10045,12 @@ enum skl_power_gate {
#define DDI_DP_COMP_CTL_SCRAMBLED_1 (5 << 28)
#define DDI_DP_COMP_CTL_HBR2_RESET (0xFC << 0)
+/* EHL */
+#define _EHL_DDI_DP_COMP_PAT_A 0x640F4
+#define _EHL_DDI_DP_COMP_PAT_B 0x641F4
+#define EHL_DDI_DP_COMP_PAT(port, i) \
+ _MMIO(_PORT(port, _EHL_DDI_DP_COMP_PAT_A, _EHL_DDI_DP_COMP_PAT_B) + (i) * 4)
+
/* DDI DP Compliance Pattern */
#define _DDI_DP_COMP_PAT_A 0x605F4
#define _DDI_DP_COMP_PAT_B 0x615F4
--
2.7.4
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH] [RFC] drm/i915/dp: DP PHY compliance for EHL/JSL
2020-09-04 4:29 ` [PATCH] " Vidya Srinivas
@ 2020-09-10 4:00 ` Vidya Srinivas
0 siblings, 0 replies; 23+ messages in thread
From: Vidya Srinivas @ 2020-09-10 4:00 UTC (permalink / raw)
To: intel-gfx, dri-devel; +Cc: Vidya Srinivas, Khaled Almahallawy
Please Note: Comment from Ville could not be addressed
as his comments are with respect to base implementation
(design) which are already merged. We need JSL changes
for compliance. Hence pushing the required changes
on top of existing design. Apoligies for that.
v2: Rebased patch on top of Khaled's (yet to be merged):
https://patchwork.freedesktop.org/series/79779/
Fixed phy patterns for JSL/EHL
Add TPS4 support for JSL/EHL
Signed-off-by: Khaled Almahallawy <khaled.almahallawy@intel.com>
Signed-off-by: Vidya Srinivas <vidya.srinivas@intel.com>
---
drivers/gpu/drm/i915/display/intel_dp.c | 106 +++++++++++++++++++++++---------
drivers/gpu/drm/i915/i915_reg.h | 18 +++++-
2 files changed, 92 insertions(+), 32 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index a8a3ffcef5dc..0a535932ed18 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -5404,26 +5404,37 @@ static void intel_dp_phy_pattern_update(struct intel_dp *intel_dp)
struct intel_crtc *crtc = to_intel_crtc(dig_port->base.base.crtc);
enum pipe pipe = crtc->pipe;
u32 pattern_val, dp_tp_ctl;
+ i915_reg_t dp_tp_reg, dp_comp_reg;
+
+ if (IS_ELKHARTLAKE(dev_priv))
+ dp_tp_reg = DP_TP_CTL(dig_port->base.port);
+ else if (IS_TIGERLAKE(dev_priv))
+ dp_tp_reg = TGL_DP_TP_CTL(pipe);
+
+ if (IS_ELKHARTLAKE(dev_priv))
+ dp_comp_reg = EHL_DDI_DP_COMP_CTL(dig_port->base.port);
+ else if (IS_TIGERLAKE(dev_priv))
+ dp_comp_reg = DDI_DP_COMP_CTL(pipe);
switch (data->phy_pattern) {
case DP_PHY_TEST_PATTERN_NONE:
DRM_DEBUG_KMS("Disable Phy Test Pattern\n");
- intel_de_write(dev_priv, DDI_DP_COMP_CTL(pipe), 0x0);
+ intel_de_write(dev_priv, dp_comp_reg, 0x0);
break;
case DP_PHY_TEST_PATTERN_D10_2:
DRM_DEBUG_KMS("Set D10.2 Phy Test Pattern\n");
- intel_de_write(dev_priv, DDI_DP_COMP_CTL(pipe),
+ intel_de_write(dev_priv, dp_comp_reg,
DDI_DP_COMP_CTL_ENABLE | DDI_DP_COMP_CTL_D10_2);
break;
case DP_PHY_TEST_PATTERN_ERROR_COUNT:
DRM_DEBUG_KMS("Set Error Count Phy Test Pattern\n");
- intel_de_write(dev_priv, DDI_DP_COMP_CTL(pipe),
+ intel_de_write(dev_priv, dp_comp_reg,
DDI_DP_COMP_CTL_ENABLE |
DDI_DP_COMP_CTL_SCRAMBLED_0);
break;
case DP_PHY_TEST_PATTERN_PRBS7:
DRM_DEBUG_KMS("Set PRBS7 Phy Test Pattern\n");
- intel_de_write(dev_priv, DDI_DP_COMP_CTL(pipe),
+ intel_de_write(dev_priv, dp_comp_reg,
DDI_DP_COMP_CTL_ENABLE | DDI_DP_COMP_CTL_PRBS7);
break;
case DP_PHY_TEST_PATTERN_80BIT_CUSTOM:
@@ -5432,14 +5443,27 @@ static void intel_dp_phy_pattern_update(struct intel_dp *intel_dp)
* current firmware of DPR-100 could not set it, so hardcoding
* now for complaince test.
*/
- DRM_DEBUG_KMS("Set 80Bit Custom Phy Test Pattern 0x3e0f83e0 0x0f83e0f8 0x0000f83e\n");
+ DRM_DEBUG_KMS("Set 80Bit Custom Phy Test Pattern \
+ 0x3e0f83e0 0x0f83e0f8 0x0000f83e\n");
pattern_val = 0x3e0f83e0;
- intel_de_write(dev_priv, DDI_DP_COMP_PAT(pipe, 0), pattern_val);
+ if (IS_ELKHARTLAKE(dev_priv))
+ intel_de_write(dev_priv, EHL_DDI_DP_COMP_PAT(dig_port->base.port, 0),
+ pattern_val);
+ else
+ intel_de_write(dev_priv, DDI_DP_COMP_PAT(pipe, 0), pattern_val);
pattern_val = 0x0f83e0f8;
- intel_de_write(dev_priv, DDI_DP_COMP_PAT(pipe, 1), pattern_val);
+ if (IS_ELKHARTLAKE(dev_priv))
+ intel_de_write(dev_priv, EHL_DDI_DP_COMP_PAT(dig_port->base.port, 1),
+ pattern_val);
+ else
+ intel_de_write(dev_priv, DDI_DP_COMP_PAT(pipe, 1), pattern_val);
pattern_val = 0x0000f83e;
- intel_de_write(dev_priv, DDI_DP_COMP_PAT(pipe, 2), pattern_val);
- intel_de_write(dev_priv, DDI_DP_COMP_CTL(pipe),
+ if (IS_ELKHARTLAKE(dev_priv))
+ intel_de_write(dev_priv, EHL_DDI_DP_COMP_PAT(dig_port->base.port, 2),
+ pattern_val);
+ else
+ intel_de_write(dev_priv, DDI_DP_COMP_PAT(pipe, 2), pattern_val);
+ intel_de_write(dev_priv, dp_comp_reg,
DDI_DP_COMP_CTL_ENABLE |
DDI_DP_COMP_CTL_CUSTOM80);
break;
@@ -5451,20 +5475,20 @@ static void intel_dp_phy_pattern_update(struct intel_dp *intel_dp)
*/
DRM_DEBUG_KMS("Set HBR2 compliance Phy Test Pattern\n");
pattern_val = 0xFB;
- intel_de_write(dev_priv, DDI_DP_COMP_CTL(pipe),
+ intel_de_write(dev_priv, dp_comp_reg,
DDI_DP_COMP_CTL_ENABLE | DDI_DP_COMP_CTL_HBR2 |
pattern_val);
break;
- case DP_PHY_TEST_PATTERN_CP2520_PAT3:
- DRM_DEBUG_KMS("Set TPS4 Phy Test Pattern\n");
- intel_de_write(dev_priv, DDI_DP_COMP_CTL(pipe), 0x0);
- dp_tp_ctl = intel_de_read(dev_priv, TGL_DP_TP_CTL(pipe));
- dp_tp_ctl &= ~DP_TP_CTL_TRAIN_PAT4_SEL_MASK;
- dp_tp_ctl |= DP_TP_CTL_TRAIN_PAT4_SEL_TP4a;
- dp_tp_ctl &= ~DP_TP_CTL_LINK_TRAIN_MASK;
- dp_tp_ctl |= DP_TP_CTL_LINK_TRAIN_PAT4;
- intel_de_write(dev_priv, TGL_DP_TP_CTL(pipe), dp_tp_ctl);
- break;
+ case DP_PHY_TEST_PATTERN_CP2520_PAT3:
+ DRM_DEBUG_KMS("Set TPS4 Phy Test Pattern\n");
+ intel_de_write(dev_priv, dp_comp_reg, 0x0);
+ dp_tp_ctl = intel_de_read(dev_priv, dp_tp_reg);
+ dp_tp_ctl &= ~DP_TP_CTL_TRAIN_PAT4_SEL_MASK;
+ dp_tp_ctl |= DP_TP_CTL_TRAIN_PAT4_SEL_TP4a;
+ dp_tp_ctl &= ~DP_TP_CTL_LINK_TRAIN_MASK;
+ dp_tp_ctl |= DP_TP_CTL_LINK_TRAIN_PAT4;
+ intel_de_write(dev_priv, dp_tp_reg, dp_tp_ctl);
+ break;
default:
WARN(1, "Invalid Phy Test Pattern\n");
}
@@ -5478,22 +5502,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(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;
+ i915_reg_t dp_tp_reg;
+
+ if (IS_ELKHARTLAKE(dev_priv)) {
+ dp_tp_reg = DP_TP_CTL(dig_port->base.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
@@ -5505,20 +5539,29 @@ intel_dp_autotest_phy_ddi_enable(struct intel_dp *intel_dp, uint8_t lane_cnt)
enum port port = dig_port->base.port;
struct intel_crtc *crtc = to_intel_crtc(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));
-
+ dp_tp_ctl_value = intel_de_read(dev_priv, dp_tp_reg);
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);
}
@@ -5590,6 +5633,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_DEBUG_KMS( \
+ "PHY compliance for platform not supported\n");
+ return;
+ }
drm_dbg_kms(&i915->drm, "PHY_PATTERN test requested\n");
response = intel_dp_autotest_phy_pattern(intel_dp);
break;
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 4850890918dc..7d3b6779661f 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -10026,10 +10026,16 @@ enum skl_power_gate {
#define DDI_BUF_BALANCE_LEG_ENABLE (1 << 31)
#define DDI_BUF_TRANS_HI(port, i) _MMIO(_PORT(port, _DDI_BUF_TRANS_A, _DDI_BUF_TRANS_B) + (i) * 8 + 4)
+/* EHL/JSL DP compliance control */
+#define _EHL_DDI_DP_COMP_CTL_A 0x640F0
+#define _EHL_DDI_DP_COMP_CTL_B 0x641F0
+#define EHL_DDI_DP_COMP_CTL(port) \
+ _MMIO_PORT(port, _EHL_DDI_DP_COMP_CTL_A, _EHL_DDI_DP_COMP_CTL_B)
+
/* DDI DP Compliance Control */
-#define _DDI_DP_COMP_CTL_A 0x605F0
-#define _DDI_DP_COMP_CTL_B 0x615F0
-#define DDI_DP_COMP_CTL(pipe) _MMIO_PIPE(pipe, _DDI_DP_COMP_CTL_A, _DDI_DP_COMP_CTL_B)
+#define _DDI_DP_COMP_CTL_A 0x605F0
+#define _DDI_DP_COMP_CTL_B 0x615F0
+#define DDI_DP_COMP_CTL(pipe) _MMIO_PIPE(pipe, _DDI_DP_COMP_CTL_A, _DDI_DP_COMP_CTL_B)
#define DDI_DP_COMP_CTL_ENABLE (1 << 31)
#define DDI_DP_COMP_CTL_D10_2 (0 << 28)
#define DDI_DP_COMP_CTL_SCRAMBLED_0 (1 << 28)
@@ -10039,6 +10045,12 @@ enum skl_power_gate {
#define DDI_DP_COMP_CTL_SCRAMBLED_1 (5 << 28)
#define DDI_DP_COMP_CTL_HBR2_RESET (0xFC << 0)
+/* EHL */
+#define _EHL_DDI_DP_COMP_PAT_A 0x640F4
+#define _EHL_DDI_DP_COMP_PAT_B 0x641F4
+#define EHL_DDI_DP_COMP_PAT(port, i) \
+ _MMIO(_PORT(port, _EHL_DDI_DP_COMP_PAT_A, _EHL_DDI_DP_COMP_PAT_B) + (i) * 4)
+
/* DDI DP Compliance Pattern */
#define _DDI_DP_COMP_PAT_A 0x605F4
#define _DDI_DP_COMP_PAT_B 0x615F4
--
2.7.4
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 23+ messages in thread
end of thread, other threads:[~2020-09-10 4:05 UTC | newest]
Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-04 5:03 [PATCH] drm/i915/dp: DP PHY compliance for JSL Vidya Srinivas
2020-06-04 19:06 ` [Intel-gfx] " Ville Syrjälä
2020-06-04 20:01 ` Almahallawy, Khaled
2020-06-04 21:03 ` Ville Syrjälä
2020-06-12 18:25 ` Manasi Navare
2020-06-12 18:36 ` Ville Syrjälä
2020-06-12 18:44 ` Manasi Navare
2020-06-12 19:01 ` Ville Syrjälä
2020-06-12 19:12 ` Manasi Navare
2020-06-12 19:21 ` Ville Syrjälä
2020-06-12 19:38 ` Manasi Navare
2020-06-15 16:19 ` Ville Syrjälä
2020-06-12 18:33 ` Manasi Navare
2020-06-12 18:39 ` Ville Syrjälä
2020-09-04 4:13 ` [PATCH 1/3] drm/dp: Add PHY_TEST_PATTERN CP2520 Pattern 2 and 3 Vidya Srinivas
2020-09-04 4:13 ` [PATCH] drm/i915/dp: DP PHY compliance for EHL/JSL Vidya Srinivas
2020-09-04 4:13 ` [PATCH 2/3] drm/i915/dp: TPS4 PHY test pattern compliance support Vidya Srinivas
2020-09-04 4:13 ` [PATCH 3/3] [RFC] drm/i915/dp: DP PHY compliance for EHL/JSL Vidya Srinivas
2020-09-04 4:14 ` [PATCH 1/3] drm/dp: Add PHY_TEST_PATTERN CP2520 Pattern 2 and 3 Vidya Srinivas
2020-09-04 4:14 ` [PATCH 2/3] drm/i915/dp: TPS4 PHY test pattern compliance support Vidya Srinivas
2020-09-04 4:14 ` [PATCH 3/3] [RFC] drm/i915/dp: DP PHY compliance for EHL/JSL Vidya Srinivas
2020-09-04 4:29 ` [PATCH] " Vidya Srinivas
2020-09-10 4:00 ` Vidya Srinivas
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).