* [PATCH] drm/i915/skl+: Add and enable DP AUX CH mutex
@ 2018-02-27 21:23 José Roberto de Souza
2018-02-27 21:34 ` Ville Syrjälä
2018-02-27 22:11 ` ✗ Fi.CI.BAT: warning for " Patchwork
0 siblings, 2 replies; 11+ messages in thread
From: José Roberto de Souza @ 2018-02-27 21:23 UTC (permalink / raw)
To: intel-gfx
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
Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
Changelog:
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
v4
- removed virtual function to get mutex register address
- enabling the mutex back only on aux channel init
- added the aux channel name to the timeout debug message
v5
- renamed DP_AUX_CH_MUTEX() parameter to aux_ch
- renamed goto label when intel_dp_aux_ch_trylock() fails
drivers/gpu/drm/i915/i915_reg.h | 9 ++++++++
drivers/gpu/drm/i915/intel_dp.c | 47 +++++++++++++++++++++++++++++++++++++++++
2 files changed, 56 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index eea5b2c537d4..bce2e6dad4c4 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(aux_ch) _MMIO_PORT(aux_ch, _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 2a3b3ae4e3da..7f4bf77227cd 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1081,6 +1081,42 @@ 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;
+
+ /* Spec says that mutex is acquired when status bit is read as unset,
+ * here waiting for 2msec(+-4 aux transactions) before give up.
+ */
+ if (intel_wait_for_register(dev_priv, DP_AUX_CH_MUTEX(intel_dp->aux_ch),
+ DP_AUX_CH_MUTEX_STATUS, 0, 2)) {
+ DRM_DEBUG_KMS("aux channel %c locked for 2msec, timing out\n",
+ aux_ch_name(intel_dp->aux_ch));
+ 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;
+
+ /* set the status bit releases the mutex + keeping mutex enabled */
+ I915_WRITE(DP_AUX_CH_MUTEX(intel_dp->aux_ch),
+ 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 +1155,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_aux_ch_unlocked;
+ }
+
/* Try to wait for any previous AUX channel activity */
for (try = 0; try < 3; try++) {
status = I915_READ_NOTRACE(ch_ctl);
@@ -1240,6 +1281,8 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
ret = recv_bytes;
out:
+ intel_dp_aux_ch_unlock(intel_dp);
+out_aux_ch_unlocked:
pm_qos_update_request(&dev_priv->pm_qos, PM_QOS_DEFAULT_VALUE);
if (vdd)
@@ -1536,6 +1579,10 @@ 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)
+ I915_WRITE(DP_AUX_CH_MUTEX(intel_dp->aux_ch),
+ DP_AUX_CH_MUTEX_ENABLE);
+
drm_dp_aux_init(&intel_dp->aux);
/* Failure to allocate our preferred name is not critical */
--
2.16.2
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] drm/i915/skl+: Add and enable DP AUX CH mutex
2018-02-27 21:23 [PATCH] drm/i915/skl+: Add and enable DP AUX CH mutex José Roberto de Souza
@ 2018-02-27 21:34 ` Ville Syrjälä
2018-02-27 23:54 ` Pandiyan, Dhinakaran
2018-02-28 0:57 ` Souza, Jose
2018-02-27 22:11 ` ✗ Fi.CI.BAT: warning for " Patchwork
1 sibling, 2 replies; 11+ messages in thread
From: Ville Syrjälä @ 2018-02-27 21:34 UTC (permalink / raw)
To: José Roberto de Souza; +Cc: intel-gfx
On Tue, Feb 27, 2018 at 01:23:59PM -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
>
> Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>
> Changelog:
> 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
> v4
> - removed virtual function to get mutex register address
> - enabling the mutex back only on aux channel init
> - added the aux channel name to the timeout debug message
> v5
> - renamed DP_AUX_CH_MUTEX() parameter to aux_ch
> - renamed goto label when intel_dp_aux_ch_trylock() fails
>
> drivers/gpu/drm/i915/i915_reg.h | 9 ++++++++
> drivers/gpu/drm/i915/intel_dp.c | 47 +++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 56 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index eea5b2c537d4..bce2e6dad4c4 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(aux_ch) _MMIO_PORT(aux_ch, _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 2a3b3ae4e3da..7f4bf77227cd 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1081,6 +1081,42 @@ 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;
> +
> + /* Spec says that mutex is acquired when status bit is read as unset,
> + * here waiting for 2msec(+-4 aux transactions) before give up.
> + */
> + if (intel_wait_for_register(dev_priv, DP_AUX_CH_MUTEX(intel_dp->aux_ch),
> + DP_AUX_CH_MUTEX_STATUS, 0, 2)) {
> + DRM_DEBUG_KMS("aux channel %c locked for 2msec, timing out\n",
> + aux_ch_name(intel_dp->aux_ch));
> + 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;
> +
> + /* set the status bit releases the mutex + keeping mutex enabled */
> + I915_WRITE(DP_AUX_CH_MUTEX(intel_dp->aux_ch),
> + 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 +1155,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_aux_ch_unlocked;
> + }
> +
> /* Try to wait for any previous AUX channel activity */
> for (try = 0; try < 3; try++) {
> status = I915_READ_NOTRACE(ch_ctl);
> @@ -1240,6 +1281,8 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
>
> ret = recv_bytes;
> out:
> + intel_dp_aux_ch_unlock(intel_dp);
> +out_aux_ch_unlocked:
> pm_qos_update_request(&dev_priv->pm_qos, PM_QOS_DEFAULT_VALUE);
>
> if (vdd)
> @@ -1536,6 +1579,10 @@ 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)
> + I915_WRITE(DP_AUX_CH_MUTEX(intel_dp->aux_ch),
> + DP_AUX_CH_MUTEX_ENABLE);
And who enables it after system/runtime PM etc.? Not sure how much sense
there is to finesse this anyway. One extra mmio write every time we try
to acquire the mutex shouldn't kill anyone.
> +
> drm_dp_aux_init(&intel_dp->aux);
>
> /* Failure to allocate our preferred name is not critical */
> --
> 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
^ permalink raw reply [flat|nested] 11+ messages in thread
* ✗ Fi.CI.BAT: warning for drm/i915/skl+: Add and enable DP AUX CH mutex
2018-02-27 21:23 [PATCH] drm/i915/skl+: Add and enable DP AUX CH mutex José Roberto de Souza
2018-02-27 21:34 ` Ville Syrjälä
@ 2018-02-27 22:11 ` Patchwork
1 sibling, 0 replies; 11+ messages in thread
From: Patchwork @ 2018-02-27 22:11 UTC (permalink / raw)
To: José Roberto de Souza; +Cc: intel-gfx
== Series Details ==
Series: drm/i915/skl+: Add and enable DP AUX CH mutex
URL : https://patchwork.freedesktop.org/series/39067/
State : warning
== Summary ==
Series 39067v1 drm/i915/skl+: Add and enable DP AUX CH mutex
https://patchwork.freedesktop.org/api/1.0/series/39067/revisions/1/mbox/
---- Possible new issues:
Test kms_force_connector_basic:
Subgroup force-connector-state:
pass -> SKIP (fi-ivb-3520m)
Subgroup force-edid:
pass -> SKIP (fi-ivb-3520m)
Subgroup force-load-detect:
pass -> SKIP (fi-ivb-3520m)
Subgroup prune-stale-modes:
pass -> SKIP (fi-ivb-3520m)
---- Known issues:
Test kms_pipe_crc_basic:
Subgroup suspend-read-crc-pipe-b:
pass -> DMESG-WARN (fi-cnl-y3) fdo#104951
fdo#104951 https://bugs.freedesktop.org/show_bug.cgi?id=104951
fi-bdw-5557u total:288 pass:267 dwarn:0 dfail:0 fail:0 skip:21 time:416s
fi-bdw-gvtdvm total:288 pass:264 dwarn:0 dfail:0 fail:0 skip:24 time:421s
fi-blb-e6850 total:288 pass:223 dwarn:1 dfail:0 fail:0 skip:64 time:372s
fi-bsw-n3050 total:288 pass:242 dwarn:0 dfail:0 fail:0 skip:46 time:483s
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:476s
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:470s
fi-byt-n2820 total:288 pass:249 dwarn:0 dfail:0 fail:0 skip:39 time:451s
fi-cfl-8700k total:288 pass:260 dwarn:0 dfail:0 fail:0 skip:28 time:396s
fi-cfl-s2 total:288 pass:262 dwarn:0 dfail:0 fail:0 skip:26 time:566s
fi-cnl-y3 total:288 pass:261 dwarn:1 dfail:0 fail:0 skip:26 time:567s
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:507s
fi-hsw-4770 total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:385s
fi-ilk-650 total:288 pass:228 dwarn:0 dfail:0 fail:0 skip:60 time:406s
fi-ivb-3520m total:288 pass:255 dwarn:0 dfail:0 fail:0 skip:33 time:442s
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:451s
fi-kbl-7560u total:288 pass:269 dwarn:0 dfail:0 fail:0 skip:19 time:493s
fi-kbl-7567u total:288 pass:268 dwarn:0 dfail:0 fail:0 skip:20 time:453s
fi-kbl-r total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:493s
fi-pnv-d510 total:288 pass:222 dwarn:1 dfail:0 fail:0 skip:65 time:578s
fi-skl-6260u total:288 pass:268 dwarn:0 dfail:0 fail:0 skip:20 time:428s
fi-skl-6600u total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:500s
fi-skl-6700hq total:288 pass:262 dwarn:0 dfail:0 fail:0 skip:26 time:517s
fi-skl-6700k2 total:288 pass:264 dwarn:0 dfail:0 fail:0 skip:24 time:481s
fi-skl-6770hq total:288 pass:268 dwarn:0 dfail:0 fail:0 skip:20 time:477s
fi-skl-guc total:288 pass:260 dwarn:0 dfail:0 fail:0 skip:28 time:405s
fi-skl-gvtdvm total:288 pass:265 dwarn:0 dfail:0 fail:0 skip:23 time:431s
fi-snb-2520m total:3 pass:2 dwarn:0 dfail:0 fail:0 skip:0
fi-snb-2600 total:288 pass:248 dwarn:0 dfail:0 fail:0 skip:40 time:391s
af8578c6d4389dd9a51ddccc43b8ce4986e27131 drm-tip: 2018y-02m-27d-20h-28m-22s UTC integration manifest
128fb513cbdd 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_8178/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] drm/i915/skl+: Add and enable DP AUX CH mutex
2018-02-27 21:34 ` Ville Syrjälä
@ 2018-02-27 23:54 ` Pandiyan, Dhinakaran
2018-02-28 0:57 ` Souza, Jose
1 sibling, 0 replies; 11+ messages in thread
From: Pandiyan, Dhinakaran @ 2018-02-27 23:54 UTC (permalink / raw)
To: ville.syrjala; +Cc: intel-gfx
On Tue, 2018-02-27 at 23:34 +0200, Ville Syrjälä wrote:
> On Tue, Feb 27, 2018 at 01:23:59PM -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
> >
> > Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> >
> > Changelog:
> > 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
> > v4
> > - removed virtual function to get mutex register address
> > - enabling the mutex back only on aux channel init
> > - added the aux channel name to the timeout debug message
> > v5
> > - renamed DP_AUX_CH_MUTEX() parameter to aux_ch
> > - renamed goto label when intel_dp_aux_ch_trylock() fails
> >
> > drivers/gpu/drm/i915/i915_reg.h | 9 ++++++++
> > drivers/gpu/drm/i915/intel_dp.c | 47 +++++++++++++++++++++++++++++++++++++++++
> > 2 files changed, 56 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index eea5b2c537d4..bce2e6dad4c4 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(aux_ch) _MMIO_PORT(aux_ch, _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 2a3b3ae4e3da..7f4bf77227cd 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -1081,6 +1081,42 @@ 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;
> > +
> > + /* Spec says that mutex is acquired when status bit is read as unset,
> > + * here waiting for 2msec(+-4 aux transactions) before give up.
> > + */
> > + if (intel_wait_for_register(dev_priv, DP_AUX_CH_MUTEX(intel_dp->aux_ch),
> > + DP_AUX_CH_MUTEX_STATUS, 0, 2)) {
> > + DRM_DEBUG_KMS("aux channel %c locked for 2msec, timing out\n",
> > + aux_ch_name(intel_dp->aux_ch));
> > + 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;
> > +
> > + /* set the status bit releases the mutex + keeping mutex enabled */
> > + I915_WRITE(DP_AUX_CH_MUTEX(intel_dp->aux_ch),
> > + 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 +1155,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_aux_ch_unlocked;
> > + }
> > +
> > /* Try to wait for any previous AUX channel activity */
> > for (try = 0; try < 3; try++) {
> > status = I915_READ_NOTRACE(ch_ctl);
> > @@ -1240,6 +1281,8 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
> >
> > ret = recv_bytes;
> > out:
> > + intel_dp_aux_ch_unlock(intel_dp);
> > +out_aux_ch_unlocked:
> > pm_qos_update_request(&dev_priv->pm_qos, PM_QOS_DEFAULT_VALUE);
> >
> > if (vdd)
> > @@ -1536,6 +1579,10 @@ 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)
> > + I915_WRITE(DP_AUX_CH_MUTEX(intel_dp->aux_ch),
> > + DP_AUX_CH_MUTEX_ENABLE);
>
> And who enables it after system/runtime PM etc.? Not sure how much sense
> there is to finesse this anyway. One extra mmio write every time we try
> to acquire the mutex shouldn't kill anyone.
>
The problem was with enabling the mutex without modifying the status
bit.
The earlier version
> + ch_mutex = intel_dp->aux_ch_mutex_reg(intel_dp);
> + I915_WRITE(ch_mutex, DP_AUX_CH_MUTEX_ENABLE)
had this in _trylock()
This overwrites the status bit.
> > +
> > drm_dp_aux_init(&intel_dp->aux);
> >
> > /* Failure to allocate our preferred name is not critical */
> > --
> > 2.16.2
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] drm/i915/skl+: Add and enable DP AUX CH mutex
2018-02-27 21:34 ` Ville Syrjälä
2018-02-27 23:54 ` Pandiyan, Dhinakaran
@ 2018-02-28 0:57 ` Souza, Jose
2018-02-28 11:09 ` Ville Syrjälä
1 sibling, 1 reply; 11+ messages in thread
From: Souza, Jose @ 2018-02-28 0:57 UTC (permalink / raw)
To: ville.syrjala; +Cc: intel-gfx
On Tue, 2018-02-27 at 23:34 +0200, Ville Syrjälä wrote:
> On Tue, Feb 27, 2018 at 01:23:59PM -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
> >
> > Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> >
> > Changelog:
> > 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
> > v4
> > - removed virtual function to get mutex register address
> > - enabling the mutex back only on aux channel init
> > - added the aux channel name to the timeout debug message
> > v5
> > - renamed DP_AUX_CH_MUTEX() parameter to aux_ch
> > - renamed goto label when intel_dp_aux_ch_trylock() fails
> >
> > drivers/gpu/drm/i915/i915_reg.h | 9 ++++++++
> > drivers/gpu/drm/i915/intel_dp.c | 47
> > +++++++++++++++++++++++++++++++++++++++++
> > 2 files changed, 56 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > b/drivers/gpu/drm/i915/i915_reg.h
> > index eea5b2c537d4..bce2e6dad4c4 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(aux_ch) _MMIO_PORT(aux_ch,
> > _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 2a3b3ae4e3da..7f4bf77227cd 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -1081,6 +1081,42 @@ 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;
> > +
> > + /* Spec says that mutex is acquired when status bit is
> > read as unset,
> > + * here waiting for 2msec(+-4 aux transactions) before
> > give up.
> > + */
> > + if (intel_wait_for_register(dev_priv,
> > DP_AUX_CH_MUTEX(intel_dp->aux_ch),
> > + DP_AUX_CH_MUTEX_STATUS, 0, 2))
> > {
> > + DRM_DEBUG_KMS("aux channel %c locked for 2msec,
> > timing out\n",
> > + aux_ch_name(intel_dp->aux_ch));
> > + 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;
> > +
> > + /* set the status bit releases the mutex + keeping mutex
> > enabled */
> > + I915_WRITE(DP_AUX_CH_MUTEX(intel_dp->aux_ch),
> > + 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 +1155,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_aux_ch_unlocked;
> > + }
> > +
> > /* Try to wait for any previous AUX channel activity */
> > for (try = 0; try < 3; try++) {
> > status = I915_READ_NOTRACE(ch_ctl);
> > @@ -1240,6 +1281,8 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
> >
> > ret = recv_bytes;
> > out:
> > + intel_dp_aux_ch_unlock(intel_dp);
> > +out_aux_ch_unlocked:
> > pm_qos_update_request(&dev_priv->pm_qos,
> > PM_QOS_DEFAULT_VALUE);
> >
> > if (vdd)
> > @@ -1536,6 +1579,10 @@ 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)
> > + I915_WRITE(DP_AUX_CH_MUTEX(intel_dp->aux_ch),
> > + DP_AUX_CH_MUTEX_ENABLE);
>
> And who enables it after system/runtime PM etc.? Not sure how much
> sense
> there is to finesse this anyway. One extra mmio write every time we
> try
> to acquire the mutex shouldn't kill anyone.
Valid point, but I guess is better enable the mutex in
intel_dp_encoder_reset():
"
@@ -5293,6 +5340,10 @@ void intel_dp_encoder_reset(struct drm_encoder
*encoder)
pps_lock(intel_dp);
+ if (INTEL_GEN(dev_priv) >= 9)
+ I915_WRITE(DP_AUX_CH_MUTEX(intel_dp->aux_ch),
+ DP_AUX_CH_MUTEX_ENABLE);
+
if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
intel_dp->active_pipe = vlv_active_pipe(intel_dp);
"
any objections?
>
> > +
> > drm_dp_aux_init(&intel_dp->aux);
> >
> > /* Failure to allocate our preferred name is not critical
> > */
> > --
> > 2.16.2
>
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] drm/i915/skl+: Add and enable DP AUX CH mutex
2018-02-28 0:57 ` Souza, Jose
@ 2018-02-28 11:09 ` Ville Syrjälä
2018-02-28 23:55 ` Souza, Jose
0 siblings, 1 reply; 11+ messages in thread
From: Ville Syrjälä @ 2018-02-28 11:09 UTC (permalink / raw)
To: Souza, Jose; +Cc: intel-gfx
On Wed, Feb 28, 2018 at 12:57:07AM +0000, Souza, Jose wrote:
> On Tue, 2018-02-27 at 23:34 +0200, Ville Syrjälä wrote:
> > On Tue, Feb 27, 2018 at 01:23:59PM -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
> > >
> > > Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > ---
> > >
> > > Changelog:
> > > 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
> > > v4
> > > - removed virtual function to get mutex register address
> > > - enabling the mutex back only on aux channel init
> > > - added the aux channel name to the timeout debug message
> > > v5
> > > - renamed DP_AUX_CH_MUTEX() parameter to aux_ch
> > > - renamed goto label when intel_dp_aux_ch_trylock() fails
> > >
> > > drivers/gpu/drm/i915/i915_reg.h | 9 ++++++++
> > > drivers/gpu/drm/i915/intel_dp.c | 47
> > > +++++++++++++++++++++++++++++++++++++++++
> > > 2 files changed, 56 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > > b/drivers/gpu/drm/i915/i915_reg.h
> > > index eea5b2c537d4..bce2e6dad4c4 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(aux_ch) _MMIO_PORT(aux_ch,
> > > _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 2a3b3ae4e3da..7f4bf77227cd 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > @@ -1081,6 +1081,42 @@ 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;
> > > +
> > > + /* Spec says that mutex is acquired when status bit is
> > > read as unset,
> > > + * here waiting for 2msec(+-4 aux transactions) before
> > > give up.
> > > + */
> > > + if (intel_wait_for_register(dev_priv,
> > > DP_AUX_CH_MUTEX(intel_dp->aux_ch),
> > > + DP_AUX_CH_MUTEX_STATUS, 0, 2))
> > > {
> > > + DRM_DEBUG_KMS("aux channel %c locked for 2msec,
> > > timing out\n",
> > > + aux_ch_name(intel_dp->aux_ch));
> > > + 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;
> > > +
> > > + /* set the status bit releases the mutex + keeping mutex
> > > enabled */
> > > + I915_WRITE(DP_AUX_CH_MUTEX(intel_dp->aux_ch),
> > > + 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 +1155,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_aux_ch_unlocked;
> > > + }
> > > +
> > > /* Try to wait for any previous AUX channel activity */
> > > for (try = 0; try < 3; try++) {
> > > status = I915_READ_NOTRACE(ch_ctl);
> > > @@ -1240,6 +1281,8 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
> > >
> > > ret = recv_bytes;
> > > out:
> > > + intel_dp_aux_ch_unlock(intel_dp);
> > > +out_aux_ch_unlocked:
> > > pm_qos_update_request(&dev_priv->pm_qos,
> > > PM_QOS_DEFAULT_VALUE);
> > >
> > > if (vdd)
> > > @@ -1536,6 +1579,10 @@ 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)
> > > + I915_WRITE(DP_AUX_CH_MUTEX(intel_dp->aux_ch),
> > > + DP_AUX_CH_MUTEX_ENABLE);
> >
> > And who enables it after system/runtime PM etc.? Not sure how much
> > sense
> > there is to finesse this anyway. One extra mmio write every time we
> > try
> > to acquire the mutex shouldn't kill anyone.
>
> Valid point, but I guess is better enable the mutex in
> intel_dp_encoder_reset():
>
> "
> @@ -5293,6 +5340,10 @@ void intel_dp_encoder_reset(struct drm_encoder
> *encoder)
>
> pps_lock(intel_dp);
>
> + if (INTEL_GEN(dev_priv) >= 9)
> + I915_WRITE(DP_AUX_CH_MUTEX(intel_dp->aux_ch),
> + DP_AUX_CH_MUTEX_ENABLE);
> +
> if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> intel_dp->active_pipe = vlv_active_pipe(intel_dp);
> "
>
> any objections?
I guess we can hope that dmc takes care of it for dc5/6. But what
about dc9?
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] drm/i915/skl+: Add and enable DP AUX CH mutex
2018-02-28 11:09 ` Ville Syrjälä
@ 2018-02-28 23:55 ` Souza, Jose
2018-03-01 10:53 ` Ville Syrjälä
0 siblings, 1 reply; 11+ messages in thread
From: Souza, Jose @ 2018-02-28 23:55 UTC (permalink / raw)
To: ville.syrjala; +Cc: intel-gfx
On Wed, 2018-02-28 at 13:09 +0200, Ville Syrjälä wrote:
> On Wed, Feb 28, 2018 at 12:57:07AM +0000, Souza, Jose wrote:
> > On Tue, 2018-02-27 at 23:34 +0200, Ville Syrjälä wrote:
> > > On Tue, Feb 27, 2018 at 01:23:59PM -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/int
> > > > el-g
> > > > fx-prm-osrc-skl-vol12-display.pdf
> > > > Page 198 - AUX programming sequence
> > > >
> > > > Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com
> > > > >
> > > > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > > ---
> > > >
> > > > Changelog:
> > > > 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
> > > > v4
> > > > - removed virtual function to get mutex register address
> > > > - enabling the mutex back only on aux channel init
> > > > - added the aux channel name to the timeout debug message
> > > > v5
> > > > - renamed DP_AUX_CH_MUTEX() parameter to aux_ch
> > > > - renamed goto label when intel_dp_aux_ch_trylock() fails
> > > >
> > > > drivers/gpu/drm/i915/i915_reg.h | 9 ++++++++
> > > > drivers/gpu/drm/i915/intel_dp.c | 47
> > > > +++++++++++++++++++++++++++++++++++++++++
> > > > 2 files changed, 56 insertions(+)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > > > b/drivers/gpu/drm/i915/i915_reg.h
> > > > index eea5b2c537d4..bce2e6dad4c4 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(aux_ch) _MMIO_PORT(aux_ch,
> > > > _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 2a3b3ae4e3da..7f4bf77227cd 100644
> > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > @@ -1081,6 +1081,42 @@ 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;
> > > > +
> > > > + /* Spec says that mutex is acquired when status bit is
> > > > read as unset,
> > > > + * here waiting for 2msec(+-4 aux transactions) before
> > > > give up.
> > > > + */
> > > > + if (intel_wait_for_register(dev_priv,
> > > > DP_AUX_CH_MUTEX(intel_dp->aux_ch),
> > > > + DP_AUX_CH_MUTEX_STATUS, 0,
> > > > 2))
> > > > {
> > > > + DRM_DEBUG_KMS("aux channel %c locked for
> > > > 2msec,
> > > > timing out\n",
> > > > + aux_ch_name(intel_dp->aux_ch));
> > > > + 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;
> > > > +
> > > > + /* set the status bit releases the mutex + keeping
> > > > mutex
> > > > enabled */
> > > > + I915_WRITE(DP_AUX_CH_MUTEX(intel_dp->aux_ch),
> > > > + 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 +1155,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_aux_ch_unlocked;
> > > > + }
> > > > +
> > > > /* Try to wait for any previous AUX channel activity
> > > > */
> > > > for (try = 0; try < 3; try++) {
> > > > status = I915_READ_NOTRACE(ch_ctl);
> > > > @@ -1240,6 +1281,8 @@ intel_dp_aux_ch(struct intel_dp
> > > > *intel_dp,
> > > >
> > > > ret = recv_bytes;
> > > > out:
> > > > + intel_dp_aux_ch_unlock(intel_dp);
> > > > +out_aux_ch_unlocked:
> > > > pm_qos_update_request(&dev_priv->pm_qos,
> > > > PM_QOS_DEFAULT_VALUE);
> > > >
> > > > if (vdd)
> > > > @@ -1536,6 +1579,10 @@ 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)
> > > > + I915_WRITE(DP_AUX_CH_MUTEX(intel_dp->aux_ch),
> > > > + DP_AUX_CH_MUTEX_ENABLE);
> > >
> > > And who enables it after system/runtime PM etc.? Not sure how
> > > much
> > > sense
> > > there is to finesse this anyway. One extra mmio write every time
> > > we
> > > try
> > > to acquire the mutex shouldn't kill anyone.
> >
> > Valid point, but I guess is better enable the mutex in
> > intel_dp_encoder_reset():
> >
> > "
> > @@ -5293,6 +5340,10 @@ void intel_dp_encoder_reset(struct
> > drm_encoder
> > *encoder)
> >
> > pps_lock(intel_dp);
> >
> > + if (INTEL_GEN(dev_priv) >= 9)
> > + I915_WRITE(DP_AUX_CH_MUTEX(intel_dp->aux_ch),
> > + DP_AUX_CH_MUTEX_ENABLE);
> > +
> > if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> > intel_dp->active_pipe = vlv_active_pipe(intel_dp);
> > "
> >
> > any objections?
>
> I guess we can hope that dmc takes care of it for dc5/6. But what
> about dc9?
Yep, for DC5 and DC6 the registers are restored by hardware but it is
not the case for DC9. If I'm not missing something looks like there is
no handling to restore registers when exiting DC9.
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] drm/i915/skl+: Add and enable DP AUX CH mutex
2018-02-28 23:55 ` Souza, Jose
@ 2018-03-01 10:53 ` Ville Syrjälä
2018-03-02 23:20 ` Pandiyan, Dhinakaran
2018-03-03 0:12 ` Rodrigo Vivi
0 siblings, 2 replies; 11+ messages in thread
From: Ville Syrjälä @ 2018-03-01 10:53 UTC (permalink / raw)
To: Souza, Jose; +Cc: intel-gfx
On Wed, Feb 28, 2018 at 11:55:39PM +0000, Souza, Jose wrote:
> On Wed, 2018-02-28 at 13:09 +0200, Ville Syrjälä wrote:
> > On Wed, Feb 28, 2018 at 12:57:07AM +0000, Souza, Jose wrote:
> > > On Tue, 2018-02-27 at 23:34 +0200, Ville Syrjälä wrote:
> > > > On Tue, Feb 27, 2018 at 01:23:59PM -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/int
> > > > > el-g
> > > > > fx-prm-osrc-skl-vol12-display.pdf
> > > > > Page 198 - AUX programming sequence
> > > > >
> > > > > Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com
> > > > > >
> > > > > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > > > ---
> > > > >
> > > > > Changelog:
> > > > > 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
> > > > > v4
> > > > > - removed virtual function to get mutex register address
> > > > > - enabling the mutex back only on aux channel init
> > > > > - added the aux channel name to the timeout debug message
> > > > > v5
> > > > > - renamed DP_AUX_CH_MUTEX() parameter to aux_ch
> > > > > - renamed goto label when intel_dp_aux_ch_trylock() fails
> > > > >
> > > > > drivers/gpu/drm/i915/i915_reg.h | 9 ++++++++
> > > > > drivers/gpu/drm/i915/intel_dp.c | 47
> > > > > +++++++++++++++++++++++++++++++++++++++++
> > > > > 2 files changed, 56 insertions(+)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > > > > b/drivers/gpu/drm/i915/i915_reg.h
> > > > > index eea5b2c537d4..bce2e6dad4c4 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(aux_ch) _MMIO_PORT(aux_ch,
> > > > > _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 2a3b3ae4e3da..7f4bf77227cd 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > > @@ -1081,6 +1081,42 @@ 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;
> > > > > +
> > > > > + /* Spec says that mutex is acquired when status bit is
> > > > > read as unset,
> > > > > + * here waiting for 2msec(+-4 aux transactions) before
> > > > > give up.
> > > > > + */
> > > > > + if (intel_wait_for_register(dev_priv,
> > > > > DP_AUX_CH_MUTEX(intel_dp->aux_ch),
> > > > > + DP_AUX_CH_MUTEX_STATUS, 0,
> > > > > 2))
> > > > > {
> > > > > + DRM_DEBUG_KMS("aux channel %c locked for
> > > > > 2msec,
> > > > > timing out\n",
> > > > > + aux_ch_name(intel_dp->aux_ch));
> > > > > + 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;
> > > > > +
> > > > > + /* set the status bit releases the mutex + keeping
> > > > > mutex
> > > > > enabled */
> > > > > + I915_WRITE(DP_AUX_CH_MUTEX(intel_dp->aux_ch),
> > > > > + 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 +1155,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_aux_ch_unlocked;
> > > > > + }
> > > > > +
> > > > > /* Try to wait for any previous AUX channel activity
> > > > > */
> > > > > for (try = 0; try < 3; try++) {
> > > > > status = I915_READ_NOTRACE(ch_ctl);
> > > > > @@ -1240,6 +1281,8 @@ intel_dp_aux_ch(struct intel_dp
> > > > > *intel_dp,
> > > > >
> > > > > ret = recv_bytes;
> > > > > out:
> > > > > + intel_dp_aux_ch_unlock(intel_dp);
> > > > > +out_aux_ch_unlocked:
> > > > > pm_qos_update_request(&dev_priv->pm_qos,
> > > > > PM_QOS_DEFAULT_VALUE);
> > > > >
> > > > > if (vdd)
> > > > > @@ -1536,6 +1579,10 @@ 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)
> > > > > + I915_WRITE(DP_AUX_CH_MUTEX(intel_dp->aux_ch),
> > > > > + DP_AUX_CH_MUTEX_ENABLE);
> > > >
> > > > And who enables it after system/runtime PM etc.? Not sure how
> > > > much
> > > > sense
> > > > there is to finesse this anyway. One extra mmio write every time
> > > > we
> > > > try
> > > > to acquire the mutex shouldn't kill anyone.
> > >
> > > Valid point, but I guess is better enable the mutex in
> > > intel_dp_encoder_reset():
> > >
> > > "
> > > @@ -5293,6 +5340,10 @@ void intel_dp_encoder_reset(struct
> > > drm_encoder
> > > *encoder)
> > >
> > > pps_lock(intel_dp);
> > >
> > > + if (INTEL_GEN(dev_priv) >= 9)
> > > + I915_WRITE(DP_AUX_CH_MUTEX(intel_dp->aux_ch),
> > > + DP_AUX_CH_MUTEX_ENABLE);
> > > +
> > > if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> > > intel_dp->active_pipe = vlv_active_pipe(intel_dp);
> > > "
> > >
> > > any objections?
> >
> > I guess we can hope that dmc takes care of it for dc5/6. But what
> > about dc9?
>
> Yep, for DC5 and DC6 the registers are restored by hardware but it is
> not the case for DC9. If I'm not missing something looks like there is
> no handling to restore registers when exiting DC9.
The design of the driver is "program all required registers when you
need them" instead of the "blindly save/restore everything" DMC firmware
approach.
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] drm/i915/skl+: Add and enable DP AUX CH mutex
2018-03-01 10:53 ` Ville Syrjälä
@ 2018-03-02 23:20 ` Pandiyan, Dhinakaran
2018-03-03 0:16 ` Rodrigo Vivi
2018-03-03 0:12 ` Rodrigo Vivi
1 sibling, 1 reply; 11+ messages in thread
From: Pandiyan, Dhinakaran @ 2018-03-02 23:20 UTC (permalink / raw)
To: ville.syrjala; +Cc: intel-gfx
On Thu, 2018-03-01 at 12:53 +0200, Ville Syrjälä wrote:
> On Wed, Feb 28, 2018 at 11:55:39PM +0000, Souza, Jose wrote:
> > On Wed, 2018-02-28 at 13:09 +0200, Ville Syrjälä wrote:
> > > On Wed, Feb 28, 2018 at 12:57:07AM +0000, Souza, Jose wrote:
> > > > On Tue, 2018-02-27 at 23:34 +0200, Ville Syrjälä wrote:
> > > > > On Tue, Feb 27, 2018 at 01:23:59PM -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/int
> > > > > > el-g
> > > > > > fx-prm-osrc-skl-vol12-display.pdf
> > > > > > Page 198 - AUX programming sequence
> > > > > >
> > > > > > Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com
> > > > > > >
> > > > > > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > > > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > > > > ---
> > > > > >
> > > > > > Changelog:
> > > > > > 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
> > > > > > v4
> > > > > > - removed virtual function to get mutex register address
> > > > > > - enabling the mutex back only on aux channel init
> > > > > > - added the aux channel name to the timeout debug message
> > > > > > v5
> > > > > > - renamed DP_AUX_CH_MUTEX() parameter to aux_ch
> > > > > > - renamed goto label when intel_dp_aux_ch_trylock() fails
> > > > > >
> > > > > > drivers/gpu/drm/i915/i915_reg.h | 9 ++++++++
> > > > > > drivers/gpu/drm/i915/intel_dp.c | 47
> > > > > > +++++++++++++++++++++++++++++++++++++++++
> > > > > > 2 files changed, 56 insertions(+)
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > > > > > b/drivers/gpu/drm/i915/i915_reg.h
> > > > > > index eea5b2c537d4..bce2e6dad4c4 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(aux_ch) _MMIO_PORT(aux_ch,
> > > > > > _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 2a3b3ae4e3da..7f4bf77227cd 100644
> > > > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > > > @@ -1081,6 +1081,42 @@ 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;
> > > > > > +
> > > > > > + /* Spec says that mutex is acquired when status bit is
> > > > > > read as unset,
> > > > > > + * here waiting for 2msec(+-4 aux transactions) before
> > > > > > give up.
> > > > > > + */
> > > > > > + if (intel_wait_for_register(dev_priv,
> > > > > > DP_AUX_CH_MUTEX(intel_dp->aux_ch),
> > > > > > + DP_AUX_CH_MUTEX_STATUS, 0,
> > > > > > 2))
> > > > > > {
> > > > > > + DRM_DEBUG_KMS("aux channel %c locked for
> > > > > > 2msec,
> > > > > > timing out\n",
> > > > > > + aux_ch_name(intel_dp->aux_ch));
> > > > > > + 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;
> > > > > > +
> > > > > > + /* set the status bit releases the mutex + keeping
> > > > > > mutex
> > > > > > enabled */
> > > > > > + I915_WRITE(DP_AUX_CH_MUTEX(intel_dp->aux_ch),
> > > > > > + 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 +1155,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_aux_ch_unlocked;
> > > > > > + }
> > > > > > +
> > > > > > /* Try to wait for any previous AUX channel activity
> > > > > > */
> > > > > > for (try = 0; try < 3; try++) {
> > > > > > status = I915_READ_NOTRACE(ch_ctl);
> > > > > > @@ -1240,6 +1281,8 @@ intel_dp_aux_ch(struct intel_dp
> > > > > > *intel_dp,
> > > > > >
> > > > > > ret = recv_bytes;
> > > > > > out:
> > > > > > + intel_dp_aux_ch_unlock(intel_dp);
> > > > > > +out_aux_ch_unlocked:
> > > > > > pm_qos_update_request(&dev_priv->pm_qos,
> > > > > > PM_QOS_DEFAULT_VALUE);
> > > > > >
> > > > > > if (vdd)
> > > > > > @@ -1536,6 +1579,10 @@ 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)
> > > > > > + I915_WRITE(DP_AUX_CH_MUTEX(intel_dp->aux_ch),
> > > > > > + DP_AUX_CH_MUTEX_ENABLE);
> > > > >
> > > > > And who enables it after system/runtime PM etc.? Not sure how
> > > > > much
> > > > > sense
> > > > > there is to finesse this anyway. One extra mmio write every time
> > > > > we
> > > > > try
> > > > > to acquire the mutex shouldn't kill anyone.
> > > >
> > > > Valid point, but I guess is better enable the mutex in
> > > > intel_dp_encoder_reset():
> > > >
> > > > "
> > > > @@ -5293,6 +5340,10 @@ void intel_dp_encoder_reset(struct
> > > > drm_encoder
> > > > *encoder)
> > > >
> > > > pps_lock(intel_dp);
> > > >
> > > > + if (INTEL_GEN(dev_priv) >= 9)
> > > > + I915_WRITE(DP_AUX_CH_MUTEX(intel_dp->aux_ch),
> > > > + DP_AUX_CH_MUTEX_ENABLE);
> > > > +
> > > > if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> > > > intel_dp->active_pipe = vlv_active_pipe(intel_dp);
> > > > "
> > > >
> > > > any objections?
> > >
> > > I guess we can hope that dmc takes care of it for dc5/6. But what
> > > about dc9?
> >
> > Yep, for DC5 and DC6 the registers are restored by hardware but it is
> > not the case for DC9. If I'm not missing something looks like there is
> > no handling to restore registers when exiting DC9.
>
> The design of the driver is "program all required registers when you
> need them" instead of the "blindly save/restore everything" DMC firmware
> approach.
>
+static bool intel_dp_aux_trylock(struct intel_dp *intel_dp)
+{
+ bool enabled;
+ u32 mutex;
+
+ /* READ will try to lock the aux channel if the mutex was
already
+ * enabled.
+ */
+ mutex = I915_READ(DP_AUX_CH_MUTEX);
+ enabled = mutex & DP_AUX_CH_MUTEX_ENABLE;
+
+ if (!enabled) {
+ /* mutex is currently disabled, enable it without
altering the
+ * status bit in case the HW is using the bit */
+ I915_WRITE(DP_AUX_CH_MUTEX(intel_dig_port->dp.aux_ch),
+ mutex | DP_AUX_CH_MUTEX_ENABLE);
+
+ /* Now that the mutex is enabled, request to lock it */
+ mutex = I915_READ(DP_AUX_CH_MUTEX);
+ }
+
+ if (mutex & DP_AUX_CH_MUTEX_STATUS == 0)
+ return true;
+
+ if (intel_wait_for_register(dev_priv,
DP_AUX_CH_MUTEX(intel_dp->aux_ch),
+ DP_AUX_CH_MUTEX_STATUS, 0, 2)) {
+
+ DRM_DEBUG_KMS("aux channel %c locked for 2msec, timing
out\n",
+ aux_ch_name(intel_dp->aux_ch));
+ return false;
+ }
+
+ return true;
+}
+
Have I missed anything here?
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] drm/i915/skl+: Add and enable DP AUX CH mutex
2018-03-01 10:53 ` Ville Syrjälä
2018-03-02 23:20 ` Pandiyan, Dhinakaran
@ 2018-03-03 0:12 ` Rodrigo Vivi
1 sibling, 0 replies; 11+ messages in thread
From: Rodrigo Vivi @ 2018-03-03 0:12 UTC (permalink / raw)
To: Ville Syrjälä, Souza, Jose; +Cc: intel-gfx
Ville Syrjälä <ville.syrjala@linux.intel.com> writes:
> On Wed, Feb 28, 2018 at 11:55:39PM +0000, Souza, Jose wrote:
>> On Wed, 2018-02-28 at 13:09 +0200, Ville Syrjälä wrote:
>> > On Wed, Feb 28, 2018 at 12:57:07AM +0000, Souza, Jose wrote:
>> > > On Tue, 2018-02-27 at 23:34 +0200, Ville Syrjälä wrote:
>> > > > On Tue, Feb 27, 2018 at 01:23:59PM -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/int
>> > > > > el-g
>> > > > > fx-prm-osrc-skl-vol12-display.pdf
>> > > > > Page 198 - AUX programming sequence
>> > > > >
>> > > > > Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com
>> > > > > >
>> > > > > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> > > > > Cc: Jani Nikula <jani.nikula@linux.intel.com>
>> > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> > > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
>> > > > > ---
>> > > > >
>> > > > > Changelog:
>> > > > > 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
>> > > > > v4
>> > > > > - removed virtual function to get mutex register address
>> > > > > - enabling the mutex back only on aux channel init
>> > > > > - added the aux channel name to the timeout debug message
>> > > > > v5
>> > > > > - renamed DP_AUX_CH_MUTEX() parameter to aux_ch
>> > > > > - renamed goto label when intel_dp_aux_ch_trylock() fails
>> > > > >
>> > > > > drivers/gpu/drm/i915/i915_reg.h | 9 ++++++++
>> > > > > drivers/gpu/drm/i915/intel_dp.c | 47
>> > > > > +++++++++++++++++++++++++++++++++++++++++
>> > > > > 2 files changed, 56 insertions(+)
>> > > > >
>> > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h
>> > > > > b/drivers/gpu/drm/i915/i915_reg.h
>> > > > > index eea5b2c537d4..bce2e6dad4c4 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(aux_ch) _MMIO_PORT(aux_ch,
>> > > > > _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 2a3b3ae4e3da..7f4bf77227cd 100644
>> > > > > --- a/drivers/gpu/drm/i915/intel_dp.c
>> > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
>> > > > > @@ -1081,6 +1081,42 @@ 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;
>> > > > > +
>> > > > > + /* Spec says that mutex is acquired when status bit is
>> > > > > read as unset,
>> > > > > + * here waiting for 2msec(+-4 aux transactions) before
>> > > > > give up.
>> > > > > + */
>> > > > > + if (intel_wait_for_register(dev_priv,
>> > > > > DP_AUX_CH_MUTEX(intel_dp->aux_ch),
>> > > > > + DP_AUX_CH_MUTEX_STATUS, 0,
>> > > > > 2))
>> > > > > {
>> > > > > + DRM_DEBUG_KMS("aux channel %c locked for
>> > > > > 2msec,
>> > > > > timing out\n",
>> > > > > + aux_ch_name(intel_dp->aux_ch));
>> > > > > + 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;
>> > > > > +
>> > > > > + /* set the status bit releases the mutex + keeping
>> > > > > mutex
>> > > > > enabled */
>> > > > > + I915_WRITE(DP_AUX_CH_MUTEX(intel_dp->aux_ch),
>> > > > > + 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 +1155,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_aux_ch_unlocked;
>> > > > > + }
>> > > > > +
>> > > > > /* Try to wait for any previous AUX channel activity
>> > > > > */
>> > > > > for (try = 0; try < 3; try++) {
>> > > > > status = I915_READ_NOTRACE(ch_ctl);
>> > > > > @@ -1240,6 +1281,8 @@ intel_dp_aux_ch(struct intel_dp
>> > > > > *intel_dp,
>> > > > >
>> > > > > ret = recv_bytes;
>> > > > > out:
>> > > > > + intel_dp_aux_ch_unlock(intel_dp);
>> > > > > +out_aux_ch_unlocked:
>> > > > > pm_qos_update_request(&dev_priv->pm_qos,
>> > > > > PM_QOS_DEFAULT_VALUE);
>> > > > >
>> > > > > if (vdd)
>> > > > > @@ -1536,6 +1579,10 @@ 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)
>> > > > > + I915_WRITE(DP_AUX_CH_MUTEX(intel_dp->aux_ch),
>> > > > > + DP_AUX_CH_MUTEX_ENABLE);
>> > > >
>> > > > And who enables it after system/runtime PM etc.? Not sure how
>> > > > much
>> > > > sense
>> > > > there is to finesse this anyway. One extra mmio write every time
>> > > > we
>> > > > try
>> > > > to acquire the mutex shouldn't kill anyone.
>> > >
>> > > Valid point, but I guess is better enable the mutex in
>> > > intel_dp_encoder_reset():
>> > >
>> > > "
>> > > @@ -5293,6 +5340,10 @@ void intel_dp_encoder_reset(struct
>> > > drm_encoder
>> > > *encoder)
>> > >
>> > > pps_lock(intel_dp);
>> > >
>> > > + if (INTEL_GEN(dev_priv) >= 9)
>> > > + I915_WRITE(DP_AUX_CH_MUTEX(intel_dp->aux_ch),
>> > > + DP_AUX_CH_MUTEX_ENABLE);
>> > > +
>> > > if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
>> > > intel_dp->active_pipe = vlv_active_pipe(intel_dp);
>> > > "
>> > >
>> > > any objections?
>> >
>> > I guess we can hope that dmc takes care of it for dc5/6. But what
>> > about dc9?
>>
>> Yep, for DC5 and DC6 the registers are restored by hardware but it is
>> not the case for DC9. If I'm not missing something looks like there is
>> no handling to restore registers when exiting DC9.
>
> The design of the driver is "program all required registers when you
> need them" instead of the "blindly save/restore everything" DMC firmware
> approach.
So, what do you suggest Ville?
Some function called form i915_drm_resume that loops on connectors
with active DP and enable its aux mutex?
But even on this case we would have the risk of unsetting the status
while hw can be using, no?
Or this or:
Option 2: Enable without RMW only on trylock and disabe without RMW on release.
Or (my favorite one) that follows spec line by line:
Option 3: Enable with RMW only on trylock and with RMW touch only Status
bit keeping it enabled.
I won't believe RMW would have any impact here because on trylock we are
anyways trying to lock so reading the status right after enabling.
Also there is no problem on RMW on release because right after reading
before writing we would be releasing it anyways.
So RMW is the simplified solution imho...
>
> --
> 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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] drm/i915/skl+: Add and enable DP AUX CH mutex
2018-03-02 23:20 ` Pandiyan, Dhinakaran
@ 2018-03-03 0:16 ` Rodrigo Vivi
0 siblings, 0 replies; 11+ messages in thread
From: Rodrigo Vivi @ 2018-03-03 0:16 UTC (permalink / raw)
To: Pandiyan, Dhinakaran; +Cc: intel-gfx
On Fri, Mar 02, 2018 at 11:20:42PM +0000, Pandiyan, Dhinakaran wrote:
>
>
>
> On Thu, 2018-03-01 at 12:53 +0200, Ville Syrjälä wrote:
> > On Wed, Feb 28, 2018 at 11:55:39PM +0000, Souza, Jose wrote:
> > > On Wed, 2018-02-28 at 13:09 +0200, Ville Syrjälä wrote:
> > > > On Wed, Feb 28, 2018 at 12:57:07AM +0000, Souza, Jose wrote:
> > > > > On Tue, 2018-02-27 at 23:34 +0200, Ville Syrjälä wrote:
> > > > > > On Tue, Feb 27, 2018 at 01:23:59PM -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/int
> > > > > > > el-g
> > > > > > > fx-prm-osrc-skl-vol12-display.pdf
> > > > > > > Page 198 - AUX programming sequence
> > > > > > >
> > > > > > > Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com
> > > > > > > >
> > > > > > > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > > > > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > > > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > > > > > ---
> > > > > > >
> > > > > > > Changelog:
> > > > > > > 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
> > > > > > > v4
> > > > > > > - removed virtual function to get mutex register address
> > > > > > > - enabling the mutex back only on aux channel init
> > > > > > > - added the aux channel name to the timeout debug message
> > > > > > > v5
> > > > > > > - renamed DP_AUX_CH_MUTEX() parameter to aux_ch
> > > > > > > - renamed goto label when intel_dp_aux_ch_trylock() fails
> > > > > > >
> > > > > > > drivers/gpu/drm/i915/i915_reg.h | 9 ++++++++
> > > > > > > drivers/gpu/drm/i915/intel_dp.c | 47
> > > > > > > +++++++++++++++++++++++++++++++++++++++++
> > > > > > > 2 files changed, 56 insertions(+)
> > > > > > >
> > > > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > > > > > > b/drivers/gpu/drm/i915/i915_reg.h
> > > > > > > index eea5b2c537d4..bce2e6dad4c4 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(aux_ch) _MMIO_PORT(aux_ch,
> > > > > > > _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 2a3b3ae4e3da..7f4bf77227cd 100644
> > > > > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > > > > @@ -1081,6 +1081,42 @@ 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;
> > > > > > > +
> > > > > > > + /* Spec says that mutex is acquired when status bit is
> > > > > > > read as unset,
> > > > > > > + * here waiting for 2msec(+-4 aux transactions) before
> > > > > > > give up.
> > > > > > > + */
> > > > > > > + if (intel_wait_for_register(dev_priv,
> > > > > > > DP_AUX_CH_MUTEX(intel_dp->aux_ch),
> > > > > > > + DP_AUX_CH_MUTEX_STATUS, 0,
> > > > > > > 2))
> > > > > > > {
> > > > > > > + DRM_DEBUG_KMS("aux channel %c locked for
> > > > > > > 2msec,
> > > > > > > timing out\n",
> > > > > > > + aux_ch_name(intel_dp->aux_ch));
> > > > > > > + 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;
> > > > > > > +
> > > > > > > + /* set the status bit releases the mutex + keeping
> > > > > > > mutex
> > > > > > > enabled */
> > > > > > > + I915_WRITE(DP_AUX_CH_MUTEX(intel_dp->aux_ch),
> > > > > > > + 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 +1155,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_aux_ch_unlocked;
> > > > > > > + }
> > > > > > > +
> > > > > > > /* Try to wait for any previous AUX channel activity
> > > > > > > */
> > > > > > > for (try = 0; try < 3; try++) {
> > > > > > > status = I915_READ_NOTRACE(ch_ctl);
> > > > > > > @@ -1240,6 +1281,8 @@ intel_dp_aux_ch(struct intel_dp
> > > > > > > *intel_dp,
> > > > > > >
> > > > > > > ret = recv_bytes;
> > > > > > > out:
> > > > > > > + intel_dp_aux_ch_unlock(intel_dp);
> > > > > > > +out_aux_ch_unlocked:
> > > > > > > pm_qos_update_request(&dev_priv->pm_qos,
> > > > > > > PM_QOS_DEFAULT_VALUE);
> > > > > > >
> > > > > > > if (vdd)
> > > > > > > @@ -1536,6 +1579,10 @@ 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)
> > > > > > > + I915_WRITE(DP_AUX_CH_MUTEX(intel_dp->aux_ch),
> > > > > > > + DP_AUX_CH_MUTEX_ENABLE);
> > > > > >
> > > > > > And who enables it after system/runtime PM etc.? Not sure how
> > > > > > much
> > > > > > sense
> > > > > > there is to finesse this anyway. One extra mmio write every time
> > > > > > we
> > > > > > try
> > > > > > to acquire the mutex shouldn't kill anyone.
> > > > >
> > > > > Valid point, but I guess is better enable the mutex in
> > > > > intel_dp_encoder_reset():
> > > > >
> > > > > "
> > > > > @@ -5293,6 +5340,10 @@ void intel_dp_encoder_reset(struct
> > > > > drm_encoder
> > > > > *encoder)
> > > > >
> > > > > pps_lock(intel_dp);
> > > > >
> > > > > + if (INTEL_GEN(dev_priv) >= 9)
> > > > > + I915_WRITE(DP_AUX_CH_MUTEX(intel_dp->aux_ch),
> > > > > + DP_AUX_CH_MUTEX_ENABLE);
> > > > > +
> > > > > if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> > > > > intel_dp->active_pipe = vlv_active_pipe(intel_dp);
> > > > > "
> > > > >
> > > > > any objections?
> > > >
> > > > I guess we can hope that dmc takes care of it for dc5/6. But what
> > > > about dc9?
> > >
> > > Yep, for DC5 and DC6 the registers are restored by hardware but it is
> > > not the case for DC9. If I'm not missing something looks like there is
> > > no handling to restore registers when exiting DC9.
> >
> > The design of the driver is "program all required registers when you
> > need them" instead of the "blindly save/restore everything" DMC firmware
> > approach.
> >
>
>
> +static bool intel_dp_aux_trylock(struct intel_dp *intel_dp)
> +{
> + bool enabled;
> + u32 mutex;
> +
> + /* READ will try to lock the aux channel if the mutex was
> already
> + * enabled.
> + */
> + mutex = I915_READ(DP_AUX_CH_MUTEX);
> + enabled = mutex & DP_AUX_CH_MUTEX_ENABLE;
> +
> + if (!enabled) {
> + /* mutex is currently disabled, enable it without
> altering the
> + * status bit in case the HW is using the bit */
> + I915_WRITE(DP_AUX_CH_MUTEX(intel_dig_port->dp.aux_ch),
> + mutex | DP_AUX_CH_MUTEX_ENABLE);
> +
> + /* Now that the mutex is enabled, request to lock it */
> + mutex = I915_READ(DP_AUX_CH_MUTEX);
> + }
> +
> + if (mutex & DP_AUX_CH_MUTEX_STATUS == 0)
> + return true;
> +
> + if (intel_wait_for_register(dev_priv,
> DP_AUX_CH_MUTEX(intel_dp->aux_ch),
> + DP_AUX_CH_MUTEX_STATUS, 0, 2)) {
> +
> + DRM_DEBUG_KMS("aux channel %c locked for 2msec, timing
> out\n",
> + aux_ch_name(intel_dp->aux_ch));
> + return false;
> + }
> +
> + return true;
> +}
> +
>
>
> Have I missed anything here?
ops, I wrote before seeing this here.
I like this approach a lot, although I don't believe it is harmful to just always set enabled bit.
anyways, +1 for this and probably only setting here.
> _______________________________________________
> 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
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2018-03-03 0:16 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-02-27 21:23 [PATCH] drm/i915/skl+: Add and enable DP AUX CH mutex José Roberto de Souza
2018-02-27 21:34 ` Ville Syrjälä
2018-02-27 23:54 ` Pandiyan, Dhinakaran
2018-02-28 0:57 ` Souza, Jose
2018-02-28 11:09 ` Ville Syrjälä
2018-02-28 23:55 ` Souza, Jose
2018-03-01 10:53 ` Ville Syrjälä
2018-03-02 23:20 ` Pandiyan, Dhinakaran
2018-03-03 0:16 ` Rodrigo Vivi
2018-03-03 0:12 ` Rodrigo Vivi
2018-02-27 22:11 ` ✗ Fi.CI.BAT: warning for " Patchwork
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.