* [PATCH 1/5] drm/i915: add and enable DP AUX CH mutex
@ 2018-02-14 0:29 José Roberto de Souza
2018-02-14 0:29 ` [PATCH 2/5] drm/i915: keep AUX powered while PSR is enabled José Roberto de Souza
` (7 more replies)
0 siblings, 8 replies; 18+ messages in thread
From: José Roberto de Souza @ 2018-02-14 0:29 UTC (permalink / raw)
To: intel-gfx; +Cc: Rodrigo Vivi
When PSR/PSR2 is enabled hardware can do AUX transactions by it self, so
lets use the mutex register that is available in gen9+ to avoid concurrent
access by hardware and driver.
Reference: https://01.org/sites/default/files/documentation/intel-gfx-prm-osrc-skl-vol12-display.pdf
Page 198 - AUX programming sequence
As gen < 9 don't have the mutex PSR needs to be disabled, that is why the
'else TODO' was added. This is also backed by spec:
"DDI A AUX channel transactions must not be sent while SRD is enabled.
SRD must be completely disabled before a DDI A AUX channel transaction can
be sent." BSpec: 7530
Cc: Rodrigo Vivi <rodrigo.vivi@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 | 48 ++++++++++++++++++++++++++++++++++++++++
drivers/gpu/drm/i915/intel_drv.h | 1 +
drivers/gpu/drm/i915/intel_psr.c | 6 +++++
4 files changed, 64 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index f6afa5e5e7c1..5d7736117cb9 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -5318,6 +5318,7 @@ enum {
#define _DPA_AUX_CH_DATA3 (dev_priv->info.display_mmio_offset + 0x6401c)
#define _DPA_AUX_CH_DATA4 (dev_priv->info.display_mmio_offset + 0x64020)
#define _DPA_AUX_CH_DATA5 (dev_priv->info.display_mmio_offset + 0x64024)
+#define _DPA_AUX_CH_MUTEX (dev_priv->info.display_mmio_offset + 0x6402C)
#define _DPB_AUX_CH_CTL (dev_priv->info.display_mmio_offset + 0x64110)
#define _DPB_AUX_CH_DATA1 (dev_priv->info.display_mmio_offset + 0x64114)
@@ -5325,6 +5326,7 @@ enum {
#define _DPB_AUX_CH_DATA3 (dev_priv->info.display_mmio_offset + 0x6411c)
#define _DPB_AUX_CH_DATA4 (dev_priv->info.display_mmio_offset + 0x64120)
#define _DPB_AUX_CH_DATA5 (dev_priv->info.display_mmio_offset + 0x64124)
+#define _DPB_AUX_CH_MUTEX (dev_priv->info.display_mmio_offset + 0x6412C)
#define _DPC_AUX_CH_CTL (dev_priv->info.display_mmio_offset + 0x64210)
#define _DPC_AUX_CH_DATA1 (dev_priv->info.display_mmio_offset + 0x64214)
@@ -5332,6 +5334,7 @@ enum {
#define _DPC_AUX_CH_DATA3 (dev_priv->info.display_mmio_offset + 0x6421c)
#define _DPC_AUX_CH_DATA4 (dev_priv->info.display_mmio_offset + 0x64220)
#define _DPC_AUX_CH_DATA5 (dev_priv->info.display_mmio_offset + 0x64224)
+#define _DPC_AUX_CH_MUTEX (dev_priv->info.display_mmio_offset + 0x6422C)
#define _DPD_AUX_CH_CTL (dev_priv->info.display_mmio_offset + 0x64310)
#define _DPD_AUX_CH_DATA1 (dev_priv->info.display_mmio_offset + 0x64314)
@@ -5339,6 +5342,7 @@ enum {
#define _DPD_AUX_CH_DATA3 (dev_priv->info.display_mmio_offset + 0x6431c)
#define _DPD_AUX_CH_DATA4 (dev_priv->info.display_mmio_offset + 0x64320)
#define _DPD_AUX_CH_DATA5 (dev_priv->info.display_mmio_offset + 0x64324)
+#define _DPD_AUX_CH_MUTEX (dev_priv->info.display_mmio_offset + 0x6432C)
#define _DPF_AUX_CH_CTL (dev_priv->info.display_mmio_offset + 0x64510)
#define _DPF_AUX_CH_DATA1 (dev_priv->info.display_mmio_offset + 0x64514)
@@ -5346,9 +5350,11 @@ enum {
#define _DPF_AUX_CH_DATA3 (dev_priv->info.display_mmio_offset + 0x6451c)
#define _DPF_AUX_CH_DATA4 (dev_priv->info.display_mmio_offset + 0x64520)
#define _DPF_AUX_CH_DATA5 (dev_priv->info.display_mmio_offset + 0x64524)
+#define _DPF_AUX_CH_MUTEX (dev_priv->info.display_mmio_offset + 0x6452C)
#define DP_AUX_CH_CTL(port) _MMIO_PORT(port, _DPA_AUX_CH_CTL, _DPB_AUX_CH_CTL)
#define DP_AUX_CH_DATA(port, i) _MMIO(_PORT(port, _DPA_AUX_CH_DATA1, _DPB_AUX_CH_DATA1) + (i) * 4) /* 5 registers */
+#define DP_AUX_CH_MUTEX(port) _MMIO_PORT(port, _DPA_AUX_CH_MUTEX, _DPB_AUX_CH_MUTEX)
#define DP_AUX_CH_CTL_SEND_BUSY (1 << 31)
#define DP_AUX_CH_CTL_DONE (1 << 30)
@@ -5378,6 +5384,9 @@ enum {
#define DP_AUX_CH_CTL_FW_SYNC_PULSE_SKL(c) (((c) - 1) << 5)
#define DP_AUX_CH_CTL_SYNC_PULSE_SKL(c) ((c) - 1)
+#define DP_AUX_CH_MUTEX_ENABLE (1 << 31)
+#define DP_AUX_CH_MUTEX_STATUS (1 << 30)
+
/*
* Computing GMCH M and N values for the Display Port link
*
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index f20b25f98e5a..bd5f1bb730ff 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1081,6 +1081,28 @@ static uint32_t intel_dp_get_aux_send_ctl(struct intel_dp *intel_dp,
aux_clock_divider);
}
+static bool intel_dp_aux_get(struct drm_i915_private *dev_priv,
+ struct intel_dp *intel_dp)
+{
+ int try, ret;
+
+ for (try = 0, ret = -1; try < 3 && ret; try++)
+ ret = wait_for(!(I915_READ_NOTRACE(intel_dp->aux_ch_mutex_reg)
+ & DP_AUX_CH_MUTEX_STATUS), 0.5);
+
+ return ret == 0;
+}
+
+static void intel_dp_aux_put(struct drm_i915_private *dev_priv,
+ struct intel_dp *intel_dp)
+{
+ /* setting the status bit releases the mutex + keep set the bit
+ * to keep the mutex enabled.
+ */
+ I915_WRITE(intel_dp->aux_ch_mutex_reg, DP_AUX_CH_MUTEX_ENABLE |
+ DP_AUX_CH_MUTEX_STATUS);
+}
+
static int
intel_dp_aux_ch(struct intel_dp *intel_dp,
const uint8_t *send, int send_bytes,
@@ -1115,6 +1137,21 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
intel_dp_check_edp(intel_dp);
+ /* Get hardware mutex when PSR is enabled */
+ if (dev_priv->psr.enabled == intel_dp) {
+ if (intel_dp->aux_ch_mutex_reg.reg) {
+ if (intel_dp_aux_get(dev_priv, intel_dp)) {
+ DRM_WARN("dp_aux_ch port locked for too long");
+ ret = -EBUSY;
+ goto out_locked;
+ }
+ } else {
+ /* TODO: go to PSR exit state waiting for change before
+ * do a aux ch transaction.
+ */
+ }
+ }
+
/* Try to wait for any previous AUX channel activity */
for (try = 0; try < 3; try++) {
status = I915_READ_NOTRACE(ch_ctl);
@@ -1243,7 +1280,16 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
recv + i, recv_bytes - i);
ret = recv_bytes;
+
out:
+ if (dev_priv->psr.enabled == intel_dp) {
+ if (intel_dp->aux_ch_mutex_reg.reg)
+ intel_dp_aux_put(dev_priv, intel_dp);
+ else
+ /* TODO: schedule PSR work to active PSR again */
+ }
+
+out_locked:
pm_qos_update_request(&dev_priv->pm_qos, PM_QOS_DEFAULT_VALUE);
if (vdd)
@@ -1496,6 +1542,8 @@ static void intel_aux_reg_init(struct intel_dp *intel_dp)
intel_dp->aux_ch_ctl_reg = intel_aux_ctl_reg(dev_priv, port);
for (i = 0; i < ARRAY_SIZE(intel_dp->aux_ch_data_reg); i++)
intel_dp->aux_ch_data_reg[i] = intel_aux_data_reg(dev_priv, port, i);
+ if (INTEL_INFO(dev_priv)->gen >= 9)
+ intel_dp->aux_ch_mutex_reg = DP_AUX_CH_MUTEX(port);
}
static void
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 898064e8bea7..0d235b7dab21 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1040,6 +1040,7 @@ struct intel_dp {
i915_reg_t output_reg;
i915_reg_t aux_ch_ctl_reg;
i915_reg_t aux_ch_data_reg[5];
+ i915_reg_t aux_ch_mutex_reg;
uint32_t DP;
int link_rate;
uint8_t lane_count;
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 2ef374f936b9..8b456e4e1b47 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -484,6 +484,9 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp,
EDP_PSR_DEBUG_MASK_HPD |
EDP_PSR_DEBUG_MASK_LPSP);
}
+
+ if (intel_dp->aux_ch_mutex_reg.reg)
+ I915_WRITE(intel_dp->aux_ch_mutex_reg, DP_AUX_CH_MUTEX_ENABLE);
}
/**
@@ -617,6 +620,9 @@ static void hsw_psr_disable(struct intel_dp *intel_dp,
else
WARN_ON(I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE);
}
+
+ if (intel_dp->aux_ch_mutex_reg.reg)
+ I915_WRITE(intel_dp->aux_ch_mutex_reg, ~DP_AUX_CH_MUTEX_ENABLE);
}
/**
--
2.16.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 2/5] drm/i915: keep AUX powered while PSR is enabled
2018-02-14 0:29 [PATCH 1/5] drm/i915: add and enable DP AUX CH mutex José Roberto de Souza
@ 2018-02-14 0:29 ` José Roberto de Souza
2018-02-14 1:19 ` Rodrigo Vivi
2018-02-14 0:29 ` [PATCH 3/5] drm/i915: Share PSR and PSR2 VSC setup José Roberto de Souza
` (6 subsequent siblings)
7 siblings, 1 reply; 18+ messages in thread
From: José Roberto de Souza @ 2018-02-14 0:29 UTC (permalink / raw)
To: intel-gfx
Hardware can send AUX transactions by it self when PSR is enabled and it
expects that AUX is kept powered up.
From spec:
"Each DisplayPort Aux channel has an Aux IO Power Request. If an Aux
channel will not be used, it does not need to be powered up.
PSR spontaneously sends Aux transactions. If PSR is enabled on a port,
then the associated Aux IO must be kept powered up."
BSpec: 4230
BSpec: 17722
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
drivers/gpu/drm/i915/intel_psr.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 8b456e4e1b47..225fde979e57 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -516,6 +516,12 @@ void intel_psr_enable(struct intel_dp *intel_dp,
goto unlock;
}
+ /*
+ * hardware can send AUX transactions by it self when PSR is enabled,
+ * and it expects that AUX is kept powered up.
+ */
+ intel_display_power_get(dev_priv, intel_dp->aux_power_domain);
+
dev_priv->psr.psr2_support = crtc_state->has_psr2;
dev_priv->psr.busy_frontbuffer_bits = 0;
@@ -656,6 +662,8 @@ void intel_psr_disable(struct intel_dp *intel_dp,
/* Disable PSR on Sink */
drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG, 0);
+ intel_display_power_put(dev_priv, intel_dp->aux_power_domain);
+
dev_priv->psr.enabled = NULL;
mutex_unlock(&dev_priv->psr.lock);
--
2.16.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 3/5] drm/i915: Share PSR and PSR2 VSC setup
2018-02-14 0:29 [PATCH 1/5] drm/i915: add and enable DP AUX CH mutex José Roberto de Souza
2018-02-14 0:29 ` [PATCH 2/5] drm/i915: keep AUX powered while PSR is enabled José Roberto de Souza
@ 2018-02-14 0:29 ` José Roberto de Souza
2018-02-14 0:29 ` [PATCH 4/5] drm/i915: Replace magic number with macro defined by drm José Roberto de Souza
` (5 subsequent siblings)
7 siblings, 0 replies; 18+ messages in thread
From: José Roberto de Souza @ 2018-02-14 0:29 UTC (permalink / raw)
To: intel-gfx
Just share the common code in PSR and PSR2.
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
drivers/gpu/drm/i915/intel_psr.c | 10 ++++------
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 225fde979e57..60ca39d3b226 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -88,11 +88,12 @@ static void hsw_psr_setup_vsc(struct intel_dp *intel_dp,
struct drm_i915_private *dev_priv = to_i915(intel_dig_port->base.base.dev);
struct edp_vsc_psr psr_vsc;
+ memset(&psr_vsc, 0, sizeof(psr_vsc));
+ psr_vsc.sdp_header.HB0 = 0;
+ psr_vsc.sdp_header.HB1 = 0x7;
+
if (dev_priv->psr.psr2_support) {
/* Prepare VSC Header for SU as per EDP 1.4 spec, Table 6.11 */
- memset(&psr_vsc, 0, sizeof(psr_vsc));
- psr_vsc.sdp_header.HB0 = 0;
- psr_vsc.sdp_header.HB1 = 0x7;
if (dev_priv->psr.colorimetry_support &&
dev_priv->psr.y_cord_support) {
psr_vsc.sdp_header.HB2 = 0x5;
@@ -106,9 +107,6 @@ static void hsw_psr_setup_vsc(struct intel_dp *intel_dp,
}
} else {
/* Prepare VSC packet as per EDP 1.3 spec, Table 3.10 */
- memset(&psr_vsc, 0, sizeof(psr_vsc));
- psr_vsc.sdp_header.HB0 = 0;
- psr_vsc.sdp_header.HB1 = 0x7;
psr_vsc.sdp_header.HB2 = 0x2;
psr_vsc.sdp_header.HB3 = 0x8;
}
--
2.16.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 4/5] drm/i915: Replace magic number with macro defined by drm
2018-02-14 0:29 [PATCH 1/5] drm/i915: add and enable DP AUX CH mutex José Roberto de Souza
2018-02-14 0:29 ` [PATCH 2/5] drm/i915: keep AUX powered while PSR is enabled José Roberto de Souza
2018-02-14 0:29 ` [PATCH 3/5] drm/i915: Share PSR and PSR2 VSC setup José Roberto de Souza
@ 2018-02-14 0:29 ` José Roberto de Souza
2018-02-14 0:29 ` [PATCH 5/5] drm/i915: Make use of unused variable in hsw_write_infoframe() José Roberto de Souza
` (4 subsequent siblings)
7 siblings, 0 replies; 18+ messages in thread
From: José Roberto de Souza @ 2018-02-14 0:29 UTC (permalink / raw)
To: intel-gfx
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
drivers/gpu/drm/i915/intel_psr.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 60ca39d3b226..c24afc73e30b 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -90,7 +90,7 @@ static void hsw_psr_setup_vsc(struct intel_dp *intel_dp,
memset(&psr_vsc, 0, sizeof(psr_vsc));
psr_vsc.sdp_header.HB0 = 0;
- psr_vsc.sdp_header.HB1 = 0x7;
+ psr_vsc.sdp_header.HB1 = DP_SDP_VSC;
if (dev_priv->psr.psr2_support) {
/* Prepare VSC Header for SU as per EDP 1.4 spec, Table 6.11 */
--
2.16.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 5/5] drm/i915: Make use of unused variable in hsw_write_infoframe()
2018-02-14 0:29 [PATCH 1/5] drm/i915: add and enable DP AUX CH mutex José Roberto de Souza
` (2 preceding siblings ...)
2018-02-14 0:29 ` [PATCH 4/5] drm/i915: Replace magic number with macro defined by drm José Roberto de Souza
@ 2018-02-14 0:29 ` José Roberto de Souza
2018-02-14 13:33 ` Ville Syrjälä
2018-02-14 0:36 ` ✗ Fi.CI.BAT: failure for series starting with [1/5] drm/i915: add and enable DP AUX CH mutex Patchwork
` (3 subsequent siblings)
7 siblings, 1 reply; 18+ messages in thread
From: José Roberto de Souza @ 2018-02-14 0:29 UTC (permalink / raw)
To: intel-gfx
data_reg was not being used but it can be used to reduce the number of
calls to hsw_dip_data_reg() and just increment the reg by the size of
uint32.
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
drivers/gpu/drm/i915/intel_hdmi.c | 12 ++++--------
1 file changed, 4 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index f5d7bfb43006..ef258eac8ae8 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -393,15 +393,11 @@ static void hsw_write_infoframe(struct drm_encoder *encoder,
I915_WRITE(ctl_reg, val);
mmiowb();
- for (i = 0; i < len; i += 4) {
- I915_WRITE(hsw_dip_data_reg(dev_priv, cpu_transcoder,
- type, i >> 2), *data);
- data++;
- }
+ for (i = 0; i < len; i += 4, data++, data_reg.reg += 4)
+ I915_WRITE(data_reg, *data);
/* Write every possible data byte to force correct ECC calculation. */
- for (; i < data_size; i += 4)
- I915_WRITE(hsw_dip_data_reg(dev_priv, cpu_transcoder,
- type, i >> 2), 0);
+ for (; i < data_size; i += 4, data_reg.reg += 4)
+ I915_WRITE(data_reg, 0);
mmiowb();
val |= hsw_infoframe_enable(type);
--
2.16.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 18+ messages in thread
* ✗ Fi.CI.BAT: failure for series starting with [1/5] drm/i915: add and enable DP AUX CH mutex
2018-02-14 0:29 [PATCH 1/5] drm/i915: add and enable DP AUX CH mutex José Roberto de Souza
` (3 preceding siblings ...)
2018-02-14 0:29 ` [PATCH 5/5] drm/i915: Make use of unused variable in hsw_write_infoframe() José Roberto de Souza
@ 2018-02-14 0:36 ` Patchwork
2018-02-14 1:30 ` [PATCH 1/5] " Rodrigo Vivi
` (2 subsequent siblings)
7 siblings, 0 replies; 18+ messages in thread
From: Patchwork @ 2018-02-14 0:36 UTC (permalink / raw)
To: José Roberto de Souza; +Cc: intel-gfx
== Series Details ==
Series: series starting with [1/5] drm/i915: add and enable DP AUX CH mutex
URL : https://patchwork.freedesktop.org/series/38201/
State : failure
== Summary ==
CHK include/config/kernel.release
CHK include/generated/uapi/linux/version.h
CHK include/generated/utsrelease.h
CHK include/generated/bounds.h
CHK include/generated/timeconst.h
CHK include/generated/asm-offsets.h
CALL scripts/checksyscalls.sh
DESCEND objtool
CHK scripts/mod/devicetable-offsets.h
CHK include/generated/compile.h
CHK kernel/config_data.h
CC [M] drivers/gpu/drm/i915/intel_dp.o
drivers/gpu/drm/i915/intel_dp.c: In function ‘intel_dp_aux_ch’:
drivers/gpu/drm/i915/intel_dp.c:1290:2: error: expected expression before ‘}’ token
}
^
scripts/Makefile.build:316: recipe for target 'drivers/gpu/drm/i915/intel_dp.o' failed
make[4]: *** [drivers/gpu/drm/i915/intel_dp.o] Error 1
scripts/Makefile.build:575: recipe for target 'drivers/gpu/drm/i915' failed
make[3]: *** [drivers/gpu/drm/i915] Error 2
scripts/Makefile.build:575: recipe for target 'drivers/gpu/drm' failed
make[2]: *** [drivers/gpu/drm] Error 2
scripts/Makefile.build:575: recipe for target 'drivers/gpu' failed
make[1]: *** [drivers/gpu] Error 2
Makefile:1048: recipe for target 'drivers' failed
make: *** [drivers] Error 2
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/5] drm/i915: keep AUX powered while PSR is enabled
2018-02-14 0:29 ` [PATCH 2/5] drm/i915: keep AUX powered while PSR is enabled José Roberto de Souza
@ 2018-02-14 1:19 ` Rodrigo Vivi
0 siblings, 0 replies; 18+ messages in thread
From: Rodrigo Vivi @ 2018-02-14 1:19 UTC (permalink / raw)
To: José Roberto de Souza, intel-gfx
José Roberto de Souza <jose.souza@intel.com> writes:
> Hardware can send AUX transactions by it self when PSR is enabled and it
> expects that AUX is kept powered up.
>
> From spec:
>
> "Each DisplayPort Aux channel has an Aux IO Power Request. If an Aux
> channel will not be used, it does not need to be powered up.
>
> PSR spontaneously sends Aux transactions. If PSR is enabled on a port,
> then the associated Aux IO must be kept powered up."
>
> BSpec: 4230
> BSpec: 17722
It seems here this is only needed for AUX IO PWs.
So maybe for that we would need some thing like POWER_DOMAIN_PSR_AUX_* that is
part of {GLK,CNL}_DISPLAY_AUX_*_POWER_DOMAINS
So here we would get and put the specific PSR_AUX ones instead of the
whole dp aux. We don't need to keep the entire well up apparently.
At least I hope we don't.
>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
> drivers/gpu/drm/i915/intel_psr.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index 8b456e4e1b47..225fde979e57 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -516,6 +516,12 @@ void intel_psr_enable(struct intel_dp *intel_dp,
> goto unlock;
> }
>
> + /*
> + * hardware can send AUX transactions by it self when PSR is enabled,
> + * and it expects that AUX is kept powered up.
> + */
> + intel_display_power_get(dev_priv, intel_dp->aux_power_domain);
> +
> dev_priv->psr.psr2_support = crtc_state->has_psr2;
> dev_priv->psr.busy_frontbuffer_bits = 0;
>
> @@ -656,6 +662,8 @@ void intel_psr_disable(struct intel_dp *intel_dp,
> /* Disable PSR on Sink */
> drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG, 0);
>
> + intel_display_power_put(dev_priv, intel_dp->aux_power_domain);
> +
> dev_priv->psr.enabled = NULL;
> mutex_unlock(&dev_priv->psr.lock);
>
> --
> 2.16.1
>
> _______________________________________________
> 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] 18+ messages in thread
* Re: [PATCH 1/5] drm/i915: add and enable DP AUX CH mutex
2018-02-14 0:29 [PATCH 1/5] drm/i915: add and enable DP AUX CH mutex José Roberto de Souza
` (4 preceding siblings ...)
2018-02-14 0:36 ` ✗ Fi.CI.BAT: failure for series starting with [1/5] drm/i915: add and enable DP AUX CH mutex Patchwork
@ 2018-02-14 1:30 ` Rodrigo Vivi
2018-02-14 2:30 ` Souza, Jose
2018-02-14 13:43 ` Ville Syrjälä
2018-02-14 7:32 ` Jani Nikula
2018-02-14 9:47 ` Jani Nikula
7 siblings, 2 replies; 18+ messages in thread
From: Rodrigo Vivi @ 2018-02-14 1:30 UTC (permalink / raw)
To: José Roberto de Souza, intel-gfx
José Roberto de Souza <jose.souza@intel.com> writes:
> When PSR/PSR2 is enabled hardware can do AUX transactions by it self, so
> lets use the mutex register that is available in gen9+ to avoid concurrent
> access by hardware and driver.
>
> Reference: https://01.org/sites/default/files/documentation/intel-gfx-prm-osrc-skl-vol12-display.pdf
> Page 198 - AUX programming sequence
>
> As gen < 9 don't have the mutex PSR needs to be disabled, that is why the
> 'else TODO' was added. This is also backed by spec:
> "DDI A AUX channel transactions must not be sent while SRD is enabled.
> SRD must be completely disabled before a DDI A AUX channel transaction can
> be sent." BSpec: 7530
>
> Cc: Rodrigo Vivi <rodrigo.vivi@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 | 48 ++++++++++++++++++++++++++++++++++++++++
> drivers/gpu/drm/i915/intel_drv.h | 1 +
> drivers/gpu/drm/i915/intel_psr.c | 6 +++++
> 4 files changed, 64 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index f6afa5e5e7c1..5d7736117cb9 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -5318,6 +5318,7 @@ enum {
> #define _DPA_AUX_CH_DATA3 (dev_priv->info.display_mmio_offset + 0x6401c)
> #define _DPA_AUX_CH_DATA4 (dev_priv->info.display_mmio_offset + 0x64020)
> #define _DPA_AUX_CH_DATA5 (dev_priv->info.display_mmio_offset + 0x64024)
> +#define _DPA_AUX_CH_MUTEX (dev_priv->info.display_mmio_offset + 0x6402C)
>
> #define _DPB_AUX_CH_CTL (dev_priv->info.display_mmio_offset + 0x64110)
> #define _DPB_AUX_CH_DATA1 (dev_priv->info.display_mmio_offset + 0x64114)
> @@ -5325,6 +5326,7 @@ enum {
> #define _DPB_AUX_CH_DATA3 (dev_priv->info.display_mmio_offset + 0x6411c)
> #define _DPB_AUX_CH_DATA4 (dev_priv->info.display_mmio_offset + 0x64120)
> #define _DPB_AUX_CH_DATA5 (dev_priv->info.display_mmio_offset + 0x64124)
> +#define _DPB_AUX_CH_MUTEX (dev_priv->info.display_mmio_offset + 0x6412C)
>
> #define _DPC_AUX_CH_CTL (dev_priv->info.display_mmio_offset + 0x64210)
> #define _DPC_AUX_CH_DATA1 (dev_priv->info.display_mmio_offset + 0x64214)
> @@ -5332,6 +5334,7 @@ enum {
> #define _DPC_AUX_CH_DATA3 (dev_priv->info.display_mmio_offset + 0x6421c)
> #define _DPC_AUX_CH_DATA4 (dev_priv->info.display_mmio_offset + 0x64220)
> #define _DPC_AUX_CH_DATA5 (dev_priv->info.display_mmio_offset + 0x64224)
> +#define _DPC_AUX_CH_MUTEX (dev_priv->info.display_mmio_offset + 0x6422C)
>
> #define _DPD_AUX_CH_CTL (dev_priv->info.display_mmio_offset + 0x64310)
> #define _DPD_AUX_CH_DATA1 (dev_priv->info.display_mmio_offset + 0x64314)
> @@ -5339,6 +5342,7 @@ enum {
> #define _DPD_AUX_CH_DATA3 (dev_priv->info.display_mmio_offset + 0x6431c)
> #define _DPD_AUX_CH_DATA4 (dev_priv->info.display_mmio_offset + 0x64320)
> #define _DPD_AUX_CH_DATA5 (dev_priv->info.display_mmio_offset + 0x64324)
> +#define _DPD_AUX_CH_MUTEX (dev_priv->info.display_mmio_offset + 0x6432C)
>
> #define _DPF_AUX_CH_CTL (dev_priv->info.display_mmio_offset + 0x64510)
> #define _DPF_AUX_CH_DATA1 (dev_priv->info.display_mmio_offset + 0x64514)
> @@ -5346,9 +5350,11 @@ enum {
> #define _DPF_AUX_CH_DATA3 (dev_priv->info.display_mmio_offset + 0x6451c)
> #define _DPF_AUX_CH_DATA4 (dev_priv->info.display_mmio_offset + 0x64520)
> #define _DPF_AUX_CH_DATA5 (dev_priv->info.display_mmio_offset + 0x64524)
> +#define _DPF_AUX_CH_MUTEX (dev_priv->info.display_mmio_offset + 0x6452C)
>
> #define DP_AUX_CH_CTL(port) _MMIO_PORT(port, _DPA_AUX_CH_CTL, _DPB_AUX_CH_CTL)
> #define DP_AUX_CH_DATA(port, i) _MMIO(_PORT(port, _DPA_AUX_CH_DATA1, _DPB_AUX_CH_DATA1) + (i) * 4) /* 5 registers */
> +#define DP_AUX_CH_MUTEX(port) _MMIO_PORT(port, _DPA_AUX_CH_MUTEX, _DPB_AUX_CH_MUTEX)
>
> #define DP_AUX_CH_CTL_SEND_BUSY (1 << 31)
> #define DP_AUX_CH_CTL_DONE (1 << 30)
> @@ -5378,6 +5384,9 @@ enum {
> #define DP_AUX_CH_CTL_FW_SYNC_PULSE_SKL(c) (((c) - 1) << 5)
> #define DP_AUX_CH_CTL_SYNC_PULSE_SKL(c) ((c) - 1)
>
> +#define DP_AUX_CH_MUTEX_ENABLE (1 << 31)
> +#define DP_AUX_CH_MUTEX_STATUS (1 << 30)
> +
> /*
> * Computing GMCH M and N values for the Display Port link
> *
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index f20b25f98e5a..bd5f1bb730ff 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1081,6 +1081,28 @@ static uint32_t intel_dp_get_aux_send_ctl(struct intel_dp *intel_dp,
> aux_clock_divider);
> }
>
> +static bool intel_dp_aux_get(struct drm_i915_private *dev_priv,
> + struct intel_dp *intel_dp)
> +{
> + int try, ret;
Here you are not getting the aux. You are only getting checking if aux
is available.
My understanding of the spec is that the actual get is set Enable bit.
> +
> + for (try = 0, ret = -1; try < 3 && ret; try++)
> + ret = wait_for(!(I915_READ_NOTRACE(intel_dp->aux_ch_mutex_reg)
> + & DP_AUX_CH_MUTEX_STATUS), 0.5);
what about
intel_wait_for_register(dev_priv, intel_dp->aux_ch_mutex_reg,
DP_AUX_CH_MUTEX_STATUS, 0,
500)
?
> +
> + return ret == 0;
> +}
> +
> +static void intel_dp_aux_put(struct drm_i915_private *dev_priv,
> + struct intel_dp *intel_dp)
> +{
> + /* setting the status bit releases the mutex + keep set the bit
> + * to keep the mutex enabled.
> + */
> + I915_WRITE(intel_dp->aux_ch_mutex_reg, DP_AUX_CH_MUTEX_ENABLE |
The enable bit is optional here.
I believe it gets cleaner without it but with RMW.
> + DP_AUX_CH_MUTEX_STATUS);
> +}
> +
> static int
> intel_dp_aux_ch(struct intel_dp *intel_dp,
> const uint8_t *send, int send_bytes,
> @@ -1115,6 +1137,21 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
>
> intel_dp_check_edp(intel_dp);
>
> + /* Get hardware mutex when PSR is enabled */
> + if (dev_priv->psr.enabled == intel_dp) {
> + if (intel_dp->aux_ch_mutex_reg.reg) {
these checks could be inside get function.
> + if (intel_dp_aux_get(dev_priv, intel_dp)) {
> + DRM_WARN("dp_aux_ch port locked for too
> long");
as well the message DRM_ERROR is enough I thing.
> + ret = -EBUSY;
also specific errno
> + goto out_locked;
> + }
> + } else {
> + /* TODO: go to PSR exit state waiting for change before
> + * do a aux ch transaction.
> + */
> + }
> + }
> +
> /* Try to wait for any previous AUX channel activity */
> for (try = 0; try < 3; try++) {
> status = I915_READ_NOTRACE(ch_ctl);
> @@ -1243,7 +1280,16 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
> recv + i, recv_bytes - i);
>
> ret = recv_bytes;
> +
> out:
> + if (dev_priv->psr.enabled == intel_dp) {
we are checking psr.enable out of psr mutexes...
probably better to only do this once on get part
and if you got you put...
> + if (intel_dp->aux_ch_mutex_reg.reg)
> + intel_dp_aux_put(dev_priv, intel_dp);
> + else
> + /* TODO: schedule PSR work to active PSR again */
> + }
> +
> +out_locked:
> pm_qos_update_request(&dev_priv->pm_qos, PM_QOS_DEFAULT_VALUE);
>
> if (vdd)
> @@ -1496,6 +1542,8 @@ static void intel_aux_reg_init(struct intel_dp *intel_dp)
> intel_dp->aux_ch_ctl_reg = intel_aux_ctl_reg(dev_priv, port);
> for (i = 0; i < ARRAY_SIZE(intel_dp->aux_ch_data_reg); i++)
> intel_dp->aux_ch_data_reg[i] = intel_aux_data_reg(dev_priv, port, i);
> + if (INTEL_INFO(dev_priv)->gen >= 9)
> + intel_dp->aux_ch_mutex_reg = DP_AUX_CH_MUTEX(port);
> }
>
> static void
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 898064e8bea7..0d235b7dab21 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1040,6 +1040,7 @@ struct intel_dp {
> i915_reg_t output_reg;
> i915_reg_t aux_ch_ctl_reg;
> i915_reg_t aux_ch_data_reg[5];
> + i915_reg_t aux_ch_mutex_reg;
> uint32_t DP;
> int link_rate;
> uint8_t lane_count;
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index 2ef374f936b9..8b456e4e1b47 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -484,6 +484,9 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp,
> EDP_PSR_DEBUG_MASK_HPD |
> EDP_PSR_DEBUG_MASK_LPSP);
> }
> +
> + if (intel_dp->aux_ch_mutex_reg.reg)
> + I915_WRITE(intel_dp->aux_ch_mutex_reg,
> DP_AUX_CH_MUTEX_ENABLE);
My understanding is that set Enable is the actual get hw mutex,
so I don't believe you want do do this here.
Also you already check for psr.enable inside the aux transaction.
setting there should be enough.
> }
>
> /**
> @@ -617,6 +620,9 @@ static void hsw_psr_disable(struct intel_dp *intel_dp,
> else
> WARN_ON(I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE);
> }
> +
> + if (intel_dp->aux_ch_mutex_reg.reg)
> + I915_WRITE(intel_dp->aux_ch_mutex_reg, ~DP_AUX_CH_MUTEX_ENABLE);
> }
>
> /**
> --
> 2.16.1
>
> _______________________________________________
> 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] 18+ messages in thread
* Re: [PATCH 1/5] drm/i915: add and enable DP AUX CH mutex
2018-02-14 1:30 ` [PATCH 1/5] " Rodrigo Vivi
@ 2018-02-14 2:30 ` Souza, Jose
2018-02-14 13:43 ` Ville Syrjälä
1 sibling, 0 replies; 18+ messages in thread
From: Souza, Jose @ 2018-02-14 2:30 UTC (permalink / raw)
To: intel-gfx, Vivi, Rodrigo
On Tue, 2018-02-13 at 17:30 -0800, Rodrigo Vivi wrote:
> José Roberto de Souza <jose.souza@intel.com> writes:
>
> > When PSR/PSR2 is enabled hardware can do AUX transactions by it
> > self, so
> > lets use the mutex register that is available in gen9+ to avoid
> > concurrent
> > access by hardware and driver.
> >
> > Reference: https://01.org/sites/default/files/documentation/intel-g
> > fx-prm-osrc-skl-vol12-display.pdf
> > Page 198 - AUX programming sequence
> >
> > As gen < 9 don't have the mutex PSR needs to be disabled, that is
> > why the
> > 'else TODO' was added. This is also backed by spec:
> > "DDI A AUX channel transactions must not be sent while SRD is
> > enabled.
> > SRD must be completely disabled before a DDI A AUX channel
> > transaction can
> > be sent." BSpec: 7530
> >
> > Cc: Rodrigo Vivi <rodrigo.vivi@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 | 48
> > ++++++++++++++++++++++++++++++++++++++++
> > drivers/gpu/drm/i915/intel_drv.h | 1 +
> > drivers/gpu/drm/i915/intel_psr.c | 6 +++++
> > 4 files changed, 64 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > b/drivers/gpu/drm/i915/i915_reg.h
> > index f6afa5e5e7c1..5d7736117cb9 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -5318,6 +5318,7 @@ enum {
> > #define _DPA_AUX_CH_DATA3 (dev_priv-
> > >info.display_mmio_offset + 0x6401c)
> > #define _DPA_AUX_CH_DATA4 (dev_priv-
> > >info.display_mmio_offset + 0x64020)
> > #define _DPA_AUX_CH_DATA5 (dev_priv-
> > >info.display_mmio_offset + 0x64024)
> > +#define _DPA_AUX_CH_MUTEX (dev_priv-
> > >info.display_mmio_offset + 0x6402C)
> >
> > #define _DPB_AUX_CH_CTL (dev_priv-
> > >info.display_mmio_offset + 0x64110)
> > #define _DPB_AUX_CH_DATA1 (dev_priv-
> > >info.display_mmio_offset + 0x64114)
> > @@ -5325,6 +5326,7 @@ enum {
> > #define _DPB_AUX_CH_DATA3 (dev_priv-
> > >info.display_mmio_offset + 0x6411c)
> > #define _DPB_AUX_CH_DATA4 (dev_priv-
> > >info.display_mmio_offset + 0x64120)
> > #define _DPB_AUX_CH_DATA5 (dev_priv-
> > >info.display_mmio_offset + 0x64124)
> > +#define _DPB_AUX_CH_MUTEX (dev_priv-
> > >info.display_mmio_offset + 0x6412C)
> >
> > #define _DPC_AUX_CH_CTL (dev_priv-
> > >info.display_mmio_offset + 0x64210)
> > #define _DPC_AUX_CH_DATA1 (dev_priv-
> > >info.display_mmio_offset + 0x64214)
> > @@ -5332,6 +5334,7 @@ enum {
> > #define _DPC_AUX_CH_DATA3 (dev_priv-
> > >info.display_mmio_offset + 0x6421c)
> > #define _DPC_AUX_CH_DATA4 (dev_priv-
> > >info.display_mmio_offset + 0x64220)
> > #define _DPC_AUX_CH_DATA5 (dev_priv-
> > >info.display_mmio_offset + 0x64224)
> > +#define _DPC_AUX_CH_MUTEX (dev_priv-
> > >info.display_mmio_offset + 0x6422C)
> >
> > #define _DPD_AUX_CH_CTL (dev_priv-
> > >info.display_mmio_offset + 0x64310)
> > #define _DPD_AUX_CH_DATA1 (dev_priv-
> > >info.display_mmio_offset + 0x64314)
> > @@ -5339,6 +5342,7 @@ enum {
> > #define _DPD_AUX_CH_DATA3 (dev_priv-
> > >info.display_mmio_offset + 0x6431c)
> > #define _DPD_AUX_CH_DATA4 (dev_priv-
> > >info.display_mmio_offset + 0x64320)
> > #define _DPD_AUX_CH_DATA5 (dev_priv-
> > >info.display_mmio_offset + 0x64324)
> > +#define _DPD_AUX_CH_MUTEX (dev_priv-
> > >info.display_mmio_offset + 0x6432C)
> >
> > #define _DPF_AUX_CH_CTL (dev_priv-
> > >info.display_mmio_offset + 0x64510)
> > #define _DPF_AUX_CH_DATA1 (dev_priv-
> > >info.display_mmio_offset + 0x64514)
> > @@ -5346,9 +5350,11 @@ enum {
> > #define _DPF_AUX_CH_DATA3 (dev_priv-
> > >info.display_mmio_offset + 0x6451c)
> > #define _DPF_AUX_CH_DATA4 (dev_priv-
> > >info.display_mmio_offset + 0x64520)
> > #define _DPF_AUX_CH_DATA5 (dev_priv-
> > >info.display_mmio_offset + 0x64524)
> > +#define _DPF_AUX_CH_MUTEX (dev_priv-
> > >info.display_mmio_offset + 0x6452C)
> >
> > #define DP_AUX_CH_CTL(port) _MMIO_PORT(port,
> > _DPA_AUX_CH_CTL, _DPB_AUX_CH_CTL)
> > #define DP_AUX_CH_DATA(port, i) _MMIO(_PORT(port,
> > _DPA_AUX_CH_DATA1, _DPB_AUX_CH_DATA1) + (i) * 4) /* 5 registers */
> > +#define DP_AUX_CH_MUTEX(port) _MMIO_PORT(port,
> > _DPA_AUX_CH_MUTEX, _DPB_AUX_CH_MUTEX)
> >
> > #define DP_AUX_CH_CTL_SEND_BUSY (1 << 31)
> > #define DP_AUX_CH_CTL_DONE (1 << 30)
> > @@ -5378,6 +5384,9 @@ enum {
> > #define DP_AUX_CH_CTL_FW_SYNC_PULSE_SKL(c) (((c) - 1) << 5)
> > #define DP_AUX_CH_CTL_SYNC_PULSE_SKL(c) ((c) - 1)
> >
> > +#define DP_AUX_CH_MUTEX_ENABLE (1 << 31)
> > +#define DP_AUX_CH_MUTEX_STATUS (1 << 30)
> > +
> > /*
> > * Computing GMCH M and N values for the Display Port link
> > *
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > b/drivers/gpu/drm/i915/intel_dp.c
> > index f20b25f98e5a..bd5f1bb730ff 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -1081,6 +1081,28 @@ static uint32_t
> > intel_dp_get_aux_send_ctl(struct intel_dp *intel_dp,
> > aux_clock_divider)
> > ;
> > }
> >
> > +static bool intel_dp_aux_get(struct drm_i915_private *dev_priv,
> > + struct intel_dp *intel_dp)
> > +{
> > + int try, ret;
>
> Here you are not getting the aux. You are only getting checking if
> aux
> is available.
> My understanding of the spec is that the actual get is set Enable
> bit.
The spec is not clear but because of this 2 statements I think this is
correct:
- statement 1:
Release with (is optional) or without disabling MUTEX function.
DDI_AUX_MUTEX[31:30]= "11"
OR
DDI_AUX_MUTEX[31:30]="01"
So here you release the mutex by always seting the bit 30(status) and
is optinal disable the mutex.
- statement 2:
Enable MUTEX without changing MUTEX status
DDI_AUX_MUTEX[31]='1'
Also adding a few aux transactions before and after PSR exit/enable I
was able to confirm that the hardware got the mutex as the status bit
was set and after sleep for 500usec it was released.
>
> > +
> > + for (try = 0, ret = -1; try < 3 && ret; try++)
> > + ret = wait_for(!(I915_READ_NOTRACE(intel_dp-
> > >aux_ch_mutex_reg)
> > + & DP_AUX_CH_MUTEX_STATUS), 0.5);
>
> what about
> intel_wait_for_register(dev_priv, intel_dp->aux_ch_mutex_reg,
> DP_AUX_CH_MUTEX_STATUS, 0,
> 500)
> ?
This difference that I can see between wait_for() and
intel_wait_for_register() is that intel_wait_for_register() will call
intel_uncore_forcewake_get__locked()/put_locked() is used to kept the
hardware domain wake?
I'm fine with both but intel_wait_for_register() have a msec parameter
so the timeout would need to be 2msec or 3mesc(something between the
actual 1.5msec).
>
> > +
> > + return ret == 0;
> > +}
> > +
> > +static void intel_dp_aux_put(struct drm_i915_private *dev_priv,
> > + struct intel_dp *intel_dp)
> > +{
> > + /* setting the status bit releases the mutex + keep set
> > the bit
> > + * to keep the mutex enabled.
> > + */
> > + I915_WRITE(intel_dp->aux_ch_mutex_reg,
> > DP_AUX_CH_MUTEX_ENABLE |
>
> The enable bit is optional here.
> I believe it gets cleaner without it but with RMW.
See why I belive this is correct above.
>
> > + DP_AUX_CH_MUTEX_STATUS);
> > +}
> > +
> > static int
> > intel_dp_aux_ch(struct intel_dp *intel_dp,
> > const uint8_t *send, int send_bytes,
> > @@ -1115,6 +1137,21 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
> >
> > intel_dp_check_edp(intel_dp);
> >
> > + /* Get hardware mutex when PSR is enabled */
> > + if (dev_priv->psr.enabled == intel_dp) {
> > + if (intel_dp->aux_ch_mutex_reg.reg) {
>
> these checks could be inside get function.
Huum and in case of gen < 9, exit PSR inside of this function?
>
> > + if (intel_dp_aux_get(dev_priv, intel_dp))
> > {
> > + DRM_WARN("dp_aux_ch port locked
> > for too
> > long");
>
> as well the message DRM_ERROR is enough I thing.
ack
>
> > + ret = -EBUSY;
>
> also specific errno
ack
>
> > + goto out_locked;
> > + }
> > + } else {
> > + /* TODO: go to PSR exit state waiting for
> > change before
> > + * do a aux ch transaction.
> > + */
> > + }
> > + }
> > +
> > /* Try to wait for any previous AUX channel activity */
> > for (try = 0; try < 3; try++) {
> > status = I915_READ_NOTRACE(ch_ctl);
> > @@ -1243,7 +1280,16 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
> > recv + i, recv_bytes - i);
> >
> > ret = recv_bytes;
> > +
> > out:
> > + if (dev_priv->psr.enabled == intel_dp) {
>
> we are checking psr.enable out of psr mutexes...
> probably better to only do this once on get part
> and if you got you put...
ack
>
> > + if (intel_dp->aux_ch_mutex_reg.reg)
> > + intel_dp_aux_put(dev_priv, intel_dp);
> > + else
> > + /* TODO: schedule PSR work to active PSR
> > again */
> > + }
> > +
> > +out_locked:
> > pm_qos_update_request(&dev_priv->pm_qos,
> > PM_QOS_DEFAULT_VALUE);
> >
> > if (vdd)
> > @@ -1496,6 +1542,8 @@ static void intel_aux_reg_init(struct
> > intel_dp *intel_dp)
> > intel_dp->aux_ch_ctl_reg = intel_aux_ctl_reg(dev_priv,
> > port);
> > for (i = 0; i < ARRAY_SIZE(intel_dp->aux_ch_data_reg);
> > i++)
> > intel_dp->aux_ch_data_reg[i] =
> > intel_aux_data_reg(dev_priv, port, i);
> > + if (INTEL_INFO(dev_priv)->gen >= 9)
> > + intel_dp->aux_ch_mutex_reg =
> > DP_AUX_CH_MUTEX(port);
> > }
> >
> > static void
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > b/drivers/gpu/drm/i915/intel_drv.h
> > index 898064e8bea7..0d235b7dab21 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1040,6 +1040,7 @@ struct intel_dp {
> > i915_reg_t output_reg;
> > i915_reg_t aux_ch_ctl_reg;
> > i915_reg_t aux_ch_data_reg[5];
> > + i915_reg_t aux_ch_mutex_reg;
> > uint32_t DP;
> > int link_rate;
> > uint8_t lane_count;
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > b/drivers/gpu/drm/i915/intel_psr.c
> > index 2ef374f936b9..8b456e4e1b47 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -484,6 +484,9 @@ static void hsw_psr_enable_source(struct
> > intel_dp *intel_dp,
> > EDP_PSR_DEBUG_MASK_HPD |
> > EDP_PSR_DEBUG_MASK_LPSP);
> > }
> > +
> > + if (intel_dp->aux_ch_mutex_reg.reg)
> > + I915_WRITE(intel_dp->aux_ch_mutex_reg,
> > DP_AUX_CH_MUTEX_ENABLE);
>
> My understanding is that set Enable is the actual get hw mutex,
> so I don't believe you want do do this here.
>
> Also you already check for psr.enable inside the aux transaction.
> setting there should be enough.
See above.
>
> > }
> >
> > /**
> > @@ -617,6 +620,9 @@ static void hsw_psr_disable(struct intel_dp
> > *intel_dp,
> > else
> > WARN_ON(I915_READ(EDP_PSR_CTL) &
> > EDP_PSR_ENABLE);
> > }
> > +
> > + if (intel_dp->aux_ch_mutex_reg.reg)
> > + I915_WRITE(intel_dp->aux_ch_mutex_reg,
> > ~DP_AUX_CH_MUTEX_ENABLE);
> > }
> >
> > /**
> > --
> > 2.16.1
> >
> > _______________________________________________
> > 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] 18+ messages in thread
* Re: [PATCH 1/5] drm/i915: add and enable DP AUX CH mutex
2018-02-14 0:29 [PATCH 1/5] drm/i915: add and enable DP AUX CH mutex José Roberto de Souza
` (5 preceding siblings ...)
2018-02-14 1:30 ` [PATCH 1/5] " Rodrigo Vivi
@ 2018-02-14 7:32 ` Jani Nikula
2018-02-14 9:47 ` Jani Nikula
7 siblings, 0 replies; 18+ messages in thread
From: Jani Nikula @ 2018-02-14 7:32 UTC (permalink / raw)
To: José Roberto de Souza, intel-gfx; +Cc: Rodrigo Vivi
On Tue, 13 Feb 2018, José Roberto de Souza <jose.souza@intel.com> wrote:
> When PSR/PSR2 is enabled hardware can do AUX transactions by it self, so
> lets use the mutex register that is available in gen9+ to avoid concurrent
> access by hardware and driver.
>
> Reference: https://01.org/sites/default/files/documentation/intel-gfx-prm-osrc-skl-vol12-display.pdf
> Page 198 - AUX programming sequence
>
> As gen < 9 don't have the mutex PSR needs to be disabled, that is why the
> 'else TODO' was added. This is also backed by spec:
And proves the patches weren't compiled not to mention tested. :(
BR,
Jani.
> "DDI A AUX channel transactions must not be sent while SRD is enabled.
> SRD must be completely disabled before a DDI A AUX channel transaction can
> be sent." BSpec: 7530
>
> Cc: Rodrigo Vivi <rodrigo.vivi@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 | 48 ++++++++++++++++++++++++++++++++++++++++
> drivers/gpu/drm/i915/intel_drv.h | 1 +
> drivers/gpu/drm/i915/intel_psr.c | 6 +++++
> 4 files changed, 64 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index f6afa5e5e7c1..5d7736117cb9 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -5318,6 +5318,7 @@ enum {
> #define _DPA_AUX_CH_DATA3 (dev_priv->info.display_mmio_offset + 0x6401c)
> #define _DPA_AUX_CH_DATA4 (dev_priv->info.display_mmio_offset + 0x64020)
> #define _DPA_AUX_CH_DATA5 (dev_priv->info.display_mmio_offset + 0x64024)
> +#define _DPA_AUX_CH_MUTEX (dev_priv->info.display_mmio_offset + 0x6402C)
>
> #define _DPB_AUX_CH_CTL (dev_priv->info.display_mmio_offset + 0x64110)
> #define _DPB_AUX_CH_DATA1 (dev_priv->info.display_mmio_offset + 0x64114)
> @@ -5325,6 +5326,7 @@ enum {
> #define _DPB_AUX_CH_DATA3 (dev_priv->info.display_mmio_offset + 0x6411c)
> #define _DPB_AUX_CH_DATA4 (dev_priv->info.display_mmio_offset + 0x64120)
> #define _DPB_AUX_CH_DATA5 (dev_priv->info.display_mmio_offset + 0x64124)
> +#define _DPB_AUX_CH_MUTEX (dev_priv->info.display_mmio_offset + 0x6412C)
>
> #define _DPC_AUX_CH_CTL (dev_priv->info.display_mmio_offset + 0x64210)
> #define _DPC_AUX_CH_DATA1 (dev_priv->info.display_mmio_offset + 0x64214)
> @@ -5332,6 +5334,7 @@ enum {
> #define _DPC_AUX_CH_DATA3 (dev_priv->info.display_mmio_offset + 0x6421c)
> #define _DPC_AUX_CH_DATA4 (dev_priv->info.display_mmio_offset + 0x64220)
> #define _DPC_AUX_CH_DATA5 (dev_priv->info.display_mmio_offset + 0x64224)
> +#define _DPC_AUX_CH_MUTEX (dev_priv->info.display_mmio_offset + 0x6422C)
>
> #define _DPD_AUX_CH_CTL (dev_priv->info.display_mmio_offset + 0x64310)
> #define _DPD_AUX_CH_DATA1 (dev_priv->info.display_mmio_offset + 0x64314)
> @@ -5339,6 +5342,7 @@ enum {
> #define _DPD_AUX_CH_DATA3 (dev_priv->info.display_mmio_offset + 0x6431c)
> #define _DPD_AUX_CH_DATA4 (dev_priv->info.display_mmio_offset + 0x64320)
> #define _DPD_AUX_CH_DATA5 (dev_priv->info.display_mmio_offset + 0x64324)
> +#define _DPD_AUX_CH_MUTEX (dev_priv->info.display_mmio_offset + 0x6432C)
>
> #define _DPF_AUX_CH_CTL (dev_priv->info.display_mmio_offset + 0x64510)
> #define _DPF_AUX_CH_DATA1 (dev_priv->info.display_mmio_offset + 0x64514)
> @@ -5346,9 +5350,11 @@ enum {
> #define _DPF_AUX_CH_DATA3 (dev_priv->info.display_mmio_offset + 0x6451c)
> #define _DPF_AUX_CH_DATA4 (dev_priv->info.display_mmio_offset + 0x64520)
> #define _DPF_AUX_CH_DATA5 (dev_priv->info.display_mmio_offset + 0x64524)
> +#define _DPF_AUX_CH_MUTEX (dev_priv->info.display_mmio_offset + 0x6452C)
>
> #define DP_AUX_CH_CTL(port) _MMIO_PORT(port, _DPA_AUX_CH_CTL, _DPB_AUX_CH_CTL)
> #define DP_AUX_CH_DATA(port, i) _MMIO(_PORT(port, _DPA_AUX_CH_DATA1, _DPB_AUX_CH_DATA1) + (i) * 4) /* 5 registers */
> +#define DP_AUX_CH_MUTEX(port) _MMIO_PORT(port, _DPA_AUX_CH_MUTEX, _DPB_AUX_CH_MUTEX)
>
> #define DP_AUX_CH_CTL_SEND_BUSY (1 << 31)
> #define DP_AUX_CH_CTL_DONE (1 << 30)
> @@ -5378,6 +5384,9 @@ enum {
> #define DP_AUX_CH_CTL_FW_SYNC_PULSE_SKL(c) (((c) - 1) << 5)
> #define DP_AUX_CH_CTL_SYNC_PULSE_SKL(c) ((c) - 1)
>
> +#define DP_AUX_CH_MUTEX_ENABLE (1 << 31)
> +#define DP_AUX_CH_MUTEX_STATUS (1 << 30)
> +
> /*
> * Computing GMCH M and N values for the Display Port link
> *
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index f20b25f98e5a..bd5f1bb730ff 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1081,6 +1081,28 @@ static uint32_t intel_dp_get_aux_send_ctl(struct intel_dp *intel_dp,
> aux_clock_divider);
> }
>
> +static bool intel_dp_aux_get(struct drm_i915_private *dev_priv,
> + struct intel_dp *intel_dp)
> +{
> + int try, ret;
> +
> + for (try = 0, ret = -1; try < 3 && ret; try++)
> + ret = wait_for(!(I915_READ_NOTRACE(intel_dp->aux_ch_mutex_reg)
> + & DP_AUX_CH_MUTEX_STATUS), 0.5);
> +
> + return ret == 0;
> +}
> +
> +static void intel_dp_aux_put(struct drm_i915_private *dev_priv,
> + struct intel_dp *intel_dp)
> +{
> + /* setting the status bit releases the mutex + keep set the bit
> + * to keep the mutex enabled.
> + */
> + I915_WRITE(intel_dp->aux_ch_mutex_reg, DP_AUX_CH_MUTEX_ENABLE |
> + DP_AUX_CH_MUTEX_STATUS);
> +}
> +
> static int
> intel_dp_aux_ch(struct intel_dp *intel_dp,
> const uint8_t *send, int send_bytes,
> @@ -1115,6 +1137,21 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
>
> intel_dp_check_edp(intel_dp);
>
> + /* Get hardware mutex when PSR is enabled */
> + if (dev_priv->psr.enabled == intel_dp) {
> + if (intel_dp->aux_ch_mutex_reg.reg) {
> + if (intel_dp_aux_get(dev_priv, intel_dp)) {
> + DRM_WARN("dp_aux_ch port locked for too long");
> + ret = -EBUSY;
> + goto out_locked;
> + }
> + } else {
> + /* TODO: go to PSR exit state waiting for change before
> + * do a aux ch transaction.
> + */
> + }
> + }
> +
> /* Try to wait for any previous AUX channel activity */
> for (try = 0; try < 3; try++) {
> status = I915_READ_NOTRACE(ch_ctl);
> @@ -1243,7 +1280,16 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
> recv + i, recv_bytes - i);
>
> ret = recv_bytes;
> +
> out:
> + if (dev_priv->psr.enabled == intel_dp) {
> + if (intel_dp->aux_ch_mutex_reg.reg)
> + intel_dp_aux_put(dev_priv, intel_dp);
> + else
> + /* TODO: schedule PSR work to active PSR again */
> + }
> +
> +out_locked:
> pm_qos_update_request(&dev_priv->pm_qos, PM_QOS_DEFAULT_VALUE);
>
> if (vdd)
> @@ -1496,6 +1542,8 @@ static void intel_aux_reg_init(struct intel_dp *intel_dp)
> intel_dp->aux_ch_ctl_reg = intel_aux_ctl_reg(dev_priv, port);
> for (i = 0; i < ARRAY_SIZE(intel_dp->aux_ch_data_reg); i++)
> intel_dp->aux_ch_data_reg[i] = intel_aux_data_reg(dev_priv, port, i);
> + if (INTEL_INFO(dev_priv)->gen >= 9)
> + intel_dp->aux_ch_mutex_reg = DP_AUX_CH_MUTEX(port);
> }
>
> static void
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 898064e8bea7..0d235b7dab21 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1040,6 +1040,7 @@ struct intel_dp {
> i915_reg_t output_reg;
> i915_reg_t aux_ch_ctl_reg;
> i915_reg_t aux_ch_data_reg[5];
> + i915_reg_t aux_ch_mutex_reg;
> uint32_t DP;
> int link_rate;
> uint8_t lane_count;
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index 2ef374f936b9..8b456e4e1b47 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -484,6 +484,9 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp,
> EDP_PSR_DEBUG_MASK_HPD |
> EDP_PSR_DEBUG_MASK_LPSP);
> }
> +
> + if (intel_dp->aux_ch_mutex_reg.reg)
> + I915_WRITE(intel_dp->aux_ch_mutex_reg, DP_AUX_CH_MUTEX_ENABLE);
> }
>
> /**
> @@ -617,6 +620,9 @@ static void hsw_psr_disable(struct intel_dp *intel_dp,
> else
> WARN_ON(I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE);
> }
> +
> + if (intel_dp->aux_ch_mutex_reg.reg)
> + I915_WRITE(intel_dp->aux_ch_mutex_reg, ~DP_AUX_CH_MUTEX_ENABLE);
> }
>
> /**
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/5] drm/i915: add and enable DP AUX CH mutex
2018-02-14 0:29 [PATCH 1/5] drm/i915: add and enable DP AUX CH mutex José Roberto de Souza
` (6 preceding siblings ...)
2018-02-14 7:32 ` Jani Nikula
@ 2018-02-14 9:47 ` Jani Nikula
2018-02-14 16:21 ` Rodrigo Vivi
7 siblings, 1 reply; 18+ messages in thread
From: Jani Nikula @ 2018-02-14 9:47 UTC (permalink / raw)
To: José Roberto de Souza, intel-gfx; +Cc: Rodrigo Vivi
On Tue, 13 Feb 2018, José Roberto de Souza <jose.souza@intel.com> wrote:
> When PSR/PSR2 is enabled hardware can do AUX transactions by it self, so
> lets use the mutex register that is available in gen9+ to avoid concurrent
> access by hardware and driver.
>
> Reference: https://01.org/sites/default/files/documentation/intel-gfx-prm-osrc-skl-vol12-display.pdf
> Page 198 - AUX programming sequence
>
> As gen < 9 don't have the mutex PSR needs to be disabled, that is why the
> 'else TODO' was added. This is also backed by spec:
> "DDI A AUX channel transactions must not be sent while SRD is enabled.
> SRD must be completely disabled before a DDI A AUX channel transaction can
> be sent." BSpec: 7530
We explicitly rejected the use of the hw mutex way back when Skylake was
enabled. At that time the understanding was that the hw will try to do
the arbitration itself. Has something changed?
Some comments inline.
>
> Cc: Rodrigo Vivi <rodrigo.vivi@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 | 48 ++++++++++++++++++++++++++++++++++++++++
> drivers/gpu/drm/i915/intel_drv.h | 1 +
> drivers/gpu/drm/i915/intel_psr.c | 6 +++++
> 4 files changed, 64 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index f6afa5e5e7c1..5d7736117cb9 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -5318,6 +5318,7 @@ enum {
> #define _DPA_AUX_CH_DATA3 (dev_priv->info.display_mmio_offset + 0x6401c)
> #define _DPA_AUX_CH_DATA4 (dev_priv->info.display_mmio_offset + 0x64020)
> #define _DPA_AUX_CH_DATA5 (dev_priv->info.display_mmio_offset + 0x64024)
> +#define _DPA_AUX_CH_MUTEX (dev_priv->info.display_mmio_offset + 0x6402C)
>
> #define _DPB_AUX_CH_CTL (dev_priv->info.display_mmio_offset + 0x64110)
> #define _DPB_AUX_CH_DATA1 (dev_priv->info.display_mmio_offset + 0x64114)
> @@ -5325,6 +5326,7 @@ enum {
> #define _DPB_AUX_CH_DATA3 (dev_priv->info.display_mmio_offset + 0x6411c)
> #define _DPB_AUX_CH_DATA4 (dev_priv->info.display_mmio_offset + 0x64120)
> #define _DPB_AUX_CH_DATA5 (dev_priv->info.display_mmio_offset + 0x64124)
> +#define _DPB_AUX_CH_MUTEX (dev_priv->info.display_mmio_offset + 0x6412C)
>
> #define _DPC_AUX_CH_CTL (dev_priv->info.display_mmio_offset + 0x64210)
> #define _DPC_AUX_CH_DATA1 (dev_priv->info.display_mmio_offset + 0x64214)
> @@ -5332,6 +5334,7 @@ enum {
> #define _DPC_AUX_CH_DATA3 (dev_priv->info.display_mmio_offset + 0x6421c)
> #define _DPC_AUX_CH_DATA4 (dev_priv->info.display_mmio_offset + 0x64220)
> #define _DPC_AUX_CH_DATA5 (dev_priv->info.display_mmio_offset + 0x64224)
> +#define _DPC_AUX_CH_MUTEX (dev_priv->info.display_mmio_offset + 0x6422C)
>
> #define _DPD_AUX_CH_CTL (dev_priv->info.display_mmio_offset + 0x64310)
> #define _DPD_AUX_CH_DATA1 (dev_priv->info.display_mmio_offset + 0x64314)
> @@ -5339,6 +5342,7 @@ enum {
> #define _DPD_AUX_CH_DATA3 (dev_priv->info.display_mmio_offset + 0x6431c)
> #define _DPD_AUX_CH_DATA4 (dev_priv->info.display_mmio_offset + 0x64320)
> #define _DPD_AUX_CH_DATA5 (dev_priv->info.display_mmio_offset + 0x64324)
> +#define _DPD_AUX_CH_MUTEX (dev_priv->info.display_mmio_offset + 0x6432C)
>
> #define _DPF_AUX_CH_CTL (dev_priv->info.display_mmio_offset + 0x64510)
> #define _DPF_AUX_CH_DATA1 (dev_priv->info.display_mmio_offset + 0x64514)
> @@ -5346,9 +5350,11 @@ enum {
> #define _DPF_AUX_CH_DATA3 (dev_priv->info.display_mmio_offset + 0x6451c)
> #define _DPF_AUX_CH_DATA4 (dev_priv->info.display_mmio_offset + 0x64520)
> #define _DPF_AUX_CH_DATA5 (dev_priv->info.display_mmio_offset + 0x64524)
> +#define _DPF_AUX_CH_MUTEX (dev_priv->info.display_mmio_offset + 0x6452C)
>
> #define DP_AUX_CH_CTL(port) _MMIO_PORT(port, _DPA_AUX_CH_CTL, _DPB_AUX_CH_CTL)
> #define DP_AUX_CH_DATA(port, i) _MMIO(_PORT(port, _DPA_AUX_CH_DATA1, _DPB_AUX_CH_DATA1) + (i) * 4) /* 5 registers */
> +#define DP_AUX_CH_MUTEX(port) _MMIO_PORT(port, _DPA_AUX_CH_MUTEX, _DPB_AUX_CH_MUTEX)
>
> #define DP_AUX_CH_CTL_SEND_BUSY (1 << 31)
> #define DP_AUX_CH_CTL_DONE (1 << 30)
> @@ -5378,6 +5384,9 @@ enum {
> #define DP_AUX_CH_CTL_FW_SYNC_PULSE_SKL(c) (((c) - 1) << 5)
> #define DP_AUX_CH_CTL_SYNC_PULSE_SKL(c) ((c) - 1)
>
> +#define DP_AUX_CH_MUTEX_ENABLE (1 << 31)
> +#define DP_AUX_CH_MUTEX_STATUS (1 << 30)
The bit definitions should immediately follow the register offset
definition. I.e. DP_AUX_CH_MUTEX() should be right above these.
> +
> /*
> * Computing GMCH M and N values for the Display Port link
> *
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index f20b25f98e5a..bd5f1bb730ff 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1081,6 +1081,28 @@ static uint32_t intel_dp_get_aux_send_ctl(struct intel_dp *intel_dp,
> aux_clock_divider);
> }
>
> +static bool intel_dp_aux_get(struct drm_i915_private *dev_priv,
> + struct intel_dp *intel_dp)
You can get at dev_priv from intel_dp, please don't pass both.
The names _get/_put imply the calls can be nested, which is not the case
here. Please use _lock/_unlock instead.
If the locking can fail, call it _trylock.
> +{
> + int try, ret;
> +
> + for (try = 0, ret = -1; try < 3 && ret; try++)
> + ret = wait_for(!(I915_READ_NOTRACE(intel_dp->aux_ch_mutex_reg)
> + & DP_AUX_CH_MUTEX_STATUS), 0.5);
0.5? Really?
What's the point of the loop around the wait_for? wait_for already loops
with increasing delays in between the condition check.
> +
> + return ret == 0;
> +}
> +
> +static void intel_dp_aux_put(struct drm_i915_private *dev_priv,
> + struct intel_dp *intel_dp)
> +{
> + /* setting the status bit releases the mutex + keep set the bit
> + * to keep the mutex enabled.
> + */
> + I915_WRITE(intel_dp->aux_ch_mutex_reg, DP_AUX_CH_MUTEX_ENABLE |
> + DP_AUX_CH_MUTEX_STATUS);
> +}
> +
> static int
> intel_dp_aux_ch(struct intel_dp *intel_dp,
> const uint8_t *send, int send_bytes,
> @@ -1115,6 +1137,21 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
>
> intel_dp_check_edp(intel_dp);
>
> + /* Get hardware mutex when PSR is enabled */
> + if (dev_priv->psr.enabled == intel_dp) {
> + if (intel_dp->aux_ch_mutex_reg.reg) {
See i915_mmio_reg_valid(). But I'd rather the check was within the
locking primitives.
Overall I think mixing all this PSR conditional stuff in the aux channel
primitives is very, very wrong.
At the very least this is racy with PSR enabling/disabling.
> + if (intel_dp_aux_get(dev_priv, intel_dp)) {
> + DRM_WARN("dp_aux_ch port locked for too long");
> + ret = -EBUSY;
> + goto out_locked;
> + }
> + } else {
> + /* TODO: go to PSR exit state waiting for change before
> + * do a aux ch transaction.
> + */
> + }
> + }
> +
> /* Try to wait for any previous AUX channel activity */
> for (try = 0; try < 3; try++) {
> status = I915_READ_NOTRACE(ch_ctl);
> @@ -1243,7 +1280,16 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
> recv + i, recv_bytes - i);
>
> ret = recv_bytes;
> +
> out:
> + if (dev_priv->psr.enabled == intel_dp) {
> + if (intel_dp->aux_ch_mutex_reg.reg)
> + intel_dp_aux_put(dev_priv, intel_dp);
> + else
> + /* TODO: schedule PSR work to active PSR again */
> + }
> +
> +out_locked:
> pm_qos_update_request(&dev_priv->pm_qos, PM_QOS_DEFAULT_VALUE);
>
> if (vdd)
> @@ -1496,6 +1542,8 @@ static void intel_aux_reg_init(struct intel_dp *intel_dp)
> intel_dp->aux_ch_ctl_reg = intel_aux_ctl_reg(dev_priv, port);
> for (i = 0; i < ARRAY_SIZE(intel_dp->aux_ch_data_reg); i++)
> intel_dp->aux_ch_data_reg[i] = intel_aux_data_reg(dev_priv, port, i);
> + if (INTEL_INFO(dev_priv)->gen >= 9)
> + intel_dp->aux_ch_mutex_reg = DP_AUX_CH_MUTEX(port);
> }
>
> static void
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 898064e8bea7..0d235b7dab21 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1040,6 +1040,7 @@ struct intel_dp {
> i915_reg_t output_reg;
> i915_reg_t aux_ch_ctl_reg;
> i915_reg_t aux_ch_data_reg[5];
> + i915_reg_t aux_ch_mutex_reg;
> uint32_t DP;
> int link_rate;
> uint8_t lane_count;
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index 2ef374f936b9..8b456e4e1b47 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -484,6 +484,9 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp,
> EDP_PSR_DEBUG_MASK_HPD |
> EDP_PSR_DEBUG_MASK_LPSP);
> }
> +
> + if (intel_dp->aux_ch_mutex_reg.reg)
> + I915_WRITE(intel_dp->aux_ch_mutex_reg, DP_AUX_CH_MUTEX_ENABLE);
You can't just go stomping on the register without locking. Racy.
> }
>
> /**
> @@ -617,6 +620,9 @@ static void hsw_psr_disable(struct intel_dp *intel_dp,
> else
> WARN_ON(I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE);
> }
> +
> + if (intel_dp->aux_ch_mutex_reg.reg)
> + I915_WRITE(intel_dp->aux_ch_mutex_reg, ~DP_AUX_CH_MUTEX_ENABLE);
Are you sure you want to set all other bits than enable?
BR,
Jani.
> }
>
> /**
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 5/5] drm/i915: Make use of unused variable in hsw_write_infoframe()
2018-02-14 0:29 ` [PATCH 5/5] drm/i915: Make use of unused variable in hsw_write_infoframe() José Roberto de Souza
@ 2018-02-14 13:33 ` Ville Syrjälä
2018-02-14 13:37 ` Chris Wilson
2018-02-16 8:41 ` Jani Nikula
0 siblings, 2 replies; 18+ messages in thread
From: Ville Syrjälä @ 2018-02-14 13:33 UTC (permalink / raw)
To: José Roberto de Souza; +Cc: intel-gfx
On Tue, Feb 13, 2018 at 04:29:17PM -0800, José Roberto de Souza wrote:
> data_reg was not being used but it can be used to reduce the number of
> calls to hsw_dip_data_reg() and just increment the reg by the size of
> uint32.
>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
> drivers/gpu/drm/i915/intel_hdmi.c | 12 ++++--------
> 1 file changed, 4 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index f5d7bfb43006..ef258eac8ae8 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -393,15 +393,11 @@ static void hsw_write_infoframe(struct drm_encoder *encoder,
> I915_WRITE(ctl_reg, val);
>
> mmiowb();
> - for (i = 0; i < len; i += 4) {
> - I915_WRITE(hsw_dip_data_reg(dev_priv, cpu_transcoder,
> - type, i >> 2), *data);a
i>>2 != 0
just kill 'data_reg'
> - data++;
> - }
> + for (i = 0; i < len; i += 4, data++, data_reg.reg += 4)
> + I915_WRITE(data_reg, *data);
> /* Write every possible data byte to force correct ECC calculation. */
> - for (; i < data_size; i += 4)
> - I915_WRITE(hsw_dip_data_reg(dev_priv, cpu_transcoder,
> - type, i >> 2), 0);
> + for (; i < data_size; i += 4, data_reg.reg += 4)
> + I915_WRITE(data_reg, 0);
> mmiowb();
>
> val |= hsw_infoframe_enable(type);
> --
> 2.16.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 5/5] drm/i915: Make use of unused variable in hsw_write_infoframe()
2018-02-14 13:33 ` Ville Syrjälä
@ 2018-02-14 13:37 ` Chris Wilson
2018-02-14 14:29 ` Ville Syrjälä
2018-02-16 8:41 ` Jani Nikula
1 sibling, 1 reply; 18+ messages in thread
From: Chris Wilson @ 2018-02-14 13:37 UTC (permalink / raw)
To: Ville Syrjälä, José Roberto de Souza; +Cc: intel-gfx
Quoting Ville Syrjälä (2018-02-14 13:33:03)
> On Tue, Feb 13, 2018 at 04:29:17PM -0800, José Roberto de Souza wrote:
> > data_reg was not being used but it can be used to reduce the number of
> > calls to hsw_dip_data_reg() and just increment the reg by the size of
> > uint32.
> >
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> > drivers/gpu/drm/i915/intel_hdmi.c | 12 ++++--------
> > 1 file changed, 4 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> > index f5d7bfb43006..ef258eac8ae8 100644
> > --- a/drivers/gpu/drm/i915/intel_hdmi.c
> > +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> > @@ -393,15 +393,11 @@ static void hsw_write_infoframe(struct drm_encoder *encoder,
> > I915_WRITE(ctl_reg, val);
> >
> > mmiowb();
mmiowb() is just a compiler barrier on x86, and meaningless wrt to our
uncached mmio. If you need a full mmio barrier, use POSTING_READ().
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/5] drm/i915: add and enable DP AUX CH mutex
2018-02-14 1:30 ` [PATCH 1/5] " Rodrigo Vivi
2018-02-14 2:30 ` Souza, Jose
@ 2018-02-14 13:43 ` Ville Syrjälä
1 sibling, 0 replies; 18+ messages in thread
From: Ville Syrjälä @ 2018-02-14 13:43 UTC (permalink / raw)
To: Rodrigo Vivi; +Cc: intel-gfx
On Tue, Feb 13, 2018 at 05:30:29PM -0800, Rodrigo Vivi wrote:
> José Roberto de Souza <jose.souza@intel.com> writes:
>
> > When PSR/PSR2 is enabled hardware can do AUX transactions by it self, so
> > lets use the mutex register that is available in gen9+ to avoid concurrent
> > access by hardware and driver.
> >
> > Reference: https://01.org/sites/default/files/documentation/intel-gfx-prm-osrc-skl-vol12-display.pdf
> > Page 198 - AUX programming sequence
> >
> > As gen < 9 don't have the mutex PSR needs to be disabled, that is why the
> > 'else TODO' was added. This is also backed by spec:
> > "DDI A AUX channel transactions must not be sent while SRD is enabled.
> > SRD must be completely disabled before a DDI A AUX channel transaction can
> > be sent." BSpec: 7530
> >
> > Cc: Rodrigo Vivi <rodrigo.vivi@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 | 48 ++++++++++++++++++++++++++++++++++++++++
> > drivers/gpu/drm/i915/intel_drv.h | 1 +
> > drivers/gpu/drm/i915/intel_psr.c | 6 +++++
> > 4 files changed, 64 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index f6afa5e5e7c1..5d7736117cb9 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -5318,6 +5318,7 @@ enum {
> > #define _DPA_AUX_CH_DATA3 (dev_priv->info.display_mmio_offset + 0x6401c)
> > #define _DPA_AUX_CH_DATA4 (dev_priv->info.display_mmio_offset + 0x64020)
> > #define _DPA_AUX_CH_DATA5 (dev_priv->info.display_mmio_offset + 0x64024)
> > +#define _DPA_AUX_CH_MUTEX (dev_priv->info.display_mmio_offset + 0x6402C)
> >
> > #define _DPB_AUX_CH_CTL (dev_priv->info.display_mmio_offset + 0x64110)
> > #define _DPB_AUX_CH_DATA1 (dev_priv->info.display_mmio_offset + 0x64114)
> > @@ -5325,6 +5326,7 @@ enum {
> > #define _DPB_AUX_CH_DATA3 (dev_priv->info.display_mmio_offset + 0x6411c)
> > #define _DPB_AUX_CH_DATA4 (dev_priv->info.display_mmio_offset + 0x64120)
> > #define _DPB_AUX_CH_DATA5 (dev_priv->info.display_mmio_offset + 0x64124)
> > +#define _DPB_AUX_CH_MUTEX (dev_priv->info.display_mmio_offset + 0x6412C)
> >
> > #define _DPC_AUX_CH_CTL (dev_priv->info.display_mmio_offset + 0x64210)
> > #define _DPC_AUX_CH_DATA1 (dev_priv->info.display_mmio_offset + 0x64214)
> > @@ -5332,6 +5334,7 @@ enum {
> > #define _DPC_AUX_CH_DATA3 (dev_priv->info.display_mmio_offset + 0x6421c)
> > #define _DPC_AUX_CH_DATA4 (dev_priv->info.display_mmio_offset + 0x64220)
> > #define _DPC_AUX_CH_DATA5 (dev_priv->info.display_mmio_offset + 0x64224)
> > +#define _DPC_AUX_CH_MUTEX (dev_priv->info.display_mmio_offset + 0x6422C)
> >
> > #define _DPD_AUX_CH_CTL (dev_priv->info.display_mmio_offset + 0x64310)
> > #define _DPD_AUX_CH_DATA1 (dev_priv->info.display_mmio_offset + 0x64314)
> > @@ -5339,6 +5342,7 @@ enum {
> > #define _DPD_AUX_CH_DATA3 (dev_priv->info.display_mmio_offset + 0x6431c)
> > #define _DPD_AUX_CH_DATA4 (dev_priv->info.display_mmio_offset + 0x64320)
> > #define _DPD_AUX_CH_DATA5 (dev_priv->info.display_mmio_offset + 0x64324)
> > +#define _DPD_AUX_CH_MUTEX (dev_priv->info.display_mmio_offset + 0x6432C)
> >
> > #define _DPF_AUX_CH_CTL (dev_priv->info.display_mmio_offset + 0x64510)
> > #define _DPF_AUX_CH_DATA1 (dev_priv->info.display_mmio_offset + 0x64514)
> > @@ -5346,9 +5350,11 @@ enum {
> > #define _DPF_AUX_CH_DATA3 (dev_priv->info.display_mmio_offset + 0x6451c)
> > #define _DPF_AUX_CH_DATA4 (dev_priv->info.display_mmio_offset + 0x64520)
> > #define _DPF_AUX_CH_DATA5 (dev_priv->info.display_mmio_offset + 0x64524)
> > +#define _DPF_AUX_CH_MUTEX (dev_priv->info.display_mmio_offset + 0x6452C)
> >
> > #define DP_AUX_CH_CTL(port) _MMIO_PORT(port, _DPA_AUX_CH_CTL, _DPB_AUX_CH_CTL)
> > #define DP_AUX_CH_DATA(port, i) _MMIO(_PORT(port, _DPA_AUX_CH_DATA1, _DPB_AUX_CH_DATA1) + (i) * 4) /* 5 registers */
> > +#define DP_AUX_CH_MUTEX(port) _MMIO_PORT(port, _DPA_AUX_CH_MUTEX, _DPB_AUX_CH_MUTEX)
> >
> > #define DP_AUX_CH_CTL_SEND_BUSY (1 << 31)
> > #define DP_AUX_CH_CTL_DONE (1 << 30)
> > @@ -5378,6 +5384,9 @@ enum {
> > #define DP_AUX_CH_CTL_FW_SYNC_PULSE_SKL(c) (((c) - 1) << 5)
> > #define DP_AUX_CH_CTL_SYNC_PULSE_SKL(c) ((c) - 1)
> >
> > +#define DP_AUX_CH_MUTEX_ENABLE (1 << 31)
> > +#define DP_AUX_CH_MUTEX_STATUS (1 << 30)
> > +
> > /*
> > * Computing GMCH M and N values for the Display Port link
> > *
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index f20b25f98e5a..bd5f1bb730ff 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -1081,6 +1081,28 @@ static uint32_t intel_dp_get_aux_send_ctl(struct intel_dp *intel_dp,
> > aux_clock_divider);
> > }
> >
> > +static bool intel_dp_aux_get(struct drm_i915_private *dev_priv,
> > + struct intel_dp *intel_dp)
> > +{
> > + int try, ret;
>
> Here you are not getting the aux. You are only getting checking if aux
> is available.
> My understanding of the spec is that the actual get is set Enable bit.
>
> > +
> > + for (try = 0, ret = -1; try < 3 && ret; try++)
> > + ret = wait_for(!(I915_READ_NOTRACE(intel_dp->aux_ch_mutex_reg)
> > + & DP_AUX_CH_MUTEX_STATUS), 0.5);
>
> what about
> intel_wait_for_register(dev_priv, intel_dp->aux_ch_mutex_reg,
> DP_AUX_CH_MUTEX_STATUS, 0,
> 500)
IIRC when I asked Art about the mutex he indicated that the hardware has
no magic fair arbitration mechanism. So I think what we want to do is
first check if the mutex is free, if not we could try to wait for the
expected 500us (or poll somewhat infrequently), but if after a while
the mutex still isn't free we should probably start hammering it hard
so that have a better chance of actually getting it.
> ?
>
> > +
> > + return ret == 0;
> > +}
> > +
> > +static void intel_dp_aux_put(struct drm_i915_private *dev_priv,
> > + struct intel_dp *intel_dp)
> > +{
> > + /* setting the status bit releases the mutex + keep set the bit
> > + * to keep the mutex enabled.
> > + */
> > + I915_WRITE(intel_dp->aux_ch_mutex_reg, DP_AUX_CH_MUTEX_ENABLE |
>
> The enable bit is optional here.
> I believe it gets cleaner without it but with RMW.
>
> > + DP_AUX_CH_MUTEX_STATUS);
> > +}
> > +
> > static int
> > intel_dp_aux_ch(struct intel_dp *intel_dp,
> > const uint8_t *send, int send_bytes,
> > @@ -1115,6 +1137,21 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
> >
> > intel_dp_check_edp(intel_dp);
> >
> > + /* Get hardware mutex when PSR is enabled */
> > + if (dev_priv->psr.enabled == intel_dp) {
> > + if (intel_dp->aux_ch_mutex_reg.reg) {
>
> these checks could be inside get function.
>
> > + if (intel_dp_aux_get(dev_priv, intel_dp)) {
> > + DRM_WARN("dp_aux_ch port locked for too
> > long");
>
> as well the message DRM_ERROR is enough I thing.
>
> > + ret = -EBUSY;
> also specific errno
>
> > + goto out_locked;
> > + }
> > + } else {
> > + /* TODO: go to PSR exit state waiting for change before
> > + * do a aux ch transaction.
> > + */
> > + }
> > + }
This looks needlessly complicated. IMO it should just be something like:
if (!skl_aux_hw_mutex_get())
goto fail;
And I'm thinking we should just ignore the PSR on vs. off case,
and just do this for eDP always. I can't recall if GTC is a thing
for normak DP, but if it is and we ever enable it then we'd obviously
need to start doing this for all DP port.
> > +
> > /* Try to wait for any previous AUX channel activity */
> > for (try = 0; try < 3; try++) {
> > status = I915_READ_NOTRACE(ch_ctl);
> > @@ -1243,7 +1280,16 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
> > recv + i, recv_bytes - i);
> >
> > ret = recv_bytes;
> > +
> > out:
> > + if (dev_priv->psr.enabled == intel_dp) {
>
> we are checking psr.enable out of psr mutexes...
> probably better to only do this once on get part
> and if you got you put...
>
> > + if (intel_dp->aux_ch_mutex_reg.reg)
> > + intel_dp_aux_put(dev_priv, intel_dp);
> > + else
> > + /* TODO: schedule PSR work to active PSR again */
> > + }
> > +
> > +out_locked:
> > pm_qos_update_request(&dev_priv->pm_qos, PM_QOS_DEFAULT_VALUE);
> >
> > if (vdd)
> > @@ -1496,6 +1542,8 @@ static void intel_aux_reg_init(struct intel_dp *intel_dp)
> > intel_dp->aux_ch_ctl_reg = intel_aux_ctl_reg(dev_priv, port);
> > for (i = 0; i < ARRAY_SIZE(intel_dp->aux_ch_data_reg); i++)
> > intel_dp->aux_ch_data_reg[i] = intel_aux_data_reg(dev_priv, port, i);
> > + if (INTEL_INFO(dev_priv)->gen >= 9)
> > + intel_dp->aux_ch_mutex_reg = DP_AUX_CH_MUTEX(port);
No point in adding this thing IMO.
> > }
> >
> > static void
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 898064e8bea7..0d235b7dab21 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1040,6 +1040,7 @@ struct intel_dp {
> > i915_reg_t output_reg;
> > i915_reg_t aux_ch_ctl_reg;
> > i915_reg_t aux_ch_data_reg[5];
> > + i915_reg_t aux_ch_mutex_reg;
> > uint32_t DP;
> > int link_rate;
> > uint8_t lane_count;
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> > index 2ef374f936b9..8b456e4e1b47 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -484,6 +484,9 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp,
> > EDP_PSR_DEBUG_MASK_HPD |
> > EDP_PSR_DEBUG_MASK_LPSP);
> > }
> > +
> > + if (intel_dp->aux_ch_mutex_reg.reg)
> > + I915_WRITE(intel_dp->aux_ch_mutex_reg,
> > DP_AUX_CH_MUTEX_ENABLE);
>
> My understanding is that set Enable is the actual get hw mutex,
> so I don't believe you want do do this here.
The register read is the 'get'. But I also think we should just set the
enable bit in the _get() function as well. I don't think there should be
any harm if we just leave the enable bit set permenently after that.
>
> Also you already check for psr.enable inside the aux transaction.
> setting there should be enough.
>
> > }
> >
> > /**
> > @@ -617,6 +620,9 @@ static void hsw_psr_disable(struct intel_dp *intel_dp,
> > else
> > WARN_ON(I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE);
> > }
> > +
> > + if (intel_dp->aux_ch_mutex_reg.reg)
> > + I915_WRITE(intel_dp->aux_ch_mutex_reg, ~DP_AUX_CH_MUTEX_ENABLE);
> > }
> >
> > /**
> > --
> > 2.16.1
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 5/5] drm/i915: Make use of unused variable in hsw_write_infoframe()
2018-02-14 13:37 ` Chris Wilson
@ 2018-02-14 14:29 ` Ville Syrjälä
0 siblings, 0 replies; 18+ messages in thread
From: Ville Syrjälä @ 2018-02-14 14:29 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
On Wed, Feb 14, 2018 at 01:37:22PM +0000, Chris Wilson wrote:
> Quoting Ville Syrjälä (2018-02-14 13:33:03)
> > On Tue, Feb 13, 2018 at 04:29:17PM -0800, José Roberto de Souza wrote:
> > > data_reg was not being used but it can be used to reduce the number of
> > > calls to hsw_dip_data_reg() and just increment the reg by the size of
> > > uint32.
> > >
> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > ---
> > > drivers/gpu/drm/i915/intel_hdmi.c | 12 ++++--------
> > > 1 file changed, 4 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> > > index f5d7bfb43006..ef258eac8ae8 100644
> > > --- a/drivers/gpu/drm/i915/intel_hdmi.c
> > > +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> > > @@ -393,15 +393,11 @@ static void hsw_write_infoframe(struct drm_encoder *encoder,
> > > I915_WRITE(ctl_reg, val);
> > >
> > > mmiowb();
>
> mmiowb() is just a compiler barrier on x86, and meaningless wrt to our
> uncached mmio. If you need a full mmio barrier, use POSTING_READ().
Yeah, someone should nuke all the mmiowb()s from the infoframe code.
I still don't understand what they were supposed to achieve.
--
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] 18+ messages in thread
* Re: [PATCH 1/5] drm/i915: add and enable DP AUX CH mutex
2018-02-14 9:47 ` Jani Nikula
@ 2018-02-14 16:21 ` Rodrigo Vivi
2018-02-14 16:37 ` Jani Nikula
0 siblings, 1 reply; 18+ messages in thread
From: Rodrigo Vivi @ 2018-02-14 16:21 UTC (permalink / raw)
To: Jani Nikula; +Cc: intel-gfx
On Wed, Feb 14, 2018 at 09:47:00AM +0000, Jani Nikula wrote:
> On Tue, 13 Feb 2018, José Roberto de Souza <jose.souza@intel.com> wrote:
> > When PSR/PSR2 is enabled hardware can do AUX transactions by it self, so
> > lets use the mutex register that is available in gen9+ to avoid concurrent
> > access by hardware and driver.
> >
> > Reference: https://01.org/sites/default/files/documentation/intel-gfx-prm-osrc-skl-vol12-display.pdf
> > Page 198 - AUX programming sequence
> >
> > As gen < 9 don't have the mutex PSR needs to be disabled, that is why the
> > 'else TODO' was added. This is also backed by spec:
> > "DDI A AUX channel transactions must not be sent while SRD is enabled.
> > SRD must be completely disabled before a DDI A AUX channel transaction can
> > be sent." BSpec: 7530
>
> We explicitly rejected the use of the hw mutex way back when Skylake was
> enabled. At that time the understanding was that the hw will try to do
> the arbitration itself. Has something changed?
By that time spec wasn't clear about the actual need or its reliability.
Nowdays spec is clear that that is absolutely needed for PSR/PSR2/GTC.
Also we were using a lot of SW tracking and nowadays we are moving PSR
to use more HW tracking.
So I believe we should add this to get a stable PSR.
>
> Some comments inline.
>
> >
> > Cc: Rodrigo Vivi <rodrigo.vivi@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 | 48 ++++++++++++++++++++++++++++++++++++++++
> > drivers/gpu/drm/i915/intel_drv.h | 1 +
> > drivers/gpu/drm/i915/intel_psr.c | 6 +++++
> > 4 files changed, 64 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index f6afa5e5e7c1..5d7736117cb9 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -5318,6 +5318,7 @@ enum {
> > #define _DPA_AUX_CH_DATA3 (dev_priv->info.display_mmio_offset + 0x6401c)
> > #define _DPA_AUX_CH_DATA4 (dev_priv->info.display_mmio_offset + 0x64020)
> > #define _DPA_AUX_CH_DATA5 (dev_priv->info.display_mmio_offset + 0x64024)
> > +#define _DPA_AUX_CH_MUTEX (dev_priv->info.display_mmio_offset + 0x6402C)
> >
> > #define _DPB_AUX_CH_CTL (dev_priv->info.display_mmio_offset + 0x64110)
> > #define _DPB_AUX_CH_DATA1 (dev_priv->info.display_mmio_offset + 0x64114)
> > @@ -5325,6 +5326,7 @@ enum {
> > #define _DPB_AUX_CH_DATA3 (dev_priv->info.display_mmio_offset + 0x6411c)
> > #define _DPB_AUX_CH_DATA4 (dev_priv->info.display_mmio_offset + 0x64120)
> > #define _DPB_AUX_CH_DATA5 (dev_priv->info.display_mmio_offset + 0x64124)
> > +#define _DPB_AUX_CH_MUTEX (dev_priv->info.display_mmio_offset + 0x6412C)
> >
> > #define _DPC_AUX_CH_CTL (dev_priv->info.display_mmio_offset + 0x64210)
> > #define _DPC_AUX_CH_DATA1 (dev_priv->info.display_mmio_offset + 0x64214)
> > @@ -5332,6 +5334,7 @@ enum {
> > #define _DPC_AUX_CH_DATA3 (dev_priv->info.display_mmio_offset + 0x6421c)
> > #define _DPC_AUX_CH_DATA4 (dev_priv->info.display_mmio_offset + 0x64220)
> > #define _DPC_AUX_CH_DATA5 (dev_priv->info.display_mmio_offset + 0x64224)
> > +#define _DPC_AUX_CH_MUTEX (dev_priv->info.display_mmio_offset + 0x6422C)
> >
> > #define _DPD_AUX_CH_CTL (dev_priv->info.display_mmio_offset + 0x64310)
> > #define _DPD_AUX_CH_DATA1 (dev_priv->info.display_mmio_offset + 0x64314)
> > @@ -5339,6 +5342,7 @@ enum {
> > #define _DPD_AUX_CH_DATA3 (dev_priv->info.display_mmio_offset + 0x6431c)
> > #define _DPD_AUX_CH_DATA4 (dev_priv->info.display_mmio_offset + 0x64320)
> > #define _DPD_AUX_CH_DATA5 (dev_priv->info.display_mmio_offset + 0x64324)
> > +#define _DPD_AUX_CH_MUTEX (dev_priv->info.display_mmio_offset + 0x6432C)
> >
> > #define _DPF_AUX_CH_CTL (dev_priv->info.display_mmio_offset + 0x64510)
> > #define _DPF_AUX_CH_DATA1 (dev_priv->info.display_mmio_offset + 0x64514)
> > @@ -5346,9 +5350,11 @@ enum {
> > #define _DPF_AUX_CH_DATA3 (dev_priv->info.display_mmio_offset + 0x6451c)
> > #define _DPF_AUX_CH_DATA4 (dev_priv->info.display_mmio_offset + 0x64520)
> > #define _DPF_AUX_CH_DATA5 (dev_priv->info.display_mmio_offset + 0x64524)
> > +#define _DPF_AUX_CH_MUTEX (dev_priv->info.display_mmio_offset + 0x6452C)
> >
> > #define DP_AUX_CH_CTL(port) _MMIO_PORT(port, _DPA_AUX_CH_CTL, _DPB_AUX_CH_CTL)
> > #define DP_AUX_CH_DATA(port, i) _MMIO(_PORT(port, _DPA_AUX_CH_DATA1, _DPB_AUX_CH_DATA1) + (i) * 4) /* 5 registers */
> > +#define DP_AUX_CH_MUTEX(port) _MMIO_PORT(port, _DPA_AUX_CH_MUTEX, _DPB_AUX_CH_MUTEX)
> >
> > #define DP_AUX_CH_CTL_SEND_BUSY (1 << 31)
> > #define DP_AUX_CH_CTL_DONE (1 << 30)
> > @@ -5378,6 +5384,9 @@ enum {
> > #define DP_AUX_CH_CTL_FW_SYNC_PULSE_SKL(c) (((c) - 1) << 5)
> > #define DP_AUX_CH_CTL_SYNC_PULSE_SKL(c) ((c) - 1)
> >
> > +#define DP_AUX_CH_MUTEX_ENABLE (1 << 31)
> > +#define DP_AUX_CH_MUTEX_STATUS (1 << 30)
>
> The bit definitions should immediately follow the register offset
> definition. I.e. DP_AUX_CH_MUTEX() should be right above these.
>
> > +
> > /*
> > * Computing GMCH M and N values for the Display Port link
> > *
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index f20b25f98e5a..bd5f1bb730ff 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -1081,6 +1081,28 @@ static uint32_t intel_dp_get_aux_send_ctl(struct intel_dp *intel_dp,
> > aux_clock_divider);
> > }
> >
> > +static bool intel_dp_aux_get(struct drm_i915_private *dev_priv,
> > + struct intel_dp *intel_dp)
>
> You can get at dev_priv from intel_dp, please don't pass both.
>
> The names _get/_put imply the calls can be nested, which is not the case
> here. Please use _lock/_unlock instead.
>
> If the locking can fail, call it _trylock.
>
> > +{
> > + int try, ret;
> > +
> > + for (try = 0, ret = -1; try < 3 && ret; try++)
> > + ret = wait_for(!(I915_READ_NOTRACE(intel_dp->aux_ch_mutex_reg)
> > + & DP_AUX_CH_MUTEX_STATUS), 0.5);
>
> 0.5? Really?
>
> What's the point of the loop around the wait_for? wait_for already loops
> with increasing delays in between the condition check.
>
> > +
> > + return ret == 0;
> > +}
> > +
> > +static void intel_dp_aux_put(struct drm_i915_private *dev_priv,
> > + struct intel_dp *intel_dp)
> > +{
> > + /* setting the status bit releases the mutex + keep set the bit
> > + * to keep the mutex enabled.
> > + */
> > + I915_WRITE(intel_dp->aux_ch_mutex_reg, DP_AUX_CH_MUTEX_ENABLE |
> > + DP_AUX_CH_MUTEX_STATUS);
> > +}
> > +
> > static int
> > intel_dp_aux_ch(struct intel_dp *intel_dp,
> > const uint8_t *send, int send_bytes,
> > @@ -1115,6 +1137,21 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
> >
> > intel_dp_check_edp(intel_dp);
> >
> > + /* Get hardware mutex when PSR is enabled */
> > + if (dev_priv->psr.enabled == intel_dp) {
> > + if (intel_dp->aux_ch_mutex_reg.reg) {
>
> See i915_mmio_reg_valid(). But I'd rather the check was within the
> locking primitives.
>
> Overall I think mixing all this PSR conditional stuff in the aux channel
> primitives is very, very wrong.
>
> At the very least this is racy with PSR enabling/disabling.
>
> > + if (intel_dp_aux_get(dev_priv, intel_dp)) {
> > + DRM_WARN("dp_aux_ch port locked for too long");
> > + ret = -EBUSY;
> > + goto out_locked;
> > + }
> > + } else {
> > + /* TODO: go to PSR exit state waiting for change before
> > + * do a aux ch transaction.
> > + */
> > + }
> > + }
> > +
> > /* Try to wait for any previous AUX channel activity */
> > for (try = 0; try < 3; try++) {
> > status = I915_READ_NOTRACE(ch_ctl);
> > @@ -1243,7 +1280,16 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
> > recv + i, recv_bytes - i);
> >
> > ret = recv_bytes;
> > +
> > out:
> > + if (dev_priv->psr.enabled == intel_dp) {
> > + if (intel_dp->aux_ch_mutex_reg.reg)
> > + intel_dp_aux_put(dev_priv, intel_dp);
> > + else
> > + /* TODO: schedule PSR work to active PSR again */
> > + }
> > +
> > +out_locked:
> > pm_qos_update_request(&dev_priv->pm_qos, PM_QOS_DEFAULT_VALUE);
> >
> > if (vdd)
> > @@ -1496,6 +1542,8 @@ static void intel_aux_reg_init(struct intel_dp *intel_dp)
> > intel_dp->aux_ch_ctl_reg = intel_aux_ctl_reg(dev_priv, port);
> > for (i = 0; i < ARRAY_SIZE(intel_dp->aux_ch_data_reg); i++)
> > intel_dp->aux_ch_data_reg[i] = intel_aux_data_reg(dev_priv, port, i);
> > + if (INTEL_INFO(dev_priv)->gen >= 9)
> > + intel_dp->aux_ch_mutex_reg = DP_AUX_CH_MUTEX(port);
> > }
> >
> > static void
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 898064e8bea7..0d235b7dab21 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1040,6 +1040,7 @@ struct intel_dp {
> > i915_reg_t output_reg;
> > i915_reg_t aux_ch_ctl_reg;
> > i915_reg_t aux_ch_data_reg[5];
> > + i915_reg_t aux_ch_mutex_reg;
> > uint32_t DP;
> > int link_rate;
> > uint8_t lane_count;
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> > index 2ef374f936b9..8b456e4e1b47 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -484,6 +484,9 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp,
> > EDP_PSR_DEBUG_MASK_HPD |
> > EDP_PSR_DEBUG_MASK_LPSP);
> > }
> > +
> > + if (intel_dp->aux_ch_mutex_reg.reg)
> > + I915_WRITE(intel_dp->aux_ch_mutex_reg, DP_AUX_CH_MUTEX_ENABLE);
>
> You can't just go stomping on the register without locking. Racy.
>
> > }
> >
> > /**
> > @@ -617,6 +620,9 @@ static void hsw_psr_disable(struct intel_dp *intel_dp,
> > else
> > WARN_ON(I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE);
> > }
> > +
> > + if (intel_dp->aux_ch_mutex_reg.reg)
> > + I915_WRITE(intel_dp->aux_ch_mutex_reg, ~DP_AUX_CH_MUTEX_ENABLE);
>
> Are you sure you want to set all other bits than enable?
>
>
> BR,
> Jani.
>
> > }
> >
> > /**
>
> --
> Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/5] drm/i915: add and enable DP AUX CH mutex
2018-02-14 16:21 ` Rodrigo Vivi
@ 2018-02-14 16:37 ` Jani Nikula
0 siblings, 0 replies; 18+ messages in thread
From: Jani Nikula @ 2018-02-14 16:37 UTC (permalink / raw)
To: Rodrigo Vivi; +Cc: intel-gfx
On Wed, 14 Feb 2018, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> On Wed, Feb 14, 2018 at 09:47:00AM +0000, Jani Nikula wrote:
>> On Tue, 13 Feb 2018, José Roberto de Souza <jose.souza@intel.com> wrote:
>> > When PSR/PSR2 is enabled hardware can do AUX transactions by it self, so
>> > lets use the mutex register that is available in gen9+ to avoid concurrent
>> > access by hardware and driver.
>> >
>> > Reference: https://01.org/sites/default/files/documentation/intel-gfx-prm-osrc-skl-vol12-display.pdf
>> > Page 198 - AUX programming sequence
>> >
>> > As gen < 9 don't have the mutex PSR needs to be disabled, that is why the
>> > 'else TODO' was added. This is also backed by spec:
>> > "DDI A AUX channel transactions must not be sent while SRD is enabled.
>> > SRD must be completely disabled before a DDI A AUX channel transaction can
>> > be sent." BSpec: 7530
>>
>> We explicitly rejected the use of the hw mutex way back when Skylake was
>> enabled. At that time the understanding was that the hw will try to do
>> the arbitration itself. Has something changed?
>
> By that time spec wasn't clear about the actual need or its reliability.
> Nowdays spec is clear that that is absolutely needed for PSR/PSR2/GTC.
>
> Also we were using a lot of SW tracking and nowadays we are moving PSR
> to use more HW tracking.
>
> So I believe we should add this to get a stable PSR.
Fair enough. The patch at hand needs a bunch of work though.
BR,
Jani.
>
>>
>> Some comments inline.
>>
>> >
>> > Cc: Rodrigo Vivi <rodrigo.vivi@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 | 48 ++++++++++++++++++++++++++++++++++++++++
>> > drivers/gpu/drm/i915/intel_drv.h | 1 +
>> > drivers/gpu/drm/i915/intel_psr.c | 6 +++++
>> > 4 files changed, 64 insertions(+)
>> >
>> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> > index f6afa5e5e7c1..5d7736117cb9 100644
>> > --- a/drivers/gpu/drm/i915/i915_reg.h
>> > +++ b/drivers/gpu/drm/i915/i915_reg.h
>> > @@ -5318,6 +5318,7 @@ enum {
>> > #define _DPA_AUX_CH_DATA3 (dev_priv->info.display_mmio_offset + 0x6401c)
>> > #define _DPA_AUX_CH_DATA4 (dev_priv->info.display_mmio_offset + 0x64020)
>> > #define _DPA_AUX_CH_DATA5 (dev_priv->info.display_mmio_offset + 0x64024)
>> > +#define _DPA_AUX_CH_MUTEX (dev_priv->info.display_mmio_offset + 0x6402C)
>> >
>> > #define _DPB_AUX_CH_CTL (dev_priv->info.display_mmio_offset + 0x64110)
>> > #define _DPB_AUX_CH_DATA1 (dev_priv->info.display_mmio_offset + 0x64114)
>> > @@ -5325,6 +5326,7 @@ enum {
>> > #define _DPB_AUX_CH_DATA3 (dev_priv->info.display_mmio_offset + 0x6411c)
>> > #define _DPB_AUX_CH_DATA4 (dev_priv->info.display_mmio_offset + 0x64120)
>> > #define _DPB_AUX_CH_DATA5 (dev_priv->info.display_mmio_offset + 0x64124)
>> > +#define _DPB_AUX_CH_MUTEX (dev_priv->info.display_mmio_offset + 0x6412C)
>> >
>> > #define _DPC_AUX_CH_CTL (dev_priv->info.display_mmio_offset + 0x64210)
>> > #define _DPC_AUX_CH_DATA1 (dev_priv->info.display_mmio_offset + 0x64214)
>> > @@ -5332,6 +5334,7 @@ enum {
>> > #define _DPC_AUX_CH_DATA3 (dev_priv->info.display_mmio_offset + 0x6421c)
>> > #define _DPC_AUX_CH_DATA4 (dev_priv->info.display_mmio_offset + 0x64220)
>> > #define _DPC_AUX_CH_DATA5 (dev_priv->info.display_mmio_offset + 0x64224)
>> > +#define _DPC_AUX_CH_MUTEX (dev_priv->info.display_mmio_offset + 0x6422C)
>> >
>> > #define _DPD_AUX_CH_CTL (dev_priv->info.display_mmio_offset + 0x64310)
>> > #define _DPD_AUX_CH_DATA1 (dev_priv->info.display_mmio_offset + 0x64314)
>> > @@ -5339,6 +5342,7 @@ enum {
>> > #define _DPD_AUX_CH_DATA3 (dev_priv->info.display_mmio_offset + 0x6431c)
>> > #define _DPD_AUX_CH_DATA4 (dev_priv->info.display_mmio_offset + 0x64320)
>> > #define _DPD_AUX_CH_DATA5 (dev_priv->info.display_mmio_offset + 0x64324)
>> > +#define _DPD_AUX_CH_MUTEX (dev_priv->info.display_mmio_offset + 0x6432C)
>> >
>> > #define _DPF_AUX_CH_CTL (dev_priv->info.display_mmio_offset + 0x64510)
>> > #define _DPF_AUX_CH_DATA1 (dev_priv->info.display_mmio_offset + 0x64514)
>> > @@ -5346,9 +5350,11 @@ enum {
>> > #define _DPF_AUX_CH_DATA3 (dev_priv->info.display_mmio_offset + 0x6451c)
>> > #define _DPF_AUX_CH_DATA4 (dev_priv->info.display_mmio_offset + 0x64520)
>> > #define _DPF_AUX_CH_DATA5 (dev_priv->info.display_mmio_offset + 0x64524)
>> > +#define _DPF_AUX_CH_MUTEX (dev_priv->info.display_mmio_offset + 0x6452C)
>> >
>> > #define DP_AUX_CH_CTL(port) _MMIO_PORT(port, _DPA_AUX_CH_CTL, _DPB_AUX_CH_CTL)
>> > #define DP_AUX_CH_DATA(port, i) _MMIO(_PORT(port, _DPA_AUX_CH_DATA1, _DPB_AUX_CH_DATA1) + (i) * 4) /* 5 registers */
>> > +#define DP_AUX_CH_MUTEX(port) _MMIO_PORT(port, _DPA_AUX_CH_MUTEX, _DPB_AUX_CH_MUTEX)
>> >
>> > #define DP_AUX_CH_CTL_SEND_BUSY (1 << 31)
>> > #define DP_AUX_CH_CTL_DONE (1 << 30)
>> > @@ -5378,6 +5384,9 @@ enum {
>> > #define DP_AUX_CH_CTL_FW_SYNC_PULSE_SKL(c) (((c) - 1) << 5)
>> > #define DP_AUX_CH_CTL_SYNC_PULSE_SKL(c) ((c) - 1)
>> >
>> > +#define DP_AUX_CH_MUTEX_ENABLE (1 << 31)
>> > +#define DP_AUX_CH_MUTEX_STATUS (1 << 30)
>>
>> The bit definitions should immediately follow the register offset
>> definition. I.e. DP_AUX_CH_MUTEX() should be right above these.
>>
>> > +
>> > /*
>> > * Computing GMCH M and N values for the Display Port link
>> > *
>> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> > index f20b25f98e5a..bd5f1bb730ff 100644
>> > --- a/drivers/gpu/drm/i915/intel_dp.c
>> > +++ b/drivers/gpu/drm/i915/intel_dp.c
>> > @@ -1081,6 +1081,28 @@ static uint32_t intel_dp_get_aux_send_ctl(struct intel_dp *intel_dp,
>> > aux_clock_divider);
>> > }
>> >
>> > +static bool intel_dp_aux_get(struct drm_i915_private *dev_priv,
>> > + struct intel_dp *intel_dp)
>>
>> You can get at dev_priv from intel_dp, please don't pass both.
>>
>> The names _get/_put imply the calls can be nested, which is not the case
>> here. Please use _lock/_unlock instead.
>>
>> If the locking can fail, call it _trylock.
>>
>> > +{
>> > + int try, ret;
>> > +
>> > + for (try = 0, ret = -1; try < 3 && ret; try++)
>> > + ret = wait_for(!(I915_READ_NOTRACE(intel_dp->aux_ch_mutex_reg)
>> > + & DP_AUX_CH_MUTEX_STATUS), 0.5);
>>
>> 0.5? Really?
>>
>> What's the point of the loop around the wait_for? wait_for already loops
>> with increasing delays in between the condition check.
>>
>> > +
>> > + return ret == 0;
>> > +}
>> > +
>> > +static void intel_dp_aux_put(struct drm_i915_private *dev_priv,
>> > + struct intel_dp *intel_dp)
>> > +{
>> > + /* setting the status bit releases the mutex + keep set the bit
>> > + * to keep the mutex enabled.
>> > + */
>> > + I915_WRITE(intel_dp->aux_ch_mutex_reg, DP_AUX_CH_MUTEX_ENABLE |
>> > + DP_AUX_CH_MUTEX_STATUS);
>> > +}
>> > +
>> > static int
>> > intel_dp_aux_ch(struct intel_dp *intel_dp,
>> > const uint8_t *send, int send_bytes,
>> > @@ -1115,6 +1137,21 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
>> >
>> > intel_dp_check_edp(intel_dp);
>> >
>> > + /* Get hardware mutex when PSR is enabled */
>> > + if (dev_priv->psr.enabled == intel_dp) {
>> > + if (intel_dp->aux_ch_mutex_reg.reg) {
>>
>> See i915_mmio_reg_valid(). But I'd rather the check was within the
>> locking primitives.
>>
>> Overall I think mixing all this PSR conditional stuff in the aux channel
>> primitives is very, very wrong.
>>
>> At the very least this is racy with PSR enabling/disabling.
>>
>> > + if (intel_dp_aux_get(dev_priv, intel_dp)) {
>> > + DRM_WARN("dp_aux_ch port locked for too long");
>> > + ret = -EBUSY;
>> > + goto out_locked;
>> > + }
>> > + } else {
>> > + /* TODO: go to PSR exit state waiting for change before
>> > + * do a aux ch transaction.
>> > + */
>> > + }
>> > + }
>> > +
>> > /* Try to wait for any previous AUX channel activity */
>> > for (try = 0; try < 3; try++) {
>> > status = I915_READ_NOTRACE(ch_ctl);
>> > @@ -1243,7 +1280,16 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
>> > recv + i, recv_bytes - i);
>> >
>> > ret = recv_bytes;
>> > +
>> > out:
>> > + if (dev_priv->psr.enabled == intel_dp) {
>> > + if (intel_dp->aux_ch_mutex_reg.reg)
>> > + intel_dp_aux_put(dev_priv, intel_dp);
>> > + else
>> > + /* TODO: schedule PSR work to active PSR again */
>> > + }
>> > +
>> > +out_locked:
>> > pm_qos_update_request(&dev_priv->pm_qos, PM_QOS_DEFAULT_VALUE);
>> >
>> > if (vdd)
>> > @@ -1496,6 +1542,8 @@ static void intel_aux_reg_init(struct intel_dp *intel_dp)
>> > intel_dp->aux_ch_ctl_reg = intel_aux_ctl_reg(dev_priv, port);
>> > for (i = 0; i < ARRAY_SIZE(intel_dp->aux_ch_data_reg); i++)
>> > intel_dp->aux_ch_data_reg[i] = intel_aux_data_reg(dev_priv, port, i);
>> > + if (INTEL_INFO(dev_priv)->gen >= 9)
>> > + intel_dp->aux_ch_mutex_reg = DP_AUX_CH_MUTEX(port);
>> > }
>> >
>> > static void
>> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> > index 898064e8bea7..0d235b7dab21 100644
>> > --- a/drivers/gpu/drm/i915/intel_drv.h
>> > +++ b/drivers/gpu/drm/i915/intel_drv.h
>> > @@ -1040,6 +1040,7 @@ struct intel_dp {
>> > i915_reg_t output_reg;
>> > i915_reg_t aux_ch_ctl_reg;
>> > i915_reg_t aux_ch_data_reg[5];
>> > + i915_reg_t aux_ch_mutex_reg;
>> > uint32_t DP;
>> > int link_rate;
>> > uint8_t lane_count;
>> > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
>> > index 2ef374f936b9..8b456e4e1b47 100644
>> > --- a/drivers/gpu/drm/i915/intel_psr.c
>> > +++ b/drivers/gpu/drm/i915/intel_psr.c
>> > @@ -484,6 +484,9 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp,
>> > EDP_PSR_DEBUG_MASK_HPD |
>> > EDP_PSR_DEBUG_MASK_LPSP);
>> > }
>> > +
>> > + if (intel_dp->aux_ch_mutex_reg.reg)
>> > + I915_WRITE(intel_dp->aux_ch_mutex_reg, DP_AUX_CH_MUTEX_ENABLE);
>>
>> You can't just go stomping on the register without locking. Racy.
>>
>> > }
>> >
>> > /**
>> > @@ -617,6 +620,9 @@ static void hsw_psr_disable(struct intel_dp *intel_dp,
>> > else
>> > WARN_ON(I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE);
>> > }
>> > +
>> > + if (intel_dp->aux_ch_mutex_reg.reg)
>> > + I915_WRITE(intel_dp->aux_ch_mutex_reg, ~DP_AUX_CH_MUTEX_ENABLE);
>>
>> Are you sure you want to set all other bits than enable?
>>
>>
>> BR,
>> Jani.
>>
>> > }
>> >
>> > /**
>>
>> --
>> Jani Nikula, Intel Open Source Technology Center
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 5/5] drm/i915: Make use of unused variable in hsw_write_infoframe()
2018-02-14 13:33 ` Ville Syrjälä
2018-02-14 13:37 ` Chris Wilson
@ 2018-02-16 8:41 ` Jani Nikula
1 sibling, 0 replies; 18+ messages in thread
From: Jani Nikula @ 2018-02-16 8:41 UTC (permalink / raw)
To: Ville Syrjälä, José Roberto de Souza; +Cc: intel-gfx
On Wed, 14 Feb 2018, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Tue, Feb 13, 2018 at 04:29:17PM -0800, José Roberto de Souza wrote:
>> data_reg was not being used but it can be used to reduce the number of
>> calls to hsw_dip_data_reg() and just increment the reg by the size of
>> uint32.
>>
>> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
>> ---
>> drivers/gpu/drm/i915/intel_hdmi.c | 12 ++++--------
>> 1 file changed, 4 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
>> index f5d7bfb43006..ef258eac8ae8 100644
>> --- a/drivers/gpu/drm/i915/intel_hdmi.c
>> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
>> @@ -393,15 +393,11 @@ static void hsw_write_infoframe(struct drm_encoder *encoder,
>> I915_WRITE(ctl_reg, val);
>>
>> mmiowb();
>> - for (i = 0; i < len; i += 4) {
>> - I915_WRITE(hsw_dip_data_reg(dev_priv, cpu_transcoder,
>> - type, i >> 2), *data);a
>
> i>>2 != 0
>
> just kill 'data_reg'
What Ville says.
Furthermore, you're *not* supposed to mess with the contents of
i915_reg_t. That we use a typedef to begin with means "don't touch the
guts of this one".
BR,
Jani.
>
>> - data++;
>> - }
>> + for (i = 0; i < len; i += 4, data++, data_reg.reg += 4)
>> + I915_WRITE(data_reg, *data);
>> /* Write every possible data byte to force correct ECC calculation. */
>> - for (; i < data_size; i += 4)
>> - I915_WRITE(hsw_dip_data_reg(dev_priv, cpu_transcoder,
>> - type, i >> 2), 0);
>> + for (; i < data_size; i += 4, data_reg.reg += 4)
>> + I915_WRITE(data_reg, 0);
>> mmiowb();
>>
>> val |= hsw_infoframe_enable(type);
>> --
>> 2.16.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2018-02-16 8:41 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-02-14 0:29 [PATCH 1/5] drm/i915: add and enable DP AUX CH mutex José Roberto de Souza
2018-02-14 0:29 ` [PATCH 2/5] drm/i915: keep AUX powered while PSR is enabled José Roberto de Souza
2018-02-14 1:19 ` Rodrigo Vivi
2018-02-14 0:29 ` [PATCH 3/5] drm/i915: Share PSR and PSR2 VSC setup José Roberto de Souza
2018-02-14 0:29 ` [PATCH 4/5] drm/i915: Replace magic number with macro defined by drm José Roberto de Souza
2018-02-14 0:29 ` [PATCH 5/5] drm/i915: Make use of unused variable in hsw_write_infoframe() José Roberto de Souza
2018-02-14 13:33 ` Ville Syrjälä
2018-02-14 13:37 ` Chris Wilson
2018-02-14 14:29 ` Ville Syrjälä
2018-02-16 8:41 ` Jani Nikula
2018-02-14 0:36 ` ✗ Fi.CI.BAT: failure for series starting with [1/5] drm/i915: add and enable DP AUX CH mutex Patchwork
2018-02-14 1:30 ` [PATCH 1/5] " Rodrigo Vivi
2018-02-14 2:30 ` Souza, Jose
2018-02-14 13:43 ` Ville Syrjälä
2018-02-14 7:32 ` Jani Nikula
2018-02-14 9:47 ` Jani Nikula
2018-02-14 16:21 ` Rodrigo Vivi
2018-02-14 16:37 ` Jani Nikula
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.