Re: [Intel-gfx] [PATCH 1/5] drm/i915: add and enable DP AUX CH mutex
On Wed, 14 Feb 2018, Rodrigo Vivi wrote:
> On Wed, Feb 14, 2018 at 09:47:00AM +, Jani Nikula wrote:
>> On Tue, 13 Feb 2018, José Roberto de Souza wrote:
>> > When PSR/PSR2 is enabled hardware can do AUX transactions by it self, so
>> > lets use the mutex register that is available in gen9+ to avoid concurrent
>> > access by hardware and driver.
>> >
>> > Reference:
>> > https://01.org/sites/default/files/documentation/intel-gfx-prm-osrc-skl-vol12-display.pdf
>> > Page 198 - AUX programming sequence
>> >
>> > As gen < 9 don't have the mutex PSR needs to be disabled, that is why the
>> > 'else TODO' was added. This is also backed by spec:
>> > "DDI A AUX channel transactions must not be sent while SRD is enabled.
>> > SRD must be completely disabled before a DDI A AUX channel transaction can
>> > be sent." BSpec: 7530
>>
>> We explicitly rejected the use of the hw mutex way back when Skylake was
>> enabled. At that time the understanding was that the hw will try to do
>> the arbitration itself. Has something changed?
>
> By that time spec wasn't clear about the actual need or its reliability.
> Nowdays spec is clear that that is absolutely needed for PSR/PSR2/GTC.
>
> Also we were using a lot of SW tracking and nowadays we are moving PSR
> to use more HW tracking.
>
> So I believe we should add this to get a stable PSR.
Fair enough. The patch at hand needs a bunch of work though.
BR,
Jani.
>
>>
>> Some comments inline.
>>
>> >
>> > Cc: Rodrigo Vivi
>> > Signed-off-by: José Roberto de Souza
>> > ---
>> > drivers/gpu/drm/i915/i915_reg.h | 9
>> > drivers/gpu/drm/i915/intel_dp.c | 48
>> >
>> > drivers/gpu/drm/i915/intel_drv.h | 1 +
>> > drivers/gpu/drm/i915/intel_psr.c | 6 +
>> > 4 files changed, 64 insertions(+)
>> >
>> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
>> > b/drivers/gpu/drm/i915/i915_reg.h
>> > index f6afa5e5e7c1..5d7736117cb9 100644
>> > --- a/drivers/gpu/drm/i915/i915_reg.h
>> > +++ b/drivers/gpu/drm/i915/i915_reg.h
>> > @@ -5318,6 +5318,7 @@ enum {
>> > #define _DPA_AUX_CH_DATA3 (dev_priv->info.display_mmio_offset + 0x6401c)
>> > #define _DPA_AUX_CH_DATA4 (dev_priv->info.display_mmio_offset + 0x64020)
>> > #define _DPA_AUX_CH_DATA5 (dev_priv->info.display_mmio_offset + 0x64024)
>> > +#define _DPA_AUX_CH_MUTEX (dev_priv->info.display_mmio_offset + 0x6402C)
>> >
>> > #define _DPB_AUX_CH_CTL (dev_priv->info.display_mmio_offset +
>> > 0x64110)
>> > #define _DPB_AUX_CH_DATA1 (dev_priv->info.display_mmio_offset + 0x64114)
>> > @@ -5325,6 +5326,7 @@ enum {
>> > #define _DPB_AUX_CH_DATA3 (dev_priv->info.display_mmio_offset + 0x6411c)
>> > #define _DPB_AUX_CH_DATA4 (dev_priv->info.display_mmio_offset + 0x64120)
>> > #define _DPB_AUX_CH_DATA5 (dev_priv->info.display_mmio_offset + 0x64124)
>> > +#define _DPB_AUX_CH_MUTEX (dev_priv->info.display_mmio_offset + 0x6412C)
>> >
>> > #define _DPC_AUX_CH_CTL (dev_priv->info.display_mmio_offset +
>> > 0x64210)
>> > #define _DPC_AUX_CH_DATA1 (dev_priv->info.display_mmio_offset + 0x64214)
>> > @@ -5332,6 +5334,7 @@ enum {
>> > #define _DPC_AUX_CH_DATA3 (dev_priv->info.display_mmio_offset + 0x6421c)
>> > #define _DPC_AUX_CH_DATA4 (dev_priv->info.display_mmio_offset + 0x64220)
>> > #define _DPC_AUX_CH_DATA5 (dev_priv->info.display_mmio_offset + 0x64224)
>> > +#define _DPC_AUX_CH_MUTEX (dev_priv->info.display_mmio_offset + 0x6422C)
>> >
>> > #define _DPD_AUX_CH_CTL (dev_priv->info.display_mmio_offset +
>> > 0x64310)
>> > #define _DPD_AUX_CH_DATA1 (dev_priv->info.display_mmio_offset + 0x64314)
>> > @@ -5339,6 +5342,7 @@ enum {
>> > #define _DPD_AUX_CH_DATA3 (dev_priv->info.display_mmio_offset + 0x6431c)
>> > #define _DPD_AUX_CH_DATA4 (dev_priv->info.display_mmio_offset + 0x64320)
>> > #define _DPD_AUX_CH_DATA5 (dev_priv->info.display_mmio_offset + 0x64324)
>> > +#define _DPD_AUX_CH_MUTEX (dev_priv->info.display_mmio_offset + 0x6432C)
>> >
>> > #define _DPF_AUX_CH_CTL (dev_priv->info.display_mmio_offset +
>> > 0x64510)
>> > #define _DPF_AUX_CH_DATA1 (dev_priv->info.display_mmio_offset + 0x64514)
>> > @@ -5346,9 +5350,11 @@ enum {
>> > #define _DPF_AUX_CH_DATA3 (dev_priv->info.display_mmio_offset + 0x6451c)
>> > #define _DPF_AUX_CH_DATA4 (dev_priv->info.display_mmio_offset + 0x64520)
>> > #define _DPF_AUX_CH_DATA5 (dev_priv->info.display_mmio_offset + 0x64524)
>> > +#define _DPF_AUX_CH_MUTEX (dev_priv->info.display_mmio_offset + 0x6452C)
>> >
>> > #define DP_AUX_CH_CTL(port) _MMIO_PORT(port, _DPA_AUX_CH_CTL,
>> > _DPB_AUX_CH_CTL)
>> > #define DP_AUX_CH_DATA(port, i) _MMIO(_PORT(port, _DPA_AUX_CH_DATA1,
>> > _DPB_AUX_CH_DATA1) + (i) * 4) /* 5 registers */
>> > +#define DP_AUX_CH_MUTEX(port) _MMIO_PORT(port, _DPA_AUX_CH_MUTEX,
>> > _DPB_AUX_CH_MUTEX)
>> >
>> > #define DP_AUX_CH_CTL_SEND_BUSY (1 << 31)
>> > #define DP_AUX_CH_CTL_DONE (1 << 30)
>> > @@ -5378,6 +5384,9 @@ en
Re: [Intel-gfx] [PATCH 1/5] drm/i915: add and enable DP AUX CH mutex
On Wed, Feb 14, 2018 at 09:47:00AM +, Jani Nikula wrote:
> On Tue, 13 Feb 2018, José Roberto de Souza wrote:
> > When PSR/PSR2 is enabled hardware can do AUX transactions by it self, so
> > lets use the mutex register that is available in gen9+ to avoid concurrent
> > access by hardware and driver.
> >
> > Reference:
> > https://01.org/sites/default/files/documentation/intel-gfx-prm-osrc-skl-vol12-display.pdf
> > Page 198 - AUX programming sequence
> >
> > As gen < 9 don't have the mutex PSR needs to be disabled, that is why the
> > 'else TODO' was added. This is also backed by spec:
> > "DDI A AUX channel transactions must not be sent while SRD is enabled.
> > SRD must be completely disabled before a DDI A AUX channel transaction can
> > be sent." BSpec: 7530
>
> We explicitly rejected the use of the hw mutex way back when Skylake was
> enabled. At that time the understanding was that the hw will try to do
> the arbitration itself. Has something changed?
By that time spec wasn't clear about the actual need or its reliability.
Nowdays spec is clear that that is absolutely needed for PSR/PSR2/GTC.
Also we were using a lot of SW tracking and nowadays we are moving PSR
to use more HW tracking.
So I believe we should add this to get a stable PSR.
>
> Some comments inline.
>
> >
> > Cc: Rodrigo Vivi
> > Signed-off-by: José Roberto de Souza
> > ---
> > drivers/gpu/drm/i915/i915_reg.h | 9
> > drivers/gpu/drm/i915/intel_dp.c | 48
> >
> > drivers/gpu/drm/i915/intel_drv.h | 1 +
> > drivers/gpu/drm/i915/intel_psr.c | 6 +
> > 4 files changed, 64 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > b/drivers/gpu/drm/i915/i915_reg.h
> > index f6afa5e5e7c1..5d7736117cb9 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -5318,6 +5318,7 @@ enum {
> > #define _DPA_AUX_CH_DATA3 (dev_priv->info.display_mmio_offset + 0x6401c)
> > #define _DPA_AUX_CH_DATA4 (dev_priv->info.display_mmio_offset + 0x64020)
> > #define _DPA_AUX_CH_DATA5 (dev_priv->info.display_mmio_offset + 0x64024)
> > +#define _DPA_AUX_CH_MUTEX (dev_priv->info.display_mmio_offset + 0x6402C)
> >
> > #define _DPB_AUX_CH_CTL(dev_priv->info.display_mmio_offset +
> > 0x64110)
> > #define _DPB_AUX_CH_DATA1 (dev_priv->info.display_mmio_offset + 0x64114)
> > @@ -5325,6 +5326,7 @@ enum {
> > #define _DPB_AUX_CH_DATA3 (dev_priv->info.display_mmio_offset + 0x6411c)
> > #define _DPB_AUX_CH_DATA4 (dev_priv->info.display_mmio_offset + 0x64120)
> > #define _DPB_AUX_CH_DATA5 (dev_priv->info.display_mmio_offset + 0x64124)
> > +#define _DPB_AUX_CH_MUTEX (dev_priv->info.display_mmio_offset + 0x6412C)
> >
> > #define _DPC_AUX_CH_CTL(dev_priv->info.display_mmio_offset +
> > 0x64210)
> > #define _DPC_AUX_CH_DATA1 (dev_priv->info.display_mmio_offset + 0x64214)
> > @@ -5332,6 +5334,7 @@ enum {
> > #define _DPC_AUX_CH_DATA3 (dev_priv->info.display_mmio_offset + 0x6421c)
> > #define _DPC_AUX_CH_DATA4 (dev_priv->info.display_mmio_offset + 0x64220)
> > #define _DPC_AUX_CH_DATA5 (dev_priv->info.display_mmio_offset + 0x64224)
> > +#define _DPC_AUX_CH_MUTEX (dev_priv->info.display_mmio_offset + 0x6422C)
> >
> > #define _DPD_AUX_CH_CTL(dev_priv->info.display_mmio_offset +
> > 0x64310)
> > #define _DPD_AUX_CH_DATA1 (dev_priv->info.display_mmio_offset + 0x64314)
> > @@ -5339,6 +5342,7 @@ enum {
> > #define _DPD_AUX_CH_DATA3 (dev_priv->info.display_mmio_offset + 0x6431c)
> > #define _DPD_AUX_CH_DATA4 (dev_priv->info.display_mmio_offset + 0x64320)
> > #define _DPD_AUX_CH_DATA5 (dev_priv->info.display_mmio_offset + 0x64324)
> > +#define _DPD_AUX_CH_MUTEX (dev_priv->info.display_mmio_offset + 0x6432C)
> >
> > #define _DPF_AUX_CH_CTL(dev_priv->info.display_mmio_offset +
> > 0x64510)
> > #define _DPF_AUX_CH_DATA1 (dev_priv->info.display_mmio_offset + 0x64514)
> > @@ -5346,9 +5350,11 @@ enum {
> > #define _DPF_AUX_CH_DATA3 (dev_priv->info.display_mmio_offset + 0x6451c)
> > #define _DPF_AUX_CH_DATA4 (dev_priv->info.display_mmio_offset + 0x64520)
> > #define _DPF_AUX_CH_DATA5 (dev_priv->info.display_mmio_offset + 0x64524)
> > +#define _DPF_AUX_CH_MUTEX (dev_priv->info.display_mmio_offset + 0x6452C)
> >
> > #define DP_AUX_CH_CTL(port)_MMIO_PORT(port, _DPA_AUX_CH_CTL,
> > _DPB_AUX_CH_CTL)
> > #define DP_AUX_CH_DATA(port, i)_MMIO(_PORT(port, _DPA_AUX_CH_DATA1,
> > _DPB_AUX_CH_DATA1) + (i) * 4) /* 5 registers */
> > +#define DP_AUX_CH_MUTEX(port) _MMIO_PORT(port, _DPA_AUX_CH_MUTEX,
> > _DPB_AUX_CH_MUTEX)
> >
> > #define DP_AUX_CH_CTL_SEND_BUSY (1 << 31)
> > #define DP_AUX_CH_CTL_DONE (1 << 30)
> > @@ -5378,6 +5384,9 @@ enum {
> > #define DP_AUX_CH_CTL_FW_SYNC_PULSE_SKL(c) (((c) - 1) << 5)
> > #define DP_AUX_CH_CTL_SYNC_PULSE_SKL(c) ((c) - 1)
> >
> > +#define DP_AUX_CH_MUTEX_ENABLE (1 <
Re: [Intel-gfx] [PATCH 1/5] drm/i915: add and enable DP AUX CH mutex
On Tue, Feb 13, 2018 at 05:30:29PM -0800, Rodrigo Vivi wrote:
> José Roberto de Souza writes:
>
> > When PSR/PSR2 is enabled hardware can do AUX transactions by it self, so
> > lets use the mutex register that is available in gen9+ to avoid concurrent
> > access by hardware and driver.
> >
> > Reference:
> > https://01.org/sites/default/files/documentation/intel-gfx-prm-osrc-skl-vol12-display.pdf
> > Page 198 - AUX programming sequence
> >
> > As gen < 9 don't have the mutex PSR needs to be disabled, that is why the
> > 'else TODO' was added. This is also backed by spec:
> > "DDI A AUX channel transactions must not be sent while SRD is enabled.
> > SRD must be completely disabled before a DDI A AUX channel transaction can
> > be sent." BSpec: 7530
> >
> > Cc: Rodrigo Vivi
> > Signed-off-by: José Roberto de Souza
> > ---
> > drivers/gpu/drm/i915/i915_reg.h | 9
> > drivers/gpu/drm/i915/intel_dp.c | 48
> >
> > drivers/gpu/drm/i915/intel_drv.h | 1 +
> > drivers/gpu/drm/i915/intel_psr.c | 6 +
> > 4 files changed, 64 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > b/drivers/gpu/drm/i915/i915_reg.h
> > index f6afa5e5e7c1..5d7736117cb9 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -5318,6 +5318,7 @@ enum {
> > #define _DPA_AUX_CH_DATA3 (dev_priv->info.display_mmio_offset + 0x6401c)
> > #define _DPA_AUX_CH_DATA4 (dev_priv->info.display_mmio_offset + 0x64020)
> > #define _DPA_AUX_CH_DATA5 (dev_priv->info.display_mmio_offset + 0x64024)
> > +#define _DPA_AUX_CH_MUTEX (dev_priv->info.display_mmio_offset + 0x6402C)
> >
> > #define _DPB_AUX_CH_CTL(dev_priv->info.display_mmio_offset +
> > 0x64110)
> > #define _DPB_AUX_CH_DATA1 (dev_priv->info.display_mmio_offset + 0x64114)
> > @@ -5325,6 +5326,7 @@ enum {
> > #define _DPB_AUX_CH_DATA3 (dev_priv->info.display_mmio_offset + 0x6411c)
> > #define _DPB_AUX_CH_DATA4 (dev_priv->info.display_mmio_offset + 0x64120)
> > #define _DPB_AUX_CH_DATA5 (dev_priv->info.display_mmio_offset + 0x64124)
> > +#define _DPB_AUX_CH_MUTEX (dev_priv->info.display_mmio_offset + 0x6412C)
> >
> > #define _DPC_AUX_CH_CTL(dev_priv->info.display_mmio_offset +
> > 0x64210)
> > #define _DPC_AUX_CH_DATA1 (dev_priv->info.display_mmio_offset + 0x64214)
> > @@ -5332,6 +5334,7 @@ enum {
> > #define _DPC_AUX_CH_DATA3 (dev_priv->info.display_mmio_offset + 0x6421c)
> > #define _DPC_AUX_CH_DATA4 (dev_priv->info.display_mmio_offset + 0x64220)
> > #define _DPC_AUX_CH_DATA5 (dev_priv->info.display_mmio_offset + 0x64224)
> > +#define _DPC_AUX_CH_MUTEX (dev_priv->info.display_mmio_offset + 0x6422C)
> >
> > #define _DPD_AUX_CH_CTL(dev_priv->info.display_mmio_offset +
> > 0x64310)
> > #define _DPD_AUX_CH_DATA1 (dev_priv->info.display_mmio_offset + 0x64314)
> > @@ -5339,6 +5342,7 @@ enum {
> > #define _DPD_AUX_CH_DATA3 (dev_priv->info.display_mmio_offset + 0x6431c)
> > #define _DPD_AUX_CH_DATA4 (dev_priv->info.display_mmio_offset + 0x64320)
> > #define _DPD_AUX_CH_DATA5 (dev_priv->info.display_mmio_offset + 0x64324)
> > +#define _DPD_AUX_CH_MUTEX (dev_priv->info.display_mmio_offset + 0x6432C)
> >
> > #define _DPF_AUX_CH_CTL(dev_priv->info.display_mmio_offset +
> > 0x64510)
> > #define _DPF_AUX_CH_DATA1 (dev_priv->info.display_mmio_offset + 0x64514)
> > @@ -5346,9 +5350,11 @@ enum {
> > #define _DPF_AUX_CH_DATA3 (dev_priv->info.display_mmio_offset + 0x6451c)
> > #define _DPF_AUX_CH_DATA4 (dev_priv->info.display_mmio_offset + 0x64520)
> > #define _DPF_AUX_CH_DATA5 (dev_priv->info.display_mmio_offset + 0x64524)
> > +#define _DPF_AUX_CH_MUTEX (dev_priv->info.display_mmio_offset + 0x6452C)
> >
> > #define DP_AUX_CH_CTL(port)_MMIO_PORT(port, _DPA_AUX_CH_CTL,
> > _DPB_AUX_CH_CTL)
> > #define DP_AUX_CH_DATA(port, i)_MMIO(_PORT(port, _DPA_AUX_CH_DATA1,
> > _DPB_AUX_CH_DATA1) + (i) * 4) /* 5 registers */
> > +#define DP_AUX_CH_MUTEX(port) _MMIO_PORT(port, _DPA_AUX_CH_MUTEX,
> > _DPB_AUX_CH_MUTEX)
> >
> > #define DP_AUX_CH_CTL_SEND_BUSY (1 << 31)
> > #define DP_AUX_CH_CTL_DONE (1 << 30)
> > @@ -5378,6 +5384,9 @@ enum {
> > #define DP_AUX_CH_CTL_FW_SYNC_PULSE_SKL(c) (((c) - 1) << 5)
> > #define DP_AUX_CH_CTL_SYNC_PULSE_SKL(c) ((c) - 1)
> >
> > +#define DP_AUX_CH_MUTEX_ENABLE (1 << 31)
> > +#define DP_AUX_CH_MUTEX_STATUS (1 << 30)
> > +
> > /*
> > * Computing GMCH M and N values for the Display Port link
> > *
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > b/drivers/gpu/drm/i915/intel_dp.c
> > index f20b25f98e5a..bd5f1bb730ff 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -1081,6 +1081,28 @@ static uint32_t intel_dp_get_aux_send_ctl(struct
> > intel_dp *intel_dp,
> > aux_clock_divider);
> > }
>
Re: [Intel-gfx] [PATCH 1/5] drm/i915: add and enable DP AUX CH mutex
On Tue, 13 Feb 2018, José Roberto de Souza wrote:
> When PSR/PSR2 is enabled hardware can do AUX transactions by it self, so
> lets use the mutex register that is available in gen9+ to avoid concurrent
> access by hardware and driver.
>
> Reference:
> https://01.org/sites/default/files/documentation/intel-gfx-prm-osrc-skl-vol12-display.pdf
> Page 198 - AUX programming sequence
>
> As gen < 9 don't have the mutex PSR needs to be disabled, that is why the
> 'else TODO' was added. This is also backed by spec:
> "DDI A AUX channel transactions must not be sent while SRD is enabled.
> SRD must be completely disabled before a DDI A AUX channel transaction can
> be sent." BSpec: 7530
We explicitly rejected the use of the hw mutex way back when Skylake was
enabled. At that time the understanding was that the hw will try to do
the arbitration itself. Has something changed?
Some comments inline.
>
> Cc: Rodrigo Vivi
> Signed-off-by: José Roberto de Souza
> ---
> drivers/gpu/drm/i915/i915_reg.h | 9
> drivers/gpu/drm/i915/intel_dp.c | 48
>
> drivers/gpu/drm/i915/intel_drv.h | 1 +
> drivers/gpu/drm/i915/intel_psr.c | 6 +
> 4 files changed, 64 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index f6afa5e5e7c1..5d7736117cb9 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -5318,6 +5318,7 @@ enum {
> #define _DPA_AUX_CH_DATA3(dev_priv->info.display_mmio_offset + 0x6401c)
> #define _DPA_AUX_CH_DATA4(dev_priv->info.display_mmio_offset + 0x64020)
> #define _DPA_AUX_CH_DATA5(dev_priv->info.display_mmio_offset + 0x64024)
> +#define _DPA_AUX_CH_MUTEX(dev_priv->info.display_mmio_offset + 0x6402C)
>
> #define _DPB_AUX_CH_CTL (dev_priv->info.display_mmio_offset +
> 0x64110)
> #define _DPB_AUX_CH_DATA1(dev_priv->info.display_mmio_offset + 0x64114)
> @@ -5325,6 +5326,7 @@ enum {
> #define _DPB_AUX_CH_DATA3(dev_priv->info.display_mmio_offset + 0x6411c)
> #define _DPB_AUX_CH_DATA4(dev_priv->info.display_mmio_offset + 0x64120)
> #define _DPB_AUX_CH_DATA5(dev_priv->info.display_mmio_offset + 0x64124)
> +#define _DPB_AUX_CH_MUTEX(dev_priv->info.display_mmio_offset + 0x6412C)
>
> #define _DPC_AUX_CH_CTL (dev_priv->info.display_mmio_offset +
> 0x64210)
> #define _DPC_AUX_CH_DATA1(dev_priv->info.display_mmio_offset + 0x64214)
> @@ -5332,6 +5334,7 @@ enum {
> #define _DPC_AUX_CH_DATA3(dev_priv->info.display_mmio_offset + 0x6421c)
> #define _DPC_AUX_CH_DATA4(dev_priv->info.display_mmio_offset + 0x64220)
> #define _DPC_AUX_CH_DATA5(dev_priv->info.display_mmio_offset + 0x64224)
> +#define _DPC_AUX_CH_MUTEX(dev_priv->info.display_mmio_offset + 0x6422C)
>
> #define _DPD_AUX_CH_CTL (dev_priv->info.display_mmio_offset +
> 0x64310)
> #define _DPD_AUX_CH_DATA1(dev_priv->info.display_mmio_offset + 0x64314)
> @@ -5339,6 +5342,7 @@ enum {
> #define _DPD_AUX_CH_DATA3(dev_priv->info.display_mmio_offset + 0x6431c)
> #define _DPD_AUX_CH_DATA4(dev_priv->info.display_mmio_offset + 0x64320)
> #define _DPD_AUX_CH_DATA5(dev_priv->info.display_mmio_offset + 0x64324)
> +#define _DPD_AUX_CH_MUTEX(dev_priv->info.display_mmio_offset + 0x6432C)
>
> #define _DPF_AUX_CH_CTL (dev_priv->info.display_mmio_offset +
> 0x64510)
> #define _DPF_AUX_CH_DATA1(dev_priv->info.display_mmio_offset + 0x64514)
> @@ -5346,9 +5350,11 @@ enum {
> #define _DPF_AUX_CH_DATA3(dev_priv->info.display_mmio_offset + 0x6451c)
> #define _DPF_AUX_CH_DATA4(dev_priv->info.display_mmio_offset + 0x64520)
> #define _DPF_AUX_CH_DATA5(dev_priv->info.display_mmio_offset + 0x64524)
> +#define _DPF_AUX_CH_MUTEX(dev_priv->info.display_mmio_offset + 0x6452C)
>
> #define DP_AUX_CH_CTL(port) _MMIO_PORT(port, _DPA_AUX_CH_CTL,
> _DPB_AUX_CH_CTL)
> #define DP_AUX_CH_DATA(port, i) _MMIO(_PORT(port, _DPA_AUX_CH_DATA1,
> _DPB_AUX_CH_DATA1) + (i) * 4) /* 5 registers */
> +#define DP_AUX_CH_MUTEX(port)_MMIO_PORT(port, _DPA_AUX_CH_MUTEX,
> _DPB_AUX_CH_MUTEX)
>
> #define DP_AUX_CH_CTL_SEND_BUSY(1 << 31)
> #define DP_AUX_CH_CTL_DONE (1 << 30)
> @@ -5378,6 +5384,9 @@ enum {
> #define DP_AUX_CH_CTL_FW_SYNC_PULSE_SKL(c) (((c) - 1) << 5)
> #define DP_AUX_CH_CTL_SYNC_PULSE_SKL(c) ((c) - 1)
>
> +#define DP_AUX_CH_MUTEX_ENABLE (1 << 31)
> +#define DP_AUX_CH_MUTEX_STATUS (1 << 30)
The bit definitions should immediately follow the register offset
definition. I.e. DP_AUX_CH_MUTEX() should be right above these.
> +
> /*
> * Computing GMCH M and N values for the Display Port link
> *
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index f20b25f98e5a..bd5f1bb730ff 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1081,6 +1081
Re: [Intel-gfx] [PATCH 1/5] drm/i915: add and enable DP AUX CH mutex
On Tue, 13 Feb 2018, José Roberto de Souza wrote:
> When PSR/PSR2 is enabled hardware can do AUX transactions by it self, so
> lets use the mutex register that is available in gen9+ to avoid concurrent
> access by hardware and driver.
>
> Reference:
> https://01.org/sites/default/files/documentation/intel-gfx-prm-osrc-skl-vol12-display.pdf
> Page 198 - AUX programming sequence
>
> As gen < 9 don't have the mutex PSR needs to be disabled, that is why the
> 'else TODO' was added. This is also backed by spec:
And proves the patches weren't compiled not to mention tested. :(
BR,
Jani.
> "DDI A AUX channel transactions must not be sent while SRD is enabled.
> SRD must be completely disabled before a DDI A AUX channel transaction can
> be sent." BSpec: 7530
>
> Cc: Rodrigo Vivi
> Signed-off-by: José Roberto de Souza
> ---
> drivers/gpu/drm/i915/i915_reg.h | 9
> drivers/gpu/drm/i915/intel_dp.c | 48
>
> drivers/gpu/drm/i915/intel_drv.h | 1 +
> drivers/gpu/drm/i915/intel_psr.c | 6 +
> 4 files changed, 64 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index f6afa5e5e7c1..5d7736117cb9 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -5318,6 +5318,7 @@ enum {
> #define _DPA_AUX_CH_DATA3(dev_priv->info.display_mmio_offset + 0x6401c)
> #define _DPA_AUX_CH_DATA4(dev_priv->info.display_mmio_offset + 0x64020)
> #define _DPA_AUX_CH_DATA5(dev_priv->info.display_mmio_offset + 0x64024)
> +#define _DPA_AUX_CH_MUTEX(dev_priv->info.display_mmio_offset + 0x6402C)
>
> #define _DPB_AUX_CH_CTL (dev_priv->info.display_mmio_offset +
> 0x64110)
> #define _DPB_AUX_CH_DATA1(dev_priv->info.display_mmio_offset + 0x64114)
> @@ -5325,6 +5326,7 @@ enum {
> #define _DPB_AUX_CH_DATA3(dev_priv->info.display_mmio_offset + 0x6411c)
> #define _DPB_AUX_CH_DATA4(dev_priv->info.display_mmio_offset + 0x64120)
> #define _DPB_AUX_CH_DATA5(dev_priv->info.display_mmio_offset + 0x64124)
> +#define _DPB_AUX_CH_MUTEX(dev_priv->info.display_mmio_offset + 0x6412C)
>
> #define _DPC_AUX_CH_CTL (dev_priv->info.display_mmio_offset +
> 0x64210)
> #define _DPC_AUX_CH_DATA1(dev_priv->info.display_mmio_offset + 0x64214)
> @@ -5332,6 +5334,7 @@ enum {
> #define _DPC_AUX_CH_DATA3(dev_priv->info.display_mmio_offset + 0x6421c)
> #define _DPC_AUX_CH_DATA4(dev_priv->info.display_mmio_offset + 0x64220)
> #define _DPC_AUX_CH_DATA5(dev_priv->info.display_mmio_offset + 0x64224)
> +#define _DPC_AUX_CH_MUTEX(dev_priv->info.display_mmio_offset + 0x6422C)
>
> #define _DPD_AUX_CH_CTL (dev_priv->info.display_mmio_offset +
> 0x64310)
> #define _DPD_AUX_CH_DATA1(dev_priv->info.display_mmio_offset + 0x64314)
> @@ -5339,6 +5342,7 @@ enum {
> #define _DPD_AUX_CH_DATA3(dev_priv->info.display_mmio_offset + 0x6431c)
> #define _DPD_AUX_CH_DATA4(dev_priv->info.display_mmio_offset + 0x64320)
> #define _DPD_AUX_CH_DATA5(dev_priv->info.display_mmio_offset + 0x64324)
> +#define _DPD_AUX_CH_MUTEX(dev_priv->info.display_mmio_offset + 0x6432C)
>
> #define _DPF_AUX_CH_CTL (dev_priv->info.display_mmio_offset +
> 0x64510)
> #define _DPF_AUX_CH_DATA1(dev_priv->info.display_mmio_offset + 0x64514)
> @@ -5346,9 +5350,11 @@ enum {
> #define _DPF_AUX_CH_DATA3(dev_priv->info.display_mmio_offset + 0x6451c)
> #define _DPF_AUX_CH_DATA4(dev_priv->info.display_mmio_offset + 0x64520)
> #define _DPF_AUX_CH_DATA5(dev_priv->info.display_mmio_offset + 0x64524)
> +#define _DPF_AUX_CH_MUTEX(dev_priv->info.display_mmio_offset + 0x6452C)
>
> #define DP_AUX_CH_CTL(port) _MMIO_PORT(port, _DPA_AUX_CH_CTL,
> _DPB_AUX_CH_CTL)
> #define DP_AUX_CH_DATA(port, i) _MMIO(_PORT(port, _DPA_AUX_CH_DATA1,
> _DPB_AUX_CH_DATA1) + (i) * 4) /* 5 registers */
> +#define DP_AUX_CH_MUTEX(port)_MMIO_PORT(port, _DPA_AUX_CH_MUTEX,
> _DPB_AUX_CH_MUTEX)
>
> #define DP_AUX_CH_CTL_SEND_BUSY(1 << 31)
> #define DP_AUX_CH_CTL_DONE (1 << 30)
> @@ -5378,6 +5384,9 @@ enum {
> #define DP_AUX_CH_CTL_FW_SYNC_PULSE_SKL(c) (((c) - 1) << 5)
> #define DP_AUX_CH_CTL_SYNC_PULSE_SKL(c) ((c) - 1)
>
> +#define DP_AUX_CH_MUTEX_ENABLE (1 << 31)
> +#define DP_AUX_CH_MUTEX_STATUS (1 << 30)
> +
> /*
> * Computing GMCH M and N values for the Display Port link
> *
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index f20b25f98e5a..bd5f1bb730ff 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1081,6 +1081,28 @@ static uint32_t intel_dp_get_aux_send_ctl(struct
> intel_dp *intel_dp,
> aux_clock_divider);
> }
>
> +static bool intel_dp_aux_get(struct drm_i915_private *dev_priv,
> + struct intel_dp
Re: [Intel-gfx] [PATCH 1/5] drm/i915: add and enable DP AUX CH mutex
On Tue, 2018-02-13 at 17:30 -0800, Rodrigo Vivi wrote:
> José Roberto de Souza writes:
>
> > When PSR/PSR2 is enabled hardware can do AUX transactions by it
> > self, so
> > lets use the mutex register that is available in gen9+ to avoid
> > concurrent
> > access by hardware and driver.
> >
> > Reference: https://01.org/sites/default/files/documentation/intel-g
> > fx-prm-osrc-skl-vol12-display.pdf
> > Page 198 - AUX programming sequence
> >
> > As gen < 9 don't have the mutex PSR needs to be disabled, that is
> > why the
> > 'else TODO' was added. This is also backed by spec:
> > "DDI A AUX channel transactions must not be sent while SRD is
> > enabled.
> > SRD must be completely disabled before a DDI A AUX channel
> > transaction can
> > be sent." BSpec: 7530
> >
> > Cc: Rodrigo Vivi
> > Signed-off-by: José Roberto de Souza
> > ---
> > drivers/gpu/drm/i915/i915_reg.h | 9
> > drivers/gpu/drm/i915/intel_dp.c | 48
> >
> > drivers/gpu/drm/i915/intel_drv.h | 1 +
> > drivers/gpu/drm/i915/intel_psr.c | 6 +
> > 4 files changed, 64 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > b/drivers/gpu/drm/i915/i915_reg.h
> > index f6afa5e5e7c1..5d7736117cb9 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -5318,6 +5318,7 @@ enum {
> > #define _DPA_AUX_CH_DATA3 (dev_priv-
> > >info.display_mmio_offset + 0x6401c)
> > #define _DPA_AUX_CH_DATA4 (dev_priv-
> > >info.display_mmio_offset + 0x64020)
> > #define _DPA_AUX_CH_DATA5 (dev_priv-
> > >info.display_mmio_offset + 0x64024)
> > +#define _DPA_AUX_CH_MUTEX (dev_priv-
> > >info.display_mmio_offset + 0x6402C)
> >
> > #define _DPB_AUX_CH_CTL(dev_priv-
> > >info.display_mmio_offset + 0x64110)
> > #define _DPB_AUX_CH_DATA1 (dev_priv-
> > >info.display_mmio_offset + 0x64114)
> > @@ -5325,6 +5326,7 @@ enum {
> > #define _DPB_AUX_CH_DATA3 (dev_priv-
> > >info.display_mmio_offset + 0x6411c)
> > #define _DPB_AUX_CH_DATA4 (dev_priv-
> > >info.display_mmio_offset + 0x64120)
> > #define _DPB_AUX_CH_DATA5 (dev_priv-
> > >info.display_mmio_offset + 0x64124)
> > +#define _DPB_AUX_CH_MUTEX (dev_priv-
> > >info.display_mmio_offset + 0x6412C)
> >
> > #define _DPC_AUX_CH_CTL(dev_priv-
> > >info.display_mmio_offset + 0x64210)
> > #define _DPC_AUX_CH_DATA1 (dev_priv-
> > >info.display_mmio_offset + 0x64214)
> > @@ -5332,6 +5334,7 @@ enum {
> > #define _DPC_AUX_CH_DATA3 (dev_priv-
> > >info.display_mmio_offset + 0x6421c)
> > #define _DPC_AUX_CH_DATA4 (dev_priv-
> > >info.display_mmio_offset + 0x64220)
> > #define _DPC_AUX_CH_DATA5 (dev_priv-
> > >info.display_mmio_offset + 0x64224)
> > +#define _DPC_AUX_CH_MUTEX (dev_priv-
> > >info.display_mmio_offset + 0x6422C)
> >
> > #define _DPD_AUX_CH_CTL(dev_priv-
> > >info.display_mmio_offset + 0x64310)
> > #define _DPD_AUX_CH_DATA1 (dev_priv-
> > >info.display_mmio_offset + 0x64314)
> > @@ -5339,6 +5342,7 @@ enum {
> > #define _DPD_AUX_CH_DATA3 (dev_priv-
> > >info.display_mmio_offset + 0x6431c)
> > #define _DPD_AUX_CH_DATA4 (dev_priv-
> > >info.display_mmio_offset + 0x64320)
> > #define _DPD_AUX_CH_DATA5 (dev_priv-
> > >info.display_mmio_offset + 0x64324)
> > +#define _DPD_AUX_CH_MUTEX (dev_priv-
> > >info.display_mmio_offset + 0x6432C)
> >
> > #define _DPF_AUX_CH_CTL(dev_priv-
> > >info.display_mmio_offset + 0x64510)
> > #define _DPF_AUX_CH_DATA1 (dev_priv-
> > >info.display_mmio_offset + 0x64514)
> > @@ -5346,9 +5350,11 @@ enum {
> > #define _DPF_AUX_CH_DATA3 (dev_priv-
> > >info.display_mmio_offset + 0x6451c)
> > #define _DPF_AUX_CH_DATA4 (dev_priv-
> > >info.display_mmio_offset + 0x64520)
> > #define _DPF_AUX_CH_DATA5 (dev_priv-
> > >info.display_mmio_offset + 0x64524)
> > +#define _DPF_AUX_CH_MUTEX (dev_priv-
> > >info.display_mmio_offset + 0x6452C)
> >
> > #define DP_AUX_CH_CTL(port)_MMIO_PORT(port,
> > _DPA_AUX_CH_CTL, _DPB_AUX_CH_CTL)
> > #define DP_AUX_CH_DATA(port, i)_MMIO(_PORT(port,
> > _DPA_AUX_CH_DATA1, _DPB_AUX_CH_DATA1) + (i) * 4) /* 5 registers */
> > +#define DP_AUX_CH_MUTEX(port) _MMIO_PORT(port,
> > _DPA_AUX_CH_MUTEX, _DPB_AUX_CH_MUTEX)
> >
> > #define DP_AUX_CH_CTL_SEND_BUSY (1 << 31)
> > #define DP_AUX_CH_CTL_DONE (1 << 30)
> > @@ -5378,6 +5384,9 @@ enum {
> > #define DP_AUX_CH_CTL_FW_SYNC_PULSE_SKL(c) (((c) - 1) << 5)
> > #define DP_AUX_CH_CTL_SYNC_PULSE_SKL(c) ((c) - 1)
> >
> > +#define DP_AUX_CH_MUTEX_ENABLE (1 << 31)
> > +#define DP_AUX_CH_MUTEX_STATUS (1 << 30)
> > +
> > /*
> > * Computing GMCH M and N values for the Display Port link
> > *
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > b/drivers/gpu/drm/i915/intel_dp.c
> > index f20b25f98e5a..bd5f1bb730ff 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -1081,6 +1081,28 @@ static uint32_t
> >
Re: [Intel-gfx] [PATCH 1/5] drm/i915: add and enable DP AUX CH mutex
José Roberto de Souza writes:
> When PSR/PSR2 is enabled hardware can do AUX transactions by it self, so
> lets use the mutex register that is available in gen9+ to avoid concurrent
> access by hardware and driver.
>
> Reference:
> https://01.org/sites/default/files/documentation/intel-gfx-prm-osrc-skl-vol12-display.pdf
> Page 198 - AUX programming sequence
>
> As gen < 9 don't have the mutex PSR needs to be disabled, that is why the
> 'else TODO' was added. This is also backed by spec:
> "DDI A AUX channel transactions must not be sent while SRD is enabled.
> SRD must be completely disabled before a DDI A AUX channel transaction can
> be sent." BSpec: 7530
>
> Cc: Rodrigo Vivi
> Signed-off-by: José Roberto de Souza
> ---
> drivers/gpu/drm/i915/i915_reg.h | 9
> drivers/gpu/drm/i915/intel_dp.c | 48
>
> drivers/gpu/drm/i915/intel_drv.h | 1 +
> drivers/gpu/drm/i915/intel_psr.c | 6 +
> 4 files changed, 64 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index f6afa5e5e7c1..5d7736117cb9 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -5318,6 +5318,7 @@ enum {
> #define _DPA_AUX_CH_DATA3(dev_priv->info.display_mmio_offset + 0x6401c)
> #define _DPA_AUX_CH_DATA4(dev_priv->info.display_mmio_offset + 0x64020)
> #define _DPA_AUX_CH_DATA5(dev_priv->info.display_mmio_offset + 0x64024)
> +#define _DPA_AUX_CH_MUTEX(dev_priv->info.display_mmio_offset + 0x6402C)
>
> #define _DPB_AUX_CH_CTL (dev_priv->info.display_mmio_offset +
> 0x64110)
> #define _DPB_AUX_CH_DATA1(dev_priv->info.display_mmio_offset + 0x64114)
> @@ -5325,6 +5326,7 @@ enum {
> #define _DPB_AUX_CH_DATA3(dev_priv->info.display_mmio_offset + 0x6411c)
> #define _DPB_AUX_CH_DATA4(dev_priv->info.display_mmio_offset + 0x64120)
> #define _DPB_AUX_CH_DATA5(dev_priv->info.display_mmio_offset + 0x64124)
> +#define _DPB_AUX_CH_MUTEX(dev_priv->info.display_mmio_offset + 0x6412C)
>
> #define _DPC_AUX_CH_CTL (dev_priv->info.display_mmio_offset +
> 0x64210)
> #define _DPC_AUX_CH_DATA1(dev_priv->info.display_mmio_offset + 0x64214)
> @@ -5332,6 +5334,7 @@ enum {
> #define _DPC_AUX_CH_DATA3(dev_priv->info.display_mmio_offset + 0x6421c)
> #define _DPC_AUX_CH_DATA4(dev_priv->info.display_mmio_offset + 0x64220)
> #define _DPC_AUX_CH_DATA5(dev_priv->info.display_mmio_offset + 0x64224)
> +#define _DPC_AUX_CH_MUTEX(dev_priv->info.display_mmio_offset + 0x6422C)
>
> #define _DPD_AUX_CH_CTL (dev_priv->info.display_mmio_offset +
> 0x64310)
> #define _DPD_AUX_CH_DATA1(dev_priv->info.display_mmio_offset + 0x64314)
> @@ -5339,6 +5342,7 @@ enum {
> #define _DPD_AUX_CH_DATA3(dev_priv->info.display_mmio_offset + 0x6431c)
> #define _DPD_AUX_CH_DATA4(dev_priv->info.display_mmio_offset + 0x64320)
> #define _DPD_AUX_CH_DATA5(dev_priv->info.display_mmio_offset + 0x64324)
> +#define _DPD_AUX_CH_MUTEX(dev_priv->info.display_mmio_offset + 0x6432C)
>
> #define _DPF_AUX_CH_CTL (dev_priv->info.display_mmio_offset +
> 0x64510)
> #define _DPF_AUX_CH_DATA1(dev_priv->info.display_mmio_offset + 0x64514)
> @@ -5346,9 +5350,11 @@ enum {
> #define _DPF_AUX_CH_DATA3(dev_priv->info.display_mmio_offset + 0x6451c)
> #define _DPF_AUX_CH_DATA4(dev_priv->info.display_mmio_offset + 0x64520)
> #define _DPF_AUX_CH_DATA5(dev_priv->info.display_mmio_offset + 0x64524)
> +#define _DPF_AUX_CH_MUTEX(dev_priv->info.display_mmio_offset + 0x6452C)
>
> #define DP_AUX_CH_CTL(port) _MMIO_PORT(port, _DPA_AUX_CH_CTL,
> _DPB_AUX_CH_CTL)
> #define DP_AUX_CH_DATA(port, i) _MMIO(_PORT(port, _DPA_AUX_CH_DATA1,
> _DPB_AUX_CH_DATA1) + (i) * 4) /* 5 registers */
> +#define DP_AUX_CH_MUTEX(port)_MMIO_PORT(port, _DPA_AUX_CH_MUTEX,
> _DPB_AUX_CH_MUTEX)
>
> #define DP_AUX_CH_CTL_SEND_BUSY(1 << 31)
> #define DP_AUX_CH_CTL_DONE (1 << 30)
> @@ -5378,6 +5384,9 @@ enum {
> #define DP_AUX_CH_CTL_FW_SYNC_PULSE_SKL(c) (((c) - 1) << 5)
> #define DP_AUX_CH_CTL_SYNC_PULSE_SKL(c) ((c) - 1)
>
> +#define DP_AUX_CH_MUTEX_ENABLE (1 << 31)
> +#define DP_AUX_CH_MUTEX_STATUS (1 << 30)
> +
> /*
> * Computing GMCH M and N values for the Display Port link
> *
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index f20b25f98e5a..bd5f1bb730ff 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1081,6 +1081,28 @@ static uint32_t intel_dp_get_aux_send_ctl(struct
> intel_dp *intel_dp,
> aux_clock_divider);
> }
>
> +static bool intel_dp_aux_get(struct drm_i915_private *dev_priv,
> + struct intel_dp *intel_dp)
> +{
> + int try, ret;
Here you are not getting the aux. You are only getting check
