[PATCH v2 0/1] drm/i915: Add and enable DP AUX CH mutex

[PATCH v2 0/1] drm/i915: Add and enable DP AUX CH mutex

[José Roberto de Souza]

v2 of https://lists.freedesktop.org/archives/intel-gfx/2018-February/155502.html and https://lists.freedesktop.org/archives/intel-gfx/2018-February/155498.html

Changelog:
- removed the PSR dependency, now getting lock all the times when available
- renamed functions to avoid nested calls
- moved register bits right after the DP_AUX_CH_MUTEX()
- removed 'drm/i915: keep AUX powered while PSR is enabled' Dhinakaran Pandiyan will sent a better and final version

José Roberto de Souza (1):
  drm/i915: Add and enable DP AUX CH mutex

 drivers/gpu/drm/i915/i915_reg.h  |  9 ++++++++
 drivers/gpu/drm/i915/intel_dp.c  | 50 ++++++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h |  1 +
 3 files changed, 60 insertions(+)

-- 
2.16.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 1/1] drm/i915: Add and enable DP AUX CH mutex

[José Roberto de Souza]

When PSR/PSR2/GTC is enabled hardware can do AUX transactions by it
self, so lets use the mutex register that is available in gen9+ to
avoid concurrent access by hardware and driver.

Reference: https://01.org/sites/default/files/documentation/intel-gfx-prm-osrc-skl-vol12-display.pdf
Page 198 - AUX programming sequence

Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h  |  9 ++++++++
 drivers/gpu/drm/i915/intel_dp.c  | 50 ++++++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h |  1 +
 3 files changed, 60 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 1412abcb27d4..a62e3c1badab 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -5318,6 +5318,7 @@ enum {
 #define _DPA_AUX_CH_DATA3	(dev_priv->info.display_mmio_offset + 0x6401c)
 #define _DPA_AUX_CH_DATA4	(dev_priv->info.display_mmio_offset + 0x64020)
 #define _DPA_AUX_CH_DATA5	(dev_priv->info.display_mmio_offset + 0x64024)
+#define _DPA_AUX_CH_MUTEX	(dev_priv->info.display_mmio_offset + 0x6402C)
 
 #define _DPB_AUX_CH_CTL		(dev_priv->info.display_mmio_offset + 0x64110)
 #define _DPB_AUX_CH_DATA1	(dev_priv->info.display_mmio_offset + 0x64114)
@@ -5325,6 +5326,7 @@ enum {
 #define _DPB_AUX_CH_DATA3	(dev_priv->info.display_mmio_offset + 0x6411c)
 #define _DPB_AUX_CH_DATA4	(dev_priv->info.display_mmio_offset + 0x64120)
 #define _DPB_AUX_CH_DATA5	(dev_priv->info.display_mmio_offset + 0x64124)
+#define _DPB_AUX_CH_MUTEX	(dev_priv->info.display_mmio_offset + 0x6412C)
 
 #define _DPC_AUX_CH_CTL		(dev_priv->info.display_mmio_offset + 0x64210)
 #define _DPC_AUX_CH_DATA1	(dev_priv->info.display_mmio_offset + 0x64214)
@@ -5332,6 +5334,7 @@ enum {
 #define _DPC_AUX_CH_DATA3	(dev_priv->info.display_mmio_offset + 0x6421c)
 #define _DPC_AUX_CH_DATA4	(dev_priv->info.display_mmio_offset + 0x64220)
 #define _DPC_AUX_CH_DATA5	(dev_priv->info.display_mmio_offset + 0x64224)
+#define _DPC_AUX_CH_MUTEX	(dev_priv->info.display_mmio_offset + 0x6422C)
 
 #define _DPD_AUX_CH_CTL		(dev_priv->info.display_mmio_offset + 0x64310)
 #define _DPD_AUX_CH_DATA1	(dev_priv->info.display_mmio_offset + 0x64314)
@@ -5339,6 +5342,7 @@ enum {
 #define _DPD_AUX_CH_DATA3	(dev_priv->info.display_mmio_offset + 0x6431c)
 #define _DPD_AUX_CH_DATA4	(dev_priv->info.display_mmio_offset + 0x64320)
 #define _DPD_AUX_CH_DATA5	(dev_priv->info.display_mmio_offset + 0x64324)
+#define _DPD_AUX_CH_MUTEX	(dev_priv->info.display_mmio_offset + 0x6432C)
 
 #define _DPF_AUX_CH_CTL		(dev_priv->info.display_mmio_offset + 0x64510)
 #define _DPF_AUX_CH_DATA1	(dev_priv->info.display_mmio_offset + 0x64514)
@@ -5346,6 +5350,7 @@ enum {
 #define _DPF_AUX_CH_DATA3	(dev_priv->info.display_mmio_offset + 0x6451c)
 #define _DPF_AUX_CH_DATA4	(dev_priv->info.display_mmio_offset + 0x64520)
 #define _DPF_AUX_CH_DATA5	(dev_priv->info.display_mmio_offset + 0x64524)
+#define _DPF_AUX_CH_MUTEX	(dev_priv->info.display_mmio_offset + 0x6452C)
 
 #define DP_AUX_CH_CTL(port)	_MMIO_PORT(port, _DPA_AUX_CH_CTL, _DPB_AUX_CH_CTL)
 #define DP_AUX_CH_DATA(port, i)	_MMIO(_PORT(port, _DPA_AUX_CH_DATA1, _DPB_AUX_CH_DATA1) + (i) * 4) /* 5 registers */
@@ -5378,6 +5383,10 @@ enum {
 #define   DP_AUX_CH_CTL_FW_SYNC_PULSE_SKL(c) (((c) - 1) << 5)
 #define   DP_AUX_CH_CTL_SYNC_PULSE_SKL(c)   ((c) - 1)
 
+#define DP_AUX_CH_MUTEX(port)	_MMIO_PORT(port, _DPA_AUX_CH_MUTEX, _DPB_AUX_CH_MUTEX)
+#define   DP_AUX_CH_MUTEX_ENABLE		(1 << 31)
+#define   DP_AUX_CH_MUTEX_STATUS		(1 << 30)
+
 /*
  * Computing GMCH M and N values for the Display Port link
  *
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index f20b25f98e5a..af07563bafba 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1081,6 +1081,45 @@ static uint32_t intel_dp_get_aux_send_ctl(struct intel_dp *intel_dp,
 						aux_clock_divider);
 }
 
+static bool intel_dp_aux_ch_trylock(struct intel_dp *intel_dp)
+{
+	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
+	struct drm_i915_private *dev_priv =
+			to_i915(intel_dig_port->base.base.dev);
+
+	if (INTEL_GEN(dev_priv) < 9)
+		return true;
+
+	I915_WRITE(intel_dp->aux_ch_mutex_reg, DP_AUX_CH_MUTEX_ENABLE);
+
+	/* Spec says that mutex is acquired when status bit is read as unset,
+	 * if set it should be read again after 500usec. Here waiting for
+	 * 2msec or 4 tries before give up.
+	 */
+	if (__intel_wait_for_register(dev_priv, intel_dp->aux_ch_mutex_reg,
+				      DP_AUX_CH_MUTEX_STATUS, 0, 500, 2,
+				      NULL)) {
+		DRM_WARN("dp_aux_ch port locked for too long");
+		return false;
+	}
+
+	return true;
+}
+
+static void intel_dp_aux_ch_unlock(struct intel_dp *intel_dp)
+{
+	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
+	struct drm_i915_private *dev_priv =
+			to_i915(intel_dig_port->base.base.dev);
+
+	if (INTEL_GEN(dev_priv) < 9)
+		return;
+
+	/* setting the status bit releases the mutex + keep mutex enabled */
+	I915_WRITE(intel_dp->aux_ch_mutex_reg, DP_AUX_CH_MUTEX_ENABLE |
+		   DP_AUX_CH_MUTEX_STATUS);
+}
+
 static int
 intel_dp_aux_ch(struct intel_dp *intel_dp,
 		const uint8_t *send, int send_bytes,
@@ -1115,6 +1154,11 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
 
 	intel_dp_check_edp(intel_dp);
 
+	if (!intel_dp_aux_ch_trylock(intel_dp)) {
+		ret = -EBUSY;
+		goto out_locked;
+	}
+
 	/* Try to wait for any previous AUX channel activity */
 	for (try = 0; try < 3; try++) {
 		status = I915_READ_NOTRACE(ch_ctl);
@@ -1244,6 +1288,8 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
 
 	ret = recv_bytes;
 out:
+	intel_dp_aux_ch_unlock(intel_dp);
+out_locked:
 	pm_qos_update_request(&dev_priv->pm_qos, PM_QOS_DEFAULT_VALUE);
 
 	if (vdd)
@@ -1496,6 +1542,10 @@ static void intel_aux_reg_init(struct intel_dp *intel_dp)
 	intel_dp->aux_ch_ctl_reg = intel_aux_ctl_reg(dev_priv, port);
 	for (i = 0; i < ARRAY_SIZE(intel_dp->aux_ch_data_reg); i++)
 		intel_dp->aux_ch_data_reg[i] = intel_aux_data_reg(dev_priv, port, i);
+	if (INTEL_INFO(dev_priv)->gen >= 9) {
+		intel_dp->aux_ch_mutex_reg = DP_AUX_CH_MUTEX(port);
+		I915_WRITE(intel_dp->aux_ch_mutex_reg, DP_AUX_CH_MUTEX_ENABLE);
+	}
 }
 
 static void
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 50874f4035cf..0c88657d6066 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1043,6 +1043,7 @@ struct intel_dp {
 	i915_reg_t output_reg;
 	i915_reg_t aux_ch_ctl_reg;
 	i915_reg_t aux_ch_data_reg[5];
+	i915_reg_t aux_ch_mutex_reg;
 	uint32_t DP;
 	int link_rate;
 	uint8_t lane_count;
-- 
2.16.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.CHECKPATCH: warning for drm/i915: Add and enable DP AUX CH mutex

[Patchwork]

== Series Details ==

Series: drm/i915: Add and enable DP AUX CH mutex
URL   : https://patchwork.freedesktop.org/series/38655/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
c4130a3bc92e drm/i915: Add and enable DP AUX CH mutex
-:13: WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#13: 
Reference: https://01.org/sites/default/files/documentation/intel-gfx-prm-osrc-skl-vol12-display.pdf

-:68: WARNING: line over 80 characters
#68: FILE: drivers/gpu/drm/i915/i915_reg.h:5386:
+#define DP_AUX_CH_MUTEX(port)	_MMIO_PORT(port, _DPA_AUX_CH_MUTEX, _DPB_AUX_CH_MUTEX)

-:69: CHECK: Prefer using the BIT macro
#69: FILE: drivers/gpu/drm/i915/i915_reg.h:5387:
+#define   DP_AUX_CH_MUTEX_ENABLE		(1 << 31)

-:70: CHECK: Prefer using the BIT macro
#70: FILE: drivers/gpu/drm/i915/i915_reg.h:5388:
+#define   DP_AUX_CH_MUTEX_STATUS		(1 << 30)

total: 0 errors, 2 warnings, 2 checks, 126 lines checked

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for drm/i915: Add and enable DP AUX CH mutex

[Patchwork]

== Series Details ==

Series: drm/i915: Add and enable DP AUX CH mutex
URL   : https://patchwork.freedesktop.org/series/38655/
State : success

== Summary ==

Series 38655v1 drm/i915: Add and enable DP AUX CH mutex
https://patchwork.freedesktop.org/api/1.0/series/38655/revisions/1/mbox/

Test gem_mmap_gtt:
        Subgroup basic-small-bo-tiledx:
                pass       -> FAIL       (fi-gdg-551) fdo#102575

fdo#102575 https://bugs.freedesktop.org/show_bug.cgi?id=102575

fi-bdw-5557u     total:288  pass:267  dwarn:0   dfail:0   fail:0   skip:21  time:414s
fi-bdw-gvtdvm    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:425s
fi-blb-e6850     total:288  pass:223  dwarn:1   dfail:0   fail:0   skip:64  time:373s
fi-bsw-n3050     total:288  pass:242  dwarn:0   dfail:0   fail:0   skip:46  time:494s
fi-bwr-2160      total:288  pass:183  dwarn:0   dfail:0   fail:0   skip:105 time:284s
fi-bxt-dsi       total:288  pass:258  dwarn:0   dfail:0   fail:0   skip:30  time:480s
fi-bxt-j4205     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:479s
fi-byt-j1900     total:288  pass:253  dwarn:0   dfail:0   fail:0   skip:35  time:462s
fi-byt-n2820     total:288  pass:249  dwarn:0   dfail:0   fail:0   skip:39  time:453s
fi-cfl-s2        total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:568s
fi-elk-e7500     total:288  pass:229  dwarn:0   dfail:0   fail:0   skip:59  time:414s
fi-gdg-551       total:288  pass:179  dwarn:0   dfail:0   fail:1   skip:108 time:284s
fi-glk-1         total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:506s
fi-hsw-4770      total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:386s
fi-ilk-650       total:288  pass:228  dwarn:0   dfail:0   fail:0   skip:60  time:408s
fi-ivb-3520m     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:450s
fi-ivb-3770      total:288  pass:255  dwarn:0   dfail:0   fail:0   skip:33  time:409s
fi-kbl-7500u     total:288  pass:263  dwarn:1   dfail:0   fail:0   skip:24  time:452s
fi-kbl-7560u     total:288  pass:269  dwarn:0   dfail:0   fail:0   skip:19  time:489s
fi-kbl-7567u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:458s
fi-kbl-r         total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:491s
fi-pnv-d510      total:288  pass:222  dwarn:1   dfail:0   fail:0   skip:65  time:582s
fi-skl-6260u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:430s
fi-skl-6600u     total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:504s
fi-skl-6700hq    total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:520s
fi-skl-6700k2    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:484s
fi-skl-6770hq    total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:464s
fi-skl-guc       total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:407s
fi-skl-gvtdvm    total:288  pass:265  dwarn:0   dfail:0   fail:0   skip:23  time:433s
fi-snb-2520m     total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:520s
fi-snb-2600      total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:396s

f727568c3b37c1349f635efcc67d64ac3cf77108 drm-tip: 2018y-02m-20d-20h-39m-03s UTC integration manifest
c4130a3bc92e drm/i915: Add and enable DP AUX CH mutex

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_8094/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 1/1] drm/i915: Add and enable DP AUX CH mutex

[Dhinakaran Pandiyan]

On Tuesday, February 20, 2018 6:23:47 PM PST José Roberto de Souza wrote:
> When PSR/PSR2/GTC is enabled hardware can do AUX transactions by it
> self, so lets use the mutex register that is available in gen9+ to
> avoid concurrent access by hardware and driver.
> 
> Reference:
> https://01.org/sites/default/files/documentation/intel-gfx-prm-osrc-skl-vol
> 12-display.pdf Page 198 - AUX programming sequence
> 
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h  |  9 ++++++++
>  drivers/gpu/drm/i915/intel_dp.c  | 50
> ++++++++++++++++++++++++++++++++++++++++ drivers/gpu/drm/i915/intel_drv.h |
>  1 +
>  3 files changed, 60 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h
> b/drivers/gpu/drm/i915/i915_reg.h index 1412abcb27d4..a62e3c1badab 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -5318,6 +5318,7 @@ enum {
>  #define _DPA_AUX_CH_DATA3	(dev_priv->info.display_mmio_offset + 0x6401c)
>  #define _DPA_AUX_CH_DATA4	(dev_priv->info.display_mmio_offset + 0x64020)
>  #define _DPA_AUX_CH_DATA5	(dev_priv->info.display_mmio_offset + 0x64024)
> +#define _DPA_AUX_CH_MUTEX	(dev_priv->info.display_mmio_offset + 0x6402C)
> 
>  #define _DPB_AUX_CH_CTL		(dev_priv->info.display_mmio_offset + 0x64110)
>  #define _DPB_AUX_CH_DATA1	(dev_priv->info.display_mmio_offset + 0x64114)
> @@ -5325,6 +5326,7 @@ enum {
>  #define _DPB_AUX_CH_DATA3	(dev_priv->info.display_mmio_offset + 0x6411c)
>  #define _DPB_AUX_CH_DATA4	(dev_priv->info.display_mmio_offset + 0x64120)
>  #define _DPB_AUX_CH_DATA5	(dev_priv->info.display_mmio_offset + 0x64124)
> +#define _DPB_AUX_CH_MUTEX	(dev_priv->info.display_mmio_offset + 0x6412C)
> 
>  #define _DPC_AUX_CH_CTL		(dev_priv->info.display_mmio_offset + 0x64210)
>  #define _DPC_AUX_CH_DATA1	(dev_priv->info.display_mmio_offset + 0x64214)
> @@ -5332,6 +5334,7 @@ enum {
>  #define _DPC_AUX_CH_DATA3	(dev_priv->info.display_mmio_offset + 0x6421c)
>  #define _DPC_AUX_CH_DATA4	(dev_priv->info.display_mmio_offset + 0x64220)
>  #define _DPC_AUX_CH_DATA5	(dev_priv->info.display_mmio_offset + 0x64224)
> +#define _DPC_AUX_CH_MUTEX	(dev_priv->info.display_mmio_offset + 0x6422C)
> 
>  #define _DPD_AUX_CH_CTL		(dev_priv->info.display_mmio_offset + 0x64310)
>  #define _DPD_AUX_CH_DATA1	(dev_priv->info.display_mmio_offset + 0x64314)
> @@ -5339,6 +5342,7 @@ enum {
>  #define _DPD_AUX_CH_DATA3	(dev_priv->info.display_mmio_offset + 0x6431c)
>  #define _DPD_AUX_CH_DATA4	(dev_priv->info.display_mmio_offset + 0x64320)
>  #define _DPD_AUX_CH_DATA5	(dev_priv->info.display_mmio_offset + 0x64324)
> +#define _DPD_AUX_CH_MUTEX	(dev_priv->info.display_mmio_offset + 0x6432C)
> 
>  #define _DPF_AUX_CH_CTL		(dev_priv->info.display_mmio_offset + 0x64510)
>  #define _DPF_AUX_CH_DATA1	(dev_priv->info.display_mmio_offset + 0x64514)
> @@ -5346,6 +5350,7 @@ enum {
>  #define _DPF_AUX_CH_DATA3	(dev_priv->info.display_mmio_offset + 0x6451c)
>  #define _DPF_AUX_CH_DATA4	(dev_priv->info.display_mmio_offset + 0x64520)
>  #define _DPF_AUX_CH_DATA5	(dev_priv->info.display_mmio_offset + 0x64524)
> +#define _DPF_AUX_CH_MUTEX	(dev_priv->info.display_mmio_offset + 0x6452C)
> 
>  #define DP_AUX_CH_CTL(port)	_MMIO_PORT(port, _DPA_AUX_CH_CTL,
> _DPB_AUX_CH_CTL) #define DP_AUX_CH_DATA(port, i)	_MMIO(_PORT(port,
> _DPA_AUX_CH_DATA1, _DPB_AUX_CH_DATA1) + (i) * 4) /* 5 registers */ @@
> -5378,6 +5383,10 @@ enum {
>  #define   DP_AUX_CH_CTL_FW_SYNC_PULSE_SKL(c) (((c) - 1) << 5)
>  #define   DP_AUX_CH_CTL_SYNC_PULSE_SKL(c)   ((c) - 1)
> 
> +#define DP_AUX_CH_MUTEX(port)	_MMIO_PORT(port, _DPA_AUX_CH_MUTEX,
> _DPB_AUX_CH_MUTEX) +#define   DP_AUX_CH_MUTEX_ENABLE		(1 << 31)
> +#define   DP_AUX_CH_MUTEX_STATUS		(1 << 30)
> +
>  /*
>   * Computing GMCH M and N values for the Display Port link
>   *
> diff --git a/drivers/gpu/drm/i915/intel_dp.c
> b/drivers/gpu/drm/i915/intel_dp.c index f20b25f98e5a..af07563bafba 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1081,6 +1081,45 @@ static uint32_t intel_dp_get_aux_send_ctl(struct
> intel_dp *intel_dp, aux_clock_divider);
>  }
> 
> +static bool intel_dp_aux_ch_trylock(struct intel_dp *intel_dp)
> +{
> +	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> +	struct drm_i915_private *dev_priv =
> +			to_i915(intel_dig_port->base.base.dev);
> +
> +	if (INTEL_GEN(dev_priv) < 9)
> +		return true;
> +
> +	I915_WRITE(intel_dp->aux_ch_mutex_reg, DP_AUX_CH_MUTEX_ENABLE);
> +
> +	/* Spec says that mutex is acquired when status bit is read as unset,
> +	 * if set it should be read again after 500usec. Here waiting for
> +	 * 2msec or 4 tries before give up.
> +	 */
> +	if (__intel_wait_for_register(dev_priv, intel_dp->aux_ch_mutex_reg,
> +				      DP_AUX_CH_MUTEX_STATUS, 0, 500, 2,
> +				      NULL)) {
> +		DRM_WARN("dp_aux_ch port locked for too long");
> +		return false;
> +	}
> +

Jose,

Thanks for the patch. One of the reasons we wanted the mutex implemented was 
PSR_STATUS hinted that the hardware was busy with AUX transactions when the 
driver called psr_activate/psr_exit (from psr_flush or psr_invalidate) . Or 
between psr_activate and psr_exit in the terminal mode. So my question is, are 
you seeing any timeouts or mutex busyness with i915.enable_psr=1 and regular 
laptop use.

-DK





_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 1/1] drm/i915: Add and enable DP AUX CH mutex

[Souza, Jose]

On Wed, 2018-02-21 at 00:59 -0800, Dhinakaran Pandiyan wrote:
> On Tuesday, February 20, 2018 6:23:47 PM PST José Roberto de Souza
> wrote:
> > When PSR/PSR2/GTC is enabled hardware can do AUX transactions by it
> > self, so lets use the mutex register that is available in gen9+ to
> > avoid concurrent access by hardware and driver.
> > 
> > Reference:
> > https://01.org/sites/default/files/documentation/intel-gfx-prm-osrc
> > -skl-vol
> > 12-display.pdf Page 198 - AUX programming sequence
> > 
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_reg.h  |  9 ++++++++
> >  drivers/gpu/drm/i915/intel_dp.c  | 50
> > ++++++++++++++++++++++++++++++++++++++++
> > drivers/gpu/drm/i915/intel_drv.h |
> >  1 +
> >  3 files changed, 60 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > b/drivers/gpu/drm/i915/i915_reg.h index 1412abcb27d4..a62e3c1badab
> > 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -5318,6 +5318,7 @@ enum {
> >  #define _DPA_AUX_CH_DATA3	(dev_priv-
> > >info.display_mmio_offset + 0x6401c)
> >  #define _DPA_AUX_CH_DATA4	(dev_priv-
> > >info.display_mmio_offset + 0x64020)
> >  #define _DPA_AUX_CH_DATA5	(dev_priv-
> > >info.display_mmio_offset + 0x64024)
> > +#define _DPA_AUX_CH_MUTEX	(dev_priv-
> > >info.display_mmio_offset + 0x6402C)
> > 
> >  #define _DPB_AUX_CH_CTL		(dev_priv-
> > >info.display_mmio_offset + 0x64110)
> >  #define _DPB_AUX_CH_DATA1	(dev_priv-
> > >info.display_mmio_offset + 0x64114)
> > @@ -5325,6 +5326,7 @@ enum {
> >  #define _DPB_AUX_CH_DATA3	(dev_priv-
> > >info.display_mmio_offset + 0x6411c)
> >  #define _DPB_AUX_CH_DATA4	(dev_priv-
> > >info.display_mmio_offset + 0x64120)
> >  #define _DPB_AUX_CH_DATA5	(dev_priv-
> > >info.display_mmio_offset + 0x64124)
> > +#define _DPB_AUX_CH_MUTEX	(dev_priv-
> > >info.display_mmio_offset + 0x6412C)
> > 
> >  #define _DPC_AUX_CH_CTL		(dev_priv-
> > >info.display_mmio_offset + 0x64210)
> >  #define _DPC_AUX_CH_DATA1	(dev_priv-
> > >info.display_mmio_offset + 0x64214)
> > @@ -5332,6 +5334,7 @@ enum {
> >  #define _DPC_AUX_CH_DATA3	(dev_priv-
> > >info.display_mmio_offset + 0x6421c)
> >  #define _DPC_AUX_CH_DATA4	(dev_priv-
> > >info.display_mmio_offset + 0x64220)
> >  #define _DPC_AUX_CH_DATA5	(dev_priv-
> > >info.display_mmio_offset + 0x64224)
> > +#define _DPC_AUX_CH_MUTEX	(dev_priv-
> > >info.display_mmio_offset + 0x6422C)
> > 
> >  #define _DPD_AUX_CH_CTL		(dev_priv-
> > >info.display_mmio_offset + 0x64310)
> >  #define _DPD_AUX_CH_DATA1	(dev_priv-
> > >info.display_mmio_offset + 0x64314)
> > @@ -5339,6 +5342,7 @@ enum {
> >  #define _DPD_AUX_CH_DATA3	(dev_priv-
> > >info.display_mmio_offset + 0x6431c)
> >  #define _DPD_AUX_CH_DATA4	(dev_priv-
> > >info.display_mmio_offset + 0x64320)
> >  #define _DPD_AUX_CH_DATA5	(dev_priv-
> > >info.display_mmio_offset + 0x64324)
> > +#define _DPD_AUX_CH_MUTEX	(dev_priv-
> > >info.display_mmio_offset + 0x6432C)
> > 
> >  #define _DPF_AUX_CH_CTL		(dev_priv-
> > >info.display_mmio_offset + 0x64510)
> >  #define _DPF_AUX_CH_DATA1	(dev_priv-
> > >info.display_mmio_offset + 0x64514)
> > @@ -5346,6 +5350,7 @@ enum {
> >  #define _DPF_AUX_CH_DATA3	(dev_priv-
> > >info.display_mmio_offset + 0x6451c)
> >  #define _DPF_AUX_CH_DATA4	(dev_priv-
> > >info.display_mmio_offset + 0x64520)
> >  #define _DPF_AUX_CH_DATA5	(dev_priv-
> > >info.display_mmio_offset + 0x64524)
> > +#define _DPF_AUX_CH_MUTEX	(dev_priv-
> > >info.display_mmio_offset + 0x6452C)
> > 
> >  #define DP_AUX_CH_CTL(port)	_MMIO_PORT(port,
> > _DPA_AUX_CH_CTL,
> > _DPB_AUX_CH_CTL) #define DP_AUX_CH_DATA(port, i)	_MMIO(_PORT
> > (port,
> > _DPA_AUX_CH_DATA1, _DPB_AUX_CH_DATA1) + (i) * 4) /* 5 registers */
> > @@
> > -5378,6 +5383,10 @@ enum {
> >  #define   DP_AUX_CH_CTL_FW_SYNC_PULSE_SKL(c) (((c) - 1) << 5)
> >  #define   DP_AUX_CH_CTL_SYNC_PULSE_SKL(c)   ((c) - 1)
> > 
> > +#define DP_AUX_CH_MUTEX(port)	_MMIO_PORT(port,
> > _DPA_AUX_CH_MUTEX,
> > _DPB_AUX_CH_MUTEX) +#define   DP_AUX_CH_MUTEX_ENABLE		
> > (1 << 31)
> > +#define   DP_AUX_CH_MUTEX_STATUS		(1 << 30)
> > +
> >  /*
> >   * Computing GMCH M and N values for the Display Port link
> >   *
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > b/drivers/gpu/drm/i915/intel_dp.c index f20b25f98e5a..af07563bafba
> > 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -1081,6 +1081,45 @@ static uint32_t
> > intel_dp_get_aux_send_ctl(struct
> > intel_dp *intel_dp, aux_clock_divider);
> >  }
> > 
> > +static bool intel_dp_aux_ch_trylock(struct intel_dp *intel_dp)
> > +{
> > +	struct intel_digital_port *intel_dig_port =
> > dp_to_dig_port(intel_dp);
> > +	struct drm_i915_private *dev_priv =
> > +			to_i915(intel_dig_port->base.base.dev);
> > +
> > +	if (INTEL_GEN(dev_priv) < 9)
> > +		return true;
> > +
> > +	I915_WRITE(intel_dp->aux_ch_mutex_reg,
> > DP_AUX_CH_MUTEX_ENABLE);
> > +
> > +	/* Spec says that mutex is acquired when status bit is
> > read as unset,
> > +	 * if set it should be read again after 500usec. Here
> > waiting for
> > +	 * 2msec or 4 tries before give up.
> > +	 */
> > +	if (__intel_wait_for_register(dev_priv, intel_dp-
> > >aux_ch_mutex_reg,
> > +				      DP_AUX_CH_MUTEX_STATUS, 0,
> > 500, 2,
> > +				      NULL)) {
> > +		DRM_WARN("dp_aux_ch port locked for too long");
> > +		return false;
> > +	}
> > +
> 
> Jose,
> 
> Thanks for the patch. One of the reasons we wanted the mutex
> implemented was 
> PSR_STATUS hinted that the hardware was busy with AUX transactions
> when the 
> driver called psr_activate/psr_exit (from psr_flush or
> psr_invalidate) . Or 
> between psr_activate and psr_exit in the terminal mode. So my
> question is, are 
> you seeing any timeouts or mutex busyness with i915.enable_psr=1 and
> regular 
> laptop use.

