* Re: [PATCH v2 1/1] drm/i915: Add and enable DP AUX CH mutex
2018-02-21 2:23 ` [PATCH v2 1/1] " José Roberto de Souza
@ 2018-02-21 8:59 ` Dhinakaran Pandiyan
2018-02-21 17:24 ` Souza, Jose
2018-02-21 20:45 ` Rodrigo Vivi
2018-02-22 18:59 ` Ville Syrjälä
2 siblings, 1 reply; 16+ messages in thread
From: Dhinakaran Pandiyan @ 2018-02-21 8:59 UTC (permalink / raw)
To: intel-gfx; +Cc: Rodrigo Vivi
On Tuesday, February 20, 2018 6:23:47 PM PST José Roberto de Souza wrote:
> When PSR/PSR2/GTC is enabled hardware can do AUX transactions by it
> self, so lets use the mutex register that is available in gen9+ to
> avoid concurrent access by hardware and driver.
>
> Reference:
> https://01.org/sites/default/files/documentation/intel-gfx-prm-osrc-skl-vol
> 12-display.pdf Page 198 - AUX programming sequence
>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
> drivers/gpu/drm/i915/i915_reg.h | 9 ++++++++
> drivers/gpu/drm/i915/intel_dp.c | 50
> ++++++++++++++++++++++++++++++++++++++++ drivers/gpu/drm/i915/intel_drv.h |
> 1 +
> 3 files changed, 60 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h
> b/drivers/gpu/drm/i915/i915_reg.h index 1412abcb27d4..a62e3c1badab 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -5318,6 +5318,7 @@ enum {
> #define _DPA_AUX_CH_DATA3 (dev_priv->info.display_mmio_offset + 0x6401c)
> #define _DPA_AUX_CH_DATA4 (dev_priv->info.display_mmio_offset + 0x64020)
> #define _DPA_AUX_CH_DATA5 (dev_priv->info.display_mmio_offset + 0x64024)
> +#define _DPA_AUX_CH_MUTEX (dev_priv->info.display_mmio_offset + 0x6402C)
>
> #define _DPB_AUX_CH_CTL (dev_priv->info.display_mmio_offset + 0x64110)
> #define _DPB_AUX_CH_DATA1 (dev_priv->info.display_mmio_offset + 0x64114)
> @@ -5325,6 +5326,7 @@ enum {
> #define _DPB_AUX_CH_DATA3 (dev_priv->info.display_mmio_offset + 0x6411c)
> #define _DPB_AUX_CH_DATA4 (dev_priv->info.display_mmio_offset + 0x64120)
> #define _DPB_AUX_CH_DATA5 (dev_priv->info.display_mmio_offset + 0x64124)
> +#define _DPB_AUX_CH_MUTEX (dev_priv->info.display_mmio_offset + 0x6412C)
>
> #define _DPC_AUX_CH_CTL (dev_priv->info.display_mmio_offset + 0x64210)
> #define _DPC_AUX_CH_DATA1 (dev_priv->info.display_mmio_offset + 0x64214)
> @@ -5332,6 +5334,7 @@ enum {
> #define _DPC_AUX_CH_DATA3 (dev_priv->info.display_mmio_offset + 0x6421c)
> #define _DPC_AUX_CH_DATA4 (dev_priv->info.display_mmio_offset + 0x64220)
> #define _DPC_AUX_CH_DATA5 (dev_priv->info.display_mmio_offset + 0x64224)
> +#define _DPC_AUX_CH_MUTEX (dev_priv->info.display_mmio_offset + 0x6422C)
>
> #define _DPD_AUX_CH_CTL (dev_priv->info.display_mmio_offset + 0x64310)
> #define _DPD_AUX_CH_DATA1 (dev_priv->info.display_mmio_offset + 0x64314)
> @@ -5339,6 +5342,7 @@ enum {
> #define _DPD_AUX_CH_DATA3 (dev_priv->info.display_mmio_offset + 0x6431c)
> #define _DPD_AUX_CH_DATA4 (dev_priv->info.display_mmio_offset + 0x64320)
> #define _DPD_AUX_CH_DATA5 (dev_priv->info.display_mmio_offset + 0x64324)
> +#define _DPD_AUX_CH_MUTEX (dev_priv->info.display_mmio_offset + 0x6432C)
>
> #define _DPF_AUX_CH_CTL (dev_priv->info.display_mmio_offset + 0x64510)
> #define _DPF_AUX_CH_DATA1 (dev_priv->info.display_mmio_offset + 0x64514)
> @@ -5346,6 +5350,7 @@ enum {
> #define _DPF_AUX_CH_DATA3 (dev_priv->info.display_mmio_offset + 0x6451c)
> #define _DPF_AUX_CH_DATA4 (dev_priv->info.display_mmio_offset + 0x64520)
> #define _DPF_AUX_CH_DATA5 (dev_priv->info.display_mmio_offset + 0x64524)
> +#define _DPF_AUX_CH_MUTEX (dev_priv->info.display_mmio_offset + 0x6452C)
>
> #define DP_AUX_CH_CTL(port) _MMIO_PORT(port, _DPA_AUX_CH_CTL,
> _DPB_AUX_CH_CTL) #define DP_AUX_CH_DATA(port, i) _MMIO(_PORT(port,
> _DPA_AUX_CH_DATA1, _DPB_AUX_CH_DATA1) + (i) * 4) /* 5 registers */ @@
> -5378,6 +5383,10 @@ enum {
> #define DP_AUX_CH_CTL_FW_SYNC_PULSE_SKL(c) (((c) - 1) << 5)
> #define DP_AUX_CH_CTL_SYNC_PULSE_SKL(c) ((c) - 1)
>
> +#define DP_AUX_CH_MUTEX(port) _MMIO_PORT(port, _DPA_AUX_CH_MUTEX,
> _DPB_AUX_CH_MUTEX) +#define DP_AUX_CH_MUTEX_ENABLE (1 << 31)
> +#define DP_AUX_CH_MUTEX_STATUS (1 << 30)
> +
> /*
> * Computing GMCH M and N values for the Display Port link
> *
> diff --git a/drivers/gpu/drm/i915/intel_dp.c
> b/drivers/gpu/drm/i915/intel_dp.c index f20b25f98e5a..af07563bafba 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1081,6 +1081,45 @@ static uint32_t intel_dp_get_aux_send_ctl(struct
> intel_dp *intel_dp, aux_clock_divider);
> }
>
> +static bool intel_dp_aux_ch_trylock(struct intel_dp *intel_dp)
> +{
> + struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> + struct drm_i915_private *dev_priv =
> + to_i915(intel_dig_port->base.base.dev);
> +
> + if (INTEL_GEN(dev_priv) < 9)
> + return true;
> +
> + I915_WRITE(intel_dp->aux_ch_mutex_reg, DP_AUX_CH_MUTEX_ENABLE);
> +
> + /* Spec says that mutex is acquired when status bit is read as unset,
> + * if set it should be read again after 500usec. Here waiting for
> + * 2msec or 4 tries before give up.
> + */
> + if (__intel_wait_for_register(dev_priv, intel_dp->aux_ch_mutex_reg,
> + DP_AUX_CH_MUTEX_STATUS, 0, 500, 2,
> + NULL)) {
> + DRM_WARN("dp_aux_ch port locked for too long");
> + return false;
> + }
> +
Jose,
Thanks for the patch. One of the reasons we wanted the mutex implemented was
PSR_STATUS hinted that the hardware was busy with AUX transactions when the
driver called psr_activate/psr_exit (from psr_flush or psr_invalidate) . Or
between psr_activate and psr_exit in the terminal mode. So my question is, are
you seeing any timeouts or mutex busyness with i915.enable_psr=1 and regular
laptop use.
-DK
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v2 1/1] drm/i915: Add and enable DP AUX CH mutex
2018-02-21 8:59 ` Dhinakaran Pandiyan
@ 2018-02-21 17:24 ` Souza, Jose
0 siblings, 0 replies; 16+ messages in thread
From: Souza, Jose @ 2018-02-21 17:24 UTC (permalink / raw)
To: intel-gfx, dhinakaran.pandiyan; +Cc: Vivi, Rodrigo
On Wed, 2018-02-21 at 00:59 -0800, Dhinakaran Pandiyan wrote:
> On Tuesday, February 20, 2018 6:23:47 PM PST José Roberto de Souza
> wrote:
> > When PSR/PSR2/GTC is enabled hardware can do AUX transactions by it
> > self, so lets use the mutex register that is available in gen9+ to
> > avoid concurrent access by hardware and driver.
> >
> > Reference:
> > https://01.org/sites/default/files/documentation/intel-gfx-prm-osrc
> > -skl-vol
> > 12-display.pdf Page 198 - AUX programming sequence
> >
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> > drivers/gpu/drm/i915/i915_reg.h | 9 ++++++++
> > drivers/gpu/drm/i915/intel_dp.c | 50
> > ++++++++++++++++++++++++++++++++++++++++
> > drivers/gpu/drm/i915/intel_drv.h |
> > 1 +
> > 3 files changed, 60 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > b/drivers/gpu/drm/i915/i915_reg.h index 1412abcb27d4..a62e3c1badab
> > 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -5318,6 +5318,7 @@ enum {
> > #define _DPA_AUX_CH_DATA3 (dev_priv-
> > >info.display_mmio_offset + 0x6401c)
> > #define _DPA_AUX_CH_DATA4 (dev_priv-
> > >info.display_mmio_offset + 0x64020)
> > #define _DPA_AUX_CH_DATA5 (dev_priv-
> > >info.display_mmio_offset + 0x64024)
> > +#define _DPA_AUX_CH_MUTEX (dev_priv-
> > >info.display_mmio_offset + 0x6402C)
> >
> > #define _DPB_AUX_CH_CTL (dev_priv-
> > >info.display_mmio_offset + 0x64110)
> > #define _DPB_AUX_CH_DATA1 (dev_priv-
> > >info.display_mmio_offset + 0x64114)
> > @@ -5325,6 +5326,7 @@ enum {
> > #define _DPB_AUX_CH_DATA3 (dev_priv-
> > >info.display_mmio_offset + 0x6411c)
> > #define _DPB_AUX_CH_DATA4 (dev_priv-
> > >info.display_mmio_offset + 0x64120)
> > #define _DPB_AUX_CH_DATA5 (dev_priv-
> > >info.display_mmio_offset + 0x64124)
> > +#define _DPB_AUX_CH_MUTEX (dev_priv-
> > >info.display_mmio_offset + 0x6412C)
> >
> > #define _DPC_AUX_CH_CTL (dev_priv-
> > >info.display_mmio_offset + 0x64210)
> > #define _DPC_AUX_CH_DATA1 (dev_priv-
> > >info.display_mmio_offset + 0x64214)
> > @@ -5332,6 +5334,7 @@ enum {
> > #define _DPC_AUX_CH_DATA3 (dev_priv-
> > >info.display_mmio_offset + 0x6421c)
> > #define _DPC_AUX_CH_DATA4 (dev_priv-
> > >info.display_mmio_offset + 0x64220)
> > #define _DPC_AUX_CH_DATA5 (dev_priv-
> > >info.display_mmio_offset + 0x64224)
> > +#define _DPC_AUX_CH_MUTEX (dev_priv-
> > >info.display_mmio_offset + 0x6422C)
> >
> > #define _DPD_AUX_CH_CTL (dev_priv-
> > >info.display_mmio_offset + 0x64310)
> > #define _DPD_AUX_CH_DATA1 (dev_priv-
> > >info.display_mmio_offset + 0x64314)
> > @@ -5339,6 +5342,7 @@ enum {
> > #define _DPD_AUX_CH_DATA3 (dev_priv-
> > >info.display_mmio_offset + 0x6431c)
> > #define _DPD_AUX_CH_DATA4 (dev_priv-
> > >info.display_mmio_offset + 0x64320)
> > #define _DPD_AUX_CH_DATA5 (dev_priv-
> > >info.display_mmio_offset + 0x64324)
> > +#define _DPD_AUX_CH_MUTEX (dev_priv-
> > >info.display_mmio_offset + 0x6432C)
> >
> > #define _DPF_AUX_CH_CTL (dev_priv-
> > >info.display_mmio_offset + 0x64510)
> > #define _DPF_AUX_CH_DATA1 (dev_priv-
> > >info.display_mmio_offset + 0x64514)
> > @@ -5346,6 +5350,7 @@ enum {
> > #define _DPF_AUX_CH_DATA3 (dev_priv-
> > >info.display_mmio_offset + 0x6451c)
> > #define _DPF_AUX_CH_DATA4 (dev_priv-
> > >info.display_mmio_offset + 0x64520)
> > #define _DPF_AUX_CH_DATA5 (dev_priv-
> > >info.display_mmio_offset + 0x64524)
> > +#define _DPF_AUX_CH_MUTEX (dev_priv-
> > >info.display_mmio_offset + 0x6452C)
> >
> > #define DP_AUX_CH_CTL(port) _MMIO_PORT(port,
> > _DPA_AUX_CH_CTL,
> > _DPB_AUX_CH_CTL) #define DP_AUX_CH_DATA(port, i) _MMIO(_PORT
> > (port,
> > _DPA_AUX_CH_DATA1, _DPB_AUX_CH_DATA1) + (i) * 4) /* 5 registers */
> > @@
> > -5378,6 +5383,10 @@ enum {
> > #define DP_AUX_CH_CTL_FW_SYNC_PULSE_SKL(c) (((c) - 1) << 5)
> > #define DP_AUX_CH_CTL_SYNC_PULSE_SKL(c) ((c) - 1)
> >
> > +#define DP_AUX_CH_MUTEX(port) _MMIO_PORT(port,
> > _DPA_AUX_CH_MUTEX,
> > _DPB_AUX_CH_MUTEX) +#define DP_AUX_CH_MUTEX_ENABLE
> > (1 << 31)
> > +#define DP_AUX_CH_MUTEX_STATUS (1 << 30)
> > +
> > /*
> > * Computing GMCH M and N values for the Display Port link
> > *
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > b/drivers/gpu/drm/i915/intel_dp.c index f20b25f98e5a..af07563bafba
> > 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -1081,6 +1081,45 @@ static uint32_t
> > intel_dp_get_aux_send_ctl(struct
> > intel_dp *intel_dp, aux_clock_divider);
> > }
> >
> > +static bool intel_dp_aux_ch_trylock(struct intel_dp *intel_dp)
> > +{
> > + struct intel_digital_port *intel_dig_port =
> > dp_to_dig_port(intel_dp);
> > + struct drm_i915_private *dev_priv =
> > + to_i915(intel_dig_port->base.base.dev);
> > +
> > + if (INTEL_GEN(dev_priv) < 9)
> > + return true;
> > +
> > + I915_WRITE(intel_dp->aux_ch_mutex_reg,
> > DP_AUX_CH_MUTEX_ENABLE);
> > +
> > + /* Spec says that mutex is acquired when status bit is
> > read as unset,
> > + * if set it should be read again after 500usec. Here
> > waiting for
> > + * 2msec or 4 tries before give up.
> > + */
> > + if (__intel_wait_for_register(dev_priv, intel_dp-
> > >aux_ch_mutex_reg,
> > + DP_AUX_CH_MUTEX_STATUS, 0,
> > 500, 2,
> > + NULL)) {
> > + DRM_WARN("dp_aux_ch port locked for too long");
> > + return false;
> > + }
> > +
>
> Jose,
>
> Thanks for the patch. One of the reasons we wanted the mutex
> implemented was
> PSR_STATUS hinted that the hardware was busy with AUX transactions
> when the
> driver called psr_activate/psr_exit (from psr_flush or
> psr_invalidate) . Or
> between psr_activate and psr_exit in the terminal mode. So my
> question is, are
> you seeing any timeouts or mutex busyness with i915.enable_psr=1 and
> regular
> laptop use.
With regular use not, I'm able to see mutex locked by hardware but to
see that I had to add a few aux ch transactions before and after
exit/active PSR.
>
> -DK
>
>
>
>
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/1] drm/i915: Add and enable DP AUX CH mutex
2018-02-21 2:23 ` [PATCH v2 1/1] " José Roberto de Souza
2018-02-21 8:59 ` Dhinakaran Pandiyan
@ 2018-02-21 20:45 ` Rodrigo Vivi
2018-02-21 21:12 ` Souza, Jose
2018-02-22 18:59 ` Ville Syrjälä
2 siblings, 1 reply; 16+ messages in thread
From: Rodrigo Vivi @ 2018-02-21 20:45 UTC (permalink / raw)
To: José Roberto de Souza
Cc: intel-gfx, Ville Syrjälä, Pandiyan, Dhinakaran
On Tue, Feb 20, 2018 at 06:23:47PM -0800, José Roberto de Souza wrote:
> When PSR/PSR2/GTC is enabled hardware can do AUX transactions by it
> self, so lets use the mutex register that is available in gen9+ to
> avoid concurrent access by hardware and driver.
>
> Reference: https://01.org/sites/default/files/documentation/intel-gfx-prm-osrc-skl-vol12-display.pdf
> Page 198 - AUX programming sequence
>
Cc: Ville
Cc: DK
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
> drivers/gpu/drm/i915/i915_reg.h | 9 ++++++++
> drivers/gpu/drm/i915/intel_dp.c | 50 ++++++++++++++++++++++++++++++++++++++++
> drivers/gpu/drm/i915/intel_drv.h | 1 +
> 3 files changed, 60 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 1412abcb27d4..a62e3c1badab 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -5318,6 +5318,7 @@ enum {
> #define _DPA_AUX_CH_DATA3 (dev_priv->info.display_mmio_offset + 0x6401c)
> #define _DPA_AUX_CH_DATA4 (dev_priv->info.display_mmio_offset + 0x64020)
> #define _DPA_AUX_CH_DATA5 (dev_priv->info.display_mmio_offset + 0x64024)
> +#define _DPA_AUX_CH_MUTEX (dev_priv->info.display_mmio_offset + 0x6402C)
>
> #define _DPB_AUX_CH_CTL (dev_priv->info.display_mmio_offset + 0x64110)
> #define _DPB_AUX_CH_DATA1 (dev_priv->info.display_mmio_offset + 0x64114)
> @@ -5325,6 +5326,7 @@ enum {
> #define _DPB_AUX_CH_DATA3 (dev_priv->info.display_mmio_offset + 0x6411c)
> #define _DPB_AUX_CH_DATA4 (dev_priv->info.display_mmio_offset + 0x64120)
> #define _DPB_AUX_CH_DATA5 (dev_priv->info.display_mmio_offset + 0x64124)
> +#define _DPB_AUX_CH_MUTEX (dev_priv->info.display_mmio_offset + 0x6412C)
>
> #define _DPC_AUX_CH_CTL (dev_priv->info.display_mmio_offset + 0x64210)
> #define _DPC_AUX_CH_DATA1 (dev_priv->info.display_mmio_offset + 0x64214)
> @@ -5332,6 +5334,7 @@ enum {
> #define _DPC_AUX_CH_DATA3 (dev_priv->info.display_mmio_offset + 0x6421c)
> #define _DPC_AUX_CH_DATA4 (dev_priv->info.display_mmio_offset + 0x64220)
> #define _DPC_AUX_CH_DATA5 (dev_priv->info.display_mmio_offset + 0x64224)
> +#define _DPC_AUX_CH_MUTEX (dev_priv->info.display_mmio_offset + 0x6422C)
>
> #define _DPD_AUX_CH_CTL (dev_priv->info.display_mmio_offset + 0x64310)
> #define _DPD_AUX_CH_DATA1 (dev_priv->info.display_mmio_offset + 0x64314)
> @@ -5339,6 +5342,7 @@ enum {
> #define _DPD_AUX_CH_DATA3 (dev_priv->info.display_mmio_offset + 0x6431c)
> #define _DPD_AUX_CH_DATA4 (dev_priv->info.display_mmio_offset + 0x64320)
> #define _DPD_AUX_CH_DATA5 (dev_priv->info.display_mmio_offset + 0x64324)
> +#define _DPD_AUX_CH_MUTEX (dev_priv->info.display_mmio_offset + 0x6432C)
>
> #define _DPF_AUX_CH_CTL (dev_priv->info.display_mmio_offset + 0x64510)
> #define _DPF_AUX_CH_DATA1 (dev_priv->info.display_mmio_offset + 0x64514)
> @@ -5346,6 +5350,7 @@ enum {
> #define _DPF_AUX_CH_DATA3 (dev_priv->info.display_mmio_offset + 0x6451c)
> #define _DPF_AUX_CH_DATA4 (dev_priv->info.display_mmio_offset + 0x64520)
> #define _DPF_AUX_CH_DATA5 (dev_priv->info.display_mmio_offset + 0x64524)
> +#define _DPF_AUX_CH_MUTEX (dev_priv->info.display_mmio_offset + 0x6452C)
>
> #define DP_AUX_CH_CTL(port) _MMIO_PORT(port, _DPA_AUX_CH_CTL, _DPB_AUX_CH_CTL)
> #define DP_AUX_CH_DATA(port, i) _MMIO(_PORT(port, _DPA_AUX_CH_DATA1, _DPB_AUX_CH_DATA1) + (i) * 4) /* 5 registers */
> @@ -5378,6 +5383,10 @@ enum {
> #define DP_AUX_CH_CTL_FW_SYNC_PULSE_SKL(c) (((c) - 1) << 5)
> #define DP_AUX_CH_CTL_SYNC_PULSE_SKL(c) ((c) - 1)
>
I believe _DPA_AUX_CH_MUTEX and _DPB_AUX_CH_MUTEX should be defined here along
with the bits as Jani suggested.
I don't see any reason to tag that far from the bits and close to CTL and DATA.
> +#define DP_AUX_CH_MUTEX(port) _MMIO_PORT(port, _DPA_AUX_CH_MUTEX, _DPB_AUX_CH_MUTEX)
> +#define DP_AUX_CH_MUTEX_ENABLE (1 << 31)
> +#define DP_AUX_CH_MUTEX_STATUS (1 << 30)
> +
> /*
> * Computing GMCH M and N values for the Display Port link
> *
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index f20b25f98e5a..af07563bafba 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1081,6 +1081,45 @@ static uint32_t intel_dp_get_aux_send_ctl(struct intel_dp *intel_dp,
> aux_clock_divider);
> }
>
> +static bool intel_dp_aux_ch_trylock(struct intel_dp *intel_dp)
> +{
> + struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> + struct drm_i915_private *dev_priv =
> + to_i915(intel_dig_port->base.base.dev);
> +
> + if (INTEL_GEN(dev_priv) < 9)
> + return true;
> +
> + I915_WRITE(intel_dp->aux_ch_mutex_reg, DP_AUX_CH_MUTEX_ENABLE);
You might be touching bits. We don't know if HW is using the reserved bits or not.
So RMW |= bit 31 here is a good idea.
> +
> + /* Spec says that mutex is acquired when status bit is read as unset,
> + * if set it should be read again after 500usec. Here waiting for
> + * 2msec or 4 tries before give up.
> + */
> + if (__intel_wait_for_register(dev_priv, intel_dp->aux_ch_mutex_reg,
> + DP_AUX_CH_MUTEX_STATUS, 0, 500, 2,
> + NULL)) {
why not intel_wait_for_register?
that seems cleaner...
> + DRM_WARN("dp_aux_ch port locked for too long");
> + return false;
> + }
> +
> + return true;
> +}
> +
> +static void intel_dp_aux_ch_unlock(struct intel_dp *intel_dp)
> +{
> + struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> + struct drm_i915_private *dev_priv =
> + to_i915(intel_dig_port->base.base.dev);
> +
> + if (INTEL_GEN(dev_priv) < 9)
> + return;
> +
> + /* setting the status bit releases the mutex + keep mutex enabled */
> + I915_WRITE(intel_dp->aux_ch_mutex_reg, DP_AUX_CH_MUTEX_ENABLE |
> + DP_AUX_CH_MUTEX_STATUS);
ditto.
> +}
> +
> static int
> intel_dp_aux_ch(struct intel_dp *intel_dp,
> const uint8_t *send, int send_bytes,
> @@ -1115,6 +1154,11 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
>
> intel_dp_check_edp(intel_dp);
>
> + if (!intel_dp_aux_ch_trylock(intel_dp)) {
> + ret = -EBUSY;
> + goto out_locked;
> + }
> +
BSpec: 4301
I believe you could reduce the lock region following the spec
and limiting it to steps 5-to-10, i.e.
inside the loop below.
> /* Try to wait for any previous AUX channel activity */
> for (try = 0; try < 3; try++) {
> status = I915_READ_NOTRACE(ch_ctl);
> @@ -1244,6 +1288,8 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
>
> ret = recv_bytes;
> out:
> + intel_dp_aux_ch_unlock(intel_dp);
> +out_locked:
> pm_qos_update_request(&dev_priv->pm_qos, PM_QOS_DEFAULT_VALUE);
>
> if (vdd)
> @@ -1496,6 +1542,10 @@ static void intel_aux_reg_init(struct intel_dp *intel_dp)
> intel_dp->aux_ch_ctl_reg = intel_aux_ctl_reg(dev_priv, port);
> for (i = 0; i < ARRAY_SIZE(intel_dp->aux_ch_data_reg); i++)
> intel_dp->aux_ch_data_reg[i] = intel_aux_data_reg(dev_priv, port, i);
> + if (INTEL_INFO(dev_priv)->gen >= 9) {
> + intel_dp->aux_ch_mutex_reg = DP_AUX_CH_MUTEX(port);
> + I915_WRITE(intel_dp->aux_ch_mutex_reg, DP_AUX_CH_MUTEX_ENABLE);
I don't believe we need an extra enable here. And hopefully with Villes
rework we don't even need to define here but only accessing it directly from
the aux function with intel_dp->aux_ch reference.
> + }
> }
>
> static void
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 50874f4035cf..0c88657d6066 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1043,6 +1043,7 @@ struct intel_dp {
> i915_reg_t output_reg;
> i915_reg_t aux_ch_ctl_reg;
> i915_reg_t aux_ch_data_reg[5];
> + i915_reg_t aux_ch_mutex_reg;
> uint32_t DP;
> int link_rate;
> uint8_t lane_count;
> --
> 2.16.2
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v2 1/1] drm/i915: Add and enable DP AUX CH mutex
2018-02-21 20:45 ` Rodrigo Vivi
@ 2018-02-21 21:12 ` Souza, Jose
2018-02-21 22:28 ` Rodrigo Vivi
0 siblings, 1 reply; 16+ messages in thread
From: Souza, Jose @ 2018-02-21 21:12 UTC (permalink / raw)
To: Vivi, Rodrigo; +Cc: intel-gfx, Syrjala, Ville, Pandiyan, Dhinakaran
On Wed, 2018-02-21 at 12:45 -0800, Rodrigo Vivi wrote:
> On Tue, Feb 20, 2018 at 06:23:47PM -0800, José Roberto de Souza
> wrote:
> > When PSR/PSR2/GTC is enabled hardware can do AUX transactions by it
> > self, so lets use the mutex register that is available in gen9+ to
> > avoid concurrent access by hardware and driver.
> >
> > Reference: https://01.org/sites/default/files/documentation/intel-g
> > fx-prm-osrc-skl-vol12-display.pdf
> > Page 198 - AUX programming sequence
> >
>
> Cc: Ville
> Cc: DK
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> > drivers/gpu/drm/i915/i915_reg.h | 9 ++++++++
> > drivers/gpu/drm/i915/intel_dp.c | 50
> > ++++++++++++++++++++++++++++++++++++++++
> > drivers/gpu/drm/i915/intel_drv.h | 1 +
> > 3 files changed, 60 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > b/drivers/gpu/drm/i915/i915_reg.h
> > index 1412abcb27d4..a62e3c1badab 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -5318,6 +5318,7 @@ enum {
> > #define _DPA_AUX_CH_DATA3 (dev_priv-
> > >info.display_mmio_offset + 0x6401c)
> > #define _DPA_AUX_CH_DATA4 (dev_priv-
> > >info.display_mmio_offset + 0x64020)
> > #define _DPA_AUX_CH_DATA5 (dev_priv-
> > >info.display_mmio_offset + 0x64024)
> > +#define _DPA_AUX_CH_MUTEX (dev_priv-
> > >info.display_mmio_offset + 0x6402C)
> >
> > #define _DPB_AUX_CH_CTL (dev_priv-
> > >info.display_mmio_offset + 0x64110)
> > #define _DPB_AUX_CH_DATA1 (dev_priv-
> > >info.display_mmio_offset + 0x64114)
> > @@ -5325,6 +5326,7 @@ enum {
> > #define _DPB_AUX_CH_DATA3 (dev_priv-
> > >info.display_mmio_offset + 0x6411c)
> > #define _DPB_AUX_CH_DATA4 (dev_priv-
> > >info.display_mmio_offset + 0x64120)
> > #define _DPB_AUX_CH_DATA5 (dev_priv-
> > >info.display_mmio_offset + 0x64124)
> > +#define _DPB_AUX_CH_MUTEX (dev_priv-
> > >info.display_mmio_offset + 0x6412C)
> >
> > #define _DPC_AUX_CH_CTL (dev_priv-
> > >info.display_mmio_offset + 0x64210)
> > #define _DPC_AUX_CH_DATA1 (dev_priv-
> > >info.display_mmio_offset + 0x64214)
> > @@ -5332,6 +5334,7 @@ enum {
> > #define _DPC_AUX_CH_DATA3 (dev_priv-
> > >info.display_mmio_offset + 0x6421c)
> > #define _DPC_AUX_CH_DATA4 (dev_priv-
> > >info.display_mmio_offset + 0x64220)
> > #define _DPC_AUX_CH_DATA5 (dev_priv-
> > >info.display_mmio_offset + 0x64224)
> > +#define _DPC_AUX_CH_MUTEX (dev_priv-
> > >info.display_mmio_offset + 0x6422C)
> >
> > #define _DPD_AUX_CH_CTL (dev_priv-
> > >info.display_mmio_offset + 0x64310)
> > #define _DPD_AUX_CH_DATA1 (dev_priv-
> > >info.display_mmio_offset + 0x64314)
> > @@ -5339,6 +5342,7 @@ enum {
> > #define _DPD_AUX_CH_DATA3 (dev_priv-
> > >info.display_mmio_offset + 0x6431c)
> > #define _DPD_AUX_CH_DATA4 (dev_priv-
> > >info.display_mmio_offset + 0x64320)
> > #define _DPD_AUX_CH_DATA5 (dev_priv-
> > >info.display_mmio_offset + 0x64324)
> > +#define _DPD_AUX_CH_MUTEX (dev_priv-
> > >info.display_mmio_offset + 0x6432C)
> >
> > #define _DPF_AUX_CH_CTL (dev_priv-
> > >info.display_mmio_offset + 0x64510)
> > #define _DPF_AUX_CH_DATA1 (dev_priv-
> > >info.display_mmio_offset + 0x64514)
> > @@ -5346,6 +5350,7 @@ enum {
> > #define _DPF_AUX_CH_DATA3 (dev_priv-
> > >info.display_mmio_offset + 0x6451c)
> > #define _DPF_AUX_CH_DATA4 (dev_priv-
> > >info.display_mmio_offset + 0x64520)
> > #define _DPF_AUX_CH_DATA5 (dev_priv-
> > >info.display_mmio_offset + 0x64524)
> > +#define _DPF_AUX_CH_MUTEX (dev_priv-
> > >info.display_mmio_offset + 0x6452C)
> >
> > #define DP_AUX_CH_CTL(port) _MMIO_PORT(port,
> > _DPA_AUX_CH_CTL, _DPB_AUX_CH_CTL)
> > #define DP_AUX_CH_DATA(port, i) _MMIO(_PORT(port,
> > _DPA_AUX_CH_DATA1, _DPB_AUX_CH_DATA1) + (i) * 4) /* 5 registers */
> > @@ -5378,6 +5383,10 @@ enum {
> > #define DP_AUX_CH_CTL_FW_SYNC_PULSE_SKL(c) (((c) - 1) << 5)
> > #define DP_AUX_CH_CTL_SYNC_PULSE_SKL(c) ((c) - 1)
> >
>
>
> I believe _DPA_AUX_CH_MUTEX and _DPB_AUX_CH_MUTEX should be defined
> here along
> with the bits as Jani suggested.
> I don't see any reason to tag that far from the bits and close to CTL
> and DATA.
>
> > +#define DP_AUX_CH_MUTEX(port) _MMIO_PORT(port,
> > _DPA_AUX_CH_MUTEX, _DPB_AUX_CH_MUTEX)
> > +#define DP_AUX_CH_MUTEX_ENABLE (1 << 31)
> > +#define DP_AUX_CH_MUTEX_STATUS (1 << 30)
> > +
> > /*
> > * Computing GMCH M and N values for the Display Port link
> > *
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > b/drivers/gpu/drm/i915/intel_dp.c
> > index f20b25f98e5a..af07563bafba 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -1081,6 +1081,45 @@ static uint32_t
> > intel_dp_get_aux_send_ctl(struct intel_dp *intel_dp,
> > aux_clock_divider)
> > ;
> > }
> >
> > +static bool intel_dp_aux_ch_trylock(struct intel_dp *intel_dp)
> > +{
> > + struct intel_digital_port *intel_dig_port =
> > dp_to_dig_port(intel_dp);
> > + struct drm_i915_private *dev_priv =
> > + to_i915(intel_dig_port->base.base.dev);
> > +
> > + if (INTEL_GEN(dev_priv) < 9)
> > + return true;
> > +
> > + I915_WRITE(intel_dp->aux_ch_mutex_reg,
> > DP_AUX_CH_MUTEX_ENABLE);
>
> You might be touching bits. We don't know if HW is using the reserved
> bits or not.
> So RMW |= bit 31 here is a good idea.
As a read in this register request the mutex lock is better avoid any
read that is not meant to request it.
>
> > +
> > + /* Spec says that mutex is acquired when status bit is
> > read as unset,
> > + * if set it should be read again after 500usec. Here
> > waiting for
> > + * 2msec or 4 tries before give up.
> > + */
> > + if (__intel_wait_for_register(dev_priv, intel_dp-
> > >aux_ch_mutex_reg,
> > + DP_AUX_CH_MUTEX_STATUS, 0,
> > 500, 2,
> > + NULL)) {
>
> why not intel_wait_for_register?
> that seems cleaner...
intel_wait_for_register() will call __intel_wait_for_register() with
fast_timeout_us set as 2usec and specs says we should wait 500usec.
>
> > + DRM_WARN("dp_aux_ch port locked for too long");
> > + return false;
> > + }
> > +
> > + return true;
> > +}
> > +
> > +static void intel_dp_aux_ch_unlock(struct intel_dp *intel_dp)
> > +{
> > + struct intel_digital_port *intel_dig_port =
> > dp_to_dig_port(intel_dp);
> > + struct drm_i915_private *dev_priv =
> > + to_i915(intel_dig_port->base.base.dev);
> > +
> > + if (INTEL_GEN(dev_priv) < 9)
> > + return;
> > +
> > + /* setting the status bit releases the mutex + keep mutex
> > enabled */
> > + I915_WRITE(intel_dp->aux_ch_mutex_reg,
> > DP_AUX_CH_MUTEX_ENABLE |
> > + DP_AUX_CH_MUTEX_STATUS);
>
> ditto.
>
> > +}
> > +
> > static int
> > intel_dp_aux_ch(struct intel_dp *intel_dp,
> > const uint8_t *send, int send_bytes,
> > @@ -1115,6 +1154,11 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
> >
> > intel_dp_check_edp(intel_dp);
> >
> > + if (!intel_dp_aux_ch_trylock(intel_dp)) {
> > + ret = -EBUSY;
> > + goto out_locked;
> > + }
> > +
>
> BSpec: 4301
> I believe you could reduce the lock region following the spec
> and limiting it to steps 5-to-10, i.e.
> inside the loop below.
We loop trying to complete the transaction but we copy data received
out of the loop and that is step 9, so the lock must be held until we
copy the data from intel_dp->aux_ch_data_reg[] to recv.
"
for (i = 0; i < recv_bytes; i += 4)
intel_dp_unpack_aux(I915_READ(intel_dp->aux_ch_data_reg[i >>
2]), recv + i, recv_bytes - i);
ret = recv_bytes;
out:
intel_dp_aux_ch_unlock(intel_dp);
"
>
> > /* Try to wait for any previous AUX channel activity */
> > for (try = 0; try < 3; try++) {
> > status = I915_READ_NOTRACE(ch_ctl);
> > @@ -1244,6 +1288,8 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
> >
> > ret = recv_bytes;
> > out:
> > + intel_dp_aux_ch_unlock(intel_dp);
> > +out_locked:
> > pm_qos_update_request(&dev_priv->pm_qos,
> > PM_QOS_DEFAULT_VALUE);
> >
> > if (vdd)
> > @@ -1496,6 +1542,10 @@ static void intel_aux_reg_init(struct
> > intel_dp *intel_dp)
> > intel_dp->aux_ch_ctl_reg = intel_aux_ctl_reg(dev_priv,
> > port);
> > for (i = 0; i < ARRAY_SIZE(intel_dp->aux_ch_data_reg);
> > i++)
> > intel_dp->aux_ch_data_reg[i] =
> > intel_aux_data_reg(dev_priv, port, i);
> > + if (INTEL_INFO(dev_priv)->gen >= 9) {
> > + intel_dp->aux_ch_mutex_reg =
> > DP_AUX_CH_MUTEX(port);
> > + I915_WRITE(intel_dp->aux_ch_mutex_reg,
> > DP_AUX_CH_MUTEX_ENABLE);
>
> I don't believe we need an extra enable here. And hopefully with
> Villes
> rework we don't even need to define here but only accessing it
> directly from
> the aux function with intel_dp->aux_ch reference.
What if hardware wants to do some aux transaction before we do our
first transaction(and enabling the mutex)? The mutex will not be
enabled so hardware will not lock it.
>
> > + }
> > }
> >
> > static void
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > b/drivers/gpu/drm/i915/intel_drv.h
> > index 50874f4035cf..0c88657d6066 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1043,6 +1043,7 @@ struct intel_dp {
> > i915_reg_t output_reg;
> > i915_reg_t aux_ch_ctl_reg;
> > i915_reg_t aux_ch_data_reg[5];
> > + i915_reg_t aux_ch_mutex_reg;
> > uint32_t DP;
> > int link_rate;
> > uint8_t lane_count;
> > --
> > 2.16.2
> >
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v2 1/1] drm/i915: Add and enable DP AUX CH mutex
2018-02-21 21:12 ` Souza, Jose
@ 2018-02-21 22:28 ` Rodrigo Vivi
2018-02-23 1:13 ` Souza, Jose
0 siblings, 1 reply; 16+ messages in thread
From: Rodrigo Vivi @ 2018-02-21 22:28 UTC (permalink / raw)
To: Souza, Jose; +Cc: intel-gfx, Syrjala, Ville, Pandiyan, Dhinakaran
On Wed, Feb 21, 2018 at 01:12:08PM -0800, Souza, Jose wrote:
> On Wed, 2018-02-21 at 12:45 -0800, Rodrigo Vivi wrote:
> > On Tue, Feb 20, 2018 at 06:23:47PM -0800, José Roberto de Souza
> > wrote:
> > > When PSR/PSR2/GTC is enabled hardware can do AUX transactions by it
> > > self, so lets use the mutex register that is available in gen9+ to
> > > avoid concurrent access by hardware and driver.
> > >
> > > Reference: https://01.org/sites/default/files/documentation/intel-g
> > > fx-prm-osrc-skl-vol12-display.pdf
> > > Page 198 - AUX programming sequence
> > >
> >
> > Cc: Ville
> > Cc: DK
> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > ---
> > > drivers/gpu/drm/i915/i915_reg.h | 9 ++++++++
> > > drivers/gpu/drm/i915/intel_dp.c | 50
> > > ++++++++++++++++++++++++++++++++++++++++
> > > drivers/gpu/drm/i915/intel_drv.h | 1 +
> > > 3 files changed, 60 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > > b/drivers/gpu/drm/i915/i915_reg.h
> > > index 1412abcb27d4..a62e3c1badab 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -5318,6 +5318,7 @@ enum {
> > > #define _DPA_AUX_CH_DATA3 (dev_priv-
> > > >info.display_mmio_offset + 0x6401c)
> > > #define _DPA_AUX_CH_DATA4 (dev_priv-
> > > >info.display_mmio_offset + 0x64020)
> > > #define _DPA_AUX_CH_DATA5 (dev_priv-
> > > >info.display_mmio_offset + 0x64024)
> > > +#define _DPA_AUX_CH_MUTEX (dev_priv-
> > > >info.display_mmio_offset + 0x6402C)
> > >
> > > #define _DPB_AUX_CH_CTL (dev_priv-
> > > >info.display_mmio_offset + 0x64110)
> > > #define _DPB_AUX_CH_DATA1 (dev_priv-
> > > >info.display_mmio_offset + 0x64114)
> > > @@ -5325,6 +5326,7 @@ enum {
> > > #define _DPB_AUX_CH_DATA3 (dev_priv-
> > > >info.display_mmio_offset + 0x6411c)
> > > #define _DPB_AUX_CH_DATA4 (dev_priv-
> > > >info.display_mmio_offset + 0x64120)
> > > #define _DPB_AUX_CH_DATA5 (dev_priv-
> > > >info.display_mmio_offset + 0x64124)
> > > +#define _DPB_AUX_CH_MUTEX (dev_priv-
> > > >info.display_mmio_offset + 0x6412C)
> > >
> > > #define _DPC_AUX_CH_CTL (dev_priv-
> > > >info.display_mmio_offset + 0x64210)
> > > #define _DPC_AUX_CH_DATA1 (dev_priv-
> > > >info.display_mmio_offset + 0x64214)
> > > @@ -5332,6 +5334,7 @@ enum {
> > > #define _DPC_AUX_CH_DATA3 (dev_priv-
> > > >info.display_mmio_offset + 0x6421c)
> > > #define _DPC_AUX_CH_DATA4 (dev_priv-
> > > >info.display_mmio_offset + 0x64220)
> > > #define _DPC_AUX_CH_DATA5 (dev_priv-
> > > >info.display_mmio_offset + 0x64224)
> > > +#define _DPC_AUX_CH_MUTEX (dev_priv-
> > > >info.display_mmio_offset + 0x6422C)
> > >
> > > #define _DPD_AUX_CH_CTL (dev_priv-
> > > >info.display_mmio_offset + 0x64310)
> > > #define _DPD_AUX_CH_DATA1 (dev_priv-
> > > >info.display_mmio_offset + 0x64314)
> > > @@ -5339,6 +5342,7 @@ enum {
> > > #define _DPD_AUX_CH_DATA3 (dev_priv-
> > > >info.display_mmio_offset + 0x6431c)
> > > #define _DPD_AUX_CH_DATA4 (dev_priv-
> > > >info.display_mmio_offset + 0x64320)
> > > #define _DPD_AUX_CH_DATA5 (dev_priv-
> > > >info.display_mmio_offset + 0x64324)
> > > +#define _DPD_AUX_CH_MUTEX (dev_priv-
> > > >info.display_mmio_offset + 0x6432C)
> > >
> > > #define _DPF_AUX_CH_CTL (dev_priv-
> > > >info.display_mmio_offset + 0x64510)
> > > #define _DPF_AUX_CH_DATA1 (dev_priv-
> > > >info.display_mmio_offset + 0x64514)
> > > @@ -5346,6 +5350,7 @@ enum {
> > > #define _DPF_AUX_CH_DATA3 (dev_priv-
> > > >info.display_mmio_offset + 0x6451c)
> > > #define _DPF_AUX_CH_DATA4 (dev_priv-
> > > >info.display_mmio_offset + 0x64520)
> > > #define _DPF_AUX_CH_DATA5 (dev_priv-
> > > >info.display_mmio_offset + 0x64524)
> > > +#define _DPF_AUX_CH_MUTEX (dev_priv-
> > > >info.display_mmio_offset + 0x6452C)
> > >
> > > #define DP_AUX_CH_CTL(port) _MMIO_PORT(port,
> > > _DPA_AUX_CH_CTL, _DPB_AUX_CH_CTL)
> > > #define DP_AUX_CH_DATA(port, i) _MMIO(_PORT(port,
> > > _DPA_AUX_CH_DATA1, _DPB_AUX_CH_DATA1) + (i) * 4) /* 5 registers */
> > > @@ -5378,6 +5383,10 @@ enum {
> > > #define DP_AUX_CH_CTL_FW_SYNC_PULSE_SKL(c) (((c) - 1) << 5)
> > > #define DP_AUX_CH_CTL_SYNC_PULSE_SKL(c) ((c) - 1)
> > >
> >
> >
> > I believe _DPA_AUX_CH_MUTEX and _DPB_AUX_CH_MUTEX should be defined
> > here along
> > with the bits as Jani suggested.
> > I don't see any reason to tag that far from the bits and close to CTL
> > and DATA.
> >
> > > +#define DP_AUX_CH_MUTEX(port) _MMIO_PORT(port,
> > > _DPA_AUX_CH_MUTEX, _DPB_AUX_CH_MUTEX)
> > > +#define DP_AUX_CH_MUTEX_ENABLE (1 << 31)
> > > +#define DP_AUX_CH_MUTEX_STATUS (1 << 30)
> > > +
> > > /*
> > > * Computing GMCH M and N values for the Display Port link
> > > *
> > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > b/drivers/gpu/drm/i915/intel_dp.c
> > > index f20b25f98e5a..af07563bafba 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > @@ -1081,6 +1081,45 @@ static uint32_t
> > > intel_dp_get_aux_send_ctl(struct intel_dp *intel_dp,
> > > aux_clock_divider)
> > > ;
> > > }
> > >
> > > +static bool intel_dp_aux_ch_trylock(struct intel_dp *intel_dp)
> > > +{
> > > + struct intel_digital_port *intel_dig_port =
> > > dp_to_dig_port(intel_dp);
> > > + struct drm_i915_private *dev_priv =
> > > + to_i915(intel_dig_port->base.base.dev);
> > > +
> > > + if (INTEL_GEN(dev_priv) < 9)
> > > + return true;
> > > +
> > > + I915_WRITE(intel_dp->aux_ch_mutex_reg,
> > > DP_AUX_CH_MUTEX_ENABLE);
> >
> > You might be touching bits. We don't know if HW is using the reserved
> > bits or not.
> > So RMW |= bit 31 here is a good idea.
>
> As a read in this register request the mutex lock is better avoid any
> read that is not meant to request it.
ok... I accept the fact that read that is locking
so you are right here.
>
> >
> > > +
> > > + /* Spec says that mutex is acquired when status bit is
> > > read as unset,
> > > + * if set it should be read again after 500usec. Here
> > > waiting for
> > > + * 2msec or 4 tries before give up.
> > > + */
> > > + if (__intel_wait_for_register(dev_priv, intel_dp-
> > > >aux_ch_mutex_reg,
> > > + DP_AUX_CH_MUTEX_STATUS, 0,
> > > 500, 2,
> > > + NULL)) {
> >
> > why not intel_wait_for_register?
> > that seems cleaner...
>
> intel_wait_for_register() will call __intel_wait_for_register() with
> fast_timeout_us set as 2usec and specs says we should wait 500usec.
So, I've just noticed that __intel_wait_for_register spreaded to intel_hdcp.
Ideally if we have __ prefix this shouldn't be called for more than 1 place.
So it seems that we have more cases hence a rework should be done here.
But also the fast case is the atomic initial case... it is the first read.
Only if it fails it will consider the slow case. That's exactly what spec tells
Maybe what we want here is a intel_wait_for_register_us to have a slow value
based in us rather than ms?
I'm not fully sure here.
>
> >
> > > + DRM_WARN("dp_aux_ch port locked for too long");
> > > + return false;
> > > + }
> > > +
> > > + return true;
> > > +}
> > > +
> > > +static void intel_dp_aux_ch_unlock(struct intel_dp *intel_dp)
> > > +{
> > > + struct intel_digital_port *intel_dig_port =
> > > dp_to_dig_port(intel_dp);
> > > + struct drm_i915_private *dev_priv =
> > > + to_i915(intel_dig_port->base.base.dev);
> > > +
> > > + if (INTEL_GEN(dev_priv) < 9)
> > > + return;
> > > +
> > > + /* setting the status bit releases the mutex + keep mutex
> > > enabled */
> > > + I915_WRITE(intel_dp->aux_ch_mutex_reg,
> > > DP_AUX_CH_MUTEX_ENABLE |
> > > + DP_AUX_CH_MUTEX_STATUS);
> >
> > ditto.
> >
> > > +}
> > > +
> > > static int
> > > intel_dp_aux_ch(struct intel_dp *intel_dp,
> > > const uint8_t *send, int send_bytes,
> > > @@ -1115,6 +1154,11 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
> > >
> > > intel_dp_check_edp(intel_dp);
> > >
> > > + if (!intel_dp_aux_ch_trylock(intel_dp)) {
> > > + ret = -EBUSY;
> > > + goto out_locked;
> > > + }
> > > +
> >
> > BSpec: 4301
> > I believe you could reduce the lock region following the spec
> > and limiting it to steps 5-to-10, i.e.
> > inside the loop below.
>
> We loop trying to complete the transaction but we copy data received
> out of the loop and that is step 9, so the lock must be held until we
> copy the data from intel_dp->aux_ch_data_reg[] to recv.
>
> "
> for (i = 0; i < recv_bytes; i += 4)
> intel_dp_unpack_aux(I915_READ(intel_dp->aux_ch_data_reg[i >>
> 2]), recv + i, recv_bytes - i);
>
> ret = recv_bytes;
> out:
> intel_dp_aux_ch_unlock(intel_dp);
> "
hm... ok... this is the actually part 9 that needs to be inside
the locked area indeed. I missed that.
>
> >
> > > /* Try to wait for any previous AUX channel activity */
> > > for (try = 0; try < 3; try++) {
> > > status = I915_READ_NOTRACE(ch_ctl);
> > > @@ -1244,6 +1288,8 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
> > >
> > > ret = recv_bytes;
> > > out:
> > > + intel_dp_aux_ch_unlock(intel_dp);
> > > +out_locked:
> > > pm_qos_update_request(&dev_priv->pm_qos,
> > > PM_QOS_DEFAULT_VALUE);
> > >
> > > if (vdd)
> > > @@ -1496,6 +1542,10 @@ static void intel_aux_reg_init(struct
> > > intel_dp *intel_dp)
> > > intel_dp->aux_ch_ctl_reg = intel_aux_ctl_reg(dev_priv,
> > > port);
> > > for (i = 0; i < ARRAY_SIZE(intel_dp->aux_ch_data_reg);
> > > i++)
> > > intel_dp->aux_ch_data_reg[i] =
> > > intel_aux_data_reg(dev_priv, port, i);
> > > + if (INTEL_INFO(dev_priv)->gen >= 9) {
> > > + intel_dp->aux_ch_mutex_reg =
> > > DP_AUX_CH_MUTEX(port);
> > > + I915_WRITE(intel_dp->aux_ch_mutex_reg,
> > > DP_AUX_CH_MUTEX_ENABLE);
> >
> > I don't believe we need an extra enable here. And hopefully with
> > Villes
> > rework we don't even need to define here but only accessing it
> > directly from
> > the aux function with intel_dp->aux_ch reference.
>
> What if hardware wants to do some aux transaction before we do our
> first transaction(and enabling the mutex)? The mutex will not be
> enabled so hardware will not lock it.
>
> >
> > > + }
> > > }
> > >
> > > static void
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > b/drivers/gpu/drm/i915/intel_drv.h
> > > index 50874f4035cf..0c88657d6066 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -1043,6 +1043,7 @@ struct intel_dp {
> > > i915_reg_t output_reg;
> > > i915_reg_t aux_ch_ctl_reg;
> > > i915_reg_t aux_ch_data_reg[5];
> > > + i915_reg_t aux_ch_mutex_reg;
> > > uint32_t DP;
> > > int link_rate;
> > > uint8_t lane_count;
> > > --
> > > 2.16.2
> > >
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v2 1/1] drm/i915: Add and enable DP AUX CH mutex
2018-02-21 22:28 ` Rodrigo Vivi
@ 2018-02-23 1:13 ` Souza, Jose
0 siblings, 0 replies; 16+ messages in thread
From: Souza, Jose @ 2018-02-23 1:13 UTC (permalink / raw)
To: Vivi, Rodrigo; +Cc: intel-gfx, Syrjala, Ville, Pandiyan, Dhinakaran
On Wed, 2018-02-21 at 14:28 -0800, Rodrigo Vivi wrote:
> On Wed, Feb 21, 2018 at 01:12:08PM -0800, Souza, Jose wrote:
> > On Wed, 2018-02-21 at 12:45 -0800, Rodrigo Vivi wrote:
> > > On Tue, Feb 20, 2018 at 06:23:47PM -0800, José Roberto de Souza
> > > wrote:
> > > > When PSR/PSR2/GTC is enabled hardware can do AUX transactions
> > > > by it
> > > > self, so lets use the mutex register that is available in gen9+
> > > > to
> > > > avoid concurrent access by hardware and driver.
> > > >
> > > > Reference: https://01.org/sites/default/files/documentation/int
> > > > el-g
> > > > fx-prm-osrc-skl-vol12-display.pdf
> > > > Page 198 - AUX programming sequence
> > > >
> > >
> > > Cc: Ville
> > > Cc: DK
> > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > > ---
> > > > drivers/gpu/drm/i915/i915_reg.h | 9 ++++++++
> > > > drivers/gpu/drm/i915/intel_dp.c | 50
> > > > ++++++++++++++++++++++++++++++++++++++++
> > > > drivers/gpu/drm/i915/intel_drv.h | 1 +
> > > > 3 files changed, 60 insertions(+)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > > > b/drivers/gpu/drm/i915/i915_reg.h
> > > > index 1412abcb27d4..a62e3c1badab 100644
> > > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > > @@ -5318,6 +5318,7 @@ enum {
> > > > #define _DPA_AUX_CH_DATA3 (dev_priv-
> > > > > info.display_mmio_offset + 0x6401c)
> > > >
> > > > #define _DPA_AUX_CH_DATA4 (dev_priv-
> > > > > info.display_mmio_offset + 0x64020)
> > > >
> > > > #define _DPA_AUX_CH_DATA5 (dev_priv-
> > > > > info.display_mmio_offset + 0x64024)
> > > >
> > > > +#define _DPA_AUX_CH_MUTEX (dev_priv-
> > > > > info.display_mmio_offset + 0x6402C)
> > > >
> > > >
> > > > #define _DPB_AUX_CH_CTL (dev_priv-
> > > > > info.display_mmio_offset + 0x64110)
> > > >
> > > > #define _DPB_AUX_CH_DATA1 (dev_priv-
> > > > > info.display_mmio_offset + 0x64114)
> > > >
> > > > @@ -5325,6 +5326,7 @@ enum {
> > > > #define _DPB_AUX_CH_DATA3 (dev_priv-
> > > > > info.display_mmio_offset + 0x6411c)
> > > >
> > > > #define _DPB_AUX_CH_DATA4 (dev_priv-
> > > > > info.display_mmio_offset + 0x64120)
> > > >
> > > > #define _DPB_AUX_CH_DATA5 (dev_priv-
> > > > > info.display_mmio_offset + 0x64124)
> > > >
> > > > +#define _DPB_AUX_CH_MUTEX (dev_priv-
> > > > > info.display_mmio_offset + 0x6412C)
> > > >
> > > >
> > > > #define _DPC_AUX_CH_CTL (dev_priv-
> > > > > info.display_mmio_offset + 0x64210)
> > > >
> > > > #define _DPC_AUX_CH_DATA1 (dev_priv-
> > > > > info.display_mmio_offset + 0x64214)
> > > >
> > > > @@ -5332,6 +5334,7 @@ enum {
> > > > #define _DPC_AUX_CH_DATA3 (dev_priv-
> > > > > info.display_mmio_offset + 0x6421c)
> > > >
> > > > #define _DPC_AUX_CH_DATA4 (dev_priv-
> > > > > info.display_mmio_offset + 0x64220)
> > > >
> > > > #define _DPC_AUX_CH_DATA5 (dev_priv-
> > > > > info.display_mmio_offset + 0x64224)
> > > >
> > > > +#define _DPC_AUX_CH_MUTEX (dev_priv-
> > > > > info.display_mmio_offset + 0x6422C)
> > > >
> > > >
> > > > #define _DPD_AUX_CH_CTL (dev_priv-
> > > > > info.display_mmio_offset + 0x64310)
> > > >
> > > > #define _DPD_AUX_CH_DATA1 (dev_priv-
> > > > > info.display_mmio_offset + 0x64314)
> > > >
> > > > @@ -5339,6 +5342,7 @@ enum {
> > > > #define _DPD_AUX_CH_DATA3 (dev_priv-
> > > > > info.display_mmio_offset + 0x6431c)
> > > >
> > > > #define _DPD_AUX_CH_DATA4 (dev_priv-
> > > > > info.display_mmio_offset + 0x64320)
> > > >
> > > > #define _DPD_AUX_CH_DATA5 (dev_priv-
> > > > > info.display_mmio_offset + 0x64324)
> > > >
> > > > +#define _DPD_AUX_CH_MUTEX (dev_priv-
> > > > > info.display_mmio_offset + 0x6432C)
> > > >
> > > >
> > > > #define _DPF_AUX_CH_CTL (dev_priv-
> > > > > info.display_mmio_offset + 0x64510)
> > > >
> > > > #define _DPF_AUX_CH_DATA1 (dev_priv-
> > > > > info.display_mmio_offset + 0x64514)
> > > >
> > > > @@ -5346,6 +5350,7 @@ enum {
> > > > #define _DPF_AUX_CH_DATA3 (dev_priv-
> > > > > info.display_mmio_offset + 0x6451c)
> > > >
> > > > #define _DPF_AUX_CH_DATA4 (dev_priv-
> > > > > info.display_mmio_offset + 0x64520)
> > > >
> > > > #define _DPF_AUX_CH_DATA5 (dev_priv-
> > > > > info.display_mmio_offset + 0x64524)
> > > >
> > > > +#define _DPF_AUX_CH_MUTEX (dev_priv-
> > > > > info.display_mmio_offset + 0x6452C)
> > > >
> > > >
> > > > #define DP_AUX_CH_CTL(port) _MMIO_PORT(port,
> > > > _DPA_AUX_CH_CTL, _DPB_AUX_CH_CTL)
> > > > #define DP_AUX_CH_DATA(port, i) _MMIO(_PORT(port,
> > > > _DPA_AUX_CH_DATA1, _DPB_AUX_CH_DATA1) + (i) * 4) /* 5 registers
> > > > */
> > > > @@ -5378,6 +5383,10 @@ enum {
> > > > #define DP_AUX_CH_CTL_FW_SYNC_PULSE_SKL(c) (((c) - 1) << 5)
> > > > #define DP_AUX_CH_CTL_SYNC_PULSE_SKL(c) ((c) - 1)
> > > >
> > >
> > >
> > > I believe _DPA_AUX_CH_MUTEX and _DPB_AUX_CH_MUTEX should be
> > > defined
> > > here along
> > > with the bits as Jani suggested.
> > > I don't see any reason to tag that far from the bits and close to
> > > CTL
> > > and DATA.
Okay moving it.
> > >
> > > > +#define DP_AUX_CH_MUTEX(port) _MMIO_PORT(port,
> > > > _DPA_AUX_CH_MUTEX, _DPB_AUX_CH_MUTEX)
> > > > +#define DP_AUX_CH_MUTEX_ENABLE (1 << 31)
> > > > +#define DP_AUX_CH_MUTEX_STATUS (1 << 30)
> > > > +
> > > > /*
> > > > * Computing GMCH M and N values for the Display Port link
> > > > *
> > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > > b/drivers/gpu/drm/i915/intel_dp.c
> > > > index f20b25f98e5a..af07563bafba 100644
> > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > @@ -1081,6 +1081,45 @@ static uint32_t
> > > > intel_dp_get_aux_send_ctl(struct intel_dp *intel_dp,
> > > > aux_clock_divi
> > > > der)
> > > > ;
> > > > }
> > > >
> > > > +static bool intel_dp_aux_ch_trylock(struct intel_dp *intel_dp)
> > > > +{
> > > > + struct intel_digital_port *intel_dig_port =
> > > > dp_to_dig_port(intel_dp);
> > > > + struct drm_i915_private *dev_priv =
> > > > + to_i915(intel_dig_port-
> > > > >base.base.dev);
> > > > +
> > > > + if (INTEL_GEN(dev_priv) < 9)
> > > > + return true;
> > > > +
> > > > + I915_WRITE(intel_dp->aux_ch_mutex_reg,
> > > > DP_AUX_CH_MUTEX_ENABLE);
> > >
> > > You might be touching bits. We don't know if HW is using the
> > > reserved
> > > bits or not.
> > > So RMW |= bit 31 here is a good idea.
> >
> > As a read in this register request the mutex lock is better avoid
> > any
> > read that is not meant to request it.
>
> ok... I accept the fact that read that is locking
> so you are right here.
>
> >
> > >
> > > > +
> > > > + /* Spec says that mutex is acquired when status bit is
> > > > read as unset,
> > > > + * if set it should be read again after 500usec. Here
> > > > waiting for
> > > > + * 2msec or 4 tries before give up.
> > > > + */
> > > > + if (__intel_wait_for_register(dev_priv, intel_dp-
> > > > > aux_ch_mutex_reg,
> > > >
> > > > + DP_AUX_CH_MUTEX_STATUS,
> > > > 0,
> > > > 500, 2,
> > > > + NULL)) {
> > >
> > > why not intel_wait_for_register?
> > > that seems cleaner...
> >
> > intel_wait_for_register() will call __intel_wait_for_register()
> > with
> > fast_timeout_us set as 2usec and specs says we should wait 500usec.
>
> So, I've just noticed that __intel_wait_for_register spreaded to
> intel_hdcp.
> Ideally if we have __ prefix this shouldn't be called for more than 1
> place.
>
> So it seems that we have more cases hence a rework should be done
> here.
>
> But also the fast case is the atomic initial case... it is the first
> read.
> Only if it fails it will consider the slow case. That's exactly what
> spec tells
>
> Maybe what we want here is a intel_wait_for_register_us to have a
> slow value
> based in us rather than ms?
>
> I'm not fully sure here.
Reading again the implementation it don't do exactly what I was
expecting.
It will check the register for the expected value until fast_timeout_us
without sleeping and if not meet the expected value it will wait until
slow_timeout_ms sleeping from 10usec to 1000usec increasing at each
iteration.
So I guess I can use the regular intel_wait_for_register() with
timeout_ms=2msec, or there is any other suggestion?
>
> >
> > >
> > > > + DRM_WARN("dp_aux_ch port locked for too
> > > > long");
> > > > + return false;
> > > > + }
> > > > +
> > > > + return true;
> > > > +}
> > > > +
> > > > +static void intel_dp_aux_ch_unlock(struct intel_dp *intel_dp)
> > > > +{
> > > > + struct intel_digital_port *intel_dig_port =
> > > > dp_to_dig_port(intel_dp);
> > > > + struct drm_i915_private *dev_priv =
> > > > + to_i915(intel_dig_port-
> > > > >base.base.dev);
> > > > +
> > > > + if (INTEL_GEN(dev_priv) < 9)
> > > > + return;
> > > > +
> > > > + /* setting the status bit releases the mutex + keep
> > > > mutex
> > > > enabled */
> > > > + I915_WRITE(intel_dp->aux_ch_mutex_reg,
> > > > DP_AUX_CH_MUTEX_ENABLE |
> > > > + DP_AUX_CH_MUTEX_STATUS);
> > >
> > > ditto.
> > >
> > > > +}
> > > > +
> > > > static int
> > > > intel_dp_aux_ch(struct intel_dp *intel_dp,
> > > > const uint8_t *send, int send_bytes,
> > > > @@ -1115,6 +1154,11 @@ intel_dp_aux_ch(struct intel_dp
> > > > *intel_dp,
> > > >
> > > > intel_dp_check_edp(intel_dp);
> > > >
> > > > + if (!intel_dp_aux_ch_trylock(intel_dp)) {
> > > > + ret = -EBUSY;
> > > > + goto out_locked;
> > > > + }
> > > > +
> > >
> > > BSpec: 4301
> > > I believe you could reduce the lock region following the spec
> > > and limiting it to steps 5-to-10, i.e.
> > > inside the loop below.
> >
> > We loop trying to complete the transaction but we copy data
> > received
> > out of the loop and that is step 9, so the lock must be held until
> > we
> > copy the data from intel_dp->aux_ch_data_reg[] to recv.
> >
> > "
> > for (i = 0; i < recv_bytes; i += 4)
> > intel_dp_unpack_aux(I915_READ(intel_dp-
> > >aux_ch_data_reg[i >>
> > 2]), recv + i, recv_bytes - i);
> >
> > ret = recv_bytes;
> > out:
> > intel_dp_aux_ch_unlock(intel_dp);
> > "
>
> hm... ok... this is the actually part 9 that needs to be inside
> the locked area indeed. I missed that.
>
> >
> > >
> > > > /* Try to wait for any previous AUX channel activity
> > > > */
> > > > for (try = 0; try < 3; try++) {
> > > > status = I915_READ_NOTRACE(ch_ctl);
> > > > @@ -1244,6 +1288,8 @@ intel_dp_aux_ch(struct intel_dp
> > > > *intel_dp,
> > > >
> > > > ret = recv_bytes;
> > > > out:
> > > > + intel_dp_aux_ch_unlock(intel_dp);
> > > > +out_locked:
> > > > pm_qos_update_request(&dev_priv->pm_qos,
> > > > PM_QOS_DEFAULT_VALUE);
> > > >
> > > > if (vdd)
> > > > @@ -1496,6 +1542,10 @@ static void intel_aux_reg_init(struct
> > > > intel_dp *intel_dp)
> > > > intel_dp->aux_ch_ctl_reg = intel_aux_ctl_reg(dev_priv,
> > > > port);
> > > > for (i = 0; i < ARRAY_SIZE(intel_dp->aux_ch_data_reg);
> > > > i++)
> > > > intel_dp->aux_ch_data_reg[i] =
> > > > intel_aux_data_reg(dev_priv, port, i);
> > > > + if (INTEL_INFO(dev_priv)->gen >= 9) {
> > > > + intel_dp->aux_ch_mutex_reg =
> > > > DP_AUX_CH_MUTEX(port);
> > > > + I915_WRITE(intel_dp->aux_ch_mutex_reg,
> > > > DP_AUX_CH_MUTEX_ENABLE);
> > >
> > > I don't believe we need an extra enable here. And hopefully with
> > > Villes
> > > rework we don't even need to define here but only accessing it
> > > directly from
> > > the aux function with intel_dp->aux_ch reference.
> >
> > What if hardware wants to do some aux transaction before we do our
> > first transaction(and enabling the mutex)? The mutex will not be
> > enabled so hardware will not lock it.
> >
> > >
> > > > + }
> > > > }
> > > >
> > > > static void
> > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > > b/drivers/gpu/drm/i915/intel_drv.h
> > > > index 50874f4035cf..0c88657d6066 100644
> > > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > > @@ -1043,6 +1043,7 @@ struct intel_dp {
> > > > i915_reg_t output_reg;
> > > > i915_reg_t aux_ch_ctl_reg;
> > > > i915_reg_t aux_ch_data_reg[5];
> > > > + i915_reg_t aux_ch_mutex_reg;
> > > > uint32_t DP;
> > > > int link_rate;
> > > > uint8_t lane_count;
> > > > --
> > > > 2.16.2
> > > >
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/1] drm/i915: Add and enable DP AUX CH mutex
2018-02-21 2:23 ` [PATCH v2 1/1] " José Roberto de Souza
2018-02-21 8:59 ` Dhinakaran Pandiyan
2018-02-21 20:45 ` Rodrigo Vivi
@ 2018-02-22 18:59 ` Ville Syrjälä
2018-02-22 19:34 ` Rodrigo Vivi
2 siblings, 1 reply; 16+ messages in thread
From: Ville Syrjälä @ 2018-02-22 18:59 UTC (permalink / raw)
To: José Roberto de Souza; +Cc: intel-gfx, Rodrigo Vivi
On Tue, Feb 20, 2018 at 06:23:47PM -0800, José Roberto de Souza wrote:
> When PSR/PSR2/GTC is enabled hardware can do AUX transactions by it
> self, so lets use the mutex register that is available in gen9+ to
> avoid concurrent access by hardware and driver.
>
> Reference: https://01.org/sites/default/files/documentation/intel-gfx-prm-osrc-skl-vol12-display.pdf
> Page 198 - AUX programming sequence
BTW since HSW/BDW don't have the mutex we should really be disabling PSR
around aux ch usage on those platforms. Not sure how big of a locking
inversion exercise it would be to make that happen though.
>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
> drivers/gpu/drm/i915/i915_reg.h | 9 ++++++++
> drivers/gpu/drm/i915/intel_dp.c | 50 ++++++++++++++++++++++++++++++++++++++++
> drivers/gpu/drm/i915/intel_drv.h | 1 +
> 3 files changed, 60 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 1412abcb27d4..a62e3c1badab 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -5318,6 +5318,7 @@ enum {
> #define _DPA_AUX_CH_DATA3 (dev_priv->info.display_mmio_offset + 0x6401c)
> #define _DPA_AUX_CH_DATA4 (dev_priv->info.display_mmio_offset + 0x64020)
> #define _DPA_AUX_CH_DATA5 (dev_priv->info.display_mmio_offset + 0x64024)
> +#define _DPA_AUX_CH_MUTEX (dev_priv->info.display_mmio_offset + 0x6402C)
>
> #define _DPB_AUX_CH_CTL (dev_priv->info.display_mmio_offset + 0x64110)
> #define _DPB_AUX_CH_DATA1 (dev_priv->info.display_mmio_offset + 0x64114)
> @@ -5325,6 +5326,7 @@ enum {
> #define _DPB_AUX_CH_DATA3 (dev_priv->info.display_mmio_offset + 0x6411c)
> #define _DPB_AUX_CH_DATA4 (dev_priv->info.display_mmio_offset + 0x64120)
> #define _DPB_AUX_CH_DATA5 (dev_priv->info.display_mmio_offset + 0x64124)
> +#define _DPB_AUX_CH_MUTEX (dev_priv->info.display_mmio_offset + 0x6412C)
>
> #define _DPC_AUX_CH_CTL (dev_priv->info.display_mmio_offset + 0x64210)
> #define _DPC_AUX_CH_DATA1 (dev_priv->info.display_mmio_offset + 0x64214)
> @@ -5332,6 +5334,7 @@ enum {
> #define _DPC_AUX_CH_DATA3 (dev_priv->info.display_mmio_offset + 0x6421c)
> #define _DPC_AUX_CH_DATA4 (dev_priv->info.display_mmio_offset + 0x64220)
> #define _DPC_AUX_CH_DATA5 (dev_priv->info.display_mmio_offset + 0x64224)
> +#define _DPC_AUX_CH_MUTEX (dev_priv->info.display_mmio_offset + 0x6422C)
>
> #define _DPD_AUX_CH_CTL (dev_priv->info.display_mmio_offset + 0x64310)
> #define _DPD_AUX_CH_DATA1 (dev_priv->info.display_mmio_offset + 0x64314)
> @@ -5339,6 +5342,7 @@ enum {
> #define _DPD_AUX_CH_DATA3 (dev_priv->info.display_mmio_offset + 0x6431c)
> #define _DPD_AUX_CH_DATA4 (dev_priv->info.display_mmio_offset + 0x64320)
> #define _DPD_AUX_CH_DATA5 (dev_priv->info.display_mmio_offset + 0x64324)
> +#define _DPD_AUX_CH_MUTEX (dev_priv->info.display_mmio_offset + 0x6432C)
>
> #define _DPF_AUX_CH_CTL (dev_priv->info.display_mmio_offset + 0x64510)
> #define _DPF_AUX_CH_DATA1 (dev_priv->info.display_mmio_offset + 0x64514)
> @@ -5346,6 +5350,7 @@ enum {
> #define _DPF_AUX_CH_DATA3 (dev_priv->info.display_mmio_offset + 0x6451c)
> #define _DPF_AUX_CH_DATA4 (dev_priv->info.display_mmio_offset + 0x64520)
> #define _DPF_AUX_CH_DATA5 (dev_priv->info.display_mmio_offset + 0x64524)
> +#define _DPF_AUX_CH_MUTEX (dev_priv->info.display_mmio_offset + 0x6452C)
>
> #define DP_AUX_CH_CTL(port) _MMIO_PORT(port, _DPA_AUX_CH_CTL, _DPB_AUX_CH_CTL)
> #define DP_AUX_CH_DATA(port, i) _MMIO(_PORT(port, _DPA_AUX_CH_DATA1, _DPB_AUX_CH_DATA1) + (i) * 4) /* 5 registers */
> @@ -5378,6 +5383,10 @@ enum {
> #define DP_AUX_CH_CTL_FW_SYNC_PULSE_SKL(c) (((c) - 1) << 5)
> #define DP_AUX_CH_CTL_SYNC_PULSE_SKL(c) ((c) - 1)
>
> +#define DP_AUX_CH_MUTEX(port) _MMIO_PORT(port, _DPA_AUX_CH_MUTEX, _DPB_AUX_CH_MUTEX)
> +#define DP_AUX_CH_MUTEX_ENABLE (1 << 31)
> +#define DP_AUX_CH_MUTEX_STATUS (1 << 30)
> +
> /*
> * Computing GMCH M and N values for the Display Port link
> *
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index f20b25f98e5a..af07563bafba 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1081,6 +1081,45 @@ static uint32_t intel_dp_get_aux_send_ctl(struct intel_dp *intel_dp,
> aux_clock_divider);
> }
>
> +static bool intel_dp_aux_ch_trylock(struct intel_dp *intel_dp)
> +{
> + struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> + struct drm_i915_private *dev_priv =
> + to_i915(intel_dig_port->base.base.dev);
> +
> + if (INTEL_GEN(dev_priv) < 9)
> + return true;
> +
> + I915_WRITE(intel_dp->aux_ch_mutex_reg, DP_AUX_CH_MUTEX_ENABLE);
> +
> + /* Spec says that mutex is acquired when status bit is read as unset,
> + * if set it should be read again after 500usec. Here waiting for
> + * 2msec or 4 tries before give up.
> + */
> + if (__intel_wait_for_register(dev_priv, intel_dp->aux_ch_mutex_reg,
> + DP_AUX_CH_MUTEX_STATUS, 0, 500, 2,
> + NULL)) {
> + DRM_WARN("dp_aux_ch port locked for too long");
> + return false;
> + }
> +
> + return true;
> +}
> +
> +static void intel_dp_aux_ch_unlock(struct intel_dp *intel_dp)
> +{
> + struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> + struct drm_i915_private *dev_priv =
> + to_i915(intel_dig_port->base.base.dev);
> +
> + if (INTEL_GEN(dev_priv) < 9)
> + return;
> +
> + /* setting the status bit releases the mutex + keep mutex enabled */
> + I915_WRITE(intel_dp->aux_ch_mutex_reg, DP_AUX_CH_MUTEX_ENABLE |
> + DP_AUX_CH_MUTEX_STATUS);
> +}
> +
> static int
> intel_dp_aux_ch(struct intel_dp *intel_dp,
> const uint8_t *send, int send_bytes,
> @@ -1115,6 +1154,11 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
>
> intel_dp_check_edp(intel_dp);
>
> + if (!intel_dp_aux_ch_trylock(intel_dp)) {
> + ret = -EBUSY;
> + goto out_locked;
> + }
> +
> /* Try to wait for any previous AUX channel activity */
> for (try = 0; try < 3; try++) {
> status = I915_READ_NOTRACE(ch_ctl);
> @@ -1244,6 +1288,8 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
>
> ret = recv_bytes;
> out:
> + intel_dp_aux_ch_unlock(intel_dp);
> +out_locked:
> pm_qos_update_request(&dev_priv->pm_qos, PM_QOS_DEFAULT_VALUE);
>
> if (vdd)
> @@ -1496,6 +1542,10 @@ static void intel_aux_reg_init(struct intel_dp *intel_dp)
> intel_dp->aux_ch_ctl_reg = intel_aux_ctl_reg(dev_priv, port);
> for (i = 0; i < ARRAY_SIZE(intel_dp->aux_ch_data_reg); i++)
> intel_dp->aux_ch_data_reg[i] = intel_aux_data_reg(dev_priv, port, i);
> + if (INTEL_INFO(dev_priv)->gen >= 9) {
> + intel_dp->aux_ch_mutex_reg = DP_AUX_CH_MUTEX(port);
> + I915_WRITE(intel_dp->aux_ch_mutex_reg, DP_AUX_CH_MUTEX_ENABLE);
> + }
> }
>
> static void
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 50874f4035cf..0c88657d6066 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1043,6 +1043,7 @@ struct intel_dp {
> i915_reg_t output_reg;
> i915_reg_t aux_ch_ctl_reg;
> i915_reg_t aux_ch_data_reg[5];
> + i915_reg_t aux_ch_mutex_reg;
> uint32_t DP;
> int link_rate;
> uint8_t lane_count;
> --
> 2.16.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v2 1/1] drm/i915: Add and enable DP AUX CH mutex
2018-02-22 18:59 ` Ville Syrjälä
@ 2018-02-22 19:34 ` Rodrigo Vivi
2018-02-22 19:57 ` Pandiyan, Dhinakaran
0 siblings, 1 reply; 16+ messages in thread
From: Rodrigo Vivi @ 2018-02-22 19:34 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: intel-gfx
On Thu, Feb 22, 2018 at 08:59:49PM +0200, Ville Syrjälä wrote:
> On Tue, Feb 20, 2018 at 06:23:47PM -0800, José Roberto de Souza wrote:
> > When PSR/PSR2/GTC is enabled hardware can do AUX transactions by it
> > self, so lets use the mutex register that is available in gen9+ to
> > avoid concurrent access by hardware and driver.
> >
> > Reference: https://01.org/sites/default/files/documentation/intel-gfx-prm-osrc-skl-vol12-display.pdf
> > Page 198 - AUX programming sequence
>
> BTW since HSW/BDW don't have the mutex we should really be disabling PSR
> around aux ch usage on those platforms. Not sure how big of a locking
> inversion exercise it would be to make that happen though.
True. But I believe that can be handled differently.
Probably with Aux power domains blocking PSR off "domain".
Something similar to DC off.
I actually had patches for that lost somewhere... :(
>
> >
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> > drivers/gpu/drm/i915/i915_reg.h | 9 ++++++++
> > drivers/gpu/drm/i915/intel_dp.c | 50 ++++++++++++++++++++++++++++++++++++++++
> > drivers/gpu/drm/i915/intel_drv.h | 1 +
> > 3 files changed, 60 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index 1412abcb27d4..a62e3c1badab 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -5318,6 +5318,7 @@ enum {
> > #define _DPA_AUX_CH_DATA3 (dev_priv->info.display_mmio_offset + 0x6401c)
> > #define _DPA_AUX_CH_DATA4 (dev_priv->info.display_mmio_offset + 0x64020)
> > #define _DPA_AUX_CH_DATA5 (dev_priv->info.display_mmio_offset + 0x64024)
> > +#define _DPA_AUX_CH_MUTEX (dev_priv->info.display_mmio_offset + 0x6402C)
> >
> > #define _DPB_AUX_CH_CTL (dev_priv->info.display_mmio_offset + 0x64110)
> > #define _DPB_AUX_CH_DATA1 (dev_priv->info.display_mmio_offset + 0x64114)
> > @@ -5325,6 +5326,7 @@ enum {
> > #define _DPB_AUX_CH_DATA3 (dev_priv->info.display_mmio_offset + 0x6411c)
> > #define _DPB_AUX_CH_DATA4 (dev_priv->info.display_mmio_offset + 0x64120)
> > #define _DPB_AUX_CH_DATA5 (dev_priv->info.display_mmio_offset + 0x64124)
> > +#define _DPB_AUX_CH_MUTEX (dev_priv->info.display_mmio_offset + 0x6412C)
> >
> > #define _DPC_AUX_CH_CTL (dev_priv->info.display_mmio_offset + 0x64210)
> > #define _DPC_AUX_CH_DATA1 (dev_priv->info.display_mmio_offset + 0x64214)
> > @@ -5332,6 +5334,7 @@ enum {
> > #define _DPC_AUX_CH_DATA3 (dev_priv->info.display_mmio_offset + 0x6421c)
> > #define _DPC_AUX_CH_DATA4 (dev_priv->info.display_mmio_offset + 0x64220)
> > #define _DPC_AUX_CH_DATA5 (dev_priv->info.display_mmio_offset + 0x64224)
> > +#define _DPC_AUX_CH_MUTEX (dev_priv->info.display_mmio_offset + 0x6422C)
> >
> > #define _DPD_AUX_CH_CTL (dev_priv->info.display_mmio_offset + 0x64310)
> > #define _DPD_AUX_CH_DATA1 (dev_priv->info.display_mmio_offset + 0x64314)
> > @@ -5339,6 +5342,7 @@ enum {
> > #define _DPD_AUX_CH_DATA3 (dev_priv->info.display_mmio_offset + 0x6431c)
> > #define _DPD_AUX_CH_DATA4 (dev_priv->info.display_mmio_offset + 0x64320)
> > #define _DPD_AUX_CH_DATA5 (dev_priv->info.display_mmio_offset + 0x64324)
> > +#define _DPD_AUX_CH_MUTEX (dev_priv->info.display_mmio_offset + 0x6432C)
> >
> > #define _DPF_AUX_CH_CTL (dev_priv->info.display_mmio_offset + 0x64510)
> > #define _DPF_AUX_CH_DATA1 (dev_priv->info.display_mmio_offset + 0x64514)
> > @@ -5346,6 +5350,7 @@ enum {
> > #define _DPF_AUX_CH_DATA3 (dev_priv->info.display_mmio_offset + 0x6451c)
> > #define _DPF_AUX_CH_DATA4 (dev_priv->info.display_mmio_offset + 0x64520)
> > #define _DPF_AUX_CH_DATA5 (dev_priv->info.display_mmio_offset + 0x64524)
> > +#define _DPF_AUX_CH_MUTEX (dev_priv->info.display_mmio_offset + 0x6452C)
> >
> > #define DP_AUX_CH_CTL(port) _MMIO_PORT(port, _DPA_AUX_CH_CTL, _DPB_AUX_CH_CTL)
> > #define DP_AUX_CH_DATA(port, i) _MMIO(_PORT(port, _DPA_AUX_CH_DATA1, _DPB_AUX_CH_DATA1) + (i) * 4) /* 5 registers */
> > @@ -5378,6 +5383,10 @@ enum {
> > #define DP_AUX_CH_CTL_FW_SYNC_PULSE_SKL(c) (((c) - 1) << 5)
> > #define DP_AUX_CH_CTL_SYNC_PULSE_SKL(c) ((c) - 1)
> >
> > +#define DP_AUX_CH_MUTEX(port) _MMIO_PORT(port, _DPA_AUX_CH_MUTEX, _DPB_AUX_CH_MUTEX)
> > +#define DP_AUX_CH_MUTEX_ENABLE (1 << 31)
> > +#define DP_AUX_CH_MUTEX_STATUS (1 << 30)
> > +
> > /*
> > * Computing GMCH M and N values for the Display Port link
> > *
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index f20b25f98e5a..af07563bafba 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -1081,6 +1081,45 @@ static uint32_t intel_dp_get_aux_send_ctl(struct intel_dp *intel_dp,
> > aux_clock_divider);
> > }
> >
> > +static bool intel_dp_aux_ch_trylock(struct intel_dp *intel_dp)
> > +{
> > + struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> > + struct drm_i915_private *dev_priv =
> > + to_i915(intel_dig_port->base.base.dev);
> > +
> > + if (INTEL_GEN(dev_priv) < 9)
> > + return true;
> > +
> > + I915_WRITE(intel_dp->aux_ch_mutex_reg, DP_AUX_CH_MUTEX_ENABLE);
> > +
> > + /* Spec says that mutex is acquired when status bit is read as unset,
> > + * if set it should be read again after 500usec. Here waiting for
> > + * 2msec or 4 tries before give up.
> > + */
> > + if (__intel_wait_for_register(dev_priv, intel_dp->aux_ch_mutex_reg,
> > + DP_AUX_CH_MUTEX_STATUS, 0, 500, 2,
> > + NULL)) {
> > + DRM_WARN("dp_aux_ch port locked for too long");
> > + return false;
> > + }
> > +
> > + return true;
> > +}
> > +
> > +static void intel_dp_aux_ch_unlock(struct intel_dp *intel_dp)
> > +{
> > + struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> > + struct drm_i915_private *dev_priv =
> > + to_i915(intel_dig_port->base.base.dev);
> > +
> > + if (INTEL_GEN(dev_priv) < 9)
> > + return;
> > +
> > + /* setting the status bit releases the mutex + keep mutex enabled */
> > + I915_WRITE(intel_dp->aux_ch_mutex_reg, DP_AUX_CH_MUTEX_ENABLE |
> > + DP_AUX_CH_MUTEX_STATUS);
> > +}
> > +
> > static int
> > intel_dp_aux_ch(struct intel_dp *intel_dp,
> > const uint8_t *send, int send_bytes,
> > @@ -1115,6 +1154,11 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
> >
> > intel_dp_check_edp(intel_dp);
> >
> > + if (!intel_dp_aux_ch_trylock(intel_dp)) {
> > + ret = -EBUSY;
> > + goto out_locked;
> > + }
> > +
> > /* Try to wait for any previous AUX channel activity */
> > for (try = 0; try < 3; try++) {
> > status = I915_READ_NOTRACE(ch_ctl);
> > @@ -1244,6 +1288,8 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
> >
> > ret = recv_bytes;
> > out:
> > + intel_dp_aux_ch_unlock(intel_dp);
> > +out_locked:
> > pm_qos_update_request(&dev_priv->pm_qos, PM_QOS_DEFAULT_VALUE);
> >
> > if (vdd)
> > @@ -1496,6 +1542,10 @@ static void intel_aux_reg_init(struct intel_dp *intel_dp)
> > intel_dp->aux_ch_ctl_reg = intel_aux_ctl_reg(dev_priv, port);
> > for (i = 0; i < ARRAY_SIZE(intel_dp->aux_ch_data_reg); i++)
> > intel_dp->aux_ch_data_reg[i] = intel_aux_data_reg(dev_priv, port, i);
> > + if (INTEL_INFO(dev_priv)->gen >= 9) {
> > + intel_dp->aux_ch_mutex_reg = DP_AUX_CH_MUTEX(port);
> > + I915_WRITE(intel_dp->aux_ch_mutex_reg, DP_AUX_CH_MUTEX_ENABLE);
> > + }
> > }
> >
> > static void
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 50874f4035cf..0c88657d6066 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1043,6 +1043,7 @@ struct intel_dp {
> > i915_reg_t output_reg;
> > i915_reg_t aux_ch_ctl_reg;
> > i915_reg_t aux_ch_data_reg[5];
> > + i915_reg_t aux_ch_mutex_reg;
> > uint32_t DP;
> > int link_rate;
> > uint8_t lane_count;
> > --
> > 2.16.2
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Ville Syrjälä
> Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v2 1/1] drm/i915: Add and enable DP AUX CH mutex
2018-02-22 19:34 ` Rodrigo Vivi
@ 2018-02-22 19:57 ` Pandiyan, Dhinakaran
2018-02-22 20:13 ` Rodrigo Vivi
0 siblings, 1 reply; 16+ messages in thread
From: Pandiyan, Dhinakaran @ 2018-02-22 19:57 UTC (permalink / raw)
To: Vivi, Rodrigo; +Cc: intel-gfx
On Thu, 2018-02-22 at 11:34 -0800, Rodrigo Vivi wrote:
> On Thu, Feb 22, 2018 at 08:59:49PM +0200, Ville Syrjälä wrote:
> > On Tue, Feb 20, 2018 at 06:23:47PM -0800, José Roberto de Souza wrote:
> > > When PSR/PSR2/GTC is enabled hardware can do AUX transactions by it
> > > self, so lets use the mutex register that is available in gen9+ to
> > > avoid concurrent access by hardware and driver.
> > >
> > > Reference: https://01.org/sites/default/files/documentation/intel-gfx-prm-osrc-skl-vol12-display.pdf
> > > Page 198 - AUX programming sequence
> >
> > BTW since HSW/BDW don't have the mutex we should really be disabling PSR
> > around aux ch usage on those platforms. Not sure how big of a locking
> > inversion exercise it would be to make that happen though.
>
> True. But I believe that can be handled differently.
> Probably with Aux power domains blocking PSR off "domain".
> Something similar to DC off.
>
> I actually had patches for that lost somewhere... :(
>
Just a note, the spec explicitly says we should disable PSR around AUX
transactions for BDW (and HSW iirc).
About using the power domain infra, I am not quite sure. The _get() and
_put() idiom makes sense when we want reference counting to handle
multiple consumers that want PSR to be disabled. Does anything else need
to enable/disable PSR?
> >
> > >
> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > ---
> > > drivers/gpu/drm/i915/i915_reg.h | 9 ++++++++
> > > drivers/gpu/drm/i915/intel_dp.c | 50 ++++++++++++++++++++++++++++++++++++++++
> > > drivers/gpu/drm/i915/intel_drv.h | 1 +
> > > 3 files changed, 60 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > > index 1412abcb27d4..a62e3c1badab 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -5318,6 +5318,7 @@ enum {
> > > #define _DPA_AUX_CH_DATA3 (dev_priv->info.display_mmio_offset + 0x6401c)
> > > #define _DPA_AUX_CH_DATA4 (dev_priv->info.display_mmio_offset + 0x64020)
> > > #define _DPA_AUX_CH_DATA5 (dev_priv->info.display_mmio_offset + 0x64024)
> > > +#define _DPA_AUX_CH_MUTEX (dev_priv->info.display_mmio_offset + 0x6402C)
> > >
> > > #define _DPB_AUX_CH_CTL (dev_priv->info.display_mmio_offset + 0x64110)
> > > #define _DPB_AUX_CH_DATA1 (dev_priv->info.display_mmio_offset + 0x64114)
> > > @@ -5325,6 +5326,7 @@ enum {
> > > #define _DPB_AUX_CH_DATA3 (dev_priv->info.display_mmio_offset + 0x6411c)
> > > #define _DPB_AUX_CH_DATA4 (dev_priv->info.display_mmio_offset + 0x64120)
> > > #define _DPB_AUX_CH_DATA5 (dev_priv->info.display_mmio_offset + 0x64124)
> > > +#define _DPB_AUX_CH_MUTEX (dev_priv->info.display_mmio_offset + 0x6412C)
> > >
> > > #define _DPC_AUX_CH_CTL (dev_priv->info.display_mmio_offset + 0x64210)
> > > #define _DPC_AUX_CH_DATA1 (dev_priv->info.display_mmio_offset + 0x64214)
> > > @@ -5332,6 +5334,7 @@ enum {
> > > #define _DPC_AUX_CH_DATA3 (dev_priv->info.display_mmio_offset + 0x6421c)
> > > #define _DPC_AUX_CH_DATA4 (dev_priv->info.display_mmio_offset + 0x64220)
> > > #define _DPC_AUX_CH_DATA5 (dev_priv->info.display_mmio_offset + 0x64224)
> > > +#define _DPC_AUX_CH_MUTEX (dev_priv->info.display_mmio_offset + 0x6422C)
> > >
> > > #define _DPD_AUX_CH_CTL (dev_priv->info.display_mmio_offset + 0x64310)
> > > #define _DPD_AUX_CH_DATA1 (dev_priv->info.display_mmio_offset + 0x64314)
> > > @@ -5339,6 +5342,7 @@ enum {
> > > #define _DPD_AUX_CH_DATA3 (dev_priv->info.display_mmio_offset + 0x6431c)
> > > #define _DPD_AUX_CH_DATA4 (dev_priv->info.display_mmio_offset + 0x64320)
> > > #define _DPD_AUX_CH_DATA5 (dev_priv->info.display_mmio_offset + 0x64324)
> > > +#define _DPD_AUX_CH_MUTEX (dev_priv->info.display_mmio_offset + 0x6432C)
> > >
> > > #define _DPF_AUX_CH_CTL (dev_priv->info.display_mmio_offset + 0x64510)
> > > #define _DPF_AUX_CH_DATA1 (dev_priv->info.display_mmio_offset + 0x64514)
> > > @@ -5346,6 +5350,7 @@ enum {
> > > #define _DPF_AUX_CH_DATA3 (dev_priv->info.display_mmio_offset + 0x6451c)
> > > #define _DPF_AUX_CH_DATA4 (dev_priv->info.display_mmio_offset + 0x64520)
> > > #define _DPF_AUX_CH_DATA5 (dev_priv->info.display_mmio_offset + 0x64524)
> > > +#define _DPF_AUX_CH_MUTEX (dev_priv->info.display_mmio_offset + 0x6452C)
> > >
> > > #define DP_AUX_CH_CTL(port) _MMIO_PORT(port, _DPA_AUX_CH_CTL, _DPB_AUX_CH_CTL)
> > > #define DP_AUX_CH_DATA(port, i) _MMIO(_PORT(port, _DPA_AUX_CH_DATA1, _DPB_AUX_CH_DATA1) + (i) * 4) /* 5 registers */
> > > @@ -5378,6 +5383,10 @@ enum {
> > > #define DP_AUX_CH_CTL_FW_SYNC_PULSE_SKL(c) (((c) - 1) << 5)
> > > #define DP_AUX_CH_CTL_SYNC_PULSE_SKL(c) ((c) - 1)
> > >
> > > +#define DP_AUX_CH_MUTEX(port) _MMIO_PORT(port, _DPA_AUX_CH_MUTEX, _DPB_AUX_CH_MUTEX)
> > > +#define DP_AUX_CH_MUTEX_ENABLE (1 << 31)
> > > +#define DP_AUX_CH_MUTEX_STATUS (1 << 30)
> > > +
> > > /*
> > > * Computing GMCH M and N values for the Display Port link
> > > *
> > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > > index f20b25f98e5a..af07563bafba 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > @@ -1081,6 +1081,45 @@ static uint32_t intel_dp_get_aux_send_ctl(struct intel_dp *intel_dp,
> > > aux_clock_divider);
> > > }
> > >
> > > +static bool intel_dp_aux_ch_trylock(struct intel_dp *intel_dp)
> > > +{
> > > + struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> > > + struct drm_i915_private *dev_priv =
> > > + to_i915(intel_dig_port->base.base.dev);
> > > +
> > > + if (INTEL_GEN(dev_priv) < 9)
> > > + return true;
> > > +
> > > + I915_WRITE(intel_dp->aux_ch_mutex_reg, DP_AUX_CH_MUTEX_ENABLE);
> > > +
> > > + /* Spec says that mutex is acquired when status bit is read as unset,
> > > + * if set it should be read again after 500usec. Here waiting for
> > > + * 2msec or 4 tries before give up.
> > > + */
> > > + if (__intel_wait_for_register(dev_priv, intel_dp->aux_ch_mutex_reg,
> > > + DP_AUX_CH_MUTEX_STATUS, 0, 500, 2,
> > > + NULL)) {
> > > + DRM_WARN("dp_aux_ch port locked for too long");
> > > + return false;
> > > + }
> > > +
> > > + return true;
> > > +}
> > > +
> > > +static void intel_dp_aux_ch_unlock(struct intel_dp *intel_dp)
> > > +{
> > > + struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> > > + struct drm_i915_private *dev_priv =
> > > + to_i915(intel_dig_port->base.base.dev);
> > > +
> > > + if (INTEL_GEN(dev_priv) < 9)
> > > + return;
> > > +
> > > + /* setting the status bit releases the mutex + keep mutex enabled */
> > > + I915_WRITE(intel_dp->aux_ch_mutex_reg, DP_AUX_CH_MUTEX_ENABLE |
> > > + DP_AUX_CH_MUTEX_STATUS);
> > > +}
> > > +
> > > static int
> > > intel_dp_aux_ch(struct intel_dp *intel_dp,
> > > const uint8_t *send, int send_bytes,
> > > @@ -1115,6 +1154,11 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
> > >
> > > intel_dp_check_edp(intel_dp);
> > >
> > > + if (!intel_dp_aux_ch_trylock(intel_dp)) {
> > > + ret = -EBUSY;
> > > + goto out_locked;
> > > + }
> > > +
> > > /* Try to wait for any previous AUX channel activity */
> > > for (try = 0; try < 3; try++) {
> > > status = I915_READ_NOTRACE(ch_ctl);
> > > @@ -1244,6 +1288,8 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
> > >
> > > ret = recv_bytes;
> > > out:
> > > + intel_dp_aux_ch_unlock(intel_dp);
> > > +out_locked:
> > > pm_qos_update_request(&dev_priv->pm_qos, PM_QOS_DEFAULT_VALUE);
> > >
> > > if (vdd)
> > > @@ -1496,6 +1542,10 @@ static void intel_aux_reg_init(struct intel_dp *intel_dp)
> > > intel_dp->aux_ch_ctl_reg = intel_aux_ctl_reg(dev_priv, port);
> > > for (i = 0; i < ARRAY_SIZE(intel_dp->aux_ch_data_reg); i++)
> > > intel_dp->aux_ch_data_reg[i] = intel_aux_data_reg(dev_priv, port, i);
> > > + if (INTEL_INFO(dev_priv)->gen >= 9) {
> > > + intel_dp->aux_ch_mutex_reg = DP_AUX_CH_MUTEX(port);
> > > + I915_WRITE(intel_dp->aux_ch_mutex_reg, DP_AUX_CH_MUTEX_ENABLE);
> > > + }
> > > }
> > >
> > > static void
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > > index 50874f4035cf..0c88657d6066 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -1043,6 +1043,7 @@ struct intel_dp {
> > > i915_reg_t output_reg;
> > > i915_reg_t aux_ch_ctl_reg;
> > > i915_reg_t aux_ch_data_reg[5];
> > > + i915_reg_t aux_ch_mutex_reg;
> > > uint32_t DP;
> > > int link_rate;
> > > uint8_t lane_count;
> > > --
> > > 2.16.2
> > >
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
> > --
> > Ville Syrjälä
> > Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v2 1/1] drm/i915: Add and enable DP AUX CH mutex
2018-02-22 19:57 ` Pandiyan, Dhinakaran
@ 2018-02-22 20:13 ` Rodrigo Vivi
2018-02-22 20:25 ` Pandiyan, Dhinakaran
0 siblings, 1 reply; 16+ messages in thread
From: Rodrigo Vivi @ 2018-02-22 20:13 UTC (permalink / raw)
To: Pandiyan, Dhinakaran; +Cc: intel-gfx
On Thu, Feb 22, 2018 at 11:57:03AM -0800, Pandiyan, Dhinakaran wrote:
> On Thu, 2018-02-22 at 11:34 -0800, Rodrigo Vivi wrote:
> > On Thu, Feb 22, 2018 at 08:59:49PM +0200, Ville Syrjälä wrote:
> > > On Tue, Feb 20, 2018 at 06:23:47PM -0800, José Roberto de Souza wrote:
> > > > When PSR/PSR2/GTC is enabled hardware can do AUX transactions by it
> > > > self, so lets use the mutex register that is available in gen9+ to
> > > > avoid concurrent access by hardware and driver.
> > > >
> > > > Reference: https://01.org/sites/default/files/documentation/intel-gfx-prm-osrc-skl-vol12-display.pdf
> > > > Page 198 - AUX programming sequence
> > >
> > > BTW since HSW/BDW don't have the mutex we should really be disabling PSR
> > > around aux ch usage on those platforms. Not sure how big of a locking
> > > inversion exercise it would be to make that happen though.
> >
> > True. But I believe that can be handled differently.
> > Probably with Aux power domains blocking PSR off "domain".
> > Something similar to DC off.
> >
> > I actually had patches for that lost somewhere... :(
> >
>
> Just a note, the spec explicitly says we should disable PSR around AUX
> transactions for BDW (and HSW iirc).
>
> About using the power domain infra, I am not quite sure. The _get() and
> _put() idiom makes sense when we want reference counting to handle
> multiple consumers that want PSR to be disabled. Does anything else need
> to enable/disable PSR?
VLV/CHV vblanks?! :/
>
>
> > >
> > > >
> > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > > ---
> > > > drivers/gpu/drm/i915/i915_reg.h | 9 ++++++++
> > > > drivers/gpu/drm/i915/intel_dp.c | 50 ++++++++++++++++++++++++++++++++++++++++
> > > > drivers/gpu/drm/i915/intel_drv.h | 1 +
> > > > 3 files changed, 60 insertions(+)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > > > index 1412abcb27d4..a62e3c1badab 100644
> > > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > > @@ -5318,6 +5318,7 @@ enum {
> > > > #define _DPA_AUX_CH_DATA3 (dev_priv->info.display_mmio_offset + 0x6401c)
> > > > #define _DPA_AUX_CH_DATA4 (dev_priv->info.display_mmio_offset + 0x64020)
> > > > #define _DPA_AUX_CH_DATA5 (dev_priv->info.display_mmio_offset + 0x64024)
> > > > +#define _DPA_AUX_CH_MUTEX (dev_priv->info.display_mmio_offset + 0x6402C)
> > > >
> > > > #define _DPB_AUX_CH_CTL (dev_priv->info.display_mmio_offset + 0x64110)
> > > > #define _DPB_AUX_CH_DATA1 (dev_priv->info.display_mmio_offset + 0x64114)
> > > > @@ -5325,6 +5326,7 @@ enum {
> > > > #define _DPB_AUX_CH_DATA3 (dev_priv->info.display_mmio_offset + 0x6411c)
> > > > #define _DPB_AUX_CH_DATA4 (dev_priv->info.display_mmio_offset + 0x64120)
> > > > #define _DPB_AUX_CH_DATA5 (dev_priv->info.display_mmio_offset + 0x64124)
> > > > +#define _DPB_AUX_CH_MUTEX (dev_priv->info.display_mmio_offset + 0x6412C)
> > > >
> > > > #define _DPC_AUX_CH_CTL (dev_priv->info.display_mmio_offset + 0x64210)
> > > > #define _DPC_AUX_CH_DATA1 (dev_priv->info.display_mmio_offset + 0x64214)
> > > > @@ -5332,6 +5334,7 @@ enum {
> > > > #define _DPC_AUX_CH_DATA3 (dev_priv->info.display_mmio_offset + 0x6421c)
> > > > #define _DPC_AUX_CH_DATA4 (dev_priv->info.display_mmio_offset + 0x64220)
> > > > #define _DPC_AUX_CH_DATA5 (dev_priv->info.display_mmio_offset + 0x64224)
> > > > +#define _DPC_AUX_CH_MUTEX (dev_priv->info.display_mmio_offset + 0x6422C)
> > > >
> > > > #define _DPD_AUX_CH_CTL (dev_priv->info.display_mmio_offset + 0x64310)
> > > > #define _DPD_AUX_CH_DATA1 (dev_priv->info.display_mmio_offset + 0x64314)
> > > > @@ -5339,6 +5342,7 @@ enum {
> > > > #define _DPD_AUX_CH_DATA3 (dev_priv->info.display_mmio_offset + 0x6431c)
> > > > #define _DPD_AUX_CH_DATA4 (dev_priv->info.display_mmio_offset + 0x64320)
> > > > #define _DPD_AUX_CH_DATA5 (dev_priv->info.display_mmio_offset + 0x64324)
> > > > +#define _DPD_AUX_CH_MUTEX (dev_priv->info.display_mmio_offset + 0x6432C)
> > > >
> > > > #define _DPF_AUX_CH_CTL (dev_priv->info.display_mmio_offset + 0x64510)
> > > > #define _DPF_AUX_CH_DATA1 (dev_priv->info.display_mmio_offset + 0x64514)
> > > > @@ -5346,6 +5350,7 @@ enum {
> > > > #define _DPF_AUX_CH_DATA3 (dev_priv->info.display_mmio_offset + 0x6451c)
> > > > #define _DPF_AUX_CH_DATA4 (dev_priv->info.display_mmio_offset + 0x64520)
> > > > #define _DPF_AUX_CH_DATA5 (dev_priv->info.display_mmio_offset + 0x64524)
> > > > +#define _DPF_AUX_CH_MUTEX (dev_priv->info.display_mmio_offset + 0x6452C)
> > > >
> > > > #define DP_AUX_CH_CTL(port) _MMIO_PORT(port, _DPA_AUX_CH_CTL, _DPB_AUX_CH_CTL)
> > > > #define DP_AUX_CH_DATA(port, i) _MMIO(_PORT(port, _DPA_AUX_CH_DATA1, _DPB_AUX_CH_DATA1) + (i) * 4) /* 5 registers */
> > > > @@ -5378,6 +5383,10 @@ enum {
> > > > #define DP_AUX_CH_CTL_FW_SYNC_PULSE_SKL(c) (((c) - 1) << 5)
> > > > #define DP_AUX_CH_CTL_SYNC_PULSE_SKL(c) ((c) - 1)
> > > >
> > > > +#define DP_AUX_CH_MUTEX(port) _MMIO_PORT(port, _DPA_AUX_CH_MUTEX, _DPB_AUX_CH_MUTEX)
> > > > +#define DP_AUX_CH_MUTEX_ENABLE (1 << 31)
> > > > +#define DP_AUX_CH_MUTEX_STATUS (1 << 30)
> > > > +
> > > > /*
> > > > * Computing GMCH M and N values for the Display Port link
> > > > *
> > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > > > index f20b25f98e5a..af07563bafba 100644
> > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > @@ -1081,6 +1081,45 @@ static uint32_t intel_dp_get_aux_send_ctl(struct intel_dp *intel_dp,
> > > > aux_clock_divider);
> > > > }
> > > >
> > > > +static bool intel_dp_aux_ch_trylock(struct intel_dp *intel_dp)
> > > > +{
> > > > + struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> > > > + struct drm_i915_private *dev_priv =
> > > > + to_i915(intel_dig_port->base.base.dev);
> > > > +
> > > > + if (INTEL_GEN(dev_priv) < 9)
> > > > + return true;
> > > > +
> > > > + I915_WRITE(intel_dp->aux_ch_mutex_reg, DP_AUX_CH_MUTEX_ENABLE);
> > > > +
> > > > + /* Spec says that mutex is acquired when status bit is read as unset,
> > > > + * if set it should be read again after 500usec. Here waiting for
> > > > + * 2msec or 4 tries before give up.
> > > > + */
> > > > + if (__intel_wait_for_register(dev_priv, intel_dp->aux_ch_mutex_reg,
> > > > + DP_AUX_CH_MUTEX_STATUS, 0, 500, 2,
> > > > + NULL)) {
> > > > + DRM_WARN("dp_aux_ch port locked for too long");
> > > > + return false;
> > > > + }
> > > > +
> > > > + return true;
> > > > +}
> > > > +
> > > > +static void intel_dp_aux_ch_unlock(struct intel_dp *intel_dp)
> > > > +{
> > > > + struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> > > > + struct drm_i915_private *dev_priv =
> > > > + to_i915(intel_dig_port->base.base.dev);
> > > > +
> > > > + if (INTEL_GEN(dev_priv) < 9)
> > > > + return;
> > > > +
> > > > + /* setting the status bit releases the mutex + keep mutex enabled */
> > > > + I915_WRITE(intel_dp->aux_ch_mutex_reg, DP_AUX_CH_MUTEX_ENABLE |
> > > > + DP_AUX_CH_MUTEX_STATUS);
> > > > +}
> > > > +
> > > > static int
> > > > intel_dp_aux_ch(struct intel_dp *intel_dp,
> > > > const uint8_t *send, int send_bytes,
> > > > @@ -1115,6 +1154,11 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
> > > >
> > > > intel_dp_check_edp(intel_dp);
> > > >
> > > > + if (!intel_dp_aux_ch_trylock(intel_dp)) {
> > > > + ret = -EBUSY;
> > > > + goto out_locked;
> > > > + }
> > > > +
> > > > /* Try to wait for any previous AUX channel activity */
> > > > for (try = 0; try < 3; try++) {
> > > > status = I915_READ_NOTRACE(ch_ctl);
> > > > @@ -1244,6 +1288,8 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
> > > >
> > > > ret = recv_bytes;
> > > > out:
> > > > + intel_dp_aux_ch_unlock(intel_dp);
> > > > +out_locked:
> > > > pm_qos_update_request(&dev_priv->pm_qos, PM_QOS_DEFAULT_VALUE);
> > > >
> > > > if (vdd)
> > > > @@ -1496,6 +1542,10 @@ static void intel_aux_reg_init(struct intel_dp *intel_dp)
> > > > intel_dp->aux_ch_ctl_reg = intel_aux_ctl_reg(dev_priv, port);
> > > > for (i = 0; i < ARRAY_SIZE(intel_dp->aux_ch_data_reg); i++)
> > > > intel_dp->aux_ch_data_reg[i] = intel_aux_data_reg(dev_priv, port, i);
> > > > + if (INTEL_INFO(dev_priv)->gen >= 9) {
> > > > + intel_dp->aux_ch_mutex_reg = DP_AUX_CH_MUTEX(port);
> > > > + I915_WRITE(intel_dp->aux_ch_mutex_reg, DP_AUX_CH_MUTEX_ENABLE);
> > > > + }
> > > > }
> > > >
> > > > static void
> > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > > > index 50874f4035cf..0c88657d6066 100644
> > > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > > @@ -1043,6 +1043,7 @@ struct intel_dp {
> > > > i915_reg_t output_reg;
> > > > i915_reg_t aux_ch_ctl_reg;
> > > > i915_reg_t aux_ch_data_reg[5];
> > > > + i915_reg_t aux_ch_mutex_reg;
> > > > uint32_t DP;
> > > > int link_rate;
> > > > uint8_t lane_count;
> > > > --
> > > > 2.16.2
> > > >
> > > > _______________________________________________
> > > > Intel-gfx mailing list
> > > > Intel-gfx@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > >
> > > --
> > > Ville Syrjälä
> > > Intel OTC
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v2 1/1] drm/i915: Add and enable DP AUX CH mutex
2018-02-22 20:13 ` Rodrigo Vivi
@ 2018-02-22 20:25 ` Pandiyan, Dhinakaran
2018-02-22 20:47 ` Rodrigo Vivi
0 siblings, 1 reply; 16+ messages in thread
From: Pandiyan, Dhinakaran @ 2018-02-22 20:25 UTC (permalink / raw)
To: Vivi, Rodrigo; +Cc: intel-gfx
On Thu, 2018-02-22 at 12:13 -0800, Rodrigo Vivi wrote:
> On Thu, Feb 22, 2018 at 11:57:03AM -0800, Pandiyan, Dhinakaran wrote:
> > On Thu, 2018-02-22 at 11:34 -0800, Rodrigo Vivi wrote:
> > > On Thu, Feb 22, 2018 at 08:59:49PM +0200, Ville Syrjälä wrote:
> > > > On Tue, Feb 20, 2018 at 06:23:47PM -0800, José Roberto de Souza wrote:
> > > > > When PSR/PSR2/GTC is enabled hardware can do AUX transactions by it
> > > > > self, so lets use the mutex register that is available in gen9+ to
> > > > > avoid concurrent access by hardware and driver.
> > > > >
> > > > > Reference: https://01.org/sites/default/files/documentation/intel-gfx-prm-osrc-skl-vol12-display.pdf
> > > > > Page 198 - AUX programming sequence
> > > >
> > > > BTW since HSW/BDW don't have the mutex we should really be disabling PSR
> > > > around aux ch usage on those platforms. Not sure how big of a locking
> > > > inversion exercise it would be to make that happen though.
> > >
> > > True. But I believe that can be handled differently.
> > > Probably with Aux power domains blocking PSR off "domain".
> > > Something similar to DC off.
> > >
> > > I actually had patches for that lost somewhere... :(
> > >
> >
> > Just a note, the spec explicitly says we should disable PSR around AUX
> > transactions for BDW (and HSW iirc).
> >
> > About using the power domain infra, I am not quite sure. The _get() and
> > _put() idiom makes sense when we want reference counting to handle
> > multiple consumers that want PSR to be disabled. Does anything else need
> > to enable/disable PSR?
>
> VLV/CHV vblanks?! :/
>
I keep forgetting VLV/CHV, sorry. This brings us back to the whole
"calling blocking functions from atomic contexts" problem. Not sure how
to handle that.
Assuming we solve that problem, we should also probably avoid polluting
the power domain code and add reference counting for PSR. However, since
all this is not required for BDW/HSW, I am inclined towards a simpler
solution for BDW/HSW AUX usage.
> >
> >
> > > >
> > > > >
> > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > > > ---
> > > > > drivers/gpu/drm/i915/i915_reg.h | 9 ++++++++
> > > > > drivers/gpu/drm/i915/intel_dp.c | 50 ++++++++++++++++++++++++++++++++++++++++
> > > > > drivers/gpu/drm/i915/intel_drv.h | 1 +
> > > > > 3 files changed, 60 insertions(+)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > > > > index 1412abcb27d4..a62e3c1badab 100644
> > > > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > > > @@ -5318,6 +5318,7 @@ enum {
> > > > > #define _DPA_AUX_CH_DATA3 (dev_priv->info.display_mmio_offset + 0x6401c)
> > > > > #define _DPA_AUX_CH_DATA4 (dev_priv->info.display_mmio_offset + 0x64020)
> > > > > #define _DPA_AUX_CH_DATA5 (dev_priv->info.display_mmio_offset + 0x64024)
> > > > > +#define _DPA_AUX_CH_MUTEX (dev_priv->info.display_mmio_offset + 0x6402C)
> > > > >
> > > > > #define _DPB_AUX_CH_CTL (dev_priv->info.display_mmio_offset + 0x64110)
> > > > > #define _DPB_AUX_CH_DATA1 (dev_priv->info.display_mmio_offset + 0x64114)
> > > > > @@ -5325,6 +5326,7 @@ enum {
> > > > > #define _DPB_AUX_CH_DATA3 (dev_priv->info.display_mmio_offset + 0x6411c)
> > > > > #define _DPB_AUX_CH_DATA4 (dev_priv->info.display_mmio_offset + 0x64120)
> > > > > #define _DPB_AUX_CH_DATA5 (dev_priv->info.display_mmio_offset + 0x64124)
> > > > > +#define _DPB_AUX_CH_MUTEX (dev_priv->info.display_mmio_offset + 0x6412C)
> > > > >
> > > > > #define _DPC_AUX_CH_CTL (dev_priv->info.display_mmio_offset + 0x64210)
> > > > > #define _DPC_AUX_CH_DATA1 (dev_priv->info.display_mmio_offset + 0x64214)
> > > > > @@ -5332,6 +5334,7 @@ enum {
> > > > > #define _DPC_AUX_CH_DATA3 (dev_priv->info.display_mmio_offset + 0x6421c)
> > > > > #define _DPC_AUX_CH_DATA4 (dev_priv->info.display_mmio_offset + 0x64220)
> > > > > #define _DPC_AUX_CH_DATA5 (dev_priv->info.display_mmio_offset + 0x64224)
> > > > > +#define _DPC_AUX_CH_MUTEX (dev_priv->info.display_mmio_offset + 0x6422C)
> > > > >
> > > > > #define _DPD_AUX_CH_CTL (dev_priv->info.display_mmio_offset + 0x64310)
> > > > > #define _DPD_AUX_CH_DATA1 (dev_priv->info.display_mmio_offset + 0x64314)
> > > > > @@ -5339,6 +5342,7 @@ enum {
> > > > > #define _DPD_AUX_CH_DATA3 (dev_priv->info.display_mmio_offset + 0x6431c)
> > > > > #define _DPD_AUX_CH_DATA4 (dev_priv->info.display_mmio_offset + 0x64320)
> > > > > #define _DPD_AUX_CH_DATA5 (dev_priv->info.display_mmio_offset + 0x64324)
> > > > > +#define _DPD_AUX_CH_MUTEX (dev_priv->info.display_mmio_offset + 0x6432C)
> > > > >
> > > > > #define _DPF_AUX_CH_CTL (dev_priv->info.display_mmio_offset + 0x64510)
> > > > > #define _DPF_AUX_CH_DATA1 (dev_priv->info.display_mmio_offset + 0x64514)
> > > > > @@ -5346,6 +5350,7 @@ enum {
> > > > > #define _DPF_AUX_CH_DATA3 (dev_priv->info.display_mmio_offset + 0x6451c)
> > > > > #define _DPF_AUX_CH_DATA4 (dev_priv->info.display_mmio_offset + 0x64520)
> > > > > #define _DPF_AUX_CH_DATA5 (dev_priv->info.display_mmio_offset + 0x64524)
> > > > > +#define _DPF_AUX_CH_MUTEX (dev_priv->info.display_mmio_offset + 0x6452C)
> > > > >
> > > > > #define DP_AUX_CH_CTL(port) _MMIO_PORT(port, _DPA_AUX_CH_CTL, _DPB_AUX_CH_CTL)
> > > > > #define DP_AUX_CH_DATA(port, i) _MMIO(_PORT(port, _DPA_AUX_CH_DATA1, _DPB_AUX_CH_DATA1) + (i) * 4) /* 5 registers */
> > > > > @@ -5378,6 +5383,10 @@ enum {
> > > > > #define DP_AUX_CH_CTL_FW_SYNC_PULSE_SKL(c) (((c) - 1) << 5)
> > > > > #define DP_AUX_CH_CTL_SYNC_PULSE_SKL(c) ((c) - 1)
> > > > >
> > > > > +#define DP_AUX_CH_MUTEX(port) _MMIO_PORT(port, _DPA_AUX_CH_MUTEX, _DPB_AUX_CH_MUTEX)
> > > > > +#define DP_AUX_CH_MUTEX_ENABLE (1 << 31)
> > > > > +#define DP_AUX_CH_MUTEX_STATUS (1 << 30)
> > > > > +
> > > > > /*
> > > > > * Computing GMCH M and N values for the Display Port link
> > > > > *
> > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > > > > index f20b25f98e5a..af07563bafba 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > > @@ -1081,6 +1081,45 @@ static uint32_t intel_dp_get_aux_send_ctl(struct intel_dp *intel_dp,
> > > > > aux_clock_divider);
> > > > > }
> > > > >
> > > > > +static bool intel_dp_aux_ch_trylock(struct intel_dp *intel_dp)
> > > > > +{
> > > > > + struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> > > > > + struct drm_i915_private *dev_priv =
> > > > > + to_i915(intel_dig_port->base.base.dev);
> > > > > +
> > > > > + if (INTEL_GEN(dev_priv) < 9)
> > > > > + return true;
> > > > > +
> > > > > + I915_WRITE(intel_dp->aux_ch_mutex_reg, DP_AUX_CH_MUTEX_ENABLE);
> > > > > +
> > > > > + /* Spec says that mutex is acquired when status bit is read as unset,
> > > > > + * if set it should be read again after 500usec. Here waiting for
> > > > > + * 2msec or 4 tries before give up.
> > > > > + */
> > > > > + if (__intel_wait_for_register(dev_priv, intel_dp->aux_ch_mutex_reg,
> > > > > + DP_AUX_CH_MUTEX_STATUS, 0, 500, 2,
> > > > > + NULL)) {
> > > > > + DRM_WARN("dp_aux_ch port locked for too long");
> > > > > + return false;
> > > > > + }
> > > > > +
> > > > > + return true;
> > > > > +}
> > > > > +
> > > > > +static void intel_dp_aux_ch_unlock(struct intel_dp *intel_dp)
> > > > > +{
> > > > > + struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> > > > > + struct drm_i915_private *dev_priv =
> > > > > + to_i915(intel_dig_port->base.base.dev);
> > > > > +
> > > > > + if (INTEL_GEN(dev_priv) < 9)
> > > > > + return;
> > > > > +
> > > > > + /* setting the status bit releases the mutex + keep mutex enabled */
> > > > > + I915_WRITE(intel_dp->aux_ch_mutex_reg, DP_AUX_CH_MUTEX_ENABLE |
> > > > > + DP_AUX_CH_MUTEX_STATUS);
> > > > > +}
> > > > > +
> > > > > static int
> > > > > intel_dp_aux_ch(struct intel_dp *intel_dp,
> > > > > const uint8_t *send, int send_bytes,
> > > > > @@ -1115,6 +1154,11 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
> > > > >
> > > > > intel_dp_check_edp(intel_dp);
> > > > >
> > > > > + if (!intel_dp_aux_ch_trylock(intel_dp)) {
> > > > > + ret = -EBUSY;
> > > > > + goto out_locked;
> > > > > + }
> > > > > +
> > > > > /* Try to wait for any previous AUX channel activity */
> > > > > for (try = 0; try < 3; try++) {
> > > > > status = I915_READ_NOTRACE(ch_ctl);
> > > > > @@ -1244,6 +1288,8 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
> > > > >
> > > > > ret = recv_bytes;
> > > > > out:
> > > > > + intel_dp_aux_ch_unlock(intel_dp);
> > > > > +out_locked:
> > > > > pm_qos_update_request(&dev_priv->pm_qos, PM_QOS_DEFAULT_VALUE);
> > > > >
> > > > > if (vdd)
> > > > > @@ -1496,6 +1542,10 @@ static void intel_aux_reg_init(struct intel_dp *intel_dp)
> > > > > intel_dp->aux_ch_ctl_reg = intel_aux_ctl_reg(dev_priv, port);
> > > > > for (i = 0; i < ARRAY_SIZE(intel_dp->aux_ch_data_reg); i++)
> > > > > intel_dp->aux_ch_data_reg[i] = intel_aux_data_reg(dev_priv, port, i);
> > > > > + if (INTEL_INFO(dev_priv)->gen >= 9) {
> > > > > + intel_dp->aux_ch_mutex_reg = DP_AUX_CH_MUTEX(port);
> > > > > + I915_WRITE(intel_dp->aux_ch_mutex_reg, DP_AUX_CH_MUTEX_ENABLE);
> > > > > + }
> > > > > }
> > > > >
> > > > > static void
> > > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > > > > index 50874f4035cf..0c88657d6066 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > > > @@ -1043,6 +1043,7 @@ struct intel_dp {
> > > > > i915_reg_t output_reg;
> > > > > i915_reg_t aux_ch_ctl_reg;
> > > > > i915_reg_t aux_ch_data_reg[5];
> > > > > + i915_reg_t aux_ch_mutex_reg;
> > > > > uint32_t DP;
> > > > > int link_rate;
> > > > > uint8_t lane_count;
> > > > > --
> > > > > 2.16.2
> > > > >
> > > > > _______________________________________________
> > > > > Intel-gfx mailing list
> > > > > Intel-gfx@lists.freedesktop.org
> > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > > >
> > > > --
> > > > Ville Syrjälä
> > > > Intel OTC
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v2 1/1] drm/i915: Add and enable DP AUX CH mutex
2018-02-22 20:25 ` Pandiyan, Dhinakaran
@ 2018-02-22 20:47 ` Rodrigo Vivi
0 siblings, 0 replies; 16+ messages in thread
From: Rodrigo Vivi @ 2018-02-22 20:47 UTC (permalink / raw)
To: Pandiyan, Dhinakaran; +Cc: intel-gfx
On Thu, Feb 22, 2018 at 12:25:46PM -0800, Pandiyan, Dhinakaran wrote:
> On Thu, 2018-02-22 at 12:13 -0800, Rodrigo Vivi wrote:
> > On Thu, Feb 22, 2018 at 11:57:03AM -0800, Pandiyan, Dhinakaran wrote:
> > > On Thu, 2018-02-22 at 11:34 -0800, Rodrigo Vivi wrote:
> > > > On Thu, Feb 22, 2018 at 08:59:49PM +0200, Ville Syrjälä wrote:
> > > > > On Tue, Feb 20, 2018 at 06:23:47PM -0800, José Roberto de Souza wrote:
> > > > > > When PSR/PSR2/GTC is enabled hardware can do AUX transactions by it
> > > > > > self, so lets use the mutex register that is available in gen9+ to
> > > > > > avoid concurrent access by hardware and driver.
> > > > > >
> > > > > > Reference: https://01.org/sites/default/files/documentation/intel-gfx-prm-osrc-skl-vol12-display.pdf
> > > > > > Page 198 - AUX programming sequence
> > > > >
> > > > > BTW since HSW/BDW don't have the mutex we should really be disabling PSR
> > > > > around aux ch usage on those platforms. Not sure how big of a locking
> > > > > inversion exercise it would be to make that happen though.
> > > >
> > > > True. But I believe that can be handled differently.
> > > > Probably with Aux power domains blocking PSR off "domain".
> > > > Something similar to DC off.
> > > >
> > > > I actually had patches for that lost somewhere... :(
> > > >
> > >
> > > Just a note, the spec explicitly says we should disable PSR around AUX
> > > transactions for BDW (and HSW iirc).
> > >
> > > About using the power domain infra, I am not quite sure. The _get() and
> > > _put() idiom makes sense when we want reference counting to handle
> > > multiple consumers that want PSR to be disabled. Does anything else need
> > > to enable/disable PSR?
> >
> > VLV/CHV vblanks?! :/
> >
>
> I keep forgetting VLV/CHV, sorry. This brings us back to the whole
> "calling blocking functions from atomic contexts" problem. Not sure how
> to handle that.
>
> Assuming we solve that problem, we should also probably avoid polluting
> the power domain code and add reference counting for PSR. However, since
> all this is not required for BDW/HSW, I am inclined towards a simpler
> solution for BDW/HSW AUX usage.
I agree. Then probably the simpler solution could than be applied on
VLV/CHV... who knows.
anyways separated from this patch here... let's keep this one clean.
>
>
> > >
> > >
> > > > >
> > > > > >
> > > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > > > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > > > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > > > > ---
> > > > > > drivers/gpu/drm/i915/i915_reg.h | 9 ++++++++
> > > > > > drivers/gpu/drm/i915/intel_dp.c | 50 ++++++++++++++++++++++++++++++++++++++++
> > > > > > drivers/gpu/drm/i915/intel_drv.h | 1 +
> > > > > > 3 files changed, 60 insertions(+)
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > > > > > index 1412abcb27d4..a62e3c1badab 100644
> > > > > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > > > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > > > > @@ -5318,6 +5318,7 @@ enum {
> > > > > > #define _DPA_AUX_CH_DATA3 (dev_priv->info.display_mmio_offset + 0x6401c)
> > > > > > #define _DPA_AUX_CH_DATA4 (dev_priv->info.display_mmio_offset + 0x64020)
> > > > > > #define _DPA_AUX_CH_DATA5 (dev_priv->info.display_mmio_offset + 0x64024)
> > > > > > +#define _DPA_AUX_CH_MUTEX (dev_priv->info.display_mmio_offset + 0x6402C)
> > > > > >
> > > > > > #define _DPB_AUX_CH_CTL (dev_priv->info.display_mmio_offset + 0x64110)
> > > > > > #define _DPB_AUX_CH_DATA1 (dev_priv->info.display_mmio_offset + 0x64114)
> > > > > > @@ -5325,6 +5326,7 @@ enum {
> > > > > > #define _DPB_AUX_CH_DATA3 (dev_priv->info.display_mmio_offset + 0x6411c)
> > > > > > #define _DPB_AUX_CH_DATA4 (dev_priv->info.display_mmio_offset + 0x64120)
> > > > > > #define _DPB_AUX_CH_DATA5 (dev_priv->info.display_mmio_offset + 0x64124)
> > > > > > +#define _DPB_AUX_CH_MUTEX (dev_priv->info.display_mmio_offset + 0x6412C)
> > > > > >
> > > > > > #define _DPC_AUX_CH_CTL (dev_priv->info.display_mmio_offset + 0x64210)
> > > > > > #define _DPC_AUX_CH_DATA1 (dev_priv->info.display_mmio_offset + 0x64214)
> > > > > > @@ -5332,6 +5334,7 @@ enum {
> > > > > > #define _DPC_AUX_CH_DATA3 (dev_priv->info.display_mmio_offset + 0x6421c)
> > > > > > #define _DPC_AUX_CH_DATA4 (dev_priv->info.display_mmio_offset + 0x64220)
> > > > > > #define _DPC_AUX_CH_DATA5 (dev_priv->info.display_mmio_offset + 0x64224)
> > > > > > +#define _DPC_AUX_CH_MUTEX (dev_priv->info.display_mmio_offset + 0x6422C)
> > > > > >
> > > > > > #define _DPD_AUX_CH_CTL (dev_priv->info.display_mmio_offset + 0x64310)
> > > > > > #define _DPD_AUX_CH_DATA1 (dev_priv->info.display_mmio_offset + 0x64314)
> > > > > > @@ -5339,6 +5342,7 @@ enum {
> > > > > > #define _DPD_AUX_CH_DATA3 (dev_priv->info.display_mmio_offset + 0x6431c)
> > > > > > #define _DPD_AUX_CH_DATA4 (dev_priv->info.display_mmio_offset + 0x64320)
> > > > > > #define _DPD_AUX_CH_DATA5 (dev_priv->info.display_mmio_offset + 0x64324)
> > > > > > +#define _DPD_AUX_CH_MUTEX (dev_priv->info.display_mmio_offset + 0x6432C)
> > > > > >
> > > > > > #define _DPF_AUX_CH_CTL (dev_priv->info.display_mmio_offset + 0x64510)
> > > > > > #define _DPF_AUX_CH_DATA1 (dev_priv->info.display_mmio_offset + 0x64514)
> > > > > > @@ -5346,6 +5350,7 @@ enum {
> > > > > > #define _DPF_AUX_CH_DATA3 (dev_priv->info.display_mmio_offset + 0x6451c)
> > > > > > #define _DPF_AUX_CH_DATA4 (dev_priv->info.display_mmio_offset + 0x64520)
> > > > > > #define _DPF_AUX_CH_DATA5 (dev_priv->info.display_mmio_offset + 0x64524)
> > > > > > +#define _DPF_AUX_CH_MUTEX (dev_priv->info.display_mmio_offset + 0x6452C)
> > > > > >
> > > > > > #define DP_AUX_CH_CTL(port) _MMIO_PORT(port, _DPA_AUX_CH_CTL, _DPB_AUX_CH_CTL)
> > > > > > #define DP_AUX_CH_DATA(port, i) _MMIO(_PORT(port, _DPA_AUX_CH_DATA1, _DPB_AUX_CH_DATA1) + (i) * 4) /* 5 registers */
> > > > > > @@ -5378,6 +5383,10 @@ enum {
> > > > > > #define DP_AUX_CH_CTL_FW_SYNC_PULSE_SKL(c) (((c) - 1) << 5)
> > > > > > #define DP_AUX_CH_CTL_SYNC_PULSE_SKL(c) ((c) - 1)
> > > > > >
> > > > > > +#define DP_AUX_CH_MUTEX(port) _MMIO_PORT(port, _DPA_AUX_CH_MUTEX, _DPB_AUX_CH_MUTEX)
> > > > > > +#define DP_AUX_CH_MUTEX_ENABLE (1 << 31)
> > > > > > +#define DP_AUX_CH_MUTEX_STATUS (1 << 30)
> > > > > > +
> > > > > > /*
> > > > > > * Computing GMCH M and N values for the Display Port link
> > > > > > *
> > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > > > > > index f20b25f98e5a..af07563bafba 100644
> > > > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > > > @@ -1081,6 +1081,45 @@ static uint32_t intel_dp_get_aux_send_ctl(struct intel_dp *intel_dp,
> > > > > > aux_clock_divider);
> > > > > > }
> > > > > >
> > > > > > +static bool intel_dp_aux_ch_trylock(struct intel_dp *intel_dp)
> > > > > > +{
> > > > > > + struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> > > > > > + struct drm_i915_private *dev_priv =
> > > > > > + to_i915(intel_dig_port->base.base.dev);
> > > > > > +
> > > > > > + if (INTEL_GEN(dev_priv) < 9)
> > > > > > + return true;
> > > > > > +
> > > > > > + I915_WRITE(intel_dp->aux_ch_mutex_reg, DP_AUX_CH_MUTEX_ENABLE);
> > > > > > +
> > > > > > + /* Spec says that mutex is acquired when status bit is read as unset,
> > > > > > + * if set it should be read again after 500usec. Here waiting for
> > > > > > + * 2msec or 4 tries before give up.
> > > > > > + */
> > > > > > + if (__intel_wait_for_register(dev_priv, intel_dp->aux_ch_mutex_reg,
> > > > > > + DP_AUX_CH_MUTEX_STATUS, 0, 500, 2,
> > > > > > + NULL)) {
> > > > > > + DRM_WARN("dp_aux_ch port locked for too long");
> > > > > > + return false;
> > > > > > + }
> > > > > > +
> > > > > > + return true;
> > > > > > +}
> > > > > > +
> > > > > > +static void intel_dp_aux_ch_unlock(struct intel_dp *intel_dp)
> > > > > > +{
> > > > > > + struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> > > > > > + struct drm_i915_private *dev_priv =
> > > > > > + to_i915(intel_dig_port->base.base.dev);
> > > > > > +
> > > > > > + if (INTEL_GEN(dev_priv) < 9)
> > > > > > + return;
> > > > > > +
> > > > > > + /* setting the status bit releases the mutex + keep mutex enabled */
> > > > > > + I915_WRITE(intel_dp->aux_ch_mutex_reg, DP_AUX_CH_MUTEX_ENABLE |
> > > > > > + DP_AUX_CH_MUTEX_STATUS);
> > > > > > +}
> > > > > > +
> > > > > > static int
> > > > > > intel_dp_aux_ch(struct intel_dp *intel_dp,
> > > > > > const uint8_t *send, int send_bytes,
> > > > > > @@ -1115,6 +1154,11 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
> > > > > >
> > > > > > intel_dp_check_edp(intel_dp);
> > > > > >
> > > > > > + if (!intel_dp_aux_ch_trylock(intel_dp)) {
> > > > > > + ret = -EBUSY;
> > > > > > + goto out_locked;
> > > > > > + }
> > > > > > +
> > > > > > /* Try to wait for any previous AUX channel activity */
> > > > > > for (try = 0; try < 3; try++) {
> > > > > > status = I915_READ_NOTRACE(ch_ctl);
> > > > > > @@ -1244,6 +1288,8 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
> > > > > >
> > > > > > ret = recv_bytes;
> > > > > > out:
> > > > > > + intel_dp_aux_ch_unlock(intel_dp);
> > > > > > +out_locked:
> > > > > > pm_qos_update_request(&dev_priv->pm_qos, PM_QOS_DEFAULT_VALUE);
> > > > > >
> > > > > > if (vdd)
> > > > > > @@ -1496,6 +1542,10 @@ static void intel_aux_reg_init(struct intel_dp *intel_dp)
> > > > > > intel_dp->aux_ch_ctl_reg = intel_aux_ctl_reg(dev_priv, port);
> > > > > > for (i = 0; i < ARRAY_SIZE(intel_dp->aux_ch_data_reg); i++)
> > > > > > intel_dp->aux_ch_data_reg[i] = intel_aux_data_reg(dev_priv, port, i);
> > > > > > + if (INTEL_INFO(dev_priv)->gen >= 9) {
> > > > > > + intel_dp->aux_ch_mutex_reg = DP_AUX_CH_MUTEX(port);
> > > > > > + I915_WRITE(intel_dp->aux_ch_mutex_reg, DP_AUX_CH_MUTEX_ENABLE);
> > > > > > + }
> > > > > > }
> > > > > >
> > > > > > static void
> > > > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > > > > > index 50874f4035cf..0c88657d6066 100644
> > > > > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > > > > @@ -1043,6 +1043,7 @@ struct intel_dp {
> > > > > > i915_reg_t output_reg;
> > > > > > i915_reg_t aux_ch_ctl_reg;
> > > > > > i915_reg_t aux_ch_data_reg[5];
> > > > > > + i915_reg_t aux_ch_mutex_reg;
> > > > > > uint32_t DP;
> > > > > > int link_rate;
> > > > > > uint8_t lane_count;
> > > > > > --
> > > > > > 2.16.2
> > > > > >
> > > > > > _______________________________________________
> > > > > > Intel-gfx mailing list
> > > > > > Intel-gfx@lists.freedesktop.org
> > > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > > > >
> > > > > --
> > > > > Ville Syrjälä
> > > > > Intel OTC
> > > > _______________________________________________
> > > > Intel-gfx mailing list
> > > > Intel-gfx@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 16+ messages in thread