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

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

[José Roberto de Souza]

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

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

Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---

Changelog:
v2
- removed the PSR dependency, now getting lock all the times when available
- renamed functions to avoid nested calls
- moved register bits right after the DP_AUX_CH_MUTEX()
- removed 'drm/i915: keep AUX powered while PSR is enabled' Dhinakaran Pandiyan will sent a better and final version
v3
- rebased on top of Ville's AUX series
- moved port registers to above DP_AUX_CH_MUTEX()
- using intel_wait_for_register() instead of the internal version
v4
- removed virtual function to get mutex register address
- enabling the mutex back only on aux channel init
- added the aux channel name to the timeout debug message
v5
- renamed DP_AUX_CH_MUTEX() parameter to aux_ch
- renamed goto label when intel_dp_aux_ch_trylock() fails

 drivers/gpu/drm/i915/i915_reg.h |  9 ++++++++
 drivers/gpu/drm/i915/intel_dp.c | 47 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 56 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index eea5b2c537d4..bce2e6dad4c4 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -5385,6 +5385,15 @@ enum {
 #define   DP_AUX_CH_CTL_FW_SYNC_PULSE_SKL(c) (((c) - 1) << 5)
 #define   DP_AUX_CH_CTL_SYNC_PULSE_SKL(c)   ((c) - 1)
 
+#define _DPA_AUX_CH_MUTEX	(dev_priv->info.display_mmio_offset + 0x6402C)
+#define _DPB_AUX_CH_MUTEX	(dev_priv->info.display_mmio_offset + 0x6412C)
+#define _DPC_AUX_CH_MUTEX	(dev_priv->info.display_mmio_offset + 0x6422C)
+#define _DPD_AUX_CH_MUTEX	(dev_priv->info.display_mmio_offset + 0x6432C)
+#define _DPF_AUX_CH_MUTEX	(dev_priv->info.display_mmio_offset + 0x6452C)
+#define DP_AUX_CH_MUTEX(aux_ch)	_MMIO_PORT(aux_ch, _DPA_AUX_CH_MUTEX, _DPB_AUX_CH_MUTEX)
+#define   DP_AUX_CH_MUTEX_ENABLE		(1 << 31)
+#define   DP_AUX_CH_MUTEX_STATUS		(1 << 30)
+
 /*
  * Computing GMCH M and N values for the Display Port link
  *
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 2a3b3ae4e3da..7f4bf77227cd 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1081,6 +1081,42 @@ static uint32_t intel_dp_get_aux_send_ctl(struct intel_dp *intel_dp,
 						aux_clock_divider);
 }
 
+static bool intel_dp_aux_ch_trylock(struct intel_dp *intel_dp)
+{
+	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
+	struct drm_i915_private *dev_priv =
+			to_i915(intel_dig_port->base.base.dev);
+
+	if (INTEL_GEN(dev_priv) < 9)
+		return true;
+
+	/* Spec says that mutex is acquired when status bit is read as unset,
+	 * here waiting for 2msec(+-4 aux transactions) before give up.
+	 */
+	if (intel_wait_for_register(dev_priv, DP_AUX_CH_MUTEX(intel_dp->aux_ch),
+				    DP_AUX_CH_MUTEX_STATUS, 0, 2)) {
+		DRM_DEBUG_KMS("aux channel %c locked for 2msec, timing out\n",
+			      aux_ch_name(intel_dp->aux_ch));
+		return false;
+	}
+
+	return true;
+}
+
+static void intel_dp_aux_ch_unlock(struct intel_dp *intel_dp)
+{
+	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
+	struct drm_i915_private *dev_priv =
+			to_i915(intel_dig_port->base.base.dev);
+
+	if (INTEL_GEN(dev_priv) < 9)
+		return;
+
+	/* set the status bit releases the mutex + keeping mutex enabled */
+	I915_WRITE(DP_AUX_CH_MUTEX(intel_dp->aux_ch),
+		   DP_AUX_CH_MUTEX_ENABLE | DP_AUX_CH_MUTEX_STATUS);
+}
+
 static int
 intel_dp_aux_ch(struct intel_dp *intel_dp,
 		const uint8_t *send, int send_bytes,
@@ -1119,6 +1155,11 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
 
 	intel_dp_check_edp(intel_dp);
 
+	if (!intel_dp_aux_ch_trylock(intel_dp)) {
+		ret = -EBUSY;
+		goto out_aux_ch_unlocked;
+	}
+
 	/* Try to wait for any previous AUX channel activity */
 	for (try = 0; try < 3; try++) {
 		status = I915_READ_NOTRACE(ch_ctl);
@@ -1240,6 +1281,8 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
 
 	ret = recv_bytes;
 out:
+	intel_dp_aux_ch_unlock(intel_dp);
+out_aux_ch_unlocked:
 	pm_qos_update_request(&dev_priv->pm_qos, PM_QOS_DEFAULT_VALUE);
 
 	if (vdd)
@@ -1536,6 +1579,10 @@ intel_dp_aux_init(struct intel_dp *intel_dp)
 	else
 		intel_dp->get_aux_send_ctl = g4x_get_aux_send_ctl;
 
+	if (INTEL_GEN(dev_priv) >= 9)
+		I915_WRITE(DP_AUX_CH_MUTEX(intel_dp->aux_ch),
+			   DP_AUX_CH_MUTEX_ENABLE);
+
 	drm_dp_aux_init(&intel_dp->aux);
 
 	/* Failure to allocate our preferred name is not critical */
-- 
2.16.2

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

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

[Ville Syrjälä]

On Tue, Feb 27, 2018 at 01:23:59PM -0800, José Roberto de Souza wrote:
> When PSR/PSR2/GTC is enabled hardware can do AUX transactions by it
> self, so lets use the mutex register that is available in gen9+ to
> avoid concurrent access by hardware and driver.
> Older gen handling will be done separated.
> 
> Reference: https://01.org/sites/default/files/documentation/intel-gfx-prm-osrc-skl-vol12-display.pdf
> Page 198 - AUX programming sequence
> 
> Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
> 
> Changelog:
> v2
> - removed the PSR dependency, now getting lock all the times when available
> - renamed functions to avoid nested calls
> - moved register bits right after the DP_AUX_CH_MUTEX()
> - removed 'drm/i915: keep AUX powered while PSR is enabled' Dhinakaran Pandiyan will sent a better and final version
> v3
> - rebased on top of Ville's AUX series
> - moved port registers to above DP_AUX_CH_MUTEX()
> - using intel_wait_for_register() instead of the internal version
> v4
> - removed virtual function to get mutex register address
> - enabling the mutex back only on aux channel init
> - added the aux channel name to the timeout debug message
> v5
> - renamed DP_AUX_CH_MUTEX() parameter to aux_ch
> - renamed goto label when intel_dp_aux_ch_trylock() fails
> 
>  drivers/gpu/drm/i915/i915_reg.h |  9 ++++++++
>  drivers/gpu/drm/i915/intel_dp.c | 47 +++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 56 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index eea5b2c537d4..bce2e6dad4c4 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -5385,6 +5385,15 @@ enum {
>  #define   DP_AUX_CH_CTL_FW_SYNC_PULSE_SKL(c) (((c) - 1) << 5)
>  #define   DP_AUX_CH_CTL_SYNC_PULSE_SKL(c)   ((c) - 1)
>  
> +#define _DPA_AUX_CH_MUTEX	(dev_priv->info.display_mmio_offset + 0x6402C)
> +#define _DPB_AUX_CH_MUTEX	(dev_priv->info.display_mmio_offset + 0x6412C)
> +#define _DPC_AUX_CH_MUTEX	(dev_priv->info.display_mmio_offset + 0x6422C)
> +#define _DPD_AUX_CH_MUTEX	(dev_priv->info.display_mmio_offset + 0x6432C)
> +#define _DPF_AUX_CH_MUTEX	(dev_priv->info.display_mmio_offset + 0x6452C)
> +#define DP_AUX_CH_MUTEX(aux_ch)	_MMIO_PORT(aux_ch, _DPA_AUX_CH_MUTEX, _DPB_AUX_CH_MUTEX)
> +#define   DP_AUX_CH_MUTEX_ENABLE		(1 << 31)
> +#define   DP_AUX_CH_MUTEX_STATUS		(1 << 30)
> +
>  /*
>   * Computing GMCH M and N values for the Display Port link
>   *
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 2a3b3ae4e3da..7f4bf77227cd 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1081,6 +1081,42 @@ static uint32_t intel_dp_get_aux_send_ctl(struct intel_dp *intel_dp,
>  						aux_clock_divider);
>  }
>  
> +static bool intel_dp_aux_ch_trylock(struct intel_dp *intel_dp)
> +{
> +	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> +	struct drm_i915_private *dev_priv =
> +			to_i915(intel_dig_port->base.base.dev);
> +
> +	if (INTEL_GEN(dev_priv) < 9)
> +		return true;
> +
> +	/* Spec says that mutex is acquired when status bit is read as unset,
> +	 * here waiting for 2msec(+-4 aux transactions) before give up.
> +	 */
> +	if (intel_wait_for_register(dev_priv, DP_AUX_CH_MUTEX(intel_dp->aux_ch),
> +				    DP_AUX_CH_MUTEX_STATUS, 0, 2)) {
> +		DRM_DEBUG_KMS("aux channel %c locked for 2msec, timing out\n",
> +			      aux_ch_name(intel_dp->aux_ch));
> +		return false;
> +	}
> +
> +	return true;
> +}
> +
> +static void intel_dp_aux_ch_unlock(struct intel_dp *intel_dp)
> +{
> +	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> +	struct drm_i915_private *dev_priv =
> +			to_i915(intel_dig_port->base.base.dev);
> +
> +	if (INTEL_GEN(dev_priv) < 9)
> +		return;
> +
> +	/* set the status bit releases the mutex + keeping mutex enabled */
> +	I915_WRITE(DP_AUX_CH_MUTEX(intel_dp->aux_ch),
> +		   DP_AUX_CH_MUTEX_ENABLE | DP_AUX_CH_MUTEX_STATUS);
> +}
> +
>  static int
>  intel_dp_aux_ch(struct intel_dp *intel_dp,
>  		const uint8_t *send, int send_bytes,
> @@ -1119,6 +1155,11 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
>  
>  	intel_dp_check_edp(intel_dp);
>  
> +	if (!intel_dp_aux_ch_trylock(intel_dp)) {
> +		ret = -EBUSY;
> +		goto out_aux_ch_unlocked;
> +	}
> +
>  	/* Try to wait for any previous AUX channel activity */
>  	for (try = 0; try < 3; try++) {
>  		status = I915_READ_NOTRACE(ch_ctl);
> @@ -1240,6 +1281,8 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
>  
>  	ret = recv_bytes;
>  out:
> +	intel_dp_aux_ch_unlock(intel_dp);
> +out_aux_ch_unlocked:
>  	pm_qos_update_request(&dev_priv->pm_qos, PM_QOS_DEFAULT_VALUE);
>  
>  	if (vdd)
> @@ -1536,6 +1579,10 @@ intel_dp_aux_init(struct intel_dp *intel_dp)
>  	else
>  		intel_dp->get_aux_send_ctl = g4x_get_aux_send_ctl;
>  
> +	if (INTEL_GEN(dev_priv) >= 9)
> +		I915_WRITE(DP_AUX_CH_MUTEX(intel_dp->aux_ch),
> +			   DP_AUX_CH_MUTEX_ENABLE);

And who enables it after system/runtime PM etc.? Not sure how much sense
there is to finesse this anyway. One extra mmio write every time we try
to acquire the mutex shouldn't kill anyone.

> +
>  	drm_dp_aux_init(&intel_dp->aux);
>  
>  	/* Failure to allocate our preferred name is not critical */
> -- 
> 2.16.2

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

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

[Pandiyan, Dhinakaran]

On Tue, 2018-02-27 at 23:34 +0200, Ville Syrjälä wrote:
> On Tue, Feb 27, 2018 at 01:23:59PM -0800, José Roberto de Souza wrote:
> > When PSR/PSR2/GTC is enabled hardware can do AUX transactions by it
> > self, so lets use the mutex register that is available in gen9+ to
> > avoid concurrent access by hardware and driver.
> > Older gen handling will be done separated.
> > 
> > Reference: https://01.org/sites/default/files/documentation/intel-gfx-prm-osrc-skl-vol12-display.pdf
> > Page 198 - AUX programming sequence
> > 
> > Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> > 
> > Changelog:
> > v2
> > - removed the PSR dependency, now getting lock all the times when available
> > - renamed functions to avoid nested calls
> > - moved register bits right after the DP_AUX_CH_MUTEX()
> > - removed 'drm/i915: keep AUX powered while PSR is enabled' Dhinakaran Pandiyan will sent a better and final version
> > v3
> > - rebased on top of Ville's AUX series
> > - moved port registers to above DP_AUX_CH_MUTEX()
> > - using intel_wait_for_register() instead of the internal version
> > v4
> > - removed virtual function to get mutex register address
> > - enabling the mutex back only on aux channel init
> > - added the aux channel name to the timeout debug message
> > v5
> > - renamed DP_AUX_CH_MUTEX() parameter to aux_ch
> > - renamed goto label when intel_dp_aux_ch_trylock() fails
> > 
> >  drivers/gpu/drm/i915/i915_reg.h |  9 ++++++++
> >  drivers/gpu/drm/i915/intel_dp.c | 47 +++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 56 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index eea5b2c537d4..bce2e6dad4c4 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -5385,6 +5385,15 @@ enum {
> >  #define   DP_AUX_CH_CTL_FW_SYNC_PULSE_SKL(c) (((c) - 1) << 5)
> >  #define   DP_AUX_CH_CTL_SYNC_PULSE_SKL(c)   ((c) - 1)
> >  
> > +#define _DPA_AUX_CH_MUTEX	(dev_priv->info.display_mmio_offset + 0x6402C)
> > +#define _DPB_AUX_CH_MUTEX	(dev_priv->info.display_mmio_offset + 0x6412C)
> > +#define _DPC_AUX_CH_MUTEX	(dev_priv->info.display_mmio_offset + 0x6422C)
> > +#define _DPD_AUX_CH_MUTEX	(dev_priv->info.display_mmio_offset + 0x6432C)
> > +#define _DPF_AUX_CH_MUTEX	(dev_priv->info.display_mmio_offset + 0x6452C)
> > +#define DP_AUX_CH_MUTEX(aux_ch)	_MMIO_PORT(aux_ch, _DPA_AUX_CH_MUTEX, _DPB_AUX_CH_MUTEX)
> > +#define   DP_AUX_CH_MUTEX_ENABLE		(1 << 31)
> > +#define   DP_AUX_CH_MUTEX_STATUS		(1 << 30)
> > +
> >  /*
> >   * Computing GMCH M and N values for the Display Port link
> >   *
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index 2a3b3ae4e3da..7f4bf77227cd 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -1081,6 +1081,42 @@ static uint32_t intel_dp_get_aux_send_ctl(struct intel_dp *intel_dp,
> >  						aux_clock_divider);
> >  }
> >  
> > +static bool intel_dp_aux_ch_trylock(struct intel_dp *intel_dp)
> > +{
> > +	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> > +	struct drm_i915_private *dev_priv =
> > +			to_i915(intel_dig_port->base.base.dev);
> > +
> > +	if (INTEL_GEN(dev_priv) < 9)
> > +		return true;
> > +
> > +	/* Spec says that mutex is acquired when status bit is read as unset,
> > +	 * here waiting for 2msec(+-4 aux transactions) before give up.
> > +	 */
> > +	if (intel_wait_for_register(dev_priv, DP_AUX_CH_MUTEX(intel_dp->aux_ch),
> > +				    DP_AUX_CH_MUTEX_STATUS, 0, 2)) {
> > +		DRM_DEBUG_KMS("aux channel %c locked for 2msec, timing out\n",
> > +			      aux_ch_name(intel_dp->aux_ch));
> > +		return false;
> > +	}
> > +
> > +	return true;
> > +}
> > +
> > +static void intel_dp_aux_ch_unlock(struct intel_dp *intel_dp)
> > +{
> > +	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> > +	struct drm_i915_private *dev_priv =
> > +			to_i915(intel_dig_port->base.base.dev);
> > +
> > +	if (INTEL_GEN(dev_priv) < 9)
> > +		return;
> > +
> > +	/* set the status bit releases the mutex + keeping mutex enabled */
> > +	I915_WRITE(DP_AUX_CH_MUTEX(intel_dp->aux_ch),
> > +		   DP_AUX_CH_MUTEX_ENABLE | DP_AUX_CH_MUTEX_STATUS);
> > +}
> > +
> >  static int
> >  intel_dp_aux_ch(struct intel_dp *intel_dp,
> >  		const uint8_t *send, int send_bytes,
> > @@ -1119,6 +1155,11 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
> >  
> >  	intel_dp_check_edp(intel_dp);
> >  
> > +	if (!intel_dp_aux_ch_trylock(intel_dp)) {
> > +		ret = -EBUSY;
> > +		goto out_aux_ch_unlocked;
> > +	}
> > +
> >  	/* Try to wait for any previous AUX channel activity */
> >  	for (try = 0; try < 3; try++) {
> >  		status = I915_READ_NOTRACE(ch_ctl);
> > @@ -1240,6 +1281,8 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
> >  
> >  	ret = recv_bytes;
> >  out:
> > +	intel_dp_aux_ch_unlock(intel_dp);
> > +out_aux_ch_unlocked:
> >  	pm_qos_update_request(&dev_priv->pm_qos, PM_QOS_DEFAULT_VALUE);
> >  
> >  	if (vdd)
> > @@ -1536,6 +1579,10 @@ intel_dp_aux_init(struct intel_dp *intel_dp)
> >  	else
> >  		intel_dp->get_aux_send_ctl = g4x_get_aux_send_ctl;
> >  
> > +	if (INTEL_GEN(dev_priv) >= 9)
> > +		I915_WRITE(DP_AUX_CH_MUTEX(intel_dp->aux_ch),
> > +			   DP_AUX_CH_MUTEX_ENABLE);
> 
> And who enables it after system/runtime PM etc.? Not sure how much sense
> there is to finesse this anyway. One extra mmio write every time we try
> to acquire the mutex shouldn't kill anyone.
> 

The problem was with enabling the mutex without modifying the status
bit.

The earlier version 
> +     ch_mutex = intel_dp->aux_ch_mutex_reg(intel_dp);
> +     I915_WRITE(ch_mutex, DP_AUX_CH_MUTEX_ENABLE)

had this in _trylock()

This overwrites the status bit.


> > +
> >  	drm_dp_aux_init(&intel_dp->aux);
> >  
> >  	/* Failure to allocate our preferred name is not critical */
> > -- 
> > 2.16.2
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Souza, Jose]

On Tue, 2018-02-27 at 23:34 +0200, Ville Syrjälä wrote:
> On Tue, Feb 27, 2018 at 01:23:59PM -0800, José Roberto de Souza
> wrote:
> > When PSR/PSR2/GTC is enabled hardware can do AUX transactions by it
> > self, so lets use the mutex register that is available in gen9+ to
> > avoid concurrent access by hardware and driver.
> > Older gen handling will be done separated.
> > 
> > Reference: https://01.org/sites/default/files/documentation/intel-g
> > fx-prm-osrc-skl-vol12-display.pdf
> > Page 198 - AUX programming sequence
> > 
> > Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> > 
> > Changelog:
> > v2
> > - removed the PSR dependency, now getting lock all the times when
> > available
> > - renamed functions to avoid nested calls
> > - moved register bits right after the DP_AUX_CH_MUTEX()
> > - removed 'drm/i915: keep AUX powered while PSR is enabled'
> > Dhinakaran Pandiyan will sent a better and final version
> > v3
> > - rebased on top of Ville's AUX series
> > - moved port registers to above DP_AUX_CH_MUTEX()
> > - using intel_wait_for_register() instead of the internal version
> > v4
> > - removed virtual function to get mutex register address
> > - enabling the mutex back only on aux channel init
> > - added the aux channel name to the timeout debug message
> > v5
> > - renamed DP_AUX_CH_MUTEX() parameter to aux_ch
> > - renamed goto label when intel_dp_aux_ch_trylock() fails
> > 
> >  drivers/gpu/drm/i915/i915_reg.h |  9 ++++++++
> >  drivers/gpu/drm/i915/intel_dp.c | 47
> > +++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 56 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > b/drivers/gpu/drm/i915/i915_reg.h
> > index eea5b2c537d4..bce2e6dad4c4 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -5385,6 +5385,15 @@ enum {
> >  #define   DP_AUX_CH_CTL_FW_SYNC_PULSE_SKL(c) (((c) - 1) << 5)
> >  #define   DP_AUX_CH_CTL_SYNC_PULSE_SKL(c)   ((c) - 1)
> >  
> > +#define _DPA_AUX_CH_MUTEX	(dev_priv-
> > >info.display_mmio_offset + 0x6402C)
> > +#define _DPB_AUX_CH_MUTEX	(dev_priv-
> > >info.display_mmio_offset + 0x6412C)
> > +#define _DPC_AUX_CH_MUTEX	(dev_priv-
> > >info.display_mmio_offset + 0x6422C)
> > +#define _DPD_AUX_CH_MUTEX	(dev_priv-
> > >info.display_mmio_offset + 0x6432C)
> > +#define _DPF_AUX_CH_MUTEX	(dev_priv-
> > >info.display_mmio_offset + 0x6452C)
> > +#define DP_AUX_CH_MUTEX(aux_ch)	_MMIO_PORT(aux_ch,
> > _DPA_AUX_CH_MUTEX, _DPB_AUX_CH_MUTEX)
> > +#define   DP_AUX_CH_MUTEX_ENABLE		(1 << 31)
> > +#define   DP_AUX_CH_MUTEX_STATUS		(1 << 30)
> > +
> >  /*
> >   * Computing GMCH M and N values for the Display Port link
> >   *
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > b/drivers/gpu/drm/i915/intel_dp.c
> > index 2a3b3ae4e3da..7f4bf77227cd 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -1081,6 +1081,42 @@ static uint32_t
> > intel_dp_get_aux_send_ctl(struct intel_dp *intel_dp,
> >  						aux_clock_divider)
> > ;
> >  }
> >  
> > +static bool intel_dp_aux_ch_trylock(struct intel_dp *intel_dp)
> > +{
> > +	struct intel_digital_port *intel_dig_port =
> > dp_to_dig_port(intel_dp);
> > +	struct drm_i915_private *dev_priv =
> > +			to_i915(intel_dig_port->base.base.dev);
> > +
> > +	if (INTEL_GEN(dev_priv) < 9)
> > +		return true;
> > +
> > +	/* Spec says that mutex is acquired when status bit is
> > read as unset,
> > +	 * here waiting for 2msec(+-4 aux transactions) before
> > give up.
> > +	 */
> > +	if (intel_wait_for_register(dev_priv,
> > DP_AUX_CH_MUTEX(intel_dp->aux_ch),
> > +				    DP_AUX_CH_MUTEX_STATUS, 0, 2))
> > {
> > +		DRM_DEBUG_KMS("aux channel %c locked for 2msec,
> > timing out\n",
> > +			      aux_ch_name(intel_dp->aux_ch));
> > +		return false;
> > +	}
> > +
> > +	return true;
> > +}
> > +
> > +static void intel_dp_aux_ch_unlock(struct intel_dp *intel_dp)
> > +{
> > +	struct intel_digital_port *intel_dig_port =
> > dp_to_dig_port(intel_dp);
> > +	struct drm_i915_private *dev_priv =
> > +			to_i915(intel_dig_port->base.base.dev);
> > +
> > +	if (INTEL_GEN(dev_priv) < 9)
> > +		return;
> > +
> > +	/* set the status bit releases the mutex + keeping mutex
> > enabled */
> > +	I915_WRITE(DP_AUX_CH_MUTEX(intel_dp->aux_ch),
> > +		   DP_AUX_CH_MUTEX_ENABLE |
> > DP_AUX_CH_MUTEX_STATUS);
> > +}
> > +
> >  static int
> >  intel_dp_aux_ch(struct intel_dp *intel_dp,
> >  		const uint8_t *send, int send_bytes,
> > @@ -1119,6 +1155,11 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
> >  
> >  	intel_dp_check_edp(intel_dp);
> >  
> > +	if (!intel_dp_aux_ch_trylock(intel_dp)) {
> > +		ret = -EBUSY;
> > +		goto out_aux_ch_unlocked;
> > +	}
> > +
> >  	/* Try to wait for any previous AUX channel activity */
> >  	for (try = 0; try < 3; try++) {
> >  		status = I915_READ_NOTRACE(ch_ctl);
> > @@ -1240,6 +1281,8 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
> >  
> >  	ret = recv_bytes;
> >  out:
> > +	intel_dp_aux_ch_unlock(intel_dp);
> > +out_aux_ch_unlocked:
> >  	pm_qos_update_request(&dev_priv->pm_qos,
> > PM_QOS_DEFAULT_VALUE);
> >  
> >  	if (vdd)
> > @@ -1536,6 +1579,10 @@ intel_dp_aux_init(struct intel_dp *intel_dp)
> >  	else
> >  		intel_dp->get_aux_send_ctl = g4x_get_aux_send_ctl;
> >  
> > +	if (INTEL_GEN(dev_priv) >= 9)
> > +		I915_WRITE(DP_AUX_CH_MUTEX(intel_dp->aux_ch),
> > +			   DP_AUX_CH_MUTEX_ENABLE);
> 
> And who enables it after system/runtime PM etc.? Not sure how much
> sense
> there is to finesse this anyway. One extra mmio write every time we
> try
> to acquire the mutex shouldn't kill anyone.

Valid point, but I guess is better enable the mutex in
intel_dp_encoder_reset():

"
@@ -5293,6 +5340,10 @@ void intel_dp_encoder_reset(struct drm_encoder
*encoder)

        pps_lock(intel_dp);

+       if (INTEL_GEN(dev_priv) >= 9)
+               I915_WRITE(DP_AUX_CH_MUTEX(intel_dp->aux_ch),
+                          DP_AUX_CH_MUTEX_ENABLE);
+
        if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
                intel_dp->active_pipe = vlv_active_pipe(intel_dp);
"

any objections?



> 
> > +
> >  	drm_dp_aux_init(&intel_dp->aux);
> >  
> >  	/* Failure to allocate our preferred name is not critical
> > */
> > -- 
> > 2.16.2
> 
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Ville Syrjälä]

On Wed, Feb 28, 2018 at 12:57:07AM +0000, Souza, Jose wrote:
> On Tue, 2018-02-27 at 23:34 +0200, Ville Syrjälä wrote:
> > On Tue, Feb 27, 2018 at 01:23:59PM -0800, José Roberto de Souza
> > wrote:
> > > When PSR/PSR2/GTC is enabled hardware can do AUX transactions by it
> > > self, so lets use the mutex register that is available in gen9+ to
> > > avoid concurrent access by hardware and driver.
> > > Older gen handling will be done separated.
> > > 
> > > Reference: https://01.org/sites/default/files/documentation/intel-g
> > > fx-prm-osrc-skl-vol12-display.pdf
> > > Page 198 - AUX programming sequence
> > > 
> > > Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > ---
> > > 
> > > Changelog:
> > > v2
> > > - removed the PSR dependency, now getting lock all the times when
> > > available
> > > - renamed functions to avoid nested calls
> > > - moved register bits right after the DP_AUX_CH_MUTEX()
> > > - removed 'drm/i915: keep AUX powered while PSR is enabled'
> > > Dhinakaran Pandiyan will sent a better and final version
> > > v3
> > > - rebased on top of Ville's AUX series
> > > - moved port registers to above DP_AUX_CH_MUTEX()
> > > - using intel_wait_for_register() instead of the internal version
> > > v4
> > > - removed virtual function to get mutex register address
> > > - enabling the mutex back only on aux channel init
> > > - added the aux channel name to the timeout debug message
> > > v5
> > > - renamed DP_AUX_CH_MUTEX() parameter to aux_ch
> > > - renamed goto label when intel_dp_aux_ch_trylock() fails
> > > 
> > >  drivers/gpu/drm/i915/i915_reg.h |  9 ++++++++
> > >  drivers/gpu/drm/i915/intel_dp.c | 47
> > > +++++++++++++++++++++++++++++++++++++++++
> > >  2 files changed, 56 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > > b/drivers/gpu/drm/i915/i915_reg.h
> > > index eea5b2c537d4..bce2e6dad4c4 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -5385,6 +5385,15 @@ enum {
> > >  #define   DP_AUX_CH_CTL_FW_SYNC_PULSE_SKL(c) (((c) - 1) << 5)
> > >  #define   DP_AUX_CH_CTL_SYNC_PULSE_SKL(c)   ((c) - 1)
> > >  
> > > +#define _DPA_AUX_CH_MUTEX	(dev_priv-
> > > >info.display_mmio_offset + 0x6402C)
> > > +#define _DPB_AUX_CH_MUTEX	(dev_priv-
> > > >info.display_mmio_offset + 0x6412C)
> > > +#define _DPC_AUX_CH_MUTEX	(dev_priv-
> > > >info.display_mmio_offset + 0x6422C)
> > > +#define _DPD_AUX_CH_MUTEX	(dev_priv-
> > > >info.display_mmio_offset + 0x6432C)
> > > +#define _DPF_AUX_CH_MUTEX	(dev_priv-
> > > >info.display_mmio_offset + 0x6452C)
> > > +#define DP_AUX_CH_MUTEX(aux_ch)	_MMIO_PORT(aux_ch,
> > > _DPA_AUX_CH_MUTEX, _DPB_AUX_CH_MUTEX)
> > > +#define   DP_AUX_CH_MUTEX_ENABLE		(1 << 31)
> > > +#define   DP_AUX_CH_MUTEX_STATUS		(1 << 30)
> > > +
> > >  /*
> > >   * Computing GMCH M and N values for the Display Port link
> > >   *
> > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > b/drivers/gpu/drm/i915/intel_dp.c
> > > index 2a3b3ae4e3da..7f4bf77227cd 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > @@ -1081,6 +1081,42 @@ static uint32_t
> > > intel_dp_get_aux_send_ctl(struct intel_dp *intel_dp,
> > >  						aux_clock_divider)
> > > ;
> > >  }
> > >  
> > > +static bool intel_dp_aux_ch_trylock(struct intel_dp *intel_dp)
> > > +{
> > > +	struct intel_digital_port *intel_dig_port =
> > > dp_to_dig_port(intel_dp);
> > > +	struct drm_i915_private *dev_priv =
> > > +			to_i915(intel_dig_port->base.base.dev);
> > > +
> > > +	if (INTEL_GEN(dev_priv) < 9)
> > > +		return true;
> > > +
> > > +	/* Spec says that mutex is acquired when status bit is
> > > read as unset,
> > > +	 * here waiting for 2msec(+-4 aux transactions) before
> > > give up.
> > > +	 */
> > > +	if (intel_wait_for_register(dev_priv,
> > > DP_AUX_CH_MUTEX(intel_dp->aux_ch),
> > > +				    DP_AUX_CH_MUTEX_STATUS, 0, 2))
> > > {
> > > +		DRM_DEBUG_KMS("aux channel %c locked for 2msec,
> > > timing out\n",
> > > +			      aux_ch_name(intel_dp->aux_ch));
> > > +		return false;
> > > +	}
> > > +
> > > +	return true;
> > > +}
> > > +
> > > +static void intel_dp_aux_ch_unlock(struct intel_dp *intel_dp)
> > > +{
> > > +	struct intel_digital_port *intel_dig_port =
> > > dp_to_dig_port(intel_dp);
> > > +	struct drm_i915_private *dev_priv =
> > > +			to_i915(intel_dig_port->base.base.dev);
> > > +
> > > +	if (INTEL_GEN(dev_priv) < 9)
> > > +		return;
> > > +
> > > +	/* set the status bit releases the mutex + keeping mutex
> > > enabled */
> > > +	I915_WRITE(DP_AUX_CH_MUTEX(intel_dp->aux_ch),
> > > +		   DP_AUX_CH_MUTEX_ENABLE |
> > > DP_AUX_CH_MUTEX_STATUS);
> > > +}
> > > +
> > >  static int
> > >  intel_dp_aux_ch(struct intel_dp *intel_dp,
> > >  		const uint8_t *send, int send_bytes,
> > > @@ -1119,6 +1155,11 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
> > >  
> > >  	intel_dp_check_edp(intel_dp);
> > >  
> > > +	if (!intel_dp_aux_ch_trylock(intel_dp)) {
> > > +		ret = -EBUSY;
> > > +		goto out_aux_ch_unlocked;
> > > +	}
> > > +
> > >  	/* Try to wait for any previous AUX channel activity */
> > >  	for (try = 0; try < 3; try++) {
> > >  		status = I915_READ_NOTRACE(ch_ctl);
> > > @@ -1240,6 +1281,8 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
> > >  
> > >  	ret = recv_bytes;
> > >  out:
> > > +	intel_dp_aux_ch_unlock(intel_dp);
> > > +out_aux_ch_unlocked:
> > >  	pm_qos_update_request(&dev_priv->pm_qos,
> > > PM_QOS_DEFAULT_VALUE);
> > >  
> > >  	if (vdd)
> > > @@ -1536,6 +1579,10 @@ intel_dp_aux_init(struct intel_dp *intel_dp)
> > >  	else
> > >  		intel_dp->get_aux_send_ctl = g4x_get_aux_send_ctl;
> > >  
> > > +	if (INTEL_GEN(dev_priv) >= 9)
> > > +		I915_WRITE(DP_AUX_CH_MUTEX(intel_dp->aux_ch),
> > > +			   DP_AUX_CH_MUTEX_ENABLE);
> > 
> > And who enables it after system/runtime PM etc.? Not sure how much
> > sense
> > there is to finesse this anyway. One extra mmio write every time we
> > try
> > to acquire the mutex shouldn't kill anyone.
> 
> Valid point, but I guess is better enable the mutex in
> intel_dp_encoder_reset():
> 
> "
> @@ -5293,6 +5340,10 @@ void intel_dp_encoder_reset(struct drm_encoder
> *encoder)
> 
>         pps_lock(intel_dp);
> 
> +       if (INTEL_GEN(dev_priv) >= 9)
> +               I915_WRITE(DP_AUX_CH_MUTEX(intel_dp->aux_ch),
> +                          DP_AUX_CH_MUTEX_ENABLE);
> +
>         if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
>                 intel_dp->active_pipe = vlv_active_pipe(intel_dp);
> "
> 
> any objections?

I guess we can hope that dmc takes care of it for dc5/6. But what
about dc9?

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

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

[Souza, Jose]

On Wed, 2018-02-28 at 13:09 +0200, Ville Syrjälä wrote:
> On Wed, Feb 28, 2018 at 12:57:07AM +0000, Souza, Jose wrote:
> > On Tue, 2018-02-27 at 23:34 +0200, Ville Syrjälä wrote:
> > > On Tue, Feb 27, 2018 at 01:23:59PM -0800, José Roberto de Souza
> > > wrote:
> > > > When PSR/PSR2/GTC is enabled hardware can do AUX transactions
> > > > by it
> > > > self, so lets use the mutex register that is available in gen9+
> > > > to
> > > > avoid concurrent access by hardware and driver.
> > > > Older gen handling will be done separated.
> > > > 
> > > > Reference: https://01.org/sites/default/files/documentation/int
> > > > el-g
> > > > fx-prm-osrc-skl-vol12-display.pdf
> > > > Page 198 - AUX programming sequence
> > > > 
> > > > Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com
> > > > >
> > > > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > > ---
> > > > 
> > > > Changelog:
> > > > v2
> > > > - removed the PSR dependency, now getting lock all the times
> > > > when
> > > > available
> > > > - renamed functions to avoid nested calls
> > > > - moved register bits right after the DP_AUX_CH_MUTEX()
> > > > - removed 'drm/i915: keep AUX powered while PSR is enabled'
> > > > Dhinakaran Pandiyan will sent a better and final version
> > > > v3
> > > > - rebased on top of Ville's AUX series
> > > > - moved port registers to above DP_AUX_CH_MUTEX()
> > > > - using intel_wait_for_register() instead of the internal
> > > > version
> > > > v4
> > > > - removed virtual function to get mutex register address
> > > > - enabling the mutex back only on aux channel init
> > > > - added the aux channel name to the timeout debug message
> > > > v5
> > > > - renamed DP_AUX_CH_MUTEX() parameter to aux_ch
> > > > - renamed goto label when intel_dp_aux_ch_trylock() fails
> > > > 
> > > >  drivers/gpu/drm/i915/i915_reg.h |  9 ++++++++
> > > >  drivers/gpu/drm/i915/intel_dp.c | 47
> > > > +++++++++++++++++++++++++++++++++++++++++
> > > >  2 files changed, 56 insertions(+)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > > > b/drivers/gpu/drm/i915/i915_reg.h
> > > > index eea5b2c537d4..bce2e6dad4c4 100644
> > > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > > @@ -5385,6 +5385,15 @@ enum {
> > > >  #define   DP_AUX_CH_CTL_FW_SYNC_PULSE_SKL(c) (((c) - 1) << 5)
> > > >  #define   DP_AUX_CH_CTL_SYNC_PULSE_SKL(c)   ((c) - 1)
> > > >  
> > > > +#define _DPA_AUX_CH_MUTEX	(dev_priv-
> > > > > info.display_mmio_offset + 0x6402C)
> > > > 
> > > > +#define _DPB_AUX_CH_MUTEX	(dev_priv-
> > > > > info.display_mmio_offset + 0x6412C)
> > > > 
> > > > +#define _DPC_AUX_CH_MUTEX	(dev_priv-
> > > > > info.display_mmio_offset + 0x6422C)
> > > > 
> > > > +#define _DPD_AUX_CH_MUTEX	(dev_priv-
> > > > > info.display_mmio_offset + 0x6432C)
> > > > 
> > > > +#define _DPF_AUX_CH_MUTEX	(dev_priv-
> > > > > info.display_mmio_offset + 0x6452C)
> > > > 
> > > > +#define DP_AUX_CH_MUTEX(aux_ch)	_MMIO_PORT(aux_ch,
> > > > _DPA_AUX_CH_MUTEX, _DPB_AUX_CH_MUTEX)
> > > > +#define   DP_AUX_CH_MUTEX_ENABLE		(1 << 31)
> > > > +#define   DP_AUX_CH_MUTEX_STATUS		(1 << 30)
> > > > +
> > > >  /*
> > > >   * Computing GMCH M and N values for the Display Port link
> > > >   *
> > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > > b/drivers/gpu/drm/i915/intel_dp.c
> > > > index 2a3b3ae4e3da..7f4bf77227cd 100644
> > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > @@ -1081,6 +1081,42 @@ static uint32_t
> > > > intel_dp_get_aux_send_ctl(struct intel_dp *intel_dp,
> > > >  						aux_clock_divi
> > > > der)
> > > > ;
> > > >  }
> > > >  
> > > > +static bool intel_dp_aux_ch_trylock(struct intel_dp *intel_dp)
> > > > +{
> > > > +	struct intel_digital_port *intel_dig_port =
> > > > dp_to_dig_port(intel_dp);
> > > > +	struct drm_i915_private *dev_priv =
> > > > +			to_i915(intel_dig_port-
> > > > >base.base.dev);
> > > > +
> > > > +	if (INTEL_GEN(dev_priv) < 9)
> > > > +		return true;
> > > > +
> > > > +	/* Spec says that mutex is acquired when status bit is
> > > > read as unset,
> > > > +	 * here waiting for 2msec(+-4 aux transactions) before
> > > > give up.
> > > > +	 */
> > > > +	if (intel_wait_for_register(dev_priv,
> > > > DP_AUX_CH_MUTEX(intel_dp->aux_ch),
> > > > +				    DP_AUX_CH_MUTEX_STATUS, 0,
> > > > 2))
> > > > {
> > > > +		DRM_DEBUG_KMS("aux channel %c locked for
> > > > 2msec,
> > > > timing out\n",
> > > > +			      aux_ch_name(intel_dp->aux_ch));
> > > > +		return false;
> > > > +	}
> > > > +
> > > > +	return true;
> > > > +}
> > > > +
> > > > +static void intel_dp_aux_ch_unlock(struct intel_dp *intel_dp)
> > > > +{
> > > > +	struct intel_digital_port *intel_dig_port =
> > > > dp_to_dig_port(intel_dp);
> > > > +	struct drm_i915_private *dev_priv =
> > > > +			to_i915(intel_dig_port-
> > > > >base.base.dev);
> > > > +
> > > > +	if (INTEL_GEN(dev_priv) < 9)
> > > > +		return;
> > > > +
> > > > +	/* set the status bit releases the mutex + keeping
> > > > mutex
> > > > enabled */
> > > > +	I915_WRITE(DP_AUX_CH_MUTEX(intel_dp->aux_ch),
> > > > +		   DP_AUX_CH_MUTEX_ENABLE |
> > > > DP_AUX_CH_MUTEX_STATUS);
> > > > +}
> > > > +
> > > >  static int
> > > >  intel_dp_aux_ch(struct intel_dp *intel_dp,
> > > >  		const uint8_t *send, int send_bytes,
> > > > @@ -1119,6 +1155,11 @@ intel_dp_aux_ch(struct intel_dp
> > > > *intel_dp,
> > > >  
> > > >  	intel_dp_check_edp(intel_dp);
> > > >  
> > > > +	if (!intel_dp_aux_ch_trylock(intel_dp)) {
> > > > +		ret = -EBUSY;
> > > > +		goto out_aux_ch_unlocked;
> > > > +	}
> > > > +
> > > >  	/* Try to wait for any previous AUX channel activity
> > > > */
> > > >  	for (try = 0; try < 3; try++) {
> > > >  		status = I915_READ_NOTRACE(ch_ctl);
> > > > @@ -1240,6 +1281,8 @@ intel_dp_aux_ch(struct intel_dp
> > > > *intel_dp,
> > > >  
> > > >  	ret = recv_bytes;
> > > >  out:
> > > > +	intel_dp_aux_ch_unlock(intel_dp);
> > > > +out_aux_ch_unlocked:
> > > >  	pm_qos_update_request(&dev_priv->pm_qos,
> > > > PM_QOS_DEFAULT_VALUE);
> > > >  
> > > >  	if (vdd)
> > > > @@ -1536,6 +1579,10 @@ intel_dp_aux_init(struct intel_dp
> > > > *intel_dp)
> > > >  	else
> > > >  		intel_dp->get_aux_send_ctl =
> > > > g4x_get_aux_send_ctl;
> > > >  
> > > > +	if (INTEL_GEN(dev_priv) >= 9)
> > > > +		I915_WRITE(DP_AUX_CH_MUTEX(intel_dp->aux_ch),
> > > > +			   DP_AUX_CH_MUTEX_ENABLE);
> > > 
> > > And who enables it after system/runtime PM etc.? Not sure how
> > > much
> > > sense
> > > there is to finesse this anyway. One extra mmio write every time
> > > we
> > > try
> > > to acquire the mutex shouldn't kill anyone.
> > 
> > Valid point, but I guess is better enable the mutex in
> > intel_dp_encoder_reset():
> > 
> > "
> > @@ -5293,6 +5340,10 @@ void intel_dp_encoder_reset(struct
> > drm_encoder
> > *encoder)
> > 
> >         pps_lock(intel_dp);
> > 
> > +       if (INTEL_GEN(dev_priv) >= 9)
> > +               I915_WRITE(DP_AUX_CH_MUTEX(intel_dp->aux_ch),
> > +                          DP_AUX_CH_MUTEX_ENABLE);
> > +
> >         if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> >                 intel_dp->active_pipe = vlv_active_pipe(intel_dp);
> > "
> > 
> > any objections?
> 
> I guess we can hope that dmc takes care of it for dc5/6. But what
> about dc9?

Yep, for DC5 and DC6 the registers are restored by hardware but it is
not the case for DC9. If I'm not missing something looks like there is
no handling to restore registers when exiting DC9.

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

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

[Ville Syrjälä]

On Wed, Feb 28, 2018 at 11:55:39PM +0000, Souza, Jose wrote:
> On Wed, 2018-02-28 at 13:09 +0200, Ville Syrjälä wrote:
> > On Wed, Feb 28, 2018 at 12:57:07AM +0000, Souza, Jose wrote:
> > > On Tue, 2018-02-27 at 23:34 +0200, Ville Syrjälä wrote:
> > > > On Tue, Feb 27, 2018 at 01:23:59PM -0800, José Roberto de Souza
> > > > wrote:
> > > > > When PSR/PSR2/GTC is enabled hardware can do AUX transactions
> > > > > by it
> > > > > self, so lets use the mutex register that is available in gen9+
> > > > > to
> > > > > avoid concurrent access by hardware and driver.
> > > > > Older gen handling will be done separated.
> > > > > 
> > > > > Reference: https://01.org/sites/default/files/documentation/int
> > > > > el-g
> > > > > fx-prm-osrc-skl-vol12-display.pdf
> > > > > Page 198 - AUX programming sequence
> > > > > 
> > > > > Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com
> > > > > >
> > > > > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > > > ---
> > > > > 
> > > > > Changelog:
> > > > > v2
> > > > > - removed the PSR dependency, now getting lock all the times
> > > > > when
> > > > > available
> > > > > - renamed functions to avoid nested calls
> > > > > - moved register bits right after the DP_AUX_CH_MUTEX()
> > > > > - removed 'drm/i915: keep AUX powered while PSR is enabled'
> > > > > Dhinakaran Pandiyan will sent a better and final version
> > > > > v3
> > > > > - rebased on top of Ville's AUX series
> > > > > - moved port registers to above DP_AUX_CH_MUTEX()
> > > > > - using intel_wait_for_register() instead of the internal
> > > > > version
> > > > > v4
> > > > > - removed virtual function to get mutex register address
> > > > > - enabling the mutex back only on aux channel init
> > > > > - added the aux channel name to the timeout debug message
> > > > > v5
> > > > > - renamed DP_AUX_CH_MUTEX() parameter to aux_ch
> > > > > - renamed goto label when intel_dp_aux_ch_trylock() fails
> > > > > 
> > > > >  drivers/gpu/drm/i915/i915_reg.h |  9 ++++++++
> > > > >  drivers/gpu/drm/i915/intel_dp.c | 47
> > > > > +++++++++++++++++++++++++++++++++++++++++
> > > > >  2 files changed, 56 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > > > > b/drivers/gpu/drm/i915/i915_reg.h
> > > > > index eea5b2c537d4..bce2e6dad4c4 100644
> > > > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > > > @@ -5385,6 +5385,15 @@ enum {
> > > > >  #define   DP_AUX_CH_CTL_FW_SYNC_PULSE_SKL(c) (((c) - 1) << 5)
> > > > >  #define   DP_AUX_CH_CTL_SYNC_PULSE_SKL(c)   ((c) - 1)
> > > > >  
> > > > > +#define _DPA_AUX_CH_MUTEX	(dev_priv-
> > > > > > info.display_mmio_offset + 0x6402C)
> > > > > 
> > > > > +#define _DPB_AUX_CH_MUTEX	(dev_priv-
> > > > > > info.display_mmio_offset + 0x6412C)
> > > > > 
> > > > > +#define _DPC_AUX_CH_MUTEX	(dev_priv-
> > > > > > info.display_mmio_offset + 0x6422C)
> > > > > 
> > > > > +#define _DPD_AUX_CH_MUTEX	(dev_priv-
> > > > > > info.display_mmio_offset + 0x6432C)
> > > > > 
> > > > > +#define _DPF_AUX_CH_MUTEX	(dev_priv-
> > > > > > info.display_mmio_offset + 0x6452C)
> > > > > 
> > > > > +#define DP_AUX_CH_MUTEX(aux_ch)	_MMIO_PORT(aux_ch,
> > > > > _DPA_AUX_CH_MUTEX, _DPB_AUX_CH_MUTEX)
> > > > > +#define   DP_AUX_CH_MUTEX_ENABLE		(1 << 31)
> > > > > +#define   DP_AUX_CH_MUTEX_STATUS		(1 << 30)
> > > > > +
> > > > >  /*
> > > > >   * Computing GMCH M and N values for the Display Port link
> > > > >   *
> > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > > > b/drivers/gpu/drm/i915/intel_dp.c
> > > > > index 2a3b3ae4e3da..7f4bf77227cd 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > > @@ -1081,6 +1081,42 @@ static uint32_t
> > > > > intel_dp_get_aux_send_ctl(struct intel_dp *intel_dp,
> > > > >  						aux_clock_divi
> > > > > der)
> > > > > ;
> > > > >  }
> > > > >  
> > > > > +static bool intel_dp_aux_ch_trylock(struct intel_dp *intel_dp)
> > > > > +{
> > > > > +	struct intel_digital_port *intel_dig_port =
> > > > > dp_to_dig_port(intel_dp);
> > > > > +	struct drm_i915_private *dev_priv =
> > > > > +			to_i915(intel_dig_port-
> > > > > >base.base.dev);
> > > > > +
> > > > > +	if (INTEL_GEN(dev_priv) < 9)
> > > > > +		return true;
> > > > > +
> > > > > +	/* Spec says that mutex is acquired when status bit is
> > > > > read as unset,
> > > > > +	 * here waiting for 2msec(+-4 aux transactions) before
> > > > > give up.
> > > > > +	 */
> > > > > +	if (intel_wait_for_register(dev_priv,
> > > > > DP_AUX_CH_MUTEX(intel_dp->aux_ch),
> > > > > +				    DP_AUX_CH_MUTEX_STATUS, 0,
> > > > > 2))
> > > > > {
> > > > > +		DRM_DEBUG_KMS("aux channel %c locked for
> > > > > 2msec,
> > > > > timing out\n",
> > > > > +			      aux_ch_name(intel_dp->aux_ch));
> > > > > +		return false;
> > > > > +	}
> > > > > +
> > > > > +	return true;
> > > > > +}
> > > > > +
> > > > > +static void intel_dp_aux_ch_unlock(struct intel_dp *intel_dp)
> > > > > +{
> > > > > +	struct intel_digital_port *intel_dig_port =
> > > > > dp_to_dig_port(intel_dp);
> > > > > +	struct drm_i915_private *dev_priv =
> > > > > +			to_i915(intel_dig_port-
> > > > > >base.base.dev);
> > > > > +
> > > > > +	if (INTEL_GEN(dev_priv) < 9)
> > > > > +		return;
> > > > > +
> > > > > +	/* set the status bit releases the mutex + keeping
> > > > > mutex
> > > > > enabled */
> > > > > +	I915_WRITE(DP_AUX_CH_MUTEX(intel_dp->aux_ch),
> > > > > +		   DP_AUX_CH_MUTEX_ENABLE |
> > > > > DP_AUX_CH_MUTEX_STATUS);
> > > > > +}
> > > > > +
> > > > >  static int
> > > > >  intel_dp_aux_ch(struct intel_dp *intel_dp,
> > > > >  		const uint8_t *send, int send_bytes,
> > > > > @@ -1119,6 +1155,11 @@ intel_dp_aux_ch(struct intel_dp
> > > > > *intel_dp,
> > > > >  
> > > > >  	intel_dp_check_edp(intel_dp);
> > > > >  
> > > > > +	if (!intel_dp_aux_ch_trylock(intel_dp)) {
> > > > > +		ret = -EBUSY;
> > > > > +		goto out_aux_ch_unlocked;
> > > > > +	}
> > > > > +
> > > > >  	/* Try to wait for any previous AUX channel activity
> > > > > */
> > > > >  	for (try = 0; try < 3; try++) {
> > > > >  		status = I915_READ_NOTRACE(ch_ctl);
> > > > > @@ -1240,6 +1281,8 @@ intel_dp_aux_ch(struct intel_dp
> > > > > *intel_dp,
> > > > >  
> > > > >  	ret = recv_bytes;
> > > > >  out:
> > > > > +	intel_dp_aux_ch_unlock(intel_dp);
> > > > > +out_aux_ch_unlocked:
> > > > >  	pm_qos_update_request(&dev_priv->pm_qos,
> > > > > PM_QOS_DEFAULT_VALUE);
> > > > >  
> > > > >  	if (vdd)
> > > > > @@ -1536,6 +1579,10 @@ intel_dp_aux_init(struct intel_dp
> > > > > *intel_dp)
> > > > >  	else
> > > > >  		intel_dp->get_aux_send_ctl =
> > > > > g4x_get_aux_send_ctl;
> > > > >  
> > > > > +	if (INTEL_GEN(dev_priv) >= 9)
> > > > > +		I915_WRITE(DP_AUX_CH_MUTEX(intel_dp->aux_ch),
> > > > > +			   DP_AUX_CH_MUTEX_ENABLE);
> > > > 
> > > > And who enables it after system/runtime PM etc.? Not sure how
> > > > much
> > > > sense
> > > > there is to finesse this anyway. One extra mmio write every time
> > > > we
> > > > try
> > > > to acquire the mutex shouldn't kill anyone.
> > > 
> > > Valid point, but I guess is better enable the mutex in
> > > intel_dp_encoder_reset():
> > > 
> > > "
> > > @@ -5293,6 +5340,10 @@ void intel_dp_encoder_reset(struct
> > > drm_encoder
> > > *encoder)
> > > 
> > >         pps_lock(intel_dp);
> > > 
> > > +       if (INTEL_GEN(dev_priv) >= 9)
> > > +               I915_WRITE(DP_AUX_CH_MUTEX(intel_dp->aux_ch),
> > > +                          DP_AUX_CH_MUTEX_ENABLE);
> > > +
> > >         if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> > >                 intel_dp->active_pipe = vlv_active_pipe(intel_dp);
> > > "
> > > 
> > > any objections?
> > 
> > I guess we can hope that dmc takes care of it for dc5/6. But what
> > about dc9?
> 
> Yep, for DC5 and DC6 the registers are restored by hardware but it is
> not the case for DC9. If I'm not missing something looks like there is
> no handling to restore registers when exiting DC9.

The design of the driver is "program all required registers when you
need them" instead of the "blindly save/restore everything" DMC firmware
approach.

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

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

[Pandiyan, Dhinakaran]

On Thu, 2018-03-01 at 12:53 +0200, Ville Syrjälä wrote:
> On Wed, Feb 28, 2018 at 11:55:39PM +0000, Souza, Jose wrote:
> > On Wed, 2018-02-28 at 13:09 +0200, Ville Syrjälä wrote:
> > > On Wed, Feb 28, 2018 at 12:57:07AM +0000, Souza, Jose wrote:
> > > > On Tue, 2018-02-27 at 23:34 +0200, Ville Syrjälä wrote:
> > > > > On Tue, Feb 27, 2018 at 01:23:59PM -0800, José Roberto de Souza
> > > > > wrote:
> > > > > > When PSR/PSR2/GTC is enabled hardware can do AUX transactions
> > > > > > by it
> > > > > > self, so lets use the mutex register that is available in gen9+
> > > > > > to
> > > > > > avoid concurrent access by hardware and driver.
> > > > > > Older gen handling will be done separated.
> > > > > > 
> > > > > > Reference: https://01.org/sites/default/files/documentation/int
> > > > > > el-g
> > > > > > fx-prm-osrc-skl-vol12-display.pdf
> > > > > > Page 198 - AUX programming sequence
> > > > > > 
> > > > > > Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com
> > > > > > >
> > > > > > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > > > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > > > > ---
> > > > > > 
> > > > > > Changelog:
> > > > > > v2
> > > > > > - removed the PSR dependency, now getting lock all the times
> > > > > > when
> > > > > > available
> > > > > > - renamed functions to avoid nested calls
> > > > > > - moved register bits right after the DP_AUX_CH_MUTEX()
> > > > > > - removed 'drm/i915: keep AUX powered while PSR is enabled'
> > > > > > Dhinakaran Pandiyan will sent a better and final version
> > > > > > v3
> > > > > > - rebased on top of Ville's AUX series
> > > > > > - moved port registers to above DP_AUX_CH_MUTEX()
> > > > > > - using intel_wait_for_register() instead of the internal
> > > > > > version
> > > > > > v4
> > > > > > - removed virtual function to get mutex register address
> > > > > > - enabling the mutex back only on aux channel init
> > > > > > - added the aux channel name to the timeout debug message
> > > > > > v5
> > > > > > - renamed DP_AUX_CH_MUTEX() parameter to aux_ch
> > > > > > - renamed goto label when intel_dp_aux_ch_trylock() fails
> > > > > > 
> > > > > >  drivers/gpu/drm/i915/i915_reg.h |  9 ++++++++
> > > > > >  drivers/gpu/drm/i915/intel_dp.c | 47
> > > > > > +++++++++++++++++++++++++++++++++++++++++
> > > > > >  2 files changed, 56 insertions(+)
> > > > > > 
> > > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > > > > > b/drivers/gpu/drm/i915/i915_reg.h
> > > > > > index eea5b2c537d4..bce2e6dad4c4 100644
> > > > > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > > > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > > > > @@ -5385,6 +5385,15 @@ enum {
> > > > > >  #define   DP_AUX_CH_CTL_FW_SYNC_PULSE_SKL(c) (((c) - 1) << 5)
> > > > > >  #define   DP_AUX_CH_CTL_SYNC_PULSE_SKL(c)   ((c) - 1)
> > > > > >  
> > > > > > +#define _DPA_AUX_CH_MUTEX	(dev_priv-
> > > > > > > info.display_mmio_offset + 0x6402C)
> > > > > > 
> > > > > > +#define _DPB_AUX_CH_MUTEX	(dev_priv-
> > > > > > > info.display_mmio_offset + 0x6412C)
> > > > > > 
> > > > > > +#define _DPC_AUX_CH_MUTEX	(dev_priv-
> > > > > > > info.display_mmio_offset + 0x6422C)
> > > > > > 
> > > > > > +#define _DPD_AUX_CH_MUTEX	(dev_priv-
> > > > > > > info.display_mmio_offset + 0x6432C)
> > > > > > 
> > > > > > +#define _DPF_AUX_CH_MUTEX	(dev_priv-
> > > > > > > info.display_mmio_offset + 0x6452C)
> > > > > > 
> > > > > > +#define DP_AUX_CH_MUTEX(aux_ch)	_MMIO_PORT(aux_ch,
> > > > > > _DPA_AUX_CH_MUTEX, _DPB_AUX_CH_MUTEX)
> > > > > > +#define   DP_AUX_CH_MUTEX_ENABLE		(1 << 31)
> > > > > > +#define   DP_AUX_CH_MUTEX_STATUS		(1 << 30)
> > > > > > +
> > > > > >  /*
> > > > > >   * Computing GMCH M and N values for the Display Port link
> > > > > >   *
> > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > > > > b/drivers/gpu/drm/i915/intel_dp.c
> > > > > > index 2a3b3ae4e3da..7f4bf77227cd 100644
> > > > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > > > @@ -1081,6 +1081,42 @@ static uint32_t
> > > > > > intel_dp_get_aux_send_ctl(struct intel_dp *intel_dp,
> > > > > >  						aux_clock_divi
> > > > > > der)
> > > > > > ;
> > > > > >  }
> > > > > >  
> > > > > > +static bool intel_dp_aux_ch_trylock(struct intel_dp *intel_dp)
> > > > > > +{
> > > > > > +	struct intel_digital_port *intel_dig_port =
> > > > > > dp_to_dig_port(intel_dp);
> > > > > > +	struct drm_i915_private *dev_priv =
> > > > > > +			to_i915(intel_dig_port-
> > > > > > >base.base.dev);
> > > > > > +
> > > > > > +	if (INTEL_GEN(dev_priv) < 9)
> > > > > > +		return true;
> > > > > > +
> > > > > > +	/* Spec says that mutex is acquired when status bit is
> > > > > > read as unset,
> > > > > > +	 * here waiting for 2msec(+-4 aux transactions) before
> > > > > > give up.
> > > > > > +	 */
> > > > > > +	if (intel_wait_for_register(dev_priv,
> > > > > > DP_AUX_CH_MUTEX(intel_dp->aux_ch),
> > > > > > +				    DP_AUX_CH_MUTEX_STATUS, 0,
> > > > > > 2))
> > > > > > {
> > > > > > +		DRM_DEBUG_KMS("aux channel %c locked for
> > > > > > 2msec,
> > > > > > timing out\n",
> > > > > > +			      aux_ch_name(intel_dp->aux_ch));
> > > > > > +		return false;
> > > > > > +	}
> > > > > > +
> > > > > > +	return true;
> > > > > > +}
> > > > > > +
> > > > > > +static void intel_dp_aux_ch_unlock(struct intel_dp *intel_dp)
> > > > > > +{
> > > > > > +	struct intel_digital_port *intel_dig_port =
> > > > > > dp_to_dig_port(intel_dp);
> > > > > > +	struct drm_i915_private *dev_priv =
> > > > > > +			to_i915(intel_dig_port-
> > > > > > >base.base.dev);
> > > > > > +
> > > > > > +	if (INTEL_GEN(dev_priv) < 9)
> > > > > > +		return;
> > > > > > +
> > > > > > +	/* set the status bit releases the mutex + keeping
> > > > > > mutex
> > > > > > enabled */
> > > > > > +	I915_WRITE(DP_AUX_CH_MUTEX(intel_dp->aux_ch),
> > > > > > +		   DP_AUX_CH_MUTEX_ENABLE |
> > > > > > DP_AUX_CH_MUTEX_STATUS);
> > > > > > +}
> > > > > > +
> > > > > >  static int
> > > > > >  intel_dp_aux_ch(struct intel_dp *intel_dp,
> > > > > >  		const uint8_t *send, int send_bytes,
> > > > > > @@ -1119,6 +1155,11 @@ intel_dp_aux_ch(struct intel_dp
> > > > > > *intel_dp,
> > > > > >  
> > > > > >  	intel_dp_check_edp(intel_dp);
> > > > > >  
> > > > > > +	if (!intel_dp_aux_ch_trylock(intel_dp)) {
> > > > > > +		ret = -EBUSY;
> > > > > > +		goto out_aux_ch_unlocked;
> > > > > > +	}
> > > > > > +
> > > > > >  	/* Try to wait for any previous AUX channel activity
> > > > > > */
> > > > > >  	for (try = 0; try < 3; try++) {
> > > > > >  		status = I915_READ_NOTRACE(ch_ctl);
> > > > > > @@ -1240,6 +1281,8 @@ intel_dp_aux_ch(struct intel_dp
> > > > > > *intel_dp,
> > > > > >  
> > > > > >  	ret = recv_bytes;
> > > > > >  out:
> > > > > > +	intel_dp_aux_ch_unlock(intel_dp);
> > > > > > +out_aux_ch_unlocked:
> > > > > >  	pm_qos_update_request(&dev_priv->pm_qos,
> > > > > > PM_QOS_DEFAULT_VALUE);
> > > > > >  
> > > > > >  	if (vdd)
> > > > > > @@ -1536,6 +1579,10 @@ intel_dp_aux_init(struct intel_dp
> > > > > > *intel_dp)
> > > > > >  	else
> > > > > >  		intel_dp->get_aux_send_ctl =
> > > > > > g4x_get_aux_send_ctl;
> > > > > >  
> > > > > > +	if (INTEL_GEN(dev_priv) >= 9)
> > > > > > +		I915_WRITE(DP_AUX_CH_MUTEX(intel_dp->aux_ch),
> > > > > > +			   DP_AUX_CH_MUTEX_ENABLE);
> > > > > 
> > > > > And who enables it after system/runtime PM etc.? Not sure how
> > > > > much
> > > > > sense
> > > > > there is to finesse this anyway. One extra mmio write every time
> > > > > we
> > > > > try
> > > > > to acquire the mutex shouldn't kill anyone.
> > > > 
> > > > Valid point, but I guess is better enable the mutex in
> > > > intel_dp_encoder_reset():
> > > > 
> > > > "
> > > > @@ -5293,6 +5340,10 @@ void intel_dp_encoder_reset(struct
> > > > drm_encoder
> > > > *encoder)
> > > > 
> > > >         pps_lock(intel_dp);
> > > > 
> > > > +       if (INTEL_GEN(dev_priv) >= 9)
> > > > +               I915_WRITE(DP_AUX_CH_MUTEX(intel_dp->aux_ch),
> > > > +                          DP_AUX_CH_MUTEX_ENABLE);
> > > > +
> > > >         if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> > > >                 intel_dp->active_pipe = vlv_active_pipe(intel_dp);
> > > > "
> > > > 
> > > > any objections?
> > > 
> > > I guess we can hope that dmc takes care of it for dc5/6. But what
> > > about dc9?
> > 
> > Yep, for DC5 and DC6 the registers are restored by hardware but it is
> > not the case for DC9. If I'm not missing something looks like there is
> > no handling to restore registers when exiting DC9.
> 
> The design of the driver is "program all required registers when you
> need them" instead of the "blindly save/restore everything" DMC firmware
> approach.
> 


+static bool intel_dp_aux_trylock(struct intel_dp *intel_dp)
+{
+       bool enabled;
+       u32 mutex;
+
+       /* READ will try to lock the aux channel if the mutex was
already
+        * enabled.
+        */
+       mutex = I915_READ(DP_AUX_CH_MUTEX);
+       enabled = mutex & DP_AUX_CH_MUTEX_ENABLE;
+
+       if (!enabled) {
+               /* mutex is currently disabled, enable it without
altering the
+                * status bit in case the HW is using the bit */
+               I915_WRITE(DP_AUX_CH_MUTEX(intel_dig_port->dp.aux_ch),
+                          mutex | DP_AUX_CH_MUTEX_ENABLE);
+
+               /* Now that the mutex is enabled, request to lock it */
+               mutex = I915_READ(DP_AUX_CH_MUTEX);
+       }
+
+       if (mutex & DP_AUX_CH_MUTEX_STATUS == 0)
+               return true;
+
+       if (intel_wait_for_register(dev_priv,
DP_AUX_CH_MUTEX(intel_dp->aux_ch),
+                                       DP_AUX_CH_MUTEX_STATUS, 0, 2)) {
+
+               DRM_DEBUG_KMS("aux channel %c locked for 2msec, timing
out\n",
+                             aux_ch_name(intel_dp->aux_ch));
+               return false;
+       }
+
+       return true;
+}
+


Have I missed anything here?
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Rodrigo Vivi]

On Fri, Mar 02, 2018 at 11:20:42PM +0000, Pandiyan, Dhinakaran wrote:
> 
> 
> 
> On Thu, 2018-03-01 at 12:53 +0200, Ville Syrjälä wrote:
> > On Wed, Feb 28, 2018 at 11:55:39PM +0000, Souza, Jose wrote:
> > > On Wed, 2018-02-28 at 13:09 +0200, Ville Syrjälä wrote:
> > > > On Wed, Feb 28, 2018 at 12:57:07AM +0000, Souza, Jose wrote:
> > > > > On Tue, 2018-02-27 at 23:34 +0200, Ville Syrjälä wrote:
> > > > > > On Tue, Feb 27, 2018 at 01:23:59PM -0800, José Roberto de Souza
> > > > > > wrote:
> > > > > > > When PSR/PSR2/GTC is enabled hardware can do AUX transactions
> > > > > > > by it
> > > > > > > self, so lets use the mutex register that is available in gen9+
> > > > > > > to
> > > > > > > avoid concurrent access by hardware and driver.
> > > > > > > Older gen handling will be done separated.
> > > > > > > 
> > > > > > > Reference: https://01.org/sites/default/files/documentation/int
> > > > > > > el-g
> > > > > > > fx-prm-osrc-skl-vol12-display.pdf
> > > > > > > Page 198 - AUX programming sequence
> > > > > > > 
> > > > > > > Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com
> > > > > > > >
> > > > > > > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > > > > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > > > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > > > > > ---
> > > > > > > 
> > > > > > > Changelog:
> > > > > > > v2
> > > > > > > - removed the PSR dependency, now getting lock all the times
> > > > > > > when
> > > > > > > available
> > > > > > > - renamed functions to avoid nested calls
> > > > > > > - moved register bits right after the DP_AUX_CH_MUTEX()
> > > > > > > - removed 'drm/i915: keep AUX powered while PSR is enabled'
> > > > > > > Dhinakaran Pandiyan will sent a better and final version
> > > > > > > v3
> > > > > > > - rebased on top of Ville's AUX series
> > > > > > > - moved port registers to above DP_AUX_CH_MUTEX()
> > > > > > > - using intel_wait_for_register() instead of the internal
> > > > > > > version
> > > > > > > v4
> > > > > > > - removed virtual function to get mutex register address
> > > > > > > - enabling the mutex back only on aux channel init
> > > > > > > - added the aux channel name to the timeout debug message
> > > > > > > v5
> > > > > > > - renamed DP_AUX_CH_MUTEX() parameter to aux_ch
> > > > > > > - renamed goto label when intel_dp_aux_ch_trylock() fails
> > > > > > > 
> > > > > > >  drivers/gpu/drm/i915/i915_reg.h |  9 ++++++++
> > > > > > >  drivers/gpu/drm/i915/intel_dp.c | 47
> > > > > > > +++++++++++++++++++++++++++++++++++++++++
> > > > > > >  2 files changed, 56 insertions(+)
> > > > > > > 
> > > > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > > > > > > b/drivers/gpu/drm/i915/i915_reg.h
> > > > > > > index eea5b2c537d4..bce2e6dad4c4 100644
> > > > > > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > > > > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > > > > > @@ -5385,6 +5385,15 @@ enum {
> > > > > > >  #define   DP_AUX_CH_CTL_FW_SYNC_PULSE_SKL(c) (((c) - 1) << 5)
> > > > > > >  #define   DP_AUX_CH_CTL_SYNC_PULSE_SKL(c)   ((c) - 1)
> > > > > > >  
> > > > > > > +#define _DPA_AUX_CH_MUTEX	(dev_priv-
> > > > > > > > info.display_mmio_offset + 0x6402C)
> > > > > > > 
> > > > > > > +#define _DPB_AUX_CH_MUTEX	(dev_priv-
> > > > > > > > info.display_mmio_offset + 0x6412C)
> > > > > > > 
> > > > > > > +#define _DPC_AUX_CH_MUTEX	(dev_priv-
> > > > > > > > info.display_mmio_offset + 0x6422C)
> > > > > > > 
> > > > > > > +#define _DPD_AUX_CH_MUTEX	(dev_priv-
> > > > > > > > info.display_mmio_offset + 0x6432C)
> > > > > > > 
> > > > > > > +#define _DPF_AUX_CH_MUTEX	(dev_priv-
> > > > > > > > info.display_mmio_offset + 0x6452C)
> > > > > > > 
> > > > > > > +#define DP_AUX_CH_MUTEX(aux_ch)	_MMIO_PORT(aux_ch,
> > > > > > > _DPA_AUX_CH_MUTEX, _DPB_AUX_CH_MUTEX)
> > > > > > > +#define   DP_AUX_CH_MUTEX_ENABLE		(1 << 31)
> > > > > > > +#define   DP_AUX_CH_MUTEX_STATUS		(1 << 30)
> > > > > > > +
> > > > > > >  /*
> > > > > > >   * Computing GMCH M and N values for the Display Port link
> > > > > > >   *
> > > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > > > > > b/drivers/gpu/drm/i915/intel_dp.c
> > > > > > > index 2a3b3ae4e3da..7f4bf77227cd 100644
> > > > > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > > > > @@ -1081,6 +1081,42 @@ static uint32_t
> > > > > > > intel_dp_get_aux_send_ctl(struct intel_dp *intel_dp,
> > > > > > >  						aux_clock_divi
> > > > > > > der)
> > > > > > > ;
> > > > > > >  }
> > > > > > >  
> > > > > > > +static bool intel_dp_aux_ch_trylock(struct intel_dp *intel_dp)
> > > > > > > +{
> > > > > > > +	struct intel_digital_port *intel_dig_port =
> > > > > > > dp_to_dig_port(intel_dp);
> > > > > > > +	struct drm_i915_private *dev_priv =
> > > > > > > +			to_i915(intel_dig_port-
> > > > > > > >base.base.dev);
> > > > > > > +
> > > > > > > +	if (INTEL_GEN(dev_priv) < 9)
> > > > > > > +		return true;
> > > > > > > +
> > > > > > > +	/* Spec says that mutex is acquired when status bit is
> > > > > > > read as unset,
> > > > > > > +	 * here waiting for 2msec(+-4 aux transactions) before
> > > > > > > give up.
> > > > > > > +	 */
> > > > > > > +	if (intel_wait_for_register(dev_priv,
> > > > > > > DP_AUX_CH_MUTEX(intel_dp->aux_ch),
> > > > > > > +				    DP_AUX_CH_MUTEX_STATUS, 0,
> > > > > > > 2))
> > > > > > > {
> > > > > > > +		DRM_DEBUG_KMS("aux channel %c locked for
> > > > > > > 2msec,
> > > > > > > timing out\n",
> > > > > > > +			      aux_ch_name(intel_dp->aux_ch));
> > > > > > > +		return false;
> > > > > > > +	}
> > > > > > > +
> > > > > > > +	return true;
> > > > > > > +}
> > > > > > > +
> > > > > > > +static void intel_dp_aux_ch_unlock(struct intel_dp *intel_dp)
> > > > > > > +{
> > > > > > > +	struct intel_digital_port *intel_dig_port =
> > > > > > > dp_to_dig_port(intel_dp);
> > > > > > > +	struct drm_i915_private *dev_priv =
> > > > > > > +			to_i915(intel_dig_port-
> > > > > > > >base.base.dev);
> > > > > > > +
> > > > > > > +	if (INTEL_GEN(dev_priv) < 9)
> > > > > > > +		return;
> > > > > > > +
> > > > > > > +	/* set the status bit releases the mutex + keeping
> > > > > > > mutex
> > > > > > > enabled */
> > > > > > > +	I915_WRITE(DP_AUX_CH_MUTEX(intel_dp->aux_ch),
> > > > > > > +		   DP_AUX_CH_MUTEX_ENABLE |
> > > > > > > DP_AUX_CH_MUTEX_STATUS);
> > > > > > > +}
> > > > > > > +
> > > > > > >  static int
> > > > > > >  intel_dp_aux_ch(struct intel_dp *intel_dp,
> > > > > > >  		const uint8_t *send, int send_bytes,
> > > > > > > @@ -1119,6 +1155,11 @@ intel_dp_aux_ch(struct intel_dp
> > > > > > > *intel_dp,
> > > > > > >  
> > > > > > >  	intel_dp_check_edp(intel_dp);
> > > > > > >  
> > > > > > > +	if (!intel_dp_aux_ch_trylock(intel_dp)) {
> > > > > > > +		ret = -EBUSY;
> > > > > > > +		goto out_aux_ch_unlocked;
> > > > > > > +	}
> > > > > > > +
> > > > > > >  	/* Try to wait for any previous AUX channel activity
> > > > > > > */
> > > > > > >  	for (try = 0; try < 3; try++) {
> > > > > > >  		status = I915_READ_NOTRACE(ch_ctl);
> > > > > > > @@ -1240,6 +1281,8 @@ intel_dp_aux_ch(struct intel_dp
> > > > > > > *intel_dp,
> > > > > > >  
> > > > > > >  	ret = recv_bytes;
> > > > > > >  out:
> > > > > > > +	intel_dp_aux_ch_unlock(intel_dp);
> > > > > > > +out_aux_ch_unlocked:
> > > > > > >  	pm_qos_update_request(&dev_priv->pm_qos,
> > > > > > > PM_QOS_DEFAULT_VALUE);
> > > > > > >  
> > > > > > >  	if (vdd)
> > > > > > > @@ -1536,6 +1579,10 @@ intel_dp_aux_init(struct intel_dp
> > > > > > > *intel_dp)
> > > > > > >  	else
> > > > > > >  		intel_dp->get_aux_send_ctl =
> > > > > > > g4x_get_aux_send_ctl;
> > > > > > >  
> > > > > > > +	if (INTEL_GEN(dev_priv) >= 9)
> > > > > > > +		I915_WRITE(DP_AUX_CH_MUTEX(intel_dp->aux_ch),
> > > > > > > +			   DP_AUX_CH_MUTEX_ENABLE);
> > > > > > 
> > > > > > And who enables it after system/runtime PM etc.? Not sure how
> > > > > > much
> > > > > > sense
> > > > > > there is to finesse this anyway. One extra mmio write every time
> > > > > > we
> > > > > > try
> > > > > > to acquire the mutex shouldn't kill anyone.
> > > > > 
> > > > > Valid point, but I guess is better enable the mutex in
> > > > > intel_dp_encoder_reset():
> > > > > 
> > > > > "
> > > > > @@ -5293,6 +5340,10 @@ void intel_dp_encoder_reset(struct
> > > > > drm_encoder
> > > > > *encoder)
> > > > > 
> > > > >         pps_lock(intel_dp);
> > > > > 
> > > > > +       if (INTEL_GEN(dev_priv) >= 9)
> > > > > +               I915_WRITE(DP_AUX_CH_MUTEX(intel_dp->aux_ch),
> > > > > +                          DP_AUX_CH_MUTEX_ENABLE);
> > > > > +
> > > > >         if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> > > > >                 intel_dp->active_pipe = vlv_active_pipe(intel_dp);
> > > > > "
> > > > > 
> > > > > any objections?
> > > > 
> > > > I guess we can hope that dmc takes care of it for dc5/6. But what
> > > > about dc9?
> > > 
> > > Yep, for DC5 and DC6 the registers are restored by hardware but it is
> > > not the case for DC9. If I'm not missing something looks like there is
> > > no handling to restore registers when exiting DC9.
> > 
> > The design of the driver is "program all required registers when you
> > need them" instead of the "blindly save/restore everything" DMC firmware
> > approach.
> > 
> 
> 
> +static bool intel_dp_aux_trylock(struct intel_dp *intel_dp)
> +{
> +       bool enabled;
> +       u32 mutex;
> +
> +       /* READ will try to lock the aux channel if the mutex was
> already
> +        * enabled.
> +        */
> +       mutex = I915_READ(DP_AUX_CH_MUTEX);
> +       enabled = mutex & DP_AUX_CH_MUTEX_ENABLE;
> +
> +       if (!enabled) {
> +               /* mutex is currently disabled, enable it without
> altering the
> +                * status bit in case the HW is using the bit */
> +               I915_WRITE(DP_AUX_CH_MUTEX(intel_dig_port->dp.aux_ch),
> +                          mutex | DP_AUX_CH_MUTEX_ENABLE);
> +
> +               /* Now that the mutex is enabled, request to lock it */
> +               mutex = I915_READ(DP_AUX_CH_MUTEX);
> +       }
> +
> +       if (mutex & DP_AUX_CH_MUTEX_STATUS == 0)
> +               return true;
> +
> +       if (intel_wait_for_register(dev_priv,
> DP_AUX_CH_MUTEX(intel_dp->aux_ch),
> +                                       DP_AUX_CH_MUTEX_STATUS, 0, 2)) {
> +
> +               DRM_DEBUG_KMS("aux channel %c locked for 2msec, timing
> out\n",
> +                             aux_ch_name(intel_dp->aux_ch));
> +               return false;
> +       }
> +
> +       return true;
> +}
> +
> 
> 
> Have I missed anything here?

ops, I wrote before seeing this here.

I like this approach a lot, although I don't believe it is harmful to just always set enabled bit.

anyways, +1 for this and probably only setting here.

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

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

[Rodrigo Vivi]

Ville Syrjälä <ville.syrjala@linux.intel.com> writes:

> On Wed, Feb 28, 2018 at 11:55:39PM +0000, Souza, Jose wrote:
>> On Wed, 2018-02-28 at 13:09 +0200, Ville Syrjälä wrote:
>> > On Wed, Feb 28, 2018 at 12:57:07AM +0000, Souza, Jose wrote:
>> > > On Tue, 2018-02-27 at 23:34 +0200, Ville Syrjälä wrote:
>> > > > On Tue, Feb 27, 2018 at 01:23:59PM -0800, José Roberto de Souza
>> > > > wrote:
>> > > > > When PSR/PSR2/GTC is enabled hardware can do AUX transactions
>> > > > > by it
>> > > > > self, so lets use the mutex register that is available in gen9+
>> > > > > to
>> > > > > avoid concurrent access by hardware and driver.
>> > > > > Older gen handling will be done separated.
>> > > > > 
>> > > > > Reference: https://01.org/sites/default/files/documentation/int
>> > > > > el-g
>> > > > > fx-prm-osrc-skl-vol12-display.pdf
>> > > > > Page 198 - AUX programming sequence
>> > > > > 
>> > > > > Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com
>> > > > > >
>> > > > > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> > > > > Cc: Jani Nikula <jani.nikula@linux.intel.com>
>> > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> > > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
>> > > > > ---
>> > > > > 
>> > > > > Changelog:
>> > > > > v2
>> > > > > - removed the PSR dependency, now getting lock all the times
>> > > > > when
>> > > > > available
>> > > > > - renamed functions to avoid nested calls
>> > > > > - moved register bits right after the DP_AUX_CH_MUTEX()
>> > > > > - removed 'drm/i915: keep AUX powered while PSR is enabled'
>> > > > > Dhinakaran Pandiyan will sent a better and final version
>> > > > > v3
>> > > > > - rebased on top of Ville's AUX series
>> > > > > - moved port registers to above DP_AUX_CH_MUTEX()
>> > > > > - using intel_wait_for_register() instead of the internal
>> > > > > version
>> > > > > v4
>> > > > > - removed virtual function to get mutex register address
>> > > > > - enabling the mutex back only on aux channel init
>> > > > > - added the aux channel name to the timeout debug message
>> > > > > v5
>> > > > > - renamed DP_AUX_CH_MUTEX() parameter to aux_ch
>> > > > > - renamed goto label when intel_dp_aux_ch_trylock() fails
>> > > > > 
>> > > > >  drivers/gpu/drm/i915/i915_reg.h |  9 ++++++++
>> > > > >  drivers/gpu/drm/i915/intel_dp.c | 47
>> > > > > +++++++++++++++++++++++++++++++++++++++++
>> > > > >  2 files changed, 56 insertions(+)
>> > > > > 
>> > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h
>> > > > > b/drivers/gpu/drm/i915/i915_reg.h
>> > > > > index eea5b2c537d4..bce2e6dad4c4 100644
>> > > > > --- a/drivers/gpu/drm/i915/i915_reg.h
>> > > > > +++ b/drivers/gpu/drm/i915/i915_reg.h
>> > > > > @@ -5385,6 +5385,15 @@ enum {
>> > > > >  #define   DP_AUX_CH_CTL_FW_SYNC_PULSE_SKL(c) (((c) - 1) << 5)
>> > > > >  #define   DP_AUX_CH_CTL_SYNC_PULSE_SKL(c)   ((c) - 1)
>> > > > >  
>> > > > > +#define _DPA_AUX_CH_MUTEX	(dev_priv-
>> > > > > > info.display_mmio_offset + 0x6402C)
>> > > > > 
>> > > > > +#define _DPB_AUX_CH_MUTEX	(dev_priv-
>> > > > > > info.display_mmio_offset + 0x6412C)
>> > > > > 
>> > > > > +#define _DPC_AUX_CH_MUTEX	(dev_priv-
>> > > > > > info.display_mmio_offset + 0x6422C)
>> > > > > 
>> > > > > +#define _DPD_AUX_CH_MUTEX	(dev_priv-
>> > > > > > info.display_mmio_offset + 0x6432C)
>> > > > > 
>> > > > > +#define _DPF_AUX_CH_MUTEX	(dev_priv-
>> > > > > > info.display_mmio_offset + 0x6452C)
>> > > > > 
>> > > > > +#define DP_AUX_CH_MUTEX(aux_ch)	_MMIO_PORT(aux_ch,
>> > > > > _DPA_AUX_CH_MUTEX, _DPB_AUX_CH_MUTEX)
>> > > > > +#define   DP_AUX_CH_MUTEX_ENABLE		(1 << 31)
>> > > > > +#define   DP_AUX_CH_MUTEX_STATUS		(1 << 30)
>> > > > > +
>> > > > >  /*
>> > > > >   * Computing GMCH M and N values for the Display Port link
>> > > > >   *
>> > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
>> > > > > b/drivers/gpu/drm/i915/intel_dp.c
>> > > > > index 2a3b3ae4e3da..7f4bf77227cd 100644
>> > > > > --- a/drivers/gpu/drm/i915/intel_dp.c
>> > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
>> > > > > @@ -1081,6 +1081,42 @@ static uint32_t
>> > > > > intel_dp_get_aux_send_ctl(struct intel_dp *intel_dp,
>> > > > >  						aux_clock_divi
>> > > > > der)
>> > > > > ;
>> > > > >  }
>> > > > >  
>> > > > > +static bool intel_dp_aux_ch_trylock(struct intel_dp *intel_dp)
>> > > > > +{
>> > > > > +	struct intel_digital_port *intel_dig_port =
>> > > > > dp_to_dig_port(intel_dp);
>> > > > > +	struct drm_i915_private *dev_priv =
>> > > > > +			to_i915(intel_dig_port-
>> > > > > >base.base.dev);
>> > > > > +
>> > > > > +	if (INTEL_GEN(dev_priv) < 9)
>> > > > > +		return true;
>> > > > > +
>> > > > > +	/* Spec says that mutex is acquired when status bit is
>> > > > > read as unset,
>> > > > > +	 * here waiting for 2msec(+-4 aux transactions) before
>> > > > > give up.
>> > > > > +	 */
>> > > > > +	if (intel_wait_for_register(dev_priv,
>> > > > > DP_AUX_CH_MUTEX(intel_dp->aux_ch),
>> > > > > +				    DP_AUX_CH_MUTEX_STATUS, 0,
>> > > > > 2))
>> > > > > {
>> > > > > +		DRM_DEBUG_KMS("aux channel %c locked for
>> > > > > 2msec,
>> > > > > timing out\n",
>> > > > > +			      aux_ch_name(intel_dp->aux_ch));
>> > > > > +		return false;
>> > > > > +	}
>> > > > > +
>> > > > > +	return true;
>> > > > > +}
>> > > > > +
>> > > > > +static void intel_dp_aux_ch_unlock(struct intel_dp *intel_dp)
>> > > > > +{
>> > > > > +	struct intel_digital_port *intel_dig_port =
>> > > > > dp_to_dig_port(intel_dp);
>> > > > > +	struct drm_i915_private *dev_priv =
>> > > > > +			to_i915(intel_dig_port-
>> > > > > >base.base.dev);
>> > > > > +
>> > > > > +	if (INTEL_GEN(dev_priv) < 9)
>> > > > > +		return;
>> > > > > +
>> > > > > +	/* set the status bit releases the mutex + keeping
>> > > > > mutex
>> > > > > enabled */
>> > > > > +	I915_WRITE(DP_AUX_CH_MUTEX(intel_dp->aux_ch),
>> > > > > +		   DP_AUX_CH_MUTEX_ENABLE |
>> > > > > DP_AUX_CH_MUTEX_STATUS);
>> > > > > +}
>> > > > > +
>> > > > >  static int
>> > > > >  intel_dp_aux_ch(struct intel_dp *intel_dp,
>> > > > >  		const uint8_t *send, int send_bytes,
>> > > > > @@ -1119,6 +1155,11 @@ intel_dp_aux_ch(struct intel_dp
>> > > > > *intel_dp,
>> > > > >  
>> > > > >  	intel_dp_check_edp(intel_dp);
>> > > > >  
>> > > > > +	if (!intel_dp_aux_ch_trylock(intel_dp)) {
>> > > > > +		ret = -EBUSY;
>> > > > > +		goto out_aux_ch_unlocked;
>> > > > > +	}
>> > > > > +
>> > > > >  	/* Try to wait for any previous AUX channel activity
>> > > > > */
>> > > > >  	for (try = 0; try < 3; try++) {
>> > > > >  		status = I915_READ_NOTRACE(ch_ctl);
>> > > > > @@ -1240,6 +1281,8 @@ intel_dp_aux_ch(struct intel_dp
>> > > > > *intel_dp,
>> > > > >  
>> > > > >  	ret = recv_bytes;
>> > > > >  out:
>> > > > > +	intel_dp_aux_ch_unlock(intel_dp);
>> > > > > +out_aux_ch_unlocked:
>> > > > >  	pm_qos_update_request(&dev_priv->pm_qos,
>> > > > > PM_QOS_DEFAULT_VALUE);
>> > > > >  
>> > > > >  	if (vdd)
>> > > > > @@ -1536,6 +1579,10 @@ intel_dp_aux_init(struct intel_dp
>> > > > > *intel_dp)
>> > > > >  	else
>> > > > >  		intel_dp->get_aux_send_ctl =
>> > > > > g4x_get_aux_send_ctl;
>> > > > >  
>> > > > > +	if (INTEL_GEN(dev_priv) >= 9)
>> > > > > +		I915_WRITE(DP_AUX_CH_MUTEX(intel_dp->aux_ch),
>> > > > > +			   DP_AUX_CH_MUTEX_ENABLE);
>> > > > 
>> > > > And who enables it after system/runtime PM etc.? Not sure how
>> > > > much
>> > > > sense
>> > > > there is to finesse this anyway. One extra mmio write every time
>> > > > we
>> > > > try
>> > > > to acquire the mutex shouldn't kill anyone.
>> > > 
>> > > Valid point, but I guess is better enable the mutex in
>> > > intel_dp_encoder_reset():
>> > > 
>> > > "
>> > > @@ -5293,6 +5340,10 @@ void intel_dp_encoder_reset(struct
>> > > drm_encoder
>> > > *encoder)
>> > > 
>> > >         pps_lock(intel_dp);
>> > > 
>> > > +       if (INTEL_GEN(dev_priv) >= 9)
>> > > +               I915_WRITE(DP_AUX_CH_MUTEX(intel_dp->aux_ch),
>> > > +                          DP_AUX_CH_MUTEX_ENABLE);
>> > > +
>> > >         if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
>> > >                 intel_dp->active_pipe = vlv_active_pipe(intel_dp);
>> > > "
>> > > 
>> > > any objections?
>> > 
>> > I guess we can hope that dmc takes care of it for dc5/6. But what
>> > about dc9?
>> 
>> Yep, for DC5 and DC6 the registers are restored by hardware but it is
>> not the case for DC9. If I'm not missing something looks like there is
>> no handling to restore registers when exiting DC9.
>
> The design of the driver is "program all required registers when you
> need them" instead of the "blindly save/restore everything" DMC firmware
> approach.

So, what do you suggest Ville?

Some function called form i915_drm_resume that loops on connectors
with active DP and enable its aux mutex?

But even on this case we would have the risk of unsetting the status
while hw can be using, no?

Or this or:

Option 2: Enable without RMW only on trylock and disabe without RMW on release.


Or (my favorite one) that follows spec line by line:

Option 3: Enable with RMW only on trylock and with RMW touch only Status
bit keeping it enabled.

I won't believe RMW would have any impact here because on trylock we are
anyways trying to lock so reading the status right after enabling.
Also there is no problem on RMW on release because right after reading
before writing we would be releasing it anyways.

So RMW is the simplified solution imho...

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

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

[Patchwork]

== Series Details ==

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

== Summary ==

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

---- Possible new issues:

Test kms_force_connector_basic:
        Subgroup force-connector-state:
                pass       -> SKIP       (fi-ivb-3520m)
        Subgroup force-edid:
                pass       -> SKIP       (fi-ivb-3520m)
        Subgroup force-load-detect:
                pass       -> SKIP       (fi-ivb-3520m)
        Subgroup prune-stale-modes:
                pass       -> SKIP       (fi-ivb-3520m)

---- Known issues:

Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-b:
                pass       -> DMESG-WARN (fi-cnl-y3) fdo#104951

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

fi-bdw-5557u     total:288  pass:267  dwarn:0   dfail:0   fail:0   skip:21  time:416s
fi-bdw-gvtdvm    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:421s
fi-blb-e6850     total:288  pass:223  dwarn:1   dfail:0   fail:0   skip:64  time:372s
fi-bsw-n3050     total:288  pass:242  dwarn:0   dfail:0   fail:0   skip:46  time:483s
fi-bwr-2160      total:288  pass:183  dwarn:0   dfail:0   fail:0   skip:105 time:284s
fi-bxt-dsi       total:288  pass:258  dwarn:0   dfail:0   fail:0   skip:30  time:476s
fi-bxt-j4205     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:481s
fi-byt-j1900     total:288  pass:253  dwarn:0   dfail:0   fail:0   skip:35  time:470s
fi-byt-n2820     total:288  pass:249  dwarn:0   dfail:0   fail:0   skip:39  time:451s
fi-cfl-8700k     total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:396s
fi-cfl-s2        total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:566s
fi-cnl-y3        total:288  pass:261  dwarn:1   dfail:0   fail:0   skip:26  time:567s
fi-elk-e7500     total:288  pass:229  dwarn:0   dfail:0   fail:0   skip:59  time:416s
fi-gdg-551       total:288  pass:179  dwarn:0   dfail:0   fail:1   skip:108 time:286s
fi-glk-1         total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:507s
fi-hsw-4770      total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:385s
fi-ilk-650       total:288  pass:228  dwarn:0   dfail:0   fail:0   skip:60  time:406s
fi-ivb-3520m     total:288  pass:255  dwarn:0   dfail:0   fail:0   skip:33  time:442s
fi-ivb-3770      total:288  pass:255  dwarn:0   dfail:0   fail:0   skip:33  time:409s
fi-kbl-7500u     total:288  pass:263  dwarn:1   dfail:0   fail:0   skip:24  time:451s
fi-kbl-7560u     total:288  pass:269  dwarn:0   dfail:0   fail:0   skip:19  time:493s
fi-kbl-7567u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:453s
fi-kbl-r         total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:493s
fi-pnv-d510      total:288  pass:222  dwarn:1   dfail:0   fail:0   skip:65  time:578s
fi-skl-6260u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:428s
fi-skl-6600u     total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:500s
fi-skl-6700hq    total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:517s
fi-skl-6700k2    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:481s
fi-skl-6770hq    total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:477s
fi-skl-guc       total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:405s
fi-skl-gvtdvm    total:288  pass:265  dwarn:0   dfail:0   fail:0   skip:23  time:431s
fi-snb-2520m     total:3    pass:2    dwarn:0   dfail:0   fail:0   skip:0  
fi-snb-2600      total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:391s

af8578c6d4389dd9a51ddccc43b8ce4986e27131 drm-tip: 2018y-02m-27d-20h-28m-22s UTC integration manifest
128fb513cbdd drm/i915/skl+: Add and enable DP AUX CH mutex

== Logs ==

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