With regular use not, I'm able to see mutex locked by hardware but to
see that I had to add a few aux ch transactions before and after
exit/active PSR.

> 
> -DK
> 
> 
> 
> 
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 1/1] drm/i915: Add and enable DP AUX CH mutex

[Rodrigo Vivi]

On Tue, Feb 20, 2018 at 06:23:47PM -0800, José Roberto de Souza wrote:
> When PSR/PSR2/GTC is enabled hardware can do AUX transactions by it
> self, so lets use the mutex register that is available in gen9+ to
> avoid concurrent access by hardware and driver.
> 
> Reference: https://01.org/sites/default/files/documentation/intel-gfx-prm-osrc-skl-vol12-display.pdf
> Page 198 - AUX programming sequence
>

Cc: Ville
Cc: DK
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h  |  9 ++++++++
>  drivers/gpu/drm/i915/intel_dp.c  | 50 ++++++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h |  1 +
>  3 files changed, 60 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 1412abcb27d4..a62e3c1badab 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -5318,6 +5318,7 @@ enum {
>  #define _DPA_AUX_CH_DATA3	(dev_priv->info.display_mmio_offset + 0x6401c)
>  #define _DPA_AUX_CH_DATA4	(dev_priv->info.display_mmio_offset + 0x64020)
>  #define _DPA_AUX_CH_DATA5	(dev_priv->info.display_mmio_offset + 0x64024)
> +#define _DPA_AUX_CH_MUTEX	(dev_priv->info.display_mmio_offset + 0x6402C)
>  
>  #define _DPB_AUX_CH_CTL		(dev_priv->info.display_mmio_offset + 0x64110)
>  #define _DPB_AUX_CH_DATA1	(dev_priv->info.display_mmio_offset + 0x64114)
> @@ -5325,6 +5326,7 @@ enum {
>  #define _DPB_AUX_CH_DATA3	(dev_priv->info.display_mmio_offset + 0x6411c)
>  #define _DPB_AUX_CH_DATA4	(dev_priv->info.display_mmio_offset + 0x64120)
>  #define _DPB_AUX_CH_DATA5	(dev_priv->info.display_mmio_offset + 0x64124)
> +#define _DPB_AUX_CH_MUTEX	(dev_priv->info.display_mmio_offset + 0x6412C)
>  
>  #define _DPC_AUX_CH_CTL		(dev_priv->info.display_mmio_offset + 0x64210)
>  #define _DPC_AUX_CH_DATA1	(dev_priv->info.display_mmio_offset + 0x64214)
> @@ -5332,6 +5334,7 @@ enum {
>  #define _DPC_AUX_CH_DATA3	(dev_priv->info.display_mmio_offset + 0x6421c)
>  #define _DPC_AUX_CH_DATA4	(dev_priv->info.display_mmio_offset + 0x64220)
>  #define _DPC_AUX_CH_DATA5	(dev_priv->info.display_mmio_offset + 0x64224)
> +#define _DPC_AUX_CH_MUTEX	(dev_priv->info.display_mmio_offset + 0x6422C)
>  
>  #define _DPD_AUX_CH_CTL		(dev_priv->info.display_mmio_offset + 0x64310)
>  #define _DPD_AUX_CH_DATA1	(dev_priv->info.display_mmio_offset + 0x64314)
> @@ -5339,6 +5342,7 @@ enum {
>  #define _DPD_AUX_CH_DATA3	(dev_priv->info.display_mmio_offset + 0x6431c)
>  #define _DPD_AUX_CH_DATA4	(dev_priv->info.display_mmio_offset + 0x64320)
>  #define _DPD_AUX_CH_DATA5	(dev_priv->info.display_mmio_offset + 0x64324)
> +#define _DPD_AUX_CH_MUTEX	(dev_priv->info.display_mmio_offset + 0x6432C)
>  
>  #define _DPF_AUX_CH_CTL		(dev_priv->info.display_mmio_offset + 0x64510)
>  #define _DPF_AUX_CH_DATA1	(dev_priv->info.display_mmio_offset + 0x64514)
> @@ -5346,6 +5350,7 @@ enum {
>  #define _DPF_AUX_CH_DATA3	(dev_priv->info.display_mmio_offset + 0x6451c)
>  #define _DPF_AUX_CH_DATA4	(dev_priv->info.display_mmio_offset + 0x64520)
>  #define _DPF_AUX_CH_DATA5	(dev_priv->info.display_mmio_offset + 0x64524)
> +#define _DPF_AUX_CH_MUTEX	(dev_priv->info.display_mmio_offset + 0x6452C)
>  
>  #define DP_AUX_CH_CTL(port)	_MMIO_PORT(port, _DPA_AUX_CH_CTL, _DPB_AUX_CH_CTL)
>  #define DP_AUX_CH_DATA(port, i)	_MMIO(_PORT(port, _DPA_AUX_CH_DATA1, _DPB_AUX_CH_DATA1) + (i) * 4) /* 5 registers */
> @@ -5378,6 +5383,10 @@ enum {
>  #define   DP_AUX_CH_CTL_FW_SYNC_PULSE_SKL(c) (((c) - 1) << 5)
>  #define   DP_AUX_CH_CTL_SYNC_PULSE_SKL(c)   ((c) - 1)
>


I believe _DPA_AUX_CH_MUTEX and _DPB_AUX_CH_MUTEX should be defined here along
with the bits as Jani suggested.
I don't see any reason to tag that far from the bits and close to CTL and DATA.

> +#define DP_AUX_CH_MUTEX(port)	_MMIO_PORT(port, _DPA_AUX_CH_MUTEX, _DPB_AUX_CH_MUTEX)
> +#define   DP_AUX_CH_MUTEX_ENABLE		(1 << 31)
> +#define   DP_AUX_CH_MUTEX_STATUS		(1 << 30)
> +
>  /*
>   * Computing GMCH M and N values for the Display Port link
>   *
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index f20b25f98e5a..af07563bafba 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1081,6 +1081,45 @@ static uint32_t intel_dp_get_aux_send_ctl(struct intel_dp *intel_dp,
>  						aux_clock_divider);
>  }
>  
> +static bool intel_dp_aux_ch_trylock(struct intel_dp *intel_dp)
> +{
> +	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> +	struct drm_i915_private *dev_priv =
> +			to_i915(intel_dig_port->base.base.dev);
> +
> +	if (INTEL_GEN(dev_priv) < 9)
> +		return true;
> +
> +	I915_WRITE(intel_dp->aux_ch_mutex_reg, DP_AUX_CH_MUTEX_ENABLE);

You might be touching bits. We don't know if HW is using the reserved bits or not.
So RMW |= bit 31 here is a good idea.

> +
> +	/* Spec says that mutex is acquired when status bit is read as unset,
> +	 * if set it should be read again after 500usec. Here waiting for
> +	 * 2msec or 4 tries before give up.
> +	 */
> +	if (__intel_wait_for_register(dev_priv, intel_dp->aux_ch_mutex_reg,
> +				      DP_AUX_CH_MUTEX_STATUS, 0, 500, 2,
> +				      NULL)) {

why not intel_wait_for_register?
that seems cleaner...

> +		DRM_WARN("dp_aux_ch port locked for too long");
> +		return false;
> +	}
> +
> +	return true;
> +}
> +
> +static void intel_dp_aux_ch_unlock(struct intel_dp *intel_dp)
> +{
> +	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> +	struct drm_i915_private *dev_priv =
> +			to_i915(intel_dig_port->base.base.dev);
> +
> +	if (INTEL_GEN(dev_priv) < 9)
> +		return;
> +
> +	/* setting the status bit releases the mutex + keep mutex enabled */
> +	I915_WRITE(intel_dp->aux_ch_mutex_reg, DP_AUX_CH_MUTEX_ENABLE |
> +		   DP_AUX_CH_MUTEX_STATUS);

ditto.

> +}
> +
>  static int
>  intel_dp_aux_ch(struct intel_dp *intel_dp,
>  		const uint8_t *send, int send_bytes,
> @@ -1115,6 +1154,11 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
>  
>  	intel_dp_check_edp(intel_dp);
>  
> +	if (!intel_dp_aux_ch_trylock(intel_dp)) {
> +		ret = -EBUSY;
> +		goto out_locked;
> +	}
> +

BSpec: 4301
I believe you could reduce the lock region following the spec
and limiting it to steps 5-to-10, i.e.
inside the loop below.

>  	/* Try to wait for any previous AUX channel activity */
>  	for (try = 0; try < 3; try++) {
>  		status = I915_READ_NOTRACE(ch_ctl);
> @@ -1244,6 +1288,8 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
>  
>  	ret = recv_bytes;
>  out:
> +	intel_dp_aux_ch_unlock(intel_dp);
> +out_locked:
>  	pm_qos_update_request(&dev_priv->pm_qos, PM_QOS_DEFAULT_VALUE);
>  
>  	if (vdd)
> @@ -1496,6 +1542,10 @@ static void intel_aux_reg_init(struct intel_dp *intel_dp)
>  	intel_dp->aux_ch_ctl_reg = intel_aux_ctl_reg(dev_priv, port);
>  	for (i = 0; i < ARRAY_SIZE(intel_dp->aux_ch_data_reg); i++)
>  		intel_dp->aux_ch_data_reg[i] = intel_aux_data_reg(dev_priv, port, i);
> +	if (INTEL_INFO(dev_priv)->gen >= 9) {
> +		intel_dp->aux_ch_mutex_reg = DP_AUX_CH_MUTEX(port);
> +		I915_WRITE(intel_dp->aux_ch_mutex_reg, DP_AUX_CH_MUTEX_ENABLE);

I don't believe we need an extra enable here. And hopefully with Villes
rework we don't even need to define here but only accessing it directly from
the aux function with intel_dp->aux_ch reference.

> +	}
>  }
>  
>  static void
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 50874f4035cf..0c88657d6066 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1043,6 +1043,7 @@ struct intel_dp {
>  	i915_reg_t output_reg;
>  	i915_reg_t aux_ch_ctl_reg;
>  	i915_reg_t aux_ch_data_reg[5];
> +	i915_reg_t aux_ch_mutex_reg;
>  	uint32_t DP;
>  	int link_rate;
>  	uint8_t lane_count;
> -- 
> 2.16.2
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 1/1] drm/i915: Add and enable DP AUX CH mutex

[Souza, Jose]

On Wed, 2018-02-21 at 12:45 -0800, Rodrigo Vivi wrote:
> On Tue, Feb 20, 2018 at 06:23:47PM -0800, José Roberto de Souza
> wrote:
> > When PSR/PSR2/GTC is enabled hardware can do AUX transactions by it
> > self, so lets use the mutex register that is available in gen9+ to
> > avoid concurrent access by hardware and driver.
> > 
> > Reference: https://01.org/sites/default/files/documentation/intel-g
> > fx-prm-osrc-skl-vol12-display.pdf
> > Page 198 - AUX programming sequence
> > 
> 
> Cc: Ville
> Cc: DK
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_reg.h  |  9 ++++++++
> >  drivers/gpu/drm/i915/intel_dp.c  | 50
> > ++++++++++++++++++++++++++++++++++++++++
> >  drivers/gpu/drm/i915/intel_drv.h |  1 +
> >  3 files changed, 60 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > b/drivers/gpu/drm/i915/i915_reg.h
> > index 1412abcb27d4..a62e3c1badab 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -5318,6 +5318,7 @@ enum {
> >  #define _DPA_AUX_CH_DATA3	(dev_priv-
> > >info.display_mmio_offset + 0x6401c)
> >  #define _DPA_AUX_CH_DATA4	(dev_priv-
> > >info.display_mmio_offset + 0x64020)
> >  #define _DPA_AUX_CH_DATA5	(dev_priv-
> > >info.display_mmio_offset + 0x64024)
> > +#define _DPA_AUX_CH_MUTEX	(dev_priv-
> > >info.display_mmio_offset + 0x6402C)
> >  
> >  #define _DPB_AUX_CH_CTL		(dev_priv-
> > >info.display_mmio_offset + 0x64110)
> >  #define _DPB_AUX_CH_DATA1	(dev_priv-
> > >info.display_mmio_offset + 0x64114)
> > @@ -5325,6 +5326,7 @@ enum {
> >  #define _DPB_AUX_CH_DATA3	(dev_priv-
> > >info.display_mmio_offset + 0x6411c)
> >  #define _DPB_AUX_CH_DATA4	(dev_priv-
> > >info.display_mmio_offset + 0x64120)
> >  #define _DPB_AUX_CH_DATA5	(dev_priv-
> > >info.display_mmio_offset + 0x64124)
> > +#define _DPB_AUX_CH_MUTEX	(dev_priv-
> > >info.display_mmio_offset + 0x6412C)
> >  
> >  #define _DPC_AUX_CH_CTL		(dev_priv-
> > >info.display_mmio_offset + 0x64210)
> >  #define _DPC_AUX_CH_DATA1	(dev_priv-
> > >info.display_mmio_offset + 0x64214)
> > @@ -5332,6 +5334,7 @@ enum {
> >  #define _DPC_AUX_CH_DATA3	(dev_priv-
> > >info.display_mmio_offset + 0x6421c)
> >  #define _DPC_AUX_CH_DATA4	(dev_priv-
> > >info.display_mmio_offset + 0x64220)
> >  #define _DPC_AUX_CH_DATA5	(dev_priv-
> > >info.display_mmio_offset + 0x64224)
> > +#define _DPC_AUX_CH_MUTEX	(dev_priv-
> > >info.display_mmio_offset + 0x6422C)
> >  
> >  #define _DPD_AUX_CH_CTL		(dev_priv-
> > >info.display_mmio_offset + 0x64310)
> >  #define _DPD_AUX_CH_DATA1	(dev_priv-
> > >info.display_mmio_offset + 0x64314)
> > @@ -5339,6 +5342,7 @@ enum {
> >  #define _DPD_AUX_CH_DATA3	(dev_priv-
> > >info.display_mmio_offset + 0x6431c)
> >  #define _DPD_AUX_CH_DATA4	(dev_priv-
> > >info.display_mmio_offset + 0x64320)
> >  #define _DPD_AUX_CH_DATA5	(dev_priv-
> > >info.display_mmio_offset + 0x64324)
> > +#define _DPD_AUX_CH_MUTEX	(dev_priv-
> > >info.display_mmio_offset + 0x6432C)
> >  
> >  #define _DPF_AUX_CH_CTL		(dev_priv-
> > >info.display_mmio_offset + 0x64510)
> >  #define _DPF_AUX_CH_DATA1	(dev_priv-
> > >info.display_mmio_offset + 0x64514)
> > @@ -5346,6 +5350,7 @@ enum {
> >  #define _DPF_AUX_CH_DATA3	(dev_priv-
> > >info.display_mmio_offset + 0x6451c)
> >  #define _DPF_AUX_CH_DATA4	(dev_priv-
> > >info.display_mmio_offset + 0x64520)
> >  #define _DPF_AUX_CH_DATA5	(dev_priv-
> > >info.display_mmio_offset + 0x64524)
> > +#define _DPF_AUX_CH_MUTEX	(dev_priv-
> > >info.display_mmio_offset + 0x6452C)
> >  
> >  #define DP_AUX_CH_CTL(port)	_MMIO_PORT(port,
> > _DPA_AUX_CH_CTL, _DPB_AUX_CH_CTL)
> >  #define DP_AUX_CH_DATA(port, i)	_MMIO(_PORT(port,
> > _DPA_AUX_CH_DATA1, _DPB_AUX_CH_DATA1) + (i) * 4) /* 5 registers */
> > @@ -5378,6 +5383,10 @@ enum {
> >  #define   DP_AUX_CH_CTL_FW_SYNC_PULSE_SKL(c) (((c) - 1) << 5)
> >  #define   DP_AUX_CH_CTL_SYNC_PULSE_SKL(c)   ((c) - 1)
> > 
> 
> 
> I believe _DPA_AUX_CH_MUTEX and _DPB_AUX_CH_MUTEX should be defined
> here along
> with the bits as Jani suggested.
> I don't see any reason to tag that far from the bits and close to CTL
> and DATA.
> 
> > +#define DP_AUX_CH_MUTEX(port)	_MMIO_PORT(port,
> > _DPA_AUX_CH_MUTEX, _DPB_AUX_CH_MUTEX)
> > +#define   DP_AUX_CH_MUTEX_ENABLE		(1 << 31)
> > +#define   DP_AUX_CH_MUTEX_STATUS		(1 << 30)
> > +
> >  /*
> >   * Computing GMCH M and N values for the Display Port link
> >   *
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > b/drivers/gpu/drm/i915/intel_dp.c
> > index f20b25f98e5a..af07563bafba 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -1081,6 +1081,45 @@ static uint32_t
> > intel_dp_get_aux_send_ctl(struct intel_dp *intel_dp,
> >  						aux_clock_divider)
> > ;
> >  }
> >  
> > +static bool intel_dp_aux_ch_trylock(struct intel_dp *intel_dp)
> > +{
> > +	struct intel_digital_port *intel_dig_port =
> > dp_to_dig_port(intel_dp);
> > +	struct drm_i915_private *dev_priv =
> > +			to_i915(intel_dig_port->base.base.dev);
> > +
> > +	if (INTEL_GEN(dev_priv) < 9)
> > +		return true;
> > +
> > +	I915_WRITE(intel_dp->aux_ch_mutex_reg,
> > DP_AUX_CH_MUTEX_ENABLE);
> 
> You might be touching bits. We don't know if HW is using the reserved
> bits or not.
> So RMW |= bit 31 here is a good idea.

As a read in this register request the mutex lock is better avoid any
read that is not meant to request it.

> 
> > +
> > +	/* Spec says that mutex is acquired when status bit is
> > read as unset,
> > +	 * if set it should be read again after 500usec. Here
> > waiting for
> > +	 * 2msec or 4 tries before give up.
> > +	 */
> > +	if (__intel_wait_for_register(dev_priv, intel_dp-
> > >aux_ch_mutex_reg,
> > +				      DP_AUX_CH_MUTEX_STATUS, 0,
> > 500, 2,
> > +				      NULL)) {
> 
> why not intel_wait_for_register?
> that seems cleaner...

intel_wait_for_register() will call __intel_wait_for_register() with
fast_timeout_us set as 2usec and specs says we should wait 500usec.

> 
> > +		DRM_WARN("dp_aux_ch port locked for too long");
> > +		return false;
> > +	}
> > +
> > +	return true;
> > +}
> > +
> > +static void intel_dp_aux_ch_unlock(struct intel_dp *intel_dp)
> > +{
> > +	struct intel_digital_port *intel_dig_port =
> > dp_to_dig_port(intel_dp);
> > +	struct drm_i915_private *dev_priv =
> > +			to_i915(intel_dig_port->base.base.dev);
> > +
> > +	if (INTEL_GEN(dev_priv) < 9)
> > +		return;
> > +
> > +	/* setting the status bit releases the mutex + keep mutex
> > enabled */
> > +	I915_WRITE(intel_dp->aux_ch_mutex_reg,
> > DP_AUX_CH_MUTEX_ENABLE |
> > +		   DP_AUX_CH_MUTEX_STATUS);
> 
> ditto.
> 
> > +}
> > +
> >  static int
> >  intel_dp_aux_ch(struct intel_dp *intel_dp,
> >  		const uint8_t *send, int send_bytes,
> > @@ -1115,6 +1154,11 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
> >  
> >  	intel_dp_check_edp(intel_dp);
> >  
> > +	if (!intel_dp_aux_ch_trylock(intel_dp)) {
> > +		ret = -EBUSY;
> > +		goto out_locked;
> > +	}
> > +
> 
> BSpec: 4301
> I believe you could reduce the lock region following the spec
> and limiting it to steps 5-to-10, i.e.
> inside the loop below.

We loop trying to complete the transaction but we copy data received
out of the loop and that is step 9, so the lock must be held until we
copy the data from intel_dp->aux_ch_data_reg[] to recv.

"
	for (i = 0; i < recv_bytes; i += 4)
		intel_dp_unpack_aux(I915_READ(intel_dp->aux_ch_data_reg[i >>
2]), recv + i, recv_bytes - i);

	ret = recv_bytes;
out:
	intel_dp_aux_ch_unlock(intel_dp);
"

> 
> >  	/* Try to wait for any previous AUX channel activity */
> >  	for (try = 0; try < 3; try++) {
> >  		status = I915_READ_NOTRACE(ch_ctl);
> > @@ -1244,6 +1288,8 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
> >  
> >  	ret = recv_bytes;
> >  out:
> > +	intel_dp_aux_ch_unlock(intel_dp);
> > +out_locked:
> >  	pm_qos_update_request(&dev_priv->pm_qos,
> > PM_QOS_DEFAULT_VALUE);
> >  
> >  	if (vdd)
> > @@ -1496,6 +1542,10 @@ static void intel_aux_reg_init(struct
> > intel_dp *intel_dp)
> >  	intel_dp->aux_ch_ctl_reg = intel_aux_ctl_reg(dev_priv,
> > port);
> >  	for (i = 0; i < ARRAY_SIZE(intel_dp->aux_ch_data_reg);
> > i++)
> >  		intel_dp->aux_ch_data_reg[i] =
> > intel_aux_data_reg(dev_priv, port, i);
> > +	if (INTEL_INFO(dev_priv)->gen >= 9) {
> > +		intel_dp->aux_ch_mutex_reg =
> > DP_AUX_CH_MUTEX(port);
> > +		I915_WRITE(intel_dp->aux_ch_mutex_reg,
> > DP_AUX_CH_MUTEX_ENABLE);
> 
> I don't believe we need an extra enable here. And hopefully with
> Villes
> rework we don't even need to define here but only accessing it
> directly from
> the aux function with intel_dp->aux_ch reference.

What if hardware wants to do some aux transaction before we do our
first transaction(and enabling the mutex)? The mutex will not be
enabled so hardware will not lock it.

> 
> > +	}
> >  }
> >  
> >  static void
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > b/drivers/gpu/drm/i915/intel_drv.h
> > index 50874f4035cf..0c88657d6066 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1043,6 +1043,7 @@ struct intel_dp {
> >  	i915_reg_t output_reg;
> >  	i915_reg_t aux_ch_ctl_reg;
> >  	i915_reg_t aux_ch_data_reg[5];
> > +	i915_reg_t aux_ch_mutex_reg;
> >  	uint32_t DP;
> >  	int link_rate;
> >  	uint8_t lane_count;
> > -- 
> > 2.16.2
> > 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 1/1] drm/i915: Add and enable DP AUX CH mutex

[Rodrigo Vivi]

On Wed, Feb 21, 2018 at 01:12:08PM -0800, Souza, Jose wrote:
> On Wed, 2018-02-21 at 12:45 -0800, Rodrigo Vivi wrote:
> > On Tue, Feb 20, 2018 at 06:23:47PM -0800, José Roberto de Souza
> > wrote:
> > > When PSR/PSR2/GTC is enabled hardware can do AUX transactions by it
> > > self, so lets use the mutex register that is available in gen9+ to
> > > avoid concurrent access by hardware and driver.
> > > 
> > > Reference: https://01.org/sites/default/files/documentation/intel-g
> > > fx-prm-osrc-skl-vol12-display.pdf
> > > Page 198 - AUX programming sequence
> > > 
> > 
> > Cc: Ville
> > Cc: DK
> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_reg.h  |  9 ++++++++
> > >  drivers/gpu/drm/i915/intel_dp.c  | 50
> > > ++++++++++++++++++++++++++++++++++++++++
> > >  drivers/gpu/drm/i915/intel_drv.h |  1 +
> > >  3 files changed, 60 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > > b/drivers/gpu/drm/i915/i915_reg.h
> > > index 1412abcb27d4..a62e3c1badab 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -5318,6 +5318,7 @@ enum {
> > >  #define _DPA_AUX_CH_DATA3	(dev_priv-
> > > >info.display_mmio_offset + 0x6401c)
> > >  #define _DPA_AUX_CH_DATA4	(dev_priv-
> > > >info.display_mmio_offset + 0x64020)
> > >  #define _DPA_AUX_CH_DATA5	(dev_priv-
> > > >info.display_mmio_offset + 0x64024)
> > > +#define _DPA_AUX_CH_MUTEX	(dev_priv-
> > > >info.display_mmio_offset + 0x6402C)
> > >  
> > >  #define _DPB_AUX_CH_CTL		(dev_priv-
> > > >info.display_mmio_offset + 0x64110)
> > >  #define _DPB_AUX_CH_DATA1	(dev_priv-
> > > >info.display_mmio_offset + 0x64114)
> > > @@ -5325,6 +5326,7 @@ enum {
> > >  #define _DPB_AUX_CH_DATA3	(dev_priv-
> > > >info.display_mmio_offset + 0x6411c)
> > >  #define _DPB_AUX_CH_DATA4	(dev_priv-
> > > >info.display_mmio_offset + 0x64120)
> > >  #define _DPB_AUX_CH_DATA5	(dev_priv-
> > > >info.display_mmio_offset + 0x64124)
> > > +#define _DPB_AUX_CH_MUTEX	(dev_priv-
> > > >info.display_mmio_offset + 0x6412C)
> > >  
> > >  #define _DPC_AUX_CH_CTL		(dev_priv-
> > > >info.display_mmio_offset + 0x64210)
> > >  #define _DPC_AUX_CH_DATA1	(dev_priv-
> > > >info.display_mmio_offset + 0x64214)
> > > @@ -5332,6 +5334,7 @@ enum {
> > >  #define _DPC_AUX_CH_DATA3	(dev_priv-
> > > >info.display_mmio_offset + 0x6421c)
> > >  #define _DPC_AUX_CH_DATA4	(dev_priv-
> > > >info.display_mmio_offset + 0x64220)
> > >  #define _DPC_AUX_CH_DATA5	(dev_priv-
> > > >info.display_mmio_offset + 0x64224)
> > > +#define _DPC_AUX_CH_MUTEX	(dev_priv-
> > > >info.display_mmio_offset + 0x6422C)
> > >  
> > >  #define _DPD_AUX_CH_CTL		(dev_priv-
> > > >info.display_mmio_offset + 0x64310)
> > >  #define _DPD_AUX_CH_DATA1	(dev_priv-
> > > >info.display_mmio_offset + 0x64314)
> > > @@ -5339,6 +5342,7 @@ enum {
> > >  #define _DPD_AUX_CH_DATA3	(dev_priv-
> > > >info.display_mmio_offset + 0x6431c)
> > >  #define _DPD_AUX_CH_DATA4	(dev_priv-
> > > >info.display_mmio_offset + 0x64320)
> > >  #define _DPD_AUX_CH_DATA5	(dev_priv-
> > > >info.display_mmio_offset + 0x64324)
> > > +#define _DPD_AUX_CH_MUTEX	(dev_priv-
> > > >info.display_mmio_offset + 0x6432C)
> > >  
> > >  #define _DPF_AUX_CH_CTL		(dev_priv-
> > > >info.display_mmio_offset + 0x64510)
> > >  #define _DPF_AUX_CH_DATA1	(dev_priv-
> > > >info.display_mmio_offset + 0x64514)
> > > @@ -5346,6 +5350,7 @@ enum {
> > >  #define _DPF_AUX_CH_DATA3	(dev_priv-
> > > >info.display_mmio_offset + 0x6451c)
> > >  #define _DPF_AUX_CH_DATA4	(dev_priv-
> > > >info.display_mmio_offset + 0x64520)
> > >  #define _DPF_AUX_CH_DATA5	(dev_priv-
> > > >info.display_mmio_offset + 0x64524)
> > > +#define _DPF_AUX_CH_MUTEX	(dev_priv-
> > > >info.display_mmio_offset + 0x6452C)
> > >  
> > >  #define DP_AUX_CH_CTL(port)	_MMIO_PORT(port,
> > > _DPA_AUX_CH_CTL, _DPB_AUX_CH_CTL)
> > >  #define DP_AUX_CH_DATA(port, i)	_MMIO(_PORT(port,
> > > _DPA_AUX_CH_DATA1, _DPB_AUX_CH_DATA1) + (i) * 4) /* 5 registers */
> > > @@ -5378,6 +5383,10 @@ enum {
> > >  #define   DP_AUX_CH_CTL_FW_SYNC_PULSE_SKL(c) (((c) - 1) << 5)
> > >  #define   DP_AUX_CH_CTL_SYNC_PULSE_SKL(c)   ((c) - 1)
> > > 
> > 
> > 
> > I believe _DPA_AUX_CH_MUTEX and _DPB_AUX_CH_MUTEX should be defined
> > here along
> > with the bits as Jani suggested.
> > I don't see any reason to tag that far from the bits and close to CTL
> > and DATA.
> > 
> > > +#define DP_AUX_CH_MUTEX(port)	_MMIO_PORT(port,
> > > _DPA_AUX_CH_MUTEX, _DPB_AUX_CH_MUTEX)
> > > +#define   DP_AUX_CH_MUTEX_ENABLE		(1 << 31)
> > > +#define   DP_AUX_CH_MUTEX_STATUS		(1 << 30)
> > > +
> > >  /*
> > >   * Computing GMCH M and N values for the Display Port link
> > >   *
> > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > b/drivers/gpu/drm/i915/intel_dp.c
> > > index f20b25f98e5a..af07563bafba 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > @@ -1081,6 +1081,45 @@ static uint32_t
> > > intel_dp_get_aux_send_ctl(struct intel_dp *intel_dp,
> > >  						aux_clock_divider)
> > > ;
> > >  }
> > >  
> > > +static bool intel_dp_aux_ch_trylock(struct intel_dp *intel_dp)
> > > +{
> > > +	struct intel_digital_port *intel_dig_port =
> > > dp_to_dig_port(intel_dp);
> > > +	struct drm_i915_private *dev_priv =
> > > +			to_i915(intel_dig_port->base.base.dev);
> > > +
> > > +	if (INTEL_GEN(dev_priv) < 9)
> > > +		return true;
> > > +
> > > +	I915_WRITE(intel_dp->aux_ch_mutex_reg,
> > > DP_AUX_CH_MUTEX_ENABLE);
> > 
> > You might be touching bits. We don't know if HW is using the reserved
> > bits or not.
> > So RMW |= bit 31 here is a good idea.
> 
> As a read in this register request the mutex lock is better avoid any
> read that is not meant to request it.

ok... I accept the fact that read that is locking
so you are right here.

> 
> > 
> > > +
> > > +	/* Spec says that mutex is acquired when status bit is
> > > read as unset,
> > > +	 * if set it should be read again after 500usec. Here
> > > waiting for
> > > +	 * 2msec or 4 tries before give up.
> > > +	 */
> > > +	if (__intel_wait_for_register(dev_priv, intel_dp-
> > > >aux_ch_mutex_reg,
> > > +				      DP_AUX_CH_MUTEX_STATUS, 0,
> > > 500, 2,
> > > +				      NULL)) {
> > 
> > why not intel_wait_for_register?
> > that seems cleaner...
> 
> intel_wait_for_register() will call __intel_wait_for_register() with
> fast_timeout_us set as 2usec and specs says we should wait 500usec.

So, I've just noticed that __intel_wait_for_register spreaded to intel_hdcp.
Ideally if we have __ prefix this shouldn't be called for more than 1 place.

So it seems that we have more cases hence a rework should be done here.

But also the fast case is the atomic initial case... it is the first read.
Only if it fails it will consider the slow case. That's exactly what spec tells

Maybe what we want here is a intel_wait_for_register_us to have a slow value
based in us rather than ms?

I'm not fully sure here.

> 
> > 
> > > +		DRM_WARN("dp_aux_ch port locked for too long");
> > > +		return false;
> > > +	}
> > > +
> > > +	return true;
> > > +}
> > > +
> > > +static void intel_dp_aux_ch_unlock(struct intel_dp *intel_dp)
> > > +{
> > > +	struct intel_digital_port *intel_dig_port =
> > > dp_to_dig_port(intel_dp);
> > > +	struct drm_i915_private *dev_priv =
> > > +			to_i915(intel_dig_port->base.base.dev);
> > > +
> > > +	if (INTEL_GEN(dev_priv) < 9)
> > > +		return;
> > > +
> > > +	/* setting the status bit releases the mutex + keep mutex
> > > enabled */
> > > +	I915_WRITE(intel_dp->aux_ch_mutex_reg,
> > > DP_AUX_CH_MUTEX_ENABLE |
> > > +		   DP_AUX_CH_MUTEX_STATUS);
> > 
> > ditto.
> > 
> > > +}
> > > +
> > >  static int
> > >  intel_dp_aux_ch(struct intel_dp *intel_dp,
> > >  		const uint8_t *send, int send_bytes,
> > > @@ -1115,6 +1154,11 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
> > >  
> > >  	intel_dp_check_edp(intel_dp);
> > >  
> > > +	if (!intel_dp_aux_ch_trylock(intel_dp)) {
> > > +		ret = -EBUSY;
> > > +		goto out_locked;
> > > +	}
> > > +
> > 
> > BSpec: 4301
> > I believe you could reduce the lock region following the spec
> > and limiting it to steps 5-to-10, i.e.
> > inside the loop below.
> 
> We loop trying to complete the transaction but we copy data received
> out of the loop and that is step 9, so the lock must be held until we
> copy the data from intel_dp->aux_ch_data_reg[] to recv.
> 
> "
> 	for (i = 0; i < recv_bytes; i += 4)
> 		intel_dp_unpack_aux(I915_READ(intel_dp->aux_ch_data_reg[i >>
> 2]), recv + i, recv_bytes - i);
> 
> 	ret = recv_bytes;
> out:
> 	intel_dp_aux_ch_unlock(intel_dp);
> "

hm... ok... this is the actually part 9 that needs to be inside
the locked area indeed. I missed that.

> 
> > 
> > >  	/* Try to wait for any previous AUX channel activity */
> > >  	for (try = 0; try < 3; try++) {
> > >  		status = I915_READ_NOTRACE(ch_ctl);
> > > @@ -1244,6 +1288,8 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
> > >  
> > >  	ret = recv_bytes;
> > >  out:
> > > +	intel_dp_aux_ch_unlock(intel_dp);
> > > +out_locked:
> > >  	pm_qos_update_request(&dev_priv->pm_qos,
> > > PM_QOS_DEFAULT_VALUE);
> > >  
> > >  	if (vdd)
> > > @@ -1496,6 +1542,10 @@ static void intel_aux_reg_init(struct
> > > intel_dp *intel_dp)
> > >  	intel_dp->aux_ch_ctl_reg = intel_aux_ctl_reg(dev_priv,
> > > port);
> > >  	for (i = 0; i < ARRAY_SIZE(intel_dp->aux_ch_data_reg);
> > > i++)
> > >  		intel_dp->aux_ch_data_reg[i] =
> > > intel_aux_data_reg(dev_priv, port, i);
> > > +	if (INTEL_INFO(dev_priv)->gen >= 9) {
> > > +		intel_dp->aux_ch_mutex_reg =
> > > DP_AUX_CH_MUTEX(port);
> > > +		I915_WRITE(intel_dp->aux_ch_mutex_reg,
> > > DP_AUX_CH_MUTEX_ENABLE);
> > 
> > I don't believe we need an extra enable here. And hopefully with
> > Villes
> > rework we don't even need to define here but only accessing it
> > directly from
> > the aux function with intel_dp->aux_ch reference.
> 
> What if hardware wants to do some aux transaction before we do our
> first transaction(and enabling the mutex)? The mutex will not be
> enabled so hardware will not lock it.
> 
> > 
> > > +	}
> > >  }
> > >  
> > >  static void
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > b/drivers/gpu/drm/i915/intel_drv.h
> > > index 50874f4035cf..0c88657d6066 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -1043,6 +1043,7 @@ struct intel_dp {
> > >  	i915_reg_t output_reg;
> > >  	i915_reg_t aux_ch_ctl_reg;
> > >  	i915_reg_t aux_ch_data_reg[5];
> > > +	i915_reg_t aux_ch_mutex_reg;
> > >  	uint32_t DP;
> > >  	int link_rate;
> > >  	uint8_t lane_count;
> > > -- 
> > > 2.16.2
> > > 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 1/1] drm/i915: Add and enable DP AUX CH mutex

[Ville Syrjälä]

On Tue, Feb 20, 2018 at 06:23:47PM -0800, José Roberto de Souza wrote:
> When PSR/PSR2/GTC is enabled hardware can do AUX transactions by it
> self, so lets use the mutex register that is available in gen9+ to
> avoid concurrent access by hardware and driver.
> 
> Reference: https://01.org/sites/default/files/documentation/intel-gfx-prm-osrc-skl-vol12-display.pdf
> Page 198 - AUX programming sequence

BTW since HSW/BDW don't have the mutex we should really be disabling PSR
around aux ch usage on those platforms. Not sure how big of a locking
inversion exercise it would be to make that happen though.

> 
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h  |  9 ++++++++
>  drivers/gpu/drm/i915/intel_dp.c  | 50 ++++++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h |  1 +
>  3 files changed, 60 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 1412abcb27d4..a62e3c1badab 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -5318,6 +5318,7 @@ enum {
>  #define _DPA_AUX_CH_DATA3	(dev_priv->info.display_mmio_offset + 0x6401c)
>  #define _DPA_AUX_CH_DATA4	(dev_priv->info.display_mmio_offset + 0x64020)
>  #define _DPA_AUX_CH_DATA5	(dev_priv->info.display_mmio_offset + 0x64024)
> +#define _DPA_AUX_CH_MUTEX	(dev_priv->info.display_mmio_offset + 0x6402C)
>  
>  #define _DPB_AUX_CH_CTL		(dev_priv->info.display_mmio_offset + 0x64110)
>  #define _DPB_AUX_CH_DATA1	(dev_priv->info.display_mmio_offset + 0x64114)
> @@ -5325,6 +5326,7 @@ enum {
>  #define _DPB_AUX_CH_DATA3	(dev_priv->info.display_mmio_offset + 0x6411c)
>  #define _DPB_AUX_CH_DATA4	(dev_priv->info.display_mmio_offset + 0x64120)
>  #define _DPB_AUX_CH_DATA5	(dev_priv->info.display_mmio_offset + 0x64124)
> +#define _DPB_AUX_CH_MUTEX	(dev_priv->info.display_mmio_offset + 0x6412C)
>  
>  #define _DPC_AUX_CH_CTL		(dev_priv->info.display_mmio_offset + 0x64210)
>  #define _DPC_AUX_CH_DATA1	(dev_priv->info.display_mmio_offset + 0x64214)
> @@ -5332,6 +5334,7 @@ enum {
>  #define _DPC_AUX_CH_DATA3	(dev_priv->info.display_mmio_offset + 0x6421c)
>  #define _DPC_AUX_CH_DATA4	(dev_priv->info.display_mmio_offset + 0x64220)
>  #define _DPC_AUX_CH_DATA5	(dev_priv->info.display_mmio_offset + 0x64224)
> +#define _DPC_AUX_CH_MUTEX	(dev_priv->info.display_mmio_offset + 0x6422C)
>  
>  #define _DPD_AUX_CH_CTL		(dev_priv->info.display_mmio_offset + 0x64310)
>  #define _DPD_AUX_CH_DATA1	(dev_priv->info.display_mmio_offset + 0x64314)
> @@ -5339,6 +5342,7 @@ enum {
>  #define _DPD_AUX_CH_DATA3	(dev_priv->info.display_mmio_offset + 0x6431c)
>  #define _DPD_AUX_CH_DATA4	(dev_priv->info.display_mmio_offset + 0x64320)
>  #define _DPD_AUX_CH_DATA5	(dev_priv->info.display_mmio_offset + 0x64324)
> +#define _DPD_AUX_CH_MUTEX	(dev_priv->info.display_mmio_offset + 0x6432C)
>  
>  #define _DPF_AUX_CH_CTL		(dev_priv->info.display_mmio_offset + 0x64510)
>  #define _DPF_AUX_CH_DATA1	(dev_priv->info.display_mmio_offset + 0x64514)
> @@ -5346,6 +5350,7 @@ enum {
>  #define _DPF_AUX_CH_DATA3	(dev_priv->info.display_mmio_offset + 0x6451c)
>  #define _DPF_AUX_CH_DATA4	(dev_priv->info.display_mmio_offset + 0x64520)
>  #define _DPF_AUX_CH_DATA5	(dev_priv->info.display_mmio_offset + 0x64524)
> +#define _DPF_AUX_CH_MUTEX	(dev_priv->info.display_mmio_offset + 0x6452C)
>  
>  #define DP_AUX_CH_CTL(port)	_MMIO_PORT(port, _DPA_AUX_CH_CTL, _DPB_AUX_CH_CTL)
>  #define DP_AUX_CH_DATA(port, i)	_MMIO(_PORT(port, _DPA_AUX_CH_DATA1, _DPB_AUX_CH_DATA1) + (i) * 4) /* 5 registers */
> @@ -5378,6 +5383,10 @@ enum {
>  #define   DP_AUX_CH_CTL_FW_SYNC_PULSE_SKL(c) (((c) - 1) << 5)
>  #define   DP_AUX_CH_CTL_SYNC_PULSE_SKL(c)   ((c) - 1)
>  
> +#define DP_AUX_CH_MUTEX(port)	_MMIO_PORT(port, _DPA_AUX_CH_MUTEX, _DPB_AUX_CH_MUTEX)
> +#define   DP_AUX_CH_MUTEX_ENABLE		(1 << 31)
> +#define   DP_AUX_CH_MUTEX_STATUS		(1 << 30)
> +
>  /*
>   * Computing GMCH M and N values for the Display Port link
>   *
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index f20b25f98e5a..af07563bafba 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1081,6 +1081,45 @@ static uint32_t intel_dp_get_aux_send_ctl(struct intel_dp *intel_dp,
>  						aux_clock_divider);
>  }
>  
> +static bool intel_dp_aux_ch_trylock(struct intel_dp *intel_dp)
> +{
> +	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> +	struct drm_i915_private *dev_priv =
> +			to_i915(intel_dig_port->base.base.dev);
> +
> +	if (INTEL_GEN(dev_priv) < 9)
> +		return true;
> +
> +	I915_WRITE(intel_dp->aux_ch_mutex_reg, DP_AUX_CH_MUTEX_ENABLE);
> +
> +	/* Spec says that mutex is acquired when status bit is read as unset,
> +	 * if set it should be read again after 500usec. Here waiting for
> +	 * 2msec or 4 tries before give up.
> +	 */
> +	if (__intel_wait_for_register(dev_priv, intel_dp->aux_ch_mutex_reg,
> +				      DP_AUX_CH_MUTEX_STATUS, 0, 500, 2,
> +				      NULL)) {
> +		DRM_WARN("dp_aux_ch port locked for too long");
> +		return false;
> +	}
> +
> +	return true;
> +}
> +
> +static void intel_dp_aux_ch_unlock(struct intel_dp *intel_dp)
> +{
> +	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> +	struct drm_i915_private *dev_priv =
> +			to_i915(intel_dig_port->base.base.dev);
> +
> +	if (INTEL_GEN(dev_priv) < 9)
> +		return;
> +
> +	/* setting the status bit releases the mutex + keep mutex enabled */
> +	I915_WRITE(intel_dp->aux_ch_mutex_reg, DP_AUX_CH_MUTEX_ENABLE |
> +		   DP_AUX_CH_MUTEX_STATUS);
> +}
> +
>  static int
>  intel_dp_aux_ch(struct intel_dp *intel_dp,
>  		const uint8_t *send, int send_bytes,
> @@ -1115,6 +1154,11 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
>  
>  	intel_dp_check_edp(intel_dp);
>  
> +	if (!intel_dp_aux_ch_trylock(intel_dp)) {
> +		ret = -EBUSY;
> +		goto out_locked;
> +	}
> +
>  	/* Try to wait for any previous AUX channel activity */
>  	for (try = 0; try < 3; try++) {
>  		status = I915_READ_NOTRACE(ch_ctl);
> @@ -1244,6 +1288,8 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
>  
>  	ret = recv_bytes;
>  out:
> +	intel_dp_aux_ch_unlock(intel_dp);
> +out_locked:
>  	pm_qos_update_request(&dev_priv->pm_qos, PM_QOS_DEFAULT_VALUE);
>  
>  	if (vdd)
> @@ -1496,6 +1542,10 @@ static void intel_aux_reg_init(struct intel_dp *intel_dp)
>  	intel_dp->aux_ch_ctl_reg = intel_aux_ctl_reg(dev_priv, port);
>  	for (i = 0; i < ARRAY_SIZE(intel_dp->aux_ch_data_reg); i++)
>  		intel_dp->aux_ch_data_reg[i] = intel_aux_data_reg(dev_priv, port, i);
> +	if (INTEL_INFO(dev_priv)->gen >= 9) {
> +		intel_dp->aux_ch_mutex_reg = DP_AUX_CH_MUTEX(port);
> +		I915_WRITE(intel_dp->aux_ch_mutex_reg, DP_AUX_CH_MUTEX_ENABLE);
> +	}
>  }
>  
>  static void
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 50874f4035cf..0c88657d6066 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1043,6 +1043,7 @@ struct intel_dp {
>  	i915_reg_t output_reg;
>  	i915_reg_t aux_ch_ctl_reg;
>  	i915_reg_t aux_ch_data_reg[5];
> +	i915_reg_t aux_ch_mutex_reg;
>  	uint32_t DP;
>  	int link_rate;
>  	uint8_t lane_count;
> -- 
> 2.16.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 1/1] drm/i915: Add and enable DP AUX CH mutex

[Rodrigo Vivi]

On Thu, Feb 22, 2018 at 08:59:49PM +0200, Ville Syrjälä wrote:
> On Tue, Feb 20, 2018 at 06:23:47PM -0800, José Roberto de Souza wrote:
> > When PSR/PSR2/GTC is enabled hardware can do AUX transactions by it
> > self, so lets use the mutex register that is available in gen9+ to
> > avoid concurrent access by hardware and driver.
> > 
> > Reference: https://01.org/sites/default/files/documentation/intel-gfx-prm-osrc-skl-vol12-display.pdf
> > Page 198 - AUX programming sequence
> 
> BTW since HSW/BDW don't have the mutex we should really be disabling PSR
> around aux ch usage on those platforms. Not sure how big of a locking
> inversion exercise it would be to make that happen though.

True. But I believe that can be handled differently.
Probably with Aux power domains blocking PSR off "domain".
Something similar to DC off.

I actually had patches for that lost somewhere... :(

> 
> > 
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_reg.h  |  9 ++++++++
> >  drivers/gpu/drm/i915/intel_dp.c  | 50 ++++++++++++++++++++++++++++++++++++++++
> >  drivers/gpu/drm/i915/intel_drv.h |  1 +
> >  3 files changed, 60 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index 1412abcb27d4..a62e3c1badab 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -5318,6 +5318,7 @@ enum {
> >  #define _DPA_AUX_CH_DATA3	(dev_priv->info.display_mmio_offset + 0x6401c)
> >  #define _DPA_AUX_CH_DATA4	(dev_priv->info.display_mmio_offset + 0x64020)
> >  #define _DPA_AUX_CH_DATA5	(dev_priv->info.display_mmio_offset + 0x64024)
> > +#define _DPA_AUX_CH_MUTEX	(dev_priv->info.display_mmio_offset + 0x6402C)
> >  
> >  #define _DPB_AUX_CH_CTL		(dev_priv->info.display_mmio_offset + 0x64110)
> >  #define _DPB_AUX_CH_DATA1	(dev_priv->info.display_mmio_offset + 0x64114)
> > @@ -5325,6 +5326,7 @@ enum {
> >  #define _DPB_AUX_CH_DATA3	(dev_priv->info.display_mmio_offset + 0x6411c)
> >  #define _DPB_AUX_CH_DATA4	(dev_priv->info.display_mmio_offset + 0x64120)
> >  #define _DPB_AUX_CH_DATA5	(dev_priv->info.display_mmio_offset + 0x64124)
> > +#define _DPB_AUX_CH_MUTEX	(dev_priv->info.display_mmio_offset + 0x6412C)
> >  
> >  #define _DPC_AUX_CH_CTL		(dev_priv->info.display_mmio_offset + 0x64210)
> >  #define _DPC_AUX_CH_DATA1	(dev_priv->info.display_mmio_offset + 0x64214)
> > @@ -5332,6 +5334,7 @@ enum {
> >  #define _DPC_AUX_CH_DATA3	(dev_priv->info.display_mmio_offset + 0x6421c)
> >  #define _DPC_AUX_CH_DATA4	(dev_priv->info.display_mmio_offset + 0x64220)
> >  #define _DPC_AUX_CH_DATA5	(dev_priv->info.display_mmio_offset + 0x64224)
> > +#define _DPC_AUX_CH_MUTEX	(dev_priv->info.display_mmio_offset + 0x6422C)
> >  
> >  #define _DPD_AUX_CH_CTL		(dev_priv->info.display_mmio_offset + 0x64310)
> >  #define _DPD_AUX_CH_DATA1	(dev_priv->info.display_mmio_offset + 0x64314)
> > @@ -5339,6 +5342,7 @@ enum {
> >  #define _DPD_AUX_CH_DATA3	(dev_priv->info.display_mmio_offset + 0x6431c)
> >  #define _DPD_AUX_CH_DATA4	(dev_priv->info.display_mmio_offset + 0x64320)
> >  #define _DPD_AUX_CH_DATA5	(dev_priv->info.display_mmio_offset + 0x64324)
> > +#define _DPD_AUX_CH_MUTEX	(dev_priv->info.display_mmio_offset + 0x6432C)
> >  
> >  #define _DPF_AUX_CH_CTL		(dev_priv->info.display_mmio_offset + 0x64510)
> >  #define _DPF_AUX_CH_DATA1	(dev_priv->info.display_mmio_offset + 0x64514)
> > @@ -5346,6 +5350,7 @@ enum {
> >  #define _DPF_AUX_CH_DATA3	(dev_priv->info.display_mmio_offset + 0x6451c)
> >  #define _DPF_AUX_CH_DATA4	(dev_priv->info.display_mmio_offset + 0x64520)
> >  #define _DPF_AUX_CH_DATA5	(dev_priv->info.display_mmio_offset + 0x64524)
> > +#define _DPF_AUX_CH_MUTEX	(dev_priv->info.display_mmio_offset + 0x6452C)
> >  
> >  #define DP_AUX_CH_CTL(port)	_MMIO_PORT(port, _DPA_AUX_CH_CTL, _DPB_AUX_CH_CTL)
> >  #define DP_AUX_CH_DATA(port, i)	_MMIO(_PORT(port, _DPA_AUX_CH_DATA1, _DPB_AUX_CH_DATA1) + (i) * 4) /* 5 registers */
> > @@ -5378,6 +5383,10 @@ enum {
> >  #define   DP_AUX_CH_CTL_FW_SYNC_PULSE_SKL(c) (((c) - 1) << 5)
> >  #define   DP_AUX_CH_CTL_SYNC_PULSE_SKL(c)   ((c) - 1)
> >  
> > +#define DP_AUX_CH_MUTEX(port)	_MMIO_PORT(port, _DPA_AUX_CH_MUTEX, _DPB_AUX_CH_MUTEX)
> > +#define   DP_AUX_CH_MUTEX_ENABLE		(1 << 31)
> > +#define   DP_AUX_CH_MUTEX_STATUS		(1 << 30)
> > +
> >  /*
> >   * Computing GMCH M and N values for the Display Port link
> >   *
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index f20b25f98e5a..af07563bafba 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -1081,6 +1081,45 @@ static uint32_t intel_dp_get_aux_send_ctl(struct intel_dp *intel_dp,
> >  						aux_clock_divider);
> >  }
> >  
> > +static bool intel_dp_aux_ch_trylock(struct intel_dp *intel_dp)
> > +{
> > +	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> > +	struct drm_i915_private *dev_priv =
> > +			to_i915(intel_dig_port->base.base.dev);
> > +
> > +	if (INTEL_GEN(dev_priv) < 9)
> > +		return true;
> > +
> > +	I915_WRITE(intel_dp->aux_ch_mutex_reg, DP_AUX_CH_MUTEX_ENABLE);
> > +
> > +	/* Spec says that mutex is acquired when status bit is read as unset,
> > +	 * if set it should be read again after 500usec. Here waiting for
> > +	 * 2msec or 4 tries before give up.
> > +	 */
> > +	if (__intel_wait_for_register(dev_priv, intel_dp->aux_ch_mutex_reg,
> > +				      DP_AUX_CH_MUTEX_STATUS, 0, 500, 2,
> > +				      NULL)) {
> > +		DRM_WARN("dp_aux_ch port locked for too long");
> > +		return false;
> > +	}
> > +
> > +	return true;
> > +}
> > +
> > +static void intel_dp_aux_ch_unlock(struct intel_dp *intel_dp)
> > +{
> > +	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> > +	struct drm_i915_private *dev_priv =
> > +			to_i915(intel_dig_port->base.base.dev);
> > +
> > +	if (INTEL_GEN(dev_priv) < 9)
> > +		return;
> > +
> > +	/* setting the status bit releases the mutex + keep mutex enabled */
> > +	I915_WRITE(intel_dp->aux_ch_mutex_reg, DP_AUX_CH_MUTEX_ENABLE |
> > +		   DP_AUX_CH_MUTEX_STATUS);
> > +}
> > +
> >  static int
> >  intel_dp_aux_ch(struct intel_dp *intel_dp,
> >  		const uint8_t *send, int send_bytes,
> > @@ -1115,6 +1154,11 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
> >  
> >  	intel_dp_check_edp(intel_dp);
> >  
> > +	if (!intel_dp_aux_ch_trylock(intel_dp)) {
> > +		ret = -EBUSY;
> > +		goto out_locked;
> > +	}
> > +
> >  	/* Try to wait for any previous AUX channel activity */
> >  	for (try = 0; try < 3; try++) {
> >  		status = I915_READ_NOTRACE(ch_ctl);
> > @@ -1244,6 +1288,8 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
> >  
> >  	ret = recv_bytes;
> >  out:
> > +	intel_dp_aux_ch_unlock(intel_dp);
> > +out_locked:
> >  	pm_qos_update_request(&dev_priv->pm_qos, PM_QOS_DEFAULT_VALUE);
> >  
> >  	if (vdd)
> > @@ -1496,6 +1542,10 @@ static void intel_aux_reg_init(struct intel_dp *intel_dp)
> >  	intel_dp->aux_ch_ctl_reg = intel_aux_ctl_reg(dev_priv, port);
> >  	for (i = 0; i < ARRAY_SIZE(intel_dp->aux_ch_data_reg); i++)
> >  		intel_dp->aux_ch_data_reg[i] = intel_aux_data_reg(dev_priv, port, i);
> > +	if (INTEL_INFO(dev_priv)->gen >= 9) {
> > +		intel_dp->aux_ch_mutex_reg = DP_AUX_CH_MUTEX(port);
> > +		I915_WRITE(intel_dp->aux_ch_mutex_reg, DP_AUX_CH_MUTEX_ENABLE);
> > +	}
> >  }
> >  
> >  static void
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 50874f4035cf..0c88657d6066 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1043,6 +1043,7 @@ struct intel_dp {
> >  	i915_reg_t output_reg;
> >  	i915_reg_t aux_ch_ctl_reg;
> >  	i915_reg_t aux_ch_data_reg[5];
> > +	i915_reg_t aux_ch_mutex_reg;
> >  	uint32_t DP;
> >  	int link_rate;
> >  	uint8_t lane_count;
> > -- 
> > 2.16.2
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 1/1] drm/i915: Add and enable DP AUX CH mutex

[Pandiyan, Dhinakaran]

On Thu, 2018-02-22 at 11:34 -0800, Rodrigo Vivi wrote:
> On Thu, Feb 22, 2018 at 08:59:49PM +0200, Ville Syrjälä wrote:
> > On Tue, Feb 20, 2018 at 06:23:47PM -0800, José Roberto de Souza wrote:
> > > When PSR/PSR2/GTC is enabled hardware can do AUX transactions by it
> > > self, so lets use the mutex register that is available in gen9+ to
> > > avoid concurrent access by hardware and driver.
> > > 
> > > Reference: https://01.org/sites/default/files/documentation/intel-gfx-prm-osrc-skl-vol12-display.pdf
> > > Page 198 - AUX programming sequence
> > 
> > BTW since HSW/BDW don't have the mutex we should really be disabling PSR
> > around aux ch usage on those platforms. Not sure how big of a locking
> > inversion exercise it would be to make that happen though.
> 
> True. But I believe that can be handled differently.
> Probably with Aux power domains blocking PSR off "domain".
> Something similar to DC off.
> 
> I actually had patches for that lost somewhere... :(
> 

Just a note, the spec explicitly says we should disable PSR around AUX
transactions for BDW (and HSW iirc).

About using the power domain infra, I am not quite sure. The _get() and
_put() idiom makes sense when we want reference counting to handle
multiple consumers that want PSR to be disabled. Does anything else need
to enable/disable PSR?


> > 
> > > 
> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_reg.h  |  9 ++++++++
> > >  drivers/gpu/drm/i915/intel_dp.c  | 50 ++++++++++++++++++++++++++++++++++++++++
> > >  drivers/gpu/drm/i915/intel_drv.h |  1 +
> > >  3 files changed, 60 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > > index 1412abcb27d4..a62e3c1badab 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -5318,6 +5318,7 @@ enum {
> > >  #define _DPA_AUX_CH_DATA3	(dev_priv->info.display_mmio_offset + 0x6401c)
> > >  #define _DPA_AUX_CH_DATA4	(dev_priv->info.display_mmio_offset + 0x64020)
> > >  #define _DPA_AUX_CH_DATA5	(dev_priv->info.display_mmio_offset + 0x64024)
> > > +#define _DPA_AUX_CH_MUTEX	(dev_priv->info.display_mmio_offset + 0x6402C)
> > >  
> > >  #define _DPB_AUX_CH_CTL		(dev_priv->info.display_mmio_offset + 0x64110)
> > >  #define _DPB_AUX_CH_DATA1	(dev_priv->info.display_mmio_offset + 0x64114)
> > > @@ -5325,6 +5326,7 @@ enum {
> > >  #define _DPB_AUX_CH_DATA3	(dev_priv->info.display_mmio_offset + 0x6411c)
> > >  #define _DPB_AUX_CH_DATA4	(dev_priv->info.display_mmio_offset + 0x64120)
> > >  #define _DPB_AUX_CH_DATA5	(dev_priv->info.display_mmio_offset + 0x64124)
> > > +#define _DPB_AUX_CH_MUTEX	(dev_priv->info.display_mmio_offset + 0x6412C)
> > >  
> > >  #define _DPC_AUX_CH_CTL		(dev_priv->info.display_mmio_offset + 0x64210)
> > >  #define _DPC_AUX_CH_DATA1	(dev_priv->info.display_mmio_offset + 0x64214)
> > > @@ -5332,6 +5334,7 @@ enum {
> > >  #define _DPC_AUX_CH_DATA3	(dev_priv->info.display_mmio_offset + 0x6421c)
> > >  #define _DPC_AUX_CH_DATA4	(dev_priv->info.display_mmio_offset + 0x64220)
> > >  #define _DPC_AUX_CH_DATA5	(dev_priv->info.display_mmio_offset + 0x64224)
> > > +#define _DPC_AUX_CH_MUTEX	(dev_priv->info.display_mmio_offset + 0x6422C)
> > >  
> > >  #define _DPD_AUX_CH_CTL		(dev_priv->info.display_mmio_offset + 0x64310)
> > >  #define _DPD_AUX_CH_DATA1	(dev_priv->info.display_mmio_offset + 0x64314)
> > > @@ -5339,6 +5342,7 @@ enum {
> > >  #define _DPD_AUX_CH_DATA3	(dev_priv->info.display_mmio_offset + 0x6431c)
> > >  #define _DPD_AUX_CH_DATA4	(dev_priv->info.display_mmio_offset + 0x64320)
> > >  #define _DPD_AUX_CH_DATA5	(dev_priv->info.display_mmio_offset + 0x64324)
> > > +#define _DPD_AUX_CH_MUTEX	(dev_priv->info.display_mmio_offset + 0x6432C)
> > >  
> > >  #define _DPF_AUX_CH_CTL		(dev_priv->info.display_mmio_offset + 0x64510)
> > >  #define _DPF_AUX_CH_DATA1	(dev_priv->info.display_mmio_offset + 0x64514)
> > > @@ -5346,6 +5350,7 @@ enum {
> > >  #define _DPF_AUX_CH_DATA3	(dev_priv->info.display_mmio_offset + 0x6451c)
> > >  #define _DPF_AUX_CH_DATA4	(dev_priv->info.display_mmio_offset + 0x64520)
> > >  #define _DPF_AUX_CH_DATA5	(dev_priv->info.display_mmio_offset + 0x64524)
> > > +#define _DPF_AUX_CH_MUTEX	(dev_priv->info.display_mmio_offset + 0x6452C)
> > >  
> > >  #define DP_AUX_CH_CTL(port)	_MMIO_PORT(port, _DPA_AUX_CH_CTL, _DPB_AUX_CH_CTL)
> > >  #define DP_AUX_CH_DATA(port, i)	_MMIO(_PORT(port, _DPA_AUX_CH_DATA1, _DPB_AUX_CH_DATA1) + (i) * 4) /* 5 registers */
> > > @@ -5378,6 +5383,10 @@ enum {
> > >  #define   DP_AUX_CH_CTL_FW_SYNC_PULSE_SKL(c) (((c) - 1) << 5)
> > >  #define   DP_AUX_CH_CTL_SYNC_PULSE_SKL(c)   ((c) - 1)
> > >  
> > > +#define DP_AUX_CH_MUTEX(port)	_MMIO_PORT(port, _DPA_AUX_CH_MUTEX, _DPB_AUX_CH_MUTEX)
> > > +#define   DP_AUX_CH_MUTEX_ENABLE		(1 << 31)
> > > +#define   DP_AUX_CH_MUTEX_STATUS		(1 << 30)
> > > +
> > >  /*
> > >   * Computing GMCH M and N values for the Display Port link
> > >   *
> > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > > index f20b25f98e5a..af07563bafba 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > @@ -1081,6 +1081,45 @@ static uint32_t intel_dp_get_aux_send_ctl(struct intel_dp *intel_dp,
> > >  						aux_clock_divider);
> > >  }
> > >  
> > > +static bool intel_dp_aux_ch_trylock(struct intel_dp *intel_dp)
> > > +{
> > > +	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> > > +	struct drm_i915_private *dev_priv =
> > > +			to_i915(intel_dig_port->base.base.dev);
> > > +
> > > +	if (INTEL_GEN(dev_priv) < 9)
> > > +		return true;
> > > +
> > > +	I915_WRITE(intel_dp->aux_ch_mutex_reg, DP_AUX_CH_MUTEX_ENABLE);
> > > +
> > > +	/* Spec says that mutex is acquired when status bit is read as unset,
> > > +	 * if set it should be read again after 500usec. Here waiting for
> > > +	 * 2msec or 4 tries before give up.
> > > +	 */
> > > +	if (__intel_wait_for_register(dev_priv, intel_dp->aux_ch_mutex_reg,
> > > +				      DP_AUX_CH_MUTEX_STATUS, 0, 500, 2,
> > > +				      NULL)) {
> > > +		DRM_WARN("dp_aux_ch port locked for too long");
> > > +		return false;
> > > +	}
> > > +
> > > +	return true;
> > > +}
> > > +
> > > +static void intel_dp_aux_ch_unlock(struct intel_dp *intel_dp)
> > > +{
> > > +	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> > > +	struct drm_i915_private *dev_priv =
> > > +			to_i915(intel_dig_port->base.base.dev);
> > > +
> > > +	if (INTEL_GEN(dev_priv) < 9)
> > > +		return;
> > > +
> > > +	/* setting the status bit releases the mutex + keep mutex enabled */
> > > +	I915_WRITE(intel_dp->aux_ch_mutex_reg, DP_AUX_CH_MUTEX_ENABLE |
> > > +		   DP_AUX_CH_MUTEX_STATUS);
> > > +}
> > > +
> > >  static int
> > >  intel_dp_aux_ch(struct intel_dp *intel_dp,
> > >  		const uint8_t *send, int send_bytes,
> > > @@ -1115,6 +1154,11 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
> > >  
> > >  	intel_dp_check_edp(intel_dp);
> > >  
> > > +	if (!intel_dp_aux_ch_trylock(intel_dp)) {
> > > +		ret = -EBUSY;
> > > +		goto out_locked;
> > > +	}
> > > +
> > >  	/* Try to wait for any previous AUX channel activity */
> > >  	for (try = 0; try < 3; try++) {
> > >  		status = I915_READ_NOTRACE(ch_ctl);
> > > @@ -1244,6 +1288,8 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
> > >  
> > >  	ret = recv_bytes;
> > >  out:
> > > +	intel_dp_aux_ch_unlock(intel_dp);
> > > +out_locked:
> > >  	pm_qos_update_request(&dev_priv->pm_qos, PM_QOS_DEFAULT_VALUE);
> > >  
> > >  	if (vdd)
> > > @@ -1496,6 +1542,10 @@ static void intel_aux_reg_init(struct intel_dp *intel_dp)
> > >  	intel_dp->aux_ch_ctl_reg = intel_aux_ctl_reg(dev_priv, port);
> > >  	for (i = 0; i < ARRAY_SIZE(intel_dp->aux_ch_data_reg); i++)
> > >  		intel_dp->aux_ch_data_reg[i] = intel_aux_data_reg(dev_priv, port, i);
> > > +	if (INTEL_INFO(dev_priv)->gen >= 9) {
> > > +		intel_dp->aux_ch_mutex_reg = DP_AUX_CH_MUTEX(port);
> > > +		I915_WRITE(intel_dp->aux_ch_mutex_reg, DP_AUX_CH_MUTEX_ENABLE);
> > > +	}
> > >  }
> > >  
> > >  static void
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > > index 50874f4035cf..0c88657d6066 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -1043,6 +1043,7 @@ struct intel_dp {
> > >  	i915_reg_t output_reg;
> > >  	i915_reg_t aux_ch_ctl_reg;
> > >  	i915_reg_t aux_ch_data_reg[5];
> > > +	i915_reg_t aux_ch_mutex_reg;
> > >  	uint32_t DP;
> > >  	int link_rate;
> > >  	uint8_t lane_count;
> > > -- 
> > > 2.16.2
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > -- 
> > Ville Syrjälä
> > Intel OTC
> _______________________________________________
> 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

Re: [PATCH v2 1/1] drm/i915: Add and enable DP AUX CH mutex

[Rodrigo Vivi]

On Thu, Feb 22, 2018 at 11:57:03AM -0800, Pandiyan, Dhinakaran wrote:
> On Thu, 2018-02-22 at 11:34 -0800, Rodrigo Vivi wrote:
> > On Thu, Feb 22, 2018 at 08:59:49PM +0200, Ville Syrjälä wrote:
> > > On Tue, Feb 20, 2018 at 06:23:47PM -0800, José Roberto de Souza wrote:
> > > > When PSR/PSR2/GTC is enabled hardware can do AUX transactions by it
> > > > self, so lets use the mutex register that is available in gen9+ to
> > > > avoid concurrent access by hardware and driver.
> > > > 
> > > > Reference: https://01.org/sites/default/files/documentation/intel-gfx-prm-osrc-skl-vol12-display.pdf
> > > > Page 198 - AUX programming sequence
> > > 
> > > BTW since HSW/BDW don't have the mutex we should really be disabling PSR
> > > around aux ch usage on those platforms. Not sure how big of a locking
> > > inversion exercise it would be to make that happen though.
> > 
> > True. But I believe that can be handled differently.
> > Probably with Aux power domains blocking PSR off "domain".
> > Something similar to DC off.
> > 
> > I actually had patches for that lost somewhere... :(
> > 
> 
> Just a note, the spec explicitly says we should disable PSR around AUX
> transactions for BDW (and HSW iirc).
> 
> About using the power domain infra, I am not quite sure. The _get() and
> _put() idiom makes sense when we want reference counting to handle
> multiple consumers that want PSR to be disabled. Does anything else need
> to enable/disable PSR?

VLV/CHV vblanks?! :/

> 
> 
> > > 
> > > > 
> > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/i915_reg.h  |  9 ++++++++
> > > >  drivers/gpu/drm/i915/intel_dp.c  | 50 ++++++++++++++++++++++++++++++++++++++++
> > > >  drivers/gpu/drm/i915/intel_drv.h |  1 +
> > > >  3 files changed, 60 insertions(+)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > > > index 1412abcb27d4..a62e3c1badab 100644
> > > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > > @@ -5318,6 +5318,7 @@ enum {
> > > >  #define _DPA_AUX_CH_DATA3	(dev_priv->info.display_mmio_offset + 0x6401c)
> > > >  #define _DPA_AUX_CH_DATA4	(dev_priv->info.display_mmio_offset + 0x64020)
> > > >  #define _DPA_AUX_CH_DATA5	(dev_priv->info.display_mmio_offset + 0x64024)
> > > > +#define _DPA_AUX_CH_MUTEX	(dev_priv->info.display_mmio_offset + 0x6402C)
> > > >  
> > > >  #define _DPB_AUX_CH_CTL		(dev_priv->info.display_mmio_offset + 0x64110)
> > > >  #define _DPB_AUX_CH_DATA1	(dev_priv->info.display_mmio_offset + 0x64114)
> > > > @@ -5325,6 +5326,7 @@ enum {
> > > >  #define _DPB_AUX_CH_DATA3	(dev_priv->info.display_mmio_offset + 0x6411c)
> > > >  #define _DPB_AUX_CH_DATA4	(dev_priv->info.display_mmio_offset + 0x64120)
> > > >  #define _DPB_AUX_CH_DATA5	(dev_priv->info.display_mmio_offset + 0x64124)
> > > > +#define _DPB_AUX_CH_MUTEX	(dev_priv->info.display_mmio_offset + 0x6412C)
> > > >  
> > > >  #define _DPC_AUX_CH_CTL		(dev_priv->info.display_mmio_offset + 0x64210)
> > > >  #define _DPC_AUX_CH_DATA1	(dev_priv->info.display_mmio_offset + 0x64214)
> > > > @@ -5332,6 +5334,7 @@ enum {
> > > >  #define _DPC_AUX_CH_DATA3	(dev_priv->info.display_mmio_offset + 0x6421c)
> > > >  #define _DPC_AUX_CH_DATA4	(dev_priv->info.display_mmio_offset + 0x64220)
> > > >  #define _DPC_AUX_CH_DATA5	(dev_priv->info.display_mmio_offset + 0x64224)
> > > > +#define _DPC_AUX_CH_MUTEX	(dev_priv->info.display_mmio_offset + 0x6422C)
> > > >  
> > > >  #define _DPD_AUX_CH_CTL		(dev_priv->info.display_mmio_offset + 0x64310)
> > > >  #define _DPD_AUX_CH_DATA1	(dev_priv->info.display_mmio_offset + 0x64314)
> > > > @@ -5339,6 +5342,7 @@ enum {
> > > >  #define _DPD_AUX_CH_DATA3	(dev_priv->info.display_mmio_offset + 0x6431c)
> > > >  #define _DPD_AUX_CH_DATA4	(dev_priv->info.display_mmio_offset + 0x64320)
> > > >  #define _DPD_AUX_CH_DATA5	(dev_priv->info.display_mmio_offset + 0x64324)
> > > > +#define _DPD_AUX_CH_MUTEX	(dev_priv->info.display_mmio_offset + 0x6432C)
> > > >  
> > > >  #define _DPF_AUX_CH_CTL		(dev_priv->info.display_mmio_offset + 0x64510)
> > > >  #define _DPF_AUX_CH_DATA1	(dev_priv->info.display_mmio_offset + 0x64514)
> > > > @@ -5346,6 +5350,7 @@ enum {
> > > >  #define _DPF_AUX_CH_DATA3	(dev_priv->info.display_mmio_offset + 0x6451c)
> > > >  #define _DPF_AUX_CH_DATA4	(dev_priv->info.display_mmio_offset + 0x64520)
> > > >  #define _DPF_AUX_CH_DATA5	(dev_priv->info.display_mmio_offset + 0x64524)
> > > > +#define _DPF_AUX_CH_MUTEX	(dev_priv->info.display_mmio_offset + 0x6452C)
> > > >  
> > > >  #define DP_AUX_CH_CTL(port)	_MMIO_PORT(port, _DPA_AUX_CH_CTL, _DPB_AUX_CH_CTL)
> > > >  #define DP_AUX_CH_DATA(port, i)	_MMIO(_PORT(port, _DPA_AUX_CH_DATA1, _DPB_AUX_CH_DATA1) + (i) * 4) /* 5 registers */
> > > > @@ -5378,6 +5383,10 @@ enum {
> > > >  #define   DP_AUX_CH_CTL_FW_SYNC_PULSE_SKL(c) (((c) - 1) << 5)
> > > >  #define   DP_AUX_CH_CTL_SYNC_PULSE_SKL(c)   ((c) - 1)
> > > >  
> > > > +#define DP_AUX_CH_MUTEX(port)	_MMIO_PORT(port, _DPA_AUX_CH_MUTEX, _DPB_AUX_CH_MUTEX)
> > > > +#define   DP_AUX_CH_MUTEX_ENABLE		(1 << 31)
> > > > +#define   DP_AUX_CH_MUTEX_STATUS		(1 << 30)
> > > > +
> > > >  /*
> > > >   * Computing GMCH M and N values for the Display Port link
> > > >   *
> > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > > > index f20b25f98e5a..af07563bafba 100644
> > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > @@ -1081,6 +1081,45 @@ static uint32_t intel_dp_get_aux_send_ctl(struct intel_dp *intel_dp,
> > > >  						aux_clock_divider);
> > > >  }
> > > >  
> > > > +static bool intel_dp_aux_ch_trylock(struct intel_dp *intel_dp)
> > > > +{
> > > > +	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> > > > +	struct drm_i915_private *dev_priv =
> > > > +			to_i915(intel_dig_port->base.base.dev);
> > > > +
> > > > +	if (INTEL_GEN(dev_priv) < 9)
> > > > +		return true;
> > > > +
> > > > +	I915_WRITE(intel_dp->aux_ch_mutex_reg, DP_AUX_CH_MUTEX_ENABLE);
> > > > +
> > > > +	/* Spec says that mutex is acquired when status bit is read as unset,
> > > > +	 * if set it should be read again after 500usec. Here waiting for
> > > > +	 * 2msec or 4 tries before give up.
> > > > +	 */
> > > > +	if (__intel_wait_for_register(dev_priv, intel_dp->aux_ch_mutex_reg,
> > > > +				      DP_AUX_CH_MUTEX_STATUS, 0, 500, 2,
> > > > +				      NULL)) {
> > > > +		DRM_WARN("dp_aux_ch port locked for too long");
> > > > +		return false;
> > > > +	}
> > > > +
> > > > +	return true;
> > > > +}
> > > > +
> > > > +static void intel_dp_aux_ch_unlock(struct intel_dp *intel_dp)
> > > > +{
> > > > +	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> > > > +	struct drm_i915_private *dev_priv =
> > > > +			to_i915(intel_dig_port->base.base.dev);
> > > > +
> > > > +	if (INTEL_GEN(dev_priv) < 9)
> > > > +		return;
> > > > +
> > > > +	/* setting the status bit releases the mutex + keep mutex enabled */
> > > > +	I915_WRITE(intel_dp->aux_ch_mutex_reg, DP_AUX_CH_MUTEX_ENABLE |
> > > > +		   DP_AUX_CH_MUTEX_STATUS);
> > > > +}
> > > > +
> > > >  static int
> > > >  intel_dp_aux_ch(struct intel_dp *intel_dp,
> > > >  		const uint8_t *send, int send_bytes,
> > > > @@ -1115,6 +1154,11 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
> > > >  
> > > >  	intel_dp_check_edp(intel_dp);
> > > >  
> > > > +	if (!intel_dp_aux_ch_trylock(intel_dp)) {
> > > > +		ret = -EBUSY;
> > > > +		goto out_locked;
> > > > +	}
> > > > +
> > > >  	/* Try to wait for any previous AUX channel activity */
> > > >  	for (try = 0; try < 3; try++) {
> > > >  		status = I915_READ_NOTRACE(ch_ctl);
> > > > @@ -1244,6 +1288,8 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
> > > >  
> > > >  	ret = recv_bytes;
> > > >  out:
> > > > +	intel_dp_aux_ch_unlock(intel_dp);
> > > > +out_locked:
> > > >  	pm_qos_update_request(&dev_priv->pm_qos, PM_QOS_DEFAULT_VALUE);
> > > >  
> > > >  	if (vdd)
> > > > @@ -1496,6 +1542,10 @@ static void intel_aux_reg_init(struct intel_dp *intel_dp)
> > > >  	intel_dp->aux_ch_ctl_reg = intel_aux_ctl_reg(dev_priv, port);
> > > >  	for (i = 0; i < ARRAY_SIZE(intel_dp->aux_ch_data_reg); i++)
> > > >  		intel_dp->aux_ch_data_reg[i] = intel_aux_data_reg(dev_priv, port, i);
> > > > +	if (INTEL_INFO(dev_priv)->gen >= 9) {
> > > > +		intel_dp->aux_ch_mutex_reg = DP_AUX_CH_MUTEX(port);
> > > > +		I915_WRITE(intel_dp->aux_ch_mutex_reg, DP_AUX_CH_MUTEX_ENABLE);
> > > > +	}
> > > >  }
> > > >  
> > > >  static void
> > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > > > index 50874f4035cf..0c88657d6066 100644
> > > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > > @@ -1043,6 +1043,7 @@ struct intel_dp {
> > > >  	i915_reg_t output_reg;
> > > >  	i915_reg_t aux_ch_ctl_reg;
> > > >  	i915_reg_t aux_ch_data_reg[5];
> > > > +	i915_reg_t aux_ch_mutex_reg;
> > > >  	uint32_t DP;
> > > >  	int link_rate;
> > > >  	uint8_t lane_count;
> > > > -- 
> > > > 2.16.2
> > > > 
> > > > _______________________________________________
> > > > Intel-gfx mailing list
> > > > Intel-gfx@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > > 
> > > -- 
> > > Ville Syrjälä
> > > Intel OTC
> > _______________________________________________
> > 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

Re: [PATCH v2 1/1] drm/i915: Add and enable DP AUX CH mutex

[Pandiyan, Dhinakaran]

On Thu, 2018-02-22 at 12:13 -0800, Rodrigo Vivi wrote:
> On Thu, Feb 22, 2018 at 11:57:03AM -0800, Pandiyan, Dhinakaran wrote:
> > On Thu, 2018-02-22 at 11:34 -0800, Rodrigo Vivi wrote:
> > > On Thu, Feb 22, 2018 at 08:59:49PM +0200, Ville Syrjälä wrote:
> > > > On Tue, Feb 20, 2018 at 06:23:47PM -0800, José Roberto de Souza wrote:
> > > > > When PSR/PSR2/GTC is enabled hardware can do AUX transactions by it
> > > > > self, so lets use the mutex register that is available in gen9+ to
> > > > > avoid concurrent access by hardware and driver.
> > > > > 
> > > > > Reference: https://01.org/sites/default/files/documentation/intel-gfx-prm-osrc-skl-vol12-display.pdf
> > > > > Page 198 - AUX programming sequence
> > > > 
> > > > BTW since HSW/BDW don't have the mutex we should really be disabling PSR
> > > > around aux ch usage on those platforms. Not sure how big of a locking
> > > > inversion exercise it would be to make that happen though.
> > > 
> > > True. But I believe that can be handled differently.
> > > Probably with Aux power domains blocking PSR off "domain".
> > > Something similar to DC off.
> > > 
> > > I actually had patches for that lost somewhere... :(
> > > 
> > 
> > Just a note, the spec explicitly says we should disable PSR around AUX
> > transactions for BDW (and HSW iirc).
> > 
> > About using the power domain infra, I am not quite sure. The _get() and
> > _put() idiom makes sense when we want reference counting to handle
> > multiple consumers that want PSR to be disabled. Does anything else need
> > to enable/disable PSR?
> 
> VLV/CHV vblanks?! :/
> 

I keep forgetting VLV/CHV, sorry. This brings us back to the whole
"calling blocking functions from atomic contexts" problem. Not sure how
to handle that.

Assuming we solve that problem, we should also probably avoid polluting
the power domain code and add reference counting for PSR. However, since
all this is not required for BDW/HSW, I am inclined towards a simpler
solution for BDW/HSW AUX usage. 


> > 
> > 
> > > > 
> > > > > 
> > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/i915_reg.h  |  9 ++++++++
> > > > >  drivers/gpu/drm/i915/intel_dp.c  | 50 ++++++++++++++++++++++++++++++++++++++++
> > > > >  drivers/gpu/drm/i915/intel_drv.h |  1 +
> > > > >  3 files changed, 60 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > > > > index 1412abcb27d4..a62e3c1badab 100644
> > > > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > > > @@ -5318,6 +5318,7 @@ enum {
> > > > >  #define _DPA_AUX_CH_DATA3	(dev_priv->info.display_mmio_offset + 0x6401c)
> > > > >  #define _DPA_AUX_CH_DATA4	(dev_priv->info.display_mmio_offset + 0x64020)
> > > > >  #define _DPA_AUX_CH_DATA5	(dev_priv->info.display_mmio_offset + 0x64024)
> > > > > +#define _DPA_AUX_CH_MUTEX	(dev_priv->info.display_mmio_offset + 0x6402C)
> > > > >  
> > > > >  #define _DPB_AUX_CH_CTL		(dev_priv->info.display_mmio_offset + 0x64110)
> > > > >  #define _DPB_AUX_CH_DATA1	(dev_priv->info.display_mmio_offset + 0x64114)
> > > > > @@ -5325,6 +5326,7 @@ enum {
> > > > >  #define _DPB_AUX_CH_DATA3	(dev_priv->info.display_mmio_offset + 0x6411c)
> > > > >  #define _DPB_AUX_CH_DATA4	(dev_priv->info.display_mmio_offset + 0x64120)
> > > > >  #define _DPB_AUX_CH_DATA5	(dev_priv->info.display_mmio_offset + 0x64124)
> > > > > +#define _DPB_AUX_CH_MUTEX	(dev_priv->info.display_mmio_offset + 0x6412C)
> > > > >  
> > > > >  #define _DPC_AUX_CH_CTL		(dev_priv->info.display_mmio_offset + 0x64210)
> > > > >  #define _DPC_AUX_CH_DATA1	(dev_priv->info.display_mmio_offset + 0x64214)
> > > > > @@ -5332,6 +5334,7 @@ enum {
> > > > >  #define _DPC_AUX_CH_DATA3	(dev_priv->info.display_mmio_offset + 0x6421c)
> > > > >  #define _DPC_AUX_CH_DATA4	(dev_priv->info.display_mmio_offset + 0x64220)
> > > > >  #define _DPC_AUX_CH_DATA5	(dev_priv->info.display_mmio_offset + 0x64224)
> > > > > +#define _DPC_AUX_CH_MUTEX	(dev_priv->info.display_mmio_offset + 0x6422C)
> > > > >  
> > > > >  #define _DPD_AUX_CH_CTL		(dev_priv->info.display_mmio_offset + 0x64310)
> > > > >  #define _DPD_AUX_CH_DATA1	(dev_priv->info.display_mmio_offset + 0x64314)
> > > > > @@ -5339,6 +5342,7 @@ enum {
> > > > >  #define _DPD_AUX_CH_DATA3	(dev_priv->info.display_mmio_offset + 0x6431c)
> > > > >  #define _DPD_AUX_CH_DATA4	(dev_priv->info.display_mmio_offset + 0x64320)
> > > > >  #define _DPD_AUX_CH_DATA5	(dev_priv->info.display_mmio_offset + 0x64324)
> > > > > +#define _DPD_AUX_CH_MUTEX	(dev_priv->info.display_mmio_offset + 0x6432C)
> > > > >  
> > > > >  #define _DPF_AUX_CH_CTL		(dev_priv->info.display_mmio_offset + 0x64510)
> > > > >  #define _DPF_AUX_CH_DATA1	(dev_priv->info.display_mmio_offset + 0x64514)
> > > > > @@ -5346,6 +5350,7 @@ enum {
> > > > >  #define _DPF_AUX_CH_DATA3	(dev_priv->info.display_mmio_offset + 0x6451c)
> > > > >  #define _DPF_AUX_CH_DATA4	(dev_priv->info.display_mmio_offset + 0x64520)
> > > > >  #define _DPF_AUX_CH_DATA5	(dev_priv->info.display_mmio_offset + 0x64524)
> > > > > +#define _DPF_AUX_CH_MUTEX	(dev_priv->info.display_mmio_offset + 0x6452C)
> > > > >  
> > > > >  #define DP_AUX_CH_CTL(port)	_MMIO_PORT(port, _DPA_AUX_CH_CTL, _DPB_AUX_CH_CTL)
> > > > >  #define DP_AUX_CH_DATA(port, i)	_MMIO(_PORT(port, _DPA_AUX_CH_DATA1, _DPB_AUX_CH_DATA1) + (i) * 4) /* 5 registers */
> > > > > @@ -5378,6 +5383,10 @@ enum {
> > > > >  #define   DP_AUX_CH_CTL_FW_SYNC_PULSE_SKL(c) (((c) - 1) << 5)
> > > > >  #define   DP_AUX_CH_CTL_SYNC_PULSE_SKL(c)   ((c) - 1)
> > > > >  
> > > > > +#define DP_AUX_CH_MUTEX(port)	_MMIO_PORT(port, _DPA_AUX_CH_MUTEX, _DPB_AUX_CH_MUTEX)
> > > > > +#define   DP_AUX_CH_MUTEX_ENABLE		(1 << 31)
> > > > > +#define   DP_AUX_CH_MUTEX_STATUS		(1 << 30)
> > > > > +
> > > > >  /*
> > > > >   * Computing GMCH M and N values for the Display Port link
> > > > >   *
> > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > > > > index f20b25f98e5a..af07563bafba 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > > @@ -1081,6 +1081,45 @@ static uint32_t intel_dp_get_aux_send_ctl(struct intel_dp *intel_dp,
> > > > >  						aux_clock_divider);
> > > > >  }
> > > > >  
> > > > > +static bool intel_dp_aux_ch_trylock(struct intel_dp *intel_dp)
> > > > > +{
> > > > > +	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> > > > > +	struct drm_i915_private *dev_priv =
> > > > > +			to_i915(intel_dig_port->base.base.dev);
> > > > > +
> > > > > +	if (INTEL_GEN(dev_priv) < 9)
> > > > > +		return true;
> > > > > +
> > > > > +	I915_WRITE(intel_dp->aux_ch_mutex_reg, DP_AUX_CH_MUTEX_ENABLE);
> > > > > +
> > > > > +	/* Spec says that mutex is acquired when status bit is read as unset,
> > > > > +	 * if set it should be read again after 500usec. Here waiting for
> > > > > +	 * 2msec or 4 tries before give up.
> > > > > +	 */
> > > > > +	if (__intel_wait_for_register(dev_priv, intel_dp->aux_ch_mutex_reg,
> > > > > +				      DP_AUX_CH_MUTEX_STATUS, 0, 500, 2,
> > > > > +				      NULL)) {
> > > > > +		DRM_WARN("dp_aux_ch port locked for too long");
> > > > > +		return false;
> > > > > +	}
> > > > > +
> > > > > +	return true;
> > > > > +}
> > > > > +
> > > > > +static void intel_dp_aux_ch_unlock(struct intel_dp *intel_dp)
> > > > > +{
> > > > > +	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> > > > > +	struct drm_i915_private *dev_priv =
> > > > > +			to_i915(intel_dig_port->base.base.dev);
> > > > > +
> > > > > +	if (INTEL_GEN(dev_priv) < 9)
> > > > > +		return;
> > > > > +
> > > > > +	/* setting the status bit releases the mutex + keep mutex enabled */
> > > > > +	I915_WRITE(intel_dp->aux_ch_mutex_reg, DP_AUX_CH_MUTEX_ENABLE |
> > > > > +		   DP_AUX_CH_MUTEX_STATUS);
> > > > > +}
> > > > > +
> > > > >  static int
> > > > >  intel_dp_aux_ch(struct intel_dp *intel_dp,
> > > > >  		const uint8_t *send, int send_bytes,
> > > > > @@ -1115,6 +1154,11 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
> > > > >  
> > > > >  	intel_dp_check_edp(intel_dp);
> > > > >  
> > > > > +	if (!intel_dp_aux_ch_trylock(intel_dp)) {
> > > > > +		ret = -EBUSY;
> > > > > +		goto out_locked;
> > > > > +	}
> > > > > +
> > > > >  	/* Try to wait for any previous AUX channel activity */
> > > > >  	for (try = 0; try < 3; try++) {
> > > > >  		status = I915_READ_NOTRACE(ch_ctl);
> > > > > @@ -1244,6 +1288,8 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
> > > > >  
> > > > >  	ret = recv_bytes;
> > > > >  out:
> > > > > +	intel_dp_aux_ch_unlock(intel_dp);
> > > > > +out_locked:
> > > > >  	pm_qos_update_request(&dev_priv->pm_qos, PM_QOS_DEFAULT_VALUE);
> > > > >  
> > > > >  	if (vdd)
> > > > > @@ -1496,6 +1542,10 @@ static void intel_aux_reg_init(struct intel_dp *intel_dp)
> > > > >  	intel_dp->aux_ch_ctl_reg = intel_aux_ctl_reg(dev_priv, port);
> > > > >  	for (i = 0; i < ARRAY_SIZE(intel_dp->aux_ch_data_reg); i++)
> > > > >  		intel_dp->aux_ch_data_reg[i] = intel_aux_data_reg(dev_priv, port, i);
> > > > > +	if (INTEL_INFO(dev_priv)->gen >= 9) {
> > > > > +		intel_dp->aux_ch_mutex_reg = DP_AUX_CH_MUTEX(port);
> > > > > +		I915_WRITE(intel_dp->aux_ch_mutex_reg, DP_AUX_CH_MUTEX_ENABLE);
> > > > > +	}
> > > > >  }
> > > > >  
> > > > >  static void
> > > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > > > > index 50874f4035cf..0c88657d6066 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > > > @@ -1043,6 +1043,7 @@ struct intel_dp {
> > > > >  	i915_reg_t output_reg;
> > > > >  	i915_reg_t aux_ch_ctl_reg;
> > > > >  	i915_reg_t aux_ch_data_reg[5];
> > > > > +	i915_reg_t aux_ch_mutex_reg;
> > > > >  	uint32_t DP;
> > > > >  	int link_rate;
> > > > >  	uint8_t lane_count;
> > > > > -- 
> > > > > 2.16.2
> > > > > 
> > > > > _______________________________________________
> > > > > Intel-gfx mailing list
> > > > > Intel-gfx@lists.freedesktop.org
> > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > > > 
> > > > -- 
> > > > Ville Syrjälä
> > > > Intel OTC
> > > _______________________________________________
> > > 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 1/1] drm/i915: Add and enable DP AUX CH mutex

[Rodrigo Vivi]

On Thu, Feb 22, 2018 at 12:25:46PM -0800, Pandiyan, Dhinakaran wrote:
> On Thu, 2018-02-22 at 12:13 -0800, Rodrigo Vivi wrote:
> > On Thu, Feb 22, 2018 at 11:57:03AM -0800, Pandiyan, Dhinakaran wrote:
> > > On Thu, 2018-02-22 at 11:34 -0800, Rodrigo Vivi wrote:
> > > > On Thu, Feb 22, 2018 at 08:59:49PM +0200, Ville Syrjälä wrote:
> > > > > On Tue, Feb 20, 2018 at 06:23:47PM -0800, José Roberto de Souza wrote:
> > > > > > When PSR/PSR2/GTC is enabled hardware can do AUX transactions by it
> > > > > > self, so lets use the mutex register that is available in gen9+ to
> > > > > > avoid concurrent access by hardware and driver.
> > > > > > 
> > > > > > Reference: https://01.org/sites/default/files/documentation/intel-gfx-prm-osrc-skl-vol12-display.pdf
> > > > > > Page 198 - AUX programming sequence
> > > > > 
> > > > > BTW since HSW/BDW don't have the mutex we should really be disabling PSR
> > > > > around aux ch usage on those platforms. Not sure how big of a locking
> > > > > inversion exercise it would be to make that happen though.
> > > > 
> > > > True. But I believe that can be handled differently.
> > > > Probably with Aux power domains blocking PSR off "domain".
> > > > Something similar to DC off.
> > > > 
> > > > I actually had patches for that lost somewhere... :(
> > > > 
> > > 
> > > Just a note, the spec explicitly says we should disable PSR around AUX
> > > transactions for BDW (and HSW iirc).
> > > 
> > > About using the power domain infra, I am not quite sure. The _get() and
> > > _put() idiom makes sense when we want reference counting to handle
> > > multiple consumers that want PSR to be disabled. Does anything else need
> > > to enable/disable PSR?
> > 
> > VLV/CHV vblanks?! :/
> > 
> 
> I keep forgetting VLV/CHV, sorry. This brings us back to the whole
> "calling blocking functions from atomic contexts" problem. Not sure how
> to handle that.
> 
> Assuming we solve that problem, we should also probably avoid polluting
> the power domain code and add reference counting for PSR. However, since
> all this is not required for BDW/HSW, I am inclined towards a simpler
> solution for BDW/HSW AUX usage. 

I agree. Then probably the simpler solution could than be applied on
VLV/CHV... who knows.

anyways separated from this patch here... let's keep this one clean.

> 
> 
> > > 
> > > 
> > > > > 
> > > > > > 
> > > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > > > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > > > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > > > > ---
> > > > > >  drivers/gpu/drm/i915/i915_reg.h  |  9 ++++++++
> > > > > >  drivers/gpu/drm/i915/intel_dp.c  | 50 ++++++++++++++++++++++++++++++++++++++++
> > > > > >  drivers/gpu/drm/i915/intel_drv.h |  1 +
> > > > > >  3 files changed, 60 insertions(+)
> > > > > > 
> > > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > > > > > index 1412abcb27d4..a62e3c1badab 100644
> > > > > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > > > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > > > > @@ -5318,6 +5318,7 @@ enum {
> > > > > >  #define _DPA_AUX_CH_DATA3	(dev_priv->info.display_mmio_offset + 0x6401c)
> > > > > >  #define _DPA_AUX_CH_DATA4	(dev_priv->info.display_mmio_offset + 0x64020)
> > > > > >  #define _DPA_AUX_CH_DATA5	(dev_priv->info.display_mmio_offset + 0x64024)
> > > > > > +#define _DPA_AUX_CH_MUTEX	(dev_priv->info.display_mmio_offset + 0x6402C)
> > > > > >  
> > > > > >  #define _DPB_AUX_CH_CTL		(dev_priv->info.display_mmio_offset + 0x64110)
> > > > > >  #define _DPB_AUX_CH_DATA1	(dev_priv->info.display_mmio_offset + 0x64114)
> > > > > > @@ -5325,6 +5326,7 @@ enum {
> > > > > >  #define _DPB_AUX_CH_DATA3	(dev_priv->info.display_mmio_offset + 0x6411c)
> > > > > >  #define _DPB_AUX_CH_DATA4	(dev_priv->info.display_mmio_offset + 0x64120)
> > > > > >  #define _DPB_AUX_CH_DATA5	(dev_priv->info.display_mmio_offset + 0x64124)
> > > > > > +#define _DPB_AUX_CH_MUTEX	(dev_priv->info.display_mmio_offset + 0x6412C)
> > > > > >  
> > > > > >  #define _DPC_AUX_CH_CTL		(dev_priv->info.display_mmio_offset + 0x64210)
> > > > > >  #define _DPC_AUX_CH_DATA1	(dev_priv->info.display_mmio_offset + 0x64214)
> > > > > > @@ -5332,6 +5334,7 @@ enum {
> > > > > >  #define _DPC_AUX_CH_DATA3	(dev_priv->info.display_mmio_offset + 0x6421c)
> > > > > >  #define _DPC_AUX_CH_DATA4	(dev_priv->info.display_mmio_offset + 0x64220)
> > > > > >  #define _DPC_AUX_CH_DATA5	(dev_priv->info.display_mmio_offset + 0x64224)
> > > > > > +#define _DPC_AUX_CH_MUTEX	(dev_priv->info.display_mmio_offset + 0x6422C)
> > > > > >  
> > > > > >  #define _DPD_AUX_CH_CTL		(dev_priv->info.display_mmio_offset + 0x64310)
> > > > > >  #define _DPD_AUX_CH_DATA1	(dev_priv->info.display_mmio_offset + 0x64314)
> > > > > > @@ -5339,6 +5342,7 @@ enum {
> > > > > >  #define _DPD_AUX_CH_DATA3	(dev_priv->info.display_mmio_offset + 0x6431c)
> > > > > >  #define _DPD_AUX_CH_DATA4	(dev_priv->info.display_mmio_offset + 0x64320)
> > > > > >  #define _DPD_AUX_CH_DATA5	(dev_priv->info.display_mmio_offset + 0x64324)
> > > > > > +#define _DPD_AUX_CH_MUTEX	(dev_priv->info.display_mmio_offset + 0x6432C)
> > > > > >  
> > > > > >  #define _DPF_AUX_CH_CTL		(dev_priv->info.display_mmio_offset + 0x64510)
> > > > > >  #define _DPF_AUX_CH_DATA1	(dev_priv->info.display_mmio_offset + 0x64514)
> > > > > > @@ -5346,6 +5350,7 @@ enum {
> > > > > >  #define _DPF_AUX_CH_DATA3	(dev_priv->info.display_mmio_offset + 0x6451c)
> > > > > >  #define _DPF_AUX_CH_DATA4	(dev_priv->info.display_mmio_offset + 0x64520)
> > > > > >  #define _DPF_AUX_CH_DATA5	(dev_priv->info.display_mmio_offset + 0x64524)
> > > > > > +#define _DPF_AUX_CH_MUTEX	(dev_priv->info.display_mmio_offset + 0x6452C)
> > > > > >  
> > > > > >  #define DP_AUX_CH_CTL(port)	_MMIO_PORT(port, _DPA_AUX_CH_CTL, _DPB_AUX_CH_CTL)
> > > > > >  #define DP_AUX_CH_DATA(port, i)	_MMIO(_PORT(port, _DPA_AUX_CH_DATA1, _DPB_AUX_CH_DATA1) + (i) * 4) /* 5 registers */
> > > > > > @@ -5378,6 +5383,10 @@ enum {
> > > > > >  #define   DP_AUX_CH_CTL_FW_SYNC_PULSE_SKL(c) (((c) - 1) << 5)
> > > > > >  #define   DP_AUX_CH_CTL_SYNC_PULSE_SKL(c)   ((c) - 1)
> > > > > >  
> > > > > > +#define DP_AUX_CH_MUTEX(port)	_MMIO_PORT(port, _DPA_AUX_CH_MUTEX, _DPB_AUX_CH_MUTEX)
> > > > > > +#define   DP_AUX_CH_MUTEX_ENABLE		(1 << 31)
> > > > > > +#define   DP_AUX_CH_MUTEX_STATUS		(1 << 30)
> > > > > > +
> > > > > >  /*
> > > > > >   * Computing GMCH M and N values for the Display Port link
> > > > > >   *
> > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > > > > > index f20b25f98e5a..af07563bafba 100644
> > > > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > > > @@ -1081,6 +1081,45 @@ static uint32_t intel_dp_get_aux_send_ctl(struct intel_dp *intel_dp,
> > > > > >  						aux_clock_divider);
> > > > > >  }
> > > > > >  
> > > > > > +static bool intel_dp_aux_ch_trylock(struct intel_dp *intel_dp)
> > > > > > +{
> > > > > > +	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> > > > > > +	struct drm_i915_private *dev_priv =
> > > > > > +			to_i915(intel_dig_port->base.base.dev);
> > > > > > +
> > > > > > +	if (INTEL_GEN(dev_priv) < 9)
> > > > > > +		return true;
> > > > > > +
> > > > > > +	I915_WRITE(intel_dp->aux_ch_mutex_reg, DP_AUX_CH_MUTEX_ENABLE);
> > > > > > +
> > > > > > +	/* Spec says that mutex is acquired when status bit is read as unset,
> > > > > > +	 * if set it should be read again after 500usec. Here waiting for
> > > > > > +	 * 2msec or 4 tries before give up.
> > > > > > +	 */
> > > > > > +	if (__intel_wait_for_register(dev_priv, intel_dp->aux_ch_mutex_reg,
> > > > > > +				      DP_AUX_CH_MUTEX_STATUS, 0, 500, 2,
> > > > > > +				      NULL)) {
> > > > > > +		DRM_WARN("dp_aux_ch port locked for too long");
> > > > > > +		return false;
> > > > > > +	}
> > > > > > +
> > > > > > +	return true;
> > > > > > +}
> > > > > > +
> > > > > > +static void intel_dp_aux_ch_unlock(struct intel_dp *intel_dp)
> > > > > > +{
> > > > > > +	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> > > > > > +	struct drm_i915_private *dev_priv =
> > > > > > +			to_i915(intel_dig_port->base.base.dev);
> > > > > > +
> > > > > > +	if (INTEL_GEN(dev_priv) < 9)
> > > > > > +		return;
> > > > > > +
> > > > > > +	/* setting the status bit releases the mutex + keep mutex enabled */
> > > > > > +	I915_WRITE(intel_dp->aux_ch_mutex_reg, DP_AUX_CH_MUTEX_ENABLE |
> > > > > > +		   DP_AUX_CH_MUTEX_STATUS);
> > > > > > +}
> > > > > > +
> > > > > >  static int
> > > > > >  intel_dp_aux_ch(struct intel_dp *intel_dp,
> > > > > >  		const uint8_t *send, int send_bytes,
> > > > > > @@ -1115,6 +1154,11 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
> > > > > >  
> > > > > >  	intel_dp_check_edp(intel_dp);
> > > > > >  
> > > > > > +	if (!intel_dp_aux_ch_trylock(intel_dp)) {
> > > > > > +		ret = -EBUSY;
> > > > > > +		goto out_locked;
> > > > > > +	}
> > > > > > +
> > > > > >  	/* Try to wait for any previous AUX channel activity */
> > > > > >  	for (try = 0; try < 3; try++) {
> > > > > >  		status = I915_READ_NOTRACE(ch_ctl);
> > > > > > @@ -1244,6 +1288,8 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
> > > > > >  
> > > > > >  	ret = recv_bytes;
> > > > > >  out:
> > > > > > +	intel_dp_aux_ch_unlock(intel_dp);
> > > > > > +out_locked:
> > > > > >  	pm_qos_update_request(&dev_priv->pm_qos, PM_QOS_DEFAULT_VALUE);
> > > > > >  
> > > > > >  	if (vdd)
> > > > > > @@ -1496,6 +1542,10 @@ static void intel_aux_reg_init(struct intel_dp *intel_dp)
> > > > > >  	intel_dp->aux_ch_ctl_reg = intel_aux_ctl_reg(dev_priv, port);
> > > > > >  	for (i = 0; i < ARRAY_SIZE(intel_dp->aux_ch_data_reg); i++)
> > > > > >  		intel_dp->aux_ch_data_reg[i] = intel_aux_data_reg(dev_priv, port, i);
> > > > > > +	if (INTEL_INFO(dev_priv)->gen >= 9) {
> > > > > > +		intel_dp->aux_ch_mutex_reg = DP_AUX_CH_MUTEX(port);
> > > > > > +		I915_WRITE(intel_dp->aux_ch_mutex_reg, DP_AUX_CH_MUTEX_ENABLE);
> > > > > > +	}
> > > > > >  }
> > > > > >  
> > > > > >  static void
> > > > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > > > > > index 50874f4035cf..0c88657d6066 100644
> > > > > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > > > > @@ -1043,6 +1043,7 @@ struct intel_dp {
> > > > > >  	i915_reg_t output_reg;
> > > > > >  	i915_reg_t aux_ch_ctl_reg;
> > > > > >  	i915_reg_t aux_ch_data_reg[5];
> > > > > > +	i915_reg_t aux_ch_mutex_reg;
> > > > > >  	uint32_t DP;
> > > > > >  	int link_rate;
> > > > > >  	uint8_t lane_count;
> > > > > > -- 
> > > > > > 2.16.2
> > > > > > 
> > > > > > _______________________________________________
> > > > > > Intel-gfx mailing list
> > > > > > Intel-gfx@lists.freedesktop.org
> > > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > > > > 
> > > > > -- 
> > > > > Ville Syrjälä
> > > > > Intel OTC
> > > > _______________________________________________
> > > > 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 1/1] drm/i915: Add and enable DP AUX CH mutex

[Souza, Jose]

On Wed, 2018-02-21 at 14:28 -0800, Rodrigo Vivi wrote:
> On Wed, Feb 21, 2018 at 01:12:08PM -0800, Souza, Jose wrote:
> > On Wed, 2018-02-21 at 12:45 -0800, Rodrigo Vivi wrote:
> > > On Tue, Feb 20, 2018 at 06:23:47PM -0800, José Roberto de Souza
> > > wrote:
> > > > When PSR/PSR2/GTC is enabled hardware can do AUX transactions
> > > > by it
> > > > self, so lets use the mutex register that is available in gen9+
> > > > to
> > > > avoid concurrent access by hardware and driver.
> > > > 
> > > > Reference: https://01.org/sites/default/files/documentation/int
> > > > el-g
> > > > fx-prm-osrc-skl-vol12-display.pdf
> > > > Page 198 - AUX programming sequence
> > > > 
> > > 
> > > Cc: Ville
> > > Cc: DK
> > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/i915_reg.h  |  9 ++++++++
> > > >  drivers/gpu/drm/i915/intel_dp.c  | 50
> > > > ++++++++++++++++++++++++++++++++++++++++
> > > >  drivers/gpu/drm/i915/intel_drv.h |  1 +
> > > >  3 files changed, 60 insertions(+)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > > > b/drivers/gpu/drm/i915/i915_reg.h
> > > > index 1412abcb27d4..a62e3c1badab 100644
> > > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > > @@ -5318,6 +5318,7 @@ enum {
> > > >  #define _DPA_AUX_CH_DATA3	(dev_priv-
> > > > > info.display_mmio_offset + 0x6401c)
> > > > 
> > > >  #define _DPA_AUX_CH_DATA4	(dev_priv-
> > > > > info.display_mmio_offset + 0x64020)
> > > > 
> > > >  #define _DPA_AUX_CH_DATA5	(dev_priv-
> > > > > info.display_mmio_offset + 0x64024)
> > > > 
> > > > +#define _DPA_AUX_CH_MUTEX	(dev_priv-
> > > > > info.display_mmio_offset + 0x6402C)
> > > > 
> > > >  
> > > >  #define _DPB_AUX_CH_CTL		(dev_priv-
> > > > > info.display_mmio_offset + 0x64110)
> > > > 
> > > >  #define _DPB_AUX_CH_DATA1	(dev_priv-
> > > > > info.display_mmio_offset + 0x64114)
> > > > 
> > > > @@ -5325,6 +5326,7 @@ enum {
> > > >  #define _DPB_AUX_CH_DATA3	(dev_priv-
> > > > > info.display_mmio_offset + 0x6411c)
> > > > 
> > > >  #define _DPB_AUX_CH_DATA4	(dev_priv-
> > > > > info.display_mmio_offset + 0x64120)
> > > > 
> > > >  #define _DPB_AUX_CH_DATA5	(dev_priv-
> > > > > info.display_mmio_offset + 0x64124)
> > > > 
> > > > +#define _DPB_AUX_CH_MUTEX	(dev_priv-
> > > > > info.display_mmio_offset + 0x6412C)
> > > > 
> > > >  
> > > >  #define _DPC_AUX_CH_CTL		(dev_priv-
> > > > > info.display_mmio_offset + 0x64210)
> > > > 
> > > >  #define _DPC_AUX_CH_DATA1	(dev_priv-
> > > > > info.display_mmio_offset + 0x64214)
> > > > 
> > > > @@ -5332,6 +5334,7 @@ enum {
> > > >  #define _DPC_AUX_CH_DATA3	(dev_priv-
> > > > > info.display_mmio_offset + 0x6421c)
> > > > 
> > > >  #define _DPC_AUX_CH_DATA4	(dev_priv-
> > > > > info.display_mmio_offset + 0x64220)
> > > > 
> > > >  #define _DPC_AUX_CH_DATA5	(dev_priv-
> > > > > info.display_mmio_offset + 0x64224)
> > > > 
> > > > +#define _DPC_AUX_CH_MUTEX	(dev_priv-
> > > > > info.display_mmio_offset + 0x6422C)
> > > > 
> > > >  
> > > >  #define _DPD_AUX_CH_CTL		(dev_priv-
> > > > > info.display_mmio_offset + 0x64310)
> > > > 
> > > >  #define _DPD_AUX_CH_DATA1	(dev_priv-
> > > > > info.display_mmio_offset + 0x64314)
> > > > 
> > > > @@ -5339,6 +5342,7 @@ enum {
> > > >  #define _DPD_AUX_CH_DATA3	(dev_priv-
> > > > > info.display_mmio_offset + 0x6431c)
> > > > 
> > > >  #define _DPD_AUX_CH_DATA4	(dev_priv-
> > > > > info.display_mmio_offset + 0x64320)
> > > > 
> > > >  #define _DPD_AUX_CH_DATA5	(dev_priv-
> > > > > info.display_mmio_offset + 0x64324)
> > > > 
> > > > +#define _DPD_AUX_CH_MUTEX	(dev_priv-
> > > > > info.display_mmio_offset + 0x6432C)
> > > > 
> > > >  
> > > >  #define _DPF_AUX_CH_CTL		(dev_priv-
> > > > > info.display_mmio_offset + 0x64510)
> > > > 
> > > >  #define _DPF_AUX_CH_DATA1	(dev_priv-
> > > > > info.display_mmio_offset + 0x64514)
> > > > 
> > > > @@ -5346,6 +5350,7 @@ enum {
> > > >  #define _DPF_AUX_CH_DATA3	(dev_priv-
> > > > > info.display_mmio_offset + 0x6451c)
> > > > 
> > > >  #define _DPF_AUX_CH_DATA4	(dev_priv-
> > > > > info.display_mmio_offset + 0x64520)
> > > > 
> > > >  #define _DPF_AUX_CH_DATA5	(dev_priv-
> > > > > info.display_mmio_offset + 0x64524)
> > > > 
> > > > +#define _DPF_AUX_CH_MUTEX	(dev_priv-
> > > > > info.display_mmio_offset + 0x6452C)
> > > > 
> > > >  
> > > >  #define DP_AUX_CH_CTL(port)	_MMIO_PORT(port,
> > > > _DPA_AUX_CH_CTL, _DPB_AUX_CH_CTL)
> > > >  #define DP_AUX_CH_DATA(port, i)	_MMIO(_PORT(port,
> > > > _DPA_AUX_CH_DATA1, _DPB_AUX_CH_DATA1) + (i) * 4) /* 5 registers
> > > > */
> > > > @@ -5378,6 +5383,10 @@ enum {
> > > >  #define   DP_AUX_CH_CTL_FW_SYNC_PULSE_SKL(c) (((c) - 1) << 5)
> > > >  #define   DP_AUX_CH_CTL_SYNC_PULSE_SKL(c)   ((c) - 1)
> > > > 
> > > 
> > > 
> > > I believe _DPA_AUX_CH_MUTEX and _DPB_AUX_CH_MUTEX should be
> > > defined
> > > here along
> > > with the bits as Jani suggested.
> > > I don't see any reason to tag that far from the bits and close to
> > > CTL
> > > and DATA.

Okay moving it.

> > > 
> > > > +#define DP_AUX_CH_MUTEX(port)	_MMIO_PORT(port,
> > > > _DPA_AUX_CH_MUTEX, _DPB_AUX_CH_MUTEX)
> > > > +#define   DP_AUX_CH_MUTEX_ENABLE		(1 << 31)
> > > > +#define   DP_AUX_CH_MUTEX_STATUS		(1 << 30)
> > > > +
> > > >  /*
> > > >   * Computing GMCH M and N values for the Display Port link
> > > >   *
> > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > > b/drivers/gpu/drm/i915/intel_dp.c
> > > > index f20b25f98e5a..af07563bafba 100644
> > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > @@ -1081,6 +1081,45 @@ static uint32_t
> > > > intel_dp_get_aux_send_ctl(struct intel_dp *intel_dp,
> > > >  						aux_clock_divi
> > > > der)
> > > > ;
> > > >  }
> > > >  
> > > > +static bool intel_dp_aux_ch_trylock(struct intel_dp *intel_dp)
> > > > +{
> > > > +	struct intel_digital_port *intel_dig_port =
> > > > dp_to_dig_port(intel_dp);
> > > > +	struct drm_i915_private *dev_priv =
> > > > +			to_i915(intel_dig_port-
> > > > >base.base.dev);
> > > > +
> > > > +	if (INTEL_GEN(dev_priv) < 9)
> > > > +		return true;
> > > > +
> > > > +	I915_WRITE(intel_dp->aux_ch_mutex_reg,
> > > > DP_AUX_CH_MUTEX_ENABLE);
> > > 
> > > You might be touching bits. We don't know if HW is using the
> > > reserved
> > > bits or not.
> > > So RMW |= bit 31 here is a good idea.
> > 
> > As a read in this register request the mutex lock is better avoid
> > any
> > read that is not meant to request it.
> 
> ok... I accept the fact that read that is locking
> so you are right here.
> 
> > 
> > > 
> > > > +
> > > > +	/* Spec says that mutex is acquired when status bit is
> > > > read as unset,
> > > > +	 * if set it should be read again after 500usec. Here
> > > > waiting for
> > > > +	 * 2msec or 4 tries before give up.
> > > > +	 */
> > > > +	if (__intel_wait_for_register(dev_priv, intel_dp-
> > > > > aux_ch_mutex_reg,
> > > > 
> > > > +				      DP_AUX_CH_MUTEX_STATUS,
> > > > 0,
> > > > 500, 2,
> > > > +				      NULL)) {
> > > 
> > > why not intel_wait_for_register?
> > > that seems cleaner...
> > 
> > intel_wait_for_register() will call __intel_wait_for_register()
> > with
> > fast_timeout_us set as 2usec and specs says we should wait 500usec.
> 
> So, I've just noticed that __intel_wait_for_register spreaded to
> intel_hdcp.
> Ideally if we have __ prefix this shouldn't be called for more than 1
> place.
> 
> So it seems that we have more cases hence a rework should be done
> here.
> 
> But also the fast case is the atomic initial case... it is the first
> read.
> Only if it fails it will consider the slow case. That's exactly what
> spec tells
> 
> Maybe what we want here is a intel_wait_for_register_us to have a
> slow value
> based in us rather than ms?
> 
> I'm not fully sure here.


Reading again the implementation it don't do exactly what I was
expecting.
It will check the register for the expected value until fast_timeout_us
without sleeping and if not meet the expected value it will wait until
slow_timeout_ms sleeping from 10usec to 1000usec increasing at each
iteration.

So I guess I can use the regular intel_wait_for_register() with
timeout_ms=2msec, or there is any other suggestion?

> 
> > 
> > > 
> > > > +		DRM_WARN("dp_aux_ch port locked for too
> > > > long");
> > > > +		return false;
> > > > +	}
> > > > +
> > > > +	return true;
> > > > +}
> > > > +
> > > > +static void intel_dp_aux_ch_unlock(struct intel_dp *intel_dp)
> > > > +{
> > > > +	struct intel_digital_port *intel_dig_port =
> > > > dp_to_dig_port(intel_dp);
> > > > +	struct drm_i915_private *dev_priv =
> > > > +			to_i915(intel_dig_port-
> > > > >base.base.dev);
> > > > +
> > > > +	if (INTEL_GEN(dev_priv) < 9)
> > > > +		return;
> > > > +
> > > > +	/* setting the status bit releases the mutex + keep
> > > > mutex
> > > > enabled */
> > > > +	I915_WRITE(intel_dp->aux_ch_mutex_reg,
> > > > DP_AUX_CH_MUTEX_ENABLE |
> > > > +		   DP_AUX_CH_MUTEX_STATUS);
> > > 
> > > ditto.
> > > 
> > > > +}
> > > > +
> > > >  static int
> > > >  intel_dp_aux_ch(struct intel_dp *intel_dp,
> > > >  		const uint8_t *send, int send_bytes,
> > > > @@ -1115,6 +1154,11 @@ intel_dp_aux_ch(struct intel_dp
> > > > *intel_dp,
> > > >  
> > > >  	intel_dp_check_edp(intel_dp);
> > > >  
> > > > +	if (!intel_dp_aux_ch_trylock(intel_dp)) {
> > > > +		ret = -EBUSY;
> > > > +		goto out_locked;
> > > > +	}
> > > > +
> > > 
> > > BSpec: 4301
> > > I believe you could reduce the lock region following the spec
> > > and limiting it to steps 5-to-10, i.e.
> > > inside the loop below.
> > 
> > We loop trying to complete the transaction but we copy data
> > received
> > out of the loop and that is step 9, so the lock must be held until
> > we
> > copy the data from intel_dp->aux_ch_data_reg[] to recv.
> > 
> > "
> > 	for (i = 0; i < recv_bytes; i += 4)
> > 		intel_dp_unpack_aux(I915_READ(intel_dp-
> > >aux_ch_data_reg[i >>
> > 2]), recv + i, recv_bytes - i);
> > 
> > 	ret = recv_bytes;
> > out:
> > 	intel_dp_aux_ch_unlock(intel_dp);
> > "
> 
> hm... ok... this is the actually part 9 that needs to be inside
> the locked area indeed. I missed that.
> 
> > 
> > > 
> > > >  	/* Try to wait for any previous AUX channel activity
> > > > */
> > > >  	for (try = 0; try < 3; try++) {
> > > >  		status = I915_READ_NOTRACE(ch_ctl);
> > > > @@ -1244,6 +1288,8 @@ intel_dp_aux_ch(struct intel_dp
> > > > *intel_dp,
> > > >  
> > > >  	ret = recv_bytes;
> > > >  out:
> > > > +	intel_dp_aux_ch_unlock(intel_dp);
> > > > +out_locked:
> > > >  	pm_qos_update_request(&dev_priv->pm_qos,
> > > > PM_QOS_DEFAULT_VALUE);
> > > >  
> > > >  	if (vdd)
> > > > @@ -1496,6 +1542,10 @@ static void intel_aux_reg_init(struct
> > > > intel_dp *intel_dp)
> > > >  	intel_dp->aux_ch_ctl_reg = intel_aux_ctl_reg(dev_priv,
> > > > port);
> > > >  	for (i = 0; i < ARRAY_SIZE(intel_dp->aux_ch_data_reg);
> > > > i++)
> > > >  		intel_dp->aux_ch_data_reg[i] =
> > > > intel_aux_data_reg(dev_priv, port, i);
> > > > +	if (INTEL_INFO(dev_priv)->gen >= 9) {
> > > > +		intel_dp->aux_ch_mutex_reg =
> > > > DP_AUX_CH_MUTEX(port);
> > > > +		I915_WRITE(intel_dp->aux_ch_mutex_reg,
> > > > DP_AUX_CH_MUTEX_ENABLE);
> > > 
> > > I don't believe we need an extra enable here. And hopefully with
> > > Villes
> > > rework we don't even need to define here but only accessing it
> > > directly from
> > > the aux function with intel_dp->aux_ch reference.
> > 
> > What if hardware wants to do some aux transaction before we do our
> > first transaction(and enabling the mutex)? The mutex will not be
> > enabled so hardware will not lock it.
> > 
> > > 
> > > > +	}
> > > >  }
> > > >  
> > > >  static void
> > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > > b/drivers/gpu/drm/i915/intel_drv.h
> > > > index 50874f4035cf..0c88657d6066 100644
> > > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > > @@ -1043,6 +1043,7 @@ struct intel_dp {
> > > >  	i915_reg_t output_reg;
> > > >  	i915_reg_t aux_ch_ctl_reg;
> > > >  	i915_reg_t aux_ch_data_reg[5];
> > > > +	i915_reg_t aux_ch_mutex_reg;
> > > >  	uint32_t DP;
> > > >  	int link_rate;
> > > >  	uint8_t lane_count;
> > > > -- 
> > > > 2.16.2
> > > > 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx