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

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

[José Roberto de Souza]

v2
- 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
v3
- rebased on top of Ville's AUX series
- moved port registers to above DP_AUX_CH_MUTEX()
- using intel_wait_for_register() instead of the internal version

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

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

-- 
2.16.2

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

[PATCH 1/1] drm/i915/skl+: 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.
Older gen handling will be done separated.

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>
Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Cc: Ville Syrjälä <ville.syrjala@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  | 67 ++++++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h |  1 +
 3 files changed, 77 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index eea5b2c537d4..f36e839b4b4f 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -5385,6 +5385,15 @@ 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 _DPA_AUX_CH_MUTEX	(dev_priv->info.display_mmio_offset + 0x6402C)
+#define _DPB_AUX_CH_MUTEX	(dev_priv->info.display_mmio_offset + 0x6412C)
+#define _DPC_AUX_CH_MUTEX	(dev_priv->info.display_mmio_offset + 0x6422C)
+#define _DPD_AUX_CH_MUTEX	(dev_priv->info.display_mmio_offset + 0x6432C)
+#define _DPF_AUX_CH_MUTEX	(dev_priv->info.display_mmio_offset + 0x6452C)
+#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 2c3eb90b9499..7be2fec51651 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);
+	i915_reg_t ch_mutex;
+
+	if (!intel_dp->aux_ch_mutex_reg)
+		return true;
+
+	ch_mutex = intel_dp->aux_ch_mutex_reg(intel_dp);
+	I915_WRITE(ch_mutex, DP_AUX_CH_MUTEX_ENABLE);
+
+	/* Spec says that mutex is acquired when status bit is read as unset,
+	 * here waiting for 2msec or 4 tries before give up.
+	 */
+	if (intel_wait_for_register(dev_priv, ch_mutex, DP_AUX_CH_MUTEX_STATUS,
+				    0, 2)) {
+		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_dp->aux_ch_mutex_reg)
+		return;
+
+	/* setting the status bit releases the mutex + keep mutex enabled */
+	I915_WRITE(intel_dp->aux_ch_mutex_reg(intel_dp),
+		   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,
@@ -1119,6 +1158,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);
@@ -1248,6 +1292,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)
@@ -1504,6 +1550,24 @@ static i915_reg_t skl_aux_data_reg(struct intel_dp *intel_dp, int index)
 	}
 }
 
+static i915_reg_t skl_aux_mutex_reg(struct intel_dp *intel_dp)
+{
+	struct drm_i915_private *dev_priv = to_i915(intel_dp_to_dev(intel_dp));
+	enum aux_ch aux_ch = intel_dp->aux_ch;
+
+	switch (aux_ch) {
+	case AUX_CH_A:
+	case AUX_CH_B:
+	case AUX_CH_C:
+	case AUX_CH_D:
+	case AUX_CH_F:
+		return DP_AUX_CH_MUTEX(aux_ch);
+	default:
+		MISSING_CASE(aux_ch);
+		return DP_AUX_CH_MUTEX(AUX_CH_A);
+	}
+}
+
 static void
 intel_dp_aux_fini(struct intel_dp *intel_dp)
 {
@@ -1544,6 +1608,9 @@ intel_dp_aux_init(struct intel_dp *intel_dp)
 	else
 		intel_dp->get_aux_send_ctl = g4x_get_aux_send_ctl;
 
+	if (INTEL_GEN(dev_priv) >= 9)
+		intel_dp->aux_ch_mutex_reg = skl_aux_mutex_reg;
+
 	drm_dp_aux_init(&intel_dp->aux);
 
 	/* Failure to allocate our preferred name is not critical */
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 8f38e584d375..267cc6c5a89f 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1128,6 +1128,7 @@ struct intel_dp {
 
 	i915_reg_t (*aux_ch_ctl_reg)(struct intel_dp *dp);
 	i915_reg_t (*aux_ch_data_reg)(struct intel_dp *dp, int index);
+	i915_reg_t (*aux_ch_mutex_reg)(struct intel_dp *dp);
 
 	/* This is called before a link training is starterd */
 	void (*prepare_link_retrain)(struct intel_dp *intel_dp);
-- 
2.16.2

_______________________________________________
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 (rev2)

[Patchwork]

== Series Details ==

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

== Summary ==

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

Test kms_chamelium:
        Subgroup dp-edid-read:
                fail       -> PASS       (fi-kbl-7500u) fdo#102505
Test kms_force_connector_basic:
        Subgroup force-connector-state:
                skip       -> PASS       (fi-snb-2520m)
        Subgroup force-edid:
                skip       -> PASS       (fi-snb-2520m)
        Subgroup force-load-detect:
                skip       -> PASS       (fi-snb-2520m)
        Subgroup prune-stale-modes:
                skip       -> PASS       (fi-snb-2520m)
Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-c:
                incomplete -> PASS       (fi-bxt-dsi) fdo#103927

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

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:438s
fi-blb-e6850     total:288  pass:223  dwarn:1   dfail:0   fail:0   skip:64  time:375s
fi-bsw-n3050     total:288  pass:242  dwarn:0   dfail:0   fail:0   skip:46  time:490s
fi-bwr-2160      total:288  pass:183  dwarn:0   dfail:0   fail:0   skip:105 time:285s
fi-bxt-dsi       total:288  pass:258  dwarn:0   dfail:0   fail:0   skip:30  time:486s
fi-bxt-j4205     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:481s
fi-byt-j1900     total:288  pass:253  dwarn:0   dfail:0   fail:0   skip:35  time:469s
fi-byt-n2820     total:288  pass:249  dwarn:0   dfail:0   fail:0   skip:39  time:466s
fi-cfl-8700k     total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:391s
fi-cfl-s2        total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:568s
fi-cnl-y3        total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:573s
fi-elk-e7500     total:288  pass:229  dwarn:0   dfail:0   fail:0   skip:59  time:416s
fi-gdg-551       total:288  pass:179  dwarn:0   dfail:0   fail:1   skip:108 time:286s
fi-glk-1         total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:505s
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:410s
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:412s
fi-kbl-7500u     total:288  pass:263  dwarn:1   dfail:0   fail:0   skip:24  time:453s
fi-kbl-7560u     total:288  pass:269  dwarn:0   dfail:0   fail:0   skip:19  time:494s
fi-kbl-7567u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:447s
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:595s
fi-skl-6260u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:425s
fi-skl-6600u     total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:502s
fi-skl-6700hq    total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:522s
fi-skl-6700k2    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:491s
fi-skl-6770hq    total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:468s
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:509s
fi-snb-2600      total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:391s

316ba650abe6c1e8ac2f812ff21eee5771546ba1 drm-tip: 2018y-02m-23d-16h-41m-52s UTC integration manifest
41b1cc01d6ec drm/i915/skl+: Add and enable DP AUX CH mutex

== Logs ==

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

✓ Fi.CI.IGT: success for drm/i915: Add and enable DP AUX CH mutex (rev2)

[Patchwork]

== Series Details ==

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

== Summary ==

Test kms_flip:
        Subgroup modeset-vs-vblank-race:
                pass       -> FAIL       (shard-hsw) fdo#103060
        Subgroup plain-flip-ts-check:
                pass       -> FAIL       (shard-hsw) fdo#100368
Test kms_chv_cursor_fail:
        Subgroup pipe-a-64x64-top-edge:
                fail       -> PASS       (shard-apl)
Test drv_suspend:
        Subgroup fence-restore-tiled2untiled:
                skip       -> PASS       (shard-hsw)
Test gem_eio:
        Subgroup in-flight:
                pass       -> INCOMPLETE (shard-apl) fdo#104945
Test kms_plane:
        Subgroup plane-position-hole-dpms-pipe-b-planes:
                fail       -> PASS       (shard-apl)
Test kms_sysfs_edid_timing:
                pass       -> WARN       (shard-apl) fdo#100047

fdo#103060 https://bugs.freedesktop.org/show_bug.cgi?id=103060
fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
fdo#104945 https://bugs.freedesktop.org/show_bug.cgi?id=104945
fdo#100047 https://bugs.freedesktop.org/show_bug.cgi?id=100047

shard-apl        total:3457 pass:1816 dwarn:1   dfail:0   fail:12  skip:1626 time:11859s
shard-hsw        total:3465 pass:1766 dwarn:1   dfail:0   fail:4   skip:1693 time:11804s
shard-snb        total:3465 pass:1358 dwarn:1   dfail:0   fail:2   skip:2104 time:6661s
Blacklisted hosts:
shard-kbl        total:3386 pass:1908 dwarn:10  dfail:0   fail:14  skip:1452 time:9221s

== Logs ==

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

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

[Pandiyan, Dhinakaran]

On Fri, 2018-02-23 at 17:51 -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.
> Older gen handling will be done separated.
> 
> 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>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@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  | 67 ++++++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h |  1 +
>  3 files changed, 77 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index eea5b2c537d4..f36e839b4b4f 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -5385,6 +5385,15 @@ 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 _DPA_AUX_CH_MUTEX	(dev_priv->info.display_mmio_offset + 0x6402C)
> +#define _DPB_AUX_CH_MUTEX	(dev_priv->info.display_mmio_offset + 0x6412C)
> +#define _DPC_AUX_CH_MUTEX	(dev_priv->info.display_mmio_offset + 0x6422C)
> +#define _DPD_AUX_CH_MUTEX	(dev_priv->info.display_mmio_offset + 0x6432C)
> +#define _DPF_AUX_CH_MUTEX	(dev_priv->info.display_mmio_offset + 0x6452C)
> +#define DP_AUX_CH_MUTEX(port)	_MMIO_PORT(port, _DPA_AUX_CH_MUTEX, _DPB_AUX_CH_MUTEX)
			  ^aux_ch similar to ctl and data.

> +#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 2c3eb90b9499..7be2fec51651 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);
> +	i915_reg_t ch_mutex;
> +
> +	if (!intel_dp->aux_ch_mutex_reg)
> +		return true;
> +
> +	ch_mutex = intel_dp->aux_ch_mutex_reg(intel_dp);
> +	I915_WRITE(ch_mutex, 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.
> 

I do not agree with the interpretation here, reading the register
*after* the mutex is enabled == request for locking. You can read the
register before enabling, and you have to read so that you don't
overwrite any other bit.

Ref: "Sticky bit set to 1 after a read to this register when Mutex is
enabled."


> +
> +	/* Spec says that mutex is acquired when status bit is read as unset,
> +	 * here waiting for 2msec or 4 tries before give up.
			     2 ms.    ^ this is not true

> +	 */
> +	if (intel_wait_for_register(dev_priv, ch_mutex, DP_AUX_CH_MUTEX_STATUS,
> +				    0, 2)) {
> +		DRM_WARN("dp_aux_ch port locked for too long");
		 DRM_DEBUG_KMS("aux channel %c locked for 2 ms, timing out\n");

1. DRM_DEBUG_KMS is the convention in this file and most parts of the
driver for things like this.
2. I prefer to add details like port/channel/pipe/connector when
printing debug messages if it doesn't cost any extra space.


 
> +		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_dp->aux_ch_mutex_reg)
> +		return;
> +
> +	/* setting the status bit releases the mutex + keep mutex enabled */
> +	I915_WRITE(intel_dp->aux_ch_mutex_reg(intel_dp),
> +		   DP_AUX_CH_MUTEX_ENABLE | DP_AUX_CH_MUTEX_STATUS);

If you are leaving the mutex enabled, you don't have to enable it for
the second transaction onwards. In that case, why not move the mutex
enabling step to intel_dp_aux_init() after checking for PSR support or
enable the mutex just before intel_psr_enable(). Please get an alternate
opinion on this.


I think there are two options:
1)
Enable mutex before psr_enable
Read status (== request for lock)
	aux_xfer
Write status(== release lock)
Disable mutex after psr_disable

2)
For every aux transaction.
Enable mutex (RMW is okay because mutex is not enabled)
Read status (== request for lock)
	aux_xfer
Write status(==  release lock)
Disable mutex

As of now, you end up writing to the status bit in
intel_dp_aux_ch_trylock(), which looks wrong.


-DK


> +}
> +
>  static int
>  intel_dp_aux_ch(struct intel_dp *intel_dp,
>  		const uint8_t *send, int send_bytes,
> @@ -1119,6 +1158,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);
> @@ -1248,6 +1292,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)
> @@ -1504,6 +1550,24 @@ static i915_reg_t skl_aux_data_reg(struct intel_dp *intel_dp, int index)
>  	}
>  }
>  
> +static i915_reg_t skl_aux_mutex_reg(struct intel_dp *intel_dp)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(intel_dp_to_dev(intel_dp));
> +	enum aux_ch aux_ch = intel_dp->aux_ch;
> +
> +	switch (aux_ch) {
> +	case AUX_CH_A:
> +	case AUX_CH_B:
> +	case AUX_CH_C:
> +	case AUX_CH_D:
> +	case AUX_CH_F:
> +		return DP_AUX_CH_MUTEX(aux_ch);
> +	default:
> +		MISSING_CASE(aux_ch);
> +		return DP_AUX_CH_MUTEX(AUX_CH_A);
> +	}
> +}
> +
>  static void
>  intel_dp_aux_fini(struct intel_dp *intel_dp)
>  {
> @@ -1544,6 +1608,9 @@ intel_dp_aux_init(struct intel_dp *intel_dp)
>  	else
>  		intel_dp->get_aux_send_ctl = g4x_get_aux_send_ctl;
>  
> +	if (INTEL_GEN(dev_priv) >= 9)
> +		intel_dp->aux_ch_mutex_reg = skl_aux_mutex_reg;
> +

Move this to the branch where control and data vfuncs are setup.

>  	drm_dp_aux_init(&intel_dp->aux);
>  
>  	/* Failure to allocate our preferred name is not critical */
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 8f38e584d375..267cc6c5a89f 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1128,6 +1128,7 @@ struct intel_dp {
>  
>  	i915_reg_t (*aux_ch_ctl_reg)(struct intel_dp *dp);
>  	i915_reg_t (*aux_ch_data_reg)(struct intel_dp *dp, int index);
> +	i915_reg_t (*aux_ch_mutex_reg)(struct intel_dp *dp);
>  
>  	/* This is called before a link training is starterd */
>  	void (*prepare_link_retrain)(struct intel_dp *intel_dp);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Souza, Jose]

On Fri, 2018-02-23 at 19:12 -0800, Pandiyan, Dhinakaran wrote:
> On Fri, 2018-02-23 at 17:51 -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.
> > Older gen handling will be done separated.
> > 
> > Reference: https://01.org/sites/default/files/documentation/intel-g
> > fx-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>
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Cc: Ville Syrjälä <ville.syrjala@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  | 67
> > ++++++++++++++++++++++++++++++++++++++++
> >  drivers/gpu/drm/i915/intel_drv.h |  1 +
> >  3 files changed, 77 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > b/drivers/gpu/drm/i915/i915_reg.h
> > index eea5b2c537d4..f36e839b4b4f 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -5385,6 +5385,15 @@ 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 _DPA_AUX_CH_MUTEX	(dev_priv-
> > >info.display_mmio_offset + 0x6402C)
> > +#define _DPB_AUX_CH_MUTEX	(dev_priv-
> > >info.display_mmio_offset + 0x6412C)
> > +#define _DPC_AUX_CH_MUTEX	(dev_priv-
> > >info.display_mmio_offset + 0x6422C)
> > +#define _DPD_AUX_CH_MUTEX	(dev_priv-
> > >info.display_mmio_offset + 0x6432C)
> > +#define _DPF_AUX_CH_MUTEX	(dev_priv-
> > >info.display_mmio_offset + 0x6452C)
> > +#define DP_AUX_CH_MUTEX(port)	_MMIO_PORT(port,
> > _DPA_AUX_CH_MUTEX, _DPB_AUX_CH_MUTEX)
> 
> 			  ^aux_ch similar to ctl and data.
> 
> > +#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 2c3eb90b9499..7be2fec51651 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);
> > +	i915_reg_t ch_mutex;
> > +
> > +	if (!intel_dp->aux_ch_mutex_reg)
> > +		return true;
> > +
> > +	ch_mutex = intel_dp->aux_ch_mutex_reg(intel_dp);
> > +	I915_WRITE(ch_mutex, 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.
> > 
> 
> I do not agree with the interpretation here, reading the register
> *after* the mutex is enabled == request for locking. You can read the
> register before enabling, and you have to read so that you don't
> overwrite any other bit.
> 
> Ref: "Sticky bit set to 1 after a read to this register when Mutex is
> enabled."

This is true but we must keep the mutex enabled all the time to
guarantee that hardware will request lock too, so any read to the
register from our side will request the lock.

> 
> 
> > +
> > +	/* Spec says that mutex is acquired when status bit is
> > read as unset,
> > +	 * here waiting for 2msec or 4 tries before give up.
> 
> 			     2 ms.    ^ this is not true
> 
> > +	 */
> > +	if (intel_wait_for_register(dev_priv, ch_mutex,
> > DP_AUX_CH_MUTEX_STATUS,
> > +				    0, 2)) {
> > +		DRM_WARN("dp_aux_ch port locked for too long");
> 
> 		 DRM_DEBUG_KMS("aux channel %c locked for 2 ms, timing
> out\n");
> 
> 1. DRM_DEBUG_KMS is the convention in this file and most parts of the
> driver for things like this.

I'm okay in change I was just using DRM_WARN because most of the
messages in intel_dp_aux_ch() is using it.

> 2. I prefer to add details like port/channel/pipe/connector when
> printing debug messages if it doesn't cost any extra space.
> 
> 
>  
> > +		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_dp->aux_ch_mutex_reg)
> > +		return;
> > +
> > +	/* setting the status bit releases the mutex + keep mutex
> > enabled */
> > +	I915_WRITE(intel_dp->aux_ch_mutex_reg(intel_dp),
> > +		   DP_AUX_CH_MUTEX_ENABLE |
> > DP_AUX_CH_MUTEX_STATUS);
> 
> If you are leaving the mutex enabled, you don't have to enable it for
> the second transaction onwards. In that case, why not move the mutex
> enabling step to intel_dp_aux_init() after checking for PSR support
> or
> enable the mutex just before intel_psr_enable(). Please get an
> alternate
> opinion on this.
> 
> 
> I think there are two options:
> 1)
> Enable mutex before psr_enable
> Read status (== request for lock)
> 	aux_xfer
> Write status(== release lock)
> Disable mutex after psr_disable

The mutex will also be needed when enabling GTC and the cost of always
leave enabled is so low that is not worthy check if we have PSR enabled
in the port.

> 
> 2)
> For every aux transaction.
> Enable mutex (RMW is okay because mutex is not enabled)
> Read status (== request for lock)
> 	aux_xfer
> Write status(==  release lock)
> Disable mutex

Enable at each aux transaction is wrong, this way hardware will not
request the lock when it is disabled.

> 
> As of now, you end up writing to the status bit in
> intel_dp_aux_ch_trylock(), which looks wrong.

Vivi belives tha we need to poke registers before read it, in my tests
I can detect that hardware is helding the mutex with or without
asserting enable bit in intel_dp_aux_ch_trylock(), I'm fine with both
ways.


> 
> 
> -DK
> 
> 
> > +}
> > +
> >  static int
> >  intel_dp_aux_ch(struct intel_dp *intel_dp,
> >  		const uint8_t *send, int send_bytes,
> > @@ -1119,6 +1158,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);
> > @@ -1248,6 +1292,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)
> > @@ -1504,6 +1550,24 @@ static i915_reg_t skl_aux_data_reg(struct
> > intel_dp *intel_dp, int index)
> >  	}
> >  }
> >  
> > +static i915_reg_t skl_aux_mutex_reg(struct intel_dp *intel_dp)
> > +{
> > +	struct drm_i915_private *dev_priv =
> > to_i915(intel_dp_to_dev(intel_dp));
> > +	enum aux_ch aux_ch = intel_dp->aux_ch;
> > +
> > +	switch (aux_ch) {
> > +	case AUX_CH_A:
> > +	case AUX_CH_B:
> > +	case AUX_CH_C:
> > +	case AUX_CH_D:
> > +	case AUX_CH_F:
> > +		return DP_AUX_CH_MUTEX(aux_ch);
> > +	default:
> > +		MISSING_CASE(aux_ch);
> > +		return DP_AUX_CH_MUTEX(AUX_CH_A);
> > +	}
> > +}
> > +
> >  static void
> >  intel_dp_aux_fini(struct intel_dp *intel_dp)
> >  {
> > @@ -1544,6 +1608,9 @@ intel_dp_aux_init(struct intel_dp *intel_dp)
> >  	else
> >  		intel_dp->get_aux_send_ctl = g4x_get_aux_send_ctl;
> >  
> > +	if (INTEL_GEN(dev_priv) >= 9)
> > +		intel_dp->aux_ch_mutex_reg = skl_aux_mutex_reg;
> > +
> 
> Move this to the branch where control and data vfuncs are setup.
> 
> >  	drm_dp_aux_init(&intel_dp->aux);
> >  
> >  	/* Failure to allocate our preferred name is not critical
> > */
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > b/drivers/gpu/drm/i915/intel_drv.h
> > index 8f38e584d375..267cc6c5a89f 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1128,6 +1128,7 @@ struct intel_dp {
> >  
> >  	i915_reg_t (*aux_ch_ctl_reg)(struct intel_dp *dp);
> >  	i915_reg_t (*aux_ch_data_reg)(struct intel_dp *dp, int
> > index);
> > +	i915_reg_t (*aux_ch_mutex_reg)(struct intel_dp *dp);
> >  
> >  	/* This is called before a link training is starterd */
> >  	void (*prepare_link_retrain)(struct intel_dp *intel_dp);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Ville Syrjälä]

On Fri, Feb 23, 2018 at 05:51:40PM -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.
> Older gen handling will be done separated.
> 
> 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>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@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  | 67 ++++++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h |  1 +
>  3 files changed, 77 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index eea5b2c537d4..f36e839b4b4f 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -5385,6 +5385,15 @@ 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 _DPA_AUX_CH_MUTEX	(dev_priv->info.display_mmio_offset + 0x6402C)
> +#define _DPB_AUX_CH_MUTEX	(dev_priv->info.display_mmio_offset + 0x6412C)
> +#define _DPC_AUX_CH_MUTEX	(dev_priv->info.display_mmio_offset + 0x6422C)
> +#define _DPD_AUX_CH_MUTEX	(dev_priv->info.display_mmio_offset + 0x6432C)
> +#define _DPF_AUX_CH_MUTEX	(dev_priv->info.display_mmio_offset + 0x6452C)
> +#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 2c3eb90b9499..7be2fec51651 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);
> +	i915_reg_t ch_mutex;
> +
> +	if (!intel_dp->aux_ch_mutex_reg)
> +		return true;
> +
> +	ch_mutex = intel_dp->aux_ch_mutex_reg(intel_dp);
> +	I915_WRITE(ch_mutex, DP_AUX_CH_MUTEX_ENABLE);
> +
> +	/* Spec says that mutex is acquired when status bit is read as unset,
> +	 * here waiting for 2msec or 4 tries before give up.
> +	 */
> +	if (intel_wait_for_register(dev_priv, ch_mutex, DP_AUX_CH_MUTEX_STATUS,
> +				    0, 2)) {
> +		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_dp->aux_ch_mutex_reg)
> +		return;
> +
> +	/* setting the status bit releases the mutex + keep mutex enabled */
> +	I915_WRITE(intel_dp->aux_ch_mutex_reg(intel_dp),
> +		   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,
> @@ -1119,6 +1158,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);
> @@ -1248,6 +1292,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)
> @@ -1504,6 +1550,24 @@ static i915_reg_t skl_aux_data_reg(struct intel_dp *intel_dp, int index)
>  	}
>  }
>  
> +static i915_reg_t skl_aux_mutex_reg(struct intel_dp *intel_dp)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(intel_dp_to_dev(intel_dp));
> +	enum aux_ch aux_ch = intel_dp->aux_ch;
> +
> +	switch (aux_ch) {
> +	case AUX_CH_A:
> +	case AUX_CH_B:
> +	case AUX_CH_C:
> +	case AUX_CH_D:
> +	case AUX_CH_F:
> +		return DP_AUX_CH_MUTEX(aux_ch);
> +	default:
> +		MISSING_CASE(aux_ch);
> +		return DP_AUX_CH_MUTEX(AUX_CH_A);
> +	}
> +}

There is no AUX mutex on pre-SKL, so abstracting the register offset
with a vfunc is pointless.

> +
>  static void
>  intel_dp_aux_fini(struct intel_dp *intel_dp)
>  {
> @@ -1544,6 +1608,9 @@ intel_dp_aux_init(struct intel_dp *intel_dp)
>  	else
>  		intel_dp->get_aux_send_ctl = g4x_get_aux_send_ctl;
>  
> +	if (INTEL_GEN(dev_priv) >= 9)
> +		intel_dp->aux_ch_mutex_reg = skl_aux_mutex_reg;
> +
>  	drm_dp_aux_init(&intel_dp->aux);
>  
>  	/* Failure to allocate our preferred name is not critical */
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 8f38e584d375..267cc6c5a89f 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1128,6 +1128,7 @@ struct intel_dp {
>  
>  	i915_reg_t (*aux_ch_ctl_reg)(struct intel_dp *dp);
>  	i915_reg_t (*aux_ch_data_reg)(struct intel_dp *dp, int index);
> +	i915_reg_t (*aux_ch_mutex_reg)(struct intel_dp *dp);
>  
>  	/* This is called before a link training is starterd */
>  	void (*prepare_link_retrain)(struct intel_dp *intel_dp);
> -- 
> 2.16.2

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

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

[Pandiyan, Dhinakaran]

On Mon, 2018-02-26 at 18:31 +0000, Souza, Jose wrote:
> On Fri, 2018-02-23 at 19:12 -0800, Pandiyan, Dhinakaran wrote:
> > On Fri, 2018-02-23 at 17:51 -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.
> > > Older gen handling will be done separated.
> > > 
> > > Reference: https://01.org/sites/default/files/documentation/intel-g
> > > fx-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>
> > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > Cc: Ville Syrjälä <ville.syrjala@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  | 67
> > > ++++++++++++++++++++++++++++++++++++++++
> > >  drivers/gpu/drm/i915/intel_drv.h |  1 +
> > >  3 files changed, 77 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > > b/drivers/gpu/drm/i915/i915_reg.h
> > > index eea5b2c537d4..f36e839b4b4f 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -5385,6 +5385,15 @@ 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 _DPA_AUX_CH_MUTEX	(dev_priv-
> > > >info.display_mmio_offset + 0x6402C)
> > > +#define _DPB_AUX_CH_MUTEX	(dev_priv-
> > > >info.display_mmio_offset + 0x6412C)
> > > +#define _DPC_AUX_CH_MUTEX	(dev_priv-
> > > >info.display_mmio_offset + 0x6422C)
> > > +#define _DPD_AUX_CH_MUTEX	(dev_priv-
> > > >info.display_mmio_offset + 0x6432C)
> > > +#define _DPF_AUX_CH_MUTEX	(dev_priv-
> > > >info.display_mmio_offset + 0x6452C)
> > > +#define DP_AUX_CH_MUTEX(port)	_MMIO_PORT(port,
> > > _DPA_AUX_CH_MUTEX, _DPB_AUX_CH_MUTEX)
> > 
> > 			  ^aux_ch similar to ctl and data.
> > 
> > > +#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 2c3eb90b9499..7be2fec51651 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);
> > > +	i915_reg_t ch_mutex;
> > > +
> > > +	if (!intel_dp->aux_ch_mutex_reg)
> > > +		return true;
> > > +
> > > +	ch_mutex = intel_dp->aux_ch_mutex_reg(intel_dp);
> > > +	I915_WRITE(ch_mutex, 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.
> > > 
> > 
> > I do not agree with the interpretation here, reading the register
> > *after* the mutex is enabled == request for locking. You can read the
> > register before enabling, and you have to read so that you don't
> > overwrite any other bit.
> > 
> > Ref: "Sticky bit set to 1 after a read to this register when Mutex is
> > enabled."
> 
> This is true but we must keep the mutex enabled all the time to
> guarantee that hardware will request lock too,

I think the status bit will still indicate busy if the HW started the
aux transaction before mutex was enabled. I think so because bspec
explicitly says to check the status bit right after enabling.


>  so any read to the
> register from our side will request the lock.
> 
> > 
> > 
> > > +
> > > +	/* Spec says that mutex is acquired when status bit is
> > > read as unset,
> > > +	 * here waiting for 2msec or 4 tries before give up.
> > 
> > 			     2 ms.    ^ this is not true
> > 
> > > +	 */
> > > +	if (intel_wait_for_register(dev_priv, ch_mutex,
> > > DP_AUX_CH_MUTEX_STATUS,
> > > +				    0, 2)) {
> > > +		DRM_WARN("dp_aux_ch port locked for too long");
> > 
> > 		 DRM_DEBUG_KMS("aux channel %c locked for 2 ms, timing
> > out\n");
> > 
> > 1. DRM_DEBUG_KMS is the convention in this file and most parts of the
> > driver for things like this.
> 
> I'm okay in change I was just using DRM_WARN because most of the
> messages in intel_dp_aux_ch() is using it.
> 
Might be worth checking how this plays with debug messages in the upper
layers. 


> > 2. I prefer to add details like port/channel/pipe/connector when
> > printing debug messages if it doesn't cost any extra space.
> > 
> > 
> >  
> > > +		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_dp->aux_ch_mutex_reg)
> > > +		return;
> > > +
> > > +	/* setting the status bit releases the mutex + keep mutex
> > > enabled */
> > > +	I915_WRITE(intel_dp->aux_ch_mutex_reg(intel_dp),
> > > +		   DP_AUX_CH_MUTEX_ENABLE |
> > > DP_AUX_CH_MUTEX_STATUS);
> > 
> > If you are leaving the mutex enabled, you don't have to enable it for
> > the second transaction onwards. In that case, why not move the mutex
> > enabling step to intel_dp_aux_init() after checking for PSR support
> > or
> > enable the mutex just before intel_psr_enable(). Please get an
> > alternate
> > opinion on this.
> > 
> > 
> > I think there are two options:
> > 1)
> > Enable mutex before psr_enable
> > Read status (== request for lock)
> > 	aux_xfer
> > Write status(== release lock)
> > Disable mutex after psr_disable
> 
> The mutex will also be needed when enabling GTC and the cost of always
> leave enabled is so low that is not worthy check if we have PSR enabled
> in the port.

Limiting the scope for changes to where they are required reduces bugs
IMO. However, if you want to do this unconditionally (without checking
for PSR) on all ports, aux_init() that Ville added might be a good
place.

> 
> > 
> > 2)
> > For every aux transaction.
> > Enable mutex (RMW is okay because mutex is not enabled)
> > Read status (== request for lock)
> > 	aux_xfer
> > Write status(==  release lock)
> > Disable mutex
> 
> Enable at each aux transaction is wrong, this way hardware will not
> request the lock when it is disabled.
> 
> > 
> > As of now, you end up writing to the status bit in
> > intel_dp_aux_ch_trylock(), which looks wrong.
> 
> Vivi belives tha we need to poke registers before read it, in my tests
> I can detect that hardware is helding the mutex with or without
> asserting enable bit in intel_dp_aux_ch_trylock(),

Doesn't this confirm my guess that enabling mutex per transaction is
okay? We'll still see status as 1.

Anyway, since we are leaning towards the "always enable mutex
approach".Please remove the enable step for every single transaction
that the driver initiates.

If the hardware had already started an aux transaction, status bit will
be 1. Now, if you go overwrite the register to enable the mutex, which
is already enabled second time onwards, you'll end-up 0'ing the status
bit.


>  I'm fine with both
> ways.
> 
> 
> > 
> > 
> > -DK
> > 
> > 
> > > +}
> > > +
> > >  static int
> > >  intel_dp_aux_ch(struct intel_dp *intel_dp,
> > >  		const uint8_t *send, int send_bytes,
> > > @@ -1119,6 +1158,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);
> > > @@ -1248,6 +1292,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)
> > > @@ -1504,6 +1550,24 @@ static i915_reg_t skl_aux_data_reg(struct
> > > intel_dp *intel_dp, int index)
> > >  	}
> > >  }
> > >  
> > > +static i915_reg_t skl_aux_mutex_reg(struct intel_dp *intel_dp)
> > > +{
> > > +	struct drm_i915_private *dev_priv =
> > > to_i915(intel_dp_to_dev(intel_dp));
> > > +	enum aux_ch aux_ch = intel_dp->aux_ch;
> > > +
> > > +	switch (aux_ch) {
> > > +	case AUX_CH_A:
> > > +	case AUX_CH_B:
> > > +	case AUX_CH_C:
> > > +	case AUX_CH_D:
> > > +	case AUX_CH_F:
> > > +		return DP_AUX_CH_MUTEX(aux_ch);
> > > +	default:
> > > +		MISSING_CASE(aux_ch);
> > > +		return DP_AUX_CH_MUTEX(AUX_CH_A);
> > > +	}
> > > +}
> > > +
> > >  static void
> > >  intel_dp_aux_fini(struct intel_dp *intel_dp)
> > >  {
> > > @@ -1544,6 +1608,9 @@ intel_dp_aux_init(struct intel_dp *intel_dp)
> > >  	else
> > >  		intel_dp->get_aux_send_ctl = g4x_get_aux_send_ctl;
> > >  
> > > +	if (INTEL_GEN(dev_priv) >= 9)
> > > +		intel_dp->aux_ch_mutex_reg = skl_aux_mutex_reg;
> > > +
> > 
> > Move this to the branch where control and data vfuncs are setup.
> > 
> > >  	drm_dp_aux_init(&intel_dp->aux);
> > >  
> > >  	/* Failure to allocate our preferred name is not critical
> > > */
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > b/drivers/gpu/drm/i915/intel_drv.h
> > > index 8f38e584d375..267cc6c5a89f 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -1128,6 +1128,7 @@ struct intel_dp {
> > >  
> > >  	i915_reg_t (*aux_ch_ctl_reg)(struct intel_dp *dp);
> > >  	i915_reg_t (*aux_ch_data_reg)(struct intel_dp *dp, int
> > > index);
> > > +	i915_reg_t (*aux_ch_mutex_reg)(struct intel_dp *dp);
> > >  
> > >  	/* This is called before a link training is starterd */
> > >  	void (*prepare_link_retrain)(struct intel_dp *intel_dp);
> _______________________________________________
> 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