On Tue, Feb 13, 2018 at 05:30:29PM -0800, Rodrigo Vivi wrote: > José Roberto de Souza <jose.souza@xxxxxxxxx> writes: > > > When PSR/PSR2 is enabled hardware can do AUX transactions by it self, so > > lets use the mutex register that is available in gen9+ to avoid concurrent > > access by hardware and driver. > > > > Reference: https://01.org/sites/default/files/documentation/intel-gfx-prm-osrc-skl-vol12-display.pdf > > Page 198 - AUX programming sequence > > > > As gen < 9 don't have the mutex PSR needs to be disabled, that is why the > > 'else TODO' was added. This is also backed by spec: > > "DDI A AUX channel transactions must not be sent while SRD is enabled. > > SRD must be completely disabled before a DDI A AUX channel transaction can > > be sent." BSpec: 7530 > > > > Cc: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx> > > Signed-off-by: José Roberto de Souza <jose.souza@xxxxxxxxx> > > --- > > drivers/gpu/drm/i915/i915_reg.h | 9 ++++++++ > > drivers/gpu/drm/i915/intel_dp.c | 48 ++++++++++++++++++++++++++++++++++++++++ > > drivers/gpu/drm/i915/intel_drv.h | 1 + > > drivers/gpu/drm/i915/intel_psr.c | 6 +++++ > > 4 files changed, 64 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > > index f6afa5e5e7c1..5d7736117cb9 100644 > > --- a/drivers/gpu/drm/i915/i915_reg.h > > +++ b/drivers/gpu/drm/i915/i915_reg.h > > @@ -5318,6 +5318,7 @@ enum { > > #define _DPA_AUX_CH_DATA3 (dev_priv->info.display_mmio_offset + 0x6401c) > > #define _DPA_AUX_CH_DATA4 (dev_priv->info.display_mmio_offset + 0x64020) > > #define _DPA_AUX_CH_DATA5 (dev_priv->info.display_mmio_offset + 0x64024) > > +#define _DPA_AUX_CH_MUTEX (dev_priv->info.display_mmio_offset + 0x6402C) > > > > #define _DPB_AUX_CH_CTL (dev_priv->info.display_mmio_offset + 0x64110) > > #define _DPB_AUX_CH_DATA1 (dev_priv->info.display_mmio_offset + 0x64114) > > @@ -5325,6 +5326,7 @@ enum { > > #define _DPB_AUX_CH_DATA3 (dev_priv->info.display_mmio_offset + 0x6411c) > > #define _DPB_AUX_CH_DATA4 (dev_priv->info.display_mmio_offset + 0x64120) > > #define _DPB_AUX_CH_DATA5 (dev_priv->info.display_mmio_offset + 0x64124) > > +#define _DPB_AUX_CH_MUTEX (dev_priv->info.display_mmio_offset + 0x6412C) > > > > #define _DPC_AUX_CH_CTL (dev_priv->info.display_mmio_offset + 0x64210) > > #define _DPC_AUX_CH_DATA1 (dev_priv->info.display_mmio_offset + 0x64214) > > @@ -5332,6 +5334,7 @@ enum { > > #define _DPC_AUX_CH_DATA3 (dev_priv->info.display_mmio_offset + 0x6421c) > > #define _DPC_AUX_CH_DATA4 (dev_priv->info.display_mmio_offset + 0x64220) > > #define _DPC_AUX_CH_DATA5 (dev_priv->info.display_mmio_offset + 0x64224) > > +#define _DPC_AUX_CH_MUTEX (dev_priv->info.display_mmio_offset + 0x6422C) > > > > #define _DPD_AUX_CH_CTL (dev_priv->info.display_mmio_offset + 0x64310) > > #define _DPD_AUX_CH_DATA1 (dev_priv->info.display_mmio_offset + 0x64314) > > @@ -5339,6 +5342,7 @@ enum { > > #define _DPD_AUX_CH_DATA3 (dev_priv->info.display_mmio_offset + 0x6431c) > > #define _DPD_AUX_CH_DATA4 (dev_priv->info.display_mmio_offset + 0x64320) > > #define _DPD_AUX_CH_DATA5 (dev_priv->info.display_mmio_offset + 0x64324) > > +#define _DPD_AUX_CH_MUTEX (dev_priv->info.display_mmio_offset + 0x6432C) > > > > #define _DPF_AUX_CH_CTL (dev_priv->info.display_mmio_offset + 0x64510) > > #define _DPF_AUX_CH_DATA1 (dev_priv->info.display_mmio_offset + 0x64514) > > @@ -5346,9 +5350,11 @@ enum { > > #define _DPF_AUX_CH_DATA3 (dev_priv->info.display_mmio_offset + 0x6451c) > > #define _DPF_AUX_CH_DATA4 (dev_priv->info.display_mmio_offset + 0x64520) > > #define _DPF_AUX_CH_DATA5 (dev_priv->info.display_mmio_offset + 0x64524) > > +#define _DPF_AUX_CH_MUTEX (dev_priv->info.display_mmio_offset + 0x6452C) > > > > #define DP_AUX_CH_CTL(port) _MMIO_PORT(port, _DPA_AUX_CH_CTL, _DPB_AUX_CH_CTL) > > #define DP_AUX_CH_DATA(port, i) _MMIO(_PORT(port, _DPA_AUX_CH_DATA1, _DPB_AUX_CH_DATA1) + (i) * 4) /* 5 registers */ > > +#define DP_AUX_CH_MUTEX(port) _MMIO_PORT(port, _DPA_AUX_CH_MUTEX, _DPB_AUX_CH_MUTEX) > > > > #define DP_AUX_CH_CTL_SEND_BUSY (1 << 31) > > #define DP_AUX_CH_CTL_DONE (1 << 30) > > @@ -5378,6 +5384,9 @@ enum { > > #define DP_AUX_CH_CTL_FW_SYNC_PULSE_SKL(c) (((c) - 1) << 5) > > #define DP_AUX_CH_CTL_SYNC_PULSE_SKL(c) ((c) - 1) > > > > +#define DP_AUX_CH_MUTEX_ENABLE (1 << 31) > > +#define DP_AUX_CH_MUTEX_STATUS (1 << 30) > > + > > /* > > * Computing GMCH M and N values for the Display Port link > > * > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > > index f20b25f98e5a..bd5f1bb730ff 100644 > > --- a/drivers/gpu/drm/i915/intel_dp.c > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > @@ -1081,6 +1081,28 @@ static uint32_t intel_dp_get_aux_send_ctl(struct intel_dp *intel_dp, > > aux_clock_divider); > > } > > > > +static bool intel_dp_aux_get(struct drm_i915_private *dev_priv, > > + struct intel_dp *intel_dp) > > +{ > > + int try, ret; > > Here you are not getting the aux. You are only getting checking if aux > is available. > My understanding of the spec is that the actual get is set Enable bit. > > > + > > + for (try = 0, ret = -1; try < 3 && ret; try++) > > + ret = wait_for(!(I915_READ_NOTRACE(intel_dp->aux_ch_mutex_reg) > > + & DP_AUX_CH_MUTEX_STATUS), 0.5); > > what about > intel_wait_for_register(dev_priv, intel_dp->aux_ch_mutex_reg, > DP_AUX_CH_MUTEX_STATUS, 0, > 500) IIRC when I asked Art about the mutex he indicated that the hardware has no magic fair arbitration mechanism. So I think what we want to do is first check if the mutex is free, if not we could try to wait for the expected 500us (or poll somewhat infrequently), but if after a while the mutex still isn't free we should probably start hammering it hard so that have a better chance of actually getting it. > ? > > > + > > + return ret == 0; > > +} > > + > > +static void intel_dp_aux_put(struct drm_i915_private *dev_priv, > > + struct intel_dp *intel_dp) > > +{ > > + /* setting the status bit releases the mutex + keep set the bit > > + * to keep the mutex enabled. > > + */ > > + I915_WRITE(intel_dp->aux_ch_mutex_reg, DP_AUX_CH_MUTEX_ENABLE | > > The enable bit is optional here. > I believe it gets cleaner without it but with RMW. > > > + DP_AUX_CH_MUTEX_STATUS); > > +} > > + > > static int > > intel_dp_aux_ch(struct intel_dp *intel_dp, > > const uint8_t *send, int send_bytes, > > @@ -1115,6 +1137,21 @@ intel_dp_aux_ch(struct intel_dp *intel_dp, > > > > intel_dp_check_edp(intel_dp); > > > > + /* Get hardware mutex when PSR is enabled */ > > + if (dev_priv->psr.enabled == intel_dp) { > > + if (intel_dp->aux_ch_mutex_reg.reg) { > > these checks could be inside get function. > > > + if (intel_dp_aux_get(dev_priv, intel_dp)) { > > + DRM_WARN("dp_aux_ch port locked for too > > long"); > > as well the message DRM_ERROR is enough I thing. > > > + ret = -EBUSY; > also specific errno > > > + goto out_locked; > > + } > > + } else { > > + /* TODO: go to PSR exit state waiting for change before > > + * do a aux ch transaction. > > + */ > > + } > > + } This looks needlessly complicated. IMO it should just be something like: if (!skl_aux_hw_mutex_get()) goto fail; And I'm thinking we should just ignore the PSR on vs. off case, and just do this for eDP always. I can't recall if GTC is a thing for normak DP, but if it is and we ever enable it then we'd obviously need to start doing this for all DP port. > > + > > /* Try to wait for any previous AUX channel activity */ > > for (try = 0; try < 3; try++) { > > status = I915_READ_NOTRACE(ch_ctl); > > @@ -1243,7 +1280,16 @@ intel_dp_aux_ch(struct intel_dp *intel_dp, > > recv + i, recv_bytes - i); > > > > ret = recv_bytes; > > + > > out: > > + if (dev_priv->psr.enabled == intel_dp) { > > we are checking psr.enable out of psr mutexes... > probably better to only do this once on get part > and if you got you put... > > > + if (intel_dp->aux_ch_mutex_reg.reg) > > + intel_dp_aux_put(dev_priv, intel_dp); > > + else > > + /* TODO: schedule PSR work to active PSR again */ > > + } > > + > > +out_locked: > > pm_qos_update_request(&dev_priv->pm_qos, PM_QOS_DEFAULT_VALUE); > > > > if (vdd) > > @@ -1496,6 +1542,8 @@ static void intel_aux_reg_init(struct intel_dp *intel_dp) > > intel_dp->aux_ch_ctl_reg = intel_aux_ctl_reg(dev_priv, port); > > for (i = 0; i < ARRAY_SIZE(intel_dp->aux_ch_data_reg); i++) > > intel_dp->aux_ch_data_reg[i] = intel_aux_data_reg(dev_priv, port, i); > > + if (INTEL_INFO(dev_priv)->gen >= 9) > > + intel_dp->aux_ch_mutex_reg = DP_AUX_CH_MUTEX(port); No point in adding this thing IMO. > > } > > > > static void > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > > index 898064e8bea7..0d235b7dab21 100644 > > --- a/drivers/gpu/drm/i915/intel_drv.h > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > @@ -1040,6 +1040,7 @@ struct intel_dp { > > i915_reg_t output_reg; > > i915_reg_t aux_ch_ctl_reg; > > i915_reg_t aux_ch_data_reg[5]; > > + i915_reg_t aux_ch_mutex_reg; > > uint32_t DP; > > int link_rate; > > uint8_t lane_count; > > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c > > index 2ef374f936b9..8b456e4e1b47 100644 > > --- a/drivers/gpu/drm/i915/intel_psr.c > > +++ b/drivers/gpu/drm/i915/intel_psr.c > > @@ -484,6 +484,9 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp, > > EDP_PSR_DEBUG_MASK_HPD | > > EDP_PSR_DEBUG_MASK_LPSP); > > } > > + > > + if (intel_dp->aux_ch_mutex_reg.reg) > > + I915_WRITE(intel_dp->aux_ch_mutex_reg, > > DP_AUX_CH_MUTEX_ENABLE); > > My understanding is that set Enable is the actual get hw mutex, > so I don't believe you want do do this here. The register read is the 'get'. But I also think we should just set the enable bit in the _get() function as well. I don't think there should be any harm if we just leave the enable bit set permenently after that. > > Also you already check for psr.enable inside the aux transaction. > setting there should be enough. > > > } > > > > /** > > @@ -617,6 +620,9 @@ static void hsw_psr_disable(struct intel_dp *intel_dp, > > else > > WARN_ON(I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE); > > } > > + > > + if (intel_dp->aux_ch_mutex_reg.reg) > > + I915_WRITE(intel_dp->aux_ch_mutex_reg, ~DP_AUX_CH_MUTEX_ENABLE); > > } > > > > /** > > -- > > 2.16.1 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx > https://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ville Syrjälä Intel OTC _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
